[Spree Upgrade] Fix adjustments sections in admin order edit page #3445
Conversation
Add missing form tag and OC and shops injectors on order form to make the OC field, the distributor field and the update button work
…pec in admin orders spec
luisramos0
force-pushed
the
2-0-order-adjusts
branch
2 times, most recently
from
2ba2d21 to
0daac42
Compare
luisramos0
changed the title
…t page so that user is aware of adjustments included in the price like for example tax rates This change happens in spree v2.2 together with other changes (like adding the, not yet available in v2.0, shipment_adjustments this way). See commit for more details: spree/spree@636d87d
luisramos0
force-pushed
the
2-0-order-adjusts
branch
from
0daac42 to
3ed6aea
Compare
| @@ -0,0 +1,14 @@ | |||
| - if adjustments.present? | |||
There was a problem hiding this comment.
what if instead, we move this conditional to wrap the lines 7 and 8 of app/views/spree/admin/orders/_form.html.haml? this would decouple this view from the details of adjustments and make it more maintainable. It feels like a responsibility that _adjustments.html.haml shouldn't be aware of and can save us some CPU cycles.
There was a problem hiding this comment.
we would require 2 if statements in _form (see that "adjustments" here are different lists in _form)
if in spree v2.2. with shipment_adjustments, that would make 3 if statements.
There was a problem hiding this comment.
Yes, I know but that is something we would need to fix in _form. If it gets a bit messy is not _adjustments.html.haml responsibility.
There was a problem hiding this comment.
I created #3475 for this.
| %th= Spree.t('amount') | ||
| %tbody#order-charges.with-border | ||
| - adjustments.each do |adjustment| | ||
| - if (adjustment.originator_type != 'Spree::ShippingMethod') && !(adjustment.originator_type == 'Spree::TaxRate' && adjustment.amount == 0) |
There was a problem hiding this comment.
Along the same lines, this couples a lot this view to the inner details of the adjustment. If we don't want to render anything for these kinds of adjustments we can avoid rendering the table altogether right?
We could accomplish this by loading from DB or filtering only the adjustments we care about and passing those in the adjustments local. This would make things a lot more maintainable IMO.
There was a problem hiding this comment.
yes, I agree, I noticed that problem. this is a problem in the spree code.
my decision was not to deviate too much from the spree code...
do you think I should do some magic in _form to get only adjustments that pass the conditional?
to be honest, instead, I think we should aim to remove these conditionals, these views should just show what's in the DB...
There was a problem hiding this comment.
do you think I should do some magic in _form to get only adjustments that pass the conditional?
to be honest, instead, I think we should aim to remove these conditionals, these views should just show what's in the DB...
I would just load from DB those that we care about in this partial (shipping method nad tax rate) or do you mean we should display all kinds of adjustments?
There was a problem hiding this comment.
I created #3476 for this
There was a problem hiding this comment.
ok, thank for creating the issue. I think this can definitely be improved!
There was a problem hiding this comment.
yes, I meant that we should display all kinds of adjustments.
|
🙈 I was totally convinced I was the second reviewer and this was blocked due to my review |
|
we can still have @mkllnk's review and I can include Maikels feedback in a follow up PR. |
| %thead | ||
| %th= Spree.t('name') | ||
| %th= Spree.t('amount') | ||
| %tbody#order-charges.with-border |
There was a problem hiding this comment.
It is now possible for the HTML ID order-charges to appear more than once in the page. I see that this is also an issue with in the 2-2-stable version of the file, but it isn't valid HTML.
There was a problem hiding this comment.
yeah, good catch. I am not sure what to do? I feel tempted to add this to one of the tech debt issues that Pau created above. maybe #3475 ?
There was a problem hiding this comment.
Sounds good to me @luisramos0
There was a problem hiding this comment.
Hmm, it isn't tech debt though. It could get in quicker than #3475, so I would create a new issue for it. 🙂 I'll do that in a bit.
There was a problem hiding this comment.
I just added this to #3477
both your points need to be fixed now @kristinalim they are not tech debt.
| %th= Spree.t('name') | ||
| %th= Spree.t('amount') | ||
| %tbody#order-charges.with-border{"data-hook" => "order_details_adjustments"} | ||
| - order.adjustments.eligible.each do |adjustment| |
There was a problem hiding this comment.
The adjustments are no longer limited to those with eligible: true.
There was a problem hiding this comment.
ah, I just verified that adjustment.set_eligibility is removing adjustments with amount zero...
so, this needs to be fixed now. I will create a spree upgrade issue to fix this and the order-charges issue above.
Thanks for you review @kristinalim
There was a problem hiding this comment.
@luisramos0 I also realized that the Enterprise Fee Summary still needs to be scoped to just eligible: true. 😁 Thanks.
What? Why?
Closes #2940
Adding price_adjustments to a separate table in admin order edit page. This is mimicking what happens in v2.2. I dont think we should pull forward that commit from v2.2 because it depends on order associations not yet available in v2.0 and also because we may start customize this page to OFN needs.
What should we test?
Tax adjustments spec in #2940 (spec/features/admin/orders_spec.rb:266) is now green.
This will need to be manually tested in detail when we test v2.0 as a whole.
Dependencies
This is totally depend on #3194 so I left the commits in this PR. This PR is composed of only one commit, this one.