-
Notifications
You must be signed in to change notification settings - Fork 175
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
[Imaging] Sites/Project/subproject filters and permission #7402
Conversation
2913594
to
abc4135
Compare
abc4135
to
d91c88b
Compare
d91c88b
to
46f3650
Compare
@nicolasbrossard Do you have time to review? This PR regroups the imaging modules from #6836 (and is therefore easier to test 😊). |
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.
@laemtl I did not test this yet as it needs to be rebased. I can test it once this is rebased. Just let me know when ready :).
I just looked at the code. See attached comment in my review.
]; | ||
$this->fieldOptions = [ | ||
|
||
'site' => $this->getSiteOptions(false, []), |
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.
Why is the array of getSiteOptions empty here? Or is it using access_all_profiles
by default?
Is there no module permissions for imaging_qc? I don't know very well that module or what should be its behaviour in terms of permissions. @ridz1208, do you know?
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.
@cmadjar Rebased !
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.
And yes, getOptions have default values:
public function getSiteOptions(
bool $numeric_keys = true,
array $allsitesPermission = ['access_all_profiles'],
bool $study_site = true
)
$allsitesPermission is empty here because there is no allsitesPermission for this module (see my comment in the description of the PR about permission imaging_browser_qc
).
46f3650
to
5f3b11e
Compare
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.
@laemtl Thank you for the rebase!
Tests done:
- imaging browser: all good 👍
- imaging uploader: on RB data, if my user belongs to project Rye and site Ottawa, I should not have any entries displayed in the table. If I click on the link that redirects to the imaging browser, it tells me (rightfully) that I do not have the permission to access this data so I should not be able to see entries in imaging upload for project Rye, site Ottawa.
- MRI violations: for the site, I see several options: All (including unknown), All (excluding unknown), Unknown and site belonging to user. Shouldn't it be the same for Project?
- imaging QC: in the data table, I can see entries that do not belong to my user's project. For example: my user belongs to site Ottawa and Project Rye but it can see in the data table entries for project Pumpernickel as well.
@laemtl I think the ball is in your court on this one. Let me know when ready for another review. Thanks! |
@cmadjar Your comments are addressed except this one:
Can you add a discussion point for next Loris meeting? |
8dd93d2
to
936665e
Compare
936665e
to
c483ca7
Compare
fe6b0b2
to
1d79899
Compare
539d037
to
4864d1f
Compare
4864d1f
to
9b14015
Compare
This PR:
Note:
The permission
imaging_browser_qc
was removed from imaging_qc.class.inc. This permission is not documented for this module and permission's description refers to theimaging_browser
module. Can a senior developer confirm if documentation needs to be updated instead?To test
Resolves #6691
Related to #6829
Replace #6729