Closed Bug 119485 Opened 18 years ago Closed 15 years ago

Templatise editusers.cgi

Categories

(Bugzilla :: Administration, task)

2.15
task
Not set

Tracking

()

RESOLVED FIXED
Bugzilla 2.20

People

(Reporter: dkl, Assigned: Wurblzap)

References

(Blocks 2 open bugs)

Details

Attachments

(2 files, 8 obsolete files)

I have this almost finished except for the edit portion of an existing account. Patch 
coming soon for review.

There was talk of combining all of the edit*.cgi in to a single cgi but I am not sure 
how far along this is and we wanted to have as much templatised for 2.16 as 
possible.
Oops, we call our installation editmembers.cgi here at RH with version 2.8. 
Changing to proper editusers.cgi for this report.
Summary: Templatise editmembers.cgi → Templatise editusers.cgi
editusers.cgi wasn't a part of the initial unification drive.  Also admin CGIs
weren't planned for templatisation for 2.16.
I understand that now. It was mainly as a combination head start on the admin cgi's 
for later and also a teaching vehicle for me to learn the new templating language.
I have the work done anyhow in editusers.cgi and will be uploading a patch soonish.
Setting target for 2.18. Should the component be user accounts or user interface?
Target Milestone: --- → Bugzilla 2.18
No idea, but the template locations for consistency with what I've done should
be at "admin/users".  Not sure about the filenames exactly these days but they
should start with create, list and edit.
Okay, If I undertstand correctly, I will create a new directory called users under admin
and place my current templates there before attaching my patch.
Depends on: 92905
The User Interface component now belongs to Gerv.  Reassigning all UNCONFIRMED
and NEW (but not ASSIGNED) bugs currently owned by Myk (the previous component
owner) to Gerv.
Assignee: myk → gerv
Reassigning back to Myk.  That stuff about Gerv taking over the User Interface
component turned out to be short-lived.  Please pardon our confusion, and I'm
very sorry about the spam.
Assignee: gerv → myk
dkl: Do you still have a patch doing (most of) this sitting around somewhere? 
I'm going to try to pick up templatizing the admin cgis, and moving bits and 
pieces around as necessary. Even a rotted patch would be useful as a starting 
point.

Thanks.
Moving this to the product all the other "templatize admin page" bugs are in.
Assignee: myk → justdave
Component: User Interface → Administration
Enhancements which don't currently have patches on them which are targetted at
2.18 are being retargetted to 2.20 because we're about to freeze for 2.18. 
Consideration will be taken for moving items back to 2.18 on a case-by-case
basis (but is unlikely for enhancements)
Target Milestone: Bugzilla 2.18 → Bugzilla 2.20
All admin-page templatization bugs: Note that just-fixed bug 244265 implemented
a basic generic structure for writing out those typical
product/milestone/whatever selection tables that exist on most of the admin
pages. If you templatize one of those, please use the generic version. If it
doesn't have all the features you need, let's discuss expanding it with new
features.
*** Bug 254024 has been marked as a duplicate of this bug. ***
Bugzilla 2.20 feature set is now frozen as of 15 Sept 2004.  Anything flagged
enhancement that hasn't already landed is being pushed out.  If this bug is
otherwise ready to land, we'll handle it on a case-by-case basis, please set the
blocking2.20 flag to '?' if you think it qualifies.
Target Milestone: Bugzilla 2.20 → Bugzilla 2.22
Attached patch Pre-Alpha (obsolete) — Splinter Review
I've been toying around with this and would like to take it.
There is a preview attached.
Assignee: justdave → wurblzap
OS: Linux → All
Hardware: Other → All
nit: Netscape Communications Corporation no longer exists, and those new files
you're adding didn't exist in 1998. :)

On the other hand, if you stole substantial chunks of code from another file
that bore that copyright, it's probably okay (but seeing as how we didn't have
any templates until late 2001 or so, that's pretty unlikely).  
Attached patch Complete rewrite (obsolete) — Splinter Review
Here we go.
I didn't validate the html yet.
Attachment #160620 - Attachment is obsolete: true
Attachment #160884 - Flags: review?
Let's see this in 2.20, shall we?
Flags: blocking2.20?
Are you going to check the HTML still, though?
Sure.
I wouldn't enjoy throwing away polished HTML, though, so I prefer doing the
polishing after positive functionality and code review...
Status: NEW → ASSIGNED
Blocks: 265898
No longer blocks: 265898
Depends on: 265898
Flags: blocking2.20? → blocking2.20-
Attached patch Polished patch (obsolete) — Splinter Review
Works, is friends with the w3 validator, can inconsistency-awarely delete users
and fixes bug 264583.
Attachment #160884 - Attachment is obsolete: true
Attachment #169177 - Flags: review?
Attachment #160884 - Flags: review?
Marc: just to let you know -- I'm having a go at reviewing this, hope to get
back to you in a few days with some comments
Attached patch Fought the rot (obsolete) — Splinter Review
Woooo! Now if this doesn't deserve a good unrotting, then I don't know what
does.
Attachment #169177 - Attachment is obsolete: true
Attachment #171123 - Flags: review?
Attachment #169177 - Flags: review?
Comment on attachment 171123 [details] [diff] [review]
Fought the rot

