475584 - Link Sharing

Status: VERIFIED FIXED

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

Description

[Nick Nguyen [:osunick]]

Neil,

Can we get a design on http://docs.google.com/Doc?id=dds6vwb4_12cnx3dbf6 that we can hand off to engineering?  Ping me with questions.

Thanks!

-n.

Comment 1

[Neil Lee [:neilio]]

Attachment: First stab at a wireframe

Here's a first run at this. The idea is this would popup when the "Share this" button gets mouseover. The tiny boxes in the popup are the service favicons. When you click on the service link the service page loads with that add-on's information added, etc.

We could open the service page in a floating div or in a pop-up window but I tend to prefer just sending them to the page, back button ftw and all of that.

Comment 2

[Neil Lee [:neilio]]

Attachment: Another option

Based on input from osunick and others, here's another run at this with a completely different direction.

Comment 3

[Neil Lee [:neilio]]

Attachment: Rollover state of previous wireframe

Comment 4

[Nick Nguyen [:osunick]]

Love the last wireframe... looks great.

Comment 5

[Neil Lee [:neilio]]

A couple of notes on how this should work (probably obvious):

- the Share This flyout should appear on click - an animation (fade in or scroll out) would be great, but not essential
- the flyout should close when users click on the close box, select a service, or click anywhere outside of the flyout itself.
- selecting a service should open its page in the current window with the exception of Facebook (which does not send the user back to the referring page).

Comment 6

[Wil Clouser [:clouserw]]

Attachment: omg

Are you really sure we only want those 5?!

Comment 7

[Nick Nguyen [:osunick]]

Um, yeah.  We can add more as people request them, to avoid the situation you just outlined.

Comment 8

[Nick Nguyen [:osunick]]

Actually, we should add Reddit, so one more.  I updated the spec with a link to the button code.

Comment 9

[Wil Clouser [:clouserw]]

Thanks Nick.  Just to be clear, we're going with attachment 360578 [details] and it should show on add-on detail pages only?

Comment 10

[Nick Nguyen [:osunick]]

Correct!

Comment 11

[Les Orchard [:lorchard]]

Attachment: patch to implement link sharing

This patch implements link sharing.  The share count clicks are tracked in the remora DB, since the APIs to query counts for each of the link sharing services are inconsistent or non-existent except for one or two of them.

Comment 12

[Les Orchard [:lorchard]]

Attachment: images for the patch

Attaching a tarball of images that would otherwise be nasty in a patch

Comment 13

[Les Orchard [:lorchard]]

Attachment: added a min-height for the addon box

Just after uploading the patch, noticed that the addon box was too short when the description was small.  so, one more tweak

Comment 14

[Ryan Doherty (:rdoherty)]

Comment on attachment 365672 [details] [diff] [review]
added a min-height for the addon box

This looks pretty good. I'm only a little worried about 3 things:

The font size for 'Share This' is pretty tiny, not sure how well it will work for Arabic, Japanese, etc.

If the text for 'Share This' gets any longer, it will expand into the description area.

I'm worried about catching share counts because the Netscalers will cache everything that's a GET request. We could add max-age:0 to the headers to prevent this.

Comment 15

[Wil Clouser [:clouserw]]

Trying to do real time stat tracking has not gone well for us in the past which is why we do all our stats from the logs now.  Since this uses external services we may not have a choice but this is going to need load testing before it can go live.

At first glance stuff like "SELECT `service`, SUM(`count`)" worries me.

Also, the netscalers haven't respected all the headers we set.  I'm not sure how Zeus feels about it but that is something else that will need plenty of testing.

Comment 16

[Les Orchard [:lorchard]]

