Problem/Motivation

This is a followup to #2914974: Migrate UI - handle sources that do not need an upgrade to work on improvements to the UI text It started from a migrate meeting where it was decided to have 3 categories of module status on the page as well as improvements to the help text. See #25 for a full summary of that discussion. That was followed by more suggestions, about the UI text, in the next 2 comments. Here is a summary of those suggestions

The suggested improvements are

  1. Add footnotes (or descriptions at the top of the tables) to each table to explain what is being presented
  2. Provide information on the page or via hook_help instead of links.
  3. Replace "paths" and "items" with language such as "these modules will be upgraded" and "these modules will not be upgraded"
  4. Allow for a third category, Incomplete, to identify modules where some of the necessary migrations are working. And use the warning icons for these modules (This will be implemented in a follow up)
  5. Use the error icon for modules that will not be upgraded

The current status of the improvements are

  1. A description has been added for the table about the modules that will not be upgraded. The text currently used is from #35

    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.

    A description is not needed for the second table, it is self explanatory. See #14

  2. Since descriptions have been added, footnotes are not necessary, See #37.

  3. The help text has been updated. This does not add a help block for this page. See #40.
  4. The next page provides an overview of the modules that will be upgraded and those that will not be upgraded, before you proceed to perform the upgrade.

  5. Implemented the third category, incomplete, that uses the warning icon.
  6. has moved to a followup. #2928147: Migrate UI - add 'incomplete' migration status
  7. The modules that will not be upgrade are now shown as errors.

Before screenshots:

Proposed resolution

Implement the above suggestions.

After screenshot:

Help text, only item #2 is modified in this patch:

Remaining tasks

Review
Commit

User interface changes

Yes, to the Migrate Drupal UI.

API changes

N/A

Data model changes

N/A

CommentFileSizeAuthor
#52 interdiff.txt607 bytesquietone
#52 2922701-52.patch14.23 KBquietone
#48 Selection_002.png11.62 KBquietone
#48 interdiff.txt1.66 KBquietone
#48 2922701-48.patch14.25 KBquietone
#45 interdiff_43-45.txt4.21 KBheddn
#45 2922701-45.patch14.23 KBheddn
#43 interdiff.txt2.44 KBquietone
#43 2922701-43.patch10.76 KBquietone
#41 Selection_006.png22.94 KBquietone
#41 Selection_004.png30.46 KBquietone
#41 2922701-41.patch7.95 KBquietone
#38 Whatwillbeupgraded.png26.62 KBquietone
#38 help_text.png22.11 KBquietone
#38 interdiff.txt4.28 KBquietone
#38 2922701-38.patch9.89 KBquietone
#33 Selection_022.png13.58 KBquietone
#33 interdiff.txt1.28 KBquietone
#33 2922701-33.patch7.05 KBquietone
#32 interdiff.txt1.71 KBquietone
#32 2922701-32.patch6.79 KBquietone
#30 interdiff.txt2.74 KBquietone
#30 2922701-30.patch8.2 KBquietone
#23 After-bottom.png21.52 KBquietone
#23 After-top.png23.65 KBquietone
#23 Before-bottom.png21.31 KBquietone
#23 Before-top.png23.75 KBquietone
#23 interdiff.txt599 bytesquietone
#23 2922701-23.patch7.4 KBquietone
#21 2922701-21.patch7.71 KBquietone
#16 interdiff.txt3.44 KBquietone
#16 2922701-16.patch7.58 KBquietone
#13 interdiff-2922701-11-13.txt1.1 KBmasipila
#13 2922701-13.patch8.53 KBmasipila
#11 interdiff-2922701-7-11.txt1.3 KBmasipila
#11 2922701-11.patch8.5 KBmasipila
#7 interdiff.txt1.28 KBquietone
#7 2922701-7.patch8.74 KBquietone
#5 Selection_003.png8.75 KBquietone
#5 Selection_002.png26.6 KBquietone
#5 interdiff.txt7.83 KBquietone
#5 2922701-5.patch7.83 KBquietone
#2 Selection_001.png28.65 KBquietone
#2 2922701-2.patch6.82 KBquietone
Members fund testing for the Drupal project. Drupal Association Learn more

Comments

quietone created an issue. See original summary.

