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

Change club serialization based on approval status #733

Open
wants to merge 7 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
62 changes: 40 additions & 22 deletions backend/clubs/serializers.py
Original file line number Diff line number Diff line change
Expand Up @@ -994,30 +994,48 @@ def get_fields(self):

def to_representation(self, instance):
"""
Return the previous approved version of a club for users
that should not see unapproved content.
Return appropriate version of club based on user permissions and club status:
- Privileged users see current version (or when bypass=True)
Copy link
Member

Choose a reason for hiding this comment

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

What's the use case for bypass=True? I don't think clubs should be allowed to show an unapproved version at all

Copy link
Member Author

@aviupadhyayula aviupadhyayula Nov 15, 2024

Choose a reason for hiding this comment

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

I think we use bypass=True when someone's accepting a membership invite to a club. (Basing this off the comments in ClubPermission from permissions.py.)

- For non-privileged users:
- During renewal period: show last approved version from past calendar year
- Outside renewal period: show last approved version from this renewal cycle
"""
if instance.ghost and not instance.approved:
user = self.context["request"].user
user = self.context["request"].user
can_see_pending = user.has_perm("clubs.see_pending_clubs") or user.has_perm(
"clubs.manage_club"
)
is_member = (
user.is_authenticated
and instance.membership_set.filter(person=user).exists()
)
bypass = (
self.context["request"].query_params.get("bypass", "").lower() == "true"
)

can_see_pending = user.has_perm("clubs.see_pending_clubs") or user.has_perm(
"clubs.manage_club"
)
is_member = (
user.is_authenticated
and instance.membership_set.filter(person=user).exists()
)
if not can_see_pending and not is_member:
historical_club = (
instance.history.filter(approved=True)
.order_by("-approved_on")
.first()
)
if historical_club is not None:
approved_instance = historical_club.instance
approved_instance._is_historical = True
return super().to_representation(approved_instance)
return super().to_representation(instance)
if can_see_pending or is_member or instance.approved or bypass:
return super().to_representation(instance)

now = timezone.now()
renewal_start, renewal_end = settings.RENEWAL_PERIOD
in_renewal_period = renewal_start <= now <= renewal_end

start_date = (
renewal_start
if not in_renewal_period
else now.replace(year=now.year - 1, month=1, day=1)
)
approved_version = (
instance.history.filter(approved=True, approved_on__gte=start_date)
.order_by("-approved_on")
.first()
)

if approved_version is not None:
approved_instance = approved_version.instance
approved_instance._is_historical = True
return super().to_representation(approved_instance)

raise serializers.ValidationError("Club not found", code="not_found")

class Meta:
model = Club
Expand Down
16 changes: 11 additions & 5 deletions backend/pennclubs/settings/base.py
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@
import os

import dj_database_url
from django.utils import timezone


# Build paths inside the project like this: os.path.join(BASE_DIR, ...)
Expand Down Expand Up @@ -204,11 +205,16 @@

OSA_EMAILS = ["[email protected]"]


# Controls whether existing clubs can submit for reapproval
REAPPROVAL_QUEUE_OPEN = True
# Controls whether new clubs can submit for initial approval
NEW_APPROVAL_QUEUE_OPEN = True
REAPPROVAL_QUEUE_OPEN = True # controls whether existing clubs can request reapproval
NEW_APPROVAL_QUEUE_OPEN = True # controls whether new clubs can request approval
RENEWAL_PERIOD = (
timezone.datetime(
timezone.now().year, 8, 1, tzinfo=timezone.get_default_timezone()
),
timezone.datetime(
timezone.now().year, 9, 30, tzinfo=timezone.get_default_timezone()
),
) # defines renewal period for club visibility

# File upload settings

Expand Down
152 changes: 152 additions & 0 deletions backend/tests/clubs/test_serializers.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,152 @@
from datetime import timedelta
from unittest import mock

from django.conf import settings
from django.contrib.auth.models import Permission, User
from django.test import TestCase
from django.urls import reverse
from django.utils import timezone
from rest_framework.exceptions import ValidationError
from rest_framework.request import Request
from rest_framework.test import APIRequestFactory

from clubs.models import Club, Membership
from clubs.serializers import ClubSerializer


class ClubSerializerTestCase(TestCase):
def setUp(self):
self.factory = APIRequestFactory()
self.user = User.objects.create_user(username="testuser", password="12345")
self.club = Club.objects.create(name="Test Club", code="test", approved=True)

# Set renewal period in settings to current time
now = timezone.now()
self.renewal_start = now - timedelta(days=30)
self.renewal_end = now + timedelta(days=30)
settings.RENEWAL_PERIOD = (self.renewal_start, self.renewal_end)

self.serializer = ClubSerializer()

def test_privileged_user_sees_current_version(self):
self.user.user_permissions.add(
Permission.objects.get(codename="see_pending_clubs")
)
request = self.factory.get(reverse("clubs-detail", args=[self.club.code]))
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there a reason for not using client like in the other test files?

