Bug 13291 - Remove prog option from OPACFallback syspref
Summary: Remove prog option from OPACFallback syspref
Status: ASSIGNED
Alias: None
Product: Koha
Classification: Unclassified
Component: System Administration (show other bugs)
Version: Main
Hardware: All All
: P5 - low enhancement
Assignee: Indranil Das Gupta
QA Contact: Testopia
URL:
Keywords:
: 13444 (view as bug list)
Depends on:
Blocks:
 
Reported: 2014-11-18 18:49 UTC by Frédéric Demians
Modified: 2019-10-30 14:33 UTC (History)
7 users (show)

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


Attachments
Bug 13291 - remove prog as fallback theme (2.66 KB, patch)
2015-07-11 18:28 UTC, Indranil Das Gupta
Details | Diff | Splinter Review
Bug 13291 - remove prog as fallback theme (3.36 KB, patch)
2015-07-11 19:46 UTC, Indranil Das Gupta
Details | Diff | Splinter Review
Bug 13291 - remove prog as fallback theme (3.36 KB, patch)
2015-07-11 19:51 UTC, Indranil Das Gupta
Details | Diff | Splinter Review
Bug 13291 - remove prog as fallback theme (4.47 KB, patch)
2015-07-11 20:46 UTC, Mark Tompsett
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Frédéric Demians 2014-11-18 18:49:54 UTC
Since 'prog' theme doesn't exist anymore, OPACFallback syspref is without object anymore.
Comment 1 Katrin Fischer 2014-11-19 19:17:05 UTC
I think the idea was to avoid a similar problem in the future and also allow poeple to add their own theme and have it fall back to the main theme.
Comment 2 Indranil Das Gupta 2015-07-11 18:05:16 UTC
*** Bug 13444 has been marked as a duplicate of this bug. ***
Comment 3 Indranil Das Gupta 2015-07-11 18:28:25 UTC Comment hidden (obsolete)
Comment 4 Mark Tompsett 2015-07-11 19:16:49 UTC
Comment on attachment 40946 [details] [review]
Bug 13291 - remove prog as fallback theme

Review of attachment 40946 [details] [review]:
-----------------------------------------------------------------

::: installer/data/mysql/atomicupdate/bug_13291-remove-prog-as-listed-fallback-theme.sql
@@ +1,1 @@
> +INSERT IGNORE INTO systempreferences (variable,value,explanation,options,type) VALUES ('OPACFallback', 'bootstrap', 'Define the fallback theme for the OPAC interface.', 'bootstrap', 'Themes');

Works fine for systems lacking the system preference, but where's the UPDATE for those which already have it set?
Comment 5 Indranil Das Gupta 2015-07-11 19:46:10 UTC Comment hidden (obsolete)
Comment 6 Indranil Das Gupta 2015-07-11 19:51:13 UTC Comment hidden (obsolete)
Comment 7 Indranil Das Gupta 2015-07-11 19:52:30 UTC
(In reply to M. Tompsett from comment #4)
> Comment on attachment 40946 [details] [review] [review]
> Bug 13291 - remove prog as fallback theme
> Works fine for systems lacking the system preference, but where's the UPDATE
> for those which already have it set?

Addressed. Also adds the missing edits to opac.pref.
Comment 8 Mark Tompsett 2015-07-11 20:46:10 UTC
Created attachment 40949 [details] [review]
Bug 13291 - remove prog as fallback theme

The 'prog' theme for OPAC was removed from Koha. The OPACFallback
syspref was added in 3.18.0 with prog|bootstrap options. This patch
proposes to remove the prog option, but still maintain the syspref
for reasons outlined in Bug 13291 comment #1.

TEST PLAN
---------
 1) back up DB
 2) in sql client:
    > select * from systempreferences where variable like '%OPACFallback%';
    -- 1 record, should have bootstrap and prog choices,
       and likely prog fallback.
 3) at command line:
    ./installer/data/mysql/updatedatabase.pl
 4) in sql client:
    > select * from systempreferences where variable like '%OPACFallback%';
    -- 1 record, should have bootstrap and bootstrap values now.
 5) in sql client:
    > delete from systempreferences where variable like '%OPACFallback%';
    -- now there should be no OPACFallback system preference.
 6) at command line:
    ./installer/data/mysql/updatedatabase.pl
 7) in sql client:
   > select * from systempreferences where variable like '%OPACFallback%';
    -- 1 record, should have bootstrap and bootstrap values now.
 8) in sql client (substitute your koha database name!):
    > drop database koha_library;
    > create database koha_library;
    > use koha_library;
 9) Run web installer
10) in sql client:
    > select * from systempreferences where variable like '%OPACFallback%';
    -- 1 record, should have bootstrap and bootstrap values now.
