From 73232b99f2f32b981513e842968aef3059535020 Mon Sep 17 00:00:00 2001 From: Lauren Qurashi Date: Mon, 2 Sep 2024 17:07:44 +0100 Subject: [PATCH 01/38] Make MeasuresBulkEditor model --- .../migrations/0017_measuresbulkeditor.py | 84 +++++++++++++++++++ measures/models/bulk_processing.py | 63 ++++++++++++++ 2 files changed, 147 insertions(+) create mode 100644 measures/migrations/0017_measuresbulkeditor.py diff --git a/measures/migrations/0017_measuresbulkeditor.py b/measures/migrations/0017_measuresbulkeditor.py new file mode 100644 index 000000000..9ea8940c4 --- /dev/null +++ b/measures/migrations/0017_measuresbulkeditor.py @@ -0,0 +1,84 @@ +# Generated by Django 4.2.15 on 2024-09-02 16:06 + +from django.conf import settings +from django.db import migrations, models +import django.db.models.deletion +import django_fsm +import measures.models.bulk_processing + + +class Migration(migrations.Migration): + + dependencies = [ + migrations.swappable_dependency(settings.AUTH_USER_MODEL), + ("workbaskets", "0008_datarow_dataupload"), + ("measures", "0016_measuresbulkcreator"), + ] + + operations = [ + migrations.CreateModel( + name="MeasuresBulkEditor", + 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)), + ( + "task_id", + models.CharField(blank=True, max_length=50, null=True, unique=True), + ), + ( + "processing_state", + django_fsm.FSMField( + choices=[ + ("AWAITING_PROCESSING", "Awaiting processing"), + ("CURRENTLY_PROCESSING", "Currently processing"), + ("SUCCESSFULLY_PROCESSED", "Successfully processed"), + ("FAILED_PROCESSING", "Failed processing"), + ("CANCELLED", "Cancelled"), + ], + db_index=True, + default="AWAITING_PROCESSING", + editable=False, + max_length=50, + protected=True, + ), + ), + ( + "successfully_processed_count", + models.PositiveIntegerField(default=0), + ), + ("form_data", models.JSONField()), + ("form_kwargs", models.JSONField()), + ("selected_measures", models.JSONField()), + ( + "user", + models.ForeignKey( + editable=False, + null=True, + on_delete=django.db.models.deletion.SET_NULL, + to=settings.AUTH_USER_MODEL, + ), + ), + ( + "workbasket", + models.ForeignKey( + editable=False, + null=True, + on_delete=measures.models.bulk_processing.REVOKE_TASKS_AND_SET_NULL, + to="workbaskets.workbasket", + ), + ), + ], + options={ + "abstract": False, + }, + ), + ] diff --git a/measures/models/bulk_processing.py b/measures/models/bulk_processing.py index aef1d8763..1a3c70ff4 100644 --- a/measures/models/bulk_processing.py +++ b/measures/models/bulk_processing.py @@ -414,3 +414,66 @@ def _log_form_errors(self, form_class, form_or_formset) -> None: for form_errors in errors: for error_key, error_values in form_errors.items(): logger.error(f"{error_key}: {error_values}") + + +class MeasuresBulkEditorManager(models.Manager): + """Model Manager for MeasuresBulkEditor models.""" + + def create( + self, + form_data: Dict, + form_kwargs: Dict, + workbasket, + user, + selected_measures, + **kwargs, + ) -> "MeasuresBulkCreator": + """Create and save an instance of MeasuresBulkEditor.""" + + return super().create( + form_data=form_data, + form_kwargs=form_kwargs, + workbasket=workbasket, + user=user, + selected_measures=selected_measures, + **kwargs, + ) + + +class MeasuresBulkEditor(BulkProcessor): + """ + Model class used to bulk edit Measures instances from serialized form + data. + The stored form data is serialized and deserialized by Forms that subclass + SerializableFormMixin. + """ + + objects = MeasuresBulkEditorManager() + + form_data = models.JSONField() + """Dictionary of all Form.data, used to reconstruct bound Form instances as + if the form data had been sumbitted by the user within the measure wizard + process.""" + + form_kwargs = models.JSONField() + """Dictionary of all form init data, excluding a form's `data` param (which + is preserved via this class's `form_data` attribute).""" + + selected_measures = models.JSONField() + """List of all measures that have been selected for bulk editing.""" + + workbasket = models.ForeignKey( + "workbaskets.WorkBasket", + on_delete=REVOKE_TASKS_AND_SET_NULL, + null=True, + editable=False, + ) + """The workbasket with which created measures are associated.""" + + user = models.ForeignKey( + settings.AUTH_USER_MODEL, + on_delete=SET_NULL, + null=True, + editable=False, + ) + """The user who submitted the task to create measures.""" \ No newline at end of file From 4fe9c655579bcf516012eb56e3197116b77fa10c Mon Sep 17 00:00:00 2001 From: Lauren Qurashi Date: Mon, 2 Sep 2024 17:23:45 +0100 Subject: [PATCH 02/38] Export the MeasuresBulkEditor model --- measures/models/__init__.py | 2 ++ 1 file changed, 2 insertions(+) diff --git a/measures/models/__init__.py b/measures/models/__init__.py index a4c641802..f49a0d12e 100644 --- a/measures/models/__init__.py +++ b/measures/models/__init__.py @@ -1,5 +1,6 @@ from measures.models.bulk_processing import BulkProcessor from measures.models.bulk_processing import MeasuresBulkCreator +from measures.models.bulk_processing import MeasuresBulkEditor from measures.models.bulk_processing import ProcessingState from measures.models.tracked_models import AdditionalCodeTypeMeasureType from measures.models.tracked_models import DutyExpression @@ -23,6 +24,7 @@ # - Classes exported from bulk_processing.py. "BulkProcessor", "MeasuresBulkCreator", + "MeasuresBulkEditor", "ProcessingState", # - Classes exported from tracked_model.py. "AdditionalCodeTypeMeasureType", From 4863177dd8a6bbc0ea5800dc97466a183b058515 Mon Sep 17 00:00:00 2001 From: Lauren Qurashi Date: Mon, 2 Sep 2024 17:26:26 +0100 Subject: [PATCH 03/38] Add flag for Measures async edit --- settings/common.py | 1 + 1 file changed, 1 insertion(+) diff --git a/settings/common.py b/settings/common.py index bbf3cdb60..c537c0db5 100644 --- a/settings/common.py +++ b/settings/common.py @@ -919,3 +919,4 @@ # Asynchronous / background (bulk) object creation and editing config. MEASURES_ASYNC_CREATION = is_truthy(os.environ.get("MEASURES_ASYNC_CREATION", "true")) +MEASURES_ASYNC_EDIT = is_truthy(os.environ.get("MEASURES_ASYNC_EDIT", "true")) \ No newline at end of file From 8d403bc75ff32e8ab917d6d09e8d934483d990de Mon Sep 17 00:00:00 2001 From: Lauren Qurashi Date: Mon, 2 Sep 2024 17:30:36 +0100 Subject: [PATCH 04/38] Split done function into sync and async done --- measures/views/wizard.py | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/measures/views/wizard.py b/measures/views/wizard.py index af82e1af0..171f03c84 100644 --- a/measures/views/wizard.py +++ b/measures/views/wizard.py @@ -231,6 +231,15 @@ def update_measure_footnote_associations(self, measure, workbasket): ) def done(self, form_list, **kwargs): + if settings.MEASURES_ASYNC_EDIT: + return self.async_done(form_list, **kwargs) + else: + return self.sync_done(form_list, **kwargs) + + def async_done(self, form_list, **kwargs): + pass + + def sync_done(self, form_list, **kwargs): cleaned_data = self.get_all_cleaned_data() selected_measures = self.get_queryset() workbasket = WorkBasket.current(self.request) From 713d758d041a1d00a66577530cfebc99b644eb01 Mon Sep 17 00:00:00 2001 From: Lauren Qurashi Date: Mon, 2 Sep 2024 17:38:54 +0100 Subject: [PATCH 05/38] Add Mixin and kwarg functions to Start date form --- measures/forms/wizard.py | 26 +++++++++++++++++++++++++- 1 file changed, 25 insertions(+), 1 deletion(-) diff --git a/measures/forms/wizard.py b/measures/forms/wizard.py index 2e68915ec..cf21c8963 100644 --- a/measures/forms/wizard.py +++ b/measures/forms/wizard.py @@ -781,7 +781,7 @@ def __init__(self, *args, **kwargs): ) -class MeasureStartDateForm(forms.Form): +class MeasureStartDateForm(forms.Form, SerializableFormMixin): start_date = DateInputFieldFixed( label="Start date", help_text="For example, 27 3 2008", @@ -818,6 +818,30 @@ def clean(self): ) return cleaned_data + + @classmethod + def serializable_init_kwargs(cls, kwargs: Dict) -> Dict: + selected_measures = kwargs.get("selected_measures") + selected_measures_pks = [] + for measure in selected_measures: + selected_measures_pks.append(measure.id) + + serializable_kwargs = { + "selected_measures": selected_measures_pks, + } + + return serializable_kwargs + + @classmethod + def deserialize_init_kwargs(cls, form_kwargs: Dict) -> Dict: + serialized_selected_measures_pks = form_kwargs.get("selected_measures") + deserialized_selected_measures = models.Measure.objects.filter(pk__in=serialized_selected_measures_pks) + + kwargs = { + "selected_measures": deserialized_selected_measures, + } + + return kwargs class MeasureEndDateForm(forms.Form): From e77b66ebb2712172117413574daf578f4a5498e8 Mon Sep 17 00:00:00 2001 From: Lauren Qurashi Date: Mon, 2 Sep 2024 17:39:56 +0100 Subject: [PATCH 06/38] Add Mixin and kwarg functions to End date form --- measures/forms/wizard.py | 26 +++++++++++++++++++++++++- 1 file changed, 25 insertions(+), 1 deletion(-) diff --git a/measures/forms/wizard.py b/measures/forms/wizard.py index cf21c8963..612249cfe 100644 --- a/measures/forms/wizard.py +++ b/measures/forms/wizard.py @@ -844,7 +844,7 @@ def deserialize_init_kwargs(cls, form_kwargs: Dict) -> Dict: return kwargs -class MeasureEndDateForm(forms.Form): +class MeasureEndDateForm(forms.Form, SerializableFormMixin): end_date = DateInputFieldFixed( label="End date", help_text="For example, 27 3 2008", @@ -884,6 +884,30 @@ def clean(self): cleaned_data["end_date"] = None return cleaned_data + + @classmethod + def serializable_init_kwargs(cls, kwargs: Dict) -> Dict: + selected_measures = kwargs.get("selected_measures") + selected_measures_pks = [] + for measure in selected_measures: + selected_measures_pks.append(measure.id) + + serializable_kwargs = { + "selected_measures": selected_measures_pks, + } + + return serializable_kwargs + + @classmethod + def deserialize_init_kwargs(cls, form_kwargs: Dict) -> Dict: + serialized_selected_measures_pks = form_kwargs.get("selected_measures") + deserialized_selected_measures = models.Measure.objects.filter(pk__in=serialized_selected_measures_pks) + + kwargs = { + "selected_measures": deserialized_selected_measures, + } + + return kwargs class MeasureRegulationForm(forms.Form): From dbab9ffa991f2a800f70b4ff1325345093d24303 Mon Sep 17 00:00:00 2001 From: Lauren Qurashi Date: Mon, 2 Sep 2024 17:40:52 +0100 Subject: [PATCH 07/38] Add mixin and kwarg functions to Regulations form --- measures/forms/wizard.py | 26 +++++++++++++++++++++++++- 1 file changed, 25 insertions(+), 1 deletion(-) diff --git a/measures/forms/wizard.py b/measures/forms/wizard.py index 612249cfe..5988c06e5 100644 --- a/measures/forms/wizard.py +++ b/measures/forms/wizard.py @@ -910,7 +910,7 @@ def deserialize_init_kwargs(cls, form_kwargs: Dict) -> Dict: return kwargs -class MeasureRegulationForm(forms.Form): +class MeasureRegulationForm(forms.Form, SerializableFormMixin): generating_regulation = AutoCompleteField( label="Regulation ID", help_text="Select the regulation which provides the legal basis for the measures.", @@ -935,6 +935,30 @@ def __init__(self, *args, **kwargs): data_prevent_double_click="true", ), ) + + @classmethod + def serializable_init_kwargs(cls, kwargs: Dict) -> Dict: + selected_measures = kwargs.get("selected_measures") + selected_measures_pks = [] + for measure in selected_measures: + selected_measures_pks.append(measure.id) + + serializable_kwargs = { + "selected_measures": selected_measures_pks, + } + + return serializable_kwargs + + @classmethod + def deserialize_init_kwargs(cls, form_kwargs: Dict) -> Dict: + serialized_selected_measures_pks = form_kwargs.get("selected_measures") + deserialized_selected_measures = models.Measure.objects.filter(pk__in=serialized_selected_measures_pks) + + kwargs = { + "selected_measures": deserialized_selected_measures, + } + + return kwargs class MeasureDutiesForm(forms.Form): From 704fc6337c0934a295a91dcb19645f2ca899e77a Mon Sep 17 00:00:00 2001 From: Lauren Qurashi Date: Mon, 2 Sep 2024 17:41:56 +0100 Subject: [PATCH 08/38] Add Mixin and kwarg functions to Duties form --- measures/forms/wizard.py | 28 ++++++++++++++++++++++++++-- 1 file changed, 26 insertions(+), 2 deletions(-) diff --git a/measures/forms/wizard.py b/measures/forms/wizard.py index 5988c06e5..36441144d 100644 --- a/measures/forms/wizard.py +++ b/measures/forms/wizard.py @@ -935,7 +935,7 @@ def __init__(self, *args, **kwargs): data_prevent_double_click="true", ), ) - + @classmethod def serializable_init_kwargs(cls, kwargs: Dict) -> Dict: selected_measures = kwargs.get("selected_measures") @@ -961,7 +961,7 @@ def deserialize_init_kwargs(cls, form_kwargs: Dict) -> Dict: return kwargs -class MeasureDutiesForm(forms.Form): +class MeasureDutiesForm(forms.Form, SerializableFormMixin): duties = forms.CharField( label="Duties", help_text="Enter the duty that applies to the measures.", @@ -1003,6 +1003,30 @@ def clean(self): validate_duties(duties, measure.valid_between.lower) return cleaned_data + + @classmethod + def serializable_init_kwargs(cls, kwargs: Dict) -> Dict: + selected_measures = kwargs.get("selected_measures") + selected_measures_pks = [] + for measure in selected_measures: + selected_measures_pks.append(measure.id) + + serializable_kwargs = { + "selected_measures": selected_measures_pks, + } + + return serializable_kwargs + + @classmethod + def deserialize_init_kwargs(cls, form_kwargs: Dict) -> Dict: + serialized_selected_measures_pks = form_kwargs.get("selected_measures") + deserialized_selected_measures = models.Measure.objects.filter(pk__in=serialized_selected_measures_pks) + + kwargs = { + "selected_measures": deserialized_selected_measures, + } + + return kwargs class MeasureGeographicalAreaExclusionsForm(forms.Form): From 21b83542b9b966ce0af964a781249eec1a486193 Mon Sep 17 00:00:00 2001 From: Lauren Qurashi Date: Mon, 2 Sep 2024 17:42:38 +0100 Subject: [PATCH 09/38] Add Mixin to Geographical Areas formset --- measures/forms/wizard.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/measures/forms/wizard.py b/measures/forms/wizard.py index 36441144d..c80f89372 100644 --- a/measures/forms/wizard.py +++ b/measures/forms/wizard.py @@ -1061,7 +1061,7 @@ def __init__(self, *args, **kwargs): ) -class MeasureGeographicalAreaExclusionsFormSet(FormSet): +class MeasureGeographicalAreaExclusionsFormSet(FormSet, SerializableFormMixin): """Allows editing the geographical area exclusions of multiple measures in `MeasureEditWizard`.""" From 647239889d5efad9d4b005eeeedff48c3d3b9f50 Mon Sep 17 00:00:00 2001 From: Lauren Qurashi Date: Mon, 2 Sep 2024 18:03:20 +0100 Subject: [PATCH 10/38] WIP - Move serializable functions into a wizard mixin --- measures/views/mixins.py | 93 ++++++++++++++++++++++++++++++++++++++++ 1 file changed, 93 insertions(+) diff --git a/measures/views/mixins.py b/measures/views/mixins.py index bab973754..de8422e5c 100644 --- a/measures/views/mixins.py +++ b/measures/views/mixins.py @@ -1,4 +1,5 @@ from typing import Type +from typing import Dict from common.models import TrackedModel from measures import models @@ -50,3 +51,95 @@ def get_queryset(self): """Get the queryset for measures that are candidates for editing/deletion.""" return models.Measure.objects.filter(pk__in=self.measure_selections) + + +class MeasureSerializableWizardMixin(): + # Make this work + def all_serializable_form_data(self) -> Dict: + """ + Returns serializable data for all wizard steps. + + This is a re-implementation of + MeasureCreateWizard.get_all_cleaned_data(), but using self.data after + is_valid() has been successfully run. + """ + + all_data = {} + + for form_key in self.get_data_form_list().keys(): + all_data[form_key] = self.serializable_form_data_for_step(form_key) + + return all_data + + def serializable_form_data_for_step(self, step) -> Dict: + """ + Returns serializable data for a wizard step. + + This is a re-implementation of WizardView.get_cleaned_data_for_step(), + returning the serializable version of data in place of the form's + regular cleaned_data. + """ + + form_obj = self.get_form( + step=step, + data=self.storage.get_step_data(step), + files=self.storage.get_step_files(step), + ) + + return form_obj.serializable_data(remove_key_prefix=step) + + def all_serializable_form_kwargs(self) -> Dict: + """Returns serializable kwargs for all wizard steps.""" + + all_kwargs = {} + + for form_key in self.get_data_form_list().keys(): + all_kwargs[form_key] = self.serializable_form_kwargs_for_step(form_key) + + return all_kwargs + + def serializable_form_kwargs_for_step(self, step) -> Dict: + """Returns serializable kwargs for a wizard step.""" + + form_kwargs = self.get_form_kwargs(step) + form_class = self.form_list[step] + + return form_class.serializable_init_kwargs(form_kwargs) + + def get_all_cleaned_data(self): + """ + Returns a merged dictionary of all step cleaned_data. If a step contains + a `FormSet`, the key will be prefixed with 'formset-' and contain a list + of the formset cleaned_data dictionaries, as expected in + `create_measures()`. + + Note: This patched version of `super().get_all_cleaned_data()` takes advantage of retrieving previously-saved + cleaned_data by summary page to avoid revalidating forms unnecessarily. + """ + all_cleaned_data = {} + for form_key in self.get_form_list(): + cleaned_data = self.get_cleaned_data_for_step(form_key) + if isinstance(cleaned_data, (tuple, list)): + all_cleaned_data.update( + { + f"formset-{form_key}": cleaned_data, + }, + ) + else: + all_cleaned_data.update(cleaned_data) + return all_cleaned_data + + def get_cleaned_data_for_step(self, step): + """ + Returns cleaned data for a given `step`. + + Note: This patched version of `super().get_cleaned_data_for_step` temporarily saves the cleaned_data + to provide quick retrieval should another call for it be made in the same request (as happens in + `get_form_kwargs()` and template for summary page) to avoid revalidating forms unnecessarily. + """ + self.cleaned_data = getattr(self, "cleaned_data", {}) + if step in self.cleaned_data: + return self.cleaned_data[step] + + self.cleaned_data[step] = super().get_cleaned_data_for_step(step) + return self.cleaned_data[step] \ No newline at end of file From 4e9659a7c6eb86db8f5c7009fcf519b30adb19cc Mon Sep 17 00:00:00 2001 From: Lauren Qurashi Date: Tue, 3 Sep 2024 15:35:36 +0100 Subject: [PATCH 11/38] Create SerializableWizardMixin for wizards that use async bulk functionality --- measures/views/mixins.py | 61 +++++++++++++--------------------------- 1 file changed, 19 insertions(+), 42 deletions(-) diff --git a/measures/views/mixins.py b/measures/views/mixins.py index de8422e5c..9d283d061 100644 --- a/measures/views/mixins.py +++ b/measures/views/mixins.py @@ -54,11 +54,27 @@ def get_queryset(self): class MeasureSerializableWizardMixin(): - # Make this work + """ A Mixin for the wizard forms that utilise asynchronous bulk processing. This mixin provides the functionality to go through each form + and serialize the data ready for storing in the database.""" + def get_data_form_list(self) -> dict: + """ + Returns a form list based on form_list, conditionally including only + those items as per condition_list and also appearing in data_form_list. + The list is generated dynamically because conditions in condition_list + may be dynamic. + Essentially, version of `WizardView.get_form_list()` filtering in only + those list items appearing in `data_form_list`. + """ + data_form_keys = [key for key, form in self.data_form_list] + return { + form_key: form_class + for form_key, form_class in self.get_form_list().items() + if form_key in data_form_keys + } + def all_serializable_form_data(self) -> Dict: """ Returns serializable data for all wizard steps. - This is a re-implementation of MeasureCreateWizard.get_all_cleaned_data(), but using self.data after is_valid() has been successfully run. @@ -74,7 +90,6 @@ def all_serializable_form_data(self) -> Dict: def serializable_form_data_for_step(self, step) -> Dict: """ Returns serializable data for a wizard step. - This is a re-implementation of WizardView.get_cleaned_data_for_step(), returning the serializable version of data in place of the form's regular cleaned_data. @@ -104,42 +119,4 @@ def serializable_form_kwargs_for_step(self, step) -> Dict: form_kwargs = self.get_form_kwargs(step) form_class = self.form_list[step] - return form_class.serializable_init_kwargs(form_kwargs) - - def get_all_cleaned_data(self): - """ - Returns a merged dictionary of all step cleaned_data. If a step contains - a `FormSet`, the key will be prefixed with 'formset-' and contain a list - of the formset cleaned_data dictionaries, as expected in - `create_measures()`. - - Note: This patched version of `super().get_all_cleaned_data()` takes advantage of retrieving previously-saved - cleaned_data by summary page to avoid revalidating forms unnecessarily. - """ - all_cleaned_data = {} - for form_key in self.get_form_list(): - cleaned_data = self.get_cleaned_data_for_step(form_key) - if isinstance(cleaned_data, (tuple, list)): - all_cleaned_data.update( - { - f"formset-{form_key}": cleaned_data, - }, - ) - else: - all_cleaned_data.update(cleaned_data) - return all_cleaned_data - - def get_cleaned_data_for_step(self, step): - """ - Returns cleaned data for a given `step`. - - Note: This patched version of `super().get_cleaned_data_for_step` temporarily saves the cleaned_data - to provide quick retrieval should another call for it be made in the same request (as happens in - `get_form_kwargs()` and template for summary page) to avoid revalidating forms unnecessarily. - """ - self.cleaned_data = getattr(self, "cleaned_data", {}) - if step in self.cleaned_data: - return self.cleaned_data[step] - - self.cleaned_data[step] = super().get_cleaned_data_for_step(step) - return self.cleaned_data[step] \ No newline at end of file + return form_class.serializable_init_kwargs(form_kwargs) \ No newline at end of file From bcfb454719235c2ac74dbc3755103093220b9820 Mon Sep 17 00:00:00 2001 From: Lauren Qurashi Date: Tue, 3 Sep 2024 15:40:09 +0100 Subject: [PATCH 12/38] Add MeasureSerializableWizardMixin to Create Measures --- measures/views/wizard.py | 72 ++-------------------------------------- 1 file changed, 3 insertions(+), 69 deletions(-) diff --git a/measures/views/wizard.py b/measures/views/wizard.py index 171f03c84..6637702c1 100644 --- a/measures/views/wizard.py +++ b/measures/views/wizard.py @@ -28,6 +28,7 @@ from measures.constants import MeasureEditSteps from measures.creators import MeasuresCreator from measures.util import diff_components +from measures.views.mixins import MeasureSerializableWizardMixin from workbaskets.models import WorkBasket from workbaskets.views.decorators import require_current_workbasket @@ -313,6 +314,8 @@ def sync_done(self, form_list, **kwargs): class MeasureCreateWizard( PermissionRequiredMixin, NamedUrlSessionWizardView, + MeasureSerializableWizardMixin + ): """ Multipart form wizard for creating a single measure. @@ -435,24 +438,6 @@ class MeasureCreateWizard( boolean or boolean values that indicate whether a wizard step should be shown.""" - def get_data_form_list(self) -> dict: - """ - Returns a form list based on form_list, conditionally including only - those items as per condition_list and also appearing in data_form_list. - - The list is generated dynamically because conditions in condition_list - may be dynamic. - - Essentially, version of `WizardView.get_form_list()` filtering in only - those list items appearing in `data_form_list`. - """ - data_form_keys = [key for key, form in self.data_form_list] - return { - form_key: form_class - for form_key, form_class in self.get_form_list().items() - if form_key in data_form_keys - } - @property def workbasket(self) -> WorkBasket: return WorkBasket.current(self.request) @@ -516,57 +501,6 @@ def async_done(self, form_list, **kwargs): expected_measures_count=measures_bulk_creator.expected_measures_count, ) - def all_serializable_form_data(self) -> Dict: - """ - Returns serializable data for all wizard steps. - - This is a re-implementation of - MeasureCreateWizard.get_all_cleaned_data(), but using self.data after - is_valid() has been successfully run. - """ - - all_data = {} - - for form_key in self.get_data_form_list().keys(): - all_data[form_key] = self.serializable_form_data_for_step(form_key) - - return all_data - - def serializable_form_data_for_step(self, step) -> Dict: - """ - Returns serializable data for a wizard step. - - This is a re-implementation of WizardView.get_cleaned_data_for_step(), - returning the serializable version of data in place of the form's - regular cleaned_data. - """ - - form_obj = self.get_form( - step=step, - data=self.storage.get_step_data(step), - files=self.storage.get_step_files(step), - ) - - return form_obj.serializable_data(remove_key_prefix=step) - - def all_serializable_form_kwargs(self) -> Dict: - """Returns serializable kwargs for all wizard steps.""" - - all_kwargs = {} - - for form_key in self.get_data_form_list().keys(): - all_kwargs[form_key] = self.serializable_form_kwargs_for_step(form_key) - - return all_kwargs - - def serializable_form_kwargs_for_step(self, step) -> Dict: - """Returns serializable kwargs for a wizard step.""" - - form_kwargs = self.get_form_kwargs(step) - form_class = self.form_list[step] - - return form_class.serializable_init_kwargs(form_kwargs) - def get_all_cleaned_data(self): """ Returns a merged dictionary of all step cleaned_data. If a step contains From 8db7aeff28c6b6188acd854a2459c6809d42eeea Mon Sep 17 00:00:00 2001 From: Lauren Qurashi Date: Tue, 3 Sep 2024 16:00:11 +0100 Subject: [PATCH 13/38] Add serializable wizard mixin and split the form lists --- measures/views/wizard.py | 12 ++++++++++-- 1 file changed, 10 insertions(+), 2 deletions(-) diff --git a/measures/views/wizard.py b/measures/views/wizard.py index 6637702c1..aa41f427c 100644 --- a/measures/views/wizard.py +++ b/measures/views/wizard.py @@ -42,6 +42,7 @@ class MeasureEditWizard( PermissionRequiredMixin, MeasureSelectionQuerysetMixin, NamedUrlSessionWizardView, + MeasureSerializableWizardMixin, ): """ Multipart form wizard for editing multiple measures. @@ -52,8 +53,7 @@ class MeasureEditWizard( storage_name = "measures.wizard.MeasureEditSessionStorage" permission_required = ["common.change_trackedmodel"] - form_list = [ - (START, forms.MeasuresEditFieldsForm), + data_form_list = [ (MeasureEditSteps.START_DATE, forms.MeasureStartDateForm), (MeasureEditSteps.END_DATE, forms.MeasureEndDateForm), (MeasureEditSteps.QUOTA_ORDER_NUMBER, forms.MeasureQuotaOrderNumberForm), @@ -64,6 +64,14 @@ class MeasureEditWizard( forms.MeasureGeographicalAreaExclusionsFormSet, ), ] + """Forms in this wizard's steps that collect user data.""" + + form_list = [ + (START, forms.MeasuresEditFieldsForm), + *data_form_list, + ] + """All Forms in this wizard's steps, including both those that collect user + data and those that don't.""" templates = { START: "measures/edit-multiple-start.jinja", From ceadfeeacbf07758958f1f870502e6367fbed672 Mon Sep 17 00:00:00 2001 From: Lauren Qurashi Date: Tue, 3 Sep 2024 16:26:05 +0100 Subject: [PATCH 14/38] Set up Celery route for Async bulk edit --- settings/common.py | 3 +++ 1 file changed, 3 insertions(+) diff --git a/settings/common.py b/settings/common.py index c537c0db5..65e0abaa4 100644 --- a/settings/common.py +++ b/settings/common.py @@ -663,6 +663,9 @@ "measures.tasks.bulk_create_measures": { "queue": "bulk-create", }, + "measures.tasks.bulk_edit_measures": { + "queue": "bulk-create", + }, } SQLITE_EXCLUDED_APPS = [ From b8cd69f827664bc2f79c4b32b955699de5142e90 Mon Sep 17 00:00:00 2001 From: Lauren Qurashi Date: Tue, 3 Sep 2024 16:40:42 +0100 Subject: [PATCH 15/38] WIP - Flesh out editing functionality - need to consider update functions --- measures/models/bulk_processing.py | 169 ++++++++++++++++++++++++++++- measures/tasks.py | 35 ++++++ measures/views/wizard.py | 29 ++++- 3 files changed, 231 insertions(+), 2 deletions(-) diff --git a/measures/models/bulk_processing.py b/measures/models/bulk_processing.py index 1a3c70ff4..5b003ba3c 100644 --- a/measures/models/bulk_processing.py +++ b/measures/models/bulk_processing.py @@ -17,6 +17,8 @@ from common.celery import app from common.models.mixins import TimestampedMixin from common.models.utils import override_current_transaction +from common.util import TaricDateRange +from common.validators import UpdateType from measures.models.tracked_models import Measure logger = logging.getLogger(__name__) @@ -476,4 +478,169 @@ class MeasuresBulkEditor(BulkProcessor): null=True, editable=False, ) - """The user who submitted the task to create measures.""" \ No newline at end of file + """The user who submitted the task to create measures.""" + + def schedule_task(self) -> AsyncResult: + """Implementation of base class method.""" + + from measures.tasks import bulk_edit_measures + + async_result = bulk_edit_measures.apply_async( + kwargs={ + "measures_bulk_editor_pk": self.pk, + }, + countdown=1, + ) + self.task_id = async_result.id + self.save() + + logger.info( + f"Measure bulk edit scheduled on task with ID {async_result.id}" + f"using MeasuresBulkEditor.pk={self.pk}.", + ) + + return async_result + + @atomic + def edit_measures(self) -> Iterable[Measure]: + logger.info("INSIDE EDIT MEASURES - BULK PROCESSING") + + with override_current_transaction( + transaction=self.workbasket.current_transaction, + ): + cleaned_data = self.get_forms_cleaned_data() + deserialized_selected_measures = Measure.objects.filter(pk__in=self.selected_measures) + + new_start_date = cleaned_data.get("start_date", None) + new_end_date = cleaned_data.get("end_date", False) + new_quota_order_number = cleaned_data.get("order_number", None) + new_generating_regulation = cleaned_data.get("generating_regulation", None) + new_duties = cleaned_data.get("duties", None) + new_exclusions = [ + e["excluded_area"] + for e in cleaned_data.get("formset-geographical_area_exclusions", []) + ] + + if deserialized_selected_measures: + edited_measures = [] + + for measure in deserialized_selected_measures: + new_measure = measure.new_version( + workbasket=self.workbasket, + update_type=UpdateType.UPDATE, + valid_between=TaricDateRange( + lower=( + new_start_date + if new_start_date + else measure.valid_between.lower + ), + upper=( + new_end_date + if new_end_date + else measure.valid_between.upper + ), + ), + order_number=( + new_quota_order_number + if new_quota_order_number + else measure.order_number + ), + generating_regulation=( + new_generating_regulation + if new_generating_regulation + else measure.generating_regulation + ), + ) + logger.info("UPDATE FUNCTIONS STARTING") + logger.info(f"BE - NEW MEASURE: {new_measure.__dict__}") + logger.info(f"BE - NEW DUTIES: {new_duties}") + + update_measure_components( + measure=new_measure, + duties=new_duties, + workbasket=self.workbasket, + ) + update_measure_condition_components( + measure=new_measure, + workbasket=self.workbasket, + ) + update_measure_excluded_geographical_areas( + edited="geographical_area_exclusions" + in cleaned_data.get("fields_to_edit", []), + measure=new_measure, + exclusions=new_exclusions, + workbasket=self.workbasket, + ) + update_measure_footnote_associations( + measure=new_measure, + workbasket=self.workbasket, + ) + logger.info(f"NEW MEASURE WITH FUNCTIONS RUN: {new_measure}") + + edited_measures.append(new_measure.id) + logger.info(f"EDITED MEASURES ARRAY ON CLOSE: {edited_measures}") + return edited_measures + + def get_forms_cleaned_data(self) -> Dict: + """ + Returns a merged dictionary of all Form cleaned_data. + + If a Form's data contains a `FormSet`, the key will be prefixed with + "formset-" and contain a list of the formset cleaned_data dictionaries. + + If form validation errors are encountered when constructing cleaned + data, then this function raises Django's `ValidationError` exception. + """ + all_cleaned_data = {} + + from measures.views import MeasureEditWizard + for form_key, form_class in MeasureEditWizard.data_form_list: + + if form_key not in self.form_data: + # Forms are conditionally included during step processing - see + # `MeasureEditWizard.show_step()` for details. + continue + + data = self.form_data[form_key] + kwargs = form_class.deserialize_init_kwargs(self.form_kwargs[form_key]) + + form = form_class(data=data, **kwargs) + + if not form.is_valid(): + self._log_form_errors(form_class=form_class, form_or_formset=form) + raise ValidationError( + f"{form_class.__name__} has {len(form.errors)} errors.", + ) + + if isinstance(form.cleaned_data, (tuple, list)): + all_cleaned_data[f"formset-{form_key}"] = form.cleaned_data + else: + all_cleaned_data.update(form.cleaned_data) + + logger.info(f"RESULT OF ALL CLEANED DATA: {all_cleaned_data}") + return all_cleaned_data + + def _log_form_errors(self, form_class, form_or_formset) -> None: + """Output errors associated with a Form or Formset instance, handling + output for each instance type in a uniform manner.""" + + logger.error( + f"MeasuresBulkEditor.edit_measures() - " + f"{form_class.__name__} has {len(form_or_formset.errors)} errors.", + ) + + # Form.errors is a dictionary of errors, but FormSet.errors is a + # list of dictionaries of Form.errors. Access their errors in + # a uniform manner. + errors = [] + + if isinstance(form_or_formset, BaseFormSet): + errors = [ + {"formset_errors": form_or_formset.non_form_errors()}, + ] + form_or_formset.errors + else: + errors = [form_or_formset.errors] + + for form_errors in errors: + for error_key, error_values in form_errors.items(): + logger.error(f"{error_key}: {error_values}") \ No newline at end of file diff --git a/measures/tasks.py b/measures/tasks.py index ed61c3f6d..3ebe5e6d4 100644 --- a/measures/tasks.py +++ b/measures/tasks.py @@ -2,6 +2,7 @@ from common.celery import app from measures.models import MeasuresBulkCreator +from measures.models import MeasuresBulkEditor logger = logging.getLogger(__name__) @@ -43,3 +44,37 @@ def bulk_create_measures(measures_bulk_creator_pk: int) -> None: f"succeeded but created no measures in " f"WorkBasket({measures_bulk_creator.workbasket.pk}).", ) + +@app.task +def bulk_edit_measures(measures_bulk_editor_pk: int) -> None: + """Bulk edit measures from serialized measures form data saved within an + instance of MeasuresBulkEditor.""" + + measures_bulk_editor = MeasuresBulkEditor.objects.get(pk=measures_bulk_editor_pk) + measures_bulk_editor.begin_processing() + measures_bulk_editor.save() + + try: + measures = measures_bulk_editor.edit_measures() + except Exception as e: + measures_bulk_editor.processing_failed() + measures_bulk_editor.save() + logger.error( + f"MeasuresBulkCreator({measures_bulk_editor.pk}) task failed " + f"attempting to edit measures in " + f"WorkBasket({measures_bulk_editor.workbasket.pk}).", + ) + raise e + + logger.info(f"MEASURES: {measures}") + + measures_bulk_editor.processing_succeeded() + measures_bulk_editor.successfully_processed_count = len(measures) + measures_bulk_editor.save() + + if measures: + logger.info( + f"MeasuresBulkEditoror({measures_bulk_editor.pk}) task " + f"succeeded in editing {len(measures)} Measures in " + f"WorkBasket({measures_bulk_editor.workbasket.pk}).", + ) \ No newline at end of file diff --git a/measures/views/wizard.py b/measures/views/wizard.py index aa41f427c..9cae93b46 100644 --- a/measures/views/wizard.py +++ b/measures/views/wizard.py @@ -108,6 +108,10 @@ def get_template_names(self): self.steps.current, "measures/edit-wizard-step.jinja", ) + + @property + def workbasket(self) -> WorkBasket: + return WorkBasket.current(self.request) def get_context_data(self, form, **kwargs): context = super().get_context_data(form=form, **kwargs) @@ -246,7 +250,30 @@ def done(self, form_list, **kwargs): return self.sync_done(form_list, **kwargs) def async_done(self, form_list, **kwargs): - pass + logger.info("Editing measures asynchronously.") + serializable_data = self.all_serializable_form_data() + serializable_form_kwargs = self.all_serializable_form_kwargs() + + db_selected_measures = [] + for measure in self.get_queryset(): + db_selected_measures.append(measure.id) + + measures_bulk_editor = models.MeasuresBulkEditor.objects.create( + form_data=serializable_data, + form_kwargs=serializable_form_kwargs, + workbasket=self.workbasket, + user=self.request.user, + selected_measures=db_selected_measures, + ) + self.session_store.clear() + measures_bulk_editor.schedule_task() + + return redirect( + reverse( + "workbaskets:workbasket-ui-review-measures", + kwargs={"pk": self.workbasket.pk}, + ), + ) def sync_done(self, form_list, **kwargs): cleaned_data = self.get_all_cleaned_data() From a3df250c8d450886b3c9661b70fdf3c1fde4be65 Mon Sep 17 00:00:00 2001 From: Lauren Qurashi Date: Tue, 3 Sep 2024 16:47:21 +0100 Subject: [PATCH 16/38] WIP - Move Update functions into measures/util - will probs give circular import issues --- measures/util.py | 112 ++++++++++++++++++++++++++++++++++++++- measures/views/wizard.py | 99 ---------------------------------- 2 files changed, 111 insertions(+), 100 deletions(-) diff --git a/measures/util.py b/measures/util.py index bf00efd6a..0b6a3c7c8 100644 --- a/measures/util.py +++ b/measures/util.py @@ -1,7 +1,6 @@ import decimal from datetime import date from math import floor -from typing import Type from common.models import TrackedModel from common.models.transactions import Transaction @@ -9,6 +8,17 @@ from measures.models import MeasureComponent from workbaskets.models import WorkBasket +from geo_areas.models import GeographicalArea +from geo_areas.utils import get_all_members_of_geo_groups +from measures import models as measure_models +from measures.util import diff_components +from typing import List +from typing import Type +from workbaskets import models as workbasket_models + +import logging +logger = logging.getLogger(__name__) + def convert_eur_to_gbp(amount: str, eur_gbp_conversion_rate: float) -> str: """Convert EUR amount to GBP and round down to nearest pence.""" @@ -91,3 +101,103 @@ def diff_components( update_type=UpdateType.DELETE, transaction=workbasket.new_transaction(), ) + + +def update_measure_components( + self, + measure: models.Measure, + duties: str, + workbasket: WorkBasket, + ): + """Updates the measure components associated to the measure.""" + diff_components( + instance=measure, + duty_sentence=duties if duties else measure.duty_sentence, + start_date=measure.valid_between.lower, + workbasket=workbasket, + transaction=workbasket.current_transaction, + ) + + def update_measure_condition_components( + self, + measure: models.Measure, + workbasket: WorkBasket, + ): + """Updates the measure condition components associated to the + measure.""" + conditions = measure.conditions.current() + for condition in conditions: + condition.new_version( + dependent_measure=measure, + workbasket=workbasket, + ) + + def update_measure_excluded_geographical_areas( + self, + edited: bool, + measure: models.Measure, + exclusions: List[GeographicalArea], + workbasket: WorkBasket, + ): + """Updates the excluded geographical areas associated to the measure.""" + existing_exclusions = measure.exclusions.current() + + # Update any exclusions to new measure version + if not edited: + for exclusion in existing_exclusions: + exclusion.new_version( + modified_measure=measure, + workbasket=workbasket, + ) + return + + new_excluded_areas = get_all_members_of_geo_groups( + validity=measure.valid_between, + geo_areas=exclusions, + ) + + for geo_area in new_excluded_areas: + existing_exclusion = existing_exclusions.filter( + excluded_geographical_area=geo_area, + ).first() + if existing_exclusion: + existing_exclusion.new_version( + modified_measure=measure, + workbasket=workbasket, + ) + else: + models.MeasureExcludedGeographicalArea.objects.create( + modified_measure=measure, + excluded_geographical_area=geo_area, + update_type=UpdateType.CREATE, + transaction=workbasket.new_transaction(), + ) + + removed_excluded_areas = { + e.excluded_geographical_area for e in existing_exclusions + }.difference(set(exclusions)) + + exclusions_to_remove = [ + existing_exclusions.get(excluded_geographical_area__id=geo_area.id) + for geo_area in removed_excluded_areas + ] + + for exclusion in exclusions_to_remove: + exclusion.new_version( + update_type=UpdateType.DELETE, + modified_measure=measure, + workbasket=workbasket, + ) + + def update_measure_footnote_associations(self, measure, workbasket): + """Updates the footnotes associated to the measure.""" + footnote_associations = ( + models.FootnoteAssociationMeasure.objects.current().filter( + footnoted_measure__sid=measure.sid, + ) + ) + for fa in footnote_associations: + fa.new_version( + footnoted_measure=measure, + workbasket=workbasket, + ) diff --git a/measures/views/wizard.py b/measures/views/wizard.py index 9cae93b46..a0f117ec6 100644 --- a/measures/views/wizard.py +++ b/measures/views/wizard.py @@ -144,105 +144,6 @@ def get_form_kwargs(self, step): return kwargs - def update_measure_components( - self, - measure: models.Measure, - duties: str, - workbasket: WorkBasket, - ): - """Updates the measure components associated to the measure.""" - diff_components( - instance=measure, - duty_sentence=duties if duties else measure.duty_sentence, - start_date=measure.valid_between.lower, - workbasket=workbasket, - transaction=workbasket.current_transaction, - ) - - def update_measure_condition_components( - self, - measure: models.Measure, - workbasket: WorkBasket, - ): - """Updates the measure condition components associated to the - measure.""" - conditions = measure.conditions.current() - for condition in conditions: - condition.new_version( - dependent_measure=measure, - workbasket=workbasket, - ) - - def update_measure_excluded_geographical_areas( - self, - edited: bool, - measure: models.Measure, - exclusions: List[GeographicalArea], - workbasket: WorkBasket, - ): - """Updates the excluded geographical areas associated to the measure.""" - existing_exclusions = measure.exclusions.current() - - # Update any exclusions to new measure version - if not edited: - for exclusion in existing_exclusions: - exclusion.new_version( - modified_measure=measure, - workbasket=workbasket, - ) - return - - new_excluded_areas = get_all_members_of_geo_groups( - validity=measure.valid_between, - geo_areas=exclusions, - ) - - for geo_area in new_excluded_areas: - existing_exclusion = existing_exclusions.filter( - excluded_geographical_area=geo_area, - ).first() - if existing_exclusion: - existing_exclusion.new_version( - modified_measure=measure, - workbasket=workbasket, - ) - else: - models.MeasureExcludedGeographicalArea.objects.create( - modified_measure=measure, - excluded_geographical_area=geo_area, - update_type=UpdateType.CREATE, - transaction=workbasket.new_transaction(), - ) - - removed_excluded_areas = { - e.excluded_geographical_area for e in existing_exclusions - }.difference(set(exclusions)) - - exclusions_to_remove = [ - existing_exclusions.get(excluded_geographical_area__id=geo_area.id) - for geo_area in removed_excluded_areas - ] - - for exclusion in exclusions_to_remove: - exclusion.new_version( - update_type=UpdateType.DELETE, - modified_measure=measure, - workbasket=workbasket, - ) - - def update_measure_footnote_associations(self, measure, workbasket): - """Updates the footnotes associated to the measure.""" - footnote_associations = ( - models.FootnoteAssociationMeasure.objects.current().filter( - footnoted_measure__sid=measure.sid, - ) - ) - for fa in footnote_associations: - fa.new_version( - footnoted_measure=measure, - workbasket=workbasket, - ) - def done(self, form_list, **kwargs): if settings.MEASURES_ASYNC_EDIT: return self.async_done(form_list, **kwargs) From e68724848bcc666aa75bc2feb769d0ee8f65c5d6 Mon Sep 17 00:00:00 2001 From: Lauren Qurashi Date: Wed, 4 Sep 2024 16:47:02 +0100 Subject: [PATCH 17/38] Move update functions into measures/util file --- measures/util.py | 177 +++++++++++++++++++-------------------- measures/views/wizard.py | 13 +-- 2 files changed, 95 insertions(+), 95 deletions(-) diff --git a/measures/util.py b/measures/util.py index 0b6a3c7c8..70ed3bf94 100644 --- a/measures/util.py +++ b/measures/util.py @@ -5,13 +5,10 @@ from common.models import TrackedModel from common.models.transactions import Transaction from common.validators import UpdateType -from measures.models import MeasureComponent -from workbaskets.models import WorkBasket from geo_areas.models import GeographicalArea from geo_areas.utils import get_all_members_of_geo_groups from measures import models as measure_models -from measures.util import diff_components from typing import List from typing import Type from workbaskets import models as workbasket_models @@ -39,9 +36,9 @@ def diff_components( instance, duty_sentence: str, start_date: date, - workbasket: WorkBasket, + workbasket: workbasket_models.WorkBasket, transaction: Type[Transaction], - component_output: Type[TrackedModel] = MeasureComponent, + component_output: Type[TrackedModel] = measure_models.MeasureComponent, reverse_attribute: str = "component_measure", ): """ @@ -104,100 +101,100 @@ def diff_components( def update_measure_components( - self, - measure: models.Measure, - duties: str, - workbasket: WorkBasket, - ): - """Updates the measure components associated to the measure.""" - diff_components( - instance=measure, - duty_sentence=duties if duties else measure.duty_sentence, - start_date=measure.valid_between.lower, + measure: measure_models.Measure, + duties: str, + workbasket: workbasket_models.WorkBasket, +): + """Updates the measure components associated to the measure.""" + diff_components( + instance=measure, + duty_sentence=duties if duties else measure.duty_sentence, + start_date=measure.valid_between.lower, + workbasket=workbasket, + transaction=workbasket.current_transaction, + ) + + +def update_measure_condition_components( + measure: measure_models.Measure, + workbasket: workbasket_models.WorkBasket, +): + """Updates the measure condition components associated to the + measure.""" + conditions = measure.conditions.current() + for condition in conditions: + condition.new_version( + dependent_measure=measure, workbasket=workbasket, - transaction=workbasket.current_transaction, ) - def update_measure_condition_components( - self, - measure: models.Measure, - workbasket: WorkBasket, - ): - """Updates the measure condition components associated to the - measure.""" - conditions = measure.conditions.current() - for condition in conditions: - condition.new_version( - dependent_measure=measure, - workbasket=workbasket, - ) - def update_measure_excluded_geographical_areas( - self, - edited: bool, - measure: models.Measure, - exclusions: List[GeographicalArea], - workbasket: WorkBasket, - ): - """Updates the excluded geographical areas associated to the measure.""" - existing_exclusions = measure.exclusions.current() - - # Update any exclusions to new measure version - if not edited: - for exclusion in existing_exclusions: - exclusion.new_version( - modified_measure=measure, - workbasket=workbasket, - ) - return - - new_excluded_areas = get_all_members_of_geo_groups( - validity=measure.valid_between, - geo_areas=exclusions, - ) +def update_measure_excluded_geographical_areas( + edited: bool, + measure: measure_models.Measure, + exclusions: List[GeographicalArea], + workbasket: workbasket_models.WorkBasket, +): + """Updates the excluded geographical areas associated to the measure.""" + existing_exclusions = measure.exclusions.current() - for geo_area in new_excluded_areas: - existing_exclusion = existing_exclusions.filter( - excluded_geographical_area=geo_area, - ).first() - if existing_exclusion: - existing_exclusion.new_version( - modified_measure=measure, - workbasket=workbasket, - ) - else: - models.MeasureExcludedGeographicalArea.objects.create( - modified_measure=measure, - excluded_geographical_area=geo_area, - update_type=UpdateType.CREATE, - transaction=workbasket.new_transaction(), - ) - - removed_excluded_areas = { - e.excluded_geographical_area for e in existing_exclusions - }.difference(set(exclusions)) - - exclusions_to_remove = [ - existing_exclusions.get(excluded_geographical_area__id=geo_area.id) - for geo_area in removed_excluded_areas - ] - - for exclusion in exclusions_to_remove: + # Update any exclusions to new measure version + if not edited: + for exclusion in existing_exclusions: exclusion.new_version( - update_type=UpdateType.DELETE, modified_measure=measure, workbasket=workbasket, ) + return - def update_measure_footnote_associations(self, measure, workbasket): - """Updates the footnotes associated to the measure.""" - footnote_associations = ( - models.FootnoteAssociationMeasure.objects.current().filter( - footnoted_measure__sid=measure.sid, - ) - ) - for fa in footnote_associations: - fa.new_version( - footnoted_measure=measure, + new_excluded_areas = get_all_members_of_geo_groups( + validity=measure.valid_between, + geo_areas=exclusions, + ) + + for geo_area in new_excluded_areas: + existing_exclusion = existing_exclusions.filter( + excluded_geographical_area=geo_area, + ).first() + if existing_exclusion: + existing_exclusion.new_version( + modified_measure=measure, workbasket=workbasket, ) + else: + measure_models.MeasureExcludedGeographicalArea.objects.create( + modified_measure=measure, + excluded_geographical_area=geo_area, + update_type=UpdateType.CREATE, + transaction=workbasket.new_transaction(), + ) + + removed_excluded_areas = { + e.excluded_geographical_area for e in existing_exclusions + }.difference(set(exclusions)) + + exclusions_to_remove = [ + existing_exclusions.get(excluded_geographical_area__id=geo_area.id) + for geo_area in removed_excluded_areas + ] + + for exclusion in exclusions_to_remove: + exclusion.new_version( + update_type=UpdateType.DELETE, + modified_measure=measure, + workbasket=workbasket, + ) + + +def update_measure_footnote_associations(measure, workbasket): + """Updates the footnotes associated to the measure.""" + footnote_associations = ( + measure_models.FootnoteAssociationMeasure.objects.current().filter( + footnoted_measure__sid=measure.sid, + ) + ) + for fa in footnote_associations: + fa.new_version( + footnoted_measure=measure, + workbasket=workbasket, + ) diff --git a/measures/views/wizard.py b/measures/views/wizard.py index a0f117ec6..0d0cb94bd 100644 --- a/measures/views/wizard.py +++ b/measures/views/wizard.py @@ -27,7 +27,10 @@ from measures.constants import START from measures.constants import MeasureEditSteps from measures.creators import MeasuresCreator -from measures.util import diff_components +from measures.util import update_measure_components +from measures.util import update_measure_condition_components +from measures.util import update_measure_excluded_geographical_areas +from measures.util import update_measure_footnote_associations from measures.views.mixins import MeasureSerializableWizardMixin from workbaskets.models import WorkBasket from workbaskets.views.decorators import require_current_workbasket @@ -216,23 +219,23 @@ def sync_done(self, form_list, **kwargs): else measure.generating_regulation ), ) - self.update_measure_components( + update_measure_components( measure=new_measure, duties=new_duties, workbasket=workbasket, ) - self.update_measure_condition_components( + update_measure_condition_components( measure=new_measure, workbasket=workbasket, ) - self.update_measure_excluded_geographical_areas( + update_measure_excluded_geographical_areas( edited="geographical_area_exclusions" in cleaned_data.get("fields_to_edit", []), measure=new_measure, exclusions=new_exclusions, workbasket=workbasket, ) - self.update_measure_footnote_associations( + update_measure_footnote_associations( measure=new_measure, workbasket=workbasket, ) From bb3767aef2cb0f7dd399c8b30fc42cba98aaa236 Mon Sep 17 00:00:00 2001 From: Lauren Qurashi Date: Thu, 5 Sep 2024 12:22:04 +0100 Subject: [PATCH 18/38] WIP - Update functions in util --- measures/models/bulk_processing.py | 6 +++++- measures/util.py | 7 +++++-- 2 files changed, 10 insertions(+), 3 deletions(-) diff --git a/measures/models/bulk_processing.py b/measures/models/bulk_processing.py index 5b003ba3c..206444c4d 100644 --- a/measures/models/bulk_processing.py +++ b/measures/models/bulk_processing.py @@ -20,6 +20,10 @@ from common.util import TaricDateRange from common.validators import UpdateType from measures.models.tracked_models import Measure +from measures.util import update_measure_components +from measures.util import update_measure_condition_components +from measures.util import update_measure_excluded_geographical_areas +from measures.util import update_measure_footnote_associations logger = logging.getLogger(__name__) @@ -554,7 +558,7 @@ def edit_measures(self) -> Iterable[Measure]: logger.info("UPDATE FUNCTIONS STARTING") logger.info(f"BE - NEW MEASURE: {new_measure.__dict__}") logger.info(f"BE - NEW DUTIES: {new_duties}") - + update_measure_components( measure=new_measure, duties=new_duties, diff --git a/measures/util.py b/measures/util.py index 70ed3bf94..0334a7702 100644 --- a/measures/util.py +++ b/measures/util.py @@ -36,9 +36,9 @@ def diff_components( instance, duty_sentence: str, start_date: date, - workbasket: workbasket_models.WorkBasket, transaction: Type[Transaction], - component_output: Type[TrackedModel] = measure_models.MeasureComponent, + workbasket: workbasket_models.WorkBasket = None, + component_output: measure_models.MeasureComponent = None, reverse_attribute: str = "component_measure", ): """ @@ -56,6 +56,9 @@ def diff_components( """ from measures.parsers import DutySentenceParser + component_output = measure_models.MeasureComponent if component_output else component_output + workbasket = workbasket_models.WorkBasket if workbasket else workbasket + parser = DutySentenceParser.create( start_date, component_output=component_output, From 7d37617c1e067d237f4d44eede39278946d4b461 Mon Sep 17 00:00:00 2001 From: Lauren Qurashi Date: Fri, 6 Sep 2024 17:18:05 +0100 Subject: [PATCH 19/38] Fix circular import errors --- measures/util.py | 24 ++++++++++++++---------- 1 file changed, 14 insertions(+), 10 deletions(-) diff --git a/measures/util.py b/measures/util.py index 0334a7702..147c33fb9 100644 --- a/measures/util.py +++ b/measures/util.py @@ -37,8 +37,8 @@ def diff_components( duty_sentence: str, start_date: date, transaction: Type[Transaction], - workbasket: workbasket_models.WorkBasket = None, - component_output: measure_models.MeasureComponent = None, + workbasket: "workbasket_models.WorkBasket", + component_output: type = None, reverse_attribute: str = "component_measure", ): """ @@ -56,8 +56,9 @@ def diff_components( """ from measures.parsers import DutySentenceParser - component_output = measure_models.MeasureComponent if component_output else component_output - workbasket = workbasket_models.WorkBasket if workbasket else workbasket + # We add in the component output type here as otherwise we run into circular import issues. + component_output = measure_models.MeasureComponent if not component_output else component_output + parser = DutySentenceParser.create( start_date, @@ -104,11 +105,14 @@ def diff_components( def update_measure_components( - measure: measure_models.Measure, duties: str, - workbasket: workbasket_models.WorkBasket, + workbasket: "workbasket_models.WorkBasket", + measure: "measure_models.Measure", ): """Updates the measure components associated to the measure.""" + workbasket = workbasket_models.WorkBasket if workbasket else workbasket + measure = measure_models.Measure if measure else measure + diff_components( instance=measure, duty_sentence=duties if duties else measure.duty_sentence, @@ -119,8 +123,8 @@ def update_measure_components( def update_measure_condition_components( - measure: measure_models.Measure, - workbasket: workbasket_models.WorkBasket, + measure: "measure_models.Measure", + workbasket: "workbasket_models.WorkBasket", ): """Updates the measure condition components associated to the measure.""" @@ -134,9 +138,9 @@ def update_measure_condition_components( def update_measure_excluded_geographical_areas( edited: bool, - measure: measure_models.Measure, + measure: "measure_models.Measure", exclusions: List[GeographicalArea], - workbasket: workbasket_models.WorkBasket, + workbasket: "workbasket_models.WorkBasket", ): """Updates the excluded geographical areas associated to the measure.""" existing_exclusions = measure.exclusions.current() From 6b894121f89c503950ea5449cc24b0491b780a66 Mon Sep 17 00:00:00 2001 From: Lauren Qurashi Date: Fri, 6 Sep 2024 17:31:38 +0100 Subject: [PATCH 20/38] Delete unnecessary if statement and comment --- measures/models/bulk_processing.py | 1 - measures/util.py | 3 --- 2 files changed, 4 deletions(-) diff --git a/measures/models/bulk_processing.py b/measures/models/bulk_processing.py index 206444c4d..93bb49024 100644 --- a/measures/models/bulk_processing.py +++ b/measures/models/bulk_processing.py @@ -621,7 +621,6 @@ def get_forms_cleaned_data(self) -> Dict: else: all_cleaned_data.update(form.cleaned_data) - logger.info(f"RESULT OF ALL CLEANED DATA: {all_cleaned_data}") return all_cleaned_data def _log_form_errors(self, form_class, form_or_formset) -> None: diff --git a/measures/util.py b/measures/util.py index 147c33fb9..0c84f2a4b 100644 --- a/measures/util.py +++ b/measures/util.py @@ -110,9 +110,6 @@ def update_measure_components( measure: "measure_models.Measure", ): """Updates the measure components associated to the measure.""" - workbasket = workbasket_models.WorkBasket if workbasket else workbasket - measure = measure_models.Measure if measure else measure - diff_components( instance=measure, duty_sentence=duties if duties else measure.duty_sentence, From 1fca1c7ea92b7d03dd67a227c9adc3db0fe39c67 Mon Sep 17 00:00:00 2001 From: Lauren Qurashi Date: Fri, 6 Sep 2024 17:41:20 +0100 Subject: [PATCH 21/38] Clean up ready for testing --- measures/models/bulk_processing.py | 8 +------- measures/tasks.py | 4 +--- 2 files changed, 2 insertions(+), 10 deletions(-) diff --git a/measures/models/bulk_processing.py b/measures/models/bulk_processing.py index 93bb49024..c939b2331 100644 --- a/measures/models/bulk_processing.py +++ b/measures/models/bulk_processing.py @@ -555,10 +555,6 @@ def edit_measures(self) -> Iterable[Measure]: else measure.generating_regulation ), ) - logger.info("UPDATE FUNCTIONS STARTING") - logger.info(f"BE - NEW MEASURE: {new_measure.__dict__}") - logger.info(f"BE - NEW DUTIES: {new_duties}") - update_measure_components( measure=new_measure, duties=new_duties, @@ -579,10 +575,8 @@ def edit_measures(self) -> Iterable[Measure]: measure=new_measure, workbasket=self.workbasket, ) - logger.info(f"NEW MEASURE WITH FUNCTIONS RUN: {new_measure}") + edited_measures.append(new_measure.id) - edited_measures.append(new_measure.id) - logger.info(f"EDITED MEASURES ARRAY ON CLOSE: {edited_measures}") return edited_measures def get_forms_cleaned_data(self) -> Dict: diff --git a/measures/tasks.py b/measures/tasks.py index 3ebe5e6d4..20387a94d 100644 --- a/measures/tasks.py +++ b/measures/tasks.py @@ -66,15 +66,13 @@ def bulk_edit_measures(measures_bulk_editor_pk: int) -> None: ) raise e - logger.info(f"MEASURES: {measures}") - measures_bulk_editor.processing_succeeded() measures_bulk_editor.successfully_processed_count = len(measures) measures_bulk_editor.save() if measures: logger.info( - f"MeasuresBulkEditoror({measures_bulk_editor.pk}) task " + f"MeasuresBulkEditor({measures_bulk_editor.pk}) task " f"succeeded in editing {len(measures)} Measures in " f"WorkBasket({measures_bulk_editor.workbasket.pk}).", ) \ No newline at end of file From d095671a56c9ce10636e09dca27633c0f1546bc2 Mon Sep 17 00:00:00 2001 From: Lauren Qurashi Date: Wed, 11 Sep 2024 15:51:13 +0100 Subject: [PATCH 22/38] Change back order of kwargs to fix test --- measures/util.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/measures/util.py b/measures/util.py index 0c84f2a4b..2ca1cdf09 100644 --- a/measures/util.py +++ b/measures/util.py @@ -36,8 +36,8 @@ def diff_components( instance, duty_sentence: str, start_date: date, - transaction: Type[Transaction], workbasket: "workbasket_models.WorkBasket", + transaction: Type[Transaction], component_output: type = None, reverse_attribute: str = "component_measure", ): From 658484a28b8bdfe2531929051d75054eb8facda2 Mon Sep 17 00:00:00 2001 From: Lauren Qurashi Date: Mon, 16 Sep 2024 14:15:54 +0100 Subject: [PATCH 23/38] WIP - Mock won't work --- measures/tests/test_views.py | 17 +++++++++-------- 1 file changed, 9 insertions(+), 8 deletions(-) diff --git a/measures/tests/test_views.py b/measures/tests/test_views.py index 28dfda8cb..7cd807e36 100644 --- a/measures/tests/test_views.py +++ b/measures/tests/test_views.py @@ -61,8 +61,9 @@ def mocked_diff_components(): """Mocks `diff_components()` inside `update_measure_components()` in `MeasureEditWizard` to prevent parsing errors where test measures lack a duty sentence.""" + with patch( - "measures.views.MeasureEditWizard.update_measure_components", + "measures.util.update_measure_components", ) as update_measure_components: yield update_measure_components @@ -1833,7 +1834,7 @@ def test_measuretype_api_list_view(valid_user_client): equals=True, ) - +@override_settings(MEASURES_ASYNC_EDIT=False) def test_multiple_measure_start_and_end_date_edit_functionality( valid_user_client, user_workbasket, @@ -1927,7 +1928,7 @@ def test_multiple_measure_start_and_end_date_edit_functionality( assert measure.valid_between.upper == datetime.date(2100, 1, 1) assert measure.effective_end_date == datetime.date(2100, 1, 1) - +@override_settings(MEASURES_ASYNC_EDIT=False) @pytest.mark.parametrize( "step, form_data, updated_attribute, expected_data", [ @@ -2038,7 +2039,7 @@ def test_multiple_measure_edit_single_form_functionality( assert measure.update_type == UpdateType.UPDATE assert reduce(getattr, updated_attribute.split("."), measure) == expected_data - +@override_settings(MEASURES_ASYNC_EDIT=False) def test_multiple_measure_edit_only_regulation( valid_user_client, user_workbasket, @@ -2263,7 +2264,7 @@ def test_measure_list_selected_measures_list(valid_user_client): assert not measure_ids_in_table.difference(selected_measures_ids) - +@override_settings(MEASURES_ASYNC_EDIT=False) def test_multiple_measure_edit_only_quota_order_number( valid_user_client, user_workbasket, @@ -2334,7 +2335,7 @@ def test_multiple_measure_edit_only_quota_order_number( assert measure.update_type == UpdateType.UPDATE assert measure.order_number == quota_order_number - +@override_settings(MEASURES_ASYNC_EDIT=False) def test_multiple_measure_edit_only_duties( valid_user_client, user_workbasket, @@ -2405,7 +2406,7 @@ def test_multiple_measure_edit_only_duties( assert measure.update_type == UpdateType.UPDATE assert measure.duty_sentence == duties - +@override_settings(MEASURES_ASYNC_EDIT=False) def test_multiple_measure_edit_preserves_footnote_associations( valid_user_client, user_workbasket, @@ -2481,7 +2482,7 @@ def test_multiple_measure_edit_preserves_footnote_associations( for footnote in measure.footnotes.all(): assert footnote in expected_footnotes - +@override_settings(MEASURES_ASYNC_EDIT=False) def test_multiple_measure_edit_geographical_area_exclusions( valid_user_client, user_workbasket, From 38404d684f1003acb6f9436b17c41cc3bb8b0d69 Mon Sep 17 00:00:00 2001 From: Lauren Qurashi Date: Mon, 16 Sep 2024 15:58:11 +0100 Subject: [PATCH 24/38] Fix mock file path --- measures/tests/test_views.py | 15 +++++++-------- 1 file changed, 7 insertions(+), 8 deletions(-) diff --git a/measures/tests/test_views.py b/measures/tests/test_views.py index 7cd807e36..b987e3221 100644 --- a/measures/tests/test_views.py +++ b/measures/tests/test_views.py @@ -61,9 +61,8 @@ def mocked_diff_components(): """Mocks `diff_components()` inside `update_measure_components()` in `MeasureEditWizard` to prevent parsing errors where test measures lack a duty sentence.""" - with patch( - "measures.util.update_measure_components", + "measures.views.wizard.update_measure_components", ) as update_measure_components: yield update_measure_components @@ -1928,7 +1927,7 @@ def test_multiple_measure_start_and_end_date_edit_functionality( assert measure.valid_between.upper == datetime.date(2100, 1, 1) assert measure.effective_end_date == datetime.date(2100, 1, 1) -@override_settings(MEASURES_ASYNC_EDIT=False) + @pytest.mark.parametrize( "step, form_data, updated_attribute, expected_data", [ @@ -2039,7 +2038,7 @@ def test_multiple_measure_edit_single_form_functionality( assert measure.update_type == UpdateType.UPDATE assert reduce(getattr, updated_attribute.split("."), measure) == expected_data -@override_settings(MEASURES_ASYNC_EDIT=False) + def test_multiple_measure_edit_only_regulation( valid_user_client, user_workbasket, @@ -2264,7 +2263,7 @@ def test_measure_list_selected_measures_list(valid_user_client): assert not measure_ids_in_table.difference(selected_measures_ids) -@override_settings(MEASURES_ASYNC_EDIT=False) + def test_multiple_measure_edit_only_quota_order_number( valid_user_client, user_workbasket, @@ -2335,7 +2334,7 @@ def test_multiple_measure_edit_only_quota_order_number( assert measure.update_type == UpdateType.UPDATE assert measure.order_number == quota_order_number -@override_settings(MEASURES_ASYNC_EDIT=False) + def test_multiple_measure_edit_only_duties( valid_user_client, user_workbasket, @@ -2406,7 +2405,7 @@ def test_multiple_measure_edit_only_duties( assert measure.update_type == UpdateType.UPDATE assert measure.duty_sentence == duties -@override_settings(MEASURES_ASYNC_EDIT=False) + def test_multiple_measure_edit_preserves_footnote_associations( valid_user_client, user_workbasket, @@ -2482,7 +2481,7 @@ def test_multiple_measure_edit_preserves_footnote_associations( for footnote in measure.footnotes.all(): assert footnote in expected_footnotes -@override_settings(MEASURES_ASYNC_EDIT=False) + def test_multiple_measure_edit_geographical_area_exclusions( valid_user_client, user_workbasket, From bc939fd7058225f8b40c8349f6a9464443f43ec3 Mon Sep 17 00:00:00 2001 From: Lauren Qurashi Date: Mon, 16 Sep 2024 17:02:09 +0100 Subject: [PATCH 25/38] Fix measure/test_views.py tests --- measures/tests/test_views.py | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/measures/tests/test_views.py b/measures/tests/test_views.py index b987e3221..2d3ec5b5b 100644 --- a/measures/tests/test_views.py +++ b/measures/tests/test_views.py @@ -1927,7 +1927,6 @@ def test_multiple_measure_start_and_end_date_edit_functionality( assert measure.valid_between.upper == datetime.date(2100, 1, 1) assert measure.effective_end_date == datetime.date(2100, 1, 1) - @pytest.mark.parametrize( "step, form_data, updated_attribute, expected_data", [ @@ -1966,6 +1965,7 @@ def test_multiple_measure_start_and_end_date_edit_functionality( ), ], ) +@override_settings(MEASURES_ASYNC_EDIT=False) def test_multiple_measure_edit_single_form_functionality( step, form_data, @@ -2038,7 +2038,7 @@ def test_multiple_measure_edit_single_form_functionality( assert measure.update_type == UpdateType.UPDATE assert reduce(getattr, updated_attribute.split("."), measure) == expected_data - +@override_settings(MEASURES_ASYNC_EDIT=False) def test_multiple_measure_edit_only_regulation( valid_user_client, user_workbasket, @@ -2263,7 +2263,7 @@ def test_measure_list_selected_measures_list(valid_user_client): assert not measure_ids_in_table.difference(selected_measures_ids) - +@override_settings(MEASURES_ASYNC_EDIT=False) def test_multiple_measure_edit_only_quota_order_number( valid_user_client, user_workbasket, @@ -2334,7 +2334,7 @@ def test_multiple_measure_edit_only_quota_order_number( assert measure.update_type == UpdateType.UPDATE assert measure.order_number == quota_order_number - +@override_settings(MEASURES_ASYNC_EDIT=False) def test_multiple_measure_edit_only_duties( valid_user_client, user_workbasket, @@ -2405,7 +2405,7 @@ def test_multiple_measure_edit_only_duties( assert measure.update_type == UpdateType.UPDATE assert measure.duty_sentence == duties - +@override_settings(MEASURES_ASYNC_EDIT=False) def test_multiple_measure_edit_preserves_footnote_associations( valid_user_client, user_workbasket, @@ -2481,7 +2481,7 @@ def test_multiple_measure_edit_preserves_footnote_associations( for footnote in measure.footnotes.all(): assert footnote in expected_footnotes - +@override_settings(MEASURES_ASYNC_EDIT=False) def test_multiple_measure_edit_geographical_area_exclusions( valid_user_client, user_workbasket, From 4fb2f0bb2e8bcb8c199fa6eb685e86ba67f62d88 Mon Sep 17 00:00:00 2001 From: Lauren Qurashi Date: Tue, 17 Sep 2024 11:34:24 +0100 Subject: [PATCH 26/38] Modify edit tests to be sync tests --- measures/tests/test_views.py | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/measures/tests/test_views.py b/measures/tests/test_views.py index 2d3ec5b5b..73aaea52c 100644 --- a/measures/tests/test_views.py +++ b/measures/tests/test_views.py @@ -57,8 +57,8 @@ @pytest.fixture() -def mocked_diff_components(): - """Mocks `diff_components()` inside `update_measure_components()` in +def mocked_sync_diff_components(): + """Mocks `diff_components()` inside `update_measure_components()` that is called in `MeasureEditWizard` to prevent parsing errors where test measures lack a duty sentence.""" with patch( @@ -1837,7 +1837,7 @@ def test_measuretype_api_list_view(valid_user_client): def test_multiple_measure_start_and_end_date_edit_functionality( valid_user_client, user_workbasket, - mocked_diff_components, + mocked_sync_diff_components, ): """Tests that MeasureEditWizard takes a list of measures, and sets their update type to update, updates their end dates and start dates, and clears @@ -1973,7 +1973,7 @@ def test_multiple_measure_edit_single_form_functionality( expected_data, valid_user_client, user_workbasket, - mocked_diff_components, + mocked_sync_diff_components, ): """Tests that MeasureEditWizard takes a list of measures, and sets their update type to update, updates their end dates and start dates, and clears @@ -2042,7 +2042,7 @@ def test_multiple_measure_edit_single_form_functionality( def test_multiple_measure_edit_only_regulation( valid_user_client, user_workbasket, - mocked_diff_components, + mocked_sync_diff_components, ): """Tests the regulation step in MeasureEditWizard.""" measure_1 = factories.MeasureFactory.create() @@ -2267,7 +2267,7 @@ def test_measure_list_selected_measures_list(valid_user_client): def test_multiple_measure_edit_only_quota_order_number( valid_user_client, user_workbasket, - mocked_diff_components, + mocked_sync_diff_components, ): """Tests the regulation step in MeasureEditWizard.""" measure_1 = factories.MeasureFactory.create() @@ -2409,7 +2409,7 @@ def test_multiple_measure_edit_only_duties( def test_multiple_measure_edit_preserves_footnote_associations( valid_user_client, user_workbasket, - mocked_diff_components, + mocked_sync_diff_components, ): """Tests that footnote associations are preserved in MeasureEditWizard.""" @@ -2485,7 +2485,7 @@ def test_multiple_measure_edit_preserves_footnote_associations( def test_multiple_measure_edit_geographical_area_exclusions( valid_user_client, user_workbasket, - mocked_diff_components, + mocked_sync_diff_components, ): """Tests that the geographical area exclusions of multiple measures can be edited in `MeasureEditWizard`.""" From 575f606cfaa64aa144359faa6338a99445bc252b Mon Sep 17 00:00:00 2001 From: Lauren Qurashi Date: Tue, 17 Sep 2024 13:46:45 +0100 Subject: [PATCH 27/38] Refactor editing measures into a editor class so that both done routes can use it --- measures/editors.py | 110 +++++++++++++++++++++++++++++ measures/models/bulk_processing.py | 2 + measures/tests/test_views.py | 2 +- measures/views/wizard.py | 80 ++++++--------------- 4 files changed, 135 insertions(+), 59 deletions(-) create mode 100644 measures/editors.py diff --git a/measures/editors.py b/measures/editors.py new file mode 100644 index 000000000..3a5f79e86 --- /dev/null +++ b/measures/editors.py @@ -0,0 +1,110 @@ +from typing import Dict +from typing import List + +from django.db.transaction import atomic + +from measures.models import Measure +from workbaskets.models import WorkBasket +from common.util import TaricDateRange +from common.validators import UpdateType +from common.models.utils import override_current_transaction +from measures.util import update_measure_components +from measures.util import update_measure_condition_components +from measures.util import update_measure_excluded_geographical_areas +from measures.util import update_measure_footnote_associations + +class MeasuresEditor: + """Utility class used to edit measures from measures wizard accumulated + data.""" + + workbasket: WorkBasket + """The workbasket with which created measures will be associated.""" + + selected_measures: List + """ The measures in which the edits will apply to.""" + + data: Dict + """Validated, cleaned and accumulated data created by the Form instances of + `MeasureEditWizard`.""" + + def __init__(self, workbasket: WorkBasket, selected_measures: List, data: Dict): + self.workbasket = workbasket + self.selected_measures = selected_measures + self.data = data + + @atomic + def edit_measures(self) -> List[Measure]: + """ + Returns a list of the edited measures. + + `data` must be a dictionary + of the accumulated cleaned / validated data created from the + `MeasureEditWizard`. + """ + + with override_current_transaction( + transaction=self.workbasket.current_transaction, + ): + new_start_date = self.data.get("start_date", None) + new_end_date = self.data.get("end_date", False) + new_quota_order_number = self.data.get("order_number", None) + new_generating_regulation = self.data.get("generating_regulation", None) + new_duties = self.data.get("duties", None) + new_exclusions = [ + e["excluded_area"] + for e in self.data.get("formset-geographical_area_exclusions", []) + ] + + edited_measures = [] + + for measure in self.selected_measures: + new_measure = measure.new_version( + workbasket=self.workbasket, + update_type=UpdateType.UPDATE, + valid_between=TaricDateRange( + lower=( + new_start_date + if new_start_date + else measure.valid_between.lower + ), + upper=( + new_end_date + if new_end_date + else measure.valid_between.upper + ), + ), + order_number=( + new_quota_order_number + if new_quota_order_number + else measure.order_number + ), + generating_regulation=( + new_generating_regulation + if new_generating_regulation + else measure.generating_regulation + ), + ) + update_measure_components( + measure=new_measure, + duties=new_duties, + workbasket=self.workbasket, + ) + update_measure_condition_components( + measure=new_measure, + workbasket=self.workbasket, + ) + update_measure_excluded_geographical_areas( + edited="geographical_area_exclusions" + in self.data.get("fields_to_edit", []), + measure=new_measure, + exclusions=new_exclusions, + workbasket=self.workbasket, + ) + update_measure_footnote_associations( + measure=new_measure, + workbasket=self.workbasket, + ) + + edited_measures.append(new_measure.id) + + return edited_measures diff --git a/measures/models/bulk_processing.py b/measures/models/bulk_processing.py index c939b2331..366db03e7 100644 --- a/measures/models/bulk_processing.py +++ b/measures/models/bulk_processing.py @@ -515,6 +515,8 @@ def edit_measures(self) -> Iterable[Measure]: cleaned_data = self.get_forms_cleaned_data() deserialized_selected_measures = Measure.objects.filter(pk__in=self.selected_measures) + + new_start_date = cleaned_data.get("start_date", None) new_end_date = cleaned_data.get("end_date", False) new_quota_order_number = cleaned_data.get("order_number", None) diff --git a/measures/tests/test_views.py b/measures/tests/test_views.py index 73aaea52c..250b0843c 100644 --- a/measures/tests/test_views.py +++ b/measures/tests/test_views.py @@ -62,7 +62,7 @@ def mocked_sync_diff_components(): `MeasureEditWizard` to prevent parsing errors where test measures lack a duty sentence.""" with patch( - "measures.views.wizard.update_measure_components", + "measures.editors.update_measure_components", ) as update_measure_components: yield update_measure_components diff --git a/measures/views/wizard.py b/measures/views/wizard.py index 0d0cb94bd..8dd21850c 100644 --- a/measures/views/wizard.py +++ b/measures/views/wizard.py @@ -27,6 +27,7 @@ from measures.constants import START from measures.constants import MeasureEditSteps from measures.creators import MeasuresCreator +from measures.editors import MeasuresEditor from measures.util import update_measure_components from measures.util import update_measure_condition_components from measures.util import update_measure_excluded_geographical_areas @@ -153,6 +154,14 @@ def done(self, form_list, **kwargs): else: return self.sync_done(form_list, **kwargs) + def edit_measures(self, selected_measures, cleaned_data): + """Synchronously edit measures within the context of the view / web + worker using accumulated data, `cleaned_data`, from all the necessary + wizard forms.""" + + measures_editor = MeasuresEditor(self.workbasket, selected_measures, cleaned_data) + return measures_editor.edit_measures() + def async_done(self, form_list, **kwargs): logger.info("Editing measures asynchronously.") serializable_data = self.all_serializable_form_data() @@ -180,71 +189,26 @@ def async_done(self, form_list, **kwargs): ) def sync_done(self, form_list, **kwargs): + """ + Handles this wizard's done step to edit measures within the context of + the web worker process. + + Because bulk editing measures can be computationally expensive, this can + take an excessive amount of time within the context of HTTP request + processing. + """ + logger.info("Editing measures synchronously.") + cleaned_data = self.get_all_cleaned_data() selected_measures = self.get_queryset() - workbasket = WorkBasket.current(self.request) - new_start_date = cleaned_data.get("start_date", None) - new_end_date = cleaned_data.get("end_date", False) - new_quota_order_number = cleaned_data.get("order_number", None) - new_generating_regulation = cleaned_data.get("generating_regulation", None) - new_duties = cleaned_data.get("duties", None) - new_exclusions = [ - e["excluded_area"] - for e in cleaned_data.get("formset-geographical_area_exclusions", []) - ] - for measure in selected_measures: - new_measure = measure.new_version( - workbasket=workbasket, - update_type=UpdateType.UPDATE, - valid_between=TaricDateRange( - lower=( - new_start_date - if new_start_date - else measure.valid_between.lower - ), - upper=( - new_end_date - if new_end_date is not False - else measure.valid_between.upper - ), - ), - order_number=( - new_quota_order_number - if new_quota_order_number - else measure.order_number - ), - generating_regulation=( - new_generating_regulation - if new_generating_regulation - else measure.generating_regulation - ), - ) - update_measure_components( - measure=new_measure, - duties=new_duties, - workbasket=workbasket, - ) - update_measure_condition_components( - measure=new_measure, - workbasket=workbasket, - ) - update_measure_excluded_geographical_areas( - edited="geographical_area_exclusions" - in cleaned_data.get("fields_to_edit", []), - measure=new_measure, - exclusions=new_exclusions, - workbasket=workbasket, - ) - update_measure_footnote_associations( - measure=new_measure, - workbasket=workbasket, - ) + + self.edit_measures(selected_measures, cleaned_data) self.session_store.clear() return redirect( reverse( "workbaskets:workbasket-ui-review-measures", - kwargs={"pk": workbasket.pk}, + kwargs={"pk": self.workbasket.pk}, ), ) From 92ca80fd22bd5107d765b50b7cce3fd234cb6e33 Mon Sep 17 00:00:00 2001 From: Lauren Qurashi Date: Tue, 17 Sep 2024 14:04:36 +0100 Subject: [PATCH 28/38] Hook the async route up to the new editor class --- measures/editors.py | 99 +++++++++++++++--------------- measures/models/bulk_processing.py | 85 +++---------------------- measures/views/wizard.py | 23 +++---- 3 files changed, 66 insertions(+), 141 deletions(-) diff --git a/measures/editors.py b/measures/editors.py index 3a5f79e86..c990dc536 100644 --- a/measures/editors.py +++ b/measures/editors.py @@ -56,55 +56,56 @@ def edit_measures(self) -> List[Measure]: ] edited_measures = [] - - for measure in self.selected_measures: - new_measure = measure.new_version( - workbasket=self.workbasket, - update_type=UpdateType.UPDATE, - valid_between=TaricDateRange( - lower=( - new_start_date - if new_start_date - else measure.valid_between.lower + + if self.selected_measures: + for measure in self.selected_measures: + new_measure = measure.new_version( + workbasket=self.workbasket, + update_type=UpdateType.UPDATE, + valid_between=TaricDateRange( + lower=( + new_start_date + if new_start_date + else measure.valid_between.lower + ), + upper=( + new_end_date + if new_end_date + else measure.valid_between.upper + ), + ), + order_number=( + new_quota_order_number + if new_quota_order_number + else measure.order_number ), - upper=( - new_end_date - if new_end_date - else measure.valid_between.upper + generating_regulation=( + new_generating_regulation + if new_generating_regulation + else measure.generating_regulation ), - ), - order_number=( - new_quota_order_number - if new_quota_order_number - else measure.order_number - ), - generating_regulation=( - new_generating_regulation - if new_generating_regulation - else measure.generating_regulation - ), - ) - update_measure_components( - measure=new_measure, - duties=new_duties, - workbasket=self.workbasket, - ) - update_measure_condition_components( - measure=new_measure, - workbasket=self.workbasket, - ) - update_measure_excluded_geographical_areas( - edited="geographical_area_exclusions" - in self.data.get("fields_to_edit", []), - measure=new_measure, - exclusions=new_exclusions, - workbasket=self.workbasket, - ) - update_measure_footnote_associations( - measure=new_measure, - workbasket=self.workbasket, - ) - - edited_measures.append(new_measure.id) + ) + update_measure_components( + measure=new_measure, + duties=new_duties, + workbasket=self.workbasket, + ) + update_measure_condition_components( + measure=new_measure, + workbasket=self.workbasket, + ) + update_measure_excluded_geographical_areas( + edited="geographical_area_exclusions" + in self.data.get("fields_to_edit", []), + measure=new_measure, + exclusions=new_exclusions, + workbasket=self.workbasket, + ) + update_measure_footnote_associations( + measure=new_measure, + workbasket=self.workbasket, + ) + + edited_measures.append(new_measure.id) - return edited_measures + return edited_measures diff --git a/measures/models/bulk_processing.py b/measures/models/bulk_processing.py index 366db03e7..ad8247a5a 100644 --- a/measures/models/bulk_processing.py +++ b/measures/models/bulk_processing.py @@ -17,13 +17,8 @@ from common.celery import app from common.models.mixins import TimestampedMixin from common.models.utils import override_current_transaction -from common.util import TaricDateRange -from common.validators import UpdateType from measures.models.tracked_models import Measure -from measures.util import update_measure_components -from measures.util import update_measure_condition_components -from measures.util import update_measure_excluded_geographical_areas -from measures.util import update_measure_footnote_associations +from measures.editors import MeasuresEditor logger = logging.getLogger(__name__) @@ -509,77 +504,13 @@ def schedule_task(self) -> AsyncResult: def edit_measures(self) -> Iterable[Measure]: logger.info("INSIDE EDIT MEASURES - BULK PROCESSING") - with override_current_transaction( - transaction=self.workbasket.current_transaction, - ): - cleaned_data = self.get_forms_cleaned_data() - deserialized_selected_measures = Measure.objects.filter(pk__in=self.selected_measures) - - - - new_start_date = cleaned_data.get("start_date", None) - new_end_date = cleaned_data.get("end_date", False) - new_quota_order_number = cleaned_data.get("order_number", None) - new_generating_regulation = cleaned_data.get("generating_regulation", None) - new_duties = cleaned_data.get("duties", None) - new_exclusions = [ - e["excluded_area"] - for e in cleaned_data.get("formset-geographical_area_exclusions", []) - ] - - if deserialized_selected_measures: - edited_measures = [] - - for measure in deserialized_selected_measures: - new_measure = measure.new_version( - workbasket=self.workbasket, - update_type=UpdateType.UPDATE, - valid_between=TaricDateRange( - lower=( - new_start_date - if new_start_date - else measure.valid_between.lower - ), - upper=( - new_end_date - if new_end_date - else measure.valid_between.upper - ), - ), - order_number=( - new_quota_order_number - if new_quota_order_number - else measure.order_number - ), - generating_regulation=( - new_generating_regulation - if new_generating_regulation - else measure.generating_regulation - ), - ) - update_measure_components( - measure=new_measure, - duties=new_duties, - workbasket=self.workbasket, - ) - update_measure_condition_components( - measure=new_measure, - workbasket=self.workbasket, - ) - update_measure_excluded_geographical_areas( - edited="geographical_area_exclusions" - in cleaned_data.get("fields_to_edit", []), - measure=new_measure, - exclusions=new_exclusions, - workbasket=self.workbasket, - ) - update_measure_footnote_associations( - measure=new_measure, - workbasket=self.workbasket, - ) - edited_measures.append(new_measure.id) - - return edited_measures + cleaned_data = self.get_forms_cleaned_data() + deserialized_selected_measures = Measure.objects.filter(pk__in=self.selected_measures) + + + measures_editor = MeasuresEditor(self.workbasket, deserialized_selected_measures, cleaned_data) + return measures_editor.edit_measures() + def get_forms_cleaned_data(self) -> Dict: """ diff --git a/measures/views/wizard.py b/measures/views/wizard.py index 8dd21850c..99ca00279 100644 --- a/measures/views/wizard.py +++ b/measures/views/wizard.py @@ -13,12 +13,9 @@ from django.views.generic import TemplateView from formtools.wizard.views import NamedUrlSessionWizardView -from common.util import TaricDateRange -from common.validators import UpdateType from geo_areas import constants from geo_areas.models import GeographicalArea from geo_areas.models import GeographicalMembership -from geo_areas.utils import get_all_members_of_geo_groups from geo_areas.validators import AreaCode from measures import forms from measures import models @@ -28,10 +25,6 @@ from measures.constants import MeasureEditSteps from measures.creators import MeasuresCreator from measures.editors import MeasuresEditor -from measures.util import update_measure_components -from measures.util import update_measure_condition_components -from measures.util import update_measure_excluded_geographical_areas -from measures.util import update_measure_footnote_associations from measures.views.mixins import MeasureSerializableWizardMixin from workbaskets.models import WorkBasket from workbaskets.views.decorators import require_current_workbasket @@ -154,14 +147,6 @@ def done(self, form_list, **kwargs): else: return self.sync_done(form_list, **kwargs) - def edit_measures(self, selected_measures, cleaned_data): - """Synchronously edit measures within the context of the view / web - worker using accumulated data, `cleaned_data`, from all the necessary - wizard forms.""" - - measures_editor = MeasuresEditor(self.workbasket, selected_measures, cleaned_data) - return measures_editor.edit_measures() - def async_done(self, form_list, **kwargs): logger.info("Editing measures asynchronously.") serializable_data = self.all_serializable_form_data() @@ -187,6 +172,14 @@ def async_done(self, form_list, **kwargs): kwargs={"pk": self.workbasket.pk}, ), ) + + def edit_measures(self, selected_measures, cleaned_data): + """Synchronously edit measures within the context of the view / web + worker using accumulated data, `cleaned_data`, from all the necessary + wizard forms.""" + + measures_editor = MeasuresEditor(self.workbasket, selected_measures, cleaned_data) + return measures_editor.edit_measures() def sync_done(self, form_list, **kwargs): """ From a316d9f9b83c3805ad76470f2344e4a7a81e3375 Mon Sep 17 00:00:00 2001 From: Lauren Qurashi Date: Tue, 17 Sep 2024 16:06:30 +0100 Subject: [PATCH 29/38] Change name of mock and fix circular imports --- measures/editors.py | 11 ++++++----- measures/tests/test_views.py | 14 +++++++------- 2 files changed, 13 insertions(+), 12 deletions(-) diff --git a/measures/editors.py b/measures/editors.py index c990dc536..d2ebbeffc 100644 --- a/measures/editors.py +++ b/measures/editors.py @@ -1,10 +1,11 @@ from typing import Dict from typing import List +from typing import Type from django.db.transaction import atomic -from measures.models import Measure -from workbaskets.models import WorkBasket +from workbaskets import models as workbasket_models +from measures import models as measure_models from common.util import TaricDateRange from common.validators import UpdateType from common.models.utils import override_current_transaction @@ -17,7 +18,7 @@ class MeasuresEditor: """Utility class used to edit measures from measures wizard accumulated data.""" - workbasket: WorkBasket + workbasket: Type["workbasket_models.WorkBasket"] """The workbasket with which created measures will be associated.""" selected_measures: List @@ -27,13 +28,13 @@ class MeasuresEditor: """Validated, cleaned and accumulated data created by the Form instances of `MeasureEditWizard`.""" - def __init__(self, workbasket: WorkBasket, selected_measures: List, data: Dict): + def __init__(self, workbasket: Type["workbasket_models.WorkBasket"], selected_measures: List, data: Dict): self.workbasket = workbasket self.selected_measures = selected_measures self.data = data @atomic - def edit_measures(self) -> List[Measure]: + def edit_measures(self) -> List["measure_models.Measure"]: """ Returns a list of the edited measures. diff --git a/measures/tests/test_views.py b/measures/tests/test_views.py index 250b0843c..5efa390d5 100644 --- a/measures/tests/test_views.py +++ b/measures/tests/test_views.py @@ -57,7 +57,7 @@ @pytest.fixture() -def mocked_sync_diff_components(): +def mocked_diff_components(): """Mocks `diff_components()` inside `update_measure_components()` that is called in `MeasureEditWizard` to prevent parsing errors where test measures lack a duty sentence.""" @@ -1837,7 +1837,7 @@ def test_measuretype_api_list_view(valid_user_client): def test_multiple_measure_start_and_end_date_edit_functionality( valid_user_client, user_workbasket, - mocked_sync_diff_components, + mocked_diff_components, ): """Tests that MeasureEditWizard takes a list of measures, and sets their update type to update, updates their end dates and start dates, and clears @@ -1973,7 +1973,7 @@ def test_multiple_measure_edit_single_form_functionality( expected_data, valid_user_client, user_workbasket, - mocked_sync_diff_components, + mocked_diff_components, ): """Tests that MeasureEditWizard takes a list of measures, and sets their update type to update, updates their end dates and start dates, and clears @@ -2042,7 +2042,7 @@ def test_multiple_measure_edit_single_form_functionality( def test_multiple_measure_edit_only_regulation( valid_user_client, user_workbasket, - mocked_sync_diff_components, + mocked_diff_components, ): """Tests the regulation step in MeasureEditWizard.""" measure_1 = factories.MeasureFactory.create() @@ -2267,7 +2267,7 @@ def test_measure_list_selected_measures_list(valid_user_client): def test_multiple_measure_edit_only_quota_order_number( valid_user_client, user_workbasket, - mocked_sync_diff_components, + mocked_diff_components, ): """Tests the regulation step in MeasureEditWizard.""" measure_1 = factories.MeasureFactory.create() @@ -2409,7 +2409,7 @@ def test_multiple_measure_edit_only_duties( def test_multiple_measure_edit_preserves_footnote_associations( valid_user_client, user_workbasket, - mocked_sync_diff_components, + mocked_diff_components, ): """Tests that footnote associations are preserved in MeasureEditWizard.""" @@ -2485,7 +2485,7 @@ def test_multiple_measure_edit_preserves_footnote_associations( def test_multiple_measure_edit_geographical_area_exclusions( valid_user_client, user_workbasket, - mocked_sync_diff_components, + mocked_diff_components, ): """Tests that the geographical area exclusions of multiple measures can be edited in `MeasureEditWizard`.""" From dba7e6c4924285e5dddd1ac0837ce8980c39f4f8 Mon Sep 17 00:00:00 2001 From: Lauren Qurashi Date: Wed, 18 Sep 2024 17:19:02 +0100 Subject: [PATCH 30/38] Test bulk processing --- measures/models/bulk_processing.py | 15 ++- measures/tests/conftest.py | 26 ++++- measures/tests/factories.py | 12 ++ measures/tests/test_bulk_processing.py | 154 ++++++++++++++++++++++++- 4 files changed, 195 insertions(+), 12 deletions(-) diff --git a/measures/models/bulk_processing.py b/measures/models/bulk_processing.py index ad8247a5a..e88501dcb 100644 --- a/measures/models/bulk_processing.py +++ b/measures/models/bulk_processing.py @@ -504,12 +504,15 @@ def schedule_task(self) -> AsyncResult: def edit_measures(self) -> Iterable[Measure]: logger.info("INSIDE EDIT MEASURES - BULK PROCESSING") - cleaned_data = self.get_forms_cleaned_data() - deserialized_selected_measures = Measure.objects.filter(pk__in=self.selected_measures) - - - measures_editor = MeasuresEditor(self.workbasket, deserialized_selected_measures, cleaned_data) - return measures_editor.edit_measures() + with override_current_transaction( + transaction=self.workbasket.current_transaction, + ): + cleaned_data = self.get_forms_cleaned_data() + deserialized_selected_measures = Measure.objects.filter(pk__in=self.selected_measures) + + logger.info(f"GET FORMS CLEANED DATA: {cleaned_data}") + measures_editor = MeasuresEditor(self.workbasket, deserialized_selected_measures, cleaned_data) + return measures_editor.edit_measures() def get_forms_cleaned_data(self) -> Dict: diff --git a/measures/tests/conftest.py b/measures/tests/conftest.py index 474d0bf16..ee751234f 100644 --- a/measures/tests/conftest.py +++ b/measures/tests/conftest.py @@ -503,9 +503,33 @@ def simple_measures_bulk_creator( @pytest.fixture() -def mocked_schedule_apply_async(): +def mocked_create_schedule_apply_async(): with patch( "measures.tasks.bulk_create_measures.apply_async", return_value=MagicMock(id=faker.Faker().uuid4()), ) as apply_async_mock: yield apply_async_mock + + +@pytest.fixture() +def simple_measures_bulk_editor( + user_empty_workbasket, + approved_transaction, +): + from measures.tests.factories import MeasuresBulkEditorFactory + + return MeasuresBulkEditorFactory.create( + form_data={}, + form_kwargs={}, + workbasket=user_empty_workbasket, + selected_measures=[], + user=None, + ) + +@pytest.fixture() +def mocked_edit_schedule_apply_async(): + with patch( + "measures.tasks.bulk_edit_measures.apply_async", + return_value=MagicMock(id=faker.Faker().uuid4()), + ) as apply_async_mock: + yield apply_async_mock \ No newline at end of file diff --git a/measures/tests/factories.py b/measures/tests/factories.py index 082d9b814..6b8301fad 100644 --- a/measures/tests/factories.py +++ b/measures/tests/factories.py @@ -106,3 +106,15 @@ class Meta: workbasket = factory.SubFactory(factories.WorkBasketFactory) form_data = {} form_kwargs = {} + + +class MeasuresBulkEditorFactory(factory.django.DjangoModelFactory): + class Meta: + model = "measures.MeasuresBulkEditor" + + user = factory.SubFactory(factories.UserFactory) + created_at = factory.Faker("date_object") + workbasket = factory.SubFactory(factories.WorkBasketFactory) + form_data = {} + form_kwargs = {} + selected_measures = [] diff --git a/measures/tests/test_bulk_processing.py b/measures/tests/test_bulk_processing.py index 189cca0ac..8336ffd99 100644 --- a/measures/tests/test_bulk_processing.py +++ b/measures/tests/test_bulk_processing.py @@ -10,8 +10,10 @@ from common.util import TaricDateRange from common.validators import ApplicabilityCode from measures.models import MeasuresBulkCreator +from measures.models import MeasuresBulkEditor from measures.models import ProcessingState from measures.tests.factories import MeasuresBulkCreatorFactory +from measures.tests.factories import MeasuresBulkEditorFactory from measures.validators import MeasureExplosionLevel pytestmark = pytest.mark.django_db @@ -19,14 +21,14 @@ def test_schedule_task_bulk_measures_create( simple_measures_bulk_creator, - mocked_schedule_apply_async, + mocked_create_schedule_apply_async, ): - """Test that calling MeasuresBulkCreator.shedule() correctly schedules a + """Test that calling MeasuresBulkCreator.schedule() correctly schedules a Celery task.""" simple_measures_bulk_creator.schedule_task() - mocked_schedule_apply_async.assert_called_once_with( + mocked_create_schedule_apply_async.assert_called_once_with( kwargs={ "measures_bulk_creator_pk": simple_measures_bulk_creator.pk, }, @@ -34,9 +36,26 @@ def test_schedule_task_bulk_measures_create( ) +def test_schedule_task_bulk_measures_edit( + simple_measures_bulk_editor, + mocked_edit_schedule_apply_async, +): + """Test that calling MeasuresBulkCreator.schedule() correctly schedules a + Celery task.""" + + simple_measures_bulk_editor.schedule_task() + + mocked_edit_schedule_apply_async.assert_called_once_with( + kwargs={ + "measures_bulk_editor_pk": simple_measures_bulk_editor.pk, + }, + countdown=ANY, + ) + + def test_REVOKE_TASKS_AND_SET_NULL( simple_measures_bulk_creator, - mocked_schedule_apply_async, + mocked_create_schedule_apply_async, ): """Test that deleting an object, referenced by a ForeignKey field that has `on_delete=BulkProcessor.REVOKE_TASKS_AND_SET_NULL`, correctly revokes any @@ -57,7 +76,7 @@ def test_REVOKE_TASKS_AND_SET_NULL( def test_cancel_task( simple_measures_bulk_creator, - mocked_schedule_apply_async, + mocked_create_schedule_apply_async, ): """Test BulkProcessor.cancel_task() behaviours correctly apply.""" @@ -167,6 +186,74 @@ def test_bulk_creator_get_forms_cleaned_data( } +# Run the form and get the form data from the sync done +@patch("measures.parsers.DutySentenceParser") +def test_bulk_editor_get_forms_cleaned_data( + mock_duty_sentence_parser, + user_empty_workbasket, + duty_sentence_parser, +): + + mock_duty_sentence_parser.return_value = duty_sentence_parser + + geo_area1 = factories.GeographicalAreaFactory.create() + geo_area2 = factories.GeographicalAreaFactory.create() + measure_1 = factories.MeasureFactory.create() + measure_2 = factories.MeasureFactory.create() + measure_3 = factories.MeasureFactory.create() + regulation = factories.RegulationFactory() + order_number = factories.QuotaOrderNumberFactory.create() + + selected_measures = [measure_1.pk, measure_2.pk, measure_3.pk] + + form_data = { + "start_date": { + "start_date_0": 1, + "start_date_1": 1, + "start_date_2": 2023, + }, + "end_date": { + "end_date_0": 2, + "end_date_1": 2, + "end_date_2": 2026, + }, + "quota_order_number": {"order_number": order_number.pk}, + "regulation": {"generating_regulation": regulation.pk}, + "duties": {"duties": '4%'}, + "geographical_area_exclusions": { + "form-0-excluded_area": geo_area1.pk, + "form-1-excluded_area": geo_area2.pk + } + } + + form_kwargs = { + "start_date": {'selected_measures': selected_measures}, + "end_date": {'selected_measures': selected_measures}, + "quota_order_number": {}, + "regulation": {'selected_measures': selected_measures}, + "duties": {'selected_measures': selected_measures}, + "geographical_area_exclusions": {}, + } + + mock_bulk_editor = MeasuresBulkEditorFactory.create( + form_data=form_data, + form_kwargs=form_kwargs, + workbasket=user_empty_workbasket, + selected_measures=selected_measures, + user=None, + ) + with override_current_transaction(user_empty_workbasket.current_transaction): + data = mock_bulk_editor.get_forms_cleaned_data() + assert data == { + "start_date": datetime.date(2023, 1, 1), + "end_date": datetime.date(2026, 2, 2), + "generating_regulation": regulation, + "order_number": order_number, + "duties": '4%', + "formset-geographical_area_exclusions": [{'excluded_area': geo_area1, 'DELETE': False }, {'excluded_area': geo_area2, 'DELETE': False }] + } + + @patch("measures.parsers.DutySentenceParser") @patch("measures.forms.wizard.LarkDutySentenceParser") def test_bulk_creator_get_forms_cleaned_data_errors( @@ -225,3 +312,60 @@ def test_bulk_creator_get_forms_cleaned_data_errors( with override_current_transaction(user_empty_workbasket.current_transaction): with pytest.raises(ValidationError): mock_bulk_creator.get_forms_cleaned_data() + + +@patch("measures.parsers.DutySentenceParser") +def test_bulk_editor_get_forms_cleaned_data_errors( + mock_duty_sentence_parser, + user_empty_workbasket, + duty_sentence_parser, +): + mock_duty_sentence_parser.return_value = duty_sentence_parser + + measure_1 = factories.MeasureFactory.create() + measure_2 = factories.MeasureFactory.create() + measure_3 = factories.MeasureFactory.create() + + selected_measures = [measure_1.pk, measure_2.pk, measure_3.pk] + + form_data = { + "start_date": { + "start_date_0": "", + "start_date_1": "", + "start_date_2": "", + }, + "end_date": { + "end_date_0": "", + "end_date_1": "", + "end_date_2": "", + }, + "quota_order_number": {"order_number": ""}, + "regulation": {"generating_regulation": ""}, + "duties": {"duties": ''}, + "geographical_area_exclusions": { + "form-0-excluded_area": "", + "form-1-excluded_area": "" + } + } + + form_kwargs = { + "start_date": {'selected_measures': selected_measures}, + "end_date": {'selected_measures': selected_measures}, + "quota_order_number": {}, + "regulation": {'selected_measures': selected_measures}, + "duties": {'selected_measures': selected_measures}, + "geographical_area_exclusions": {}, + } + + mock_bulk_editor = MeasuresBulkEditorFactory.create( + form_data=form_data, + form_kwargs=form_kwargs, + workbasket=user_empty_workbasket, + selected_measures=selected_measures, + user=None, + ) + + with override_current_transaction(user_empty_workbasket.current_transaction): + with pytest.raises(ValidationError): + mock_bulk_editor.get_forms_cleaned_data() + From 12c1846f63bc228a441ec1b324831d139a98c199 Mon Sep 17 00:00:00 2001 From: Lauren Qurashi Date: Wed, 18 Sep 2024 17:20:18 +0100 Subject: [PATCH 31/38] Take out log --- measures/models/bulk_processing.py | 1 - 1 file changed, 1 deletion(-) diff --git a/measures/models/bulk_processing.py b/measures/models/bulk_processing.py index e88501dcb..1f8955167 100644 --- a/measures/models/bulk_processing.py +++ b/measures/models/bulk_processing.py @@ -510,7 +510,6 @@ def edit_measures(self) -> Iterable[Measure]: cleaned_data = self.get_forms_cleaned_data() deserialized_selected_measures = Measure.objects.filter(pk__in=self.selected_measures) - logger.info(f"GET FORMS CLEANED DATA: {cleaned_data}") measures_editor = MeasuresEditor(self.workbasket, deserialized_selected_measures, cleaned_data) return measures_editor.edit_measures() From 10545141de7c7eebee76008930bd1c72279f4a68 Mon Sep 17 00:00:00 2001 From: Lauren Qurashi Date: Thu, 19 Sep 2024 22:30:40 +0100 Subject: [PATCH 32/38] Test for Paul to look at --- measures/tests/conftest.py | 18 ++++++ measures/tests/test_forms.py | 122 +++++++++++++++++++++++++++++++++++ 2 files changed, 140 insertions(+) diff --git a/measures/tests/conftest.py b/measures/tests/conftest.py index ee751234f..83ee654ba 100644 --- a/measures/tests/conftest.py +++ b/measures/tests/conftest.py @@ -324,12 +324,25 @@ def mock_request(rf, valid_user, valid_user_client): request.requests_session = requests.Session() return request +@pytest.fixture() +def measure_edit_start_date_form_data(): + return {"start_date": datetime.date(2023, 1, 1)} + + +@pytest.fixture() +def measure_edit_end_date_form_data(): + return {"end_date": datetime.date(2026, 2, 2)} + @pytest.fixture() def measure_regulation_id_form_data(): return {"generating_regulation": factories.RegulationFactory.create().pk} +@pytest.fixture() +def measure_edit_regulation_form_data(): + return {"generating_regulation": factories.RegulationFactory.create()} + @pytest.fixture() def measure_details_form_data(date_ranges): return { @@ -487,6 +500,11 @@ def measure_geo_area_geo_group_exclusions_form_data(erga_omnes): } +@pytest.fixture() +def measure_edit_duties_form_data(): + return {"duties": '4%'} + + @pytest.fixture() def simple_measures_bulk_creator( user_empty_workbasket, diff --git a/measures/tests/test_forms.py b/measures/tests/test_forms.py index ac6123392..c5044a988 100644 --- a/measures/tests/test_forms.py +++ b/measures/tests/test_forms.py @@ -1923,3 +1923,125 @@ def test_measure_forms_geo_area_serialize_deserialize(form_data, request): assert deserialized_form.is_valid() assert type(deserialized_form) == forms.MeasureGeographicalAreaForm assert deserialized_form.data == form.data + + + +# Write these tests + + +@pytest.mark.parametrize( + "form_class, form_data, form_kwargs", + [ + ( + forms.MeasureStartDateForm, + "measure_edit_start_date_form_data", + True, + ), + ( + forms.MeasureEndDateForm, + "measure_edit_end_date_form_data", + True, + ), + ( + forms.MeasureQuotaOrderNumberForm, + "measure_quota_order_number_form_data", + False, + ), + ( + forms.MeasureRegulationForm, + "measure_edit_regulation_form_data", + True, + ), + ( + forms.MeasureDutiesForm, + "measure_edit_duties_form_data", + True, + ), + ], + ids=[ + "measure_edit_start_date_form", + "measure_edit_end_date_form", + "measure_edit_quota_order_number_form", + "measure_edit_regulation_form", + "measure_edit_duties_form", + ], +) +def test_simple_measure_edit_forms_serialize_deserialize(form_class, form_data, form_kwargs, request): + """Test that the EditMeasure simple forms that use the + SerializableFormMixin behave correctly and as expected.""" + + # Create some measures to apply this data to, for the kwargs + measure_1 = factories.MeasureFactory.create() + measure_2 = factories.MeasureFactory.create() + measure_3 = factories.MeasureFactory.create() + selected_measures = [measure_1.pk, measure_2.pk, measure_3.pk] + + # Check the forms are valid on data submission + form_data = request.getfixturevalue(form_data) + if form_kwargs: + form_kwarg_data = { + "kwargs": { + "selected_measures": selected_measures, + }, + }, + form = form_class( + data=form_data, + **form_kwarg_data, + ) + else: + form = form_class(form_data) + + assert form.is_valid() + + # Create the serialized data + serialized_data = form.serializable_data() + if form_kwargs: + serialized_form_kwargs = form.serializable_init_kwargs(form_kwarg_data) + + # Make a form from serialized data + deserialized_form = form_class(data=serialized_data) + + if form_kwargs: + deserialized_form = form_class( + data=serialized_data, + **serialized_form_kwargs, + ) + + + # Check the form is the right type, valid, and the data that went in is the same that comes out + assert type(deserialized_form) == form_class + assert deserialized_form.is_valid() + assert deserialized_form.data == form_data + + +@pytest.mark.parametrize( + "form_data", + [ + ("measure_geo_area_erga_omnes_form_data"), + ("measure_geo_area_erga_omnes_exclusions_form_data"), + ("measure_geo_area_geo_group_form_data"), + ("measure_geo_area_geo_group_exclusions_form_data"), + ], + ids=[ + "erga-omnes", + "erga-omnes-exclusions", + "geo-group", + "geo-group-exclusions", + ], +) +def test_measure_edit_forms_geo_area_exclusions_serialize_deserialize(form_data, request): + form_data = request.getfixturevalue(form_data) + with override_current_transaction(Transaction.objects.last()): + form = forms.MeasureGeographicalAreaForm( + form_data, + ) + assert form.is_valid() + + serializable_form_data = form.serializable_data() + + deserialized_form = forms.MeasureGeographicalAreaForm( + data=serializable_form_data, + ) + assert deserialized_form.is_valid() + assert type(deserialized_form) == forms.MeasureGeographicalAreaForm + assert deserialized_form.data == form.data \ No newline at end of file From 46e3a1e0d315f9e3b3d2b57f0abcc8d22bfaaea2 Mon Sep 17 00:00:00 2001 From: Lauren Qurashi Date: Fri, 20 Sep 2024 14:55:39 +0100 Subject: [PATCH 33/38] Add test to check form serialize and deserialize functionality --- measures/tests/conftest.py | 12 +++++- measures/tests/test_forms.py | 74 +++++++++++++++++++----------------- 2 files changed, 49 insertions(+), 37 deletions(-) diff --git a/measures/tests/conftest.py b/measures/tests/conftest.py index 83ee654ba..0bded8d7a 100644 --- a/measures/tests/conftest.py +++ b/measures/tests/conftest.py @@ -326,12 +326,20 @@ def mock_request(rf, valid_user, valid_user_client): @pytest.fixture() def measure_edit_start_date_form_data(): - return {"start_date": datetime.date(2023, 1, 1)} + return { + "start_date_0": 1, + "start_date_1": 1, + "start_date_2": 2023, + } @pytest.fixture() def measure_edit_end_date_form_data(): - return {"end_date": datetime.date(2026, 2, 2)} + return { + "end_date_0": 2, + "end_date_1": 2, + "end_date_2": 2026, + } @pytest.fixture() diff --git a/measures/tests/test_forms.py b/measures/tests/test_forms.py index c5044a988..b8b8f00e9 100644 --- a/measures/tests/test_forms.py +++ b/measures/tests/test_forms.py @@ -1925,12 +1925,8 @@ def test_measure_forms_geo_area_serialize_deserialize(form_data, request): assert deserialized_form.data == form.data - -# Write these tests - - @pytest.mark.parametrize( - "form_class, form_data, form_kwargs", + "form_class, form_data_fixture, has_form_kwargs", [ ( forms.MeasureStartDateForm, @@ -1966,54 +1962,62 @@ def test_measure_forms_geo_area_serialize_deserialize(form_data, request): "measure_edit_duties_form", ], ) -def test_simple_measure_edit_forms_serialize_deserialize(form_class, form_data, form_kwargs, request): +def test_simple_measure_edit_forms_serialize_deserialize( + form_class, + form_data_fixture, + has_form_kwargs, + date_ranges, + request, + duty_sentence_parser, +): """Test that the EditMeasure simple forms that use the SerializableFormMixin behave correctly and as expected.""" - + # Create some measures to apply this data to, for the kwargs - measure_1 = factories.MeasureFactory.create() - measure_2 = factories.MeasureFactory.create() - measure_3 = factories.MeasureFactory.create() - selected_measures = [measure_1.pk, measure_2.pk, measure_3.pk] + quota_order_number = factories.QuotaOrderNumberFactory() + regulation = factories.RegulationFactory.create() + selected_measures = factories.MeasureFactory.create_batch( + 4, + valid_between=date_ranges.normal, + order_number=quota_order_number, + generating_regulation=regulation, + ) # Check the forms are valid on data submission - form_data = request.getfixturevalue(form_data) - if form_kwargs: + form_data = request.getfixturevalue(form_data_fixture) + form_kwarg_data = {} + + if has_form_kwargs: form_kwarg_data = { - "kwargs": { - "selected_measures": selected_measures, - }, - }, - form = form_class( - data=form_data, - **form_kwarg_data, - ) - else: - form = form_class(form_data) - + "selected_measures": selected_measures, + } + + form = form_class(form_data, **form_kwarg_data) assert form.is_valid() # Create the serialized data serialized_data = form.serializable_data() - if form_kwargs: - serialized_form_kwargs = form.serializable_init_kwargs(form_kwarg_data) + serialized_data_kwargs = {} - # Make a form from serialized data - deserialized_form = form_class(data=serialized_data) - - if form_kwargs: - deserialized_form = form_class( - data=serialized_data, - **serialized_form_kwargs, - ) + if has_form_kwargs: + serialized_data_kwargs = form.serializable_init_kwargs(form_kwarg_data) + # Deserialize the kwargs + deserialized_form_kwargs = form.deserialize_init_kwargs( + serialized_data_kwargs, + ) + + # Make a form from serialized data.Check the form is the right type, valid, and the data that went in is the same that comes out + deserialized_form = form_class( + data=serialized_data, + **deserialized_form_kwargs, + ) # Check the form is the right type, valid, and the data that went in is the same that comes out assert type(deserialized_form) == form_class assert deserialized_form.is_valid() assert deserialized_form.data == form_data - @pytest.mark.parametrize( "form_data", [ From 54be1ce54f17ba240ffbcbf3082cb3ec2b75d4e2 Mon Sep 17 00:00:00 2001 From: Lauren Qurashi Date: Fri, 20 Sep 2024 16:16:58 +0100 Subject: [PATCH 34/38] Add Geo Areas serialize deserialise test --- measures/tests/test_forms.py | 32 ++++++++++++-------------------- 1 file changed, 12 insertions(+), 20 deletions(-) diff --git a/measures/tests/test_forms.py b/measures/tests/test_forms.py index b8b8f00e9..15bc69d44 100644 --- a/measures/tests/test_forms.py +++ b/measures/tests/test_forms.py @@ -2018,34 +2018,26 @@ def test_simple_measure_edit_forms_serialize_deserialize( assert deserialized_form.is_valid() assert deserialized_form.data == form_data -@pytest.mark.parametrize( - "form_data", - [ - ("measure_geo_area_erga_omnes_form_data"), - ("measure_geo_area_erga_omnes_exclusions_form_data"), - ("measure_geo_area_geo_group_form_data"), - ("measure_geo_area_geo_group_exclusions_form_data"), - ], - ids=[ - "erga-omnes", - "erga-omnes-exclusions", - "geo-group", - "geo-group-exclusions", - ], -) -def test_measure_edit_forms_geo_area_exclusions_serialize_deserialize(form_data, request): - form_data = request.getfixturevalue(form_data) + +def test_measure_edit_forms_geo_area_exclusions_serialize_deserialize(): + geo_area1 = factories.GeographicalAreaFactory.create() + geo_area2 = factories.GeographicalAreaFactory.create() + + form_data = { + "form-0-excluded_area": geo_area1, + "form-1-excluded_area": geo_area2 + } with override_current_transaction(Transaction.objects.last()): - form = forms.MeasureGeographicalAreaForm( + form = forms.MeasureGeographicalAreaExclusionsFormSet( form_data, ) assert form.is_valid() serializable_form_data = form.serializable_data() - deserialized_form = forms.MeasureGeographicalAreaForm( + deserialized_form = forms.MeasureGeographicalAreaExclusionsFormSet( data=serializable_form_data, ) assert deserialized_form.is_valid() - assert type(deserialized_form) == forms.MeasureGeographicalAreaForm + assert type(deserialized_form) == forms.MeasureGeographicalAreaExclusionsFormSet assert deserialized_form.data == form.data \ No newline at end of file From c9755e5da21bd31a3c006324614aebe558825c20 Mon Sep 17 00:00:00 2001 From: Lauren Qurashi Date: Mon, 23 Sep 2024 12:08:43 +0100 Subject: [PATCH 35/38] Run linter --- measures/editors.py | 10 +++++-- measures/forms/wizard.py | 22 ++++++++++----- measures/models/bulk_processing.py | 16 +++++++---- measures/tasks.py | 5 ++-- measures/tests/conftest.py | 23 +++++++++------- measures/tests/test_bulk_processing.py | 38 ++++++++++++++------------ measures/tests/test_forms.py | 7 ++--- measures/tests/test_views.py | 7 +++++ measures/util.py | 8 ++++-- measures/views/mixins.py | 7 +++-- measures/views/wizard.py | 13 ++++----- 11 files changed, 93 insertions(+), 63 deletions(-) diff --git a/measures/editors.py b/measures/editors.py index d2ebbeffc..0a55451f2 100644 --- a/measures/editors.py +++ b/measures/editors.py @@ -14,6 +14,7 @@ from measures.util import update_measure_excluded_geographical_areas from measures.util import update_measure_footnote_associations + class MeasuresEditor: """Utility class used to edit measures from measures wizard accumulated data.""" @@ -28,7 +29,12 @@ class MeasuresEditor: """Validated, cleaned and accumulated data created by the Form instances of `MeasureEditWizard`.""" - def __init__(self, workbasket: Type["workbasket_models.WorkBasket"], selected_measures: List, data: Dict): + def __init__( + self, + workbasket: Type["workbasket_models.WorkBasket"], + selected_measures: List, + data: Dict, + ): self.workbasket = workbasket self.selected_measures = selected_measures self.data = data @@ -106,7 +112,7 @@ def edit_measures(self) -> List["measure_models.Measure"]: measure=new_measure, workbasket=self.workbasket, ) - + edited_measures.append(new_measure.id) return edited_measures diff --git a/measures/forms/wizard.py b/measures/forms/wizard.py index c80f89372..d83142df3 100644 --- a/measures/forms/wizard.py +++ b/measures/forms/wizard.py @@ -818,7 +818,7 @@ def clean(self): ) return cleaned_data - + @classmethod def serializable_init_kwargs(cls, kwargs: Dict) -> Dict: selected_measures = kwargs.get("selected_measures") @@ -835,7 +835,9 @@ def serializable_init_kwargs(cls, kwargs: Dict) -> Dict: @classmethod def deserialize_init_kwargs(cls, form_kwargs: Dict) -> Dict: serialized_selected_measures_pks = form_kwargs.get("selected_measures") - deserialized_selected_measures = models.Measure.objects.filter(pk__in=serialized_selected_measures_pks) + deserialized_selected_measures = models.Measure.objects.filter( + pk__in=serialized_selected_measures_pks + ) kwargs = { "selected_measures": deserialized_selected_measures, @@ -884,7 +886,7 @@ def clean(self): cleaned_data["end_date"] = None return cleaned_data - + @classmethod def serializable_init_kwargs(cls, kwargs: Dict) -> Dict: selected_measures = kwargs.get("selected_measures") @@ -901,7 +903,9 @@ def serializable_init_kwargs(cls, kwargs: Dict) -> Dict: @classmethod def deserialize_init_kwargs(cls, form_kwargs: Dict) -> Dict: serialized_selected_measures_pks = form_kwargs.get("selected_measures") - deserialized_selected_measures = models.Measure.objects.filter(pk__in=serialized_selected_measures_pks) + deserialized_selected_measures = models.Measure.objects.filter( + pk__in=serialized_selected_measures_pks + ) kwargs = { "selected_measures": deserialized_selected_measures, @@ -952,7 +956,9 @@ def serializable_init_kwargs(cls, kwargs: Dict) -> Dict: @classmethod def deserialize_init_kwargs(cls, form_kwargs: Dict) -> Dict: serialized_selected_measures_pks = form_kwargs.get("selected_measures") - deserialized_selected_measures = models.Measure.objects.filter(pk__in=serialized_selected_measures_pks) + deserialized_selected_measures = models.Measure.objects.filter( + pk__in=serialized_selected_measures_pks + ) kwargs = { "selected_measures": deserialized_selected_measures, @@ -1003,7 +1009,7 @@ def clean(self): validate_duties(duties, measure.valid_between.lower) return cleaned_data - + @classmethod def serializable_init_kwargs(cls, kwargs: Dict) -> Dict: selected_measures = kwargs.get("selected_measures") @@ -1020,7 +1026,9 @@ def serializable_init_kwargs(cls, kwargs: Dict) -> Dict: @classmethod def deserialize_init_kwargs(cls, form_kwargs: Dict) -> Dict: serialized_selected_measures_pks = form_kwargs.get("selected_measures") - deserialized_selected_measures = models.Measure.objects.filter(pk__in=serialized_selected_measures_pks) + deserialized_selected_measures = models.Measure.objects.filter( + pk__in=serialized_selected_measures_pks + ) kwargs = { "selected_measures": deserialized_selected_measures, diff --git a/measures/models/bulk_processing.py b/measures/models/bulk_processing.py index 1f8955167..908e90d98 100644 --- a/measures/models/bulk_processing.py +++ b/measures/models/bulk_processing.py @@ -508,11 +508,14 @@ def edit_measures(self) -> Iterable[Measure]: transaction=self.workbasket.current_transaction, ): cleaned_data = self.get_forms_cleaned_data() - deserialized_selected_measures = Measure.objects.filter(pk__in=self.selected_measures) - - measures_editor = MeasuresEditor(self.workbasket, deserialized_selected_measures, cleaned_data) - return measures_editor.edit_measures() + deserialized_selected_measures = Measure.objects.filter( + pk__in=self.selected_measures + ) + measures_editor = MeasuresEditor( + self.workbasket, deserialized_selected_measures, cleaned_data + ) + return measures_editor.edit_measures() def get_forms_cleaned_data(self) -> Dict: """ @@ -527,6 +530,7 @@ def get_forms_cleaned_data(self) -> Dict: all_cleaned_data = {} from measures.views import MeasureEditWizard + for form_key, form_class in MeasureEditWizard.data_form_list: if form_key not in self.form_data: @@ -549,7 +553,7 @@ def get_forms_cleaned_data(self) -> Dict: all_cleaned_data[f"formset-{form_key}"] = form.cleaned_data else: all_cleaned_data.update(form.cleaned_data) - + return all_cleaned_data def _log_form_errors(self, form_class, form_or_formset) -> None: @@ -575,4 +579,4 @@ def _log_form_errors(self, form_class, form_or_formset) -> None: for form_errors in errors: for error_key, error_values in form_errors.items(): - logger.error(f"{error_key}: {error_values}") \ No newline at end of file + logger.error(f"{error_key}: {error_values}") diff --git a/measures/tasks.py b/measures/tasks.py index 20387a94d..755e64fc8 100644 --- a/measures/tasks.py +++ b/measures/tasks.py @@ -45,6 +45,7 @@ def bulk_create_measures(measures_bulk_creator_pk: int) -> None: f"WorkBasket({measures_bulk_creator.workbasket.pk}).", ) + @app.task def bulk_edit_measures(measures_bulk_editor_pk: int) -> None: """Bulk edit measures from serialized measures form data saved within an @@ -65,7 +66,7 @@ def bulk_edit_measures(measures_bulk_editor_pk: int) -> None: f"WorkBasket({measures_bulk_editor.workbasket.pk}).", ) raise e - + measures_bulk_editor.processing_succeeded() measures_bulk_editor.successfully_processed_count = len(measures) measures_bulk_editor.save() @@ -75,4 +76,4 @@ def bulk_edit_measures(measures_bulk_editor_pk: int) -> None: f"MeasuresBulkEditor({measures_bulk_editor.pk}) task " f"succeeded in editing {len(measures)} Measures in " f"WorkBasket({measures_bulk_editor.workbasket.pk}).", - ) \ No newline at end of file + ) diff --git a/measures/tests/conftest.py b/measures/tests/conftest.py index 0bded8d7a..df5d8e12c 100644 --- a/measures/tests/conftest.py +++ b/measures/tests/conftest.py @@ -324,22 +324,23 @@ def mock_request(rf, valid_user, valid_user_client): request.requests_session = requests.Session() return request + @pytest.fixture() def measure_edit_start_date_form_data(): return { - "start_date_0": 1, - "start_date_1": 1, - "start_date_2": 2023, - } + "start_date_0": 1, + "start_date_1": 1, + "start_date_2": 2023, + } @pytest.fixture() def measure_edit_end_date_form_data(): return { - "end_date_0": 2, - "end_date_1": 2, - "end_date_2": 2026, - } + "end_date_0": 2, + "end_date_1": 2, + "end_date_2": 2026, + } @pytest.fixture() @@ -351,6 +352,7 @@ def measure_regulation_id_form_data(): def measure_edit_regulation_form_data(): return {"generating_regulation": factories.RegulationFactory.create()} + @pytest.fixture() def measure_details_form_data(date_ranges): return { @@ -510,7 +512,7 @@ def measure_geo_area_geo_group_exclusions_form_data(erga_omnes): @pytest.fixture() def measure_edit_duties_form_data(): - return {"duties": '4%'} + return {"duties": "4%"} @pytest.fixture() @@ -552,10 +554,11 @@ def simple_measures_bulk_editor( user=None, ) + @pytest.fixture() def mocked_edit_schedule_apply_async(): with patch( "measures.tasks.bulk_edit_measures.apply_async", return_value=MagicMock(id=faker.Faker().uuid4()), ) as apply_async_mock: - yield apply_async_mock \ No newline at end of file + yield apply_async_mock diff --git a/measures/tests/test_bulk_processing.py b/measures/tests/test_bulk_processing.py index 8336ffd99..e58f05ec0 100644 --- a/measures/tests/test_bulk_processing.py +++ b/measures/tests/test_bulk_processing.py @@ -186,7 +186,7 @@ def test_bulk_creator_get_forms_cleaned_data( } -# Run the form and get the form data from the sync done +# Run the form and get the form data from the sync done @patch("measures.parsers.DutySentenceParser") def test_bulk_editor_get_forms_cleaned_data( mock_duty_sentence_parser, @@ -219,19 +219,19 @@ def test_bulk_editor_get_forms_cleaned_data( }, "quota_order_number": {"order_number": order_number.pk}, "regulation": {"generating_regulation": regulation.pk}, - "duties": {"duties": '4%'}, + "duties": {"duties": "4%"}, "geographical_area_exclusions": { "form-0-excluded_area": geo_area1.pk, - "form-1-excluded_area": geo_area2.pk - } + "form-1-excluded_area": geo_area2.pk, + }, } form_kwargs = { - "start_date": {'selected_measures': selected_measures}, - "end_date": {'selected_measures': selected_measures}, + "start_date": {"selected_measures": selected_measures}, + "end_date": {"selected_measures": selected_measures}, "quota_order_number": {}, - "regulation": {'selected_measures': selected_measures}, - "duties": {'selected_measures': selected_measures}, + "regulation": {"selected_measures": selected_measures}, + "duties": {"selected_measures": selected_measures}, "geographical_area_exclusions": {}, } @@ -249,8 +249,11 @@ def test_bulk_editor_get_forms_cleaned_data( "end_date": datetime.date(2026, 2, 2), "generating_regulation": regulation, "order_number": order_number, - "duties": '4%', - "formset-geographical_area_exclusions": [{'excluded_area': geo_area1, 'DELETE': False }, {'excluded_area': geo_area2, 'DELETE': False }] + "duties": "4%", + "formset-geographical_area_exclusions": [ + {"excluded_area": geo_area1, "DELETE": False}, + {"excluded_area": geo_area2, "DELETE": False}, + ], } @@ -341,19 +344,19 @@ def test_bulk_editor_get_forms_cleaned_data_errors( }, "quota_order_number": {"order_number": ""}, "regulation": {"generating_regulation": ""}, - "duties": {"duties": ''}, + "duties": {"duties": ""}, "geographical_area_exclusions": { "form-0-excluded_area": "", - "form-1-excluded_area": "" - } + "form-1-excluded_area": "", + }, } form_kwargs = { - "start_date": {'selected_measures': selected_measures}, - "end_date": {'selected_measures': selected_measures}, + "start_date": {"selected_measures": selected_measures}, + "end_date": {"selected_measures": selected_measures}, "quota_order_number": {}, - "regulation": {'selected_measures': selected_measures}, - "duties": {'selected_measures': selected_measures}, + "regulation": {"selected_measures": selected_measures}, + "duties": {"selected_measures": selected_measures}, "geographical_area_exclusions": {}, } @@ -368,4 +371,3 @@ def test_bulk_editor_get_forms_cleaned_data_errors( with override_current_transaction(user_empty_workbasket.current_transaction): with pytest.raises(ValidationError): mock_bulk_editor.get_forms_cleaned_data() - diff --git a/measures/tests/test_forms.py b/measures/tests/test_forms.py index 15bc69d44..294fe9ea5 100644 --- a/measures/tests/test_forms.py +++ b/measures/tests/test_forms.py @@ -2023,10 +2023,7 @@ def test_measure_edit_forms_geo_area_exclusions_serialize_deserialize(): geo_area1 = factories.GeographicalAreaFactory.create() geo_area2 = factories.GeographicalAreaFactory.create() - form_data = { - "form-0-excluded_area": geo_area1, - "form-1-excluded_area": geo_area2 - } + form_data = {"form-0-excluded_area": geo_area1, "form-1-excluded_area": geo_area2} with override_current_transaction(Transaction.objects.last()): form = forms.MeasureGeographicalAreaExclusionsFormSet( form_data, @@ -2040,4 +2037,4 @@ def test_measure_edit_forms_geo_area_exclusions_serialize_deserialize(): ) assert deserialized_form.is_valid() assert type(deserialized_form) == forms.MeasureGeographicalAreaExclusionsFormSet - assert deserialized_form.data == form.data \ No newline at end of file + assert deserialized_form.data == form.data diff --git a/measures/tests/test_views.py b/measures/tests/test_views.py index 5efa390d5..f6faf38b6 100644 --- a/measures/tests/test_views.py +++ b/measures/tests/test_views.py @@ -1833,6 +1833,7 @@ def test_measuretype_api_list_view(valid_user_client): equals=True, ) + @override_settings(MEASURES_ASYNC_EDIT=False) def test_multiple_measure_start_and_end_date_edit_functionality( valid_user_client, @@ -1927,6 +1928,7 @@ def test_multiple_measure_start_and_end_date_edit_functionality( assert measure.valid_between.upper == datetime.date(2100, 1, 1) assert measure.effective_end_date == datetime.date(2100, 1, 1) + @pytest.mark.parametrize( "step, form_data, updated_attribute, expected_data", [ @@ -2038,6 +2040,7 @@ def test_multiple_measure_edit_single_form_functionality( assert measure.update_type == UpdateType.UPDATE assert reduce(getattr, updated_attribute.split("."), measure) == expected_data + @override_settings(MEASURES_ASYNC_EDIT=False) def test_multiple_measure_edit_only_regulation( valid_user_client, @@ -2263,6 +2266,7 @@ def test_measure_list_selected_measures_list(valid_user_client): assert not measure_ids_in_table.difference(selected_measures_ids) + @override_settings(MEASURES_ASYNC_EDIT=False) def test_multiple_measure_edit_only_quota_order_number( valid_user_client, @@ -2334,6 +2338,7 @@ def test_multiple_measure_edit_only_quota_order_number( assert measure.update_type == UpdateType.UPDATE assert measure.order_number == quota_order_number + @override_settings(MEASURES_ASYNC_EDIT=False) def test_multiple_measure_edit_only_duties( valid_user_client, @@ -2405,6 +2410,7 @@ def test_multiple_measure_edit_only_duties( assert measure.update_type == UpdateType.UPDATE assert measure.duty_sentence == duties + @override_settings(MEASURES_ASYNC_EDIT=False) def test_multiple_measure_edit_preserves_footnote_associations( valid_user_client, @@ -2481,6 +2487,7 @@ def test_multiple_measure_edit_preserves_footnote_associations( for footnote in measure.footnotes.all(): assert footnote in expected_footnotes + @override_settings(MEASURES_ASYNC_EDIT=False) def test_multiple_measure_edit_geographical_area_exclusions( valid_user_client, diff --git a/measures/util.py b/measures/util.py index 2ca1cdf09..80849bde4 100644 --- a/measures/util.py +++ b/measures/util.py @@ -14,6 +14,7 @@ from workbaskets import models as workbasket_models import logging + logger = logging.getLogger(__name__) @@ -56,10 +57,11 @@ def diff_components( """ from measures.parsers import DutySentenceParser - # We add in the component output type here as otherwise we run into circular import issues. - component_output = measure_models.MeasureComponent if not component_output else component_output + # We add in the component output type here as otherwise we run into circular import issues. + component_output = ( + measure_models.MeasureComponent if not component_output else component_output + ) - parser = DutySentenceParser.create( start_date, component_output=component_output, diff --git a/measures/views/mixins.py b/measures/views/mixins.py index 9d283d061..feb0d21ac 100644 --- a/measures/views/mixins.py +++ b/measures/views/mixins.py @@ -53,9 +53,10 @@ def get_queryset(self): return models.Measure.objects.filter(pk__in=self.measure_selections) -class MeasureSerializableWizardMixin(): - """ A Mixin for the wizard forms that utilise asynchronous bulk processing. This mixin provides the functionality to go through each form +class MeasureSerializableWizardMixin: + """A Mixin for the wizard forms that utilise asynchronous bulk processing. This mixin provides the functionality to go through each form and serialize the data ready for storing in the database.""" + def get_data_form_list(self) -> dict: """ Returns a form list based on form_list, conditionally including only @@ -119,4 +120,4 @@ def serializable_form_kwargs_for_step(self, step) -> Dict: form_kwargs = self.get_form_kwargs(step) form_class = self.form_list[step] - return form_class.serializable_init_kwargs(form_kwargs) \ No newline at end of file + return form_class.serializable_init_kwargs(form_kwargs) diff --git a/measures/views/wizard.py b/measures/views/wizard.py index 99ca00279..b69b6d1b9 100644 --- a/measures/views/wizard.py +++ b/measures/views/wizard.py @@ -105,7 +105,7 @@ def get_template_names(self): self.steps.current, "measures/edit-wizard-step.jinja", ) - + @property def workbasket(self) -> WorkBasket: return WorkBasket.current(self.request) @@ -172,13 +172,15 @@ def async_done(self, form_list, **kwargs): kwargs={"pk": self.workbasket.pk}, ), ) - + def edit_measures(self, selected_measures, cleaned_data): """Synchronously edit measures within the context of the view / web worker using accumulated data, `cleaned_data`, from all the necessary wizard forms.""" - measures_editor = MeasuresEditor(self.workbasket, selected_measures, cleaned_data) + measures_editor = MeasuresEditor( + self.workbasket, selected_measures, cleaned_data + ) return measures_editor.edit_measures() def sync_done(self, form_list, **kwargs): @@ -208,10 +210,7 @@ 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. From 5725a65618fedf8664b261a76333f3cd09a2d220 Mon Sep 17 00:00:00 2001 From: Lauren Qurashi Date: Mon, 23 Sep 2024 12:09:49 +0100 Subject: [PATCH 36/38] Run linter --- settings/common.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/settings/common.py b/settings/common.py index 83c64ce06..a4c235f55 100644 --- a/settings/common.py +++ b/settings/common.py @@ -926,4 +926,4 @@ # Asynchronous / background (bulk) object creation and editing config. MEASURES_ASYNC_CREATION = is_truthy(os.environ.get("MEASURES_ASYNC_CREATION", "true")) -MEASURES_ASYNC_EDIT = is_truthy(os.environ.get("MEASURES_ASYNC_EDIT", "true")) \ No newline at end of file +MEASURES_ASYNC_EDIT = is_truthy(os.environ.get("MEASURES_ASYNC_EDIT", "true")) From 9d00d1e134702a07414df981523e4c6be34ed100 Mon Sep 17 00:00:00 2001 From: Lauren Qurashi Date: Wed, 25 Sep 2024 17:00:44 +0100 Subject: [PATCH 37/38] Add test to check errors are logged in a detailed manor --- measures/tests/test_bulk_processing.py | 53 ++++++++++++++++++++++++++ 1 file changed, 53 insertions(+) diff --git a/measures/tests/test_bulk_processing.py b/measures/tests/test_bulk_processing.py index e58f05ec0..f20239e61 100644 --- a/measures/tests/test_bulk_processing.py +++ b/measures/tests/test_bulk_processing.py @@ -9,6 +9,7 @@ from common.tests import factories from common.util import TaricDateRange from common.validators import ApplicabilityCode +from measures import forms from measures.models import MeasuresBulkCreator from measures.models import MeasuresBulkEditor from measures.models import ProcessingState @@ -371,3 +372,55 @@ def test_bulk_editor_get_forms_cleaned_data_errors( with override_current_transaction(user_empty_workbasket.current_transaction): with pytest.raises(ValidationError): mock_bulk_editor.get_forms_cleaned_data() + + + +@pytest.mark.parametrize( + "form_class, form_data, expected_error", + [ + ( + forms.MeasureStartDateForm, + {"start_date": { + "start_date_0": "", + "start_date_1": "", + "start_date_2": "", + }}, + "Enter the day, month and year", + ), + ( + forms.MeasureRegulationForm, + {"regulation": {"generating_regulation": ""}}, + "This field is required", + ), + ], + ids=[ + "measure_edit_start_date_form", + "measure_edit_regulation_form", + ], +) +def test_bulk_editor_log_form_errors_displays_detailed_error( + form_class, + form_data, + expected_error, + caplog, + +): + + # Create measures for the form kwargs + measure_1 = factories.MeasureFactory.create() + measure_2 = factories.MeasureFactory.create() + measure_3 = factories.MeasureFactory.create() + + form_kwargs = {"selected_measures": [measure_1, measure_2, measure_3]} + form = form_class(data=form_data, **form_kwargs) + + mock_bulk_editor = MeasuresBulkEditorFactory.create() + + import logging + # Ensure logging propagation is enabled else log messages won't + # reach this module. + logger = logging.getLogger("measures") + logger.propagate = True + + mock_bulk_editor._log_form_errors(form_class=form_class, form_or_formset=form) + assert expected_error in caplog.text \ No newline at end of file From 038a41cf8823caf984b421ef7a80a498cab22e7b Mon Sep 17 00:00:00 2001 From: Lauren Qurashi Date: Thu, 26 Sep 2024 10:11:27 +0100 Subject: [PATCH 38/38] linter --- measures/tests/test_bulk_processing.py | 21 +++++++++++---------- 1 file changed, 11 insertions(+), 10 deletions(-) diff --git a/measures/tests/test_bulk_processing.py b/measures/tests/test_bulk_processing.py index f20239e61..0b29330df 100644 --- a/measures/tests/test_bulk_processing.py +++ b/measures/tests/test_bulk_processing.py @@ -374,17 +374,18 @@ def test_bulk_editor_get_forms_cleaned_data_errors( mock_bulk_editor.get_forms_cleaned_data() - @pytest.mark.parametrize( "form_class, form_data, expected_error", [ ( forms.MeasureStartDateForm, - {"start_date": { - "start_date_0": "", - "start_date_1": "", - "start_date_2": "", - }}, + { + "start_date": { + "start_date_0": "", + "start_date_1": "", + "start_date_2": "", + } + }, "Enter the day, month and year", ), ( @@ -403,9 +404,8 @@ def test_bulk_editor_log_form_errors_displays_detailed_error( form_data, expected_error, caplog, - ): - + # Create measures for the form kwargs measure_1 = factories.MeasureFactory.create() measure_2 = factories.MeasureFactory.create() @@ -417,10 +417,11 @@ def test_bulk_editor_log_form_errors_displays_detailed_error( mock_bulk_editor = MeasuresBulkEditorFactory.create() import logging + # Ensure logging propagation is enabled else log messages won't # reach this module. logger = logging.getLogger("measures") logger.propagate = True - mock_bulk_editor._log_form_errors(form_class=form_class, form_or_formset=form) - assert expected_error in caplog.text \ No newline at end of file + mock_bulk_editor._log_form_errors(form_class=form_class, form_or_formset=form) + assert expected_error in caplog.text