Problem/Motivation

If you edit an order and submit a custom adjustment (or any type) without a label you get a PHP error message.

The website encountered an unexpected error. Please try again later.</br></br><em class="placeholder">InvalidArgumentException</em>: Missing required property label. in <em class="placeholder">Drupal\commerce_order\Adjustment-&gt;__construct()</em> (line <em class="placeholder">73</em> of <em class="placeholder">modules/contrib/commerce/modules/order/src/Adjustment.php</em>). <pre class="backtrace">Drupal\commerce_order\Plugin\Field\FieldWidget\AdjustmentDefaultWidget-&gt;massageFormValues(Array, Array, Object) (Line: 368)

Proposed resolution

Make the label required in form.

Remaining tasks

User interface changes

None

API changes

None

Data model changes

None

CommentFileSizeAuthor
#18 2926563-2-17.patch3.44 KBalexpott
#18 2926563-2-17.test-only.patch1.72 KBalexpott
#13 2926563-2-13.patch2.74 KBalexpott
#12 2926563-12.patch6.95 KBsorabh.v6
#9 Schermafbeelding 2017-12-05 om 16.36.40.png23.3 KBharings_rob
#9 Edit___Drupal 3.png77.6 KBharings_rob
#9 Edit___Drupal.png117.34 KBharings_rob
#2 2926563-2.patch2.27 KBalexpott
#2 2926563-2.test-only.patch1.16 KBalexpott

Comments

alexpott created an issue. See original summary.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Status: Active » Needs review
StatusFileSize
new 2926563-2.test-only.patch1.16 KB
new 2926563-2.patch2.27 KB

The last submitted patch, 2: 2926563-2.test-only.patch, failed testing. View results

Status: Needs review » Needs work

The last submitted patch, 2: 2926563-2.patch, failed testing. View results

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Status: Needs work » Needs review

The fail for the non test-only patch in #2 seems unrelated tried to re-create locally and it does not happen.

The last submitted patch, 2: 2926563-2.test-only.patch, failed testing. View results

  • bojanz committed d5b83ab on 8.x-2.x authored by alexpott 
    Issue #2926563 by alexpott: Adjustment label is required but not on the...
bojanz’s picture

bojanz commented
Status: Needs review » Fixed

Thanks!

harings_rob’s picture

harings_rob commented
StatusFileSize
new Edit___Drupal.png117.34 KB
new Edit___Drupal 3.png77.6 KB
new Schermafbeelding 2017-12-05 om 16.36.40.png23.3 KB

This does not seem correct. Adding the required = TRUE leads to some regression.

Following steps are to reproduce:
1. Start creating order in backend.
2. Fill in the order but not the adjustments
3. Save (You cannot because of html validation)
4. Disable html validation
5. Save
6. Get an error message that the field is mandatory, but no form is highlighted
7. Select an adjustment to see the error field.

Below are some screenshots:
https://www.drupal.org/files/issues/Edit___Drupal.png
https://www.drupal.org/files/issues/Edit___Drupal%203.png
https://www.drupal.org/files/issues/Schermafbeelding%202017-12-05%20om%2...

Could you let me know what the intended behavior is? Maybe I can provide a fix.

smccabe’s picture

smccabe commented
Status: Fixed » Needs work

Can confirm what @harings_rob sees, I also get the same issue and the required = TRUE is wrong. Seems like a larger widget rework would be needed before this would work? It errors the form if you are not adding an adjustment at all, but the error is hidden.

For now I think a revert of just that small piece is best?

harings_rob’s picture

harings_rob commented
Status: Needs work » Needs review

Hi @smccabe,

I forgot to include the PR for this:

https://github.com/drupalcommerce/commerce/pull/841

sorabh.v6’s picture

sorabh.v6
He/His
Primary language Hindi
commented
StatusFileSize
new 2926563-12.patch6.95 KB

I confirm the code in PR in #11 is working for me. Posting the patch from #11.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Status: Needs review » Reviewed & tested by the community
StatusFileSize
new 2926563-2-13.patch2.74 KB 
+++ b/modules/order/tests/src/Functional/OrderAdminTest.php
@@ -117,6 +117,63 @@ public function testCreateOrder() {
+  /**
+   * Tests a basic version of the order creation as it should work as well.
+   */
+  public function testCreateOrderWorksWithoutAdjustment() {
...
+  /**
+   * Tests a adjustment version of the order creation as it should work as well.
+   */
+  public function testOrderWithInvalidAdjustmentShowsError() {

I think we should just have one test called testCreateOrderAdjustmentValidation() - actually looking at the test further I don't think we need a new method at all.

harings_rob’s picture

harings_rob commented

Hi @alexpott,

However we can indeed test them from a single method, I wanted to be more verbose on the test case.

If the current test fails, it might not be directly visible what the issue is except that it is when creating an order.

If testCreateOrderWorksWithoutAdjustment fails, it is visible that there will be an issue when there is no adjustment, same for testOrderWithInvalidAdjustmentShowsError.

Ideally tests should be compact, and focussed on a single functionality.

This is, I guess, a personal opinion. But I just wanted to clarify why I did it this way.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented

@harings_rob - thinking about this some more testCreateOrderWorksWithoutAdjustment is definitely worth it. As that is the bug you're fixing. My bad.

But actually that test does not successfully create an order.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Status: Reviewed & tested by the community » Needs work

Needs work #15. I'll improve the test coverage.

harings_rob’s picture

harings_rob commented

It indeed does not, could be an improvement to the test.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Status: Needs work » Needs review
StatusFileSize
new 2926563-2-17.test-only.patch1.72 KB
new 2926563-2-17.patch3.44 KB

@harings_rob

Ideally tests should be compact, and focussed on a single functionality.

I totally agree for unit tests this should be the aim. With functional tests there is always a balance between cost of maintenance, running each test, and verbosity and these things.

The last submitted patch, 18: 2926563-2-17.test-only.patch, failed testing. View results

mortona2k’s picture

mortona2k commented

I didn't see the php error, but when I tried to save the order form with an empty adjustment, I got this js console error:
An invalid form control with name='adjustments[1][definition][label]' is not focusable.

Applying #18 fixes the error.

smccabe’s picture

smccabe commented
Status: Needs review » Reviewed & tested by the community

Tested patch as well, also works for me.

  • bojanz committed cb3d04b on 8.x-2.x authored by harings_rob 
    Issue #2926563 by alexpott, sorabh.v6, harings_rob, bojanz: Adjustment...
bojanz’s picture

bojanz commented
Status: Reviewed & tested by the community » Fixed

Thanks!

     foreach ($plugin_manager->getDefinitions() as $id => $definition) {
-      if ($definition['has_ui'] == TRUE) {
+      if ((bool) $definition['has_ui'] === TRUE) {
         $types[$id] = $definition['label'];
       }

Replaced this with a !empty, it's cleaner.

   public function massageFormValues(array $values, array $form, FormStateInterface $form_state) {
     foreach ($values as $key => $value) {
-      if ($value['type'] == '_none') {
+      if ($value['type'] === '_none') {

Reverted this change. While an identical comparison is theoretically better, we don't do this for any other string comparison in Commerce, so consistency wins out.

Status: Fixed » Closed (fixed)

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