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.

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

[API] Add Project endpoint "Dicoms" #6775

Closed

Conversation

spell00
Copy link
Contributor

@spell00 spell00 commented Jun 27, 2020

Brief summary of changes

This Pull Request adds an endpoint at /projects/<project>/dicoms. It returns a complete list of DICOM files contained in a project (and other pertinent informations)

Testing instructions

Go to https://<hostname>/api/v0.0.3/projects/rye/dicoms in a browser. The page should return a JSON in a string with information on all DICOM contained in the project

Link(s) to related issue(s)

@spell00 spell00 added the State: Needs work PR awaiting additional work by the author to proceed label Jun 29, 2020
@spell00 spell00 force-pushed the 2020-06-23-AddEndpointProjectDicoms branch from d93a9bf to 50d007c Compare June 30, 2020 16:46
@spell00 spell00 added Area: API PR or issue related to the API Category: Feature PR or issue that aims to introduce a new feature and removed State: Needs work PR awaiting additional work by the author to proceed labels Jun 30, 2020
@spell00 spell00 requested a review from xlecours June 30, 2020 19:29
@spell00 spell00 changed the base branch from master to main July 23, 2020 14:23
@christinerogers
Copy link
Contributor

@spell00 Is this PR up to date?
If so, @xlecours do you have time to review this? if not let me know and I'll find someone perhaps Cecile.

@spell00 spell00 force-pushed the 2020-06-23-AddEndpointProjectDicoms branch from 91c389d to 976796e Compare August 3, 2020 19:32
cmadjar
cmadjar previously requested changes Aug 3, 2020
Copy link
Collaborator

@cmadjar cmadjar left a comment

Choose a reason for hiding this comment

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

@spell00 Nice work!

See my comment on the query. Let me know if I missed something or misunderstood the goal of the endpoint. Thanks!

*
* @param array $row An array of image propeties
*/
public function __construct(array $row)
Copy link
Contributor

Choose a reason for hiding this comment

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

That constructor is never called.
PDO:statement fetchall instanciate the objects and set their values using __set.

modules/api/php/endpoints/project/dicoms.class.inc Outdated Show resolved Hide resolved
@spell00 spell00 force-pushed the 2020-06-23-AddEndpointProjectDicoms branch from 66c17ec to fe1002e Compare August 20, 2020 06:36
@spell00
Copy link
Contributor Author

spell00 commented Aug 20, 2020

now I get {"Dicoms":{}} when trying to access <host>/api/v0.0.3/projects/rye/dicoms

@spell00 spell00 requested review from xlecours and cmadjar August 21, 2020 21:54
@spell00
Copy link
Contributor Author

spell00 commented Aug 21, 2020

it is now working again, including the changes requested

@christinerogers
Copy link
Contributor

@xlecours and @kongtiaowang - would you have a few minutes to (re)review this PR this week?

@christinerogers
Copy link
Contributor

@spell00 this seems like a minor change request -- can you address it and move this forward today?

@spell00
Copy link
Contributor Author

spell00 commented Sep 1, 2020

ok @christinerogers , I'll take care of it today

@spell00
Copy link
Contributor Author

spell00 commented Sep 1, 2020

This new endpoint is not in the specs for v0.0.3, so the changes suggested in this PR need to be added to the API v0.0.4 initiated in PR #6944, which is in progress.

@spell00 spell00 added the State: Blocked PR or issue awaiting an external event such as the merge or another PR to proceed label Sep 1, 2020
@spell00 spell00 added the State: Blocked PR or issue awaiting an external event such as the merge or another PR to proceed label Nov 2, 2020
@spell00
Copy link
Contributor Author

spell00 commented Nov 2, 2020

blocked by #7101

@spell00 spell00 force-pushed the 2020-06-23-AddEndpointProjectDicoms branch from 35d1182 to e7e4cb6 Compare November 4, 2020 20:41
@spell00 spell00 removed the State: Blocked PR or issue awaiting an external event such as the merge or another PR to proceed label Nov 5, 2020
@spell00
Copy link
Contributor Author

