From 8fe5dccfd4eebf1ff1e141462c1cf3d5931f7147 Mon Sep 17 00:00:00 2001 From: Tom Mc Donagh <94466845+TomMacca@users.noreply.github.com> Date: Tue, 12 Sep 2023 15:40:53 +0100 Subject: [PATCH 1/5] TP2000 1038 adjusts handling of goods nomenclature description updates with no periods (#1027) * - adds conditional statement in create_missing_goods_nomenclature_description_period that looks for existing goods nomenclature description and if exists, uses the existing validity date. It reverts to todays date if none found * - adds tests for conditional statement in create_missing_goods_nomenclature_description_period * - adjusts formatting for commodity import handlers * - adjusts formatting for commodity import handlers tests * - modifies factory creates in the goods nomenclature description period tests so not to create a related goods nomenclature description automatically * - adds comment about goods_nomenclature_description --- commodities/import_handlers.py | 25 +++++++++- commodities/tests/test_importer.py | 74 ++++++++++++++++++++++++++++++ 2 files changed, 98 insertions(+), 1 deletion(-) diff --git a/commodities/import_handlers.py b/commodities/import_handlers.py index 0a38b2417..afd6a212f 100644 --- a/commodities/import_handlers.py +++ b/commodities/import_handlers.py @@ -7,6 +7,7 @@ from commodities import import_parsers as parsers from commodities import models from commodities import serializers +from commodities.models.orm import GoodsNomenclatureDescription from common.validators import UpdateType from footnotes.models import Footnote from importer.handlers import BaseHandler @@ -154,6 +155,10 @@ def create_missing_goods_nomenclature_description_period( TODO : implement correct period handling to correctly resolve this crude workaround """ data = dict() + + # The code below looks for an existing goods nomenclature description in our db (using the description sid) + # and if it finds one it will use the validity start date from that record to use in the new description. + # If there is no existing description found then it will use todays date as the validity start date. # Setting to today's date is not perfect, and could cause issues in circumstances where the end date of the # goods nomenclature is before today, however if that's the case it does not really matter. # an additional possibility is that there already exists a description with this date, which will cause a rule @@ -161,7 +166,25 @@ def create_missing_goods_nomenclature_description_period( # Ultimately this is a crude temporary measure that will be superseded by a correct implementation splitting # out description periods into a new table - data["validity_start"] = date.today() + + # goods_nomenclature_description = None if no description exists + goods_nomenclature_description = ( + GoodsNomenclatureDescription.objects.filter(sid=self.data["sid"]) + .latest_approved() + .first() + ) + + if ( + goods_nomenclature_description + and goods_nomenclature_description.described_goods_nomenclature.valid_between.lower + ): + data[ + "validity_start" + ] = ( + goods_nomenclature_description.described_goods_nomenclature.valid_between.lower + ) + else: + data["validity_start"] = date.today() # Update the goods nomenclature description with the start date goods_nomenclature_description_handler.data.update(data) diff --git a/commodities/tests/test_importer.py b/commodities/tests/test_importer.py index 06720ce61..4b945883c 100644 --- a/commodities/tests/test_importer.py +++ b/commodities/tests/test_importer.py @@ -1,7 +1,11 @@ +from datetime import date +from unittest.mock import Mock + import pytest from commodities import models from commodities import serializers +from commodities.import_handlers import GoodsNomenclatureDescriptionHandler from common.tests import factories from common.validators import UpdateType @@ -27,6 +31,76 @@ def test_goods_nomenclature_description_importer( ) +def test_goods_nomenclature_description_update_that_comes_without_a_description_period_but_exists_in_our_db( + imported_fields_match, + date_ranges, +): + goods_nomenclature = factories.GoodsNomenclatureFactory.create( + sid=123, + valid_between=date_ranges.normal, + description=None, + ) + goods_nomenclature_description = ( + factories.GoodsNomenclatureDescriptionFactory.create( + sid=321, + validity_start=date_ranges.normal.lower, + described_goods_nomenclature=goods_nomenclature, + ) + ) + + goods_nomenclature_description_handler = { + "transaction_id": 1, + "data": { + "sid": 321, + "described_goods_nomenclature": goods_nomenclature, + }, + } + + nursary = Mock() + handler = GoodsNomenclatureDescriptionHandler( + goods_nomenclature_description_handler, + nursary, + ) + + handler.create_missing_goods_nomenclature_description_period(handler) + + assert ( + goods_nomenclature_description_handler["data"]["validity_start"] + == goods_nomenclature_description.validity_start + ) + + +def test_goods_nomenclature_description_update_that_comes_without_a_description_period_but_does_not_exist_in_our_db( + imported_fields_match, + date_ranges, +): + goods_nomenclature = factories.GoodsNomenclatureFactory.create( + sid=123, + valid_between=date_ranges.normal, + description=None, + ) + + goods_nomenclature_description_handler = { + "transaction_id": 1, + "data": { + "sid": 321, + "described_goods_nomenclature": goods_nomenclature, + }, + } + + nursary = Mock() + handler = GoodsNomenclatureDescriptionHandler( + goods_nomenclature_description_handler, + nursary, + ) + + handler.create_missing_goods_nomenclature_description_period(handler) + + assert ( + goods_nomenclature_description_handler["data"]["validity_start"] == date.today() + ) + + def test_goods_nomenclature_origin_importer( update_type, date_ranges, From 5c942412a10fd2a89a14834eea2acac67994fd82 Mon Sep 17 00:00:00 2001 From: Tash Boyse <57753415+nboyse@users.noreply.github.com> Date: Thu, 14 Sep 2023 09:15:07 +0100 Subject: [PATCH 2/5] feat: hide Import EU Taric files (#1030) --- common/forms.py | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-) diff --git a/common/forms.py b/common/forms.py index 713830908..e651f16c7 100644 --- a/common/forms.py +++ b/common/forms.py @@ -250,10 +250,13 @@ class HMRCCDSManagerActions(TextChoices): class CommonUserActions(TextChoices): SEARCH = "SEARCH", "Search the tariff" - IMPORT = "IMPORT", "Import EU Taric files" # Change this to be dependent on permissions later +class ImportUserActions(TextChoices): + IMPORT = "IMPORT", "Import EU Taric files" + + class HomeForm(forms.Form): def __init__(self, *args, **kwargs): self.user = kwargs.pop("user") @@ -272,6 +275,11 @@ def __init__(self, *args, **kwargs): choices += CommonUserActions.choices + if self.user.has_perm("common.add_trackedmodel") or self.user.has_perm( + "common.change_trackedmodel" + ): + choices += ImportUserActions.choices + self.fields["workbasket_action"] = forms.ChoiceField( label="", choices=choices, From 3fef8f6b5e4d6a6473e78b8518df5b612353dac2 Mon Sep 17 00:00:00 2001 From: Edie Pearce Date: Thu, 14 Sep 2023 11:37:48 +0100 Subject: [PATCH 3/5] Fix flaky tests (#1035) --- geo_areas/tests/bdd/conftest.py | 17 ++++++++++++++--- 1 file changed, 14 insertions(+), 3 deletions(-) diff --git a/geo_areas/tests/bdd/conftest.py b/geo_areas/tests/bdd/conftest.py index 2b9e2c518..2a9b259d1 100644 --- a/geo_areas/tests/bdd/conftest.py +++ b/geo_areas/tests/bdd/conftest.py @@ -7,17 +7,19 @@ "geographical_area 1001 with a description and area_code 0", target_fixture="geographical_area_1001", ) -def geographical_area_1001(): +def geographical_area_1001(date_ranges): area = factories.GeographicalAreaFactory(id=1001, area_id=1001, area_code=0) factories.GeographicalAreaDescriptionFactory( described_geographicalarea=area, description="This is 1001", + validity_start=date_ranges.earlier.lower, ) factories.GeographicalAreaDescriptionFactory( described_geographicalarea=factories.GeographicalMembershipFactory( member=area, ).geo_group, description="random group description", + validity_start=date_ranges.normal.lower, ) return area @@ -26,14 +28,23 @@ def geographical_area_1001(): "geographical_area 1002 with a description and area_code 1", target_fixture="geographical_area_1002", ) -def geographical_area_1002(geographical_area_1001): - area = factories.GeographicalAreaFactory(id=1002, area_id=1002, area_code=1) +def geographical_area_1002(geographical_area_1001, date_ranges): + area = factories.GeographicalAreaFactory( + id=1002, + area_id=1002, + area_code=1, + valid_between=date_ranges.big_no_end, + description=None, + ) factories.GeographicalAreaDescriptionFactory( described_geographicalarea=area, description="This is 1002", + validity_start=date_ranges.normal.lower, + transaction=area.transaction, ) factories.GeographicalMembershipFactory( member=geographical_area_1001, geo_group=area, + transaction=area.transaction, ) return area From 145db4d884bf2d729e380e3828eb68c7c0ea258d Mon Sep 17 00:00:00 2001 From: Edie Pearce Date: Thu, 14 Sep 2023 14:00:08 +0100 Subject: [PATCH 4/5] TP2000-974 Workbasket data comparison prototype (#1002) * Add workbasket comparison prototype * Hide sections if data does not exist * Find duty sentence matches * Content tweak * Tweak duty sentence filtering * Fix transaction error * Refactor * Add tests * Finish tests for utils * Add form tests * Test workbasket compare form submit * Move duty sentence parser and related fixtures into top level conftest file * Remove print statement * Standardise naming * Add tests * Content changes, fix incorrect attribute name * Use only item_id for commodity --- common/tariffs_api.py | 1 - common/tests/factories.py | 22 ++ conftest.py | 273 +++++++++++++++++ measures/tests/conftest.py | 275 ------------------ workbaskets/forms.py | 30 ++ .../includes/workbaskets/navigation.jinja | 3 + workbaskets/jinja2/workbaskets/compare.jinja | 107 +++++++ .../migrations/0008_datarow_dataupload.py | 72 +++++ workbaskets/models.py | 35 +++ workbaskets/tests/test_forms.py | 56 +++- workbaskets/tests/test_util.py | 97 ++++++ workbaskets/tests/test_views.py | 120 +++++++- workbaskets/urls.py | 5 + workbaskets/util.py | 139 +++++++++ workbaskets/views/ui.py | 118 +++++--- 15 files changed, 1010 insertions(+), 343 deletions(-) create mode 100644 workbaskets/jinja2/workbaskets/compare.jinja create mode 100644 workbaskets/migrations/0008_datarow_dataupload.py create mode 100644 workbaskets/tests/test_util.py diff --git a/common/tariffs_api.py b/common/tariffs_api.py index 1ebd6b2aa..55568e0e3 100644 --- a/common/tariffs_api.py +++ b/common/tariffs_api.py @@ -41,7 +41,6 @@ def parse_response(response): def get_commodity_data(id): url = f"{Endpoints.COMMODITIES.value}{id}" - print(url) return parse_response(requests.get(url)) diff --git a/common/tests/factories.py b/common/tests/factories.py index 278cf4b3d..b7d7c6950 100644 --- a/common/tests/factories.py +++ b/common/tests/factories.py @@ -1404,3 +1404,25 @@ class CrownDependenciesPublishingOperationalStatusFactory( class Meta: model = "publishing.CrownDependenciesPublishingOperationalStatus" + + +class DataUploadFactory(factory.django.DjangoModelFactory): + """Creates a DataUpload instance.""" + + class Meta: + model = "workbaskets.DataUpload" + + raw_data = factory.Faker("text", max_nb_chars=500) + workbasket = factory.SubFactory(WorkBasketFactory) + + +class DataRowFactory(factory.django.DjangoModelFactory): + """Creates a DataRow instance.""" + + class Meta: + model = "workbaskets.DataRow" + + data_upload = factory.SubFactory(DataUploadFactory) + commodity = factory.SubFactory(GoodsNomenclatureFactory) + duty_sentence = factory.Faker("text", max_nb_chars=24) + valid_between = date_ranges("no_end") diff --git a/conftest.py b/conftest.py index 0f69c106b..354490825 100644 --- a/conftest.py +++ b/conftest.py @@ -8,6 +8,7 @@ from typing import Dict from typing import Optional from typing import Sequence +from typing import Tuple from typing import Type from unittest.mock import patch @@ -53,10 +54,18 @@ from common.tests.util import make_duplicate_record from common.tests.util import make_non_duplicate_record from common.tests.util import raises_if +from common.validators import ApplicabilityCode from common.validators import UpdateType from importer.models import ImportBatchStatus from importer.nursery import get_nursery from importer.taric import process_taric_xml_stream +from measures.models import DutyExpression +from measures.models import MeasureConditionComponent +from measures.models import Measurement +from measures.models import MeasurementUnit +from measures.models import MeasurementUnitQualifier +from measures.models import MonetaryUnit +from measures.parsers import DutySentenceParser from workbaskets.models import WorkBasket from workbaskets.models import get_partition_scheme from workbaskets.validators import WorkflowStatus @@ -1547,3 +1556,267 @@ def empty_goods_import_batch(): status=ImportBatchStatus.SUCCEEDED, workbasket_id=archived_workbasket.id, ) + + +@pytest.fixture +def duty_sentence_parser( + duty_expressions: Dict[int, DutyExpression], + monetary_units: Dict[str, MonetaryUnit], + measurements: Dict[Tuple[str, Optional[str]], Measurement], +) -> DutySentenceParser: + return DutySentenceParser( + duty_expressions.values(), + monetary_units.values(), + measurements.values(), + ) + + +@pytest.fixture +def percent_or_amount() -> DutyExpression: + return factories.DutyExpressionFactory( + sid=1, + prefix="", + duty_amount_applicability_code=ApplicabilityCode.MANDATORY, + measurement_unit_applicability_code=ApplicabilityCode.PERMITTED, + monetary_unit_applicability_code=ApplicabilityCode.PERMITTED, + ) + + +@pytest.fixture +def plus_percent_or_amount() -> DutyExpression: + return factories.DutyExpressionFactory( + sid=4, + prefix="+", + duty_amount_applicability_code=ApplicabilityCode.MANDATORY, + measurement_unit_applicability_code=ApplicabilityCode.PERMITTED, + monetary_unit_applicability_code=ApplicabilityCode.PERMITTED, + ) + + +@pytest.fixture +def plus_agri_component() -> DutyExpression: + return factories.DutyExpressionFactory( + sid=12, + prefix="+ AC", + duty_amount_applicability_code=ApplicabilityCode.NOT_PERMITTED, + measurement_unit_applicability_code=ApplicabilityCode.PERMITTED, + monetary_unit_applicability_code=ApplicabilityCode.PERMITTED, + ) + + +@pytest.fixture +def plus_amount_only() -> DutyExpression: + return factories.DutyExpressionFactory( + sid=20, + prefix="+", + duty_amount_applicability_code=ApplicabilityCode.MANDATORY, + measurement_unit_applicability_code=ApplicabilityCode.MANDATORY, + monetary_unit_applicability_code=ApplicabilityCode.MANDATORY, + ) + + +@pytest.fixture +def nothing() -> DutyExpression: + return factories.DutyExpressionFactory( + sid=37, + prefix="NIHIL", + duty_amount_applicability_code=ApplicabilityCode.NOT_PERMITTED, + measurement_unit_applicability_code=ApplicabilityCode.NOT_PERMITTED, + monetary_unit_applicability_code=ApplicabilityCode.NOT_PERMITTED, + ) + + +@pytest.fixture +def supplementary_unit() -> DutyExpression: + return factories.DutyExpressionFactory( + sid=99, + prefix="", + duty_amount_applicability_code=ApplicabilityCode.PERMITTED, + measurement_unit_applicability_code=ApplicabilityCode.MANDATORY, + monetary_unit_applicability_code=ApplicabilityCode.NOT_PERMITTED, + ) + + +@pytest.fixture +def duty_expressions( + percent_or_amount: DutyExpression, + plus_percent_or_amount: DutyExpression, + plus_agri_component: DutyExpression, + plus_amount_only: DutyExpression, + supplementary_unit: DutyExpression, + nothing: DutyExpression, +) -> Dict[int, DutyExpression]: + return { + d.sid: d + for d in [ + percent_or_amount, + plus_percent_or_amount, + plus_agri_component, + plus_amount_only, + supplementary_unit, + nothing, + ] + } + + +@pytest.fixture +def monetary_units() -> Dict[str, MonetaryUnit]: + return { + m.code: m + for m in [ + factories.MonetaryUnitFactory(code="EUR"), + factories.MonetaryUnitFactory(code="GBP"), + factories.MonetaryUnitFactory(code="XEM"), + ] + } + + +@pytest.fixture +def measurement_units() -> Sequence[MeasurementUnit]: + return [ + factories.MeasurementUnitFactory(code="KGM", abbreviation="kg"), + factories.MeasurementUnitFactory(code="DTN", abbreviation="100 kg"), + factories.MeasurementUnitFactory(code="MIL", abbreviation="1,000 p/st"), + ] + + +@pytest.fixture +def unit_qualifiers() -> Sequence[MeasurementUnitQualifier]: + return [ + factories.MeasurementUnitQualifierFactory(code="Z", abbreviation="lactic."), + ] + + +@pytest.fixture +def measurements( + measurement_units, + unit_qualifiers, +) -> Dict[Tuple[str, Optional[str]], Measurement]: + measurements = [ + *[ + factories.MeasurementFactory( + measurement_unit=m, + measurement_unit_qualifier=None, + ) + for m in measurement_units + ], + factories.MeasurementFactory( + measurement_unit=measurement_units[1], + measurement_unit_qualifier=unit_qualifiers[0], + ), + ] + return { + ( + m.measurement_unit.code, + m.measurement_unit_qualifier.code if m.measurement_unit_qualifier else None, + ): m + for m in measurements + } + + +@pytest.fixture +def condition_duty_sentence_parser( + duty_expressions: Dict[int, DutyExpression], + monetary_units: Dict[str, MonetaryUnit], + measurements: Dict[Tuple[str, Optional[str]], Measurement], +) -> DutySentenceParser: + return DutySentenceParser( + duty_expressions.values(), + monetary_units.values(), + measurements.values(), + MeasureConditionComponent, + ) + + +@pytest.fixture +def get_component_data(duty_expressions, monetary_units, measurements) -> Callable: + def getter( + duty_expression_id, + amount, + monetary_unit_code, + measurement_codes, + ) -> Dict: + return { + "duty_expression": duty_expressions.get(duty_expression_id), + "duty_amount": amount, + "monetary_unit": monetary_units.get(monetary_unit_code), + "component_measurement": measurements.get(measurement_codes), + } + + return getter + + +@pytest.fixture( + params=( + ("4.000%", [(1, 4.0, None, None)]), + ("1.230 EUR / kg", [(1, 1.23, "EUR", ("KGM", None))]), + ("0.300 XEM / 100 kg / lactic.", [(1, 0.3, "XEM", ("DTN", "Z"))]), + ( + "12.900% + 20.000 EUR / kg", + [(1, 12.9, None, None), (4, 20.0, "EUR", ("KGM", None))], + ), + ("kg", [(99, None, None, ("KGM", None))]), + ("100 kg", [(99, None, None, ("DTN", None))]), + ("1.000 EUR", [(1, 1.0, "EUR", None)]), + ("0.000% + AC", [(1, 0.0, None, None), (12, None, None, None)]), + ), + ids=[ + "simple_ad_valorem", + "simple_specific_duty", + "unit_with_qualifier", + "multi_component_expression", + "supplementary_unit", + "supplementary_unit_with_numbers", + "monetary_unit_without_measurement", + "non_amount_expression", + ], +) +def reversible_duty_sentence_data(request, get_component_data): + """Duty sentence test cases that are syntactically correct and are also + formatted correctly.""" + expected, component_data = request.param + return expected, [get_component_data(*args) for args in component_data] + + +@pytest.fixture( + params=( + ("20.0 EUR/100kg", [(1, 20.0, "EUR", ("DTN", None))]), + ("1.0 EUR/1000 p/st", [(1, 1.0, "EUR", ("MIL", None))]), + ), + ids=[ + "parses_without_spaces", + "parses_without_commas", + ], +) +def irreversible_duty_sentence_data(request, get_component_data): + """Duty sentence test cases that are syntactically correct but are not in + the canonical rendering format with spaces and commas in the correct + places.""" + expected, component_data = request.param + 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 diff --git a/measures/tests/conftest.py b/measures/tests/conftest.py index 257fd023f..b94e9bb01 100644 --- a/measures/tests/conftest.py +++ b/measures/tests/conftest.py @@ -1,9 +1,5 @@ import datetime -from typing import Callable from typing import Dict -from typing import Optional -from typing import Sequence -from typing import Tuple import pytest import requests @@ -19,16 +15,9 @@ from geo_areas.validators import AreaCode from measures.forms import MEASURE_CONDITIONS_FORMSET_PREFIX from measures.forms import MeasureForm -from measures.models import DutyExpression from measures.models import Measure from measures.models import MeasureAction from measures.models import MeasureConditionCode -from measures.models import MeasureConditionComponent -from measures.models import Measurement -from measures.models import MeasurementUnit -from measures.models import MeasurementUnitQualifier -from measures.models import MonetaryUnit -from measures.parsers import DutySentenceParser @pytest.fixture @@ -168,94 +157,6 @@ def existing_measure(request, existing_goods_nomenclature): return factories.MeasureWithQuotaFactory.create(**data, **now), overlaps_normal -@pytest.fixture -def percent_or_amount() -> DutyExpression: - return factories.DutyExpressionFactory( - sid=1, - prefix="", - duty_amount_applicability_code=ApplicabilityCode.MANDATORY, - measurement_unit_applicability_code=ApplicabilityCode.PERMITTED, - monetary_unit_applicability_code=ApplicabilityCode.PERMITTED, - ) - - -@pytest.fixture -def plus_percent_or_amount() -> DutyExpression: - return factories.DutyExpressionFactory( - sid=4, - prefix="+", - duty_amount_applicability_code=ApplicabilityCode.MANDATORY, - measurement_unit_applicability_code=ApplicabilityCode.PERMITTED, - monetary_unit_applicability_code=ApplicabilityCode.PERMITTED, - ) - - -@pytest.fixture -def plus_agri_component() -> DutyExpression: - return factories.DutyExpressionFactory( - sid=12, - prefix="+ AC", - duty_amount_applicability_code=ApplicabilityCode.NOT_PERMITTED, - measurement_unit_applicability_code=ApplicabilityCode.PERMITTED, - monetary_unit_applicability_code=ApplicabilityCode.PERMITTED, - ) - - -@pytest.fixture -def plus_amount_only() -> DutyExpression: - return factories.DutyExpressionFactory( - sid=20, - prefix="+", - duty_amount_applicability_code=ApplicabilityCode.MANDATORY, - measurement_unit_applicability_code=ApplicabilityCode.MANDATORY, - monetary_unit_applicability_code=ApplicabilityCode.MANDATORY, - ) - - -@pytest.fixture -def nothing() -> DutyExpression: - return factories.DutyExpressionFactory( - sid=37, - prefix="NIHIL", - duty_amount_applicability_code=ApplicabilityCode.NOT_PERMITTED, - measurement_unit_applicability_code=ApplicabilityCode.NOT_PERMITTED, - monetary_unit_applicability_code=ApplicabilityCode.NOT_PERMITTED, - ) - - -@pytest.fixture -def supplementary_unit() -> DutyExpression: - return factories.DutyExpressionFactory( - sid=99, - prefix="", - duty_amount_applicability_code=ApplicabilityCode.PERMITTED, - measurement_unit_applicability_code=ApplicabilityCode.MANDATORY, - monetary_unit_applicability_code=ApplicabilityCode.NOT_PERMITTED, - ) - - -@pytest.fixture -def duty_expressions( - percent_or_amount: DutyExpression, - plus_percent_or_amount: DutyExpression, - plus_agri_component: DutyExpression, - plus_amount_only: DutyExpression, - supplementary_unit: DutyExpression, - nothing: DutyExpression, -) -> Dict[int, DutyExpression]: - return { - d.sid: d - for d in [ - percent_or_amount, - plus_percent_or_amount, - plus_agri_component, - plus_amount_only, - supplementary_unit, - nothing, - ] - } - - @pytest.fixture def condition_codes() -> Dict[str, MeasureConditionCode]: return { @@ -292,182 +193,6 @@ def certificates(): } -@pytest.fixture -def monetary_units() -> Dict[str, MonetaryUnit]: - return { - m.code: m - for m in [ - factories.MonetaryUnitFactory(code="EUR"), - factories.MonetaryUnitFactory(code="GBP"), - factories.MonetaryUnitFactory(code="XEM"), - ] - } - - -@pytest.fixture -def measurement_units() -> Sequence[MeasurementUnit]: - return [ - factories.MeasurementUnitFactory(code="KGM", abbreviation="kg"), - factories.MeasurementUnitFactory(code="DTN", abbreviation="100 kg"), - factories.MeasurementUnitFactory(code="MIL", abbreviation="1,000 p/st"), - ] - - -@pytest.fixture -def unit_qualifiers() -> Sequence[MeasurementUnitQualifier]: - return [ - factories.MeasurementUnitQualifierFactory(code="Z", abbreviation="lactic."), - ] - - -@pytest.fixture -def measurements( - measurement_units, - unit_qualifiers, -) -> Dict[Tuple[str, Optional[str]], Measurement]: - measurements = [ - *[ - factories.MeasurementFactory( - measurement_unit=m, - measurement_unit_qualifier=None, - ) - for m in measurement_units - ], - factories.MeasurementFactory( - measurement_unit=measurement_units[1], - measurement_unit_qualifier=unit_qualifiers[0], - ), - ] - return { - ( - m.measurement_unit.code, - m.measurement_unit_qualifier.code if m.measurement_unit_qualifier else None, - ): m - for m in measurements - } - - -@pytest.fixture -def duty_sentence_parser( - duty_expressions: Dict[int, DutyExpression], - monetary_units: Dict[str, MonetaryUnit], - measurements: Dict[Tuple[str, Optional[str]], Measurement], -) -> DutySentenceParser: - return DutySentenceParser( - duty_expressions.values(), - monetary_units.values(), - measurements.values(), - ) - - -@pytest.fixture -def condition_duty_sentence_parser( - duty_expressions: Dict[int, DutyExpression], - monetary_units: Dict[str, MonetaryUnit], - measurements: Dict[Tuple[str, Optional[str]], Measurement], -) -> DutySentenceParser: - return DutySentenceParser( - duty_expressions.values(), - monetary_units.values(), - measurements.values(), - MeasureConditionComponent, - ) - - -@pytest.fixture -def get_component_data(duty_expressions, monetary_units, measurements) -> Callable: - def getter( - duty_expression_id, - amount, - monetary_unit_code, - measurement_codes, - ) -> Dict: - return { - "duty_expression": duty_expressions.get(duty_expression_id), - "duty_amount": amount, - "monetary_unit": monetary_units.get(monetary_unit_code), - "component_measurement": measurements.get(measurement_codes), - } - - return getter - - -@pytest.fixture( - params=( - ("4.000%", [(1, 4.0, None, None)]), - ("1.230 EUR / kg", [(1, 1.23, "EUR", ("KGM", None))]), - ("0.300 XEM / 100 kg / lactic.", [(1, 0.3, "XEM", ("DTN", "Z"))]), - ( - "12.900% + 20.000 EUR / kg", - [(1, 12.9, None, None), (4, 20.0, "EUR", ("KGM", None))], - ), - ("kg", [(99, None, None, ("KGM", None))]), - ("100 kg", [(99, None, None, ("DTN", None))]), - ("1.000 EUR", [(1, 1.0, "EUR", None)]), - ("0.000% + AC", [(1, 0.0, None, None), (12, None, None, None)]), - ), - ids=[ - "simple_ad_valorem", - "simple_specific_duty", - "unit_with_qualifier", - "multi_component_expression", - "supplementary_unit", - "supplementary_unit_with_numbers", - "monetary_unit_without_measurement", - "non_amount_expression", - ], -) -def reversible_duty_sentence_data(request, get_component_data): - """Duty sentence test cases that are syntactically correct and are also - formatted correctly.""" - expected, component_data = request.param - return expected, [get_component_data(*args) for args in component_data] - - -@pytest.fixture( - params=( - ("20.0 EUR/100kg", [(1, 20.0, "EUR", ("DTN", None))]), - ("1.0 EUR/1000 p/st", [(1, 1.0, "EUR", ("MIL", None))]), - ), - ids=[ - "parses_without_spaces", - "parses_without_commas", - ], -) -def irreversible_duty_sentence_data(request, get_component_data): - """Duty sentence test cases that are syntactically correct but are not in - the canonical rendering format with spaces and commas in the correct - places.""" - expected, component_data = request.param - 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( diff --git a/workbaskets/forms.py b/workbaskets/forms.py index df31474d8..eaafb9ab7 100644 --- a/workbaskets/forms.py +++ b/workbaskets/forms.py @@ -9,6 +9,7 @@ from common.validators import SymbolValidator from workbaskets import models from workbaskets import validators +from workbaskets.util import serialize_uploaded_data class WorkbasketCreateForm(forms.ModelForm): @@ -150,3 +151,32 @@ def cleaned_data_no_prefix(self): SelectableObjectsForm.object_id_from_field_name(key): value for key, value in self.cleaned_data.items() } + + +class WorkbasketCompareForm(forms.Form): + data = forms.CharField( + label="Compare worksheet data against the measures in this workbasket", + widget=forms.Textarea(), + validators=[SymbolValidator], + ) + + def clean(self): + if self.cleaned_data: + serialized = serialize_uploaded_data(self.cleaned_data["data"]) + return {"data": serialized, "raw_data": self.cleaned_data["data"]} + + def __init__(self, *args, **kwargs): + super().__init__(*args, **kwargs) + + self.helper = FormHelper(self) + self.helper.label_size = Size.SMALL + self.helper.legend_size = Size.SMALL + self.helper.layout = Layout( + Field.textarea("data", rows=10), + Submit( + "submit", + "Compare", + data_module="govuk-button--secondary", + data_prevent_double_click="true", + ), + ) diff --git a/workbaskets/jinja2/includes/workbaskets/navigation.jinja b/workbaskets/jinja2/includes/workbaskets/navigation.jinja index 67935a359..1817982a9 100644 --- a/workbaskets/jinja2/includes/workbaskets/navigation.jinja +++ b/workbaskets/jinja2/includes/workbaskets/navigation.jinja @@ -16,6 +16,9 @@
  • Review goods
    • +
  • + Worksheet check +
    • {% endmacro %} \ No newline at end of file diff --git a/workbaskets/jinja2/workbaskets/compare.jinja b/workbaskets/jinja2/workbaskets/compare.jinja new file mode 100644 index 000000000..683b964f6 --- /dev/null +++ b/workbaskets/jinja2/workbaskets/compare.jinja @@ -0,0 +1,107 @@ +{% extends "layouts/form.jinja" %} + +{% from 'macros/create_link.jinja' import create_link %} +{% from 'macros/footnotes_display.jinja' import footnotes_display %} +{% from "includes/measures/conditions.jinja" import conditions_list %} +{% from "components/breadcrumbs/macro.njk" import govukBreadcrumbs %} +{% from "components/button/macro.njk" import govukButton %} +{% from "includes/workbaskets/navigation.jinja" import navigation %} +{% from "components/table/macro.njk" import govukTable %} + +{% set page_title %} + Workbasket {{ workbasket.id if workbasket else request.session.workbasket.id }} - Summary +{% endset %} + +{% set change_workbasket_details_link = url("workbaskets:workbasket-ui-update", kwargs={"pk": workbasket.pk}) %} + +{% block breadcrumb %} + {{ govukBreadcrumbs({ + "items": [ + {"text": "Home", "href": url("home")}, + {"text": "Edit an existing workbasket", "href": url("workbaskets:workbasket-ui-list")}, + {"text": "Workbasket " ~ request.session.workbasket.id ~ " - Summary" } + ]}) + }} +{% endblock %} + +{% block content %} +

    + {{ page_title }} +

    + + {{ navigation(request, "compare") }} + + {% call django_form() %} + {{ crispy(form) }} + {% endcall %} + + {% if data_upload %} +

    Worksheet data

    + + {% set table_rows = [] %} + {% for row in data_upload.serialized %} + {{ table_rows.append([ + {"text": row.commodity if row.commodity else "—"}, + {"text": row.duty_sentence if row.duty_sentence else "—" }, + {"text": "{:%d %b %Y}".format(row.valid_between.lower) }, + {"text": "{:%d %b %Y}".format(row.valid_between.upper) if row.valid_between.upper else "—" }, + ]) or "" }} + {% endfor %} + + {{ govukTable({ + "head": [ + {"text": "Commodity code"}, + {"text": "Duties"}, + {"text": "Start date"}, + {"text": "End date"}, + ], + "rows": table_rows, + "classes": "govuk-table-m" + }) }} + + {% endif %} + + {% if matching_measures %} +

    {{ matching_measures|length }} matching measure{{ matching_measures|pluralize }} found

    + + {% set table_rows = [] %} + {% for measure in matching_measures %} + {% set measure_link -%} + {{measure.sid}} + {%- endset %} + {{ table_rows.append([ + {"html": measure_link}, + {"text": measure.measure_type.sid ~ " - " ~ measure.measure_type.description}, + {"text": create_link(url("commodity-ui-detail", kwargs={"sid": measure.goods_nomenclature.sid}), measure.goods_nomenclature.item_id) if measure.goods_nomenclature else '-', "classes": "govuk-!-width-one-eighth"}, + {"text": measure.duty_sentence if measure.duty_sentence else '-'}, + {"text": "{:%d %b %Y}".format(measure.valid_between.lower) }, + {"text": "{:%d %b %Y}".format(measure.effective_end_date) if measure.effective_end_date else "-" }, + {"html": create_link(url("additional_code-ui-detail", kwargs={"sid": measure.additional_code.sid}), measure.additional_code.type.sid ~ measure.additional_code.code) if measure.additional_code else '-'}, + {"html": create_link(url("geo_area-ui-detail", kwargs={"sid": measure.geographical_area.sid}), measure.geographical_area.area_id ~ " - " ~ measure.geographical_area.get_description().description) if measure.geographical_area else '-'}, + {"text": create_link(measure.order_number.get_url(), measure.order_number.order_number) if measure.order_number else '-'}, + {"text": footnotes_display(measure.footnoteassociationmeasure_set.current())}, + {"text": conditions_list(measure) if measure.conditions.current() else "-"}, + ]) or "" }} + {% endfor %} + + {{ govukTable({ + "head": [ + {"text": "Measure SID"}, + {"text": "Measure type"}, + {"text": "Commodity code"}, + {"text": "Duties"}, + {"text": "Start date"}, + {"text": "End date"}, + {"text": "Additional code"}, + {"text": "Geographical area"}, + {"text": "Quota"}, + {"text": "Footnote"}, + {"text": "Conditions"}, + ], + "rows": table_rows, + "classes": "govuk-table-m" + }) }} + + {% endif %} + +{% endblock %} diff --git a/workbaskets/migrations/0008_datarow_dataupload.py b/workbaskets/migrations/0008_datarow_dataupload.py new file mode 100644 index 000000000..02d74ee3b --- /dev/null +++ b/workbaskets/migrations/0008_datarow_dataupload.py @@ -0,0 +1,72 @@ +# Generated by Django 3.2.20 on 2023-09-12 15:37 + +import django.db.models.deletion +from django.db import migrations +from django.db import models + +import common.fields + + +class Migration(migrations.Migration): + dependencies = [ + ("workbaskets", "0007_alter_workbasket_options"), + ] + + operations = [ + migrations.CreateModel( + name="DataUpload", + fields=[ + ( + "id", + models.AutoField( + auto_created=True, + primary_key=True, + serialize=False, + verbose_name="ID", + ), + ), + ("raw_data", models.TextField()), + ( + "workbasket", + models.ForeignKey( + editable=False, + null=True, + on_delete=django.db.models.deletion.CASCADE, + to="workbaskets.workbasket", + ), + ), + ], + ), + migrations.CreateModel( + name="DataRow", + fields=[ + ( + "id", + models.AutoField( + auto_created=True, + primary_key=True, + serialize=False, + verbose_name="ID", + ), + ), + ("valid_between", common.fields.TaricDateRangeField(db_index=True)), + ("commodity", models.CharField(blank=True, max_length=255, null=True)), + ( + "duty_sentence", + models.CharField(blank=True, max_length=255, null=True), + ), + ( + "data_upload", + models.ForeignKey( + null=True, + on_delete=django.db.models.deletion.CASCADE, + related_name="rows", + to="workbaskets.dataupload", + ), + ), + ], + options={ + "abstract": False, + }, + ), + ] diff --git a/workbaskets/models.py b/workbaskets/models.py index b3ae6a103..aa6372f2f 100644 --- a/workbaskets/models.py +++ b/workbaskets/models.py @@ -19,6 +19,7 @@ from checks.models import TrackedModelCheck from checks.models import TransactionCheck from common.models.mixins import TimestampedMixin +from common.models.mixins.validity import ValidityMixin from common.models.tracked_qs import TrackedModelQuerySet from common.models.trackedmodel import TrackedModel from common.models.transactions import Transaction @@ -26,6 +27,7 @@ from common.models.transactions import TransactionQueryset from measures.models import Measure from measures.querysets import MeasuresQuerySet +from workbaskets.util import serialize_uploaded_data from workbaskets.validators import WorkflowStatus logger = logging.getLogger(__name__) @@ -625,3 +627,36 @@ def unchecked_or_errored_transactions(self): class Meta: verbose_name = "workbasket" verbose_name_plural = "workbaskets" + + +class DataUpload(models.Model): + raw_data = models.TextField() + workbasket = models.ForeignKey( + WorkBasket, + on_delete=models.CASCADE, + editable=False, + null=True, + ) + + @property + def serialized(self): + return serialize_uploaded_data(self.raw_data) + + +class DataRow(ValidityMixin, models.Model): + data_upload = models.ForeignKey( + DataUpload, + on_delete=models.CASCADE, + null=True, + related_name="rows", + ) + commodity = models.CharField( + max_length=255, + null=True, + blank=True, + ) + duty_sentence = models.CharField( + max_length=255, + null=True, + blank=True, + ) diff --git a/workbaskets/tests/test_forms.py b/workbaskets/tests/test_forms.py index 6906198a0..64dadc4df 100644 --- a/workbaskets/tests/test_forms.py +++ b/workbaskets/tests/test_forms.py @@ -1,9 +1,7 @@ import pytest from common.tests import factories -from workbaskets.forms import SelectableObjectField -from workbaskets.forms import SelectableObjectsForm -from workbaskets.forms import WorkbasketCreateForm +from workbaskets import forms from workbaskets.validators import tops_jira_number_validator pytestmark = pytest.mark.django_db @@ -12,7 +10,7 @@ def test_workbasket_create_form_valid_data(): """Test that WorkbasketCreateForm is valid when required fields in data.""" data = {"title": "123", "reason": "testing testing"} - form = WorkbasketCreateForm(data=data) + form = forms.WorkbasketCreateForm(data=data) assert form.is_valid() @@ -21,17 +19,17 @@ def test_workbasket_create_form_invalid_data(): """Test that WorkbasketCreateForm is not valid when required fields not in data.""" - form = WorkbasketCreateForm(data={}) + form = forms.WorkbasketCreateForm(data={}) assert not form.is_valid() assert "This field is required." in form.errors["title"] assert "This field is required." in form.errors["reason"] - form = WorkbasketCreateForm(data={"title": "abc", "reason": "test"}) + form = forms.WorkbasketCreateForm(data={"title": "abc", "reason": "test"}) assert not form.is_valid() assert tops_jira_number_validator.message in form.errors["title"] factories.WorkBasketFactory(title="123321") - form = WorkbasketCreateForm(data={"title": "123321", "reason": "test"}) + form = forms.WorkbasketCreateForm(data={"title": "123321", "reason": "test"}) assert not form.is_valid() assert "Workbasket with this Title already exists." in form.errors["title"] @@ -41,7 +39,7 @@ def test_selectable_objects_form(): each item passed to it, and the class methods and properties provide the expected data for the item.""" measures = factories.MeasureFactory.create_batch(5) - form = SelectableObjectsForm(initial={}, prefix=None, objects=measures) + form = forms.SelectableObjectsForm(initial={}, prefix=None, objects=measures) # grab a list of the ids tacked on to the end of the field name, as this # should be the pk of the measure. form_fields_keys = [str.partition("_")[2] for str in form.fields.keys()] @@ -55,18 +53,23 @@ def test_selectable_objects_form(): for item in measures: # Check that the field name class method returns the right name. assert ( - SelectableObjectsForm.field_name_for_object(measures[measures.index(item)]) + forms.SelectableObjectsForm.field_name_for_object( + measures[measures.index(item)], + ) == f"selectableobject_{item.pk}" ) # Check that the id class method returns the right id. assert ( - SelectableObjectsForm.object_id_from_field_name( + forms.SelectableObjectsForm.object_id_from_field_name( list(form.fields.keys())[measures.index(item)], ) == measure_pks[measures.index(item)] ) # Check that the fields are of SelectableObjectField type - assert type(form.fields[f"selectableobject_{item.pk}"]) is SelectableObjectField + assert ( + type(form.fields[f"selectableobject_{item.pk}"]) + is forms.SelectableObjectField + ) # Check that the cleaned_data_no_prefix property returns a list of the data with no prefixes. form.cleaned_data = { @@ -87,4 +90,33 @@ def test_selectable_objects_form(): } # Check that the fields are of SelectableObjectField type for item in measures: - assert type(form.fields[f"selectableobject_{item.pk}"]) is SelectableObjectField + assert ( + type(form.fields[f"selectableobject_{item.pk}"]) + is forms.SelectableObjectField + ) + + +def test_workbasket_compare_form_valid(): + data = { + "data": ( + "2909500090\t0.000% + 2.000 GBP / 100 kg\t20/05/2021\t31/08/2024\n" + "2909500090\t0.000%\t\t31/08/2024\n" + "3945875\tfoo bar\t438573\t\n" # line with nonsense data + ), + } + form = forms.WorkbasketCompareForm(data=data) + assert form.is_valid() + assert form.cleaned_data + + +def test_workbasket_compare_form_invalid(): + data = { + "data": ( + "2909500090\t0.000% + 2.000 GBP / 100 kg\t20/05/2021\t31/08/2024\n" + "290950<>0090\t0.000%\t\t31/08/2024\n" + "3945875\tfoo bar\t438573\t\n" # line with nonsense data + ), + } + form = forms.WorkbasketCompareForm(data=data) + assert not form.is_valid() + assert "Only symbols" in form.errors["data"][0] diff --git a/workbaskets/tests/test_util.py b/workbaskets/tests/test_util.py new file mode 100644 index 000000000..04a629446 --- /dev/null +++ b/workbaskets/tests/test_util.py @@ -0,0 +1,97 @@ +from datetime import date + +import pytest + +from common.tests import factories +from common.util import TaricDateRange +from workbaskets.util import TableRow +from workbaskets.util import assign_validity +from workbaskets.util import find_comm_code +from workbaskets.util import find_date +from workbaskets.util import get_delimiter +from workbaskets.util import parse_dates +from workbaskets.util import serialize_uploaded_data + + +@pytest.mark.parametrize( + "date_string,exp", + [ + ("30-12-2020", "-"), + ("30/12/2020", "/"), + ("30+12+2020", None), + ], +) +def test_get_date_delimiter(date_string, exp): + delimiter = get_delimiter(date_string) + assert delimiter == exp + + +@pytest.mark.django_db +def test_get_comm_code(): + created_commodity = factories.GoodsNomenclatureFactory() + row_data = TableRow() + find_comm_code(created_commodity.item_id, row_data) + assert row_data.commodity == created_commodity.item_id + + +@pytest.mark.parametrize( + "cell,exp", + [ + ("12-05-2020", "12-05-2020"), + ("12230984", None), + ("04-04", None), + ("-----", None), + ], +) +def test_find_dates(cell, exp): + found_date = find_date(cell) + assert found_date == exp + + +@pytest.mark.parametrize( + "dates", + [ + (["01-01-2020", "01-01-2000"]), + (["2020-01-01", "2000-01-01"]), + (["2020-01-01", "01-01-2000"]), + ], +) +def test_parse_dates(dates): + assert set(parse_dates(dates)) == {date(2020, 1, 1), date(2000, 1, 1)} + + +@pytest.mark.parametrize( + "dates", + [ + (["01-01-2020", "01-01-2000"]), + (["01-01-2000", "01-01-2020"]), + ], +) +def test_assign_validity(dates): + row_data = TableRow() + assign_validity(dates, row_data) + assert row_data.valid_between == TaricDateRange(date(2000, 1, 1), date(2020, 1, 1)) + + +@pytest.mark.django_db +def test_serialize_uploaded_data(): + commodity1 = factories.GoodsNomenclatureFactory() + commodity2 = factories.GoodsNomenclatureFactory() + + # format of copypasted data from excel + # each cell is separated by a \t tab character + # at the moment we only match measures against comm code, duty, and start/end dates + raw_data = ( + f"{commodity1.item_id}\t0.000% + 2.000 GBP / 100 kg\t20/05/2021\t31/08/2024\n" + f"{commodity2.item_id}\t0.000%\t\t31/08/2024\n" + "3945875\tfoo bar\t438573\t\n" # line with nonsense data + ) + serialized = serialize_uploaded_data(raw_data) + assert len(serialized) == 2 + assert serialized[0].valid_between == TaricDateRange( + date(2021, 5, 20), + date(2024, 8, 31), + ) + assert serialized[0].commodity == commodity1.item_id + assert serialized[1].valid_between == TaricDateRange(date(2024, 8, 31), None) + assert serialized[1].commodity == commodity2.item_id diff --git a/workbaskets/tests/test_views.py b/workbaskets/tests/test_views.py index 6a8559f80..5ecfc3fcb 100644 --- a/workbaskets/tests/test_views.py +++ b/workbaskets/tests/test_views.py @@ -12,9 +12,6 @@ from checks.tests.factories import TrackedModelCheckFactory from common.models.utils import override_current_transaction from common.tests import factories -from common.tests.factories import GeographicalAreaFactory -from common.tests.factories import GoodsNomenclatureFactory -from common.tests.factories import MeasureFactory from exporter.tasks import upload_workbaskets from measures.models import Measure from workbaskets import models @@ -165,7 +162,7 @@ def test_review_workbasket_displays_objects_in_current_workbasket( selection form of the review workbasket page.""" with session_workbasket.new_transaction(): - GoodsNomenclatureFactory.create() + factories.GoodsNomenclatureFactory.create() response = valid_user_client.get( reverse( @@ -189,7 +186,7 @@ def test_review_workbasket_displays_rule_violation_summary( detailing the number of tracked model changes and business rule violations, dated to the most recent `TrackedModelCheck`.""" with session_workbasket.new_transaction() as transaction: - good = GoodsNomenclatureFactory.create(transaction=transaction) + good = factories.GoodsNomenclatureFactory.create(transaction=transaction) check = TrackedModelCheckFactory.create( transaction_check__transaction=transaction, model=good, @@ -555,7 +552,7 @@ def test_run_business_rules(check_workbasket, valid_user_client, session_workbas assert not session_workbasket.rule_check_task_id with session_workbasket.new_transaction() as transaction: - good = GoodsNomenclatureFactory.create(transaction=transaction) + good = factories.GoodsNomenclatureFactory.create(transaction=transaction) check = TrackedModelCheckFactory.create( transaction_check__transaction=transaction, model=good, @@ -630,9 +627,9 @@ def test_submit_for_packaging(valid_user_client, session_workbasket): """Test that a GET request to the submit-for-packaging endpoint returns a 302, redirecting to the create packaged workbasket page.""" with session_workbasket.new_transaction() as transaction: - good = GoodsNomenclatureFactory.create(transaction=transaction) - measure = MeasureFactory.create(transaction=transaction) - geo_area = GeographicalAreaFactory.create(transaction=transaction) + good = factories.GoodsNomenclatureFactory.create(transaction=transaction) + measure = factories.MeasureFactory.create(transaction=transaction) + geo_area = factories.GeographicalAreaFactory.create(transaction=transaction) objects = [good, measure, geo_area] for obj in objects: TrackedModelCheckFactory.create( @@ -689,7 +686,7 @@ def test_workbasket_violations(valid_user_client, session_workbasket): "workbaskets:workbasket-ui-violations", ) with session_workbasket.new_transaction() as transaction: - good = GoodsNomenclatureFactory.create(transaction=transaction) + good = factories.GoodsNomenclatureFactory.create(transaction=transaction) check = TrackedModelCheckFactory.create( transaction_check__transaction=transaction, model=good, @@ -721,7 +718,7 @@ def test_workbasket_violations(valid_user_client, session_workbasket): def test_violation_detail_page(valid_user_client, session_workbasket): with session_workbasket.new_transaction() as transaction: - good = GoodsNomenclatureFactory.create(transaction=transaction) + good = factories.GoodsNomenclatureFactory.create(transaction=transaction) check = TrackedModelCheckFactory.create( transaction_check__transaction=transaction, model=good, @@ -864,9 +861,9 @@ def test_violation_detail_page_non_superuser_override_violation( @pytest.fixture def setup(session_workbasket, valid_user_client): with session_workbasket.new_transaction() as transaction: - good = GoodsNomenclatureFactory.create(transaction=transaction) - measure = MeasureFactory.create(transaction=transaction) - geo_area = GeographicalAreaFactory.create(transaction=transaction) + good = factories.GoodsNomenclatureFactory.create(transaction=transaction) + measure = factories.MeasureFactory.create(transaction=transaction) + geo_area = factories.GeographicalAreaFactory.create(transaction=transaction) regulation = factories.RegulationFactory.create(transaction=transaction) additional_code = factories.AdditionalCodeFactory.create( transaction=transaction, @@ -1193,3 +1190,98 @@ def test_application_access_after_workbasket_delete( # workbasket deletion. assert response.status_code == 200 assert not page.select("header nav a.workbasket-link") + + +def test_workbasket_compare_200(valid_user_client, session_workbasket): + url = reverse("workbaskets:workbasket-ui-compare") + response = valid_user_client.get(url) + assert response.status_code == 200 + + +def test_workbasket_compare_prev_uploaded(valid_user_client, session_workbasket): + factories.GoodsNomenclatureFactory() + factories.GoodsNomenclatureFactory() + factories.DataUploadFactory(workbasket=session_workbasket) + url = reverse("workbaskets:workbasket-ui-compare") + response = valid_user_client.get(url) + assert "Worksheet data" in response.content.decode(response.charset) + + +def test_workbasket_update_prev_uploaded(valid_user_client, session_workbasket): + factories.GoodsNomenclatureFactory() + factories.GoodsNomenclatureFactory() + data_upload = factories.DataUploadFactory(workbasket=session_workbasket) + url = reverse("workbaskets:workbasket-ui-compare") + data = { + "data": ( + "0000000001\t1.000%\t20/05/2021\t31/08/2024\n" + "0000000002\t2.000%\t20/05/2021\t31/08/2024" + ), + } + response = valid_user_client.post(url, data) + assert response.status_code == 302 + data_upload.refresh_from_db() + assert data_upload.raw_data == data["data"] + + +def test_workbasket_compare_form_submit_302(valid_user_client, session_workbasket): + url = reverse("workbaskets:workbasket-ui-compare") + data = { + "data": ( + "0000000001\t1.000%\t20/05/2021\t31/08/2024\n" + "0000000002\t2.000%\t20/05/2021\t31/08/2024\n" + ), + } + response = valid_user_client.post(url, data) + assert response.status_code == 302 + assert response.url == url + + +def test_workbasket_compare_found_measures( + valid_user_client, + session_workbasket, + date_ranges, + duty_sentence_parser, + percent_or_amount, +): + commodity = factories.GoodsNomenclatureFactory() + + with session_workbasket.new_transaction(): + measure = factories.MeasureFactory( + valid_between=date_ranges.normal, + goods_nomenclature=commodity, + ) + duty_string = "4.000%" + # create measure components equivalent to a duty sentence of "4.000%" + factories.MeasureComponentFactory.create( + component_measure=measure, + duty_expression=percent_or_amount, + duty_amount=4.0, + monetary_unit=None, + component_measurement=None, + ) + + url = reverse("workbaskets:workbasket-ui-compare") + data = { + "data": ( + # this first line should match the measure in the workbasket + f"{commodity.item_id}\t{duty_string}\t{date_ranges.normal.lower.isoformat()}\t{date_ranges.normal.upper.isoformat()}\n" + "0000000002\t2.000%\t20/05/2021\t31/08/2024\n" + ), + } + response = valid_user_client.post(url, data) + assert response.status_code == 302 + assert response.url == url + + # view the uploaded data + response2 = valid_user_client.get(response.url) + assert response2.status_code == 200 + decoded = response2.content.decode(response2.charset) + soup = BeautifulSoup(decoded, "html.parser") + assert "1 matching measure found" in soup.select("h2")[1].text + + # previously uploaded data + assert len(soup.select("tbody")[0].select("tr")) == 2 + + # measure found + assert len(soup.select("tbody")[1].select("tr")) == 1 diff --git a/workbaskets/urls.py b/workbaskets/urls.py index 1c3fd1fed..472294a9b 100644 --- a/workbaskets/urls.py +++ b/workbaskets/urls.py @@ -81,6 +81,11 @@ ui_views.WorkBasketDeleteChangesDone.as_view(), name="workbasket-ui-delete-changes-done", ), + path( + f"compare/", + ui_views.WorkBasketCompare.as_view(), + name="workbasket-ui-compare", + ), path( f"/changes/", ui_views.WorkBasketChanges.as_view(), diff --git a/workbaskets/util.py b/workbaskets/util.py index 6f2f2f279..adeee825a 100644 --- a/workbaskets/util.py +++ b/workbaskets/util.py @@ -1,5 +1,11 @@ +import re +from datetime import date + from django.db import transaction +from parsec import ParseError +from commodities.validators import ITEM_ID_REGEX +from common.util import TaricDateRange from importer.nursery import get_nursery @@ -41,3 +47,136 @@ def delete_workbasket(workbasket): itself.""" clear_workbasket(workbasket) workbasket.delete() + + +# permits both date formats +# YYYY-MM-DD +# DD-MM-YYYY +# and either - or / separator +DATE_REGEX = ( + r"([0-9]{2}(\/|-)[0-9]{2}(\/|-)[0-9]{4})|([0-9]{4}(\/|-)[0-9]{2}(\/|-)[0-9]{2})" +) + + +class TableRow: + def __init__(self, commodity=None, valid_between=None, duty_sentence=None): + self.commodity = commodity + self.valid_between = valid_between + self.duty_sentence = duty_sentence + + @property + def all_none(self): + return not any([self.commodity, self.valid_between, self.duty_sentence]) + + +def find_comm_code(cell, row_data): + matches = re.compile(ITEM_ID_REGEX).match(cell) + if matches: + row_data.commodity = cell + return True + return False + + +def find_date(cell): + matches = re.compile(DATE_REGEX).match(cell) + if matches: + return cell + + +def find_duty_sentence(cell, row_data): + from measures.parsers import DutySentenceParser + + # because we may not know the measure validity period, take today's date instead + # we only need to look for something that looks like a duty sentence, not necessarily a valid one + duty_sentence_parser = DutySentenceParser.create( + date.today(), + ) + duty_sentence = cell.replace(" ", "") + try: + duty_sentence_parser.parse(duty_sentence) + row_data.duty_sentence = cell + return True + except ParseError: + return False + + +def get_delimiter(date_string): + if "/" in date_string: + return "/" + elif "-" in date_string: + return "-" + return None + + +def parse_dates(dates): + parsed_dates = [] + for date_string in dates: + delimiter = get_delimiter(date_string) + components = [int(component) for component in date_string.split(delimiter)] + + # year first + if components[0] > 1000: + parsed_dates.append( + date(components[0], components[1], components[2]), + ) + # year last + elif components[-1] > 1000: + parsed_dates.append( + date(components[2], components[1], components[0]), + ) + return parsed_dates + + +def assign_validity(dates, row_data): + parsed_dates = parse_dates(dates) + + # assume start date for an open-ended measure + if len(parsed_dates) == 1: + row_data.valid_between = TaricDateRange( + parsed_dates[0], + None, + ) + elif len(parsed_dates) == 2: + if parsed_dates[0] > parsed_dates[1]: + row_data.valid_between = TaricDateRange( + parsed_dates[1], + parsed_dates[0], + ) + elif parsed_dates[1] > parsed_dates[0]: + row_data.valid_between = TaricDateRange( + parsed_dates[0], + parsed_dates[1], + ) + + +def serialize_uploaded_data(data): + serialized = [] + rows = data.strip().split("\n") + table = [row.strip().split("\t") for row in rows] + + for row in table: + row_data = TableRow() + dates = [] + for cell in row: + if not cell: + continue + + commodity = find_comm_code(cell, row_data) + if commodity: + continue + + found_date = find_date(cell) + if found_date: + dates.append(found_date) + continue + + duty_sentence = find_duty_sentence(cell, row_data) + if duty_sentence: + continue + + assign_validity(dates, row_data) + + if not row_data.all_none: + serialized.append(row_data) + + return serialized diff --git a/workbaskets/views/ui.py b/workbaskets/views/ui.py index 9f67951ca..66c1d3729 100644 --- a/workbaskets/views/ui.py +++ b/workbaskets/views/ui.py @@ -39,6 +39,8 @@ from measures.filters import MeasureFilter from measures.models import Measure from workbaskets import forms +from workbaskets.models import DataRow +from workbaskets.models import DataUpload from workbaskets.models import WorkBasket from workbaskets.session_store import SessionStore from workbaskets.tasks import call_check_workbasket_sync @@ -369,6 +371,7 @@ class CurrentWorkBasket(FormView): "remove-all": "workbaskets:workbasket-ui-delete-changes", "page-prev": "workbaskets:current-workbasket", "page-next": "workbaskets:current-workbasket", + "compare-data": "workbaskets:current-workbasket", } @property @@ -409,20 +412,6 @@ def _append_url_page_param(self, url, form_action): 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) - @atomic def run_business_rules(self): """Remove old checks, start new checks via a Celery task and save the @@ -442,42 +431,19 @@ def run_business_rules(self): def get_success_url(self): form_action = self.request.POST.get("form-action") - if form_action == "remove-selected": - return reverse( - self.action_success_url_names[form_action], - ) - elif form_action == "remove-all": - return reverse( - self.action_success_url_names[form_action], - ) - elif form_action in ("page-prev", "page-next"): - return self._append_url_page_param( - reverse( - self.action_success_url_names[form_action], - ), - form_action, - ) - elif form_action == "run-business-rules": + if form_action == "run-business-rules": self.run_business_rules() - return self._append_url_page_param( - reverse( - self.action_success_url_names[form_action], - ), - form_action, - ) - elif form_action == "submit-for-packaging": - return reverse( - self.action_success_url_names[form_action], - ) elif form_action == "terminate-rule-check": self.workbasket.terminate_rule_check() + try: return self._append_url_page_param( reverse( self.action_success_url_names[form_action], ), form_action, ) - return reverse("home") + except KeyError: + return reverse("home") def get_initial(self): store = SessionStore( @@ -716,3 +682,73 @@ def get_context_data(self, **kwargs): context_data = super().get_context_data(**kwargs) context_data["deleted_pk"] = self.kwargs["deleted_pk"] return context_data + + +class WorkBasketCompare(WithCurrentWorkBasket, FormView): + success_url = reverse_lazy("workbaskets:workbasket-ui-compare") + template_name = "workbaskets/compare.jinja" + form_class = forms.WorkbasketCompareForm + + @property + def workbasket_measures(self): + return self.workbasket.measures.all() + + @property + def data_upload(self): + try: + return DataUpload.objects.get(workbasket=self.workbasket) + except DataUpload.DoesNotExist: + return None + + def form_valid(self, form): + try: + existing = DataUpload.objects.get(workbasket=self.workbasket) + existing.raw_data = form.cleaned_data["raw_data"] + existing.rows.all().delete() + for row in form.cleaned_data["data"]: + DataRow.objects.create( + valid_between=row.valid_between, + duty_sentence=row.duty_sentence, + commodity=row.commodity, + data_upload=existing, + ) + existing.save() + except DataUpload.DoesNotExist: + data_upload = DataUpload.objects.create( + raw_data=form.cleaned_data["raw_data"], + workbasket=self.workbasket, + ) + for row in form.cleaned_data["data"]: + DataRow.objects.create( + valid_between=row.valid_between, + duty_sentence=row.duty_sentence, + commodity=row.commodity, + data_upload=data_upload, + ) + return super().form_valid(form) + + @property + def matching_measures(self): + measures = [] + if self.data_upload: + for row in self.data_upload.rows.all(): + matches = self.workbasket_measures.filter( + valid_between=row.valid_between, + goods_nomenclature__item_id=row.commodity, + ) + duty_matches = [ + measure + for measure in matches + if measure.duty_sentence == row.duty_sentence + ] + measures += duty_matches + return measures + + def get_context_data(self, *args, **kwargs): + return super().get_context_data( + workbasket=self.workbasket, + data_upload=self.data_upload, + matching_measures=self.matching_measures, + *args, + **kwargs, + ) From 98262d672b149703b7ffe5447c251c9fcf1422ff Mon Sep 17 00:00:00 2001 From: Lauren Qurashi <46787754+LaurenQurashi@users.noreply.github.com> Date: Fri, 15 Sep 2023 10:41:41 +0100 Subject: [PATCH 5/5] TP2000-959 measure list pagination fix (#1026) * Fix pagination and refactor template * Add comment * Increase per page number * Refactor context --- measures/jinja2/measures/list.jinja | 117 +++++++++++++++++++++++++++- measures/views.py | 34 +++++++- 2 files changed, 148 insertions(+), 3 deletions(-) diff --git a/measures/jinja2/measures/list.jinja b/measures/jinja2/measures/list.jinja index 40085e0ef..3c79c71f5 100644 --- a/measures/jinja2/measures/list.jinja +++ b/measures/jinja2/measures/list.jinja @@ -1,4 +1,119 @@ +{% extends "layouts/layout.jinja" %} + +{% from "components/button/macro.njk" import govukButton %} +{% from "components/input/macro.njk" import govukInput %} +{% from "components/table/macro.njk" import govukTable %} + {% set form_url = "measure-ui-list" %} {% set list_include = "includes/measures/list.jinja" %} -{%- include "layouts/list_vertical.jinja" -%} +{% set page_title = "Find and edit " ~ object_list.model._meta.verbose_name_plural %} + + +{% block content %} +

    {{ page_title }}

    + +
    +
    +

    Search and filter

    +

    + Select one or more options to search +

    +
    + {{ crispy(filter.form) }} +
    +
    + + +
    + {% if results_count > 0 %} + + {% set objects_count = '{0:,}'.format(results_count) %} + {% if results_limit_breached %} + {% set objects_count = objects_count ~ '+' %} + {% endif %} + +

    + {{ objects_count }} results +

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

    There are no results for your search, please:

    +
      +
    • check the spelling of your keywords
      • +
    • use more general keywords
      • +
    • select or deselect different filters
      • +
    • get help via our support and feedback form
      • +
    + {% endif %} + + {% if has_other_pages %} + {% set objects_count = '{0:,}'.format(results_count) %} + {% set page_count = '{0:,}'.format(page_count) %} + {% if results_limit_breached %} + {% set objects_count = 'more than ' ~ objects_count %} + {% set page_count = 'more than ' ~ page_count %} + {% endif %} + + + {% endif %} + +
    +
    +{% endblock %} diff --git a/measures/views.py b/measures/views.py index 848938a88..71531e6ec 100644 --- a/measures/views.py +++ b/measures/views.py @@ -27,6 +27,7 @@ from common.forms import unprefix_formset_data from common.models import TrackedModel +from common.pagination import build_pagination_list from common.serializers import AutoCompleteSerializer from common.util import TaricDateRange from common.validators import UpdateType @@ -150,10 +151,39 @@ def get_form_kwargs(self): def paginator(self): filterset_class = self.get_filterset_class() self.filterset = self.get_filterset(filterset_class) - return MeasurePaginator(self.filterset.qs, per_page=20) + return MeasurePaginator(self.filterset.qs, per_page=40) def get_context_data(self, **kwargs): - context = super().get_context_data(**kwargs) + # References to page or pagination in the template were heavily increasing load time. By setting everything we need in the context, + # we can reduce load time + page = self.paginator.get_page(self.request.GET.get("page", 1)) + context = {} + context.update( + { + "filter": kwargs["filter"], + "form": self.get_form(), + "view": self, + "is_paginated": True, + "results_count": self.paginator.count, + "results_limit_breached": self.paginator.limit_breached, + "page_count": self.paginator.num_pages, + "has_other_pages": page.has_other_pages(), + "has_previous_page": page.has_previous(), + "has_next_page": page.has_next(), + "page_number": page.number, + "list_items_count": self.paginator.per_page, + "object_list": page.object_list, + "page_links": build_pagination_list( + page.number, + page.paginator.num_pages, + ), + }, + ) + if context["has_previous_page"]: + context["prev_page_number"] = page.previous_page_number() + if context["has_next_page"]: + context["next_page_number"] = page.next_page_number() + measure_selections = [ SelectableObjectsForm.object_id_from_field_name(name) for name in self.measure_selections