Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Support variable distribution period length #1691
Support variable distribution period length #1691
Changes from 7 commits
03b63f8
3b9e246
b6c71c1
a922cb4
aa52565
95b4838
7035686
11b4f90
46d8d70
0361950
d455769
8a0e335
f9252c1
543311f
cb081c2
51568c3
9162a5f
3dba4e2
024ad27
8c239d5
5938968
c1ec47a
f1c40c1
2261521
52ad851
6e378e8
0fc41eb
2dd2102
cd18f62
44c5d3b
7023b85
f5c684a
0d84402
4368f65
43b070b
8f2519d
b7767f5
d19ed2b
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
DRY: replace
getters.currentGroupState.paymentsByPeriod ?? {}
withgetters.groupPeriodPayments
which does the same thing.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This code should be replaced by the previous logic we had in the
periodStampGivenDate
function intime.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 insortedPeriodKeys
to it. That would keep this function here short & sweet while having the messy logic intime.js
.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This block is only called when
recentDate >= distributionDate
, and I don't thinkdistributionDate
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.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated to handle future dates using the period length in a loop like before (re-using
periodStampGivenDate
intime.js
)There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it should be
if (recentDate < sortedPeriodKeys[i]) return sortedPeriodKeys[i - 1]
.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wow, can't believe no test caught this - thanks
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think you need to do this conversion, because
distributionDate
is not aDate
object. It is in fact a period stamp that you can use directly.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 sincedistributionDate
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.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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 aDate
or astring
:And yet
dateFromPeriodStamp
only returns aDate
:But yeah, in the end,
distributionDate
should be treated as a period stamp :-\There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, thanks for explaining! 😄
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I do not understand what is going on here or why this code is so complicated.
AFAICT this is what this function should be:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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)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 substractingdistributionPeriodLength
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
(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)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK, that's a good point regarding
distributionDate
- my code doesn't check for that because it assumes (incorrectly) that that is already stored insortedPeriods
, which you're absolutely right, it might not be.Also, my code has a bug related to this check:
I think that should be:
The code that's currently here in the PR needs to be changed for a few reasons:
distributionDate
todateToPeriodStamp
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 compareperiodStamp
to that, and then bails if the check fails.We need something conceptually equivalent to the code I wrote above that:
distributionDate
Mental model
Our mental model is as follows:
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:If we want to go forward in time from
periodStamp
pointing at[ B ]
stamp, thenperiodAfterPeriod
points us to[ C ]
andperiodBeforePeriod
points us to[ A ]
.If, however, we have this situation:
Then:
periodAfterPeriod
addsdistributionPeriodLength
toperiodStamp
and returns the resultperiodBeforePeriod
returns[ B ]
And, if we have this situation:
Then:
periodAfterPeriod
returns[ B ]
periodBeforePeriod
returnsundefined
Note: that this is the only scenario where
undefined
can be returned.Also note: in that case
historicalPeriodBeforePeriod
(please rename from currently calledgetPeriodBeforePeriod
) does the exact same logic asperiodBeforePeriod
, except it performs the check on a version of[ SORTED_KEYS ]
that includes historical payments from then archive.In fact, because both
periodBeforePeriod
andhistoricalPeriodBeforePeriod
contain line-by-line exactly the same logic, please create a shared function inside oftime.js
that does this logic. WhenhistoricalPeriodBeforePeriod
andperiodBeforePeriod
call that function, they will include an array containing the[ SORTED_KEYS ]
.Finally, to take into account the
distributionDate
, simply add in thedistributionDate
to the[ SORTED_KEYS ]
that you pass in to the internalperiodBeforePeriod
inside oftime.js
.And similarly, there will be a corresponding shared
periodAfterPeriod
intime.js
that is called from bothgroup.js
andPaymentsMixin.js
.Hope that's clear! Let me know if you have any questions.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
returnsB
for any date betweenA
andB
not just forA
, then maybe they could as well be renamed for consistency:(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
accordinglyThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.😅
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice update 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this
currentPaymentPeriod
?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 propertiesThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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:new Date()
is not a reactive property itself, and is not updated as a result of a reactive Vue.js-related change (asreactiveDate
is), the computed property doesn't get updated either because that code is never re-run