[Bug 43249] New: A duplicate report limit of 1 prevents users from running reports at all
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=43249 Bug ID: 43249 Summary: A duplicate report limit of 1 prevents users from running reports at all Initiative type: --- Sponsorship --- status: Product: Koha Version: Main Hardware: All OS: All Status: NEW Severity: normal Priority: P5 - low Component: Reports Assignee: koha-bugs@lists.koha-community.org Reporter: nick@bywatersolutions.com QA Contact: testopia@bugs.koha-community.org CC: lisette@bywatersolutions.com Target Milestone: --- The code that checks runnign reports checks: $running >= $duplicate_limit, but it appears a report is marked as running during prep_report Additionally Koha::Reports->running returns the reports instances, and not a count - we need to call count on the result -- You are receiving this mail because: You are the assignee for the bug. You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=43249 Nick Clemens (kidclamp) <nick@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Depends on| |41918 Referenced Bugs: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=41918 [Bug 41918] Prevent users from running the same report multiple times concurrently -- You are receiving this mail because: You are the assignee for the bug. You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=43249 --- Comment #1 from Nick Clemens (kidclamp) <nick@bywatersolutions.com> --- Created attachment 203352 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=203352&action=edit Bug 43249: Count the number of running reports prove -v t/db_dependent/Koha/Reports.t -- You are receiving this mail because: You are watching all bug changes. You are the assignee for the bug.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=43249 --- Comment #2 from Nick Clemens (kidclamp) <nick@bywatersolutions.com> --- Created attachment 203353 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=203353&action=edit Bug 43249: Adjust unit tests The mock here says return two instances, but I don't believe it does this, when removed the tests still pass, with a limit of 1 an exception is thrown during prep report To test: 1 - Apply this 2 - prove -v t/db_dependent/Koha/Reports.t Still passes? -- You are receiving this mail because: You are watching all bug changes. You are the assignee for the bug.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=43249 Nick Clemens (kidclamp) <nick@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #203352|0 |1 is obsolete| | -- You are receiving this mail because: You are watching all bug changes. You are the assignee for the bug.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=43249 Nick Clemens (kidclamp) <nick@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|NEW |Needs Signoff Patch complexity|--- |Trivial patch Assignee|koha-bugs@lists.koha-commun |nick@bywatersolutions.com |ity.org | -- You are receiving this mail because: You are watching all bug changes. You are the assignee for the bug.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=43249 --- Comment #3 from Nick Clemens (kidclamp) <nick@bywatersolutions.com> --- Created attachment 203356 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=203356&action=edit Bug 43249: Don't count our query against the duplicate report limit We were throwing an exception for a single run of the report when limit was as one because we were finding our query for counting the instances in the processlist, we need to skip our selves To test: 1 - Apply this patch 2 - prove -v t/db_dependent/Koha/Reports.t 3 - It fails! -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=43249 --- Comment #4 from Nick Clemens (kidclamp) <nick@bywatersolutions.com> --- Created attachment 203357 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=203357&action=edit Bug 43249: Update unit tests This patch restores the mock to make it seem as if one copy of the report is running, and checks that we are over the limit of 1 To test: 1 - Apply this patch 2 - prove -v t/db_dependent/Koha/Reports.t 3 - It passes! -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=43249 --- Comment #5 from Nick Clemens (kidclamp) <nick@bywatersolutions.com> --- Worth noting here, I tested with a report like: SELECT items.barcode FROM items, items items2, items items3 I set the limit to 1 If I open that in 3 tabs and click run report - they run consecutively, and I don't get an error If I open a new private browser window, sign in to Koha, and run from each window, then I get the limit So a single user could theoretically still tie up the DB by setting of a whole bunch of these in different tabs and have them running one after the other -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=43249 David Nind <david@davidnind.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Needs Signoff |Signed Off -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=43249 David Nind <david@davidnind.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #203353|0 |1 is obsolete| | Attachment #203356|0 |1 is obsolete| | Attachment #203357|0 |1 is obsolete| | -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=43249 --- Comment #6 from David Nind <david@davidnind.com> --- Created attachment 203358 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=203358&action=edit Bug 43249: Adjust unit tests The mock here says return two instances, but I don't believe it does this, when removed the tests still pass, with a limit of 1 an exception is thrown during prep report To test: 1 - Apply this 2 - prove -v t/db_dependent/Koha/Reports.t Still passes? Signed-off-by: David Nind <david@davidnind.com> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=43249 --- Comment #7 from David Nind <david@davidnind.com> --- Created attachment 203359 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=203359&action=edit Bug 43249: Don't count our query against the duplicate report limit We were throwing an exception for a single run of the report when limit was as one because we were finding our query for counting the instances in the processlist, we need to skip our selves To test: 1 - Apply this patch 2 - prove -v t/db_dependent/Koha/Reports.t 3 - It fails! Signed-off-by: David Nind <david@davidnind.com> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=43249 --- Comment #8 from David Nind <david@davidnind.com> --- Created attachment 203360 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=203360&action=edit Bug 43249: Update unit tests This patch restores the mock to make it seem as if one copy of the report is running, and checks that we are over the limit of 1 To test: 1 - Apply this patch 2 - prove -v t/db_dependent/Koha/Reports.t 3 - It passes! Signed-off-by: David Nind <david@davidnind.com> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=43249 Andrew Fuerste-Henry <andrew@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |andrew@bywatersolutions.com -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=43249 Lisette Scheer <lisette@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- QA Contact|testopia@bugs.koha-communit |tomascohen@gmail.com |y.org | -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=43249 Andrew Fuerste-Henry <andrew@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Signed Off |Failed QA --- Comment #9 from Andrew Fuerste-Henry <andrew@bywatersolutions.com> --- This isn't working as intended for me. <duplicate_running_reports_per_user_limit> = 1 correctly allows 1 instance of a report to run, but upping the limit to 2 causes the limit to not be enforced. To recreate: - apply patch - update koha-conf, add to config: <duplicate_running_reports_per_user_limit>1</duplicate_running_reports_per_user_limit> - restart_all - create a report "select sleep(90)" - view your report in 2 tabs - in your terminal, sudo koha-mysql kohadev to access the SQL server - click Run in your first report tab - in your terminal, "show processlist;" and confirm your query is running - click Run in your second report tab, get a message that your report is already running -- Yay! that's desired! - let your report finish - update koha-conf to: <duplicate_running_reports_per_user_limit>2</duplicate_running_reports_per_user_limit> - restart_all - view your report in 3 tabs - in your terminal, sudo koha-mysql kohadev to access the SQL server - click Run in your first report tab - in your terminal, "show processlist;" and confirm your query is running - click Run in your second report tab - in your terminal, "show processlist;" and confirm your second query is running (the second query has been taking about 25 seconds to show up on my KTD) - click Run in your third report tab - in your terminal, "show processlist;" and confirm your third query is running (again, takes about 25 seconds to show up) - You should have gotten that "report is already running" error on this third instance -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=43249 --- Comment #10 from Nick Clemens (kidclamp) <nick@bywatersolutions.com> --- (In reply to Andrew Fuerste-Henry from comment #9)
This isn't working as intended for me. <duplicate_running_reports_per_user_limit> = 1 correctly allows 1 instance of a report to run, but upping the limit to 2 causes the limit to not be enforced.
Andrew is correct - the "running" sub returns a count of DIFFERENT reports running We get the processlist, loop through it, and set a hash key of the report_id to 1 We then do a search of reports using those keys For a report running three times, we set the same key three times, then fetch that report a single time. That doesn't work We could increment the value - but even then, we would do a search for the report, and it would return the single report. It isn't designed to count any report more than once -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=43249 --- Comment #11 from Kyle M Hall (khall) <kyle@bywatersolutions.com> --- Created attachment 207910 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=207910&action=edit Bug 43249: (follow-up) Count each running copy of a report against the limits Patch from commit d2b32ee -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=43249 Kyle M Hall (khall) <kyle@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |kyle@bywatersolutions.com Status|Failed QA |Signed Off -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=43249 Andrew Fuerste-Henry <andrew@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Signed Off |Passed QA QA Contact|tomascohen@gmail.com |andrew@bywatersolutions.com -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=43249 Andrew Fuerste-Henry <andrew@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #203358|0 |1 is obsolete| | Attachment #203359|0 |1 is obsolete| | Attachment #203360|0 |1 is obsolete| | Attachment #207910|0 |1 is obsolete| | -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=43249 --- Comment #12 from Andrew Fuerste-Henry <andrew@bywatersolutions.com> --- Created attachment 207960 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=207960&action=edit Bug 43249: Adjust unit tests The mock here says return two instances, but I don't believe it does this, when removed the tests still pass, with a limit of 1 an exception is thrown during prep report To test: 1 - Apply this 2 - prove -v t/db_dependent/Koha/Reports.t Still passes? Signed-off-by: David Nind <david@davidnind.com> Signed-off-by: Andrew Fuerste Henry <andrew@bywatersolutions.com> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=43249 --- Comment #13 from Andrew Fuerste-Henry <andrew@bywatersolutions.com> --- Created attachment 207961 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=207961&action=edit Bug 43249: Don't count our query against the duplicate report limit We were throwing an exception for a single run of the report when limit was as one because we were finding our query for counting the instances in the processlist, we need to skip our selves To test: 1 - Apply this patch 2 - prove -v t/db_dependent/Koha/Reports.t 3 - It fails! Signed-off-by: David Nind <david@davidnind.com> Signed-off-by: Andrew Fuerste Henry <andrew@bywatersolutions.com> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=43249 --- Comment #14 from Andrew Fuerste-Henry <andrew@bywatersolutions.com> --- Created attachment 207962 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=207962&action=edit Bug 43249: Update unit tests This patch restores the mock to make it seem as if one copy of the report is running, and checks that we are over the limit of 1 To test: 1 - Apply this patch 2 - prove -v t/db_dependent/Koha/Reports.t 3 - It passes! Signed-off-by: David Nind <david@davidnind.com> Signed-off-by: Andrew Fuerste Henry <andrew@bywatersolutions.com> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=43249 --- Comment #15 from Andrew Fuerste-Henry <andrew@bywatersolutions.com> --- Created attachment 207963 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=207963&action=edit Bug 43249: (follow-up) Count each running copy of a report against the limits Signed-off-by: Andrew Fuerste Henry <andrew@bywatersolutions.com> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=43249 Marcel de Rooy <m.de.rooy@rijksmuseum.nl> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |m.de.rooy@rijksmuseum.nl -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=43249 --- Comment #16 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- I was looking at this report at the end of the KohaCon. And I was not convinced by the unit tests. We mock the sub running by something that looks overcomplicated. In a number of cases we could just return 1 || true or return e.g. 3 for prep_report() total_running_reports_per_instance_limit, etc. Or you could argue that we are actually only testing our mocked routine since it is too hard to test the real running sub. But that does not make much sense? Yeah, 5 > 3. But we knew that, right? -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=43249 --- Comment #17 from Nick Clemens (kidclamp) <nick@bywatersolutions.com> --- (In reply to Marcel de Rooy from comment #16)
I was looking at this report at the end of the KohaCon. And I was not convinced by the unit tests.
We mock the sub running by something that looks overcomplicated. In a number of cases we could just return 1 || true or return e.g. 3 for prep_report() total_running_reports_per_instance_limit, etc.
Or you could argue that we are actually only testing our mocked routine since it is too hard to test the real running sub. But that does not make much sense? Yeah, 5 > 3. But we knew that, right?
I don't disagree, but I am not sure how to test this properly - if we try with Cypress it depends on having several plack workers and is subject to race conditions. I tried with forking and wasn't able to solve it easily. Do you have a suggestion? -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=43249 Martin Renvoize (ashimema) <martin.renvoize@openfifth.co.uk> changed: What |Removed |Added ---------------------------------------------------------------------------- Version(s)| |26.11.00 released in| | --- Comment #18 from Martin Renvoize (ashimema) <martin.renvoize@openfifth.co.uk> --- Pushed to main for 26.11 with consent from Pedro whilst he's on annual leave -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=43249 Martin Renvoize (ashimema) <martin.renvoize@openfifth.co.uk> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Passed QA |Pushed to main -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=43249 Andrew Fuerste-Henry <andrew@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Keywords| |rel_26_05_candidate -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=43249 Lucas Gass (lukeg) <lucas@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Pushed to main |Pushed to stable Version(s)|26.11.00 |26.11.00,26.05.05 released in| | -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=43249 --- Comment #19 from Lucas Gass (lukeg) <lucas@bywatersolutions.com> --- Nice work everyone! Pushed to 26.05.x for the upcoming 26.05.05 release. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=43249 Lucas Gass (lukeg) <lucas@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |lucas@bywatersolutions.com Keywords|rel_26_05_candidate | -- You are receiving this mail because: You are watching all bug changes.
participants (1)
-
bugzilla-daemon＠bugs.koha-community.org