From 13446fedb99c9609b5972ebfbded4e128768b31e Mon Sep 17 00:00:00 2001 From: Subhash Bhushan Date: Tue, 18 Jun 2024 09:52:59 -0700 Subject: [PATCH] Ensure open-ended book holds do not expire Also rename date and datetime fields to reflect their content. Date fields end with `on` and DateTime fields end with `at`. --- README.md | 30 +++++++++- src/lending/checkout_service.py | 4 +- src/lending/daily_sheet_service.py | 4 +- src/lending/holding_service.py | 22 +++++-- src/lending/patron.py | 58 +++++++++---------- .../bdd/checkouts/step_defs/checkout_steps.py | 8 +-- .../open_and_close_ended_holds.feature | 1 + .../lending/bdd/holds/step_defs/hold_steps.py | 9 ++- tests/lending/conftest.py | 18 +++--- .../tdd/patron/test_checkout_entity.py | 2 +- 10 files changed, 101 insertions(+), 55 deletions(-) diff --git a/README.md b/README.md index 25e804a..70ddb50 100644 --- a/README.md +++ b/README.md @@ -7,4 +7,32 @@ Protean example implementation | Source | [DDD by Examples - Library](https://github.com/ddd-by-examples/library) | | Pattern | CQRS | | Protean Version | 0.12.0 | -| Coverage | [![codecov](https://codecov.io/github/proteanhq/library-cqrs/graph/badge.svg?token=onIFcl4Dg5)](https://codecov.io/github/proteanhq/library-cqrs)| \ No newline at end of file +| Coverage | [![codecov](https://codecov.io/github/proteanhq/library-cqrs/graph/badge.svg?token=onIFcl4Dg5)](https://codecov.io/github/proteanhq/library-cqrs)| + +## Contributing + +- Clone this repository: + +`git clone git@github.com:proteanhq/library-cqrs.git` + +- Set up and activate Python virtual environment: + +`python3 -m venv .venv` + +`source .venv/bin/activate` + +- Install [poetry](https://python-poetry.org/docs/#installation) + +- Install dependencies: + +`poetry install` + +- Install pre-commit hooks: + +`pre-commit install` + +## Running Tests + +- Basic: `make test` + +- With coverage: `make test-cov` \ No newline at end of file diff --git a/src/lending/checkout_service.py b/src/lending/checkout_service.py index 54a28a0..fc28137 100644 --- a/src/lending/checkout_service.py +++ b/src/lending/checkout_service.py @@ -52,7 +52,7 @@ def __call__(self): patron_type=self.patron.patron_type, book_id=self.book.id, branch_id=self.branch_id, - checkout_date=checkout.checkout_date, - due_date=checkout.due_date, + checked_out_at=checkout.checked_out_at, + due_on=checkout.due_on, ) ) diff --git a/src/lending/daily_sheet_service.py b/src/lending/daily_sheet_service.py index 38654fe..ff6b37a 100644 --- a/src/lending/daily_sheet_service.py +++ b/src/lending/daily_sheet_service.py @@ -19,12 +19,12 @@ def _expire_holds(self): for hold in patron.holds: if ( hold.status == HoldStatus.ACTIVE.value - and hold.expiry_date < date.today() + and hold.expires_on < date.today() ): patron.expire_hold(hold.id) def _overdue_checkouts(self): for patron in self.patrons: for checkout in patron.checkouts: - if checkout.due_date < date.today(): + if checkout.due_on < date.today(): checkout.overdue() diff --git a/src/lending/holding_service.py b/src/lending/holding_service.py index efc792d..7ba42bd 100644 --- a/src/lending/holding_service.py +++ b/src/lending/holding_service.py @@ -63,7 +63,7 @@ def patron_cannot_not_have_more_than_two_overdue_checkouts_in_branch(self): overdue_checkouts_in_branch = [ checkout for checkout in self.patron.checkouts - if checkout.due_date < date.today() and checkout.branch_id == self.branch_id + if checkout.due_on < date.today() and checkout.branch_id == self.branch_id ] if len(overdue_checkouts_in_branch) > 2: raise ValidationError( @@ -74,14 +74,26 @@ def patron_cannot_not_have_more_than_two_overdue_checkouts_in_branch(self): } ) + @invariant.post + def open_holds_do_not_have_expiry_date(self): + if self.hold_type == HoldType.OPEN_ENDED: + if self.patron.holds[-1].expires_on: + raise ValidationError( + {"expires_on": ["Open-ended holds do not have an expiry date"]} + ) + def __call__(self): + expires_on = None + if self.hold_type == HoldType.CLOSED_ENDED: + expires_on = date.today() + timedelta(days=7) + hold = Hold( book_id=self.book.id, branch_id=self.branch_id, hold_type=self.hold_type.value, status=HoldStatus.ACTIVE.value, - request_date=datetime.now(), - expiry_date=date.today() + timedelta(days=7), + requested_at=datetime.now(), + expires_on=expires_on, ) self.patron.add_holds(hold) @@ -93,7 +105,7 @@ def __call__(self): book_id=hold.book_id, branch_id=hold.branch_id, hold_type=hold.hold_type, - request_date=hold.request_date, - expiry_date=hold.expiry_date, + requested_at=hold.requested_at, + expires_on=hold.expires_on, ) ) diff --git a/src/lending/patron.py b/src/lending/patron.py index 6cff379..7fd7674 100644 --- a/src/lending/patron.py +++ b/src/lending/patron.py @@ -37,8 +37,8 @@ class HoldExpired: branch_id = Identifier(required=True) book_id = Identifier(required=True) hold_type = String(required=True) - request_date = DateTime(required=True) - expiry_date = DateTime(required=True) + requested_at = DateTime(required=True) + expires_on = Date() @lending.event(part_of="Patron") @@ -51,8 +51,8 @@ class HoldPlaced: book_id = Identifier(required=True) branch_id = Identifier(required=True) hold_type = String(required=True) - request_date = DateTime(required=True) - expiry_date = DateTime(required=True) + requested_at = DateTime(required=True) + expires_on = Date() @lending.event(part_of="Patron") @@ -65,8 +65,8 @@ class HoldCancelled: branch_id = Identifier(required=True) book_id = Identifier(required=True) hold_type = String(required=True) - request_date = DateTime(required=True) - expiry_date = DateTime(required=True) + requested_at = DateTime(required=True) + expires_on = Date() @lending.entity(part_of="Patron") @@ -78,8 +78,8 @@ class Hold: branch_id = Identifier(required=True) hold_type = String(max_length=12, default=HoldType.CLOSED_ENDED.value) status = String(max_length=11, default=HoldStatus.ACTIVE.value) - request_date = DateTime(required=True) - expiry_date = Date(required=True) + requested_at = DateTime(required=True) + expires_on = Date() def checkout(self): self.status = HoldStatus.CHECKED_OUT.value @@ -94,13 +94,13 @@ def expire(self): branch_id=self.branch_id, book_id=self.book_id, hold_type=self.hold_type, - request_date=self.request_date, - expiry_date=self.expiry_date, + requested_at=self.requested_at, + expires_on=self.expires_on, ) ) def cancel(self): - if self.status == HoldStatus.EXPIRED.value or self.expiry_date < date.today(): + if self.status == HoldStatus.EXPIRED.value or self.expires_on < date.today(): raise ValidationError({"expired_hold": ["Cannot cancel expired holds"]}) if self.status == HoldStatus.CHECKED_OUT.value: @@ -116,8 +116,8 @@ def cancel(self): book_id=self.book_id, branch_id=self.branch_id, hold_type=self.hold_type, - request_date=self.request_date, - expiry_date=self.expiry_date, + requested_at=self.requested_at, + expires_on=self.expires_on, ) ) @@ -136,8 +136,8 @@ class BookCheckedOut: patron_type = String(required=True) book_id = Identifier(required=True) branch_id = Identifier(required=True) - checkout_date = DateTime(required=True) - due_date = Date(required=True) + checked_out_at = DateTime(required=True) + due_on = Date(required=True) @lending.event(part_of="Patron") @@ -148,9 +148,9 @@ class BookReturned: patron_type = String(required=True) book_id = Identifier(required=True) branch_id = Identifier(required=True) - checkout_date = DateTime(required=True) - due_date = Date(required=True) - return_date = DateTime(required=True) + checked_out_at = DateTime(required=True) + due_on = Date(required=True) + returned_at = DateTime(required=True) @lending.event(part_of="Patron") @@ -161,8 +161,8 @@ class BookOverdue: patron_type = String(required=True) book_id = Identifier(required=True) branch_id = Identifier(required=True) - checkout_date = DateTime(required=True) - due_date = Date(required=True) + checked_out_at = DateTime(required=True) + due_on = Date(required=True) @lending.entity(part_of="Patron") @@ -172,18 +172,18 @@ class Checkout: book_id = Identifier(required=True) branch_id = Identifier(required=True) - checkout_date = DateTime(required=True, default=utc_now) + checked_out_at = DateTime(required=True, default=utc_now) status = String(max_length=11, default=CheckoutStatus.ACTIVE.value) - due_date = Date( + due_on = Date( required=True, default=lambda: date.today() + timedelta(days=lending.config["custom"]["CHECKOUT_PERIOD"]), ) - return_date = DateTime() + returned_at = DateTime() def return_(self): self.status = CheckoutStatus.RETURNED.value - self.return_date = datetime.now() + self.returned_at = datetime.now() self.raise_( BookReturned( @@ -191,9 +191,9 @@ def return_(self): patron_type=self._owner.patron_type, book_id=self.book_id, branch_id=self.branch_id, - checkout_date=self.checkout_date, - due_date=self.due_date, - return_date=self.return_date, + checked_out_at=self.checked_out_at, + due_on=self.due_on, + returned_at=self.returned_at, ) ) @@ -206,8 +206,8 @@ def overdue(self): patron_type=self._owner.patron_type, book_id=self.book_id, branch_id=self.branch_id, - checkout_date=self.checkout_date, - due_date=self.due_date, + checked_out_at=self.checked_out_at, + due_on=self.due_on, ) ) diff --git a/tests/lending/bdd/checkouts/step_defs/checkout_steps.py b/tests/lending/bdd/checkouts/step_defs/checkout_steps.py index 2a3667a..b71c3f6 100644 --- a/tests/lending/bdd/checkouts/step_defs/checkout_steps.py +++ b/tests/lending/bdd/checkouts/step_defs/checkout_steps.py @@ -63,7 +63,7 @@ def patron_with_checkout(regular_patron, book): @given("the book is overdue") def mark_checkout_overdue(): patron = g.current_user - patron.checkouts[0].due_date = patron.checkouts[0].due_date - timedelta(days=1) + patron.checkouts[0].due_on = patron.checkouts[0].due_on - timedelta(days=1) patron.checkouts[0].status = CheckoutStatus.OVERDUE.value @@ -89,7 +89,7 @@ def system_has_overdue_checkouts(): ] ) # Manually expire a checkout - patron1.checkouts[0].due_date = date.today() - timedelta(days=1) + patron1.checkouts[0].due_on = date.today() - timedelta(days=1) patron2.add_checkouts( Checkout( @@ -98,7 +98,7 @@ def system_has_overdue_checkouts(): ) ) # Manually exipre a checkout - patron2.checkouts[0].due_date = date.today() - timedelta(days=1) + patron2.checkouts[0].due_on = date.today() - timedelta(days=1) g.current_patrons = [patron1, patron2] @@ -138,7 +138,7 @@ def confirm_checkout_book(): @then(cfparse("the checkout has a validity of {validity_days_config}")) def confirm_checkout_expiry(validity_days_config): checkout = g.current_user.checkouts[0] - assert checkout.due_date == date.today() + timedelta( + assert checkout.due_on == date.today() + timedelta( days=current_domain.config["custom"][validity_days_config] ) diff --git a/tests/lending/bdd/holds/features/open_and_close_ended_holds.feature b/tests/lending/bdd/holds/features/open_and_close_ended_holds.feature index 90a4067..ceecb8a 100644 --- a/tests/lending/bdd/holds/features/open_and_close_ended_holds.feature +++ b/tests/lending/bdd/holds/features/open_and_close_ended_holds.feature @@ -5,6 +5,7 @@ Feature: Handle Open-ended and Closed-ended Holds And a researcher patron is logged in When the patron places an open-ended hold Then the hold is successfully placed + And the hold does not have an expiry date Scenario: Regular patron tries to place an open-ended hold Given a circulating book is available diff --git a/tests/lending/bdd/holds/step_defs/hold_steps.py b/tests/lending/bdd/holds/step_defs/hold_steps.py index dbd3736..192916d 100644 --- a/tests/lending/bdd/holds/step_defs/hold_steps.py +++ b/tests/lending/bdd/holds/step_defs/hold_steps.py @@ -71,7 +71,7 @@ def closed_ended_hold_placed(): @given("the hold has reached its expiry date") def hold_expired(): - g.current_user.holds[0].expiry_date = date.today() - timedelta(days=1) + g.current_user.holds[0].expires_on = date.today() - timedelta(days=1) @given("patron has fewer than five holds") @@ -100,7 +100,7 @@ def patron_with_expired_hold(patron, book): g.current_book = book place_hold(g.current_user, book, "1", HoldType.CLOSED_ENDED)() - g.current_user.holds[0].expiry_date = date.today() - timedelta(days=1) + g.current_user.holds[0].expires_on = date.today() - timedelta(days=1) @given("a patron has a hold that has been checked out") @@ -230,3 +230,8 @@ def check_hold_cancellation_rejected(): assert "HoldCancelled" not in [ event.__class__.__name__ for event in g.current_user._events ] + + +@then("the hold does not have an expiry date") +def confirm_no_expiry_date(): + assert g.current_user.holds[0].expires_on is None diff --git a/tests/lending/conftest.py b/tests/lending/conftest.py index 22075bd..6d34d6c 100644 --- a/tests/lending/conftest.py +++ b/tests/lending/conftest.py @@ -71,27 +71,27 @@ def researcher_patron(patron): @pytest.fixture def overdue_checkouts_patron(patron): - checkout_date1 = fake.date_between(start_date="-90d", end_date="-61d") - checkout_date2 = fake.date_between(start_date="-80d", end_date="-71d") - checkout_date3 = fake.date_between(start_date="-85d", end_date="-75d") + checkout_date1 = fake.date_time_between(start_date="-90d", end_date="-61d") + checkout_date2 = fake.date_time_between(start_date="-80d", end_date="-71d") + checkout_date3 = fake.date_time_between(start_date="-85d", end_date="-75d") patron.checkouts = [ lending.Checkout( branch_id="1", book_id=fake.uuid4(), - checkout_date=checkout_date1, - due_date=checkout_date1 + timedelta(days=60), + checked_out_at=checkout_date1, + due_on=checkout_date1.date() + timedelta(days=60), ), lending.Checkout( branch_id="1", book_id=fake.uuid4(), - checkout_date=checkout_date2, - due_date=checkout_date2 + timedelta(days=60), + checked_out_at=checkout_date2, + due_on=checkout_date2.date() + timedelta(days=60), ), lending.Checkout( branch_id="1", book_id=fake.uuid4(), - checkout_date=checkout_date3, - due_date=checkout_date3 + timedelta(days=60), + checked_out_at=checkout_date3, + due_on=checkout_date3.date() + timedelta(days=60), ), ] return patron diff --git a/tests/lending/tdd/patron/test_checkout_entity.py b/tests/lending/tdd/patron/test_checkout_entity.py index 758e18c..84c1a5c 100644 --- a/tests/lending/tdd/patron/test_checkout_entity.py +++ b/tests/lending/tdd/patron/test_checkout_entity.py @@ -11,5 +11,5 @@ def test_checkout_entity_element_type(): def test_checkout_entity_declared_fields(): assert all( field_name in declared_fields(Checkout) - for field_name in ["id", "book_id", "checkout_date", "due_date"] + for field_name in ["id", "book_id", "checked_out_at", "due_on"] )