From af7583b7adc4ef4c466fa93abb8d34f3af90c80e Mon Sep 17 00:00:00 2001 From: Daniele Guido <1181642+danieleguido@users.noreply.github.com> Date: Tue, 21 Apr 2026 15:22:36 +0200 Subject: [PATCH 01/30] check metadata field of SpecialMembershipDataset Model --- impresso/admin.py | 24 +++++++++++++++++++-- impresso/models/specialMembershipDataset.py | 2 ++ 2 files changed, 24 insertions(+), 2 deletions(-) diff --git a/impresso/admin.py b/impresso/admin.py index cc4900b..0c8734e 100644 --- a/impresso/admin.py +++ b/impresso/admin.py @@ -96,7 +96,11 @@ def clean_metadata(self) -> dict: if not isinstance(metadata, dict): raise ValidationError("Metadata must be a JSON object.") - allowed_keys = {"modality"} + allowed_keys = { + "modality", + "enableTemporaryAutomaticAcceptance", + "revokeAfterDays", + } extra_keys = set(metadata.keys()) - allowed_keys if extra_keys: raise ValidationError( @@ -115,7 +119,23 @@ def clean_metadata(self) -> dict: if modality not in allowed_modalities: allowed = ", ".join(sorted(allowed_modalities)) raise ValidationError(f"metadata.modality must be one of: {allowed}.") - + enable_temporary_automatic_acceptance = metadata.get( + "enableTemporaryAutomaticAcceptance" + ) + if enable_temporary_automatic_acceptance is not None and not isinstance( + enable_temporary_automatic_acceptance, bool + ): + raise ValidationError( + "metadata.enableTemporaryAutomaticAcceptance must be a boolean." + ) + revoke_after_days = metadata.get("revokeAfterDays") + if revoke_after_days is not None: + if not isinstance(revoke_after_days, int): + raise ValidationError("metadata.revokeAfterDays must be an integer.") + if revoke_after_days < 0: + raise ValidationError( + "metadata.revokeAfterDays must be a non-negative integer." + ) return metadata diff --git a/impresso/models/specialMembershipDataset.py b/impresso/models/specialMembershipDataset.py index 9bd1c1c..811a242 100644 --- a/impresso/models/specialMembershipDataset.py +++ b/impresso/models/specialMembershipDataset.py @@ -11,6 +11,8 @@ class Metadata(TypedDict, total=False): """ modality: Optional[str] # e.g., "cc_reviewer", "notify_reviewer" + enableTemporaryAutomaticAcceptance: Optional[bool] + revokeAfterDays: Optional[int] class SpecialMembershipDataset(models.Model): From 2f4446572fe4a70a417f4170c76652f9929b4e58 Mon Sep 17 00:00:00 2001 From: Daniele Guido <1181642+danieleguido@users.noreply.github.com> Date: Tue, 21 Apr 2026 15:30:33 +0200 Subject: [PATCH 02/30] Add temporary approval to membership requests Introduce a temporary approval flow for UserSpecialMembershipRequest: add STATUS_APPROVED_TEMPORARY and STATUS_REVOKED constants, include both in the status choices (with user-facing labels), and add a temporary_expires_at DateTimeField for automatic temporary approvals. Update the ChangelogEntry status typing to reflect the new possible values. These changes enable tracking of temporary approvals and subsequent revocations in requests' lifecycle. --- impresso/models/userSpecialMembershipRequest.py | 15 ++++++++++++++- 1 file changed, 14 insertions(+), 1 deletion(-) diff --git a/impresso/models/userSpecialMembershipRequest.py b/impresso/models/userSpecialMembershipRequest.py index 340cab9..4dae314 100644 --- a/impresso/models/userSpecialMembershipRequest.py +++ b/impresso/models/userSpecialMembershipRequest.py @@ -11,7 +11,9 @@ class ChangelogEntry(TypedDict): Defines the strict type structure for a special membership request changelog entry. """ - status: str # e.g., "pending", "approved", "rejected" + status: ( + str # e.g., "pending", "approved", "approved_temporary", "rejected", "revoked" + ) subscription: Optional[str] # The title of the subscription date: str # ISO formatted date string reviewer: Optional[str] # Username of the reviewer @@ -28,7 +30,9 @@ class UserSpecialMembershipRequest(models.Model): Attributes: STATUS_PENDING (str): Status indicating the request is pending. STATUS_APPROVED (str): Status indicating the request is approved. + STATUS_APPROVED_TEMPORARY (str): Status indicating the request is temporarily approved. STATUS_REJECTED (str): Status indicating the request is rejected. + STATUS_REVOKED (str): Status indicating the request has been revoked after temporary approval. user (ForeignKey): Foreign key to the User model representing the user making the request. reviewer (ForeignKey): Foreign key to the User model representing the reviewer of the request. @@ -52,7 +56,9 @@ class UserSpecialMembershipRequest(models.Model): STATUS_PENDING = "pending" STATUS_APPROVED = "approved" + STATUS_APPROVED_TEMPORARY = "temporary" STATUS_REJECTED = "rejected" + STATUS_REVOKED = "revoked" user = models.ForeignKey(User, on_delete=models.CASCADE, related_name="request") reviewer = models.ForeignKey( @@ -66,13 +72,20 @@ class UserSpecialMembershipRequest(models.Model): ) date_created = models.DateTimeField(auto_now_add=True) date_last_modified = models.DateTimeField(auto_now=True) + temporary_expires_at = models.DateTimeField( + null=True, + blank=True, + help_text="Expiration date used for temporary automatic approvals", + ) status = models.CharField( max_length=10, default=STATUS_PENDING, choices=( (STATUS_PENDING, "Pending"), (STATUS_APPROVED, "Approved"), + (STATUS_APPROVED_TEMPORARY, "Approved (Temporary)"), (STATUS_REJECTED, "Rejected"), + (STATUS_REVOKED, "Revoked"), ), ) changelog = models.JSONField(null=True, blank=True, default=list) From d71176a9ab019e1e839c78d9c6a43a0aff3913d0 Mon Sep 17 00:00:00 2001 From: Daniele Guido <1181642+danieleguido@users.noreply.github.com> Date: Tue, 21 Apr 2026 15:38:30 +0200 Subject: [PATCH 03/30] Update settings.py --- impresso/settings.py | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/impresso/settings.py b/impresso/settings.py index 80d4cc8..d0c1522 100644 --- a/impresso/settings.py +++ b/impresso/settings.py @@ -414,9 +414,15 @@ IMPRESSO_EMAIL_SUBJECT_AFTER_USER_SPECIAL_MEMBERSHIP_REQUEST_ACCEPTED_TO_USER = ( "Your Special Membership Request has been approved" ) +IMPRESSO_EMAIL_SUBJECT_AFTER_USER_SPECIAL_MEMBERSHIP_REQUEST_ACCEPTED_TEMPORARY_TO_USER = ( + "Your temporary Special Membership access has been approved" +) IMPRESSO_EMAIL_SUBJECT_AFTER_USER_SPECIAL_MEMBERSHIP_REQUEST_REJECTED_TO_USER = ( "Your Special Membership Request has been rejected" ) +IMPRESSO_EMAIL_SUBJECT_AFTER_USER_SPECIAL_MEMBERSHIP_REQUEST_REVOKED_TO_USER = ( + "Your temporary Special Membership access has expired" +) IMPRESSO_EMAIL_SUBJECT_AFTER_USER_ACTIVATION_PLAN_REJECTED_TO_USER = ( "Access granted to Impresso (Basic User Access)" From 98ff7c01bb67444a0600ad98136e17f436abc371 Mon Sep 17 00:00:00 2001 From: Daniele Guido <1181642+danieleguido@users.noreply.github.com> Date: Tue, 21 Apr 2026 15:39:20 +0200 Subject: [PATCH 04/30] temporary request (wip) --- ...ip_request_approved_temporary_to_user.html | 23 +++++++++++++++++++ ...hip_request_approved_temporary_to_user.txt | 13 +++++++++++ 2 files changed, 36 insertions(+) create mode 100644 impresso/templates/emails/user_special_membership_request_approved_temporary_to_user.html create mode 100644 impresso/templates/emails/user_special_membership_request_approved_temporary_to_user.txt diff --git a/impresso/templates/emails/user_special_membership_request_approved_temporary_to_user.html b/impresso/templates/emails/user_special_membership_request_approved_temporary_to_user.html new file mode 100644 index 0000000..92d2499 --- /dev/null +++ b/impresso/templates/emails/user_special_membership_request_approved_temporary_to_user.html @@ -0,0 +1,23 @@ +Dear {{ user.first_name }}, + +

+ Great news: your request for special membership access has been + temporarily approved. +

+

+ You now have access to: + {{ user_special_membership_request.subscription.title }}. +

+

+ This temporary access is valid until + {{ user_special_membership_request.temporary_expires_at|date:"Y-m-d H:i" }}. + After that date, access will be automatically revoked unless a permanent approval is granted. +

+

+ If you have questions or need an extension, please reply to this email. +

+

+ Best regards, +
+ The Impresso Team +

diff --git a/impresso/templates/emails/user_special_membership_request_approved_temporary_to_user.txt b/impresso/templates/emails/user_special_membership_request_approved_temporary_to_user.txt new file mode 100644 index 0000000..c75da5d --- /dev/null +++ b/impresso/templates/emails/user_special_membership_request_approved_temporary_to_user.txt @@ -0,0 +1,13 @@ +Dear {{ user.first_name }}, + +Great news: your request for special membership access has been temporarily approved. + +You now have access to: {{ user_special_membership_request.subscription.title }}. + +This temporary access is valid until {{ user_special_membership_request.temporary_expires_at|date:"Y-m-d H:i" }}. +After that date, access will be automatically revoked unless a permanent approval is granted. + +If you have questions or need an extension, please reply to this email. + +Best regards, +The Impresso Team From bee5c84ef8dc367430e43b000b636ddf1c47ea3c Mon Sep 17 00:00:00 2001 From: Daniele Guido <1181642+danieleguido@users.noreply.github.com> Date: Tue, 21 Apr 2026 15:39:35 +0200 Subject: [PATCH 05/30] membership revoked template (WIP) --- ...ial_membership_request_revoked_to_user.html | 18 ++++++++++++++++++ ...cial_membership_request_revoked_to_user.txt | 10 ++++++++++ 2 files changed, 28 insertions(+) create mode 100644 impresso/templates/emails/user_special_membership_request_revoked_to_user.html create mode 100644 impresso/templates/emails/user_special_membership_request_revoked_to_user.txt diff --git a/impresso/templates/emails/user_special_membership_request_revoked_to_user.html b/impresso/templates/emails/user_special_membership_request_revoked_to_user.html new file mode 100644 index 0000000..9deae2c --- /dev/null +++ b/impresso/templates/emails/user_special_membership_request_revoked_to_user.html @@ -0,0 +1,18 @@ +Dear {{ user.first_name }}, + +

+ Your temporary special membership access has expired and is now + revoked. +

+

+ Expired membership option: + {{ user_special_membership_request.subscription.title }}. +

+

+ If you still need access, you can submit a new request or contact the reviewer. +

+

+ Best regards, +
+ The Impresso Team +

diff --git a/impresso/templates/emails/user_special_membership_request_revoked_to_user.txt b/impresso/templates/emails/user_special_membership_request_revoked_to_user.txt new file mode 100644 index 0000000..63e86e9 --- /dev/null +++ b/impresso/templates/emails/user_special_membership_request_revoked_to_user.txt @@ -0,0 +1,10 @@ +Dear {{ user.first_name }}, + +Your temporary special membership access has expired and is now revoked. + +Expired membership option: {{ user_special_membership_request.subscription.title }}. + +If you still need access, you can submit a new request or contact the reviewer. + +Best regards, +The Impresso Team From fa82f9580289bff384ffb57d5fea277be708b4c9 Mon Sep 17 00:00:00 2001 From: Daniele Guido <1181642+danieleguido@users.noreply.github.com> Date: Tue, 21 Apr 2026 16:07:01 +0200 Subject: [PATCH 06/30] Create 0058_remove_userrequest_subscription_and_more.py this is a weird migration, use FAKE --- ...emove_userrequest_subscription_and_more.py | 103 ++++++++++++++++++ 1 file changed, 103 insertions(+) create mode 100644 impresso/migrations/0058_remove_userrequest_subscription_and_more.py diff --git a/impresso/migrations/0058_remove_userrequest_subscription_and_more.py b/impresso/migrations/0058_remove_userrequest_subscription_and_more.py new file mode 100644 index 0000000..e571dbd --- /dev/null +++ b/impresso/migrations/0058_remove_userrequest_subscription_and_more.py @@ -0,0 +1,103 @@ +# Generated by Django 5.2.10 on 2026-04-21 14:05 + +import django.db.models.deletion +from django.conf import settings +from django.db import migrations, models + + +class Migration(migrations.Migration): + + dependencies = [ + ('impresso', '0056_collection_search_query_hash_squashed_0057_remove_collection_search_query_hash_and_more'), + migrations.swappable_dependency(settings.AUTH_USER_MODEL), + ] + + operations = [ + migrations.RemoveField( + model_name='userrequest', + name='subscription', + ), + migrations.AlterUniqueTogether( + name='userrequest', + unique_together=None, + ), + migrations.RemoveField( + model_name='userrequest', + name='reviewer', + ), + migrations.RemoveField( + model_name='userrequest', + name='user', + ), + migrations.AlterField( + model_name='collection', + name='serialized_search_query', + field=models.TextField(blank=True, help_text='Initial search query that generated the collection.', null=True), + ), + migrations.AlterField( + model_name='searchquery', + name='hash', + field=models.TextField(blank=True, null=True), + ), + migrations.CreateModel( + name='SpecialMembershipDataset', + fields=[ + ('id', models.AutoField(auto_created=True, primary_key=True, serialize=False, verbose_name='ID')), + ('title', models.CharField(db_column='name', max_length=255)), + ('bitmap_position', models.PositiveIntegerField(blank=True, null=True, unique=True)), + ('metadata', models.JSONField(blank=True, default=dict)), + ('reviewer', models.ForeignKey(blank=True, null=True, on_delete=django.db.models.deletion.SET_NULL, related_name='reviewed_datasets', to=settings.AUTH_USER_MODEL)), + ], + options={ + 'verbose_name': 'Special Membership Access', + 'verbose_name_plural': 'Special Membership Accesses', + 'db_table': 'impresso_datasetbitmapposition', + }, + ), + migrations.CreateModel( + name='UserBitmapSubscription', + fields=[ + ('id', models.AutoField(auto_created=True, primary_key=True, serialize=False, verbose_name='ID')), + ('specialmembershipdataset', models.ForeignKey(db_column='datasetbitmapposition_id', on_delete=django.db.models.deletion.CASCADE, to='impresso.specialmembershipdataset')), + ('userbitmap', models.ForeignKey(db_column='userbitmap_id', on_delete=django.db.models.deletion.CASCADE, to='impresso.userbitmap')), + ], + options={ + 'verbose_name': 'User Special Membership Access', + 'verbose_name_plural': 'User Special Membership Accesses', + 'db_table': 'impresso_userbitmap_subscriptions', + 'unique_together': {('userbitmap', 'specialmembershipdataset')}, + }, + ), + migrations.AlterField( + model_name='userbitmap', + name='subscriptions', + field=models.ManyToManyField(blank=True, through='impresso.UserBitmapSubscription', to='impresso.specialmembershipdataset'), + ), + migrations.CreateModel( + name='UserSpecialMembershipRequest', + fields=[ + ('id', models.AutoField(auto_created=True, primary_key=True, serialize=False, verbose_name='ID')), + ('date_created', models.DateTimeField(auto_now_add=True)), + ('date_last_modified', models.DateTimeField(auto_now=True)), + ('temporary_expires_at', models.DateTimeField(blank=True, help_text='Expiration date used for temporary automatic approvals', null=True)), + ('status', models.CharField(choices=[('pending', 'Pending'), ('approved', 'Approved'), ('temporary', 'Approved (Temporary)'), ('rejected', 'Rejected'), ('revoked', 'Revoked')], default='pending', max_length=10)), + ('changelog', models.JSONField(blank=True, default=list, null=True)), + ('notes', models.TextField(blank=True, null=True)), + ('reviewer', models.ForeignKey(blank=True, null=True, on_delete=django.db.models.deletion.SET_NULL, related_name='review', to=settings.AUTH_USER_MODEL)), + ('subscription', models.ForeignKey(help_text='The special membership dataset being requested', null=True, on_delete=django.db.models.deletion.SET_NULL, to='impresso.specialmembershipdataset')), + ('user', models.ForeignKey(on_delete=django.db.models.deletion.CASCADE, related_name='request', to=settings.AUTH_USER_MODEL)), + ], + options={ + 'verbose_name': 'User Special Membership Request', + 'verbose_name_plural': 'User Special Membership Requests', + 'db_table': 'impresso_userrequest', + 'unique_together': {('user', 'subscription')}, + }, + ), + migrations.DeleteModel( + name='DatasetBitmapPosition', + ), + migrations.DeleteModel( + name='UserRequest', + ), + ] From 2734c2ca5d2ec20fc7e23860662242b54b4c0fd6 Mon Sep 17 00:00:00 2001 From: Daniele Guido <1181642+danieleguido@users.noreply.github.com> Date: Tue, 21 Apr 2026 16:07:22 +0200 Subject: [PATCH 07/30] WIP --- .../userSpecialMembershipRequest_tasks.py | 98 ++++++++++++++++++- .../test_userSpecialMembershipRequest.py | 91 +++++++++++++++++ .../tasks/userSpecialMembershipRequest.py | 25 ++++- 3 files changed, 205 insertions(+), 9 deletions(-) diff --git a/impresso/tasks/userSpecialMembershipRequest_tasks.py b/impresso/tasks/userSpecialMembershipRequest_tasks.py index 87aeca7..283fa73 100644 --- a/impresso/tasks/userSpecialMembershipRequest_tasks.py +++ b/impresso/tasks/userSpecialMembershipRequest_tasks.py @@ -1,9 +1,8 @@ -from typing import Dict +from datetime import timedelta + from celery.utils.log import get_task_logger -from django.db.utils import IntegrityError +from django.utils import timezone -from impresso.models.specialMembershipDataset import SpecialMembershipDataset -from celery import shared_task from ..celery import app from ..models.userSpecialMembershipRequest import UserSpecialMembershipRequest from impresso.utils.tasks.userSpecialMembershipRequest import ( @@ -15,6 +14,34 @@ logger = get_task_logger(__name__) +def _is_temporary_auto_accept_enabled(req: UserSpecialMembershipRequest) -> bool: + if not req.subscription: + return False + return bool(req.subscription.metadata.get("enableTemporaryAutomaticAcceptance")) + + +def _get_revoke_after_days(req: UserSpecialMembershipRequest) -> int | None: + if not req.subscription: + return None + revoke_after_days = req.subscription.metadata.get("revokeAfterDays") + if isinstance(revoke_after_days, int) and revoke_after_days > 0: + return revoke_after_days + return None + + +def _schedule_temporary_revocation(req: UserSpecialMembershipRequest) -> None: + revoke_after_days = _get_revoke_after_days(req) + if revoke_after_days is None: + logger.warning( + f"[instance:{req.pk}] STATUS_APPROVED_TEMPORARY but revokeAfterDays is missing/invalid, skipping scheduling" + ) + return + revoke_at = timezone.now() + timedelta(days=revoke_after_days) + revoke_special_membership_request.apply_async( + kwargs={"instance_id": req.pk}, eta=revoke_at + ) + + @app.task( bind=True, autoretry_for=(Exception,), @@ -40,6 +67,26 @@ def after_special_membership_request_created(self, instance_id: int) -> None: logger.info( f"[UserSpecialMembershipRequest:{instance_id}] triggered signals after_special_membership_request_created, for user={req.user.username} subscription={req.subscription.title if req.subscription else 'None'} status={req.status}" ) + + if ( + req.status == UserSpecialMembershipRequest.STATUS_PENDING + and _is_temporary_auto_accept_enabled(req) + ): + revoke_after_days = _get_revoke_after_days(req) + if revoke_after_days is None: + logger.warning( + f"[instance:{instance_id}] enableTemporaryAutomaticAcceptance is true but revokeAfterDays is missing or invalid, continuing regular pending flow" + ) + else: + req.status = UserSpecialMembershipRequest.STATUS_APPROVED_TEMPORARY + req.save() + logger.info( + f"[instance:{instance_id}] auto-accepted as temporary for {revoke_after_days} day(s)" + ) + # The save above triggers the update task via signal, which applies bitmap, + # sends the temporary-approval email, and schedules revocation. + return + apply_special_membership_to_bitmap(instance=req, created=True, logger=logger) send_email_after_user_special_membership_request_created( instance=req, logger=logger @@ -83,3 +130,46 @@ def after_special_membership_request_updated(self, instance_id: int) -> None: send_email_after_user_special_membership_request_updated( instance=req, logger=logger ) + + if req.status == UserSpecialMembershipRequest.STATUS_APPROVED_TEMPORARY: + _schedule_temporary_revocation(req) + + +@app.task( + bind=True, + autoretry_for=(Exception,), + exponential_backoff=2, + retry_kwargs={"max_retries": 5}, + retry_jitter=True, +) +def revoke_special_membership_request(self, instance_id: int) -> None: + """ + Revoke a temporary special membership request when its expiration date is reached. + + Args: + self: The task instance. + instance_id (int): The ID of the UserSpecialMembershipRequest instance. + """ + try: + req = UserSpecialMembershipRequest.objects.get(pk=instance_id) + except UserSpecialMembershipRequest.DoesNotExist: + logger.error( + f"[instance:{instance_id}] Cannot revoke temporary access, request not found" + ) + return + + if req.status != UserSpecialMembershipRequest.STATUS_APPROVED_TEMPORARY: + logger.info( + f"[instance:{instance_id}] revocation skipped because status is {req.status}" + ) + return + + req.status = UserSpecialMembershipRequest.STATUS_REVOKED + req.save() + logger.info( + f"[instance:{instance_id}] temporary membership revoked for user={req.user.pk}" + ) + apply_special_membership_to_bitmap(instance=req, created=False, logger=logger) + send_email_after_user_special_membership_request_updated( + instance=req, logger=logger + ) diff --git a/impresso/tests/utils/tasks/test_userSpecialMembershipRequest.py b/impresso/tests/utils/tasks/test_userSpecialMembershipRequest.py index 5ea8315..38fa31e 100644 --- a/impresso/tests/utils/tasks/test_userSpecialMembershipRequest.py +++ b/impresso/tests/utils/tasks/test_userSpecialMembershipRequest.py @@ -1,4 +1,6 @@ import logging +from datetime import timedelta + from django.conf import settings from django.test import TestCase from django.contrib.auth.models import Group, User @@ -8,6 +10,10 @@ from impresso.models import SpecialMembershipDataset, UserSpecialMembershipRequest from impresso.models.profile import Profile from impresso.models.userBitmap import UserBitmap +from impresso.tasks.userSpecialMembershipRequest_tasks import ( + after_special_membership_request_created, + revoke_special_membership_request, +) from impresso.utils.tasks.userSpecialMembershipRequest import ( send_email_after_user_special_membership_request_created, ) @@ -353,3 +359,88 @@ def test_reviewer_email_html_alternative(self): "Email should have HTML alternative", ) self.assertGreater(len(reviewer_email.alternatives), 0) + + +class TestTemporaryAutomaticAcceptance(TestCase): + def setUp(self): + self.reviewer = User.objects.create_user( + username="temp-reviewer", + email="temp-reviewer@example.com", + password="testpass123", + ) + self.user = User.objects.create_user( + username="temp-user", + first_name="Temp", + email="temp-user@example.com", + password="testpass123", + ) + self.dataset = SpecialMembershipDataset.objects.create( + title="Temporary Dataset", + reviewer=self.reviewer, + metadata={ + "enableTemporaryAutomaticAcceptance": True, + "revokeAfterDays": 7, + "modality": settings.IMPRESSO_EMAIL_MODALITY_SPECIAL_MEMBERSHIP_REQUEST_NOTIFY_REVIEWER, + }, + ) + mail.outbox = [] + + def test_created_request_is_auto_approved_temporarily(self): + req = UserSpecialMembershipRequest.objects.create( + user=self.user, + reviewer=self.reviewer, + subscription=self.dataset, + status=UserSpecialMembershipRequest.STATUS_PENDING, + ) + + mail.outbox = [] + after_special_membership_request_created(None, req.pk) + req.refresh_from_db() + + self.assertEqual( + req.status, + UserSpecialMembershipRequest.STATUS_APPROVED_TEMPORARY, + ) + self.assertIsNotNone(req.temporary_expires_at) + self.assertEqual(self.user.bitmap.subscriptions.count(), 1) + self.assertEqual(len(mail.outbox), 1) + self.assertEqual( + mail.outbox[0].subject, + settings.IMPRESSO_EMAIL_SUBJECT_AFTER_USER_SPECIAL_MEMBERSHIP_REQUEST_ACCEPTED_TEMPORARY_TO_USER, + ) + + +class TestTemporaryAutomaticRevocation(TestCase): + def setUp(self): + self.user = User.objects.create_user( + username="revoked-user", + first_name="Revoked", + email="revoked-user@example.com", + password="testpass123", + ) + self.dataset = SpecialMembershipDataset.objects.create( + title="Revoked Dataset", + metadata={"revokeAfterDays": 1}, + ) + mail.outbox = [] + + def test_expired_temporary_request_is_revoked(self): + req = UserSpecialMembershipRequest.objects.create( + user=self.user, + subscription=self.dataset, + status=UserSpecialMembershipRequest.STATUS_APPROVED_TEMPORARY, + temporary_expires_at=timezone.now() - timedelta(days=1), + ) + self.user.bitmap.subscriptions.add(self.dataset) + mail.outbox = [] + + revoke_special_membership_request(None, req.pk) + req.refresh_from_db() + + self.assertEqual(req.status, UserSpecialMembershipRequest.STATUS_REVOKED) + self.assertEqual(self.user.bitmap.subscriptions.count(), 0) + self.assertEqual(len(mail.outbox), 1) + self.assertEqual( + mail.outbox[0].subject, + settings.IMPRESSO_EMAIL_SUBJECT_AFTER_USER_SPECIAL_MEMBERSHIP_REQUEST_REVOKED_TO_USER, + ) diff --git a/impresso/utils/tasks/userSpecialMembershipRequest.py b/impresso/utils/tasks/userSpecialMembershipRequest.py index 1dce446..463573d 100644 --- a/impresso/utils/tasks/userSpecialMembershipRequest.py +++ b/impresso/utils/tasks/userSpecialMembershipRequest.py @@ -1,7 +1,6 @@ import logging from logging import Logger from django.conf import settings -from django.contrib.auth.models import User from impresso.models.userBitmap import UserBitmap from impresso.utils.models.user import ( get_number_of_special_memberships, @@ -19,9 +18,11 @@ def apply_special_membership_to_bitmap( logger: Logger = default_logger, ) -> None: """ - Applies the special membership to the user's bitmap based on the status of the UserSpecialMembershipRequest instance. - If the request is approved, the corresponding subscription is added to the user's bitmap. - If the request is rejected or pending, the corresponding subscription is removed from the user's bitmap. + Applies the special membership to the user's bitmap based on the status of the + UserSpecialMembershipRequest instance. + + Approved statuses add the subscription to the bitmap; non-approved statuses + remove it. Args: instance (UserSpecialMembershipRequest): The UserSpecialMembershipRequest instance. created (bool): A boolean indicating whether the instance was created or updated. @@ -41,12 +42,16 @@ def apply_special_membership_to_bitmap( f"UserSpecialMembershipRequest {instance.pk} has no subscription? skipping bitmap update." ) return - if instance.status == UserSpecialMembershipRequest.STATUS_APPROVED: + if instance.status in [ + UserSpecialMembershipRequest.STATUS_APPROVED, + UserSpecialMembershipRequest.STATUS_APPROVED_TEMPORARY, + ]: user_bitmap.subscriptions.add(instance.subscription) elif instance.status in [ UserSpecialMembershipRequest.STATUS_REJECTED, UserSpecialMembershipRequest.STATUS_PENDING, + UserSpecialMembershipRequest.STATUS_REVOKED, ]: user_bitmap.subscriptions.remove(instance.subscription) # this should update the bitmap thanks to the signal @m2m_changed update_user_bitmap @@ -72,11 +77,21 @@ def send_email_after_user_special_membership_request_updated( subject = ( settings.IMPRESSO_EMAIL_SUBJECT_AFTER_USER_SPECIAL_MEMBERSHIP_REQUEST_ACCEPTED_TO_USER ) + elif instance.status == UserSpecialMembershipRequest.STATUS_APPROVED_TEMPORARY: + template = "user_special_membership_request_approved_temporary_to_user" + subject = ( + settings.IMPRESSO_EMAIL_SUBJECT_AFTER_USER_SPECIAL_MEMBERSHIP_REQUEST_ACCEPTED_TEMPORARY_TO_USER + ) elif instance.status == UserSpecialMembershipRequest.STATUS_REJECTED: subject = ( settings.IMPRESSO_EMAIL_SUBJECT_AFTER_USER_SPECIAL_MEMBERSHIP_REQUEST_REJECTED_TO_USER ) template = "user_special_membership_request_rejected_to_user" + elif instance.status == UserSpecialMembershipRequest.STATUS_REVOKED: + subject = ( + settings.IMPRESSO_EMAIL_SUBJECT_AFTER_USER_SPECIAL_MEMBERSHIP_REQUEST_REVOKED_TO_USER + ) + template = "user_special_membership_request_revoked_to_user" else: subject = ( settings.IMPRESSO_EMAIL_SUBJECT_AFTER_USER_SPECIAL_MEMBERSHIP_REQUEST_PENDING_TO_USER From 9424393385a9d9528cc9bdf86ca47ab47f54240c Mon Sep 17 00:00:00 2001 From: Daniele Guido <1181642+danieleguido@users.noreply.github.com> Date: Mon, 4 May 2026 17:05:49 +0200 Subject: [PATCH 08/30] Add temp membership check & revocation beat Introduce a management command (checktemporarymemberships) to list special membership requests with STATUS_APPROVED_TEMPORARY (with a --dry-run flag) and add tests for it. Add a Celery beat schedule and a periodic task (revoke_expired_temporary_memberships_beat) that finds expired temporary memberships and enqueues revoke_special_membership_request for each; schedule runs hourly. Tweak createspecialmembership help text and add a unit test to ensure the beat task enqueues revocations only for expired requests. --- .../commands/checktemporarymemberships.py | 41 +++++++++ .../commands/createspecialmembership.py | 4 +- impresso/settings.py | 8 ++ .../userSpecialMembershipRequest_tasks.py | 28 ++++++ .../test_checktemporarymemberships.py | 87 +++++++++++++++++++ .../test_userSpecialMembershipRequest.py | 24 +++++ 6 files changed, 189 insertions(+), 3 deletions(-) create mode 100644 impresso/management/commands/checktemporarymemberships.py create mode 100644 impresso/tests/management/test_checktemporarymemberships.py diff --git a/impresso/management/commands/checktemporarymemberships.py b/impresso/management/commands/checktemporarymemberships.py new file mode 100644 index 0000000..d59e408 --- /dev/null +++ b/impresso/management/commands/checktemporarymemberships.py @@ -0,0 +1,41 @@ +from typing import Any +from django.core.management.base import BaseCommand +from django.utils import timezone +from impresso.models import UserSpecialMembershipRequest + +class Command(BaseCommand): + help = "Check special memberships that have the temporary approved status." + + def add_arguments(self, parser) -> None: + parser.add_argument( + "--dry-run", + action="store_true", + help="Run the command without making any actual changes to the database.", + ) + + def handle(self, *args: Any, **options: Any) -> None: + dry_run = options.get("dry_run", False) + + if dry_run: + self.stdout.write(self.style.NOTICE("Running in DRY RUN mode.")) + + requests = UserSpecialMembershipRequest.objects.filter( + status=UserSpecialMembershipRequest.STATUS_APPROVED_TEMPORARY + ) + + count = requests.count() + self.stdout.write(self.style.SUCCESS(f"Found {count} special memberships with temporary approval.")) + + for req in requests: + expires_at = req.temporary_expires_at + expired_str = "" + if expires_at and expires_at < timezone.now(): + expired_str = " (EXPIRED)" + elif expires_at: + expired_str = f" (Expires: {expires_at.strftime('%Y-%m-%d %H:%M:%S %Z')})" + + dataset_title = req.subscription.title if req.subscription else "None" + self.stdout.write( + f"- Request ID: {req.pk}, User: {req.user.username}, " + f"Dataset: {dataset_title}{expired_str}" + ) diff --git a/impresso/management/commands/createspecialmembership.py b/impresso/management/commands/createspecialmembership.py index 4890c1e..43aedde 100644 --- a/impresso/management/commands/createspecialmembership.py +++ b/impresso/management/commands/createspecialmembership.py @@ -9,9 +9,7 @@ class Command(BaseCommand): - help = ( - "Create a pending special membership request for a specific user and dataset." - ) + help = "Create a special membership request for a specific user and dataset." def add_arguments(self, parser) -> None: parser.add_argument( diff --git a/impresso/settings.py b/impresso/settings.py index d0c1522..2865f29 100644 --- a/impresso/settings.py +++ b/impresso/settings.py @@ -173,6 +173,14 @@ get_env_variable("CELERY_BROKER_CONNECTION_RETRY_ON_STARTUP", False) == "True" ) +from celery.schedules import crontab +CELERY_BEAT_SCHEDULE = { + "revoke-expired-temporary-memberships-every-hour": { + "task": "impresso.tasks.userSpecialMembershipRequest_tasks.revoke_expired_temporary_memberships_beat", + "schedule": crontab(minute=0), + }, +} + IMPRESSO_BASE_URL = get_env_variable("IMPRESSO_BASE_URL", "https://impresso-project.ch") IMPRESSO_INSTITUTIONS_ACCESS_URL = get_env_variable( "IMPRESSO_INSTITUTIONS_ACCESS_URL", diff --git a/impresso/tasks/userSpecialMembershipRequest_tasks.py b/impresso/tasks/userSpecialMembershipRequest_tasks.py index 283fa73..706f0ca 100644 --- a/impresso/tasks/userSpecialMembershipRequest_tasks.py +++ b/impresso/tasks/userSpecialMembershipRequest_tasks.py @@ -173,3 +173,31 @@ def revoke_special_membership_request(self, instance_id: int) -> None: send_email_after_user_special_membership_request_updated( instance=req, logger=logger ) + + +@app.task( + bind=True, + autoretry_for=(Exception,), + exponential_backoff=2, + retry_kwargs={"max_retries": 5}, + retry_jitter=True, +) +def revoke_expired_temporary_memberships_beat(self) -> None: + """ + Periodic task to revoke any STATUS_APPROVED_TEMPORARY memberships + that have passed their temporary_expires_at date. + """ + expired_requests = UserSpecialMembershipRequest.objects.filter( + status=UserSpecialMembershipRequest.STATUS_APPROVED_TEMPORARY, + temporary_expires_at__lt=timezone.now() + ) + + count = expired_requests.count() + if count == 0: + logger.info("No expired temporary memberships found to revoke.") + return + + logger.info(f"Found {count} expired temporary memberships to revoke.") + + for req in expired_requests: + revoke_special_membership_request.delay(instance_id=req.pk) diff --git a/impresso/tests/management/test_checktemporarymemberships.py b/impresso/tests/management/test_checktemporarymemberships.py new file mode 100644 index 0000000..a3ceb24 --- /dev/null +++ b/impresso/tests/management/test_checktemporarymemberships.py @@ -0,0 +1,87 @@ +from datetime import timedelta +from io import StringIO + +from django.contrib.auth.models import User +from django.core.management import call_command +from django.test import TestCase +from django.utils import timezone + +from impresso.models import SpecialMembershipDataset, UserSpecialMembershipRequest +from impresso.signals import create_default_groups + + +class TestCheckTemporaryMembershipsCommand(TestCase): + """Test the checktemporarymemberships management command.""" + + def setUp(self) -> None: + create_default_groups(sender="impresso") + + self.user1 = User.objects.create_user( + username="user1", + first_name="Alice", + last_name="Smith", + email="alice@example.com", + password="testpass123", + ) + + self.user2 = User.objects.create_user( + username="user2", + first_name="Bob", + last_name="Jones", + email="bob@example.com", + password="testpass123", + ) + + self.dataset = SpecialMembershipDataset.objects.create( + title="Dataset Alpha", + ) + + def test_finds_temporary_memberships(self) -> None: + # Create one pending, one temporary approved + UserSpecialMembershipRequest.objects.create( + user=self.user1, + subscription=self.dataset, + status=UserSpecialMembershipRequest.STATUS_PENDING, + ) + + temp_req = UserSpecialMembershipRequest.objects.create( + user=self.user2, + subscription=self.dataset, + status=UserSpecialMembershipRequest.STATUS_APPROVED_TEMPORARY, + temporary_expires_at=timezone.now() + timedelta(days=7), + ) + + out = StringIO() + call_command("checktemporarymemberships", stdout=out) + + output = out.getvalue() + self.assertIn("Found 1 special memberships with temporary approval.", output) + self.assertIn(f"Request ID: {temp_req.pk}", output) + self.assertIn("User: user2", output) + self.assertIn("Dataset: Dataset Alpha", output) + self.assertIn("(Expires:", output) + + def test_shows_expired_memberships(self) -> None: + # Create a temporary approved request that has already expired + temp_req = UserSpecialMembershipRequest.objects.create( + user=self.user1, + subscription=self.dataset, + status=UserSpecialMembershipRequest.STATUS_APPROVED_TEMPORARY, + temporary_expires_at=timezone.now() - timedelta(days=1), + ) + + out = StringIO() + call_command("checktemporarymemberships", stdout=out) + + output = out.getvalue() + self.assertIn("Found 1 special memberships with temporary approval.", output) + self.assertIn(f"Request ID: {temp_req.pk}", output) + self.assertIn("(EXPIRED)", output) + + def test_dry_run_flag(self) -> None: + out = StringIO() + call_command("checktemporarymemberships", "--dry-run", stdout=out) + + output = out.getvalue() + self.assertIn("Running in DRY RUN mode.", output) + self.assertIn("Found 0 special memberships with temporary approval.", output) diff --git a/impresso/tests/utils/tasks/test_userSpecialMembershipRequest.py b/impresso/tests/utils/tasks/test_userSpecialMembershipRequest.py index 38fa31e..361eb68 100644 --- a/impresso/tests/utils/tasks/test_userSpecialMembershipRequest.py +++ b/impresso/tests/utils/tasks/test_userSpecialMembershipRequest.py @@ -444,3 +444,27 @@ def test_expired_temporary_request_is_revoked(self): mail.outbox[0].subject, settings.IMPRESSO_EMAIL_SUBJECT_AFTER_USER_SPECIAL_MEMBERSHIP_REQUEST_REVOKED_TO_USER, ) + + def test_revoke_expired_temporary_memberships_beat(self): + from unittest.mock import patch + from impresso.tasks.userSpecialMembershipRequest_tasks import revoke_expired_temporary_memberships_beat + + req1 = UserSpecialMembershipRequest.objects.create( + user=self.user, + subscription=self.dataset, + status=UserSpecialMembershipRequest.STATUS_APPROVED_TEMPORARY, + temporary_expires_at=timezone.now() - timedelta(days=1), + ) + + user2 = User.objects.create_user(username="other-user") + req2 = UserSpecialMembershipRequest.objects.create( + user=user2, + subscription=self.dataset, + status=UserSpecialMembershipRequest.STATUS_APPROVED_TEMPORARY, + temporary_expires_at=timezone.now() + timedelta(days=1), + ) + + with patch("impresso.tasks.userSpecialMembershipRequest_tasks.revoke_special_membership_request.delay") as mock_delay: + revoke_expired_temporary_memberships_beat(None) + + mock_delay.assert_called_once_with(instance_id=req1.pk) From 0847dc8cc232a87bf535426c78f63539e77dda2e Mon Sep 17 00:00:00 2001 From: Daniele Guido <1181642+danieleguido@users.noreply.github.com> Date: Tue, 5 May 2026 17:02:03 +0200 Subject: [PATCH 09/30] revoke after days metadata accepts float values --- impresso/admin.py | 8 +++++--- impresso/models/specialMembershipDataset.py | 2 +- 2 files changed, 6 insertions(+), 4 deletions(-) diff --git a/impresso/admin.py b/impresso/admin.py index 0c8734e..3f9ba71 100644 --- a/impresso/admin.py +++ b/impresso/admin.py @@ -130,11 +130,13 @@ def clean_metadata(self) -> dict: ) revoke_after_days = metadata.get("revokeAfterDays") if revoke_after_days is not None: - if not isinstance(revoke_after_days, int): - raise ValidationError("metadata.revokeAfterDays must be an integer.") + if not isinstance(revoke_after_days, (int, float)): + raise ValidationError( + "metadata.revokeAfterDays must be an integer or float." + ) if revoke_after_days < 0: raise ValidationError( - "metadata.revokeAfterDays must be a non-negative integer." + "metadata.revokeAfterDays must be a non-negative integer or float." ) return metadata diff --git a/impresso/models/specialMembershipDataset.py b/impresso/models/specialMembershipDataset.py index 811a242..2f9f242 100644 --- a/impresso/models/specialMembershipDataset.py +++ b/impresso/models/specialMembershipDataset.py @@ -12,7 +12,7 @@ class Metadata(TypedDict, total=False): modality: Optional[str] # e.g., "cc_reviewer", "notify_reviewer" enableTemporaryAutomaticAcceptance: Optional[bool] - revokeAfterDays: Optional[int] + revokeAfterDays: Optional[float] class SpecialMembershipDataset(models.Model): From c637e679d99792373db83ecd3a9cca7903d19324 Mon Sep 17 00:00:00 2001 From: Daniele Guido <1181642+danieleguido@users.noreply.github.com> Date: Tue, 5 May 2026 17:02:06 +0200 Subject: [PATCH 10/30] Update AGENTS.md --- AGENTS.md | 40 +++++++++++++++++++++++++++++++++++++++- 1 file changed, 39 insertions(+), 1 deletion(-) diff --git a/AGENTS.md b/AGENTS.md index 65f7ff5..4651156 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -18,12 +18,51 @@ This repository contains AI coding instructions and architectural guidelines in: - `.github/copilot-instructions.md` Those instructions define: + - Coding style - Task conventions - Architectural constraints All agents and contributors MUST follow those rules when adding or modifying tasks. +## Testing and Celery Behavior + +### Custom test runner + +The project uses a custom Django test runner configured in [impresso/settings.py](impresso/settings.py#L104): + +- `TEST_RUNNER = "impresso.tests.test_runner.TestRunner"` + +The runner implementation is in [impresso/tests/test_runner.py](impresso/tests/test_runner.py#L1) and enables these Celery settings for tests: + +- `CELERY_TASK_ALWAYS_EAGER=True` +- `CELERY_TASK_EAGER_PROPAGATES=True` +- `CELERY_TASK_STORE_EAGER_RESULT=False` + +### Practical implications for tests + +- Calls to Celery queue APIs like `.delay()` and `.apply_async()` run inline during tests in this repository. +- Exceptions raised by task code propagate to the test process (because eager propagates is enabled). +- For scheduling logic, do not wait for wall-clock execution in tests. Instead, patch the enqueue call and assert arguments. + +### `apply_async(..., eta=...)` in tests + +When eager mode is enabled by the custom test runner, `apply_async(..., eta=...)` does not behave like a real delayed worker schedule. The call executes immediately in-process, while `eta` is still passed as metadata. + +For scheduling assertions, patch the task method and assert it was called with the expected `eta` and kwargs, for example: + +```python +with patch( + "impresso.tasks.userSpecialMembershipRequest_tasks.revoke_special_membership_request.apply_async" +) as mock_apply_async: + _schedule_temporary_revocation(req) + + mock_apply_async.assert_called_once() + _, call_kwargs = mock_apply_async.call_args + assert call_kwargs["kwargs"] == {"instance_id": req.pk} + assert "eta" in call_kwargs +``` + ## Email and templates ### Overview @@ -79,4 +118,3 @@ send_magic_link_email( ``` The function appends `?token=` to `magic_link_callback_url` and passes the full URL as `magic_link` to the template context. - From 195cdcb8a74425210725101aee37815efabb3b39 Mon Sep 17 00:00:00 2001 From: Daniele Guido <1181642+danieleguido@users.noreply.github.com> Date: Tue, 5 May 2026 17:03:01 +0200 Subject: [PATCH 11/30] add temporary_expires_at field in userSpecialMembershipRequest --- ...emove_userrequest_subscription_and_more.py | 252 ++++++++++++------ .../models/userSpecialMembershipRequest.py | 3 +- .../userSpecialMembershipRequest_tasks.py | 64 +++-- ...hip_request_approved_temporary_to_user.txt | 32 ++- .../test_checktemporarymemberships.py | 0 .../test_userSpecialMembershipRequest.py | 129 ++++++--- .../tasks/userSpecialMembershipRequest.py | 27 +- 7 files changed, 361 insertions(+), 146 deletions(-) rename impresso/tests/management/{ => commands}/test_checktemporarymemberships.py (100%) diff --git a/impresso/migrations/0058_remove_userrequest_subscription_and_more.py b/impresso/migrations/0058_remove_userrequest_subscription_and_more.py index e571dbd..8fad295 100644 --- a/impresso/migrations/0058_remove_userrequest_subscription_and_more.py +++ b/impresso/migrations/0058_remove_userrequest_subscription_and_more.py @@ -8,96 +8,178 @@ class Migration(migrations.Migration): dependencies = [ - ('impresso', '0056_collection_search_query_hash_squashed_0057_remove_collection_search_query_hash_and_more'), + ( + "impresso", + "0056_collection_search_query_hash_squashed_0057_remove_collection_search_query_hash_and_more", + ), migrations.swappable_dependency(settings.AUTH_USER_MODEL), ] operations = [ - migrations.RemoveField( - model_name='userrequest', - name='subscription', - ), - migrations.AlterUniqueTogether( - name='userrequest', - unique_together=None, - ), - migrations.RemoveField( - model_name='userrequest', - name='reviewer', - ), - migrations.RemoveField( - model_name='userrequest', - name='user', - ), - migrations.AlterField( - model_name='collection', - name='serialized_search_query', - field=models.TextField(blank=True, help_text='Initial search query that generated the collection.', null=True), - ), - migrations.AlterField( - model_name='searchquery', - name='hash', - field=models.TextField(blank=True, null=True), - ), - migrations.CreateModel( - name='SpecialMembershipDataset', - fields=[ - ('id', models.AutoField(auto_created=True, primary_key=True, serialize=False, verbose_name='ID')), - ('title', models.CharField(db_column='name', max_length=255)), - ('bitmap_position', models.PositiveIntegerField(blank=True, null=True, unique=True)), - ('metadata', models.JSONField(blank=True, default=dict)), - ('reviewer', models.ForeignKey(blank=True, null=True, on_delete=django.db.models.deletion.SET_NULL, related_name='reviewed_datasets', to=settings.AUTH_USER_MODEL)), + migrations.SeparateDatabaseAndState( + database_operations=[], + state_operations=[ + migrations.CreateModel( + name="SpecialMembershipDataset", + fields=[ + ( + "id", + models.AutoField( + auto_created=True, + primary_key=True, + serialize=False, + verbose_name="ID", + ), + ), + ("title", models.CharField(db_column="name", max_length=255)), + ( + "bitmap_position", + models.PositiveIntegerField( + blank=True, null=True, unique=True + ), + ), + ("metadata", models.JSONField(blank=True, default=dict)), + ( + "reviewer", + models.ForeignKey( + blank=True, + null=True, + on_delete=django.db.models.deletion.SET_NULL, + related_name="reviewed_datasets", + to=settings.AUTH_USER_MODEL, + ), + ), + ], + options={ + "verbose_name": "Special Membership Access", + "verbose_name_plural": "Special Membership Accesses", + "db_table": "impresso_datasetbitmapposition", + }, + ), + migrations.CreateModel( + name="UserBitmapSubscription", + fields=[ + ( + "id", + models.AutoField( + auto_created=True, + primary_key=True, + serialize=False, + verbose_name="ID", + ), + ), + ( + "specialmembershipdataset", + models.ForeignKey( + db_column="datasetbitmapposition_id", + on_delete=django.db.models.deletion.CASCADE, + to="impresso.specialmembershipdataset", + ), + ), + ( + "userbitmap", + models.ForeignKey( + db_column="userbitmap_id", + on_delete=django.db.models.deletion.CASCADE, + to="impresso.userbitmap", + ), + ), + ], + options={ + "verbose_name": "User Special Membership Access", + "verbose_name_plural": "User Special Membership Accesses", + "db_table": "impresso_userbitmap_subscriptions", + "unique_together": {("userbitmap", "specialmembershipdataset")}, + }, + ), + migrations.CreateModel( + name="UserSpecialMembershipRequest", + fields=[ + ( + "id", + models.AutoField( + auto_created=True, + primary_key=True, + serialize=False, + verbose_name="ID", + ), + ), + ("date_created", models.DateTimeField(auto_now_add=True)), + ("date_last_modified", models.DateTimeField(auto_now=True)), + ( + "status", + models.CharField( + choices=[ + ("pending", "Pending"), + ("approved", "Approved"), + ("temporary", "Approved (Temporary)"), + ("rejected", "Rejected"), + ("revoked", "Revoked"), + ], + default="pending", + max_length=10, + ), + ), + ( + "changelog", + models.JSONField(blank=True, default=list, null=True), + ), + ("notes", models.TextField(blank=True, null=True)), + ( + "reviewer", + models.ForeignKey( + blank=True, + null=True, + on_delete=django.db.models.deletion.SET_NULL, + related_name="review", + to=settings.AUTH_USER_MODEL, + ), + ), + ( + "subscription", + models.ForeignKey( + help_text="The special membership dataset being requested", + null=True, + on_delete=django.db.models.deletion.SET_NULL, + to="impresso.specialmembershipdataset", + ), + ), + ( + "user", + models.ForeignKey( + on_delete=django.db.models.deletion.CASCADE, + related_name="request", + to=settings.AUTH_USER_MODEL, + ), + ), + ], + options={ + "verbose_name": "User Special Membership Request", + "verbose_name_plural": "User Special Membership Requests", + "db_table": "impresso_userrequest", + "unique_together": {("user", "subscription")}, + }, + ), + migrations.AlterField( + model_name="userbitmap", + name="subscriptions", + field=models.ManyToManyField( + blank=True, + through="impresso.UserBitmapSubscription", + to="impresso.specialmembershipdataset", + ), + ), + migrations.DeleteModel(name="DatasetBitmapPosition"), + migrations.DeleteModel(name="UserRequest"), ], - options={ - 'verbose_name': 'Special Membership Access', - 'verbose_name_plural': 'Special Membership Accesses', - 'db_table': 'impresso_datasetbitmapposition', - }, - ), - migrations.CreateModel( - name='UserBitmapSubscription', - fields=[ - ('id', models.AutoField(auto_created=True, primary_key=True, serialize=False, verbose_name='ID')), - ('specialmembershipdataset', models.ForeignKey(db_column='datasetbitmapposition_id', on_delete=django.db.models.deletion.CASCADE, to='impresso.specialmembershipdataset')), - ('userbitmap', models.ForeignKey(db_column='userbitmap_id', on_delete=django.db.models.deletion.CASCADE, to='impresso.userbitmap')), - ], - options={ - 'verbose_name': 'User Special Membership Access', - 'verbose_name_plural': 'User Special Membership Accesses', - 'db_table': 'impresso_userbitmap_subscriptions', - 'unique_together': {('userbitmap', 'specialmembershipdataset')}, - }, - ), - migrations.AlterField( - model_name='userbitmap', - name='subscriptions', - field=models.ManyToManyField(blank=True, through='impresso.UserBitmapSubscription', to='impresso.specialmembershipdataset'), - ), - migrations.CreateModel( - name='UserSpecialMembershipRequest', - fields=[ - ('id', models.AutoField(auto_created=True, primary_key=True, serialize=False, verbose_name='ID')), - ('date_created', models.DateTimeField(auto_now_add=True)), - ('date_last_modified', models.DateTimeField(auto_now=True)), - ('temporary_expires_at', models.DateTimeField(blank=True, help_text='Expiration date used for temporary automatic approvals', null=True)), - ('status', models.CharField(choices=[('pending', 'Pending'), ('approved', 'Approved'), ('temporary', 'Approved (Temporary)'), ('rejected', 'Rejected'), ('revoked', 'Revoked')], default='pending', max_length=10)), - ('changelog', models.JSONField(blank=True, default=list, null=True)), - ('notes', models.TextField(blank=True, null=True)), - ('reviewer', models.ForeignKey(blank=True, null=True, on_delete=django.db.models.deletion.SET_NULL, related_name='review', to=settings.AUTH_USER_MODEL)), - ('subscription', models.ForeignKey(help_text='The special membership dataset being requested', null=True, on_delete=django.db.models.deletion.SET_NULL, to='impresso.specialmembershipdataset')), - ('user', models.ForeignKey(on_delete=django.db.models.deletion.CASCADE, related_name='request', to=settings.AUTH_USER_MODEL)), - ], - options={ - 'verbose_name': 'User Special Membership Request', - 'verbose_name_plural': 'User Special Membership Requests', - 'db_table': 'impresso_userrequest', - 'unique_together': {('user', 'subscription')}, - }, - ), - migrations.DeleteModel( - name='DatasetBitmapPosition', ), - migrations.DeleteModel( - name='UserRequest', + migrations.AddField( + model_name="userspecialmembershiprequest", + name="temporary_expires_at", + field=models.DateTimeField( + blank=True, + help_text="Expiration date used for temporary automatic approvals", + null=True, + ), ), ] diff --git a/impresso/models/userSpecialMembershipRequest.py b/impresso/models/userSpecialMembershipRequest.py index 4dae314..f7d1a54 100644 --- a/impresso/models/userSpecialMembershipRequest.py +++ b/impresso/models/userSpecialMembershipRequest.py @@ -25,7 +25,7 @@ class UserSpecialMembershipRequest(models.Model): UserRequest model represents a request made by a user for a subscription. Note: The unique_together constraint ensures that each user can only have one request per subscription, regardless of the reviewer. This is to prevent duplicate requests for the same subscription by the same user. - Check post_save_user_special_membership_request signal to handle the approval process. + Check `impresso.signals.post_save_user_special_membership_request` signal to handle the approval process. Attributes: STATUS_PENDING (str): Status indicating the request is pending. @@ -39,6 +39,7 @@ class UserSpecialMembershipRequest(models.Model): subscription (ForeignKey): Foreign key to the SpecialMembershipDataset model representing the subscription requested. date_created (DateTimeField): The date and time when the request was created. date_last_modified (DateTimeField): The date and time when the request was last modified. + temporary_expires_at (DateTimeField): The expiration date used for temporary automatic approvals. status (CharField): The current status of the request. changelog (JSONField): A list of changes made to the request. notes (TextField): Additional notes related to the request. diff --git a/impresso/tasks/userSpecialMembershipRequest_tasks.py b/impresso/tasks/userSpecialMembershipRequest_tasks.py index 706f0ca..7ab06e2 100644 --- a/impresso/tasks/userSpecialMembershipRequest_tasks.py +++ b/impresso/tasks/userSpecialMembershipRequest_tasks.py @@ -2,7 +2,7 @@ from celery.utils.log import get_task_logger from django.utils import timezone - +from django.conf import settings from ..celery import app from ..models.userSpecialMembershipRequest import UserSpecialMembershipRequest from impresso.utils.tasks.userSpecialMembershipRequest import ( @@ -20,12 +20,21 @@ def _is_temporary_auto_accept_enabled(req: UserSpecialMembershipRequest) -> bool return bool(req.subscription.metadata.get("enableTemporaryAutomaticAcceptance")) -def _get_revoke_after_days(req: UserSpecialMembershipRequest) -> int | None: +def _is_modality_cc_reviewer_enabled(req: UserSpecialMembershipRequest) -> bool: + if not req.subscription: + return False + return bool( + req.subscription.metadata.get("modality") + == settings.IMPRESSO_EMAIL_MODALITY_SPECIAL_MEMBERSHIP_REQUEST_CC_REVIEWER + ) + + +def _get_revoke_after_days(req: UserSpecialMembershipRequest) -> float | None: if not req.subscription: return None revoke_after_days = req.subscription.metadata.get("revokeAfterDays") - if isinstance(revoke_after_days, int) and revoke_after_days > 0: - return revoke_after_days + if isinstance(revoke_after_days, (int, float)) and revoke_after_days > 0: + return float(revoke_after_days) return None @@ -51,14 +60,16 @@ def _schedule_temporary_revocation(req: UserSpecialMembershipRequest) -> None: ) def after_special_membership_request_created(self, instance_id: int) -> None: """ - THe request has been created outside of the flow in the admin panel. - In this case we should manually apply the post-create actions. - + The special membership request (pk:{instance_id}) has already been created in the database. + In Django Admin site, this task is scheduled when the `impresso.signals.post_save_user_special_membership_request` signal is triggered; + in other backends, this task must be called manually using the backend Celery integration. + Note that this task can be called with a status other than PENDING, thus triggering the + `after_special_membership_request_updated` logic immediately and skipping the + `send_email_after_user_special_membership_request_created` logic. Args: self: The task instance. instance_id (int): The ID of the UserSpecialMembershipRequest instance that was created. """ - # get request try: req = UserSpecialMembershipRequest.objects.get(pk=instance_id) except UserSpecialMembershipRequest.DoesNotExist: @@ -67,11 +78,13 @@ def after_special_membership_request_created(self, instance_id: int) -> None: logger.info( f"[UserSpecialMembershipRequest:{instance_id}] triggered signals after_special_membership_request_created, for user={req.user.username} subscription={req.subscription.title if req.subscription else 'None'} status={req.status}" ) - + # If the request is pending and temporary auto-accept is enabled, approve it as temporary and schedule revocation, + # instead of going through the regular pending flow. if ( req.status == UserSpecialMembershipRequest.STATUS_PENDING and _is_temporary_auto_accept_enabled(req) ): + revoke_after_days = _get_revoke_after_days(req) if revoke_after_days is None: logger.warning( @@ -79,6 +92,10 @@ def after_special_membership_request_created(self, instance_id: int) -> None: ) else: req.status = UserSpecialMembershipRequest.STATUS_APPROVED_TEMPORARY + # add the temporary expiration date using the last modified date + revoke_after_days + req.temporary_expires_at = req.date_last_modified + timedelta( + days=revoke_after_days + ) req.save() logger.info( f"[instance:{instance_id}] auto-accepted as temporary for {revoke_after_days} day(s)" @@ -103,10 +120,14 @@ def after_special_membership_request_created(self, instance_id: int) -> None: def after_special_membership_request_updated(self, instance_id: int) -> None: """ Send emails after a UserSpecialMembershipRequest is updated. - TODO: - If its status is "STATUS_APPROVED", notify the user with the details of the special membership granted. - If its status is "STATUS_PENDING", notify the institution reviewer with the details of the request AND to the user that their request is pending review. - If its status is "STATUS_REJECTED", notify the user that their request has been rejected. + This task doesn't change the status of the request, but only reacts to the change by sending the appropriate email notifications and applying the special membership to the bitmap. + + In Django Admin site, this task is scheduled when the `impresso.signals.post_save_user_special_membership_request` signal is triggered and the instance is not newly created (i.e., it's an update); + in other backends, this task must be called manually using the backend Celery integration. + + If current status is "STATUS_APPROVED", congratulate the user with the details of the special membership granted. + If current status is "STATUS_PENDING", notify the institution reviewer with the details of the request AND to the user that their request is pending review. + If current status is "STATUS_REJECTED", notify the user that their request has been rejected. Args: self: The task instance. @@ -128,10 +149,15 @@ def after_special_membership_request_updated(self, instance_id: int) -> None: ) apply_special_membership_to_bitmap(instance=req, created=False, logger=logger) send_email_after_user_special_membership_request_updated( - instance=req, logger=logger + instance=req, + logger=logger, + is_modality_cc_reviewer_enabled=_is_modality_cc_reviewer_enabled(req), ) if req.status == UserSpecialMembershipRequest.STATUS_APPROVED_TEMPORARY: + logger.info( + f"[instance:{instance_id}] STATUS_APPROVED_TEMPORARY, scheduling revocation..." + ) _schedule_temporary_revocation(req) @@ -171,7 +197,9 @@ def revoke_special_membership_request(self, instance_id: int) -> None: ) apply_special_membership_to_bitmap(instance=req, created=False, logger=logger) send_email_after_user_special_membership_request_updated( - instance=req, logger=logger + instance=req, + logger=logger, + is_modality_cc_reviewer_enabled=_is_modality_cc_reviewer_enabled(req), ) @@ -189,15 +217,15 @@ def revoke_expired_temporary_memberships_beat(self) -> None: """ expired_requests = UserSpecialMembershipRequest.objects.filter( status=UserSpecialMembershipRequest.STATUS_APPROVED_TEMPORARY, - temporary_expires_at__lt=timezone.now() + temporary_expires_at__lt=timezone.now(), ) - + count = expired_requests.count() if count == 0: logger.info("No expired temporary memberships found to revoke.") return logger.info(f"Found {count} expired temporary memberships to revoke.") - + for req in expired_requests: revoke_special_membership_request.delay(instance_id=req.pk) diff --git a/impresso/templates/emails/user_special_membership_request_approved_temporary_to_user.txt b/impresso/templates/emails/user_special_membership_request_approved_temporary_to_user.txt index c75da5d..74cadf9 100644 --- a/impresso/templates/emails/user_special_membership_request_approved_temporary_to_user.txt +++ b/impresso/templates/emails/user_special_membership_request_approved_temporary_to_user.txt @@ -1,13 +1,31 @@ +{% load tz %} + Dear {{ user.first_name }}, -Great news: your request for special membership access has been temporarily approved. +We have received your request for Special Membership Access to {{ user_special_membership_request.subscription.title }}. +To allow you to begin your work immediately, your account has been granted Provisional Access for a period of {{ user_special_membership_request_duration }} +until {{ user_special_membership_request.temporary_expires_at|date:"l, Y-m-d H:i" }} +and gives you full access contents from Test Dataset. Note that Impresso Terms of Use always apply. + +Next Steps & Extensions: +Access will be automatically revoked after {{ user_special_membership_request_duration }} from now. For continued access, please request "Standard Access" via the Impresso platform. +Please note that institutional approval for Standard Access can take some time. You may contact the institutional reviewer directly by replying to this email. + +Here are the details of the access request for your reference: -You now have access to: {{ user_special_membership_request.subscription.title }}. + Requester: {{ user.get_full_name|default:user.username }} + Requester Email: {{ user.email }} + Requester Plan: {{ plan_label }} -This temporary access is valid until {{ user_special_membership_request.temporary_expires_at|date:"Y-m-d H:i" }}. -After that date, access will be automatically revoked unless a permanent approval is granted. + Affiliation: {% if user.profile.affiliation %}{{ user.profile.affiliation }}{% else %}Not provided{% endif %} + Public Institutional Webpage: {% if user.profile.institutional_url %}{{ user.profile.institutional_url }}{% else %}Not provided{% endif %} + + Membership Option: {{ user_special_membership_request.subscription.title }} + Request Date: {{ user_special_membership_request.date_created|timezone:"UTC"|date:"l, d.m.Y, H:i T" }} + Expiration Date: {{ user_special_membership_request.temporary_expires_at|timezone:"UTC"|date:"l, d.m.Y, H:i T" }} + Request Status: {{ user_special_membership_request.status}} -If you have questions or need an extension, please reply to this email. +Should you need any further assistance from our side, the Impresso Team remains at your service. -Best regards, -The Impresso Team +Warm regards, +The Impresso Team \ No newline at end of file diff --git a/impresso/tests/management/test_checktemporarymemberships.py b/impresso/tests/management/commands/test_checktemporarymemberships.py similarity index 100% rename from impresso/tests/management/test_checktemporarymemberships.py rename to impresso/tests/management/commands/test_checktemporarymemberships.py diff --git a/impresso/tests/utils/tasks/test_userSpecialMembershipRequest.py b/impresso/tests/utils/tasks/test_userSpecialMembershipRequest.py index 361eb68..31fe4ed 100644 --- a/impresso/tests/utils/tasks/test_userSpecialMembershipRequest.py +++ b/impresso/tests/utils/tasks/test_userSpecialMembershipRequest.py @@ -2,7 +2,7 @@ from datetime import timedelta from django.conf import settings -from django.test import TestCase +from django.test import TestCase, TransactionTestCase from django.contrib.auth.models import Group, User from django.core import mail from django.utils import timezone @@ -361,7 +361,7 @@ def test_reviewer_email_html_alternative(self): self.assertGreater(len(reviewer_email.alternatives), 0) -class TestTemporaryAutomaticAcceptance(TestCase): +class TestTemporaryAutomaticAcceptance(TransactionTestCase): def setUp(self): self.reviewer = User.objects.create_user( username="temp-reviewer", @@ -386,29 +386,85 @@ def setUp(self): mail.outbox = [] def test_created_request_is_auto_approved_temporarily(self): - req = UserSpecialMembershipRequest.objects.create( - user=self.user, - reviewer=self.reviewer, - subscription=self.dataset, - status=UserSpecialMembershipRequest.STATUS_PENDING, - ) + from unittest.mock import patch mail.outbox = [] - after_special_membership_request_created(None, req.pk) - req.refresh_from_db() + with patch( + "impresso.tasks.userSpecialMembershipRequest_tasks.revoke_special_membership_request.apply_async" + ) as mock_apply_async: + req = UserSpecialMembershipRequest.objects.create( + user=self.user, + reviewer=self.reviewer, + subscription=self.dataset, + status=UserSpecialMembershipRequest.STATUS_PENDING, + ) + + req.refresh_from_db() + + self.assertEqual( + req.status, + UserSpecialMembershipRequest.STATUS_APPROVED_TEMPORARY, + ) + self.assertEqual(self.user.bitmap.subscriptions.count(), 1) + self.assertEqual(len(mail.outbox), 1) + self.assertEqual( + mail.outbox[0].subject, + settings.IMPRESSO_EMAIL_SUBJECT_AFTER_USER_SPECIAL_MEMBERSHIP_REQUEST_ACCEPTED_TEMPORARY_TO_USER, + ) + + mock_apply_async.assert_called_once() + _, call_kwargs = mock_apply_async.call_args + self.assertEqual(call_kwargs["kwargs"], {"instance_id": req.pk}) + self.assertIn("eta", call_kwargs) + logger.info( + f"Scheduled revocation ETA: {call_kwargs['kwargs']['instance_id']} at {call_kwargs['eta']}" + ) + + def test_created_requests_with_float_days_for_revoke_after_days(self): + from unittest.mock import patch - self.assertEqual( - req.status, - UserSpecialMembershipRequest.STATUS_APPROVED_TEMPORARY, - ) - self.assertIsNotNone(req.temporary_expires_at) - self.assertEqual(self.user.bitmap.subscriptions.count(), 1) - self.assertEqual(len(mail.outbox), 1) - self.assertEqual( - mail.outbox[0].subject, - settings.IMPRESSO_EMAIL_SUBJECT_AFTER_USER_SPECIAL_MEMBERSHIP_REQUEST_ACCEPTED_TEMPORARY_TO_USER, + dataset_with_float_revoke_after_days = SpecialMembershipDataset.objects.create( + title="Float Revoke After Days Dataset", + reviewer=self.reviewer, + metadata={ + "enableTemporaryAutomaticAcceptance": True, + "revokeAfterDays": 0.5, # 12 hours + "modality": settings.IMPRESSO_EMAIL_MODALITY_SPECIAL_MEMBERSHIP_REQUEST_NOTIFY_REVIEWER, + }, ) + mail.outbox = [] + with patch( + "impresso.tasks.userSpecialMembershipRequest_tasks.revoke_special_membership_request.apply_async" + ) as mock_apply_async: + req = UserSpecialMembershipRequest.objects.create( + user=self.user, + reviewer=self.reviewer, + subscription=dataset_with_float_revoke_after_days, + status=UserSpecialMembershipRequest.STATUS_PENDING, + ) + + req.refresh_from_db() + + self.assertEqual( + req.status, + UserSpecialMembershipRequest.STATUS_APPROVED_TEMPORARY, + ) + self.assertEqual(self.user.bitmap.subscriptions.count(), 1) + self.assertEqual(len(mail.outbox), 1) + self.assertEqual( + mail.outbox[0].subject, + settings.IMPRESSO_EMAIL_SUBJECT_AFTER_USER_SPECIAL_MEMBERSHIP_REQUEST_ACCEPTED_TEMPORARY_TO_USER, + ) + + mock_apply_async.assert_called_once() + _, call_kwargs = mock_apply_async.call_args + self.assertEqual(call_kwargs["kwargs"], {"instance_id": req.pk}) + self.assertIn("eta", call_kwargs) + logger.info( + f"Scheduled revocation ETA: {call_kwargs['kwargs']['instance_id']} at {call_kwargs['eta']}" + ) + class TestTemporaryAutomaticRevocation(TestCase): def setUp(self): @@ -434,20 +490,25 @@ def test_expired_temporary_request_is_revoked(self): self.user.bitmap.subscriptions.add(self.dataset) mail.outbox = [] - revoke_special_membership_request(None, req.pk) + revoke_special_membership_request.delay(instance_id=req.pk) req.refresh_from_db() self.assertEqual(req.status, UserSpecialMembershipRequest.STATUS_REVOKED) self.assertEqual(self.user.bitmap.subscriptions.count(), 0) - self.assertEqual(len(mail.outbox), 1) - self.assertEqual( - mail.outbox[0].subject, - settings.IMPRESSO_EMAIL_SUBJECT_AFTER_USER_SPECIAL_MEMBERSHIP_REQUEST_REVOKED_TO_USER, + self.assertEqual(len(mail.outbox), 2) + self.assertTrue( + all( + email.subject + == settings.IMPRESSO_EMAIL_SUBJECT_AFTER_USER_SPECIAL_MEMBERSHIP_REQUEST_REVOKED_TO_USER + for email in mail.outbox + ) ) def test_revoke_expired_temporary_memberships_beat(self): from unittest.mock import patch - from impresso.tasks.userSpecialMembershipRequest_tasks import revoke_expired_temporary_memberships_beat + from impresso.tasks.userSpecialMembershipRequest_tasks import ( + revoke_expired_temporary_memberships_beat, + ) req1 = UserSpecialMembershipRequest.objects.create( user=self.user, @@ -455,16 +516,20 @@ def test_revoke_expired_temporary_memberships_beat(self): status=UserSpecialMembershipRequest.STATUS_APPROVED_TEMPORARY, temporary_expires_at=timezone.now() - timedelta(days=1), ) - - user2 = User.objects.create_user(username="other-user") - req2 = UserSpecialMembershipRequest.objects.create( + + user2 = User.objects.create_user( + username="other-user", email="other-user@example.com" + ) + UserSpecialMembershipRequest.objects.create( user=user2, subscription=self.dataset, status=UserSpecialMembershipRequest.STATUS_APPROVED_TEMPORARY, temporary_expires_at=timezone.now() + timedelta(days=1), ) - with patch("impresso.tasks.userSpecialMembershipRequest_tasks.revoke_special_membership_request.delay") as mock_delay: - revoke_expired_temporary_memberships_beat(None) - + with patch( + "impresso.tasks.userSpecialMembershipRequest_tasks.revoke_special_membership_request.delay" + ) as mock_delay: + revoke_expired_temporary_memberships_beat.delay() + mock_delay.assert_called_once_with(instance_id=req1.pk) diff --git a/impresso/utils/tasks/userSpecialMembershipRequest.py b/impresso/utils/tasks/userSpecialMembershipRequest.py index 463573d..84f1bb4 100644 --- a/impresso/utils/tasks/userSpecialMembershipRequest.py +++ b/impresso/utils/tasks/userSpecialMembershipRequest.py @@ -60,6 +60,7 @@ def apply_special_membership_to_bitmap( def send_email_after_user_special_membership_request_updated( instance: UserSpecialMembershipRequest, fail_silently: bool = False, + is_modality_cc_reviewer_enabled: bool = False, logger: Logger = default_logger, ) -> None: """ @@ -68,10 +69,29 @@ def send_email_after_user_special_membership_request_updated( Args: instance (UserSpecialMembershipRequest): The UserSpecialMembershipRequest instance. fail_silently (bool, optional): Whether to fail silently if there is an error sending the email. Defaults to False. + + is_modality_cc_reviewer_enabled (bool, optional): Whether the modality for CC reviewers is enabled. Defaults to False. logger (Logger, optional): The logger to use for logging information. Defaults to default_logger. Raises: Exception: If there is an error sending the email. """ + reply_to = [] + plan_label, plan_group = get_plan_from_user_groups(instance.user) + duration = None + if instance.temporary_expires_at: + delta = instance.temporary_expires_at - instance.date_created + total_hours = int(delta.total_seconds() // 3600) + if total_hours < 24: + duration = f"{total_hours} hour{'s' if total_hours != 1 else ''}" + else: + total_days = int(delta.total_seconds() // 86400) + duration = f"{total_days} day{'s' if total_days != 1 else ''}" + if is_modality_cc_reviewer_enabled: + reviewer = instance.reviewer or ( + instance.subscription.reviewer if instance.subscription else None + ) + if reviewer and reviewer.email: + reply_to.append(reviewer.email) if instance.status == UserSpecialMembershipRequest.STATUS_APPROVED: template = "user_special_membership_request_approved_to_user" subject = ( @@ -104,15 +124,16 @@ def send_email_after_user_special_membership_request_updated( context={ "user": instance.user, "user_special_membership_request": instance, + "plan_label": plan_label, + "plan_group": plan_group, + "user_special_membership_request_duration": duration, }, from_email=settings.IMPRESSO_EMAIL_LABEL_DEFAULT_FROM_EMAIL, to=[ instance.user.email, ], cc=[], - reply_to=[ - settings.DEFAULT_FROM_EMAIL, - ], + reply_to=reply_to, logger=logger, fail_silently=fail_silently, ) From 06f027ff2f03f9bdd5674da6dbff48fed2fd08fd Mon Sep 17 00:00:00 2001 From: Daniele Guido <1181642+danieleguido@users.noreply.github.com> Date: Tue, 5 May 2026 17:03:13 +0200 Subject: [PATCH 12/30] add custom command to create SMR --- .../createspecialmembershiprequest.py | 60 ++++++++ .../tests/management/commands/__init__.py | 0 .../test_createspecialmembershiprequest.py | 132 ++++++++++++++++++ 3 files changed, 192 insertions(+) create mode 100644 impresso/management/commands/createspecialmembershiprequest.py create mode 100644 impresso/tests/management/commands/__init__.py create mode 100644 impresso/tests/management/commands/test_createspecialmembershiprequest.py diff --git a/impresso/management/commands/createspecialmembershiprequest.py b/impresso/management/commands/createspecialmembershiprequest.py new file mode 100644 index 0000000..4f23156 --- /dev/null +++ b/impresso/management/commands/createspecialmembershiprequest.py @@ -0,0 +1,60 @@ +from typing import Any + +from django.contrib.auth.models import User +from django.core.management.base import BaseCommand, CommandError +from django.db import IntegrityError + +from impresso.models import SpecialMembershipDataset, UserSpecialMembershipRequest + + +class Command(BaseCommand): + help = ( + "Create a pending special membership request on behalf of a user " + "using user email and dataset id." + ) + + def add_arguments(self, parser) -> None: + parser.add_argument( + "user_email", + type=str, + help="Email of the user for whom the request will be created", + ) + parser.add_argument( + "dataset_id", + type=int, + help="SpecialMembershipDataset id", + ) + + def handle(self, user_email: str, dataset_id: int, *args: Any, **options: Any) -> None: + try: + user = User.objects.get(email=user_email) + except User.DoesNotExist as exc: + raise CommandError(f"User with email '{user_email}' does not exist.") from exc + + try: + dataset = SpecialMembershipDataset.objects.select_related("reviewer").get( + pk=dataset_id + ) + except SpecialMembershipDataset.DoesNotExist as exc: + raise CommandError( + f"SpecialMembershipDataset with id={dataset_id} does not exist." + ) from exc + + try: + request = UserSpecialMembershipRequest.objects.create( + user=user, + reviewer=dataset.reviewer, + subscription=dataset, + status=UserSpecialMembershipRequest.STATUS_PENDING, + ) + except IntegrityError as exc: + raise CommandError( + "A request for this user and dataset already exists or could not be created." + ) from exc + + self.stdout.write( + self.style.SUCCESS( + "Created special membership request " + f"id={request.pk} for user='{user.email}' and dataset_id={dataset.pk}." + ) + ) \ No newline at end of file diff --git a/impresso/tests/management/commands/__init__.py b/impresso/tests/management/commands/__init__.py new file mode 100644 index 0000000..e69de29 diff --git a/impresso/tests/management/commands/test_createspecialmembershiprequest.py b/impresso/tests/management/commands/test_createspecialmembershiprequest.py new file mode 100644 index 0000000..85b49fe --- /dev/null +++ b/impresso/tests/management/commands/test_createspecialmembershiprequest.py @@ -0,0 +1,132 @@ +from io import StringIO +from django.core import mail +from django.contrib.auth.models import User +from django.core.management import CommandError, call_command +from django.test import TestCase + +from impresso.models import SpecialMembershipDataset, UserSpecialMembershipRequest +from impresso.signals import create_default_groups + + +class TestCreateSpecialMembershipRequestCommand(TestCase): + """Test the createspecialmembershiprequest management command. + ENV=test pipenv run python manage.py test impresso.tests.management.commands.test_createspecialmembershiprequest.TestCreateSpecialMembershipRequestCommand + """ + + def setUp(self) -> None: + create_default_groups(sender="impresso") + + self.reviewer = User.objects.create_user( + username="reviewer", + first_name="John", + last_name="Reviewer", + email="reviewer@example.com", + password="testpass123", + ) + self.user = User.objects.create_user( + username="request-user", + first_name="Alice", + last_name="Smith", + email="alice@example.com", + password="testpass123", + ) + self.dataset = SpecialMembershipDataset.objects.create( + title="Dataset Alpha", + reviewer=self.reviewer, + ) + + def test_creates_request_with_1_day_revokeable_period(self) -> None: + out = StringIO() + dataset_with_revokeable_period = SpecialMembershipDataset.objects.create( + title="Dataset with Revokeable Period", + reviewer=self.reviewer, + metadata={ + "revokeAfterDays": 1, + "enableTemporaryAutomaticAcceptance": True, + }, + ) + mail.outbox = [] + call_command( + "createspecialmembershiprequest", + self.user.email, + str(dataset_with_revokeable_period.pk), + stdout=out, + ) + print(mail.outbox[0].body) + request = UserSpecialMembershipRequest.objects.get( + user=self.user, + subscription=dataset_with_revokeable_period, + ) + print("olalalla this terminates here", request.date_created, request.status) + print(mail.outbox[1].body) + print(out.getvalue()) + # date dataset_with_revokeable_period should sset and be more or less 24 h from now + self.assertIsNotNone(request.temporary_expires_at) + now = request.date_created + expires_at = request.temporary_expires_at + + self.assertIn( + "Created special membership request", + out.getvalue(), + ) + + def test_creates_pending_request_for_user_and_dataset(self) -> None: + out = StringIO() + + call_command( + "createspecialmembershiprequest", + self.user.email, + str(self.dataset.pk), + stdout=out, + ) + + request = UserSpecialMembershipRequest.objects.get( + user=self.user, + subscription=self.dataset, + ) + self.assertEqual(request.status, UserSpecialMembershipRequest.STATUS_PENDING) + self.assertEqual(request.reviewer, self.reviewer) + self.assertIn( + "Created special membership request", + out.getvalue(), + ) + + def test_fails_when_user_does_not_exist(self) -> None: + with self.assertRaisesMessage( + CommandError, + "User with email 'missing@example.com' does not exist.", + ): + call_command( + "createspecialmembershiprequest", + "missing@example.com", + str(self.dataset.pk), + ) + + def test_fails_when_dataset_does_not_exist(self) -> None: + with self.assertRaisesMessage( + CommandError, + "SpecialMembershipDataset with id=99999 does not exist.", + ): + call_command( + "createspecialmembershiprequest", + self.user.email, + "99999", + ) + + def test_fails_when_request_already_exists(self) -> None: + UserSpecialMembershipRequest.objects.create( + user=self.user, + reviewer=self.reviewer, + subscription=self.dataset, + status=UserSpecialMembershipRequest.STATUS_PENDING, + ) + + with self.assertRaisesMessage( + CommandError, + "A request for this user and dataset already exists or could not be created.", + ): + call_command( + "createspecialmembershiprequest", + self.user.email, + str(self.dataset.pk), + ) From 2181263c182e65c15fe13a122d151e482c6a77d3 Mon Sep 17 00:00:00 2001 From: Daniele Guido Date: Wed, 6 May 2026 08:37:30 +0200 Subject: [PATCH 13/30] add status label to email templates and add this to specific command --- AGENTS.md | 2 + .../createspecialmembershiprequest.py | 35 +++++++++-- .../models/userSpecialMembershipRequest.py | 17 ++--- ...embership_request_created_to_reviewer.html | 2 +- ...membership_request_created_to_reviewer.txt | 2 +- ...al_membership_request_created_to_user.html | 2 +- ...ial_membership_request_created_to_user.txt | 2 +- ...p_request_created_to_user_cc_reviewer.html | 2 +- ...ip_request_created_to_user_cc_reviewer.txt | 2 +- .../test_createspecialmembershiprequest.py | 63 +++++++++++++++++++ .../tasks/userSpecialMembershipRequest.py | 10 ++- 11 files changed, 120 insertions(+), 19 deletions(-) diff --git a/AGENTS.md b/AGENTS.md index 4651156..be7ace1 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -52,6 +52,8 @@ When eager mode is enabled by the custom test runner, `apply_async(..., eta=...) For scheduling assertions, patch the task method and assert it was called with the expected `eta` and kwargs, for example: ```python +from unittest.mock import patch + with patch( "impresso.tasks.userSpecialMembershipRequest_tasks.revoke_special_membership_request.apply_async" ) as mock_apply_async: diff --git a/impresso/management/commands/createspecialmembershiprequest.py b/impresso/management/commands/createspecialmembershiprequest.py index 4f23156..a50fb7f 100644 --- a/impresso/management/commands/createspecialmembershiprequest.py +++ b/impresso/management/commands/createspecialmembershiprequest.py @@ -24,12 +24,38 @@ def add_arguments(self, parser) -> None: type=int, help="SpecialMembershipDataset id", ) + parser.add_argument( + "--status", + type=str, + default=UserSpecialMembershipRequest.STATUS_PENDING, + choices=[ + UserSpecialMembershipRequest.STATUS_PENDING, + UserSpecialMembershipRequest.STATUS_APPROVED, + UserSpecialMembershipRequest.STATUS_APPROVED_TEMPORARY, + UserSpecialMembershipRequest.STATUS_REJECTED, + UserSpecialMembershipRequest.STATUS_REVOKED, + ], + help=( + "Initial status of the request " + f"(default: {UserSpecialMembershipRequest.STATUS_PENDING})" + ), + ) + parser.add_argument( + "--notes", + type=str, + default=None, + help="Optional notes to attach to the request", + ) - def handle(self, user_email: str, dataset_id: int, *args: Any, **options: Any) -> None: + def handle( + self, user_email: str, dataset_id: int, *args: Any, **options: Any + ) -> None: try: user = User.objects.get(email=user_email) except User.DoesNotExist as exc: - raise CommandError(f"User with email '{user_email}' does not exist.") from exc + raise CommandError( + f"User with email '{user_email}' does not exist." + ) from exc try: dataset = SpecialMembershipDataset.objects.select_related("reviewer").get( @@ -45,7 +71,8 @@ def handle(self, user_email: str, dataset_id: int, *args: Any, **options: Any) - user=user, reviewer=dataset.reviewer, subscription=dataset, - status=UserSpecialMembershipRequest.STATUS_PENDING, + status=options["status"], + notes=options["notes"], ) except IntegrityError as exc: raise CommandError( @@ -57,4 +84,4 @@ def handle(self, user_email: str, dataset_id: int, *args: Any, **options: Any) - "Created special membership request " f"id={request.pk} for user='{user.email}' and dataset_id={dataset.pk}." ) - ) \ No newline at end of file + ) diff --git a/impresso/models/userSpecialMembershipRequest.py b/impresso/models/userSpecialMembershipRequest.py index f7d1a54..602a2b0 100644 --- a/impresso/models/userSpecialMembershipRequest.py +++ b/impresso/models/userSpecialMembershipRequest.py @@ -60,7 +60,14 @@ class UserSpecialMembershipRequest(models.Model): STATUS_APPROVED_TEMPORARY = "temporary" STATUS_REJECTED = "rejected" STATUS_REVOKED = "revoked" - + # Define the choices for the status field using the class-level constants + STATUS_CHOICES = ( + (STATUS_PENDING, "Pending"), + (STATUS_APPROVED, "Approved"), + (STATUS_APPROVED_TEMPORARY, "Approved (Temporary)"), + (STATUS_REJECTED, "Rejected"), + (STATUS_REVOKED, "Revoked"), + ) user = models.ForeignKey(User, on_delete=models.CASCADE, related_name="request") reviewer = models.ForeignKey( User, on_delete=models.SET_NULL, related_name="review", null=True, blank=True @@ -81,13 +88,7 @@ class UserSpecialMembershipRequest(models.Model): status = models.CharField( max_length=10, default=STATUS_PENDING, - choices=( - (STATUS_PENDING, "Pending"), - (STATUS_APPROVED, "Approved"), - (STATUS_APPROVED_TEMPORARY, "Approved (Temporary)"), - (STATUS_REJECTED, "Rejected"), - (STATUS_REVOKED, "Revoked"), - ), + choices=STATUS_CHOICES, ) changelog = models.JSONField(null=True, blank=True, default=list) notes = models.TextField(null=True, blank=True) diff --git a/impresso/templates/emails/user_special_membership_request_created_to_reviewer.html b/impresso/templates/emails/user_special_membership_request_created_to_reviewer.html index 179c636..32875c3 100644 --- a/impresso/templates/emails/user_special_membership_request_created_to_reviewer.html +++ b/impresso/templates/emails/user_special_membership_request_created_to_reviewer.html @@ -30,7 +30,7 @@
  • Request Date: {{ user_special_membership_request.date_created|date:"l, d.m.Y, H:i T" }}
    • -
  • Request Status: Pending Review
    • +
  • Request Status: {{ status_label }}
  • Notes: {% if user_special_membership_request.notes %} diff --git a/impresso/templates/emails/user_special_membership_request_created_to_reviewer.txt b/impresso/templates/emails/user_special_membership_request_created_to_reviewer.txt index c6af73a..e913c01 100644 --- a/impresso/templates/emails/user_special_membership_request_created_to_reviewer.txt +++ b/impresso/templates/emails/user_special_membership_request_created_to_reviewer.txt @@ -16,7 +16,7 @@ Here are the details of the access request for your reference: Membership Option: {{ user_special_membership_request.subscription.title }} Request Date: {{ user_special_membership_request.date_created|date:"l, d.m.Y, H:i T" }} - Request Status: Pending Review + Request Status: {{ status_label }} Notes: {% if user_special_membership_request.notes %}{{ user_special_membership_request.notes }}{% else %}No notes provided.{% endif %} diff --git a/impresso/templates/emails/user_special_membership_request_created_to_user.html b/impresso/templates/emails/user_special_membership_request_created_to_user.html index 46302b2..a30117c 100644 --- a/impresso/templates/emails/user_special_membership_request_created_to_user.html +++ b/impresso/templates/emails/user_special_membership_request_created_to_user.html @@ -8,7 +8,7 @@
    • Membership Option: {{ user_special_membership_request.subscription.title }}
    • Request Submitted: {{ user_special_membership_request.date_created | date:"l, d.m.Y, H:i T" }}
      • -
    • Current Status: Under Review
      • +
    • Current Status: {{ status_label }}

    diff --git a/impresso/templates/emails/user_special_membership_request_created_to_user.txt b/impresso/templates/emails/user_special_membership_request_created_to_user.txt index f9d7876..cc06fe0 100644 --- a/impresso/templates/emails/user_special_membership_request_created_to_user.txt +++ b/impresso/templates/emails/user_special_membership_request_created_to_user.txt @@ -6,7 +6,7 @@ Here are the details of the access you requested, for your reference: Membership Option: {{ user_special_membership_request.subscription.title }} Request Date: {{ user_special_membership_request.date_created | date:"l, d.m.Y, H:i T" }} - Current Status: Pending Review + Current Status: {{ status_label }} Notes: {% if user_special_membership_request.notes %}{{ user_special_membership_request.notes }}{% else %}No notes provided.{% endif %} diff --git a/impresso/templates/emails/user_special_membership_request_created_to_user_cc_reviewer.html b/impresso/templates/emails/user_special_membership_request_created_to_user_cc_reviewer.html index d529784..d73a49d 100644 --- a/impresso/templates/emails/user_special_membership_request_created_to_user_cc_reviewer.html +++ b/impresso/templates/emails/user_special_membership_request_created_to_user_cc_reviewer.html @@ -47,7 +47,7 @@ Request Date: {{ user_special_membership_request.date_created|date:"l, d.m.Y, H:i T" }}

    • -
  • Request Status: Pending Review
    • +
  • Request Status: {{ status_label }}
  • Notes: {% if user_special_membership_request.notes %} diff --git a/impresso/templates/emails/user_special_membership_request_created_to_user_cc_reviewer.txt b/impresso/templates/emails/user_special_membership_request_created_to_user_cc_reviewer.txt index dff5f9d..c9c087c 100644 --- a/impresso/templates/emails/user_special_membership_request_created_to_user_cc_reviewer.txt +++ b/impresso/templates/emails/user_special_membership_request_created_to_user_cc_reviewer.txt @@ -17,7 +17,7 @@ Here are the details of the access request for your reference: Membership Option: {{ user_special_membership_request.subscription.title }} Request Date: {{ user_special_membership_request.date_created|date:"l, d.m.Y, H:i T" }} - Request Status: Pending Review + Request Status: {{ status_label }} Notes: {% if user_special_membership_request.notes %}{{ user_special_membership_request.notes }}{% else %}No notes provided.{% endif %} diff --git a/impresso/tests/management/commands/test_createspecialmembershiprequest.py b/impresso/tests/management/commands/test_createspecialmembershiprequest.py index 85b49fe..642242a 100644 --- a/impresso/tests/management/commands/test_createspecialmembershiprequest.py +++ b/impresso/tests/management/commands/test_createspecialmembershiprequest.py @@ -7,6 +7,8 @@ from impresso.models import SpecialMembershipDataset, UserSpecialMembershipRequest from impresso.signals import create_default_groups +from unittest.mock import patch + class TestCreateSpecialMembershipRequestCommand(TestCase): """Test the createspecialmembershiprequest management command. @@ -130,3 +132,64 @@ def test_fails_when_request_already_exists(self) -> None: self.user.email, str(self.dataset.pk), ) + + +class TestCreateSpecialMembershipRequestCommandWithOptions(TestCase): + """Test the createspecialmembershiprequest management command with different options. + ENV=test pipenv run python manage.py test impresso.tests.management.commands.test_createspecialmembershiprequest.TestCreateSpecialMembershipRequestCommandWithOptions + """ + + def setUp(self) -> None: + create_default_groups(sender="impresso") + + create_default_groups(sender="impresso") + + self.reviewer = User.objects.create_user( + username="reviewer", + first_name="John", + last_name="Reviewer", + email="reviewer@example.com", + password="testpass123", + ) + self.user = User.objects.create_user( + username="request-user", + first_name="Alice", + last_name="Smith", + email="alice@example.com", + password="testpass123", + ) + self.dataset = SpecialMembershipDataset.objects.create( + title="Dataset Alpha", + reviewer=self.reviewer, + ) + + def test_add_notes_and_approved_status(self) -> None: + out = StringIO() + mail.outbox = [] + call_command( + "createspecialmembershiprequest", + self.user.email, + str(self.dataset.pk), + "--status", + UserSpecialMembershipRequest.STATUS_APPROVED, + "--notes", + "This is an approved request with notes.", + stdout=out, + ) + print(len(mail.outbox)) + print(mail.outbox[1].body) + request = UserSpecialMembershipRequest.objects.get( + user=self.user, + subscription=self.dataset, + ) + self.assertEqual(request.status, UserSpecialMembershipRequest.STATUS_APPROVED) + self.assertEqual(request.reviewer, self.reviewer) + self.assertIn( + "Created special membership request", + out.getvalue(), + ) + + +# with patch( +# "impresso.tasks.userSpecialMembershipRequest_tasks.revoke_special_membership_request.apply_async" +# ) as mock_apply_async: diff --git a/impresso/utils/tasks/userSpecialMembershipRequest.py b/impresso/utils/tasks/userSpecialMembershipRequest.py index 84f1bb4..5e95412 100644 --- a/impresso/utils/tasks/userSpecialMembershipRequest.py +++ b/impresso/utils/tasks/userSpecialMembershipRequest.py @@ -118,6 +118,9 @@ def send_email_after_user_special_membership_request_updated( ) template = "user_special_membership_request_pending_to_user" + status_label = dict(UserSpecialMembershipRequest.STATUS_CHOICES).get( + instance.status, instance.status + ) send_templated_email_with_context( template=template, subject=subject, @@ -127,6 +130,7 @@ def send_email_after_user_special_membership_request_updated( "plan_label": plan_label, "plan_group": plan_group, "user_special_membership_request_duration": duration, + "status_label": status_label, }, from_email=settings.IMPRESSO_EMAIL_LABEL_DEFAULT_FROM_EMAIL, to=[ @@ -170,7 +174,9 @@ def send_email_after_user_special_membership_request_created( ) plan_label, plan_group = get_plan_from_user_groups(instance.user) number_of_special_memberships = get_number_of_special_memberships(instance.user) - + status_label = dict(UserSpecialMembershipRequest.STATUS_CHOICES).get( + instance.status, instance.status + ) # get modality from instance metadata, default to NOTIFY_REVIEWER if no reviewer or no modality specified modality = ( instance.subscription.metadata.get("modality", None) @@ -215,6 +221,7 @@ def send_email_after_user_special_membership_request_created( "plan_label": plan_label, "plan_group": plan_group, "number_of_special_memberships": number_of_special_memberships, + "status_label": status_label, }, from_email=settings.IMPRESSO_EMAIL_LABEL_DEFAULT_FROM_EMAIL, to=[ @@ -249,6 +256,7 @@ def send_email_after_user_special_membership_request_created( "user_special_membership_request": instance, "plan_label": plan_label, "plan_group": plan_group, + "status_label": status_label, "number_of_special_memberships": number_of_special_memberships, }, from_email=settings.IMPRESSO_EMAIL_LABEL_DEFAULT_FROM_EMAIL, From d8680cc0b3563db104fcaad15869ef041e2bd6c8 Mon Sep 17 00:00:00 2001 From: Daniele Guido Date: Wed, 6 May 2026 09:12:16 +0200 Subject: [PATCH 14/30] align templates HTML and TXT --- .../userSpecialMembershipRequest_tasks.py | 13 +++++ .../account_created_mailto_educational.html | 3 +- .../account_created_mailto_researcher.html | 3 +- .../emails/account_created_mailto_user.html | 3 +- ...pending_requests_reminder_to_reviewer.html | 12 ++-- .../pending_requests_summary_to_reviewer.html | 17 +++--- ...r_special_membership_request_accepted.html | 2 +- ...ip_request_approved_temporary_to_user.html | 57 +++++++++++++++---- ...hip_request_approved_temporary_to_user.txt | 2 +- ...l_membership_request_approved_to_user.html | 2 +- ...al_membership_request_created_to_user.html | 34 +++++++---- ...p_request_created_to_user_cc_reviewer.html | 10 ++-- .../test_createspecialmembershiprequest.py | 7 ++- .../test_userSpecialMembershipRequest.py | 10 +++- 14 files changed, 122 insertions(+), 53 deletions(-) diff --git a/impresso/tasks/userSpecialMembershipRequest_tasks.py b/impresso/tasks/userSpecialMembershipRequest_tasks.py index 7ab06e2..c425617 100644 --- a/impresso/tasks/userSpecialMembershipRequest_tasks.py +++ b/impresso/tasks/userSpecialMembershipRequest_tasks.py @@ -104,7 +104,20 @@ def after_special_membership_request_created(self, instance_id: int) -> None: # sends the temporary-approval email, and schedules revocation. return + # Apply special membership to bitmap before sending emails, so that the user gets the access as soon as they receive the email apply_special_membership_to_bitmap(instance=req, created=True, logger=logger) + + if req.status != UserSpecialMembershipRequest.STATUS_PENDING: + logger.info( + f"[instance:{instance_id}] created with status {req.status}, skipping created email and sending update email instead" + ) + send_email_after_user_special_membership_request_updated( + instance=req, + logger=logger, + is_modality_cc_reviewer_enabled=_is_modality_cc_reviewer_enabled(req), + ) + return + send_email_after_user_special_membership_request_created( instance=req, logger=logger ) diff --git a/impresso/templates/emails/account_created_mailto_educational.html b/impresso/templates/emails/account_created_mailto_educational.html index 88f7487..8af230b 100644 --- a/impresso/templates/emails/account_created_mailto_educational.html +++ b/impresso/templates/emails/account_created_mailto_educational.html @@ -36,7 +36,8 @@

    We hope you will enjoy working with Impresso and do let us know how you get on. To reach us, you can reply to this email, or join the Impresso community - Discord group + Discord group or click the + feedback button in the lower right of the interface.

    diff --git a/impresso/templates/emails/account_created_mailto_researcher.html b/impresso/templates/emails/account_created_mailto_researcher.html index 88f7487..8af230b 100644 --- a/impresso/templates/emails/account_created_mailto_researcher.html +++ b/impresso/templates/emails/account_created_mailto_researcher.html @@ -36,7 +36,8 @@

    We hope you will enjoy working with Impresso and do let us know how you get on. To reach us, you can reply to this email, or join the Impresso community - Discord group + Discord group or click the + feedback button in the lower right of the interface.

    diff --git a/impresso/templates/emails/account_created_mailto_user.html b/impresso/templates/emails/account_created_mailto_user.html index 88f7487..8af230b 100644 --- a/impresso/templates/emails/account_created_mailto_user.html +++ b/impresso/templates/emails/account_created_mailto_user.html @@ -36,7 +36,8 @@

    We hope you will enjoy working with Impresso and do let us know how you get on. To reach us, you can reply to this email, or join the Impresso community - Discord group + Discord group or click the + feedback button in the lower right of the interface.

    diff --git a/impresso/templates/emails/pending_requests_reminder_to_reviewer.html b/impresso/templates/emails/pending_requests_reminder_to_reviewer.html index 5b1e86a..c844518 100644 --- a/impresso/templates/emails/pending_requests_reminder_to_reviewer.html +++ b/impresso/templates/emails/pending_requests_reminder_to_reviewer.html @@ -12,11 +12,11 @@

      {% for request in latest_requests %}
    • - Request from: {{ request.user.get_full_name|default:request.user.username}} + Request from: {{ request.user.get_full_name|default:request.user.username }}
        +
      • Email: {{ request.user.email }}
      • - Email: {{ request.user.email }}
        • - Affiliation: + Affiliation: {% if request.user.profile.affiliation %} {{ request.user.profile.affiliation }} {% else %} @@ -24,7 +24,7 @@ {% endif %}
      • - Public Institutional Webpage: + Public Institutional Webpage: {% if request.user.profile.institutional_url %} {{ request.user.profile.institutional_url }} {% else %} @@ -35,7 +35,7 @@ Membership Option: {{ request.subscription.title }}
      • - Request Date: {{ request.date_created|date:"l, d.m.Y, H:i T" }} + Submitted: {{ request.date_created|date:"F j, Y" }} ({{ request.days_waiting }} day{{ request.days_waiting|pluralize }} ago) @@ -67,6 +67,6 @@

        Thank you for your continued support!

        - Warm regards,
        + Best regards,
        The Impresso Team

        \ No newline at end of file diff --git a/impresso/templates/emails/pending_requests_summary_to_reviewer.html b/impresso/templates/emails/pending_requests_summary_to_reviewer.html index 9766409..ca2f488 100644 --- a/impresso/templates/emails/pending_requests_summary_to_reviewer.html +++ b/impresso/templates/emails/pending_requests_summary_to_reviewer.html @@ -18,11 +18,11 @@
          {% for request in latest_requests %}
        • - Request from: {{ request.user.get_full_name|default:request.user.username}} + Request from: {{ request.user.get_full_name|default:request.user.username }}
            +
          • Email: {{ request.user.email }}
          • - Email: {{ request.user.email }}
            • - Affiliation: + Affiliation: {% if request.user.profile.affiliation %} {{ request.user.profile.affiliation }} {% else %} @@ -30,7 +30,7 @@ {% endif %}
          • - Public Institutional Webpage: + Public Institutional Webpage: {% if request.user.profile.institutional_url %} {{ request.user.profile.institutional_url }} {% else %} @@ -41,14 +41,15 @@ Membership Option: {{ request.subscription.title }}
          • - Request Date: {{ request.date_created|date:"l, d.m.Y, H:i T" }} + Submitted: {{ request.date_created|date:"F j, Y" }} ({{ request.days_waiting }} day{{ request.days_waiting|pluralize }} ago)
            • - {% if request.notes %} -
          • Notes: {{ request.notes|truncatewords:20 }}
            • - {% endif %} +
          • + Notes: + {% if request.notes %}{{ request.notes|truncatewords:20 }}{% else %}Not provided{% endif %} +
          • {% endfor %} diff --git a/impresso/templates/emails/user_special_membership_request_accepted.html b/impresso/templates/emails/user_special_membership_request_accepted.html index 3fcd4c3..27f71e2 100644 --- a/impresso/templates/emails/user_special_membership_request_accepted.html +++ b/impresso/templates/emails/user_special_membership_request_accepted.html @@ -8,7 +8,7 @@ You now have full access to: {{ user_special_membership_request.subscription.title }}.

          -

          You can begin using your new features and data immediately. Please

          +

          You can begin using your new features and data immediately.

          If you have any questions about your new access, please don't hesitate to reach out. diff --git a/impresso/templates/emails/user_special_membership_request_approved_temporary_to_user.html b/impresso/templates/emails/user_special_membership_request_approved_temporary_to_user.html index 92d2499..487d5e8 100644 --- a/impresso/templates/emails/user_special_membership_request_approved_temporary_to_user.html +++ b/impresso/templates/emails/user_special_membership_request_approved_temporary_to_user.html @@ -1,23 +1,58 @@ +{% load tz %} + Dear {{ user.first_name }},

          - Great news: your request for special membership access has been - temporarily approved. -

          -

          - You now have access to: + We have received your request for Special Membership Access to {{ user_special_membership_request.subscription.title }}. + To allow you to begin your work immediately, your account has been granted + Provisional Access for a period of {{ user_special_membership_request_duration }} + until + {{ user_special_membership_request.temporary_expires_at|date:"l, Y-m-d H:i" }} + and gives you full access contents from Test Dataset. Note that Impresso Terms + of Use always apply.

          +

          - This temporary access is valid until - {{ user_special_membership_request.temporary_expires_at|date:"Y-m-d H:i" }}. - After that date, access will be automatically revoked unless a permanent approval is granted. + Next Steps & Extensions:
          + Access will be automatically revoked after + {{ user_special_membership_request_duration }} from now. For continued access, + please request "Standard Access" via the Impresso platform. Please note that + institutional approval for Standard Access can take some time. You may contact + the institutional reviewer directly by replying to this email.

          + +

          Here are the details of the access request for your reference:

          +
            +
          • Requester: {{ user.get_full_name|default:user.username }}
            • +
          • Requester Email: {{ user.email }}
            • +
          • Requester Plan: {{ plan_label }}
            • +
          • + Affiliation: + {% if user.profile.affiliation %}{{ user.profile.affiliation }}{% else %}Not provided{% endif %} +
            • +
          • + Public Institutional Webpage: + {% if user.profile.institutional_url %}{{ user.profile.institutional_url }}{% else %}Not provided{% endif %} +
            • +
          • Membership Option: {{ user_special_membership_request.subscription.title }}
            • +
          • + Request Date: + {{ user_special_membership_request.date_created|timezone:"UTC"|date:"l, d.m.Y, H:i T" }} +
            • +
          • + Expiration Date: + {{ user_special_membership_request.temporary_expires_at|timezone:"UTC"|date:"l, d.m.Y, H:i T" }} +
            • +
          • Request Status: {{ status_label }}
            • +
          +

          - If you have questions or need an extension, please reply to this email. + Should you need any further assistance from our side, the Impresso Team remains + at your service.

          +

          - Best regards, -
          + Warm regards,
          The Impresso Team

          diff --git a/impresso/templates/emails/user_special_membership_request_approved_temporary_to_user.txt b/impresso/templates/emails/user_special_membership_request_approved_temporary_to_user.txt index 74cadf9..da8333d 100644 --- a/impresso/templates/emails/user_special_membership_request_approved_temporary_to_user.txt +++ b/impresso/templates/emails/user_special_membership_request_approved_temporary_to_user.txt @@ -23,7 +23,7 @@ Here are the details of the access request for your reference: Membership Option: {{ user_special_membership_request.subscription.title }} Request Date: {{ user_special_membership_request.date_created|timezone:"UTC"|date:"l, d.m.Y, H:i T" }} Expiration Date: {{ user_special_membership_request.temporary_expires_at|timezone:"UTC"|date:"l, d.m.Y, H:i T" }} - Request Status: {{ user_special_membership_request.status}} + Request Status: {{ status_label }} Should you need any further assistance from our side, the Impresso Team remains at your service. diff --git a/impresso/templates/emails/user_special_membership_request_approved_to_user.html b/impresso/templates/emails/user_special_membership_request_approved_to_user.html index 3fcd4c3..27f71e2 100644 --- a/impresso/templates/emails/user_special_membership_request_approved_to_user.html +++ b/impresso/templates/emails/user_special_membership_request_approved_to_user.html @@ -8,7 +8,7 @@ You now have full access to: {{ user_special_membership_request.subscription.title }}.

          -

          You can begin using your new features and data immediately. Please

          +

          You can begin using your new features and data immediately.

          If you have any questions about your new access, please don't hesitate to reach out. diff --git a/impresso/templates/emails/user_special_membership_request_created_to_user.html b/impresso/templates/emails/user_special_membership_request_created_to_user.html index a30117c..82ebdbf 100644 --- a/impresso/templates/emails/user_special_membership_request_created_to_user.html +++ b/impresso/templates/emails/user_special_membership_request_created_to_user.html @@ -1,25 +1,35 @@ Dear {{ user.first_name }},

          - Thank you for requesting special membership access on Impresso. We've successfully received your request and our team is now reviewing it. + Thank you for requesting special membership access on Impresso. We've + successfully received your request and our team is now reviewing it.

          Here are the details of the access you requested, for your reference: -

            -
          • Membership Option: {{ user_special_membership_request.subscription.title }}
            • -
          • Request Submitted: {{ user_special_membership_request.date_created | date:"l, d.m.Y, H:i T" }}
            • -
          • Current Status: {{ status_label }}
            • -

          +
            +
          • Membership Option: {{ user_special_membership_request.subscription.title }}
            • +
          • Request Date: {{ user_special_membership_request.date_created | date:"l, d.m.Y, H:i T" }}
            • +
          • Current Status: {{ status_label }}
            • +
          • + Notes: + {% if user_special_membership_request.notes %} + {{ user_special_membership_request.notes }} + {% else %} + No notes provided. + {% endif %} +
            • +

          - We aim to complete the review as quickly as possible and will notify you as soon as a decision has been made. -
          - Please check your inbox regularly, as the reviewer may reach out to you via email (e.g., to send a contract for signature). + We aim to complete the review as quickly as possible and will notify you as + soon as a decision has been made. Please check your inbox regularly, as the + reviewer may reach out to you via email (e.g., to send a contract for + signature).

          We appreciate your patience! -
          - Best regards, -
          +

          +

          + Best regards,
          The Impresso Team

          \ No newline at end of file diff --git a/impresso/templates/emails/user_special_membership_request_created_to_user_cc_reviewer.html b/impresso/templates/emails/user_special_membership_request_created_to_user_cc_reviewer.html index d73a49d..998d18d 100644 --- a/impresso/templates/emails/user_special_membership_request_created_to_user_cc_reviewer.html +++ b/impresso/templates/emails/user_special_membership_request_created_to_user_cc_reviewer.html @@ -28,7 +28,7 @@ {% if user.profile.affiliation %} {{ user.profile.affiliation }} {% else %} - - not provided + Not provided {% endif %}
        • @@ -36,7 +36,7 @@ {% if user.profile.institutional_url %} {{ user.profile.institutional_url }} {% else %} - - not provided + Not provided {% endif %}
        • @@ -51,11 +51,9 @@
        • Notes: {% if user_special_membership_request.notes %} -
          - {{ user_special_membership_request.notes }} -
          + {{ user_special_membership_request.notes }} {% else %} - - not provided + No notes provided. {% endif %}
        diff --git a/impresso/tests/management/commands/test_createspecialmembershiprequest.py b/impresso/tests/management/commands/test_createspecialmembershiprequest.py index 642242a..3ee6e38 100644 --- a/impresso/tests/management/commands/test_createspecialmembershiprequest.py +++ b/impresso/tests/management/commands/test_createspecialmembershiprequest.py @@ -176,12 +176,15 @@ def test_add_notes_and_approved_status(self) -> None: "This is an approved request with notes.", stdout=out, ) - print(len(mail.outbox)) - print(mail.outbox[1].body) + self.assertEqual(len(mail.outbox), 1, "Expected only one email to be sent when creating an APPROVED request") + self.assertEqual(mail.outbox[0].to, [self.user.email]) + self.assertIn("approved", mail.outbox[0].subject.lower()) + request = UserSpecialMembershipRequest.objects.get( user=self.user, subscription=self.dataset, ) + self.assertEqual(request.status, UserSpecialMembershipRequest.STATUS_APPROVED) self.assertEqual(request.reviewer, self.reviewer) self.assertIn( diff --git a/impresso/tests/utils/tasks/test_userSpecialMembershipRequest.py b/impresso/tests/utils/tasks/test_userSpecialMembershipRequest.py index 31fe4ed..e31278d 100644 --- a/impresso/tests/utils/tasks/test_userSpecialMembershipRequest.py +++ b/impresso/tests/utils/tasks/test_userSpecialMembershipRequest.py @@ -97,7 +97,10 @@ def test_created_sends_email_to_user_and_reviewer(self): email.body, "Email body should contain the user's full name.", ) - self.assertIn("Pending Review", email.body) + status_label = dict(UserSpecialMembershipRequest.STATUS_CHOICES).get( + instance.status, instance.status + ) + self.assertIn(status_label, email.body) self.assertIn("Test Dataset", email.body) def test_created_based_on_metadata_modality(self): @@ -251,7 +254,10 @@ def test_created_sends_email_to_user_and_reviewer(self): settings.IMPRESSO_EMAIL_SUBJECT_AFTER_USER_SPECIAL_MEMBERSHIP_REQUEST_CREATED_TO_USER, ) self.assertIn("Dear Alice,", user_email.body) - self.assertIn("Pending Review", user_email.body) + status_label = dict(UserSpecialMembershipRequest.STATUS_CHOICES).get( + instance.status, instance.status + ) + self.assertIn(status_label, user_email.body) self.assertIn("Test Dataset", user_email.body) self.assertIn("Please review my request.", user_email.body) From 7aac2363bad0d120816c36d97323660c3ce68711 Mon Sep 17 00:00:00 2001 From: Daniele Guido Date: Wed, 6 May 2026 11:51:18 +0200 Subject: [PATCH 15/30] fix tests --- .../createspecialmembershiprequest.py | 17 +++ impresso/settings.py | 3 + .../userSpecialMembershipRequest_tasks.py | 53 ++++--- .../test_checktemporarymemberships.py | 49 +++++-- .../test_createspecialmembershiprequest.py | 135 ++++++++++++++++-- .../test_userSpecialMembershipRequest.py | 10 +- 6 files changed, 215 insertions(+), 52 deletions(-) diff --git a/impresso/management/commands/createspecialmembershiprequest.py b/impresso/management/commands/createspecialmembershiprequest.py index a50fb7f..ad3b08f 100644 --- a/impresso/management/commands/createspecialmembershiprequest.py +++ b/impresso/management/commands/createspecialmembershiprequest.py @@ -1,8 +1,10 @@ +from datetime import timedelta from typing import Any from django.contrib.auth.models import User from django.core.management.base import BaseCommand, CommandError from django.db import IntegrityError +from django.utils import timezone from impresso.models import SpecialMembershipDataset, UserSpecialMembershipRequest @@ -46,6 +48,13 @@ def add_arguments(self, parser) -> None: default=None, help="Optional notes to attach to the request", ) + parser.add_argument( + "--revoke-after", + type=float, + default=None, + dest="revoke_after", + help="Number of days after which the temporary membership expires (sets temporary_expires_at and overrides the default value from subscrption metadata)", + ) def handle( self, user_email: str, dataset_id: int, *args: Any, **options: Any @@ -66,6 +75,13 @@ def handle( f"SpecialMembershipDataset with id={dataset_id} does not exist." ) from exc + revoke_after: float | None = options["revoke_after"] + temporary_expires_at = ( + timezone.now() + timedelta(days=revoke_after) + if revoke_after is not None + else None + ) + try: request = UserSpecialMembershipRequest.objects.create( user=user, @@ -73,6 +89,7 @@ def handle( subscription=dataset, status=options["status"], notes=options["notes"], + temporary_expires_at=temporary_expires_at, ) except IntegrityError as exc: raise CommandError( diff --git a/impresso/settings.py b/impresso/settings.py index 2865f29..5f65b8b 100644 --- a/impresso/settings.py +++ b/impresso/settings.py @@ -174,6 +174,7 @@ ) from celery.schedules import crontab + CELERY_BEAT_SCHEDULE = { "revoke-expired-temporary-memberships-every-hour": { "task": "impresso.tasks.userSpecialMembershipRequest_tasks.revoke_expired_temporary_memberships_beat", @@ -391,6 +392,8 @@ (IMPRESSO_GROUP_USER_PLAN_EDUCATIONAL, IMPRESSO_GROUP_USER_PLAN_EDUCATIONAL_LABEL), (IMPRESSO_GROUP_USER_PLAN_RESEARCHER, IMPRESSO_GROUP_USER_PLAN_RESEARCHER_LABEL), ) + +IMPRESSO_SPECIAL_MEMBERSHIP_TEMPORARY_APPROVAL_DEFAULT_DAYS = 1 IMPRESSO_EMAIL_LABEL_DEFAULT_FROM_EMAIL = f"Impresso Team <{DEFAULT_FROM_EMAIL}>" IMPRESSO_EMAIL_SUBJECT_AFTER_USER_REGISTRATION_PLAN_BASIC = "Access to Impresso" diff --git a/impresso/tasks/userSpecialMembershipRequest_tasks.py b/impresso/tasks/userSpecialMembershipRequest_tasks.py index c425617..f8ec361 100644 --- a/impresso/tasks/userSpecialMembershipRequest_tasks.py +++ b/impresso/tasks/userSpecialMembershipRequest_tasks.py @@ -29,23 +29,37 @@ def _is_modality_cc_reviewer_enabled(req: UserSpecialMembershipRequest) -> bool: ) -def _get_revoke_after_days(req: UserSpecialMembershipRequest) -> float | None: +def _get_revoke_after_days(req: UserSpecialMembershipRequest) -> float: + """ + Always return a positive number of days from the request's subscription metadata. + If they're wrong or missing, get the value from the default settings.IMPRESSO_SPECIAL_MEMBERSHIP_TEMPORARY_APPROVAL_DEFAULT_DAYS + """ + req.refresh_from_db() # ensure we have the latest subscription metadata + DEFAULT_DAYS: float = ( + settings.IMPRESSO_SPECIAL_MEMBERSHIP_TEMPORARY_APPROVAL_DEFAULT_DAYS + ) if not req.subscription: - return None + logger.warning( + f"[instance:{req.pk}] No subscription associated with the request, cannot get revokeAfterDays, using default {DEFAULT_DAYS} day(s)" + ) + return DEFAULT_DAYS + revoke_after_days = req.subscription.metadata.get("revokeAfterDays") if isinstance(revoke_after_days, (int, float)) and revoke_after_days > 0: return float(revoke_after_days) - return None + + logger.warning( + f"[instance:{req.pk}] revokeAfterDays is missing or invalid in subscription metadata (value={revoke_after_days}), using default {DEFAULT_DAYS} day(s)" + ) + return DEFAULT_DAYS def _schedule_temporary_revocation(req: UserSpecialMembershipRequest) -> None: revoke_after_days = _get_revoke_after_days(req) - if revoke_after_days is None: - logger.warning( - f"[instance:{req.pk}] STATUS_APPROVED_TEMPORARY but revokeAfterDays is missing/invalid, skipping scheduling" - ) - return - revoke_at = timezone.now() + timedelta(days=revoke_after_days) + if req.temporary_expires_at is not None: + revoke_at = req.temporary_expires_at + else: + revoke_at = timezone.now() + timedelta(days=revoke_after_days) revoke_special_membership_request.apply_async( kwargs={"instance_id": req.pk}, eta=revoke_at ) @@ -111,13 +125,9 @@ def after_special_membership_request_created(self, instance_id: int) -> None: logger.info( f"[instance:{instance_id}] created with status {req.status}, skipping created email and sending update email instead" ) - send_email_after_user_special_membership_request_updated( - instance=req, - logger=logger, - is_modality_cc_reviewer_enabled=_is_modality_cc_reviewer_enabled(req), - ) + after_special_membership_request_updated.delay(instance_id=instance_id) return - + send_email_after_user_special_membership_request_created( instance=req, logger=logger ) @@ -166,8 +176,10 @@ def after_special_membership_request_updated(self, instance_id: int) -> None: logger=logger, is_modality_cc_reviewer_enabled=_is_modality_cc_reviewer_enabled(req), ) - - if req.status == UserSpecialMembershipRequest.STATUS_APPROVED_TEMPORARY: + if ( + req.status == UserSpecialMembershipRequest.STATUS_APPROVED_TEMPORARY + or req.temporary_expires_at is not None + ): logger.info( f"[instance:{instance_id}] STATUS_APPROVED_TEMPORARY, scheduling revocation..." ) @@ -205,15 +217,10 @@ def revoke_special_membership_request(self, instance_id: int) -> None: req.status = UserSpecialMembershipRequest.STATUS_REVOKED req.save() + # usual signals will take care of applying the bitmap changes and sending the email notification about revocation logger.info( f"[instance:{instance_id}] temporary membership revoked for user={req.user.pk}" ) - apply_special_membership_to_bitmap(instance=req, created=False, logger=logger) - send_email_after_user_special_membership_request_updated( - instance=req, - logger=logger, - is_modality_cc_reviewer_enabled=_is_modality_cc_reviewer_enabled(req), - ) @app.task( diff --git a/impresso/tests/management/commands/test_checktemporarymemberships.py b/impresso/tests/management/commands/test_checktemporarymemberships.py index a3ceb24..950de2b 100644 --- a/impresso/tests/management/commands/test_checktemporarymemberships.py +++ b/impresso/tests/management/commands/test_checktemporarymemberships.py @@ -1,6 +1,6 @@ from datetime import timedelta from io import StringIO - +from django.core import mail from django.contrib.auth.models import User from django.core.management import call_command from django.test import TestCase @@ -9,9 +9,13 @@ from impresso.models import SpecialMembershipDataset, UserSpecialMembershipRequest from impresso.signals import create_default_groups +from unittest.mock import patch + class TestCheckTemporaryMembershipsCommand(TestCase): - """Test the checktemporarymemberships management command.""" + """Test the checktemporarymemberships management command. + ENV=test pipenv run python manage.py test impresso.tests.management.commands.test_checktemporarymemberships.TestCheckTemporaryMembershipsCommand + """ def setUp(self) -> None: create_default_groups(sender="impresso") @@ -38,18 +42,30 @@ def setUp(self) -> None: def test_finds_temporary_memberships(self) -> None: # Create one pending, one temporary approved + # pending request should not be included in the output, only the temporary approved one UserSpecialMembershipRequest.objects.create( user=self.user1, subscription=self.dataset, status=UserSpecialMembershipRequest.STATUS_PENDING, ) - - temp_req = UserSpecialMembershipRequest.objects.create( - user=self.user2, - subscription=self.dataset, - status=UserSpecialMembershipRequest.STATUS_APPROVED_TEMPORARY, - temporary_expires_at=timezone.now() + timedelta(days=7), - ) + self.assertEqual( + len(mail.outbox), + 1, + "Always expect an email to be sent when creating a request", + ) # Debug assertion to check mail outbox length + mail.outbox = [] + # Clear the outbox before creating the temporary approved request + + with patch( + "impresso.tasks.userSpecialMembershipRequest_tasks.revoke_special_membership_request.apply_async" + ) as mock_apply_async: + temp_req = UserSpecialMembershipRequest.objects.create( + user=self.user2, + subscription=self.dataset, + status=UserSpecialMembershipRequest.STATUS_APPROVED_TEMPORARY, + temporary_expires_at=timezone.now() + timedelta(days=7), + ) + self.assertEqual(len(mail.outbox), 1) out = StringIO() call_command("checktemporarymemberships", stdout=out) @@ -63,12 +79,15 @@ def test_finds_temporary_memberships(self) -> None: def test_shows_expired_memberships(self) -> None: # Create a temporary approved request that has already expired - temp_req = UserSpecialMembershipRequest.objects.create( - user=self.user1, - subscription=self.dataset, - status=UserSpecialMembershipRequest.STATUS_APPROVED_TEMPORARY, - temporary_expires_at=timezone.now() - timedelta(days=1), - ) + with patch( + "impresso.tasks.userSpecialMembershipRequest_tasks.revoke_special_membership_request.apply_async" + ): + temp_req = UserSpecialMembershipRequest.objects.create( + user=self.user1, + subscription=self.dataset, + status=UserSpecialMembershipRequest.STATUS_APPROVED_TEMPORARY, + temporary_expires_at=timezone.now() - timedelta(days=1), + ) out = StringIO() call_command("checktemporarymemberships", stdout=out) diff --git a/impresso/tests/management/commands/test_createspecialmembershiprequest.py b/impresso/tests/management/commands/test_createspecialmembershiprequest.py index 3ee6e38..2267958 100644 --- a/impresso/tests/management/commands/test_createspecialmembershiprequest.py +++ b/impresso/tests/management/commands/test_createspecialmembershiprequest.py @@ -1,8 +1,10 @@ +from datetime import timedelta from io import StringIO from django.core import mail from django.contrib.auth.models import User from django.core.management import CommandError, call_command from django.test import TestCase +from django.utils import timezone from impresso.models import SpecialMembershipDataset, UserSpecialMembershipRequest from impresso.signals import create_default_groups @@ -54,14 +56,10 @@ def test_creates_request_with_1_day_revokeable_period(self) -> None: str(dataset_with_revokeable_period.pk), stdout=out, ) - print(mail.outbox[0].body) request = UserSpecialMembershipRequest.objects.get( user=self.user, subscription=dataset_with_revokeable_period, ) - print("olalalla this terminates here", request.date_created, request.status) - print(mail.outbox[1].body) - print(out.getvalue()) # date dataset_with_revokeable_period should sset and be more or less 24 h from now self.assertIsNotNone(request.temporary_expires_at) now = request.date_created @@ -176,15 +174,19 @@ def test_add_notes_and_approved_status(self) -> None: "This is an approved request with notes.", stdout=out, ) - self.assertEqual(len(mail.outbox), 1, "Expected only one email to be sent when creating an APPROVED request") + self.assertEqual( + len(mail.outbox), + 1, + "Expected only one email to be sent when creating an APPROVED request", + ) self.assertEqual(mail.outbox[0].to, [self.user.email]) self.assertIn("approved", mail.outbox[0].subject.lower()) - + request = UserSpecialMembershipRequest.objects.get( user=self.user, subscription=self.dataset, ) - + self.assertEqual(request.status, UserSpecialMembershipRequest.STATUS_APPROVED) self.assertEqual(request.reviewer, self.reviewer) self.assertIn( @@ -192,7 +194,120 @@ def test_add_notes_and_approved_status(self) -> None: out.getvalue(), ) + def test_add_notes_and_approved_temporary_status(self) -> None: + out = StringIO() + mail.outbox = [] + call_command( + "createspecialmembershiprequest", + self.user.email, + str(self.dataset.pk), + "--status", + UserSpecialMembershipRequest.STATUS_APPROVED, + "--notes", + "This is an approved request with notes.", + stdout=out, + ) + # check user_bitmap.subscriptions + + # with patch( + # "impresso.tasks.userSpecialMembershipRequest_tasks.revoke_special_membership_request.apply_async" + # ) as mock_apply_async: + def test_create_pending_when_temporary_automatic_acceptance_is_enabled( + self, + ) -> None: + revokable_dataset = SpecialMembershipDataset.objects.create( + title="Revokable Dataset", + reviewer=self.reviewer, + metadata={ + "revokeAfterDays": 5, + "enableTemporaryAutomaticAcceptance": True, + }, + ) + out = StringIO() + mail.outbox = [] + with patch( + "impresso.tasks.userSpecialMembershipRequest_tasks.revoke_special_membership_request.apply_async" + ) as mock_apply_async: + call_command( + "createspecialmembershiprequest", + self.user.email, + str(revokable_dataset.pk), + stdout=out, + ) + request = UserSpecialMembershipRequest.objects.get( + user=self.user, + subscription=revokable_dataset, + ) + self.assertEqual( + request.status, UserSpecialMembershipRequest.STATUS_APPROVED_TEMPORARY + ) + # check that the revoke task was scheduled with the correct eta (approximately now + 5 days) + self.assertTrue( + request.temporary_expires_at is not None, + "temporary_expires_at should be set for temporary approved requests", + ) + self.assertEqual( + request.temporary_expires_at.date(), + ( + timezone.now() + + timedelta(days=revokable_dataset.metadata["revokeAfterDays"]) + ).date(), + "temporary_expires_at should be approximately now + 5 days", + ) + + self.assertTrue( + mock_apply_async.called, + "Expected revoke_special_membership_request.apply_async to be called for temporary approval", + ) + _args, kwargs = mock_apply_async.call_args + self.assertIn("eta", kwargs, "Expected 'eta' argument in apply_async call") + + def test_revoke_after_half_day_sets_temporary_expires_at(self) -> None: + """ + --revoke-after 0.5 should set temporary_expires_at to approximately now + 12 hours. + """ + out = StringIO() + before = timezone.now() + call_command( + "createspecialmembershiprequest", + self.user.email, + str(self.dataset.pk), + "--revoke-after", + "0.5", + "--status", + UserSpecialMembershipRequest.STATUS_APPROVED_TEMPORARY, + stdout=out, + ) + after = timezone.now() + + request = UserSpecialMembershipRequest.objects.get( + user=self.user, + subscription=self.dataset, + ) + self.assertIsNotNone(request.temporary_expires_at) -# with patch( -# "impresso.tasks.userSpecialMembershipRequest_tasks.revoke_special_membership_request.apply_async" -# ) as mock_apply_async: + expected_lower = before + timedelta(days=0.5) + expected_upper = after + timedelta(days=0.5) + + self.assertGreaterEqual( + request.temporary_expires_at, + expected_lower, + "temporary_expires_at should be at least now + 0.5 days", + ) + self.assertLessEqual( + request.temporary_expires_at, + expected_upper, + "temporary_expires_at should be at most now + 0.5 days", + ) + self.assertIn("Created special membership request", out.getvalue()) + self.assertIn("Access will be automatically revoked after", mail.outbox[0].body) + # second email is revoke (in test, all celery taks are executed synchronously, so the revoke email is sent immediately after the creation email) + self.assertEqual( + len(mail.outbox), + 2, + "Expected two emails to be sent during testing without patch: one for creation and one for revoke", + ) + self.assertIn( + "Your temporary special membership access has expired and is now revoked", + mail.outbox[1].body, + ) diff --git a/impresso/tests/utils/tasks/test_userSpecialMembershipRequest.py b/impresso/tests/utils/tasks/test_userSpecialMembershipRequest.py index e31278d..ed6fdbf 100644 --- a/impresso/tests/utils/tasks/test_userSpecialMembershipRequest.py +++ b/impresso/tests/utils/tasks/test_userSpecialMembershipRequest.py @@ -75,7 +75,9 @@ def test_created_sends_email_to_user_and_reviewer(self): # Set pk and dates manually to avoid triggering signals instance.pk = 1 instance.date_created = instance.date_last_modified = timezone.now() - + self.assertEqual( + len(mail.outbox), 0, "No emails should be sent before the task is called" + ) send_email_after_user_special_membership_request_created( instance=instance, logger=logger, @@ -154,15 +156,15 @@ def test_created_with_already_existing_subscriptions(self): subscription=dataset_existing, status=UserSpecialMembershipRequest.STATUS_APPROVED, ) - + # reload from db + self.user.refresh_from_db() self.assertEqual( self.user.bitmap.subscriptions.count(), 1, "User should have 1 existing special membership subscription as it is already approved.", ) - self.assertIn( - "Membership Option: Existing Dataset", + "You now have full access to: Existing Dataset.", mail.outbox[0].body, "Email context should include the correct number of existing special memberships.", ) From 376bf05fd912152d79ee200f734ffe3a671b3de1 Mon Sep 17 00:00:00 2001 From: Daniele Guido Date: Wed, 6 May 2026 13:01:09 +0200 Subject: [PATCH 16/30] Update test_userSpecialMembershipRequest.py --- .../tasks/test_userSpecialMembershipRequest.py | 18 ++++++++++-------- 1 file changed, 10 insertions(+), 8 deletions(-) diff --git a/impresso/tests/utils/tasks/test_userSpecialMembershipRequest.py b/impresso/tests/utils/tasks/test_userSpecialMembershipRequest.py index ed6fdbf..bb8546c 100644 --- a/impresso/tests/utils/tasks/test_userSpecialMembershipRequest.py +++ b/impresso/tests/utils/tasks/test_userSpecialMembershipRequest.py @@ -9,9 +9,7 @@ from impresso.models import SpecialMembershipDataset, UserSpecialMembershipRequest from impresso.models.profile import Profile -from impresso.models.userBitmap import UserBitmap from impresso.tasks.userSpecialMembershipRequest_tasks import ( - after_special_membership_request_created, revoke_special_membership_request, ) from impresso.utils.tasks.userSpecialMembershipRequest import ( @@ -475,6 +473,10 @@ def test_created_requests_with_float_days_for_revoke_after_days(self): class TestTemporaryAutomaticRevocation(TestCase): + """ + ENV=test pipenv run ./manage.py test impresso.tests.utils.tasks.test_userSpecialMembershipRequest.TestTemporaryAutomaticRevocation + """ + def setUp(self): self.user = User.objects.create_user( username="revoked-user", @@ -528,16 +530,16 @@ def test_revoke_expired_temporary_memberships_beat(self): user2 = User.objects.create_user( username="other-user", email="other-user@example.com" ) - UserSpecialMembershipRequest.objects.create( - user=user2, - subscription=self.dataset, - status=UserSpecialMembershipRequest.STATUS_APPROVED_TEMPORARY, - temporary_expires_at=timezone.now() + timedelta(days=1), - ) with patch( "impresso.tasks.userSpecialMembershipRequest_tasks.revoke_special_membership_request.delay" ) as mock_delay: + UserSpecialMembershipRequest.objects.create( + user=user2, + subscription=self.dataset, + status=UserSpecialMembershipRequest.STATUS_APPROVED_TEMPORARY, + temporary_expires_at=timezone.now() + timedelta(days=1), + ) revoke_expired_temporary_memberships_beat.delay() mock_delay.assert_called_once_with(instance_id=req1.pk) From c8e638d827328a62e7a005d6f21ea78fa90ff8fe Mon Sep 17 00:00:00 2001 From: Daniele Guido <1181642+danieleguido@users.noreply.github.com> Date: Wed, 6 May 2026 17:05:30 +0200 Subject: [PATCH 17/30] Update test_userSpecialMembershipRequest.py --- .../test_userSpecialMembershipRequest.py | 89 ++++++++++++------- 1 file changed, 59 insertions(+), 30 deletions(-) diff --git a/impresso/tests/utils/tasks/test_userSpecialMembershipRequest.py b/impresso/tests/utils/tasks/test_userSpecialMembershipRequest.py index bb8546c..52e5b78 100644 --- a/impresso/tests/utils/tasks/test_userSpecialMembershipRequest.py +++ b/impresso/tests/utils/tasks/test_userSpecialMembershipRequest.py @@ -1,5 +1,6 @@ import logging from datetime import timedelta +from unittest.mock import patch from django.conf import settings from django.test import TestCase, TransactionTestCase @@ -11,6 +12,7 @@ from impresso.models.profile import Profile from impresso.tasks.userSpecialMembershipRequest_tasks import ( revoke_special_membership_request, + revoke_expired_temporary_memberships_beat, ) from impresso.utils.tasks.userSpecialMembershipRequest import ( send_email_after_user_special_membership_request_created, @@ -491,55 +493,82 @@ def setUp(self): mail.outbox = [] def test_expired_temporary_request_is_revoked(self): - req = UserSpecialMembershipRequest.objects.create( - user=self.user, - subscription=self.dataset, - status=UserSpecialMembershipRequest.STATUS_APPROVED_TEMPORARY, - temporary_expires_at=timezone.now() - timedelta(days=1), - ) - self.user.bitmap.subscriptions.add(self.dataset) - mail.outbox = [] + with patch( + "impresso.tasks.userSpecialMembershipRequest_tasks.revoke_special_membership_request.delay" + ) as mock_delay: + req = UserSpecialMembershipRequest.objects.create( + user=self.user, + subscription=self.dataset, + status=UserSpecialMembershipRequest.STATUS_APPROVED_TEMPORARY, + temporary_expires_at=timezone.now() - timedelta(days=1), + ) + + self.assertEqual( + req.status, + UserSpecialMembershipRequest.STATUS_APPROVED_TEMPORARY, + ) revoke_special_membership_request.delay(instance_id=req.pk) req.refresh_from_db() self.assertEqual(req.status, UserSpecialMembershipRequest.STATUS_REVOKED) + self.user.refresh_from_db() self.assertEqual(self.user.bitmap.subscriptions.count(), 0) self.assertEqual(len(mail.outbox), 2) - self.assertTrue( - all( - email.subject - == settings.IMPRESSO_EMAIL_SUBJECT_AFTER_USER_SPECIAL_MEMBERSHIP_REQUEST_REVOKED_TO_USER - for email in mail.outbox - ) - ) + # self.assertTrue( + # all( + # email.subject + # == settings.IMPRESSO_EMAIL_SUBJECT_AFTER_USER_SPECIAL_MEMBERSHIP_REQUEST_REVOKED_TO_USER + # for email in mail.outbox + # ) + # ) def test_revoke_expired_temporary_memberships_beat(self): - from unittest.mock import patch - from impresso.tasks.userSpecialMembershipRequest_tasks import ( - revoke_expired_temporary_memberships_beat, - ) - - req1 = UserSpecialMembershipRequest.objects.create( - user=self.user, - subscription=self.dataset, - status=UserSpecialMembershipRequest.STATUS_APPROVED_TEMPORARY, - temporary_expires_at=timezone.now() - timedelta(days=1), - ) user2 = User.objects.create_user( username="other-user", email="other-user@example.com" ) with patch( - "impresso.tasks.userSpecialMembershipRequest_tasks.revoke_special_membership_request.delay" + "impresso.tasks.userSpecialMembershipRequest_tasks.revoke_special_membership_request.apply_async" ) as mock_delay: - UserSpecialMembershipRequest.objects.create( + + request_that_need_to_be_revoked = ( + UserSpecialMembershipRequest.objects.create( + user=self.user, + subscription=self.dataset, + status=UserSpecialMembershipRequest.STATUS_APPROVED_TEMPORARY, + temporary_expires_at=timezone.now() - timedelta(days=1), + ) + ) + + req2 = UserSpecialMembershipRequest.objects.create( user=user2, subscription=self.dataset, status=UserSpecialMembershipRequest.STATUS_APPROVED_TEMPORARY, temporary_expires_at=timezone.now() + timedelta(days=1), ) - revoke_expired_temporary_memberships_beat.delay() + all_subscriptions = UserSpecialMembershipRequest.objects.filter( + status=UserSpecialMembershipRequest.STATUS_APPROVED_TEMPORARY + ) + # they are still "acive" + self.assertEqual(all_subscriptions.count(), 2) + self.assertEqual( + ",".join(all_subscriptions.values_list("status", flat=True)), + "temporary,temporary", + "both requests should not be expired yet", + ) + print("statuseeeeeeees", self.user.bitmap.subscriptions.count()) + print("statuseeeeeeees", user2.bitmap.subscriptions.count()) + active_subscriptions = UserSpecialMembershipRequest.objects.filter( + status=UserSpecialMembershipRequest.STATUS_APPROVED_TEMPORARY, + temporary_expires_at__gt=timezone.now(), + ) + self.assertEqual(active_subscriptions.count(), 1) - mock_delay.assert_called_once_with(instance_id=req1.pk) + print("DODODODODODODODODD", active_subscriptions.count()) + print( + "statuseeeeeeees", active_subscriptions.values_list("status", flat=True) + ) + + revoke_expired_temporary_memberships_beat.delay() From 26e3596bdcd43eeb33353a92a54c7c87caada77f Mon Sep 17 00:00:00 2001 From: Daniele Guido <1181642+danieleguido@users.noreply.github.com> Date: Thu, 7 May 2026 11:15:28 +0200 Subject: [PATCH 18/30] Make temporary revocations beat-driven Switch temporary special-membership revocation to be driven by a periodic celery beat job instead of scheduling per-request apply_async ETAs. Update AGENTS.md to document the new strategy. Persist temporary_expires_at in changelog entries and add a dedicated changelog append routine that avoids duplicate consecutive entries; add calculate_temporary_expiration helper. Remove inline scheduling logic from tasks and stop scheduling revocation on save/update; after-creation flow now sets STATUS_APPROVED_TEMPORARY and temporary_expires_at and relies on beat for revocation. Update tests to assert persisted state and email behavior (no apply_async ETA assertions) and adjust the management command comment accordingly. --- AGENTS.md | 14 +++ .../createspecialmembershiprequest.py | 1 + .../models/userSpecialMembershipRequest.py | 55 ++++++++--- .../userSpecialMembershipRequest_tasks.py | 53 +++-------- .../test_createspecialmembershiprequest.py | 76 ++++++---------- ...voke_expired_temporary_memberships_beat.py | 65 +++++++++++++ .../test_userSpecialMembershipRequest.py | 91 +++++++++---------- 7 files changed, 209 insertions(+), 146 deletions(-) create mode 100644 impresso/tests/tasks/test_revoke_expired_temporary_memberships_beat.py diff --git a/AGENTS.md b/AGENTS.md index be7ace1..d71d00d 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -65,6 +65,20 @@ with patch( assert "eta" in call_kwargs ``` +### Temporary special membership revocation strategy + +Temporary special membership revocation is beat-driven in this repository: + +- Requests are marked with `temporary_expires_at` when temporarily approved. +- Periodic task `revoke_expired_temporary_memberships_beat` is the single revocation trigger. +- Save/update flows should not enqueue direct revocation scheduling for temporary requests. + +Testing implications for this feature: + +- Do not patch `revoke_special_membership_request.apply_async` to assert ETA scheduling in temporary auto-accept tests. +- Prefer asserting persisted state (`status`, `temporary_expires_at`) and beat-driven revocation behavior. +- Keep generic `apply_async(..., eta=...)` assertions only for features that still use explicit ETA scheduling. + ## Email and templates ### Overview diff --git a/impresso/management/commands/createspecialmembershiprequest.py b/impresso/management/commands/createspecialmembershiprequest.py index ad3b08f..cf07c79 100644 --- a/impresso/management/commands/createspecialmembershiprequest.py +++ b/impresso/management/commands/createspecialmembershiprequest.py @@ -76,6 +76,7 @@ def handle( ) from exc revoke_after: float | None = options["revoke_after"] + # normally this would be set from request.creation_date + revoke_after, but since we're creating the request now we can set it from now + revoke_after :) temporary_expires_at = ( timezone.now() + timedelta(days=revoke_after) if revoke_after is not None diff --git a/impresso/models/userSpecialMembershipRequest.py b/impresso/models/userSpecialMembershipRequest.py index 602a2b0..910d2d7 100644 --- a/impresso/models/userSpecialMembershipRequest.py +++ b/impresso/models/userSpecialMembershipRequest.py @@ -18,6 +18,7 @@ class ChangelogEntry(TypedDict): date: str # ISO formatted date string reviewer: Optional[str] # Username of the reviewer notes: str # Additional notes (guaranteed to be a string, not None) + temporary_expires_at: Optional[str] # ISO formatted date string or None class UserSpecialMembershipRequest(models.Model): @@ -102,19 +103,51 @@ class Meta: verbose_name_plural = "User Special Membership Requests" db_table = "impresso_userrequest" - def save(self, *args: Any, **kwargs: Any) -> None: - changelog_entry: ChangelogEntry = { + def _append_changelog(self) -> None: + entry: ChangelogEntry = { "status": self.status, "subscription": self.subscription.title if self.subscription else None, - "date": ( - timezone.now().isoformat() - if not self.pk - else self.date_last_modified.isoformat() - ), + "date": timezone.now().isoformat(), "reviewer": self.reviewer.username if self.reviewer else None, - "notes": self.notes if self.notes else "", + "notes": self.notes or "", + "temporary_expires_at": ( + self.temporary_expires_at.isoformat() + if self.temporary_expires_at + else None + ), } - current_changelog: List[ChangelogEntry] = self.changelog or [] - current_changelog.append(changelog_entry) - self.changelog = current_changelog # Assign the updated list back + current_changelog = self.changelog or [] + latest_entry = current_changelog[-1] if current_changelog else None + # Avoid duplicate consecutive entries + if latest_entry: + comparable_fields = [ + "status", + "reviewer", + "notes", + "temporary_expires_at", + "subscription", + ] + is_same = all( + latest_entry.get(field) == entry.get(field) + for field in comparable_fields + ) + + if is_same: + return + self.changelog = current_changelog + [entry] + + def save(self, *args: Any, **kwargs: Any) -> None: + self._append_changelog() super().save(*args, **kwargs) + + def calculate_temporary_expiration( + self, revoke_after_days: float + ) -> timezone.datetime: + """ + Calculates the expiration date for temporary approvals from request creation date. + Supports fractional days (e.g., 0.5 for 12 hours). + Returns: + timezone.datetime: The calculated expiration datetime for temporary approvals. + """ + initial_datetime = self.date_created or timezone.now() + return initial_datetime + timezone.timedelta(days=revoke_after_days) diff --git a/impresso/tasks/userSpecialMembershipRequest_tasks.py b/impresso/tasks/userSpecialMembershipRequest_tasks.py index f8ec361..fd73c03 100644 --- a/impresso/tasks/userSpecialMembershipRequest_tasks.py +++ b/impresso/tasks/userSpecialMembershipRequest_tasks.py @@ -1,5 +1,3 @@ -from datetime import timedelta - from celery.utils.log import get_task_logger from django.utils import timezone from django.conf import settings @@ -54,17 +52,6 @@ def _get_revoke_after_days(req: UserSpecialMembershipRequest) -> float: return DEFAULT_DAYS -def _schedule_temporary_revocation(req: UserSpecialMembershipRequest) -> None: - revoke_after_days = _get_revoke_after_days(req) - if req.temporary_expires_at is not None: - revoke_at = req.temporary_expires_at - else: - revoke_at = timezone.now() + timedelta(days=revoke_after_days) - revoke_special_membership_request.apply_async( - kwargs={"instance_id": req.pk}, eta=revoke_at - ) - - @app.task( bind=True, autoretry_for=(Exception,), @@ -92,31 +79,25 @@ def after_special_membership_request_created(self, instance_id: int) -> None: logger.info( f"[UserSpecialMembershipRequest:{instance_id}] triggered signals after_special_membership_request_created, for user={req.user.username} subscription={req.subscription.title if req.subscription else 'None'} status={req.status}" ) - # If the request is pending and temporary auto-accept is enabled, approve it as temporary and schedule revocation, + # If the request is pending and temporary auto-accept is enabled, approve it as temporary, + # and let celery beat handle revocation after expiration. # instead of going through the regular pending flow. if ( req.status == UserSpecialMembershipRequest.STATUS_PENDING and _is_temporary_auto_accept_enabled(req) ): - + # Always set an expires at date in case of temporary approval revoke_after_days = _get_revoke_after_days(req) - if revoke_after_days is None: - logger.warning( - f"[instance:{instance_id}] enableTemporaryAutomaticAcceptance is true but revokeAfterDays is missing or invalid, continuing regular pending flow" - ) - else: - req.status = UserSpecialMembershipRequest.STATUS_APPROVED_TEMPORARY - # add the temporary expiration date using the last modified date + revoke_after_days - req.temporary_expires_at = req.date_last_modified + timedelta( - days=revoke_after_days - ) - req.save() - logger.info( - f"[instance:{instance_id}] auto-accepted as temporary for {revoke_after_days} day(s)" - ) - # The save above triggers the update task via signal, which applies bitmap, - # sends the temporary-approval email, and schedules revocation. - return + req.status = UserSpecialMembershipRequest.STATUS_APPROVED_TEMPORARY + # add the temporary expiration date using the last modified date + revoke_after_days + req.temporary_expires_at = req.calculate_temporary_expiration(revoke_after_days) + req.save() + logger.info( + f"[instance:{instance_id}] auto-accepted as temporary for {revoke_after_days} day(s)" + ) + # The save above triggers the update task via signal, which applies bitmap + # and sends the temporary-approval email. Revocation is handled by celery beat. + return # Apply special membership to bitmap before sending emails, so that the user gets the access as soon as they receive the email apply_special_membership_to_bitmap(instance=req, created=True, logger=logger) @@ -176,14 +157,6 @@ def after_special_membership_request_updated(self, instance_id: int) -> None: logger=logger, is_modality_cc_reviewer_enabled=_is_modality_cc_reviewer_enabled(req), ) - if ( - req.status == UserSpecialMembershipRequest.STATUS_APPROVED_TEMPORARY - or req.temporary_expires_at is not None - ): - logger.info( - f"[instance:{instance_id}] STATUS_APPROVED_TEMPORARY, scheduling revocation..." - ) - _schedule_temporary_revocation(req) @app.task( diff --git a/impresso/tests/management/commands/test_createspecialmembershiprequest.py b/impresso/tests/management/commands/test_createspecialmembershiprequest.py index 2267958..8c9ed93 100644 --- a/impresso/tests/management/commands/test_createspecialmembershiprequest.py +++ b/impresso/tests/management/commands/test_createspecialmembershiprequest.py @@ -9,8 +9,6 @@ from impresso.models import SpecialMembershipDataset, UserSpecialMembershipRequest from impresso.signals import create_default_groups -from unittest.mock import patch - class TestCreateSpecialMembershipRequestCommand(TestCase): """Test the createspecialmembershiprequest management command. @@ -209,9 +207,6 @@ def test_add_notes_and_approved_temporary_status(self) -> None: ) # check user_bitmap.subscriptions - # with patch( - # "impresso.tasks.userSpecialMembershipRequest_tasks.revoke_special_membership_request.apply_async" - # ) as mock_apply_async: def test_create_pending_when_temporary_automatic_acceptance_is_enabled( self, ) -> None: @@ -225,42 +220,31 @@ def test_create_pending_when_temporary_automatic_acceptance_is_enabled( ) out = StringIO() mail.outbox = [] - with patch( - "impresso.tasks.userSpecialMembershipRequest_tasks.revoke_special_membership_request.apply_async" - ) as mock_apply_async: - call_command( - "createspecialmembershiprequest", - self.user.email, - str(revokable_dataset.pk), - stdout=out, - ) - request = UserSpecialMembershipRequest.objects.get( - user=self.user, - subscription=revokable_dataset, - ) - self.assertEqual( - request.status, UserSpecialMembershipRequest.STATUS_APPROVED_TEMPORARY - ) - # check that the revoke task was scheduled with the correct eta (approximately now + 5 days) - self.assertTrue( - request.temporary_expires_at is not None, - "temporary_expires_at should be set for temporary approved requests", - ) - self.assertEqual( - request.temporary_expires_at.date(), - ( - timezone.now() - + timedelta(days=revokable_dataset.metadata["revokeAfterDays"]) - ).date(), - "temporary_expires_at should be approximately now + 5 days", - ) - - self.assertTrue( - mock_apply_async.called, - "Expected revoke_special_membership_request.apply_async to be called for temporary approval", - ) - _args, kwargs = mock_apply_async.call_args - self.assertIn("eta", kwargs, "Expected 'eta' argument in apply_async call") + call_command( + "createspecialmembershiprequest", + self.user.email, + str(revokable_dataset.pk), + stdout=out, + ) + request = UserSpecialMembershipRequest.objects.get( + user=self.user, + subscription=revokable_dataset, + ) + self.assertEqual( + request.status, UserSpecialMembershipRequest.STATUS_APPROVED_TEMPORARY + ) + self.assertTrue( + request.temporary_expires_at is not None, + "temporary_expires_at should be set for temporary approved requests", + ) + self.assertEqual( + request.temporary_expires_at.date(), + ( + timezone.now() + + timedelta(days=revokable_dataset.metadata["revokeAfterDays"]) + ).date(), + "temporary_expires_at should be approximately now + 5 days", + ) def test_revoke_after_half_day_sets_temporary_expires_at(self) -> None: """ @@ -301,13 +285,9 @@ def test_revoke_after_half_day_sets_temporary_expires_at(self) -> None: ) self.assertIn("Created special membership request", out.getvalue()) self.assertIn("Access will be automatically revoked after", mail.outbox[0].body) - # second email is revoke (in test, all celery taks are executed synchronously, so the revoke email is sent immediately after the creation email) + # Revocation is beat-driven, so only the temporary-approval email is sent here. self.assertEqual( len(mail.outbox), - 2, - "Expected two emails to be sent during testing without patch: one for creation and one for revoke", - ) - self.assertIn( - "Your temporary special membership access has expired and is now revoked", - mail.outbox[1].body, + 1, + "Expected one email to be sent: temporary approval notification", ) diff --git a/impresso/tests/tasks/test_revoke_expired_temporary_memberships_beat.py b/impresso/tests/tasks/test_revoke_expired_temporary_memberships_beat.py new file mode 100644 index 0000000..ff3dd39 --- /dev/null +++ b/impresso/tests/tasks/test_revoke_expired_temporary_memberships_beat.py @@ -0,0 +1,65 @@ +from datetime import timedelta + +from django.contrib.auth.models import User +from django.test import TestCase +from django.utils import timezone + +from impresso.models import SpecialMembershipDataset, UserSpecialMembershipRequest +from impresso.tasks.userSpecialMembershipRequest_tasks import ( + revoke_expired_temporary_memberships_beat, +) + + +class TestRevokeExpiredTemporaryMembershipsBeat(TestCase): + def setUp(self) -> None: + self.dataset = SpecialMembershipDataset.objects.create( + title="Beat Revocation Dataset", + metadata={"revokeAfterDays": 1}, + ) + self.expired_user = User.objects.create_user( + username="expired-user", + email="expired@example.com", + password="testpass123", + ) + self.active_user = User.objects.create_user( + username="active-user", + email="active@example.com", + password="testpass123", + ) + + def test_beat_updates_bitmap_counts_for_expired_requests(self) -> None: + expired_request = UserSpecialMembershipRequest.objects.create( + user=self.expired_user, + subscription=self.dataset, + status=UserSpecialMembershipRequest.STATUS_APPROVED_TEMPORARY, + temporary_expires_at=timezone.now() - timedelta(days=1), + ) + active_request = UserSpecialMembershipRequest.objects.create( + user=self.active_user, + subscription=self.dataset, + status=UserSpecialMembershipRequest.STATUS_APPROVED_TEMPORARY, + temporary_expires_at=timezone.now() + timedelta(days=1), + ) + + self.expired_user.refresh_from_db() + self.active_user.refresh_from_db() + + self.assertEqual(self.expired_user.bitmap.subscriptions.count(), 1) + self.assertEqual(self.active_user.bitmap.subscriptions.count(), 1) + + revoke_expired_temporary_memberships_beat.delay() + + expired_request.refresh_from_db() + active_request.refresh_from_db() + self.expired_user.refresh_from_db() + self.active_user.refresh_from_db() + + self.assertEqual( + expired_request.status, UserSpecialMembershipRequest.STATUS_REVOKED + ) + self.assertEqual( + active_request.status, + UserSpecialMembershipRequest.STATUS_APPROVED_TEMPORARY, + ) + self.assertEqual(self.expired_user.bitmap.subscriptions.count(), 0) + self.assertEqual(self.active_user.bitmap.subscriptions.count(), 1) diff --git a/impresso/tests/utils/tasks/test_userSpecialMembershipRequest.py b/impresso/tests/utils/tasks/test_userSpecialMembershipRequest.py index 52e5b78..0de7b11 100644 --- a/impresso/tests/utils/tasks/test_userSpecialMembershipRequest.py +++ b/impresso/tests/utils/tasks/test_userSpecialMembershipRequest.py @@ -394,43 +394,29 @@ def setUp(self): mail.outbox = [] def test_created_request_is_auto_approved_temporarily(self): - from unittest.mock import patch - mail.outbox = [] - with patch( - "impresso.tasks.userSpecialMembershipRequest_tasks.revoke_special_membership_request.apply_async" - ) as mock_apply_async: - req = UserSpecialMembershipRequest.objects.create( - user=self.user, - reviewer=self.reviewer, - subscription=self.dataset, - status=UserSpecialMembershipRequest.STATUS_PENDING, - ) - - req.refresh_from_db() + req = UserSpecialMembershipRequest.objects.create( + user=self.user, + reviewer=self.reviewer, + subscription=self.dataset, + status=UserSpecialMembershipRequest.STATUS_PENDING, + ) - self.assertEqual( - req.status, - UserSpecialMembershipRequest.STATUS_APPROVED_TEMPORARY, - ) - self.assertEqual(self.user.bitmap.subscriptions.count(), 1) - self.assertEqual(len(mail.outbox), 1) - self.assertEqual( - mail.outbox[0].subject, - settings.IMPRESSO_EMAIL_SUBJECT_AFTER_USER_SPECIAL_MEMBERSHIP_REQUEST_ACCEPTED_TEMPORARY_TO_USER, - ) + req.refresh_from_db() - mock_apply_async.assert_called_once() - _, call_kwargs = mock_apply_async.call_args - self.assertEqual(call_kwargs["kwargs"], {"instance_id": req.pk}) - self.assertIn("eta", call_kwargs) - logger.info( - f"Scheduled revocation ETA: {call_kwargs['kwargs']['instance_id']} at {call_kwargs['eta']}" - ) + self.assertEqual( + req.status, + UserSpecialMembershipRequest.STATUS_APPROVED_TEMPORARY, + ) + self.assertEqual(self.user.bitmap.subscriptions.count(), 1) + self.assertEqual(len(mail.outbox), 1) + self.assertEqual( + mail.outbox[0].subject, + settings.IMPRESSO_EMAIL_SUBJECT_AFTER_USER_SPECIAL_MEMBERSHIP_REQUEST_ACCEPTED_TEMPORARY_TO_USER, + ) + self.assertIsNotNone(req.temporary_expires_at) def test_created_requests_with_float_days_for_revoke_after_days(self): - from unittest.mock import patch - dataset_with_float_revoke_after_days = SpecialMembershipDataset.objects.create( title="Float Revoke After Days Dataset", reviewer=self.reviewer, @@ -442,36 +428,47 @@ def test_created_requests_with_float_days_for_revoke_after_days(self): ) mail.outbox = [] + req = UserSpecialMembershipRequest.objects.create( + user=self.user, + reviewer=self.reviewer, + subscription=dataset_with_float_revoke_after_days, + status=UserSpecialMembershipRequest.STATUS_PENDING, + ) + + req.refresh_from_db() + + self.assertEqual( + req.status, + UserSpecialMembershipRequest.STATUS_APPROVED_TEMPORARY, + ) + self.assertEqual(self.user.bitmap.subscriptions.count(), 1) + self.assertEqual(len(mail.outbox), 1) + self.assertEqual( + mail.outbox[0].subject, + settings.IMPRESSO_EMAIL_SUBJECT_AFTER_USER_SPECIAL_MEMBERSHIP_REQUEST_ACCEPTED_TEMPORARY_TO_USER, + ) + self.assertIsNotNone(req.temporary_expires_at) + + def test_update_does_not_schedule_temporary_revocation(self): with patch( "impresso.tasks.userSpecialMembershipRequest_tasks.revoke_special_membership_request.apply_async" ) as mock_apply_async: req = UserSpecialMembershipRequest.objects.create( user=self.user, reviewer=self.reviewer, - subscription=dataset_with_float_revoke_after_days, + subscription=self.dataset, status=UserSpecialMembershipRequest.STATUS_PENDING, ) req.refresh_from_db() + req.notes = "Admin updated notes" + req.save() self.assertEqual( req.status, UserSpecialMembershipRequest.STATUS_APPROVED_TEMPORARY, ) - self.assertEqual(self.user.bitmap.subscriptions.count(), 1) - self.assertEqual(len(mail.outbox), 1) - self.assertEqual( - mail.outbox[0].subject, - settings.IMPRESSO_EMAIL_SUBJECT_AFTER_USER_SPECIAL_MEMBERSHIP_REQUEST_ACCEPTED_TEMPORARY_TO_USER, - ) - - mock_apply_async.assert_called_once() - _, call_kwargs = mock_apply_async.call_args - self.assertEqual(call_kwargs["kwargs"], {"instance_id": req.pk}) - self.assertIn("eta", call_kwargs) - logger.info( - f"Scheduled revocation ETA: {call_kwargs['kwargs']['instance_id']} at {call_kwargs['eta']}" - ) + mock_apply_async.assert_not_called() class TestTemporaryAutomaticRevocation(TestCase): From 04c59f679af7a9f7af6d609503b42dac459b69f2 Mon Sep 17 00:00:00 2001 From: Daniele Guido Date: Thu, 7 May 2026 12:04:14 +0200 Subject: [PATCH 19/30] Potential fix for pull request finding Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com> --- ...er_special_membership_request_approved_temporary_to_user.txt | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/impresso/templates/emails/user_special_membership_request_approved_temporary_to_user.txt b/impresso/templates/emails/user_special_membership_request_approved_temporary_to_user.txt index da8333d..e901d9c 100644 --- a/impresso/templates/emails/user_special_membership_request_approved_temporary_to_user.txt +++ b/impresso/templates/emails/user_special_membership_request_approved_temporary_to_user.txt @@ -5,7 +5,7 @@ Dear {{ user.first_name }}, We have received your request for Special Membership Access to {{ user_special_membership_request.subscription.title }}. To allow you to begin your work immediately, your account has been granted Provisional Access for a period of {{ user_special_membership_request_duration }} until {{ user_special_membership_request.temporary_expires_at|date:"l, Y-m-d H:i" }} -and gives you full access contents from Test Dataset. Note that Impresso Terms of Use always apply. +and gives you access to the contents from {{ user_special_membership_request.subscription.title }}. Note that Impresso Terms of Use always apply. Next Steps & Extensions: Access will be automatically revoked after {{ user_special_membership_request_duration }} from now. For continued access, please request "Standard Access" via the Impresso platform. From 4e3261cb3b3850ddd0577aabe92e12e55cf79b86 Mon Sep 17 00:00:00 2001 From: Daniele Guido <1181642+danieleguido@users.noreply.github.com> Date: Thu, 7 May 2026 12:13:37 +0200 Subject: [PATCH 20/30] Update user_special_membership_request_approved_temporary_to_user.html --- ...r_special_membership_request_approved_temporary_to_user.html | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/impresso/templates/emails/user_special_membership_request_approved_temporary_to_user.html b/impresso/templates/emails/user_special_membership_request_approved_temporary_to_user.html index 487d5e8..f7b17c0 100644 --- a/impresso/templates/emails/user_special_membership_request_approved_temporary_to_user.html +++ b/impresso/templates/emails/user_special_membership_request_approved_temporary_to_user.html @@ -9,7 +9,7 @@ Provisional Access for a period of {{ user_special_membership_request_duration }} until {{ user_special_membership_request.temporary_expires_at|date:"l, Y-m-d H:i" }} - and gives you full access contents from Test Dataset. Note that Impresso Terms + and gives you full access contents from {{ user_special_membership_request.subscription.title }}. Note that Impresso Terms of Use always apply.

        From b489d01bbf71ad46c0be383bd668b4a1d7643289 Mon Sep 17 00:00:00 2001 From: Daniele Guido <1181642+danieleguido@users.noreply.github.com> Date: Thu, 7 May 2026 12:15:44 +0200 Subject: [PATCH 21/30] Update userSpecialMembershipRequest.py --- impresso/models/userSpecialMembershipRequest.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/impresso/models/userSpecialMembershipRequest.py b/impresso/models/userSpecialMembershipRequest.py index 910d2d7..b969757 100644 --- a/impresso/models/userSpecialMembershipRequest.py +++ b/impresso/models/userSpecialMembershipRequest.py @@ -3,7 +3,7 @@ from django.contrib.auth.models import User from .specialMembershipDataset import SpecialMembershipDataset from django.utils import timezone - +from datetime import timedelta # --- Typing Definition for Changelog Entry --- class ChangelogEntry(TypedDict): @@ -150,4 +150,4 @@ def calculate_temporary_expiration( timezone.datetime: The calculated expiration datetime for temporary approvals. """ initial_datetime = self.date_created or timezone.now() - return initial_datetime + timezone.timedelta(days=revoke_after_days) + return initial_datetime + timedelta(days=revoke_after_days) From 584815e726d10044fb2c07e176707e06771171b7 Mon Sep 17 00:00:00 2001 From: Daniele Guido <1181642+danieleguido@users.noreply.github.com> Date: Thu, 7 May 2026 12:43:29 +0200 Subject: [PATCH 22/30] Use settings default for temporary revoke days If revoke_after isn't passed for a temporary-approved SpecialMembershipRequest, derive it from dataset.get_revoke_after_days with a fallback to settings.IMPRESSO_SPECIAL_MEMBERSHIP_TEMPORARY_APPROVAL_DEFAULT_DAYS. Import django.conf.settings, compute temporary_expires_at from now + revoke_after, and update the test to use get_revoke_after_days(default_days=1). Also fix a typo in the help text (subscription). --- .../createspecialmembershiprequest.py | 22 ++++- .../test_createspecialmembershiprequest.py | 15 +++- .../test_userSpecialMembershipRequest.py | 86 +++++++++---------- 3 files changed, 71 insertions(+), 52 deletions(-) diff --git a/impresso/management/commands/createspecialmembershiprequest.py b/impresso/management/commands/createspecialmembershiprequest.py index cf07c79..ec1791a 100644 --- a/impresso/management/commands/createspecialmembershiprequest.py +++ b/impresso/management/commands/createspecialmembershiprequest.py @@ -6,6 +6,7 @@ from django.db import IntegrityError from django.utils import timezone +from django.conf import settings from impresso.models import SpecialMembershipDataset, UserSpecialMembershipRequest @@ -53,7 +54,7 @@ def add_arguments(self, parser) -> None: type=float, default=None, dest="revoke_after", - help="Number of days after which the temporary membership expires (sets temporary_expires_at and overrides the default value from subscrption metadata)", + help="Number of days after which the temporary membership expires (sets temporary_expires_at and overrides the default value from subscription metadata)", ) def handle( @@ -76,7 +77,24 @@ def handle( ) from exc revoke_after: float | None = options["revoke_after"] - # normally this would be set from request.creation_date + revoke_after, but since we're creating the request now we can set it from now + revoke_after :) + # Check that revoke_after options is valid int or float greater than 0 + if revoke_after is not None: + if revoke_after <= 0: + raise CommandError( + f"Invalid value for --revoke-after: {revoke_after}. It must be a positive number." + ) + + if ( + revoke_after is None + and options["status"] + == UserSpecialMembershipRequest.STATUS_APPROVED_TEMPORARY + ): + revoke_after = dataset.resolve_revoke_after_days( + default_days=settings.IMPRESSO_SPECIAL_MEMBERSHIP_TEMPORARY_APPROVAL_DEFAULT_DAYS + ) + + # normally this would be set from request.creation_date + revoke_after, + # but since we're creating the request now we can set it from now + revoke_after. temporary_expires_at = ( timezone.now() + timedelta(days=revoke_after) if revoke_after is not None diff --git a/impresso/tests/management/commands/test_createspecialmembershiprequest.py b/impresso/tests/management/commands/test_createspecialmembershiprequest.py index 8c9ed93..f923ec7 100644 --- a/impresso/tests/management/commands/test_createspecialmembershiprequest.py +++ b/impresso/tests/management/commands/test_createspecialmembershiprequest.py @@ -138,8 +138,6 @@ class TestCreateSpecialMembershipRequestCommandWithOptions(TestCase): def setUp(self) -> None: create_default_groups(sender="impresso") - create_default_groups(sender="impresso") - self.reviewer = User.objects.create_user( username="reviewer", first_name="John", @@ -206,6 +204,15 @@ def test_add_notes_and_approved_temporary_status(self) -> None: stdout=out, ) # check user_bitmap.subscriptions + request = UserSpecialMembershipRequest.objects.get( + user=self.user, + subscription=self.dataset, + ) + self.user.refresh_from_db() + self.assertEqual(request.status, UserSpecialMembershipRequest.STATUS_APPROVED) + self.assertEqual(request.reviewer, self.reviewer) + self.assertEqual(request.notes, "This is an approved request with notes.") + self.assertEqual(self.user.bitmap.subscriptions.count(), 1) def test_create_pending_when_temporary_automatic_acceptance_is_enabled( self, @@ -241,7 +248,9 @@ def test_create_pending_when_temporary_automatic_acceptance_is_enabled( request.temporary_expires_at.date(), ( timezone.now() - + timedelta(days=revokable_dataset.metadata["revokeAfterDays"]) + + timedelta( + days=revokable_dataset.resolve_revoke_after_days(default_days=1) + ) ).date(), "temporary_expires_at should be approximately now + 5 days", ) diff --git a/impresso/tests/utils/tasks/test_userSpecialMembershipRequest.py b/impresso/tests/utils/tasks/test_userSpecialMembershipRequest.py index 0de7b11..e299ea0 100644 --- a/impresso/tests/utils/tasks/test_userSpecialMembershipRequest.py +++ b/impresso/tests/utils/tasks/test_userSpecialMembershipRequest.py @@ -512,13 +512,6 @@ def test_expired_temporary_request_is_revoked(self): self.user.refresh_from_db() self.assertEqual(self.user.bitmap.subscriptions.count(), 0) self.assertEqual(len(mail.outbox), 2) - # self.assertTrue( - # all( - # email.subject - # == settings.IMPRESSO_EMAIL_SUBJECT_AFTER_USER_SPECIAL_MEMBERSHIP_REQUEST_REVOKED_TO_USER - # for email in mail.outbox - # ) - # ) def test_revoke_expired_temporary_memberships_beat(self): @@ -526,46 +519,45 @@ def test_revoke_expired_temporary_memberships_beat(self): username="other-user", email="other-user@example.com" ) - with patch( - "impresso.tasks.userSpecialMembershipRequest_tasks.revoke_special_membership_request.apply_async" - ) as mock_delay: - - request_that_need_to_be_revoked = ( - UserSpecialMembershipRequest.objects.create( - user=self.user, - subscription=self.dataset, - status=UserSpecialMembershipRequest.STATUS_APPROVED_TEMPORARY, - temporary_expires_at=timezone.now() - timedelta(days=1), - ) - ) + request_that_need_to_be_revoked = UserSpecialMembershipRequest.objects.create( + user=self.user, + subscription=self.dataset, + status=UserSpecialMembershipRequest.STATUS_APPROVED_TEMPORARY, + temporary_expires_at=timezone.now() - timedelta(days=1), + ) - req2 = UserSpecialMembershipRequest.objects.create( - user=user2, - subscription=self.dataset, - status=UserSpecialMembershipRequest.STATUS_APPROVED_TEMPORARY, - temporary_expires_at=timezone.now() + timedelta(days=1), - ) - all_subscriptions = UserSpecialMembershipRequest.objects.filter( - status=UserSpecialMembershipRequest.STATUS_APPROVED_TEMPORARY - ) - # they are still "acive" - self.assertEqual(all_subscriptions.count(), 2) - self.assertEqual( - ",".join(all_subscriptions.values_list("status", flat=True)), - "temporary,temporary", - "both requests should not be expired yet", - ) - print("statuseeeeeeees", self.user.bitmap.subscriptions.count()) - print("statuseeeeeeees", user2.bitmap.subscriptions.count()) - active_subscriptions = UserSpecialMembershipRequest.objects.filter( - status=UserSpecialMembershipRequest.STATUS_APPROVED_TEMPORARY, - temporary_expires_at__gt=timezone.now(), - ) - self.assertEqual(active_subscriptions.count(), 1) + request_still_active = UserSpecialMembershipRequest.objects.create( + user=user2, + subscription=self.dataset, + status=UserSpecialMembershipRequest.STATUS_APPROVED_TEMPORARY, + temporary_expires_at=timezone.now() + timedelta(days=1), + ) - print("DODODODODODODODODD", active_subscriptions.count()) - print( - "statuseeeeeeees", active_subscriptions.values_list("status", flat=True) - ) + all_subscriptions = UserSpecialMembershipRequest.objects.filter( + status=UserSpecialMembershipRequest.STATUS_APPROVED_TEMPORARY + ) + # they are both "STATUS_APPROVED_TEMPORARY" + self.assertEqual( + ",".join(all_subscriptions.values_list("status", flat=True)), + "temporary,temporary", + "both requests should not be expired yet", + ) + # one of them has already expired based on temporary_expires_at + expired_subscriptions = UserSpecialMembershipRequest.objects.filter( + status=UserSpecialMembershipRequest.STATUS_APPROVED_TEMPORARY, + temporary_expires_at__lt=timezone.now(), + ) + self.assertEqual(expired_subscriptions.count(), 1) - revoke_expired_temporary_memberships_beat.delay() + revoke_expired_temporary_memberships_beat.delay() + # now self user request should be revoked, while the other user's request should still be active + request_that_need_to_be_revoked.refresh_from_db() + request_still_active.refresh_from_db() + self.assertEqual( + request_that_need_to_be_revoked.status, + UserSpecialMembershipRequest.STATUS_REVOKED, + ) + self.assertEqual( + request_still_active.status, + UserSpecialMembershipRequest.STATUS_APPROVED_TEMPORARY, + ) From bb4e84b01928d1f52dda553afa8b3886a2883404 Mon Sep 17 00:00:00 2001 From: Daniele Guido <1181642+danieleguido@users.noreply.github.com> Date: Thu, 7 May 2026 12:43:40 +0200 Subject: [PATCH 23/30] Update settings.py schedule every 30 min --- impresso/settings.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/impresso/settings.py b/impresso/settings.py index 5f65b8b..24eba2c 100644 --- a/impresso/settings.py +++ b/impresso/settings.py @@ -176,9 +176,9 @@ from celery.schedules import crontab CELERY_BEAT_SCHEDULE = { - "revoke-expired-temporary-memberships-every-hour": { + "revoke-expired-temporary-memberships-every-five-minutes": { "task": "impresso.tasks.userSpecialMembershipRequest_tasks.revoke_expired_temporary_memberships_beat", - "schedule": crontab(minute=0), + "schedule": crontab(minute="*/30"), }, } From f65eb83a6a66cb7eeb546b2ef6f82f9dc6e34306 Mon Sep 17 00:00:00 2001 From: Daniele Guido <1181642+danieleguido@users.noreply.github.com> Date: Thu, 7 May 2026 12:44:11 +0200 Subject: [PATCH 24/30] specialMembershipDataset: add common method on model directly --- impresso/models/specialMembershipDataset.py | 31 ++++++++- .../models/test_specialMembershipDataset.py | 66 +++++++++++++++++++ 2 files changed, 96 insertions(+), 1 deletion(-) create mode 100644 impresso/tests/models/test_specialMembershipDataset.py diff --git a/impresso/models/specialMembershipDataset.py b/impresso/models/specialMembershipDataset.py index 2f9f242..b1596c7 100644 --- a/impresso/models/specialMembershipDataset.py +++ b/impresso/models/specialMembershipDataset.py @@ -1,5 +1,6 @@ from typing import Optional, TypedDict - +from numbers import Real +from django.conf import settings from django.db import models from django.db.models import Max @@ -54,6 +55,34 @@ class SpecialMembershipDataset(models.Model): def __str__(self): return self.title + def is_temporary_auto_accept_enabled(self) -> bool: + return bool(self.metadata.get("enableTemporaryAutomaticAcceptance")) + + def is_modality_cc_reviewer_enabled(self) -> bool: + return bool( + self.metadata.get("modality") + == settings.IMPRESSO_EMAIL_MODALITY_SPECIAL_MEMBERSHIP_REQUEST_CC_REVIEWER + ) + + def resolve_revoke_after_days(self, default_days: Optional[float]) -> float: + """ + Returns the number of days after which a temporary approval + should be revoked. + + Priority: + 1. metadata["revokeAfterDays"] + 2. provided default_days + 3. Django settings fallback + """ + revoke_after_days = self.metadata.get("revokeAfterDays") + if isinstance(revoke_after_days, Real) and revoke_after_days > 0: + return float(revoke_after_days) + if isinstance(default_days, Real) and default_days > 0: + return float(default_days) + return float( + settings.IMPRESSO_SPECIAL_MEMBERSHIP_TEMPORARY_APPROVAL_DEFAULT_DAYS + ) + class Meta: verbose_name = "Special Membership Access" verbose_name_plural = "Special Membership Accesses" diff --git a/impresso/tests/models/test_specialMembershipDataset.py b/impresso/tests/models/test_specialMembershipDataset.py new file mode 100644 index 0000000..f568029 --- /dev/null +++ b/impresso/tests/models/test_specialMembershipDataset.py @@ -0,0 +1,66 @@ +from django.conf import settings +from django.test import TestCase + +from impresso.models import SpecialMembershipDataset + + +class SpecialMembershipDatasetMetadataMethodsTestCase(TestCase): + def test_is_temporary_auto_accept_enabled(self) -> None: + dataset_enabled = SpecialMembershipDataset.objects.create( + title="Dataset Enabled", + metadata={"enableTemporaryAutomaticAcceptance": True}, + ) + dataset_disabled = SpecialMembershipDataset.objects.create( + title="Dataset Disabled", + metadata={"enableTemporaryAutomaticAcceptance": False}, + ) + + self.assertTrue(dataset_enabled.is_temporary_auto_accept_enabled()) + self.assertFalse(dataset_disabled.is_temporary_auto_accept_enabled()) + + def test_is_modality_cc_reviewer_enabled(self) -> None: + dataset_cc = SpecialMembershipDataset.objects.create( + title="Dataset CC", + metadata={ + "modality": settings.IMPRESSO_EMAIL_MODALITY_SPECIAL_MEMBERSHIP_REQUEST_CC_REVIEWER + }, + ) + dataset_notify = SpecialMembershipDataset.objects.create( + title="Dataset Notify", + metadata={ + "modality": settings.IMPRESSO_EMAIL_MODALITY_SPECIAL_MEMBERSHIP_REQUEST_NOTIFY_REVIEWER + }, + ) + + self.assertTrue(dataset_cc.is_modality_cc_reviewer_enabled()) + self.assertFalse(dataset_notify.is_modality_cc_reviewer_enabled()) + + def test_resolve_revoke_after_days_from_metadata(self) -> None: + dataset = SpecialMembershipDataset.objects.create( + title="Dataset Revoke", + metadata={"revokeAfterDays": 2.5}, + ) + + self.assertEqual(dataset.resolve_revoke_after_days(default_days=7), 2.5) + + def test_resolve_revoke_after_days_falls_back_to_default(self) -> None: + dataset_missing = SpecialMembershipDataset.objects.create( + title="Dataset Missing", + metadata={}, + ) + dataset_zero = SpecialMembershipDataset.objects.create( + title="Dataset Zero", + metadata={"revokeAfterDays": 0}, + ) + dataset_invalid = SpecialMembershipDataset.objects.create( + title="Dataset Invalid", + metadata={"revokeAfterDays": "seven"}, + ) + + self.assertEqual(dataset_missing.resolve_revoke_after_days(default_days=7), 7.0) + self.assertEqual(dataset_zero.resolve_revoke_after_days(default_days=7), 7.0) + self.assertEqual(dataset_invalid.resolve_revoke_after_days(default_days=7), 7.0) + self.assertEqual( + dataset_zero.resolve_revoke_after_days(default_days=-7), + settings.IMPRESSO_SPECIAL_MEMBERSHIP_TEMPORARY_APPROVAL_DEFAULT_DAYS, + ) From b3356b41926cad5fe8d7d09f1382aea114696e7b Mon Sep 17 00:00:00 2001 From: Daniele Guido <1181642+danieleguido@users.noreply.github.com> Date: Thu, 7 May 2026 12:44:24 +0200 Subject: [PATCH 25/30] Update .gitignore --- .gitignore | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/.gitignore b/.gitignore index 8eb1e5e..178f4ff 100644 --- a/.gitignore +++ b/.gitignore @@ -120,4 +120,5 @@ ___* # local docker settings and config .docker/* -!.docker/.gitkeep \ No newline at end of file +!.docker/.gitkeep +*.db From 1c9fa8709ee28b42881ec80632ce83ce310fee1c Mon Sep 17 00:00:00 2001 From: Daniele Guido <1181642+danieleguido@users.noreply.github.com> Date: Thu, 7 May 2026 12:44:45 +0200 Subject: [PATCH 26/30] use newly created methods --- .../userSpecialMembershipRequest_tasks.py | 21 ++++++++----------- .../tasks/userSpecialMembershipRequest.py | 14 ++++--------- 2 files changed, 13 insertions(+), 22 deletions(-) diff --git a/impresso/tasks/userSpecialMembershipRequest_tasks.py b/impresso/tasks/userSpecialMembershipRequest_tasks.py index fd73c03..938875e 100644 --- a/impresso/tasks/userSpecialMembershipRequest_tasks.py +++ b/impresso/tasks/userSpecialMembershipRequest_tasks.py @@ -15,19 +15,16 @@ def _is_temporary_auto_accept_enabled(req: UserSpecialMembershipRequest) -> bool: if not req.subscription: return False - return bool(req.subscription.metadata.get("enableTemporaryAutomaticAcceptance")) + return req.subscription.is_temporary_auto_accept_enabled() def _is_modality_cc_reviewer_enabled(req: UserSpecialMembershipRequest) -> bool: if not req.subscription: return False - return bool( - req.subscription.metadata.get("modality") - == settings.IMPRESSO_EMAIL_MODALITY_SPECIAL_MEMBERSHIP_REQUEST_CC_REVIEWER - ) + return req.subscription.is_modality_cc_reviewer_enabled() -def _get_revoke_after_days(req: UserSpecialMembershipRequest) -> float: +def _resolve_revoke_after_days(req: UserSpecialMembershipRequest) -> float: """ Always return a positive number of days from the request's subscription metadata. If they're wrong or missing, get the value from the default settings.IMPRESSO_SPECIAL_MEMBERSHIP_TEMPORARY_APPROVAL_DEFAULT_DAYS @@ -42,14 +39,14 @@ def _get_revoke_after_days(req: UserSpecialMembershipRequest) -> float: ) return DEFAULT_DAYS - revoke_after_days = req.subscription.metadata.get("revokeAfterDays") - if isinstance(revoke_after_days, (int, float)) and revoke_after_days > 0: - return float(revoke_after_days) + revoke_after_days = req.subscription.resolve_revoke_after_days(DEFAULT_DAYS) + if revoke_after_days != float(DEFAULT_DAYS): + return revoke_after_days logger.warning( - f"[instance:{req.pk}] revokeAfterDays is missing or invalid in subscription metadata (value={revoke_after_days}), using default {DEFAULT_DAYS} day(s)" + f"[instance:{req.pk}] revokeAfterDays is missing or invalid in subscription metadata, using default {DEFAULT_DAYS} day(s)" ) - return DEFAULT_DAYS + return revoke_after_days @app.task( @@ -87,7 +84,7 @@ def after_special_membership_request_created(self, instance_id: int) -> None: and _is_temporary_auto_accept_enabled(req) ): # Always set an expires at date in case of temporary approval - revoke_after_days = _get_revoke_after_days(req) + revoke_after_days = _resolve_revoke_after_days(req) req.status = UserSpecialMembershipRequest.STATUS_APPROVED_TEMPORARY # add the temporary expiration date using the last modified date + revoke_after_days req.temporary_expires_at = req.calculate_temporary_expiration(revoke_after_days) diff --git a/impresso/utils/tasks/userSpecialMembershipRequest.py b/impresso/utils/tasks/userSpecialMembershipRequest.py index 5e95412..ecb25ae 100644 --- a/impresso/utils/tasks/userSpecialMembershipRequest.py +++ b/impresso/utils/tasks/userSpecialMembershipRequest.py @@ -177,16 +177,10 @@ def send_email_after_user_special_membership_request_created( status_label = dict(UserSpecialMembershipRequest.STATUS_CHOICES).get( instance.status, instance.status ) - # get modality from instance metadata, default to NOTIFY_REVIEWER if no reviewer or no modality specified - modality = ( - instance.subscription.metadata.get("modality", None) - if instance.subscription - else None - ) - if not modality: - modality = ( - settings.IMPRESSO_EMAIL_MODALITY_SPECIAL_MEMBERSHIP_REQUEST_NOTIFY_REVIEWER - ) + # Default to NOTIFY_REVIEWER unless dataset metadata explicitly enables CC_REVIEWER. + modality = settings.IMPRESSO_EMAIL_MODALITY_SPECIAL_MEMBERSHIP_REQUEST_NOTIFY_REVIEWER + if instance.subscription and instance.subscription.is_modality_cc_reviewer_enabled(): + modality = settings.IMPRESSO_EMAIL_MODALITY_SPECIAL_MEMBERSHIP_REQUEST_CC_REVIEWER if ( modality == settings.IMPRESSO_EMAIL_MODALITY_SPECIAL_MEMBERSHIP_REQUEST_CC_REVIEWER From 926bb48a8a483c857b0e65909cf91caf4cb93dcc Mon Sep 17 00:00:00 2001 From: Daniele Guido <1181642+danieleguido@users.noreply.github.com> Date: Thu, 7 May 2026 13:59:19 +0200 Subject: [PATCH 27/30] remove celery beat --- .../commands/checktemporarymemberships.py | 23 ++++++++++- impresso/settings.py | 9 ---- .../userSpecialMembershipRequest_tasks.py | 13 ++++-- .../test_checktemporarymemberships.py | 41 ++++++++++--------- .../test_createspecialmembershiprequest.py | 2 +- ...voke_expired_temporary_memberships_beat.py | 4 +- .../test_userSpecialMembershipRequest.py | 6 +-- 7 files changed, 58 insertions(+), 40 deletions(-) diff --git a/impresso/management/commands/checktemporarymemberships.py b/impresso/management/commands/checktemporarymemberships.py index d59e408..c4a598e 100644 --- a/impresso/management/commands/checktemporarymemberships.py +++ b/impresso/management/commands/checktemporarymemberships.py @@ -2,9 +2,15 @@ from django.core.management.base import BaseCommand from django.utils import timezone from impresso.models import UserSpecialMembershipRequest +from impresso.tasks.userSpecialMembershipRequest_tasks import ( + revoke_expired_temporary_memberships, +) class Command(BaseCommand): - help = "Check special memberships that have the temporary approved status." + help = ( + "Check temporary special memberships and enqueue asynchronous revocation for " + "expired ones." + ) def add_arguments(self, parser) -> None: parser.add_argument( @@ -39,3 +45,18 @@ def handle(self, *args: Any, **options: Any) -> None: f"- Request ID: {req.pk}, User: {req.user.username}, " f"Dataset: {dataset_title}{expired_str}" ) + + if dry_run: + self.stdout.write( + self.style.NOTICE( + "Dry run completed: task dispatch skipped." + ) + ) + return + + async_result = revoke_expired_temporary_memberships.delay() + self.stdout.write( + self.style.SUCCESS( + f"Enqueued revoke_expired_temporary_memberships task (id={async_result.id})." + ) + ) diff --git a/impresso/settings.py b/impresso/settings.py index 24eba2c..f69f918 100644 --- a/impresso/settings.py +++ b/impresso/settings.py @@ -173,15 +173,6 @@ get_env_variable("CELERY_BROKER_CONNECTION_RETRY_ON_STARTUP", False) == "True" ) -from celery.schedules import crontab - -CELERY_BEAT_SCHEDULE = { - "revoke-expired-temporary-memberships-every-five-minutes": { - "task": "impresso.tasks.userSpecialMembershipRequest_tasks.revoke_expired_temporary_memberships_beat", - "schedule": crontab(minute="*/30"), - }, -} - IMPRESSO_BASE_URL = get_env_variable("IMPRESSO_BASE_URL", "https://impresso-project.ch") IMPRESSO_INSTITUTIONS_ACCESS_URL = get_env_variable( "IMPRESSO_INSTITUTIONS_ACCESS_URL", diff --git a/impresso/tasks/userSpecialMembershipRequest_tasks.py b/impresso/tasks/userSpecialMembershipRequest_tasks.py index 938875e..2ef37f2 100644 --- a/impresso/tasks/userSpecialMembershipRequest_tasks.py +++ b/impresso/tasks/userSpecialMembershipRequest_tasks.py @@ -76,8 +76,9 @@ def after_special_membership_request_created(self, instance_id: int) -> None: logger.info( f"[UserSpecialMembershipRequest:{instance_id}] triggered signals after_special_membership_request_created, for user={req.user.username} subscription={req.subscription.title if req.subscription else 'None'} status={req.status}" ) - # If the request is pending and temporary auto-accept is enabled, approve it as temporary, - # and let celery beat handle revocation after expiration. + # If the request is pending and temporary auto-accept is enabled, approve it as temporary. + # Expired temporary memberships are later revoked by a dedicated Celery task, + # typically triggered from a cron-scheduled management command. # instead of going through the regular pending flow. if ( req.status == UserSpecialMembershipRequest.STATUS_PENDING @@ -93,7 +94,7 @@ def after_special_membership_request_created(self, instance_id: int) -> None: f"[instance:{instance_id}] auto-accepted as temporary for {revoke_after_days} day(s)" ) # The save above triggers the update task via signal, which applies bitmap - # and sends the temporary-approval email. Revocation is handled by celery beat. + # and sends the temporary-approval email. return # Apply special membership to bitmap before sending emails, so that the user gets the access as soon as they receive the email @@ -200,7 +201,7 @@ def revoke_special_membership_request(self, instance_id: int) -> None: retry_kwargs={"max_retries": 5}, retry_jitter=True, ) -def revoke_expired_temporary_memberships_beat(self) -> None: +def revoke_expired_temporary_memberships(self) -> None: """ Periodic task to revoke any STATUS_APPROVED_TEMPORARY memberships that have passed their temporary_expires_at date. @@ -219,3 +220,7 @@ def revoke_expired_temporary_memberships_beat(self) -> None: for req in expired_requests: revoke_special_membership_request.delay(instance_id=req.pk) + + +# Backward-compatible alias for older imports/usages. +revoke_expired_temporary_memberships_beat = revoke_expired_temporary_memberships diff --git a/impresso/tests/management/commands/test_checktemporarymemberships.py b/impresso/tests/management/commands/test_checktemporarymemberships.py index 950de2b..1b15c4c 100644 --- a/impresso/tests/management/commands/test_checktemporarymemberships.py +++ b/impresso/tests/management/commands/test_checktemporarymemberships.py @@ -56,19 +56,20 @@ def test_finds_temporary_memberships(self) -> None: mail.outbox = [] # Clear the outbox before creating the temporary approved request - with patch( - "impresso.tasks.userSpecialMembershipRequest_tasks.revoke_special_membership_request.apply_async" - ) as mock_apply_async: - temp_req = UserSpecialMembershipRequest.objects.create( - user=self.user2, - subscription=self.dataset, - status=UserSpecialMembershipRequest.STATUS_APPROVED_TEMPORARY, - temporary_expires_at=timezone.now() + timedelta(days=7), - ) - self.assertEqual(len(mail.outbox), 1) + temp_req = UserSpecialMembershipRequest.objects.create( + user=self.user2, + subscription=self.dataset, + status=UserSpecialMembershipRequest.STATUS_APPROVED_TEMPORARY, + temporary_expires_at=timezone.now() + timedelta(days=7), + ) + self.assertEqual(len(mail.outbox), 1) out = StringIO() - call_command("checktemporarymemberships", stdout=out) + with patch( + "impresso.management.commands.checktemporarymemberships.revoke_expired_temporary_memberships.delay" + ) as mock_delay: + call_command("checktemporarymemberships", stdout=out) + mock_delay.assert_called_once_with() output = out.getvalue() self.assertIn("Found 1 special memberships with temporary approval.", output) @@ -76,18 +77,17 @@ def test_finds_temporary_memberships(self) -> None: self.assertIn("User: user2", output) self.assertIn("Dataset: Dataset Alpha", output) self.assertIn("(Expires:", output) + self.assertIn("Enqueued revoke_expired_temporary_memberships task", output) def test_shows_expired_memberships(self) -> None: # Create a temporary approved request that has already expired - with patch( - "impresso.tasks.userSpecialMembershipRequest_tasks.revoke_special_membership_request.apply_async" - ): - temp_req = UserSpecialMembershipRequest.objects.create( - user=self.user1, - subscription=self.dataset, - status=UserSpecialMembershipRequest.STATUS_APPROVED_TEMPORARY, - temporary_expires_at=timezone.now() - timedelta(days=1), - ) + + temp_req = UserSpecialMembershipRequest.objects.create( + user=self.user1, + subscription=self.dataset, + status=UserSpecialMembershipRequest.STATUS_APPROVED_TEMPORARY, + temporary_expires_at=timezone.now() - timedelta(days=1), + ) out = StringIO() call_command("checktemporarymemberships", stdout=out) @@ -104,3 +104,4 @@ def test_dry_run_flag(self) -> None: output = out.getvalue() self.assertIn("Running in DRY RUN mode.", output) self.assertIn("Found 0 special memberships with temporary approval.", output) + self.assertIn("Dry run completed: task dispatch skipped.", output) diff --git a/impresso/tests/management/commands/test_createspecialmembershiprequest.py b/impresso/tests/management/commands/test_createspecialmembershiprequest.py index f923ec7..f2eb2b0 100644 --- a/impresso/tests/management/commands/test_createspecialmembershiprequest.py +++ b/impresso/tests/management/commands/test_createspecialmembershiprequest.py @@ -294,7 +294,7 @@ def test_revoke_after_half_day_sets_temporary_expires_at(self) -> None: ) self.assertIn("Created special membership request", out.getvalue()) self.assertIn("Access will be automatically revoked after", mail.outbox[0].body) - # Revocation is beat-driven, so only the temporary-approval email is sent here. + # Revocation is asynchronous, so only the temporary-approval email is sent here. self.assertEqual( len(mail.outbox), 1, diff --git a/impresso/tests/tasks/test_revoke_expired_temporary_memberships_beat.py b/impresso/tests/tasks/test_revoke_expired_temporary_memberships_beat.py index ff3dd39..4ab515b 100644 --- a/impresso/tests/tasks/test_revoke_expired_temporary_memberships_beat.py +++ b/impresso/tests/tasks/test_revoke_expired_temporary_memberships_beat.py @@ -6,7 +6,7 @@ from impresso.models import SpecialMembershipDataset, UserSpecialMembershipRequest from impresso.tasks.userSpecialMembershipRequest_tasks import ( - revoke_expired_temporary_memberships_beat, + revoke_expired_temporary_memberships, ) @@ -47,7 +47,7 @@ def test_beat_updates_bitmap_counts_for_expired_requests(self) -> None: self.assertEqual(self.expired_user.bitmap.subscriptions.count(), 1) self.assertEqual(self.active_user.bitmap.subscriptions.count(), 1) - revoke_expired_temporary_memberships_beat.delay() + revoke_expired_temporary_memberships.delay() expired_request.refresh_from_db() active_request.refresh_from_db() diff --git a/impresso/tests/utils/tasks/test_userSpecialMembershipRequest.py b/impresso/tests/utils/tasks/test_userSpecialMembershipRequest.py index e299ea0..c634504 100644 --- a/impresso/tests/utils/tasks/test_userSpecialMembershipRequest.py +++ b/impresso/tests/utils/tasks/test_userSpecialMembershipRequest.py @@ -12,7 +12,7 @@ from impresso.models.profile import Profile from impresso.tasks.userSpecialMembershipRequest_tasks import ( revoke_special_membership_request, - revoke_expired_temporary_memberships_beat, + revoke_expired_temporary_memberships, ) from impresso.utils.tasks.userSpecialMembershipRequest import ( send_email_after_user_special_membership_request_created, @@ -513,7 +513,7 @@ def test_expired_temporary_request_is_revoked(self): self.assertEqual(self.user.bitmap.subscriptions.count(), 0) self.assertEqual(len(mail.outbox), 2) - def test_revoke_expired_temporary_memberships_beat(self): + def test_revoke_expired_temporary_memberships(self): user2 = User.objects.create_user( username="other-user", email="other-user@example.com" @@ -549,7 +549,7 @@ def test_revoke_expired_temporary_memberships_beat(self): ) self.assertEqual(expired_subscriptions.count(), 1) - revoke_expired_temporary_memberships_beat.delay() + revoke_expired_temporary_memberships.delay() # now self user request should be revoked, while the other user's request should still be active request_that_need_to_be_revoked.refresh_from_db() request_still_active.refresh_from_db() From 2588ecc3148177bcb0ebcd1bf33e583bf10bbadb Mon Sep 17 00:00:00 2001 From: Daniele Guido <1181642+danieleguido@users.noreply.github.com> Date: Thu, 7 May 2026 14:06:55 +0200 Subject: [PATCH 28/30] exclude 0 when changing revoke_after_days property in the admin --- impresso/admin.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/impresso/admin.py b/impresso/admin.py index 3f9ba71..15c5bf8 100644 --- a/impresso/admin.py +++ b/impresso/admin.py @@ -134,9 +134,9 @@ def clean_metadata(self) -> dict: raise ValidationError( "metadata.revokeAfterDays must be an integer or float." ) - if revoke_after_days < 0: + if revoke_after_days <= 0: raise ValidationError( - "metadata.revokeAfterDays must be a non-negative integer or float." + "metadata.revokeAfterDays must be a positive integer or float." ) return metadata From 22bf338770fd5f47f696fdda3592c96ee66f66ec Mon Sep 17 00:00:00 2001 From: Daniele Guido <1181642+danieleguido@users.noreply.github.com> Date: Thu, 7 May 2026 14:36:53 +0200 Subject: [PATCH 29/30] Create 0059_alter_collection_serialized_search_query_and_more.py --- ...ection_serialized_search_query_and_more.py | 23 +++++++++++++++++++ 1 file changed, 23 insertions(+) create mode 100644 impresso/migrations/0059_alter_collection_serialized_search_query_and_more.py diff --git a/impresso/migrations/0059_alter_collection_serialized_search_query_and_more.py b/impresso/migrations/0059_alter_collection_serialized_search_query_and_more.py new file mode 100644 index 0000000..9a75531 --- /dev/null +++ b/impresso/migrations/0059_alter_collection_serialized_search_query_and_more.py @@ -0,0 +1,23 @@ +# Generated by Django 5.2.10 on 2026-05-07 12:23 + +from django.db import migrations, models + + +class Migration(migrations.Migration): + + dependencies = [ + ('impresso', '0058_remove_userrequest_subscription_and_more'), + ] + + operations = [ + migrations.AlterField( + model_name='collection', + name='serialized_search_query', + field=models.TextField(blank=True, help_text='Initial search query that generated the collection.', null=True), + ), + migrations.AlterField( + model_name='searchquery', + name='hash', + field=models.TextField(blank=True, null=True), + ), + ] From ca97983d9bdc570a0838fa24786f9f182fa0a0fd Mon Sep 17 00:00:00 2001 From: Daniele Guido <1181642+danieleguido@users.noreply.github.com> Date: Thu, 7 May 2026 15:04:16 +0200 Subject: [PATCH 30/30] refine admin --- impresso/admin.py | 56 ++++++++++++++++++--- impresso/models/specialMembershipDataset.py | 27 ++++++++-- 2 files changed, 73 insertions(+), 10 deletions(-) diff --git a/impresso/admin.py b/impresso/admin.py index 15c5bf8..d740577 100644 --- a/impresso/admin.py +++ b/impresso/admin.py @@ -7,7 +7,7 @@ from django.contrib.auth.models import User from django.utils.translation import ngettext from django.utils import timezone - +from django.utils.html import format_html from impresso.models.userBitmapSubscription import UserBitmapSubscription from .models import Issue, Job, Page, Newspaper from .models import SearchQuery, ContentItem @@ -87,20 +87,23 @@ def set_terms_accepted_date(self, request, queryset): class SpecialMembershipDatasetAdminForm(forms.ModelForm): + class Meta: model = SpecialMembershipDataset fields = "__all__" + help_texts = { + "metadata": format_html( + "Allowed Values: 
        {}

        Note: revokeAfterDays is working only if enableTemporaryAutomaticAcceptance is set to true.", + ", ".join(sorted(SpecialMembershipDataset.METADATA_ALLOWED_KEYS)), + ), + } def clean_metadata(self) -> dict: metadata = self.cleaned_data.get("metadata") or {} if not isinstance(metadata, dict): raise ValidationError("Metadata must be a JSON object.") - allowed_keys = { - "modality", - "enableTemporaryAutomaticAcceptance", - "revokeAfterDays", - } + allowed_keys = SpecialMembershipDataset.METADATA_ALLOWED_KEYS extra_keys = set(metadata.keys()) - allowed_keys if extra_keys: raise ValidationError( @@ -143,12 +146,51 @@ def clean_metadata(self) -> dict: @admin.register(SpecialMembershipDataset) class SpecialMembershipDatasetAdmin(ModelAdmin): - list_display = ("title", "bitmap_position", "reviewer", "metadata") + list_display = ( + "title", + "bitmap_position", + "reviewer", + "modality", + "enable_temporary_automatic_acceptance", + "revoke_after_days_display", + ) search_fields = ["title", "reviewer__username", "reviewer__email"] readonly_fields = ("bitmap_position",) autocomplete_fields = ["reviewer"] form = SpecialMembershipDatasetAdminForm + @admin.display(description="Revoke after") + def revoke_after_days_display(self, obj: SpecialMembershipDataset) -> str: + days = obj.revoke_after_days + if days is None: + return "-" + total_minutes = round(days * 24 * 60) + parts = [] + years, remainder = divmod(total_minutes, 365 * 24 * 60) + if years: + parts.append(f"{years}y") + months, remainder = divmod(remainder, 30 * 24 * 60) + if months: + parts.append(f"{months}mo") + weeks, remainder = divmod(remainder, 7 * 24 * 60) + if weeks: + parts.append(f"{weeks}w") + day_part, remainder = divmod(remainder, 24 * 60) + if day_part: + parts.append(f"{day_part}d") + hours, minutes = divmod(remainder, 60) + if hours: + parts.append(f"{hours}h") + if minutes: + parts.append(f"{minutes}m") + return format_html( + ( + f"{obj.revoke_after_days} days
        " + " ".join(parts) + if parts + else "0m" + ), + ) + @admin.register(Issue) class IssueAdmin(ModelAdmin): diff --git a/impresso/models/specialMembershipDataset.py b/impresso/models/specialMembershipDataset.py index b1596c7..f8becc3 100644 --- a/impresso/models/specialMembershipDataset.py +++ b/impresso/models/specialMembershipDataset.py @@ -44,6 +44,7 @@ class SpecialMembershipDataset(models.Model): blank=True, ) metadata: Metadata = models.JSONField(default=dict, blank=True) + reviewer = models.ForeignKey( "auth.User", on_delete=models.SET_NULL, @@ -52,15 +53,35 @@ class SpecialMembershipDataset(models.Model): blank=True, ) + METADATA_ALLOWED_KEYS = { + "modality", + "enableTemporaryAutomaticAcceptance", + "revokeAfterDays", + } + def __str__(self): return self.title + @property + def modality(self) -> Optional[str]: + return self.metadata.get("modality") + + @property + def enable_temporary_automatic_acceptance(self) -> Optional[bool]: + value = self.metadata.get("enableTemporaryAutomaticAcceptance") + return bool(value) if value is not None else None + + @property + def revoke_after_days(self) -> Optional[float]: + value = self.metadata.get("revokeAfterDays") + return float(value) if value is not None else None + def is_temporary_auto_accept_enabled(self) -> bool: - return bool(self.metadata.get("enableTemporaryAutomaticAcceptance")) + return bool(self.enable_temporary_automatic_acceptance) def is_modality_cc_reviewer_enabled(self) -> bool: - return bool( - self.metadata.get("modality") + return ( + self.modality == settings.IMPRESSO_EMAIL_MODALITY_SPECIAL_MEMBERSHIP_REQUEST_CC_REVIEWER )