Closed Bug 1000134 Opened 7 years ago Closed 7 years ago

Desktop client needs to revoke a generated URL

Categories

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

defect

Tracking

(Not tracked)

RESOLVED FIXED
34 Sprint 1- 8/4

People

(Reporter: RT, Assigned: aoprea)

References

Details

(Whiteboard: [p=1])

User Story

As a FF browser user (signed-in or not) receiving an incoming call notification originating from a clicked URL, I can decide to revoke that URL so that no one will be able to call me through that URL anymore.

To do:

- Implement chevron "Ignore" button with additional option of "Ignore & Block".
- On selecting "Ignore & Block", send a "DELETE /call-url/{token}" message to the server to indicate block of connection.
- Terminate the call as normal.

https://people.mozilla.org/~dhenein/labs/loop-mvp-spec/#call-incoming
https://docs.services.mozilla.com/loop/apis.html#delete-call-url-token

Attachments

(3 files, 2 obsolete files)

No description provided.
User Story: (updated)
Priority: -- → P3
Whiteboard: [s=ui32]
Target Milestone: --- → mozilla32
Whiteboard: [s=ui32] → [p=?]
Target Milestone: mozilla32 → mozilla33
Summary: Desktop client needs UI to revoke a generated URL → Desktop client needs to revoke a generated URL
Blocks: 1022590
No longer blocks: 990678
Priority: P3 → P1
Blocks: 1027062
Attached image Sample UX
Attaching sample UX from docs
User Story: (updated)
Whiteboard: [p=?] → [p=1]
Updating to match trello
Assignee: nobody → standard8
QA Contact: aoprea
We need a mockup with the state of the dropdown menu when the chevron is clicked. For example this view https://people.mozilla.org/~dhenein/labs/loop-mvp-spec/#start-call-account
Flags: needinfo?(mmaslaney)
For the button, use the hover/active states defined in the spec and native platform-styling for the dropdown:

