Bug 18355 - Permanent location should show with cart location
Summary: Permanent location should show with cart location
Status: Passed QA
Alias: None
Product: Koha
Classification: Unclassified
Component: Circulation (show other bugs)
Version: master
Hardware: All All
: P5 - low enhancement (vote)
Assignee: Christopher Brannon
QA Contact: Marcel de Rooy
URL:
Keywords: rel_20_05_target
Depends on: 21466
Blocks:
  Show dependency treegraph
 
Reported: 2017-03-30 18:23 UTC by Christopher Brannon
Modified: 2019-11-29 11:18 UTC (History)
12 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:


Attachments
Bug 18355: Modifies location on detail pages. (3.11 KB, patch)
2017-04-07 21:00 UTC, Christopher Brannon
Details | Diff | Splinter Review
Attachment to Bug 18355 - Permanent location should show with cart location (2.67 KB, patch)
2017-04-07 21:28 UTC, Christopher Brannon
Details | Diff | Splinter Review
Bug 18355: Permanent location should show with cart location (2.89 KB, patch)
2017-04-08 08:17 UTC, Marc Véron
Details | Diff | Splinter Review
Bug 18355: Modifies location on detail pages. (7.49 KB, patch)
2017-05-02 22:42 UTC, Christopher Brannon
Details | Diff | Splinter Review
Bug 18355: Modifies location on detail pages. (7.18 KB, patch)
2017-08-25 03:26 UTC, M. Tompsett
Details | Diff | Splinter Review
Bug 18355: Modifies location on detail pages. (7.73 KB, patch)
2017-08-27 20:51 UTC, Alex Buckley
Details | Diff | Splinter Review
Bug 18355: Modifies location on detail pages. (15.80 KB, patch)
2018-09-12 00:13 UTC, Christopher Brannon
Details | Diff | Splinter Review
Bug 18355: Modifies location on detail pages. (15.87 KB, patch)
2018-09-15 22:30 UTC, ByWater Sandboxes
Details | Diff | Splinter Review
Bug 18355: Modifies location on detail pages. (15.96 KB, patch)
2018-09-15 23:00 UTC, Lisette Scheer
Details | Diff | Splinter Review
Bug 18355: Modifies location on details pages. (3.68 KB, patch)
2018-10-01 00:51 UTC, Christopher Brannon
Details | Diff | Splinter Review
[ALTERNATIVE-PATCH %] Bug 18355: Display permanent location with cart location (2.85 KB, patch)
2018-10-01 14:24 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 18355: Remove obsolete code from pl (1014 bytes, patch)
2018-10-01 21:35 UTC, Christopher Brannon
Details | Diff | Splinter Review
Bug 18355: Update opac-detail.pl/tt to show both (4.41 KB, patch)
2018-10-05 23:12 UTC, Christopher Brannon
Details | Diff | Splinter Review
Bug 18355: Update opac-detail.pl/tt to show both (4.48 KB, patch)
2019-01-04 21:33 UTC, José-Mario Monteiro-Santos
Details | Diff | Splinter Review
Bug 18355: Display permanent location with cart location (2.73 KB, patch)
2019-11-25 10:02 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 18355: Display permanent location with cart location (2.84 KB, patch)
2019-11-29 11:18 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 18355: (QA follow-up) Rearrange comments, improve code (3.47 KB, patch)
2019-11-29 11:18 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 Christopher Brannon 2017-03-30 18:23:59 UTC
It would be helpful if the permanent location would show even when the cart location is in use.  For example:

Location
CART (Adult Non-fiction)

It is difficult to track an item if it has an unusual location, like a display location, and it is recently returned.

This information should show anywhere the shelving location is displayed.
Comment 1 Christopher Brannon 2017-04-07 21:00:16 UTC
Created attachment 61978 [details] [review]
Bug 18355: Modifies location on detail pages.
Comment 2 Christopher Brannon 2017-04-07 21:05:59 UTC
Sorry, the attach turned out wonky.

TEST PLAN
1) Make sure ReturnToShelvingCart is set to Move. 
2) Check in an item. 
3) View
 the item in detail.pl and opac-detail.pl and see that only the cart location
 shows. 
