Postponed on #2897114: Products migrated to both nodes and products

Problem/Motivation

The test fixture needs to be multilingual. Once translations are in the fixtures, we need to solve the following:

I have successfully migrated orders + products + order products. I don't have variations/attributes in my D6 use case. After import I modify a product variation and it gives me this error: "The SKU "birthday sea experience" is already in use and must be unique.". This is because migrate has migrated the English and Spanish nodes from D6 as two separate product variations in D8, both marked as English

See image attached. This is a query in the database for other variations with the same SKU. As you can see, its the same product, translated, but imported as 2 separate products.

I have come across this issue with migrate with normal nodes. Not sure if this is therefore migrate or commerce_migrate.

Proposed resolution

Add translations
Fix Product migrations ending up in 2 entities: Products migrated to both nodes and products
Import product variation language properly and do not import the products to two different NIDs
Also apply the same to product_kits

Remaining tasks

Do it.
Make patch
..

User interface changes

N/A

API changes

N/A

Data model changes

N/A

CommentFileSizeAuthor
#39 interdiff.txt600 bytesquietone
#39 2902425-39.patch842.04 KBquietone
#37 interdiff-31-37.txt6.82 KBquietone
#37 2902425-37.patch841.91 KBquietone
#35 interdiff.txt5.92 KBquietone
#35 2902425-35.patch842.98 KBquietone
#31 interdiff.txt1.33 KBquietone
#31 2902425-31.patch841.14 KBquietone
#29 2902425-29.patch840.57 KBquietone
#29 2902425-29-review-only.txt23.89 KBquietone
#27 2902425-27.patch839.97 KBquietone
#24 2902425-24-review-only.txt28.51 KBquietone
#24 2902425-24.patch856.12 KBquietone
#20 interdiff-18-20.txt56.86 KBquietone
#20 2902425-20-review-only.patch62.06 KBquietone
#20 2902425-20.patch889.68 KBquietone
#18 review_only-18.txt5.81 KBheddn
#16 2902425-16.patch833.45 KBquietone
#9 fixturetranslation-2902425-9.patch828.81 KBquietone
#7 fixturetranslation-2902425-7.patch827.62 KBquietone
#4 commerce_product_variation_field_data.txt499 byteswilleaton
#2 fixturetranslation-2902425-1.patch827.18 KBwilleaton
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

quietone created an issue. See original summary.

willeaton’s picture

willeaton CreditAttribution: willeaton as a volunteer commented
FileSize
fixturetranslation-2902425-1.patch827.18 KB

Fixed the various errors the installation produced. Enabled translation modules, created Spanish language, translated taxonomy terms (simply translation not language vocabs), created product translations, created order with translated product.

Documentation page also created: https://www.drupal.org/docs/8/modules/commerce-migrate/generating-databa...

willeaton’s picture

willeaton CreditAttribution: willeaton as a volunteer commented
Assigned: Unassigned » willeaton
Status: Active » Needs review
willeaton’s picture

willeaton CreditAttribution: willeaton as a volunteer commented
Title: Add translations to test fixture » [Ubercart D6] Migrate product translations
Issue summary: View changes
Status: Needs review » Needs work
FileSize
commerce_product_variation_field_data.txt499 bytes
willeaton’s picture

willeaton CreditAttribution: willeaton as a volunteer commented

The patch I created for the D6 fixtures failed and I need help to fix it. Also, modified the spec of this issue to be more related to the product translations import.

willeaton’s picture

willeaton CreditAttribution: willeaton as a volunteer commented
Issue summary: View changes

Added missing reference to product_kits

quietone’s picture

quietone CreditAttribution: quietone at Acro Commerce commented
Status: Needs work » Needs review
FileSize
fixturetranslation-2902425-7.patch827.62 KB

Reroll.

Status: Needs review » Needs work

The last submitted patch, 7: fixturetranslation-2902425-7.patch, failed testing. View results

