group-income
group-income copied to clipboard
Implement PayGroup page (V1)
Problem
The PayGroup page is unimplemented.
Solution
Implement the first version of the PayGroup page. The second version is being discussed here: #757
Implement having multiple frozen payments distributions (see comment in Payments.vue).
Closing this issue implies closing #206 (cash-based payments)
Update May: based on #900, here's what's still missing:
What doesn't work
- [ ] Figuring out what to do when a payment is marked as not received (no proper followup to this) — @mmbotelho @dotmacro please play with it and see if you can offer suggestions
- [ ] Proper display of currencies based on the group's currency setting
- [ ] Conversion of currencies or handling currency change proposals
- [ ] Payments list pagination
- [x] Display and record Late payments (started at #918)
- [x] Partial payments that are done in more than 2 parts. (e.g. $100 paid 25 + 40 + 35)
- [ ] Support fetching old payments (+1 month ago)
What needs to be done next
TODO:
- [ ] Sorting and reverse sorting the payments list by clicking the tabs
- [ ] Implement and use the
prevMonthDueDate
computed property for late payments inPaymentRowTodo.vue
- [ ] Cancelling payments? Not sure how that flow is supposed to work, wasn't clear from Figma (cc @mmbotelho)
- [ ] Maybe make it so that payments can be marked as completed immediately when
paymentType === PAYMENT_TYPE_MANUAL
(see TODO in/payment
ingroup.js
), although this one isn't that important, it works well enough as it is now without it (two messages sent, one/payment
followed by a/paymentUpdate
) - [ ] Making the payment date information use the time given by the server instead of the system clock (so no
new Date()
usage, but creating dates based on an API call, see #531 and the/time
API defined on the backend) - [ ] Implement anything I forgot to mention from the "what doesn't work" section above
Done #951
- [x] Making the
ESC
key clear the search field - [x] Fixing a bug where if we switched from sending payments to receiving payments, we suddenly lose the "Sent" tab
- [x] Add tests to received payments and a scenario where a user changes from giving to receiving mincome
Done #917
- [x] Someone needs to go through and clean up all of the CSS — deduplicate the all of the copy/pasted junk in
PaymentRow*.vue
files and DRY it all so that it's all stored in one of the.scss
files and not repeated - [x] Create mixins and remove no longer used computed properties/methods from the
PaymentRow*.vue
files (and any other payment related files) - [x] Delete
humanDate.js
and move thehumanDate
function intotime.js
- [x] Delete outdated comments in the markup
- [x] Cleanup and DRY the markup, CSS, and any JS, but especially the CSS
Done #918
- [x] Correctly implement the
PaymentDetails.vue
file - [x] Sidebar overview of payments (the stats on the righthand side)
- [x] Proper display of payment due dates and late payment dates
- [x] Correctly implement the
MonthOverview.vue
file - [x] Remove the
Math.random()
stuff in thePaymentRow*.vue
files (mostly related to payment date stuff) and replace it with the actual data - [x] Reset button in RecordPayments modal, and other functionality like the memo field (didn't hook that up, needs to be done, but should be straightforward now)
- [x] Error handling!! I did not add any proper error handling or error displaying for sending payments, or for the input forms. Vuelidate validations need to be added where appropriate as well.
This is not designed yet... It's a bit confusing to have this issue open already
@mmbotelho oh but this issue refers to the designs you've already made and that have already been implemented. My plan was to implement the logic in a way that should be compatible with the new designs you're working on (or at the very least doesn't require much change by way of the core contract logic), with what we already have implemented from you previous designs. Does that sound ok to you?
It does, yes, but what I'm trying to say is that we should discuss everything and do the mockups first before we move on to implementing any logic or UI. This way we prevent having miscommunications and implementing and designing things that don't match.
I believe @pieer already implemented this, right? Can we close it @taoeffect ?
I’m currently working on this issue. Pierre implemented the UI, the functionality is still unfinished.
ok @taoeffect!
I updated this issue description with what's missing from #900. I'll review the list and pick a small chunk to fix on a new PR. (I'll let you know which parts soon). If I have questions, I'll leave them here.
-- EDIT --
I'll start with cleaning up PaymentRow*.vue
components using named slots and avoiding v-if
.
- Making the payment date information use the time given by the server instead of the system clock (so no new Date() usage, but creating dates based on an API call, see #531 and the /time API defined on the backend) ...
- Tests!! I did not add a single test for to this, and it will be challenging to test some things like late payments since that requires time manipulation of some kind
Currently, late payments can be manually tested by adjusting the system clock. Can we leave the system clock as an option at least until we have other working tests for late payments?
Currently, late payments can be manually tested by adjusting the system clock. Can we leave the system clock as an option at least until we have other working tests for late payments?
Oh, yeah, we will have special code I'm guessing for testing purposes to adjust the clock, not based on system time but based on whatever we need to do the tests.
As for the May 2020 update - I left answers to these questions on PR #900 but maybe it got buried, I'll copy paste the answers here:
Figuring out what to do when a payment is marked as not received (no proper followup to this) — @mmbotelho @dotmacro please play with it and see if you can offer suggestions
This flow is specified on Figma - Read the orange notes there for more info! If you still have questions, let me know!
Cancelling payments? Not sure how that flow is supposed to work, wasn't clear from Figma (cc @mmbotelho)
For manual payments, the payment is no longer “done”, therefore it returns to its original state in the todo page. It’s basically an undo.
@taoeffect, technical question:
While I'm implementing the logic for PaymentDetail.vue
(the modal), I realized that the monthstamp
should also include the year. E.g 04
(May) should be 2020-04
.
This is needed in the UI where it says "Relative to" and also to be future proof through the years. (ex: December 2019 to Jan 2020)
@sandrina-p The monthstamp
already does include the year...?
@taoeffect, apologize me, for some reason when I checked I got the idea it was not there, but it is. Nevermind! 🤦♀️ 🤦♀️
While playing with payments, I noticed multiple scenarios with unexpected outcomes regarding payments distribution:
Chunk A: When someone updates their income details after a payment is already made:
Scenario 1:
- Create a group with $1000 mincome and 3 members:
- u1: pledge 100$
- u2: Income 925$ (a.k.a needs $75)
- u3: no income details added.
- Login u1 and send $75 to u2. - The payment goes as expected.
- Switch to u3 and add income details: Income 950$ (a.k.a needs $50)
- Result: It shows "You'll receive $40" in the graphic summary.
- Expected: Should receive only $25 from u1 (100 - 75)
- Switch to u1 and go to the payments page.
- Result: Shows 1 payment to u3 of $40. The sidebar says "Amount sent $75 out of $115"
- Expected: It should show 1 payment to u3 of $25. The sidebar should say "Amount sent $75 of $100"
Scenario 2:
- Create a group with $1000 mincome and 3 members:
- u1: pledge 100$
- u2: Income 900$ (a.k.a needs $100)
- u3: no income details added.
- Login u1 and send $100 to u2. - The payment goes as expected.
- Switch to u3 and add income details: Income 950$ (a.k.a needs $50)
- Result: It shows "You'll receive $33.333" in the graphic summary.
- Expected: Should not receive anything from u1 because u1 already pledge all their money to u2.
- Switch to u1 and go to the payments page.
- Result: Shows 1 payment to u3 of $33.33. The sidebar says "Amount sent $100 out of $133"
- Expected: Don't show any "todo" payments. The sidebar should say "Amount sent $100 of $100"
Scenario 3:
(EDIT by Greg: Steven noticed an issue with Scenario 3, please read this comment)
- Create a group with $1000 mincome and 3 members:
- u1: pledge 100$
- u2: Income 950$ (a.k.a needs $50)
- u3: no income details added.
- Login u1 and send $25 to u2 (a partial payment). - The payment goes as expected.
- Switch to u3 and add income details: Income 700$ (a.k.a needs $300)
- Result: It shows "You'll receive $85.71" in the graphic summary.
- Expected: It shows "You'll receive $75" in the graphic summary. Why? It's the result of
85.71 - (25 - 14.29)
. The u2 now should only receive $14.29 instead of the needed $50. But u1 already sent $25, so the difference should be discounted from $85.71.
- Switch to u1 and go to the payments page.
- Result: Shows 1 "todo" payment to u3 of $85.71. The sidebar says "Amount sent $25 out of $110.71"
- Expected: Shows 1 "todo" payment to u3 of $75. The sidebar says "Amount sent $25 out of $100"
Scenario 4:
- Create a group with $1000 mincome and 3 members:
- u1: pledge 100$
- u2: income 950$ (a.k.a needs $50)
- u3: income 900$ (a.k.a needs $100)
- Login u1 and send $50 to u3 (a partial payment). - The payment goes as expected.
- Change the pledge amount from $100 to $50.
- Result: Shows 1 "todo" payment to u2 of $16.67. The sidebar says "Amount sent $50 out of $66.67"
- Expected: Don't show any payment to u3 because u1 already pledge all their money.
Scenario 4.1: (continuation)
- Invite a new member u4, who can pledge $150.
- Result_1: u4 is asked to send 35.70 to u2 and 75 to u3.
- Expected_1: u4 should be asked to send $50 to u2 and $50 to u3.
- Result_2: u3's payments page says "Amount received $50 out of $125".
- Expected_2: u3's payments page should say "Amount received $50 out of $100".
- Result_3: u1 has a payment to do to u2 of $12.50
- Expected_3: u1 should have no "todo" payments because u1 already pledge all their money.
A lot of similar "buggy" scenarios can be found... I didn't test when someone leaves the group or how re-distribution works regarding late payments but I'd assume it's also buggy.
Chunk B: Changing group mincome
I also found other bugs that happen when the group's mincome is changed. The income details of each user may not make sense anymore and lead to bugs. For example:
Scenario 1:
- Create a group with $1000 mincome and 2 members:
- u1: pledge 100$
- u2: Income 950$ (a.k.a needs $50)
- Change the mincome from $1000 to $500
- Result: u1 still pledges 100$ and u2 needs "$-450" (nonsense value)
- Expected: I don't know... we never discussed this.
Scenario 2:
- Create a group with $500 mincome and 2 members:
- u1: pledge 100$
- u2: Income 450$ (a.k.a needs $50)
- Change the mincome from $500 to $750
- Result: u1 still pledges 100$ and u2 needs "$300"
- Expected: Same, no idea. Maybe clear everyone's mincome and ask to fill it again? Or automatically update everyone's income details based on the new mincome and ask the user to double-check it?
Here are my thoughts on this: In the first 3 scenarios, the problem seems to be that that whenever someone new adds their income details or someone changes their income details, the distribution is re-calculated but payments already sent in that month are not accounted for. I can think of 2 solutions for this:
- Changes to the income details will only take place in the next month;
- Make the distribution algorithm account for payments that were already sent.
Now, the last 2 scenarios. Scenario 1 group b (😅):
Let's think about how we calculate how much money someone needs. We subtract their income from the mincome, correct? In this case, 500-950 is in fact negative $450. In human speak, however, what's happening here is that u2 now has $450 more than the mincome of the group. I believe we should treat it as the person no longer needs to receive a mincome, but because they have not added a pledge, they are a user that is "sending" mincome, but has a $0 pledge.
Scenario 1 group b (😅): u1 is still pledging $100 and will only meet 1/3 of the new u2 needs
Making a distribution algorithm that accounts for payments already sent (or accounting new members, or members that leave) seems complex, especially for the prototype... but @taoeffect can better decide that.
I'd agree that changes to the new income details should only take place in the next month, but that does not make sense when new members join the group and want to pledge immediately in that month. It's not very UX friendly...
(EDIT): I'm glad we have a "Dismiss payment" button. That can "solve" many of the incorrect payment values.
Regarding the chunk b of scenarios...
u1 is still pledging $100 and will only meet 1/3 of the new u2 needs
Not exactly. It depends a lot.
- In the "mathematical approach" u1 wouldn't pledge anymore. This assuming that their income was $600 (500 + 100). So, now u1 would need $150 (750-600), but this might not be the truth...
- IRL alternative A, u1 may have a huge income (let's say $10k) and doesn't mind to actually increase even more their pledging amount to support a bigger group mincome.
- IRL alternative B, u1 has already an income of $900 but now with a group mincome of $750, they don't feel comfortable in pledging to the group anymore.
I think we shouldn't change automatically each member's income details because we don't know enough about them and each personal financial life is unique regardless of how much a member pledges/needs. TLDR: In my opinion, the safer option is to reset the income details or mark them as "stale" and ask the user to verify/update it. And only then, when all members had verified their income details, a new group distribution can be generated.
(EDIT): Another idea for that: (not sure if good though) When the user votes in a mincome proposal, ask them to update their income details, just in case the proposal gets approved. This way we don't need to guess new incomes details and we don't need to notify/prompt the user later to update their income details.
In my opinion, the safer option is to reset the income details or mark them as "stale" and ask the user to verify/update it. And only then, when all members had verified their income details, a new group distribution can be generated.
This sentence contradicts the sentence that came immediately before it.
Let's break this down.
Imagine a group that has a $1000 mincome. The group passes a proposal to change the mincome to $500. What is expected to happen is for pledges to stay the same and for users who were receiving contributions before BUT noww are abvove the mincome threshold, no longer receive mincome.
Resetting everyone's income details is not a good solution at all, because our app should be smart enough to calculate what should happen without harming anyone, and not force people to come fill out probably the same information they entered before. That would be very frustrating.
I see... When the mincome is decreased, what you are saying @mmbotelho, makes total sense. But if the mincome is increased, the pledging amounts may or may not make sense anymore and that's the tricky part.
I managed to get in some time to investigate Scenario 1 above. My investigation is incomplete, and I will continue it, but I will jot down my notes here:
There are two issues:
-
The "Amount sent" shows $75 of $115 instead of $75 of $100. The reason for this is because of the following calculation:
amountTotal: todo.reduce((total, p) => total + p.amount, 0) + sent.reduce((total, p) => total + p.data.amount, 0)
The main issue being that the value for
todo
is "faulty" there, as in it includes a list item for u3 who added their income details later, and doesn't take into account the fact that we have no more $ left to send them. This can be fixed either by fixing the value returned intodo
, or by using aMath.min
operation. The more "correct" way to fix this, I am guessing, is to havetodo
contain the correct values that take into account that we cannot at this time send any $ to u3, and fixing it that way may fix many of the other issues you noticed. -
The TODO list contains an entry for $40 to u3 when it should contain an entry for $25 to u3 instead. I'm pretty sure this is a problem with how the
groupIncomeDistribution
calculates the adjusted distribution, i.e. here:if (adjusted) { // if this user has already made some payments to other users this // month, we need to take that into account and adjust the distribution. // this will be used by the Payments page to tell how much still // needs to be paid (if it was a partial payment). for (const p of dist) { const alreadyPaid = getters.paymentTotalFromUserToUser(p.from, p.to, monthstamp) // if we "overpaid" because we sent late payments, remove us from consideration p.amount = saferFloat(Math.max(0, p.amount - alreadyPaid)) } dist = dist.filter(p => p.amount > 0) }
Note that it's adjusting for whether u1 payed u2 previously, and not adjusting for u3 at all. So this math needs to be fixed.
Am getting closer to fixing it. Now the only thing left is to not make it a partial payment:

var dist = incomeDistribution(currentIncomeDistribution, mincomeAmount)
if (!adjusted) {
return dist
} else {
// if this user has already made some payments to other users this
// month, we need to take that into account and adjust the distribution.
// this will be used by the Payments page to tell how much still
// needs to be paid (if it was a partial payment).
const carried = {}
const adjustedDist = []
for (const p of dist) {
const alreadyPaid = getters.paymentTotalFromUserToUser(p.from, p.to, monthstamp)
const carryAmount = p.amount - alreadyPaid
// if we "overpaid" because we sent late payments, remove us from consideration
p.amount = saferFloat(Math.max(0, carryAmount))
// calculate our carried adjustment (used when distribution changes due to new users)
if (carried[p.from]) {
carried[p.from].total += p.amount
} else {
carried[p.from] = { carry: 0, total: p.amount }
}
if (carryAmount < 0) {
carried[p.from].carry += -carryAmount
}
}
// we loop through and proportionally subtract the amount that we've already paid
for (const p of dist) {
if (p.amount > 0) {
const c = carried[p.from]
p.amount = saferFloat(p.amount - (c.carry * p.amount / c.total))
adjustedDist.push(p)
}
}
// console.debug('adjustedDist', adjustedDist, 'carried', carried)
return adjustedDist
}
This code should be equivalent to that but with a smaller diff from master:
var dist = incomeDistribution(currentIncomeDistribution, mincomeAmount)
if (adjusted) {
// if this user has already made some payments to other users this
// month, we need to take that into account and adjust the distribution.
// this will be used by the Payments page to tell how much still
// needs to be paid (if it was a partial payment).
const carried = Object.create(null)
for (const p of dist) {
const alreadyPaid = getters.paymentTotalFromUserToUser(p.from, p.to, monthstamp)
const carryAmount = p.amount - alreadyPaid
// ex: it wants us to pay $2, but we already paid $3, thus: carryAmount = -$1 (all done paying)
// ex: it wants us to pay $3, but we already paid $2, thus: carryAmount = $1 (remaining to pay)
// if we "overpaid" because we sent late payments, remove us from consideration
p.amount = saferFloat(Math.max(0, carryAmount))
// calculate our carried adjustment (used when distribution changes due to new users)
if (!carried[p.from]) carried[p.from] = { carry: 0, total: 0 }
carried[p.from].total += p.amount
if (carryAmount < 0) carried[p.from].carry += -carryAmount
}
// we loop through and proportionally subtract the amount that we've already paid
dist = dist.filter(p => p.amount > 0)
for (const p of dist) {
const c = carried[p.from]
p.amount = saferFloat(p.amount - (c.carry * p.amount / c.total))
}
// console.debug('adjustedDist', adjustedDist, 'carried', carried)
}
return dist
This is the other "more correct" solution that I tried:
// in groupIncomeDistribution.js
const alreadyPaid = adjusted ? getters.paymentTotalFromUser(username, monthstamp) : 0
currentIncomeDistribution.push({
name: username,
amount: saferFloat(amount - alreadyPaid)
})
// in getters:
// identical to paymentTotalFromUserToUser, except it adds up all payments to all users
paymentTotalFromUser (state, getters) {
return (fromUser, paymentMonthstamp) => {
const payments = getters.currentGroupState.payments
const monthlyPayments = getters.groupMonthlyPayments
let total = 0
const { paymentsFrom, mincomeExchangeRate } = monthlyPayments[paymentMonthstamp] || {}
if (!paymentsFrom) return total
const paymentsFromUser = paymentsFrom[fromUser]
if (!paymentsFromUser) return total
for (const paymentsToUser of Object.values(paymentsFromUser)) {
for (const hash of paymentsToUser) {
const payment = payments[hash]
var { amount, exchangeRate, status } = payment.data
if (status !== PAYMENT_COMPLETED) continue
const paymentCreatedMonthstamp = ISOStringToMonthstamp(payment.meta.createdDate)
// if this payment is from a previous month, then make sure to take into account
// any proposals that passed in between the payment creation and the payment
// completion that modified the group currency by multiplying both month's
// exchange rates
if (paymentMonthstamp !== paymentCreatedMonthstamp) {
exchangeRate *= monthlyPayments[paymentCreatedMonthstamp].mincomeExchangeRate
}
total += amount * exchangeRate * mincomeExchangeRate
}
}
return saferFloat(total)
}
},
The idea being, subtract what every user has already paid, from their "available" funds, before running their data through var dist = incomeDistribution(currentIncomeDistribution, mincomeAmount)
. Then, just let that function do all the distribution work as normal.
I think this is the more correct way to do it. However, I could have some mistaken assumptions about some of the semantics in the system. Either way, when I tried to verify manually that this had any affect on Scenario 1, I still saw the same results as before. This is what I meant in my last comment last week on Slack.
Trying to translate Scenario 1 to a Cypress test. Got this far, but it gets stuck after u1 adds his income details.
// https://github.com/okTurtles/group-income-simple/issues/763#issuecomment-656250338
it.only('scenario 1', () => {
cy.visit('/')
// Create a group with $1000 mincome and 3 members:
cy.giSignup(`u1-${userId}`, { bypassUI: true })
cy.giCreateGroup(groupName, { mincome: 1000, bypassUI: true })
cy.giGetInvitationAnyone().then(url => {
invitationLinks.anyone = url
})
// u1: pledge 100$
setIncomeDetails(true, 100)
cy.giLogout()
// u2: Income 925$ (a.k.a needs $75)
cy.giAcceptGroupInvite(invitationLinks.anyone, { username: `u2-${userId}`, groupName, bypassUI: true, shouldLogoutAfter: false })
setIncomeDetails(false, 925)
cy.giLogout()
// u3: no income details added.
cy.giAcceptGroupInvite(invitationLinks.anyone, { username: `u3-${userId}`, groupName, bypassUI: true, shouldLogoutAfter: false })
// Login u1 and send $75 to u2. - The payment goes as expected.
cy.giSwitchUser(`u1-${userId}`, { bypassUI: true })
cy.getByDT('paymentsLink').click()
cy.getByDT('recordPayment').click()
cy.getByDT('modal').within(() => {
cy.getByDT('payRow').eq(0).find('label[data-test="check"]').click()
cy.get('button[type="submit"]').click()
cy.getByDT('successClose').click()
cy.getByDT('closeModal').should('not.exist')
})
})
Fixed the aforementioned bug: the code inside the .then
was running after the code which used the data it mutated. Putting the rest of the code inside the promise's callback fixed it.
Finished creating a test in 6a2e22f that mimics Scenario 1 as far as I understood it. (The text in line 315 didn't mention $40 as line 316 has but I hope that's correct, because that's what the test sees.)
The solution @taoeffect posted does solve this, but only when adjusted
is true. In cae467a I had to change ourContributionSummary
to access a version of groupIncomeDistribution
that sets adjusted
to true.
My only concern is that I'm not sure whether all views which use ourContributionSummary
should have an adjusted view of this month's distribution. The only views which use it are:
- ProfileCard: uses
receivingMonetary
to determine whether to show "Edit payment info" link - ContributionsWidget: this doesn't seem to be used, except in the "components" key of GroupDashboard, but I still can't figure out what that key is used for in Vue
- Contributions: this is what we just solved
The "more correct" solution I listed above doesn't actually make the tests pass at all. It's a bit concerning to me because I thought it should actually resolve the issue, and more reliably (because it adjusts the data before sending it into existing machinery, instead of trying to adjust it after the data comes out), which shows that I'm missing something.
This passes all 3 tests in the Chunk A: When someone updates their income details after a payment is already made
section.
The "Scenario 4.1: (continuation)" section fails. But it should probably be renamed to "Chunk 2: When someone is added to a group" or something, because that's why it fails.
I haven't yet got to Chunk B: Changing group mincome
.
Regarding Chunk B: Scenario 1:
What should happen in this situation?
- group has mincome = 1000
- u1 pledges 100
- u2 needs 50
- group changes mincome to 500
IMO, neither setting should be changed:
- u1 can still pledge 100 safely, although they may want to increase it
- u2 is recorded as "making 950" in the system, which isn't untrue now, they simply don't pledge anything.
Instead, I suggest:
- u1 can probably remain pledging 100 safely, until they change it.
- the system simply calculate u2 as needing nothing (since 500 - 950 <= 0) and do all other calculations as normal
However, there's one thing to consider about this from a social perspective: They may have agreed to change the mincome because of a decrease in one or more members' incomes. Thus:
- If u1 decreased income (e.g. to 300), then the 100 pledge may not be safe anymore. They may need an opportunity to change this. Thus, we may want to "pause" their pledge, until they can verify or revise it.
- If u2 decreased income, they're already not pledging anything, so there's less "danger", but they may need/want to record this change in the system. Thus we may want to "ping" them and let them know they can change it, but continue to assume they can contribute nothing. However, their need may have increased, e.g. if they decreased to 300, then they need 200 now, not 50.
Overall, because of the above thoughts, I think that if the group votes to change the mincome, we should immediately pause all activity in the system and "invalidate" everyone's status (without erasing it in the system), so that no actions happen based on it, until they have a chance to look at it and revise their info.
What should happen in this situation?
So regardless of what happens u2 must have its settings switched from needing income to pledging something.
So the code to do that must be implemented, and the only question remaining is what amount to set them as pledging by default. This too is fairly easily answered by observing the following: u1's pledge is not being altered, it remains at 100. We therefore cannot switch u2 to pledging more than them, which is what would happen if we converted the excess amount above 500 into a pledge. Therefore we will simply set them as pledging 0.
And what happens if there is a third member in the group, u3, who needs 150 after mincome changes, but u1's pledge doesn't supply that fully (150 - 100 = 50 left)? Should u2 be bumped up to pledging 50 automatically?