The syspref reservemaxpickupdelay says how long a hold is waiting for patron pickup. This could be set at issuingrule level, to let a student of library A 4 days and a teacher of library B 10 days
Created attachment 12130 [details] [review] Bug 8367: Add more granular level for ReservesMaxPickUpDelay This patch adds: - a new column in the issuing rules - a new field issuingrules.holdspickupdelay You can now specify a pickup delay for an hold function of a patron category and/or a item type and/or a library. To test: Check there is no regression with a normal reserve workflow. Add one or more issuingrules. In 4 templates, you can see the max pickup delay for an hold (circ/circulation.tt, circ/waitingreserves.tt, members/moremember.tt, opac-user.tt).
I tried to manually fix the merge errors and ended up getting this error: The 'days' parameter (undef) to DateTime::Duration::new was an 'undef', which is not one of the allowed types: scalar at /usr/lib/perl5/DateTime/Duration.pm line 31 DateTime::Duration::new(undef, 'days', undef) called at /usr/lib/perl5/DateTime.pm line 1640 DateTime::add('DateTime=HASH(0xa1d3494)', 'days', undef) called at /home/oleonard/kohaclone/C4/Reserves.pm line 552 C4::Reserves::GetMaxPickupDate('HASH(0xa11168c)', undef) called at /home/oleonard/kohaclone/circ/waitingreserves.pl line 107 Not sure if this is a real error or whether I made a mistake in merging?
Created attachment 13139 [details] [review] Bug 8367: Add more granular level for ReservesMaxPickUpDelay This patch adds: - a new column in the issuing rules - a new field issuingrules.holdspickupdelay You can now specify a pickup delay for an hold function of a patron category and/or a item type and/or a library. To test: Check there is no regression with a normal reserve workflow. Add one or more issuingrules. In 4 templates, you can see the max pickup delay for an hold (circ/circulation.tt, circ/waitingreserves.tt, members/moremember.tt, opac-user.tt).
(In reply to comment #2) > Not sure if this is a real error or whether I made a mistake in merging? Maybe both :) The merge was not easy to resolve and errors exist in the previous patch.
This patch looks good in all cases except for waitingreserves.pl. Even though my hold is past its pickup window, it is showing in the waiting holds tab, and not in the holds over tab is it should be.
Hi Kyle, What is the value filled for the "Available since-until" column ? I have 2 holds and 1 by tab: first have "19/11/2012 - 26/11/2012", second (over) "28/10/2012 - 04/11/2012". And it is what is expected.
On circulation.pl it shows: Item 1 waiting at Centerville until 11/20/2012 On waitingreserves.pl it shows: 11/19/2012 - 11/26/2012 so it appears to be using the new issue rules setting for circulation.pl, but it's still using the system preference for waitingreserves.pl I have my rule set to 1 day, and my system pref set to 7.
Created attachment 13602 [details] [review] Bug 8367: Add more granular level for ReservesMaxPickUpDelay - Followup
Created attachment 13603 [details] [review] Bug 8367: Add more granular level for ReservesMaxPickUpDelay This patch adds: - a new column in the issuing rules - a new field issuingrules.holdspickupdelay You can now specify a pickup delay for an hold function of a patron category and/or a item type and/or a library. To test: Check there is no regression with a normal reserve workflow. Add one or more issuingrules. In 4 templates, you can see the max pickup delay for an hold (circ/circulation.tt, circ/waitingreserves.tt, members/moremember.tt, opac-user.tt). Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Created attachment 13604 [details] [review] Bug 8367: Add more granular level for ReservesMaxPickUpDelay - Followup
Created attachment 13624 [details] [review] Bug 8367: Add more granular level for ReservesMaxPickUpDelay - Followup Signed-off-by: Jonathan Druart <jonathan.druart@biblibre.com>
(In reply to comment #10) > Created attachment 13604 [details] [review] > Bug 8367: Add more granular level for ReservesMaxPickUpDelay - Followup Good catch Kyle, Thanks! It would be worth having an other SO for these patches
It's been a month without another signer. I think it's time to move these patches to QA.
(In reply to comment #13) > It's been a month without another signer. I think it's time to move these > patches to QA. agreed (and agreed also because now QAers are supposed to test the patch)
QA: Looking at this now. Rebasing it first..
Created attachment 15334 [details] [review] [SIGNED-OFF] Bug 8367: Add more granular level for ReservesMaxPickUpDelay This patch adds: - a new column in the issuing rules - a new field issuingrules.holdspickupdelay You can now specify a pickup delay for an hold function of a patron category and/or a item type and/or a library. To test: Check there is no regression with a normal reserve workflow. Add one or more issuingrules. In 4 templates, you can see the max pickup delay for an hold (circ/circulation.tt, circ/waitingreserves.tt, members/moremember.tt, opac-user.tt). Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Rebased on Feb 15, 2013 (Marcel de Rooy): Merge conflict in opac/opac-user.pl Merge conflict in koha-tmpl/intranet-tmpl/prog/en/modules/circ/circulation.tt Merge conflict in installer/data/mysql/updatedatabase.pl Merge conflict in circ/waitingreserves.pl
Created attachment 15335 [details] [review] [SIGNED-OFF] Bug 8367: Add more granular level for ReservesMaxPickUpDelay - Followup Signed-off-by: Jonathan Druart <jonathan.druart@biblibre.com>
QA Comment: I like the feature! But IMO it needs some more attention before pushing it: Issue while resolving merge conflict in circ/waitingreserves.pl: The dateformat template var has been removed in master (was in the patch) but is still used in one line of template (waitingreserves.pl). Please check. IMO it is confusing that the old pref exists besides the new column in issuing rules. I would suggest simplification by obsoleting pref ReservesMaxPickUpDelay. At this time behavior is not so clear anymore. Note that there is no comment either on the preferences form that this syspref could be overruled by issuing rules. And the other way around: If no rule is found, the pref still holds. Does the user still understand that from the rules form? GetMaxPickupDate: It may be worth to consider if it would be easier to add a maxpickupdate to the reserves table instead of recalculating it "everywhere". GetMaxPickupDelay: Only called by GetMaxPickupDate. A serious issue IMO is the FIXME in CancelExpiredReserves. Instead of calling GetMaxPickupDelay, you just use the pref value! I understand that from performance point of view, but this is "killing" the feature. When using the cron job to remove holds, the new pickup delay should be used not the old one. Note that you could easily resolve this by looking at a maxpickupdate when adding this to the reserve table and calculating it at confirm hold time. Minor representation issue: In the opac user form I see the max pickup date. Would it be consistent to show it on staff catalogue detail too (Holds tab)? Setting to Failed QA, mainly for the cancelling holds issue.
Just another thought: If you would remove the pref, please add a db change to copy it into the new column of the existing issuing rules.
(In reply to comment #18) > QA Comment: > Note that you could easily resolve this by looking at a maxpickupdate when > adding this to the reserve table and calculating it at confirm hold time. Hi Marcel, Thanks for QAing this patch! It is a great idea even though it was not so easy :) A patch is coming.
Created attachment 15369 [details] [review] Bug 8367: Followup Add more granular level for ReservesMaxPickUpDelay This patch removes the ReservesMaxPickUpDelay syspref. The update database script will update the issuingrules table with the correct value before removing it. 1 new column: reserves.maxpickupdate (and for old_reserves). It contains the waitingdate + the corresponding max pickup delay. Each time the waitingdate is modified, this value will be modified too. To test: Play with the reserve workflow (Add, remove, change priority, etc.)
Questions, does this take into account days the library is closed? It would make sense for the expiration day for pickups to skip to the next open day if they land on a holiday. This is something that needed has needed to be addressed for some time now.
(In reply to comment #20) > It is a great idea even though it was not so easy :) Yes, I see. In circulation nothing seems to be easy.. Will try to have a look at your patches again later this week. Just a question from quickly glancing through your code. In CancelExpiredReserves: + my $query = "SELECT * FROM reserves WHERE TO_DAYS( NOW() ) - TO_DAYS( waitingdate ) > maxpickupdate AND found = 'W' AND priority = 0"; Does that work? Looks to me that it doesn't. Shouldn't you just look for maxpickupdate < now?
(In reply to comment #22) > Questions, does this take into account days the library is closed? It would > make sense for the expiration day for pickups to skip to the next open day > if they land on a holiday. This is something that needed has needed to be > addressed for some time now. Looks like that this report does not do that. I agree that it should, but you could say that it is beyond scope here? Followup or additional report? But this report does pave the way. We only have to refine the routine where maxpickupdate is calculated now.
Hm, there is a downside to the new column too - we loose a bit of flexibility. Say a library closes for some time and changes the pickup delay to be longer, so the holds don't go stale - with the patch in effect they would have to do a database update to those holds. It's only one use case I have come up with so far. There might be other disadvantages/advantages of both ways of doing it.
(In reply to comment #23) > Just a question from quickly glancing through your code. In > CancelExpiredReserves: > + my $query = "SELECT * FROM reserves WHERE TO_DAYS( NOW() ) - > TO_DAYS( waitingdate ) > maxpickupdate AND found = 'W' AND priority = 0"; > Does that work? Looks to me that it doesn't. Shouldn't you just look for > maxpickupdate < now? Yes, of course! (new patch coming) (In reply to comment #24) > (In reply to comment #22) > > Questions, does this take into account days the library is closed? It would > > make sense for the expiration day for pickups to skip to the next open day > > if they land on a holiday. This is something that needed has needed to be > > addressed for some time now. > > Looks like that this report does not do that. I agree that it should, but > you could say that it is beyond scope here? Followup or additional report? > But this report does pave the way. We only have to refine the routine where > maxpickupdate is calculated now. Yes, that can be developed later without investing too much effort. (In reply to comment #25) > Hm, there is a downside to the new column too - we loose a bit of > flexibility. Say a library closes for some time and changes the pickup delay > to be longer, so the holds don't go stale - with the patch in effect they > would have to do a database update to those holds. It's only one use case I > have come up with so far. There might be other disadvantages/advantages of > both ways of doing it. You are right. But do you think many librarians often change this value? I don't see how to manage the feature and this use case. We can imagine a script that modifies directly the max pickup date in DB according some variables.
Created attachment 15498 [details] [review] Bug 8367: Followup Add more granular level for ReservesMaxPickUpDelay This patch removes the ReservesMaxPickUpDelay syspref. The update database script will update the issuingrules table with the correct value before removing it. 1 new column: reserves.maxpickupdate (and for old_reserves). It contains the waitingdate + the corresponding max pickup delay. Each time the waitingdate is modified, this value will be modified too. To test: Play with the reserve workflow (Add, remove, change priority, etc.)
(In reply to comment #25) > Hm, there is a downside to the new column too - we loose a bit of > flexibility. Say a library closes for some time and changes the pickup delay > to be longer, so the holds don't go stale - with the patch in effect they > would have to do a database update to those holds. It's only one use case I > have come up with so far. There might be other disadvantages/advantages of > both ways of doing it. What is Koha doing if I adjust the loan period? It does not change the due dates in the system. The due date was set with the active guidelines of that moment. You could say: Doing the same for holds is consistent :) If the library closes, they should stop confirming holds in due time, limiting the number of waiting holds. Pending holds are no problem. The pickup date should be calculated at confirm hold time. They could also stop the cron job for cancelling holds until they processed the holds queue after reopening? Would they really need a db update?
As I said - I think thre are arguments for both approaches. :) Only I am thinking that this would not work: >If the library closes, they should stop confirming holds in due time, limiting >the number of waiting holds. Pending holds are no problem. The pickup date >should be calculated at confirm hold time. While that might, but requires you have access to the system or your support vendor lets you do it: >They could also stop the cron job for cancelling holds until they processed the >holds queue after reopening? Would they really need a db update?
Ah sorry, I missed half the comments: @Jonathan: No, I don't think it happens often that the value changes. I just have been thinking about the consequences of not recalculating. I didn't think about the calendar - that would be a solution (see bug 8735) Maybe making it possible to change the date from the interface (out of scope here) would be nice too at some point.
QA: Looking again here now..
Preliminary comments: Report Holds awaiting pickup: A DateTime object can only be compared to another DateTime object (DateTime=HASH(0x53d7020), 2013-02-22). at /usr/share/koha/testclone/circ/waitingreserves.pl line 110 Another serious issue: During upgrade all reserves need a maxpickupdate. Now they are null. Before I would signoff and pass qa, it would be very useful to have another signoff on this one. After testing your changes thorougly, another external signoff (maybe from Kyle?) is welcome too. The nature of these patches asks for more extensive testing. Please note that a more detailed test plan could help. For now setting status to Failed QA.
Please have a look too at bug 5786.
(In reply to comment #32) > Preliminary comments: > Report Holds awaiting pickup: > A DateTime object can only be compared to another DateTime object > (DateTime=HASH(0x53d7020), 2013-02-22). at > /usr/share/koha/testclone/circ/waitingreserves.pl line 110 > > Another serious issue: During upgrade all reserves need a maxpickupdate. Now > they are null. Fixed. > Before I would signoff and pass qa, it would be very useful to have another > signoff on this one. After testing your changes thorougly, another external > signoff (maybe from Kyle?) is welcome too. The nature of these patches asks > for more extensive testing. Yes! I tested some cases but there are so many specific cases, it is difficult to test everything alone. > Please note that a more detailed test plan could help. Since the patch affects all the pages related to holds, I don't know how to write a complete test plan. (In reply to comment #33) > Please have a look too at bug 5786. I did. I think my patch is closer to be pushed to master than bug 5786. I didn't find where the new column issuingrules.reservesmaxpickupdelay is used elsewhere than in the issuingrules management (add/remove in DB + in tt files).
Created attachment 15613 [details] [review] Bug 8367: Followup Add more granular level for ReservesMaxPickUpDelay This patch removes the ReservesMaxPickUpDelay syspref. The update database script will update the issuingrules table with the correct value before removing it. 1 new column: reserves.maxpickupdate (and for old_reserves). It contains the waitingdate + the corresponding max pickup delay. Each time the waitingdate is modified, this value will be modified too. To test: 1/ Play with the reserve workflow (Add, remove, change priority, etc.) and check there are no regression. 2/ Update the new column 'Holds pickup delay' in your issuing rules. According to a library and an item type, the maxpickupdate value will be equal to the waiting date + the pickup delay defined.
Created attachment 15658 [details] [review] [SIGNED-OFF] Bug 8367: Followup Add more granular level for ReservesMaxPickUpDelay This patch removes the ReservesMaxPickUpDelay syspref. The update database script will update the issuingrules table with the correct value before removing it. 1 new column: reserves.maxpickupdate (and for old_reserves). It contains the waitingdate + the corresponding max pickup delay. Each time the waitingdate is modified, this value will be modified too. To test: 1/ Play with the reserve workflow (Add, remove, change priority, etc.) and check there are no regression. 2/ Update the new column 'Holds pickup delay' in your issuing rules. According to a library and an item type, the maxpickupdate value will be equal to the waiting date + the pickup delay defined. Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Created attachment 15659 [details] [review] Bug 8367: Skip to next open day if max pickup date is a holiday
(In reply to comment #37) > Created attachment 15659 [details] [review] [review] > Bug 8367: Skip to next open day if max pickup date is a holiday I should note this also fixes a bug, where the call to GetReservesFromItemnumber assumed it would receive a reserve hashref, but it does not.
The last patch needs a commit message with a test plan.
(In reply to comment #39) > The last patch needs a commit message with a test plan. Kyle, since this patch is from you, could you provide a test plan please?
(In reply to comment #40) > (In reply to comment #39) > > The last patch needs a commit message with a test plan. > > Kyle, since this patch is from you, could you provide a test plan please? Sure! 1) Note your ReservesMaxPickUpDelay setting. 2) Create and trap a hold 3) View waitingreserves.pl, note the last pickup date, and the pickup location 4) Edit the calendar for the given location, and make the pickup date a holiday 4) View waitingreserves.pl, the last pickup date should now be the increased by one day I would add this to the patch, but at the moment the first patch isn't applying.
Created attachment 19093 [details] [review] Bug 8367: Add more granular level for ReservesMaxPickUpDelay This patch adds: - a new column in the issuing rules - a new field issuingrules.holdspickupdelay You can now specify a pickup delay for an hold function of a patron category and/or a item type and/or a library. To test: Check there is no regression with a normal reserve workflow. Add one or more issuingrules. In 4 templates, you can see the max pickup delay for an hold (circ/circulation.tt, circ/waitingreserves.tt, members/moremember.tt, opac-user.tt). Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Rebased on Feb 15, 2013 (Marcel de Rooy): CONFLICT (content): Merge conflict in opac/opac-user.pl CONFLICT (content): Merge conflict in koha-tmpl/intranet-tmpl/prog/en/modules/circ/circulation.tt CONFLICT (content): Merge conflict in installer/data/mysql/updatedatabase.pl CONFLICT (content): Merge conflict in circ/waitingreserves.pl
Created attachment 19094 [details] [review] Bug 8367: Add more granular level for ReservesMaxPickUpDelay - Followup Signed-off-by: Jonathan Druart <jonathan.druart@biblibre.com>
Created attachment 19095 [details] [review] Bug 8367: Followup Add more granular level for ReservesMaxPickUpDelay This patch removes the ReservesMaxPickUpDelay syspref. The update database script will update the issuingrules table with the correct value before removing it. 1 new column: reserves.maxpickupdate (and for old_reserves). It contains the waitingdate + the corresponding max pickup delay. Each time the waitingdate is modified, this value will be modified too. To test: 1/ Play with the reserve workflow (Add, remove, change priority, etc.) and check there are no regression. 2/ Update the new column 'Holds pickup delay' in your issuing rules. According to a library and an item type, the maxpickupdate value will be equal to the waiting date + the pickup delay defined. Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Created attachment 19096 [details] [review] Bug 8367: Skip to next open day if max pickup date is a holiday Test plan: 1) Note your ReservesMaxPickUpDelay setting. 2) Create and trap a hold 3) View waitingreserves.pl, note the last pickup date, and the pickup location 4) Edit the calendar for the given location, and make the pickup date a holiday 4) View waitingreserves.pl, the last pickup date should now be the increased by one day
I rebased 4 patches and added a test plan for the last one. Thanks Kyle!
Created attachment 19104 [details] [review] Bug 8367: Add more granular level for ReservesMaxPickUpDelay This patch adds: - a new column in the issuing rules - a new column reserves.maxpickupdate (+old_reserves) It contains the waitingdate + the corresponding max pickup delay. Each time the waitingdate is modified, this value will be modified too. - a new field issuingrules.holdspickupdelay This patch removes the ReservesMaxPickUpDelay syspref. The update database script will update the issuingrules table with the correct value before removing it. You can now specify a pickup delay for an hold function of a patron category and/or a item type and/or a library. To test: Check there is no regression with a normal reserve workflow. Add one or more issuingrules. Update the new column 'Holds pickup delay' in your issuing rules. In 4 templates, you can see the max pickup delay for an hold (circ/circulation.tt, circ/waitingreserves.tt, members/moremember.tt, opac-user.tt). According to a library and an item type, the maxpickupdate value will be equal to the waiting date + the pickup delay defined.
Created attachment 19105 [details] [review] Bug 8367: Skip to next open day if max pickup date is a holiday Test plan: 1) Note your ReservesMaxPickUpDelay setting. 2) Create and trap a hold 3) View waitingreserves.pl, note the last pickup date, and the pickup location 4) Edit the calendar for the given location, and make the pickup date a holiday 4) View waitingreserves.pl, the last pickup date should now be the increased by one day
I squashed the 3 first patches, it has become too difficult to rebase them... It needs a SO.
Created attachment 19107 [details] [review] Bug 8367: Add more granular level for ReservesMaxPickUpDelay This patch adds: - a new column in the issuing rules - a new column reserves.maxpickupdate (+old_reserves) It contains the waitingdate + the corresponding max pickup delay. Each time the waitingdate is modified, this value will be modified too. - a new field issuingrules.holdspickupdelay This patch removes the ReservesMaxPickUpDelay syspref. The update database script will update the issuingrules table with the correct value before removing it. You can now specify a pickup delay for an hold function of a patron category and/or a item type and/or a library. To test: Check there is no regression with a normal reserve workflow. Add one or more issuingrules. Update the new column 'Holds pickup delay' in your issuing rules. In 4 templates, you can see the max pickup delay for an hold (circ/circulation.tt, circ/waitingreserves.tt, members/moremember.tt, opac-user.tt). According to a library and an item type, the maxpickupdate value will be equal to the waiting date + the pickup delay defined.
Created attachment 19108 [details] [review] Bug 8367: Skip to next open day if max pickup date is a holiday Test plan: 1) Note your ReservesMaxPickUpDelay setting. 2) Create and trap a hold 3) View waitingreserves.pl, note the last pickup date, and the pickup location 4) Edit the calendar for the given location, and make the pickup date a holiday 4) View waitingreserves.pl, the last pickup date should now be the increased by one day
Note: Unit tests for GetMaxPickupDate are provided by bug 10487 (depends on 10289)
When I tried to create my first rule on a fresh database with the first patch only, I got this error: smart-rules.pl: DBD::mysql::st execute failed: called with 18 bind variables when 17 are needed at /home/koha/kohaclone/admin/smart-rules.pl line 137., referer: http://staff.kohadev/cgi-bin/koha/admin/smart-rules.pl
Created attachment 19116 [details] [review] Bug 8367: Followup FIX a sql statement (bad conflict merge)
Created attachment 19180 [details] [review] [SIGNED-OFF] Bug 8367: Add more granular level for ReservesMaxPickUpDelay This patch adds: - a new column in the issuing rules - a new column reserves.maxpickupdate (+old_reserves) It contains the waitingdate + the corresponding max pickup delay. Each time the waitingdate is modified, this value will be modified too. - a new field issuingrules.holdspickupdelay This patch removes the ReservesMaxPickUpDelay syspref. The update database script will update the issuingrules table with the correct value before removing it. You can now specify a pickup delay for an hold function of a patron category and/or a item type and/or a library. To test: Check there is no regression with a normal reserve workflow. Add one or more issuingrules. Update the new column 'Holds pickup delay' in your issuing rules. In 4 templates, you can see the max pickup delay for an hold (circ/circulation.tt, circ/waitingreserves.tt, members/moremember.tt, opac-user.tt). According to a library and an item type, the maxpickupdate value will be equal to the waiting date + the pickup delay defined. Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Created attachment 19181 [details] [review] Bug 8367: Skip to next open day if max pickup date is a holiday Test plan: 1) Note your ReservesMaxPickUpDelay setting. 2) Create and trap a hold 3) View waitingreserves.pl, note the last pickup date, and the pickup location 4) Edit the calendar for the given location, and make the pickup date a holiday 4) View waitingreserves.pl, the last pickup date should now be the increased by one day
Created attachment 19182 [details] [review] [SIGNED-OFF] Bug 8367: Followup FIX a sql statement (bad conflict merge) Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
(In reply to Kyle M Hall from comment #56) > Created attachment 19181 [details] [review] [review] > Bug 8367: Skip to next open day if max pickup date is a holiday Kyle, I don't understand how this patch can worked. The maxpickupdate is stored in the DB, when adding or updating an hold. The patch works but not with this test plan.
You're right, I think the test plan should be: Test plan: 1) Note your ReservesMaxPickUpDelay setting. 2) Create and trap a hold 3) View waitingreserves.pl, note the last pickup date, and the pickup location 4) Cancel this hold 5) Edit the calendar for the given location, and make the pickup date a holiday 6) Repeat step 2 7) View waitingreserves.pl, the last pickup date for this new hold should now be the increased by one day
Kyle, I agree with your test plan. I am signing off your patch. But I think I found a bug, could you take a look at this new patch please?
Created attachment 19360 [details] [review] Bug 8367: Skip to next open day if max pickup date is a holiday Test plan: 1) Note your ReservesMaxPickUpDelay setting. 2) Create and trap a hold 3) View waitingreserves.pl, note the last pickup date, and the pickup location 4) Cancel this hold 5) Edit the calendar for the given location, and make the pickup date a holiday 6) Repeat step 2 7) View waitingreserves.pl, the last pickup date for this new hold should now be the increased by one day Signed-off-by: Jonathan Druart <jonathan.druart@biblibre.com>
Created attachment 19361 [details] [review] Bug 8367: Fix the maxpickupdate when we set waitingdate to NOW() ModReserveAffect get the maxpickupdate before changing the waitingdate to NOW. We should update the waitingdate and next calculate the maxpickupdate.
Hmm, I think NOW() is a mysqlism. I know we probably have lots of those in the codebase already, but maybe worth trying not to add more and find best practices for replacing them? Mtj suggested using CURRENT_TIMESTAMP() instead on IRC: http://dev.mysql.com/doc/refman/5.1/en/date-and-time-functions.html
(In reply to Katrin Fischer from comment #63) > Hmm, I think NOW() is a mysqlism. I know we probably have lots of those in > the codebase already, but maybe worth trying not to add more and find best > practices for replacing them? > > Mtj suggested using CURRENT_TIMESTAMP() instead on IRC: > http://dev.mysql.com/doc/refman/5.1/en/date-and-time-functions.html I don't add it, it already exists.
Created attachment 19362 [details] [review] Bug 8367: Fix the maxpickupdate when we set waitingdate to NOW() ModReserveAffect get the maxpickupdate before changing the waitingdate to NOW. We should update the waitingdate and next calculate the maxpickupdate. Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Ok, maybe we could take a look at the NOW()s another time.
(In reply to Katrin Fischer from comment #66) > Ok, maybe we could take a look at the NOW()s another time. Yes, I would suggest filing a bug report to take care of any NOW()'s in the codebase. It seems likely however, that the upcoming DBIx::Class switchover will take care of those anyway.
Hi Kyle, there is a conflict in Reserves.pm I am not sure how to fix - could you take a look please? I think it's because your patch for using reserve_id is in now, so it might need a little work there.
Created attachment 20300 [details] [review] Bug 8367: Add more granular level for ReservesMaxPickUpDelay This patch adds: - a new column in the issuing rules - a new column reserves.maxpickupdate (+old_reserves) It contains the waitingdate + the corresponding max pickup delay. Each time the waitingdate is modified, this value will be modified too. - a new field issuingrules.holdspickupdelay This patch removes the ReservesMaxPickUpDelay syspref. The update database script will update the issuingrules table with the correct value before removing it. You can now specify a pickup delay for an hold function of a patron category and/or a item type and/or a library. To test: Check there is no regression with a normal reserve workflow. Add one or more issuingrules. Update the new column 'Holds pickup delay' in your issuing rules. In 4 templates, you can see the max pickup delay for an hold (circ/circulation.tt, circ/waitingreserves.tt, members/moremember.tt, opac-user.tt). According to a library and an item type, the maxpickupdate value will be equal to the waiting date + the pickup delay defined. Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Created attachment 20301 [details] [review] Bug 8367: Add more granular level for ReservesMaxPickUpDelay This patch adds: - a new column in the issuing rules - a new column reserves.maxpickupdate (+old_reserves) It contains the waitingdate + the corresponding max pickup delay. Each time the waitingdate is modified, this value will be modified too. - a new field issuingrules.holdspickupdelay This patch removes the ReservesMaxPickUpDelay syspref. The update database script will update the issuingrules table with the correct value before removing it. You can now specify a pickup delay for an hold function of a patron category and/or a item type and/or a library. To test: Check there is no regression with a normal reserve workflow. Add one or more issuingrules. Update the new column 'Holds pickup delay' in your issuing rules. In 4 templates, you can see the max pickup delay for an hold (circ/circulation.tt, circ/waitingreserves.tt, members/moremember.tt, opac-user.tt). According to a library and an item type, the maxpickupdate value will be equal to the waiting date + the pickup delay defined. Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Created attachment 20302 [details] [review] Bug 8367: Followup FIX a sql statement (bad conflict merge) Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Created attachment 20303 [details] [review] Bug 8367: Skip to next open day if max pickup date is a holiday Test plan: 1) Note your ReservesMaxPickUpDelay setting. 2) Create and trap a hold 3) View waitingreserves.pl, note the last pickup date, and the pickup location 4) Cancel this hold 5) Edit the calendar for the given location, and make the pickup date a holiday 6) Repeat step 2 7) View waitingreserves.pl, the last pickup date for this new hold should now be the increased by one day Signed-off-by: Jonathan Druart <jonathan.druart@biblibre.com>
Created attachment 20304 [details] [review] Bug 8367: Fix the maxpickupdate when we set waitingdate to NOW() ModReserveAffect get the maxpickupdate before changing the waitingdate to NOW. We should update the waitingdate and next calculate the maxpickupdate. Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
(In reply to Katrin Fischer from comment #68) > Hi Kyle, > there is a conflict in Reserves.pm I am not sure how to fix - could you take > a look please? I think it's because your patch for using reserve_id is in > now, so it might need a little work there. Conflicts solved.
Hi Jonathan, I am sorry, there are a couple of new conflicts now in this patch: Apply? [(y)es, (n)o, (i)nteractive] y Applying: Bug 8367: Add more granular level for ReservesMaxPickUpDelay Using index info to reconstruct a base tree... Falling back to patching base and 3-way merge... Auto-merging opac/opac-user.pl CONFLICT (content): Merge conflict in opac/opac-user.pl Auto-merging koha-tmpl/opac-tmpl/prog/en/modules/opac-user.tt Auto-merging koha-tmpl/intranet-tmpl/prog/en/modules/circ/circulation.tt Auto-merging koha-tmpl/intranet-tmpl/prog/en/modules/admin/preferences/circulation.pref Auto-merging installer/data/mysql/updatedatabase.pl CONFLICT (content): Merge conflict in installer/data/mysql/updatedatabase.pl Auto-merging installer/data/mysql/kohastructure.sql Auto-merging C4/Reserves.pm CONFLICT (content): Merge conflict in C4/Reserves.pm Failed to merge in the changes. Patch failed at 0001 Bug 8367: Add more granular level for ReservesMaxPickUpDelay When you have resolved this problem run "git bz apply --continue". If you would prefer to skip this patch, instead run "git bz apply --skip". To restore the original branch and stop patching run "git bz apply --abort". Patch left in /tmp/Bug-8367-Add-more-granular-level-for-ReservesMaxPi-BWSyFY.patch
We would like the option to have a shorter pickup period for some item types.
(In reply to Agnes Rivers-Moore from comment #76) > We would like the option to have a shorter pickup period for some item types. Sorry, but this patch does not apply anymore, and should be rewritten almost from scratch because another patch has been pushed before this one, that make what Jonathan made invalid. So, without funding to re-do the work, we (BibLibre) will not work on it anymore, sorry.
Hi there gentlemen and esteemed gentle lady! I just finished refactoring this feature to master and I am looking forward to sharing the results ASAP I get it better tested. Our bookmobiles are suffering from constantly canceled/billed reserves and this feature solves a big issue for them. Unfortunately this feature breaks a lot of our in-house work, but such is life :) I hope you can sharpen your "Sign off"-daggers and start stabbing, presuming you or your client is looking forward to getting this feature to master.
Created attachment 32786 [details] [review] Bug 8367 - How long is a hold waiting for pickup at a more granular level This patch adds: - a new column in the issuing rules, including help topics. - a new column reserves.lastpickupdate (+old_reserves) It contains the waitingdate + the corresponding "holds pickup wait". Each time the waitingdate is modified, this value will be modified too. - a new field issuingrules.holdspickupwait - a new function C4::Reserves::MoveWaitingdate() to help in testing this feature. This patch removes the ReservesMaxPickUpDelay syspref. - Also removes a code section from C4::Letters::_parseLetter() which worked blatantly disrespectfully towards Koha Calendar. - TODO put the <<reserves.lastpickupdate>> as a placeholder in C4::Letters. - TODO figure out how to rename existing letter placeholders <<expirationdate>> with <<lastpickupdate>> The update database script will update the issuingrules table with the correct value before removing it. You can now specify a pickup delay for an hold function of a patron category and/or a item type and/or a library. -TODO Refactoring unit tests is in progress. $$$$$$$$$$$$$ $ TEST PLAN $ $$$$$$$$$$$$$ ****** ****** *** *** *** TEST: BASIC WORKFLOW * ****** ****** 1. Check there is no regression with a normal reserve workflow. 2. Add one or more issuingrules. 3. Update the new column 'Holds pickup wait' in your issuing rules. 4. In 4 templates, you can see the 'Last pickup date' for an hold (circ/circulation.tt, circ/waitingreserves.tt, members/moremember.tt, opac-user.tt). 5. According to a library and an item type, the lastpickupdate value will be equal to the waiting date + the "holds pickup wait" defined. ** ** ** ** ** ** *** *** *** TEST: RESPECTING CALENDAR HOLIDAYS * ** ** ** ** ** ** 1) Note your ReservesMaxPickUpDelay setting. 2) Create and trap a hold 3) View waitingreserves.pl, note the last pickup date, and the pickup location 4) Cancel this hold 5) Edit the calendar for the given location, and make the pickup date a single, unrepeated, holiday. Then make the next day a weekly repeating holiday. Then make the next two days an annual holiday repeatable every year. 6) Repeat step 2 7) View waitingreserves.pl, the last pickup date for this new hold should now be increased by four days. ** ** ** ** ** ** *** *** *** TEST: LETTER lastpickupdate PLACEHOLDER AND REGRESSION * ** ** ** ** ** ** 1. Modify the HOLD-letter, add <<reserves.lastpickupdate>> and <<reserves.waitingdate>> somewhere. 2. Catch a hold for a borrower. 3. See the members/notices.pl for the message queued and verify that the lastpickupdate-placeholder is set in your desired datetime format and that it is not on a holiday. Verify that the waitingdate is today.
Making unit tests for the smart-rules.pl is rather tedious with the lack of proper getters/setters for rules. Need to introduce a C4::SmartRules.pm to access them more conveniently. I hope this is not getting way out of scope for this feture?
Hm, introducing a new module just for testing seems a bit out of scope :) Have you taken a look at some of the other tests for recently added features? I think there are some for the new automatic renewal and maybe others that could be useful.
Created attachment 32826 [details] [review] Bug 8367 - How long is a hold waiting for pickup at a more granular level You can now specify a pickup delay for an hold function of a patron category and/or a item type and/or a library. The lastpickupdate-column is set based on the given instruction and lands only on an open library day. Also cancel_expired_reserves.pl now cancels only on open library days after the last pickup date has expired. And waitingreserves.pl respects the new DB column. This patch adds: - a new column in the issuing rules, including help topics. - a new column reserves.lastpickupdate (+old_reserves) It contains the waitingdate + the corresponding "holds pickup wait". Each time the waitingdate is modified, this value will be modified too. - a new field issuingrules.holdspickupwait - a new function C4::Reserves::MoveWaitingdate() to help in testing this feature. - a new function C4::Reserves::GetLastPickupDate() to calculate the proper date from Calendar. This patch removes: - the ReservesMaxPickUpDelay syspref. - the ExpireReservesOnHolidays syspref, because it became obsolete due to the changes in handling the last pickup date with Koha Calendar. - Also removes a code section from C4::Letters::_parseLetter() which worked blatantly disrespectfully towards Koha Calendar. - TODO put the <<reserves.lastpickupdate>> as a placeholder in C4::Letters. - TODO figure out how to rename existing letter placeholders <<expirationdate>> with <<lastpickupdate>> TODO Refactoring unit tests is in progress. $$$$$$$$$$$$$ $ TEST PLAN $ $$$$$$$$$$$$$ ****** ****** *** *** *** TEST: BASIC WORKFLOW * ****** ****** 1. Check there is no regression with a normal reserve workflow. 2. Add one or more issuingrules. 3. Update the new column 'Holds pickup wait' in your issuing rules. 4. In 4 templates, you can see the 'Last pickup date' for an hold (circ/circulation.tt, circ/waitingreserves.tt, members/moremember.tt, opac-user.tt). 5. According to a library and an item type, the lastpickupdate value will be equal to the waiting date + the "holds pickup wait" defined. ** ** ** ** ** ** *** *** *** TEST: RESPECTING CALENDAR HOLIDAYS * ** ** ** ** ** ** 1) Note your ReservesMaxPickUpDelay setting. 2) Create and trap a hold 3) View waitingreserves.pl, note the last pickup date, and the pickup location 4) Cancel this hold 5) Edit the calendar for the given location, and make the pickup date a single, unrepeated, holiday. Then make the next day a weekly repeating holiday. Then make the next two days an annual holiday repeatable every year. 6) Repeat step 2 7) View waitingreserves.pl, the last pickup date for this new hold should now be increased by four days. ** ** ** ** ** ** *** *** *** TEST: LETTER lastpickupdate PLACEHOLDER AND REGRESSION * ** ** ** ** ** ** 1. Modify the HOLD-letter, add <<reserves.lastpickupdate>> and <<reserves.waitingdate>> somewhere. 2. Catch a hold for a borrower. 3. See the members/notices.pl for the message queued and verify that the lastpickupdate-placeholder is set in your desired datetime format and that it is not on a holiday. Verify that the waitingdate is today.
Created attachment 32884 [details] [review] Bug 8367 - How long is a hold waiting for pickup at a more granular level You can now specify a pickup delay for an hold function of a patron category and/or a item type and/or a library. The lastpickupdate-column is set based on the given instruction and lands only on an open library day. Also cancel_expired_reserves.pl now cancels only on open library days after the last pickup date has expired. And waitingreserves.pl respects the new DB column. This patch adds: - a new column in the issuing rules, including help topics. - a new column reserves.lastpickupdate (+old_reserves) It contains the waitingdate + the corresponding "holds pickup wait". Each time the waitingdate is modified, this value will be modified too. - a new field issuingrules.holdspickupwait - a new function C4::Reserves::MoveWaitingdate() to help in testing this feature. - a new function C4::Reserves::GetLastPickupDate() to calculate the proper date from Calendar. This patch removes: - the ReservesMaxPickUpDelay syspref. - the ExpireReservesOnHolidays syspref, because it became obsolete due to the changes in handling the last pickup date with Koha Calendar. - Also removes a code section from C4::Letters::_parseLetter() which worked blatantly disrespectfully towards Koha Calendar. - TODO figure out how to rename existing letter placeholders <<expirationdate>> with <<lastpickupdate>> TODO Refactoring unit tests is in progress. $$$$$$$$$$$$$ $ TEST PLAN $ $$$$$$$$$$$$$ ****** ****** *** *** *** TEST: BASIC WORKFLOW * ****** ****** 1. Check there is no regression with a normal reserve workflow. 2. Add one or more issuingrules. 3. Update the new column 'Holds pickup wait' in your issuing rules. 4. In 4 templates, you can see the 'Last pickup date' for an hold (circ/circulation.tt, circ/waitingreserves.tt, members/moremember.tt, opac-user.tt). 5. According to a library and an item type, the lastpickupdate value will be equal to the waiting date + the "holds pickup wait" defined moved to the next open library day. ****** ****** *** *** *** TEST: LETTER PLACEHOLDER (no code modifications here, just regression)* ****** ****** 1. Modify the RESERVESLIP->email Letter. Add "Last pickup date<<reserves.lastpickupdate>>" somewhere. 2. Make a hold for a borrower. 3. Catch the hold and print the reserve slip. Verify that the lastpickupdate is actually the (current date + the 'Holds pickup wait' in your issuing rules) in days. ** ** ** ** ** ** *** *** *** TEST: RESPECTING CALENDAR HOLIDAYS * ** ** ** ** ** ** 1) Note your ReservesMaxPickUpDelay setting. 2) Create and trap a hold 3) View waitingreserves.pl, note the last pickup date, and the pickup location 4) Cancel this hold 5) Edit the calendar for the given location, and make the pickup date a single, unrepeated, holiday. Then make the next day a weekly repeating holiday. Then make the next two days an annual holiday repeatable every year. 6) Repeat step 2 7) View waitingreserves.pl, the last pickup date for this new hold should now be increased by four days. ** ** ** ** ** ** *** *** *** TEST: LETTER lastpickupdate PLACEHOLDER AND REGRESSION * ** ** ** ** ** ** 1. Modify the HOLD-letter, add <<reserves.lastpickupdate>> and <<reserves.waitingdate>> somewhere. 2. Catch a hold for a borrower. 3. See the members/notices.pl for the message queued and verify that the lastpickupdate-placeholder is set in your desired datetime format and that it is not on a holiday. Verify that the waitingdate is today.
Created attachment 32885 [details] using this to obsolete previous patches
Created attachment 32916 [details] [review] Bug 8367 - How long is a hold waiting for pickup at a more granular level You can now specify a pickup delay for an hold function of a patron category and/or a item type and/or a library. The lastpickupdate-column is set based on the given instruction and lands only on an open library day. Also cancel_expired_reserves.pl now cancels only on open library days after the last pickup date has expired. And waitingreserves.pl respects the new DB column. This patch adds: - a new column in the issuing rules, including help topics. - a new column reserves.lastpickupdate (+old_reserves) It contains the waitingdate + the corresponding "holds pickup wait". Each time the waitingdate is modified, this value will be modified too. - a new field issuingrules.holdspickupwait - a new function C4::Reserves::MoveWaitingdate() to help in testing this feature. - a new function C4::Reserves::GetLastPickupDate() to calculate the proper date from Calendar. This patch removes: - the ReservesMaxPickUpDelay syspref. - the ExpireReservesOnHolidays syspref, because it became obsolete due to the changes in handling the last pickup date with Koha Calendar. - Also removes a code section from C4::Letters::_parseLetter() which worked blatantly disrespectfully towards Koha Calendar. - TODO figure out how to rename existing letter placeholders <<expirationdate>> with <<lastpickupdate>> TODO Refactoring unit tests is in progress. $$$$$$$$$$$$$ $ TEST PLAN $ $$$$$$$$$$$$$ ****** ****** *** *** *** TEST: BASIC WORKFLOW * ****** ****** 1. Check there is no regression with a normal reserve workflow. 2. Add one or more issuingrules. 3. Update the new column 'Holds pickup wait' in your issuing rules. 4. In 4 templates, you can see the 'Last pickup date' for an hold (circ/circulation.tt, circ/waitingreserves.tt, members/moremember.tt, opac-user.tt). 5. According to a library and an item type, the lastpickupdate value will be equal to the waiting date + the "holds pickup wait" defined moved to the next open library day. ****** ****** *** *** *** TEST: LETTER PLACEHOLDER (no code modifications here, just regression)* ****** ****** 1. Modify the RESERVESLIP->email Letter. Add "Last pickup date<<reserves.lastpickupdate>>" somewhere. 2. Make a hold for a borrower. 3. Catch the hold and print the reserve slip. Verify that the lastpickupdate is actually the (current date + the 'Holds pickup wait' in your issuing rules) in days. ** ** ** ** ** ** *** *** *** TEST: RESPECTING CALENDAR HOLIDAYS * ** ** ** ** ** ** 1) Note your ReservesMaxPickUpDelay setting. 2) Create and trap a hold 3) View waitingreserves.pl, note the last pickup date, and the pickup location 4) Cancel this hold 5) Edit the calendar for the given location, and make the pickup date a single, unrepeated, holiday. Then make the next day a weekly repeating holiday. Then make the next two days an annual holiday repeatable every year. 6) Repeat step 2 7) View waitingreserves.pl, the last pickup date for this new hold should now be increased by four days. ** ** ** ** ** ** *** *** *** TEST: LETTER lastpickupdate PLACEHOLDER AND REGRESSION * ** ** ** ** ** ** 1. Modify the HOLD-letter, add <<reserves.lastpickupdate>> and <<reserves.waitingdate>> somewhere. 2. Catch a hold for a borrower. 3. See the members/notices.pl for the message queued and verify that the lastpickupdate-placeholder is set in your desired datetime format and that it is not on a holiday. Verify that the waitingdate is today.
Kind Olli-Antti Kivilahti, I've applied the patch against master 3.17.00.035. I've upgraded the DB from a one that had been filled with some holds. After having applied, by mysql command line, on the DB I saw the new field "lastpickupdate" of the table "reserves" as required; in all the holds this new field was set to NULL. About this step: 4. In the 4 templates, you can see the 'Last pickup date' for an hold (circ/circulation.tt, circ/waitingreserves.tt, members/moremember.tt, opac-user.tt) in circ/circulation.tt (CIRC, Check out, Holds) and members/moremember.tt (Patrons, patron X, Details, Holds) there was the title "Last pickup date" of the new column (the 2-nd from the left) of the table, but the datas in the cells below it were the titles. All the other datas on the right were shifted on the left by 1 column, and the last column "Delete?" of the table had nothing in the cells below it. I created a new hold for a patron without holds, and the problem was the same on both the *.tt . About opac-user.tt, I logged in to OPAC as a patron with n holds. Here ("your summary", tag "Holds (n)") I didn't see any new column. So I pass the patch to "Failed QA" status.
Created attachment 32930 [details] [review] Bug 8367 - How long is a hold waiting for pickup at a more granular level You can now specify a pickup delay for an hold function of a patron category and/or a item type and/or a library. The lastpickupdate-column is set based on the given instruction and lands only on an open library day. Also cancel_expired_reserves.pl now cancels only on open library days after the last pickup date has expired. And waitingreserves.pl respects the new DB column. If ReservesMaxPickUpDelay is not set prior to the databaseupdate, the holdspickupwait-directive is never defined, thus disabling this feature until the holdspickupwait is set. If the issuingrules holdspickupwait is 0 or less, this feature is disabled. So each issuingrule can disable this feature for the material it matches, while being fully functional with other rules. This patch adds: - a new column in the issuing rules, including help topics. - a new column reserves.lastpickupdate (+old_reserves) It contains the waitingdate + the corresponding "holds pickup wait". Each time the waitingdate is modified, this value will be modified too. - a new field issuingrules.holdspickupwait - a new function C4::Reserves::MoveWaitingdate() to help in testing this feature. - a new function C4::Reserves::GetLastPickupDate() to calculate the proper date from Calendar. This patch removes: - the ReservesMaxPickUpDelay syspref. - the ExpireReservesOnHolidays syspref, because it became obsolete due to the changes in handling the last pickup date with Koha Calendar. - Also removes a code section from C4::Letters::_parseLetter() which worked blatantly disrespectfully towards Koha Calendar. - TODO figure out how to rename existing letter placeholders <<expirationdate>> with <<lastpickupdate>> Unit tests included. $$$$$$$$$$$$$ $ TEST PLAN $ $$$$$$$$$$$$$ 0. BEFORE UPDATING THE DATABASE! MAKE SURE THAT THE SYSPREF 'ReservesMaxPickUpDelay' is bigger than 0. Otherwise existing reserves won't get UPDATEd with the new lastpickupdate-values. ****** ****** *** *** *** TEST: BASIC WORKFLOW * ****** ****** 1. Check there is no regression with a normal reserve workflow. 2. Add one or more issuingrules. 3. Update the new column 'Holds pickup wait' in your issuing rules. 4. In the 3 templates of staff client, you can see the 'Last pickup date' for an hold (circ/circulation.pl, circ/waitingreserves.pl, members/moremember.pl) 5. In opac-user.pl the last pickup date is displayed in the status-column if available. 5. According to a library and an item type, the lastpickupdate value will be equal to the waiting date + the "holds pickup wait" defined moved to the next open library day. ****** ****** *** *** *** TEST: LETTER PLACEHOLDER (no code modifications here, just regression)* ****** ****** 1. Modify the RESERVESLIP->email Letter. Add "Last pickup date<<reserves.lastpickupdate>>" somewhere. 2. Make a hold for a borrower. 3. Catch the hold and print the reserve slip. Verify that the lastpickupdate is actually the (current date + the 'Holds pickup wait' in your issuing rules) in days. ** ** ** ** ** ** *** *** *** TEST: RESPECTING CALENDAR HOLIDAYS * ** ** ** ** ** ** 1) Note your ReservesMaxPickUpDelay setting. 2) Create and trap a hold 3) View waitingreserves.pl, note the last pickup date, and the pickup location 4) Cancel this hold 5) Edit the calendar for the given location, and make the pickup date a single, unrepeated, holiday. Then make the next day a weekly repeating holiday. Then make the next two days an annual holiday repeatable every year. 6) Repeat step 2 7) View waitingreserves.pl, the last pickup date for this new hold should now be increased by four days. ** ** ** ** ** ** *** *** *** TEST: LETTER lastpickupdate PLACEHOLDER AND REGRESSION * ** ** ** ** ** ** 1. Modify the HOLD-letter, add <<reserves.lastpickupdate>> and <<reserves.waitingdate>> somewhere. 2. Catch a hold for a borrower. 3. See the members/notices.pl for the message queued and verify that the lastpickupdate-placeholder is set in your desired datetime format and that it is not on a holiday. Verify that the waitingdate is today.
(In reply to Paola Rossi from comment #86) > Kind Olli-Antti Kivilahti, > Thank you so much for testing this patch Ms. or Mrs. Rossi! > I've applied the patch against master 3.17.00.035. > > I've upgraded the DB from a one that had been filled with some holds. > > After having applied, by mysql command line, on the DB I saw the new field > "lastpickupdate" of the table "reserves" as required; in all the holds this > new field was set to NULL. > Sorry, my test plans were not complete. I added the following clause: 0. BEFORE UPDATING THE DATABASE! MAKE SURE THAT THE SYSPREF 'ReservesMaxPickUpDelay' is bigger than 0. Otherwise existing reserves won't get UPDATEd with the new lastpickupdate-values. > About this step: > 4. In the 4 templates, you can see the 'Last pickup date' for an hold > (circ/circulation.tt, circ/waitingreserves.tt, members/moremember.tt, > opac-user.tt) > > in circ/circulation.tt (CIRC, Check out, Holds) and members/moremember.tt > (Patrons, patron X, Details, Holds) there was the title "Last pickup date" > of the new column (the 2-nd from the left) of the table, but the datas in > the cells below it were the titles. All the other datas on the right were > shifted on the left by 1 column, and the last column "Delete?" of the table > had nothing in the cells below it. > > I created a new hold for a patron without holds, and the problem was the > same on both the *.tt . > Are you sure you used the english language? I have encountered this issue mostly with not-up-to-date language templates. I retested this feature and was unable to replicate your report. > About opac-user.tt, I logged in to OPAC as a patron with n holds. Here > ("your summary", tag "Holds (n)") I didn't see any new column. > Sorry, updated the TEST PLAN as follows: 5. In opac-user.pl the last pickup date is displayed in the status-column if available. > So I pass the patch to "Failed QA" status. Thanks for testing! Hope to hear from you soon :)
(In reply to Olli-Antti Kivilahti from comment #88) > [...] Hope to hear from you soon Thanks, Olli-Antti, for your kind feedbacks. Today I've applied the patch against master 3.17.00.043. After having applied, I've created a hold reserving an item that was checked out. So the reserve/hold's status became "W" after checking in it. On the browser I refreshed the cache (Ctrl r, Ctrl F5): so I saw the hold by the 4 *.tt as required in step 4. In particular: on OPAC, for a logged patron with holds, the lastpickupdate was in the "until" part of the "Status" column of the hold. On circ/waitingreserves.tt (Home › Circulation › Holds awaiting pickup) the lastpickupdate was in the "until" part of the first column "Available" of "n Hold(s) waiting" tag. The tables with holds with NULL lastpickupdate (holds created reserving items not checked out, as allowed by syspref AllowOnShelfHolds set to "Allow") were shown OK too. So I pass the patch back to "Needs Signoff" status. I've changed the syspref 'ReservesMaxPickUpDelay' from the default 0 value, thank you again.
(In reply to Paola Rossi from comment #89) > (In reply to Olli-Antti Kivilahti from comment #88) > > [...] Hope to hear from you soon > > Thanks, Olli-Antti, for your kind feedbacks. > > Today I've applied the patch against master 3.17.00.043. > > After having applied, I've created a hold reserving an item that was checked > out. So the reserve/hold's status became "W" after checking in it. On the > browser I refreshed the cache (Ctrl r, Ctrl F5): so I saw the hold by the 4 > *.tt as required in step 4. > > In particular: on OPAC, for a logged patron with holds, the lastpickupdate > was in the "until" part of the "Status" column of the hold. > On circ/waitingreserves.tt (Home › Circulation › Holds awaiting pickup) the > lastpickupdate was in the "until" part of the first column "Available" of "n > Hold(s) waiting" tag. > > The tables with holds with NULL lastpickupdate (holds created reserving > items not checked out, as allowed by syspref AllowOnShelfHolds set to > "Allow") were shown OK too. > > So I pass the patch back to "Needs Signoff" status. > > I've changed the syspref 'ReservesMaxPickUpDelay' from the default 0 value, > thank you again. Nice to hear from you! Is there anything here that prevents your from signing this off? I believe this feature needs more than one sign off to be considered fit for QA and yours would be a great start!
[I beg you pardon for the length of this comment, my delay in answering and possible inaccuracies.] I've applied the patch against master 3.17.00.044 [HEAD 10860]. To test the patch IMHO on ADMIN it is better: - (as required by the test plan) kept the value of syspref "ReservesMaxPickupDelay" > 0 - set the syspref "EnhancedMessagingPreferences" to ON (default is OFF) To test the patch IMHO on ADMIN it could be better: - kept the default value "Don't allow" to the syspref "AllowOnShelfHolds" - set the "Charge" of the item-types to 0.00 (default 5.00) for all types - [remember to set the circulation rules] - set the syspref "KohaAdminEmailAddress" to a value not filtered by antispam's own emailing-system [to receive notices' email] - [remeber to refresh the cache [Ctrl r - Ctrl F5] after seeing master's behaviour]. At my tests, the RESERVESLIP and HOLD notices are allowed to be cloned to your specific library, but I saw that koha uses only the "ALL LIBRARIES" notices, and never the cloned ones. So I changed those 2 notices to test the 2 parts of the test plan about LETTER. To test the part of the test plan about the RESERVESLIP notice, i.e. see below: *** TEST: LETTER PLACEHOLDER ***, IMHO the functions to test are "Confirm"/"Print and confirm", on cheching in a record already on hold by another patron. To test the last part of the test plan about the HOLD notice, i.e. see below: *** TEST: LETTER lastpickupdate PLACEHOLDER AND REGRESSION ****, if I'm not wrong : - set the notice's email [i.e. "Primary email"] of the test patron to your own email - if this patron had a found/status W hold, you'll receive a mail soon after: perl ./misc/cronjobs/advance_notices.pl -c perl ./misc/cronjobs/process_message_queue.pl IMHO it would be better explicitly adding a further last part to the test plan about the provided: t/db_dependent/Holds.t [perhaps other parts of the preamble could need a testing action too.] At my tests the upgrade of the DB is OK. The 4 *.tt are OK. NB1.: on ADMIN, in the "Circ and fines issuing rules", perhaps the new column "Holds wait for pickup (day)" could be better set at the right of "Holds allowed (count)", at the right of all the 4 columns about renewal, and not among these 4 ones. NB2.: on CIRC, on waitingreserves.pl (="Holds awaiting pickup"), on the "Holds over" tag, the present string: "Holds listed here have been awaiting pickup for more than days." could be modified. In master it was: "Holds listed here have been awaiting pickup for more than X days." where X was the value of the syspref "ReservesMaxPickUpDelay" which has been removed by this patch. NB3.: On opac-user.pl, for a logged in user with W holds, the W holds had not neigher the "Cancel" icon nor the "Suspend" icon. [I didn't check the master about this behaviour, sorry. If this patch had changed the behaviour, this regression would be on error, and this point will be added to the errors' list.] PROBLEMS: NB4.: [MASTER] at my tests the "W" found/status of a hold/reserve is set in only 1 case: it is set to a hold, that previously had been opened on a biblio record which was already checked out (found=NULL), and then checked in (found=W) without "Ignore" submit. [I'd like to have a feedback about it, please, because all my tests were about this assumption.] NB5.: [MASTER] So, at my tests, the waitingdate of a hold/reserve can be only the current date, i.e. the current day on which I check-in a biblio record. [I'd like to have a feedback about it, please, because all my tests were about this assumption.] 1) I saw no reserves.lastpickupdate nor the reserves.waitingdate on RESERVESLIP notice. But I think that this problem is out of this bug, because the master too behaves so. [On the contrary, on master (and after applying too) the reserves.reservesnotes is correctly shown by the notice.] 2) Whilst a check out lasts 1 more day for an intervening holiday, the hold's period doesn't: I'd like to have a feedback about this, please. If the W hold would last 1 more day, the patch would be in error. 3) On upgrading the DB, by syspref "ReservesMaxPickupDelay" > 0, all the holds for all the patrons' categories and item types would receive the same lasting period. Whilst the patch, once applied, could assign a different value to each of them. I think this remarkable to get coherent DB datas. 4) The found/status "W" meaning is about the lasting period available to the patron to pick up the document. But when a "Transfer" is needed from the check-out' library, to the hold's library, koha [master] doesn't set the found/status to "W", but to NULL/T: I think that this behaviour could be evaluated, that it can be a master's bug, and, if this is the case, that this patch could be tested after having resolved this bug on a DB with such cases. 5) prove t/db_dependent/Holds.t t/db_dependent/Holds.t .. 1/46 # Failed test 'GetLastPickupDate(): Not using Calendar' # at t/db_dependent/Holds.t line 436. # got: '2014-11-10' # expected: '2014-11-05' # Failed test 'GetLastPickupDate(): Moving lastpickupdate over holidays, but not affected by them' # at t/db_dependent/Holds.t line 448. # got: '2014-11-10' # expected: '2014-11-05' # Failed test 'MoveWaitingdate(): Moving to past' # at t/db_dependent/Holds.t line 401. # got: '' # expected: '1' # Failed test 'Waiting reserve with lastpickupdate for 2014-11-04 totally canceled' # at t/db_dependent/Holds.t line 367. # got: '1' # expected: '0' # Looks like you failed 4 tests of 46. t/db_dependent/Holds.t .. Dubious, test returned 4 (wstat 1024, 0x400) Failed 4/46 subtests Test Summary Report ------------------- t/db_dependent/Holds.t (Wstat: 1024 Tests: 46 Failed: 4) Failed tests: 37, 39, 41, 44 Non-zero exit status: 4 Files=1, Tests=46, 1 wallclock secs ( 0.01 usr 0.01 sys + 0.91 cusr 0.29 csys = 1.22 CPU) Result: FAIL For the 5-th problem I pass the patch to "Failed QA" status.
I beg your pardon for the following comment, further in addition to the 91-th comment. It's about the syspref "AllowOnShelfHolds". As the manual 3.16 says: "Asks: ___ hold requests to be placed on items that are not checked out." At my tests, the default value is "Don't allow" instead of the manual's default "Allow". The "Allow" status is the most interesting one, of course. [on MASTER] A) About a multi branch system, with "Transfer" from L1 to L2 ["if the library system is a multi branch system and patrons used the hold system to request items from other libraries"], the 3 status/found (of the just created hold, of the transferred hold by L1, and also after cancelling the Transfer by L2's receiving] are all NULL. IMHO the status/found of the hold could be "W" after cancelling. B) About reserving from home ["if the library wanted to allow users to place holds on items [NOT OUT!] from home"], IMHO the status/found of the hold could be "W", but at my tests it's NULL. If A) and B) be bugs, and fixed, all these new "W" cases of a hold should be tested in this patch 8367 too.
I've tried to apply the patch against master 3.19.00.002 Applying: Bug 8367 - How long is a hold waiting for pickup at a more granular level Using index info to reconstruct a base tree... Falling back to patching base and 3-way merge... Auto-merging t/db_dependent/Holds.t Auto-merging svc/holds Auto-merging koha-tmpl/intranet-tmpl/prog/en/modules/members/moremember.tt Auto-merging koha-tmpl/intranet-tmpl/prog/en/modules/circ/waitingreserves.tt CONFLICT (content): Merge conflict in koha-tmpl/intranet-tmpl/prog/en/modules/circ/waitingreserves.tt Auto-merging koha-tmpl/intranet-tmpl/prog/en/modules/circ/circulation.tt Auto-merging koha-tmpl/intranet-tmpl/prog/en/modules/admin/preferences/circulation.pref Auto-merging koha-tmpl/intranet-tmpl/prog/en/js/holds.js Auto-merging installer/data/mysql/updatedatabase.pl CONFLICT (content): Merge conflict in installer/data/mysql/updatedatabase.pl Auto-merging installer/data/mysql/sysprefs.sql Auto-merging installer/data/mysql/kohastructure.sql Auto-merging circ/waitingreserves.pl Auto-merging C4/Reserves.pm Auto-merging C4/Letters.pm Failed to merge in the changes. Patch failed at 0001 Bug 8367 - How long is a hold waiting for pickup at a more granular level So I pass the patch to "Patch doesn't apply" status.
I'll start working on this and I decided to create a new patch and implement the same changes on it.
Created attachment 42706 [details] [review] Bug 8367 - How long is a hold waiting for pickup at a more granular level This patch applies now. You can now specify a pickup delay for an hold function of a patron category and/or a item type and/or a library. The lastpickupdate-column is set based on the given instruction and lands only on an open library day. Also cancel_expired_reserves.pl now cancels only on open library days after the last pickup date has expired. And waitingreserves.pl respects the new DB column. If ReservesMaxPickUpDelay is not set prior to the databaseupdate, the holdspickupwait-directive is never defined, thus disabling this feature until the holdspickupwait is set. If the issuingrules holdspickupwait is 0 or less, this feature is disabled. So each issuingrule can disable this feature for the material it matches, while being fully functional with other rules. This patch adds: - a new column in the issuing rules, including help topics. - a new column reserves.lastpickupdate (+old_reserves) It contains the waitingdate + the corresponding "holds pickup wait". Each time the waitingdate is modified, this value will be modified too. - a new field issuingrules.holdspickupwait - a new function C4::Reserves::MoveWaitingdate() to help in testing this feature. - a new function C4::Reserves::GetLastPickupDate() to calculate the proper date from Calendar. This patch removes: - the ReservesMaxPickUpDelay syspref. - the ExpireReservesOnHolidays syspref, because it became obsolete due to the changes in handling the last pickup date with Koha Calendar. - Also removes a code section from C4::Letters::_parseLetter() which worked blatantly disrespectfully towards Koha Calendar. - TODO figure out how to rename existing letter placeholders <<expirationdate>> with <<lastpickupdate>> Unit tests included. $$$$$$$$$$$$$ $ TEST PLAN $ $$$$$$$$$$$$$ 0. BEFORE UPDATING THE DATABASE! MAKE SURE THAT THE SYSPREF 'ReservesMaxPickUpDelay' is bigger than 0. Otherwise existing reserves won't get UPDATEd with the new lastpickupdate-values. ****** ****** *** *** *** TEST: BASIC WORKFLOW * ****** ****** 1. Check there is no regression with a normal reserve workflow. 2. Add one or more issuingrules. 3. Update the new column 'Holds pickup wait' in your issuing rules. 4. In the 3 templates of staff client, you can see the 'Last pickup date' for an hold (circ/circulation.pl, circ/waitingreserves.pl, members/moremember.pl) 5. In opac-user.pl the last pickup date is displayed in the status-column if available. 5. According to a library and an item type, the lastpickupdate value will be equal to the waiting date + the "holds pickup wait" defined moved to the next open library day. ****** ****** *** *** *** TEST: LETTER PLACEHOLDER (no code modifications here, just regression)* ****** ****** 1. Modify the RESERVESLIP->email Letter. Add "Last pickup date<<reserves.lastpickupdate>>" somewhere. 2. Make a hold for a borrower. 3. Catch the hold and print the reserve slip. Verify that the lastpickupdate is actually the (current date + the 'Holds pickup wait' in your issuing rules) in days. ** ** ** ** ** ** *** *** *** TEST: RESPECTING CALENDAR HOLIDAYS * ** ** ** ** ** ** 1) Note your ReservesMaxPickUpDelay setting. 2) Create and trap a hold 3) View waitingreserves.pl, note the last pickup date, and the pickup location 4) Cancel this hold 5) Edit the calendar for the given location, and make the pickup date a single, unrepeated, holiday. Then make the next day a weekly repeating holiday. Then make the next two days an annual holiday repeatable every year. 6) Repeat step 2 7) View waitingreserves.pl, the last pickup date for this new hold should now be increased by four days. ** ** ** ** ** ** *** *** *** TEST: LETTER lastpickupdate PLACEHOLDER AND REGRESSION * ** ** ** ** ** ** 1. Modify the HOLD-letter, add <<reserves.lastpickupdate>> and <<reserves.waitingdate>> somewhere. 2. Catch a hold for a borrower. 3. See the members/notices.pl for the message queued and verify that the lastpickupdate-placeholder is set in your desired datetime format and that it is not on a holiday. Verify that the waitingdate is today.
Created attachment 42754 [details] [review] Bug 8367 - How long is a hold waiting for pickup at a more granular level You can now specify a pickup delay for an hold function of a patron category and/or a item type and/or a library. The lastpickupdate-column is set based on the given instruction and lands only on an open library day. Also cancel_expired_reserves.pl now cancels only on open library days after the last pickup date has expired. And waitingreserves.pl respects the new DB column. If ReservesMaxPickUpDelay is not set prior to the databaseupdate, the holdspickupwait-directive is never defined, thus disabling this feature until the holdspickupwait is set. If the issuingrules holdspickupwait is 0 or less, this feature is disabled. So each issuingrule can disable this feature for the material it matches, while being fully functional with other rules. This patch adds: - a new column in the issuing rules, including help topics. - a new column reserves.lastpickupdate (+old_reserves) It contains the waitingdate + the corresponding "holds pickup wait". Each time the waitingdate is modified, this value will be modified too. - a new field issuingrules.holdspickupwait - a new function C4::Reserves::MoveWaitingdate() to help in testing this feature. - a new function C4::Reserves::GetLastPickupDate() to calculate the proper date from Calendar. This patch removes: - the ReservesMaxPickUpDelay syspref. - the ExpireReservesOnHolidays syspref, because it became obsolete due to the changes in handling the last pickup date with Koha Calendar. - Also removes a code section from C4::Letters::_parseLetter() which worked blatantly disrespectfully towards Koha Calendar. - TODO figure out how to rename existing letter placeholders <<expirationdate>> with <<lastpickupdate>> Unit tests included. $$$$$$$$$$$$$ $ TEST PLAN $ $$$$$$$$$$$$$ 0. BEFORE UPDATING THE DATABASE! MAKE SURE THAT THE SYSPREF 'ReservesMaxPickUpDelay' is bigger than 0. Otherwise existing reserves won't get UPDATEd with the new lastpickupdate-values. ****** ****** *** *** *** TEST: BASIC WORKFLOW * ****** ****** 1. Check there is no regression with a normal reserve workflow. 2. Add one or more issuingrules. 3. Update the new column 'Holds pickup wait' in your issuing rules. 4. In the 3 templates of staff client, you can see the 'Last pickup date' for an hold (circ/circulation.pl, circ/waitingreserves.pl, members/moremember.pl) 5. In opac-user.pl the last pickup date is displayed in the status-column if available. 5. According to a library and an item type, the lastpickupdate value will be equal to the waiting date + the "holds pickup wait" defined moved to the next open library day. ****** ****** *** *** *** TEST: LETTER PLACEHOLDER (no code modifications here, just regression)* ****** ****** 1. Modify the RESERVESLIP->email Letter. Add "Last pickup date<<reserves.lastpickupdate>>" somewhere. 2. Make a hold for a borrower. 3. Catch the hold and print the reserve slip. Verify that the lastpickupdate is actually the (current date + the 'Holds pickup wait' in your issuing rules) in days. ** ** ** ** ** ** *** *** *** TEST: RESPECTING CALENDAR HOLIDAYS * ** ** ** ** ** ** 1) Note your ReservesMaxPickUpDelay setting. 2) Create and trap a hold 3) View waitingreserves.pl, note the last pickup date, and the pickup location 4) Cancel this hold 5) Edit the calendar for the given location, and make the pickup date a single, unrepeated, holiday. Then make the next day a weekly repeating holiday. Then make the next two days an annual holiday repeatable every year. 6) Repeat step 2 7) View waitingreserves.pl, the last pickup date for this new hold should now be increased by four days. ** ** ** ** ** ** *** *** *** TEST: LETTER lastpickupdate PLACEHOLDER AND REGRESSION * ** ** ** ** ** ** 1. Modify the HOLD-letter, add <<reserves.lastpickupdate>> and <<reserves.waitingdate>> somewhere. 2. Catch a hold for a borrower. 3. See the members/notices.pl for the message queued and verify that the lastpickupdate-placeholder is set in your desired datetime format and that it is not on a holiday. Verify that the waitingdate is today.
After this patch gets to master, the system preference ExpireReservesMaxPickUpDelay could to be changed to something like ExpireReservesAfterLastPickupDay. I guess it is then more descriptive. There was about 15-20 files depending on ExpireReservesMaxPickUpDelay preference, so it would be better to do maybe a new bug report for it?
fatal: previous rebase directory .git/rebase-apply still exists but mbox given. at ./getter.pl line 196.
Is anyone still working on this? Being able to provide extended hold dates (e.g. for items set aside for our housebound patrons) would be very useful.
Hi there I have gone through and rebased this, master had progressed quite a lot since this patch was added and so I had to make a few assumptions during the rebase. Please let me know if changes need to be made as a result of the rebase. I have also corrected the commit messages to the format 'Bug XXXX: ' I have also written two other patches which correct template issues, and remove the check for the value of the 'ReserveMaxPickupDelay' syspref in Koha/Hold.pm in the set_waiting() function. This check is unneccessary as the main patch on this bug report removes the ReserveMaxPickupDelay syspref, and so I replaced this with a retrieval of the appropriate issuingrule and setting the exprationdate = waitingdate + issuingrule.lastpickupwait I am currently working on the unit tests t/db_dependent/Hold.t and so I am assigning this the status of 'Assigned' until I have attached a commit with the corrected t/db_dependent/Hold.t
Created attachment 79790 [details] [review] Bug 8367: How long is a hold waiting for pickup at a more granular level You can now specify a pickup delay for an hold function of a patron category and/or a item type and/or a library. The lastpickupdate-column is set based on the given instruction and lands only on an open library day. Also cancel_expired_reserves.pl now cancels only on open library days after the last pickup date has expired. And waitingreserves.pl respects the new DB column. If ReservesMaxPickUpDelay is not set prior to the databaseupdate, the holdspickupwait-directive is never defined, thus disabling this feature until the holdspickupwait is set. If the issuingrules holdspickupwait is 0 or less, this feature is disabled. So each issuingrule can disable this feature for the material it matches, while being fully functional with other rules. This patch adds: - a new column in the issuing rules, including help topics. - a new column reserves.lastpickupdate (+old_reserves) It contains the waitingdate + the corresponding "holds pickup wait". Each time the waitingdate is modified, this value will be modified too. - a new field issuingrules.holdspickupwait - a new function C4::Reserves::MoveWaitingdate() to help in testing this feature. - a new function C4::Reserves::GetLastPickupDate() to calculate the proper date from Calendar. This patch removes: - the ReservesMaxPickUpDelay syspref. - the ExpireReservesOnHolidays syspref, because it became obsolete due to the changes in handling the last pickup date with Koha Calendar. - Also removes a code section from C4::Letters::_parseLetter() which worked blatantly disrespectfully towards Koha Calendar. - TODO figure out how to rename existing letter placeholders <<expirationdate>> with <<lastpickupdate>> Unit tests included. $$$$$$$$$$$$$ $ TEST PLAN $ $$$$$$$$$$$$$ 0. BEFORE UPDATING THE DATABASE! MAKE SURE THAT THE SYSPREF 'ReservesMaxPickUpDelay' is bigger than 0. Otherwise existing reserves won't get UPDATEd with the new lastpickupdate-values. ****** ****** *** *** *** TEST: BASIC WORKFLOW * ****** ****** 1. Check there is no regression with a normal reserve workflow. 2. Add one or more issuingrules. 3. Update the new column 'Holds pickup wait' in your issuing rules. 4. In the 3 templates of staff client, you can see the 'Last pickup date' for an hold (circ/circulation.pl, circ/waitingreserves.pl, members/moremember.pl) 5. In opac-user.pl the last pickup date is displayed in the status-column if available. 5. According to a library and an item type, the lastpickupdate value will be equal to the waiting date + the "holds pickup wait" defined moved to the next open library day. ****** ****** *** *** *** TEST: LETTER PLACEHOLDER (no code modifications here, just regression)* ****** ****** 1. Modify the RESERVESLIP->email Letter. Add "Last pickup date<<reserves.lastpickupdate>>" somewhere. 2. Make a hold for a borrower. 3. Catch the hold and print the reserve slip. Verify that the lastpickupdate is actually the (current date + the 'Holds pickup wait' in your issuing rules) in days. ** ** ** ** ** ** *** *** *** TEST: RESPECTING CALENDAR HOLIDAYS * ** ** ** ** ** ** 1) Note your ReservesMaxPickUpDelay setting. 2) Create and trap a hold 3) View waitingreserves.pl, note the last pickup date, and the pickup location 4) Cancel this hold 5) Edit the calendar for the given location, and make the pickup date a single, unrepeated, holiday. Then make the next day a weekly repeating holiday. Then make the next two days an annual holiday repeatable every year. 6) Repeat step 2 7) View waitingreserves.pl, the last pickup date for this new hold should now be increased by four days. ** ** ** ** ** ** *** *** *** TEST: LETTER lastpickupdate PLACEHOLDER AND REGRESSION * ** ** ** ** ** ** 1. Modify the HOLD-letter, add <<reserves.lastpickupdate>> and <<reserves.waitingdate>> somewhere. 2. Catch a hold for a borrower. 3. See the members/notices.pl for the message queued and verify that the lastpickupdate-placeholder is set in your desired datetime format and that it is not on a holiday. Verify that the waitingdate is today. Conflicts: installer/data/mysql/updatedatabase.pl
Created attachment 79791 [details] [review] Bug 8367: Template fixes + fixes to C4/Reserves.pm and Koha/Hold.pm Fixed waitingreserves.tt template so the borrowernumber is successfully passed to waitingreserves.pl upon cancelling a hold, so users can now successfully cancel holds from the waitingreserves.pl interface. Removed check for ReserveMaxPickupdateDelay in Koha/Hold.pm as this syspref is removed in the previous patch. Instead the expirationdate is calculated the same way as the lastpickupdatedate that is retrieving the holdpickupwait value for the appropriate issuingrule and then adding that onto the waiting date. Same test plan as main patch Sponsored-By: Brimbank Library, Australia
Created attachment 79798 [details] [review] Bug 8367: How long is a hold waiting for pickup at a more granular level You can now specify a pickup delay for an hold function of a patron category and/or a item type and/or a library. The lastpickupdate-column is set based on the given instruction and lands only on an open library day. Also cancel_expired_reserves.pl now cancels only on open library days after the last pickup date has expired. And waitingreserves.pl respects the new DB column. If ReservesMaxPickUpDelay is not set prior to the databaseupdate, the holdspickupwait-directive is never defined, thus disabling this feature until the holdspickupwait is set. If the issuingrules holdspickupwait is 0 or less, this feature is disabled. So each issuingrule can disable this feature for the material it matches, while being fully functional with other rules. This patch adds: - a new column in the issuing rules, including help topics. - a new column reserves.lastpickupdate (+old_reserves) It contains the waitingdate + the corresponding "holds pickup wait". Each time the waitingdate is modified, this value will be modified too. - a new field issuingrules.holdspickupwait - a new function C4::Reserves::MoveWaitingdate() to help in testing this feature. - a new function C4::Reserves::GetLastPickupDate() to calculate the proper date from Calendar. This patch removes: - the ReservesMaxPickUpDelay syspref. - the ExpireReservesOnHolidays syspref, because it became obsolete due to the changes in handling the last pickup date with Koha Calendar. - Also removes a code section from C4::Letters::_parseLetter() which worked blatantly disrespectfully towards Koha Calendar. - TODO figure out how to rename existing letter placeholders <<expirationdate>> with <<lastpickupdate>> Unit tests included. $$$$$$$$$$$$$ $ TEST PLAN $ $$$$$$$$$$$$$ 0. BEFORE UPDATING THE DATABASE! MAKE SURE THAT THE SYSPREF 'ReservesMaxPickUpDelay' is bigger than 0. Otherwise existing reserves won't get UPDATEd with the new lastpickupdate-values. ****** ****** *** *** *** TEST: BASIC WORKFLOW * ****** ****** 1. Check there is no regression with a normal reserve workflow. 2. Add one or more issuingrules. 3. Update the new column 'Holds pickup wait' in your issuing rules. 4. In the 3 templates of staff client, you can see the 'Last pickup date' for an hold (circ/circulation.pl, circ/waitingreserves.pl, members/moremember.pl) 5. In opac-user.pl the last pickup date is displayed in the status-column if available. 5. According to a library and an item type, the lastpickupdate value will be equal to the waiting date + the "holds pickup wait" defined moved to the next open library day. ****** ****** *** *** *** TEST: LETTER PLACEHOLDER (no code modifications here, just regression)* ****** ****** 1. Modify the RESERVESLIP->email Letter. Add "Last pickup date<<reserves.lastpickupdate>>" somewhere. 2. Make a hold for a borrower. 3. Catch the hold and print the reserve slip. Verify that the lastpickupdate is actually the (current date + the 'Holds pickup wait' in your issuing rules) in days. ** ** ** ** ** ** *** *** *** TEST: RESPECTING CALENDAR HOLIDAYS * ** ** ** ** ** ** 1) Note your ReservesMaxPickUpDelay setting. 2) Create and trap a hold 3) View waitingreserves.pl, note the last pickup date, and the pickup location 4) Cancel this hold 5) Edit the calendar for the given location, and make the pickup date a single, unrepeated, holiday. Then make the next day a weekly repeating holiday. Then make the next two days an annual holiday repeatable every year. 6) Repeat step 2 7) View waitingreserves.pl, the last pickup date for this new hold should now be increased by four days. ** ** ** ** ** ** *** *** *** TEST: LETTER lastpickupdate PLACEHOLDER AND REGRESSION * ** ** ** ** ** ** 1. Modify the HOLD-letter, add <<reserves.lastpickupdate>> and <<reserves.waitingdate>> somewhere. 2. Catch a hold for a borrower. 3. See the members/notices.pl for the message queued and verify that the lastpickupdate-placeholder is set in your desired datetime format and that it is not on a holiday. Verify that the waitingdate is today. Conflicts: installer/data/mysql/updatedatabase.pl
Created attachment 79799 [details] [review] Bug 8367: Template fixes + fixes to C4/Reserves.pm and Koha/Hold.pm Fixed waitingreserves.tt template so the borrowernumber is successfully passed to waitingreserves.pl upon cancelling a hold, so users can now successfully cancel holds from the waitingreserves.pl interface. Removed check for ReserveMaxPickupdateDelay in Koha/Hold.pm as this syspref is removed in the previous patch. Instead the expirationdate is calculated the same way as the lastpickupdatedate that is retrieving the holdpickupwait value for the appropriate issuingrule and then adding that onto the waiting date. Same test plan as main patch Sponsored-By: Brimbank Library, Australia
Created attachment 79800 [details] [review] Bug 8367: Removed unneccessary warns and commented code Sponsored-By: Brimbank Library, Australia
Created attachment 79853 [details] [review] Bug 8367: Moved database update out of updatedatabase.pl into atomicupdates dir
Created attachment 79854 [details] [review] Bug 8367: Altered expirationdate calc in Koha/Hold.pm and altered C4/Reserves/CancelExpiredReserves() Added check for holidays to expirationdate calculation in Koha/Hold.pm so the expirationdate respects holidays as the lastpickupdate does. Changed C4/Reserve/CancelExpiredReserves() to check the reserves.lastpickupdate rather than rserves.expirationdate. Therefore it will now automatically expire holds if they have not been picked up within the time period specified in the hold pickup delay defined in the issuing rules (which is added to reserves.waitingdate to calculate the reserves.lastpickupdate) AND the ExpireReservesMaxPickUpDelay syspref is enabled. Sponsored-By: Brimbank Library, Australia
Created attachment 79855 [details] [review] Bug 8367: Fixed how expirationdate is calculated when ExcludeHolidaysFromMaxPickUpDelay syspref is enabled Sponsored-By: Brimbank Library, Australia
Created attachment 79859 [details] [review] Bug 8367: reserves.lastpickupdate calculation checks and uses ExcludeHolidaysFromMaxPickUpDelay syspref Sponsored-By: Brimbank Library, Australia
Hi Alex, is assigned the correct status here?
Is this bug still in process? We could really use the granularity to assign different wait items on different item types. Thank you
Created attachment 92412 [details] [review] Bug 8367: How long is a hold waiting for pickup at a more granular level You can now specify a pickup delay for an hold function of a patron category and/or a item type and/or a library. The lastpickupdate-column is set based on the given instruction and lands only on an open library day. Also cancel_expired_reserves.pl now cancels only on open library days after the last pickup date has expired. And waitingreserves.pl respects the new DB column. If ReservesMaxPickUpDelay is not set prior to the databaseupdate, the holdspickupwait-directive is never defined, thus disabling this feature until the holdspickupwait is set. If the issuingrules holdspickupwait is 0 or less, this feature is disabled. So each issuingrule can disable this feature for the material it matches, while being fully functional with other rules. This patch adds: - a new column in the issuing rules, including help topics. - a new column reserves.lastpickupdate (+old_reserves) It contains the waitingdate + the corresponding "holds pickup wait". Each time the waitingdate is modified, this value will be modified too. - a new field issuingrules.holdspickupwait - a new function C4::Reserves::MoveWaitingdate() to help in testing this feature. - a new function C4::Reserves::GetLastPickupDate() to calculate the proper date from Calendar. This patch removes: - the ReservesMaxPickUpDelay syspref. - the ExpireReservesOnHolidays syspref, because it became obsolete due to the changes in handling the last pickup date with Koha Calendar. - Also removes a code section from C4::Letters::_parseLetter() which worked blatantly disrespectfully towards Koha Calendar. - TODO figure out how to rename existing letter placeholders <<expirationdate>> with <<lastpickupdate>> Unit tests included. $$$$$$$$$$$$$ $ TEST PLAN $ $$$$$$$$$$$$$ 0. BEFORE UPDATING THE DATABASE! MAKE SURE THAT THE SYSPREF 'ReservesMaxPickUpDelay' is bigger than 0. Otherwise existing reserves won't get UPDATEd with the new lastpickupdate-values. ****** ****** *** *** *** TEST: BASIC WORKFLOW * ****** ****** 1. Check there is no regression with a normal reserve workflow. 2. Add one or more issuingrules. 3. Update the new column 'Holds pickup wait' in your issuing rules. 4. In the 3 templates of staff client, you can see the 'Last pickup date' for an hold (circ/circulation.pl, circ/waitingreserves.pl, members/moremember.pl) 5. In opac-user.pl the last pickup date is displayed in the status-column if available. 5. According to a library and an item type, the lastpickupdate value will be equal to the waiting date + the "holds pickup wait" defined moved to the next open library day. ****** ****** *** *** *** TEST: LETTER PLACEHOLDER (no code modifications here, just regression)* ****** ****** 1. Modify the RESERVESLIP->email Letter. Add "Last pickup date<<reserves.lastpickupdate>>" somewhere. 2. Make a hold for a borrower. 3. Catch the hold and print the reserve slip. Verify that the lastpickupdate is actually the (current date + the 'Holds pickup wait' in your issuing rules) in days. ** ** ** ** ** ** *** *** *** TEST: RESPECTING CALENDAR HOLIDAYS * ** ** ** ** ** ** 1) Note your ReservesMaxPickUpDelay setting. 2) Create and trap a hold 3) View waitingreserves.pl, note the last pickup date, and the pickup location 4) Cancel this hold 5) Edit the calendar for the given location, and make the pickup date a single, unrepeated, holiday. Then make the next day a weekly repeating holiday. Then make the next two days an annual holiday repeatable every year. 6) Repeat step 2 7) View waitingreserves.pl, the last pickup date for this new hold should now be increased by four days. ** ** ** ** ** ** *** *** *** TEST: LETTER lastpickupdate PLACEHOLDER AND REGRESSION * ** ** ** ** ** ** 1. Modify the HOLD-letter, add <<reserves.lastpickupdate>> and <<reserves.waitingdate>> somewhere. 2. Catch a hold for a borrower. 3. See the members/notices.pl for the message queued and verify that the lastpickupdate-placeholder is set in your desired datetime format and that it is not on a holiday. Verify that the waitingdate is today. Conflicts: installer/data/mysql/updatedatabase.pl
Created attachment 92413 [details] [review] Bug 8367: How long is a hold waiting for pickup at a more granular level You can now specify a pickup delay for an hold function of a patron category and/or a item type and/or a library. The lastpickupdate-column is set based on the given instruction and lands only on an open library day. Also cancel_expired_reserves.pl now cancels only on open library days after the last pickup date has expired. And waitingreserves.pl respects the new DB column. If ReservesMaxPickUpDelay is not set prior to the databaseupdate, the holdspickupwait-directive is never defined, thus disabling this feature until the holdspickupwait is set. If the issuingrules holdspickupwait is 0 or less, this feature is disabled. So each issuingrule can disable this feature for the material it matches, while being fully functional with other rules. This patch adds: - a new column in the issuing rules, including help topics. - a new column reserves.lastpickupdate (+old_reserves) It contains the waitingdate + the corresponding "holds pickup wait". Each time the waitingdate is modified, this value will be modified too. - a new field issuingrules.holdspickupwait - a new function C4::Reserves::MoveWaitingdate() to help in testing this feature. - a new function C4::Reserves::GetLastPickupDate() to calculate the proper date from Calendar. This patch removes: - the ReservesMaxPickUpDelay syspref. - the ExpireReservesOnHolidays syspref, because it became obsolete due to the changes in handling the last pickup date with Koha Calendar. - Also removes a code section from C4::Letters::_parseLetter() which worked blatantly disrespectfully towards Koha Calendar. - TODO figure out how to rename existing letter placeholders <<expirationdate>> with <<lastpickupdate>> Unit tests included. $$$$$$$$$$$$$ $ TEST PLAN $ $$$$$$$$$$$$$ 0. BEFORE UPDATING THE DATABASE! MAKE SURE THAT THE SYSPREF 'ReservesMaxPickUpDelay' is bigger than 0. Otherwise existing reserves won't get UPDATEd with the new lastpickupdate-values. ****** ****** *** *** *** TEST: BASIC WORKFLOW * ****** ****** 1. Check there is no regression with a normal reserve workflow. 2. Add one or more issuingrules. 3. Update the new column 'Holds pickup wait' in your issuing rules. 4. In the 3 templates of staff client, you can see the 'Last pickup date' for an hold (circ/circulation.pl, circ/waitingreserves.pl, members/moremember.pl) 5. In opac-user.pl the last pickup date is displayed in the status-column if available. 5. According to a library and an item type, the lastpickupdate value will be equal to the waiting date + the "holds pickup wait" defined moved to the next open library day. ****** ****** *** *** *** TEST: LETTER PLACEHOLDER (no code modifications here, just regression)* ****** ****** 1. Modify the RESERVESLIP->email Letter. Add "Last pickup date<<reserves.lastpickupdate>>" somewhere. 2. Make a hold for a borrower. 3. Catch the hold and print the reserve slip. Verify that the lastpickupdate is actually the (current date + the 'Holds pickup wait' in your issuing rules) in days. ** ** ** ** ** ** *** *** *** TEST: RESPECTING CALENDAR HOLIDAYS * ** ** ** ** ** ** 1) Note your ReservesMaxPickUpDelay setting. 2) Create and trap a hold 3) View waitingreserves.pl, note the last pickup date, and the pickup location 4) Cancel this hold 5) Edit the calendar for the given location, and make the pickup date a single, unrepeated, holiday. Then make the next day a weekly repeating holiday. Then make the next two days an annual holiday repeatable every year. 6) Repeat step 2 7) View waitingreserves.pl, the last pickup date for this new hold should now be increased by four days. ** ** ** ** ** ** *** *** *** TEST: LETTER lastpickupdate PLACEHOLDER AND REGRESSION * ** ** ** ** ** ** 1. Modify the HOLD-letter, add <<reserves.lastpickupdate>> and <<reserves.waitingdate>> somewhere. 2. Catch a hold for a borrower. 3. See the members/notices.pl for the message queued and verify that the lastpickupdate-placeholder is set in your desired datetime format and that it is not on a holiday. Verify that the waitingdate is today. Conflicts: installer/data/mysql/updatedatabase.pl
Created attachment 92414 [details] [review] Bug 8367: Template fixes + fixes to C4/Reserves.pm and Koha/Hold.pm Fixed waitingreserves.tt template so the borrowernumber is successfully passed to waitingreserves.pl upon cancelling a hold, so users can now successfully cancel holds from the waitingreserves.pl interface. Removed check for ReserveMaxPickupdateDelay in Koha/Hold.pm as this syspref is removed in the previous patch. Instead the expirationdate is calculated the same way as the lastpickupdatedate that is retrieving the holdpickupwait value for the appropriate issuingrule and then adding that onto the waiting date. Same test plan as main patch Sponsored-By: Brimbank Library, Australia
Created attachment 92415 [details] [review] Bug 8367: Removed unneccessary warns and commented code Sponsored-By: Brimbank Library, Australia
Created attachment 92416 [details] [review] Bug 8367: Template fixes + fixes to C4/Reserves.pm and Koha/Hold.pm Fixed waitingreserves.tt template so the borrowernumber is successfully passed to waitingreserves.pl upon cancelling a hold, so users can now successfully cancel holds from the waitingreserves.pl interface. Removed check for ReserveMaxPickupdateDelay in Koha/Hold.pm as this syspref is removed in the previous patch. Instead the expirationdate is calculated the same way as the lastpickupdatedate that is retrieving the holdpickupwait value for the appropriate issuingrule and then adding that onto the waiting date. Same test plan as main patch Sponsored-By: Brimbank Library, Australia
Created attachment 92417 [details] [review] Bug 8367: Removed unneccessary warns and commented code Sponsored-By: Brimbank Library, Australia
Created attachment 92418 [details] [review] Bug 8367: Moved database update out of updatedatabase.pl into atomicupdates dir
Created attachment 92419 [details] [review] Bug 8367: Altered expirationdate calc in Koha/Hold.pm and altered C4/Reserves/CancelExpiredReserves() Added check for holidays to expirationdate calculation in Koha/Hold.pm so the expirationdate respects holidays as the lastpickupdate does. Changed C4/Reserve/CancelExpiredReserves() to check the reserves.lastpickupdate rather than rserves.expirationdate. Therefore it will now automatically expire holds if they have not been picked up within the time period specified in the hold pickup delay defined in the issuing rules (which is added to reserves.waitingdate to calculate the reserves.lastpickupdate) AND the ExpireReservesMaxPickUpDelay syspref is enabled. Sponsored-By: Brimbank Library, Australia
Created attachment 92420 [details] [review] Bug 8367: Fixed how expirationdate is calculated when ExcludeHolidaysFromMaxPickUpDelay syspref is enabled Sponsored-By: Brimbank Library, Australia
Created attachment 92421 [details] [review] Bug 8367: reserves.lastpickupdate calculation checks and uses ExcludeHolidaysFromMaxPickUpDelay syspref Sponsored-By: Brimbank Library, Australia
Created attachment 92422 [details] [review] Bug 8367: (follow-up) Updates to DBIx class files Ran the update_dbix_class_files.pl script to regenerate the Koha/Schema/Result/ files again to match the database changes Making the atomicupdate file idempotent Sponsored-by: Brimbank Library, Australia Authored-by: Hayley Mapley and Alex Buckley
Created attachment 92423 [details] [review] Bug 8367: Fixes to make the reserves workflow work with new schema Sponsored-by: Brimbank library, Australia
Created attachment 92424 [details] [review] Bug 8367: Fixing display of lastpickupdate Changed circ/waitingreserves.pl to correctly display lastpickupdate in Circulation>Holds awaiting pickup. Changed includes/holds-table.inc to display lastpickupdate in opac-user.pl in Holds tab. Sponsored-by: Brimbank District Council Authored-by: Hayley Mapley and Alex Buckley (Catalyst IT)
Created attachment 92425 [details] [review] Bug 8367: How long is a hold waiting for pickup at a more granular level You can now specify a pickup delay for an hold function of a patron category and/or a item type and/or a library. The lastpickupdate-column is set based on the given instruction and lands only on an open library day. Also cancel_expired_reserves.pl now cancels only on open library days after the last pickup date has expired. And waitingreserves.pl respects the new DB column. If ReservesMaxPickUpDelay is not set prior to the databaseupdate, the holdspickupwait-directive is never defined, thus disabling this feature until the holdspickupwait is set. If the issuingrules holdspickupwait is 0 or less, this feature is disabled. So each issuingrule can disable this feature for the material it matches, while being fully functional with other rules. This patch adds: - a new column in the issuing rules, including help topics. - a new column reserves.lastpickupdate (+old_reserves) It contains the waitingdate + the corresponding "holds pickup wait". Each time the waitingdate is modified, this value will be modified too. - a new field issuingrules.holdspickupwait - a new function C4::Reserves::MoveWaitingdate() to help in testing this feature. - a new function C4::Reserves::GetLastPickupDate() to calculate the proper date from Calendar. This patch removes: - the ReservesMaxPickUpDelay syspref. - the ExpireReservesOnHolidays syspref, because it became obsolete due to the changes in handling the last pickup date with Koha Calendar. - Also removes a code section from C4::Letters::_parseLetter() which worked blatantly disrespectfully towards Koha Calendar. - TODO figure out how to rename existing letter placeholders <<expirationdate>> with <<lastpickupdate>> Unit tests included. $$$$$$$$$$$$$ $ TEST PLAN $ $$$$$$$$$$$$$ 0. BEFORE UPDATING THE DATABASE! MAKE SURE THAT THE SYSPREF 'ReservesMaxPickUpDelay' is bigger than 0. Otherwise existing reserves won't get UPDATEd with the new lastpickupdate-values. ****** ****** *** *** *** TEST: BASIC WORKFLOW * ****** ****** 1. Check there is no regression with a normal reserve workflow. 2. Add one or more issuingrules. 3. Update the new column 'Holds pickup wait' in your issuing rules. 4. In the following 3 templates of staff client, you can see the 'Last pickup date' for a hold -circ/circulation.pl, Circulation>Check outs>Search for Patron>Holds tab -circ/waitingreserves.pl Circulation>Holds awaiting pickup -members/moremember.pl Patrons>Search for patron>Holds tab 5. In opac-user.pl the last pickup date is displayed in the Status column if available. 6. According to a library and an item type, the lastpickupdate value will be equal to the waiting date + the "holds pickup wait" defined moved to the next open library day. ** ** ** ** ** ** *** *** *** TEST: RESPECTING CALENDAR HOLIDAYS * ** ** ** ** ** ** 1) Create and trap a hold 2) View waitingreserves.pl (Circulation>Holds awaiting pickup), note the last pickup date, and the pickup location 3) Cancel this hold 4) Edit the calendar for the given location, and make the pickup date a single, unrepeated, holiday. Then make the next day a weekly repeating holiday. Then make the next two days an annual holiday repeatable every year. 5) Repeat step 2 6) View waitingreserves.pl, the last pickup date for this new hold should now be increased by four days. ** ** ** ** ** ** *** *** *** TEST: LETTER lastpickupdate PLACEHOLDER AND REGRESSION * ** ** ** ** ** ** 1. Modify the HOLD_SLIP, add <<reserves.lastpickupdate>> and <<reserves.waitingdate>> somewhere. 2. Catch a hold for a borrower and print the slip. Verify that the lastpickupdate is actually (currentdate + the 'Holds pickup wait' in your issuing rules) in days. 3. See the members/notices.pl for the message queued and verify that the lastpickupdate-placeholder is set in your desired datetime format and that it is not on a holiday. Verify that the waitingdate is today. Originally Authored-by: Joonas Kylmälä <j.kylmala@gmail.com> Taken up by: Hayley Mapley and Alex Buckley (Catalyst IT) Sponsored-by: Brimbank District Council
Created attachment 92426 [details] [review] Bug 8367: Altered expirationdate calc in Koha/Hold.pm and altered C4/Reserves/CancelExpiredReserves() Added check for holidays to expirationdate calculation in Koha/Hold.pm so the expirationdate respects holidays as the lastpickupdate does. Changed C4/Reserve/CancelExpiredReserves() to check the reserves.lastpickupdate rather than rserves.expirationdate. Therefore it will now automatically expire holds if they have not been picked up within the time period specified in the hold pickup delay defined in the issuing rules (which is added to reserves.waitingdate to calculate the reserves.lastpickupdate) AND the ExpireReservesMaxPickUpDelay syspref is enabled. Sponsored-By: Brimbank Library, Australia
Created attachment 92427 [details] [review] Bug 8367: (follow-up) Updates to DBIx class files Ran the update_dbix_class_files.pl script to regenerate the Koha/Schema/Result/ files again to match the database changes Making the atomicupdate file idempotent Sponsored-by: Brimbank Library, Australia Authored-by: Hayley Mapley and Alex Buckley
Created attachment 92428 [details] [review] Bug 8367: Fixes to make the workflow of master fit, including display of lastpickupdate Changed circ/waitingreserves.pl to correctly display lastpickupdate in Circulation>Holds awaiting pickup. Changed includes/holds-table.inc to display lastpickupdate in opac-user.pl in Holds tab. Sponsored-by: Brimbank District Council Authored-by: Hayley Mapley and Alex Buckley (Catalyst IT)
There seems to be a bug in Koha/Hold.pm, set_waiting() where GetReservesControlBranch() is called as a method rather than a function causing the package name to become the first argument. This line: my $controlbranch = C4::Reserves->GetReservesControlBranch( $item, $patron ); Should be: my $controlbranch = C4::Reserves::GetReservesControlBranch( $item, $patron ); ...if I'm not mistaken. The current code causes an error with checkin on the pickup library for the book.
Created attachment 92958 [details] [review] Bug 8367: How long is a hold waiting for pickup at a more granular level You can now specify a pickup delay for an hold function of a patron category and/or a item type and/or a library. The lastpickupdate-column is set based on the given instruction and lands only on an open library day. Also cancel_expired_reserves.pl now cancels only on open library days after the last pickup date has expired. And waitingreserves.pl respects the new DB column. If ReservesMaxPickUpDelay is not set prior to the databaseupdate, the holdspickupwait-directive is never defined, thus disabling this feature until the holdspickupwait is set. If the issuingrules holdspickupwait is 0 or less, this feature is disabled. So each issuingrule can disable this feature for the material it matches, while being fully functional with other rules. This patch adds: - a new column in the issuing rules, including help topics. - a new column reserves.lastpickupdate (+old_reserves) It contains the waitingdate + the corresponding "holds pickup wait". Each time the waitingdate is modified, this value will be modified too. - a new field issuingrules.holdspickupwait - a new function C4::Reserves::MoveWaitingdate() to help in testing this feature. - a new function C4::Reserves::GetLastPickupDate() to calculate the proper date from Calendar. This patch removes: - the ReservesMaxPickUpDelay syspref. - the ExpireReservesOnHolidays syspref, because it became obsolete due to the changes in handling the last pickup date with Koha Calendar. - Also removes a code section from C4::Letters::_parseLetter() which worked blatantly disrespectfully towards Koha Calendar. - TODO figure out how to rename existing letter placeholders <<expirationdate>> with <<lastpickupdate>> Unit tests included. $$$$$$$$$$$$$ $ TEST PLAN $ $$$$$$$$$$$$$ 0. BEFORE UPDATING THE DATABASE! MAKE SURE THAT THE SYSPREF 'ReservesMaxPickUpDelay' is bigger than 0. Otherwise existing reserves won't get UPDATEd with the new lastpickupdate-values. ****** ****** *** *** *** TEST: BASIC WORKFLOW * ****** ****** 1. Check there is no regression with a normal reserve workflow. 2. Add one or more issuingrules. 3. Update the new column 'Holds pickup wait' in your issuing rules. 4. In the following 3 templates of staff client, you can see the 'Last pickup date' for a hold -circ/circulation.pl, Circulation>Check outs>Search for Patron>Holds tab -circ/waitingreserves.pl Circulation>Holds awaiting pickup -members/moremember.pl Patrons>Search for patron>Holds tab 5. In opac-user.pl the last pickup date is displayed in the Status column if available. 6. According to a library and an item type, the lastpickupdate value will be equal to the waiting date + the "holds pickup wait" defined moved to the next open library day. ** ** ** ** ** ** *** *** *** TEST: RESPECTING CALENDAR HOLIDAYS * ** ** ** ** ** ** 1) Create and trap a hold 2) View waitingreserves.pl (Circulation>Holds awaiting pickup), note the last pickup date, and the pickup location 3) Cancel this hold 4) Edit the calendar for the given location, and make the pickup date a single, unrepeated, holiday. Then make the next day a weekly repeating holiday. Then make the next two days an annual holiday repeatable every year. 5) Repeat step 2 6) View waitingreserves.pl, the last pickup date for this new hold should now be increased by four days. ** ** ** ** ** ** *** *** *** TEST: LETTER lastpickupdate PLACEHOLDER AND REGRESSION * ** ** ** ** ** ** 1. Modify the HOLD_SLIP, add <<reserves.lastpickupdate>> and <<reserves.waitingdate>> somewhere. 2. Catch a hold for a borrower and print the slip. Verify that the lastpickupdate is actually (currentdate + the 'Holds pickup wait' in your issuing rules) in days. 3. See the members/notices.pl for the message queued and verify that the lastpickupdate-placeholder is set in your desired datetime format and that it is not on a holiday. Verify that the waitingdate is today. Originally Authored-by: Joonas Kylmälä <j.kylmala@gmail.com> Taken up by: Hayley Mapley and Alex Buckley (Catalyst IT) Sponsored-by: Brimbank District Council
Created attachment 92959 [details] [review] Bug 8367: Altered expirationdate calc in Koha/Hold.pm and altered C4/Reserves/CancelExpiredReserves() Added check for holidays to expirationdate calculation in Koha/Hold.pm so the expirationdate respects holidays as the lastpickupdate does. Changed C4/Reserve/CancelExpiredReserves() to check the reserves.lastpickupdate rather than rserves.expirationdate. Therefore it will now automatically expire holds if they have not been picked up within the time period specified in the hold pickup delay defined in the issuing rules (which is added to reserves.waitingdate to calculate the reserves.lastpickupdate) AND the ExpireReservesMaxPickUpDelay syspref is enabled. Sponsored-By: Brimbank Library, Australia
Created attachment 92960 [details] [review] Bug 8367: (follow-up) Updates to DBIx class files Ran the update_dbix_class_files.pl script to regenerate the Koha/Schema/Result/ files again to match the database changes Making the atomicupdate file idempotent Sponsored-by: Brimbank Library, Australia Authored-by: Hayley Mapley and Alex Buckley
Created attachment 92961 [details] [review] Bug 8367: Fixes to make the workflow of master fit, including display of lastpickupdate Changed circ/waitingreserves.pl to correctly display lastpickupdate in Circulation>Holds awaiting pickup. Changed includes/holds-table.inc to display lastpickupdate in opac-user.pl in Holds tab. Sponsored-by: Brimbank District Council Authored-by: Hayley Mapley and Alex Buckley (Catalyst IT)
It seems like this block was incorrectly removed, probably as part of recent rebase: - [% UNLESS ( borrowers ) %] - [% IF borrowernumber and patron %] - <div class="col-sm-2 col-sm-pull-10"> - <aside> - [% INCLUDE 'circ-menu.inc' %] - </aside> - </div> <!-- /.col-sm-2 col-sm-pull-10 --> - [% END %] - [% END %] This makes the sidebar menu disappear on the circulation page. Removed this change from the "Bug 8367: How long is a hold waiting for pickup at a more granular level"-commit.
I'm really unclear of the relationship between the two columns lastpickupdate and expirationdate from this patch. This patch introduces lastpickupdate, but set_waiting() sets expirationdate. Cancelling of an expired hold is then checked against lastpickupdate (used to be expirationdate). All of the rest of the Koha code that is not affected by this patch uses expirationdate. Why is lastpickupdate there? Why is cancelling comparing against something that set_waiting() didn't set?
This patch also explicitly sets 'W' without checking if it should be transferred or not. To be more specific, it sets 'T' for transfer, then immediately after sets 'W' anyway, using an added ModReserveStatus() call.
Created attachment 103425 [details] [review] Bug 8367: Add holds_pickup_period circulation rule So that pickup delay can have a different value per patron category, item type or branch. To test: 1) Update database, restart services 2) Set ReservesMaxPickUpDelay syspref (if not already set) 3) Edit your circulation rules and set a value under 'Holds pickup period (day) that is DIFFERENT from ReservesMaxPickUpDelay 4) Place a hold on a biblio 5) Check in an item from that biblio and confirm the hold as waiting 6) Confirm the expiration date is calculated using the 'Holds pickup period' value instead of the ReservesMaxPickUpDelay syspref 7) Revert the waiting status and delete the hold 8) Re-place the hold on the biblio 9) Create a holiday with a date that will overlap with the 'Holds pickup period' 10) Check in an item from that biblio and confirm the hold as waiting 11) Confirm the expiration date is calculated using the 'Holds pickup period' value AND considers the special holiday 12) Confirm tests pass t/db_dependent/Holds/WaitingReserves.t I've also added this to the TalkingTech_itiva_outbound.pl cronjob, I'm not sure how to test this. Please test properly if you can, otherwise maybe put some warns in and confirm the correct expiration date is calculated. Sponsored-by: Catalyst IT
Patch doesn't apply 8-(.. (The status was changed earlier in the month - not sure whether just changing the status sends a message or not).
(In reply to David Nind from comment #138) > Patch doesn't apply 8-(.. > > (The status was changed earlier in the month - not sure whether just > changing the status sends a message or not). It does! :) This is on my list to get to when I have time.
Created attachment 113211 [details] [review] Bug 8367: Add holds_pickup_period circulation rule So that pickup delay can have a different value per patron category, item type or branch. To test: 1) Update database, restart services 2) Set ReservesMaxPickUpDelay syspref (if not already set) 3) Edit your circulation rules and set a value under 'Holds pickup period (day) that is DIFFERENT from ReservesMaxPickUpDelay 4) Place a hold on a biblio 5) Check in an item from that biblio and confirm the hold as waiting 6) Confirm the expiration date is calculated using the 'Holds pickup period' value instead of the ReservesMaxPickUpDelay syspref 7) Revert the waiting status and delete the hold 8) Re-place the hold on the biblio 9) Create a holiday with a date that will overlap with the 'Holds pickup period' 10) Check in an item from that biblio and confirm the hold as waiting 11) Confirm the expiration date is calculated using the 'Holds pickup period' value AND considers the special holiday 12) Confirm tests pass t/db_dependent/Holds/WaitingReserves.t I've also added this to the TalkingTech_itiva_outbound.pl cronjob, I'm not sure how to test this. Please test properly if you can, otherwise maybe put some warns in and confirm the correct expiration date is calculated. Sponsored-by: Catalyst IT
Patch doesn't apply.
Created attachment 117284 [details] [review] Bug 8367: Add holds_pickup_period circulation rule So that pickup delay can have a different value per patron category, item type or branch. To test: 1) Update database, restart services 2) Set ReservesMaxPickUpDelay syspref (if not already set) 3) Edit your circulation rules and set a value under 'Holds pickup period (day) that is DIFFERENT from ReservesMaxPickUpDelay 4) Place a hold on a biblio 5) Check in an item from that biblio and confirm the hold as waiting 6) Confirm the expiration date is calculated using the 'Holds pickup period' value instead of the ReservesMaxPickUpDelay syspref 7) Revert the waiting status and delete the hold 8) Re-place the hold on the biblio 9) Create a holiday with a date that will overlap with the 'Holds pickup period' 10) Check in an item from that biblio and confirm the hold as waiting 11) Confirm the expiration date is calculated using the 'Holds pickup period' value AND considers the special holiday 12) Confirm tests pass t/db_dependent/Holds/WaitingReserves.t I've also added this to the TalkingTech_itiva_outbound.pl cronjob, I'm not sure how to test this. Please test properly if you can, otherwise maybe put some warns in and confirm the correct expiration date is calculated. Sponsored-by: Catalyst IT
According to the bug 9004 you can test TalkingTech_itiva_outbound.pl by running: perl /path/to/koha/misc/cronjobs/thirdparty/TalkingTech_itiva_outbound.pl -o ~/itiva.tmp -w 0 --type=RESERVE. First enable "TalkingTechItivaPhoneNotification" and add content to HOLD slips Phone (itiva) part. However it doesn't seem to take holidays into account. Also there's a warning: > "my" variable $item masks earlier declaration in same scope at misc/cronjobs/thirdparty/TalkingTech_itiva_outbound.pl line 347.
I love this feature! While testing, I did find something that I think would need to change to make this work more efficiently- currently when a library is using their 'Holds Awaiting Pickup' report, there are two tabs. Current Holds and Holds that are Watinging over X number of days. This X is pulled from the system preference, ReservesMaxPickUpDelay. However, now that Koha is giving the ability to have different pickup delays, could we change the tab to say " Holds Past Expiration Date"? I also would love to see the expiration date to this report - however, I can file a separate bug for this if this is out of the scope. In addition, this should be documented that the Pickup Location is what is driving the rule- so where the patron chooses to pick the hold up is what rule this will follow. Finally, I am not sure about the itiva cron- I also saw the same issue as Emmi did while running this.
(In reply to Kelly McElligott from comment #144) > I love this feature! While testing, I did find something that I think would > need to change to make this work more efficiently- currently when a library > is using their 'Holds Awaiting Pickup' report, there are two tabs. Current > Holds and Holds that are Watinging over X number of days. This X is pulled > from the system preference, ReservesMaxPickUpDelay. However, now that Koha > is giving the ability to have different pickup delays, could we change the > tab to say " Holds Past Expiration Date"? I also would love to see the > expiration date to this report - however, I can file a separate bug for this > if this is out of the scope. This is really great feature idea and our librarians have been asking for it too! :) I would probably file it as separate bug, as it already now applies to the current solution, doesn't it? And maybe we would want to show the holds past expiration day on a day by day basis, like holds expired 2020-03-03, 2020-03-04 and so on (up until like 5 opening days? Or some configuration value for how many days to show?).
Failing QA because of the itiva cron, will come back to this
I would say to keep it to two different tabs, one with active holds and ones that are past their expiration date, maybe something like, Holds Past Expiration Date. If we have more than 2 tabs, that could potentially be a lot of clicking for librarians to check every tab!
Created attachment 118017 [details] [review] Bug 8367: Add holds_pickup_period circulation rule So that pickup delay can have a different value per patron category, item type or branch. To test: 1) Update database, restart services 2) Set ReservesMaxPickUpDelay syspref (if not already set) 3) Edit your circulation rules and set a value under 'Holds pickup period (day) that is DIFFERENT from ReservesMaxPickUpDelay. Set a few different numbers for different branches as well. 4) Place a hold on a biblio from the staff client. 5) Check in an item from that biblio and confirm the hold as waiting 6) Confirm the expiration date is calculated using the 'Holds pickup period' value instead of the ReservesMaxPickUpDelay syspref 7) Revert the waiting status and delete the hold 8) Re-place the hold on the biblio on the OPAC. Notice that when you change the pick up location, the number of days in the pickup message below the dropdown changes based on the circ rules. 9) Create a holiday with a date that will overlap with the 'Holds pickup period' 10) Check in an item from that biblio and confirm the hold as waiting 11) Confirm the expiration date is calculated using the 'Holds pickup period' value AND considers the special holiday 12) Confirm tests pass t/db_dependent/Holds/WaitingReserves.t 13) Test Talking Tech: 13a) Enable TalkingTechItivaPhoneNotification 13b) Go to Tools -> Notices & slips. Add content to the HOLD phone (itiva) notice. 13c) In your terminal, run perl /path/to/koha/misc/cronjobs/thirdparty/TalkingTech_itiva_outbound.pl -o ~/itiva.tmp -w 0 --type=RESERVE Sponsored-by: Catalyst IT
Created attachment 118021 [details] [review] Bug 8367: Add holds_pickup_period circulation rule So that pickup delay can have a different value per patron category, item type or branch. To test: 1) Update database, restart services 2) Set ReservesMaxPickUpDelay syspref (if not already set) 3) Edit your circulation rules and set a value under 'Holds pickup period (day) that is DIFFERENT from ReservesMaxPickUpDelay. Set a few different numbers for different branches as well. 4) Place a hold on a biblio from the staff client. 5) Check in an item from that biblio and confirm the hold as waiting 6) Confirm the expiration date is calculated using the 'Holds pickup period' value instead of the ReservesMaxPickUpDelay syspref 7) Revert the waiting status and delete the hold 8) Re-place the hold on the biblio on the OPAC. Notice that when you change the pick up location, the number of days in the pickup message below the dropdown changes based on the circ rules. 9) Create a holiday with a date that will overlap with the 'Holds pickup period' 10) Check in an item from that biblio and confirm the hold as waiting 11) Confirm the expiration date is calculated using the 'Holds pickup period' value AND considers the special holiday 12) Confirm tests pass t/db_dependent/Holds/WaitingReserves.t 13) Test Talking Tech: 13a) Enable TalkingTechItivaPhoneNotification 13b) Go to Tools -> Notices & slips. Add content to the HOLD phone (itiva) notice. 13c) In your terminal, run perl /path/to/koha/misc/cronjobs/thirdparty/TalkingTech_itiva_outbound.pl -o ~/itiva.tmp -w 0 --type=RESERVE Sponsored-by: Catalyst IT Signed-off-by: Emmi Takkinen <emmi.takkinen@koha-suomi.fi>
What is the status of this fix? We are eager to have this functionality for our hold lockers. Rhonda Kuiper
(In reply to Rhonda Kuiper from comment #150) > What is the status of this fix? We are eager to have this functionality for > our hold lockers. > Rhonda Kuiper At the moment this is waiting for QA. But as we are just about to release 21.05 it might wait a little longer while we focus on fixing bugs in the upcoming release.
Rhonda, it could help QA to add another signoff on the patch :)
I started the signoff and everything worked great on the staff side. The problem I saw was on step 8 of the test plan. When I went to put a biblio on hold the message on the hold screen said "When your item is ready for pickup, you will have days to pick it up from this library". There was no number before the 'days'. I tried changing the library but the message still read the same. Also, when I set a holiday within the pickup time, I put an item on hold and the expiration date didn't change (extend for one more day). I didn't get to step 13 with the talking tech test. Not sure what status to change this to... Rhonda K.
If you think you found a problem, which it reads like, setting "Failed QA" is always the way to go. It usually alerts the dev to check what's going on. It's really a status for everyone to use.
We're interested in this one too. I might be able to take a look at this one.
The bug_8367-add_holds_pickup_period_circrule.perl script is not idempotent. It shouldn't re-add the rule on subsequent runs.
Created attachment 124568 [details] [review] Bug 8367: Make database update idempotent
I'm finding this test plan a bit difficult to follow. What does "7) Revert the waiting status" mean? -- Repeating Rhonda's mention that the pickup message doesn't change for "8) Re-place the hold on the biblio on the OPAC. Notice that when you change the pick up location, the number of days in the pickup message below the dropdown changes based on the circ rules." Not seeing any relevant Javascript errors. I'll look into that next...
Comment on attachment 118021 [details] [review] Bug 8367: Add holds_pickup_period circulation rule Review of attachment 118021 [details] [review]: ----------------------------------------------------------------- ::: koha-tmpl/opac-tmpl/bootstrap/en/modules/opac-reserve.tt @@ +486,5 @@ > $(radios).first().attr("checked", "checked"); > } > > + function calculate_delay_days(){ > + var selected_branch = $("#branch_1").val(); This #branch_1 is wrong, as the HTML is populated using "branch_[% bibitemloo.biblionumber | html %]". The current code would only ever work for biblio 1.
Comment on attachment 118021 [details] [review] Bug 8367: Add holds_pickup_period circulation rule Review of attachment 118021 [details] [review]: ----------------------------------------------------------------- ::: koha-tmpl/opac-tmpl/bootstrap/en/modules/opac-reserve.tt @@ +272,5 @@ > </ul> > > + [% IF pickup_delays %] > + <p id="pickup_delay" style="display:none;">When your item is ready for pickup, you will have <span id="pickup_delay_days"></span> days to pick it up from this library.</p> > + [% END %] This doesn't display for any bibs but the first one if you have multiple bibs to place holds on. @@ +490,5 @@ > + var selected_branch = $("#branch_1").val(); > + [% FOREACH branchcode IN pickup_delays.keys %] > + var branchcode = "[% branchcode | html %]"; > + if ( branchcode == selected_branch ) { > + $("#pickup_delay_days").text([% pickup_delays.$branchcode | html %]); #pickup_delay_days can't work because it's not biblio-specific, so this will break if you're placing holds on more than 1 bib at a time. @@ +499,4 @@ > $(document).ready(function() { > + calculate_delay_days(); > + $("#pickup_delay").show(); > + $("#branch_1").change(function(){ This has the same selector problem because it's hard-coded to biblionumber 1. ::: opac/opac-reserve.pl @@ +694,5 @@ > + }); > + $hold_pickup_delay_by_branch{$branch->branchcode} = $rule->rule_value if ( $rule and $rule->rule_value ); > +} > + > +$template->param( pickup_delays => \%hold_pickup_delay_by_branch ); "pickup_delays" is a confusing name, and it looks like there's no consideration of ReservesMaxPickUpDelay here, even though Hold.pm seems to take it into consideration... which could lead to confusing displays.
I don't think this patch takes into account C4::Reserves::CancelExpiredReserves which gets called by the cronjob /usr/share/koha/bin/cronjobs/holds/cancel_expired_holds.pl
Actually, I don't think we really need the changes to opac-reserve.pl and opac-reserve.tt. Those changes just add complexity. While they give patrons a little bit more information about when the hold expires, I don't know that we need to give that information at that point.
(In reply to David Cook from comment #161) > I don't think this patch takes into account > C4::Reserves::CancelExpiredReserves which gets called by the cronjob > /usr/share/koha/bin/cronjobs/holds/cancel_expired_holds.pl Actually, that's irrelevant. I was mixing up "ReservesMaxPickUpDelay" and "ExpireReservesMaxPickUpDelay". The description of "ExpireReservesMaxPickUpDelay" is actually a bit misleading and isn't relevant after all (since a user could nominate their own expiry regardless of ReservesMaxPickUpDelay).
(In reply to David Cook from comment #157) > Created attachment 124568 [details] [review] [review] > Bug 8367: Make database update idempotent This query works on MariaDB 10.1.48 (used by koha-testing-docker), but it's not working for MariaDB 10.3.29 out in the wild on Debian 10...
Created attachment 125087 [details] [review] Bug 8367: Make database update idempotent (across different MariaDB versions) The previous query seemed to work on MariaDB 10.1.48 (used by koha-testing-docker), but it didn't work on MariaDB 10.3.29 (default in Debian 10 as of this writing), but this patched query works on both indempotently.
What's the status of this bug? Is it still Failed QA or could it be changed as Needs Signoff?
Created attachment 131299 [details] [review] Bug 8367: Add holds_pickup_period circulation rule So that pickup delay can have a different value per patron category, item type or branch. To test: 1) Update database, restart services 2) Set ReservesMaxPickUpDelay syspref (if not already set) 3) Edit your circulation rules and set a value under 'Holds pickup period (day) that is DIFFERENT from ReservesMaxPickUpDelay. Set a few different numbers for different branches as well. 4) Place a hold on a biblio from the staff client. 5) Check in an item from that biblio and confirm the hold as waiting 6) Confirm the expiration date is calculated using the 'Holds pickup period' value instead of the ReservesMaxPickUpDelay syspref 7) Revert the waiting status and delete the hold 8) Re-place the hold on the biblio on the OPAC. Notice that when you change the pick up location, the number of days in the pickup message below the dropdown changes based on the circ rules. 9) Create a holiday with a date that will overlap with the 'Holds pickup period' 10) Check in an item from that biblio and confirm the hold as waiting 11) Confirm the expiration date is calculated using the 'Holds pickup period' value AND considers the special holiday 12) Confirm tests pass t/db_dependent/Holds/WaitingReserves.t 13) Test Talking Tech: 13a) Enable TalkingTechItivaPhoneNotification 13b) Go to Tools -> Notices & slips. Add content to the HOLD phone (itiva) notice. 13c) In your terminal, run perl /path/to/koha/misc/cronjobs/thirdparty/TalkingTech_itiva_outbound.pl -o ~/itiva.tmp -w 0 --type=RESERVE Sponsored-by: Catalyst IT Signed-off-by: Emmi Takkinen <emmi.takkinen@koha-suomi.fi>
Created attachment 131300 [details] [review] Bug 8367: Make database update idempotent
Created attachment 131301 [details] [review] Bug 8367: Make database update idempotent (across different MariaDB versions) The previous query seemed to work on MariaDB 10.1.48 (used by koha-testing-docker), but it didn't work on MariaDB 10.3.29 (default in Debian 10 as of this writing), but this patched query works on both indempotently.
Created attachment 131302 [details] [review] Bug 8367: (follow-up) Fix pickup delay text on OPAC and other bits
Sorry but I'm unable to repeat step 8, since number of days doesn't change when I change library. It seems number is based on "Standard rules for all libraries", not individual rules as described. After deleting standard rule, number of days is set by ReservesMaxPickUpDelay. Also tests fail for me: not ok 12 - Expiration date should be set to today + 3 based on circulation rules # Failed test 'Expiration date should be set to today + 3 based on circulation rules' # at t/db_dependent/Holds/WaitingReserves.t line 244. # got: '2022-03-07' # expected: '2022-03-10' not ok 13 - Expiration date should be set to today + 4 based on circulation rules and including a holiday # Failed test 'Expiration date should be set to today + 4 based on circulation rules and including a holiday' # at t/db_dependent/Holds/WaitingReserves.t line 266. # got: '2022-03-07' # expected: '2022-03-12' Other than these, this seems to be working as described.
Created attachment 131646 [details] [review] Bug 8367: (follow-up) Remove pickup delay message from OPAC It adds unnecessary complexity and information.
Created attachment 131647 [details] [review] Bug 8367: (follow-up) Fix tests
Created attachment 131718 [details] [review] Bug 8367: Add holds_pickup_period circulation rule So that pickup delay can have a different value per patron category, item type or branch. To test: 1) Update database, restart services 2) Set ReservesMaxPickUpDelay syspref (if not already set) 3) Edit your circulation rules and set a value under 'Holds pickup period (day) that is DIFFERENT from ReservesMaxPickUpDelay. Set a few different numbers for different branches as well. 4) Place a hold on a biblio from the staff client. 5) Check in an item from that biblio and confirm the hold as waiting 6) Confirm the expiration date is calculated using the 'Holds pickup period' value instead of the ReservesMaxPickUpDelay syspref 7) Revert the waiting status and delete the hold 8) Re-place the hold on the biblio on the OPAC. Notice that when you change the pick up location, the number of days in the pickup message below the dropdown changes based on the circ rules. 9) Create a holiday with a date that will overlap with the 'Holds pickup period' 10) Check in an item from that biblio and confirm the hold as waiting 11) Confirm the expiration date is calculated using the 'Holds pickup period' value AND considers the special holiday 12) Confirm tests pass t/db_dependent/Holds/WaitingReserves.t 13) Test Talking Tech: 13a) Enable TalkingTechItivaPhoneNotification 13b) Go to Tools -> Notices & slips. Add content to the HOLD phone (itiva) notice. 13c) In your terminal, run perl /path/to/koha/misc/cronjobs/thirdparty/TalkingTech_itiva_outbound.pl -o ~/itiva.tmp -w 0 --type=RESERVE Sponsored-by: Catalyst IT Signed-off-by: Emmi Takkinen <emmi.takkinen@koha-suomi.fi>
Created attachment 131719 [details] [review] Bug 8367: Make database update idempotent Signed-off-by: Emmi Takkinen <emmi.takkinen@koha-suomi.fi>
Created attachment 131720 [details] [review] Bug 8367: Make database update idempotent (across different MariaDB versions) but it didn't work on MariaDB 10.3.29 (default in Debian 10 as of this writing), but this patched query works on both indempotently. Signed-off-by: Emmi Takkinen <emmi.takkinen@koha-suomi.fi>
Created attachment 131722 [details] [review] Bug 8367: (follow-up) Fix pickup delay text on OPAC and other bits Signed-off-by: Emmi Takkinen <emmi.takkinen@koha-suomi.fi>
Created attachment 131723 [details] [review] Bug 8367: (follow-up) Remove pickup delay message from OPAC It adds unnecessary complexity and information. Signed-off-by: Emmi Takkinen <emmi.takkinen@koha-suomi.fi>
Created attachment 131724 [details] [review] Bug 8367: (follow-up) Fix tests Signed-off-by: Emmi Takkinen <emmi.takkinen@koha-suomi.fi>
Tested, works as described.
Created attachment 133089 [details] [review] Bug 8367: Add holds_pickup_period circulation rule So that pickup delay can have a different value per patron category, item type or branch. To test: 1) Update database, restart services 2) Set ReservesMaxPickUpDelay syspref (if not already set) 3) Edit your circulation rules and set a value under 'Holds pickup period (day) that is DIFFERENT from ReservesMaxPickUpDelay. Set a few different numbers for different branches as well. 4) Place a hold on a biblio from the staff client. 5) Check in an item from that biblio and confirm the hold as waiting 6) Confirm the expiration date is calculated using the 'Holds pickup period' value instead of the ReservesMaxPickUpDelay syspref 7) Revert the waiting status and delete the hold 8) Re-place the hold on the biblio on the OPAC. Notice that when you change the pick up location, the number of days in the pickup message below the dropdown changes based on the circ rules. 9) Create a holiday with a date that will overlap with the 'Holds pickup period' 10) Check in an item from that biblio and confirm the hold as waiting 11) Confirm the expiration date is calculated using the 'Holds pickup period' value AND considers the special holiday 12) Confirm tests pass t/db_dependent/Holds/WaitingReserves.t 13) Test Talking Tech: 13a) Enable TalkingTechItivaPhoneNotification 13b) Go to Tools -> Notices & slips. Add content to the HOLD phone (itiva) notice. 13c) In your terminal, run perl /path/to/koha/misc/cronjobs/thirdparty/TalkingTech_itiva_outbound.pl -o ~/itiva.tmp -w 0 --type=RESERVE Sponsored-by: Catalyst IT Signed-off-by: Emmi Takkinen <emmi.takkinen@koha-suomi.fi>
Created attachment 133090 [details] [review] Bug 8367: Make database update idempotent Signed-off-by: Emmi Takkinen <emmi.takkinen@koha-suomi.fi>
Created attachment 133091 [details] [review] Bug 8367: Make database update idempotent (across different MariaDB versions) The previous query seemed to work on MariaDB 10.1.48 (used by koha-testing-docker), but it didn't work on MariaDB 10.3.29 (default in Debian 10 as of this writing), but this patched query works on both indempotently. Signed-off-by: Emmi Takkinen <emmi.takkinen@koha-suomi.fi>
Created attachment 133092 [details] [review] Bug 8367: (follow-up) Fix pickup delay text on OPAC and other bits Signed-off-by: Emmi Takkinen <emmi.takkinen@koha-suomi.fi>
Created attachment 133093 [details] [review] Bug 8367: (follow-up) Remove pickup delay message from OPAC It adds unnecessary complexity and information. Signed-off-by: Emmi Takkinen <emmi.takkinen@koha-suomi.fi>
Created attachment 133094 [details] [review] Bug 8367: (follow-up) Fix tests Signed-off-by: Emmi Takkinen <emmi.takkinen@koha-suomi.fi>
Hi Aleisha, This looks great, but I found a few issues: 1 - Needs a minor rebase on top of article request rules 2 - The changes to iTiva outbound should be rebased on 31356 - essentially they aren't needed as we should use the already calculated expiration date 3 - I think the atomic update can simply be dropped - if the rule is not defined, it won't be used, no need to set it as blank 4 - When saving a more specific rule it is saved as blank, this still makes it the effective rule i.e. - Set a default rule with "Holds pickup period" as 25 - Set syspref ReservesMaxPickUpDelay to 1 - Set an branch specific all/all rule leaving "Holds pickup period" blank - Place two holds - one for pickup at branch with specific rule, one at another - Sign in as the correct libraries and check in items and confirm holds - Branch with specific rule - hold gets 1 day delay - Other branch - hold gets 25 day delay
*** Bug 19309 has been marked as a duplicate of this bug. ***
Created attachment 142897 [details] [review] Bug 8367: Add holds_pickup_period circulation rule So that pickup delay can have a different value per patron category, item type or branch. To test: 1) Update database, restart services 2) Set ReservesMaxPickUpDelay syspref (if not already set) 3) Edit your circulation rules and set a value under 'Holds pickup period (day) that is DIFFERENT from ReservesMaxPickUpDelay. Set a few different numbers for different branches as well. 4) Place a hold on a biblio from the staff client. 5) Check in an item from that biblio and confirm the hold as waiting 6) Confirm the expiration date is calculated using the 'Holds pickup period' value instead of the ReservesMaxPickUpDelay syspref 7) Revert the waiting status and delete the hold 8) Re-place the hold on the biblio on the OPAC. Notice that when you change the pick up location, the number of days in the pickup message below the dropdown changes based on the circ rules. 9) Create a holiday with a date that will overlap with the 'Holds pickup period' 10) Check in an item from that biblio and confirm the hold as waiting 11) Confirm the expiration date is calculated using the 'Holds pickup period' value AND considers the special holiday 12) Confirm tests pass t/db_dependent/Holds/WaitingReserves.t 13) Test Talking Tech: 13a) Enable TalkingTechItivaPhoneNotification 13b) Go to Tools -> Notices & slips. Add content to the HOLD phone (itiva) notice. 13c) In your terminal, run perl /path/to/koha/misc/cronjobs/thirdparty/TalkingTech_itiva_outbound.pl -o ~/itiva.tmp -w 0 --type=RESERVE Sponsored-by: Catalyst IT Signed-off-by: Emmi Takkinen <emmi.takkinen@koha-suomi.fi>
Created attachment 142898 [details] [review] Bug 8367: Make database update idempotent Signed-off-by: Emmi Takkinen <emmi.takkinen@koha-suomi.fi>
Created attachment 142899 [details] [review] Bug 8367: Make database update idempotent (across different MariaDB versions) The previous query seemed to work on MariaDB 10.1.48 (used by koha-testing-docker), but it didn't work on MariaDB 10.3.29 (default in Debian 10 as of this writing), but this patched query works on both indempotently. Signed-off-by: Emmi Takkinen <emmi.takkinen@koha-suomi.fi>
Created attachment 142900 [details] [review] Bug 8367: (follow-up) Fix pickup delay text on OPAC and other bits Signed-off-by: Emmi Takkinen <emmi.takkinen@koha-suomi.fi>
Created attachment 142901 [details] [review] Bug 8367: (follow-up) Remove pickup delay message from OPAC It adds unnecessary complexity and information. Signed-off-by: Emmi Takkinen <emmi.takkinen@koha-suomi.fi>
Created attachment 142902 [details] [review] Bug 8367: (follow-up) Fix tests Signed-off-by: Emmi Takkinen <emmi.takkinen@koha-suomi.fi>
Created attachment 142903 [details] [review] Bug 8367: (follow-up) Remove unnecessary code and atomic update
Patches no longer apply. Applying: Bug 8367: Add holds_pickup_period circulation rule Using index info to reconstruct a base tree... M Koha/Hold.pm M koha-tmpl/intranet-tmpl/prog/en/modules/admin/smart-rules.tt M koha-tmpl/opac-tmpl/bootstrap/en/modules/opac-reserve.tt M opac/opac-reserve.pl Falling back to patching base and 3-way merge... Auto-merging opac/opac-reserve.pl Auto-merging koha-tmpl/opac-tmpl/bootstrap/en/modules/opac-reserve.tt Auto-merging koha-tmpl/intranet-tmpl/prog/en/modules/admin/smart-rules.tt CONFLICT (content): Merge conflict in koha-tmpl/intranet-tmpl/prog/en/modules/admin/smart-rules.tt Auto-merging Koha/Hold.pm error: Failed to merge in the changes. Patch failed at 0001 Bug 8367: Add holds_pickup_period circulation rule hint: Use 'git am --show-current-patch=diff' to see the failed patch When you have resolved this problem run "git bz apply --continue". If you would prefer to skip this patch, instead run "git bz apply --skip". To restore the original branch and stop patching run "git bz apply --abort". Patch left in /tmp/Bug-8367-Add-holdspickupperiod-circulation-rule-PsnpwK.patch
I'll try to keep my eyes upon to QA this one when it's ready.
Note that there is a bug here similar to bug 30694 but with holds_pickup_period.
Created attachment 144097 [details] [review] Bug 8367: Add holds_pickup_period circulation rule So that pickup delay can have a different value per patron category, item type or branch. To test: 1) Update database, restart services 2) Set ReservesMaxPickUpDelay syspref (if not already set) 3) Edit your circulation rules and set a value under 'Holds pickup period (day) that is DIFFERENT from ReservesMaxPickUpDelay. Set a few different numbers for different branches as well. 4) Place a hold on a biblio from the staff client. 5) Check in an item from that biblio and confirm the hold as waiting 6) Confirm the expiration date is calculated using the 'Holds pickup period' value instead of the ReservesMaxPickUpDelay syspref 7) Revert the waiting status and delete the hold 8) Re-place the hold on the biblio on the OPAC. Notice that when you change the pick up location, the number of days in the pickup message below the dropdown changes based on the circ rules. 9) Create a holiday with a date that will overlap with the 'Holds pickup period' 10) Check in an item from that biblio and confirm the hold as waiting 11) Confirm the expiration date is calculated using the 'Holds pickup period' value AND considers the special holiday 12) Confirm tests pass t/db_dependent/Holds/WaitingReserves.t 13) Test Talking Tech: 13a) Enable TalkingTechItivaPhoneNotification 13b) Go to Tools -> Notices & slips. Add content to the HOLD phone (itiva) notice. 13c) In your terminal, run perl /path/to/koha/misc/cronjobs/thirdparty/TalkingTech_itiva_outbound.pl -o ~/itiva.tmp -w 0 --type=RESERVE Sponsored-by: Catalyst IT Signed-off-by: Emmi Takkinen <emmi.takkinen@koha-suomi.fi>
Created attachment 144098 [details] [review] Bug 8367: Make database update idempotent Signed-off-by: Emmi Takkinen <emmi.takkinen@koha-suomi.fi>
Created attachment 144099 [details] [review] Bug 8367: Make database update idempotent (across different MariaDB versions) The previous query seemed to work on MariaDB 10.1.48 (used by koha-testing-docker), but it didn't work on MariaDB 10.3.29 (default in Debian 10 as of this writing), but this patched query works on both indempotently. Signed-off-by: Emmi Takkinen <emmi.takkinen@koha-suomi.fi>
Created attachment 144100 [details] [review] Bug 8367: (follow-up) Fix pickup delay text on OPAC and other bits Signed-off-by: Emmi Takkinen <emmi.takkinen@koha-suomi.fi>
Created attachment 144101 [details] [review] Bug 8367: (follow-up) Remove pickup delay message from OPAC It adds unnecessary complexity and information. Signed-off-by: Emmi Takkinen <emmi.takkinen@koha-suomi.fi>
Created attachment 144102 [details] [review] Bug 8367: (follow-up) Fix tests Signed-off-by: Emmi Takkinen <emmi.takkinen@koha-suomi.fi>
Created attachment 144103 [details] [review] Bug 8367: (follow-up) Remove unnecessary code and atomic update
Created attachment 144146 [details] [review] Bug 8367: (follow-up) Remove unnecessary code and atomic update Signed-off-by: Emmi Takkinen <emmi.takkinen@koha-suomi.fi>
Tested this again and everything seems to work as described. However circulation rule with "Holds pickup period (day)" column filled cannot be deleted. There is a similar problem with column "Decreased loan period for high holds" (bug 30694) where rule isn't included to deletion in smart-rules.pl. Not sure what is a proper status for this, is it "Patch doesn't apply" or "Failed QA"?
(In reply to Emmi Takkinen from comment #207) > Tested this again and everything seems to work as described. However > circulation rule with "Holds pickup period (day)" column filled cannot be > deleted. There is a similar problem with column "Decreased loan period for > high holds" (bug 30694) where rule isn't included to deletion in > smart-rules.pl. Not sure what is a proper status for this, is it "Patch > doesn't apply" or "Failed QA"? Hi Emmi, probably only needs a small follow-up, but Failed QA would be the correct one for now. Thx for linking the other bug already, that's a good clue.
Settig this as Failed QA for reason mentioned in comment 207.
Created attachment 145644 [details] [review] Bug 8367: (follow-up) possible to delete circ rule
Created attachment 145657 [details] [review] Bug 8367: (follow-up) possible to delete circ rule Signed-off-by: Emmi Takkinen <emmi.takkinen@koha-suomi.fi>
Created attachment 147798 [details] [review] Bug 8367: Add holds_pickup_period circulation rule So that pickup delay can have a different value per patron category, item type or branch. To test: 1) Update database, restart services 2) Set ReservesMaxPickUpDelay syspref (if not already set) 3) Edit your circulation rules and set a value under 'Holds pickup period (day) that is DIFFERENT from ReservesMaxPickUpDelay. Set a few different numbers for different branches as well. 4) Place a hold on a biblio from the staff client. 5) Check in an item from that biblio and confirm the hold as waiting 6) Confirm the expiration date is calculated using the 'Holds pickup period' value instead of the ReservesMaxPickUpDelay syspref 7) Revert the waiting status and delete the hold 8) Re-place the hold on the biblio on the OPAC. Notice that when you change the pick up location, the number of days in the pickup message below the dropdown changes based on the circ rules. 9) Create a holiday with a date that will overlap with the 'Holds pickup period' 10) Check in an item from that biblio and confirm the hold as waiting 11) Confirm the expiration date is calculated using the 'Holds pickup period' value AND considers the special holiday 12) Confirm tests pass t/db_dependent/Holds/WaitingReserves.t 13) Test Talking Tech: 13a) Enable TalkingTechItivaPhoneNotification 13b) Go to Tools -> Notices & slips. Add content to the HOLD phone (itiva) notice. 13c) In your terminal, run perl /path/to/koha/misc/cronjobs/thirdparty/TalkingTech_itiva_outbound.pl -o ~/itiva.tmp -w 0 --type=RESERVE Sponsored-by: Catalyst IT Signed-off-by: Emmi Takkinen <emmi.takkinen@koha-suomi.fi> Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Created attachment 147799 [details] [review] Bug 8367: Make database update idempotent Signed-off-by: Emmi Takkinen <emmi.takkinen@koha-suomi.fi> Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Created attachment 147800 [details] [review] Bug 8367: Make database update idempotent (across different MariaDB versions) The previous query seemed to work on MariaDB 10.1.48 (used by koha-testing-docker), but it didn't work on MariaDB 10.3.29 (default in Debian 10 as of this writing), but this patched query works on both indempotently. Signed-off-by: Emmi Takkinen <emmi.takkinen@koha-suomi.fi> Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Created attachment 147801 [details] [review] Bug 8367: (follow-up) Fix pickup delay text on OPAC and other bits Signed-off-by: Emmi Takkinen <emmi.takkinen@koha-suomi.fi> Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Created attachment 147802 [details] [review] Bug 8367: (follow-up) Remove pickup delay message from OPAC It adds unnecessary complexity and information. Signed-off-by: Emmi Takkinen <emmi.takkinen@koha-suomi.fi> Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Created attachment 147803 [details] [review] Bug 8367: (follow-up) Fix tests Signed-off-by: Emmi Takkinen <emmi.takkinen@koha-suomi.fi> Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Created attachment 147804 [details] [review] Bug 8367: (follow-up) Remove unnecessary code and atomic update Signed-off-by: Emmi Takkinen <emmi.takkinen@koha-suomi.fi> Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Created attachment 147805 [details] [review] Bug 8367: (follow-up) possible to delete circ rule Signed-off-by: Emmi Takkinen <emmi.takkinen@koha-suomi.fi> Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Created attachment 147806 [details] [review] Bug 8376: (QA follow-up) Update message on Hold waiting pickup report Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Created attachment 147807 [details] [review] Bug 8367: (QA follow-up) Improve display when value not set for rule When a rule is not set for a speficic line, the value in the syspref will be used. This should be made clear in the interface. To test: 1 - Add a new rule for a specific item type 2 - Do not fill the 'Hold pickup period' 3 - Confirm that the rule defaults to the system preference 4 - Hover on 'Default' in the circ rules under 'Hold pickup period' 5 - Confirm the text is readable Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Moving back to Signed off, need another pair of eyes here After talking to educators here, they believe if a line is left blank, we should default back to the system preference, and I added a line to make that clear. If this is the case, however, we do need to insert the blank value into the DB for all existing rules - otherwise rules will fall back to the most specific rule when undefined, and will use the system preference when blank - so we could end up having two behaviors The patches should be squashed once a decision is made
I'm interested in this one, so planning to take a look. Have a few other things I need to do first, but just a FYI
Created attachment 148536 [details] [review] Bug 8367: Add holds_pickup_period circulation rule So that pickup delay can have a different value per patron category, item type or branch. To test: 1) Update database, restart services 2) Set ReservesMaxPickUpDelay syspref (if not already set) 3) Edit your circulation rules and set a value under 'Holds pickup period (day) that is DIFFERENT from ReservesMaxPickUpDelay. Set a few different numbers for different branches as well. 4) Place a hold on a biblio from the staff client. 5) Check in an item from that biblio and confirm the hold as waiting 6) Confirm the expiration date is calculated using the 'Holds pickup period' value instead of the ReservesMaxPickUpDelay syspref 7) Revert the waiting status and delete the hold 8) Re-place the hold on the biblio on the OPAC. Notice that when you change the pick up location, the number of days in the pickup message below the dropdown changes based on the circ rules. 9) Create a holiday with a date that will overlap with the 'Holds pickup period' 10) Check in an item from that biblio and confirm the hold as waiting 11) Confirm the expiration date is calculated using the 'Holds pickup period' value AND considers the special holiday 12) Confirm tests pass t/db_dependent/Holds/WaitingReserves.t 13) Test Talking Tech: 13a) Enable TalkingTechItivaPhoneNotification 13b) Go to Tools -> Notices & slips. Add content to the HOLD phone (itiva) notice. 13c) In your terminal, run perl /path/to/koha/misc/cronjobs/thirdparty/TalkingTech_itiva_outbound.pl -o ~/itiva.tmp -w 0 --type=RESERVE Sponsored-by: Catalyst IT Signed-off-by: Emmi Takkinen <emmi.takkinen@koha-suomi.fi> Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Signed-off-by: Kelly <kelly@bywatersolutions.com>
Created attachment 148537 [details] [review] Bug 8367: Make database update idempotent Signed-off-by: Emmi Takkinen <emmi.takkinen@koha-suomi.fi> Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Signed-off-by: Kelly <kelly@bywatersolutions.com>
Created attachment 148538 [details] [review] Bug 8367: Make database update idempotent (across different MariaDB versions) The previous query seemed to work on MariaDB 10.1.48 (used by koha-testing-docker), but it didn't work on MariaDB 10.3.29 (default in Debian 10 as of this writing), but this patched query works on both indempotently. Signed-off-by: Emmi Takkinen <emmi.takkinen@koha-suomi.fi> Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Signed-off-by: Kelly <kelly@bywatersolutions.com>
Created attachment 148539 [details] [review] Bug 8367: (follow-up) Fix pickup delay text on OPAC and other bits Signed-off-by: Emmi Takkinen <emmi.takkinen@koha-suomi.fi> Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Signed-off-by: Kelly <kelly@bywatersolutions.com>
Created attachment 148540 [details] [review] Bug 8367: (follow-up) Remove pickup delay message from OPAC It adds unnecessary complexity and information. Signed-off-by: Emmi Takkinen <emmi.takkinen@koha-suomi.fi> Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Signed-off-by: Kelly <kelly@bywatersolutions.com>
Created attachment 148541 [details] [review] Bug 8367: (follow-up) Fix tests Signed-off-by: Emmi Takkinen <emmi.takkinen@koha-suomi.fi> Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Signed-off-by: Kelly <kelly@bywatersolutions.com>
Created attachment 148542 [details] [review] Bug 8367: (follow-up) Remove unnecessary code and atomic update Signed-off-by: Emmi Takkinen <emmi.takkinen@koha-suomi.fi> Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Signed-off-by: Kelly <kelly@bywatersolutions.com>
Created attachment 148543 [details] [review] Bug 8367: (follow-up) possible to delete circ rule Signed-off-by: Emmi Takkinen <emmi.takkinen@koha-suomi.fi> Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Signed-off-by: Kelly <kelly@bywatersolutions.com>
Created attachment 148544 [details] [review] Bug 8376: (QA follow-up) Update message on Hold waiting pickup report Signed-off-by: Nick Clemens <nick@bywatersolutions.com> https://bugs.koha-community.org/show_bug.cgi?id=8367 Signed-off-by: Kelly <kelly@bywatersolutions.com>
Created attachment 148545 [details] [review] Bug 8367: (QA follow-up) Improve display when value not set for rule When a rule is not set for a speficic line, the value in the syspref will be used. This should be made clear in the interface. To test: 1 - Add a new rule for a specific item type 2 - Do not fill the 'Hold pickup period' 3 - Confirm that the rule defaults to the system preference 4 - Hover on 'Default' in the circ rules under 'Hold pickup period' 5 - Confirm the text is readable Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Signed-off-by: Kelly <kelly@bywatersolutions.com>
There are lots of signoffs on these patches, can this be marked Passed QA?
(In reply to Aleisha Amohia from comment #234) > There are lots of signoffs on these patches, can this be marked Passed QA? Hi Aleisha, It appears I was wrong in comment 187, see comment 222 It is a question of how the fallback should work, so we either need a DB update, or to change the expectation for when a rule is not set
(In reply to Nick Clemens from comment #235) > (In reply to Aleisha Amohia from comment #234) > > There are lots of signoffs on these patches, can this be marked Passed QA? > > Hi Aleisha, > > It appears I was wrong in comment 187, see comment 222 > > It is a question of how the fallback should work, so we either need a DB > update, or to change the expectation for when a rule is not set It sounds like this needs a little bit more work?
Created attachment 150437 [details] [review] Bug 8376: (QA follow-up) Update message on Hold waiting pickup report Signed-off-by: Nick Clemens <nick@bywatersolutions.com> https://bugs.koha-community.org/show_bug.cgi?id=8367 Signed-off-by: Kelly <kelly@bywatersolutions.com>
Created attachment 150438 [details] [review] Bug 8367: (QA follow-up) Improve display when value not set for rule When a rule is not set for a speficic line, the value in the syspref will be used. This should be made clear in the interface. To test: 1 - Add a new rule for a specific item type 2 - Do not fill the 'Hold pickup period' 3 - Confirm that the rule defaults to the system preference 4 - Hover on 'Default' in the circ rules under 'Hold pickup period' 5 - Confirm the text is readable Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Signed-off-by: Kelly <kelly@bywatersolutions.com>
Created attachment 150439 [details] [review] Bug 8367: Add holds_pickup_period fallback circulation rule
(In reply to Katrin Fischer from comment #236) > (In reply to Nick Clemens from comment #235) > > (In reply to Aleisha Amohia from comment #234) > > > There are lots of signoffs on these patches, can this be marked Passed QA? > > > > Hi Aleisha, > > > > It appears I was wrong in comment 187, see comment 222 > > > > It is a question of how the fallback should work, so we either need a DB > > update, or to change the expectation for when a rule is not set > > It sounds like this needs a little bit more work? Attached an atomic update patch, not sure how it affects the bug status.
Created attachment 153235 [details] [review] Bug 8367: Add holds_pickup_period circulation rule So that pickup delay can have a different value per patron category, item type or branch. To test: 1) Update database, restart services 2) Set ReservesMaxPickUpDelay syspref (if not already set) 3) Edit your circulation rules and set a value under 'Holds pickup period (day) that is DIFFERENT from ReservesMaxPickUpDelay. Set a few different numbers for different branches as well. 4) Place a hold on a biblio from the staff client. 5) Check in an item from that biblio and confirm the hold as waiting 6) Confirm the expiration date is calculated using the 'Holds pickup period' value instead of the ReservesMaxPickUpDelay syspref 7) Revert the waiting status and delete the hold 8) Re-place the hold on the biblio on the OPAC. Notice that when you change the pick up location, the number of days in the pickup message below the dropdown changes based on the circ rules. 9) Create a holiday with a date that will overlap with the 'Holds pickup period' 10) Check in an item from that biblio and confirm the hold as waiting 11) Confirm the expiration date is calculated using the 'Holds pickup period' value AND considers the special holiday 12) Confirm tests pass t/db_dependent/Holds/WaitingReserves.t 13) Test Talking Tech: 13a) Enable TalkingTechItivaPhoneNotification 13b) Go to Tools -> Notices & slips. Add content to the HOLD phone (itiva) notice. 13c) In your terminal, run perl /path/to/koha/misc/cronjobs/thirdparty/TalkingTech_itiva_outbound.pl -o ~/itiva.tmp -w 0 --type=RESERVE Sponsored-by: Catalyst IT Signed-off-by: Emmi Takkinen <emmi.takkinen@koha-suomi.fi> Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Signed-off-by: Kelly <kelly@bywatersolutions.com>
Created attachment 153237 [details] [review] Bug 8367: Make database update idempotent Signed-off-by: Emmi Takkinen <emmi.takkinen@koha-suomi.fi> Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Signed-off-by: Kelly <kelly@bywatersolutions.com>
Created attachment 153238 [details] [review] Bug 8367: Make database update idempotent (across different MariaDB versions) The previous query seemed to work on MariaDB 10.1.48 (used by koha-testing-docker), but it didn't work on MariaDB 10.3.29 (default in Debian 10 as of this writing), but this patched query works on both indempotently. Signed-off-by: Emmi Takkinen <emmi.takkinen@koha-suomi.fi> Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Signed-off-by: Kelly <kelly@bywatersolutions.com>
Created attachment 153239 [details] [review] Bug 8367: (follow-up) Fix pickup delay text on OPAC and other bits Signed-off-by: Emmi Takkinen <emmi.takkinen@koha-suomi.fi> Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Signed-off-by: Kelly <kelly@bywatersolutions.com>
Created attachment 153240 [details] [review] Bug 8367: (follow-up) Remove pickup delay message from OPAC It adds unnecessary complexity and information. Signed-off-by: Emmi Takkinen <emmi.takkinen@koha-suomi.fi> Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Signed-off-by: Kelly <kelly@bywatersolutions.com>
Created attachment 153241 [details] [review] Bug 8367: (follow-up) Fix tests Signed-off-by: Emmi Takkinen <emmi.takkinen@koha-suomi.fi> Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Signed-off-by: Kelly <kelly@bywatersolutions.com>
Created attachment 153242 [details] [review] Bug 8367: (follow-up) Remove unnecessary code and atomic update Signed-off-by: Emmi Takkinen <emmi.takkinen@koha-suomi.fi> Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Signed-off-by: Kelly <kelly@bywatersolutions.com>
Created attachment 153243 [details] [review] Bug 8367: (follow-up) possible to delete circ rule Signed-off-by: Emmi Takkinen <emmi.takkinen@koha-suomi.fi> Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Signed-off-by: Kelly <kelly@bywatersolutions.com>
Created attachment 153244 [details] [review] Bug 8367: (QA follow-up) Update message on Hold waiting pickup report Signed-off-by: Nick Clemens <nick@bywatersolutions.com> https://bugs.koha-community.org/show_bug.cgi?id=8367 Signed-off-by: Kelly <kelly@bywatersolutions.com>
Created attachment 153245 [details] [review] Bug 8367: (QA follow-up) Improve display when value not set for rule When a rule is not set for a speficic line, the value in the syspref will be used. This should be made clear in the interface. To test: 1 - Add a new rule for a specific item type 2 - Do not fill the 'Hold pickup period' 3 - Confirm that the rule defaults to the system preference 4 - Hover on 'Default' in the circ rules under 'Hold pickup period' 5 - Confirm the text is readable Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Signed-off-by: Kelly <kelly@bywatersolutions.com>
Created attachment 153246 [details] [review] Bug 8367: Add holds_pickup_period fallback circulation rule
Working on this one, but will not finish today.
Created attachment 157138 [details] [review] Bug 8367: Add holds_pickup_period circulation rule So that pickup delay can have a different value per patron category, item type or branch. To test: 1) Update database, restart services 2) Set ReservesMaxPickUpDelay syspref (if not already set) 3) Edit your circulation rules and set a value under 'Holds pickup period (day) that is DIFFERENT from ReservesMaxPickUpDelay. Set a few different numbers for different branches as well. 4) Place a hold on a biblio from the staff client. 5) Check in an item from that biblio and confirm the hold as waiting 6) Confirm the expiration date is calculated using the 'Holds pickup period' value instead of the ReservesMaxPickUpDelay syspref 7) Revert the waiting status and delete the hold 8) Re-place the hold on the biblio on the OPAC. Notice that when you change the pick up location, the number of days in the pickup message below the dropdown changes based on the circ rules. 9) Create a holiday with a date that will overlap with the 'Holds pickup period' 10) Check in an item from that biblio and confirm the hold as waiting 11) Confirm the expiration date is calculated using the 'Holds pickup period' value AND considers the special holiday 12) Confirm tests pass t/db_dependent/Holds/WaitingReserves.t 13) Test Talking Tech: 13a) Enable TalkingTechItivaPhoneNotification 13b) Go to Tools -> Notices & slips. Add content to the HOLD phone (itiva) notice. 13c) In your terminal, run perl /path/to/koha/misc/cronjobs/thirdparty/TalkingTech_itiva_outbound.pl -o ~/itiva.tmp -w 0 --type=RESERVE Sponsored-by: Catalyst IT Signed-off-by: Emmi Takkinen <emmi.takkinen@koha-suomi.fi> Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Signed-off-by: Kelly <kelly@bywatersolutions.com> Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Created attachment 157139 [details] [review] Bug 8367: Make database update idempotent Signed-off-by: Emmi Takkinen <emmi.takkinen@koha-suomi.fi> Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Signed-off-by: Kelly <kelly@bywatersolutions.com> Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Created attachment 157140 [details] [review] Bug 8367: Make database update idempotent (across different MariaDB versions) The previous query seemed to work on MariaDB 10.1.48 (used by koha-testing-docker), but it didn't work on MariaDB 10.3.29 (default in Debian 10 as of this writing), but this patched query works on both indempotently. Signed-off-by: Emmi Takkinen <emmi.takkinen@koha-suomi.fi> Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Signed-off-by: Kelly <kelly@bywatersolutions.com> Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Created attachment 157141 [details] [review] Bug 8367: (follow-up) Fix pickup delay text on OPAC and other bits Signed-off-by: Emmi Takkinen <emmi.takkinen@koha-suomi.fi> Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Signed-off-by: Kelly <kelly@bywatersolutions.com> Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Created attachment 157142 [details] [review] Bug 8367: (follow-up) Remove pickup delay message from OPAC It adds unnecessary complexity and information. Signed-off-by: Emmi Takkinen <emmi.takkinen@koha-suomi.fi> Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Signed-off-by: Kelly <kelly@bywatersolutions.com> Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Created attachment 157143 [details] [review] Bug 8367: (follow-up) Fix tests Signed-off-by: Emmi Takkinen <emmi.takkinen@koha-suomi.fi> Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Signed-off-by: Kelly <kelly@bywatersolutions.com> Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Created attachment 157144 [details] [review] Bug 8367: (follow-up) Remove unnecessary code and atomic update Signed-off-by: Emmi Takkinen <emmi.takkinen@koha-suomi.fi> Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Signed-off-by: Kelly <kelly@bywatersolutions.com> Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Created attachment 157145 [details] [review] Bug 8367: (follow-up) possible to delete circ rule Signed-off-by: Emmi Takkinen <emmi.takkinen@koha-suomi.fi> Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Signed-off-by: Kelly <kelly@bywatersolutions.com> Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Created attachment 157146 [details] [review] Bug 8367: (QA follow-up) Update message on Hold waiting pickup report Signed-off-by: Nick Clemens <nick@bywatersolutions.com> https://bugs.koha-community.org/show_bug.cgi?id=8367 Signed-off-by: Kelly <kelly@bywatersolutions.com> Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Created attachment 157147 [details] [review] Bug 8367: (QA follow-up) Improve display when value not set for rule When a rule is not set for a speficic line, the value in the syspref will be used. This should be made clear in the interface. To test: 1 - Add a new rule for a specific item type 2 - Do not fill the 'Hold pickup period' 3 - Confirm that the rule defaults to the system preference 4 - Hover on 'Default' in the circ rules under 'Hold pickup period' 5 - Confirm the text is readable Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Signed-off-by: Kelly <kelly@bywatersolutions.com> Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Created attachment 157148 [details] [review] Bug 8367: Add holds_pickup_period fallback circulation rule Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Created attachment 157149 [details] [review] Bug 8367: (QA follow-up) Fix QA script * Add +x to atomic database update file * Perltidy * Add spans to rewritten tab label for translatability Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Created attachment 157150 [details] [review] Bug 8367: Make sure "Default" is not copied when editing circulation rules When editing a circulation rules the "Default" was copied into the input field. This would cause problems on saving and especially with translations. So now the field will be empty and appear as "Default" after saving as we already do it for other values with "unlimited". Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
This is almost there, but we need a small fix for the circulation rules editor. When a rule is edited and the Pickup period is "Default" (was saved as empty before), the "Default" shows up in the input field. This is bad as it will break on save and also when translated. We need to make it show up as empty when the rule is edited. And as 0 when 0 was entered. My patch didn't work and created a display issue at the beginning of the table (second column). Aleisha, could you have a go please?
Created attachment 157298 [details] [review] Bug 8367: (follow-up) Fix for circ rules editor Make sure holds pickup period field is empty when editing a 'default' rule.
(In reply to Katrin Fischer from comment #266) > This is almost there, but we need a small fix for the circulation rules > editor. > > When a rule is edited and the Pickup period is "Default" (was saved as empty > before), the "Default" shows up in the input field. This is bad as it will > break on save and also when translated. > > We need to make it show up as empty when the rule is edited. And as 0 when 0 > was entered. > > My patch didn't work and created a display issue at the beginning of the > table (second column). > > Aleisha, could you have a go please? I don't know what the difference is between your patch that didn't work and the one I just attached, but it looks good to me!
Created attachment 157343 [details] [review] Bug 8367: Add holds_pickup_period circulation rule So that pickup delay can have a different value per patron category, item type or branch. To test: 1) Update database, restart services 2) Set ReservesMaxPickUpDelay syspref (if not already set) 3) Edit your circulation rules and set a value under 'Holds pickup period (day) that is DIFFERENT from ReservesMaxPickUpDelay. Set a few different numbers for different branches as well. 4) Place a hold on a biblio from the staff client. 5) Check in an item from that biblio and confirm the hold as waiting 6) Confirm the expiration date is calculated using the 'Holds pickup period' value instead of the ReservesMaxPickUpDelay syspref 7) Revert the waiting status and delete the hold 8) Re-place the hold on the biblio on the OPAC. Notice that when you change the pick up location, the number of days in the pickup message below the dropdown changes based on the circ rules. 9) Create a holiday with a date that will overlap with the 'Holds pickup period' 10) Check in an item from that biblio and confirm the hold as waiting 11) Confirm the expiration date is calculated using the 'Holds pickup period' value AND considers the special holiday 12) Confirm tests pass t/db_dependent/Holds/WaitingReserves.t 13) Test Talking Tech: 13a) Enable TalkingTechItivaPhoneNotification 13b) Go to Tools -> Notices & slips. Add content to the HOLD phone (itiva) notice. 13c) In your terminal, run perl /path/to/koha/misc/cronjobs/thirdparty/TalkingTech_itiva_outbound.pl -o ~/itiva.tmp -w 0 --type=RESERVE Sponsored-by: Catalyst IT Signed-off-by: Emmi Takkinen <emmi.takkinen@koha-suomi.fi> Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Signed-off-by: Kelly <kelly@bywatersolutions.com> Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de> Signed-off-by: Kelly <kelly@bywatersolutions.com>
Created attachment 157344 [details] [review] Bug 8367: Make database update idempotent Signed-off-by: Emmi Takkinen <emmi.takkinen@koha-suomi.fi> Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Signed-off-by: Kelly <kelly@bywatersolutions.com> Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de> Signed-off-by: Kelly <kelly@bywatersolutions.com>
Created attachment 157345 [details] [review] Bug 8367: Make database update idempotent (across different MariaDB versions) The previous query seemed to work on MariaDB 10.1.48 (used by koha-testing-docker), but it didn't work on MariaDB 10.3.29 (default in Debian 10 as of this writing), but this patched query works on both indempotently. Signed-off-by: Emmi Takkinen <emmi.takkinen@koha-suomi.fi> Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Signed-off-by: Kelly <kelly@bywatersolutions.com> Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de> Signed-off-by: Kelly <kelly@bywatersolutions.com>
Created attachment 157346 [details] [review] Bug 8367: (follow-up) Fix pickup delay text on OPAC and other bits Signed-off-by: Emmi Takkinen <emmi.takkinen@koha-suomi.fi> Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Signed-off-by: Kelly <kelly@bywatersolutions.com> Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de> Signed-off-by: Kelly <kelly@bywatersolutions.com>
Created attachment 157347 [details] [review] Bug 8367: (follow-up) Remove pickup delay message from OPAC It adds unnecessary complexity and information. Signed-off-by: Emmi Takkinen <emmi.takkinen@koha-suomi.fi> Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Signed-off-by: Kelly <kelly@bywatersolutions.com> Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de> Signed-off-by: Kelly <kelly@bywatersolutions.com>
Created attachment 157348 [details] [review] Bug 8367: (follow-up) Fix tests Signed-off-by: Emmi Takkinen <emmi.takkinen@koha-suomi.fi> Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Signed-off-by: Kelly <kelly@bywatersolutions.com> Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de> Signed-off-by: Kelly <kelly@bywatersolutions.com>
Created attachment 157349 [details] [review] Bug 8367: (follow-up) Remove unnecessary code and atomic update Signed-off-by: Emmi Takkinen <emmi.takkinen@koha-suomi.fi> Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Signed-off-by: Kelly <kelly@bywatersolutions.com> Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de> Signed-off-by: Kelly <kelly@bywatersolutions.com>
Created attachment 157350 [details] [review] Bug 8367: (follow-up) possible to delete circ rule Signed-off-by: Emmi Takkinen <emmi.takkinen@koha-suomi.fi> Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Signed-off-by: Kelly <kelly@bywatersolutions.com> Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de> Signed-off-by: Kelly <kelly@bywatersolutions.com>
Created attachment 157351 [details] [review] Bug 8367: (QA follow-up) Update message on Hold waiting pickup report Signed-off-by: Nick Clemens <nick@bywatersolutions.com> https://bugs.koha-community.org/show_bug.cgi?id=8367 Signed-off-by: Kelly <kelly@bywatersolutions.com> Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de> Signed-off-by: Kelly <kelly@bywatersolutions.com>
Created attachment 157352 [details] [review] Bug 8367: (QA follow-up) Improve display when value not set for rule When a rule is not set for a speficic line, the value in the syspref will be used. This should be made clear in the interface. To test: 1 - Add a new rule for a specific item type 2 - Do not fill the 'Hold pickup period' 3 - Confirm that the rule defaults to the system preference 4 - Hover on 'Default' in the circ rules under 'Hold pickup period' 5 - Confirm the text is readable Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Signed-off-by: Kelly <kelly@bywatersolutions.com> Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de> Signed-off-by: Kelly <kelly@bywatersolutions.com>
Created attachment 157354 [details] [review] Bug 8367: Add holds_pickup_period fallback circulation rule Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de> Signed-off-by: Kelly <kelly@bywatersolutions.com>
Created attachment 157355 [details] [review] Bug 8367: (QA follow-up) Fix QA script * Add +x to atomic database update file * Perltidy * Add spans to rewritten tab label for translatability Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de> Signed-off-by: Kelly <kelly@bywatersolutions.com>
Created attachment 157356 [details] [review] Bug 8367: (follow-up) Fix for circ rules editor Make sure holds pickup period field is empty when editing a 'default' rule. Signed-off-by: Kelly <kelly@bywatersolutions.com>
Kelly, it looks like you forgot to switch the status here and on some other bugs, please double check when signing off!
Hi Aleisha, I rebased the patch set (will re-apply my patches here), because bug 25393 was pushed before yours, also adding a new circulation condition to the table. This means we need to adjust the column from 31 to 32, but I didn't write the follow-up for that, because I found what broke the behavior: * Turn on UnseenRenewals system preference It throws the column count...and then we are back at the default copying into the input field. Could you try to fix that?
Created attachment 157697 [details] [review] Bug 8367: Add holds_pickup_period circulation rule So that pickup delay can have a different value per patron category, item type or branch. To test: 1) Update database, restart services 2) Set ReservesMaxPickUpDelay syspref (if not already set) 3) Edit your circulation rules and set a value under 'Holds pickup period (day) that is DIFFERENT from ReservesMaxPickUpDelay. Set a few different numbers for different branches as well. 4) Place a hold on a biblio from the staff client. 5) Check in an item from that biblio and confirm the hold as waiting 6) Confirm the expiration date is calculated using the 'Holds pickup period' value instead of the ReservesMaxPickUpDelay syspref 7) Revert the waiting status and delete the hold 8) Re-place the hold on the biblio on the OPAC. Notice that when you change the pick up location, the number of days in the pickup message below the dropdown changes based on the circ rules. 9) Create a holiday with a date that will overlap with the 'Holds pickup period' 10) Check in an item from that biblio and confirm the hold as waiting 11) Confirm the expiration date is calculated using the 'Holds pickup period' value AND considers the special holiday 12) Confirm tests pass t/db_dependent/Holds/WaitingReserves.t 13) Test Talking Tech: 13a) Enable TalkingTechItivaPhoneNotification 13b) Go to Tools -> Notices & slips. Add content to the HOLD phone (itiva) notice. 13c) In your terminal, run perl /path/to/koha/misc/cronjobs/thirdparty/TalkingTech_itiva_outbound.pl -o ~/itiva.tmp -w 0 --type=RESERVE Sponsored-by: Catalyst IT Signed-off-by: Emmi Takkinen <emmi.takkinen@koha-suomi.fi> Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Signed-off-by: Kelly <kelly@bywatersolutions.com> Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de> Signed-off-by: Kelly <kelly@bywatersolutions.com>
Created attachment 157698 [details] [review] Bug 8367: Make database update idempotent Signed-off-by: Emmi Takkinen <emmi.takkinen@koha-suomi.fi> Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Signed-off-by: Kelly <kelly@bywatersolutions.com> Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de> Signed-off-by: Kelly <kelly@bywatersolutions.com>
Created attachment 157699 [details] [review] Bug 8367: Make database update idempotent (across different MariaDB versions) The previous query seemed to work on MariaDB 10.1.48 (used by koha-testing-docker), but it didn't work on MariaDB 10.3.29 (default in Debian 10 as of this writing), but this patched query works on both indempotently. Signed-off-by: Emmi Takkinen <emmi.takkinen@koha-suomi.fi> Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Signed-off-by: Kelly <kelly@bywatersolutions.com> Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de> Signed-off-by: Kelly <kelly@bywatersolutions.com>
Created attachment 157700 [details] [review] Bug 8367: (follow-up) Fix pickup delay text on OPAC and other bits Signed-off-by: Emmi Takkinen <emmi.takkinen@koha-suomi.fi> Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Signed-off-by: Kelly <kelly@bywatersolutions.com> Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de> Signed-off-by: Kelly <kelly@bywatersolutions.com>
Created attachment 157701 [details] [review] Bug 8367: (follow-up) Remove pickup delay message from OPAC It adds unnecessary complexity and information. Signed-off-by: Emmi Takkinen <emmi.takkinen@koha-suomi.fi> Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Signed-off-by: Kelly <kelly@bywatersolutions.com> Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de> Signed-off-by: Kelly <kelly@bywatersolutions.com>
Created attachment 157702 [details] [review] Bug 8367: (follow-up) Fix tests Signed-off-by: Emmi Takkinen <emmi.takkinen@koha-suomi.fi> Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Signed-off-by: Kelly <kelly@bywatersolutions.com> Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de> Signed-off-by: Kelly <kelly@bywatersolutions.com>
Created attachment 157703 [details] [review] Bug 8367: (follow-up) Remove unnecessary code and atomic update Signed-off-by: Emmi Takkinen <emmi.takkinen@koha-suomi.fi> Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Signed-off-by: Kelly <kelly@bywatersolutions.com> Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de> Signed-off-by: Kelly <kelly@bywatersolutions.com>
Created attachment 157704 [details] [review] Bug 8367: (follow-up) possible to delete circ rule Signed-off-by: Emmi Takkinen <emmi.takkinen@koha-suomi.fi> Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Signed-off-by: Kelly <kelly@bywatersolutions.com> Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de> Signed-off-by: Kelly <kelly@bywatersolutions.com>
Created attachment 157705 [details] [review] Bug 8367: (QA follow-up) Update message on Hold waiting pickup report Signed-off-by: Nick Clemens <nick@bywatersolutions.com> https://bugs.koha-community.org/show_bug.cgi?id=8367 Signed-off-by: Kelly <kelly@bywatersolutions.com> Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de> Signed-off-by: Kelly <kelly@bywatersolutions.com>
Created attachment 157706 [details] [review] Bug 8367: (QA follow-up) Improve display when value not set for rule When a rule is not set for a speficic line, the value in the syspref will be used. This should be made clear in the interface. To test: 1 - Add a new rule for a specific item type 2 - Do not fill the 'Hold pickup period' 3 - Confirm that the rule defaults to the system preference 4 - Hover on 'Default' in the circ rules under 'Hold pickup period' 5 - Confirm the text is readable Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Signed-off-by: Kelly <kelly@bywatersolutions.com> Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de> Signed-off-by: Kelly <kelly@bywatersolutions.com>
Created attachment 157707 [details] [review] Bug 8367: Add holds_pickup_period fallback circulation rule Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de> Signed-off-by: Kelly <kelly@bywatersolutions.com>
Created attachment 157708 [details] [review] Bug 8367: (QA follow-up) Fix QA script * Add +x to atomic database update file * Perltidy * Add spans to rewritten tab label for translatability Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de> Signed-off-by: Kelly <kelly@bywatersolutions.com>
Created attachment 157709 [details] [review] Bug 8367: (follow-up) Fix for circ rules editor Make sure holds pickup period field is empty when editing a 'default' rule. Signed-off-by: Kelly <kelly@bywatersolutions.com>
Created attachment 157715 [details] [review] Bug 8367: (follow-up) Fix for circ rules editor Make sure holds pickup period field is empty when editing a 'default' rule.
(In reply to Katrin Fischer from comment #283) > Hi Aleisha, > > I rebased the patch set (will re-apply my patches here), because bug 25393 > was pushed before yours, also adding a new circulation condition to the > table. > > This means we need to adjust the column from 31 to 32, but I didn't write > the follow-up for that, because I found what broke the behavior: > > * Turn on UnseenRenewals system preference > > It throws the column count...and then we are back at the default copying > into the input field. > > Could you try to fix that? Hi Katrin, I've done a new fix for this. I noticed the UnseenRenewals circ rule breaks a bunch of other rules in the same way, so maybe we can adopt this new way of identifying those columns in another bug report.
Created attachment 158023 [details] [review] Bug 8367: Add holds_pickup_period circulation rule So that pickup delay can have a different value per patron category, item type or branch. To test: 1) Update database, restart services 2) Set ReservesMaxPickUpDelay syspref (if not already set) 3) Edit your circulation rules and set a value under 'Holds pickup period (day) that is DIFFERENT from ReservesMaxPickUpDelay. Set a few different numbers for different branches as well. 4) Place a hold on a biblio from the staff client. 5) Check in an item from that biblio and confirm the hold as waiting 6) Confirm the expiration date is calculated using the 'Holds pickup period' value instead of the ReservesMaxPickUpDelay syspref 7) Revert the waiting status and delete the hold 8) Re-place the hold on the biblio on the OPAC. Notice that when you change the pick up location, the number of days in the pickup message below the dropdown changes based on the circ rules. 9) Create a holiday with a date that will overlap with the 'Holds pickup period' 10) Check in an item from that biblio and confirm the hold as waiting 11) Confirm the expiration date is calculated using the 'Holds pickup period' value AND considers the special holiday 12) Confirm tests pass t/db_dependent/Holds/WaitingReserves.t 13) Test Talking Tech: 13a) Enable TalkingTechItivaPhoneNotification 13b) Go to Tools -> Notices & slips. Add content to the HOLD phone (itiva) notice. 13c) In your terminal, run perl /path/to/koha/misc/cronjobs/thirdparty/TalkingTech_itiva_outbound.pl -o ~/itiva.tmp -w 0 --type=RESERVE Sponsored-by: Catalyst IT Signed-off-by: Emmi Takkinen <emmi.takkinen@koha-suomi.fi> Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Signed-off-by: Kelly <kelly@bywatersolutions.com> Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de> Signed-off-by: Kelly <kelly@bywatersolutions.com> Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Created attachment 158024 [details] [review] Bug 8367: Make database update idempotent Signed-off-by: Emmi Takkinen <emmi.takkinen@koha-suomi.fi> Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Signed-off-by: Kelly <kelly@bywatersolutions.com> Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de> Signed-off-by: Kelly <kelly@bywatersolutions.com> Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Created attachment 158025 [details] [review] Bug 8367: Make database update idempotent (across different MariaDB versions) The previous query seemed to work on MariaDB 10.1.48 (used by koha-testing-docker), but it didn't work on MariaDB 10.3.29 (default in Debian 10 as of this writing), but this patched query works on both indempotently. Signed-off-by: Emmi Takkinen <emmi.takkinen@koha-suomi.fi> Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Signed-off-by: Kelly <kelly@bywatersolutions.com> Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de> Signed-off-by: Kelly <kelly@bywatersolutions.com> Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Created attachment 158026 [details] [review] Bug 8367: (follow-up) Fix pickup delay text on OPAC and other bits Signed-off-by: Emmi Takkinen <emmi.takkinen@koha-suomi.fi> Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Signed-off-by: Kelly <kelly@bywatersolutions.com> Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de> Signed-off-by: Kelly <kelly@bywatersolutions.com> Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Created attachment 158027 [details] [review] Bug 8367: (follow-up) Remove pickup delay message from OPAC It adds unnecessary complexity and information. Signed-off-by: Emmi Takkinen <emmi.takkinen@koha-suomi.fi> Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Signed-off-by: Kelly <kelly@bywatersolutions.com> Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de> Signed-off-by: Kelly <kelly@bywatersolutions.com> Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Created attachment 158028 [details] [review] Bug 8367: (follow-up) Fix tests Signed-off-by: Emmi Takkinen <emmi.takkinen@koha-suomi.fi> Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Signed-off-by: Kelly <kelly@bywatersolutions.com> Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de> Signed-off-by: Kelly <kelly@bywatersolutions.com> Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Created attachment 158029 [details] [review] Bug 8367: (follow-up) Remove unnecessary code and atomic update Signed-off-by: Emmi Takkinen <emmi.takkinen@koha-suomi.fi> Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Signed-off-by: Kelly <kelly@bywatersolutions.com> Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de> Signed-off-by: Kelly <kelly@bywatersolutions.com> Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Created attachment 158030 [details] [review] Bug 8367: (follow-up) possible to delete circ rule Signed-off-by: Emmi Takkinen <emmi.takkinen@koha-suomi.fi> Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Signed-off-by: Kelly <kelly@bywatersolutions.com> Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de> Signed-off-by: Kelly <kelly@bywatersolutions.com> Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Created attachment 158031 [details] [review] Bug 8367: (QA follow-up) Update message on Hold waiting pickup report Signed-off-by: Nick Clemens <nick@bywatersolutions.com> https://bugs.koha-community.org/show_bug.cgi?id=8367 Signed-off-by: Kelly <kelly@bywatersolutions.com> Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de> Signed-off-by: Kelly <kelly@bywatersolutions.com> Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Created attachment 158032 [details] [review] Bug 8367: (QA follow-up) Improve display when value not set for rule When a rule is not set for a speficic line, the value in the syspref will be used. This should be made clear in the interface. To test: 1 - Add a new rule for a specific item type 2 - Do not fill the 'Hold pickup period' 3 - Confirm that the rule defaults to the system preference 4 - Hover on 'Default' in the circ rules under 'Hold pickup period' 5 - Confirm the text is readable Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Signed-off-by: Kelly <kelly@bywatersolutions.com> Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de> Signed-off-by: Kelly <kelly@bywatersolutions.com> Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Created attachment 158033 [details] [review] Bug 8367: Add holds_pickup_period fallback circulation rule Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de> Signed-off-by: Kelly <kelly@bywatersolutions.com> Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Created attachment 158034 [details] [review] Bug 8367: (QA follow-up) Fix QA script * Add +x to atomic database update file * Perltidy * Add spans to rewritten tab label for translatability Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de> Signed-off-by: Kelly <kelly@bywatersolutions.com> Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Created attachment 158035 [details] [review] Bug 8367: (follow-up) Fix for circ rules editor Make sure holds pickup period field is empty when editing a 'default' rule. Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
(In reply to Aleisha Amohia from comment #298) > > This means we need to adjust the column from 31 to 32, but I didn't write > > the follow-up for that, because I found what broke the behavior: > > > > * Turn on UnseenRenewals system preference > > > > It throws the column count...and then we are back at the default copying > > into the input field. > > > > Could you try to fix that? > > Hi Katrin, I've done a new fix for this. I noticed the UnseenRenewals circ > rule breaks a bunch of other rules in the same way, so maybe we can adopt > this new way of identifying those columns in another bug report. Hi Aleisha, thanks for that! I failed to come up with the right syntax :) I have filed a new bug to deal with the pre-existing problem for the other circulation rules: Bug 35182 - Activating "UnseenRenewals" causes problems in circulation rules editor Happy to pass QA now.
Pushed to master for 23.11. Nice work everyone, thanks!
Created attachment 158254 [details] [review] Bug 8367: (QA follow-up) Fix broken tests Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Created attachment 158255 [details] [review] Bug 8367: (QA follow-up) Fix broken tests in Reserves.t Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Enhancement not pushed to 23.05.x