From e0a51edd856b36bfdf1cacb4d7e8d70fad7884fa Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Alexandre=20K=C3=BChn?= Date: Sat, 11 Aug 2018 21:18:51 +0200 Subject: [PATCH] [FIX] mail: tests: 'load more messages on scroll' works with phantomjs Before this commit, this test was failing due to phantomjs assuming that there could be more messages to display on the screen. The issues comes from the bad support of flexbox with PhantomJS, and this test broke when we changed a DIV display with 'flex' instead of 'block'. For instance, this is the expected values: thread clientHeight: 960 thread scrollHeight: 1629 message clientHeight: 50 With 30 messages, 50 x 30 = 1500 < 1629 and 960 < 1629, so there are enough messages loaded with this screen and we are not at the top of the thread (this is the case when clientHeight = scrollHeight). However, with phantomJS we have the following values: thread clientHeight: 944 thread scrollHeight: 944 message clientHeight: 259 As a result, with 30 messages, 50 x 30 = 1500 > 944 (looks like messages do not vertically collide ?) and 944 = 944 so it assumes we are at the top of the thread, so it fetches for more messages. This commit fixes this test by enforcing a special rendering of the thread for phantomJS. Unfortunately, we can't simply make the hack on the test only. We have to make discuss and thread aware of this because they should adapt their rendering right away --- addons/mail/static/src/js/discuss.js | 12 ++++++++---- addons/mail/static/src/xml/thread.xml | 10 ++++++++-- addons/mail/static/tests/discuss_tests.js | 3 ++- addons/mail/static/tests/helpers/test_utils.js | 8 ++++++-- 4 files changed, 24 insertions(+), 9 deletions(-) diff --git a/addons/mail/static/src/js/discuss.js b/addons/mail/static/src/js/discuss.js index d3ed2b06a1d..ede167aa6a5 100644 --- a/addons/mail/static/src/js/discuss.js +++ b/addons/mail/static/src/js/discuss.js @@ -196,10 +196,10 @@ var Discuss = AbstractAction.extend(ControlPanelMixin, { /** * @override * @param {Object} [options] - // * @param {boolean} [options.phantomjs=false] if set, rendering of non-empty - // * thread will use 'block' display instead of 'flex', because phantomjs - // * sucks with flexbox... TODO: remove this option when we no longer use - // * phantomJS + * @param {boolean} [options.phantomjs=false] if set, rendering of non-empty + * thread will use 'block' display instead of 'flex', because phantomjs + * sucks with flexbox... TODO: remove this option when we no longer use + * phantomJS */ init: function (parent, action, options) { this._super.apply(this, arguments); @@ -223,6 +223,9 @@ var Discuss = AbstractAction.extend(ControlPanelMixin, { this._selectedMessage = null; this._throttledUpdateThreads = _.throttle( this._updateThreads.bind(this), 100, { leading: false }); + + // TODO: remove this option when we no longer use phantomJS + this._withPhantomJS = this.options.phantomjs || false; }, /** * @override @@ -456,6 +459,7 @@ var Discuss = AbstractAction.extend(ControlPanelMixin, { displayEmailIcons: false, displayReplyIcons: true, displayBottomThreadFreeSpace: true, + withPhantomJS: this._withPhantomJS, // TODO: remove this option when we no longer use phantomJS }; }, /** diff --git a/addons/mail/static/src/xml/thread.xml b/addons/mail/static/src/xml/thread.xml index b33452382ac..9b81e5e8d28 100644 --- a/addons/mail/static/src/xml/thread.xml +++ b/addons/mail/static/src/xml/thread.xml @@ -156,6 +156,9 @@ @param {boolean} [options.displayLoadMore=false] @param {string|integer} [options.messagesSeparatorPosition] 'top' or message ID, the separator is placed just after this message. + @param {boolean} [options.withPhantomJS=false] phantomJS is a mess with flexbox + so it may break some features like auto-fetch message on scroll. + TODO: remove this option when we no longer use phantomJS _____________ | | @@ -169,7 +172,7 @@ --> -
+
@@ -182,6 +185,9 @@ @param {Object} options @param {boolean} [options.displayBottomThreadFreeSpace=false] @param {boolean} [options.displayLoadMore=false] + @param {boolean} [options.withPhantomJS=false] phantomJS is a mess with flexbox + so it may break some features like auto-fetch message on scroll. + TODO: remove this option when we no longer use phantomJS _____________ | | @@ -194,7 +200,7 @@ DESC Order --> -
+
diff --git a/addons/mail/static/tests/discuss_tests.js b/addons/mail/static/tests/discuss_tests.js index 6f605d24680..4129911e654 100644 --- a/addons/mail/static/tests/discuss_tests.js +++ b/addons/mail/static/tests/discuss_tests.js @@ -406,7 +406,8 @@ QUnit.test('older messages are loaded on scroll', function (assert) { } return this._super.apply(this, arguments); }, - }).then(function (discuss) { + }, { phantomjs: true }) // TODO: remove this option when we no longer use phantomJS + .then(function (discuss) { assert.verifySteps(['message_fetch'], "should fetch messages once for needaction messages (Inbox)"); diff --git a/addons/mail/static/tests/helpers/test_utils.js b/addons/mail/static/tests/helpers/test_utils.js index 033866522e6..5e670701494 100644 --- a/addons/mail/static/tests/helpers/test_utils.js +++ b/addons/mail/static/tests/helpers/test_utils.js @@ -25,9 +25,13 @@ var Widget = require('web.Widget'); * This is async due to mail_manager/mail_service that needs to be ready. * * @param {Object} params + * @param {Object} options + * @param {boolean} options.phantomjs if set, rendering of non-empty thread will + * use 'block' display instead of 'flex', because phantomjs sucks with flexbox + * TODO: remove this option when we no longer use phantomJS * @return {$.Promise} resolved with the discuss widget */ -function createDiscuss(params) { +function createDiscuss(params, options) { var Parent = Widget.extend({ do_push_state: function () {}, }); @@ -37,7 +41,7 @@ function createDiscuss(params) { 'mail.message,false,search': '', }, })); - var discuss = new Discuss(parent, params); + var discuss = new Discuss(parent, params, options); discuss.set_cp_bus(new Widget()); var selector = params.debug ? 'body' : '#qunit-fixture'; var controlPanel = new ControlPanel(parent);