#2528 Missed-payments streak issue

[SebinSong]

closes #2528

I can see it now shows 1 missed-payment items too.

[SebinSong]

@taoeffect The PR is ready besides the heisen-bug in group-chat-direct-message.spec.js
(The spec passes fine when running the test locally)

[taoeffect]

@SebinSong It seems this isn't a heisenbug, but a bug that we need to fix. It might be as a result of recent merges to master. My guess is it might have to do with recent changes to ChatMain. Let me know if you have any ideas!

[taoeffect]

Nice work @SebinSong. The tests are failing because of a bug in ChatMain that was introduced in a recent PR, so none of these failing builds are related to this PR.

Please revert the changes to the strings, and this PR should be good to merge.

[cypress]

group-income    Run #3831

Run Properties:   Passed #3831  •  856b7e2f8d ℹ️: Merge a04fafdf1b13eed32111752200af9551c1cca70d into 34156c8d373c0d04c25fcbd66e3c...

Project	group-income
Branch Review	sebin/task/#2528-missed-payments-streak-issue
Run status	Passed #3831
Run duration	11m 13s
Commit	856b7e2f8d ℹ️: Merge a04fafdf1b13eed32111752200af9551c1cca70d into 34156c8d373c0d04c25fcbd66e3c...
Committer	Sebin Song
View all properties for this run ↗︎

---

Test results
Failures	0
Flaky	0
Pending	10
Skipped	0
Passing	112
View all changes introduced in this branch ↗︎

[taoeffect]

So I tested this locally with GI_PERSIST=sqlite grunt dev and this didn't seem to fix the problem when testing with our group's database.

One other note, if you run into issues running grunt dev like this:

Running "esbuild:watch" (esbuild) task
✘ [ERROR] Invalid define value (must be an entity name or valid JSON syntax): 'v1.2.1-13-ga04fafdf1
'

✘ [ERROR] Invalid define value (must be an entity name or valid JSON syntax): 'v1.2.1-13-ga04fafdf1
'

The solution is to modify the Gruntfile to replace:

const GI_GIT_VERSION = process.env.CI ? process.env.GI_VERSION : execSync('git describe --dirty').toString('ascii')

With:

const GI_GIT_VERSION = process.env.CI ? process.env.GI_VERSION : execSync('git describe --dirty').toString('utf8').trim()

[SebinSong]

@taoeffect
Just tested with the GIG DB shared to me and it works as expected:

[taoeffect]

I don't get this at all with the database... I also didn't get the extra empty month:

So I thought maybe forcing it to show the extra month by running sbp('gi.actions/group/forceDistributionDate', { contractID: sbp('state/vuex/state').currentGroupId }) would fix it, but no, that didn't cause the missed payments to show either.

Very odd. I've sent you my copy of the group state in case it helps (as a DM on GIG).

[SebinSong]

@taoeffect Right. That's actually something I observed too. If you run the app with the DB and go to the payments page and then check the distribution period in the current group settings, you will be able to observe this discrepancy:

Do you remember we worked on adding a logic to automatically detect/resolve this discrepancy in state.js a couple of years ago? (below)

group-income/frontend/model/state.js

Lines 391 to 415 in 22a497f

	store.watch(
	(state, getters) => getters.currentPaymentPeriod,
	(newPeriod, oldPeriod) => {
	// This watcher is for automatically syncing 'currentPaymentPeriod' with 'groupSettings.distributionDate'.
	// Before bringing this logic in, how the app updates the state related to group distribution period was,
	// 'currentPaymentPeriod': gets auto-updated(t1) in response to the change of 'reactiveDate.date' when it passes into the new period.
	// 'groupSettings.distributionDate': gets updated manually by calling 'updateCurrentDistribution' function(t2) in group.js
	// This logic removes the inconsistency that exists between these two from the point of time t1 till t2.
	// Note: if this code gets called when we're in the period before the 1st distribution
	// period, then the distributionDate will get updated to the previous distribution date
	// (incorrectly). That in turn will cause the Payments page to update and display TODOs
	// before it should.
	// NOTE: `distributionStarted` allows distributionDate can be updated automatically ONLY after
	// the distribution is started. And it fixes the issue mentioned above.
	const distributionDateInSettings = store.getters.groupSettings.distributionDate
	const distributionStarted = store.getters.groupDistributionStarted(reactiveDate.date)
	if (oldPeriod && newPeriod && distributionStarted && (newPeriod !== distributionDateInSettings)) {
	sbp('gi.actions/group/updateDistributionDate', { contractID: store.state.currentGroupId })
	.catch((e) => {
	console.error('okTurtles.events/on CONTRACT_REGISTERED Error calling updateDistributionDate', e)
	})
	}
	}
	)

This Vuex watcher is currently wrapped within a CONTRACT_REGISTERED event listener (with some extra if blocks) but it appears that this listener is never called, meaning the app is currently not benefiting from this auto-handling logic.

---

Anyways, this is a separate issue to this PR (which is why I did not mention it in the previous comment) and regarding how I got that empty last column in the graph was just exactly doing what the comments in the above state.js code says:

// Before bringing this logic in, how the app updates the state related to group distribution period was, 
// 'currentPaymentPeriod': gets auto-updated(t1) in response to the change of 'reactiveDate.date' when it passes into the new period. 
// 'groupSettings.distributionDate': gets updated manually by calling 'updateCurrentDistribution' function(t2) in group.js

updateCurrentDistribution function here is where updateGroupStreaks is executed, which updates all sorts of group streak fields.

In order to call this updateCurrentDistribution function, I just updated the income details of my account. (It also can be triggered by any member making a payment, or kicking out a member, as you might be well aware.)

---

Long story short,

1. The reason why you don't see the last column is due to the discrepancy between getters.currentPaymentPriod and the actual groupSettings.distributionDate.
2. The app does have the logic to automatically resolve this discrepancy in state.js but it's currently wrapped within CONTRACT_REGISTERED event listener but it appears that this is never called. (I might be wrong and I think we need Ricardo's help for correctly investigating it)
3. To get groupSettings.distributionDate updated, updateCurrentDistribution() function needs to be called and that's either by any group member updating their income details, or any group member leaving, or any pledging member recording a payment.
4. Once you take one of the actions in 3) and groupSettings.distributionDate is correctly set to current period, you will see the fix in this PR work.

Hope these things make sense. Thanks.

[taoeffect]

Had a chat with @corrideat about this on Slack:

greg Yesterday at 8:03 AM
Ricardo is CONTRACT_REGISTERED a missing propagated event?

Ricardo Yesterday at 11:51 PM
That's correct. It'd seem like CONTRACT_REGISTERED is a missing propagated event. Fixing that part would be pretty simple, it needs to be added to sw-primary.js (there's a list of events, ll. 106-113).
However, even if it's 'fixed' that way, it might still not be enough. I took a look at the event handler in question, and it seems like this would benefit from being in the SW (possibly as a periodic notification):
Events are not guaranteed to be forwarded (there may be no windows to forward events to)
Even when the event is forwarded, if there are many open windows each of them will be executing that logic and trying to submit an action.
The way it's currently written, it only works for the current group.

Ricardo Today at 5:22 AM
CONTRACT_REGISTERED is forwarded in PR #2568

[taoeffect]

I'm gonna assume this is fixed based on @SebinSong's comments and with the PR #2568 merged.