678146 - create a "tell us more" extension

Status: RESOLVED WONTFIX

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

Description

[:glob ✱]

tracking bug for the tell-us-more bmo work.

Comment 1

[:glob ✱]

Attachment: patch v1

tell-us-more extension.

setup:
  1. add a product "Untriaged Bugs"
  2. add "General" and "Firefox" components to "Untriaged Bugs"
  3. add a user "[e-mail]" password "password"

Comment 2

[David Lawrence [:dkl]]

Comment on attachment 553478 [details] [diff] [review]
patch v1

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

Initial review, more tomorrow when I actually run it.

::: Bugzilla/Token.pm
@@ +247,4 @@
>      $column ||= "token";
>  
>      my $dbh = Bugzilla->dbh;
> +    my $sth = $dbh->prepare("SELECT 1 FROM $table WHERE $column = ?");

Should this go upstream?

::: extensions/TellUsMore/lib/Data.pm
@@ +16,5 @@
> +#
> +# Contributor(s):
> +#   Byron Jones <[e-mail]>
> +
> +package Bugzilla::Extension::TellUsMore::Data;

Nit: Traditionally called Constants.

::: extensions/TellUsMore/lib/Process.pm
@@ +135,5 @@
> +    
> +    # version --> unspecified
> +
> +    my $version = Bugzilla::Version->new({ product => $product, name => $params->{version} })
> +        || $version = Bugzilla::Version->new({ product => $product, name => DEFAULT_VERSION });

Causes compilation error:

Global symbol "$version" requires explicit package name at /loader/0x4714240/Bugzilla/Extension/TellUsMore/Process.pm line 139.

Instead:

my $version = Bugzilla::Version->new({ product => $product, name => $params->{version} })
    || Bugzilla::Version->new({ product => $product, name => DEFAULT_VERSION });

@@ +180,5 @@
> +        bug_severity => $params->{bug_severity},
> +        priority => $params->{priority},
> +    };
> +    if ($params->{groups}) {
> +        $create->{groups} = [ $params->{groups} ];

Nit: $params->{groups} can really only be one group so maybe call it $params->{group}.

::: extensions/TellUsMore/lib/Submit.pm
@@ +16,5 @@
> +#
> +# Contributor(s):
> +#   Byron Jones <[e-mail]>
> +
> +package Bugzilla::Extension::TellUsMore::Submit;

Nit: Normally call this WebService.

@@ +123,5 @@
> +
> +    # priority, bug_severity
> +
> +    $params->{priority} = DEFAULT_PRIORITY;
> +    $params->{bug_severity} = DEFAULT_SEVERITY;

Why not use Bugzilla->params->{defaultpriority} and Bugzilla->params->{defaultseverity}?

::: extensions/TellUsMore/lib/Util.pm
@@ +25,5 @@
> +
> +sub dumper {
> +    require Data::Dumper;
> +    my $output = Data::Dumper::Dumper(@_);
> +    $output =~ s/</&klt;/g;

$output =~ s/</&lt;/g;

::: extensions/TellUsMore/lib/VersionMirror.pm
@@ +27,5 @@
> +
> +# versions in this product
> +use constant SOURCE_PRODUCT => 'Firefox';
> +# will be mirrored into this product
> +use constant TARGET_PRODUCT => 'Untriaged Bugs';

Move to Data.pm (or Constants.pm if you rename it).

::: extensions/TellUsMore/template/en/default/hook/global/user-error-errors.html.tmpl
@@ +20,5 @@
> +
> +[% IF error == "tum_auth_failure" %]
> +  You are not authorized to use this feature.
> +
> +[% ELSIF error == "tum_disabled" %]

Maybe should be tum_temp_disabled. tum_disabled sounds like the whole system is disabled for submission when it is just temp down
for custom field creation, etc.

@@ +24,5 @@
> +[% ELSIF error == "tum_disabled" %]
> +  Issue reporting is temporarily disabled; please wait a few minutes and try again.
> +
> +[% ELSIF error == "tum_account_disabled" %]
> +  The account associated with '[% user.login FILTER none %]' is disabled:

Why all of the FILTER none's in this file? Why not FILTER html?

@@ +31,5 @@
> +[% ELSIF error == "tum_too_many_attachments" %]
> +  More than [% max FILTER none %] attachments were provided.
> +
> +[% ELSIF error == "tum_attachment_too_large" %]
> +  [% filename FILTER none %] is larger than [% max %]k.

max FILTER html

@@ +45,5 @@
> +[% ELSIF error == "tum_rate_exceeded" %]
> +  You cannot report more than [% max FILTER none %] issues per minute.
> +
> +[% ELSIF error == "tum_invalid_component" %]
> +  The product [% product FILTER none %] is missing the component [% component FILTER none %]

Add period.

::: extensions/TellUsMore/template/en/default/pages/tellusmore.html.tmpl
@@ +29,5 @@
> +[% ELSE %]
> +
> +<h4>success!</h4>
> +
> +<a href="show_bug.cgi?id=[% bug.id FILTER url %]">bug [% bug.id FILTER html %]</a>

FILTER url_quote (soon to be FILTER uri in 4.2). Also change 'bug' to [% terms.bug %]. I know this will likely change anyway when they give us the proper content.

Comment 3

[:glob ✱]

thanks for the quick review :)

> > +    my $sth = $dbh->prepare("SELECT 1 FROM $table WHERE $column = ?");
> 
> Should this go upstream?

already done, bug 678959
 
> > +    my $version = Bugzilla::Version->new({ product => $product, name => $params->{version} })
> > +        || $version = Bugzilla::Version->new({ product => $product, name => DEFAULT_VERSION });
> 
> Causes compilation error:
> 
> Global symbol "$version" requires explicit package name at
> /loader/0x4714240/Bugzilla/Extension/TellUsMore/Process.pm line 139.

weird; worked for my version of perl.  clearly wrong though :)

