1060610 - Loop Client shouldn't note URL expiration time before it can be used

Status: VERIFIED FIXED

(Hello (Loop) :: Client, defect)

Description

[Adam Roach [:abr]]

Currently, the desktop Loop client updates the "latest URL expiration date" whenever a new link is generated. Whenever this value is in the future, the client will ensure that a simple push registration stays up. This means that simply opening the panel (say, out of curiosity), and then closing it without using the URL will cause a simple push registration to happen, and will do so for the next 30 days.

To reduce load on the simple push server, we shouldn't cause this to happen until the user indicates further interest in the service, by taking action to exfiltrate the link.

Comment 1

[Adam Roach [:abr]]

Attachment: Don't update latest callUrl expiration until it is exfiltrated

This patch moves the point at which URL expiration is noted. Instead of updating when the link is generated, it instead updates it when one of the three following events happens:

- The user clicks on the "copy" button
- The user clicks on the "email link" button
- The user copies the contents of the text field (as captured by the "onCopy" react event)

Comment 2

[Adam Roach [:abr]]

Anthony: This would be tricky to automate; here are some MozTrap instructions:

1) In Firefox, type "about:config" in the URL bar
2) In the "search" box, type (or paste) "loop.urlsExpiryTimeSeconds"
3) Take a note of the current value of the associated pref
4) Click on the Loop/Hello button, and wait for a new URL to appear
5) Verify that the value of the pref has not changed.
6) Click on the "Email" button and verify that the value *has* changed.
7) Close the Loop/Hello panel.
8) Repeat steps 3 through 7, except press the "Copy" button instead of the "Email" button.
9) Repeat steps 3 through 7, except copy the link by highlighting, right-clicking, and selecting "copy" instead of pressing the "Email" button.
10) Repeat steps 3 through 7, except copy the link by highlighting it and pressing CTRL-C (Windows/Linux) or Command-C (Mac) instead of pressing the "Email" button.

Comment 3

[Mark Banner (:standard8)]

Comment on attachment 8481597 [details] [diff] [review]
Don't update latest callUrl expiration until it is exfiltrated

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

This looks reasonable. The unit tests need updating to go with it.

My other concern is that we're currently not going to catch it if the user does ctrl-c on the url. Maybe we can detect that?

Comment 4

[Adam Roach [:abr]]

(In reply to Mark Banner (:standard8) from comment #3)
> Comment on attachment 8481597 [details] [diff] [review]
> This looks reasonable. The unit tests need updating to go with it.

Cool, thanks.

> My other concern is that we're currently not going to catch it if the user
> does ctrl-c on the url. Maybe we can detect that?

That's what the "onCopy" does here:

> <input type="url" value={this.state.callUrl} readOnly="true"
>		onCopy={this.handleLinkExfiltration}
>		className={inputCSSClass} />

Comment 5

[Adam Roach [:abr]]

Attachment: Don't update latest callUrl expiration until it is exfiltrated

Now with client tests fixed and added.

Comment 6

[Maire Reavy [:mreavy]]

[Tracking Requested - why for this release]:

This goes with Bug 1055139 and Bug 1055319.  We need this to reduce load on the push infrastructure.  This may land before uplift, but if it doesn't, we need this in Fx34.

Comment 7

[Mark Banner (:standard8)]

Comment on attachment 8481655 [details] [diff] [review]
Don't update latest callUrl expiration until it is exfiltrated

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

Looks good, r=Standard8

Comment 8

[Adam Roach [:abr]]

https://hg.mozilla.org/integration/fx-team/rev/5bd6ed513f88

Comment 9

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/5bd6ed513f88

Comment 10

[u279076]

(In reply to Adam Roach [:abr] from comment #2)
> Anthony: This would be tricky to automate; here are some MozTrap
> instructions:
> 
> 1) In Firefox, type "about:config" in the URL bar
> 2) In the "search" box, type (or paste) "loop.urlsExpiryTimeSeconds"
> 3) Take a note of the current value of the associated pref
> 4) Click on the Loop/Hello button, and wait for a new URL to appear
> 5) Verify that the value of the pref has not changed.
> 6) Click on the "Email" button and verify that the value *has* changed.
> 7) Close the Loop/Hello panel.
> 8) Repeat steps 3 through 7, except press the "Copy" button instead of the
> "Email" button.
> 9) Repeat steps 3 through 7, except copy the link by highlighting,
> right-clicking, and selecting "copy" instead of pressing the "Email" button.
> 10) Repeat steps 3 through 7, except copy the link by highlighting it and
> pressing CTRL-C (Windows/Linux) or Command-C (Mac) instead of pressing the
> "Email" button.

