Migrate UI - refer to modules and add help text

1. +++ b/core/modules/migrate_drupal_ui/src/Form/MigrateUpgradeForm.php
   @@ -2,6 +2,7 @@
   +use Drupal\Component\Utility\Random;
   

   Unused import.

2. +++ b/core/modules/migrate_drupal_ui/src/Form/MigrateUpgradeForm.php
   @@ -535,17 +537,17 @@ public function buildConfirmForm(array $form, FormStateInterface $form_state) {
   +      '#description' => $this->t('The following modules will not be upgraded. For more information see <a href=":migrate">Upgrading from Drupal 6 or 7 to Drupal 8</a>.', [':migrate' => 'https://www.drupal.org/upgrade/migrate']),
   

   It would be super nice/helpful to refer this to a specific section on the upgrade handbook page that discusses that modules without upgrade paths isn't a super bad thing in a lot of cases.

Re #8.2. I can write a new documentation page for this during the next couple of days so that we can link directly to that.

The correct page in the handbook actually exists already: https://www.drupal.org/docs/8/upgrade/upgrade-using-the-browser-user-int.... I'll update that page to reflect the UI changes here.

Addressin 8.1 and 8.2. I also renamed the handbook page to 'Upgrade using web browser'.

+++ b/core/modules/migrate_drupal_ui/src/Form/MigrateUpgradeForm.php
@@ -535,17 +536,17 @@ public function buildConfirmForm(array $form, FormStateInterface $form_state) {
+      '#description' => $this->t('The following modules will not be upgraded. For more information, see the <a href=":migrate">Upgrading to Drupal 8 handbook</a>.', [':migrate' => 'https://www.drupal.org/docs/8/upgrade/upgrade-using-web-browser']),

Can we update the title here? And/or can we link to a #target on the page that specifically addresses modules not getting upgraded.

I updated the handbook page to reflect changes introduced in this issue. The attached patch now links directly to the correct anchor on the page as suggested by heddn in his previous comment.

Back to NR.

Cheers,
Markus

This is looking pretty good; thanks for your work on this!

1. +++ b/core/modules/migrate_drupal_ui/src/Form/MigrateUpgradeForm.php
   @@ -169,8 +170,8 @@ public function buildOverviewForm(array $form, FormStateInterface $form_state) {
   -          ':url' => 'https://www.drupal.org/upgrade/migrate',
   -        ]),
   +            ':url' => 'https://www.drupal.org/upgrade/migrate',
   +          ]),
   

   This looks out of scope.