> Why all of the FILTER none's in this file? Why not FILTER html?

most of these errors are thrown back by the web service; i didn't want to return html there.

Comment 4

[David Lawrence [:dkl]]

(In reply to Byron Jones ‹:glob› from comment #3)
> most of these errors are thrown back by the web service; i didn't want to
> return html there.

Yeah. Called "should not review when tired" :) I realized that this morning after sleep and I thought about it some more.

dkl

Comment 5

[:glob ✱]

Attachment: patch v2

this revision addresses review points, and adds the ability for multiple source products to version sync.

Comment 6

[David Lawrence [:dkl]]

Couple of errors I encountered getting this setup. 

If you run checksetup.pl without creating the new "Untriaged Bugs" and require components, you get the following error:

...
Removing existing compiled templates...
Precompiling templates...done.
Fixing file permissions...
Can't call method "versions" on an undefined value at /loader/0x6daa178/Bugzilla/Extension/TellUsMore/VersionMirror.pm line 154.

$self->_target returns undefined if the product doesn't exist.

After you create the product/components, checksetup.pl gives this error:

Removing existing compiled templates...
Precompiling templates...done.
Fixing file permissions...
The version 'Trunk' already exists for product 'Untriaged Bugs'.

It doesn't like the fact that Firefox and Fennec both have Trunk as versions and Bugzilla complains when trying to create it again.

Oddly enough, when you run checksetup.pl the third time without changing anything else it goes through without error.

More testing to come.

dkl

Comment 7

[:glob ✱]

