[Ubercart D6] Taxes

Retesting now that HEAD is fixed.

1. +++ b/migration_templates/d6_ubercart_tax.yml
   @@ -0,0 +1,26 @@
   +  'configuration/rates': config_rate
   

   I think we can use sub-fields or even sub_process here instead of the work in the source plugin.

2. +++ b/src/Plugin/migrate/source/ubercart/Tax.php
   @@ -0,0 +1,103 @@
   +      'rate' => $this->t('Tax Rate'),
   ...
   +    $row->setSourceProperty('config_rate', $config_rate);
   

   I don't see config_rate in the fields array.

3. +++ b/src/Plugin/migrate/source/ubercart/Tax.php
   @@ -0,0 +1,103 @@
   +        'id' => $this->uuidGenerator->generate(),
   

   This makes the source migration non-idempotent. Meaning, I cannot re-run the migration and expect the same results. Can we leave it to the destination entity to auto-generate the UUID for us?

And, it needs another re-roll again.

1. +++ b/src/EventSubscriber/MigrateTaxType.php
   @@ -0,0 +1,64 @@
   +class MigrateTaxType implements EventSubscriberInterface {
   
   +++ b/tests/src/Kernel/CommerceMigrateTestTrait.php
   @@ -314,4 +315,22 @@ trait CommerceMigrateTestTrait {
   +  public function assertTaxType($id, $expected_label, $expected_plugin) {
   

   I'd like to see a test that verifies we are assigning the UUID. I bet that could be easily added to assertTaxType() pretty easily.

2. +++ b/src/EventSubscriber/MigrateTaxType.php
   @@ -0,0 +1,64 @@
   \ No newline at end of file
   

   Nit: missing new line.

I'm not seeing a UUID assignment test. Back to NW for that last piece.

I did some more research also why we need to set the UUID. It only seems to apply to custom tax rates. Non-custom don't use a UUID. So we need to check for type of tax in the subscriber. Also, it might make sense to move the assignment of the UUID into an event subscriber in commerce core, rather than just in migrate. If anyone wants to interact with a custom tax rate using the entity api, they are going to run into the same situation.

+++ b/modules/ubercart/migration_templates/d6_ubercart_tax.yml
@@ -0,0 +1,27 @@
+  id:
+    -
+      plugin: machine_name
+      source: name

We don't need a UUID. This is good enough.

EDIT: We can use same pattern for the rate ID.

OK, to summarize a chat I had with @bojanz and mglamman in Slack:

The rate id doesn't need to be a UUID. It can be any value that is unique within that tax type. The id in D6 UC is unique. So, let's ditch the event subscriber and just map the id as the rate's id too.

	+      'id' => $this->t('Tax ID'),
+      'name' => $this->t('Tax Name'),
+      'rate' => $this->t('Tax Rate'),

+++ b/modules/ubercart/migration_templates/d6_ubercart_tax.yml
@@ -0,0 +1,27 @@
+  id:
+    -
+      plugin: machine_name
+      source: name
+    -
+      plugin: make_unique_entity_field
+      entity_type: commerce_tax_type
+      field: id
+      length: 32
+  label: name
+  'plugin': 'constants/plugin'
+  'configuration/rates/0/label': name
+  'configuration/rates/0/amount': rate
+destination:

Or better yet, just map the @id to configuration/rates/0/id.

Like this:

  id:
    -
      plugin: machine_name
      source: name
    -
      plugin: make_unique_entity_field
      entity_type: commerce_tax_type
      field: id
      length: 32
  label: name
  'plugin': 'constants/plugin'
  'configuration/rates/0/id': '@id'  # <== See this?
  'configuration/rates/0/label': name
  'configuration/rates/0/amount': rate

\o/ yay. Looks good to me. Thanks for updated fix.

The following needs removing from modules/ubercart/migration_templates/d6_ubercart_tax.yml

migration_group: ubercart

As per https://www.drupal.org/node/2898755

Looks good. Just a bunch of changes to catch up with some naming changes happening in parallel. Also, when I got to the end of that patch I saw the test was for a tax type. So, the migration and source plugin names should be changed to include 'type'.

1. +++ b/modules/ubercart/migration_templates/d6_ubercart_tax.yml
   @@ -0,0 +1,28 @@
   +label: Migrate taxes from Ubercart
   

   Please start with Ubercart, 'Ubercart 6 taxes'

2. +++ b/modules/ubercart/migration_templates/d6_ubercart_tax.yml
   @@ -0,0 +1,28 @@
   +label: Migrate taxes from Ubercart
   ...
   +  - Drupal 6 Ubercart
   

   Just 'Ubercart'

3. +++ b/modules/ubercart/migration_templates/d6_ubercart_tax.yml
   @@ -0,0 +1,28 @@
   +migration_group: ubercart
   

   Remove migration_group

4. +++ b/tests/src/Kernel/CommerceMigrateTestTrait.php
   @@ -314,4 +315,29 @@ trait CommerceMigrateTestTrait {
   +   * @param string $expected_label
   

   Remove the expected_ prefix. s/expected_//

5. +++ b/modules/ubercart/migration_templates/d6_ubercart_tax.yml
   @@ -0,0 +1,28 @@
   +id: d6_ubercart_tax
   

   s/d6_ubercart_tax/d6_ubercart_tax_type/

6. +++ b/modules/ubercart/migration_templates/d6_ubercart_tax.yml
   @@ -0,0 +1,28 @@
   +  plugin: d6_ubercart_tax
   

   s/d6_ubercart_tax/d6_ubercart_tax_type/

7. +++ b/modules/ubercart/src/Plugin/migrate/source/Tax.php
   @@ -0,0 +1,51 @@
   + *   id = "d6_ubercart_tax"
   

   s/d6_ubercart_tax/d6_ubercart_tax_type/

8. +++ b/modules/ubercart/tests/src/Kernel/Migrate/d6/TaxTest.php
   @@ -0,0 +1,40 @@
   + * Tests tax migration.
   

   s/tax/tax type/

9. +++ b/modules/ubercart/tests/src/Kernel/Migrate/d6/TaxTest.php
   @@ -0,0 +1,40 @@
   +class TaxTest extends Ubercart6TestBase {
   

   s/TaxTest/TaxTypeTest/

Thanks, swickham. Just 2 nits.

1. +++ b/modules/ubercart/migration_templates/d6_ubercart_tax_type.yml
   @@ -0,0 +1,27 @@
   +# Ubercart 6 taxes.
   

   Should be tax types. Or, since there is a label field, the comment is superfluous, delete this comment.

2. +++ b/modules/ubercart/migration_templates/d6_ubercart_tax_type.yml
   @@ -0,0 +1,27 @@
   +label: Migrate taxes from Ubercart
   

   Should be tax types. Suggest just 'Ubercart 6 tax types'.

@swickham, thanks. All points addresses.