[Bug 32013] New: Autorenewals is effectively a bulk action and should be treated as such
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=32013 Bug ID: 32013 Summary: Autorenewals is effectively a bulk action and should be treated as such Change sponsored?: --- Product: Koha Version: master Hardware: All OS: All Status: NEW Severity: enhancement Priority: P5 - low Component: Architecture, internals, and plumbing Assignee: koha-bugs@lists.koha-community.org Reporter: martin.renvoize@ptfs-europe.com QA Contact: testopia@bugs.koha-community.org 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. -- You are receiving this mail because: You are watching all bug changes. You are the assignee for the bug.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=32013 --- Comment #1 from Martin Renvoize <martin.renvoize@ptfs-europe.com> --- 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. -- You are receiving this mail because: You are the assignee for the bug. You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=32013 Martin Renvoize <martin.renvoize@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Assignee|koha-bugs@lists.koha-commun |matt.blenkinsop@ptfs-europe |ity.org |.com -- You are receiving this mail because: You are watching all bug changes. You are the assignee for the bug.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=32013 --- Comment #2 from Matt Blenkinsop <matt.blenkinsop@ptfs-europe.com> --- Created attachment 144442 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=144442&action=edit 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 -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=32013 Matt Blenkinsop <matt.blenkinsop@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|NEW |Needs Signoff -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=32013 --- Comment #3 from Matt Blenkinsop <matt.blenkinsop@ptfs-europe.com> --- Created attachment 144464 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=144464&action=edit 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 -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=32013 Martin Renvoize <martin.renvoize@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |nick@bywatersolutions.com -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=32013 Martin Renvoize <martin.renvoize@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |jonathan.druart+koha@gmail. | |com -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=32013 Nick Clemens <nick@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Needs Signoff |Signed Off -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=32013 Nick Clemens <nick@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #144442|0 |1 is obsolete| | --- Comment #4 from Nick Clemens <nick@bywatersolutions.com> --- Created attachment 145150 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=145150&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=32013 Nick Clemens <nick@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #144464|0 |1 is obsolete| | --- Comment #5 from Nick Clemens <nick@bywatersolutions.com> --- Created attachment 145151 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=145151&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=32013 --- Comment #6 from Nick Clemens <nick@bywatersolutions.com> --- Created attachment 145152 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=145152&action=edit Bug 32013: (QA follow-up) Move mock to avoid warns from sample data creation Signed-off-by: Nick Clemens <nick@bywatersolutions.com> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=32013 Martin Renvoize <martin.renvoize@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #145150|0 |1 is obsolete| | --- Comment #7 from Martin Renvoize <martin.renvoize@ptfs-europe.com> --- Created attachment 145186 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=145186&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=32013 Martin Renvoize <martin.renvoize@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #145151|0 |1 is obsolete| | --- Comment #8 from Martin Renvoize <martin.renvoize@ptfs-europe.com> --- Created attachment 145187 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=145187&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=32013 Martin Renvoize <martin.renvoize@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #145152|0 |1 is obsolete| | --- Comment #9 from Martin Renvoize <martin.renvoize@ptfs-europe.com> --- Created attachment 145188 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=145188&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=32013 --- Comment #10 from Martin Renvoize <martin.renvoize@ptfs-europe.com> --- 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. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=32013 Nick Clemens <nick@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Signed Off |Passed QA -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=32013 --- Comment #11 from Nick Clemens <nick@bywatersolutions.com> --- Minor patch, moving to PQA -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=32013 Nick Clemens <nick@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- QA Contact|testopia@bugs.koha-communit |nick@bywatersolutions.com |y.org | -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=32013 Tomás Cohen Arazi <tomascohen@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |tomascohen@gmail.com Status|Passed QA |Patch doesn't apply --- Comment #12 from Tomás Cohen Arazi <tomascohen@gmail.com> --- Please rebase ASAP. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=32013 --- Comment #13 from Matt Blenkinsop <matt.blenkinsop@ptfs-europe.com> --- 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? -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=32013 Nick Clemens <nick@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Patch doesn't apply |Passed QA -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=32013 Nick Clemens <nick@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #145186|0 |1 is obsolete| | --- Comment #14 from Nick Clemens <nick@bywatersolutions.com> --- Created attachment 149276 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=149276&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=32013 Nick Clemens <nick@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #145187|0 |1 is obsolete| | --- Comment #15 from Nick Clemens <nick@bywatersolutions.com> --- Created attachment 149277 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=149277&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=32013 Nick Clemens <nick@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #145188|0 |1 is obsolete| | --- Comment #16 from Nick Clemens <nick@bywatersolutions.com> --- Created attachment 149278 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=149278&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=32013 --- Comment #17 from Nick Clemens <nick@bywatersolutions.com> --- Created attachment 149279 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=149279&action=edit Bug 32013: Adjust unit tests -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=32013 --- Comment #18 from Nick Clemens <nick@bywatersolutions.com> --- (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? -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=32013 Nick Clemens <nick@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Blocks| |33444 Referenced Bugs: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=33444 [Bug 33444] AddRenewal should take a hash of parameters -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=32013 --- Comment #19 from Matt Blenkinsop <matt.blenkinsop@ptfs-europe.com> --- Seems logical to me and code looks fine - thanks Nick! -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=32013 Tomás Cohen Arazi <tomascohen@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Passed QA |Pushed to master Version(s)| |23.05.00 released in| | -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=32013 --- Comment #20 from Tomás Cohen Arazi <tomascohen@gmail.com> --- Pushed to master for 23.05. Nice work everyone, thanks! -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=32013 Jacob O'Mara <jacob.omara@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Pushed to master |Needs documenting CC| |jacob.omara@ptfs-europe.com --- Comment #21 from Jacob O'Mara <jacob.omara@ptfs-europe.com> --- Enhancement - not backporting to 22.11.x Nice work everyone! -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=32013 Aude Charillon <aude.charillon@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |aude.charillon@ptfs-europe. | |com Resolution|--- |FIXED Status|Needs documenting |RESOLVED --- Comment #22 from Aude Charillon <aude.charillon@ptfs-europe.com> --- No need for any changes in the Manual; updating status from Needs documenting to Resolved. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=32013 Martin Renvoize <martin.renvoize@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Text to go in the| |**Sponsored by** *PTFS release notes| |Europe* -- You are receiving this mail because: You are watching all bug changes.
participants (1)
-
bugzilla-daemon＠bugs.koha-community.org