284262 - Bundle of small editusers.cgi post-checkin fixes

Status: RESOLVED FIXED

(Bugzilla :: Administration, task)

Description

[Marc Schumann [:Wurblzap]]

(In reply to bug 119485, comment #36)

The following points are being addressed here:

1.
> >+=item C<disabledtext>
> >+
> >+Returns the disable text of the user, if any.
> >+
> 
>   You didn't actually add an accessor for disabledtext, even though you
> documented one here. That is, we need a:
> 
>   sub disabledtext { return $self->{;disabledtext'}; }

2.
>   Also, ideally, we'd have a convenience function called is_disabled. But
> that's not necessary right now.

3.
> > Params: $username (scalar, string) - The login name for the new user.
> >         $realname (scalar, string) - The full name for the new user.
> >+        $password (scalar, string) - Optional. The password for the new user;
> >+                                     if not given, a random password will be
> >+                                     generated.
> >+        $disabledtext (scalar, string) - Optional. The disable text for the new
> >+                                         user; if not given, it will be empty.
> 
>   Nit: It would be nice to note here that putting anything in $disabledtext
> will cause the user to be "disabled," and to document what "disabled" means.

4.
> >+   desc => 'The pages to edit users can also let you delete a user. ' .
> 
>   Nit: 'The page for editing users has the ability to delete a user.'

5.
>   Also, you can use q{} so you don't have to escape the inner quotes.

I followed surrounding code convention. Should I change that anyway?

6.
> >+my $cgi       = Bugzilla->cgi();
> >+my $template  = Bugzilla->template();
> >+my $dbh       = Bugzilla->dbh;
> >+my $user      = Bugzilla->user();
> >+my $userid    = $user->id();
> 
>   Nit: Usually we don't use the () on those. It's just a consistency thing.

7.
> >+my $editusers = UserInGroup('editusers');
> 
>   Nit: Bugzilla->user->in_group($group) is preferred now, by the way. I'm going
> to make an effort to get rid of UserInGroup(), I think.

8.
> >+# Reject access if there is no sense in continuing.
> >+$editusers
> >+    || Bugzilla->user->can_bless()
> >+    || ThrowUserError("auth_failure", {group  => "editusers",
> >+                                       reason => "cant_bless",
> >+                                       action => "edit",
> >+                                       object => "users"});
> > 
> >+print Bugzilla->cgi->header();
> 
>   Shouldn't the header-printing go above the possible ThrowUserError?

Bugzilla::Error::_throw_error does it, too. It doesn't really matter; if you
insist, I'll move it up.

9.
> >+    my $query = 'SELECT DISTINCT userid, login_name, realname, disabledtext ' .
> >+                'FROM profiles';
> 
> >+    my $login = $cgi->param('login');
> >+    my $password = $cgi->param('password');
> >+
> >+    # Cleanups
> >+    my $realname = trim($cgi->param('name') || '');
> >+    my $disabledtext = trim($cgi->param('disabledtext') || '');
> 
>   Nit: I usually prefer to do all of those at the top of the file, so we have
> a nice list of possible CGI parameters. (I just do an || '' or || 0 on them.)

10.
> >+    # Cleanups
> >+    my $login           = trim($cgi->param('login')           || '');
> >+    my $loginold        =      $cgi->param('loginold')        || '';
> >+    my $realname        = trim($cgi->param('name')            || '');
> >+    my $realnameold     =      $cgi->param('nameold')         || '';
> >+    my $password        =      $cgi->param('password')        || '';
> >+    my $disabledtext    = trim($cgi->param('disabledtext')    || '');
> >+    my $disabledtextold =      $cgi->param('disabledtextold') || '';
> 
>   Nit: Once again, it's easier to do these all at the top.

Keeping the *old ones right here because that's the only place we need 'em. I'm
not too fond of script-global vars, in fact, I'm not too sure about moving any
of these up in the script -- locally scoped vars keep you from accessing them
from script subs.

11.
Changing FIXME to XXX for all occurrances.

12.
> >+    $vars->{'groups'} = $dbh->selectall_arrayref(
> >+        qq{SELECT name
> >+           FROM groups, user_group_map
> >+           WHERE id = group_id
> >+           AND user_id = ?
> >+           AND isbless = 0
> >+           ORDER BY name
> 
>   That duplicates (values %{$otherUser->groups}). You can just create a new
> Bugzilla::User. It's pretty fast.

There is a Bugzilla::User object already there, and it's even given to the
template. Scrapping this piece of code and accessing the user object's group
method directly from the template. Thanks for the hint.

Comment 1

[Marc Schumann [:Wurblzap]]

Attachment: Patch

1 -    Done
2 -    Done
3 -    Done
4 -    Will you settle for "The user editing pages are capable of letting you
       delete user accounts"?
5 -    I followed surrounding code convention. Should I change that anyway?
6 -    Done
7 -    Done
8 -    Bugzilla::Error::_throw_error does it, too. It doesn't really matter; if

       you insist, I'll move it up.
9+10 - Done. Keeping the *old ones right here because that's the only place we
       need 'em. I'm not too fond of script-global vars, in fact, I'm not too
       sure about moving any of these up in the script -- locally scoped vars
       keep you from accessing them from script subs.
11 -   Done
12 -   There is a Bugzilla::User object already there, and it's even given to
       the template. Scrapping this piece of code and accessing the user
       object's group method directly from the template. Thanks for the hint.

Comment 2

[Marc Schumann [:Wurblzap]]

Attachment: Patch

I forgot to include the fix to a bug I found -- the kind of responsibility was
not displayed correctly because of a bug in confirm-delete.html.tmpl.

Otherwise, same as above.

Comment 3

[Max Kanat-Alexander]

Comment on attachment 175954 [details] [diff] [review]
Patch

  Cool. I also had a few more comments about some of the other subs -- will
those be adressed? (They don't have to be adressed in this bug.)

  As far as having the CGI parameters as global vars -- you're probably right
about the value of not having them accessible by subs. Of course, if you
wanted, you could put the entire "main" block of code into a {} block.

  If you feel like it would be better to have them back where they were before,
you can feel free to put them back there. I now have no preference either way,
now that you've pointed that out.

  However, the trick_taints should stay within the code blocks -- I don't want
somebody using the new variables in a way when they add code that might
otherwise be dangerous. I'd like to keep all trick_taint calls as
locally-scoped as possible.

  As far as the q{} thing goes -- yeah, it's much easier to read than a bunch
of escaped quotes.

>+sub disabledtext { $_[0]->{'disabledtext'}; }
>+sub is_disabled { $_[0]->disabledtext? 1: 0; }

  Nit: I'd put a space between the ? and "disabledtext" and between the 1 and
the colon.

  Everything else looks fine on inspection. I trust that you've tested it all?
:-)

  I would like a new patch, though, with the stuff I've mentioned fixed. You
can carry forward r+.

Comment 4

[Marc Schumann [:Wurblzap]]

Attachment: Addressing comments

Comment 5

[Shane H. W. Travis]

Checking in defparams.pl;
/cvsroot/mozilla/webtools/bugzilla/defparams.pl,v  <--  defparams.pl
new revision: 1.152; previous revision: 1.151
done
Checking in editusers.cgi;
/cvsroot/mozilla/webtools/bugzilla/editusers.cgi,v  <--  editusers.cgi
new revision: 1.82; previous revision: 1.81
done
Checking in Bugzilla/User.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/User.pm,v  <--  User.pm
new revision: 1.44; previous revision: 1.43
done
Checking in template/en/default/admin/users/confirm-delete.html.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/admin/users/confirm-
delete.html.tmpl,v  <--  confirm-delete.html.tmpl
new revision: 1.2; previous revision: 1.1
done