Created attachment 94032 [details] [review] bug23800 : fix regression 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 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 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 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 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 Oh using order_by on all barcodes may be a performance issue. See if we can change this code. 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. *** Bug 23496 has been marked as a duplicate of this bug. *** The patch I attached also fixes bug 23800. 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> Looking here 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.
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> 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> (In reply to Jonathan Druart from comment #8) > Code is duplicated, but refactoring is out of the scope. ;) (In reply to Marcel de Rooy from comment #15) > The solution for items does not work, since it compares barcodes. items => itemnumbers + my @barcodes = grep { exists $itemdata{lc $_} } @contentlist;
THis line is even useless?
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> Nice work everyone! Pushed to master for 20.05 Pushed to 19.11.01 Having some trouble rebasing this for 19.05.x, if this is needed in 19.05 could a rebase be provided? |
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).