localgov profile doesn't enable the localgov_roles submodule

[Adnan-cds]

The LocalGov profile currently doesn't enable the localgov_roles submodule from the localgov_core module. I wasn't even aware the localgov_roles module exists and was wondering what roles to create immediately after installing the LocalGov Drupal distribution recently. Somehow I discovered the localgov_roles submodule and was happy. Others may not be so lucky. Perhaps we should install this module as part of the LocalGov profile installation.

FAO @stephen-cox @finnlewis @andybroomfield

[stephen-cox]

Sounds sensible to me. The roles module is very basic and needs extending, but all sites based off the profile will need a basic set of roles and permissions.

[finnlewis]

Agreed - also might make sense to add an 'Admin' role and set it to be the default admin role. Onl Lambeth we created a roled called 'Admin' for that purpose.

[tebb]

So @MattRFletcher, same? as previous issue, it looks like just adding this near line 43 in localgov/localgov.info.yml (respecting alphabetical order)

• localgov_core:localgov_roles

[tebb]

This has been an excellent issue to get up to speed on PHP dev with Lando and Xdebug. It's working out really well, setting it up for myself and explain things to Matt so that he benefits - Thanks @Adnan-cds @finnlewis !

I have various things configured, working and understood: .lando.local.yml + php.ini with Xdebug and lando phpunitdebug with 2 connections into vscode. Cool tools!

Where I'm stuck: I'm not clear on the terminology - exists/installed/enabled - and as a result, what tests are needed.

After adding the roles submodule here in the 'install:' section of localgov.info.yml

  - localgov_core:localgov_media
  - localgov_core:localgov_roles
  - localgov_menu_link_group:localgov_menu_link_group

... this 'module exists' assertion succeeds as you'd expect:

$this->assertTrue($this->container->get('module_handler')->moduleExists('localgov_roles'));

... but we want to test for enabled don't we? ... and 'exists' != 'enabled' is it?

So do I also need to test using BrowserTestBase for the module's checkbox being ticked?

I looked at this with Chrome dev tools and counter-intuitively, this means testing for the checkbox having attribute disabled='disabled' in the admin/modules list.

Any, brief guidance would be appreciated :)

[danchamp]

It's not clear from the function name, but moduleExists checks both that the module is installed and enabled:

moduleExists documentation

HTH!

[finnlewis]

Thanks for working on this @tebb

Another test might be to confirm that an expected role is present, but the test for that would probably be better living in the localgov_roles module itself. In which case, just testing that the module is enabled should be enough from a profile perspective.

[tebb]

@danchamp Thanks for your help and Happy Hogmanay! Ha, it was that simple! ... still, I learnt a few things digging around.

(Next on the to-do list: Getting 'go to definition' fully working in VS Code for Drupal.)

@finnlewis Yes, we noticed the two roles it currently adds. Quite a lot of thinking went into 2 lines of code :)

The tests for this pr won't run because it's trying to use PHP 8.0. The issue is noted on the Technical Group Slack channel where I have added:

"Just had the same failure due to tests being attempted on PHP 8.0. In case it helps - from a very quick scan - so may be entirely wrong!

Perhaps pin Ubuntu to ubuntu-18.04 rather than ubuntu-latest until LGD works with PHP 7.4?

... and because 18.04 may also default to 8.0, set PHP version:

- name: Blah
  with:
    php-version: '7.2'

... from: https://github.com/marketplace/actions/setup-php-action#cloud-osplatform-support "

[Adnan-cds]

... and because 18.04 may also default to 8.0, set PHP version:

Thanks Tebb. Yeah, looks like that's what we need as a temporary fix. Andy and Finn has already added it to the test file template, now we need something similar in each repository. You can try adding a similar change to this repo's test workflow file as part of this issue's pull request and see if that works.

[andybroomfield]

This looks done.