Closed Bug 1102841 Opened 10 years ago Closed 9 years ago

Choosing "cancel and block" on a direct call doesn't work

Categories

(Hello (Loop) :: Client, defect, P1)

defect
Points:
5

Tracking

(firefox34+ verified, firefox35+ verified, firefox36 verified)

VERIFIED FIXED
mozilla36
Iteration:
36.3
Tracking Status
firefox34 + verified
firefox35 + verified
firefox36 --- verified
backlog Fx34+

People

(Reporter: mreavy, Assigned: abr)

References

Details

Attachments

(3 files, 3 obsolete files)

+++ This bug was initially created as a clone of Bug #1102806 +++

Choosing "cancel and block" on a direct call doesn't handle the block.
Assignee: nobody → mdeboer
Status: NEW → ASSIGNED
Iteration: --- → 36.3
Points: --- → 5
Flags: qe-verify+
Flags: needinfo?(mmucci)
Flags: firefox-backlog+
Added to IT 36.3
Flags: needinfo?(mmucci)
I'm working on tests for all the direct calling stuff. They're missing.
Attachment #8526757 - Flags: review?(standard8)
Comment on attachment 8526757 [details] [diff] [review]
Patch 1: implement Cancel and Block a call

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

::: browser/components/loop/content/js/conversation.jsx
@@ +19,5 @@
>    var OutgoingConversationView = loop.conversationViews.OutgoingConversationView;
>    var CallIdentifierView = loop.conversationViews.CallIdentifierView;
>    var DesktopRoomConversationView = loop.roomViews.DesktopRoomConversationView;
>  
> +  var EMAIL_OR_PHONE_RE = /^(:?\S+@\S+|\+\d+)$/;

comment with a description of the intended set to match
Attachment #8526757 - Flags: feedback+
Comment on attachment 8526757 [details] [diff] [review]
Patch 1: implement Cancel and Block a call

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

r+, assuming you take care of the error handling issue that I highlight.

::: browser/components/loop/content/js/conversation.jsx
@@ +19,5 @@
>    var OutgoingConversationView = loop.conversationViews.OutgoingConversationView;
>    var CallIdentifierView = loop.conversationViews.CallIdentifierView;
>    var DesktopRoomConversationView = loop.roomViews.DesktopRoomConversationView;
>  
> +  var EMAIL_OR_PHONE_RE = /^(:?\S+@\S+|\+\d+)$/;

I'm a little sad that we have this regex both here and in LoopCalls.jsm. If you can give a little thought about how to prevent replication, I'd appreciate it.

