[Ubercart D6] Create line item migration

This looks like the adjustments field on the destination commerce_order_item entity.

And on the source, uc_order_line_items.

So, I created a new migration with a new source and destination plugin. Ran it and get 'shipping is an invalid adjustment type'. That's right there isn't a shipping adjustment type. Should adjustment types that don't match the existing ones be given type 'Custom' or something else?

Let's check if commerce_shipping is enabled. If it is, let's use `shipping` adjustment type (http://cgit.drupalcode.org/commerce_shipping/tree/commerce_shipping.comm...). Otherwise, yeah let's make it custom.

Having some success with this.

TODO: All the source orders have a shipping line item. It would be prudent to test an order that does not include shipping.

There needs to be a test with commerce_shipping enabled. But when I try that locally I get an exception 'The "physical_measurement" plugin does not exist.'. What needs to happen so that plugin is found?

'The "physical_measurement" plugin does not exist.'

Commerce Shipping requires Physical module

@mglaman, thx. I should've seen that.

Patch now with expanded tests and a new test for testing orders with the shipping modules enabled.

The patch helps.

This moves the creation of the Adjustment and Price to a new destination plugin, since that isn't really a transform operation. There are few things to address, such as the currency is hard coded and maybe the test for the presence of commerce_shipping can be moved to a sub-module as suggested in #2871241: Offer submodules that enforce configuration to match specific sources.

Try again.

Awesome! One concern

+++ b/src/Plugin/migrate/process/ubercart/d6/UbercartLineItem.php
@@ -0,0 +1,44 @@
+            'currency_code' => 'NZD',

This is forcing a currency code which is not the store default, or existing possible currency code.

Thanks, I completely forgot to go back and fix the currency.

The interdiff failed, but I added a diff since it was small.

Right something has changed, the failing tests have nothing to do with this patch. I was working off core 8.3 and these test pass. But they fail with core 8.4, without this patch.

Moving the patch from #6 in #2888764: Process plugin for default store currency to here.

• +++ b/src/Plugin/migrate/source/ubercart/d6/Order.php
  @@ -44,20 +79,38 @@ class Order extends SqlBase {
  +    $row->setSourceProperty('currency', $this->getUbercartCurrency());
  

  A possible problem here is that this will conflict with a custom currency migration. Maybe that is only relevant if multiple currencies are in use.

• +++ b/src/Plugin/migrate/source/ubercart/d6/Order.php
  @@ -69,4 +122,19 @@ class Order extends SqlBase {
  +  public function getUbercartCurrency() {
  

This is simple enough that a new method isn't needed.

A possible problem here is that this will conflict with a custom currency migration. Maybe that is only relevant if multiple currencies are in use.

In Drupal 6 I think it was very difficult to achieve multiple currencies unless you had multisites (as one of my sites does). As D6 ubercart did not save currencies in the order, or store, but in the site variables I think its safe to load it from D6 variables which I believe is what this line is doing (correct me if Im wrong)...

$this->variableGet('uc_currency_code', 'USD');

This is simple enough that a new method isn't needed.

Not sure I understand, are you saying that we shouldn't add it or that we can reuse the same method from elsewhere?

Please note that some of my changes seem to be removed from the patch 2888764-6.patch. In getAdjustmentData() I added a freach on teh adjustments to add the missing currency. The order save was throwing up loads of errors in Price.php because we are not passing the currency. Maybe this is staying in the other case...? https://www.drupal.org/node/2888764

Assigning to myself for review.

@willeaton, thanks for the information about Ubercart currencies. That keeps things simple.

Re the VariableGet, I mean we can put the VariableGet in prepareRow(). Which I have done in this patch. Also, there is a bit of renaming and the source plugin extends from DrupalSqlBase instead of SqlBase.

I have restored the setting of the currency_code in prepareRow. To be consistent we should, ideally, get the currency from the currency migration, which this is dependent on. But, as explained in #21, that isn't necessary in this case.

I think some of my feedback would be address with a re-roll now that #2894123: Move commerce to submodule has landed.

1. +++ b/src/Plugin/migrate/destination/ubercart/d6/EntityOrder.php
   @@ -0,0 +1,35 @@
   +namespace Drupal\commerce_migrate\Plugin\migrate\destination\ubercart\d6;
   ...
   +class EntityOrder extends EntityContentBase {
   

   Shouldn't this be called simply Order and not be placed in a uc6 sub-folder? This is the destination and we want the destinations to be re-usable.

2. +++ b/src/Plugin/migrate/destination/ubercart/d6/EntityOrder.php
   @@ -0,0 +1,35 @@
   +    $values = $row->getDestinationProperty('adjustments');
   +    if ($values) {
   +      $adjustments = [];
   +      foreach ($values as $adjustment) {
   +        $adjustment['amount'] = new Price($adjustment['amount']['number'], $adjustment['amount']['currency_code']);
   +        $adjustments[] = new Adjustment($adjustment);
   +      }
   +      $row->setDestinationProperty('adjustments', $adjustments);
   
   +++ b/src/Plugin/migrate/process/ubercart/d6/UbercartLineItem.php
   @@ -0,0 +1,44 @@
   +      foreach ($value as $adjustment) {
   +        $adjust = [];
   +        $adjust['delta'] = $i++;
   +        $adjust['type'] = 'custom';
   +        $adjust['label'] = $adjustment['title'];
   +        $adjust['amount'] =
   +          [
   +            'number' => $adjustment['amount'],
   +            'currency_code' => $adjustment['currency_code'],
   +          ];
   +        // @TODO: Adjustment sourceId and included.
   +        $adjustments[] = $adjust;
   

   I wondered here for a moment if it made sense to tie the source data so closely to what the destination order is expected. But after looking at it, I think it makes sense.

   OK, I take that back now. Why don't we just do everything in the process plugin for adjustments. Why do we need the altered destination and pre-import stuff.

3. +++ b/src/Plugin/migrate/process/ubercart/d6/UbercartLineItem.php
   @@ -0,0 +1,44 @@
   +        // @TODO: Adjustment sourceId and included.
   

   This @TODO isn't clear what is needed to be done.

4. +++ b/src/Plugin/migrate/source/ubercart/d6/Order.php
   @@ -44,20 +81,44 @@ class Order extends SqlBase {
        // Ubercart 6 stores credit card information in a hash. Since this probably
   ...
   +    $row->setSourceProperty('commerce_shipping', $this->moduleHandler->moduleExists('commerce_shipping'));
   

   This seems out of place here. Or at least a comment could help explain why we need the boolean.

1. Destination plugin gone.
2. Moved the creation of the Price and Adjustment to the process plugin.
3. The TODO was actually a note to myself, not meant to be left in the code.
4. Comment added. And thanks, this lead me to realize that the code using the commerce_shipping source value was missing. It is restored now and a test added to check the adjustment type is 'shipping' when commerce_shipping is enabled on the source.

Added '@requires module' to the annotation in the test.

Reroll. Including moving some files to the newish ubercart submodule.

Failure is

1) Drupal\Tests\commerce_migrate_ubercart\Kernel\Migrate\d6\OrderShippingTest::testOrder
PHPUnit_Framework_Exception: Unavailable module: 'commerce_shipping'. If this module needs to be downloaded separately, annotate the test class with '@requires module commerce_shipping'.

But the @requires is there.

Note the test passes locally, where the required modules are present.

The @requires doesn't appear to do anything. I looked at there is https://www.drupal.org/node/2728579, https://www.drupal.org/node/1273478 and a draft CR https://www.drupal.org/node/2857979 (which isn't very clear to me) All up, no indication of what to do now with such a test. Asked in #drupal-contribute, larowlan wasn't sure either.

So, I decided to put it into a submodule. It works locally.

Per #31 I'd like to talk to the PHPUnit people or Mixologic.

updated patch as per proposed solution in https://www.drupal.org/node/2728579. maybe it will clear test fails.

Here is an attempt to add require-dev. it's proven that @require does not work, and when talking to dawehner defining test_dependency will not work until we tag a release.

+++ b/composer.json
@@ -3,9 +3,12 @@
+        "drupal/coder": "^8",
+        "drupal/commerce_shipping": "~2.0"

I'm fine adding this as a dev dependency. This is really just a development module, anyways.

Real life test seems to work. I see custom line items imported without issues! Ready for dev?

1. +++ b/composer.json
   @@ -3,9 +3,12 @@
   +        "drupal/commerce_shipping": "~2.0"
   

   Nit: probably should be ^ instead of ~.

2. +++ b/modules/ubercart/tests/src/Kernel/Migrate/d6/OrderShippingTest.php
   @@ -0,0 +1,89 @@
   +  public function testOrder() {
   

   Nit: could this use a data provider?

3. +++ b/modules/ubercart/tests/src/Kernel/Migrate/d6/OrderTest.php
   @@ -54,9 +56,30 @@ class OrderTest extends Ubercart6TestBase {
   +      'label' => 'Shipping',
   

   Why are we testing this in two places? Isn't a single place enough?

1.Fixed
2. No, don't think so. Discussed this on IRC and we agree that dataproviders are useful, and preferred, but the don't really suite the migration environment and will make it difficult to find what item is causing the error. However, we should have a follow up to add the adjustments and cc_data to the assertOrder method. The adjusments aren't tested in commerce, so we can deal with all that in the followup.
3. Because 'test all the things'. And, the fixtures change over time, we should continue to confirm that the adjusment is correct.

Follow up added, #2901409: Add adjustments and data to assertOrder test

That's a little strange. Back in #37, there's a testOrder method. But it isn't there in #41. Did we drop something?

We didn't drop anything. The original OrderTest::testOrder is still there and so is the new on OrderShippingTest::testOrder. I double checked and applied the patch.

1. +++ b/modules/ubercart/src/Plugin/migrate/process/d6/UbercartLineItem.php
   @@ -0,0 +1,46 @@
   +        $adjust['type'] = $row->getSourceProperty('commerce_shipping') ? 'shipping' : 'custom';
   
   +++ b/modules/ubercart/src/Plugin/migrate/source/d6/Order.php
   @@ -45,20 +82,45 @@ class Order extends SqlBase {
   +    // The line item type depends on whether commerce_shipping is installed.
   +    // Save the state here for the UbercartLineItem process plugin.
   +    $row->setSourceProperty('commerce_shipping', $this->moduleHandler->moduleExists('commerce_shipping'));
   

   A comment about why if commerce shipping is installed we assume that the line item is a shipping would be helpful. There are non-shipping line items too, so documenting why we can safely assume all line items are shipping would be good. Or even better, do it in both places and improve the comment in Order::prepareRow().

2. +++ b/modules/ubercart/src/Plugin/migrate/source/d6/Order.php
   copy from modules/ubercart/tests/src/Kernel/Migrate/d6/OrderTest.php
   copy to modules/ubercart/tests/src/Kernel/Migrate/d6/OrderShippingTest.php
   

   I see why I didn't see the testOrder method. Git thought this was a copy.

3. +++ b/modules/ubercart/tests/src/Kernel/Migrate/d6/OrderShippingTest.php
   @@ -23,6 +26,9 @@ class OrderTest extends Ubercart6TestBase {
   +    'physical',
   

   I don't see this module listed as a test dependency in composer.json

4. +++ b/modules/ubercart/tests/src/Kernel/Migrate/d6/OrderShippingTest.php
   @@ -54,9 +60,30 @@ class OrderTest extends Ubercart6TestBase {
   +      'amount' => new Price('2.99', 'NZD'),
   ...
   +      'amount' => new Price('1000.00', 'NZD'),
   
   +++ b/modules/ubercart/tests/src/Kernel/Migrate/d6/OrderTest.php
   @@ -54,9 +56,30 @@ class OrderTest extends Ubercart6TestBase {
   +      'amount' => new Price('2.99', 'NZD'),
   ...
   +      'amount' => new Price('1000.00', 'NZD'),
   +++ b/modules/ubercart/src/Plugin/migrate/source/d6/Order.php
   @@ -45,20 +82,45 @@ class Order extends SqlBase {
   +    $currency_code = $this->variableGet('uc_currency_code', 'USD');
   

   I see this coming through in the fixture, but I don't see where we test the default currency code of USD coming from . I don't see test coverage of this. All the scenarios use NZD.

1. Not really sure about this. I implement what @mglaman suggested in #5
2. Oh yes, I copied it.
3. It is a dependency of commerce_shipping, doesn't composer pick that up from the commerce_shipping composer.json?
4. To test the default currency the source db needs to be modified. Is this necessary? I don't recall this being done in core for all the variable defaults.

Hmm, so what if our process plugin to create the adjustment also accepted the adjustment type? Then in the source we set the type to shipping or custom. Then the process plugin is more reusable.

Start with a reroll.

Implemented suggested changes in #47.

1. +++ b/modules/ubercart/src/Plugin/migrate/process/d6/UbercartLineItem.php
   @@ -0,0 +1,45 @@
   +        $adjust['type'] = $adjustment['type'];
   +        $adjust['label'] = $adjustment['title'];
   ...
   +          'number' => $adjustment['amount'],
   +          'currency_code' => $adjustment['currency_code'],
   

   So, we have 'type', 'title', 'amount', 'currency_code' in a nested array. If we want to make this process plugin re-usable for any and all line items, not just uc6, are those the best key names? They seem ok to me.

   So, the last thing then is to move this up to commerce_migrate as a re-usable process plugin. Naming... what about 'commerce_adjustments' / CommerceAdjustments? Plural because it seems to return an array of adjustments.

2. +++ b/modules/ubercart/src/Plugin/migrate/source/d6/Order.php
   @@ -112,10 +109,14 @@ class Order extends DrupalSqlBase {
   +    // The line item type depends on whether commerce_shipping is installed.
   +    // Save the state here for the UbercartLineItem process plugin.
   

   Some type of comment to the effect that by default the destination has custom adjustments in core and shipping if shipping module is enabled. But other adjustment types are possible, just not bundled with the vanilla migration. Maybe @mglaman can comment here with some better wording. For example from the IS, tax are also an adjustment. Are we just mapping them to custom? That seems strange.

are those the best key names? They seem ok to me.

Yes, here are the keys defined for adjustment value object: https://github.com/drupalcommerce/commerce/blob/8.x-2.x/modules/order/sr...

    $this->type = $definition['type'];
    $this->label = $definition['label'];
    $this->amount = $definition['amount'];
    $this->percentage = !empty($definition['percentage']) ? $definition['percentage'] : NULL;
    $this->sourceId = !empty($definition['source_id']) ? $definition['source_id'] : NULL;
    $this->included = !empty($definition['included']);

Adjustment mapping:

Taxes were never line items in 1.x; not sure Ubercart. In Ubercart and 1.x shipping is a line item, which we can and should migrate to shipping adjustment type. Anythign non-product should be custom for now. People can run alters to say "fee" or whatever else.

Actually. There's a new curveball here for shipping line items.

We need shipping line items to migrate to a shipment entity; which will cause the shipping adjustment to populate. Otherwise, if the order is refreshed, all adjustments not "custom" will be removed.

Confirmed with bojanz.

I'm not sure where to go. Perhaps we need a deriver which creates migrations per line item type discovered? Like the Node migration does. And have shipping line items go off and create shipment entities. This is where I'm not sure.

Finally, getting back to this.

Perhaps we need a deriver which creates migrations per line item type discovered?

I think that means that that each line item where the type and title form a unique set get migrated to a 'something'. I say 'something' because if type = 'shipping' then I am guessing it will always (?) be migrated to a shipping method. Then if type = 'tax' then it gets migrated to a tax type?, not sure about that.
I don't think this will work. Let's use shipping as the example. The uc_order_line_item contains the type (shipping) and the title (Acme transport). That isn't sufficient to determine the shipping plugin to use. At least this is true for Flat Rate service, which is the only shipping service I've explored. Maybe this is doable for many 'standard' services where perhaps a static map could be used to determine the plugin? Would that work? Possibly not as both the type field and title field are alterable by the user.

In the meantime, I've made migrations for the Flat Rate service for both Ubercart 6 and Commerce 1. I've yet to try them out with the patch here which surely needs a reroll.

Rerolled but it will fail tests.

And have shipping line items go off and create shipment entities. This is where I'm not sure.

Tried that anyway and turns out the shipment needs the order_id and it is a required field.
Required shipment field "order_id" is empty.

So, migrate the non shipping line items, then migrate the order, and the migrate shipping line items? Or can the shipping information be included in the order migration and will that automatically create the shipment entity?

We need shipping line items to migrate to a shipment entity;

How does this happen in Commerce 2? What does one due to create a shipment entity? I was thinking this might happen if a flat rate service was to be applied to all orders but the shipping is never applied to the order. The order type is 'default' and set to shippable as are all the product variations.

> How does this happen in Commerce 2? What does one due to create a shipment entity? I

That is a really good question!

I've just spent 5-10 minutes going through the code of the Commerce Shipping module, and I can't find where the shipment entities are actually created! The only call to create() I can see is in PackManager::packToShipments(), but that gets the shipments already in the order... I suspect that the first time that's called, the shipments array from the order is empty, and it's designed so that subsequent calls re-pack the order.

In which case, the creation of shipments is done when the ShippingInformation pane form is loaded.

If that doesn't happen (because we're migrating orders so the checkout UI isn't loaded), the subsequent order refresh calls to ShipmentOrderProcessor will have no effect, because that has a guard clause to check whether the order has any shipments at all.

So I reckon that during a migration, no shipment entities will be created.

The code to add a shipment is scattered all over. I had to pull it all together over in #2961463: Multiple destination shipping though, so you should see a manual way to pull it all together. Specifically, MultipleDestinationsForm:submitForm(). One thing of note, the ShippingOrderProcessor has shouldRepack(). This checks if 'owned_by_packer' is set. I think in our case, we don't want it set, but we do want to make sure payments are added on re-processing. We just don't want the packer to do it.

Needed a reroll.

This was all about getting the test to pass. The adjustments array has been removed for assertOrder since the adjustment is an object containing another object as well, the price, the assertSame didn't like it. Asserting the adjustments is done separately now. Since the assertion on the adjustment has been removed from assertOrder the other usages needed to be changed as well.

Rework the way the adjustment type is chosen in the source plugin and then updated the source plugin accordingly. The source plugin now gets the status of commerce_shipping module once and then only tests if 'shipping' needs to be changed to custom if the type is 'shipping'.
Also removed OrderShippingTest, it appears to be the start of a test to check just shipping and that idea isn't needed, we can test the adjustments in the order.

+++ b/modules/ubercart/src/Plugin/migrate/source/uc6/Order.php
@@ -17,9 +22,47 @@ use Drupal\migrate_drupal\Plugin\migrate\source\DrupalSqlBase;
+  /**

insert a blank line

I don't think the points in #50 and #51 have been addressed.

Back to NW for #50/#51/#59

The addresses #50 and #51. The process plugin has been moved to commerce_migrate and comment slightly improved in the Order source plugin.

Starting over, beginning with a reroll of the patch in #64.

Odd, that one line didn't get updated in the reroll process.

Reapply the fixes for #50 and #51 from #67.

The interdiff for the above patch

Update the IS

Now address #59 by suggesting that shipping be done in a follow up.

This patch adds tax line items and a 'service charge' line item and migrates them as part of the order. The order tests are updated to test these adjustments.

Some cleanup.

Follow up made to handle shipping line items. #2992272: Migrate shipping line items to order adjustments

Looks good now.

Sweet! 'Tis a day when some good news is needed too, extra bonus for that.

Needs reroll

Looks like I missed a file in that patch.

Tests passing, since this was just a reroll setting back to RTBC.

Thanks everyone!