Bug 14135 - Local Use syspref addition form missing variable type 'Free'
Summary: Local Use syspref addition form missing variable type 'Free'
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Templates (show other bugs)
Version: master
Hardware: All All
: P5 - low minor (vote)
Assignee: Indranil Das Gupta
QA Contact: Testopia
URL:
Whiteboard:
Keywords:
Depends on:
Blocks: 14268
  Show dependency treegraph
 
Reported: 2015-05-04 13:06 UTC by Indranil Das Gupta
Modified: 2016-12-05 21:23 UTC (History)
9 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Trivial patch
Bot Control: ---
When did the bot last check this:
Who signed the patch off:
Text to go in the release notes:


Attachments
Bug 14135 [ENH] Adds 'Free' to variabletypes in systempreferences.tt The 'Local Use' system preference addition/modification template provides the following options against "Variable Type" - Choice, YesNo, Integer, Textarea, Float, Themes, Languages, Uplo (3.34 KB, patch)
2015-05-04 13:50 UTC, Indranil Das Gupta
Details | Diff | Splinter Review
Bug 14135 Adds 'Free' to variabletypes in systempreferences.tt The 'Local Use' system preference addition/modification template provides the following options against "Variable Type" - Choice, YesNo, Integer, Textarea, Float, Themes, Languages, Upload or (3.50 KB, patch)
2015-05-06 19:51 UTC, M. Tompsett
Details | Diff | Splinter Review
Bug 14135 - Local Use syspref addition form missing variable type 'Free' (3.56 KB, patch)
2015-05-15 05:11 UTC, Amit Gupta
Details | Diff | Splinter Review
Bug 14135 [ENH] Adds 'Free' to variabletypes in systempreferences.tt The 'Local Use' system preference addition/modification template provides the following options against "Variable Type" - Choice, YesNo, Integer, Textarea, Float, Themes, Languages, Uplo (3.63 KB, patch)
2015-05-22 13:35 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 14135: Adds 'Free' to variabletypes in systempreferences.tt (3.63 KB, patch)
2015-05-22 13:36 UTC, Jonathan Druart
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Indranil Das Gupta 2015-05-04 13:06:55 UTC
The 'Local Use' system preference addition/modification template provides the following options against "Variable Type" - Choice, YesNo, Integer, Textarea, Float, Themes, Languages, Upload or ClassSource.

But there is no option presented for "Free" which seems to be the most used variable type out-of-the-box (i.e. INTRAdidyoumean, OPACdidyoumean, UsageStatsID and UsageStatsLastUpdateTime)

This trivial patch proposes to modify the systempreferences.tt and add the option 'Free' to the list offered to users.
Comment 1 Indranil Das Gupta 2015-05-04 13:50:03 UTC Comment hidden (obsolete)
Comment 2 M. Tompsett 2015-05-06 19:44:51 UTC
This is not an enhancement. It's a bug fix, since Free should have been there in the first place. This is not adding anything new (another type) that wasn't there before, nor is it brand new functionality.
Comment 3 M. Tompsett 2015-05-06 19:46:04 UTC
Obviously, Indranil Das Gupta is the bug taker, not Owen Leonard. :)
Comment 4 M. Tompsett 2015-05-06 19:51:09 UTC Comment hidden (obsolete)
Comment 5 Indranil Das Gupta 2015-05-06 19:55:35 UTC
(In reply to M. Tompsett from comment #3)
> Obviously, Indranil Das Gupta is the bug taker, not Owen Leonard. :)

Ack! thanks for the cluebat! :P
Comment 6 Amit Gupta 2015-05-15 05:11:17 UTC Comment hidden (obsolete)
Comment 7 Marcel de Rooy 2015-05-21 07:12:14 UTC
Just a dumb question: What happens if you translate the staff client with these pref types here? Clicking on a translation would still work (insert the original English), but would the user understand?
Should we not convert this text box to a select now ?
Comment 8 M. Tompsett 2015-05-21 10:53:16 UTC
(In reply to Marcel de Rooy from comment #7)
> Just a dumb question: What happens if you translate the staff client with
> these pref types here? Clicking on a translation would still work (insert
> the original English), but would the user understand?
> Should we not convert this text box to a select now ?

Actually, that is a valid point. Converting to a drop down selection list would be better. That would also prevent the entry of invalid types. My sign off was to encourage development. :)
Comment 9 Jonathan Druart 2015-05-22 13:35:33 UTC Comment hidden (obsolete)
Comment 10 Jonathan Druart 2015-05-22 13:36:06 UTC
Created attachment 39408 [details] [review]
Bug 14135: Adds 'Free' to variabletypes in systempreferences.tt

The 'Local Use' system preference addition/modification template provides the following options against "Variable Type" - Choice, YesNo, Integer, Textarea, Float, Themes, Languages, Upload or ClassSource.

There is no option presented for "Free" which seems to be the most
used variable type out-of-the-box (i.e. INTRAdidyoumean,
OPACdidyoumean, UsageStatsID and UsageStatsLastUpdateTime)

This trivial patch proposes to modify the systempreferences.tt
and add the option 'Free' to the list offered to users.

Test Plan
=========

1/ Go to Home > Administration > System preferences > Local use
2/ Click on 'New preference'.
3/ In the fieldset 'Koha Internal', the variable types offered
   are Choice, YesNo, Integer, Textarea, Float, Themes,
   Languages, Upload or ClassSources.
4/ Clicking on 'Choice' should set the 'preftype' field as
   'Choice'.
