Make payment periods a linked list #1673
Conversation
snowteamer
force-pushed
the
payment-periods
branch
3 times, most recently
from
3e4c4ca to
6c73468
Compare
| import { unadjustedDistribution, adjustedDistribution } from './shared/distribution/distribution.js' | ||
| import currencies, { saferFloat } from './shared/currencies.js' | ||
| import { inviteType, chatRoomAttributesType } from './shared/types.js' | ||
| import { arrayOf, mapOf, objectOf, objectMaybeOf, optional, string, number, boolean, object, unionOf, tupleOf } from '~/frontend/model/contracts/misc/flowTyper.js' | ||
|
|
||
| // Just a shortcut for Vue.set() with a default initial value. |
| // Should we throw instead? | ||
| if (!isPeriodStamp(givenStamp)) return null |
There was a problem hiding this comment.
Yes, please throw TypeError('must be periodstamp') (note: purposefully not localized because too obscure and developer-facing)
| // TODO: leave this undefined until the next PaymentPeriod object has been created. | ||
| nextPeriodID, |
There was a problem hiding this comment.
What is this TODO comment about?
There was a problem hiding this comment.
For now this refers to a period that doesn't exist yet, and things which don't exist yet are maybe not supposed to already have IDs. So this TODO is about setting the nextPeriodID only when the corresponding period object is actually created. But that's not a high priority
| state.paymentsByPeriod[distributionDate].nextPaymentPeriodID = period | ||
| curPeriodPayments.previousPaymentPeriodID = distributionDate |
There was a problem hiding this comment.
Why are these named nextPaymentPeriodID and previousPaymentPeriodID instead of previousPeriodID and nextPeriodID as used elsewhere in the code?
Looks like a bug to me - please remember to using forceDistributionDate!
There was a problem hiding this comment.
Yea most likely a bug. A test case in group-paying.spec.js is already using giForceDistributionDateToNow, which depends on forceDistributionDate, so I'm a bit surprised it didn't catch this. Could also have been caught with better typechecking.
| } | ||
| return currentPeriodStamp | ||
| } else { | ||
| // Iterate backwards to find a first previous period stamp that's lower than the given date. |
There was a problem hiding this comment.
clearOldPayments is going to clear out the payment periods available for iteration after 6 months (e.g. MAX_SAVED_PERIODS).
This code will break if a date is given outside of that range, and will return null.
Therefore this code in PaymentDetail.vue will break:
this.payment.periodstamp = this.periodStampGivenDate(this.payment.meta.createdDate)Along with other code in the project that uses periodStampGivenDate.
You'll have to comb through the project for all uses of periodStampGivenDate and handle appropriately, perhaps creating a more accurate periodStampGivenDate async function in time.js that tries to use this version of periodStampGivenDate, and upon getting null, checks the archives for older payment periods. You'll need to look at 'gi.contracts/group/archivePayments' for details.
Similarly, you'll need to do a project-wide search also for periodBeforePeriod and verify that all uses are safe and revert to checking the archives if necessary.
Note: you cannot use async stuff in getters, which must be synchronous and can only access data from state.
There was a problem hiding this comment.
Thank you for clarifying this, it was not obvious to me. So, PaymentDetail.vue and other locations need the ability to display information for payments belonging to older deleted periods?
There was a problem hiding this comment.
@snowteamer Yes, the Payments page has Sent payments and Received payments that will show historical payments, and the ability to inspect them to show details about those old payments that are stored in IndexedDB under special keys. See also the file PaymentsMixin.js
| periodAfterPeriod (state, getters): string | void { | ||
| return (periodStamp: string) => getters.groupPeriodPayments[periodStamp]?.nextPeriodID |
There was a problem hiding this comment.
This needs to be modified because sometimes we are given dates in the future, for example, have a look at this code in state.js:
const reactiveDate = Vue.observable({ date: new Date() })
setInterval(function () {
// We want the getters to recalculate all of the payments within 1 minute of us entering a new period.
const rememberedPeriodStamp = store.getters.periodStampGivenDate?.(reactiveDate.date)
const currentPeriodStamp = store.getters.periodStampGivenDate?.(new Date())
if (rememberedPeriodStamp !== currentPeriodStamp) {
reactiveDate.date = new Date()
}
}, 60 * 1000)This code will break with the way periodAfterPeriod is currently implemented, but it would work if you re-introduced the code we have before. You might want to combine the two, e.g. if nextPeriodID doesn't exist, use the old code to generate the correct periodStamp using the distributionPeriodLength
|
Closing in favor of #1691 |
Closes #1631
Summary of changes:
Previously, the
periodStampGivenDate()getter would return a timestamp for an "imaginary" payment period when the given date was too far in the future or in the past. It could do so by assuming the period length was a constant. Now since that assumption no longer holds, it will returnnullif no actual payment period data can be found for the given date, without trying to extrapolate in the future or the past.The eponymous function has been removed from
time.js.