Bug 23800 - Batch modification tool orders items by barcode incremental by default (regression to 17.11)
Summary: Batch modification tool orders items by barcode incremental by default (regre...
Status: RESOLVED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Cataloging (show other bugs)
Version: master
Hardware: All All
: P5 - low normal (vote)
Assignee: Jonathan Druart
QA Contact: Marcel de Rooy
URL:
Keywords:
: 23496 (view as bug list)
Depends on:
Blocks:
 
Reported: 2019-10-11 09:31 UTC by Arthur Suzuki
Modified: 2020-06-04 20:38 UTC (History)
11 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Small patch
Who signed the patch off:
Text to go in the release notes:
Version(s) released in:
20.05.00, 19.11.01


Attachments
Screen capture (94.53 KB, application/vnd.openxmlformats-officedocument.wordprocessingml.document)
2019-10-11 09:31 UTC, Arthur Suzuki
Details
bug23800 : fix regression (1.36 KB, patch)
2019-10-11 21:50 UTC, Arthur Suzuki
Details | Diff | Splinter Review
bug23800 : fix regression in batch item modification tool (1.37 KB, patch)
2019-10-11 22:24 UTC, Arthur Suzuki
Details | Diff | Splinter Review
bug23800 : fix regression in batch item modification tool (1.37 KB, patch)
2019-10-11 22:27 UTC, Arthur Suzuki
Details | Diff | Splinter Review
Bug 23800: fix regression in batch item modification tool (1.70 KB, patch)
2019-10-16 18:58 UTC, Arthur Suzuki
Details | Diff | Splinter Review
Bug 23800: fix regression in batch item modification tool (1.70 KB, patch)
2019-10-16 19:01 UTC, Arthur Suzuki
Details | Diff | Splinter Review
[ALTERNATIVE-PATCH] Bug 23800: Does not order items by barcode in batch item modification (3.94 KB, patch)
2019-11-11 10:14 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 23800: Does not order items by barcode in batch item modification (4.00 KB, patch)
2019-11-21 14:32 UTC, ByWater Sandboxes
Details | Diff | Splinter Review
Bug 23800: Does not order items by barcode in batch item modification (4.10 KB, patch)
2019-12-13 09:53 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 23800: (QA follow-up) Fix item case, reduce code (4.45 KB, patch)
2019-12-13 09:53 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 23800: (QA follow-up) Remove useless line (1.18 KB, patch)
2019-12-13 10:04 UTC, Marcel de Rooy
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Arthur Suzuki 2019-10-11 09:31:56 UTC
Created attachment 93987 [details]
Screen capture

When using the batch modification tool and entering barcodes in a list,
the screen which shows the different items matched is sorted by barcode ordered incrementally.
It's quite inconvenient for a librarian who have a pile of book on his desk with a different order (which was just scanned).
Comment 1 Arthur Suzuki 2019-10-11 21:50:15 UTC
Created attachment 94032 [details] [review]
bug23800 : fix regression
Comment 2 Arthur Suzuki 2019-10-11 21:51:06 UTC
    bug23800 : fix regression in batch item modification tool
    
    Test plan :
    1/ scan several items in the Tools / Batch Item Modification
    2/ click continue
    3/ check that items were not displayed in the order they appeared in
    4/ apply patch
    5/ repeat step 1 and 2
    6/ verify that items now appear in the order they were scanned
Comment 3 Arthur Suzuki 2019-10-11 22:24:55 UTC
Created attachment 94033 [details] [review]
bug23800 : fix regression in batch item modification tool

Test plan :
1/ scan several items in the Tools / Batch Item Modification
2/ click continue
3/ check that items were not displayed in the order they appeared in
4/ apply patch
5/ repeat step 1 and 2
6/ verify that items now appear in the order they were scanned
Comment 4 Arthur Suzuki 2019-10-11 22:27:51 UTC
Created attachment 94034 [details] [review]
bug23800 : fix regression in batch item modification tool

Test plan :
1/ scan several items in the Tools / Batch Item Modification
2/ click continue
3/ check that items were not displayed in the order they appeared in
4/ apply patch
5/ repeat step 1 and 2
6/ verify that items now appear in the order they were scanned
Comment 5 Arthur Suzuki 2019-10-16 18:58:50 UTC
Created attachment 94353 [details] [review]
Bug 23800: fix regression in batch item modification tool

Test plan :
1/ scan several items in the Tools / Batch Item Modification
2/ click continue
3/ check that items were not displayed in the order they appeared in
4/ apply patch
5/ repeat step 1 and 2
6/ verify that items now appear in the order they were scanned
Comment 6 Arthur Suzuki 2019-10-16 19:01:17 UTC
Created attachment 94354 [details] [review]
Bug 23800: fix regression in batch item modification tool

Test plan :
1/ scan several items in the Tools / Batch Item Modification
2/ click continue
3/ check that items were not displayed in the order they appeared in
4/ apply patch
5/ repeat step 1 and 2
6/ verify that items now appear in the order they were scanned
Comment 7 Fridolin Somers 2019-11-05 08:37:39 UTC
Oh using order_by on all barcodes may be a performance issue.
See if we can change this code.
Comment 8 Jonathan Druart 2019-11-11 10:14:07 UTC
Created attachment 95252 [details] [review]
[ALTERNATIVE-PATCH] Bug 23800: Does not order items by barcode in batch item modification

They must be displayed in the same order they have been scanned (or they
appear in the file)

This is an alternative patch. Same behavior for barcodes or itemnumbers,
as well as if a file has been used or items scanned.
Code is duplicated, but refactoring is out of the scope.
Comment 9 Jonathan Druart 2019-11-21 11:46:08 UTC
*** Bug 23496 has been marked as a duplicate of this bug. ***
Comment 10 Jonathan Druart 2019-11-21 11:54:51 UTC
The patch I attached also fixes bug 23800.
Comment 11 ByWater Sandboxes 2019-11-21 14:32:52 UTC
Created attachment 95659 [details] [review]
Bug 23800: Does not order items by barcode in batch item modification

They must be displayed in the same order they have been scanned (or they
appear in the file)

This is an alternative patch. Same behavior for barcodes or itemnumbers,
as well as if a file has been used or items scanned.
Code is duplicated, but refactoring is out of the scope.

Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com>
Comment 12 Marcel de Rooy 2019-12-13 07:40:26 UTC
Looking here
Comment 13 Marcel de Rooy 2019-12-13 09:53:13 UTC
IN code part $filecontent eq 'itemid_file', this does not work:
@existing_items = map {
                my $barcode = $_;
                grep { $_->{barcode} eq $barcode ? $_ : () } @existing_items
            } @contentlist;
Here you should compare with itemnumbers !
Resolved in follow-up.
Comment 14 Marcel de Rooy 2019-12-13 09:53:41 UTC
Created attachment 96241 [details] [review]
Bug 23800: Does not order items by barcode in batch item modification

They must be displayed in the same order they have been scanned (or they
appear in the file)

This is an alternative patch. Same behavior for barcodes or itemnumbers,
as well as if a file has been used or items scanned.
Code is duplicated, but refactoring is out of the scope.

Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 15 Marcel de Rooy 2019-12-13 09:53:46 UTC
Created attachment 96242 [details] [review]
Bug 23800: (QA follow-up) Fix item case, reduce code

The solution for items does not work, since it compares barcodes.
Instead of a grep in a map, we could do simpler.

Test plan:
Try barcode file, order not by itemnumber. Toggle with case. Add wrong code.
Try same via barcode list text area.
Try an itemnumber file, reorder again. Add wrong number.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 16 Marcel de Rooy 2019-12-13 09:54:47 UTC
(In reply to Jonathan Druart from comment #8)
> Code is duplicated, but refactoring is out of the scope.

;)
Comment 17 Marcel de Rooy 2019-12-13 09:58:39 UTC
(In reply to Marcel de Rooy from comment #15)
> The solution for items does not work, since it compares barcodes.

items => itemnumbers
Comment 18 Marcel de Rooy 2019-12-13 10:02:15 UTC
+            my @barcodes = grep { exists $itemdata{lc $_} } @contentlist;

THis line is even useless?
Comment 19 Marcel de Rooy 2019-12-13 10:04:25 UTC
Created attachment 96243 [details] [review]
Bug 23800: (QA follow-up) Remove useless line

The lines with my @barcodes makes no sense anymore.

Test plan:
Test with a barcode file.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 20 Martin Renvoize 2019-12-13 15:31:54 UTC
Nice work everyone!

Pushed to master for 20.05
Comment 21 Joy Nelson 2019-12-20 16:19:55 UTC
Pushed to 19.11.01
Comment 22 Lucas Gass 2020-01-06 22:14:43 UTC
Having some trouble rebasing this for 19.05.x, if this is needed in 19.05 could a rebase be provided?