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

Additional patient validators and tests #277

Merged
merged 16 commits into from
Sep 19, 2024
Merged

Conversation

mbarton
Copy link
Member

@mbarton mbarton commented Sep 18, 2024

  • Port across the existing tests against the patient model to the patient form where the validators run currently
  • Add tests for CSV upload (fixes Add csv upload tests #220)
  • Re-arrange the code that modifies patient fields to follow what (I think) is Django best practice:
    • Run them in to_python on a subclass of field if they normalise the data (eg remove spaces from postcode)
    • Run them in clean_XXXX if they only validate a single field
    • Run them in clean if they need to validate a field using the values from others (eg diagnosis date not before dob)
    • Calculate the IMD overriding save on the form rather than on the model
  • Only catch RequestException when wrapping remote network validators
    • That way we don't swallow logic errors but do swallow transient network errors
  • Mock out all network calls in the unit test
    • I did this originally to mock in return values but it's good practice anyway and speeds up the tests too

This PR adds a couple of behaviour changes:

- Use reusable not in the future validator
- Reusable Postcode field to clean both postcode and gp_practice_postcode
- Move validation that only needs one field to specific cleaners (clean_date_of_birth etc)
- Structured ValidationErrors following Django best practice
- Call self.add_error in clean function to ensure all errors are recorded, not just one
# We don't want to call remote services in unit tests
@pytest.fixture(autouse=True)
def mock_remote_calls():
with patch("project.npda.forms.patient_form.validate_postcode", Mock(return_value=True)):
Copy link
Member Author

Choose a reason for hiding this comment

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

I tried reducing the nesting here with an ExitStack but it didn't work. There's probably a way to do it but it's not that bad and wrapped up in a fixture anyway

@mbarton mbarton force-pushed the mbarton/validation-tests-only branch from af8701a to 9b94dcb Compare September 19, 2024 14:13
…date_postcode

It's easier to read in the form with spaces
@mbarton mbarton merged commit f98d445 into live Sep 19, 2024
1 check passed
@mbarton mbarton deleted the mbarton/validation-tests-only branch September 19, 2024 14:30
@mbarton
Copy link
Member Author

mbarton commented Sep 19, 2024

Seen on STAGING (merged by @mbarton 5 minutes and 22 seconds ago) Please check your changes!

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

Successfully merging this pull request may close these issues.

Add csv upload tests Can only look up GP in new patient form if postcode has a space
1 participant