Describe your bug or feature request.

If you have previously saved an entity that uses inline conditions, like a Promotion, with a condition that has a required form element and you want to disable it, if you clear out the form element's value before disabling the condition, the form will not save. For whatever reason, the form attempts to evaluate the form elements from the condition plugin before it evaluates the checkbox on the condition to see if it's still enabled.

If a bug, provide steps to reproduce it from a clean install.

On a clean install, create a promotion, add a condition based on the customer role, and submit with any role value. Then re-edit the same promotion, uncheck whatever roles you previously selected, and then uncheck the box for the customer role condition itself. When you submit the form, the page will reload with a validation failure message that a customer role is required.

CommentFileSizeAuthor
#4 3187344-4.patch1.54 KBjsacksick
#2 commerce-fix-remove-conditions-when-remove-required-field-value-first-3187344-2.patch1.99 KBkhiminrm
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

rszrama created an issue. See original summary.

khiminrm’s picture

khiminrm CreditAttribution: khiminrm at Centarro commented
FileSize
commerce-fix-remove-conditions-when-remove-required-field-value-first-3187344-2.patch1.99 KB

@rszrama, I've found that ConditionsWidget::formElement builds default values based on items received from promotion entity's 'conditions' field values.
From EntityFormDisplay::buildForm:

if ($widget = $this->getRenderer($name)) {
        $items = $entity->get($name);
        $items->filterEmptyItems();
        $form[$name] = $widget->form($items, $form, $form_state);

Where $name in this case will be 'conditions'.

I've created patch to improve code in ConditionsWidget::formElement to compare values with user input. I'm not sure if it's the best solution for fixing the issue.

If you will check HTML markup of the elements for role options without the patch, you will see that when 'Customer role' has been unchecked, role options are not removed from the HTML, they're hidden with form states js.

khiminrm’s picture

khiminrm CreditAttribution: khiminrm at Centarro commented
Status: Active » Needs review
jsacksick’s picture

jsacksick CreditAttribution: jsacksick at Centarro commented
FileSize
3187344-4.patch1.54 KB

@rszrama: Good catch! The problem is the configuration is not properly "cleared" when unchecking the checkbox.

The attached patch seems to do the trick.

@khiminrm: I haven't actually tested your patch because the code seems to contain duplicated code from the Conditions element which doesn't seem to be necessary.

Also note that you're calling \Drupal::service('plugin.manager.commerce_condition'); which is already injected in the class ($this->conditionManager).

amateescu’s picture

amateescu CreditAttribution: amateescu at Centarro commented
Status: Needs review » Reviewed & tested by the community

This clearValues() pattern seems to be used in other places throughout Commerce, so I think #4 looks good :)

  • jsacksick committed 1ea25dd on 8.x-2.x 
    Issue #3187344 by amateescu, khiminrm, jsacksick, rszrama: Cannot remove...
jsacksick’s picture

jsacksick CreditAttribution: jsacksick at Centarro commented
Status: Reviewed & tested by the community » Fixed

Committed! Thanks everyone!

  • jsacksick committed 7af7227 on 8.x-2.x 
    Issue #3187344 followup by jsacksick: Skip applying promotion offers on...

Status: Fixed » Closed (fixed)

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