Create an admin UI for shipments

It's far away from being done, but sending my work in progress (See https://cl.ly/3y2u1L271Z06), the patch contains the initial ShipmentListBuilder with the delete operation.

Just wondering: Is there, coincidentally, any progress in the mean time on this (e.g. a new patch)? If not, I'd need to quickly patch in some temporary solution for a current project.

Took another pass at this. Changes in the patch

- Setup a canonical url for shipments at '/admin/commerce/orders/{commerce_order}/shipments/{commerce_shipment}'
- Created a view on the collection path '/admin/commerce/orders/{commerce_order}/shipments' that overrides the listbuilder , this mimics what is currently done for orders at '/admin/commerce/orders'
- Changed the default shipment view mode to display a lot more fields
- Created a field formatter that formats 'commerce_shipment_item' fields as a table

Removing the image I put in the issue summary by accident.

Added:

• Shipment Add/Edit Form
  • new/edit shipping profile for the shipment
  • Allows you to select OrderItems in the order and add them as ShipmentItems. Doesn't show order items that are already tied to other shipments
  • Recalculate button that repopulates the shipping methods available.
• ShipmentController that handles routing based on whether multiple ShipmentTypes exist
• Modified ShipmentListBuilder so that you can click on the title to view the shipment.

I only tested using Commerce_FedEx and FlatRates. Commerce_FedEx was set to package items individually, right now it errors out when its set to anything else. I have a fix for it but right now FedEx's test site is down so its hard to work on. I'll post an updated patch when I can.

A couple more things:

1. Need to change $form['items'] to required in ShipmentForm class. Without this, you get an error trying to create a new shipment if no shipment items were added to it. I will fix this in next patch.
2. When saving a new shipment, should shipping adjustment be created and added to the order like it does when shipments are created through the cart?
3. When deleting an order item, should we loop through shipments to see if it is being referenced and remove the item from the shipment? If the shipment only references that one item, should we delete the shipment?
4. I think once https://www.drupal.org/node/2844920 is figured out it'd be nice to be able to select existing shipping profiles (if available) when adding a shipment.
5. If an order item has multiple quantities, should you be able to split it between shipments?

Added the ability to select the PackageType for the shipment. Made sure that any changes to the shipping profile address were being used when recalculating shipping.

Known Issues:
- In FedEx shipping method, you can configure whether shipments should be packaged all in one, individually, or calculated. This makes sense for the Checkout pane, because you want to display all of the shipments as one rate to the customer. However, that configuration setting also changes rate calculations when creating a shipment through the admin UI. If you have 3 shipment items in the shipment through the Admin UI, and FedEx is set to package items individually, the rate request is sent as if that shipment is 3 boxes. It's my opinion that when creating a shipment manually, shipment = one package, and you are specifying what items are in that package, so it should always calculate as "all in one box" when being created manually.

Just noticed that you get "Illegal choice has been detected" message if no rates are available when calculating shipping. Patch sets shipping methods #access to false if no options are available.

I think we should create a new field widget plugin for the "package_type" base field instead of define a form element in the \Drupal\commerce_shipping\Form\ShipmentForm::form.
Similar to the \Drupal\content_moderation\Plugin\Field\FieldWidget\ModerationStateWidget.

It is not sure there is a package_type saved to the shipment.
Only modification in this patch compared to the #11 is that checks that the package_type is empty or not.

Moving priority to Major. Being able to create a shipment from the admin interface is a key feature of Commerce.

Confirmed that patch #12 works. I did not complete thorough testing or code review on it.

Attached is a small modification to #12 by adding a "Shipments" entity operation similar to what Payments does. See interdiff.

Thanks everyone for you work on this. Patch #13 seems to be working as expected for me.

Reroll on latest 8.x-2.x.

Attempt to split out permissions on viewing shipment entities. This is completely untested! I copy / pasted from what I saw in orders.

Next step: test this and see if you can correctly control permission to CRUD operations on the Shipment entity, similar to how you can with Orders.

I'm adding a related issue because it will most certainly impact the Admin UI. I advise anyone following this issue to take a look at https://www.drupal.org/project/commerce_shipping/issues/2953483 and check out the repo and video I posted. I have made significant changes to the Admin UI there.

Everyone can ignore this patch unless you're using beta 4. Since we are using beta 4, I needed to get a patch for it. It will match my follow up comment for latest 8.x-2.x.

