396974 - Custom fields: Ability to say "during this status, this field must have a value"

Status: NEW

(Bugzilla :: Bugzilla-General, enhancement, P3)

Description

[Kevin Benton]

If the user attempts to file or update a bug with an invalid value, the JavaScript field validator complains as does the CGI.  Required fields default to "Unspecified" or "CURRENT" (in special circumstances).

Here's an example of one use of this:

First Affected Release Rev - defaults to Unspecified and may not be filed until changed from Unspecified
Last Affected Release Rev - defaults to CURRENT until state is changed to RESOLVED at which point CURRENT is no longer allowed.

The exception to both these examples is when there is only one valid Rev available for the product, at which point, the default is the only allowed value.

Comment 1

[Pascal Held]

I've some ideas for this request:

At first every custom field get a field for a JavaScript is_visible that returns a true if the condition is given to show the field or false if the field should hidden. This sequenz should be defined on the edit custom field page.

On the edit bugs page this function will be run, when any field on the page changes. If the result is true the field will be show otherwise the field will be hidden.

I would try to implement this...

Comment 2

[Pascal Held]

Attachment: v1

I add an column to the fielddefs table to save the condition. On the edit custom field there is a new field, where the condition can be post. This condition will be check on the edit bugs or create bugs page

Comment 3

[Kevin Benton]

Comment on attachment 325950 [details] [diff] [review]
v1

I can see that it could be useful to have fields be unavailable in certain cases, however, this hides fields when the status doesn't dictate that the field must be filled.

Here's a good use case:  Defect Injection Stage may not be known when a bug is filed but I don't want to prevent users from entering that information (by hiding the field) when the status doesn't require the data (aka < RESOLVED).

I would also hope that fields would change style in such a way that when the status requires the field to have a value, that it becomes blatantly obvious that it's required (i.e. making the field heading bold/red).

This is a nice step in the right direction, however. :-)

Continuing through the patch - it looks like you have duplicate JS code.  I would consider putting it in one place and parameterizing it.

Maybe I'm missing it, however, I didn't see any code on the back-end to enforce that when the status changes, that the custom field has a new and valid (not Unspecified or ---) value...

Comment 4

[Pascal Held]

Attachment: v2

I also implement the setting for must set, reset and read only fields.

Comment 5

[Pascal Held]

Comment on attachment 326648 [details] [diff] [review]
v2

Sorry a made a misstake with the patch file.

Comment 6

[Pascal Held]

Attachment: v3

I add some changes to my first version.

1. Now you can set if the field is invisible, read only, necessary and if the field will be reset on submit. 

Read only field were disabled by the javascript, so you can't change them. If the field gets disabled during changing the bug fields, the field will be reset to the value before changing. On submit all fields were enabled, becouse some browsers don't send disabled fields.

Necessary fields where marked red, when no value is given.

Fields can be reset on submit. Here's a use case for this feature: You have a real bug with a severity. Now you mark the bug as an enhancement, so you don't want a value for the severity. It will deleted.

2. I removed all dublicated JS code

Comment 7

[Pascal Held]

This patch also implement Bug #371995 and Bug #291433. Maybe mark the others as duplicate of this bug?

Maybe a solution for bugzilla 3.2?

Comment 8

[Max Kanat-Alexander]

3.2 has been feature-frozen for a long time, this is definitely not a blocker.

Comment 9

[Myk Melez [:myk] [@mykmelez]]

Comment on attachment 325950 [details] [diff] [review]
v1

Canceling this review request, as I'm no longer able to do Bugzilla reviews.

Comment 10

[Myk Melez [:myk] [@mykmelez]]

Comment on attachment 327069 [details] [diff] [review]
v3

Canceling this review request, as I'm no longer able to do Bugzilla reviews.

Comment 11

[Myk Melez [:myk] [@mykmelez]]

Comment on attachment 325950 [details] [diff] [review]
v1

Canceling this review request, as I'm no longer able to do Bugzilla reviews.

Comment 12

[timeless]

Comment on attachment 327069 [details] [diff] [review]
v3

