Problem/Motivation
Use AJAX to add the adjustments for instead of state. This will eventually allow us to have different forms for different adjustment types. Right now it'll mean that the order form does not contain unnecessary fields.
Proposed resolution
Use AJAX to get adjustment form.
Remaining tasks
User interface changes
Not really
API changes
None
Data model changes
None
Original report
When an adjustment on an order is not set (to something besides '_none'), the order form does not pass validation but warns that an 'Amount' field is required (further, the amount field in question also happens to not be visible because of #states).
This is caused by the price field 'commerce_price' element requiring a 'number' value.
Not sure how to handle this.
Comment | File | Size | Author |
---|---|---|---|
#30 | 2840134-29.patch | 23.06 KB | alexpott |
| |||
#30 | 28-29-interdiff.txt | 1.08 KB | alexpott |
#28 | 25-28-interdiff.txt | 1.08 KB | alexpott |
#28 | 2840134-28.patch | 23.16 KB | alexpott |
#26 | 2840134-25.patch | 23.06 KB | alexpott |
|
Comments
Comment #2
mglamanWhat we need to _really_ do is AJAX in the adjustment type amount form when the select changes. This is the proper way to handle this, not via states. One reason I got caught up in the initial adjustment type port because I added PluginForm classes so that adjustment types could say "here is how you enter me." For example, manually adding and locking in a promotion. The promotion adjustment type would let you autocomplete search a source and "that's it" for example, make the adjustment immutable, whatever.
So we'll need AdjustmentTypePluginForm class which is the amount and type part of the form that gets embedded once the select is chosen. This lovely bit.
Moved to "Order" component, since that contains adjustments.
Comment #3
bojanz CreditAttribution: bojanz at Centarro commentedDiscussed this with Matt, we said that we don't want to make the form pluggable for now.
We definitely want to use #ajax instead of states though.
Comment #4
ndf CreditAttribution: ndf at Dx Experts for Media.Monks commentedNoticed that the validation passes when saving the form a second time.
Dropping it here as a work-around that can help.
Comment #5
bojanz CreditAttribution: bojanz at Centarro commentedComment #6
alexpottHere's a first stab - very early cut - much cleanup to take place. Let's see what break. Also I think we need to defer to the adjustment type for the form so different adjustment types can present different forms.
Comment #8
alexpottAh reading #2 and #3 says let's not make the form pluggable here - fair enough - I'll open a followup to do that.
Comment #9
alexpottFixing the non-JS test and a couple of code cleanups. Need to add JS tests. We get non-js functionality just by adding the form properly if someone clicks the add more adjustments link.
Comment #11
alexpottThe failure in
Drupal\Tests\commerce_cart\FunctionalJavascript\MultipleCart
does not happen locally - I guess this is a random fail.New patch adds a JS test for the adjustments part of the order form.
At the moment you can remove an adjustment by setting it to none - but I think maybe we should add a remove button.
Comment #12
smccabe CreditAttribution: smccabe as a volunteer and at Acro Commerce commentedAgree on the remove button, will try and review later today.
Comment #13
alexpottWow how hard is it to add a remove button!
Comment #14
alexpott@heddn showed me the way with #1038316: Allow for deletion of a single value of a multiple value field so building off that...
Comment #16
alexpottNow with more tests and passing tests.
Comment #17
heddnPasting in feedback from #1038316-159: Allow for deletion of a single value of a multiple value field
This isn't only used for adding any more. Its also used for removing.
Did we want to drop the "_more" part of this since it isn't always adding more. It is also adding the initial, first element.
Comment #18
alexpott@heddn thanks for the review.
form .field-add-more-submit in form.css.
Made some further tidy ups to the code.
Comment #19
travis-bradbury CreditAttribution: travis-bradbury at Acro Commerce commentedAlex, I'm running this patch and using the widget in commerce_pos and I have an issue where adding an adjustment is awkward. Reporting it here because I think it's related to the widget's ajax.
We're adding an adjustment to the order and it happens like so:
If I continued to the next page and came back the adjustment widget does have the existing adjustment so I think this might be related to this issue.
Comment #20
alexpott@tbradbury I've tested your scenario locally and everything works. Maybe something in the Pos. I checked out the 8.x-2.0-beta2 (released just before you left #19) version of commerce pos and reran your tests and I still could reproduce it.
Comment #21
subhojit777I was able to replicate the problem with 8.x-2.0-beta2 POS
Comment #22
bojanz CreditAttribution: bojanz at Centarro commentedCould the patch be simplified by always assuming unlimited cardinality? Do we have a use case for a limited number of adjustments?
Comment #23
krystalcode CreditAttribution: krystalcode at Acro Commerce commentedThe issue on POS happens because the submitted values are bound to the form object after the form has been built. The adjustments at that point are therefore empty.
Not come to a solution yet, but I have posted a temporary fix for POS for anyone facing this issue here: https://www.drupal.org/project/commerce_pos/issues/2924956#comment-12679569
Comment #24
alexpottRe #22 unless we lock it down in the API/UI that cardinality always has to be unlimited then I wouldn't change that.
Comment #25
bojanz CreditAttribution: bojanz at Centarro commented@alexpott
Yeah, let's do that (lock down the cardinality to unlimited). There is no use case for a limited number of adjustments.
Comment #26
alexpottTested again with commerce POS found there was a problem with how the ajax replacement was done.
It'd be great to get more understanding about #19, #21 and #23. I've tested the patch with Commerce POS and everything works. I think the problem being report occurs when you add an order item that also adds an automatic adjustment. Am I right?
Comment #27
alexpottI can reproduce the problem reported in #19 but it has nothing to do with this patch. In fact it appears to me that it does not require this patch for the problem to occur. It's because of the way that AJAX works and the commerce_pos order form uses it. I've reproduced the problem by adding a test module that has an order processor that does:
if I add an order item using the commerce_pos form the form refreshes and adds the item as expected but because of the way the AJAX for the order items works we've not added the adjustment in time for the adjustment widget to have the correct values.
So for me this is good to go and we need to work on fixing the commerce_pos form in #2924956: Handle adjustments on POS form.
Comment #28
alexpottFinally made everything play nice together! Minor change to the patch here to calculate the number of items in a better way.
See #2924956: Handle adjustments on POS form for updates to that issue to make it good for POS.
Comment #30
alexpottOk we need to fix POS inside POS and revert #28.