Bug 20260 - Use CodeMirror for the SQL reports editor
Summary: Use CodeMirror for the SQL reports editor
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Reports (show other bugs)
Version: Main
Hardware: All All
: P5 - low enhancement
Assignee: Owen Leonard
QA Contact: Katrin Fischer
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2018-02-21 10:27 UTC by Magnus Enger
Modified: 2022-11-11 10:34 UTC (History)
6 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Small patch
Documentation contact:
Documentation submission:
Text to go in the release notes:
Version(s) released in:
Circulation function:


Attachments
Bug 20260: Use CodeMirror for the SQL reports editor (38.34 KB, patch)
2018-05-08 17:57 UTC, Owen Leonard
Details | Diff | Splinter Review
Bug 20260: Use CodeMirror for the SQL reports editor (85.99 KB, patch)
2018-10-10 16:46 UTC, Owen Leonard
Details | Diff | Splinter Review
Bug 20260: Use CodeMirror for the SQL reports editor (86.05 KB, patch)
2018-10-11 19:45 UTC, Michal Denar
Details | Diff | Splinter Review
Bug 21541: Use raw filter for report results (1.62 KB, patch)
2018-10-11 21:36 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 20260: Use CodeMirror for the SQL reports editor (86.09 KB, patch)
2018-10-11 21:38 UTC, Katrin Fischer
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Magnus Enger 2018-02-21 10:27:29 UTC
We are already using CodeMirror (http://codemirror.net) somewhere. I would guess in Rancor, the advanced cataloguing editor. How about also using it for syntax highlighting when editing the SQL of an SQL report?
Comment 1 Magnus Enger 2018-02-21 10:27:56 UTC
CodeMirror in SQL mode: http://codemirror.net/mode/sql/index.html
Comment 2 Katrin Fischer 2018-02-23 08:51:23 UTC
+1 !
Comment 3 Owen Leonard 2018-05-08 17:57:02 UTC
Created attachment 75180 [details] [review]
Bug 20260: Use CodeMirror for the SQL reports editor

This patch adds CodeMirror syntax highlighting to the SQL reports
editor. Newly added is a file containing the sql-specific syntax
highlighting code, in both compressed and uncompressed versions.

Unrelated change: Some JavaScript has been moved from inside the body of
the page to the footer.

To test, apply the patch and go to Reports -> Saved SQL reports

- Edit an existing report and confirm that the SQL is correctly
  highlighted and that there are no display problems when editing.
- Confirm that changes are saved successfully.
- Perform the same test when creating a new SQL report.
- View an existing report and confirm that the SQL is syntax-highlighted
  but is read-only.
Comment 4 Owen Leonard 2018-05-08 18:00:02 UTC
I've tried to also address Bug 19399 in these changes. If testers agree, please mark that bug as a duplicate.
Comment 5 Mark Tompsett 2018-05-08 18:15:49 UTC
Comment on attachment 75180 [details] [review]
Bug 20260: Use CodeMirror for the SQL reports editor

Review of attachment 75180 [details] [review]:
-----------------------------------------------------------------

::: koha-tmpl/intranet-tmpl/prog/en/modules/reports/guided_reports_start.tt
@@ +31,4 @@
>  [%- END -%]</title>
>  
>  [% INCLUDE 'doc-head-close.inc' %]
> +[% Asset.css("lib/codemirror/codemirror.css") %]

Where is this?

@@ +953,4 @@
>          [% INCLUDE 'datatables.inc' %]
>          [% INCLUDE 'columns_settings.inc' %]
>      [% END %]
> +    [% Asset.js( "lib/codemirror/codemirror-compressed.js" ) %]

Where is this?
Comment 6 Mark Tompsett 2018-05-08 18:17:52 UTC
(In reply to M. Tompsett from comment #5)
> Where is this?

DOH! find . | grep codemirror and comment #0.
Comment 7 Mark Tompsett 2018-05-08 18:31:59 UTC
It was beautiful (edit and create), but the show is empty.
Inspecting the source, I see a display: none with the correct SQL outside the CodeMirror divs. I think Failed QA is appropriate, because of that.
Comment 8 Owen Leonard 2018-10-10 16:46:13 UTC
Created attachment 80367 [details] [review]
Bug 20260: Use CodeMirror for the SQL reports editor

This patch adds CodeMirror syntax highlighting to the SQL reports
editor. Newly added is a file containing the sql-specific syntax
highlighting code, in both compressed and uncompressed versions.

Unrelated changes:

 - Some JavaScript has been moved from inside the body of the page to
   the footer.
 - Paths to some assets have been corrected to no use relative paths.

To test, apply the patch and go to Reports -> Saved SQL reports

- Edit an existing report and confirm that the SQL is correctly
  highlighted and that there are no display problems when editing.
- Confirm that changes are saved successfully.
- Perform the same test when creating a new SQL report.
- View an existing report and confirm that the SQL is syntax-highlighted
  but is read-only.
Comment 9 Michal Denar 2018-10-11 19:45:48 UTC
Created attachment 80419 [details] [review]
Bug 20260: Use CodeMirror for the SQL reports editor

This patch adds CodeMirror syntax highlighting to the SQL reports
editor. Newly added is a file containing the sql-specific syntax
highlighting code, in both compressed and uncompressed versions.

Unrelated changes:

 - Some JavaScript has been moved from inside the body of the page to
   the footer.
 - Paths to some assets have been corrected to no use relative paths.

To test, apply the patch and go to Reports -> Saved SQL reports

- Edit an existing report and confirm that the SQL is correctly
  highlighted and that there are no display problems when editing.
- Confirm that changes are saved successfully.
- Perform the same test when creating a new SQL report.
- View an existing report and confirm that the SQL is syntax-highlighted
  but is read-only.

Signed-off-by: Michal Denar <black23@gmail.com>
Comment 10 Katrin Fischer 2018-10-11 21:34:59 UTC
I really like that now when you make a mistake and it gives you a line number to look at, there are actually line numbers now!
Comment 11 Katrin Fischer 2018-10-11 21:36:57 UTC
Created attachment 80431 [details] [review]
Bug 21541: Use raw filter for report results

To test:
 1 - Create new report
 SELECT CONCAT('<a href="/cgi-bin/koha/members/moremember.pl?borrowernumber=',borrowernumber,'">',"",firstname,"",surname,'</a>')
 AS people
 FROM borrowers
 2 - Run this report
 3 - Note the results contain raw text, they are not hyperlinks
 4 - Apply patch
 5 - Reload the results
 6 - They should now be links

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 12 Katrin Fischer 2018-10-11 21:38:09 UTC
Created attachment 80432 [details] [review]
Bug 20260: Use CodeMirror for the SQL reports editor

This patch adds CodeMirror syntax highlighting to the SQL reports
editor. Newly added is a file containing the sql-specific syntax
highlighting code, in both compressed and uncompressed versions.

Unrelated changes:

 - Some JavaScript has been moved from inside the body of the page to
   the footer.
 - Paths to some assets have been corrected to no use relative paths.

To test, apply the patch and go to Reports -> Saved SQL reports

- Edit an existing report and confirm that the SQL is correctly
  highlighted and that there are no display problems when editing.
- Confirm that changes are saved successfully.
- Perform the same test when creating a new SQL report.
- View an existing report and confirm that the SQL is syntax-highlighted
  but is read-only.

Signed-off-by: Michal Denar <black23@gmail.com>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 13 Tomás Cohen Arazi (tcohen) 2018-10-12 18:59:41 UTC
Pushed for 18.11. Thanks all!
Comment 14 Martin Renvoize (ashimema) 2018-10-16 11:46:05 UTC
Just come across this, lovely enhancement! :)

Won't be backported however.. too large a change.
Comment 15 Andrew Isherwood 2018-10-16 12:02:22 UTC
Any reason why this couldn't also be used for intranet/opacuserjs & intranet/opacusercss?
Comment 16 Magnus Enger 2018-10-16 15:17:47 UTC
(In reply to Andrew Isherwood from comment #15)
> Any reason why this couldn't also be used for intranet/opacuserjs &
> intranet/opacusercss?

Sounds like an excellent idea to me. Create a new bug report for it?
Comment 17 Andrew Isherwood 2018-10-16 15:30:57 UTC
Done :) https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=21582