[ENG-10338] Registrations not becoming public when embargo ends

admin/nodes/views.py

@@ -6,7 +6,7 @@
 from django.contrib import messages
 from django.contrib.auth.mixins import PermissionRequiredMixin
 from django.core.exceptions import PermissionDenied, ValidationError
-from django.db.models import F, Case, When, IntegerField
+from django.db.models import F, Case, When, IntegerField, Prefetch
 from django.http import HttpResponse
 from django.shortcuts import redirect, reverse, get_object_or_404
 from django.urls import NoReverseMatch
@@ -487,42 +487,50 @@ class EmbargoReportView(PermissionRequiredMixin, TemplateView):
     - active embargoes that are past their end date
     - upcoming active embargoes
     """
+
     template_name = 'nodes/embargo_report.html'
     permission_required = 'osf.view_registration'
     raise_exception = True
 
-    def get_context_data(self, **kwargs):
-        context = super().get_context_data(**kwargs)
-        pending_embargoes = Embargo.objects.pending_embargoes().select_related('initiated_by')
-        active_embargoes = Embargo.objects.active_embargoes().select_related('initiated_by')
-
-        pending_overdue_embargoes = [
-            embargo for embargo in pending_embargoes
-            if embargo.should_be_embargoed
-        ]
-
-        overdue_embargoes = [
-            embargo for embargo in active_embargoes
-            if embargo.should_be_completed
-        ]
-
-        upcoming_queryset = active_embargoes.filter(
-            end_date__gte=timezone.now(),
-        ).order_by('end_date')
+    def _embargo_report_queryset(self, queryset):
+        return queryset.select_related('initiated_by').prefetch_related(
+            Prefetch(
+                'registrations',
+                queryset=Registration.objects.filter(is_deleted=False).prefetch_related(
+                    'guids',
+                ).only('id', 'title', 'is_public', 'embargo_id'),
+            ),
+        )
 
-        page_number = self.request.GET.get('page') or 1
-        paginator = Paginator(upcoming_queryset, 10)
+    def paginate_embargo_report(self, request, queryset, page_param):
+        paginator = Paginator(queryset, settings.EMBARGO_REPORT_PAGE_SIZE)
+        page_number = request.GET.get(page_param) or 1
         try:
-            upcoming_page = paginator.page(page_number)
+            return paginator.page(page_number)
         except InvalidPage:
-            upcoming_page = paginator.page(1)
+            return paginator.page(1)
+
+    def get_context_data(self, **kwargs):
+        context = super().get_context_data(**kwargs)
+        request = self.request
 
         context.update({
             'now': timezone.now(),
-            'pending_overdue_embargoes': pending_overdue_embargoes,
-            'overdue_embargoes': overdue_embargoes,
-            'upcoming_embargoes': upcoming_page.object_list,
-            'upcoming_page': upcoming_page,
+            'upcoming_page': self.paginate_embargo_report(
+                request,
+                self._embargo_report_queryset(Embargo.objects.active_upcoming()),
+                'upcoming_page',
+            ),
+            'pending_page': self.paginate_embargo_report(
+                request,
+                self._embargo_report_queryset(Embargo.objects.pending_past_activation_window()),
+                'pending_page',
+            ),
+            'overdue_page': self.paginate_embargo_report(
+                request,
+                self._embargo_report_queryset(Embargo.objects.active_past_end_date()),
+                'overdue_page',
+            ),
         })
         return context
 

admin/templates/nodes/embargo_report.html

@@ -18,10 +18,12 @@ <h2>Upcoming Active Embargoes</h2>
             <th>Initiated By</th>
         </tr>
     </thead>
-    {% include "util/pagination.html" with items=upcoming_page status='' pagin=False order='' %}
+    {% if upcoming_page.paginator.num_pages > 1 %}
+        {% include "util/pagination.html" with items=upcoming_page page_param="upcoming_page" status='' pagin=False order='' %}
+    {% endif %}
     <tbody>
-        {% for embargo in upcoming_embargoes %}
-            {% with registration=embargo.registrations.first %}
+        {% for embargo in upcoming_page %}
+            {% with registration=embargo.registrations.all.0 %}
                 {% if registration %}
                 <tr>
                     <td>
@@ -66,9 +68,12 @@ <h2>Pending Embargoes Past Pending Window</h2>
             <th>Initiated By</th>
         </tr>
     </thead>
+    {% if pending_page.paginator.num_pages > 1 %}
+        {% include "util/pagination.html" with items=pending_page page_param="pending_page" status='' pagin=False order='' %}
+    {% endif %}
     <tbody>
-        {% for embargo in pending_overdue_embargoes %}
-            {% with registration=embargo.registrations.first %}
+        {% for embargo in pending_page %}
+            {% with registration=embargo.registrations.all.0 %}
                 {% if registration %}
                 <tr>
                     <td>
@@ -113,9 +118,12 @@ <h2>Active Embargoes Past Pending Window</h2>
             <th>Initiated By</th>
         </tr>
     </thead>
+    {% if overdue_page.paginator.num_pages > 1 %}
+        {% include "util/pagination.html" with items=overdue_page page_param="overdue_page" status='' pagin=False order='' %}
+    {% endif %}
     <tbody>
-        {% for embargo in overdue_embargoes %}
-            {% with registration=embargo.registrations.first %}
+        {% for embargo in overdue_page %}
+            {% with registration=embargo.registrations.all.0 %}
                 {% if registration %}
                 <tr>
                     <td>
@@ -154,4 +162,3 @@ <h2>Active Embargoes Past Pending Window</h2>
 </table>
 
 {% endblock %}
-

admin/templates/util/pagination.html

@@ -3,11 +3,11 @@
     <div class="pagination pagination-lg">
         <span>
             {% if items.has_previous %}
-                <a href="?page=1&amp;status={{ status }}&amp;p={{ pagin }}&amp;order_by={{ order }}{{ extra_query_params }}"
+                <a href="?{{ page_param|default:"page" }}=1&amp;status={{ status }}&amp;p={{ pagin }}&amp;order_by={{ order }}{{ extra_query_params }}"
                    class="btn btn-primary">
                     |
                 </a>
-                <a href="?page={{ items.previous_page_number }}&amp;status={{ status }}&amp;p={{ pagin }}&amp;order_by={{ order }}{{ extra_query_params }}"
+                <a href="?{{ page_param|default:"page" }}={{ items.previous_page_number }}&amp;status={{ status }}&amp;p={{ pagin }}&amp;order_by={{ order }}{{ extra_query_params }}"
                    class="btn btn-primary">
                     <i class="fa fa-angle-left"></i>
                 </a>
@@ -25,11 +25,11 @@
             </span>
 
             {% if items.has_next %}
-                <a href="?page={{ items.next_page_number }}&amp;status={{ status }}&amp;p={{ pagin }}&amp;order_by={{ order }}{{ extra_query_params }}"
+                <a href="?{{ page_param|default:"page" }}={{ items.next_page_number }}&amp;status={{ status }}&amp;p={{ pagin }}&amp;order_by={{ order }}{{ extra_query_params }}"
                    class="btn btn-primary">
                     <i class="fa fa-angle-right"></i>
                 </a>
-                <a href="?page={{ items.paginator.num_pages }}&amp;status={{ status }}&amp;p={{ pagin }}&amp;order_by={{ order }}{{ extra_query_params }}"
+                <a href="?{{ page_param|default:"page" }}={{ items.paginator.num_pages }}&amp;status={{ status }}&amp;p={{ pagin }}&amp;order_by={{ order }}{{ extra_query_params }}"
                    class="btn btn-primary">
                     |
                 </a>

admin_tests/nodes/test_views.py

@@ -18,6 +18,7 @@
     AbstractNode,
     RegistrationApproval,
     Embargo,
+    Sanction,
     SchemaResponse,
     DraftRegistration,
 )
@@ -39,7 +40,8 @@
     CheckArchiveStatusRegistrationsView,
     ForceArchiveRegistrationsView,
     ApprovalBacklogListView,
-    ConfirmApproveBacklogView
+    ConfirmApproveBacklogView,
+    EmbargoReportView,
 )
 from admin_tests.utilities import setup_log_view, setup_view, handle_post_view_request
 from api_tests.share._utils import mock_update_share
@@ -57,14 +59,15 @@
     RegistrationApprovalFactory,
     RegistrationProviderFactory,
     DraftRegistrationFactory,
+    EmbargoFactory,
     get_default_metaschema
 )
 from osf.utils.workflows import ApprovalStates, RegistrationModerationStates
 from osf.utils import permissions
 from osf.exceptions import NodeStateError
 
 
-from website.settings import REGISTRATION_APPROVAL_TIME
+from website.settings import REGISTRATION_APPROVAL_TIME, EMBARGO_PENDING_TIME
 
 
 def patch_messages(request):
@@ -1069,3 +1072,63 @@ def test_file_is_removed_from_registration_osfstorage(self):
             name=registration_osfstorage.archive_folder_name
         ).children.exists()
         assert not self.registration_registered_from.files.exists()
+
+
+class TestEmbargoReportView(AdminTestCase):
+
+    def setUp(self):
+        super().setUp()
+        self.request = RequestFactory().get('/nodes/embargo_report/')
+        self.view = setup_log_view(EmbargoReportView(), self.request)
+
+    def test_pending_past_activation_window_in_report(self):
+        embargo = EmbargoFactory(approve=False)
+        embargo.initiation_date = timezone.now() - EMBARGO_PENDING_TIME - timezone.timedelta(days=1)
+        embargo.save()
+
+        context = self.view.get_context_data()
+        assert embargo in context['pending_page']
+
+    def test_recent_pending_embargo_excluded(self):
+        embargo = EmbargoFactory(approve=False)
+        embargo.initiation_date = timezone.now()
+        embargo.save()
+
+        context = self.view.get_context_data()
+        assert embargo not in context['pending_page']
+
+    def test_active_past_end_date_in_report(self):
+        embargo = EmbargoFactory(
+            approve=True,
+            end_date=timezone.now() - timezone.timedelta(days=1),
+        )
+        embargo.state = Sanction.APPROVED
+        embargo.save()
+
+        context = self.view.get_context_data()
+        assert embargo in context['overdue_page']
+
+    def test_active_upcoming_in_report(self):
+        embargo = EmbargoFactory(
+            approve=True,
+            end_date=timezone.now() + timezone.timedelta(days=30),
+        )
+        embargo.state = Sanction.APPROVED
+        embargo.save()
+
+        context = self.view.get_context_data()
+        assert embargo in context['upcoming_page']
+
+    def test_deleted_registration_embargo_excluded(self):
+        embargo = EmbargoFactory(
+            approve=True,
+            end_date=timezone.now() - timezone.timedelta(days=1),
+        )
+        embargo.state = Sanction.APPROVED
+        embargo.save()
+        registration = embargo.registrations.first()
+        registration.is_deleted = True
+        registration.save()
+
+        context = self.view.get_context_data()
+        assert embargo not in context['overdue_page']

osf/models/sanctions.py

@@ -456,15 +456,60 @@ def _email_template_context(self, user, node, is_authorizer=False, urls=None):
         return {}
 
 
+class EmbargoQuerySet(models.QuerySet):
+
+    def has_non_deleted_registrations(self):
+        """Keep embargoes linked to at least one non-deleted registration.
+
+        Excludes orphaned embargoes (no registration) and embargoes whose only
+        linked registration(s) are soft-deleted. Matches Embargo.is_deleted.
+        """
+        return self.filter(registrations__is_deleted=False).distinct()
+
+
 class EmbargoManager(models.Manager):
 
-    def pending_embargoes(self):
+    def get_queryset(self):
+        return EmbargoQuerySet(self.model, using=self._db)
+
+    def pending_embargoes(self, exclude_deleted=False):
         """Embargoes that are still awaiting admin approval."""
-        return self.filter(state=self.model.UNAPPROVED)
+        queryset = self.filter(state=self.model.UNAPPROVED)
+        if exclude_deleted:
+            queryset = queryset.has_non_deleted_registrations()
+        return queryset
 
-    def active_embargoes(self):
+    def active_embargoes(self, exclude_deleted=False):
         """Embargoes that have been approved and are currently in effect."""
-        return self.filter(state=self.model.APPROVED)
+        queryset = self.filter(state=self.model.APPROVED)
+        if exclude_deleted:
+            queryset = queryset.has_non_deleted_registrations()
+        return queryset
+
+    def pending_past_activation_window(self):
+        """Pending embargoes that should have been activated (matches should_be_embargoed)."""
+        cutoff = timezone.now() - osf_settings.EMBARGO_PENDING_TIME
+        return (
+            self.pending_embargoes(exclude_deleted=True)
+            .filter(initiation_date__lte=cutoff)
+            .order_by('initiation_date')
+        )
+
+    def active_past_end_date(self):
+        """Active embargoes past end date (matches should_be_completed)."""
+        return (
+            self.active_embargoes(exclude_deleted=True)
+            .filter(end_date__lt=timezone.now())
+            .order_by('end_date')
+        )
+
+    def active_upcoming(self):
+        """Active embargoes with a future end date."""
+        return (
+            self.active_embargoes(exclude_deleted=True)
+            .filter(end_date__gte=timezone.now())
+            .order_by('end_date')
+        )
 
 
 class Embargo(SanctionCallbackMixin, EmailApprovableSanction):

website/settings/defaults.py

@@ -74,6 +74,7 @@ def parent_dir(path):
 # Date range for embargo periods
 EMBARGO_END_DATE_MIN = datetime.timedelta(days=2)
 EMBARGO_END_DATE_MAX = datetime.timedelta(days=1460)  # Four years
+EMBARGO_REPORT_PAGE_SIZE = 10
 
 # Question titles to be reomved for anonymized VOL
 ANONYMIZED_TITLES = ['Authors']