spell00 commented Nov 5, 2020

@xlecours @driusan It is now passing Travis and ready for final review

@spell00 spell00 requested a review from driusan November 5, 2020 16:37
@spell00 spell00 requested a review from xlecours November 6, 2020 06:31
Copy link
Contributor

@xlecours xlecours left a comment

Choose a reason for hiding this comment

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

I wouldn't mind having @cmadjar backing on this. Are the properties of the Dicoms items ok?

modules/api/docs/LorisRESTAPI_v0.0.4-dev.md Show resolved Hide resolved
@spell00 spell00 requested a review from xlecours November 11, 2020 14:12
@cmadjar
Copy link
Collaborator

cmadjar commented Jun 8, 2021

@xlecours can you take this PR over please? Thank you :)

@cmadjar cmadjar assigned xlecours and unassigned spell00 Jun 8, 2021
@ridz1208 ridz1208 dismissed stale reviews from cmadjar and xlecours July 6, 2021 17:30

outdated

@ridz1208
Copy link
Collaborator

ridz1208 commented Jul 6, 2021

@cmadjar it seems like @xlecours 's next to latest review approved the changes and asked you to take a second look at it. I think this one is also in your court in that case

from @xlecours

I wouldn't mind having @cmadjar backing on this. Are the properties of the Dicoms items ok?

Squashed commit of the following:

commit 817b148
Author: Simon Pelletier <simonjpelletier@gmail.com>
Date:   Mon Nov 9 15:17:49 2020 -0500

    adding comment about since parameter

commit f4892d6
Author: Simon Pelletier <simonjpelletier@gmail.com>
Date:   Fri Nov 6 13:12:55 2020 -0500

    change classname in construct to include namespace

commit b4f7f60
Author: Simon Pelletier <simonjpelletier@gmail.com>
Date:   Thu Nov 5 14:40:50 2020 -0500

    corrections

commit 65eefba
Author: Simon Pelletier <simonjpelletier@gmail.com>
Date:   Thu Nov 5 00:12:31 2020 -0500

    fixing endpoint name

commit 226230e
Author: Simon Pelletier <simonjpelletier@gmail.com>
Date:   Thu Nov 5 00:00:42 2020 -0500

    remove old files

commit cc79194
Author: Simon Pelletier <simonjpelletier@gmail.com>
Date:   Wed Nov 4 16:35:17 2020 -0500

    CHANGELOG was not updated

commit ec0f492
Author: Simon Pelletier <simonjpelletier@gmail.com>
Date:   Wed Nov 4 16:34:23 2020 -0500

    CHANGELOG was not updated

commit e8c4ad2
Author: Simon Pelletier <simonjpelletier@gmail.com>
Date:   Wed Nov 4 16:23:51 2020 -0500

    updating doc update

commit 6f34893
Author: Simon Pelletier <simonjpelletier@gmail.com>
Date:   Wed Nov 4 16:05:12 2020 -0500

    getting out of rebase hell and solving conflicts

commit 2350ea0
Author: Simon Pelletier <simonjpelletier@gmail.com>
Date:   Wed Nov 4 15:46:16 2020 -0500

    Revert "rollback to v1.x"

    This reverts commit d9db7d7.

commit 48fdddd
Author: Simon Pelletier <simonjpelletier@gmail.com>
Date:   Wed Nov 4 15:46:08 2020 -0500

    Revert "update"

    This reverts commit 211d96d.

commit 2fbf050
Author: Simon Pelletier <simonjpelletier@gmail.com>
Date:   Wed Nov 4 15:45:59 2020 -0500

    Revert "Revert "update""

    This reverts commit 0022745.

commit c1c90fc
Author: Simon Pelletier <simonjpelletier@gmail.com>
Date:   Wed Nov 4 15:45:33 2020 -0500

    Revert "removing change to travis"

    This reverts commit e7e4cb6.

