515084 - update client list of favorites when user "favorites" on server

Status: VERIFIED FIXED

(Mozilla Labs Graveyard :: Personas Plus, defect)

Description

[Suneel]

Problem:  Logged-in Personas users are able to indicate "favorites" on the server side, and  505846 was fixed to display a user's favorites on the client side. Members of the community reported an issue with the fix, which was filed as 510123 . We have since concluded that 510123 is not a result of an issue with the original fix, but rather an expected result given the cookies we currently have in place.

The purpose of this bug is to add events that notify the extension when a user "favorites" a persona design on the server side.

Steps to recreate:

1. Install the development release of Personas. See first entry of this thread for directions: http://bit.ly/3nW1eX

2. Log in to the Personas gallery

3. Favorite a persona (by clicking on the "Add to favorites" star) (eg, Groovy Blue: http://sm-personas01.mozilla.org/en-US/persona/33)

4. Go to Tools > Personas for Firefox > Favorites

Expected Result: Groovy Blue is listed as a favorite

Actual Result: Groovy Blue is not listed as a favorite until you shut down and restart Firefox.

We propose that the gallery fire events when a user adds and removes favorites (just as it does when a user previews or selects a persona):

AddFavoritePersona
RemoveFavoritePersona

The extension would then listen for these events to know when a user has added a persona to their favorites or removed a persona from them, and it would update its list of favorite personas accordingly.

Assigning to Ryan for web dev work needed to trigger event similar to the event triggered when user previews a design.

Including Jose to implement fix on the client side.

Comment 1

[Jose E. Bolanos]

Hi,

Question about the DOM events. Will they carry information about the persona which has been added/removed to the favorites? Or should the client just refresh the whole list when any of these events is received?

Thanks

Comment 2

[Ryan Doherty (:rdoherty)]

(In reply to comment #1)
> Hi,
> 
> Question about the DOM events. Will they carry information about the persona
> which has been added/removed to the favorites? Or should the client just
> refresh the whole list when any of these events is received?
> 
> Thanks

I can put the normal json object on the dom object that triggered the event (same as preview/reset/select events)

Comment 3

[Jose E. Bolanos]

It would be simpler just to detect either event and refresh the whole list. The other way would be to add or remove the dom object persona from the local favorite list, doing the necessary validations.

Let me know the way to go. Thanks.

Comment 4

[Ryan Doherty (:rdoherty)]

(In reply to comment #3)
> It would be simpler just to detect either event and refresh the whole list. The
> other way would be to add or remove the dom object persona from the local
> favorite list, doing the necessary validations.
> 
> Let me know the way to go. Thanks.

Sure, just detecting the event works for me. Can the event be 'UpdateFavorites'?

Comment 5

[Jose E. Bolanos]

That would be fine, since both events will be treated the same way. I'll use just this event then.

Comment 6

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

It may be more complicated to implement, but the code should still add/remove personas individually rather than reloading the list each time it changes because updating the list with information already available in the page is less resource intensive and more performant than reloading the list each time.

Comment 7

[Jose E. Bolanos]

We must keep the original events then. I will implement it this way.

Comment 8

[Jose E. Bolanos]

Attachment: First patch

This patch adds support for the "AddFavoritePersona" and "RemoveFavoritePersona" DOM events, adding and removing the persona in the local favorites list.

Comment 9

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

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

>+      case "AddFavoritePersona":
>+        this.onAddFavoritePersona(aEvent);
>+        break;
>+      case "RemoveFavoritePersona":
>+        this.onRemoveFavoritePersona(aEvent);
>+        break;

As with the handling of the other events that can bubble up from content, this needs to call onAddFavoritePersonaFromContent and onRemoveFavoritePersonaFromContent methods that check whether the website is authorized to trigger personas changes before we handle the event.  See the onSelectPersonaFromContent and onSelectPersona methods for how this should work.


>+  addFavoritePersona : function(aPersona) {
>+    // Make sure the favorites list exists.
>+    if (this.personas == null || this.personas.favorites == null) {
>+      this.personas.favorites = [];
>+    }

If this.personas is null, then you won't be able to set this.personas.favorites, so you would need to do something like:

if (this.personas == null)
  this.personas = {};
if (this.personas.favorites == null)
  this.personas.favorites = [];

Also, nit: simply check for falseness rather than equivalence with null, i.e.:

if (!this.personas)
  this.personas = {};
if (!this.personas.favorites)
  this.personas.favorites = [];

However, this reveals a weakness with the way we refresh favorites: it depends on the presence of this.personas, which gets created when we refresh general data about personas, even though there's no code to ensure that this.personas exists when we refresh favorites, and this.personas.favorites will get obliterated every time this.personas is refreshed.

I wonder if this weakness is the cause of bug 510123.  It would certainly explain the difficulty of getting a reproducible testcase for that bug, since refresh order is non-deterministic.

One solution would be to add code to do the dependency checking and preserve this.personas.favorites each time we refresh this.personas, but that would be time-consuming, complex, and brittle.  A better solution is to store favorites in a separate top-level property of the service, i.e. this.favorites.

So let's do that: rather than storing favorites in this.personas.favorites, modify the existing code to store it in a separate this.favorites.  Then here you only need to check if that property is defined:

if (!this.favorites)
  this.favorites = [];


>+    let i = this._findPersonaInArray(aPersona, this.personas.favorites);
>+    if (i >= 0) {
>+      this.personas.favorites[i] = aPersona;
>+    } else {
>+      this.personas.favorites[this.personas.favorites.length] = aPersona;
>+    }

Minor nits: here and elsewhere, there's no need to place brackets around one-line blocks (unless the conditional expression spans multiple lines); also, you can add an item to an array via Array.push, i.e.:

  this.personas.favorites.push(aPersona);

Comment 10

[Jose E. Bolanos]

Attachment: Second patch attempt

Second patch attempt. Corrected the issues from the first patch. Now also using the updated code from the patch of bug 510123.

Comment 11

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

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

This looks great, r=myk.  I haven't tested it, however, since the website doesn't yet generate these events.

Comment 12

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

Committed as changeset http://hg.mozilla.org/labs/personas/rev/287790765b0b, and I pushed a new dev build with these changes:

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

Note, however, that the dev build won't update the list of favorites as it changes until the server starts dispatching the necessary events.

Since the client and server changes are separate chunks of work, I've filed bug 516588 on the server side work and am resolving this bug fixed.

Thanks for the fix!

Comment 13

[Ryan Doherty (:rdoherty)]

Events have been added to the 'Add to favorites'/'Remove from favorites' links. Should show up on staging in ~10 mins.

Comment 14

[Carsten Book [:Tomcat]]

verified ! Thanks guys !