OK, here it is, my first review of any length, so all should be
fore-warned....

General Issues
--------------------
Should there be some sort of common footer template?

Should there be more <label> tags? (i.e. on all form items)

Can (some of) the css be moved to an admin.css, and shared with the
other admin templates?

Some INTERFACE comments are missing, and some are incomplete.

And a brief comment on the helper routines in editusers.cgi would be
good as well

I don't think the link in the footer back to the index is needed, as
there is a 'Home' link in the full BZ footer these days

Your 2 helper templates (listselectvars and userdata) should be named
XXX.none.tmpl I think instead of XXX.html.tmpl, as they don't output
anything - is that correct?


user-error template
--------------------
Typo in error msg: neiter -> neither


userdata.html.tmpl:
--------------------
The 'oldvalues' parameter is used for some additional password
description text, as well as hidden fields, so perhaps there is a
better name??


search.html.tmpl
--------------------
<input type=submit... - add quotes around submit

If there is only one group - could we do away with the dropdown?


listselectvars.html.tmpl
--------------------
hidden fields should html_quoted instead of url_quoted


list.html.tmpl
--------------------
columns.push better than .2?

should we allow html content in user list for login and real name? I
don't think so

do we use &nbsp; for empty table cells???

why print number of users found twice?


edit.html.tmpl
--------------------
Should the message after a creation or update be done with standard bz
message system?

I think we should say what has been updated.

And it should not say that the user was updated, when nothing changed.

'You may want to edit the group settings now below': the 'now below'
sounds wrong, perhaps: '...settings as shown below', or ...settings
now, using the form below'

What is the empty directive for:    [% %]<input

Is it OK to do the group background color line only with css?


deleted.html.tmpl
--------------------
I think we should say which user was deleted?


confirm-delete.html.tmpl
--------------------
In the editcomponents.cgi links, it is nicer and slightly more
consistent with other parts of BZ if the 'action=edit' part is the
first url param

'peculiar user account' -> particular?

This template says that the bugs will not appear in buglists any more
if a user is deleted, but the bugs will still appear, won't they? (We
will just have the db consistency problems mentioned)


editusers.cgi
--------------------
I'm not sure if all the old contributers should be removed, though I
will concede that it is a pretty complete re-write

It's more usual to not have the brackets on Bugzilla->dbh()

What about a multi-select for groups when searching for users? (Maybe
another bug)

Spaces between comma and columns in SELECTs

regexp is not cross db compatible (not something you've added, but is
there a better way?) (another bug maybe)

What is that Slice parameter for in most of the DB calls? I can see
it's need for some, but not in all cases. Also, why use {} for 2nd
param in dbi calls - is that better, worse, or just an alternative to
'undef' when we have no attrs to set.

Comments on trick_taints?

Could userids be detainted using detaint_natural?

Should we LOCK TABLES when deleting?

You check flag setter id, but don't actually do any deletion or
updating regarding the flag setter?

You have an UPDATE in your 'simple deletions' section

listOfVisibleGroups returns a string - maybe call
visible_groups_as_string or VisibleGroupsAsString?

If the action is unknown, it shouldn't be a code error should it?

Is DISTINCT needed on the user select SQL?

Why was visibility groups support not in editusers before - oversight?
intentional?

CanSeeUser: is this useful enough to go into Bugzilla::User.pm? You
should also comment that this routine also checks that the user
exists, as well as checking we can see the user, and so replaces the
old calls to CheckUser()

If we delete tokens when changing login_name - we should specify the
token type, shouldn't we??

Should we use the Token.pm calls to remove tokens -
DeletePasswordTokens()

We used to logout user when disabled text or password changed, but we
don't seem to now - oversight?

What about profiles activity updates (at least for groups)

And refreshed_when field in profiles?

