Last Comment Bug 870020 - Multiple "oh no! an error occurred" errors on accessing a pending app
: Multiple "oh no! an error occurred" errors on accessing a pending app
Status: RESOLVED FIXED
[fireplace]
: regression
Product: Marketplace
Classification: Server Software
Component: Consumer Pages (show other bugs)
: 1.0
: All All
: P2 normal (vote)
: 2013-06-06
Assigned To: Davor Spasovski [:spasovski]
:
:
Mentors:
https://marketplace-altdev.allizom.or...
: 862037 873019 880455 (view as bug list)
Depends on: 873150
Blocks: 859511
  Show dependency treegraph
 
Reported: 2013-05-08 10:51 PDT by krupa raj[:krupa]
Modified: 2013-06-17 17:15 PDT (History)
8 users (show)
See Also:
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
screenshot (281.22 KB, image/png)
2013-05-08 10:51 PDT, krupa raj[:krupa]
no flags Details
screenshot for reopening (362.04 KB, image/png)
2013-06-05 20:58 PDT, krupa raj[:krupa]
no flags Details

Description krupa raj[:krupa] 2013-05-08 10:51:40 PDT
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
Comment 1 Wil Clouser [:clouserw] 2013-05-15 11:15:35 PDT
-> 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] 2013-05-16 06:22:45 PDT
(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 3 Andrew Williamson [:eviljeff] 2013-05-16 06:25:43 PDT
*** Bug 873019 has been marked as a duplicate of this bug. ***
Comment 4 Rob Hudson [:robhudson] 2013-05-16 10:08:52 PDT
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] 2013-05-16 10:52:45 PDT
(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] 2013-05-16 11:06:16 PDT
(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] 2013-05-16 11:12:52 PDT
(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] 2013-05-23 07:31:19 PDT
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] 2013-05-23 07:56:59 PDT
(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] 2013-05-23 08:21:16 PDT
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] 2013-06-05 15:52:02 PDT
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] 2013-06-05 17:16:26 PDT
The other bug for response 451 is: https://bugzilla.mozilla.org/show_bug.cgi?id=862037
Comment 13 krupa raj[:krupa] 2013-06-05 20:58:01 PDT
This doesn't look good. See https://marketplace-dev.allizom.org/app/facebook-2/?lang=en

See screenshot
Comment 14 krupa raj[:krupa] 2013-06-05 20:58:37 PDT
Created attachment 758959 [details]
screenshot for reopening
Comment 15 Andrew Williamson [:eviljeff] 2013-06-06 02:58:06 PDT
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] 2013-06-14 14:48:46 PDT
https://github.com/mozilla/fireplace/commit/b4ab92f
Comment 17 Davor Spasovski [:spasovski] 2013-06-17 17:05:57 PDT
*** Bug 862037 has been marked as a duplicate of this bug. ***
Comment 18 Davor Spasovski [:spasovski] 2013-06-17 17:15:36 PDT
*** Bug 880455 has been marked as a duplicate of this bug. ***

Note You need to log in before you can comment on or make changes to this bug.