"More from (null)" in current selected persona submenu for custom personas

VERIFIED FIXED in 1.4

Status

VERIFIED FIXED
9 years ago
2 years ago

People

(Reporter: juan, Assigned: erik)

Tracking

Details

Attachments

(2 attachments, 3 obsolete attachments)

(Reporter)

Description

9 years ago
User-Agent:       Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.1.4) Gecko/20091016 Firefox/3.5.4
Build Identifier: trunk

When you build a custom persona and apply it, the current persona (statusbar) menu element changes to the custom persona, the first option in its submenu is disabled (which is right) and the second reads "More From (null)".

Reproducible: Always

Steps to Reproduce:
1. compose a custom persona
2. apply it
3. go to current persona sub menu
Actual Results:  
second option is enabled and reads "More from (null)"

Expected Results:  
I don't know which the expected behavior is, but certainly "More from (null)" doesn't seem to be correct.

Disable the option perhaps? Hide it?
(Reporter)

Comment 1

9 years ago
Created attachment 410533 [details]
"More from (null)" submenu

a screenshot of this bug
(Reporter)

Updated

9 years ago
Version: unspecified → Trunk
note, i saw this problem also with a non-custom persona like groovy blue and got the same error. 

When i clicked in the link i got also forwarded to http://www.getpersonas.com/en-US/gallery/Designer/undefined
Status: UNCONFIRMED → NEW
Ever confirmed: true
Target Milestone: -- → 1.4

Updated

9 years ago
Assignee: cbeard → erik

Updated

9 years ago
Blocks: 527270

Comment 3

9 years ago
Erik, I saw an update on this in basecamp. Could you also be sure to update the bug with an ETD?
(Reporter)

Comment 4

9 years ago
@Suneel

In order to have an estimate on this bug we would require to know what's the expected behavior for it.

Prevent notification display (the black notification shown on top of the current page) and disable the current persona submenu perhaps?

Please let us know your thoughts

Comment 5

9 years ago
The expected behavior is "More from [Designer Display Name]". If the designer does not have a display name, then it should be "More from [Designer Login Name]"
(Reporter)

Comment 6

9 years ago
Created attachment 411223 [details] [diff] [review]
proposed patch for "More from (null)" issue

This patch applies only for custom personas. 

I've never been able to reproduce with any other persona as Carsten mentions in comment #2

Proposed solution in this patch:

- current persona submenu only shows the first (View Details) option grayed out. The "More from XXX" menu item is collapsed for custom personas so it doesn't show up with wrong/invalid/bad information.

- black notification shown on top of the page is not displayed when the custom persona is applied and it is not in the list of recent personas.

Added Jose to review the implemented patch, then forward it to Myk.
Attachment #411223 - Flags: review?(jose)
(Reporter)

Comment 7

9 years ago
Created attachment 411251 [details] [diff] [review]
proposed patch for "More from (null)" issue V2

Found an error in the patch file posted previously. 

This is the correct one

Sorry about that :P
Attachment #411223 - Attachment is obsolete: true
Attachment #411251 - Flags: review?(jose)
Attachment #411223 - Flags: review?(jose)

Comment 8

9 years ago
Comment on attachment 411251 [details] [diff] [review]
proposed patch for "More from (null)" issue V2

Looks good. These were the changes discussed on today's call with Suneel.
Attachment #411251 - Flags: review?(myk)
Attachment #411251 - Flags: review?(jose)
Attachment #411251 - Flags: review+
Comment on attachment 411251 [details] [diff] [review]
proposed patch for "More from (null)" issue V2

