Bug 22551 - Stray "//" appears at bottom of opac-detail.tt
Summary: Stray "//" appears at bottom of opac-detail.tt
Status: RESOLVED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: OPAC (show other bugs)
Version: 18.11
Hardware: All All
: P5 - low normal (vote)
Assignee: Eric Phetteplace
QA Contact: Testopia
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2019-03-20 16:33 UTC by Eric Phetteplace
Modified: 2019-10-14 20:03 UTC (History)
5 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Trivial patch
Who signed the patch off:
Text to go in the release notes:
Version(s) released in:
19.05.00, 18.11.05


Attachments
screenshot of stray characters (26.58 KB, image/png)
2019-03-20 16:33 UTC, Eric Phetteplace
Details
patch (1.35 KB, patch)
2019-03-20 17:05 UTC, Eric Phetteplace
Details | Diff | Splinter Review
patch (1.69 KB, patch)
2019-04-08 23:08 UTC, Eric Phetteplace
Details | Diff | Splinter Review
Bug 22551: Stray "//" appears at bottom of opac-detail.tt (1.75 KB, patch)
2019-04-16 04:24 UTC, Hayley Mapley
Details | Diff | Splinter Review
Bug 22551: Stray "//" appears at bottom of opac-detail.tt (1.81 KB, patch)
2019-04-25 06:55 UTC, Martin Renvoize
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Eric Phetteplace 2019-03-20 16:33:36 UTC
Created attachment 86822 [details]
screenshot of stray characters

In the "SocialNetworks" JavaScript area of the OPAC display, there's this code (currently lines 1390-1393 of opac-detail.tt):

//<![CDATA[
{lang: '[% lang | html %]'}
//]]>
</script>

It seems like an opening <script> tag is missing (the closing one is orphaned) which results in the "//" characters displaying on the page towards the very bottom (see screenshot). It's not 100% clear to me what this code was intended to do but I think it was meant to be part of the old Google+ (removed from Koha earlier this year) script tag like this:

<script src="https://apis.google.com/js/plusone.js" type="text/javascript">
//<![CDATA[
{"lang":"en-US"}
//]]>
</script>

However, right now it's only effect is the stray "//"—the JS code is commented out and even if it wasn't I don't think declaring {"lang": "en-US"} as an object literal unassigned to any variable has any effect.

I can make a patch for this shortly.
Comment 1 Eric Phetteplace 2019-03-20 17:05:19 UTC
Created attachment 86824 [details] [review]
patch
Comment 2 Owen Leonard 2019-04-08 13:07:39 UTC
I think you're right to identify that section as obsolete, but I don't think that's what's causing the problem. There is a stray "//]]>" further down in the template. A "//<![CDATA[" marker was removed without cleaning up the ending marker.

Please also take a look at our guidelines for commit messages:

https://wiki.koha-community.org/wiki/Commit_messages
Comment 3 Eric Phetteplace 2019-04-08 22:59:27 UTC
I do see that other stray "//]]>" around line 1720 of opac-detail.tt but it is not what's causing the text to display—inspect an opac-detail page in your web browser, the segment I removed (just above an orphaned closing </script> tag and the Twitter script) falls outside any <script> tags which is why the browser renders it as a text node. That second stray "//]]>" is inside a very long <script> tag with several functions (OpacHighlightedWords, OPACShelfBrowser,IDreamBooksReviews, etc.) and thus not rendered as text.

In any case, I can re-submit a patch that removes both since that seems to be the right thing to do. It's not actually clear to me what's wrong with my commit message...I can be more descriptive but I thought I stated the bug number/name in the subject and outlined the testing plan already.
Comment 4 Eric Phetteplace 2019-04-08 23:08:20 UTC
Created attachment 87568 [details] [review]
patch
Comment 5 Hayley Mapley 2019-04-16 03:44:25 UTC
(In reply to Eric Phetteplace from comment #3)

> In any case, I can re-submit a patch that removes both since that seems to
> be the right thing to do. It's not actually clear to me what's wrong with my
> commit message...I can be more descriptive but I thought I stated the bug
> number/name in the subject and outlined the testing plan already.

It doesn't look like the bug number or description is in the first line of your patches, it should be. Also the patch should be descriptive, the commit you attached was:

'patch'

It should be:
'Bug 22551: Removed stray characters from opac-detail.tt

There are stray characters, for the reasons outlined in the bug report. This patch fixes that.

Test plan:
1) Note the extra characters on the template
2) Apply the patch
3) Verify the characters have disappeared' 

or something similar.

