Support variable distribution period length

[snowteamer]

Closes #1631
Closes #1662

[taoeffect]

Nicely done @snowteamer!

Preliminary review ready for you! I didn't have time to do a full review this time and had to top short after reviewing periodStampGivenDate, but did my best to get what I had out to you.

[taoeffect]

This check doesn't make sense, because recentDate is conceptually not a period stamp, it's a date, so please remove it.

You can convert a date to a periodstamp using dateToPeriodStamp if you need to.

The reason why we have these functions and must use them is because even though dates and period stamps happen to be the same thing, they are conceptually different. Treating them as the different types that they are and using the corresponding conversation functions allows us to change the format of a period stamp in the future if we need to.

[snowteamer]

Agreed, and in fact I was looking for an isIsoString function, but there is no such function in time.js, and adding one would have duplicated the code in isPeriodStamp. So I just used that

[taoeffect]

This code should be replaced by the previous logic we had in the periodStampGivenDate function in time.js, so that it can extrapolate multiple periods out. There's no reason not to, and we have the code for that already.

I would recommend re-introducing that function in time.js, and passing in sortedPeriodKeys to it. That would keep this function here short & sweet while having the messy logic in time.js.

[snowteamer]

Maybe you're right, but I haven't seen any use case where extrapolating more than one period in the future would be actually useful. I thought it didn't make much sense.

[taoeffect]

Say the user has their clock accidentally incorrectly set a year in the past. They will be browsing old payments they sent and using this getter to fetch the period in which the payment occurred. This getter will return the wrong value. Why should we return the wrong value when I've already written the code to return the correct value?

[snowteamer]

This block is only called when recentDate >= distributionDate, and I don't think distributionDate can depend on the user's clock.

Therefore in case the clock incorrectly thinks it's already several period lengths in the future, way past the distribution date, so that this block gets called, then there just won't be any matching payment to be displayed but I think that's correct.

If the user has their clock accidentally incorrectly set a year in the past, so that recentDate is one year lower than expected, then this block won't be called. The sorted periods will be searched instead, and I think the user should still be able to browse any stored payment period up to the distribution date.

[taoeffect]

Hmm, ok, that does seem to be true. Still, I don't see why we can't include a loop here that was already written, and therefore not need to worry about this returning an incorrect value.

[snowteamer]

Updated to handle future dates using the period length in a loop like before (re-using periodStampGivenDate in time.js)

[taoeffect]

Nice work @snowteamer! Preliminary review ready!

[taoeffect]

Say the user has their clock accidentally incorrectly set a year in the past. They will be browsing old payments they sent and using this getter to fetch the period in which the payment occurred. This getter will return the wrong value. Why should we return the wrong value when I've already written the code to return the correct value?

[taoeffect]

Minor change requests for tonight, still reviewing...

[Silver-IT]

Impressive work, @snowteamer. I didn't fully review this PR yet, but I did a preliminary review.

[Silver-IT]

I think it should be if (recentDate < sortedPeriodKeys[i]) return sortedPeriodKeys[i - 1].

[snowteamer]

Wow, can't believe no test caught this - thanks

[Silver-IT]

Is this currentPaymentPeriod?

[snowteamer]

It should be but currentPaymentPeriod is a bit special in that it's controlled by reactiveDate, which doesn't update in real time but in interval. So if I understand correctly it will sometimes be wrong relative to the actual current date, therefore I chose to not rely on it in other computed properties

[taoeffect]

@snowteamer Alex is right, because Vue.js is not going to automatically update the UI when new Date() moves us into a new period. This is for 2 reasons:

1. This is a method, not a computed property
2. Even if it were a computed property, it still wouldn't work because computed properties are only updated in response to reactive changes. Since new Date() is not a reactive property itself, and is not updated as a result of a reactive Vue.js-related change (as reactiveDate is), the computed property doesn't get updated either because that code is never re-run

[taoeffect]

Another preliminary review. Will wait until everything is addressed in previous reviews before proceeding with another review

[taoeffect]

DRY: replace getters.currentGroupState.paymentsByPeriod ?? {} with getters.groupPeriodPayments which does the same thing.

[taoeffect]

I don't think you need to do this conversion, because distributionDate is not a Date object. It is in fact a period stamp that you can use directly.

[snowteamer]

I thought distributionDate was an ISO date string, which happens to also be a period stamp but is conceptually a different thing (as you explained to me). It made sense to me since distributionDate comes from the group settings, whereas "period stamp" is a more internal type I think.

