VED-375 Search Parameter Error #854
Conversation
|
This branch is working on a ticket in the NHS England VED JIRA Project. Here's a handy link to the ticket: VED-375 |
dlzhry2nhs
left a comment
dlzhry2nhs
left a comment
There was a problem hiding this comment.
Couple of changes requested and queries about test cases.
Overall change looks good - in the right direction and the tests are well constructed.
| """ | ||
| # TODO: is disease type a mandatory field? (I assumed it is) | ||
| # i.e. Should we provide a search option for getting Patient's entire imms history? | ||
| if not nhs_number_mod11_check(nhs_number): |
There was a problem hiding this comment.
Can we do something about the above TODOs? One to check with business or product. Think point 1 we can just remove - from the validation it is clearly a mandatory field.
From point 2, I think this might be beyond the scope of our API but it might be worth checking. Once we know, let's remove it because we should try to get rid of comments and TODOs like this in the codebase unless it actually needs action.
There was a problem hiding this comment.
Reason being that legacy comments like this are confusing and introduce uncertainty.
There was a problem hiding this comment.
Can we do anything with the TODOs now that we have clarified with Martin?
|
3 participants
Summary
Add any other relevant notes or explanations here. Remove this line if you have nothing to add.
Reviews Required
Review Checklist
ℹ️ This section is to be filled in by the reviewer.