For the last select in userDataToVars(), I think the SQL is easier to
read with the 'table_name.' prefixes - can they return?) (Also for
some other selects)

In productresponsibilities: the 'IN' syntax is new on me - is that
valid/safe/crossdb safe sql:
>             WHERE products.id = product_id
>                AND ? IN (initialowner, initialqacontact)

productresponsibilities should be called product_responsibilities or
productResponsibilities?

If there are, say less than 20 users in a system, why not just list
them all immediately?

You delete from the cc table, but don't check or warn about the user
being cc'd on a bug - intentional? If so comment maybe? (also for
namedqueries and votes)

You delete the profiles entry before all the other rows from other
tables have been removed. mysql is OK because we have no proper
referential integrity, but in a DB with proper foreign key referential
integrity checks, we would have problems here, wouldn't we?

We should get Gerv to check series deletion, Joel to check group
issues, and the whine creater (Eric?) to check whine deletions.



------------------------------------------------------------ 

For my sake, and documentation purposes, and for the eventual approver
we need a list of the new functionality which is included in this
templatisation: (here is my take on this list, and some may change
subject to responses to my questions above:)

1) usevisibility groups used

2) can search on group membership

3) removes tokens if a login name changes (should just remove login
   change tokens?)

4) better warning on cross references for a user when deleting

5) Lots more stuff deleted on a user deletion
Attachment #171123 - Flags: review? → review-
Blocks: 248993
Blocks: 121386
Blocks: 163890
Blocks: 194750
Blocks: 236396
Blocks: 237683
Attached patch Patch 1.2 (obsolete) — Splinter Review
(In reply to comment #23)

Gavin, thank you very much for your detailed review! Here is a new patch,
addressing your comments.

> Should there be some sort of common footer template?

I was thinking about this, but I like the individually crafted ones better.
I'll do one if you insist :)

> Should there be more <label> tags? (i.e. on all form items)

Did that.

> Can (some of) the css be moved to an admin.css, and shared with the
> other admin templates?

This can be done, of course, but there isn't much worth sharing besides
maybe warningmessages.
There is no other admin template set currently; how about leaving
it up to the next one doing one to pull out what may be shared?

> Some INTERFACE comments are missing, and some are incomplete.

All set up now.

> And a brief comment on the helper routines in editusers.cgi would be
> good as well

Done.

> I don't think the link in the footer back to the index is needed, as
> there is a 'Home' link in the full BZ footer these days

Gone.

> Your 2 helper templates (listselectvars and userdata) should be named
> XXX.none.tmpl I think instead of XXX.html.tmpl, as they don't output
> anything - is that correct?

I don't think so... They create HTML, after all. I may be wrong, though.

> Typo in error msg: neiter -> neither

Corrected.

> The 'oldvalues' parameter is used for some additional password
> description text, as well as hidden fields, so perhaps there is a
> better name??

This critter is called editform now -- if set, it's an edit form, if unset,
it's a create form.

> <input type=submit... - add quotes around submit

Done.

> If there is only one group - could we do away with the dropdown?

We could. Can we do this in another bug?

> hidden fields should html_quoted instead of url_quoted

Right. Done.

> columns.push better than .2?

True. I corrected several .0 checks into .size checks, too.

> should we allow html content in user list for login and real name? I
> don't think so

Good catch; the login and the real name should definitely be filtered.
I'm doing so outside from admin/table.html.tmpl now. We cannot have it
filter for us because we need HTML to style the content (for disabled
users and for missing real names), so allow_html_content => 1 needs to stay.

> do we use &nbsp; for empty table cells???

Replaced by a red 'missing'.

> why print number of users found twice?

Because buglist.cgi does so.
I've removed the one at the bottom now.

> Should the message after a creation or update be done with standard bz
> message system?

Yes. It does now. The deleted.html.tmpl template could go during this change.

> I think we should say what has been updated.

It does so now.

> And it should not say that the user was updated, when nothing changed.

It says "you didn't request any change" now.

> 'You may want to edit the group settings now below': the 'now below'
> sounds wrong, perhaps: '...settings as shown below', or ...settings
> now, using the form below'

It says so now.

> What is the empty directive for:    [% %]<input

This concatenates the line to the previous line, so that there is no
whitespace. It's ugly if brackets or asterisks around the checkbox wrap to
another line.

> Is it OK to do the group background color line only with css?

Yes, the official Bugzilla way to go is CSS.

> I think we should say which user was deleted?

Doing so now (in messages.html.tmpl).

> In the editcomponents.cgi links, it is nicer and slightly more
> consistent with other parts of BZ if the 'action=edit' part is the
> first url param

Done.

> 'peculiar user account' -> particular?

Of course. Reworded the warnings.

> This template says that the bugs will not appear in buglists any more
> if a user is deleted, but the bugs will still appear, won't they? (We
> will just have the db consistency problems mentioned)

