[rt-devel] Proposed patch: Friendlier 'mandatory' custom fields

[Thomas Sibley]

On 12/24/2012 02:34 PM, David Good wrote:
> We have a customer that some time ago wanted to have mandatory custom
> fields be a little friendlier.  I've been maintaining this customization
> over several RT versions and I think it may be of enough general use
> that I though I'd submit it here.  Hopefully it's found to be useful
> enough that I won't have to port it to a new RT release again :-)

There's been a little work on the CF validation hints in 4.2 (on the git
master branch), although the mandatory wording is still the same.

I've quickly reviewed your code below, pointing out a number of issues
that would need resolving for this to be reconsidered.  The biggest
blocker is the general approach to detecting mandatory CFs; a core RT
solution should add some sort of field metadata rather than rely on
string matching the Pattern.

All of that said, a better "This field is mandatory" message would be
great.  "Input must match [Mandatory]" is pretty oblique!  :)

> What is does:
> 
>  - When a mandatory custom field has no value when a ticket edit is
>    submitted, rather than the usual 'Input does not match: [Mandatory]'
>    message, it will instead give the error message 'You must select a
>    value'.

"Select" isn't necessarily the right verb; the CF may be an "Enter one
value" type.

>  - When editing a ticket, mandatory custom fields will have
>    '[Mandatory]' appear under the field hint (i.e. 'Select one value').
>    The '[Mandatory]' text uses the 'cfinvalidfield' CSS class, so by
>    default it will be red and italic.

Part of the 4.2 work I mention above is making the CF hints visible more
often on initial load.  I think you'll find that due to those changes,
special casing mandatory patterns here is unnecessary.

> I've made only very limited attempts to use localization, mainly due to
> my own unfamiliarity with it.  I'd be happy to take suggestions on how
> to improve it.

There are some basic localization (l10n) mistakes, most pointed out
below.  You may be interested in this article about l10n:
https://metacpan.org/module/TODDR/Locale-Maketext-1.23/lib/Locale/Maketext/TPJ13.pod

> The patch was created from RT 4.0.8:

Any change like this for core would need to be based on 4.2.

