Hide personal information for non-authenticated users on club page (#732

)

Add granular visibility per advisor (admin, students, public) and hide question respondent names for unauthed view

backend/clubs/admin.py

@@ -324,7 +324,7 @@ def club(self, obj):
 
 class AdvisorAdmin(admin.ModelAdmin):
     search_fields = ("name", "title", "email", "phone", "club__name")
-    list_display = ("name", "title", "email", "phone", "club", "public")
+    list_display = ("name", "title", "email", "phone", "club", "visibility")
 
     def club(self, obj):
         return obj.club.name

backend/clubs/management/commands/deactivate.py

@@ -93,7 +93,8 @@ def handle(self, *args, **kwargs):
                         num_ghosted += 1
 
                     club.save()
-                    cache.delete(f"clubs:{club.id}")  # clear cache
+                    cache.delete(f"clubs:{club.id}-authed")  # clear cache
+                    cache.delete(f"clubs:{club.id}-anon")
 
             self.stdout.write(
                 f"{clubs.count()} clubs deactivated! {num_ghosted} clubs ghosted!"

backend/clubs/management/commands/populate.py

@@ -455,7 +455,7 @@ def get_image(url):
                 department="Accounting Department",
                 email="[email protected]",
                 phone="+12158985000",
-                defaults={"public": True},
+                defaults={"visibility": Advisor.ADVISOR_VISIBILITY_STUDENTS},
             )
 
             club.tags.add(tag_undergrad)

backend/clubs/migrations/0114_alter_advisor_public.py

@@ -0,0 +1,35 @@
+# Generated by Django 5.0.4 on 2024-09-29 17:00
+
+from django.db import migrations, models
+
+
+def migrate_public_to_enum(apps, schema_editor):
+    Advisor = apps.get_model("clubs", "Advisor")
+    # Update 'public' field, assume public=True means only students by default
+    Advisor.objects.filter(public=False).update(visibility=1)
+    Advisor.objects.filter(public=True).update(visibility=2)
+
+def reverse_migrate_public_to_enum(apps, schema_editor):
+    Advisor = apps.get_model("clubs", "Advisor")
+    Advisor.objects.filter(visibility=1).update(public=False)
+    Advisor.objects.filter(visibility__in=[2, 3]).update(public=True)
+
+class Migration(migrations.Migration):
+    dependencies = [
+        ("clubs", "0113_badge_message"),
+    ]
+    operations = [
+        migrations.AddField(
+            model_name="advisor",
+            name="visibility",
+            field=models.IntegerField(
+                choices=[(1, "Admin Only"), (2, "Signed-in Students"), (3, "Public")],
+                default=2,
+            ),
+        ),
+        migrations.RunPython(migrate_public_to_enum, reverse_migrate_public_to_enum),
+        migrations.RemoveField(
+            model_name="advisor",
+            name="public",
+        ),
+    ]

backend/clubs/models.py

@@ -1136,7 +1136,20 @@ class Advisor(models.Model):
     phone = PhoneNumberField(null=False, blank=True)
 
     club = models.ForeignKey(Club, on_delete=models.CASCADE)
-    public = models.BooleanField()
+
+    ADVISOR_VISIBILITY_ADMIN = 1
+    ADVISOR_VISIBILITY_STUDENTS = 2
+    ADVISOR_VISIBILITY_ALL = 3
+
+    ADVISOR_VISIBILITY_CHOICES = (
+        (ADVISOR_VISIBILITY_ADMIN, "Admin Only"),
+        (ADVISOR_VISIBILITY_STUDENTS, "Signed-in Students"),
+        (ADVISOR_VISIBILITY_ALL, "Public"),
+    )
+
+    visibility = models.IntegerField(
+        choices=ADVISOR_VISIBILITY_CHOICES, default=ADVISOR_VISIBILITY_STUDENTS
+    )
 
     def __str__(self):
         return self.name

backend/clubs/serializers.py

@@ -351,7 +351,7 @@ class AdvisorSerializer(
 ):
     class Meta:
         model = Advisor
-        fields = ("id", "name", "title", "department", "email", "phone", "public")
+        fields = ("id", "name", "title", "department", "email", "phone", "visibility")
 
 
 class ClubEventSerializer(serializers.ModelSerializer):
@@ -1249,8 +1249,14 @@ def get_approved_by(self, obj):
         return obj.approved_by.get_full_name()
 
     def get_advisor_set(self, obj):
