see also should support appspot bug urls

RESOLVED FIXED in Bugzilla 4.4

Status

()

--
enhancement
RESOLVED FIXED
8 years ago
6 years ago

People

(Reporter: timeless, Assigned: selsky)

Tracking

(Depends on: 1 bug)

3.6.3
Bugzilla 4.4
Bug Flags:
approval +

Details

Attachments

(1 attachment, 5 obsolete attachments)

(Reporter)

Description

8 years ago
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
Assignee: nobody → create-and-change
Component: Bugzilla: Other b.m.o Issues → Creating/Changing Bugs
Product: mozilla.org → Bugzilla
QA Contact: other-bmo-issues → default-qa
Version: other → 3.6.3
(Assignee)

Comment 1

8 years ago
Created attachment 513281 [details] [diff] [review]
Add support for appspot.com bugs
Assignee: create-and-change → selsky
Status: NEW → ASSIGNED
Attachment #513281 - Flags: review?(timello)

Comment 2

8 years ago
This looks like a code review tool, actually, not a bug-tracker. We should still be able to link to it, though.
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.
Attachment #513281 - Flags: review?(timello) → review-
(Assignee)

Comment 4

8 years ago
Created attachment 513821 [details] [diff] [review]
Add support for breakpad.appspot.com bugs v2
Attachment #513281 - Attachment is obsolete: true
Attachment #513821 - Flags: review?(timello)

Comment 5

8 years ago
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.
(Reporter)

Comment 6

8 years ago
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.
(Assignee)

Comment 7

8 years ago
These are all instances of Rietveld (http://code.google.com/p/rietveld/), correct?
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.
Attachment #513821 - Flags: review?(timello) → review-
(Assignee)

Comment 9

8 years ago
Created attachment 518396 [details] [diff] [review]
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?
Attachment #513821 - Attachment is obsolete: true
Attachment #518396 - Flags: review?(timello)

Comment 10

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

Comment 12

8 years ago
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).
(Assignee)

Comment 13

8 years ago
Created attachment 518660 [details] [diff] [review]
Add support for Rietveld installations on appspot.com v3

Convert URLs to shortest form.
Attachment #518396 - Attachment is obsolete: true
Attachment #518660 - Flags: review?(timello)
Attachment #518396 - Flags: review?(timello)
(Assignee)

Updated

8 years ago
Blocks: 661533
(Assignee)

Updated

8 years ago
No longer blocks: 661533
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;
Attachment #518660 - Flags: review?(timello) → review-
(Assignee)

Comment 15

7 years ago
Created attachment 552847 [details] [diff] [review]
Add support for Rietveld installations on appspot.com v4

Patch updated as per comments.
Attachment #518660 - Attachment is obsolete: true
Attachment #552847 - Flags: review?(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!
Attachment #552847 - Flags: review?(timello) → review+

Updated

7 years ago
Flags: approval?

Updated

7 years ago
Target Milestone: --- → Bugzilla 5.0

Comment 17

7 years ago
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

7 years ago
Canceling request for approval as the patch must be updated.
Flags: approval?
Keywords: relnote
(Assignee)

Comment 19

7 years ago
Created attachment 586376 [details] [diff] [review]
Add support for Rietveld installations on appspot.com v5

Updated as requested.
Attachment #552847 - Attachment is obsolete: true
Attachment #586376 - Flags: review?(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!
Attachment #586376 - Flags: review?(timello) → review+

Updated

7 years ago
Flags: approval?

Updated

7 years ago
Flags: approval? → approval+
Committing to: bzr+ssh://timello%40gmail.com@bzr.mozilla.org/bugzilla/trunk/                                                                                                                                    
modified Bugzilla/BugUrl.pm
added Bugzilla/BugUrl/Rietveld.pm
modified template/en/default/global/user-error.html.tmpl
Committed revision 8097.
Status: ASSIGNED → RESOLVED
Last Resolved: 7 years ago
Resolution: --- → FIXED

Comment 22

6 years ago
Added to relnotes for 4.4.
Keywords: relnote
You need to log in before you can comment on or make changes to this bug.