f site is in maintenance mode omit instruction to put site in maintenance mode in DbUpdateController.php suggestion: i

This seems reasonable so I updated the patch and added screenshots to the IS.

This should be better

I was going to say this needs test but was easier to add it.

Switch to system.module because it is not in update.module(why I found this issue)

Easy to test.

Applied MR 4524
Put site into maintenance mode
Verified line wasn't in update.php
Took out out of maintenance mdoe
Verified line was there

I'm triaging RTBC issues.

I looked at this late last night and after reading the IS and comments I don't see anything left to do. And thanks to @tedbow for adding a test. I ran the test locally without the fix and does indeed fail.

1) Drupal\Tests\system\Functional\UpdateSystem\UpdateScriptTest::testSuccessfulUpdateFunctionality with data set #0 (true)
Behat\Mink\Exception\ResponseTextException: The text "Put your site into maintenance mode" appears in the text of this page, but it should not.

/var/www/html/vendor/behat/mink/src/WebAssert.php:811
/var/www/html/vendor/behat/mink/src/WebAssert.php:279
/var/www/html/core/modules/system/tests/src/Functional/UpdateSystem/UpdateScriptTest.php:867
/var/www/html/core/modules/system/tests/src/Functional/UpdateSystem/UpdateScriptTest.php:701
/var/www/html/vendor/phpunit/phpunit/src/Framework/TestResult.php:728

Leaving at RTBC.

I'm not sure I agree we should be omitting the text about maintenance mode from the list when maintenance mode is enabled. To me, this page looks like a static list that describes the recommended steps for updating Drupal. I would not expect it to change depending on what steps I've taken before.

I'm triaging RTBC issues → .

Reading the previous comment made me look at the IS again. The first image is now out of date and I am going to remove it. The screenshots to look at are in the "User interface changes" section.

Regarding #20, would not it be better from the usability point of view to have that point checked, or highlighted some other way to be clear, that the point is done? I also understand this text as a static guide, so I would not expect some points missing from it.

For me, #46 and #48 imply that this is a won't fix.

I will ask in #usability for guidance.

So should this be closed?

Lets get an opinion for a usability review to resolve this.

Usability review

We discussed this issue at 📌 Drupal Usability Meeting 2023-09-15 Fixed . The direct link to the recording of the meeting is: https://youtu.be/2bqTj97QiJ8?si=6-TmlJHwXvMVVtWJ&t=1260

For the record, the attendees at the usability meeting were @AaronMcHale, @benjifisher, @rkoller, @shaal, and @worldlinemine.

If you want more feedback from the usability team, a good way to reach out is in the #ux channel in Slack.

At first apologies for the belated comment. @quietone brought up the issue on the #ux channel last Friday. That issue looked quite familiar, so I did some digging in past issues before the meeting and it turned out we've already discussed it in September last year, where @quietone brought up the issue for the first time, but somehow we've failed to comment on the issue after the meeting. :( I've spent the weekend to watch the recording and re-familiarize with the issue. In the following a summary of the conclusions from back then.

There was a clear consensus among the attendees that simply hiding step 3 in case the requirement of an active maintenance mode is met is probably not the right thing to do here. Due to the fact you are having a numbered (ordered) list it might be a source for potential confusion if in one scenario (maintenance mode active) that list has three steps while in the other scenario (maintenance mode inactive) that very same list has four steps instead. That might be potentially problematic in the context of for example support questions referencing step 3 or 4.
The consensus what approach to take was in line with the suggestion made in #48 ✨ If site is in maintenance omit instruction to put site in maintenance mode in DbUpdateController.php Needs review , instead of hiding step 3 add a simple green checkmark or text illustrating that the requirement is met. On the other hand simply closing the issue as suggested in #50 ✨ If site is in maintenance omit instruction to put site in maintenance mode in DbUpdateController.php Needs review would be problematic as well, without an indication like the suggested checkmark and having step 3 still in leaves the uncertainty for people with a small working memory if the maintenance mode was really already set or not.
We've also discussed adopting the pattern used in the Verify requirements step in the installe. But the problem as noted in #46 ✨ If site is in maintenance omit instruction to put site in maintenance mode in DbUpdateController.php Needs review is, that the only requirement that is possible to be automatically tested in that list is step 3, the other steps are just recommended calls to action. Therefore doing anything more than just adding a checkmark would be probably not worth the effort.

Aside that it might be probably a good idea to rework the micro copy of the Drupal database update page in general in a follow up issue. For example it isn't clear at all what does Update your files (as described in the handbook page linked above). in step 4 actually refer to?
The upgrading handbook link it points to doesn't contain any clues in regards of "Update your files". If you search for update file or update your files you have no match - the linked https://www.drupal.org/updating-and-upgrading-drupal-core → was last updated on the 5th of December 2017. It might make sense to review if that page might need an update as well. At least as a resource in the context of the provided link on the Drupal database update it is not that helpful at the moment.
On a side note, directional instructions on the Drupal database update page like "steps above" pointing to the handbook page should be avoided. It is not advised to utilize any directional instructions/language that requires the reader to see the layout or design of the page (for example https://styleguide.mailchimp.com/writing-for-accessibility/).
But all of those points are out of the scope for this issue and should better be tackled in follow-up issue(s).

I'll remove the Needs usability review tag and change the status back to Needs work

@rkoller, thanks for responding so quickly!

I have updated the remaining tasks to indicate a check mark is to be added. And I made the followup for the item in the last paragraph, 📌 Improve text on database update page Active

Hiding patches as we have an MR. Added steps to reproduce. Updated title to better align with usability review.