1008764 - Add a web service to create and update Flag types

Status: RESOLVED FIXED

(Bugzilla :: WebService, enhancement, P2)

Description

[Simon Green]

SSIA really. This is a bug to add a web service to create and update flag types for users that have the correct privileges to do so. Patch will be coming later this week (brc already have FlagTypes.create, but not .update)

Comment 1

[Simon Green]

Attachment: bug1008764-v1.patch

Comment 2

[Frédéric Buclin]

Comment on attachment 8422134 [details] [diff] [review]
bug1008764-v1.patch

>=== added file 'Bugzilla/WebService/FlagType.pm'

Missing MPL 2.0 license.


>+use Bugzilla;

Never call Bugzilla from a Bugzilla module.

Comment 3

[Simon Green]

Attachment: bug1008764-v2.patch

Comment 4

[:glob ✱]

Comment on attachment 8422802 [details] [diff] [review]
bug1008764-v2.patch

Review of attachment 8422802 [details] [diff] [review]:
-----------------------------------------------------------------

here's some comments from a quick read-through of the code.
i'll perform testing of the code soon and provide an updated review.

::: Bugzilla/FlagType.pm
@@ +645,5 @@
>  
>      if ($criteria->{name}) {
> +        if (ref($criteria->{name}) eq 'ARRAY') {
> +            my @names = map { $dbh->quote($_) } @{$criteria->{name}};
> +            trick_taint($_) foreach @names; # Detaint data as we have quoted it.

use variable names in foreach, not $_

::: Bugzilla/WebService/FlagType.pm
@@ +290,5 @@
> +C<string> a comprehensive description of this type.
> +
> +=item inclusions B<optional>
> +
> +An array of strings or hashes containing product names.

this description needs to be expanded, as it is much more than just product names.

@@ +292,5 @@
> +=item inclusions B<optional>
> +
> +An array of strings or hashes containing product names.
> +
> +EG

nit: "e.g." or "for example"

@@ +299,5 @@
> +  {
> +    BarProduct => [ 'C1', 'C3' ],
> +    BazProduct => [ 'C7' ]
> +  }
> +]

when viewed with perldoc, this code is collapsed onto a single line, making it hard to read.

EG and formatting issues occur multiple times in this perldoc

@@ +313,5 @@
> +This will exclude the flag from all products and components specified.
> +
> +=item sortkey B<optional>
> +
> +C<int> A number between 1 and 32767 by which this type will be sorted when displayed to users in a list; ignore if you don't care what order the types appear in or if you want them to appear in alphabetical order.

that's a mighty long line you have there.

@@ +328,5 @@
> +
> +=item cc_list B<optional>
> +
> +C<array> An array of strings.
> +if requestable, who should get carbon copied on email notification of requests. This is an array of full e-mail addresses which do not need to be Red Hat Bugzilla logins.

while it's correct that these email addresses "do not need to be Red Hat Bugzilla logins", we probably don't need to mention Red Hat.

@@ +334,5 @@
> +=item is_specifically_requestable B<optional>
> +
> +C<boolean> default is B<true>
> +
> +specifically requestable (users can ask specific other users to set flags of this type as opposed to just asking the wind)

some of these descriptions start with a capital letter and finish with a full stop.  others do not.  please be consistent.

Comment 5

[Frédéric Buclin]

(In reply to Byron Jones ‹:glob› from comment #4)

> > +            trick_taint($_) foreach @names; # Detaint data as we have quoted it.
> 
> use variable names in foreach, not $_

I really don't see the point to use variable names in foreach for such a thing. That's really a stupid rule which has no rationale and which you don't even follow yourself. 2 examples coming from your own recent patches:

  my @sources = map { @$_ } @_; from bug 977969 (good luck to guess what @_ and $_ contain)

or

  my @group_ids = map { $_->id } @{ $self->groups }; from bug 993894


It's very common to use $_ in foreach loop when doing one single action on each item.

Comment 6

[Simon Green]

Attachment: bug1008764-v3.patch

FWIW, I think nonamed foreach is okay too, but I have changed it as requested.

I've also made the other suggested changes to the POD.

Comment 7

[:glob ✱]

(In reply to Frédéric Buclin from comment #5)
> (In reply to Byron Jones ‹:glob› from comment #4)
> I really don't see the point to use variable names in foreach for such a
> thing. That's really a stupid rule which has no rationale

"Just because you CAN do something a particular way doesn't mean that you SHOULD do it that way. Perl is designed to give you several ways to do anything, so consider picking the most readable one"

> and which you don't even follow yourself. 2 examples coming from your own recent patches:

neither of those examples are foreach loops; you can't use named variables with map.

Comment 8

[Frédéric Buclin]

(In reply to Byron Jones ‹:glob› from comment #7)
> "Just because you CAN do something a particular way doesn't mean that you
> SHOULD do it that way. Perl is designed to give you several ways to do
> anything, so consider picking the most readable one"

Using $_ is perfectly readable in this trivial case. It's dishonest to say it's not. Your interpretation of "readable" is severely biased.


> neither of those examples are foreach loops; you can't use named variables
> with map.

Sure, but map() is nothing more than:

  foreach my $foo (@list) {
     # do something.
  }

So we could argue that you should do it this way instead of using map(), for readability. In that case, you could also replace:

  trick_taint($_) foreach @names;

by:

  map { trick_taint($_) } @names

Happy now? It uses map() so it's more readable?

Comment 9

[:glob ✱]

Comment on attachment 8423479 [details] [diff] [review]
bug1008764-v3.patch

Review of attachment 8423479 [details] [diff] [review]:
-----------------------------------------------------------------

r=glob

nice.

the issues below can be fixed on commit.

::: Bugzilla/WebService/FlagType.pm
@@ +29,5 @@
> +    Bugzilla->user->in_group('editcomponents')
> +        || scalar(@{$user->get_products_by_permission('editcomponents')})
> +        || ThrowUserError("auth_failure", { group => "editcomponents",
> +                                         action => "add",
> +                                         object => "flagtypes"});

nit: missing space before }

@@ +158,5 @@
> +    my @result;
> +    foreach my $flag_type_id (keys %changes) {
> +        my %hash = (
> +            id      => $flag_type_id,
> +            name    => $changes{$flag_type_id}{name},

the id and name field are missing their type information..
  id => $self->type('int', $flag_type_id),
  ...

Comment 10

[Simon Green]

To ssh://git.mozilla.org/bugzilla/bugzilla.git
   abf6ec5..9bd3f08  master -> master

Comment 11

[Mark Côté [:mcote]]

The above commit included changes that were not in the final, reviewed patch.  I've filed bug 1077018 to back those parts out.

Comment 12

[Frédéric Buclin]

(In reply to Mark Côté [:mcote] from comment #11)
> The above commit included changes that were not in the final, reviewed
> patch.  I've filed bug 1077018 to back those parts out.

It was accidentally mixed with bug 39120.