global: improve generation of records permissions

When we calculate permissions for records, we have double call on get_links_to_me().
To fix that, IlsRecord.can_delete return a tuple with True|False and reasons not to delete
if False.

Co-Authored-by: Laurent Dubois <laurent.dubois@itld-solutions.be>

rero_ils/modules/api.py

@@ -309,7 +309,8 @@ def count(cls, with_deleted=False):
 
     def delete(self, force=False, dbcommit=False, delindex=False):
         """Delete record and persistent identifier."""
-        if self.can_delete:
+        can, _ = self.can_delete
+        if can:
             if delindex:
                 self.delete_from_index()
             persistent_identifier = self.get_persistent_identifier(self.id)
@@ -425,8 +426,12 @@ def reasons_not_to_delete(self):
 
     @property
     def can_delete(self):
-        """Record can be deleted."""
-        return len(self.reasons_not_to_delete()) == 0
+        """Record can be deleted.
+
+        :return a tuple with True|False and reasons not to delete if False.
+        """
+        reasons = self.reasons_not_to_delete()
+        return len(reasons) == 0, reasons
 
     @property
     def organisation_pid(self):

rero_ils/modules/permissions.py

@@ -76,10 +76,10 @@ def record_permissions(record_pid=None, route_name=None):
             # before ; either the `delete_permissions_factory` for this record
             # should be called. If this call send 'False' then the
             # reason_not_to_delete should be "permission denied"
+            can_delete, reasons = record.can_delete
             permissions['delete']['can'] = \
-                record.can_delete and \
+                can_delete and \
                 record_permissions_factory['delete'](record=record).can()
-            reasons = record.reasons_not_to_delete()
             if not permissions['delete']['can'] and not reasons:
                 # in this case, it's because config delete factory return
                 # `False`, so the reason is 'Permission denied'

rero_ils/permissions.py

@@ -50,7 +50,7 @@ def librarian_update_permission_factory(record, *args, **kwargs):
 def librarian_delete_permission_factory(
         record, credentials_only=False, *args, **kwargs):
     """User can delete record."""
-    if credentials_only or record.can_delete:
+    if credentials_only or record.can_delete[0]:
         return librarian_permission
     return type('Check', (), {'can': lambda x: False})()
 

tests/api/acq_accounts/test_acq_accounts_rest.py

@@ -167,13 +167,9 @@ def test_acq_accounts_can_delete(
         client, document, acq_account_fiction_martigny,
         acq_order_line_fiction_martigny, acq_order_fiction_martigny):
     """Test can delete an acq account."""
