[To Main] Add metadata management role #2506
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #2506 +/- ##
==========================================
+ Coverage 75.78% 75.82% +0.04%
==========================================
Files 597 597
Lines 21334 21333 -1
Branches 1568 1568
==========================================
+ Hits 16167 16175 +8
+ Misses 4913 4904 -9
Partials 254 254
Flags with carried forward coverage won't be shown. Click here to find out more.
|
There was a problem hiding this comment.
I haven't completed review of the tests yet but could you please update the changes to allow users who could edit engagement metadata to be able to perform CRUD operations on them? We want to make sure that many users can still access/modify metadata on individual engagements. Taxons should be restricted to metadata managers.
Please see the ticket or speak with Nat for more information!
| def get(engagement_id): | ||
| """Fetch engagement metadata entries by engagement id.""" | ||
| authorization.check_auth( |
There was a problem hiding this comment.
Levels of abstraction are tricky here, but the ticket still calls for engagement metadata to allow CRUD operations for users who can edit engagements.
I believe it's the metadata_taxon file that will restrict its operations to the MANAGE_METADATA role.
There was a problem hiding this comment.
Thanks, changed it back to use edit engagement. The only change here being allowing team members assigned to the engagement to edit as well.
|
| @@ -131,11 +146,16 @@ class EngagementMetadataById(Resource): | |||
|
|
|||
| @staticmethod | |||
| @cross_origin(origins=allowedorigins()) | |||
| @require_role(VIEW_ENGAGEMENT_ROLES) | |||
| @_jwt.requires_auth | |||
There was a problem hiding this comment.
Are we sure about requiring authorization for this route? What if we're pulling engagement metadata on the frontend for public users?
There was a problem hiding this comment.
I chatted with Vineet about my questions:
- We don't need to allow public access to metadata for engagements yet. Tickets are coming for how we handle metadata on the frontend
- Tests look for user groups which include the new metadata manager role, so we're covered in terms of testing it
| @@ -75,9 +73,16 @@ class EngagementMetadata(Resource): | |||
| @cross_origin(origins=allowedorigins()) | |||
| @API.doc(security='apikey') | |||
| @API.marshal_list_with(metadata_return_model) | |||
| @require_role(VIEW_ENGAGEMENT_ROLES) | |||
| @_jwt.requires_auth | |||
There was a problem hiding this comment.
Will public users still be able to get engagement metadata?
| @@ -182,3 +189,25 @@ def test_delete_engagement_metadata(client, jwt, session): | |||
| content_type=ContentType.JSON.value) | |||
| assert response.status_code == HTTPStatus.NO_CONTENT, (f'Wrong response code; ' | |||
| f'HTTP {response.status_code} -> {response.text}') | |||
|
|
|||
|
|
|||
| def test_get_engagement_metadata_by_id(client, jwt, session): | |||
There was a problem hiding this comment.
Do we have a way to test requests based on roles? It would be nice to have some tests directly related to the work for this ticket. Although I appreciate that you added a couple more general metadata tests
Issue #: https://apps.itsm.gov.bc.ca/jira/browse/DESENG-603
Description of changes:
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of the met-public license (Apache 2.0).