-
Notifications
You must be signed in to change notification settings - Fork 942
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
Add support for arbitrary line item codes #1062
Merged
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
lyyder
force-pushed
the
custom-pricing-support
branch
7 times, most recently
from
April 4, 2019 11:54
09ab57e
to
97eb274
Compare
Gnito
reviewed
Apr 4, 2019
Gnito
reviewed
Apr 4, 2019
Gnito
reviewed
Apr 4, 2019
Gnito
reviewed
Apr 4, 2019
Gnito
requested changes
Apr 4, 2019
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 added a couple of small comments.
Have you tested this on staging against Joe Dunphy's transactions? (I wonder if they change with this PR.)
lyyder
force-pushed
the
custom-pricing-support
branch
2 times, most recently
from
April 5, 2019 08:22
74ee237
to
dbf901a
Compare
Remove the requirement that a certain kind of line item is found from a transaction.
lyyder
force-pushed
the
custom-pricing-support
branch
from
April 5, 2019 08:30
dbf901a
to
9a0f1df
Compare
lyyder
force-pushed
the
custom-pricing-support
branch
from
April 5, 2019 08:37
9a0f1df
to
3b70d2d
Compare
Update line items in test to match those returned from the API.
Add examples with arbitrary line items.
Make the subtotal line stick out a bit more.
lyyder
force-pushed
the
custom-pricing-support
branch
from
April 5, 2019 08:47
3b70d2d
to
551dff0
Compare
Merged
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Update 2019-04-05: The booking breakdown layout has been improved for a more visible sub total row, see updated screenshots
With custom pricing line items can have any sort of code as long as it begins with
line-item/
.This PR loosens the line item code validation and changes booking breakdown so that all unknown line items are rendered too.
About rendering unknown line items, initial plan was to render quantity based line items on top of subtotal and the percentage based items below it:
However, this design got a bit tricky with refunds. Previously the refund has been right after sub total and commissions have their own refund line. If percentage based unknown line items where below the sub total, should they have their own refund line in the breakdown. The simplest solution was to add everything except commissions above the sub total, refund matching those lines below sub total and commissions as they are. The main upside of this design is that the refund always matches the sub total which makes it clear to read.
So current the current design is:
And with refund:
The downside of the design is that unknown line items only present the human readable code and line total. If a line item has quantity other than one for example, it would be nice to be able to read from the breakdown. Maybe one way to show it would be: