Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

TP2000-1478: Missing measures check #1359

Merged
merged 17 commits into from
Jan 16, 2025
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
@@ -0,0 +1,75 @@
# Generated by Django 4.2.16 on 2024-12-12 15:14

import django.db.models.deletion
from django.db import migrations
from django.db import models


class Migration(migrations.Migration):

dependencies = [
("commodities", "0013_alter_goodsnomenclature_origins_and_more"),
("workbaskets", "0009_workbasket_missing_measures_check_task_id"),
("checks", "0008_alter_trackedmodelcheck_processing_time"),
]

operations = [
migrations.CreateModel(
name="MissingMeasuresCheck",
fields=[
(
"id",
models.AutoField(
auto_created=True,
primary_key=True,
serialize=False,
verbose_name="ID",
),
),
("created_at", models.DateTimeField(auto_now_add=True)),
("updated_at", models.DateTimeField(auto_now=True)),
("successful", models.BooleanField(null=True)),
(
"workbasket",
models.OneToOneField(
null=True,
on_delete=django.db.models.deletion.CASCADE,
related_name="missing_measures_check",
to="workbaskets.workbasket",
),
),
],
options={
"abstract": False,
},
),
migrations.CreateModel(
name="MissingMeasureCommCode",
fields=[
(
"id",
models.AutoField(
auto_created=True,
primary_key=True,
serialize=False,
verbose_name="ID",
),
),
(
"commodity",
models.ForeignKey(
on_delete=django.db.models.deletion.CASCADE,
to="commodities.goodsnomenclature",
),
),
(
"missing_measures_check",
models.ForeignKey(
on_delete=django.db.models.deletion.CASCADE,
related_name="model_checks",
to="checks.missingmeasurescheck",
),
),
],
),
]
29 changes: 29 additions & 0 deletions checks/models.py
Original file line number Diff line number Diff line change
Expand Up @@ -156,3 +156,32 @@ def rule_code(self):
Returns business rule code (e.g. `FO4`).
"""
return self.check_name.split(".")[-1][:-1]


class MissingMeasuresCheck(TimestampedMixin):
"""Stores a timestamp for when check_workbasket_for_missing_measures was
last run, FK to the workbasket and whether the check was successful."""

workbasket = models.OneToOneField(
"workbaskets.WorkBasket",
on_delete=models.CASCADE,
null=True,
related_name="missing_measures_check",
)

successful = fields.BooleanField(null=True)


class MissingMeasureCommCode(models.Model):
"""Links a GoodsNomenclature to a MissingMeasuresCheck when the commodity
fails the check."""

commodity = models.ForeignKey(
"commodities.GoodsNomenclature",
on_delete=models.CASCADE,
)
missing_measures_check = models.ForeignKey(
MissingMeasuresCheck,
on_delete=models.CASCADE,
related_name="model_checks",
)
Comment on lines +161 to +187
Copy link
Collaborator

@paulpepper-trade paulpepper-trade Jan 9, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems to take a different approach to business rule checks.

Business rule checks have a matching TrackedModelCheck.successful=True and new timestamp for each WorkBasket TrackedModel when all business rules have passed. But if there's an incomplete set of TrackedModelChecks, or one or more with a successful attribute set to False, or timestamp older than TrackedModel.updated_at , then business rules are considered to have failed.

Goods are unlikely to change in a workbasket. But measures may do, which would make a clean MissingMeasuresCheck instance invalid. Is staleness and check completeness validation required or supported in this implementation?

Either way, a docstring would be useful, perhaps in MissingMeasuresCheck, to say a little about the checking strategy because of the differing approaches.

16 changes: 16 additions & 0 deletions checks/tests/factories.py
Original file line number Diff line number Diff line change
Expand Up @@ -87,3 +87,19 @@ class Meta:
check_name = factories.string_sequence()
successful = True
message = factory.Faker("text", max_nb_chars=50)


class MissingMeasuresCheckFactory(factory.django.DjangoModelFactory):
class Meta:
model = models.MissingMeasuresCheck

workbasket = factory.SubFactory(factories.WorkBasketFactory)
successful = True


class MissingMeasureCommCodeFactory(factory.django.DjangoModelFactory):
class Meta:
model = models.MissingMeasureCommCode

commodity = factory.SubFactory(factories.SimpleGoodsNomenclatureFactory)
missing_measures_check = factory.SubFactory(MissingMeasuresCheckFactory)
1 change: 1 addition & 0 deletions commodities/tests/test_helpers.py
Original file line number Diff line number Diff line change
Expand Up @@ -26,5 +26,6 @@ def test_modc_query(seed_database_with_indented_goods):
)

result = get_measures_on_declarable_commodities(measure2.transaction, "2903691900")
assert result.count() == 2
assert measure1 in result
assert measure2 in result
5 changes: 5 additions & 0 deletions common/forms.py
Original file line number Diff line number Diff line change
Expand Up @@ -931,3 +931,8 @@ def add_extra_error(self, field, error):
self._errors[field].extend(error_list)
if field in self.cleaned_data:
del self.cleaned_data[field]


class DummyForm(forms.Form):
"""Form with no fields used as a placeholder for when the view requires a
form class."""
4 changes: 2 additions & 2 deletions common/jinja2/macros/fake_tabs.jinja
Original file line number Diff line number Diff line change
@@ -1,8 +1,8 @@
{% macro fake_tabs(links_list, is_nested) -%}
{% macro fake_tabs(links_list, is_nested, font_size="medium") -%}
<ul class="govuk-tabs__list {% if is_nested %}govuk-!-margin-top-4{% endif %}">
{% for link in links_list %}
<li class="govuk-tabs__list-item {% if link.selected %}govuk-tabs__list-item--selected{% endif %}">
<a class="govuk-tabs__tab" href="{{ link.href }}">{{ link.text }}</a>
<a class="govuk-tabs__tab {% if font_size == 'small' %}govuk-!-font-size-16{% endif %} " href="{{ link.href }}">{{ link.text }}</a>
</li>
{% endfor %}
</ul>
Expand Down
9 changes: 9 additions & 0 deletions common/static/common/scss/_utils.scss
Original file line number Diff line number Diff line change
Expand Up @@ -6,4 +6,13 @@

.background-light-grey {
background: govuk-colour("light-grey");
}

.text-align-right {
text-align: right;
}

.success-summary {
@extend .govuk-error-summary;
border-color: govuk-colour("green");
}
9 changes: 9 additions & 0 deletions conftest.py
Original file line number Diff line number Diff line change
Expand Up @@ -58,6 +58,7 @@
from common.tests.util import raises_if
from common.validators import ApplicabilityCode
from common.validators import UpdateType
from geo_areas.validators import AreaCode
from importer.models import ImportBatchStatus
from importer.nursery import get_nursery
from importer.taric import process_taric_xml_stream
Expand Down Expand Up @@ -2262,3 +2263,11 @@ def factory_method(**kwargs):
)

return factory_method


@pytest.fixture
def erga_omnes():
return factories.GeographicalAreaFactory.create(
area_code=AreaCode.GROUP,
area_id="1011",
)
4 changes: 0 additions & 4 deletions measures/forms/wizard.py
Original file line number Diff line number Diff line change
Expand Up @@ -145,10 +145,6 @@ def deserialize_init_kwargs(cls, form_kwargs: Dict) -> Dict:
return kwargs


class MeasureCreateStartForm(forms.Form):
pass


class MeasureDetailsForm(
SerializableFormMixin,
ValidityPeriodForm,
Expand Down
8 changes: 0 additions & 8 deletions measures/tests/conftest.py
Original file line number Diff line number Diff line change
Expand Up @@ -198,14 +198,6 @@ def certificates():
}


@pytest.fixture
def erga_omnes():
return factories.GeographicalAreaFactory.create(
area_code=AreaCode.GROUP,
area_id="1011",
)


@pytest.fixture
def measure_form_data():
measure = factories.MeasureFactory.create()
Expand Down
23 changes: 13 additions & 10 deletions measures/views/wizard.py
Original file line number Diff line number Diff line change
@@ -1,18 +1,15 @@
import logging
from typing import Dict
from typing import List

from crispy_forms_gds.helper import FormHelper
from django.conf import settings
from django.contrib.auth.mixins import PermissionRequiredMixin
from django.http import HttpResponseRedirect
from django.shortcuts import redirect
from django.shortcuts import render
from django.template.loader import render_to_string
from django.utils.decorators import method_decorator
from django.views.generic import TemplateView
from formtools.wizard.views import NamedUrlSessionWizardView

from common.forms import DummyForm
from geo_areas import constants
from geo_areas.models import GeographicalArea
from geo_areas.models import GeographicalMembership
Expand Down Expand Up @@ -177,7 +174,9 @@ def edit_measures(self, selected_measures, cleaned_data):
wizard forms."""