11) run koha qa test tools

Signed-off-by: Mark Tompsett <mtompset@hotmail.com>
Comment 9 Jonathan Druart 2015-07-13 13:08:57 UTC
I am not sure a syspref with 1 option is really useful.
The fallback could be done automatically on the bootstrap theme, don't you think?
Comment 10 Katrin Fischer 2015-07-13 13:49:58 UTC
Hm, thinking about this, you are probably right. If you have your own set of templates, hardcoding the fallback to the only available OPAC template seems right. OTH if we want to allow people to add other fallbacks and avoid changing files in Koha, this should not be a pull down but a text field?
Hm, I feel like I am missing something.
Comment 11 Indranil Das Gupta 2015-07-13 19:40:54 UTC
(In reply to Jonathan Druart from comment #9)
> I am not sure a syspref with 1 option is really useful.

we already have a related syspref with 1 option - opacthemes

> The fallback could be done automatically on the bootstrap theme, don't you
> think?

We seem to have that already in C4/Templates.pm in the form of

my $hardcoded_theme = ($interface eq 'intranet') ? 'prog' : 'bootstrap';
Comment 12 Indranil Das Gupta 2015-07-13 19:47:06 UTC
(In reply to Katrin Fischer from comment #10)
> Hm, thinking about this, you are probably right. If you have your own set of
> templates, hardcoding the fallback to the only available OPAC template seems
> right. OTH if we want to allow people to add other fallbacks and avoid
> changing files in Koha, this should not be a pull down but a text field?

Sounds like a fair argument - like say bootstrap|prog|custom_theme_name

The ordering of these could then drive the selection in  C4::Templates::themelanguage()

Not that it would address Tomas's observation in line 243 about the hardcoding in case of an unpopulated db
Comment 13 Mark Tompsett 2015-07-13 19:59:28 UTC
(In reply to Indranil Das Gupta from comment #12)
> (In reply to Katrin Fischer from comment #10)
> > OTH if we want to allow people to add other fallbacks and avoid
> > changing files in Koha, this should not be a pull down but a text field?
> 
> Sounds like a fair argument - like say bootstrap|prog|custom_theme_name

Yuck, no. Let's not do that. Drop downs prevent people messing up data. If people wish to install custom themes, it has to go in a known directory. Can't we calculate the drop down from the directory, rather than require the list be updated in the opac.pref file? Yes, I know the system preference code might get ugly looking. But I rather have ugly code than bad, hard to debug data.
Comment 14 Mark Tompsett 2015-07-13 20:01:38 UTC
(In reply to Jonathan Druart from comment #9)
> I am not sure a syspref with 1 option is really useful.
> The fallback could be done automatically on the bootstrap theme, don't you
> think?

Though 1 is kind of silly now, it does allow for bringing back multiple themes in the future.
Comment 15 Katrin Fischer 2015-07-19 21:45:26 UTC
(In reply to M. Tompsett from comment #13)
> (In reply to Indranil Das Gupta from comment #12)
> > (In reply to Katrin Fischer from comment #10)
> > > OTH if we want to allow people to add other fallbacks and avoid
> > > changing files in Koha, this should not be a pull down but a text field?
> > 
> > Sounds like a fair argument - like say bootstrap|prog|custom_theme_name
> 
> Yuck, no. Let's not do that. Drop downs prevent people messing up data. If
> people wish to install custom themes, it has to go in a known directory.
> Can't we calculate the drop down from the directory, rather than require the
> list be updated in the opac.pref file? Yes, I know the system preference
> code might get ugly looking. But I rather have ugly code than bad, hard to
> debug data.

I think that would be overengineering this a bit. We have other prefs that should not really be changed without knowing what you do. And I wonder what would happen if you enter something wrong - we could always make it fall back to the standard templates.
Comment 16 Nick Clemens (kidclamp) 2019-06-12 10:58:55 UTC
Four years later, no more themes to choose from. Perhaps we could remove this syspref?
Comment 17 Mark Tompsett 2019-06-13 03:25:21 UTC
(In reply to Nick Clemens from comment #16)
> Four years later, no more themes to choose from. Perhaps we could remove
> this syspref?

I believe our naming/id'ing of objects has really moved us away from themes via separate files, and towards CSS-based themes only. As such, I agree this "In Discussion" should probably be changed to a "Let's scrap the Fallback. Here's a patch. Needs Sign off". Anyone object?
Comment 18 Katrin Fischer 2019-06-13 05:44:55 UTC
I know that themes are still used in libraries, even some around here. Does it really hurt us to keep it? Hea shows also shows use of opacthemes, but OpacFallback seems not to be recorded.