[IMP] mail: closed chat window using keyboard ESC key #16138

mitali-willdooit · 2017-03-29T11:42:51Z

Description of the issue/feature this PR addresses:
Task: https://www.odoo.com/web#id=32316&view_type=form&model=project.task&action=333&active_id=131&menu_id=4720
Pad: https://pad.odoo.com/p/r.65cde9c930c29052ed5b151ba0b419c0
Current behavior before PR:

Desired behavior after PR is merged:

--
I confirm I have signed the CLA and read the PR guidelines at www.odoo.com/submit-pr

hmo-odoo · 2017-04-20T12:39:19Z

addons/mail/static/src/js/chat_window.js

+    set_focus:function() {
+        var self = this;
+        // Need Timeout: Text selected and first click on chat window should set focus to composer again
+        setTimeout(function() {


why need setTimeout ?

ged-odoo

The code is quite good, but I requested a few changes, to make it even better. Please ping me when you are done

ged-odoo · 2017-05-16T14:06:25Z

addons/mail/static/src/js/composer.js

+                    event.stopPropagation();
+                    this.mention_manager.reset_suggestions();
+                } else {
+                    this.trigger("keyup_escape");


this should do a trigger_up, not a trigger (we want to catch the event somewhere up the component tree)

also, the name of the event is not so good. rename keyup_escape into 'escape_pressed'

ged-odoo · 2017-05-16T14:09:19Z

addons/mail/static/src/js/extended_chat_window.js

@@ -39,6 +39,11 @@ return ChatWindow.extend({
                basic_composer.mention_set_enabled_commands(commands);
                basic_composer.mention_set_prefetched_partners(partners);
            });
+            basic_composer.on('keyup_escape', self, function () {


you should not bind an event handler on the basic composer. The event should come from a trigger_up, so, it should be bound in the custom_events key on top of this file.

ged-odoo · 2017-05-16T14:11:26Z

addons/mail/static/tests/chatter_tests.js

+
+    var upKeyEvent = jQuery.Event( "keyup", {which: 27});
+    var close_chat = chat_window.$el.find('.o_composer_input').trigger(upKeyEvent);
+    assert.equal(!chat_window.folded, true, "Closed chat Window");


I think that you should write assert.strictEqual(chat_window.folded, false, "chat window should be closed")

ged-odoo · 2017-05-16T14:12:08Z

addons/mail/static/tests/chatter_tests.js

+    assert.equal(chat_window.$input.is(":focus"), true, "Focused");
+
+    var upKeyEvent = jQuery.Event( "keyup", {which: 27});
+    var close_chat = chat_window.$el.find('.o_composer_input').trigger(upKeyEvent);


you can use chat_window.$(...) instead of chat_window.$el.find

ged-odoo · 2017-05-16T14:13:26Z

addons/mail/static/tests/chatter_tests.js

@@ -819,5 +821,41 @@ QUnit.test('followers widget: do not display follower duplications', function (a
    form.destroy();
 });

+QUnit.test('chatter: close chat window using ESCAPE key', function (assert) {


test should not be named with 'chatter': it is a chat window test, so maybe, just "chatter: close chat window using ESCAPE key"

Also, this is not a chatter_test, so it should go into a new file, chat_window_test.js

ged-odoo

I requested some other small changes. once it is done, it should be green

ged-odoo · 2017-05-17T08:25:01Z

addons/mail/static/src/js/extended_chat_window.js

@@ -9,6 +9,9 @@ var composer = require('mail.composer');

 return ChatWindow.extend({
    template: "mail.ExtendedChatWindow",
+    custom_events: {


this should be in chat_window.js instead.

ged-odoo · 2017-05-17T08:25:15Z

addons/mail/static/src/js/extended_chat_window.js

@@ -43,6 +46,11 @@ return ChatWindow.extend({
        }
        return $.when(this._super(), def);
    },
+    _onEscapePressed: function(event) {


and this should also be in chat_window.js

ged-odoo · 2017-05-17T08:26:58Z

addons/mail/static/src/js/chat_window.js

@@ -140,6 +141,12 @@ return Widget.extend({
            this.trigger("fold_channel", this.channel_id, this.folded);
        }
    },
+    _onChatWindowClicked: function() {
+        var selectObj = window.getSelection();
+        if (selectObj.toString().length === 0){


here, you should replace this line with:
if (selectObj.anchorOffset === selectObj.focusOffset){

ged-odoo · 2017-05-17T08:27:53Z

addons/mail/static/tests/chat_window_test.js

+
+QUnit.module('mail', {}, function () {
+
+    QUnit.test('chat_window: close using ESCAPE key', function (assert) {


add a line QUnit.module('chat_window'); above this line. This is useful to separate properly the tests when looking at them in the logs

ged-odoo · 2017-05-17T08:28:06Z

addons/mail/static/tests/chat_window_test.js

+        data: {},
+    });
+
+    var chat_window = new ChatWindow(parent, 1, "user", false, messages, {});


rename chat_window into chatWindow

also, we cannot test that the window is closed at the end, but we can test that the close_chat_session event has been triggered. You can add these lines:

chatWindow.on('close_chat_session', null, function () { assert.ok(true, "chat window should trigger a close event"); });

(and you will need to change the assert.expect accordingly)

ged-odoo · 2017-05-17T08:28:21Z

addons/mail/static/tests/chat_window_test.js

+    chat_window.appendTo($('#qunit-fixture'));
+
+    chat_window.thread.$el.trigger("click");
+    assert.equal(chat_window.$input.is(":focus"), true, "Focused");


use assert.strictEqual

ged-odoo · 2017-05-17T08:28:57Z

addons/mail/static/tests/chat_window_test.js

+    var upKeyEvent = jQuery.Event( "keyup", {which: 27});
+    chat_window.$('.o_composer_input').trigger(upKeyEvent);
+    assert.strictEqual(chat_window.folded, false, "Closed chat Window");
+});


you forgot to destroy the chatwindow at the end of the test. This is a memory leak, but is easily fixed: add chatWindow.destroy(); just before the end of the test

ged-odoo · 2017-05-17T10:50:19Z

merged in master (commits are squashed: 6ea3b8d)

mitali-willdooit force-pushed the master-mail-close-chat-window-mpa branch from a16f35c to 808b176 Compare March 29, 2017 12:40

C3POdoo added the RD research & development, internal work label Mar 29, 2017

mitali-willdooit force-pushed the master-mail-close-chat-window-mpa branch from 808b176 to 78ac1c4 Compare March 30, 2017 13:45

msh-odoo force-pushed the master-mail-close-chat-window-mpa branch from 237d852 to e8094d5 Compare March 31, 2017 10:24

mitali-willdooit force-pushed the master-mail-close-chat-window-mpa branch 2 times, most recently from c68a1a2 to 394728e Compare April 11, 2017 09:22

msh-odoo force-pushed the master-mail-close-chat-window-mpa branch from 394728e to c68a1a2 Compare April 11, 2017 09:37

mitali-willdooit force-pushed the master-mail-close-chat-window-mpa branch 2 times, most recently from 1429258 to c8640c3 Compare April 18, 2017 05:34

mitali-willdooit force-pushed the master-mail-close-chat-window-mpa branch from c8640c3 to 003e298 Compare April 19, 2017 10:20

hmo-odoo reviewed Apr 20, 2017

View reviewed changes

mitali-willdooit force-pushed the master-mail-close-chat-window-mpa branch 3 times, most recently from a7bf407 to f7afd8f Compare April 24, 2017 06:55

mitali-willdooit force-pushed the master-mail-close-chat-window-mpa branch 5 times, most recently from 29c65ed to dd52db1 Compare May 2, 2017 06:43

mitali-willdooit force-pushed the master-mail-close-chat-window-mpa branch from dd52db1 to 9421e15 Compare May 16, 2017 11:33

ged-odoo requested changes May 16, 2017

View reviewed changes

mitali-willdooit force-pushed the master-mail-close-chat-window-mpa branch 6 times, most recently from 93c4a35 to 4b5db84 Compare May 17, 2017 08:00

ged-odoo requested changes May 17, 2017

View reviewed changes

[IMP] mail: closed chat window using keyboard ESC key

b6bbdd5

mitali-willdooit force-pushed the master-mail-close-chat-window-mpa branch from 4b5db84 to 00cb949 Compare May 17, 2017 09:49

[ADD] mail: added qunit testcase

6221a63

mitali-willdooit force-pushed the master-mail-close-chat-window-mpa branch from 00cb949 to 6221a63 Compare May 17, 2017 09:52

ged-odoo closed this May 17, 2017

hmo-odoo deleted the master-mail-close-chat-window-mpa branch May 17, 2017 12:22

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[IMP] mail: closed chat window using keyboard ESC key #16138

[IMP] mail: closed chat window using keyboard ESC key #16138

mitali-willdooit commented Mar 29, 2017

hmo-odoo Apr 20, 2017

ged-odoo left a comment

ged-odoo May 16, 2017

ged-odoo May 16, 2017

ged-odoo May 16, 2017

ged-odoo May 16, 2017

ged-odoo May 16, 2017

ged-odoo May 16, 2017

ged-odoo left a comment

ged-odoo May 17, 2017

ged-odoo May 17, 2017

ged-odoo May 17, 2017

ged-odoo May 17, 2017

ged-odoo May 17, 2017

ged-odoo May 17, 2017

ged-odoo May 17, 2017

ged-odoo May 17, 2017

ged-odoo commented May 17, 2017


		QUnit.module('mail', {}, function () {

		QUnit.test('chat_window: close using ESCAPE key', function (assert) {

[IMP] mail: closed chat window using keyboard ESC key #16138

[IMP] mail: closed chat window using keyboard ESC key #16138

Conversation

mitali-willdooit commented Mar 29, 2017

Choose a reason for hiding this comment

ged-odoo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ged-odoo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ged-odoo commented May 17, 2017