4) Apply patch and view the item again in check detail.pl and
 opac-detail.pl.  You should now see the permanent location, and the cart
 location in parenthesis. 
5) Check another item that was not checked in.  You
 should see the permanent location and not a secondary location.
Comment 3 Christopher Brannon 2017-04-07 21:11:08 UTC
I'm not sure how to fix the subject in the attachment.
Comment 4 Christopher Brannon 2017-04-07 21:21:28 UTC
Never mind.  I fixed it.  :)

I am only addressing the detail pages in the patch.  I looked over the search results, and the item information is being pulled in differently, so I am not sure how to add the information there.  It looks like it is being pulled in via marc records, which seems a little odd.

Anyway, I consider this the extent of the patch.  If there are other locations that would benefit from this added information, we can address it in another patch.
Comment 5 Christopher Brannon 2017-04-07 21:28:49 UTC
Created attachment 61981 [details] [review]
Attachment to Bug 18355 - Permanent location should show with cart location

Bug 18355: Modifies location on detail pages.
Comment 6 Marc Véron 2017-04-08 08:17:16 UTC
Created attachment 61984 [details] [review]
Bug 18355: Permanent location should show with cart location

Followed test plan from Comment #2, tested on current Master,
works as expected
Signed-off-by: Marc Véron <veron@veron.ch>
Comment 7 Marc Véron 2017-04-08 08:19:00 UTC
Setting version to master (tested on master)
Comment 8 Nick Clemens 2017-04-28 18:35:21 UTC
I like the idea, two strange display issues:

1 - Items with no location before cart display as "CART (Book Cart)", I would expect just to see "Book cart"
2 - Items with a shelving location not tied to an authorized value also display as "CART (Book Cart)" I would expect to see "Book Cart (value)"
Comment 9 Christopher Brannon 2017-05-02 22:42:19 UTC
Created attachment 62999 [details] [review]
Bug 18355: Modifies location on detail pages.
Comment 10 Christopher Brannon 2017-05-02 22:45:33 UTC
(In reply to Nick Clemens from comment #8)
> I like the idea, two strange display issues:
> 
> 1 - Items with no location before cart display as "CART (Book Cart)", I
> would expect just to see "Book cart"
> 2 - Items with a shelving location not tied to an authorized value also
> display as "CART (Book Cart)" I would expect to see "Book Cart (value)"

Good catch.  It is fixed.  Didn't have the same issue on the OPAC.
Comment 11 Alex Buckley 2017-07-30 19:24:47 UTC
Bug 18355 - Permanent location should show with cart location

62999 - Bug 18355: Modifies location on detail pages.

Apply? [(y)es, (n)o, (i)nteractive] y
Applying: Bug 18355: Modifies location on detail pages.
Using index info to reconstruct a base tree...
M	catalogue/detail.pl
M	opac/opac-detail.pl
Falling back to patching base and 3-way merge...
Auto-merging opac/opac-detail.pl
Auto-merging catalogue/detail.pl
Comment 12 Alex Buckley 2017-07-30 19:26:56 UTC
Hi Christopher

Can you please merge your patches with the up to date master branch?

Cheers
Alex
Comment 13 M. Tompsett 2017-08-25 03:25:30 UTC
(In reply to Alex Buckley from comment #12)
> Can you please merge your patches with the up to date master branch?

Git properly merged this, there is no reason to mark this as does not apply. I, however, will push up a cleaner apply.
Comment 14 M. Tompsett 2017-08-25 03:26:06 UTC
Created attachment 66432 [details] [review]
Bug 18355: Modifies location on detail pages.
Comment 15 Christopher Brannon 2017-08-25 17:27:48 UTC
(In reply to M. Tompsett from comment #13)
> (In reply to Alex Buckley from comment #12)
> > Can you please merge your patches with the up to date master branch?
> 
> Git properly merged this, there is no reason to mark this as does not apply.
> I, however, will push up a cleaner apply.

Thank you.  I have been swamped, and have been trying to figure out how to do this since I rebuilt my kohadevbox.  :)
Comment 16 Alex Buckley 2017-08-27 20:51:00 UTC
Created attachment 66514 [details] [review]
Bug 18355: Modifies location on detail pages.

TEST PLAN
1) Make sure ReturnToShelvingCart is set to Move.
2) Check in an item.
3) View the item in detail.pl and opac-detail.pl and see that only the cart location shows.
4) Apply patch and view the item again in check detail.pl and
opac-detail.pl. You should now see the permanent location, and the
cart location in parenthesis.
5) Check another item that was not checked in. You should see the permanent location and not a secondary location.