Attached is a minor change to #16 to ensure the permission is grouped correctly. Consider this the most recent patch on this ticket.

Attached are also 2 screenshots showing the difference in permissions between 13 and 19. Here, you can see that there are now granular permissions on the Shipments. There is also a "View own shipments" permission, which could probably be removed? I'm 80% confident this patch isn't totally right, so if I need to revise I'll follow up.

To clarify: the problem I'm trying to solve with the permissions is to allow someone with Administer Shipments to be able to add a shipment to an order without also needing to have Administer Shipment Types permission.

Probably https://www.drupal.org/project/commerce_shipping/issues/2981945 is a duplicate of this issue.

I've re-rolled the patch on top of 8.x-2.x - just a small conflict in commerce_shipping.module's use statements.

Coding standards clean-up.

Fixing the tests

I like where this is headed. The two pieces that I noticed are :

1) Shipping adjustment is not automatically added to order. Should there be a flag on the shipment to force this?

2) The entity reference on the order is not populated so the order is unaware of the shipment (e.g. doesn't show shipping address)

3) Variations that do no extend the shippable trait are included in the shipment UI

Updates to #26 to fix issues found in #27 attached

Ok, here's a patch with all of the improvements mentioned in #27 + test coverage. I think this is pretty close to ready, but would like some extra eyes on it.

The remaining non-blocking (imo) things that should be addressed once this is settled are as follows.

1) Lock adjustments on order edit form when they are tied to a shipment
2) Provide ability to override shipment amount

I've been doing some manual testing today and expanded the patch a little:

First of all, IMO the shipments tab should only appear if the order type has the "shipping enabled".

Also, on my local installation, I didn't have any package types configured, and clicking on "Recalculate shipping" would eventually return a 500 (Because we're trying to instantiate a package type with an empty string).

I fixed these 2 things and I'm now wondering if we shouldn't add tests coverage for the shipments admin (although this would require more time...)

Btw, shouldn't commerce_shipping_entity_operation() check the "administer commerce_shipment" permission instead of "adminster commerce_shipment_type"?

@jsacksick we definitely need to add test coverage here. Adding features without test coverage makes them liable to break.

I started writing a functional test for the ShipmentAdmin but it still needs to be expanded, posting my work in progress.

I expanded the ShipmentAdminTest and found 2 bugs while doing that:

1) There was a bug with the views caching (In the tests, we navigate to the shipments tab, add a shipment then save, then we get redirected to the shipments page again and the page was empty after adding a new shipment), so I configured the caching to "none" (I don't think there's a point in caching that view anyway)....
2) The logic in ShipmentForm wasn't actually adding existing shipment items to the $shipment_item_options array, that was done from the $order_items loop, the problem with that is that the order item label was used instead of the actual shipment item title that we get by calling $shipment_item->getTitle(), that could probably be confusing.

Besides that, I think we can commit the patch as is, and probably work on the remaining things that @andyg5000 mentioned in comment #33 after this is committed (in follow-up issues, because the patch is already big enough).

Ok, actually, I realized we forgot "2) Perform state transitions on the created shipments.", the attached patch is taking care of that (we now output the transition form instead of the state label, see screenshot below):

Ok, sorry for the noise :p, but realized we don't need all these changes in the routing.yml and we were directly calling in several places the entity class for creating/loading entities (i.e Profile::create or OrderItem::load).

I made some changes to the ShipmentRouteProvider, as well as the ShipmentController which now extends the EntityController, I'm implementing a different method for the "add-page" though since we have to pass the order ID to the shipment add form.

Additionally ShipmentForm was still passing the entity manager, which we were still using in many places although it's deprecated...

I hope we're good to go this time!

+++ b/config/install/views.view.order_shipments.yml
@@ -2,9 +2,9 @@ langcode: en
-    - commerce_price
-    - commerce_shipping
-    - options
+  - commerce_price
+  - commerce_shipping
+  - state_machine

@@ -412,11 +412,11 @@ display:
-        - 'languages:language_content'
-        - 'languages:language_interface'
-        - url
+      - 'languages:language_content'
+      - 'languages:language_interface'
+      - url

@@ -427,9 +427,9 @@ display:
-        - 'languages:language_content'
-        - 'languages:language_interface'
-        - url
+      - 'languages:language_content'
+      - 'languages:language_interface'
+      - url

These change does not look right - has the view been hand edited?

Minor changes to the view:

