Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

transition: implement extension of a loan #55

Merged
merged 1 commit into from
Aug 17, 2018
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
19 changes: 14 additions & 5 deletions invenio_circulation/config.py
Original file line number Diff line number Diff line change
Expand Up @@ -15,11 +15,13 @@
from .search import LoansSearch
from .transitions.transitions import CreatedToItemOnLoan, CreatedToPending, \
ItemAtDeskToItemOnLoan, ItemInTransitHouseToItemReturned, \
ItemOnLoanToItemInTransitHouse, ItemOnLoanToItemReturned, \
PendingToItemAtDesk, PendingToItemInTransitPickup
from .utils import get_default_loan_duration, is_item_available, \
is_loan_duration_valid, item_exists, item_location_retriever, \
patron_exists
ItemOnLoanToItemInTransitHouse, ItemOnLoanToItemOnLoan, \
ItemOnLoanToItemReturned, PendingToItemAtDesk, \
PendingToItemInTransitPickup
from .utils import get_default_extension_duration, \
get_default_extension_max_count, get_default_loan_duration, \
is_item_available, is_loan_duration_valid, item_exists, \
item_location_retriever, patron_exists

_CIRCULATION_LOAN_PID_TYPE = 'loan_pid'
"""."""
Expand Down Expand Up @@ -72,6 +74,8 @@
dict(dest='ITEM_RETURNED', transition=ItemOnLoanToItemReturned),
dict(dest='ITEM_IN_TRANSIT_TO_HOUSE',
transition=ItemOnLoanToItemInTransitHouse),
dict(dest='ITEM_ON_LOAN', transition=ItemOnLoanToItemOnLoan,
trigger='extend'),
dict(dest='CANCELLED', trigger='cancel')
],
'ITEM_IN_TRANSIT_TO_HOUSE': [
Expand Down Expand Up @@ -102,6 +106,11 @@
duration_validate=is_loan_duration_valid,
item_available=is_item_available
),
extension=dict(
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For the extension policies, I noticed that the initialisation of it, happens at utils.py, and it is a static one. I would suggest to replace the functions with the actual values here. It is going to be cleaner, and if anyone needs to override it, they can do it in place. Either if there was some logic in the calculation of initial values I think it should be under a config property i.e. CIRCULATION_EXTENSION_CONDITION.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See comment: #55 (comment)

from_end_date=True,
duration_default=get_default_extension_duration,
max_count=get_default_extension_max_count
),
)
"""."""

Expand Down
44 changes: 43 additions & 1 deletion invenio_circulation/transitions/transitions.py
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@

"""Invenio Circulation custom transitions."""

from datetime import timedelta
from datetime import datetime, timedelta

from flask import current_app
from invenio_db import db
Expand Down Expand Up @@ -59,6 +59,34 @@ def _update_document_pending_request_for_item(item_pid):
# TODO: index loan again?


def _ensure_valid_extension(loan):
"""Validate end dates for a extended loan."""
get_extension_max_count = current_app.config['CIRCULATION_POLICIES'][
'extension']['max_count']
extension_max_count = get_extension_max_count(loan)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks like there is a mix of concerns, extension_max_count is a policy and get_extention_max_count takes loan as a parameter. If loan has anything to do with it extention_max_count could be a property of it.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is the same as the checkout policies. It enables the calculation of future complex policies.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we want this policy to be overriden or extendable?

Copy link
Contributor

@jma jma Aug 7, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@zzacharo extendable for sure! For example, extension_max_count will probably depends on patron type or item type. This mechanism should be also added to the request.


extension_count = loan.get('extension_count', 0)
extension_count += 1
if extension_count > extension_max_count + 1:
msg = 'Max extension count reached `{0}`'.format(extension_max_count)
raise TransitionConstraintsViolation(msg=msg)

loan['extension_count'] = extension_count

get_extension_duration = current_app.config['CIRCULATION_POLICIES'][
'extension']['duration_default']
number_of_days = get_extension_duration(loan)
get_extension_from_end_date = current_app.config[
'CIRCULATION_POLICIES']['extension']['from_end_date']

end_date = parse_date(loan['end_date'])
if not get_extension_from_end_date:
end_date = loan.get('transaction_date')