-    links = acq_account_fiction_martigny.get_links_to_me()
-    assert 'acq_order_lines' in links
-
-    assert not acq_account_fiction_martigny.can_delete
-
-    reasons = acq_account_fiction_martigny.reasons_not_to_delete()
-    assert 'links' in reasons
+    can, reasons = acq_account_fiction_martigny.can_delete
+    assert not can
+    assert reasons['links']['acq_order_lines']
 
 
 def test_filtered_acq_accounts_get(

tests/api/acq_invoices/test_acq_invoices_rest.py

@@ -172,13 +172,9 @@ def test_acquisition_invoices_post_put_delete(
 
 def test_acquisition_invoices_can_delete(client, acq_invoice_fiction_martigny):
     """Test can delete an acquisition invoice."""
-    links = acq_invoice_fiction_martigny.get_links_to_me()
-    assert not links
-
-    assert acq_invoice_fiction_martigny.can_delete
-
-    reasons = acq_invoice_fiction_martigny.reasons_not_to_delete()
-    assert not reasons
+    can, reasons = acq_invoice_fiction_martigny.can_delete
+    assert can
+    assert reasons == {}
 
 
 def test_filtered_acquisition_invoices_get(

tests/api/acq_order_lines/test_acq_order_lines_rest.py

@@ -148,21 +148,16 @@ def test_acq_order_lines_post_put_delete(
 
 def test_acq_order_lines_can_delete(client, acq_order_line_fiction_martigny):
     """Test can delete an acq order line."""
-    links = acq_order_line_fiction_martigny.get_links_to_me()
-    assert not links
-
-    assert acq_order_line_fiction_martigny.can_delete
-
-    reasons = acq_order_line_fiction_martigny.reasons_not_to_delete()
-    assert not reasons
+    can, reasons = acq_order_line_fiction_martigny.can_delete
+    assert can
+    assert reasons == {}
 
 
 def test_acq_order_lines_document_can_delete(
         client, document, acq_order_line_fiction_martigny):
     """Test can delete a document with a linked acquisition order line."""
-    assert not document.can_delete
-
-    reasons = document.reasons_not_to_delete()
+    can, reasons = document.can_delete
+    assert not can
     assert reasons['links']['acq_order_lines']
 
 

tests/api/acq_orders/test_acq_orders_rest.py

@@ -164,13 +164,9 @@ def test_acq_orders_can_delete(
         client, document, acq_order_fiction_martigny,
         acq_order_line_fiction_martigny):
     """Test can delete an acq order."""
-    links = acq_order_fiction_martigny.get_links_to_me()
-    assert 'acq_order_lines' in links
-
-    assert not acq_order_fiction_martigny.can_delete
-
-    reasons = acq_order_fiction_martigny.reasons_not_to_delete()
-    assert 'links' in reasons
+    can, reasons = acq_order_fiction_martigny.can_delete
+    assert not can
+    assert reasons['links']['acq_order_lines']
 
 
 def test_filtered_acq_orders_get(

tests/api/budgets/test_budgets_rest.py

@@ -147,16 +147,10 @@ def test_budgets_post_put_delete(client,
 def test_budgets_can_delete(
         client, budget_2020_martigny, acq_account_fiction_martigny):
     """Test can delete an acq account."""
-    links = budget_2020_martigny.get_links_to_me()
-    assert 'acq_accounts' in links
-
-    assert not budget_2020_martigny.can_delete
-
-    reasons_to_keep = budget_2020_martigny.reasons_to_keep()
-    assert reasons_to_keep.get('is_default')
-
-    reasons = budget_2020_martigny.reasons_not_to_delete()
-    assert 'links' in reasons
+    can, reasons = budget_2020_martigny.can_delete
+    assert not can
+    assert reasons['links']['acq_accounts']
+    assert reasons['others']['is_default']
 
 
 def test_filtered_budgets_get(

tests/api/holdings/test_holdings_rest.py

@@ -105,13 +105,9 @@ def test_holdings_get(client, holding_lib_martigny):
 def test_holding_can_delete_and_utils(client, holding_lib_martigny, document,
                                       item_type_standard_martigny):
     """Test can delete a holding."""
-    links = holding_lib_martigny.get_links_to_me()
-    assert 'items' not in links
-
-    assert holding_lib_martigny.can_delete
-
-    reasons = holding_lib_martigny.reasons_not_to_delete()
-    assert 'links' not in reasons
+    can, reasons = holding_lib_martigny.can_delete
+    assert can
+    assert reasons == {}
 
     assert holding_lib_martigny.document_pid == document.pid
     assert holding_lib_martigny.circulation_category_pid == \

tests/api/ill_requests/test_ill_requests_rest.py

@@ -129,8 +129,9 @@ def test_ill_requests_post_put_delete(client, org_martigny, json_header,
 
 def test_ill_requests_can_delete(client, ill_request_martigny):
     """Test can delete an ill request."""
-    assert ill_request_martigny.can_delete
-    assert not ill_request_martigny.reasons_not_to_delete()
+    can, reasons = ill_request_martigny.can_delete
+    assert can
+    assert reasons == {}
 
 
 def test_filtered_ill_requests_get(

tests/api/item_types/test_item_types_rest.py

@@ -184,14 +184,10 @@ def test_item_types_can_delete(client, item_type_standard_martigny,
                                item_lib_martigny,
                                circulation_policies):
     """Test can delete an item type."""
-    links = item_type_standard_martigny.get_links_to_me()
-    assert 'circ_policies' in links
-    assert 'items' in links
-
-    assert not item_type_standard_martigny.can_delete
-
-    reasons = item_type_standard_martigny.reasons_not_to_delete()
-    assert 'links' in reasons
+    can, reasons = item_type_standard_martigny.can_delete
+    assert not can
+    assert reasons['links']['circ_policies']
+    assert reasons['links']['items']
 
 
 def test_filtered_item_types_get(

tests/api/libraries/test_libraries_rest.py

@@ -170,14 +170,10 @@ def test_library_never_open(lib_sion):
 def test_library_can_delete(lib_martigny, librarian_martigny,
                             loc_public_martigny):
     """Test can delete a library."""
-    links = lib_martigny.get_links_to_me()
-    assert 'locations' in links
-    assert 'patrons' in links
-
-    assert not lib_martigny.can_delete
-
-    reasons = lib_martigny.reasons_not_to_delete()
-    assert 'links' in reasons
+    can, reasons = lib_martigny.can_delete
+    assert not can
+    assert reasons['links']['locations']
+    assert reasons['links']['patrons']
 
 
 def test_filtered_libraries_get(

tests/api/loans/test_loans_rest.py

@@ -242,12 +242,12 @@ def test_due_soon_loans(client, librarian_martigny,
     item = item_lib_martigny
     item_pid = item.pid
     patron_pid = patron_martigny.pid
-
+    can, reasons = item.can_delete
+    assert can
+    assert reasons == {}
+    assert item.available
     assert not get_last_transaction_loc_for_item(item_pid)
-
     assert not item.patron_has_an_active_loan_on_item(patron_martigny)
-    assert item.can_delete
-    assert item.available
 
     from rero_ils.modules.circ_policies.api import CircPolicy
     circ_policy = CircPolicy.provide_circ_policy(

tests/api/locations/test_locations_rest.py

@@ -222,13 +222,9 @@ def test_locations_post_put_delete(client, lib_martigny,
 
 def test_location_can_delete(client, item_lib_martigny, loc_public_martigny):
     """Test can delete a location."""
-    links = loc_public_martigny.get_links_to_me()
-    assert 'items' in links
-
-    assert not loc_public_martigny.can_delete
-
-    reasons = loc_public_martigny.reasons_not_to_delete()
-    assert 'links' in reasons
+    can, reasons = loc_public_martigny.can_delete
+    assert not can
+    assert reasons['links']['items']
 
 
 def test_filtered_locations_get(client, librarian_martigny,

tests/api/notifications/test_notifications_rest.py

@@ -298,12 +298,8 @@ def test_notifications_post_put_delete(
     res = client.get(item_url)
     assert res.status_code == 410
 
-    links = notif.get_links_to_me()
-    assert links == {}
-
-    assert notif.can_delete
-
-    reasons = notif.reasons_not_to_delete()
+    can, reasons = notif.can_delete
+    assert can
     assert reasons == {}
 
     notif.delete(dbcommit=True, delindex=True)

tests/api/organisations/test_organisations_rest_api.py

@@ -88,13 +88,9 @@ def test_organisation_secure_api_update(client, json_header, org_martigny,
 
 def test_location_can_delete(client, org_martigny, lib_martigny):
     """Test can delete an organisation."""
-    links = org_martigny.get_links_to_me()
-    assert 'libraries' in links
-
-    assert not org_martigny.can_delete
-
-    reasons = org_martigny.reasons_not_to_delete()
-    assert 'links' in reasons
+    can, reasons = org_martigny.can_delete
+    assert not can
+    assert reasons['links']['libraries']
 
 
 def test_organisation_secure_api(client, json_header, org_martigny,

tests/api/patron_transaction_events/test_patron_transaction_events_rest.py

@@ -171,13 +171,9 @@ def test_patron_transaction_event_utils_shortcuts(
         client, patron_transaction_overdue_event_martigny,
         loan_overdue_martigny):
     """Test patron transaction utils and shortcuts."""
-    links = patron_transaction_overdue_event_martigny.get_links_to_me()
-    assert not links
-
-    assert patron_transaction_overdue_event_martigny.can_delete
-
-    reasons = patron_transaction_overdue_event_martigny.reasons_not_to_delete()
-    assert not reasons
+    can, reasons = patron_transaction_overdue_event_martigny.can_delete
+    assert can
+    assert reasons == {}
 
     assert patron_transaction_overdue_event_martigny.patron_pid == \
         loan_overdue_martigny.patron_pid

tests/api/patron_transactions/test_patron_transactions_rest.py

@@ -195,13 +195,9 @@ def test_patron_transaction_photocopy_create(
 def test_patron_transaction_shortcuts_utils(
         client, patron_transaction_overdue_martigny, loan_overdue_martigny):
     """Test patron transaction shortcuts and utils."""
-    links = patron_transaction_overdue_martigny.get_links_to_me()
-    assert 'events' in links
-
-    assert not patron_transaction_overdue_martigny.can_delete
-
-    reasons = patron_transaction_overdue_martigny.reasons_not_to_delete()
-    assert 'links' in reasons
+    can, reasons = patron_transaction_overdue_martigny.can_delete
+    assert not can
+    assert reasons['links']['events']
 
     assert patron_transaction_overdue_martigny.loan_pid == \
         loan_overdue_martigny.pid

tests/api/patron_types/test_patron_types_rest.py

@@ -186,15 +186,10 @@ def test_patron_types_can_delete(client, patron_type_children_martigny,
                                  patron_martigny,
                                  circulation_policies):
     """Test can delete a patron type."""
-    patron_type = patron_type_children_martigny
-    links = patron_type.get_links_to_me()
-    assert 'circ_policies' in links
-    assert 'patrons' in links
-
-    assert not patron_type.can_delete
-
-    reasons = patron_type.reasons_not_to_delete()
-    assert 'links' in reasons
+    can, reasons = patron_type_children_martigny.can_delete
+    assert not can
+    assert reasons['links']['circ_policies']
+    assert reasons['links']['patrons']
 
 
 def test_filtered_patron_types_get(

tests/api/patrons/test_patrons_views.py

@@ -55,13 +55,9 @@ def test_patron_can_delete(client, librarian_martigny,
     assert res.status_code == 200
     loan_pid = data.get('action_applied')[LoanAction.REQUEST].get('pid')
 
-    links = patron_martigny.get_links_to_me()
-    assert 'loans' in links
-
-    assert not patron_martigny.can_delete
-
-    reasons = patron_martigny.reasons_not_to_delete()
-    assert 'links' in reasons
+    can, reasons = patron_martigny.can_delete
+    assert not can
+    assert reasons['links']['loans']
 
     res, data = postdata(
         client,

tests/api/vendors/test_vendors.py

@@ -75,9 +75,8 @@ def test_vendors_can_delete(
         client, vendor_martigny, acq_order_fiction_martigny,
         acq_invoice_fiction_martigny, holding_lib_martigny_w_patterns):
     """Test can delete a vendor with a linked acquisition order."""
-    assert not vendor_martigny.can_delete
-
-    reasons = vendor_martigny.reasons_not_to_delete()
+    can, reasons = vendor_martigny.can_delete
+    assert not can
     assert reasons['links']['acq_orders']
     assert reasons['links']['acq_invoices']
     assert reasons['links']['holdings']

tests/ui/circ_policies/test_circ_policies_api.py

@@ -107,24 +107,23 @@ def test_circ_policy_exist_name_and_organisation_pid(
 def test_circ_policy_can_not_delete(circ_policy_default_martigny,
                                     circ_policy_short_martigny):
     """Test can not delete a policy."""
-    others = circ_policy_default_martigny.reasons_to_keep()
-    assert others['is_default']
-    assert not circ_policy_default_martigny.can_delete
+    can, reasons = circ_policy_default_martigny.can_delete
+    assert not can
+    assert reasons['others']['is_default']
 
-    others = circ_policy_short_martigny.reasons_to_keep()
-    assert 'is_default' not in others
-    assert circ_policy_short_martigny.can_delete
+    can, reasons = circ_policy_short_martigny.can_delete
+    assert can
+    assert reasons == {}
 
 
 def test_circ_policy_can_delete(app, circ_policy_martigny_data_tmp):
     """Test can delete a policy."""
     circ_policy_martigny_data_tmp['is_default'] = False
     cipo = CircPolicy.create(circ_policy_martigny_data_tmp, delete_pid=True)
-    assert cipo.get_links_to_me() == {}
-    assert cipo.can_delete
 
-    reasons = cipo.reasons_not_to_delete()
-    assert 'links' not in reasons
+    can, reasons = cipo.can_delete
+    assert can
+    assert reasons == {}
 
     with mock.patch(
             'rero_ils.modules.circ_policies.api.CircPolicy.get_links_to_me'