Skip to content
New issue

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

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

Already on GitHub? Sign in to your account

fix: use current() instead of approved_up_to_transaction() in codebase #1089

Open
wants to merge 11 commits into
base: master
Choose a base branch
from

Conversation

nboyse
Copy link
Collaborator

@nboyse nboyse commented Nov 13, 2023

TP-2000-396 - Use current() instead of approved_up_to_transaction() in codebase

Why

approved_up_to_transaction() is largely redundant and the functionality that it holds instead sits under the current function instead.

Instead of passing around transactions between views and forms before supplying approved_up_to_transaction, we should use the current queryset method instead. This ticket is to replace any instances of the former with the latter.

What

Essentially replacing (almost) every instance of approved_up_to_transaction() with current(). This also means in some cases, the extra arguments that are passed to functions are also being removed as part of this. Thorough regression testing will be carried out by myself and the product manager to ensure this does not break any features within TAP.

@nboyse nboyse force-pushed the fix/use-current-instead-of-approved_up_to_transaction branch from 894a04e to c56a89c Compare November 13, 2023 14:01
@paulpepper-trade
Copy link
Collaborator

I'm just about to push the button on a review, Tash. Sorry there're so many repeat comments, but I did stop before the end of my diff, so there may be more to follow - or not if some of my (repeated) comments are ill-founded!

models.TestModel1.objects.current().get().pk
== unapproved_version.pk
)
assert models.TestModel1.objects.current().get().pk == unapproved_version.pk
Copy link
Collaborator

@paulpepper-trade paulpepper-trade Nov 13, 2023

Choose a reason for hiding this comment

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

You don't actually need to reference pk when testing for model instance equality, Django defines a Model.__eq__() method in order to do the right thing when comparing instances. Not applicable here, but generally it can avoid problems if an instance is None.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yeah this is fair, will update alongside other changes, it wasn't my test initially :)

@@ -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()
Copy link
Collaborator

Choose a reason for hiding this comment

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

There's no longer an opportunity for the optional asof_transaction param to override current and so this may not be correct behaviour. May not because the use of the optional asof_transaction param seems to run counter to the name (and intent?) of this function.

footnotes/views.py Show resolved Hide resolved
footnotes/views.py Show resolved Hide resolved
footnotes/views.py Show resolved Hide resolved
@@ -92,7 +85,7 @@ class ON5(BusinessRule):
def validate(self, origin):
if (
type(origin)
.objects.approved_up_to_transaction(origin.transaction)
.objects.current()
Copy link
Collaborator

Choose a reason for hiding this comment

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

current equivalence issue?

measures = measures_models.Measure.objects.approved_up_to_transaction(
order_number_origin.transaction,
)
measures = measures_models.Measure.objects.current()
Copy link
Collaborator

Choose a reason for hiding this comment

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

current equivalence issue?

.objects.approved_up_to_transaction(
quota_definition.transaction,
)
.objects.current()
Copy link
Collaborator

Choose a reason for hiding this comment

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

current equivalence issue?

if quota_definition.sub_quota_associations.approved_up_to_transaction(
self.transaction,
).exists():
if quota_definition.sub_quota_associations.current().exists():
Copy link
Collaborator

Choose a reason for hiding this comment

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

current equivalence issue?

association.main_quota.sub_quota_associations.approved_up_to_transaction(
association.transaction,
)
association.main_quota.sub_quota_associations.current()
Copy link
Collaborator

Choose a reason for hiding this comment

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

current equivalence issue?

@@ -47,7 +47,7 @@ def latest_approved(self) -> TrackedModelQuerySet:
update_type=UpdateType.DELETE,
)

def current(self) -> TrackedModelQuerySet:
def current(self, transaction=None) -> TrackedModelQuerySet:
Copy link
Collaborator

@paulpepper-trade paulpepper-trade Nov 14, 2023

Choose a reason for hiding this comment

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

Introducing a transaction parameter changes the meaning / intent of this function. I think retaining and using approved_up_to_transaction() is a clearer way of filtering tracked models to up to a specific transaction that is not the current one.

Copy link
Collaborator

Choose a reason for hiding this comment

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

... TrackedModelQuerySet.current() is a convenience and short-cut to calling TrackedModelQuerySet.approved_up_to_transaction() for specific situations.

eadpearce and others added 7 commits November 14, 2023 13:53
* 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
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](aio-libs/aiohttp@v3.8.5...v3.8.6)

---
updated-dependencies:
- dependency-name: aiohttp
  dependency-type: direct:production
...

Signed-off-by: dependabot[bot] <[email protected]>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants