|
Description
Olli-Antti Kivilahti
2015-01-08 17:27:28 UTC
Created attachment 35038 [details] [review] Bug 13540 - Item's permanent_location is set to CART|PROC if an Item is edited when it's location is CART|PROC. This bug is rather nasty as it happens very discreetly when editing Items that are in either locations 'CART' or 'PROC'. For example when changing one of the Item's statuses or barcode, the permanent_location gets overwritten without the user noticing it immediately. The damage becomes apparent only weeks later when there are potentially thousands of Items missing the permanent_location and location. :-:TEST SETUP:-: 0a. System preference "ReturnToShelvingCart" must be 'Move'. 0b. Find any Item (I69) 0c. Set I69's location to "Book Cart" (BC) or anything but empty. :-:REPLICATE ISSUE:-: 1. Check-in the I69 and the location should turn to 'CART' (returned today). 2. Go to the "Edit Item"-view for I69, don't change anything, simply click "Save changes" 3. Observe that the permanent_location has changed to 'CART'. This is not good since now the location data is forever lost again (Bugg 7817). The "cart_to_shelf.pl --hours 24"-script cannot return the Item to the proper place ;( :-:AFTER THIS PATCH:-: Repeat steps 1-2. 3. Observe that the permanent_location is still the old one, instead of getting overwritten by 'CART'. Unit tests to follow, gotta go home! Created attachment 35047 [details] [review] Bug 13540 - Item's permanent_location is set to CART|PROC when... - Unit tests Setting importance to Major P2, because this causes real loss of data in a very conspicious manner. Repairing the damage afterwards can be impossible. Hi Olli, we barely use the P* values, I have upped this to critical. Created attachment 35055 [details] [review] Bug 13540 - Item's permanent_location is set to CART|PROC when... - Unit tests Created attachment 35336 [details] [review] Bug 13540 - Item's permanent_location is set to CART|PROC if an Item is edited when it's location is CART|PROC. This bug is rather nasty as it happens very discreetly when editing Items that are in either locations 'CART' or 'PROC'. For example when changing one of the Item's statuses or barcode, the permanent_location gets overwritten without the user noticing it immediately. The damage becomes apparent only weeks later when there are potentially thousands of Items missing the permanent_location and location. :-:TEST SETUP:-: 0a. System preference "ReturnToShelvingCart" must be 'Move'. 0b. Find any Item (I69) 0c. Set I69's location to "Book Cart" (BC) or anything but empty. :-:REPLICATE ISSUE:-: 1. Check-in the I69 and the location should turn to 'CART' (returned today). 2. Go to the "Edit Item"-view for I69, don't change anything, simply click "Save changes" 3. Observe that the permanent_location has changed to 'CART'. This is not good since now the location data is forever lost again (Bugg 7817). The "cart_to_shelf.pl --hours 24"-script cannot return the Item to the proper place ;( :-:AFTER THIS PATCH:-: Repeat steps 1-2. 3. Observe that the permanent_location is still the old one, instead of getting overwritten by 'CART'. Signed-off-by: Chris Cormack <chris@bigballofwax.co.nz> Created attachment 35337 [details] [review] Bug 13540 - Item's permanent_location is set to CART|PROC when... - Unit tests Signed-off-by: Chris Cormack <chris@bigballofwax.co.nz> Looks like something is wrong for me: % prove t/db_dependent/Circulation.t t/db_dependent/Circulation.t .. 23/67 # Failed test 'ReturnToShelvingCart: Adding a new Item with location also sets the permanent_location.' # at t/db_dependent/Circulation.t line 648. # Looks like you failed 1 test of 67. Does it come from my DB? Created attachment 35951 [details] [review] Bug 13540 - Item's permanent_location is set to CART|PROC when... - Followup 1 To fix the failing test "ReturnToShelvingCart: Adding a new Item with location also sets the permanent_location." When adding an Item, set the permanent_location to location if present. This behaviour is consistent with ModItem where permanent_location is set to location. Created attachment 35952 [details] [review] Bug 13540 - Item's permanent_location is set to CART|PROC when... - Followup 1 To fix the failing test "ReturnToShelvingCart: Adding a new Item with location also sets the permanent_location." When adding an Item, set the permanent_location to location if present. This behaviour is consistent with ModItem where permanent_location is set to location. Thanks Chris for the sign off! Thanks Joubu for kicking my ass! Followup supplied. Created attachment 35953 [details] [review] Bug 13540 - Item's permanent_location is set to CART|PROC when... - Followup 1 To fix the failing test "ReturnToShelvingCart: Adding a new Item with location also sets the permanent_location." When adding an Item, set the permanent_location to location if present. This behaviour is consistent with ModItem where permanent_location is set to location. 1/ FAIL C4/Items.pm
FAIL pod
*** ERROR: Unknown command 'head' in file C4/Items.pm
*** ERROR: Spurious text after =cut in file C4/Items.pm
You need an empty line avec the =cut
2/ Please prefer creating a subtest (`git grep subtest t` for examples), instead of a sub, in the test file.
3/ Why do you create a new routine?
IMO you should use C4::Items::GetItem($itemnumber)->{permanent_location}.
You need an empty line avec the =cut means: You need to add an empty line avec the '=cut' in the pod. s/avec/after (hum, I need some fresh air...) I created a new subroutine, because I only need the location-data. It's a bit like, if one chocolate bar makes you happy, why eat 4? (In reply to Olli-Antti Kivilahti from comment #17) > I created a new subroutine, because I only need the location-data. It's another subroutine to maintain, I am not sure it's a good idea. I agree with Jonathan - in this case a new subroutine is probably a bit too specific. (In reply to Katrin Fischer from comment #20) > I agree with Jonathan - in this case a new subroutine is probably a bit too > specific. So, failing QA. Can we get a new patch for this? It's critical :( Created attachment 40183 [details] [review] Bug 13540: Remove the too specific subroutine This patch replaces the too specific GetRealItemLocations subroutine with a call to GetItem Created attachment 40214 [details] [review] Bug 13540 - Item's permanent_location is set to CART|PROC if an Item is edited when it's location is CART|PROC. This bug is rather nasty as it happens very discreetly when editing Items that are in either locations 'CART' or 'PROC'. For example when changing one of the Item's statuses or barcode, the permanent_location gets overwritten without the user noticing it immediately. The damage becomes apparent only weeks later when there are potentially thousands of Items missing the permanent_location and location. :-:TEST SETUP:-: 0a. System preference "ReturnToShelvingCart" must be 'Move'. 0b. Find any Item (I69) 0c. Set I69's location to "Book Cart" (BC) or anything but empty. :-:REPLICATE ISSUE:-: 1. Check-in the I69 and the location should turn to 'CART' (returned today). 2. Go to the "Edit Item"-view for I69, don't change anything, simply click "Save changes" 3. Observe that the permanent_location has changed to 'CART'. This is not good since now the location data is forever lost again (Bugg 7817). The "cart_to_shelf.pl --hours 24"-script cannot return the Item to the proper place ;( :-:AFTER THIS PATCH:-: Repeat steps 1-2. 3. Observe that the permanent_location is still the old one, instead of getting overwritten by 'CART'. Signed-off-by: Chris Cormack <chris@bigballofwax.co.nz> Signed-off-by: Olli-Antti Kivilahti <olli-antti.kivilahti@jns.fi> Working in prod for ~6 months. Tests are broken... :( (In reply to Olli-Antti Kivilahti from comment #25) > Tests are broken... :( Which ones? I tried t/db_dependent/Circulation.t, and they pass. Created attachment 40878 [details] [review] Bug 13540 - Item's permanent_location is set to CART|PROC when... - Unit tests Signed-off-by: Chris Cormack <chris@bigballofwax.co.nz> I have rebased the last patch hopefully correctly and fixed problems with the POD in the test file noted by the QA script. But: $ prove t/db_dependent/Circulation.t t/db_dependent/Circulation.t .. 25/69 # Failed test 'ReturnToShelvingCart: Modifying the whole Item doesn't overwrite permanent_location!' # at t/db_dependent/Circulation.t line 684. # Failed test 'ReturnToShelvingCart: Adding a new Item with location also sets the permanent_location.'$ prove t/db_dependent/Circulation.t t/db_dependent/Circulation.t .. 25/69 # Failed test 'ReturnToShelvingCart: Modifying the whole Item doesn't overwrite permanent_location!' # at t/db_dependent/Circulation.t line 684. # Failed test 'ReturnToShelvingCart: Adding a new Item with location also sets the permanent_location.' # at t/db_dependent/Circulation.t line 704. # Looks like you failed 2 tests of 69. t/db_dependent/Circulation.t .. Dubious, test returned 2 (wstat 512, 0x200) Failed 2/69 subtests Test Summary Report ------------------- t/db_dependent/Circulation.t (Wstat: 512 Tests: 69 Failed: 2) Failed tests: 67, 69 Non-zero exit status: 2 Files=1, Tests=69, 2 wallclock secs ( 0.03 usr 0.00 sys + 1.50 cusr 0.08 csys = 1.61 CPU) Result: FAIL # at t/db_dependent/Circulation.t line 704. # Looks like you failed 2 tests of 69. t/db_dependent/Circulation.t .. Dubious, test returned 2 (wstat 512, 0x200) Failed 2/69 subtests Test Summary Report ------------------- t/db_dependent/Circulation.t (Wstat: 512 Tests: 69 Failed: 2) Failed tests: 67, 69 Non-zero exit status: 2 Files=1, Tests=69, 2 wallclock secs ( 0.03 usr 0.00 sys + 1.50 cusr 0.08 csys = 1.61 CPU) Result: FAIL Leaving this in Failed QA for now - but would love to see this fixed. *** This bug has been marked as a duplicate of bug 14811 *** |