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

Feature/erssup 80338 #1824

Open
wants to merge 5 commits into
base: develop
Choose a base branch
from
Open

Conversation

francisco-videira-nhs
Copy link
Contributor

Summary

  • ✨ New Feature

Reviews Required

  • Dev
  • Test
  • Tech Author
  • Product Owner

Review Checklist

ℹ️ This section is to be filled in by the reviewer.

  • I have reviewed the changes in this PR and they fill all or part of the acceptance criteria of the ticket, and the code is in a mergeable state.
  • If there were infrastructure, operational, or build changes, I have made sure there is sufficient evidence that the changes will work.
  • I have ensured the changelog has been updated by the submitter, if necessary.

@francisco-videira-nhs francisco-videira-nhs requested a review from a team as a code owner October 16, 2024 13:32
Copy link
Contributor

@nhsd-jack-wainwright nhsd-jack-wainwright left a comment

Choose a reason for hiding this comment

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

Looks good to me, just a few minor comments 🙂. I think it's probably worth including a sandbox test for both the new retrieveBinary and retrieveBinaryHelper endpoints also.

const objectStore = "/ObjectStore/RetrieveBinary/d497bbe3-f88b-45f1-b3d4-9c563e4c0f5f";
const location = url.split('/FHIR')[0] + objectStore;

if (uuid === '704c3791-0873-45e9-9a04-b51996f8d93f' && request.method === 'get') {
Copy link
Contributor

Choose a reason for hiding this comment

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

I think it's worth also supporting a file ID utilising the legacy ID format here also. It might also be worth supporting any UUID or legacy ID here, just always returning the same file.

Makefile Outdated
@@ -29,7 +29,7 @@ clean:
publish: clean copy-examples
mkdir -p build
npm run publish 2> /dev/null
poetry run python scripts/validate_oas_examples.py
# poetry run python scripts/validate_oas_examples.py
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think this should be commented out?

const filePath = `./r4/requestUrlForFileDownload/${filename}`
const responseCode = 200

if (uuid === 'd497bbe3-f88b-45f1-b3d4-9c563e4c0f5f') {
Copy link
Contributor

Choose a reason for hiding this comment

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

As highlighted above I wonder if it's worth supporting any ID in the legacy or UUID format here?

required: true
schema:
type: string
example: 'TODO Do we want to provide an example?'
Copy link
Contributor

Choose a reason for hiding this comment

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

I wonder if it's simpler to not include an example here just so clients to make any assumptions about the URLs format?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants