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.

Sign up for GitHub

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

Jump to bottom

Dicom archive siteproject main #9549

Open
wants to merge 4 commits into
base: main
Choose a base branch
from
Open

Dicom archive siteproject main #9549

wants to merge 4 commits into from

Conversation

ridz1208
Copy link
Collaborator

@ridz1208 ridz1208 commented Feb 4, 2025 •
edited
Loading

Brief summary of changes

This PR adds advanced site and project permissions to the dicom archive. It is designed to be fully backwards compatible based on the useImagingSiteProjectPermissions configuration setting. Here is how the permissions are designed to work

  • If useImagingSiteProjectPermissions is disabled (status-quo)
    • A user with dicom_archive_allsites can see all the data (DEFAULT)
    • A user with own sites can only see data at their own sites (can not see data with no session ID)
    • A user with own sites and nosessionid permission can see their site data + dicoms not associated to a session
    • A user can always see DICOMS of scans they uploaded
  • If useImagingSiteProjectPermissions is enabled
    • A user with all sites permissions can see all the data from all sites as long as the projectID is defined and they are affiliated with that project (null sessions WILL NOT SHOW)
    • A user with own sites, same logic, only projects they are affiliated with (NO NULL sessions)
    • A user with either site permissions + nosessionID permission can see respectively the same as above + DICOMS not affiliated to a sessions
    • A user can always see DICOMS they have uploaded

TO BE NOTED: The core of this PR is the work done in the dicom_archive::getDataProvisionerWithFilters() function. The code there could have been made slightly more concise but I wrote it in that way because I would like it to eventually become the DEFAULT dataframework filtering logic since it accounts for most if not all usecases (replacing the current UserSiteMAtchOrHasAnyPermissions). It defines flags that I would like to make standard going forward like dataSessionCanBeNull where multiple module seem to lack a session related to the resource.

TODO:

  • Expand Readme
  • complete test plan
  • add SQL patch

Testing instructions (if applicable)

  1. Follow test plan

Link(s) to related issue(s)

  • Resolves # (Reference the issue this fixes, if any.)

@ridz1208 ridz1208 added Release: Add to release notes PR whose changes should be highlighted in the release notes Category: Feature PR or issue that aims to introduce a new feature Language: SQL PR or issue that update SQL code Priority: High PR or issue should be prioritised over others for review and testing State: Needs documentation PR that needs a more exhaustive documentation to proceed Language: PHP PR or issue that update PHP code Project: C-BIG Issue or PR related to the C-BIG project Difficulty: Medium PR or issue that require a moderate effort or expertise to implement, review, or test Area: Imaging PR or issue related to imaging Module: dicom_archive PR or issue related to dicom_archive module labels Feb 4, 2025
ridz1208
ridz1208 commented Feb 4, 2025
View reviewed changes
php/libraries/DataFrameworkMenu.class.inc Outdated
Comment on lines 139 to 150
// Check if resources with null should be included in the result set
if (!empty($this->nullProjectPermissionNames())) {
$provisioner = $provisioner->filter(
new \LORIS\Data\Filters\NullProjectOrUserProjectMatch(
$this->nullProjectPermissionNames()
)
);
} else {
$provisioner = $provisioner->filter(
new \LORIS\Data\Filters\UserProjectMatch()
);
}
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

changes should be removed as module defines its own version of this function

@ridz1208 ridz1208 mentioned this pull request Feb 4, 2025
Open
@ridz1208 ridz1208 force-pushed the dicom_archive_siteproject_main branch 5 times, most recently from e4149f6 to 9ce3a51 Compare February 5, 2025 19:29
@ridz1208
[dicom_archive] Site&Project check with config setting and permission
01cc574 
DA refactor

undo changes dataframework

Update dicom_archive.class.inc

Update viewdetails.class.inc

Update viewdetails.class.inc

double session
@ridz1208 ridz1208 force-pushed the dicom_archive_siteproject_main branch from 6704ee9 to 01cc574 Compare February 5, 2025 20:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
Area: Imaging PR or issue related to imaging Category: Feature PR or issue that aims to introduce a new feature Difficulty: Medium PR or issue that require a moderate effort or expertise to implement, review, or test Language: PHP PR or issue that update PHP code Language: SQL PR or issue that update SQL code Module: dicom_archive PR or issue related to dicom_archive module Priority: High PR or issue should be prioritised over others for review and testing Project: C-BIG Issue or PR related to the C-BIG project Release: Add to release notes PR whose changes should be highlighted in the release notes State: Needs documentation PR that needs a more exhaustive documentation to proceed
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
1 participant
@ridz1208