Problem/Motivation

While taking a look at the current merge request for #3284936: Improve messaging to users to backup database I've noticed that the error message for the readiness checks lists the failed checks indented with bullet points:
error message for failed readiness checks
that way it is fairly easy for the user to distinguish the different calls to action.
In contrast the warning message on the ready to update page doesn't have any ident or bullet points at all for the modules in question. In case only skimming the user might get under the impression the word belongs to the sentence before due to a line wrap. the user has to read more carefully.

warning message on the ready to update page stating CKeditor needs a database update

If the styling would be like the readiness checks the user would only have to skim " Possible database updates"...."following extension" and then jump to the bullet point list or read everything if it is the first update with a required database update.

Proposed resolution

- prefix each module in the list with indent and a bullet point

CommentFileSizeAuthor
#13 composer_patches.png149.62 KBrkoller
ready_to_update_warning.png216.48 KBrkoller
readiness_error.png121.8 KBrkoller

Issue fork automatic_updates-3303113

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

About issue forks

Comments

rkoller created an issue. See original summary.

phenaproxima made their first commit to this issue’s fork.

phenaproxima’s picture

phenaproxima
he/him
Primary language English
Location Massachusetts
commented

Yup, we can do this. Here's one way:

Right now, in UpdateReady::buildForm(), we have this code:

    if ($pending_updates) {
      $messages[MessengerInterface::TYPE_WARNING][] = $this->t('Possible database updates were detected in the following extensions; you may be redirected to the database update page in order to complete the update process.');
      foreach ($pending_updates as $pending_update) {
        $messages[MessengerInterface::TYPE_WARNING][] = $pending_update;
      }
    }

We're setting each pending update as its own message. Instead, we should render them as an item list. The item list's #prefix should be the existing warning text ("Possible database updates were detected...") and the list's #items should be the names of the extensions with updates (i.e., '#items' => $pending_updates).

We'll need to inject the renderer service into the form and render the item list (use renderRoot()), then add that rendered list to $messages. We won't have to worry about backwards compatibility when injecting the renderer, since the form is both final and explicitly internal.

I think that would have the desired result, and we could (and should) easily adjust our test coverage to make sure we're rendering the expected HTML.

rahul_’s picture

rahul_ commented
Assigned: Unassigned » rahul_
rkoller’s picture

rkoller
he/him
Primary language German
Location Nürnberg, Germany
commented

I had a quick discussion with @phenaproxima on Slack yesterday and we agreed that i forward the information into this issue even though it might be out of scope but at least as reference for a potential follow up issue. Initially i've asked three questions:

Q 1: are there any cases if a database update is needed that the user isn’t forwarded to the database update page?
A 1: :thinking_face: I don’t think so…the validator is detecting potential changes in the staged files, so it will forward you. If nothing is needed, the update page will just say nothing is needed. (I think.)

Q 2: what are “extensions”?
A 2: Blanket term for “modules and themes”. We could maybe rephrase it to “modules or themes”, since we cannot tell, at that stage, which ones are which.

Q 3: ...and might there be an item in the list returned that has to be fixed outside the database update page manually?
A 3: Extremely unlikely. I never heard of something like that.

Based on his answer i came up with the following feedback:

i wonder if the sentence should be rephrased then?
remove you may be redirected, remove the divide by the semicolon and create shorter sentences?
and one nitpick currently there is the reference to the list of modules in question in the first half of the sentence then in the second half the explanation what will happen after (so there is a disconnect between the sentence referring to the list and the list itself). if there are three shorter sentences it might be easier to digest. something like in the first sentence just state that database updates are necessary. in the second sentence that the user will be redirected to the database update page. and in the third sentence the reference to the list of modules and themes.

Necessary database updates were detected. In the next step you will be redirected to the database update page in order to complete the update process. The following themes and modules need updates:

rahul_ opened merge request !426

rahul_’s picture

rahul_ commented
Status: Active » Needs review

I updated code and created MR.

phenaproxima’s picture

phenaproxima
he/him
Primary language English
Location Massachusetts
commented
Status: Needs review » Needs work
Issue tags: +Needs tests

This is a great start, but there are a few things I don't understand, and a couple of things we could adjust.

Also, we'll need automated test coverage. We have existing coverage of this message in UpdaterFormTest, but we'll want to modify it so that we're also asserting that the listed extensions are actually shown as items in an unordered list.

rkoller’s picture

rkoller
he/him
Primary language German
Location Nürnberg, Germany
commented

while i was typing my reply @phenaproxima already posted feedback. only from my end one additional observation reposted. I've tried to apply the patch to the latest dev version of automatic updates on a drupal 9.4.2 install. but unfortunately composer-patches was unable to apply the patch.

rahul_’s picture

rahul_ commented
Status: Needs work » Needs review

I fixed the MR improvements suggestions, could you please re-review.
Thanks.

rkoller’s picture

rkoller
he/him
Primary language German
Location Nürnberg, Germany
commented
Status: Needs review » Needs work

hmmm i had set the diff file to patches-ignore for the time @rahul_ was working on the changes. i've moved it back to the patches section in my projects composer.json. but somehow on composer update drupal/automatic_updates -w the patch still fails to apply :/ setting it back to needs work.

phenaproxima’s picture

phenaproxima
he/him
Primary language English
Location Massachusetts
commented
Status: Needs work » Needs review

@rkoller, are you applying the patch against a tagged release of Automatic Updates, or against HEAD of 8.x-2.x? I suspect that only the latter will work since this module is changing so rapidly.

composer require drupal/automatic_updates:2.x-dev

rkoller’s picture

rkoller
he/him
Primary language German
Location Nürnberg, Germany
commented
StatusFileSize
new composer_patches.png149.62 KB

when applying patches i only use the dev release. i've even set the other automatic updates patch to patches-ignoreto iron out the possibility that the two might overlap. but still the patch from this issue fails to apply.
failed to apply the patch in composer patches in the terminal

phenaproxima’s picture

phenaproxima
he/him
Primary language English
Location Massachusetts
commented
Assigned: rahul_ » phenaproxima
Status: Needs review » Needs work

Code looks great. Nice work, @rahul_.

One more small suggestion, and then all this needs is test coverage. I'll take those on, since the test changes are not very obvious.

phenaproxima’s picture

phenaproxima
he/him
Primary language English
Location Massachusetts
commented
Assigned: phenaproxima » tedbow
Status: Needs work » Needs review
Issue tags: -Needs tests

Added test coverage.

I think that, once @tedbow thinks this looks good, we should also transfer it into Automatic Updates' UpdateReady form (and the corresponding test) before RTBC.

tedbow’s picture

tedbow
he/him
Primary language English
Location Ithaca, NY, USA
commented
Status: Needs review » Reviewed & tested by the community
phenaproxima’s picture

phenaproxima
he/him
Primary language English
Location Massachusetts
commented

  • phenaproxima committed 9e2c8f7 on 8.x-2.x authored by rahul_ 
    Issue #3303113 by phenaproxima, rahul_, rkoller, tedbow: Add bullet...
phenaproxima’s picture

phenaproxima
he/him
Primary language English
Location Massachusetts
commented
Status: Reviewed & tested by the community » Fixed

Finally! Nice work everyone. Merged into 8.x-2.x.

Status: Fixed » Closed (fixed)

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