• Add an empty text to the view ("There are no shipments yet").
• Sort by shipments created DESC.
• Make the title & amount columns sortable.

Attaching the right interdiff between #40 & 42.

1. Some thoughts... not a complete review

2. +++ b/commerce_shipping.module
   @@ -200,3 +202,23 @@ function commerce_shipping_preprocess_commerce_order_receipt(&$variables) {
   +  if ($entity->getEntityTypeId() !== 'commerce_order' ||
   +    ($entity->hasField('cart') && $entity->get('cart')->value) ||
   +    !\Drupal::currentUser()->hasPermission('administer commerce_shipment')) {
   +    return;
   +  }
   

   I think a comment explaining this if would be great. Also do we have test coverage?

3. +++ b/src/Access/ShipmentCollectionAccessCheck.php
   @@ -0,0 +1,69 @@
   +    if (!$order) {
   +      return AccessResult::forbidden();
   +    }
   
   +++ b/src/Entity/Shipment.php
   @@ -43,6 +55,14 @@ use Drupal\profile\Entity\ProfileInterface;
   + *     "collection" = "/admin/commerce/orders/{commerce_order}/shipments",
   
   +++ b/src/ShipmentRouteProvider.php
   @@ -0,0 +1,64 @@
   +      $route->setOption('parameters', [
   +        'commerce_order' => [
   +          'type' => 'entity:commerce_order',
   +        ],
   +      ]);
   +      $route->setRequirement('_shipment_collection_access', 'TRUE');
   

   I don't think the if (!$order)... is necessary - with no order this route will not check access. It'll 404.

4. +++ b/src/Access/ShipmentCollectionAccessCheck.php
   @@ -0,0 +1,69 @@
   +    $order_type_storage = $this->entityTypeManager->getStorage('commerce_order_type');
   +    /** @var \Drupal\commerce_order\Entity\OrderTypeInterface $order_type */
   +    $order_type = $order_type_storage->load($order->bundle());
   +    $shipment_type_id = $order_type->getThirdPartySetting('commerce_shipping', 'shipment_type');
   +    if (empty($shipment_type_id)) {
   +      // Order types with no shipment type selected don't need the Shipments
   +      // tab.
   +      return AccessResult::forbidden()->addCacheableDependency($order_type);
   +    }
   +
   +    $permissions = [
   +      'administer commerce_shipment',
   +    ];
   +    return AccessResult::allowedIfHasPermissions($account, $permissions);
   

   I think this won't result in the right cacheability and is not as clear as it could be. Something like:

       /** @var \Drupal\commerce_order\Entity\OrderInterface $order */
       $order = $route_match->getParameter('commerce_order');
       $order_type_storage = $this->entityTypeManager->getStorage('commerce_order_type');
       /** @var \Drupal\commerce_order\Entity\OrderTypeInterface $order_type */
       $order_type = $order_type_storage->load($order->bundle());
       $shipment_type_id = $order_type->getThirdPartySetting('commerce_shipping', 'shipment_type');
   
       // Only allow access if order type has a corresponding shipment type.
       // @todo should we validate that the shipment type exists.
       return AccessResult::allowedIf($shipment_type_id !== NULL)
         ->addCacheableDependency($order_type)
         ->andIf(AccessResult::allowedIfHasPermission($account, 'administer commerce_shipment'));
   

   Also is there test coverage for the case where the order type does not have a shipment type?

5. +++ b/src/Controller/ShipmentController.php
   @@ -0,0 +1,115 @@
   +    // If only one shipment type is available, bypass the bundle selection.
   +    if (count($content) == 1) {
   +      $type = array_shift($content);
   +      return $this->redirect('entity.commerce_shipment.add_form', [
   +        'commerce_order' => $order_id,
   +        'commerce_shipment_type' => $type['type']
   +      ]);
   +    }
   

   Is there test coverage of this logic? Either where there is one of more shipment types?

6. +++ b/src/Entity/Shipment.php
   @@ -357,6 +386,69 @@ class Shipment extends ContentEntityBase implements ShipmentInterface {
   +      $adjustment_exits = FALSE;
   ...
   +            $adjustment_exits = TRUE;
   ...
   +      if (empty($adjustment_exits)) {
   

   exists - also given we know this is boolean then if (!$adjustment_exists) is fine - the empty() makes you ponder why.

7. +++ b/src/Form/ShipmentForm.php
   @@ -0,0 +1,310 @@
   +        in_array($order_item->id(), $already_on_shipment)) {
   

   in_array() - let's make it type safe using the third param - set it to TRUE.

8. +++ b/src/Form/ShipmentForm.php
   @@ -0,0 +1,310 @@
   +    // Add the shipment to the order if it doesn't exits.
   

   exist

9. +++ b/src/OrderShipmentSummary.php
   @@ -40,6 +40,10 @@ class OrderShipmentSummary implements OrderShipmentSummaryInterface {
        $shipments = $order->get('shipments')->referencedEntities();
        $first_shipment = reset($shipments);
   +    // Make sure the shipment still exists.
   +    if (empty($first_shipment)) {
   +      return [];
   +    }
        $shipping_profile = $first_shipment->getShippingProfile();
   

   Won't it be clearer to check if shipments is empty? Is the reason for this code tested?

@alexpott: Thanks a lot for this review! It turns out some more changes were actually required to make it right.

I had some discussions with @bojanz today and made the following changes:

• We're no longer manually adding shipping adjustments, but we now trigger an order refresh so that the adjustment is added by the processor during the refresh process (Manually adding adjustments isn't a good idea since they'd be blown away by OrderRefresh).
• I moved the delete() logic to postDelete() and also trigger an order refresh there so that the shipping adjustments are removed automatically.

I'm just posting my progress here, but the patch is actually still incomplete... There's an issue with the Access checker which doesn't seem to be called for some reason (I remember it working which is kind of driving me crazy...).

Also, we're still missing test coverage to make sure the "Shipments" tab / operation isn't shown for cart orders...

I've continued working on this today and made once again several changes:

1. I realized the assertions that were introduced in #33 to the ShipmentTest were never evaluated because the order doesn't have adjustments.
2. I removed the postSave() logic and moved that to the save() method in ShipmentForm (because in other places, we usually save the order after adding the shipments to the order (or that's done by the refresh process automatically), with the postSave() logic the order would be saved everytime a shipment is saved, so in a loop that'd potentially mean saving the same order multiple times in a row.
3. I finally found out why the route requirement I'm adding (_shipment_collection_access) didn't seem to be in place, it was caused by Views which is altering the route and not merging the route settings in PathPluginBase::alterRoutes(), so I had to implement a route subscriber to put back the route requirement that makes sure the shipments tab isn't shown for cart orders & non shippable orders.
4. I expanded the test coverage, I added tests for making sure that the shipments tab isn't shown for cart orders / non shippable orders.
5. I updated the logic in ShipmentController::addShipmentPage() because I realized that there's no point in showing a "shipment types" list since there's an order type setting that lets you configure the shipment type per order type, so the logic now redirects to the add shipment page for the configured shipment type.

I'm wondering if we need to prevent updating shipments that are "owned_by_packer" (that flag is added by the packer), because a modification to a shipment that's owned by the packer could be blown away (We might have to just forbid updating the "shipment items" field in that case).
The problem with doing that is that could make this entire UI useless (Although you might want to perform updates to shipments that are owned by the packer once the order is placed).

At least we're forbidding access to the shipments tab for cart orders, which is probably sufficient.

I've moved the unrelated code style changes to #3013669: Update phpcs to 3.x so people don't have to ponder why they are here. Also fixed the typehints in #47 which had some duplication.

1. +++ b/commerce_shipping.module
   @@ -200,3 +202,26 @@ function commerce_shipping_preprocess_commerce_order_receipt(&$variables) {
   +  // Show the "Shipments" operation link for orders, if the current user
   +  // has the "administer commerce_shipment" permission and if the order is not
   +  // a "cart" order.
   +  if ($entity->getEntityTypeId() !== 'commerce_order' ||
   +    ($entity->hasField('cart') && $entity->get('cart')->value) ||
   +    !\Drupal::currentUser()->hasPermission('administer commerce_shipment')) {
   +    return;
   +  }
   

   Given this is access related I think it is better to split this out into separate ifs. It's much clearer as to what is going on and helps lessen the cognitive load of working out what's happening.

2. +++ b/src/Access/ShipmentCollectionAccessCheck.php
   @@ -0,0 +1,66 @@
   +    /** @var \Drupal\commerce_order\Entity\OrderInterface $order */
   +    $order = $route_match->getParameter('commerce_order');
   +    $order_type_storage = $this->entityTypeManager->getStorage('commerce_order_type');
   +    /** @var \Drupal\commerce_order\Entity\OrderTypeInterface $order_type */
   +    $order_type = $order_type_storage->load($order->bundle());
   +    $shipment_type_id = $order_type->getThirdPartySetting('commerce_shipping', 'shipment_type');
   +    // Check if this is a cart order.
   +    $order_is_cart = $order->hasField('cart') && $order->get('cart')->value;
   +
   +    // Only allow access if order type has a corresponding shipment type.
   +    // @todo should we validate that the shipment type exists?
   +    return AccessResult::allowedIf($shipment_type_id !== NULL)
   +      ->andIf(AccessResult::allowedIfHasPermission($account, 'administer commerce_shipment'))
   +      ->andIf(AccessResult::allowedIf(!$order_is_cart))
   +      ->addCacheableDependency($order_type)
   +      ->addCacheableDependency($order);
   

   This is very readable. Nice. It really helps when access related stuff is easy to grok.

3. +++ b/src/EventSubscriber/RouteSubscriber.php
   @@ -0,0 +1,40 @@
   +    // @todo: Remove once Views will merge the route requirements.
   

   Is there a core issue for this?

4. We can have less code in the controller.
5. I think that ShipmentListBuilder is completely untested. It's the non-views fallback for the shipments listing page.

Actually point #49.5 appears to be wrong. We've no test with views installed as far as I can see.

Lol actually #49.5 was correct. The Views module is required by the Commerce module. Here's a test to prove the listing works with and without the view.

Hmmm some unrelated change crept in :(

Small coding standards fix.

@smccabe review this and suggest the UI needed a little polish but that it could be done in a follow-up - so I've created #3015345: Improve shipment admin UI look and feel

1. Fixed. We already have shipment item in the UI. See \Drupal\commerce_shipping\Plugin\Commerce\ShippingMethod\FlatRatePerItem::buildConfigurationForm() I guess we should add a follow-up to discuss how to rename that if you want to do that.
2. Fixed
3. @todo - so here should we move all the order saving logic into Shipment::save(). I'm a little dubious of this because saving other entities during another entity save is often very very problematic.
4. Fixed
5. Fixed
6. Let's get rid of ShipmentPermissionProvider entirely. The permissions are not used at all. Less code FTW and if we need a viewer permission we can add that later.
7. I couldn't make this happen. I created a cart order and then went to admin/commerce/orders/carts and shipments is not in the list. There is code in commerce_shipping_entity_operation() to prevent this. I've added test coverage because I couldn't see any.
8. Fixed
9. @todo - I've asked @bojanz which way to go here. Manually editing tracking codes feels funny. Maybe that's why it is not on the edit form but showing them to the operator feels correct.

I agree with @alexpott on 3) I didn't want to put the order refresh logic in a postSave() as I think this could potentially be very problematic. Let's say you have custom code that adds multiple shipments to the order, you'll most probably save the order yourself and you wouldn't want the order to be saved each time a shipment is saved (from within a loop for instance).

ShipmentOrderProcessor itself loops on the shipments that are returned by the packer and save them in case changes were detected (so multiple shipments are returned, the same order would be saved many times).
Furthermore, the order processors are called from OrderRefresh::refresh() which is itself called from doOrderPresave() on the order storage so I'm sure we'd like to prevent the order to be saved inside a presave.

Discussed the tracking code on the list builder with @bojanz in Slack. His recommendation was to remove it. I then convinced him to keep it and make it displayed by setting display options in \Drupal\commerce_shipping\Entity\Shipment().

This turns out to be complex. Shipping methods only support tracking codes if they implement \Drupal\commerce_shipping\Plugin\Commerce\ShippingMethod\SupportsTrackingInterface. However as @bojanz pointed out in slack, there's nothing to stop a user

sell(ing) something with flat rate shipping, get a tracking code at the post office, enter it in the UI

Therefore the patch attached sets the dispaly options for the tracking code so users can edit it. Given that this is all standard field API stuff I've not added any test coverage.

We need a follow-up to make the commerce_tracking_link formatter support cases where the shipping method does not have SupportsTrackingInterface and then we can make the listbuilder and view use it for enhanced usability. I'll create the issue.

Created #3016633: Use TrackingLinkFormatter in Shipments admin UI and allow it to work when SupportsTrackingInterface is not implemented

I've run into some issues with this fix when used in conjunction with Commerce UPS and the checkout flow is setup for the customer to pick a shipping option during checkout.

1) When editing the shipment after checkout it will allow you change boxes and remove items, and then recalculate the shipping cost. However, saving the shipment and then going back into edit again none of the changes are reflected (The new shipping total is correct though). Making the same changes a second time and saving it again seems to resolve this.

2) If a customer gets to the review order page (i.e. post-shipping selection) and then edits the cart to add/remove/update items and/or quantities resulting in a new shipment total the next time they go through checkout then when viewing the order's Shipments tab it lists all of the shipments that had been on the order at some point. The last one selected has Send & Cancel options under State, and the others will have Finalize & Cancel options.

3) There is no way to split an item with more than 1 quantity across shipments.

4) If you update a shipment's cost it will be reflected in the order's totals. This does not seem to be a desired behavior after the customer has already paid.

I don't know if any of this might be an issue for the UPS Module to address, but it all felt like core shipping issues.

@MElpers thanks for the comment.

1. I've tried to make this occur with UPS enabled and configured but I can't make it happen. Can you detail the exact steps - thanks!
2. I've tried to reproduce this but I can't make it occur either. Do you have exact steps?
3. I've opened #3025093: Allow splitting an item with a quantity > 1 across shipments to work on that functionality
4. I've opened #3025096: Changing a shipment after the customer has paid recalculates the total - it'd be great if you can describe what you think the behaviour should be on that issue.

@Alexpott thanks for the two new issues. I'll follow those and comment on #4 shortly.

For the first two, hopefully this will help. First, I ran all of these on fresh install with none of our customer's custom code. I just setup 2 products with a single variation each. The modules I'm testing with are:

• Core 8.6.4
• Commerce 2.11
• Commerce Payeezy: 1.0-beta1
• Commerce Connector for AvaTax 1.0-beta4
• Commerce UPS: 3.0-dev

I can provide the patch list too if you need it.

Issue #1:

1. Place an order for 1 item.
2. Go to the Shipment Tab and Edit the shipment. See Example1a.jpg.
3. Note the shipping amount & package type. See Example1b.jpg.
4. Change the package type and click Recalculate Shipping. (You may need a custom package setup). See Example1c.jpg. Save it.
5. Back on the Shipments tab, note that the Amount is now the new re-calculated value. See Example1d.jpg.
6. Edit the shipment again.
7. The form has the original shipping cost & box options. See Example1e.jpg.

I can also recreate this when there are multiple items in the cart and 1 or more of the item checkboxes are unchecked & then the shipping is recalculated.

Running through the same steps a second time to get the same new shipping totals & saving it again will then allow the new values to show up if the shipment is edited a third time.

Issue #2:

1. Add 1 of item # 1 to my cart.
2. View Cart > Checkout
3. Fill out shipping & payment if needed. Select UPS Ground. Continue to Review.
4. Confirm the Summary has the shipping & tax information.
5. Return to the site and add 1 of item #2 to the cart.
6. View Cart > Checkout
7. Increase the quantity of item #2 to 2 and click the Update Cart button.
8. Fill out shipping & payment if needed. Select UPS Ground. Continue to Review.
9. Pay and Complete purchase.
10. Open Commerce Orders page and then open the Shipments tab. See Example2a.jpg.

Let me know if you need any additional info.

@MElpers can you confirm what version of commerce_shipping you are on? Are you on a beta version with the patch applied or have you checkout the commit? Because the first problem sounds a lot like #2920242: Shipment always using last shipping method's package type

Composer tells me it is 2.x-dev 7f02042. I am applying patches for

Multiple destination shipping: https://www.drupal.org/files/issues/2018-10-11/2961463-29.patch
Auto-recalculate shipping when the address changes: https://www.drupal.org/files/issues/2018-10-11/2849756-83.patch
Commerce_profile_select reusable profiles: https://www.drupal.org/files/issues/2018-10-05/2948954-35.patch

Yay I've managed to recreate #1 there appears to be some random-ness but if you change packages and recalculate shipping enough times eventually it will go wrong - I'm investigating the bug.

I've opened #3028227: Package type not saved when changed in admin UI to address #66 issue 1.

I'm addressing #66.2 in #2994345: Remove shipping when cart is emptied - this issue has kind of been reported before.

@MElpers I might be wrong in #71. I'm struggling to reproduce the issue but via reading the code I've come up with #3033195: Need to repack when number of items changes