circulation: allow checkout from never open lib

- 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 rero#2419.

Co-Authored-by: Pascal Repond <pascal.repond@rero.ch>
Co-Authored-by: Johnny Mariéthoz <Johnny.Mariethoz@rero.ch>

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,

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

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,

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']

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'] = [
         {

tests/ui/circulation/test_actions_checkout.py

@@ -31,6 +31,103 @@
 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 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,
@@ -41,6 +138,7 @@ def test_checkout_on_item_on_shelf(
         item_on_shelf_martigny_patron_and_loan_pending):
     """Test checkout on an ON_SHELF item."""
     # Create a new item in ON_SHELF (without Loan)
+    print()
     data = deepcopy(item_lib_martigny)
     data.pop('barcode')
     data.setdefault('status', ItemStatus.ON_SHELF)