Bug 26581

Summary: Elasticsearch - Records can be indexed multiple times during returns
Product: Koha Reporter: Nick Clemens <nick>
Component: Searching - ZebraAssignee: Nick Clemens <nick>
Status: CLOSED FIXED QA Contact:
Severity: major    
Priority: P5 - low CC: aleisha, jonathan.druart, lucas, martin.renvoize, severine.queune
Version: Main   
Hardware: All   
OS: All   
See Also: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=25265
Change sponsored?: --- Patch complexity: ---
Documentation contact: Documentation submission:
Text to go in the release notes:
Version(s) released in:
20.11.00, 20.05.06
Bug Depends on: 25265    
Bug Blocks: 28472    
Attachments: Bug 26581: Unit tests
Bug 26581: Only reindex records once per checkin
Bug 26581: Unit tests
Bug 26581: Only reindex records once per checkin
Bug 26581: Unit tests
Bug 26581: Only reindex records once per checkin
Bug 26581: [20.05.x] Only reindex records once per checkin
Bug 26581: (follow-up) Restore unsetting of doreturn
Bug 26581: (follow-up) Skip merge when deleting authority

Description Nick Clemens 2020-09-30 16:51:22 UTC
Throughout add return we actually make several updates to the item:
We set onloan -> null
We update item location if pref says so
We update datelast seen
We update not for loan is pref says so
etc.

For ES, this means a new connection and a reindex each time. This is problematic
Comment 1 Nick Clemens 2020-09-30 16:56:50 UTC
Created attachment 111001 [details] [review]
Bug 26581: Unit tests

These tests cover the changes to ensure the AddReturn calls index_records once
per call and that other calls pass the skip_record_index parameter correctly
Comment 2 Nick Clemens 2020-09-30 16:56:54 UTC
Created attachment 111002 [details] [review]
Bug 26581: Only reindex records once per checkin

This patch simply passes skip_record_index calls to Koha:Item:store
for all the changes done in AddReturn.

Testing is really verifiying that items are still correctly indexed at the end

For both search engines
To test:
1 - Find or create a record with multipel items
2 - Populate both:
     UpdateItemLocationOnCheckin
     UpdateNotForLoanStatusOnCheckin