Followed test plan, patch worked as described. Passes QA test

Signed-off-by: Alex Buckley <alexbuckley@catalyst.net.nz>
Comment 17 Alex Buckley 2017-08-27 20:53:26 UTC
Hi Christopher

Your patch works as described and I have signed off on it, I have added your test plan from comment 2 onto the patch as I know it makes it easier for the Release Manager
Comment 18 Christopher Brannon 2017-08-29 15:08:36 UTC
(In reply to Alex Buckley from comment #17)
> Hi Christopher
> 
> Your patch works as described and I have signed off on it, I have added your
> test plan from comment 2 onto the patch as I know it makes it easier for the
> Release Manager

Thank you.
Comment 19 Nick Clemens 2017-10-24 18:07:39 UTC
If permanent_location is NULL the current location shows in parens

If the current location is not defined in authorised values:
On staff side there is nothing displayed (current behavior on staff sideshows the code)
On OPAC you get blank parens for null permanent_location


I think the proper logic is:
1 - If permanent location is defined
    a - display the description if available, display the code if not
    b - display the current location in parens
2 - If permanent location is not defined, but location is defined
    a - display the current location description if available, display the code if not
3 - If neither are defined, show nothing
Comment 20 Christopher Brannon 2018-09-12 00:13:13 UTC
Created attachment 78584 [details] [review]
Bug 18355: Modifies location on detail pages.

TEST PLAN
1) Make sure ReturnToShelvingCart is set to Move.
2) Check in an item.
3) View the item in detail.pl and opac-detail.pl and see that only the
cart location shows.
4) Apply patch and view the item again in check detail and
opac-detail.pl.  You should now see the permanent location, and the cart
location in parenthesis.
5) Try removing descriptions from locations.  Codes should replace
descriptions when not available.  Nothing should show if a value (code
is missing.
6) Check another item that was not checked in.  You should see the
permanent location and not a secondary location.
Comment 21 Christopher Brannon 2018-09-12 00:16:26 UTC
Updated patch (FINALLY!) and updated test plan.  Responded to Nick's concerns.
Comment 22 Christopher Brannon 2018-09-14 23:44:07 UTC
UPDATED TEST PLAN
1) Make sure ReturnToShelvingCart is set to Move.
2) Check in an item.
3) View the item in detail.pl and opac-detail.pl and see that only the
cart location shows.
4) Apply patch and view the item again in check detail and
opac-detail.pl.  You should now see the permanent location, and the cart
location in parenthesis.
5) Try removing descriptions from locations.  Codes should replace
descriptions when not available.  
6) Check another item that was not checked in (don't check in).  You should see the permanent location and not a secondary location.
Comment 23 ByWater Sandboxes 2018-09-15 22:30:55 UTC
Created attachment 78908 [details] [review]
Bug 18355: Modifies location on detail pages.

TEST PLAN
1) Make sure ReturnToShelvingCart is set to Move.
2) Check in an item.
3) View the item in detail.pl and opac-detail.pl and see that only the
cart location shows.
4) Apply patch and view the item again in check detail and
opac-detail.pl.  You should now see the permanent location, and the cart
location in parenthesis.
5) Try removing descriptions from locations.  Codes should replace
descriptions when not available.  Nothing should show if a value (code
is missing.
6) Check another item that was not checked in.  You should see the
permanent location and not a secondary location.

Signed-off-by: Spencer <ssmith3@mckinneytexas.org>
Comment 24 Lisette Scheer 2018-09-15 23:00:12 UTC
Created attachment 78913 [details] [review]
Bug 18355: Modifies location on detail pages.

TEST PLAN
1) Make sure ReturnToShelvingCart is set to Move.
2) Check in an item.
3) View the item in detail.pl and opac-detail.pl and see that only the
cart location shows.
4) Apply patch and view the item again in check detail and
opac-detail.pl.  You should now see the permanent location, and the cart
location in parenthesis.
5) Try removing descriptions from locations.  Codes should replace
descriptions when not available.  Nothing should show if a value (code
is missing.
6) Check another item that was not checked in.  You should see the
permanent location and not a secondary location.

