571740 - Add support for getsatisfaction for the See Also field

Status: RESOLVED FIXED

(Bugzilla :: Creating/Changing Bugs, enhancement)

Description

[Thomas D. (:thomas8)]

STR

0 Realize that users are being encouraged to commit their feedback (incl. bug reports) at Thunderbird's getsatisfaction support site (http://getsatisfaction.com/mozilla_messaging).
1 On the real bug in bugzilla, you want to create a reference link to some user's getsatisfaction feedback, as it contains bug description, ideas for solving, improving or other useful information
2 try adding a reference URL for relevant getsatisfaction content to the "See also" field of the bug (e.g. http://getsatisfaction.com/mozilla_messaging/topics/relevant_topic)

Actual

cannot add getsatisfaction URLs to See Also
(although it's an official place where mozilla messaging expects people to report bugs)

Expected

allow adding getsatisfaction URLs to See Also (and reflect once again if it really makes sense to restrict this field to certain URLs only, see below)

Further information

I've seen a lot of bmo bugs where people from bug triage posted lists of related getsatisfaction links in comments, which is very ineffecient compared to adding them directly to the bug for reference. As a side note (as has been said before), we really need a field where we can add *any* URL that is relevant for triaging or solving a bug. "See also" field looks like a good place for that, and we can still have all the future API and normalization magic for supported URLs, while leaving any other URLs untouched as they are added.

Comment 1

[Frédéric Buclin]

Why don't you use the URL field for that? The See Also field specified "Add Bug URLs", so we really want to point to a bug report, not some random URL.

Comment 2

[Thomas D. (:thomas8)]

Frédéric, thx for quick reply.
(In reply to comment #1)
> Why don't you use the URL field for that?

1) Unfortunately, URL field does not allow *multiple* URLs to be added (they will not be linkified). E.g., as mentioned in comment 0, we often need to point to multiple getsatisfaction links to provide contextual information and an impression of the popularity of a bug/RFE. And there's no duplicate system at getsatisfaction that I'm aware of, so there's no other way of referencing them all-in-one.
2) Does it make sense to treat getsatisfaction bug reports different from other bug reports and link them in two different spots on the bug's page?
3) So far, my assumption was that the URL field is more for test cases etc., but I can't tell because there's no documentation about URL field in https://bugzilla.mozilla.org/page.cgi?id=fields.html

> The See Also field specified "Add Bug URLs", so we really want to point to a
> bug report, not some random URL.

I understand and appreciate that idea, but then:
- how do I link to bug reports from smaller bug systems that you have not yet listed? It doesn't seem very practical having to open a new bug for each system whose bugs we want to link to
- how do I link efficiently to multiple "bug reports", feature requests, solutions etc. that happen to be at some other place, like mozillazine, mozilla forums, hendrix, product reviews, private blogs etc?

This is not about adding "random URLs", it is about an efficient and scalable way of adding links that provide helpful information for triaging/solving a bug. For more than one link, this is currently not possible, and adding such resources somewhere buried in the comments won't be very useful, let alone posting the actual contents of such external sources into bugzilla comments.

Btw, I am not the first to propose that we need a way of referencing *any* URL (see Tanner M. Young bug 543667, comment 1 and bug 543667, comment 3). 

Notwithstanding, the focus of *this bug* is primarily to have at least getsatisfaction.com included into the list of allowed URLs for "See also".

Comment 3

[Thomas D. (:thomas8)]

Adding some interested parties to CC - Pls note the above comments about the need for/benefit of providing some way of adding multiple "any URLs" to a bug.

Let me add that I fully appreciate and look forward to Max Kanat Alexander's work on improving the functionality of the "see also" field by exploiting APIs etc. It would be nice to find a way how that work could co-exist with the option of adding any relevant URL to some field of the bug.

Comment 4

[timeless]

getsatisfaction appears to be a bug database, so it's a perfectly reasonable target for see also.

Comment 5

[Thomas D. (:thomas8)]

Roland, Wayne, ^^?

Comment 6

[Wayne Mery (:wsmwk)]

getsatisfaction is analogous to a summomo. however, in the context of the way we are using it,  is functioning as a database.  

Too early in the morning for me to definitively suggest whether we should be using URL field or "related bugs" for gfsn links, but related bugs feels more correct.  Note: take into consideration here, that we might evolve in the future to using a different support venue, and gfsn might not always be what we use

Comment 7

[Matt Selsky [:selsky]]

Attachment: Add support for getsatisfaction.com bugs

Comment 8

[Tiago Mello [:timello]]

Comment on attachment 513290 [details] [diff] [review]
Add support for getsatisfaction.com bugs

