896320 - increase the visibility of review suggestions (report, webservice)

Status: RESOLVED FIXED

(bugzilla.mozilla.org :: Extensions, defect)

Description

[:glob ✱]

currently the list of product/component review suggestions is only visible to admins, or when trying to set the flag.

we need to add a report showing all the products, components, and suggestions, as well as a web service so tools such as bzexport can use that list.

Comment 1

[:glob ✱]

Attachment: 896320_1.patch

Comment 2

[David Lawrence [:dkl]]

Comment on attachment 779783 [details] [diff] [review]
896320_1.patch

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

Reviewers Page Nits (review_suggestions.html.tmpl):

- Wish there was a way to add the report link to the prior <ul> instead of being in it's own <ul>. Looks bad but not a big deal. Realize that the menu-end.html.tmpl in creates the "Other Reports" <ul> so not easy to add to another extension's markup.
- Can we get alternating background for components?
- Also when mouseover a row, there is some spacing between the component name and the reviewers.

dkl

::: extensions/Review/lib/WebService.pm
@@ +28,5 @@
> +    }
> +    elsif (exists $params->{product}) {
> +        $product = Bugzilla::Product->check($params->{product});
> +        if (exists $params->{component}) {
> +            my $component = Bugzilla::Component->check({

Remove 'my' or $component is undef later regardless.

@@ +50,5 @@
> +    foreach my $reviewer (@$reviewers) {
> +        push @result, {
> +            id    => $reviewer->id,
> +            email => $reviewer->login,
> +            name  => $reviewer->name,

Each of these needs $self->type() wrappers. Also make sure to use 'email' for the datatype for $reviewer->login.

@@ +58,5 @@
> +}
> +
> +sub rest_resources {
> +    return [
> +        qr{^/Review/suggestions/(\d+)$}, {

nit: Should we be lower case for consistency? Maybe change to qr{^/Review/suggestions/(\d+)$}i

@@ +66,5 @@
> +                    return { bug_id => $_[0] };
> +                },
> +            },
> +        },
> +        qr{^/Review/suggestions/([^/]+)/(.+)$}, {

this will work as long as don't have any product with '/' in the name. The / would need to be url encoded but is is converted back to '/' when we get here?

::: extensions/Review/template/en/default/pages/review_suggestions.html.tmpl
@@ +58,5 @@
> +      <td class="other_components">All other components</td>
> +      <td>
> +        [% FOREACH reviewer = product.reviewers %]
> +          <span title="[% reviewer.name FILTER html %]">
> +          [% reviewer.email FILTER html %]</span>

FILTER email

Comment 3

[:glob ✱]

(In reply to David Lawrence [:dkl] from comment #2)
> > +sub rest_resources {
> > +    return [
> > +        qr{^/Review/suggestions/(\d+)$}, {
> 
> nit: Should we be lower case for consistency?
> Maybe change to qr{^/Review/suggestions/(\d+)$}i

that would make it inconsistent with the upstream rest endpoints ;)
lowercase it is (i was thinking of the uppercased xmlrpc endpoints, instead of rest).

> FILTER email

i don't want to use the email filter because it adds a lot to an already large report (production will have a *lot* of reviewers once fully configured).  there's no need to protect the email address as login is required.

> > +        qr{^/Review/suggestions/([^/]+)/(.+)$}, {
> 
> this will work as long as don't have any product with '/' in the name. The /
> would need to be url encoded but is is converted back to '/' when we get
> here?

nice catch - products with slashes don't work, but something weird is going on... i get 404's and B::W::S::Rest::handle() isn't even hit.  will investigate.

Comment 4

[:glob ✱]

(In reply to Byron Jones ‹:glob› from comment #3)
> nice catch - products with slashes don't work, but something weird is going
> on... i get 404's and B::W::S::Rest::handle() isn't even hit.  will
> investigate.

i filed bug 899911 for that.  for this report i'll make it accept names in both the url and via named parameters in the query string.

Comment 5

[:glob ✱]

Attachment: 896320_2.patch

Comment 6

[David Lawrence [:dkl]]

Comment on attachment 783618 [details] [diff] [review]
896320_2.patch

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

\o/ r=dkl

Comment 7

[:glob ✱]

Attachment: webservice docs

Comment 8

[:glob ✱]

Committing to: bzr+ssh://[e-mail]/bmo/4.2/
modified extensions/BMO/web/styles/reports.css
modified extensions/Review/Extension.pm
added extensions/Review/lib
added extensions/Review/lib/WebService.pm
added extensions/Review/template/en/default/pages
added extensions/Review/template/en/default/hook/reports
modified extensions/Review/template/en/default/hook/global/user-error-errors.html.tmpl
added extensions/Review/template/en/default/hook/reports/menu-end.html.tmpl
added extensions/Review/template/en/default/pages/review_suggestions.html.tmpl
added extensions/Review/web/styles/reports.css
Committed revision 8919.