refactor(gsr): simplify permission check error handling and add logging

  - Add logging to is_wharton() and is_seas() to track API failures
  - Return False instead of raising errors for failed permission checks
  - Remove need for try/except boilerplate from views, models, and commands

Author: darrentweng

backend/gsr_booking/api_wrapper.py

@@ -1,4 +1,5 @@
 import datetime
+import logging
 import re
 from abc import ABC, abstractmethod
 from enum import Enum
@@ -21,6 +22,9 @@
 from utils.errors import APIError
 
 
+logger = logging.getLogger(__name__)
+
+
 User = get_user_model()
 
 
@@ -158,17 +162,22 @@ def get_reservations(self, user):
         ]
 
     def is_wharton(self, user):
+        """
+        Check if user has Wharton privileges.
+        Returns False if user doesn't have access or if API fails.
+        Logs errors for monitoring.
+        """
         url = f"{WHARTON_URL}{user.username}/privileges"
         try:
             response = self.request("GET", url)
             if response.status_code != 200:
-                return None
+                logger.error(f"Wharton API error for {user.username}: HTTP {response.status_code}")
+                return False
             res_json = response.json()
             return res_json.get("type") == "whartonMBA" or res_json.get("type") == "whartonUGR"
-        except APIError:
-            raise
-        except Exception as e:
-            raise APIError(f"Wharton: Error checking privileges: {str(e)}")
+        except APIError as e:
+            logger.error(f"Wharton API error for {user.username}: {e}")
+            return False
 
 
 class PennGroupsBookingWrapper(AbstractBookingWrapper):
@@ -280,12 +289,17 @@ def get_authorized_rooms(self, user):
             raise APIError(f"PennGroups: Error accessing API: {str(e)}")
 
     def is_seas(self, user):
-        """Check if user has SEAS status"""
+        """
+        Check if user has SEAS status.
+        Returns False if user doesn't have SEAS access or if API fails.
+        Logs errors for monitoring.
+        """
         try:
             rooms = self.get_authorized_rooms(user)
             return len(rooms) > 0
-        except APIError:
-            raise
+        except APIError as e:
+            logger.error(f"PennGroups API error for {user.username}: {e}")
+            return False
 
     def extract_room_number(self, libcal_name):
         """Extract room number from LibCal name like 'AGH 334' -> '334' or 'AGH 300A' -> '300A'"""

backend/gsr_booking/management/commands/add_pennlabs_users.py

@@ -35,6 +35,7 @@ def handle(self, *args, **options):
             except User.DoesNotExist:
                 self.stdout.write(f"User with PennKey {pennkey} does not exist. Skipping.")
                 continue
+
             users.append(user)
             is_wharton = WhartonGSRBooker.is_wharton(user)
             wharton_statuses.append(is_wharton)

backend/gsr_booking/management/commands/update_seas_status.py

@@ -11,14 +11,19 @@ class Command(BaseCommand):
     def handle(self, *args, **kwargs):
         users = GroupMembership.objects.values_list("user__username", flat=True).distinct()
         print(f"Checking {len(users)} users...")
+        penngroups_wrapper = PennGroupsBookingWrapper()
+        updated = 0
+
         for username in users:
             user = get_user_model().objects.get(username=username)
-            is_seas = PennGroupsBookingWrapper().is_seas(user)
+            is_seas = penngroups_wrapper.is_seas(user)
             memberships = GroupMembership.objects.filter(user__username=user)
             for membership in memberships:
                 if membership.is_seas != is_seas:
                     membership.is_seas = is_seas
                     membership.save()
                     status = "now" if is_seas else "no longer"
                     print(f"User {user} is {status} a SEAS user.")
-        print("Done updating SEAS statuses.")
+                    updated += 1
+
+        print(f"Done updating SEAS statuses. Updated: {updated} users.")

backend/gsr_booking/management/commands/update_wharton_status.py

@@ -11,14 +11,19 @@ class Command(BaseCommand):
     def handle(self, *args, **kwargs):
         users = GroupMembership.objects.values_list("user__username", flat=True).distinct()
         print(f"Checking {len(users)} users...")
+        wharton_wrapper = WhartonBookingWrapper()
+        updated = 0
+
         for username in users:
             user = get_user_model().objects.get(username=username)
-            is_wharton = WhartonBookingWrapper().is_wharton(user)
+            is_wharton = wharton_wrapper.is_wharton(user)
             memberships = GroupMembership.objects.filter(user__username=user)
             for membership in memberships:
                 if membership.is_wharton != is_wharton:
                     membership.is_wharton = is_wharton
                     membership.save()
                     status = "now" if is_wharton else "no longer"
                     print(f"User {user} is {status} a Wharton user.")
-        print("Done updating Wharton statuses.")
+                    updated += 1
+
+        print(f"Done updating Wharton statuses. Updated: {updated} users.")

backend/gsr_booking/models.py

@@ -7,8 +7,6 @@
 from django.dispatch import receiver
 from django.utils import timezone
 
-from utils.errors import APIError
-
 
 User = get_user_model()
 
@@ -54,16 +52,10 @@ def save(self, *args, **kwargs):
         super().save(*args, **kwargs)
 
     def check_wharton(self):
-        try:
-            return WhartonGSRBooker.is_wharton(self.user)
-        except APIError:
-            return False
+        return WhartonGSRBooker.is_wharton(self.user)
 
     def check_seas(self):
-        try:
-            return PennGroupsGSRBooker.is_seas(self.user)
-        except APIError:
-            return False
+        return PennGroupsGSRBooker.is_seas(self.user)
 
     class Meta:
         verbose_name = "Group Membership"

backend/tests/gsr_booking/test_gsr_views.py

@@ -7,7 +7,6 @@
 from django.urls import reverse
 from rest_framework.test import APIClient
 
-from gsr_booking.api_wrapper import APIError
 from gsr_booking.models import GSR, Group, GroupMembership, GSRBooking
 
 
@@ -211,14 +210,14 @@ def test_get_location_penn_labs_member(self):
         self.assertIn(self.agh_gsr.id, gsr_ids, "AGH GSR should be visible")
         self.assertIn(self.weigle_gsr.id, gsr_ids, "Weigle GSR should be visible")
 
-    @mock.patch("gsr_booking.views.WhartonGSRBooker.is_wharton", side_effect=APIError("API Error"))
-    @mock.patch("gsr_booking.views.PennGroupsGSRBooker.is_seas", side_effect=APIError("API Error"))
+    @mock.patch("gsr_booking.views.WhartonGSRBooker.is_wharton", return_value=False)
+    @mock.patch("gsr_booking.views.PennGroupsGSRBooker.is_seas", return_value=False)
     def test_get_location_api_error_handling(self, mock_is_seas, mock_is_wharton):
-        """Test that API errors are handled gracefully and users still see LibCal GSRs"""
+        """Test that when permission checks return False, users only see LibCal GSRs"""
         response = self.client.get(reverse("locations"))
         res_json = json.loads(response.content)
 
-        # Even if API calls fail, users should still see LibCal GSRs
+        # When permission checks return False, users should only see LibCal GSRs
         for entry in res_json:
             self.assertIn("kind", entry)
             self.assertEqual(entry["kind"], GSR.KIND_LIBCAL)