commit e7e4cb6
Author: Simon Pelletier <simonjpelletier@gmail.com>
Date:   Wed Nov 4 15:39:04 2020 -0500

    removing change to travis

commit 0022745
Author: Simon Pelletier <simonjpelletier@gmail.com>
Date:   Wed Nov 4 15:36:07 2020 -0500

    Revert "update"

    This reverts commit 0e89111.

commit 211d96d
Author: Simon Pelletier <simonjpelletier@gmail.com>
Date:   Tue Nov 3 13:01:13 2020 -0500

    update

commit d9db7d7
Author: Simon Pelletier <simonjpelletier@gmail.com>
Date:   Tue Nov 3 11:48:04 2020 -0500

    rollback to v1.x

commit f8aae97
Author: Simon Pelletier <simonjpelletier@gmail.com>
Date:   Mon Nov 2 14:16:09 2020 -0500

    Changing classes names ProjectDicoms

commit 71203c1
Author: Simon Pelletier <simonjpelletier@gmail.com>
Date:   Mon Nov 2 13:37:51 2020 -0500

    v0.0.4-dev not v0.0.4

commit e125284
Author: Simon Pelletier <simonjpelletier@gmail.com>
Date:   Thu Oct 29 16:11:27 2020 -0400

    LorisApiAuthenticatedTest.php had unnecessary modifications

commit 92b9517
Author: Simon Pelletier <simonjpelletier@gmail.com>
Date:   Thu Oct 29 16:09:58 2020 -0400

    LorisApiAuthenticatedTest.php had unnecessary modifications

commit 8c9df99
Author: Simon Pelletier <simonjpelletier@gmail.com>
Date:   Thu Oct 29 11:31:21 2020 -0400

    Trying to readd newline at end of file because git complains

commit 2f9359a
Author: Simon Pelletier <simonjpelletier@gmail.com>
Date:   Thu Oct 29 11:30:03 2020 -0400

    Trying to readd newline at end of file because git complains

commit ba45736
Author: Simon Pelletier <simonjpelletier@gmail.com>
Date:   Thu Oct 29 10:32:18 2020 -0400

    fixing things

commit 2333bed
Author: Simon Pelletier <simonjpelletier@gmail.com>
Date:   Fri Sep 18 12:17:30 2020 -0400

    correct integration test typo

commit 682e337
Author: Simon Pelletier <simonjpelletier@gmail.com>
Date:   Fri Sep 18 11:11:22 2020 -0400

    [API] Corrects the tests for the new Projects Dicoms endpoint

commit 6dd799a
Author: Simon Pelletier <simonjpelletier@gmail.com>
Date:   Fri Sep 4 15:12:43 2020 -0400

    Adds the integration tests for \Project Dicoms endpoint

commit d6ca120
Author: Simon Pelletier <simonjpelletier@gmail.com>
Date:   Fri Aug 21 17:47:57 2020 -0400

    [API] weird new file removed

commit d36d982
Author: Simon Pelletier <simonjpelletier@gmail.com>
Date:   Fri Aug 21 17:43:26 2020 -0400

    [API] Fix Travis again

commit d3bdd67
Author: Simon Pelletier <simonjpelletier@gmail.com>
Date:   Mon Aug 3 15:29:55 2020 -0400

    [API] Changed toJSON to jsonSerialize and resored strict_types declaration

commit 4b70446
Author: Simon Pelletier <simonjpelletier@gmail.com>
Date:   Mon Jun 29 13:30:08 2020 -0400

    [API] In \project dicoms endpoints: Corrected travis errors

commit 6606b4f
Author: Simon Pelletier <simonjpelletier@gmail.com>
Date:   Tue Jun 23 16:11:12 2020 -0400

    [API] Creation of endpoint /project/{project}/dicoms

