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

Update API summary header and qc too grep more info #4077

Merged
merged 10 commits into from
Feb 4, 2019
Merged

Update API summary header and qc too grep more info #4077

merged 10 commits into from
Feb 4, 2019

Conversation

cmadjar
Copy link
Collaborator

@cmadjar cmadjar commented Nov 7, 2018

Brief summary of changes

Updates to the API v.0.0.3 on the imaging side:

  • Addition of scanner information to the summary header of the API for a given filename (manufacturer, model, software version, serial number)
  • Addition of the image Caveat flag with the reason for the Caveat flag in the file qc page of the API

Fix to API v0.0.2 on the imaging side:

  • The selected were not queried at the right place. The selected has been part of the files_qcstatus for several LORIS releases so updated the API queries so we can grep the Selected flag (before it returned some hash, now it returns True or False)

This resolves issue...

Did not take the time to create an issue for that. Needed for the phantom VM so I figured I would issue a PR to the LORIS repo at the same time :).

To test this change...

You need imaging data to test this

  • test $API/candidates/$CandID/$VisitLabel/images/$filename/headers (should see ScannerInfo now in addition to everything else that was already in the JSON)
  • test $API/candidates/$CandID/$VisitLabel/images/$filename/qc (make sure the selected value is properly displayed with this PR and that Caveats is added to the JSON)

If this sentence shows up it means I didn't read the instructions!

Yes I read it ;)

@cmadjar cmadjar added Category: Bug PR or issue that aims to report or fix a bug Area: API PR or issue related to the API [branch] minor labels Nov 7, 2018
cmadjar
cmadjar commented Nov 12, 2018
View reviewed changes
Copy link
Collaborator Author

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

@PapillonMcGill Thanks a lot for the suggestions. I believe I applied them all. Ready for another round of review when you are. Thanks!

@cmadjar
Copy link
Collaborator Author

cmadjar commented Nov 12, 2018

@johnsaigle Thank you for your feedback. I believe I addressed all your comments. Please re-review when you have time. Thanks!

PapillonMcGill
PapillonMcGill previously approved these changes Nov 13, 2018
View reviewed changes
@PapillonMcGill PapillonMcGill dismissed their stale review November 13, 2018 13:17

Travis objection for return type

@cmadjar
Copy link
Collaborator Author

cmadjar commented Nov 19, 2018

@driusan @ridz1208 I don't understand why Travis is not happy. Any ideas why?

@driusan
Copy link
Collaborator

driusan commented Nov 19, 2018

The @return starts with a sentence and comment doesn't include the type.

"htdocs/api/v0.0.3-dev/candidates/visits/images/qc/QC.php:94 PhanTypeMismatchDeclaredReturn Doc-block of getImageCaveats contains declared return type \Loris\API\Candidates\Candidate\Visit\Imaging\Qc\an which is incompatible with the return type array declared in the signature"

@cmadjar
Copy link
Collaborator Author

cmadjar commented Nov 19, 2018

@driusan @johnsaigle Cool! Thanks for the clarification. Should be fixed now.

PapillonMcGill
PapillonMcGill previously approved these changes Nov 20, 2018
View reviewed changes
johnsaigle
johnsaigle previously approved these changes Jan 9, 2019
View reviewed changes
driusan
driusan requested changes Jan 9, 2019
View reviewed changes
Copy link
Collaborator

@driusan driusan left a comment

Choose a reason for hiding this comment

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

This doesn't appear to include the documentation update to docs/API/LorisRESTAPI_v0.0.3.md

@johnsaigle johnsaigle added the State: Needs documentation PR that needs a more exhaustive documentation to proceed label Jan 9, 2019
@cmadjar
Copy link
Collaborator Author

cmadjar commented Jan 10, 2019

@driusan Good catch! Thank you for bringing it up. I updated the documentation in the last commit.

@cmadjar cmadjar removed the State: Needs documentation PR that needs a more exhaustive documentation to proceed label Jan 10, 2019
driusan
driusan reviewed Jan 14, 2019
View reviewed changes
docs/API/LorisRESTAPI_v0.0.3.md Outdated Show resolved Hide resolved
docs/API/LorisRESTAPI_v0.0.3.md Outdated Show resolved Hide resolved
cmadjar
cmadjar commented Jan 14, 2019
View reviewed changes
Copy link
Collaborator Author

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

@driusan I modified the documentation according to your comments. Let me know if you want me to actually hardcode 'warn' in the documentation. Figured I would wait for your feedback on that last one. Thanks!

docs/API/LorisRESTAPI_v0.0.3.md Outdated Show resolved Hide resolved
docs/API/LorisRESTAPI_v0.0.3.md Outdated Show resolved Hide resolved
@cmadjar
Copy link
Collaborator Author

cmadjar commented Feb 4, 2019

@driusan A little reminder. I updated the documentation like you asked to. Ready for another review. Thanks!

@driusan driusan merged commit f33c522 into aces:minor Feb 4, 2019
@driusan
Copy link
Collaborator

driusan commented Feb 4, 2019

@cmadjar merged, but please keep in touch with @xlecours to make sure it doesn't get lost in his API module refactoring.

@cmadjar cmadjar mentioned this pull request Feb 4, 2019
Merged
24 tasks
@ridz1208 ridz1208 added this to the 20.3.0 milestone Feb 11, 2019
kchatpar pushed a commit to kchatpar/Loris that referenced this pull request Apr 15, 2019
@cmadjar
Update API summary header and qc to include more info (aces#4077)
955b675 
Updates to the API v.0.0.3 on the imaging side:

- Addition of scanner information to the summary header of the API for a given filename (manufacturer, model, software version, serial number)
- Addition of the image Caveat flag with the reason for the Caveat flag in the file qc page of the API

Fix to API v0.0.2 on the imaging side:

- The selected were not queried at the right place. The selected has been part of the files_qcstatus for several LORIS releases so updated the API queries so we can grep the Selected flag (before it returned some hash, now it returns True or False)
@cmadjar cmadjar deleted the update_api_summary_header_and_qc_too_grep_more_info branch September 24, 2021 19:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@driusan driusan driusan approved these changes

@PapillonMcGill PapillonMcGill PapillonMcGill left review comments

@johnsaigle johnsaigle johnsaigle left review comments

Assignees
No one assigned
Labels
Area: API PR or issue related to the API Category: Bug PR or issue that aims to report or fix a bug
Projects
None yet
Milestone
20.3.0
Development

Successfully merging this pull request may close these issues.
5 participants
@cmadjar @driusan @johnsaigle @PapillonMcGill @ridz1208