Not a dig, just trying to make what Owen said clearer :) I will test this patch now
Comment 6 Hayley Mapley 2019-04-16 04:23:25 UTC
(In reply to Hayley Mapley from comment #5)
> (In reply to Eric Phetteplace from comment #3)
> 
> > In any case, I can re-submit a patch that removes both since that seems to
> > be the right thing to do. It's not actually clear to me what's wrong with my
> > commit message...I can be more descriptive but I thought I stated the bug
> > number/name in the subject and outlined the testing plan already.
> 
> It doesn't look like the bug number or description is in the first line of
> your patches, it should be. Also the patch should be descriptive, the commit
> you attached was:
> 
> 'patch'
> 
> It should be:
> 'Bug 22551: Removed stray characters from opac-detail.tt
> 
> There are stray characters, for the reasons outlined in the bug report. This
> patch fixes that.
> 
> Test plan:
> 1) Note the extra characters on the template
> 2) Apply the patch
> 3) Verify the characters have disappeared' 
> 
> or something similar.
> 
> Not a dig, just trying to make what Owen said clearer :) I will test this
> patch now
(In reply to Eric Phetteplace from comment #3)
> I do see that other stray "//]]>" around line 1720 of opac-detail.tt but it
> is not what's causing the text to display—inspect an opac-detail page in
> your web browser, the segment I removed (just above an orphaned closing
> </script> tag and the Twitter script) falls outside any <script> tags which
> is why the browser renders it as a text node. That second stray "//]]>" is
> inside a very long <script> tag with several functions
> (OpacHighlightedWords, OPACShelfBrowser,IDreamBooksReviews, etc.) and thus
> not rendered as text.
> 
> In any case, I can re-submit a patch that removes both since that seems to
> be the right thing to do. It's not actually clear to me what's wrong with my
> commit message...I can be more descriptive but I thought I stated the bug
> number/name in the subject and outlined the testing plan already.

(In reply to Hayley Mapley from comment #5)
> (In reply to Eric Phetteplace from comment #3)
> 
> > In any case, I can re-submit a patch that removes both since that seems to
> > be the right thing to do. It's not actually clear to me what's wrong with my
> > commit message...I can be more descriptive but I thought I stated the bug
> > number/name in the subject and outlined the testing plan already.
> 
> It doesn't look like the bug number or description is in the first line of
> your patches, it should be. Also the patch should be descriptive, the commit
> you attached was:
> 
> 'patch'
> 
> It should be:
> 'Bug 22551: Removed stray characters from opac-detail.tt
> 
> There are stray characters, for the reasons outlined in the bug report. This
> patch fixes that.
> 
> Test plan:
> 1) Note the extra characters on the template
> 2) Apply the patch
> 3) Verify the characters have disappeared' 
> 
> or something similar.
> 
> Not a dig, just trying to make what Owen said clearer :) I will test this
> patch now

Ahh I see. Did you remove the description when you attached the commit? I see it now that I am amending to your commit that your commit message is perfect.
Comment 7 Hayley Mapley 2019-04-16 04:24:28 UTC
Created attachment 88041 [details] [review]
Bug 22551: Stray "//" appears at bottom of opac-detail.tt

Removes a couple stray "//<![CDATA[" like comments which are artifacts of other
code that since been removed. One of the comments causes two forward slashes to
appear at the bottom of each OPAC detail page.

Testing plan:

- enable SocialNetworks
- visit any OPAC detail page, observe "//" at bottom of page
- apply patch
- revisit page, slashes should be gone

Signed-off-by: Hayley Mapley <hayleymapley@catalyst.net.nz>
Comment 8 Eric Phetteplace 2019-04-16 22:25:30 UTC
Thank you, Hayley, that helps me understand what I did wrong. You're exactly right—I didn't enter a description or "comment" when I selected "add an attachment" in bugzilla because I thought the patch file itself was the only thing that mattered; my patch looks the same as yours it's just I didn't fill out the Bugzilla form correctly. I'll do that next time!
Comment 9 Martin Renvoize 2019-04-25 06:55:44 UTC
Created attachment 88688 [details] [review]
Bug 22551: Stray "//" appears at bottom of opac-detail.tt

Removes a couple stray "//<![CDATA[" like comments which are artifacts of other
code that since been removed. One of the comments causes two forward slashes to
appear at the bottom of each OPAC detail page.

Testing plan:

- enable SocialNetworks
- visit any OPAC detail page, observe "//" at bottom of page
- apply patch
- revisit page, slashes should be gone

Signed-off-by: Hayley Mapley <hayleymapley@catalyst.net.nz>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 10 Martin Renvoize 2019-04-25 06:56:19 UTC
It's great to see such teamwork going on here guys, well done.

Patch works well and causes no regressions.. Passing QA
Comment 11 Martin Renvoize 2019-04-25 06:57:46 UTC
As an addendum to the advice given here, you might find git-bz helpful Eric. https://wiki.koha-community.org/wiki/Git_bz_configuration

It's automates the creation and upload of patches to bugzilla (as well as making the application of such patches to your development environment very simple too)
Comment 12 Nick Clemens 2019-04-25 11:56:47 UTC
Awesome work all!

Pushed to master for 19.05
Comment 13 Martin Renvoize 2019-04-26 15:25:26 UTC
Pushed to 18.11.x for 18.11.05