287332 - Ability to add custom "Bugzilla user" fields to a bug

Status: NEW

(Bugzilla :: Bugzilla-General, enhancement)

Description

[Max Kanat-Alexander]

This type of custom field is a field that contains a user_id, and displays the
login-name/email of that user.

For example, QA Contact is this type of field.

Comment 1

[Max Kanat-Alexander]

I don't think that this bug should also include the ability to set defaults for
these fields per-component, like we have for QA Contact currently. That can be
dealt with in another bug, perhaps.

Comment 2

[Max Kanat-Alexander]

*** Bug 335589 has been marked as a duplicate of this bug. ***

Comment 6

[Jeff]

Once these fields are possible we will also likely want to limit which users should be able to assigned to these fields. See bug#465844 and bug#323870 for more information.

Comment 7

[miketosh]

If we can target this for the next release, which it sounds like will be 4.2 (what happened to 4.0???) I am MORE than willing to complete this and submit a patch.

Comment 8

[Max Kanat-Alexander]

Nothing happened to 4.0, it's branched and we're working on fixing the blockers.

Comment 9

[miketosh]

Attachment: Patch v1 for BugzillaTrunk (4.1)

This patch isn't complete, but it works.

1) Add custom field of type USER - DONE
2) Allow field to only pick users from a designated group - PARTIAL
3) Display field in the create (when applicable) and edit pages for bugs - DONE
4) Ensure proper function for User Selection and User Matching - PARTIAL
4) Search for custom user fields - NOT STARTED
5) Allow email to be sent to assignees of the field - NOT STARTED

If anyone on the CC for this issue can check this out and provide back comments, that would be greatly appreciated.

Comment 10

[miketosh]

Oh, and I need to limit the users from the control group returned or displayed to only active users.  Having disabled users being able to be assigned makes no sense.

Comment 11

[Max Kanat-Alexander]

Comment on attachment 481031 [details] [diff] [review]
Patch v1 for BugzillaTrunk (4.1)

It's really cool that you're working on this and that you've got so much of it done! Actually, most of it looks really great.

The "groups" part of this patch should be an entirely separate patch on a separate bug, though.

Comment 12

[miketosh]

