Bug 32013 - Autorenewals is effectively a bulk action and should be treated as such
Summary: Autorenewals is effectively a bulk action and should be treated as such
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Architecture, internals, and plumbing (show other bugs)
Version: Main
Hardware: All All
: P5 - low enhancement (vote)
Assignee: Matt Blenkinsop
QA Contact: Nick Clemens
URL:
Keywords:
Depends on:
Blocks: 33444
  Show dependency treegraph
 
Reported: 2022-10-27 10:26 UTC by Martin Renvoize
Modified: 2023-12-28 20:42 UTC (History)
5 users (show)

See Also:
Change sponsored?: ---
Patch complexity: ---
Documentation contact:
Documentation submission:
Text to go in the release notes:
**Sponsored by** *PTFS Europe*
Version(s) released in:
23.05.00


Attachments
Bug 32013: Autorenewal batch indexing (4.27 KB, patch)
2022-12-06 10:58 UTC, Matt Blenkinsop
Details | Diff | Splinter Review
Bug 32013: (follow-up) Add unit test (3.26 KB, patch)
2022-12-07 11:15 UTC, Matt Blenkinsop
Details | Diff | Splinter Review
Bug 32013: Autorenewal batch indexing (4.30 KB, patch)
2023-01-09 15:44 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 32013: (follow-up) Add unit test (3.29 KB, patch)
2023-01-09 15:44 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 32013: (QA follow-up) Move mock to avoid warns from sample data creation (1.73 KB, patch)
2023-01-09 15:44 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 32013: Autorenewal batch indexing (4.37 KB, patch)
2023-01-10 14:12 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 32013: (follow-up) Add unit test (3.36 KB, patch)
2023-01-10 14:12 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 32013: (QA follow-up) Move mock to avoid warns from sample data creation (1.80 KB, patch)
2023-01-10 14:12 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 32013: Autorenewal batch indexing (4.67 KB, patch)
2023-04-07 13:41 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 32013: (follow-up) Add unit test (3.35 KB, patch)
2023-04-07 13:41 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 32013: (QA follow-up) Move mock to avoid warns from sample data creation (1.80 KB, patch)
2023-04-07 13:41 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 32013: Adjust unit tests (1.32 KB, patch)
2023-04-07 13:41 UTC, Nick Clemens
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Martin Renvoize 2022-10-27 10:26:58 UTC
The autorenewals cron script is in effect doing a bulk renew action.. but right now it iterates through all renewals and calls 'AddRenewal' on each item individually..

This can cause things, like elastic indexing, to queue up but without actioning it via the skip_index option.

We aught to be able to pass skip_indexing to AddRenewal to allow us to action the indexing step at the end as a bulk change for the ES case.
Comment 1 Martin Renvoize 2022-10-27 10:37:42 UTC
As a temporary fix I've replaced the throw with a warn in Koha/SearchEngine/Elasticsearch/Indexer.pm so that we fail more gracefully rather than dieing and killing the autorenewals script part way through.

This will of course mean the item in question isn't properly indexed.. but at least the rest of the process will run to completion.
Comment 2 Matt Blenkinsop 2022-12-06 10:58:02 UTC
Created attachment 144442 [details] [review]
Bug 32013: Autorenewal batch indexing

The automatic_renewals.pl cron script currently loops through items for automatic renewal and calls the indexer for each one individually. skip_record_index has now been added as a parameter to the AddRenewal function to skip the indexing process. The item numbers are now added to an array and then the indexer is called once from within automatic_renewals.pl and passed the array to queue one indexing job instead of multiple jobs.

Test plan:
1) AddRenewal uses Koha::Items->store() to trigger the indexing process. Run prove -vv t/db_dependent/Koha/SearchEngine/Indexer.t and check tests 5,6,29,30. These tests prove whether passing skip_record_index to store() triggers or skips the indexing process. All four tests should pass to show that skip_index_records can prevent the indexing being triggered.
2) Add multiple renewals that are able to be autorenewed and run the automatic_renewals.pl script. There should be multiple items queued in zebraqueue.
3) Apply patch and try again
4) There should now only be one job queued in zebraqueue
Comment 3 Matt Blenkinsop 2022-12-07 11:15:01 UTC
Created attachment 144464 [details] [review]
Bug 32013: (follow-up) Add unit test

