Bug 14576

Summary: Allow automatic update of location on checkin
Product: Koha Reporter: Jesse Weaver <jweaver>
Component: CirculationAssignee: Nick Clemens <nick>
Status: CLOSED FIXED QA Contact: Katrin Fischer <katrin.fischer>
Severity: enhancement    
Priority: P5 - low CC: 1joynelson, alexbuckley, amy, black23, clackman, claire_gravely, cmurdock, david, fraktik, gmcharlt, hayleypelham, jonathan.druart, josef.moravec, katrin.fischer, kyle.m.hall, lisa, m.de.rooy, marjorie.barry-vila, martin.renvoize, nicole, rkuiper, veron, wizzyrea
Version: Main   
Hardware: All   
OS: All   
See Also: http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=11629
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=17104
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=21159
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28472
Change sponsored?: Sponsored Patch complexity: Medium patch
Documentation contact: Documentation submission:
Text to go in the release notes:
This enhancement adds a new system preference "UpdateItemLocationOnCheckin" which accepts pairs of shelving locations. On check in the item's location is compared to the location on the left and, if it matches, is updated to the location on the right. This preference replaces the ReturnToShelvingCart and InProcessingToShelvingCart preferences. Note that existing functionality for all items in the PROC location being returned to permanent_location is preserved by default. Also, any items issued from the CART location will be returned to their permanent location on check out (if it differs). Special values for this system preference are: _ALL_ - used on left side only to affect all items, and overrides all other rules _BLANK_ - used on either side to match on or set to blank (actual blanks will work, but this is an easier to read option) _PERM_ - used on right side only to return items to their permanent location Syntax highlighting is used in the text area to make it easier to read.
Version(s) released in:
19.05.00
Bug Depends on: 19726    
Bug Blocks: 16698, 21159, 21502, 22960    
Attachments: Bug 14576 - Allow arbitrary automatic update of location on checkin
Bug 14576 - Allow arbitrary automatic update of location on checkin
Bug 14576 - Unit tests
Bug 14576 - Allow arbitrary automatic update of location on checkin
Bug 14576 - Unit tests
Bug 14576 - Allow arbitrary automatic update of location on checkin
Bug 14576 - Unit tests
Bug 14576 - Allow arbitrary automatic update of location on checkin
Bug 14576 - Unit tests
Bug 14576 - (QA followup) Fix message for use of value not in authorized values
Bug 14576 - Allow arbitrary automatic update of location on checkin
Bug 14576 - Unit tests
Bug 14576 - (QA followup) Fix message for use of value not in authorized values
Bug 14576 - Allow arbitrary automatic update of location on checkin
Bug 14576 - Unit tests
Bug 14576 - (QA followup) Fix message for use of value not in authorized values
Bug 14576 (QA Folllowup) Address issues
Bug 14576 - Allow arbitrary automatic update of location on checkin
Bug 14576 - Unit tests
Bug 14576 - (QA followup) Fix message for use of value not in authorized values
Bug 14576 (QA Folllowup) Address issues
Bug 14576 - (QA Followup) Remove other instances of ReturnToShelvingCart/InprocessingToShelvingCart
Bug 14576 - Allow arbitrary automatic update of location on checkin
Bug 14576 - Unit tests
Bug 14576 - (QA followup) Fix message for use of value not in authorized values
Bug 14576 (QA Folllowup) Address issues
Bug 14576 - (QA Followup) Remove other instances of ReturnToShelvingCart/InprocessingToShelvingCart
Bug 14576 - (QA Followup) Remove other instances of ReturnToShelvingCart/InprocessingToShelvingCart
Bug 14576 (QA Followup) Fix undelcared use of $item Move strings from js files Delete duplicated lines
Bug 14576 - Allow arbitrary automatic update of location on checkin
Bug 14576 - Unit tests
Bug 14576 - (QA followup) Fix message for use of value not in authorized values
Bug 14576 (QA Folllowup) Address issues
Bug 14576 - (QA Followup) Remove other instances of ReturnToShelvingCart/InprocessingToShelvingCart
Bug 14576 (QA Followup) Fix undelcared use of $item Move strings from js files Delete duplicated lines
Bug 14576 (QA Followup) Remove C4:::Items::ShelfToCart
Bug 14576 (QA Followup) Fix issue.t
Bug 14576 - (QA Followup) Fix Returns.t
Bug 14576 - Allow arbitrary automatic update of location on checkin
Bug 14576 - Unit tests
Bug 14576 - (QA followup) Fix message for use of value not in authorized values
Bug 14576 (QA Folllowup) Address issues
Bug 14576 - (QA Followup) Remove other instances of ReturnToShelvingCart/InprocessingToShelvingCart
Bug 14576 (QA Followup) Fix undelcared use of $item Move strings from js files Delete duplicated lines
Bug 14576 (QA Followup) Remove C4:::Items::ShelfToCart
Bug 14576 (QA Followup) Fix issue.t
Bug 14576 - (QA Followup) Fix Returns.t
Bug 14576 (QA Followup) Fix warnings in tests
Bug 14576 - Allow arbitrary automatic update of location on checkin
Bug 14576 - Unit tests
Bug 14576 - (QA followups squashed)
Bug 14576 - Allow arbitrary automatic update of location on checkin
Bug 14576: Unit tests
Bug 14576: Followup
Bug 14576 - Allow arbitrary automatic update of location on checkin
Bug 14576: Unit tests
Bug 14576: Followup
Bug 14576 - Allow arbitrary automatic update of location on checkin
Bug 14576: Unit tests
Bug 14576: Followup
Bug 14576: Allow arbitrary automatic update of location on checkin
Bug 14576: Unit tests
Bug 14576: (follow-up)
Bug 14576: Allow arbitrary automatic update of location on checkin
Bug 14576: Unit tests
Bug 14576: Allow arbitrary automatic update of location on checkin
Bug 14576: Unit tests
Bug 14576: Allow arbitrary automatic update of location on checkin
Bug 14576: Unit tests
Bug 14576: Update to use get_yaml_pref_hash and perl update statement
Bug 14576: Allow arbitrary automatic update of location on checkin
Bug 14576: Unit tests
Bug 14576: Update to use get_yaml_pref_hash
Bug 14576: (follow-up) fix some typos and style
Bug 14576: Allow arbitrary automatic update of location on checkin
Bug 14576: Unit tests
Bug 14576: Update to use get_yaml_pref_hash
Bug 14576: (follow-up) fix some typos and style
Bug 14576: Allow arbitrary automatic update of location on checkin
Bug 14576: Unit tests
Bug 14576: Update to use get_yaml_pref_hash
Bug 14576: (follow-up) fix some typos and style
Bug 14576: Rebase fixes Fix test Rebase code
Bug 14576: Replaced calls using deprecated method GetItem with Koha::Items->find
Bug 14576: (follow-up) Using item objects, not hashrefs
Bug 14576: Allow arbitrary automatic update of location on checkin
Bug 14576: Unit tests
Bug 14576: Update to use get_yaml_pref_hash
Bug 14576: (follow-up) fix some typos and style
Bug 14576: Rebase fixes Fix test Rebase code
Bug 14576: Replaced calls using deprecated method GetItem with Koha::Items->find
Bug 14576: (follow-up) Using item objects, not hashrefs
Bug 14576: (QA Follow up) Tidy up feature documentation
Bug 14576: (QA follow-up) Tidy up feature documentation
Bug 14576: Allow arbitrary automatic update of location on checkin
Bug 14576: Unit tests
Bug 14576: Update to use get_yaml_pref_hash
Bug 14576: (follow-up) fix some typos and style
Bug 14576: Rebase fixes Fix test Rebase code
Bug 14576: Replaced calls using deprecated method GetItem with Koha::Items->find
Bug 14576: (follow-up) Using item objects, not hashrefs
Bug 14576: (QA follow-up) Tidy up feature documentation
Bug 14576: (follow-up) Add code highlighting to system preference UpdateItemLocationOnCheckin
Bug 14576: Allow arbitrary automatic update of location on checkin
Bug 14576: Unit tests
Bug 14576: Update to use get_yaml_pref_hash
Bug 14576: (follow-up) fix some typos and style
Bug 14576: Rebase fixes Fix test Rebase code
Bug 14576: Replaced calls using deprecated method GetItem with Koha::Items->find
Bug 14576: (follow-up) Using item objects, not hashrefs
Bug 14576: (QA follow-up) Tidy up feature documentation
Bug 14576: (follow-up) Add code highlighting to system preference UpdateItemLocationOnCheckin

