622679 - Autocomplete suggests inactive/disabled accounts as matches

Status: RESOLVED FIXED

(Bugzilla :: WebService, defect)

Description

[Daniel Veditz [:dveditz]]

The in-page autocomplete widget finds and suggests matches from inative/disabled accounts as well. If you ignore the autocomplete dropdown and submit the form with a partial name it will match only against active accounts.

Steps to reproduce:
1) at bugzilla.mozilla.org click the "(edit)" link next to a field that contains user names (e.g. CC list)
2) type "dveditz", wait for matches

result: two matches for dveditz@, one active (mozilla), one not (netscape.net)

expected: a single match for the one active account

Seen on BMO, but LpSolit says he can reproduce upstream

Comment 1

[Guy Pyrzak]

http://www.bugzilla.org/docs/tip/en/html/api/Bugzilla/WebService/User.html#get

That's what is being called, not sure if it is or isn't supposed to return disabled users.

Comment 2

[Guy Pyrzak]

the call being made via jsonrpc is: 

{"method":"User.get","id":4,"params":[{"match":["LpSolit"],"include_fields":["email","real_name"]}]}

Comment 3

[Frédéric Buclin]

I just checked, and ('User.get', {match => ['foo']}) also returns inactive accounts, despite Bugzilla::User::match() explicitly ignores them.

Comment 4

[Frédéric Buclin]

Ah, User.get calls:

 my $matched = Bugzilla::User::match($match_string, $limit);

but Bugzilla::User::match() only ignores disabled accounts when the 3rd argument is true. User.get() should offer the option to ignore disabled accounts.

Comment 5

[David Lawrence [:dkl]]

Attachment: Patch to exclude disabled user accounts in autocompletion (v1)

Patch to add exclude_disabled option to User.get which can be used by the username auto-completion UI feature.

Comment 6

[David Lawrence [:dkl]]

Attachment: Patch to exclude disabled user accounts in autocompletion (v2)

Added exclude_disabled to User.get history in POD.

Comment 7

[David Lawrence [:dkl]]

Attachment: Patch to exclude disabled user accounts in autocompletion (v3)

Actually attach the right patch this time :)

Comment 8

[Max Kanat-Alexander]

Comment on attachment 501144 [details] [diff] [review]
Patch to exclude disabled user accounts in autocompletion (v3)

Okay, instead of this, we're going to change the behavior of User.get.

By default, it should never return disabled accounts.

We should then add an "include_disabled" boolean that allows including disabled accounts.

Comment 9

[David Lawrence [:dkl]]

Attachment: Patch to exclude disabled user accounts in autocompletion (v4)

New patch incorporating Max's suggested changes. User.get will now exclude disabled accounts by default. Passing in include_disabled adds the disabled accounts back to the results. No changes were needed therefore with js/field.js.

Dave

Comment 10

[Max Kanat-Alexander]

Comment on attachment 505873 [details] [diff] [review]
Patch to exclude disabled user accounts in autocompletion (v4)

>=== modified file 'Bugzilla/WebService/User.pm'
>+=item C<include_disabled> (boolean)
>+
>+By default User.get will only return accounts that are not disabled.

  Let's say "enabled" instead of "not disabled".

>+Setting include_disabled to true will include any users that are set 
>+to disabled in the returned results.

  Setting C<include_disabled> to C<true> will include disabled user accounts in the returned results.

>+=item C<exclude_disabled> added in Bugzilla B<4.0>.

  This should be include_disabled, and needs to also document the fact that the default behavior changed.

Comment 11

[David Lawrence [:dkl]]

Attachment: Patch to exclude disabled user accounts in autocompletion (v4)

New patch with suggested documentation changes.

Comment 12

[Max Kanat-Alexander]

Comment on attachment 505934 [details] [diff] [review]
Patch to exclude disabled user accounts in autocompletion (v4)

>=== modified file 'Bugzilla/WebService/User.pm'
>+=item C<include_disabled> (boolean)
>+
>+By default User.get will only return accounts that are enabled. 

  I just realized that this is not true. It should instead say:

  By default, when using the C<match> parameter, disabled users are excluded from the returned results.

>+=item C<include_disabled> added in Bugzilla B<4.0>. Default behavior has changed
>+from returning all accounts to only enabled ones.

  And this should note that we're talking about the default behavior of C<match>, not of the whole function.

  Also, that line looks pretty long; is it actually shorter than 80 characters?

Comment 13

[David Lawrence [:dkl]]

Attachment: Patch to exclude disabled user accounts in autocompletion (v5)

Thanks Max. New patch with suggested changes.

Comment 14

