fix(circ): item can wrongfully become at desk

* Fixes incorrect test for action CANCEL_REQUEST_5_1_2.
* Closes #3403.
* Closes #3569.

Co-Authored-by: Pascal Repond <[email protected]>

rero_ils/modules/items/api/circulation.py

@@ -474,28 +474,32 @@ def cancel_item_request(self, pid, **kwargs):
             self.status_update(
                 self, dbcommit=True, reindex=True, forceindex=True)
             actions.update({LoanAction.UPDATE: loan})
-            item = self
         elif actions_to_execute.get('validate_first_pending'):
             pending = self.get_first_loan_by_state(state=LoanState.PENDING)
-            # When the loan to cancel is ITEM_IN_TRANSIT_FOR_PICKUP, we need
-            # virtually to undo the last transaction and restart from the last
-            # location and validate the first pending request.
-            # to achieve this, we pass the cancelled loan pickup_location_pid
-            # as a transaction_location_pid for the validation of the next
-            # pending loan. This way the validation logic is not modified.
-            cancelled_loan_pickup_pid = None
-            if loan.get('state') == LoanState.ITEM_IN_TRANSIT_FOR_PICKUP:
-                cancelled_loan_pickup_pid = loan.get('pickup_location_pid')
-            item, actions = self.cancel_loan(pid=loan.pid, **kwargs)
-            if cancelled_loan_pickup_pid and \
-               pending['pickup_location_pid'] != cancelled_loan_pickup_pid:
-                kwargs['transaction_location_pid'] = cancelled_loan_pickup_pid
+            loan_pickup = loan.get('pickup_location_pid', None)
+            pending_pickup = pending.get('pickup_location_pid', None)
+            # If the item is at_desk at the same location as the next loan
+            # pickup we can validate the next loan so that it becomes at desk
+            # for the next patron.
+            if loan.get('state') == LoanState.ITEM_AT_DESK\
+                    and loan_pickup == pending_pickup:
+                item, actions = self.cancel_loan(pid=loan.pid, **kwargs)
+                kwargs['transaction_location_pid'] = loan_pickup
                 kwargs.pop('transaction_library_pid', None)
-            item, validate_actions = self.validate_request(
-                pid=pending.pid, **kwargs)
-            actions.update(validate_actions)
-        else:
-            item = self
+                item, validate_actions = self.validate_request(
+                    pid=pending.pid,
+                    **kwargs)
+                actions.update(validate_actions)
+            # Otherwise, we simply change the state of the next loan and it
+            # will be validated at the next checkin at the pickup location.
+            else:
+                pending['state'] = LoanState.ITEM_IN_TRANSIT_FOR_PICKUP
+                pending.update(pending, dbcommit=True, reindex=True)
+                item, actions = self.cancel_loan(pid=loan.pid, **kwargs)
+                self.status_update(self, dbcommit=True,
+                                   reindex=True, forceindex=True)
+                actions.update({LoanAction.UPDATE: loan})
+        item = self
         return item, actions
 
     def checks_before_a_cancel_item_request(self, loan, **kwargs):
@@ -532,6 +536,9 @@ def checks_before_a_cancel_item_request(self, loan, **kwargs):
                 # and the loan becomes ITEM_IN_TRANSIT_TO_HOUSE.
                 actions_to_execute['loan_update']['state'] = \
                     LoanState.ITEM_IN_TRANSIT_TO_HOUSE
+                # Mark the loan to be cancelled to create an
+                # OperationLog about this cancellation.
+                actions_to_execute['cancel_loan'] = True
             elif loan['state'] == LoanState.ITEM_AT_DESK:
                 if not libraries['item_pickup_libraries']:
                     # CANCEL_REQUEST_2_1_1_1: when item library and pickup
@@ -553,8 +560,9 @@ def checks_before_a_cancel_item_request(self, loan, **kwargs):
             actions_to_execute['validate_first_pending'] = True
         elif loan['state'] == LoanState.ITEM_IN_TRANSIT_TO_HOUSE and \
                 LoanState.PENDING in states:
