Master website chatter jem #15405

jem-odoo · 2017-02-08T10:39:36Z

Task : https://www.odoo.com/web#id=30059&view_type=form&model=project.task&action=333&active_id=809&menu_id=4720

Purpose :
In all website modules (blog, ecommerce, ...) we can post comments. But having a huge number of comments can be problemtic, since they all will be displayed on the same page (loading page time can be .... long).
The udemy comment zone is quite smart (see https://www.udemy.com/learn-html5-programming-from-scratch/ for instance). There is a pager for comments, and you can filter the rating (ecommerce support rating).

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

tde-banana-odoo · 2017-03-21T08:55:08Z

addons/website_mail/controllers/main.py

+
+def _special_access_object(res_model, res_id, token='', token_field=''):
+    record = request.env[res_model].browse(res_id)
+    if token and record and token == getattr(record.sudo(), token_field, None):


Shouldn't we use consteq to compare tokens ?

Ok, but it have to check than the token_field is filled to avoid given None as argument to consteq. So the condition looks like :
if token and record and getattr(record.sudo(), token_field, None) and consteq(getattr(record.sudo(), token_field, None), str(token)):

tde-banana-odoo · 2017-03-21T08:56:21Z

addons/website_mail/controllers/main.py

@@ -27,7 +33,9 @@ def _message_post_helper(res_model='', res_id=None, message='', token='', token_
    """
    record = request.env[res_model].browse(res_id)
    author_id = request.env.user.partner_id.id if request.env.user.partner_id else False
-    if token and record and token == getattr(record.sudo(), token_field, None):
+
+    access_as_sudo = _special_access_object(res_model, res_id, token=token, token_field=token_field)


Shouldn't we do something if the token is not correct ? If I understand well the record will not be accessed as sudo, but maybe it could just avoid doing stuff if a wrong token is given.

Like raising a forbidden ? It could be cool indeed !

so do we do it ? does it make sense or is it overkill ?

tde-banana-odoo · 2017-03-21T08:57:57Z

addons/website_mail/controllers/main.py

        return request.redirect(url)
+
+    def _website_message_post(self, res_model, res_id, message, **kw):


What is the purpose of that wrapper ?

It is a hook, to create the rating in website_rating. Otherwise, super will return the url, and don't have the message_id to associate the rating with.
Maybe _message_post_helper should be a controller method. What do you think ?

tde-banana-odoo · 2017-03-21T08:59:12Z

addons/website_mail/controllers/main.py

    @http.route(['/website_mail/post/post'], type='http', methods=['POST'], auth='public', website=True)
-    def chatter_post(self, res_model='', res_id=None, message='', redirect=None, **kw):
+    def chatter_post(self, res_model, res_id, message, **kw):


Just a naming thing: chatter_post, website_message_post, message_post_helper, website_chatter_init, website_message_fetch -> there is some space for prefixing convention cleaning ;) .

tde-banana-odoo · 2017-03-21T08:59:49Z

addons/website_mail/controllers/main.py

+
+    @http.route('/website_mail/fetch', type='json', auth='public', website=True)
+    def website_message_fetch(self, res_model, res_id, domain=False, limit=False, offset=False, **kw):
+        if not limit:


Why not putting it directly in the parameters ?

tde-banana-odoo · 2017-03-21T09:01:02Z

addons/website_mail/controllers/main.py

+        # Only search into website_message_ids, so apply the same domain to perform only one search
+        # extract domain from the 'website_message_ids' field
+        field_domain = request.env[res_model]._fields['website_message_ids'].domain
+        domain += field_domain(request.env[res_model].browse(res_id)) if callable(field_domain) else field_domain


Actually I don't think we have to browse res_id. Indeed domains are only model dependent, not record dependent at the ORM level. I think we should keep the same semantic.

Smart guy !

tde-banana-odoo · 2017-03-21T09:01:53Z

addons/website_mail/controllers/main.py

+        domain += [('res_id', '=', res_id)]
+        # Check access
+        Message = request.env['mail.message']
+        access_as_sudo = _special_access_object(res_model, res_id, token=kw.get('token'), token_field=kw.get('token_field'))


For example here, shouldn't a wrong token send you to hell, instead of probably crashing due to access rights issues ?

tde-banana-odoo

Technical review. Ow.

tde-banana-odoo · 2017-03-21T09:04:16Z

addons/website_mail/static/src/js/website_mail.js

+        },
+        fetch_message: function(domain){
+            var self = this;
+            var data = this._prepare_fetch_message_params();


Unnecessary variable

tde-banana-odoo · 2017-03-21T09:04:42Z

addons/website_mail/static/src/js/website_mail.js

+        _load_templates: function(){
+            return ajax.loadXML('/website_mail/static/src/xml/website_mail.xml', qweb);
+        },
+        fetch_message: function(domain){


Shouldn't we use the same naming convention, and use message_fetch, message_fetch_prepare, ... ?

tde-banana-odoo · 2017-03-21T09:05:09Z

addons/website_mail/static/src/js/website_mail.js

+        init: function(parent, options){
+            this._super.apply(this, arguments);
+            console.log($(parent).data());
+            this.options = _.defaults($(parent).data(), {


Strange construct, where does it come from ?

tde-banana-odoo · 2017-03-21T09:07:24Z

addons/website_mail/static/src/js/website_mail.js

+            });
+            return messages;
+        },
+        _pager: function(page, total, step, scope){


I would either remove parameters or explicitely set them to this.get(parameter) when calling the function, to lessen the number of variable processing.

tde-banana-odoo · 2017-03-21T09:07:46Z

addons/website_mail/static/src/js/website_mail.js

+            var pmin = Math.max(page - parseInt(Math.floor(scope/2)), 1);
+            var pmax = Math.min(pmin + scope, page_count);
+
+            if(pmax - pmin < scope){


I simply transform the algo from python side. Without trying to understand ;)

yeah, looks like the python side did the work twice because it wasn't sure it did it well ?

It looks like it's a verification of the min/max done above :p It shouldn't ever be useful

tde-banana-odoo · 2017-03-21T09:19:42Z

addons/website_rating/models/mail_message.py

+            message.rating_value = mapping.get(message.id, 0.0)
+
+    def _search_rating_value(self, operator, value):
+        self.env.cr.execute("""


I fear it will become quickly hard to perform a search. Once you have a lot of ratings, you will have a domain term like [('id', 'in', [A MAGNIFICIENT ARRAY])]. Don't you think we should store the rating value in order to search efficiently on it ?

tde-banana-odoo · 2017-03-21T09:21:46Z

addons/website_rating/static/src/js/website_mail.js

+            this.change_current_page(1, domain);
+        },
+        // utils
+        round_to_half: function(value) {


I am not a JS expert, but don't we have access to some method allowing to do it :p ?

tde-banana-odoo · 2017-03-21T09:21:56Z

addons/website_rating/static/src/js/website_mail.js

+            if(this.get('rating_value')){
+                domain = [['rating_value', '=', this.get('rating_value')]];
+            }
+            console.log('change to ',this.get('rating_value'));


Leftover log.

tde-banana-odoo · 2017-03-21T09:22:15Z

addons/website_sale/models/__init__.py

@@ -1,5 +1,4 @@
 import ir_http
-import rating


Normal, since I have moved this file from website_sale ;)

tde-banana-odoo · 2017-03-21T09:24:38Z

addons/website_mail/security/website_mail_security.xml

@@ -2,6 +2,12 @@
 <odoo>
    <data noupdate="1">

+        <record id="mail_message_rule_public" model="ir.rule">


There is some mismatch in the commits. One commit add a hardcoded domain, one commit adds a void file, and this one finishes the work by removing the hardcoded domain and adding stuff in the void file. I fear you will have to do strange stuff when rebasing. Bon courage.

Icallhimtest · 2017-06-15T08:12:46Z

addons/website_mail/controllers/main.py

+
+def _special_access_object(res_model, res_id, token='', token_field=''):
+    record = request.env[res_model].browse(res_id)
+    if token and record and getattr(record.sudo(), token_field, None) and consteq(getattr(record.sudo(), token_field, None), token):


Maybe we should remove the None from the second getattr(record.sudo(), token_field, None)

We already know it exists so no default is needed, and should we remove the verification before somehow, consteq raises a TypeError with a None argument.

I'd also do a .exists() on the record, and maybe put the .sudo() on the first line too.

Icallhimtest · 2017-06-15T08:25:03Z

addons/website_mail/controllers/main.py

@@ -27,7 +33,9 @@ def _message_post_helper(res_model='', res_id=None, message='', token='', token_
    """
    record = request.env[res_model].browse(res_id)
    author_id = request.env.user.partner_id.id if request.env.user.partner_id else False
-    if token and record and token == getattr(record.sudo(), token_field, None):
+
+    access_as_sudo = _special_access_object(res_model, res_id, token=token, token_field=token_field)


so do we do it ? does it make sense or is it overkill ?

Icallhimtest · 2017-06-16T08:47:59Z

addons/website_mail/static/src/js/website_mail.js

+        //--------------------------------------------------------------------------
+
+        /**
+         * Fetch the messages nad the message count from the server for the


Icallhimtest · 2017-06-16T08:58:22Z

addons/website_mail/static/src/js/website_mail.js

+         *
+         * @private
+         * @params {Number} page
+         * @returns {Object}


optional parameters are set in brackets @param {number} [page]

(+ param singular and JS types don't start with a capital letters :p )

Icallhimtest · 2017-06-16T09:05:05Z

addons/website_mail/static/src/js/website_mail.js

+            var step = this.options['pager_step'];
+
+            // Compute Pager
+            var page_count = parseInt(Math.ceil(parseFloat(total) / step));


I don't think the parseInt and parseFloat are necessary here, unless this.get('message_count') is a string, but I don't think it can be one.

Icallhimtest · 2017-06-16T14:46:02Z

addons/website_mail/static/src/js/website_mail.js

+            if(domain){
+                d = domain;
+            }
+            this.set('domain', d); // trigger fetch message


I don't understand why we set this.get('domain') to a clone of itself, or why we clone the this.get('domain') if we're setting it to the domain parameter.

If it's to trigger the domain onchange, maybe we could just the method instead of cloning it.

if we keep the onchange triggers this way: var d = (domain ? domain : _.clone(this.get('domain'))

But imo changing the domain should change the page (or force its onchange if no page change), and changing the page should trigger the message fetching.

Icallhimtest · 2017-06-16T15:03:52Z

addons/website_mail/static/src/js/website_mail.js

+            var pmin = Math.max(page - parseInt(Math.floor(scope/2)), 1);
+            var pmax = Math.min(pmin + scope, page_count);
+
+            if(pmax - pmin < scope){


yeah, looks like the python side did the work twice because it wasn't sure it did it well ?

It looks like it's a verification of the min/max done above :p It shouldn't ever be useful

Icallhimtest · 2017-06-19T08:03:54Z

addons/website_mail/static/src/xml/website_mail.xml

+                                <input type='hidden' name="sha_time" t-att-value="widget.options['sha_time']" t-if="widget.options['sha_time']"/>
+                                <input type='hidden' name="token_field" t-att-value="widget.options['token_field']" t-if="widget.options['token_field']"/>
+                                <div class="alert alert-danger mt8 mb0 o_website_mail_composer_error" style="display:none;">
+                                    Oops! Something went wrong. Try to reload the page and to log in.


gotto make a choice "and log in" or "to log in" but not "and to log in" ^^

Icallhimtest · 2017-06-19T08:30:27Z

addons/website_rating/static/src/js/website_mail.js

+        /**
+         * Update the messages format
+         *
+         * @params {Array<Object>}


@param

comment to propagate in this file too

Icallhimtest · 2017-06-19T08:50:38Z

addons/website_rating/static/src/js/website_mail.js

+         * @private
+         * @param {MouseEvent} event
+         */
+        _onMoveStar: function(e){


while testing the stars on the website, I found it a bit odd that after you click a certain rating, if you happen to hover on another star, your clicked rating gets lost.

Also, if you send a rating without any message, you get a refresh, but the rating doesn't go through. It'd be better if it didn't submit but had something like when you don't fill a required field on a form.

and it's done !

jem-odoo

first batch of answers

jem-odoo · 2017-06-28T12:59:08Z

addons/website_mail/controllers/main.py

+        is_user_public = bool(request.env.user == request.website.user_id)
+        message_data = self.website_message_fetch(res_model, res_id, domain=domain, limit=limit, **kwargs)
+        display_composer = False
+        if kwargs.get('allow_composer'):


Those are very optional parameters, so not really necessary, IMHO.

jem-odoo · 2017-06-28T12:59:49Z

addons/website_mail/controllers/main.py

+        }
+
+    @http.route('/website_mail/fetch', type='json', auth='public', website=True)
+    def website_message_fetch(self, res_model, res_id, domain=False, limit=10, offset=0, **kw):


same answer here ;)

jem-odoo · 2017-06-28T13:44:27Z

addons/website_mail/static/src/js/website_mail.js

+        //--------------------------------------------------------------------------
+
+        /**
+         * Fetch the messages nad the message count from the server for the


jem-odoo · 2017-06-28T15:23:03Z

addons/website_mail/views/website_mail_templates.xml

-            </ul>
-        </section>
+        <div id="discussion" class="hidden-print o_website_mail_thread"
+            t-att-data-token="token" t-att-public-user="website.env.user == request.website.user_id" t-att-data-res_model="object._name" t-att-data-res_id="object.id" t-att-data-token_field="token_field" t-att-data-pager_step="message_per_page or 10" t-att-data-allow_composer="'0' if disable_composer else '1'">


Setting t-att-token=="object[token_field]" will crash if token_field is not set. And since we usually use the template without the token mecanism, it should be better like this ;)

jem-odoo

second

jem-odoo · 2017-06-29T08:13:48Z

addons/website_mail/static/src/js/website_mail.js

+         * @param {Number} page
+         * @param {Array} domain
+         */
+        _changeCurrentPage: function(page, domain){


Yes that for this, but when changing domain, I have to reset the current_page to trigger the messages fetch. But if I set the current_page to 1, and I'm already on the page one, replacing 1 by 1 does not trigger the fetching. So I have to use the domain, which is an object to trigger the fetching.

jem-odoo · 2017-06-29T08:16:40Z

addons/website_rating/static/src/js/website_mail.js

+         * @private
+         * @param {MouseEvent} event
+         */
+        _onMoveStar: function(e){


and it's done !

mail.thread mixin will (re)provide the field 'website_message_ids'. Models can redifined its domain, since it can differ a bit (blog, forum, ... are not the same). Even if the domain can differ, we still keep it on the mixin since it will be required for the new chatter frontend (see following commits).

Message are now fetched and display using javascript to optimize performance. Also the chatter is completely rewrite : add pager, filter possiblity, ... This commit is mainly technical: posting feature does not change. Only the display with pager is new. The goal is also to prepare frontend chatter for a better integration with rating widget. The mail.thread mixin is extended to manage the field website_message_ids (display all messages that can be seen on frontend by user (employee or not). When fetching or posting message, a check is done to see if we need to do it as sudo(). website_mail implement posting messages with token or sha_in (using in website_quote). The same verification is now done when fetching messages, via '_special_access_object' method.

rating module provide now post a message with a rating value, though `message_post` method. 'mail.message' also has now a `rating_value` field to ease searching and filtering. This will help the ecommerce (and other website modules) to post a comment associate with a rating.

Extend new dynamic chatter to manage rating when posting comment. The model should inherit of rating.mixin and its frontend chatter should allow rating to enjoy star widget and rating card. This is more generic : stats, rating, ... are manage in one place (this module) for all document.

…website_rating

Slide provided a chatter where you can post message as public user. We decided to drop that feature in order to avoid spamming. Like for all other document, you must at least, be portal user to post a comment. Reusing the standart chatter highly simplify the code.

This commit brings a new frontend chatter. The old one displayed all message on the website page, and that could be very slow when many many messages are posted on a document. The new chatter is more dynamic: we introduce a pager, to fetch and display only a few messages. It also still support to post a message if you are not logged in, using a token mecanism (see website_quote). Moreover, a better integration with rating: in the eshop, when posting a comment, the visitor can add a rating value (stars).

jem-odoo added the RD research & development, internal work label Feb 8, 2017

jem-odoo force-pushed the master-website-chatter-jem branch from 5e9d7ff to ec8001c Compare February 8, 2017 12:40

jem-odoo force-pushed the master-website-chatter-jem branch from ec8001c to c4d11db Compare February 16, 2017 09:32

jem-odoo force-pushed the master-website-chatter-jem branch from c4d11db to b383828 Compare February 27, 2017 15:21

jem-odoo force-pushed the master-website-chatter-jem branch from b383828 to eb775a0 Compare March 17, 2017 10:19

tde-banana-odoo reviewed Mar 21, 2017

View reviewed changes

tde-banana-odoo requested changes Mar 21, 2017

View reviewed changes

jem-odoo force-pushed the master-website-chatter-jem branch 4 times, most recently from 5c794d7 to 839be6a Compare March 23, 2017 14:18

jem-odoo force-pushed the master-website-chatter-jem branch from 839be6a to a329f63 Compare April 5, 2017 07:46

jem-odoo force-pushed the master-website-chatter-jem branch 3 times, most recently from 217c6bf to 5fdac06 Compare May 12, 2017 13:35

jem-odoo force-pushed the master-website-chatter-jem branch 5 times, most recently from 5bafd83 to e934cde Compare May 23, 2017 14:56

jem-odoo force-pushed the master-website-chatter-jem branch from e934cde to 344de27 Compare June 14, 2017 07:04

Icallhimtest reviewed Jun 19, 2017

View reviewed changes

jem-odoo commented Jun 28, 2017

View reviewed changes

odoo deleted a comment from tde-banana-odoo Jun 29, 2017

jem-odoo force-pushed the master-website-chatter-jem branch from 344de27 to e3067fb Compare June 29, 2017 07:26

jem-odoo commented Jun 29, 2017

View reviewed changes

jem-odoo force-pushed the master-website-chatter-jem branch from e3067fb to 707ad75 Compare June 29, 2017 08:18

odoo deleted a comment from Icallhimtest Jun 29, 2017

jem-odoo force-pushed the master-website-chatter-jem branch from 707ad75 to b1a97df Compare June 29, 2017 09:09

jem-odoo added 9 commits June 30, 2017 11:06

[IMP] website_quote: clean page style

890e665

[IMP] website_*: adapt website modules using frontend chatter

0339264

[IMP] website_mail: use access right rather then hardcoded domain

548126f

[CLN] rating: remove deprecated stuffs, since they are integrated in …

760c677

…website_rating

jem-odoo force-pushed the master-website-chatter-jem branch from 13f6626 to 870b511 Compare June 30, 2017 09:07

jem-odoo merged commit 66bd45a into odoo:master Jun 30, 2017

jem-odoo deleted the master-website-chatter-jem branch December 12, 2017 14:28

		return request.redirect(url)

		def _website_message_post(self, res_model, res_id, message, **kw):

Master website chatter jem #15405

Master website chatter jem #15405

Conversation

jem-odoo commented Feb 8, 2017 • edited

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tde-banana-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

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

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

Choose a reason for hiding this comment

jem-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

jem-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

jem-odoo commented Feb 8, 2017 •

edited