Create menu sub item to "Add Feeds Migration"

Tagging for DrupalCamp Asheville

Tagging for the next to North American contrib days, Asheville and Colorado

DrupalCamp Asheville contrib days are July 13-14, 2019
DrupalCamp Colorado contrib day is Aug 4, 2019

Patch forthcoming.

Patch attached.

Revised patch addressing coding standards. No changes from this patch should affect unit testing.

Hi,

After applying the patch

1) i get to see submenu item called "Add Migration" under "Migrations" Menu
2) I can confirm that the Source plugin dropdown set to "url". and Migration Group dropdown set to "Feeds Migration" (Note: Remember "Feeds Migration" should exist there)

I have screencasted the testing experience here: https://www.screencast.com/t/Ebeo0eIai

Thanks
Vijay

Thanks for your review Vijay.

The patch failed testing though so we should resolve those test failures before we can mark RBTC.

There were 2 errors:

1) Drupal\Tests\feeds_migrate\FunctionalJavascript\Form\MigrationFormTest::testAddMigration
Behat\Mink\Exception\ElementNotFoundException: Form field with id|name|label|value "migration[label]" not found.

/var/www/html/vendor/behat/mink/src/WebAssert.php:638
/var/www/html/modules/contrib/feeds_migrate/tests/src/FunctionalJavascript/Form/MigrationFormTest.php:42

2) Drupal\Tests\feeds_migrate\FunctionalJavascript\Form\MigrationFormTest::testAddMigrationForTaxonomyTerm
Behat\Mink\Exception\ElementNotFoundException: Form field with id|name|label|value "migration[label]" not found.

/var/www/html/vendor/behat/mink/src/WebAssert.php:638
/var/www/html/modules/contrib/feeds_migrate/tests/src/FunctionalJavascript/Form/MigrationFormTest.php:108

I have tried out this patch in my local environment.

Patch: feeds_migrate-menu_sub_item-3063073-9.patch
Drupal: 8.8.x-dev
PHP: 7.3

After applied the patch, I ran the test and I got the following error.

After fixing the deprecated notices, I ran the test successfully and all are get passed.

Here is the latest patch
feeds_migrate-menu_sub_item-3063073-14.patch

Thanks.

Thanks for the patch!

If you set the Status to "Needs review" once you submit a patch the test bots will check it over here too. Setting to "Needs review".

Looks like testbot is failing due to a composer require error.

Talking with the Feeds team it sounds like this may be a common problem with failing tests and composer.

Lets identify the issue and create a new issue for it.

https://www.drupal.org/project/feeds_migrate/issues/3002362#comment-1331...

Tagging for sprint.

retagging

Lets check to see if test bot is still failing.

Can anyone help me on this.

1) The patch #14 passes the test only from drupal 8.8 .
2) It stops with composer error in previous version .
3) Actually patch #6 is working fine.
4) But the bot says that, It failed with 2 tests.
5) I am unable to replicate that error in local environment.

Here is that error: https://www.drupal.org/project/feeds_migrate/issues/3063073#comment-1323...

Thanks.

Trying the patch from earlier to see if the test bots like it now in D8.8.

I can see that the element identified by migration[label] is not being found in the markup generated by the form.

This function is failing when trying to find it in two places: https://api.drupal.org/api/drupal/vendor%21behat%21mink%21src%21WebAsser...

I'd check the HTML output of the tests after they run, making sure you're using: Drupal 8.8.x

If there is no migration[label] in the markup, we need to find where that markup is being added and why it's failing.

This could be failing as a result of changing the test class. It's weird the form would get messed up from a simple menu change.

-use Drupal\Tests\taxonomy\Functional\TaxonomyTestTrait;
+use Drupal\Tests\taxonomy\Traits\TaxonomyTestTrait;

@jamesdixon

1) I am sure that the test is not failing because of deprecation fix.

-use Drupal\Tests\taxonomy\Functional\TaxonomyTestTrait;
+use Drupal\Tests\taxonomy\Traits\TaxonomyTestTrait;

Because before making those changes ( https://www.drupal.org/project/feeds_migrate/issues/3063073#comment-1320... ) we faced the same error.

2) I didn't find the "migration[label]" markup in the code level.

But I find it here /admin/structure/migrate/sources/add .

In the test file MigrationFormTest.php(/tests/src/FunctionalJavascript/Form/MigrationFormTest.php) they are checking for the field "migration[label]" in the path /admin/structure/migrate/sources/add .

The path "/admin/structure/migrate/sources/add" will be accessible only if the feeds_migrate_ui is enabled otherwise it returns 404.

So my guessing is

a) The test accessing this path /admin/structure/migrate/sources/add .
b) Because of the module feeds_migrate_ui is not enabled it will get 404 and unable to find the "migration[label]" field.

