From c92a74787198320749e0c41cbcea0abdd8275f8e Mon Sep 17 00:00:00 2001 From: Gabe Naughton Date: Fri, 24 Jun 2022 12:58:14 +0100 Subject: [PATCH 01/28] update pattern logic to accept sid and add tests (#608) --- measures/patterns.py | 2 +- measures/tests/test_views.py | 41 ++++++++++++++++++++++++++++++++++-- measures/views.py | 1 + 3 files changed, 41 insertions(+), 3 deletions(-) diff --git a/measures/patterns.py b/measures/patterns.py index a1eb46f87..ec1739f3e 100644 --- a/measures/patterns.py +++ b/measures/patterns.py @@ -288,7 +288,7 @@ def create_condition_and_components( measure condition components from newly created condition """ condition = MeasureCondition( - sid=self.measure_condition_sid_counter(), + sid=data.get("sid") or self.measure_condition_sid_counter(), component_sequence_number=component_sequence_number, dependent_measure=measure, update_type=data.get("update_type") or UpdateType.CREATE, diff --git a/measures/tests/test_views.py b/measures/tests/test_views.py index 222d10e69..4b63602db 100644 --- a/measures/tests/test_views.py +++ b/measures/tests/test_views.py @@ -356,6 +356,7 @@ def test_measure_update_create_conditions( condition.action.pk == measure_edit_conditions_data["measure-conditions-formset-0-action"] ) + assert condition.update_type == UpdateType.CREATE components = condition.components.approved_up_to_transaction(tx).all() @@ -380,11 +381,15 @@ def test_measure_update_edit_conditions( correct. """ measure = Measure.objects.with_duty_sentence().first() - previous_condition = measure.conditions.last() url = reverse("measure-ui-edit", args=(measure.sid,)) client.force_login(valid_user) client.post(url, data=measure_edit_conditions_data) transaction_count = Transaction.objects.count() + tx = Transaction.objects.last() + measure_with_condition = Measure.objects.approved_up_to_transaction(tx).get( + sid=measure.sid, + ) + previous_condition = measure_with_condition.conditions.last() measure_edit_conditions_data[ "measure-conditions-formset-0-required_certificate" ] = "" @@ -404,10 +409,11 @@ def test_measure_update_edit_conditions( condition = updated_measure.conditions.approved_up_to_transaction(tx).first() - assert condition != previous_condition + assert condition.pk != previous_condition.pk assert condition.required_certificate == None assert condition.duty_amount == 3 assert condition.update_type == UpdateType.UPDATE + assert condition.sid == previous_condition.sid components = condition.components.approved_up_to_transaction(tx).all() @@ -420,6 +426,37 @@ def test_measure_update_edit_conditions( assert component.transaction == condition.transaction +# The measure edit form will always show changes until we fix this bug https://uktrade.atlassian.net/browse/TP2000-247 /PS-IGNORE +# When fixed, we should uncomment and add logic to prevent updates when there are no changes to a condition + +# def test_measure_update_no_conditions_changes( +# client, +# valid_user, +# measure_edit_conditions_data, /PS-IGNORE +# duty_sentence_parser, +# erga_omnes, +# ): +# measure = Measure.objects.with_duty_sentence().first() +# url = reverse("measure-ui-edit", args=(measure.sid,)) +# client.force_login(valid_user) /PS-IGNORE +# client.post(url, data=measure_edit_conditions_data) /PS-IGNORE +# tx = Transaction.objects.last() +# measure_with_condition = Measure.objects.approved_up_to_transaction(tx).get( /PS-IGNORE +# sid=measure.sid, /PS-IGNORE +# ) +# previous_condition = measure_with_condition.conditions.approved_up_to_transaction(tx).first() +# client.post(url, data=measure_edit_conditions_data) /PS-IGNORE +# tx = Transaction.objects.last() +# updated_measure = Measure.objects.approved_up_to_transaction(tx).get( /PS-IGNORE +# sid=measure.sid, /PS-IGNORE +# ) +# condition = updated_measure.conditions.approved_up_to_transaction(tx).first() + +# assert condition.pk == previous_condition.pk +# assert condition.update_type == UpdateType.CREATE +# assert condition.sid == previous_condition.sid /PS-IGNORE + + def test_measure_update_remove_conditions( client, valid_user, diff --git a/measures/views.py b/measures/views.py index b305b75ab..aca8d816a 100644 --- a/measures/views.py +++ b/measures/views.py @@ -415,6 +415,7 @@ def create_conditions(self, obj): condition_data["version_group"] = existing_conditions.get( sid=f.initial["condition_sid"], ).version_group + condition_data["sid"] = f.initial["condition_sid"] # If changed and condition_sid not in changed_data, then this is a newly created condition elif f.has_changed() and "condition_sid" not in f.changed_data: update_type = UpdateType.CREATE From 5c7193b0d19eaaf8be82297ca1716bdb068c5edc Mon Sep 17 00:00:00 2001 From: Edie Pearce Date: Wed, 29 Jun 2022 11:52:05 +0100 Subject: [PATCH 02/28] TP2000-378 Add sid to conditions tab, add conditions link (#610) * Add sid to conditions tab, add conditions link * Update test * Update test - findChildren is deprecated --- common/models/trackedmodel.py | 12 ++++++++-- common/tests/test_models.py | 3 +++ .../includes/measures/tabs/conditions.jinja | 3 ++- measures/models.py | 2 ++ measures/tests/test_views.py | 23 +++++++++---------- 5 files changed, 28 insertions(+), 15 deletions(-) diff --git a/common/models/trackedmodel.py b/common/models/trackedmodel.py index 84fde6afc..2ae497626 100644 --- a/common/models/trackedmodel.py +++ b/common/models/trackedmodel.py @@ -141,10 +141,17 @@ class TrackedModel(PolymorphicModel): model, but in places where this does not exist models can declare their own. (Note that because multiple versions of each model will exist this does not actually equate to a ``UNIQUE`` constraint in the database.) - + TrackedModel itself defaults to ("pk",) as it does not have an SID. """ + url_suffix = "" + """ + This is to add a link within a page for get_url() e.g. for linking to a + Measure's conditions tab. If url_suffix is set to '#conditions' the output + detail url will be /measures/12345678/#conditions + """ + def new_version( self: Cls, workbasket, @@ -623,10 +630,11 @@ def get_url(self, action: str = "detail") -> Optional[str]: if action not in ["list", "create"]: kwargs = self.get_identifying_fields() try: - return reverse( + url = reverse( f"{self.get_url_pattern_name_prefix()}-ui-{action}", kwargs=kwargs, ) + return f"{url}{self.url_suffix}" except NoReverseMatch: return None diff --git a/common/tests/test_models.py b/common/tests/test_models.py index 63df905e1..102fc8535 100644 --- a/common/tests/test_models.py +++ b/common/tests/test_models.py @@ -409,6 +409,9 @@ def test_trackedmodel_get_url(trackedmodel_factory): # None is returned for models that have no URL return + if instance.url_suffix: + assert instance.url_suffix in url + assert len(url) # Verify URL is not local diff --git a/measures/jinja2/includes/measures/tabs/conditions.jinja b/measures/jinja2/includes/measures/tabs/conditions.jinja index 913142e4a..b8a5599ba 100644 --- a/measures/jinja2/includes/measures/tabs/conditions.jinja +++ b/measures/jinja2/includes/measures/tabs/conditions.jinja @@ -23,6 +23,7 @@ {% set table_rows = [] %} {% for condition in conditions -%} {{ table_rows.append([ + {"text": condition.sid }, {"text": requirement(condition) }, {"text": condition.action.description}, {"text": condition.duty_sentence if condition.duty_sentence else '-'}, @@ -30,6 +31,7 @@ {% endfor %} {{ govukTable({ "head": [ + {"text": "SID"}, {"text": "Required certificate or amount", "classes": "govuk-!-width-two-thirds"}, {"text": "Action"}, {"text": "Applicable duties"}, @@ -41,4 +43,3 @@ {% if has_conditions %}

This measure has no conditions.

{% endif %} - diff --git a/measures/models.py b/measures/models.py index 6223dc4cc..e5946c35c 100644 --- a/measures/models.py +++ b/measures/models.py @@ -669,6 +669,8 @@ class MeasureCondition(TrackedModel): record_code = "430" subrecord_code = "10" + url_pattern_name_prefix = "measure" + url_suffix = "#conditions" identifying_fields = ("sid",) diff --git a/measures/tests/test_views.py b/measures/tests/test_views.py index 4b63602db..378235179 100644 --- a/measures/tests/test_views.py +++ b/measures/tests/test_views.py @@ -150,27 +150,27 @@ def test_measure_detail_conditions(client, valid_user): page.find("h3").text == f"{condition_code.code}: {condition_code.description}" ) - rows = page.find("table").findChildren(["th", "tr"]) # ignore everything above the first condition row - first_row = rows[4] - cells = first_row.findChildren(["td"]) + cells = page.select("table > tbody > tr:first-child > td") certificate = certificate_condition.required_certificate + assert cells[0].text == str(certificate_condition.sid) assert ( - cells[0].text + cells[1].text == f"{certificate.code}:\n {certificate.get_description(transaction=certificate.transaction).description}" ) - assert cells[1].text == certificate_condition.action.description - assert cells[2].text == "-" + assert cells[2].text == certificate_condition.action.description + assert cells[3].text == "-" - second_row = rows[5] - cells = second_row.findChildren(["td"]) + cells = page.select("table > tbody > tr:nth-child(2) > td") + assert cells[0].text == str(amount_condition.sid) assert ( - cells[0].text + cells[1].text == f"\n 1000.000\n {amount_condition.monetary_unit.code}" ) - assert len(rows) == 6 + rows = page.select("table > tbody > tr") + assert len(rows) == 2 @pytest.mark.parametrize( @@ -532,8 +532,7 @@ def test_measure_update_invalid_conditions( response.content.decode(response.charset), features="lxml", ) - ul = page.find_all("ul", {"class": "govuk-list govuk-error-summary__list"})[0] - a_tags = ul.findChildren("a") + a_tags = page.select("ul.govuk-list.govuk-error-summary__list a") assert a_tags[0].attrs["href"] == "#measure-conditions-formset-0-applicable_duty" assert a_tags[0].text == "Enter a valid duty sentence." From 089a5c543c9e5ef1329ac0773ead27e514424b8f Mon Sep 17 00:00:00 2001 From: Edie Pearce Date: Wed, 29 Jun 2022 17:30:54 +0100 Subject: [PATCH 03/28] TP2000-377 Add footnotes tab to measure detail page (#613) * Add footnotes tab to measure detail page * Fix logic for no conditions * Fix logic for no footnotes --- .../includes/measures/tabs/conditions.jinja | 48 ++++++------- .../includes/measures/tabs/footnotes.jinja | 22 ++++++ measures/jinja2/measures/detail.jinja | 8 +++ measures/tests/test_views.py | 70 +++++++++++++++++++ measures/views.py | 1 + 5 files changed, 124 insertions(+), 25 deletions(-) create mode 100644 measures/jinja2/includes/measures/tabs/footnotes.jinja diff --git a/measures/jinja2/includes/measures/tabs/conditions.jinja b/measures/jinja2/includes/measures/tabs/conditions.jinja index b8a5599ba..1ee4f3828 100644 --- a/measures/jinja2/includes/measures/tabs/conditions.jinja +++ b/measures/jinja2/includes/measures/tabs/conditions.jinja @@ -16,30 +16,28 @@

Conditions

-{% set has_conditions = false %} -{% for condition_code, conditions in condition_groups %} -

{{ condition_code.code }}: {{ condition_code.description }}

- {% set has_conditions = true %} - {% set table_rows = [] %} - {% for condition in conditions -%} - {{ table_rows.append([ - {"text": condition.sid }, - {"text": requirement(condition) }, - {"text": condition.action.description}, - {"text": condition.duty_sentence if condition.duty_sentence else '-'}, - ]) or "" }} - {% endfor %} - {{ govukTable({ - "head": [ - {"text": "SID"}, - {"text": "Required certificate or amount", "classes": "govuk-!-width-two-thirds"}, - {"text": "Action"}, - {"text": "Applicable duties"}, - ], - "rows": table_rows - }) }} -{% endfor %} - {% if has_conditions %} -

This measure has no conditions.

+ {% for condition_code, conditions in condition_groups %} +

{{ condition_code.code }}: {{ condition_code.description }}

+ {% set table_rows = [] %} + {% for condition in conditions -%} + {{ table_rows.append([ + {"text": condition.sid }, + {"text": requirement(condition) }, + {"text": condition.action.description}, + {"text": condition.duty_sentence if condition.duty_sentence else '-'}, + ]) or "" }} + {% endfor %} + {{ govukTable({ + "head": [ + {"text": "SID"}, + {"text": "Required certificate or amount", "classes": "govuk-!-width-two-thirds"}, + {"text": "Action"}, + {"text": "Applicable duties"}, + ], + "rows": table_rows + }) }} + {% endfor %} +{% else %} +

This measure has no conditions.

{% endif %} diff --git a/measures/jinja2/includes/measures/tabs/footnotes.jinja b/measures/jinja2/includes/measures/tabs/footnotes.jinja new file mode 100644 index 000000000..b2047baab --- /dev/null +++ b/measures/jinja2/includes/measures/tabs/footnotes.jinja @@ -0,0 +1,22 @@ +{% from 'macros/create_link.jinja' import create_link %} + +

Footnotes

+ +{% if object.footnotes.all() %} + {% set table_rows = [] %} + {% for footnote in object.footnotes.all() %} + {{ table_rows.append([ + {"text": create_link(footnote.get_url(), footnote|string) }, + {"text": footnote.descriptions.first().description }, + ]) or "" }} + {% endfor %} + {{ govukTable({ + "head": [ + {"text": "SID"}, + {"text": "Description"}, + ], + "rows": table_rows + }) }} +{% else %} +

This measure has no footnotes.