@@ +511,5 @@
> +
> +      // If this is a direct call, we'll need to block the caller directly.
> +      if (callerId && EMAIL_OR_PHONE_RE.test(callerId)) {
> +        navigator.mozLoop.calls.blockDirectCaller(callerId, function(err) {
> +          throw err;

This throw here isn't going to do what you want to -- it's just going to fall off the end of the Task promise chain inside blockDirectCaller, and produce an ugly "you used Promises wrong" warning on the console.

Either add a final .catch() clause inside blockDirectCaller that does something sensible with this, or add more reasonable error handling here. Minimally, that means logging the error. Given the nature of this failure ("you asked me to block someone, but I didn't"), it's also something we really want to surface to the user. Do that here or open a new bug to do so (and put the bug number here).
Attachment #8526757 - Flags: review?(standard8) → review+
(In reply to Adam Roach [:abr] from comment #4)
> I'm a little sad that we have this regex both here and in LoopCalls.jsm. If
> you can give a little thought about how to prevent replication, I'd
> appreciate it.

I just figured out how to circumvent doing this. Will put that in v2.

> Either add a final .catch() clause inside blockDirectCaller that does
> something sensible with this, or add more reasonable error handling here.

The `.then(callback, callback)` already does this; an exception is passed as an argument to the reject case.

> Minimally, that means logging the error. Given the nature of this failure
> ("you asked me to block someone, but I didn't"), it's also something we
> really want to surface to the user. Do that here or open a new bug to do so
> (and put the bug number here).

I'll log the error; the conversation window closes too soon to bubble the error upward so we'd need to show it in the panel that's active. Or something.

Bug will be filed.
fixed typos; works for link-clicker (still can block) and FxA direct (cancel only); on direct still has the dropdown arrow with no dropdown
Attachment #8526842 - Attachment is obsolete: true
Comment on attachment 8526757 [details] [diff] [review]
Patch 1: implement Cancel and Block a call

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

One more comment -- the changes in MozLoopAPI.jsm seem to be unrelated to the purpose of this patch, and we're getting flak for the patch being "too big to uplift." Please remove them from this patch, and open a new bug to improve the error handling.
(In reply to Mike de Boer [:mikedeboer] from comment #5)
> The `.then(callback, callback)` already does this; an exception is passed as
> an argument to the reject case.

I don't think you're getting what I'm saying. Follow it from there. What happens? Which callback gets called? And what does it do?
Comment on attachment 8526876 [details] [diff] [review]
alternate patch to remove "Cancel and Block" for FxA calls

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

::: browser/components/loop/content/js/conversation.jsx
@@ +100,5 @@
>        });
>  
> +      var buttonBlockClasses = React.addons.classSet({
> +        "btn-block": true,
> +        "hide": !this.props.model.get("callToken")

Can we add a class to the btn-group instead of the dropdown list item to say to hide the secondary button? This would make the decline&block not accessible and the arrow gone for direct calls but still visible for link-clickers.
Blocks: 1103150
Attachment #8526757 - Attachment is obsolete: true
Assignee: mdeboer → adam
New patch addresses key comments from me and Jesup on patch #1.

Filed Bug 1103156 for the MozLoopAPI.jsm changes.

Filed Bug 1103150 for surfacing "failure to block caller" errors to the user.

Running patch through its paces now, prior to landing.
Attachment #8526991 - Flags: review+
Attachment #8526991 - Attachment is obsolete: true
Attachment #8526996 - Flags: review+
https://hg.mozilla.org/integration/fx-team/rev/39a1305d7119
Tested locally; all looks good, both blocking people in the list and those not in the list
[Tracking Requested - why for this release]:

The shortcut for "cancel and block" off the cancel button doesn't work -- and we need to decide to fix it or remove it.

-----------------

Hi Mike -- Could you provide your analysis for how risky your patch ("Implement Cancel and Block a call") is?  

After talking with several folks on the project and connected to the project, your patch is the one we really want to take all the way up to Aurora and Beta(Fx35 and Fx34).  There is just general concern about the risk since we're so close to releasing Fx34 and the patch is not small.  I know when we talked on irc on Friday, you felt the risk was "quite low" to Hello/Loop (and of course no risk outside of Hello/Loop).  If you could provide more detail about the risk, that would be very useful to me and others (like lmandel) who are deciding what to take for Beta.

The other patch on this bug is mostly done (we just need to remove the arrow from the "cancel" button), but it does take away a very useful shortcut to the block function -- and most folks I've talked to (myself included after careful thought this weekend) are not comfortable with removing it.  I haven't heard back yet from Mika (who did Loop's/Hello's privacy review).

Thanks!
Flags: needinfo?(mdeboer)
backlog: --- → Fx34+
https://hg.mozilla.org/mozilla-central/rev/39a1305d7119
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla36
(In reply to Maire Reavy [:mreavy] (Plz needinfo me) from comment #15)
> There is just general concern about the
> risk since we're so close to releasing Fx34 and the patch is not small.

Two notes on patch size:

1) The patch is now smaller by one file than it was when the release drivers first looked at it; and

2) The patch *looks* one file larger than it really is: conversation.js is a generated file. The source is conversation.jsx. The file "conversation.js" wouldn't typically be part of the patch, except that it requires node.js to transpile, and including node.js as part of the normal compilation is controversial.

So, when evaluating the size of this patch, we're talking about:

a) A small (~30 line) new function, and a stub call to that function in LoopCalls.jsm
b) A new conditional statement in conversation.jsx that calls that function iff the incoming call is a direct call (again, this looks larger than it is because of indenting changes to existing code); and
c) One new field in the conversation store.
Comment on attachment 8526996 [details] [diff] [review]
implement Cancel and Block a call.

Approval Request Comment
[Feature/regressing bug #]: "Cancel and block" shortcut off the Cancel button
[User impact if declined]: "Cancel and block" (if you want to cancel a call and block the caller) won't be a one-step process, and the user will need to cancel the call and then go into the panel and add a contact to block based on who was calling them (which is shown).  This requires a certain knowledge of how blocking contacts from the Hello panel works.
[Describe test coverage new/current, TBPL]: manually tested
[Risks and why]: Risk to Hello is low;  patch is not as big as it seems.  No risk outside of Hello.  Without it, the user would have to jump through more hurdles to block a caller.
[String/UUID change made/needed]: No strings
Attachment #8526996 - Flags: approval-mozilla-beta?
Attachment #8526996 - Flags: approval-mozilla-aurora?
(In reply to Adam Roach [:abr] from comment #17)
> a) A small (~30 line) new function, and a stub call to that function in
> LoopCalls.jsm
> b) A new conditional statement in conversation.jsx that calls that function
> iff the incoming call is a direct call (again, this looks larger than it is
> because of indenting changes to existing code); and
> c) One new field in the conversation store.

