Bug 17221 - Orphan comma in shelf browser
Summary: Orphan comma in shelf browser
Status: Pushed to oldstable
Alias: None
Product: Koha
Classification: Unclassified
Component: OPAC (show other bugs)
Version: master
Hardware: All All
: P5 - low minor (vote)
Assignee: Katrin Fischer
QA Contact: Testopia
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2016-08-29 20:49 UTC by Eric Phetteplace
Modified: 2020-03-18 17:16 UTC (History)
5 users (show)

See Also:
Change sponsored?: Sponsored
Patch complexity: Trivial patch
Who signed the patch off:
Text to go in the release notes:
Version(s) released in:
20.05.00, 19.11.04, 19.05.09


Attachments
screenshot of bug (188.25 KB, image/png)
2016-08-29 20:49 UTC, Eric Phetteplace
Details
patch (1.78 KB, patch)
2016-08-29 21:11 UTC, Eric Phetteplace
Details | Diff | Splinter Review
Bug 17221: Remove orphan commas in shelf browser (1.89 KB, patch)
2020-01-12 11:07 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 17221: (follow-up) Orphan comma in shelf browser (2.88 KB, patch)
2020-01-12 11:08 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 17221: Remove orphan commas in shelf browser (1.94 KB, patch)
2020-02-13 14:05 UTC, Owen Leonard
Details | Diff | Splinter Review
Bug 17221: (follow-up) Orphan comma in shelf browser (2.93 KB, patch)
2020-02-13 14:05 UTC, Owen Leonard
Details | Diff | Splinter Review
Bug 17221: Remove orphan commas in shelf browser (2.01 KB, patch)
2020-02-18 14:24 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 17221: (follow-up) Orphan comma in shelf browser (3.00 KB, patch)
2020-02-18 14:24 UTC, Jonathan Druart
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Eric Phetteplace 2016-08-29 20:49:56 UTC
Created attachment 54997 [details]
screenshot of bug

Example screenshot attached, here's the bib record in our catalog:

https://library.cca.edu/cgi-bin/koha/opac-detail.pl?biblionumber=21729&shelfbrowse_itemnumber=34420#holdings

Basically, since starting_homebranch is null in the template file, nothing is printed out, but the next IF condition wrongly assumes that the first must have been true and prints out a comma before it's text string. Steps to recreate:

- in settings, set ShelfBrowserUsesHomeBranch to "Don't Use"
- set ShelfBrowserUsesLocation to "Use"
- visit an item, click the "Browse shelf" link in the Holdings' tab's Call Number column
- note the extraneous comma in the <h5> tag that appears

Patch coming shortly, it appears that simply adding an additional condition around the comma itself should resolve this problem.
Comment 1 Eric Phetteplace 2016-08-29 21:11:09 UTC
Created attachment 54998 [details] [review]
patch

patch anticipates same problem with collection code in list, too
Comment 2 Owen Leonard 2016-08-30 15:17:51 UTC
If you have ShelfBrowserUsesCcode, ShelfBrowserUsesHomeBranch, and ShelfBrowserUsesLocation all set to 'Use," but you are browsing an item which doesn't have a shelving location there are extra commas:

" Browsing Athens Shelves , , Collection code: Juvenile fiction "

I wonder if that scenario can be handled as well?

By the way, please include a test plan in your commit messages. And if you would like to be credited by name in Koha's release notes, about page, etc., configure git with your real name so it appears in the patch.

Thanks for the patch!
Comment 3 Eric Phetteplace 2016-08-30 22:47:39 UTC
Thanks for the feedback Owen, I'll take another look & develop a better testing plan. It's difficult because there are so many permutations of settings and then fields that could be empty on the item records but that's all the more reason to have a well-defined plan.

I want to be credited to my GitHub username (phette23) so the lack of my real name in the commit notes is intentional.
Comment 4 Katrin Fischer 2016-09-04 19:22:47 UTC
I think you can still get credited if it's not a direct match between your commit message and your user name.
Comment 5 Katrin Fischer 2020-01-12 11:07:57 UTC
Created attachment 97267 [details] [review]
Bug 17221: Remove orphan commas in shelf browser

This patch adds more conditions to the shelf browser template
such that the displayed text reads as a proper list no matter
what combination of system preferences are utilized

Sponsored-by: California College of the Arts

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 6 Katrin Fischer 2020-01-12 11:08:00 UTC
Created attachment 97268 [details] [review]
Bug 17221: (follow-up) Orphan comma in shelf browser

This further improves the display if no location or no
collection has been set. Example output:

Browsing Centerville shelves, Collection: Non-fiction

Also changes 'Collection code' to 'collection' and takes
care of additional spaces before the comma.

To test:
- Make sure your OpacShelfBrowser pref is set to "Show"
- Search for a record with items and callnumbers in your OPAC
- "Browse shelf"
- Navigate back and forth, verify the information showing on
  top of the list is correctly formatted
- Try different combinations for the following prefs:
  - ShelfBrowserUsesCcode
  - ShelfBrowserUsesHomeBranch
  - ShelfBrowserUsesLocation
- Verify the display is always nicely formatted

Note: fixing capitalization here is tricky, as it also
depends on what was displayed before. If you have no
homebranch, you would want "Shelving...", if you have one
we'd like "shelving...". Left as I found it for now.

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 7 Owen Leonard 2020-02-13 14:05:38 UTC
Created attachment 98830 [details] [review]
Bug 17221: Remove orphan commas in shelf browser

This patch adds more conditions to the shelf browser template
such that the displayed text reads as a proper list no matter
what combination of system preferences are utilized

Sponsored-by: California College of the Arts

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>

Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Comment 8 Owen Leonard 2020-02-13 14:05:40 UTC
Created attachment 98831 [details] [review]
Bug 17221: (follow-up) Orphan comma in shelf browser

This further improves the display if no location or no
collection has been set. Example output:

Browsing Centerville shelves, Collection: Non-fiction

Also changes 'Collection code' to 'collection' and takes
care of additional spaces before the comma.

To test:
- Make sure your OpacShelfBrowser pref is set to "Show"
- Search for a record with items and callnumbers in your OPAC
- "Browse shelf"
- Navigate back and forth, verify the information showing on
  top of the list is correctly formatted
- Try different combinations for the following prefs:
  - ShelfBrowserUsesCcode
  - ShelfBrowserUsesHomeBranch
  - ShelfBrowserUsesLocation
- Verify the display is always nicely formatted

Note: fixing capitalization here is tricky, as it also
depends on what was displayed before. If you have no
homebranch, you would want "Shelving...", if you have one
we'd like "shelving...". Left as I found it for now.

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>

Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Comment 9 Jonathan Druart 2020-02-18 14:24:48 UTC
Created attachment 99188 [details] [review]
Bug 17221: Remove orphan commas in shelf browser

This patch adds more conditions to the shelf browser template
such that the displayed text reads as a proper list no matter
what combination of system preferences are utilized

Sponsored-by: California College of the Arts

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>

Signed-off-by: Owen Leonard <oleonard@myacpl.org>

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 10 Jonathan Druart 2020-02-18 14:24:51 UTC
Created attachment 99189 [details] [review]
Bug 17221: (follow-up) Orphan comma in shelf browser

This further improves the display if no location or no
collection has been set. Example output:

Browsing Centerville shelves, Collection: Non-fiction

Also changes 'Collection code' to 'collection' and takes
care of additional spaces before the comma.

To test:
- Make sure your OpacShelfBrowser pref is set to "Show"
- Search for a record with items and callnumbers in your OPAC
- "Browse shelf"
- Navigate back and forth, verify the information showing on
  top of the list is correctly formatted
- Try different combinations for the following prefs:
  - ShelfBrowserUsesCcode
  - ShelfBrowserUsesHomeBranch
  - ShelfBrowserUsesLocation
- Verify the display is always nicely formatted

Note: fixing capitalization here is tricky, as it also
depends on what was displayed before. If you have no
homebranch, you would want "Shelving...", if you have one
we'd like "shelving...". Left as I found it for now.

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>

Signed-off-by: Owen Leonard <oleonard@myacpl.org>

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 11 Martin Renvoize 2020-02-19 11:33:59 UTC
Nice work everyone!

Pushed to master for 20.05
Comment 12 Joy Nelson 2020-03-13 00:16:39 UTC
Pushed to 19.11.x branch for 19.11.04
Comment 13 Lucas Gass 2020-03-18 17:16:56 UTC
backported to 19.05.x for 19.05.09