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
- Add footnotes (or descriptions at the top of the tables) to each table to explain what is being presented
- Provide information on the page or via hook_help instead of links.
- Replace "paths" and "items" with language such as "these modules will be upgraded" and "these modules will not be upgraded"
- 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)
- Use the error icon for modules that will not be upgraded
The current status of the improvements are
- 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
- The help text has been updated. This does not add a help block for this page. See #40.
- Implemented the third category, incomplete, that uses the warning icon.
- has moved to a followup. #2928147: Migrate UI - add 'incomplete' migration status
- The modules that will not be upgrade are now shown as errors.
Since descriptions have been added, footnotes are not necessary, See #37.
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.
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
Comment | File | Size | Author |
---|---|---|---|
#52 | interdiff.txt | 607 bytes | quietone |
#52 | 2922701-52.patch | 14.23 KB | quietone |
#48 | Selection_002.png | 11.62 KB | quietone |
#48 | interdiff.txt | 1.66 KB | quietone |
#48 | 2922701-48.patch | 14.25 KB | quietone |
Comments
Comment #2
quietone CreditAttribution: quietone as a volunteer commentedOf the 5 suggestions in the IS this does #3 and #5.
And a small screenshot
Comment #3
quietone CreditAttribution: quietone as a volunteer commentedRemove parent issue.
Comment #5
quietone CreditAttribution: quietone as a volunteer commentedAdded 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.
Comment #7
quietone CreditAttribution: quietone as a volunteer commentedAlso needed to changed the xpath search string for the tests.
Comment #8
heddnUnused import.
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.
Comment #9
masipila CreditAttribution: masipila as a volunteer commentedRe #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.
Comment #10
masipila CreditAttribution: masipila as a volunteer commentedThe 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.
Comment #11
masipila CreditAttribution: masipila as a volunteer commentedAddressin 8.1 and 8.2. I also renamed the handbook page to 'Upgrade using web browser'.
Comment #12
heddnCan we update the title here? And/or can we link to a #target on the page that specifically addresses modules not getting upgraded.
Comment #13
masipila CreditAttribution: masipila as a volunteer commentedI 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
Comment #14
xjmThis is looking pretty good; thanks for your work on this!
This looks out of scope.
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.)
Similarly, I think this text is unneeded and should be removed.
"Pre-upgrade" should be hyphenated.
Comment #15
xjm@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. :)
Comment #16
quietone CreditAttribution: quietone as a volunteer commentedThis should address #14-1,2,3
Comment #17
quietone CreditAttribution: quietone as a volunteer commentedThe follow up, item #4 in the IS, has been created. #2928147: Migrate UI - add 'incomplete' migration status
Comment #18
heddnAll feedback is now addressed. Looks good.
Comment #20
quietone CreditAttribution: quietone as a volunteer commentedComment #21
quietone CreditAttribution: quietone as a volunteer commentedSimple reroll
Comment #22
phenaproximaOnly found a couple of minor things...
This doesn't appear to be used anywhere?
Should the strings have the @count placeholder in them? For example, "@count modules will not be upgraded"?
Same here.
I feel like this needs screenshots and possibly usability review, so I'm leaving it in review for now...
Comment #23
quietone CreditAttribution: quietone as a volunteer commented1. Fixed
2. According to the PluralTranslatableMarkup that is not needed.
Comment #24
phenaproximaAlrighty then. RTBC once Drupal CI passes it.
Comment #25
quietone CreditAttribution: quietone as a volunteer commented@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.
Comment #26
quietone CreditAttribution: quietone as a volunteer commentedTagging
Comment #27
quietone CreditAttribution: quietone as a volunteer commentedComment #28
benjifisherThis 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?#14.1 asked for the removal of an out-of-scope indentation fix. Unless I am missing something, this is introducing an indentation error.
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!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:
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"!)
Comment #29
benjifisherMaybe d.o is the one being too aggressive about hiding files. If I hide the interdiff, will the latest patch show?
Comment #30
quietone CreditAttribution: quietone as a volunteer commentedNeeded a reroll.
Comment #32
quietone CreditAttribution: quietone as a volunteer commentedClearly was not paying attention during that reroll.
Comment #33
quietone CreditAttribution: quietone as a volunteer commented1. 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.
Comment #35
benjifisherIf you do not like any of these, you can keep it the way it is.
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.Comment #36
quietone CreditAttribution: quietone as a volunteer commented1. 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?
Setting to NR for the question above. Once that is done, I'll update the IS etc
Comment #37
benjifisherOK, 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 frommigrate_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) tomigrate_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 withComment #38
quietone CreditAttribution: quietone as a volunteer commentedChanged 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:
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.
Comment #40
benjifisherWe 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,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.)
Comment #41
quietone CreditAttribution: quietone as a volunteer commented@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.
Comment #43
quietone CreditAttribution: quietone as a volunteer commentedSomehow missed changes to the test files.
Comment #44
phenaproximaI 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.
drupal_set_message() is, at long last, deprecated :) This should be done with the injected messenger service.
Comment #45
heddnI've fixed all the dsm in the form, because that's how we roll in migrate land.
Comment #46
phenaproximaThanks, @heddn! All good once Drupal CI passes it.
Comment #47
catchCouple of minor things, not everything is necessarily in scope but was trying to read all the help text together.
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'.
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.
Comment #48
quietone CreditAttribution: quietone as a volunteer commentedI 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.
Comment #49
quietone CreditAttribution: quietone as a volunteer commentedNew issues made for points in #47
1. #2939328: Migrate UI - review help text
2. #2939329: Improve error handling if form step is invalid
Comment #50
maxocub CreditAttribution: maxocub as a volunteer commentedI 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.
Comment #51
larowlan#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?
was there a reason we chose to include this change here, technically out of scope...but not going to block on that alone
don't need the else here - the if returns early?
Comment #52
quietone CreditAttribution: quietone as a volunteer commented@benjifisher, Can you answer the question about the usability review tag?
1. I think phenaproxima summed it up nicely in #44
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
Comment #53
benjifisher@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.
Comment #54
larowlanThanks @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?
Comment #55
benjifisherThe 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.
Comment #57
Gábor HojtsyReviewed 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.
Comment #58
Gábor Hojtsy