[UX] Hide "Run Database Updates" / next steps if there are no next steps

[laryn]

Describe your issue or idea

When updating a module via the UI, there is not always a database update to complete, but the "Next Steps" section always shows with the "Run database updates" link:

When you click the link, you are informed that there are no database updates pending (and a warning about backing up before you run updates).

It would be better to just display a "No updates pending" message after the first step.

[klonos]

Yeah, thanks for filing this ticket @laryn 👍 ...I don't understand what is the use of the "project_name: Installed project_name successfully" list.

I also do not understand why we are listing "Your projects have been downloaded and updated." as a task. It feels like it should be a message, but we already have "Update was completed successfully." in place, so perhaps we should just remove it altogether.

If we remove the "Your projects have been downloaded and updated." task, then we are left only with the "Run database updates" task, in which case, why aren't we detecting whether there are any db updates or not and act accordingly (either redirect to /core/update.php, or back to /admin/modules/update)?

Also noting that this step is in /authorize.php, which we are looking to remove (#3208).

[klonos]

...yep, the code for this is in installer_authorize_update_batch_finished(), which states:

Batch callback: Performs actions when the authorized update batch is done.

This processes the results and stashes them into SESSION such that
authorize.php will render a report. Also responsible for putting the site
back online and clearing the update status cache after a successful update.

[klonos]

...when installing a module (as opposed to when updating), you get this screen instead:

The list of tasks here makes more sense.

[laryn]

Just ran into this again while testing core update via the UI. At a minimum it would be a cleaner experience without the "Run database updates" link under "Next steps" (which takes you through another two pages that are not very friendly) only to then tell you that you didn't need to do anything.

[yorkshire-pudding]

I totally agree with this ticket, we need to focus on what they need to do. Very related to this workflow is how the UI handles maintenance mode for installs and updates.
It advises you to put into maintenance mode to install the module/update and gives you a handy tick box to do it. Then it takes you out of maintenance mode. It then (let's assume there are database updates) says to do the database updates and recommends they're done in maintenance mode, however there is no option to re-enable it; you would need to come out re-enable maintenance mode then go back and activate the database updates.

What it should do, taking into account the above:

1. Download modules/updates
2. Recommend to put into maintenance mode and keep the option
3. If there are database updates, keep it in maintenance mode
4. Once database updates are done take out of maintenance mode

Let me know if this needs to be a separate issue.

Just had a look at the PR on tugboat and maybe this is covered? Couldn't tell

[stpaultim]

What is the easiest way to test this?

[laryn]

@stpaultim
Install an outdated version of a module in the sandbox (or locally on a site running this patch), making sure that there are no database updates in between the version you install and the latest version. Update the module through the UI and try to verify that it doesn't walk you through to the database updates page, and that the module does actually get successfully updated.

[klonos]

I'll have a look at the PR soon, I promise.

Just ran into this again while testing core update via the UI.

Yeah, I just run the recent security update for core and got this:

I would suggest a few improvements:

• split the 2 different things (update finished successfully / out of maintenance mode) into separate messages
• use the human name of the module(s)/project(s) being updated
• "your projects have been downloaded and updated" doesn't belong in the "Tasks" section - it should be a message.
• list multiple projects using theme_item_list()

[yorkshire-pudding]

I'd like to reiterate my suggestion that the process should handle maintenance mode for the database updates. If we recommend (and I agree with the recommendation) that they put the site into maintenance mode for the database updates, then we should help them with that.

[laryn]

@klonos I'm still interested in this but won't have time to look at it in the near future. Feel free to take over if you wish!

[yorkshire-pudding]

@argiepiano - that works for with db update. Will quickly find a suitable case for testing without to cover all bases.

[argiepiano]

You can use Entity Plus version 15 and version 16

[argiepiano]

I just realized that there are cases where a submodule may have updates. My PR only loads the install files for the main modules. I think I need to revert to loading the install files for all the enabled modules. I'm going to modify the PR and commit those changes in a minute

[yorkshire-pudding]

Ok. Tested with Site_alert and all looked good

d

[argiepiano]

This is going to be trickier than it seemed at first.

Even the current PR, which worked for you, will fail if there are more than one module being updated. The issue is that backdrop_get_schema_versions() static-caches the list of enabled modules and uses that cache to check for hook_update_N in them in the future. The problem is that the call to backdrop_get_schema_versions() inside the new code in installer.authorize.inc happens at a point where we are apparently in bootstrap or maintenance mode, not in runtime mode, and therefore module_list() inside backdrop_get_schema_versions() only return 2 modules: system and update, which are cached. So the next time backdrop_get_schema_versions() is invoked (when there are more than one module being updated), that limited cache will be used, and therefore the new code won't detect that there are updates available.

So... this is getting complicated. One solution may be to to a backdrop_static_reset('backdrop_get_schema_versions') in the new code in every iteration before calling backdrop_get_schema_versions. Which seems really overkill for a feature that is really not that important (not showing a link to database update, which only admins see, doesn't seem so crucial).

Perhaps I'll let @laryn decide whether he wants to pick this up and find another way?

[argiepiano]

Well, after further testing it turns out that the PR as it stands now DOES work when there are multiple module updates. My PR only iterates on the successfully updated modules to see if there are any database updates to be run. The original PR from @laryn used module_list(), which doesn't work in that context.

However, the issue remains to be tested on whether database updates in submodules (rather than on the main module) will be picked up.

[klonos]

These PRs will need to be updated after the change from "database updates" to "site updates" landed with #5630.

@argiepiano, @laryn, do you have capacity to work on this, or should I take a crack at it? (starting off of your PRs)

I also need to check if there's overlap with #512 (or how much overlap)

[laryn]

@klonos You are welcome to it. I'm not sure when I'd be able to come back to this one.

[klonos]

Thanks @laryn 🙏🏼 ...I believe that we can close your PR. Can you please do that?

@argiepiano I believe that your PR is close. Can you please resolve the conflicts, and have a look at the comments I've left?

[argiepiano]

@klonos, I'm on the road for the next 2 weeks, with very little capacity. Feel free to pick up. I'm closing mine.