Hiding the "More from [author]..." item is a good idea (among other things, it doesn't require localization changes, which would be hard to do at this stage of the release cycle), and overall this looks good, with just a couple of issues.

In particular, although the current patch resolves the case of a custom persona, it doesn't resolve the other case that Carsten, I, and other folks have noticed where a regular persona loaded from the gallery will exhibit this behavior.  I looked into it a bit, and I think the case occurs when a user has selected an old persona record that doesn't include the "author" property.

This can happen with the current persona if the user hasn't used Personas since before the author property was added to the records, and it can happen with personas in the Recent Personas menu that the user hasn't used since before that change, since those records don't get updated on a daily basis the way the current persona does.

This fix should handle that case as well by also hiding the "More from [author]..." item when a persona doesn't provide an "author" property.


>     // Show the notification if the selected persona is not in the favorite or
>-    // recent lists.
>-    if (!inRecent && !inFavorites)
>+    // recent lists and is not a custom persona
>+    if (!inRecent && !inFavorites && !persona.custom) {
>       this._showPersonaChangeNotification();
>+    }

Nit: there's no need to bracket one-line blocks.  But keep the period at the end of the sentence in the comment!


>    * Reverts the current persona to the previously selected persona, if
>-   * available                                    
>+   * available

Note: this patch didn't apply cleanly when I imported it into my working copy, and it looks like this whitespace change was the culprit, as it's already been committed in changeset <http://hg.mozilla.org/labs/personas/rev/68dd3b327e9f>.
Attachment #411251 - Flags: review?(myk) → review-
(Reporter)

Comment 10

9 years ago
Created attachment 411363 [details] [diff] [review]
proposed patch for "More from (null)" issue V3

This patch includes the necessary adjustments to address the comments made by Myk. 

Now we display the current persona's "More From [author]" submenu only if the author property is present.

Likewise, we only display the notification to undo the persona switch if the author property is present or the display_username property is present, since either one of them is enough to prevent the "PERSONA_NAME by null" message generation in this case.
Attachment #411251 - Attachment is obsolete: true
Attachment #411363 - Flags: review?
(Reporter)

Updated

9 years ago
Attachment #411363 - Flags: review? → review?(myk)
Comment on attachment 411363 [details] [diff] [review]
proposed patch for "More from (null)" issue V3

Almost there!

>+    // collapse the "More From User" menu item for custom personas or personas
>+    // with null author. In this case we only check the author is not null
>+    // because it is used to generate the url to go to the persona designer page
>+    // (bug 526788).
>+    if (PersonaService.currentPersona.custom || !PersonaService.currentPersona.author) {
>+      personaStatusDesigner.setAttribute("collapsed", true);
>+    } else {
>+      personaStatusDesigner.removeAttribute("collapsed");
>+      let designerLabel = PersonaService.currentPersona.display_username ?
>+                            PersonaService.currentPersona.display_username : PersonaService.currentPersona.author;
>+      personaStatusDesigner.setAttribute("label", this._strings.get("viewDesigner", [designerLabel]));
>+      let designerURL = this._siteURL + "gallery/Designer/" + PersonaService.currentPersona.author;
>+      personaStatusDesigner.setAttribute("oncommand",
>+                            "PersonaController.openURLInTab('" + designerURL + "')");
>+    }

I talked with Toby about "author" vs. "display_username", and it turns out that "display_username" is a column in the website's database schema but is not a property in the JSON feeds.  Bug 527710 adds a "username" property to the JSON feeds that contains the username of the author, however, which is the string that should be part of the URL, while the "author" property contains the human-readable name of the author, which should be the string we display to the user.

So this code should only show the menu item if the "username" string is present, since otherwise we can't generate the proper URL (using the "author" property will currently work in 90% of cases, but that percentage will presumably go down over time as designers are encouraged to provide an "author" name in addition to their username).

And when it shows the menu item, this code should label it with the "author" if present (in the long run it'll always be present, but for now it's possible that it'll be empty), otherwise the "username".
Attachment #411363 - Flags: review?(myk) → review-
(Reporter)

Comment 12

9 years ago
Created attachment 411572 [details] [diff] [review]
proposed patch for "More from (null)" issue V4

This one has the corrections noted by Myk (comment #11).

I tested it in my local enviroment and noticed that using the username property to determine whether or not to display the "More From [author]" submenu prevents it from being displayed every time.

This is because the persona's JSON does not contain any username property (yet?).

I guess the server will return this information soon. Here's the patch anyway.
Attachment #411363 - Attachment is obsolete: true
Attachment #411572 - Flags: review?(myk)
Attachment #411572 - Flags: review?(myk) → review+
Comment on attachment 411572 [details] [diff] [review]
proposed patch for "More from (null)" issue V4

This looks great, thanks!

Indeed, the servers don't include the username property in their JSON feeds yet.  I've asked Toby to fix this on the staging server (http://sm-personas01.mozilla.org/), and it should get fixed on the production server once the fix for bug 527710 gets pushed to production (bug 527792).
Committed as changeset http://hg.mozilla.org/labs/personas/rev/94c9d3f51b4e.  Thanks for the fix!
Status: NEW → RESOLVED
Last Resolved: 9 years ago
Resolution: --- → FIXED
verified on stage ! will wait for the final verification till this is on production !
verified fixed also on production with 1.4rc3
Status: RESOLVED → VERIFIED
Product: Mozilla Labs → Mozilla Labs Graveyard
You need to log in before you can comment on or make changes to this bug.