From c56a89cada631f5206a78bb2684799aa0630459b Mon Sep 17 00:00:00 2001 From: nboyse Date: Mon, 13 Nov 2023 13:46:43 +0000 Subject: [PATCH 1/9] fix: use current() instead of approved_up_to_transaction() in codebase --- additional_codes/forms.py | 10 ++++----- additional_codes/views.py | 9 +++----- certificates/forms.py | 8 +++---- certificates/views.py | 9 +++----- commodities/business_rules.py | 31 +++++++++------------------ commodities/forms.py | 2 +- commodities/models/dc.py | 8 +++---- commodities/models/orm.py | 6 +++--- commodities/tests/test_views.py | 7 ++---- commodities/views.py | 9 ++------ common/business_rules.py | 10 ++++----- common/models/tracked_qs.py | 3 ++- common/models/trackedmodel.py | 12 +++++------ common/querysets.py | 2 +- common/tests/test_models.py | 2 +- conftest.py | 4 +--- docs/source/architecture/index.rst | 2 +- footnotes/forms.py | 2 +- footnotes/views.py | 6 +++--- geo_areas/business_rules.py | 4 ++-- geo_areas/forms.py | 2 +- geo_areas/models.py | 4 ++-- geo_areas/views.py | 4 ++-- measures/business_rules.py | 24 +++++++++------------ measures/forms.py | 11 +++------- measures/models.py | 4 ++-- measures/patterns.py | 2 +- measures/querysets.py | 4 +--- measures/tests/factories.py | 8 ++----- measures/tests/test_util.py | 16 ++++---------- measures/tests/test_views.py | 34 +++++++++++++----------------- measures/util.py | 4 +--- measures/views.py | 23 ++++++-------------- quotas/business_rules.py | 28 +++++++----------------- quotas/models.py | 4 ++-- quotas/tests/test_views.py | 28 ++++++++---------------- quotas/views.py | 14 +++++------- regulations/business_rules.py | 2 +- regulations/forms.py | 3 +-- regulations/models.py | 2 +- regulations/views.py | 6 +++--- workbaskets/views/mixins.py | 6 +----- 42 files changed, 138 insertions(+), 241 deletions(-) diff --git a/additional_codes/forms.py b/additional_codes/forms.py index e718c6987..879716c7e 100644 --- a/additional_codes/forms.py +++ b/additional_codes/forms.py @@ -155,13 +155,11 @@ def clean(self): def save(self, commit=True): instance = super().save(commit=False) - tx = WorkBasket.get_current_transaction(self.request) - highest_sid = ( - models.AdditionalCode.objects.approved_up_to_transaction(tx).aggregate( - Max("sid"), - )["sid__max"] - ) or 0 + models.AdditionalCode.objects.current().aggregate( + Max("sid"), + )["sid__max"] + ) or 0 instance.sid = highest_sid + 1 if commit: diff --git a/additional_codes/views.py b/additional_codes/views.py index 0b2cabd71..c9460cae1 100644 --- a/additional_codes/views.py +++ b/additional_codes/views.py @@ -45,9 +45,8 @@ class AdditionalCodeViewSet(viewsets.ReadOnlyModelViewSet): filter_backends = [AdditionalCodeFilterBackend] def get_queryset(self): - tx = WorkBasket.get_current_transaction(self.request) return ( - AdditionalCode.objects.approved_up_to_transaction(tx) + AdditionalCode.objects.current() .select_related("type") .prefetch_related("descriptions") ) @@ -65,8 +64,7 @@ class AdditionalCodeMixin: model: Type[TrackedModel] = AdditionalCode def get_queryset(self): - tx = WorkBasket.get_current_transaction(self.request) - return AdditionalCode.objects.approved_up_to_transaction(tx).select_related( + return AdditionalCode.objects.current().select_related( "type", ) @@ -208,8 +206,7 @@ class AdditionalCodeDescriptionMixin: model: Type[TrackedModel] = AdditionalCodeDescription def get_queryset(self): - tx = WorkBasket.get_current_transaction(self.request) - return AdditionalCodeDescription.objects.approved_up_to_transaction(tx) + return AdditionalCodeDescription.objects.current() class AdditionalCodeDescriptionCreate( diff --git a/certificates/forms.py b/certificates/forms.py index eca9e8cdc..ccc9b93ec 100644 --- a/certificates/forms.py +++ b/certificates/forms.py @@ -50,8 +50,7 @@ def __init__(self, *args, **kwargs): def filter_certificates_for_sid(self, sid): certificate_type = self.cleaned_data["certificate_type"] - tx = WorkBasket.get_current_transaction(self.request) - return models.Certificate.objects.approved_up_to_transaction(tx).filter( + return models.Certificate.objects.current().filter( sid=sid, certificate_type=certificate_type, ) @@ -64,14 +63,13 @@ def next_sid(self, instance): form's save() method (with its commit param set either to True or False). """ - current_transaction = WorkBasket.get_current_transaction(self.request) # Filter certificate by type and find the highest sid, using regex to # ignore legacy, non-numeric identifiers return get_next_id( - models.Certificate.objects.filter( + models.Certificate.objects.current().filter( sid__regex=r"^[0-9]*$", certificate_type__sid=instance.certificate_type.sid, - ).approved_up_to_transaction(current_transaction), + ), instance._meta.get_field("sid"), max_len=3, ) diff --git a/certificates/views.py b/certificates/views.py index 6535a791c..7b8681367 100644 --- a/certificates/views.py +++ b/certificates/views.py @@ -40,9 +40,8 @@ class CertificatesViewSet(viewsets.ReadOnlyModelViewSet): permission_classes = [permissions.IsAuthenticated] def get_queryset(self): - tx = WorkBasket.get_current_transaction(self.request) return ( - models.Certificate.objects.approved_up_to_transaction(tx) + models.Certificate.objects.current() .select_related("certificate_type") .prefetch_related("descriptions") ) @@ -60,8 +59,7 @@ class CertificateMixin: model: Type[TrackedModel] = models.Certificate def get_queryset(self): - tx = WorkBasket.get_current_transaction(self.request) - return models.Certificate.objects.approved_up_to_transaction(tx).select_related( + return models.Certificate.objects.current().select_related( "certificate_type", ) @@ -237,8 +235,7 @@ class CertificateDescriptionMixin: model: Type[TrackedModel] = models.CertificateDescription def get_queryset(self): - tx = WorkBasket.get_current_transaction(self.request) - return models.CertificateDescription.objects.approved_up_to_transaction(tx) + return models.CertificateDescription.objects.current() class CertificateCreateDescriptionMixin: diff --git a/commodities/business_rules.py b/commodities/business_rules.py index de91d8951..f5b08a00a 100644 --- a/commodities/business_rules.py +++ b/commodities/business_rules.py @@ -43,12 +43,8 @@ def __init__(self, transaction=None): self.logger = logging.getLogger(type(self).__name__) def parent_spans_child(self, parent, child) -> bool: - parent_validity = parent.indented_goods_nomenclature.version_at( - self.transaction, - ).valid_between - child_validity = child.indented_goods_nomenclature.version_at( - self.transaction, - ).valid_between + parent_validity = parent.indented_goods_nomenclature.version_at().valid_between + child_validity = child.indented_goods_nomenclature.version_at().valid_between return validity_range_contains_range(parent_validity, child_validity) def parents_span_childs_future(self, parents, child): @@ -59,17 +55,13 @@ def parents_span_childs_future(self, parents, child): parents_validity = [] for parent in parents: parents_validity.append( - parent.indented_goods_nomenclature.version_at( - self.transaction, - ).valid_between, + parent.indented_goods_nomenclature.version_at().valid_between, ) # sort by start date so any gaps will be obvious parents_validity.sort(key=lambda daterange: daterange.lower) - child_validity = child.indented_goods_nomenclature.version_at( - self.transaction, - ).valid_between + child_validity = child.indented_goods_nomenclature.version_at().valid_between if ( not child_validity.upper_inf @@ -108,7 +100,7 @@ def validate(self, indent): from commodities.models.dc import get_chapter_collection try: - good = indent.indented_goods_nomenclature.version_at(self.transaction) + good = indent.indented_goods_nomenclature.version_at() except TrackedModel.DoesNotExist: self.logger.warning( "Goods nomenclature %s no longer exists at transaction %s " @@ -166,10 +158,9 @@ def validate(self, good): if not ( good.code.is_chapter - or GoodsNomenclatureOrigin.objects.filter( + or GoodsNomenclatureOrigin.objects.current().filter( new_goods_nomenclature__sid=good.sid, ) - .approved_up_to_transaction(good.transaction) .exists() ): raise self.violation( @@ -252,9 +243,9 @@ class NIG11(ValidityStartDateRules): def get_objects(self, good): GoodsNomenclatureIndent = good.indents.model - return GoodsNomenclatureIndent.objects.filter( + return GoodsNomenclatureIndent.objects.current().filter( indented_goods_nomenclature__sid=good.sid, - ).approved_up_to_transaction(self.transaction) + ) class NIG12(DescriptionsRules): @@ -305,7 +296,7 @@ def validate(self, association): goods_nomenclature__sid=association.goods_nomenclature.sid, valid_between__overlap=association.valid_between, ) - .approved_up_to_transaction(association.transaction) + .current() .exclude( id=association.pk, ) @@ -351,9 +342,7 @@ def has_violation(self, good): goods_nomenclature__sid=good.sid, additional_code__isnull=False, ) - .approved_up_to_transaction( - self.transaction, - ) + .current() .exists() ) diff --git a/commodities/forms.py b/commodities/forms.py index 8a80857ac..c01fec547 100644 --- a/commodities/forms.py +++ b/commodities/forms.py @@ -76,7 +76,7 @@ def init_fields(self): ].help_text = "Leave empty if the footnote is needed for an unlimited time" self.fields[ "associated_footnote" - ].queryset = Footnote.objects.approved_up_to_transaction(self.tx).filter( + ].queryset = Footnote.objects.current().filter( footnote_type__application_code__in=[1, 2], ) self.fields[ diff --git a/commodities/models/dc.py b/commodities/models/dc.py index 38ba18ce2..3c9bd5ed0 100644 --- a/commodities/models/dc.py +++ b/commodities/models/dc.py @@ -502,7 +502,7 @@ def get_dependent_measures( measure_qs = Measure.objects.filter(goods_sid_query) if self.moment.clock_type.is_transaction_clock: - measure_qs = measure_qs.approved_up_to_transaction(self.moment.transaction) + measure_qs = measure_qs.current() else: measure_qs = measure_qs.latest_approved() @@ -823,7 +823,7 @@ def get_snapshot( date=snapshot_date, ) - commodities = self._get_snapshot_commodities(transaction, snapshot_date) + commodities = self._get_snapshot_commodities(snapshot_date) return CommodityTreeSnapshot( moment=moment, @@ -832,7 +832,6 @@ def get_snapshot( def _get_snapshot_commodities( self, - transaction: Transaction, snapshot_date: date, ) -> List[Commodity]: """ @@ -853,8 +852,7 @@ def _get_snapshot_commodities( that match the latest_version goods. """ item_ids = {c.item_id for c in self.commodities if c.obj} - goods = GoodsNomenclature.objects.approved_up_to_transaction( - transaction, + goods = GoodsNomenclature.objects.current( ).filter( item_id__in=item_ids, valid_between__contains=snapshot_date, diff --git a/commodities/models/orm.py b/commodities/models/orm.py index 02e2456c9..fb4683f76 100644 --- a/commodities/models/orm.py +++ b/commodities/models/orm.py @@ -111,9 +111,9 @@ def get_url(self): return reverse("commodity-ui-detail", kwargs={"sid": self.sid}) def get_dependent_measures(self, transaction=None): - return self.measures.model.objects.filter( + return self.measures.model.objects.current().filter( goods_nomenclature__sid=self.sid, - ).approved_up_to_transaction(transaction) + ) @property def is_taric_code(self) -> bool: @@ -200,7 +200,7 @@ def get_good_indents( ) -> QuerySet: """Return the related goods indents based on approval status.""" good = self.indented_goods_nomenclature - return good.indents.approved_up_to_transaction( + return good.indents.current( as_of_transaction or self.transaction, ) diff --git a/commodities/tests/test_views.py b/commodities/tests/test_views.py index dffc16191..41b2082fb 100644 --- a/commodities/tests/test_views.py +++ b/commodities/tests/test_views.py @@ -68,7 +68,7 @@ def test_commodity_list_queryset(): good_1 = factories.SimpleGoodsNomenclatureFactory.create(item_id="1010000000") good_2 = factories.SimpleGoodsNomenclatureFactory.create(item_id="1000000000") tx = Transaction.objects.last() - commodity_count = GoodsNomenclature.objects.approved_up_to_transaction(tx).count() + commodity_count = GoodsNomenclature.objects.current().count() with override_current_transaction(tx): qs = view.get_queryset() @@ -522,11 +522,8 @@ def test_commodity_footnote_update_success(valid_user_client, date_ranges): "end_date": "", } response = valid_user_client.post(url, data) - tx = Transaction.objects.last() updated_association = ( - FootnoteAssociationGoodsNomenclature.objects.approved_up_to_transaction( - tx, - ).first() + FootnoteAssociationGoodsNomenclature.objects.current().first() ) assert response.status_code == 302 assert response.url == updated_association.get_url("confirm-update") diff --git a/commodities/views.py b/commodities/views.py index cccb84fc4..2eff8aaa5 100644 --- a/commodities/views.py +++ b/commodities/views.py @@ -49,9 +49,7 @@ def get_queryset(self): """ tx = WorkBasket.get_current_transaction(self.request) return ( - GoodsNomenclature.objects.approved_up_to_transaction( - tx, - ) + GoodsNomenclature.objects.current() .prefetch_related("descriptions") .as_at_and_beyond(date.today()) .filter(suffix=80) @@ -69,10 +67,7 @@ class FootnoteAssociationMixin: model = FootnoteAssociationGoodsNomenclature def get_queryset(self): - tx = WorkBasket.get_current_transaction(self.request) - return FootnoteAssociationGoodsNomenclature.objects.approved_up_to_transaction( - tx, - ) + return self.model.objects.current() class CommodityList(CommodityMixin, WithPaginationListView): diff --git a/common/business_rules.py b/common/business_rules.py index 79b7a6283..c7d6a9377 100644 --- a/common/business_rules.py +++ b/common/business_rules.py @@ -144,7 +144,7 @@ def get_linked_models( related_instances = [getattr(model, field.name)] for instance in related_instances: try: - yield instance.version_at(transaction) + yield instance.version_at() except TrackedModel.DoesNotExist: # `related_instances` will contain all instances, even # deleted ones, and `version_at` will return @@ -278,8 +278,7 @@ def validate(self, model): if ( type(model) - .objects.filter(**query) - .approved_up_to_transaction(self.transaction) + .objects.current().filter(**query) .exclude(version_group=model.version_group) .exists() ): @@ -305,8 +304,7 @@ def validate(self, model): query["valid_between__overlap"] = model.valid_between if ( - model.__class__.objects.filter(**query) - .approved_up_to_transaction(self.transaction) + model.__class__.objects.current().filter(**query) .exclude(version_group=model.version_group) .exists() ): @@ -573,7 +571,7 @@ def validate(self, exclusion): Membership = geo_group._meta.get_field("members").related_model if ( - not Membership.objects.approved_up_to_transaction(self.transaction) + not Membership.objects.current() .filter( geo_group__sid=geo_group.sid, member__sid=exclusion.excluded_geographical_area.sid, diff --git a/common/models/tracked_qs.py b/common/models/tracked_qs.py index 10d672aff..e5082939b 100644 --- a/common/models/tracked_qs.py +++ b/common/models/tracked_qs.py @@ -69,7 +69,8 @@ def current(self) -> TrackedModelQuerySet: ) def approved_up_to_transaction(self, transaction=None) -> TrackedModelQuerySet: - """Get the approved versions of the model being queried, unless there + """This function is called using the current() function instead of directly calling it on model queries. + Get the approved versions of the model being queried, unless there exists a version of the model in a draft state within a transaction preceding (and including) the given transaction in the workbasket of the given transaction.""" diff --git a/common/models/trackedmodel.py b/common/models/trackedmodel.py index d2cc3947c..f3cddd514 100644 --- a/common/models/trackedmodel.py +++ b/common/models/trackedmodel.py @@ -342,7 +342,7 @@ def current_version(self: Cls) -> Cls: raise self.__class__.DoesNotExist("Object has no current version") return current_version - def version_at(self: Cls, transaction) -> Cls: + def version_at(self: Cls) -> Cls: """ The latest version of this model that was approved as of the given transaction. @@ -350,7 +350,7 @@ def version_at(self: Cls, transaction) -> Cls: :param transaction Transaction: Limit versions to this transaction :rtype TrackedModel: """ - return self.get_versions().approved_up_to_transaction(transaction).get() + return self.get_versions().current().get() @classproperty def copyable_fields(cls): @@ -504,12 +504,12 @@ def copy( kwargs.update(nested_fields) if not ignore: - for model in queryset.approved_up_to_transaction(transaction): + for model in queryset.current(): model.copy(transaction, **kwargs) return new_object - def in_use_by(self, via_relation: str, transaction=None) -> QuerySet[TrackedModel]: + def in_use_by(self, via_relation: str) -> QuerySet[TrackedModel]: """ Returns all of the models that are referencing this one via the specified relation and exist as of the passed transaction. @@ -526,7 +526,7 @@ def in_use_by(self, via_relation: str, transaction=None) -> QuerySet[TrackedMode return remote_model.objects.filter( **{f"{remote_field_name}__version_group": self.version_group} - ).approved_up_to_transaction(transaction) + ).current() def in_use(self, transaction=None, *relations: str) -> bool: """ @@ -572,7 +572,7 @@ def in_use(self, transaction=None, *relations: str) -> bool: # If we find any objects for any relation, then the model is in use. for relation_name in using_models: - relation_queryset = self.in_use_by(relation_name, transaction) + relation_queryset = self.in_use_by(relation_name) if relation_queryset.exists(): return True diff --git a/common/querysets.py b/common/querysets.py index 77dedac89..4939af314 100644 --- a/common/querysets.py +++ b/common/querysets.py @@ -66,7 +66,7 @@ def not_current(self, asof_transaction=None) -> QuerySet: :param transaction Transaction: The transaction to limit versions to. :rtype QuerySet: """ - current = self.approved_up_to_transaction(asof_transaction) + current = self.current() return self.difference(current) diff --git a/common/tests/test_models.py b/common/tests/test_models.py index 61d43f3ac..34745fbf3 100644 --- a/common/tests/test_models.py +++ b/common/tests/test_models.py @@ -310,7 +310,7 @@ def test_current_as_of(sample_model): assert models.TestModel1.objects.latest_approved().get().pk == sample_model.pk assert ( - models.TestModel1.objects.approved_up_to_transaction(transaction).get().pk + models.TestModel1.objects.current().get().pk == unapproved_version.pk ) diff --git a/conftest.py b/conftest.py index 8beb83b14..315c588eb 100644 --- a/conftest.py +++ b/conftest.py @@ -835,9 +835,7 @@ def check( assert workbasket is not None try: - imported = model_class.objects.approved_up_to_transaction( - workbasket.current_transaction, - ).get(**db_kwargs) + imported = model_class.objects.current().get(**db_kwargs) except model_class.DoesNotExist: if model.update_type == UpdateType.DELETE: imported = ( diff --git a/docs/source/architecture/index.rst b/docs/source/architecture/index.rst index 6dcdd0b69..bdabea63d 100644 --- a/docs/source/architecture/index.rst +++ b/docs/source/architecture/index.rst @@ -106,7 +106,7 @@ that is not draft is considered to be "current". There are a number of convenience methods for finding "current" models. .. autoclass:: common.models.trackedmodel.TrackedModelQuerySet - :members: latest_approved, approved_up_to_transaction + :members: latest_approved, current Workbaskets diff --git a/footnotes/forms.py b/footnotes/forms.py index 5d34b7dd6..c67b79b41 100644 --- a/footnotes/forms.py +++ b/footnotes/forms.py @@ -128,7 +128,7 @@ def next_sid(self, instance): return get_next_id( models.Footnote.objects.filter( footnote_type__footnote_type_id=instance.footnote_type.footnote_type_id, - ).approved_up_to_transaction(tx), + ).current(), instance._meta.get_field("footnote_id"), max_len=3, ) diff --git a/footnotes/views.py b/footnotes/views.py index 10b9345c8..5aa940b31 100644 --- a/footnotes/views.py +++ b/footnotes/views.py @@ -46,7 +46,7 @@ class FootnoteViewSet(viewsets.ReadOnlyModelViewSet): def get_queryset(self): tx = WorkBasket.get_current_transaction(self.request) return ( - models.Footnote.objects.approved_up_to_transaction(tx) + models.Footnote.objects.current() .select_related("footnote_type") .prefetch_related("descriptions") ) @@ -68,7 +68,7 @@ class FootnoteMixin: def get_queryset(self): tx = WorkBasket.get_current_transaction(self.request) - return models.Footnote.objects.approved_up_to_transaction(tx).select_related( + return models.Footnote.objects.current().select_related( "footnote_type", ) @@ -78,7 +78,7 @@ class FootnoteDescriptionMixin: def get_queryset(self): tx = WorkBasket.get_current_transaction(self.request) - return models.FootnoteDescription.objects.approved_up_to_transaction(tx) + return models.FootnoteDescription.objects.current() class FootnoteCreateDescriptionMixin: diff --git a/geo_areas/business_rules.py b/geo_areas/business_rules.py index 43ba73545..59daf160f 100644 --- a/geo_areas/business_rules.py +++ b/geo_areas/business_rules.py @@ -172,7 +172,7 @@ def validate(self, membership): member__version_group=membership.member.version_group, valid_between__overlap=membership.valid_between, ) - .approved_up_to_transaction(membership.transaction) + .current() .exclude( id=membership.id, ) @@ -209,7 +209,7 @@ def validate(self, membership): parent.members.filter( member__sid=membership.member.sid, ) - .approved_up_to_transaction(membership.transaction) + .current() .exclude( valid_between__contains=membership.valid_between, ) diff --git a/geo_areas/forms.py b/geo_areas/forms.py index f59241703..9a638adba 100644 --- a/geo_areas/forms.py +++ b/geo_areas/forms.py @@ -329,7 +329,7 @@ def clean(self): if membership and action == GeoMembershipAction.DELETE: tx = WorkBasket.get_current_transaction(self.request) - if membership.member_used_in_measure_exclusion(transaction=tx): + if membership.member_used_in_measure_exclusion(): self.add_error( "membership", f"{membership.member.structure_description} is referenced as an excluded geographical area in a measure and cannot be deleted as a member of the area group.", diff --git a/geo_areas/models.py b/geo_areas/models.py index 66f1c5e52..edc8b6075 100644 --- a/geo_areas/models.py +++ b/geo_areas/models.py @@ -224,8 +224,8 @@ def other(self, area: GeographicalArea) -> GeographicalArea: else: raise ValueError(f"{area} is not part of membership {self}") - def member_used_in_measure_exclusion(self, transaction): - return self.member.in_use(transaction, "measureexcludedgeographicalarea") + def member_used_in_measure_exclusion(self): + return self.member.in_use("measureexcludedgeographicalarea") class GeographicalAreaDescription(DescriptionMixin, TrackedModel): diff --git a/geo_areas/views.py b/geo_areas/views.py index f29fb6baa..b80380c3f 100644 --- a/geo_areas/views.py +++ b/geo_areas/views.py @@ -57,7 +57,7 @@ class GeoAreaMixin: def get_queryset(self): tx = WorkBasket.get_current_transaction(self.request) - return GeographicalArea.objects.approved_up_to_transaction(tx) + return GeographicalArea.objects.current() class GeoAreaDescriptionMixin: @@ -65,7 +65,7 @@ class GeoAreaDescriptionMixin: def get_queryset(self): tx = WorkBasket.get_current_transaction(self.request) - return GeographicalAreaDescription.objects.approved_up_to_transaction(tx) + return GeographicalAreaDescription.objects.current() class GeoAreaCreateDescriptionMixin: diff --git a/measures/business_rules.py b/measures/business_rules.py index 1c317f338..3c2222b69 100644 --- a/measures/business_rules.py +++ b/measures/business_rules.py @@ -212,11 +212,10 @@ def validate(self, measure): goods = ( type(measure.goods_nomenclature) - .objects.filter( + .objects.current().filter( sid=measure.goods_nomenclature.sid, valid_between__overlap=measure.effective_valid_between, ) - .approved_up_to_transaction(measure.transaction) ) explosion_level = measure.measure_type.measure_explosion_level @@ -614,8 +613,7 @@ def validate(self, measure): ) if ( measure.additional_code - and not AdditionalCodeTypeMeasureType.objects.approved_up_to_transaction( - self.transaction, + and not AdditionalCodeTypeMeasureType.objects.current( ) .filter( additional_code_type__sid=measure.additional_code.type.sid, @@ -841,7 +839,7 @@ class ME43(BusinessRule): def validate(self, measure_component): duty_expressions_used = ( type(measure_component) - .objects.approved_up_to_transaction(measure_component.transaction) + .objects.current() .exclude(pk=measure_component.pk if measure_component.pk else None) .excluding_versions_of(version_group=measure_component.version_group) .filter( @@ -891,7 +889,7 @@ def validate(self, measure): ) if ( components.filter(inapplicable) - .approved_up_to_transaction(self.transaction) + .current() .exists() ): raise self.violation(measure, self.messages[code].format(self)) @@ -1028,7 +1026,7 @@ def validate(self, measure_condition): version_group=measure_condition.version_group, ) .filter(**kwargs) - .approved_up_to_transaction(self.transaction) + .current() .exists() ): raise self.violation(measure_condition) @@ -1103,7 +1101,7 @@ class ME108(BusinessRule): def validate(self, component): if ( type(component) - .objects.approved_up_to_transaction(component.transaction) + .objects.current() .exclude(pk=component.pk or None) .excluding_versions_of(version_group=component.version_group) .filter( @@ -1145,7 +1143,7 @@ class ActionRequiresDuty(BusinessRule): condition component must be created with a duty amount.""" def validate(self, condition): - components = condition.components.approved_up_to_transaction(self.transaction) + components = condition.components.current() components_have_duty = any([c.duty_amount is not None for c in components]) if condition.action.requires_duty and not components_have_duty: raise self.violation( @@ -1245,7 +1243,7 @@ def validate(self, exclusion): from measures.models import Measure # Need to get latest version of measure - measures = Measure.objects.approved_up_to_transaction(self.transaction).filter( + measures = Measure.objects.current().filter( sid=exclusion.modified_measure.sid, ) @@ -1255,9 +1253,7 @@ def validate(self, exclusion): if measures.exists(): measure = measures.last() geo_area = measure.geographical_area - members = geo_area.members.approved_up_to_transaction( - self.transaction, - ) + members = geo_area.members.current() matching_members_to_exclusion_period = members.filter( Q( @@ -1324,7 +1320,7 @@ class ME70(BusinessRule): def validate(self, association): if ( type(association) - .objects.approved_up_to_transaction(association.transaction) + .objects.current() .exclude(pk=association.pk or None) .excluding_versions_of(version_group=association.version_group) .filter( diff --git a/measures/forms.py b/measures/forms.py index 6e4ddf1d5..76ae8dda8 100644 --- a/measures/forms.py +++ b/measures/forms.py @@ -541,9 +541,7 @@ def __init__(self, *args, **kwargs): if f"instance_footnotes_{self.instance.sid}" not in self.request.session.keys(): tx = WorkBasket.get_current_transaction(self.request) associations = ( - models.FootnoteAssociationMeasure.objects.approved_up_to_transaction( - tx, - ).filter( + models.FootnoteAssociationMeasure.objects.current().filter( footnoted_measure__sid=self.instance.sid, ) ) @@ -717,15 +715,12 @@ def save(self, commit=True): for pk in footnote_pks: footnote = ( - Footnote.objects.filter(pk=pk) - .approved_up_to_transaction(instance.transaction) + Footnote.objects.current().filter(pk=pk) .first() ) existing_association = ( - models.FootnoteAssociationMeasure.objects.approved_up_to_transaction( - instance.transaction, - ) + models.FootnoteAssociationMeasure.objects.current() .filter( footnoted_measure__sid=instance.sid, associated_footnote__footnote_id=footnote.footnote_id, diff --git a/measures/models.py b/measures/models.py index de9eaa92e..bd75fdad1 100644 --- a/measures/models.py +++ b/measures/models.py @@ -656,14 +656,14 @@ def auto_value_fields(cls): def has_components(self, transaction): return ( - MeasureComponent.objects.approved_up_to_transaction(transaction) + MeasureComponent.objects.current() .filter(component_measure__sid=self.sid) .exists() ) def has_condition_components(self, transaction): return ( - MeasureConditionComponent.objects.approved_up_to_transaction(transaction) + MeasureConditionComponent.objects.current() .filter(condition__dependent_measure__sid=self.sid) .exists() ) diff --git a/measures/patterns.py b/measures/patterns.py index 332692a91..ef025d49b 100644 --- a/measures/patterns.py +++ b/measures/patterns.py @@ -517,7 +517,7 @@ def get_measures(self, code: GoodsNomenclature, as_at: date): date.""" return ( Measure.objects.with_validity_field() - .approved_up_to_transaction(self.workbasket.transactions.last()) + .current() .as_at(as_at) .filter(goods_nomenclature__sid=code.sid) ) diff --git a/measures/querysets.py b/measures/querysets.py index 62c23fc34..75a8ca3c6 100644 --- a/measures/querysets.py +++ b/measures/querysets.py @@ -94,9 +94,7 @@ def duty_sentence( # Components with the greatest transaction_id that is less than # or equal to component_parent's transaction_id, are considered 'current'. - component_qs = component_parent.components.approved_up_to_transaction( - component_parent.transaction, - ) + component_qs = component_parent.components.current() if not component_qs: return "" latest_transaction_id = component_qs.aggregate( diff --git a/measures/tests/factories.py b/measures/tests/factories.py index 21a9db843..850f4d449 100644 --- a/measures/tests/factories.py +++ b/measures/tests/factories.py @@ -31,17 +31,13 @@ class Meta: measure_type_description = factory.SelfAttribute("measure.measure_type.description") duty_sentence = factory.sequence(lambda n: f"{n}.00%") origin_description = factory.LazyAttribute( - lambda m: m.measure.geographical_area.descriptions.approved_up_to_transaction( - transaction=m.measure.geographical_area.transaction, - ) + lambda m: m.measure.geographical_area.descriptions.current() .last() .description, ) excluded_origin_descriptions = factory.LazyAttribute( lambda m: random.choice(MeasureSheetRow.separators).join( - e.excluded_geographical_area.descriptions.approved_up_to_transaction( - transaction=e.excluded_geographical_area.transaction, - ) + e.excluded_geographical_area.descriptions.current() .last() .description for e in m.measure.exclusions.all() diff --git a/measures/tests/test_util.py b/measures/tests/test_util.py index f1eaef53c..2190cd146 100644 --- a/measures/tests/test_util.py +++ b/measures/tests/test_util.py @@ -40,9 +40,7 @@ def test_diff_components_update( MeasureComponent, "component_measure", ) - components = new_measure.components.approved_up_to_transaction( - workbasket.current_transaction, - ) + components = new_measure.components.current() assert components.count() == 1 @@ -86,9 +84,7 @@ def test_diff_components_update_multiple( MeasureComponent, "component_measure", ) - components = component_1.component_measure.components.approved_up_to_transaction( - workbasket.current_transaction, - ) + components = component_1.component_measure.components.current() assert components.count() == 2 @@ -116,9 +112,7 @@ def test_diff_components_create(workbasket, duty_sentence_parser): MeasureComponent, "component_measure", ) - components = measure.components.approved_up_to_transaction( - workbasket.current_transaction, - ) + components = measure.components.current() assert components.count() == 1 @@ -151,9 +145,7 @@ def test_diff_components_delete( MeasureComponent, "component_measure", ) - components = component.component_measure.components.approved_up_to_transaction( - workbasket.current_transaction, - ) + components = component.component_measure.components.current() assert components.count() == 0 diff --git a/measures/tests/test_views.py b/measures/tests/test_views.py index 4bea4a79d..e7548fda4 100644 --- a/measures/tests/test_views.py +++ b/measures/tests/test_views.py @@ -540,10 +540,10 @@ def test_measure_update_duty_sentence( if update_data: tx = Transaction.objects.last() - measure = Measure.objects.approved_up_to_transaction(tx).get( + measure = Measure.objects.current().get( sid=measure_form.instance.sid, ) - components = measure.components.approved_up_to_transaction(tx).filter( + components = measure.components.current().filter( component_measure__sid=measure_form.instance.sid, ) @@ -701,13 +701,13 @@ def test_measure_update_create_conditions( assert response.url == reverse("measure-ui-confirm-update", args=(measure.sid,)) tx = Transaction.objects.last() - updated_measure = Measure.objects.approved_up_to_transaction(tx).get( + updated_measure = Measure.objects.current().get( sid=measure.sid, ) - assert updated_measure.conditions.approved_up_to_transaction(tx).count() == 1 + assert updated_measure.conditions.current().count() == 1 - condition = updated_measure.conditions.approved_up_to_transaction(tx).first() + condition = updated_measure.conditions.current().first() assert ( condition.condition_code.pk @@ -727,7 +727,7 @@ def test_measure_update_create_conditions( ) assert condition.update_type == UpdateType.CREATE - components = condition.components.approved_up_to_transaction(tx).order_by( + components = condition.components.current().order_by( *MeasureConditionComponent._meta.ordering ) @@ -757,7 +757,7 @@ def test_measure_update_edit_conditions( client.post(url, data=measure_edit_conditions_data) transaction_count = Transaction.objects.count() tx = Transaction.objects.last() - measure_with_condition = Measure.objects.approved_up_to_transaction(tx).get( + measure_with_condition = Measure.objects.current().get( sid=measure.sid, ) previous_condition = measure_with_condition.conditions.last() @@ -772,15 +772,15 @@ def test_measure_update_edit_conditions( ] = "10 GBP / 100 kg" client.post(url, data=measure_edit_conditions_data) tx = Transaction.objects.last() - updated_measure = Measure.objects.approved_up_to_transaction(tx).get( + updated_measure = Measure.objects.current().get( sid=measure.sid, ) # We expect one transaction for updating the measure and updating the condition, one for deleting a component and updating a component assert Transaction.objects.count() == transaction_count + 2 - assert updated_measure.conditions.approved_up_to_transaction(tx).count() == 1 + assert updated_measure.conditions.current().count() == 1 - condition = updated_measure.conditions.approved_up_to_transaction(tx).first() + condition = updated_measure.conditions.current().first() assert condition.pk != previous_condition.pk assert condition.required_certificate == None @@ -788,7 +788,7 @@ def test_measure_update_edit_conditions( assert condition.update_type == UpdateType.UPDATE assert condition.sid == previous_condition.sid - components = condition.components.approved_up_to_transaction(tx).all() + components = condition.components.current().all() assert components.count() == 1 @@ -878,11 +878,11 @@ def test_measure_update_remove_conditions( assert Transaction.objects.count() == transaction_count + 1 tx = Transaction.objects.last() - updated_measure = Measure.objects.approved_up_to_transaction(tx).get( + updated_measure = Measure.objects.current().get( sid=measure.sid, ) - assert updated_measure.conditions.approved_up_to_transaction(tx).count() == 0 + assert updated_measure.conditions.current().count() == 0 def test_measure_update_negative_condition( @@ -1059,15 +1059,11 @@ def test_measure_update_group_exclusion(client, valid_user, erga_omnes): }, ) - assert not MeasureExcludedGeographicalArea.objects.approved_up_to_transaction( - Transaction.objects.last(), - ).exists() + assert not MeasureExcludedGeographicalArea.objects.current().exists() client.post(url, data=data) measure_area_exclusions = ( - MeasureExcludedGeographicalArea.objects.approved_up_to_transaction( - Transaction.objects.last(), - ) + MeasureExcludedGeographicalArea.objects.current() ) assert measure_area_exclusions.count() == 2 diff --git a/measures/util.py b/measures/util.py index bf00efd6a..d2aebb36f 100644 --- a/measures/util.py +++ b/measures/util.py @@ -55,9 +55,7 @@ def diff_components( ) new_components = parser.parse(duty_sentence) - old_components = instance.components.approved_up_to_transaction( - workbasket.current_transaction, - ) + old_components = instance.components.current() new_by_id = {c.duty_expression.id: c for c in new_components} old_by_id = {c.duty_expression.id: c for c in old_components} all_ids = set(new_by_id.keys()) | set(old_by_id.keys()) diff --git a/measures/views.py b/measures/views.py index 178a4852a..b8775cf51 100644 --- a/measures/views.py +++ b/measures/views.py @@ -76,7 +76,7 @@ class MeasureTypeViewSet(viewsets.ReadOnlyModelViewSet): def get_queryset(self): tx = WorkBasket.get_current_transaction(self.request) - return MeasureType.objects.approved_up_to_transaction(tx).order_by( + return MeasureType.objects.current().order_by( "description", ) @@ -87,7 +87,7 @@ class MeasureMixin: def get_queryset(self): tx = WorkBasket.get_current_transaction(self.request) - return Measure.objects.approved_up_to_transaction(tx) + return Measure.objects.current() class MeasureSessionStoreMixin: @@ -1069,7 +1069,7 @@ def get_form(self, step=None, data=None, files=None): if hasattr(f, "fields"): for field in f.fields.values(): if hasattr(field, "queryset"): - field.queryset = field.queryset.approved_up_to_transaction(tx) + field.queryset = field.queryset.current() form.is_valid() if hasattr(form, "cleaned_data"): @@ -1108,26 +1108,20 @@ def get_form(self, form_class=None): if hasattr(form, "field"): for field in form.fields.values(): if hasattr(field, "queryset"): - field.queryset = field.queryset.approved_up_to_transaction(tx) + field.queryset = field.queryset.current() return form def get_footnotes(self, measure): - tx = WorkBasket.get_current_transaction(self.request) - associations = FootnoteAssociationMeasure.objects.approved_up_to_transaction( - tx, - ).filter( + associations = FootnoteAssociationMeasure.objects.current().filter( footnoted_measure__sid=measure.sid, ) return [a.associated_footnote for a in associations] def get_conditions(self, measure): - tx = WorkBasket.get_current_transaction(self.request) return ( - measure.conditions.with_reference_price_string().approved_up_to_transaction( - tx, - ) + measure.conditions.with_reference_price_string().current() ) def get_context_data(self, **kwargs): @@ -1194,10 +1188,7 @@ def create_conditions(self, obj): formset = self.get_context_data()["conditions_formset"] excluded_sids = [] conditions_data = [] - workbasket = WorkBasket.current(self.request) - existing_conditions = obj.conditions.approved_up_to_transaction( - workbasket.get_current_transaction(self.request), - ) + existing_conditions = obj.conditions.current() for f in formset.forms: f.is_valid() diff --git a/quotas/business_rules.py b/quotas/business_rules.py index 5d625626c..04e33d02c 100644 --- a/quotas/business_rules.py +++ b/quotas/business_rules.py @@ -34,9 +34,7 @@ def validate(self, quota_definition): related_model = self.get_relation_model(quota_definition) if quota_definition.update_type == UpdateType.DELETE: kwargs = {f"{self.sid_prefix}sid": quota_definition.sid} - if related_model.objects.approved_up_to_transaction( - transaction=quota_definition.transaction, - ).filter(**kwargs): + if related_model.objects.current().filter(**kwargs): raise self.violation(quota_definition) @@ -53,7 +51,7 @@ class ON2(BusinessRule): def validate(self, order_number): if ( type(order_number) - .objects.approved_up_to_transaction(order_number.transaction) + .objects.current() .filter( order_number=order_number.order_number, valid_between__overlap=order_number.valid_between, @@ -73,9 +71,7 @@ def validate(self, order_number): origin_exists = False for order_number_version in order_number_versions: if ( - order_number_version.origins.approved_up_to_transaction( - order_number.transaction, - ).count() + order_number_version.origins.current().count() > 0 ): origin_exists = True @@ -92,7 +88,7 @@ class ON5(BusinessRule): def validate(self, origin): if ( type(origin) - .objects.approved_up_to_transaction(origin.transaction) + .objects.current() .filter( order_number__sid=origin.order_number.sid, geographical_area__sid=origin.geographical_area.sid, @@ -186,9 +182,7 @@ def validate(self, order_number_origin): check that there are no measures linked to the origin . """ - measures = measures_models.Measure.objects.approved_up_to_transaction( - order_number_origin.transaction, - ) + measures = measures_models.Measure.objects.current() if not measures.exists(): return @@ -326,9 +320,7 @@ class OverlappingQuotaDefinition(BusinessRule): def validate(self, quota_definition): potential_quota_definition_matches = ( type(quota_definition) - .objects.approved_up_to_transaction( - quota_definition.transaction, - ) + .objects.current() .filter( order_number=quota_definition.order_number, valid_between__overlap=quota_definition.valid_between, @@ -358,9 +350,7 @@ def validate(self, quota_definition): if quota_definition.valid_between.lower < datetime.date.today(): return True - if quota_definition.sub_quota_associations.approved_up_to_transaction( - self.transaction, - ).exists(): + if quota_definition.sub_quota_associations.current().exists(): return True if quota_definition.volume != quota_definition.initial_volume: @@ -471,9 +461,7 @@ class QA6(BusinessRule): def validate(self, association): if ( - association.main_quota.sub_quota_associations.approved_up_to_transaction( - association.transaction, - ) + association.main_quota.sub_quota_associations.current() .values( "sub_quota_relation_type", ) diff --git a/quotas/models.py b/quotas/models.py index db28e5694..5d2b88afd 100644 --- a/quotas/models.py +++ b/quotas/models.py @@ -170,8 +170,8 @@ class QuotaOrderNumberOrigin(TrackedModel, ValidityMixin): UpdateValidity, ) - def order_number_in_use(self, transaction): - return self.order_number.in_use(transaction) + def order_number_in_use(self): + return self.order_number.in_use() @property def structure_description(self): diff --git a/quotas/tests/test_views.py b/quotas/tests/test_views.py index e6efe2a3f..b00c08e43 100644 --- a/quotas/tests/test_views.py +++ b/quotas/tests/test_views.py @@ -728,12 +728,10 @@ def test_quota_edit_origin_new_versions(valid_user_client): tx = Transaction.objects.last() - quota = models.QuotaOrderNumber.objects.approved_up_to_transaction(tx).get( + quota = models.QuotaOrderNumber.objects.current().get( sid=quota.sid, ) - origins = models.QuotaOrderNumberOrigin.objects.approved_up_to_transaction( - tx, - ).filter( + origins = models.QuotaOrderNumberOrigin.objects.current().filter( order_number=quota, ) @@ -779,7 +777,7 @@ def test_quota_edit_origin_exclusions( tx = Transaction.objects.last() - origin = models.QuotaOrderNumberOrigin.objects.approved_up_to_transaction(tx).get( + origin = models.QuotaOrderNumberOrigin.objects.current().get( sid=origin.sid, ) @@ -837,25 +835,21 @@ def test_quota_edit_origin_exclusions_remove( tx = Transaction.objects.last() - updated_quota = models.QuotaOrderNumber.objects.approved_up_to_transaction(tx).get( + updated_quota = models.QuotaOrderNumber.objects.current().get( sid=quota.sid, ) updated_origin = ( - updated_quota.quotaordernumberorigin_set.approved_up_to_transaction(tx) + updated_quota.quotaordernumberorigin_set.current() ).first() assert ( - updated_origin.quotaordernumberoriginexclusion_set.approved_up_to_transaction( - tx, - ).count() + updated_origin.quotaordernumberoriginexclusion_set.current().count() == 0 ) assert ( country1 - not in updated_origin.quotaordernumberoriginexclusion_set.approved_up_to_transaction( - tx, - ) + not in updated_origin.quotaordernumberoriginexclusion_set.current() ) @@ -896,11 +890,8 @@ def test_update_quota_definition(valid_user_client, date_ranges): kwargs={"sid": quota_definition.sid}, ) - tx = Transaction.objects.last() - updated_definition = models.QuotaDefinition.objects.approved_up_to_transaction( - tx, - ).get( + updated_definition = models.QuotaDefinition.objects.current().get( sid=quota_definition.sid, ) @@ -978,8 +969,7 @@ def test_quota_create_origin( assert response.status_code == 302 - tx = Transaction.objects.last() - origin = models.QuotaOrderNumberOrigin.objects.approved_up_to_transaction(tx).get( + origin = models.QuotaOrderNumberOrigin.objects.current().get( sid=response.url.split("/")[2], ) diff --git a/quotas/views.py b/quotas/views.py index b20210bb8..28c1a280d 100644 --- a/quotas/views.py +++ b/quotas/views.py @@ -50,8 +50,7 @@ class QuotaOrderNumberViewset(viewsets.ReadOnlyModelViewSet): filter_backends = [OrderNumberFilterBackend] def get_queryset(self): - tx = WorkBasket.get_current_transaction(self.request) - return models.QuotaOrderNumber.objects.approved_up_to_transaction(tx) + return models.QuotaOrderNumber.objects.current() class QuotaOrderNumberOriginViewset(viewsets.ReadOnlyModelViewSet): @@ -101,8 +100,7 @@ class QuotaOrderNumberMixin: model = models.QuotaOrderNumber def get_queryset(self): - tx = WorkBasket.get_current_transaction(self.request) - return models.QuotaOrderNumber.objects.approved_up_to_transaction(tx) + return models.QuotaOrderNumber.objects.current() class QuotaList(QuotaOrderNumberMixin, TamatoListView): @@ -277,9 +275,7 @@ def get_result_object(self, form): object = super().get_result_object(form) existing_origins = ( - models.QuotaOrderNumberOrigin.objects.approved_up_to_transaction( - object.transaction, - ).filter( + models.QuotaOrderNumberOrigin.objects.current().filter( order_number__sid=object.sid, ) ) @@ -325,7 +321,7 @@ class QuotaOrderNumberOriginMixin: def get_queryset(self): tx = WorkBasket.get_current_transaction(self.request) - return models.QuotaOrderNumberOrigin.objects.approved_up_to_transaction(tx) + return models.QuotaOrderNumberOrigin.objects.current() class QuotaOrderNumberOriginUpdateMixin( @@ -465,7 +461,7 @@ class QuotaDefinitionMixin: def get_queryset(self): tx = WorkBasket.get_current_transaction(self.request) - return models.QuotaDefinition.objects.approved_up_to_transaction(tx) + return models.QuotaDefinition.objects.current() class QuotaDefinitionUpdateMixin( diff --git a/regulations/business_rules.py b/regulations/business_rules.py index c308a7955..e55f79d05 100644 --- a/regulations/business_rules.py +++ b/regulations/business_rules.py @@ -32,7 +32,7 @@ def validate(self, regulation): generating_regulation__regulation_id=regulation.regulation_id, generating_regulation__role_type=regulation.role_type, ) - .approved_up_to_transaction(self.transaction) + .transaction() .exclude( valid_between__contained_by=regulation.valid_between, ) diff --git a/regulations/forms.py b/regulations/forms.py index 9c37829b5..dc206d4f1 100644 --- a/regulations/forms.py +++ b/regulations/forms.py @@ -178,11 +178,10 @@ def _get_next_part_value(self, partial_regulation_id): RegulationCreateForm._make_partial_regulation_id()).""" tx = WorkBasket.get_current_transaction(self.request) last_matching_regulation = ( - Regulation.objects.filter( + Regulation.objects.current().filter( regulation_id__startswith=partial_regulation_id, role_type=FIXED_ROLE_TYPE, ) - .approved_up_to_transaction(tx) .order_by("-regulation_id") .first() ) diff --git a/regulations/models.py b/regulations/models.py index 01880e41e..420a3ada6 100644 --- a/regulations/models.py +++ b/regulations/models.py @@ -256,7 +256,7 @@ def used_as_terminating_regulation_or_draft_generating_and_terminating_regulatio generating_regulation__regulation_id=self.regulation_id, generating_regulation__role_type=self.role_type, ) - .approved_up_to_transaction(transaction) + .current() .exists() ) diff --git a/regulations/views.py b/regulations/views.py index 6426e0008..f29e536bd 100644 --- a/regulations/views.py +++ b/regulations/views.py @@ -36,7 +36,7 @@ class RegulationViewSet(viewsets.ReadOnlyModelViewSet): def get_queryset(self): tx = WorkBasket.get_current_transaction(self.request) - return Regulation.objects.approved_up_to_transaction(tx).select_related( + return Regulation.objects.current().select_related( "regulation_group", ) @@ -55,7 +55,7 @@ class RegulationMixin: def get_queryset(self): tx = WorkBasket.get_current_transaction(self.request) - return Regulation.objects.approved_up_to_transaction(tx).select_related( + return Regulation.objects.current().select_related( "regulation_group", ) @@ -180,7 +180,7 @@ class RegulationConfirmCreate(TrackedModelDetailView): def get_queryset(self): tx = WorkBasket.get_current_transaction(self.request) - return Regulation.objects.approved_up_to_transaction(tx) + return Regulation.objects.current() class RegulationUpdate( diff --git a/workbaskets/views/mixins.py b/workbaskets/views/mixins.py index 83a626352..671392774 100644 --- a/workbaskets/views/mixins.py +++ b/workbaskets/views/mixins.py @@ -10,9 +10,5 @@ def workbasket(self) -> WorkBasket: def get_queryset(self): qs = super().get_queryset() - transaction = None - current = self.workbasket - if current: - transaction = current.transactions.last() - return qs.approved_up_to_transaction(transaction) + return qs.current() From d4d72d44be8647bb98aff5899c0869da30856116 Mon Sep 17 00:00:00 2001 From: nboyse Date: Mon, 13 Nov 2023 15:05:27 +0000 Subject: [PATCH 2/9] linting and test correction --- additional_codes/forms.py | 8 ++++---- commodities/business_rules.py | 3 ++- commodities/forms.py | 4 +--- commodities/models/dc.py | 3 +-- commodities/tests/test_views.py | 4 +--- common/business_rules.py | 6 ++++-- common/tests/test_models.py | 5 +---- conftest.py | 4 +++- measures/business_rules.py | 12 ++++-------- measures/forms.py | 11 +++-------- measures/tests/factories.py | 8 ++------ measures/tests/test_views.py | 4 +--- measures/views.py | 4 +--- quotas/business_rules.py | 5 +---- quotas/tests/test_views.py | 15 +++------------ quotas/views.py | 6 ++---- regulations/forms.py | 3 ++- 17 files changed, 36 insertions(+), 69 deletions(-) diff --git a/additional_codes/forms.py b/additional_codes/forms.py index 879716c7e..515769470 100644 --- a/additional_codes/forms.py +++ b/additional_codes/forms.py @@ -156,10 +156,10 @@ def save(self, commit=True): instance = super().save(commit=False) highest_sid = ( - models.AdditionalCode.objects.current().aggregate( - Max("sid"), - )["sid__max"] - ) or 0 + models.AdditionalCode.objects.current().aggregate( + Max("sid"), + )["sid__max"] + ) or 0 instance.sid = highest_sid + 1 if commit: diff --git a/commodities/business_rules.py b/commodities/business_rules.py index f5b08a00a..c1cc5e87e 100644 --- a/commodities/business_rules.py +++ b/commodities/business_rules.py @@ -158,7 +158,8 @@ def validate(self, good): if not ( good.code.is_chapter - or GoodsNomenclatureOrigin.objects.current().filter( + or GoodsNomenclatureOrigin.objects.current() + .filter( new_goods_nomenclature__sid=good.sid, ) .exists() diff --git a/commodities/forms.py b/commodities/forms.py index c01fec547..dc07a074e 100644 --- a/commodities/forms.py +++ b/commodities/forms.py @@ -74,9 +74,7 @@ def init_fields(self): self.fields[ "end_date" ].help_text = "Leave empty if the footnote is needed for an unlimited time" - self.fields[ - "associated_footnote" - ].queryset = Footnote.objects.current().filter( + self.fields["associated_footnote"].queryset = Footnote.objects.current().filter( footnote_type__application_code__in=[1, 2], ) self.fields[ diff --git a/commodities/models/dc.py b/commodities/models/dc.py index 3c9bd5ed0..a9caf5837 100644 --- a/commodities/models/dc.py +++ b/commodities/models/dc.py @@ -852,8 +852,7 @@ def _get_snapshot_commodities( that match the latest_version goods. """ item_ids = {c.item_id for c in self.commodities if c.obj} - goods = GoodsNomenclature.objects.current( - ).filter( + goods = GoodsNomenclature.objects.current().filter( item_id__in=item_ids, valid_between__contains=snapshot_date, ) diff --git a/commodities/tests/test_views.py b/commodities/tests/test_views.py index 41b2082fb..9968fbe7d 100644 --- a/commodities/tests/test_views.py +++ b/commodities/tests/test_views.py @@ -522,9 +522,7 @@ def test_commodity_footnote_update_success(valid_user_client, date_ranges): "end_date": "", } response = valid_user_client.post(url, data) - updated_association = ( - FootnoteAssociationGoodsNomenclature.objects.current().first() - ) + updated_association = FootnoteAssociationGoodsNomenclature.objects.current().first() assert response.status_code == 302 assert response.url == updated_association.get_url("confirm-update") diff --git a/common/business_rules.py b/common/business_rules.py index c7d6a9377..97c7414a1 100644 --- a/common/business_rules.py +++ b/common/business_rules.py @@ -278,7 +278,8 @@ def validate(self, model): if ( type(model) - .objects.current().filter(**query) + .objects.current() + .filter(**query) .exclude(version_group=model.version_group) .exists() ): @@ -304,7 +305,8 @@ def validate(self, model): query["valid_between__overlap"] = model.valid_between if ( - model.__class__.objects.current().filter(**query) + model.__class__.objects.current() + .filter(**query) .exclude(version_group=model.version_group) .exists() ): diff --git a/common/tests/test_models.py b/common/tests/test_models.py index 34745fbf3..9da564e9f 100644 --- a/common/tests/test_models.py +++ b/common/tests/test_models.py @@ -309,10 +309,7 @@ def test_current_as_of(sample_model): ) assert models.TestModel1.objects.latest_approved().get().pk == sample_model.pk - assert ( - models.TestModel1.objects.current().get().pk - == unapproved_version.pk - ) + assert models.TestModel1.objects.current().get().pk == unapproved_version.pk def test_create_with_description(): diff --git a/conftest.py b/conftest.py index 315c588eb..8beb83b14 100644 --- a/conftest.py +++ b/conftest.py @@ -835,7 +835,9 @@ def check( assert workbasket is not None try: - imported = model_class.objects.current().get(**db_kwargs) + imported = model_class.objects.approved_up_to_transaction( + workbasket.current_transaction, + ).get(**db_kwargs) except model_class.DoesNotExist: if model.update_type == UpdateType.DELETE: imported = ( diff --git a/measures/business_rules.py b/measures/business_rules.py index 3c2222b69..6b246b255 100644 --- a/measures/business_rules.py +++ b/measures/business_rules.py @@ -212,7 +212,8 @@ def validate(self, measure): goods = ( type(measure.goods_nomenclature) - .objects.current().filter( + .objects.current() + .filter( sid=measure.goods_nomenclature.sid, valid_between__overlap=measure.effective_valid_between, ) @@ -613,8 +614,7 @@ def validate(self, measure): ) if ( measure.additional_code - and not AdditionalCodeTypeMeasureType.objects.current( - ) + and not AdditionalCodeTypeMeasureType.objects.current() .filter( additional_code_type__sid=measure.additional_code.type.sid, measure_type__sid=measure.measure_type.sid, @@ -887,11 +887,7 @@ def validate(self, measure): == ApplicabilityCode.MANDATORY, } ) - if ( - components.filter(inapplicable) - .current() - .exists() - ): + if components.filter(inapplicable).current().exists(): raise self.violation(measure, self.messages[code].format(self)) diff --git a/measures/forms.py b/measures/forms.py index 76ae8dda8..8d8f01f6c 100644 --- a/measures/forms.py +++ b/measures/forms.py @@ -540,10 +540,8 @@ def __init__(self, *args, **kwargs): # store all the pks of a measure's footnotes on the session, using the measure sid as key if f"instance_footnotes_{self.instance.sid}" not in self.request.session.keys(): tx = WorkBasket.get_current_transaction(self.request) - associations = ( - models.FootnoteAssociationMeasure.objects.current().filter( - footnoted_measure__sid=self.instance.sid, - ) + associations = models.FootnoteAssociationMeasure.objects.current().filter( + footnoted_measure__sid=self.instance.sid, ) self.request.session[f"instance_footnotes_{self.instance.sid}"] = [ a.associated_footnote.pk for a in associations @@ -714,10 +712,7 @@ def save(self, commit=True): ) for pk in footnote_pks: - footnote = ( - Footnote.objects.current().filter(pk=pk) - .first() - ) + footnote = Footnote.objects.current().filter(pk=pk).first() existing_association = ( models.FootnoteAssociationMeasure.objects.current() diff --git a/measures/tests/factories.py b/measures/tests/factories.py index 850f4d449..b0bfd13bb 100644 --- a/measures/tests/factories.py +++ b/measures/tests/factories.py @@ -31,15 +31,11 @@ class Meta: measure_type_description = factory.SelfAttribute("measure.measure_type.description") duty_sentence = factory.sequence(lambda n: f"{n}.00%") origin_description = factory.LazyAttribute( - lambda m: m.measure.geographical_area.descriptions.current() - .last() - .description, + lambda m: m.measure.geographical_area.descriptions.current().last().description, ) excluded_origin_descriptions = factory.LazyAttribute( lambda m: random.choice(MeasureSheetRow.separators).join( - e.excluded_geographical_area.descriptions.current() - .last() - .description + e.excluded_geographical_area.descriptions.current().last().description for e in m.measure.exclusions.all() ), ) diff --git a/measures/tests/test_views.py b/measures/tests/test_views.py index e7548fda4..4e89d11c5 100644 --- a/measures/tests/test_views.py +++ b/measures/tests/test_views.py @@ -1062,9 +1062,7 @@ def test_measure_update_group_exclusion(client, valid_user, erga_omnes): assert not MeasureExcludedGeographicalArea.objects.current().exists() client.post(url, data=data) - measure_area_exclusions = ( - MeasureExcludedGeographicalArea.objects.current() - ) + measure_area_exclusions = MeasureExcludedGeographicalArea.objects.current() assert measure_area_exclusions.count() == 2 diff --git a/measures/views.py b/measures/views.py index b8775cf51..4d0793802 100644 --- a/measures/views.py +++ b/measures/views.py @@ -1120,9 +1120,7 @@ def get_footnotes(self, measure): return [a.associated_footnote for a in associations] def get_conditions(self, measure): - return ( - measure.conditions.with_reference_price_string().current() - ) + return measure.conditions.with_reference_price_string().current() def get_context_data(self, **kwargs): context = super().get_context_data(**kwargs) diff --git a/quotas/business_rules.py b/quotas/business_rules.py index 04e33d02c..cf6776086 100644 --- a/quotas/business_rules.py +++ b/quotas/business_rules.py @@ -70,10 +70,7 @@ def validate(self, order_number): order_number_versions = order_number.get_versions() origin_exists = False for order_number_version in order_number_versions: - if ( - order_number_version.origins.current().count() - > 0 - ): + if order_number_version.origins.current().count() > 0: origin_exists = True break diff --git a/quotas/tests/test_views.py b/quotas/tests/test_views.py index b00c08e43..39e1a97cd 100644 --- a/quotas/tests/test_views.py +++ b/quotas/tests/test_views.py @@ -838,19 +838,11 @@ def test_quota_edit_origin_exclusions_remove( updated_quota = models.QuotaOrderNumber.objects.current().get( sid=quota.sid, ) - updated_origin = ( - updated_quota.quotaordernumberorigin_set.current() - ).first() + updated_origin = (updated_quota.quotaordernumberorigin_set.current()).first() - assert ( - updated_origin.quotaordernumberoriginexclusion_set.current().count() - == 0 - ) + assert updated_origin.quotaordernumberoriginexclusion_set.current().count() == 0 - assert ( - country1 - not in updated_origin.quotaordernumberoriginexclusion_set.current() - ) + assert country1 not in updated_origin.quotaordernumberoriginexclusion_set.current() def test_update_quota_definition_page_200(valid_user_client): @@ -890,7 +882,6 @@ def test_update_quota_definition(valid_user_client, date_ranges): kwargs={"sid": quota_definition.sid}, ) - updated_definition = models.QuotaDefinition.objects.current().get( sid=quota_definition.sid, ) diff --git a/quotas/views.py b/quotas/views.py index 28c1a280d..83f867882 100644 --- a/quotas/views.py +++ b/quotas/views.py @@ -274,10 +274,8 @@ class QuotaUpdateMixin( def get_result_object(self, form): object = super().get_result_object(form) - existing_origins = ( - models.QuotaOrderNumberOrigin.objects.current().filter( - order_number__sid=object.sid, - ) + existing_origins = models.QuotaOrderNumberOrigin.objects.current().filter( + order_number__sid=object.sid, ) # this will be needed even if origins have not been edited in the form diff --git a/regulations/forms.py b/regulations/forms.py index dc206d4f1..e6b8ef614 100644 --- a/regulations/forms.py +++ b/regulations/forms.py @@ -178,7 +178,8 @@ def _get_next_part_value(self, partial_regulation_id): RegulationCreateForm._make_partial_regulation_id()).""" tx = WorkBasket.get_current_transaction(self.request) last_matching_regulation = ( - Regulation.objects.current().filter( + Regulation.objects.current() + .filter( regulation_id__startswith=partial_regulation_id, role_type=FIXED_ROLE_TYPE, ) From c9262b76aa339652867e94153bce203502a08fa1 Mon Sep 17 00:00:00 2001 From: Charlie Prichard <46421052+CPrich905@users.noreply.github.com> Date: Tue, 14 Nov 2023 09:47:22 +0000 Subject: [PATCH 3/9] TP-2000-1085 Replace View Violations and Worksheet checker with a blended tab (#1078) --- workbaskets/forms.py | 4 +- .../includes/workbaskets/navigation.jinja | 9 +- .../workbasket-business-rules.jinja | 138 ++++++++++++++++++ workbaskets/jinja2/workbaskets/checks.jinja | 35 +++++ workbaskets/jinja2/workbaskets/compare.jinja | 24 ++- workbaskets/jinja2/workbaskets/review.jinja | 2 +- .../workbaskets/summary-workbasket.jinja | 135 ----------------- .../jinja2/workbaskets/violation_detail.jinja | 1 - .../jinja2/workbaskets/violations.jinja | 5 +- workbaskets/tests/test_views.py | 35 ++--- workbaskets/urls.py | 7 +- workbaskets/views/ui.py | 107 ++++++++++---- 12 files changed, 304 insertions(+), 198 deletions(-) create mode 100644 workbaskets/jinja2/includes/workbaskets/workbasket-business-rules.jinja create mode 100644 workbaskets/jinja2/workbaskets/checks.jinja diff --git a/workbaskets/forms.py b/workbaskets/forms.py index 15ea74aaa..904b3657d 100644 --- a/workbaskets/forms.py +++ b/workbaskets/forms.py @@ -156,7 +156,9 @@ def cleaned_data_no_prefix(self): class WorkbasketCompareForm(forms.Form): data = forms.CharField( label="Compare worksheet data against the measures in this workbasket", - widget=forms.Textarea(), + widget=forms.Textarea( + attrs={"placeholder": "Add your worksheet data here"}, + ), validators=[SymbolValidator], ) diff --git a/workbaskets/jinja2/includes/workbaskets/navigation.jinja b/workbaskets/jinja2/includes/workbaskets/navigation.jinja index 18a73de00..c07e13741 100644 --- a/workbaskets/jinja2/includes/workbaskets/navigation.jinja +++ b/workbaskets/jinja2/includes/workbaskets/navigation.jinja @@ -7,14 +7,11 @@
  • Add/edit items
    • -
  • - View violations ({{request.session.workbasket.error_count}}) +
  • + Check workbasket
  • - Review ({{request.session.workbasket.measure_count}}) -
    • -
  • - Worksheet check + Review changes
    • diff --git a/workbaskets/jinja2/includes/workbaskets/workbasket-business-rules.jinja b/workbaskets/jinja2/includes/workbaskets/workbasket-business-rules.jinja new file mode 100644 index 000000000..919e14d0e --- /dev/null +++ b/workbaskets/jinja2/includes/workbaskets/workbasket-business-rules.jinja @@ -0,0 +1,138 @@ +{% from "components/button/macro.njk" import govukButton %} + +{% set run_business_rules_form_icon %} +
    + + + +
    +{% endset %} + + +{% set run_business_rules_form %} +
    + + + {{ govukButton({ + "text": "Run business rules", + "classes": "govuk-button--secondary govuk-!-margin-0", + "name": "form-action", + "value": "run-business-rules", + "preventDoubleClick": true, + }) }} +
    +{% endset %} + +{% macro rule_check_result_content() %} +

    + + Number of changes: {{workbasket.tracked_models.count()}}   + + Review changes + + Last Run: ({{ "{:%d %b %Y %H:%M}".format(localtime(workbasket.tracked_model_checks.order_by("created_at").last().created_at)) }}) + {{ run_business_rules_form_icon }} + +

    +

    + Number of violations: {{workbasket.tracked_model_check_errors.count()}} +

    +{% endmacro %} + +{% set terminate_rule_check_form %} +
    + + + {{ govukButton({ + "text": "Stop rule check", + "classes": "govuk-button--warning govuk-!-margin-0", + "name": "form-action", + "value": "terminate-rule-check", + "preventDoubleClick": true, + }) }} +
    +{% endset %} + +{% if rule_check_in_progress %} + {% set live_rule_check_status %} + Rule check in progress. Checking {{ workbasket.tracked_models.count() }} objects in basket. + {{ rule_check_progress }}. + Come back later or refresh to see results. + {% endset %} + {% set rule_check_button = terminate_rule_check_form %} + {% set rule_check_block %}{% endset %} + +{% elif workbasket.tracked_model_checks.exists() and not workbasket.unchecked_or_errored_transactions.exists() %} + {% set live_rule_check_status = "Business rule check passed. No business rule violations" %} + {% set rule_check_button = run_business_rules_form %} + {% set rule_check_block %} +
    +
    + {{ rule_check_result_content() }} + {% if unsent_notification %} +
    + For commodity code imports a channel islands notification must be sent from the review goods tab before packaging. +
    + + Send to packaging queue + + {% else %} + + Send to packaging queue + + {% endif %} +
    +
    + {% endset %} + +{% elif workbasket.tracked_model_check_errors.exists() and not rule_check_in_progress %} + {% set live_rule_check_status = "Business rule check failed. Please check the box above for more details" %} + {% set rule_check_button = run_business_rules_form %} + {% set rule_check_block %} + + {% endset %} + +{% elif workbasket.tracked_models.exists() %} + {% set live_rule_check_status = "Business rule check has not been run" %} + {% set rule_check_button = run_business_rules_form %} + {% set rule_check_block %}{% endset %} + +{% else %} + {% set live_rule_check_status = "No objects in workbasket" %} + {% set rule_check_button %}{% endset %} + {% set rule_check_block %}{% endset %} +{% endif %} +{{rule_check_block}} +
    +
    +
    + Live status +
    +
    + {{ live_rule_check_status }} +
    +
    + {{ rule_check_button }} +
    +
    +
    \ No newline at end of file diff --git a/workbaskets/jinja2/workbaskets/checks.jinja b/workbaskets/jinja2/workbaskets/checks.jinja new file mode 100644 index 000000000..6c02c08e8 --- /dev/null +++ b/workbaskets/jinja2/workbaskets/checks.jinja @@ -0,0 +1,35 @@ +{% extends "layouts/layout.jinja" %} +{% from "components/button/macro.njk" import govukButton %} +{% from "includes/workbaskets/navigation.jinja" import navigation %} + +{% set page_title %} + Workbasket {{ workbasket.id if workbasket else request.session.workbasket.id }} - Checks +{% endset %} + +{% block content %} +

    {{ page_title }}

    + {{ navigation(request, "check") }} + + {% include "includes/workbaskets/workbasket-business-rules.jinja" %} +
    +
    +
    +
    + Worksheet check +
    +
    + Compare worksheet data with workbasket data +
    +
    + {{ + govukButton({ + "text": "Compare data", + "href": url("workbaskets:workbasket-check-ui-compare"), + "classes": "govuk-button--secondary govuk-!-margin-0" + })}} +
    +
    +
    +
    + +{% endblock %} \ No newline at end of file diff --git a/workbaskets/jinja2/workbaskets/compare.jinja b/workbaskets/jinja2/workbaskets/compare.jinja index 683b964f6..25982b7f6 100644 --- a/workbaskets/jinja2/workbaskets/compare.jinja +++ b/workbaskets/jinja2/workbaskets/compare.jinja @@ -9,7 +9,7 @@ {% from "components/table/macro.njk" import govukTable %} {% set page_title %} - Workbasket {{ workbasket.id if workbasket else request.session.workbasket.id }} - Summary + Workbasket {{ workbasket.id if workbasket else request.session.workbasket.id }} - Compare with worksheet data {% endset %} {% set change_workbasket_details_link = url("workbaskets:workbasket-ui-update", kwargs={"pk": workbasket.pk}) %} @@ -19,7 +19,9 @@ "items": [ {"text": "Home", "href": url("home")}, {"text": "Edit an existing workbasket", "href": url("workbaskets:workbasket-ui-list")}, - {"text": "Workbasket " ~ request.session.workbasket.id ~ " - Summary" } + {"text": "Workbasket " ~ request.session.workbasket.id ~ " - Summary", "href": url("workbaskets:current-workbasket") }, + {"text": "Workbasket " ~ request.session.workbasket.id ~ " - Checks", "href": url("workbaskets:workbasket-checks") }, + {"text": "Workbasket " ~ request.session.workbasket.id ~ " - Compare" } ]}) }} {% endblock %} @@ -29,15 +31,23 @@ {{ page_title }} - {{ navigation(request, "compare") }} + {{ navigation(request, "check") }} +

    Introduction

    +

    You can use this checker to compare worksheet measure data (specifically commodity codes, duty rates, start dates and end dates) with measures in your workbasket. It will list all matching measures that are in both your worksheet and workbasket.

    +

    Currently, it won’t show you any measures that are in your worksheet but not in your workbasket and vice versa. It also won’t check whether you are comparing the same number of worksheet and workbasket measures.

    + +

    Worksheet data

    +

    Paste your worksheet data below. This must include commodity codes, duty rates and start dates. End dates are optional.

    +

    If you are pasting both start and end dates, the start dates must appear before the end dates.

    +

    Duty rates must be to 3 decimal places and dates must be in a DD/MM/YYYY format (e.g. 01/01/2024).

    {% call django_form() %} {{ crispy(form) }} {% endcall %} {% if data_upload %} -

    Worksheet data

    - +

    Formatted worksheet data

    +

    This is your worksheet data in a table format.

    {% set table_rows = [] %} {% for row in data_upload.serialized %} {{ table_rows.append([ @@ -60,9 +70,9 @@ }) }} {% endif %} - +

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

    {% if matching_measures %} -

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

    +

    These are measures that are the same across both your worksheet and your workbasket.

    {% set table_rows = [] %} {% for measure in matching_measures %} diff --git a/workbaskets/jinja2/workbaskets/review.jinja b/workbaskets/jinja2/workbaskets/review.jinja index 891898da9..229862ac7 100644 --- a/workbaskets/jinja2/workbaskets/review.jinja +++ b/workbaskets/jinja2/workbaskets/review.jinja @@ -5,7 +5,7 @@ {% from "macros/fake_tabs.jinja" import fake_tabs %} {% if workbasket == session_workbasket %} - {% set page_title %}Workbasket {{ workbasket.id }} - Review{% endset %} + {% set page_title %}Workbasket {{ workbasket.id }} - Review changes{% endset %} {% else %} {% set page_title %} Workbasket {{ workbasket.id }} - {{ workbasket.status }} {% endset %} {% endif %} diff --git a/workbaskets/jinja2/workbaskets/summary-workbasket.jinja b/workbaskets/jinja2/workbaskets/summary-workbasket.jinja index 46d4014a7..fc267f2b7 100644 --- a/workbaskets/jinja2/workbaskets/summary-workbasket.jinja +++ b/workbaskets/jinja2/workbaskets/summary-workbasket.jinja @@ -10,128 +10,6 @@ {% set change_workbasket_details_link = url("workbaskets:workbasket-ui-update", kwargs={"pk": workbasket.pk}) %} -{% set run_business_rules_form_icon %} -
    - - - -
    -{% endset %} - -{% set run_business_rules_form %} -
    - - - {{ govukButton({ - "text": "Run business rules", - "classes": "govuk-button--secondary govuk-!-margin-0", - "name": "form-action", - "value": "run-business-rules", - "preventDoubleClick": true, - }) }} -
    -{% endset %} - -{% macro rule_check_result_content() %} -

    - - Number of changes: {{workbasket.tracked_models.count()}}   - - Review changes - - Last Run: ({{ "{:%d %b %Y %H:%M}".format(localtime(workbasket.tracked_model_checks.order_by("created_at").last().created_at)) }}) - {{ run_business_rules_form_icon }} - -

    -

    - Number of violations: {{workbasket.tracked_model_check_errors.count()}} -

    -{% endmacro %} - -{% set terminate_rule_check_form %} -
    - - - {{ govukButton({ - "text": "Stop rule check", - "classes": "govuk-button--warning govuk-!-margin-0", - "name": "form-action", - "value": "terminate-rule-check", - "preventDoubleClick": true, - }) }} -
    -{% endset %} - -{% if rule_check_in_progress %} - {% set live_rule_check_status %} - Rule check in progress. Checking {{ workbasket.tracked_models.count() }} objects in basket. - {{ rule_check_progress }}. - Come back later or refresh to see results. - {% endset %} - {% set rule_check_button = terminate_rule_check_form %} - {% set rule_check_block %}{% endset %} - -{% elif workbasket.tracked_model_checks.exists() and not workbasket.unchecked_or_errored_transactions.exists() %} - {% set live_rule_check_status = "Business rule check passed. No business rule violations" %} - {% set rule_check_button = run_business_rules_form %} - {% set rule_check_block %} -
    -
    - {{ rule_check_result_content() }} - {% if unsent_notification %} -
    - For commodity code imports a channel islands notification must be sent from the review goods tab before packaging. -
    - - Send to packaging queue - - {% else %} - - Send to packaging queue - - {% endif %} -
    -
    - {% endset %} - -{% elif workbasket.tracked_model_check_errors.exists() and not rule_check_in_progress %} - {% set live_rule_check_status = "Business rule check failed. Please check the box above for more details" %} - {% set rule_check_button = run_business_rules_form %} - {% set rule_check_block %} - - {% endset %} - -{% elif workbasket.tracked_models.exists() %} - {% set live_rule_check_status = "Business rule check has not been run" %} - {% set rule_check_button = run_business_rules_form %} - {% set rule_check_block %}{% endset %} - -{% else %} - {% set live_rule_check_status = "No objects in workbasket" %} - {% set rule_check_button %}{% endset %} - {% set rule_check_block %}{% endset %} -{% endif %} - {% block breadcrumb %} {{ govukBreadcrumbs({ "items": [ @@ -149,8 +27,6 @@ {{ navigation(request, "summary") }} - {{ rule_check_block }} -
    @@ -178,17 +54,6 @@
    -
    -
    - Live status -
    -
    - {{ live_rule_check_status }} -
    -
    - {{ rule_check_button }} -
    -
    {% include "includes/workbaskets/workbasket-selectable-items.jinja" %} diff --git a/workbaskets/jinja2/workbaskets/violation_detail.jinja b/workbaskets/jinja2/workbaskets/violation_detail.jinja index 3d6e55dad..c48f687db 100644 --- a/workbaskets/jinja2/workbaskets/violation_detail.jinja +++ b/workbaskets/jinja2/workbaskets/violation_detail.jinja @@ -47,7 +47,6 @@ details
    - {# Review violation #} {% if request.user.is_superuser %}
    diff --git a/workbaskets/jinja2/workbaskets/violations.jinja b/workbaskets/jinja2/workbaskets/violations.jinja index 8781a41f0..1b47f1890 100644 --- a/workbaskets/jinja2/workbaskets/violations.jinja +++ b/workbaskets/jinja2/workbaskets/violations.jinja @@ -24,7 +24,7 @@ {{ check.pk }} {%- endset %} {{ table_rows.append([ - {"html": check_link, }, + {"html": check_link }, {"text": check.model._meta.verbose_name.title()}, {"text": check.rule_code}, {"text": check.message}, @@ -51,7 +51,8 @@ {"text": "Item ID"}, {"html": item}, {"text": violation}, - {"text": "Description"}, + {"text": "Description", + "classes": "govuk-!-width-one-half"}, {"text": activity_date}, ], "rows": table_rows diff --git a/workbaskets/tests/test_views.py b/workbaskets/tests/test_views.py index b40880d06..4d922c42c 100644 --- a/workbaskets/tests/test_views.py +++ b/workbaskets/tests/test_views.py @@ -202,7 +202,7 @@ def test_review_workbasket_displays_rule_violation_summary( response = valid_user_client.get( reverse( - "workbaskets:current-workbasket", + "workbaskets:workbasket-checks", ), ) page = BeautifulSoup( @@ -702,7 +702,7 @@ def test_run_business_rules(check_workbasket, valid_user_client, session_workbas } session.save() url = reverse( - "workbaskets:current-workbasket", + "workbaskets:workbasket-checks", ) response = valid_user_client.post( url, @@ -723,13 +723,11 @@ def test_run_business_rules(check_workbasket, valid_user_client, session_workbas assert not session_workbasket.tracked_model_checks.exists() -def test_workbasket_business_rule_status(valid_user_client): +def test_workbasket_business_rule_status(valid_user_client, session_empty_workbasket): """Testing that the live status of a workbasket resets after an item has been updated, created or deleted in the workbasket.""" - workbasket = factories.WorkBasketFactory.create( - status=WorkflowStatus.EDITING, - ) - with workbasket.new_transaction() as transaction: + + with session_empty_workbasket.new_transaction() as transaction: footnote = factories.FootnoteFactory.create( transaction=transaction, footnote_type__transaction=transaction, @@ -739,9 +737,8 @@ def test_workbasket_business_rule_status(valid_user_client): model=footnote, successful=True, ) - workbasket.save_to_session(valid_user_client.session) - url = reverse("workbaskets:current-workbasket") + url = reverse("workbaskets:workbasket-checks") response = valid_user_client.get(url) page = BeautifulSoup(response.content.decode(response.charset)) success_banner = page.find( @@ -750,8 +747,8 @@ def test_workbasket_business_rule_status(valid_user_client): ) assert success_banner - footnote2 = factories.FootnoteFactory.create( - transaction=workbasket.new_transaction(), + factories.FootnoteFactory.create( + transaction=session_empty_workbasket.new_transaction(), ) response = valid_user_client.get(url) page = BeautifulSoup(response.content.decode(response.charset)) @@ -884,7 +881,7 @@ def test_submit_for_packaging_disabled( import_batch.save() response = valid_user_client.get( - reverse("workbaskets:current-workbasket"), + reverse("workbaskets:workbasket-checks"), ) assert response.status_code == 200 @@ -902,7 +899,7 @@ def test_terminate_rule_check(valid_user_client, session_workbasket): session_workbasket.rule_check_task_id = 123 url = reverse( - "workbaskets:current-workbasket", + "workbaskets:workbasket-checks", ) response = valid_user_client.post( url, @@ -1817,7 +1814,7 @@ def test_application_access_after_workbasket_delete( def test_workbasket_compare_200(valid_user_client, session_workbasket): - url = reverse("workbaskets:workbasket-ui-compare") + url = reverse("workbaskets:workbasket-check-ui-compare") response = valid_user_client.get(url) assert response.status_code == 200 @@ -1826,7 +1823,7 @@ def test_workbasket_compare_prev_uploaded(valid_user_client, session_workbasket) factories.GoodsNomenclatureFactory() factories.GoodsNomenclatureFactory() factories.DataUploadFactory(workbasket=session_workbasket) - url = reverse("workbaskets:workbasket-ui-compare") + url = reverse("workbaskets:workbasket-check-ui-compare") response = valid_user_client.get(url) assert "Worksheet data" in response.content.decode(response.charset) @@ -1835,7 +1832,7 @@ def test_workbasket_update_prev_uploaded(valid_user_client, session_workbasket): factories.GoodsNomenclatureFactory() factories.GoodsNomenclatureFactory() data_upload = factories.DataUploadFactory(workbasket=session_workbasket) - url = reverse("workbaskets:workbasket-ui-compare") + url = reverse("workbaskets:workbasket-check-ui-compare") data = { "data": ( "0000000001\t1.000%\t20/05/2021\t31/08/2024\n" @@ -1849,7 +1846,7 @@ def test_workbasket_update_prev_uploaded(valid_user_client, session_workbasket): def test_workbasket_compare_form_submit_302(valid_user_client, session_workbasket): - url = reverse("workbaskets:workbasket-ui-compare") + url = reverse("workbaskets:workbasket-check-ui-compare") data = { "data": ( "0000000001\t1.000%\t20/05/2021\t31/08/2024\n" @@ -1885,7 +1882,7 @@ def test_workbasket_compare_found_measures( component_measurement=None, ) - url = reverse("workbaskets:workbasket-ui-compare") + url = reverse("workbaskets:workbasket-check-ui-compare") data = { "data": ( # this first line should match the measure in the workbasket @@ -1902,7 +1899,7 @@ def test_workbasket_compare_found_measures( assert response2.status_code == 200 decoded = response2.content.decode(response2.charset) soup = BeautifulSoup(decoded, "html.parser") - assert "1 matching measure found" in soup.select("h2")[1].text + assert "1 matching measure found" in soup.select("h2")[3].text # previously uploaded data assert len(soup.select("tbody")[0].select("tr")) == 2 diff --git a/workbaskets/urls.py b/workbaskets/urls.py index 690c6d1e9..cd24dfae2 100644 --- a/workbaskets/urls.py +++ b/workbaskets/urls.py @@ -46,6 +46,11 @@ ui_views.EditWorkbasketView.as_view(), name="edit-workbasket", ), + path( + f"current/checks/", + ui_views.WorkBasketChecksView.as_view(), + name="workbasket-checks", + ), path( f"/review-additional-codes/", ui_views.WorkBasketReviewAdditionalCodesView.as_view(), @@ -104,7 +109,7 @@ path( f"compare/", ui_views.WorkBasketCompare.as_view(), - name="workbasket-ui-compare", + name="workbasket-check-ui-compare", ), path( f"/", diff --git a/workbaskets/views/ui.py b/workbaskets/views/ui.py index d4b357236..953acc255 100644 --- a/workbaskets/views/ui.py +++ b/workbaskets/views/ui.py @@ -326,8 +326,6 @@ class CurrentWorkBasket(FormView): # Form action mappings to URL names. action_success_url_names = { "submit-for-packaging": "publishing:packaged-workbasket-queue-ui-create", - "run-business-rules": "workbaskets:current-workbasket", - "terminate-rule-check": "workbaskets:current-workbasket", "page-prev": "workbaskets:current-workbasket", "page-next": "workbaskets:current-workbasket", "compare-data": "workbaskets:current-workbasket", @@ -376,30 +374,9 @@ def _append_url_page_param(self, url, form_action): page_number = page.next_page_number() return f"{url}?page={page_number}" - @atomic - def run_business_rules(self): - """Remove old checks, start new checks via a Celery task and save the - newly created task's ID on the workbasket.""" - workbasket = self.workbasket - workbasket.delete_checks() - task = call_check_workbasket_sync.apply_async( - (workbasket.pk,), - countdown=1, - ) - logger.info( - f"Started rule check against workbasket.id={workbasket.pk} " - f"on task.id={task.id}", - ) - workbasket.rule_check_task_id = task.id - workbasket.save() - def get_success_url(self): form_action = self.request.POST.get("form-action") - if form_action == "run-business-rules": - self.run_business_rules() - elif form_action == "terminate-rule-check": - self.workbasket.terminate_rule_check() - elif form_action in ["remove-selected", "remove-all"]: + if form_action in ["remove-selected", "remove-all"]: return reverse( "workbaskets:workbasket-ui-changes-delete", kwargs={"pk": self.workbasket.pk}, @@ -1044,7 +1021,7 @@ def get_context_data(self, **kwargs): class WorkBasketCompare(WithCurrentWorkBasket, FormView): - success_url = reverse_lazy("workbaskets:workbasket-ui-compare") + success_url = reverse_lazy("workbaskets:workbasket-check-ui-compare") template_name = "workbaskets/compare.jinja" form_class = forms.WorkbasketCompareForm @@ -1113,6 +1090,86 @@ def get_context_data(self, *args, **kwargs): ) +class WorkBasketChecksView(FormView): + template_name = "workbaskets/checks.jinja" + form_class = forms.SelectableObjectsForm + + # Form action mappings to URL names. + action_success_url_names = { + "run-business-rules": "workbaskets:workbasket-checks", + "terminate-rule-check": "workbaskets:workbasket-checks", + "page-prev": "workbaskets:workbasket-checks", + "page-next": "workbaskets:workbasket-checks", + } + + @property + def workbasket(self) -> WorkBasket: + return WorkBasket.current(self.request) + + @atomic + def run_business_rules(self): + """Remove old checks, start new checks via a Celery task and save the + newly created task's ID on the workbasket.""" + workbasket = self.workbasket + workbasket.delete_checks() + task = call_check_workbasket_sync.apply_async( + (workbasket.pk,), + countdown=1, + ) + logger.info( + f"Started rule check against workbasket.id={workbasket.pk} " + f"on task.id={task.id}", + ) + workbasket.rule_check_task_id = task.id + workbasket.save() + + def get_success_url(self): + form_action = self.request.POST.get("form-action") + if form_action == "run-business-rules": + self.run_business_rules() + elif form_action == "terminate-rule-check": + self.workbasket.terminate_rule_check() + return reverse("workbaskets:workbasket-checks") + + def get_context_data(self, **kwargs): + context = super().get_context_data(**kwargs) + # set to true if there is an associated goods import batch with an unsent notification + try: + import_batch = self.workbasket.importbatch + unsent_notifcation = ( + import_batch + and import_batch.goods_import + and not Notification.objects.filter( + notified_object_pk=import_batch.pk, + notification_type=NotificationTypeChoices.GOODS_REPORT, + ).exists() + ) + except ObjectDoesNotExist: + unsent_notifcation = False + context.update( + { + "workbasket": self.workbasket, + "rule_check_in_progress": False, + "unsent_notification": unsent_notifcation, + }, + ) + if self.workbasket.rule_check_task_id: + result = AsyncResult(self.workbasket.rule_check_task_id) + if result.status != "SUCCESS": + context.update({"rule_check_in_progress": True}) + else: + self.workbasket.save_to_session(self.request.session) + + num_completed, total = self.workbasket.rule_check_progress() + context.update( + { + "rule_check_progress": f"Completed {num_completed} out of {total} checks", + }, + ) + + return context + + class WorkBasketReviewView(PermissionRequiredMixin, WithPaginationListView): """Base view from which nested workbasket review tab views inherit.""" From 10b113ad5177afddb050d26070fecc267bb17695 Mon Sep 17 00:00:00 2001 From: nboyse Date: Tue, 14 Nov 2023 12:05:15 +0000 Subject: [PATCH 4/9] fix: lots of tests and formatting --- commodities/models/dc.py | 4 ++-- common/models/tracked_qs.py | 11 +++++++---- common/querysets.py | 2 +- footnotes/views.py | 3 --- geo_areas/forms.py | 1 - geo_areas/tests/test_views.py | 2 +- geo_areas/views.py | 2 -- measures/business_rules.py | 4 ++-- measures/forms.py | 2 -- measures/models.py | 4 ++-- measures/querysets.py | 4 +++- measures/tests/factories.py | 12 ++++++++++-- measures/tests/test_views.py | 5 ----- measures/views.py | 4 ---- quotas/business_rules.py | 25 ++++++++++++++++++------- 15 files changed, 46 insertions(+), 39 deletions(-) diff --git a/commodities/models/dc.py b/commodities/models/dc.py index a9caf5837..7bbce5cfd 100644 --- a/commodities/models/dc.py +++ b/commodities/models/dc.py @@ -852,10 +852,10 @@ def _get_snapshot_commodities( that match the latest_version goods. """ item_ids = {c.item_id for c in self.commodities if c.obj} - goods = GoodsNomenclature.objects.current().filter( + goods = GoodsNomenclature.objects.filter( item_id__in=item_ids, valid_between__contains=snapshot_date, - ) + ).current() latest_versions = get_latest_versions(goods) pks = {good.pk for good in latest_versions} diff --git a/common/models/tracked_qs.py b/common/models/tracked_qs.py index e5082939b..0eb02b378 100644 --- a/common/models/tracked_qs.py +++ b/common/models/tracked_qs.py @@ -47,7 +47,7 @@ def latest_approved(self) -> TrackedModelQuerySet: update_type=UpdateType.DELETE, ) - def current(self) -> TrackedModelQuerySet: + def current(self, transaction=None) -> TrackedModelQuerySet: """ Returns a queryset of approved versions of the model up to the globally defined current transaction (see ``common.models.utils`` for details of @@ -64,9 +64,12 @@ def current(self) -> TrackedModelQuerySet: (see ``set_current_transaction()`` and ``override_current_transaction()`` in ``common.models.utils``). """ - return self.approved_up_to_transaction( - LazyTransaction(get_value=get_current_transaction), - ) + if transaction: + return self.approved_up_to_transaction(transaction) + else: + return self.approved_up_to_transaction( + LazyTransaction(get_value=get_current_transaction), + ) def approved_up_to_transaction(self, transaction=None) -> TrackedModelQuerySet: """This function is called using the current() function instead of directly calling it on model queries. diff --git a/common/querysets.py b/common/querysets.py index 4939af314..a96a0c8d1 100644 --- a/common/querysets.py +++ b/common/querysets.py @@ -66,7 +66,7 @@ def not_current(self, asof_transaction=None) -> QuerySet: :param transaction Transaction: The transaction to limit versions to. :rtype QuerySet: """ - current = self.current() + current = self.current(transaction=asof_transaction) return self.difference(current) diff --git a/footnotes/views.py b/footnotes/views.py index 5aa940b31..d871bc283 100644 --- a/footnotes/views.py +++ b/footnotes/views.py @@ -44,7 +44,6 @@ class FootnoteViewSet(viewsets.ReadOnlyModelViewSet): ] def get_queryset(self): - tx = WorkBasket.get_current_transaction(self.request) return ( models.Footnote.objects.current() .select_related("footnote_type") @@ -67,7 +66,6 @@ class FootnoteMixin: model: Type[TrackedModel] = models.Footnote def get_queryset(self): - tx = WorkBasket.get_current_transaction(self.request) return models.Footnote.objects.current().select_related( "footnote_type", ) @@ -77,7 +75,6 @@ class FootnoteDescriptionMixin: model: Type[TrackedModel] = models.FootnoteDescription def get_queryset(self): - tx = WorkBasket.get_current_transaction(self.request) return models.FootnoteDescription.objects.current() diff --git a/geo_areas/forms.py b/geo_areas/forms.py index 9a638adba..2827c1d47 100644 --- a/geo_areas/forms.py +++ b/geo_areas/forms.py @@ -328,7 +328,6 @@ def clean(self): ) if membership and action == GeoMembershipAction.DELETE: - tx = WorkBasket.get_current_transaction(self.request) if membership.member_used_in_measure_exclusion(): self.add_error( "membership", diff --git a/geo_areas/tests/test_views.py b/geo_areas/tests/test_views.py index f4ca956de..77308b047 100644 --- a/geo_areas/tests/test_views.py +++ b/geo_areas/tests/test_views.py @@ -444,7 +444,7 @@ def test_geo_area_edit_create_view( area_code=AreaCode.REGION, area_id="TR", valid_between=date_ranges.no_end, - transaction=session_workbasket.new_transaction(), + transaction__workbasket=session_workbasket, ) data_changes = {**date_post_data("end_date", date_ranges.normal.upper)} diff --git a/geo_areas/views.py b/geo_areas/views.py index b80380c3f..468388560 100644 --- a/geo_areas/views.py +++ b/geo_areas/views.py @@ -56,7 +56,6 @@ class GeoAreaMixin: model: Type[TrackedModel] = GeographicalArea def get_queryset(self): - tx = WorkBasket.get_current_transaction(self.request) return GeographicalArea.objects.current() @@ -64,7 +63,6 @@ class GeoAreaDescriptionMixin: model: Type[TrackedModel] = GeographicalAreaDescription def get_queryset(self): - tx = WorkBasket.get_current_transaction(self.request) return GeographicalAreaDescription.objects.current() diff --git a/measures/business_rules.py b/measures/business_rules.py index 6b246b255..35302c78e 100644 --- a/measures/business_rules.py +++ b/measures/business_rules.py @@ -212,11 +212,11 @@ def validate(self, measure): goods = ( type(measure.goods_nomenclature) - .objects.current() - .filter( + .objects.filter( sid=measure.goods_nomenclature.sid, valid_between__overlap=measure.effective_valid_between, ) + .current() ) explosion_level = measure.measure_type.measure_explosion_level diff --git a/measures/forms.py b/measures/forms.py index 8d8f01f6c..6d9973a79 100644 --- a/measures/forms.py +++ b/measures/forms.py @@ -520,8 +520,6 @@ def __init__(self, *args, **kwargs): self.request = kwargs.pop("request", None) super().__init__(*args, **kwargs) - tx = WorkBasket.get_current_transaction(self.request) - self.initial["duty_sentence"] = self.instance.duty_sentence self.request.session[ f"instance_duty_sentence_{self.instance.sid}" diff --git a/measures/models.py b/measures/models.py index bd75fdad1..8619e806e 100644 --- a/measures/models.py +++ b/measures/models.py @@ -656,14 +656,14 @@ def auto_value_fields(cls): def has_components(self, transaction): return ( - MeasureComponent.objects.current() + MeasureComponent.objects.current(transaction=transaction) .filter(component_measure__sid=self.sid) .exists() ) def has_condition_components(self, transaction): return ( - MeasureConditionComponent.objects.current() + MeasureConditionComponent.objects.current(transaction=transaction) .filter(condition__dependent_measure__sid=self.sid) .exists() ) diff --git a/measures/querysets.py b/measures/querysets.py index 75a8ca3c6..4f2107d46 100644 --- a/measures/querysets.py +++ b/measures/querysets.py @@ -94,7 +94,9 @@ def duty_sentence( # Components with the greatest transaction_id that is less than # or equal to component_parent's transaction_id, are considered 'current'. - component_qs = component_parent.components.current() + component_qs = component_parent.components.current( + transaction=component_parent.transaction + ) if not component_qs: return "" latest_transaction_id = component_qs.aggregate( diff --git a/measures/tests/factories.py b/measures/tests/factories.py index b0bfd13bb..b0ee019f7 100644 --- a/measures/tests/factories.py +++ b/measures/tests/factories.py @@ -31,11 +31,19 @@ class Meta: measure_type_description = factory.SelfAttribute("measure.measure_type.description") duty_sentence = factory.sequence(lambda n: f"{n}.00%") origin_description = factory.LazyAttribute( - lambda m: m.measure.geographical_area.descriptions.current().last().description, + lambda m: m.measure.geographical_area.descriptions.current( + transaction=m.measure.geographical_area.transaction + ) + .last() + .description, ) excluded_origin_descriptions = factory.LazyAttribute( lambda m: random.choice(MeasureSheetRow.separators).join( - e.excluded_geographical_area.descriptions.current().last().description + e.excluded_geographical_area.descriptions.current( + transaction=e.excluded_geographical_area + ) + .last() + .description for e in m.measure.exclusions.all() ), ) diff --git a/measures/tests/test_views.py b/measures/tests/test_views.py index 4e89d11c5..19421ba61 100644 --- a/measures/tests/test_views.py +++ b/measures/tests/test_views.py @@ -539,7 +539,6 @@ def test_measure_update_duty_sentence( assert response.status_code == 302 if update_data: - tx = Transaction.objects.last() measure = Measure.objects.current().get( sid=measure_form.instance.sid, ) @@ -700,7 +699,6 @@ def test_measure_update_create_conditions( assert response.status_code == 302 assert response.url == reverse("measure-ui-confirm-update", args=(measure.sid,)) - tx = Transaction.objects.last() updated_measure = Measure.objects.current().get( sid=measure.sid, ) @@ -756,7 +754,6 @@ def test_measure_update_edit_conditions( client.force_login(valid_user) client.post(url, data=measure_edit_conditions_data) transaction_count = Transaction.objects.count() - tx = Transaction.objects.last() measure_with_condition = Measure.objects.current().get( sid=measure.sid, ) @@ -771,7 +768,6 @@ def test_measure_update_edit_conditions( f"{MEASURE_CONDITIONS_FORMSET_PREFIX}-0-applicable_duty" ] = "10 GBP / 100 kg" client.post(url, data=measure_edit_conditions_data) - tx = Transaction.objects.last() updated_measure = Measure.objects.current().get( sid=measure.sid, ) @@ -877,7 +873,6 @@ def test_measure_update_remove_conditions( # We expect one transaction for the measure update and condition deletion assert Transaction.objects.count() == transaction_count + 1 - tx = Transaction.objects.last() updated_measure = Measure.objects.current().get( sid=measure.sid, ) diff --git a/measures/views.py b/measures/views.py index 4d0793802..6cf473197 100644 --- a/measures/views.py +++ b/measures/views.py @@ -75,7 +75,6 @@ class MeasureTypeViewSet(viewsets.ReadOnlyModelViewSet): filter_backends = [MeasureTypeFilterBackend] def get_queryset(self): - tx = WorkBasket.get_current_transaction(self.request) return MeasureType.objects.current().order_by( "description", ) @@ -85,7 +84,6 @@ class MeasureMixin: model: Type[TrackedModel] = Measure def get_queryset(self): - tx = WorkBasket.get_current_transaction(self.request) return Measure.objects.current() @@ -1061,7 +1059,6 @@ def get_form_kwargs(self, step): def get_form(self, step=None, data=None, files=None): form = super().get_form(step, data, files) - tx = WorkBasket.get_current_transaction(self.request) forms = [form] if hasattr(form, "forms"): forms = form.forms @@ -1103,7 +1100,6 @@ def get_form_kwargs(self): def get_form(self, form_class=None): form = super().get_form(form_class=form_class) - tx = WorkBasket.get_current_transaction(self.request) if hasattr(form, "field"): for field in form.fields.values(): diff --git a/quotas/business_rules.py b/quotas/business_rules.py index cf6776086..6b1b9b293 100644 --- a/quotas/business_rules.py +++ b/quotas/business_rules.py @@ -51,7 +51,7 @@ class ON2(BusinessRule): def validate(self, order_number): if ( type(order_number) - .objects.current() + .objects.current(transaction=order_number.transaction) .filter( order_number=order_number.order_number, valid_between__overlap=order_number.valid_between, @@ -70,7 +70,12 @@ def validate(self, order_number): order_number_versions = order_number.get_versions() origin_exists = False for order_number_version in order_number_versions: - if order_number_version.origins.current().count() > 0: + if ( + order_number_version.origins.current( + transaction=order_number.transaction + ).count() + > 0 + ): origin_exists = True break @@ -85,7 +90,7 @@ class ON5(BusinessRule): def validate(self, origin): if ( type(origin) - .objects.current() + .objects.current(transaction=origin.transaction) .filter( order_number__sid=origin.order_number.sid, geographical_area__sid=origin.geographical_area.sid, @@ -179,7 +184,9 @@ def validate(self, order_number_origin): check that there are no measures linked to the origin . """ - measures = measures_models.Measure.objects.current() + measures = measures_models.Measure.objects.current( + transaction=order_number_origin.transaction + ) if not measures.exists(): return @@ -317,7 +324,7 @@ class OverlappingQuotaDefinition(BusinessRule): def validate(self, quota_definition): potential_quota_definition_matches = ( type(quota_definition) - .objects.current() + .objects.current(transaction=quota_definition.transaction) .filter( order_number=quota_definition.order_number, valid_between__overlap=quota_definition.valid_between, @@ -347,7 +354,9 @@ def validate(self, quota_definition): if quota_definition.valid_between.lower < datetime.date.today(): return True - if quota_definition.sub_quota_associations.current().exists(): + if quota_definition.sub_quota_associations.current( + transaction=self.transaction + ).exists(): return True if quota_definition.volume != quota_definition.initial_volume: @@ -458,7 +467,9 @@ class QA6(BusinessRule): def validate(self, association): if ( - association.main_quota.sub_quota_associations.current() + association.main_quota.sub_quota_associations.current( + transaction=association.transaction + ) .values( "sub_quota_relation_type", ) From 7714a0b2fd2555f943990c960101b01b08aac21f Mon Sep 17 00:00:00 2001 From: Edie Pearce Date: Tue, 14 Nov 2023 13:53:02 +0000 Subject: [PATCH 5/9] TP2000-1083: Quota order number create (#1081) * Add quota order number create view * Add validation * Update Quota factory to pass validation * Edit confirm page and link to quota origin form * Update validation, add tests * Use correct url format * Add create quota link to workbasket page, refactor * Fix test runner error * Add migrations to alter regex validator * Address PR comments * Amend business rules --- common/jinja2/layouts/confirm.jinja | 4 +- common/tests/factories.py | 3 +- .../0015_alter_measure_dead_order_number.py | 25 +++ quotas/forms.py | 129 ++++++++++++- .../quota-definitions/confirm-create.jinja | 2 +- quotas/jinja2/quotas/confirm-create.jinja | 41 +++++ ...008_alter_quotaordernumber_order_number.py | 23 +++ quotas/tests/test_views.py | 128 ++++++++++++- quotas/urls.py | 12 +- quotas/validators.py | 2 +- quotas/views.py | 23 +++ .../jinja2/workbaskets/edit-workbasket.jinja | 170 ++++++------------ 12 files changed, 431 insertions(+), 131 deletions(-) create mode 100644 measures/migrations/0015_alter_measure_dead_order_number.py create mode 100644 quotas/jinja2/quotas/confirm-create.jinja create mode 100644 quotas/migrations/0008_alter_quotaordernumber_order_number.py diff --git a/common/jinja2/layouts/confirm.jinja b/common/jinja2/layouts/confirm.jinja index 810784bd7..17a71cbf8 100644 --- a/common/jinja2/layouts/confirm.jinja +++ b/common/jinja2/layouts/confirm.jinja @@ -17,7 +17,9 @@
    {% block panel %}{% endblock%}

    Next steps

    -

    To complete your task you must publish your change.

    + {% block next_steps %} +

    To complete your task you must publish your change.

    + {% endblock%}
    {% block main_button %}{% endblock%}
    {{ govukButton({ "text": "Go to your workbasket summary", diff --git a/common/tests/factories.py b/common/tests/factories.py index 696275669..a2fe14464 100644 --- a/common/tests/factories.py +++ b/common/tests/factories.py @@ -6,6 +6,7 @@ import factory from factory.fuzzy import FuzzyChoice +from factory.fuzzy import FuzzyText from faker import Faker from common.models import TrackedModel @@ -745,7 +746,7 @@ class Meta: model = "quotas.QuotaOrderNumber" sid = numeric_sid() - order_number = string_sequence(6, characters=string.digits) + order_number = FuzzyText(length=4, chars=string.digits, prefix="05") mechanism = 0 category = 1 valid_between = date_ranges("big_no_end") diff --git a/measures/migrations/0015_alter_measure_dead_order_number.py b/measures/migrations/0015_alter_measure_dead_order_number.py new file mode 100644 index 000000000..13062a45c --- /dev/null +++ b/measures/migrations/0015_alter_measure_dead_order_number.py @@ -0,0 +1,25 @@ +# Generated by Django 3.2.20 on 2023-10-31 15:16 + +import django.core.validators +from django.db import migrations +from django.db import models + + +class Migration(migrations.Migration): + dependencies = [ + ("measures", "0014_action_pair_data_migration"), + ] + + operations = [ + migrations.AlterField( + model_name="measure", + name="dead_order_number", + field=models.CharField( + blank=True, + db_index=True, + max_length=6, + null=True, + validators=[django.core.validators.RegexValidator("^05[0-9]{4}$")], + ), + ), + ] diff --git a/quotas/forms.py b/quotas/forms.py index 8ac5eba37..1e0b3edc4 100644 --- a/quotas/forms.py +++ b/quotas/forms.py @@ -9,6 +9,7 @@ from crispy_forms_gds.layout import Size from crispy_forms_gds.layout import Submit from django import forms +from django.core.exceptions import ValidationError from django.template.loader import render_to_string from django.urls import reverse_lazy @@ -26,6 +27,13 @@ from quotas import validators from quotas.constants import QUOTA_ORIGIN_EXCLUSIONS_FORMSET_PREFIX +CATEGORY_HELP_TEXT = "Categories are required for the TAP database but will not appear as a TARIC3 object in your workbasket" +SAFEGUARD_HELP_TEXT = ( + "Once the quota category has been set as ‘Safeguard’, this cannot be changed" +) +START_DATE_HELP_TEXT = "If possible, avoid putting a start date in the past as this may cause issues with CDS downstream" +ORDER_NUMBER_HELP_TEXT = "The order number must begin with 05 and be 6 digits long. Licensed quotas must begin 054 and safeguards must begin 058" + class QuotaFilterForm(forms.Form): def __init__(self, *args, **kwargs): @@ -115,12 +123,6 @@ class QuotaUpdateForm( ValidityPeriodForm, forms.ModelForm, ): - CATEGORY_HELP_TEXT = "Categories are required for the TAP database but will not appear as a TARIC3 object in your workbasket" - SAFEGUARD_HELP_TEXT = ( - "Once the quota category has been set as ‘Safeguard’, this cannot be changed" - ) - START_DATE_HELP_TEXT = "If possible, avoid putting a start date in the past as this may cause issues with CDS downstream" - class Meta: model = models.QuotaOrderNumber fields = [ @@ -149,12 +151,12 @@ def init_fields(self): attrs={"disabled": True}, choices=validators.QuotaCategory.choices, ) - self.fields["category"].help_text = self.SAFEGUARD_HELP_TEXT + self.fields["category"].help_text = SAFEGUARD_HELP_TEXT else: self.fields["category"].choices = validators.QuotaCategoryEditing.choices - self.fields["category"].help_text = self.CATEGORY_HELP_TEXT + self.fields["category"].help_text = CATEGORY_HELP_TEXT - self.fields["start_date"].help_text = self.START_DATE_HELP_TEXT + self.fields["start_date"].help_text = START_DATE_HELP_TEXT def init_layout(self): self.helper = FormHelper(self) @@ -199,6 +201,115 @@ def init_layout(self): ) +class QuotaOrderNumberCreateForm( + ValidityPeriodForm, + forms.ModelForm, +): + class Meta: + model = models.QuotaOrderNumber + fields = [ + "order_number", + "valid_between", + "category", + "mechanism", + ] + + order_number = forms.CharField( + help_text=ORDER_NUMBER_HELP_TEXT, + validators=[validators.quota_order_number_validator], + error_messages={ + "invalid": "Order number must be six digits long and begin with 05", + "required": "Enter the order number", + }, + ) + category = forms.ChoiceField( + choices=validators.QuotaCategory.choices, + help_text=CATEGORY_HELP_TEXT, + error_messages={ + "invalid_choice": "Please select a valid category", + "required": "Choose the category", + }, + ) + mechanism = forms.ChoiceField( + choices=validators.AdministrationMechanism.choices, + error_messages={ + "invalid_choice": "Please select a valid mechanism", + "required": "Choose the mechanism", + }, + ) + + def __init__(self, *args, **kwargs): + super().__init__(*args, **kwargs) + self.init_fields() + self.init_layout() + + def init_fields(self): + self.fields["start_date"].help_text = START_DATE_HELP_TEXT + + def init_layout(self): + self.helper = FormHelper(self) + self.helper.label_size = Size.SMALL + self.helper.legend_size = Size.SMALL + + self.helper.layout = Layout( + Accordion( + AccordionSection( + "Order number", + "order_number", + ), + AccordionSection( + "Validity", + "start_date", + "end_date", + ), + AccordionSection( + "Category and mechanism", + "category", + "mechanism", + ), + css_class="govuk-width-!-two-thirds", + ), + Submit( + "submit", + "Save", + data_module="govuk-button", + data_prevent_double_click="true", + ), + ) + + def clean(self): + category = self.cleaned_data.get("category") + mechanism = self.cleaned_data.get("mechanism") + order_number = self.cleaned_data.get("order_number", "") + + if ( + mechanism is not None + and int(mechanism) == validators.AdministrationMechanism.LICENSED + ): + if int(category) == validators.QuotaCategory.SAFEGUARD: + raise ValidationError( + "Mechanism cannot be set to licensed for safeguard quotas", + ) + if not order_number.startswith("054"): + raise ValidationError( + "The order number for licensed quotas must begin with 054", + ) + + if ( + category is not None + and int( + category, + ) + == validators.QuotaCategory.SAFEGUARD + and not order_number.startswith("058") + ): + raise ValidationError( + "The order number for safeguard quotas must begin with 058", + ) + + return super().clean() + + class QuotaOrderNumberOriginForm( FormSetSubmitMixin, ValidityPeriodForm, diff --git a/quotas/jinja2/quota-definitions/confirm-create.jinja b/quotas/jinja2/quota-definitions/confirm-create.jinja index 5a5480c6f..2f30a528e 100644 --- a/quotas/jinja2/quota-definitions/confirm-create.jinja +++ b/quotas/jinja2/quota-definitions/confirm-create.jinja @@ -15,7 +15,7 @@ {% block main_button %} {{ govukButton({ "text": "Create another", - "href": url("quota_definition-ui-create", args=[object.order_number.sid]), + "href": object.get_url('create'), "classes": "govuk-button--primary" }) }} {% endblock%} diff --git a/quotas/jinja2/quotas/confirm-create.jinja b/quotas/jinja2/quotas/confirm-create.jinja new file mode 100644 index 000000000..77f6678b9 --- /dev/null +++ b/quotas/jinja2/quotas/confirm-create.jinja @@ -0,0 +1,41 @@ +{% extends "common/confirm_create.jinja" %} +{% from "components/breadcrumbs.jinja" import breadcrumbs %} +{% from "components/button/macro.njk" import govukButton %} +{% from "components/warning-text/macro.njk" import govukWarningText %} + + +{% block breadcrumb %} + {{ breadcrumbs(request, [ + {"text": "Find and edit quotas", "href": url("quota-ui-list")}, + {"text": page_title} + ]) + }} +{% endblock %} + +{% block next_steps %} + {{ govukWarningText({ + "text": "Before you can associate this quota order number with a measure you will need to create a quota origin." + }) }} +{% endblock%} + +{% block main_button %} +
    +{{ govukButton({ + "text": "Create a quota origin", + "href": url('quota_order_number_origin-ui-create', args=[object.sid]), + "classes": "govuk-button--primary" +}) }} +
    +
    +{{ govukButton({ + "text": "Create another quota order number", + "href": object.get_url('create'), + "classes": "govuk-button--secondary" +}) }} +
    +{% endblock%} + +{% block actions %} +
  • View this quota order number
    • +
  • Find and edit quotas
    • +{% endblock %} \ No newline at end of file diff --git a/quotas/migrations/0008_alter_quotaordernumber_order_number.py b/quotas/migrations/0008_alter_quotaordernumber_order_number.py new file mode 100644 index 000000000..54640d891 --- /dev/null +++ b/quotas/migrations/0008_alter_quotaordernumber_order_number.py @@ -0,0 +1,23 @@ +# Generated by Django 3.2.20 on 2023-10-31 15:16 + +import django.core.validators +from django.db import migrations +from django.db import models + + +class Migration(migrations.Migration): + dependencies = [ + ("quotas", "0007_auto_20221130_1427"), + ] + + operations = [ + migrations.AlterField( + model_name="quotaordernumber", + name="order_number", + field=models.CharField( + db_index=True, + max_length=6, + validators=[django.core.validators.RegexValidator("^05[0-9]{4}$")], + ), + ), + ] diff --git a/quotas/tests/test_views.py b/quotas/tests/test_views.py index e6efe2a3f..5eb69db8d 100644 --- a/quotas/tests/test_views.py +++ b/quotas/tests/test_views.py @@ -1068,7 +1068,11 @@ def test_quota_create_origin_no_overlapping_origins( @pytest.mark.django_db def test_quota_order_number_and_origin_edit_create_view( - valid_user_client, date_ranges, approved_transaction, geo_group1, geo_group2 + valid_user_client, + date_ranges, + approved_transaction, + geo_group1, + geo_group2, ): quota = factories.QuotaOrderNumberFactory.create( valid_between=date_ranges.no_end, @@ -1103,7 +1107,11 @@ def test_quota_order_number_and_origin_edit_create_view( @pytest.mark.django_db def test_quota_order_number_update_view( - valid_user_client, date_ranges, approved_transaction, geo_group1, geo_group2 + valid_user_client, + date_ranges, + approved_transaction, + geo_group1, + geo_group2, ): quota = factories.QuotaOrderNumberFactory.create( valid_between=date_ranges.no_end, @@ -1216,6 +1224,7 @@ def test_create_new_quota_definition_business_rule_violation( url = reverse("quota_definition-ui-create", kwargs={"sid": quota.sid}) response = valid_user_client.post(url, form_data) + assert response.status_code == 200 soup = BeautifulSoup(response.content.decode(response.charset), "html.parser") @@ -1226,3 +1235,118 @@ def test_create_new_quota_definition_business_rule_violation( "The validity period of the quota definition must be spanned by one of the validity periods of the referenced quota order number." in errors ) + + +@pytest.mark.django_db +def test_quota_order_number_create_200( + valid_user_client, +): + response = valid_user_client.get(reverse("quota-ui-create")) + + assert response.status_code == 200 + + +@pytest.mark.django_db +def test_quota_order_number_create_errors_required( + valid_user_client, +): + form_data = { + "submit": "Save", + } + response = valid_user_client.post(reverse("quota-ui-create"), form_data) + + assert response.status_code == 200 + + soup = BeautifulSoup(response.content.decode(response.charset), "html.parser") + + errors = {e.text for e in soup.select(".govuk-error-summary__list li a")} + + assert { + "Enter the order number", + "Choose the category", + "Choose the mechanism", + "Enter the day, month and year", + } == errors + + +@pytest.mark.django_db +@pytest.mark.parametrize( + "order_number,category,mechanism,exp_error", + [ + ( + "050000", + validators.QuotaCategory.SAFEGUARD.value, + validators.AdministrationMechanism.LICENSED.value, + "Mechanism cannot be set to licensed for safeguard quotas", + ), + ( + "050000", + validators.QuotaCategory.WTO.value, + validators.AdministrationMechanism.LICENSED.value, + "The order number for licensed quotas must begin with 054", + ), + ( + "050000", + validators.QuotaCategory.SAFEGUARD.value, + validators.AdministrationMechanism.FCFS.value, + "The order number for safeguard quotas must begin with 058", + ), + ], +) +def test_quota_order_number_create_validation( + order_number, + mechanism, + category, + exp_error, + valid_user_client, + date_ranges, +): + form_data = { + "start_date_0": date_ranges.normal.lower.day, + "start_date_1": date_ranges.normal.lower.month, + "start_date_2": date_ranges.normal.lower.year, + "order_number": order_number, + "mechanism": mechanism, + "category": category, + "submit": "Save", + } + response = valid_user_client.post(reverse("quota-ui-create"), form_data) + + assert response.status_code == 200 + + soup = BeautifulSoup(response.content.decode(response.charset), "html.parser") + + errors = {e.text for e in soup.select(".govuk-error-summary__list li a")} + + assert exp_error in errors + + +@pytest.mark.django_db +def test_quota_order_number_create_success( + valid_user_client, + date_ranges, +): + form_data = { + "start_date_0": date_ranges.normal.lower.day, + "start_date_1": date_ranges.normal.lower.month, + "start_date_2": date_ranges.normal.lower.year, + "order_number": "054000", + "mechanism": validators.AdministrationMechanism.LICENSED.value, + "category": validators.QuotaCategory.WTO.value, + "submit": "Save", + } + response = valid_user_client.post(reverse("quota-ui-create"), form_data) + + assert response.status_code == 302 + + quota = models.QuotaOrderNumber.objects.last() + + assert response.url == reverse("quota-ui-confirm-create", kwargs={"sid": quota.sid}) + + response2 = valid_user_client.get(response.url) + + soup = BeautifulSoup(response2.content.decode(response2.charset), "html.parser") + + assert ( + soup.find("h1").text.strip() == f"Quota {quota.order_number} has been created" + ) diff --git a/quotas/urls.py b/quotas/urls.py index 892b2b792..d5de3ca42 100644 --- a/quotas/urls.py +++ b/quotas/urls.py @@ -44,6 +44,16 @@ urlpatterns = [ path("quotas/", include(ui_patterns)), + path( + f"quotas/create/", + views.QuotaCreate.as_view(), + name="quota-ui-create", + ), + path( + f"quotas//confirm-create/", + views.QuotaConfirmCreate.as_view(), + name="quota-ui-confirm-create", + ), path( f"quotas//quota-definitions/", views.QuotaDefinitionList.as_view(), @@ -75,7 +85,7 @@ name="quota_order_number_origin-ui-edit-create", ), path( - f"quotas//quota_order_number_origins/", + f"quotas//quota_order_number_origins/create/", views.QuotaOrderNumberOriginCreate.as_view(), name="quota_order_number_origin-ui-create", ), diff --git a/quotas/validators.py b/quotas/validators.py index 6ce0c896b..2d18d777d 100644 --- a/quotas/validators.py +++ b/quotas/validators.py @@ -5,7 +5,7 @@ from django.core.validators import RegexValidator from django.db import models -quota_order_number_validator = RegexValidator(r"^[0-9]{6}$") +quota_order_number_validator = RegexValidator(r"^05[0-9]{4}$") monetary_unit_code_validator = RegexValidator(r"^[A-Z]{3}$") measurement_unit_code_validator = RegexValidator(r"^[A-Z]{3}$") measurement_unit_qualifier_code_validator = RegexValidator(r"^[A-Z]$") diff --git a/quotas/views.py b/quotas/views.py index b20210bb8..472cc78a6 100644 --- a/quotas/views.py +++ b/quotas/views.py @@ -105,6 +105,29 @@ def get_queryset(self): return models.QuotaOrderNumber.objects.approved_up_to_transaction(tx) +class QuotaCreate(QuotaOrderNumberMixin, CreateTaricCreateView): + form_class = forms.QuotaOrderNumberCreateForm + template_name = "layouts/create.jinja" + + permission_required = ["common.add_trackedmodel"] + + validate_business_rules = ( + business_rules.ON1, + business_rules.ON2, + UniqueIdentifyingFields, + UpdateValidity, + ) + + def get_context_data(self, **kwargs): + return super().get_context_data( + page_title="Create a new quota order number", **kwargs + ) + + +class QuotaConfirmCreate(QuotaOrderNumberMixin, TrackedModelDetailView): + template_name = "quotas/confirm-create.jinja" + + class QuotaList(QuotaOrderNumberMixin, TamatoListView): """Returns a list of QuotaOrderNumber objects.""" diff --git a/workbaskets/jinja2/workbaskets/edit-workbasket.jinja b/workbaskets/jinja2/workbaskets/edit-workbasket.jinja index 326b6eda3..f5c60ad16 100644 --- a/workbaskets/jinja2/workbaskets/edit-workbasket.jinja +++ b/workbaskets/jinja2/workbaskets/edit-workbasket.jinja @@ -27,123 +27,63 @@ {{ navigation(request, "edit") }} -
    -
    -

    Additional codes

    - -
    -
    -

    Certificates

    - -
    + {% macro workbasket_column(title, links) %}
    -

    Footnotes

    - -
    -
    -

    Geographical areas

    - -
    +

    {{ title }}

    + +
    + {% endmacro %} + +
    + {{ workbasket_column("Additional codes", [ + {"text": "Create a new additional code", "url": url('additional_code-ui-create')}, + {"text": "Find and edit additional codes", "url": url("additional_code-ui-list")}, + ]) + }} + {{ workbasket_column("Certificates", [ + {"text": "Create a new certificate", "url": url('certificate-ui-create')}, + {"text": "Find and edit certificates", "url": url("certificate-ui-list")}, + ]) + }} + {{ workbasket_column("Footnotes", [ + {"text": "Create a new footnote", "url": url('footnote-ui-create')}, + {"text": "Find and edit footnotes", "url": url('footnote-ui-list')}, + ]) + }} + {{ workbasket_column("Geographical areas", [ + {"text": "Create a new geographical area", "url": url('geo_area-ui-create')}, + {"text": "Find and edit geographical areas", "url": url('geo_area-ui-list')}, + ]) + }}
    -
    -

    Commodities

    - -
    -
    -

    Measures

    - -
    -
    -

    Quotas

    - -
    -
    -

    Regulations

    - -
    + {{ workbasket_column("Commodities", [ + {"text": "Import commodities", "url": url('commodity_importer-ui-create')}, + {"text": "Find commodities", "url": url('commodity-ui-list')}, + ]) + }} + {{ workbasket_column("Measures", [ + {"text": "Create new measure", "url": url('measure-ui-create', kwargs={"step": "start"})}, + {"text": "Find and edit measures", "url": url('measure-ui-search')}, + ]) + }} + {{ workbasket_column("Quotas", [ + {"text": "Create new quota", "url": url('quota-ui-create')}, + {"text": "Find and edit quotas", "url": url('quota-ui-list')}, + ]) + }} + {{ workbasket_column("Regulations", [ + {"text": "Create new regulation", "url": url('regulation-ui-create')}, + {"text": "Find and edit regulations", "url": url('regulation-ui-list')}, + ]) + }}
    {% endblock %} From 0842ed4c6285f807c3818d6373ebef1ddb795ba5 Mon Sep 17 00:00:00 2001 From: "dependabot[bot]" <49699333+dependabot[bot]@users.noreply.github.com> Date: Wed, 15 Nov 2023 09:58:15 +0000 Subject: [PATCH 6/9] Bump aiohttp from 3.8.5 to 3.8.6 (#1093) Bumps [aiohttp](https://github.com/aio-libs/aiohttp) from 3.8.5 to 3.8.6. - [Release notes](https://github.com/aio-libs/aiohttp/releases) - [Changelog](https://github.com/aio-libs/aiohttp/blob/master/CHANGES.rst) - [Commits](https://github.com/aio-libs/aiohttp/compare/v3.8.5...v3.8.6) --- updated-dependencies: - dependency-name: aiohttp dependency-type: direct:production ... Signed-off-by: dependabot[bot] Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> --- requirements.txt | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/requirements.txt b/requirements.txt index e19f13f49..bf9d00095 100644 --- a/requirements.txt +++ b/requirements.txt @@ -1,4 +1,4 @@ -aiohttp==3.8.5 +aiohttp==3.8.6 apsw==3.43.0.0 aioresponses==0.7.4 allure-pytest-bdd==2.8.40 From 1da728b86056f53c6a70047f7fa81ea0fd67dd82 Mon Sep 17 00:00:00 2001 From: nboyse Date: Wed, 15 Nov 2023 12:19:00 +0000 Subject: [PATCH 7/9] remove redundant pk --- common/tests/test_models.py | 6 +++--- conftest.py | 4 +++- 2 files changed, 6 insertions(+), 4 deletions(-) diff --git a/common/tests/test_models.py b/common/tests/test_models.py index 34745fbf3..cc439bb08 100644 --- a/common/tests/test_models.py +++ b/common/tests/test_models.py @@ -308,10 +308,10 @@ def test_current_as_of(sample_model): version_group=sample_model.version_group, ) - assert models.TestModel1.objects.latest_approved().get().pk == sample_model.pk + assert models.TestModel1.objects.latest_approved().get() == sample_model assert ( - models.TestModel1.objects.current().get().pk - == unapproved_version.pk + models.TestModel1.objects.current().get() + == unapproved_version ) diff --git a/conftest.py b/conftest.py index 315c588eb..8beb83b14 100644 --- a/conftest.py +++ b/conftest.py @@ -835,7 +835,9 @@ def check( assert workbasket is not None try: - imported = model_class.objects.current().get(**db_kwargs) + imported = model_class.objects.approved_up_to_transaction( + workbasket.current_transaction, + ).get(**db_kwargs) except model_class.DoesNotExist: if model.update_type == UpdateType.DELETE: imported = ( From a9bea1b353734bd389abb856f36bf42f71b03ab1 Mon Sep 17 00:00:00 2001 From: nboyse Date: Wed, 15 Nov 2023 12:40:17 +0000 Subject: [PATCH 8/9] remove unused transactions and revert some of the conversion to current --- commodities/business_rules.py | 18 +++++++++++++----- commodities/views.py | 1 - common/models/tracked_qs.py | 11 ++++------- common/querysets.py | 2 +- common/tests/test_models.py | 5 +---- docs/source/architecture/index.rst | 2 +- footnotes/forms.py | 4 ---- measures/forms.py | 1 - measures/models.py | 4 ++-- measures/querysets.py | 2 +- measures/tests/factories.py | 4 ++-- measures/views.py | 1 - quotas/business_rules.py | 16 +++++++++------- quotas/tests/test_views.py | 6 ------ quotas/views.py | 2 -- regulations/business_rules.py | 2 +- regulations/forms.py | 5 ++--- regulations/views.py | 3 --- 18 files changed, 37 insertions(+), 52 deletions(-) diff --git a/commodities/business_rules.py b/commodities/business_rules.py index c1cc5e87e..ec64c9f7e 100644 --- a/commodities/business_rules.py +++ b/commodities/business_rules.py @@ -43,8 +43,12 @@ def __init__(self, transaction=None): self.logger = logging.getLogger(type(self).__name__) def parent_spans_child(self, parent, child) -> bool: - parent_validity = parent.indented_goods_nomenclature.version_at().valid_between - child_validity = child.indented_goods_nomenclature.version_at().valid_between + parent_validity = parent.indented_goods_nomenclature.version_at( + self.transaction + ).valid_between + child_validity = child.indented_goods_nomenclature.version_at( + self.transaction + ).valid_between return validity_range_contains_range(parent_validity, child_validity) def parents_span_childs_future(self, parents, child): @@ -55,13 +59,17 @@ def parents_span_childs_future(self, parents, child): parents_validity = [] for parent in parents: parents_validity.append( - parent.indented_goods_nomenclature.version_at().valid_between, + parent.indented_goods_nomenclature.version_at( + self.transaction + ).valid_between, ) # sort by start date so any gaps will be obvious parents_validity.sort(key=lambda daterange: daterange.lower) - child_validity = child.indented_goods_nomenclature.version_at().valid_between + child_validity = child.indented_goods_nomenclature.version_at( + self.transaction + ).valid_between if ( not child_validity.upper_inf @@ -100,7 +108,7 @@ def validate(self, indent): from commodities.models.dc import get_chapter_collection try: - good = indent.indented_goods_nomenclature.version_at() + good = indent.indented_goods_nomenclature.version_at(self.transaction) except TrackedModel.DoesNotExist: self.logger.warning( "Goods nomenclature %s no longer exists at transaction %s " diff --git a/commodities/views.py b/commodities/views.py index 2eff8aaa5..a1cefb8a9 100644 --- a/commodities/views.py +++ b/commodities/views.py @@ -47,7 +47,6 @@ def get_queryset(self): Only return valid names that are products (suffix=80) """ - tx = WorkBasket.get_current_transaction(self.request) return ( GoodsNomenclature.objects.current() .prefetch_related("descriptions") diff --git a/common/models/tracked_qs.py b/common/models/tracked_qs.py index 0eb02b378..e5082939b 100644 --- a/common/models/tracked_qs.py +++ b/common/models/tracked_qs.py @@ -47,7 +47,7 @@ def latest_approved(self) -> TrackedModelQuerySet: update_type=UpdateType.DELETE, ) - def current(self, transaction=None) -> TrackedModelQuerySet: + def current(self) -> TrackedModelQuerySet: """ Returns a queryset of approved versions of the model up to the globally defined current transaction (see ``common.models.utils`` for details of @@ -64,12 +64,9 @@ def current(self, transaction=None) -> TrackedModelQuerySet: (see ``set_current_transaction()`` and ``override_current_transaction()`` in ``common.models.utils``). """ - if transaction: - return self.approved_up_to_transaction(transaction) - else: - return self.approved_up_to_transaction( - LazyTransaction(get_value=get_current_transaction), - ) + return self.approved_up_to_transaction( + LazyTransaction(get_value=get_current_transaction), + ) def approved_up_to_transaction(self, transaction=None) -> TrackedModelQuerySet: """This function is called using the current() function instead of directly calling it on model queries. diff --git a/common/querysets.py b/common/querysets.py index a96a0c8d1..77dedac89 100644 --- a/common/querysets.py +++ b/common/querysets.py @@ -66,7 +66,7 @@ def not_current(self, asof_transaction=None) -> QuerySet: :param transaction Transaction: The transaction to limit versions to. :rtype QuerySet: """ - current = self.current(transaction=asof_transaction) + current = self.approved_up_to_transaction(asof_transaction) return self.difference(current) diff --git a/common/tests/test_models.py b/common/tests/test_models.py index cc439bb08..aa28b6f1d 100644 --- a/common/tests/test_models.py +++ b/common/tests/test_models.py @@ -309,10 +309,7 @@ def test_current_as_of(sample_model): ) assert models.TestModel1.objects.latest_approved().get() == sample_model - assert ( - models.TestModel1.objects.current().get() - == unapproved_version - ) + assert models.TestModel1.objects.current().get() == unapproved_version def test_create_with_description(): diff --git a/docs/source/architecture/index.rst b/docs/source/architecture/index.rst index bdabea63d..d181bbaa3 100644 --- a/docs/source/architecture/index.rst +++ b/docs/source/architecture/index.rst @@ -106,7 +106,7 @@ that is not draft is considered to be "current". There are a number of convenience methods for finding "current" models. .. autoclass:: common.models.trackedmodel.TrackedModelQuerySet - :members: latest_approved, current + :members: latest_approved, current, approved_up_to_transaction Workbaskets diff --git a/footnotes/forms.py b/footnotes/forms.py index c67b79b41..a97507155 100644 --- a/footnotes/forms.py +++ b/footnotes/forms.py @@ -120,10 +120,6 @@ def next_sid(self, instance): form's save() method (with its commit param set either to True or False). """ - workbasket = WorkBasket.current(self.request) - tx = None - if workbasket: - tx = workbasket.transactions.order_by("order").last() return get_next_id( models.Footnote.objects.filter( diff --git a/measures/forms.py b/measures/forms.py index 6d9973a79..cc10f11e2 100644 --- a/measures/forms.py +++ b/measures/forms.py @@ -537,7 +537,6 @@ def __init__(self, *args, **kwargs): # If no footnote keys are stored in the session for a measure, # store all the pks of a measure's footnotes on the session, using the measure sid as key if f"instance_footnotes_{self.instance.sid}" not in self.request.session.keys(): - tx = WorkBasket.get_current_transaction(self.request) associations = models.FootnoteAssociationMeasure.objects.current().filter( footnoted_measure__sid=self.instance.sid, ) diff --git a/measures/models.py b/measures/models.py index 8619e806e..de9eaa92e 100644 --- a/measures/models.py +++ b/measures/models.py @@ -656,14 +656,14 @@ def auto_value_fields(cls): def has_components(self, transaction): return ( - MeasureComponent.objects.current(transaction=transaction) + MeasureComponent.objects.approved_up_to_transaction(transaction) .filter(component_measure__sid=self.sid) .exists() ) def has_condition_components(self, transaction): return ( - MeasureConditionComponent.objects.current(transaction=transaction) + MeasureConditionComponent.objects.approved_up_to_transaction(transaction) .filter(condition__dependent_measure__sid=self.sid) .exists() ) diff --git a/measures/querysets.py b/measures/querysets.py index 4f2107d46..552c4327d 100644 --- a/measures/querysets.py +++ b/measures/querysets.py @@ -94,7 +94,7 @@ def duty_sentence( # Components with the greatest transaction_id that is less than # or equal to component_parent's transaction_id, are considered 'current'. - component_qs = component_parent.components.current( + component_qs = component_parent.components.approved_up_to_transaction( transaction=component_parent.transaction ) if not component_qs: diff --git a/measures/tests/factories.py b/measures/tests/factories.py index b0ee019f7..061fdd49c 100644 --- a/measures/tests/factories.py +++ b/measures/tests/factories.py @@ -31,7 +31,7 @@ class Meta: measure_type_description = factory.SelfAttribute("measure.measure_type.description") duty_sentence = factory.sequence(lambda n: f"{n}.00%") origin_description = factory.LazyAttribute( - lambda m: m.measure.geographical_area.descriptions.current( + lambda m: m.measure.geographical_area.descriptions.approved_up_to_transaction( transaction=m.measure.geographical_area.transaction ) .last() @@ -39,7 +39,7 @@ class Meta: ) excluded_origin_descriptions = factory.LazyAttribute( lambda m: random.choice(MeasureSheetRow.separators).join( - e.excluded_geographical_area.descriptions.current( + e.excluded_geographical_area.descriptions.approved_up_to_transaction( transaction=e.excluded_geographical_area ) .last() diff --git a/measures/views.py b/measures/views.py index 6cf473197..7bf44eeb9 100644 --- a/measures/views.py +++ b/measures/views.py @@ -84,7 +84,6 @@ class MeasureMixin: model: Type[TrackedModel] = Measure def get_queryset(self): - return Measure.objects.current() diff --git a/quotas/business_rules.py b/quotas/business_rules.py index 6b1b9b293..460d0f3ae 100644 --- a/quotas/business_rules.py +++ b/quotas/business_rules.py @@ -51,7 +51,7 @@ class ON2(BusinessRule): def validate(self, order_number): if ( type(order_number) - .objects.current(transaction=order_number.transaction) + .objects.approved_up_to_transaction(transaction=order_number.transaction) .filter( order_number=order_number.order_number, valid_between__overlap=order_number.valid_between, @@ -71,7 +71,7 @@ def validate(self, order_number): origin_exists = False for order_number_version in order_number_versions: if ( - order_number_version.origins.current( + order_number_version.origins.approved_up_to_transaction( transaction=order_number.transaction ).count() > 0 @@ -90,7 +90,7 @@ class ON5(BusinessRule): def validate(self, origin): if ( type(origin) - .objects.current(transaction=origin.transaction) + .objects.approved_up_to_transaction(transaction=origin.transaction) .filter( order_number__sid=origin.order_number.sid, geographical_area__sid=origin.geographical_area.sid, @@ -184,7 +184,7 @@ def validate(self, order_number_origin): check that there are no measures linked to the origin . """ - measures = measures_models.Measure.objects.current( + measures = measures_models.Measure.objects.approved_up_to_transaction( transaction=order_number_origin.transaction ) @@ -324,7 +324,9 @@ class OverlappingQuotaDefinition(BusinessRule): def validate(self, quota_definition): potential_quota_definition_matches = ( type(quota_definition) - .objects.current(transaction=quota_definition.transaction) + .objects.approved_up_to_transaction( + transaction=quota_definition.transaction + ) .filter( order_number=quota_definition.order_number, valid_between__overlap=quota_definition.valid_between, @@ -354,7 +356,7 @@ def validate(self, quota_definition): if quota_definition.valid_between.lower < datetime.date.today(): return True - if quota_definition.sub_quota_associations.current( + if quota_definition.sub_quota_associations.approved_up_to_transaction( transaction=self.transaction ).exists(): return True @@ -467,7 +469,7 @@ class QA6(BusinessRule): def validate(self, association): if ( - association.main_quota.sub_quota_associations.current( + association.main_quota.sub_quota_associations.approved_up_to_transaction( transaction=association.transaction ) .values( diff --git a/quotas/tests/test_views.py b/quotas/tests/test_views.py index 39e1a97cd..8985270cd 100644 --- a/quotas/tests/test_views.py +++ b/quotas/tests/test_views.py @@ -726,8 +726,6 @@ def test_quota_edit_origin_new_versions(valid_user_client): form_data, ) - tx = Transaction.objects.last() - quota = models.QuotaOrderNumber.objects.current().get( sid=quota.sid, ) @@ -775,8 +773,6 @@ def test_quota_edit_origin_exclusions( assert response.status_code == 302 - tx = Transaction.objects.last() - origin = models.QuotaOrderNumberOrigin.objects.current().get( sid=origin.sid, ) @@ -833,8 +829,6 @@ def test_quota_edit_origin_exclusions_remove( assert response.status_code == 302 - tx = Transaction.objects.last() - updated_quota = models.QuotaOrderNumber.objects.current().get( sid=quota.sid, ) diff --git a/quotas/views.py b/quotas/views.py index 83f867882..9d6db3e28 100644 --- a/quotas/views.py +++ b/quotas/views.py @@ -318,7 +318,6 @@ class QuotaOrderNumberOriginMixin: model = models.QuotaOrderNumberOrigin def get_queryset(self): - tx = WorkBasket.get_current_transaction(self.request) return models.QuotaOrderNumberOrigin.objects.current() @@ -458,7 +457,6 @@ class QuotaDefinitionMixin: model = models.QuotaDefinition def get_queryset(self): - tx = WorkBasket.get_current_transaction(self.request) return models.QuotaDefinition.objects.current() diff --git a/regulations/business_rules.py b/regulations/business_rules.py index e55f79d05..9849f4377 100644 --- a/regulations/business_rules.py +++ b/regulations/business_rules.py @@ -32,7 +32,7 @@ def validate(self, regulation): generating_regulation__regulation_id=regulation.regulation_id, generating_regulation__role_type=regulation.role_type, ) - .transaction() + .current() .exclude( valid_between__contained_by=regulation.valid_between, ) diff --git a/regulations/forms.py b/regulations/forms.py index e6b8ef614..95badca3d 100644 --- a/regulations/forms.py +++ b/regulations/forms.py @@ -176,13 +176,12 @@ def _get_next_part_value(self, partial_regulation_id): """Get the next available part value that can be appended to a partial regulation_id (see RegulationCreateForm._make_partial_regulation_id()).""" - tx = WorkBasket.get_current_transaction(self.request) last_matching_regulation = ( - Regulation.objects.current() - .filter( + Regulation.objects.filter( regulation_id__startswith=partial_regulation_id, role_type=FIXED_ROLE_TYPE, ) + .current() .order_by("-regulation_id") .first() ) diff --git a/regulations/views.py b/regulations/views.py index f29e536bd..5b929aa54 100644 --- a/regulations/views.py +++ b/regulations/views.py @@ -35,7 +35,6 @@ class RegulationViewSet(viewsets.ReadOnlyModelViewSet): filter_backends = [RegulationFilterBackend] def get_queryset(self): - tx = WorkBasket.get_current_transaction(self.request) return Regulation.objects.current().select_related( "regulation_group", ) @@ -54,7 +53,6 @@ class RegulationMixin: model: Type[TrackedModel] = Regulation def get_queryset(self): - tx = WorkBasket.get_current_transaction(self.request) return Regulation.objects.current().select_related( "regulation_group", ) @@ -179,7 +177,6 @@ class RegulationConfirmCreate(TrackedModelDetailView): model = Regulation def get_queryset(self): - tx = WorkBasket.get_current_transaction(self.request) return Regulation.objects.current() From 5ca30e0c078e11951807cc0b5252b6cf3e1c59dc Mon Sep 17 00:00:00 2001 From: nboyse Date: Wed, 15 Nov 2023 16:16:31 +0000 Subject: [PATCH 9/9] attempt to fix some tests --- commodities/models/dc.py | 11 +++++++---- common/business_rules.py | 12 ++++++------ common/models/trackedmodel.py | 12 ++++++------ measures/querysets.py | 2 +- measures/tests/factories.py | 2 +- workbaskets/views/mixins.py | 6 +++++- 6 files changed, 26 insertions(+), 19 deletions(-) diff --git a/commodities/models/dc.py b/commodities/models/dc.py index 7bbce5cfd..38ba18ce2 100644 --- a/commodities/models/dc.py +++ b/commodities/models/dc.py @@ -502,7 +502,7 @@ def get_dependent_measures( measure_qs = Measure.objects.filter(goods_sid_query) if self.moment.clock_type.is_transaction_clock: - measure_qs = measure_qs.current() + measure_qs = measure_qs.approved_up_to_transaction(self.moment.transaction) else: measure_qs = measure_qs.latest_approved() @@ -823,7 +823,7 @@ def get_snapshot( date=snapshot_date, ) - commodities = self._get_snapshot_commodities(snapshot_date) + commodities = self._get_snapshot_commodities(transaction, snapshot_date) return CommodityTreeSnapshot( moment=moment, @@ -832,6 +832,7 @@ def get_snapshot( def _get_snapshot_commodities( self, + transaction: Transaction, snapshot_date: date, ) -> List[Commodity]: """ @@ -852,10 +853,12 @@ def _get_snapshot_commodities( that match the latest_version goods. """ item_ids = {c.item_id for c in self.commodities if c.obj} - goods = GoodsNomenclature.objects.filter( + goods = GoodsNomenclature.objects.approved_up_to_transaction( + transaction, + ).filter( item_id__in=item_ids, valid_between__contains=snapshot_date, - ).current() + ) latest_versions = get_latest_versions(goods) pks = {good.pk for good in latest_versions} diff --git a/common/business_rules.py b/common/business_rules.py index 97c7414a1..79b7a6283 100644 --- a/common/business_rules.py +++ b/common/business_rules.py @@ -144,7 +144,7 @@ def get_linked_models( related_instances = [getattr(model, field.name)] for instance in related_instances: try: - yield instance.version_at() + yield instance.version_at(transaction) except TrackedModel.DoesNotExist: # `related_instances` will contain all instances, even # deleted ones, and `version_at` will return @@ -278,8 +278,8 @@ def validate(self, model): if ( type(model) - .objects.current() - .filter(**query) + .objects.filter(**query) + .approved_up_to_transaction(self.transaction) .exclude(version_group=model.version_group) .exists() ): @@ -305,8 +305,8 @@ def validate(self, model): query["valid_between__overlap"] = model.valid_between if ( - model.__class__.objects.current() - .filter(**query) + model.__class__.objects.filter(**query) + .approved_up_to_transaction(self.transaction) .exclude(version_group=model.version_group) .exists() ): @@ -573,7 +573,7 @@ def validate(self, exclusion): Membership = geo_group._meta.get_field("members").related_model if ( - not Membership.objects.current() + not Membership.objects.approved_up_to_transaction(self.transaction) .filter( geo_group__sid=geo_group.sid, member__sid=exclusion.excluded_geographical_area.sid, diff --git a/common/models/trackedmodel.py b/common/models/trackedmodel.py index f3cddd514..d2cc3947c 100644 --- a/common/models/trackedmodel.py +++ b/common/models/trackedmodel.py @@ -342,7 +342,7 @@ def current_version(self: Cls) -> Cls: raise self.__class__.DoesNotExist("Object has no current version") return current_version - def version_at(self: Cls) -> Cls: + def version_at(self: Cls, transaction) -> Cls: """ The latest version of this model that was approved as of the given transaction. @@ -350,7 +350,7 @@ def version_at(self: Cls) -> Cls: :param transaction Transaction: Limit versions to this transaction :rtype TrackedModel: """ - return self.get_versions().current().get() + return self.get_versions().approved_up_to_transaction(transaction).get() @classproperty def copyable_fields(cls): @@ -504,12 +504,12 @@ def copy( kwargs.update(nested_fields) if not ignore: - for model in queryset.current(): + for model in queryset.approved_up_to_transaction(transaction): model.copy(transaction, **kwargs) return new_object - def in_use_by(self, via_relation: str) -> QuerySet[TrackedModel]: + def in_use_by(self, via_relation: str, transaction=None) -> QuerySet[TrackedModel]: """ Returns all of the models that are referencing this one via the specified relation and exist as of the passed transaction. @@ -526,7 +526,7 @@ def in_use_by(self, via_relation: str) -> QuerySet[TrackedModel]: return remote_model.objects.filter( **{f"{remote_field_name}__version_group": self.version_group} - ).current() + ).approved_up_to_transaction(transaction) def in_use(self, transaction=None, *relations: str) -> bool: """ @@ -572,7 +572,7 @@ def in_use(self, transaction=None, *relations: str) -> bool: # If we find any objects for any relation, then the model is in use. for relation_name in using_models: - relation_queryset = self.in_use_by(relation_name) + relation_queryset = self.in_use_by(relation_name, transaction) if relation_queryset.exists(): return True diff --git a/measures/querysets.py b/measures/querysets.py index 552c4327d..62c23fc34 100644 --- a/measures/querysets.py +++ b/measures/querysets.py @@ -95,7 +95,7 @@ def duty_sentence( # Components with the greatest transaction_id that is less than # or equal to component_parent's transaction_id, are considered 'current'. component_qs = component_parent.components.approved_up_to_transaction( - transaction=component_parent.transaction + component_parent.transaction, ) if not component_qs: return "" diff --git a/measures/tests/factories.py b/measures/tests/factories.py index 061fdd49c..3f8a00539 100644 --- a/measures/tests/factories.py +++ b/measures/tests/factories.py @@ -40,7 +40,7 @@ class Meta: excluded_origin_descriptions = factory.LazyAttribute( lambda m: random.choice(MeasureSheetRow.separators).join( e.excluded_geographical_area.descriptions.approved_up_to_transaction( - transaction=e.excluded_geographical_area + transaction=e.excluded_geographical_area.transaction, ) .last() .description diff --git a/workbaskets/views/mixins.py b/workbaskets/views/mixins.py index 671392774..83a626352 100644 --- a/workbaskets/views/mixins.py +++ b/workbaskets/views/mixins.py @@ -10,5 +10,9 @@ def workbasket(self) -> WorkBasket: def get_queryset(self): qs = super().get_queryset() + transaction = None + current = self.workbasket + if current: + transaction = current.transactions.last() - return qs.current() + return qs.approved_up_to_transaction(transaction)