http://cl.ly/image/261j2I280u2m
Flags: needinfo?(mmaslaney)
(In reply to mmaslaney from comment #4)
> For the button, use the hover/active states defined in the spec and native
> platform-styling for the dropdown:
> 
> http://cl.ly/image/261j2I280u2m

Thanks! I still have some questions.

The mockup is not similar to the native platform style [0] should I follow your mockup ?

What should the hover state background color be ? (at least in osx that color is customizable)

In the MVP spec [1] the answer and ignore buttons are at the bottom of the panel and there is no room for the dropdown panel. Should I raise the buttons as in your mockup ? 

[0] http://i.imgur.com/TuGH0x8.png
[1] https://people.mozilla.org/~dhenein/labs/loop-mvp-spec/#call-incoming-unknown
(In reply to Andrei Oprea [:andreio] from comment #5)
> (In reply to mmaslaney from comment #4)
> > For the button, use the hover/active states defined in the spec and native
> > platform-styling for the dropdown:
> > 
> > http://cl.ly/image/261j2I280u2m
> 
> Thanks! I still have some questions.
> 
> The mockup is not similar to the native platform style [0] should I follow
> your mockup ?

No, please use the native platform styling.

> What should the hover state background color be ? (at least in osx that
> color is customizable)

I would leave the hover state native and avoid customization.
 
> In the MVP spec [1] the answer and ignore buttons are at the bottom of the
> panel and there is no room for the dropdown panel. Should I raise the
> buttons as in your mockup ? 

In this case Yes, I would raise the buttons.

> [0] http://i.imgur.com/TuGH0x8.png
> [1]
> https://people.mozilla.org/~dhenein/labs/loop-mvp-spec/#call-incoming-unknown
Assignee: standard8 → aoprea
Target Milestone: mozilla33 → 34 Sprint 1- 8/4
Attached image declineblock.jpg
Attachment #8460544 - Flags: ui-review?(sfranks)
Comment on attachment 8460354 [details] [diff] [review]
Implement revoke generated URL for incoming call view

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

This is looking great; once we've sanded off a few rough edges, it should be ready to go.

::: browser/components/loop/content/js/client.js
@@ +165,5 @@
> +          cb(null);
> +
> +          this.mozLoop.noteCallUrlExpiry((new Date()).getTime() / 1000);
> +        } catch (err) {
> +          console.log("Error requesting call info", err);

Change this to "Error deleting call info"

::: browser/components/loop/content/js/conversation.jsx
@@ +79,3 @@
>        var conversationPanelClass = "incoming-call " + this._getTargetPlatform();
> +      var cx = React.addons.classSet;
> +      var dropdownMenu = cx({

A different name?  Maybe dropdownMenuClasses?

@@ +88,5 @@
>            <h2>{__("incoming_call")}</h2>
>            <div className="button-group">
> +            <div className={btnClassDecline} onClick={this._handleDecline}>
> +              <span className="btn__chevron-text">
> +                {__("incoming_call_decline_button")}

incoming_call_ignore_button if this string something with the same semantics.

@@ +90,5 @@
> +            <div className={btnClassDecline} onClick={this._handleDecline}>
> +              <span className="btn__chevron-text">
> +                {__("incoming_call_decline_button")}
> +              </span>
> +              <span className="btn__chevron"

If we can stick with prevailing local style (hyphens rather than underscores) for classes, I think that'll make the code more coherent.

@@ +95,5 @@
> +                onMouseEnter={this._toggleDeclineMenu}
> +                onMouseLeave={this._toggleDeclineMenu}>
> +                <ul className={dropdownMenu}>
> +                  <li className="btn-block" onClick={this._handleIgnoreBlock}>
> +                    Decline and Block

Ignore and Block; needs localization

@@ +150,5 @@
>        "call/accept": "accept",
>        "call/decline": "decline",
>        "call/ongoing": "conversation",
> +      "call/ended": "ended",
> +      "call/block": "ignoreAndBlock"

Please make the route use the same more explicit description of what it's doing: call/ignoreAndBlock.

@@ +213,5 @@
>  
>      /**
> +     * Decline & block an incoming call
> +     */
> +    ignoreAndBlock: function() {

Please change the comment to use the word ignore, and add jsdoc descript (@note i guess) about where the token comes from and what we're making happen.

::: browser/components/loop/content/js/panel.jsx
@@ +175,5 @@
>        } else {
> +        var callUrl = callUrlData.callUrl || callUrlData.call_url;
> +        // XXX the current server vers does not implement the callToken field
> +        // but it exists in the API. This workaround should be removed in the future
> +        var token = callUrlData.callToken || callUrl.split('/').pop();

It'd be great to use URL() here if that's practical/easy.

@@ +180,2 @@
>  
> +        navigator.mozLoop.setLoopCharPref('loopToken', token);

This needs some sort of documentation about why it's doing this.

::: browser/components/loop/content/shared/css/common.css
@@ +64,5 @@
>  /* Buttons */
>  
>  .btn {
>    display: inline-block;
> +  position: relative;

Let's avoid fragility as discussed by making two sibling boxes inside a parent.

::: browser/components/loop/content/shared/css/conversation.css
@@ +188,5 @@
> +}
> +
> +.decline-block-menu {
> +  /* Align the menu with the chevron */
> +  bottom: -34px;

At least XXX this with a theory about doing better if it's not easily fixable using containing boxes and justify or align properties or whatever.

::: browser/components/loop/test/desktop-local/client_test.js
@@ +108,5 @@
> +
> +        sinon.assert.calledOnce(callback);
> +        sinon.assert.calledWithMatch(callback, sinon.match(function(err) {
> +          return /400.*invalid token/.test(err.message);
> +        }));

We now have enough instances of this generic matcher that I think it's worth hoisting this out both to save code, but also to make the calling code more readable.

::: browser/components/loop/test/desktop-local/conversation_test.js
@@ +283,5 @@
> +          sinon.assert.calledOnce(log);
> +          sinon.assert.calledWithExactly(log, fakeError);
> +        });
> +
> +        it("close the window", function() {

This wants to start with the word "should".  :-)

@@ +389,5 @@
>          sinon.assert.calledOnce(model.trigger);
>          sinon.assert.calledWith(model.trigger, "decline");
>          });
>      });
> +

We also need to test and implement that clicking on the chevron drops down the menu.

You'll probably need to experiment a bit and/or ping sevaan about how to make the menu disappear.
Attachment #8460354 - Flags: review?(dmose) → review-
Comment on attachment 8460544 [details]
declineblock.jpg

Looks good :andreio! Thanks.
Attachment #8460544 - Flags: ui-review?(sfranks)
Attachment #8460354 - Attachment is obsolete: true
Comment on attachment 8462010 [details] [diff] [review]
Implement revoke generated URL for incoming call view

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

r=dmose in person; comments addressed during review.  No ux-review required as we have a moratorium until the ux/visual refresh lands.
Attachment #8462010 - Flags: review?(dmose) → review+
Comment on attachment 8462879 [details] [diff] [review]
Implement revoke generated URL for incoming call view, r=dmose

With review comments addressed.
Attachment #8462879 - Flags: review+
Attachment #8462010 - Attachment is obsolete: true
https://hg.mozilla.org/mozilla-central/rev/ca1c3ca7d314
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Depends on: 1045569
Does this have sufficient in-testsuite coverage or will this require manual smoketesting?
Flags: qe-verify?
QA Contact: aoprea → anthony.s.hughes
Removing qe-verify since this is basically gone away now.
Flags: qe-verify?
You need to log in before you can comment on or make changes to this bug.