+        user = self.context["request"].user
+        visibility_level = (
+            Advisor.ADVISOR_VISIBILITY_STUDENTS
+            if user.is_authenticated
+            else Advisor.ADVISOR_VISIBILITY_ALL
+        )
         return AdvisorSerializer(
-            obj.advisor_set.filter(public=True).order_by("name"),
+            obj.advisor_set.filter(visibility__gte=visibility_level),
             many=True,
             read_only=True,
             context=self.context,

backend/clubs/views.py

@@ -1204,8 +1204,8 @@ def upload(self, request, *args, **kwargs):
         """
         # ensure user is allowed to upload image
         club = self.get_object()
-        key = f"clubs:{club.id}"
-        cache.delete(key)
+        cache.delete(f"clubs:{club.id}-authed")
+        cache.delete(f"clubs:{club.id}-anon")
 
         # reset approval status after upload
         resp = upload_endpoint_helper(request, club, "file", "image", save=False)
@@ -2024,7 +2024,8 @@ def retrieve(self, *args, **kwargs):
         if self._has_elevated_view_perms(club):
             return super().retrieve(*args, **kwargs)
 
-        key = f"clubs:{club.id}"
+        key = f"""clubs:{club.id}-{"authed" if self.request.user.is_authenticated
+                                            else "anon"}"""
         cached = cache.get(key)
         if cached:
             return Response(cached)
@@ -2038,17 +2039,17 @@ def update(self, request, *args, **kwargs):
         Invalidate caches
         """
         self.check_approval_permission(request)
-        key = f"clubs:{self.get_object().id}"
-        cache.delete(key)
+        cache.delete(f"clubs:{self.get_object().id}-authed")
+        cache.delete(f"clubs:{self.get_object().id}-anon")
         return super().update(request, *args, **kwargs)
 
     def partial_update(self, request, *args, **kwargs):
         """
         Invalidate caches
         """
         self.check_approval_permission(request)
-        key = f"clubs:{self.get_object().id}"
-        cache.delete(key)
+        cache.delete(f"clubs:{self.get_object().id}-authed")
+        cache.delete(f"clubs:{self.get_object().id}-anon")
         return super().partial_update(request, *args, **kwargs)
 
     def perform_destroy(self, instance):
@@ -2279,19 +2280,22 @@ class AdvisorSearchFilter(filters.BaseFilterBackend):
 
     def filter_queryset(self, request, queryset, view):
         public = request.GET.get("public")
-
         if public is not None:
             public = public.strip().lower()
             if public in {"true", "false"}:
-                queryset = queryset.filter(public=public == "true")
+                queryset = queryset.filter(
+                    visibility__gte=Advisor.ADVISOR_VISIBILITY_STUDENTS
+                    if public == "true"
+                    else Advisor.ADVISOR_VISIBILITY_ADMIN
+                )
 
         return queryset
 
 
 class AdvisorViewSet(viewsets.ModelViewSet):
     """
     list:
-    Return a list of advisors for this club.
+    Return a list of advisors for this club for club administrators.
 
     create:
     Add an advisor to this club.
@@ -2310,7 +2314,7 @@ class AdvisorViewSet(viewsets.ModelViewSet):
     """
 
     serializer_class = AdvisorSerializer
-    permission_classes = [ClubItemPermission | IsSuperuser]
+    permission_classes = [ClubSensitiveItemPermission | IsSuperuser]
     filter_backends = [AdvisorSearchFilter]
     lookup_field = "id"
     http_method_names = ["get", "post", "put", "patch", "delete"]
@@ -2818,7 +2822,8 @@ def create_tickets(self, request, *args, **kwargs):
             event.ticket_drop_time = drop_time
             event.save()
 
-        cache.delete(f"clubs:{event.club.id}")
+        cache.delete(f"clubs:{event.club.id}-authed")
+        cache.delete(f"clubs:{event.club.id}-anon")
         return Response({"detail": "Successfully created tickets"})
 
     @action(detail=True, methods=["post"])
@@ -3422,7 +3427,8 @@ def get_queryset(self):
         )
 
         if not self.request.user.is_authenticated:
