Bug 24670

Summary: Circulation and fine rules page has performance issues since issuingrules change
Product: Koha Reporter: Katrin Fischer <katrin.fischer>
Component: System AdministrationAssignee: Jonathan Druart <jonathan.druart>
Status: CLOSED FIXED QA Contact: Testopia <testopia>
Severity: major    
Priority: P5 - low CC: 1joynelson, andrewfh, gmcharlt, jonathan.druart, lisettepalouse+koha, martin.renvoize
Version: Main   
Hardware: All   
OS: All   
Change sponsored?: --- Patch complexity: ---
Documentation contact: Documentation submission:
Text to go in the release notes:
Version(s) released in:
20.05.00
Bug Depends on: 18936    
Bug Blocks:    
Attachments: Bug 24670: Fetch all circulation rules at once to fix perf regression
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
Bug 24670: Fetch all circulation rules at once to fix perf regression
Bug 24670: (QA follow-up) Add missing $rules definition
Bug 24670: (QA follow-up) Add missing $rules definition
Bug 24670: (QA follow-up) Add missing $rules definition

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
Comment 21 Joy Nelson 2020-03-13 00:49:09 UTC
not backported to 19.11.x due to dependencies