> 
> --- lib/RT/CustomField.pm	2012-12-19 13:13:09.025953917 -0800
> +++ local/lib/RT/CustomField.pm	2012-12-24 12:23:32.814404980 -0800
> @@ -1464,7 +1464,15 @@
>      }
>  
>      unless ( $self->MatchPattern($args{'Content'}) ) {
> -        return ( 0, $self->loc('Input must match [_1]', $self->FriendlyPattern) );
> +        my $msg = $self->Name . ": ";
> +        $RT::Logger->debug("FriendlyPattern is: '" . $self->FriendlyPattern . "'");

Unnecessary debug line.

> +        if ($self->FriendlyPattern eq '[Mandatory]'){

This eq will fail if "Mandatory" is translated and the current user's
language is not english.  The FriendlyPattern method localizes the
string before returning it.  You always have to compare strings before
localization.

A larger issue, however, is that there should be a better, more reliable
way to indicate mandatory-ness than string eq on the Pattern comment.

> +            $msg .= $self->loc("You must select a value");
> +        }
> +        else {
> +            $msg .= $self->loc('Input must match [_1]', $self->FriendlyPattern);
> +        }
> +        return ( 0, $msg );
>      }
>  
>      $RT::Handle->BeginTransaction;
> @@ -1625,7 +1633,15 @@
>  
>      # for single-value fields, we need to validate that empty string is a valid value for it
>      if ( $self->SingleValue and not $self->MatchPattern( '' ) ) {
> -        return ( 0, $self->loc('Input must match [_1]', $self->FriendlyPattern) );
> +        my $msg = $self->Name . ": ";
> +        $RT::Logger->debug("FriendlyPattern is: '" . $self->FriendlyPattern . "'");
> +        if ($self->FriendlyPattern eq '[Mandatory]'){

Ditto to all the above.

> +            $msg .= $self->loc("You must select a value");
> +        }
> +        else {
> +            $msg .= $self->loc('Input must match [_1]', $self->FriendlyPattern);
> +        }
> +        return ( 0, $msg );
>      }
>  
>      # delete it
> --- lib/RT/Interface/Web.pm	2012-12-19 13:13:08.697946357 -0800
> +++ local/lib/RT/Interface/Web.pm	2012-12-21 15:37:41.074189388 -0800
> @@ -2430,6 +2430,8 @@
>          Queue
>      );
>  
> +    my @results = ();
> +
>      # Canonicalize Queue and Owner to their IDs if they aren't numeric
>      for my $field (qw(Queue Owner)) {
>          if ( $ARGSRef->{$field} and ( $ARGSRef->{$field} !~ /^(\d+)$/ ) ) {
> @@ -2446,7 +2448,56 @@
>      # RT core complains if you try
>      delete $ARGSRef->{'Status'} unless $ARGSRef->{'Status'};
>  
> -    my @results = UpdateRecordObject(
> +    #
> +    # Check custom fields to make sure any mandatory values are filled
> +    #
> +    #
> +    # This bit was swiped from ProcessObjectCustomFieldUpdates, below...

Swiping large swaths of code is generally frowned upon, as it leads to
duplication and the code diverges from each other.  There's been a lot
of refactoring work in this area for 4.2 on the master branch, as well.

> +    #
> +    my %custom_fields;
> +    ARG:
> +    foreach my $arg ( keys %$ARGSRef ) {
> +        # format: Object-<object class>-<object id>-CustomField-<CF id>-<commands>
> +        next unless $arg =~ /^Object-([\w:]+)-(\d*)-CustomField-(\d+)-(.*)$/;
> +    
> +        # For each of those objects, find out what custom fields we want to work with.
> +        $custom_fields{ $1 }{ $2 || 0 }{ $3 }{ $4 } = $ARGSRef->{ $arg };
> +    }   
> +    foreach my $class ( keys %custom_fields ) {
> +        foreach my $id ( keys %{$custom_fields{$class}} ) {
> +            my $Object = $args{'Object'};
> +            $Object = $class->new( $session{'CurrentUser'} )
> +                unless $Object && ref $Object eq $class;
> +
> +            $Object->Load( $id ) unless ($Object->id || 0) == $id;
> +            unless ( $Object->id ) {
> +                $RT::Logger->warning("Couldn't load object $class #$id");
> +                next;
> +            }
> +            foreach my $cf ( keys %{ $custom_fields{ $class }{ $id } } ) {
> +                my $CustomFieldObj = RT::CustomField->new( $session{'CurrentUser'} );
> +                $CustomFieldObj->LoadById( $cf );
> +                unless ( $CustomFieldObj->id ) {
> +                    $RT::Logger->warning("Couldn't load custom field #$cf");
> +                    next;
> +                }
> +                my $value = "";
> +                $value = $custom_fields{$class}{$id}{$cf}{Values}
> +                    if exists $custom_fields{$class}{$id}{$cf}{Values};
> +
> +		if (not $value and $CustomFieldObj->Pattern eq '(?#Mandatory).'){

Matching on the full Pattern itself is as undesirable as matching on
just the comment is, for the same reason as noted above.

> +		    my $msg = sprintf "%s: You must select a value",
> +			$CustomFieldObj->Name;
> +		    push(@results, $msg);
> +		}
> +	    }
> +	}
> +    }
> +    if (@results){ 
> +        return(@results);
> +    }       

Perhaps more importantly for this bit, why did you add CF checking at
all to ProcessTicketBasics()?  Why is ProcessObjectCustomFieldUpdates()
not enough?

> +    @results = UpdateRecordObject(
>          AttributesRef => \@attribs,
>          Object        => $TicketObj,
>          ARGSRef       => $ARGSRef,
> @@ -2616,6 +2667,7 @@
>      }
>  
>      my @results;
> +    ARG:
>      foreach my $arg ( keys %{ $args{'ARGS'} } ) {
>  
>          # skip category argument
> @@ -2627,7 +2679,10 @@
>  
>              # We don't care about the magic, if there's really a values element;
>              next if defined $args{'ARGS'}->{'Value'}  && length $args{'ARGS'}->{'Value'};
> -            next if defined $args{'ARGS'}->{'Values'} && length $args{'ARGS'}->{'Values'};
> +            #next if defined $args{'ARGS'}->{'Values'} && length $args{'ARGS'}->{'Values'};
> +            # Sometimes we do get null values, though (like when selecting 
> +            # '(no value)')
> +            next if defined $args{'ARGS'}->{'Values'};

There's been a lot of refactoring here in 4.2, you'd should take a look
if you're planning on reworking the patch for resubmission.

>  
>              # "Empty" values does not mean anything for Image and Binary fields
>              next if $cf_type =~ /^(?:Image|Binary)$/;
> @@ -2684,6 +2739,15 @@
>          } elsif ( $arg eq 'Values' && !$cf->Repeated ) {
>              my $cf_values = $args{'Object'}->CustomFieldValues( $cf->id );
>  
> +            if (not @values){
> +                if ($cf->Pattern eq '(?#Mandatory).'){
> +                    # This error should already have been generated by 
> +                    # ProcessTicketBasics...
> +                    #my $msg = sprintf "%s: You must select a value", $cf->Name;
> +                    #push(@results, $msg);
> +                    next ARG;
> +                }
> +            }

Leftover cruft code should be removed.

>              my %values_hash;
>              foreach my $value (@values) {
>                  if ( my $entry = $cf_values->HasEntry($value) ) {
> --- share/html/Elements/ValidateCustomFields	2012-12-19 13:13:12.632978972 -0800
> +++ local/html/Elements/ValidateCustomFields	2012-12-24 13:44:35.158188898 -0800
> @@ -109,7 +109,14 @@
>  
>          next if $CF->MatchPattern($value);
>  
> -        my $msg = loc("Input must match [_1]", $CF->FriendlyPattern);
> +	my $msg = '';
> +	if ($CF->FriendlyPattern eq '[Mandatory]'){

Similar eq and l10n failures.

> +	    $msg = "You must select a value";
> +	}
> +	else {
> +	    $msg = loc("Input must match [_1]", $CF->FriendlyPattern);
> +	}
> +
>          $m->notes( ('InvalidField-' . $CF->Id) => $msg );
>          push @res, $msg;
>          $valid = 0;
> --- share/html/Ticket/Elements/EditCustomFields	2012-12-24 12:27:43.941266648 -0800
> +++ local/html/Ticket/Elements/EditCustomFields	2012-12-24 13:15:02.418187709 -0800
> @@ -56,6 +56,10 @@
>      <<% $CELL %> class="cflabel">
>        <span class="name"><% loc($CustomField->Name) %></span><br />
>        <span class="type"><% $CustomField->FriendlyType %></span>
> +%  if ($CustomField->FriendlyPattern eq '[Mandatory]'){

Ditto.

> +        <br />
> +        <span class="cfinvalidfield"><% loc('[Mandatory]') %></span>

Localizing the [] should probably be avoided, or if not, at least
escaped with ~ (the escape character for [ and ] and other special chars
in loc-strings).

I think this whole section though is probably unnecessary with the new
CF hints in 4.2.

> +%  }
>      </<% $CELL %>>
>      <<% $CELL %> class="entry">
>  % my $default = $m->notes('Field-' . $CustomField->Id);
> @@ -67,7 +71,9 @@
>            NamePrefix => $NamePrefix,
>            Default => $default,
>        &>
> -%  if (my $msg = $m->notes('InvalidField-' . $CustomField->Id)) {
> +%  my $msg = $m->notes('InvalidField-' . $CustomField->Id);
> +%  # ignore "Mandatory" notes since they're included above...
> +%  if ($msg and $msg !~ /\[Mandatory\]$/ and $msg !~ /You must select a value/){

This type of matching and filtering fails prey to the same "comparing
against a localized value" problem, and is also generally brittle.  For
an RT core solution, there should be added metadata, rather than string
matching heuristics.

>          <br />
>          <span class="cfinvalidfield"><% $msg %></span>
>  %  }
>