Needs work
Project:
Commerce Point of Sale (POS)
Version:
8.x-2.x-dev
Component:
Code
Priority:
Normal
Category:
Bug report
Assigned:
Unassigned
Reporter:
Created:
20 Nov 2017 at 14:25 UTC
Updated:
16 Jul 2018 at 19:30 UTC
Jump to comment: Most recent, Most recent file
Comments
Comment #2
subhojit777Could you please tell me how you checked the adjustments on Drupal 7.
Comment #3
subhojit777I think it would make sense to keep the adjustment section disabled/hidden/not-present initially.
When an order item is added, then the adjustment will be available. When an adjustment type is selected, we provide the option to select one of the order item, and we make it mandatory. I think this is how we would be able to associate an adjustment with order item.
I will try to compose an init code.
Comment #4
subhojit777Some work came up.
Comment #5
smccabe commentedI believe this issue moved to a commerce core fix and has a pending patch, is that correct @alexpott?
Comment #6
smccabe commentedpending this issue #2840134: Rewrite AdjustmentDefaultWidget to use #ajax
Comment #7
krystalcode commentedThe issue 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.
The attached patch is not really fixing the issue, but provides a temporary solution. Reload the page at the end of the AJAX request. That does break however the "set as return item" functionality.
Comment #8
krystalcode commentedPatch with temporary fix always displaying the "Set as return item" checkbox. I'm not sure what was the logic behind not displaying it always before, so I'm not sure if that's correct.
Comment #9
alexpottWorked on integrating POS and #2840134: Rewrite AdjustmentDefaultWidget to use #ajax. As far as I can see we have a problem regardless of using that patch with add adjustments using a OrderProcessor. Patch attached plus the latest on #2840134: Rewrite AdjustmentDefaultWidget to use #ajax actually makes it work the way we'd like, as far as I can tell.
Comment #11
alexpottWhoops
Comment #12
smccabe commentedPatch looks good and passes, can we add a test for this instance? setup a test product with an integrated discount? or is that something better handled by commerce itself?
Comment #13
alexpottOkay need to update this again to fix the adjustments issue better. Once that issue lands we can then add tests for this here.
Comment #14
krystalcode commentedClearing the widget state (`WidgetBase::setWidgetState([], 'adjustments', $form_state, []);`) is required when adding the first order item; however it breaks updating the quantity for an existing item or adding a second order item: it removes the added adjustment. That is because the `extractFormValues` on the base widget will add the original deltas on the widget state and consequentially (widget state not being empty anymore) the widget state will not be recreated with the items count required to render the items.
Not sure if that's a case that should be taken into account in the base widget class, or it needs to be handled differently here.
Steps to reproduce:
1. Start with a clean POS order.
2. Add an order item, the adjustment will be displayed in the widget.
3. Update the quantity for the order item, or add a new order item, the adjustment will be removed from the widget.
4. Repeat #3, the adjustments will show up again.
5. Repeat #3, the adjustments will be removed again.
Comment #15
alexpott@krystalcode nice testing. So maybe we just need to update the delta if the number of adjustments has changed?
Comment #16
alexpottI've played with doing different things here. I'm not sure we've really got many good options. Form processing and AJAX just is not designed for this type of side effect - ie. adding one thing affecting something totally different. We can keep on trying to move the point when different things occur but nothing will be as safe as #7 - ie. to redirect to the form again after adding an order item. This will allow anything to do what it needs to (ie. an order processor add an adjustment) and then the form should be correct.
Anyone else got another idea?
Comment #17
krystalcode commentedFrom previous experience working on a similar situation I had also come to a similar conclusion, that it's very difficult to make it work smoothly with AJAX.
I don't know when I'll get time to give it another go, I'd suggest we merge #7 (needs a bit more work to make it work well with the "Set as return item" functionality) and then open a separate issue of secondary priority to continue investigating whether it's possible to make it work smoothly with AJAX.
Comment #18
alexpottYeah I think that in order to make it work smoothly with AJAX we'll need to take over all the widgets so we can be sure that only one thing is changing at a time. The current mix makes it very hard to do the right thing and whilst we might make it work for adjustments - it'll be the same for coupons or any other custom field added to the order form.