circulation: fix request with different locations

* Checks the owning location `allow_request` property to decide if an item can be requested.
* Closes #2234.
* Logs an error and do a db rollback for holding views.
* Fixes tests that failed every two monthes.

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

rero_ils/modules/holdings/api_views.py

@@ -24,8 +24,9 @@
 
 from flask import Blueprint, abort, current_app, jsonify
 from flask import request as flask_request
+from invenio_db import db
 from jinja2.exceptions import TemplateSyntaxError, UndefinedError
-from werkzeug.exceptions import NotFound
+from werkzeug.exceptions import NotFound, Unauthorized
 
 from rero_ils.modules.views import check_authentication
 
@@ -49,7 +50,7 @@ def jsonify_error(func):
     def decorated_view(*args, **kwargs):
         try:
             return func(*args, **kwargs)
-        except NotFound as error:
+        except (Unauthorized, NotFound) as error:
             raise(error)
         except TemplateSyntaxError as error:
             return jsonify({'status': 'error: {error}'.format(
@@ -58,8 +59,10 @@ def decorated_view(*args, **kwargs):
             return jsonify({'status': 'error: {error}'.format(
                 error=error)}), 400
         except Exception as error:
-            raise(error)
+            # uncomment for debug:
+            # raise(errqor)
             current_app.logger.error(str(error))
+            db.session.rollback()
             return jsonify({'status': 'error: {error}'.format(
                 error=error)}), 500
     return decorated_view

rero_ils/modules/loans/utils.py

@@ -159,8 +159,9 @@ def can_be_requested(loan):
     if patron.patron.get('blocked', False):
         return False
 
-    # 2) Check if location allows request
-    location = Location.get_record_by_pid(loan.location_pid)
+    # 2) Check if owning location allows request
+    location_pid = Item.get_record_by_pid(loan.item_pid).holding_location_pid
+    location = Location.get_record_by_pid(location_pid)
     if not location or not location.get('allow_request'):
         return False
 

tests/api/items/test_items_rest.py

@@ -907,6 +907,65 @@ def test_patron_request(client, patron_martigny, loc_public_martigny,
         )
     )
     assert res.status_code == 200
+    loan_pid = data.get('action_applied')[LoanAction.REQUEST].get('pid')
+    params = {
+        'pid': loan_pid,
+        'transaction_location_pid': loc_public_martigny.pid,
+        'transaction_user_pid': patron_martigny.pid
+    }
+    item_lib_martigny.cancel_item_request(**params)
+
+
+def test_requests_with_different_locations(
+    client, patron_martigny, librarian_saxon, loc_public_saxon,
+    loc_public_martigny, item_lib_martigny, circulation_policies, lib_saxon
+):
+    """Test patron and librarian request with different locations."""
+    login_user_via_session(client, patron_martigny.user)
+    loc_public_saxon['allow_request'] = False
+    loc_public_saxon.update(loc_public_saxon, True, True)
+    res, data = postdata(
+        client,
+        'api_item.patron_request',
+        dict(
+            item_pid=item_lib_martigny.pid,
+            pickup_location_pid=loc_public_saxon.pid
+        )
+    )
+    assert res.status_code == 200
+
+    loan_pid = data.get('action_applied')[LoanAction.REQUEST].get('pid')
+    params = {
+        'pid': loan_pid,
+        'transaction_location_pid': loc_public_martigny.pid,
+        'transaction_user_pid': patron_martigny.pid
+    }
+    item_lib_martigny.cancel_item_request(**params)
+
+    login_user_via_session(client, librarian_saxon.user)
+    res, data = postdata(
+        client,
+        'api_item.librarian_request',
+        dict(
+            item_pid=item_lib_martigny.pid,
+            patron_pid=patron_martigny.pid,
+            pickup_location_pid=loc_public_martigny.pid,
+            transaction_library_pid=lib_saxon.pid,
+            transaction_user_pid=librarian_saxon.pid
+        )
+    )
+    assert res.status_code == 200
+
+    loan_pid = data.get('action_applied')[LoanAction.REQUEST].get('pid')
+    params = {
+        'pid': loan_pid,
+        'transaction_location_pid': loc_public_saxon.pid,
+        'transaction_user_pid': librarian_saxon.pid
+    }
+    item_lib_martigny.cancel_item_request(**params)
+
+    loc_public_saxon['allow_request'] = True
+    loc_public_saxon.update(loc_public_saxon, True, True)
 
 
 def test_item_possible_actions(client, item_lib_martigny,

tests/ui/loans/test_loans_api.py

@@ -30,7 +30,7 @@
 
 from rero_ils.modules.circ_policies.api import DUE_SOON_REMINDER_TYPE
 from rero_ils.modules.libraries.api import Library
-from rero_ils.modules.loans.api import Loan, LoanState, get_loans_by_patron_pid
+from rero_ils.modules.loans.api import Loan, LoanState
 from rero_ils.modules.loans.tasks import loan_anonymizer
 from rero_ils.modules.loans.utils import get_circ_policy, \
     get_default_loan_duration, sum_for_fees
@@ -52,22 +52,28 @@ def test_loans_create(loan_pending_martigny):
     assert loan_pending_martigny.get('state') == LoanState.PENDING
 
 
-def test_item_loans_elements(
-        loan_pending_martigny, item_lib_fully, circ_policy_default_martigny):
-    """Test loan elements."""
-    assert loan_pending_martigny.item_pid == item_lib_fully.pid
-    loan = list(get_loans_by_patron_pid(loan_pending_martigny.patron_pid))[0]
-    assert loan.pid == loan_pending_martigny.get('pid')
-
-    new_loan = deepcopy(loan_pending_martigny)
+def test_item_loans_default_duration(
+        item_lib_martigny, librarian_martigny, patron_martigny,
+        loc_public_martigny, circulation_policies):
+    """Test default loan duration."""
+
+    item, actions = item_lib_martigny.request(
+        pickup_location_pid=loc_public_martigny.pid,
+        patron_pid=patron_martigny.pid,
+        transaction_location_pid=loc_public_martigny.pid,
+        transaction_user_pid=librarian_martigny.pid
+    )
+    loan_pid = actions['request']['pid']
+    loan = Loan.get_record_by_pid(loan_pid)
+    new_loan = deepcopy(loan)
     del new_loan['transaction_location_pid']
     assert get_default_loan_duration(new_loan, None) == \
-        get_default_loan_duration(loan_pending_martigny, None)
-
-    assert item_lib_fully.last_location_pid == item_lib_fully.location_pid
-    del circ_policy_default_martigny['checkout_duration']
-    circ_policy_default_martigny.update(
-        circ_policy_default_martigny, dbcommit=True, reindex=True)
+        get_default_loan_duration(loan, None)
+    item_lib_martigny.cancel_item_request(
+        pid=loan.pid,
+        transaction_location_pid=loc_public_martigny.pid,
+        transaction_user_pid=librarian_martigny.pid
+    )
 
 
 def test_is_due_soon(