Closed Bug 1060610 Opened 5 years ago Closed 5 years ago

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

Categories

(Hello (Loop) :: Client, defect)

defect
Not set

Tracking

(firefox34+ verified)

VERIFIED FIXED
mozilla34
Tracking Status
firefox34 + verified

People

(Reporter: abr, Assigned: abr)

References

Details

Attachments

(1 file, 1 obsolete file)

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.
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)
Attachment #8481597 - Flags: review?(standard8)
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.
Flags: needinfo?(anthony.s.hughes)
Blocks: 1015988
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?
Attachment #8481597 - Flags: review?(standard8) → feedback+
(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} />
Now with client tests fixed and added.
Attachment #8481655 - Flags: review?(standard8)
Attachment #8481597 - Attachment is obsolete: true
[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 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
Attachment #8481655 - Flags: review?(standard8) → review+
https://hg.mozilla.org/mozilla-central/rev/5bd6ed513f88
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla34
(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?
Flags: needinfo?(anthony.s.hughes)
Flags: qe-verify+
(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
Status: RESOLVED → VERIFIED
(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.
Sorry for the delay. I have now added a testcase:
https://moztrap.mozilla.org/manage/case/14646/

Please review it and send me feedback.
Flags: needinfo?(adam)
(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.
Flags: needinfo?(adam)
(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.
(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.
Great, thanks for your help with this Adam.
You need to log in before you can comment on or make changes to this bug.