-
Notifications
You must be signed in to change notification settings - Fork 1
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] View Permit Holder API hookup #50
Conversation
@@ -71,10 +71,10 @@ CREATE TABLE physicians ( | |||
address_line_1 VARCHAR(255) NOT NULL, | |||
address_line_2 VARCHAR(255), | |||
city VARCHAR(255) NOT NULL, | |||
province Province NOT NULL, | |||
province Province NOT NULL DEFAULT 'BC', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Upserting a physician was failing because province and status are not specified in EditDoctorInformationModal
so I added default values to db. The default values could have also been specified somewhere else instead but I think BC
and ACTIVE
are sensible defaults to have on the database level so we don't have to specify these values every time we handle physician creation
</Box> | ||
)} | ||
|
||
{medicalInformation.notes && ( | ||
{application.notes && ( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think application.notes
isn't correct here as I believe this refers to notes on the application itself and not notes relating to medical information but I kept it here as a placeholder. There doesn't seem to be a field for that so I'm not sure what should be used here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, this is correct. We don't have medical information notes on a per-application basis. It is best to just leave the application.notes
here, as it is most likely that the notes attached to an application are related to the medical information
d8fd6b4
to
e8a2161
Compare
…/uwblueprint/richmond-centre-for-disability into jb/view-permit-holder-api-hookup
…/uwblueprint/richmond-centre-for-disability into jb/view-permit-holder-api-hookup
Co-authored-by: jennifer tsai <jennifertsai@users.noreply.github.com>
…/uwblueprint/richmond-centre-for-disability into jb/view-permit-holder-api-hookup
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great work guys! And thanks a lot @chrischan325 @angeladietz and @jennifertsai for picking up this ticket. I left some items to resolve. Some of the request changes will break the other files (eg. the moving the types to a tools file rather than leaving it in the page file) so be careful of that. Thanks again y'all :))
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A lot better! A couple of small things and we should be good to go :)
…te for EditDoctorInformationModal, other small changes
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
Notion ticket link
View + Edit Permit Holder API hookup
Implementation description
General TODO list:
EditUserInformationModal
API hookupEditDoctorInformationModal
API hookupapplicantApplicationProcessingHistoryResolver
field resolverNotes
Checklist
[Feature]
,[Improvement]
or[Fix]
,