-            return questions.filter(approved=True)
+            # Hide responder if not authenticated
+            return questions.filter(approved=True).extra(select={"responder": "NULL"})
 
         membership = Membership.objects.filter(
             club__code=club_code, person=self.request.user

backend/tests/clubs/test_commands.py

@@ -616,7 +616,7 @@ def test_deactivate_invalidates_cache(self):
         self.client.get(reverse("clubs-detail", args=(club.code,)))
 
         # club should now be cached
-        cache_key = f"clubs:{club.id}"
+        cache_key = f"clubs:{club.id}-anon"
         self.assertIsNotNone(caches["default"].get(cache_key))
 
         call_command("deactivate", "all", "--force")

backend/tests/clubs/test_models.py

@@ -198,7 +198,10 @@ def setUp(self):
             code="a", name="a", subtitle="a", founded=date, description="a", size=1
         )
         self.advisor = Advisor.objects.create(
-            name="Eric Wang", phone="+12025550133", club=club, public=True
+            name="Eric Wang",
+            phone="+12025550133",
+            club=club,
+            visibility=Advisor.ADVISOR_VISIBILITY_ALL,
         )
 
     def test_str(self):

backend/tests/clubs/test_views.py

@@ -18,6 +18,7 @@
 
 from clubs.filters import DEFAULT_PAGE_SIZE
 from clubs.models import (
+    Advisor,
     ApplicationSubmission,
     Asset,
     Badge,
@@ -204,6 +205,77 @@ def setUp(self):
             responder=self.user1,
         )
 
+        self.advisor_admin = Advisor.objects.create(
+            name="Anonymous Avi",
+            phone="+12025550133",
+            club=self.club1,
+            visibility=Advisor.ADVISOR_VISIBILITY_ADMIN,
+        )
+
+        self.advisor_students = Advisor.objects.create(
+            name="Reclusive Rohan",
+            phone="+12025550133",
+            club=self.club1,
+            visibility=Advisor.ADVISOR_VISIBILITY_STUDENTS,
+        )
+
+        self.advisor_public = Advisor.objects.create(
+            name="Jocular Julian",
+            phone="+12025550133",
+            club=self.club1,
+            visibility=Advisor.ADVISOR_VISIBILITY_ALL,
+        )
+
+    def test_advisor_visibility(self):
+        """
+        Tests each tier of advisor visibility.
+        """
+        # Anonymous view
+        self.client.logout()
+        resp = self.client.get(reverse("clubs-detail", args=(self.club1.code,)))
+        self.assertIn(resp.status_code, [200, 201], resp.content)
+        self.assertEqual(len(resp.data["advisor_set"]), 1)
+        self.assertEqual(resp.data["advisor_set"][0]["name"], "Jocular Julian")
+
+        # Student view
+        self.client.login(username=self.user1.username, password="test")
+        resp = self.client.get(reverse("clubs-detail", args=(self.club1.code,)))
+        self.assertIn(resp.status_code, [200, 201], resp.content)
+        self.assertEqual(len(resp.data["advisor_set"]), 2)
+        sorted_advisors = sorted(
+            [advisor["name"] for advisor in resp.data["advisor_set"]]
+        )
+        self.assertEqual(sorted_advisors, ["Jocular Julian", "Reclusive Rohan"])
+
+        # Admin view
+        self.client.login(username=self.user5.username, password="test")
+        resp = self.client.get(reverse("clubs-detail", args=(self.club1.code,)))
+        self.assertIn(resp.status_code, [200, 201], resp.content)
+        self.assertEqual(len(resp.data["advisor_set"]), 3)
+        sorted_advisors = sorted(
+            [advisor["name"] for advisor in resp.data["advisor_set"]]
+        )
+        self.assertEqual(
+            sorted_advisors, ["Anonymous Avi", "Jocular Julian", "Reclusive Rohan"]
+        )
+
+    def test_advisor_viewset(self):
+        # Make sure we can't view advisors via the viewset if not authed
+        self.client.logout()
+        resp = self.client.get(reverse("club-advisors-list", args=(self.club1.code,)))
+        self.assertIn(resp.status_code, [400, 403], resp.content)
+        self.assertIn("detail", resp.data)
+
+        self.client.login(username=self.user1.username, password="test")
+        resp = self.client.get(reverse("club-advisors-list", args=(self.club1.code,)))
+        self.assertIn(resp.status_code, [400, 403], resp.content)
+        self.assertIn("detail", resp.data)
+
+        self.client.login(username=self.user5.username, password="test")
+        resp = self.client.get(reverse("club-advisors-list", args=(self.club1.code,)))
+        self.assertIn(resp.status_code, [200, 201], resp.content)
+        self.assertEqual(len(resp.data), 3)
+
     def test_club_upload(self):
         """
         Test uploading a club logo.