(In reply to David Lawrence [:dkl] from comment #6)
> If you run checksetup.pl without creating the new "Untriaged Bugs" and
> require components, you get the following error:

you need to follow the setup instructions in comment 1; the code assumes you have done so prior to installation.

> It doesn't like the fact that Firefox and Fennec both have Trunk as versions
> and Bugzilla complains when trying to create it again.

ah, i haven't tested it with a clean product; i'll fix this today.

Comment 8

[:glob ✱]

(In reply to David Lawrence [:dkl] from comment #6)
> It doesn't like the fact that Firefox and Fennec both have Trunk as versions
> and Bugzilla complains when trying to create it again.

ahhh.. when you create a version, bugzilla doesn't clear the product's cached list of versions, so when the second 'trunk' is encountered, the product is incorrectly reporting that the version doesn't exist.

rasied as bug 681191, will work around in the extension.

Comment 9

[:glob ✱]

Attachment: patch v3

this revision has better error reporting, and fixes a few bugs.

Comment 10

[David Lawrence [:dkl]]

Comment on attachment 555037 [details] [diff] [review]
patch v3

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

Doing some more incremental review...

I noticed my attachments in test.pl are not getting attached to the new bug. Will test some more to see what is happening.

::: extensions/TellUsMore/lib/WebService.pm
@@ +51,5 @@
> +        ThrowUserError('tum_auth_failure');
> +    }
> +
> +    if (Bugzilla->params->{disable_bug_updates}) {
> +        ThrowUserError('tum_updated_disabled');

ThrowUserError('tum_updates_disabled');

::: extensions/TellUsMore/template/en/default/pages/tellusmore.html.tmpl
@@ +29,5 @@
> +[% ELSE %]
> +
> +<h4>success!</h4>
> +
> +<a href="show_bug.cgi?id=[% bug.id FILTER url %]">bug [% bug.id FILTER html %]</a>

I realize this is just a placeholder page anyway.

<a href="show_bug.cgi?id=[% bug.id FILTER url_quote %]">[% terms.bug %] [% bug.id FILTER html %]</a>

Comment 11

[David Lawrence [:dkl]]

Comment on attachment 555037 [details] [diff] [review]
patch v3

(In reply to David Lawrence [:dkl] from comment #10)
> I noticed my attachments in test.pl are not getting attached to the new bug.
> Will test some more to see what is happening.

I figured out what was wrong. The test.pl script was passing to the webservice the param 'no_attachments' and the webservice was expecting the param to be named 'attachments'. After that I was able to continue testing and it worked as expected.

r- til the ThrowUserError issue is resolved.

dkl

Comment 12

[:glob ✱]

Attachment: patch v4

this revision fixes the ThrowUserError typo, and adds a param to set the url field.

Comment 13

[David Lawrence [:dkl]]

Comment on attachment 561674 [details] [diff] [review]
patch v4

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

Looks good and works as expected. r=dkl

::: extensions/TellUsMore/lib/WebService.pm
@@ +136,5 @@
> +    }
> +
> +    # set url
> +
> +    $params->{bug_file_loc} = $params->{url};

Nit: $params->{bug_file_loc} = delete $params->{url};

Comment 14

[:glob ✱]

thanks dkl.

i'll hold off committing this until we have the content of the emails and pages (bug 680094).

Comment 15

[Aakash Desai [:aakashd]]

E-mail content added as an attachment to https://bugzilla.mozilla.org/show_bug.cgi?id=680094#c2

As for the content of the pages, are you referring to the design, the text content or both?

Comment 16

[:glob ✱]

(In reply to Aakash Desai [:aakashd] from comment #15)
> As for the content of the pages, are you referring to the design, the text
> content or both?

both.

Comment 17

[:glob ✱]

Attachment: patch v6

latest revision with tweaks following development on -stage-tip.
carrying forward r+

Comment 18

[:glob ✱]

Committing to: bzr+ssh://[e-mail]/bmo/4.0/
modified Bugzilla/Token.pm
added extensions/TellUsMore
modified extensions/BMO/Extension.pm
added extensions/TellUsMore/Config.pm
added extensions/TellUsMore/Extension.pm
added extensions/TellUsMore/lib
added extensions/TellUsMore/template
added extensions/TellUsMore/lib/Constants.pm
added extensions/TellUsMore/lib/Process.pm
added extensions/TellUsMore/lib/VersionMirror.pm
added extensions/TellUsMore/lib/WebService.pm
added extensions/TellUsMore/template/en
added extensions/TellUsMore/template/en/default
added extensions/TellUsMore/template/en/default/email
added extensions/TellUsMore/template/en/default/hook
added extensions/TellUsMore/template/en/default/email/existing-account.header.tmpl
added extensions/TellUsMore/template/en/default/email/existing-account.html.tmpl
added extensions/TellUsMore/template/en/default/email/existing-account.txt.tmpl
added extensions/TellUsMore/template/en/default/email/new-account.header.tmpl
added extensions/TellUsMore/template/en/default/email/new-account.html.tmpl
added extensions/TellUsMore/template/en/default/email/new-account.txt.tmpl
added extensions/TellUsMore/template/en/default/hook/global
added extensions/TellUsMore/template/en/default/hook/global/user-error-errors.html.tmpl
Committed revision 8105.

Comment 19

[:glob ✱]

this extension has been committed and deployed (as part of a bulk schema changing update), however it's currently disabled pending the outcome of the security review.

Comment 20

[:glob ✱]

this project has been shelved, the code has been removed from the bmo/4.2 branch.