Proposal to update distribution date

[Silver-IT]

Summary of Changes

• Closes Create a proposal for changing the distribution date #1543
• Closes Jump to latest message button should be invisible #1710

[Silver-IT]

@taoeffect, seems ready for your review.

[SebinSong]

My apologies. I accidentally deleted the source branch of this PR, thinking it's an old working branch of mine while I was organising local branches in my laptop..! just restored it.

[SebinSong]

Looks good to me @Silver-IT, Great work!

[taoeffect]

Nicely done @Silver-IT, I haven't finished reviewing everything yet, but I left another preliminary review and did some testing.

I noticed a few issues during testing:

Issue 1: disabled menus

When u2 joins the group this is what his menu looks like for creating proposals:

It should look the same as on master:

Issue 2: Can't change the distribution date again

I created a group today (Aug 29) and the distribution was set to Sept 1 initially (if I remember correctly). As u1 I changed it to be Aug 30th. Now I can no longer change it again:

I should be able to change it as many times as I want until the first period starts.

[taoeffect]

I'm not sure about this code. Why is this here and what is it doing?

[Silver-IT]

If the distributionDate is changed by proposal, paymentsByPeriod should be changed too because the keys of paymentsByPeriod are the distribution dates.
Also we shouldn't initialize the paymentPeriod by calling initPaymentPeriod function, because there could be users who have already entered their income details.

[taoeffect]

OK, that doesn't make much sense because if the user entered their income details, that will not modify or affect the values in state.paymentsByPeriod, instead it will affect their group profile.

What we should do instead is the following:

1. First we need to make sure that we are only updating the distribution date if it is safe to update it. It is only safe to update the distribution date if meta.createdDate is before the current distribution date. So you need to first create an if condition here to check for that.
2. Next, we can verify that there is only 1 group period currently in the state.paymentsByPeriod.
3. If so, we delete it / reset state.paymentsByPeriod to an empty object.
4. Next, we update the distribution date to the new distribution date. So state.settings.distributionDate is updated to the new date.
5. Finally, we call initFetchPeriodPayments exactly in the same way that it is called in the process function for 'gi.contracts/group'

This will ensure the exact same steps as happened in the group constructor are followed for resetting the distribution date, and that it is done safely only at the appropriate time.

[Silver-IT]

@taoeffect, Please correct me, if I am wrong.
I think we should set haveNeedsSnapshot using getters.haveNeedsForThisPeriod(period) after calling initFetPeriodPayments.

[snowteamer]

if the user entered their income details, that will not modify or affect the values in state.paymentsByPeriod

I'm not 100% sure but I think the haveNeedSnapshot field of the payment period object gets updated when users update their income details

[taoeffect]

I'm not 100% sure but I think the haveNeedSnapshot field of the payment period object gets updated when users update their income details

That's true, but AFAICT that code is not affected by this code.

[taoeffect]

Why was this added?

[Silver-IT]

So we don't allow a group member to update voting rules when group has less than 3 members and he is not group creator.

[taoeffect]

OK but this should be disabled at the menu item on the dashboard for bringing up the modal, not here, right?

[Silver-IT]

