Bug 40896 - Run report button should be disabled after click
Summary: Run report button should be disabled after click
Status: Signed Off
Alias: None
Product: Koha
Classification: Unclassified
Component: Reports (show other bugs)
Version: Main
Hardware: All All
: P5 - low enhancement
Assignee: Jake Deery
QA Contact: Testopia
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2025-09-29 12:44 UTC by Jake Deery
Modified: 2025-12-12 14:55 UTC (History)
3 users (show)

See Also:
GIT URL:
Initiative type: ---
Sponsorship status: ---
Comma delimited list of Sponsors:
Crowdfunding goal: 0
Patch complexity: ---
Documentation contact:
Documentation submission:
Text to go in the release notes:
Version(s) released in:
Circulation function:


Attachments
Bug 40896: Disable run report buttons upon click (4.71 KB, patch)
2025-09-29 13:30 UTC, Jake Deery
Details | Diff | Splinter Review
Bug 40896: Disable run report buttons upon click (4.78 KB, patch)
2025-10-01 13:50 UTC, Andrew Fuerste-Henry
Details | Diff | Splinter Review
Bug 40896: (follow-up) DO NOT PUSH (1.81 KB, patch)
2025-10-07 10:50 UTC, Jake Deery
Details | Diff | Splinter Review
Bug 40896: (follow-up) Use icon wrapper elements and visibility toggling (19.98 KB, patch)
2025-10-17 16:03 UTC, Paul Derscheid
Details | Diff | Splinter Review
Bug 40896: (follow-up) Use icon wrapper elements and visibility toggling (19.82 KB, patch)
2025-12-12 11:54 UTC, Jake Deery
Details | Diff | Splinter Review
Bug 40896: (follow-up) Use icon wrapper elements and visibility toggling (20.05 KB, patch)
2025-12-12 12:00 UTC, Jake Deery
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Jake Deery 2025-09-29 12:44:46 UTC
When a staff member clicks the run report button in the guided reports interface, we should disable the button, and display some indication an action is occurring (such as a spinner).

Jake.
Comment 1 Jake Deery 2025-09-29 13:30:50 UTC
Created attachment 187044 [details] [review]
Bug 40896: Disable run report buttons upon click

This patch will disable the run report button found on both the list page, and the individual operation pages (e.g. run, edit, saved successfully). This will stop librarians from unintentionally crashing their Koha system, by pressing the run report button repeatedly when there is no need to re-request it.

TO TEST:
a)  go to /cgi-bin/koha/reports/guided_reports.pl?op=list
b)  create three new sql test reports:
    *)  Name: Test report #1, SQL: SELECT count(*) FROM biblio
    *)  Name: Test report #2, SQL: SELECT count(*) FROM items
    *)  Name: Test report #3, SQL: SELECT * FROM items WHERE items.dateaccessioned =  <<Date accessioned|date>>
c)  run all of the above test reports, noticing how the run report button can be activated repeatedly
APPLY PATCH
d)  redo step a
e)  run test report #1 from the table, notice how now the button can only be activated once before it is greyed out and the play button is replaced with a spinner
f)  return to the list, edit test report #2 from the table, click run report from the edit page, and notice how this run report button exhibits the same behaviour as the button in step e
g)  return to the list, run test report #3, select a date, click run report from this page, and notice again how the run report button exhibits the same behaviour as the buttons in steps e and f
SIGN OFF
Comment 2 Andrew Fuerste-Henry 2025-10-01 13:50:51 UTC
Created attachment 187193 [details] [review]
Bug 40896: Disable run report buttons upon click

This patch will disable the run report button found on both the list page, and the individual operation pages (e.g. run, edit, saved successfully). This will stop librarians from unintentionally crashing their Koha system, by pressing the run report button repeatedly when there is no need to re-request it.

TO TEST:
a)  go to /cgi-bin/koha/reports/guided_reports.pl?op=list
b)  create three new sql test reports:
    *)  Name: Test report #1, SQL: SELECT count(*) FROM biblio
    *)  Name: Test report #2, SQL: SELECT count(*) FROM items
    *)  Name: Test report #3, SQL: SELECT * FROM items WHERE items.dateaccessioned =  <<Date accessioned|date>>
c)  run all of the above test reports, noticing how the run report button can be activated repeatedly
APPLY PATCH
d)  redo step a
e)  run test report #1 from the table, notice how now the button can only be activated once before it is greyed out and the play button is replaced with a spinner
f)  return to the list, edit test report #2 from the table, click run report from the edit page, and notice how this run report button exhibits the same behaviour as the button in step e
g)  return to the list, run test report #3, select a date, click run report from this page, and notice again how the run report button exhibits the same behaviour as the buttons in steps e and f
SIGN OFF

Signed-off-by: Andrew Fuerste Henry <andrew@bywatersolutions.com>
Comment 3 Andrew Fuerste-Henry 2025-10-01 13:53:22 UTC
This is a great idea! If you have time for it, it'd be good to also do this on the "Update and run SQL" button on the edit screen.
Comment 4 Paul Derscheid 2025-10-06 11:55:26 UTC
Hi Jake, was looking to QA this. Could you make one change?
Could you replace the match on i.fa-{spinner,play} with usage of ids or data attributes instead? 