quietone’s picture

  • Removed references to paths, items , upgrade module and source module.
  • Use the error icons for modules that will not be upgraded.
  • Added a explanatory sentence at the start of the page (I recall yoroy suggesting that in other issues).
  • And I took the liberty of changing the title to 'Pre upgrade analysis' because 'Upgrade analysis report' sounded like a report that one would get after the upgrade, not before. Although, we probably should consider this title in conjunction with the title used by the audit report.
  • I added an explanatory sentence at the beginning of the 'modules that will be upgrade table' but it was mostly a repeat of the title and it was just clutter on the screen for me. That is, it didn't add any useful information. Any suggestions?

Of the 5 suggestions in the IS this does #3 and #5.

And a small screenshot

quietone’s picture

Status: Needs review » Needs work

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

quietone’s picture

Status: Needs work » Needs review
FileSize
7.83 KB
7.83 KB
26.6 KB
8.75 KB

Added some more text so this now covers 1, 2, 3 and 5 of the IS. Number 4 is about a followup, so if tests pass this will be ready for review.

Status: Needs review » Needs work

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

quietone’s picture

Status: Needs work » Needs review
FileSize
8.74 KB
1.28 KB

Also needed to changed the xpath search string for the tests.

heddn’s picture

Status: Needs review » Needs work
  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.

masipila’s picture

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.

masipila’s picture

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.

masipila’s picture

Status: Needs work » Needs review
FileSize
8.5 KB
1.3 KB

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

heddn’s picture

Status: Needs review » Needs work
+++ 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.

masipila’s picture

Status: Needs work » Needs review
FileSize
8.53 KB
1.1 KB

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

xjm’s picture

Status: Needs review » Needs work

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.

xjm’s picture

Priority: Critical » Major

@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. :)

quietone’s picture

Status: Needs work » Needs review
FileSize
7.58 KB
3.44 KB

This should address #14-1,2,3

quietone’s picture

Issue tags: -Needs followup

The follow up, item #4 in the IS, has been created. #2928147: Migrate UI - add 'incomplete' migration status

heddn’s picture

Status: Needs review » Reviewed & tested by the community
Issue tags: +Migrate January 2018 Sprint

All feedback is now addressed. Looks good.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 16: 2922701-16.patch, failed testing. View results

quietone’s picture

Assigned: Unassigned » quietone
quietone’s picture

Status: Needs work » Needs review
FileSize
7.71 KB

Simple reroll

phenaproxima’s picture

Issue tags: +Needs screenshots

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...

quietone’s picture

Issue summary: View changes
Issue tags: -Needs screenshots
FileSize
7.4 KB
599 bytes
23.75 KB
21.31 KB
23.65 KB
21.52 KB

1. Fixed
2. According to the PluralTranslatableMarkup that is not needed.

phenaproxima’s picture

Status: Needs review » Reviewed & tested by the community

Alrighty then. RTBC once Drupal CI passes it.

quietone’s picture

Status: Reviewed & tested by the community » Needs review

@phenaproxima, no usability review as as you mentioned in #22.

What of the two items in the Remaining Tasks list?

Setting to NR to these questions.

quietone’s picture

Issue tags: +Needs usability review

Tagging

quietone’s picture

Assigned: quietone » Unassigned
benjifisher’s picture

  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.

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

benjifisher’s picture

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

quietone’s picture

Status: Needs work » Needs review
FileSize
8.2 KB
2.74 KB

Needed a reroll.

Status: Needs review » Needs work

The last submitted patch, 30: 2922701-30.patch, failed testing. View results

quietone’s picture

Status: Needs work » Needs review
FileSize
6.79 KB
1.71 KB

Clearly was not paying attention during that reroll.

quietone’s picture

1. That is strange, glad you found the patch.

2. This change is point #5 in the IS, why is that out of scope? Changing to 'error' instead of 'warning' was suggested, by me, in #2914974: Migrate UI - handle sources that do not need an upgrade comment #26 and supports another migrate decision, which is to have 2 categories of modules listed on the page. The change to using 'error' was agreed to by jhoddon and phenaproxima in the following two comments of the original issue.

A followup has been made to change the paths to the svg files as suggested, #2936399: Migrate UI - change path to svg files

3. This was accidentally fixed in the reroll.

4. Thank you.

5. I look forward to your suggestion for the form title.

6. The suggested text with some modification has been added with screen shot.
s/Drupal 8 site/your new site/ - For consistency with the initial page displayed and the conflict id form.
s/will be lost/needed by these modules will not be available on your new site/ - Because the data is not lost, it will remain in the source db. As I write this I think the text should be to the point, "If you proceed with the upgrade now these modules will not work". Comments?