Description Jesse Weaver 2015-07-21 03:34:56 UTC Comment hidden (obsolete)
Comment 1 Nick Clemens 2016-01-08 19:46:52 UTC Comment hidden (obsolete)
Comment 2 Nick Clemens 2016-01-08 20:51:37 UTC Comment hidden (obsolete)
Comment 3 Nick Clemens 2016-01-08 20:51:45 UTC Comment hidden (obsolete)
Comment 4 Owen Leonard 2016-01-15 16:49:35 UTC Comment hidden (obsolete)
Comment 5 Nick Clemens 2016-02-19 22:03:23 UTC Comment hidden (obsolete)
Comment 6 Nick Clemens 2016-02-19 22:03:31 UTC Comment hidden (obsolete)
Comment 7 Owen Leonard 2016-03-04 15:32:16 UTC Comment hidden (obsolete)
Comment 8 Nick Clemens 2016-04-25 17:09:59 UTC Comment hidden (obsolete)
Comment 9 Nick Clemens 2016-04-25 17:10:04 UTC Comment hidden (obsolete)
Comment 10 Nick Clemens 2016-06-09 13:49:16 UTC Comment hidden (obsolete)
Comment 11 Nick Clemens 2016-06-09 13:49:20 UTC Comment hidden (obsolete)
Comment 12 Nick Clemens 2016-06-09 13:49:25 UTC Comment hidden (obsolete)
Comment 13 Nicole C. Engard 2016-06-09 19:17:07 UTC Comment hidden (obsolete)
Comment 14 Nicole C. Engard 2016-06-09 19:17:15 UTC Comment hidden (obsolete)
Comment 15 Nicole C. Engard 2016-06-09 19:17:20 UTC Comment hidden (obsolete)
Comment 16 Kyle M Hall 2016-06-10 11:41:03 UTC Comment hidden (obsolete)
Comment 17 Kyle M Hall 2016-06-10 11:41:11 UTC Comment hidden (obsolete)
Comment 18 Kyle M Hall 2016-06-10 11:41:14 UTC Comment hidden (obsolete)
Comment 19 Jonathan Druart 2016-06-13 17:56:14 UTC Comment hidden (obsolete)
Comment 20 Nick Clemens 2016-06-14 10:32:16 UTC Comment hidden (obsolete)
Comment 21 Jonathan Druart 2016-07-02 12:54:14 UTC Comment hidden (obsolete)
Comment 22 Nick Clemens 2016-07-05 19:14:06 UTC Comment hidden (obsolete)
Comment 23 Nick Clemens 2016-07-05 19:14:12 UTC Comment hidden (obsolete)
Comment 24 Nick Clemens 2016-07-05 19:14:17 UTC Comment hidden (obsolete)
Comment 25 Nick Clemens 2016-07-05 19:14:22 UTC Comment hidden (obsolete)
Comment 26 Nick Clemens 2016-07-05 19:22:17 UTC Comment hidden (obsolete)
Comment 27 Jonathan Druart 2016-08-01 15:40:45 UTC Comment hidden (obsolete)
Comment 28 Nick Clemens 2016-08-10 19:54:59 UTC Comment hidden (obsolete)
Comment 29 Jonathan Druart 2016-08-19 14:12:33 UTC Comment hidden (obsolete)
Comment 30 Nick Clemens 2016-08-19 15:29:46 UTC Comment hidden (obsolete)
Comment 31 Nick Clemens 2016-08-19 15:29:52 UTC Comment hidden (obsolete)
Comment 32 Nick Clemens 2016-08-19 15:29:57 UTC Comment hidden (obsolete)
Comment 33 Nick Clemens 2016-08-19 15:30:04 UTC Comment hidden (obsolete)
Comment 34 Nick Clemens 2016-08-19 15:30:10 UTC Comment hidden (obsolete)
Comment 35 Nick Clemens 2016-08-19 15:31:34 UTC Comment hidden (obsolete)
Comment 36 Nick Clemens 2016-08-19 16:41:48 UTC Comment hidden (obsolete)
Comment 37 Katrin Fischer 2016-08-28 23:12:44 UTC Comment hidden (obsolete)
Comment 38 Nick Clemens 2016-09-15 14:12:26 UTC Comment hidden (obsolete)
Comment 39 Nick Clemens 2016-09-15 14:24:27 UTC Comment hidden (obsolete)
Comment 40 Owen Leonard 2016-10-21 15:13:44 UTC Comment hidden (obsolete)
Comment 41 Nick Clemens 2017-02-24 21:42:00 UTC Comment hidden (obsolete)
Comment 42 Nick Clemens 2017-02-24 21:42:07 UTC Comment hidden (obsolete)
Comment 43 Nick Clemens 2017-02-24 21:42:13 UTC Comment hidden (obsolete)
Comment 44 Nick Clemens 2017-02-24 21:42:19 UTC Comment hidden (obsolete)
Comment 45 Nick Clemens 2017-02-24 21:42:24 UTC Comment hidden (obsolete)
Comment 46 Nick Clemens 2017-02-24 21:42:30 UTC Comment hidden (obsolete)
Comment 47 Jonathan Druart 2017-02-27 10:45:55 UTC Comment hidden (obsolete)
Comment 48 Nick Clemens 2017-03-01 02:05:10 UTC Comment hidden (obsolete)
Comment 49 Nick Clemens 2017-03-01 02:05:18 UTC Comment hidden (obsolete)
Comment 50 Nick Clemens 2017-03-01 02:07:27 UTC Comment hidden (obsolete)
Comment 51 Nick Clemens 2017-03-01 10:41:41 UTC Comment hidden (obsolete)
Comment 52 Jonathan Druart 2017-03-29 19:10:44 UTC Comment hidden (obsolete)
Comment 53 Nick Clemens 2017-04-06 15:36:59 UTC Comment hidden (obsolete)
Comment 54 Nick Clemens 2017-04-06 15:37:08 UTC Comment hidden (obsolete)
Comment 55 Nick Clemens 2017-04-06 15:37:31 UTC Comment hidden (obsolete)
Comment 56 Nick Clemens 2017-04-06 15:37:39 UTC Comment hidden (obsolete)
Comment 57 Nick Clemens 2017-04-06 15:37:46 UTC Comment hidden (obsolete)
Comment 58 Nick Clemens 2017-04-06 15:37:53 UTC Comment hidden (obsolete)
Comment 59 Nick Clemens 2017-04-06 15:38:01 UTC Comment hidden (obsolete)
Comment 60 Nick Clemens 2017-04-06 15:38:08 UTC Comment hidden (obsolete)
Comment 61 Nick Clemens 2017-04-06 15:38:14 UTC Comment hidden (obsolete)
Comment 62 Nick Clemens 2017-04-06 15:38:21 UTC Comment hidden (obsolete)
Comment 63 Marcel de Rooy 2017-06-16 10:17:28 UTC Comment hidden (obsolete)
Comment 64 Nick Clemens 2017-06-26 15:22:55 UTC Comment hidden (obsolete)
Comment 65 Nick Clemens 2017-06-26 15:23:00 UTC Comment hidden (obsolete)
Comment 66 Nick Clemens 2017-06-26 15:23:05 UTC Comment hidden (obsolete)
Comment 67 Nick Clemens 2017-10-04 15:24:15 UTC Comment hidden (obsolete)
Comment 68 Nick Clemens 2017-10-04 15:24:19 UTC Comment hidden (obsolete)
Comment 69 Alex Buckley 2017-10-05 22:22:26 UTC
Hi Nick