commit 63be090
Author: Simon Pelletier <simonjpelletier@gmail.com>
Date:   Tue Jun 23 16:08:40 2020 -0400

    [API] Creation of endpoint /project/{project}/dicoms

commit 5f8c88d
Author: Simon Pelletier <simonjpelletier@gmail.com>
Date:   Tue Sep 1 17:56:07 2020 -0400

    [API] adds v0.0.4 in the supported versions of the /candidate dicom endpoint

commit 97ef8f6
Author: Simon Pelletier <simonjpelletier@gmail.com>
Date:   Tue Sep 1 16:27:15 2020 -0400

    [API] correct travis errors

commit 4a67189
Author: Simon Pelletier <simonjpelletier@gmail.com>
Date:   Tue Sep 1 15:24:01 2020 -0400

    [API] changes version in new spec document and removes other errors

commit fd87690
Author: Simon Pelletier <simonjpelletier@gmail.com>
Date:   Tue Sep 1 15:12:35 2020 -0400

    initial commit of transition to api v0.0.4

commit 002e090
Author: Simon Pelletier <simonjpelletier@gmail.com>
Date:   Tue Sep 1 14:58:08 2020 -0400

    initial commit of transition to api v0.0.4

commit a55b304
Author: Simon Pelletier <simonjpelletier@gmail.com>
Date:   Tue Sep 1 13:04:02 2020 -0400

    [API] Candidates are now filtered by allowed project ids

commit fe3cb93
Author: Simon Pelletier <simonjpelletier@gmail.com>
Date:   Tue Sep 1 10:34:35 2020 -0400

    [API] Add getProjectID to candidatesrow model

commit b192d15
Author: Simon Pelletier <simonjpelletier@gmail.com>
Date:   Thu Oct 29 10:32:18 2020 -0400

    fixing things

commit 806e9b5
Author: Simon Pelletier <simonjpelletier@gmail.com>
Date:   Fri Sep 18 12:35:46 2020 -0400

    Announce of the new version of the API in CHANGELOG.md

commit 9eb937e
Author: Simon Pelletier <simonjpelletier@gmail.com>
Date:   Fri Sep 4 16:22:37 2020 -0400

    Addresses comment to clean code

commit fdef2d8
Author: Simon Pelletier <simonjpelletier@gmail.com>
Date:   Wed Sep 2 14:15:28 2020 -0400

    [API] Fixes Travis checks in projectdicomsobject

commit 9c1cf05
Author: Simon Pelletier <simonjpelletier@gmail.com>
Date:   Wed Sep 2 14:06:22 2020 -0400

    [API] Adds Projects \Dicoms endpoint to the specs and corrects FileName to be displayed

commit 51b1169
Author: Simon Pelletier <simonjpelletier@gmail.com>
Date:   Wed Sep 2 13:55:30 2020 -0400

    Makes it available only in v0.0.4 and Path changed to filename

commit 83903b7
Author: Simon Pelletier <simonjpelletier@gmail.com>
Date:   Tue Sep 1 11:12:49 2020 -0400

    [API] Removes useless condition in _hasAccess function

commit 40afcfc
Author: Simon Pelletier <simonjpelletier@gmail.com>
Date:   Tue Aug 25 15:07:15 2020 -0400

    [API] Adding checkign user permission

commit 3cd3a66
Author: Simon Pelletier <simonjpelletier@gmail.com>
Date:   Sun Aug 23 14:28:41 2020 -0400

    Solves Possibly zero references to use statement in travis

commit 0266890
Author: Simon Pelletier <simonjpelletier@gmail.com>
Date:   Fri Aug 21 17:53:54 2020 -0400

    [API] correct mistakes

commit 42a8d5f
Author: Simon Pelletier <simonjpelletier@gmail.com>
Date:   Fri Aug 21 17:47:57 2020 -0400

    [API] weird new file removed

commit 27c00c1
Author: Simon Pelletier <simonjpelletier@gmail.com>
Date:   Fri Aug 21 17:46:26 2020 -0400

    [API] weird new file removed