Copy link
Member Author

Choose a reason for hiding this comment

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

We don't want to actually send the request to the API -- we just want to create the request object, and then pass it to the serializer. This lets us test the serializer itself instead of the entire view.

The user_permissions.add pattern was unintentional. Changed to self.user.is_superuser = True instead.

request.user = self.user

self.serializer.context["request"] = Request(request)
result = self.serializer.to_representation(self.club)
self.assertEqual(result["name"], "Test Club")

def test_member_sees_current_version(self):
Membership.objects.create(person=self.user, club=self.club)
request = self.factory.get(reverse("clubs-detail", args=[self.club.code]))
request.user = self.user

self.serializer.context["request"] = Request(request)
result = self.serializer.to_representation(self.club)
self.assertEqual(result["name"], "Test Club")

def test_approved_club_visible_to_all(self):
request = self.factory.get(reverse("clubs-detail", args=[self.club.code]))
request.user = self.user

self.serializer.context["request"] = Request(request)
result = self.serializer.to_representation(self.club)
self.assertEqual(result["name"], "Test Club")

def test_bypass_flag_shows_current_version(self):
request = self.factory.get(
f"{reverse('clubs-detail', args=[self.club.code])}?bypass=true"
)
request.user = self.user

self.serializer.context["request"] = Request(request)
result = self.serializer.to_representation(self.club)
self.assertEqual(result["name"], "Test Club")

def test_non_privileged_user_during_renewal_period(self):
# Simulate deactivation at the beginning of the renewal period
self.club.save()
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this intentional?

self.club.approved = False
self.club.save()

# Insert a previously approved version into the club's history
past_time = timezone.now() - timedelta(days=180)
with mock.patch("django.utils.timezone.now", return_value=past_time):
self.club.name = "Past Approved Version"
self.club.approved = True
self.club.approved_on = past_time
self.club.save()

self.club.name = "Current Unapproved Version"
self.club.approved = False
self.club.approved_on = None
self.club.save()

request = self.factory.get("/fake-url/")
request.user = self.user
serializer = ClubSerializer(context={"request": Request(request)})

result = serializer.to_representation(self.club)
self.assertEqual(result["name"], "Past Approved Version")
self.assertNotEqual(result["name"], "Current Unapproved Version")

def test_non_privileged_user_outside_renewal_period_ghost_club(self):
self.club.approved = False
self.club.ghost = True
self.club.save()

current_date = self.renewal_end + timedelta(days=1)
with mock.patch("django.utils.timezone.now", return_value=current_date):
approved_date = self.renewal_start + timedelta(days=1)
with mock.patch("django.utils.timezone.now", return_value=approved_date):
self.club.name = "Current Cycle Approved Version"
self.club.approved = True
self.club.approved_on = approved_date
self.club.save()
Copy link
Contributor

Choose a reason for hiding this comment

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

Unless there is internal logic in club.save or serializer.to_representation that uses django.utils.timezone.now, we might not need the with clauses since we are explicitly setting club.approved_on


self.club.name = "Current Unapproved Version"
self.club.approved = False
self.club.approved_on = None
self.club.save()

request = self.factory.get(reverse("clubs-detail", args=[self.club.code]))
request.user = self.user

self.serializer.context["request"] = Request(request)
result = self.serializer.to_representation(self.club)
self.assertEqual(result["name"], "Current Cycle Approved Version")
self.assertNotEqual(result["name"], "Current Unapproved Version")

def test_non_privileged_user_no_approved_version(self):
self.club.approved = False
self.club.save()

request = self.factory.get(reverse("clubs-detail", args=[self.club.code]))
request.user = self.user
self.serializer.context["request"] = Request(request)

with self.assertRaises(ValidationError) as context:
self.serializer.to_representation(self.club)

self.assertEqual(context.exception.detail[0], "Club not found")
self.assertEqual(context.exception.get_codes()[0], "not_found")

def test_non_privileged_user_outside_renewal_period_non_ghost_club(self):
self.club.approved = False
self.club.ghost = False
self.club.save()

current_date = self.renewal_end + timedelta(days=1)
with mock.patch("django.utils.timezone.now", return_value=current_date):
request = self.factory.get(reverse("clubs-detail", args=[self.club.code]))
request.user = self.user

self.serializer.context["request"] = Request(request)
with self.assertRaises(ValidationError) as context:
self.serializer.to_representation(self.club)

self.assertEqual(context.exception.detail[0], "Club not found")
self.assertEqual(context.exception.get_codes()[0], "not_found") #
3 changes: 2 additions & 1 deletion backend/tests/clubs/test_views.py
Original file line number Diff line number Diff line change
Expand Up @@ -175,8 +175,9 @@ def setUp(self):
self.club1 = Club.objects.create(
code="test-club",
name="Test Club",
approved=True,
email="[email protected]",
approved=True,
approved_on=timezone.now(),
)

self.event1 = Event.objects.create(
Expand Down
Loading