diff --git a/README.rst b/README.rst index d1744d782b..e81f7999f9 100644 --- a/README.rst +++ b/README.rst @@ -221,6 +221,14 @@ Open another terminal and start a Celery worker: celery -A common.celery worker --loglevel=info +To monitor celery workers or individual tasks run: + +.. code:: sh + + celery flower + +See `flower docs `_ for more details + Manually trigger the upload to s3 ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ diff --git a/commodities/assets/commodities_taric3.xsd b/commodities/assets/commodities_taric3.xsd index f8630f89f6..951c6c273d 100644 --- a/commodities/assets/commodities_taric3.xsd +++ b/commodities/assets/commodities_taric3.xsd @@ -577,7 +577,7 @@ - + diff --git a/commodities/forms.py b/commodities/forms.py index 1f0bfbf23b..f5ed7cf1ab 100644 --- a/commodities/forms.py +++ b/commodities/forms.py @@ -8,6 +8,7 @@ from crispy_forms_gds.layout import Size from crispy_forms_gds.layout import Submit from django import forms +from django.conf import settings from django.contrib.auth.models import User from django.db import transaction @@ -35,6 +36,7 @@ class CommodityImportForm(ImportForm): help_text="", label="Select an XML file", ) + xsd_file = settings.PATH_XSD_COMMODITIES_TARIC def __init__(self, *args, **kwargs): super().__init__(*args, **kwargs) diff --git a/commodities/models/orm.py b/commodities/models/orm.py index 3bb43a5b65..833f7f9bb6 100644 --- a/commodities/models/orm.py +++ b/commodities/models/orm.py @@ -91,6 +91,7 @@ def footnote_application_codes(self) -> Set[ApplicationCode]: business_rules = ( business_rules.NIG1, business_rules.NIG5, + business_rules.NIG11, business_rules.NIG12, business_rules.NIG30, business_rules.NIG31, diff --git a/commodities/tests/test_files/quota_definition.xml b/commodities/tests/test_files/quota_definition.xml new file mode 100644 index 0000000000..fc3a387142 --- /dev/null +++ b/commodities/tests/test_files/quota_definition.xml @@ -0,0 +1,32 @@ + + + + + + + 19802301 + 370 + 00 + 450 + 3 + + 21830 + 091528 + 2023-01-01 + 2023-12-31 + 2554 + 2550000 + 2550000 + LTR + 3 + N + 90 + Commission Implementation Regulation (EU) No 343/2011 - Opening and providing for the administration of Union tariff quotas for wines originating in Bosnia and Herzegovina + L 96/12 of 9/04/2011 + Increase of volumes with COMMISSION IMPLEMENTING REGULATION (EU) 2017/822 of 15 May 2017 amending Implementing Regulation (EU) No 343/2011 opening and providing for the administration of Union tariff quotas for wines originating in Bosnia and Herzegovina (see OJ L 123/1) + + + + + + diff --git a/commodities/tests/test_forms.py b/commodities/tests/test_forms.py index 02b73c4301..70d8de3cec 100644 --- a/commodities/tests/test_forms.py +++ b/commodities/tests/test_forms.py @@ -62,3 +62,20 @@ def test_import_form_non_xml_file(): assert not form.is_valid() assert "The selected file must be XML" in form.errors["taric_file"] + + +# https://uktrade.atlassian.net/browse/TP2000-571 +def test_import_form_long_definition_description(): + """Tests that form is valid when provided with QuotaDefinition description + longer than 500 characters.""" + with open(f"{TEST_FILES_PATH}/quota_definition.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() diff --git a/commodities/tests/test_views.py b/commodities/tests/test_views.py index b57f51ce0c..c300ecaa95 100644 --- a/commodities/tests/test_views.py +++ b/commodities/tests/test_views.py @@ -1,3 +1,4 @@ +import json from os import path from unittest.mock import patch @@ -145,3 +146,26 @@ def test_commodities_detail_views( don't return an error.""" assert_model_view_renders(view, url_pattern, valid_user_client) + + +def test_goods_nomenclature(valid_user_client, date_ranges): + past_good = factories.GoodsNomenclatureFactory.create( + valid_between=date_ranges.earlier, + ) + present_good = factories.GoodsNomenclatureFactory.create( + valid_between=date_ranges.normal, + ) + future_good = factories.GoodsNomenclatureFactory.create( + valid_between=date_ranges.later, + ) + url = reverse("goodsnomenclature-list") + response = valid_user_client.get(url) + + assert response.status_code == 200 + + goods = json.loads(response.content)["results"] + pks = [good["value"] for good in goods] + + assert past_good.pk not in pks + assert present_good.pk in pks + assert future_good.pk in pks diff --git a/commodities/views.py b/commodities/views.py index 56f1688089..24e85fce15 100644 --- a/commodities/views.py +++ b/commodities/views.py @@ -38,7 +38,7 @@ def get_queryset(self): tx, ) .prefetch_related("descriptions") - .as_at(date.today()) + .as_at_and_beyond(date.today()) .filter(suffix=80) ) diff --git a/common/assets/envelope.xsd b/common/assets/envelope.xsd index 4b7d51969a..3d670ad53a 100644 --- a/common/assets/envelope.xsd +++ b/common/assets/envelope.xsd @@ -41,7 +41,7 @@ - + diff --git a/common/assets/taric3.xsd b/common/assets/taric3.xsd index 51b09bc545..620e698487 100644 --- a/common/assets/taric3.xsd +++ b/common/assets/taric3.xsd @@ -577,7 +577,7 @@ - + diff --git a/common/business_rules.py b/common/business_rules.py index 0e145b32f3..30b073f5ef 100644 --- a/common/business_rules.py +++ b/common/business_rules.py @@ -457,6 +457,20 @@ def validate(self, model): raise self.violation(model) +class MustExistNotNull(BusinessRule): + """Rule enforcing a referenced record exists, Null values raise + violation.""" + + reference_field_name: Optional[str] = None + + def validate(self, model): + try: + if getattr(model, self.reference_field_name) is None: + raise self.violation(model) + except ObjectDoesNotExist: + raise self.violation(model) + + class ValidityStartDateRules(BusinessRule): """Repeated rule pattern for descriptions.""" diff --git a/common/jinja2/common/app_info.jinja b/common/jinja2/common/app_info.jinja new file mode 100644 index 0000000000..c93742b85e --- /dev/null +++ b/common/jinja2/common/app_info.jinja @@ -0,0 +1,71 @@ +{% extends "layouts/layout.jinja" %} + +{% from "components/inset-text/macro.njk" import govukInsetText %} +{% from "components/table/macro.njk" import govukTable %} + + +{% set page_title = "Application information" %} + +{% block breadcrumb %} +
+
    +
  1. + Home +
    2. +
  2. + {{ page_title }} +
    3. +
+
+{% endblock %} + +{% block content %} +

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

+ +

+ Active business rule checks +

+ + {% set table_head = [ + {"text": "Workbasket ID"}, + {"text": "Time started"}, + {"text": "Celery task ID"}, + ] %} + {% set table_rows = [] %} + + {% if celery_healthy %} + {% if active_checks %} + {% for check in active_checks %} + {{ table_rows.append([ + {"text": check.workbasket_id}, + {"text": check.date_time_start}, + {"text": check.task_id}, + ]) or "" }} + {% endfor %} + {% else %} + {{ table_rows.append([ + { + "text": "No active checks.", + "colspan": 3, + } + ]) or "" }} + {% endif %} + {% else %} + {{ table_rows.append([ + { + "text": "Business rule check details are currently unavailable.", + "colspan": 3, + } + ]) or "" }} + {% endif %} + + {{ govukTable({ + "head": table_head, + "rows": table_rows + }) }} + +{% endblock %} diff --git a/common/migrations/0006_auto_20221114_1000.py b/common/migrations/0006_auto_20221114_1000.py new file mode 100644 index 0000000000..f6e6f76153 --- /dev/null +++ b/common/migrations/0006_auto_20221114_1000.py @@ -0,0 +1,12 @@ +# Generated by Django 3.1.14 on 2022-02-21 14:57 + +from django.db import migrations + + +class Migration(migrations.Migration): + dependencies = [ + ("common", "0005_transaction_index"), + ("measures", "0010_add_requires_to_action_and_accepts_to_condition_code"), + ] + + operations = [] diff --git a/common/migrations/0007_auto_20221114_1040_fix_missing_current_versions.py b/common/migrations/0007_auto_20221114_1040_fix_missing_current_versions.py new file mode 100644 index 0000000000..139d769d60 --- /dev/null +++ b/common/migrations/0007_auto_20221114_1040_fix_missing_current_versions.py @@ -0,0 +1,50 @@ +# Generated by Django 3.1.14 on 2022-02-21 14:57 + +from django.db import migrations + + +def current_versions_without_tracked_models(apps): + from common.models.transactions import TransactionPartition + from workbaskets.validators import WorkflowStatus + + """:return: Queryset of tracked models with no current version, but are published, and on the correct partition.""" + + TrackedModel = apps.get_model("common", "TrackedModel") + + return ( + TrackedModel.objects.select_related("version_group") + .filter( + version_group__current_version_id__isnull=True, + ) + .select_related("transaction") + .filter( + transaction__partition=TransactionPartition.REVISION, + transaction__workbasket__status=WorkflowStatus.PUBLISHED, + ) + .order_by("transaction__order") + ) + + +def fix_current_versions(apps, schemaeditor): + """Update affected models and run through in transaction order, to correctly + update the missing correct current version.""" + + results = current_versions_without_tracked_models(apps) + + for result in results: + version_group = result.version_group + version_group.current_version_id = result.id + version_group.save() + + +class Migration(migrations.Migration): + dependencies = [ + ("common", "0006_auto_20221114_1000"), + ] + + operations = [ + migrations.RunPython( + fix_current_versions, + lambda apps, schema: None, + ), + ] diff --git a/common/models/transactions.py b/common/models/transactions.py index 48f50a0ac0..3038236a6d 100644 --- a/common/models/transactions.py +++ b/common/models/transactions.py @@ -64,6 +64,10 @@ class TransactionsAlreadyApproved(Exception): pass +class TransactionsAlreadyInDraft(Exception): + pass + + class TransactionQueryset(models.QuerySet): @property def tracked_models(self): @@ -80,6 +84,11 @@ def approved(self): partition__lte=TransactionPartition.get_highest_approved_partition(), ) + def unapproved(self): + return self.exclude( + partition__lte=TransactionPartition.get_highest_approved_partition(), + ) + def preorder_negative_transactions(self) -> None: """ Makes all order numbers negative if there is even one negative order @@ -145,16 +154,16 @@ def save_drafts(self, partition_scheme): Contained tracked models to become the current version. Order field is updated so these transactions are at the end of the approved partition """ + if not self.exists(): + logger.info("Draft contains no transactions, bailing out early.") + return + if self.approved().exists(): pks = self.values_list("pk") msg = f"One or more Transactions was not in the DRAFT partition: {pks}" logger.error(msg) raise TransactionsAlreadyApproved(msg) - if self.first() is None: - logger.info("Draft contains no transactions, bailing out early.") - return - # Find the transaction in the destination approved partition with the highest order # get_approved_partition may raise a ValueError, e.g. when attempting to create # a seed transaction when revisions exist, so it is called before any of the @@ -166,7 +175,7 @@ def save_drafts(self, partition_scheme): "Approved partition %s selected by partition_scheme.", approved_partition, ) - logger.debug("Update versions_group.") + logger.debug("Update version_group.") for obj in self.tracked_models.order_by("pk"): version_group = obj.version_group @@ -175,6 +184,49 @@ def save_drafts(self, partition_scheme): self.move_to_end_of_partition(approved_partition) + @atomic + def revert_current_version(self): + """Set current_version to previous version or None on a basket's tracked + model version groups.""" + for obj in self.tracked_models.order_by("-pk").select_related("version_group"): + version_group = obj.version_group + versions = ( + version_group.versions.has_approved_state() + .order_by("-pk") + .exclude(pk=obj.pk) + ) + if versions.count() == 0: + version_group.current_version = None + else: + version_group.current_version = versions.first() + version_group.save() + + @atomic + def move_to_draft(self): + """ + Save SEED_FILE or REVISION transactions as DRAFT. + + Set current_version to previous version or None on a basket's tracked + model version groups. + """ + if not self.exists(): + logger.info("Queryset contains no transactions, bailing out early.") + return + + if self.unapproved().exists(): + pks = self.values_list("pk") + msg = f"One or more Transactions was already in the DRAFT partition: {pks}" + logger.error(msg) + raise TransactionsAlreadyInDraft(msg) + + logger.debug("Update version_group.") + + self.revert_current_version() + + logger.info("Save with DRAFT partition scheme") + + self.move_to_end_of_partition(TransactionPartition.DRAFT) + class Transaction(TimestampedMixin): """ diff --git a/common/querysets.py b/common/querysets.py index 01684608e9..15830f27ad 100644 --- a/common/querysets.py +++ b/common/querysets.py @@ -96,6 +96,13 @@ def as_at_today(self) -> QuerySet: """ return self.as_at(date.today()) + def as_at_and_beyond(self, date) -> QuerySet: + """Return the instances of the model that are respresented at the + current date as well as any future instances.""" + return self.filter( + Q(valid_between__contains=date) | Q(valid_between__startswith__gt=date), + ) + class TransactionPartitionQuerySet(QuerySet): @classmethod diff --git a/common/tests/factories.py b/common/tests/factories.py index 72387e76c4..be18994966 100644 --- a/common/tests/factories.py +++ b/common/tests/factories.py @@ -128,7 +128,7 @@ class Meta: approver = factory.SubFactory(UserFactory) status = WorkflowStatus.APPROVED transaction = factory.RelatedFactory( - "common.tests.factories.TransactionFactory", + "common.tests.factories.ApprovedTransactionFactory", factory_related_name="workbasket", ) diff --git a/common/tests/test_business_rules.py b/common/tests/test_business_rules.py index ffeccbf311..8a4d68c44c 100644 --- a/common/tests/test_business_rules.py +++ b/common/tests/test_business_rules.py @@ -12,6 +12,8 @@ from common.business_rules import NoOverlapping from common.business_rules import PreventDeleteIfInUse from common.business_rules import UniqueIdentifyingFields +from common.business_rules import skip_when_deleted +from common.business_rules import skip_when_not_deleted from common.models.mixins.description import DescriptionMixin from common.tests import factories from common.tests.util import raises_if @@ -158,3 +160,35 @@ def test_prevent_delete_if_in_use(approved_transaction): TestInUse(model.transaction).validate(model) assert model.in_use.called + + +@skip_when_deleted +class SkipWhenDeletedRule(BusinessRule): + def validate(): + pass + + +@pytest.mark.s +def test_skip_when_deleted(capfd): + model = factories.TestModel1Factory.create(update_type=UpdateType.DELETE) + SkipWhenDeletedRule(model.transaction).validate(model) + + assert "Skipping SkipWhenDeletedRule: update_type is 2" in capfd.readouterr().err + + +@skip_when_not_deleted +class SkipWhenNotDeletedRule(BusinessRule): + def validate(): + pass + + +@pytest.mark.s +@pytest.mark.parametrize("update_type", [UpdateType.CREATE, UpdateType.UPDATE]) +def test_skip_when_not_deleted(capfd, update_type): + model = factories.TestModel1Factory.create(update_type=update_type) + SkipWhenNotDeletedRule(model.transaction).validate(model) + + assert ( + f"Skipping SkipWhenNotDeletedRule: update_type is {update_type}" + in capfd.readouterr().err + ) diff --git a/common/tests/test_migrations.py b/common/tests/test_migrations.py new file mode 100644 index 0000000000..a215160ae4 --- /dev/null +++ b/common/tests/test_migrations.py @@ -0,0 +1,77 @@ +from datetime import date +from datetime import datetime + +import pytest + +from common.models.transactions import TransactionPartition +from common.util import TaricDateRange +from common.validators import UpdateType + + +@pytest.mark.django_db() +def test_missing_current_version_fix(migrator): + migrator.reset() + + """Ensures that the initial migration works.""" + new_state = migrator.apply_initial_migration(("common", "0006_auto_20221114_1000")) + # setup + + user_class = new_state.apps.get_model("auth", "User") + workbasket_class = new_state.apps.get_model("workbaskets", "WorkBasket") + measurement_unit_class = new_state.apps.get_model("measures", "MeasurementUnit") + transaction_class = new_state.apps.get_model("common", "Transaction") + version_group_class = new_state.apps.get_model("common", "VersionGroup") + + # create user + user = user_class.objects.create( + email="123123@sdfsdf.com", + username="sdfsdfsdf", + first_name="sdfdsf", + last_name="sdfsdf", + ) + # Create version group + version_group = version_group_class.objects.create() + # create workbasket + workbasket = workbasket_class.objects.create( + title=f"xxx {datetime.time}", + reason="some reason", + author=user, + ) + # create transaction + transaction = transaction_class.objects.create( + workbasket=workbasket, + order=1, + partition=TransactionPartition.REVISION, + ) + + kwargs = { + "update_type": UpdateType.CREATE, + "transaction": transaction, + "valid_between": TaricDateRange(date(2000, 1, 1), None), + "code": "XXX", + "description": "made up measurement unit", + "abbreviation": "xxx", + "version_group": version_group, + } + + # create measurement unit + measurement_unit = measurement_unit_class.objects.create(**kwargs) + measurement_unit_id = measurement_unit.trackedmodel_ptr_id + + # publish workbasket + workbasket.status = "PUBLISHED" + workbasket.save() + + assert measurement_unit.version_group.current_version_id is None + + # run fix migration + migrator.apply_tested_migration( + ("common", "0007_auto_20221114_1040_fix_missing_current_versions"), + ) + measurement_unit = measurement_unit_class.objects.get( + trackedmodel_ptr_id=measurement_unit_id, + ) + + # assert + assert measurement_unit.version_group.current_version_id == measurement_unit_id + migrator.reset() diff --git a/common/tests/test_models.py b/common/tests/test_models.py index 9d8e01b210..dd37a92e08 100644 --- a/common/tests/test_models.py +++ b/common/tests/test_models.py @@ -11,6 +11,7 @@ from common.models import TrackedModel from common.models.transactions import Transaction from common.models.transactions import TransactionPartition +from common.models.transactions import TransactionsAlreadyInDraft from common.models.utils import LazyString from common.models.utils import override_current_transaction from common.tests import factories @@ -532,6 +533,57 @@ def test_save_drafts_transaction_updates(unordered_transactions): assert_transaction_order(Transaction.objects.all()) +def test_move_to_draft_unapproved_transactions(unapproved_transaction): + with pytest.raises(TransactionsAlreadyInDraft) as exception: + unapproved_transaction.workbasket.transactions.move_to_draft() + + pks = unapproved_transaction.workbasket.transactions.values_list("pk") + + assert ( + exception.value.args[0] + == f"One or more Transactions was already in the DRAFT partition: {pks}" + ) + + +@pytest.mark.s +def test_move_to_draft_no_transactions(capfd): + wb = factories.ApprovedWorkBasketFactory.create(transaction=None) + + assert wb.transactions.move_to_draft() == None + assert ( + "Queryset contains no transactions, bailing out early." + in capfd.readouterr().err + ) + + +@pytest.mark.s +def test_move_to_draft(capfd, approved_workbasket): + approved_workbasket.transactions.move_to_draft() + readout = capfd.readouterr().err + + assert "Update version_group." in readout + assert "Save with DRAFT partition scheme" in readout + + for transaction in approved_workbasket.transactions.all(): + assert transaction.partition == TransactionPartition.DRAFT + + +def test_revert_current_version(approved_workbasket): + original_version = factories.TestModel1Factory.create( + transaction__workbasket=approved_workbasket, + ) + other_workbasket = factories.ApprovedWorkBasketFactory.create() + new_version = original_version.new_version(other_workbasket) + + # sanity check + assert new_version.version_group.current_version == new_version + + new_version.transaction.workbasket.transactions.revert_current_version() + new_version.refresh_from_db() + + assert new_version.version_group.current_version == original_version + + def test_structure_description(trackedmodel_factory): model = trackedmodel_factory.create() with override_current_transaction(model.transaction): diff --git a/common/tests/test_views.py b/common/tests/test_views.py index fd6ba9c666..db72943bdd 100644 --- a/common/tests/test_views.py +++ b/common/tests/test_views.py @@ -62,3 +62,12 @@ 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_app_info(valid_user_client): + response = valid_user_client.get(reverse("app-info")) + + assert response.status_code == 200 + + page = BeautifulSoup(str(response.content), "html.parser") + assert "Active business rule checks" in page.select("h2")[0].text diff --git a/common/urls.py b/common/urls.py index 82264cffc0..fcda91c7e1 100644 --- a/common/urls.py +++ b/common/urls.py @@ -16,6 +16,7 @@ urlpatterns = [ path("", views.HomeView.as_view(), name="home"), path("healthcheck", views.healthcheck, name="healthcheck"), + path("app-info", views.AppInfoView.as_view(), name="app-info"), path("login", views.LoginView.as_view(), name="login"), path("logout", views.LogoutView.as_view(), name="logout"), path("api-auth/", include("rest_framework.urls")), diff --git a/common/views.py b/common/views.py index b48536c4d9..92b2425b40 100644 --- a/common/views.py +++ b/common/views.py @@ -1,11 +1,14 @@ """Common views.""" import time +from datetime import datetime from typing import Optional from typing import Tuple from typing import Type import django.contrib.auth.views +import kombu.exceptions from django.conf import settings +from django.contrib.auth.mixins import LoginRequiredMixin from django.contrib.auth.mixins import PermissionRequiredMixin from django.contrib.auth.mixins import UserPassesTestMixin from django.core.cache import cache @@ -19,8 +22,10 @@ from django.http import HttpResponse from django.shortcuts import redirect from django.urls import reverse +from django.utils.timezone import make_aware from django.views import generic from django.views.generic import FormView +from django.views.generic import TemplateView from django.views.generic.base import View from django.views.generic.edit import FormMixin from django_filters.views import FilterView @@ -29,6 +34,7 @@ from common import forms from common.business_rules import BusinessRule from common.business_rules import BusinessRuleViolation +from common.celery import app from common.models import TrackedModel from common.pagination import build_pagination_list from common.validators import UpdateType @@ -97,6 +103,54 @@ def healthcheck(request): return response +class AppInfoView( + LoginRequiredMixin, + TemplateView, +): + template_name = "common/app_info.jinja" + + def active_checks(self): + results = [] + inspect = app.control.inspect() + if not inspect: + return results + + active_tasks = inspect.active() + if not active_tasks: + return results + + for _, task_info_list in active_tasks.items(): + for task_info in task_info_list: + if ( + task_info.get("name") + == "workbaskets.tasks.call_check_workbasket_sync" + ): + date_time_start = make_aware( + datetime.fromtimestamp( + task_info.get("time_start"), + ), + ).strftime("%Y-%m-%d, %H:%M:%S") + results.append( + { + "task_id": task_info.get("id"), + "workbasket_id": task_info.get("args", [""])[0], + "date_time_start": date_time_start, + }, + ) + + return results + + def get_context_data(self, **kwargs): + data = super().get_context_data(**kwargs) + try: + data["active_checks"] = self.active_checks() + data["celery_healthy"] = True + except kombu.exceptions.OperationalError as oe: + data["celery_healthy"] = False + + return data + + class LoginView(django.contrib.auth.views.LoginView): template_name = "common/login.jinja" diff --git a/conftest.py b/conftest.py index 36b161496d..1e3bb512ef 100644 --- a/conftest.py +++ b/conftest.py @@ -1192,3 +1192,31 @@ def model2_with_history(date_ranges): version_group=factories.VersionGroupFactory.create(), custom_sid=1, ) + + +# As per this open issue with pytest https://github.com/pytest-dev/pytest/issues/5997, +# some tests can only be run with global capturing disabled. Until we find a way +# to disable capturing from within the test itself we can mark tests that should be skipped +# unless global capturing is disabled via the "-s" flag. + +# TODO https://uktrade.atlassian.net/browse/TP2000-591 + +# See pytest docs for implementation below +# https://docs.pytest.org/en/7.1.x/example/simple.html#control-skipping-of-tests-according-to-command-line-option + + +def pytest_configure(config): + config.addinivalue_line( + "markers", + "s: mark test as needing global capturing disabled to run", + ) + + +def pytest_collection_modifyitems(config, items): + if config.getoption("-s") == "no": + # -s given in cli: do not skip s tests + return + skip_s = pytest.mark.skip(reason="need -s option to run") + for item in items: + if "s" in item.keywords: + item.add_marker(skip_s) diff --git a/docs/source/architecture/index.rst b/docs/source/architecture/index.rst index f19d8e6228..ff3534260f 100644 --- a/docs/source/architecture/index.rst +++ b/docs/source/architecture/index.rst @@ -38,6 +38,8 @@ third parties via open data. Concepts -------- +.. _TARIC: + TARIC ^^^^^ @@ -106,6 +108,53 @@ There are a number of convenience methods for finding "current" models. .. autoclass:: common.models.trackedmodel.TrackedModelQuerySet :members: latest_approved, approved_up_to_transaction + +Workbaskets +^^^^^^^^^^^ +A workbasket is a way of grouping together a set of changes to the tariff, +specifically a set of transactions. These transactions should in turn contain one +or more tracked models (see :class:`~common.models.trackedmodel.TrackedModel` ), +with one transaction corresponding to one :ref:`TARIC` object. E.g. one transaction +might contain three tracked models, a Measure and two components dependent on that +measure, such as a :class:`~measures.models.MeasureComponent`, and +:class:`~measures.models.FootnoteAssociationMeasure`. Empty transactions are possible, +though redundant. + +In database relationship terms, a workbasket has a one-to-many +connection with a transaction, which itself has a one-to-many relationship with a +tracked model. The ordering of transactions in a workbasket is important because the +exporter generates xml by passing over each transaction sequentially (see +:ref:`12-ordering-of-tariff-transactions`). Business rule validation is also run in +order with each transaction only aware of objects in preceding transactions in the same +basket and transactions in an already "approved" basket. + +A workbasket is a `finite state machine `_ and +a workbasket can be considered "approved" when it is in an `APPROVED`, `SENT`, or +`PUBLISHED` state. Other states include `ARCHIVED`, `PROPOSED`, and `ERRORED`. See below +for a full map of the different possible state transitions. + +.. image:: wb-state.svg + +Certain transition methods do more than just change a workbasket's status: + +:meth:`~workbaskets.models.WorkBasket.submit_for_approval` + +Performs Django model validation, ensures the workbasket contains transactions, and checks +that rules have been run successfully against those transactions. + +:meth:`~workbaskets.models.WorkBasket.approve` + +Sets `approver_id` to be that of current request user, moves all transactions from DRAFT +to REVISION status (see :class:`~common.models.transactions.TransactionPartition`), making these +changes visible to workbaskets in an unapproved status, and calls +:py:meth:`~exporter.tasks.upload_workbaskets` which should generate an XML envelope and +upload to an S3 bucket (this functionality is broken as of 20/10/2022). + +:meth:`~workbaskets.models.WorkBasket.cds_error` + +Unsets the `current_version` for each object in the basket, undoing the effects of `approve` +and making objects invisible to other unapproved workbaskets. + Domain Modules -------------- @@ -286,3 +335,7 @@ description. :mod:`taric` ^^^^^^^^^^^^ .. automodule:: taric + +:mod:`workbaskets` +^^^^^^^^^^^^^^^^^^ +.. automodule:: workbaskets diff --git a/docs/source/architecture/wb-state.svg b/docs/source/architecture/wb-state.svg new file mode 100644 index 0000000000..37d34e5edf --- /dev/null +++ b/docs/source/architecture/wb-state.svg @@ -0,0 +1,4 @@ + + + +
SENT
SENT
PROPOSED
PROPOSED
APPROVED
APPROVED
EDITING
EDITING
ARCHIVED
ARCHIVED
archive
archive
unarchive
unarchive
submit for approval
submit for...
withdraw
withdraw
approve
approve
CDS error
CDS error
PUBLISHED
PUBLISHED
export to cds
export to...
cds confirmed
cds confir...
ERRORED
ERROREDText is not SVG - cannot display \ No newline at end of file diff --git a/docs/source/glossary.rst b/docs/source/glossary.rst index ad61193923..5d027cca47 100644 --- a/docs/source/glossary.rst +++ b/docs/source/glossary.rst @@ -6,3 +6,7 @@ Glossary Transaction A transaction is a grouping of changes to the tariff data that must be applied atomically. + + Workbasket + A workbasket is a grouping of transactions to be applied in the same order in + which they were created. diff --git a/importer/forms.py b/importer/forms.py index 7600587919..cd6fccc598 100644 --- a/importer/forms.py +++ b/importer/forms.py @@ -64,7 +64,7 @@ def clean_taric_file(self): capture_exception(e) raise ValidationError(generic_error_message) - with open(settings.PATH_XSD_COMMODITIES_TARIC) as xsd_file: + with open(self.xsd_file) as xsd_file: xmlschema = lxml.etree.XMLSchema(file=xsd_file) try: @@ -93,6 +93,7 @@ class UploadTaricForm(ImportForm): required=False, initial=False, ) + xsd_file = settings.PATH_XSD_TARIC def __init__(self, *args, **kwargs): super().__init__(*args, **kwargs) diff --git a/measures/business_rules.py b/measures/business_rules.py index 7db24c16a8..57506735cb 100644 --- a/measures/business_rules.py +++ b/measures/business_rules.py @@ -11,6 +11,7 @@ from common.business_rules import ExclusionMembership from common.business_rules import FootnoteApplicability from common.business_rules import MustExist +from common.business_rules import MustExistNotNull from common.business_rules import PreventDeleteIfInUse from common.business_rules import UniqueIdentifyingFields from common.business_rules import ValidityPeriodContained @@ -158,7 +159,7 @@ class ME5(ValidityPeriodContained): container_field_name = "geographical_area" -class ME6(MustExist): +class ME6(MustExistNotNull): """The goods code must exist.""" reference_field_name = "goods_nomenclature" @@ -448,6 +449,7 @@ def validate(self, measure): raise self.violation(measure) +@skip_when_deleted @only_applicable_after("2007-12-31") class ME119(ValidityPeriodContained): """When a quota order number is used in a measure then the validity period @@ -714,6 +716,7 @@ def validate(self, measure): # -- Measure component +@skip_when_deleted class ME40(BusinessRule): """ If the flag "duty expression" on measure type is "mandatory" then at least diff --git a/measures/tests/test_business_rules.py b/measures/tests/test_business_rules.py index 362f5831ad..561177ebc7 100644 --- a/measures/tests/test_business_rules.py +++ b/measures/tests/test_business_rules.py @@ -286,6 +286,22 @@ def teardown(good): business_rules.ME6(measure.transaction).validate(measure) +def test_ME6_happy(reference_nonexistent_record): + measure = factories.MeasureFactory.create() + + assert business_rules.ME6(measure.transaction).validate(measure) is None + + +def test_ME6_null_goods(reference_nonexistent_record): + measure = factories.MeasureFactory.create( + goods_nomenclature__suffix="00", + goods_nomenclature=None, + ) + + with pytest.raises(BusinessRuleViolation): + business_rules.ME6(measure.transaction).validate(measure) + + def test_ME7(): """The goods nomenclature code must be a product code; that is, it may not be an intermediate line.""" @@ -763,6 +779,14 @@ def test_ME119_multiple_valid_origins(): business_rules.ME119(later_measure.transaction).validate(later_measure) +@pytest.mark.s +def test_ME119_skipped_when_deleted(capfd): + measure = factories.MeasureFactory.create(update_type=UpdateType.DELETE) + business_rules.ME119(measure.transaction).validate(measure) + + assert "Skipping ME119: update_type is 2" in capfd.readouterr().err + + def test_quota_origin_matching_area(): origin = factories.QuotaOrderNumberOriginFactory.create( geographical_area__sid="666", @@ -1173,6 +1197,14 @@ def test_ME40(applicability_code, component, condition_component, error_expected ).validate(measure) +@pytest.mark.s +def test_ME40_skipped_when_deleted(capfd): + measure = factories.MeasureFactory.create(update_type=UpdateType.DELETE) + business_rules.ME40(measure.transaction).validate(measure) + + assert "Skipping ME40: update_type is 2" in capfd.readouterr().err + + def test_ME41(reference_nonexistent_record): """The referenced duty expression must exist.""" diff --git a/package-lock.json b/package-lock.json index d6ee37bdab..9da0f3c3da 100644 --- a/package-lock.json +++ b/package-lock.json @@ -1011,9 +1011,9 @@ } }, "node_modules/loader-utils": { - "version": "2.0.2", - "resolved": "https://registry.npmjs.org/loader-utils/-/loader-utils-2.0.2.tgz", - "integrity": "sha512-TM57VeHptv569d/GKh6TAYdzKblwDNiumOdkFnejjD0XwTH87K90w3O7AiJRqdQoXygvi1VQTJTLGhJl7WqA7A==", + "version": "2.0.4", + "resolved": "https://registry.npmjs.org/loader-utils/-/loader-utils-2.0.4.tgz", + "integrity": "sha512-xXqpXoINfFhgua9xiqD8fPFHgkoq1mmmpE92WlDbm9rNRd/EbRb+Gqf908T2DMfuHjjJlksiK2RbHVOdD/MqSw==", "dependencies": { "big.js": "^5.2.2", "emojis-list": "^3.0.0", @@ -2682,9 +2682,9 @@ "integrity": "sha512-92+huvxMvYlMzMt0iIOukcwYBFpkYJdpl2xsZ7LrlayO7E8SOv+JJUEK17B/dJIHAOLMfh2dZZ/Y18WgmGtYNw==" }, "loader-utils": { - "version": "2.0.2", - "resolved": "https://registry.npmjs.org/loader-utils/-/loader-utils-2.0.2.tgz", - "integrity": "sha512-TM57VeHptv569d/GKh6TAYdzKblwDNiumOdkFnejjD0XwTH87K90w3O7AiJRqdQoXygvi1VQTJTLGhJl7WqA7A==", + "version": "2.0.4", + "resolved": "https://registry.npmjs.org/loader-utils/-/loader-utils-2.0.4.tgz", + "integrity": "sha512-xXqpXoINfFhgua9xiqD8fPFHgkoq1mmmpE92WlDbm9rNRd/EbRb+Gqf908T2DMfuHjjJlksiK2RbHVOdD/MqSw==", "requires": { "big.js": "^5.2.2", "emojis-list": "^3.0.0", diff --git a/pii-ner-exclude.txt b/pii-ner-exclude.txt index a741a251f5..d8d2e2294a 100644 --- a/pii-ner-exclude.txt +++ b/pii-ner-exclude.txt @@ -1242,3 +1242,11 @@ indent=0 run_business_rules_form f"empty Exit +meth:`~workbaskets.models +https://uktrade.atlassian.net/browse/TP2000-571 +validity.end.date>2023 +Implementing RegulationY-%m-%d +date_time_start +Y-%m-%d +Skipping ME119: update_type +Implementing Regulation \ No newline at end of file diff --git a/pii-secret-exclude.txt b/pii-secret-exclude.txt index 5adc87d18c..ac0d362855 100644 --- a/pii-secret-exclude.txt +++ b/pii-secret-exclude.txt @@ -13,3 +13,4 @@ runtime.txt sample.env common/jinja2/components/sort_icon.jinja workbaskets/tests/test_admin.py +common/tests/test_migrations.py diff --git a/quotas/business_rules.py b/quotas/business_rules.py index 2190e06fb0..418f7bfa56 100644 --- a/quotas/business_rules.py +++ b/quotas/business_rules.py @@ -62,6 +62,19 @@ def validate(self, order_number): raise self.violation(order_number) +class ON4(BusinessRule): + """The referenced geographical area must exist.""" + + def validate(self, order_number): + if ( + order_number.origins.approved_up_to_transaction( + order_number.transaction, + ).count() + == 0 + ): + raise self.violation(order_number) + + class ON5(BusinessRule): """There may be no overlap in time of two quota order number origins with the same quota order number SID and geographical area id.""" diff --git a/quotas/models.py b/quotas/models.py index fb3dfbff32..d8de059da2 100644 --- a/quotas/models.py +++ b/quotas/models.py @@ -66,6 +66,7 @@ class QuotaOrderNumber(TrackedModel, ValidityMixin): business_rules = ( business_rules.ON1, business_rules.ON2, + business_rules.ON4, business_rules.ON9, business_rules.ON11, UniqueIdentifyingFields, diff --git a/quotas/tests/test_business_rules.py b/quotas/tests/test_business_rules.py index 93377788ce..9b91f91cfe 100644 --- a/quotas/tests/test_business_rules.py +++ b/quotas/tests/test_business_rules.py @@ -54,6 +54,26 @@ def test_ON2(date_ranges, existing_range, new_range, ranges_overlap): business_rules.ON2(order_number.transaction).validate(order_number) +def test_ON4_pass(date_ranges, approved_transaction, unapproved_transaction): + order_number = factories.QuotaOrderNumberFactory.create( + valid_between=date_ranges.normal, + transaction=unapproved_transaction, + ) + + assert business_rules.ON4(order_number.transaction).validate(order_number) is None + + +def test_ON4_fail(date_ranges, approved_transaction, unapproved_transaction): + order_number = factories.QuotaOrderNumberFactory.create( + valid_between=date_ranges.normal, + transaction=approved_transaction, + origin=None, + ) + + with pytest.raises(BusinessRuleViolation): + business_rules.ON4(order_number.transaction).validate(order_number) + + def test_ON5(date_ranges, approved_transaction, unapproved_transaction): """There may be no overlap in time of two quota order number origins with the same quota order number SID and geographical area id.""" diff --git a/requirements.txt b/requirements.txt index 25d350ff0c..9d7c2ca339 100644 --- a/requirements.txt +++ b/requirements.txt @@ -26,11 +26,13 @@ django-rest-polymorphic==0.1.9 django-sequences==2.6 django-staff-sso-client==3.1.1 django-storages==1.11.1 +django-test-migrations==1.2.0 django-webpack-loader==1.0.0 djangorestframework==3.12.2 drf-flex-fields==0.8.9 elastic-apm==6.7.2 factory-boy==3.2.0 +flower==1.2.0 freezegun==1.1.0 gevent==21.12.0 govuk-frontend-jinja @ git+https://github.com/alphagov/govuk-frontend-jinja.git@15845e4cca3a05df72c6e13ec6a7e35acc682f52 diff --git a/sample.env b/sample.env index 766e9efe84..05729d1641 100644 --- a/sample.env +++ b/sample.env @@ -3,6 +3,7 @@ ENABLE_DJANGO_DEBUG_TOOLBAR=True ENV=dev LOG_LEVEL=DEBUG SENTRY_DSN= +CELERY_BROKER_URL=redis://127.0.0.1:6379/1 # S3 Bucket for HMRC envelope uploads. HMRC_STORAGE_BUCKET_NAME=hmrc diff --git a/workbaskets/admin.py b/workbaskets/admin.py index 7676b6294c..0a7b5b55e3 100644 --- a/workbaskets/admin.py +++ b/workbaskets/admin.py @@ -30,10 +30,6 @@ class Meta: ) transition = forms.ChoiceField(required=False) - rule_check_task_id = forms.CharField( - required=False, - widget=AdminTextInputWidget(), - ) rule_check_task_status = forms.CharField( required=False, widget=AdminTextInputWidget(), diff --git a/workbaskets/jinja2/includes/workbaskets/workbasket-detail.jinja b/workbaskets/jinja2/includes/workbaskets/workbasket-detail.jinja deleted file mode 100644 index 78c10bc52f..0000000000 --- a/workbaskets/jinja2/includes/workbaskets/workbasket-detail.jinja +++ /dev/null @@ -1,42 +0,0 @@ -{% from "components/table/macro.njk" import govukTable %} - -{% if workbasket %} -
-
-

- Your tariff changes -

- {% set change_count = workbasket.tracked_models.count() %} -

{{ change_count }} change{{ pluralize(change_count) }}

-

- Publish all changes -

- {% set table_rows = [] %} - {% for obj in page_obj.object_list %} - {% set object_link -%} - {{ obj.get_described_object() if obj.get_described_object else obj.structure_code }} - {%- endset %} - {{ table_rows.append([ - {"text": obj.update_type_str}, - {"text": obj._meta.verbose_name|capitalize, "classes": "govuk-!-width-one-quarter"}, - {"html": object_link, "classes": "govuk-!-width-one-quarter"}, - {"text": obj.structure_description if obj.structure_description else "-", "classes": "govuk-!-width-one-quarter"}, - {"text": "{:%d %b %Y}".format(obj.transaction.updated_at), "classes": "govuk-!-width-one-quarter"}, - ]) or "" }} - {% endfor %} - - {{ govukTable({ - "head": [ - {"text": "Action"}, - {"text": "Type"}, - {"text": "ID"}, - {"text": "Description"}, - {"text": "Activity date"}, - ], - "rows": table_rows - }) }} - - {% include "includes/common/pagination.jinja" %} -
-
-{% endif %} \ No newline at end of file diff --git a/workbaskets/jinja2/workbaskets/delete_changes_confirm.jinja b/workbaskets/jinja2/workbaskets/delete_changes_confirm.jinja index 4ea6bb458f..b900908611 100644 --- a/workbaskets/jinja2/workbaskets/delete_changes_confirm.jinja +++ b/workbaskets/jinja2/workbaskets/delete_changes_confirm.jinja @@ -1,6 +1,7 @@ {% extends "layouts/layout.jinja" %} {% from "components/panel/macro.njk" import govukPanel %} +{% from "components/button/macro.njk" import govukButton %} {% set page_title = "Remove tariff changes" %} @@ -22,9 +23,16 @@ "classes": "govuk-!-margin-bottom-7" }) }} -

+ {% if request.session.workbasket %} + {{ govukButton({ + "text": "Go to your tariff changes", + "href": url("workbaskets:workbasket-ui-detail", args=[request.session.workbasket.id]), + "classes": "govuk-button--secondary" + }) }} + {% endif %} +

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

    +
{% endblock %} diff --git a/workbaskets/models.py b/workbaskets/models.py index 3c7f45d8a1..8d42661755 100644 --- a/workbaskets/models.py +++ b/workbaskets/models.py @@ -443,19 +443,18 @@ def cds_confirmed(self): def cds_error(self): """If a workbasket, after approval, is then rejected by CDS it is important to roll back the current models to the previous approved - version.""" - for obj in self.tracked_models.order_by("-pk").select_related("version_group"): - version_group = obj.version_group - versions = ( - version_group.versions.has_approved_state() - .order_by("-pk") - .exclude(pk=obj.pk) - ) - if versions.count() == 0: - version_group.current_version = None - else: - version_group.current_version = versions.first() - version_group.save() + version and revert transaction partition to DRAFT.""" + self.transactions.move_to_draft() + + @transition( + field=status, + source=WorkflowStatus.ERRORED, + target=WorkflowStatus.EDITING, + custom={"label": "Restore for further editing."}, + ) + def restore(self): + """WorkBasket is ready to be worked on again after being rejected by + CDS.""" def save_to_session(self, session): session["workbasket"] = { diff --git a/workbaskets/tests/test_admin.py b/workbaskets/tests/test_admin.py index bccb39af24..8e0e318e21 100644 --- a/workbaskets/tests/test_admin.py +++ b/workbaskets/tests/test_admin.py @@ -142,3 +142,34 @@ def test_terminate_workbasket_rule_check(client, superadmin): assert response.status_code == 302 workbasket.refresh_from_db() assert not workbasket.rule_check_task_id + + +# https://uktrade.atlassian.net/browse/TP2000-556 +@override_settings(CELERY_TASK_ALWAYS_EAGER=True) +def test_workbasket_empty_rule_check_task_id_value(client, superadmin): + """Test that admin change view saves workbasket rule_check_task_id as a null + value, rather than an empty string, avoiding duplicate value errors.""" + + factories.WorkBasketFactory.create(rule_check_task_id="") + workbasket = factories.WorkBasketFactory.create( + status=WorkflowStatus.EDITING, + rule_check_task_id=None, + ) + change_url = reverse( + "admin:workbaskets_workbasket_change", + args=[workbasket.id], + ) + client.force_login(superadmin) + response = client.post( + change_url, + data={ + "transition": "submit_for_approval", + "reason": workbasket.reason, + "title": workbasket.title, + }, + ) + + assert response.status_code == 302 + workbasket.refresh_from_db() + + assert workbasket.rule_check_task_status == None diff --git a/workbaskets/tests/test_models.py b/workbaskets/tests/test_models.py index acaf9aa8d2..667529d196 100644 --- a/workbaskets/tests/test_models.py +++ b/workbaskets/tests/test_models.py @@ -2,6 +2,7 @@ from unittest.mock import patch import pytest +from django.conf import settings from django_fsm import TransitionNotAllowed from common.models import TrackedModel @@ -345,3 +346,43 @@ def test_current_transaction_returns_last_approved_transaction( current = WorkBasket.get_current_transaction(session_request) assert current == approved_transaction + + +@pytest.mark.parametrize( + "method, source, target", + [ + ("archive", "EDITING", "ARCHIVED"), + ("unarchive", "ARCHIVED", "EDITING"), + ("submit_for_approval", "EDITING", "PROPOSED"), + ("withdraw", "PROPOSED", "EDITING"), + ("export_to_cds", "APPROVED", "SENT"), + ("cds_confirmed", "SENT", "PUBLISHED"), + ("cds_error", "SENT", "ERRORED"), + ("restore", "ERRORED", "EDITING"), + ], +) +def test_workbasket_transition_methods(method, source, target): + """Test that workbasket transition methods move workbasket status from + source to target.""" + + wb = factories.WorkBasketFactory.create(status=getattr(WorkflowStatus, source)) + getattr(wb, method)() + + assert wb.status == getattr(WorkflowStatus, target) + + +@patch("exporter.tasks.upload_workbaskets.delay") +def test_approve(upload, valid_user): + """Test that approve transitions workbasket from PROPOSED to APPROVED, + setting approver and shifting transaction from DRAFT to REVISION + partition.""" + + wb = factories.WorkBasketFactory.create(status=WorkflowStatus.PROPOSED) + wb.approve(valid_user.pk, settings.TRANSACTION_SCHEMA) + + assert wb.status == WorkflowStatus.APPROVED + assert wb.approver == valid_user + upload.assert_called_once() + + for transaction in wb.transactions.all(): + assert transaction.partition == TransactionPartition.REVISION diff --git a/workbaskets/tests/test_views.py b/workbaskets/tests/test_views.py index 81381b32ef..1297c3f42b 100644 --- a/workbaskets/tests/test_views.py +++ b/workbaskets/tests/test_views.py @@ -3,7 +3,6 @@ import pytest from bs4 import BeautifulSoup -from django.test import override_settings from django.urls import reverse from django.utils.timezone import localtime @@ -13,14 +12,10 @@ from common.tests.factories import GeographicalAreaFactory from common.tests.factories import GoodsNomenclatureFactory from common.tests.factories import MeasureFactory -from common.tests.util import validity_period_post_data -from common.validators import UpdateType from exporter.tasks import upload_workbaskets from measures.models import Measure 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 from workbaskets.views import ui @@ -83,113 +78,6 @@ def test_workbasket_create_without_permission(client): assert response.status_code == 403 -@override_settings(CELERY_TASK_ALWAYS_EAGER=True, CELERY_TASK_EAGER_PROPOGATES=True) -@patch("exporter.tasks.upload_workbaskets") -def test_submit_workbasket( - mock_upload, - approved_transaction, - unapproved_transaction, - valid_user, - client, -): - workbasket = unapproved_transaction.workbasket - assert_workbasket_valid(workbasket) - - url = reverse( - "workbaskets:workbasket-ui-submit", - kwargs={"pk": workbasket.pk}, - ) - - client.force_login(valid_user) - response = client.get(url) - - assert response.status_code == 302 - assert response.url == reverse("home") - - workbasket.refresh_from_db() - - assert workbasket.approver is not None - assert "workbasket" not in client.session - mock_upload.delay.assert_called_once_with() - - -@pytest.mark.parametrize( - ("other_statuses", "should_reuse"), - ( - ({}, False), - ({WorkflowStatus.PROPOSED, WorkflowStatus.ARCHIVED}, False), - ({WorkflowStatus.EDITING}, True), - ), - ids=( - "will create basket if none exists", - "will not reuse unapproved baskets", - "will reuse basket in EDITING state", - ), -) -@override_settings(CELERY_TASK_ALWAYS_EAGER=True, CELERY_TASK_EAGER_PROPOGATES=True) -@patch("exporter.tasks.upload_workbaskets") -def test_edit_after_submit( - upload, - valid_user_client, - date_ranges, - other_statuses, - should_reuse, -): - # submit a workbasket containing a newly created footnote - workbasket = factories.WorkBasketFactory.create() - with workbasket.new_transaction(): - footnote = factories.FootnoteFactory.create( - update_type=UpdateType.CREATE, - ) - assert footnote.transaction.workbasket == workbasket - - assert_workbasket_valid(workbasket) - - # create workbaskets in different unapproved states - # to check that the system doesn't select these - other_baskets = [ - factories.WorkBasketFactory.create(status=other_status) - for other_status in other_statuses - ] - - response = valid_user_client.get( - reverse( - "workbaskets:workbasket-ui-submit", - kwargs={"pk": workbasket.pk}, - ), - ) - assert response.status_code == 302 - - # edit the footnote description start date, to avoid FO4 violation - description = footnote.descriptions.first() - description.validity_start = date_ranges.later.lower - description.save(force_write=True) - - # edit the footnote - response = valid_user_client.post( - footnote.get_url("edit"), - validity_period_post_data( - date_ranges.later.lower, - date_ranges.later.upper, - ), - ) - assert response.status_code == 302 - - # check that the session workbasket has been replaced by a new one - session_workbasket = WorkBasket.load_from_session(valid_user_client.session) - assert session_workbasket.id != workbasket.id - assert session_workbasket.status == WorkflowStatus.EDITING - assert (session_workbasket in other_baskets) == should_reuse - - # check that the footnote edit is in the new session workbasket - assert session_workbasket.transactions.count() == 1 - tx = session_workbasket.transactions.first() - assert tx.tracked_models.count() == 1 - new_footnote_version = tx.tracked_models.first() - assert new_footnote_version.pk != footnote.pk - assert new_footnote_version.version_group == footnote.version_group - - def test_download( approved_workbasket, client, @@ -358,7 +246,6 @@ def test_select_workbasket_without_permission(client): @pytest.mark.parametrize( "form_action, url_name", [ - ("publish-all", "workbaskets:workbasket-ui-submit"), ("remove-selected", "workbaskets:workbasket-ui-delete-changes"), ("page-prev", "workbaskets:workbasket-ui-detail"), ("page-next", "workbaskets:workbasket-ui-detail"), @@ -401,11 +288,10 @@ def test_delete_changes_confirm_200(valid_user_client, session_workbasket): ( "workbaskets:workbasket-ui-delete-changes", "workbaskets:edit-workbasket", - "workbaskets:workbasket-ui-submit", ), ) def test_workbasket_views_without_permission(url_name, client, session_workbasket): - """Tests that delete, edit, and submit endpoints return 403s to user without + """Tests that delete and edit endpoints return 403s to user without permissions.""" url = reverse( url_name, diff --git a/workbaskets/urls.py b/workbaskets/urls.py index b1ba8a537b..239e545637 100644 --- a/workbaskets/urls.py +++ b/workbaskets/urls.py @@ -51,11 +51,6 @@ ui_views.WorkBasketConfirmCreate.as_view(), name="workbasket-ui-confirm-create", ), - path( - f"/submit/", - ui_views.WorkBasketSubmit.as_view(), - name="workbasket-ui-submit", - ), path( f"/delete-changes/", ui_views.WorkBasketDeleteChanges.as_view(), diff --git a/workbaskets/views/ui.py b/workbaskets/views/ui.py index e3593acdd2..c6fb68f636 100644 --- a/workbaskets/views/ui.py +++ b/workbaskets/views/ui.py @@ -17,12 +17,10 @@ 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 @@ -38,7 +36,6 @@ from measures.models import Measure from measures.pagination import MeasurePaginator from workbaskets import forms -from workbaskets import tasks from workbaskets.models import WorkBasket from workbaskets.tasks import call_check_workbasket_sync from workbaskets.validators import WorkflowStatus @@ -128,34 +125,6 @@ def post(self, request, *args, **kwargs): return redirect(reverse("workbaskets:workbasket-ui-list")) -class WorkBasketSubmit(PermissionRequiredMixin, SingleObjectMixin, RedirectView): - """UI endpoint for submitting a workbasket to HMRC CDS.""" - - model = WorkBasket - permission_required = "workbaskets.change_workbasket" - - def get_redirect_url(self, *args, **kwargs) -> str: - return reverse("home") - - def get(self, *args, **kwargs): - workbasket: WorkBasket = self.get_object() - - ( - tasks.transition.si( - workbasket.pk, - "submit_for_approval", - ) - | tasks.transition.si( - workbasket.pk, - "approve", - self.request.user.pk, - settings.TRANSACTION_SCHEMA, - ) - ).delay() - - return super().get(*args, **kwargs) - - class WorkBasketDeleteChanges(PermissionRequiredMixin, ListView): """UI for user review of WorkBasket item deletion.""" @@ -297,7 +266,6 @@ class WorkBasketDetail(TemplateResponseMixin, FormMixin, View): # Form action mappings to URL names. action_success_url_names = { "run-business-rules": "workbaskets:workbasket-ui-detail", - "publish-all": "workbaskets:workbasket-ui-submit", "remove-selected": "workbaskets:workbasket-ui-delete-changes", "page-prev": "workbaskets:workbasket-ui-detail", "page-next": "workbaskets:workbasket-ui-detail", @@ -371,7 +339,7 @@ def run_business_rules(self): def get_success_url(self): form_action = self.request.POST.get("form-action") - if form_action in ("publish-all", "remove-selected"): + if form_action == "remove-selected": return reverse( self.action_success_url_names[form_action], kwargs={"pk": self.workbasket.pk},