Skip to content
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

Proration and credit calculated amounts #1878

Merged
merged 29 commits into from
Feb 11, 2022
Merged

Conversation

tanmoyAtb
Copy link
Contributor

closes #1869


Submission checklist:

Layout

  • Change looks good in the desktop web ui
  • Change looks good in the mobile web ui

Theme

  • Components / elements inspected in light mode
  • Components / elements inspected in dark mode

@render
Copy link

render bot commented Jan 25, 2022

@render
Copy link

render bot commented Jan 25, 2022

@render
Copy link

render bot commented Jan 25, 2022

@github-actions github-actions bot added the Type: Feature Added to PRs to identify that the change is a new feature. label Jan 25, 2022
@tanmoyAtb tanmoyAtb requested review from FSM1, Tbaut and asnaith January 25, 2022 14:25
Copy link
Collaborator

@Tbaut Tbaut left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I went briefly though the code, lgtm, I'll play with it now.

Copy link
Collaborator

@Tbaut Tbaut left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking good, it's simple and does the job well IMHO! Left some more nits.

tanmoyAtb and others added 7 commits January 27, 2022 20:53
@Tbaut Tbaut enabled auto-merge (squash) January 27, 2022 15:44
@asnaith
Copy link
Contributor

asnaith commented Jan 27, 2022

This seems to remember only the last payment when performing the calculations. Is this how it should work? I'm finding it a bit confusing so I'm sure users will too.

I'm not sure this is a genuine flow that someone would do but currently -

If I purchase a premium plan for $199 then if I downgrade to a standard plan then I don't owe anything because I paid more for the premium. However, if I then try to upgrade the standard plan back to premium I will have to pay an additional $100 as the standard account was only $99...but really the account should still have another $100 associated with it and no cost to go back to premium because the initial payment was $199.

Similarly, I purchase an annual standard plan for $99 and then I switch to a monthly premium plan the cost is $0.00 but it's not clear to me if the previously paid $99 will mean I don't get billed for the first 5 months when on the premium plan.

Hope this makes sense!

@Tbaut
Copy link
Collaborator

Tbaut commented Jan 28, 2022

I had this thought too regarding the fact that we don't say anything about future invoices that might be impacted by the current account balance, but I figured this is something that the user intuitively understands, when telling them how much their account balance is.

I had a somewhat similar "issue" that my balance seemed to have been reset when switching to the free plan, but I guess this might be something to discuss with the api team.

@tanmoyAtb
Copy link
Contributor Author

@asnaith Thanks for bringing this up. I noticed the first flow mismatch as well. However the calculations are fully done on the API side. Can you post the two flows on the files channel and we can discuss it with the team.

also we're only getting the next payment amounts from the API. IMO thats sufficient for the user to understand that the next few payments will be adjusted.

@tanmoyAtb tanmoyAtb disabled auto-merge January 28, 2022 11:29
Copy link
Contributor

@FSM1 FSM1 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Works great. The only thing I would change here is some of the labels - instead of
image

I would display the following:

Plan price: $ 19.99
Credit used: $9.99
--------------------
Payment Due: $10.00

@tanmoyAtb
Copy link
Contributor Author

Works great. The only thing I would change here is some of the labels - instead of image

I would display the following:

Plan price: $ 19.99
Credit used: $9.99
--------------------
Payment Due: $10.00

@FSM1 do you mean credit used or credit remaining ?

@tanmoyAtb tanmoyAtb added the Status: Do Not Merge 🛑 Added to PRs that are not allowed to be merged. label Jan 31, 2022
@FSM1
Copy link
Contributor

FSM1 commented Jan 31, 2022

Works great. The only thing I would change here is some of the labels - instead of image
I would display the following:

Plan price: $ 19.99
Credit used: $9.99
--------------------
Payment Due: $10.00

@FSM1 do you mean credit used or credit remaining ?

I was just thinking of that... how about we go for something like the:

Plan price: $ 19.99
Credit used: $9.99
--------------------
Payment Due: $10.00
Credit remaining: $ 0 (Only display if there is still credit remaining - i.e. Payment Due === 0

@tanmoyAtb
Copy link
Contributor Author

Works great. The only thing I would change here is some of the labels - instead of image
I would display the following:

Plan price: $ 19.99
Credit used: $9.99
--------------------
Payment Due: $10.00

@FSM1 do you mean credit used or credit remaining ?

I was just thinking of that... how about we go for something like the:

Plan price: $ 19.99
Credit used: $9.99
--------------------
Payment Due: $10.00
Credit remaining: $ 0 (Only display if there is still credit remaining - i.e. Payment Due === 0

Looks relatable to me, I'll update this once API makes the changes for credit remaining and updates the field names.

@FSM1 FSM1 self-requested a review February 8, 2022 18:08
@tanmoyAtb
Copy link
Contributor Author

tanmoyAtb commented Feb 11, 2022

Hey guys, The PR is good to be reviewed IMO. As long as the terminology looks relatable to you, Its good to go.

@FSM1 FSM1 removed the Status: Do Not Merge 🛑 Added to PRs that are not allowed to be merged. label Feb 11, 2022
@tanmoyAtb tanmoyAtb enabled auto-merge (squash) February 11, 2022 14:54
@tanmoyAtb tanmoyAtb merged commit 0c55db8 into dev Feb 11, 2022
@tanmoyAtb tanmoyAtb deleted the feat/price-calculations-1869 branch February 11, 2022 14:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Type: Feature Added to PRs to identify that the change is a new feature.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Display any applicable credit
5 participants