(In reply to comment #11)
> The "groups" part of this patch should be an entirely separate patch on a
> separate bug, though.

Would Bug 465844 be an the appropriate place to do that?

Comment 13

[Max Kanat-Alexander]

(In reply to comment #12)
> Would Bug 465844 be an the appropriate place to do that?

  Oh, good question. I think that you should probably file a new bug that blocks that one, since your bug will fix it, but will also do other things.

Comment 14

[miketosh]

(In reply to comment #13)
>   Oh, good question. I think that you should probably file a new bug that
> blocks that one, since your bug will fix it, but will also do other things.

Funny, I just checked the blocked issues, and I had opened one (bug 393450) for just that purpose back in 2007.  Can you please assign me to that one also?  I'll split the patch into 3 parts and check them into here, bug 393450, and bug 602889 for emailing roles.

Comment 15

[miketosh]

Attachment: Code Patch Ver 2

I removed all group-control parts of this and ignored the "email custom user roles".

Comment 16

[Frédéric Buclin]

Comment on attachment 565287 [details] [diff] [review]
Code Patch Ver 2

>=== modified file 'Bugzilla/Bug.pm'

>+sub _check_user_field {
>+  my ($invocant, $value, $field) = @_;
>+  my $object = Bugzilla::User->check($value);
>+  return $object->id;
>+}

If it's a user field, then it must follow the same rules as any other hardcoded user field. You must check strict isolation too, if enabled.


>+sub _single_user_accessor {
>+    my ($class, $field) = @_;
>+    my $accessor = sub {
>+        my ($self) = @_;
>+        $self->{$field . "obj"} ||= Bugzilla::User->new($self->{$field});

Use $field . "_obj" to match other foo_obj field names.


That's only a quick review. mkanat will probably have more to say.

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.

Comment 18

[miketosh]

Too bad I had done a bit of work on this, hopefully someone else has the time to take what I've done.  Didn't look like it would be much more to do for a complete patch.

Comment 19

[Fred Kleindenst]

Attachment: Code Patch Ver 3 for bug 287332

I've taken the work done on this to date in this thread, and applied it (and added the necessary fixes) to 4.4.

Attached please find a diff  "Code patch ver 3"

Comment 20

[Fred Kleindenst]

Attachment: Diff between trunk branch (4.5) 287332.diff

Correcting diff.   This diff comes from the main development branch (trunk)

Comment 21

[Fred Kleindenst]

To test/use this feature, you must first create a custom field of type "single_user".

This field will show up in bug creation / bug editing.  This field allows the user to enter/select an existing user of bugzilla to fill into this field.

Comment 22

[Fred Kleindenst]

Comment on attachment 778045 [details] [diff] [review]
Diff between trunk branch   (4.5)   287332.diff

Review of attachment 778045 [details] [diff] [review]:
-----------------------------------------------------------------

Please review

Comment 23

[Thomas Kendall]

I'm looking to add this patch to my 4.4.1 install but could use some clarification on how to do this.  Should I directly edit the files listed in the diff between 4.5 and this patch or can I run the diff from patch -1 < 287332.diff?

Comment 24

[Thomas Kendall]

After patching this to 4.4.1 I'm getting this error when i try to add a new custom field of type Single User. To me it looks like something is missing in the Constant.pm file or something.

URL: http://bugtracker.mycompanyname.com/editfields.cgi?name=cf_busnuser&enter_bug=1&desc=BusinessUser&type=10&sortkey=201&long_desc=&visibility_field_id=&action=new&token=Lx4oyHApGd

The type 10 is not a valid field type.
Traceback:

 at Bugzilla/Field.pm line 354
	Bugzilla::Field::_check_type(...) called at Bugzilla/Object.pm line 517
	Bugzilla::Object::run_create_validators(...) called at Bugzilla/Field.pm line 1074
	Bugzilla::Field::create(...) called at /var/www/html/bugzilla/editfields.cgi line 49

Comment 25

[David Lawrence [:dkl]]

Comment on attachment 778045 [details] [diff] [review]
Diff between trunk branch   (4.5)   287332.diff

Review of attachment 778045 [details] [diff] [review]:
-----------------------------------------------------------------

Sorry for the delay in looking at this. 

Small amount of bitrot:

patching file Bugzilla/Bug.pm
Hunk #2 succeeded at 2113 (offset -14 lines).
Hunk #3 succeeded at 4348 (offset 28 lines).
Hunk #4 FAILED at 4347.
1 out of 4 hunks FAILED -- saving rejects to file Bugzilla/Bug.pm.rej

Also in process_bug.cgi and post_bug.cgi you need to be able to match on the field values so that partial strings can be used. For example in process_bug.cgi you would do:

# do a match on the fields if applicable
my $user_match_fields = {
    'qa_contact'                => { 'type' => 'single' },
    'newcc'                     => { 'type' => 'multi'  },
    'masscc'                    => { 'type' => 'multi'  },
    'assigned_to'               => { 'type' => 'single' },
};
foreach my $field (Bugzilla->active_custom_fields) {
    if ($field->type eq FIELD_TYPE_SINGLE_USER) {
        $user_match_fields->{$field->name} = { 'type' => 'single' };
    }
}
Bugzilla::User::match_field($user_match_fields);

Bugzilla/Bug.pm needs to be updated in update() to properly log the login names of the users when making a change as well. Currently it is logging the user ids in the bugs_activity table.
See line 956+ for example code on how to deal with custom fields and enter values in the %$changes hash for later inserting into the bugs_activity table.

Will review some more once these are resolved.

dkl

::: Bugzilla/Field.pm
@@ +567,5 @@
> +
> +sub is_user {
> +    my ($invocant, $params) = @_;
> +    # This allows this method to be called by create() validators.
> +    my $type = blessed($invocant) ? $invocant->type : $params->{type}; 

remove trailing whitespace

::: Bugzilla/WebService/Bug.pm
@@ +965,5 @@
>              my @values = map { $self->type('string', $_) } @{ $bug->$name };
>              $item{$name} = \@values;
>          }
> +        elsif ($field->type == FIELD_TYPE_SINGLE_USER) {
> +            $item{$name} = $self->type('string', $bug->$name->login);

$item{$name} = $self->type('email', $bug->$name->login);

::: template/en/default/bug/field.html.tmpl
@@ +212,5 @@
> +      [% CASE constants.FIELD_TYPE_SINGLE_USER %]
> +        [% INCLUDE global/userselect.html.tmpl
> +             id => field.name
> +             name => field.name
> +             value => value.name

nit-pick: line up the =>

Also add:

classes => ["bz_userfield"]
size    => 30

Also when viewing the bug after adding a value, it is displaying the user's real name in the text field instead of the login name. So change

value => value.name

to 

value => value.login

Ideally it would be nice if this field acted like assignee and qa contact in that it displayed the readonly value of the user's identity (real name <email>) and have a (edit) link next to it. Clicking edit would display the text field.

Comment 26

[David Lawrence [:dkl]]

Comment on attachment 778045 [details] [diff] [review]
Diff between trunk branch   (4.5)   287332.diff

Sorry meant for this to be r- not r+ :)

Comment 27

[Fred Kleindenst]

Attachment: patch with the latest fixes for this bug

this a patch diffed from the head of the git repo (hope this is correct).

I've addressed each of the points made in the last comment. Thanks, for those BTW.  

I'm happy to address any other issues on this.

Comment 29

[David Lawrence [:dkl]]

Comment on attachment 8390752 [details] [diff] [review]
patch with the latest fixes for this bug

Review of attachment 8390752 [details] [diff] [review]:
-----------------------------------------------------------------

t/008filter.t ........ 49/285 
#   Failed test '(en/default) template/en/default/bug/field.html.tmpl has unfiltered directives:
#   216: field.name 
#   219: field.name 
#   222: field.name 
#   233: field.name 
#   234: field.name 
#   235: field.name 
#   236: field.name 
# --ERROR'
#   at t/008filter.t line 116.

Some need FILTER html and some need FILTER js.

Another thing that needs to be fixed is that you need to make some changes to Bugzilla/Search.pm to allow for searching for bugs with the login
as the value of the new custom field. Right now it only works if you use the user's ID as the value and that is not valid.

You can look at other user type fields such as assigned_to, reporter, etc. and use those as examples to figure out what changes are needed.
Also you should update pronoun to allow for doing %<cf_user>% type values when doing advanced searches.

Otherwise the rest is looking good.
dkl

::: Bugzilla/Bug.pm
@@ +956,4 @@
>                           $user->id, $delta_ts, $comment->id);
>      }
>  
> +    # custom fields of type : FIELD_TYPE_SINGLE_USER 

nit: trailing whitespace

::: Bugzilla/WebService/Bug.pm
@@ +1290,5 @@
>              $item{$name} = \@values;
>          }
> +        elsif ($field->type == FIELD_TYPE_SINGLE_USER) {
> +            $item{$name} = $self->type('email', $bug->$name->login);
> +        }

With more thought on this, also include the _detail portion as well.

        $item{$name} = $self->type('email', $bug->$name->login);
        $item{"${name}_detail"} = $self->_user_to_hash($bug->$name, $params, undef, $name);

::: template/en/default/bug/field.html.tmpl
@@ +231,5 @@
> +          <script type="text/javascript">
> +           hideEditableField('bz_[% field.name %]_edit_container', 
> +                             'bz_[% field.name %]_input', 
> +                             'bz_[% field.name %]_edit_action', 
> +                             '[% field.name %]', 

Remove all trailing whitespace

@@ +238,3 @@
>    [% END %]
>  [% ELSE %]
>    [% SWITCH field.type %]

You also need to add a section to this ELSE block when the field is not editable such as the user is logged out or does not have edit rights.

Example:

    [% CASE constants.FIELD_TYPE_SINGLE_USER %]
      [% INCLUDE global/user.html.tmpl who = value %]
    [% CASE %]
      [% value.join(', ') FILTER html %]
  [% END %]

Comment 30

[Fred Kleindenst]

Attachment: gitdiff.287332.diff

this is a git diff between my changes and the latest "master". 
I'm not sure of the appropriate workflow now that we are using git repos.
Please let me know if this is incorrect.

Comment 31

[David Lawrence [:dkl]]

Comment on attachment 8434411 [details] [diff] [review]
gitdiff.287332.diff

Hmm somehow you uploaded just a diff of the changes that were made to your changes from the last review. We will need a full patch instead to do the review against. 

A quick way would be to checkout a new clone of the master git repo, apply your last patch that failed review, make your fixes, and then do a new full diff with a bump in the rev number.

dkl

Comment 32

[Fred Kleindenst]

Attachment: new287332.diff Latest round of fixes

I think this diff is correct.  I did a git diff master..<my_branch_changes>

Please let me know if my diff is accurate so I can fix my process if needed.

Comment 33

[David Lawrence [:dkl]]

Comment on attachment 8434493 [details] [diff] [review]
new287332.diff    Latest round of fixes

Almost but not quite. Looks like the diff is reversed and your changes are being removed rather than added. Try reversing the git diff command.

Comment 34

[Fred Kleindenst]

Attachment: new287332reversed.diff Reversed the last diff

Third time is the charm.  Uploading latest code changes.

Comment 35

[David Lawrence [:dkl]]

Comment on attachment 8434497 [details] [diff] [review]
new287332reversed.diff  Reversed the last diff

Review of attachment 8434497 [details] [diff] [review]:
-----------------------------------------------------------------

Found another issue I missed before when viewing the XML form of a bug.

http://bzweb/287332/show_bug.cgi?id=35&ctype=xml:

<cf_user_test>Bugzilla::User=HASH(0x42f6998)</cf_user_test>

Not sure what the proper solution here would be as we only pass the field names into show.xml.tmpl and not a list of field objects. So not easy to
see if a field is a single user type.

::: Bugzilla/Bug.pm
@@ +4457,5 @@
> +sub _single_user_accessor {
> +    my ($class, $field) = @_;
> +    my $accessor = sub {
> +        my ($self) = @_;
> +        $self->{$field . "_obj"} ||= Bugzilla::User->new($self->{$field});

$self->{"${field}_obj"} ||= new Bugzilla::User({ id => $self->{$field}, cache => 1 });

::: template/en/default/bug/field.html.tmpl
@@ +232,5 @@
> +          <script type="text/javascript">
> +           hideEditableField('bz_[% field.name FILTER js %]_edit_container', 
> +                             'bz_[% field.name FILTER js %]_input', 
> +                             'bz_[% field.name FILTER js %]_edit_action', 
> +                             '[% field.name FILTER js %]', 

trailing whitespace

@@ +259,5 @@
>            </li>
>          [% END %]
>        [% '</ul>' IF value.size %]
> +    [% CASE constants.FIELD_TYPE_SINGLE_USER %]
> +      [% INCLUDE global/user.html.tmpl who = value.login %]

When not logged in, the value does not display if one is set. Needs to be: 

[% INCLUDE global/user.html.tmpl who = value %]

Comment 36

[Fred Kleindenst]

Attachment: new287332-vers4.diff

New version with fixes.

Also, a proposed solution to the XML problem.  Added a hash of field types passed to show.xml.tmpl.   Might be a useful addition in the template for other use cases.

Comment 37

[Fred Kleindenst]

Ping on this.  Did I set all the required review settings on this bug?  I'm looking for DKL to review/approve.

Comment 38

[Simon Green]

(In reply to Fred Kleindenst from comment #37)
> Ping on this.  Did I set all the required review settings on this bug?  I'm
> looking for DKL to review/approve.

You set all the of flags correctly. dkl is a little busy with his day job, so it might be a while before you get a response.

  -- simon

Comment 39

[David Lawrence [:dkl]]

Comment on attachment 8435147 [details] [diff] [review]
new287332-vers4.diff

Review of attachment 8435147 [details] [diff] [review]:
-----------------------------------------------------------------

show.xml.tmpl is still generating:

<cf_user_test>Bugzilla::User=HASH(0x4a04148)</cf_user_test>

I do not see where you passed in the field objects. Is this a new patch? It looks like the previous one.

::: template/en/default/bug/field.html.tmpl
@@ +232,5 @@
> +          <script type="text/javascript">
> +           hideEditableField('bz_[% field.name FILTER js %]_edit_container', 
> +                             'bz_[% field.name FILTER js %]_input', 
> +                             'bz_[% field.name FILTER js %]_edit_action', 
> +                             '[% field.name FILTER js %]', 

remove trailing whitespace

@@ +259,5 @@
>            </li>
>          [% END %]
>        [% '</ul>' IF value.size %]
> +    [% CASE constants.FIELD_TYPE_SINGLE_USER %]
> +      [% INCLUDE global/user.html.tmpl who = value.login %]

who = value

Comment 41

[Albert Ting]

FYI, I ended up modifying this patch to make it work in 4.0.  Two things that came up:

1. Need to handle edit-multiple, otherwise it resets all custom user fields to blank.  Just a one-line change:

bug/field.html.tmpl
 +            [% INCLUDE global/userselect.html.tmpl
 +              id      => field.name
 +              name    => field.name
-+              value   => value.login
++              value   => (value == dontchange ? value : value.login)
 +              classes => ["bz_userfield"]
 +              size    => 30
 +            %]

2. Need to add all user fields to the advanced search form.  

--- query.cgi~	2014-09-05 19:42:30.668253199 -0500
+++ query.cgi	2014-09-05 19:49:27.724667457 -0500
@@ -262,6 +262,11 @@
     }
 }
 
+# grab custom user fields
+my @custom_user_fields =
+    grep { $_->type == FIELD_TYPE_SINGLE_USER} Bugzilla->active_custom_fields;
+$vars->{'custom_user_fields'} = \@custom_user_fields;
+
 @fields = sort {lc($a->description) cmp lc($b->description)} @fields;
 unshift(@fields, { name => "noop", description => "---" });
 $vars->{'fields'} = \@fields;
--- template/en/default/search/form.html.tmpl~	2014-09-05 19:42:29.853499712 -0500
+++ template/en/default/search/form.html.tmpl	2014-09-05 19:53:17.191754906 -0500
@@ -273,6 +273,15 @@
                   label=> "a ${field_descs.cc} list member" } %]
       [% PROCESS role_types field = { count => n, name => "emaillongdesc", 
                   label=> " a ${field_descs.commenter}" } %]
+
+      [% IF custom_user_fields.size > 0 %]
+        [% FOREACH field = custom_user_fields %]
+          [% PROCESS role_types field = { count => n,
+                                          name => "email" _ field.name,
+                                          label=> " the ${field.description}" } %]
+        [% END %]
+      [% END %]
+
       <select name="emailtype[% n %]">
       [% FOREACH qv = [
         { name => "substring", description => "contains" },

Comment 42

[Albert Ting]

Attachment: bug-287332-custom-user

For what it's worth, I've updated this patch to work with 5.0 and addressed the issues noted in comment #39.  Also added custom user fields in the advanced search and change-column code. 

Fred, feel free to take over.  Just want to see this code checked in.

Comment 43

[Frédéric Buclin]

Comment on attachment 8543817 [details] [diff] [review]
bug-287332-custom-user

I will review it. The patch still applies cleanly, except a minor conflict in bug/field.html.tmpl.

Comment 44

[Frédéric Buclin]

Comment on attachment 8543817 [details] [diff] [review]
bug-287332-custom-user

>+++ Bugzilla/Field.pm

>+    FIELD_TYPE_SINGLE_USER,   { TYPE => 'INT3'       },

A foreign key pointing to the profiles table is required to correctly delete user accounts. Else you get:

  Can't call method "login" on an undefined value at Bugzilla/Bug.pm line 1110.

If a user account is used in a custom user field, this information must also be reported in editusers.cgi when deleting the account.


Also, the user field is too large when filing a new bug, due to the bz_userfield CSS class and its "width: 100%" rule.


Otherwise, it seems to work correctly. I didn't look at the code yet.