A couple of points I have found whilst testing your first patch;

1) Can you clarify the _ALL_ value because when I set this on the left hand side e.g 
_ALL_: FIC
FIC: GEN

The only item that was set to FIC was the item was was checking in, not all items.

2) I entered incorrect syntax such as !!! into the pref and I was not warned instead this invalid syntax was saved
Comment 70 Nick Clemens 2017-10-06 10:34:24 UTC
(In reply to Alex Buckley from comment #69)
> 1) Can you clarify the _ALL_ value because when I set this on the left hand
> side e.g /_ALL_: FIC/ FIC: GEN
> The only item that was set to FIC was the item was was checking in, not all
> items.
That is the correct behaviour - all items that are checked in, meaning regardless of original location, set the location to 'FIC' (it does NOT mean 'all items on the record')

> 2) I entered incorrect syntax such as !!! into the pref and I was not warned
> instead this invalid syntax was saved
Hmm..I am warned everytime - but you are only warned on change of pref and not prevented from saving. On bad data the pref will simply not work any more and a warn in the logs. I think this should be sufficient - the text is very explanatory, the warning should give you pause, and it only works if set right
Comment 71 Katrin Fischer 2017-10-14 23:32:43 UTC Comment hidden (obsolete)
Comment 72 Nick Clemens 2017-10-19 13:51:22 UTC Comment hidden (obsolete)
Comment 73 Nick Clemens 2017-10-19 13:52:18 UTC
(In reply to Katrin Fischer from comment #71)
> With the patch applied the message "Not checked out." appears twice when
> returning an item that has not been checked out.
Ah, rebase duplication, cleared

> QA tools also highlight some issues:
Fixed, pod coverage is false positive (I remove POD because I remove a function)
Comment 74 Katrin Fischer 2017-12-28 07:09:08 UTC
Hi Nick, can you please rebase? JavaScript on the preferences template has since be moved to the bottom. Thx!
Comment 75 Nick Clemens 2018-01-22 13:30:33 UTC Comment hidden (obsolete)
Comment 76 Nick Clemens 2018-01-22 13:30:38 UTC Comment hidden (obsolete)
Comment 77 Nick Clemens 2018-01-22 13:30:42 UTC Comment hidden (obsolete)
Comment 78 Nick Clemens 2018-02-21 16:44:28 UTC Comment hidden (obsolete)
Comment 79 Nick Clemens 2018-02-21 16:44:33 UTC Comment hidden (obsolete)
Comment 80 Nick Clemens 2018-02-21 16:44:37 UTC Comment hidden (obsolete)
Comment 81 Nick Clemens 2018-05-30 13:41:52 UTC
Created attachment 75661 [details] [review]
Bug 14576: Allow arbitrary automatic update of location on checkin

This patch adds a new syspref "UpdateItemLocationOnCheckin" which
accepts pairs of shelving locations.  On check-in the items location is
compared ot the location on the left and, if it matches, is updated to
the location on the left.

This preference replaces ReturnToShelvingCart and
InProcessingToShelvingCart preferences.  The update statement should
insert values that replciate these functions.  Note existing
functionality of all items in PROC location being returned to
permanent_location is preserved by default.  Also, any items issued from
CART location will be returned to their permanent location on issue (if
it differs)

Special values for this pref are:
_ALL_ - used on left side only to affect all items
_BLANK_ - used on either side to match on/set to blank (actual blanks
        will work, but this is an easier to read option)
_PERM_ - used on right side only to return items to permanent location

Test Plan:
1) Apply this patch
2) Run updatedatabase.pl
3) Set the new system preference UpdateitemLocationOnCheckin
   to the following (assuming sample data):
   NEW: FIC
   FIC: GEN
4) Create an item, set its location to NEW
5) Check in the item, note its location is now FIC
6) Check in the item again, note its location is now GEN
7) Check in the item again, note its location remains GEN
8) Test using _ALL_, _BLANK_ and _PERM_ for updates
9) Try entering various incorrect syntax in the pref and note you are warned

Sponsored by:
    Arcadia Public Library (http://library.ci.arcadia.ca.us/)
    Middletown Township Public Library (http://www.mtpl.org/)
    Round Rock Public Library (https://www.roundrocktexas.gov/departments/library/)
Comment 82 Nick Clemens 2018-05-30 13:41:57 UTC
Created attachment 75662 [details] [review]
Bug 14576: Unit tests
Comment 83 Nick Clemens 2018-05-30 13:42:02 UTC
Created attachment 75663 [details] [review]
Bug 14576: (follow-up)

Fix (remove) extra call for item
Fix rebase error code duplication
Comment 84 Claire Gravely 2018-08-22 12:12:41 UTC
does not apply :(

M	C4/Circulation.pm
M	C4/Items.pm
M	C4/Reserves.pm
M	C4/UsageStats.pm
M	circ/returns.pl
M	installer/data/mysql/sysprefs.sql
M	koha-tmpl/intranet-tmpl/prog/en/modules/admin/preferences.tt
M	koha-tmpl/intranet-tmpl/prog/en/modules/admin/preferences/circulation.pref
M	koha-tmpl/intranet-tmpl/prog/en/modules/circ/returns.tt
M	koha-tmpl/intranet-tmpl/prog/en/modules/help/admin/authorised_values.tt
Falling back to patching base and 3-way merge...
Auto-merging koha-tmpl/intranet-tmpl/prog/en/modules/help/admin/authorised_values.tt
Auto-merging koha-tmpl/intranet-tmpl/prog/en/modules/circ/returns.tt
CONFLICT (content): Merge conflict in koha-tmpl/intranet-tmpl/prog/en/modules/circ/returns.tt
Auto-merging koha-tmpl/intranet-tmpl/prog/en/modules/admin/preferences/circulation.pref
Auto-merging koha-tmpl/intranet-tmpl/prog/en/modules/admin/preferences.tt
Auto-merging installer/data/mysql/sysprefs.sql
Auto-merging circ/returns.pl
Auto-merging C4/UsageStats.pm
Auto-merging C4/Reserves.pm
Auto-merging C4/Items.pm
Auto-merging C4/Circulation.pm
Failed to merge in the changes.
Patch failed at 0001 Bug 14576: Allow arbitrary automatic update of location on checkin
Comment 85 Nick Clemens 2018-09-21 17:59:14 UTC
Created attachment 79245 [details] [review]
Bug 14576: Allow arbitrary automatic update of location on checkin

This patch adds a new syspref "UpdateItemLocationOnCheckin" which
accepts pairs of shelving locations.  On check-in the items location is
compared ot the location on the left and, if it matches, is updated to
the location on the left.

This preference replaces ReturnToShelvingCart and
InProcessingToShelvingCart preferences.  The update statement should
insert values that replciate these functions.  Note existing
functionality of all items in PROC location being returned to
permanent_location is preserved by default.  Also, any items issued from
CART location will be returned to their permanent location on issue (if
it differs)

Special values for this pref are:
_ALL_ - used on left side only to affect all items
_BLANK_ - used on either side to match on/set to blank (actual blanks
        will work, but this is an easier to read option)
_PERM_ - used on right side only to return items to permanent location

Test Plan:
1) Apply this patch
2) Run updatedatabase.pl
3) Set the new system preference UpdateitemLocationOnCheckin
   to the following (assuming sample data):
   NEW: FIC
   FIC: GEN
4) Create an item, set its location to NEW
5) Check in the item, note its location is now FIC
6) Check in the item again, note its location is now GEN
7) Check in the item again, note its location remains GEN
8) Test using _ALL_, _BLANK_ and _PERM_ for updates
9) Try entering various incorrect syntax in the pref and note you are warned

