Allow editing extern_id for users from the admin interface

RESOLVED FIXED in Bugzilla 4.2

Status

()

--
enhancement
RESOLVED FIXED
11 years ago
a year ago

People

(Reporter: vr, Assigned: jochen.wiedmann)

Tracking

(Blocks: 3 bugs)

unspecified
Bugzilla 4.2
Dependency tree / graph
Bug Flags:
approval +

Details

Attachments

(1 attachment, 9 obsolete attachments)

(Reporter)

Description

11 years ago
User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8.1.12) Gecko/20080201 Firefox/2.0.0.12
Build Identifier: 

There's no UI for setting extern_id field in user profile create/update forms.


Reproducible: Always

Updated

11 years ago
Severity: normal → enhancement

Updated

11 years ago
Whiteboard: DUPEME?

Updated

9 years ago
Status: UNCONFIRMED → NEW
Ever confirmed: true
Summary: No UI for extern_id field in user profile. → Allow editing extern_id for users from the admin interface
Whiteboard: DUPEME?
(Assignee)

Comment 1

9 years ago
As discussed in bug 392482, I am ready to take this and extract the related parts from attachment 392113 [details] [diff] [review].

Question: How to enable this feature? Through a parameter?

Comment 2

9 years ago
Well, perhaps instead there should be a uses_extern_id constant that can be called against all forms of Auth, including Auth::Stack and if it's true (that is, if any auth system currently used supports extern_id) then we display the extern_id editing field.
(Assignee)

Comment 3

9 years ago
(In reply to comment #2)
> Well, perhaps instead there should be a uses_extern_id constant that can be
> called against all forms of Auth, including Auth::Stack and if it's true (that
> is, if any auth system currently used supports extern_id) then we display the
> extern_id editing field.

Sounds much better than a parameter. If noone else intervenes, I'll follow that direction.
(Assignee)

Comment 4

9 years ago
Created attachment 394774 [details] [diff] [review]
Proposed patch
(Assignee)

Updated

9 years ago
Flags: approval?

Comment 5

9 years ago
You must request for review on the attachment itself. Approval will be requested by the reviewer.
Flags: approval?
(Assignee)

Updated

9 years ago
Attachment #394774 - Flags: review?

Comment 6

9 years ago
Comment on attachment 394774 [details] [diff] [review]
Proposed patch

Hey Jochen. Make sure to request review specifically from somebody, or the patch will probably never get reviewed, in the future. (The development process we're using is explained at http://wiki.mozilla.org/Bugzilla:Developers ).

I will do this review now, though.

Comment 7

9 years ago
Comment on attachment 394774 [details] [diff] [review]
Proposed patch

>Index: Bugzilla/Auth.pm
>+sub extern_id_required {
>+   my ($self) = @_;
>+   return $self->{_info_getter}->extern_id_required()
>+      ||  $self->{_verifier}->extern_id_required();
>+}

  Hmmm, is it actually "required" or is it just "used" for everything that uses extern_id? For example, I don't think that Env *requires* an extern_id, right?

>Index: Bugzilla/User.pm
>     'disable_mail'   => 0,
>+    'extern_id'      => ''
> };

  I don't see a reason why DEFAULT_USER has to have an extern_id.

>@@ -178,6 +182,25 @@
>+# Check, whether the extern_id is unique.
>+sub _check_extern_id {
>+    my ($invocant, $extern_id) = @_;
>+    $extern_id = trim($extern_id);
>+    if (!Bugzilla->user->authorizer->extern_id_required) {
>+        return $extern_id;
>+    }

  Actually, we should always be checking this, because even if it's disabled, we don't want to be inserting invalid extern ids into the DB.

>+    $extern_id || ThrowUserError('extern_id_required');

  Here, we should probably just be returning undef instead of throwing an error.

>+    if (!ref($invocant) || $invocant->extern_id ne $extern_id) {
>+        my $existing_login_name = extern_id_to_login_name($extern_id);

  You don't need to add this method. You can use:

  Bugzilla::Object::match($class, { extern_id => $extern_id })

  (The only reason we don't call it as $class->match is because there's already a Bugzilla::User::match that does something different, which we need to rename. You're welcome to do that in a separate patch, too, if you'd like.)

>+        if ($existing_login_name) {
>+            ThrowUserError('extern_id_exists',
>+                           { extern_id => $extern_id,
>+                             existing_login_name => $existing_login_name});
>+        }

  You should only be doing this if the returned user isn't the current user. It's OK for somebody to not change their extern_id.

>@@ -217,6 +240,7 @@
> 
> sub set_disabledtext { $_[0]->set('disabledtext', $_[1]); }
> sub set_disable_mail { $_[0]->set('disable_mail', $_[1]); }
>+sub set_extern_id { $_[0]->set('extern_id', $_[1]); }

  Nit: It would be nice if the braces were aligned.

>Index: template/en/default/global/messages.html.tmpl
>+            [% ELSIF field == 'extern_id' %]
>+              The users login ID has been modified.

  "The user's"

  Also, probably best to capitalize "Login" since we do on the form.

>+The default value is C<false>.

  C<0> is more accurate. There is no C<false> in Perl, really, so we just literally document it as C<0>.

>Index: Bugzilla/Auth/Login/Stack.pm
>Index: editusers.cgi
>+++ editusers.cgi	17 Aug 2009 06:06:44 -0000
>@@ -209,6 +209,9 @@
>         realname      => scalar $cgi->param('name'),
>         disabledtext  => scalar $cgi->param('disabledtext'),
>         disable_mail  => scalar $cgi->param('disable_mail')});
>+    if ($new_user->authorizer->extern_id_required) {
>+        $new_user->set_extern_id(scalar $cgi->param('extern_id'));
>+    }

  Just be pass in extern_id to create()--don't call set_ on a new user.

