Skip to content
This repository has been archived by the owner on Feb 1, 2024. It is now read-only.

Ensure at most one approved claim per facility #585

Merged
merged 1 commit into from Jun 13, 2019

Conversation

kellyi
Copy link
Contributor

@kellyi kellyi commented Jun 13, 2019

Overview

When attempting to approve a facility claim, check whether there are
already any other approved facility claims and, if so, return a bad
request response. Also add a test to demonstrate the feature
functionality.

Connects #581

Testing Instructions

It should suffice just to run the tests and verify that they continue to pass.

You can also go through the loop of:

  • turn on claim a facility
  • sign in as c2@example.com
  • submit the claim form 2x for the same facility
  • sign out, then sign in as a superuser
  • visit /dashboard/claims
  • approve one of the facilities and verify that it works
  • attempt to approve the other facility and verify that it does not work and that you see an error

Checklist

  • fixup! commits have been squashed
  • CI passes after rebase
  • CHANGELOG.md updated with summary of features or fixes, following Keep a Changelog guidelines

@kellyi kellyi requested a review from jwalgran June 13, 2019 00:32
@kellyi kellyi force-pushed the ki/ensure-at-most-one-approved-claim-per-facility branch from 2d486c8 to e52b7d7 Compare June 13, 2019 00:33
@kellyi kellyi requested review from rajadain and removed request for jwalgran June 13, 2019 00:57
@kellyi kellyi assigned rajadain and unassigned jwalgran Jun 13, 2019
src/django/api/tests.py Outdated Show resolved Hide resolved
@kellyi kellyi force-pushed the ki/ensure-at-most-one-approved-claim-per-facility branch from e52b7d7 to b83f009 Compare June 13, 2019 14:01
@rajadain
Copy link
Contributor

Taking a look now.

When attempting to approve a facility claim, check whether there are
already any other approved facility claims and, if so, return a bad
request response. Also add a test to demonstrate the feature
functionality.
@kellyi kellyi force-pushed the ki/ensure-at-most-one-approved-claim-per-facility branch from b83f009 to 8ee1649 Compare June 13, 2019 16:28
Copy link
Contributor

@rajadain rajadain left a comment

Choose a reason for hiding this comment

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

Works as advertised. It would've been nicer if the error explained that the reason for failure was the presence of an approve claim, rather than the generic "there was an error", but that can be an improvement down the line.

I was able to successfully deny the second claim, and it did not affect the way the facility is shown in the front-end. It still shows the first claim correctly.

@rajadain rajadain assigned kellyi and unassigned rajadain Jun 13, 2019
@kellyi
Copy link
Contributor Author

kellyi commented Jun 13, 2019

Thanks for testing this! For this:

It would've been nicer if the error explained that the reason for failure was the presence of an approve claim, rather than the generic "there was an error"

The server error will be logged to the browser console, so we can always get it that way we encounter this again.

@kellyi kellyi merged commit 6122cf6 into develop Jun 13, 2019
@kellyi kellyi deleted the ki/ensure-at-most-one-approved-claim-per-facility branch June 13, 2019 17:11
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants