Bug 13540 - Item's permanent_location is set to CART|PROC if an Item is edited when it's location is CART|PROC.
Summary: Item's permanent_location is set to CART|PROC if an Item is edited when it's ...
Status: RESOLVED DUPLICATE of bug 14811
Alias: None
Product: Koha
Classification: Unclassified
Component: Circulation (show other bugs)
Version: Main
Hardware: All All
: P2 critical (vote)
Assignee: Olli-Antti Kivilahti
QA Contact: Testopia
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2015-01-08 17:27 UTC by Olli-Antti Kivilahti
Modified: 2015-11-07 08:15 UTC (History)
5 users (show)

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


Attachments
Bug 13540 - Item's permanent_location is set to CART|PROC if an Item is edited when it's location is CART|PROC. (3.12 KB, patch)
2015-01-08 18:32 UTC, Olli-Antti Kivilahti
Details | Diff | Splinter Review
Bug 13540 - Item's permanent_location is set to CART|PROC when... - Unit tests (6.27 KB, patch)
2015-01-09 12:42 UTC, Olli-Antti Kivilahti
Details | Diff | Splinter Review
Bug 13540 - Item's permanent_location is set to CART|PROC when... - Unit tests (7.00 KB, patch)
2015-01-09 13:07 UTC, Olli-Antti Kivilahti
Details | Diff | Splinter Review
Bug 13540 - Item's permanent_location is set to CART|PROC if an Item is edited when it's location is CART|PROC. (3.17 KB, patch)
2015-01-18 05:17 UTC, Chris Cormack
Details | Diff | Splinter Review
Bug 13540 - Item's permanent_location is set to CART|PROC when... - Unit tests (7.05 KB, patch)
2015-01-18 05:19 UTC, Chris Cormack
Details | Diff | Splinter Review
Bug 13540 - Item's permanent_location is set to CART|PROC when... - Followup 1 (1.02 KB, patch)
2015-02-16 15:52 UTC, Olli-Antti Kivilahti
Details | Diff | Splinter Review
Bug 13540 - Item's permanent_location is set to CART|PROC when... - Followup 1 (1.02 KB, patch)
2015-02-16 15:55 UTC, Olli-Antti Kivilahti
Details | Diff | Splinter Review
Bug 13540 - Item's permanent_location is set to CART|PROC when... - Followup 1 (1.98 KB, patch)
2015-02-16 16:00 UTC, Olli-Antti Kivilahti
Details | Diff | Splinter Review
Bug 13540: Remove the too specific subroutine (1.74 KB, patch)
2015-06-16 13:51 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 13540 - Item's permanent_location is set to CART|PROC if an Item is edited when it's location is CART|PROC. (3.26 KB, patch)
2015-06-17 08:04 UTC, Olli-Antti Kivilahti
Details | Diff | Splinter Review
Bug 13540 - Item's permanent_location is set to CART|PROC when... - Unit tests (7.15 KB, patch)
2015-07-09 11:40 UTC, Katrin Fischer
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Olli-Antti Kivilahti 2015-01-08 17:27:28 UTC
:-: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 ;(
Comment 1 Olli-Antti Kivilahti 2015-01-08 18:32:09 UTC Comment hidden (obsolete)
Comment 2 Olli-Antti Kivilahti 2015-01-08 18:32:51 UTC
Unit tests to follow, gotta go home!
Comment 3 Olli-Antti Kivilahti 2015-01-09 12:42:22 UTC Comment hidden (obsolete)
Comment 4 Olli-Antti Kivilahti 2015-01-09 12:45:50 UTC
Setting importance to Major P2, because this causes real loss of data in a very conspicious manner. Repairing the damage afterwards can be impossible.
Comment 5 Katrin Fischer 2015-01-09 12:48:48 UTC
Hi Olli, we barely use the P* values, I have upped this to critical.
Comment 6 Olli-Antti Kivilahti 2015-01-09 13:07:17 UTC Comment hidden (obsolete)
Comment 7 Chris Cormack 2015-01-18 05:17:55 UTC Comment hidden (obsolete)
Comment 8 Chris Cormack 2015-01-18 05:19:30 UTC
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>
Comment 9 Jonathan Druart 2015-01-20 16:09:28 UTC
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?
Comment 10 Olli-Antti Kivilahti 2015-02-16 15:52:05 UTC Comment hidden (obsolete)
Comment 11 Olli-Antti Kivilahti 2015-02-16 15:55:58 UTC Comment hidden (obsolete)
Comment 12 Olli-Antti Kivilahti 2015-02-16 15:57:36 UTC
Thanks Chris for the sign off!
Thanks Joubu for kicking my ass!
Followup supplied.
Comment 13 Olli-Antti Kivilahti 2015-02-16 16:00:56 UTC
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.
Comment 14 Jonathan Druart 2015-02-16 16:15:44 UTC
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}.
Comment 15 Jonathan Druart 2015-02-16 16:16:39 UTC
You need an empty line avec the =cut

means:
You need to add an empty line avec the '=cut' in the pod.
Comment 16 Jonathan Druart 2015-02-16 16:17:24 UTC
s/avec/after

(hum, I need some fresh air...)
Comment 17 Olli-Antti Kivilahti 2015-02-18 13:27:11 UTC
I created a new subroutine, because I only need the location-data.
Comment 18 Olli-Antti Kivilahti 2015-02-18 13:28:40 UTC
It's a bit like, if one chocolate bar makes you happy, why eat 4?
Comment 19 Jonathan Druart 2015-02-18 14:20:45 UTC
(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.
Comment 20 Katrin Fischer 2015-02-22 15:30:32 UTC
I agree with Jonathan - in this case a new subroutine is probably a bit too specific.
Comment 21 Jonathan Druart 2015-02-26 14:07:25 UTC
(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.
Comment 22 Katrin Fischer 2015-06-09 23:51:06 UTC
Can we get a new patch for this? It's critical :(
Comment 23 Jonathan Druart 2015-06-16 13:51:41 UTC
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
Comment 24 Olli-Antti Kivilahti 2015-06-17 08:04:08 UTC Comment hidden (obsolete)
Comment 25 Olli-Antti Kivilahti 2015-06-17 08:16:39 UTC
Tests are broken... :(
Comment 26 Jonathan Druart 2015-06-17 08:44:11 UTC
(In reply to Olli-Antti Kivilahti from comment #25)
> Tests are broken... :(

Which ones?
I tried t/db_dependent/Circulation.t, and they pass.
Comment 27 Katrin Fischer 2015-07-09 11:40:57 UTC
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>
Comment 28 Katrin Fischer 2015-07-09 11:42:07 UTC
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.
Comment 29 Josef Moravec 2015-11-07 08:15:56 UTC

*** This bug has been marked as a duplicate of bug 14811 ***