Require valid reference order for submission/publication (#2300)

Many existing projects have an invalid order of entries in their
References section, due to issue #2137. "Invalid" means:

(a) One or more references have `order=None`, typically because it was
copied from a past published version.

(b) Two references have the same `order` value, typically because a
reference was deleted from the middle of the list and then a new one was
added.

(c) `order` is distinct but doesn't match `id` order, typically because
*two* references were deleted from the middle of the list and then a new
one was added.

Whatever the reason for having an invalid order of references, it is
*not feasible to fix the order automatically*, so we want to require the
author/editor to review and fix it by hand before the project is
published.

The demo project "MIMIC-III Clinical Database" currently has
`order=None` for its three references. With these changes, you should
see:

- If you open the preview page, there is an error message "Order of
references may be incorrect".

- If you open the Project Content page, there is a warning at the bottom
of the page.

- If you add or edit references, and click "Save Description" (without
checking the checkbox), the references should be saved but the warning
should still be present.

- If you check the checkbox and then click "Save Description", the
warning should go away.

- Alternatively, if you *delete* all three of the original references
and then refresh the page, the warning should go away.

physionet-django/console/templates/console/copyedit_submission.html

@@ -54,6 +54,7 @@ <h3>Description</h3>
           {% include "project/content_inline_form_snippet.html" with form=description_form %}
           {% include "project/content_inline_form_snippet.html" with form=ethics_form %}
           {% include 'project/item_list.html' with item="reference" item_label=reference_formset.item_label formset=reference_formset form_name=reference_formset.form_name add_item_url=add_item_url remove_item_url=remove_item_url %}
+          {% include "project/confirm_reference_order_form.html" %}
           <h3>Access</h3>
           <div id="access">
             {% include "project/content_inline_form_snippet.html" with form=access_form %}

physionet-django/project/forms.py

@@ -732,11 +732,20 @@ class ReferenceFormSet(BaseGenericInlineFormSet):
     item_label = 'References'
     max_forms = 50
 
-    def __init__(self, *args, **kwargs):
-        super().__init__(*args, **kwargs)
+    def __init__(self, data=None, *args, **kwargs):
+        super().__init__(data, *args, **kwargs)
         self.max_forms = ReferenceFormSet.max_forms
         self.help_text = 'Numbered references specified in the metadata. Article citations must be in <a href=http://www.bibme.org/citation-guide/apa/ target=_blank>APA</a> format. Maximum of {}.'.format(self.max_forms)
 
+        # If user checked the "confirm reference order" box then they
+        # are confirming the order of references (as displayed in the
+        # form) is correct.  If user did not check that box, then do
+        # not touch the existing order.
+        if data and data.get('confirm_reference_order') == '1':
+            self.confirm_reference_order = True
+        else:
+            self.confirm_reference_order = False
+
     def clean(self):
         """
         - Check max forms due to POST refresh issue
@@ -760,9 +769,26 @@ def clean(self):
                 descriptions.append(description)
 
     def save(self, *args, **kwargs):
-        # change the value of order. set it as index of form
-        for form in self.forms:
-            form.instance.order = self.forms.index(form) + 1
+        if self.confirm_reference_order:
+            # If "confirm reference order" was checked, set the order
+            # of all references in the formset.
+            for form in self.forms:
+                form.instance.order = self.forms.index(form) + 1
+                form.changed_data = True
+        else:
+            # If "confirm reference order" was not checked, then set
+            # the order only for newly created references, leaving
+            # existing references alone.  New references should have
+            # "order" greater than any existing reference.
+            max_order = 0
+            for form in self.forms:
+                if form.instance.order is not None:
+                    max_order = max(max_order, form.instance.order)
+            for form in self.forms:
+                if form.instance.pk is None:
+                    form.instance.order = max_order + 1
+                    max_order += 1
+
         super().save(*args, **kwargs)
 
 

physionet-django/project/modelcomponents/activeproject.py

@@ -349,6 +349,10 @@ def check_integrity(self):
                 l = self.LABELS[self.resource_type.id][attr] if attr in self.LABELS[self.resource_type.id] else attr.title().replace('_', ' ')
                 self.integrity_errors.append('Missing required field: {0}'.format(l))
 
+        # References
+        if not self.has_valid_reference_order():
+            self.integrity_errors.append('Order of references may be incorrect')
+
         published_projects = self.core_project.publishedprojects.all()
         if published_projects:
             published_versions = [p.version for p in published_projects]
@@ -370,6 +374,30 @@ def check_integrity(self):
         else:
             return True
 
+    def has_valid_reference_order(self):
+        """
+        Check whether order of references is valid.
+
+        Past bugs in the project editing forms can result in
+        references having 'order' set to None, or the order of 'order'
+        not matching the order displayed in the content/copyedit page.
+        It is impractical to repair all existing projects
+        automatically since that requires guessing the author's
+        intent.
+
+        Therefore, if a project's reference order is undefined or
+        inconsistent, we want to require the author or editor to
+        address it before the project can be submitted or published.
+        """
+        references = self.references.order_by('id')
+        order_list = [r.order for r in references]
+
+        for order1, order2 in zip(order_list, order_list[1:]):
+            if order1 is None or order2 is None or order1 >= order2:
+                return False
+
+        return True
+
     def is_submittable(self):
         """
         Whether the project can be submitted

physionet-django/project/templates/project/confirm_reference_order_form.html

@@ -0,0 +1,28 @@
+{% comment %}
+Past bugs in the project editing forms can result in references having
+'order' set to None, or the order of 'order' not matching the order
+displayed in the content/copyedit page.  It is impractical to repair
+all existing projects automatically since that requires guessing the
+author's intent.
+
+Therefore, if a project's reference order is undefined or
+inconsistent, the message below should be displayed on the content
+page (when the project is author-editable) or copyedit page (when the
+project is copyeditable.)
+
+The "confirm_reference_order" checkbox will be handled by
+ReferenceFormSet (see project/forms.py).
+{% endcomment %}
+{% if not project.has_valid_reference_order %}
+<div class="alert alert-form alert-warning">
+  <div>
+    The References list may be incorrect due to a server error.
+    Please verify that the list shown above corresponds to the
+    correct numbered citations in the project text.
+  </div>
+  <label>
+    <input type="checkbox" name="confirm_reference_order" value="1">
+    Order of references is correct
+  </label>
+</div>
+{% endif %}

physionet-django/project/templates/project/project_content.html

@@ -17,7 +17,7 @@ <h2 class="form-signin-heading">2. Project Content</h2>
 <p>Please adhere to the standards specified in the helpstrings. Required fields are indicated by a <a style="color:red">*</a>.</p>
 <hr>
 
-<form action="{% url 'project_content' project.slug %}" onsubmit="return validateItems('reference-list', 'description', 'References')" method="post" class="no-pd">
+<form action="{% url 'project_content' project.slug %}" onsubmit="return validateItems('reference-list', 'description', 'References')" method="post">
   {% if not project.author_editable %}
     <div class="alert alert-form alert-warning alert-dismissible">
       <strong>The project cannot be edited right now.</strong>
@@ -32,6 +32,7 @@ <h2 class="form-signin-heading">2. Project Content</h2>
   {% include "project/content_inline_form_snippet.html" with form=description_form %}
   {% include 'project/item_list.html' with item="reference" item_label=reference_formset.item_label formset=reference_formset form_name=reference_formset.form_name add_item_url=add_item_url remove_item_url=remove_item_url %}
   {% if is_submitting and project.author_editable %}
+  {% include "project/confirm_reference_order_form.html" %}
   <hr>
   <button class="btn btn-primary btn-rsp btn-left" type="submit" name="edit_description">Save Description</button>
   {% endif %}

physionet-django/project/test_views.py

@@ -1,5 +1,6 @@
 
 import base64
+import html.parser
 import os
 from http import HTTPStatus
 import json
@@ -41,6 +42,26 @@ def _basic_auth(username, password, encoding='UTF-8'):
     return 'Basic ' + token
 
 
+def _parse_html_form_fields(content):
+    """
+    Parse HTML and return all form fields as a dictionary.
+
+    Note that this currently only handles input elements, not other
+    form elements such as select or textarea.  It also makes no
+    distinction between multiple forms.
+    """
+    fields = {}
+
+    class Parser(html.parser.HTMLParser):
+        def handle_starttag(self, tag, attrs):
+            attrs = dict(attrs)
+            if tag == 'input' and 'name' in attrs:
+                fields[attrs['name']] = attrs.get('value', '')
+
+    Parser().feed(content)
+    return fields
+
+
 class TestAccessPresubmission(TestMixin):
     """
     Test that certain views or content in their various states can only
@@ -558,6 +579,55 @@ def test_content(self):
         project.refresh_from_db()
         self.assertFalse(project.is_submittable())
 
+    def test_reference_order(self):
+        """
+        Test handling of references with invalid order.
+        """
+        self.client.login(username=self.AUTHOR, password=self.PASSWORD)
+
+        project = ActiveProject.objects.get(title=self.PROJECT_TITLE)
+        self.assertEqual(project.references.count(), 0)
+        self.assertTrue(project.is_submittable())
+
+        # References with distinct order values are okay.
+        ref1 = project.references.create(description="asdf", order=1)
+        ref2 = project.references.create(description="ghjk", order=2)
+        self.assertTrue(project.is_submittable())
+
+        # Same order value for two references is an error.
+        ref1.order = 2
+        ref1.save()
+        self.assertFalse(project.is_submittable())
+
+        # Order value of None is an error.
+        ref2.order = None
+        ref2.save()
+        self.assertFalse(project.is_submittable())
+
+        content_url = reverse('project_content', args=(project.slug,))
+        response = self.client.get(content_url)
+        data = _parse_html_form_fields(response.content.decode())
+
+        # Try submitting form without confirm_reference_order.
+        # Existing order values should be unchanged.
+        data.pop('confirm_reference_order', None)
+        response = self.client.post(content_url, data=data)
+        ref1.refresh_from_db()
+        self.assertEqual(ref1.order, 2)
+        ref2.refresh_from_db()
+        self.assertEqual(ref2.order, None)
+        self.assertFalse(project.is_submittable())
+
+        # Try submitting form with confirm_reference_order.
+        # Order values should be unique.
+        data['confirm_reference_order'] = '1'
+        response = self.client.post(content_url, data=data)
+        ref1.refresh_from_db()
+        self.assertEqual(ref1.order, 1)
+        ref2.refresh_from_db()
+        self.assertEqual(ref2.order, 2)
+        self.assertTrue(project.is_submittable())
+
 
 class TestProjectTransfer(TestCase):
     """

physionet-django/static/custom/css/physionet.css

@@ -98,7 +98,7 @@ th, td {
   color: white;
 }
 
-.navbar-check{
+input[type=checkbox].navbar-check{
   display: none;
 }
 

physionet-django/static/custom/css/settings.css

@@ -29,6 +29,9 @@ input, select, django-ckeditor-widget, textarea {
   border-radius: 0.25rem;
   transition: border-color ease-in-out 0.15s, box-shadow ease-in-out 0.15s;
 }
+input[type=checkbox], input[type=radio] {
+  display: initial;
+}
 
 input::-ms-expand {
   background-color: transparent;