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.
Created attachment 61978 [details] [review] Bug 18355: Modifies location on detail pages.
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.
I'm not sure how to fix the subject in the attachment.
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.
Created attachment 61981 [details] [review] Attachment to Bug 18355 - Permanent location should show with cart location Bug 18355: Modifies location on detail pages.
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>
Setting version to master (tested on master)
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)"
Created attachment 62999 [details] [review] Bug 18355: Modifies location on detail pages.
(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.
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
Hi Christopher Can you please merge your patches with the up to date master branch? Cheers Alex
(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.
Created attachment 66432 [details] [review] Bug 18355: Modifies location on detail pages.
(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. :)
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>
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
(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.
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
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.
Updated patch (FINALLY!) and updated test plan. Responded to Nick's concerns.
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.
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>
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>
Found a few more places to make this correction.
I have decided to make other corrections in other patches so these can go out quicker. Returning to signed off.
Please resubmit without the indentation changes.
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.
(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.
Created attachment 79717 [details] [review] [ALTERNATIVE-PATCH %] Bug 18355: Display permanent location with cart location
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?
(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.
(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.
(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?
Yes. I checked for my mods and they are not there. Just yours.
I also did a restart just to be sure.
Created attachment 79786 [details] [review] Bug 18355: Remove obsolete code from pl
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. :/
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.
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.
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>
The status of all attached patches is not very clear. Please obsolete what is not relevant any more.
Created attachment 95775 [details] [review] Bug 18355: Display permanent location with cart location Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
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>
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>
Currently awaiting QA of bug 21466
Nice work everyone! Pushed to master for 20.05