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] Project Installer: Submodules not available to be enabled during the last step. #2200

Closed
klonos opened this issue Sep 16, 2016 · 18 comments · Fixed by backdrop/backdrop#3295

Comments

@klonos
Copy link
Member

klonos commented Sep 16, 2016

Part of #1288

Use case: I want to be able to quickly install Devel in order to be able to generate sample content for demoing Backdrop.

Process: I find the module through Project Installer, I add it to the installation queue and install it. At the last step, after the project has been downloaded and extracted for me, I am asked whether I want to enable it. I select to do so, but once it is enabled, I see no "Generate content" menu option. I then remember that the Devel Generate sub-module needs to be enabled for that. Why wasn't I given the option to enable that?

...anyways, Move on to the List modules page, scan for Devel Generate and finally enable it in order to get the "Generate content" menu option that I was initially after. Not ideal UX there.

@klonos
Copy link
Member Author

klonos commented Feb 19, 2017

OMG, I've been annoyed by this for so long!! PR up for review...

  • any submodules of installed projects are added to the list in /admin/installer/install/enable. You can test with Devel:

backdrop-issue2200-pr1795-devel_submodules

backdrop-issue2200-pr1795-references-no_submit_button_when_missing_requirements

@klonos klonos added this to the 1.6.2 milestone Feb 19, 2017
@klonos
Copy link
Member Author

klonos commented Feb 19, 2017

  • the list of modules + submodules needs to be ordered alphabetically by module title.
  • perhaps add some JS to automatically select required modules checkboxes when these are on the list (might need some help with that).
  • perhaps leave the warning about installing dependencies to be implemented as a dialog(??).

@quicksketch quicksketch modified the milestones: 1.6.2, 1.6.3, 1.6.4 Mar 15, 2017
@Dinornis
Copy link

Thanks Gregory, well done, this is a nice UX improvement.
I ran the Installer (module) test and there are 4 Fails.
Maybe I run the tests incorrectly?

FileName: installer.test
Function: InstallerBrowserAdministrationTestCase->testProjectBrowserInstallPage()
Messages:

FAIL: Found the Enable modules button		
FAIL: Found the requested form fields at
FAIL: DDD Installer test was enabled.		
FAIL: EEE Installer test was enabled.

@klonos
Copy link
Member Author

klonos commented Apr 28, 2017

Thanx for testing @Dinornis 👍 ...this PR is a WIP as many additional small things need to be addressed.

Nope, I don't think you have done anything wrong. I just need to fix the 492 automated test failures that come with this change 😄 ...Unfortunately, I don't have enough time to get this finished by the upcoming 1.7.0 release 😞 ...but then again this is a UX issue and will most likely be eligible for any 1.7.x release after that. I believe I will have more time after mid May. Sorry.

@Dinornis Dinornis modified the milestones: 1.x-future, 1.6.4 Apr 29, 2017
klonos added a commit to klonos/backdrop that referenced this issue Aug 7, 2018
… to be enabled during the last step.

This PR replaces backdrop#1795

A first step towards fixing backdrop/backdrop-issues#2200

- any submodules of installed projects are added to the list in `/admin/installer/install/enable`.
- bypasses the warning about installing required modules, but does install any requirements in the background.
- does not account for subthemes or layout packages that include multiple layout templates (separate issue?)
@klonos klonos modified the milestones: 1.x-future, 1.10.2 Aug 12, 2018
@serundeputy serundeputy removed this from the 1.10.2 milestone Sep 15, 2018
@docwilmot
Copy link
Contributor

@klonos have a look at this PR please. backdrop/backdrop#3295.

Tests passing, could use more tests, but the code for tests for Installer is a bit intricate and my first attempts failed (we have to try to trick Backdrop into thinking its pulling some fake release XML files from the main site and pretend that we're downloading zip files from GitHub, and somehow it wont all behave).

One detail, copied from your code, for checking for submodules, is that $module->info['project'] only gets a project key based on dependencies, so if submodules say dependencies[] = mainmodule in their info file then this code will detect and list them, but if for whatever reason there is a submodule which does not depend on its packaging main module, it wont be listed. If this is a likely situation then we could check by paths instead (more overhead likely as we'd need to do a string operation like strpos() on every module path returned by system_rebuild_module_data(), which is a lot).

@klonos
Copy link
Member Author

klonos commented Sep 7, 2020

Thanks @docwilmot ...I actually started reworking this on my local over the weekend. I'll look into your feedback as well.

@klonos klonos removed their assignment Sep 7, 2020
@klonos
Copy link
Member Author

klonos commented Sep 7, 2020

...I see. New PR 😅 ...I've closed mine in favor of yours @docwilmot 👍

I've tested this in the PR sandbox, and it works as expected, also taking into account whether the submodule is set to be hidden 👍

One minor improvement would be to nest submodules under each "parent" module if possible (can be a follow-up if too complicated). This is otherwise RTBC! Thanks @docwilmot.

@quicksketch
Copy link
Member

Can you post some before and after screenshots? The code looks harmless but I'm not sure what the end result is here.

@ghost
Copy link

ghost commented Sep 12, 2020

I haven't looked at the code or tried to test this, but reading the posts above I have one concern:

bypasses the warning about installing required modules (do we actually need this?), but does install any requirements in the background.

I think people should know if additional modules are being installed for them. It doesn't have to be a warning, but some sort of message indicating what additional modules will be installed would be helpful. Assuming this is still part of the latest PR...

@klonos
Copy link
Member Author

klonos commented Sep 13, 2020

Can you post some before and after screenshots?

Sure @quicksketch 👍 ...before:

image

...and after:

image

@klonos
Copy link
Member Author

klonos commented Sep 13, 2020

I think people should know if additional modules are being installed for them. ... Assuming this is still part of the latest PR

@BWPanda the TL;DR version is that the behaviour has NOT changed with @docwilmot's PR. Things work as previously.

...bypasses the warning about installing required modules (do we actually need this?), but does install any requirements in the background.

I think that this was an issue specific to my initial PR, which had the changes in installer_browser_installation_enable_page(). @docwilmot had suggested to move them into installer_browser_installation_enable_form(), which is where the change lives in his PR, and it solves all that.

Take installing webform_validate for example... After adding the module to the installation queue and clicking "Install", you're taken to the page where you are given the chance to select a version other than the latest:

image

After that step, you are taken to the "Install Dependencies" step (this is the step that was missing in my PR):

image

...after clicking "Install", any dependencies would be dowloaded, and you'd be asked to enable all the downloaded modules (including the dependencies of the module you initially selected):

image

@quicksketch
Copy link
Member

Thanks folks! Merged the fix in backdrop/backdrop#3295 into 1.x and 1.16.x!

@herbdool
Copy link

Follow-up bug: #5054

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