Problem/Motivation

According to @bnjmnm, Project Browser was originally envisioned to have more of an "add to cart" UX - as you browse, you choose which projects you want, and then you can click a single button to do the whole installation. Due to evolutionary and historical reasons, this wasn't implemented; we instead go through a laborious process of creating and destroying a whole sandbox every time you install a single project.

Proposed resolution

With the backend changes made by #3472206: Change the project_browser.stage.require and project_browser.activate routes to accept a list of project IDs in place, we are in a position to actually have this shopping cart type of UX. So let's do it - each project that can be installed should show an "Add to queue" button -- we can workshop the wording later. As you click them, you build up the list of projects you'll install. At the bottom of the screen, you should see a single "Install 3 projects" button, which when clicked kicks off the process of creating a single sandbox with the selected projects.

Select projects to install

Installing phase, with loader

After install phase

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:

Comments

bnjmnm created an issue. See original summary.

chrisfromredfin’s picture

Issue tags: +core-post-mvp
narendrar’s picture

Version: 1.0.x-dev » 2.0.x-dev

narendrar changed the visibility of the branch 2.0.x to hidden.

narendrar changed the visibility of the branch 3330887-gui-install-multiple to hidden.

narendrar’s picture

Assigned: bnjmnm » Unassigned
Status: Active » Needs work
Issue tags: +Needs tests

This MR still needs work, but it would be helpful to get early feedback on code/functionality.

To test this feature, enable Allow multiple installs at once checkbox at admin/config/development/project_browser

rkoller’s picture

thank you! a few initial thoughts testing the current state.

  • on the first look it feels odd to have "Queue" as the only available option / button label. if i want to simply install a single module i have to still queue a module and then process the queue with just a single module in. i am unable to directly install that module with one click.
  • Queue and Deque are quite abstract terms micro copy wise, same for the button label "Process queue". Plus "Process queue" sort of hides the fact that modules are being installed.
  • when you queue a module the process queue button gets appended at the end of the result list. problem here is for one for screenreader users the appearence of that button isnt announced (same as the state change of the toggled button from queued to dequeued) but it is also sort of cumbersome for keyboard users to have the process queue button not in close proximity where they just queued another module. if you queue one of the modules at the top of your list and you have for example 48 modules shown per page it is quite the stretch to reach the process queue button at the end of the result list by just tabbing.

oh and one fun fact. i've tried to process a queue but that had no effect (the button label jumed back to process queue after after a while). i then had one module queued then switched the number of modules shown per page count. that made the list of results reload. but the spinner spun indefinitely before any new results showed. so after while i simply did a reload and now i am presented with the installer and i am forwarded to core/install.php :D that was completely unexpected. not sure if there is a way to provide any infos what might have caused that behavior? i dont want to just run a siteinstall and wipe the database (it seems to be it is still available and in place), wanted to wait on a feedback first. without the gui changes in front of me right now i am unable to ideate on potential other approaches ui wise for now.

rkoller’s picture

ha it looks like it is reproducible. I have set up a new instance with the MR applied:

  • i've queued token and pathauto
  • then processed the queue.
  • after a while processing the progress spinner disappeared again (by the way it would be good to have the spinner inline and in context of the process queue button, at the moment it is just an overlay across module cards, which has a high cognitive load - was the same for spinner for the install button, there the spinner was also an overlay at first in the context of the module card for the queue the context is the entire viewport)
  • after that i simply changed from 12 to 48 modules shown -> the indefinite spinner starts again and on reload the drupal installer is shown again.
phenaproxima’s picture

Issue tags: +Needs followup

@rkoller, thanks for testing this. I think we might want to handle some of your feedback in follow-up issues (as well as re-titling this issue for clarity), since we're simply trying to get the plumbing in place to install multiple modules at once, which is a significant lift, and UI issues will take a back seat for this issue.

Tagging this for a follow-up to make sure that all your feedback is captured in another issue.

rkoller’s picture

all good, thanks for the clarification! the only detail perhaps relevant within the scope of this issue is the problem outlined in #10, or is it only caused by the ui and isn't caused in the plumbing?

p.s. even if i run drush si another time i am still getting re directed to the install page

narendrar’s picture

Still more work needs to be done, but here is current functionality:
1. In branch 2.0.x, After clicking Add and Install and then reloading page. (Does not worked, need to Unlock Install stage)
2. In branch 2.0.x, After clicking Add and Install and then increasing pagination size from 12 to 24 (It worked, but status on button does not changed to installed, will create issue for that)
3. Add multiple modules with multiple install MR and click Process Queue button and wait. (It works as expected)
4. Repeated step 1 for multiple install MR. (Same problem as step 1, need to unlock stage)
5. Repeated step 2 for multiple install MR. (Same problem as step 1, need to unlock stage, but if you wait, modules will get installed)

