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

Role merge #1185

Merged
merged 20 commits into from
Mar 22, 2024
Merged

Role merge #1185

merged 20 commits into from
Mar 22, 2024

Conversation

dbauszus-glx
Copy link
Member

Role objects should not be merged in the mapp library when a layer is decorated.

The method to merge role objects has been added to the Roles util module.

The role objects maybe on the local, layer, or any nested object (e.g. infoj entries).

The Roles.objMerge() method will be called before returning a layer or locale from their respective workspace modules.

This makes the Role.filter() method which is only called in the query mod after the layer is returned from getLayer mod redundant.

@dbauszus-glx dbauszus-glx added Feature New feature requests or changes to the behaviour or look of existing application features. Code Issues related to the code structure and performance. labels Mar 15, 2024
@dbauszus-glx dbauszus-glx linked an issue Mar 15, 2024 that may be closed by this pull request
@dbauszus-glx
Copy link
Member Author

I removed the jest test.

Copy link
Contributor

@RobAndrewHurst RobAndrewHurst left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Giving this a test on bugs-testing/roles/roles_filter_workspace.json

I can see a layer with the "Super" role defined even though my user doesn't have that role assigned.

image

@dbauszus-glx
Copy link
Member Author

The objMerge must happen in the workspace module before the obj is returned from the response.

@AlexanderGeere
Copy link
Contributor

AlexanderGeere commented Mar 20, 2024

@dbauszus-glx the roles are affecting the layers correctly, but when I tested on bugs-testing/roles/roles_merge_workspace and set my role to Super or user I can see both dataviews.

tested on 4.8.1 as well and it works as expected where i can only see the dataview visible to my role.

Copy link
Contributor

@RobAndrewHurst RobAndrewHurst left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice one! All working for me again!

@RobAndrewHurst RobAndrewHurst mentioned this pull request Mar 21, 2024
Copy link
Contributor

@simon-leech simon-leech left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Role restriction on locales / layers is working for me.

  1. But merging layer information is not working with roles.
    A roles object on a layer of
"roles": { 
"super": { 
"toggleLocationViewEdits": true
},
}

When I don't have the super role I am still able to toggle editing, i shouldn't be able to do so.

  1. If I have a default locale containing layers, and try to remove one layer from a specific locale - I hit an error message. This was introduced in PR Merge locale into template #1040 but no longer works on this PR.
    e.g.
    "locale": {
      "layers": {
        "test": {},
        "test_a": {}
      }
    },
    "locales": {
      "UK": {
        "layers": {
          "test": null
        }
      }
    }

@dbauszus-glx
Copy link
Member Author

@simon-leech the check on negated roles was borked. this should now work.

The negatedRole flag should be assigned for every user without the 'foo' role.

"!foo": {
  "negatedRole": true
}

@dbauszus-glx
Copy link
Member Author

@simon-leech

The '*' role will now be spliced into the user_roles array prior to the objMerge.

@dbauszus-glx
Copy link
Member Author

@simon-leech

A null layer should now remove the layer from layers list.

  "locale": {
    "layers": {
      "OSM": {
        "display": true,
        "format": "tiles",
        "URI": "https://{a-c}.tile.openstreetmap.org/{z}/{x}/{y}.png",
        "attribution": {
          "© OpenStreetMap": "http://www.openstreetmap.org/copyright"
        }
      }
    }
  },
  "locales": {
    "test": {
      "layers": {
        "OSM": null,
        "retailpoints": {}
    }
  }
}

Copy link
Contributor

@simon-leech simon-leech left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've tested this and it looks good to me -
Layer restrictions
Layer null in locale from default locale
Infoj role restrictions
Layer editing role restrictions

All of these were working for me - i tested using a role, two roles and no role and I got the expected outcomes.

Copy link
Contributor

@simon-leech simon-leech left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Noticed an issue with filtering and using a filter.current or filter.default.
When you toggle a thematic option in a style you end up with an incorrect SQL WHERE Clause that ends with an AND incorrectly.

Copy link

Quality Gate Passed Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
No data about Coverage
0.0% Duplication on New Code

See analysis details on SonarCloud

@RobAndrewHurst
Copy link
Contributor

@dbauszus-glx I have updated the tests for the roles utility module!

@RobAndrewHurst RobAndrewHurst merged commit aed8af6 into GEOLYTIX:main Mar 22, 2024
5 checks passed
@dbauszus-glx dbauszus-glx deleted the role-merge branch June 14, 2024 16:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Code Issues related to the code structure and performance. Feature New feature requests or changes to the behaviour or look of existing application features.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Filter invalid role objects from locale
4 participants