From cb2484757d4e19f2a2472d074430228acec05832 Mon Sep 17 00:00:00 2001 From: PascalRepond Date: Wed, 14 Jun 2023 11:38:18 +0200 Subject: [PATCH] circulation: allow checkout from never open lib MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Fixes infinite loop when checking out an item from library without open days but with open exception dates in the past. - Add an extended validation to libraries to disallow open exception dates without specifying times. - Fixes LibraryNeverOpen error not being passed to monitoring. - Fixes checkout not possible from library without opening hours. - Closes #2419. Co-Authored-by: Pascal Repond Co-Authored-by: Johnny MarieĢthoz --- rero_ils/modules/items/api/circulation.py | 13 ++- rero_ils/modules/libraries/api.py | 26 ++++- rero_ils/modules/loans/utils.py | 20 +++- tests/api/libraries/test_libraries_rest.py | 26 +++++ tests/api/loans/test_loans_rest.py | 1 - tests/ui/circulation/test_actions_checkout.py | 100 ++++++++++++++++++ 6 files changed, 172 insertions(+), 14 deletions(-) diff --git a/rero_ils/modules/items/api/circulation.py b/rero_ils/modules/items/api/circulation.py index 6138872c11..cfefb19ef1 100644 --- a/rero_ils/modules/items/api/circulation.py +++ b/rero_ils/modules/items/api/circulation.py @@ -17,6 +17,7 @@ """API for manipulating item circulation transactions.""" +from contextlib import suppress from copy import deepcopy from datetime import datetime @@ -45,6 +46,7 @@ from ...errors import NoCirculationAction from ...item_types.api import ItemType from ...libraries.api import Library +from ...libraries.exceptions import LibraryNeverOpen from ...loans.api import Loan, get_last_transaction_loc_for_item, \ get_request_by_item_pid_by_patron_pid from ...loans.models import LoanAction, LoanState @@ -420,10 +422,13 @@ def checkout(self, current_loan, **kwargs): transaction_library_pid = self.library_pid library = Library.get_record_by_pid(transaction_library_pid) if not library.is_open(action_params['end_date'], True): - new_end_date = library.next_open(action_params['end_date']) - new_end_date = new_end_date.astimezone()\ - .replace(microsecond=0).isoformat() - action_params['end_date'] = new_end_date + # If library has no open dates, keep the default due date + # to avoid circulation errors + with suppress(LibraryNeverOpen): + new_end_date = library.next_open(action_params['end_date']) + new_end_date = new_end_date.astimezone()\ + .replace(microsecond=0).isoformat() + action_params['end_date'] = new_end_date # Call invenio_circulation for 'checkout' trigger loan = current_circulation.circulation.trigger( current_loan, diff --git a/rero_ils/modules/libraries/api.py b/rero_ils/modules/libraries/api.py index 1ed723cbe7..cddae93881 100644 --- a/rero_ils/modules/libraries/api.py +++ b/rero_ils/modules/libraries/api.py @@ -25,6 +25,8 @@ from dateutil import parser from dateutil.rrule import FREQNAMES, rrule +from flask_babelex import gettext as _ + from rero_ils.modules.api import IlsRecord, IlsRecordsIndexer, IlsRecordsSearch from rero_ils.modules.fetchers import id_fetcher from rero_ils.modules.locations.api import LocationsSearch @@ -76,6 +78,17 @@ class Library(IlsRecord): } } + def extended_validation(self, **kwargs): + """Add additional record validation. + + :return: reason for validation failure, otherwise True + """ + for exception_date in self.get('exception_dates', []): + if exception_date['is_open'] and not exception_date.get('times'): + return _('Opening times must be specified for an open ' + 'exception date.') + return True + @property def online_location(self): """Get the online location.""" @@ -168,14 +181,17 @@ def _is_betweentimes(self, time_to_test, times): return times_open def _has_is_open(self): - """Test if library has opening days.""" + """Test if library has opening days in the future.""" if opening_hours := self.get('opening_hours'): for opening_hour in opening_hours: if opening_hour['is_open']: return True if exception_dates := self.get('exception_dates'): for exception_date in exception_dates: - if exception_date['is_open']: + start_date = date_string_to_utc(exception_date['start_date']) + # avoid next_open infinite loop if an open exception date is + # in the past + if start_date > datetime.now(pytz.utc): return True return False @@ -241,7 +257,7 @@ def is_open(self, date=None, day_only=False): # is into this periods (depending of `day_only` method argument). day_name = date.strftime("%A").lower() regular_rule = [ - rule for rule in self['opening_hours'] + rule for rule in self.get('opening_hours', []) if rule['day'] == day_name ] if regular_rule: @@ -280,7 +296,9 @@ def next_open(self, date=None, previous=False, ensure=False): """Get next open day.""" date = date or datetime.now(pytz.utc) if not self._has_is_open(): - raise LibraryNeverOpen + raise LibraryNeverOpen( + f'No open days found for library (pid: {self.pid})' + ) if isinstance(date, str): date = parser.parse(date) add_day = -1 if previous else 1 diff --git a/rero_ils/modules/loans/utils.py b/rero_ils/modules/loans/utils.py index daf0024361..e5ab235243 100644 --- a/rero_ils/modules/loans/utils.py +++ b/rero_ils/modules/loans/utils.py @@ -26,6 +26,7 @@ from rero_ils.modules.circ_policies.api import CircPolicy from rero_ils.modules.items.api import Item from rero_ils.modules.libraries.api import Library +from rero_ils.modules.libraries.exceptions import LibraryNeverOpen from rero_ils.modules.locations.api import Location from rero_ils.modules.patrons.api import Patron from rero_ils.modules.utils import get_ref_for_pid @@ -83,10 +84,14 @@ def get_default_loan_duration(loan, initial_loan): due_date_eve = now_in_library_timezone \ + timedelta(days=policy.get('checkout_duration', 0)) \ - timedelta(days=1) - next_open_date = library.next_open(date=due_date_eve) + try: + end_date = library.next_open(date=due_date_eve) + except LibraryNeverOpen: + # if the library has no open day, use standard loan duration from cipo + end_date = due_date_eve + timedelta(days=1) # all libraries are closed at 23h59 # the next_open returns UTC. - end_date_in_library_timezone = next_open_date.astimezone( + end_date_in_library_timezone = end_date.astimezone( library.get_timezone()).replace( hour=23, minute=59, @@ -152,11 +157,16 @@ def extend_loan_data_is_valid(end_date, renewal_duration, library_pid): renewal_duration = renewal_duration or 0 end_date = ciso8601.parse_datetime(end_date) library = Library.get_record_by_pid(library_pid) - first_open_date = library.next_open( - date=datetime.now(timezone.utc) + try: + first_open_date = library.next_open( + date=datetime.now(timezone.utc) + + timedelta(days=renewal_duration) + - timedelta(days=1) + ) + except LibraryNeverOpen: + first_open_date = datetime.now(timezone.utc) + timedelta(days=renewal_duration) - timedelta(days=1) - ) return first_open_date.date() > end_date.date() diff --git a/tests/api/libraries/test_libraries_rest.py b/tests/api/libraries/test_libraries_rest.py index 829042a2e7..86fff317a2 100644 --- a/tests/api/libraries/test_libraries_rest.py +++ b/tests/api/libraries/test_libraries_rest.py @@ -21,6 +21,7 @@ import mock import pytest +from datetime import datetime, timedelta from flask import url_for from invenio_accounts.testutils import login_user_via_session from utils import VerifyRecordPermissionPatch, get_json, postdata, \ @@ -151,8 +152,33 @@ def test_library_never_open(lib_sion): assert lib_sion.next_open() del lib_sion['opening_hours'] + # add an exception date in the past + open_exception = { + 'is_open': True, + 'start_date': '2012-01-09', + 'title': 'Ouverture exceptionnelle', + 'times': [ + { + 'end_time': '16:00', + 'start_time': '12:00' + } + ] + } + + lib_sion['exception_dates'].append(open_exception) lib_sion.update(lib_sion, dbcommit=True, reindex=True) + # check that the exception in the past is not considered for next open date + with pytest.raises(LibraryNeverOpen): + assert lib_sion.next_open() + + # compute a date in the future and add it as exception date + today = datetime.today() + future_date = (today + timedelta(days=14)).strftime('%Y-%m-%d') + open_exception['start_date'] = future_date + lib_sion.update(lib_sion, dbcommit=True, reindex=True) + + # check that the exception in the future is considered as open date assert lib_sion._has_is_open() del lib_sion['exception_dates'] diff --git a/tests/api/loans/test_loans_rest.py b/tests/api/loans/test_loans_rest.py index def7d66a0d..6a30e6f112 100644 --- a/tests/api/loans/test_loans_rest.py +++ b/tests/api/loans/test_loans_rest.py @@ -532,7 +532,6 @@ def test_timezone_due_date(client, librarian_martigny, login_user_via_session(client, librarian_martigny.user) # Close the library all days. Except Monday. - del lib_martigny['opening_hours'] del lib_martigny['exception_dates'] lib_martigny['opening_hours'] = [ { diff --git a/tests/ui/circulation/test_actions_checkout.py b/tests/ui/circulation/test_actions_checkout.py index 342d86c73c..c473786db1 100644 --- a/tests/ui/circulation/test_actions_checkout.py +++ b/tests/ui/circulation/test_actions_checkout.py @@ -31,6 +31,106 @@ from rero_ils.modules.loans.models import LoanAction, LoanState +def test_checkout_library_never_open( + circulation_policies, + patron_martigny, + lib_martigny, + item_lib_martigny, + loc_public_martigny, + librarian_martigny + ): + """Test checkout from a library without opening hours.""" + + # Test checkout if library has no open days but has exception days/hours + # in the past + lib_martigny['opening_hours'] = [ + { + "day": "monday", + "is_open": False, + "times": [] + }, + { + "day": "tuesday", + "is_open": False, + "times": [] + }, + { + "day": "wednesday", + "is_open": False, + "times": [] + }, + { + "day": "thursday", + "is_open": False, + "times": [] + }, + { + "day": "friday", + "is_open": False, + "times": [] + }, + { + "day": "saturday", + "is_open": False, + "times": [] + }, + { + "day": "sunday", + "is_open": False, + "times": [] + } + ] + lib_martigny.commit() + + data = deepcopy(item_lib_martigny) + data.pop('barcode') + data.setdefault('status', ItemStatus.ON_SHELF) + created_item = Item.create( + data=data, dbcommit=True, reindex=True, delete_pid=True) + + params = { + 'patron_pid': patron_martigny.pid, + 'transaction_location_pid': loc_public_martigny.pid, + 'transaction_user_pid': librarian_martigny.pid, + 'pickup_location_pid': loc_public_martigny.pid + } + onloan_item, actions = created_item.checkout(**params) + loan = Loan.get_record_by_pid(actions[LoanAction.CHECKOUT].get('pid')) + + # check that can_extend method does not raise exception + assert Loan.can_extend(onloan_item)[0] in [True, False] + + # Check loan is ITEM_ON_LOAN and item is ON_LOAN + assert onloan_item.status == ItemStatus.ON_LOAN + assert loan['state'] == LoanState.ITEM_ON_LOAN + + # Test checkout if library has no open days and no exception days/hours + del lib_martigny['exception_dates'] + lib_martigny.commit() + + data = deepcopy(item_lib_martigny) + data.pop('barcode') + data.setdefault('status', ItemStatus.ON_SHELF) + created_item = Item.create( + data=data, dbcommit=True, reindex=True, delete_pid=True) + + params = { + 'patron_pid': patron_martigny.pid, + 'transaction_location_pid': loc_public_martigny.pid, + 'transaction_user_pid': librarian_martigny.pid, + 'pickup_location_pid': loc_public_martigny.pid + } + onloan_item, actions = created_item.checkout(**params) + loan = Loan.get_record_by_pid(actions[LoanAction.CHECKOUT].get('pid')) + + # Check loan is ITEM_ON_LOAN and item is ON_LOAN + assert onloan_item.status == ItemStatus.ON_LOAN + assert loan['state'] == LoanState.ITEM_ON_LOAN + + from invenio_db import db + db.session.rollback() + + def test_checkout_on_item_on_shelf( circulation_policies, patron_martigny,