617802 - see also should support appspot bug urls

Status: RESOLVED FIXED

(Bugzilla :: Creating/Changing Bugs, enhancement)

Description

[timeless]

bug 590961 comment 16 is a reference to an upstream issue tracker:
http://breakpad.appspot.com/241001/show

please enable see also to support it

Comment 1

[Matt Selsky [:selsky]]

Attachment: Add support for appspot.com bugs

Comment 2

[Max Kanat-Alexander]

This looks like a code review tool, actually, not a bug-tracker. We should still be able to link to it, though.

Comment 3

[Tiago Mello [:timello]]

Comment on attachment 513281 [details] [diff] [review]
Add support for appspot.com bugs

The web application is BreakPad and not Appspot. So it should be BreakPad instead of Appspot. Appspot is a platform for development and hosting provide by Google. If we want to allow all urls from appspot.com we need change it to something more generic.

Comment 4

[Matt Selsky [:selsky]]

Attachment: Add support for breakpad.appspot.com bugs v2

Comment 5

[Max Kanat-Alexander]

Ah, I don't think that this is what we want. Breakpad is not a bug tracker, it's a library for sending crash reports. Are we looking at a bug tracker just for the Breakpad library? I wouldn't accept that.

Comment 6

[timeless]

http://codereview.appspot.com/1955042/show is another instance of the pattern. so this isn't limited to breakpad.

http://gwt-code-reviews.appspot.com/160801/show is another instance

http://page-speed-codereview.appspot.com/77001/show is another

this is clearly a set pattern, not just once instance.

Comment 7

[Matt Selsky [:selsky]]

These are all instances of Rietveld (http://code.google.com/p/rietveld/), correct?

Comment 8

[Tiago Mello [:timello]]

Comment on attachment 513821 [details] [diff] [review]
Add support for breakpad.appspot.com bugs v2

So, we don't want Breakpad... we need to figure out what the best name for it... by the comment #6 the it would be Appspot then.

Comment 9

[Matt Selsky [:selsky]]

Attachment: Add support for Rietveld installations on appspot.com

All of the above URLs are for Rietveld instances installed on appspot.com.  The attached patch detects these.  Rietveld can be installed on a non-appspot.com server if you can meet all of the Django pre-requisites yourself.  Do we want to support that as well?

Do we want to support non-Rietveld URLs on appspot.com?

Comment 10

[Max Kanat-Alexander]

I think it's a "review request" or a "code review" in Rietveld, not an "issue", no?

Comment 11

[Matt Selsky [:selsky]]

http://code.google.com/p/rietveld/source/browse/trunk/templates/base.html says "issue".

Comment 12

[Max Kanat-Alexander]

Works for me. :-) You're also going to want to be sure that you convert all input URLs to one canonical form (preferably the shortest form).

Comment 13

[Matt Selsky [:selsky]]

Attachment: Add support for Rietveld installations on appspot.com v3

Convert URLs to shortest form.

Comment 14

[Tiago Mello [:timello]]

Comment on attachment 518660 [details] [diff] [review]
Add support for Rietveld installations on appspot.com v3

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

When trying to add a new see also entry:

Can't modify non-lvalue subroutine call at Bugzilla/BugUrl/Rietveld.pm line 50.

This is because you are trying to assign a new value in:

$uri->path = '/' . $1;

Comment 15

[Matt Selsky [:selsky]]

Attachment: Add support for Rietveld installations on appspot.com v4

Patch updated as per comments.

Comment 16

[Tiago Mello [:timello]]

Comment on attachment 552847 [details] [diff] [review]
Add support for Rietveld installations on appspot.com v4

Patch applies cleanly! The given example in the comment just doesn't work because we are restricting it to appspot.com, but that can be fixed in the checkin.

Thanks Matt!

Comment 17

[Frédéric Buclin]

Comment on attachment 552847 [details] [diff] [review]
Add support for Rietveld installations on appspot.com v4

>=== added file 'Bugzilla/BugUrl/Rietveld.pm'

>+# The contents of this file are subject to the Mozilla Public
>+# License Version 1.1 (the "License");

You must now use MPL 2.0 for each new file. See bug 680131 comment 22 for the exact license header to use with the Bugzilla project.


Also, this patch doesn't apply cleanly due to the commit of bug 661533.

Comment 18

[Frédéric Buclin]

Canceling request for approval as the patch must be updated.

Comment 19

[Matt Selsky [:selsky]]

Attachment: Add support for Rietveld installations on appspot.com v5

Updated as requested.

Comment 20

[Tiago Mello [:timello]]

Comment on attachment 586376 [details] [diff] [review]
Add support for Rietveld installations on appspot.com v5

Nit: we are not using the first line in the header anymore, but that can be fixed in the checkin.

Thanks Matt!

Comment 21

[Tiago Mello [:timello]]

Committing to: bzr+ssh://[e-mail]/bugzilla/trunk/                                                                                                                                    
modified Bugzilla/BugUrl.pm
added Bugzilla/BugUrl/Rietveld.pm
modified template/en/default/global/user-error.html.tmpl
Committed revision 8097.

Comment 22

[Frédéric Buclin]

Added to relnotes for 4.4.