Formset fixes

core/forms.py

@@ -198,13 +198,22 @@ def clean(self):
         self.additional_documentation_items_formset.clean()
         self.schema_refs_formset.clean()
         cleaned_data = super().clean()
+        # Make sure none of the schema refs have the same URL
+        schema_ref_urls = set()
+        for schema_ref_form in self.schema_refs_formset:
+            schema_ref_urls.add(schema_ref_form.cleaned_data.get('url'))
+        if len(schema_ref_urls) < len(self.schema_refs_formset):
+            raise ValidationError('Each schema definition URL must be unique')
 
         return cleaned_data
 
     def is_valid(self):
-        is_form_valid = super().is_valid()
         is_documentation_items_formset_valid = self.additional_documentation_items_formset.is_valid()
         is_schema_refs_formset_valid = self.schema_refs_formset.is_valid()
+        # This must be called after the two above,
+        # as the clean method requires access
+        # to formset cleaned_data
+        is_form_valid = super().is_valid()
         return is_form_valid and is_documentation_items_formset_valid and is_schema_refs_formset_valid
 
 
@@ -280,8 +289,12 @@ def clean(self):
             schema_ref_slug = schema_ref_form.cleaned_data.get('slug')
             if schema_ref_slug in slugs:
                 raise ValidationError('Each URL must be unique.')
+            if schema_ref_slug:
+                slugs.add(schema_ref_slug)
 
         return cleaned_data
 
     def is_valid(self):
+        # This order is important, as self.clean()
+        # requires access to the formset's cleaned_data
         return self.schema_ref_permanent_url_formset.is_valid() and super().is_valid()

core/static/js/site.js

@@ -22,7 +22,7 @@
   };
 
   /** @param {HTMLElement} formsetElement */
-  const attachFormsetControlHandlers = (formsetElement) => {
+  const initializeFormsetElement = (formsetElement) => {
     Array.from(
       formsetElement.querySelectorAll('[data-formset-close-trigger]')
     ).forEach((element) => {
@@ -41,6 +41,13 @@
         formsetElement.classList.add('formset--collapsed');
       });
     });