commit 9cd209d
Author: Simon Pelletier <simonjpelletier@gmail.com>
Date:   Fri Aug 21 17:43:26 2020 -0400

    [API] Fix Travis again

commit a961386
Author: Simon Pelletier <simonjpelletier@gmail.com>
Date:   Fri Aug 21 17:38:09 2020 -0400

    [API] Restroring useless declare(strict_types=1)

commit 069b167
Author: Simon Pelletier <simonjpelletier@gmail.com>
Date:   Fri Aug 21 17:33:35 2020 -0400

    [API] Remove useless stuff

commit 601a5e6
Author: Simon Pelletier <simonjpelletier@gmail.com>
Date:   Fri Aug 21 17:16:25 2020 -0400

    [API] Project Dicoms endpoint works with ObjectProvisioner

commit 32d997f
Author: Simon Pelletier <simonjpelletier@gmail.com>
Date:   Thu Aug 20 02:34:15 2020 -0400

    [API] Chenged to ProjectDicomsObjectProvisioner

commit efedeef
Author: Simon Pelletier <simonjpelletier@gmail.com>
Date:   Tue Aug 4 16:37:26 2020 -0400

    [API] redeclare strict types and simplify projectdicomsrowprovisioner

commit 39b4a26
Author: Simon Pelletier <simonjpelletier@gmail.com>
Date:   Tue Aug 4 16:08:55 2020 -0400

    [API] redeclare strict types and simplify projectdicomsrowprovisioner

commit 756f93e
Author: Simon Pelletier <simonjpelletier@gmail.com>
Date:   Mon Aug 3 15:29:55 2020 -0400

    [API] Changed toJSON to jsonSerialize and resored strict_types declaration

commit 93f2c1d
Author: Simon Pelletier <simonjpelletier@gmail.com>
Date:   Mon Aug 3 14:10:58 2020 -0400

    [API] Corrections to pass Travis checks

commit 108121d
Author: Simon Pelletier <simonjpelletier@gmail.com>
Date:   Wed Jul 1 14:41:50 2020 -0400

    [API] Added information on tarchive

commit 6376f72
Author: Simon Pelletier <simonjpelletier@gmail.com>
Date:   Tue Jun 30 15:19:59 2020 -0400

    [API] Fix other typos

commit 5ac0b5b
Author: Simon Pelletier <simonjpelletier@gmail.com>
Date:   Tue Jun 30 15:16:42 2020 -0400

    [API] Fix other typos

commit 3c7c1e6
Author: Simon Pelletier <simonjpelletier@gmail.com>
Date:   Tue Jun 30 12:41:15 2020 -0400

    [API] added endpoints/project/dicoms.class.inc that was left behind

commit a50d83c
Author: Simon Pelletier <simonjpelletier@gmail.com>
Date:   Mon Jun 29 13:30:08 2020 -0400

    [API] In \project dicoms endpoints: Corrected travis errors

commit 97beb63
Author: Simon Pelletier <simonjpelletier@gmail.com>
Date:   Mon Jun 29 11:53:03 2020 -0400

    [API] In \project dicoms endpoints: Corrected travis errors

commit 26ef029
Author: Simon Pelletier <simonjpelletier@gmail.com>
Date:   Fri Jun 26 20:05:06 2020 -0400

    [API] New Project /dicoms endpoint

commit 9c51213
Author: Simon Pelletier <simonjpelletier@gmail.com>
Date:   Tue Jun 23 16:11:12 2020 -0400

    [API] Creation of endpoint /project/{project}/dicoms

commit 8bdad92
Author: Simon Pelletier <simonjpelletier@gmail.com>
Date:   Tue Jun 23 16:10:52 2020 -0400

    [API] Creation of endpoint /project/{project}/dicoms