(In reply to comment #14)
> (From update of attachment 365672 [details] [diff] [review])
> The font size for 'Share This' is pretty tiny, not sure how well it will work
> for Arabic, Japanese, etc.
> 
> If the text for 'Share This' gets any longer, it will expand into the
> description area.

I made some tweaks to widen the button a bit, so there's some expansion room to fit as many as 4-5 average English words.  Hopefully that will work without too much compromise.

> I'm worried about catching share counts because the Netscalers will cache
> everything that's a GET request. We could add max-age:0 to the headers to
> prevent this.

To address this, I threw a &__={UNIQ} param on the end, which is updated on the client in javascript with the current time in milliseconds.  This should make each of the link sharing links look new to the cache when clicked.

(In reply to comment #15)
> Trying to do real time stat tracking has not gone well for us in the past which
> is why we do all our stats from the logs now.  Since this uses external
> services we may not have a choice but this is going to need load testing before
> it can go live.

This doesn't need to be real-time per se, and should be cached by the netscaler.  Since I'll need to research how we do load testing for AMO, this won't make 5.0.3 code freeze.

This could conceivably be done by logs, though, since the sharing URLs take the form of:

http://dev.addons.mozilla.org/en-US/firefox/addon/{ADDON ID}/share?service={SERVICE NAME}&__=12341234234

I don't know what the next steps for counting by logs would be, though.  We could hold off on this feature until that's worked out, if it's a big enough issue.

> At first glance stuff like "SELECT `service`, SUM(`count`)" worries me.

Counters by service are maintained by date, so worst case if an addon got shared every day for a year, there'd be 5 * 365 = 1825 rows to sum, which mysql should handle pretty well.  If it's an issue, this can be cached in memcache, and should already get cached by netscaler

> Also, the netscalers haven't respected all the headers we set.  I'm not sure
> how Zeus feels about it but that is something else that will need plenty of
> testing.

Not going to use headers, using instead a parameter based on client-side time to keep the link URLs changing

Comment 17

[Nick Nguyen [:osunick]]

How about we separate out the counters into phase 2 and target 5.0.4 (assuming Les has time with his other commitments)?

Comment 18

[Wil Clouser [:clouserw]]

> > I'm worried about catching share counts because the Netscalers will cache
> > everything that's a GET request. We could add max-age:0 to the headers to
> > prevent this.
> 
> To address this, I threw a &__={UNIQ} param on the end, which is updated on the
> client in javascript with the current time in milliseconds.  This should make
> each of the link sharing links look new to the cache when clicked.

Can that be some kind of hash instead?  A thousand hits in a second isn't inconceivable on an add-on's page (on a rating button, it's stretching it, but that's what we do!)

> (In reply to comment #15)
> > Trying to do real time stat tracking has not gone well for us in the past which
> > is why we do all our stats from the logs now.  Since this uses external
> > services we may not have a choice but this is going to need load testing before
> > it can go live.
> 
> This doesn't need to be real-time per se, and should be cached by the
> netscaler.  Since I'll need to research how we do load testing for AMO, this
> won't make 5.0.3 code freeze.
> 
> This could conceivably be done by logs, though, since the sharing URLs take the
> form of:
> 
> http://dev.addons.mozilla.org/en-US/firefox/addon/{ADDON
> ID}/share?service={SERVICE NAME}&__=12341234234
> 
> I don't know what the next steps for counting by logs would be, though.  We
> could hold off on this feature until that's worked out, if it's a big enough
> issue.

I assumed it would need to be real time because we were keeping a counter next to the button and people would expect it to go up.  If that's not a big deal I'm definitely for log counting.

> > At first glance stuff like "SELECT `service`, SUM(`count`)" worries me.
> 
> Counters by service are maintained by date, so worst case if an addon got
> shared every day for a year, there'd be 5 * 365 = 1825 rows to sum, which mysql
> should handle pretty well.  If it's an issue, this can be cached in memcache,
> and should already get cached by netscaler
We've already got several caching tables for our stats, let's use those again (they are also by date).  They are calculated offline by cron jobs.

Comment 19

[Les Orchard [:lorchard]]

Attachment: link share counts removed; CSS tweaked for wider button

Okay, made tweaks to widen the button, and excised the database link share counters.

Comment 20

[Les Orchard [:lorchard]]

Comment on attachment 366430 [details] [diff] [review]
link share counts removed; CSS tweaked for wider button

Adding some r?'s for good measure, but I think this should do it.

Comment 21

[Les Orchard [:lorchard]]

Attachment: Did an svn up, refreshed the patch, and added site/app/config/constants.php

Ugh, another patch.

Comment 22

[Les Orchard [:lorchard]]

Attachment: Maybe this time?

Comment 23

[Les Orchard [:lorchard]]

Attachment: or maybe this time, without the count placeholders in constants.php

Comment 24

[Ryan Doherty (:rdoherty)]

Comment on attachment 366448 [details] [diff] [review]
or maybe this time, without the count placeholders in constants.php

Looks really good. Yay for good JS!

Note: I do see %s placeholders, but I'm pretty sure it's an issue with my local install.

Comment 25

[Wil Clouser [:clouserw]]

Thanks guys.  Can you put the table you had back in to remora.sql when you commit?  Let's get it all ready for tracking stats.

Comment 26

[Les Orchard [:lorchard]]

checked in as r23044

Comment 27

[Les Orchard [:lorchard]]

also checked in as r23045, including the images and SQL this time

Comment 28

[Wil Clouser [:clouserw]]

Stats bug: bug 482389
Maintenance bug: bug 482396

Comment 29

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

Les: would you mind providing a sample URL where this can be verified?  Thanks!

Comment 30

[Nick Nguyen [:osunick]]

check it:

https://preview.addons.mozilla.org/en-US/firefox/addon/1843

there's a "share this" button under the screenshot on the left side of the screen.

Comment 31

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

(In reply to comment #30)
> check it:
> 
> https://preview.addons.mozilla.org/en-US/firefox/addon/1843
> 
> there's a "share this" button under the screenshot on the left side of the
> screen.

Yeah, sorry for being a dunderhead and not checking preview like a good QA engineer; anyway, verified fixed on the various services.

1) There is some tweaking to be made in 5.0.4, though, like removing the onMouseover event listener and replacing it with onClick (I think!)

2) Also note that Digg doesn't retain the passed-in URL unless you're signed in, but I don't think there's anything we can do about that?