Bug 7618 - HTML special characters break result table and incorrectly show SQL in Run This Report
Summary: HTML special characters break result table and incorrectly show SQL in Run Th...
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Reports (show other bugs)
Version: 3.6
Hardware: All All
: P5 - low minor (vote)
Assignee: David Cook
QA Contact: Ian Walls
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-02-29 04:55 UTC by David Cook
Modified: 2013-12-05 19:57 UTC (History)
3 users (show)

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


Attachments
Bug 7618 HTML Escaped SQL (1.27 KB, patch)
2012-03-02 00:13 UTC, David Cook
Details | Diff | Splinter Review
Bug 7618 HTML Escaped SQL (master) (1.17 KB, patch)
2012-03-12 23:28 UTC, David Cook
Details | Diff | Splinter Review
Bug 7618 Escape HTML special characters for SQL displayed above results in Report module (1.18 KB, patch)
2012-03-12 23:48 UTC, Chris Cormack
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description David Cook 2012-02-29 04:55:43 UTC
The HTML special characters (often angle brackets in the case of older marcxml look-ups) in guided_reports_start.tmpl need to be escaped, so that the SQL displays properly and so that the results table properly forms.
Comment 1 David Cook 2012-03-02 00:13:13 UTC Comment hidden (obsolete)
Comment 2 Jared Camins-Esakov 2012-03-12 11:45:51 UTC
This patch is for Koha 3.2 which is no longer maintained by the community. Also, reports are often written which include links (using <a href=...> concatenated into the results). This patch would break those links. Marking failed QA.
Comment 3 David Cook 2012-03-12 22:29:54 UTC
I'm not sure that I understand why/how Koha 3.2 wouldn't be supported by the community (am I not a member of the community?), since the branch is still available via the git. 

Also, it appears that you are incorrect about the patch breaking the links in the report. If you look at guided_reports_start.tmpl, you'll see that I'm just escaping the HTML characters in the SQL that is displayed above the results. The HTML in the actual results still works perfectly fine.
Comment 4 Jared Camins-Esakov 2012-03-12 22:34:41 UTC
David,

Sorry, I misunderstood what it did! It doesn't apply, so I wasn't able to test it. What I meant by "is no longer maintained" is that no one has stepped forward to act as release maintainer for Koha 3.2, so there will be no further releases in 3.2.x (unless you want to volunteer, which would be awesome!).

Regards,
Jared
Comment 5 Chris Cormack 2012-03-12 22:38:27 UTC
(In reply to comment #3)
> I'm not sure that I understand why/how Koha 3.2 wouldn't be supported by the
> community (am I not a member of the community?), since the branch is still
> available via the git. 
> 

Not supported, maintained. It's still available on git, but has been end of lifed. There will be no more 3.2.x releases ever. 

The 2 stable branches are 3.4.x and 3.6.x .. the current stable releases are 3.4.7 and 3.6.4. The last release for 3.2.x was 3.2.11 3 months ago.

When 3.8.0 is released in april, the 3.4.x branch will be end of lifed also.

We keep 2 stable branches and 1 development branch running.

> Also, it appears that you are incorrect about the patch breaking the links
> in the report. If you look at guided_reports_start.tmpl, you'll see that I'm
> just escaping the HTML characters in the SQL that is displayed above the
> results. The HTML in the actual results still works perfectly fine.

There are no longer any .tmpl files in Koha, since 3.4.0 we switched to template toolkit. This is a major reason this patch will not apply for master. Patches should be written to be applied to master, then cherry-picked by the release maintainer to the appropriate stable branch.

If this patch was rewritten to apply to the master (development branch) then it could be backported to 3.4.x and 3.6.x and be in the next releases of those. As well as being in the next feature release 3.8.0.

Hope this clears it up.
Comment 6 David Cook 2012-03-12 22:44:54 UTC
(In reply to comment #5)
> (In reply to comment #3)
> > I'm not sure that I understand why/how Koha 3.2 wouldn't be supported by the
> > community (am I not a member of the community?), since the branch is still
> > available via the git. 
> > 
> 
> Not supported, maintained. It's still available on git, but has been end of
> lifed. There will be no more 3.2.x releases ever. 
> 
> The 2 stable branches are 3.4.x and 3.6.x .. the current stable releases are
> 3.4.7 and 3.6.4. The last release for 3.2.x was 3.2.11 3 months ago.
> 
> When 3.8.0 is released in april, the 3.4.x branch will be end of lifed also.
> 
> We keep 2 stable branches and 1 development branch running.
> 
> > Also, it appears that you are incorrect about the patch breaking the links
> > in the report. If you look at guided_reports_start.tmpl, you'll see that I'm
> > just escaping the HTML characters in the SQL that is displayed above the
> > results. The HTML in the actual results still works perfectly fine.
> 
> There are no longer any .tmpl files in Koha, since 3.4.0 we switched to
> template toolkit. This is a major reason this patch will not apply for
> master. Patches should be written to be applied to master, then
> cherry-picked by the release maintainer to the appropriate stable branch.
> 
> If this patch was rewritten to apply to the master (development branch) then
> it could be backported to 3.4.x and 3.6.x and be in the next releases of
> those. As well as being in the next feature release 3.8.0.
> 
> Hope this clears it up.

Thanks Jared and Chris. That really does clear things up a lot. 

I've taken a look at the master code, and the fix looks conceptually the same, despite the change in template management. Should I upload a new patch for the master branch on this same bug (after changing the version details)?
Comment 7 Chris Cormack 2012-03-12 22:48:06 UTC
(In reply to comment #6)

> 
> I've taken a look at the master code, and the fix looks conceptually the
> same, despite the change in template management. Should I upload a new patch
> for the master branch on this same bug (after changing the version details)?

That would be perfect in TT to filter you would do
| html

eg [% sql | html %]
Comment 8 David Cook 2012-03-12 23:28:10 UTC Comment hidden (obsolete)
Comment 9 Chris Cormack 2012-03-12 23:48:31 UTC
Created attachment 8193 [details] [review]
Bug 7618 Escape HTML special characters for SQL displayed above results in Report module

Signed-off-by: Chris Cormack <chrisc@catalyst.net.nz>
Comment 10 Chris Cormack 2012-03-12 23:50:17 UTC
Small change, works well, signed off
Comment 11 Ian Walls 2012-03-14 14:24:09 UTC
Just adds an html filter to the SQL.  Safe and easy, marking as Passed QA.
Comment 12 Paul Poulain 2012-03-16 10:55:16 UTC
Patch pushed, and welcomed David, you're the 178th dev to have a patch pushed ! (added you on about page)
Comment 13 Jared Camins-Esakov 2012-05-23 23:58:09 UTC
This patch will be in 3.6.5.