From cce67a0a4d86bb2d370d624271fd3624f3de8198 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Thibault=20Delavall=C3=A9e?= Date: Thu, 2 Dec 2021 11:47:10 +0000 Subject: [PATCH] [REF] rating, various: cleanup rating from routes PURPOSE Cleanup code and flow of customer rating: routes, rating_apply, rating and message creation and update. SPECIFICATIONS Cleanup code about rating controllers and rating_apply. Notably correctly link a message to a rating once a feedback is posted, whatever the flow. Update ``rating_apply`` to use either a token, either an existing rating to udpate it and post a message. Always link the message and the rating to be sure DB data are coherent. This requires an update in several demo data to correctly set tokens on ratings and use it in calls to ``rating_apply`` done in xml files. Extract default subtype computation of rating_apply in a sub-method to allow setting this parameter without having to deal with ``rating_apply`` details. CODE CLEANUP Split behavior that is generic but actually used only in project about stage update based on rating. Move it directly in project. As it is used only once and as generic approach is hardcoded based on fields names better keep it localized. Task-2812665 (Rating: Cleanup rating flow code) Part-of: odoo/odoo#80707 --- .../data/im_livechat_channel_demo.xml | 39 ++++++++--- addons/mail/models/mail_thread.py | 8 +-- addons/project/data/project_demo.xml | 16 +++-- addons/project/models/project.py | 12 +++- addons/rating/__manifest__.py | 2 +- addons/rating/controllers/main.py | 49 +++++++++---- addons/rating/models/mail_thread.py | 16 ++++- addons/rating/models/rating.py | 7 +- addons/rating/models/rating_mixin.py | 68 +++++++++++-------- .../sale_timesheet/data/sale_service_demo.xml | 9 ++- addons/test_mail_full/__manifest__.py | 1 + .../data/mail_message_subtype_data.xml | 10 +++ .../models/test_mail_models_mail.py | 3 + addons/test_mail_full/tests/test_rating.py | 38 +++++++++-- 14 files changed, 202 insertions(+), 76 deletions(-) create mode 100644 addons/test_mail_full/data/mail_message_subtype_data.xml diff --git a/addons/im_livechat/data/im_livechat_channel_demo.xml b/addons/im_livechat/data/im_livechat_channel_demo.xml index 360a4646a42..6027cf3e7f9 100644 --- a/addons/im_livechat/data/im_livechat_channel_demo.xml +++ b/addons/im_livechat/data/im_livechat_channel_demo.xml @@ -25,13 +25,15 @@ Visitor #234, Mitchell Admin + LIVECHAT_1 - + mail.channel @@ -81,13 +83,15 @@ Visitor #323, Marc Demo + LIVECHAT_2 - + mail.channel @@ -137,13 +141,16 @@ Joel Willis, Mitchell Admin + LIVECHAT_3 - + + mail.channel @@ -185,13 +192,16 @@ Joel Willis, Marc Demo + LIVECHAT_4 - + + mail.channel @@ -225,13 +235,15 @@ Visitor #532, Mitchell Admin + LIVECHAT_5 - + mail.channel @@ -273,13 +285,15 @@ Visitor #649, Mitchell Admin + LIVECHAT_6 - + mail.channel @@ -321,13 +335,15 @@ Joel Willis, Mitchell Admin + LIVECHAT_7 - + mail.channel @@ -369,13 +385,15 @@ Visitor #722, Marc Demo + LIVECHAT_8 - + mail.channel @@ -470,14 +488,15 @@ mail.channel + LIVECHAT_9 - - + diff --git a/addons/mail/models/mail_thread.py b/addons/mail/models/mail_thread.py index a28122f38cc..b59ea4a8d7e 100644 --- a/addons/mail/models/mail_thread.py +++ b/addons/mail/models/mail_thread.py @@ -2930,9 +2930,6 @@ class MailThread(models.AbstractModel): # DISCORDUSS API # ------------------------------------------------------ - def _message_update_content_after_hook(self, message): - """ Hook to add custom behavior after having updated the message content. """ - def _message_add_reaction_after_hook(self, message, content): """ Hook to add custom behavior after having added a reaction to a message. """ @@ -2940,7 +2937,7 @@ class MailThread(models.AbstractModel): """ Hook to add custom behavior after having removed a reaction from a message. """ # ------------------------------------------------------ - # WRAPPERS AND TOOLS + # THREAD MESSAGE UPDATE # ------------------------------------------------------ def message_change_thread(self, new_thread, new_parent_message=False): @@ -2983,6 +2980,9 @@ class MailThread(models.AbstractModel): msg_not_comment.write(msg_vals) return True + def _message_update_content_after_hook(self, message): + """ Hook to add custom behavior after having updated the message content. """ + # ------------------------------------------------------ # CONTROLLERS # ------------------------------------------------------ diff --git a/addons/project/data/project_demo.xml b/addons/project/data/project_demo.xml index f4b1d689012..79ed148921a 100644 --- a/addons/project/data/project_demo.xml +++ b/addons/project/data/project_demo.xml @@ -1227,36 +1227,44 @@ Send it ASAP, its urgent. + PROJECT_1 - + + PROJECT_2 - + + PROJECT_3 - + + PROJECT_4 - + diff --git a/addons/project/models/project.py b/addons/project/models/project.py index c6a2abbdd64..6d80e90a3f9 100644 --- a/addons/project/models/project.py +++ b/addons/project/models/project.py @@ -9,6 +9,7 @@ from datetime import timedelta, datetime, time from random import randint from odoo import api, Command, fields, models, tools, SUPERUSER_ID, _, _lt +from odoo.addons.rating.models.rating_data import RATING_LIMIT_OK from odoo.exceptions import UserError, ValidationError, AccessError from odoo.osv.expression import OR, AND from odoo.tools.misc import get_lang @@ -2278,8 +2279,15 @@ class Task(models.Model): return self.project_id.partner_id return res - def rating_apply(self, rate, token=None, feedback=None, subtype_xmlid=None): - return super(Task, self).rating_apply(rate, token=token, feedback=feedback, subtype_xmlid="project.mt_task_rating") + def rating_apply(self, rate, token=None, rating=None, feedback=None, subtype_xmlid=None): + rating = super(Task, self).rating_apply(rate, token=token, rating=rating, feedback=feedback, subtype_xmlid=subtype_xmlid) + if self.stage_id and self.stage_id.auto_validation_kanban_state: + kanban_state = 'done' if rating.rating >= RATING_LIMIT_OK else 'blocked' + self.write({'kanban_state': kanban_state}) + return rating + + def _rating_apply_get_default_subtype_id(self): + return self.env['ir.model.data']._xmlid_to_res_id("project.mt_task_rating") def _rating_get_parent_field_name(self): return 'project_id' diff --git a/addons/rating/__manifest__.py b/addons/rating/__manifest__.py index c2588e06b8d..6752803e432 100644 --- a/addons/rating/__manifest__.py +++ b/addons/rating/__manifest__.py @@ -3,7 +3,7 @@ { 'name': 'Customer Rating', - 'version': '1.0', + 'version': '1.1', 'category': 'Productivity', 'description': """ This module allows a customer to give rating. diff --git a/addons/rating/controllers/main.py b/addons/rating/controllers/main.py index 7f075f7b0d4..e3b3bca5e24 100644 --- a/addons/rating/controllers/main.py +++ b/addons/rating/controllers/main.py @@ -2,6 +2,7 @@ # Part of Odoo. See LICENSE file for full copyright and licensing details. import logging +import werkzeug from odoo import http from odoo.http import request @@ -20,31 +21,51 @@ class Rating(http.Controller): @http.route('/rate//', type='http', auth="public", website=True) def action_open_rating(self, token, rate, **kwargs): - assert rate in (1, 3, 5), "Incorrect rating" + if rate not in (1, 3, 5): + raise ValueError(_("Incorrect rating: should be 1, 3 or 5 (received %d)"), rate) + rating = request.env['rating.rating'].sudo().search([('access_token', '=', token)]) if not rating: - return request.not_found() - rate_names = { - 5: _("Satisfied"), - 3: _("Okay"), - 1: _("Dissatisfied") - } + return werkzeug.exceptions.Forbidden() + + record_sudo = request.env[rating.res_model].sudo().browse(rating.res_id) + if not record_sudo.exists(): + return werkzeug.exceptions.Forbidden() + rating.write({'rating': rate, 'consumed': True}) + lang = rating.partner_id.lang or get_lang(request.env).code return request.env['ir.ui.view'].with_context(lang=lang)._render_template('rating.rating_external_page_submit', { - 'rating': rating, 'token': token, - 'rate_names': rate_names, 'rate': rate + 'rating': rating, + 'token': token, + 'rate_names': { + 5: _("Satisfied"), + 3: _("Okay"), + 1: _("Dissatisfied"), + }, + 'rate': rate, }) @http.route(['/rate//submit_feedback'], type="http", auth="public", methods=['post'], website=True) - def action_submit_rating(self, token, **kwargs): - rate = int(kwargs.get('rate')) - assert rate in (1, 3, 5), "Incorrect rating" + def action_submit_rating(self, token, rate, **kwargs): + rate = int(rate) + if rate not in (1, 3, 5): + raise ValueError(_("Incorrect rating: should be 1, 3 or 5 (received %d)"), rate) + rating = request.env['rating.rating'].sudo().search([('access_token', '=', token)]) if not rating: - return request.not_found() + return werkzeug.exceptions.Forbidden() + record_sudo = request.env[rating.res_model].sudo().browse(rating.res_id) - record_sudo.rating_apply(rate, token=token, feedback=kwargs.get('feedback')) + if not record_sudo.exists(): + return werkzeug.exceptions.Forbidden() + + record_sudo.rating_apply(rate, + rating=rating, + feedback=kwargs.get('feedback'), + subtype_xmlid=None, # force default subtype choice + ) + lang = rating.partner_id.lang or get_lang(request.env).code return request.env['ir.ui.view'].with_context(lang=lang)._render_template('rating.rating_external_page_view', { 'web_base_url': rating.get_base_url(), diff --git a/addons/rating/models/mail_thread.py b/addons/rating/models/mail_thread.py index 8725bc81c44..eacfc11931e 100644 --- a/addons/rating/models/mail_thread.py +++ b/addons/rating/models/mail_thread.py @@ -1,7 +1,7 @@ # -*- coding: utf-8 -*- # Part of Odoo. See LICENSE file for full copyright and licensing details. -from odoo import api, models +from odoo import api, models, _ class MailThread(models.AbstractModel): @@ -9,6 +9,7 @@ class MailThread(models.AbstractModel): @api.returns('mail.message', lambda value: value.id) def message_post(self, **kwargs): + rating_id = kwargs.pop('rating_id', False) rating_value = kwargs.pop('rating_value', False) rating_feedback = kwargs.pop('rating_feedback', False) message = super(MailThread, self).message_post(**kwargs) @@ -25,4 +26,17 @@ class MailThread(models.AbstractModel): 'consumed': True, 'partner_id': self.env.user.partner_id.id, }) + elif rating_id: + self.env['rating.rating'].browse(rating_id).write({'message_id': message.id}) + return message + + def _message_create(self, values_list): + """ Force usage of rating-specific methods and API allowing to delegate + computation to records. Keep methods optimized and skip rating_ids + support to simplify MailThrad main API. """ + if not isinstance(values_list, (list)): + values_list = [values_list] + if any(values.get('rating_ids') for values in values_list): + raise ValueError(_("Posting a rating should be done using message post API.")) + return super()._message_create(values_list) diff --git a/addons/rating/models/rating.py b/addons/rating/models/rating.py index 5cae372d02e..dcab6718b2a 100644 --- a/addons/rating/models/rating.py +++ b/addons/rating/models/rating.py @@ -42,6 +42,7 @@ class Rating(models.Model): partner_id = fields.Many2one('res.partner', string='Customer', help="Author of the rating") rating = fields.Float(string="Rating Value", group_operator="avg", default=0, help="Rating value: 0=Unhappy, 5=Happy") rating_image = fields.Binary('Image', compute='_compute_rating_image') + rating_image_url = fields.Char('Image URL', compute='_compute_rating_image') rating_text = fields.Selection(rating_data.RATING_TEXT, string='Rating', store=True, compute='_compute_rating_text', readonly=True) feedback = fields.Text('Comment', help="Reason of the rating") message_id = fields.Many2one( @@ -91,13 +92,17 @@ class Rating(models.Model): self.ensure_one() return 'rating_%s.png' % rating_data._rating_to_threshold(self.rating) + @api.depends('rating') def _compute_rating_image(self): + self.rating_image_url = False + self.rating_image = False for rating in self: try: image_path = get_resource_path('rating', 'static/src/img', rating._get_rating_image_filename()) + rating.rating_image_url = '/rating/static/src/img/%s' % rating._get_rating_image_filename() rating.rating_image = base64.b64encode(open(image_path, 'rb').read()) if image_path else False except (IOError, OSError): - rating.rating_image = False + pass @api.depends('rating') def _compute_rating_text(self): diff --git a/addons/rating/models/rating_mixin.py b/addons/rating/models/rating_mixin.py index ecc661f376b..7001a59592a 100644 --- a/addons/rating/models/rating_mixin.py +++ b/addons/rating/models/rating_mixin.py @@ -173,42 +173,52 @@ class RatingMixin(models.AbstractModel): subtype_id=subtype_id ) - def rating_apply(self, rate, token=None, feedback=None, subtype_xmlid=None): - """ Apply a rating given a token. If the current model inherits from - mail.thread mixin, a message is posted on its chatter. User going through - this method should have at least employee rights because of rating - manipulation (either employee, either sudo-ed in public controllers after - security check granting access). + def rating_apply(self, rate, token=None, rating=None, feedback=None, subtype_xmlid=None): + """ Apply a rating to the record. This rating can either be linked to a + token (customer flow) or directly a rating record (code flow). - :param float rate : the rating value to apply - :param string token : access token - :param string feedback : additional feedback - :param string subtype_xmlid : xml id of a valid mail.message.subtype + If the current model inherits from mail.thread mixin a message is posted + on its chatter. User going through this method should have at least + employee rights as well as rights on the current record because of rating + manipulation and chatter post (either employee, either sudo-ed in public + controllers after security check granting access). - :returns rating.rating record + :param float rate: the rating value to apply (from 0 to 5); + :param string token: access token to fetch the rating to apply (optional); + :param record rating: rating.rating to apply (if no token); + :param string feedback: additional feedback (plaintext); + :param string subtype_xmlid: xml id of a valid mail.message.subtype used + to post the message (if it applies). If not given a classic comment is + posted; + + :returns rating: rating.rating record """ - rating = None + if rate < 0 or rate > 5: + raise ValueError('Wrong rating value. A rate should be between 0 and 5 (received %d).' % rate) if token: rating = self.env['rating.rating'].search([('access_token', '=', token)], limit=1) - else: - rating = self.env['rating.rating'].search([('res_model', '=', self._name), ('res_id', '=', self.ids[0])], limit=1) - if rating: - rating.write({'rating': rate, 'feedback': feedback, 'consumed': True}) - if hasattr(self, 'message_post'): - feedback = tools.plaintext2html(feedback or '') - self.message_post( - body=":%s/5%s" - % (rate, rate, feedback), - subtype_xmlid=subtype_xmlid or "mail.mt_comment", - author_id=rating.partner_id and rating.partner_id.id or None # None will set the default author in mail_thread.py - ) - if hasattr(self, 'stage_id') and self.stage_id and hasattr(self.stage_id, 'auto_validation_kanban_state') and self.stage_id.auto_validation_kanban_state: - if rating.rating > 2: - self.write({'kanban_state': 'done'}) - else: - self.write({'kanban_state': 'blocked'}) + if not rating: + raise ValueError('Invalid token or rating.') + + rating.write({'rating': rate, 'feedback': feedback, 'consumed': True}) + if hasattr(self, 'message_post'): + if subtype_xmlid is None: + subtype_id = self._rating_apply_get_default_subtype_id() + else: + subtype_id = self.env['ir.model.data']._xmlid_to_res_id(subtype_xmlid) + feedback = tools.plaintext2html(feedback or '') + self.message_post( + author_id=rating.partner_id.id or None, # None will set the default author in mail_thread.py + body=":%s/5%s" + % (rating.rating_image_url, rate, feedback), + rating_id=rating.id, + subtype_id=subtype_id, + ) return rating + def _rating_apply_get_default_subtype_id(self): + return self.env['ir.model.data']._xmlid_to_res_id("mail.mt_comment") + def _rating_get_repartition(self, add_stats=False, domain=None): """ get the repatition of rating grade for the given res_ids. :param add_stats : flag to add stat to the result diff --git a/addons/sale_timesheet/data/sale_service_demo.xml b/addons/sale_timesheet/data/sale_service_demo.xml index 6590e55aaf0..66532b53668 100644 --- a/addons/sale_timesheet/data/sale_service_demo.xml +++ b/addons/sale_timesheet/data/sale_service_demo.xml @@ -1045,21 +1045,24 @@ + TS_0 - + + TS_1 - - + diff --git a/addons/test_mail_full/__manifest__.py b/addons/test_mail_full/__manifest__.py index 0fdf6b0212d..c0c97caefd4 100644 --- a/addons/test_mail_full/__manifest__.py +++ b/addons/test_mail_full/__manifest__.py @@ -24,6 +24,7 @@ real applications. """, 'sms', ], 'data': [ + 'data/mail_message_subtype_data.xml', 'security/ir.model.access.csv', 'security/ir_rule_data.xml', ], diff --git a/addons/test_mail_full/data/mail_message_subtype_data.xml b/addons/test_mail_full/data/mail_message_subtype_data.xml new file mode 100644 index 00000000000..ad8c172a6d2 --- /dev/null +++ b/addons/test_mail_full/data/mail_message_subtype_data.xml @@ -0,0 +1,10 @@ + + + + Rating Done + Rating Done + mail.test.rating + + + + diff --git a/addons/test_mail_full/models/test_mail_models_mail.py b/addons/test_mail_full/models/test_mail_models_mail.py index 80bdc88dad7..3c254a0c25d 100644 --- a/addons/test_mail_full/models/test_mail_models_mail.py +++ b/addons/test_mail_full/models/test_mail_models_mail.py @@ -51,5 +51,8 @@ class MailTestRating(models.Model): elif not rating.phone_nbr: rating.phone_nbr = False + def _rating_apply_get_default_subtype_id(self): + return self.env['ir.model.data']._xmlid_to_res_id("test_mail_full.mt_mail_test_rating_rating_done") + def rating_get_partner_id(self): return self.customer_id diff --git a/addons/test_mail_full/tests/test_rating.py b/addons/test_mail_full/tests/test_rating.py index a0e867e73e2..097ac9ff9b3 100644 --- a/addons/test_mail_full/tests/test_rating.py +++ b/addons/test_mail_full/tests/test_rating.py @@ -48,27 +48,51 @@ class TestRatingFlow(TestMailFullCommon, TestMailFullRecipients): @mute_logger('odoo.addons.mail.models.mail_mail') def test_rating_rating_apply(self): record_rating = self.env['mail.test.rating'].browse(self.record_rating.ids) + record_messages = record_rating.message_ids # prepare rating token access_token = record_rating.rating_get_access_token() record_rating.invalidate_cache(fnames=['rating_ids']) - # apply a rate + # apply a rate as note (first click) with self.mock_mail_gateway(mail_unlink_sent=False), self.mock_mail_app(): - record_rating.rating_apply(5, token=access_token, feedback='Top Feedback') + record_rating.rating_apply(5, token=access_token, feedback='Top Feedback', subtype_xmlid='mail.mt_note') message = record_rating.message_ids[0] rating = record_rating.rating_ids # check posted message + self.assertEqual(record_rating.message_ids, record_messages + message) self.assertIn('Top Feedback', message.body) - self.assertIn('rating/static/src/img/rating_5.png', message.body) + self.assertIn('/rating/static/src/img/rating_5.png', message.body) self.assertEqual(message.author_id, self.partner_1) - self.assertFalse(message.rating_ids) - self.assertEqual(message.notified_partner_ids, self.partner_admin) - self.assertEqual(message.subtype_id, self.env.ref('mail.mt_comment')) + self.assertEqual(message.rating_ids, rating) + self.assertFalse(message.notified_partner_ids) + self.assertEqual(message.subtype_id, self.env.ref('mail.mt_note')) # check rating update self.assertTrue(rating.consumed) self.assertEqual(rating.feedback, 'Top Feedback') - self.assertFalse(rating.message_id) + self.assertEqual(rating.message_id, message) self.assertEqual(rating.rating, 5) + + # apply a rate again (second click with feedback) + with self.mock_mail_gateway(mail_unlink_sent=False), self.mock_mail_app(): + record_rating.rating_apply(1, token=access_token, feedback='Bad Feedback') + + # check posted message: a new message is posted with default subtype + update_message = record_rating.message_ids[0] + self.assertNotEqual(update_message, message) + self.assertEqual(record_rating.message_ids, record_messages + message + update_message) + self.assertIn('Bad Feedback', update_message.body) + self.assertIn('/rating/static/src/img/rating_1.png', update_message.body) + self.assertEqual(update_message.author_id, self.partner_1) + self.assertEqual(update_message.rating_ids, rating) + self.assertEqual(update_message.notified_partner_ids, self.partner_admin) + self.assertEqual(update_message.subtype_id, self.env.ref("test_mail_full.mt_mail_test_rating_rating_done")) + + # check rating update + rating = record_rating.rating_ids + self.assertTrue(rating.consumed) + self.assertEqual(rating.feedback, 'Bad Feedback') + self.assertEqual(rating.message_id, update_message) + self.assertEqual(rating.rating, 1)