283937 - editusers.cgi templatisation post-checkin cleanups

Status: RESOLVED FIXED

(Bugzilla :: Administration, task)

Description

[Marc Schumann [:Wurblzap]]

This bug exists to address what's said in bug 119485 comment 36.

Comment 1

[Frédéric Buclin]

This should be fixed before the 2.20 freezing period.

Comment 2

[Marc Schumann [:Wurblzap]]

Quick fixes go into bug 284262, bigger ones (especially ones touching other
files in a non-simple manner) into individual bugs.

(In reply to bug 119485, comment #36)
> >+=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'}; }

Bug 284262, even though touching User.pm.

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

Bug 284262, even though touching User.pm.

> > 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.

Bug 284262, even though touching User.pm.

> >+   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.'

Bug 284262, even though touching defparams.pl.

>   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?

> >-# The Initial Developer of the Original Code is Holger
> >-# Schurig. Portions created by Holger Schurig are
> >-# Copyright (C) 1999 Holger Schurig. All
> >-# Rights Reserved.
> 
>   Normally this section *does* stay I think, but I'm not certain. I've also
> removed it in some of my recent patches.

It's a rewrite, so I took the license header for new files.

> > require "CGI.pl";
> > require "globals.pl";
> 
>   Are these actually required anymore? I'd like to get rid of them, and most
> new pages (particularly admin pages) don't need them.

We need at least GetFieldID from globals.pl and at least CheckEmailSyntax from
CGI.pl.

> >+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.

Bug 284262.

> >+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.

Bug 284262.

> >+# 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 in bug 284262.

> >+    $vars->{'restrictablegroups'} = groupsUserMayBless($user, 'id', 'name');
> 
>   That duplicates Bugzilla->user->bless_groups, I'm pretty sure, since I
> checked that code in.

Bug 284263.

> >+    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.)

Bug 284262.

> >+    canSeeUser($otherUserID)
> 
>   Nit: That really should be a method of Bugzilla::User, I'd think.

Bug 284264.

> >+    # 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.

Bug 284262; 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.

> >+            # Since we change the login, silently delete any tokens.
> >+            $dbh->do('DELETE FROM tokens WHERE userid = ?', {}, $otherUserID);
> 
>   Does Bugzilla::Token::DeletePasswordTokens not work for that?

No (see bug 119485, comment 24).

>   If not, we should have a Bugzilla::User function that cleans the token table
> for that user, I think. That's just a nit, though.

Bug 284267.

> >+            # FIXME: should create profiles_activity entries.
> 
>   *cough* :-)

The profiles_activity table currently works for group membership changes only.

> >+        $dbh->do('UPDATE profiles SET refreshed_when=? WHERE userid = ?',
> >+                 undef, ('1900-01-01 00:00:00', $otherUserID));
> 
>   Whoa, no, don't you want that to be NOW()?? That means that we'll NEVER
> derive_groups when we create that user...

Refreshs happen if profiles.refreshed_when <= groups.last_changed, so we need to
make sure that profiles.refreshed_when is earlier than groups.last_changed. A
1900 date should take care of that :)

> >+    # FIXME: should create profiles_activity entries for blesser changes.
> 
>   And that. If these aren't critical, they should be XXX, not FIXME. "FIXME"
> usually means, "this code is broken in a way important to current
> functionality."

Same as above.

Changing FIXME to XXX for all occurrances.

> >+    $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 in bug 284262 and accessing the user
object's group method directly from the template. Thanks for the hint.

> >+    # FIXME: if there was some change on these tables after the deletion
> >+    #        confirmation checks, we may do something here we haven't warned
> >+    #        about.
> 
>   Then you should just move the lock_tables earlier.

Can't do that -- what I'm talking about is this: maybe the admin views the
deletion confirmation page, giving no warnings. The user now adds a comment to a
bug. Then, the admin confirms the deletion. Result: an admin wondering why
there's a red number in his sanitycheck.cgi output.
This cannot be handled by locking because we can't lock a table across page views.

> >+###########################################################################
> >+# Helpers
> >+###########################################################################
> 
>   These really ought to be at the top of the file; it makes it much easier to
> read when I know that these subroutines are defined within this file and what
> they are.

When reading a script, I like to get the global view first, and if I wish to go
into detail, I read on.
Let's call it a tie :)