Just to decouple the logic from the icon classes so that we don't have to touch it if we change icon libraries.
Comment 5 Jake Deery 2025-10-07 10:50:21 UTC
Created attachment 187512 [details] [review]
Bug 40896: (follow-up) DO NOT PUSH
Comment 6 Jake Deery 2025-10-07 10:53:03 UTC
Hi Paul,

Take a look at that patch. We're just looking for any FontAwesome class within the element targeted, now. There is still a line that will `replaceWith` the HTML after 120 seconds, but to be honest I can't think of another sane way of doing this, without any easily usable ID or data attribute on the FontAwesome icon itself?

If it looks good, I'll merge it down into one patchfile for pushing.

Ta,
Jake
Comment 7 Paul Derscheid 2025-10-17 16:03:22 UTC
Created attachment 188106 [details] [review]
Bug 40896: (follow-up) Use icon wrapper elements and visibility toggling

- Wrap icons in semantic container elements for visibility toggling
- Create throttled-button.inc with Template Toolkit BLOCKs to reduce duplication
- Extract throttle logic to throttledButton.js with event delegation
- Make timeout configurable via data-throttle-timeout attribute
- Replace string-based modal generation with template element to support
  SSR

To test:
1) Follow the test plan from the previous commit
2) Additionally verify the throttled button behavior works in all locations:
   - Reports list table: "Run" button for each saved report
   - Report toolbar: "Run report" button (when viewing/editing a report)
   - Parameters form: "Run the report" button
   - Edit SQL form: "Update and run SQL" button
   - Preview SQL modal: "Run report" button (click "Preview SQL" in dropdown)
3) For each location, confirm:
   - Button shows play icon initially
   - Upon click, button becomes disabled with spinner icon
   - Button has aria-busy="true" when disabled
   - After navigation/submission, button state is handled correctly
4) Test the modal button specifically:
   - Open Preview SQL modal from any report dropdown
   - Click "Run report" in the modal
   - Verify throttle behavior works for this dynamically created button
5) Verify that the set timeout works well, I thought 120 seconds is too
   long if you go back via bfcache (back button in the browser). You'd
   have to either reload or wait 2 minutes for the button to become
   interactive again.
6) Sign off
Comment 8 Paul Derscheid 2025-10-17 16:07:15 UTC
Hi Jake, got a little caught up during the DACH hackfest. Sorry for the late reply.

I liked the original version better as it was cleaner so obsoleted the second patch.

But I thought we will need this elsewhere as this is a common problem.

For example, we also got Bug 15792 and Bug 20170 (which I was tasked to fix by LMSCloud).

Let me know what you think of the attached solution, it was prettier but then I had to collapse a lot because the qa script complained.

What I commonly do is to include both states and then just visibility toggle between them as it's less expensive.
Comment 9 Jake Deery 2025-12-12 11:54:16 UTC Comment hidden (obsolete)
Comment 10 Jake Deery 2025-12-12 11:57:08 UTC
Hi Paul,

Sorry, been busy here too. Took a play with the patch today, had to put some effort in to get it to apply cleanly to main, but with that done, I ran through your test plan, and it seems to work for me!

I'll run the QA tools against it, but I am happy to sign it off. A shame it didn't make it to 25.11, but it will be amazing to get this into 26.05!

Ta,
Jake
Comment 11 Jake Deery 2025-12-12 12:00:49 UTC
Created attachment 190458 [details] [review]
Bug 40896: (follow-up) Use icon wrapper elements and visibility toggling

- Wrap icons in semantic container elements for visibility toggling
- Create throttled-button.inc with Template Toolkit BLOCKs to reduce duplication
- Extract throttle logic to throttledButton.js with event delegation
- Make timeout configurable via data-throttle-timeout attribute
- Replace string-based modal generation with template element to support
  SSR

To test:
1) Follow the test plan from the previous commit
2) Additionally verify the throttled button behavior works in all locations:
   - Reports list table: "Run" button for each saved report
   - Report toolbar: "Run report" button (when viewing/editing a report)
   - Parameters form: "Run the report" button
   - Edit SQL form: "Update and run SQL" button
   - Preview SQL modal: "Run report" button (click "Preview SQL" in dropdown)
3) For each location, confirm:
   - Button shows play icon initially
   - Upon click, button becomes disabled with spinner icon
   - Button has aria-busy="true" when disabled
   - After navigation/submission, button state is handled correctly
4) Test the modal button specifically:
   - Open Preview SQL modal from any report dropdown
   - Click "Run report" in the modal
   - Verify throttle behavior works for this dynamically created button
5) Verify that the set timeout works well, I thought 120 seconds is too
   long if you go back via bfcache (back button in the browser). You'd
   have to either reload or wait 2 minutes for the button to become
   interactive again.
6) Sign off

Signed-off-by: Jake Deery <jake.deery@openfifth.co.uk>
Comment 12 Paul Derscheid 2025-12-12 14:55:07 UTC
Hi Jake, thanks.

Wondered whether I overdid it here a bit, but since this is now an include, it's probably justifiable.