It would be great to have the ability to mark the custom field as mandatory to guarantee that the user will set a value for the field. It could display a message like: "You must set a value for the field...".

Hi Max, Could you review this patch? It is still missing the new attribute usage in the post_bug and process_bug scripts, just wondering if I'm in the right way. Thank you!

Comment on attachment 399068 [details] [diff] [review] Adds a new attribute 'mandatory' to custom fields Let's call it is_mandatory. >Index: Bugzilla/Field.pm >+ mandatory => \&_check_mandatory, You can just use Bugzilla::Object::check_boolean. You're welcome to convert the other boolean checkers in this file simultaneously, if you want. >Index: Bugzilla/Install/DB.pm >+ # Bug 494395 >+ if (!$dbh->bz_column_info('fielddefs', 'mandatory')) { >+ $dbh->bz_add_column('fielddefs', 'mandatory', >+ {TYPE => 'BOOLEAN', NOTNULL => 1, DEFAULT => 'FALSE'}); >+ } >+ >+ >+ Nit: No need to add so many newlines after this. >Index: template/en/default/admin/custom_fields/create.html.tmpl >+ <th align="right"><label for="mandatory">Mandatory field:</label></th> Let's call it "Mandatory (must be set)" But yeah, generally this looks like the right way to go.

I've tried patching this into 3.6rc1 and Failed.. Is there any estimation when will this enhancement introduced in Code Also , is it possible to make a field mandatory upon other fields status (i.e. if Dev RESOLVES a bug "Fixed in build" (custom field) becomes mandatory )

Hmm, the patch is missing a part, sorry for that, I'm going to re-attach the correct one.

