The fact that permissions_by_term includes both Behat Feature Context and Features is awesome! However it could be more awesome if the module extended the Drupal Extension to Behat and Mink by harnessing the subcontext capability. To do so, we'd need to add a `.behat.inc` file with the current Feature Context. This should be a fairly straight forward process of converting PermissionByTermContext to a class extending DrupalSubContextBase and implementing DrupalSubContextInterface. Once enabled, permissions_by_term step definitions can easily be utilized by including the subcontext of the entire contrib module directory (e.g. docroot/modules/contrib).

CommentFileSizeAuthor
#8 permissions_by_term-3075174-behat_inc-8.patch11.52 KBxurizaemon
#6 add_behat_inc_file_for_behat_testing_using_drupal_extension_subcontext_3075174_6.patch12 KBbrooke_heaton
#3 add_behat_inc_file_for_behat_testing_using_drupal_extension_subcontext_3075174_3.patch11.96 KBbrooke_heaton

Issue fork permissions_by_term-3075174

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:

About issue forks

Comments

brooke_heaton created an issue. See original summary.

brooke_heaton’s picture

brooke_heaton commented
Issue summary: View changes
brooke_heaton’s picture

brooke_heaton commented
StatusFileSize
new add_behat_inc_file_for_behat_testing_using_drupal_extension_subcontext_3075174_3.patch11.96 KB

Added new `behat.inc` file with PermissionsByTermDrupalSubContext class extending DrupalSubContextBase implementing DrupalSubContextInterface. This can now be utilized with the following `behat.yml` configuration:

Drupal\DrupalExtension:
blackbox: ~
api_driver: 'drupal'
drupal:
# This must be an absolute path.
drupal_root: /path-to-drupal
drush:
alias: '@mysite.local'
subcontexts:
paths:
- /docroot/modules/contrib/

brooke_heaton’s picture

brooke_heaton commented
Status: Active » Needs review
brooke_heaton’s picture

brooke_heaton commented
Issue summary: View changes
brooke_heaton’s picture

brooke_heaton commented
StatusFileSize
new add_behat_inc_file_for_behat_testing_using_drupal_extension_subcontext_3075174_6.patch12 KB

Updating the filename to `permissions_by_term.behat.inc` This seems to be the proper naming convention.

xurizaemon’s picture

xurizaemon
he/him
Location Ōtepoti, Aotearoa 🏝
commented
Status: Needs review » Needs work

This seems really good! A few notes from a bit of debugging today :)

PermissionsByTermDrupalSubContext::createRoles() uses the rid to reference roles. DrupalExtension's DrupalContext::assertAuthenticatedByRole() and similar steps discover by role name (in Drupal\Driver\Core\Drupal8::userAddRole()). I propose we follow Behat/DrupalExtension "style" which seems to be to use the user-facing labels; I'm happy to make it continue supporting rid for this since I don't want to break existing tests.

The same method will create a new term every time, but recycle an existing role. This confused me - it was duplicating the term, then creating nodes associated with the pre-existing term, then (correctly) not showing the restricted nodes ... I propose we recycle an existing term if matched which feels most natural.

While I think of it: I want to relabel "Node access records are rebuild" as "Node access records are rebuilt" (step def and method name).

Thanks heaps! Expect a patch in the next week or so, or assume I forgot and feel free to ping me - for tonight I've just adapted the tests to suit the required inputs:

    Given restricted "access_control" terms:
      | name      | field_ip_access_range | access_role |
      | TempTerm1 |                       | sponsor     |
      | TempTerm2 |                       | subscriber  |
xurizaemon’s picture

xurizaemon
he/him
Location Ōtepoti, Aotearoa 🏝
commented
StatusFileSize
new permissions_by_term-3075174-behat_inc-8.patch11.52 KB

Couple of minor changes in this:

  • My patch removes the gatherContexts() and class internal $drupal and $mink references. These weren't used AFAICT and attempting to discover them prevented the context from loading in our CI environment.
  • Nitpicking rephrase of "Node access records are rebuild" to "node access records are rebuilt" because language consistency helps people like me save brain cycles

I expect to look at the other items, just getting this patch out early so I don't forget to do it later.

xurizaemon’s picture

xurizaemon
he/him
Location Ōtepoti, Aotearoa 🏝
commented

This has been working for us for some time - review / consideration welcome!

jepster_’s picture

jepster_
Location 🇩🇪Düsseldorf
commented
Status: Needs work » Fixed

Thanks for providing the patch. It's really much more straightforward to use the .behat.inc file. Especially for using within a CI pipeline.

I have slightly improved the changes and created the releases for the Drupal 9 and 8 version:

- D9: https://www.drupal.org/project/permissions_by_term/releases/3.1.12
- D8: https://www.drupal.org/project/permissions_by_term/releases/8.x-2.33

jepster_’s picture

jepster_
Location 🇩🇪Düsseldorf
commented
Status: Fixed » Closed (fixed)

jnicola made their first commit to this issue’s fork.

xurizaemon’s picture

xurizaemon
he/him
Location Ōtepoti, Aotearoa 🏝
commented

@jnicola it looks like you've opened an MR on a closed issue - if there is a bug with the existing code, or there are new changes to propose, then I suggest opening a new issue for that MR. If related (branch name on your MR suggests so?) then you could add a comment linking to that issue from here for visibility.

jnicola’s picture

jnicola commented

Hey guys sorry about that! I meant to open this on the Group module behat ticket but got this one instead and by the time I figured it out I didn't see how to delete the fork... probably best I stop trying to do behat extension work late at night :)