28.28 KB, image/jpeg
7.16 KB, patch
|Details | Diff | Splinter Review|
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
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-
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.
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)
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-
Created attachment 310309 [details] [diff] [review] correct patch sorry about that. here is the correct patch.
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-
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.
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.
I meant home.thtml in the last comment not home controller.
Created attachment 310854 [details] [diff] [review] addresses issues of last review
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+
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
Product: addons.mozilla.org → addons.mozilla.org Graveyard
You need to log in before you can comment on or make changes to this bug.