-
Notifications
You must be signed in to change notification settings - Fork 8
Per-request permission management #386
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
base: master
Are you sure you want to change the base?
Conversation
* Working version: visibility for groups and users * Added filtering for completed requests * Added translations for the card title * Fixed session errors in test * Added basic test for the per-request ownership * Move ownership-related parts into an external class * Added db migrations, similar to existing ones
|
Thank you for the contribution, will look it next week. |
|
Hi! Any progress on the pull request review? |
Not yet, I didn't have time. I'll keep you updated when it's done. Thanks for your patience |
benoitregamey
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hello!
I’ve tested your PR and everything works perfectly — great job!
From our side, we find this feature interesting and would like to merge it into the main project. However, in its current form, it changes the default behavior of the application, which could be problematic for users who rely on strict ACL configurations.
To be merged, the feature would need to be disabled by default and enabled via a toggle. This could be done either at the application level (through the main settings), or ideally at the process level (for example, with a toggle next to the assigned operators).
What do you think? Would it be possible to implement this change?
Yes, it's possible to hide the controls and the related code under the feature flags. But the schema-reated changes will stay (e.g. many-to-many tables will be in the database, but they will be empty) |
|
Added a feature flag that blocks per-request ownership.
|
benoitregamey
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay, that seems pretty good to me !
Since I'll meet the Extract Users Group next week on Monday, I'll discuss with them if it's okay to merge incoming PR if the default behaviour of the app is not changed.
I keep you updated !
Thanks for the contribution !
* Working version: visibility for groups and users * Added filtering for completed requests * Added translations for the card title * Fixed session errors in test * Added basic test for the per-request ownership * Move ownership-related parts into an external class * Added db migrations, similar to existing ones
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please, rebase your branch onto the master and make all tests pass. FYI, there is now 3000+ tests implemented and integration tests are failing with your PR.
Thank you for your understanding and patience
Issue #379
Purpose: per-request user ownership, make it possible for one operator to make a single request visible to another operator, even if the second operator is not an owner of the parent process.
Behavior: