From 83d2affbe96976af217ce855ac73f8ab281ee8b5 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Thibault=20Delavall=C3=A9e?= Date: Tue, 22 Feb 2022 14:51:19 +0000 Subject: [PATCH] [MOV][FIX] rating: move rating values, thresholds and converters into a single file Purpose is to try to have a single entry point when dealing with rating values. Also fix incoherency introduced with odoo/odoo@32ba41754996727551106a57d51ac70e92430c04 where grades are not coherent in all computation. This new computation seems better as bad is now limited to <= 1 everywhere. Task-2728564 Part-of: odoo/odoo#82792 --- addons/project/report/project_report.py | 2 +- addons/rating/models/__init__.py | 1 + addons/rating/models/rating.py | 33 ++-------- addons/rating/models/rating_data.py | 73 +++++++++++++++++++++ addons/rating/models/rating_mixin.py | 54 ++++----------- addons/rating/models/rating_parent_mixin.py | 24 +++---- 6 files changed, 101 insertions(+), 86 deletions(-) create mode 100644 addons/rating/models/rating_data.py diff --git a/addons/project/report/project_report.py b/addons/project/report/project_report.py index a40c6dfee97..9b3e911fd17 100644 --- a/addons/project/report/project_report.py +++ b/addons/project/report/project_report.py @@ -3,7 +3,7 @@ from odoo import fields, models, tools -from odoo.addons.rating.models.rating import RATING_LIMIT_MIN +from odoo.addons.rating.models.rating_data import RATING_LIMIT_MIN class ReportProjectTaskUser(models.Model): _name = "report.project.task.user" diff --git a/addons/rating/models/__init__.py b/addons/rating/models/__init__.py index 4492fc6fc0f..d236d87301b 100644 --- a/addons/rating/models/__init__.py +++ b/addons/rating/models/__init__.py @@ -1,6 +1,7 @@ # -*- coding: utf-8 -*- from . import rating +from . import rating_data from . import rating_mixin from . import rating_parent_mixin from . import mail_thread diff --git a/addons/rating/models/rating.py b/addons/rating/models/rating.py index 78d412531b2..5cae372d02e 100644 --- a/addons/rating/models/rating.py +++ b/addons/rating/models/rating.py @@ -4,19 +4,9 @@ import base64 import uuid from odoo import api, fields, models - +from odoo.addons.rating.models import rating_data from odoo.modules.module import get_resource_path -RATING_LIMIT_SATISFIED = 4 -RATING_LIMIT_OK = 3 -RATING_LIMIT_MIN = 1 -RATING_TEXT = [ - ('top', 'Satisfied'), - ('ok', 'Okay'), - ('ko', 'Dissatisfied'), - ('none', 'No Rating yet'), -] - class Rating(models.Model): _name = "rating.rating" @@ -52,7 +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_text = fields.Selection(RATING_TEXT, string='Rating', store=True, compute='_compute_rating_text', readonly=True) + 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( 'mail.message', string="Message", @@ -99,15 +89,7 @@ class Rating(models.Model): def _get_rating_image_filename(self): self.ensure_one() - if self.rating >= RATING_LIMIT_SATISFIED: - rating_int = 5 - elif self.rating >= RATING_LIMIT_OK: - rating_int = 3 - elif self.rating >= RATING_LIMIT_MIN: - rating_int = 1 - else: - rating_int = 0 - return 'rating_%s.png' % rating_int + return 'rating_%s.png' % rating_data._rating_to_threshold(self.rating) def _compute_rating_image(self): for rating in self: @@ -120,14 +102,7 @@ class Rating(models.Model): @api.depends('rating') def _compute_rating_text(self): for rating in self: - if rating.rating >= RATING_LIMIT_SATISFIED: - rating.rating_text = 'top' - elif rating.rating >= RATING_LIMIT_OK: - rating.rating_text = 'ok' - elif rating.rating >= RATING_LIMIT_MIN: - rating.rating_text = 'ko' - else: - rating.rating_text = 'none' + rating.rating_text = rating_data._rating_to_text(rating.rating) @api.model_create_multi def create(self, vals_list): diff --git a/addons/rating/models/rating_data.py b/addons/rating/models/rating_data.py new file mode 100644 index 00000000000..62cd206887a --- /dev/null +++ b/addons/rating/models/rating_data.py @@ -0,0 +1,73 @@ +# -*- coding: utf-8 -*- +# Part of Odoo. See LICENSE file for full copyright and licensing details. + +import operator + +from odoo.tools.float_utils import float_compare + +RATING_AVG_TOP = 3.66 +RATING_AVG_OK = 2.33 +RATING_AVG_MIN = 1 + +RATING_LIMIT_SATISFIED = 4 +RATING_LIMIT_OK = 3 +RATING_LIMIT_MIN = 1 +RATING_TEXT = [ + ('top', 'Satisfied'), + ('ok', 'Okay'), + ('ko', 'Dissatisfied'), + ('none', 'No Rating yet'), +] + +OPERATOR_MAPPING = { + '=': operator.eq, + '!=': operator.ne, + '<': operator.lt, + '<=': operator.le, + '>': operator.gt, + '>=': operator.ge, +} + +def _rating_avg_to_text(rating_avg): + if float_compare(rating_avg, RATING_AVG_TOP, 2) >= 0: + return 'top' + if float_compare(rating_avg, RATING_AVG_OK, 2) >= 0: + return 'ok' + if float_compare(rating_avg, RATING_AVG_MIN, 2) >= 0: + return 'ko' + return 'none' + +def _rating_assert_value(rating_value): + assert 0 <= rating_value <= 5 + +def _rating_to_grade(rating_value): + """ From a rating value give a text-based mean value. """ + _rating_assert_value(rating_value) + if rating_value >= RATING_LIMIT_SATISFIED: + return 'great' + if rating_value >= RATING_LIMIT_OK: + return 'okay' + return 'bad' + +def _rating_to_text(rating_value): + """ From a rating value give a text-based mean value. """ + _rating_assert_value(rating_value) + if rating_value >= RATING_LIMIT_SATISFIED: + return 'top' + if rating_value >= RATING_LIMIT_OK: + return 'ok' + if rating_value >= RATING_LIMIT_MIN: + return 'ko' + return 'none' + +def _rating_to_threshold(rating_value): + """ From a rating value, return the thresholds in form of 0-1-3-5 used + notably for images. """ + _rating_assert_value(rating_value) + if rating_value >= RATING_LIMIT_SATISFIED: + return 5 + if rating_value >= RATING_LIMIT_OK: + return 3 + if rating_value >= RATING_LIMIT_MIN: + return 1 + return 0 diff --git a/addons/rating/models/rating_mixin.py b/addons/rating/models/rating_mixin.py index 4b6aad4668e..2b8ec44136d 100644 --- a/addons/rating/models/rating_mixin.py +++ b/addons/rating/models/rating_mixin.py @@ -4,22 +4,10 @@ import operator from odoo import api, fields, models, tools -from odoo.addons.rating.models.rating import RATING_LIMIT_SATISFIED, RATING_LIMIT_OK, RATING_LIMIT_MIN, RATING_TEXT +from odoo.addons.rating.models import rating_data from odoo.osv import expression from odoo.tools.float_utils import float_compare -OPERATOR_MAPPING = { - '=': operator.eq, - '!=': operator.ne, - '<': operator.lt, - '<=': operator.le, - '>': operator.gt, - '>=': operator.ge, -} -RATING_AVG_TOP = 3.66 -RATING_AVG_OK = 2.33 -RATING_AVG_MIN = RATING_LIMIT_MIN - class RatingMixin(models.AbstractModel): _name = 'rating.mixin' @@ -32,7 +20,7 @@ class RatingMixin(models.AbstractModel): rating_count = fields.Integer('Rating count', compute="_compute_rating_stats", compute_sudo=True) rating_avg = fields.Float("Average Rating", groups='base.group_user', compute='_compute_rating_stats', compute_sudo=True, search='_search_rating_avg') - rating_avg_text = fields.Selection(RATING_TEXT, groups='base.group_user', + rating_avg_text = fields.Selection(rating_data.RATING_TEXT, groups='base.group_user', compute='_compute_rating_avg_text', compute_sudo=True) rating_percentage_satisfaction = fields.Float("Rating Satisfaction", compute='_compute_rating_satisfaction', compute_sudo=True) rating_last_text = fields.Selection(string="Rating Text", groups='base.group_user', related="rating_ids.rating_text") @@ -46,7 +34,7 @@ class RatingMixin(models.AbstractModel): @api.depends('rating_ids.res_id', 'rating_ids.rating') def _compute_rating_stats(self): """ Compute avg and count in one query, as thoses fields will be used together most of the time. """ - domain = expression.AND([self._rating_domain(), [('rating', '>=', RATING_LIMIT_MIN)]]) + domain = expression.AND([self._rating_domain(), [('rating', '>=', rating_data.RATING_LIMIT_MIN)]]) read_group_res = self.env['rating.rating'].read_group(domain, ['rating:avg'], groupby=['res_id'], lazy=False) # force average on rating column mapping = {item['res_id']: {'rating_count': item['__count'], 'rating_avg': item['rating']} for item in read_group_res} for record in self: @@ -54,48 +42,38 @@ class RatingMixin(models.AbstractModel): record.rating_avg = mapping.get(record.id, {}).get('rating_avg', 0) def _search_rating_avg(self, operator, value): - if operator not in OPERATOR_MAPPING: + if operator not in rating_data.OPERATOR_MAPPING: raise NotImplementedError('This operator %s is not supported in this search method.' % operator) rating_read_group = self.env['rating.rating'].sudo().read_group( - [('res_model', '=', self._name), ('consumed', '=', True), ('rating', '>=', RATING_LIMIT_MIN)], + [('res_model', '=', self._name), ('consumed', '=', True), ('rating', '>=', rating_data.RATING_LIMIT_MIN)], ['res_id', 'rating_avg:avg(rating)'], ['res_id']) res_ids = [ res['res_id'] for res in rating_read_group - if OPERATOR_MAPPING[operator](float_compare(res['rating_avg'], value, 2), 0) + if rating_data.OPERATOR_MAPPING[operator](float_compare(res['rating_avg'], value, 2), 0) ] return [('id', 'in', res_ids)] @api.depends('rating_avg') def _compute_rating_avg_text(self): for record in self: - if float_compare(record.rating_avg, RATING_AVG_TOP, 2) >= 0: - record.rating_avg_text = 'top' - elif float_compare(record.rating_avg, RATING_AVG_OK, 2) >= 0: - record.rating_avg_text = 'ok' - elif float_compare(record.rating_avg, RATING_AVG_MIN, 2) >= 0: - record.rating_avg_text = 'ko' - else: - record.rating_avg_text = 'none' + record.rating_avg_text = rating_data._rating_avg_to_text(record.rating_avg) @api.depends('rating_ids.res_id', 'rating_ids.rating') def _compute_rating_satisfaction(self): """ Compute the rating satisfaction percentage, this is done separately from rating_count and rating_avg since the query is different, to avoid computing if it is not necessary""" - domain = expression.AND([self._rating_domain(), [('rating', '>=', RATING_LIMIT_MIN)]]) + domain = expression.AND([self._rating_domain(), [('rating', '>=', rating_data.RATING_LIMIT_MIN)]]) # See `_compute_rating_percentage_satisfaction` above read_group_res = self.env['rating.rating'].read_group(domain, ['res_id', 'rating'], groupby=['res_id', 'rating'], lazy=False) default_grades = {'great': 0, 'okay': 0, 'bad': 0} grades_per_record = {record_id: default_grades.copy() for record_id in self.ids} + for group in read_group_res: record_id = group['res_id'] - rating = group['rating'] - if rating > RATING_LIMIT_OK: - grades_per_record[record_id]['great'] += group['__count'] - elif rating > RATING_LIMIT_MIN: - grades_per_record[record_id]['okay'] += group['__count'] - else: - grades_per_record[record_id]['bad'] += group['__count'] + grade = rating_data._rating_to_grade(group['rating']) + grades_per_record[record_id][grade] += group['__count'] + for record in self: grade_repartition = grades_per_record.get(record.id, default_grades) grade_count = sum(grade_repartition.values()) @@ -272,12 +250,8 @@ class RatingMixin(models.AbstractModel): data = self._rating_get_repartition(domain=domain) res = dict.fromkeys(['great', 'okay', 'bad'], 0) for key in data: - if key >= RATING_LIMIT_SATISFIED: - res['great'] += data[key] - elif key >= RATING_LIMIT_OK: - res['okay'] += data[key] - else: - res['bad'] += data[key] + grade = rating_data._rating_to_grade(key) + res[grade] += data[key] return res def rating_get_stats(self, domain=None): diff --git a/addons/rating/models/rating_parent_mixin.py b/addons/rating/models/rating_parent_mixin.py index 62c040c4ba1..ed8995613d8 100644 --- a/addons/rating/models/rating_parent_mixin.py +++ b/addons/rating/models/rating_parent_mixin.py @@ -1,14 +1,11 @@ # -*- coding: utf-8 -*- # Part of Odoo. See LICENSE file for full copyright and licensing details. -import operator - from collections import defaultdict from datetime import timedelta from odoo import api, fields, models -from odoo.addons.rating.models.rating import RATING_LIMIT_OK, RATING_LIMIT_MIN -from odoo.addons.rating.models.rating_mixin import OPERATOR_MAPPING +from odoo.addons.rating.models import rating_data from odoo.osv import expression from odoo.tools.float_utils import float_compare @@ -36,7 +33,7 @@ class RatingParentMixin(models.AbstractModel): @api.depends('rating_ids.rating', 'rating_ids.consumed') def _compute_rating_percentage_satisfaction(self): # build domain and fetch data - domain = [('parent_res_model', '=', self._name), ('parent_res_id', 'in', self.ids), ('rating', '>=', RATING_LIMIT_MIN), ('consumed', '=', True)] + domain = [('parent_res_model', '=', self._name), ('parent_res_id', 'in', self.ids), ('rating', '>=', rating_data.RATING_LIMIT_MIN), ('consumed', '=', True)] if self._rating_satisfaction_days: domain += [('write_date', '>=', fields.Datetime.to_string(fields.datetime.now() - timedelta(days=self._rating_satisfaction_days)))] data = self.env['rating.rating'].read_group(domain, ['parent_res_id', 'rating'], ['parent_res_id', 'rating'], lazy=False) @@ -47,14 +44,9 @@ class RatingParentMixin(models.AbstractModel): rating_scores_per_parent = defaultdict(int) # contains the total of the rating values per record for item in data: parent_id = item['parent_res_id'] - rating = item['rating'] - if rating > RATING_LIMIT_OK: - grades_per_parent[parent_id]['great'] += item['__count'] - elif rating > RATING_LIMIT_MIN: - grades_per_parent[parent_id]['okay'] += item['__count'] - else: - grades_per_parent[parent_id]['bad'] += item['__count'] - rating_scores_per_parent[parent_id] += rating * item['__count'] + grade = rating_data._rating_to_grade(item['rating']) + grades_per_parent[parent_id][grade] += item['__count'] + rating_scores_per_parent[parent_id] += item['rating'] * item['__count'] # compute percentage per parent for record in self: @@ -66,9 +58,9 @@ class RatingParentMixin(models.AbstractModel): record.rating_avg_percentage = record.rating_avg / 5 def _search_rating_avg(self, operator, value): - if operator not in OPERATOR_MAPPING: + if operator not in rating_data.OPERATOR_MAPPING: raise NotImplementedError('This operator %s is not supported in this search method.' % operator) - domain = [('parent_res_model', '=', self._name), ('consumed', '=', True), ('rating', '>=', RATING_LIMIT_MIN)] + domain = [('parent_res_model', '=', self._name), ('consumed', '=', True), ('rating', '>=', rating_data.RATING_LIMIT_MIN)] if self._rating_satisfaction_days: min_date = fields.datetime.now() - timedelta(days=self._rating_satisfaction_days) domain = expression.AND([domain, [('write_date', '>=', fields.Datetime.to_string(min_date))]]) @@ -76,6 +68,6 @@ class RatingParentMixin(models.AbstractModel): parent_res_ids = [ res['parent_res_id'] for res in rating_read_group - if OPERATOR_MAPPING[operator](float_compare(res['rating_avg'], value, 2), 0) + if rating_data.OPERATOR_MAPPING[operator](float_compare(res['rating_avg'], value, 2), 0) ] return [('id', 'in', parent_res_ids)]