422975 - Don't require login to access sandboxed add-ons' details pages

Status: VERIFIED FIXED

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

Description

[Fred Wenzel [:wenzel]]

As agreed on the AMO admin mailing list, we are going to open sandboxed add-ons' details pages for public access. The installation should stay behind a login as well as the other user-only actions such as writing reviews.

Comment 1

[Michael Morgan [:morgamic]]

Need to remove status checks for everything except the download controller.

Comment 2

[Chris Pollett]

Attachment: patch to disable sandbox check for details

This patch provides the functionality wenzel asked for. As you can see I just commented out the if-else-check. I wasn't sure if this might be added back later or not, so I didn't outright delete the code. If people are happy with the patch functionality, I can check in the code either way. That is, with the commented out section, or where I remove the code.

Comment 3

[Michael Morgan [:morgamic]]

Comment on attachment 310029 [details] [diff] [review]
patch to disable sandbox check for details

Hey Chris, this works but we should not have to comment code out like that (svn is there for that).  Also, looks like we need to do two things as a part of this:
1. add the experimental flag to the feature box
2. fix the style for the install button?

Could you look into those two issues?

Comment 4

[Fred Wenzel [:wenzel]]

I concur with Mike -- if it's right to delete something, do so. If we need to get it back, that's what version control is for.

re: 2. fix the style for the install button?
I do think the install widget will do this automagically, but please confirm that when you look at it.

Comment 5

[Chris Pollett]

Attachment: This patch address commented out code add experimental notice to sandbox addons

Okay, I removed the commented out code. Then I added the flag to indicate as experimental when a viewer is looking at a sandboxed. The style issue with the download button was addressed by previous css changes of morgamic.

Comment 6

[Fred Wenzel [:wenzel]]

Comment on attachment 310284 [details] [diff] [review]
This patch address commented out code add experimental notice to sandbox addons

Actually, that's the same patch as the first one, I think you accidentally uploaded the old file again. Can you check?

Comment 7

[Chris Pollett]

Attachment: correct patch

sorry about that. here is the correct patch.

Comment 8

[Fred Wenzel [:wenzel]]

Comment on attachment 310309 [details] [diff] [review]
correct patch

Okay it looks pretty good but I still see a few problems:

- I don't understand what adding the addon status in the home page view (home.thtml) has to do with this patch, but maybe I am missing something? If it's unrelated, please make that change in a different commit.
- in the addons controller you should remove the sandbox access check altogether because it is not needed to determine access to the display page anymore. this->publish and this->status are not needed either. (Then of course you need to change $addonStatus in the view to $addon...['status']).
- the comment "// is addon recommended or experimental?" should be changed or removed since you are not actually checking for recommended status there.
- Another thing I noticed is that the install button on experimental addons pages is green instead of white (= disabled look). Are these the CSS issues that you mentioned earlier? Also, the "log in to install" words are off (I will attach a screenshot shortly).
- really not a biggie, but please indent the code correctly so that it fits in with the rest, for example where you deleted the if clause, and in the display.thtml view.
- additionally (not something you missed but not needed anymore) you should remove the if clause that defines "/status:1" as a URL part on the details page (and possible places in that view that use it); since we have removed the strong distinction between public and sandbox, this has done its deed.

Comment 9

[Fred Wenzel [:wenzel]]

Attachment: install button style probs on experimental add-ons page

This is the button style and "log in" link problem I talked about in my previous comment.

Comment 10

[Chris Pollett]

This line 
+            'addonStatus' => $feature1['Addon']['status'],                     
needed to be added to home controllers to handle the case where a sandboxed item
was the main featured item. This makes the experimental word show up.

I work on supplying a patch to address the other points shortly. The main machine I do development on died today and I have been trying to resurrect it.

Comment 11

[Chris Pollett]

I meant home.thtml in the last comment not home controller.

Comment 12

[Chris Pollett]

Attachment: addresses issues of last review

Comment 13

[Chris Pollett]

Attachment: deletes something asked to delete but forgot in last attachment

Here are some remarks on how comments #8 questions were addressed:

Point 1: I indicated in my last post why the changes to home.thtml were done

Point 2: Removing publishing the addonStatus, actually makes the code in the display and install elements uglier, so I opted for leaving this.

Point 3: I deleted this as requested

Point 4: The install button should look right now. There is one use of an inline style. This is because this copies what the javascript in the addon.js function addCompatibilityHints outputs. This function didn't seem to be
be immediately adjustable to the present circumstance, and we really need just
a grayed-out button, not to do any calculation of version or the like before this happens.

Point 5: did try to indent code, use spaces for tabs this time.
Point 6: this was removed as requested.

Comment 14

[Fred Wenzel [:wenzel]]

Comment on attachment 310856 [details] [diff] [review]
deletes something asked to delete but forgot in last attachment

With the exception of a debug statement (echo $addon_data['Addon']['status'];) that you should still remove before checking in, I can r+ this now.

The inline CSS isn't that great either, but I think this has to change for bug 424229 anyway (in addons.js and elsewhere), so you don't need to cover it here.

Thanks, Chris.

Comment 15

[Chris Pollett]

Just checked it in. Thanks for reviewing it Fred.

Comment 16

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

Verified FIXED; tested around this with Firefox 2/3, Opera 9.25, Safari 3.1, and IE 7; I suck for not finding bug 42489; thanks for Madhava for filing!