Loris icon indicating copy to clipboard operation
Loris copied to clipboard

Published 20 hours ago • verified publisher icon aces

[API] Add Project endpoint "Dicoms"

Open spell00 opened this issue 4 years ago • 17 comments

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)

  • Resolves #6774

spell00 avatar Jun 27 '20 00:06 spell00

@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.

christinerogers avatar Aug 03 '20 14:08 christinerogers

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

spell00 avatar Aug 20 '20 07:08 spell00

it is now working again, including the changes requested

spell00 avatar Aug 21 '20 21:08 spell00

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

christinerogers avatar Aug 24 '20 21:08 christinerogers

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

christinerogers avatar Aug 27 '20 19:08 christinerogers

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

spell00 avatar Sep 01 '20 14:09 spell00

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 avatar Sep 01 '20 20:09 spell00

@spell00 is this ready for re-review? it's not clear if you're still working on resolving the comment you added.

christinerogers avatar Oct 29 '20 23:10 christinerogers

@christinerogers It is ready for review.

Earlier, I was trying to point out that in the changed files, there is indications that I removed the last line in some files. I did not remove the last line and I can't get rid of the indication there is a change. I was trying to get things faster, I am expecting having a comment about these.

spell00 avatar Oct 30 '20 02:10 spell00

blocked by #7101

spell00 avatar Nov 02 '20 20:11 spell00

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

spell00 avatar Nov 05 '20 07:11 spell00

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

cmadjar avatar Jun 08 '21 13:06 cmadjar

@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?

ridz1208 avatar Jul 06 '21 17:07 ridz1208

@xlecours @cmadjar just rebased the PR

ridz1208 avatar Jul 06 '21 17:07 ridz1208

@xlecours this one is back in your court

ridz1208 avatar Jul 07 '21 17:07 ridz1208

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

cmadjar avatar Aug 10 '21 13:08 cmadjar

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 avatar Nov 29 '22 17:11 xlecours

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

ridz1208 avatar Jul 02 '24 13:07 ridz1208

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

cmadjar avatar Jul 04 '24 14:07 cmadjar