If my guessing is right, we may have to add test dependencies in feeds_migrate.info.yml or it should be included in composer.json .

But I am wondering why this is happening, because the required modules array already contains "feeds_migrate_ui".

Thanks.

@jamesdixon, @Chandru Ranganathan
The reason the test failed is because a fatal errors occurs:
Drupal\Component\Plugin\Exception\PluginNotFoundException: The "" plugin does not exist. Valid plugin IDs for Drupal\migrate\Plugin\MigrateSourcePluginManager are: null, empty, embedded_data, url in Drupal\Core\Plugin\DefaultPluginManager->doGetDefinition() (line 53 of core/lib/Drupal/Component/Plugin/Discovery/DiscoveryTrait.php).
See https://dispatcher.drupalci.org/job/drupal8_contrib_patches/14365/artifa...

To see details of a failed test:

1. Click on the red bar that displays the test results:
   
2. Click on "View results on dispatcher":
   
3. Go to "Artifacts":
   
4. For javascript tests, go to jenkins-drupal8_contrib_patches-14365/artifacts/run_tests.js/simpletest_html.
   For functional tests, go to jenkins-drupal8_contrib_patches-14365/artifacts/run_tests.standard/simpletest_html.

You'll then a see list of html files that were output during the tests:

Click the ones belonging to failed tests to check the output.

Hi MegaChriz,

Thanks. You are absolutely right and I am able to replicate it in my local environment.
Actually the problem is with assigning the default plugin.
Now I have fixed this issue.
Here is the latest patch.
feeds_migrate-menu_sub_item-3063073-26.patch
Can anyone please review it.
Thanks.

1) Actually in drupal 8.7 the tests are failing because of deprecation fix we have done for drupal 8.8
2) I have reverted the deprecation fix .
3) I think Now it will get passed in drupal 8.7

Thanks.

I've tested this patch and confirmed 3 things are happening:

1) The Structure > Migrations > Add Migration link is added
2) When adding a Migration the default migration group is set if it exists
3) When adding a Migration the source plugin is set to default of url if it exists

The code looks good to me.

@Megachriz mentioned this patch conflicts with: https://www.drupal.org/project/feeds_migrate/issues/3002362

Should we commit this feature, or wait until https://www.drupal.org/project/feeds_migrate/issues/3002362 is moved along further?

I committed #3002362-23: Foundation: Feeds migrate Processors/Tamper UI .

Here is a reroll.

Patch is functional, though I have the following remarks:

1. +++ b/modules/feeds_migrate_ui/src/Form/MigrationForm.php
   @@ -109,6 +129,13 @@ class MigrationForm extends EntityForm {
   +    // If the migration group 'feeds_migration' exists, set it as the entity's
   +    // migration group.
   

   The comment here talks about the migration group 'feeds_migration'. But what the group defined in the url is, doesn't have to be that. Therefore, I think this comment need to be reworded.

2. +++ b/modules/feeds_migrate_ui/src/Form/MigrationForm.php
   @@ -109,6 +129,13 @@ class MigrationForm extends EntityForm {
   +    if (array_key_exists($params['migration_group'], $groups)) {
   

   When going to "/admin/structure/migrate/sources/add" (this action link is displayed on /admin/structure/migrate/manage/feeds_migration/migrations), then you get a PHP notice:

   Undefined index: migration_group in Drupal\feeds_migrate_ui\Form\MigrationForm->prepareEntity()

   I know there are more notices on the page, but this one is new.

Thanks @irinaz.

Hi @MegaChriz

You are right, there is a new notice with (migration_group).
I have fixed it.

Here is the latest patch. feeds_migrate-menu-sub-item-3063073-36.patch

Please take a look and let me know your comments.

Thanks..

+++ b/modules/feeds_migrate_ui/src/Form/MigrationForm.php
@@ -109,6 +129,13 @@ class MigrationForm extends EntityForm {
+    if (array_key_exists('migration_group', $params) && array_key_exists($params['migration_group'], $groups)) {

@megachriz suggests we use isset($the_array['key']) instead of array_key_exists() for both $params and $groups. array_key_exists() should only be used if the array value NULL is acceptable.

+++ b/modules/feeds_migrate_ui/src/Form/MigrationForm.php
@@ -109,6 +129,13 @@ class MigrationForm extends EntityForm {
+    // Default migration group is set to 'feeds_migration'.
+    // If 'feeds_migration' group exists, set it as the entity's migration group.

I think the comment shouldn't mention 'feeds_migration' at all.

How about the following:

If on the url a query parameter for 'migration_group' is set, check if a migration group exists with that name. If so, set it as the entity's migration group.

Other suggestions are welcome. :)

Here I have made changes as suggested in comment #37 and #39.

This one looks good to go. Thanks for the patch ravi!

This has been committed.

Thanks everyone!