Add backend support for ownership requests #740

gabeweng · 2024-10-15T18:45:55Z

Implement backend for new Ownership Requests feature and update Membership Request feature.

New:

OwnershipRequest model
UserMembershipRequestSerializer and OwnershipRequestViewSet to see/create/destroy Ownership Requests for a user
Routes for users to manage their own requests under api/requests/ownership
OwnershipRequestSerializer and OwnershipRequestManagementViewSet for management (including accept and deny) of Ownership Requests by club admin and site admin
Routes for clubs and superusers to manage requests under api/clubs/{club_code}/ownershiprequests
Email template for sending out ownership requests

Modified:

MembershipRequest model (person renamed to requester, withdrew renamed to withdrawn, related names for MembershipRequest added for requester and club)
api/requests/membership used instead of api/requests/ for membership requests

…s and migrate

rm03

Off to a great start! Left some comments; feel free to reply/reach out with any questions. Please also add some test cases when you get a chance 🙂

backend/clubs/admin.py

backend/clubs/migrations/0114_ownershiprequest.py

backend/clubs/models.py

backend/clubs/views.py

backend/templates/emails/ownershiprequest.html

aviupadhyayula

Thanks for the PR, this looks like a great start! Rohan was (very) thorough, not much to add. One last nit: consider updating the PR's title and description to be a little more descriptive. (Take a look at past PRs for examples.) It's a small detail, but when you're reviewing code years in the future, it's great to understand what the author was thinking.

…mentViewSet

codecov · 2024-10-18T22:23:03Z

Codecov Report

Attention: Patch coverage is 93.05556% with 10 lines in your changes missing coverage. Please review.

Project coverage is 72.59%. Comparing base (d121715) to head (01dc669).
Report is 2 commits behind head on master.

Files with missing lines	Patch %	Lines
backend/clubs/admin.py	66.66%	6 Missing ⚠️
backend/clubs/models.py	92.30%	2 Missing ⚠️
backend/clubs/permissions.py	88.88%	1 Missing ⚠️
backend/clubs/views.py	98.18%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #740      +/-   ##
==========================================
+ Coverage   71.88%   72.59%   +0.70%     
==========================================
  Files          32       32              
  Lines        6933     7039     +106     
==========================================
+ Hits         4984     5110     +126     
+ Misses       1949     1929      -20

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

…manage_club

… viewing membership requests

…/penn-clubs into feat/ownership-requests

rm03

Thanks for resolving the comments from earlier! Left a few more, but mostly looks good to me!

rm03 · 2024-11-12T23:53:14Z

backend/clubs/migrations/0119_rename_withdrew_ownershiprequest_withdrawn_and_more.py

@@ -0,0 +1,72 @@
+# Generated by Django 5.0.4 on 2024-10-18 11:42


Would you be able to delete these migration files and make a new one? Typically, we try and stick to one migration per PR.

rm03 · 2024-11-15T00:48:52Z

backend/clubs/models.py

@@ -1082,44 +1082,100 @@ class MembershipRequest(models.Model):
    Used when users are not in the club but request membership from the owner


Hmm, just occured to me that it might be beneficial to move the shared fields from OwnershipRequest and MembershipRequest into an abstract base class – lmk what you think!

rm03 · 2024-11-15T00:55:01Z

backend/clubs/serializers.py

