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)
Hello (Loop)
Client
Tracking
(firefox34+ verified, firefox35+ verified, firefox36 verified)
backlog | Fx34+ |
People
(Reporter: mreavy, Assigned: abr)
References
Details
Attachments
(3 files, 3 obsolete files)
4.08 KB,
patch
|
Details | Diff | Splinter Review | |
10.40 KB,
patch
|
abr
:
review+
lsblakk
:
approval-mozilla-aurora+
lmandel
:
approval-mozilla-beta-
|
Details | Diff | Splinter Review |
10.41 KB,
patch
|
mikedeboer
:
review+
lmandel
:
approval-mozilla-beta+
|
Details | Diff | Splinter Review |
+++ This bug was initially created as a clone of Bug #1102806 +++ Choosing "cancel and block" on a direct call doesn't handle the block.
Updated•10 years ago
|
Assignee: nobody → mdeboer
Status: NEW → ASSIGNED
Iteration: --- → 36.3
Points: --- → 5
Flags: qe-verify+
Flags: needinfo?(mmucci)
Flags: firefox-backlog+
Comment 2•10 years ago
|
||
I'm working on tests for all the direct calling stuff. They're missing.
Attachment #8526757 -
Flags: review?(standard8)
Comment 3•10 years ago
|
||
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+
Assignee | ||
Comment 4•10 years ago
|
||
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+
Comment 5•10 years ago
|
||
(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.
Comment 6•10 years ago
|
||
Comment 7•10 years ago
|
||
fixed typos; works for link-clicker (still can block) and FxA direct (cancel only); on direct still has the dropdown arrow with no dropdown
Updated•10 years ago
|
Attachment #8526842 -
Attachment is obsolete: true
Assignee | ||
Comment 8•10 years ago
|
||
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.
Assignee | ||
Comment 9•10 years ago
|
||
(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 10•10 years ago
|
||
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.
Assignee | ||
Comment 11•10 years ago
|
||
Assignee | ||
Updated•10 years ago
|
Attachment #8526757 -
Attachment is obsolete: true
Assignee | ||
Updated•10 years ago
|
Assignee: mdeboer → adam
Assignee | ||
Comment 12•10 years ago
|
||
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.
Assignee | ||
Updated•10 years ago
|
Attachment #8526991 -
Flags: review+
Assignee | ||
Comment 13•10 years ago
|
||
Assignee | ||
Updated•10 years ago
|
Attachment #8526991 -
Attachment is obsolete: true
Assignee | ||
Updated•10 years ago
|
Attachment #8526996 -
Flags: review+
Comment 14•10 years ago
|
||
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
Reporter | ||
Comment 15•9 years ago
|
||
[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!
tracking-firefox34:
--- → ?
Flags: needinfo?(mdeboer)
Reporter | ||
Updated•9 years ago
|
backlog: --- → Fx34+
Comment 16•9 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/39a1305d7119
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla36
Assignee | ||
Comment 17•9 years ago
|
||
(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.
Reporter | ||
Comment 18•9 years ago
|
||
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?
Comment 19•9 years ago
|
||
(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 20•9 years ago
|
||
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-
Assignee | ||
Comment 21•9 years ago
|
||
(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.
Assignee | ||
Comment 22•9 years ago
|
||
(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.
Comment 23•9 years ago
|
||
Flags: needinfo?(mdeboer)
Attachment #8527828 -
Flags: review+
Comment 24•9 years ago
|
||
Comment on attachment 8527828 [details] [diff] [review] Beta branch patch Approval Request Comment: see comment 18.
Attachment #8527828 -
Flags: approval-mozilla-beta?
Comment 25•9 years ago
|
||
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 26•9 years ago
|
||
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.
Comment 27•9 years ago
|
||
Does this have Engineering testsuite coverage?
status-firefox34:
--- → affected
status-firefox35:
--- → affected
status-firefox36:
--- → fixed
Flags: in-testsuite?
Flags: in-moztrap?
Updated•9 years ago
|
tracking-firefox35:
--- → +
Comment 28•9 years ago
|
||
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.
Updated•9 years ago
|
Attachment #8526996 -
Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Assignee | ||
Comment 29•9 years ago
|
||
(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.
Comment 30•9 years ago
|
||
https://hg.mozilla.org/releases/mozilla-beta/rev/75519081f3bb https://hg.mozilla.org/releases/mozilla-release/rev/75519081f3bb
Comment 32•9 years ago
|
||
Also tested on tbpl build from Ryan's uplift to the release repo; all works correctly.
Comment 33•9 years ago
|
||
Verified fixed FF 34 RC on Win 7
Comment 34•9 years ago
|
||
Untracking for Moztrap coverage given comment 29.
Flags: in-moztrap? → in-moztrap-
Comment 35•9 years ago
|
||
Verified fixed FF 35b3, 36.0a2 (2014-12-15) Win 7
Comment 36•9 years ago
|
||
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.
Description
•