501541 - Breadcrumb on meet the developer points to firefox for thunderbird add-ons

Status: VERIFIED FIXED

(addons.mozilla.org Graveyard :: Public Pages, defect, P3)

Description

[Jeff Balogh (:jbalogh)]

Redirection to other products doesn't work, so going to a thunderbird
add-on still says "add-ons for firefox" at the top.

clouserw said you'd handled this before, so assigning to wenzel for 5.0.8.

Comment 1

[Fred Wenzel [:wenzel]]

Attachment: Patch, rev. 1

Here you go.

Comment 2

[Jeff Balogh (:jbalogh)]

Comment on attachment 389900 [details] [diff] [review]
Patch, rev. 1

1. In the function `developers`, a loop overwrites the variable `$addon` before you publish it so the wrong addon is getting published.  I'd change the loop variable to $_addon.
2. Returning after app_redirect was really confusing to me.  The redirect feels like it should be the last thing happening in a return statement.  What do you think about returning $targetapp from app_redirect and having something like the following?

    $redirect = $this->_app_redirect($compatible_apps);
    if ($redirect) {
        $this->redirect($redirect.$rest_of_url);
    }

3. Can you write a test for app_redirect?

Comment 3

[Fred Wenzel [:wenzel]]

(In reply to comment #2)
> (From update of attachment 389900 [details] [diff] [review])
> 1. In the function `developers`, a loop overwrites the variable `$addon` before
> you publish it so the wrong addon is getting published.  I'd change the loop
> variable to $_addon.

Thanks.

> 2. Returning after app_redirect was really confusing to me.  The redirect feels
> like it should be the last thing happening in a return statement.  What do you
> think about returning $targetapp from app_redirect and having something like
> the following?

Sounds good. I didn't like it either that I had to return from the calling function again. I find it curious that Cake requires you to return yourself anyway, but that's a different story...

> 3. Can you write a test for app_redirect?

Sure.

Comment 4

[Fred Wenzel [:wenzel]]

Attachment: Patch, rev. 2

Newer, fresher, now with twice as much redirection awesomeness.

Comment 5

[Jeff Balogh (:jbalogh)]

Comment on attachment 390173 [details] [diff] [review]
Patch, rev. 2

thanks!

Comment 6

[Fred Wenzel [:wenzel]]

r30330.

Comment 7

[Stephen Donner [:stephend] Not actively reading bugmail]

Verified FIXED:

GET /en-US/firefox/addon/5326/developers HTTP/1.1[CRLF]
Host: preview.addons.mozilla.org[CRLF]

<snip>

HTTP Status Code: HTTP/1.0 302 Found
Date:	Fri, 24 Jul 2009 08:52:24 GMT	
Server:	Apache/2.2.3 (Red Hat)	
X-Powered-By:	PHP/5.1.6	
X-AMO-ServedBy:	pm-app-amo15	
Location:	https://preview.addons.mozilla.org/en-US/thunderbird/addon/5326/developers

Comment 8

[Stephen Donner [:stephend] Not actively reading bugmail]

(Er, and ending up with the final page after the redirect, the breadcrumb is correct.)

Comment 9

[Fred Wenzel [:wenzel]]

Thanks, Stephen!