>Index: template/en/default/bug/create/created.html.tmpl

>+[% onload="" %]
>+[% IF Param("use_advanced_field_settings") %]
>+  [% onload="addUpdater()" %]
>+[% END %]

i'm not sure if this style is preferable over if/else...

>-  style_urls = [ "skins/standard/yui/calendar.css", "skins/standard/show_bug.css" ]
>+  style_urls = [ "skins/standard/yui/calendar.css", "skins/standard/show_bug.css" 
>+  onload = onload ]

this looks wrong, i don't think you want the ] moved...

>Index: template/en/default/bug/create/create.html.tmpl
>   javascript_urls = [ "js/attachment.js", "js/util.js", "js/keyword-chooser.js",
>                       "js/yui/yahoo-dom-event.js", "js/yui/calendar.js",
>                       "js/field.js" ]
>+  onload=onload

note that yo don't move the bracket here.

>+      enctype="multipart/form-data"  onsubmit="[% "return checkSubmit();" IF Param("use_advanced_field_settings") %]">

i'd rather onsubmit be inside the [% <here> IF %]

i presume this will work properly:
[% ' onsubmit="return checkSubmit();"' IF Param("use_advanced_field_settings") %]

>Index: template/en/default/bug/show.html.tmpl
>@@ -48,6 +52,7 @@
>     doc_section = "bug_page.html"
>+    onload=onload

style clearly includes spaces around =, please include them

>Index: template/en/default/bug/field.html.tmpl

>@@ -71,8 +70,7 @@
>                         [% SET field_size = field.legal_values.size %]
>                     [% END %]
>                     size="[% field_size FILTER html %]" multiple="multiple"
>-                [% END %]
>-                >
>+                [% END %]>

why this change? could you use -%] or +%] instead?

>Index: template/en/default/bug/edit.html.tmpl


