-
Notifications
You must be signed in to change notification settings - Fork 4.2k
feat: User agreements API for generic agreement records #35895
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
base: master
Are you sure you want to change the base?
Changes from all commits
3b51154
1abd876
70cf60b
152a2fc
e39370c
4f136c5
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -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() | ||||||||||||||||||||||||||||||||||||||
|
|
@@ -240,3 +238,45 @@ 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_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", "user"): | ||||||||||||||||||||||||||||||||||||||
| yield UserAgreementRecordData.from_model(agreement_record) | ||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||
| def get_latest_user_agreement_record( | ||||||||||||||||||||||||||||||||||||||
| user: User, | ||||||||||||||||||||||||||||||||||||||
| agreement_type: str, | ||||||||||||||||||||||||||||||||||||||
| ) -> 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, | ||||||||||||||||||||||||||||||||||||||
| ) | ||||||||||||||||||||||||||||||||||||||
| record = record_query.latest("timestamp") | ||||||||||||||||||||||||||||||||||||||
| return UserAgreementRecordData.from_model(record) | ||||||||||||||||||||||||||||||||||||||
| except UserAgreementRecord.DoesNotExist: | ||||||||||||||||||||||||||||||||||||||
| return None | ||||||||||||||||||||||||||||||||||||||
|
Comment on lines
+261
to
+269
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||
| 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. | ||||||||||||||||||||||||||||||||||||||
|
Comment on lines
+274
to
+275
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If we are creating or updating, should we use create_or_update instead? ref: https://docs.djangoproject.com/en/6.0/ref/models/querysets/#update-or-create |
||||||||||||||||||||||||||||||||||||||
| """ | ||||||||||||||||||||||||||||||||||||||
| record = UserAgreementRecord.objects.create( | ||||||||||||||||||||||||||||||||||||||
| user=user, | ||||||||||||||||||||||||||||||||||||||
| agreement__type=agreement_type, | ||||||||||||||||||||||||||||||||||||||
| timestamp=datetime.now(), | ||||||||||||||||||||||||||||||||||||||
| ) | ||||||||||||||||||||||||||||||||||||||
| return UserAgreementRecordData.from_model(record) | ||||||||||||||||||||||||||||||||||||||
| 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, UserAgreement | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Relative import! |
||
|
|
||
|
|
||
| @attr.s(frozen=True, auto_attribs=True) | ||
| class LTIToolsReceivingPIIData: | ||
|
|
@@ -21,3 +26,45 @@ class LTIPIISignatureData: | |
| course_id: str | ||
| lti_tools: str | ||
| lti_tools_hash: str | ||
|
|
||
|
|
||
|
|
||
| @dataclass | ||
| class UserAgreementData: | ||
| """ | ||
| Data for a user agreement record. | ||
| """ | ||
| type: str | ||
| name: str | ||
| summary: str | ||
| text: str|None | ||
| 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 | ||
| ) | ||
|
|
||
| @dataclass | ||
| class UserAgreementRecordData: | ||
| """ | ||
| Data for a single user agreement record. | ||
| """ | ||
| username: str | ||
| agreement_type: str | ||
| accepted_at: datetime | ||
| is_current: bool = True | ||
|
|
||
| @classmethod | ||
| def from_model(cls, model: UserAgreementRecord): | ||
| return UserAgreementRecordData( | ||
| username=model.user.username, | ||
| agreement_type=model.agreement.type, | ||
| accepted_at=model.timestamp, | ||
| is_current=model.agreement.updated < model.timestamp | ||
| ) | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,25 @@ | ||
| # Generated by Django 4.2.16 on 2024-12-06 11:34 | ||
|
|
||
| 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)), | ||
| ], | ||
| ), | ||
| ] |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,63 @@ | ||
| # Generated by Django 5.2.10 on 2026-01-26 10:21 | ||
|
|
||
| import django.db.models.deletion | ||
| import simple_history.models | ||
| from django.conf import settings | ||
| from django.db import migrations, models | ||
|
|
||
|
|
||
| class Migration(migrations.Migration): | ||
|
|
||
| dependencies = [ | ||
| ('agreements', '0006_useragreementrecord'), | ||
| migrations.swappable_dependency(settings.AUTH_USER_MODEL), | ||
| ] | ||
|
|
||
| operations = [ | ||
| migrations.CreateModel( | ||
| name='HistoricalUserAgreement', | ||
| fields=[ | ||
| ('id', models.IntegerField(auto_created=True, blank=True, db_index=True, verbose_name='ID')), | ||
| ('type', models.CharField(db_index=True, max_length=255)), | ||
| ('name', models.CharField(help_text='Human-readable name for the agreement type. Will be displayed to users in alert to accept the agreement.', max_length=255)), | ||
| ('summary', models.TextField(help_text='Brief summary of the agreement content. Will be displayed to users in alert to accept the agreement.', max_length=1024)), | ||
| ('text', models.TextField(blank=True, help_text='Full text of the agreement. (Required if url is not provided)', null=True)), | ||
| ('url', models.URLField(blank=True, help_text='URL where the full agreement can be accessed. Will be used for "Learn More" link in alert to accept the agreement.', null=True)), | ||
| ('created', models.DateTimeField(blank=True, editable=False)), | ||
| ('updated', models.DateTimeField(help_text='Timestamp of the last update to this agreement. If changed users will be prompted to accept the agreement again.')), | ||
| ('history_id', models.AutoField(primary_key=True, serialize=False)), | ||
| ('history_date', models.DateTimeField()), | ||
| ('history_change_reason', models.CharField(max_length=100, null=True)), | ||
| ('history_type', models.CharField(choices=[('+', 'Created'), ('~', 'Changed'), ('-', 'Deleted')], max_length=1)), | ||
| ('history_user', models.ForeignKey(null=True, on_delete=django.db.models.deletion.SET_NULL, related_name='+', to=settings.AUTH_USER_MODEL)), | ||
| ], | ||
| options={ | ||
| 'verbose_name': 'historical user agreement', | ||
| 'verbose_name_plural': 'historical user agreements', | ||
| 'ordering': ('-history_date', '-history_id'), | ||
| 'get_latest_by': ('history_date', 'history_id'), | ||
| }, | ||
| bases=(simple_history.models.HistoricalChanges, models.Model), | ||
| ), | ||
| migrations.CreateModel( | ||
| name='UserAgreement', | ||
| fields=[ | ||
| ('id', models.AutoField(auto_created=True, primary_key=True, serialize=False, verbose_name='ID')), | ||
| ('type', models.CharField(max_length=255, unique=True)), | ||
| ('name', models.CharField(help_text='Human-readable name for the agreement type. Will be displayed to users in alert to accept the agreement.', max_length=255)), | ||
| ('summary', models.TextField(help_text='Brief summary of the agreement content. Will be displayed to users in alert to accept the agreement.', max_length=1024)), | ||
| ('text', models.TextField(blank=True, help_text='Full text of the agreement. (Required if url is not provided)', null=True)), | ||
| ('url', models.URLField(blank=True, help_text='URL where the full agreement can be accessed. Will be used for "Learn More" link in alert to accept the agreement.', null=True)), | ||
| ('created', models.DateTimeField(auto_now_add=True)), | ||
| ('updated', models.DateTimeField(help_text='Timestamp of the last update to this agreement. If changed users will be prompted to accept the agreement again.')), | ||
| ], | ||
| options={ | ||
| 'constraints': [models.CheckConstraint(condition=models.Q(('text__isnull', False), ('url__isnull', False), _connector='OR'), name='agreement_has_text_or_url')], | ||
| }, | ||
| ), | ||
| migrations.AddField( | ||
| model_name='useragreementrecord', | ||
| name='agreement', | ||
| field=models.ForeignKey(null=True, on_delete=django.db.models.deletion.CASCADE, related_name='records', to='agreements.useragreement'), | ||
| ), | ||
| ] |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,38 @@ | ||
| # Generated by Django 5.2.10 on 2026-01-26 10:22 | ||
|
|
||
| import django.db.models.deletion | ||
| from django.db import migrations, models | ||
|
|
||
|
|
||
|
|
||
| def migrate_agreement_type(apps, schema_editor): | ||
| UserAgreementRecord = apps.get_model('agreements', 'UserAgreementRecord') | ||
| UserAgreement = apps.get_model('agreements', 'UserAgreement') | ||
| for user_agreement_record in UserAgreementRecord.objects.all(): | ||
| user_agreement_record.agreement = UserAgreement.objects.get_or_create(type=user_agreement_record.agreement_type, defaults=dict(text='')) | ||
|
|
||
|
|
||
| def migrate_agreement_type_rev(apps, schema_editor): | ||
| UserAgreementRecord = apps.get_model('agreements', 'UserAgreementRecord') | ||
| for user_agreement_record in UserAgreementRecord.objects.all(): | ||
| user_agreement_record.agreement_type = user_agreement_record.agreement.type | ||
|
|
||
|
|
||
| class Migration(migrations.Migration): | ||
|
|
||
| dependencies = [ | ||
| ('agreements', '0007_historicaluseragreement_useragreement_and_more'), | ||
| ] | ||
|
|
||
| operations = [ | ||
| migrations.RunPython(migrate_agreement_type, migrate_agreement_type_rev), | ||
| migrations.RemoveField( | ||
| model_name='useragreementrecord', | ||
| name='agreement_type', | ||
| ), | ||
| migrations.AlterField( | ||
| model_name='useragreementrecord', | ||
| name='agreement', | ||
| field=models.ForeignKey(on_delete=django.db.models.deletion.CASCADE, related_name='records', to='agreements.useragreement'), | ||
| ), | ||
| ] |
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
|
|
@@ -6,6 +6,7 @@ | |||||
| from django.db import models | ||||||
| from model_utils.models import TimeStampedModel | ||||||
| from opaque_keys.edx.django.models import CourseKeyField | ||||||
| from simple_history.models import HistoricalRecords | ||||||
|
|
||||||
| User = get_user_model() | ||||||
|
|
||||||
|
|
@@ -70,3 +71,57 @@ class ProctoringPIISignature(TimeStampedModel): | |||||
|
|
||||||
| class Meta: | ||||||
| app_label = 'agreements' | ||||||
|
|
||||||
|
|
||||||
| class UserAgreement(models.Model): | ||||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Should we use "TimeStampedModel" here, which will give us created and updated dates
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Not really, we want the creation timestamp to be automatic but not the update timestamp. The update timestamp will invalidate old agreements, i.e. if a user accepts an agreement, then updating the agreement will invalidate the old acceptance. I also considered adding an extra form field which allows overriding whether this is a update that will need re acceptance. In either case the update date should be be applied automatically.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Perhaps there should be a created, updated, major update timestap or similar. |
||||||
| """ | ||||||
| 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 an alert to accept/reject the agreement.', | ||||||
| ) | ||||||
| summary = models.TextField( | ||||||
| max_length=1024, | ||||||
| help_text='Brief summary of the agreement content. Will be displayed to users in alert to accept the agreement.', | ||||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is this a required field?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, we need it to display an alert. |
||||||
| ) | ||||||
| text = models.TextField( | ||||||
| help_text='Full text of the agreement. (Required if url is not provided)', | ||||||
| null=True, blank=True, | ||||||
| ) | ||||||
| url = models.URLField( | ||||||
| help_text='URL where the full agreement can be accessed. Will be used for "Learn More" link in alert to accept the agreement.', | ||||||
| null=True, blank=True, | ||||||
| ) | ||||||
| created = models.DateTimeField(auto_now_add=True) | ||||||
| updated = models.DateTimeField( | ||||||
| help_text='Timestamp of the last update to this agreement. If changed users will be prompted to accept the agreement again.') | ||||||
| history = HistoricalRecords() | ||||||
|
|
||||||
| class Meta: | ||||||
| app_label = 'agreements' | ||||||
| constraints = [ | ||||||
| models.CheckConstraint(check=models.Q(text__isnull=False) | models.Q(url__isnull=False), | ||||||
| name='agreement_has_text_or_url') | ||||||
| ] | ||||||
|
|
||||||
|
|
||||||
| 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` at a particular time. | ||||||
|
|
||||||
| .. no_pii: | ||||||
| """ | ||||||
| user = models.ForeignKey(User, db_index=True, on_delete=models.CASCADE) | ||||||
| agreement = models.ForeignKey(UserAgreement, on_delete=models.CASCADE, related_name='records') | ||||||
| timestamp = models.DateTimeField(auto_now_add=True) | ||||||
|
|
||||||
| class Meta: | ||||||
| app_label = 'agreements' | ||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -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 | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Relative imports? |
||
|
|
||
|
|
||
| class IntegritySignatureSerializer(serializers.ModelSerializer): | ||
| """ | ||
|
|
@@ -31,3 +32,24 @@ class LTIPIISignatureSerializer(serializers.ModelSerializer): | |
| class Meta: | ||
| model = LTIPIISignature | ||
| fields = ('username', 'course_id', 'lti_tools', 'created_at') | ||
|
|
||
|
|
||
| class UserAgreementSerializer(serializers.Serializer): | ||
| """ | ||
| Serializer for UserAgreement model | ||
| """ | ||
| type = serializers.CharField(read_only=True) | ||
| name = serializers.CharField(read_only=True) | ||
| summary = serializers.CharField(read_only=True) | ||
| text = serializers.CharField(read_only=True) | ||
| url = serializers.URLField(read_only=True) | ||
| updated = serializers.DateTimeField(read_only=True) | ||
|
|
||
|
|
||
| class UserAgreementRecordSerializer(serializers.Serializer): | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @xitij2000 any specific reason for not using ModelSerializer? I think it might help here. |
||
| """ | ||
| Serializer for UserAgreementRecord model | ||
| """ | ||
| username = serializers.CharField(read_only=True) | ||
| agreement_type = serializers.CharField(read_only=True) | ||
| accepted_at = serializers.DateTimeField() | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should avoid relative import and go with absolute imports