The warnings reflect what happened when I tried.

> I'm not sure if all the old contributers should be removed, though I
> will concede that it is a pretty complete re-write

One of the reasons to do a full re-write was to get a current copyright notice.


> It's more usual to not have the brackets on Bugzilla->dbh()

Did that. I don't like it, though.

> What about a multi-select for groups when searching for users? (Maybe
> another bug)

Yes please, another bug :)

> Spaces between comma and columns in SELECTs

Corrected.

> regexp is not cross db compatible (not something you've added, but is
> there a better way?) (another bug maybe)

Another bug, please :)

> What is that Slice parameter for in most of the DB calls? I can see
> it's need for some, but not in all cases. Also, why use {} for 2nd
> param in dbi calls - is that better, worse, or just an alternative to
> 'undef' when we have no attrs to set.

I removed 'Slice' where not needed, and I replaced {} by undef (they may be
used alternatively, and I think undef is more readable).

> Comments on trick_taints?

Done.
I use (a slightly enhanced) InsertNewUser from globals.pl now, too.

> Could userids be detainted using detaint_natural?

I'm using Bugzilla::User now.

> Should we LOCK TABLES when deleting?

I think so; doing so now. On updates, too.
Fixing Bugzilla::Error.pm and CGI.pl along the way (came across these during
testing).
Added a FIXME code comment about the race condition at concurrent
delete/update.

> You check flag setter id, but don't actually do any deletion or
> updating regarding the flag setter?

If the user is a flag setter, I'm issuing a warning message saying that
deleting the user causes inconsistencies.

> You have an UPDATE in your 'simple deletions' section

Labeled that 'Reference removals'.

> listOfVisibleGroups returns a string - maybe call
> visible_groups_as_string or VisibleGroupsAsString?

Good idea. Done.

> If the action is unknown, it shouldn't be a code error should it?

If a regular user stumbles across an unknown action, (s)he has imo indeed
uncovered a code error. The idea is to have user errors for errors the user
may reach *without* hacking :)
Other code throw code errors, too; I'm recycling an existing error message,
after all.

> Is DISTINCT needed on the user select SQL?

Yes, because of possibly SELECTing from a second table (user_group_map),
creating the cross product.

> Why was visibility groups support not in editusers before - oversight?
> intentional?

I'd say this was an oversight. We're better off supporting it here. If we
wish for full visibility to persons, we can use editgroups.cgi to give it to
them.

> CanSeeUser: is this useful enough to go into Bugzilla::User.pm? You

Maybe. Another bug?

> should also comment that this routine also checks that the user
> exists, as well as checking we can see the user, and so replaces the
> old calls to CheckUser()

Did that.

> If we delete tokens when changing login_name - we should specify the
> token type, shouldn't we??

Well, this is no technical problem -- but which token types should I refrain
from deleting? Shouldn't I delete all?

> Should we use the Token.pm calls to remove tokens -
> DeletePasswordTokens()

This deletes password tokens only. Moreover, it calls Bugzilla::Token::Cancel,
which sends e-mail to the affected user and the Bugzilla maintainer. I don't
think we should do that.

> We used to logout user when disabled text or password changed, but we
> don't seem to now - oversight?

Oversight. Included now.

> What about profiles activity updates (at least for groups)

Same here. That's what I get for doing a re-write.
Added FIXME code comments on lacking profiles_activity updates.

> And refreshed_when field in profiles?

Same here.

> For the last select in userDataToVars(), I think the SQL is easier to
> read with the 'table_name.' prefixes - can they return?) (Also for
> some other selects)

I put these in now for most SELECTs from more than one table.

> In productresponsibilities: the 'IN' syntax is new on me - is that
> valid/safe/crossdb safe sql:
> >		WHERE products.id = product_id
> >		   AND ? IN (initialowner, initialqacontact)

It is, afaik. Second-review?

> productresponsibilities should be called product_responsibilities or
> productResponsibilities?

Renamed.

> If there are, say less than 20 users in a system, why not just list
> them all immediately?

Another bug?

> You delete from the cc table, but don't check or warn about the user
> being cc'd on a bug - intentional? If so comment maybe? (also for
> namedqueries and votes)

