Description
Nick Clemens (kidclamp)
2020-01-22 00:53:50 UTC
Created attachment 97766 [details] [review] Bug 24476: Database updates Created attachment 97767 [details] [review] Bug 24476: API Updates Created attachment 97768 [details] [review] Bug 24476: Change CanBookBeRenewed and adjust tests Created attachment 97769 [details] [review] Bug 24476: DO NOT PUSH - Schema updates is_boolean line must be copied however Created attachment 97770 [details] [review] Bug 24476: Allow patrons to opt out of auto-renewal To test: 1 - Verify on staff side that patron can be edited to opt in our out of auto renewal 2 - Check out some items to a patron opted in to auto renewal 3 - Ensure the items are checked out and set to autorenew 4 - Login on the opac at the patron 5 - Verify items cannot be renewed as scheduled for auto-renewal 6 - On staff side, opt patron out of auto renewal 7 - Verify on opac items are no longer marked for auto renewal 8 - Run the auto renewal cron job, items are not renewed 9 - Set 'no renewal before' to a setting that would prevent renewal 10 - Verify that opting patron in or out of auto renewal changes only the reason items cannot be renewed 11 - Set 'no renewal before' to a setting that would allow for renewal 12 - Verify that opting patron in/out changes their ability to renew 13 - Verify that when opted out cron does not renew 14 - Verify that when opted in the item is auto renewed 15 - Reset the due date, opt out, verify manual renewal succeeds Failing on step 7. Once I've opted the patron out of auto-renew, they still show on the OPAC as scheduled for auto-renew. Taking a step back, I think this could be a little more clear if we weren't defining it as a negative. Right now it's "select Yes to not auto-renew, select No to not not auto-renew." How about "Allow auto-renew: Yes/No" rather than "Disallow auto-renew: Yes/No"? It doesn't look like there's a way to turn off this feature. Could it be added to patron category configuration? I know my library would not want patrons to be able to opt out. (In reply to Owen Leonard from comment #7) > It doesn't look like there's a way to turn off this feature. Could it be > added to patron category configuration? I know my library would not want > patrons to be able to opt out. I agree - academic libraries here might not want that either. To Owen's concern, as is there isn't a way for the patron to opt out for themselves from the OPAC, so that makes it easier for a library to disallow it. But ... Seems like if we're going to add this feature it'd be good to also give the patron the option to opt out for themselves from the OPAC. Following existing conventions, this sounds like two sysprefs and a new toggle on the category: IntranetAutoRenewOptOut OPACAutoRenewOptOut and then a thingy in the category to allow an individual category to differ from the syspref (In reply to Andrew Fuerste-Henry from comment #9) > To Owen's concern, as is there isn't a way for the patron to opt out for > themselves from the OPAC They can submit the change via the same form they use to update their other personal details. However, this reveals another bug: The request doesn't show up in the staff client in the "Update patron records" page. It shows that a request was made but doesn't list it. > Following existing conventions, this sounds like two sysprefs and a new > toggle on the category: > IntranetAutoRenewOptOut > OPACAutoRenewOptOut Is there a need for the global preferences if the option can be configured at the patron category level? Seems redundant. Whoops, right you are. Yeah, that doesn't exactly fit with the personal details updating stuff. Maybe it needs to be broken out somehow? As we do with privacy choices and messaging preferences? I'm not married to the idea of a syspref AND a category configuration, that's just how we've handled other things, like the ability to do a password reset. On the one hand, it's nice to have one main switch for libraries that want all their categories to behave the same. On the other hand, it's always good to avoid making a syspref if possible. Created attachment 97831 [details] [review] Bug 24476: Database updates Created attachment 97832 [details] [review] Bug 24476: API Updates Created attachment 97833 [details] [review] Bug 24476: Change CanBookBeRenewed and adjust tests Created attachment 97834 [details] [review] Bug 24476: Allow patrons to opt out of auto-renewal To test: 1 - Verify on staff side that patron can be edited to opt in our out of auto renewal 2 - Check out some items to a patron opted in to auto renewal 3 - Ensure the items are checked out and set to autorenew 4 - Login on the opac at the patron 5 - Verify items cannot be renewed as scheduled for auto-renewal 6 - On staff side, opt patron out of auto renewal 7 - Verify on opac items are no longer marked for auto renewal 8 - Run the auto renewal cron job, items are not renewed 9 - Set 'no renewal before' to a setting that would prevent renewal 10 - Verify that opting patron in or out of auto renewal changes only the reason items cannot be renewed 11 - Set 'no renewal before' to a setting that would allow for renewal 12 - Verify that opting patron in/out changes their ability to renew 13 - Verify that when opted out cron does not renew 14 - Verify that when opted in the item is auto renewed 15 - Reset the due date, opt out, verify manual renewal succeeds Created attachment 97835 [details] [review] Bug 24476: DO NOT PUSH - Schema updates Created attachment 97836 [details] [review] Bug 24476: Add boolean for all TINYINT I rewrote the column to be a positive instead of negative - all patrons are 'opted in' then can prevent autorenewal I altered the update to automatically add this field to the unwanted prefs for opac update and self-reg. I still need to address the column not showing up on patron update requests Created attachment 97923 [details] [review] Bug 24476: Database updates Created attachment 97924 [details] [review] Bug 24476: API Updates Created attachment 97925 [details] [review] Bug 24476: Change CanBookBeRenewed and adjust tests Created attachment 97926 [details] [review] Bug 24476: Allow patrons to opt out of auto-renewal To test: 1 - Verify on staff side that patron can be edited to opt in our out of auto renewal 2 - Check out some items to a patron opted in to auto renewal 3 - Ensure the items are checked out and set to autorenew 4 - Login on the opac at the patron 5 - Verify items cannot be renewed as scheduled for auto-renewal 6 - On staff side, opt patron out of auto renewal 7 - Verify on opac items are no longer marked for auto renewal 8 - Run the auto renewal cron job, items are not renewed 9 - Set 'no renewal before' to a setting that would prevent renewal 10 - Verify that opting patron in or out of auto renewal changes only the reason items cannot be renewed 11 - Set 'no renewal before' to a setting that would allow for renewal 12 - Verify that opting patron in/out changes their ability to renew 13 - Verify that when opted out cron does not renew 14 - Verify that when opted in the item is auto renewed 15 - Reset the due date, opt out, verify manual renewal succeeds Created attachment 97927 [details] [review] Bug 24476: DO NOT PUSH - Schema updates Created attachment 97928 [details] [review] Bug 24476: Add boolean for all TINYINT New test plan: 1 - Set loan period and No Renewal Before such than an item is eligable for renewal immediately upon checkout 2 - Check out an item to a patron opted in to auto renewal 3 - Ensure the item is checked out and set to autorenew 4 - Opt patron out of auto-renew 5 - Verify that patron's checkout now shows as available for manual renewal 6 - Make sure OpacRenewalAllowed is set to Allow and "autorenewal" is not in PatronSelfModificationBorrowerUnwantedField 7 - Login on the opac at the patron 8 - Verify checkout shows as available for manual renewal 9 - Verify you can submit change request from OPAC to opt patron in to auto-renew 10 - Find change request on intranet, verify it shows change to autorenew opt-in, accept change request 11 - Reload patron summary on OPAC, verify it now shows checkout is scheduled for auto-renewal 12 - On staff side, opt patron out of auto-renew 13 - Run misc/cronjobs/automatic_renewals.pl, verify item is not renewed 14 - Manually renew item from the intranet 15 - Manually renew item from the OPAC 16 - Opt patron in for auto-renew 17 - Run misc/cronjobs/automatic_renewals.pl, verify item is renewed 18 - Set No Renewal Before value so item is no longer eligable for renewal 19 - Verify intranet and OPAC still show checkout scheduled for auto-renewal 20 - Opt patron out of auto-renew 21 - Verify intranet and OPAC show checkout ineligible for manual renewal due to No Renewal Before 22 - Opt patron in to auto-renew 22 - Add "autorenewal to PatronSelfModificationBorrowerUnwantedField and BorrowerUnwantedField 23 - Confirm patron can no longer be opted out of auto-renew on intranet or OPAC Created attachment 97997 [details] [review] Bug 24476: Database updates Signed-off-by: Andrew <andrew@bywatersolutions.com> Created attachment 97998 [details] [review] Bug 24476: API Updates Signed-off-by: Andrew <andrew@bywatersolutions.com> Created attachment 97999 [details] [review] Bug 24476: Change CanBookBeRenewed and adjust tests Signed-off-by: Andrew <andrew@bywatersolutions.com> Created attachment 98000 [details] [review] Bug 24476: Allow patrons to opt out of auto-renewal To test: 1 - Verify on staff side that patron can be edited to opt in our out of auto renewal 2 - Check out some items to a patron opted in to auto renewal 3 - Ensure the items are checked out and set to autorenew 4 - Login on the opac at the patron 5 - Verify items cannot be renewed as scheduled for auto-renewal 6 - On staff side, opt patron out of auto renewal 7 - Verify on opac items are no longer marked for auto renewal 8 - Run the auto renewal cron job, items are not renewed 9 - Set 'no renewal before' to a setting that would prevent renewal 10 - Verify that opting patron in or out of auto renewal changes only the reason items cannot be renewed 11 - Set 'no renewal before' to a setting that would allow for renewal 12 - Verify that opting patron in/out changes their ability to renew 13 - Verify that when opted out cron does not renew 14 - Verify that when opted in the item is auto renewed 15 - Reset the due date, opt out, verify manual renewal succeeds Signed-off-by: Andrew <andrew@bywatersolutions.com> Created attachment 98001 [details] [review] Bug 24476: DO NOT PUSH - Schema updates Signed-off-by: Andrew <andrew@bywatersolutions.com> Created attachment 98002 [details] [review] Bug 24476: Add boolean for all TINYINT Signed-off-by: Andrew <andrew@bywatersolutions.com> Does not apply: CONFLICT (content): Merge conflict in C4/Circulation.pm Any chances to get a more meaningful column's name (and so method). Koha::Patron->autorenewal could be related to renewing a registration. Created attachment 99793 [details] [review] Bug 24476: Database updates Signed-off-by: Andrew <andrew@bywatersolutions.com> Created attachment 99794 [details] [review] Bug 24476: API Updates Signed-off-by: Andrew <andrew@bywatersolutions.com> Created attachment 99795 [details] [review] Bug 24476: Change CanBookBeRenewed and adjust tests Signed-off-by: Andrew <andrew@bywatersolutions.com> Created attachment 99796 [details] [review] Bug 24476: Allow patrons to opt out of auto-renewal To test: 1 - Verify on staff side that patron can be edited to opt in our out of auto renewal 2 - Check out some items to a patron opted in to auto renewal 3 - Ensure the items are checked out and set to autorenew 4 - Login on the opac at the patron 5 - Verify items cannot be renewed as scheduled for auto-renewal 6 - On staff side, opt patron out of auto renewal 7 - Verify on opac items are no longer marked for auto renewal 8 - Run the auto renewal cron job, items are not renewed 9 - Set 'no renewal before' to a setting that would prevent renewal 10 - Verify that opting patron in or out of auto renewal changes only the reason items cannot be renewed 11 - Set 'no renewal before' to a setting that would allow for renewal 12 - Verify that opting patron in/out changes their ability to renew 13 - Verify that when opted out cron does not renew 14 - Verify that when opted in the item is auto renewed 15 - Reset the due date, opt out, verify manual renewal succeeds Signed-off-by: Andrew <andrew@bywatersolutions.com> Created attachment 99797 [details] [review] Bug 24476: Rename autorenewal to autorenew_checkouts Created attachment 99798 [details] [review] Bug 24476: DO NOT PUSH - Schema Updates Created attachment 99799 [details] [review] Bug 24476: Add boolean for TINYINT Created attachment 99800 [details] [review] Bug 24476: Allow direct editing of autorenew_checkouts by patron Created attachment 100022 [details] [review] Bug 24476: Database updates Signed-off-by: Andrew <andrew@bywatersolutions.com> Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com> Created attachment 100023 [details] [review] Bug 24476: API Updates Signed-off-by: Andrew <andrew@bywatersolutions.com> Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com> Created attachment 100024 [details] [review] Bug 24476: Change CanBookBeRenewed and adjust tests Signed-off-by: Andrew <andrew@bywatersolutions.com> Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com> Created attachment 100025 [details] [review] Bug 24476: Allow patrons to opt out of auto-renewal To test: 1 - Verify on staff side that patron can be edited to opt in our out of auto renewal 2 - Check out some items to a patron opted in to auto renewal 3 - Ensure the items are checked out and set to autorenew 4 - Login on the opac at the patron 5 - Verify items cannot be renewed as scheduled for auto-renewal 6 - On staff side, opt patron out of auto renewal 7 - Verify on opac items are no longer marked for auto renewal 8 - Run the auto renewal cron job, items are not renewed 9 - Set 'no renewal before' to a setting that would prevent renewal 10 - Verify that opting patron in or out of auto renewal changes only the reason items cannot be renewed 11 - Set 'no renewal before' to a setting that would allow for renewal 12 - Verify that opting patron in/out changes their ability to renew 13 - Verify that when opted out cron does not renew 14 - Verify that when opted in the item is auto renewed 15 - Reset the due date, opt out, verify manual renewal succeeds Signed-off-by: Andrew <andrew@bywatersolutions.com> Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com> Created attachment 100026 [details] [review] Bug 24476: Rename autorenewal to autorenew_checkouts Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com> Created attachment 100027 [details] [review] Bug 24476: DO NOT PUSH - Schema Updates Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com> Created attachment 100028 [details] [review] Bug 24476: Add boolean for TINYINT Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com> Created attachment 100029 [details] [review] Bug 24476: Allow direct editing of autorenew_checkouts by patron Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com> Created attachment 101385 [details] [review] Bug 24476 Standardize on 'auto renew(al)' and improve db update There were several versions: auto renew, auto-renew, autorenew I've decided to use auto renew (from automatic renewal) to keep things a bit more consistent. The DB update was missing AFTER and IGNORE for the sys pref. Also fixed a tiny typo (tp > to) Also changed I am stuck on this one :( The schema update patch would not apply, so I skipped it. Tried running DBIC: Koha::Schema::Result::Zebraqueue->set_primary_key("id"); DBIx::Class::ResultSource::column_info(): No such column autorenew_checkouts at (eval 1297) line 1656 Tried running the updatedatabase script (with and without my patch): $ updatedatabase {UNKNOWN}: DBIx::Class::ResultSource::column_info(): No such column autorenew_checkouts at /home/vagrant/kohaclone/Koha/Schema/Result/Borrower.pm line 1656 Compilation failed in require at /usr/share/perl5/Class/C3/Componentised.pm line 150. at /usr/share/perl5/Class/C3/Componentised.pm line 155 Compilation failed in require at /home/vagrant/kohaclone/Koha/Database.pm line 50. Can you please check and rabase? Also added a follo-up, please have a look. Created attachment 101419 [details] [review] Bug 24476: Database updates Signed-off-by: Andrew <andrew@bywatersolutions.com> Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com> Created attachment 101420 [details] [review] Bug 24476: API Updates Signed-off-by: Andrew <andrew@bywatersolutions.com> Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com> Created attachment 101421 [details] [review] Bug 24476: Change CanBookBeRenewed and adjust tests Signed-off-by: Andrew <andrew@bywatersolutions.com> Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com> Created attachment 101422 [details] [review] Bug 24476: Allow patrons to opt out of auto-renewal To test: 1 - Verify on staff side that patron can be edited to opt in our out of auto renewal 2 - Check out some items to a patron opted in to auto renewal 3 - Ensure the items are checked out and set to autorenew 4 - Login on the opac at the patron 5 - Verify items cannot be renewed as scheduled for auto-renewal 6 - On staff side, opt patron out of auto renewal 7 - Verify on opac items are no longer marked for auto renewal 8 - Run the auto renewal cron job, items are not renewed 9 - Set 'no renewal before' to a setting that would prevent renewal 10 - Verify that opting patron in or out of auto renewal changes only the reason items cannot be renewed 11 - Set 'no renewal before' to a setting that would allow for renewal 12 - Verify that opting patron in/out changes their ability to renew 13 - Verify that when opted out cron does not renew 14 - Verify that when opted in the item is auto renewed 15 - Reset the due date, opt out, verify manual renewal succeeds Signed-off-by: Andrew <andrew@bywatersolutions.com> Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com> Created attachment 101423 [details] [review] Bug 24476: Rename autorenewal to autorenew_checkouts Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com> Created attachment 101424 [details] [review] Bug 24476: Allow direct editing of autorenew_checkouts by patron Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com> Created attachment 101425 [details] [review] Bug 24476: (QA follow-up) Standardize on 'auto renew(al)' and improve db update There were several versions: auto renew, auto-renew, autorenew I've decided to use auto renew (from automatic renewal) to keep things a bit more consistent. The DB update was missing AFTER and IGNORE for the sys pref. Also fixed a tiny typo (tp > to) Also changed Created attachment 101426 [details] [review] Bug 24476: DO NOT PUSH - Schema changes Created attachment 101427 [details] [review] Bug 24476: Add boolean for TINYINT Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com> (In reply to Katrin Fischer from comment #50) > I am stuck on this one :( > > The schema update patch would not apply, so I skipped it. > Tried running DBIC: The patch after DBIC relies on that one, I rebased them - I left as separate patch as reminder to RM > Tried running the updatedatabase script (with and without my patch): Above should fix that > Can you please check and rabase? Also added a follo-up, please have a look. Follow-up is good, tests pass so things appear to work Created attachment 101511 [details]
Auto-renew setting in the OPAC
I think the placement of the new option in the OPAC is not ideal, it appears in a very prominent spot, even atop of our warnings and other messages. I'd suggest to move it lower, even below the checkouts table maybe, but below the warnings.
It will also appear for all patron categories independent of if they are set up to have auto-renewals or not. That's out of the scope of this bug - adding for documentation.
Created attachment 101512 [details] [review] Bug 24476: Database updates Signed-off-by: Andrew <andrew@bywatersolutions.com> Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com> Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de> Created attachment 101513 [details] [review] Bug 24476: API Updates Signed-off-by: Andrew <andrew@bywatersolutions.com> Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com> Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de> Created attachment 101514 [details] [review] Bug 24476: Change CanBookBeRenewed and adjust tests Signed-off-by: Andrew <andrew@bywatersolutions.com> Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com> Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de> Created attachment 101515 [details] [review] Bug 24476: Allow patrons to opt out of auto-renewal To test: 1 - Verify on staff side that patron can be edited to opt in our out of auto renewal 2 - Check out some items to a patron opted in to auto renewal 3 - Ensure the items are checked out and set to autorenew 4 - Login on the opac at the patron 5 - Verify items cannot be renewed as scheduled for auto-renewal 6 - On staff side, opt patron out of auto renewal 7 - Verify on opac items are no longer marked for auto renewal 8 - Run the auto renewal cron job, items are not renewed 9 - Set 'no renewal before' to a setting that would prevent renewal 10 - Verify that opting patron in or out of auto renewal changes only the reason items cannot be renewed 11 - Set 'no renewal before' to a setting that would allow for renewal 12 - Verify that opting patron in/out changes their ability to renew 13 - Verify that when opted out cron does not renew 14 - Verify that when opted in the item is auto renewed 15 - Reset the due date, opt out, verify manual renewal succeeds Signed-off-by: Andrew <andrew@bywatersolutions.com> Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com> Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de> Created attachment 101516 [details] [review] Bug 24476: Rename autorenewal to autorenew_checkouts Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com> Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de> Created attachment 101517 [details] [review] Bug 24476: Allow direct editing of autorenew_checkouts by patron Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com> Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de> Created attachment 101518 [details] [review] Bug 24476: (QA follow-up) Standardize on 'auto renew(al)' and improve db update There were several versions: auto renew, auto-renew, autorenew I've decided to use auto renew (from automatic renewal) to keep things a bit more consistent. The DB update was missing AFTER and IGNORE for the sys pref. Also fixed a tiny typo (tp > to) Also changed Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de> Created attachment 101519 [details] [review] Bug 24476: DO NOT PUSH - Schema changes Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de> Created attachment 101520 [details] [review] Bug 24476: Add boolean for TINYINT Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com> Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de> Created attachment 101521 [details] [review] Bug 24476: (QA follow-up) Move new option below warnings and messages in OPAC account This makes it so the new option appears right above the table of checkouts etc. but below any warnings, messages or dialogs. Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de> (In reply to Katrin Fischer from comment #61) > Created attachment 101511 [details] > Auto-renew setting in the OPAC > > I think the placement of the new option in the OPAC is not ideal, it appears > in a very prominent spot, even atop of our warnings and other messages. I'd > suggest to move it lower, even below the checkouts table maybe, but below > the warnings. > > It will also appear for all patron categories independent of if they are set > up to have auto-renewals or not. That's out of the scope of this bug - > adding for documentation. Added a follow up to move it lower (right above the table still) - RM/Nick, please check! Created attachment 101542 [details] [review] Bug 24476: (QA follow-up) Move new option below warnings and messages in OPAC account This makes it so the new option appears right above the table of checkouts etc. but below any warnings, messages or dialogs. Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de> Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Nice work everyone! Pushed to master for 20.05 enhancement not backported to 19.11.x See also bug 24988. But if a patron has no autorenewal, he should not even have this choice. It would be useless and confusing. (In reply to Marcel de Rooy from comment #76) > See also bug 24988. But if a patron has no autorenewal, he should not even > have this choice. It would be useless and confusing. It could be confusing for patrons to view and update an autorenewal setting if no autorenewals are allowed, but I'm not sure that there is a simple reasonable alternative. On one hand, you could calculate the circulation rules to determine if a patron is allowed to have autorenewals. On the other hand, that wouldn't take into account their existing autorenewal checkouts *plus* it's possible to set autorenewals at checkout time. So determining if an item is autorenewable and whether a patron allows autorenewals seems to me like 2 completely separate things in the end. |