phenaproxima’s picture

I discussed this issue with @bnjmnm today.

In reviewing the code with @narendrar, I felt a certain tension between what the code is trying to do -- which, by the way, is a perfectly reasonable approach -- and the way Project Browser wants to work: install one thing at a time, creating and destroying a sandbox for each thing that gets installed. That has led to the current code needing to do some very tricky state management and tip-toeing around the possibility of page refreshes, etc.

Then Ben and I remembered something -- Package Manager can require multiple things at once into a single sandbox! That, I realized, is game-changing.

Instead of trying to create a queue of projects to require into the sandbox, I think we should actually keep the current approach, wherein you create the sandbox, require everything, and then apply the sandbox, in one go. The only difference should be that, rather than requiring one module at a time into the sandbox, we require several!

To put it another way: if the user selects Token, Metatag, and Pathauto in Project Browser, then the following single call should be made to Package Manager:

$installer->require(['drupal/token', 'drupal/metatag', 'drupal/pathauto']);

This has the major advantage that it will immediately fail if any of the chosen projects have dependency conflicts amongst themselves. It also means that we get to keep the front-end changes relatively simple, and don't need to implement a queue.

That said, the main blocker to being able to do this is the fact that Project Browser has a route for requiring a project into the sandbox, and it assumes you're only requiring a single project, whose ID is passed in the URL. We need to change that route so that it can accept a list of project IDs, not just a single one.

So I propose the following: let's open an issue that blocks this one, and which changes the project_browser.stage.require route such that:

  • Its URL becomes /admin/modules/project_browser/install-require. (No more {source} or {id} parameters).
  • It is a POST route, not a GET route.
  • The request body needs to be a JSON-encoded array of arrays, where each array is a source ID and project ID. Like this:
    [["some_source","some_project_id"],["some_source","another_project_id"]]
    

In that same issue, let's also change the project_browser.activate route to work the same way, so we can activate multiple projects at once.

Once we have that committed, we can come back to this issue, and then implement a much simpler set of changes that simply adjusts the current front-end code to pass a list of chosen projects to the backend. (The "Queue" and "Dequeue" stuff, and the "Install N projects" button, will function more or less the way they already do.)

I think this will get us where we need to be much more cleanly and quite a bit more simply. It's a win.

phenaproxima’s picture

phenaproxima’s picture

Title: [PP-1] GUI install multiple modules at once. » GUI install multiple modules at once.
Status: Postponed » Needs work

And, unblocked!

narendrar changed the visibility of the branch multiple_modules to hidden.

narendrar’s picture

Issue tags: +Needs tests

narendrar changed the visibility of the branch 3330887-Testing-do-not-use to hidden.

narendrar’s picture

Status: Needs work » Needs review
Issue tags: -Needs tests

phenaproxima changed the visibility of the branch gui_branch to hidden.

phenaproxima changed the visibility of the branch gui_branch to active.

phenaproxima’s picture

Status: Needs review » Needs work

I solved the failing tests!

Turns out the problems is that the GitLab CI template now runs Chromedriver in W3C mode (at least when the next major version of core is in use), which doesn't allow you to "click" an option element. So I don't think our changes here introduced this problem - it was a latent thing waiting to be discovered. Having said that, the fix was easy; just handle option elements properly in clickWithWait(). We're good now.

Beyond that, I think something in the JS could be a little more cleanly encapsulated but I don't think I have too much to complain about, overall. Coding standards checks are failing, too, but should be easy to resolve.

narendrar’s picture

Status: Needs work » Needs review
phenaproxima’s picture

Status: Needs review » Needs work

I gave this another review and I'm a little concerned about certain scoping issues and a few other things. I don't want to hold up commit, but this feels like it could be a little tighter in some places...

chrisfromredfin’s picture

I think in going "all in" on the multiple-at-a-time, it's good to remove / change the functionality of the "add and install" button. That button becomes more of a checkbox of what you're putting in your "Cart" (for lack of a better word), and we move the "Install >" action ("checkout") somewhere else (my current random thought is a sticky ribbon at the bottom of the page?)

phenaproxima’s picture

Issue summary: View changes
narendrar’s picture

Status: Needs work » Needs review
phenaproxima’s picture

Status: Needs review » Needs work