Signed-off-by: Spencer <ssmith3@mckinneytexas.org>
Signed-off-by: Lisette Scheer <lisetteslatah@gmail.com>

Signed-off-by: Lisette Scheer <lisetteslatah@gmail.com>
Comment 25 Christopher Brannon 2018-09-27 18:33:20 UTC
Found a few more places to make this correction.
Comment 26 Christopher Brannon 2018-09-27 18:36:15 UTC
I have decided to make other corrections in other patches so these can go out quicker.  Returning to signed off.
Comment 27 Jonathan Druart 2018-09-30 23:08:24 UTC
Please resubmit without the indentation changes.
Comment 28 Christopher Brannon 2018-10-01 00:51:48 UTC
Created attachment 79684 [details] [review]
Bug 18355: Modifies location on details pages.

TEST PLAN:
1) Make sure ReturnToShelvingCart is set to Move.
2) Check in an item.
3) View the item in detail.pl and opac-detail.pl and see that only the
cart location shows.
4) Apply patch and view the item again in check detail and
opac-detail.pl.  You should now see the permanent location, and the cart
location in parenthesis.
5) Try removing descriptions from locations.  Codes should replace
descriptions when not available.  Nothing should show if a value (code
is missing.
6) Check another item that was not checked in.  You should see
the permanent location and not a secondary location.
Comment 29 Christopher Brannon 2018-10-01 00:53:53 UTC
(In reply to Jonathan Druart from comment #27)
> Please resubmit without the indentation changes.

Sorry.  Didn't understand this was an issue.  Please look and see if this is okay now.
Comment 30 Jonathan Druart 2018-10-01 14:24:46 UTC
Created attachment 79717 [details] [review]
[ALTERNATIVE-PATCH %] Bug 18355: Display permanent location with cart location
Comment 31 Jonathan Druart 2018-10-01 14:27:22 UTC
Hi Christopher,
I would suggest to move the display logic to the template for several reasons:
- the pl logic from the first patch is not easy to read
- not defined AV are handled properly by the template plugin (Koha::Template::Plugin::AuthorisedValues)
- as we are going to reuse this logic, we will move it to an include file to avoid copy/paste

Could you take a look at tell me what you think about it?
Comment 32 Christopher Brannon 2018-10-01 16:51:56 UTC
(In reply to Jonathan Druart from comment #31)
> Hi Christopher,
> I would suggest to move the display logic to the template for several
> reasons:
> - the pl logic from the first patch is not easy to read
> - not defined AV are handled properly by the template plugin
> (Koha::Template::Plugin::AuthorisedValues)
> - as we are going to reuse this logic, we will move it to an include file to
> avoid copy/paste
> 
> Could you take a look at tell me what you think about it?

Jonathan,
Thanks for the suggestion.  I have no problem trying to move it to the tt.  I put it in the pl because that is where it was already being done.

Sorry for the short code.  I opted for short code for efficiency but realize it isn't efficient for everyone else.  I will work on keeping things more readable.

Thank you for your input.  I will work on this.
Comment 33 Christopher Brannon 2018-10-01 18:58:01 UTC
(In reply to Jonathan Druart from comment #31)
> Hi Christopher,
> I would suggest to move the display logic to the template for several
> reasons:
> - the pl logic from the first patch is not easy to read
> - not defined AV are handled properly by the template plugin
> (Koha::Template::Plugin::AuthorisedValues)
> - as we are going to reuse this logic, we will move it to an include file to
> avoid copy/paste
> 
> Could you take a look at tell me what you think about it?

Something funky is going on in your patch.  item.location is resolving to a description while item.permanent_location is resolving to a code.  Thus, the two can't be compared.  Need to figure out where item.location is being defined, because when I look at the table, the code is definitely being stored in the table and not the description.
Comment 34 Jonathan Druart 2018-10-01 19:49:14 UTC
(In reply to Christopher Brannon from comment #33)
> (In reply to Jonathan Druart from comment #31)
> > Hi Christopher,
> > I would suggest to move the display logic to the template for several
> > reasons:
> > - the pl logic from the first patch is not easy to read
> > - not defined AV are handled properly by the template plugin
> > (Koha::Template::Plugin::AuthorisedValues)
> > - as we are going to reuse this logic, we will move it to an include file to
> > avoid copy/paste
> > 
> > Could you take a look at tell me what you think about it?
> 
> Something funky is going on in your patch.  item.location is resolving to a
> description while item.permanent_location is resolving to a code.  Thus, the
> two can't be compared.  Need to figure out where item.location is being
> defined, because when I look at the table, the code is definitely being
> stored in the table and not the description.

Can you confirm you applied *only* the second patch?
Comment 35 Christopher Brannon 2018-10-01 20:49:30 UTC
Yes.  I checked for my mods and they are not there.  Just yours.
Comment 36 Christopher Brannon 2018-10-01 20:50:15 UTC
I also did a restart just to be sure.
Comment 37 Christopher Brannon 2018-10-01 21:35:25 UTC
Created attachment 79786 [details] [review]
Bug 18355: Remove obsolete code from pl
Comment 38 Christopher Brannon 2018-10-01 21:38:25 UTC
I've accepted the alternative patch as the better way to go.  This patch will depend on fixing AuthorisedValues.pm, per our discussion, to display the code if a description doesn't exist.  That will have to be fixed before this is pushed.  :/
Comment 39 Christopher Brannon 2018-10-05 23:03:11 UTC
While this works properly, it will work better once 21503 is applied.  As is, AuthoriseValues.pm (plugin) will not fail to code.  21503 will fix that issue.
Comment 40 Christopher Brannon 2018-10-05 23:12:53 UTC
Created attachment 80153 [details] [review]
Bug 18355: Update opac-detail.pl/tt to show both

Adding dependancy of bug 21503 for templates to fully fail correctly.

To Test this patch:

1) Apply patch / restart_all
2) Check out an item.
3) Check item in with ReturnToShelvingCart set to Move.
4) View the item in opac-detail.pl and see the permanent location, and
the cart location in parentesis.
5) Try removing opac description from the cart and item sleving
location.  Refresh opac-detail.pl.  Should default to regular
description.  (If not, may need patch 21503 to test.)
6) To test for code replacement if regular and opac description are
missing, you WILL need patch 21503.
Comment 41 José-Mario Monteiro-Santos 2019-01-04 21:33:04 UTC
Created attachment 83665 [details] [review]
Bug 18355: Update opac-detail.pl/tt to show both