So anywhere I had the chance to compare a distributionDate value against a date directly, I didn't do any conversion.
I will likely now have to add a few of such conversions there if I start to consider distributionDate as being of the "period stamp" type.

[taoeffect]

@snowteamer I think we made a mistake in how we named that variable... it is in fact a period stamp... and it happens to be a date represented as an ISO string.

Maybe you can add a comment to the constructor to that effect? (Renaming it now is not a good idea since there are live groups running).

Also, it's a bit confusing with how we've implemented dateToPeriodStamp, which takes either a Date or a string:

export function dateToPeriodStamp (date: string | Date): string {
  return new Date(date).toISOString()
}

And yet dateFromPeriodStamp only returns a Date:

export function dateFromPeriodStamp (daystamp: string): Date {
  return new Date(daystamp)
}

But yeah, in the end, distributionDate should be treated as a period stamp :-\

[snowteamer]

Ok, thanks for explaining! 😄

[taoeffect]

I do not understand what is going on here or why this code is so complicated.

AFAICT this is what this function should be:

    periodBeforePeriod (state, getters) {
      return (periodStamp: string) => {
        const { distributionPeriodLength } = getters.groupSettings
        const sortedPeriods = getters.groupSortedPeriodKeys
        for (const i = sortedPeriods.length - 1; i >= 0; --i) {
          const latestPeriod = sortedPeriods[i]
          if (periodStamp >= latestPeriod) return latestPeriod
        }
        return dateToPeriodStamp(addTimeToDate(dateFromPeriodStamp(periodStamp), -distributionPeriodLength))
      }
    }

[snowteamer]

This code is indeed a bit complicated, in part because of the periodStamp === distributionDateStamp case, so I've tried to explain it with a a lot of comments (but failed to make it clear enough)

this is what this function should be:

If I'm not mistaken, this won't ever return undefined to defer searching to the archive code. And since every archived period could have a different length, simply substracting distributionPeriodLength is not correct in that case.
Also it doesn't do anything special for the aforementioned special case.

Would be great if that case was actually not special and we could just use a simple loop like yours, but for now the suggested code fails this assertion line 118 in group-paying.spec.js

      expect(periodBeforePeriod(onePeriodLengthAhead) === distributionDate).to.be.true