I juggled that about in my mind for a while. I used to follow my self-imposed
rule not to show warnings for tables not causing inconsistencies. I'm warning
on everything now, and I like it better this way :)

> You delete the profiles entry before all the other rows from other
> tables have been removed. mysql is OK because we have no proper
> referential integrity, but in a DB with proper foreign key referential
> integrity checks, we would have problems here, wouldn't we?

For sure. The profiles entry is the last removal now.

> We should get Gerv to check series deletion, Joel to check group
> issues, and the whine creater (Eric?) to check whine deletions.

Feel free to ask for second reviews on that :)
Attachment #171123 - Attachment is obsolete: true
Attachment #172436 - Flags: review?(bugzilla)
Depends on: 280124
Marc: Just to let you know, I am doing the review for this now, and I'll get it
back to you in a day or 2
Blocks: 194355
Comment on attachment 172436 [details] [diff] [review]
Patch 1.2

These are all pretty minor, so next version should be a winner....


Bugzilla/Error.pm
------------------------------
Can these be fixed in another bug please? 

InsertNewUser() changes
------------------------------
now called insert_new_user() (because I took so long to review, sorry :(

I think we need to add |$disabledtext ||= ''| to it

Need to add trick_taints for the new params as well

And the perldoc for insert_new_user() needs updating

And the prototype need updating


Other changes:
------------------------------

Crypt() has become bz_crypt(), and ValidateNewUser() has also changed
now. Apologies for taking so long. Any future review will be quicker -
honest.

Some of your id='' fields for the <labels> were added to <td>
elements - I think they need to be on the actual form control, don't they?
(e.g. for password and disabledtext form fields)

Some items are still missing from the INTERFACE
comments. e.g. confirm-delete.html.tmpl: cc, namedqueries,
profilesactivity, votes, watch.watcher watch.watched. edit.html.tmpl:
disabledtext, permissions, message (I haven't checked all the others)

There are still some <input> tags which don't end />, and you seem to
be trying to make them all consistent

Sometimes the unused middle parameter to ThrowUserError() is {}, and
sometimes undef - should be consistent within this patch. There does
not seem to be a Bugzilla standard.

messages:
------------------------------
'granted _rights to_ bless'?

Nice to have what the realname changed to, or the disabled text, but
I'm not saying do it, just noting for any future improvements

editusers.cgi
------------------------------
Still a bit of debug code in there: 'if (0) ...'

Should the first ThrowUserError() in the 'update' block specify the
3rd abort param, as it is after the LOCK TABLES?


confirm-delete.html.tmpl
------------------------------

'resulting in this bug to not appear' -> 'resulting in this bug not
appearing'. However, when you say that this bug will not appear in bug
lists anymore - do you mean stored queries with the username as part
of the stored query? The bug a deleted user reported still appears for
me, but the reporter field is empty, and an httpd error is
reported. [That is not something you have added, but we should try and
resolve that later.] Anyway, could you confirm what you mean by 'not
appear in bug lists'?

'comment to be not visible anymore' -> 'comment not appearing any
more', or 'comment being lost'...

'Please be aware of the consequences' should be in red, and/or bold,
and or bigger.
Attachment #172436 - Flags: review?(bugzilla) → review-
(In reply to comment #26)
> confirm-delete.html.tmpl
> ---------------------------- 
> 'resulting in this bug to not appear' -> 'resulting in this bug not
> appearing'. [...] The bug a deleted user reported still appears for
> me, but the reporter field is empty, and an httpd error is
> reported.

Marc, ignore what I said here, you are correct, the bug does not appear in
buglists, so your message is (almost) correct. We should investigate why and
explain this (in another bug) -- I suspect it is to do with the JOIN type in
Bugzilla::Search...
Blocks: 282446
Attached patch Patch 1.3 (obsolete) — Splinter Review
Thank you for your review :)

>Bugzilla/Error.pm
>Can these be fixed in another bug please? 

Sure. Let's not forget it :)

>I think we need to add |$disabledtext ||= ''| to it

Yes; better safe than sorry.

>Need to add trick_taints for the new params as well

I don't think so. We should untaint where the validation happens.
That way, we can be sure that only validated data gets to touch the database
or gets to be sent to the user's browser.
I added untainting commands in editusers.cgi.

>And the perldoc for insert_new_user() needs updating
>And the prototype need updating

Done and done.

>Crypt() has become bz_crypt(), and ValidateNewUser() has also changed

Taken into account.

>Some of your id='' fields for the <labels> were added to <td>

Very true. I don't know what got into me.

>Some items are still missing from the INTERFACE

I hope I got all. The edit.html.tmpl ones besides message are contained in
otheruser, which is a Bugzilla::User object and which imho shouldn't be
documented here.

>There are still some <input> tags which don't end />, and you seem to
>be trying to make them all consistent

You're right. Thank you for catching these for me :)

>Sometimes the unused middle parameter to ThrowUserError() is {}, and
>sometimes undef - should be consistent within this patch. There does

Made all of these to say undef.

>messages:
>'granted _rights to_ bless'?

It did say so once, in fact. For a short time -- it seemed to verbose too me.
It's saying so again (in editusers.cgi, too).

>Nice to have what the realname changed to, or the disabled text, but
>I'm not saying do it, just noting for any future improvements

I think displaying the realname is a good idea (and I think it's an even
better idea to do it in a new bug). I disagree on the disable text, though --
it may be long and is likely to clutter the message box.

>editusers.cgi
>Still a bit of debug code in there: 'if (0) ...'

Gone.

>Should the first ThrowUserError() in the 'update' block specify the
>3rd abort param, as it is after the LOCK TABLES?

Good catch.

>confirm-delete.html.tmpl
>'resulting in this bug to not appear' -> 'resulting in this bug not

Thanks, done. The similar ones, too.

>'Please be aware of the consequences' should be in red, and/or bold,
>and or bigger.

It's now in red, 120% size, and bold.
I put the style of it into admin.css, too, taking the warningmessages style
with it.
Attachment #172436 - Attachment is obsolete: true
Attachment #174667 - Flags: review?(bugzilla)
Just a quick note to mention that the 'LOCK TABLES' will need to be updated to
use the new cross-DB locking calls. I'll try and do the review this week-end
Attached patch Patch 1.4 (obsolete) — Splinter Review
- De-rotted
- bz_lock_tables+bz_unlock_tables
Attachment #174667 - Attachment is obsolete: true
Attachment #175148 - Flags: review?(bugzilla)
Attachment #174667 - Flags: review?(bugzilla)
Comment on attachment 175148 [details] [diff] [review]
Patch 1.4

1) 'case sensitive' regexp is not case sensitive (not what you added,
   but noting so that I can come back to this)

2) 'granted _rights_ to bless' -- sorry about the misundertsanding,
   but I was referring to the template output text, and not the
   variable names :(

3) disabled text is not displayed, or updated properly. (partially
   because it is not part of the otheruser object in
   userdata.html.tmpl, I think)

4) mailto_userid in whine_schedules has changed (to mailto, I think)
Attachment #175148 - Flags: review?(bugzilla) → review-
Attached patch Patch 1.5Splinter Review
> 1) 'case sensitive' regexp is not case sensitive (not what you added,
>    but noting so that I can come back to this)