screenshot of the text added.

Status: Needs review » Needs work

The last submitted patch, 33: 2922701-33.patch, failed testing. View results

benjifisher’s picture

  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.
quietone’s picture

Status: Needs work » Needs review

1. Interesting.
2. Ah, that is clear now. Credit due to heddn for me marking it novice, he often reminds us to tag issues novice.
3,4. Done
5. I'd go with the simple, "What will be upgraded?".
6. 'Available on' is preferred, migrate is not used for the UI, it is all 'upgrade'. For someone using the UI they don't need to know that it is a migration under the hood, just that their site is 'upgraded' to Drupal 8.
7. Thanks

What is your opinion on the 2 remaining tasks is the IS?

  1. Add footnotes - Are footnotes needed? What should they say?
  2. Provide information on the page or via hook_help instead of links - What needs to be added for this.

Setting to NR for the question above. Once that is done, I'll update the IS etc

benjifisher’s picture

Status: Needs review » Needs work
Issue tags: -Needs usability review, -Needs followup

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();
quietone’s picture

Issue summary: View changes
Status: Needs work » Needs review
FileSize
9.89 KB
4.28 KB
22.11 KB
26.62 KB

Changed the title of the modules not to be/to be upgraded "What will be upgraded?". On testing the title of the id conflicts page was changed as well, which is not a change for this issue. To prevent that I added an if statement in getQuestion to select the desired question based on the form step.

The help text text has been updated as well.

That just leaves one item to fix:

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

Tried to do this but can't figure out what text on the handbook page should be moved to help. I presume this is because I don't know what information should be on a help page and what should be in the handbook.

Version: 8.5.x-dev » 8.6.x-dev

Drupal 8.5.0-alpha1 will be released the week of January 17, 2018, which means new developments and disruptive changes should now be targeted against the 8.6.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

benjifisher’s picture

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.)

quietone’s picture

@benjifisher, thanks for your reviews! I too think the current patch gives enough information so the user can decide to proceed or not with the upgrade. While not opposed to more help, like the help bock, if that is wanted it should be done in a follow up. There are more changes for this form to do and an there is already an issue to split it up.

The IS has been updated, including new screenshots, as suggested in #40. The patch rerolled.

Status: Needs review » Needs work

The last submitted patch, 41: 2922701-41.patch, failed testing. View results

quietone’s picture

Status: Needs work » Needs review
FileSize
10.76 KB
2.44 KB

Somehow missed changes to the test files.

phenaproxima’s picture

Status: Needs review » Needs work

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.

heddn’s picture

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

phenaproxima’s picture

Status: Needs review » Reviewed & tested by the community

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

catch’s picture

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.

quietone’s picture

Issue summary: View changes
Status: Reviewed & tested by the community » Needs review
FileSize
14.25 KB
1.66 KB
11.62 KB

I am not sure of the status of this issue, I am assuming Needs Work.

1. Removed the word 'then'. IS updated to include the help text as text and in a screenshot. I'll make a new issue to review the help text in entirety.
2. Yes, this should be handled better, I' make an issue for this too.

quietone’s picture

maxocub’s picture

Status: Needs review » Reviewed & tested by the community

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.

larowlan’s picture

Status: Reviewed & tested by the community » Needs review

#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?

quietone’s picture

@benjifisher, Can you answer the question about the usability review tag?

1. I think phenaproxima summed it up nicely in #44

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

And since it is still unclear to me when it is OK to remove deprecated code and when to leave it alone, I went with his suggestion.
2. Fixed

benjifisher’s picture

@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.

larowlan’s picture

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?

benjifisher’s picture

Status: Needs review » Reviewed & tested by the community

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.

  • Gábor Hojtsy committed a01e02d on 8.6.x
    Issue #2922701 by quietone, masipila, heddn, benjifisher, phenaproxima,...
Gábor Hojtsy’s picture

Status: Reviewed & tested by the community » Fixed

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.

Gábor Hojtsy’s picture

Version: 8.6.x-dev » 8.5.x-dev

  • Gábor Hojtsy committed 4d13415 on 8.5.x
    Issue #2922701 by quietone, masipila, heddn, benjifisher, phenaproxima,...

Status: Fixed » Closed (fixed)

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