Bug 37270

Summary:	Deleting a report from the actions menu on a list of saved reports does not work
Product:	Koha	Reporter:	Phil Ringnalda <phil>
Component:	Reports	Assignee:	Owen Leonard <oleonard>
Status:	Pushed to stable ---	QA Contact:	Martin Renvoize (ashimema) <martin.renvoize>
Severity:	major
Priority:	P5 - low	CC:	cj.lynce, eric, kebliss, lucas
Version:	Main
Hardware:	All
OS:	All
Change sponsored?:	---	Patch complexity:	Trivial patch
Documentation contact:		Documentation submission:
Text to go in the release notes:		Version(s) released in:	24.11.00,24.05.05
Circulation function:
Bug Depends on:	36192
Bug Blocks:
Attachments:	Bug 37270: Fix deletion of saved reports Bug 37270: Fix deletion of saved reports Bug 37270: Fix deletion of saved reports Bug 37270: (follow-up) Fix mixed quotes in preview modal JS

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 (ashimema) 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 (ashimema) 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 (ashimema) 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

Comment 12 Phil Ringnalda 2024-10-14 18:55:03 UTC

*** Bug 38160 has been marked as a duplicate of this bug. ***

Comment 13 Eric Swenson 2024-10-24 17:13:17 UTC

Would this be able to be backported?

Comment 14 Lucas Gass (lukeg) 2024-10-25 16:41:12 UTC

Backported to 24.05.x for upcoming 24.05.05

Comment 15 CJ Lynce 2024-10-30 17:06:43 UTC

Lucas, it appears backporting this patch on 24.05.x caused a regression, breaking the Delete button in the toolbar drop-down menu when running or editing reports.

It looks like removal of lines 2252-2258 in guided_reports_start.tt (in the first patch) broke the drop-down delete option.

This only affects the 24.05.x branch, not main.