[FOLIO-1101] Move Renewal Due Date Logic to the Server Created: 02/Mar/18 Updated: 12/Nov/18 Resolved: 09/Jul/18 |
|
| Status: | Closed |
| Project: | FOLIO |
| Components: | None |
| Affects versions: | None |
| Fix versions: | None |
| Type: | Umbrella | Priority: | P2 |
| Reporter: | Cate Boerema (Inactive) | Assignee: | Marc Johnson |
| Resolution: | Done | Votes: | 0 |
| Labels: | core, sprint35, sprint36, sprint37, sprint41 | ||
| Remaining Estimate: | Not Specified | ||
| Time Spent: | Not Specified | ||
| Original estimate: | Not Specified | ||
| Attachments: |
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| Issue links: |
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| Sprint: | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| Tester Assignee: | Adam Shire | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| Description |
|
As I understand it, we have implemented the logic for calculating renewal due dates for based on loan policy in the front end. We need to move the logic to the back end so it can be used in other systems such as discovery services which will integrate with FOLIO via API. Stories that have been added (some are complete, others aren't):
|
| Comments |
| Comment by Cate Boerema (Inactive) [ 02/Mar/18 ] | ||||||||||||||||||||||||||||||||||||||||
|
Jakub Skoczen, I created this based on our conversation but I didn't know which project it should go in. I think it's a pretty high priority. | ||||||||||||||||||||||||||||||||||||||||
| Comment by Michal Kuklis [ 05/Mar/18 ] | ||||||||||||||||||||||||||||||||||||||||
|
Hey everyone here are the current API calls we use in order to perform a checkout:
| ||||||||||||||||||||||||||||||||||||||||
| Comment by Michal Kuklis [ 05/Mar/18 ] | ||||||||||||||||||||||||||||||||||||||||
|
I think ideally it would be great to just do #1 and #7 and the POST circulation/loans should return an error if the loan validation doesn't pass. Also it would be nice if the dueDate for the new loan was calculated on the server. | ||||||||||||||||||||||||||||||||||||||||
| Comment by Jakub Skoczen [ 06/Mar/18 ] | ||||||||||||||||||||||||||||||||||||||||
|
Marc Johnson shale99 The list from Michal Kuklis above is impressive. I would also add to this optional first steps when you check out an item "as proxy": 1. Given a user checks out proxy, validate the user-proxy relationship before allowing to create a loan. | ||||||||||||||||||||||||||||||||||||||||
| Comment by Jakub Skoczen [ 22/Mar/18 ] | ||||||||||||||||||||||||||||||||||||||||
|
Marc Johnson I am preliminarily scheduling this and some other issues for the next (I'll be gone). | ||||||||||||||||||||||||||||||||||||||||
| Comment by Cate Boerema (Inactive) [ 09/Apr/18 ] | ||||||||||||||||||||||||||||||||||||||||
|
Jakub Skoczen, is this on your radar for the backend guys? Does the story need to be moved to another project? | ||||||||||||||||||||||||||||||||||||||||
| Comment by Marc Johnson [ 30/Apr/18 ] | ||||||||||||||||||||||||||||||||||||||||
|
Cate Boerema Jakub Skoczen I cannot seem to see [UIU-405]. Can you help me find it, has it been removed / hidden? | ||||||||||||||||||||||||||||||||||||||||
| Comment by Cate Boerema (Inactive) [ 30/Apr/18 ] | ||||||||||||||||||||||||||||||||||||||||
|
Hmmm. Not sure what that one was. I'll remove it from the story name and put the actual related stories in the body. | ||||||||||||||||||||||||||||||||||||||||
| Comment by Marc Johnson [ 30/Apr/18 ] | ||||||||||||||||||||||||||||||||||||||||
|
Cate Boerema Thanks | ||||||||||||||||||||||||||||||||||||||||
| Comment by Michal Kuklis [ 30/Apr/18 ] | ||||||||||||||||||||||||||||||||||||||||
|
Marc Johnson here is what we are currently doing during renew (Most of this logic is here: https://github.com/folio-org/ui-users/blob/master/withRenew.js#L73-L84)
Also here are the errors:
| ||||||||||||||||||||||||||||||||||||||||
| Comment by Marc Johnson [ 06/Jun/18 ] | ||||||||||||||||||||||||||||||||||||||||
|
Hi Emma Boettcher, Cate Boerema, Jakub Skoczen and Michal Kuklis Apologies for my (now deleted) comments yesterday. This comment should supersede all of them and hopefully be more coherent. Please ask if something does not make sense. Truncation of Due Date by Due Date Limit SchedulesI couldn’t find a scenario for truncation by the loan due date schedules within
I’ve assumed it behaves very similarly to with the alternate due date limit schedules, please let me know if this is incorrect. Alternate PeriodI’d like to check my understanding of how alternate periods work for rolling profile loan policies. When a different period has been chosen it supersedes the original loan period during a renewal (
When a different period has been chosen and it has been limited by alternate due date schedules, the due date should be truncated (
If a different period has been chosen, but alternate due date limit schedules have not been chosen, should it use the loan (original) due date limit schedule, or not apply one? At the moment I have assumed that the period and the (optional) due date limit schedules are effectively a pair, so if an alternate isn’t chosen for the different period, then it is not due date limited (even if the loan period is limited). Does that make sense? Prevent Renewal Due To Same or Earlier Due DateAt the moment, the UI converts both the current and proposed due date to the start of the (local) day when making the comparison to see if a proposed due date would be the same or before the existing due date. As the API currently does not know what the beginning of the (local) day is, it compares the current due date and the proposed due date without moving them to the beginning of the day. This means the behaviour has likely changed slightly. A renewal for a rolling policy which renews from the system date will likely be rejected by the current UI implementation when it is triggered on the same day as the check out. This would likely be accepted by the API, as a small extension to the due date. Is it ok for this behaviour to change until the discussions around the time aspects of due dates progress? Adopting the existing behaviour would mean the API needing to know when the local beginning of the day is (and so would mean fetching this from configuration and converting due dates between locales), so I would prefer to defer undertaking this until we know more. Not RenewableI haven’t seen an issue for rejecting renewals if the policy states that renewals are not allowed, have I missed it? What would be an appropriate message for that circumstance? Multiple Validation MessagesAt the moment the UI presents multiple renewal validation messages as a single statement (as per
For example (see attached screenshot): Loan Policy SectionThe loan policy used for the renewal attempt could be different to the loan policy used for the check out, or for the last renewal, if the loan rules have changed. We currently only update the loan policy associated with the loan when a successful action is taken. For the “See <LoanPolicyAssociatedwLoan> for more information” section”, is the LoanPolicyAssociatedwLoan the loan policy that was used for that renewal attempt, or the loan policy last used to successfully change the loan (I am assuming the former at the moment)? Construction of MessageWhen I first looked at this, I anticipated the API responding with multiple messages (currently called validation errors, see example at end). An aspect that potentially makes this more complicated is the loan policy section of the message. I think we have a couple of options for approaching this:
The first option is highly coupled with the current UI and may not satisfy the needs of other clients, e.g. for the patron facing service EBSCO are making (CIRC-100). The second option provides flexibility however burdens the current UI with an additional lookup. Option 3 avoids that additional lookup. This seems like a reasonable compromise (in the sense that this property does not exist in a record) given that it is quite likely other clients will want to use the loan policy name. Does that sound sensible? Have I missed any aspects of this? Validation errors currently look like: "errors" : [ { "message" : "Loan has reached it's maximum number of renewals", "parameters" : [ { "key" : "loanPolicyId", "value" : "73521907-9875-4909-8b46-6691c5c39d07" } ] }, { "message" : "Renewal at this time would not change the due date", "parameters" : [ { "key" : "loanPolicyId", "value" : "73521907-9875-4909-8b46-6691c5c39d07" } ] }] | ||||||||||||||||||||||||||||||||||||||||
| Comment by Michal Kuklis [ 06/Jun/18 ] | ||||||||||||||||||||||||||||||||||||||||
|
Marc Johnson I do like the option 3 the most too. We will need to refactor some parts on the UI to handle there errors differently but I think that's ok. Option 1 works for now but I feel it's too coupled. It would be nice to avoid additional lookup so option 2 is also not ideal. Also the proposed error structure follows what we already have in the checkout API so I think this will work well here too. | ||||||||||||||||||||||||||||||||||||||||
| Comment by Emma Boettcher [ 06/Jun/18 ] | ||||||||||||||||||||||||||||||||||||||||
|
Cate Boerema may have better answers to some of these questions, but here's my best shot: Truncation of Due Date: does
Alternate Period: Yes, I think it makes sense to have to deliberately choose a due date limit if you would like one to be applied for an alternate due date. (If the system assumes the original due date limit should be used, then there's no way in the current Edit Loan Policy for the user to indicate that they would not like that to happen) Prevent Renewal Due to Same or Earlier Due Date: Yes, I think it's fine to leave this until the time question is answered. Not Renewable: From my cursory glance I don't think you missed it. Possible message: "Items with this loan policy cannot be renewed" Loan Policy Section: I'm not sure I understand this. On Monday, patron checks out book with policy X. On Tuesday, staff change the policy for that item to Policy Y. Patron renews the book on Wednesday and it succeeds according to Policy X, but then applies Policy Y to loan. Patron tries to renew the book again on Friday, and it fails according to Policy Y without checking Policy X - so then the system should tell them to look at Policy Y, not Policy X. Have I understood the sequence of events correctly? | ||||||||||||||||||||||||||||||||||||||||
| Comment by Cate Boerema (Inactive) [ 07/Jun/18 ] | ||||||||||||||||||||||||||||||||||||||||
Because the label was defined as "Alternate fixed due date schedule (due date limit) for renewals", I do think the intention was that, if nothing was selected, it would be truncated using the original truncation (as defined in Fixed due date schedule (due date limit)). This is the meaning the label "Alternate..." is used to convey elsewhere in the loan policy form. That said, I agree with Emma's point that it would be nice to offer the option to have no truncation for renewals. I'd suggest we add a value to the "Alternate fixed due date schedule (due date limit) for renewals" menu for "No due date limit". I can do that now (
Agreed, but let's make sure we clearly note this for the testers. I guess once this work is done, Marc, we should move all the stories that were previously tested after the logic was built into the UI back into In Review for re-testing. Let's just make sure we don't put these In Review until after the questions have been answered and the code has been updated:
BTW, are there plans to get the questions answered? Was that on my plate? I can't recall what the open questions were at this point.
Correct. This story hasn't been written. I agree with Emma's recommended message but I don't think we can just put requirements in comments. They need to go into user stories so they can be tested. Marc Johnson, there are a good number of loan policy settings that still don't have user stories. Can you work around that for this story and only focus on those that do?
Correct. It should be the former. | ||||||||||||||||||||||||||||||||||||||||
| Comment by Marc Johnson [ 07/Jun/18 ] | ||||||||||||||||||||||||||||||||||||||||
|
Cate Boerema Emma Boettcher Thank you for the answers to my questions, they are really helpful in expanding my general understanding of the loans policy and in trying to confirm the behaviour I've implemented.
That said, I agree with Emma's point that it would be nice to offer the option to have no truncation for renewals. I'd suggest we add a value to the "Alternate fixed due date schedule (due date limit) for renewals" menu for "No due date limit". I can do that now (
Thank you for explaining the distinction between different and alternate, that is helpful context. Thank you for raising
Prior to
Should the meaning of alternate be respected, so that if there is a due date limit defined in the loan policy but not in the different renewal period, then the loan policy limit should be applied (and it isn't possible to have a non-limited renewal if the loan is limited)? Or should it not apply any limit if different period is selected but no schedule chosen.
Agreed, we need to make these distinctions clear for the testers. I think whether we can either signal the need to re-test by re-opening the previous issue or with a fresh one (for the migration of the UI to the API). Should we block
Cate Boerema Are the questions you refer to around the time when an item is due back? As far as I'm aware, I don't believe we've answered the questions about when on a day an item loaned on a daily, weekly or monthly period is due back. I believe it was intended to be discussed in the SIG and was deferred, was it discussed at a subsequent meeting?
Cate Boerema yeah, as far as possible I'm trying to ignore the elements of the policy where there aren't issues. I'm happy to wait for issues which define the scenarios where possible. I think whether an item is renewable might be a special case, where we need a fallback behaviour, as I wouldn't expect there to be any renewals policy for interpret for a loans policy that states non-renewal. Prior to the issue being written, what would we expect the behaviour of the system to be when a renewal is attempted using a non-renewable loans policy? And finally, a really silly question but it crossed my mind, I wanted to check. A renewal should use the policy decided by the loan rules for that item and patron (which is what I've done, and what I infer from the answer above) and not the loan policy applied at point of check out? | ||||||||||||||||||||||||||||||||||||||||
| Comment by Cate Boerema (Inactive) [ 07/Jun/18 ] | ||||||||||||||||||||||||||||||||||||||||
Hmmm. If this is a big deal, I think we can deprioritize
The expected behavior should be what was defined in scenario 2 from
I'd rather use the existing stories (or clones of them) because they outline all the expected behavior. I guess there is no point in continuing testing on
I remember now. Emma Boettcher, were you able to raise this with the Loans group? If not, should we see if we can get it on the agenda for the big SIG today? You had a nice deck on the topic, as I recall.
When renewable = N, you should get a popup letting you know. Ideally we would add a new text field in the loan policy for specifying the text for this popup which would only display when renewable is not selected. There needs to be the ability to override this with the right permissions and specify a due date.
Correct | ||||||||||||||||||||||||||||||||||||||||
| Comment by Marc Johnson [ 07/Jun/18 ] | ||||||||||||||||||||||||||||||||||||||||
|
Thanks Cate Boerema
Apologies for not spotting that this had already been defined, thank you for moving it anyway. I'll make sure the API supports scenario 5 of
For the time being I'll put a simple check in for renewable = N, just to display a message, until we have an issue which describes the needed behaviour more fully. Shall I use "items with this loan policy cannot be renewed" for this interim message? Do we need to add this as a scenario for the testers?
I believe Michal Kuklis is picking up
| ||||||||||||||||||||||||||||||||||||||||
| Comment by Aditya matukumalli [ 07/Jun/18 ] | ||||||||||||||||||||||||||||||||||||||||
|
Cate Boerema Yes, we can wait before testing
| ||||||||||||||||||||||||||||||||||||||||
| Comment by Cate Boerema (Inactive) [ 07/Jun/18 ] | ||||||||||||||||||||||||||||||||||||||||
Yes, I like this message for now.
Yes, I think that's important. Emma Boettcher, can I ask you to add the scenario for this? | ||||||||||||||||||||||||||||||||||||||||
| Comment by Emma Boettcher [ 07/Jun/18 ] | ||||||||||||||||||||||||||||||||||||||||
|
Cate Boerema added it to
| ||||||||||||||||||||||||||||||||||||||||
| Comment by Marc Johnson [ 27/Jun/18 ] | ||||||||||||||||||||||||||||||||||||||||
|
Cate Boerema Jakub Skoczen I've unblocked this, as both versions of the renewal API are in place (
| ||||||||||||||||||||||||||||||||||||||||
| Comment by Cate Boerema (Inactive) [ 28/Jun/18 ] | ||||||||||||||||||||||||||||||||||||||||
|
Works for me. Jakub Skoczen? |