MySQL docs say that's as of MySQL version 3.23.4 -- the original is probably
older then. The text in the dropdown box now says 'case insensitive'.

> 2) 'granted _rights_ to bless' -- sorry about the misundertsanding,
>    but I was referring to the template output text, and not the
>    variable names :(

Done.

> 3) disabled text is not displayed, or updated properly. (partially
>    because it is not part of the otheruser object in
>    userdata.html.tmpl, I think)

Right. Made the Bugzilla::User class aware of disabledtext.

> 4) mailto_userid in whine_schedules has changed (to mailto, I think)

Taken that into account.
Attachment #175148 - Attachment is obsolete: true
Attachment #175425 - Flags: review?(bugzilla)
Comment on attachment 175425 [details] [diff] [review]
Patch 1.5

Great!
Attachment #175425 - Flags: review?(bugzilla) → review+
Note to approver (and for documentation):

This patch adds the following functionality, as well as the actual templatisation:

1) usevisibility groups used
2) can search on group membership
3) removes tokens if a login name changes
4) better warning on cross references for a user when deleting
5) Lots more stuff deleted on a user deletion
Flags: approval?
Comment on attachment 175425 [details] [diff] [review]
Patch 1.5

I'd like a second review on this just because of the magnitude of the new
features being added at the same time.	(I'm drooling over this though ;)
Attachment #175425 - Flags: review?
Flags: approval?
Target Milestone: Bugzilla 2.22 → Bugzilla 2.20
Comment on attachment 175425 [details] [diff] [review]
Patch 1.5


  OK, *everything* below is a nit. However, there's a lot of them, so I'd
appreciate if you could make a new bug out of this comment, and fix these
things after we check this in.

  The only thing that might not be a nit is my comment about refreshed_when --
could you respond to that before we check this in?

>+=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'}; }

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

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

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

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

>-# 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 should just say your name.

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

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

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

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

>+    $vars->{'restrictablegroups'} = groupsUserMayBless($user, 'id', 'name');

  That duplicates Bugzilla->user->bless_groups, I'm pretty sure, since I
checked that code in.

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

>+    canSeeUser($otherUserID)

  Nit: That really should be a method of Bugzilla::User, I'd think.

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

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

  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.

>+            # FIXME: should create profiles_activity entries.

  *cough* :-)

  Really, we need "Bugzilla::User should be able to write to the database."
