Bug 37270 - Deleting a report from the actions menu on a list of saved reports does not work
Summary: Deleting a report from the actions menu on a list of saved reports does not work
Status: Pushed to main
Alias: None
Product: Koha
Classification: Unclassified
Component: Reports (show other bugs)
Version: Main
Hardware: All All
: P5 - low major
Assignee: Owen Leonard
QA Contact: Martin Renvoize
URL:
Keywords:
Depends on: 36192
Blocks:
  Show dependency treegraph
 
Reported: 2024-07-07 22:36 UTC by Phil Ringnalda
Modified: 2024-07-25 11:23 UTC (History)
0 users

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


Attachments
Bug 37270: Fix deletion of saved reports (7.00 KB, patch)
2024-07-09 13:53 UTC, Owen Leonard
Details | Diff | Splinter Review
Bug 37270: Fix deletion of saved reports (7.05 KB, patch)
2024-07-09 15:19 UTC, Phil Ringnalda
Details | Diff | Splinter Review
Bug 37270: Fix deletion of saved reports (7.12 KB, patch)
2024-07-17 15:19 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 37270: (follow-up) Fix mixed quotes in preview modal JS (5.41 KB, patch)
2024-07-18 16:55 UTC, Owen Leonard
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Phil Ringnalda 2024-07-07 22:36:31 UTC
You need at least one saved report to see this. Reports - Use saved - for any listed report in the Actions column click the up-arrow next to Run and choose Delete. Confirm that you want to delete, then nothing happens.

Our JS thinks that the DOM is 

<form>
</form>
<a href="#" class="delete" title="Delete this saved report"><i class="fa fa-trash-can"></i> Delete</a>

And attaches a click handler to .delete at https://git.koha-community.org/Koha-community/Koha/src/commit/10853d1edc9c31bcef096ffa514bb3025e0a760e/koha-tmpl/intranet-tmpl/prog/en/modules/reports/guided_reports_start.tt#L2252 which does `return $(this).siblings('form').submit()`.

That fails because there's already a form open above that form, and you can't nest forms, so the form we want to submit doesn't exist because the browser has ignored that tag.

Now that we have form-submit.js, we should just use it instead.
Comment 1 Owen Leonard 2024-07-09 13:53:27 UTC
Created attachment 168653 [details] [review]
Bug 37270: Fix deletion of saved reports

This patch fixes deletion of saved reports in two instances which
were broken by the CSRF changes: From the dropdown menu in the table of
saved reports, and in the modal window which appears if you click the
"Preview" dropdown menu in the table of saved reports.

The patch also makes a minor change to form-submit.js so that the event
handler will attach to dynamically-generated elements (as is the case
with the preview modal).

To test, apply the patch and go to Reports -> Use saved.

- Add one or more reports if necessary.
- In the table of reports, click the secondary dropdown link in the
  "Run" button.
- Click "Delete." You should be asked to confirm, and confirming should
  correctly delete the report.
- Now test the "Preview SQL" link in the menu.
- A modal window should appear showing you the SQL of the report.
- In the footer of the modal, test the "Delete" button.

Sponsored-by: Athens County Public Libraries
Comment 2 Owen Leonard 2024-07-09 13:54:31 UTC
"FAIL	koha-tmpl/intranet-tmpl/prog/en/modules/reports/guided_reports_start.tt
   FAIL	  forbidden patterns
		forbidden pattern: simple-quote string (line 2461)"

I think this isn't correct in this situation.
Comment 3 Phil Ringnalda 2024-07-09 15:05:18 UTC
That failure made me look at what that code was trying to do (since it's certainly obscure that the start of that single quote is 15 lines above), which made me discover that SQL preview actually has actions and is thus what I want to do first when I can't remember whether a forgotten report named 'tempfoo' is actually junk, so correct or not it was certainly useful to me :)
Comment 4 Phil Ringnalda 2024-07-09 15:19:50 UTC
Created attachment 168667 [details] [review]
Bug 37270: Fix deletion of saved reports

This patch fixes deletion of saved reports in two instances which
were broken by the CSRF changes: From the dropdown menu in the table of
saved reports, and in the modal window which appears if you click the
"Preview" dropdown menu in the table of saved reports.

The patch also makes a minor change to form-submit.js so that the event
handler will attach to dynamically-generated elements (as is the case
with the preview modal).

To test, apply the patch and go to Reports -> Use saved.

- Add one or more reports if necessary.
- In the table of reports, click the secondary dropdown link in the
  "Run" button.
- Click "Delete." You should be asked to confirm, and confirming should
  correctly delete the report.
- Now test the "Preview SQL" link in the menu.
- A modal window should appear showing you the SQL of the report.
- In the footer of the modal, test the "Delete" button.

Sponsored-by: Athens County Public Libraries
Signed-off-by: Phil Ringnalda <phil@chetcolibrary.org>
Comment 5 Martin Renvoize 2024-07-17 15:19:35 UTC
Created attachment 169091 [details] [review]
Bug 37270: Fix deletion of saved reports

This patch fixes deletion of saved reports in two instances which
were broken by the CSRF changes: From the dropdown menu in the table of
saved reports, and in the modal window which appears if you click the
"Preview" dropdown menu in the table of saved reports.

The patch also makes a minor change to form-submit.js so that the event
handler will attach to dynamically-generated elements (as is the case
with the preview modal).

To test, apply the patch and go to Reports -> Use saved.

- Add one or more reports if necessary.
- In the table of reports, click the secondary dropdown link in the
  "Run" button.
- Click "Delete." You should be asked to confirm, and confirming should
  correctly delete the report.
- Now test the "Preview SQL" link in the menu.
- A modal window should appear showing you the SQL of the report.
- In the footer of the modal, test the "Delete" button.

Sponsored-by: Athens County Public Libraries
Signed-off-by: Phil Ringnalda <phil@chetcolibrary.org>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 6 Martin Renvoize 2024-07-17 15:19:58 UTC
Nice bit of cleanup alongside the fix here.

Passing QA
Comment 7 Katrin Fischer 2024-07-18 14:39:33 UTC
Can you please fix this QA script complaint:

 FAIL	koha-tmpl/intranet-tmpl/prog/en/modules/reports/guided_reports_start.tt
   FAIL	  forbidden patterns
		forbidden pattern: simple-quote string (line 2461)

It's in a big block of JS so switching the quotes is quite confusing.
Comment 8 Katrin Fischer 2024-07-18 15:29:33 UTC
This actually breaks the test, so waiting for the follow-up before pushing it.

# Looks like you failed 1 test of 1.
xt/single_quotes.t ............... Dubious, test returned 1 (wstat 256, 0x100)
Failed 1/1 subtests 
xt/tt_valid.t .....
Comment 9 Owen Leonard 2024-07-18 16:55:13 UTC
Created attachment 169155 [details] [review]
Bug 37270: (follow-up) Fix mixed quotes in preview modal JS
Comment 10 Owen Leonard 2024-07-18 16:56:00 UTC
I was not able to get the translation tool to run, so I don't know if this really works in translated templates.

If you can, please try it!
Comment 11 Martin Renvoize 2024-07-22 06:38:04 UTC
Thanks for all the hard work!

Pushed to main for the next 24.11.00 release as RM Assistant