283581 - Move UserInGroup out of globals.pl

Status: RESOLVED FIXED

(Bugzilla :: Bugzilla-General, enhancement, P2)

Description

[Max Kanat-Alexander]

UserInGroup is basically syntactic sugar for "exists
Bugzilla->user->groups->{'name'}"

As such, it should live in Bugzilla::User or Bugzilla. Probably Bugzilla::User.

Comment 1

[Max Kanat-Alexander]

You know, really, we should replace this with Bugzilla->user->in_group($name).
And in_group should always use ->groups for speed, instead of doing a lookup
when ->groups doesn't exist.

But that's really another bug, which I will also file.

Comment 2

[Colin Ogilvie [:cso]]

Attachment: Patch v1

First go at the main point of this bug.

Comment 3

[Max Kanat-Alexander]

Comment on attachment 177302 [details] [diff] [review]
Patch v1

>--- Bugzilla/Series.pm	17 Feb 2005 21:57:27 -0000	1.7
>+++ Bugzilla/Series.pm	13 Mar 2005 19:23:31 -0000
>@@ -33,7 +33,7 @@
> 
> use Bugzilla;
> use Bugzilla::Util;
>-use Bugzilla::User;
>+#use Bugzilla::User;

  Don't comment it out -- just delete it entirely.

>             # UserInGroup. Deprecated - use the user.* functions instead
>-            'UserInGroup' => \&::UserInGroup,
>+            'UserInGroup' => \Bugzilla::User::UserInGroup,

  This works? You don't need the & in this case?