>@@ -251,6 +254,10 @@
>             if $cgi->param('password');
>         $otherUser->set_disabledtext($cgi->param('disabledtext'));
>         $otherUser->set_disable_mail($cgi->param('disable_mail'));
>+        if ($otherUser->authorizer->extern_id_required) {
>+            my $extern_id = $cgi->param('extern_id');
>+            $otherUser->set_extern_id($cgi->param('extern_id'));

  Just always set it if extern_id is defined. (Ideally we just want to control the UI, not the backend--otherwise we get too much complexity with optional features.)
Attachment #394774 - Flags: review? → review-
(Assignee)

Comment 8

9 years ago
(In reply to comment #7)

>   Hmmm, is it actually "required" or is it just "used" for everything that
> uses extern_id? For example, I don't think that Env *requires* an
> extern_id, right?

My personal understanding is that the modules is able to use it, thus requires the UI to support it. But I renamed extern_id_required to extern_id_used.

> I don't see a reason why DEFAULT_USER has to have an extern_id.

I admit that I always had the impression that these are also the defaults
for creating new users. Removed.

>  Actually, we should always be checking this, because even if it's
> disabled, we don't want to be inserting invalid extern ids into the DB.

Ok.

>   Here, we should probably just be returning undef instead of throwing an
> error.

Ok.


>   You don't need to add this method. You can use:
> 
>   Bugzilla::Object::match($class, { extern_id => $extern_id })

Good to know. Removed extern_it_to_login.


>   (The only reason we don't call it as $class->match is because there's already
> a Bugzilla::User::match that does something different, which we need to rename.
> You're welcome to do that in a separate patch, too, if you'd like.)

Possibly later on. For now, let's get this out of the box.


>   You should only be doing this if the returned user isn't the current user.
> It's OK for somebody to not change their extern_id.

You're right.



>   Nit: It would be nice if the braces were aligned.

:-) Done.


>   "The user's"
> 
>   Also, probably best to capitalize "Login" since we do on the form.

Fixed.


>   C<0> is more accurate. There is no C<false> in Perl, really, so we just
> literally document it as C<0>.

Changed.

>   Just be pass in extern_id to create()--don't call set_ on a new user.

Done.


>   Just always set it if extern_id is defined. (Ideally we just want to control
> the UI, not the backend--otherwise we get too much complexity with optional
> features.)

I don't share your opinion here completely, but you're the one to decide. Done.
(Assignee)

Comment 9

9 years ago
Created attachment 395014 [details] [diff] [review]
Updated patch proposal
Attachment #394774 - Attachment is obsolete: true
Attachment #395014 - Flags: review?(mkanat)
Might want to review bug 387715, code review was rejected there, but I believe that was due to the newer work on this bug and Max requesting the UI edit side, per user being moved to this bug.

bug 387715 implements extern_id for ENV auth, I'm reviewing you changes here for the constant and will work them in to that bug. Already stripped the user editing changes there will resubmit shortly.
re comment 10 referenced wrong bug should be bug 503372 not 387715 which is the attachment id for the submission in bug 503372
Alright cool, looks like all the changes will be on my code in 503372, nice work Jochen. I've stripped my old UI tweaks out in favor of yours.
Blocks: 503372

Updated

9 years ago
Blocks: 521536

Updated

9 years ago
Blocks: 521543

Comment 13

9 years ago
Comment on attachment 395014 [details] [diff] [review]
Updated patch proposal

>+++ Bugzilla/User.pm	18 Aug 2009 07:12:18 -0000
>@@ -77,7 +77,7 @@
>     'login_name'     => '',
>     'showmybugslink' => 0,
>     'disabledtext'   => '',
>-    'disable_mail'   => 0,
>+    'disable_mail'   => 0

  I'm assuming this was some change made by some code auto-formatter. Make sure it doesn't go in as part of the patch. :-)

