Closed Bug 119485 Opened 23 years ago Closed 20 years ago

Templatise editusers.cgi

Categories

(Bugzilla :: Administration, task)

Product:
Bugzilla
Component:
Administration
Version:
2.15
Type:
task
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
Bugzilla 2.20

People

(Reporter: dkl, Assigned: Wurblzap)

References

(Blocks 2 open bugs)

Details

Attachments

(2 files, 8 obsolete files)

Patch 1.5
107.49 KB, patch
bugzilla
: review+
mkanat
: review+
Details | Diff | Splinter Review
documentation patch, v1.1
9.35 KB, patch
justdave
: review+
Details | Diff | Splinter Review
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
Blocks: 264583
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.5 — — Splinter 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: 20 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.

Attachment

General

Creator:
Created:
Updated:
Size: