feat: Enhance feature segment permissions with tag validation #5192
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎ 3 Skipped Deployments
|
|
@pre-commit-ci[bot] is attempting to deploy a commit to the Flagsmith Team on Vercel. A member of the Team first needs to authorize it. |
francescolofranco
requested review from
matthewelwell
and removed request for
a team
Uffizzi Ephemeral Environment
|
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #5192 +/- ##
==========================================
+ Coverage 97.49% 97.50% +0.01%
==========================================
Files 1223 1225 +2
Lines 42581 42723 +142
==========================================
+ Hits 41516 41659 +143
+ Misses 1065 1064 -1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Thanks @francescolofranco. I've added a couple of suggestions for code improvements, but on the whole, I think it looks good. I've also approved the linked PR in the flagsmith-common repository which we'll need to release and bump the version of here before release.
Regarding the RBAC repository, I think there are no changes actually needed. Your comment about testing though is a good one, I'm not sure how we've handled this so far - perhaps @gagantrivedi knows? Based on the testing in the RBAC repo though, I suggest we can get away with the testing you've already added here @francescolofranco. Perhaps, if we wanted to go slightly further, we could mock the call to the function in the rbac_wrapper module and verify that the tags are being correctly passed?
There was a problem hiding this comment.
I'm interested in knowing how do you feel about these types of tests -> d0aca97 ?
I know this is a diversion from the usual way of testing in this project, so I'm ok if you decide to discard this commit.
If you find those useful, I can also cover the CreateSegmentOverridePermissions, just let me know 🙏🏻
Thanks for submitting a PR! Please check the boxes below:
docs/if required so people know about the feature!Changes
This PR partially enables a check against tags for the management of segments overrides. Testing is limited as this would require the settings.IS_RBAC_INSTALLED to be true, forcing it in tests causes an obvious error.
What remains to do is some additional testing, I think.
How did you test this code?
Added a couple of automated tests, which are not really checking much of the logic, since rbac repo is needed for this.
More info
I'm afraid this PR is only partial as to fully complete this work I would need access to the private rbac repo. This PR also requires the addition of the MANAGE_SEGMENT_OVERRIDES permission to the TAG_SUPPORTED_PERMISSIONS in the common repo. PR -> Flagsmith/flagsmith-common#14