>+    if (!ref($invocant) || $invocant->extern_id ne $extern_id) {


>+        my $existing_logins = Bugzilla::Object::match("Bugzilla::User",
>+                                                      { extern_id => $extern_id });

  extern_id is supposed to be unique, yes? You could do "|| []" there to simplify the below code.

  You could also add extern_id as a value for Bugzilla::User->new, which I think would be really useful.

>Index: template/en/default/global/user-error.html.tmpl
>+    [% IF extern_id %]
>+      the login ID [% extern_id FILTER html %].
>+    [% ELSE %]
>+      that login ID.
>+    [% END %]

  Won't there always be an extern_id defined?

>+=item C<extern_id_used>
>+
>+Whether or not this verifier requires the extern_id field.


  Mmm, I wouldn't say requires. I'd say "uses".

>Index: Bugzilla/Auth/Login.pm
>+Whether or not this login method requires the extern_id field. If

  And same there.

>Index: editusers.cgi
>+    my $user_attributes = {
>         login_name    => scalar $cgi->param('login'),
>         cryptpassword => scalar $cgi->param('password'),
>         realname      => scalar $cgi->param('name'),
>         disabledtext  => scalar $cgi->param('disabledtext'),
>-        disable_mail  => scalar $cgi->param('disable_mail')});
>+        disable_mail  => scalar $cgi->param('disable_mail')
>+    };
>+    if ($user->authorizer->extern_id_used) {
>+        $user_attributes->{'extern_id'} = scalar $cgi->param('extern_id');
>+    }
>+    my $new_user = Bugzilla::User->create($user_attributes);

  Might as well always pass it, for create. It'll just be set to undef if it's undef.

>@@ -251,6 +256,8 @@
>             if $cgi->param('password');
>         $otherUser->set_disabledtext($cgi->param('disabledtext'));
>         $otherUser->set_disable_mail($cgi->param('disable_mail'));
>+        my @extern_ids = $cgi->param('extern_id');
>+        $otherUser->set_extern_id(shift @extern_ids);

  No reason to do the "my @" stuff. Also, you'll want to only call this if the field appears in the template (if defined $cgi->param('extern_id')) otherwise you're going to be unsetting it if people switch methods.


  With all of these fixes, the patch should pass review.
Attachment #395014 - Flags: review?(mkanat) → review-
Will work on adapting attachment 395014 [details] [diff] [review] + current tip changes + comment 13 changes
Assignee: user-accounts → mockodin
Status: NEW → ASSIGNED
Created attachment 414685 [details] [diff] [review]
v2

Modified the original patch based on comment 13

Note I have not tested the patch file.
Attachment #395014 - Attachment is obsolete: true
Attachment #414685 - Flags: review?(mkanat)

Comment 16

9 years ago
Comment on attachment 414685 [details] [diff] [review]
v2