quietone’s picture

quietone CreditAttribution: quietone at Acro Commerce commented
Status: Needs work » Needs review
FileSize
fixturetranslation-2902425-9.patch828.81 KB

Update the timestamps in the test to reflect what is really in the fixture.

quietone’s picture

quietone CreditAttribution: quietone at Acro Commerce commented
Issue summary: View changes
Status: Needs review » Postponed

Postponing on #2897114: Products migrated to both nodes and products

quietone’s picture

quietone CreditAttribution: quietone at Acro Commerce commented

Over in #2897114: Products migrated to both nodes and products willeaton tested the patch in #150
with a db with translations, nodes and products and reported the results #153. Referencing it here so we are sure to fix those problems. I had similar problems testing with the fixture in this patch but haven't looked at the results in detail.

edit: fix the issue number.

quietone’s picture

quietone CreditAttribution: quietone at Acro Commerce commented

Did some testing with this and #2897114: Products migrated to both nodes and products. There are still lots of errors, no need to list them all, to sort out. But for this issue lets select the first one, the migration of the language content settings. These are not migrated for the products, which makes sense because d6_language_content_settings is another migration with a constant value 'node' for the entity type.

Keeping this to adding the fixture and one migration, d6_language_content_settings with followups for any other error. That way this is a manageable size and sensible scope.

willeaton’s picture

willeaton CreditAttribution: willeaton as a volunteer commented

Sounds good. There are various translation issues which we can break down into manageable issues. I'll be monitoring the issue to help whenever you have doubts or ready to test anything

willeaton’s picture

willeaton CreditAttribution: willeaton as a volunteer commented

Hi @quietone. Can I help you out in any way with this? If you give me some pointers I could try and look into this but Im not even sure where to start really. Do you have any time booked in to look at this soon?
Thanks
Will

quietone’s picture

quietone CreditAttribution: quietone at Acro Commerce commented
Assigned: willeaton » Unassigned
Status: Postponed » Needs work

Making a brief note, I plan on making a patch after I take a walk. I made a d6_ubercart_language_content_settings migration based on the existing language_content_settings migration. Once that is run the various node_translation migrations seem to work. Promising

I have noticed 3 problems. One, the language_content_settings migration does not rollback properly. Once is it rolled back when the migration is run again this error is reported, [error] 'language_content_settings' entity with ID 'commerce_product.product' already exists. Two, the content_language_settings enables translation on the content but does not tick the individual fields (title, body etc). And three, there is nothing to enable translation of product variations.

quietone’s picture

quietone CreditAttribution: quietone at Acro Commerce commented
Status: Needs work » Needs review
FileSize
2902425-16.patch833.45 KB

This adds a migration for language content settings for commerce product and a test. No interdiff since it needed a reroll.

quietone’s picture

quietone CreditAttribution: quietone at Acro Commerce commented

Noting that I was able to migrate translations, the text fixture, when running migrations by hand. Starting from a clean install, enable commerces. Then migrate the node and product types and enable translations for product and product kit types. The migrate nodes and node translations. Still working on a reliable way to replicate that.

heddn’s picture

heddn
Primary language English
Location Nicaragua
CreditAttribution: heddn at MTech, LLC, Acro Commerce commented
FileSize
review_only-18.txt5.81 KB

Adding a review only patch.

quietone’s picture

quietone CreditAttribution: quietone at Acro Commerce commented

Been a bit of a comedy of errors to get the steps to reproduce migrating translations.

  1. fresh install, enable commerce modules and content_translation
  2. drush migrate-upgrade --legacy-db-url='your credentials' --configure-only
  3. drush -y cex
  4. edit upgrade_d6_node_product and upgrade_d6_node_translation_product
  • Change nid: tnid to product_id: tnid
  • Change the destination from destination: plugin: 'entity:node' to destination: plugin: 'entity:commerce_product'
  • Add this line to the required dependencies - upgrade_d6_ubercart_product_type
  • drush -y cim --partial
  • drush mi upgrade_d6_ubercart_product_type
  • Go to /admin/config/regional/content-language
  • Enable translation for Product (make sure body is enabled). For reason still unknown when running the full migration the body field become unticked, that is, not translatable.
  • drush mi --execute-dependencies upgrade_d6_node_product
  • drush mi --execute-dependencies upgrade_d6_node_translation_product
    • quietone’s picture

    quietone CreditAttribution: quietone at Acro Commerce commented
    FileSize
    2902425-20.patch889.68 KB
    2902425-20-review-only.patch62.06 KB
    interdiff-18-20.txt56.86 KB

    The first problem was that d6_ubercart_language_content_settings migration was not setting the body field translatable on the translated products. Finally tracked that down to MigrateProductType::postRowSave(), which add the stores, variation_ids and body fields to the product type. But the body field was defaulting to non-translatable. Therefor, a test was added that will set the body field translatable if content_translation exists.

    Next was to get the translations migrated. Here, it was simplest to let the node deriver do its work and just modify the product migrations as needed. But still no translations were migrated. That was because the product migration was modified to use the source plugin 'd6_ubercart_product' and that did not have any code to handle translations. So, changed the plugin to extend the core node source plugin and updated the query, fields, and prepareRow methods as needed. After that the translations were migrated.

    Now, since the d6_ubercart_product source plugin and the d6_ubercart_product migrate are no longer used, they have been deleted and tests updated as needed.

    And thanks to heddn I have remembered to add a review only patch.

    The last submitted patch, 20: 2902425-20.patch, failed testing. View results
    - codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

    Status: Needs review » Needs work

    The last submitted patch, 20: 2902425-20-review-only.patch, failed testing. View results
    - codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

    quietone’s picture

    quietone CreditAttribution: quietone at Acro Commerce commented

    That error is no surprise, I get it locally, and good news there were no others. The failure is in the ProductTest on this test taken from the core MigrateNodeTest. I don't know about the content_translation manager, yet.

    $manager = $this->container->get('content_translation.manager');
    $this->assertSame('en', $manager->getTranslationMetadata($product->getTranslation('es'))->getSource());
    quietone’s picture

    quietone CreditAttribution: quietone at Acro Commerce commented
    Status: Needs work » Needs review
    FileSize
    2902425-24.patch856.12 KB
    2902425-24-review-only.txt28.51 KB

    The test was missing migrate_plus, which is needed for the prepare row event. Once that was running found that the d6_node_type wasn't migrating the node types because the node configuration wasn't installed for the test.

    quietone’s picture

    quietone CreditAttribution: quietone at Acro Commerce commented
    Assigned: Unassigned » quietone
    Issue tags: +Needs reroll
    quietone’s picture

    quietone CreditAttribution: quietone at Acro Commerce commented
    Status: Needs review » Needs work
    quietone’s picture

    quietone CreditAttribution: quietone at Acro Commerce commented
    Status: Needs work » Needs review
    Issue tags: -Needs reroll
    FileSize
    2902425-27.patch839.97 KB

    I found rerolling the test fixture extremely difficult, which just continues to convince me we should have a 'complete as possible' fixture to start with and thus avoid difficult rerolls. This is not passing ProductTest and I don't see why. Let's find out if anything else is failing and that may point to the problem.

    Status: Needs review » Needs work

    The last submitted patch, 27: 2902425-27.patch, failed testing. View results
    - codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

    quietone’s picture

    quietone CreditAttribution: quietone at Acro Commerce commented
    Status: Needs work » Needs review
    FileSize
    2902425-29-review-only.txt23.89 KB
    2902425-29.patch840.57 KB

    Its always simple when you find it. Think this will work. Added a few comments as well.

    Status: Needs review » Needs work

    The last submitted patch, 29: 2902425-29.patch, failed testing. View results
    - codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

    quietone’s picture

    quietone CreditAttribution: quietone at Acro Commerce commented
    Status: Needs work » Needs review
    FileSize
    2902425-31.patch841.14 KB
    interdiff.txt1.33 KB

    Yes, the source plugin was modified so updated the tests. Removed debug stuff left in.

    Status: Needs review » Needs work

    The last submitted patch, 31: 2902425-31.patch, failed testing. View results
    - codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

    quietone’s picture

    quietone CreditAttribution: quietone at Acro Commerce commented
    Status: Needs work » Needs review

    Tests are passing

    heddn’s picture

    heddn
    Primary language English
    Location Nicaragua
    CreditAttribution: heddn at MTech, LLC, Acro Commerce commented
    Status: Needs review » Needs work 
    1. +++ b/modules/ubercart/commerce_migrate_ubercart.module
