diff --git a/core/forms.py b/core/forms.py index 147fe1e..736bd04 100644 --- a/core/forms.py +++ b/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() diff --git a/core/static/js/site.js b/core/static/js/site.js index edfc460..6ac47e5 100644 --- a/core/static/js/site.js +++ b/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); - }); }); }); diff --git a/core/templates/core/manage/documentation_item_form.html b/core/templates/core/manage/documentation_item_form.html index c53e8e8..59e3013 100644 --- a/core/templates/core/manage/documentation_item_form.html +++ b/core/templates/core/manage/documentation_item_form.html @@ -10,6 +10,7 @@
Documentation Item {{ count }}
+ {{ form.id }}
{% include "core/manage/field.html" with field=form.name %} {% include "core/manage/field.html" with field=form.role %} diff --git a/core/templates/core/manage/schema_ref_form.html b/core/templates/core/manage/schema_ref_form.html index 1c4b611..369d993 100644 --- a/core/templates/core/manage/schema_ref_form.html +++ b/core/templates/core/manage/schema_ref_form.html @@ -10,6 +10,7 @@
Schema {{ count }}
+ {{ form.id }}
{% include "core/manage/field.html" with field=form.name %} {% include "core/manage/field.html" with field=form.url %} diff --git a/core/views.py b/core/views.py index 50afea4..f760f5a 100644 --- a/core/views.py +++ b/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 } diff --git a/tests/factories.py b/tests/factories.py index f487228..e2567d5 100644 --- a/tests/factories.py +++ b/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) diff --git a/tests/test_forms.py b/tests/test_forms.py index 150eef6..38d01d2 100644 --- a/tests/test_forms.py +++ b/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.' + diff --git a/tests/test_views.py b/tests/test_views.py index 040c1fb..a0f44a6 100644 --- a/tests/test_views.py +++ b/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 +