Adding dependancy of bug 21503 for templates to fully fail correctly.

To Test this patch:

1) Apply patch / restart_all
2) Check out an item.
3) Check item in with ReturnToShelvingCart set to Move.
4) View the item in opac-detail.pl and see the permanent location, and
the cart location in parentesis.
5) Try removing opac description from the cart and item sleving
location.  Refresh opac-detail.pl.  Should default to regular
description.  (If not, may need patch 21503 to test.)
6) To test for code replacement if regular and opac description are
missing, you WILL need patch 21503.

Signed-off-by: Jose-Mario <jose-mario.monteiro-santos@inLibro.com>
Comment 42 Marcel de Rooy 2019-02-15 07:37:18 UTC
The status of all attached patches is not very clear. Please obsolete what is not relevant any more.
Comment 43 Martin Renvoize 2019-11-25 10:02:33 UTC
Created attachment 95775 [details] [review]
Bug 18355: Display permanent location with cart location

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 44 Marcel de Rooy 2019-11-29 11:18:20 UTC
Created attachment 95879 [details] [review]
Bug 18355: Display permanent location with cart location

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 45 Marcel de Rooy 2019-11-29 11:18:26 UTC
Created attachment 95880 [details] [review]
Bug 18355: (QA follow-up) Rearrange comments, improve code

Comments were hard to read (for me) but tastes differ..

Code should reflect that permanent_location is a code and that location
may be already an authval. See detail.pl:
$item->{'location'} = $shelflocations->{$shelfcode} if ( defined( $shelfcode ) && defined($shelflocations) && exists( $shelflocations->{$shelfcode} ) );

Obviously, this kind of logic divided over two places should be reduced.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>