end_date += timedelta(days=number_of_days)
loan['end_date'] = end_date.isoformat()


class CreatedToPending(Transition):
"""Action to request to loan an item."""

Expand Down Expand Up @@ -165,6 +193,20 @@ def after(self, loan):
super(ItemAtDeskToItemOnLoan, self).after(loan)


class ItemOnLoanToItemOnLoan(Transition):
"""Extend action to perform a item loan extension."""

def before(self, loan, **kwargs):
"""Validate extension action."""
super(ItemOnLoanToItemOnLoan, self).before(loan, **kwargs)

_ensure_valid_extension(loan)

def after(self, loan):
"""."""
super(ItemOnLoanToItemOnLoan, self).after(loan)


class ItemOnLoanToItemInTransitHouse(Transition):
"""Check-in action when returning an item not to its belonging location."""

Expand Down
10 changes: 10 additions & 0 deletions invenio_circulation/utils.py
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,16 @@ def get_default_loan_duration(loan):
return 30


def get_default_extension_duration(loan):
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we move the initial values to config.py these can be removed.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I spoke to the librarians and they're saying it should be possible to calculate these values similarly as for the checkout

"""Return a default extension duration in number of days."""
return 30


def get_default_extension_max_count(loan):
"""Return a default extensions max count."""
return float("inf")


def is_loan_duration_valid(loan):
"""Validate the loan duration."""
return loan['end_date'] > loan['start_date'] and \
Expand Down
56 changes: 56 additions & 0 deletions tests/test_loan_transitions.py
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@

import mock
import pytest
from flask import current_app
from helpers import SwappedConfig, SwappedNestedConfig

from invenio_circulation.api import Loan, is_item_available
Expand Down Expand Up @@ -60,6 +61,61 @@ def test_loan_request(loan_created, db, params):
assert loan['state'] == 'PENDING'


def test_loan_extend(loan_created, db, params,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we are testing more than aspects in this test, could be split in more.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it is better to have one test for the extension even if we test two aspects of it. Otherwise we have to reinit the loan a second time to test.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it would be really helpful to put some comments in order to follow the scenario that is tested. Just for the future us to be able to understand really quick the test.

Copy link
Contributor Author

@rerowep rerowep Aug 7, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@zzacharo Yes this will be very helpful and I think this will be added with the task Write docstrings and documentation #30

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The two cases should be tested from_end_date == True and from_end_date == False. It can be in two different test functions.

mock_is_item_available):
"""Test loan extend action."""

def get_max_count_1(loan):
return 1

loan = current_circulation.circulation.trigger(
loan_created, **dict(params, trigger='checkout')
)
db.session.commit()
end_date = parse_date(loan['end_date'])

loan = current_circulation.circulation.trigger(
loan, **dict(params, trigger='extend')
)
db.session.commit()
new_end_date = parse_date(loan['end_date'])
assert new_end_date == end_date + timedelta(days=30)
assert loan['extension_count'] == 1
loan = current_circulation.circulation.trigger(
loan, **dict(params, trigger='extend')
)
db.session.commit()

# test to manny extensions
current_app.config['CIRCULATION_POLICIES']['extension'][
'max_count'] = get_max_count_1
with pytest.raises(TransitionConstraintsViolation):
loan = current_circulation.circulation.trigger(
loan, **dict(params, trigger='extend')
)


def test_loan_extend_from_enddate(loan_created, db, params,
mock_is_item_available):
"""Test loan extend action from transaction date."""

loan = current_circulation.circulation.trigger(
loan_created, **dict(params, trigger='checkout')
)
db.session.commit()
extension_date = parse_date(loan.get('transaction_date'))
current_app.config['CIRCULATION_POLICIES']['extension'][
'from_end_date'] = False

loan = current_circulation.circulation.trigger(
loan, **dict(params, trigger='extend')
)
db.session.commit()
new_end_date = parse_date(loan['end_date'])
assert new_end_date == extension_date + timedelta(days=30)
assert loan['extension_count'] == 1


def test_cancel_action(loan_created, db, params,
mock_is_item_available):
"""Test should pass when calling `cancel` from `ITEM_ON_LOAN`."""
Expand Down