Bug 18291 - Remove SQL from preferences.pl administrative script
Summary: Remove SQL from preferences.pl administrative script
Status: CLOSED INVALID
Alias: None
Product: Koha
Classification: Unclassified
Component: Architecture, internals, and plumbing (show other bugs)
Version: unspecified
Hardware: All All
: P5 - low enhancement (vote)
Assignee: Alex Buckley
QA Contact: Testopia
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2017-03-17 07:01 UTC by Alex Buckley
Modified: 2017-12-07 22:16 UTC (History)
6 users (show)

See Also:
Change sponsored?: ---
Patch complexity: ---
Documentation contact:
Documentation submission:
Text to go in the release notes:
Version(s) released in:


Attachments
Bug 18291 - Removed the SQL query out of admin/preferences.pl (3.85 KB, patch)
2017-03-18 01:10 UTC, Alex Buckley
Details | Diff | Splinter Review
Bug 18291 - Fixed intranet-error log error, by changing back to using SQL query. (3.67 KB, patch)
2017-03-18 01:11 UTC, Alex Buckley
Details | Diff | Splinter Review
Bug 18291 - Removed the SQL query out of admin/preferences.pl (4.03 KB, patch)
2017-04-21 06:41 UTC, Marc Véron
Details | Diff | Splinter Review
Bug 18291 - Fixed intranet-error log error, by changing back to using SQL query. (3.81 KB, patch)
2017-04-21 06:44 UTC, Marc Véron
Details | Diff | Splinter Review
Bug 18291 - Implemented call to C4::Context->preference($name) in admin/preferences.pl which uses cached preference values (3.14 KB, patch)
2017-05-01 09:50 UTC, Alex Buckley
Details | Diff | Splinter Review
Bug 18291 - Implemented call to C4::Context->preference($name) in admin/preferences.pl which uses cached preference values (1.49 KB, patch)
2017-05-01 09:52 UTC, Alex Buckley
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Alex Buckley 2017-03-17 07:01:08 UTC
This bug report requires the removal of SQL code from the preferences.pl administrative script into a separate perl module.

It also requires the creation of unit tests and POD for the new subroutines in the perl modules.
Comment 1 Alex Buckley 2017-03-18 01:10:52 UTC
Created attachment 61235 [details] [review]
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
Comment 2 Alex Buckley 2017-03-18 01:11:25 UTC
Created attachment 61236 [details] [review]
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
Comment 3 Alex Buckley 2017-03-18 01:24:34 UTC
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
Comment 4 Marc Véron 2017-04-21 06:41:19 UTC
Created attachment 62517 [details] [review]
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

Followed test plan from comment #3, works as expected
Signed-off-by: Marc Véron <veron@veron.ch>
Comment 5 Marc Véron 2017-04-21 06:44:56 UTC
Created attachment 62518 [details] [review]
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>
Comment 6 Jonathan Druart 2017-04-24 16:04:47 UTC
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 ;)
Comment 7 Alex Buckley 2017-05-01 09:50:24 UTC
Created attachment 62905 [details] [review]
Bug 18291 - Implemented call to C4::Context->preference($name) in admin/preferences.pl which uses cached preference values
Comment 8 Alex Buckley 2017-05-01 09:52:55 UTC
Created attachment 62906 [details] [review]
Bug 18291 - Implemented call to C4::Context->preference($name) in admin/preferences.pl which uses cached preference values
Comment 9 Alex Buckley 2017-05-01 10:17:14 UTC
Hi Jonathan

I think see what you mean about the use of Koha::Context->SysPref and Koha::Context->SysPref.

I have chosen to use the C4::Context::preference() call rather than the call to the Koha::Config::Sysprefs->find( $variable ) as both present a one line patch and the use of retrieving cached values seems better from a performance point of view.

In addition to this call I have had to change  $row->{'<value>'}; to $piece->{'<value>'}; in several places to allow preferences.pl to run successfully.

Could you please test my patch and let me know if I have fixed the issues that previously existed?

Cheers
Alex
Comment 10 Jonathan Druart 2017-05-02 15:39:10 UTC
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.
Comment 11 Alex Buckley 2017-05-02 20:44:06 UTC
Hi Jonathan

>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.

So regarding the other sql removal patches I have been working on such as bug 17944, 18247, 18291, 18299 do you think that they are not needed because replacing sql queries with the use of cache values will be problematic if the cache values are not set correctly?
Comment 12 Jonathan Druart 2017-05-02 21:35:29 UTC
Nope. Not everything is cached, but sysprefs are.
Comment 13 Alex Buckley 2017-05-02 21:38:17 UTC
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?
Comment 14 Jonathan Druart 2017-05-02 22:18:41 UTC
(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 :)
Comment 15 Marcel de Rooy 2017-05-03 12:44:29 UTC
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.
Comment 16 Jonathan Druart 2017-05-03 14:32:51 UTC
(In reply to Marcel de Rooy from comment #15)
> 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.

Yes and no. It would make sense to call Sysprefs->find everywhere and cache values in the Koha::Config::Sysprefs module.
Which is a lot of work for not much added value.