Migrate UI: Improve 'Review Upgrade' page UX

Looks like there are elements from the status report page we could reuse here. The summary items would then link to their respective tables:

See #665790: Redesign the status report page

This is a blocker for #2905491: Mark Migrate Drupal UI as stable

@jjpoole, thanks for improving the page!

One point to consider is that it was a deliberate decision to put the missing modules at the top of the list. For someone using the UI to do an upgrade, seeing what should be installed needs to be prominent. The list of available modules can be quite long and the missing module list could be off the screen.

Reviewed this in our weekly migrate meeting and agreed this is in fact a migrate critical.

• For convenience, I think each module should have a clickable link to the project page on drupal.org.
• I think we should more explicitly warn that a site with out of date modules is unlikely to function properly.
• I think we should clarify if partial upgrades can be rerun in future if missing modules become available (and what happens to data then).
• I think it's usually a good idea to link to the page that explains the difference between Update and Upgrade.
• If the screenshot is real and not just a mockup, then Drupal in the second table column is spelt wrong.
• Also, I think it looks OK :)

re #8:

Thanks for your invaluable feedback.
1. These are core modules so linking to a module page isn't really possible.
2. This is done on the initial page of the upgrade wizard. See #2905222: Migrate UI: Initial Screen UX improvements
3. Ditto to #2
4. Ditto to #2
5. Can you check #2905222: Migrate UI: Initial Screen UX improvements and see if we cover that well enough?
6. Glad you like the mock-up!

Also updated the IS to include current and proposed screenshots.

Tagging for sprinting in Vienna.

Would be awesome if we can get help from @ckrina in here!

And can't wait to start working on this in Vienna :)

Per discussion with @catch, we're promoting the "Migrate critical" issues to critical given the importance of a stable migration path at this point in the release cycle. (The framework and release managers will confirm this in a later triage for individual issues.)

It sounds like the next step is to implement a combination of #3 and the proposal in the summary.

From Bojhan: #2859304-41: Show field type migrations correctly in Migrate Drupal UI

Can we "group them" and just say 27 missing modules, and then the user can opt into looking at them?

Looking at the status page, and the proposed design for this page, would be good if we keep a little bit of consistency and reuse the same icons we already have.

A more concrete mockup:

Still wondering about:
- are missing upgrade paths errors or warnings?
- collapse the available paths by default?

Other attention points:

- The "Are you sure?" page title is not ideal :)
- Do we need the explanation text per section (missing/available) or can we combine that in a single bit of help at the top of the page. I think the latter could be enough.

After a quick discussion at Drupalcon Vienna:

- We consider missing upgrade paths warnings, not errors because you may not want to upgrade those things. It's "info", not "wrong".
- There is no explanation text in the "available" section so we can manage with regular help text on top of the page
- I'm fine with keeping the "available" section collapsed as it is now.

Discussed the UI changes with yoroy and laurii at DrupalCon Vienna last week and I will be taking on this issue.

We need to move those critical issues! Since nothing has been done here yet, I'll start working on it.

@starshaped: If you have time in the future, fell free to help or review.

Ok, here's a first patch.

I tried to use the CSS and templates of the status report page, wherever possible, but maybe we will want our own.

One thing I don't know yet how to do is to open the collapsed available paths when we click on "Details".

Screenshots:

This patch looks pretty magnificent at first glance. I only have minor concerns, then TBH I think we can send this directly to UX review.

1. +++ b/core/modules/migrate_drupal_ui/migrate_drupal_ui.libraries.yml
   @@ -0,0 +1,6 @@
   +    # Adjust the weights to load these early.
   

   Why?

2. +++ b/core/modules/migrate_drupal_ui/src/Form/MigrateUpgradeForm.php
   @@ -503,24 +507,30 @@ public function buildConfirmForm(array $form, FormStateInterface $form_state) {
   +          'source_module' => [
   +            '#plain_text' => $source_module,
   +            '#prefix' => '<span class="upgrade-analysis-report__status-icon upgrade-analysis-report__status-icon--warning">',
   +            '#suffix' => '</span>',
   +          ],
   

   Do we still have the html_tag renderable element? If so, can we use that (so as to make the tag, and the classes attached to it, overrideable)?

Re #21:

1. Oups! That's a copy/paste left over from system.libraries.yml
2. Done

Also in this patch:

• Fixed the coding standard warnings
• Fixed the anchor link inside the collapsed Available paths

Here's another improvement: we don't display an empty table if there's no missing or available paths.

IS update.

+++ b/core/modules/migrate_drupal_ui/src/Form/MigrateUpgradeForm.php
@@ -599,7 +654,7 @@ protected function getDatabaseTypes() {
-    return $this->t('Are you sure?');
+    return $this->t('Upgrade analysis report');

Glad to see this is changed.

Applied the patch and checked the page using the test fixtures. The new layout is a huge improvement and all the links work as expected. Also, set the missing count to 0 and get the same result as in #23. Did a brief review of the code and didn't see anything, but I don't know enough about css to review that and thus don't think I can RTBC this.

@maxcub, Thank you!

I’m out of town until tomorrow, but I can look over the CSS when I get home.

The CSS looks OK to me, but I am not an expert so I don't feel qualified to RTBC it. However, everything else is great. Fantastic work, @maxocub.

@starshaped, feel free to mark this issue RTBC if the CSS passes muster. :)

Just checked the CSS in the patch #23 and looks fine to me.

Great work @maxocub.

Thanks for the review, @vulcanr! Everything seems to check out here, so let us sally forth to RTBC.

That is a random fail.

I think the commit that was causing these fails has been reverted: #2843765-18: EntityResource: Provide comprehensive test coverage for EntityViewDisplay entity.
Back to RTBC.

Really great work on this. This is a huge improvement.

I waffled a little as to whether "Upgrade analysis report" was the best title for the page, but couldn't come up with something that was clearly better, and the rest of this is such a nice improvement that I think it's worth just getting it in.

I think we also could do a little more to clarify that a missing upgrade path isn't necessarily a problem (e.g. Toolbar really doesn't need one). Currently it links to the handbook page, but we could explicitly state something like: "Not all modules require an upgrade path." But I think that's out of scope here and could be addressed in a followup.

Also, @wturrell's suggestion that the modules should link the relevant modules' pages is actually not unreasonable, since every module must have a page linked in its hook_help() (we even have a test for this). So I'd suggest filing a followup for that as well.

Committed to 8.5.x. Since Migrate Drupal UI is still alpha, I also cherry-picked it to 8.4.x. Thanks everyone!

One critical less, yeah!

Here are the followups:

• #2918448: Migrate UI: Clarify that a missing upgrade path isn't necessarily a problem
• #2918450: Migrate UI: Modules should link the relevant modules' pages

Thanks @maxocub.

We already had #2914974: Migrate UI - handle sources that do not need an upgrade to address the missing paths. It could be dropped from critical though, if we decide it doesn't block a stable release.