That would solve a lot of this.

>+    # FIXME: checking for existence of each user_group_map entry
>+    #        would allow to display a friendlier error message on page reloads.

  And that, too.

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

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

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

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

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

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

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

>+    $user->derive_groups(1);

  That should never be necesary, by the way, unless you've changed the groups
after you've created the User.

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

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

>+# Retrieve user data for the user editing form. User creation and user
>+# editing code rely on this to call derive_groups().
>+sub userDataToVars {
>+    my $userid = shift;
>+    my $user = new Bugzilla::User($userid);
>+    my $query;
>+
>+    $user->derive_groups();

  As I mentioned above, when you made a new user, it already called
derive_groups if that was necessary...

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

>+ul.warningmessages {

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

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

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


  I'm not going to look over the templates; if they have problems I'm sure
we'll catch them.
Attachment #175425 - Flags: review? → review+
Blocks: 283937
Flags: approval?
(In reply to comment #36)

Added bug 283937 for this.
Flags: approval? → approval+
If we have individual bugs for the actual features that were added with this,
the docs request can be moved to those I suppose if it makes things any easier.
Flags: documentation?
A massive, much-needed patch. Way to go, Marc!

Checking in CGI.pl;
/cvsroot/mozilla/webtools/bugzilla/CGI.pl,v  <--  CGI.pl
new revision: 1.230; previous revision: 1.229
done
Checking in defparams.pl;
/cvsroot/mozilla/webtools/bugzilla/defparams.pl,v  <--  defparams.pl
new revision: 1.149; previous revision: 1.148
done
Checking in editusers.cgi;
/cvsroot/mozilla/webtools/bugzilla/editusers.cgi,v  <--  editusers.cgi
new revision: 1.79; previous revision: 1.78
done
Checking in globals.pl;
/cvsroot/mozilla/webtools/bugzilla/globals.pl,v  <--  globals.pl
new revision: 1.311; previous revision: 1.310
done
Checking in Bugzilla/User.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/User.pm,v  <--  User.pm
new revision: 1.41; previous revision: 1.40
done
RCS file: /cvsroot/mozilla/webtools/bugzilla/skins/standard/admin.css,v
done
Checking in skins/standard/admin.css;
/cvsroot/mozilla/webtools/bugzilla/skins/standard/admin.css,v  <--  admin.css
initial revision: 1.1
done
RCS file: /cvsroot/mozilla/webtools/bugzilla/skins/standard/editusers.css,v
done
Checking in skins/standard/editusers.css;
/cvsroot/mozilla/webtools/bugzilla/skins/standard/editusers.css,v  <--  
editusers.css
initial revision: 1.1
done
Checking in template/en/default/filterexceptions.pl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/filterexceptions.pl,v  <-
-  filterexceptions.pl
new revision: 1.36; previous revision: 1.35
done
Checking in template/en/default/admin/groups/delete.html.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/admin/groups/delete.html.
tmpl,v  <--  delete.html.tmpl
new revision: 1.4; previous revision: 1.3
done
RCS 
file: /cvsroot/mozilla/webtools/bugzilla/template/en/default/admin/users/confirm
-delete.html.tmpl,v
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
initial revision: 1.1
done
RCS 
file: /cvsroot/mozilla/webtools/bugzilla/template/en/default/admin/users/create.
html.tmpl,v
done
Checking in template/en/default/admin/users/create.html.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/admin/users/create.html.t
mpl,v  <--  create.html.tmpl
initial revision: 1.1
done
RCS 
file: /cvsroot/mozilla/webtools/bugzilla/template/en/default/admin/users/edit.ht
ml.tmpl,v
done
Checking in template/en/default/admin/users/edit.html.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/admin/users/edit.html.tmp
l,v  <--  edit.html.tmpl
initial revision: 1.1
done
RCS 
file: /cvsroot/mozilla/webtools/bugzilla/template/en/default/admin/users/list.ht
ml.tmpl,v
done
Checking in template/en/default/admin/users/list.html.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/admin/users/list.html.tmp
l,v  <--  list.html.tmpl
initial revision: 1.1
done
RCS 
file: /cvsroot/mozilla/webtools/bugzilla/template/en/default/admin/users/listsel
ectvars.html.tmpl,v
done
Checking in template/en/default/admin/users/listselectvars.html.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/admin/users/listselectvar
s.html.tmpl,v  <--  listselectvars.html.tmpl
initial revision: 1.1
done
RCS 
file: /cvsroot/mozilla/webtools/bugzilla/template/en/default/admin/users/search.
html.tmpl,v
done
Checking in template/en/default/admin/users/search.html.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/admin/users/search.html.t
mpl,v  <--  search.html.tmpl
initial revision: 1.1
done
RCS 
file: /cvsroot/mozilla/webtools/bugzilla/template/en/default/admin/users/userdat
a.html.tmpl,v
done
Checking in template/en/default/admin/users/userdata.html.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/admin/users/userdata.html
.tmpl,v  <--  userdata.html.tmpl
initial revision: 1.1
done
Checking in template/en/default/global/code-error.html.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/global/code-
error.html.tmpl,v  <--  code-error.html.tmpl
new revision: 1.45; previous revision: 1.44
done
Checking in template/en/default/global/messages.html.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/global/messages.html.tmpl
,v  <--  messages.html.tmpl
new revision: 1.25; previous revision: 1.24
done
Checking in template/en/default/global/user-error.html.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/global/user-
error.html.tmpl,v  <--  user-error.html.tmpl
new revision: 1.98; previous revision: 1.97
done
Status: ASSIGNED → RESOLVED
Closed: 15 years ago
Resolution: --- → FIXED
Thanks to Gavin and you all for making this possible :)

Next stop: bug 283937 :)
Attached patch documentation patch, v1 (obsolete) — Splinter Review
Attachment #246215 - Flags: review?(documentation)
Note to self: on checkin, remove the "Disable Bugmail" part on branches + the "exact (find this user)" sentence for 2.20. Everything else can stay.
Comment on attachment 246215 [details] [diff] [review]
documentation patch, v1

>+++ docs/xml/administration.xml	21 Nov 2006 23:05:49 -0000
>@@ -339,9 +339,9 @@
>         <para>If you wish to add more administrative users, add them to 
>-        the "admin" group and, optionally, add edit the tweakparams, editusers,
>+        the "admin" group and, optionally, and edit the tweakparams, editusers,
>         creategroups, editcomponents, and editkeywords groups to add the
>-        entire admin group to those groups.
>+        entire admin group to those groups (which is the case by default).

The word you modified should just be removed.  It doesn't make any sense either way. (The "and" in question is already given right after the word "group" so the one before "edit" is redundant, and the "add" didn't make sense anyway.

>@@ -349,62 +349,90 @@
>+          box. You can match by case-insensitive substring (the default),
>+          regular expression, a <emphasis>reverse</emphasis> regular expression
>+          match, which finds every user name which does NOT match the regular
>+          expression (please see the <command>man regexp</command> manual page
>+          for details on regular expression syntax) or the exact string if you
>+          know exactly who you are looking for.

We should probably find a web-based regexp reference, or point that at "perldoc perlre" instead, and or the database regexp docs for the databases, or?
Attachment #246215 - Flags: review?(documentation) → review-
Attachment #246215 - Attachment is obsolete: true
Attachment #246222 - Flags: review?(documentation)
Attachment #246222 - Flags: review?(documentation) → review+
tip:

Checking in docs/xml/administration.xml;
/cvsroot/mozilla/webtools/bugzilla/docs/xml/administration.xml,v  <--  administration.xml
new revision: 1.66; previous revision: 1.65
done

2.22.1:

Checking in docs/xml/administration.xml;
/cvsroot/mozilla/webtools/bugzilla/docs/xml/administration.xml,v  <--  administration.xml
new revision: 1.55.2.7; previous revision: 1.55.2.6
done

2.20.3:

Checking in docs/xml/administration.xml;
/cvsroot/mozilla/webtools/bugzilla/docs/xml/administration.xml,v  <--  administration.xml
new revision: 1.50.2.10; previous revision: 1.50.2.9
done
Flags: documentation?
Flags: documentation2.22+
Flags: documentation2.20+
Flags: documentation+
QA Contact: matty_is_a_geek → default-qa
You need to log in before you can comment on or make changes to this bug.