From 12b580c953a762e40fb4a07a3fe5be3d119fa406 Mon Sep 17 00:00:00 2001 From: RomainLibert Date: Mon, 22 Jan 2018 12:08:56 +0100 Subject: [PATCH] [IMP] rating: Parent document name update For objects inherting from rating.mixin, when their parent model change (e.i.: task from a project to another), the "Parent document name" does not change. The rating is still considered belonging to the old parent object (rating stat of project is not correct). Change the rating API. In rating mixin, instead of having 2 methods returning parent id and parent model (rating_get_parent_id and rating_get_parent_model_name), we should have one method returning the 'parent relation field' (inevitably m2o field). Then we can deduce its parent_res_id/parent_res_model, and check this parent field is in values of write to trigger the recompute of parent_res_name. Also, add "ondelete=cascade" on parent_res_model_id Impacted modules: project, rating, helpdesk, livechat --- addons/im_livechat/models/mail_channel.py | 7 +-- addons/project/models/project.py | 7 +-- addons/project/tests/test_project_flow.py | 72 +++++++++++++++++++++++ addons/rating/models/rating.py | 32 +++++----- 4 files changed, 93 insertions(+), 25 deletions(-) diff --git a/addons/im_livechat/models/mail_channel.py b/addons/im_livechat/models/mail_channel.py index efd2346c2a0..0e140f9de7f 100644 --- a/addons/im_livechat/models/mail_channel.py +++ b/addons/im_livechat/models/mail_channel.py @@ -134,8 +134,5 @@ class MailChannel(models.Model): # Rating Mixin - def rating_get_parent_model_name(self, values): - return 'im_livechat.channel' - - def rating_get_parent_id(self): - return self.livechat_channel_id.id + def rating_get_parent(self): + return 'livechat_channel_id' diff --git a/addons/project/models/project.py b/addons/project/models/project.py index ff9181938bc..4f43b0ae616 100644 --- a/addons/project/models/project.py +++ b/addons/project/models/project.py @@ -1044,11 +1044,8 @@ class Task(models.Model): def rating_apply(self, rate, token=None, feedback=None, subtype=None): return super(Task, self).rating_apply(rate, token=token, feedback=feedback, subtype="project.mt_task_rating") - def rating_get_parent_model_name(self, vals): - return 'project.project' - - def rating_get_parent_id(self): - return self.project_id.id + def rating_get_parent(self): + return 'project_id' class ProjectTags(models.Model): diff --git a/addons/project/tests/test_project_flow.py b/addons/project/tests/test_project_flow.py index 6a7c426f492..c814c0d68cf 100644 --- a/addons/project/tests/test_project_flow.py +++ b/addons/project/tests/test_project_flow.py @@ -1,8 +1,11 @@ # -*- coding: utf-8 -*- # Part of Odoo. See LICENSE file for full copyright and licensing details. +import base64 + from .test_project_base import TestProjectBase from odoo.tools import mute_logger +from odoo.modules.module import get_resource_path EMAIL_TPL = """Return-Path: @@ -117,3 +120,72 @@ class TestProjectFlow(TestProjectBase): }) self.assertEqual(parent_task.partner_id, child_task.partner_id, "Subtask partner should not change when changing project") + + def test_rating(self): + """Check if rating works correctly even when task is changed from project A to project B""" + Task = self.env['project.task'].with_context({'tracking_disable': True}) + first_task = Task.create({ + 'name': 'first task', + 'user_id': self.user_projectuser.id, + 'project_id': self.project_pigs.id, + 'partner_id': self.partner_2.id, + }) + + self.assertEqual(first_task.rating_count, 0, "Task should have no rating associated with it") + + Rating = self.env['rating.rating'] + rating_good = Rating.create({ + 'res_model_id': self.env['ir.model']._get('project.task').id, + 'res_id': first_task.id, + 'parent_res_model_id': self.env['ir.model']._get('project.project').id, + 'parent_res_id': self.project_pigs.id, + 'rated_partner_id': self.partner_2.id, + 'partner_id': self.partner_2.id, + 'rating': 10, + 'consumed': False, + }) + + rating_bad = Rating.create({ + 'res_model_id': self.env['ir.model']._get('project.task').id, + 'res_id': first_task.id, + 'parent_res_model_id': self.env['ir.model']._get('project.project').id, + 'parent_res_id': self.project_pigs.id, + 'rated_partner_id': self.partner_2.id, + 'partner_id': self.partner_2.id, + 'rating': 5, + 'consumed': True, + }) + + # We need to invalidate cache since it is not done automatically by the ORM + # Our One2Many is linked to a res_id (int) for which the orm doesn't create an inverse + first_task.invalidate_cache() + + self.assertEqual(rating_good.rating_text, 'satisfied') + self.assertEqual(rating_bad.rating_text, 'not_satisfied') + self.assertEqual(first_task.rating_count, 1, "Task should have only one rating associated, since one is not consumed") + self.assertEqual(rating_good.parent_res_id, self.project_pigs.id) + self.assertEqual(self.project_goats.percentage_satisfaction_task, -1) + self.assertEqual(self.project_pigs.percentage_satisfaction_task, -1) + + # Consuming rating_good + first_task.rating_apply(10, rating_good.access_token) + + # We need to invalidate cache since it is not done automatically by the ORM + # Our One2Many is linked to a res_id (int) for which the orm doesn't create an inverse + first_task.invalidate_cache() + + self.assertEqual(first_task.rating_count, 2, "Task should have two ratings associated with it") + self.assertEqual(rating_good.parent_res_id, self.project_pigs.id) + self.assertEqual(self.project_goats.percentage_satisfaction_task, -1) + self.assertEqual(self.project_pigs.percentage_satisfaction_task, 50) + + # We change the task from project_pigs to project_goats, ratings should be associated with the new project + first_task.project_id = self.project_goats.id + + # We need to invalidate cache since it is not done automatically by the ORM + # Our One2Many is linked to a res_id (int) for which the orm doesn't create an inverse + first_task.invalidate_cache() + + self.assertEqual(rating_good.parent_res_id, self.project_goats.id) + self.assertEqual(self.project_goats.percentage_satisfaction_task, 50) + self.assertEqual(self.project_pigs.percentage_satisfaction_task, -1) diff --git a/addons/rating/models/rating.py b/addons/rating/models/rating.py index 8995504a6dc..5e46c48aa90 100644 --- a/addons/rating/models/rating.py +++ b/addons/rating/models/rating.py @@ -37,7 +37,7 @@ class Rating(models.Model): res_model = fields.Char(string='Document Model', related='res_model_id.model', store=True, index=True, readonly=True) res_id = fields.Integer(string='Document', required=True, help="Identifier of the rated object", index=True) parent_res_name = fields.Char('Parent Document Name', compute='_compute_parent_res_name', store=True) - parent_res_model_id = fields.Many2one('ir.model', 'Parent Related Document Model', index=True) + parent_res_model_id = fields.Many2one('ir.model', 'Parent Related Document Model', index=True, ondelete='cascade') parent_res_model = fields.Char('Parent Document Model', store=True, related='parent_res_model_id.model', index=True) parent_res_id = fields.Integer('Parent Document', index=True) rated_partner_id = fields.Many2one('res.partner', string="Rated person", help="Owner of the rated resource") @@ -105,10 +105,10 @@ class Rating(models.Model): 'parent_res_model_id': False, 'parent_res_id': False, } - if hasattr(current_record, 'rating_get_parent_model_name'): - parent_res_model = current_record.rating_get_parent_model_name(values) - data['parent_res_model_id'] = self.env['ir.model']._get(parent_res_model).id - data['parent_res_id'] = current_record.rating_get_parent_id() + if hasattr(current_record, 'rating_get_parent'): + parent_res_model = getattr(current_record, current_record.rating_get_parent()) + data['parent_res_model_id'] = self.env['ir.model']._get(parent_res_model._name).id + data['parent_res_id'] = parent_res_model.id return data @api.multi @@ -150,6 +150,7 @@ class RatingMixin(models.AbstractModel): record.rating_last_value = ratings.rating @api.multi + @api.depends('rating_ids') def _compute_rating_count(self): read_group_res = self.env['rating.rating'].read_group( [('res_model', '=', self._name), ('res_id', 'in', self.ids), ('consumed', '=', True)], @@ -161,11 +162,15 @@ class RatingMixin(models.AbstractModel): record.rating_count = result.get(record.id) def write(self, values): - """ If the rated ressource name is modified, we should update the rating res_name too. """ + """ If the rated ressource name is modified, we should update the rating res_name too. + If the rated ressource parent is changed we should update the parent_res_id too""" result = super(RatingMixin, self).write(values) - for r in self: - if r._rec_name in values: - r.rating_ids._compute_res_name() + for record in self: + if record._rec_name in values: + record.rating_ids._compute_res_name() + if record.rating_get_parent() in values: + for rating in record.rating_ids: + rating.parent_res_id = record[record.rating_get_parent()].id return result def unlink(self): @@ -175,12 +180,9 @@ class RatingMixin(models.AbstractModel): self.env['rating.rating'].sudo().search([('res_model', '=', self._name), ('res_id', 'in', record_ids)]).unlink() return result - def rating_get_parent_model_name(self, vals): - """ Return the parent model name """ - return None - - def rating_get_parent_id(self): - """ Return the parent record id """ + def rating_get_parent(self): + """Return the parent relation field name + Should return a Many2One""" return None def rating_get_partner_id(self):