Put back adjustments to Shipments

Thanks for your work on this issue ; however it is missing the following parameters in the getAdjustments() method to be compliant with a related interface and avoid a PHP fatal error : array $adjustment_types = [].
Sorry, I don't know how to patch yet.

I am updating the patch against the latest beta7.
Changes
* Fix getAdjustments() to match interface.
* Add adjustments to other view display install configs

We have decided to proceed with this issue.

I discussed with jsacksick the possibility of adding adjustments directly to the order, and not adding the adjustments field to shipments at all.
However, that would mean that we're unable to get the adjusted price and totals per shipment.

The next patch iteration needs to rework the order processor code. With the current patch we have ShipmentClearAdjustmentsOrderProcessor and ShipmentOrderProcessor, which is confusing. Let's rename them to EarlyOrderProcessor and LateOrderProcessor, and add docblocks explaining what each is doing. Let's also move the packing to the early order processor.

Summary:
- EarlyOrderProcessor: Pack the shipments (without saving them) and clear the adjustments
- Other order processors (add taxes promotions and fees)
- LateOrderProcessor: Save the shipments and transfer shipment amounts / adjustments to the order.

Here is an updated patch.

It has one API change, ShippingOrderManager::pack() no longer saves the shipments. From what I've seen there is no use case for saving immediately (including our internal API usage).

Note that this approach assumes that promotions/taxes/fees will not modify the shipment amount, which I think makes sense.
If we were to allow modifying the shipment amount, we'd need a rate_amount field that stores the original rate pre-modification, and which is set as the main amount at the beginning of each order refresh. I originally implemented this, but then realized we don't need it.

/**
   * Gets the rate amount.
   *
   * Represents the cost of shipping the shipment using
   * the selected shipping method and service.
   *
   * @return \Drupal\commerce_price\Price|null
   *   The rate amount, or NULL if unknown.
   */
  public function getRateAmount();

  /**
   * Sets the rate amount.
   *
   * @param \Drupal\commerce_price\Price $rate_amount
   *   The rate amount.
   *
   * @return $this
   */
  public function setRateAmount(Price $rate_amount);

  /**
   * Gets the shipment amount.
   *
   * Calculated from the rate amount by applying
   * promotions, taxes and fees during the
   * order refresh process.
   *
   * @return \Drupal\commerce_price\Price|null
   *   The shipment amount, or NULL if unknown.
   */
  public function getAmount();

  /**
   * Sets the shipment amount.
   *
   * @param \Drupal\commerce_price\Price $amount
   *   The shipment amount.
   *
   * @return $this
   */
  public function setAmount(Price $amount);

    foreach ($shipments as $shipment) {
      $rate_amount = $shipment->getRateAmount();
      if ($rate_amount) {
        $shipment->setAmount($rate_amount);
      }
      $shipment->clearAdjustments();
    }

Assuming the rate amount is $10, and you want to charge your customers a "handling fee" of $3, what would $shipment->getAmount() return? I guess $10, with a non "included" adjustment right? Wouldn't it make more sense for $shipment->getAmount() to return $13, and have the rate amount stored in a "rate_amount" field that returns $10? Or couldn't we simply store the rate amount as an "included" adjustment? To be able to store a record of the rate amount?

We don't store the rate_amount because we don't support "included" discounts / fees, because we have no way to show them on the shipping rate radios. Showing discounted amounts on the radios will require a lot more work.

The ShippingInformation checkout pane still relies on the packer manager (instead of the shipping order manager), I guess the only remaining obstacle for the conversion is the fact that the shipments are removed right away? (which could have a significant performance impact assuming the repacking happens on each ajax refresh...), and shipments are removed each time this happens (I'm not 100% sure under which circumstances the shipments are being removed)

Definitely not planning to convert the checkout pane. I think its current "commit on submit" logic makes sense.

Here's the last patch for this issue.

It changes the weight of the shipping adjustment type, so that it appears before the other adjustment types (otherwise shipping costs would appear after their discounts).

Jonathan and I agreed that further changes are needed to support display inclusive promotions, but we'll chase that in a different issue, since this one (and the existing shipping promotion one) are already huge.

Committed.

This commit made it into beta8

Upgrading from beta7 gives me errors because this table is missing. Can someone else test to confirm (it's possible I had a weird combinations of upgrades) Maybe there should be a schema update to ensure the table is there?

@Fool2
There was no reason to reopen this issue, there's already #3110569: missing table 'xxxx.commerce_shipment__adjustments for the issue you're reporting (and where we're missing information for diagnosing it). This issue added the update function commerce_shipping_update_8202() which creates the adjustment field and its table when update.php is run.

Hi, thanks for pushing commerce_shipping module forward, much appreciated! Sorry for noticing this that late but some code I wrote is now failing because ShipmentOrderProcessor was renamed to EarlyOrderProcessor #3110783: Remove ShipmentOrderProcessor usage. I just wonder if this is considered a BC break (service removed, non-@internal-class name changed) or tagged services are exempted from BC policy?

@zaporylie
Shipping is in beta, there are no BC guarantees yet.
The RC1 release will break all shipping methods, for example (and unfortunately).
Only after RC1 can we expect calmness.

Furthermore, I would definitely expect every order processor to be treated as @internal, even though we're not using that tag yet.