>Index: bugzilla/editusers.cgi
>@@ -208,7 +208,9 @@
>         cryptpassword => scalar $cgi->param('password'),
>         realname      => scalar $cgi->param('name'),
>         disabledtext  => scalar $cgi->param('disabledtext'),
>-        disable_mail  => scalar $cgi->param('disable_mail')});
>+        disable_mail  => scalar $cgi->param('disable_mail'),
>+	extern_id     => scalar $cgi->param('extern_id'),

  Nit: Spacing. (Don't use tabs.)

>@@ -251,6 +253,8 @@
>             if $cgi->param('password');
>         $otherUser->set_disabledtext($cgi->param('disabledtext'));
>         $otherUser->set_disable_mail($cgi->param('disable_mail'));
>+	$otherUser->set_extern_id($cgi->param('extern_id'))
>+            if $cgi->param('extern_id');

  Nit: Tabs.
 
>Index: bugzilla/Bugzilla/Auth.pm
>+sub extern_id_used {
>+   my ($self) = @_;
>+   return $self->{_info_getter}->extern_id_used()
>+      ||  $self->{_verifier}->extern_id_used();

  You can omit the () from the end of those, because they are just simple accessors (getters).

>@@ -341,6 +347,10 @@
>+=item C<extern_id_used>
>+
>+Description: Whether or not editing the extern_id is enabled.

  Well, no, it's really more "whether or not this authentication method uses the extern_id column of the profiles table to store a unique identifier from an external login system" or something like that.

>Index: bugzilla/Bugzilla/User.pm
> [snip]
>+    if (!ref($invocant) || $invocant->extern_id ne $extern_id) {
>+        my $existing_logins = Bugzilla::Object::match("Bugzilla::User",
>+                                                      { extern_id => $extern_id }) || [];


  Mmmm, instead of using match, just modify Bugzilla::User->new to also take { extern_id => 'something' }.

>+            if (!ref($invocant)  ||  $invocant->id ne $existing_login->id) {

  You don't actually have to check that, right? Because you already checked it with "ne" above. (If you do need to keep this check, numbers should be compared with != instead of "ne".)

>+sub extern_id { $_[0]->{extern_id}; }

  There's another patch that also creates this accessor, so I think we'll have to decide on its name.

>Index: bugzilla/Bugzilla/Auth/Login.pm
>+Whether or not this login method uses the extern_id field. If
>+so, editing the extern_id will be enabled in the UI. Note, that
>+this enables editing the extern_id for all users! There is no way
>+to restrict this feature to certain users.

  Mmm, that sounds a bit like everybody will be able to edit everybody's extern_id. You're welcome to leave out the last two sentences entirely, if you want. (Same note for Verify.pm.)

>Index: bugzilla/template/en/default/admin/users/userdata.html.tmpl

>+[% IF user.authorizer.extern_id_used %]

  Mmm, it's not user.authorizer. You want the global authorizer. Otherwise, if they just logged in, this won't be accurate.

>+<tr>
>+  <th><label for="extern_id">Login ID:</label></th>

  So we have to decide whether we want to call this "Login ID" or "External Login ID". I'm undecided on it.

>+  <td>
>+    [% IF editusers %]
>+      <input size="64" maxlength="255" name="extern_id"

  Right now it's a varchar(64), not 255.

  Also, size=64 is enormous. I think a much shorter size would be appropriate.

>Index: bugzilla/template/en/default/global/user-error.html.tmpl
>+  [% ELSIF error == "extern_id_used" %]
>+    [% title = "Missing Login ID" %]
>+    You must enter a login name for the new user.

  The name and text of that error don't match. Also, you haven't actually put enough data into Bugzilla to know whether or not extern_id is required--that would require a separate extern_id_required accessor in Auth.pm to note that no auth methods are enabled that *don't* need an extern_id, right?
Attachment #414685 - Flags: review?(mkanat) → review-
Created attachment 417124 [details] [diff] [review]
v3

direct edited the patch, so there are a number of extra blank lines where I just removed stuff. Purely for the sake of being on a machine without a copy of source code and wanting to throw up some corrections.

(In reply to comment #16)

>   Nit: Spacing. (Don't use tabs.)
WTB VI hints in files to help enforce this
>   Nit: Tabs.
ditto

>   You can omit the () from the end of those, because they are just simple
> accessors (getters).

yep

>   Well, no, it's really more "whether or not this authentication method uses
> the extern_id column of the profiles table to store a unique identifier from an
> external login system" or something like that.

skipping update for the moment

>   Mmmm, instead of using match, just modify Bugzilla::User->new to also take {
> extern_id => 'something' }.

skipping atm, only because it works, will review later if we really want to changed.

>   You don't actually have to check that, right? Because you already checked it
> with "ne" above. (If you do need to keep this check, numbers should be compared
> with != instead of "ne".)

same as previous, though with != fixed

> >+sub extern_id { $_[0]->{extern_id}; }
> 
>   There's another patch that also creates this accessor, so I think we'll have
> to decide on its name.

could that have been from my other bug? bug 503372 though I backed out most changes there that duplicated here

> >Index: bugzilla/Bugzilla/Auth/Login.pm
> >+Whether or not this login method uses the extern_id field. If
> >+so, editing the extern_id will be enabled in the UI. Note, that
> >+this enables editing the extern_id for all users! There is no way
> >+to restrict this feature to certain users.
> 

Kinda cleaned up, will fix extra lines later.

>   Mmm, it's not user.authorizer. You want the global authorizer. Otherwise, if
> they just logged in, this won't be accurate.

Hint as to where to find this? Or blunt look at line xx in file yy would work too :-P

> >+<tr>
> >+  <th><label for="extern_id">Login ID:</label></th>
> 
>   So we have to decide whether we want to call this "Login ID" or "External
> Login ID". I'm undecided on it.

voting "External Login ID" makes more sense, I did not par attention to what it had been set as, confused me a moment when I went to set one in my dev instance.


>   Right now it's a varchar(64), not 255.
> 
>   Also, size=64 is enormous. I think a much shorter size would be appropriate.

I'd say leave it as 64, I have some users with freaken long last names.. But yeah should be max length 64 not 255

> >Index: bugzilla/template/en/default/global/user-error.html.tmpl
> >+  [% ELSIF error == "extern_id_used" %]
> >+    [% title = "Missing Login ID" %]
> >+    You must enter a login name for the new user.
> 
>   The name and text of that error don't match. Also, you haven't actually put
> enough data into Bugzilla to know whether or not extern_id is required--that
> would require a separate extern_id_required accessor in Auth.pm to note that no
> auth methods are enabled that *don't* need an extern_id, right?

I actually think we should remove the requirement possibility anyhow. Else there is also the issue of dual auth where one requires and one does not. Require is harsh anyhow as the lack of simply means auth fails for the system that used extern_id. If I have a choice I'd rather deny access than let some one in. Also noted on review that no place in the changes actually called that error anyhow.
Attachment #414685 - Attachment is obsolete: true
Attachment #417124 - Flags: review?(mkanat)

Comment 18

9 years ago
Hey Michael. Thanks for the updates, and I'm really glad that you got up a new patch so quickly, but I really would like a patch that actually has all the comments addressed. The problem is that it's too easy to miss things that I thought I already reviewed, and that if I don't know that all the comments have been addressed, I have to re-look at the entire patch every time, as opposed to just the parts that were updated.
Understood, the parts that were skipped either worked, even if it was comment that another method might be better, or were documentation so were secondary to code fixes. I should have time in the next few weeks to work on a complete patch. Though any help on documentation is good. I like to code things, documentation always seems an afterthought to me.

Comment 20

9 years ago
(In reply to comment #19)
> Understood, the parts that were skipped either worked, even if it was comment
> that another method might be better, or were documentation so were secondary to
> code fixes. I should have time in the next few weeks to work on a complete
> patch.

  Okay. It will make it into 3.8 instead of 3.6 in that case.

> Though any help on documentation is good. I like to code things,
> documentation always seems an afterthought to me.

  Sure, feel free to ask me in IRC if you want any help.

Comment 21

9 years ago
Comment on attachment 417124 [details] [diff] [review]
v3

r- based on my last comments.
Attachment #417124 - Flags: review?(mkanat) → review-
Created attachment 426465 [details] [diff] [review]
v4

I believe this Addresses everything except:
bugzilla/template/en/default/admin/users/userdata.html.tmpl
>+[% IF user.authorizer.extern_id_used %]

I'm not sure what you are suggesting that I look at using instead.
Attachment #417124 - Attachment is obsolete: true
Attachment #426465 - Flags: review?(mkanat)

Comment 23

9 years ago
(In reply to comment #22)
> I believe this Addresses everything except:
> bugzilla/template/en/default/admin/users/userdata.html.tmpl
> >+[% IF user.authorizer.extern_id_used %]
> 
> I'm not sure what you are suggesting that I look at using instead.

  You need to pass Bugzilla::Auth->extern_id_used to the template, most likely (that's my suggestion without looking at this patch again in detail).

Updated

9 years ago
Target Milestone: --- → Bugzilla 3.8

Comment 24

9 years ago
Comment on attachment 426465 [details] [diff] [review]
v4

  This is looking pretty good! A few things to fix, though:

>Index: Bugzilla/User.pm
>+# Check, whether the extern_id is unique.
>+sub _check_extern_id {
>+    my ($invocant, $extern_id) = @_;
>+    $extern_id = trim($extern_id);
>+    return undef unless $extern_id;

  "0" could be a valid extern_id.

>+    if (!ref($invocant) || $invocant->extern_id != $extern_id) {
>+        my $existing_login = Bugzilla::User->new({
>+                                                  condition => 'extern_id = ?',
>+                                                  values    => [$extern_id],
>+                                                 });

  condition/values isn't supposed to be used like this--perhaps that should be clearer in the POD. They're only supposed to be used from within implementations of new(). If you want, add an "extern_id" argument as a possibility directly to new() (which I think would be a good idea anyhow).

>+            ThrowUserError('extern_id_exists',
>+                          { extern_id => $extern_id,

  Nit: Spacing; should be one space further.

>+                            existing_login_name => $existing_login->login});

  Nit: Spacing; should have one space before }.

>Index: template/en/default/admin/users/userdata.html.tmpl
>+[% IF user.authorizer.extern_id_used %]
>+<tr>

  Nit: Spacing; That <tr> (and all its content) should be indented two spaces.

>Index: template/en/default/global/messages.html.tmpl
>+            [% ELSIF field == 'extern_id' %]
>+              The user's Login ID has been modified.

  External Login ID.

>Index: template/en/default/global/user-error.html.tmpl
>+  [% ELSIF error == "extern_id_exists" %]
>+    [% title = "Account Already Exists" %]
>+    There is already an account
>+    [% IF existing_login_name %]
>+      ([% existing_login_name FILTER html %])
>+    [% END %]
>+    with the login ID [% extern_id FILTER html %].

  You should probably put quotes around the extern_id there.


  You didn't set extern_id_used to 1 for any of the login or verify methods.
Attachment #426465 - Flags: review?(mkanat) → review-

Comment 25

9 years ago
  Oh, and the error should say External Login ID instead of "login ID".
 Bug 503372(In reply to comment #24)
>   You didn't set extern_id_used to 1 for any of the login or verify methods.

 Bug 503372 - Does

Comment 27

9 years ago
(In reply to comment #26)
>  Bug 503372(In reply to comment #24)
> >   You didn't set extern_id_used to 1 for any of the login or verify methods.
> 
>  Bug 503372 - Does

  Okay. This bug is the one where you added extern_id_used in Bugzilla::Auth, and added the docs for it, so let's implement it here.
(In reply to comment #24)
> >+    if (!ref($invocant) || $invocant->extern_id != $extern_id) {
> >+        my $existing_login = Bugzilla::User->new({
> >+                                                  condition => 'extern_id = ?',
> >+                                                  values    => [$extern_id],
> >+                                                 });
> 
>   condition/values isn't supposed to be used like this--perhaps that should be
> clearer in the POD. They're only supposed to be used from within
> implementations of new(). If you want, add an "extern_id" argument as a
> possibility directly to new() (which I think would be a good idea anyhow).

Directly in NEW
User->new calls the base of Object->new which calls Object->Init
Using condition/values already makes use of init functionality, I'm unsure of the change you'd like to see. Adding to User->new feels like were breaking the point of Object.pm. We return extra data, as in the whole User Object if we match but that doesn't feel like an issue.

So other than that I have a patch ready for everything else.

Comment 29

9 years ago
  Ahh, okay. See Bugzilla::Component's new() sub for how we'd do it. Basically what I want is to be able to say Bugzilla::User->new({ extern_id => 'aadsfasdf' });
Created attachment 429616 [details] [diff] [review]
v5

updated based on comment 24 and comment 29
Attachment #426465 - Attachment is obsolete: true
Attachment #429616 - Flags: review?(mkanat)

Comment 31

9 years ago
Comment on attachment 429616 [details] [diff] [review]
v5

>     $vars->{'token'} = issue_session_token('add_user');
>+    my $auth = new Bugzilla::Auth;
>+    $vars->{'extern_id_used'} = $auth->extern_id_used;

  Hmmm. Instead of this...it seems like it would be better to add a global_authorizer variable to the global template variables.

>@@ -256,6 +261,8 @@
>             if $cgi->param('password');
>         $otherUser->set_disabledtext($cgi->param('disabledtext'));
>         $otherUser->set_disable_mail($cgi->param('disable_mail'));
>+        $otherUser->set_extern_id($cgi->param('extern_id'))
>+            if $cgi->param('extern_id');

  That should be if defined, not just if. (Otherwise people couldn't empty it.)

>Index: MSSQL_BUGZILLA/Bugzilla/Auth.pm
>+sub extern_id_used {
>+    my ($self) = @_;
>+    return $self->{_info_getter}->extern_id_used
>+        ||  $self->{_verifier}->extern_id_used;

  Nit on spacing: I'd align || with the $ above.

>Index: MSSQL_BUGZILLA/Bugzilla/User.pm
>@@ -133,6 +136,12 @@
>     bless ($user, $class);
>     return $user unless $param;
> 
>+    if (ref $param eq 'HASH'){

  I'd do ref($param) there. Not sure what precedence is, there.

>+        if (defined $param->{extern_id}){
>+            $param = { condition => 'extern_id = ?' , values => [$param->{extern_id}] };
>+        }
>+        unshift @_, $param;

  No, that's not right, because you didn't shift $param off. You'll want to change the ($param) = @_ to $param = shift.

>@@ -178,6 +187,22 @@
>+# Check, whether the extern_id is unique.
>+sub _check_extern_id {
>+    my ($invocant, $extern_id) = @_;
>+    $extern_id = trim($extern_id);
>+    return undef unless defined $extern_id;

  Actually, you should be returning undef for an empty string as well.

>Index: MSSQL_BUGZILLA/Bugzilla/Auth/Login/Stack.pm
>+sub extern_id_used {
>+    my ($self) = @_;
>+    foreach my $object (@{$self->{_stack}}) {
>+        return 1 if $object->extern_id_used;
>+    }

  Now that we can use List::Util, you might be able to do this with "first", instead. There's some docs in the List::Util POD on how to check if anything in an array is true.

>Index: MSSQL_BUGZILLA/template/en/default/admin/users/userdata.html.tmpl
>+      [% IF editusers %]
>+          <input size="64" maxlength="64" name="extern_id"
>+                 id="extern_id" value="[% otheruser.extern_id FILTER html %]" />

  No / at the end of the tag--we're in HTML, not XHTML.


  Otherwise, looks pretty good. :-)
Attachment #429616 - Flags: review?(mkanat) → review-

Updated

8 years ago
Target Milestone: Bugzilla 4.0 → ---
(Assignee)

Comment 32

8 years ago
Assuming that I provide an updated patch: Any change to get this into 4.0?

Comment 33

8 years ago
(In reply to comment #32)
> Assuming that I provide an updated patch: Any change to get this into 4.0?

  At this point, no, it would go into 4.2.
(Assignee)

Comment 34

8 years ago
Created attachment 516985 [details] [diff] [review]
Updated patch against Bugzilla 4.0
(Assignee)

Comment 35

8 years ago
Max, here's an updated patch against Bugzilla 4.0. I followed all your comments except this one:

> >     $vars->{'token'} = issue_session_token('add_user');
> >+    my $auth = new Bugzilla::Auth;
> >+    $vars->{'extern_id_used'} = $auth->extern_id_used;

>   Hmmm. Instead of this...it seems like it would be better to add a
> global_authorizer variable to the global template variables.

Sorry, but I don't understand this. Could you please be so kind an explain?

Comment 36

8 years ago
> Sorry, but I don't understand this. Could you please be so kind an explain?

  Hey Jochen. Ah, sure! See the "VARIABLES" item in Bugzilla/Template.pm. That specifies a bunch of variables or functions that are globally available in every template.
(Assignee)

Comment 37

8 years ago
Created attachment 517110 [details] [diff] [review]
Updated patch against Bugzilla 4.0
Attachment #516985 - Attachment is obsolete: true
(Assignee)

Updated

8 years ago
Attachment #517110 - Flags: review?(mkanat)
(Assignee)

Comment 38

8 years ago
One more update, this time moving the definition of an extern_id_used variable from editbugs.cgi to Bugzilla/Template.pm. Also fixing one minor issue (missing perldoc entry) from the previous patch. Asking for review.
(Assignee)

Comment 39

8 years ago
Created attachment 517118 [details] [diff] [review]
Updated patch against Bugzilla 4.0

Rats, picked up the wrong patch file. Sorry! Here's the right one: One more update, this time moving the definition of an extern_id_used variable
from editbugs.cgi to Bugzilla/Template.pm. Also fixing one minor issue (missing
perldoc entry) from the previous patch. Asking for review.
Attachment #517110 - Attachment is obsolete: true
Attachment #517118 - Flags: review?(mkanat)
Attachment #517110 - Flags: review?(mkanat)

Comment 40

8 years ago
  Awesome, thanks for the patch, Jochen! I think I can get to reviewing it this week. :-)

Updated

8 years ago
Assignee: mockodin → jochen.wiedmann

Comment 41

8 years ago
Comment on attachment 517118 [details] [diff] [review]
Updated patch against Bugzilla 4.0

>diff -ubrN bugzilla-4.0.orig/Bugzilla/Auth/Login/Stack.pm bugzilla-4.0.extern_id/Bugzilla/Auth/Login/Stack.pm
>+use List::Util();

  Usually we import the functions we're going to use, for List::Util, so in this case qw(first).

>+sub extern_id_used {
>+    my ($self) = @_;
>+    return List::Util::first { $_->extern_id_used } @{$self->{_stack}};

  I think "any" would be more appropriate, no? (Although I imagine it's basically the same thing in this case.) (We also can use List::MoreUtils.)

>diff -ubrN bugzilla-4.0.orig/Bugzilla/Template.pm bugzilla-4.0.extern_id/Bugzilla/Template.pm
>+    my $auth = new Bugzilla::Auth;
>     my $config = {
>         # Colon-separated list of directories containing templates.
>         INCLUDE_PATH => $opts{'include_path'} 
>@@ -937,6 +938,7 @@
>                 }
>                 return \@optional;
>             },
>+            'extern_id_used' => $auth->extern_id_used,

  Hmm, instead of this, how about:

  default_authorizer => new Bugzilla::Auth(),

  Then we can get any information we want about the default authorizer in the templates.

>@@ -129,12 +132,18 @@
>-    my ($param) = @_;
>+    my($param) = @_;

  Nit: We usually keep that space.

>+    if (ref($param) eq 'HASH'){

  Nit: Space before the {

>+        if (defined $param->{extern_id}){

  And there too.

>+# Check, whether the extern_id is unique.
>+sub _check_extern_id {
>+    my ($invocant, $extern_id) = @_;
>+    $extern_id = trim($extern_id);
>+    return undef unless defined($extern_id) && $extern_id ne "";

>+    if (!ref($invocant) || $invocant->extern_id != $extern_id) {

  That should be "ne" not "!="

>+        my $existing_login = Bugzilla::User->new({ extern_id => $extern_id });

  Maybe make that $invocant->new for subclassing's sake.


  Overall, I would like to say that this is a beautiful patch. I'm sorry that it took so long for me to get to the review; if you attach a new one I will review it more quickly.
Attachment #517118 - Flags: review?(mkanat) → review-
(Assignee)

Comment 42

8 years ago
Max, before proposing a new patch, I've got a question. My original implementation of check_extern_id contained the following snippet:

  my $existing_logins = Bugzilla::Object::match("Bugzilla::User",
                                { extern_id => $extern_id }) || [];

which looks good to me. Now, the current version looks (after applying your suggestion)

  my $existing_login = $invocant->new({ extern_id => $extern_id });
  if ($existing_login) {

And this one doesn't look good to me. Isn't the test always true, because "new" always returns an object?

Comment 43

8 years ago
Hey Jochen. Oh, no, new() returns undef if it doesn't find something.
(Assignee)

Comment 44

8 years ago
Created attachment 527416 [details] [diff] [review]
Updated patch against Bugzilla 4.1.1

Updated patch, following all of Max' suggestions with the exception of importing List::Util qw(any), because "any" is not exported.
Attachment #517118 - Attachment is obsolete: true
Attachment #527416 - Flags: review?(mkanat)

Comment 45

8 years ago
Ah, "any" is probably in List::MoreUtils.

Comment 46

8 years ago
Comment on attachment 527416 [details] [diff] [review]
Updated patch against Bugzilla 4.1.1

Review of attachment 527416 [details] [diff] [review]:

This is beautiful. :-) A few small notes, but they can be fixed on checkin:

::: bugzilla-4.1.1.orig/Bugzilla/Auth/Login/Stack.pm
@@ +28,4 @@
 );
 use Hash::Util qw(lock_keys);
 use Bugzilla::Hook;
+use List::Util ();

I generally prefer to actually import the ::Util subroutines.

::: bugzilla-4.1.1.orig/Bugzilla/Auth/Verify/Stack.pm
@@ +88,5 @@
 
+sub extern_id_used {
+    my ($self) = @_;
+    foreach my $object (@{$self->{_stack}}) {
+        return 1 if $object->extern_id_used;

This is a case for "any" too, no?

::: bugzilla-4.1.1.orig/Bugzilla/User.pm
@@ +189,4 @@
 sub _check_disable_mail { return $_[1] ? 1 : 0; }
 sub _check_disabledtext { return trim($_[1]) || ''; }
 
+# Check, whether the extern_id is unique.

Nit: No comma necessary there.
Attachment #527416 - Flags: review?(mkanat) → review+

Updated

8 years ago
Flags: approval+
Target Milestone: --- → Bugzilla 4.2

Updated

8 years ago
Attachment #429616 - Attachment is obsolete: true

Comment 47

8 years ago
All checkin fixes done, and committed. Thanks, Jochen! :-)

Committing to: bzr+ssh://bzr.mozilla.org/bugzilla/trunk/                       
modified editusers.cgi
modified Bugzilla/Auth.pm
modified Bugzilla/Template.pm
modified Bugzilla/User.pm
modified Bugzilla/Auth/Login.pm
modified Bugzilla/Auth/Verify.pm
modified Bugzilla/Auth/Login/Env.pm
modified Bugzilla/Auth/Login/Stack.pm
modified Bugzilla/Auth/Verify/Stack.pm
modified template/en/default/admin/users/userdata.html.tmpl
modified template/en/default/global/messages.html.tmpl
modified template/en/default/global/user-error.html.tmpl
Committed revision 7796.
Status: ASSIGNED → RESOLVED
Last Resolved: 8 years ago
Resolution: --- → FIXED

Updated

8 years ago
Blocks: 653713

Updated

6 years ago
Blocks: 829939

Updated

a year ago
Blocks: 1408862
You need to log in before you can comment on or make changes to this bug.