Sponsored by:
    Arcadia Public Library (http://library.ci.arcadia.ca.us/)
    Middletown Township Public Library (http://www.mtpl.org/)
    Round Rock Public Library (https://www.roundrocktexas.gov/departments/library/)
Comment 86 Nick Clemens 2018-09-21 17:59:20 UTC
Created attachment 79246 [details] [review]
Bug 14576: Unit tests

To test:
1 - prove -v t/db_dependent/Circulation/Returns.t
2 - prove -v t/db_dependent/Circulation/issue.t
3 - prove -v t/db_dependent/UsageStats.t
4 - All should pass
Comment 87 Michal Denar 2018-10-05 06:29:51 UTC
Created attachment 79995 [details] [review]
Bug 14576: Allow arbitrary automatic update of location on checkin

This patch adds a new syspref "UpdateItemLocationOnCheckin" which
accepts pairs of shelving locations.  On check-in the items location is
compared ot the location on the left and, if it matches, is updated to
the location on the left.

This preference replaces ReturnToShelvingCart and
InProcessingToShelvingCart preferences.  The update statement should
insert values that replciate these functions.  Note existing
functionality of all items in PROC location being returned to
permanent_location is preserved by default.  Also, any items issued from
CART location will be returned to their permanent location on issue (if
it differs)

Special values for this pref are:
_ALL_ - used on left side only to affect all items
_BLANK_ - used on either side to match on/set to blank (actual blanks
        will work, but this is an easier to read option)
_PERM_ - used on right side only to return items to permanent location

Test Plan:
1) Apply this patch
2) Run updatedatabase.pl
3) Set the new system preference UpdateitemLocationOnCheckin
   to the following (assuming sample data):
   NEW: FIC
   FIC: GEN
4) Create an item, set its location to NEW
5) Check in the item, note its location is now FIC
6) Check in the item again, note its location is now GEN
7) Check in the item again, note its location remains GEN
8) Test using _ALL_, _BLANK_ and _PERM_ for updates
9) Try entering various incorrect syntax in the pref and note you are warned

Sponsored by:
    Arcadia Public Library (http://library.ci.arcadia.ca.us/)
    Middletown Township Public Library (http://www.mtpl.org/)
    Round Rock Public Library (https://www.roundrocktexas.gov/departments/library/)

Signed-off-by: Michal Denar <black23@gmail.com>
Comment 88 Michal Denar 2018-10-05 06:29:58 UTC
Created attachment 79996 [details] [review]
Bug 14576: Unit tests

To test:
1 - prove -v t/db_dependent/Circulation/Returns.t
2 - prove -v t/db_dependent/Circulation/issue.t
3 - prove -v t/db_dependent/UsageStats.t
4 - All should pass

Signed-off-by: Michal Denar <black23@gmail.com>

Signed-off-by: Michal Denar <black23@gmail.com>
Comment 89 Josef Moravec 2018-10-05 12:50:24 UTC
Nick, why do you prefer to _ALL_ rule override everything? Shouldn't the most specific rule be the first taken into account and _ALL_ rule used as fallback? I imagine it could simplify the setting  on systems with many locations.
Comment 90 Alex Buckley 2018-10-19 01:11:51 UTC
Hi Nick

Can you please rebase? The conflict against master to be resolved is only small but git-bz fails to merge it automatically.

Cheers,
Alex
Comment 91 Alex Buckley 2018-10-19 01:12:34 UTC
Applying: Bug 14576: Allow arbitrary automatic update of location on checkin
Using index info to reconstruct a base tree...
M	C4/Items.pm
M	C4/Reserves.pm
M	installer/data/mysql/sysprefs.sql
M	koha-tmpl/intranet-tmpl/prog/en/modules/admin/preferences/circulation.pref
M	koha-tmpl/intranet-tmpl/prog/en/modules/circ/returns.tt
Falling back to patching base and 3-way merge...
Auto-merging koha-tmpl/intranet-tmpl/prog/en/modules/circ/returns.tt
Auto-merging koha-tmpl/intranet-tmpl/prog/en/modules/admin/preferences/circulation.pref
Auto-merging installer/data/mysql/sysprefs.sql
Auto-merging C4/Reserves.pm
Auto-merging C4/Items.pm
CONFLICT (content): Merge conflict in C4/Items.pm
Failed to merge in the changes.
Patch failed at 0001 Bug 14576: Allow arbitrary automatic update of location on checkin
The copy of the patch that failed is found in:
   /home/vagrant/kohaclone/.git/rebase-apply/patch
When you have resolved this problem run "git bz apply --continue".
If you would prefer to skip this patch, instead run "git bz apply --skip".
To restore the original branch and stop patching run "git bz apply --abort".
Patch left in /tmp/Bug-14576-Allow-arbitrary-automatic-update-of-loca-FkmjWf.patch
Comment 92 Nick Clemens 2018-12-12 13:30:51 UTC
Created attachment 83113 [details] [review]
Bug 14576: Allow arbitrary automatic update of location on checkin

This patch adds a new syspref "UpdateItemLocationOnCheckin" which
accepts pairs of shelving locations.  On check-in the items location is
compared ot the location on the left and, if it matches, is updated to
the location on the left.

This preference replaces ReturnToShelvingCart and
InProcessingToShelvingCart preferences.  The update statement should
insert values that replciate these functions.  Note existing
functionality of all items in PROC location being returned to
permanent_location is preserved by default.  Also, any items issued from
CART location will be returned to their permanent location on issue (if
it differs)

Special values for this pref are:
_ALL_ - used on left side only to affect all items
_BLANK_ - used on either side to match on/set to blank (actual blanks
        will work, but this is an easier to read option)
_PERM_ - used on right side only to return items to permanent location

Test Plan:
1) Apply this patch
2) Run updatedatabase.pl
3) Set the new system preference UpdateitemLocationOnCheckin
   to the following (assuming sample data):
   NEW: FIC
   FIC: GEN
4) Create an item, set its location to NEW
5) Check in the item, note its location is now FIC
6) Check in the item again, note its location is now GEN
7) Check in the item again, note its location remains GEN
8) Test using _ALL_, _BLANK_ and _PERM_ for updates
9) Try entering various incorrect syntax in the pref and note you are warned

Sponsored by:
    Arcadia Public Library (http://library.ci.arcadia.ca.us/)
    Middletown Township Public Library (http://www.mtpl.org/)
    Round Rock Public Library (https://www.roundrocktexas.gov/departments/library/)

Signed-off-by: Michal Denar <black23@gmail.com>

Signed-off-by: Michal Denar <black23@gmail.com>
Comment 93 Nick Clemens 2018-12-12 13:30:59 UTC
Created attachment 83114 [details] [review]
Bug 14576: Unit tests

To test:
1 - prove -v t/db_dependent/Circulation/Returns.t
2 - prove -v t/db_dependent/Circulation/issue.t
3 - prove -v t/db_dependent/UsageStats.t
4 - All should pass

Signed-off-by: Michal Denar <black23@gmail.com>

Signed-off-by: Michal Denar <black23@gmail.com>
Comment 94 Nick Clemens 2018-12-12 13:31:08 UTC
Created attachment 83115 [details] [review]
Bug 14576: Update to use get_yaml_pref_hash and perl update statement
Comment 95 Nick Clemens 2019-01-07 17:38:47 UTC
(In reply to Josef Moravec from comment #89)
> Nick, why do you prefer to _ALL_ rule override everything? Shouldn't the
> most specific rule be the first taken into account and _ALL_ rule used as
> fallback? I imagine it could simplify the setting  on systems with many
> locations.

I suppose my thought was that _ALL_ could be used to circumvent all others in a scenario where you had a closing or something.

I am not sure I envisioned a scenario where ALL would be used constantly
Comment 96 Nick Clemens 2019-01-07 18:04:14 UTC
Created attachment 83696 [details] [review]
Bug 14576: Allow arbitrary automatic update of location on checkin

This patch adds a new syspref "UpdateItemLocationOnCheckin" which
accepts pairs of shelving locations.  On check-in the items location is
compared ot the location on the left and, if it matches, is updated to
the location on the left.

This preference replaces ReturnToShelvingCart and
InProcessingToShelvingCart preferences.  The update statement should
insert values that replciate these functions.  Note existing
functionality of all items in PROC location being returned to
permanent_location is preserved by default.  Also, any items issued from
CART location will be returned to their permanent location on issue (if
it differs)

Special values for this pref are:
_ALL_ - used on left side only to affect all items
_BLANK_ - used on either side to match on/set to blank (actual blanks
        will work, but this is an easier to read option)
_PERM_ - used on right side only to return items to permanent location

Test Plan:
1) Apply this patch
2) Run updatedatabase.pl
3) Set the new system preference UpdateitemLocationOnCheckin
   to the following (assuming sample data):
   NEW: FIC
   FIC: GEN
4) Create an item, set its location to NEW
5) Check in the item, note its location is now FIC
6) Check in the item again, note its location is now GEN
7) Check in the item again, note its location remains GEN
8) Test using _ALL_, _BLANK_ and _PERM_ for updates
9) Try entering various incorrect syntax in the pref and note you are warned

Sponsored by:
    Arcadia Public Library (http://library.ci.arcadia.ca.us/)
    Middletown Township Public Library (http://www.mtpl.org/)
    Round Rock Public Library (https://www.roundrocktexas.gov/departments/library/)

Signed-off-by: Michal Denar <black23@gmail.com>

Signed-off-by: Michal Denar <black23@gmail.com>
Comment 97 Nick Clemens 2019-01-07 18:04:19 UTC
Created attachment 83697 [details] [review]
Bug 14576: Unit tests

To test:
1 - prove -v t/db_dependent/Circulation/Returns.t
2 - prove -v t/db_dependent/Circulation/issue.t
3 - prove -v t/db_dependent/UsageStats.t
4 - All should pass

Signed-off-by: Michal Denar <black23@gmail.com>

Signed-off-by: Michal Denar <black23@gmail.com>
Comment 98 Nick Clemens 2019-01-07 18:04:25 UTC
Created attachment 83698 [details] [review]
Bug 14576: Update to use get_yaml_pref_hash
Comment 99 Katrin Fischer 2019-02-20 07:28:06 UTC
Starting another round of review with reading code and comments:

1) This example in the system preference is wrong (appears in a few spots):
E.g. ''PROC: FIC'' will cause an item that was set to ''Book Cart''
Should e: CART: FIC

2) Missing dot, also: PROC not?
Items in the CART location will be returned to their permanent location on checkout 

3) Use <b> instead of ** to highlight:
**Use of an _ALL_ rule will override/ignore any other values**

4) Not really about this patch, but stil trying to see how it all fits together:
NewItemsDefaultLocation referst to the cart_to_shelf cronjob, but I think this might be incorrect.

I will continue testing, just some first notes and all easy to fix!
Comment 100 Nick Clemens 2019-02-20 18:46:03 UTC
Created attachment 85406 [details] [review]
Bug 14576: (follow-up) fix some typos and style
Comment 101 Nick Clemens 2019-02-20 18:49:45 UTC
(In reply to Katrin Fischer from comment #99)
> 1) This example in the system preference is wrong (appears in a few spots):
Updated the description to say 'Processing center'

> 2) Missing dot, also: PROC not?

CART are always returned to permanent on checkout - proc is only update if included in this pref now (used to be syspref controlled by InProcessingToShelvingCart)
kout 

> 3) Use <b> instead of ** to highlight:
Done 

> 4) Not really about this patch, but stil trying to see how it all fits
> together:
> NewItemsDefaultLocation referst to the cart_to_shelf cronjob, but I think
> this might be incorrect.
cart_to_shelf stil exists - if you set items to be returned to the cart then you still need to reset them to original location after they are returned (or add a rule to move from CART to PERM and check them in again)
Comment 102 Katrin Fischer 2019-02-20 18:55:36 UTC
> > 4) Not really about this patch, but stil trying to see how it all fits
> > together:
> > NewItemsDefaultLocation referst to the cart_to_shelf cronjob, but I think
> > this might be incorrect.
> cart_to_shelf stil exists - if you set items to be returned to the cart then
> you still need to reset them to original location after they are returned
> (or add a rule to move from CART to PERM and check them in again)

I know it does still exist :) But I think this pref isn't about the cart? What if I chose 'new item shelf' as my default location? Or PROC.
Comment 103 Katrin Fischer 2019-02-25 19:34:12 UTC
(In reply to Katrin Fischer from comment #102)
> > > 4) Not really about this patch, but stil trying to see how it all fits
> > > together:
> > > NewItemsDefaultLocation referst to the cart_to_shelf cronjob, but I think
> > > this might be incorrect.
> > cart_to_shelf stil exists - if you set items to be returned to the cart then
> > you still need to reset them to original location after they are returned
> > (or add a rule to move from CART to PERM and check them in again)
> 
> I know it does still exist :) But I think this pref isn't about the cart?
> What if I chose 'new item shelf' as my default location? Or PROC?

Discussion for a different bug.
Comment 104 Katrin Fischer 2019-02-28 06:57:55 UTC
Sorry, Nick, can you please rebase?
Comment 105 Nick Clemens 2019-02-28 11:55:21 UTC
Created attachment 85850 [details] [review]
Bug 14576: Allow arbitrary automatic update of location on checkin

This patch adds a new syspref "UpdateItemLocationOnCheckin" which
accepts pairs of shelving locations.  On check-in the items location is
compared ot the location on the left and, if it matches, is updated to
the location on the left.

This preference replaces ReturnToShelvingCart and
InProcessingToShelvingCart preferences.  The update statement should
insert values that replciate these functions.  Note existing
functionality of all items in PROC location being returned to
permanent_location is preserved by default.  Also, any items issued from
CART location will be returned to their permanent location on issue (if
it differs)

Special values for this pref are:
_ALL_ - used on left side only to affect all items
_BLANK_ - used on either side to match on/set to blank (actual blanks
        will work, but this is an easier to read option)
_PERM_ - used on right side only to return items to permanent location

Test Plan:
1) Apply this patch
2) Run updatedatabase.pl
3) Set the new system preference UpdateitemLocationOnCheckin
   to the following (assuming sample data):
   NEW: FIC
   FIC: GEN
4) Create an item, set its location to NEW
5) Check in the item, note its location is now FIC
6) Check in the item again, note its location is now GEN
7) Check in the item again, note its location remains GEN
8) Test using _ALL_, _BLANK_ and _PERM_ for updates
9) Try entering various incorrect syntax in the pref and note you are warned

