Have Bugzilla::Group implement Bugzilla::Object->create, and make checksetup.pl use it

RESOLVED FIXED in Bugzilla 3.0

Status

()

Bugzilla
Installation & Upgrading
--
enhancement
RESOLVED FIXED
12 years ago
12 years ago

People

(Reporter: Max Kanat-Alexander, Assigned: Max Kanat-Alexander)

Tracking

2.23
Bugzilla 3.0
Dependency tree / graph
Bug Flags:
approval +

Details

Attachments

(1 attachment, 4 obsolete attachments)

(Assignee)

Description

12 years ago
Right now checksetup.pl has its own "AddGroup" function. Instead, there should be a more-useful "Bugzilla::Group->create" function, and we should use that.
(Assignee)

Comment 1

12 years ago
Created attachment 231344 [details] [diff] [review]
v1 (use -p1)

Okay, here's the patch. It touches Bugzilla::Groups pretty heavily, so I figure it needs review. (Since I don't own Groups.)

You don't need to review the checksetup part if you don't want to.

I copied RederiveRegexp exactly from editgroups.cgi.
Assignee: installation → mkanat
Status: NEW → ASSIGNED
Attachment #231344 - Flags: review?(LpSolit)
(Assignee)

Comment 2

12 years ago
I'm going to re-work this patch to use Bugzilla::Object->create instead.
Depends on: 339383, 347061
Summary: Create Bugzilla::Group->create and make checksetup.pl use it → Have Bugzilla::Group implement Bugzilla::Object->create, and make checksetup.pl use it
(Assignee)

Comment 3

12 years ago
Created attachment 231912 [details] [diff] [review]
v2 (use -p1)

Okay, here's the second version, which uses Bugzilla::Object->create.

In a sense, bug 339383, bug 347061, and this bug are all one bug, split up into three patches.
Attachment #231344 - Attachment is obsolete: true
Attachment #231912 - Flags: review?(bugzilla-mozilla)
Attachment #231344 - Flags: review?(LpSolit)

Comment 4

12 years ago
Comment on attachment 231912 [details] [diff] [review]
v2 (use -p1)

The checksetup.pl stuff did not apply.
Attachment #231912 - Flags: review?(bugzilla-mozilla) → review-
(Assignee)

Comment 5

12 years ago
Created attachment 234687 [details] [diff] [review]
v3

Ah, nice catch! :-) I also noticed (by running the code on Pg) that isbuggroup is a required field. I didn't want to set a default for it in the DB, because I want callers to set it explicitly (I don't want a bug in checksetup.pl to pop up in the future because of setting the isbuggroup DEFAULT to 1).

I also removed the setting of last_changed from Bugzilla::Group->create, since the column is gone now.
Attachment #231912 - Attachment is obsolete: true
Attachment #234687 - Flags: review?(bugzilla-mozilla)
(Assignee)

Comment 6

12 years ago
This patch is still awaiting review, bkor, if you have a chance to get to it sometime soon. It still applies.

Comment 7

12 years ago
Comment on attachment 234687 [details] [diff] [review]
v3

>Index: checksetup.pl

>-if (!GroupDoesExist('bz_sudoers')) {

>-    $sth->execute($sudoers_group, $sudo_protect_group, GROUP_MEMBERSHIP);



>Index: Bugzilla/Group.pm

>+sub create {

>+        $sth->execute($admin->id, $group->id, GROUP_MEMBERSHIP);
>+        $sth->execute($admin->id, $group->id, GROUP_BLESS);
>+        $sth->execute($admin->id, $group->id, GROUP_VISIBLE);


From what I see, your code doesn't automatically add members of the bz_sudoers group into the bz_sudo_protect group, which is very bad.
Attachment #234687 - Flags: review?(bugzilla-mozilla) → review-

Comment 8

12 years ago
(In reply to comment #7)
> >+        $sth->execute($admin->id, $group->id, GROUP_VISIBLE);

Also, note that admins have not GROUP_VISIBLE "privs" by default.
(Assignee)

Comment 9

12 years ago
Created attachment 237836 [details] [diff] [review]
v4

Okay, I fixed the sudo thing, and I also updated the patch to have $invocant on all the validators.

Admins *should* have GROUP_VISIBLE by default. The fact that they don't is an error, which happened because checksetup.pl is so complicated.
Attachment #234687 - Attachment is obsolete: true
Attachment #237836 - Flags: review?(LpSolit)

Comment 10

12 years ago
Comment on attachment 237836 [details] [diff] [review]
v4

>Index: checksetup.pl

> # Create bz_canusewhineatothers and bz_canusewhines

>-    $dbh->do("INSERT INTO group_group_map " .
>-             "(member_id, grantor_id, grant_type) " .
>-             "VALUES (${whineatothers_group}, ${whine_group}, " .
>-             GROUP_MEMBERSHIP . ")") unless $group_exists;

>+    $dbh->do('INSERT INTO group_group_map (grantor_id, member_id) VALUES (?,?)',
>+             undef, $whineatothers->id, $whine->id);

OOPS! The grantor is $whine, not $whineatothers. This is the reason of my r-. This is critical enough to force an updated patch.



>Index: Bugzilla/Group.pm

>+=item C<create>
>+
>+Note that in addition to what L<Bugzilla::Object/create($params)>
>+normally does, this function also makes the new group be inherited
>+by the C<admin> group. That is, the C<admin> group will automatically
>+be a member of this group.

Nit: maybe could you also mention that it sets inheritance correctly too.


Else your patch is working fine on a fresh installation. I will r+ it with the error above fixed.
Attachment #237836 - Flags: review?(LpSolit) → review-
(Assignee)

Comment 11

12 years ago
Created attachment 238015 [details] [diff] [review]
v5

Oh wow, okay. You're right. Wow, those columns are named really badly.
Attachment #237836 - Attachment is obsolete: true
Attachment #238015 - Flags: review?(LpSolit)

Comment 12

12 years ago
Comment on attachment 238015 [details] [diff] [review]
v5

interdiff looks good. r=LpSolit
Attachment #238015 - Flags: review?(LpSolit) → review+

Updated

12 years ago
Flags: approval?
Flags: approval? → approval+
(Assignee)

Comment 13

12 years ago
Checking in checksetup.pl;
/cvsroot/mozilla/webtools/bugzilla/checksetup.pl,v  <--  checksetup.pl
new revision: 1.541; previous revision: 1.540
done
Checking in editgroups.cgi;
/cvsroot/mozilla/webtools/bugzilla/editgroups.cgi,v  <--  editgroups.cgi
new revision: 1.80; previous revision: 1.79
done
Checking in Bugzilla/Group.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/Group.pm,v  <--  Group.pm
new revision: 1.16; previous revision: 1.15
done
Checking in Bugzilla/DB/Schema.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/DB/Schema.pm,v  <--  Schema.pm
new revision: 1.72; previous revision: 1.71
done
Status: ASSIGNED → RESOLVED
Last Resolved: 12 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.