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

#232: Removed "user" from the request and added it to each of the def… #425

Merged
merged 3 commits into from
Nov 13, 2018

Conversation

brynrhodes
Copy link
Contributor

@brynrhodes brynrhodes commented Nov 7, 2018

Removed "user" from the request and added it to each of the defined hooks as "userId". Updated relevant examples, yaml, and removed the "user" prefetch token defined by the spec.

Fixes #232
Fixes #307

…ined hooks as "userId". Updated relevant examples, yaml, and removed the "user" prefetch token defined by the spec.
@kpshek kpshek temporarily deployed to cds-hooks-docs-pr-425 November 7, 2018 05:07 Inactive
@isaacvetter
Copy link
Member

isaacvetter commented Nov 7, 2018

@brynrhodes - why change from user to userId?

The services in the wild that I've been seeing have been assuming that the element name won't change.

...
But, of course you're just making the user field consistent with the patientId and encounterId elements already defined in context. This will further break some implementations, but it makes sense.

I'm approving.

@kpshek
Copy link
Contributor

kpshek commented Nov 7, 2018

@brynrhodes - why change from user to userId?

Thank you for catching this @isaacvetter. I actually missed that when reviewing. I'm not opposed to the renaming but we should capture the reasoning here for posterity.

@brynrhodes
Copy link
Contributor Author

Yeah, I debated leaving it as user but it felt so inconsistent with the other hooks that I opted for the change, on the basis that it's a breaking change anyway, so now would be the time to make it.

@isaacvetter
Copy link
Member

Fixes #307

Copy link
Contributor

@kpshek kpshek left a comment

Choose a reason for hiding this comment

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

Carrying over the comments from #421 now that those changes have been merged in:

@isaacvetter said:

Hey Kevin - I think that this user text strongly suggested that Patient facing use-cases are as supported as Practitioner facing use-cases; which I don't think is true.

While there's nothing stopping CDS Hooks from being used in a patient-facing scenario, it would make sense to communicate to the reader that provider-facing is primary.

Would you be willing to add a small amount of text to indicate this?

@kpshek said:

@isaacvetter - Can you suggest something concrete?

Our prior documentation referenced the Patient and RelatedPerson resources -- both of which are not providers. The name of this field is user (or soon to be userId) which further reinforces the expanded intention of this field.

Should this type of clarification be best specified in each hook? For instance, medication-prescribe should probably always be a Practitioner id rather than something more broad, right?

@brynrhodes said:

@kpshek @isaacvetter , I agree with Kevin's suggestion that this should be specified more concretely in the hook, I'm happy to add that to the changes there.

@kpshek kpshek merged commit b437b01 into master Nov 13, 2018
@kpshek kpshek deleted the issue/232-move-user-to-context branch November 13, 2018 22:00
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.

None yet

3 participants