Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[UX] System updates form: Merge the manual updates section into the main updates table #5085

Open
klonos opened this issue May 10, 2021 · 12 comments · May be fixed by backdrop/backdrop#3621
Open

Comments

@klonos
Copy link
Member

klonos commented May 10, 2021

The form under admin/config/system/updates seems too busy when the "Manual updates" option is disabled, having two sections/tables, and some help text in between:

image

How about something like this instead?:

issue-5085-merge-manual-updates-section

So basically:

  • merge the manual updates section (only core really) with the main tableselect
  • have the checkbox(es) for projects that require manual updates either be disabled/locked, or remove them completely
  • indicate which projects require manual update via a suffix in their title
  • add a more/less toggle link (same as those used in the modules listing and the status report page), which reveals manual update info
@klonos
Copy link
Member Author

klonos commented May 10, 2021

PR up for review: backdrop/backdrop#3621

PS: I've cleaned up some bits of the code that we still assuming that we have separate sections for enabled/disabled modules - we now have a dedicated "Project status" column in the tableselect for that.

@jenlampton jenlampton added this to the 1.19.0 milestone May 16, 2021
@jenlampton
Copy link
Member

I think this is an improvement, but I have some suggestions for improved text. I'll do the code review first.

@jenlampton
Copy link
Member

Hm. Actually, this may need more work than we can do before the 1.19.0 release.

In the world of Backdrop updates, we already use the word "Manual" to mean something else:
Screen Shot 2021-05-15 at 7 22 16 PM

In the settings form (see screenshot) "Manual" means that someone can click a button in the Backdrop UI and the files on disk will be replaced for them.

In the PR for this issue, the word "manual" is used to mean that someone will need to go into their filesystem, and replace files themselves. I think it will be confusing to use the same word for those two different actions. The second action requires that someone has ssh or ftp access to their code. It's a whole different level of access to a site.

@klonos
Copy link
Member Author

klonos commented May 16, 2021

In the world of Backdrop updates, we already use the word "Manual" to mean something else.

@jenlampton do you mind moving the discussion for this in #5089 instead? I feel that it might get really long 😅

@klonos
Copy link
Member Author

klonos commented May 16, 2021

...considering that:

Can we then please not block this UI improvement over the terminology clarification?

@jenlampton
Copy link
Member

Sorry @klonos, I hadn't seen (or forgotten about) the other issue.

@jenlampton jenlampton modified the milestones: 1.19.1, 1.19.2 May 26, 2021
@quicksketch quicksketch modified the milestones: 1.19.2, 1.19.3 Jul 21, 2021
@jenlampton jenlampton modified the milestones: 1.19.3, 1.19.4 Aug 12, 2021
@izmeez
Copy link

izmeez commented Oct 9, 2023

I like this suggestion. When there are several updates the core manual update can get lost further down the page. With this approach and the absence of a checkbox and the additional more text it brings the information for quickly to attention. Since the update settings have been set to not self-update, it's good to quickly see if a new update is available.

@izmeez
Copy link

izmeez commented Oct 26, 2023

Tested the PR and left comment with the PR.

@klonos
Copy link
Member Author

klonos commented Oct 26, 2023

Thanks for testing @izmeez 🙏🏼 ...setting this to NW, to work on the issue you discovered in the PR.

FTR, here's the issue for the benefit of others in this thread:

I like this idea and the PR is working in that I can launch the tugboat instance and sign in. However, since it is using the development branch on the update report there is no update needed and the manual update required with drop down more text is not displayed. I will have to apply a patch to to test against an older version. If I press on within tugboat to install system updates, it doesn't tell me I can't instead it goes ahead and then hits an error:

bd-tugbot-pr-available-updates-install-system-updates-on-1-27-x-error

Plus, it invites me to continue and we all know this is going to lead nowhere, so why are we going down this path. I thought before with PR on the dev branch I thought I had seen a message that updates were disabled and had to be done manually. I'll have to sort that out in my mind.

@klonos
Copy link
Member Author

klonos commented Oct 26, 2023

However, since it is using the development branch on the update report there is no update needed and the manual update required with drop down more text is not displayed.

Yeah, that won't be able to be tested on the PR sandbox - you'll need to test that on your local. ...although, we could manually set BACKDROP_VERSION in bootstrap.inc to 1.26.0 or older, which would trigger the UI changes that need to be reviewed/tested here, and then make a note for our core committers to revert that change before they merge this.

edit: I've filed #6279 in order to explore ways to mock the backdrop core version, but only within the context of PR sandboxes

@klonos
Copy link
Member Author

klonos commented Oct 26, 2023

For reference to the current situation (w/o the changes in this PR), here's what the expected result is:
image

Although I don't find the UX for the above ideal, that's what we have in place (so if we want to improve that, it'd need to be a separate issue I believe). Anyway, when you click "Continue", this is what you should get (core/authorize.php):

Whereas with the current PR, clicking the "Continue" button does absolutely nothing (and that's what needs to be fixed).

@klonos
Copy link
Member Author

klonos commented Oct 26, 2023

...also, this PR introduces custom JS for the more/less toggle, which we don't need if #5090 gets merged. If this change here gets merged first, we'll need to update the PR over in #5090 to remove the custom JS we are introducing here, and instead use a more/less toggle based on a <details> element.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment