Ownership request status #812
Conversation
There was a problem hiding this comment.
Copilot reviewed 6 out of 6 changed files in this pull request and generated no comments.
Comments suppressed due to low confidence (1)
backend/clubs/models.py:1129
- [nitpick] Consider adding a clarifying comment on the design rationale for retaining the 'withdrawn' field in MembershipRequest while replacing it with a 'status' field in OwnershipRequest to help future maintainers understand the distinction.
+ withdrawn = models.BooleanField(default=False)
There was a problem hiding this comment.
LGTM, thanks for taking the initiative on this Gabe. Left a couple comments.
Also, is this compatible with @beladelgado2's work in #815? If not, let's add the necessary frontend changes to this PR before merging.
backend/clubs/views.py
Outdated
| request_instance = OwnershipRequest.objects.filter( | ||
| club__code=club, requester=request.user | ||
| ).first() |
| request_object.status = OwnershipRequest.ACCEPTED | ||
| request_object.save(update_fields=["status"]) |
There was a problem hiding this comment.
Just realized: do we update requesters when the status of their request is updated? Feels like we should do that (if we don't already).
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #812 +/- ##
==========================================
+ Coverage 74.54% 74.69% +0.14%
==========================================
Files 33 33
Lines 7831 7875 +44
==========================================
+ Hits 5838 5882 +44
Misses 1993 1993 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
| @@ -284,3 +286,210 @@ def test_send_mail_retry_logic(self): | |||
| ) | |||
|
|
|||
| self.assertEqual(mocked_send.call_count, 3) | |||
|
|
|||
|
|
|||
| class OwnershipRequestTestCase(TestCase): | |||
There was a problem hiding this comment.
Honestly I'm not sure how necessary it is to test the business logic in both model unit tests and view tests. If I get the go-ahead I will probably just delete all of these model tests.
|
FYI currently this is compatible with the front end, but there is also backend functionality that isn't used in the frontend (withdrawing your own requests, club owners seeing and handling requests to their clubs). I don't think this is very likely to be used anyway though. |
We want to keep track of ownership requests that are accepted or denied as opposed to deleting them when they are handled. This is done by replacing the "withdrawn" field with a "status" field, which can be PENDING, WITHDRAWN, ACCEPTED, or DENIED.