Configuration Option "enabled_on_bugnote_add_form" #37
Conversation
…sBT standard category concept.
config_update.php refactoring.
| ) | ||
| ); | ||
| } | ||
| function register() { |
There was a problem hiding this comment.
Please follow Mantis general coding guidelines. Especially:
Indentations: use TABS with a size of 4 (not spaces)
Your commit has marked all the file as diff because it has turned tabs into spaces.
There was a problem hiding this comment.
I'm sorry, I'm going to check my editor settings
| maybe_set_option( 'categories', gpc_get_string( 'categories' ) ); | ||
| maybe_set_option( 'stopwatch_enabled', gpc_get_int( 'stopwatch_enabled', OFF ) ); | ||
| $config = \TimeTrackingPlugin::getConfig(); | ||
| foreach($config as $opt => $default ) { |
There was a problem hiding this comment.
Is this really needed? seems a lot of complexity for a few set of options
What happens if at some point there are some config options that are not managed through this form?
There was a problem hiding this comment.
I personally do not like a lot seeing lot of similar lines that seems copy & paste, that's why I prefer what I consider a simple algorithm.
If I'm not wrong it is supposed that all config options will be managed by config page.
| $fn = 'gpc_get_int'; | ||
| if( strpos($opt, 'enabled') !== FALSE ) { | ||
| // is a bool like 1/0 | ||
| $default = OFF; |
There was a problem hiding this comment.
Actually, to be correct, we should use gpc_get_bool()
| if( is_string($default) ) { | ||
| $fn = 'gpc_get_string'; | ||
| } | ||
| plugin_config_set( $opt, $fn( $opt, $default ) ); |
There was a problem hiding this comment.
This is not clean in the way that it will always save the option in database, even if the value is the same as the default.
That was the intention of the previous function maybe_set_option(). Actually that function may be also wrong, as it really should do:
- if the new value is different from default, save it
- if the new value is same as default: don't save it, and delete the current value if it exists.
That's the usual way mantis treats configuration options
There was a problem hiding this comment.
I can rethink this, but it's not clear for me why this so important to avoid a write if value has not been changed. If standard way to work has to be as described, then IMHO it will be better that plugin_config_set() will do all the logic. What do you think about this last approach?
There was a problem hiding this comment.
Reviewing the working of plugin config, it's a bit different from core config.
The plugin config options are always stored in database, as part of plugin setup, which reads the initial values from calling config()
So, it's not bad to "always write" the option. Not clean either... but..ok
Note, the scnario i described was for core configurations, where values may exists as defaults in configuration file instead of database
important to avoid a write if value has not been changed
one reason is to avoid unneeded database writes.
TimeTracking/TimeTracking.php
Outdated
| 'stopwatch_enabled' => ON, | ||
|
|
||
| 'categories' => '', | ||
| 'enabled_on_bugnote_add_form' => ON |
There was a problem hiding this comment.
maybe a shorter name is cleaner?
Eg: bugnote_form_enabled
(placing "enabled" at the end, following the other option naming)
There was a problem hiding this comment.
I've though about putting enabled at the end, but I was not able to find a good name. bugnote_form_enabled is not clear IMHO, I've given a look to original config option on standard MantisBT and name was $g_time_tracking_without_note, if you think it will be better this name can be used, please let me know
There was a problem hiding this comment.
time_tracking_without_note was used in a different way: to allow a time entry with an empty bugnote. Because in core, a bugnote is required to have content, that options was used to override that validation and allow inserting an empty bugnote for the purpose of storing the time entry.
Here we don't have that limitation, and not use for that option.
As we are talking about the naming, and following your latest change, i'm fine with just bugnote_form_enabled.
TimeTracking/TimeTracking.php
Outdated
| * @param type $p_bug_id | ||
| */ | ||
| function ev_bugnote_add_form( $p_event, $p_bug_id ) { | ||
| $t_enabled_on_bugnote_add_form = plugin_config_get('enabled_on_bugnote_add_form'); |
There was a problem hiding this comment.
Note that config options can be overridden for specific projects and/or users.
I see value at setting this option individually for some projects. Even if the UI does not support, this should be supported here.
Should we add explicit project from the bug_id as plugin_config_get parameter?
Omitting them, will relay to config_get for current user & project overrides, but:
Is there a scenario where this form is rendered for a bug_id, with a different "current project" than the bug's project?
Configuration Option "enabled_on_bugnote_add_form"
config_update.php refactoring
changed label from 'category' to 'activity type'