1000134 - Desktop client needs to revoke a generated URL

Status: RESOLVED FIXED

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

Comment 1

[Mark Banner (:standard8)]

Attachment: Sample UX

Attaching sample UX from docs

Comment 2

[Maire Reavy [:mreavy]]

Updating to match trello

Comment 3

[Andrei Oprea [mention me on [e-mail]]]

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

Comment 4

[Michael Maslaney [:mmaslaney] ([e-mail])]

For the button, use the hover/active states defined in the spec and native platform-styling for the dropdown:

http://cl.ly/image/261j2I280u2m

Comment 5

[Andrei Oprea [mention me on [e-mail]]]

(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

Comment 6

[Michael Maslaney [:mmaslaney] ([e-mail])]

(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

Comment 7

[Andrei Oprea [mention me on [e-mail]]]

Attachment: Implement revoke generated URL for incoming call view

Comment 8

[Andrei Oprea [mention me on [e-mail]]]

Attachment: declineblock.jpg

Comment 9

[Dan Mosedale (:dmosedale, :dmose)]

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.

Comment 10

[Sevaan Franks [:sevaan]]

Comment on attachment 8460544 [details]
declineblock.jpg

Looks good :andreio! Thanks.

Comment 11

[Andrei Oprea [mention me on [e-mail]]]

Attachment: Implement revoke generated URL for incoming call view

Comment 12

[Dan Mosedale (:dmosedale, :dmose)]

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.

Comment 13

[Dan Mosedale (:dmosedale, :dmose)]

Attachment: Implement revoke generated URL for incoming call view, r=dmose

Comment 14

[Dan Mosedale (:dmosedale, :dmose)]

Comment on attachment 8462879 [details] [diff] [review]
Implement revoke generated URL for incoming call view, r=dmose

With review comments addressed.

Comment 15

[Dan Mosedale (:dmosedale, :dmose)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/ca1c3ca7d314

Comment 16

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/ca1c3ca7d314

Comment 17

[u279076]

Does this have sufficient in-testsuite coverage or will this require manual smoketesting?

Comment 18

[Mark Banner (:standard8)]

Removing qe-verify since this is basically gone away now.