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-1503 Auto end-date measures #1345

Open
wants to merge 18 commits into
base: master
Choose a base branch
from
Open
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
1 change: 1 addition & 0 deletions common/tests/util.py
Original file line number Diff line number Diff line change
@@ -706,6 +706,7 @@ class Dates:
"normal_first_half": (relativedelta(), relativedelta(days=+14)),
"starts_1_month_ago_to_delta": (relativedelta(months=-1), relativedelta()),
"starts_delta_to_1_month_ahead": (relativedelta(), relativedelta(months=1)),
"starts_delta_to_2_months_ahead": (relativedelta(), relativedelta(months=2)),
"starts_1_month_ago_to_1_month_ahead": (
relativedelta(months=-1),
relativedelta(months=1),
2 changes: 1 addition & 1 deletion geo_areas/views.py
Original file line number Diff line number Diff line change
@@ -141,7 +141,7 @@ def get_context_data(self, *args, **kwargs):

class GeoAreaDetailMeasures(SortingMixin, WithPaginationListMixin, ListView):
"""Displays a paginated list of measures for a geo area as a simulated tab
on regulation detail view."""
on geo area detail view."""

model = Measure
template_name = "includes/geo_areas/tabs/measures.jinja"
3 changes: 3 additions & 0 deletions settings/common.py
Original file line number Diff line number Diff line change
@@ -671,6 +671,9 @@
"measures.tasks.bulk_edit_measures": {
"queue": "bulk-create",
},
"workbaskets.tasks.call_end_measures": {
"queue": "bulk-create",
},
re.compile(r"(reference_documents)\.tasks\..*"): {
"queue": "standard",
},
88 changes: 88 additions & 0 deletions workbaskets/jinja2/workbaskets/auto_end_date_measures.jinja
Original file line number Diff line number Diff line change
@@ -0,0 +1,88 @@
{% extends "layouts/layout.jinja" %}
{% from "components/create_sortable_anchor.jinja" import create_sortable_anchor %}
{% from "components/table/macro.njk" import govukTable %}
{% from "components/button/macro.njk" import govukButton %}


{% set page_title %}
Workbasket {{ workbasket.id if workbasket else request.user.current_workbasket.id }} - Auto end-date measures
{% endset %}

{% block content %}
<h1 class="govuk-heading-xl govuk-!-margin-bottom-3">{{ page_title }}</h1>

{% set table_rows = [] %}
{% for measure in object_list %}
{% set measure_link %}
<a class="govuk-link govuk-!-font-weight-bold" href="{{ url('measure-ui-detail', kwargs={'sid': measure.sid}) }}">{{ measure.sid }}</a>
{% endset %}

{% set commodity_link %}
<a class="govuk-link govuk-!-font-weight-bold" href="{{ url('commodity-ui-detail', args=[measure.goods_nomenclature.sid]) }}">{{ measure.goods_nomenclature.item_id }}</a>
{% endset %}

{% set action %}
{% if measure.valid_between.lower > today %}To be deleted{% else %}To be end-dated{% endif %}
{% endset %}

{{ table_rows.append([
{"text": measure_link},
{"html": commodity_link if measure.goods_nomenclature else "-"},
{"text": "{:%d %b %Y}".format(measure.goods_nomenclature.version_at(workbasket.transactions.last()).valid_between.upper) if measure.goods_nomenclature.version_at(workbasket.transactions.last()).valid_between.upper 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 '-' },
{"text": action},
]) or "" }}
{% endfor %}

{% set base_url = url('workbaskets:workbasket-ui-auto-end-date-measures') %}

{% set commodity_code %}
{{ create_sortable_anchor(request, "goods_nomenclature", "Commodity code", base_url) }}
{% endset %}

{% set start_date %}
{{ create_sortable_anchor(request, "start_date", "Measure start date", base_url) }}
{% endset %}

{% set measure_sid %}
{{ create_sortable_anchor(request, "sid", "Measure SID", base_url) }}
{% endset %}

{% if object_list %}
<p>The following measures are linked to commodity codes that have been end-dated in your workbasket. Click submit to automatically end-date or delete these measures.</p>
{{ govukTable({
"head": [
{"text": measure_sid},
{"text": commodity_code},
{"text": "Commodity end date"},
{"text": start_date},
{"text": 'Measure end date'},
{"text": "Action"},
],
"rows": table_rows
}) }}

<form
method="post"
>
<input type="hidden" name="csrfmiddlewaretoken" value="{{ csrf_token }}">
<input type="hidden" name="workbasket_id" value="{{ workbasket.id }}">
{{ govukButton({
'text': "Submit",
'preventDoubleClick': true,
"name": "action",
"value": "auto-end-date-measures",
}) }}
</form>



{% else %}
<p class="govuk-body">No measures to end-date have been found. </p>
<p class="govuk-body">There are either no active measures related to commodity codes updated in this workbasket or the measures already have end-dates in line with their respective commodities.</p>
{% endif %}
{% include "includes/common/pagination.jinja" %}


{% endblock %}
Original file line number Diff line number Diff line change
@@ -0,0 +1,34 @@
{% extends "layouts/layout.jinja" %}

{% from "components/breadcrumbs/macro.njk" import govukBreadcrumbs %}
{% from "components/panel/macro.njk" import govukPanel %}
{% from "components/button/macro.njk" import govukButton %}

{% set page_title = "Measures submitted to be ended" %}

{% block breadcrumb %}
{{ govukBreadcrumbs({
"items": [
{"text": "Home", "href": url("home")},
{"text": "Workbasket " ~ request.user.current_workbasket.id, "href": url("workbaskets:edit-workbasket")},
{"text": "Workbasket " ~ request.user.current_workbasket.id ~ " auto end date measures"},
{"text": page_title}
]
}) }}
{% endblock %}

{% set measure_count = request.session['count_ended_measures'] if request.session['count_ended_measures'] else '0'%}

{% block content %}
<div class="govuk-grid-row">
<div class="govuk-grid-column-two-thirds">
{{ govukPanel({
"titleText": measure_count ~ " measure" ~ measure_count|pluralize ~ " will be ended",
"text": "You have successfully submitted " ~ measure_count ~ " measure" ~ measure_count|pluralize ~ " to be end-dated or deleted and added to workbasket " ~ request.user.current_workbasket.id ~ " when complete.",
"classes": "govuk-!-margin-bottom-7"
}) }}
<h2 class="govuk-heading-m">Next steps</h2>
<a href="{{ url('workbaskets:edit-workbasket') }}" class="govuk-button govuk-button--primary">Continue to workbasket</a>
</div>
</div>
{% endblock %}
10 changes: 10 additions & 0 deletions workbaskets/jinja2/workbaskets/summary-workbasket.jinja
Original file line number Diff line number Diff line change
@@ -169,6 +169,16 @@
{% endif %}
</div>
{% endif %}
<div class="govuk-summary-list__row">
<dt class="govuk-summary-list__key">Auto end-date measures</dt>
<dd class="govuk-summary-list__value">Automatically end-date measures on commodities which have been ended in this workbasket.</dd>
<dd class="govuk-summary-list__actions">
<a
class="govuk-link"
href="{{ url('workbaskets:workbasket-ui-auto-end-date-measures') }}"
>Auto end-date measures</a>
</dd>
</div>
</dl>

{% if comments and can_view_comment %}
48 changes: 48 additions & 0 deletions workbaskets/models.py
Original file line number Diff line number Diff line change
@@ -14,9 +14,14 @@
from django.core.exceptions import ImproperlyConfigured
from django.core.exceptions import ValidationError
from django.db import models
from django.db.models import Case
from django.db.models import DateField
from django.db.models import F
from django.db.models import Max
from django.db.models import QuerySet
from django.db.models import Subquery
from django.db.models import Value
from django.db.models import When
from django_fsm import FSMField
from django_fsm import transition

@@ -687,6 +692,49 @@ def assigned_reviewers(self):
user_ids = self.reviewer_assignments.values_list("user_id", flat=True)
return User.objects.filter(id__in=user_ids)

def get_measures_to_end_date(self) -> QuerySet:
"""
Returns a queryset of measures on end-dated commodities in the
workbasket along with those commodities' end-dates.

It filters out measures which have already ended.
"""

from commodities.models.orm import GoodsNomenclature

end_dated_commodities = GoodsNomenclature.objects.current().filter(
transaction__workbasket=self,
valid_between__upper_inf=False,
)
commodity_dict = {
commodity.sid: commodity.valid_between
for commodity in end_dated_commodities
}
measures_on_commodities = (
Measure.objects.current()
.with_effective_valid_between()
.filter(
goods_nomenclature__sid__in=commodity_dict.keys(),
)
)
conditions = [
When(
goods_nomenclature__sid=commodity_sid,
then=Value(commodity_valid_between),
)
for commodity_sid, commodity_valid_between in commodity_dict.items()
]
measures = measures_on_commodities.annotate(
commodity_valid_between=Case(
*conditions,
output_field=DateField(),
),
)

return measures.exclude(
db_effective_valid_between__not_gt=F("commodity_valid_between"),
)

class Meta:
verbose_name = "workbasket"
verbose_name_plural = "workbaskets"
75 changes: 75 additions & 0 deletions workbaskets/tasks.py
Original file line number Diff line number Diff line change
@@ -1,12 +1,21 @@
from datetime import date

from celery import group
from celery import shared_task
from celery.utils.log import get_task_logger
from django.db.models import F
from django.db.transaction import atomic

from checks.tasks import check_transaction
from checks.tasks import check_transaction_sync
from commodities.models.orm import GoodsNomenclature
from common.celery import app
from common.models.transactions import Transaction
from common.util import TaricDateRange
from common.validators import UpdateType
from measures.models.tracked_models import Measure
from workbaskets.models import WorkBasket
from workbaskets.validators import WorkflowStatus

# Celery logger adds the task id and status and outputs via the worker.
logger = get_task_logger(__name__)
@@ -64,3 +73,69 @@ def call_check_workbasket_sync(self, workbasket_id: int):
workbasket: WorkBasket = WorkBasket.objects.get(pk=workbasket_id)
workbasket.delete_checks()
check_workbasket_sync(workbasket)


def promote_measure_to_top(promoted_measure, workbasket_transactions):
Copy link
Collaborator

@paulpepper-trade paulpepper-trade Dec 18, 2024

Choose a reason for hiding this comment

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

Although this function is currently only called from end_measures() it would be worth wrapping it with @atomic too: it'd be harmless to do so and would signal that there are multiple DB writes occuring within it that should be considered atomic.

"""Set the transaction order of `promoted_measure` to be first in the
workbasket, demoting the transactions that came before it."""

top_transaction = workbasket_transactions.first()

if (
not promoted_measure
or not top_transaction
or promoted_measure == top_transaction
):
return

current_position = promoted_measure.order
top_position = top_transaction.order
workbasket_transactions.filter(order__lt=current_position).update(
order=F("order") + 1,
)
promoted_measure.order = top_position
promoted_measure.save(update_fields=["order"])
Comment on lines +93 to +97
Copy link
Collaborator

@paulpepper-trade paulpepper-trade Dec 18, 2024

Choose a reason for hiding this comment

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

There's almost certainly a race condition here against other transactions being moved or created in the same workbasket. However, if other transaction operations can be avoided, then I think this should be fine.

This (draft) PR aims to resolve a similar race difficulty with an existing, flawed use ofselect_for_update() by evaluating the associated queryset before updating the selected DB rows.



@atomic
def end_measures(measures, workbasket):
mattjamc marked this conversation as resolved.
Show resolved Hide resolved
"""Iterate through measures on commodities, end-date those which have
already began and delete those which have not yet started."""
for measure in measures:
workbasket_transactions = Transaction.objects.filter(
workbasket=workbasket,
workbasket__status=WorkflowStatus.EDITING,
).order_by("order")
commodity = (
GoodsNomenclature.objects.all()
.filter(
sid=measure.goods_nomenclature.sid,
transaction__workbasket=workbasket,
)
.last()
)
if measure.valid_between.lower > min(
date.today(),
commodity.valid_between.upper,
):
new_measure_version = measure.new_version(
workbasket=workbasket,
update_type=UpdateType.DELETE,
)
else:
new_measure_version = measure.new_version(
workbasket=workbasket,
update_type=UpdateType.UPDATE,
valid_between=TaricDateRange(
measure.valid_between.lower,
commodity.valid_between.upper,
),
)
promote_measure_to_top(new_measure_version.transaction, workbasket_transactions)


@app.task
def call_end_measures(measure_pks, workbasket_pk):
workbasket = WorkBasket.objects.all().get(pk=workbasket_pk)
measures = Measure.objects.all().filter(pk__in=measure_pks)
end_measures(measures, workbasket)
Loading

Unchanged files with check annotations Beta

).not.toBeInTheDocument();
});
it.skip("should update the exclusions options list when a group is selected", () => {

Check warning on line 176 in common/static/common/js/components/GeoAreaForm/tests/index.test.js

GitHub Actions / Lint and static checks

Disabled test
const mockInitial = {
geoAreaType: "GROUP",
geographicalAreaGroup: "",