870020 - Multiple "oh no! an error occurred" errors on accessing a pending app

Status: RESOLVED FIXED

(Marketplace Graveyard :: Consumer Pages, defect, P2)

Description

[krupa raj[:krupa]]

Attachment: screenshot

steps to reproduce:
1. Pick an app which is pending review
2. As an anonymous user, load the details page for that app or load https://marketplace-altdev.allizom.org/app/test-app-bav1/

expected behavior:
We show this app is awaiting review like we do on -dev @ https://marketplace-dev.allizom.org/app/mozillaball-2/

observed behavior:
We show "oh no! an error occured" multiple times. See screenshot

Comment 1

[Wil Clouser [:clouserw]]

-> p3 because this only shows up if you don't have access (which means someone sent you a URL or something)

Comment 2

[Andrew Williamson [:eviljeff]]

(In reply to Wil Clouser [:clouserw] from comment #1)
> -> p3 because this only shows up if you don't have access (which means
> someone sent you a URL or something)

It happens even when you do have access too.  Is a really bad experience for a developer to try to access their app's page and get an error page rather than an explanation and links to how they can resubmit, etc.
It should be higher than a p3.

Comment 4

[Rob Hudson [:robhudson]]

The "Oh no!" is a 403, not a 500.

I think that the API returns 403 for rejected and pending apps is appropriate, but fireplace just needs to handle this a little better in this case.

If the user owns the app they do get to see the object, e.g.:
http://cl.ly/image/3y382k1n3a0O

But this differs from old behavior in that we don't show the links to manage the app like we used to.
http://cl.ly/image/06151w0b0f3l
We should probably look at user.developed in the API response to show these buttons.

We are also not checking reviewers or admins in the API and old behavior was to let reviewers and admins see this.

Comment 5

[Andrew Williamson [:eviljeff]]

(In reply to Rob Hudson [:robhudson] from comment #4)
> The "Oh no!" is a 403, not a 500.
> 
> I think that the API returns 403 for rejected and pending apps is
> appropriate, but fireplace just needs to handle this a little better in this
> case.

does it return anything to differentiate the two states like we see at the moment?

> If the user owns the app they do get to see the object, e.g.:
> http://cl.ly/image/3y382k1n3a0O
> 
> But this differs from old behavior in that we don't show the links to manage
> the app like we used to.
> http://cl.ly/image/06151w0b0f3l
> We should probably look at user.developed in the API response to show these
> buttons.
> 
> We are also not checking reviewers or admins in the API and old behavior was
> to let reviewers and admins see this.

yes, we need to be able to see what the developer sees at least

Comment 6

[Rob Hudson [:robhudson]]

(In reply to Andrew Williamson [:eviljeff] from comment #5)
> does it return anything to differentiate the two states like we see at the
> moment?

What two states are those?

> yes, we need to be able to see what the developer sees at least

I filed bug 873150 for the API.

Comment 7

[Andrew Williamson [:eviljeff]]

(In reply to Rob Hudson [:robhudson] from comment #6)
> (In reply to Andrew Williamson [:eviljeff] from comment #5)
> > does it return anything to differentiate the two states like we see at the
> > moment?
> 
> What two states are those?

Rejected and pending.  And probably disabled too - different messages are displayed currently which explains the app's state and why they can't see it (if not logged in or authorised)

> > yes, we need to be able to see what the developer sees at least
> 
> I filed bug 873150 for the API.

Comment 8

[Andrew Williamson [:eviljeff]]

Now the page isn't a lot of oops errors, but is the full listing instead, with no indication the app hasn't been approved.  Installation fails, but without a useful error message.

Comment 9

[Andrew Williamson [:eviljeff]]

(In reply to Andrew Williamson [:eviljeff] from comment #8)
> Now the page isn't a lot of oops errors, but is the full listing instead,
> with no indication the app hasn't been approved.  Installation fails, but
> without a useful error message.

Looks like I was hitting the bug where I wasn't logged out when I selected log out.  After deleting cookies I see the oops-es again.

Comment 10

[Rob Hudson [:robhudson]]

The API needs to return additional data along with the 403 that Fireplace can use to display why the app was 403'd for that user.

Here's reference to how we used to handle this:
https://github.com/mozilla/zamboni/blob/master/mkt/detail/templates/detail/protected_app.html#L67

It looks like we'd need to pass along the status and disabled_by_user state also.

Comment 11

[Davor Spasovski [:spasovski]]

This commit addressed the bug but not robhudson's comment.

https://github.com/mozilla/fireplace/commit/eb0f551

I'll mark this fixed and we can open a new bug for a 403 reason. Basta does this sound like the `except 451` we talked about?

Comment 12

[Davor Spasovski [:spasovski]]

The other bug for response 451 is: https://bugzilla.mozilla.org/show_bug.cgi?id=862037

Comment 13

[krupa raj[:krupa]]

This doesn't look good. See https://marketplace-dev.allizom.org/app/facebook-2/?lang=en

See screenshot

Comment 14

[krupa raj[:krupa]]

Attachment: screenshot for reopening

Comment 15

[Andrew Williamson [:eviljeff]]

I don't agree this bug is fixed until the 403 reason is displayed.  Similar to bug 877494, a lot of extra and useful information was thrown away when the switch was made to Fireplace and it needs to come back.

Comment 16

[Davor Spasovski [:spasovski]]

https://github.com/mozilla/fireplace/commit/b4ab92f