fix: fixes based on review feedback, tests etc

Co-authored-by: Farhaan Bukhsh <[email protected]>

Author: xitij2000

openedx/core/djangoapps/agreements/admin.py

@@ -3,10 +3,14 @@
 """
 
 from django.contrib import admin
-from openedx.core.djangoapps.agreements.models import IntegritySignature, UserAgreement
-from openedx.core.djangoapps.agreements.models import LTIPIITool
-from openedx.core.djangoapps.agreements.models import LTIPIISignature
-from openedx.core.djangoapps.agreements.models import ProctoringPIISignature
+
+from openedx.core.djangoapps.agreements.models import (
+    IntegritySignature,
+    LTIPIISignature,
+    LTIPIITool,
+    ProctoringPIISignature,
+    UserAgreement
+)
 
 
 class IntegritySignatureAdmin(admin.ModelAdmin):

openedx/core/djangoapps/agreements/api.py

@@ -4,14 +4,23 @@
 
 import logging
 from datetime import datetime
-from typing import Iterable, Optional
+from typing import Optional, Iterator
 
 from django.contrib.auth import get_user_model
 from django.core.exceptions import ObjectDoesNotExist
 from opaque_keys.edx.keys import CourseKey
 
-from .data import LTIPIISignatureData, LTIToolsReceivingPIIData, UserAgreementRecordData
-from .models import IntegritySignature, LTIPIISignature, LTIPIITool, UserAgreementRecord
+from openedx.core.djangoapps.agreements.data import (
+    LTIPIISignatureData,
+    LTIToolsReceivingPIIData,
+    UserAgreementRecordData
+)
+from openedx.core.djangoapps.agreements.models import (
+    IntegritySignature,
+    LTIPIISignature,
+    LTIPIITool,
+    UserAgreementRecord, UserAgreement
+)
 
 log = logging.getLogger(__name__)
 User = get_user_model()
@@ -240,11 +249,11 @@ def _user_signature_out_of_date(username, course_id):
         return user_lti_pii_signature_hash != course_lti_pii_tools_hash
 
 
-def get_user_agreement_records(user: User) -> Iterable[UserAgreementRecordData]:
+def get_user_agreement_records(user: User) -> Iterator[UserAgreementRecordData]:
     """
     Retrieves all the agreements that the specified user has acknowledged.
     """
-    for agreement_record in UserAgreementRecord.objects.filter(user=user).select_related("agreement"):
+    for agreement_record in UserAgreementRecord.objects.filter(user=user).select_related("agreement", "user"):
         yield UserAgreementRecordData.from_model(agreement_record)
 
 
@@ -258,25 +267,26 @@ def get_latest_user_agreement_record(
     An agreement update timestamp can be provided to return a record only if it
     was signed after that timestamp.
     """
-    try:
-        record_query = UserAgreementRecord.objects.filter(
-            user=user,
-            agreement__type=agreement_type,
-        )
-        record = record_query.latest("timestamp")
-        return UserAgreementRecordData.from_model(record)
-    except UserAgreementRecord.DoesNotExist:
-        return None
+    record_query = UserAgreementRecord.objects.filter(
+        user=user,
+        agreement__type=agreement_type,
+    )
+    if record_query.exists():
+        return UserAgreementRecordData.from_model(record_query.latest("timestamp"))
+    return UserAgreementRecordData(
+        username=user.get_username(),
+        agreement_type=agreement_type,
+    )
 
 
 def create_user_agreement_record(user: User, agreement_type: str) -> UserAgreementRecordData:
     """
-    Creates a user agreement record if one doesn't already exist, or updates existing
-    record to current timestamp.
+    Creates a user agreement record with current timestamp.
     """
+    agreement = UserAgreement.objects.get(type=agreement_type)
     record = UserAgreementRecord.objects.create(
         user=user,
-        agreement__type=agreement_type,
+        agreement=agreement,
         timestamp=datetime.now(),
     )
     return UserAgreementRecordData.from_model(record)

openedx/core/djangoapps/agreements/data.py

@@ -6,7 +6,7 @@
 
 import attr
 
-from .models import UserAgreementRecord, UserAgreement
+from openedx.core.djangoapps.agreements.models import UserAgreement, UserAgreementRecord
 
 
 @attr.s(frozen=True, auto_attribs=True)
@@ -28,7 +28,6 @@ class LTIPIISignatureData:
     lti_tools_hash: str
 
 
-
 @dataclass
 class UserAgreementData:
     """
@@ -37,28 +36,29 @@ class UserAgreementData:
     type: str
     name: str
     summary: str
-    text: str|None
-    url: str|None
+    has_text: bool
+    url: str | None
 
     @classmethod
     def from_model(cls, model: UserAgreement):
         return UserAgreementData(
             type=model.type,
             name=model.name,
             summary=model.summary,
-            text=model.text,
-            url=model.url
+            url=model.url,
+            has_text=bool(model.text),
         )
 
+
 @dataclass
 class UserAgreementRecordData:
     """
     Data for a single user agreement record.
     """
     username: str
     agreement_type: str
-    accepted_at: datetime
-    is_current: bool = True
+    accepted_at: datetime | None = None
+    is_current: bool = False
 
     @classmethod
     def from_model(cls, model: UserAgreementRecord):

openedx/core/djangoapps/agreements/models.py

@@ -75,15 +75,15 @@ class Meta:
 
 class UserAgreement(models.Model):
     """