+            "school",
+            "username",
+        )
+        validators = [


Any reason why we're keeping this?

rm03 · 2024-11-15T01:14:49Z

backend/clubs/views.py

+        """
+        club = request.data.get("club", None)
+
+        club_instance = Club.objects.get(code=club)


This will raise an exception if the club does not exist, would recommend using Club.objects.filter(code=club).first() and handling this case

rm03 · 2024-11-15T01:43:56Z

backend/tests/clubs/test_views.py

@@ -2923,3 +2925,1035 @@ def test_club_approval_response_templates(self):
            content_type="application/json",
        )
        self.assertEqual(resp.status_code, 403)
+


Appreciate the effort here but not sure if we need 1k lines of tests for this feature 😅 . From a quick glance, it looks like you're testing every action with every permission level – it's ok to omit testing everything except the most basic functionality.

+1, unit tests are best when they're contained but comprehensive. (Here's a good read on it.) Just glancing through this, I see duplicate checks of the same functionality. (There are 11 repeated checks of the membership-requests-list route alone.)

As an example, I asked GPT to create a condensed version of the ownership requests tests. Might be a good starting point.

def test_request_creation_permissions(self): """Test who can create ownership requests""" for user in [self.officer, self.member, self.non_member]: self.client.force_login(user) resp = self.client.post( reverse("ownership-requests-list"), {"club": self.club.code}, content_type="application/json" ) self.assertEqual(resp.status_code, 201) self.assertTrue( OwnershipRequest.objects.filter( club=self.club, requester=user ).exists() ) # Verify email was sent self.assertEqual(len(mail.outbox), 1) mail.outbox.clear() def test_request_viewing_permissions(self): """Test who can view ownership requests""" # Create a request request = OwnershipRequest.objects.create( club=self.club, requester=self.officer ) # Test requester can view own request self.client.force_login(self.officer) resp = self.client.get(reverse("ownership-requests-list")) self.assertEqual(resp.status_code, 200) self.assertEqual(len(resp.json()), 1) # Test owner can view club requests self.client.force_login(self.owner) resp = self.client.get( reverse("club-ownership-requests-list", args=(self.club.code,)) ) self.assertEqual(resp.status_code, 200) self.assertEqual(len(resp.json()), 1) # Test non-owners cannot view club requests for user in [self.officer, self.member, self.non_member]: self.client.force_login(user) resp = self.client.get( reverse("club-ownership-requests-list", args=(self.club.code,)) ) self.assertEqual(resp.status_code, 403) def test_request_withdrawal(self): """Test request withdrawal functionality""" request = OwnershipRequest.objects.create( club=self.club, requester=self.officer ) self.client.force_login(self.officer) resp = self.client.delete( reverse("ownership-requests-detail", args=(self.club.code,)) ) self.assertIn(resp.status_code, [200, 204]) # Verify request is marked withdrawn request.refresh_from_db() self.assertTrue(request.withdrawn) # Verify withdrawn request is not visible resp = self.client.get( reverse("ownership-requests-detail", args=(self.club.code,)) ) self.assertEqual(resp.status_code, 404) def test_request_acceptance(self): """Test request acceptance functionality""" request = OwnershipRequest.objects.create( club=self.club, requester=self.officer ) # Test non-owners cannot accept self.client.force_login(self.officer) resp = self.client.post( reverse("club-ownership-requests-accept", kwargs={ "club_code": self.club.code, "requester__username": self.officer.username } ) ) self.assertEqual(resp.status_code, 403) # Test owner can accept self.client.force_login(self.owner) resp = self.client.post( reverse("club-ownership-requests-accept", kwargs={ "club_code": self.club.code, "requester__username": self.officer.username } ) ) self.assertEqual(resp.status_code, 200) # Verify membership was updated self.assertTrue( Membership.objects.filter( club=self.club, person=self.officer, role=Membership.ROLE_OWNER ).exists() )

aviupadhyayula

Great work Gabe! Rohan did a great job with his review, so not much to add outside of minor comments.

aviupadhyayula · 2024-11-15T01:40:51Z

backend/tests/clubs/test_views.py

@@ -2923,3 +2925,1035 @@ def test_club_approval_response_templates(self):
            content_type="application/json",
        )
        self.assertEqual(resp.status_code, 403)
+
+    def test_ownershiprequests_create_and_view(self):


Nit: can we make this test_ownership_requests_...

aviupadhyayula · 2024-11-15T04:00:58Z

backend/clubs/models.py

+    def send_request(self, request=None):
+        domain = get_domain(request)
+
+        edit_url = settings.EDIT_URL.format(domain=domain, club=self.club.code)
+
+        club_name = self.club.name
+
+        full_name = self.requester.get_full_name()
+
+        context = {
+            "club_name": club_name,
+            "edit_url": f"{edit_url}/member",
+            "full_name": full_name,
+        }
+
+        owner_emails = list(
+            self.club.membership_set.filter(
+                role=Membership.ROLE_OWNER, active=True
+            ).values_list("person__email", flat=True)
+        )
+
+        send_mail_helper(
+            name="ownership_request",
+            subject=f"Ownership Request from {full_name} for {club_name}",
+            emails=owner_emails,
+            context=context,
+        )


Nit: use whitespace to break up a large block of code into its smaller logical chunks. The attributes (domain, edit_url, etc.) should be grouped together. (Same goes for the rest of the PR.)

aviupadhyayula · 2024-11-15T04:03:12Z

backend/clubs/serializers.py

+
+class UserOwnershipRequestSerializer(serializers.ModelSerializer):
+    """
+    Used by the users to return the clubs that the user has sent OwnershipRequest to.


Nit: should be "... that the user has sent an OwnershipRequest to."

aviupadhyayula · 2024-11-15T04:10:38Z

backend/clubs/views.py

+        queryset = OwnershipRequest.objects.filter(
+            withdrawn=False, created_at__lte=timezone.now() - datetime.timedelta(days=7)
+        )


@rm03 Do you know why we're limiting this to requests that are a week old? I feel like admins should be able to view all requests. We've had situations where someone needs to take over a club a day before the renewal deadline / Wharton applications / club fair.

aviupadhyayula · 2024-11-15T04:13:07Z

backend/templates/emails/ownership_request.html

+<!-- TYPES:
+club_name:
+    type: string
+edit_url:
+    type: string
+full_name:
+    type: string
+-->
+{% extends 'emails/base.html' %}
+
+{% block content %}
+    <h2>Request for ownership for <b>{{ club_name }}</b> from <b>{{ full_name }}</b></h2>
+    <p style="font-size: 1.2em"><b>{{ full_name }}</b> has submitted a request for ownership of <b>{{ club_name }}</b> through the Penn
+        Clubs website. To approve this request, use the button below to navigate to the Penn Clubs website.</p>
+    <a style="text-decoration: none; padding: 5px 20px; font-size: 1.5em; margin-top: 20px; color: white; background-color: #4954f4; border-radius: 3px; font-weight: bold"
+        href="{{ edit_url }}">Approve Request</a>
+{% endblock %}


This is great, do you mind updating the membership request email template to match this as well (including the file name format)?

aviupadhyayula · 2024-11-15T04:13:46Z

backend/templates/emails/ownership_request.html

+{% extends 'emails/base.html' %}
+
+{% block content %}
+    <h2>Request for ownership for <b>{{ club_name }}</b> from <b>{{ full_name }}</b></h2>


Nit: might be cleaner to say "Request for Ownership of {club_name} from {full_name}"

gabeweng added 2 commits October 13, 2024 10:57

Make models, perms, serializers, urls, and views for ownershiprequest…

42e953b

…s and migrate

add API view for admin to see all ownershiprequests older than a week

9439212

gabeweng requested a review from rm03 October 15, 2024 18:45

merge conflicting migrations

32baced

rm03 requested changes Oct 16, 2024

View reviewed changes

aviupadhyayula reviewed Oct 16, 2024

View reviewed changes

gabeweng added 7 commits October 18, 2024 00:41

Merge branch 'master' into feat/ownership-requests

38224e3

migrate ownershiprequests

dc828b2

edit descriptions of OwnershipRequestSerializer and OwnershipRequest

5fba7b8

rename, update ownershiprequest model fields

6b511f0

edit ownership_request.html template

f36d1f9

Combine club admin and superuser viewsets for ownership requests

c5b6c4c

Specify YAML documentation for old_requests in OwnershipRequestManage…

917c1fc

…mentViewSet

gabeweng added 9 commits October 20, 2024 13:23

Fix permissions for OwnershipRequest management to not specify clubs.…

ec1597f

…manage_club

Change url basename for user ownership requests

d8ee660

Use update_or_create logic for Ownership Requests

f905122

Write test for creating and viewing Ownership Requests

fa2b7e7

Return 204 for Requester view for destroy ownershiprequest

5462032

Edit OwnershipRequestAdmin for admin dashboard

a16d758

Create test for withdraw feature of OwnershipRequests

ffed858

Create test for accept feature of OwnershipRequests

60f0962

Create test for destroy feature of OwnershipRequests

d5e4b6c

gabeweng force-pushed the feat/ownership-requests branch from 8808888 to d5e4b6c Compare November 8, 2024 03:23

gabeweng and others added 6 commits November 7, 2024 22:28

Merge branch 'master' into feat/ownership-requests

d94a5f4

Change comments and assert status codes for test_ownership_requests

4e31e86

Change basename for membership requests and add test for creating and…

ac1f4de

… viewing membership requests

Add test for withdrawing membership requests

cf89829

Add test for accepting membership requests

038a1bb

Add test for destroying membership requests

826f54c

gabeweng added 2 commits November 8, 2024 18:06

Update Membership Requests model and logic to match Ownership Requests

ff1719e

Merge branch 'feat/ownership-requests' of https://github.com/pennlabs…

01dc669

…/penn-clubs into feat/ownership-requests

gabeweng added the backend label Nov 10, 2024

gabeweng marked this pull request as ready for review November 10, 2024 17:17

aviupadhyayula requested review from rm03 and aviupadhyayula November 11, 2024 17:37

aviupadhyayula changed the title ~~Feat/ownership requests~~ Add backend support for ownership requests Nov 13, 2024

rm03 reviewed Nov 15, 2024

View reviewed changes

aviupadhyayula reviewed Nov 15, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add backend support for ownership requests #740

Add backend support for ownership requests #740

gabeweng commented Oct 15, 2024 •

edited

Loading

rm03 left a comment

aviupadhyayula left a comment •

edited

Loading

codecov bot commented Oct 18, 2024 •

edited

Loading

rm03 left a comment

rm03 Nov 12, 2024

rm03 Nov 15, 2024

rm03 Nov 15, 2024

rm03 Nov 15, 2024

rm03 Nov 15, 2024

aviupadhyayula Nov 15, 2024 •

edited

Loading

aviupadhyayula Nov 15, 2024 •

edited

Loading

aviupadhyayula left a comment

aviupadhyayula Nov 15, 2024

aviupadhyayula Nov 15, 2024

aviupadhyayula Nov 15, 2024

aviupadhyayula Nov 15, 2024 •

edited

Loading

aviupadhyayula Nov 15, 2024

aviupadhyayula Nov 15, 2024

		@@ -0,0 +1,72 @@
		# Generated by Django 5.0.4 on 2024-10-18 11:42

		@@ -1082,44 +1082,100 @@ class MembershipRequest(models.Model):
		Used when users are not in the club but request membership from the owner

Add backend support for ownership requests #740

Are you sure you want to change the base?

Add backend support for ownership requests #740

Conversation

gabeweng commented Oct 15, 2024 • edited Loading

rm03 left a comment

Choose a reason for hiding this comment

aviupadhyayula left a comment • edited Loading

Choose a reason for hiding this comment

codecov bot commented Oct 18, 2024 • edited Loading

Codecov Report

rm03 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aviupadhyayula Nov 15, 2024 • edited Loading

Choose a reason for hiding this comment

aviupadhyayula Nov 15, 2024 • edited Loading

Choose a reason for hiding this comment

aviupadhyayula left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aviupadhyayula Nov 15, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gabeweng commented Oct 15, 2024 •

edited

Loading

aviupadhyayula left a comment •

edited

Loading

codecov bot commented Oct 18, 2024 •

edited

Loading

aviupadhyayula Nov 15, 2024 •

edited

Loading

aviupadhyayula Nov 15, 2024 •

edited

Loading

aviupadhyayula Nov 15, 2024 •

edited

Loading