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

VERIFIED FIXED in 3.2

Status

addons.mozilla.org Graveyard
Public Pages
VERIFIED FIXED
10 years ago
2 years ago

People

(Reporter: wenzel, Assigned: Chris Pollett)

Tracking

Details

Attachments

(2 attachments, 4 obsolete attachments)

(Reporter)

Description

10 years ago
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.
Need to remove status checks for everything except the download controller.
Assignee: nobody → cpollett
(Assignee)

Comment 2

10 years ago
Created attachment 310029 [details] [diff] [review]
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.
Attachment #310029 - Flags: review?
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?
Attachment #310029 - Flags: review? → review-
(Reporter)

Comment 4

10 years ago
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.
(Assignee)

Comment 5

10 years ago
Created attachment 310284 [details] [diff] [review]
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.
Attachment #310284 - Flags: review?(fwenzel)
(Reporter)

Comment 6

10 years ago
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?
Attachment #310284 - Flags: review?(fwenzel) → review-
(Assignee)

Comment 7

10 years ago
Created attachment 310309 [details] [diff] [review]
correct patch

sorry about that. here is the correct patch.
Attachment #310029 - Attachment is obsolete: true
Attachment #310284 - Attachment is obsolete: true
Attachment #310309 - Flags: review?(fwenzel)
(Reporter)

Comment 8

10 years ago
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.
Attachment #310309 - Flags: review?(fwenzel) → review-
(Reporter)

Comment 9

10 years ago
Created attachment 310320 [details]
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.
(Assignee)

Comment 10

10 years ago
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.
(Assignee)

Comment 11

10 years ago
I meant home.thtml in the last comment not home controller.
(Assignee)

Comment 12

10 years ago
Created attachment 310854 [details] [diff] [review]
addresses issues of last review
Attachment #310309 - Attachment is obsolete: true
Attachment #310854 - Flags: review?(fwenzel)
(Assignee)

Comment 13

10 years ago
Created attachment 310856 [details] [diff] [review]
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.
Attachment #310854 - Attachment is obsolete: true
Attachment #310856 - Flags: review?(fwenzel)
Attachment #310854 - Flags: review?(fwenzel)
(Assignee)

Updated

10 years ago
Attachment #310856 - Attachment is patch: true
Attachment #310856 - Attachment mime type: application/octet-stream → text/plain
(Reporter)

Comment 14

10 years ago
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.
Attachment #310856 - Flags: review?(fwenzel) → review+
(Assignee)

Comment 15

10 years ago
Just checked it in. Thanks for reviewing it Fred.
Status: NEW → RESOLVED
Last Resolved: 10 years ago
Resolution: --- → FIXED
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!
Status: RESOLVED → VERIFIED
(Reporter)

Updated

10 years ago
Duplicate of this bug: 420279
Product: addons.mozilla.org → addons.mozilla.org Graveyard
You need to log in before you can comment on or make changes to this bug.