[Koha-bugs] [Bug 18291] Remove SQL from preferences.pl administrative script

[bugzilla-daemon]

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18291

--- Comment #16 from Jonathan Druart  
---
(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.

-- 
You are receiving this mail because:
You are watching all bug changes.
___
Koha-bugs mailing list
Koha-bugs@lists.koha-community.org
http://lists.koha-community.org/cgi-bin/mailman/listinfo/koha-bugs
website : http://www.koha-community.org/
git : http://git.koha-community.org/
bugs : http://bugs.koha-community.org/

[Koha-bugs] [Bug 18291] Remove SQL from preferences.pl administrative script

[bugzilla-daemon]

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18291

--- Comment #15 from Marcel de Rooy  ---
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.
___
Koha-bugs mailing list
Koha-bugs@lists.koha-community.org
http://lists.koha-community.org/cgi-bin/mailman/listinfo/koha-bugs
website : http://www.koha-community.org/
git : http://git.koha-community.org/
bugs : http://bugs.koha-community.org/

[Koha-bugs] [Bug 18291] Remove SQL from preferences.pl administrative script

[bugzilla-daemon]

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18291

--- Comment #14 from Jonathan Druart  
---
(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.
___
Koha-bugs mailing list
Koha-bugs@lists.koha-community.org
http://lists.koha-community.org/cgi-bin/mailman/listinfo/koha-bugs
website : http://www.koha-community.org/
git : http://git.koha-community.org/
bugs : http://bugs.koha-community.org/

[Koha-bugs] [Bug 18291] Remove SQL from preferences.pl administrative script

[bugzilla-daemon]

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18291

Jonathan Druart  changed:

   What|Removed |Added

 Status|In Discussion   |RESOLVED
 Resolution|--- |INVALID

-- 
You are receiving this mail because:
You are watching all bug changes.
___
Koha-bugs mailing list
Koha-bugs@lists.koha-community.org
http://lists.koha-community.org/cgi-bin/mailman/listinfo/koha-bugs
website : http://www.koha-community.org/
git : http://git.koha-community.org/
bugs : http://bugs.koha-community.org/

[Koha-bugs] [Bug 18291] Remove SQL from preferences.pl administrative script

[bugzilla-daemon]

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18291

--- Comment #13 from Alex Buckley  ---
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?

-- 
You are receiving this mail because:
You are watching all bug changes.
___
Koha-bugs mailing list
Koha-bugs@lists.koha-community.org
http://lists.koha-community.org/cgi-bin/mailman/listinfo/koha-bugs
website : http://www.koha-community.org/
git : http://git.koha-community.org/
bugs : http://bugs.koha-community.org/

[Koha-bugs] [Bug 18291] Remove SQL from preferences.pl administrative script

[bugzilla-daemon]

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18291

--- Comment #12 from Jonathan Druart  
---
Nope. Not everything is cached, but sysprefs are.

-- 
You are receiving this mail because:
You are watching all bug changes.
___
Koha-bugs mailing list
Koha-bugs@lists.koha-community.org
http://lists.koha-community.org/cgi-bin/mailman/listinfo/koha-bugs
website : http://www.koha-community.org/
git : http://git.koha-community.org/
bugs : http://bugs.koha-community.org/

[Koha-bugs] [Bug 18291] Remove SQL from preferences.pl administrative script

[bugzilla-daemon]

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18291

--- Comment #11 from Alex Buckley  ---
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?

-- 
You are receiving this mail because:
You are watching all bug changes.
___
Koha-bugs mailing list
Koha-bugs@lists.koha-community.org
http://lists.koha-community.org/cgi-bin/mailman/listinfo/koha-bugs
website : http://www.koha-community.org/
git : http://git.koha-community.org/
bugs : http://bugs.koha-community.org/

[Koha-bugs] [Bug 18291] Remove SQL from preferences.pl administrative script

[bugzilla-daemon]

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18291

Jonathan Druart  changed:

   What|Removed |Added

 Status|Needs Signoff   |In Discussion
 CC||m.de.r...@rijksmuseum.nl,
   ||martin.renvoize@ptfs-europe
   ||.com,
   ||n...@bywatersolutions.com,
   ||tomasco...@gmail.com

--- Comment #10 from Jonathan Druart  
---
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.
___
Koha-bugs mailing list
Koha-bugs@lists.koha-community.org
http://lists.koha-community.org/cgi-bin/mailman/listinfo/koha-bugs
website : http://www.koha-community.org/
git : http://git.koha-community.org/
bugs : http://bugs.koha-community.org/

[Koha-bugs] [Bug 18291] Remove SQL from preferences.pl administrative script

[bugzilla-daemon]

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18291

Alex Buckley  changed:

   What|Removed |Added

 Status|Failed QA   |Needs Signoff

--- Comment #9 from Alex Buckley  ---
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->{''}; to
$piece->{''}; 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

-- 
You are receiving this mail because:
You are watching all bug changes.
___
Koha-bugs mailing list
Koha-bugs@lists.koha-community.org
http://lists.koha-community.org/cgi-bin/mailman/listinfo/koha-bugs
website : http://www.koha-community.org/
git : http://git.koha-community.org/
bugs : http://bugs.koha-community.org/

[Koha-bugs] [Bug 18291] Remove SQL from preferences.pl administrative script

[bugzilla-daemon]

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18291

--- Comment #8 from Alex Buckley  ---
Created attachment 62906
  -->
https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=62906=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.
___
Koha-bugs mailing list
Koha-bugs@lists.koha-community.org
http://lists.koha-community.org/cgi-bin/mailman/listinfo/koha-bugs
website : http://www.koha-community.org/
git : http://git.koha-community.org/
bugs : http://bugs.koha-community.org/

[Koha-bugs] [Bug 18291] Remove SQL from preferences.pl administrative script

[bugzilla-daemon]

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18291

Alex Buckley  changed:

   What|Removed |Added

  Attachment #62905|0   |1
is obsolete||

-- 
You are receiving this mail because:
You are watching all bug changes.
___
Koha-bugs mailing list
Koha-bugs@lists.koha-community.org
http://lists.koha-community.org/cgi-bin/mailman/listinfo/koha-bugs
website : http://www.koha-community.org/
git : http://git.koha-community.org/
bugs : http://bugs.koha-community.org/

[Koha-bugs] [Bug 18291] Remove SQL from preferences.pl administrative script

[bugzilla-daemon]

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18291

--- Comment #7 from Alex Buckley  ---
Created attachment 62905
  -->
https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=62905=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.
___
Koha-bugs mailing list
Koha-bugs@lists.koha-community.org
http://lists.koha-community.org/cgi-bin/mailman/listinfo/koha-bugs
website : http://www.koha-community.org/
git : http://git.koha-community.org/
bugs : http://bugs.koha-community.org/

[Koha-bugs] [Bug 18291] Remove SQL from preferences.pl administrative script

[bugzilla-daemon]

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18291

Alex Buckley  changed:

   What|Removed |Added

  Attachment #62518|0   |1
is obsolete||

-- 
You are receiving this mail because:
You are watching all bug changes.
___
Koha-bugs mailing list
Koha-bugs@lists.koha-community.org
http://lists.koha-community.org/cgi-bin/mailman/listinfo/koha-bugs
website : http://www.koha-community.org/
git : http://git.koha-community.org/
bugs : http://bugs.koha-community.org/

[Koha-bugs] [Bug 18291] Remove SQL from preferences.pl administrative script

[bugzilla-daemon]

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18291

Alex Buckley  changed:

   What|Removed |Added

  Attachment #62517|0   |1
is obsolete||

-- 
You are receiving this mail because:
You are watching all bug changes.
___
Koha-bugs mailing list
Koha-bugs@lists.koha-community.org
http://lists.koha-community.org/cgi-bin/mailman/listinfo/koha-bugs
website : http://www.koha-community.org/
git : http://git.koha-community.org/
bugs : http://bugs.koha-community.org/

[Koha-bugs] [Bug 18291] Remove SQL from preferences.pl administrative script

[bugzilla-daemon]

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18291

Jonathan Druart  changed:

   What|Removed |Added

 CC||jonathan.dru...@bugs.koha-c
   ||ommunity.org
 Status|Signed Off  |Failed QA

--- Comment #6 from Jonathan Druart  
---
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.
___
Koha-bugs mailing list
Koha-bugs@lists.koha-community.org
http://lists.koha-community.org/cgi-bin/mailman/listinfo/koha-bugs
website : http://www.koha-community.org/
git : http://git.koha-community.org/
bugs : http://bugs.koha-community.org/

[Koha-bugs] [Bug 18291] Remove SQL from preferences.pl administrative script

[bugzilla-daemon]

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18291

Jonathan Druart  changed:

   What|Removed |Added

Version|17.05   |unspecified

-- 
You are receiving this mail because:
You are watching all bug changes.
___
Koha-bugs mailing list
Koha-bugs@lists.koha-community.org
http://lists.koha-community.org/cgi-bin/mailman/listinfo/koha-bugs
website : http://www.koha-community.org/
git : http://git.koha-community.org/
bugs : http://bugs.koha-community.org/

[Koha-bugs] [Bug 18291] Remove SQL from preferences.pl administrative script

[bugzilla-daemon]

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18291

Marc Véron  changed:

   What|Removed |Added

 Status|Needs Signoff   |Signed Off
 CC||ve...@veron.ch

-- 
You are receiving this mail because:
You are watching all bug changes.
___
Koha-bugs mailing list
Koha-bugs@lists.koha-community.org
http://lists.koha-community.org/cgi-bin/mailman/listinfo/koha-bugs
website : http://www.koha-community.org/
git : http://git.koha-community.org/
bugs : http://bugs.koha-community.org/

[Koha-bugs] [Bug 18291] Remove SQL from preferences.pl administrative script

[bugzilla-daemon]

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18291

Marc Véron  changed:

   What|Removed |Added

  Attachment #61236|0   |1
is obsolete||

--- Comment #5 from Marc Véron  ---
Created attachment 62518
  -->
https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=62518=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 

-- 
You are receiving this mail because:
You are watching all bug changes.
___
Koha-bugs mailing list
Koha-bugs@lists.koha-community.org
http://lists.koha-community.org/cgi-bin/mailman/listinfo/koha-bugs
website : http://www.koha-community.org/
git : http://git.koha-community.org/
bugs : http://bugs.koha-community.org/

[Koha-bugs] [Bug 18291] Remove SQL from preferences.pl administrative script

[bugzilla-daemon]

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18291

Marc Véron  changed:

   What|Removed |Added

  Attachment #61235|0   |1
is obsolete||

--- Comment #4 from Marc Véron  ---
Created attachment 62517
  -->
https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=62517=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

Followed test plan from comment #3, works as expected
Signed-off-by: Marc Véron 

-- 
You are receiving this mail because:
You are watching all bug changes.
___
Koha-bugs mailing list
Koha-bugs@lists.koha-community.org
http://lists.koha-community.org/cgi-bin/mailman/listinfo/koha-bugs
website : http://www.koha-community.org/
git : http://git.koha-community.org/
bugs : http://bugs.koha-community.org/

[Koha-bugs] [Bug 18291] Remove SQL from preferences.pl administrative script

[bugzilla-daemon]

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18291

Alex Buckley  changed:

   What|Removed |Added

 Status|ASSIGNED|Needs Signoff

--- Comment #3 from Alex Buckley  ---
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 

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.
___
Koha-bugs mailing list
Koha-bugs@lists.koha-community.org
http://lists.koha-community.org/cgi-bin/mailman/listinfo/koha-bugs
website : http://www.koha-community.org/
git : http://git.koha-community.org/
bugs : http://bugs.koha-community.org/

[Koha-bugs] [Bug 18291] Remove SQL from preferences.pl administrative script

[bugzilla-daemon]

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18291

--- Comment #2 from Alex Buckley  ---
Created attachment 61236
  -->
https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=61236=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

-- 
You are receiving this mail because:
You are watching all bug changes.
___
Koha-bugs mailing list
Koha-bugs@lists.koha-community.org
http://lists.koha-community.org/cgi-bin/mailman/listinfo/koha-bugs
website : http://www.koha-community.org/
git : http://git.koha-community.org/
bugs : http://bugs.koha-community.org/

[Koha-bugs] [Bug 18291] Remove SQL from preferences.pl administrative script

[bugzilla-daemon]

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18291

--- Comment #1 from Alex Buckley  ---
Created attachment 61235
  -->
https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=61235=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.
___
Koha-bugs mailing list
Koha-bugs@lists.koha-community.org
http://lists.koha-community.org/cgi-bin/mailman/listinfo/koha-bugs
website : http://www.koha-community.org/
git : http://git.koha-community.org/
bugs : http://bugs.koha-community.org/