3 - Confirm that checking in an item correctly updates the item status in search results
4 - Test with items issued, and items not issued
5 - Test when generating a transfer (checkin at different branch)
6 - Test when item was marked lost
7 - Test when filling transfer
Comment 3 Jonathan Druart 2020-09-30 18:06:15 UTC
Shouldn't actually do the reverse and index on demand?
Comment 4 Jonathan Druart 2020-09-30 18:09:21 UTC
(In reply to Jonathan Druart from comment #3)
> Shouldn't actually do the reverse and index on demand?

*we*

Well, it's just a thought, I haven't investigated it!
Comment 5 Nick Clemens 2020-09-30 18:52:00 UTC
(In reply to Jonathan Druart from comment #4)
> (In reply to Jonathan Druart from comment #3)
> > Shouldn't actually do the reverse and index on demand?
> 
> *we*
> 
> Well, it's just a thought, I haven't investigated it!

Can you explain a bit more? Are you suggesting 'index_record' and don't index unless passed?
Comment 6 Jonathan Druart 2020-09-30 19:01:35 UTC
(In reply to Nick Clemens from comment #5)
> (In reply to Jonathan Druart from comment #4)
> > (In reply to Jonathan Druart from comment #3)
> > > Shouldn't actually do the reverse and index on demand?
> > 
> > *we*
> > 
> > Well, it's just a thought, I haven't investigated it!
> 
> Can you explain a bit more? Are you suggesting 'index_record' and don't
> index unless passed?

Yes
Comment 7 Nick Clemens 2020-10-01 10:19:08 UTC
(In reply to Jonathan Druart from comment #6)
> (In reply to Nick Clemens from comment #5)
> > (In reply to Jonathan Druart from comment #4)
> > > (In reply to Jonathan Druart from comment #3)
> > > > Shouldn't actually do the reverse and index on demand?
> > > 
> > > *we*
> > > 
> > > Well, it's just a thought, I haven't investigated it!
> > 
> > Can you explain a bit more? Are you suggesting 'index_record' and don't
> > index unless passed?
> 
> Yes

I am not opposed to the idea, however, it would mean redoing a lot of the code which was just done in bug 25265 and other places. I suspect it would not eliminate code, but just change the basic idea.

Here we could possibly eliminate some of the storage calls, to reduce the number we have to 'skip' butlarger refactoring is needed overall so this was a start
Comment 8 Séverine Queune 2020-10-14 06:34:12 UTC
I think I've tested all the different possible cases with ES and it works as expected, but I don't have Zebra running on devbox.
Should I sign the patch thought ?
Comment 9 Andrew Fuerste-Henry 2020-10-14 11:10:09 UTC
Created attachment 111596 [details] [review]
Bug 26581: Unit tests

These tests cover the changes to ensure the AddReturn calls index_records once
per call and that other calls pass the skip_record_index parameter correctly

Signed-off-by: Christoper Brannon <cbrannon@cdalibrary.org>
Comment 10 Andrew Fuerste-Henry 2020-10-14 11:10:22 UTC
Created attachment 111597 [details] [review]
Bug 26581: Only reindex records once per checkin

This patch simply passes skip_record_index calls to Koha:Item:store
for all the changes done in AddReturn.

Testing is really verifiying that items are still correctly indexed at the end

For both search engines
To test:
1 - Find or create a record with multipel items
2 - Populate both:
     UpdateItemLocationOnCheckin
     UpdateNotForLoanStatusOnCheckin
3 - Confirm that checking in an item correctly updates the item status in search results
4 - Test with items issued, and items not issued
5 - Test when generating a transfer (checkin at different branch)
6 - Test when item was marked lost
7 - Test when filling transfer

Signed-off-by: Christoper Brannon <cbrannon@cdalibrary.org
>
;
>


Signed-off-by: Christoper Brannon <cbrannon@cdalibrary.org>
Comment 11 Martin Renvoize 2020-10-16 14:07:35 UTC
Created attachment 111878 [details] [review]
Bug 26581: Unit tests

These tests cover the changes to ensure the AddReturn calls index_records once
per call and that other calls pass the skip_record_index parameter correctly

Signed-off-by: Séverine Queune <severine.queune@bulac.fr>
Signed-off-by: Christoper Brannon <cbrannon@cdalibrary.org>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 12 Martin Renvoize 2020-10-16 14:07:39 UTC
Created attachment 111879 [details] [review]
Bug 26581: Only reindex records once per checkin

This patch simply passes skip_record_index calls to Koha:Item:store
for all the changes done in AddReturn.

Testing is really verifiying that items are still correctly indexed at the end

For both search engines
To test:
1 - Find or create a record with multipel items
2 - Populate both:
     UpdateItemLocationOnCheckin
     UpdateNotForLoanStatusOnCheckin
3 - Confirm that checking in an item correctly updates the item status in search results
4 - Test with items issued, and items not issued
5 - Test when generating a transfer (checkin at different branch)
6 - Test when item was marked lost
7 - Test when filling transfer

Signed-off-by: Séverine Queune <severine.queune@bulac.fr>
Signed-off-by: Christoper Brannon <cbrannon@cdalibrary.org>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 13 Martin Renvoize 2020-10-16 14:08:41 UTC
This works as expected and passes all tests.

I would really like to see us reduce the calls to store in the long run, rather than just adding a flag to prevent the re-indexing.. but this certainly does the trick for now and that's a longer term goal.

Passing QA
Comment 14 Jonathan Druart 2020-10-22 08:25:25 UTC
Pushed to master for 20.11, thanks to everybody involved!
Comment 15 Lucas Gass 2020-10-23 15:55:18 UTC
this patchset does not apply cleanly to 20.05.x, please rebase if needed
Comment 16 Jonathan Druart 2020-10-25 21:33:25 UTC
There are two failing tests caused by this change:

t/db_dependent/SIP/Message.t .. 1/9 
    #   Failed test 'OK flag is false when we check in at the wrong branch and we do not allow it'
    #   at t/db_dependent/SIP/Message.t line 632.
    #          got: '1'
    #     expected: '0'

    #   Failed test 'Code AF not found in '|AOz2h4hfYk|ABjPj5C_zEvKlqRRsSsLQf|AQz2h4hfYk|AJSome boring read|CKMHE|CTz2h4hfYk|CV04|'?'
    #   at t/db_dependent/SIP/Message.t line 733.
    #          got: '0'
    #     expected: '1'
    # Looks like you failed 2 tests of 33.
t/db_dependent/SIP/Message.t .. 3/9 
#   Failed test 'Checkin V2'
#   at t/db_dependent/SIP/Message.t line 76.


Also maybe t/db_dependent/Koha/SearchEngine/Indexer.t that is failing on jenkins (but cannot recreate locally).

Please fix asap.

Also I don't understand what happened but the authorship of the first patch changed during the QA process.
Comment 17 Nick Clemens 2020-10-27 11:53:19 UTC
Created attachment 112563 [details] [review]
Bug 26581: [20.05.x] Only reindex records once per checkin

This patch simply passes skip_record_index calls to Koha:Item:store
for all the changes done in AddReturn.

Testing is really verifiying that items are still correctly indexed at the end

For both search engines
To test:
1 - Find or create a record with multipel items
2 - Populate both:
     UpdateItemLocationOnCheckin
     UpdateNotForLoanStatusOnCheckin
3 - Confirm that checking in an item correctly updates the item status in search results
4 - Test with items issued, and items not issued
5 - Test when generating a transfer (checkin at different branch)
6 - Test when item was marked lost
7 - Test when filling transfer

Signed-off-by: Séverine Queune <severine.queune@bulac.fr>
Signed-off-by: Christoper Brannon <cbrannon@cdalibrary.org>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Bug 26581: Unit tests

These tests cover the changes to ensure the AddReturn calls index_records once
per call and that other calls pass the skip_record_index parameter correctly

Signed-off-by: Séverine Queune <severine.queune@bulac.fr>
Signed-off-by: Christoper Brannon <cbrannon@cdalibrary.org>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 18 Nick Clemens 2020-10-27 14:36:49 UTC
Created attachment 112578 [details] [review]
Bug 26581: (follow-up) Restore unsetting of doreturn
Comment 19 Jonathan Druart 2020-11-02 08:47:15 UTC
(In reply to Nick Clemens from comment #18)
> Created attachment 112578 [details] [review] [review]
> Bug 26581: (follow-up) Restore unsetting of doreturn

Is this supposed to fix comment 16?
Comment 20 Jonathan Druart 2020-11-02 10:06:02 UTC
(In reply to Nick Clemens from comment #18)
> Created attachment 112578 [details] [review] [review]
> Bug 26581: (follow-up) Restore unsetting of doreturn

Pushed to master for 20.11
Comment 21 Nick Clemens 2020-11-02 12:48:30 UTC
Created attachment 112829 [details] [review]
Bug 26581: (follow-up) Skip merge when deleting authority

Tests failing, seems to be because the indexes haven't been created yet.
When deleting authority we perform a 'merge' where we search for biblios
containing the authority.

Since these are simply tets we can skip this step, avoid the search, and avoid
a crash
Comment 22 Jonathan Druart 2020-11-02 12:51:36 UTC
(In reply to Nick Clemens from comment #21)
> Created attachment 112829 [details] [review] [review]
> Bug 26581: (follow-up) Skip merge when deleting authority
> 
> Tests failing, seems to be because the indexes haven't been created yet.
> When deleting authority we perform a 'merge' where we search for biblios
> containing the authority.
> 
> Since these are simply tets we can skip this step, avoid the search, and
> avoid
> a crash

Pushed to master, thanks Nick!
Comment 23 Lucas Gass 2020-11-16 18:06:02 UTC
backported to 20.05.x for 20.05.06
Comment 24 Aleisha Amohia 2020-11-17 05:11:44 UTC
does not apply cleanly on 19.11.x, please rebase
Comment 25 Nick Clemens 2020-11-23 17:01:19 UTC
(In reply to Aleisha Amohia from comment #24)
> does not apply cleanly on 19.11.x, please rebase

Relies on 25265 which relies on 26507 (though not really) which relies on 23463.

Changes a bit bigger than I can clear through easily, anyone else is welcome to take stab, but I think it's okay if only in stable
Comment 26 Aleisha Amohia 2020-11-23 20:00:35 UTC
(In reply to Nick Clemens from comment #25)
> (In reply to Aleisha Amohia from comment #24)
> > does not apply cleanly on 19.11.x, please rebase
> 
> Relies on 25265 which relies on 26507 (though not really) which relies on
> 23463.
> 
Thanks, I have added the dependency to the bug report.

missing dependencies, not backported to 19.11.x