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

NPA-3181: update post questionnaire oas #94

Merged
merged 10 commits into from
Jul 25, 2024

Conversation

Stoods-hd
Copy link
Contributor

Pull Request Checklist

Ticket Link

https://nhsd-jira.digital.nhs.uk/browse/NPA-3181

Description/Change Summary

Updated the OAS spec to reflect the new data dictionary and FHIR specification

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.

Copy link

This branch is work on a ticket in the NHS Digital NPA JIRA Project. Here's a handy link to the ticket:

NPA-3181

miiisterjim

This comment was marked as outdated.

Copy link
Contributor

@JackPlowman JackPlowman 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. I've got a couple of comments I would like to be answered. But happy to approve if necessary.

Copy link
Contributor

@miiisterjim miiisterjim 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. Some cosmetic bits n bobs

One standout thing, I THINK oneOf only enforces the shape of objects and that there is no more than one, it doesn't enforce "requiredness". So I think we need a smattering of required's adding throughout. In particular questionnaire, subject and source (whilst not mandated in the QuestionnaireResponse schema, we need them, so should be mandating those and others for our own context). Unless we intend to set authored and status server-side, then these will need to be mandated too (perhaps it does make more sense to set those two server-side, but that needs capturing in a ticket somewhere obvs if not already)

Copy link
Contributor

@chris-young-12-nhs chris-young-12-nhs left a comment

Choose a reason for hiding this comment

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

Have responded to other reviewers questions. Think these need to be resolved before this is good to go. Also as per @JackPlowman's comment. We want to hold off merging until we have updated the tests to work with the new spec.

Copy link

This branch is work on a ticket in the NHS Digital NPA JIRA Project. Here's a handy link to the ticket:

NPA-3181

@Stoods-hd Stoods-hd dismissed stale reviews from chris-young-12-nhs and miiisterjim July 22, 2024 07:32

All comments actioned

Copy link
Contributor

@miiisterjim miiisterjim left a comment

Choose a reason for hiding this comment

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

Some tweaks please. I'd suggest raising tickets (or ensuring we have explicit acceptance criteria) to capture the conditional mandatory demographics for patient & proxy

Thanks!

Copy link

This branch is work on a ticket in the NHS Digital NPA JIRA Project. Here's a handy link to the ticket:

NPA-3181

Copy link

This branch is work on a ticket in the NHS Digital NPA JIRA Project. Here's a handy link to the ticket:

NPA-3181

@Stoods-hd Stoods-hd dismissed miiisterjim’s stale review July 22, 2024 13:38

All suggested changes made

Copy link
Contributor

@chris-young-12-nhs chris-young-12-nhs left a comment

Choose a reason for hiding this comment

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

LGTM based on @JackPlowman & @miiisterjim's feedback and actions from @Stoods-hd

Copy link

This branch is work on a ticket in the NHS Digital NPA JIRA Project. Here's a handy link to the ticket:

NPA-3181

Copy link

This branch is work on a ticket in the NHS Digital NPA JIRA Project. Here's a handy link to the ticket:

NPA-3181

Copy link

This branch is work on a ticket in the NHS Digital NPA JIRA Project. Here's a handy link to the ticket:

NPA-3181

Copy link

This branch is work on a ticket in the NHS Digital NPA JIRA Project. Here's a handy link to the ticket:

NPA-3181

@chris-young-12-nhs chris-young-12-nhs added documentation Improvements or additions to documentation enhancement New feature or request and removed do not merge labels Jul 25, 2024
Copy link
Contributor

@chris-young-12-nhs chris-young-12-nhs left a comment

Choose a reason for hiding this comment

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

@chris-young-12-nhs chris-young-12-nhs merged commit ca29242 into master Jul 25, 2024
9 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Improvements or additions to documentation enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants