1051655 - mentor field updated/reset when a bug is updated as a result of a change on a different bug (eg. see also, duplicate)

Status: RESOLVED FIXED

(bugzilla.mozilla.org :: General, defect)

Description

[Andrew Sutherland [:asuth] (he/him)]

I just filed bug 1051653, creating it with both an attachment and a "see also" to bug 906580 (which was RESOLVED FIXED).  The mentor got cleared from bug 906580 as it was reciprocally/magically updated.  I've manually corrected the bug but didn't see an existing open/fixed bug on this, so filing this.

Comment 1

[:glob ✱]

investigating

Comment 2

[David Lawrence [:dkl]]

Attachment: 1051655_1.patch

My proposed fix

Comment 3

[:glob ✱]

Comment on attachment 8471825 [details] [diff] [review]
1051655_1.patch

i'm not sure that tracking state with a global is the right approach here; i fear that it's fragile and may not work in all scenarios.

i'm exploring the viability of using the object_end_of_set_all hook instead.  this will likely need a new hook to add bug_mentor to the list of bug fields (in both post_bug.cgi and process_bug.cgi).

i'm not going to r- this patch yet in case my idea turns out to be silly :)

Comment 4

[:glob ✱]

this also happens when a bug with a mentor is duplicated to another bug .. the mentor is passed to the 'original' bug.

Comment 5

[:glob ✱]

Attachment: 1051655_1.patch

here's my approach:
- add hooks to post_bug.cgi and process_bug.cgi so we can inject bugs_mentors into the list of fields to read from input_params
- because bugs_mentors isn't a column on the bugs table, B:Bug->create needs two hooks: one to remove the field from the list of create/insert data, and another to update the database

Comment 6

[David Lawrence [:dkl]]

Comment on attachment 8473503 [details] [diff] [review]
1051655_1.patch

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

Also I can no longer set multiple new mentors at the same time. If I enter more than one, I get the error:

 There is no user named '[e-mail], [e-mail],'. Either you mis-typed the name or that user has not yet registered for a Bugzilla account. 

one mentor value works fine. Maybe you need a _check_bug_mentors that splits the value on whitespace?

dkl

::: Bugzilla/Bug.pm
@@ +717,5 @@
> +    # BMO - support for fields which are not bug columns (eg bug_mentors)
> +    # extensions should move fields from $params to $stash, then use the
> +    # bug_create_stash hook to update the database
> +    my $stash = {};
> +    Bugzilla::Hook::process('bug_create_params', { params => $params, stash => $stash });

It's funny. See this jarred my memory and I did something just like this at Red Hat to fix a similar issue. I would like to see it this way if possible

   my $deleted_params = {};
   Bugzilla::Hook::process('bug_before_create',
       { params => $params, deleted_params => $deleted_params });

and have it present earlier on in Bugzilla::Bug::create so it could also be used for other things if needed. I also think this could be pushed upstream as it is a common need for extension authors.

@@ +801,5 @@
>      }
>  
> +    # BMO - support for fields which are not bug columns (eg bug_mentors)
> +    Bugzilla::Hook::process('bug_create_stash', { bug => $bug, stash => $stash });
> +

Instead of adding this new hook just use bug_end_of_create and pass in the deleted_params hash.

    Bugzilla::Hook::process('bug_end_of_create',
        { bug => $bug, timestamp => $timestamp, deleted_params => $deleted_params });

also an upstreamable(word?) change with the other hook IMO.

Comment 8

[:glob ✱]

status update - this is my highest priority issue.

this is panning out to be more complicated that i expected.  we can't call user::match_fields from the setter or checker because that would be triggered by api driven updates.  i'm working on a slightly different approach that should work.

Comment 9

[:glob ✱]

Attachment: 1051655_2.patch

this patch:
- adds getter, setter, and checker for bug_mentor field to bug object to allow
  bugzilla::object methods do most of the heavy lifting, and ensure the tables
  are updated with the correct values
- adds a bug_user_match_fields hook, allowing extensions to add their own
  user fields that are resolved to user matching by post_bug and process_bug
- adds stash to bug_before_create and bug_end_of_create to allow extensions
  to add fields to bug which are not columns

Comment 10

[David Lawrence [:dkl]]

Comment on attachment 8475952 [details] [diff] [review]
1051655_2.patch

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

Otherwise looks good and works well once I make the below change. Fix on commit. r=dkl

::: extensions/Review/Extension.pm
@@ +163,5 @@
> +    if (my $mentors = $stash->{bug_mentors}) {
> +        $self->_update_user_table({
> +            object      => $bug,
> +            old_users   => [],
> +            new_users   => $mentors,

$mentors needs to be a list of user objects and not a list of logins it seems as i get an error when creating a new bug with mentor values

$mentors = [ map { Bugzilla::User->check({ name => $_, cache => 1 }) }
             ref($mentors) ? @$mentors : ($mentors) ];

fixes the issue for me for both single and multiple mentors

Comment 11

[:glob ✱]

To ssh://[e-mail]/webtools/bmo/bugzilla.git
   83c1212..d5c1d67  master -> master

Comment 12

[:glob ✱]

this broke the ability for users without editbugs to comment on bugs:

To ssh://[e-mail]/webtools/bmo/bugzilla.git
   fe5deaa..726d3c1  master -> master