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

RESOLVED FIXED in 2013-06-06

Status

Marketplace
Consumer Pages
P2
normal
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: krupa, Assigned: spasovski)

Tracking

({regression})

2013-06-06
regression
Points:
---
Dependency tree / graph

Details

(Whiteboard: [fireplace], URL)

Attachments

(2 attachments)

(Reporter)

Description

4 years ago
Created attachment 747034 [details]
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
Summary: Multiple "oh no! an error occurred" errors on accessing a pending review → Multiple "oh no! an error occurred" errors on accessing a pending app
Priority: -- → P2
-> p3 because this only shows up if you don't have access (which means someone sent you a URL or something)
Priority: P2 → P3
(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.
Duplicate of this bug: 873019
Keywords: regression
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.
(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
(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.
(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.
Priority: P3 → P2
Depends on: 873150
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.
(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.
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.
Target Milestone: --- → 2013-05-30
Assignee: nobody → dspasovski
Target Milestone: 2013-05-30 → ---
(Assignee)

Comment 11

4 years ago
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?
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
(Assignee)

Comment 12

4 years ago
The other bug for response 451 is: https://bugzilla.mozilla.org/show_bug.cgi?id=862037
Target Milestone: --- → 2013-06-06
(Reporter)

Comment 13

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

See screenshot
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(Reporter)

Comment 14

4 years ago
Created attachment 758959 [details]
screenshot for reopening
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.
(Assignee)

Comment 16

4 years ago
https://github.com/mozilla/fireplace/commit/b4ab92f
Status: REOPENED → RESOLVED
Last Resolved: 4 years ago4 years ago
Resolution: --- → FIXED
(Assignee)

Updated

4 years ago
Duplicate of this bug: 862037
(Assignee)

Updated

4 years ago
Duplicate of this bug: 880455
You need to log in before you can comment on or make changes to this bug.