Bug 30727 - Holds queue updates can be called multiple times on batch record deletion
Summary: Holds queue updates can be called multiple times on batch record deletion
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Architecture, internals, and plumbing (show other bugs)
Version: unspecified
Hardware: All All
: P5 - low normal
Assignee: Tomás Cohen Arazi (tcohen)
QA Contact: Martin Renvoize (ashimema)
URL:
Keywords:
Depends on: 30710
Blocks: 30728
  Show dependency treegraph
 
Reported: 2022-05-10 15:25 UTC by Tomás Cohen Arazi (tcohen)
Modified: 2023-12-28 20:43 UTC (History)
4 users (show)

See Also:
Change sponsored?: ---
Patch complexity: ---
Documentation contact:
Documentation submission:
Text to go in the release notes:
Version(s) released in:
22.05.00
Circulation function:


Attachments
Bug 30727: Regression tests (3.11 KB, patch)
2022-05-10 15:29 UTC, Tomás Cohen Arazi (tcohen)
Details | Diff | Splinter Review
Bug 30727: Avoid holds queue updates multiple times on BatchDeleteBiblio (2.80 KB, patch)
2022-05-10 15:29 UTC, Tomás Cohen Arazi (tcohen)
Details | Diff | Splinter Review
Bug 30727: Regression tests (3.15 KB, patch)
2022-05-10 22:35 UTC, David Nind
Details | Diff | Splinter Review
Bug 30727: Avoid holds queue updates multiple times on BatchDeleteBiblio (2.84 KB, patch)
2022-05-10 22:35 UTC, David Nind
Details | Diff | Splinter Review
Bug 30727: Regression tests (3.22 KB, patch)
2022-05-11 10:01 UTC, Martin Renvoize (ashimema)
Details | Diff | Splinter Review
Bug 30727: Avoid holds queue updates multiple times on BatchDeleteBiblio (2.91 KB, patch)
2022-05-11 10:01 UTC, Martin Renvoize (ashimema)
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Tomás Cohen Arazi (tcohen) 2022-05-10 15:25:17 UTC

    
Comment 1 Tomás Cohen Arazi (tcohen) 2022-05-10 15:29:03 UTC
Created attachment 134820 [details] [review]
Bug 30727: Regression tests

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 2 Tomás Cohen Arazi (tcohen) 2022-05-10 15:29:07 UTC
Created attachment 134821 [details] [review]
Bug 30727: Avoid holds queue updates multiple times on BatchDeleteBiblio

This patch makes the background job only enqueue holds queue updates
once per biblio, when appropriate.

To test:
1. Apply the regression tests patch
2. Run:
   $ kshell
  k$ prove t/db_dependent/Koha/BackgroundJobs/BatchDeleteBiblio.t
=> FAIL: It enqueues 3 times for a bib!
3. Apply this patch
4. Repeat 2
=> SUCCESS: It only enqueues once!
5. Sign off :-D

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 3 David Nind 2022-05-10 20:50:17 UTC
The tests fail for me for step 4:

prove -v t/db_dependent/Koha/BackgroundJobs/BatchDeleteBiblio.t
t/db_dependent/Koha/BackgroundJobs/BatchDeleteBiblio.t .. 
1..1
# Subtest: process() tests
    1..1
    not ok 1 - Holds queue update is enqueued only once

    #   Failed test 'Holds queue update is enqueued only once'
    #   at t/db_dependent/Koha/BackgroundJobs/BatchDeleteBiblio.t line 81.
    #          got: '4'
    #     expected: '1'
    # Looks like you failed 1 test of 1.
not ok 1 - process() tests

#   Failed test 'process() tests'
#   at t/db_dependent/Koha/BackgroundJobs/BatchDeleteBiblio.t line 84.
# Looks like you failed 1 test of 1.
Dubious, test returned 1 (wstat 256, 0x100)
Failed 1/1 subtests 

Test Summary Report
-------------------
t/db_dependent/Koha/BackgroundJobs/BatchDeleteBiblio.t (Wstat: 256 Tests: 1 Failed: 1)
  Failed test:  1
  Non-zero exit status: 1
Files=1, Tests=1,  3 wallclock secs ( 0.02 usr  0.01 sys +  2.25 cusr  0.18 csys =  2.46 CPU)
Result: FAIL
Comment 4 Tomás Cohen Arazi (tcohen) 2022-05-10 22:01:33 UTC
Sorry, I missed the fact the skip_holds_queue parameter was added on bug 30710. Tweaked the dependencies.
Comment 5 David Nind 2022-05-10 22:34:44 UTC
(In reply to Tomás Cohen Arazi from comment #4)
> Sorry, I missed the fact the skip_holds_queue parameter was added on bug
> 30710. Tweaked the dependencies.

Thanks Tomás! Tests now pass, so sign-off on the way.
Comment 6 David Nind 2022-05-10 22:35:52 UTC
Created attachment 134840 [details] [review]
Bug 30727: Regression tests

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

Signed-off-by: David Nind <david@davidnind.com>
Comment 7 David Nind 2022-05-10 22:35:57 UTC
Created attachment 134841 [details] [review]
Bug 30727: Avoid holds queue updates multiple times on BatchDeleteBiblio

This patch makes the background job only enqueue holds queue updates
once per biblio, when appropriate.

To test:
1. Apply the regression tests patch
2. Run:
   $ kshell
  k$ prove t/db_dependent/Koha/BackgroundJobs/BatchDeleteBiblio.t
=> FAIL: It enqueues 3 times for a bib!
3. Apply this patch
4. Repeat 2
=> SUCCESS: It only enqueues once!
5. Sign off :-D

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

Signed-off-by: David Nind <david@davidnind.com>
Comment 8 Martin Renvoize (ashimema) 2022-05-11 10:01:37 UTC
Created attachment 134855 [details] [review]
Bug 30727: Regression tests

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Signed-off-by: David Nind <david@davidnind.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 9 Martin Renvoize (ashimema) 2022-05-11 10:01:41 UTC
Created attachment 134856 [details] [review]
Bug 30727: Avoid holds queue updates multiple times on BatchDeleteBiblio

This patch makes the background job only enqueue holds queue updates
once per biblio, when appropriate.

To test:
1. Apply the regression tests patch
2. Run:
   $ kshell
  k$ prove t/db_dependent/Koha/BackgroundJobs/BatchDeleteBiblio.t
=> FAIL: It enqueues 3 times for a bib!
3. Apply this patch
4. Repeat 2
=> SUCCESS: It only enqueues once!
5. Sign off :-D

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Signed-off-by: David Nind <david@davidnind.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 10 Martin Renvoize (ashimema) 2022-05-11 10:03:54 UTC
Tests pass, QA Script is happy and the code works as described.

A solid fix, thanks Tomas.

PQA
Comment 11 Fridolin Somers 2022-05-13 08:17:25 UTC
Pushed to master for 22.05, thanks to everybody involved ﷐[U+1F984]﷑