transition: implement extension of a loan #55

rerowep · 2018-08-02T09:18:29Z

* (closes #49)

Signed-off-by: Peter Weber Peter.Weber@rero.ch
Co-authored-by: Igor Milhit Igor.Milhit@rero.ch

topless

I noted a few things down that got my attention, as an initial step to further discussion, I am not fully aware of all the necessities so please feel free to comment further.

topless · 2018-08-02T12:49:41Z

invenio_circulation/transitions/transitions.py

@@ -18,7 +18,7 @@
 from ..utils import parse_date


-def _ensure_valid_loan_duration(loan):
+def _ensure_valid_loan_duration(loan, from_end_date=True):


from_end_date is not used anywhere in the function.

topless · 2018-08-02T13:12:28Z

invenio_circulation/config.py

@@ -90,6 +93,11 @@
        duration_validate=is_loan_duration_valid,
        item_available=is_item_available
    ),
+    extension=dict(


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.

See comment: #55 (comment)

topless · 2018-08-02T13:39:54Z

invenio_circulation/transitions/transitions.py

+    """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)


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.

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

Do we want this policy to be overriden or extendable?

@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.

topless · 2018-08-02T13:43:45Z

invenio_circulation/transitions/transitions.py

+
+    extension_count = loan.get('extension_count', 0)
+    extension_count += 1
+    if extension_max_count != -1 and extension_count > extension_max_count + 1:


This implied logic of infinity is difficult to follow, and logically incorrect, because the extension_count here has to be strictly greater than the max_count increased by one, so its not the max. I would initialise the extension_max_count with a big number.

topless · 2018-08-02T13:48:44Z

invenio_circulation/transitions/transitions.py

+        'CIRCULATION_POLICIES']['extension']['from_end_date']
+    from_end_date = get_extension_from_end_date(loan)
+
+    end_date = parse_date(loan['end_date'])


Could be like

end_date = datetime.now() + timedelta(days=number_of_days) if from_end_date: end_date = parse_date(loan['end_date']) + timedelta(days=number_of_days)

topless · 2018-08-02T13:49:39Z

invenio_circulation/utils.py

@@ -38,6 +38,25 @@ def get_default_loan_duration(loan):
    return 30


+def get_default_extension_duration(loan):


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

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

topless · 2018-08-02T13:50:55Z

tests/test_loan_transitions.py

@@ -54,6 +55,40 @@ def test_loan_request(db, params):
    assert loan['state'] == 'PENDING'


+def test_loan_extend(loan_created, db, params,


I think we are testing more than aspects in this test, could be split in 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.

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.

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

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

jma · 2018-08-09T13:45:32Z

invenio_circulation/config.py

@@ -102,6 +106,11 @@
        duration_validate=is_loan_duration_valid,
        item_available=is_item_available
    ),
+    extension=dict(
+        from_end_date=extend_from_end_date,


duration_default and max_count can be changed by the ILS, but from_en_date is just a boolean. Thus we can replace extend_from_end_date can be removed by a boolean value.

jma · 2018-08-09T13:46:35Z

invenio_circulation/config.py

@@ -102,6 +106,11 @@
        duration_validate=is_loan_duration_valid,
        item_available=is_item_available
    ),
+    extension=dict(
+        from_end_date=extend_from_end_date,
+        duration_default=get_default_extension_duration,


I would rename duration_default by duration. We can do the same for checkout policy.

jma · 2018-08-09T13:47:15Z

invenio_circulation/transitions/transitions.py

+
+    end_date = parse_date(loan['end_date'])
+    if not get_extension_from_end_date(loan):
+        end_date = datetime.now()


It should be replaced by transaction_date.

jma · 2018-08-09T13:47:41Z

invenio_circulation/utils.py

+    return float("inf")
+
+
+def extend_from_end_date(loan):


This can be removed.

jma · 2018-08-09T13:48:29Z

tests/test_loan_transitions.py

@@ -54,6 +55,40 @@ def test_loan_request(db, params):
    assert loan['state'] == 'PENDING'


+def test_loan_extend(loan_created, db, params,


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

* (closes #49) Signed-off-by: Peter Weber <Peter.Weber@rero.ch> Co-authored-by: Igor Milhit <Igor.Milhit@rero.ch>

topless reviewed Aug 2, 2018

View reviewed changes

jma requested changes Aug 9, 2018

View reviewed changes

transition: implement extension of a loan

4e009ed

* (closes #49) Signed-off-by: Peter Weber <Peter.Weber@rero.ch> Co-authored-by: Igor Milhit <Igor.Milhit@rero.ch>

jma approved these changes Aug 9, 2018

View reviewed changes

ntarocco merged commit 21a702c into inveniosoftware:master Aug 17, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

transition: implement extension of a loan #55

transition: implement extension of a loan #55

rerowep commented Aug 2, 2018

topless left a comment

topless Aug 2, 2018

topless Aug 2, 2018

rerowep Aug 6, 2018

topless Aug 2, 2018

rerowep Aug 6, 2018

zzacharo Aug 7, 2018

jma Aug 7, 2018 •

edited

Loading

topless Aug 2, 2018

topless Aug 2, 2018

topless Aug 2, 2018

rerowep Aug 6, 2018

topless Aug 2, 2018

rerowep Aug 6, 2018

zzacharo Aug 7, 2018

rerowep Aug 7, 2018 •

edited

Loading

jma Aug 9, 2018

jma Aug 9, 2018

jma Aug 9, 2018

jma Aug 9, 2018

jma Aug 9, 2018

jma Aug 9, 2018

		@@ -38,6 +38,25 @@ def get_default_loan_duration(loan):
		return 30


		def get_default_extension_duration(loan):

		@@ -54,6 +55,40 @@ def test_loan_request(db, params):
		assert loan['state'] == 'PENDING'


		def test_loan_extend(loan_created, db, params,

transition: implement extension of a loan #55

transition: implement extension of a loan #55

Conversation

rerowep commented Aug 2, 2018

topless left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jma Aug 7, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rerowep Aug 7, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jma Aug 7, 2018 •

edited

Loading

rerowep Aug 7, 2018 •

edited

Loading