A unit test has been added to show the pass-through of skip_record_index from AddRenewal() to the Item->store() method.

Test plan:
1) In the shell run prove -vv t/db_dependent/Koha/SearchEngine/Indexer.t
2) Observe Subtest 3: Test AddRenewal indexer call
3) All four tests should pass
Comment 4 Nick Clemens 2023-01-09 15:44:50 UTC
Created attachment 145150 [details] [review]
Bug 32013: Autorenewal batch indexing

The automatic_renewals.pl cron script currently loops through items for automatic renewal and calls the indexer for each one individually. skip_record_index has now been added as a parameter to the AddRenewal function to skip the indexing process. The item numbers are now added to an array and then the indexer is called once from within automatic_renewals.pl and passed the array to queue one indexing job instead of multiple jobs.

Test plan:
1) AddRenewal uses Koha::Items->store() to trigger the indexing process. Run prove -vv t/db_dependent/Koha/SearchEngine/Indexer.t and check tests 5,6,29,30. These tests prove whether passing skip_record_index to store() triggers or skips the indexing process. All four tests should pass to show that skip_index_records can prevent the indexing being triggered.
2) Add multiple renewals that are able to be autorenewed and run the automatic_renewals.pl script. There should be multiple items queued in zebraqueue.
3) Apply patch and try again
4) There should now only be one job queued in zebraqueue

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 5 Nick Clemens 2023-01-09 15:44:54 UTC
Created attachment 145151 [details] [review]
Bug 32013: (follow-up) Add unit test

A unit test has been added to show the pass-through of skip_record_index from AddRenewal() to the Item->store() method.

Test plan:
1) In the shell run prove -vv t/db_dependent/Koha/SearchEngine/Indexer.t
2) Observe Subtest 3: Test AddRenewal indexer call
3) All four tests should pass

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 6 Nick Clemens 2023-01-09 15:44:59 UTC
Created attachment 145152 [details] [review]
Bug 32013: (QA follow-up) Move mock to avoid warns from sample data creation

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 7 Martin Renvoize 2023-01-10 14:12:27 UTC
Created attachment 145186 [details] [review]
Bug 32013: Autorenewal batch indexing

The automatic_renewals.pl cron script currently loops through items for automatic renewal and calls the indexer for each one individually. skip_record_index has now been added as a parameter to the AddRenewal function to skip the indexing process. The item numbers are now added to an array and then the indexer is called once from within automatic_renewals.pl and passed the array to queue one indexing job instead of multiple jobs.

Test plan:
1) AddRenewal uses Koha::Items->store() to trigger the indexing process. Run prove -vv t/db_dependent/Koha/SearchEngine/Indexer.t and check tests 5,6,29,30. These tests prove whether passing skip_record_index to store() triggers or skips the indexing process. All four tests should pass to show that skip_index_records can prevent the indexing being triggered.
2) Add multiple renewals that are able to be autorenewed and run the automatic_renewals.pl script. There should be multiple items queued in zebraqueue.
3) Apply patch and try again
4) There should now only be one job queued in zebraqueue

Mentored-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 8 Martin Renvoize 2023-01-10 14:12:32 UTC
Created attachment 145187 [details] [review]
Bug 32013: (follow-up) Add unit test

A unit test has been added to show the pass-through of skip_record_index from AddRenewal() to the Item->store() method.

Test plan:
1) In the shell run prove -vv t/db_dependent/Koha/SearchEngine/Indexer.t
2) Observe Subtest 3: Test AddRenewal indexer call
3) All four tests should pass