This all makes sense but this is still a patch that has just landed on m-c, for which I have no information about test coverage, and that doesn't appear to fix a stop ship issue. We are about to merge mozilla-beta to mozilla-release and kick off our release candidate build. Do you think this truly blocks the release?
Flags: needinfo?(adam)
Comment on attachment 8526996 [details] [diff] [review]
implement Cancel and Block a call.

I spoke with Maire about this. Because of the privacy implication, she thinks this is a must have for release. While she said she wouldn't hold the release for this, should would advocate for shipping a point release to get this fix. This fix has been manually tested and the potential fallout is limited. As such, we'll take this in our 34 RC. I'm denying this page as it doesn't apply cleanly to Beta. abr is working on a Beta specific patch now and I'm told should have it ready shortly.
Flags: needinfo?(adam)
Attachment #8526996 - Flags: approval-mozilla-beta? → approval-mozilla-beta-
(In reply to Lawrence Mandel [:lmandel] (use needinfo) from comment #19)
> (In reply to Adam Roach [:abr] from comment #17)
> > a) A small (~30 line) new function, and a stub call to that function in
> > LoopCalls.jsm
> > b) A new conditional statement in conversation.jsx that calls that function
> > iff the incoming call is a direct call (again, this looks larger than it is
> > because of indenting changes to existing code); and
> > c) One new field in the conversation store.
> 
> This all makes sense but this is still a patch that has just landed on m-c,
> for which I have no information about test coverage, and that doesn't appear
> to fix a stop ship issue. We are about to merge mozilla-beta to
> mozilla-release and kick off our release candidate build. Do you think this
> truly blocks the release?

I understand your concern, but the current behavior is a serious flaw that has implications that are at least tangentially related to privacy: users believe they've prevented a user from pestering them, but they have not. This is, IMHO, a hard release blocker.

We can fix it one of two ways; either by removing the option from the UI, or by making it work. Both are a change, and both are going to have approximately as much test coverage. Since we need to land one of them, the decision hangs on which one makes more sense. There's a smaller patch that makes things much worse for users (but at least doesn't mislead them), and a larger (but still small) patch that simply makes the feature work as expected.

Testing so far has been manual, but given the nature of the change in the larger patch, this doesn't cause me any heartburn. There's no risk for variance among platforms, and the two tests Maire ran have exercised all the code paths save for database errors.
(In reply to Lawrence Mandel [:lmandel] (use needinfo) from comment #20)
> abr is working on a Beta specific patch now
> and I'm told should have it ready shortly.

To be clear, :mikedeboer is taking care of putting this uplift patch together.
Flags: needinfo?(mdeboer)
Attachment #8527828 - Flags: review+
Comment on attachment 8527828 [details] [diff] [review]
Beta branch patch

Approval Request Comment: see comment 18.
Attachment #8527828 - Flags: approval-mozilla-beta?
Comment on attachment 8527828 [details] [diff] [review]
Beta branch patch

Beta+ as per my comment 20.
Attachment #8527828 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Comment on attachment 8526996 [details] [diff] [review]
implement Cancel and Block a call.

Mike tells me that this patch applies cleanly to Aurora but requires that bug 1102806 be uplifted as well.
Does this have Engineering testsuite coverage?
Flags: in-testsuite?
Flags: in-moztrap?
I've tested this on top of current beta, with all scenario variations (cancel, accept, cancel█ with both direct calls from someone in your addressbook and someone not in it; unblocking afterwards and calling; and also for good measure tested blocking a call from a link-clicker.  All work as expected.
Attachment #8526996 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
(In reply to Anthony Hughes, QA Mentor (:ashughes) from comment #27)
> Does this have Engineering testsuite coverage?

Not as far as I can see -- filed Bug 1104273.
Also tested on tbpl build from Ryan's uplift to the release repo; all works correctly.
Verified fixed FF 34 RC on Win 7
Untracking for Moztrap coverage given comment 29.
Flags: in-moztrap? → in-moztrap-
Verified fixed FF 35b3, 36.0a2 (2014-12-15) Win 7
Status: RESOLVED → VERIFIED
Clearing in-testsuite requests for features that are being removed from Hello as part of the user journey work in bug 1209713.
Flags: in-testsuite?
You need to log in before you can comment on or make changes to this bug.