Bug 30727

Summary: Holds queue updates can be called multiple times on batch record deletion
Product: Koha Reporter: Tomás Cohen Arazi <tomascohen>
Component: Architecture, internals, and plumbingAssignee: Tomás Cohen Arazi <tomascohen>
Status: CLOSED FIXED QA Contact: Martin Renvoize <martin.renvoize>
Severity: normal    
Priority: P5 - low CC: david, jonathan.druart, martin.renvoize, nick
Version: unspecified   
Hardware: All   
OS: All   
Change sponsored?: --- Patch complexity: ---
Documentation contact: Documentation submission:
Text to go in the release notes:
Version(s) released in:
22.05.00
Bug Depends on: 30710    
Bug Blocks: 30728    
Attachments: Bug 30727: Regression tests
Bug 30727: Avoid holds queue updates multiple times on BatchDeleteBiblio
Bug 30727: Regression tests
Bug 30727: Avoid holds queue updates multiple times on BatchDeleteBiblio
Bug 30727: Regression tests
Bug 30727: Avoid holds queue updates multiple times on BatchDeleteBiblio

Description Tomás Cohen Arazi 2022-05-10 15:25:17 UTC

    
Comment 1 Tomás Cohen Arazi 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 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 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 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 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 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 🦄