https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18291 --- Comment #1 from Alex Buckley <alexbuckley@catalyst.net.nz> --- Created attachment 61235 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=61235&action=edit Bug 18291 - Removed the SQL query out of admin/preferences.pl Placed a subroutine into Context.pm which in turn calls Koha::Config::SysPref.pm where the SQL query has been re-written as a DBIx query -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18291 Alex Buckley <alexbuckley@catalyst.net.nz> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|ASSIGNED |Needs Signoff --- Comment #3 from Alex Buckley <alexbuckley@catalyst.net.nz> --- Test Plan: 1. Restart memcached: sudo service memcached restart 2. Drop and recreate the Koha instance database 3. Go through the web installer, selecting all data to be installed in step 3 4. After the web installer is finished log in with the Koha database administrator credentials 5. Create yourself a patron account 6. Set the patron account to have superlibrarian privileges 7. Log out and back in as your newly created patron 8. Navigate to the Administration area 9. Write in the URL: cgi-bin/koha/admin/preferences.pl 10. Notice a table of system preferences and associated values 11. Apply all patches associated with this bug report 12. Repeat steps 1,2,3,4,5,6,7,8,9 13. Again notice the systempreference table is full of preferences and their values is displayed 14. In your terminal navigate from your Koha instance root directory to t/db_dependent 15. Enter koha shell: sudo koha-shell <instancename> 16. Run sysprefs.t unit test: prove -v sysprefs.t 17. The tests should run successful 18. Exit the koha shell 19. Navigate back to your Koha instance root directory 20. View admin/preferences.pl script and observe it contains no SQL queries 21. Navigate back to your Koha instance root directory 22. View Koha/Config/SysPref.pm and notice that the select_systempreferences subroutine (the subroutine preferences.pl now calls to run the SQL query) contains POD -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18291 Marc Véron <veron@veron.ch> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #61236|0 |1 is obsolete| | --- Comment #5 from Marc Véron <veron@veron.ch> --- Created attachment 62518 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=62518&action=edit Bug 18291 - Fixed intranet-error log error, by changing back to using SQL query. Removed select_systempreferences subroutine from C4::Context.pm The subroutine in Koha::Config::SysPref.pm includes a successful unit test and POD Signed-off-by: Marc Véron <veron@veron.ch> -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18291 Jonathan Druart <jonathan.druart@bugs.koha-community.org> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |jonathan.druart@bugs.koha-c | |ommunity.org Status|Signed Off |Failed QA --- Comment #6 from Jonathan Druart <jonathan.druart@bugs.koha-community.org> --- Alex, There are several things wrong in these patches: 1. The test you added to t/db_dependent/sysprefs.t does not test the new method you added to Koha::Config::SysPref 2. For modules based on Koha::Object[s], we have 2 modules: Koha::Config::Syspref and Koha::Config::Sysprefs (notice the s at the end). One if for a single record, the other one is for collection/list. Modules based on Koha::Object[s] already provide you a lot of CRUD functionalities, without doing anything in the module. For instance Koha::Config::Sysprefs->find( $variable ) will return an object representing the system preference $variable. It is exactly what you are doing with your new method. 3. To access sysprefs you need to call C4::Context->preference. This subroutine uses a cache and is the one to call to access sysprefs. I do not see a valid reason not to fetch the in cache values here, so a 1 line patch should do the trick ;) -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18291 Alex Buckley <alexbuckley@catalyst.net.nz> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #62517|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=18291 --- Comment #7 from Alex Buckley <alexbuckley@catalyst.net.nz> --- Created attachment 62905 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=62905&action=edit Bug 18291 - Implemented call to C4::Context->preference($name) in admin/preferences.pl which uses cached preference values -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18291 --- Comment #8 from Alex Buckley <alexbuckley@catalyst.net.nz> --- Created attachment 62906 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=62906&action=edit Bug 18291 - Implemented call to C4::Context->preference($name) in admin/preferences.pl which uses cached preference values -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18291 Jonathan Druart <jonathan.druart@bugs.koha-community.org> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Needs Signoff |In Discussion CC| |m.de.rooy@rijksmuseum.nl, | |martin.renvoize@ptfs-europe | |.com, | |nick@bywatersolutions.com, | |tomascohen@gmail.com --- Comment #10 from Jonathan Druart <jonathan.druart@bugs.koha-community.org> --- Hi Alex, I thought a bit more about this. (In reply to Jonathan Druart from comment #6)

I do not see a valid reason not to fetch the in cache values here, so a 1 line patch should do the trick ;)

Actually there is one, if the cache is not correctly set, for whatever reason, we would like to display the DB value. I do not think it is wrong to have a SELECT statement here. -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18291 --- Comment #12 from Jonathan Druart <jonathan.druart@bugs.koha-community.org> --- Nope. Not everything is cached, but sysprefs are. -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18291 --- Comment #14 from Jonathan Druart <jonathan.druart@bugs.koha-community.org> --- (In reply to Alex Buckley from comment #13)

Ah ok so this this patch isn't necessary but I can use a similar approach to how you suggested to solve this bug with the other sql removal bug patches?

Exactly :) -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18291 --- Comment #15 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- If cache value is incorrect, we may certainly have serious problems on other places too. If we do not replace with a context call here, we could still do a Koha::Config::Syspref call? Assuming that the intention of this report is to remove SQL statements from scripts. -- You are receiving this mail because: You are watching all bug changes.