2. +++ b/core/modules/migrate_drupal_ui/src/Form/MigrateUpgradeForm.php
   @@ -561,31 +562,33 @@ public function buildConfirmForm(array $form, FormStateInterface $form_state) {
          '#weight' => 3,
   
   @@ -632,12 +635,16 @@ public function buildConfirmForm(array $form, FormStateInterface $form_state) {
   +      $form['upgrade_option_item'] = [
   +        '#type' => 'item',
   +        '#description' => $this->t('Here is a summary of the upgrade status for all installed modules on the old site.'),
   +      ];
   

   I think this text is unneeded and should be removed. (Counterintuitively, less help text is usually better for UX, especially if it doesn't add much information.)

   +++ b/core/modules/migrate_drupal_ui/src/Form/MigrateUpgradeForm.php
   @@ -561,31 +563,33 @@ public function buildConfirmForm(array $form, FormStateInterface $form_state) {
   +      '#description' => $this->t('This is a list of the modules that will be upgraded and the Drupal 8 module or modules it will be upgraded to.'),
   

   Similarly, I think this text is unneeded and should be removed.

3. +++ b/core/modules/migrate_drupal_ui/src/Form/MigrateUpgradeForm.php
   @@ -697,7 +704,7 @@ protected function getDatabaseTypes() {
   -    return $this->t('Upgrade analysis report');
   +    return $this->t('Pre upgrade analysis');
   
   +++ b/core/modules/migrate_drupal_ui/tests/src/Functional/MigrateUpgradeTestBase.php
   @@ -154,7 +154,7 @@ public function testMigrateUpgrade() {
   -    $this->assertText('Upgrade analysis report');
   +    $this->assertText('Pre upgrade analysis');
   
   @@ -173,7 +173,7 @@ public function testMigrateUpgrade() {
   +    $this->assertSession()->pageTextContains('Pre upgrade analysis');
   

   "Pre-upgrade" should be hyphenated.

@plach, @catch, @larowlan, @effulgentsia, and I discussed this issue today. We agreed that unlike most of the Migrate issues, this issue can be major (we wouldn't block marking a stable release on it). I think it's close in any case. :)

All feedback is now addressed. Looks good.

Only found a couple of minor things...

1. +++ b/core/modules/migrate_drupal_ui/src/Form/MigrateUpgradeForm.php
   @@ -15,6 +15,7 @@
   +use Drupal\migrate_drupal_ui\MigrateUpgrade;
   

   This doesn't appear to be used anywhere?

2. +++ b/core/modules/migrate_drupal_ui/src/Form/MigrateUpgradeForm.php
   @@ -776,8 +778,8 @@ public function buildConfirmForm(array $form, FormStateInterface $form_state) {
   +        '#text' => $this->formatPlural($missing_count, 'Module will not be upgraded', 'Modules will not be upgraded'),
   

   Should the strings have the @count placeholder in them? For example, "@count modules will not be upgraded"?

3. +++ b/core/modules/migrate_drupal_ui/src/Form/MigrateUpgradeForm.php
   @@ -786,7 +788,7 @@ public function buildConfirmForm(array $form, FormStateInterface $form_state) {
   +        '#text' => $this->formatPlural($available_count, 'Module will be upgraded', 'Modules will be upgraded'),
   

   Same here.

I feel like this needs screenshots and possibly usability review, so I'm leaving it in review for now...

Alrighty then. RTBC once Drupal CI passes it.

1. Someone was too aggressive in hiding files. We want to see the final patch! (fixed, I think)

2. +++ b/core/modules/migrate_drupal_ui/css/components/upgrade-analysis-report-tables.css
   @@ -18,3 +18,6 @@
    .upgrade-analysis-report__status-icon--checked:before {
      background-image: url(../../../../misc/icons/73b355/check.svg);
    }
   +.upgrade-analysis-report__status-icon--error:before {
   +  background-image: url(../../../../misc/icons/e32700/error.svg);
   +}
   

   This is totally out of scope, and I see you are being consistent with the code that is already there, but this looks very fragile. Can you file an issue to change those to /core/misc/icons/73b355/check.svg and /core/misc/icons/e32700/error.svg? Maybe there is already an issue?

3. +++ b/core/modules/migrate_drupal_ui/tests/src/Functional/MigrateUpgradeTestBase.php
   @@ -191,13 +191,13 @@ public function testMigrateUpgrade() {
        $all_available = $this->getAvailablePaths();
        foreach ($all_available as $available) {
          $session->elementExists('xpath', "//span[contains(@class, 'checked') and text() = '$available']");
   -      $session->elementNotExists('xpath', "//span[contains(@class, 'warning') and text() = '$available']");
   +$session->elementNotExists('xpath', "//span[contains(@class, 'error') and text() = '$available']");
        }
   

   #14.1 asked for the removal of an out-of-scope indentation fix. Unless I am missing something, this is introducing an indentation error.

4. I guess the formatPlural() calls questioned in #22.1 and #22.2 end up at the top of the page, and '#theme' => 'status_report_counter', takes care of prefixing them with the count. Is that right? This is much easier to see now that there are screenshots, so thanks for that!
5. @quietone makes a good point (Comment #2) that "Upgrade analysis report" suggests that the upgrade has already been done. I do not love the replacement, "Pre-upgrade analysis". Maybe I will think of something better after I get some sleep.
6. The one thing that I fear is potentially confusing is referring to "Modules that will (not) be upgraded." I think of upgrading a module as replacing the module in the codebase with a newer version. That is not what this report is about! The site builder is responsible for installing the required modules on the D8 site before getting to this point (or coming back to this page after installing a few more modules). The upgrade process that comes after this page is about converting configuration and content related to the modules in these tables.

   I agree with @xjm (Comment #14) that the page is better without the additional text at the top of the page. But I think the best reason for getting rid of that sentence is that it did not add any information. In the spirit of "less is more", I do not want to change any of the phrases IN ALL CAPS: keep them short, even if that means keeping them imprecise.

   My suggestion is to expand the sentence under the heading "Modules that will not be upgraded". I think this is the fieldset that is open by default. We need to be clear here about what will happen if the user goes ahead with the upgrade: configuration and content will be lost. Here is a draft suggestion:

   There are no modules installed on the Drupal 8 site to replace these modules. If you proceed with the upgrade now, configuration and/or content related to these modules will be lost. For more information, see [Review the pre-upgrade analysis] in the [Upgrading to Drupal 8] handbook.

   Maybe "managed by" or "handled by" instead of "related to"?

   When linking to the handbook, my personal preference is to use the title of the target page as the link text. For example, I do not make "handbook" part of the link. With my longer proposed text, I think it is OK to include two links. That would be too much in the shorter version.

(I hope the block quote inside the list looks better when I click "Save" than it does after "Preview"!)

Maybe d.o is the one being too aggressive about hiding files. If I hide the interdiff, will the latest patch show?

1. I guess that d.o shows at most five files. When I first looked at the issue, that meant four screenshots and one interdiff, no patches.
2. Sorry, what I meant was that my suggestion was out off scope, which is why I requested the follow-up. Thanks for creating the new issue. Good idea to tag it as a Novice issue!
3. OK
4. OK
5. Here are some suggestions:
   • Upgrade readiness
   • Upgrade readiness analysis
   • What will be upgraded?

   If you do not like any of these, you can keep it the way it is.

6. Good point that "configuration and/or content ... will be lost" is inaccurate. The current draft is
   

   There are no modules installed on your new site to replace these modules. If you proceed with the upgrade now, configuration and/or content needed by these modules will not be available on your new site. For more information, see Review the pre-upgrade analysis in the Upgrading to Drupal 8 handbook

   Maybe s/available on/migrated to/? I still prefer <a href=":migrate">Upgrading to Drupal 8</a> handbook but you can keep it as is if you think it is better. I do not like the suggested change to "these modules will not work": they will still work on the old site, and they are (often? usually?) not installed on the new site, so it does not make sense to say they are not working.

7. It looks as though we have an intermittent failure from the Media module. I already worked on #2934997: Intermittent failure in MediaUiJavascriptTest so I will look into this new one.

OK, let's use "What will be upgraded?" as the page title. I am moving the issue back to NW for that.

The IS currently says, "Add footnotes (or descriptions at the top of the tables) to each table ...". The current patch adds the text we have been discussing as a description for the first table, and I think that is enough. I suggest you update the IS to refer to just the one table. As @xjm pointed out in #14, "less help text is usually better for UX, especially if it doesn't add much information." So let's not add a description to the other table just for symmetry.

The text on /admin/help/migrate_drupal_ui comes from migrate_drupal_ui_help(). That text still refers to "upgrade paths", so it should be updated to be consistent. I guess the remaining task in the IS amounts to copying the text from the handbook (the text that @masipila added as part of this issue) to migrate_drupal_ui_help(). Then you could replace the handbook link with a link to the help page. You can generate the path to that page with

use Drupal\Core\Url;
$path = Url::fromRoute('help.page', ['name' => 'migrate_drupal_ui'])->toString();

We discussed this at the UX meeting on January 16.

It seems that I misunderstood item (2) in the IS: "Provide information on the page or via hook_help instead of links."

The idea is to use hook_help() to provide a help block for this page, not to provide a link to /admin/help/migrate_drupal_ui. As it says on the API page,

By implementing hook_help(), a module can make documentation available to the user for the module as a whole, or for specific pages. ...

The page-specific help information provided by this hook appears in the Help block (provided by the core Help module), if the block is displayed on that page.

That said, my personal opinion is that the current patch already does a good job of "[p]rovid[ing] information on the page". The text we agreed on, and the placement at the head of the "Modules that will not be upgraded" table, gets the point across well. But that is just my opinion.

I suggest that you just update the IS. Update the screenshots there, and copy the blockquote from Comment #35, so that reviewers can get the whole story from the IS. With an updated IS, I think I will be ready to mark this issue RTBC ... and then we will see if the core committers agree with me. (I will also do another review of the re-rolled patch.)

I love this patch. I love any patch that takes weird jargon-ey language and replaces it with something that humans can understand :) Only one thing, then this is totally, madly, passionately RTBC.

+++ b/core/modules/migrate_drupal_ui/src/Form/MigrateUpgradeForm.php
@@ -229,7 +236,7 @@ public function buildForm(array $form, FormStateInterface $form_state) {
+        drupal_set_message($this->t('Unrecognized form step @step', ['@step' => $this->step]), 'error');

drupal_set_message() is, at long last, deprecated :) This should be done with the injected messenger service.

I've fixed all the dsm in the form, because that's how we roll in migrate land.

Thanks, @heddn! All good once Drupal CI passes it.

Couple of minor things, not everything is necessarily in scope but was trying to read all the help text together.

1. +++ b/core/modules/migrate_drupal_ui/migrate_drupal_ui.module
   @@ -26,7 +26,7 @@ function migrate_drupal_ui_help($route_name, RouteMatchInterface $route_match) {
          $output .= '<dd><ol><li>' . t('You need to enter the database credentials of the Drupal site that you want to upgrade. You can also include its files directory in the upgrade.') . '</li>';
   -      $output .= '<li>' . t('The next page then provides an overview of which upgrade paths are available or missing, before you proceed to perform the upgrade.') . '</li>';
   +      $output .= '<li>' . t('The next page then provides an overview of the modules that will be upgraded and those that will not be upgraded, before you proceed to perform the upgrade.') . '</li>';
          $output .= '<li>' . t('Lastly, a message is displayed about the number of upgrade tasks that were successful or failed.') . '</li></ol></dd>';
          $output .= '<dt>' . t('Reviewing the upgrade log') . '</dt>';
   

   Is the 'then' necessary, would think 'The next page provides' would be OK. Also it's out of patch context but would change 'Lastly' to 'Finally'.

2. +++ b/core/modules/migrate_drupal_ui/src/Form/MigrateUpgradeForm.php
   @@ -229,7 +246,7 @@ public function buildForm(array $form, FormStateInterface $form_state) {
    
          default:
   -        drupal_set_message($this->t('Unrecognized form step @step', ['@step' => $step]), 'error');
   +        $this->messenger->addError($this->t('Unrecognized form step @step', ['@step' => $this->step]));
            return [];
   

   How do we end up here? Should it not throw an AccessDeniedHttpException or something? I wouldn't know what to do if I got this error message except try to navigate back to the beginning of the form via URL hacking or something.

I think it's safe to say that the scope of this issue has been completed and that all follow ups have been created. Back to RTBC.

#37 removed the needs usability review and needs follow up tags, but didn't say why - was that intentional? I see follow-ups created in #49 so assume that is ok - so that just leaves the usability review?

1. +++ b/core/modules/migrate_drupal_ui/src/Form/MigrateUpgradeForm.php
   @@ -180,13 +195,14 @@ class MigrateUpgradeForm extends ConfirmFormBase {
   +    $this->messenger = $messenger;
   
   @@ -199,7 +215,8 @@ public static function create(ContainerInterface $container) {
   +      $container->get('messenger')
   

   was there a reason we chose to include this change here, technically out of scope...but not going to block on that alone

2. +++ b/core/modules/migrate_drupal_ui/src/Form/MigrateUpgradeForm.php
   @@ -960,7 +978,12 @@ protected function getDatabaseTypes() {
   +    else {
   ...
   +    }
   

   don't need the else here - the if returns early?

@larowlan:

I removed the "Needs usability review" tag because my comments #28 and #37 included a usability review. @quietone updated the text on the page in response to my suggestions.

I am not sure what the rules are for providing usability reviews. I have been attending the weekly UX meeting for the past few months. Does that make me a member of the UX team?

As I said in #40, we also discussed this issue at the weekly UX meeting.

Thanks @benjifisher, I just wanted to be sure the review had been conducted, it wasn't obvious from the comments, and I clearly missed the first line of comment #40

Are you happy to put it back to RTBC?

The possibly out-of-scope change (#51.1) was requested in #44 and explicitly called non-blocking in #51. The other change (#51.2) has been made. Other than checking that, I have not reviewed the patches since #40, but it was declared RTBC in #46 and #50.

The usability review is done. If that confirmation is the only thing holding the issue back, then I can move it back to RTBC.

Reviewed the changes, the UI improvements look great. Also showed it to others at Drupal Global Sprint Weekend Budapest and it got thumbs-up here too. Looking at the code it is relatively straightforward. I don't feel strong about the messenger service introduction here, IMHO its fine.