I'm running low on things to complain about. You're probably sick of me kicking this back by now. But this really does make sense and I like the UX improvement we're doing here, yet there are a few things I can't sign off on -- at least not without more explanation.

  • Changing that test assertion to account for a different number of items is a problem - that number shouldn't have changed. Nothing in this code should result in that. Changing it just to make the tests pass isn't good enough unless we can also explain why that change makes the test pass.
  • I'm not clear on why the list of queued projects is segmented by tab. That really shouldn't be the case - projects are specific to a particular source. The queue should be everything queued across all sources, regardless of which tab they came from. The whole segmenting-things-by-tab is a pattern I would like to see removed, ultimately, from Project Browser.
  • There are some random failures waiting to happen in the JS test coverage. Also, there's some coverage that was removed which I'm not sure should be.
narendrar’s picture

Status: Needs work » Needs review

Re #30,

  1. This is failing in 2.0.x also https://git.drupalcode.org/project/project_browser/-/merge_requests/567#note_384275
  2. Can we do it in a follow-up?
  3. Done
phenaproxima’s picture

This is failing in 2.0.x also

Oh, are these random failures?? If they are, I'm not clear on how changing the assertion is going to fix them.

Can we do it in a follow-up?

Sure, but I'm curious why. Is it harder to flatten the queue than to segment it by tab? If so, why is that?

narendrar’s picture

Oh, are these random failures?? If they are, I'm not clear on how changing the assertion is going to fix them.

These are failure in 2.0.x and nothing to do with changes we have done.

Sure, but I'm curious why. Is it harder to flatten the queue than to segment it by tab? If so, why is that?

It's not hard to do, recommending it so that what you suggested can be done in a single issue. Currently we are showing 'Install selected projects' based on queued projects under specific plugin which otherwise needs to check each queued project and then needs to be updated.

phenaproxima’s picture

I ran a git bisect and confirmed that #3318817: Improve the categories filter type in context to the rest of the filter component ui broke the results count.

Tests passed in that issue, though, so it's not clear why it didn't break then and there, or indeed, in any subsequent issues. There have been several commits since that one.

However, I'm satisfied that we did not cause this problem. I don't fully trust that we've actually fixed it, and we still don't know the root cause, but I don't think there's any need to hold this issue up on that anymore. At least we've got a trail we can follow for forensic purposes now.

phenaproxima’s picture

Status: Needs review » Needs work
Issue tags: +Needs screenshots, +Needs manual testing

There are a few assertions which could be made less flaky, and one follow-up to file.

Otherwise, I'd call this RTBC from a code perspective. There are things we could fine-tune and streamline and clean-up but this appears to accomplish what it sets out to do.

@narendrar, can you add screenshots?

Marking for manual testing as well.

narendrar’s picture

Issue summary: View changes
Issue tags: -Needs screenshots
StatusFileSize
new603.09 KB
new613.49 KB
new556.98 KB

I have addressed all feedback and added screenshots.

rkoller’s picture

StatusFileSize
new405.83 KB
new406.78 KB

I've tested the latest changes. one detail to note, the install selected projects button is visible and reachable with the toolbar module installed

the toolbar on top of project browser search page and at the bottom you have the install selected projects on the bottom left

while it is not visible nor reachable with the navigation module installed on drupal 11.x

the navigation sidebar on the left of project browser search page and no install selected projects button visible anywhere

narendrar’s picture

Re #37, Thanks for reporting this @rkoller. Actually the install selected projects button was hidden under admin toolbar which is fixed now.

narendrar’s picture

Status: Needs work » Needs review
Issue tags: -Needs manual testing
phenaproxima’s picture

Status: Needs review » Needs work

No complaints at the code level.

This has been manually tested now by @rkoller, and he didn't find too much, which is a good sign because he usually finds a lot! It's also been reviewed, although I'm not sure how deeply, by @bnjmnm - his feedback has not yet been addressed. Since that will help make the tests less fragile, I'm kicking this back to just do what he has asked, and once that's done, this is ready in my opinion.

narendrar’s picture

Status: Needs work » Needs review

I tried to address Ben's feedback in previous commit.

phenaproxima’s picture

Status: Needs review » Reviewed & tested by the community

OK, well, as far as I can tell we're good to go here. At long last, let's ship it.

chrisfromredfin’s picture

I've manually tested this and while I don't LOVE the UI, my feedback is very likely minor, most likely considered feature requests, and can readily be dealt with as a follow-up. This is a big step for 2.0.x-dev as we move forward. To get the plumbing in there's no reason to wait on this.

chrisfromredfin’s picture

Status: Reviewed & tested by the community » Fixed

Huge!

Status: Fixed » Closed (fixed)

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