Products migrate fine, however they're missing their store field and it is not assigned. I'm assuming it is due to this missing dependency.

CommentFileSizeAuthor
#2 product_display-2899149-2.patch475 bytesmglaman

Comments

mglaman created an issue. See original summary.

mglaman’s picture

mglaman
Primary language English
Location WI, USA
commented
Status: Active » Needs review
StatusFileSize
new product_display-2899149-2.patch475 bytes
heddn’s picture

heddn
Primary language English
Location Illinois
commented
Status: Needs review » Needs work
Issue tags: +Needs tests

So, this probably needs tests then. Back to NW for tests.

mglaman’s picture

mglaman
Primary language English
Location WI, USA
commented

Hm, I'm wondering how we could test since we have to manually run migration dependencies in tests.

See http://cgit.drupalcode.org/commerce_migrate/tree/modules/commerce/tests/...

    $this->executeMigrations([
      'd7_user_role',
      'd7_user',
      'd7_product_type',
      'd7_product',
      'd7_product_display_type',
    ]);

    $this->executeMigrations([
      'd7_product_display',
    ]);
heddn’s picture

heddn
Primary language English
Location Illinois
commented

Interesting point in #4. We should probably add an integration test that tests all the things. Would that catch this? Or we can introspect a migration for its dependencies and run them. So we don't have to manually figure out what they are.

BTW, I don't see anywhere in d7_product_display_type.yml or d7_product_display.yml where we are mapping the store.

quietone’s picture

quietone commented

Correct me if I am wrong, but AFAIKT there isn't a similar test in core for migrations. This would be a new type of test to be added to all the commerce migrations. If that is true, I'd prefer to see that in a follow up. Having said that the closest thing in core is the UI testing in migrate_drupal_ui. It will run the migrations found by discovery and run them in dependency order. If a dependency is missing then a migration would fail in that test. Maybe we can use that method here. If that seems reasonable then the source plugin annotations need to fixed, #2899495: Add source_module and destination_module to the sources and destionations

I don't see anywhere in d7_product_display_type.yml or d7_product_display.yml where we are mapping the store.

.
Yes, the default store migration should be there as a dependency. Those migration tests run MigrateStore so it is there for the tests.


+++ b/modules/commerce/migration_templates/d7_product_display.yml
@@ -23,5 +23,6 @@ destination:
     - d7_product_type

This can be removed, d7_product depends on d7_product_type.

And, I think, d7_product should have a dependency on d7_default_commerce_store_added No, I just mixed up the names. This is being fixed in #2899062: Order migration does not list store migration as dependency

heddn’s picture

heddn
Primary language English
Location Illinois
commented
Status: Needs work » Closed (duplicate)
Related issues: +#2899062: Order migration does not list store migration as dependency

This would be a new type of test to be added to all the commerce migrations. If that is true, I'd prefer to see that in a follow up.

I've opened #2899672: Commerce 1 Test "All The Things".

I also just committed #2899062: Order migration does not list store migration as dependency, to which I'm closing this as duplicate.