Problem/Motivation

Follow-up to #3339526: Automatic Updates hosting test result on Dreamhost


This image here I think is trying to show a message from \Drupal\package_manager\Validator\StagedDBUpdateValidator::checkForStagedDatabaseUpdates

but it does not show the message "Possible database updates have been detected in the following extensions." only the module that has the update. I think this is because \Drupal\automatic_updates\Form\UpdateFormBase::displayResults assumes if there is only 1 messages(the module name) then you don't need to show the summary(in this case the "Possible database updates have.." message). This is not correct.

Steps to reproduce

Proposed resolution

  1. \Drupal\automatic_updates\Form\UpdateFormBase::displayResults we should on always use the theme unless unless there is only 1 message and there is not summary.

    In \Drupal\package_manager\ValidationResult::__construct we throw an error if there are more than 1 message and there is not a summary but you can create a result with only 1 message and still have a summary.

  2. Look at the other places will display results and determine if this is problem.

    at least \Drupal\automatic_updates\Validation\StatusCheckRequirements::getRequirements and \Drupal\automatic_updates\Validation\ValidationResultDisplayTrait::displayResults

Remaining tasks

User interface changes

API changes

Data model changes

CommentFileSizeAuthor
#14 3339657-13.png95.1 KBphenaproxima

Issue fork automatic_updates-3339657

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

tedbow created an issue. See original summary.

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

phenaproxima opened merge request !687

phenaproxima’s picture

phenaproxima
he/him
Primary language English
Location Massachusetts
commented
Status: Active » Needs review
tedbow’s picture

tedbow
he/him
Primary language English
Location Ithaca, NY, USA
commented

@phenaproxima did you check the other place mentioned in Proposed resolution 2)?

tedbow’s picture

tedbow
he/him
Primary language English
Location Ithaca, NY, USA
commented
Status: Needs review » Needs work

Looks good but I don't the original problem documented in the screenshot has test coverage.

This is when the User is on the UpdateReady form and there is a status check warning with 1 message and summary.

the closest test coverage we have is \Drupal\Tests\automatic_updates\Functional\UpdateWarningTest but it only deals with an warning with more than 1 message, that is why it was passing.

That should be update to handle both cases. I think just setting a different `TestSubscriber::setTestResult([$warning], StatusCheckEvent::class);` and reloading the page should be ok

phenaproxima’s picture

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

phenaproxima
he/him
Primary language English
Location Massachusetts
commented

Crediting you for review.

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
Status: Needs review » Needs work

🤯 This is such a no-brainer improvement. This explains why I've gotten confused in the past by the assertions in the build tests 😄

But … there's still something that I don't quite get: why we sometimes want only the summary, and not also the non-summarized messages?

phenaproxima’s picture

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

why we sometimes want only the summary, and not also the non-summarized messages?

As I understand it:

On the status report, and on the updater form, we want to show everything. Summaries and details. These areas are where the user is most concerned with the details, and could use as much information as possible to fix the problems.

On any other admin page (like /admin/structure and friends), we want only the summaries (or first message, if there is no summary). In such a context, we just want people to know that problems exist, and give people a basic idea of what those problems are. But providing the full details would be clutter.

Not seeing any other actionable feedback here, so this is back to needing review...

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
Assigned: Unassigned » phenaproxima

On any other admin page (like /admin/structure and friends), we want only the summaries (or first message, if there is no summary). In such a context, we just want people to know that problems exist, and give people a basic idea of what those problems are. But providing the full details would be clutter.

But how do we expect people to act on problems surfaced in the status report if they literally cannot see the details? 😅

phenaproxima’s picture

phenaproxima
he/him
Primary language English
Location Massachusetts
commented

They can see the details on the status report...? (By "status report", by the way, I'm referring specifically to /admin/reports/status.)

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented

That's not what I'm seeing in StatusCheckTest — see https://git.drupalcode.org/project/automatic_updates/-/merge_requests/68....

I verified locally by doing 

var_dump($expected_results['1 error']);
var_dump($this->getSession()->getPage()->getContent());
return;

immediately after that.

What am I missing? 🙈

phenaproxima’s picture

phenaproxima
he/him
Primary language English
Location Massachusetts
commented
Issue summary: View changes
StatusFileSize
new 3339657-13.png95.1 KB

If I'm understanding correctly, you're missing the fact that the result you're looking at only has one message. In which case, that's the only thing that gets displayed.

If I add the following code to the end of ComposerExecutableValidator:

    $event->addError([
      'One message with some details.',
      'Another message with some details.',
    ], t('A test summary'));

I get this on the status report:

A screenshot of a status check error on the status report page, expanded to show the two messages that were flagged during the status check. The messages themselves are just random strings.

Which is exactly what I would expect to see.

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
Assigned: phenaproxima » Unassigned
Status: Needs review » Reviewed & tested by the community

OMG — I must've missed that then in my detailed test 🙈 Sorry for the distraction!

tedbow’s picture

tedbow
he/him
Primary language English
Location Ithaca, NY, USA
commented
Status: Reviewed & tested by the community » Fixed

Status: Fixed » Closed (fixed)

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