Thanks, I'll get those added to Moztrap, but what is the expected result? How do I check to confirm that "expiration" is occurring as expected?

Comment 11

[Paul Silaghi, QA [:pauly]]

(In reply to Anthony Hughes, QA Mentor (:ashughes) from comment #10)
> How do I check to confirm that "expiration" is occurring as expected?
I guess the value of the pref should change for steps 8-10.
Verified fixed 34.0a2 (2014-09-05) Win 7, OS X 10.9.5, Ubuntu 13.04

Comment 12

[Adam Roach [:abr]]

(In reply to Paul Silaghi, QA [:pauly] from comment #11)
> (In reply to Anthony Hughes, QA Mentor (:ashughes) from comment #10)
> > How do I check to confirm that "expiration" is occurring as expected?
> I guess the value of the pref should change for steps 8-10.
> Verified fixed 34.0a2 (2014-09-05) Win 7, OS X 10.9.5, Ubuntu 13.04

Sorry, I missed Anthony's question (please needinfo me!) -- Paul is correct. If you want to be rigorous, you can check that the new value is exactly 30 days in the future, but that's probably overkill. Noting that it changes should be sufficient.

Comment 13

[u279076]

Sorry for the delay. I have now added a testcase:
https://moztrap.mozilla.org/manage/case/14646/

Please review it and send me feedback.

Comment 14

[Adam Roach [:abr]]

(In reply to Anthony Hughes, QA Mentor (:ashughes) from comment #13)
> Sorry for the delay. I have now added a testcase:
> https://moztrap.mozilla.org/manage/case/14646/
> 
> Please review it and send me feedback.

It looks good overall. The only real issue I see is that you're calling the Loop button an "Invite someone to talk" button, and it might not be clear what is meant by this term.

One very minor nit: on the Mac keyboard, the key is always labeled "Command," not "CMD" - I suggest either using either "Command-C" or "⌘C" in the instruction sequence.

Comment 15

[u279076]

(In reply to Adam Roach [:abr] from comment #14)
> It looks good overall. The only real issue I see is that you're calling the
> Loop button an "Invite someone to talk" button, and it might not be clear
> what is meant by this term.

The tooltip refers to this as "Invite someone to talk" -- what else should I call it? I think calling the button by the tooltip is less ambiguous than just calling it the "Loop" button. Also, I refer to this button as "Invite someone to talk" throughout all of my tests, so at least it's consistent.

Comment 16

[Adam Roach [:abr]]

(In reply to Anthony Hughes, QA Mentor (:ashughes) from comment #15)
> (In reply to Adam Roach [:abr] from comment #14)
> > It looks good overall. The only real issue I see is that you're calling the
> > Loop button an "Invite someone to talk" button, and it might not be clear
> > what is meant by this term.
> 
> The tooltip refers to this as "Invite someone to talk" -- what else should I
> call it? I think calling the button by the tooltip is less ambiguous than
> just calling it the "Loop" button. Also, I refer to this button as "Invite
> someone to talk" throughout all of my tests, so at least it's consistent.

Ah! Okay. To give you an idea of how prominent that is, though, I work on the project, and didn't know the button had a tooltip. :)

You might consider putting some text at the top explaining that the "Invite someone to talk" button is the toolbar button that is used to interact with the Loop service (or something like that).

But if this is how you've done it elsewhere, I guess it's okay as-is.

Comment 17

[u279076]

Great, thanks for your help with this Adam.