>
>=== modified file 'template/en/default/global/user-error.html.tmpl'
>--- template/en/default/global/user-error.html.tmpl	2011-02-16 17:41:08 +0000
>+++ template/en/default/global/user-error.html.tmpl	2011-02-17 23:26:07 +0000
>@@ -259,6 +259,7 @@
>         <li>A ticket in a Trac installation.</li>
>         <li>A b[% %]ug in a MantisBT installation.</li>
>         <li>A b[% %]ug on sourceforge.net.</li>
>+        <li>A b[% %]ug on getsatisfaction.com.</li>

They call 'bug' as 'problem'. So I think it should be 'problem' instead.

Comment 9

[Tiago Mello [:timello]]

Comment on attachment 513290 [details] [diff] [review]
Add support for getsatisfaction.com bugs

The small fix can be done in the checkin.

Thanks for the patch Matt! :-)

Comment 10

[Max Kanat-Alexander]

Comment on attachment 513290 [details] [diff] [review]
Add support for getsatisfaction.com bugs

You need to specify a "reason" for bug_url_invalid.

Also, as a side note, we should make "show_bug" the default reason if no reason is sent. (That is, make it the last ELSE and use that text if no reason is passed.)

Comment 11

[Matt Selsky [:selsky]]

Attachment: Add support for getsatisfaction.com bugs v2

Instead of "problem", I changed it to "topic" on the help page since that's what getsatisfaction seems to call it.

Also, I added the reason for bug_url_invalid, as requested.

Comment 12

[Matt Selsky [:selsky]]

Attachment: Make "show_bug" the default reason

Make show_bug the default reason by making it the final ELSE.

Comment 13

[Tiago Mello [:timello]]

Comment on attachment 518296 [details] [diff] [review]
Add support for getsatisfaction.com bugs v2

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

Could you merge both patches in one?

::: Bugzilla/BugUrl/GetSatisfaction.pm
@@ +22,5 @@
> +use strict;
> +use base qw(Bugzilla::BugUrl);
> +
> +use Bugzilla::Error;
> +use Bugzilla::Util;

You are not using any Bugzilla::Util routine.

