When looking at the items tab in the staff client, the comma between the 245$a and the 245$b causes things to look a bit messy sometimes. I think we can safely remove that comma and it will greatly improve the display.
Created attachment 6288 [details] [review] Proposed fix Because I can't just take out one comma, I reworked the way moredetail.pl fetches and outputs biblio data based on the way it does in detail.pl. This allows for simpler output in the template. In the process I've added subtitle information to the template. The before and after the patch the display should look the same except for the change in the way author and subtitle are displayed.
Created attachment 6289 [details] [review] Revised fix Revision adds line break for readability
Created attachment 6291 [details] [review] [SIGNED-OFF] Fix for Bug 7121 - remove comma on items info Because I can't just take out one comma, I reworked the way moredetail.pl fetches and outputs biblio data based on the way it does in detail.pl. This allows for simpler output in the template. In the process I've added subtitle information to the template. Revision adds line break for readability
this patch requires keyword to marc mappings for subtitles: so: subtitle 245 $b subtitle 245 $p subtitle 245 $n
sorry that should be as follows: subtitle 245 b parts 245 p number 245 n medium 245 h
But it works without the mapping as well - it just doesn't show the subtitle, which it didn't show before so this patch is an improvement over the way it was. Nicole
except he explicitly stated that he made a change to the subtitle display, which you can't test without the mappings.
I'm confused. Are you asking for more tests? Or saying it doesn't work, cause it looks to me like my mappings are showing up right. Nicole
No, what I'm saying is, that the subtitle view won't work unless you have the mapping. When I went to test it, I missed that you had already signed off on it, and in my install the subtitles weren't showing up, which led me to dig deeper. Since the bug states that subtitles are supposed to show on moredetail.pl, I asked about the circumstances of their display, and found out that for the subtitles to display, you have to have the mappings. I added them, and the subtitles were displayed. I would have considered it reasonable to overturn your signoff had I not been able to make the subtitles work in my test install, but since my testing shows they are ok with proper configuration, I don't see a problem with letting it stand and concur with your findings that the patch is ok for inclusion. The note also serves as a heads up for QA, who may or may not have the mappings in their system.
QA comment: prove xt/author/translatable-templates.t xt/author/translatable-templates.t .. ok prove xt/author/valid-templates.t xt/author/valid-templates.t .. ok perlcritic catalogue/moredetail.pl catalogue/moredetail.pl source OK passed QA patch pushed, please test
Included in 3.6 prior to 3.6.5.