-            # CANCEL_REQUEST_5_1_2: when item at desk with pending loan, cancel
-            # the loan triggers an automatic validation of first pending loan.
+            # CANCEL_REQUEST_5_1_2: when item in_transit with pending loan,
+            # cancelling the loan triggers an automatic validation of first
+            # pending loan.
             actions_to_execute['validate_first_pending'] = True
         elif loan['state'] == LoanState.PENDING and \
             any(state in states for state in [

tests/api/notifications/test_notifications_rest.py

@@ -1199,6 +1199,7 @@ def test_booking_notifications(client, patron_martigny, patron_sion,
     }
     item_lib_martigny.cancel_item_request(
         request_loan_pid,
+        transaction_user_pid=librarian_martigny.pid,
         transaction_location_pid=loc_public_martigny.pid)
     item_lib_martigny.receive(**params)
 

tests/ui/circulation/test_actions_cancel_request.py

@@ -18,6 +18,8 @@
 """Test item circulation cancel request actions."""
 
 
+from copy import deepcopy
+
 import pytest
 from utils import item_record_to_a_specific_loan_state
 
@@ -28,6 +30,47 @@
 from rero_ils.modules.loans.models import LoanState
 
 
+def test_cancel_item_request_in_transit_for_pickup_with_requests_same_lib(
+        client, item3_in_transit_martigny_patron_and_loan_for_pickup,
+        loc_public_martigny, librarian_martigny, librarian_fully,
+        loc_public_fully, patron2_martigny):
+    """Test cancel requests on an in_transit for pickup item with requests."""
+    item, patron, loan = item3_in_transit_martigny_patron_and_loan_for_pickup
+    origin_loan = deepcopy(loan)
+    # the following tests a special case of the circulation action
+    # CANCEL_REQUEST_4_1_2 an item in_transit for pickup with
+    # other pending loans and the pickup locations are the same.
+
+    params = {
+        'patron_pid': patron2_martigny.pid,
+        'transaction_location_pid': loc_public_fully.pid,
+        'transaction_user_pid': librarian_fully.pid,
+        'pickup_location_pid': loc_public_fully.pid
+    }
+    item, requested_loan = item_record_to_a_specific_loan_state(
+        item=item, loan_state=LoanState.PENDING,
+        params=params, copy_item=False)
+    assert requested_loan['state'] == LoanState.PENDING
+
+    params = {
+        'pid': loan.pid,
+        'transaction_location_pid': loc_public_fully.pid,
+        'transaction_user_pid': librarian_fully.pid
+    }
+    item.cancel_item_request(**params)
+    item = Item.get_record_by_pid(item.pid)
+    loan = Loan.get_record_by_pid(loan.pid)
+    requested_loan = Loan.get_record_by_pid(requested_loan.pid)
+    assert item.status == ItemStatus.IN_TRANSIT
+    assert loan['state'] == LoanState.CANCELLED
+    assert requested_loan['state'] == LoanState.ITEM_IN_TRANSIT_FOR_PICKUP
+
+    # Clean created data
+    requested_loan.delete(force=True, dbcommit=True, delindex=True)
+    loan.replace(origin_loan, True, True, True)
+    Item.status_update(item=item, on_shelf=False, dbcommit=True, reindex=True)
+
+
 def test_cancel_request_on_item_on_shelf(
         item_lib_martigny, item_on_shelf_martigny_patron_and_loan_pending,
         loc_public_martigny, librarian_martigny,
@@ -412,9 +455,9 @@ def test_cancel_request_on_item_in_transit_to_house_with_requests(
     item = Item.get_record_by_pid(item.pid)
     loan = Loan.get_record_by_pid(loan.pid)
     requested_loan = Loan.get_record_by_pid(requested_loan.pid)
-    assert item.status == ItemStatus.AT_DESK
+    assert item.status == ItemStatus.IN_TRANSIT
     assert loan['state'] == LoanState.CANCELLED
-    assert requested_loan['state'] == LoanState.ITEM_AT_DESK
+    assert requested_loan['state'] == LoanState.ITEM_IN_TRANSIT_FOR_PICKUP
 
 
 def test_cancel_pending_on_item_in_transit_to_house(