Problem/Motivation

#3088597: Add a constraint to purchased_entity which checks the availability manager added support for integrating with the availability manager in the AddToCartForm thanks to entity validation. We now need the commerce_order_item_edit_quantity views field plugin to do the same.

Proposed resolution

The validate should adjust the order item quantity and then validate the entity to see if purchased_entity still is valid with the new quantity changes.

Remaining tasks

User interface changes

API changes

Data model changes

Release notes snippet

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

mglaman created an issue. See original summary.

mglaman’s picture

Status: Active » Needs review
FileSize
1.83 KB

Blind patch to at least scaffold out what the code should kind of look like.

mglaman’s picture

Status: Needs review » Needs work
jsacksick’s picture

Status: Needs work » Needs review
FileSize
7.82 KB
7.6 KB

I've been working on this today, we should definitely fix this ASAP, cause right now, if an order availability checker indicates that an order item is unavailable, the quantity update goes through, and then the item is removed on the next refresh (with a successful message saying "Your shopping cart has been updated").

I tried finding an issue for that this morning but couldn't, and then finally found yours :p.

We need tests coverage for this, and we also need to set the initial quantity back, so the logic in viewsFormSubmit() works properly.

For some reason, when looping over the violations using the getEntityViolations(), no violations are present. so I'm looping on the violations directly and it seems to work.

Attaching patch for review.

jsacksick’s picture

FileSize
7.83 KB
722 bytes
mglaman’s picture

  1. --- a/modules/cart/src/Plugin/views/field/EditQuantity.php
    +++ b/modules/cart/src/Plugin/views/field/EditQuantity.php
    
    @@ -169,6 +180,54 @@ class EditQuantity extends FieldPluginBase {
    +  public function viewsFormValidate(array &$form, FormStateInterface $form_state) {
    

    😂 had to find this, man.. Views.

        // Call the validation method on every field handler that has it.
        foreach ($view->field as $field) {
          if (method_exists($field, 'viewsFormValidate')) {
            $field->viewsFormValidate($form, $form_state);
          }
        }
    
  2. +++ b/modules/cart/src/Plugin/views/field/EditQuantity.php
    @@ -169,6 +180,54 @@ class EditQuantity extends FieldPluginBase {
    +      if (!is_numeric($quantity) || $quantity < 0) {
    ...
    +      if ($order_item->getQuantity() == $quantity || $quantity <= 0) {
    +        // The quantity hasn't changed.
    +        continue;
    

    Above we check $quantity < 0 and here we check if it is also equal to 0. The could still mean the quantity changed. Should we change

    if (!is_numeric($quantity) || $quantity < 0) {
    

    to

    if (!is_numeric($quantity) || $quantity =< 0) {
    

    and update the inline comments? This handles invalid values or purposely set to 0 quantities?

  3. +++ b/modules/cart/src/Plugin/views/field/EditQuantity.php
    @@ -169,6 +180,54 @@ class EditQuantity extends FieldPluginBase {
    +        ->filterByFieldAccess($this->currentUser)
    

    I don't think we need to filter by field access if we are then filtering by the two specified fields

  4. +++ b/modules/cart/src/Plugin/views/field/EditQuantity.php
    @@ -169,6 +180,54 @@ class EditQuantity extends FieldPluginBase {
    +      $order_item->setValidationRequired(FALSE);
    

    Is this required? This isn't a content entity form, so I don't think this affects anything.

    And, upon further digging.. it looks like this flag is mostly used when importing entities via Migrate, not even the content entity form.

  5. +++ b/modules/cart/src/Plugin/views/field/EditQuantity.php
    @@ -169,6 +180,54 @@ class EditQuantity extends FieldPluginBase {
    +      // Restore the quantity so the logic in viewsFormSubmit() proceeds with
    +      // the quantity update.
    +      $order_item->setQuantity($original_quantity);
    

    Actually, we could remove this and just update viewsFormSubmit to trust the set quantity value. We've validated it already, so the form submit no longer needs to.

jsacksick’s picture

FileSize
7.6 KB
1.63 KB

Actually, we could remove this and just update viewsFormSubmit to trust the set quantity value. We've validated it already, so the form submit no longer needs to.

Well, we can't remove the checks in viewsFormSubmit() because viewsFormValidate ignore "invalid" quantities and doesn't throw an error.

And we still need to keep ignoring quantities that haven't changed. Remember, we're checking quantities of all "rows", not just a single one.

mglaman’s picture

Status: Needs review » Reviewed & tested by the community

Ah okay. Thanks @jsacksick for clarifying Views form handling 🙂.

Moving to RTBC as my sign off, if you'd like the honors of commit.

  • jsacksick committed a305bd3 on 8.x-2.x
    Issue #3109312 by jsacksick, mglaman: Validate order items modified on...
jsacksick’s picture

Status: Reviewed & tested by the community » Fixed

Committed!

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.