Sponsored by:
    Arcadia Public Library (http://library.ci.arcadia.ca.us/)
    Middletown Township Public Library (http://www.mtpl.org/)
    Round Rock Public Library (https://www.roundrocktexas.gov/departments/library/)

Signed-off-by: Michal Denar <black23@gmail.com>
Comment 106 Nick Clemens 2019-02-28 11:55:26 UTC
Created attachment 85851 [details] [review]
Bug 14576: Unit tests

To test:
1 - prove -v t/db_dependent/Circulation/Returns.t
2 - prove -v t/db_dependent/Circulation/issue.t
3 - prove -v t/db_dependent/UsageStats.t
4 - All should pass

Signed-off-by: Michal Denar <black23@gmail.com>

Signed-off-by: Michal Denar <black23@gmail.com>
Comment 107 Nick Clemens 2019-02-28 11:55:31 UTC
Created attachment 85852 [details] [review]
Bug 14576: Update to use get_yaml_pref_hash
Comment 108 Nick Clemens 2019-02-28 11:55:36 UTC
Created attachment 85853 [details] [review]
Bug 14576: (follow-up) fix some typos and style
Comment 109 Nick Clemens 2019-02-28 11:57:23 UTC
(In reply to Katrin Fischer from comment #104)
> Sorry, Nick, can you please rebase?

Rebased. Please note:
QA complaint of POD is false positive (coverage decreased because subroutine removed)

The ModItem calls made for location change are logged - some calls were silenced recently to avoid spamming the logs, if we are updating because of a location change though it should be logged I believe
Comment 110 Michal Denar 2019-03-17 21:35:26 UTC
Nick,
can you rebase it again? I get:
Patch failed at 0001 Bug 14576: Allow arbitrary automatic update of location on checkin
The copy of the patch that failed is found in: .git/rebase-apply/patch
When you have resolved this problem run "git bz apply --continue".
If you would prefer to skip this patch, instead run "git bz apply --skip".
To restore the original branch and stop patching run "git bz apply --abort".
Patch left in /tmp/Bug-14576-Allow-arbitrary-automatic-update-of-loca-9ePdA7.patch

Thank you.
Comment 111 Nick Clemens 2019-03-20 15:47:32 UTC
Created attachment 86812 [details] [review]
Bug 14576: Allow arbitrary automatic update of location on checkin

This patch adds a new syspref "UpdateItemLocationOnCheckin" which
accepts pairs of shelving locations.  On check-in the items location is
compared ot the location on the left and, if it matches, is updated to
the location on the left.

This preference replaces ReturnToShelvingCart and
InProcessingToShelvingCart preferences.  The update statement should
insert values that replciate these functions.  Note existing
functionality of all items in PROC location being returned to
permanent_location is preserved by default.  Also, any items issued from
CART location will be returned to their permanent location on issue (if
it differs)

Special values for this pref are:
_ALL_ - used on left side only to affect all items
_BLANK_ - used on either side to match on/set to blank (actual blanks
        will work, but this is an easier to read option)
_PERM_ - used on right side only to return items to permanent location

Test Plan:
1) Apply this patch
2) Run updatedatabase.pl
3) Set the new system preference UpdateitemLocationOnCheckin
   to the following (assuming sample data):
   NEW: FIC
   FIC: GEN
4) Create an item, set its location to NEW
5) Check in the item, note its location is now FIC
6) Check in the item again, note its location is now GEN
7) Check in the item again, note its location remains GEN
8) Test using _ALL_, _BLANK_ and _PERM_ for updates
9) Try entering various incorrect syntax in the pref and note you are warned

Sponsored by:
    Arcadia Public Library (http://library.ci.arcadia.ca.us/)
    Middletown Township Public Library (http://www.mtpl.org/)
    Round Rock Public Library (https://www.roundrocktexas.gov/departments/library/)

Signed-off-by: Michal Denar <black23@gmail.com>
Comment 112 Nick Clemens 2019-03-20 15:47:37 UTC
Created attachment 86813 [details] [review]
Bug 14576: Unit tests

To test:
1 - prove -v t/db_dependent/Circulation/Returns.t
2 - prove -v t/db_dependent/Circulation/issue.t
3 - prove -v t/db_dependent/UsageStats.t
4 - All should pass

Signed-off-by: Michal Denar <black23@gmail.com>

Signed-off-by: Michal Denar <black23@gmail.com>
Comment 113 Nick Clemens 2019-03-20 15:47:42 UTC
Created attachment 86814 [details] [review]
Bug 14576: Update to use get_yaml_pref_hash
Comment 114 Nick Clemens 2019-03-20 15:47:47 UTC
Created attachment 86815 [details] [review]
Bug 14576: (follow-up) fix some typos and style
Comment 115 Nick Clemens 2019-03-20 15:47:52 UTC
Created attachment 86816 [details] [review]
Bug 14576: Rebase fixes Fix test Rebase code
Comment 116 Hayley Pelham 2019-03-26 22:21:44 UTC
This patch fails when checking in an item, due to use of a deprecated method GetItem (removed in patch 21206) in C4/Items.pm. This method is also called in C4/Reserves.pm. This should be changed to Koha::Items->find function.
Comment 117 Hayley Pelham 2019-03-26 23:24:56 UTC
Created attachment 87040 [details] [review]
Bug 14576: Replaced calls using deprecated method GetItem with Koha::Items->find

Test plan:
1) Apply all patches except this one
2) Checkout out an item to a patron
3) Check the item in - note that this will fail
4) Apply this patch, and repeat steps 1-3, noting instead that the the
check is instead successful
5) Verify also that reserves act as expected
5) Sign off

Sponsored-by: Catalyst IT
Comment 118 Nick Clemens 2019-03-27 11:37:07 UTC
Created attachment 87061 [details] [review]
Bug 14576: (follow-up) Using item objects, not hashrefs
Comment 119 Liz Rea 2019-03-28 16:45:46 UTC
Created attachment 87144 [details] [review]
Bug 14576: Allow arbitrary automatic update of location on checkin

This patch adds a new syspref "UpdateItemLocationOnCheckin" which
accepts pairs of shelving locations.  On check-in the items location is
compared ot the location on the left and, if it matches, is updated to
the location on the left.

This preference replaces ReturnToShelvingCart and
InProcessingToShelvingCart preferences.  The update statement should
insert values that replciate these functions.  Note existing
functionality of all items in PROC location being returned to
permanent_location is preserved by default.  Also, any items issued from
CART location will be returned to their permanent location on issue (if
it differs)

Special values for this pref are:
_ALL_ - used on left side only to affect all items
_BLANK_ - used on either side to match on/set to blank (actual blanks
        will work, but this is an easier to read option)
_PERM_ - used on right side only to return items to permanent location

Test Plan:
1) Apply this patch
2) Run updatedatabase.pl
3) Set the new system preference UpdateitemLocationOnCheckin
   to the following (assuming sample data):
   NEW: FIC
   FIC: GEN
4) Create an item, set its location to NEW
5) Check in the item, note its location is now FIC
6) Check in the item again, note its location is now GEN
7) Check in the item again, note its location remains GEN
8) Test using _ALL_, _BLANK_ and _PERM_ for updates
9) Try entering various incorrect syntax in the pref and note you are warned

