Bug 24670 - Circulation and fine rules page has performance issues since issuingrules change
Summary: Circulation and fine rules page has performance issues since issuingrules change
Status: Pushed to master
Alias: None
Product: Koha
Classification: Unclassified
Component: System Administration (show other bugs)
Version: master
Hardware: All All
: P5 - low major (vote)
Assignee: Jonathan Druart
QA Contact: Testopia
URL:
Keywords:
Depends on: 18936
Blocks:
  Show dependency treegraph
 
Reported: 2020-02-14 21:34 UTC by Katrin Fischer
Modified: 2020-02-20 08:46 UTC (History)
5 users (show)

See Also:
Change sponsored?: ---
Patch complexity: ---
Who signed the patch off:
Text to go in the release notes:
Version(s) released in:
20.05.00


Attachments
Bug 24670: Fetch all circulation rules at once to fix perf regression (8.33 KB, patch)
2020-02-17 11:56 UTC, Jonathan Druart
Details | Diff | Splinter Review
Attachment to Bug 24670 - Circulation and fine rules page has performance issues since issuingrules change (8.38 KB, patch)
2020-02-19 21:00 UTC, Lisette Scheer
Details | Diff | Splinter Review
Bug 24670: Fetch all circulation rules at once to fix perf regression (8.43 KB, patch)
2020-02-19 21:17 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 24670: Fetch all circulation rules at once to fix perf regression (8.43 KB, patch)
2020-02-19 21:33 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 24670: (QA follow-up) Add missing $rules definition (910 bytes, patch)
2020-02-19 21:33 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 24670: (QA follow-up) Add missing $rules definition (969 bytes, patch)
2020-02-19 22:15 UTC, Lisette Scheer
Details | Diff | Splinter Review
Bug 24670: (QA follow-up) Add missing $rules definition (1.02 KB, patch)
2020-02-20 07:56 UTC, Jonathan Druart
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Katrin Fischer 2020-02-14 21:34:06 UTC
Opening the circulation and fines rules page takes much longer than before bug 18936 even if there is only a single rule.
Saving and editing a rule also takes longer.
Comment 1 Andrew Fuerste-Henry 2020-02-14 22:11:37 UTC
+1 on this. We've spent some time exploring the new interface and it's painfully slow.
Comment 2 Jonathan Druart 2020-02-17 11:15:28 UTC
(In reply to Andrew Fuerste-Henry from comment #1)
> +1 on this. We've spent some time exploring the new interface and it's
> painfully slow.

The new interface you are referring to is bug 15522. Not necessarily related to bug 18936. On bug 15522 the whole smart-rules.pl script is removed.
Comment 3 Jonathan Druart 2020-02-17 11:56:01 UTC
Created attachment 99082 [details] [review]
Bug 24670: Fetch all circulation rules at once to fix perf regression
Comment 4 Jonathan Druart 2020-02-17 11:57:04 UTC
I do not have much circulation rules defined in my dev install, so it's a bit hard to notice the difference. Could you test this patch and tell me if it improves the performance?
Comment 5 Lisette Scheer 2020-02-19 19:31:40 UTC
When I try to load the page, I get this message:

Could not compile /home/vagrant/kohaclone/admin/smart-rules.pl: Global symbol "$rules" requires explicit package name (did you forget to declare "my $rules"?) at /home/vagrant/kohaclone/admin/smart-rules.pl line 584.
Global symbol "$rules" requires explicit package name (did you forget to declare "my $rules"?) at /home/vagrant/kohaclone/admin/smart-rules.pl line 595.
BEGIN not safe after errors--compilation aborted at /home/vagrant/kohaclone/admin/smart-rules.pl line 633.
 at /usr/share/perl5/CGI/Compile.pm line 132


I reset_all and dbic and yarn build but still get this message. Is there another command I'm missing?
Comment 6 Lisette Scheer 2020-02-19 21:00:45 UTC
Created attachment 99278 [details] [review]
Attachment to Bug 24670 - Circulation and fine rules page has performance issues since issuingrules change

Bug 24670: Fetch all circulation rules at once to fix perf regression

Signed-off-by: Lisette Scheer <lisettes@latahlibrary.org>
Comment 7 Lisette Scheer 2020-02-19 21:02:18 UTC
I went back later and it worked this time. Much faster now!
Comment 8 Martin Renvoize 2020-02-19 21:17:01 UTC
Created attachment 99280 [details] [review]
Bug 24670: Fetch all circulation rules at once to fix perf regression

Signed-off-by: Lisette Scheer <lisettes@latahlibrary.org>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 10 Martin Renvoize 2020-02-19 21:26:30 UTC
Ooops.. false positive there.. had the wrong branch checked out.

I see the same error as Lisette I'm afraid.. looks like $rules is defined in a local scope but then assigned to in a global one.
Comment 11 Martin Renvoize 2020-02-19 21:33:24 UTC
Created attachment 99281 [details] [review]
Bug 24670: Fetch all circulation rules at once to fix perf regression

Signed-off-by: Lisette Scheer <lisettes@latahlibrary.org>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 12 Martin Renvoize 2020-02-19 21:33:28 UTC
Created attachment 99282 [details] [review]
Bug 24670: (QA follow-up) Add missing $rules definition

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 13 Martin Renvoize 2020-02-19 21:34:49 UTC
Now it's looking good to me.. Setting back to SO as I'd like Jonathan to check my followup.. seems strange it was missed.
Comment 14 Lisette Scheer 2020-02-19 22:15:57 UTC
Created attachment 99286 [details] [review]
Bug 24670: (QA follow-up) Add missing $rules definition

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Signed-off-by: Lisette Scheer <lisettes@latahlibrary.org>
Comment 15 Jonathan Druart 2020-02-20 07:56:28 UTC
Created attachment 99300 [details] [review]
Bug 24670: (QA follow-up) Add missing $rules definition

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Signed-off-by: Lisette Scheer <lisettes@latahlibrary.org>

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 16 Jonathan Druart 2020-02-20 07:57:18 UTC
(In reply to Martin Renvoize from comment #13)
> Now it's looking good to me.. Setting back to SO as I'd like Jonathan to
> check my followup.. seems strange it was missed.

Oops, I have the bad habit to modify my patches after I already tested them. I guess I renamed something just before attaching it here, sorry about that!
Comment 17 Martin Renvoize 2020-02-20 08:33:23 UTC
No worries, I often do the same :P
Comment 18 Katrin Fischer 2020-02-20 08:43:49 UTC
*puts hands over her ears and sings loudly*
Comment 19 Katrin Fischer 2020-02-20 08:44:30 UTC
(In reply to Katrin Fischer from comment #18)
> *puts hands over her ears and sings loudly*

... hands over the eyes might work better in this case.
Comment 20 Martin Renvoize 2020-02-20 08:46:23 UTC
Nice work everyone!

Pushed to master for 20.05