From b09523cf1a29680b0b979d07fe68b6c9897f667e Mon Sep 17 00:00:00 2001 From: Aly Badr Date: Wed, 1 Dec 2021 13:43:27 +0100 Subject: [PATCH] items: delete checked-in provisional items * Adds a daily task to delete checked-in provisional items having not associated fees. Co-Authored-by: Aly Badr * Migration instructions: This task need to be enabled for the production instance - invenio reroils scheduler enable_tasks -n delete-provisional-items -v --- rero_ils/config.py | 5 +++ rero_ils/modules/items/tasks.py | 34 ++++++++++++++++ rero_ils/modules/items/utils.py | 29 ++++++++++++++ scripts/setup | 2 +- tests/api/holdings/test_provisional_items.py | 42 ++++++++++++++++++-- 5 files changed, 108 insertions(+), 4 deletions(-) diff --git a/rero_ils/config.py b/rero_ils/config.py index ab9a283a15..46fcbef467 100644 --- a/rero_ils/config.py +++ b/rero_ils/config.py @@ -385,6 +385,11 @@ def _(x): # Every week on Saturday at 22:22 UTC, 'enabled': False }, + 'delete-provisional-items': { + 'task': 'rero_ils.modules.items.tasks.delete_provisional_items', + 'schedule': crontab(minute=0, hour=3), # Every day at 03:00 UTC, + 'enabled': False, + }, # 'mef-harvester': { # 'task': 'rero_ils.modules.apiharvester.tasks.harvest_records', # 'schedule': timedelta(minutes=60), diff --git a/rero_ils/modules/items/tasks.py b/rero_ils/modules/items/tasks.py index ced157d3bd..2ddbe23473 100644 --- a/rero_ils/modules/items/tasks.py +++ b/rero_ils/modules/items/tasks.py @@ -23,11 +23,45 @@ from flask import current_app from .api import Item +from .utils import get_provisional_items_pids_candidate_to_delete from ..api import IlsRecordError from ..holdings.api import Holding from ..utils import extracted_data_from_ref, set_timestamp +@shared_task() +def delete_provisional_items(verbose=True): + """Delete checked-in provisional items. + + For the list of candidates item pids to delete, this method tries to delete + each item. The item will not be deleted if the method `can_delete` is True. + The reasons not to delete a provisional item are the same as other items: + 1. no active loans + 2. no fees + + :param verbose: is the task should be verbose. + """ + count_items, deleted_items = 0, 0 + for count_items, item_pid in enumerate( + get_provisional_items_pids_candidate_to_delete(), 1): + try: + item = Item.get_record_by_pid(item_pid) + item.delete(item, dbcommit=True, delindex=True) + deleted_items += 1 + except IlsRecordError.NotDeleted: + pass + except Exception as error: + current_app.logger.error(error) + + msg_dict = { + 'number_of_candidate_items_to_delete': count_items, + 'numner_of_deleted_items': deleted_items + } + set_timestamp('claims-creation', **msg_dict) + + return msg_dict + + @shared_task def process_late_claimed_issues( expected_issues_to_late=True, create_first_claim=True, diff --git a/rero_ils/modules/items/utils.py b/rero_ils/modules/items/utils.py index 83469744cd..cafa9497d8 100644 --- a/rero_ils/modules/items/utils.py +++ b/rero_ils/modules/items/utils.py @@ -17,7 +17,10 @@ """Item utils.""" + +from rero_ils.modules.items.models import ItemStatus, TypeOfItem from rero_ils.modules.locations.api import LocationsSearch +from rero_ils.modules.patron_transactions.api import PatronTransactionsSearch def item_pid_to_object(item_pid): @@ -85,3 +88,29 @@ def exists_available_item(items=[]): if item.available: return True return False + + +def get_provisional_items_pids_candidate_to_delete(): + """Returns checked-in provisional items pids. + + Returns list of candidate provisional items pids to delete, based on the + status of the item. Filtering by the status `ItemStatus.ON_SHELF` removes + items with active loans. in addition, remove items with active fees. + :return an item pid generator. + """ + from rero_ils.modules.items.api import ItemsSearch + + # query ES index for open fees + query_fees = PatronTransactionsSearch()\ + .filter('term', status='open')\ + .filter('range', total_amount={'gt': 0})\ + .source('item') + # list of item pids with open fees + item_pids_with_fees = [hit.item.pid for hit in query_fees.scan()] + query = ItemsSearch()\ + .filter('term', type=TypeOfItem.PROVISIONAL) \ + .filter('terms', status=[ItemStatus.ON_SHELF]) \ + .exclude('terms', pid=item_pids_with_fees)\ + .source('pid') + for hit in query.scan(): + yield hit.pid diff --git a/scripts/setup b/scripts/setup index 72dc12cd15..82d5395aae 100755 --- a/scripts/setup +++ b/scripts/setup @@ -524,7 +524,7 @@ then else eval ${PREFIX} invenio reroils scheduler enable_tasks -n scheduler-timestamp -n bulk-indexer -n anonymize-loans -n claims-creation -n accounts -n clear_and_renew_subscriptions -n collect-stats -v eval ${PREFIX} invenio reroils scheduler enable_tasks -n notification-creation -n notification-dispatch-availability -n notification-dispatch-recall -n find-contribution -v - eval ${PREFIX} invenio reroils scheduler enable_tasks -n cancel-expired-request -v + eval ${PREFIX} invenio reroils scheduler enable_tasks -n cancel-expired-request -n delete-provisional-items -v info_msg "For ebooks harvesting run:" msg "\tinvenio reroils oaiharvester harvest -n ebooks -a max=100 -q" fi diff --git a/tests/api/holdings/test_provisional_items.py b/tests/api/holdings/test_provisional_items.py index b8ab32cf21..14c3666dd9 100644 --- a/tests/api/holdings/test_provisional_items.py +++ b/tests/api/holdings/test_provisional_items.py @@ -20,14 +20,20 @@ from __future__ import absolute_import, print_function +from datetime import datetime, timezone + from flask import url_for from invenio_accounts.testutils import login_user_via_session from utils import get_json, postdata from rero_ils.modules.items.api import Item from rero_ils.modules.items.models import ItemStatus, TypeOfItem +from rero_ils.modules.items.tasks import delete_provisional_items +from rero_ils.modules.items.utils import \ + get_provisional_items_pids_candidate_to_delete from rero_ils.modules.loans.api import Loan from rero_ils.modules.loans.models import LoanAction, LoanState +from rero_ils.modules.patron_transactions.api import PatronTransaction from rero_ils.modules.utils import get_ref_for_pid @@ -102,7 +108,8 @@ def test_provisional_items_creation(client, document, org_martigny, def test_holding_requests(client, patron_martigny, loc_public_martigny, circulation_policies, librarian_martigny, - holding_lib_martigny_w_patterns, lib_martigny): + holding_lib_martigny_w_patterns, lib_martigny, + item_lib_martigny, org_martigny): """Test holding patron request.""" login_user_via_session(client, patron_martigny.user) holding = holding_lib_martigny_w_patterns @@ -171,8 +178,6 @@ def test_holding_requests(client, patron_martigny, loc_public_martigny, assert res.status_code == 200 item = Item.get_record_by_pid(item.pid) assert item.status == ItemStatus.ON_SHELF - # TODO: add additional tests for the task to delete provisional items with - # no active loans. # test requests made by a librarian # test fails when there are missing parameters @@ -235,3 +240,34 @@ def test_holding_requests(client, patron_martigny, loc_public_martigny, assert item_2.holding_pid == holding.pid assert item_2.get('enumerationAndChronology') == description assert item_2.pid != item.pid + + all_item_pids = [pid for pid in Item.get_all_pids()] + assert all_item_pids + + # test delete provisional items with no active fees/loans + report = delete_provisional_items() + assert report.get('numner_of_deleted_items') + assert report.get('number_of_candidate_items_to_delete') + # assert that not deleted items are either having loans/fees or not + # provisional items + left_item_pids = [pid for pid in Item.get_all_pids()] + assert left_item_pids + for pid in left_item_pids: + record = Item.get_record_by_pid(pid) + can, _ = record.can_delete + assert not can or record.get('type') != TypeOfItem.PROVISIONAL + # item_2 has pending loans then it should not be removed + assert item_2.pid in left_item_pids + assert item_2.pid in get_provisional_items_pids_candidate_to_delete() + # add fee to item_2 and make sure it will not be candidate at the deletion. + data = { + 'loan': {'$ref': get_ref_for_pid('loanid', loan_2.pid)}, + 'patron': {'$ref': get_ref_for_pid('patrons', patron_martigny.pid)}, + 'organisation': {'$ref': get_ref_for_pid('org', org_martigny.pid)}, + 'status': 'open', + 'total_amount': 0.6, + 'type': 'overdue', + 'creation_date': datetime.now(timezone.utc).isoformat() + } + PatronTransaction.create(data, dbcommit=True, reindex=True) + assert item_2.pid not in get_provisional_items_pids_candidate_to_delete()