Bug 30642

Summary: We should record the renewal type (automatic/manual)
Product: Koha Reporter: Martin Renvoize <martin.renvoize>
Component: CirculationAssignee: Matt Blenkinsop <matt.blenkinsop>
Status: CLOSED FIXED QA Contact: Kyle M Hall <kyle>
Severity: enhancement    
Priority: P5 - low CC: aude.charillon, caroline.cyr-la-rose, gmcharlt, kyle.m.hall, kyle, lucas, matt.blenkinsop, tomascohen
Version: master   
Hardware: All   
OS: All   
Change sponsored?: --- Patch complexity: ---
Documentation contact: Aude Charillon Documentation submission: https://gitlab.com/koha-community/koha-manual/-/merge_requests/684
Text to go in the release notes:
**Sponsored by** *PTFS Europe* This ensures that the type of a renewal, automatic or manual, is stored in the new column `renewal_type` of the `checkout_renewals` table.
Version(s) released in:
23.05.00,22.11.03
Bug Depends on: 30275    
Bug Blocks:    
Attachments: Bug 30642: Record renewal type
Bug 30642: Record renewal type
Bug 30642: Record renewal type
Bug 30642: (follow-up) Change sql and adapt for translations
Bug 30642: Record renewal type
Bug 30642: (follow-up) Change sql and adapt for translations
Bug 30642: Record renewal type
Bug 30642: (follow-up) Change sql and adapt for translations
Bug 30642: (QA follow-up) Do not rely on script names in modules, add unit test
Bug 30642: Record renewal type
Bug 30642: (follow-up) Change sql and adapt for translations
Bug 30642: (QA follow-up) Do not rely on script names in modules, add unit test
Bug 30642: Make renewal_type an enum in spec and add test
Bug 30642: Make renewal_type an enum in spec and add test
Bug 30642: (follow-up) Remove use of deleted file

Description Martin Renvoize 2022-04-28 12:23:46 UTC
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.
Comment 1 Matt Blenkinsop 2022-12-06 16:55:50 UTC
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"
Comment 2 Matt Blenkinsop 2022-12-06 17:19:18 UTC
Ignore patch above
Comment 3 Matt Blenkinsop 2022-12-06 18:18:06 UTC
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.
Comment 4 Lucas Gass 2022-12-06 18:50:57 UTC
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";
Comment 5 Matt Blenkinsop 2022-12-07 09:39:48 UTC
Thanks for the correction! I will update and recommit the patch :)
Comment 6 Matt Blenkinsop 2022-12-07 09:40:39 UTC
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 7 Martin Renvoize 2022-12-07 09:42:55 UTC
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.
Comment 8 Katrin Fischer 2022-12-07 20:21:36 UTC
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!
Comment 9 Matt Blenkinsop 2022-12-08 14:33:34 UTC
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.
Comment 10 Lucas Gass 2022-12-08 16:25:50 UTC
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>
Comment 11 Lucas Gass 2022-12-08 16:25:54 UTC
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>
Comment 12 Kyle M Hall 2023-01-20 14:58:58 UTC
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>
Comment 13 Kyle M Hall 2023-01-20 14:59:09 UTC
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>
Comment 14 Kyle M Hall 2023-01-20 14:59:14 UTC
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>
Comment 15 Tomás Cohen Arazi 2023-02-03 13:29:38 UTC
What y'all think about moving the column into an ENUM type? I can do it, and willing to push this ASAP.
Comment 16 Martin Renvoize 2023-02-03 14:02:55 UTC
No issue at all with it being an ENUM.. makes sense to me :)
Comment 17 Matt Blenkinsop 2023-02-06 10:44:07 UTC
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
Comment 18 Tomás Cohen Arazi 2023-02-06 10:56:23 UTC
(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.
Comment 19 Matt Blenkinsop 2023-02-06 10:59:29 UTC
Ah yes I see what you mean, missed that first time round!
Comment 20 Tomás Cohen Arazi 2023-02-10 12:43:26 UTC
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>
Comment 21 Tomás Cohen Arazi 2023-02-10 12:43:29 UTC
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>
Comment 22 Tomás Cohen Arazi 2023-02-10 12:43:33 UTC
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
Comment 23 Tomás Cohen Arazi 2023-02-10 12:43:36 UTC
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>
Comment 24 Tomás Cohen Arazi 2023-02-10 13:58:08 UTC
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>
Comment 25 Tomás Cohen Arazi 2023-02-10 14:08:24 UTC
Pushed to master for 23.05.

Nice work everyone, thanks!
Comment 26 Jacob O'Mara 2023-02-13 09:45:52 UTC
Nice work, thanks everyone!

Pushed to 22.11.x for the next release.
Comment 27 Tomás Cohen Arazi 2023-02-14 13:28:41 UTC
Created attachment 146614 [details] [review]
Bug 30642: (follow-up) Remove use of deleted file

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 28 Tomás Cohen Arazi 2023-02-14 13:29:12 UTC
Follow-up required in stable.
Comment 29 Matt Blenkinsop 2023-02-14 15:25:46 UTC
Nice work everyone!

Pushed to stable for 22.11.x
Comment 30 Lucas Gass 2023-02-15 22:12:21 UTC
Enhancement will not be backported to 22.05.x
Comment 31 Aude Charillon 2023-02-24 11:26:32 UTC
Reopening for documentation (agreed to be thorough and replace screenshot)