Sponsored by:
    Arcadia Public Library (http://library.ci.arcadia.ca.us/)
    Middletown Township Public Library (http://www.mtpl.org/)
    Round Rock Public Library (https://www.roundrocktexas.gov/departments/library/)

Signed-off-by: Michal Denar <black23@gmail.com>

Signed-off-by: Liz Rea <wizzyrea@gmail.com>
Comment 120 Liz Rea 2019-03-28 16:45:52 UTC
Created attachment 87145 [details] [review]
Bug 14576: Unit tests

To test:
1 - prove -v t/db_dependent/Circulation/Returns.t
2 - prove -v t/db_dependent/Circulation/issue.t
3 - prove -v t/db_dependent/UsageStats.t
4 - All should pass

Signed-off-by: Michal Denar <black23@gmail.com>

Signed-off-by: Michal Denar <black23@gmail.com>

Signed-off-by: Liz Rea <wizzyrea@gmail.com>
Comment 121 Liz Rea 2019-03-28 16:45:58 UTC
Created attachment 87146 [details] [review]
Bug 14576: Update to use get_yaml_pref_hash

Signed-off-by: Liz Rea <wizzyrea@gmail.com>
Comment 122 Liz Rea 2019-03-28 16:46:05 UTC
Created attachment 87147 [details] [review]
Bug 14576: (follow-up) fix some typos and style

Signed-off-by: Liz Rea <wizzyrea@gmail.com>
Comment 123 Liz Rea 2019-03-28 16:46:10 UTC
Created attachment 87148 [details] [review]
Bug 14576: Rebase fixes Fix test Rebase code

Signed-off-by: Liz Rea <wizzyrea@gmail.com>
Comment 124 Liz Rea 2019-03-28 16:46:17 UTC
Created attachment 87149 [details] [review]
Bug 14576: Replaced calls using deprecated method GetItem with Koha::Items->find

Test plan:
1) Apply all patches except this one
2) Checkout out an item to a patron
3) Check the item in - note that this will fail
4) Apply this patch, and repeat steps 1-3, noting instead that the the
check is instead successful
5) Verify also that reserves act as expected
5) Sign off

Sponsored-by: Catalyst IT

Signed-off-by: Liz Rea <wizzyrea@gmail.com>
Comment 125 Liz Rea 2019-03-28 16:46:24 UTC
Created attachment 87150 [details] [review]
Bug 14576: (follow-up) Using item objects, not hashrefs

Signed-off-by: Liz Rea <wizzyrea@gmail.com>
Comment 126 Liz Rea 2019-03-28 16:46:30 UTC
Created attachment 87151 [details] [review]
Bug 14576: (QA Follow up) Tidy up feature documentation

To test:
Apply this patch with all of the others
run the updatedatabase, it should work
install a new Koha, this pref should be installed and look fine in the
interface.
Check the syspref, make sure the text looks ok and makes sense.

Bonus points:
for checking to make sure PROC: _PERM_ appears in the new
pereference if InProcessingToShelvingCart was set
for checking to make sure _ALL_: CART appears in the new preferenec if ReturnToShelvingCart was previously set.
Comment 127 Liz Rea 2019-03-28 16:58:05 UTC
I note there is a QA tool false positive here:

FAIL	  pod coverage
		POD coverage was greater before, try perl -MPod::Coverage=PackageName -e666


POD coverage decreased because a sub was removed.
Comment 128 Katrin Fischer 2019-04-12 12:38:49 UTC
Hi Liz, can you please write (QA follow-up)? The other spelling makes the QA tools unhappy (consistency...).
Comment 129 Nick Clemens 2019-04-12 14:55:46 UTC
Created attachment 87925 [details] [review]
Bug 14576: (QA follow-up) Tidy up feature documentation

To test:
Apply this patch with all of the others
run the updatedatabase, it should work
install a new Koha, this pref should be installed and look fine in the
interface.
Check the syspref, make sure the text looks ok and makes sense.

Bonus points:
for checking to make sure PROC: _PERM_ appears in the new
pereference if InProcessingToShelvingCart was set
for checking to make sure _ALL_: CART appears in the new preferenec if ReturnToShelvingCart was previously set.
Comment 130 Josef Moravec 2019-04-23 12:04:58 UTC
Created attachment 88451 [details] [review]
Bug 14576: Allow arbitrary automatic update of location on checkin

This patch adds a new syspref "UpdateItemLocationOnCheckin" which
accepts pairs of shelving locations.  On check-in the items location is
compared ot the location on the left and, if it matches, is updated to
the location on the left.

This preference replaces ReturnToShelvingCart and
InProcessingToShelvingCart preferences.  The update statement should
insert values that replciate these functions.  Note existing
functionality of all items in PROC location being returned to
permanent_location is preserved by default.  Also, any items issued from
CART location will be returned to their permanent location on issue (if
it differs)

Special values for this pref are:
_ALL_ - used on left side only to affect all items
_BLANK_ - used on either side to match on/set to blank (actual blanks
        will work, but this is an easier to read option)
_PERM_ - used on right side only to return items to permanent location

Test Plan:
1) Apply this patch
2) Run updatedatabase.pl
3) Set the new system preference UpdateitemLocationOnCheckin
   to the following (assuming sample data):
   NEW: FIC
   FIC: GEN
4) Create an item, set its location to NEW
5) Check in the item, note its location is now FIC
6) Check in the item again, note its location is now GEN
7) Check in the item again, note its location remains GEN
8) Test using _ALL_, _BLANK_ and _PERM_ for updates
9) Try entering various incorrect syntax in the pref and note you are warned

Sponsored by:
    Arcadia Public Library (http://library.ci.arcadia.ca.us/)
    Middletown Township Public Library (http://www.mtpl.org/)
    Round Rock Public Library (https://www.roundrocktexas.gov/departments/library/)

Signed-off-by: Michal Denar <black23@gmail.com>

Signed-off-by: Liz Rea <wizzyrea@gmail.com>
Comment 131 Josef Moravec 2019-04-23 12:05:04 UTC
Created attachment 88452 [details] [review]
Bug 14576: Unit tests

To test:
1 - prove -v t/db_dependent/Circulation/Returns.t
2 - prove -v t/db_dependent/Circulation/issue.t
3 - prove -v t/db_dependent/UsageStats.t
4 - All should pass

Signed-off-by: Michal Denar <black23@gmail.com>

Signed-off-by: Michal Denar <black23@gmail.com>

Signed-off-by: Liz Rea <wizzyrea@gmail.com>
Comment 132 Josef Moravec 2019-04-23 12:05:10 UTC
Created attachment 88453 [details] [review]
Bug 14576: Update to use get_yaml_pref_hash

Signed-off-by: Liz Rea <wizzyrea@gmail.com>
Comment 133 Josef Moravec 2019-04-23 12:05:15 UTC
Created attachment 88454 [details] [review]
Bug 14576: (follow-up) fix some typos and style

Signed-off-by: Liz Rea <wizzyrea@gmail.com>
Comment 134 Josef Moravec 2019-04-23 12:05:21 UTC
Created attachment 88455 [details] [review]
Bug 14576: Rebase fixes Fix test Rebase code

Signed-off-by: Liz Rea <wizzyrea@gmail.com>
Comment 135 Josef Moravec 2019-04-23 12:05:27 UTC
Created attachment 88456 [details] [review]
Bug 14576: Replaced calls using deprecated method GetItem with Koha::Items->find

Test plan:
1) Apply all patches except this one
2) Checkout out an item to a patron
3) Check the item in - note that this will fail
4) Apply this patch, and repeat steps 1-3, noting instead that the the
check is instead successful
5) Verify also that reserves act as expected
5) Sign off

Sponsored-by: Catalyst IT

Signed-off-by: Liz Rea <wizzyrea@gmail.com>
Comment 136 Josef Moravec 2019-04-23 12:05:32 UTC
Created attachment 88457 [details] [review]
Bug 14576: (follow-up) Using item objects, not hashrefs

Signed-off-by: Liz Rea <wizzyrea@gmail.com>
Comment 137 Josef Moravec 2019-04-23 12:05:38 UTC
Created attachment 88458 [details] [review]
Bug 14576: (QA follow-up) Tidy up feature documentation

To test:
Apply this patch with all of the others
run the updatedatabase, it should work
install a new Koha, this pref should be installed and look fine in the
interface.
Check the syspref, make sure the text looks ok and makes sense.

Bonus points:
for checking to make sure PROC: _PERM_ appears in the new
pereference if InProcessingToShelvingCart was set
for checking to make sure _ALL_: CART appears in the new preferenec if ReturnToShelvingCart was previously set.
Comment 138 Josef Moravec 2019-04-23 12:05:43 UTC
Created attachment 88459 [details] [review]
Bug 14576: (follow-up) Add code highlighting to system preference UpdateItemLocationOnCheckin