[Max Kanat-Alexander]

Comment on attachment 507488 [details] [diff] [review]
Patch to exclude disabled user accounts in autocompletion (v5)

Beautiful. :-)

Comment 15

[David Lawrence [:dkl]]

Thanks!

trunk:
Committing to: bzr+ssh://[e-mail]/bugzilla/trunk/ modified Bugzilla/WebService/User.pm
Committed revision 7685.

4.0:
Committing to: bzr+ssh://[e-mail]/bugzilla/4.0/   modified Bugzilla/WebService/User.pm
Committed revision 7539.

Comment 16

[David Lawrence [:dkl]]

bmo/4.0:
Committing to: bzr+ssh://[e-mail]/bmo/4.0/     modified Bugzilla/WebService/User.pm
Committed revision 7504.

bmo/3.6:
Committing to: bzr+ssh://[e-mail]/bmo/3.6/      modified Bugzilla/WebService/User.pm
Committed revision 7239.

Comment 17

[Max Kanat-Alexander]

(In reply to comment #16)
> bmo/3.6:

  Ah, are you sure that's not going to break people who depend on BzAPI? Does BzAPI use User.get?

Comment 18

[David Lawrence [:dkl]]

(In reply to comment #17)
> (In reply to comment #16)
> > bmo/3.6:
> 
>   Ah, are you sure that's not going to break people who depend on BzAPI? Does
> BzAPI use User.get?

Good question. When I saw wanted-bmo I assumed 3.6. 

Gerv, what do you think?

Dave

Comment 19

[Reed Loden [:reed]]

(In reply to comment #16)
> bmo/4.0:
> Committing to: bzr+ssh://[e-mail]/bmo/4.0/    
> modified Bugzilla/WebService/User.pm
> Committed revision 7504.

Why are you landing stuff separately in bmo/4.0 that has landed upstream? Just merge...

Also, wanted-bmo doesn't mean 3.6. It just means wanted at some point on bmo, which at this point, it would probably make sense to wait until 4.0.

Comment 20

[David Lawrence [:dkl]]

(In reply to comment #19)
> (In reply to comment #16)
> > bmo/4.0:
> > Committing to: bzr+ssh://[e-mail]/bmo/4.0/    
> > modified Bugzilla/WebService/User.pm
> > Committed revision 7504.
> 
> Why are you landing stuff separately in bmo/4.0 that has landed upstream? Just
> merge...

Oversight on my part and will be corrected.
 
> Also, wanted-bmo doesn't mean 3.6. It just means wanted at some point on bmo,
> which at this point, it would probably make sense to wait until 4.0.

Apologize for this as well as I am still learning the procedures relating to BMO. I mistakenly assumed wanted-bmo meant as soon as possible if it was a low-impact change. Is there a way we differentiate between is wanted but can wait and what is wanted asap?

Dave

Comment 21

[Gervase Markham [:gerv]]

Max: yes, BzAPI uses User.get but the docs do not specify whether or not disabled users are returned.
https://wiki.mozilla.org/Bugzilla:REST_API:Methods#Search_for_users_.28.2Fuser_GET.29

I think the right thing is to do what you have done, and make it an option. I don't have an issue with changing the default behaviour to only return enabled users.

Dave: stuff that's wanted ASAP will mean there are managers putting pressure on your manager ;-)

Gerv

Comment 22

[Max Kanat-Alexander]

(In reply to comment #21)
> I think the right thing is to do what you have done, and make it an option. I
> don't have an issue with changing the default behaviour to only return enabled
> users.

  Okay. But do you want that change right now, midstream on a stable release?

Comment 23

[Gervase Markham [:gerv]]

Oh, sorry, I misunderstood. I plan to document the call as working differently on different versions of Bugzilla if you don't pass the include_disabled parameter. Whether I say >= 3.6 or >= 4.0 doesn't matter so much. So I am happy if you want to patch 3.6 to change this behaviour, and happy if you don't.

Or if you mean BzAPI: BzAPI has not yet had a stable release :-)

Gerv

Comment 24

[David Lawrence [:dkl]]

(In reply to comment #23)
> Oh, sorry, I misunderstood. I plan to document the call as working differently
> on different versions of Bugzilla if you don't pass the include_disabled
> parameter. Whether I say >= 3.6 or >= 4.0 doesn't matter so much. So I am happy
> if you want to patch 3.6 to change this behaviour, and happy if you don't.

I think the safe thing to do here then is to just wait on this til bmo/4.0 which means there is nothing more to do here.

Dave

Comment 25

[Frédéric Buclin]

Added to relnotes in bug 713346.