>Index: template/en/default/admin/custom_fields/list.html.tmpl
> [% delete_contentlink = BLOCK %]editfields.cgi?action=del&amp;name=%%name%%[% END %]
>-
> [% columns = [

why did you remove that line?

>Index: checksetup.pl
> Bugzilla::Field::populate_field_definitions();
> 
>+
>+
> ###########################################################################

why did you add those lines?

>Index: Bugzilla/Field.pm
>@@ -409,6 +417,10 @@
> sub set_obsolete       { $_[0]->set('obsolete',    $_[1]); }
> sub set_sortkey        { $_[0]->set('sortkey',     $_[1]); }
> sub set_in_new_bugmail { $_[0]->set('mailhead',    $_[1]); }
>+sub set_show_condition { $_[0]->set('show_condition',$_[1]); }
>+sub set_read_only_condition { $_[0]->set('read_only_condition',$_[1]); }
>+sub set_must_set_condition { $_[0]->set('must_set_condition',$_[1]); }
>+sub set_reset_condition { $_[0]->set('reset_condition',$_[1]); }

the code seems to have all the { and $_'s aligned.... perhaps you should keep that custom?

>Index: Bugzilla/DB/Schema.pm
>@@ -624,6 +624,10 @@
>                             DEFAULT => 'FALSE'},
>             enter_bug   => {TYPE => 'BOOLEAN', NOTNULL => 1,
>                             DEFAULT => 'FALSE'},
>+            show_condition => {TYPE => 'LONGTEXT', NOTNULL => 0},
>+            read_only_condition => {TYPE => 'LONGTEXT', NOTNULL => 0},
>+    		must_set_condition => {TYPE => 'LONGTEXT', NOTNULL => 0},
>+    		reset_condition => {TYPE => 'LONGTEXT', NOTNULL => 0},

you're using tabs. don't.

>Index: template/en/default/admin/params/bugfields.html.tmpl
>                   "$terms.Bugzilla will then use the operating system that the browser " _
>-                  "reports to be running on as the default." }
>+                  "reports to be running on as the default.", 
>+                  
>+  use_advanced_field_settings => "This enables advandced settings for custom fields"

there's a spelling error here, and the line wrapped in bugzilla, i'd suggest that you include a line break near =>

>\ No newline at end of file

someone should fix that...

>Index: template/en/default/bug/advanced-field-settings.js.tmpl
>+    function addEvent(obj, eventType, func, useCaption){
>+	  if (obj.addEventListener) {
>+	    obj.addEventListener(eventType, func, useCaption);
>+	    return true;

since you're using "return" here, there's no need for "else".
>+	  } else if (obj.attachEvent) {

so it should be:
}
if (obj.attachEvent) {

>+	    var retVal = obj.attachEvent("on"+eventType, func);
>+	    return retVal;
>+	  } else {

same here, just:
}
return false;

(no extra {} block)
>+	    return false;

>+	

trailing whitespace is annoying, please don't use it :)
>+	function addUpdater() {

last complaint about tabs. please don't use them.

>+		for (var i=0;i<document.forms[1].elements.length;i++) {

i suspect style says to include a space after ;'s here ^^.

>+	}	

last complaint about trailing whitespace
>+	
>+  [% FOREACH field = Bugzilla.active_custom_fields %]
>+      function [% field.name FILTER html %]_is_visible() {
>+      function [% field.name FILTER html %]_must_set() {
>+      function [% field.name FILTER html %]_read_only() {
>+      function [% field.name FILTER html %]_reset() {

my that's a lot of functions....

>+      }
>+      else {

i think style is } else {

>+        document.getElementById("tr_[% field.name FILTER html%]").style.display="none"

please include a trailing ; (first and only warning)

>+        if (document.getElementById("[% field.name FILTER html%]").value!="[% bug.${field.name} FILTER html%]") {

this line's too long, please break near != (I'd use spaces around ==,!=,=...)

please cache document.getElementById() as a local variable, that'll solve your line length problems.

>+        	need_one_more_update=true;

see above

>+  if (need_one_more_update) {
>+    updateView();
>+  }
>+  }

something's wrong here.

>+    if (!ok) {
>+      alert(msg);
>+    }

I'm not a big fan of alerts...

>+    else {
>+     [% FOREACH field = Bugzilla.active_custom_fields %]
this isn't indented properly...
>+      [% IF field.read_only_condition %]
>+        document.getElementById("[% field.name FILTER html%]").disabled=false;

use a local variable for this getElementById before the if, and reuse the variable for the switch below.

>Index: template/en/default/admin/custom_fields/advanced-settings.html.tmpl

>+  # Contributor(s): Frédéric Buclin <[e-mail]>

is this a refactor? 

>+  #%]
>+ 
>+    [% IF Param("use_advanced_field_settings") %]
>+      <tr>
>+        <th align="right"><label for="show_condition">Show condition:</label></th>
>+        <td>
>+          [% INCLUDE global/textarea.html.tmpl 
>+          		cols="50" 

is this the proper alignment?

>+            Here you can set a JavaScript-Code that will check if the field is visible. It must return true or false.

Here you can include JavaScript to control whether the field should be visible.
Returning true will cause the field to be visible.

(and similarly for the others)

Comment 13

[Pascal Held]

Attachment: v4

I fixed the things from the last review.

Comment 14

[Frédéric Buclin]

Comment on attachment 331917 [details] [diff] [review]
v4

>-# Contributor(s): Terry Weissman <[e-mail]>
>+# Contributor(s): Pascal Held <[e-mail]>
>+#                 Terry Weissman <[e-mail]>
> #                 Dawn Endico <[e-mail]>
> #                 Dan Mosedale <[e-mail]>
> #                 Joe Robins <[e-mail]>

Add your name at the end of the contributors list, not at the top. You are not the creator or main author of these files.

Comment 15

[Frédéric Buclin]

Comment on attachment 331917 [details] [diff] [review]
v4


>-[% PROCESS global/header.html.tmpl
>+[% IF Param("use_advanced_field_settings") %]
>+  [% PROCESS global/header.html.tmpl
>+    title = "$terms.Bug $id Submitted"
>+    javascript_urls = [ "js/util.js", "js/keyword-chooser.js", "js/field.js",
>+                        "js/yui/yahoo-dom-event.js", "js/yui/calendar.js" ]
>+    style_urls = [ "skins/standard/yui/calendar.css", "skins/standard/show_bug.css" ] 
>+    onload = "addUpdater()"
>+  %]
>+[% ELSE %]
>+  [% PROCESS global/header.html.tmpl
>   title = "$terms.Bug $id Submitted"
>   javascript_urls = [ "js/util.js", "js/keyword-chooser.js", "js/field.js",
>                         "js/yui/yahoo-dom-event.js", "js/yui/calendar.js" ]
>-  style_urls = [ "skins/standard/yui/calendar.css", "skins/standard/show_bug.css" ]
>-  
>-  
>-%]

Don't duplicate code. Only append addUpdater() if required. This remark applies in several places.



>Index: template/en/default/admin/custom_fields/list.html.tmpl

>-  # Contributor(s): Frédéric Buclin <[e-mail]>
>+  # Contributor(s): Pascal Held <[e-mail]>
>+  #                 Frédéric Buclin <[e-mail]>

No reason to add yourself to the list of contributors of a file you didn't alter. Please remove your name from this list. (Now my personal feeling about this list of contributors: adding his own name to the list when the only change consists to add or remove a single line doesn't justify it, IMO.)



>Index: editfields.cgi

>+        show_condition      => $cgi->param('show_condition'),   

All these lines need "scalar" being prepended to $cgi->param(), else data is incorrectly passed (when a param is undefined or an array, e.g. by hacking the URL).


>+    $field->set_show_condition($cgi->param('show_condition'));

Same here.



>Index: checksetup.pl

>+if (!$dbh->bz_column_info('fielddefs', 'show_condition')) {
>+    $dbh->bz_add_column('fielddefs', 'show_condition', 
>+    	{TYPE => 'LONGTEXT', NOTNULL => 0});
>+}

Don't change the DB from here. Look at Bugzilla/Install/DB.pm.



>Index: Bugzilla/Field.pm

>+sub set_show_condition      { $_[0]->set('show_condition',      $_[1]); }
>+sub set_read_only_condition { $_[0]->set('read_only_condition', $_[1]); }
>+sub set_must_set_condition  { $_[0]->set('must_set_condition',  $_[1]); }
>+sub set_reset_condition     { $_[0]->set('reset_condition',     $_[1]); }

Those need real validators. One may pass invalid regexps to JS. We cannot accept everything and hope it will just work.


>+            show_condition      => {TYPE => 'LONGTEXT', NOTNULL => 0},
>+            read_only_condition => {TYPE => 'LONGTEXT', NOTNULL => 0},
>+            must_set_condition  => {TYPE => 'LONGTEXT', NOTNULL => 0},
>+            reset_condition     => {TYPE => 'LONGTEXT', NOTNULL => 0},

Don't write NOTNULL => 0. You only need to write it when its value is 1.



>Index: template/en/default/admin/params/bugfields.html.tmpl

>+  use_advanced_field_settings => 
>+                  "This enables advanced settings for custom fields" }

This is a bit vague. How are we supposed to know what these advanced settings are?


Note that this is an ultra-quick review. I didn't look at the logic of the patch at all, nor if it works as expected, nor if the UI is fine,... nor if that's the way we want it to be implemented (mkanat's opinion would be interesting about it).

Comment 16

[Max Kanat-Alexander]

I'm honestly not even sure I want this feature upstream yet. If we add it, it has to be *very* unobtrusive. If it can't be architected and displayed in an unobtrusive fashion, we should wait until we have the general architecture of Bugzilla improved to the point where this is simple to add.

Comment 17

[Frédéric Buclin]

We are going to branch for Bugzilla 4.4 next week and this bug is either too invasive to be accepted for 4.4 at this point or shows no recent activity. The target milestone is reset and will be set again *only* when a patch is attached and approved.

I ask the assignee to reassign the bug to the default assignee if you don't plan to work on this bug in the near future, to make it clearer which bugs should be fixed by someone else.