Test plan:
1) Go to system preferences and find preference UpdateItemLocationOnCheckin
2) Ensure the yaml code higlighting is used on textarea when editing
this setting
Comment 139 Josef Moravec 2019-04-23 12:06:46 UTC
Rebased and added code highlighting on syspref UpdateItemLocationOnCheckin
Comment 140 Katrin Fischer 2019-04-24 20:56:44 UTC
Created attachment 88646 [details] [review]
Bug 14576: Allow arbitrary automatic update of location on checkin

This patch adds a new syspref "UpdateItemLocationOnCheckin" which
accepts pairs of shelving locations.  On check-in the items location is
compared ot the location on the left and, if it matches, is updated to
the location on the left.

This preference replaces ReturnToShelvingCart and
InProcessingToShelvingCart preferences.  The update statement should
insert values that replciate these functions.  Note existing
functionality of all items in PROC location being returned to
permanent_location is preserved by default.  Also, any items issued from
CART location will be returned to their permanent location on issue (if
it differs)

Special values for this pref are:
_ALL_ - used on left side only to affect all items
_BLANK_ - used on either side to match on/set to blank (actual blanks
        will work, but this is an easier to read option)
_PERM_ - used on right side only to return items to permanent location

Test Plan:
1) Apply this patch
2) Run updatedatabase.pl
3) Set the new system preference UpdateitemLocationOnCheckin
   to the following (assuming sample data):
   NEW: FIC
   FIC: GEN
4) Create an item, set its location to NEW
5) Check in the item, note its location is now FIC
6) Check in the item again, note its location is now GEN
7) Check in the item again, note its location remains GEN
8) Test using _ALL_, _BLANK_ and _PERM_ for updates
9) Try entering various incorrect syntax in the pref and note you are warned

Sponsored by:
    Arcadia Public Library (http://library.ci.arcadia.ca.us/)
    Middletown Township Public Library (http://www.mtpl.org/)
    Round Rock Public Library (https://www.roundrocktexas.gov/departments/library/)

Signed-off-by: Michal Denar <black23@gmail.com>

Signed-off-by: Liz Rea <wizzyrea@gmail.com>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 141 Katrin Fischer 2019-04-24 20:56:50 UTC
Created attachment 88647 [details] [review]
Bug 14576: Unit tests

To test:
1 - prove -v t/db_dependent/Circulation/Returns.t
2 - prove -v t/db_dependent/Circulation/issue.t
3 - prove -v t/db_dependent/UsageStats.t
4 - All should pass

Signed-off-by: Michal Denar <black23@gmail.com>

Signed-off-by: Michal Denar <black23@gmail.com>

Signed-off-by: Liz Rea <wizzyrea@gmail.com>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 142 Katrin Fischer 2019-04-24 20:56:57 UTC
Created attachment 88648 [details] [review]
Bug 14576: Update to use get_yaml_pref_hash

Signed-off-by: Liz Rea <wizzyrea@gmail.com>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 143 Katrin Fischer 2019-04-24 20:57:03 UTC
Created attachment 88649 [details] [review]
Bug 14576: (follow-up) fix some typos and style

Signed-off-by: Liz Rea <wizzyrea@gmail.com>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 144 Katrin Fischer 2019-04-24 20:57:09 UTC
Created attachment 88650 [details] [review]
Bug 14576: Rebase fixes Fix test Rebase code

Signed-off-by: Liz Rea <wizzyrea@gmail.com>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 145 Katrin Fischer 2019-04-24 20:57:16 UTC
Created attachment 88651 [details] [review]
Bug 14576: Replaced calls using deprecated method GetItem with Koha::Items->find

Test plan:
1) Apply all patches except this one
2) Checkout out an item to a patron
3) Check the item in - note that this will fail
4) Apply this patch, and repeat steps 1-3, noting instead that the the
check is instead successful
5) Verify also that reserves act as expected
5) Sign off

Sponsored-by: Catalyst IT

Signed-off-by: Liz Rea <wizzyrea@gmail.com>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 146 Katrin Fischer 2019-04-24 20:57:22 UTC
Created attachment 88652 [details] [review]
Bug 14576: (follow-up) Using item objects, not hashrefs

Signed-off-by: Liz Rea <wizzyrea@gmail.com>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 147 Katrin Fischer 2019-04-24 20:57:28 UTC
Created attachment 88653 [details] [review]
Bug 14576: (QA follow-up) Tidy up feature documentation

To test:
Apply this patch with all of the others
run the updatedatabase, it should work
install a new Koha, this pref should be installed and look fine in the
interface.
Check the syspref, make sure the text looks ok and makes sense.

Bonus points:
for checking to make sure PROC: _PERM_ appears in the new
pereference if InProcessingToShelvingCart was set
for checking to make sure _ALL_: CART appears in the new preferenec if ReturnToShelvingCart was previously set.

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 148 Katrin Fischer 2019-04-24 20:57:35 UTC
Created attachment 88654 [details] [review]
Bug 14576: (follow-up) Add code highlighting to system preference UpdateItemLocationOnCheckin

Test plan:
1) Go to system preferences and find preference UpdateItemLocationOnCheckin
2) Ensure the yaml code higlighting is used on textarea when editing
this setting

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 149 Nick Clemens 2019-04-25 11:51:34 UTC
Awesome work all!

Pushed to master for 19.05
Comment 150 Martin Renvoize 2019-04-26 15:32:10 UTC
Enhancement will not be backported to 18.11.x series.
Comment 151 David Nind 2019-05-29 18:27:53 UTC
Not all the sponsored develo are shown in the release notes - from comment 140 should include:
    Arcadia Public Library (http://library.ci.arcadia.ca.us/)
    Middletown Township Public Library (http://www.mtpl.org/)
    Round Rock Public Library (https://www.roundrocktexas.gov/departments/library/)
Comment 152 Jonathan Druart 2019-05-29 21:35:24 UTC
(In reply to David Nind from comment #151)
> Not all the sponsored develo are shown in the release notes - from comment
> 140 should include:
>     Arcadia Public Library (http://library.ci.arcadia.ca.us/)
>     Middletown Township Public Library (http://www.mtpl.org/)
>     Round Rock Public Library
> (https://www.roundrocktexas.gov/departments/library/)

I guess it should have been 3 different lines to have made them retrieved correctly from the release notes script.
Comment 153 Tomáš Wünsch 2019-10-23 07:11:53 UTC
https://koha-community.org/koha-19-05-release/
NONSENSICAL explanation in release note:
" On check in the item’s location is compared to the location on the left and, if it matches, is updated to the location on the left. "
(2x left) 
in system preferences rules is better version:
"General rule: if the location value on the left of the colon (:) matches the item's current location, it will be updated to match the location value on the right of the colon (:)."
Comment 154 Katrin Fischer 2019-10-23 07:15:31 UTC
Fixed the 'release notes' field here in bugzilla, not sure we can do more at this point.
Comment 155 Marcel de Rooy 2022-01-28 11:04:27 UTC
Somewhere an extra defined test on item->location has been introduced to this feature, which effectively causes location NULL items to be skipped from marking them with CART.
Probably only with the good intention to remove a warn, but adding this side-effect.
Comment 156 Marcel de Rooy 2022-01-31 08:26:23 UTC
(In reply to Marcel de Rooy from comment #155)
> Somewhere an extra defined test on item->location has been introduced to
> this feature, which effectively causes location NULL items to be skipped
> from marking them with CART.
> Probably only with the good intention to remove a warn, but adding this
> side-effect.

This has been fixed on bug 28472