+    Array.from(
+      formsetElement.querySelectorAll('[data-url-format-selector-for]')
+    )
+      .filter((element) => element instanceof HTMLSelectElement)
+      .forEach((formatSelectElement) => {
+        initializeFormatSelectElement(formatSelectElement);
+      });
   };
 
   /**
@@ -87,7 +94,7 @@
           formsetListElement.getElementsByClassName('formset');
         totalFormInput.value = formsetElements.length.toString();
         // If the number of formset items changed,
-        // insert the new count for each.
+        // update any index-based values
         if (
           (mutation.addedNodes && mutation.addedNodes.length) ||
           (mutation.removedNodes && mutation.removedNodes.length)
@@ -101,21 +108,16 @@
                   countElement.innerText = count;
                 }
               });
+            // Rename any attributes with the current formset index
+            formsetElement.innerHTML = formsetElement.innerHTML.replace(
+              new RegExp(`${formsetListId}-[\\d+]`, 'g'),
+              `${formsetListId}-${index}`
+            );
+            if (formsetElement instanceof HTMLElement) {
+              initializeFormsetElement(formsetElement);
+            }
           });
         }
-        // If nodes were not added, we're done.
-        // Otherwise, wire up the handlers and insert the correct count.
-        if (!mutation.addedNodes || !mutation.addedNodes.length) {
-          return;
-        }
-        Array.from(mutation.addedNodes).forEach((node) => {
-          if (
-            node instanceof HTMLElement &&
-            node.classList.contains('formset')
-          ) {
-            attachFormsetControlHandlers(node);
-          }
-        });
       });
     });
     mutationObserver.observe(formsetListElement, { childList: true });
@@ -235,7 +237,7 @@
           formsetListElement.getElementsByClassName('formset')
         ).forEach((formsetElement) => {
           if (formsetElement instanceof HTMLElement) {
-            attachFormsetControlHandlers(formsetElement);
+            initializeFormsetElement(formsetElement);
           }
         });
       }
@@ -270,16 +272,6 @@
           currentFormItemCount.toString()
         );
         formsetListElement.insertAdjacentHTML('beforeend', nextFormItemHtml);
-        const formsetItems = formsetListElement.querySelectorAll('.formset');
-        Array.from(
-          formsetItems[formsetItems.length - 1].querySelectorAll(
-            '[data-url-format-selector-for]'
-          )
-        )
-          .filter((element) => element instanceof HTMLSelectElement)
-          .forEach((formatSelectElement) => {
-            initializeFormatSelectElement(formatSelectElement);
-          });
       });
     });
 

core/templates/core/manage/documentation_item_form.html

@@ -10,6 +10,7 @@
   <div>
     Documentation Item <span data-formset-count>{{ count }}</span>
   </div>
+  {{ form.id }}
   <div class="split-field-container split-field-container--67-33 formset-content--collapsible">
     {% include "core/manage/field.html" with field=form.name %}
     {% include "core/manage/field.html" with field=form.role %}

core/templates/core/manage/schema_ref_form.html

@@ -10,6 +10,7 @@
   <div>
     Schema <span data-formset-count>{{ count }}</span>
   </div>
+  {{ form.id }}
   <div class="split-field-container split-field-container--50-50 formset-content--collapsible">
     {% include "core/manage/field.html" with field=form.name %}
     {% include "core/manage/field.html" with field=form.url %}

core/views.py

@@ -224,7 +224,7 @@ def manage_schema(request, schema_id=None):
             previous_documentation_items = schema.documentationitem_set.exclude(role__in=[
                 DocumentationItem.DocumentationItemRole.README,
                 DocumentationItem.DocumentationItemRole.License
-            ])
+            ]).all()
             previous_documentation_items_by_id = {
                 item.id: item for item in previous_documentation_items
             }

tests/factories.py

@@ -95,7 +95,7 @@ class DocumentationItemFactory(ReferenceItemFactory):
     class Meta:
         model = DocumentationItem
 
-    name = factory.Faker('words', nb=3)
+    name = factory.Faker('catch_phrase')
     description = factory.Faker('paragraph')
     schema = factory.SubFactory(SchemaFactory)
     role = factory.Iterator(DocumentationItem.DocumentationItemRole.values)

tests/test_forms.py

@@ -9,7 +9,8 @@
     SchemaRefFactory,
     OrganizationSchemaRefFactory,
     OrganizationSchemaFactory,
-    PermanentURLFactory
+    PermanentURLFactory,
+    DocumentationItemFactory
 )
 from core.models import DocumentationItem
 
@@ -94,6 +95,29 @@ def test_schema_management_form_requires_one_schema_ref():
         assert error == 'A schema must have at least one definition'
 
 
+@pytest.mark.django_db
+def test_schema_management_form_prevents_duplicate_schema_ref_urls():
+    spec_url = 'http://example.com/schema.json'
+    with requests_mock.Mocker() as m:
+        m.get('http://example.com', text='{}')
+        m.get(spec_url, text='{}')
+        form = SchemaForm(data={
+            'name': 'New schema',
+            'readme_url': 'http://example.com',
+            'name': 'New schema',
+            'schema_refs-0-url': spec_url,
+            'schema_refs-1-url': spec_url,
+            'readme_url': 'http://example.com',
+            'documentation_items-TOTAL_FORMS': 0,
+            'documentation_items-INITIAL_FORMS': 0,
+            'schema_refs-TOTAL_FORMS': 2,
+            'schema_refs-INITIAL_FORMS': 0
+        })
+        assert not form.is_valid()
+        error = form.non_field_errors()[0]
+        assert error == 'Each schema definition URL must be unique'
+
+
 @pytest.mark.django_db
 @pytest.mark.parametrize(
     "attempted_slug_field_name",
@@ -138,3 +162,24 @@ def test_schema_permanent_url_form_prevents_new_duplicate_urls():
     error = form.non_field_errors()[0]
     assert error == 'Each URL must be unique.'
 
+
+@pytest.mark.django_db
+def test_schema_permanent_url_form_prevents_new_duplicate_schema_ref_urls():
+    managed_schema_ref_1 = OrganizationSchemaRefFactory()
+    managed_schema_ref_2 = SchemaRefFactory(
+        created_by=managed_schema_ref_1.created_by,
+        schema=managed_schema_ref_1.schema
+    )
+    duplicate_slug_value = 'duplicate_slug'
+    form_data = {
+        'schema_slug': '',
+        'form-0-slug': duplicate_slug_value,
+        'form-1-slug': duplicate_slug_value,
+        'form-TOTAL_FORMS': 2,
+        'form-INITIAL_FORMS': 2
+    }
+    form = PermanentURLsForm(schema=managed_schema_ref_1.schema, data=form_data)
+    assert not form.is_valid()
+    error = form.non_field_errors()[0]
+    assert error == 'Each URL must be unique.'
+

tests/test_views.py

@@ -219,3 +219,38 @@ def test_permanent_urlmanagement_form_404_for_private_schema():
     response = client.get(f'/schemas/{schema.id}/permanent-urls', follow=True)
     assert response.status_code == 404
 
+
+@pytest.mark.django_db
+def test_saving_schemas_preserves_existing_reference_items():
+    schema = SchemaFactory()
+    schema_ref = SchemaRefFactory(schema=schema, url='https://examle.com/file.json')
+    documentation_item = DocumentationItemFactory(
+        schema=schema,
+        role=None
+    )
+    client = Client()
+    client.force_login(schema.created_by)
+    new_schema_ref_name = 'NEW ' + schema_ref.name if schema_ref.name else 'NEW NAME'
+    new_documentation_item_name = 'NEW ' + documentation_item.name
+    with requests_mock.Mocker() as m:
+        m.get(schema_ref.url, text='{}')
+        m.get('https://example.com', text='{}')
+        client.post(f'/manage/schema/{schema.id}', {
+            'name': schema.name,
+            'schema_refs-0-id': schema_ref.id,
+            'schema_refs-0-url': schema_ref.url,
+            'schema_refs-0-name': new_schema_ref_name,
+            'readme_url': 'https://example.com',
+            'documentation_items-0-id': documentation_item.id,
+            'documentation_items-0-name': new_documentation_item_name,
+            'documentation_items-0-url': documentation_item.url,
+            'documentation_items-TOTAL_FORMS': 1,
+            'documentation_items-INITIAL_FORMS': 1,
+            'schema_refs-TOTAL_FORMS': 1,
+            'schema_refs-INITIAL_FORMS': 1
+        })
+    schema_ref.refresh_from_db()
+    documentation_item.refresh_from_db()
+    assert schema_ref.name == new_schema_ref_name
+    assert documentation_item.name == new_documentation_item_name
+