Open taoeffect opened 4 years ago
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:
(EDIT by Greg: Steven noticed an issue with Scenario 3, please read this comment)
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.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.
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:
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:
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.
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 in todo
, or by using a Math.min
operation. The more "correct" way to fix this, I am guessing, is to have todo
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:
receivingMonetary
to determine whether to show "Edit payment info" linkThe "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?
IMO, neither setting should be changed:
Instead, I suggest:
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:
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?
No. In this scenario if there are any members who still need mincome, they will need less to help them achieve the new mincome. We never increase anyone's pledge. We only switch a person from needing mincome to not needing mincome (pledging 0), and inform them that they are now welcome and encouraged to increase their pledge if they can.
It looks like the code fix above did not actually fix the failing scenarios. The code behaves the same on all of them with or without it. It was actually the code in state.js in cae467a that fixed them. So we may be able to revert the code fix above. Unless it fixes another scenario, but I no longer remember how it works or what it does.
Scenario 3:
- 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.
Over in Slack @sdegutis noticed correctly that this scenario isn't quite right, since after u3 enters their income details, there will be $75 left over that now needs to be split between u2 and u3, and so u3 cannot be expected to receive the full $75 that's remaining.
Instead, we likely need to run the distribution algorithm "again" over the remaining amounts needed between u2 and u3 (u2 needing $25, and u3 needing $300), so the remaining $75 will be split proportionally among those (and off the top of my head I don't know what that amounts to, but most of the $75 would go to u3, and some of it would go to u2).
Thanks to @sdegutis for noticing this (and @sandrina-p for coming up with these scenarios!).
Although my patch in #1000 fixes those scenarios in the unit tests, the Scenario 3 Cypress test is failing, in a strange way:
Sender | Recipient | Unit Test | Cypress Test |
---|---|---|---|
u1 | u2 | $5.769230769230769 | $15 |
u1 | u3 | $69.23076923076924 | $60 |
(The other Scenarios are passing in the Cypress tests.)
It doesn't make sense why the same function is having two different results. It seems there are more calculations being done here. This is an example of where I would reach for a debugger to step through the code and find out what's happening.
Besides that, at least two other Cypress tests are now also failing:
They have different amounts of payments with different amounts, and they show "-1 out of 2" for payments sent.
I'll translate them into the unit tests, to see if we get a similar difference as the table above.
Based on my understanding of the proportional-split algorithm:
Based on this understanding, correct or not, the unit tests are correct, and the Cypress tests show the wrong result.
I'm not sure yet how the Cypress tests came up with this answer.
@sdegutis Have you looked at how the Vue-path connects to the lower level code? I know off-hand that it goes through some getters like ourPayments
, ourPaymentsSummary
and ourContributionSummary
. That might be where the differences are being generated.
Figured it out. The Cypress test for Scenario 3 was just written wrong. It had $900 instead of $950. So the algorithm works fine and the code is fixed.
The problem with "Payments Sent: -1 out of 2" is caused by it saying there is 1 sent payment and 2 todo payments, therefore 1 - 2 = -1. That's at least the logic in ourPaymentsSummary
which I'm not sure makes any sense. It makes sense to rather be 1 out of 2 + 1, or "sent" out of "sent + todo".
Yes I agree, that's an excellent observation! Feel free to make the necessary changes to make that display 1 out of 3 Payments (or whatever makes sense for the particular scenario), and if you'd like me to do a deep dive into the code about this, let me know on Slack and I'll hop to it!
Yes I agree, that's an excellent observation! Feel free to make the necessary changes to make that display 1 out of 3 Payments (or whatever makes sense for the particular scenario), and if you'd like me to do a deep dive into the code about this, let me know on Slack and I'll hop to it!
Actually, what should happen here?
Should it be 1 out of 2, because there were 2 to begin with, and you only did a partial? Or should it include the partial payment that you sent, plus the current TODOs, and say 1 out of 3?
I think the way that it was designed is that part of the progress bar is supposed to turn a lighter shade of blue when there is a partial payment that exists, and the right-side of the "out of" should be the total number of users who you are expected to pay (including any late payments from last month!). So if there are 2 users you need to pay this month, then it should always show 2.
Also, it seems that the "user1 sends $71.43 to user2 (total)" test in Cypress is incorrect. It should end up having two TODOs, not one, because although user1 sent user2 some money, they still owe user2. Unless user4, who has some pledge, is supposed to take care of one of user2 or user3 fully, so that user1 only has one to take care of? I'm not sure what this test should be, and if it's correct or not.
Unless user4, who has some pledge, is supposed to take care of one of user2 or user3 fully, so that user1 only has one to take care of?
I believe there is never a scenario where, when using the proportional distribution algorithm, those who are pledging ever end up covering everything for a specific user (EDIT: unless there is only 1 person pledging). Rather, the proportional distribution algorithm is designed to ensure that everyone who is pledging will send some amount to all users who are classified as needing funds.
I think the way that it was designed is that part of the progress bar is supposed to turn a lighter shade of blue when there is a partial payment that exists
That's happening now, yeah.
and the right-side of the "out of" should be the total number of users who you are expected to pay (including any late payments from last month!). So if there are 2 users you need to pay this month, then it should always show 2.
Oh. That was not what I assumed by the wording "Payments sent", which made me think that "1 out of 2" meant "1 payment sent out of 2 payments". Probably either the wording should be changed, or the purpose and semantics of this widget.
I agree, it might need a wording change... /cc @dotmacro
It's supposed to be like an "invoice", and so really there are 2 invoices that need to be paid, and if you partially pay it, there's still a single total invoice that needs payment.
Good point. If it's like an invoice, then only 2 payments were needed, even if there were 7 partials, so 7 out of 2 would make sense there in that case. Still, it just seems strange.
Good point as well, we can either change the wording, or change the number. /cc @dotmacro @mmbotelho your input (whenever you'd like) would be valuable on this!
Okay I figured some things out by turning this Cypress test into this unit test.
The Cypress test assumes that user1 should have a single todo payment of $178.57 to user3, but the unit tests shows that it actually results in two payments, split evenly between user2 and user3. It also has user4 splitting his money evenly between both user2 and user3.
I'm not sure what the app should be doing: have both pledgers give evenly to both needers, or have each pledger give to only one needer? I assume the first, since otherwise I don't think we can guarantee that their excess will be split evenly, in cases where the need is disproportionate, e.g. user2 needs $20 and user3 needs $100, but user1 has $1000 and user4 only has $20, then user4 is giving all he has (to either user) but user1 could afford to give more.
The Cypress test assumes that user1 should have a single todo payment of $178.57 to user3, but the unit tests shows that it actually results in two payments, split evenly between user2 and user3. It also has user4 splitting his money evenly between both user2 and user3.
I am not super familiar with these tests since I didn't write them, and am finding it difficult to find the section you're referring to. Could you assist me by pointing to the exact minimum number of line(s) where the assertion is failing/incorrect?
I'm not sure what the app should be doing: have both pledgers give evenly to both needers, or have each pledger give to only one needer? I assume the first
Given your description, your assumption would be correct. It is indeed correct when there are 2 pledging and 2 receiving, that — using the proportional distribution algorithm we're currently using — both pledgers will proportionally give to the receivers based on how much they pledged.
In that case I've updated the Cypress tests to match the unit tests. 👍
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
What needs to be done next
TODO:
prevMonthDueDate
computed property for late payments inPaymentRowTodo.vue
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
)new Date()
usage, but creating dates based on an API call, see #531 and the/time
API defined on the backend)Done #951
ESC
key clear the search fieldDone #917
PaymentRow*.vue
files and DRY it all so that it's all stored in one of the.scss
files and not repeatedPaymentRow*.vue
files (and any other payment related files)humanDate.js
and move thehumanDate
function intotime.js
Done #918
PaymentDetails.vue
fileMonthOverview.vue
fileMath.random()
stuff in thePaymentRow*.vue
files (mostly related to payment date stuff) and replace it with the actual data