(not 100% sure why but my guess it's because in that case there exists a stored object for the waiting period but not for the current distribution date yet)

[taoeffect]

OK, that's a good point regarding distributionDate - my code doesn't check for that because it assumes (incorrectly) that that is already stored in sortedPeriods, which you're absolutely right, it might not be.

Also, my code has a bug related to this check:

if (periodStamp >= latestPeriod) return latestPeriod

I think that should be:

if (periodStamp > latestPeriod) return latestPeriod

The code that's currently here in the PR needs to be changed for a few reasons:

1. As discussed above, we need to avoid passing distributionDate to dateToPeriodStamp
2. It needs to be simplified similarly to what I wrote above. Right now it's got some very questionable lines, like this:

        if (periodStamp > distributionDateStamp) {
          // Only extrapolate one period length in the future.
          const onePeriodLenghtAhead = addTimeToDate(distributionDate, distributionPeriodLength)
          if (periodStamp === dateToPeriodStamp(onePeriodLenghtAhead)) return distributionDate
          else return
        }

This is very strange. The purpose of this function is to return the period before a given period stamp. That's not what the lines above do at all. Instead they seemingly randomly add a period length to distributionDate and compare periodStamp to that, and then bails if the check fails.

We need something conceptually equivalent to the code I wrote above that:

1. Takes into account distributionDate
2. Has a consistent mental model for how period stamps are created and iterated forward/backward

Mental model

Our mental model is as follows:

<--- [ UNKNOWN PAST ] --- [ SORTED_KEYS ] ---- [ UNKOWN FUTURE ] --->

Here we see 3 section. There is a "known" timeline in the center, and an unknown timeline at each end.

If we are given a period stamp that is inside of [ SORTED_KEYS ], here's what iterating it looks like:

[ A ] <-> [ B ] <-> [ C ]
            ^
            |
      periodStamp

If we want to go forward in time from periodStamp pointing at [ B ] stamp, then periodAfterPeriod points us to [ C ] and periodBeforePeriod points us to [ A ].

If, however, we have this situation:

[ B ] <-> [ C ] <-> [ UNKNOWN FUTURE ]
            ^
            |
      periodStamp

Then:

• periodAfterPeriod adds distributionPeriodLength to periodStamp and returns the result
• periodBeforePeriod returns [ B ]

And, if we have this situation:

[ UNKNOWN PAST ] <-> [ A ] <-> [ B ]
                       ^
                       |
                   periodStamp

Then:

• periodAfterPeriod returns [ B ]
• periodBeforePeriod returns undefined

Note: that this is the only scenario where undefined can be returned.

Also note: in that case historicalPeriodBeforePeriod (please rename from currently called getPeriodBeforePeriod) does the exact same logic as periodBeforePeriod, except it performs the check on a version of [ SORTED_KEYS ] that includes historical payments from then archive.

In fact, because both periodBeforePeriod and historicalPeriodBeforePeriod contain line-by-line exactly the same logic, please create a shared function inside of time.js that does this logic. When historicalPeriodBeforePeriod and periodBeforePeriod call that function, they will include an array containing the [ SORTED_KEYS ].

Finally, to take into account the distributionDate, simply add in the distributionDate to the [ SORTED_KEYS ] that you pass in to the internal periodBeforePeriod inside of time.js.

And similarly, there will be a corresponding shared periodAfterPeriod in time.js that is called from both group.js and PaymentsMixin.js.

Hope that's clear! Let me know if you have any questions.

[snowteamer]

This is very strange. The purpose of this function is to return the period before a given period stamp.

Exactly, so if the given date is not actually a past, current or future period stamp but just a random ISO string, my assumption was that undefined had to be returned.

If that assumption shoud be dropped so that these getters behave like periodStampGivenDate, e.g. periodAfterPeriod returns B for any date between A and B not just for A, then maybe they could as well be renamed for consistency:

• nextPeriodStampGivenDate
• periodStampGivenDate
• previousPeriodStampGivenDate
  (I'd rather use -forDate than -givenDate). Otherwise periodAfter/BeforeDate also sound good to me.

Another assumption in this PR, is that distributionPeriodLength is now a variable setting whose value only holds for the current period. It can no longer be safely used to iterate between existing periods, but is just a hint to initially setup a default next distribution date.
Moreover, in the waiting period the distribution date i.e. the next period stamp can be updated without updating distributionPeriodLength accordingly

[taoeffect]

Rename to historicalPeriodStampGivenDate

[snowteamer]

It can also return non-historical results, so I think it would be a bit confusing

[taoeffect]

True, but the point is that it unlike periodStampGivenDate it can give historical payment periods, so that's why it needs to be named that way. See also the mental mode comment I just posted.

[snowteamer]

I'm not fan of dropping the get- prefix here, as all other methods in this file are get- prefixed. It likely helps to distinguish them from computed properties. But I also think getHistoricalPeriodStampGivenDate would be a tad long...

[snowteamer]

Should all PaymentsMixin.js methods that can return both in-memory and historical results be named that way (some currently don't)?

[taoeffect]

Yeah feel free to make it consistent and clear 👍
(I haven't checked your latest changes as of this comment but will have a look now)

[Silver-IT]

Finished my review. Nice work, @snowteamer

[taoeffect]

Another preliminary review ready!

[taoeffect]

Note: !keys.length should always be false because the contract constructor calls initFetchPeriodPayments, ensuring at least one entry

[snowteamer]

In fact, initFetchPeriodPayments calls periodStampGivenDate before doing anything else, which in turn calls groupSortedPeriodKeys

[taoeffect]

Ah, I forgot that, that's a good point. 👍

I investigated this further and surprisingly yes this code actually seems to reproduce the behavior of the code on master, which also initializes state.paymentsByPeriod with the periodstamp of the period before the first distributionDate — the never-used "waiting period". Which is kind of surprising but yeah this code seems to do the same thing so 👍

[taoeffect]

I don't think this logic captures whether or not we're in the waiting period.

By "waiting period" I assume you mean in the period before the very first distribution has started. However, this logic doesn't check for that. We could be in in the 3rd period since the group started and this would still evaluate to true.

Whether or not we're in the first waiting period is determined really by the number of keys in getter.groupPeriodPayments. If there is only 1 key in there, and meta.createdDate < current, only then can we be considered to be inWaitingPeriod. And then you also don't need the redundant check of meta.createdDate !== current below:

[snowteamer]

We could be in in the 3rd period since the group started and this would still evaluate to true.

In a "normal" (not waiting) period, meta.createdDate < current should always be false, since the distribution date is when the current period started, not when the next period will start

Otherwise if I'm wrong, couldn't I just use the new getters.groupDistributionStarted(meta.createdDate) implemented by Alex?

Although I see it's not used alone somewhere else in this file, but combined with a check for the number of stored period keys:

      if (getters.groupDistributionStarted(meta.createdDate) || Object.keys(getters.groupPeriodPayments).length > 1)) {

(not sure why the getter isn't enough to ensure we're out of the waiting period)

[taoeffect]

In a "normal" (not waiting) period, meta.createdDate < current should always be false, since the distribution date is when the current period started, not when the next period will start

That's probably true.

couldn't I just use the new getters.groupDistributionStarted(meta.createdDate) implemented by Alex?

The two seem to be equivalent. You could do:

const inWaitingPeriod = !getters.groupDistributionStarted(meta.createdDate)

That would probably be best from a DRY perspective.

but combined with a check for the number of stored period keys:

Yeah I'm not sure what that's about. @Silver-IT couldn't you remove that || Object.keys(getters.groupPeriodPayments).length > 1 check? Although that might be there for a reason related to adjusting the distribution date that I don't remember.

Yeah, it could be that the distribution date was adjusted via a proposal to suddenly be in the past?

This is the code: https://github.com/okTurtles/group-income/pull/1643/files#diff-5c6306168381a1d6902fce7da1cdf9a6811e2ed4ac74d7d97bfff5145087b626R122

It seems like the proposal only allows for updating the distribution date to a date in future, starting with 1 day in the future.

However, what if by the time the proposal passes we are already ahead of that date? For example, what today is Sept. 1st, and we set the distribution date to Sept 30th, but then create a proposal to make it Sept. 2nd, but by the time the proposal passes it's already Sept 3rd?

Need to consider this.... 🤕

[taoeffect]

These look useful, you could export these too...

[taoeffect]

This is redundant when date is an iso string already. Perhaps this logic could be rewritten like so:

const timestamp = typeof date === 'string' ? date : date.toISOString()

[snowteamer]

Indeed - This time I wanted to keep the code simple, but actually I don't like creating Date objects when not necessary

[taoeffect]

This can be rewritten to remove the redundant typeof date === 'string' check like so:

if (!(isIsoString(date) || date instanceof Date)) {

(Note: for obsessive-compulsive "performance" reasons I've placed the check that's most likely to pass first, in this case the parameter is more likely to be an ISOString, so that check happens first to avoid triggering the instanceof Date check)

[snowteamer]

You're right, and I would even remove the date instanceof Date check if possible, since Flow annotations should already handle that.
Initially I added the isIsoString check only because string was are a superset of ISO strings

Edit: After further testing, it appears that .toISOString() throws when called on invalid date objects like new Date('foo'). So to avoid possible uncaught errors here we'll have to do a bit more validation or use a try block...

What about this:

  // `isNaN` - not `Number.isNaN` - is used here to catch `Invalid Date` objects.
  if (!isIsoString(date) && !(date instanceof Date && !isNaN(date))) {
    throw new TypeError('must be ISO string or valid Date object')
  }
  const timestamp = typeof date === 'string' ? date : date.toISOString()

Or maybe we could restrict the API to just not accept Date objects at all?

[taoeffect]

I don't understand why you changed the condition. The one I used should work fine, and it also avoids both checks most of the time because it uses ||

The flow annotations won't always save us (type info is lost when using SBP)

[snowteamer]

In this suggestion I used && rather than || to avoid an extra pair or parens, and isNaN to guard against Invalid Date values. These are quite uncommon but could cause the toISOString() call to throw

Otherwise it's equivalent to if (!(isIsoString(date) || date instanceof Date))

[taoeffect]

used && rather than || to avoid an extra pair or parens

It's better to have the parens than to have an extra logical check imo

and isNaN to guard against Invalid Date values.

I really don't think that's needed

Otherwise it's equivalent to if (!(isIsoString(date) || date instanceof Date))

It's not... the || condition results in 1 check most of the time, whereas the modified && results in 3 different check every time

[taoeffect]

• We don't need to ?? undefined here (undefined is returned for us)
• Don't forget to break out of the loop

[taoeffect]

this.getSortedPeriodKeys doesn't exist!

(Please test code before pushing to make sure it works. I might not always be a good enough of a reviewer to catch stuff like this, but the computer will always catch it if you test these cases)

[snowteamer]

Well I ran our test suite - I'm unfortunately no better at catching typos or old names, and need my tools to do it for me.😅

[taoeffect]

@snowteamer Alex is right, because Vue.js is not going to automatically update the UI when new Date() moves us into a new period. This is for 2 reasons:

1. This is a method, not a computed property
2. Even if it were a computed property, it still wouldn't work because computed properties are only updated in response to reactive changes. Since new Date() is not a reactive property itself, and is not updated as a result of a reactive Vue.js-related change (as reactiveDate is), the computed property doesn't get updated either because that code is never re-run

[taoeffect]

This is supposed to be a computed property, not a method. Methods and computed properties are different. When computed properties change the UI gets updated automatically. A method cannot be bound to something as a computed property and won't result in UI updates when its return value changes. Check the Vue.js docs for more info.

[taoeffect]

Please check the Vue.js documentation for how to set data properly. data cannot be an object, it must be a function that returns an object.

[snowteamer]

Hmm, good idea, my Vue.js is visibly a bit rusty...

[taoeffect]

@snowteamer I thought I might be able to merge this PR but actually there are some serious errors still (testing still TBD).

Flow errors

PRs must never introduce flow errors. The following errors are present:

Running "exec:flow" (exec) task
Error -------------------------------------------------------------------- frontend/model/contracts/shared/time.js:25:47

Cannot get `Object.prototype.toString` because property `toString` [1] cannot be unbound from the context [2] where it
was defined. [method-unbinding]

   frontend/model/contracts/shared/time.js:25:47
    25|   if (!(isIsoString(date) || Object.prototype.toString.call(date) === '[object Date]')) {
                                                      ^^^^^^^^ [1]

References:
   /private/tmp/flow/flowlib_e73d01e77da1cc86_501/core.js:236:5
   236|     toString(): string;
            ^^^^^^^^^^^^^^^^^^ [2]

If you're certain this code is correct then you can use FlowFixMe comments to disable flow checking on that line (search the project for examples if you need).

Contract files are incorrect

Running grunt pin:0.1.8 --overwrite shows that the generated contracts are in fact different from the changes in this PR, indicating that they're out of date. Whenever you modify contracts that command needs to be run before pushing a commit.

Boyscoutting

I left some other comments earlier (e.g. see the comment about mounted placement, this.humanDate instead of humanDate), etc. that can now be addressed since these other changes are being requested.

While you're working on these changes I'll also be testing this PR and will get back to you with any other issues I find (if any).

[taoeffect]

So far the tests have gone pretty well!

I did run into one problem though:

To reproduce:

1. Create a $1000 group with u1 and invite u2, u3

2. u1 income details pledging $100, u2 making $800, u3 making $780

3. In console enter:

   sbp('gi.actions/group/forceDistributionDate', { contractID: sbp('state/vuex/state').currentGroupId })

4. Make 1 full payment, and 1 partial payment

5. Move to the next distribution period by running sbp('gi.actions/group/forceDistributionDate', { contractID: sbp('state/vuex/state').currentGroupId }) again

6. As u1 check the completed payments

[taoeffect]

Almost perfect!

Don't forget to always run grunt pin:0.1.8 --overwrite before pushing changes that contain modifications to contracts code (directly or indirectly through includes).

EDIT: found one more bug!

This time it appears under the RECEIVED tab for users receiving payments:

[taoeffect]

Two issues:

1. The contracts are still out of sync with the latest changes. Please run grunt pin:0.1.8 --overwrite each time any modifications are made to contracts before pushing a commit
2. The Record Payments modal displays 2 rows for some reason in the 2nd payment period:

To reproduce:

1. Create group with u1 and u2, and add income details for both
2. Run sbp('gi.actions/group/forceDistributionDate', { contractID: sbp('state/vuex/state').currentGroupId }) to enter new period
3. u1 pays u2
4. Run sbp('gi.actions/group/forceDistributionDate', { contractID: sbp('state/vuex/state').currentGroupId }) to enter 2nd period
5. u1 pays u2 — modal shows 2 rows for some reason

[taoeffect]

Why did this remove cloneDeep(this.ourPayments?.sent || []) and cloneDeep(this.ourPayments?.received || [])?

[snowteamer]

To avoid collecting the same entries twice since the current period is going to be processed anyway in the below loop. Another idea would be to keep the cloneDeep calls, but then exclude the current period from the loop.

[taoeffect]

I see..

[taoeffect]

WOOHOOOOOO!!!! 😄 🎉

GREAT JOB @snowteamer !!!!!

This was a doozy of a PR but you got it working!! 😄 🥇 🏆