commit 7465fe3
Author: Simon Pelletier <simonjpelletier@gmail.com>
Date:   Tue Jun 23 16:08:40 2020 -0400

    [API] Creation of endpoint /project/{project}/dicoms

commit cf758cd
Author: Simon Pelletier <simonjpelletier@gmail.com>
Date:   Tue Sep 1 13:04:02 2020 -0400

    [API] Candidates are now filtered by allowed project ids

commit c3ac7fa
Author: Simon Pelletier <simonjpelletier@gmail.com>
Date:   Tue Sep 1 10:53:09 2020 -0400

    [API] Adding UserProjectMatch to candidates endpoint

commit 1954102
Author: Simon Pelletier <simonjpelletier@gmail.com>
Date:   Tue Sep 1 10:34:35 2020 -0400

    [API] Add getProjectID to candidatesrow model
@ridz1208 ridz1208 force-pushed the 2020-06-23-AddEndpointProjectDicoms branch from 817b148 to 8a68dfa Compare July 6, 2021 17:42
@ridz1208
Copy link
Collaborator

ridz1208 commented Jul 6, 2021

@xlecours @cmadjar just rebased the PR

Copy link
Collaborator

@cmadjar cmadjar left a comment

Choose a reason for hiding this comment

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

@ridz1208 thank you for pinging me on this, I did not see the previous comment. See attached minor suggestions. Let me know your thoughts.

"DicomArchiveID": "1.2.840.113745.101000.1022000.39911.6153.5242769",
"Archive": "2018/DCM_2018-04-20_ImagingUpload-13-42-zcoZR0.tar",
"Source": "/tmp/ImagingUpload-13-42-zcoZR0",
"FileName": "DCM_2018-04-20_ImagingUpload-13-42-zcoZR0.tar"
Copy link
Collaborator

Choose a reason for hiding this comment

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

is there any reason why the following fields are not returned?

AcquisitionCount
DicomFileCount
ScannerManufacturer
ScannerModel
ScannerSoftware
ScannerSerialNumber

Those are fields I could see as being informative to the user accessing the DICOM archives.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Also, I notice that in the JSON response, keys have all kinds of different convention. I would tend to make them consistent (either stick to CamelCase, or _ separated values, consistent casing etc...).

Sorry, my brain gets easily distracted with inconsistencies ;).

t.TarchiveID as tarchiveid,
t.DicomArchiveID as DicomArchiveID,
t.ArchiveLocation as Archive,
t.SourceLocation as Source
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
t.SourceLocation as Source
t.SourceLocation as Source

@ridz1208
Copy link
Collaborator

ridz1208 commented Jul 7, 2021

@xlecours this one is back in your court

@cmadjar
Copy link
Collaborator

cmadjar commented Aug 10, 2021

@xlecours this one is in your court. Should be straightforward though?

@xlecours xlecours added the State: Needs work PR awaiting additional work by the author to proceed label Nov 29, 2022
@xlecours
Copy link
Contributor

This still needs work.

  1. The property names in the response should be case-consistent
  2. I think we should hide : CenterID, tarchiveid and Source since they are only meaningful from within the database of on the filesystem.
  3. There should be a link to the resource (where it can be downloaded)
  4. Documentation should be add in the api specification of the api module (schema.yml)

On a side note: could this be done using /dicom_archive instead?

@xlecours xlecours removed their assignment Jan 3, 2024
@ridz1208
Copy link
Collaborator

ridz1208 commented Jul 2, 2024

@cmadjar assigning to you to bring up at Imaging meeting see if this is still needed

@cmadjar
Copy link
Collaborator

cmadjar commented Jul 4, 2024

Discussed during the imaging call of Jul 4th. It is stale and needs work that will not be done in the short term.

@cmadjar cmadjar closed this Jul 4, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area: API PR or issue related to the API Category: Feature PR or issue that aims to introduce a new feature State: Needs work PR awaiting additional work by the author to proceed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[API] New Project /dicoms endpoint
6 participants