With bug 30275 we introduce more logging of renewals.. but we don't differentiate between automatic renewals and renewals by other means.. it could be helpful to do so.
Created attachment 144451 [details] [review] Bug 30642: Record renewal type A requirement has been requested to record whether a renewal was done manually or automatically. A column has been added to the checkout_renewals table in the database to record this and a check is now in place to determine whether the renewal was manual or automatic. Test plan: 1) In the database shell run "show columns from checkout_renewals;" and observe that there is currently no column for recording the type of renewal 2) Apply patch 3) In the shell run "dbic" and "perl installer/data/mysql/updatedatabase.pl" to update the database schema with the new column. 4) Create some checkouts 5) Renew some checkouts manually and observe in the database that there is now a column called "renewal_type" that will have recorded these as "Manual" 6) Create some checkouts that can be automatically renewed 7) Run the cron script in automatic_renewals.pl and observe that there are now also entries with a renewal_type of "Automatic"
Ignore patch above
Created attachment 144457 [details] [review] Bug 30642: Record renewal type A requirement has been requested to record whether a renewal was done manually or automatically. A column has been added to the checkout_renewals table in the database to record this and a check is now in place to determine whether the renewal was manual or automatic. The API has also been updated to reflect this new column and return the data when requested. The renewals modal view has also been updated to show what type the renewal was. Test plan: 1) In the database shell run "show columns from checkout_renewals;" and observe that there is currently no column for recording the type of renewal 2) Apply patch 3) In the shell run "dbic" and "perl installer/data/mysql/updatedatabase.pl" to update the database schema with the new column. 4) Create some checkouts 5) Renew some checkouts manually and observe in the database that there is now a column called "renewal_type" that will have recorded these as "Manual" 6) Create some checkouts that can be automatically renewed 7) Run the cron script in automatic_renewals.pl and observe that there are now also entries with a renewal_type of "Automatic" 8) Send a GET request to http://localhost:8081/api/v1/checkouts/1/renewals and observe that the renewal_type is now returned in the response 9) In the Item Details tab for a record, there is the "Current renewals" option which has a button to view renewals. Click on this and observe that the modal now displays the new information.
This will be very helpful! I do encounter a problem when testing with automatic_renewals.pl. They are showing up as a "Manual" renewal type. Looking a Circulation.pm I see that my file name is "/kohadevbox/koha/misc/cronjobs/automatic_renewals.pl" not "automatic_renewals.pl". If I switch this it works: -my $renewal_type = $filename eq "automatic_renewals.pl" ? "Automatic" : "Manual"; +my $renewal_type = $filename =~ m/automatic_renewals.pl/ ? "Automatic" : "Manual";
Thanks for the correction! I will update and recommit the patch :)
Created attachment 144463 [details] [review] Bug 30642: Record renewal type A requirement has been requested to record whether a renewal was done manually or automatically. A column has been added to the checkout_renewals table in the database to record this and a check is now in place to determine whether the renewal was manual or automatic. The API has also been updated to reflect this new column and return the data when requested. The renewals modal view has also been updated to show what type the renewal was. Test plan: 1) In the database shell run "show columns from checkout_renewals;" and observe that there is currently no column for recording the type of renewal 2) Apply patch 3) In the shell run "dbic" and "perl installer/data/mysql/updatedatabase.pl" to update the database schema with the new column. 4) Create some checkouts 5) Renew some checkouts manually and observe in the database that there is now a column called "renewal_type" that will have recorded these as "Manual" 6) Create some checkouts that can be automatically renewed 7) Run the cron script in automatic_renewals.pl and observe that there are now also entries with a renewal_type of "Automatic" 8) Send a GET request to http://localhost:8081/api/v1/checkouts/1/renewals and observe that the renewal_type is now returned in the response 9) In the Item Details tab for a record, there is the "Current renewals" option which has a button to view renewals. Click on this and observe that the modal now displays the new information.
Comment on attachment 144457 [details] [review] Bug 30642: Record renewal type Review of attachment 144457 [details] [review]: ----------------------------------------------------------------- This is a great submission Matt. I agree with Lucas's comment, we'll need to use a regex to catch the file as the install process can put the file into different locations depending on the install type... but otherwise the code is working well. A few minor code review comments added too. ::: installer/data/mysql/atomicupdate/bug_30642-add_renewal_type.pl @@ +1,4 @@ > +use Modern::Perl; > + > +return { > + bug_number => "BUG_30642", You only need the number here Matt ;P @@ +8,5 @@ > + my ($dbh, $out) = @$args{qw(dbh out)}; > + > + if( !column_exists( 'checkout_renewals', 'renewal_type' ) ) { > + $dbh->do(q{ > + ALTER TABLE checkout_renewals ADD COLUMN `renewal_type` varchar(9) NOT NULL AFTER `timestamp` Thinking about it, this could be an ENUM rather than a varchar(9) perhaps? Also, as we're adding a 'NOT NULL' we probably need to deal with existing rows.. Especially as we've not added a DEFAULT.. perhaps 'DEFAULT 'MANUAL' and ENUM('AUTO','MANUAL'). ::: koha-tmpl/intranet-tmpl/prog/js/checkout_renewals_modal.js @@ +20,4 @@ > }); > }); > function createLi(renewal) { > + return '<li><span style="font-weight:bold">' + $datetime(renewal.timestamp) + '</span> ' + renewed + ' <span style="font-weight:bold">' + $patron_to_html(renewal.renewer) + '</span>' + renewed_type + ' <span style="font-weight:bold">' + renewal.renewal_type + '</span></li>'; This is interesting, I hadn't really considered it before today. I imagine 'auto' renewals don't have a renewal.renewer associated.. worth a little manual testing to see what happens there. Finally.. translations.. we'll need to think a little about this. With the ENUM change suggested above we could template out the descriptive string and use __('Manual') to expose the display strings to the translation system.
Hi, just looking here for a moment - it seems like the patch might have been attached twice? Changing to failed QA for Martin's last comment. Please keep going!
Created attachment 144499 [details] [review] Bug 30642: (follow-up) Change sql and adapt for translations New column has now been changed to an enum in line with comments and the strings have been amended to be picked up for translation. The file koha-tmpl/intranet-tmpl/prog/en/includes/str/checkout_renewals.inc has been removed as the variables can be included within the javascript file. Test plan: 1) In the database shell run "show columns from checkout_renewals;" and observe that there is currently no column for recording the type of renewal 2) Apply patch 3) In the shell run "dbic" and "perl installer/data/mysql/updatedatabase.pl" to update the database schema with the new column. 4) Create some checkouts 5) Renew some checkouts manually and observe in the database that there is now a column called "renewal_type" that will have recorded these as "Manual" 6) Create some checkouts that can be automatically renewed 7) Run the cron script in automatic_renewals.pl and observe that there are now also entries with a renewal_type of "Automatic" 8) Send a GET request to http://localhost:8081/api/v1/checkouts/1/renewals and observe that the renewal_type is now returned in the response 9) In the Item Details tab for a record, there is the "Current renewals" option which has a button to view renewals. Click on this and observe that the modal now displays the new information.
Created attachment 144504 [details] [review] Bug 30642: Record renewal type A requirement has been requested to record whether a renewal was done manually or automatically. A column has been added to the checkout_renewals table in the database to record this and a check is now in place to determine whether the renewal was manual or automatic. The API has also been updated to reflect this new column and return the data when requested. The renewals modal view has also been updated to show what type the renewal was. Test plan: 1) In the database shell run "show columns from checkout_renewals;" and observe that there is currently no column for recording the type of renewal 2) Apply patch 3) In the shell run "dbic" and "perl installer/data/mysql/updatedatabase.pl" to update the database schema with the new column. 4) Create some checkouts 5) Renew some checkouts manually and observe in the database that there is now a column called "renewal_type" that will have recorded these as "Manual" 6) Create some checkouts that can be automatically renewed 7) Run the cron script in automatic_renewals.pl and observe that there are now also entries with a renewal_type of "Automatic" 8) Send a GET request to http://localhost:8081/api/v1/checkouts/1/renewals and observe that the renewal_type is now returned in the response 9) In the Item Details tab for a record, there is the "Current renewals" option which has a button to view renewals. Click on this and observe that the modal now displays the new information. Signed-off-by: Lucas Gass <lucas@bywatersolutions.com>
Created attachment 144505 [details] [review] Bug 30642: (follow-up) Change sql and adapt for translations New column has now been changed to an enum in line with comments and the strings have been amended to be picked up for translation. The file koha-tmpl/intranet-tmpl/prog/en/includes/str/checkout_renewals.inc has been removed as the variables can be included within the javascript file. Test plan: 1) In the database shell run "show columns from checkout_renewals;" and observe that there is currently no column for recording the type of renewal 2) Apply patch 3) In the shell run "dbic" and "perl installer/data/mysql/updatedatabase.pl" to update the database schema with the new column. 4) Create some checkouts 5) Renew some checkouts manually and observe in the database that there is now a column called "renewal_type" that will have recorded these as "Manual" 6) Create some checkouts that can be automatically renewed 7) Run the cron script in automatic_renewals.pl and observe that there are now also entries with a renewal_type of "Automatic" 8) Send a GET request to http://localhost:8081/api/v1/checkouts/1/renewals and observe that the renewal_type is now returned in the response 9) In the Item Details tab for a record, there is the "Current renewals" option which has a button to view renewals. Click on this and observe that the modal now displays the new information. Signed-off-by: Lucas Gass <lucas@bywatersolutions.com>
Created attachment 145518 [details] [review] Bug 30642: Record renewal type A requirement has been requested to record whether a renewal was done manually or automatically. A column has been added to the checkout_renewals table in the database to record this and a check is now in place to determine whether the renewal was manual or automatic. The API has also been updated to reflect this new column and return the data when requested. The renewals modal view has also been updated to show what type the renewal was. Test plan: 1) In the database shell run "show columns from checkout_renewals;" and observe that there is currently no column for recording the type of renewal 2) Apply patch 3) In the shell run "dbic" and "perl installer/data/mysql/updatedatabase.pl" to update the database schema with the new column. 4) Create some checkouts 5) Renew some checkouts manually and observe in the database that there is now a column called "renewal_type" that will have recorded these as "Manual" 6) Create some checkouts that can be automatically renewed 7) Run the cron script in automatic_renewals.pl and observe that there are now also entries with a renewal_type of "Automatic" 8) Send a GET request to http://localhost:8081/api/v1/checkouts/1/renewals and observe that the renewal_type is now returned in the response 9) In the Item Details tab for a record, there is the "Current renewals" option which has a button to view renewals. Click on this and observe that the modal now displays the new information. Signed-off-by: Lucas Gass <lucas@bywatersolutions.com> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Created attachment 145519 [details] [review] Bug 30642: (follow-up) Change sql and adapt for translations New column has now been changed to an enum in line with comments and the strings have been amended to be picked up for translation. The file koha-tmpl/intranet-tmpl/prog/en/includes/str/checkout_renewals.inc has been removed as the variables can be included within the javascript file. Test plan: 1) In the database shell run "show columns from checkout_renewals;" and observe that there is currently no column for recording the type of renewal 2) Apply patch 3) In the shell run "dbic" and "perl installer/data/mysql/updatedatabase.pl" to update the database schema with the new column. 4) Create some checkouts 5) Renew some checkouts manually and observe in the database that there is now a column called "renewal_type" that will have recorded these as "Manual" 6) Create some checkouts that can be automatically renewed 7) Run the cron script in automatic_renewals.pl and observe that there are now also entries with a renewal_type of "Automatic" 8) Send a GET request to http://localhost:8081/api/v1/checkouts/1/renewals and observe that the renewal_type is now returned in the response 9) In the Item Details tab for a record, there is the "Current renewals" option which has a button to view renewals. Click on this and observe that the modal now displays the new information. Signed-off-by: Lucas Gass <lucas@bywatersolutions.com> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Created attachment 145520 [details] [review] Bug 30642: (QA follow-up) Do not rely on script names in modules, add unit test Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
What y'all think about moving the column into an ENUM type? I can do it, and willing to push this ASAP.
No issue at all with it being an ENUM.. makes sense to me :)
Is it not already an ENUM? Martin raised it in his feedback and it was changed in the "(follow-up) Change sql and adapt for translations" patch
(In reply to Matt Blenkinsop from comment #17) > Is it not already an ENUM? Martin raised it in his feedback and it was > changed in the "(follow-up) Change sql and adapt for translations" patch It is and ENUM on the DB. What's missing is reflecting that on the API. It is currently defined as a nullable string.
Ah yes I see what you mean, missed that first time round!
Created attachment 146471 [details] [review] Bug 30642: Record renewal type A requirement has been requested to record whether a renewal was done manually or automatically. A column has been added to the checkout_renewals table in the database to record this and a check is now in place to determine whether the renewal was manual or automatic. The API has also been updated to reflect this new column and return the data when requested. The renewals modal view has also been updated to show what type the renewal was. Test plan: 1) In the database shell run "show columns from checkout_renewals;" and observe that there is currently no column for recording the type of renewal 2) Apply patch 3) In the shell run "dbic" and "perl installer/data/mysql/updatedatabase.pl" to update the database schema with the new column. 4) Create some checkouts 5) Renew some checkouts manually and observe in the database that there is now a column called "renewal_type" that will have recorded these as "Manual" 6) Create some checkouts that can be automatically renewed 7) Run the cron script in automatic_renewals.pl and observe that there are now also entries with a renewal_type of "Automatic" 8) Send a GET request to http://localhost:8081/api/v1/checkouts/1/renewals and observe that the renewal_type is now returned in the response 9) In the Item Details tab for a record, there is the "Current renewals" option which has a button to view renewals. Click on this and observe that the modal now displays the new information. Signed-off-by: Lucas Gass <lucas@bywatersolutions.com> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Created attachment 146472 [details] [review] Bug 30642: (follow-up) Change sql and adapt for translations New column has now been changed to an enum in line with comments and the strings have been amended to be picked up for translation. The file koha-tmpl/intranet-tmpl/prog/en/includes/str/checkout_renewals.inc has been removed as the variables can be included within the javascript file. Test plan: 1) In the database shell run "show columns from checkout_renewals;" and observe that there is currently no column for recording the type of renewal 2) Apply patch 3) In the shell run "dbic" and "perl installer/data/mysql/updatedatabase.pl" to update the database schema with the new column. 4) Create some checkouts 5) Renew some checkouts manually and observe in the database that there is now a column called "renewal_type" that will have recorded these as "Manual" 6) Create some checkouts that can be automatically renewed 7) Run the cron script in automatic_renewals.pl and observe that there are now also entries with a renewal_type of "Automatic" 8) Send a GET request to http://localhost:8081/api/v1/checkouts/1/renewals and observe that the renewal_type is now returned in the response 9) In the Item Details tab for a record, there is the "Current renewals" option which has a button to view renewals. Click on this and observe that the modal now displays the new information. Signed-off-by: Lucas Gass <lucas@bywatersolutions.com> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Created attachment 146473 [details] [review] Bug 30642: (QA follow-up) Do not rely on script names in modules, add unit test Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Edit: Kyle, stop impersonating John Doe
Created attachment 146474 [details] [review] Bug 30642: Make renewal_type an enum in spec and add test This patch makes the renewal_type an enum, to match the change on the DB. A test is added to account the fact the API is always setting 'Manual' request type. Bonus: small portion of code gets a tidy, should've been asked by QA. Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Created attachment 146476 [details] [review] Bug 30642: Make renewal_type an enum in spec and add test This patch makes the renewal_type an enum, to match the change on the DB. A test is added to account the fact the API is always setting 'Manual' request type. Bonus: small portion of code gets a tidy, should've been asked by QA. Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Pushed to master for 23.05. Nice work everyone, thanks!
Nice work, thanks everyone! Pushed to 22.11.x for the next release.
Created attachment 146614 [details] [review] Bug 30642: (follow-up) Remove use of deleted file Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Follow-up required in stable.
Nice work everyone! Pushed to stable for 22.11.x
Enhancement will not be backported to 22.05.x
Reopening for documentation (agreed to be thorough and replace screenshot)