456139 - [W-1.5.1] Publishing (from AMO display pages)

Status: RESOLVED FIXED

(addons.mozilla.org Graveyard :: Collections, defect)

Description

[Justin Scott [:fligtar]]

Users logged into AMO with collections linked to their account will see a dropdown listing on each add-on’s display page for publishing or “bookmarking” add-ons to collections from the web.

Comment 1

[Justin Scott [:fligtar]]

->M4

Will add details and assignee closer to then.

Comment 2

[Justin Scott [:fligtar]]

Attachment: mockup, v1

First crack at mockup for this and bug 456140. Couple of issues:
1) The section is labeled "Collections". I feel like we should be more specific in saying that these are collections the add-on is in, but that's a bit wordy for the heading.
2) I said that if the user isn't logged in, we shouldn't show the publisher select box. I feel like not every user that visits every add-on should be presented with this extra component that may or may not be obvious that creating a collection requires an account and a bit of a process. We want to make collections discoverable and get people to create them, but I sorta feel like having this list of collections on every add-on page and linking to the directory where we heavily promote creating your own collection is sufficient.

Thoughts on these issues?

Comment 3

[Justin Scott [:fligtar]]

Moving to revised M5 milestone (4/28).

Comment 4

[Justin Scott [:fligtar]]

Nick, thoughts on comment #2?

Comment 5

[Nick Nguyen [:osunick]]

It should say "Related Collections" or something, I think.  I think hiding the publisher box is fine since the collections directory and listings seem like better places to introduce this feature to new users.

Comment 6

[Justin Scott [:fligtar]]

Okay, this is ready for implementation then, with the change of "Collections" header to "Related Collections".

Shouldn't really need any design love for this, and I suspect we'll make it pretty fancy in the upcoming add-on display page redesign anyway.

Comment 7

[Justin Scott [:fligtar]]

Moving to next milestone since late feedback cut this one too close.

Comment 8

[Fred Wenzel [:wenzel]]

The "and 335 more extensions" link points where? I don't think there's a page for this yet.

Comment 9

[Justin Scott [:fligtar]]

Hmm, that's a good point. Filed bug 491407.

Comment 10

[Fred Wenzel [:wenzel]]

Attachment: Publishing to existing collections

This time you get two patches that are hopefully more easily digestible. Note that all of this code is based on the "collection management" patch from bug 456132, as the AJAX API from there can be used here.

This patch allows adding an add-on to existing collections via its display page.

Comment 11

[Fred Wenzel [:wenzel]]

Attachment: Publishing to a new collection

This second patch is based on the first one and adds the functionality for publishing an add-on to a *new* collection.

Comment 12

[Fred Wenzel [:wenzel]]

Comment on attachment 375908 [details] [diff] [review]
Publishing to existing collections

Jeff, do you mind looking at this? Don't want to bury you in work though, so tell me if it's too much.

Also note that the patches in this bug also cover bug 456140, which I accidentally solved all in one piece, as I worked off the mockup which contains both features at once.

Comment 13

[Fred Wenzel [:wenzel]]

Comment on attachment 375908 [details] [diff] [review]
Publishing to existing collections

Right when I ask you to review, I need to make amendments. Sorry. Obsoleting these attachments, and I'll get back to you when I have included fixes for bug 492654 and bug 492663.

Comment 14

[Fred Wenzel [:wenzel]]

Attachment: Patch, rev. 2

This fixes:
- this bug (adding addons to collections from display pages, to new and existing collections), including non-JS fallback.
- bug 492663 (does not show "auto_publisher" collections)
- and it obsoletes and removes the addtocollection page (bug 492654).

Please note that it is based on the generic notification code from bug 492689.

Comment 15

[Fred Wenzel [:wenzel]]

(In reply to comment #14)
> Created an attachment (id=377074) [details]

I forgot: It also (still) includes the "popular collections" code, fixing bug 492663.

Comment 16

[Fred Wenzel [:wenzel]]

Fligtar: Will this also need to trigger the bandwagon refresh on successfully adding a addon to a collection?

jbalogh: Please feel free to proceed reviewing, this question does not significantly alter the code.

Comment 17

[Jeff Balogh (:jbalogh)]

Comment on attachment 377074 [details] [diff] [review]
Patch, rev. 2

Thanks for picking this up Wil!

Comment 18

[Wil Clouser [:clouserw]]

Comment on attachment 377074 [details] [diff] [review]
Patch, rev. 2

This is awesome!  Couple thoughts:

> +        $this->redirect("/addon/{$this->data['addon_id']}");
When you use $this->data a few lines above this you urlencode().  I couldn't exploit this when I tried but maybe it's a potential problem?

I think there should be a "what's this?" link next to "add to a collection" - particularly if they have no collections.  Fligtar, nick: any objections?

Is it possible to style the "Select a collection..." line in the dropdown to make it more apparent that it's not an option you can choose?  Even if it only works in FF3.  If nothing else, maybe just a separator between it an the options?  I'm just throwing out ideas.

When I try to save without JS on I get "Element Not Found: /home/clouserw/public_html/remora/site/app/views/elements/notification.thtml"

Comment 19

[Fred Wenzel [:wenzel]]

(In reply to comment #18)
> (From update of attachment 377074 [details] [diff] [review])
> This is awesome!  Couple thoughts:
> 
> > +        $this->redirect("/addon/{$this->data['addon_id']}");
> When you use $this->data a few lines above this you urlencode().  I couldn't
> exploit this when I tried but maybe it's a potential problem?

I am now explicitly checking for the addon ID to be numeric. The argument was properly escaped before, but this is just cleaner. Thanks.

> I think there should be a "what's this?" link next to "add to a collection" -
> particularly if they have no collections.  Fligtar, nick: any objections?

All right. Where would this link point?

> Is it possible to style the "Select a collection..." line in the dropdown to
> make it more apparent that it's not an option you can choose?  Even if it only
> works in FF3.  If nothing else, maybe just a separator between it an the
> options?  I'm just throwing out ideas.

Hm. I noticed that if I style it with display:none, it does show up as on the page when you first open it, but it's not an option in the dropdown anymore. I was unsure if display:none was a good idea accessibility-wise, as screen readers may ignore it altogether. However, I wrapped the heading ("add to...") as a label for the dropdown now, which is probably sufficient for showing what the dropdown is about. Let me know though if you'd like to do something else, though.

> When I try to save without JS on I get "Element Not Found:
> /home/clouserw/public_html/remora/site/app/views/elements/notification.thtml"

That was not committed yet from bug 492689.

Comment 20

[Fred Wenzel [:wenzel]]

(In reply to comment #19)
> > I think there should be a "what's this?" link next to "add to a collection" -
> > particularly if they have no collections.  Fligtar, nick: any objections?
> 
> All right. Where would this link point?

Just to the main collections page, maybe? I assume there will be plenty of collection talk on there.

Comment 21

[Fred Wenzel [:wenzel]]

I committed this to r25749. Please feel free to file followup bugs or reopen this if you want the link changed or similar.

Note that this is not skinned yet: that's part of bug 490887.

Comment 22

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

removing "push-needed" from 105 AMO 5.0.6 bugs; filter on "I hate stephend!"