Users can open the modal even though we disable the menu item.
They can copy the URL (e.g, http://localhost:8000/app/dashboard?rule=percentage&modal=ChangeVotingRules) and open it directly in the browser. So I thought we should check the permission inside the modal and should do one of the following two things.

• Close the modal
• Disable the submission

[taoeffect]

Review ready!

[taoeffect]

OK, that doesn't make much sense because if the user entered their income details, that will not modify or affect the values in state.paymentsByPeriod, instead it will affect their group profile.

What we should do instead is the following:

1. First we need to make sure that we are only updating the distribution date if it is safe to update it. It is only safe to update the distribution date if meta.createdDate is before the current distribution date. So you need to first create an if condition here to check for that.
2. Next, we can verify that there is only 1 group period currently in the state.paymentsByPeriod.
3. If so, we delete it / reset state.paymentsByPeriod to an empty object.
4. Next, we update the distribution date to the new distribution date. So state.settings.distributionDate is updated to the new date.
5. Finally, we call initFetchPeriodPayments exactly in the same way that it is called in the process function for 'gi.contracts/group'

This will ensure the exact same steps as happened in the group constructor are followed for resetting the distribution date, and that it is done safely only at the appropriate time.

[taoeffect]

OK but this should be disabled at the menu item on the dashboard for bringing up the modal, not here, right?

[taoeffect]

What is this skipToPropose thing?

[Silver-IT]

In the solution of the issue #1543, it describes that the group creator should not make propose to update distribution date before the first distribution started.
So an additional field is needed to make decision to propose or not for some proposals (Only DistributionDate for now, but not sure for later).

[Silver-IT]

Should we change the name then?

• shouldImmediateUpdate
• shouldNotPropose
• ...

[taoeffect]

Oh, ok, yes, the name skipToPropose is very confusing in English (skipProposal would make more sense).

I would name this: applyChangesWithoutProposal, EDIT: but it seems you've renamed it to shouldImmediateChange which works too 👍

[taoeffect]

This is causing a DRY violation. We should disable bringing up the modal from the dropdown menu, not here.

[taoeffect]

Same issue here. Please revert related changes

[taoeffect]

@Silver-IT ready!

[taoeffect]

@Silver-IT This is better, nice. 👍

A few things are missing here from the review though:

It is only safe to update the distribution date if meta.createdDate is before the current distribution date. So you need to first create an if condition here to check for that.

And:

Next, we update the distribution date to the new distribution date. So state.settings.distributionDate is updated to the new date.

However, in the code above, the distribution date is always being updated. This is dangerous because as written this contract does not prevent the distribution date from being updated at any point in time. It must be updated only when conditions are correct (as mention the previous review). To fix this, I would move the entire if condition before the loop on line 1091 and do something like this:

if ('distributionDate' in data) {
  if (meta.createdDate < state.settings.distributionDate) {
    // ...
  } else {
    delete data.distributionDate
  }
}

Finally, sorry for not answering your question earlier about haveNeedsSnapshot. As far as I can tell, that does not need to be updated because it is entirely unrelated to the distribution date. So lines 1098 and 1099 above should be deleted, and similarly, line 1097 can be rewritten so that initFetchPeriodPayments is called without capturing its return value.

[Silver-IT]

It is only safe to update the distribution date if meta.createdDate is before the current distribution date. So you need to first create an if condition here to check for that.

I think I did it in the validate function.

else if ('distributionDate' in data &&
    (getters.groupDistributionStarted(meta.createdDate) || Object.keys(getters.groupPeriodPayments).length > 1)) {
    throw new TypeError(L('Can\'t change distribution date because distribution period has already started.'))
}

[taoeffect]

I'm not convinced this is needed. What happens if this change is removed?

[Silver-IT]

It's possible to update distributionDate before the distribution is started by calling gi.contracts/group/updateSettings function. Once the distributionDate is updated before the distribution is started, this currentPaymentPeriod watch also calls the gi.actions/group/updateDistributionDate function again and sets a wrong distributionDate.
reactiveDate and this watch should be allowed to update distributionDate only after the distribution is started.

[taoeffect]

Hmm, OK. One thing though, I think the comment is wrong. You could use store.getters.groupDistributionStarted(reactiveDate.date), it would be the same result. Can you make that change to DRY things?

[taoeffect]

Final review ready!

[taoeffect]

Hmm, OK. One thing though, I think the comment is wrong. You could use store.getters.groupDistributionStarted(reactiveDate.date), it would be the same result. Can you make that change to DRY things?

[taoeffect]

While we're boy-scoutting, three change requests:

1. We forgot to wrap all of these in L functions, please update this line and the lines below for each name entry to wrap the string in an L function and re-run strings
2. Also, can you change this specific string from 'Change to percentage base' to: 'Change voting threshold' ?
3. Also wrap using L function the name for the header (e.g. 'Voting Systems' => L('Voting Systems') --- all user-facing strings must be localized

[taoeffect]

Approved!! Great job @Silver-IT!! 😄 🎉