Add inline entity form support for managing Shipments

Tagging for Drupal Global Contribution Weekend

@selinav: Could you provide more information (A list of steps that'd help us reproducing the issue)? I just tried editing existing shipments and saving and didn't get any error.

Renaming massageFormValues() will just mean that the parent function is being called instead which explains why the error is gone, but by doing that the shipping profile is probably not going to be saved properly.

I am facing the same issue. Updated the issue title and steps to reproduce.

As far as I can see this could be a regression from #3023313: Replace commerce_profile_select usage with the customer_profile inline form due to the following change:

     $element['array_parents'] = [
       '#type' => 'value',
-      '#value' => [$items->getName(), 'widget', $delta],
+      '#value' => array_merge($element['#field_parents'], [$items->getName(), 'widget', $delta]),
     ];

A more obvious error appears if you follow the described steps in the issue summary and replace step 5 with: 5. Click "Edit" on the Shipment item and try to save the order.

Edit: Similar error appears if you press "Add new shippment":

An AJAX HTTP error occurred.
HTTP Result Code: 200
Debugging information follows.
Path: /admin/commerce/orders/2/edit?ajax_form=1
StatusText: OK
ResponseText: Error: Call to a member function getStore() on null in Drupal\commerce_shipping\ShippingMethodStorage->loadMultipleForShipment() (line 21 of /commerce/web/modules/contrib/commerce_shipping/src/ShippingMethodStorage.php). 

Here is a test-only patch that should fail with an exception.

I get the error message referenced in the first post on switching the shipping profile during the checkout when the selected shipping method is not available for the shipping profile I switched to.
I can get ride of the error by replacing the line 136 of commerce_shipping/src/Plugin/Field/FieldWidget/ShippingRateWidget.php :

-  if ($selected_value) {
+  if (isset($element[$selected_value])) {

However, I still get a validation error on changing the shipping profile.
'hope it helps

In fact, in my case the main error is an illegal choice detected.
I'm not sure how the selected shipping method should be reset if the value is no longer available after switching from a profile to another.

Debugging this.

Reading #6:

    $query = $this->getQuery();
    $query
      ->condition('stores', $shipment->getOrder()->getStore()->id())

Looks like we have a logic whole when trying to load shipping methods for a new shipment. The IEF doesn't preset the shipment's order.

Here's a quick test to check #13

Okay here's the problem.

$value['array_parents'] has: shipments][form][inline_entity_form][entities][0][form][shipping_profile][widget][0

BUT it's actually shipments][form][inline_entity_form][entities][0][form][shipping_profile][0][widget][0

#5 caught the edit. IEF sends in the form without needin field_parents to the widget. I still want to investigate #13 for creating a shipment within IEF.

IEF is a soft dependency because of the Order module and could go away if we make a custom order items widget. This code should go in a specific module that tests IEF integration and marks @requires module inline_entity_form

So we can fix the array parents for IEF edit. But I think we should fix all IEF errors. Using IEF to add a shipment dies because the order isn't set in the profile widget

  public function formElement(FieldItemListInterface $items, $delta, array $element, array &$form, FormStateInterface $form_state) {
    /** @var \Drupal\commerce_shipping\Entity\ShipmentInterface $shipment */
    $shipment = $items[$delta]->getEntity();
    $order = $shipment->getOrder();
    $store = $order->getStore();

🧐IEF doesn't call buildEntity before building the form, which I think normal content entity forms do.

here's a WIP patch until I have more time.This fixes the array path and starts supporting "Add" in IEF, which probably fixes other bugs along the way. This is basically exposing a lot of specific logic in ShipmentForm.

This patch fixes the Add and Edit forms with IEF. There are probably other things needed, but we can do those in follow-ups.

Let's see after #3116985: CheckoutPaneTest are failing because credit card in tests are using past dates for expiration

It isn't pretty, but I think it's the best we can do beyond trying to fetch from route parameters.

+    if ($shipment->isNew()) {
+      $form_object = $form_state->getFormObject();
+      assert($form_object instanceof OrderForm);
+      $order = $form_object->getEntity();
+      assert($order instanceof OrderInterface);
+      $shipment->set('order_id', $order);

@mglaman: I tested the patch manually myself and found an interesting bug...

Clicking on "Update shipment" no longer "breaks", thanks to the array_parents change but... the shipping profile only gets updated when you submit the main form (But... and that's where it gets interesting, only when the IEF form is opened).

Otherwise, if you switch to a different profile, then click on the IEF submit button, go back to editing the shipment or just save the main form the shipping profile is not updated... Most probably something we can test...

Tests should be failing, but the following assertion should in theory pass:

from line 615:

$this->assertEquals($this->defaultAddress, array_filter($shipment->getShippingProfile()->get('address')->first()->toArray()));

Note that as discussed with @mglaman, assertRenderedAddress is not designed to work when multiple addresses are shown on the same page. ProfileFieldCopyTest overrides it, but copying the same method doesn't work in our context.

For me, during manual testing, I noticed that updating the profile, and then saving using the main "Save" button was working, but the following doesn't:
1) Edit an order to update an existing shipment
2) Update the shipping profile
3) Click on "Update shipment" (First of all, coming back to the shipment edit shows the previously selected profile, instead of the right one) and then saving the main form doesn't correctly update the profile.

Testing the actual data doesn't seem to work as expected, but testing what's shown in the UI actually reflects what I noticed during manual testing...

So now I have a failing test that replicates what I experienced during manual testing...

I added assertRenderedAddressTextOnly() (which has a terrible name and just for the purpose of testing, but we need to fix the actual parent assertRenderedAddress...).

I'm not expecting this to be committed, I did it just for the purpose of demonstrating the failure...

I've been deep into debugging this issue. It looks like the problem now isn't IEF but the InlineForm plugins when modifying a referenced entity inside of IEF. It's completely OK when the IEF form is open, as the submit process is completely different. However if you submit the IEF and then submit the parent form things fall apart.

Okay, here is a patch. The IEF widget works fine unless you click "Update shipment", because the CustomerProfile inline form is never submitted. IEF widget works just fine if you never "close" it by saving.

So this patch disables the ief_edit_save button. IEF + Shipping isn't on our strategic roadmap right now. This fixes the issue without diving into a giant rabbit hole, while lettng folks continue to use IEF without much disruption.

Whoops, list got converted thanks to PhpStorm.

This fixes linting and also removes data checking to avoid cache invalidations. It solely uses checking the rendered address on the order view page after saving the order.

@mglaman: I'm actually having second thoughts on this one... While the patch provides basic "IEF" support, there are several things that are missing in order for this to be usable...

Technically, the IEF form for shipment doesn't provide the button to recalculate rates, and no checkboxes for selecting the shipment items...
Basically, we're missing all the logic that's present in the regular ShipmentForm.

Should we mark this as "won't fix"? Or postponed, or are we fine with providing really basic support?

Those are all valid problems that need to be fixed regardless of IEF or not.

I think we open immediate follow up about hacks in ShipmentForm that need to be addressed and commit this. If people ask "Why doesn't IEF have XYZ" then we can point to that issue, versus more "IEF throws an error"

Ok, let's proceed with that then, feel free to commit it.

Okay, I'll commit after I open the general follow ups. Working on this issue showed me ShipmentForm has a lot of clean up to be done.

Committed! This makes the form somewhat usable. See #3117476: ShipmentForm hardcodes logic for package types, shipment items, and rate calculation for feature parity improvements. The community is more than welcome to help start that.