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.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

steveoliver created an issue. See original summary.

mglaman’s picture

Component: Price » Order

What 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.

    $element['definition'] = [
      '#type' => 'container',
      '#weight' => 2,
      '#states' => [
        'invisible' => [
          'select[name="' . $states_selector_name . '"]' => ['value' => '_none'],
        ],
      ],
    ];
    $element['definition']['label'] = [
      '#type' => 'textfield',
      '#title' => $this->t('Label'),
      '#default_value' => ($adjustment) ? $adjustment->getLabel() : '',
    ];
    $element['definition']['amount'] = [
      '#type' => 'commerce_price',
      '#title' => t('Amount'),
      '#default_value' => ($adjustment) ? $adjustment->getAmount()->toArray() : NULL,
      '#states' => [
        'optional' => [
          'select[name="' . $states_selector_name . '"]' => ['value' => '_none'],
        ],
      ],
    ];

Moved to "Order" component, since that contains adjustments.

bojanz’s picture

Discussed 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.

ndf’s picture

Noticed that the validation passes when saving the form a second time.
Dropping it here as a work-around that can help.

bojanz’s picture

Title: New Adjustment type '_none' should be ignored » Rewrite AdjustmentDefaultWidget to use #ajax
Category: Bug report » Task
Priority: Normal » Major
alexpott’s picture

Status: Active » Needs review
FileSize
4.31 KB

Here'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.

Status: Needs review » Needs work

The last submitted patch, 6: 2840134-6.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

alexpott’s picture

Ah reading #2 and #3 says let's not make the form pluggable here - fair enough - I'll open a followup to do that.

alexpott’s picture

Status: Needs work » Needs review
FileSize
3.07 KB
5.35 KB

Fixing 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.

Status: Needs review » Needs work

The last submitted patch, 9: 2840134-9.patch, failed testing. View results

alexpott’s picture

Issue summary: View changes
Status: Needs work » Needs review
FileSize
5.13 KB
10.48 KB

The 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.

smccabe’s picture

Agree on the remove button, will try and review later today.

alexpott’s picture

Wow how hard is it to add a remove button!

alexpott’s picture

Status: Needs review » Needs work

The last submitted patch, 14: 2840134-14.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

alexpott’s picture

Status: Needs work » Needs review
FileSize
6.94 KB
23.05 KB

Now with more tests and passing tests.

heddn’s picture

Pasting in feedback from #1038316-159: Allow for deletion of a single value of a multiple value field

  1. +++ b/modules/order/src/Plugin/Field/FieldWidget/AdjustmentDefaultWidget.php
    @@ -64,39 +70,197 @@ class AdjustmentDefaultWidget extends WidgetBase {
    +    $wrapper_id = Html::getUniqueId($id_prefix . '-add-more-wrapper');
    

    This isn't only used for adding any more. Its also used for removing.

  2. +++ b/modules/order/src/Plugin/Field/FieldWidget/AdjustmentDefaultWidget.php
    @@ -64,39 +70,197 @@ class AdjustmentDefaultWidget extends WidgetBase {
    +      $elements['add_more'] = [
    ...
    +        '#name' => strtr($id_prefix, '-', '_') . '_add_more',
    

    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.

alexpott’s picture

@heddn thanks for the review.

  1. Fixed
  2. I'd prefer not to because of things like form .field-add-more-submit in form.css.

Made some further tidy ups to the code.

travis-bradbury’s picture

FileSize
79.47 KB

Alex, 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:

  1. Add a product to the order. The whole POS page is replaced but the adjustments widget doesn't show the adjustment.
  2. Click Add adjustment. Now we see the existing adjustment but don't have a place to enter a new adjustment.
  3. Click Add another adjustment. Now we can add an actual new adjustment.
    1. 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.

alexpott’s picture

@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.

subhojit777’s picture

I was able to replicate the problem with 8.x-2.0-beta2 POS

bojanz’s picture

+    $cardinality = $this->fieldDefinition->getFieldStorageDefinition()
+      ->getCardinality();
+    $parents = $form['#parents'];
+
+    // Determine the number of widgets to display.
+    switch ($cardinality) {
+      case FieldStorageDefinitionInterface::CARDINALITY_UNLIMITED:
+        $field_state = static::getWidgetState($parents, $field_name, $form_state);
+        $max = $field_state['items_count'] - 1;
+        $is_multiple = TRUE;
+        break;
+
+      default:
+        $max = $cardinality - 1;
+        $is_multiple = ($cardinality > 1);
+        break;
+    }

Could the patch be simplified by always assuming unlimited cardinality? Do we have a use case for a limited number of adjustments?

krystalcode’s picture

The 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

alexpott’s picture

Re #22 unless we lock it down in the API/UI that cardinality always has to be unlimited then I wouldn't change that.

bojanz’s picture

@alexpott
Yeah, let's do that (lock down the cardinality to unlimited). There is no use case for a limited number of adjustments.

alexpott’s picture

Tested 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?

alexpott’s picture

I 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:

  /**
   * {@inheritdoc}
   */
  public function process(OrderInterface $order) {
    $store_currency = $order->getStore()->getDefaultCurrencyCode();
    $order->addAdjustment(new Adjustment([
      'type' => 'fee',
      'label' => t('Bottle deposit'),
      'amount' =>  new Price(10, $store_currency),
    ]));
  }

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.

alexpott’s picture

Finally 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.

Status: Needs review » Needs work

The last submitted patch, 28: 2840134-28.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

alexpott’s picture

Status: Needs work » Needs review
FileSize
1.08 KB
23.06 KB

Ok we need to fix POS inside POS and revert #28.