Sorry for the spam :(.

Comment on attachment 441157 [details] [diff] [review] Small change in the patch. >=== modified file 'Bugzilla/Bug.pm' >+sub _check_field_mandatory { >+ my ($value, $field_name) = @_; This should be a method that's called on the class or object, not a function. >+ my $field = Bugzilla::Field->new({ name => $field_name }); You can use check() there instead. >+ $value = trim($value); >+ if (!defined($value) >+ or $value eq "" >+ or ($value eq '---' and $field->FIELD_TYPE_SINGLE_SELECT)) In a date field, "all 0's + punctuation" is also empty. >+ { >+ my $field_desc = $field ? $field->description : $field_name; >+ ThrowUserError('required_field', { field => $field_desc }); Just pass the field object to the error, and use field_descs in the template. >+ _check_field_mandatory($date_time, $field); Why not just put this into something like _set_global_validator and then into run_create_validators? Then it could be set on any field and we won't have to remember it in the future. Also, you could only run it on fields that were set as mandatory. In fact, this has to be done, for the case where people don't even pass in the variable name to create(). >=== modified file 'Bugzilla/DB/Schema.pm' >+ mandatory => {TYPE => 'BOOLEAN', NOTNULL => 1, >+ DEFAULT => 'FALSE'}, Let's call it is_mandatory. Since we're going to do a match() on it (as per my above comment), it will also need an index. >=== modified file 'Bugzilla/Install/DB.pm' >+ # 2010-04-21 - Tiago Mello <timello@linux.vnet.ibm.com> - Bug 494395 >+ if (!$dbh->bz_column_info('fielddefs', 'mandatory')) { You don't need the "if". Also, I think the purpose of this is obvious, so we don't need the comment above it. >=== modified file 'template/en/default/admin/custom_fields/edit.html.tmpl' >+ <th align="right"><label for="mandatory">Mandatory field:</label></th> >+ <td><input type="checkbox" id="mandatory" name="mandatory" value="1" >+ [%- " checked" IF field.mandatory %]></td> Since we're still HTML4, ' checked="checked"' is more correct. >=== modified file 'template/en/default/bug/edit.html.tmpl' >--- template/en/default/bug/edit.html.tmpl 2010-02-18 19:04:14 +0000 >+++ template/en/default/bug/edit.html.tmpl 2010-04-23 21:14:16 +0000 >@@ -141,6 +141,12 @@ > [% PROCESS section_title %] > <table class="edit_form"> > <tr> >+ <td colspan="3" align="right"> >+ (<span class="required_star">*</span> = >+ <span class="required_explanation">Required Field</span>) I don't want this to show up when editing a bug, actually. I don't think people even need the stars, when editing a bug. I think the visual indication that a field is mandatory is only necessary when filing a bug. >=== modified file 'template/en/default/bug/field.html.tmpl' >- <th class="field_label [% ' bz_hidden_field' IF hidden %]" >+ <th class="field_label [% ' bz_hidden_field' IF hidden %] [% ' required' IF field.mandatory %]" I'd also like to set aria-required on the fields, like we do now for mandatory fields on enter_bug.cgi. >=== modified file 'template/en/default/global/user-error.html.tmpl' >+ [% ELSIF error == "required_field" %] >+ [% title = "Field Needed" %] Field Must Be Set

Comment on attachment 441944 [details] [diff] [review] Fixes. >=== modified file 'Bugzilla/Bug.pm' >+sub check_required_create_fields { >+ my ($class, $params) = @_; >+ my $mandatory_fields = Bugzilla::Field->match({ is_mandatory => 1, >+ enter_bug => 1 }); Also, obsolete => 0. Also, don't use Field::Match, use Bugzilla->get_fields. >+ foreach my $field (@$mandatory_fields) { >+ $class->_check_field_is_mandatory($params->{$field->name}, >+ $field->name); This is very clever, but this loop should go at the end of run_create_validators instead, I think. That way, we know that the *validated* value isn't blank, right? >@@ -1677,6 +1688,26 @@ >+ if (ref($value) eq 'ARRAY' and $field->type == FIELD_TYPE_MULTI_SELECT) { Actually, I think you should just check if it's an ARRAY at all, no matter what type it is. >+ or ($value eq '0000-00-00 0:00' and $field->FIELD_TYPE_DATETIME)) That's not quite right. You should check it with this regex instead: ^[0\-:\sA-Za-z]+$ >@@ -1821,6 +1852,8 @@ > my $current = $self->$field; > my $privs; > >+ $self->_check_field_is_mandatory($value, $field); I think you should check this after _check_can_change_field. After all, if they can't change the field at all, no point in telling them that it needs to be set. >=== modified file 'Bugzilla/Field.pm' >+ is_mandatory => \&_check_is_mandatory, This should just be Bugzilla::Object::check_boolean. >=== modified file 'template/en/default/bug/field.html.tmpl' >- <th class="field_label [% ' bz_hidden_field' IF hidden %]" >+ <th class="field_label [% ' bz_hidden_field' IF hidden %] [% ' required' IF field.is_mandatory %]" Nit: I think this line is too long now. >@@ -74,11 +74,13 @@ > <input id="[% field.name FILTER html %]" class="text_input" > name="[% field.name FILTER html %]" > value="[% value FILTER html %]" size="40" >- maxlength="[% constants.MAX_FREETEXT_LENGTH FILTER none %]"> >+ maxlength="[% constants.MAX_FREETEXT_LENGTH FILTER none %]" >+ [%+ 'aria-required="true"' IF field.is_mandatory %]> Nit: Instead of the [%+, just do ' aria-required="true"' (add a space at the start). (Same for the other instances of aria-required. >@@ -121,6 +125,7 @@ > [% END %] > size="[% field_size FILTER html %]" multiple="multiple" > [% END %] >+ [% 'aria-required="true"' IF field.is_mandatory %] So, I haven't studied up on aria-required very much, but does it actually make sense for single-selects? Particularly because we're not providing any info to the screen reader (or other accessibility software) on the fact that "---" isn't a valid option. >@@ -177,7 +182,8 @@ > <a href="[% url FILTER html %]">[% url FILTER html %]</a> > [% IF editable %] > <label><input type="checkbox" value="[% url FILTER html %]" >- name="remove_[% field.name FILTER html %]"> >+ name="remove_[% field.name FILTER html %]" >+ [% 'aria-required="true"' IF field.is_mandatory %]> > Remove</label> aria-required doesn't go on the checkbox, it goes on the URL input. We can't have custom bug-url fields, so this doesn't matter too much right now, but it's also important to not highlight the checkbox as though it were required.

(In reply to comment #11) > (From update of attachment 441944 [details] [diff] [review]) > >=== modified file 'template/en/default/bug/field.html.tmpl' > >@@ -121,6 +125,7 @@ > > [% END %] > > size="[% field_size FILTER html %]" multiple="multiple" > > [% END %] > >+ [% 'aria-required="true"' IF field.is_mandatory %] > > So, I haven't studied up on aria-required very much, but does it actually > make sense for single-selects? Particularly because we're not providing any > info to the screen reader (or other accessibility software) on the fact that > "---" isn't a valid option. Well, I think we can show the aria-required when the field is mandatory and the current selected value is '---', or show it anyway. The documentation says it cab be used in combobox and list box (http://www.w3.org/TR/wai-aria/states_and_properties#aria-required).

(In reply to comment #12) I've decided to not show the aria-required for single-selects in the next patch.

Right patch.

Comment on attachment 442108 [details] [diff] [review] Small fixes. >=== modified file 'Bugzilla/Bug.pm' >+ foreach my $field (@mandatory_fields) { >+ $class->_check_field_is_mandatory($params->{$field->name}, >+ $field->name); Just pass $field, and in _check_field_is_mandatory, do a blessed($field) ? $field : Bugzilla::Field->check(). That way we don't have to instantiate the object twice. >+sub _check_field_is_mandatory { >+ or ($value =~ m/^[0\-:\sA-Za-z]+$/ and $field->FIELD_TYPE_DATETIME)) You know, we should probably put that regex into a constant somewhere. We could call it EMPTY_DATETIME. We don't have to do that in this patch, though, if you don't want to. But there might be other places where we're checking if a datetime is empty that could be unified into using the constant. >=== modified file 'template/en/default/admin/custom_fields/create.html.tmpl' Hmm, could you move up Reverse Relationship Description one row so that there isn't an odd gap in the page? >=== modified file 'template/en/default/admin/custom_fields/edit.html.tmpl' >+ <th align="right"><label for="is_mandatory">Is mandatory:</label></th> >+ <td><input type="checkbox" id="is_mandatory" name="is_mandatory" value="1" >+ [%- ' checked="checked"' IF field.is_mandatory %]></td> >+ </tr> >+ <tr> >+ <td>&nbsp;</td> I think that td needs a colspan="2".

https://landfill.bugzilla.org/timello/ has been updated.

i think that when creating a bug, mandatory fields should always be visible, even if advanced fields are hidden.

(In reply to comment #19) > i think that when creating a bug, mandatory fields should always be visible, > even if advanced fields are hidden. Oh yes, I agree with glob 100%.

(In reply to comment #20) > (In reply to comment #19) > > i think that when creating a bug, mandatory fields should always be visible, > > even if advanced fields are hidden. > > Oh yes, I agree with glob 100%. Alright, I'm going to update the patch.

Comment on attachment 444151 [details] [diff] [review] Some other fixes. Great! Looks great other than glob's comment.

Please, take a look in the field.html.tmpl template changes.

Comment on attachment 445141 [details] [diff] [review] Required fields should not be hidden. This looks OK, but it seems silly to be passing "required" in to field-label separately when it already gets a field object, and that field object already has is_mandatory. We could set the standard mandatory fields as is_mandatory = 1 in fielddefs, too, couldn't we? That would handle this pretty easily....

(In reply to comment #24) > (From update of attachment 445141 [details] [diff] [review]) > This looks OK, but it seems silly to be passing "required" in to field-label > separately when it already gets a field object, and that field object already > has is_mandatory. Definitely! > > We could set the standard mandatory fields as is_mandatory = 1 in fielddefs, > too, couldn't we? That would handle this pretty easily.... Alright!

(In reply to comment #24) > (From update of attachment 445141 [details] [diff] [review]) > We could set the standard mandatory fields as is_mandatory = 1 in fielddefs, > too, couldn't we? That would handle this pretty easily.... Is that what you mean? http://bugzilla.pastebin.mozilla.org/723803

(In reply to comment #26) > Is that what you mean? http://bugzilla.pastebin.mozilla.org/723803 Yes, except Install::DB is not the place to update the *data* that's in fielddefs--that should be done in Bugzilla::Field::populate_field_definitions. Also, don't remove the "required" class from the input for short_desc.

Max, I think it worths a try to handle that in this patch. Please, take a look. Thank you!

Comment on attachment 445486 [details] [diff] [review] Considering visibility field. >=== modified file 'Bugzilla/Bug.pm' >+ if ($field->visibility_field) { >+ my $visibility_field = $field->visibility_field->name; >+ my $visibility_value = $field->visibility_value->{value}; >+ my $selected_visibility_value = $self->$visibility_field >+ || $params->{$visibility_field}; >+ return if $selected_visibility_value ne $visibility_value; This looks good, except: 1) This should be in a Bugzilla::Field function called is_visible_on_bug. See is_visible_on_bug in Bugzilla::Field::ChoiceInterface. 2) You shouldn't be doing $self->visibility_field unless "blessed($invocant)" (and $self should be called $invocant, because we don't know if it's a class or an object). 3) You shouldn't be calling ->{value} on any object. I think you want ->name. >=== modified file 'Bugzilla/Field.pm' >+@Bugzilla::Field::EXPORT = qw( >+ check_field >+ get_field_id >+ get_legal_field_values >+ EMPTY_DATETIME_REGEX Instead of exporting this from Bugzilla::Field, it should be in Bugzilla::Constants.

And when you implement is_visible_on_bug, you'll want to use $value->is_set_on_bug($bug).

Comment on attachment 445486 [details] [diff] [review] Considering visibility field. >@@ -1019,6 +1049,7 @@ > $field->set_in_new_bugmail($def->{in_new_bugmail}); > $field->set_buglist($def->{buglist}); > $field->_set_type($def->{type}) if $def->{type}; >+ $field->set_is_mandatory(1) if $def->{is_mandatory}; Also, here in Bugzilla::Field, you should just pass in $def->{is_mandatory} without checking the if, like we do for in_new_bugmail.

Both methods are similar, Bugzilla:Field and Bugzilla::Field::ChoiceInterface, couldn't we unify them somehow?

Comment on attachment 445764 [details] [diff] [review] $field->is_visible_on_bug. >=== modified file 'Bugzilla/Field.pm' >+sub is_visible_on_bug { >+ my ($invocant, $bug) = @_; >+ >+ return if !blessed($invocant); That's not necessary. When would $invocant ever be not blessed? Also, it should be $self instead. $invocant = Could be either $class or $self. Otherwise, this looks OK.

Comment on attachment 445766 [details] [diff] [review] $self instead of $invocant. Looks great! :-) Thank you for doing so much work on this, timello!!! :-)

Comment on attachment 445766 [details] [diff] [review] $self instead of $invocant. >=== modified file 'template/en/default/global/textarea.html.tmpl' >--- template/en/default/global/textarea.html.tmpl 2009-10-23 21:32:06 +0000 >+++ template/en/default/global/textarea.html.tmpl 2010-05-17 14:25:32 +0000 >@@ -31,6 +31,8 @@ > # minrows will be used. > # cols: (required) Number of columns the textarea shall have. > # defaultcontent: (optional) Default content for the textarea. >+ # required: (optional) Boolean specifying whether or not the textarea >+ # is mandatory. > #%] > > <textarea [% IF name %]name="[% name FILTER html %]"[% END %] >@@ -47,4 +49,7 @@ > cols="[% cols FILTER html %]" > [% IF maxrows && user.settings.zoom_textareas.value == 'on' %] > onFocus="this.rows=[% maxrows FILTER html %]" >+ [% END %] >+ [% IF required %] >+ aria-required="true" > [% END %]>[% defaultcontent FILTER html %]</textarea> This looks wrong, especially considering you got rid of 'required' earlier in this patch...

Oh yeah, update the patch.

It should be 'mandatory' instead of 'required'.

Thanks to timello for all this work, and everybody else who found issues with the patch! :-) Committing to: bzr+ssh://bzr.mozilla.org/bugzilla/trunk/ modified editfields.cgi modified Bugzilla/Bug.pm modified Bugzilla/Constants.pm modified Bugzilla/Field.pm modified Bugzilla/DB/Schema.pm modified Bugzilla/Install/DB.pm modified template/en/default/admin/custom_fields/create.html.tmpl modified template/en/default/admin/custom_fields/edit.html.tmpl modified template/en/default/admin/custom_fields/list.html.tmpl modified template/en/default/bug/field-label.html.tmpl modified template/en/default/bug/field.html.tmpl modified template/en/default/bug/create/create.html.tmpl modified template/en/default/global/textarea.html.tmpl modified template/en/default/global/user-error.html.tmpl Committed revision 7181.

Added to the release notes in bug 604256.