-    This model stores agreements that as user can accept that can gate certain
+    This model stores agreements that the user can accept, which can gate certain
     platform features.
 
     .. no_pii:
     """
     type = models.CharField(max_length=255, unique=True)
     name = models.CharField(
         max_length=255,
-        help_text='Human-readable name for the agreement type. Will be displayed to users in alert to accept the agreement.',
+        help_text='Human-readable name for the agreement type. Will be displayed to users in an alert to accept/reject the agreement.',
     )
     summary = models.TextField(
         max_length=1024,

openedx/core/djangoapps/agreements/serializers.py

@@ -3,10 +3,9 @@
 """
 from rest_framework import serializers
 
+from openedx.core.djangoapps.agreements.models import IntegritySignature, LTIPIISignature
 from openedx.core.lib.api.serializers import CourseKeyField
 
-from .models import IntegritySignature, LTIPIISignature
-
 
 class IntegritySignatureSerializer(serializers.ModelSerializer):
     """
@@ -41,7 +40,7 @@ class UserAgreementSerializer(serializers.Serializer):
     type = serializers.CharField(read_only=True)
     name = serializers.CharField(read_only=True)
     summary = serializers.CharField(read_only=True)
-    text = serializers.CharField(read_only=True)
+    has_text = serializers.BooleanField(read_only=True)
     url = serializers.URLField(read_only=True)
     updated = serializers.DateTimeField(read_only=True)
 
@@ -53,3 +52,4 @@ class UserAgreementRecordSerializer(serializers.Serializer):
     username = serializers.CharField(read_only=True)
     agreement_type = serializers.CharField(read_only=True)
     accepted_at = serializers.DateTimeField()
+    is_current = serializers.BooleanField(read_only=True)

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

@@ -9,22 +9,21 @@
 from testfixtures import LogCapture
 
 from common.djangoapps.student.tests.factories import UserFactory
-from openedx.core.djangolib.testing.utils import skip_unless_lms
-from xmodule.modulestore.tests.django_utils import SharedModuleStoreTestCase
-from xmodule.modulestore.tests.factories import CourseFactory
-
-from ..api import (
+from openedx.core.djangoapps.agreements.api import (
     create_integrity_signature,
     create_lti_pii_signature,
     create_user_agreement_record,
     get_integrity_signature,
     get_integrity_signatures_for_course,
+    get_latest_user_agreement_record,
     get_lti_pii_signature,
     get_pii_receiving_lti_tools,
-    get_latest_user_agreement_record,
     get_user_agreement_records
 )
-from ..models import LTIPIITool
+from openedx.core.djangoapps.agreements.models import LTIPIITool, UserAgreement
+from openedx.core.djangolib.testing.utils import skip_unless_lms
+from xmodule.modulestore.tests.django_utils import SharedModuleStoreTestCase
+from xmodule.modulestore.tests.factories import CourseFactory
 
 LOGGER_NAME = "openedx.core.djangoapps.agreements.api"
 
@@ -199,6 +198,14 @@ class UserAgreementsTests(TestCase):
     """
     def setUp(self):
         self.user = UserFactory()
+        self.agreement = UserAgreement.objects.create(
+            type='test_type',
+            name='test agreement',
+            summary='test summary',
+            url='https://example.com',
+            text='test text',
+            updated=datetime.now(),
+        )
 
     def test_get_user_agreements(self, ):
         result = list(get_user_agreement_records(self.user))
@@ -218,9 +225,13 @@ def test_get_user_agreement_record(self):
 
         assert result == record
 
-        result = get_latest_user_agreement_record(self.user, 'test_type', datetime.now() + timedelta(days=1))
+        self.agreement.updated = datetime.now() + timedelta(days=1)
+        self.agreement.save()
+
+        result = get_latest_user_agreement_record(self.user, 'test_type')
 
-        assert result is None
+        assert result.is_current is False
 
     def tearDown(self):
         self.user.delete()
+        self.agreement.delete()

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

@@ -1,9 +1,11 @@
 """
 Tests for Agreements models
 """
+from datetime import datetime
 
 from django.db import IntegrityError
 from django.test import TestCase
+
 from openedx.core.djangoapps.agreements.models import UserAgreement
 
 
@@ -21,7 +23,8 @@ def test_agreement_must_have_text_or_url(self):
             name='Name 1',
             summary='Summary 1',
             text='Some text',
-            url='https://example.com'
+            url='https://example.com',
+            updated=datetime.now(),
         )
         self.assertIsNotNone(agreement.pk)
 
@@ -31,7 +34,8 @@ def test_agreement_must_have_text_or_url(self):
             name='Name 2',
             summary='Summary 2',
             text='Some text',
-            url=None
+            url=None,
+            updated=datetime.now(),
         )
         self.assertIsNotNone(agreement.pk)
 
@@ -41,7 +45,8 @@ def test_agreement_must_have_text_or_url(self):
             name='Name 3',
             summary='Summary 3',
             text=None,
-            url='https://example.com'
+            url='https://example.com',
+            updated=datetime.now(),
         )
         self.assertIsNotNone(agreement.pk)
 
@@ -52,7 +57,8 @@ def test_agreement_must_have_text_or_url(self):
                 name='Name 4',
                 summary='Summary 4',
                 text=None,
-                url=None
+                url=None,
+                updated=datetime.now(),
             )
 
     def test_agreement_with_empty_strings(self):
@@ -67,7 +73,8 @@ def test_agreement_with_empty_strings(self):
             name='Name 5',
             summary='Summary 5',
             text='',
-            url=None
+            url=None,
+            updated=datetime.now(),
         )
         self.assertIsNotNone(agreement.pk)
 
@@ -77,6 +84,7 @@ def test_agreement_with_empty_strings(self):
             name='Name 6',
             summary='Summary 6',
             text=None,
-            url=''
+            url='',
+            updated=datetime.now(),
         )
         self.assertIsNotNone(agreement.pk)