Bug 30467 - BatchDeleteItem task does not deal with indexation correctly
Summary: BatchDeleteItem task does not deal with indexation correctly
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 (vote)
Assignee: Jonathan Druart
QA Contact: Martin Renvoize
URL:
Keywords:
Depends on: 27344
Blocks: 27341 30710
  Show dependency treegraph
 
Reported: 2022-04-06 12:04 UTC by Jonathan Druart
Modified: 2022-12-12 21:24 UTC (History)
6 users (show)

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


Attachments
Bug 30467: Don't send the delete request for each item (1.00 KB, patch)
2022-04-06 12:14 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 30467: Don't send the delete request for each item (1022 bytes, patch)
2022-04-29 17:13 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 30467: Don't send the delete request for each item (1.06 KB, patch)
2022-05-05 14:01 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 30467: (QA follow-up) Restore indexing for item only deletion (2.18 KB, patch)
2022-05-06 07:12 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 30467: (QA follow-up) Restore indexing for item only deletion (3.27 KB, patch)
2022-05-06 07:26 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 30467: (QA follow-up) Fix copy paste error to update updated_biblionumbers (1000 bytes, patch)
2022-05-06 14:36 UTC, Nick Clemens (kidclamp)
Details | Diff | Splinter Review
Bug 30467: (QA follow-up) Fix copy paste error to update updated_biblionumbers (1.04 KB, patch)
2022-05-06 15:07 UTC, Martin Renvoize
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Jonathan Druart 2022-04-06 12:04:15 UTC

    
Comment 1 Jonathan Druart 2022-04-06 12:14:59 UTC
Created attachment 132992 [details] [review]
Bug 30467: Don't send the delete request for each item

We are sending the request when all the items have been deleted.
Comment 2 Tomás Cohen Arazi 2022-04-29 17:13:17 UTC
Created attachment 134398 [details] [review]
Bug 30467: Don't send the delete request for each item

We are sending the request when all the items have been deleted.
Comment 3 Martin Renvoize 2022-05-05 14:01:17 UTC
Created attachment 134640 [details] [review]
Bug 30467: Don't send the delete request for each item

We are sending the request when all the items have been deleted.

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 4 Martin Renvoize 2022-05-05 14:02:12 UTC
Yup, confirmed...

All works and this prevents a double indexing process.

Passing QA
Comment 5 Fridolin Somers 2022-05-05 20:46:35 UTC
Sorry does not work for me.

In Koha::BackgroundJob::BatchDeleteItem :

I see there is indexing for deleted biblio records :
                    if (@deleted_biblionumbers) {
                        my $indexer = Koha::SearchEngine::Indexer->new(
                            { index => $Koha::SearchEngine::BIBLIOS_INDEX } );

                        $indexer->index_records( \@deleted_biblionumbers,
                            'recordDelete', "biblioserver", undef );
                    }

But I see not indexing for deleted items.

I see batch items update uses Koha::Items->batch_update() with :
    if (@modified_itemnumbers) {
        my @biblionumbers = uniq(
            Koha::Items->search( { itemnumber => \@modified_itemnumbers } )
                       ->get_column('biblionumber'));

        if ( @biblionumbers ) {
            my $indexer = Koha::SearchEngine::Indexer->new(
                { index => $Koha::SearchEngine::BIBLIOS_INDEX } );

            $indexer->update_index( \@biblionumbers );
        }
    }

Should we also create Koha::Items->batch_delete() ?
Maybe in another bug report.
Comment 6 Martin Renvoize 2022-05-06 06:50:41 UTC
Item updates result in biblio re-indexes and these item deletes are all attached to the biblio's we're deleting.. so the re-index is caught by the biblio re-index call you spotted.

Or am I missing something more involved?
Comment 7 Martin Renvoize 2022-05-06 07:12:10 UTC
Created attachment 134672 [details] [review]
Bug 30467: (QA follow-up) Restore indexing for item only deletion

This patch restores indexing for item only bulk deletions.
Comment 8 Martin Renvoize 2022-05-06 07:12:52 UTC
I was totally missing something, good catch.

Follow-up attached, but I'd love to get a QA stamp from someone to check my work on it
Comment 9 Jonathan Druart 2022-05-06 07:26:38 UTC
Created attachment 134675 [details] [review]
Bug 30467: (QA follow-up) Restore indexing for item only deletion

This patch restores indexing for item only bulk deletions.

JD amended patch:
* remove trailing whitespaces
* Move if inside first condition

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 10 Nick Clemens (kidclamp) 2022-05-06 14:36:25 UTC
Created attachment 134741 [details] [review]
Bug 30467: (QA follow-up) Fix copy paste error to update updated_biblionumbers
Comment 11 Martin Renvoize 2022-05-06 15:07:07 UTC
Created attachment 134744 [details] [review]
Bug 30467: (QA follow-up) Fix copy paste error to update updated_biblionumbers

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 12 Fridolin Somers 2022-05-06 19:16:00 UTC
Works perfect, thanks for your fast and great work
Comment 13 Fridolin Somers 2022-05-06 20:34:25 UTC
Pushed to master for 22.05, thanks to everybody involved 🦄