@@ +48,5 @@
> +    } else {
> +        ThrowUserError('bug_url_invalid', { url => $value, reason => 'show_bug' });
> +    }
> +    my $topic_name;
> +    if ($uri->path =~ m|^/(?:[^/]+)/([^/]+)|) {

A comment would be nice here...

@@ +59,5 @@
> +    # always go with the HTTP scheme, as that's the default.
> +    $value = "http://getsatisfaction.com/" . $project_name .
> +             "/topics/" . $topic_name;
> +
> +    return new URI($value);

You should be explicit be importing URI module: 'use URI'

Comment 14

[Tiago Mello [:timello]]

Comment on attachment 518301 [details] [diff] [review]
Make "show_bug" the default reason

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

::: template/en/default/global/user-error.html.tmpl
@@ +251,4 @@
>        URLs must start with "http" or "https".
>      [% ELSIF reason == 'path_only' %]
>        You must specify a full URL.
> +    [% ELSIF reason == 'id' %]

This will break Bugzilla::BurUrl since it still uses 'show_bug' reason.

Comment 15

[Matt Selsky [:selsky]]

Attachment: Add support for getsatisfaction.com bugs and make show_bug the default reason v3

The patches have been merged into one.

> You should be explicit be importing URI module: 'use URI'

OK, added.  I was following the examples for LaunchPad, Debian, and Google modules.  Should I patch those files as well?

> This will break Bugzilla::BurUrl since it still uses 'show_bug' reason.

That is handled by the "else" case.  But I've remove the reference to 'show_bug' in Bugzilla::BugUrl to be more clear.

Comment 16

[Thomas D. (:thomas8)]

Tiago (review?), ping?

Having to wait more than 4 months to get a patch reviewed might be a discouraging experience for volunteers...

Comment 17

[Tiago Mello [:timello]]

(In reply to Thomas D. from comment #16)
> Tiago (review?), ping?
> 
> Having to wait more than 4 months to get a patch reviewed might be a
> discouraging experience for volunteers...

Hi Matt and Thomas, indeed. Sorry about that. I'm going to review them soon I also have other patches in the queue. Thanks for the reminder!

Comment 18

[Tiago Mello [:timello]]

Comment on attachment 553010 [details] [diff] [review]
Add support for getsatisfaction.com bugs and make show_bug the default reason v3

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

::: Bugzilla/BugUrl/GetSatisfaction.pm
@@ +46,5 @@
> +    if ($uri->path =~ m|^/([^/]+)/topics/([^/]+)|) {
> +        $project_name = $1;
> +        $topic_name = $2;
> +    } else {
> +        ThrowUserError('bug_url_invalid', { url => $value, reason => 'id' });

I think, instead of having this else, we could catch this pattern in the should_handle method above as we do for the others... so if the url does not match the right pattern the class_for will throw the error.

Comment 19

[Matt Selsky [:selsky]]

Attachment: Add support for getsatisfaction.com bugs and make show_bug the default reason v4

Patch updated as per comment 18.  One question, when would a reason of 'id' ever be used?  We now check the entire URL at once in should_handle.  Or should we break the check into two parts to be able to know that the user is trying to reference a certain type of BugUrl, but just getting the specific URL wrong?

Comment 20

[Matt Selsky [:selsky]]

Attachment: Add support for getsatisfaction.com bugs and make show_bug the default reason v5

Don't use classes we don't need.

Comment 21

[Tiago Mello [:timello]]

(In reply to Matt Selsky [:selsky] from comment #19)
> Created attachment 588802 [details] [diff] [review]
> Add support for getsatisfaction.com bugs and make show_bug the default
> reason v4
> 
> Patch updated as per comment 18.  One question, when would a reason of 'id'
> ever be used?  We now check the entire URL at once in should_handle.  Or
> should we break the check into two parts to be able to know that the user is
> trying to reference a certain type of BugUrl, but just getting the specific
> URL wrong?

BugUrl::Bugzilla currently uses reason => id and I think it makes more sense for Bugzilla BugUrl type but not for the others.
I think we just want to valid a certain URL and tell if it is one type or another and save that in the database. If in the future for any reason a specific error message is going to be needed, then we implement it.

Comment 22

[Tiago Mello [:timello]]

Comment on attachment 588807 [details] [diff] [review]
Add support for getsatisfaction.com bugs and make show_bug the default reason v5

>=== added file 'Bugzilla/BugUrl/GetSatisfaction.pm'
>+
>+sub should_handle {
>+    my ($class, $uri) = @_;
>+
>+    # GetSatisfaction URLs only have one form:
>+    #   http(s)://getsatisfaction.com/PROJECT_NAME/topics/TOPIC_NAME
>+    return ($uri->authority =~ /^getsatisfaction.com$/i
>+            and $uri->path =~ m|^/[^/]+/[^/]+/issues/\d+$|) ? 1 : 0;

The example form says 'topics' and the code 'issues'. What is the correct form?

Actually, I think this is wrong. It would accept something like:

http://getsatisfaction.com/AAA/BBB/issues/9999

is this the expected form? I think the example is right but the code is not.

Comment 23

[Matt Selsky [:selsky]]

Attachment: patch, v6

I had mistakenly copied and pasted the regex from the Github module.

As per https://getsatisfaction.com/devcommunity/topics/bringing_https_to_our_community_pages_in_detail SSL is now preferred so I updated _check_value to force the scheme to https.

Comment 24

[Frédéric Buclin]

Comment on attachment 609443 [details] [diff] [review]
patch, v6

Definitely a candidate for the extension.

Comment 25

[Matt Selsky [:selsky]]

Attachment: Add support for getsatisfaction.com bugs and make show_bug the default reason, v7

Moved to MoreBugUrl extension.

Comment 26

[Tiago Mello [:timello]]

Comment on attachment 609500 [details] [diff] [review]
Add support for getsatisfaction.com bugs and make show_bug the default reason, v7

r=timello, thanks for the patch!

Comment 27

[Frédéric Buclin]

This patch is going to conflict with bug 731727. Holding approval for now.

(and I still have some problems to consider getsatisfaction.com as a bug tracker)

Comment 28

[Matt Selsky [:selsky]]

Attachment: Add support for getsatisfaction.com bugs and make show_bug the default reason, v8

Patch updated to resolve conflicts with bug 731727.

Comment 29

[Frédéric Buclin]

The patch must be reviewed first.

Comment 30

[Tiago Mello [:timello]]

Comment on attachment 616125 [details] [diff] [review]
Add support for getsatisfaction.com bugs and make show_bug the default reason, v8

The patch applies cleanly, it passes in all tests and works as expected.

Thanks for the patch!

Comment 31

[Tiago Mello [:timello]]

Committing to: bzr+ssh://[e-mail]/bugzilla/trunk/                                                                                                                                    
modified Bugzilla/BugUrl.pm
modified extensions/MoreBugUrl/Extension.pm
added extensions/MoreBugUrl/lib/GetSatisfaction.pm                                                                                                                                                              
modified extensions/MoreBugUrl/template/en/default/hook/global/user-error-bug_url_invalid_tracker.html.tmpl
modified template/en/default/global/user-error.html.tmpl
Committed revision 8218.

Comment 32

[Frédéric Buclin]

Added to relnotes for 4.4.