Mentored-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 9 Martin Renvoize 2023-01-10 14:12:36 UTC
Created attachment 145188 [details] [review]
Bug 32013: (QA follow-up) Move mock to avoid warns from sample data creation

Mentored-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 10 Martin Renvoize 2023-01-10 14:13:44 UTC
Added my mentoring lines whilst doing a double check of the code.. I'm really happy with this improvement, thanks Matt.

Thanks Nick for the follow-up and review.
Comment 11 Nick Clemens 2023-01-10 17:50:07 UTC
Minor patch, moving to PQA
Comment 12 Tomás Cohen Arazi 2023-04-06 13:00:02 UTC
Please rebase ASAP.
Comment 13 Matt Blenkinsop 2023-04-06 13:19:25 UTC
Looks like a conflict with bug 33309 which is avoiding a race condition - this would still call the indexer on each call to AddRenewal rather than skipping the individual indexing jobs and running one indexing job on the collected biblio ids. 

I don't think I can rebase in a way that doesn't involve deleting the intent of bug 33309 - looks like Nick wrote that bug and should be on the thread for this as he signed it off. 

Nick - Does this patch cover what you were trying to achieve with 33309 or do we need to combine them in some way?
Comment 14 Nick Clemens 2023-04-07 13:41:01 UTC
Created attachment 149276 [details] [review]
Bug 32013: Autorenewal batch indexing

The automatic_renewals.pl cron script currently loops through items for automatic renewal and calls the indexer for each one individually. skip_record_index has now been added as a parameter to the AddRenewal function to skip the indexing process. The item numbers are now added to an array and then the indexer is called once from within automatic_renewals.pl and passed the array to queue one indexing job instead of multiple jobs.

Test plan:
1) AddRenewal uses Koha::Items->store() to trigger the indexing process. Run prove -vv t/db_dependent/Koha/SearchEngine/Indexer.t and check tests 5,6,29,30. These tests prove whether passing skip_record_index to store() triggers or skips the indexing process. All four tests should pass to show that skip_index_records can prevent the indexing being triggered.
2) Add multiple renewals that are able to be autorenewed and run the automatic_renewals.pl script. There should be multiple items queued in zebraqueue.
3) Apply patch and try again
4) There should now only be one job queued in zebraqueue

Mentored-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 15 Nick Clemens 2023-04-07 13:41:03 UTC
Created attachment 149277 [details] [review]
Bug 32013: (follow-up) Add unit test

A unit test has been added to show the pass-through of skip_record_index from AddRenewal() to the Item->store() method.

Test plan:
1) In the shell run prove -vv t/db_dependent/Koha/SearchEngine/Indexer.t
2) Observe Subtest 3: Test AddRenewal indexer call
3) All four tests should pass

Mentored-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 16 Nick Clemens 2023-04-07 13:41:05 UTC
Created attachment 149278 [details] [review]
Bug 32013: (QA follow-up) Move mock to avoid warns from sample data creation

Mentored-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 17 Nick Clemens 2023-04-07 13:41:07 UTC
Created attachment 149279 [details] [review]
Bug 32013: Adjust unit tests
Comment 18 Nick Clemens 2023-04-07 13:42:01 UTC
(In reply to Matt Blenkinsop from comment #13)
> Nick - Does this patch cover what you were trying to achieve with 33309 or
> do we need to combine them in some way?

I think leaving the check I added, and moving your check down to the new section afterward seems to work - can you and Martin give my updates a once over?
Comment 19 Matt Blenkinsop 2023-04-11 12:45:01 UTC
Seems logical to me and code looks fine - thanks Nick!
Comment 20 Tomás Cohen Arazi 2023-04-12 12:27:36 UTC
Pushed to master for 23.05.

Nice work everyone, thanks!
Comment 21 Jacob O'Mara 2023-04-17 07:28:46 UTC
Enhancement - not backporting to 22.11.x

Nice work everyone!
Comment 22 Aude Charillon 2023-05-16 09:03:13 UTC
No need for any changes in the Manual; updating status from Needs documenting to Resolved.