> >+# Copy incoming list selection values from CGI params to template variables.
> >+sub mirrorListSelectionValues {
> 
>   That's not really the best sub name, but I can't think of a better one
> (because I don't fully understand it).

Let's rename it when we find a better name then.

> >+# Give a list of IDs of groups the user may bless.
> >+sub groupsUserMayBless {
> 
>   As I mentioned above, you can do most of this with
> Bugzilla->user->bless_groups. You could have also done it with
> UserCanBlessGroups() or whatever it was called before in globals.pl.

Bug 284263.

> >+    $user->derive_groups(1);
> 
>   That should never be necesary, by the way, unless you've changed the groups
> after you've created the User.

Bug 284272.

> >+    # If visibilitygroups are used, restrict the set of groups.
> >+    if (Param('usevisibilitygroups')) {
> >+        my $visibleGroups = visibleGroupsAsString();
> >+        $query .= " $connector id in ($visibleGroups)";
> >+    }
> 
>   We should probably work to do this inside of Bugzilla::User, somehow. That's
> just a thought for the future.

Ok.

> >+# Retrieve product responsibilities, usable for both display and verification.
> >+sub productResponsibilities {
> 
>   We should probably have this in Bugzilla::User, so it will be easier to
> update if we ever create more "responsibilities" for the user.

Bug 284273.

> >+    $vars->{'permissions'} = $dbh->selectall_hashref(
> >+        qq{SELECT id,
> 
>   WHOA! That's a huge thing... Are you doing this all just to get the counts of
> these different things? 
> 
>   Oh, I see, you're doing it to display how the person got their permissions.
> 
>   I think it would be easier to add a groups_direct and bless_groups_direct
> method to Bugzilla::User and then iterate over how they are different from
> groups and bless_groups. That would tell us what we inherited.

I'll keep it here for the moment.

> >+ul.warningmessages {
> 
>   Nit: I'm not totally sure we need the "ul" on that.

It's not needed, but I wanted it to be there -- this is a style for warning
message lists.

> >+    background-color: white;
> >+    border-style: solid;
> >+    border-width: 1px;
> >+    border-color: yellow;
> >+    padding: 1ex 1ex 1ex 4ex;
> 
>   Nit: I think "em" is more reliable, usually.

What do you mean by reliable? An em is an m-width, an ex is an x-height. Both
should be equally reliable.

> >+table.main {
> >+    border-spacing: 1em;
> 
>   I'm not totally sure that .main is a good class name... is this going to be
> standard admin-template CSS? If so, it would be nice to have it in
> an admin.css file.

This is just editusers.css. Please file a new bug if you feel the necessity for
a change.

Comment 3

[Max Kanat-Alexander]

> This cannot be handled by locking because we can't lock a table across page 
> views.

  In that case, you just have to re-do the checks that you did on confirm-delete
inside the actual delete block, inside of a lock.

> I'll keep it here for the moment.

  OK. Although please do file bugs for the change. They don't need to go in by
2.20, but I'm pretty afraid of what will happen to that huge chunk of SQL if we
ever make changes to the groups system.

> >   Nit: I think "em" is more reliable, usually.
> What do you mean by reliable? An em is an m-width, an ex is an x-height. Both
> should be equally reliable.

  I mean it behaves better cross-browser.

Comment 4

[Marc Schumann [:Wurblzap]]

(In reply to comment #3)
> > This cannot be handled by locking because we can't lock a table across page 
> > views.
> 
>   In that case, you just have to re-do the checks that you did on confirm-delete
> inside the actual delete block, inside of a lock.

The point is that even if we're doing checks now, the admin *has already agreed
to delete the user in spite of all warnings*. On the first page, we said "the
user has commented 4 times on bugs, do you want to delete anyway?". After
checking the comments thoroughly (cough) the admin decides to go ahead anyway.
The case that's difficult to catch now is the case when the user has added a
fifth comment in the meantime. We cannot lock accross the
confirm-delete.html.tmpl page view and the deleted.html.tmpl page view.

> > I'll keep it here for the moment.
> 
>   OK. Although please do file bugs for the change. They don't need to go in by
> 2.20, but I'm pretty afraid of what will happen to that huge chunk of SQL if
> we ever make changes to the groups system.

Looking at that particular part of the code, I think it should stay... I'm
reluctant to give up optimized SELECTs for perl workarounds... Maybe I'm not
entirely clear on your intentions. If you really want the change, please file a
bug stating your goals.

> > >   Nit: I think "em" is more reliable, usually.
> > What do you mean by reliable? An em is an m-width, an ex is an x-height.
> > Both should be equally reliable.
> 
>   I mean it behaves better cross-browser.

Can you give an example?
In that place, it's just a more or less random spacer between the yellow warning
frame and its content. Feel free to change it, though, if you want.

Comment 5

[Max Kanat-Alexander]

(In reply to comment #4)
> The point is that even if we're doing checks now, the admin *has already 
> agreed to delete the user in spite of all warnings*.

[snip]

  OK, agreed.

> Looking at that particular part of the code, I think it should stay... I'm
> reluctant to give up optimized SELECTs for perl workarounds...

  OK, I've filed bug 296146 in response.

> >   I mean it behaves better cross-browser.
> 
> Can you give an example?

  No, actually, I just recall that some browsers like em but don't like ex. I'm
not sure which, though -- it's been a while. I always use em, though.

Comment 6

[Marc Schumann [:Wurblzap]]

Bug 284267 and bug 296146 have stopped really qualifying as post-checkin cleanup bugs, I think.