Problem/Motivation

This module has a few problems (e.g. #3551233: "Which user type should be allowed" (negate) setting is implemented incorrectly, #3437797: Role hiding assumption is too optimistic, etc.) we should refactor some of the code in the module.

Steps to reproduce

Proposed resolution

Refactor the module

Remaining tasks

User interface changes

API changes

Data model changes

Issue fork jsonapi_role_access-3558968

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

grevil created an issue. See original summary.

grevil opened merge request !4

grevil’s picture

grevil commented
Status: Active » Needs review

All right, all done. Quite a bit of refactoring, but everything is pretty straigth forward, except the removal of the javascript, but hear me out on this one.

Conditionally hiding / showing checkboxes has many problems:

  • Roles that are checked, but hidden through js still exist in config.
  • Error prone, (see #3437797: Role hiding assumption is too optimistic).
  • Too opinionated: anonymous, authenticated and admin roles can be whatever the website creator wants it to be. This module only covers the default configuration for these roles.

Also, the user is now also able to select the "administrator" role. Unsure, why this was filtered out from the options.

Please review!

anybody’s picture

anybody
Primary language German
Location Porta Westfalica
commented
Status: Needs review » Needs work

Okay I'm fine with the decision, but we should keep "negate" as Drupal standard IMHO

grevil’s picture

grevil commented
Status: Needs work » Needs review

Done!

anybody’s picture

anybody
Primary language German
Location Porta Westfalica
commented
Status: Needs review » Reviewed & tested by the community
grevil’s picture

grevil commented
Status: Reviewed & tested by the community » Fixed

Pipeline display is stuck. Pipeline is green: https://git.drupalcode.org/project/jsonapi_role_access/-/pipelines/666020. Merging....

Now that this issue is closed, review the contribution record.

As a contributor, attribute any organization that helped you, or if you volunteered your own time.

Maintainers, credit people who helped resolve this issue.

grevil’s picture

grevil commented
Version: 1.0.x-dev » 2.x-dev

  • grevil committed 5709eece on 2.x 
    Fix settings page, related to issue #3558968

Status: Fixed » Closed (fixed)

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