fixup!: apply review fixes

Author: xitij2000

openedx/core/djangoapps/agreements/api.py

@@ -248,10 +248,10 @@ def get_user_agreements(user: User) -> Iterable[UserAgreementRecordData]:
         yield UserAgreementRecordData.from_model(agreement_record)
 
 
-def get_user_agreement_record(
+def get_latest_user_agreement_record(
     user: User,
     agreement_type: str,
-    agreement_update_timestamp: datetime = None,
+    agreed_after: datetime = None,
 ) -> Optional[UserAgreementRecordData]:
     """
     Retrieve the user agreement record for the specified user and agreement type.
@@ -264,9 +264,9 @@ def get_user_agreement_record(
             user=user,
             agreement_type=agreement_type,
         )
-        if agreement_update_timestamp:
-            record_query = record_query.filter(timestamp__gte=agreement_update_timestamp)
-        record = record_query.get()
+        if agreed_after:
+            record_query = record_query.filter(timestamp__gte=agreed_after)
+        record = record_query.latest("timestamp")
         return UserAgreementRecordData.from_model(record)
     except UserAgreementRecord.DoesNotExist:
         return None
@@ -277,11 +277,9 @@ def create_user_agreement_record(user: User, agreement_type: str) -> UserAgreeme
     Creates a user agreement record if one doesn't already exist, or updates existing
     record to current timestamp.
     """
-    record, _ = UserAgreementRecord.objects.update_or_create(
+    record = UserAgreementRecord.objects.create(
         user=user,
         agreement_type=agreement_type,
-        defaults={
-            "timestamp": datetime.now(),
-        },
+        timestamp=datetime.now(),
     )
     return UserAgreementRecordData.from_model(record)

openedx/core/djangoapps/agreements/migrations/0006_useragreementrecord.py

@@ -1,4 +1,4 @@
-# Generated by Django 4.2.16 on 2024-11-14 11:47
+# Generated by Django 4.2.16 on 2024-12-06 11:34
 
 from django.conf import settings
 from django.db import migrations, models
@@ -21,8 +21,5 @@ class Migration(migrations.Migration):
                 ('timestamp', models.DateTimeField(auto_now_add=True)),
                 ('user', models.ForeignKey(on_delete=django.db.models.deletion.CASCADE, to=settings.AUTH_USER_MODEL)),
             ],
-            options={
-                'unique_together': {('user', 'agreement_type')},
-            },
         ),
     ]

openedx/core/djangoapps/agreements/models.py

@@ -77,7 +77,7 @@ class UserAgreementRecord(models.Model):
     This model stores the agreements a user has accepted or acknowledged.
 
     Each record here represents a user agreeing to the agreement type represented
-    by `agreement_type`.
+    by `agreement_type` at a particular time.
 
     .. no_pii:
     """
@@ -87,5 +87,3 @@ class UserAgreementRecord(models.Model):
 
     class Meta:
         app_label = 'agreements'
-        # A user can only have a single record for a single agreement type.
-        unique_together = [['user', 'agreement_type']]

openedx/core/djangoapps/agreements/serializers.py

@@ -38,6 +38,6 @@ class UserAgreementsSerializer(serializers.Serializer):
     """
     Serializer for UserAgreementRecord model
     """
-    accepted_at = serializers.DateTimeField()
-    agreement_type = serializers.CharField(read_only=True)
     username = serializers.CharField(read_only=True)
+    agreement_type = serializers.CharField(read_only=True)
+    accepted_at = serializers.DateTimeField()

openedx/core/djangoapps/agreements/tests/test_api.py

@@ -21,7 +21,7 @@
     get_integrity_signatures_for_course,
     get_lti_pii_signature,
     get_pii_receiving_lti_tools,
-    get_user_agreement_record,
+    get_latest_user_agreement_record,
     get_user_agreements
 )
 from ..models import LTIPIITool
@@ -214,11 +214,11 @@ def test_get_user_agreements(self, ):
 
     def test_get_user_agreement_record(self):
         record = create_user_agreement_record(self.user, 'test_type')
-        result = get_user_agreement_record(self.user, 'test_type')
+        result = get_latest_user_agreement_record(self.user, 'test_type')
 
         assert result == record
 
-        result = get_user_agreement_record(self.user, 'test_type', datetime.now() + timedelta(days=1))
+        result = get_latest_user_agreement_record(self.user, 'test_type', datetime.now() + timedelta(days=1))
 
         assert result is None
 

openedx/core/djangoapps/agreements/tests/test_views.py

@@ -301,8 +301,11 @@ class UserAgreementsViewTests(APITestCase):
 
     def setUp(self):
         self.user = UserFactory(username="testuser", password="password")
-        self.client.login(username="testuser", password="password")
         self.url = reverse('user_agreements', kwargs={'agreement_type': 'sample_agreement'})
+        self.login()
+
+    def login(self):
+        self.client.login(username="testuser", password="password")
 
     def test_get_user_agreement_record_no_data(self):
         response = self.client.get(self.url)
@@ -326,6 +329,8 @@ def test_post_user_agreement(self):
             response = self.client.post(self.url)
         assert response.status_code == status.HTTP_201_CREATED
 
+        self.login()
+
         response = self.client.get(self.url)
         assert response.status_code == status.HTTP_200_OK
 

openedx/core/djangoapps/agreements/views.py

@@ -20,9 +20,10 @@
     create_lti_pii_signature,
     create_user_agreement_record,
     get_integrity_signature,
-    get_user_agreement_record
+    get_latest_user_agreement_record
 )
 from .serializers import IntegritySignatureSerializer, LTIPIISignatureSerializer, UserAgreementsSerializer
+from ...lib.api.view_utils import view_auth_classes
 
 
 def is_user_course_or_global_staff(user, course_id):
@@ -167,7 +168,8 @@ def post(self, request, course_id):
         return Response(data=serializer.data, status=statusStr)
 
 
-class UserAgreementsView(AuthenticatedAPIView):
+@view_auth_classes(is_authenticated=True)
+class UserAgreementsView(APIView):
     """
     Endpoint for the user agreements API.
     """
@@ -207,7 +209,7 @@ def get(self, request, agreement_type):
         params = UserAgreementsView.QueryFilterForm(request.query_params)
         if not params.is_valid():
             return Response(status=status.HTTP_400_BAD_REQUEST)
-        record = get_user_agreement_record(request.user, agreement_type, params.cleaned_data.get('after'))
+        record = get_latest_user_agreement_record(request.user, agreement_type, params.cleaned_data.get('after'))
         if record is None:
             return Response(status=status.HTTP_404_NOT_FOUND)
         serializer = UserAgreementsSerializer(record)