@@ -78,9 +81,20 @@ function commerce_migrate_ubercart_migration_plugins_alter(&$migrations) {
+      // content_translation_update_definitions, that is not changable in the

      Is there a core issue for this? Is is good to fix this upstream so it is configurable? If so, let's add an @TODO with link to the issue. 

    2. +++ b/src/EventSubscriber/MigrateProduct.php
@@ -33,13 +33,13 @@ class MigrateProduct implements EventSubscriberInterface {
+//    if ($event->getMigration()->id() == 'd7_commerce_product') {
+//      $product_id = $event->getRow()->getDestinationProperty('product_id');

+++ b/tests/src/Kernel/CommerceMigrateTestTrait.php
@@ -400,7 +400,7 @@ trait CommerceMigrateTestTrait {
+    //$this->assertSame($variations, $product->getVariationIds());

      Let's not leave code commented out in the code base unless there's a good reason and a comment.

    quietone’s picture

    quietone CreditAttribution: quietone at Acro Commerce commented
    Status: Needs work » Needs review
    FileSize
    2902425-35.patch842.98 KB
    interdiff.txt5.92 KB

    1. Made an issue, https://www.drupal.org/node/2930050
    2. Looks like I wasn't finished with this at all. I had 'parked' the variations and didn't get back to it. Removed the commented lines, updated tests, and was able to remove the postRow save. Let's see if I broke something else.

    Status: Needs review » Needs work

    The last submitted patch, 35: 2902425-35.patch, failed testing. View results

    quietone’s picture

    quietone CreditAttribution: quietone at Acro Commerce commented
    Status: Needs work » Needs review
    FileSize
    2902425-37.patch841.91 KB
    interdiff-31-37.txt6.82 KB

    Head moved while I was working on this. Ignore #35.

    Status: Needs review » Needs work

    The last submitted patch, 37: 2902425-37.patch, failed testing. View results
    - codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

    quietone’s picture

    quietone CreditAttribution: quietone at Acro Commerce commented
    Status: Needs work » Needs review
    FileSize
    2902425-39.patch842.04 KB
    interdiff.txt600 bytes

    Add the dependencies to the test.

    heddn’s picture

    heddn
    Primary language English
    Location Nicaragua
    CreditAttribution: heddn at MTech, LLC, Acro Commerce commented
    Status: Needs review » Reviewed & tested by the community

    I cannot see anything wrong here. Tests seem to cover everything. LGTM.

    • heddn committed 39f93e0 on 8.x-2.x authored by quietone 
      Issue #2902425 by quietone, willeaton, heddn: [Ubercart D6] Migrate...
    heddn’s picture

    heddn
    Primary language English
    Location Nicaragua
    CreditAttribution: heddn at MTech, LLC, Acro Commerce commented
    Status: Reviewed & tested by the community » Fixed
    willeaton’s picture

    willeaton CreditAttribution: willeaton commented

    Great news. I'll test it out tomorrow!

    Status: Fixed » Closed (fixed)

    Automatically closed - issue fixed for 2 weeks with no activity.