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

feat: User agreements API for generic agreement records #35895

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from
Draft
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
57 changes: 51 additions & 6 deletions openedx/core/djangoapps/agreements/api.py
Original file line number Diff line number Diff line change
Expand Up @@ -3,17 +3,15 @@
"""

import logging
from datetime import datetime
from typing import Iterable, Optional

from django.contrib.auth import get_user_model
from django.core.exceptions import ObjectDoesNotExist
from opaque_keys.edx.keys import CourseKey

from openedx.core.djangoapps.agreements.models import IntegritySignature
from openedx.core.djangoapps.agreements.models import LTIPIITool
from openedx.core.djangoapps.agreements.models import LTIPIISignature

from .data import LTIToolsReceivingPIIData
from .data import LTIPIISignatureData
from .data import LTIPIISignatureData, LTIToolsReceivingPIIData, UserAgreementRecordData
from .models import IntegritySignature, LTIPIISignature, LTIPIITool, UserAgreementRecord

log = logging.getLogger(__name__)
User = get_user_model()
Expand Down Expand Up @@ -240,3 +238,50 @@ def _user_signature_out_of_date(username, course_id):
return False
else:
return user_lti_pii_signature_hash != course_lti_pii_tools_hash


def get_user_agreements(user: User) -> Iterable[UserAgreementRecordData]:
"""
Retrieves all the agreements that the specified user has acknowledged.
"""
for agreement_record in UserAgreementRecord.objects.filter(user=user):
yield UserAgreementRecordData.from_model(agreement_record)


def get_user_agreement_record(
user: User,
agreement_type: str,
agreement_update_timestamp: datetime = None,
) -> Optional[UserAgreementRecordData]:
"""
Retrieve the user agreement record for the specified user and agreement type.
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,
)
if agreement_update_timestamp:
record_query = record_query.filter(timestamp__gte=agreement_update_timestamp)
record = record_query.get()
return UserAgreementRecordData.from_model(record)
except UserAgreementRecord.DoesNotExist:
return None


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.
"""
record, _ = UserAgreementRecord.objects.update_or_create(
Copy link
Contributor

@kaustavb12 kaustavb12 Nov 28, 2024

Choose a reason for hiding this comment

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

Since the primary use of this record would be for generating complaince reports, I feel we should only create records here and not update them. This would also be useful if, for example, some user agreement needs to be accepted periodically.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The idea here is that if you have an agreement, let's say "termsofservice", that can be updated, you can check if a user has accepted the agreement and also if they've accepted the latest agreement. For a report you can simply check which user's have a record for the agreement created after the date of the new agreement. If it needs to be accepted periodically then you can simply check in the frontend that the agreement was accepted in the current period.

Copy link
Contributor

Choose a reason for hiding this comment

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

The idea here is that if you have an agreement, let's say "termsofservice", that can be updated, you can check if a user has accepted the agreement and also if they've accepted the latest agreement

We can do that just the same even if we keep all records and not update them. No ?

In fact, specifically for the "terms of sevice" example, keeping all old records would enable someone doing an audit to determine under which version of the terms did an old user activity (for example if the user uploaded copyright protected files) take place.

I can see how old records can be retained even with update_or_create function by perhaps changing the agreement type for each version of the agreement, but it seems right to retain things like agreement records instead of updating them.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't think automatically changing the agreement type is a good idea. It's definitely an option that can be adopted by consumers of these APIs. For instance, you could have an agreement type of "toc-2024" and then "toc-2026" and they will both be tracked separately.

However, I guess I'm just not seeing what you're talking about as a realistic scenario in this case. What you are talking about is a more complex requirement for auditing and I don't think this is trying to be that.

What I'm looking at is a few simple scenarios. A ToS agreement, a fair use agreement etc. All you want to know is if the person has agreed to the current form of the agreement. You can do that by recording the date on which the updated agreement was posted and comparing against the date that the user last accepted the agreement. If they accepted after the latest agreement was posted, then they are good to go, otherwise, ask them to accept the agreement again.

If you want to, you can make the logic more complex and check if there have been multiple updates to the agreement since their last acceptance, and if so, show then a UI that shows the differences between versions. Or at the very least show an "our terms have changed" UI.

In the scenario you shared, it probably doesn't matter in what duration those files were posted, as long as they were posted before the current agreement, because they might not align with it.

I don't think it would be a big deal or a lot of effort to adapt this code to keep one record for each acceptance, but currently this will scale with users * agreements and with your proposed change it will scale with users * agreements * agreement change and someone could simply code a loop posting agreements and flood the db.

I guess they still can because the agreement type is not validated, so perhaps that is one way to go next.

Copy link
Contributor

Choose a reason for hiding this comment

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

However, I guess I'm just not seeing what you're talking about as a realistic scenario in this case. What you are talking about is a more complex requirement for auditing and I don't think this is trying to be that.

I was basing my assumption about auditing requirements based on this portion of the client's requirements which triggered this PR :

  • Log user interactions with the agreement (e.g., acceptance date) for auditing and compliance.
  • Enable report generation to check who has accepted the agreement within a given timeframe.

It is a bit unclear in these statements as to how complex these "auditing and compliance" requirements are, and thought maintaining all records would be a safer option (without needing too much changes to the current implementation) which would provide users the flexibility to go beyond simple acceptance checking if need be.

But for now I guess I am ok with moving forward with the implementation as is. We can always change it later if so required.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I interpreted that as log the acceptance date not log every acceptance date, however, maybe I'm wrong and we can consult with them to clarify.

For the report generation they only wanted that it be possible for them to do so using SQL on their side.

user=user,
agreement_type=agreement_type,
defaults={
"timestamp": datetime.now(),
},
)
return UserAgreementRecordData.from_model(record)
23 changes: 23 additions & 0 deletions openedx/core/djangoapps/agreements/data.py
Original file line number Diff line number Diff line change
@@ -1,8 +1,13 @@
"""
Public data structures for this app.
"""
from dataclasses import dataclass
from datetime import datetime

import attr

from .models import UserAgreementRecord


@attr.s(frozen=True, auto_attribs=True)
class LTIToolsReceivingPIIData:
Expand All @@ -21,3 +26,21 @@ class LTIPIISignatureData:
course_id: str
lti_tools: str
lti_tools_hash: str


@dataclass
class UserAgreementRecordData:
"""
Data for a single user agreement record.
"""
username: str
agreement_type: str
accepted_at: datetime

@classmethod
def from_model(cls, model: UserAgreementRecord):
return UserAgreementRecordData(
username=model.user.username,
agreement_type=model.agreement_type,
accepted_at=model.timestamp,
)
Original file line number Diff line number Diff line change
@@ -0,0 +1,28 @@
# Generated by Django 4.2.16 on 2024-11-14 11:47

from django.conf import settings
from django.db import migrations, models
import django.db.models.deletion


class Migration(migrations.Migration):

dependencies = [
migrations.swappable_dependency(settings.AUTH_USER_MODEL),
('agreements', '0005_timestampedmodels'),
]

operations = [
migrations.CreateModel(
name='UserAgreementRecord',
fields=[
('id', models.AutoField(auto_created=True, primary_key=True, serialize=False, verbose_name='ID')),
('agreement_type', models.CharField(max_length=255)),
('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')},
},
),
]
19 changes: 19 additions & 0 deletions openedx/core/djangoapps/agreements/models.py
Original file line number Diff line number Diff line change
Expand Up @@ -70,3 +70,22 @@ class ProctoringPIISignature(TimeStampedModel):

class Meta:
app_label = 'agreements'


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`.
.. no_pii:
"""
user = models.ForeignKey(User, db_index=True, on_delete=models.CASCADE)
agreement_type = models.CharField(max_length=255)
timestamp = models.DateTimeField(auto_now_add=True)