measures_editor = MeasuresEditor(
self.workbasket, selected_measures, cleaned_data
self.workbasket,
selected_measures,
cleaned_data,
)
return measures_editor.edit_measures()

Expand Down Expand Up @@ -205,7 +204,9 @@ def sync_done(self, form_list, **kwargs):

@method_decorator(require_current_workbasket, name="dispatch")
class MeasureCreateWizard(
PermissionRequiredMixin, NamedUrlSessionWizardView, MeasureSerializableWizardMixin
PermissionRequiredMixin,
NamedUrlSessionWizardView,
MeasureSerializableWizardMixin,
):
"""
Multipart form wizard for creating a single measure.
Expand Down Expand Up @@ -244,7 +245,7 @@ class MeasureCreateWizard(
"""Forms in this wizard's steps that collect user data."""

form_list = [
(START, forms.MeasureCreateStartForm),
(START, DummyForm),
*data_form_list,
(SUMMARY, forms.MeasureReviewForm),
]
Expand Down Expand Up @@ -613,7 +614,8 @@ def get_template_names(self):


class MeasuresWizardAsyncConfirm(TemplateView):
"""A success view that serves both the bulk create and bulk edit asynchronous pathways."""
"""A success view that serves both the bulk create and bulk edit
asynchronous pathways."""

