505846 - Add favorites to the extension

Status: VERIFIED FIXED

(Mozilla Labs Graveyard :: Personas Plus, defect)

Description

[Suneel]

GOAL

493876 added "favorites" functionality to getpersonas.com (see screenshot of favorites).

The goal of this fix is to add functionality to the client, which enables a logged in user to choose from and rotate his/her favorites. Here is the user experience:

USER EXPERIENCE FLOW

1. User logs in to getpersonas.com. Note that a user is currently not offered the option to favorite a design on getpersonas.com unless he/she is logged in.

2. User navigates to a design detail page (eg, http://getpersonas.com/persona/33)

3. User clicks star next to "add to favorites" on top right of the detail page.

4. User clicks on extension (little fox on bottom left of browser) and navigates to "favorites". He/she then has the option to select a particular favorite or select "random selection", which will rotate the designs at the frequency  Please follow attached "Personas Favorites Spec" with two exceptions (see below).

5. Expected Result: a particular persona is selected or if random selection, a rotation begins.


Exceptions to Attached Spec

1. Remove the "Clear All" option. In the near future, we will add this functionality to the server side.

2. Remove the "Reorder/Remove" option. We may consider adding this functionality in the future if community feedback indicates it is valuable.

3. For a user that is not logged in, the "favorites" menu item should navigate to a "sign in" link that navigates to https://www.getpersonas.com/signin?return=/

4. Please spell "favorites" by EN-US standards (without a "u")

Comment 1

[Suneel]

Attachment: screenshot of favorites on server side

Comment 2

[Suneel]

Attachment: personas favorites spec

Comment 3

[Jose E. Bolanos]

Hi,

In order to show the list of favorite personas, there needs to be a way of obtaining them. Is there any API method to get them? Can you explain how this can be done please?

Thank you

Comment 4

[Myk Melez [:myk] [@mykmelez]]

(In reply to comment #3)
> In order to show the list of favorite personas, there needs to be a way of
> obtaining them. Is there any API method to get them? Can you explain how this
> can be done please?

Ryan and Toby will know more about this; cc:ing them.

Comment 5

[Toby Elliott [:telliott]]

http://www.getpersonas.com/gallery/All/Favorites?rss=1

Will that do? I can write a json version pretty easily.

Comment 6

[Jose E. Bolanos]

A json version would be better as a matter of fact. Thanks!

Comment 7

[Jose E. Bolanos]

@Toby - Will the favorites json be output in the same json where the rest of the info is returned? Or will it be a separate json (i.e. another URL)?

Thanks

Comment 8

[Toby Elliott [:telliott]]

same url, with json=1 at the end. 

It's done. If you can access http://sm-personas01.mozilla.org/gallery/All/Favorites?json=1 you'll see it there.

Comment 9

[Jose E. Bolanos]

I'm trying to open the URL, signing in to sm-personas01.mozilla.org and all, but I'm getting a blank page with no content. Any ideas?

Another question: the favorites are loaded at startup, but how often should they be refreshed? We're thinking of refreshing it whenever the user signs in by monitoring the PERSONA_USER cookie, but perhaps this is not enough to guarantee a good user experience. Is there a way to detect when a user adds or removes a favorite in the website?

Thanks

Comment 10

[Toby Elliott [:telliott]]

I suspect you have no personas favorited on sm-personas01.

Myk may have mre suggestions on refresh rate, and there may be some javascript we can issue to cause a refresh. Daily sounds reasonable.

Comment 11

[Myk Melez [:myk] [@mykmelez]]

In the future we'll want something speedier, perhaps something push-y, but I agree with Toby that daily (which is how often we update the categorized popular personas) is reasonable for now.

Comment 12

[Jose E. Bolanos]

@Toby - The same happens when we try to see the detail page of one persona, a blank screen appears. For example this page is blank:
http://sm-personas01.mozilla.org/persona/33

We'll set a daily update then. Thanks

Comment 13

[Jose E. Bolanos]

@Toby - We cleaned Firefox cache and now it appears. Not sure why though, but it works. Thanks

Comment 14

[Jose E. Bolanos]

@Toby - Sorry, we just discovered that the page still appears blank but only when we're signed in, thus we're unable to add favorites. Any idea why?

Comment 15

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

Live HTTP Headers tell me that the content-length of that page is 0:

HTTP/1.x 200 OK

Date: Tue, 28 Jul 2009 21:38:20 GMT

Server: Apache/2.2.3 (CentOS)

X-Powered-By: PHP/5.1.6

Content-Length: 0

Connection: close

Content-Type: text/html; charset=UTF-8


On addons.mozilla.org, that can mean OOM errors; not sure here?

Comment 16

[Toby Elliott [:telliott]]

Try it again now. Should be better.

Comment 17

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

(In reply to comment #16)
> Try it again now. Should be better.

WFM now while logged in; Jose?

Comment 18

[Jose E. Bolanos]

The page appears correctly now, but when we try to add a favorite the link hangs with the label "loading...".

Comment 19

[Toby Elliott [:telliott]]

Whoops, needed to restart apache. Fixed

Comment 20

[Jose E. Bolanos]

Attachment: First patch

This patch fixes the bug entirely, but uses the favorites json URL which is not live yet, i.e. http://www.getpersonas.com/gallery/All/Favorites?json=1.

Some other details:
- The time interval between rotations is determined by a preference named "extensions.personas.rotationTimeInterval", and it is initially set to one hour (in seconds). It can be modified to test.
- Two additional strings were added to the main bundle, but only under en-US.

Please review and let us know if modifications are needed.
Thank you.

Comment 21

[Jose E. Bolanos]

Note: the rotation interval preference is named "extensions.personas.rotateFavoritesInterval". Thanks

Comment 22

[Myk Melez [:myk] [@mykmelez]]

Comment on attachment 391274 [details] [diff] [review]
First patch

Generally the code looks good, although I haven't tested it yet.  I'm a bit concerned about the way we're relying on the presence of a cookie to determine the user's auth status, since it presumably isn't definitive, and the cookie could remain around after the server has forgotten about the user's session.

Also, ideally we should be soliciting the user's credentials, storing them in the password manager, and authenticating them each time we retrieve favorites (rather than not retrieving favorites if the user hasn't logged into the website lately).  That'd probably require changes on the server side to handle HTTP basic auth for these requests in addition to the client-side work to solicit and store user credentials.  Maybe what we have is ok for now.

Finally, as Suneel mentioned in comment 0 (although perhaps it wasn't entirely clear), instead of providing a "Rotation" feature that rotates through the list of favorites, this patch should be providing a "Random" feature that periodically picks a random persona from the list of favorites the same way we provide such a feature for the categories.

Comment 23

[Jose E. Bolanos]

So, to recap:

When the feature is active, instead of switching the current persona to the next favorite persona in order, the next favorite persona should be chosen randomly? If this is correct, should the feature's menu item remain the same? (The current label is "Wear all in rotation", as determined by the specification PDF)

Let us know to resubmit a patch.

Comment 24

[Myk Melez [:myk] [@mykmelez]]

(In reply to comment #23)
> So, to recap:
> 
> When the feature is active, instead of switching the current persona to the
> next favorite persona in order, the next favorite persona should be chosen
> randomly?

Yes, that's right.


> If this is correct, should the feature's menu item remain the same?
> (The current label is "Wear all in rotation", as determined by the
> specification PDF)

No, the menu item should be "Random Selection from Favorites", and it should appear at the bottom of the menu, separated from the list of favorites by a menu separator, just like in the category submenus.

You should be able to reuse the useRandomPersona property string for this, so you only need to add one string.

Comment 25

[Jose E. Bolanos]

Attachment: Second patch attempt

This patch is slightly different from the first proposed patch:
- Favorites are changed randomly when the timer ticks. The _getRandomPersona method was split in order to be reused. Now two methods exist: _getRandomPersonaFromArray and _getRandomPersonaFromCategory (which uses the former). This way, the _getRandomPersonaFromArray is used passing the favorites array.
- The rotation menu item is now located at the bottom of the popup and labeled "Random Selection from Favorites".

Comment 26

[Myk Melez [:myk] [@mykmelez]]

Comment on attachment 391477 [details] [diff] [review]
Second patch attempt

Regarding the architecture, rather than having a separate "ignoreRotation" flag to specify that we're in "random from favorites" mode, the flag should be built into the existing mechanism for specifying the mode, i.e. the "selected" preference, whose possible values are currently "default" (the default Firefox theme), "random" (a random persona from the selected category), and "current" (a specific persona).

Since "Favorites" isn't a category, and it's not clear how to shoehorn it into the existing code paths for "random from category" mode, we should probably add a fourth value for the "selected" preference, "randomFavorite", that indicates the user is in "random from favorites" mode.


Regarding the user experience, it's confusing that the Favorites menu item takes you to the website when you select it.  It's not clear that you're being asked to log in because you need to be authenticated to access your favorites in the extension.  Nor is it clear that a menu of your favorites will appear in the extension once you authenticate.

Ultimately we should integrate authentication into the extension such that you can give the extension your credentials and have it automatically authenticate you when retrieving your favorites.  It's probably better to do that in a separate round of development, however; among other reasons, it'll need some server-side changes like support for HTTP basic auth.

So for this round, let's optimize the current user experience with the following changes:

1. When you aren't logged in, the Favorites item is a menu with one item that says "Sign In to Access Your Favorites", so it's clear what you are going to do when you select that item.

2. When you select that item, the extension loads the /signin?return=/gallery/All/Favorites page, which returns you to /gallery/All/Favorites after you sign in.  That way you see your favorites the moment you sign in, so you can access them even if you don't realize you can go back to the menu afterwards to access them.


A few more issues:

>diff -rupN old/chrome/personas/content/personas.js new/chrome/personas/content/personas.js
>--- old/chrome/personas/content/personas.js	2009-06-15 00:04:16.000000000 -0600
>+++ new/chrome/personas/content/personas.js	2009-07-29 17:16:10.000000000 -0600

These paths don't apply cleanly to the source repository.  There's an extra "personas" directory between "chrome" and "content" that doesn't exist in the repository.


Reading through the code, favorites are supposed get retrieved when you start Firefox and also the moment you sign in, but that didn't seem to happen for me.

I set extensions.personas.siteURL to http://sm-personas01.mozilla.org/, set extensions.personas.url to http://sm-personas01.mozilla.org/static/, and added sm-personas01.mozilla.org to extensions.personas.authorizedHosts.  I've also selected five favorite personas on the testing site, which show up at http://sm-personas01.mozilla.org/gallery/All/Favorites (and also in its JSON equivalent, http://sm-personas01.mozilla.org/gallery/All/Favorites?json=1).

But when I open the Favorites menu in the extension, it is empty except for the separator and "Random Selection from Favorites" item.  That's true after I start Firefox and also after I sign out and back in.

I just took a closer look, and it looks like this is because refreshFavorites has hard-coded getpersonas.com into the URL it loads.  It should construct the URL using extensions.personas.siteURL as the base URL.


When you select the Favorites menuitem, the Sign In page loads in the current tab.  Like the View Gallery menuitem, it should load in a new tab.


onCookieChanged doesn't check to see if the host that set the cookie is one of the authorized hosts.  It should do so.  Otherwise, a malicious site could DOS the browser by repeatedly changing that cookie, causing Personas to repeatedly refresh favorites.

Comment 27

[Jose E. Bolanos]

Sorry about the diff having an extra folder, our bad.

Some of the requirements were not 100% clear, but this last post is very clear and thorough. We'll implement these changes and resubmit a patch ASAP.

Thank you

Comment 28

[Jose E. Bolanos]

There is one last thing that is not clear: when the user chooses the "random from favorites" mode, a new random persona from favorites is chosen every hour. Should the same behavior occur when the "random from category" mode is chosen? Meaning, should a new random persona from the currently selected category be chosen every hour? Or does that "rotation" feature only apply to favorites?

Comment 29

[Myk Melez [:myk] [@mykmelez]]

(In reply to comment #28)
> There is one last thing that is not clear: when the user chooses the "random
> from favorites" mode, a new random persona from favorites is chosen every hour.
> Should the same behavior occur when the "random from category" mode is chosen?
> Meaning, should a new random persona from the currently selected category be
> chosen every hour? Or does that "rotation" feature only apply to favorites?

I really like the feature you added that picks a new favorite every hour.  I think we should adopt it for "random from category" as well.  It's much better than "random upon startup", which is what we currently do.

I'm not sure that hourly is the optimal interval for regular users, but I think it's a good place to start as we experiment with the feature and solicit feedback from testers.  So let's start with that, and I'll make sure to mention the hidden pref when talking about the feature in the discussion forum, so folks can experiment with different intervals.

Comment 30

[Jose E. Bolanos]

Attachment: Third patch attempt

This patch includes all the corrections requested by Myk on his last comment. In addition to favorite rotation, it also includes rotation of personas by category.

Comment 31

[Myk Melez [:myk] [@mykmelez]]

Comment on attachment 391912 [details] [diff] [review]
Third patch attempt

Overall this code looks great (haven't tested it yet), I just noticed a couple issues as I dug deeper into the cookie stuff...

>+  changeToRandomFavoritePersona : function() {
>+    if (this.personas &&
>+        this.personas.favorites &&
>+        this.personas.favorites.length > 0 &&
>+        this.isUserSignedIn) {

I'm not sure you need to check isUserSignedIn here.  If a user has a set of favorites from an earlier session, it seems ok for the random persona selector to change to a new one even if the user is no longer signed in to the server, no?


>+  /**
>+   * Whether or not a user is signed in, determined by the presence of a cookie
>+   * named "PERSONA_USER" by any of the authorized hosts.
>+   * @return True if signed in, false otherwise.
>+   */
>+  get isUserSignedIn() {
>+    let authorizedHosts = this._prefs.get("authorizedHosts").split(/[, ]+/);
>+    let cookieManager =
>+      Cc["@mozilla.org/cookiemanager;1"].getService(Ci.nsICookieManager);
>+    let cookieEnu = cookieManager.enumerator;
>+
>+    while (cookieEnu.hasMoreElements()) {
>+      let cookie = cookieEnu.getNext().QueryInterface(Ci.nsICookie);
>+
>+      if (cookie.name == COOKIE_USER &&
>+          authorizedHosts.some(function(v) v == cookie.host)) {
>+        return true;
>+      }
>+    }
>+    return false;
>+  },

I'm concerned about the algorithm used here.  Traversing XPCOM boundaries can be expensive, and it'll happen a lot when the code enumerates all cookies for users with many cookies.

A better approach would be to construct a cookie for each authorized host and use nsICookieManager2::cookieExists to determine whether or not the cookie is present, since it calls nsCookieService::FindCookie to find the cookie, and that iterates only across cookies for the specific host.

Something like this code sample should work (although it's possible that some additional properties of the cookie object will need to be defined):

 let authorizedHosts = this._prefs.get("authorizedHosts").split(/[, ]+/);
 let cookieManager = Cc["@mozilla.org/cookiemanager;1"].getService(Ci.nsICookieManager2);
 for each (let host in authorizedHosts) {
   let cookie = {
     QueryInterface: XPCOMUtils.generateQI([Ci.nsICookie2, Ci.nsICookie]),
     host: host,
     path: "/",
     name: COOKIE_USER
   };
   if (cookieManager.cookieExists(cookie))
     return true;
 }
 return false;


Lastly, this checks all authorized hosts, but favorites are only ever loaded from the "main" host identified by the "url" and "siteURL" preferences (www.getpersonas.com).  That's problematic, because it means a secondary host could make the extension mistakenly think the user is signed in to the primary host from which it will try to load favorites.

Unfortunately, it turns out that's even true for the default value of the "authorizedHost" preference, since getpersonas.com and www.getpersonas.com set "host cookies," which only apply to those specific hostnames, instead of a "domain cookie" for ".getpersonas.com", which would apply to both hostnames.  So you can be signed in to one while not being signed in to the other.

I filed bug 507756 on that problem.  I listed two possible fixes in that bug.  Depending on the fix, this code will be slightly different.  In both cases, this code should create a new preference "extensions.personas.host" that contains "www.getpersonas.com" by default (since that's what "url" and "siteURL" use) and only use that one host to check for the cookie (instead of looping through all authorized hosts).

If the fix for bug 507756 is for getpersonas.com to redirect to www.getpersonas.com, and that site continues to set a host cookie for www.getpersonas.com, then this code can use nsICookieManager2::cookieExists, as in the sample code above.

If the fix for bug 507756 is for the site to set a domain cookie for .getpersonas.com, however, then this code will have to use nsICookieManager2::getCookiesFromHost to find the cookie, since the code implementing nsICookieManager2::cookieExists appears to require an exact host and not work with domain cookies.

Comment 32

[Jose E. Bolanos]

"I'm not sure you need to check isUserSignedIn here.  If a user has a set of
favorites from an earlier session, it seems ok for the random persona selector
to change to a new one even if the user is no longer signed in to the server,
no?"

We thought the user would not have access to her favorites while signed out from the site, even if the favorites had been loaded already. We'll remove that validation.

Regarding the signed-in cookie issue, should we wait until bug 507756 is resolved before continuing? Or should we assume we'll be able to use nsICookieManager2::cookieExists and the code sample you provided?

Thank you

Comment 33

[Myk Melez [:myk] [@mykmelez]]

(In reply to comment #32)
> We thought the user would not have access to her favorites while signed out
> from the site, even if the favorites had been loaded already.

Hmm, that's a good point.  If a user signs out, perhaps we don't want to make site information available for privacy reasons.  I'm not sure, but go ahead and leave it as is for now.


> Regarding the signed-in cookie issue, should we wait until bug 507756 is
> resolved before continuing? Or should we assume we'll be able to use
> nsICookieManager2::cookieExists and the code sample you provided?

Let's assume the fix for bug 507756 is going to be an unconditional redirect to www.getpersonas.com and implement this patch with the extensions.personas.host preference and using nsICookieManager2::cookieExists to check for the cookie.

Comment 34

[Jose E. Bolanos]

Attachment: Fourth patch

This patch contains all previous modifications plus the changes requested in comment #33.

Comment 35

[Myk Melez [:myk] [@mykmelez]]

Comment on attachment 392344 [details] [diff] [review]
Fourth patch

This looks great, r=myk.

Comment 36

[Myk Melez [:myk] [@mykmelez]]

Fixed in changeset http://hg.mozilla.org/labs/personas/rev/1c3684e2ad13, and I spun a development build with the fix:

https://people.mozilla.com/~cbeard/personas/dist/personas-dev.xpi

Thanks for the fix!

Note: localizations other than en-US don't yet work.  I have submitted the development build to Babelzilla for localization.

Also, the feature doesn't yet work against the production instance of getpersonas.com, since the website doesn't have the JSON version of the favorites page (nor does it unconditionally redirect getpersonas.com to www.getpersonas.com).

You can, however, test it against the testing instance of getpersonas.com, which lives at http://sm-personas01.mozilla.org/ (although at the moment the testing site shows you popular personas rather than your favorites because of a bug in its configuration).

Comment 37

[Suneel]

Myk spun a development build to get feedback from the community. One bug was reported by the community and filed as 510123. Reopening this bug and setting a dependency.

Comment 38

[Myk Melez [:myk] [@mykmelez]]

Resolving this bug fixed, since this feature has indeed been implemented (even though there may be bugs in it or additional feature requests related to the functionality it provides), and that resolution is the more accurate reflection of the current state of affairs.

Comment 39

[Carsten Book [:Tomcat]]

verified fixed (tested on 1.3)