When carrying out a renewal transaction Koha should check for both the maximum number of renewals allowed and also the number of unseen renewals. If it finds a value equal to or greater than the number of renewals already carried out against that loan, it prevents the renewal. The purpose of this is to prevent borrowers from continually renewing a book when they have, in fact, lost or misplaced it! When a renewal takes place via a method that doesn't require visible sight or possession of the item (e.g. OPAC, automatic renewal) then the number of unseen renewals for that item should be incremented. SIP2 renewals can be considered seen since they will require the physical item to be scanned. The number of allowed unseen renewals before a renewal is denied should be specifiable in the circulation rules. If a renewal is denied due to the unseen maximum having been reached, the flow should be the same as when a renewal limit has been reached, though with appropriate messages displayed to the user. It should be possible to override the denial in the same was as when a renewal limit is reached.
Created attachment 95861 [details] [review] Bug 24083: Required atomic updates & DB update This patch adds: - An "UnseenRenewals" circulation syspref that enables/disables the functionality added in this bug - A change to the issuingrules table schema and a corresponding database upgrade to add issuingrules.unseen_renewals_allowed - A change to the issues & old_issues table schemas and corresponding database upgrades to add issues.unseen_renewals & old_issues.unseen_renewals - An update for the renewals letter to include a message for unseen renewals
Created attachment 95862 [details] [review] Bug 24083: Add circ rules management This patch adds the ability to set the "Unseen renewals allowed" value in the circulation rules
Created attachment 95863 [details] [review] Bug 24083: Add support for unseen_renewals This patch adds support for unseen renewals. Here we retrofit knowledge of unseen renewals and add the display of unseen renewal counts and warnings, in addition to adding the ability to specify a renewal as being "unseen". The functionality added here is goverened by the UnseenRenewals syspref.
Created attachment 95864 [details] [review] Bug 24083: Add unit tests This patch adds unit tests for unseen renewals
Created attachment 95865 [details] [review] Bug 24083: Required atomic updates & DB update This patch adds: - An "UnseenRenewals" circulation syspref that enables/disables the functionality added in this bug - A change to the issuingrules table schema and a corresponding database upgrade to add issuingrules.unseen_renewals_allowed - A change to the issues & old_issues table schemas and corresponding database upgrades to add issues.unseen_renewals & old_issues.unseen_renewals - An update for the renewals letter to include a message for unseen renewals
Created attachment 95866 [details] [review] Bug 24083: Add circ rules management This patch adds the ability to set the "Unseen renewals allowed" value in the circulation rules
Created attachment 95867 [details] [review] Bug 24083: Add support for unseen_renewals This patch adds support for unseen renewals. Here we retrofit knowledge of unseen renewals and add the display of unseen renewal counts and warnings, in addition to adding the ability to specify a renewal as being "unseen". The functionality added here is goverened by the UnseenRenewals syspref.
Created attachment 95868 [details] [review] Bug 24083: Add unit tests This patch adds unit tests for unseen renewals
- Apply patches (if on a platform requiring this to be done manually) - Run DB updates and rebuild schema (if on a platform requiring this to be done manually) - Rebuild CSS (if on a platform requiring this to be done manually) - Issue an item to a patron - Navigate to Koha Administration > Circulation and fines rules => TEST: Observe that there *isn't* a column titled "Unseen renewals allowed (count)" - Navigate to a patron's checkouts list => TEST: Observe that in the "Renew" column, only details of their remaining renewals are shown, unseen renewals are not mentioned - Navigate to a patron's "Your account" page => TEST: Observe that in the "Renew" column, only details of their remaining renewals are shown, unseen renewals are not mentioned - Turn on the "UnseenRenewals" syspref - Turn on the "OpacRenewalAllowed" syspref - Navigate to Koha Administration > Circulation and fines rules - If you have no circulation rules defined, define one that will: - Apply to all patron categories - Apply to item types - Allow renewals => TEST: Observe that there *is* a column titled "Unseen renewals allowed (count)" and the value is empty - Add a value for "Unseen renewals allowed", make the value reasonably low so we can test what happens when that value is reached - Navigate to a patron's checkouts list in the staff client => TEST: Observe that in the "Renew" column, details of their remaining unseen renewals are now shown - Navigate to a patron's "Your account" page => TEST: Observe that in the "Renew" column, details of their remaining unseen renewals are now shown - In the OPAC, renew an item => TEST: Observe that, since the renewal took place in the OPAC, the number of unseen renewals left is decremented - In the OPAC, repeatedly renew the item until the unseen renewals limit is reached => TEST: Observe that the item can no longer be renewed and the borrower is advised to take the item to the library - Navigate to the patron's checkouts list in the staff client => TEST: Observe that the item is listed as "Must be renewed at the library" and is blocked from renewal - Select the "Override renewal restrictions" checkbox => TEST: Observe that the item can now be selected for renewal - Select the item and click "Renew or check in selected items" => TEST: Observe that now the item has been renewed by the library, the unseen renewals remaining count has been reset - Select the item for renewal again and check the "Renew as unseen if appropriate" checkbox, click "Renew or check in selected items" => TEST: Observe that, since the item was renewed as "unseen", the unseen renewals remaining has been decremented - Navigate to Circulation > Renew => TEST: Observe that a new "Record renewal as unseen if appropriate" checkbox is displayed - Renew the item, do not check the unseen checkbox => TEST: Observe in the patron's checkouts list that the unseen renewals count has not been decremented - Renew the item, check the unseen checkbox => TEST: Observe in the patron's checkouts list that the unseen renewals count has been decremented - Navigate to Tools > Notices & Slips, edit the "AUTO_RENEWALS" Email notice => TEST: Observe that there is now a test for the "too_unseen" error, with an appropriate message SIP RENEWALS: - Use sip_cli_emulator.pl to send a renew message for the item, for example: ./misc/sip_cli_emulator.pl -a 127.0.0.1 -p 6001 -su <kiosk_username> -sp <kiosk_password> --patron <patron_username> --password <patron_password> -l CPL -i <item_barcode> -m renew - TEST: Observe that, as the SIP renewal is considered to be "seen", the item's unseen renewals count has been reset
This needs to be rebuilt on top of bug 18936. And even bug 15522 I'd say.
Created attachment 98749 [details] [review] Bug 24083: Required atomic updates & DB update This patch adds: - An "UnseenRenewals" circulation syspref that enables/disables the functionality added in this bug - Add unseen_renewals_allowed to circulation_rules - A change to the issues & old_issues table schemas and corresponding database upgrades to add issues.unseen_renewals & old_issues.unseen_renewals - An update for the renewals letter to include a message for unseen renewals
Created attachment 98750 [details] [review] Bug 24083: Add circ rules management This patch adds the ability to set the "Unseen renewals allowed" value in the circulation rules
Created attachment 98751 [details] [review] Bug 24083: Add support for unseen_renewals This patch adds support for unseen renewals. Here we retrofit knowledge of unseen renewals and add the display of unseen renewal counts and warnings, in addition to adding the ability to specify a renewal as being "unseen". The functionality added here is goverened by the UnseenRenewals syspref.
Created attachment 98752 [details] [review] Bug 24083: Add unit tests This patch adds unit tests for unseen renewals
Created attachment 98765 [details] [review] Bug 24083: Required atomic updates & DB update This patch adds: - An "UnseenRenewals" circulation syspref that enables/disables the functionality added in this bug - Add unseen_renewals_allowed to circulation_rules - A change to the issues & old_issues table schemas and corresponding database upgrades to add issues.unseen_renewals & old_issues.unseen_renewals - An update for the renewals letter to include a message for unseen renewals
Created attachment 98766 [details] [review] Bug 24083: Add circ rules management This patch adds the ability to set the "Unseen renewals allowed" value in the circulation rules
Created attachment 98767 [details] [review] Bug 24083: Add support for unseen_renewals This patch adds support for unseen renewals. Here we retrofit knowledge of unseen renewals and add the display of unseen renewal counts and warnings, in addition to adding the ability to specify a renewal as being "unseen". The functionality added here is goverened by the UnseenRenewals syspref.
Created attachment 98768 [details] [review] Bug 24083: Add unit tests This patch adds unit tests for unseen renewals
- Apply patches (if on a platform requiring this to be done manually) - Run DB updates and rebuild schema (if on a platform requiring this to be done manually) - Rebuild CSS (if on a platform requiring this to be done manually) - Issue an item to a patron - Navigate to Koha Administration > Circulation and fines rules => TEST: Observe that there *isn't* a column titled "Unseen renewals allowed (count)" - Navigate to a patron's checkouts list => TEST: Observe that in the "Renew" column, only details of their remaining renewals are shown, unseen renewals are not mentioned - Turn on the "OpacRenewalAllowed" syspref - Navigate to a patron's "Your account" page => TEST: Observe that in the "Renew" column, only details of their remaining renewals are shown, unseen renewals are not mentioned - Turn on the "UnseenRenewals" syspref - Navigate to Koha Administration > Circulation and fines rules - If you have no circulation rules defined, define one that will: - Apply to all patron categories - Apply to item types - Allow renewals => TEST: Observe that there *is* a column titled "Unseen renewals allowed (count)" and the value is empty - Add a value for "Unseen renewals allowed", make the value reasonably low so we can test what happens when that value is reached - Navigate to a patron's checkouts list in the staff client => TEST: Observe that in the "Renew" column, details of their remaining unseen renewals are now shown - Navigate to a patron's "Your account" page => TEST: Observe that in the "Renew" column, details of their remaining unseen renewals are now shown - In the OPAC, renew an item => TEST: Observe that, since the renewal took place in the OPAC, the number of unseen renewals left is decremented - In the OPAC, repeatedly renew the item until the unseen renewals limit is reached => TEST: Observe that the item can no longer be renewed and the borrower is advised to take the item to the library - Navigate to the patron's checkouts list in the staff client => TEST: Observe that the item is listed as "Must be renewed at the library" and is blocked from renewal - Select the "Override renewal restrictions" checkbox => TEST: Observe that the item can now be selected for renewal - Select the item and click "Renew or check in selected items" => TEST: Observe that now the item has been renewed by the library, the unseen renewals remaining count has been reset - Select the item for renewal again and check the "Renew as unseen if appropriate" checkbox, click "Renew or check in selected items" => TEST: Observe that, since the item was renewed as "unseen", the unseen renewals remaining has been decremented - Navigate to Circulation > Renew => TEST: Observe that a new "Record renewal as unseen if appropriate" checkbox is displayed - Renew the item, do not check the unseen checkbox => TEST: Observe in the patron's checkouts list that the unseen renewals count has not been decremented - Renew the item, check the unseen checkbox => TEST: Observe in the patron's checkouts list that the unseen renewals count has been decremented - Navigate to Tools > Notices & Slips, edit the "AUTO_RENEWALS" Email notice => TEST: Observe that there is now a test for the "too_unseen" error, with an appropriate message SIP RENEWALS: - Use sip_cli_emulator.pl to send a renew message for the item, for example: ./misc/sip_cli_emulator.pl -a 127.0.0.1 -p 6001 -su <kiosk_username> -sp <kiosk_password> --patron <patron_username> --password <patron_password> -l CPL -i <item_barcode> -m renew - TEST: Observe that, as the SIP renewal is considered to be "seen", the item's unseen renewals count has been reset
Created attachment 100244 [details] [review] Bug 24083: Required atomic updates & DB update This patch adds: - An "UnseenRenewals" circulation syspref that enables/disables the functionality added in this bug - Add unseen_renewals_allowed to circulation_rules - A change to the issues & old_issues table schemas and corresponding database upgrades to add issues.unseen_renewals & old_issues.unseen_renewals - An update for the renewals letter to include a message for unseen renewals
Created attachment 100245 [details] [review] Bug 24083: Add circ rules management This patch adds the ability to set the "Unseen renewals allowed" value in the circulation rules
Created attachment 100246 [details] [review] Bug 24083: Add support for unseen_renewals This patch adds support for unseen renewals. Here we retrofit knowledge of unseen renewals and add the display of unseen renewal counts and warnings, in addition to adding the ability to specify a renewal as being "unseen". The functionality added here is goverened by the UnseenRenewals syspref.
Created attachment 100247 [details] [review] Bug 24083: Add unit tests This patch adds unit tests for unseen renewals
Created attachment 100380 [details] [review] Bug 24083: Required atomic updates & DB update This patch adds: - An "UnseenRenewals" circulation syspref that enables/disables the functionality added in this bug - Add unseen_renewals_allowed to circulation_rules - A change to the issues & old_issues table schemas and corresponding database upgrades to add issues.unseen_renewals & old_issues.unseen_renewals - An update for the renewals letter to include a message for unseen renewals Signed-off-by: Sally Healey <sally.Healey@cheshirewestandchester.gov.uk>
Created attachment 100381 [details] [review] Bug 24083: Add circ rules management This patch adds the ability to set the "Unseen renewals allowed" value in the circulation rules Signed-off-by: Sally Healey <sally.Healey@cheshirewestandchester.gov.uk>
Created attachment 100382 [details] [review] Bug 24083: Add support for unseen_renewals This patch adds support for unseen renewals. Here we retrofit knowledge of unseen renewals and add the display of unseen renewal counts and warnings, in addition to adding the ability to specify a renewal as being "unseen". The functionality added here is goverened by the UnseenRenewals syspref. Signed-off-by: Sally Healey <sally.Healey@cheshirewestandchester.gov.uk>
Created attachment 100383 [details] [review] Bug 24083: Add unit tests This patch adds unit tests for unseen renewals Signed-off-by: Sally Healey <sally.Healey@cheshirewestandchester.gov.uk>
Created attachment 100392 [details] [review] Bug 24083: (follow-up) Fix params to AddRenewal The merging of Bug 23051 has added a paramter that can be passed to C4::AddRenewal. This bug does also, so the offset of this bug's new paramter was wrong. This patch fixes it.
conflicts in opac-renew.pl and opac-user.tt the previous rebase on CanBookBeRenewed looks wrong too - it readds a lot of code at first glance here I like that this circ rule type can be disabled, but I wish it used a more generic method. Something like a table of rule names and an 'enabled' column - then we could join that when searching for effective rules and simply not use disabled ones - not a blocker
Looks like bug 19014 was where the rebase gets confused.. I've not played with the patch with It certainly does look like a lot of code was re-introduced.
Created attachment 110607 [details] [review] Bug 24083: Required atomic updates & DB update This patch adds: - An "UnseenRenewals" circulation syspref that enables/disables the functionality added in this bug - Add unseen_renewals_allowed to circulation_rules - A change to the issues & old_issues table schemas and corresponding database upgrades to add issues.unseen_renewals & old_issues.unseen_renewals - An update for the renewals letter to include a message for unseen renewals Signed-off-by: Sally Healey <sally.Healey@cheshirewestandchester.gov.uk>
Created attachment 110608 [details] [review] Bug 24083: Add circ rules management This patch adds the ability to set the "Unseen renewals allowed" value in the circulation rules Signed-off-by: Sally Healey <sally.Healey@cheshirewestandchester.gov.uk>
Created attachment 110609 [details] [review] Bug 24083: Add support for unseen_renewals This patch adds support for unseen renewals. Here we retrofit knowledge of unseen renewals and add the display of unseen renewal counts and warnings, in addition to adding the ability to specify a renewal as being "unseen". The functionality added here is goverened by the UnseenRenewals syspref. Signed-off-by: Sally Healey <sally.Healey@cheshirewestandchester.gov.uk>
Created attachment 110610 [details] [review] Bug 24083: Add unit tests This patch adds unit tests for unseen renewals Signed-off-by: Sally Healey <sally.Healey@cheshirewestandchester.gov.uk>
Created attachment 110611 [details] [review] Bug 24083: (follow-up) Fix params to AddRenewal The merging of Bug 23051 has added a paramter that can be passed to C4::AddRenewal. This bug does also, so the offset of this bug's new paramter was wrong. This patch fixes it.
- Apply patches (if on a platform requiring this to be done manually) - Run DB updates and rebuild schema (if on a platform requiring this to be done manually) - Rebuild CSS (if on a platform requiring this to be done manually) - Issue an item to a patron - Navigate to Koha Administration > Circulation and fines rules => TEST: Observe that there *isn't* a column titled "Unseen renewals allowed (count)" - Navigate to a patron's checkouts list => TEST: Observe that in the "Renew" column, only details of their remaining renewals are shown, unseen renewals are not mentioned - Navigate to a patron's "Your account" page => TEST: Observe that in the "Renew" column, only details of their remaining renewals are shown, unseen renewals are not mentioned - Turn on the "UnseenRenewals" syspref - Turn on the "OpacRenewalAllowed" syspref - Navigate to Koha Administration > Circulation and fines rules - If you have no circulation rules defined, define one that will: - Apply to all patron categories - Apply to item types - Allow renewals => TEST: Observe that there *is* a column titled "Unseen renewals allowed (count)" and the value is empty - Add a value for "Unseen renewals allowed", make the value reasonably low so we can test what happens when that value is reached - Navigate to a patron's checkouts list in the staff client => TEST: Observe that in the "Renew" column, details of their remaining unseen renewals are now shown - Navigate to a patron's "Your account" page => TEST: Observe that in the "Renew" column, details of their remaining unseen renewals are now shown - In the OPAC, renew an item => TEST: Observe that, since the renewal took place in the OPAC, the number of unseen renewals left is decremented - In the OPAC, repeatedly renew the item until the unseen renewals limit is reached => TEST: Observe that the item can no longer be renewed and the borrower is advised to take the item to the library - Navigate to the patron's checkouts list in the staff client => TEST: Observe that the item is listed as "Must be renewed at the library" and is blocked from renewal - Select the "Override renewal restrictions" checkbox => TEST: Observe that the item can now be selected for renewal - Select the item and click "Renew or check in selected items" => TEST: Observe that now the item has been renewed by the library, the unseen renewals remaining count has been reset - Select the item for renewal again and check the "Renew as unseen if appropriate" checkbox, click "Renew or check in selected items" => TEST: Observe that, since the item was renewed as "unseen", the unseen renewals remaining has been decremented - Navigate to Circulation > Renew => TEST: Observe that a new "Record renewal as unseen if appropriate" checkbox is displayed - Renew the item, do not check the unseen checkbox => TEST: Observe in the patron's checkouts list that the unseen renewals count has been reset - Renew the item, check the unseen checkbox => TEST: Observe in the patron's checkouts list that the unseen renewals count has been decremented - Navigate to Tools > Notices & Slips, edit the "AUTO_RENEWALS" Email notice => TEST: Observe that there is now a test for the "too_unseen" error, with an appropriate message SIP RENEWALS: - Use sip_cli_emulator.pl to send a renew message for the item, for example: ./misc/sip_cli_emulator.pl -a 127.0.0.1 -p 6001 -su <kiosk_username> -sp <kiosk_password> --patron <patron_username> --password <patron_password> -l CPL -i <item_barcode> -m renew - TEST: Observe that, as the SIP renewal is considered to be "seen", the item's unseen renewals count has been reset
Rebased on master
Hi Andrew, starting with some QA work here. Tbh I've never heard of this specific functionality in an ILS before and am a bit stumped on the use case. Can you explain a bit on how this is used in libraries? 1) QA script fails FAIL C4/Circulation.pm OK critic FAIL forbidden patterns forbidden pattern: Use of DateTime->now should certainly be replaced with dt_from_string (bug 24840) (line 2933) FAIL t/db_dependent/Circulation/issue.t OK critic OK forbidden patterns OK git manipulation OK pod OK spelling FAIL valid Can't locate Koha/IssuingRules.pm in @INC (you may need to install the Koha::IssuingRules module) (@INC contains: /home/vagrant/kohaclone /home/vagrant/qa-test-tools /etc/perl /usr/local/lib/x86_64-linux-gnu/perl/5.24.1 /usr/local/share/perl/5.24.1 /usr/lib/x86_64-linux-gnu/perl5/5.24 /usr/share/perl5 /usr/lib/x86_64-linux-gnu/perl/5.24 /usr/share/perl/5.24 /usr/local/lib/site_perl /usr/lib/x86_64-linux-gnu/perl-base /var/lib/koha/kohadev/plugins) 2) Database updates There is lots of them :) a) The one thing that gives me a bit of a headache is the change to the notices for existing installations. I think we should leave that one out as we cannot safely assume what people have 'done' to this notice - they will have translated it, might have decided to not inform patrons of the reasons at all, changed the structure etc. I feel we should not interfere here. b) You have a mismatch between kohastructure.sql and the updates for issues/old_issues: AFTER renewals vs. after auto_renew 3) Strings in Javascript This is an old patch, I am aware, but not sure if you have seen yet: We are moving the strings back into the js as they can be finally translated within the file now. See bug 21156 for examples. I am not going to block this, but it would be nice if the patches could be changed to use the new mechanism right away. 4) Terminology We just did a major clean-up on the system preferences, that's why I am a bit nit-picky there. Every change later requires translators to do the work again too - so it might be forgiven: + - renewals to be recorded as "unseen" by the library, and count against the borrowers unseen renewals limit Can you please make the borrowers patrons and add a .? 5) Web based self check Renewals are also possible with the web based self check - so I feel some changes will be needed there as well!
Created attachment 111147 [details] [review] Bug 24083: Required atomic updates & DB update This patch adds: - An "UnseenRenewals" circulation syspref that enables/disables the functionality added in this bug - Add unseen_renewals_allowed to circulation_rules - A change to the issues & old_issues table schemas and corresponding database upgrades to add issues.unseen_renewals & old_issues.unseen_renewals - An update for the renewals letter to include a message for unseen renewals Signed-off-by: Sally Healey <sally.Healey@cheshirewestandchester.gov.uk>
Created attachment 111148 [details] [review] Bug 24083: Add circ rules management This patch adds the ability to set the "Unseen renewals allowed" value in the circulation rules Signed-off-by: Sally Healey <sally.Healey@cheshirewestandchester.gov.uk>
Created attachment 111149 [details] [review] Bug 24083: Add support for unseen_renewals This patch adds support for unseen renewals. Here we retrofit knowledge of unseen renewals and add the display of unseen renewal counts and warnings, in addition to adding the ability to specify a renewal as being "unseen". The functionality added here is goverened by the UnseenRenewals syspref. Signed-off-by: Sally Healey <sally.Healey@cheshirewestandchester.gov.uk>
Created attachment 111150 [details] [review] Bug 24083: Add unit tests This patch adds unit tests for unseen renewals Signed-off-by: Sally Healey <sally.Healey@cheshirewestandchester.gov.uk>
Created attachment 111151 [details] [review] Bug 24083: (follow-up) Fix params to AddRenewal The merging of Bug 23051 has added a paramter that can be passed to C4::AddRenewal. This bug does also, so the offset of this bug's new paramter was wrong. This patch fixes it.
Created attachment 111152 [details] [review] Bug 24083: (follow-up) Respond to QA feedback This patch implements changes requested by Katrin in QA feedback (comment #38): - Fix QA script failures - Remove notice text database update - Fix alter table column order mismatch - Use new convention for JS strings - Fix terminology
Hi Katrin > 1) QA script fails Fixed > 2) Database updates a) This is now removed b) Fixed > 3) Strings in Javascript This was actually causing merge conflicts when I applied this bug to latest master, so I made this switch as part of the merging > 4) Terminology Fixed > 5) Web based self check I'm not sure what you mean here. Do you mean the renewal function in the "My account" in the OPAC? If so, unseen renewals are already handled there. I'm guessing you're referring to something else though...
> > > 5) Web based self check > > I'm not sure what you mean here. Do you mean the renewal function in the "My > account" in the OPAC? If so, unseen renewals are already handled there. I'm > guessing you're referring to something else though... Hi Andrew, thx for the follow-ups, what I meant is the web based self check module, see SelfCheckInModule and WebBasedSelfCheck - the latter also has the option to make renewals either by scanning in an item agan (seen?) and by renewing them directly (unseen).
Created attachment 111283 [details] [review] Bug 24083: Required atomic updates & DB update This patch adds: - An "UnseenRenewals" circulation syspref that enables/disables the functionality added in this bug - Add unseen_renewals_allowed to circulation_rules - A change to the issues & old_issues table schemas and corresponding database upgrades to add issues.unseen_renewals & old_issues.unseen_renewals - An update for the renewals letter to include a message for unseen renewals Signed-off-by: Sally Healey <sally.Healey@cheshirewestandchester.gov.uk>
Created attachment 111284 [details] [review] Bug 24083: Add circ rules management This patch adds the ability to set the "Unseen renewals allowed" value in the circulation rules Signed-off-by: Sally Healey <sally.Healey@cheshirewestandchester.gov.uk>
Created attachment 111285 [details] [review] Bug 24083: Add support for unseen_renewals This patch adds support for unseen renewals. Here we retrofit knowledge of unseen renewals and add the display of unseen renewal counts and warnings, in addition to adding the ability to specify a renewal as being "unseen". The functionality added here is goverened by the UnseenRenewals syspref. Signed-off-by: Sally Healey <sally.Healey@cheshirewestandchester.gov.uk>
Created attachment 111286 [details] [review] Bug 24083: Add unit tests This patch adds unit tests for unseen renewals Signed-off-by: Sally Healey <sally.Healey@cheshirewestandchester.gov.uk>
Created attachment 111287 [details] [review] Bug 24083: (follow-up) Fix params to AddRenewal The merging of Bug 23051 has added a paramter that can be passed to C4::AddRenewal. This bug does also, so the offset of this bug's new paramter was wrong. This patch fixes it.
Created attachment 111288 [details] [review] Bug 24083: (follow-up) Respond to QA feedback This patch implements changes requested by Katrin in QA feedback (comment #38): - Fix QA script failures - Remove notice text database update - Fix alter table column order mismatch - Use new convention for JS strings - Fix terminology
Created attachment 111289 [details] [review] Bug 24083: (follow-up) Include SelfCheckInModule As highlighted by Katrin, the selfcheckin module was not aware of this work. This commits gives it awareness of the too_unseen renew error and the "seen" argument that can be passed to AddRenewal
Created attachment 111290 [details] [review] Bug 24083: (follow-up) Fix error in checkouts JS This commit fixes a small error in the checkouts related JS
Created attachment 111291 [details] [review] Bug 24083: (follow-up) Fix GetRenewCount return GetRenewCount now returns 6 values when a call to it succeeds, a failed call should also return the same number of values. This commit adds these additional values.
Created attachment 111292 [details] [review] Bug 24083: (follow-up) Fix unit tests Some changes in issue.t had broken the tests for unseen renewals (the unseen tests were using variables that had been moved out of the tests' scope). Also now using Koha::CirculationRules::set_rules to set circ rules rather than using SQL queries. Fixed expected number of return values from GetRenewCount Moved unseen tests in issue.t to the bottom of the file to remove the risk of interference with other test circ rules.
Created attachment 111293 [details] [review] Bug 24083: (follow-up) Fix bad rebase There was a real mess in C4/Circulation.pm due to a bad rebase back in February. Frankly it's a wonder anything worked at all. This commit fixes that problem and reinstates the correct patch for C4/Circulation.pm
(In reply to Katrin Fischer from comment #46) > Hi Andrew, > > thx for the follow-ups, what I meant is the web based self check module, see > SelfCheckInModule and WebBasedSelfCheck - the latter also has the option to > make renewals either by scanning in an item agan (seen?) and by renewing > them directly (unseen). Hi Katrin Thanks for the clarification, I didn't even know this area of Koha existed! Anyway, I have now added awareness of seen vs unseen to it so should prevent renewals when appropriate with a suitable error message. I've also fixed some unit tests that were broken as a result of changes since this was last rebased. I've also fixed a major rebase error that crept in in February, to be honest it's amazing anything worked at all with it in, anyway it's fixed now. Setting this back to "Signed off" with crossed fingers...
I am sorry Andrew, but I can't get the tests to pass :( I applied the patches, ran updatedatabase and then dbic after. Can you please check? t/db_dependent/Circulation.t ........ 1/50 # Failed test 'Cannot renew, 0 unseen renewals allowed' # at t/db_dependent/Circulation.t line 1174. # got: '1' # expected: '0' # Failed test 'Cannot renew, returned code is too_unseen' # at t/db_dependent/Circulation.t line 1175. # got: undef # expected: 'too_unseen' # Looks like you failed 2 tests of 89. t/db_dependent/Circulation.t ........ 15/50 # Failed test 'CanBookBeRenewed tests' # at t/db_dependent/Circulation.t line 1288. t/db_dependent/Circulation.t ........ 50/50 # Looks like you failed 1 test of 50. t/db_dependent/Circulation.t ........ Dubious, test returned 1 (wstat 256, 0x100) Failed 1/50 subtests t/db_dependent/Circulation/issue.t .. ok Test Summary Report ------------------- t/db_dependent/Circulation.t (Wstat: 256 Tests: 50 Failed: 1) Failed test: 15 Non-zero exit status: 1
Created attachment 111849 [details] [review] Bug 24083: Required atomic updates & DB update This patch adds: - An "UnseenRenewals" circulation syspref that enables/disables the functionality added in this bug - Add unseen_renewals_allowed to circulation_rules - A change to the issues & old_issues table schemas and corresponding database upgrades to add issues.unseen_renewals & old_issues.unseen_renewals - An update for the renewals letter to include a message for unseen renewals Signed-off-by: Sally Healey <sally.Healey@cheshirewestandchester.gov.uk>
Created attachment 111850 [details] [review] Bug 24083: Add circ rules management This patch adds the ability to set the "Unseen renewals allowed" value in the circulation rules Signed-off-by: Sally Healey <sally.Healey@cheshirewestandchester.gov.uk>
Created attachment 111851 [details] [review] Bug 24083: Add support for unseen_renewals This patch adds support for unseen renewals. Here we retrofit knowledge of unseen renewals and add the display of unseen renewal counts and warnings, in addition to adding the ability to specify a renewal as being "unseen". The functionality added here is goverened by the UnseenRenewals syspref. Signed-off-by: Sally Healey <sally.Healey@cheshirewestandchester.gov.uk>
Created attachment 111852 [details] [review] Bug 24083: Add unit tests This patch adds unit tests for unseen renewals Signed-off-by: Sally Healey <sally.Healey@cheshirewestandchester.gov.uk>
Created attachment 111853 [details] [review] Bug 24083: (follow-up) Fix params to AddRenewal The merging of Bug 23051 has added a paramter that can be passed to C4::AddRenewal. This bug does also, so the offset of this bug's new paramter was wrong. This patch fixes it.
Created attachment 111854 [details] [review] Bug 24083: (follow-up) Respond to QA feedback This patch implements changes requested by Katrin in QA feedback (comment #38): - Fix QA script failures - Remove notice text database update - Fix alter table column order mismatch - Use new convention for JS strings - Fix terminology
Created attachment 111855 [details] [review] Bug 24083: (follow-up) Include SelfCheckInModule As highlighted by Katrin, the selfcheckin module was not aware of this work. This commits gives it awareness of the too_unseen renew error and the "seen" argument that can be passed to AddRenewal
Created attachment 111856 [details] [review] Bug 24083: (follow-up) Fix error in checkouts JS This commit fixes a small error in the checkouts related JS
Created attachment 111857 [details] [review] Bug 24083: (follow-up) Fix GetRenewCount return GetRenewCount now returns 6 values when a call to it succeeds, a failed call should also return the same number of values. This commit adds these additional values.
Created attachment 111858 [details] [review] Bug 24083: (follow-up) Fix unit tests Some changes in issue.t had broken the tests for unseen renewals (the unseen tests were using variables that had been moved out of the tests' scope). Also now using Koha::CirculationRules::set_rules to set circ rules rather than using SQL queries. Fixed expected number of return values from GetRenewCount Moved unseen tests in issue.t to the bottom of the file to remove the risk of interference with other test circ rules.
Created attachment 111859 [details] [review] Bug 24083: (follow-up) Fix bad rebase There was a real mess in C4/Circulation.pm due to a bad rebase back in February. Frankly it's a wonder anything worked at all. This commit fixes that problem and reinstates the correct patch for C4/Circulation.pm
Created attachment 111860 [details] [review] Bug 24083: (follow-up) Fix bad rebase This commit fixes a couple of lines that got missed during the last rebase
Sorry Katrin, another bad rebase. All these rebases are a nightmare for introducing errors. Anyway, hopefully it will now be good.
Could someone please take over testing the SIP bit of the test plan for me?
Done lots of successful testing, but getting stuck a bit at the mechanics of this: 1) I turned on the unseen/seen renewals feature. My total allowed renewals are 5. Unseen 4. My checked out item in the OPAC now shows: Renew ( 2 of 5 renewals remaining / 4 of 4 renewals left before the item must be seen by the library ) In this case it's extra confusing because I have done renewals before the feature was activated. Then I have renewed the item by checking it out again. It now reads: Renew ( 1 of 5 renewals remaining / 4 of 4 renewals left before the item must be seen by the library ) So it appears to have reset the renewals. But: If I renew the item in the OPAC now, I get: Renewed! Not renewable I feel like it's confusing to patrons that it says I have x renewals left, but this number is smaller than the unseen renewals available. It would make sense if the "seen" renewal reset the counter, but that doesn't appear to happen. 2) Checked my item in again and back out to reset all counters. OPAC reads: Renew ( 5 of 5 renewals remaining / 4 of 4 renewals left before the item must be seen by the library ) Renew once in the OPAC. Renew ( 4 of 5 renewals remaining / 3 of 4 renewals left before the item must be seen by the library ) Renew using the renew tab in staff client: ( 3 of 5 renewals remaining / 4 of 4 renewals left before the item must be seen by the library ) So this appears to have reset the "unseen" renewals, which I think is correct. My issue here is that I think it will be very confusing to the patrons, do they have 3 or 4 renewals left now? (actually only 3, otherwise the item has to be checked in and out again and everything will be reset. Maybe we should just show the patrons how many more online renewals are possible before they have to take the item to the library for renewal? OR: ( 3 of 5 renewals remaining / 4 of 4 renewals left before the item must be seen by the library ) Would change to the following, when the unseen renewals are higher than the actual remaining ones: ( 3 of 5 renewals remaining ) AND: ( 4 of 5 renewals remaining / 2 of 2 renewals left before the item must be seen by the library ) Could be: ( 2 online renewals remaining before the item must be seen by the library ) I guess besides the confusing display I wonder if the count of renewals should be reset by a seen renewal?
3) New test case: - 5 renewals allowed - 2 renewals unseen - Item checked out - Renewed twice via the OPAC - Now no renewal information displays, the cell is empty Expected: Must be renewed at the library and some information about remaining renewals. Where it's very verbose before, once you reach that state, there is no information to the user at all.
4) Please add the new test for too_unseen to the AUTO_RENEW default notices, so that new installations will have it and we have a default notice to refer to. Addition to 1) and 2), trying to be clearer about the issue: In my last test case I see: Renew ( 4 of 5 renewals remaining / 1 of 2 renewals left before the item must be seen by the library ) This does kind of make sense to me now. I still think it might be clearer to leave out the unseen renewals if they no longer apply, because all remaining renewals can be done online. (2 of 5, 2 of 2 => 2 of 5) I think 3) and 4) should be fixed. 1) and 2) might be good to put some more thought into separately, not blocker for me.
Hi Katrin Thanks for that, and for all the detail of your testing procedures. > 1) Yes, I think you're right, some improvements could be made to consolidate the various renewal counts depending on which is more pertinent, your ideas make sense. I'll see what I can do > 2) Checked my item in again and back out to reset all counters. > [...] > I guess besides the confusing display I wonder if the count of renewals > should be reset by a seen renewal? Do you mean the unseen count should be reset by a seen renewal? That is what should happen. In all my testing it has worked correctly, am I missing your point? > 3. New test case: > [...] > - Now no renewal information displays, the cell is empty That sounds like a bug, I'll check it out > 4) Please add the new test for too_unseen to the AUTO_RENEW default notices, so > that new installations will have it and we have a default notice to refer to. Will do
Created attachment 112230 [details] [review] Bug 24083: Required atomic updates & DB update This patch adds: - An "UnseenRenewals" circulation syspref that enables/disables the functionality added in this bug - Add unseen_renewals_allowed to circulation_rules - A change to the issues & old_issues table schemas and corresponding database upgrades to add issues.unseen_renewals & old_issues.unseen_renewals - An update for the renewals letter to include a message for unseen renewals Signed-off-by: Sally Healey <sally.Healey@cheshirewestandchester.gov.uk>
Created attachment 112231 [details] [review] Bug 24083: Add circ rules management This patch adds the ability to set the "Unseen renewals allowed" value in the circulation rules Signed-off-by: Sally Healey <sally.Healey@cheshirewestandchester.gov.uk>
Created attachment 112232 [details] [review] Bug 24083: Add support for unseen_renewals This patch adds support for unseen renewals. Here we retrofit knowledge of unseen renewals and add the display of unseen renewal counts and warnings, in addition to adding the ability to specify a renewal as being "unseen". The functionality added here is goverened by the UnseenRenewals syspref. Signed-off-by: Sally Healey <sally.Healey@cheshirewestandchester.gov.uk>
Created attachment 112233 [details] [review] Bug 24083: Add unit tests This patch adds unit tests for unseen renewals Signed-off-by: Sally Healey <sally.Healey@cheshirewestandchester.gov.uk>
Created attachment 112234 [details] [review] Bug 24083: (follow-up) Fix params to AddRenewal The merging of Bug 23051 has added a paramter that can be passed to C4::AddRenewal. This bug does also, so the offset of this bug's new paramter was wrong. This patch fixes it.
Created attachment 112235 [details] [review] Bug 24083: (follow-up) Respond to QA feedback This patch implements changes requested by Katrin in QA feedback (comment #38): - Fix QA script failures - Remove notice text database update - Fix alter table column order mismatch - Use new convention for JS strings - Fix terminology
Created attachment 112236 [details] [review] Bug 24083: (follow-up) Include SelfCheckInModule As highlighted by Katrin, the selfcheckin module was not aware of this work. This commits gives it awareness of the too_unseen renew error and the "seen" argument that can be passed to AddRenewal
Created attachment 112237 [details] [review] Bug 24083: (follow-up) Fix error in checkouts JS This commit fixes a small error in the checkouts related JS
Created attachment 112238 [details] [review] Bug 24083: (follow-up) Fix GetRenewCount return GetRenewCount now returns 6 values when a call to it succeeds, a failed call should also return the same number of values. This commit adds these additional values.
Created attachment 112239 [details] [review] Bug 24083: (follow-up) Fix unit tests Some changes in issue.t had broken the tests for unseen renewals (the unseen tests were using variables that had been moved out of the tests' scope). Also now using Koha::CirculationRules::set_rules to set circ rules rather than using SQL queries. Fixed expected number of return values from GetRenewCount Moved unseen tests in issue.t to the bottom of the file to remove the risk of interference with other test circ rules.
Created attachment 112240 [details] [review] Bug 24083: (follow-up) Fix bad rebase There was a real mess in C4/Circulation.pm due to a bad rebase back in February. Frankly it's a wonder anything worked at all. This commit fixes that problem and reinstates the correct patch for C4/Circulation.pm
Created attachment 112241 [details] [review] Bug 24083: (follow-up) Fix bad rebase This commit fixes a couple of lines that got missed during the last rebase
Created attachment 112242 [details] [review] Bug 24083: (follow-up) Fix misaligned columns Somehow I'd never noticed this before but the columns in smart-rules.tt were misaligned when UnseenRenewals was turned off. This was due to the display of a <td> not being conditional when it should have been. This is now fixed.
Created attachment 112243 [details] [review] Bug 24083: (follow-up) Fix QA issues This commit fixes items 1 & 2 descibed by Katrin in comment #74 -> comment #76. - Fixed missing check for too_unseen in opac-user.tt, this test did used to exist but got lost during sizeable rebase a few weeks ago :-( - Added test for too_unseen to all AUTO_RENEWAL notice templates apart from de-DE (as previously requested by Katrin)
Hi Katrin I've just fixed 1 & 2. 1 was due to a mistake during a rebase back in early October. For 2, I didn't modify the de-DE notice as you previously requested. As far as 3 & 4 are concerned, I'd be super keen for those to be enhancements after this is PQA. As has just been proved, repeatedly rebasing this is tricky as it's quite a sprawling piece and every time introduces the potential for error. So I, for one, would be keen to get it in as it is, then address any enhancements to wording, particularly as they're likely to be subjective.
(In reply to Andrew Isherwood from comment #92) > Hi Katrin > > I've just fixed 1 & 2. 1 was due to a mistake during a rebase back in early > October. For 2, I didn't modify the de-DE notice as you previously requested. > > As far as 3 & 4 are concerned, I'd be super keen for those to be > enhancements after this is PQA. As has just been proved, repeatedly rebasing > this is tricky as it's quite a sprawling piece and every time introduces the > potential for error. So I, for one, would be keen to get it in as it is, > then address any enhancements to wording, particularly as they're likely to > be subjective. I think you meant fixed 3 and 4 and to wait with 1 and 2 :) But I am ok with this as I wrote before, improving language is not blocker for me right now.
Created attachment 112410 [details] [review] Bug 24083: Required atomic updates & DB update This patch adds: - An "UnseenRenewals" circulation syspref that enables/disables the functionality added in this bug - Add unseen_renewals_allowed to circulation_rules - A change to the issues & old_issues table schemas and corresponding database upgrades to add issues.unseen_renewals & old_issues.unseen_renewals - An update for the renewals letter to include a message for unseen renewals Signed-off-by: Sally Healey <sally.Healey@cheshirewestandchester.gov.uk> Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Created attachment 112411 [details] [review] Bug 24083: Add circ rules management This patch adds the ability to set the "Unseen renewals allowed" value in the circulation rules Signed-off-by: Sally Healey <sally.Healey@cheshirewestandchester.gov.uk> Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Created attachment 112412 [details] [review] Bug 24083: Add support for unseen_renewals This patch adds support for unseen renewals. Here we retrofit knowledge of unseen renewals and add the display of unseen renewal counts and warnings, in addition to adding the ability to specify a renewal as being "unseen". The functionality added here is goverened by the UnseenRenewals syspref. Signed-off-by: Sally Healey <sally.Healey@cheshirewestandchester.gov.uk> Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Created attachment 112413 [details] [review] Bug 24083: Add unit tests This patch adds unit tests for unseen renewals Signed-off-by: Sally Healey <sally.Healey@cheshirewestandchester.gov.uk> Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Created attachment 112414 [details] [review] Bug 24083: (follow-up) Fix params to AddRenewal The merging of Bug 23051 has added a paramter that can be passed to C4::AddRenewal. This bug does also, so the offset of this bug's new paramter was wrong. This patch fixes it. Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Created attachment 112415 [details] [review] Bug 24083: (follow-up) Respond to QA feedback This patch implements changes requested by Katrin in QA feedback (comment #38): - Fix QA script failures - Remove notice text database update - Fix alter table column order mismatch - Use new convention for JS strings - Fix terminology Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Created attachment 112416 [details] [review] Bug 24083: (follow-up) Include SelfCheckInModule As highlighted by Katrin, the selfcheckin module was not aware of this work. This commits gives it awareness of the too_unseen renew error and the "seen" argument that can be passed to AddRenewal Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Created attachment 112417 [details] [review] Bug 24083: (follow-up) Fix error in checkouts JS This commit fixes a small error in the checkouts related JS Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Created attachment 112418 [details] [review] Bug 24083: (follow-up) Fix GetRenewCount return GetRenewCount now returns 6 values when a call to it succeeds, a failed call should also return the same number of values. This commit adds these additional values. Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Created attachment 112419 [details] [review] Bug 24083: (follow-up) Fix unit tests Some changes in issue.t had broken the tests for unseen renewals (the unseen tests were using variables that had been moved out of the tests' scope). Also now using Koha::CirculationRules::set_rules to set circ rules rather than using SQL queries. Fixed expected number of return values from GetRenewCount Moved unseen tests in issue.t to the bottom of the file to remove the risk of interference with other test circ rules. Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Created attachment 112420 [details] [review] Bug 24083: (follow-up) Fix bad rebase There was a real mess in C4/Circulation.pm due to a bad rebase back in February. Frankly it's a wonder anything worked at all. This commit fixes that problem and reinstates the correct patch for C4/Circulation.pm Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Created attachment 112421 [details] [review] Bug 24083: (follow-up) Fix bad rebase This commit fixes a couple of lines that got missed during the last rebase Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Created attachment 112422 [details] [review] Bug 24083: (follow-up) Fix misaligned columns Somehow I'd never noticed this before but the columns in smart-rules.tt were misaligned when UnseenRenewals was turned off. This was due to the display of a <td> not being conditional when it should have been. This is now fixed. Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Created attachment 112423 [details] [review] Bug 24083: (follow-up) Fix QA issues This commit fixes items 1 & 2 descibed by Katrin in comment #74 -> comment #76. - Fixed missing check for too_unseen in opac-user.tt, this test did used to exist but got lost during sizeable rebase a few weeks ago :-( - Added test for too_unseen to all AUTO_RENEWAL notice templates apart from de-DE (as previously requested by Katrin) Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Andrew, could you try to squash some patches here? Especially the atomicupdate file that is added then removed, and the 2 "Fix bad rebase" patches. And more if you can (the several tiny "fix" patches).
Created attachment 113073 [details] [review] Bug 24083: Required atomic updates & DB update This patch adds: - An "UnseenRenewals" circulation syspref that enables/disables the functionality added in this bug - Add unseen_renewals_allowed to circulation_rules - A change to the issues & old_issues table schemas and corresponding database upgrades to add issues.unseen_renewals & old_issues.unseen_renewals Signed-off-by: Sally Healey <sally.Healey@cheshirewestandchester.gov.uk> Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Created attachment 113074 [details] [review] Bug 24083: Add circ rules management This patch adds the ability to set the "Unseen renewals allowed" value in the circulation rules Signed-off-by: Sally Healey <sally.Healey@cheshirewestandchester.gov.uk> Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Created attachment 113075 [details] [review] Bug 24083: Add support for unseen_renewals This patch adds support for unseen renewals. Here we retrofit knowledge of unseen renewals and add the display of unseen renewal counts and warnings, in addition to adding the ability to specify a renewal as being "unseen". The functionality added here is goverened by the UnseenRenewals syspref. Signed-off-by: Sally Healey <sally.Healey@cheshirewestandchester.gov.uk> Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Created attachment 113076 [details] [review] Bug 24083: Add unit tests This patch adds unit tests for unseen renewals Signed-off-by: Sally Healey <sally.Healey@cheshirewestandchester.gov.uk> Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Created attachment 113077 [details] [review] Bug 24083: (follow-up) Fix params to AddRenewal The merging of Bug 23051 has added a paramter that can be passed to C4::AddRenewal. This bug does also, so the offset of this bug's new paramter was wrong. This patch fixes it. Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Created attachment 113078 [details] [review] Bug 24083: (follow-up) Respond to QA feedback This patch implements changes requested by Katrin in QA feedback (comment #38): - Fix QA script failures - Fix alter table column order mismatch - Use new convention for JS strings - Fix terminology Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Created attachment 113079 [details] [review] Bug 24083: (follow-up) Include SelfCheckInModule As highlighted by Katrin, the selfcheckin module was not aware of this work. This commits gives it awareness of the too_unseen renew error and the "seen" argument that can be passed to AddRenewal Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Created attachment 113080 [details] [review] Bug 24083: (follow-up) Squashed follow ups This squashed commit fixes: - a small error in the checkouts related JS - GetRenewCount now returns 6 values when a call to it succeeds, a failed call should also return the same number of values. This commit adds these additional values. - Some changes in issue.t had broken the tests for unseen renewals (the unseen tests were using variables that had been moved out of the tests' scope). - Also now using Koha::CirculationRules::set_rules to set circ rules rather than using SQL queries. - Fixed expected number of return values from GetRenewCount - Moved unseen tests in issue.t to the bottom of the file to remove the risk of interference with other test circ rules. - There was a real mess in C4/Circulation.pm due to a bad rebase back in February. Frankly it's a wonder anything worked at all. This commit fixes that problem and reinstates the correct patch for C4/Circulation.pm - Somehow I'd never noticed this before but the columns in smart-rules.tt were misaligned when UnseenRenewals was turned off. This was due to the display of a <td> not being conditional when it should have been. This is now fixed. - This commit also fixes items 1 & 2 descibed by Katrin in comment #74 -> comment #76. - Fixed missing check for too_unseen in opac-user.tt, this test did used to exist but got lost during sizeable rebase a few weeks ago :-( - Added test for too_unseen to all AUTO_RENEWAL notice templates apart from de-DE (as previously requested by Katrin) Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Hi Jonathan - squashed as requested
Created attachment 113314 [details] [review] Bug 24083: Delete the new rule when needed
Created attachment 113315 [details] [review] Bug 24083: Swap the column for "Unlimited" handling
Hi Andrew, I think I've found some problem with this enhancement. 1. I have set the default circ rule: unseen_renewals_allowed | 2 renewalsallowed | 5 And an issue has: renewals: 2 unseen_renewals: 2 so number of unseen is reached. I am expecting a staff member to be allowed to renew, however I get: Cannot renew: E Street shuffle : ( BC_1 ) has been renewed the maximum number of consecutive times without being seen by the library ) Can you confirm it's a bug? 2. If 1. is valid, why cannot we renew from circulation.pl if the max number of renewals is not reached (but number of unseen renewals is)? 3. If I am understanding correctly, the AddRenewal call in sco-main.pl always has $seen set to 1. Maybe we should then make it hardcoded in the controller (ie. no need to have the template var) 4. It seems that the too_unseen error should be handled in misc/cronjobs/automatic_renewals.pl Don't you think? It can still be part of 20.11 if I get answers/follow-ups before Wednesday.
Created attachment 113484 [details] [review] Bug 24083: Required atomic updates & DB update This patch adds: - An "UnseenRenewals" circulation syspref that enables/disables the functionality added in this bug - Add unseen_renewals_allowed to circulation_rules - A change to the issues & old_issues table schemas and corresponding database upgrades to add issues.unseen_renewals & old_issues.unseen_renewals Signed-off-by: Sally Healey <sally.Healey@cheshirewestandchester.gov.uk> Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Created attachment 113485 [details] [review] Bug 24083: Add circ rules management This patch adds the ability to set the "Unseen renewals allowed" value in the circulation rules Signed-off-by: Sally Healey <sally.Healey@cheshirewestandchester.gov.uk> Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Created attachment 113486 [details] [review] Bug 24083: Add support for unseen_renewals This patch adds support for unseen renewals. Here we retrofit knowledge of unseen renewals and add the display of unseen renewal counts and warnings, in addition to adding the ability to specify a renewal as being "unseen". The functionality added here is goverened by the UnseenRenewals syspref. Signed-off-by: Sally Healey <sally.Healey@cheshirewestandchester.gov.uk> Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Created attachment 113487 [details] [review] Bug 24083: Add unit tests This patch adds unit tests for unseen renewals Signed-off-by: Sally Healey <sally.Healey@cheshirewestandchester.gov.uk> Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Created attachment 113488 [details] [review] Bug 24083: (follow-up) Fix params to AddRenewal The merging of Bug 23051 has added a paramter that can be passed to C4::AddRenewal. This bug does also, so the offset of this bug's new paramter was wrong. This patch fixes it. Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Created attachment 113489 [details] [review] Bug 24083: (follow-up) Respond to QA feedback This patch implements changes requested by Katrin in QA feedback (comment #38): - Fix QA script failures - Fix alter table column order mismatch - Use new convention for JS strings - Fix terminology Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Created attachment 113490 [details] [review] Bug 24083: (follow-up) Include SelfCheckInModule As highlighted by Katrin, the selfcheckin module was not aware of this work. This commits gives it awareness of the too_unseen renew error and the "seen" argument that can be passed to AddRenewal Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Created attachment 113491 [details] [review] Bug 24083: (follow-up) Squashed follow ups This squashed commit fixes: - a small error in the checkouts related JS - GetRenewCount now returns 6 values when a call to it succeeds, a failed call should also return the same number of values. This commit adds these additional values. - Some changes in issue.t had broken the tests for unseen renewals (the unseen tests were using variables that had been moved out of the tests' scope). - Also now using Koha::CirculationRules::set_rules to set circ rules rather than using SQL queries. - Fixed expected number of return values from GetRenewCount - Moved unseen tests in issue.t to the bottom of the file to remove the risk of interference with other test circ rules. - There was a real mess in C4/Circulation.pm due to a bad rebase back in February. Frankly it's a wonder anything worked at all. This commit fixes that problem and reinstates the correct patch for C4/Circulation.pm - Somehow I'd never noticed this before but the columns in smart-rules.tt were misaligned when UnseenRenewals was turned off. This was due to the display of a <td> not being conditional when it should have been. This is now fixed. - This commit also fixes items 1 & 2 descibed by Katrin in comment #74 -> comment #76. - Fixed missing check for too_unseen in opac-user.tt, this test did used to exist but got lost during sizeable rebase a few weeks ago :-( - Added test for too_unseen to all AUTO_RENEWAL notice templates apart from de-DE (as previously requested by Katrin) Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Created attachment 113492 [details] [review] Bug 24083: Delete the new rule when needed
Created attachment 113493 [details] [review] Bug 24083: Swap the column for "Unlimited" handling
Created attachment 113494 [details] [review] Bug 24083: (follow-up) Make requested changes This commit makes the changes requested in comment #120
(In reply to Jonathan Druart from comment #120) Hi Jonathan > Cannot renew: > > E Street shuffle : ( BC_1 ) has been renewed the maximum number of > consecutive times without being seen by the library ) > > Can you confirm it's a bug? I think you've missed the step in the test plan that says "Select the "Override renewal restrictions" checkbox" with this enabled, you will have the ability to override the restriction via a "Override limit and renew button" > 3. If I am understanding correctly, the AddRenewal call in sco-main.pl > always has $seen set to 1. Maybe we should then make it hardcoded in the > controller (ie. no need to have the template var) Good point, now changed > > 4. It seems that the too_unseen error should be handled in > misc/cronjobs/automatic_renewals.pl > Don't you think? Good point, now added > It can still be part of 20.11 if I get answers/follow-ups before Wednesday. Sorry, I somehow missed the email for this bug until this morning, hence my reply today!
(In reply to Andrew Isherwood from comment #132) > (In reply to Jonathan Druart from comment #120) > > Hi Jonathan > > > Cannot renew: > > > > E Street shuffle : ( BC_1 ) has been renewed the maximum number of > > consecutive times without being seen by the library ) > > > > Can you confirm it's a bug? > > I think you've missed the step in the test plan that says "Select the > "Override renewal restrictions" checkbox" with this enabled, you will have > the ability to override the restriction via a "Override limit and renew > button" Right, I missed that! > > 4. It seems that the too_unseen error should be handled in > > misc/cronjobs/automatic_renewals.pl > > Don't you think? > > Good point, now added But then, if we decide to send the notice, the AUTO_RENEWALS template will need to be adjust. Otherwise the notice will be generated without a correct content.
(In reply to Jonathan Druart from comment #133) > > > 4. It seems that the too_unseen error should be handled in > > > misc/cronjobs/automatic_renewals.pl > > > Don't you think? > > > > Good point, now added > > But then, if we decide to send the notice, the AUTO_RENEWALS template will > need to be adjust. Otherwise the notice will be generated without a correct > content. too_unseen is catered for in the AUTO_RENEWALS sample notice
Pushed to master for 20.11, thanks to everybody involved!
Pushed but more work needed: 1. > > 4. It seems that the too_unseen error should be handled in > > misc/cronjobs/automatic_renewals.pl > > Don't you think? > > Good point, now added But then, if we decide to send the notice, the AUTO_RENEWALS template will need to be adjust. Otherwise the notice will be generated without a correct content. 2. The insert from the upgrade process is not correct. The rule_value is not passed. But even if passed it's not idempotent (ie. the row in inserted several times if run several times).
3. We have 2 failing tests $ prove t/db_dependent/api/v1/checkouts.t t/db_dependent/api/v1/checkouts.t .. 7/93 # Failed test 'exact match for JSON Pointer ""' # at t/db_dependent/api/v1/checkouts.t line 198. # Structures begin differing at: # $got->{unseen_renewals} = '0' # $expected->{unseen_renewals} = Does not exist t/db_dependent/api/v1/checkouts.t .. 84/93 # Failed test 'exact match for JSON Pointer ""' # at t/db_dependent/api/v1/checkouts.t line 217. # Structures begin differing at: # $got->{unseen_renewals} = '0' # $expected->{unseen_renewals} = Does not exist # Looks like you failed 2 tests of 93.
(In reply to Jonathan Druart from comment #136) > 2. The insert from the upgrade process is not correct. The rule_value is not > passed. But even if passed it's not idempotent (ie. the row in inserted > several times if run several times). Hi Jonathan - thanks for that. I'm afraid I'm not sure what you mean here. Are you talking about the circulation rule DB upgrade. As far as I can see it works and is idempotent. This makes me think you're talking about something else. Any chance you could clarify please?
Created attachment 113546 [details] [review] Bug 24083: (QA follow-up) Fix failing tests This commit fixes the failing tests mentioned by Jonathan in comment
(In reply to Andrew Isherwood from comment #138) > (In reply to Jonathan Druart from comment #136) > > > 2. The insert from the upgrade process is not correct. The rule_value is not > > passed. But even if passed it's not idempotent (ie. the row in inserted > > several times if run several times). > > Hi Jonathan - thanks for that. I'm afraid I'm not sure what you mean here. > Are you talking about the circulation rule DB upgrade. As far as I can see > it works and is idempotent. This makes me think you're talking about > something else. Any chance you could clarify please? No it's not: MySQL [koha_kohadev]> select count(*) from circulation_rules; +----------+ | count(*) | +----------+ | 26 | +----------+ 1 row in set (0.02 sec) MySQL [koha_kohadev]> INSERT IGNORE INTO circulation_rules (rule_name, rule_value) VALUES ('unseen_renewals_allowed', ''); Query OK, 1 row affected (0.02 sec) MySQL [koha_kohadev]> INSERT IGNORE INTO circulation_rules (rule_name, rule_value) VALUES ('unseen_renewals_allowed', ''); Query OK, 1 row affected (0.01 sec) MySQL [koha_kohadev]> select count(*) from circulation_rules; +----------+ | count(*) | +----------+ | 28 | +----------+ 1 row in set (0.01 sec)
Created attachment 113556 [details] [review] Bug 24083: (QA follow-up) Fix DB upgrade This commit makes the addition of the 'unseen_renewals_allowed' circulation rule idempotent, as highlighted by Jonathan in comment #140
(In reply to Jonathan Druart from comment #140) > (In reply to Andrew Isherwood from comment #138) > > (In reply to Jonathan Druart from comment #136) > > > > > 2. The insert from the upgrade process is not correct. The rule_value is not > > > passed. But even if passed it's not idempotent (ie. the row in inserted > > > several times if run several times). > > > > Hi Jonathan - thanks for that. I'm afraid I'm not sure what you mean here. > > Are you talking about the circulation rule DB upgrade. As far as I can see > > it works and is idempotent. This makes me think you're talking about > > something else. Any chance you could clarify please? > > No it's not: > > MySQL [koha_kohadev]> select count(*) from circulation_rules; > +----------+ > | count(*) | > +----------+ > | 26 | > +----------+ > 1 row in set (0.02 sec) > > MySQL [koha_kohadev]> INSERT IGNORE INTO circulation_rules > (rule_name, rule_value) VALUES ('unseen_renewals_allowed', ''); > Query OK, 1 row affected (0.02 sec) > > MySQL [koha_kohadev]> INSERT IGNORE INTO circulation_rules > (rule_name, rule_value) VALUES ('unseen_renewals_allowed', ''); > Query OK, 1 row affected (0.01 sec) > > MySQL [koha_kohadev]> select count(*) from circulation_rules; > +----------+ > | count(*) | > +----------+ > | 28 | > +----------+ > 1 row in set (0.01 sec) Aha, you're right! Now fixed.
As an aside, while looking through updatedatabase.pl for how this is usually handled in DB upgrades, I noticed the upgrade in Bug 14866 has the same problemhttps://bugs.koha-community.org/bugzilla3/attachment.cgi?id=112225&action=diff
Created attachment 113557 [details] [review] Bug 24083: (QA follow-up) Fix for previous It helps if you don't commit commented out code...
Last patches squashed and pushed, thanks Andrew!
enhancement will not be backported to 20.05.x
Created attachment 113814 [details] [review] Bug 24083: (follow-up) Assign renewals remaining string to content
(In reply to Nick Clemens from comment #147) > Created attachment 113814 [details] [review] [review] > Bug 24083: (follow-up) Assign renewals remaining string to content Pushed to master for 20.11
Enhancement, not backported to 20.05