>--- Bugzilla/User.pm	11 Mar 2005 19:31:36 -0000	1.46
>+++ Bugzilla/User.pm	13 Mar 2005 19:34:40 -0000
>@@ -44,7 +44,7 @@
> 
> use base qw(Exporter);
> @Bugzilla::User::EXPORT = qw(insert_new_user is_available_username
>-    login_to_id
>+    login_to_id UserInGroup

  Put UserInGroup on a different line -- that's why I structured the EXPORT
statement like I did, so we could just add lines.

>+sub UserInGroup {
>+    if ($_[1]) {
>+        die "UserInGroup no longer takes a second parameter.";
>+    }

  Instead of doing this, add a prototype. (I know it was there in the old code,
but this is a cleanup patch, also, so let's just clean that up. :-) )
UserInGroup ($) {

>+    return 0 unless $_[0];

  You don't need that -- Buzilla->user->groups will handle it for you.

>+    return defined Bugzilla->user->groups->{$_[0]};

  This really ought to use in_group, and in_group ought to always call ->groups
instead of doing it's own funky thing. (I forget, did I fix that already?)
That's not necessary, it's just a hopeful suggestion. :-)


>@@ -1350,6 +1358,10 @@
> However, consider using a Bugzilla::User object instead of this function
> if you need more information about the user than just their ID.
> 
>+=item C<UserInGroup($groupname)>
>+
>+Takes a name of a group, and returns 1 if a user is in the group, 0 otherwise.

  You should make that true by adding a ? 1 : 0 to the return statement.
Otherwise perl will return whatever it feels like for false.

  Oh, and hrm, did I miss a =back in my original POD docs, there? Looks like we
need one.

  Overall, a good patch. A few too many nits for +, though, quite just yet. :-)

Comment 4

[Colin Ogilvie [:cso]]

(In reply to comment #3)
> (From update of attachment 177302 [details] [diff] [review] [edit])
> >--- Bugzilla/Series.pm	17 Feb 2005 21:57:27 -0000	1.7
> >+++ Bugzilla/Series.pm	13 Mar 2005 19:23:31 -0000
> >@@ -33,7 +33,7 @@
> > 
> > use Bugzilla;
> > use Bugzilla::Util;
> >-use Bugzilla::User;
> >+#use Bugzilla::User;
> 
>   Don't comment it out -- just delete it entirely.

Actually, it needs to be there - I was testing something and must have forgotten
to uncoment it
 
> >             # UserInGroup. Deprecated - use the user.* functions instead
> >-            'UserInGroup' => \&::UserInGroup,
> >+            'UserInGroup' => \Bugzilla::User::UserInGroup,
> 
>   This works? You don't need the & in this case?

It seemed to... but I've added tha & in anyway.


> >--- Bugzilla/User.pm	11 Mar 2005 19:31:36 -0000	1.46
> >+++ Bugzilla/User.pm	13 Mar 2005 19:34:40 -0000
> >@@ -44,7 +44,7 @@
> > 
> > use base qw(Exporter);
> > @Bugzilla::User::EXPORT = qw(insert_new_user is_available_username
> >-    login_to_id
> >+    login_to_id UserInGroup
> 
>   Put UserInGroup on a different line -- that's why I structured the EXPORT
> statement like I did, so we could just add lines.

Odd, I'd have expected it to have a new line after it - the way it looks to me
is that you just keep adding one on the end.


> >+    return 0 unless $_[0];
> 
>   You don't need that -- Buzilla->user->groups will handle it for you.

Without it, checksetup.pl seems to keep throwing a warning, but for some reason
the prototype's fixed that.
 
> >+    return defined Bugzilla->user->groups->{$_[0]};
> 
>   This really ought to use in_group, and in_group ought to always call ->groups
> instead of doing it's own funky thing. (I forget, did I fix that already?)
> That's not necessary, it's just a hopeful suggestion. :-)

bug 283582 appears to be that - I'd rather do this then someone else can look at
that in the future.

> >@@ -1350,6 +1358,10 @@
> > However, consider using a Bugzilla::User object instead of this function
> > if you need more information about the user than just their ID.
> > 
> >+=item C<UserInGroup($groupname)>
> >+
> >+Takes a name of a group, and returns 1 if a user is in the group, 0 otherwise.
> 
>   You should make that true by adding a ? 1 : 0 to the return statement.
> Otherwise perl will return whatever it feels like for false.

Done.

>   Oh, and hrm, did I miss a =back in my original POD docs, there? Looks like we
> need one.

It was above the one I copied (which was below it)... I've moved it but I'm not
a pod expert.

New patch in a few minutes.

Comment 5

[Colin Ogilvie [:cso]]

Attachment: Patch v2

Fixes nites, and comments

Comment 6

[Max Kanat-Alexander]

Comment on attachment 177320 [details] [diff] [review]
Patch v2

Perfect.

I'm allowing it to stay UpperCaseNamed because it's going away. :-)

Comment 7

[Colin Ogilvie [:cso]]

Attachment: Fix Bitrot

Travis informs me this patch has bitrotted - here's an un-bit-rotten one.

Comment 8

[Max Kanat-Alexander]

Comment on attachment 177521 [details] [diff] [review]
Fix Bitrot

Carrying forward r+ after brief inspection of the interdiff.

Comment 9

[Shane H. W. Travis]

Checking in CGI.pl;
/cvsroot/mozilla/webtools/bugzilla/CGI.pl,v  <--  CGI.pl
new revision: 1.235; previous revision: 1.234
done
Checking in buglist.cgi;
/cvsroot/mozilla/webtools/bugzilla/buglist.cgi,v  <--  buglist.cgi
new revision: 1.287; previous revision: 1.286
done
Checking in chart.cgi;
/cvsroot/mozilla/webtools/bugzilla/chart.cgi,v  <--  chart.cgi
new revision: 1.10; previous revision: 1.9
done
Checking in colchange.cgi;
/cvsroot/mozilla/webtools/bugzilla/colchange.cgi,v  <--  colchange.cgi
new revision: 1.47; previous revision: 1.46
done
Checking in describekeywords.cgi;
/cvsroot/mozilla/webtools/bugzilla/describekeywords.cgi,v  <--  
describekeywords.cgi
new revision: 1.13; previous revision: 1.12
done
Checking in doeditparams.cgi;
/cvsroot/mozilla/webtools/bugzilla/doeditparams.cgi,v  <--  doeditparams.cgi
new revision: 1.33; previous revision: 1.32
done
Checking in editflagtypes.cgi;
/cvsroot/mozilla/webtools/bugzilla/editflagtypes.cgi,v  <--  editflagtypes.cgi
new revision: 1.16; previous revision: 1.15
done
Checking in editgroups.cgi;
/cvsroot/mozilla/webtools/bugzilla/editgroups.cgi,v  <--  editgroups.cgi
new revision: 1.50; previous revision: 1.49
done
Checking in editkeywords.cgi;
/cvsroot/mozilla/webtools/bugzilla/editkeywords.cgi,v  <--  editkeywords.cgi
new revision: 1.25; previous revision: 1.24
done
Checking in editmilestones.cgi;
/cvsroot/mozilla/webtools/bugzilla/editmilestones.cgi,v  <--  editmilestones.cgi
new revision: 1.34; previous revision: 1.33
done
Checking in editparams.cgi;
/cvsroot/mozilla/webtools/bugzilla/editparams.cgi,v  <--  editparams.cgi
new revision: 1.24; previous revision: 1.23
done
Checking in editproducts.cgi;
/cvsroot/mozilla/webtools/bugzilla/editproducts.cgi,v  <--  editproducts.cgi
new revision: 1.74; previous revision: 1.73
done
Checking in editsettings.cgi;
/cvsroot/mozilla/webtools/bugzilla/editsettings.cgi,v  <--  editsettings.cgi
new revision: 1.2; previous revision: 1.1
done
Checking in editversions.cgi;
/cvsroot/mozilla/webtools/bugzilla/editversions.cgi,v  <--  editversions.cgi
new revision: 1.33; previous revision: 1.32
done
Checking in enter_bug.cgi;
/cvsroot/mozilla/webtools/bugzilla/enter_bug.cgi,v  <--  enter_bug.cgi
new revision: 1.109; previous revision: 1.108
done
Checking in globals.pl;
/cvsroot/mozilla/webtools/bugzilla/globals.pl,v  <--  globals.pl
new revision: 1.318; previous revision: 1.317
done
Checking in query.cgi;
/cvsroot/mozilla/webtools/bugzilla/query.cgi,v  <--  query.cgi
new revision: 1.143; previous revision: 1.142
done
Checking in sanitycheck.cgi;
/cvsroot/mozilla/webtools/bugzilla/sanitycheck.cgi,v  <--  sanitycheck.cgi
new revision: 1.89; previous revision: 1.88
done
Checking in show_bug.cgi;
/cvsroot/mozilla/webtools/bugzilla/show_bug.cgi,v  <--  show_bug.cgi
new revision: 1.32; previous revision: 1.31
done
Checking in showdependencytree.cgi;
/cvsroot/mozilla/webtools/bugzilla/showdependencytree.cgi,v  <--  
showdependencytree.cgi
new revision: 1.31; previous revision: 1.30
done
Checking in summarize_time.cgi;
/cvsroot/mozilla/webtools/bugzilla/summarize_time.cgi,v  <--  summarize_time.cgi
new revision: 1.2; previous revision: 1.1
done
Checking in Bugzilla/Attachment.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/Attachment.pm,v  <--  Attachment.pm
new revision: 1.20; previous revision: 1.19
done
Checking in Bugzilla/DB.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/DB.pm,v  <--  DB.pm
new revision: 1.31; previous revision: 1.30
done
Checking in Bugzilla/Search.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/Search.pm,v  <--  Search.pm
new revision: 1.90; previous revision: 1.89
done
Checking in Bugzilla/Series.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/Series.pm,v  <--  Series.pm
new revision: 1.8; previous revision: 1.7
done
Checking in Bugzilla/Template.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/Template.pm,v  <--  Template.pm
new revision: 1.23; previous revision: 1.22
done
Checking in Bugzilla/User.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/User.pm,v  <--  User.pm
new revision: 1.48; previous revision: 1.47
done