Bug 32054 - GetImportRecordMatches returns the wrong match when passed 'best only'
Summary: GetImportRecordMatches returns the wrong match when passed 'best only'
Status: RESOLVED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Tools (show other bugs)
Version: Main
Hardware: All All
: P5 - low major (vote)
Assignee: Nick Clemens
QA Contact: Marcel de Rooy
URL:
Keywords:
Depends on:
Blocks: 32437
  Show dependency treegraph
 
Reported: 2022-10-31 17:54 UTC by Nick Clemens
Modified: 2023-12-28 20:44 UTC (History)
2 users (show)

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


Attachments
Bug 32054: Add get_import_record_matches object method and use it (9.50 KB, patch)
2022-10-31 18:15 UTC, Nick Clemens
Details | Diff | Splinter Review
Sample biblio from sample database (6.23 KB, application/xml)
2022-11-01 19:20 UTC, Nick Clemens
Details
Bug 32054: Add get_import_record_matches object method and use it (9.55 KB, patch)
2022-11-04 22:33 UTC, David Nind
Details | Diff | Splinter Review
Bug 32054: Add get_import_record_matches object method and use it (9.65 KB, patch)
2023-01-17 15:25 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 30254: [22.05.x] Don't charge overdue fines unless some fine exists (6.77 KB, patch)
2023-02-22 19:56 UTC, Nick Clemens
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Nick Clemens 2022-10-31 17:54:11 UTC
The routine orders by score/biblionumber desc - it limits to the first match when 'best only' is passed.

With bug 22785 we introduced a 'chosen' field to allow the best, or no match, be chosen

We need to honor this when getting the matches

This causes order imports to choose the wrong match
Comment 1 Nick Clemens 2022-10-31 18:15:15 UTC
Created attachment 142838 [details] [review]
Bug 32054: Add get_import_record_matches object method and use it

Thispatch adds the new method and alters addorderiso2907.pl to use this
rather than GetRecordImportMatches

To test:
 1 - Import the attached record several times
 2 - Set up a matching rule:
    TitleAuthor threshold: 100
    Matchpoint:
    search index: title, score: 100, tag: 245$a
    search index: author, score: 100, tag:100$a
 3 - Edit one of the imported records to have a different author
 4 - Stage the file again. and match using the matchpoint above
 5 - Note that matches are found and listed on batch management, with the lowest scored match last
 6 - Choose that match
 7 - In acquisitions, add to a basket from the staged file
 8 - Check the box for the record
 9 - Note the match lists the biblionumber for the highest scoring match, not the chosen one
10 - Add an order and note it is for the wrong biblio
11 - Appy patch
12 - Restart_all
13 - Stage the file again and choose a lower scoring match
14 - Confirm when adding to basket this match is preserved
15 - Complete order and verify correct biblio ordered
16 - Stage again, select no match
17 - Confirm no match listed when adding to basket, and choose 'Do not look for matching records' while adding
18 - Confirm order is created on a new biblio
Comment 2 David Nind 2022-11-01 18:52:15 UTC
I'm happy to try and test, however there is no attached record (step 1 of the test plan).
Comment 3 Nick Clemens 2022-11-01 19:20:11 UTC
Created attachment 142896 [details]
Sample biblio from sample database
Comment 4 David Nind 2022-11-04 22:33:15 UTC
Created attachment 143293 [details] [review]
Bug 32054: Add get_import_record_matches object method and use it

Thispatch adds the new method and alters addorderiso2907.pl to use this
rather than GetRecordImportMatches

To test:
 1 - Import the attached record several times
 2 - Set up a matching rule:
    TitleAuthor threshold: 100
    Matchpoint:
    search index: title, score: 100, tag: 245$a
    search index: author, score: 100, tag:100$a
 3 - Edit one of the imported records to have a different author
 4 - Stage the file again. and match using the matchpoint above
 5 - Note that matches are found and listed on batch management, with the lowest scored match last
 6 - Choose that match
 7 - In acquisitions, add to a basket from the staged file
 8 - Check the box for the record
 9 - Note the match lists the biblionumber for the highest scoring match, not the chosen one
10 - Add an order and note it is for the wrong biblio
11 - Appy patch
12 - Restart_all
13 - Stage the file again and choose a lower scoring match
14 - Confirm when adding to basket this match is preserved
15 - Complete order and verify correct biblio ordered
16 - Stage again, select no match
17 - Confirm no match listed when adding to basket, and choose 'Do not look for matching records' while adding
18 - Confirm order is created on a new biblio

Signed-off-by: David Nind <david@davidnind.com>
Comment 5 David Nind 2022-11-04 22:36:36 UTC
I think I tested this correctly - this is the first time I've used matching rules and adding to an order from a staged record.

Mainly found the options for steps 14-15, and the duplicate record message and choices, a bit confusing.
Comment 6 Marcel de Rooy 2022-12-16 10:12:05 UTC
This patch leaves us with one actual remaining call of GetImportRecordMatches:

C4/ImportBatch.pm:      GetImportRecordMatches
C4/ImportBatch.pm:=head2 GetImportRecordMatches
C4/ImportBatch.pm:  my $results = GetImportRecordMatches($import_record_id, $best_only);
C4/ImportBatch.pm:sub GetImportRecordMatches {
tools/batch_records_ajax.pl:use C4::ImportBatch qw( GetImportBatch GetImportRecordsRange GetImportRecordMatches );
tools/batch_records_ajax.pl:    my $matches = GetImportRecordMatches( $record->{'import_record_id'} );

The last one is the only real one.

koha-tmpl/intranet-tmpl/prog/en/modules/tools/manage-marc-import.tt:                    "sAjaxSource": 'batch_records_ajax.pl',
Comment 7 Marcel de Rooy 2022-12-16 10:13:50 UTC
Having issues with the background jobs here. The staged file job is stuck on New status. Restarted stuff etc. No change at all. Logs dont make me wiser. Please confirm if it still works..
Comment 8 Nick Clemens 2022-12-16 15:33:31 UTC
(In reply to Marcel de Rooy from comment #7)
> Having issues with the background jobs here. The staged file job is stuck on
> New status. Restarted stuff etc. No change at all. Logs dont make me wiser.
> Please confirm if it still works..

I am able to stage and import without issue
Comment 9 Marcel de Rooy 2023-01-10 15:19:51 UTC
(In reply to Nick Clemens from comment #8)
> (In reply to Marcel de Rooy from comment #7)
> > Having issues with the background jobs here. The staged file job is stuck on
> > New status. Restarted stuff etc. No change at all. Logs dont make me wiser.
> > Please confirm if it still works..
> 
> I am able to stage and import without issue

Resolved that now ;) background_jobs..
Comment 10 Marcel de Rooy 2023-01-10 15:20:40 UTC
We could go ahead but we could also try here to remove the last occurrence. What do you think? See comment6.
Comment 11 Nick Clemens 2023-01-13 11:57:18 UTC
(In reply to Marcel de Rooy from comment #10)
> We could go ahead but we could also try here to remove the last occurrence.
> What do you think? See comment6.

The real fix there is to remove that script and use API to build the datatables - we could update it to remove the use, but it feels like it would be double work. Either way, I think that should be on its own report
Comment 12 Marcel de Rooy 2023-01-17 15:25:45 UTC
Created attachment 145360 [details] [review]
Bug 32054: Add get_import_record_matches object method and use it

Thispatch adds the new method and alters addorderiso2907.pl to use this
rather than GetRecordImportMatches

To test:
 1 - Import the attached record several times
 2 - Set up a matching rule:
    TitleAuthor threshold: 100
    Matchpoint:
    search index: title, score: 100, tag: 245$a
    search index: author, score: 100, tag:100$a
 3 - Edit one of the imported records to have a different author
 4 - Stage the file again. and match using the matchpoint above
 5 - Note that matches are found and listed on batch management, with the lowest scored match last
 6 - Choose that match
 7 - In acquisitions, add to a basket from the staged file
 8 - Check the box for the record
 9 - Note the match lists the biblionumber for the highest scoring match, not the chosen one
10 - Add an order and note it is for the wrong biblio
11 - Appy patch
12 - Restart_all
13 - Stage the file again and choose a lower scoring match
14 - Confirm when adding to basket this match is preserved
15 - Complete order and verify correct biblio ordered
16 - Stage again, select no match
17 - Confirm no match listed when adding to basket, and choose 'Do not look for matching records' while adding
18 - Confirm order is created on a new biblio

Signed-off-by: David Nind <david@davidnind.com>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 13 Martin Renvoize 2023-01-18 16:00:22 UTC
Nice work everyone!

Pushed to 23.05.x for the next release
Comment 14 Jacob O'Mara 2023-01-18 17:21:27 UTC
Nice work, thanks everyone!

Pushed to 22.11.x for the next release.
Comment 15 Nick Clemens 2023-02-22 19:56:56 UTC Comment hidden (obsolete)