class Meta:
app_label = 'agreements'
# A user can only have a single record for a single agreement type.
unique_together = [['user', 'agreement_type']]
12 changes: 11 additions & 1 deletion openedx/core/djangoapps/agreements/serializers.py
Original file line number Diff line number Diff line change
Expand Up @@ -3,9 +3,10 @@
"""
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):
"""
Expand All @@ -31,3 +32,12 @@ class LTIPIISignatureSerializer(serializers.ModelSerializer):
class Meta:
model = LTIPIISignature
fields = ('username', 'course_id', 'lti_tools', 'created_at')


class UserAgreementsSerializer(serializers.Serializer):
"""
Serializer for UserAgreementRecord model
"""
accepted_at = serializers.DateTimeField()
xitij2000 marked this conversation as resolved.
Show resolved Hide resolved
agreement_type = serializers.CharField(read_only=True)
username = serializers.CharField(read_only=True)
58 changes: 48 additions & 10 deletions openedx/core/djangoapps/agreements/tests/test_api.py
Original file line number Diff line number Diff line change
Expand Up @@ -2,25 +2,29 @@
Tests for the Agreements API
"""
import logging
from datetime import datetime, timedelta

from django.test import TestCase
from opaque_keys.edx.keys import CourseKey
from testfixtures import LogCapture

from common.djangoapps.student.tests.factories import UserFactory
from openedx.core.djangoapps.agreements.api import (
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 (
create_integrity_signature,
create_lti_pii_signature,
create_user_agreement_record,
get_integrity_signature,
get_integrity_signatures_for_course,
get_lti_pii_signature,
get_pii_receiving_lti_tools,
create_lti_pii_signature,
get_lti_pii_signature
get_user_agreement_record,
get_user_agreements
)
from openedx.core.djangolib.testing.utils import skip_unless_lms
from xmodule.modulestore.tests.django_utils import SharedModuleStoreTestCase # lint-amnesty, pylint: disable=wrong-import-order
from xmodule.modulestore.tests.factories import CourseFactory # lint-amnesty, pylint: disable=wrong-import-order
from ..models import (
LTIPIITool,
)
from opaque_keys.edx.keys import CourseKey
from ..models import LTIPIITool

LOGGER_NAME = "openedx.core.djangoapps.agreements.api"

Expand Down Expand Up @@ -186,3 +190,37 @@ def _assert_ltitools(self, lti_list):
Helper function to assert the returned list has the correct tools
"""
self.assertEqual(self.lti_tools, lti_list)