5/ Apply this patch.
6/ Refresh the page.
7/ The variable types list should read - "Free, Choice, YesNo,
   Integer, Textarea, Float, Themes, Languages, Upload or
   ClassSources".
8/ Clicking on 'Free' should set the 'preftype' field as 'Free'.

Signed-off-by: Mark Tompsett <mtompset@hotmail.com>

NOTE: Discovered that there is no validation on the type field.
      However, that is beyond the scope of this bug.

Signed-off-by: Amit Gupta <amit.gupta@informaticsglobal.com>

Signed-off-by: Jonathan Druart <jonathan.druart@koha-community.org>
Comment 11 Marcel de Rooy 2015-05-22 13:53:18 UTC
Jonathan: See comments above. I would prefer a select instead of adding more text.
Comment 12 Katrin Fischer 2015-05-24 14:17:33 UTC
Maybe we could switch this to a select on a separate bug? I wonder why it wasn't one in the first place - maybe to allow adding of new types?
Comment 13 M. Tompsett 2015-05-24 15:52:28 UTC
(In reply to Katrin Fischer from comment #12)
> Maybe we could switch this to a select on a separate bug?

I agree. :) This is a good temporary step.
Comment 14 Indranil Das Gupta 2015-05-24 16:49:56 UTC
(In reply to comment #11, comment #12 and comment #13)

Hi Katrin / Marcel / Mark,

my thoughts - bug 14135 fixes a bug, whereas what you all want is an enhancement.

1/ should the QA manager and RM agree, then lets push this into master

2/ open a new bug which proposes to move to a select

3/ have the types read off (perhaps) a yaml file. less hardcoding, more manageable.

I've the patch ready for that. what say you?
Comment 15 M. Tompsett 2015-05-24 17:04:41 UTC
(In reply to Indranil Das Gupta from comment #14)
> I've the patch ready for that. what say you?

Yes. Less talk more bug creation. I need a sign off. ;)
Comment 16 Katrin Fischer 2015-05-24 21:59:08 UTC
Agreeing to 1 and 2. 3 seems a bit overkill as it seems not very likely that we are going to add more types at the moment.
Comment 17 Marcel de Rooy 2015-05-25 08:42:12 UTC
(In reply to Indranil Das Gupta from comment #14)
> (In reply to comment #11, comment #12 and comment #13)
> 
> Hi Katrin / Marcel / Mark,
> 
> my thoughts - bug 14135 fixes a bug, whereas what you all want is an
> enhancement.
> 
> 1/ should the QA manager and RM agree, then lets push this into master
> 
> 2/ open a new bug which proposes to move to a select
> 
> 3/ have the types read off (perhaps) a yaml file. less hardcoding, more
> manageable.
> 
> I've the patch ready for that. what say you?

Indranil: Do not keep your patches there, submit them :)
I agree with Katrin about point 3.
Comment 18 Indranil Das Gupta 2015-05-25 11:24:44 UTC
(In reply to comment #8, comment #11 and comment #16)

New dropdown enhancement patch at Bug 14268

Note: has bug 14135 as dependency
Comment 19 Indranil Das Gupta 2015-05-25 22:28:07 UTC
(In reply to Marcel de Rooy from comment #17)
> (In reply to Indranil Das Gupta from comment #14)
> > (In reply to comment #11, comment #12 and comment #13)
<snipped>
> > 3/ have the types read off (perhaps) a yaml file. less hardcoding, more
> > manageable.
> > 
> > I've the patch ready for that. what say you?
> 
> Indranil: Do not keep your patches there, submit them :)
> I agree with Katrin about point 3.

Ok! so I'm in a loop here again. Based on 14135, we have the following list - 'Free', 'Choice', 'YesNo', 'Integer', 'Textarea', 'Float', 'Themes', 'Languages', 'Upload' and 'ClassSources'

On a hunch, I decided to fire `SELECT DISTINCT type FROM systempreferences`. What is got back was : Choice, YesNo, Free, Textarea, multiple, Integer, ClassSources, Languages, File, Themes, NULL, short

I can see the 1-to-1 mapping for most of the types,  but not all. Can someone take a look and comment?
Comment 20 M. Tompsett 2015-05-26 01:24:28 UTC
(In reply to Indranil Das Gupta from comment #19)
> Ok! so I'm in a loop here again. ... we have the following list
> - 'Free', 'Choice', 'YesNo', 'Integer', 'Textarea', 'Float', 'Themes',
> 'Languages', 'Upload' and 'ClassSources'
> 
> On a hunch, I decided to fire `SELECT DISTINCT type FROM systempreferences`.
> What is got back was : Choice, YesNo, Free, Textarea, multiple, Integer,
> ClassSources, Languages, File, Themes, NULL, short
> 
> I can see the 1-to-1 mapping for most of the types,  but not all. Can
> someone take a look and comment?

I can confirm seeing 'multiple', 'File', '', and 'short'. I believe this is beyond the scope of this patch, though it may merit a clean up bug report. No need to comment or reply on this bug any further. :)
Comment 21 Tomás Cohen Arazi 2015-07-07 18:30:46 UTC
Patch pushed to master.

Thanks Indranil!
Comment 22 Chris Cormack 2015-07-14 08:23:00 UTC
Pushed to 3.20.x will be in 3.20.2
Comment 23 Liz Rea 2015-07-20 22:15:21 UTC
Pushed to 3.18.x will be in 3.18.09
Comment 24 Mason James 2015-07-25 13:06:30 UTC
Pushed to 3.16.x, will be in 3.16.13