+{% endif %} diff --git a/measures/jinja2/measures/detail.jinja b/measures/jinja2/measures/detail.jinja index 3c473e289..1084e533d 100644 --- a/measures/jinja2/measures/detail.jinja +++ b/measures/jinja2/measures/detail.jinja @@ -7,6 +7,7 @@ {% set core_data_tab_html %}{% include "includes/measures/tabs/core_data.jinja" %}{% endset %} {% set conditions_tab_html %}{% include "includes/measures/tabs/conditions.jinja" %}{% endset %} +{% set footnotes_tab_html %}{% include "includes/measures/tabs/footnotes.jinja" %}{% endset %} {% set tabs = { "items": [ @@ -23,6 +24,13 @@ "panel": { "html": conditions_tab_html } + }, + { + "label": "Footnotes", + "id": "footnotes", + "panel": { + "html": footnotes_tab_html + } } ] }%} diff --git a/measures/tests/test_views.py b/measures/tests/test_views.py index 378235179..3ac069a20 100644 --- a/measures/tests/test_views.py +++ b/measures/tests/test_views.py @@ -173,6 +173,76 @@ def test_measure_detail_conditions(client, valid_user): assert len(rows) == 2 +def test_measure_detail_no_conditions(client, valid_user): + measure = factories.MeasureFactory.create() + url = reverse("measure-ui-detail", kwargs={"sid": measure.sid}) + "#conditions" + client.force_login(valid_user) + response = client.get(url) + page = BeautifulSoup( + response.content.decode(response.charset), + "html.parser", + ) + + assert ( + page.select("#conditions .govuk-body")[0].text + == "This measure has no conditions." + ) + + +def test_measure_detail_footnotes(client, valid_user): + measure = factories.MeasureFactory.create() + footnote1 = factories.FootnoteAssociationMeasureFactory.create( + footnoted_measure=measure, + ).associated_footnote + footnote2 = factories.FootnoteAssociationMeasureFactory.create( + footnoted_measure=measure, + ).associated_footnote + url = reverse("measure-ui-detail", kwargs={"sid": measure.sid}) + "#footnotes" + client.force_login(valid_user) + response = client.get(url) + page = BeautifulSoup( + response.content.decode(response.charset), + "html.parser", + ) + + rows = page.select("#footnotes table > tbody > tr") + assert len(rows) == 2 + + first_column_links = page.select( + "#footnotes table > tbody > tr > td:first-child > a", + ) + assert {link.text for link in first_column_links} == { + str(footnote1), + str(footnote2), + } + assert {link.get("href") for link in first_column_links} == { + footnote1.get_url(), + footnote2.get_url(), + } + + second_column = page.select("#footnotes table > tbody > tr > td:nth-child(2)") + assert {cell.text for cell in second_column} == { + footnote1.descriptions.first().description, + footnote2.descriptions.first().description, + } + + +def test_measure_detail_no_footnotes(client, valid_user): + measure = factories.MeasureFactory.create() + url = reverse("measure-ui-detail", kwargs={"sid": measure.sid}) + "#footnotes" + client.force_login(valid_user) + response = client.get(url) + page = BeautifulSoup( + response.content.decode(response.charset), + "html.parser", + ) + + assert ( + page.select("#footnotes .govuk-body")[0].text + == "This measure has no footnotes." + ) + + @pytest.mark.parametrize( ("view", "url_pattern"), get_class_based_view_urls_matching_url( diff --git a/measures/views.py b/measures/views.py index aca8d816a..a08d18ec6 100644 --- a/measures/views.py +++ b/measures/views.py @@ -90,6 +90,7 @@ def get_context_data(self, **kwargs: Any): context = super().get_context_data(**kwargs) context["condition_groups"] = condition_groups + context["has_conditions"] = bool(len(conditions)) return context From 3e882d5be656a9cf9d1929e840180c09fe630ac2 Mon Sep 17 00:00:00 2001 From: Edie Pearce Date: Wed, 29 Jun 2022 18:33:53 +0100 Subject: [PATCH 04/28] TP2000-376 Add version control tab to measure detail page (#612) * Add version control tab to measure detail page * Update selectors in test --- .../measures/tabs/version_control.jinja | 21 ++++++++++++++ measures/jinja2/measures/detail.jinja | 8 +++++ measures/tests/test_views.py | 29 ++++++++++++++++--- 3 files changed, 54 insertions(+), 4 deletions(-) create mode 100644 measures/jinja2/includes/measures/tabs/version_control.jinja diff --git a/measures/jinja2/includes/measures/tabs/version_control.jinja b/measures/jinja2/includes/measures/tabs/version_control.jinja new file mode 100644 index 000000000..326e067e4 --- /dev/null +++ b/measures/jinja2/includes/measures/tabs/version_control.jinja @@ -0,0 +1,21 @@ +

Version control

+{% set table_rows = [] %} +{% for object in object.version_group.versions.all() %} + {{ table_rows.append([ + {"text": object.get_update_type_display()}, + {"text": "{:%d %b %Y}".format(object.version_group.created_at)}, + {"text": object.transaction.workbasket.get_status_display()}, + {"text": "{:%d %b %Y}".format(object.valid_between.lower)}, + {"text": "{:%d %b %Y}".format(object.valid_between.upper) if object.valid_between.upper else "-"}, + ]) or "" }} +{% endfor %} +{{ govukTable({ + "head": [ + {"text": "Activity"}, + {"text": "Date"}, + {"text": "Status"}, + {"text": "Start date"}, + {"text": "End date"}, + ], + "rows": table_rows +}) }} diff --git a/measures/jinja2/measures/detail.jinja b/measures/jinja2/measures/detail.jinja index 1084e533d..fe1863cb1 100644 --- a/measures/jinja2/measures/detail.jinja +++ b/measures/jinja2/measures/detail.jinja @@ -8,6 +8,7 @@ {% set core_data_tab_html %}{% include "includes/measures/tabs/core_data.jinja" %}{% endset %} {% set conditions_tab_html %}{% include "includes/measures/tabs/conditions.jinja" %}{% endset %} {% set footnotes_tab_html %}{% include "includes/measures/tabs/footnotes.jinja" %}{% endset %} +{% set version_control_tab_html %}{% include "includes/measures/tabs/version_control.jinja" %}{% endset %} {% set tabs = { "items": [ @@ -31,6 +32,13 @@ "panel": { "html": footnotes_tab_html } + }, + { + "label": "Version control", + "id": "versions", + "panel": { + "html": version_control_tab_html + } } ] }%} diff --git a/measures/tests/test_views.py b/measures/tests/test_views.py index 3ac069a20..9c5a8a674 100644 --- a/measures/tests/test_views.py +++ b/measures/tests/test_views.py @@ -151,7 +151,7 @@ def test_measure_detail_conditions(client, valid_user): ) # ignore everything above the first condition row - cells = page.select("table > tbody > tr:first-child > td") + cells = page.select("#conditions table > tbody > tr:first-child > td") certificate = certificate_condition.required_certificate assert cells[0].text == str(certificate_condition.sid) @@ -162,14 +162,14 @@ def test_measure_detail_conditions(client, valid_user): assert cells[2].text == certificate_condition.action.description assert cells[3].text == "-" - cells = page.select("table > tbody > tr:nth-child(2) > td") + cells = page.select("#conditions table > tbody > tr:nth-child(2) > td") assert cells[0].text == str(amount_condition.sid) assert ( cells[1].text == f"\n 1000.000\n {amount_condition.monetary_unit.code}" ) - rows = page.select("table > tbody > tr") + rows = page.select("#conditions table > tbody > tr") assert len(rows) == 2 @@ -204,7 +204,6 @@ def test_measure_detail_footnotes(client, valid_user): response.content.decode(response.charset), "html.parser", ) - rows = page.select("#footnotes table > tbody > tr") assert len(rows) == 2 @@ -243,6 +242,28 @@ def test_measure_detail_no_footnotes(client, valid_user): ) +def test_measure_detail_version_control(client, valid_user): + measure = factories.MeasureFactory.create() + measure.new_version(measure.transaction.workbasket) + measure.new_version(measure.transaction.workbasket) + + url = reverse("measure-ui-detail", kwargs={"sid": measure.sid}) + "#versions" + client.force_login(valid_user) + response = client.get(url) + soup = BeautifulSoup( + response.content.decode(response.charset), + "html.parser", + ) + rows = soup.select("#versions table > tbody > tr") + assert len(rows) == 3 + + update_types = { + cell.text + for cell in soup.select("#versions table > tbody > tr > td:first-child") + } + assert update_types == {"Create", "Update"} + + @pytest.mark.parametrize( ("view", "url_pattern"), get_class_based_view_urls_matching_url( From 4264f106fbbe9f9884c7e5f1f16637f9bb3f85d3 Mon Sep 17 00:00:00 2001 From: Edie Pearce Date: Fri, 1 Jul 2022 12:11:31 +0100 Subject: [PATCH 05/28] Add quota order number to measure detail page (#614) --- .../jinja2/includes/measures/tabs/core_data.jinja | 2 +- measures/tests/test_views.py | 14 ++++++++++++++ 2 files changed, 15 insertions(+), 1 deletion(-) diff --git a/measures/jinja2/includes/measures/tabs/core_data.jinja b/measures/jinja2/includes/measures/tabs/core_data.jinja index 11e195dd5..ea5d20be3 100644 --- a/measures/jinja2/includes/measures/tabs/core_data.jinja +++ b/measures/jinja2/includes/measures/tabs/core_data.jinja @@ -58,7 +58,7 @@ }, { "key": {"text": "Quota order number"}, - "value": {"text": object.order_number.sid if object.order_number else '-'}, + "value": {"text": object.order_number if object.order_number else '-'}, "actions": {"items": []} }, { diff --git a/measures/tests/test_views.py b/measures/tests/test_views.py index 9c5a8a674..6cced7e8f 100644 --- a/measures/tests/test_views.py +++ b/measures/tests/test_views.py @@ -242,6 +242,20 @@ def test_measure_detail_no_footnotes(client, valid_user): ) +def test_measure_detail_quota_order_number(client, valid_user): + quota_order_number = factories.QuotaOrderNumberFactory.create() + measure = factories.MeasureFactory.create(order_number=quota_order_number) + url = reverse("measure-ui-detail", kwargs={"sid": measure.sid}) + client.force_login(valid_user) + response = client.get(url) + page = BeautifulSoup( + response.content.decode(response.charset), + "html.parser", + ) + items = [element.text.strip() for element in page.select("#core-data dl dd")] + assert str(quota_order_number) in items + + def test_measure_detail_version_control(client, valid_user): measure = factories.MeasureFactory.create() measure.new_version(measure.transaction.workbasket) From 5f0d330d723a9a1636e0bedb4acaa51df56c9717 Mon Sep 17 00:00:00 2001 From: Paul Pepper <85895113+paulpepper-trade@users.noreply.github.com> Date: Thu, 7 Jul 2022 14:48:31 +0100 Subject: [PATCH 06/28] Remove duplicate pytest-celery package, already included in requirements.txt. (#607) --- requirements-dev.txt | 1 - 1 file changed, 1 deletion(-) diff --git a/requirements-dev.txt b/requirements-dev.txt index 22ba633cc..f5448caa1 100644 --- a/requirements-dev.txt +++ b/requirements-dev.txt @@ -2,4 +2,3 @@ django_debug_toolbar pre-commit -pytest-celery From c991f5b5794920a3eeffc9410abed23a786bedec Mon Sep 17 00:00:00 2001 From: "dependabot[bot]" <49699333+dependabot[bot]@users.noreply.github.com> Date: Fri, 8 Jul 2022 10:55:18 +0100 Subject: [PATCH 07/28] Bump lxml from 4.6.5 to 4.9.1 (#618) Bumps [lxml](https://github.com/lxml/lxml) from 4.6.5 to 4.9.1. - [Release notes](https://github.com/lxml/lxml/releases) - [Changelog](https://github.com/lxml/lxml/blob/master/CHANGES.txt) - [Commits](https://github.com/lxml/lxml/compare/lxml-4.6.5...lxml-4.9.1) --- updated-dependencies: - dependency-name: lxml dependency-type: direct:production ... Signed-off-by: dependabot[bot] Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> --- requirements.txt | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/requirements.txt b/requirements.txt index ddfd8ac72..15166e790 100644 --- a/requirements.txt +++ b/requirements.txt @@ -35,7 +35,7 @@ govuk-frontend-jinja @ git+https://github.com/alphagov/govuk-frontend-jinja.git@ govuk-tech-docs-sphinx-theme==1.0.0 gunicorn==20.0.4 Jinja2==2.11.3 -lxml==4.6.5 +lxml==4.9.1 markupsafe==2.0.1 moto==2.1.0 openpyxl==3.0.7 From a920c36ddc1bae8ddb6d3f5bd48debc36ce9d9f8 Mon Sep 17 00:00:00 2001 From: Gabe Naughton Date: Fri, 8 Jul 2022 15:21:44 +0100 Subject: [PATCH 08/28] Make copy accept saved related models (#621) * add new test, broke others * update copy to handle passing saved objects --- common/models/trackedmodel.py | 14 +++++++--- common/tests/test_models.py | 49 ++++++++++++++++++++++++++++++++++- 2 files changed, 58 insertions(+), 5 deletions(-) diff --git a/common/models/trackedmodel.py b/common/models/trackedmodel.py index 2ae497626..e20938f63 100644 --- a/common/models/trackedmodel.py +++ b/common/models/trackedmodel.py @@ -473,15 +473,21 @@ def copy( ignore = False # Check if user passed related model into overrides argument if field.name in subrecord_fields.keys(): - # If user passed a new unsaved model, set the remote field value equal to self for each model passed - # e.g. if a Measure is copied and a MeasureCondition is passed, update `dependent_measure` field to `self` + # If user passed a new unsaved model, set the remote field value equal to new_object for each model passed + # e.g. if a Measure is copied and a MeasureCondition is passed, update `dependent_measure` field to `new_object` if subrecord_fields[field.name]: for subrecord in subrecord_fields[field.name]: remote_field = [ f for f in self._meta.get_fields() if f.name == field.name ][0].remote_field.name - setattr(subrecord, remote_field, self) - subrecord.save() + if not subrecord.pk: + setattr(subrecord, remote_field, new_object) + subrecord.save() + else: + # If user passed a saved object, create a copy of that object with remote_field pointing at the new copied object + # set ignore to True, so that duplicate copies are not made below + subrecord.copy(transaction, **{remote_field: new_object}) + ignore = True # Else, if an empty or None value is passed, set ignore to True, so that related models are not copied # e.g. if an existing Measure with two conditions is copied with conditions=[], the copy will have no conditions else: diff --git a/common/tests/test_models.py b/common/tests/test_models.py index 102fc8535..7fcbfa4f3 100644 --- a/common/tests/test_models.py +++ b/common/tests/test_models.py @@ -24,6 +24,7 @@ from common.validators import UpdateType from footnotes.models import FootnoteType from measures.models import MeasureCondition +from measures.models import MeasureExcludedGeographicalArea from regulations.models import Group from regulations.models import Regulation from taric.models import Envelope @@ -622,12 +623,13 @@ def test_copy_fk_related_models(): "component_sequence_number": 1, "condition_code_id": code.pk, } - + assert measure.conditions.count() == 0 copied_measure = measure.copy( transaction, conditions=[MeasureCondition(**condition_data)], ) + assert measure.conditions.count() == 0 assert copied_measure.conditions.count() == 1 @@ -689,6 +691,51 @@ def test_copy_nested_field_two_levels_deep(): assert copied_measure.conditions.first().components.first().duty_amount == 0 +def test_copy_saved_related_models(): + """Test that copy accepts a queryset of saved objects, creates a new measure + with all of these related objects, and preserves original measure's + relationships.""" + workbasket = factories.WorkBasketFactory.create() + measure = factories.MeasureFactory.create(transaction=workbasket.new_transaction()) + exclusion_1 = factories.MeasureExcludedGeographicalAreaFactory.create( + transaction=workbasket.new_transaction(), + modified_measure=measure, + ) + exclusion_2 = factories.MeasureExcludedGeographicalAreaFactory.create( + transaction=workbasket.new_transaction(), + modified_measure=measure, + ) + exclusion_3 = factories.MeasureExcludedGeographicalAreaFactory.create( + transaction=workbasket.new_transaction(), + modified_measure=measure, + ) + exclusions = MeasureExcludedGeographicalArea.objects.exclude(pk=exclusion_1.pk) + copied_measure = measure.copy(workbasket.new_transaction(), exclusions=exclusions) + + assert measure.exclusions.count() == 3 + assert copied_measure.exclusions.count() == 2 + assert exclusion_1 in measure.exclusions.all() + assert not copied_measure.exclusions.filter( + modified_measure__sid=exclusion_1.modified_measure.sid, + ).exists() + assert exclusion_2 in measure.exclusions.all() + assert copied_measure.exclusions.filter( + modified_measure__sid=copied_measure.sid, + excluded_geographical_area=exclusion_2.excluded_geographical_area, + ).exists() + assert not copied_measure.exclusions.filter( + modified_measure__sid=exclusion_2.modified_measure.sid, + ).exists() + assert exclusion_3 in measure.exclusions.all() + assert copied_measure.exclusions.filter( + modified_measure__sid=copied_measure.sid, + excluded_geographical_area=exclusion_3.excluded_geographical_area, + ).exists() + assert not copied_measure.exclusions.filter( + modified_measure__sid=exclusion_3.modified_measure.sid, + ).exists() + + def test_transaction_summary(approved_transaction): """Verify that transaction.summary returns a LazyString which evaluates to a string with the expected fields.""" From 5c7c1e40b0e7fbe63ac6970a583727554a22f5ad Mon Sep 17 00:00:00 2001 From: Edie Pearce Date: Fri, 8 Jul 2022 15:53:12 +0100 Subject: [PATCH 09/28] TP2000-225 Fix geo area search (#617) * Fix geo area search --- geo_areas/tests/test_util.py | 19 +++++++++++++++++++ geo_areas/util.py | 20 +++++++++++++++----- geo_areas/views.py | 21 ++++++++++++++------- 3 files changed, 48 insertions(+), 12 deletions(-) diff --git a/geo_areas/tests/test_util.py b/geo_areas/tests/test_util.py index 5ed3c3202..87799c118 100644 --- a/geo_areas/tests/test_util.py +++ b/geo_areas/tests/test_util.py @@ -36,3 +36,22 @@ def test_with_latest_description_multiple_descriptions(): assert qs.count() == 1 assert later_description.description == qs.first().description + + +def test_with_latest_description_multiple_descriptions_same_date(): + """Tests that when a GeographicalArea has more than one description with the + same validity start date, the description from the later transaction is used + to generate the description string.""" + area = factories.GeographicalAreaFactory.create() + first_description = factories.GeographicalAreaDescriptionFactory.create( + validity_start=datetime.today(), + described_geographicalarea=area, + ) + second_description = factories.GeographicalAreaDescriptionFactory.create( + validity_start=datetime.today(), + described_geographicalarea=area, + ) + qs = util.with_latest_description_string(models.GeographicalArea.objects.all()) + + assert qs.count() == 1 + assert second_description.description == qs.first().description diff --git a/geo_areas/util.py b/geo_areas/util.py index 1eb44f081..e7216b123 100644 --- a/geo_areas/util.py +++ b/geo_areas/util.py @@ -2,12 +2,22 @@ def with_latest_description_string(qs): - """Returns a queryset annotate with its latest description's validity_start - date, filtered by this date and then annotated with that latest description - object's description field value.""" + """Returns a queryset annotated with the latest validity_start date and + current version, filtered by these values and then annotated with that + latest description object's description field value.""" return ( - qs.annotate(latest_description_date=models.Max("descriptions__validity_start")) - .filter(descriptions__validity_start=models.F("latest_description_date")) + qs.annotate( + description_current_version=models.Max( + "descriptions__version_group__current_version__pk", + ), + latest_description_date=models.Max("descriptions__validity_start"), + ) + .filter( + descriptions__version_group__current_version__pk=models.F( + "description_current_version", + ), + descriptions__validity_start=models.F("latest_description_date"), + ) .annotate( description=models.F( "descriptions__description", diff --git a/geo_areas/views.py b/geo_areas/views.py index 70d851d72..ac2c44d73 100644 --- a/geo_areas/views.py +++ b/geo_areas/views.py @@ -14,6 +14,7 @@ from geo_areas.forms import GeographicalAreaCreateDescriptionForm from geo_areas.models import GeographicalArea from geo_areas.models import GeographicalAreaDescription +from geo_areas.util import with_latest_description_string from workbaskets.models import WorkBasket from workbaskets.views.generic import DraftCreateView from workbaskets.views.generic import DraftDeleteView @@ -22,21 +23,22 @@ class GeoAreaViewSet(viewsets.ReadOnlyModelViewSet): """API endpoint that allows geographical areas to be viewed.""" - queryset = GeographicalArea.objects.latest_approved().prefetch_related( - "descriptions", + queryset = with_latest_description_string( + GeographicalArea.objects.all().prefetch_related( + "descriptions", + ), ) serializer_class = AutoCompleteSerializer permission_classes = [permissions.IsAuthenticated] filterset_class = GeographicalAreaFilter - search_fields = ["sid", "area_code"] + search_fields = ["sid", "area_code", "descriptions__description"] class GeoAreaMixin: model: Type[TrackedModel] = GeographicalArea def get_queryset(self): - tx = WorkBasket.get_current_transaction(self.request) - return GeographicalArea.objects.approved_up_to_transaction(tx) + return with_latest_description_string(GeographicalArea.objects.all()) class GeoAreaDescriptionMixin: @@ -61,16 +63,21 @@ def get_context_data(self, **kwargs): class GeoAreaList(GeoAreaMixin, TamatoListView): template_name = "geo_areas/list.jinja" filterset_class = GeographicalAreaFilter - search_fields = ["sid", "descriptions__description"] + search_fields = ["sid", "area_code", "descriptions__description"] class GeoAreaDetail(GeoAreaMixin, TrackedModelDetailView): template_name = "geo_areas/detail.jinja" -class GeoAreaDelete(GeoAreaMixin, TrackedModelDetailMixin, DraftDeleteView): +class GeoAreaDelete(TrackedModelDetailMixin, DraftDeleteView): form_class = forms.GeographicalAreaDeleteForm success_path = "list" + model: Type[TrackedModel] = GeographicalArea + + def get_queryset(self): + tx = WorkBasket.get_current_transaction(self.request) + return GeographicalArea.objects.approved_up_to_transaction(tx) validate_business_rules = ( business_rules.GA21, From 4aa5b5b3c460bc74c96438b8b963c8a2ff7dc732 Mon Sep 17 00:00:00 2001 From: Gabe Naughton Date: Fri, 8 Jul 2022 16:06:56 +0100 Subject: [PATCH 10/28] TP2000-386 get_current_memberships uses latest_approved instead of approved_up_to_transaction (#620) * make get_current_memberships use .current, and pass in transaction to geo area forms from measure wizard and edit form * set current transaction for membership tests * add comment on CountryRegionForm * use current() instead of passing transaction * add set_current_transaction to tests --- geo_areas/models.py | 2 +- geo_areas/tests/test_models.py | 5 +++++ measures/forms.py | 24 +++++------------------- measures/tests/conftest.py | 3 +++ measures/tests/test_forms.py | 17 +++++++++++++++++ measures/views.py | 1 + 6 files changed, 32 insertions(+), 20 deletions(-) diff --git a/geo_areas/models.py b/geo_areas/models.py index 23b6c4367..4d0de75e1 100644 --- a/geo_areas/models.py +++ b/geo_areas/models.py @@ -94,7 +94,7 @@ def get_current_memberships(self): GeographicalMembership.objects.filter( Q(geo_group__sid=self.sid) | Q(member__sid=self.sid), ) - .latest_approved() + .current() .select_related("member", "geo_group") ) diff --git a/geo_areas/tests/test_models.py b/geo_areas/tests/test_models.py index c3173c7c6..84d2bec9c 100644 --- a/geo_areas/tests/test_models.py +++ b/geo_areas/tests/test_models.py @@ -1,5 +1,7 @@ import pytest +from common.models.transactions import Transaction +from common.models.utils import set_current_transaction from common.tests import factories pytestmark = pytest.mark.django_db @@ -24,6 +26,7 @@ def test_get_current_memberships_on_groups(membership_data, expected): group = factories.GeoGroupFactory() for data in membership_data(group): factories.GeographicalMembershipFactory(**data) + set_current_transaction(Transaction.objects.last()) assert len(group.get_current_memberships()) == expected @@ -47,6 +50,7 @@ def test_get_current_memberships_on_areas(membership_data, expected): area = factories.CountryFactory() for data in membership_data(area): factories.GeographicalMembershipFactory(**data) + set_current_transaction(Transaction.objects.last()) assert len(area.get_current_memberships()) == expected @@ -56,6 +60,7 @@ def test_get_current_memberships_when_region_and_country_share_sid(): region = factories.RegionFactory.create(sid=country.sid) country_membership = factories.GeographicalMembershipFactory.create(member=country) region_membership = factories.GeographicalMembershipFactory.create(member=region) + set_current_transaction(Transaction.objects.last()) country_memberships = country.get_current_memberships() region_memberships = region.get_current_memberships() diff --git a/measures/forms.py b/measures/forms.py index a499862a2..c5e5e8c69 100644 --- a/measures/forms.py +++ b/measures/forms.py @@ -90,8 +90,6 @@ class GeoGroupForm(forms.Form): ) def __init__(self, *args, **kwargs): - tx = kwargs.pop("transaction", None) - self.transaction = tx super().__init__(*args, **kwargs) self.fields[ "geographical_area_group" @@ -100,7 +98,7 @@ def __init__(self, *args, **kwargs): descriptions__description__isnull=True, ) .as_at_today() - .approved_up_to_transaction(tx) + .current() .with_latest_links("descriptions") .prefetch_related("descriptions") .order_by("descriptions__description"), @@ -125,15 +123,13 @@ class ErgaOmnesExclusionsForm(forms.Form): ) def __init__(self, *args, **kwargs): - tx = kwargs.pop("transaction", None) - self.transaction = tx super().__init__(*args, **kwargs) self.fields["erga_omnes_exclusion"].queryset = with_latest_description_string( GeographicalArea.objects.exclude( descriptions__description__isnull=True, ) .as_at_today() - .approved_up_to_transaction(tx) + .current() .with_latest_links("descriptions") .prefetch_related("descriptions") .order_by("descriptions__description"), @@ -155,15 +151,13 @@ class GeoGroupExclusionsForm(forms.Form): ) def __init__(self, *args, **kwargs): - tx = kwargs.pop("transaction", None) - self.transaction = tx super().__init__(*args, **kwargs) self.fields["geo_group_exclusion"].queryset = with_latest_description_string( GeographicalArea.objects.exclude( descriptions__description__isnull=True, ) .as_at_today() - .approved_up_to_transaction(tx) + .current() .with_latest_links("descriptions") .prefetch_related("descriptions") .order_by("descriptions__description"), @@ -222,13 +216,11 @@ class CountryRegionForm(forms.Form): ) def __init__(self, *args, **kwargs): - tx = kwargs.pop("transaction", None) - self.transaction = tx super().__init__(*args, **kwargs) self.fields["geographical_area_country_or_region"].queryset = ( self.fields["geographical_area_country_or_region"] .queryset.as_at_today() - .approved_up_to_transaction(tx) + .current() .with_latest_links("descriptions") .prefetch_related("descriptions") .order_by("descriptions__description") @@ -855,8 +847,6 @@ class MeasureGeographicalAreaForm(BindNestedFormMixin, forms.Form): ) def __init__(self, *args, **kwargs): - tx = kwargs.pop("transaction", None) - self.transaction = tx super().__init__(*args, **kwargs) kwargs.pop("initial") @@ -898,11 +888,7 @@ def __init__(self, *args, **kwargs): @property def erga_omnes_instance(self): - return ( - GeographicalArea.objects.approved_up_to_transaction(self.transaction) - .erga_omnes() - .get() - ) + return GeographicalArea.objects.current().erga_omnes().get() def clean(self): cleaned_data = super().clean() diff --git a/measures/tests/conftest.py b/measures/tests/conftest.py index dc6c496cc..bb6121ad6 100644 --- a/measures/tests/conftest.py +++ b/measures/tests/conftest.py @@ -10,6 +10,8 @@ from django.core.exceptions import ValidationError from django.forms.models import model_to_dict +from common.models.transactions import Transaction +from common.models.utils import set_current_transaction from common.tests import factories from common.util import TaricDateRange from common.validators import ApplicabilityCode @@ -380,6 +382,7 @@ def measure_edit_conditions_data(measure_form_data): @pytest.fixture def measure_form(measure_form_data, session_with_workbasket, erga_omnes): + set_current_transaction(Transaction.objects.last()) return MeasureForm( data=measure_form_data, instance=Measure.objects.with_duty_sentence().first(), diff --git a/measures/tests/test_forms.py b/measures/tests/test_forms.py index 94cd081c8..e8b7679de 100644 --- a/measures/tests/test_forms.py +++ b/measures/tests/test_forms.py @@ -3,6 +3,8 @@ import pytest from django.forms.models import model_to_dict +from common.models.transactions import Transaction +from common.models.utils import set_current_transaction from common.tests import factories from geo_areas.validators import AreaCode from measures import forms @@ -86,6 +88,7 @@ def test_measure_forms_geo_area_valid_data_erga_omnes(erga_omnes): data = { f"{GEO_AREA_FORM_PREFIX}-geo_area": forms.GeoAreaType.ERGA_OMNES, } + set_current_transaction(Transaction.objects.last()) form = forms.MeasureGeographicalAreaForm( data, initial={}, @@ -102,6 +105,7 @@ def test_measure_forms_geo_area_valid_data_erga_omnes_exclusions(erga_omnes): "erga_omnes_exclusions_formset-0-erga_omnes_exclusion": geo_area1.pk, "erga_omnes_exclusions_formset-1-erga_omnes_exclusion": geo_area2.pk, } + set_current_transaction(Transaction.objects.last()) form = forms.MeasureGeographicalAreaForm( data, initial={}, @@ -117,6 +121,7 @@ def test_measure_forms_geo_area_valid_data_erga_omnes_exclusions_delete(erga_omn "erga_omnes_exclusions_formset-0-erga_omnes_exclusion": geo_area1.pk, "erga_omnes_exclusions_formset-0-DELETE": "1", } + set_current_transaction(Transaction.objects.last()) form = forms.MeasureGeographicalAreaForm( data, initial={}, @@ -133,6 +138,7 @@ def test_measure_forms_geo_area_valid_data_geo_group_exclusions(erga_omnes): f"{GEO_AREA_FORM_PREFIX}-geographical_area_group": geo_group.pk, "geo_group_exclusions_formset-0-geo_group_exclusion": geo_area1.pk, } + set_current_transaction(Transaction.objects.last()) form = forms.MeasureGeographicalAreaForm( data, initial={}, @@ -150,6 +156,7 @@ def test_measure_forms_geo_area_valid_data_geo_group_exclusions_delete(erga_omne "geo_group_exclusions_formset-0-geo_group_exclusion": geo_area1.pk, "geo_group_exclusions_formset-0-DELETE": "1", } + set_current_transaction(Transaction.objects.last()) form = forms.MeasureGeographicalAreaForm( data, initial={}, @@ -165,6 +172,7 @@ def test_measure_forms_geo_area_valid_data_erga_omnes_exclusions_add(erga_omnes) "erga_omnes_exclusions_formset-0-erga_omnes_exclusion": geo_area1.pk, "erga_omnes_exclusions_formset-ADD": "1", } + set_current_transaction(Transaction.objects.last()) form = forms.MeasureGeographicalAreaForm( data, initial={}, @@ -179,6 +187,7 @@ def test_measure_forms_geo_area_valid_data_geo_group(erga_omnes): f"{GEO_AREA_FORM_PREFIX}-geo_area": forms.GeoAreaType.GROUP, f"{GEO_AREA_FORM_PREFIX}-geographical_area_group": geo_group.pk, } + set_current_transaction(Transaction.objects.last()) form = forms.MeasureGeographicalAreaForm( data, initial={}, @@ -195,6 +204,7 @@ def test_measure_forms_geo_area_valid_data_countries(erga_omnes): "country_region_formset-0-geographical_area_country_or_region": geo_area1.pk, "country_region_formset-1-geographical_area_country_or_region": geo_area2.pk, } + set_current_transaction(Transaction.objects.last()) form = forms.MeasureGeographicalAreaForm( data, initial={}, @@ -212,6 +222,7 @@ def test_measure_forms_geo_area_valid_data_countries_delete(erga_omnes): "country_region_formset-1-geographical_area_country_or_region": geo_area2.pk, "country_region_formset-1-DELETE": "on", } + set_current_transaction(Transaction.objects.last()) form = forms.MeasureGeographicalAreaForm( data, initial={}, @@ -227,6 +238,7 @@ def test_measure_forms_geo_area_valid_data_countries_add(erga_omnes): "country_region_formset-0-geographical_area_country_or_region": geo_area1.pk, "country_region_formset-ADD": "1", } + set_current_transaction(Transaction.objects.last()) form = forms.MeasureGeographicalAreaForm( data, initial={}, @@ -241,6 +253,7 @@ def test_measure_forms_geo_area_invalid_data_geo_group(erga_omnes): f"{GEO_AREA_FORM_PREFIX}-geo_area": forms.GeoAreaType.GROUP, "geographical_area_group-geographical_area_group": geo_area1.pk, } + set_current_transaction(Transaction.objects.last()) form = forms.MeasureGeographicalAreaForm( data, initial={}, @@ -270,6 +283,7 @@ def test_measure_forms_geo_area_invalid_data_geo_group(erga_omnes): ], ) def test_measure_forms_geo_area_invalid_data_error_messages(data, error, erga_omnes): + set_current_transaction(Transaction.objects.last()) form = forms.MeasureGeographicalAreaForm( data, initial={}, @@ -684,6 +698,7 @@ def test_measure_form_valid_data(erga_omnes, session_with_workbasket): start_date_1=start_date.month, start_date_2=start_date.year, ) + set_current_transaction(Transaction.objects.last()) form = forms.MeasureForm( data=data, initial={}, @@ -748,6 +763,7 @@ def test_measure_form_cleaned_data_geo_exclusions_group( "geo_group_exclusions_formset-1-geo_group_exclusion": excluded_country2.pk, } data.update(exclusions_data) + set_current_transaction(Transaction.objects.last()) form = forms.MeasureForm( data=data, initial={}, @@ -784,6 +800,7 @@ def test_measure_form_cleaned_data_geo_exclusions_erga_omnes( "erga_omnes_exclusions_formset-1-erga_omnes_exclusion": excluded_country2.pk, } data.update(exclusions_data) + set_current_transaction(Transaction.objects.last()) form = forms.MeasureForm( data=data, initial={}, diff --git a/measures/views.py b/measures/views.py index a08d18ec6..5ad495cbe 100644 --- a/measures/views.py +++ b/measures/views.py @@ -277,6 +277,7 @@ def get_form_kwargs(self, step): ) # commodities/duties step is a formset which expects form_kwargs to pass kwargs to its child forms kwargs["form_kwargs"] = {"measure_start_date": valid_between.lower} + return kwargs def get_form(self, step=None, data=None, files=None): From ac65327185af8c255c85ca07cb04fc7b7cbc9529 Mon Sep 17 00:00:00 2001 From: Gabe Naughton Date: Tue, 12 Jul 2022 09:43:43 +0100 Subject: [PATCH 11/28] Revert "TP2000-225 Fix geo area search (#617)" (#622) This reverts commit 5c7c1e40b0e7fbe63ac6970a583727554a22f5ad. --- geo_areas/tests/test_util.py | 19 ------------------- geo_areas/util.py | 20 +++++--------------- geo_areas/views.py | 21 +++++++-------------- 3 files changed, 12 insertions(+), 48 deletions(-) diff --git a/geo_areas/tests/test_util.py b/geo_areas/tests/test_util.py index 87799c118..5ed3c3202 100644 --- a/geo_areas/tests/test_util.py +++ b/geo_areas/tests/test_util.py @@ -36,22 +36,3 @@ def test_with_latest_description_multiple_descriptions(): assert qs.count() == 1 assert later_description.description == qs.first().description - - -def test_with_latest_description_multiple_descriptions_same_date(): - """Tests that when a GeographicalArea has more than one description with the - same validity start date, the description from the later transaction is used - to generate the description string.""" - area = factories.GeographicalAreaFactory.create() - first_description = factories.GeographicalAreaDescriptionFactory.create( - validity_start=datetime.today(), - described_geographicalarea=area, - ) - second_description = factories.GeographicalAreaDescriptionFactory.create( - validity_start=datetime.today(), - described_geographicalarea=area, - ) - qs = util.with_latest_description_string(models.GeographicalArea.objects.all()) - - assert qs.count() == 1 - assert second_description.description == qs.first().description diff --git a/geo_areas/util.py b/geo_areas/util.py index e7216b123..1eb44f081 100644 --- a/geo_areas/util.py +++ b/geo_areas/util.py @@ -2,22 +2,12 @@ def with_latest_description_string(qs): - """Returns a queryset annotated with the latest validity_start date and - current version, filtered by these values and then annotated with that - latest description object's description field value.""" + """Returns a queryset annotate with its latest description's validity_start + date, filtered by this date and then annotated with that latest description + object's description field value.""" return ( - qs.annotate( - description_current_version=models.Max( - "descriptions__version_group__current_version__pk", - ), - latest_description_date=models.Max("descriptions__validity_start"), - ) - .filter( - descriptions__version_group__current_version__pk=models.F( - "description_current_version", - ), - descriptions__validity_start=models.F("latest_description_date"), - ) + qs.annotate(latest_description_date=models.Max("descriptions__validity_start")) + .filter(descriptions__validity_start=models.F("latest_description_date")) .annotate( description=models.F( "descriptions__description", diff --git a/geo_areas/views.py b/geo_areas/views.py index ac2c44d73..70d851d72 100644 --- a/geo_areas/views.py +++ b/geo_areas/views.py @@ -14,7 +14,6 @@ from geo_areas.forms import GeographicalAreaCreateDescriptionForm from geo_areas.models import GeographicalArea from geo_areas.models import GeographicalAreaDescription -from geo_areas.util import with_latest_description_string from workbaskets.models import WorkBasket from workbaskets.views.generic import DraftCreateView from workbaskets.views.generic import DraftDeleteView @@ -23,22 +22,21 @@ class GeoAreaViewSet(viewsets.ReadOnlyModelViewSet): """API endpoint that allows geographical areas to be viewed.""" - queryset = with_latest_description_string( - GeographicalArea.objects.all().prefetch_related( - "descriptions", - ), + queryset = GeographicalArea.objects.latest_approved().prefetch_related( + "descriptions", ) serializer_class = AutoCompleteSerializer permission_classes = [permissions.IsAuthenticated] filterset_class = GeographicalAreaFilter - search_fields = ["sid", "area_code", "descriptions__description"] + search_fields = ["sid", "area_code"] class GeoAreaMixin: model: Type[TrackedModel] = GeographicalArea def get_queryset(self): - return with_latest_description_string(GeographicalArea.objects.all()) + tx = WorkBasket.get_current_transaction(self.request) + return GeographicalArea.objects.approved_up_to_transaction(tx) class GeoAreaDescriptionMixin: @@ -63,21 +61,16 @@ def get_context_data(self, **kwargs): class GeoAreaList(GeoAreaMixin, TamatoListView): template_name = "geo_areas/list.jinja" filterset_class = GeographicalAreaFilter - search_fields = ["sid", "area_code", "descriptions__description"] + search_fields = ["sid", "descriptions__description"] class GeoAreaDetail(GeoAreaMixin, TrackedModelDetailView): template_name = "geo_areas/detail.jinja" -class GeoAreaDelete(TrackedModelDetailMixin, DraftDeleteView): +class GeoAreaDelete(GeoAreaMixin, TrackedModelDetailMixin, DraftDeleteView): form_class = forms.GeographicalAreaDeleteForm success_path = "list" - model: Type[TrackedModel] = GeographicalArea - - def get_queryset(self): - tx = WorkBasket.get_current_transaction(self.request) - return GeographicalArea.objects.approved_up_to_transaction(tx) validate_business_rules = ( business_rules.GA21, From 28890728abf5bc6ecd02f46cb0b552b80718b09c Mon Sep 17 00:00:00 2001 From: Gabe Naughton Date: Mon, 18 Jul 2022 14:42:43 +0100 Subject: [PATCH 12/28] TP2000-402 500 when trying to create erga omnes measure with phytosanitary group exclusion (#624) * update membership checking logic to use sid and add test * remove redundant all() from create_measure_excluded_geographical_areas --- measures/patterns.py | 16 ++++------ measures/tests/test_patterns.py | 54 +++++++++++++++++++++++++++++++++ 2 files changed, 60 insertions(+), 10 deletions(-) diff --git a/measures/patterns.py b/measures/patterns.py index ec1739f3e..eda848a15 100644 --- a/measures/patterns.py +++ b/measures/patterns.py @@ -228,20 +228,16 @@ def create_measure_excluded_geographical_areas( m.member for m in GeographicalMembership.objects.as_at( measure.valid_between.lower, - ) - .filter( + ).filter( geo_group=measure.geographical_area, ) - .all() ) - for membership in ( - GeographicalMembership.objects.as_at(measure.valid_between.lower) - .filter(geo_group=exclusion) - .all() - ): + for membership in GeographicalMembership.objects.as_at( + measure.valid_between.lower, + ).filter(geo_group=exclusion): member = membership.member - assert ( - member in measure_origins + assert member.sid in list( + m.sid for m in measure_origins ), f"{member.area_id} not in {list(x.area_id for x in measure_origins)}" yield MeasureExcludedGeographicalArea.objects.create( modified_measure=measure, diff --git a/measures/tests/test_patterns.py b/measures/tests/test_patterns.py index c5e29acc1..e8beed7aa 100644 --- a/measures/tests/test_patterns.py +++ b/measures/tests/test_patterns.py @@ -481,3 +481,57 @@ def test_suspension_pattern_does_not_recreate_old_mfn( == UpdateType.CREATE ) assert not type(suspension).objects.filter(additional_code=None).exists() + + +# https://uktrade.atlassian.net/browse/TP2000-402 +# The above bug occurred because the erga omnes group was created with an early version of Anguilla. +# This country was later changed to a region and another group (Phytosanitary 9) created with this region. +# We then tried to create an erga omnes measure and exclude this Phytosanitary 9 group from it. /PS-IGNORE +# This caused a 500 error because our system didn't see this later version of Anguilla as belonging to erga omnes. +# Therefore, we updated the logic to use the GeographicalArea identifying field `sid`, instead of pk, and disregard version. /PS-IGNORE +# This follows the precedent set by get_current_memberships, which uses sid and means that the erga omnes group shows the latest version of Anguilla as one of its members. /PS-IGNORE +def test_create_measure_excluded_geographical_areas_assertion(measure_creation_pattern): + """ + Tests the assertion in create_measure_excluded_geographical_areas by + creating a group containing two countries, creating a measure with this + group, updating one country to a later version with a different pk but the + same sid, creating a group with this later version of the country, and then + trying to exclude this group from the measure group. + + We expect the assertion to pass and a MeasureExcludedGeographicalArea to be + created with the measure and new version of the country + """ + measure_group = factories.GeographicalAreaFactory.create(area_code=1) + country_1 = factories.GeographicalAreaFactory.create(area_code=0) + country_2 = factories.GeographicalAreaFactory.create(area_code=0) + factories.GeographicalMembershipFactory.create( + geo_group=measure_group, + member=country_1, + ) + factories.GeographicalMembershipFactory.create( + geo_group=measure_group, + member=country_2, + ) + excluded_group = factories.GeographicalAreaFactory.create(area_code=1) + new_country_2 = country_2.new_version( + workbasket=measure_creation_pattern.workbasket, + ) + factories.GeographicalMembershipFactory.create( + geo_group=excluded_group, + member=new_country_2, + ) + measure = factories.MeasureFactory.create(geographical_area=measure_group) + generator = measure_creation_pattern.create_measure_excluded_geographical_areas( + measure=measure, + exclusion=excluded_group, + ) + exclusion = [gen for gen in generator][0] + + assert exclusion.modified_measure == measure + + # sanity check assumptions about result of calling new_version /PS-IGNORE + assert country_2.sid == new_country_2.sid + assert country_2.pk != new_country_2.pk + + assert exclusion.excluded_geographical_area.sid == country_2.sid + assert exclusion.excluded_geographical_area.pk == new_country_2.pk From cbe0c23ef49eb1b477ccd1f13118fdc89abde4da Mon Sep 17 00:00:00 2001 From: Paul Pepper <85895113+paulpepper-trade@users.noreply.github.com> Date: Mon, 25 Jul 2022 16:22:48 +0100 Subject: [PATCH 13/28] TP2000-286 multiple workbaskets (#627) --- additional_codes/tests/test_views.py | 7 +- certificates/tests/test_views.py | 7 +- .../jinja2/commodities/import-success.jinja | 2 +- commodities/jinja2/commodities/import.jinja | 2 +- common/forms.py | 26 ++ common/jinja2/common/confirm_create.jinja | 4 +- .../common/confirm_create_description.jinja | 12 +- common/jinja2/common/confirm_update.jinja | 2 +- .../common/confirm_update_description.jinja | 12 +- common/jinja2/common/create_description.jinja | 2 +- .../common/{index.jinja => dashboard.jinja} | 21 +- common/jinja2/common/delete.jinja | 4 +- common/jinja2/common/edit.jinja | 2 +- common/jinja2/common/edit_description.jinja | 2 +- common/jinja2/common/workbasket_action.jinja | 21 ++ common/jinja2/layouts/confirm.jinja | 10 +- common/jinja2/layouts/detail.jinja | 4 +- common/jinja2/layouts/layout.jinja | 43 ++- common/jinja2/layouts/list.jinja | 2 +- common/jinja2/layouts/list_vertical.jinja | 2 +- common/static/common/scss/_components.scss | 45 ++++ common/static/common/scss/_text.scss | 24 ++ common/static/common/scss/application.scss | 4 +- common/tests/factories.py | 13 +- common/tests/test_views.py | 139 +++------- common/urls.py | 2 +- common/views.py | 166 +----------- .../confirm_update_description.jinja | 6 +- .../jinja2/footnotes/edit_description.jinja | 2 +- footnotes/tests/test_views.py | 7 +- geo_areas/jinja2/geo_areas/detail.jinja | 2 +- geo_areas/tests/test_views.py | 7 +- importer/jinja2/importer/create.jinja | 2 +- .../measures/confirm-create-multiple.jinja | 10 +- measures/jinja2/measures/create-start.jinja | 2 +- measures/jinja2/measures/edit.jinja | 2 +- measures/tests/test_views.py | 7 +- pii-ner-exclude.txt | 6 + regulations/jinja2/regulations/edit.jinja | 2 +- regulations/tests/test_views.py | 14 +- settings/common.py | 2 +- workbaskets/forms.py | 43 +++ .../workbaskets/edit-workbasket-list.jinja | 37 +++ .../workbasket-selectable-items.jinja | 55 ++-- .../jinja2/workbaskets/confirm_create.jinja | 28 ++ workbaskets/jinja2/workbaskets/create.jinja | 13 + .../jinja2/workbaskets/delete_changes.jinja | 2 +- .../workbaskets/delete_changes_confirm.jinja | 2 +- workbaskets/jinja2/workbaskets/detail.jinja | 2 +- .../jinja2/workbaskets/edit-workbasket.jinja | 150 +++++++++++ .../workbaskets/preview-workbasket.jinja | 47 ++++ .../workbaskets/review-workbasket.jinja | 35 +++ .../workbaskets/select-workbasket.jinja | 45 ++++ workbaskets/models.py | 1 + workbaskets/tests/test_forms.py | 50 ++++ workbaskets/tests/test_views.py | 148 ++++++++++- workbaskets/urls.py | 32 ++- workbaskets/views/ui.py | 246 +++++++++++++++--- 58 files changed, 1156 insertions(+), 431 deletions(-) rename common/jinja2/common/{index.jinja => dashboard.jinja} (92%) create mode 100644 common/jinja2/common/workbasket_action.jinja create mode 100644 common/static/common/scss/_components.scss create mode 100644 common/static/common/scss/_text.scss create mode 100644 workbaskets/jinja2/includes/workbaskets/edit-workbasket-list.jinja create mode 100644 workbaskets/jinja2/workbaskets/confirm_create.jinja create mode 100644 workbaskets/jinja2/workbaskets/create.jinja create mode 100644 workbaskets/jinja2/workbaskets/edit-workbasket.jinja create mode 100644 workbaskets/jinja2/workbaskets/preview-workbasket.jinja create mode 100644 workbaskets/jinja2/workbaskets/review-workbasket.jinja create mode 100644 workbaskets/jinja2/workbaskets/select-workbasket.jinja create mode 100644 workbaskets/tests/test_forms.py diff --git a/additional_codes/tests/test_views.py b/additional_codes/tests/test_views.py index ec5b666e2..adf0ffc3e 100644 --- a/additional_codes/tests/test_views.py +++ b/additional_codes/tests/test_views.py @@ -65,7 +65,12 @@ def test_additional_code_delete_form(factory, use_delete_form): ), ids=view_urlpattern_ids, ) -def test_additional_codes_detail_views(view, url_pattern, valid_user_client): +def test_additional_codes_detail_views( + view, + url_pattern, + valid_user_client, + session_with_workbasket, +): """Verify that additional code detail views are under the url additional_codes/ and don't return an error.""" model_overrides = { diff --git a/certificates/tests/test_views.py b/certificates/tests/test_views.py index 47cb87484..ca6ec06f7 100644 --- a/certificates/tests/test_views.py +++ b/certificates/tests/test_views.py @@ -58,7 +58,12 @@ def test_certificate_create_form_creates_certificate_description_object( ), ids=view_urlpattern_ids, ) -def test_certificate_detail_views(view, url_pattern, valid_user_client): +def test_certificate_detail_views( + view, + url_pattern, + valid_user_client, + session_with_workbasket, +): """Verify that certificate detail views are under the url certificates/ and don't return an error.""" model_overrides = { diff --git a/commodities/jinja2/commodities/import-success.jinja b/commodities/jinja2/commodities/import-success.jinja index 4dd070f3b..c9708a9d8 100644 --- a/commodities/jinja2/commodities/import-success.jinja +++ b/commodities/jinja2/commodities/import-success.jinja @@ -7,7 +7,7 @@ {% block beforeContent %} {{ govukBreadcrumbs({ "items": [ - {"text": "Home", "href": url("index")}, + {"text": "Home", "href": url("home")}, {"text": page_title} ] }) }} diff --git a/commodities/jinja2/commodities/import.jinja b/commodities/jinja2/commodities/import.jinja index 193374305..2f184f0c4 100644 --- a/commodities/jinja2/commodities/import.jinja +++ b/commodities/jinja2/commodities/import.jinja @@ -7,7 +7,7 @@ {% block beforeContent %} {{ govukBreadcrumbs({ "items": [ - {"text": "Home", "href": url("index")}, + {"text": "Home", "href": url("home")}, {"text": page_title} ] }) }} diff --git a/common/forms.py b/common/forms.py index ee50443ab..1fe1bf9d0 100644 --- a/common/forms.py +++ b/common/forms.py @@ -12,6 +12,7 @@ from django import forms from django.contrib.postgres.forms.ranges import DateRangeField from django.core.exceptions import ValidationError +from django.db.models import TextChoices from django.forms import TypedChoiceField from django.forms import formsets from django.forms.renderers import get_default_renderer @@ -153,6 +154,31 @@ def get_bound_field(self, form, field_name): return super().get_bound_field(form, field_name) +class HomeForm(forms.Form): + class WorkbasketActions(TextChoices): + CREATE = "CREATE", "Create a new workbasket" + EDIT = ( + "EDIT", + "Edit an existing workbasket", + ) + + workbasket_action = forms.ChoiceField( + label="What would you like to do?", + choices=WorkbasketActions.choices, + widget=forms.RadioSelect, + required=True, + ) + + def __init__(self, *args, **kwargs): + super().__init__(*args, **kwargs) + self.helper = FormHelper(self) + self.helper.legend_size = Size.EXTRA_LARGE + self.helper.layout = Layout( + "workbasket_action", + Submit("submit", "Continue"), + ) + + class DescriptionHelpBox(Div): template = "components/description_help.jinja" diff --git a/common/jinja2/common/confirm_create.jinja b/common/jinja2/common/confirm_create.jinja index ce0c2e11f..a99d6a513 100644 --- a/common/jinja2/common/confirm_create.jinja +++ b/common/jinja2/common/confirm_create.jinja @@ -5,7 +5,7 @@ {% block beforeContent %} {{ govukBreadcrumbs({ "items": [ - {"text": "Home", "href": url("index")}, + {"text": "Home", "href": url("home")}, {"text": page_title} ] }) }} @@ -17,4 +17,4 @@ "text": "This new " ~ object._meta.verbose_name ~ " has been added to your tariff changes", "classes": "govuk-!-margin-bottom-7" }) }} -{% endblock %} \ No newline at end of file +{% endblock %} diff --git a/common/jinja2/common/confirm_create_description.jinja b/common/jinja2/common/confirm_create_description.jinja index 45a0225d1..859f5008c 100644 --- a/common/jinja2/common/confirm_create_description.jinja +++ b/common/jinja2/common/confirm_create_description.jinja @@ -10,7 +10,7 @@ {% block beforeContent %} {{ govukBreadcrumbs({ "items": [ - {"text": "Home", "href": url("index")}, + {"text": "Home", "href": url("home")}, {"text": "Find and edit " ~ described_object._meta.verbose_name_plural, "href": described_object.get_url("list")}, {"text": described_object._meta.verbose_name|capitalize ~ ": " ~ described_object|string, "href": described_object.get_url()}, {"text": page_title} @@ -23,7 +23,7 @@
Confirmation

{{ page_title }}

- + {{ govukPanel({ "titleText": "A new description of " ~ described_object._meta.verbose_name ~ " " ~ described_object|string ~ " has been created", "text": "This new description has been added to your tariff changes", @@ -32,15 +32,19 @@

Next steps

To complete your task you must publish your change.

+ {% if request.session.workbasket %} {{ govukButton({ "text": "Go to your tariff changes", - "href": url("index"), + "href": url("workbaskets:edit-workbasket", args=[request.session.workbasket.id]), "classes": "govuk-button--secondary" }) }} + {% endif %}
diff --git a/common/jinja2/common/confirm_update.jinja b/common/jinja2/common/confirm_update.jinja index cd3f600e7..d931c3198 100644 --- a/common/jinja2/common/confirm_update.jinja +++ b/common/jinja2/common/confirm_update.jinja @@ -5,7 +5,7 @@ {% block beforeContent %} {{ govukBreadcrumbs({ "items": [ - {"text": "Home", "href": url("index")}, + {"text": "Home", "href": url("home")}, {"text": "Find and edit " ~ object._meta.verbose_name_plural, "href": object.get_url("list")}, {"text": object._meta.verbose_name|capitalize ~ ": " ~ object|string, "href": object.get_url()}, {"text": page_title} diff --git a/common/jinja2/common/confirm_update_description.jinja b/common/jinja2/common/confirm_update_description.jinja index 4358ee5e9..dd703bf05 100644 --- a/common/jinja2/common/confirm_update_description.jinja +++ b/common/jinja2/common/confirm_update_description.jinja @@ -10,7 +10,7 @@ {% block beforeContent %} {{ govukBreadcrumbs({ "items": [ - {"text": "Home", "href": url("index")}, + {"text": "Home", "href": url("home")}, {"text": "Find and edit " ~ described_object._meta.verbose_name_plural, "href": described_object.get_url("list")}, {"text": described_object._meta.verbose_name|capitalize ~ ": " ~ described_object|string, "href": described_object.get_url()}, {"text": page_title} @@ -23,7 +23,7 @@
Confirmation

{{ page_title }}

- + {{ govukPanel({ "titleText": "Description of " ~ described_object._meta.verbose_name ~ " " ~ described_object|string ~ " has been updated", "text": "This change has been added to your tariff changes", @@ -32,15 +32,19 @@

Next steps

To complete your task you must publish your change.

+ {% if request.session.workbasket %} {{ govukButton({ "text": "Go to your tariff changes", - "href": url("index"), + "href": url("workbaskets:review-workbasket", args=[request.session.workbasket.id]), "classes": "govuk-button--secondary" }) }} + {% endif %}
diff --git a/common/jinja2/common/create_description.jinja b/common/jinja2/common/create_description.jinja index cd721e763..8ac7060fe 100644 --- a/common/jinja2/common/create_description.jinja +++ b/common/jinja2/common/create_description.jinja @@ -43,7 +43,7 @@ {% block beforeContent %} {{ govukBreadcrumbs({ "items": [ - {"text": "Home", "href": url("index")}, + {"text": "Home", "href": url("home")}, {"text": "Find and edit " ~ described_object._meta.verbose_name_plural, "href": described_object.get_url("list")}, {"text": described_object._meta.verbose_name|capitalize ~ ": " ~ described_object|string, "href": described_object.get_url()}, {"text": page_title} diff --git a/common/jinja2/common/index.jinja b/common/jinja2/common/dashboard.jinja similarity index 92% rename from common/jinja2/common/index.jinja rename to common/jinja2/common/dashboard.jinja index 920a4dfe6..9aa516aa4 100644 --- a/common/jinja2/common/index.jinja +++ b/common/jinja2/common/dashboard.jinja @@ -3,18 +3,19 @@ {% from "components/inset-text/macro.njk" import govukInsetText %} {% set page_title = "Manage trade tariffs" %} +{% set page_subtitle = "This service lets you manage UK trade tariffs" %} {% block content %} -
-
-

- {{ page_title }} - - This service lets you manage UK trade tariffs - -

-
-
+

+ {% block page_title %} + {{ page_title }} + {% endblock %} + + {% block page_subtitle %} + {{ page_subtitle }} + {% endblock %} + +

diff --git a/common/jinja2/common/delete.jinja b/common/jinja2/common/delete.jinja index f709e6d58..48bfba5c4 100644 --- a/common/jinja2/common/delete.jinja +++ b/common/jinja2/common/delete.jinja @@ -7,7 +7,7 @@ {% block beforeContent %} {{ govukBreadcrumbs({ "items": [ - {"text": "Home", "href": url("index")}, + {"text": "Home", "href": url("home")}, {"text": "Find and edit " ~ object._meta.verbose_name_plural, "href": object.get_url("list")}, {"text": object._meta.verbose_name|capitalize ~ ": " ~ object|string, "href": object.get_url()}, {"text": page_title} @@ -23,4 +23,4 @@ {{ crispy(form) }} {% endcall %} Cancel -{% endblock %} \ No newline at end of file +{% endblock %} diff --git a/common/jinja2/common/edit.jinja b/common/jinja2/common/edit.jinja index b6233b7a8..8d87070b6 100644 --- a/common/jinja2/common/edit.jinja +++ b/common/jinja2/common/edit.jinja @@ -5,7 +5,7 @@ {% block beforeContent %} {{ govukBreadcrumbs({ "items": [ - {"text": "Home", "href": url("index")}, + {"text": "Home", "href": url("home")}, {"text": "Find and edit " ~ object._meta.verbose_name_plural, "href": object.get_url("list")}, {"text": object._meta.verbose_name|capitalize ~ ": " ~ object|string, "href": object.get_url()}, {"text": page_title} diff --git a/common/jinja2/common/edit_description.jinja b/common/jinja2/common/edit_description.jinja index 5ae907c8f..cab5548bb 100644 --- a/common/jinja2/common/edit_description.jinja +++ b/common/jinja2/common/edit_description.jinja @@ -45,7 +45,7 @@ {% block beforeContent %} {{ govukBreadcrumbs({ "items": [ - {"text": "Home", "href": url("index")}, + {"text": "Home", "href": url("home")}, {"text": "Find and edit " ~ described_object._meta.verbose_name_plural, "href": described_object.get_url("list")}, {"text": described_object._meta.verbose_name|capitalize ~ ": " ~ described_object|string, "href": described_object.get_url()}, {"text": page_title} diff --git a/common/jinja2/common/workbasket_action.jinja b/common/jinja2/common/workbasket_action.jinja new file mode 100644 index 000000000..706b3ecc9 --- /dev/null +++ b/common/jinja2/common/workbasket_action.jinja @@ -0,0 +1,21 @@ +{% extends "layouts/form.jinja" %} + +{% set page_title = "Tariff Management Tool" %} + +{% block content %} +

{{ page_title }}

+ This service lets you manage UK trade tariffs + + {% block form %} +
+
+ {% call django_form() %} + {{ crispy(form) }} + {% endcall %} +
+
+ {% endblock %} +{% endblock %} + + + diff --git a/common/jinja2/layouts/confirm.jinja b/common/jinja2/layouts/confirm.jinja index 3b6711c19..c509cc198 100644 --- a/common/jinja2/layouts/confirm.jinja +++ b/common/jinja2/layouts/confirm.jinja @@ -6,7 +6,7 @@ {% block beforeContent %} {{ govukBreadcrumbs({ "items": [ - {"text": "Home", "href": url("index")}, + {"text": "Home", "href": url("home")}, {"text": "Find and edit " ~ object._meta.verbose_name_plural, "href": object.get_url("list")}, {"text": object._meta.verbose_name|capitalize ~ ": " ~ object|string, "href": object.get_url()}, {"text": page_title} @@ -22,15 +22,19 @@ {% block panel %}{% endblock%}

Next steps

To complete your task you must publish your change.

+ {% if request.session.workbasket %} {{ govukButton({ "text": "Go to your tariff changes", - "href": url("index"), + "href": url("workbaskets:review-workbasket", args=[request.session.workbasket.id]), "classes": "govuk-button--secondary" }) }} + {% endif %}
diff --git a/common/jinja2/layouts/detail.jinja b/common/jinja2/layouts/detail.jinja index a7e190034..81555bd57 100644 --- a/common/jinja2/layouts/detail.jinja +++ b/common/jinja2/layouts/detail.jinja @@ -3,7 +3,7 @@ {% block breadcrumb %} {{ govukBreadcrumbs({ "items": [ - {"text": "Home", "href": url("index")}, + {"text": "Home", "href": url("home")}, {"text": "Find and edit " ~ object._meta.verbose_name_plural, "href": object.get_url(action="list")}, {"text": page_title} ] @@ -17,4 +17,4 @@ {{ govukTabs(tabs) }} -{% endblock %} \ No newline at end of file +{% endblock %} diff --git a/common/jinja2/layouts/layout.jinja b/common/jinja2/layouts/layout.jinja index 24925a0af..0be6bca4e 100644 --- a/common/jinja2/layouts/layout.jinja +++ b/common/jinja2/layouts/layout.jinja @@ -9,17 +9,39 @@ {{ page_title }} | {{ service_name }} {% endblock %} +{% set workbasket_svg %} + {# /PS-IGNORE #} + {# /PS-IGNORE #} + {# /PS-IGNORE #} + +{% endset %} + {% block header %} - {{ govukHeader({ - "serviceName": service_name, - "serviceUrl": "#", - "navigation": [ - { - "href": url("logout") if request.user.is_authenticated else settings.LOGIN_URL, - "text": "Sign out" if request.user.is_authenticated else "Sign In" - } - ] - }) }} +{% set workbasket_url %} + {% if request.session.workbasket %} + {{ url('workbaskets:workbasket-ui-detail', args=[request.session.workbasket.id]) }} + {% else %} + {% endif %} +{% endset %} + +{% set workbasket_html %} + {% if request.session.workbasket %} + Workbasket {{ request.session.workbasket.id }} {{ workbasket_svg }} + {% endif %} +{% endset %} + +{{ govukHeader({ + "homepageUrl": "https://gov.uk/", + "serviceName": service_name, + "serviceUrl": "/", + "navigation": [ + { "html": workbasket_html } if request.session.workbasket else "", + { + "href": url("logout") if request.user.is_authenticated else settings.LOGIN_URL, + "text": "Sign out" if request.user.is_authenticated else "Sign In" + }, + ] +}) }} {% endblock %} {% block beforeContent %} {{ govukPhaseBanner({ @@ -31,4 +53,3 @@ {% block breadcrumb %} {% endblock %} {% endblock %} - diff --git a/common/jinja2/layouts/list.jinja b/common/jinja2/layouts/list.jinja index 735ecfab8..eb8a44997 100644 --- a/common/jinja2/layouts/list.jinja +++ b/common/jinja2/layouts/list.jinja @@ -11,7 +11,7 @@ {% block breadcrumb %} {{ govukBreadcrumbs({ "items": [ - {"text": "Home", "href": url("index")}, + {"text": "Home", "href": url("home")}, {"text": page_title} ] }) }} diff --git a/common/jinja2/layouts/list_vertical.jinja b/common/jinja2/layouts/list_vertical.jinja index 5ad7553ab..726c5248f 100644 --- a/common/jinja2/layouts/list_vertical.jinja +++ b/common/jinja2/layouts/list_vertical.jinja @@ -11,7 +11,7 @@ {% block breadcrumb %} {{ govukBreadcrumbs({ "items": [ - {"text": "Home", "href": url("index")}, + {"text": "Home", "href": url("home")}, {"text": page_title} ] }) }} diff --git a/common/static/common/scss/_components.scss b/common/static/common/scss/_components.scss new file mode 100644 index 000000000..73ad4cc43 --- /dev/null +++ b/common/static/common/scss/_components.scss @@ -0,0 +1,45 @@ +.workbasket-filters { + + > * { + display: inline-block; + } + + .govuk-list { + margin: 0; + + li { + display: inline-block; + padding-right: 25px; + padding-left: 25px; + } + } + + .govuk-link { + color: govuk-colour("black"); + } + + .selected-link { + color: govuk-colour("white"); + background-color: govuk-colour("blue"); + text-decoration: none; + } + + .selected-link:visited { + color: govuk-colour("white"); + text-decoration: none; + } + + .selected-link:hover { + text-decoration: underline; + } +} + +.govuk-header { + + .workbasket-link { + + svg { + margin-bottom: -4px; + } + } +} diff --git a/common/static/common/scss/_text.scss b/common/static/common/scss/_text.scss new file mode 100644 index 000000000..11f3b8ad5 --- /dev/null +++ b/common/static/common/scss/_text.scss @@ -0,0 +1,24 @@ + +.status-badge { + font-weight: bold; + color: govuk-colour("white"); + background-color: govuk-colour("blue"); + padding: 0px 10px; +} + +// makes a button or input element look like a hyperlink +.button-link { + background: none; + border: none; + padding: 0; + font-family: sans-serif; + font-size: inherit; + text-decoration: underline; + cursor: pointer; + color: govuk-colour("blue"); + @extend .govuk-link; + + &:focus { + background-color: govuk-colour("yellow"); + } +} diff --git a/common/static/common/scss/application.scss b/common/static/common/scss/application.scss index 34a5f3e38..f11513908 100644 --- a/common/static/common/scss/application.scss +++ b/common/static/common/scss/application.scss @@ -14,6 +14,8 @@ $govuk-image-url-function: frontend-image-url; @import "icon-action-list"; @import "layout"; +@import "text"; +@import "components"; @import "autocomplete"; @import "accordion"; @import "_table"; @@ -25,4 +27,4 @@ $govuk-image-url-function: frontend-image-url; @import "step-by-step-header"; @import "step-by-step-nav"; @import "step-by-step-related"; -@import "fake-link"; \ No newline at end of file +@import "fake-link"; diff --git a/common/tests/factories.py b/common/tests/factories.py index 4c1beee01..364ab3b15 100644 --- a/common/tests/factories.py +++ b/common/tests/factories.py @@ -118,7 +118,7 @@ class Meta: model = "workbaskets.WorkBasket" author = factory.SubFactory(UserFactory) - title = factory.Faker("text", max_nb_chars=255) + title = factory.Faker("sentence", nb_words=4) class ApprovedWorkBasketFactory(WorkBasketFactory): @@ -958,13 +958,12 @@ class Meta: class MeasureActionFactory(TrackedModelMixin, ValidityFactoryMixin): """ - MeasureActions in the TaMaTo are essentially fixed, it - would be more realistic to test using a fixed list, however it - is convenient. + MeasureActions in the TaMaTo are essentially fixed, it would be more + realistic to test using a fixed list, however it is convenient. - As MeasureActionFactory is used in tests, it is possible to - generate more than 999 MeasureActions, to avoid creating MeasureAction codes - with four digits, which is not allowed, the code wraps back to 000 every 1000 + As MeasureActionFactory is used in tests, it is possible to generate more + than 999 MeasureActions, to avoid creating MeasureAction codes with four + digits, which is not allowed, the code wraps back to 000 every 1000 iterations. """ diff --git a/common/tests/test_views.py b/common/tests/test_views.py index c4b90c6a4..494d0eba4 100644 --- a/common/tests/test_views.py +++ b/common/tests/test_views.py @@ -4,83 +4,49 @@ from bs4 import BeautifulSoup from django.urls import reverse -from common.tests import factories -from common.tests.factories import GoodsNomenclatureFactory -from common.views import DashboardView from common.views import HealthCheckResponse -from workbaskets.forms import SelectableObjectsForm -from workbaskets.models import WorkBasket -from workbaskets.validators import WorkflowStatus pytestmark = pytest.mark.django_db -def test_index_creates_workbasket_if_needed(valid_user_client, approved_workbasket): - assert WorkBasket.objects.is_not_approved().count() == 0 - response = valid_user_client.get(reverse("index")) - assert response.status_code == 200 - assert WorkBasket.objects.is_not_approved().count() == 1 - - -def test_index_doesnt_creates_workbasket_if_not_needed( - valid_user_client, - new_workbasket, -): - assert WorkBasket.objects.is_not_approved().count() == 1 - response = valid_user_client.get(reverse("index")) - assert response.status_code == 200 - assert WorkBasket.objects.is_not_approved().count() == 1 - - -def test_index_workbasket_unaffected_by_archived_workbasket( - valid_user_client, -): - response = valid_user_client.get(reverse("index")) - assert response.status_code == 200 - view = response.context_data["view"] - view_workbasket = view.workbasket +def test_index_displays_workbasket_action_form(valid_user_client): + response = valid_user_client.get(reverse("home")) - factories.WorkBasketFactory.create(status=WorkflowStatus.ARCHIVED) - response = valid_user_client.get(reverse("index")) assert response.status_code == 200 - view = response.context_data["view"] - assert view.workbasket == view_workbasket - factories.WorkBasketFactory.create(status=WorkflowStatus.EDITING) - response = valid_user_client.get(reverse("index")) - assert response.status_code == 200 - view = response.context_data["view"] - assert view.workbasket == view_workbasket + page = BeautifulSoup(str(response.content), "html.parser") + assert "What would you like to do?" in page.select("legend")[0].text + assert "Edit an existing workbasket" in page.select("label")[1].text + assert "Create a new workbasket" in page.select("label")[0].text -def test_index_displays_objects_in_current_workbasket( - valid_user_client, - workbasket, +@pytest.mark.parametrize( + ("data", "response_url"), + ( + ( + { + "workbasket_action": "EDIT", + }, + "workbaskets:workbasket-ui-list", + ), + ( + { + "workbasket_action": "CREATE", + }, + "workbaskets:workbasket-ui-create", + ), + ), +) +def test_workbasket_action_form_response_redirects_user( + valid_user, + client, + data, + response_url, ): - """Verify that changes in the current workbasket are displayed on the bulk - selection form of the index page.""" - with workbasket.new_transaction(): - GoodsNomenclatureFactory.create() - - response = valid_user_client.get(reverse("index")) - page = BeautifulSoup( - response.content.decode(response.charset), - features="lxml", - ) - for obj in workbasket.tracked_models.all(): - field_name = SelectableObjectsForm.field_name_for_object(obj) - assert page.find("input", {"name": field_name}) - - -def test_index_with_each_type_of_object_in_current_workbasket( - valid_user_client, - workbasket, - trackedmodel_factory, -): - with workbasket.new_transaction(): - trackedmodel_factory.create() - response = valid_user_client.get(reverse("index")) - assert response.status_code == 200 + client.force_login(valid_user) + response = client.post(reverse("home"), data) + assert response.status_code == 302 + assert response.url == reverse(response_url) @pytest.mark.parametrize( @@ -97,44 +63,3 @@ def test_healthcheck_response(response, status_code, status): assert payload[0].tag == "status" assert payload[0].text == status assert payload[1].tag == "response_time" - - -def test_handles_multiple_unapproved_workbaskets(valid_user_client, new_workbasket): - workbasket = factories.WorkBasketFactory.create( - status=WorkflowStatus.EDITING, - ) - transaction = factories.TransactionFactory.create(workbasket=workbasket) - with transaction: - factories.FootnoteTypeFactory.create_batch(2) - - assert WorkBasket.objects.is_not_approved().count() == 2 - - response = valid_user_client.get(reverse("index")) - - assert response.status_code == 200 - - -def test_dashboard_view_uploaded_envelope_dates(): - envelope = factories.EnvelopeFactory.create() - first_txn = factories.EnvelopeTransactionFactory.create( - envelope=envelope, - ).transaction - last_txn = factories.EnvelopeTransactionFactory.create( - envelope=envelope, - ).transaction - factories.UploadFactory.create(envelope=envelope) - view = DashboardView() - - assert view.uploaded_envelope_dates["start"] == first_txn.updated_at - assert view.uploaded_envelope_dates["end"] == last_txn.updated_at - - -def test_dashboard_view_latest_upload(): - view = DashboardView() - - assert view.latest_upload == None - - factories.UploadFactory.create() - latest_upload = factories.UploadFactory.create() - - assert view.latest_upload == latest_upload diff --git a/common/urls.py b/common/urls.py index 929c20850..82264cffc 100644 --- a/common/urls.py +++ b/common/urls.py @@ -14,7 +14,7 @@ register_converter(NumericSIDConverter, "sid") urlpatterns = [ - path("", views.DashboardView.as_view(), name="index"), + path("", views.HomeView.as_view(), name="home"), path("healthcheck", views.healthcheck, name="healthcheck"), path("login", views.LoginView.as_view(), name="login"), path("logout", views.LogoutView.as_view(), name="logout"), diff --git a/common/views.py b/common/views.py index 42b27170c..b48536c4d 100644 --- a/common/views.py +++ b/common/views.py @@ -17,179 +17,33 @@ from django.db.models import QuerySet from django.http import Http404 from django.http import HttpResponse +from django.shortcuts import redirect from django.urls import reverse -from django.utils.decorators import method_decorator from django.views import generic -from django.views.generic.base import TemplateResponseMixin +from django.views.generic import FormView from django.views.generic.base import View from django.views.generic.edit import FormMixin from django_filters.views import FilterView from redis.exceptions import TimeoutError as RedisTimeoutError +from common import forms from common.business_rules import BusinessRule from common.business_rules import BusinessRuleViolation from common.models import TrackedModel from common.pagination import build_pagination_list from common.validators import UpdateType -from exporter.models import Upload -from workbaskets.forms import SelectableObjectsForm -from workbaskets.models import WorkBasket -from workbaskets.session_store import SessionStore -from workbaskets.views.decorators import require_current_workbasket from workbaskets.views.mixins import WithCurrentWorkBasket -@method_decorator(require_current_workbasket, name="dispatch") -class DashboardView(TemplateResponseMixin, FormMixin, View): - """ - UI endpoint providing a dashboard view, including a WorkBasket (list) of - paged user-selectable TrackedModel instances (items). Pages contain a - configurable maximum number of items. - - Items are selectable (user selections) via an associated checkbox - widget so that bulk operations may be performed against them. - - Each page displays a maximum number of items (10 being the default), with - user selections preserved when navigating between pages. - - Item selection is preserved in the user's session. Whenever page navigation - or a bulk operation is performed, item selection state is updated. - - User item selection changes are calculated and applied to the Django - Session object by performing a three way diff between: - * Current selection state held in the session, - * Available list items (have any new items been added, or somehow removed), - * User submitted changes (from form POST requests) - - Note: - -- - Options considered to manage paged selection: - * Full list in form, hiding items not on current page. This would require - either including all items in GET request's URI after POST, dropping use - of GET after POST, neither seem very reasonable. - * Use django formtools wizard. This doesn't fit the wizard's usecase and - design very well and may make for an over complicated implementation. - * Store user selection in the session. Simplest, complete and most elegant. - """ - - form_class = SelectableObjectsForm - template_name = "common/index.jinja" - - # Form action mappings to URL names. - action_success_url_names = { - "publish-all": "workbaskets:workbasket-ui-submit", - "remove-selected": "workbaskets:workbasket-ui-delete-changes", - "page-prev": "index", - "page-next": "index", - } - - @property - def workbasket(self) -> WorkBasket: - return WorkBasket.current(self.request) - - @property - def paginator(self): - return Paginator(self.workbasket.tracked_models, per_page=10) - - @property - def latest_upload(self): - return Upload.objects.order_by("created_date").last() - - @property - def uploaded_envelope_dates(self): - """Gets a list of all transactions from the `latest_approved_workbasket` - in the order they were updated and returns a dict with the first and - last transactions as values for "start" and "end" keys respectively.""" - if self.latest_upload: - transactions = self.latest_upload.envelope.transactions.order_by( - "updated_at", - ) - return { - "start": transactions.first().updated_at, - "end": transactions.last().updated_at, - } - return None - - def _append_url_page_param(self, url, form_action): - """Based upon 'form_action', append a 'page' URL parameter to the given - url param and return the result.""" - page = self.paginator.get_page(self.request.GET.get("page", 1)) - page_number = 1 - if form_action == "page-prev": - page_number = page.previous_page_number() - elif form_action == "page-next": - page_number = page.next_page_number() - return f"{url}?page={page_number}" - - def get(self, request, *args, **kwargs): - """Service GET requests by displaying the page and form.""" - return self.render_to_response(self.get_context_data()) - - def post(self, request, *args, **kwargs): - """Manage POST requests, which can either be requests to change the - paged form data while preserving the user's form changes, or finally - submit the form data for processing.""" - form = self.get_form() - if form.is_valid(): - return self.form_valid(form) - else: - return self.form_invalid(form) - - def get_success_url(self): - form_action = self.request.POST.get("form-action") - if form_action in ("publish-all", "remove-selected"): - return reverse( - self.action_success_url_names[form_action], - kwargs={"pk": self.workbasket.pk}, - ) - elif form_action in ("page-prev", "page-next"): - return self._append_url_page_param( - reverse(self.action_success_url_names[form_action]), - form_action, - ) - return reverse("index") - - def get_initial(self): - store = SessionStore( - self.request, - f"WORKBASKET_SELECTIONS_{self.workbasket.pk}", - ) - return store.data.copy() - - def get_form_kwargs(self): - kwargs = super().get_form_kwargs() - page = self.paginator.get_page(self.request.GET.get("page", 1)) - kwargs["objects"] = page.object_list - return kwargs - - def get_context_data(self, **kwargs): - context = super().get_context_data(**kwargs) - page = self.paginator.get_page(self.request.GET.get("page", 1)) - context.update( - { - "workbasket": self.workbasket, - "page_obj": page, - "uploaded_envelope_dates": self.uploaded_envelope_dates, - }, - ) - return context +class HomeView(FormView, View): + template_name = "common/workbasket_action.jinja" + form_class = forms.HomeForm def form_valid(self, form): - store = SessionStore( - self.request, - f"WORKBASKET_SELECTIONS_{self.workbasket.pk}", - ) - to_add = { - key: value for key, value in form.cleaned_data_no_prefix.items() if value - } - to_remove = { - key: value - for key, value in form.cleaned_data_no_prefix.items() - if key not in to_add - } - store.add_items(to_add) - store.remove_items(to_remove) - return super().form_valid(form) + if form.cleaned_data["workbasket_action"] == "EDIT": + return redirect(reverse("workbaskets:workbasket-ui-list")) + elif form.cleaned_data["workbasket_action"] == "CREATE": + return redirect(reverse("workbaskets:workbasket-ui-create")) class HealthCheckResponse(HttpResponse): diff --git a/footnotes/jinja2/footnotes/confirm_update_description.jinja b/footnotes/jinja2/footnotes/confirm_update_description.jinja index 79d7c961d..86ea9125d 100644 --- a/footnotes/jinja2/footnotes/confirm_update_description.jinja +++ b/footnotes/jinja2/footnotes/confirm_update_description.jinja @@ -7,7 +7,7 @@ {% block beforeContent %} {{ govukBreadcrumbs({ "items": [ - {"text": "Home", "href": url("index")}, + {"text": "Home", "href": url("home")}, {"text": "Footnotes", "href": url("footnote-ui-list")}, {"text": "Footnote " ~ object.described_footnote|string, "href": object.described_footnote.get_url()}, {"text": page_title} @@ -25,7 +25,7 @@ {% endblock %} diff --git a/footnotes/jinja2/footnotes/edit_description.jinja b/footnotes/jinja2/footnotes/edit_description.jinja index e7919ff66..842f9b7f9 100644 --- a/footnotes/jinja2/footnotes/edit_description.jinja +++ b/footnotes/jinja2/footnotes/edit_description.jinja @@ -9,7 +9,7 @@ {% block beforeContent %} {{ govukBreadcrumbs({ "items": [ - {"text": "Home", "href": url("index")}, + {"text": "Home", "href": url("home")}, {"text": "Footnotes", "href": url("footnote-ui-list")}, {"text": "Footnote " ~ object.described_footnote|string, "href": object.described_footnote.get_url()}, {"text": page_title} diff --git a/footnotes/tests/test_views.py b/footnotes/tests/test_views.py index 41a872dfc..3f776ee67 100644 --- a/footnotes/tests/test_views.py +++ b/footnotes/tests/test_views.py @@ -102,7 +102,12 @@ def test_delete_form(factory, use_delete_form): ), ids=view_urlpattern_ids, ) -def test_footnote_detail_views(view, url_pattern, valid_user_client): +def test_footnote_detail_views( + view, + url_pattern, + valid_user_client, + session_with_workbasket, +): """Verify that measure detail views are under the url footnotes/ and don't return an error.""" model_overrides = {"footnotes.views.FootnoteDescriptionCreate": Footnote} diff --git a/geo_areas/jinja2/geo_areas/detail.jinja b/geo_areas/jinja2/geo_areas/detail.jinja index ae0197544..3dd6bfff2 100644 --- a/geo_areas/jinja2/geo_areas/detail.jinja +++ b/geo_areas/jinja2/geo_areas/detail.jinja @@ -15,7 +15,7 @@ {% block breadcrumb %} {{ govukBreadcrumbs({ "items": [ - {"text": "Home", "href": url("index")}, + {"text": "Home", "href": url("home")}, {"text": "Find and edit geographical areas", "href": url("geo_area-ui-list")}, {"text": page_title} ] diff --git a/geo_areas/tests/test_views.py b/geo_areas/tests/test_views.py index 78c6b7bb8..d68fa917f 100644 --- a/geo_areas/tests/test_views.py +++ b/geo_areas/tests/test_views.py @@ -29,7 +29,12 @@ def test_geo_area_delete(factory, use_delete_form): ), ids=view_urlpattern_ids, ) -def test_geographical_area_detail_views(view, url_pattern, valid_user_client): +def test_geographical_area_detail_views( + view, + url_pattern, + valid_user_client, + session_with_workbasket, +): """Verify that geographical detail views are under the url geographical- areas and don't return an error.""" model_overrides = { diff --git a/importer/jinja2/importer/create.jinja b/importer/jinja2/importer/create.jinja index 747d2bab6..b516bf4d6 100644 --- a/importer/jinja2/importer/create.jinja +++ b/importer/jinja2/importer/create.jinja @@ -9,7 +9,7 @@ {% block beforeContent %} {{ govukBreadcrumbs({ "items": [ - {"text": "Home", "href": url("index")}, + {"text": "Home", "href": url("home")}, {"text": "Find and edit import batches", "href": url("import_batch-ui-list")}, {"text": page_title} ] diff --git a/measures/jinja2/measures/confirm-create-multiple.jinja b/measures/jinja2/measures/confirm-create-multiple.jinja index b0d67afac..cd49e29b3 100644 --- a/measures/jinja2/measures/confirm-create-multiple.jinja +++ b/measures/jinja2/measures/confirm-create-multiple.jinja @@ -21,7 +21,7 @@ {% block beforeContent %} {{ govukBreadcrumbs({ "items": [ - {"text": "Home", "href": url("index")}, + {"text": "Home", "href": url("home")}, {"text": page_title} ] }) }} @@ -39,15 +39,19 @@ }) }}

Next steps

To complete your task you must publish your change.

+ {% if request.session.workbasket %} {{ govukButton({ "text": "Go to your tariff changes", - "href": url("index"), + "href": url("workbaskets:edit-workbasket", args=[request.session.workbasket.id]), "classes": "govuk-button--secondary" }) }} + {% endif %} diff --git a/measures/jinja2/measures/create-start.jinja b/measures/jinja2/measures/create-start.jinja index 2f72521e7..8ee06396e 100644 --- a/measures/jinja2/measures/create-start.jinja +++ b/measures/jinja2/measures/create-start.jinja @@ -14,7 +14,7 @@

You'll also need to use the - + manage trade tariffs service to find or create details of the following: diff --git a/measures/jinja2/measures/edit.jinja b/measures/jinja2/measures/edit.jinja index 6e6d5731c..f798e46f6 100644 --- a/measures/jinja2/measures/edit.jinja +++ b/measures/jinja2/measures/edit.jinja @@ -14,7 +14,7 @@ {% block beforeContent %} {{ govukBreadcrumbs({ "items": [ - {"text": "Home", "href": url("index")}, + {"text": "Home", "href": url("home")}, {"text": "Find and edit " ~ object._meta.verbose_name_plural, "href": object.get_url("list")}, {"text": object._meta.verbose_name|capitalize ~ ": " ~ object.sid, "href": object.get_url()}, {"text": page_title} diff --git a/measures/tests/test_views.py b/measures/tests/test_views.py index 6cced7e8f..26567c82c 100644 --- a/measures/tests/test_views.py +++ b/measures/tests/test_views.py @@ -118,7 +118,12 @@ def test_measure_delete(use_delete_form): ), ids=view_urlpattern_ids, ) -def test_measure_detail_views(view, url_pattern, valid_user_client): +def test_measure_detail_views( + view, + url_pattern, + valid_user_client, + session_with_workbasket, +): """Verify that measure detail views are under the url measures/ and don't return an error.""" assert_model_view_renders(view, url_pattern, valid_user_client) diff --git a/pii-ner-exclude.txt b/pii-ner-exclude.txt index ee637fdfb..140af2b2a 100644 --- a/pii-ner-exclude.txt +++ b/pii-ner-exclude.txt @@ -23,8 +23,14 @@ NM target.quota.definition.sid MeasureType ImportBatch +f"/edit/ BatchDependencies +Jira +MeasureActions +do?{% +MeasureAction Quota +url('my SID the Euro ISO the Euro Definition diff --git a/regulations/jinja2/regulations/edit.jinja b/regulations/jinja2/regulations/edit.jinja index 33ae68702..051ba2bc0 100644 --- a/regulations/jinja2/regulations/edit.jinja +++ b/regulations/jinja2/regulations/edit.jinja @@ -7,7 +7,7 @@ {{ govukBreadcrumbs({ "items": [ - {"text": "Home", "href": url("index")}, + {"text": "Home", "href": url("home")}, {"text": "Find and edit " ~ object._meta.verbose_name_plural, "href": object.get_url("list")}, {"text": object._meta.verbose_name|capitalize ~ ": " ~ object|string, "href": object.get_url()}, {"text": page_title} diff --git a/regulations/tests/test_views.py b/regulations/tests/test_views.py index 819616f41..fec3c3efa 100644 --- a/regulations/tests/test_views.py +++ b/regulations/tests/test_views.py @@ -46,7 +46,12 @@ def test_regulation_delete(factory, use_delete_form): ), ids=view_urlpattern_ids, ) -def test_regulation_detail_views(view, url_pattern, valid_user_client): +def test_regulation_detail_views( + view, + url_pattern, + valid_user_client, + session_with_workbasket, +): """Verify that regulation detail views are under the url regulations/ and don't return an error.""" assert_model_view_renders(view, url_pattern, valid_user_client) @@ -61,7 +66,12 @@ def test_regulation_detail_views(view, url_pattern, valid_user_client): ), ids=view_urlpattern_ids, ) -def test_regulation_list_view(view, url_pattern, valid_user_client): +def test_regulation_list_view( + view, + url_pattern, + valid_user_client, + session_with_workbasket, +): """Verify that regulation list view is under the url regulations/ and doesn't return an error.""" assert_model_view_renders(view, url_pattern, valid_user_client) diff --git a/settings/common.py b/settings/common.py index 0d966cbf9..d530cc188 100644 --- a/settings/common.py +++ b/settings/common.py @@ -167,7 +167,7 @@ if SSO_ENABLED: LOGIN_URL = reverse_lazy("authbroker_client:login") -LOGIN_REDIRECT_URL = reverse_lazy("index") +LOGIN_REDIRECT_URL = reverse_lazy("home") AUTHBROKER_URL = os.environ.get("AUTHBROKER_URL", "https://sso.trade.gov.uk") AUTHBROKER_CLIENT_ID = os.environ.get("AUTHBROKER_CLIENT_ID") diff --git a/workbaskets/forms.py b/workbaskets/forms.py index 675dc5528..ac32a9cdf 100644 --- a/workbaskets/forms.py +++ b/workbaskets/forms.py @@ -1,5 +1,48 @@ +from crispy_forms_gds.helper import FormHelper +from crispy_forms_gds.layout import Field +from crispy_forms_gds.layout import Layout +from crispy_forms_gds.layout import Size +from crispy_forms_gds.layout import Submit from django import forms +from common.forms import DescriptionHelpBox +from workbaskets import models + + +class WorkbasketCreateForm(forms.ModelForm): + """The form for creating a new workbasket.""" + + title = forms.CharField( + label="Tops/Jira number", + widget=forms.TextInput, + required=True, + ) + + reason = forms.CharField( + label="Description", + help_text="Add your notes here. You may enter HTML formatting if required. See the guide below for more information.", + widget=forms.Textarea, + required=True, + ) + + def __init__(self, *args, **kwargs): + self.request = kwargs.pop("request", None) + super().__init__(*args, **kwargs) + + self.helper = FormHelper(self) + self.helper.label_size = Size.SMALL + self.helper.legend_size = Size.SMALL + self.helper.layout = Layout( + "title", + Field.textarea("reason", rows=5), + DescriptionHelpBox(), + Submit("submit", "Create"), + ) + + class Meta: + model = models.WorkBasket + fields = ("title", "reason") + class SelectableObjectField(forms.BooleanField): """Associates an object instance with a BooleanField.""" diff --git a/workbaskets/jinja2/includes/workbaskets/edit-workbasket-list.jinja b/workbaskets/jinja2/includes/workbaskets/edit-workbasket-list.jinja new file mode 100644 index 000000000..de81b84c3 --- /dev/null +++ b/workbaskets/jinja2/includes/workbaskets/edit-workbasket-list.jinja @@ -0,0 +1,37 @@ +{% from "components/button/macro.njk" import govukButton %} +{% from "components/input/macro.njk" import govukInput %} +{% from "components/table/macro.njk" import govukTable %} + +{%- set table_rows = [] -%} +{% for workbasket in object_list %} + {%- set workbasket_linked_pk -%} +

+ + +
+ {%- endset -%} + {%- set workbasket_status -%} + {{ workbasket.status }} + {%- endset -%} + + {{ table_rows.append([ + {"text": workbasket_linked_pk}, + {"html": workbasket.title}, + {"text": workbasket.reason}, + {"text": workbasket_status}, + ]) or "" }} +{% endfor %} + +{{ govukTable({ + "head": [ + {"text": "Number"}, + {"text": "TOPS/Jira"}, + {"text": "Description"}, + {"text": "Status"}, + ], + "rows": table_rows +}) }} diff --git a/workbaskets/jinja2/includes/workbaskets/workbasket-selectable-items.jinja b/workbaskets/jinja2/includes/workbaskets/workbasket-selectable-items.jinja index d9ef12a9d..06e26827e 100644 --- a/workbaskets/jinja2/includes/workbaskets/workbasket-selectable-items.jinja +++ b/workbaskets/jinja2/includes/workbaskets/workbasket-selectable-items.jinja @@ -35,12 +35,18 @@ {% if workbasket %} +{% set change_count = workbasket.tracked_models.count() %}
-

Your tariff changes

+ {% if change_count %} +

Select a component you would like to edit or remove:

+ {% else %} +

There is nothing in your workbasket yet.

+ Edit workbasket + {% endif %}
@@ -62,7 +68,7 @@
This is a place holder for help text about tariff managers' actions and/or next steps
- + #}
@@ -72,43 +78,10 @@
{% endif %} - +
- {% set change_count = workbasket.tracked_models.count() %} -

{{ change_count }} changes

- {% if form.fields %} - {% set actions_details_html %} -
-
- -
-
- -
-
- {%- endset%} - {{ govukDetails({ - "summaryText": "Actions on changes below", - "html": actions_details_html - }) }} {% set table_rows = [] %} {% for field in form %} @@ -125,9 +98,9 @@ {{ table_rows.append([ {"html": checkbox}, - {"text": obj.update_type_str}, - {"text": obj._meta.verbose_name.title(), "classes": "govuk-!-width-one-quarter"}, {"html": object_link, "classes": "govuk-!-width-one-quarter"}, + {"text": obj._meta.verbose_name.title(), "classes": "govuk-!-width-one-quarter"}, + {"text": obj.update_type_str}, {"text": object_description, "classes": "govuk-!-width-one-quarter"}, {"text": "{:%d %b %Y}".format(obj.transaction.updated_at), "classes": "govuk-!-width-one-quarter"}, ]) or "" }} @@ -136,9 +109,9 @@ {{ govukTable({ "head": [ {"html": checkbox_check_all}, - {"text": "Action"}, - {"text": "Type"}, {"text": "ID"}, + {"text": "Component"}, + {"text": "Action"}, {"text": "Description"}, {"text": "Activity date"}, ], @@ -149,5 +122,7 @@ {% endif %}
+ + {% endif %} diff --git a/workbaskets/jinja2/workbaskets/confirm_create.jinja b/workbaskets/jinja2/workbaskets/confirm_create.jinja new file mode 100644 index 000000000..1a0b98d7f --- /dev/null +++ b/workbaskets/jinja2/workbaskets/confirm_create.jinja @@ -0,0 +1,28 @@ +{% extends "layouts/layout.jinja" %} + +{% from "components/panel/macro.njk" import govukPanel %} +{% from "components/button/macro.njk" import govukButton %} + +{% set page_title = "Create a new " ~ object._meta.verbose_name %} + +{% block beforeContent %} + {{ govukBreadcrumbs({ + "items": [ + {"text": "Home", "href": url("home")}, + {"text": page_title} + ] + }) }} +{% endblock %} + +{% block content %} +
+
+ {{ govukPanel({ + "titleText": object._meta.verbose_name|title ~ " " ~ object.id, + "text": "You have created a new workbasket", + "classes": "govuk-!-margin-bottom-7" + }) }} + Continue to workbasket +
+
+{% endblock %} diff --git a/workbaskets/jinja2/workbaskets/create.jinja b/workbaskets/jinja2/workbaskets/create.jinja new file mode 100644 index 000000000..e53efa2bd --- /dev/null +++ b/workbaskets/jinja2/workbaskets/create.jinja @@ -0,0 +1,13 @@ +{% extends "layouts/create.jinja" %} +{% from "components/breadcrumbs/macro.njk" import govukBreadcrumbs %} + + +{% set page_title = "Create a new workbasket" %} +{% block breadcrumb %} + {{ govukBreadcrumbs({ + "items": [ + {"text": "Home", "href": url("home")}, + {"text": "Create a new workbasket"} + ] + }) }} +{% endblock %} diff --git a/workbaskets/jinja2/workbaskets/delete_changes.jinja b/workbaskets/jinja2/workbaskets/delete_changes.jinja index c55e7dbf0..bef3951f8 100644 --- a/workbaskets/jinja2/workbaskets/delete_changes.jinja +++ b/workbaskets/jinja2/workbaskets/delete_changes.jinja @@ -11,7 +11,7 @@ {% block breadcrumb %} {{ govukBreadcrumbs({ "items": [ - {"text": "Home", "href": url("index")}, + {"text": "Home", "href": url("home")}, {"text": page_title} ] }) }} diff --git a/workbaskets/jinja2/workbaskets/delete_changes_confirm.jinja b/workbaskets/jinja2/workbaskets/delete_changes_confirm.jinja index 99a1a403e..e67f91126 100644 --- a/workbaskets/jinja2/workbaskets/delete_changes_confirm.jinja +++ b/workbaskets/jinja2/workbaskets/delete_changes_confirm.jinja @@ -9,7 +9,7 @@ {% block breadcrumb %} {{ govukBreadcrumbs({ "items": [ - {"text": "Home", "href": url("index")}, + {"text": "Home", "href": url("home")}, {"text": page_title} ] }) }} diff --git a/workbaskets/jinja2/workbaskets/detail.jinja b/workbaskets/jinja2/workbaskets/detail.jinja index bce05d82f..521a356f2 100644 --- a/workbaskets/jinja2/workbaskets/detail.jinja +++ b/workbaskets/jinja2/workbaskets/detail.jinja @@ -12,7 +12,7 @@ {% block breadcrumb %} {{ govukBreadcrumbs({ "items": [ - {"text": "Home", "href": url("index")}, + {"text": "Home", "href": url("home")}, {"text": "Find and edit work baskets", "href": url("workbaskets:workbasket-ui-list")}, {"text": page_title} ] diff --git a/workbaskets/jinja2/workbaskets/edit-workbasket.jinja b/workbaskets/jinja2/workbaskets/edit-workbasket.jinja new file mode 100644 index 000000000..9a190bd75 --- /dev/null +++ b/workbaskets/jinja2/workbaskets/edit-workbasket.jinja @@ -0,0 +1,150 @@ +{% extends "layouts/layout.jinja" %} +{% from "components/breadcrumbs/macro.njk" import govukBreadcrumbs %} + +{% set page_title %}Workbasket {{ request.session.workbasket.id }}{% if request.session.workbasket.title %} - {{ request.session.workbasket.title }}{% endif %}{% endset %} +{% set page_subtitle %}What would you like to do?{% endset %} + +{% block breadcrumb %} + {% if request.GET.edit %} + {{ govukBreadcrumbs({ + "items": [ + {"text": "Home", "href": url("home")}, + {"text": "Edit an existing workbasket", "href": url("workbaskets:workbasket-ui-list")}, + {"text": "Workbasket " ~ request.session.workbasket.id ~ " - Menu" } + ] + }) }} + {% else %} + {{ govukBreadcrumbs({ + "items": [ + {"text": "Home", "href": url("home")}, + {"text": "Workbasket " ~ request.session.workbasket.id ~ " - Menu" } + ] + }) }} + {% endif %} +{% endblock %} + + +{% block content %} +

+ {{ page_title }} + + {{ page_subtitle }} + +

+
+
+

+ Main menu +

+
+
+
+
+

Additional codes

+ +
+
+

Certificates

+ +
+
+

Footnotes

+ +
+
+

Geographical areas

+ +
+
+
+
+

Commodities

+ +
+
+

Measures

+ +
+
+

Quotas

+ +
+
+

Regulations

+ +
+
+{% endblock %} diff --git a/workbaskets/jinja2/workbaskets/preview-workbasket.jinja b/workbaskets/jinja2/workbaskets/preview-workbasket.jinja new file mode 100644 index 000000000..430f38e42 --- /dev/null +++ b/workbaskets/jinja2/workbaskets/preview-workbasket.jinja @@ -0,0 +1,47 @@ +{% extends "layouts/layout.jinja" %} + +{% from "components/breadcrumbs/macro.njk" import govukBreadcrumbs %} + +{% set page_title %}Workbasket {{ request.session.workbasket.id }}{% endset %} +{% set page_subtitle %}At a glance{% endset %} + +{% block breadcrumb %} + {% if request.GET.edit %} + {{ govukBreadcrumbs({ + "items": [ + {"text": "Home", "href": url("home")}, + {"text": "Edit an existing workbasket", "href": url("workbaskets:workbasket-ui-list")}, + {"text": "Workbasket " ~ request.session.workbasket.id ~ " - At a glance" } + ] + }) }} + {% else %} + {{ govukBreadcrumbs({ + "items": [ + {"text": "Home", "href": url("home")}, + {"text": "Workbasket " ~ request.session.workbasket.id ~ " - At a glance" } + ] + }) }} + {% endif %} +{% endblock %} + + +{% block content %} +

+ {{ page_title }} +

+

+ {{ page_subtitle }} +

+
+
+
    +
  • Status: {{ workbasket.status.title() }}
    • + {% set change_count = workbasket.tracked_models.count() %} +
  • {{ change_count }} change{{ pluralize(change_count) }} found
    • +
+
+
+ {% set params %}{% if request.GET %}?{{ request.GET.urlencode() }}{% endif %}{% endset %} + Review workbasket + Edit workbasket +{% endblock %} diff --git a/workbaskets/jinja2/workbaskets/review-workbasket.jinja b/workbaskets/jinja2/workbaskets/review-workbasket.jinja new file mode 100644 index 000000000..6d9db5939 --- /dev/null +++ b/workbaskets/jinja2/workbaskets/review-workbasket.jinja @@ -0,0 +1,35 @@ +{% extends "layouts/layout.jinja" %} + +{% from "components/breadcrumbs/macro.njk" import govukBreadcrumbs %} + +{% set page_title %}Workbasket {{ request.session.workbasket.id }}{% endset %} +{% set page_subtitle %}Summary - View workbasket components{% endset %} + +{% block breadcrumb %} +{% if request.GET.edit %} + {{ govukBreadcrumbs({ + "items": [ + {"text": "Home", "href": url("home")}, + {"text": "Edit an existing workbasket", "href": url("workbaskets:workbasket-ui-list")}, + {"text": "Workbasket " ~ request.session.workbasket.id ~ " - At a glance" } + ] + }) }} +{% else %} + {{ govukBreadcrumbs({ + "items": [ + {"text": "Home", "href": url("home")}, + {"text": "Workbasket " ~ request.session.workbasket.id ~ " - At a glance" } + ] + }) }} +{% endif %} +{% endblock %} + +{% block content %} +

+ {{ page_title }} +

+

+ {{ page_subtitle }} +

+ {% include "includes/workbaskets/workbasket-selectable-items.jinja" %} +{% endblock %} diff --git a/workbaskets/jinja2/workbaskets/select-workbasket.jinja b/workbaskets/jinja2/workbaskets/select-workbasket.jinja new file mode 100644 index 000000000..0e4daa96f --- /dev/null +++ b/workbaskets/jinja2/workbaskets/select-workbasket.jinja @@ -0,0 +1,45 @@ +{% extends "layouts/layout.jinja" %} + +{% from "components/table/macro.njk" import govukTable %} + +{% set page_title = "Edit an existing workbasket" %} +{% set list_include = "includes/workbaskets/edit-workbasket-list.jinja" %} + +{% block breadcrumb %} + {{ govukBreadcrumbs({ + "items": [ + {"text": "Home", "href": url("home")}, + {"text": page_title} + ] + }) }} +{% endblock %} + +{% block content %} +

{{ page_title }}

+ + +
+ + {% if object_list %} + {% include list_include %} + {% else %} +

There are no workbaskets with this status.

+ {% endif %} + {% include "includes/common/pagination.jinja" %} +{% endblock %} diff --git a/workbaskets/models.py b/workbaskets/models.py index aeb118f3d..53f1e6d0e 100644 --- a/workbaskets/models.py +++ b/workbaskets/models.py @@ -408,6 +408,7 @@ def save_to_session(self, session): session["workbasket"] = { "id": self.pk, "status": self.status, + "title": self.title, } @property diff --git a/workbaskets/tests/test_forms.py b/workbaskets/tests/test_forms.py new file mode 100644 index 000000000..25ccda433 --- /dev/null +++ b/workbaskets/tests/test_forms.py @@ -0,0 +1,50 @@ +import pytest +from django import forms + +pytestmark = pytest.mark.django_db + + +class WorkBasketForm(forms.Form): + title = forms.CharField() + reason = forms.CharField() + + def __init__(self, *args, **kwargs): + super().__init__(*args, **kwargs) + + +def test_workbasket_form_validation(): + form = WorkBasketForm( + { + "title": "some title", + "reason": "", + }, + ) + assert not form.is_valid() + assert "reason" in form.errors + + form = WorkBasketForm( + { + "title": "", + "reason": "some reason", + }, + ) + assert not form.is_valid() + assert "title" in form.errors + + form = WorkBasketForm( + { + "title": "", + "reason": "", + }, + ) + assert not form.is_valid() + assert "title" in form.errors + assert "reason" in form.errors + + form = WorkBasketForm( + { + "title": "some title", + "reason": "some reason", + }, + ) + assert form.is_valid() diff --git a/workbaskets/tests/test_views.py b/workbaskets/tests/test_views.py index 14e8b747a..dfb844a94 100644 --- a/workbaskets/tests/test_views.py +++ b/workbaskets/tests/test_views.py @@ -2,13 +2,17 @@ from unittest.mock import patch import pytest +from bs4 import BeautifulSoup from django.test import override_settings from django.urls import reverse from common.tests import factories +from common.tests.factories import GoodsNomenclatureFactory from common.tests.util import validity_period_post_data from common.validators import UpdateType from exporter.tasks import upload_workbaskets +from workbaskets import models +from workbaskets.forms import SelectableObjectsForm from workbaskets.models import WorkBasket from workbaskets.tests.util import assert_workbasket_valid from workbaskets.validators import WorkflowStatus @@ -16,6 +20,29 @@ pytestmark = pytest.mark.django_db +def test_workbasket_create_form_creates_workbasket_object( + valid_user_api_client, +): + + # Post a form + create_url = reverse("workbaskets:workbasket-ui-create") + + form_data = { + "title": "My new workbasket", + "reason": "Making a new workbasket", + } + + response = valid_user_api_client.post(create_url, form_data) + # get the workbasket we have made, and make sure it matches title and description + workbasket = models.WorkBasket.objects.filter( + title=form_data["title"], + )[0] + + assert str(workbasket.id) in response.url + assert workbasket.title == form_data["title"] + assert workbasket.reason == form_data["reason"] + + @override_settings(CELERY_TASK_ALWAYS_EAGER=True, CELERY_TASK_EAGER_PROPOGATES=True) @patch("exporter.tasks.upload_workbaskets") def test_submit_workbasket( @@ -37,7 +64,7 @@ def test_submit_workbasket( response = client.get(url) assert response.status_code == 302 - assert response.url == reverse("index") + assert response.url == reverse("home") workbasket.refresh_from_db() @@ -157,3 +184,122 @@ def test_download( assert response.status_code == 302 assert expected_url in response.url + + +def test_review_workbasket_displays_objects_in_current_workbasket( + valid_user_client, + session_workbasket, +): + """Verify that changes in the current workbasket are displayed on the bulk + selection form of the review workbasket page.""" + + with session_workbasket.new_transaction(): + GoodsNomenclatureFactory.create() + + response = valid_user_client.get( + reverse("workbaskets:review-workbasket", kwargs={"pk": session_workbasket.id}), + ) + page = BeautifulSoup( + response.content.decode(response.charset), + features="lxml", + ) + for obj in session_workbasket.tracked_models.all(): + field_name = SelectableObjectsForm.field_name_for_object(obj) + assert page.find("input", {"name": field_name}) + + +def test_edit_workbasket_page_sets_workbasket(valid_user_client, session_workbasket): + response = valid_user_client.get( + reverse("workbaskets:edit-workbasket", kwargs={"pk": session_workbasket.pk}), + ) + assert response.status_code == 200 + soup = BeautifulSoup(str(response.content), "html.parser") + assert session_workbasket.title in soup.select(".govuk-heading-xl")[0].text + assert str(session_workbasket.pk) in soup.select(".govuk-heading-xl")[0].text + + +@pytest.mark.parametrize( + "url_name", + [ + ("workbaskets:edit-workbasket"), + ("workbaskets:review-workbasket"), + ("workbaskets:workbasket-ui-detail"), + ], +) +def test_edit_workbasket_page_displays_breadcrumb( + url_name, + valid_user_client, + session_workbasket, +): + url = reverse(url_name, kwargs={"pk": session_workbasket.pk}) + response = valid_user_client.get( + f"{url}?edit=1", + ) + assert response.status_code == 200 + soup = BeautifulSoup(str(response.content), "html.parser") + breadcrumb_links = [ + element.text for element in soup.select(".govuk-breadcrumbs__link") + ] + assert "Edit an existing workbasket" in breadcrumb_links + + +def test_workbasket_detail_page_url_params( + valid_user_client, + session_workbasket, +): + url = reverse( + "workbaskets:workbasket-ui-detail", + kwargs={"pk": session_workbasket.pk}, + ) + response = valid_user_client.get(url) + assert response.status_code == 200 + soup = BeautifulSoup(str(response.content), "html.parser") + buttons = soup.select(".govuk-button.govuk-button--primary") + for button in buttons: + # test that accidental spacing in template hasn't mangled the url + assert " " not in button.get("href") + assert "%20" not in button.get("href") + + +def test_edit_workbasket_page_hides_breadcrumb(valid_user_client, session_workbasket): + url = reverse("workbaskets:edit-workbasket", kwargs={"pk": session_workbasket.pk}) + response = valid_user_client.get( + f"{url}?edit=", + ) + assert response.status_code == 200 + soup = BeautifulSoup(str(response.content), "html.parser") + breadcrumb_links = [ + element.text for element in soup.select(".govuk-breadcrumbs__link") + ] + assert "Edit an existing workbasket" not in breadcrumb_links + + +def test_select_workbasket_page_200(valid_user_client): + """ + Checks the page returns 200. + + Then checks that only workbaskets with certain statuses are displayed i.e. + we don't want users to be able to edit workbaskets that are archived, sent, + or published. + """ + factories.WorkBasketFactory.create(status=WorkflowStatus.ARCHIVED) + factories.WorkBasketFactory.create(status=WorkflowStatus.SENT) + factories.WorkBasketFactory.create(status=WorkflowStatus.PUBLISHED) + factories.WorkBasketFactory.create(status=WorkflowStatus.EDITING) + factories.WorkBasketFactory.create(status=WorkflowStatus.APPROVED) + factories.WorkBasketFactory.create(status=WorkflowStatus.PROPOSED) + factories.WorkBasketFactory.create(status=WorkflowStatus.ERRORED) + valid_statuses = { + WorkflowStatus.EDITING, + WorkflowStatus.APPROVED, + WorkflowStatus.PROPOSED, + WorkflowStatus.ERRORED, + } + response = valid_user_client.get(reverse("workbaskets:workbasket-ui-list")) + assert response.status_code == 200 + soup = BeautifulSoup(str(response.content), "html.parser") + statuses = [ + element.text for element in soup.select(".govuk-table__row .status-badge") + ] + assert len(statuses) == 4 + assert not set(statuses).difference(valid_statuses) diff --git a/workbaskets/urls.py b/workbaskets/urls.py index 003660300..d8fc5adc6 100644 --- a/workbaskets/urls.py +++ b/workbaskets/urls.py @@ -13,14 +13,39 @@ ui_patterns = [ path( "", - ui_views.WorkBasketList.as_view(), + ui_views.SelectWorkbasketView.as_view(), name="workbasket-ui-list", ), + path( + "create/", + ui_views.WorkBasketCreate.as_view(), + name="workbasket-ui-create", + ), + path( + f"/edit/", + ui_views.EditWorkbasketView.as_view(), + name="edit-workbasket", + ), + path( + f"/review/", + ui_views.ReviewWorkbasketView.as_view(), + name="review-workbasket", + ), + path( + "download", + ui_views.download_envelope, + name="workbasket-download", + ), path( f"/", ui_views.WorkBasketDetail.as_view(), name="workbasket-ui-detail", ), + path( + f"/confirm-create/", + ui_views.WorkBasketConfirmCreate.as_view(), + name="workbasket-ui-confirm-create", + ), path( f"/submit/", ui_views.WorkBasketSubmit.as_view(), @@ -36,11 +61,6 @@ ui_views.WorkBasketDeleteChangesDone.as_view(), name="workbasket-ui-delete-changes-done", ), - path( - "download", - ui_views.download_envelope, - name="workbasket-download", - ), ] urlpatterns = [ diff --git a/workbaskets/views/ui.py b/workbaskets/views/ui.py index b13183fb1..733929528 100644 --- a/workbaskets/views/ui.py +++ b/workbaskets/views/ui.py @@ -1,6 +1,7 @@ import boto3 from botocore.client import Config from django.conf import settings +from django.contrib.auth import get_user_model from django.contrib.auth.mixins import PermissionRequiredMixin from django.core.paginator import Paginator from django.db.models import ProtectedError @@ -9,19 +10,26 @@ from django.shortcuts import redirect from django.urls import reverse from django.urls import reverse_lazy +from django.utils.decorators import method_decorator +from django.views.generic import CreateView from django.views.generic.base import RedirectView +from django.views.generic.base import TemplateResponseMixin from django.views.generic.base import TemplateView +from django.views.generic.base import View from django.views.generic.detail import DetailView from django.views.generic.detail import SingleObjectMixin +from django.views.generic.edit import FormMixin from django.views.generic.list import ListView from common.filters import TamatoFilter -from common.pagination import build_pagination_list from common.views import WithPaginationListView from exporter.models import Upload +from workbaskets import forms from workbaskets import tasks from workbaskets.models import WorkBasket from workbaskets.session_store import SessionStore +from workbaskets.validators import WorkflowStatus +from workbaskets.views.decorators import require_current_workbasket class WorkBasketFilter(TamatoFilter): @@ -38,56 +46,69 @@ class Meta: fields = ["search", "status"] -class WorkBasketList(WithPaginationListView): - """UI endpoint for viewing and filtering workbaskets.""" +class WorkBasketConfirmCreate(DetailView): + template_name = "workbaskets/confirm_create.jinja" + model = WorkBasket + queryset = WorkBasket.objects.all() + + +class WorkBasketCreate(CreateView): + """UI endpoint for creating workbaskets.""" + + permission_required = "workbaskets.add_workbasket" + template_name = "workbaskets/create.jinja" + form_class = forms.WorkbasketCreateForm + + def form_valid(self, form): + user = get_user_model().objects.get(username=self.request.user.username) + self.object = form.save(commit=False) + self.object.author = user + self.object.save() + self.object.save_to_session(self.request.session) + return redirect( + reverse( + "workbaskets:workbasket-ui-confirm-create", + kwargs={"pk": self.object.pk}, + ), + ) - template_name = "workbaskets/list.jinja" - filterset_class = WorkBasketFilter - search_fields = [ - "title", - "reason", - ] + def get_form_kwargs(self): + kwargs = super().get_form_kwargs() + kwargs["request"] = self.request + return kwargs - def get_queryset(self): - return WorkBasket.objects.order_by("-updated_at") +class SelectWorkbasketView(WithPaginationListView): + """UI endpoint for viewing and filtering workbaskets.""" -class WorkBasketDetail(DetailView): - """UI endpoint for viewing a specified workbasket.""" - - model = WorkBasket - template_name = "workbaskets/detail.jinja" - paginate_by = 50 - # paginate_by = 2 + filterset_class = WorkBasketFilter + template_name = "workbaskets/select-workbasket.jinja" + permission_required = "workbaskets.change_workbasket" - def get_context_data(self, **kwargs): - """ - Although this is a detail view of a WorkBasket instance, it provides a - view of its contained items (TrackedModel instances) as a paged list. + def get_queryset(self): + return ( + WorkBasket.objects.exclude(status=WorkflowStatus.PUBLISHED) + .exclude(status=WorkflowStatus.ARCHIVED) + .exclude(status=WorkflowStatus.SENT) + .order_by("-updated_at") + ) - A paginator and related objects are therefore added to page context. - """ - items = self.get_object().tracked_models.all() + def post(self, request, *args, **kwargs): + workbasket_pk = request.POST.get("workbasket") - paginator = Paginator(items, WorkBasketDetail.paginate_by) - try: - page_number = int(self.request.GET.get("page", 1)) - except ValueError: - page_number = 1 - page_obj = paginator.get_page(page_number) + if workbasket_pk: + workbasket = WorkBasket.objects.get(pk=workbasket_pk) - context = super().get_context_data(**kwargs) + if workbasket: + workbasket.save_to_session(request.session) + redirect_url = reverse( + "workbaskets:workbasket-ui-detail", + kwargs={"pk": workbasket_pk}, + ) - context["paginator"] = paginator - context["page_obj"] = page_obj - context["is_paginated"] = True - context["object_list"] = items - context["page_links"] = build_pagination_list( - page_number, - page_obj.paginator.num_pages, - ) + return redirect(f"{redirect_url}?edit=1") - return context + return redirect(reverse("workbaskets:workbasket-ui-list")) class WorkBasketSubmit(PermissionRequiredMixin, SingleObjectMixin, RedirectView): @@ -97,7 +118,7 @@ class WorkBasketSubmit(PermissionRequiredMixin, SingleObjectMixin, RedirectView) permission_required = "workbaskets.change_workbasket" def get_redirect_url(self, *args, **kwargs) -> str: - return reverse("index") + return reverse("home") def get(self, *args, **kwargs): workbasket: WorkBasket = self.get_object() @@ -154,7 +175,7 @@ def get_queryset(self): def post(self, request, *args, **kwargs): if request.POST.get("action", None) != "delete": # The user has cancelled out of the deletion process. - return redirect("index") + return redirect("home") # By reverse ordering on record_code + subrecord_code we're able to # delete child entities first, avoiding protected foreign key @@ -223,3 +244,142 @@ def download_envelope(request): ) return HttpResponseRedirect(url) + + +@method_decorator(require_current_workbasket, name="dispatch") +class EditWorkbasketView(TemplateView): + template_name = "workbaskets/edit-workbasket.jinja" + permission_required = "workbaskets.change_workbasket" + + +@method_decorator(require_current_workbasket, name="dispatch") +class WorkBasketDetail(DetailView): + model = WorkBasket + template_name = "workbaskets/preview-workbasket.jinja" + + def get_context_data(self, **kwargs): + context = super().get_context_data(**kwargs) + context["workbasket"] = WorkBasket.load_from_session(self.request.session) + return context + + +@method_decorator(require_current_workbasket, name="dispatch") +class ReviewWorkbasketView(TemplateResponseMixin, FormMixin, View): + template_name = "workbaskets/review-workbasket.jinja" + form_class = forms.SelectableObjectsForm + + # Form action mappings to URL names. + action_success_url_names = { + "publish-all": "workbaskets:workbasket-ui-submit", + "remove-selected": "workbaskets:workbasket-ui-delete-changes", + "page-prev": "workbaskets:review-workbasket", + "page-next": "workbaskets:review-workbasket", + } + + @property + def workbasket(self) -> WorkBasket: + return WorkBasket.current(self.request) + + @property + def paginator(self): + return Paginator(self.workbasket.tracked_models, per_page=10) + + @property + def latest_upload(self): + return Upload.objects.order_by("created_date").last() + + @property + def uploaded_envelope_dates(self): + """Gets a list of all transactions from the `latest_approved_workbasket` + in the order they were updated and returns a dict with the first and + last transactions as values for "start" and "end" keys respectively.""" + if self.latest_upload: + transactions = self.latest_upload.envelope.transactions.order_by( + "updated_at", + ) + return { + "start": transactions.first().updated_at, + "end": transactions.last().updated_at, + } + return None + + def _append_url_page_param(self, url, form_action): + """Based upon 'form_action', append a 'page' URL parameter to the given + url param and return the result.""" + page = self.paginator.get_page(self.request.GET.get("page", 1)) + page_number = 1 + if form_action == "page-prev": + page_number = page.previous_page_number() + elif form_action == "page-next": + page_number = page.next_page_number() + return f"{url}?page={page_number}" + + def get(self, request, *args, **kwargs): + """Service GET requests by displaying the page and form.""" + return self.render_to_response(self.get_context_data()) + + def post(self, request, *args, **kwargs): + """Manage POST requests, which can either be requests to change the + paged form data while preserving the user's form changes, or finally + submit the form data for processing.""" + form = self.get_form() + if form.is_valid(): + return self.form_valid(form) + else: + return self.form_invalid(form) + + def get_success_url(self): + form_action = self.request.POST.get("form-action") + if form_action in ("publish-all", "remove-selected"): + return reverse( + self.action_success_url_names[form_action], + kwargs={"pk": self.workbasket.pk}, + ) + elif form_action in ("page-prev", "page-next"): + return self._append_url_page_param( + reverse(self.action_success_url_names[form_action]), + form_action, + ) + return reverse("home") + + def get_initial(self): + store = SessionStore( + self.request, + f"WORKBASKET_SELECTIONS_{self.workbasket.pk}", + ) + return store.data.copy() + + def get_form_kwargs(self): + kwargs = super().get_form_kwargs() + page = self.paginator.get_page(self.request.GET.get("page", 1)) + kwargs["objects"] = page.object_list + return kwargs + + def get_context_data(self, **kwargs): + context = super().get_context_data(**kwargs) + page = self.paginator.get_page(self.request.GET.get("page", 1)) + context.update( + { + "workbasket": self.workbasket, + "page_obj": page, + "uploaded_envelope_dates": self.uploaded_envelope_dates, + }, + ) + return context + + def form_valid(self, form): + store = SessionStore( + self.request, + f"WORKBASKET_SELECTIONS_{self.workbasket.pk}", + ) + to_add = { + key: value for key, value in form.cleaned_data_no_prefix.items() if value + } + to_remove = { + key: value + for key, value in form.cleaned_data_no_prefix.items() + if key not in to_add + } + store.add_items(to_add) + store.remove_items(to_remove) + return super().form_valid(form) From bbbcd5bed25225853386f519ec87156c49320bd9 Mon Sep 17 00:00:00 2001 From: Gabe Naughton Date: Tue, 26 Jul 2022 10:02:38 +0100 Subject: [PATCH 14/28] update business rule to use association transaction and add test (#629) --- quotas/business_rules.py | 7 +++++-- quotas/tests/test_business_rules.py | 15 +++++++++++++++ 2 files changed, 20 insertions(+), 2 deletions(-) diff --git a/quotas/business_rules.py b/quotas/business_rules.py index cde71b9cc..e3c292d6e 100644 --- a/quotas/business_rules.py +++ b/quotas/business_rules.py @@ -355,11 +355,14 @@ class QA6(BusinessRule): def validate(self, association): if ( - association.main_quota.sub_quota_associations.values( + association.main_quota.sub_quota_associations.approved_up_to_transaction( + association.transaction, + ) + .values( "sub_quota_relation_type", ) .order_by("sub_quota_relation_type") - .distinct("sub_quota_relation_type") + .distinct() .count() > 1 ): diff --git a/quotas/tests/test_business_rules.py b/quotas/tests/test_business_rules.py index 6d48b7bd0..4fb97a42e 100644 --- a/quotas/tests/test_business_rules.py +++ b/quotas/tests/test_business_rules.py @@ -600,6 +600,21 @@ def test_QA6(existing_relation, new_relation, error_expected): business_rules.QA6(assoc.transaction).validate(assoc) +# https://uktrade.atlassian.net/browse/TP2000-434 +def test_QA6_new_association_version(): + """Tests that previous versions of an association are not compared when + looking for sub-quotas associated with the same main quota.""" + original_version = factories.QuotaAssociationFactory.create( + sub_quota_relation_type="EQ", + ) + later_version = original_version.new_version( + original_version.transaction.workbasket, + sub_quota_relation_type="NM", + ) + + business_rules.QA6(later_version.transaction).validate(later_version) + + @pytest.mark.parametrize( ("mechanism", "error_expected"), ( From 733b8fe488a7e0f5f05c233d607d2735cffa6a27 Mon Sep 17 00:00:00 2001 From: Edie Pearce Date: Tue, 26 Jul 2022 11:29:07 +0100 Subject: [PATCH 15/28] TP2000-433: Fix missing pk in review workbasket url (#630) * Fix missing pk in review workbasket url * Rewrite test --- pii-ner-exclude.txt | 1 + workbaskets/tests/test_views.py | 32 ++++++++++++++++++++++++++++++++ workbaskets/views/ui.py | 5 ++++- 3 files changed, 37 insertions(+), 1 deletion(-) diff --git a/pii-ner-exclude.txt b/pii-ner-exclude.txt index 140af2b2a..bc57d570b 100644 --- a/pii-ner-exclude.txt +++ b/pii-ner-exclude.txt @@ -23,6 +23,7 @@ NM target.quota.definition.sid MeasureType ImportBatch +f"{url}?page=2 f"/edit/ BatchDependencies Jira diff --git a/workbaskets/tests/test_views.py b/workbaskets/tests/test_views.py index dfb844a94..e557ec330 100644 --- a/workbaskets/tests/test_views.py +++ b/workbaskets/tests/test_views.py @@ -303,3 +303,35 @@ def test_select_workbasket_page_200(valid_user_client): ] assert len(statuses) == 4 assert not set(statuses).difference(valid_statuses) + + +@pytest.mark.parametrize( + "form_action, url_name", + [ + ("publish-all", "workbaskets:workbasket-ui-submit"), + ("remove-selected", "workbaskets:workbasket-ui-delete-changes"), + ("page-prev", "workbaskets:review-workbasket"), + ("page-next", "workbaskets:review-workbasket"), + ], +) +def test_review_workbasket_redirects( + form_action, + url_name, + valid_user_client, +): + workbasket = factories.WorkBasketFactory.create( + status=WorkflowStatus.EDITING, + ) + with workbasket.new_transaction() as tx: + factories.FootnoteTypeFactory.create_batch(30, transaction=tx) + url = reverse("workbaskets:review-workbasket", kwargs={"pk": workbasket.pk}) + data = {"form-action": form_action} + response = valid_user_client.post(f"{url}?page=2", data) + assert response.status_code == 302 + assert reverse(url_name, kwargs={"pk": workbasket.pk}) in response.url + + if form_action == "page-prev": + assert "?page=1" in response.url + + elif form_action == "page-next": + assert "?page=3" in response.url diff --git a/workbaskets/views/ui.py b/workbaskets/views/ui.py index 733929528..b24d17364 100644 --- a/workbaskets/views/ui.py +++ b/workbaskets/views/ui.py @@ -337,7 +337,10 @@ def get_success_url(self): ) elif form_action in ("page-prev", "page-next"): return self._append_url_page_param( - reverse(self.action_success_url_names[form_action]), + reverse( + self.action_success_url_names[form_action], + kwargs={"pk": self.workbasket.pk}, + ), form_action, ) return reverse("home") From 439b4a0212ba48e1e114ae5fd39b9c220dd3902a Mon Sep 17 00:00:00 2001 From: Gabe Naughton Date: Wed, 27 Jul 2022 09:57:42 +0100 Subject: [PATCH 16/28] add test and return list instead of object (#631) --- measures/forms.py | 2 +- measures/tests/test_forms.py | 2 ++ 2 files changed, 3 insertions(+), 1 deletion(-) diff --git a/measures/forms.py b/measures/forms.py index c5e5e8c69..74532aad1 100644 --- a/measures/forms.py +++ b/measures/forms.py @@ -908,7 +908,7 @@ def clean(self): elif geo_area_choice == GeoAreaType.GROUP: data_key = SUBFORM_PREFIX_MAPPING[geo_area_choice] - cleaned_data["geo_area_list"] = cleaned_data[data_key] + cleaned_data["geo_area_list"] = [cleaned_data[data_key]] elif geo_area_choice == GeoAreaType.COUNTRY: field_name = geographical_area_fields[geo_area_choice] diff --git a/measures/tests/test_forms.py b/measures/tests/test_forms.py index e8b7679de..0706d1117 100644 --- a/measures/tests/test_forms.py +++ b/measures/tests/test_forms.py @@ -194,6 +194,8 @@ def test_measure_forms_geo_area_valid_data_geo_group(erga_omnes): prefix=GEO_AREA_FORM_PREFIX, ) assert form.is_valid() + # https://uktrade.atlassian.net/browse/TP2000-437 500 error where object instead of a list of objects + assert type(form.cleaned_data["geo_area_list"]) == list def test_measure_forms_geo_area_valid_data_countries(erga_omnes): From fafaf84ffdeff06834c412775ec1da074420681f Mon Sep 17 00:00:00 2001 From: Edie Pearce Date: Wed, 27 Jul 2022 14:00:02 +0100 Subject: [PATCH 17/28] Use include for main menu link, replace old index url (#632) --- commodities/jinja2/commodities/import-success.jinja | 2 +- common/jinja2/includes/common/main-menu-link.jinja | 3 +++ common/jinja2/layouts/confirm.jinja | 4 +--- measures/jinja2/measures/confirm-create-multiple.jinja | 4 +--- .../jinja2/workbaskets/delete_changes_confirm.jinja | 2 +- workbaskets/tests/test_views.py | 9 +++++++++ 6 files changed, 16 insertions(+), 8 deletions(-) create mode 100644 common/jinja2/includes/common/main-menu-link.jinja diff --git a/commodities/jinja2/commodities/import-success.jinja b/commodities/jinja2/commodities/import-success.jinja index c9708a9d8..3979b9e72 100644 --- a/commodities/jinja2/commodities/import-success.jinja +++ b/commodities/jinja2/commodities/import-success.jinja @@ -20,7 +20,7 @@

Your file is being imported

We have confirmed that the file is valid and are processing the changes. This might take up to 20 minutes.

You will be able to review the changes and finish the import from the main page.

- Return to the main page + {% include "includes/common/main-menu-link.jinja" %} {% endblock %} diff --git a/common/jinja2/includes/common/main-menu-link.jinja b/common/jinja2/includes/common/main-menu-link.jinja new file mode 100644 index 000000000..4880ea75e --- /dev/null +++ b/common/jinja2/includes/common/main-menu-link.jinja @@ -0,0 +1,3 @@ +{% if request.session.workbasket %} +
  • Return to main menu
    • +{% endif %} diff --git a/common/jinja2/layouts/confirm.jinja b/common/jinja2/layouts/confirm.jinja index c509cc198..2347fd784 100644 --- a/common/jinja2/layouts/confirm.jinja +++ b/common/jinja2/layouts/confirm.jinja @@ -32,9 +32,7 @@ diff --git a/measures/jinja2/measures/confirm-create-multiple.jinja b/measures/jinja2/measures/confirm-create-multiple.jinja index cd49e29b3..2c03bc695 100644 --- a/measures/jinja2/measures/confirm-create-multiple.jinja +++ b/measures/jinja2/measures/confirm-create-multiple.jinja @@ -49,9 +49,7 @@ diff --git a/workbaskets/jinja2/workbaskets/delete_changes_confirm.jinja b/workbaskets/jinja2/workbaskets/delete_changes_confirm.jinja index e67f91126..d128a8ec1 100644 --- a/workbaskets/jinja2/workbaskets/delete_changes_confirm.jinja +++ b/workbaskets/jinja2/workbaskets/delete_changes_confirm.jinja @@ -35,7 +35,7 @@ }) }}

    - Return to main menu + {% include "includes/common/main-menu-link.jinja" %}

    diff --git a/workbaskets/tests/test_views.py b/workbaskets/tests/test_views.py index e557ec330..1bbb44aed 100644 --- a/workbaskets/tests/test_views.py +++ b/workbaskets/tests/test_views.py @@ -335,3 +335,12 @@ def test_review_workbasket_redirects( elif form_action == "page-next": assert "?page=3" in response.url + + +def test_delete_changes_confirm_200(valid_user_client, session_workbasket): + url = reverse( + "workbaskets:workbasket-ui-delete-changes-done", + kwargs={"pk": session_workbasket.pk}, + ) + response = valid_user_client.get(url) + assert response.status_code == 200 From a8b8e6ac61a51290e734e0fe254b84af44c7d065 Mon Sep 17 00:00:00 2001 From: "dependabot[bot]" <49699333+dependabot[bot]@users.noreply.github.com> Date: Thu, 28 Jul 2022 12:03:52 +0100 Subject: [PATCH 18/28] Bump terser from 5.10.0 to 5.14.2 (#626) Bumps [terser](https://github.com/terser/terser) from 5.10.0 to 5.14.2. - [Release notes](https://github.com/terser/terser/releases) - [Changelog](https://github.com/terser/terser/blob/master/CHANGELOG.md) - [Commits](https://github.com/terser/terser/commits) --- updated-dependencies: - dependency-name: terser dependency-type: indirect ... Signed-off-by: dependabot[bot] Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> Co-authored-by: Gabe Naughton --- package-lock.json | 136 +++++++++++++++++++++++++++++++++++----------- 1 file changed, 105 insertions(+), 31 deletions(-) diff --git a/package-lock.json b/package-lock.json index a5700f2d7..d6ee37bda 100644 --- a/package-lock.json +++ b/package-lock.json @@ -38,6 +38,58 @@ "node": ">=10.0.0" } }, + "node_modules/@jridgewell/gen-mapping": { + "version": "0.3.2", + "resolved": "https://registry.npmjs.org/@jridgewell/gen-mapping/-/gen-mapping-0.3.2.tgz", + "integrity": "sha512-mh65xKQAzI6iBcFzwv28KVWSmCkdRBWoOh+bYQGW3+6OZvbbN3TqMGo5hqYxQniRcH9F2VZIoJCm4pa3BPDK/A==", + "dependencies": { + "@jridgewell/set-array": "^1.0.1", + "@jridgewell/sourcemap-codec": "^1.4.10", + "@jridgewell/trace-mapping": "^0.3.9" + }, + "engines": { + "node": ">=6.0.0" + } + }, + "node_modules/@jridgewell/resolve-uri": { + "version": "3.1.0", + "resolved": "https://registry.npmjs.org/@jridgewell/resolve-uri/-/resolve-uri-3.1.0.tgz", + "integrity": "sha512-F2msla3tad+Mfht5cJq7LSXcdudKTWCVYUgw6pLFOOHSTtZlj6SWNYAp+AhuqLmWdBO2X5hPrLcu8cVP8fy28w==", + "engines": { + "node": ">=6.0.0" + } + }, + "node_modules/@jridgewell/set-array": { + "version": "1.1.2", + "resolved": "https://registry.npmjs.org/@jridgewell/set-array/-/set-array-1.1.2.tgz", + "integrity": "sha512-xnkseuNADM0gt2bs+BvhO0p78Mk762YnZdsuzFV018NoG1Sj1SCQvpSqa7XUaTam5vAGasABV9qXASMKnFMwMw==", + "engines": { + "node": ">=6.0.0" + } + }, + "node_modules/@jridgewell/source-map": { + "version": "0.3.2", + "resolved": "https://registry.npmjs.org/@jridgewell/source-map/-/source-map-0.3.2.tgz", + "integrity": "sha512-m7O9o2uR8k2ObDysZYzdfhb08VuEml5oWGiosa1VdaPZ/A6QyPkAJuwN0Q1lhULOf6B7MtQmHENS743hWtCrgw==", + "dependencies": { + "@jridgewell/gen-mapping": "^0.3.0", + "@jridgewell/trace-mapping": "^0.3.9" + } + }, + "node_modules/@jridgewell/sourcemap-codec": { + "version": "1.4.14", + "resolved": "https://registry.npmjs.org/@jridgewell/sourcemap-codec/-/sourcemap-codec-1.4.14.tgz", + "integrity": "sha512-XPSJHWmi394fuUuzDnGz1wiKqWfo1yXecHQMRf2l6hztTO+nPru658AyDngaBe7isIxEkRsPR3FZh+s7iVa4Uw==" + }, + "node_modules/@jridgewell/trace-mapping": { + "version": "0.3.14", + "resolved": "https://registry.npmjs.org/@jridgewell/trace-mapping/-/trace-mapping-0.3.14.tgz", + "integrity": "sha512-bJWEfQ9lPTvm3SneWwRFVLzrh6nhjwqw7TUFFBEMzwvg7t7PCDenf2lDwqo4NQXzdpgBXyFgDWnQA+2vkruksQ==", + "dependencies": { + "@jridgewell/resolve-uri": "^3.0.3", + "@jridgewell/sourcemap-codec": "^1.4.10" + } + }, "node_modules/@types/eslint": { "version": "8.4.1", "resolved": "https://registry.npmjs.org/@types/eslint/-/eslint-8.4.1.tgz", @@ -1642,12 +1694,13 @@ } }, "node_modules/terser": { - "version": "5.10.0", - "resolved": "https://registry.npmjs.org/terser/-/terser-5.10.0.tgz", - "integrity": "sha512-AMmF99DMfEDiRJfxfY5jj5wNH/bYO09cniSqhfoyxc8sFoYIgkJy86G04UoZU5VjlpnplVu0K6Tx6E9b5+DlHA==", + "version": "5.14.2", + "resolved": "https://registry.npmjs.org/terser/-/terser-5.14.2.tgz", + "integrity": "sha512-oL0rGeM/WFQCUd0y2QrWxYnq7tfSuKBiqTjRPWrRgB46WD/kiwHwF8T23z78H6Q6kGCuuHcPB+KULHRdxvVGQA==", "dependencies": { + "@jridgewell/source-map": "^0.3.2", + "acorn": "^8.5.0", "commander": "^2.20.0", - "source-map": "~0.7.2", "source-map-support": "~0.5.20" }, "bin": { @@ -1655,14 +1708,6 @@ }, "engines": { "node": ">=10" - }, - "peerDependencies": { - "acorn": "^8.5.0" - }, - "peerDependenciesMeta": { - "acorn": { - "optional": true - } } }, "node_modules/terser-webpack-plugin": { @@ -1698,14 +1743,6 @@ } } }, - "node_modules/terser/node_modules/source-map": { - "version": "0.7.3", - "resolved": "https://registry.npmjs.org/source-map/-/source-map-0.7.3.tgz", - "integrity": "sha512-CkCj6giN3S+n9qrYiBTX5gystlENnRW5jZeNLHpe6aue+SrHcG5VYwujhW9s4dY31mEGsxBDrHR6oI69fTXsaQ==", - "engines": { - "node": ">= 8" - } - }, "node_modules/to-regex-range": { "version": "5.0.1", "resolved": "https://registry.npmjs.org/to-regex-range/-/to-regex-range-5.0.1.tgz", @@ -1916,6 +1953,49 @@ "integrity": "sha512-ws57AidsDvREKrZKYffXddNkyaF14iHNHm8VQnZH6t99E8gczjNN0GpvcGny0imC80yQ0tHz1xVUKk/KFQSUyA==", "dev": true }, + "@jridgewell/gen-mapping": { + "version": "0.3.2", + "resolved": "https://registry.npmjs.org/@jridgewell/gen-mapping/-/gen-mapping-0.3.2.tgz", + "integrity": "sha512-mh65xKQAzI6iBcFzwv28KVWSmCkdRBWoOh+bYQGW3+6OZvbbN3TqMGo5hqYxQniRcH9F2VZIoJCm4pa3BPDK/A==", + "requires": { + "@jridgewell/set-array": "^1.0.1", + "@jridgewell/sourcemap-codec": "^1.4.10", + "@jridgewell/trace-mapping": "^0.3.9" + } + }, + "@jridgewell/resolve-uri": { + "version": "3.1.0", + "resolved": "https://registry.npmjs.org/@jridgewell/resolve-uri/-/resolve-uri-3.1.0.tgz", + "integrity": "sha512-F2msla3tad+Mfht5cJq7LSXcdudKTWCVYUgw6pLFOOHSTtZlj6SWNYAp+AhuqLmWdBO2X5hPrLcu8cVP8fy28w==" + }, + "@jridgewell/set-array": { + "version": "1.1.2", + "resolved": "https://registry.npmjs.org/@jridgewell/set-array/-/set-array-1.1.2.tgz", + "integrity": "sha512-xnkseuNADM0gt2bs+BvhO0p78Mk762YnZdsuzFV018NoG1Sj1SCQvpSqa7XUaTam5vAGasABV9qXASMKnFMwMw==" + }, + "@jridgewell/source-map": { + "version": "0.3.2", + "resolved": "https://registry.npmjs.org/@jridgewell/source-map/-/source-map-0.3.2.tgz", + "integrity": "sha512-m7O9o2uR8k2ObDysZYzdfhb08VuEml5oWGiosa1VdaPZ/A6QyPkAJuwN0Q1lhULOf6B7MtQmHENS743hWtCrgw==", + "requires": { + "@jridgewell/gen-mapping": "^0.3.0", + "@jridgewell/trace-mapping": "^0.3.9" + } + }, + "@jridgewell/sourcemap-codec": { + "version": "1.4.14", + "resolved": "https://registry.npmjs.org/@jridgewell/sourcemap-codec/-/sourcemap-codec-1.4.14.tgz", + "integrity": "sha512-XPSJHWmi394fuUuzDnGz1wiKqWfo1yXecHQMRf2l6hztTO+nPru658AyDngaBe7isIxEkRsPR3FZh+s7iVa4Uw==" + }, + "@jridgewell/trace-mapping": { + "version": "0.3.14", + "resolved": "https://registry.npmjs.org/@jridgewell/trace-mapping/-/trace-mapping-0.3.14.tgz", + "integrity": "sha512-bJWEfQ9lPTvm3SneWwRFVLzrh6nhjwqw7TUFFBEMzwvg7t7PCDenf2lDwqo4NQXzdpgBXyFgDWnQA+2vkruksQ==", + "requires": { + "@jridgewell/resolve-uri": "^3.0.3", + "@jridgewell/sourcemap-codec": "^1.4.10" + } + }, "@types/eslint": { "version": "8.4.1", "resolved": "https://registry.npmjs.org/@types/eslint/-/eslint-8.4.1.tgz", @@ -3054,20 +3134,14 @@ "integrity": "sha512-GNzQvQTOIP6RyTfE2Qxb8ZVlNmw0n88vp1szwWRimP02mnTsx3Wtn5qRdqY9w2XduFNUgvOwhNnQsjwCp+kqaQ==" }, "terser": { - "version": "5.10.0", - "resolved": "https://registry.npmjs.org/terser/-/terser-5.10.0.tgz", - "integrity": "sha512-AMmF99DMfEDiRJfxfY5jj5wNH/bYO09cniSqhfoyxc8sFoYIgkJy86G04UoZU5VjlpnplVu0K6Tx6E9b5+DlHA==", + "version": "5.14.2", + "resolved": "https://registry.npmjs.org/terser/-/terser-5.14.2.tgz", + "integrity": "sha512-oL0rGeM/WFQCUd0y2QrWxYnq7tfSuKBiqTjRPWrRgB46WD/kiwHwF8T23z78H6Q6kGCuuHcPB+KULHRdxvVGQA==", "requires": { + "@jridgewell/source-map": "^0.3.2", + "acorn": "^8.5.0", "commander": "^2.20.0", - "source-map": "~0.7.2", "source-map-support": "~0.5.20" - }, - "dependencies": { - "source-map": { - "version": "0.7.3", - "resolved": "https://registry.npmjs.org/source-map/-/source-map-0.7.3.tgz", - "integrity": "sha512-CkCj6giN3S+n9qrYiBTX5gystlENnRW5jZeNLHpe6aue+SrHcG5VYwujhW9s4dY31mEGsxBDrHR6oI69fTXsaQ==" - } } }, "terser-webpack-plugin": { From 0a08c9d0971d187d5803f0c376a74a2887b50e7a Mon Sep 17 00:00:00 2001 From: Gabe Naughton Date: Fri, 29 Jul 2022 08:54:58 +0100 Subject: [PATCH 19/28] TP2000-225 Users cannot find some countries from geo area autocomplete or search (#628) * refactor with_latest_description_string to remove filter logic * add with_current_description annotate method using subquery * add docstrings and view tests * fix queryset test * move method from util to queryset --- geo_areas/models.py | 15 ++++++++ geo_areas/tests/test_models.py | 19 ++++++++++ geo_areas/tests/test_util.py | 38 -------------------- geo_areas/tests/test_views.py | 46 ++++++++++++++++++++++++ geo_areas/util.py | 16 --------- geo_areas/views.py | 14 ++++++-- measures/forms.py | 64 ++++++++++++---------------------- 7 files changed, 114 insertions(+), 98 deletions(-) delete mode 100644 geo_areas/tests/test_util.py delete mode 100644 geo_areas/util.py diff --git a/geo_areas/models.py b/geo_areas/models.py index 4d0de75e1..26b9328bc 100644 --- a/geo_areas/models.py +++ b/geo_areas/models.py @@ -1,7 +1,9 @@ from django.db import models from django.db.models import CheckConstraint from django.db.models import Max +from django.db.models import OuterRef from django.db.models import Q +from django.db.models import Subquery from polymorphic.managers import PolymorphicManager from common.business_rules import UniqueIdentifyingFields @@ -24,6 +26,19 @@ class GeographicalAreaQuerySet(TrackedModelQuerySet): def erga_omnes(self): return self.filter(area_code=AreaCode.GROUP, area_id=1011) + def with_current_description(qs): + """Returns a GeographicalArea queryset annotated with the latest result + of a GeographicalAreaDescription subquery's description value, linking + these two queries on version_group field.""" + current_descriptions = ( + GeographicalAreaDescription.objects.current() + .filter(described_geographicalarea__version_group=OuterRef("version_group")) + .order_by("transaction__order") + ) + return qs.annotate( + description=Subquery(current_descriptions.values("description")[:1]), + ) + class GeographicalArea(TrackedModel, ValidityMixin, DescribedMixin): """ diff --git a/geo_areas/tests/test_models.py b/geo_areas/tests/test_models.py index 84d2bec9c..a68f3c878 100644 --- a/geo_areas/tests/test_models.py +++ b/geo_areas/tests/test_models.py @@ -3,6 +3,7 @@ from common.models.transactions import Transaction from common.models.utils import set_current_transaction from common.tests import factories +from geo_areas import models pytestmark = pytest.mark.django_db @@ -129,3 +130,21 @@ def test_geo_area_update_types( check_update_validation, ): assert check_update_validation(factory) + + +def test_with_current_description(): + """Tests that, after updating a geo area description, + with_current_description returns a queryset with one geo area annotated with + only the latest description.""" + description = factories.GeographicalAreaDescriptionFactory.create( + description="blarghhh", + ) + current_description = description.new_version( + description.transaction.workbasket, + description="bleurgh", + ) + set_current_transaction(current_description.transaction) + qs = models.GeographicalArea.objects.current().with_current_description() + + assert qs.count() == 1 + assert qs.first().description == "bleurgh" diff --git a/geo_areas/tests/test_util.py b/geo_areas/tests/test_util.py deleted file mode 100644 index 5ed3c3202..000000000 --- a/geo_areas/tests/test_util.py +++ /dev/null @@ -1,38 +0,0 @@ -from datetime import datetime -from datetime import timedelta - -import pytest - -from common.tests import factories -from geo_areas import models -from geo_areas import util - -pytestmark = pytest.mark.django_db - - -def test_with_latest_description_returns_description_string(): - factories.GeographicalAreaDescriptionFactory.create(description="Guernsey") - geo_area = util.with_latest_description_string( - models.GeographicalArea.objects.all(), - ).first() - - assert geo_area.description == "Guernsey" - - -def test_with_latest_description_multiple_descriptions(): - """Tests that when a GeographicalArea has more than one description, the - description with a later validity_start date is used to generate the - description string.""" - area = factories.GeographicalAreaFactory.create() - earlier_description = factories.GeographicalAreaDescriptionFactory.create( - validity_start=datetime.today(), - described_geographicalarea=area, - ) - later_description = factories.GeographicalAreaDescriptionFactory.create( - validity_start=datetime.today() + timedelta(days=1), - described_geographicalarea=area, - ) - qs = util.with_latest_description_string(models.GeographicalArea.objects.all()) - - assert qs.count() == 1 - assert later_description.description == qs.first().description diff --git a/geo_areas/tests/test_views.py b/geo_areas/tests/test_views.py index d68fa917f..aa73a353d 100644 --- a/geo_areas/tests/test_views.py +++ b/geo_areas/tests/test_views.py @@ -1,5 +1,8 @@ import pytest +from bs4 import BeautifulSoup +from rest_framework.reverse import reverse +from common.models.utils import set_current_transaction from common.tests import factories from common.tests.util import assert_model_view_renders from common.tests.util import get_class_based_view_urls_matching_url @@ -57,3 +60,46 @@ def test_geographical_area_list_view(view, url_pattern, valid_user_client): """Verify that geographical list view is under the url geographical-areas/ and doesn't return an error.""" assert_model_view_renders(view, url_pattern, valid_user_client) + + +def test_geographical_area_list_queryset(): + """Tests that geo area list queryset contains only the latest version of a + geo_area annotated with the current description.""" + description = factories.GeographicalAreaDescriptionFactory.create( + description="Englund", # /PS-IGNORE + ) + new_description = description.new_version( + description.transaction.workbasket, + description="England", # /PS-IGNORE + ) + new_area = new_description.described_geographicalarea.new_version( + description.transaction.workbasket, + ) + view = GeoAreaList() + qs = view.get_queryset() + set_current_transaction(new_area.transaction) + + assert qs.count() == 1 + assert qs.first().description == "England" # /PS-IGNORE + assert qs.first() == new_area + + +# https://uktrade.atlassian.net/browse/TP2000-225 +@pytest.mark.parametrize("search_terms", ["greenla", "gREEnla", "GL", "gl"]) +def test_geographical_area_list_filter(search_terms, valid_user_client): + """Tests that an updated geographical area is still searchable by the + description created alongside the original version.""" + geo_area = factories.GeographicalAreaDescriptionFactory.create( + description="Greenland", + described_geographicalarea__area_id="GL", + ).described_geographicalarea + geo_area.new_version(geo_area.transaction.workbasket) + list_url = reverse("geo_area-ui-list") + url = f"{list_url}?search={search_terms}" + response = valid_user_client.get(url) + page = BeautifulSoup( + response.content.decode(response.charset), + "html.parser", + ) + + assert page.find("tbody").find("td", text="Greenland") diff --git a/geo_areas/util.py b/geo_areas/util.py deleted file mode 100644 index 1eb44f081..000000000 --- a/geo_areas/util.py +++ /dev/null @@ -1,16 +0,0 @@ -from django.db import models - - -def with_latest_description_string(qs): - """Returns a queryset annotate with its latest description's validity_start - date, filtered by this date and then annotated with that latest description - object's description field value.""" - return ( - qs.annotate(latest_description_date=models.Max("descriptions__validity_start")) - .filter(descriptions__validity_start=models.F("latest_description_date")) - .annotate( - description=models.F( - "descriptions__description", - ), - ) - ) diff --git a/geo_areas/views.py b/geo_areas/views.py index 70d851d72..520089c12 100644 --- a/geo_areas/views.py +++ b/geo_areas/views.py @@ -22,9 +22,14 @@ class GeoAreaViewSet(viewsets.ReadOnlyModelViewSet): """API endpoint that allows geographical areas to be viewed.""" - queryset = GeographicalArea.objects.latest_approved().prefetch_related( - "descriptions", + queryset = ( + GeographicalArea.objects.latest_approved() + .with_current_description() + .prefetch_related( + "descriptions", + ) ) + serializer_class = AutoCompleteSerializer permission_classes = [permissions.IsAuthenticated] filterset_class = GeographicalAreaFilter @@ -61,7 +66,10 @@ def get_context_data(self, **kwargs): class GeoAreaList(GeoAreaMixin, TamatoListView): template_name = "geo_areas/list.jinja" filterset_class = GeographicalAreaFilter - search_fields = ["sid", "descriptions__description"] + filterset_class.search_fields = ["area_id", "description"] + + def get_queryset(self): + return GeographicalArea.objects.current().with_current_description() class GeoAreaDetail(GeoAreaMixin, TrackedModelDetailView): diff --git a/measures/forms.py b/measures/forms.py index 74532aad1..7f790e29e 100644 --- a/measures/forms.py +++ b/measures/forms.py @@ -29,7 +29,6 @@ from common.validators import UpdateType from footnotes.models import Footnote from geo_areas.models import GeographicalArea -from geo_areas.util import with_latest_description_string from geo_areas.validators import AreaCode from measures import models from measures.parsers import DutySentenceParser @@ -91,19 +90,14 @@ class GeoGroupForm(forms.Form): def __init__(self, *args, **kwargs): super().__init__(*args, **kwargs) - self.fields[ - "geographical_area_group" - ].queryset = with_latest_description_string( - GeographicalArea.objects.exclude( - descriptions__description__isnull=True, - ) + self.fields["geographical_area_group"].queryset = ( + GeographicalArea.objects.current() + .with_current_description() + .filter(area_code=AreaCode.GROUP) .as_at_today() - .current() - .with_latest_links("descriptions") - .prefetch_related("descriptions") - .order_by("descriptions__description"), - # descriptions__description" should make this implicitly distinct() + .order_by("description") ) + # descriptions__description" should make this implicitly distinct() self.fields[ "geographical_area_group" ].label_from_instance = lambda obj: obj.description @@ -124,16 +118,11 @@ class ErgaOmnesExclusionsForm(forms.Form): def __init__(self, *args, **kwargs): super().__init__(*args, **kwargs) - self.fields["erga_omnes_exclusion"].queryset = with_latest_description_string( - GeographicalArea.objects.exclude( - descriptions__description__isnull=True, - ) + self.fields["erga_omnes_exclusion"].queryset = ( + GeographicalArea.objects.current() + .with_current_description() .as_at_today() - .current() - .with_latest_links("descriptions") - .prefetch_related("descriptions") - .order_by("descriptions__description"), - # descriptions__description" should make this implicitly distinct() + .order_by("description") ) self.fields[ "erga_omnes_exclusion" @@ -152,16 +141,11 @@ class GeoGroupExclusionsForm(forms.Form): def __init__(self, *args, **kwargs): super().__init__(*args, **kwargs) - self.fields["geo_group_exclusion"].queryset = with_latest_description_string( - GeographicalArea.objects.exclude( - descriptions__description__isnull=True, - ) + self.fields["geo_group_exclusion"].queryset = ( + GeographicalArea.objects.current() + .with_current_description() .as_at_today() - .current() - .with_latest_links("descriptions") - .prefetch_related("descriptions") - .order_by("descriptions__description"), - # descriptions__description" should make this implicitly distinct() + .order_by("description") ) self.fields[ "geo_group_exclusion" @@ -206,11 +190,9 @@ class CountryRegionForm(forms.Form): prefix = COUNTRY_REGION_PREFIX geographical_area_country_or_region = forms.ModelChoiceField( - queryset=with_latest_description_string( - GeographicalArea.objects.exclude( - area_code=AreaCode.GROUP, - descriptions__description__isnull=True, - ), + queryset=GeographicalArea.objects.exclude( + area_code=AreaCode.GROUP, + descriptions__description__isnull=True, ), error_messages={"required": "A country or region is required."}, ) @@ -218,13 +200,13 @@ class CountryRegionForm(forms.Form): def __init__(self, *args, **kwargs): super().__init__(*args, **kwargs) self.fields["geographical_area_country_or_region"].queryset = ( - self.fields["geographical_area_country_or_region"] - .queryset.as_at_today() - .current() - .with_latest_links("descriptions") - .prefetch_related("descriptions") - .order_by("descriptions__description") + GeographicalArea.objects.current() + .with_current_description() + .exclude(area_code=AreaCode.GROUP) + .as_at_today() + .order_by("description") ) + self.fields[ "geographical_area_country_or_region" ].label_from_instance = lambda obj: obj.description From 575f0f6e4e91c5d14ee53ccf4476d0844c16002b Mon Sep 17 00:00:00 2001 From: Gabe Naughton Date: Fri, 29 Jul 2022 12:35:50 +0100 Subject: [PATCH 20/28] TP2000-446 Implement a guard against double form submits (#636) * add data-prevent-double-click to buttons * add data-module attribute --- additional_codes/forms.py | 14 ++++++++++-- certificates/forms.py | 14 ++++++++++-- commodities/forms.py | 7 +++++- common/forms.py | 22 ++++++++++++++++--- footnotes/forms.py | 14 ++++++++++-- .../jinja2/footnotes/edit_description.jinja | 2 +- measures/forms.py | 21 +++++++++++++++--- measures/jinja2/measures/create-review.jinja | 2 +- measures/jinja2/measures/edit.jinja | 1 + regulations/forms.py | 14 ++++++++++-- workbaskets/forms.py | 7 +++++- 11 files changed, 100 insertions(+), 18 deletions(-) diff --git a/additional_codes/forms.py b/additional_codes/forms.py index 815672de1..bae2dacf7 100644 --- a/additional_codes/forms.py +++ b/additional_codes/forms.py @@ -54,7 +54,12 @@ def __init__(self, *args, **kwargs): Field("type"), Field("start_date"), Field("end_date"), - Submit("submit", "Save"), + Submit( + "submit", + "Save", + data_module="govuk-button", + data_prevent_double_click="true", + ), ) def clean(self): @@ -122,7 +127,12 @@ def __init__(self, *args, **kwargs): "start_date", Field.textarea("description", rows=5), DescriptionHelpBox(), - Submit("submit", "Save"), + Submit( + "submit", + "Save", + data_module="govuk-button", + data_prevent_double_click="true", + ), ) def clean(self): diff --git a/certificates/forms.py b/certificates/forms.py index ef447d5ba..a0fd69f99 100644 --- a/certificates/forms.py +++ b/certificates/forms.py @@ -57,7 +57,12 @@ def __init__(self, *args, **kwargs): "start_date", Field.textarea("description", rows=5), DescriptionHelpBox(), - Submit("submit", "Save"), + Submit( + "submit", + "Save", + data_module="govuk-button", + data_prevent_double_click="true", + ), ) def clean_sid(self): @@ -142,7 +147,12 @@ def __init__(self, *args, **kwargs): "certificate_type", "start_date", "end_date", - Submit("submit", "Save"), + Submit( + "submit", + "Save", + data_module="govuk-button", + data_prevent_double_click="true", + ), ) def clean(self): diff --git a/commodities/forms.py b/commodities/forms.py index e946bf903..5f1b626ff 100644 --- a/commodities/forms.py +++ b/commodities/forms.py @@ -43,7 +43,12 @@ def __init__(self, *args, **kwargs): self.helper = FormHelper(self) self.helper.layout = Layout( "taric_file", - Submit("submit", "Continue"), + Submit( + "submit", + "Continue", + data_module="govuk-button", + data_prevent_double_click="true", + ), ) def clean_taric_file(self): diff --git a/common/forms.py b/common/forms.py index 1fe1bf9d0..f4dccfd4d 100644 --- a/common/forms.py +++ b/common/forms.py @@ -175,7 +175,12 @@ def __init__(self, *args, **kwargs): self.helper.legend_size = Size.EXTRA_LARGE self.helper.layout = Layout( "workbasket_action", - Submit("submit", "Continue"), + Submit( + "submit", + "Continue", + data_module="govuk-button", + data_prevent_double_click="true", + ), ) @@ -289,7 +294,12 @@ def __init__(self, *args, **kwargs): self.helper.layout = Layout( Field("validity_start", context={"legend_size": "govuk-label--s"}), Field.textarea("description", label_size=Size.SMALL, rows=5), - Submit("submit", "Save"), + Submit( + "submit", + "Save", + data_module="govuk-button", + data_prevent_double_click="true", + ), ) class Meta: @@ -376,7 +386,13 @@ def __init__(self, *args, **kwargs): self.helper.label_size = Size.SMALL self.helper.legend_size = Size.SMALL self.helper.layout = Layout( - Submit("submit", "Delete", css_class="govuk-button--warning"), + Submit( + "submit", + "Delete", + css_class="govuk-button--warning", + data_module="govuk-button", + data_prevent_double_click="true", + ), ) diff --git a/footnotes/forms.py b/footnotes/forms.py index 9d05db778..a1b3b0b59 100644 --- a/footnotes/forms.py +++ b/footnotes/forms.py @@ -53,7 +53,12 @@ def __init__(self, *args, **kwargs): Field("footnote_type"), Field("start_date"), Field("end_date"), - Submit("submit", "Save"), + Submit( + "submit", + "Save", + data_module="govuk-button", + data_prevent_double_click="true", + ), ) def clean(self): @@ -111,7 +116,12 @@ def __init__(self, *args, **kwargs): "start_date", Field.textarea("description", rows=5), DescriptionHelpBox(), - Submit("submit", "Save"), + Submit( + "submit", + "Save", + data_module="govuk-button", + data_prevent_double_click="true", + ), ) def clean(self): diff --git a/footnotes/jinja2/footnotes/edit_description.jinja b/footnotes/jinja2/footnotes/edit_description.jinja index 842f9b7f9..a87ee6f46 100644 --- a/footnotes/jinja2/footnotes/edit_description.jinja +++ b/footnotes/jinja2/footnotes/edit_description.jinja @@ -43,6 +43,6 @@ }) }}

    Finish now

    - {{ govukButton({"text": "Add to workbasket"}) }} Cancel + {{ govukButton({"text": "Add to workbasket", "preventDoubleClick": true,}) }} Cancel {% endcall %} {% endblock %} diff --git a/measures/forms.py b/measures/forms.py index 7f790e29e..a43e61230 100644 --- a/measures/forms.py +++ b/measures/forms.py @@ -796,7 +796,12 @@ def __init__(self, *args, **kwargs): "order_number", "start_date", "end_date", - Submit("submit", "Continue"), + Submit( + "submit", + "Continue", + data_module="govuk-button", + data_prevent_double_click="true", + ), ) def clean(self): @@ -865,7 +870,12 @@ def __init__(self, *args, **kwargs): "from or exports to the selected area." ), ), - Submit("submit", "Continue"), + Submit( + "submit", + "Continue", + data_module="govuk-button", + data_prevent_double_click="true", + ), ) @property @@ -933,7 +943,12 @@ def __init__(self, *args, **kwargs): self.helper = FormHelper(self) self.helper.layout = Layout( "additional_code", - Submit("submit", "Continue"), + Submit( + "submit", + "Continue", + data_module="govuk-button", + data_prevent_double_click="true", + ), ) diff --git a/measures/jinja2/measures/create-review.jinja b/measures/jinja2/measures/create-review.jinja index c5c8adddf..10226601b 100644 --- a/measures/jinja2/measures/create-review.jinja +++ b/measures/jinja2/measures/create-review.jinja @@ -115,5 +115,5 @@ {% endif %} {% endcall %} - {{ govukButton({"text": "Create"}) }} + {{ govukButton({"text": "Create", "preventDoubleClick": true,}) }} {% endblock %} diff --git a/measures/jinja2/measures/edit.jinja b/measures/jinja2/measures/edit.jinja index f798e46f6..cefc458b3 100644 --- a/measures/jinja2/measures/edit.jinja +++ b/measures/jinja2/measures/edit.jinja @@ -144,6 +144,7 @@ }) }} {{ govukButton({ "text":"Save", + "preventDoubleClick": true, }) }} diff --git a/regulations/forms.py b/regulations/forms.py index 13dba692d..1c08d1ff1 100644 --- a/regulations/forms.py +++ b/regulations/forms.py @@ -144,7 +144,12 @@ def __init__(self, *args, **kwargs): ), "approved", ), - Submit("submit", "Save"), + Submit( + "submit", + "Save", + data_module="govuk-button", + data_prevent_double_click="true", + ), ) def _make_partial_regulation_id( @@ -295,7 +300,12 @@ def __init__(self, *args, **kwargs): ), "approved", ), - Submit("submit", "Save"), + Submit( + "submit", + "Save", + data_module="govuk-button", + data_prevent_double_click="true", + ), ) def clean(self): diff --git a/workbaskets/forms.py b/workbaskets/forms.py index ac32a9cdf..fdfb4784b 100644 --- a/workbaskets/forms.py +++ b/workbaskets/forms.py @@ -36,7 +36,12 @@ def __init__(self, *args, **kwargs): "title", Field.textarea("reason", rows=5), DescriptionHelpBox(), - Submit("submit", "Create"), + Submit( + "submit", + "Create", + data_module="govuk-button", + data_prevent_double_click="true", + ), ) class Meta: From ff30bfd70e4296bc969955793a43af904ef0c6cf Mon Sep 17 00:00:00 2001 From: Gabe Naughton Date: Mon, 1 Aug 2022 11:26:52 +0100 Subject: [PATCH 21/28] add new business rules and tests (#635) --- quotas/business_rules.py | 27 +++++++++++++++++++++++ quotas/models.py | 2 ++ quotas/tests/test_business_rules.py | 34 +++++++++++++++++++++++++++++ 3 files changed, 63 insertions(+) diff --git a/quotas/business_rules.py b/quotas/business_rules.py index e3c292d6e..899248e27 100644 --- a/quotas/business_rules.py +++ b/quotas/business_rules.py @@ -250,6 +250,24 @@ def get_relation_model(self, quota_definition): return quota_definition.quotablocking_set.model +class OverlappingQuotaDefinition(BusinessRule): + """There may be no overlap in time of two quota definitions with the same + quota order number id.""" + + def validate(self, quota_definition): + if ( + type(quota_definition) + .objects.approved_up_to_transaction(quota_definition.transaction) + .filter( + order_number=quota_definition.order_number, + valid_between__overlap=quota_definition.valid_between, + ) + .exclude(sid=quota_definition.sid) + .exists() + ): + raise self.violation(quota_definition) + + class QA1(UniqueIdentifyingFields): """The association between two quota definitions must be unique.""" @@ -369,6 +387,15 @@ def validate(self, association): raise self.violation(association) +class SameMainAndSubQuota(BusinessRule): + """A quota association may only exist between two distinct quota + definitions.""" + + def validate(self, association): + if association.main_quota.sid == association.sub_quota.sid: + raise self.violation(association) + + class BlockingOnlyOfFCFSQuotas(BusinessRule): """Blocking periods are only applicable to FCFS quotas.""" diff --git a/quotas/models.py b/quotas/models.py index 3e2791a9a..584cb5bb8 100644 --- a/quotas/models.py +++ b/quotas/models.py @@ -225,6 +225,7 @@ class QuotaDefinition(TrackedModel, ValidityMixin): business_rules.QuotaAssociationMustReferToANonDeletedSubQuota, business_rules.QuotaSuspensionMustReferToANonDeletedQuotaDefinition, business_rules.QuotaBlockingPeriodMustReferToANonDeletedQuotaDefinition, + business_rules.OverlappingQuotaDefinition, UniqueIdentifyingFields, UpdateValidity, ) @@ -287,6 +288,7 @@ class QuotaAssociation(TrackedModel): business_rules.QA5, business_rules.QA6, UpdateValidity, + business_rules.SameMainAndSubQuota, ) diff --git a/quotas/tests/test_business_rules.py b/quotas/tests/test_business_rules.py index 4fb97a42e..dd71331ce 100644 --- a/quotas/tests/test_business_rules.py +++ b/quotas/tests/test_business_rules.py @@ -461,6 +461,23 @@ def test_linking_models_must_refer_to_a_non_deleted_sub_quota( business_rule(deleted.transaction).validate(deleted) +def test_overlapping_quota_definition(date_ranges): + order_number = factories.QuotaOrderNumberFactory.create() + factories.QuotaDefinitionFactory.create( + order_number=order_number, + valid_between=date_ranges.normal, + ) + overlapping_definition = factories.QuotaDefinitionFactory.create( + order_number=order_number, + valid_between=date_ranges.overlap_normal, + ) + + with pytest.raises(BusinessRuleViolation): + business_rules.OverlappingQuotaDefinition( + overlapping_definition.transaction, + ).validate(overlapping_definition) + + def test_QA1(assert_handles_duplicates): """The association between two quota definitions must be unique.""" @@ -604,6 +621,7 @@ def test_QA6(existing_relation, new_relation, error_expected): def test_QA6_new_association_version(): """Tests that previous versions of an association are not compared when looking for sub-quotas associated with the same main quota.""" + original_version = factories.QuotaAssociationFactory.create( sub_quota_relation_type="EQ", ) @@ -615,6 +633,22 @@ def test_QA6_new_association_version(): business_rules.QA6(later_version.transaction).validate(later_version) +def test_same_main_and_sub_quota(): + """A quota association may only exist between two distinct quota + definitions.""" + + definition = factories.QuotaDefinitionFactory.create() + association = factories.QuotaAssociationFactory.create( + main_quota=definition, + sub_quota=definition, + ) + + with pytest.raises(BusinessRuleViolation): + business_rules.SameMainAndSubQuota(association.transaction).validate( + association, + ) + + @pytest.mark.parametrize( ("mechanism", "error_expected"), ( From cec7cb6156ac412d2de41a785c16cfbca73feaf8 Mon Sep 17 00:00:00 2001 From: Gabe Naughton Date: Wed, 3 Aug 2022 10:13:28 +0100 Subject: [PATCH 22/28] TP2000-410 editing measure exclusions only excludes first country of group (#639) * unpack generator, write test (still need to fix test) * add view test --- measures/forms.py | 2 +- .../includes/measures/tabs/core_data.jinja | 4 +- measures/tests/test_views.py | 56 +++++++++++++++++++ 3 files changed, 59 insertions(+), 3 deletions(-) diff --git a/measures/forms.py b/measures/forms.py index a43e61230..88c94e998 100644 --- a/measures/forms.py +++ b/measures/forms.py @@ -623,7 +623,7 @@ def save(self, commit=True): exclusion, ) ) - next(pattern) + [p for p in pattern] if ( self.request.session[f"instance_duty_sentence_{self.instance.sid}"] diff --git a/measures/jinja2/includes/measures/tabs/core_data.jinja b/measures/jinja2/includes/measures/tabs/core_data.jinja index ea5d20be3..c36ff1374 100644 --- a/measures/jinja2/includes/measures/tabs/core_data.jinja +++ b/measures/jinja2/includes/measures/tabs/core_data.jinja @@ -1,8 +1,8 @@ {%- extends "includes/common/tabs/core_data.jinja" -%} {% set geographical_exclusions %} -{% if object.exclusions.all() %} - {% for exclusion in object.exclusions.all() %} +{% if object.exclusions.current() %} + {% for exclusion in object.exclusions.current() %} {{ exclusion.excluded_geographical_area.get_description().description }}{{ ", " if not loop.last else "" }} {% endfor %} {% else %}-{% endif %} diff --git a/measures/tests/test_views.py b/measures/tests/test_views.py index 26567c82c..e58a0977f 100644 --- a/measures/tests/test_views.py +++ b/measures/tests/test_views.py @@ -24,6 +24,7 @@ from measures.models import FootnoteAssociationMeasure from measures.models import Measure from measures.models import MeasureCondition +from measures.models import MeasureExcludedGeographicalArea from measures.validators import validate_duties from measures.views import MeasureCreateWizard from measures.views import MeasureFootnotesUpdate @@ -653,6 +654,61 @@ def test_measure_update_invalid_conditions( ) +def test_measure_update_group_exclusion(client, valid_user, erga_omnes): + """ + Tests that measure edit view handles exclusion of one group from another + group. + + We create an erga omnes measure and a group containing two areas that also + belong to the erga omnes group. Then post to edit endpoint with this group + as an exclusion and check that two MeasureExcludedGeographicalArea objects + are created with the two area sids in that excluded group. + """ + measure = factories.MeasureFactory.create(geographical_area=erga_omnes) + geo_group = factories.GeoGroupFactory.create() + area_1 = factories.GeographicalMembershipFactory.create(geo_group=geo_group).member + area_2 = factories.GeographicalMembershipFactory.create(geo_group=geo_group).member + factories.GeographicalMembershipFactory.create(geo_group=erga_omnes, member=area_1) + factories.GeographicalMembershipFactory.create(geo_group=erga_omnes, member=area_2) + url = reverse("measure-ui-edit", args=(measure.sid,)) + client.force_login(valid_user) + data = model_to_dict(measure) + data = {k: v for k, v in data.items() if v is not None} + start_date = data["valid_between"].lower + data.update( + { + "start_date_0": start_date.day, + "start_date_1": start_date.month, + "start_date_2": start_date.year, + "geo_area": "ERGA_OMNES", + "erga_omnes_exclusions_formset-__prefix__-erga_omnes_exclusion": geo_group.pk, + }, + ) + + assert not MeasureExcludedGeographicalArea.objects.approved_up_to_transaction( + Transaction.objects.last(), + ).exists() + + client.post(url, data=data) + measure_area_exclusions = ( + MeasureExcludedGeographicalArea.objects.approved_up_to_transaction( + Transaction.objects.last(), + ) + ) + + assert measure_area_exclusions.count() == 2 + + area_sids = [ + sid[0] + for sid in measure_area_exclusions.values_list( + "excluded_geographical_area__sid", + ) + ] + + assert area_1.sid in area_sids + assert area_2.sid in area_sids + + @pytest.mark.django_db def test_measure_form_wizard_start(valid_user_client): url = reverse("measure-ui-create", kwargs={"step": "start"}) From ac46ebf4cd338c2a7088b5604faa94070e3a192d Mon Sep 17 00:00:00 2001 From: Edie Pearce Date: Wed, 3 Aug 2022 10:56:06 +0100 Subject: [PATCH 23/28] TP2000-401: Add test for chunk_taric (#637) * Add test for chunk_taric * Remove placeholder test * Add docstring and assertion --- importer/tests/test_chunker.py | 22 +++++++++++++++++++++- importer/tests/test_files/goods.xml | 23 +++++++++++++++++++++++ 2 files changed, 44 insertions(+), 1 deletion(-) create mode 100644 importer/tests/test_files/goods.xml diff --git a/importer/tests/test_chunker.py b/importer/tests/test_chunker.py index bb2b50023..5fe0c2d06 100644 --- a/importer/tests/test_chunker.py +++ b/importer/tests/test_chunker.py @@ -1,17 +1,23 @@ import xml.etree.ElementTree as ET from io import BytesIO +from os import path from typing import Sequence from unittest import mock import pytest +from django.core.files.uploadedfile import SimpleUploadedFile from common.tests import factories from importer import chunker +from importer.chunker import chunk_taric from importer.chunker import filter_transaction_records +from importer.models import ImporterXMLChunk from importer.namespaces import TTags from .test_namespaces import get_snippet_transaction +TEST_FILES_PATH = path.join(path.dirname(__file__), "test_files") + pytestmark = pytest.mark.django_db @@ -40,7 +46,7 @@ def filter_snippet_transaction( @mock.patch("importer.chunker.TemporaryFile") def test_get_chunk(mock_temp_file: mock.MagicMock): - """Asserts that the correct chung is found or created for writing to.""" + """Asserts that the correct chunk is found or created for writing to.""" mock_temp_file.side_effect = BytesIO chunks_in_progress = {} @@ -115,3 +121,17 @@ def test_filter_transaction_records_negative( ) assert transaction is None + + +def test_chunk_taric(): + """Tests that the chunker creates an ImporterXMLChunk object in the db from + the loaded XML file.""" + assert not ImporterXMLChunk.objects.count() + with open(f"{TEST_FILES_PATH}/goods.xml", "rb") as f: + content = f.read() + taric_file = SimpleUploadedFile("goods.xml", content, content_type="text/xml") + batch = factories.ImportBatchFactory.create() + chunk_taric(taric_file, batch) + assert ImporterXMLChunk.objects.count() + chunk = ImporterXMLChunk.objects.first() + assert chunk.chunk_text diff --git a/importer/tests/test_files/goods.xml b/importer/tests/test_files/goods.xml new file mode 100644 index 000000000..40a0ea32f --- /dev/null +++ b/importer/tests/test_files/goods.xml @@ -0,0 +1,23 @@ + + + + + + + 19282658 + 400 + 00 + 2872 + 3 + + 108096 + 7019720081 + 80 + 2022-02-26 + 0 + + + + From 6026142f836c2081ab204d74ed322ec7f387e707 Mon Sep 17 00:00:00 2001 From: Paul Pepper <85895113+paulpepper-trade@users.noreply.github.com> Date: Thu, 4 Aug 2022 11:13:10 +0100 Subject: [PATCH 24/28] TP2000-247 Reverse related aggregated duties fix (#638) --- measures/forms.py | 10 ++-- measures/models.py | 13 +++++ measures/patterns.py | 1 - measures/querysets.py | 84 ++++++++++++++++++++----------- measures/tests/conftest.py | 28 ++++++++++- measures/tests/test_forms.py | 20 ++------ measures/tests/test_models.py | 5 +- measures/tests/test_querysets.py | 86 +++++++++++++++++++++++++++++--- measures/tests/test_views.py | 15 +++--- measures/views.py | 13 +++-- pii-ner-exclude.txt | 18 +++++++ pii-secret-exclude.txt | 3 +- workbaskets/management/util.py | 2 +- 13 files changed, 223 insertions(+), 75 deletions(-) diff --git a/measures/forms.py b/measures/forms.py index 88c94e998..915345307 100644 --- a/measures/forms.py +++ b/measures/forms.py @@ -509,11 +509,6 @@ def __init__(self, *args, **kwargs): tx = WorkBasket.get_current_transaction(self.request) - if not hasattr(self.instance, "duty_sentence"): - raise AttributeError( - "Measure instance is missing `duty_sentence` attribute. Try calling `with_duty_sentence` queryset method", - ) - self.initial["duty_sentence"] = self.instance.duty_sentence self.request.session[ f"instance_duty_sentence_{self.instance.sid}" @@ -636,7 +631,10 @@ def save(self, commit=True): WorkBasket.current(self.request), models.MeasureComponent, "component_measure", - # Creating components in the same transaction as the new version of the measure minimises number of transaction and groups the creation of measure and related objects in the same transaction. + # Creating components in the same transaction as the new version + # of the measure minimises number of transaction and groups the + # creation of measure and related objects in the same + # transaction. transaction=instance.transaction, ) diff --git a/measures/models.py b/measures/models.py index e5946c35c..a8e4863ae 100644 --- a/measures/models.py +++ b/measures/models.py @@ -16,6 +16,7 @@ from footnotes import validators as footnote_validators from measures import business_rules from measures import validators +from measures.querysets import ComponentQuerySet from measures.querysets import MeasureConditionQuerySet from measures.querysets import MeasuresQuerySet from quotas import business_rules as quotas_business_rules @@ -582,6 +583,10 @@ def effective_valid_between(self) -> TaricDateRange: return TaricDateRange(self.valid_between.lower, self.effective_end_date) + @property + def duty_sentence(self) -> str: + return MeasureComponent.objects.duty_sentence(self) + @classproperty def auto_value_fields(cls): """Remove export refund SID because we don't want to auto-increment it – @@ -657,6 +662,8 @@ class MeasureComponent(TrackedModel): UpdateValidity, ) + objects = TrackedModelManager.from_queryset(ComponentQuerySet)() + class MeasureCondition(TrackedModel): """ @@ -805,6 +812,10 @@ def condition_string(self) -> str: return "".join(out) + @property + def duty_sentence(self) -> str: + return MeasureConditionComponent.objects.duty_sentence(self) + class MeasureConditionComponent(TrackedModel): """Contains the duty information or part of the duty information of the @@ -860,6 +871,8 @@ class Meta: UpdateValidity, ) + objects = TrackedModelManager.from_queryset(ComponentQuerySet)() + class MeasureExcludedGeographicalArea(TrackedModel): """The measure excluded geographical area modifies the applicable diff --git a/measures/patterns.py b/measures/patterns.py index eda848a15..17d0aac7a 100644 --- a/measures/patterns.py +++ b/measures/patterns.py @@ -501,7 +501,6 @@ def get_measures(self, code: GoodsNomenclature, as_at: date): date.""" return ( Measure.objects.with_validity_field() - .with_duty_sentence() .approved_up_to_transaction(self.workbasket.transactions.last()) .as_at(as_at) .filter(goods_nomenclature__sid=code.sid) diff --git a/measures/querysets.py b/measures/querysets.py index 8a6dddeec..62c23fc34 100644 --- a/measures/querysets.py +++ b/measures/querysets.py @@ -1,10 +1,11 @@ +from typing import Union + from django.contrib.postgres.aggregates import StringAgg from django.db.models import Case from django.db.models import CharField from django.db.models import F from django.db.models import Func from django.db.models import Q -from django.db.models import QuerySet from django.db.models import Value from django.db.models import When from django.db.models.aggregates import Max @@ -16,6 +17,7 @@ from django.db.models.functions.text import Trim from django_cte.cte import With +import measures from common.fields import TaricDateRangeField from common.models.tracked_qs import TrackedModelQuerySet from common.querysets import ValidityQuerySet @@ -23,20 +25,28 @@ from common.util import StartDate -class DutySentenceMixin(QuerySet): - def with_duty_sentence(self) -> QuerySet: +class ComponentQuerySet(TrackedModelQuerySet): + """QuerySet that can be used with MeasureComponent or + MeasureConditionComponent.""" + + def duty_sentence( + self, + component_parent: Union["measures.Measure", "measures.MeasureCondition"], + ): """ - Annotates the query set with a human-readable string that represents the - aggregation of all of the linked components into a single duty sentence. + Returns the human-readable "duty sentence" for a Measure or + MeasureCondition instance as a string. The returned string value is a + (Postgresql) string aggregation of all the measure's "current" + components. This operation relies on the `prefix` and `abbreviation` fields being filled in on duty expressions and units, which are not supplied by the TARIC3 XML by default. - Strings output by this annotation should be valid input to the + Strings output by this aggregation should be valid input to the :class:`~measures.parsers.DutySentenceParser`. - The annotated field will be generated using the below SQL: + The string aggregation will be generated using the below SQL: .. code:: SQL @@ -81,70 +91,85 @@ def with_duty_sentence(self) -> QuerySet: ), ) AS "duty_sentence" """ - return self.annotate( + + # Components with the greatest transaction_id that is less than + # or equal to component_parent's transaction_id, are considered 'current'. + component_qs = component_parent.components.approved_up_to_transaction( + component_parent.transaction, + ) + if not component_qs: + return "" + latest_transaction_id = component_qs.aggregate( + latest_transaction_id=Max( + "transaction_id", + ), + ).get("latest_transaction_id") + component_qs = component_qs.filter(transaction_id=latest_transaction_id) + + # Aggregate all the current Components for component_parent to form its + # duty sentence. + duty_sentence = component_qs.aggregate( duty_sentence=StringAgg( expression=Trim( Concat( Case( When( - Q(components__duty_expression__prefix__isnull=True) - | Q(components__duty_expression__prefix=""), + Q(duty_expression__prefix__isnull=True) + | Q(duty_expression__prefix=""), then=Value(""), ), default=Concat( - F("components__duty_expression__prefix"), + F("duty_expression__prefix"), Value(" "), ), ), - "components__duty_amount", + "duty_amount", Case( When( - components__monetary_unit=None, - components__duty_amount__isnull=False, + monetary_unit=None, + duty_amount__isnull=False, then=Value("%"), ), When( - components__duty_amount__isnull=True, + duty_amount__isnull=True, then=Value(""), ), default=Concat( Value(" "), - F("components__monetary_unit__code"), + F("monetary_unit__code"), ), ), Case( When( - Q(components__component_measurement=None) - | Q( - components__component_measurement__measurement_unit=None, - ) + Q(component_measurement=None) + | Q(component_measurement__measurement_unit=None) | Q( - components__component_measurement__measurement_unit__abbreviation=None, + component_measurement__measurement_unit__abbreviation=None, ), then=Value(""), ), When( - components__monetary_unit__isnull=True, + monetary_unit__isnull=True, then=F( - "components__component_measurement__measurement_unit__abbreviation", + "component_measurement__measurement_unit__abbreviation", ), ), default=Concat( Value(" / "), F( - "components__component_measurement__measurement_unit__abbreviation", + "component_measurement__measurement_unit__abbreviation", ), ), ), Case( When( - components__component_measurement__measurement_unit_qualifier__abbreviation=None, + component_measurement__measurement_unit_qualifier__abbreviation=None, then=Value(""), ), default=Concat( Value(" / "), F( - "components__component_measurement__measurement_unit_qualifier__abbreviation", + "component_measurement__measurement_unit_qualifier__abbreviation", ), ), ), @@ -152,12 +177,13 @@ def with_duty_sentence(self) -> QuerySet: ), ), delimiter=" ", - ordering="components__duty_expression__sid", + ordering="duty_expression__sid", ), ) + return duty_sentence.get("duty_sentence", "") -class MeasuresQuerySet(TrackedModelQuerySet, DutySentenceMixin, ValidityQuerySet): +class MeasuresQuerySet(TrackedModelQuerySet, ValidityQuerySet): def with_validity_field(self): return self.with_effective_valid_between() @@ -243,7 +269,7 @@ def with_effective_valid_between(self): ) -class MeasureConditionQuerySet(TrackedModelQuerySet, DutySentenceMixin): +class MeasureConditionQuerySet(TrackedModelQuerySet): def with_reference_price_string(self): """ Returns a MeasureCondition queryset annotated with diff --git a/measures/tests/conftest.py b/measures/tests/conftest.py index bb6121ad6..91c76493f 100644 --- a/measures/tests/conftest.py +++ b/measures/tests/conftest.py @@ -333,6 +333,32 @@ def irreversible_duty_sentence_data(request, get_component_data): return expected, [get_component_data(*args) for args in component_data] +@pytest.fixture( + params=( + ( + ( + "0.000% + AC", + [(1, 0.0, None, None), (12, None, None, None)], + ), + ( + "12.900% + 20.000 EUR / kg", + [(1, 12.9, None, None), (4, 20.0, "EUR", ("KGM", None))], + ), + ), + ), +) +def duty_sentence_x_2_data(request, get_component_data): + """Duty sentence test cases that can be used to create a history of + components.""" + history = [] + for version in request.param: + expected, component_data = version + history.append( + (expected, [get_component_data(*args) for args in component_data]), + ) + return history + + @pytest.fixture def erga_omnes(): return factories.GeographicalAreaFactory.create( @@ -385,7 +411,7 @@ def measure_form(measure_form_data, session_with_workbasket, erga_omnes): set_current_transaction(Transaction.objects.last()) return MeasureForm( data=measure_form_data, - instance=Measure.objects.with_duty_sentence().first(), + instance=Measure.objects.first(), request=session_with_workbasket, initial={}, ) diff --git a/measures/tests/test_forms.py b/measures/tests/test_forms.py index 0706d1117..704f15a7a 100644 --- a/measures/tests/test_forms.py +++ b/measures/tests/test_forms.py @@ -36,16 +36,6 @@ def test_diff_components_called(diff_components, measure_form, duty_sentence_par assert diff_components.called == True -def test_error_raised_if_no_duty_sentence(session_with_workbasket): - measure = factories.MeasureFactory.create() - - with pytest.raises( - AttributeError, - match="Measure instance is missing `duty_sentence` attribute. Try calling `with_duty_sentence` queryset method", - ): - MeasureForm(data={}, instance=measure, request=session_with_workbasket) - - def test_measure_form_invalid_conditions_data( measure_form_data, session_with_workbasket, @@ -63,7 +53,7 @@ def test_measure_form_invalid_conditions_data( measure_form = MeasureForm( data=measure_form_data, initial={}, - instance=Measure.objects.with_duty_sentence().first(), + instance=Measure.objects.first(), request=session_with_workbasket, ) @@ -704,7 +694,7 @@ def test_measure_form_valid_data(erga_omnes, session_with_workbasket): form = forms.MeasureForm( data=data, initial={}, - instance=Measure.objects.with_duty_sentence().first(), + instance=Measure.objects.first(), request=session_with_workbasket, ) assert form.is_valid() @@ -737,7 +727,7 @@ def test_measure_form_initial_data_geo_area( form = forms.MeasureForm( data=data, initial={}, - instance=Measure.objects.with_duty_sentence().first(), + instance=Measure.objects.first(), request=session_with_workbasket, ) assert form.initial["geo_area"] == geo_area_to_choice[measure.geographical_area] @@ -769,7 +759,7 @@ def test_measure_form_cleaned_data_geo_exclusions_group( form = forms.MeasureForm( data=data, initial={}, - instance=Measure.objects.with_duty_sentence().first(), + instance=Measure.objects.first(), request=session_with_workbasket, ) assert form.is_valid() @@ -806,7 +796,7 @@ def test_measure_form_cleaned_data_geo_exclusions_erga_omnes( form = forms.MeasureForm( data=data, initial={}, - instance=Measure.objects.with_duty_sentence().first(), + instance=Measure.objects.first(), request=session_with_workbasket, ) assert form.is_valid() diff --git a/measures/tests/test_models.py b/measures/tests/test_models.py index a26e311d2..055efa0e5 100644 --- a/measures/tests/test_models.py +++ b/measures/tests/test_models.py @@ -29,13 +29,13 @@ def test_measure_conditions_list(): duty_expression__prefix="", duty_amount=Decimal("2.5"), monetary_unit=None, + transaction=cond.transaction, ) cond = ( type(cond) .objects.latest_approved() .with_reference_price_string() - .with_duty_sentence() .get(pk=cond.pk) ) assert cond.reference_price_string == "48.100 EUR / 100 kg" @@ -62,6 +62,7 @@ def test_stringify_measure_condition(): duty_expression__prefix="", duty_amount=Decimal("2.5"), monetary_unit=None, + transaction=cond.transaction, ) factories.MeasureConditionComponentFactory.create( condition=cond, @@ -74,13 +75,13 @@ def test_stringify_measure_condition(): measurement_unit__abbreviation="100 kg", measurement_unit_qualifier=None, ), + transaction=cond.transaction, ) cond = ( type(cond) .objects.latest_approved() .with_reference_price_string() - .with_duty_sentence() .get(pk=cond.pk) ) assert cond.reference_price_string == "0.000 EUR / 100 kg" diff --git a/measures/tests/test_querysets.py b/measures/tests/test_querysets.py index 755b0f234..c8dd0fbdf 100644 --- a/measures/tests/test_querysets.py +++ b/measures/tests/test_querysets.py @@ -17,6 +17,23 @@ pytestmark = pytest.mark.django_db +def create_duty_components( + component_factory, + component_data, + field_name, + model, +): + components = [] + for kwargs in component_data: + component = component_factory( + **{field_name: model}, + transaction=model.transaction, + **kwargs, + ) + components.append(component) + return components + + @pytest.mark.parametrize( "component_factory,model_factory,field", ( @@ -66,13 +83,70 @@ def test_duty_sentence_generation( expected, component_data = reversible_duty_sentence_data model = model_factory() - for kwargs in reorder(component_data): - component_factory( - **{field: model}, - **kwargs, - ) + create_duty_components(component_factory, component_data, field, model) + + test_instance = model_factory._meta.model.objects.get() + assert test_instance.duty_sentence == expected + + +@pytest.mark.parametrize( + "component_factory,model_factory,field_name", + ( + ( + factories.MeasureComponentFactory, + factories.MeasureFactory, + "component_measure", + ), + ( + factories.MeasureConditionComponentFactory, + factories.MeasureConditionFactory, + "condition", + ), + ), + ids=( + factories.MeasureFactory._meta.model.__name__, + factories.MeasureConditionFactory._meta.model.__name__, + ), +) +def test_duty_sentence_with_history( + component_factory: factory.django.DjangoModelFactory, + model_factory: factory.django.DjangoModelFactory, + field_name: str, + duty_sentence_x_2_data: List[Tuple[str, List[Dict]]], +): + """ + Sets up a history of model instances (Measure or MeasureCondition) and + linked components. + + The test then checks that the correct duty sentence is available via each + object's duty_sentence property throughout the object's history, including + going back in transaction time. + """ + model_qs = model_factory._meta.model.objects.all() + + # Create model instance and associate it with duty components. + model_1 = model_factory() + expected, component_data = duty_sentence_x_2_data[0] + create_duty_components(component_factory, component_data, field_name, model_1) + test_instance = model_qs.get_latest_version(sid=model_1.sid) + assert test_instance.duty_sentence == expected + + # Create a new version of the model and associate it with new duty components. + model_2 = model_1.new_version(model_1.transaction.workbasket) + expected, component_data = duty_sentence_x_2_data[1] + create_duty_components(component_factory, component_data, field_name, model_2) + test_instance = model_qs.get_latest_version(sid=model_2.sid) + assert test_instance.duty_sentence == expected + + # Create a new version of the model but don't update duty components. + model_3 = model_2.new_version(model_2.transaction.workbasket) + test_instance = model_qs.get_latest_version(sid=model_3.sid) + assert test_instance.duty_sentence == expected - test_instance = model_factory._meta.model.objects.with_duty_sentence().get() + # Go back in time to the model's fist version to check its duty_sentence is + # still correctly constructed and retrieved. + expected, component_data = duty_sentence_x_2_data[0] + test_instance = model_qs.get_first_version(sid=model_1.sid) assert test_instance.duty_sentence == expected diff --git a/measures/tests/test_views.py b/measures/tests/test_views.py index e58a0977f..f6d243e94 100644 --- a/measures/tests/test_views.py +++ b/measures/tests/test_views.py @@ -425,6 +425,9 @@ def test_measure_update_updates_footnote_association(measure_form, client, valid assert new_assoc.version_group == assoc.version_group +@pytest.mark.skip( + reason="Temporary skip - indeterminate components ordering => invalid assertions. See TP2000-452", +) def test_measure_update_create_conditions( client, valid_user, @@ -440,7 +443,7 @@ def test_measure_update_create_conditions( Also tests that related objects (certificate and condition code) are present. """ - measure = Measure.objects.with_duty_sentence().first() + measure = Measure.objects.first() url = reverse("measure-ui-edit", args=(measure.sid,)) client.force_login(valid_user) client.post(url, data=measure_edit_conditions_data) @@ -469,7 +472,7 @@ def test_measure_update_create_conditions( ) assert condition.update_type == UpdateType.CREATE - components = condition.components.approved_up_to_transaction(tx).all() + components = condition.components.approved_up_to_transaction(tx) assert components.count() == 2 assert components.first().duty_amount == 3.5 @@ -491,7 +494,7 @@ def test_measure_update_edit_conditions( Checks that previous conditions are removed and new field values are correct. """ - measure = Measure.objects.with_duty_sentence().first() + measure = Measure.objects.first() url = reverse("measure-ui-edit", args=(measure.sid,)) client.force_login(valid_user) client.post(url, data=measure_edit_conditions_data) @@ -547,7 +550,7 @@ def test_measure_update_edit_conditions( # duty_sentence_parser, # erga_omnes, # ): -# measure = Measure.objects.with_duty_sentence().first() +# measure = Measure.objects.first() # url = reverse("measure-ui-edit", args=(measure.sid,)) # client.force_login(valid_user) /PS-IGNORE # client.post(url, data=measure_edit_conditions_data) /PS-IGNORE @@ -583,7 +586,7 @@ def test_measure_update_remove_conditions( endpoint and that the updated measure has no currently approved conditions associated with it. """ - measure = Measure.objects.with_duty_sentence().first() + measure = Measure.objects.first() url = reverse("measure-ui-edit", args=(measure.sid,)) client.force_login(valid_user) client.post(url, data=measure_edit_conditions_data) @@ -632,7 +635,7 @@ def test_measure_update_invalid_conditions( measure_edit_conditions_data[ "measure-conditions-formset-0-applicable_duty" ] = "invalid" - measure = Measure.objects.with_duty_sentence().first() + measure = Measure.objects.first() url = reverse("measure-ui-edit", args=(measure.sid,)) client.force_login(valid_user) response = client.post(url, data=measure_edit_conditions_data) diff --git a/measures/views.py b/measures/views.py index 5ad495cbe..0ed5a66dd 100644 --- a/measures/views.py +++ b/measures/views.py @@ -56,7 +56,7 @@ class MeasureMixin: def get_queryset(self): tx = WorkBasket.get_current_transaction(self.request) - return Measure.objects.with_duty_sentence().approved_up_to_transaction(tx) + return Measure.objects.approved_up_to_transaction(tx) class MeasureList(MeasureMixin, TamatoListView): @@ -70,12 +70,11 @@ class MeasureList(MeasureMixin, TamatoListView): class MeasureDetail(MeasureMixin, TrackedModelDetailView): model = Measure template_name = "measures/detail.jinja" - queryset = Measure.objects.with_duty_sentence().latest_approved() + queryset = Measure.objects.latest_approved() def get_context_data(self, **kwargs: Any): conditions = ( self.object.conditions.current() - .with_duty_sentence() .prefetch_related( "condition_code", "required_certificate", @@ -313,7 +312,7 @@ class MeasureUpdate( form_class = forms.MeasureForm permission_required = "common.change_trackedmodel" template_name = "measures/edit.jinja" - queryset = Measure.objects.with_duty_sentence() + queryset = Measure.objects.all() def get_form_kwargs(self): kwargs = super().get_form_kwargs() @@ -344,9 +343,9 @@ def get_footnotes(self, measure): def get_conditions(self, measure): tx = WorkBasket.get_current_transaction(self.request) return ( - measure.conditions.with_duty_sentence() - .with_reference_price_string() - .approved_up_to_transaction(tx) + measure.conditions.with_reference_price_string().approved_up_to_transaction( + tx, + ) ) def get_context_data(self, **kwargs): diff --git a/pii-ner-exclude.txt b/pii-ner-exclude.txt index bc57d570b..a331833d8 100644 --- a/pii-ner-exclude.txt +++ b/pii-ner-exclude.txt @@ -1148,3 +1148,21 @@ self.linked_model WorkBasketOutputFormat param kwargs: Enum +7930d36f +*** measure_qs = < +CharField\n +OuterRef\n +django.db.models.functions.comparison +Cast\n +django.db.models.functions.text +measure_sid = 20185730\n +measure_qs +measure_qs}\")\n +kernelspec +*** measure_qs.count +*** components_qs +components_qs}\")\n +measure_qs)\n +MeasureComponent +Components +TP2000-452 diff --git a/pii-secret-exclude.txt b/pii-secret-exclude.txt index f9cadd356..fb943460c 100644 --- a/pii-secret-exclude.txt +++ b/pii-secret-exclude.txt @@ -4,9 +4,10 @@ Dockerfile docs/requirements.txt docs/source/CODE_OF_CONDUCT.rst package-lock.json +Postcode pyproject.toml README.rst requirements.txt requirements-dev.txt runtime.txt -sample.env \ No newline at end of file +sample.env diff --git a/workbaskets/management/util.py b/workbaskets/management/util.py index 3c6be99c3..88b938c39 100644 --- a/workbaskets/management/util.py +++ b/workbaskets/management/util.py @@ -39,7 +39,7 @@ def _output_workbasket_compact(self, workbasket, show_transaction_info, **kwargs def output_workbasket( self, workbasket, - show_transaction_info, + show_transaction_info=False, output_format=WorkBasketOutputFormat.READABLE, **kwargs, ): From e8641fb713f903c47301fe4f23bb850170f68efb Mon Sep 17 00:00:00 2001 From: Paul Pepper <85895113+paulpepper-trade@users.noreply.github.com> Date: Thu, 4 Aug 2022 14:54:36 +0100 Subject: [PATCH 25/28] Correctly order queryset of MeasureConditionComponents. (#642) --- measures/tests/test_views.py | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/measures/tests/test_views.py b/measures/tests/test_views.py index f6d243e94..5ba5a2fc6 100644 --- a/measures/tests/test_views.py +++ b/measures/tests/test_views.py @@ -24,6 +24,7 @@ from measures.models import FootnoteAssociationMeasure from measures.models import Measure from measures.models import MeasureCondition +from measures.models import MeasureConditionComponent from measures.models import MeasureExcludedGeographicalArea from measures.validators import validate_duties from measures.views import MeasureCreateWizard @@ -425,9 +426,6 @@ def test_measure_update_updates_footnote_association(measure_form, client, valid assert new_assoc.version_group == assoc.version_group -@pytest.mark.skip( - reason="Temporary skip - indeterminate components ordering => invalid assertions. See TP2000-452", -) def test_measure_update_create_conditions( client, valid_user, @@ -472,7 +470,9 @@ def test_measure_update_create_conditions( ) assert condition.update_type == UpdateType.CREATE - components = condition.components.approved_up_to_transaction(tx) + components = condition.components.approved_up_to_transaction(tx).order_by( + *MeasureConditionComponent._meta.ordering + ) assert components.count() == 2 assert components.first().duty_amount == 3.5 From 9e96ebd4ab0c42e90f1c5db7872b5117ef079edc Mon Sep 17 00:00:00 2001 From: Gabe Naughton Date: Thu, 4 Aug 2022 15:56:10 +0100 Subject: [PATCH 26/28] TP2000-311 Envelope schema expects a six-digit number to be present, the TARIC file only contains five (#641) * update envelope xml schema file to expect 5 digit id * update docstring in importer and actually commit envelope change * create separate commodity assets * use with --- commodities/assets/commodities_envelope.xsd | 114 ++ commodities/assets/commodities_taric3.xsd | 1803 +++++++++++++++++++ commodities/forms.py | 2 +- commodities/tests/test_files/broken.xml | 2 +- commodities/tests/test_files/invalid.xml | 2 +- commodities/tests/test_files/invalid_id.xml | 24 + commodities/tests/test_files/valid.xml | 2 +- commodities/tests/test_forms.py | 39 + importer/views.py | 2 +- pii-ner-exclude.txt | 1 + settings/common.py | 9 + 11 files changed, 1995 insertions(+), 5 deletions(-) create mode 100644 commodities/assets/commodities_envelope.xsd create mode 100644 commodities/assets/commodities_taric3.xsd create mode 100644 commodities/tests/test_files/invalid_id.xml create mode 100644 commodities/tests/test_forms.py diff --git a/commodities/assets/commodities_envelope.xsd b/commodities/assets/commodities_envelope.xsd new file mode 100644 index 000000000..fb2e6b352 --- /dev/null +++ b/commodities/assets/commodities_envelope.xsd @@ -0,0 +1,114 @@ + + + + + + Message envelope + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + diff --git a/commodities/assets/commodities_taric3.xsd b/commodities/assets/commodities_taric3.xsd new file mode 100644 index 000000000..f8630f89f --- /dev/null +++ b/commodities/assets/commodities_taric3.xsd @@ -0,0 +1,1803 @@ + + + + + + Get access to the xml: attribute groups for xml:lang as declared on + 'schema' and 'documentation' + below + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + 1-Update, 2-Delete, 3-Insert + + + + + Update + + + + + Delete + + + + + Insert + + + + + + + + + Contains a transmission + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + Date format + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + AAA + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + Timestamp format + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + diff --git a/commodities/forms.py b/commodities/forms.py index 5f1b626ff..01c84ec41 100644 --- a/commodities/forms.py +++ b/commodities/forms.py @@ -66,7 +66,7 @@ def clean_taric_file(self): capture_exception(e) raise ValidationError(generic_error_message) - with open(settings.PATH_XSD_TARIC) as xsd_file: + with open(settings.PATH_XSD_COMMODITIES_TARIC) as xsd_file: xmlschema = lxml.etree.XMLSchema(file=xsd_file) try: diff --git a/commodities/tests/test_files/broken.xml b/commodities/tests/test_files/broken.xml index 01bdef2cc..804a92202 100644 --- a/commodities/tests/test_files/broken.xml +++ b/commodities/tests/test_files/broken.xml @@ -1,5 +1,5 @@ - + diff --git a/commodities/tests/test_files/invalid.xml b/commodities/tests/test_files/invalid.xml index 0cad35746..8c0c12cf9 100644 --- a/commodities/tests/test_files/invalid.xml +++ b/commodities/tests/test_files/invalid.xml @@ -1,5 +1,5 @@ - + diff --git a/commodities/tests/test_files/invalid_id.xml b/commodities/tests/test_files/invalid_id.xml new file mode 100644 index 000000000..56c82c67c --- /dev/null +++ b/commodities/tests/test_files/invalid_id.xml @@ -0,0 +1,24 @@ + + + + + + + 392602 + 400 + 00 + 1 + 1 + + 100460 + 0709999030 + 80 + 2016-09-15 + 2021-12-31 + 0 + + + + + + diff --git a/commodities/tests/test_files/valid.xml b/commodities/tests/test_files/valid.xml index 56c82c67c..8d4393761 100644 --- a/commodities/tests/test_files/valid.xml +++ b/commodities/tests/test_files/valid.xml @@ -1,5 +1,5 @@ - + diff --git a/commodities/tests/test_forms.py b/commodities/tests/test_forms.py new file mode 100644 index 000000000..6afafd0a5 --- /dev/null +++ b/commodities/tests/test_forms.py @@ -0,0 +1,39 @@ +from os import path + +from django.core.files.uploadedfile import SimpleUploadedFile + +from commodities import forms + +TEST_FILES_PATH = path.join(path.dirname(__file__), "test_files") + + +def test_import_form_valid_envelope_id(): + with open(f"{TEST_FILES_PATH}/valid.xml", "rb") as upload_file: + file_data = { + "taric_file": SimpleUploadedFile( + upload_file.name, + upload_file.read(), + content_type="text/xml", + ), + } + form = forms.CommodityImportForm({}, file_data) + + assert form.is_valid() + + +def test_import_form_invalid_envelope_id(): + with open(f"{TEST_FILES_PATH}/invalid_id.xml", "rb") as upload_file: + file_data = { + "taric_file": SimpleUploadedFile( + upload_file.name, + upload_file.read(), + content_type="text/xml", + ), + } + form = forms.CommodityImportForm({}, file_data) + + assert not form.is_valid() + assert ( + "The selected file could not be uploaded - try again" + in form.errors["taric_file"] + ) diff --git a/importer/views.py b/importer/views.py index 4af97cfdd..c047745c8 100644 --- a/importer/views.py +++ b/importer/views.py @@ -11,7 +11,7 @@ class ImportBatchList(RequiresSuperuserMixin, WithPaginationListView): - """UI endpoint for viewing and filtering Additional Codes.""" + """UI endpoint for viewing and filtering Import Batches.""" queryset = ( models.ImportBatch.objects.all() diff --git a/pii-ner-exclude.txt b/pii-ner-exclude.txt index a331833d8..ff3b62fc3 100644 --- a/pii-ner-exclude.txt +++ b/pii-ner-exclude.txt @@ -1148,6 +1148,7 @@ self.linked_model WorkBasketOutputFormat param kwargs: Enum +schemaLocation="commodities_envelope.xsd"/ 7930d36f *** measure_qs = < CharField\n diff --git a/settings/common.py b/settings/common.py index d530cc188..506a33cab 100644 --- a/settings/common.py +++ b/settings/common.py @@ -479,6 +479,15 @@ PATH_XSD_ENVELOPE = Path(PATH_ASSETS, "envelope.xsd") PATH_XSD_TARIC = Path(PATH_ASSETS, "taric3.xsd") +PATH_COMMODITIES_ASSETS = Path(BASE_DIR, "commodities", "assets") + +PATH_XSD_COMMODITIES_ENVELOPE = Path( + PATH_COMMODITIES_ASSETS, + "commodities_envelope.xsd", +) +PATH_XSD_COMMODITIES_TARIC = Path(PATH_COMMODITIES_ASSETS, "commodities_taric3.xsd") + + # Default username for envelope data imports DATA_IMPORT_USERNAME = os.environ.get("TAMATO_IMPORT_USERNAME", "test") From ca229fc1f37c4b3d54b9d17f51a514d6697c6025 Mon Sep 17 00:00:00 2001 From: Gabe Naughton Date: Fri, 5 Aug 2022 10:34:07 +0100 Subject: [PATCH 27/28] update get_initial to use current (#643) --- certificates/tests/test_views.py | 22 ++++++++++++++++++++++ certificates/views.py | 2 +- 2 files changed, 23 insertions(+), 1 deletion(-) diff --git a/certificates/tests/test_views.py b/certificates/tests/test_views.py index ca6ec06f7..c784e45ac 100644 --- a/certificates/tests/test_views.py +++ b/certificates/tests/test_views.py @@ -4,7 +4,9 @@ from django.urls import reverse from certificates import models +from certificates.views import CertificateDescriptionCreate from certificates.views import CertificateList +from common.models.utils import override_current_transaction from common.tests import factories from common.tests.util import assert_model_view_renders from common.tests.util import get_class_based_view_urls_matching_url @@ -13,6 +15,8 @@ from common.views import TamatoListView from common.views import TrackedModelDetailMixin +pytestmark = pytest.mark.django_db + @pytest.mark.parametrize( "factory", @@ -86,3 +90,21 @@ def test_certificate_list_view(view, url_pattern, valid_user_client): """Verify that certificate list view is under the url certificates/ and doesn't return an error.""" assert_model_view_renders(view, url_pattern, valid_user_client) + + +# https://uktrade.atlassian.net/browse/TP2000-450 /PS-IGNORE +def test_description_create_get_initial(): + """Test that, where more than one version of a certificate exists, + get_initial returns only the current version.""" + certificate = factories.CertificateFactory.create() + new_version = certificate.new_version(certificate.transaction.workbasket) + view = CertificateDescriptionCreate( + kwargs={ + "certificate_type__sid": certificate.certificate_type.sid, + "sid": certificate.sid, + }, + ) + with override_current_transaction(new_version.transaction): + initial = view.get_initial() + + assert initial["described_certificate"] == new_version diff --git a/certificates/views.py b/certificates/views.py index dfe47f0d9..5d3f212e9 100644 --- a/certificates/views.py +++ b/certificates/views.py @@ -157,7 +157,7 @@ class CertificateDescriptionCreate( ): def get_initial(self): initial = super().get_initial() - initial["described_certificate"] = models.Certificate.objects.get( + initial["described_certificate"] = models.Certificate.objects.current().get( certificate_type__sid=(self.kwargs.get("certificate_type__sid")), sid=(self.kwargs.get("sid")), ) From 2b52aac3e307ae6b0a4f46f773a147f2d1fa57da Mon Sep 17 00:00:00 2001 From: Gabe Naughton Date: Fri, 5 Aug 2022 15:55:12 +0100 Subject: [PATCH 28/28] TP2000-450 certificate multiple objects returned error on certificate description create (pt. 2) (#644) * update get_initial to use current * add form and view tests, update get_context_data to use current queryset --- certificates/tests/test_forms.py | 23 +++++++++++++++++++++++ certificates/tests/test_views.py | 26 ++++++++++++++++++++++++++ certificates/views.py | 2 +- 3 files changed, 50 insertions(+), 1 deletion(-) diff --git a/certificates/tests/test_forms.py b/certificates/tests/test_forms.py index 015209593..89ff7c4c7 100644 --- a/certificates/tests/test_forms.py +++ b/certificates/tests/test_forms.py @@ -178,3 +178,26 @@ def test_validation_error_raised_for_duplicate_sid(session_with_workbasket): f"Certificate with sid A01 and type {certificate_type} already exists." in form.errors["sid"] ) + + +def test_certificate_description_valid_data(): + certificate = factories.CertificateFactory.create() + data = { + "described_certificate": certificate.pk, + "description": "certifiably certified", + "validity_start_0": 1, + "validity_start_1": 1, + "validity_start_2": 2022, + } + form = forms.CertificateCreateDescriptionForm(data=data) + + assert form.is_valid() + + +def test_certificate_description_invalid_data(): + form = forms.CertificateCreateDescriptionForm(data={}) + + assert not form.is_valid() + assert "This field is required." in form.errors["described_certificate"] + assert "This field is required." in form.errors["description"] + assert "Enter the day, month and year" in form.errors["validity_start"] diff --git a/certificates/tests/test_views.py b/certificates/tests/test_views.py index c784e45ac..cd941f5a1 100644 --- a/certificates/tests/test_views.py +++ b/certificates/tests/test_views.py @@ -108,3 +108,29 @@ def test_description_create_get_initial(): initial = view.get_initial() assert initial["described_certificate"] == new_version + + +def test_description_create_get_context_data(valid_user_api_client): + """Test that posting to certificate create endpoint with valid data returns + a 302 and creates new description matching certificate.""" + certificate = factories.CertificateFactory.create(description=None) + new_version = certificate.new_version(certificate.transaction.workbasket) + url = reverse( + "certificate-ui-description-create", + args=(certificate.certificate_type.sid, certificate.sid), + ) + post_data = { + "description": "certifiably certified", + "described_certificate": new_version.pk, + "validity_start_0": 1, + "validity_start_1": 1, + "validity_start_2": 2022, + } + assert not models.CertificateDescription.objects.exists() + response = valid_user_api_client.post(url, post_data) + + assert response.status_code == 302 + assert models.CertificateDescription.objects.filter( + described_certificate__sid=new_version.sid, + described_certificate__certificate_type__sid=new_version.certificate_type.sid, + ).exists() diff --git a/certificates/views.py b/certificates/views.py index 5d3f212e9..57d866bc7 100644 --- a/certificates/views.py +++ b/certificates/views.py @@ -143,7 +143,7 @@ class CertificateCreateDescriptionMixin: def get_context_data(self, **kwargs): context = super().get_context_data(**kwargs) - context["described_object"] = models.Certificate.objects.get( + context["described_object"] = models.Certificate.objects.current().get( certificate_type__sid=(self.kwargs.get("certificate_type__sid")), sid=(self.kwargs.get("sid")), )