template_name = "measures/confirm-create-multiple-async.jinja"

Expand All @@ -624,13 +626,14 @@ def get_context_data(self, **kwargs):


class MeasuresWizardSyncConfirm(TemplateView):
"""A success view that serves both the bulk create and bulk edit synchronous pathways."""
"""A success view that serves both the bulk create and bulk edit synchronous
pathways."""

template_name = "measures/confirm-edit-multiple.jinja"

def get_context_data(self, **kwargs):
context = super().get_context_data(**kwargs)
context["edited_or_created_measures_count"] = self.kwargs.get(
"edited_or_created_measures_count"
"edited_or_created_measures_count",
)
return context
4 changes: 0 additions & 4 deletions quotas/forms/wizards.py
Original file line number Diff line number Diff line change
Expand Up @@ -25,10 +25,6 @@
from workbaskets.forms import SelectableObjectsForm


class DuplicateQuotaDefinitionPeriodStartForm(forms.Form):
pass


class QuotaOrderNumbersSelectForm(forms.Form):
main_quota_order_number = AutoCompleteField(
label="Main quota order number",
Expand Down
3 changes: 2 additions & 1 deletion quotas/views/wizards.py
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@
from django.views.generic import TemplateView
from formtools.wizard.views import NamedUrlSessionWizardView

from common.forms import DummyForm
from common.serializers import serialize_date
from common.validators import UpdateType
from common.views import BusinessRulesMixin
Expand Down Expand Up @@ -44,7 +45,7 @@ class DuplicateDefinitionsWizard(
COMPLETE = "complete"

form_list = [
(START, forms.DuplicateQuotaDefinitionPeriodStartForm),
(START, DummyForm),
(QUOTA_ORDER_NUMBERS, forms.QuotaOrderNumbersSelectForm),
(SELECT_DEFINITION_PERIODS, forms.SelectSubQuotaDefinitionsForm),
(SELECTED_DEFINITIONS, forms.SelectedDefinitionsForm),
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -80,8 +80,7 @@ govuk-button{% if workbasket.tracked_model_checks.exists() %} govuk-button--seco
{% endset %}
{% set rule_check_button = terminate_rule_check_form %}
{% set rule_check_block %}{% endset %}

{% elif workbasket.tracked_model_checks.exists() and not workbasket.unchecked_or_errored_transactions.exists() %}
{% elif workbasket.tracked_model_checks.exists() and not workbasket.unchecked_or_errored_transactions.exists() and not missing_measures_check_successful %}
{% set live_rule_check_status = "Business rule check passed. No business rule violations" %}
{% set rule_check_button = run_business_rules_form %}
{% set rule_check_block %}
Expand Down Expand Up @@ -133,16 +132,14 @@ govuk-button{% if workbasket.tracked_model_checks.exists() %} govuk-button--seco
{% set rule_check_block %}{% endset %}
{% endif %}
{{rule_check_block}}
<dl class="govuk-summary-list">
<div class="govuk-summary-list__row">
<dt class="govuk-summary-list__key">
Live rule check status
</dt>
<dd class="govuk-summary-list__value">
{{ live_rule_check_status }}
</dd>
<dd class="govuk-summary-list__actions">
{{ rule_check_button }}
</dd>
<div class="govuk-grid-row">
<div class="govuk-grid-column-one-quarter">
<h2 class="govuk-heading-s">Live rule check status</h2>
</div>
<div class="govuk-grid-column-one-half">
{{ live_rule_check_status }}
</div>
<div class="govuk-grid-column-one-quarter">
{{ rule_check_button }}
</div>
</dl>
</div>
Loading
Loading