Closed Bug 936712 Opened 7 years ago Closed 6 years ago

mark button usage is in-determinant if providers user is not logged in.

Categories

(Firefox Graveyard :: SocialAPI, defect)

26 Branch
x86
macOS
defect
Not set

Tracking

(firefox26 unaffected, firefox27 verified, firefox28 verified)

VERIFIED FIXED
Firefox 28
Tracking Status
firefox26 --- unaffected
firefox27 --- verified
firefox28 --- verified

People

(Reporter: mixedpuppy, Assigned: mixedpuppy)

Details

Attachments

(2 files, 2 obsolete files)

The social mark button is designed to not show ui on the first click.  The problem is, if the user is not logged into that provider, the provider is unable to respond to the mark request.  They are unable to prompt the user to login.

The solution discussed with Boriss is that, if the user is not logged in, we will show the mark panel on first click.  This gives the mark provider a chance to ask the user to login.

In the short term, we will use the existing provider.haveLoggedInUser api.  If that returns false, we show the panel.

In the long term we would like to get rid of that api since it otherwise has no value.  We will have to think about how we deal with this situation then.
Assignee: nobody → mixedpuppy
Attachment #829625 - Flags: ui-review?(jboriss)
Attachment #829625 - Flags: review?(mhammond)
Comment on attachment 829625 [details] [diff] [review]
open on first click if not logged in

Review of attachment 829625 [details] [diff] [review]:
-----------------------------------------------------------------

I'm afraid I really don't understand the test, so I'd like to see that reworked a little.  The test seems like a copy of an existing test with the only difference being the user is not logged in at the start, but I see no tests specific to that fact.  Eg, if the user isn't logged in so the panel is shown, but then they decline to log in, we should test the page isn't marked.  It's alot of text complexity for a single line code change, so ideally we would *only* be testing things specific to the case of the user not being logged in.

::: browser/base/content/socialmarks.xml
@@ +150,5 @@
>          if (!src || src == "about:blank") {
>            this.loadPanel();
>            this.isMarked = true;
>          }
> +        if (openPanel || !this.provider.haveLoggedInUser())

can't this extra condition go into openPanel?  Also, should isMarked really be true in this case?  I'd expect the panel would show UI to log the user in, and only after that succeeds would the page be marked.