@skip_unless_lms
class UserAgreementsTests(TestCase):
"""
Tests for the python APIs related to user agreements.
"""
def setUp(self):
self.user = UserFactory()

def test_get_user_agreements(self, ):
result = list(get_user_agreements(self.user))
assert len(result) == 0

record = create_user_agreement_record(self.user, 'test_type')
result = list(get_user_agreements(self.user))

assert len(result) == 1
assert result[0].agreement_type == 'test_type'
assert result[0].username == self.user.username
assert result[0].accepted_at == record.accepted_at

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')

assert result == record

result = get_user_agreement_record(self.user, 'test_type', datetime.now() + timedelta(days=1))

assert result is None

def tearDown(self):
self.user.delete()
64 changes: 56 additions & 8 deletions openedx/core/djangoapps/agreements/tests/test_views.py
Original file line number Diff line number Diff line change
Expand Up @@ -2,26 +2,28 @@
Tests for agreements views
"""

import json
from datetime import datetime, timedelta
from unittest.mock import patch

from django.conf import settings
from django.urls import reverse
from rest_framework.test import APITestCase
from rest_framework import status
from freezegun import freeze_time
import json
from rest_framework import status
from rest_framework.test import APITestCase

from common.djangoapps.student.tests.factories import UserFactory, AdminFactory
from common.djangoapps.student.roles import CourseStaffRole
from openedx.core.djangoapps.agreements.api import (
from common.djangoapps.student.tests.factories import AdminFactory, UserFactory
from openedx.core.djangolib.testing.utils import skip_unless_lms
from xmodule.modulestore.tests.django_utils import ModuleStoreTestCase
from xmodule.modulestore.tests.factories import CourseFactory

from ..api import (
create_integrity_signature,
create_user_agreement_record,
get_integrity_signatures_for_course,
get_lti_pii_signature
)
from openedx.core.djangolib.testing.utils import skip_unless_lms
from xmodule.modulestore.tests.django_utils import ModuleStoreTestCase # lint-amnesty, pylint: disable=wrong-import-order
from xmodule.modulestore.tests.factories import CourseFactory # lint-amnesty, pylint: disable=wrong-import-order


@skip_unless_lms
Expand Down Expand Up @@ -289,3 +291,49 @@ def test_post_lti_pii_signature(self):
signature = get_lti_pii_signature(self.user.username, self.course_id)
self.assertEqual(signature.user.username, self.user.username)
self.assertEqual(signature.lti_tools, self.lti_tools)


@skip_unless_lms
class UserAgreementsViewTests(APITestCase):
"""
Tests for the UserAgreementsView
"""

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'})

def test_get_user_agreement_record_no_data(self):
response = self.client.get(self.url)
assert response.status_code == status.HTTP_404_NOT_FOUND

def test_get_user_agreement_record_invalid_date(self):
response = self.client.get(self.url, {'after': 'invalid_date'})
assert response.status_code == status.HTTP_400_BAD_REQUEST

def test_get_user_agreement_record(self):
create_user_agreement_record(self.user, 'sample_agreement')
response = self.client.get(self.url)
assert response.status_code == status.HTTP_200_OK
assert 'accepted_at' in response.data

response = self.client.get(self.url, {"after": str(datetime.now() + timedelta(days=1))})
assert response.status_code == status.HTTP_404_NOT_FOUND

def test_post_user_agreement(self):
with freeze_time("2024-11-21 12:00:00"):
response = self.client.post(self.url)
assert response.status_code == status.HTTP_201_CREATED

response = self.client.get(self.url)
assert response.status_code == status.HTTP_200_OK

response = self.client.get(self.url, {"after": "2024-11-21T13:00:00Z"})
assert response.status_code == status.HTTP_404_NOT_FOUND

response = self.client.post(self.url)
assert response.status_code == status.HTTP_201_CREATED

response = self.client.get(self.url, {"after": "2024-11-21T13:00:00Z"})
assert response.status_code == status.HTTP_200_OK
5 changes: 3 additions & 2 deletions openedx/core/djangoapps/agreements/urls.py
Original file line number Diff line number Diff line change
Expand Up @@ -3,9 +3,9 @@
"""

from django.conf import settings
from django.urls import re_path
from django.urls import path, re_path

from .views import IntegritySignatureView, LTIPIISignatureView
from .views import IntegritySignatureView, LTIPIISignatureView, UserAgreementsView

urlpatterns = [
re_path(r'^integrity_signature/{course_id}$'.format(
Expand All @@ -14,4 +14,5 @@
re_path(r'^lti_pii_signature/{course_id}$'.format(
course_id=settings.COURSE_ID_PATTERN
), LTIPIISignatureView.as_view(), name='lti_pii_signature'),
path("agreement/<slug:agreement_type>", UserAgreementsView.as_view(), name="user_agreements"),
]
Loading
Loading