::: browser/base/content/test/social/browser_social_marks.js
@@ +256,5 @@
> +            port.postMessage({topic: "test-logout"});
> +            waitForCondition(function() !provider.profile.userName,
> +                function() {
> +                  ok(!provider.profile.userName, "profile was unset by test worker");
> +                  // first click marks the page, second click opens the page. We have to

this comment looks wrong - first click opens the page in this test IIUC

@@ +266,5 @@
> +            break;
> +          case "got-social-panel-visibility":
> +            ok(true, "got the panel message " + e.data.result);
> +            if (e.data.result == "shown") {
> +              // unmark the page via the button in the page

I expected this to be the first open of the panel after the user clicks the marks button when not logged in, so I wouldn't expect it to be marked here?

@@ +275,5 @@
> +            } else {
> +              // page should no longer be marked
> +              port.close();
> +              waitForCondition(function() !btn.isMarked, function() {
> +                // after closing the addons tab, verify provider is still installed

isn't the provider's activation tab being closed here?
Attachment #829625 - Flags: review?(mhammond) → feedback+
(In reply to Mark Hammond [:markh] from comment #2)
> Comment on attachment 829625 [details] [diff] [review]

> I'm afraid I really don't understand the test, so I'd like to see that
> reworked a little.  The test seems like a copy of an existing test with the
> only difference being the user is not logged in at the start, but I see no
> tests specific to that fact.  Eg, if the user isn't logged in so the panel
> is shown, but then they decline to log in, we should test the page isn't
> marked.  

There is a way for the page to tell us the marked state, I think, given this issue, that we should not be marking the page ourselves, but letting the page tell us it is marked, I'm making that change in the next version.  The two tests are indeed very similar, the only real difference is whether we open the panel on first click or second click.

> It's alot of text complexity for a single line code change, so
> ideally we would *only* be testing things specific to the case of the user
> not being logged in.
> 
> ::: browser/base/content/socialmarks.xml
> @@ +150,5 @@
> >          if (!src || src == "about:blank") {
> >            this.loadPanel();
> >            this.isMarked = true;
> >          }
> > +        if (openPanel || !this.provider.haveLoggedInUser())
> 
> can't this extra condition go into openPanel?  

Not clear on that question, there isn't an openPanel method.

> Also, should isMarked really
> be true in this case?  I'd expect the panel would show UI to log the user
> in, and only after that succeeds would the page be marked.

Yes, we shouldn't be setting isMarked unless the page signals us to do so.
This patch removes our setting the page as marked and requires the social mark provider to signal us that it has been marked.
Attachment #829625 - Attachment is obsolete: true
Attachment #829625 - Flags: ui-review?(jboriss)
Attachment #831086 - Flags: ui-review?(jboriss)
Attachment #831086 - Flags: review?(mhammond)
(In reply to Shane Caraveo (:mixedpuppy) from comment #3)
> (In reply to Mark Hammond [:markh] from comment #2)

> > > +        if (openPanel || !this.provider.haveLoggedInUser())
> > 
> > can't this extra condition go into openPanel?  
> 
> Not clear on that question, there isn't an openPanel method.

heh, I only just now grokked what you're saying.
Comment on attachment 831086 [details] [diff] [review]
open on first click if not logged in

Review of attachment 831086 [details] [diff] [review]:
-----------------------------------------------------------------

I guess this is fine, so long as you are sure that everyone who supports (or is about to support) marks does explicitly tell us about it

::: browser/base/content/socialmarks.xml
@@ +152,2 @@
>          }
> +        if (openPanel || !this.provider.haveLoggedInUser())

what I meant was: let openPanel = this.isMarked || aOpenPanel || !this.provider.haveLoggedInUser();
Attachment #831086 - Flags: review?(mhammond) → review+
(In reply to Mark Hammond [:markh] from comment #6)
> Comment on attachment 831086 [details] [diff] [review]
> open on first click if not logged in
> 
> Review of attachment 831086 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I guess this is fine, so long as you are sure that everyone who supports (or
> is about to support) marks does explicitly tell us about it

yeah, those working on it now can adjust.

> ::: browser/base/content/socialmarks.xml
> @@ +152,2 @@
> >          }
> > +        if (openPanel || !this.provider.haveLoggedInUser())
> 
> what I meant was: let openPanel = this.isMarked || aOpenPanel ||
> !this.provider.haveLoggedInUser();

yeah, it didn't get through my skull until afterwards.  I'll adjust the patch for that.
Comment on attachment 831133 [details] [diff] [review]
open on first click if not logged in

[Approval Request Comment]
Bug caused by (feature/regressing bug #): social marks
User impact if declined: if user is not logged in, social marks may not work on first click
Testing completed (on m-c, etc.): fx-team
Risk to taking this patch (and alternatives if risky): low, current implementations are in progress and will adjust
String or IDL/UUID changes made by this patch: none
Attachment #831133 - Flags: approval-mozilla-aurora?
https://hg.mozilla.org/mozilla-central/rev/ff80ceff2e75
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 28
Keywords: verifyme
Comment on attachment 831133 [details] [diff] [review]
open on first click if not logged in

Adding verifyme so QA can help with Social marks testing.
Attachment #831133 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Attached image ScreenShot .png
I confirm that with Firefox 27 beta 4 on Mac OS X 10.8.5 in 32-bit mode, if the user is not logged in, the mark panel is shown on first click.

I've also noticed that on first click the page is already marked, as shown in the screenshot by the presence of the "Unmark" the button. Is this expected?
Flags: needinfo?(mixedpuppy)
Forgot to mention in comment 14 that I've used the http://mixedpuppy.github.io/socialapi-demo/ demo for testing.
(In reply to Manuela Muntean [:Manuela] [QA] from comment #14)

> I've also noticed that on first click the page is already marked, as shown
> in the screenshot by the presence of the "Unmark" the button. Is this
> expected?

Yes, the demo provider sends the message to mark the page when that page is loaded.
Flags: needinfo?(mixedpuppy)
Thanks Shane! Marking this verified based on comment 14 and comment 16.
QA Contact: manuela.muntean
Verified as fixed on latest Aurora 28.0a2 (buildID: 20140113004002) using Mac OS X 10.8.5
Status: RESOLVED → VERIFIED
Keywords: verifyme
Product: Firefox → Firefox Graveyard
You need to log in before you can comment on or make changes to this bug.