From 8a80efd7cd9f564338c6ca6c72e49f6890190b4c Mon Sep 17 00:00:00 2001 From: Jeremy Kersten Date: Thu, 11 Jul 2019 17:00:30 +0000 Subject: [PATCH] [FIX] gamification: perf on write karma_min on gamification.karma.rank MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This commits avoid to call recompute when you write on the rank without touch the karma_min value. On write on karma_min, we have 2 cases: - Rank order are the same, just need to recompute the rank for the user that could be impacted. Eg. if karma_min changes from 45 to 50, we need to recheck for all user between 45 and 50 their new rank. - Rank order are not the same, we need to recompute for all users. (could be still improved if needed) When you recompute the rank on a list of users, depending of the number of users (bulk if more than 3 * number of rank) we check for each user what is the new rank, or for each rank whats is the new users. task-internal No way to edit a rank on the prod server. Lock sql -> crash server closes odoo/odoo#34835 Signed-off-by: Jérémy Kersten (jke) --- .../models/gamification_karma_rank.py | 15 +- addons/gamification/models/res_users.py | 84 +++++++++++- addons/gamification/tests/__init__.py | 1 + .../gamification/tests/test_recompute_rank.py | 128 ++++++++++++++++++ 4 files changed, 222 insertions(+), 6 deletions(-) create mode 100644 addons/gamification/tests/test_recompute_rank.py diff --git a/addons/gamification/models/gamification_karma_rank.py b/addons/gamification/models/gamification_karma_rank.py index 78b8e40ac50..1d930fefa4d 100644 --- a/addons/gamification/models/gamification_karma_rank.py +++ b/addons/gamification/models/gamification_karma_rank.py @@ -47,8 +47,19 @@ class KarmaRank(models.Model): @api.multi def write(self, vals): + if 'karma_min' in vals: + previous_ranks = self.env['gamification.karma.rank'].search([], order="karma_min DESC").ids + low = min(vals['karma_min'], self.karma_min) + high = max(vals['karma_min'], self.karma_min) + tools.image_resize_images(vals) res = super(KarmaRank, self).write(vals) - users = self.env['res.users'].sudo().search([('karma', '>', 0)]) - users._recompute_rank() + + if 'karma_min' in vals: + after_ranks = self.env['gamification.karma.rank'].search([], order="karma_min DESC").ids + if previous_ranks != after_ranks: + users = self.env['res.users'].sudo().search([('karma', '>', 0)]) + else: + users = self.env['res.users'].sudo().search([('karma', '>=', low), ('karma', '<=', high)]) + users._recompute_rank() return res diff --git a/addons/gamification/models/res_users.py b/addons/gamification/models/res_users.py index a0ca65ced45..4f9319a4001 100644 --- a/addons/gamification/models/res_users.py +++ b/addons/gamification/models/res_users.py @@ -7,9 +7,11 @@ import logging _logger = logging.getLogger(__name__) + class Users(models.Model): _inherit = 'res.users' + karma = fields.Integer('Karma', default=0) karma_position = fields.Integer('Karma position', compute="_compute_karma_position", store=False) badge_ids = fields.One2many('gamification.badge.user', 'user_id', string='Badges', copy=False) @@ -69,18 +71,32 @@ class Users(models.Model): return True def _rank_changed(self): - if self.rank_id.karma_min > 0: - template = self.env.ref('gamification.mail_template_data_new_rank_reached', raise_if_not_found=False) - if template: - template.send_mail(self.id, force_send=True, notif_layout='mail.mail_notification_light') + """ + Method that can be called on a batch of users with the same new rank + """ + template = self.env.ref('gamification.mail_template_data_new_rank_reached', raise_if_not_found=False) + if template: + for u in self: + if u.rank_id.karma_min > 0: + template.send_mail(u.id, force_send=len(self) == 1, notif_layout='mail.mail_notification_light') def _recompute_rank(self): """ The caller should filter the users on karma > 0 before calling this method to avoid looping on every single users + + Compute rank of each user by user. + For each user, check the rank of this user """ + ranks = [{'rank': rank, 'karma_min': rank.karma_min} for rank in self.env['gamification.karma.rank'].search([], order="karma_min DESC")] + + # 3 is the number of search/requests used by rank in _recompute_rank_bulk() + if len(self) > len(ranks) * 3: + self._recompute_rank_bulk() + return + for user in self: old_rank = user.rank_id if user.karma == 0 and ranks: @@ -96,6 +112,66 @@ class Users(models.Model): if old_rank != user.rank_id: user._rank_changed() + def _recompute_rank_bulk(self): + """ + Compute rank of each user by rank. + For each rank, check which users need to be ranked + + """ + ranks = [{'rank': rank, 'karma_min': rank.karma_min} for rank in + self.env['gamification.karma.rank'].search([], order="karma_min DESC")] + + users_todo = self + + next_rank_id = False + # wtf, next_rank_id should be a related on rank_id.next_rank_id and life might get easier. + # And we only need to recompute next_rank_id on write with min_karma or in the create on rank model. + for r in ranks: + rank_id = r['rank'].id + dom = [ + ('karma', '>=', r['karma_min']), + ('id', 'in', users_todo.ids), + '|', # noqa + '|', ('rank_id', '!=', rank_id), ('rank_id', '=', False), + '|', ('next_rank_id', '!=', next_rank_id), ('next_rank_id', '=', False if next_rank_id else -1), + ] + users = self.env['res.users'].search(dom) + if users: + users_to_notify = self.env['res.users'].search([ + ('karma', '>=', r['karma_min']), + '|', ('rank_id', '!=', rank_id), ('rank_id', '=', False), + ('id', 'in', users.ids), + ]) + users.write({ + 'rank_id': rank_id, + 'next_rank_id': next_rank_id, + }) + users_to_notify._rank_changed() + users_todo -= users + + nothing_to_do_users = self.env['res.users'].search([ + ('karma', '>=', r['karma_min']), + '|', ('rank_id', '=', rank_id), ('next_rank_id', '=', next_rank_id), + ('id', 'in', users_todo.ids), + ]) + users_todo -= nothing_to_do_users + next_rank_id = r['rank'].id + + if ranks: + lower_rank = ranks[-1]['rank'] + users = self.env['res.users'].search([ + ('karma', '>=', 0), + ('karma', '<', lower_rank.karma_min), + '|', ('rank_id', '!=', False), ('next_rank_id', '!=', lower_rank.id), + ('id', 'in', users_todo.ids), + ]) + if users: + users.write({ + 'rank_id': False, + 'next_rank_id': lower_rank.id, + }) + + def _get_next_rank(self): """ For fresh users with 0 karma that don't have a rank_id and next_rank_id yet this method returns the first karma rank (by karma ascending). This acts as a diff --git a/addons/gamification/tests/__init__.py b/addons/gamification/tests/__init__.py index 0219f6754d0..a6a599bf6c8 100644 --- a/addons/gamification/tests/__init__.py +++ b/addons/gamification/tests/__init__.py @@ -2,3 +2,4 @@ # Part of Odoo. See LICENSE file for full copyright and licensing details. from . import test_challenge +from . import test_recompute_rank diff --git a/addons/gamification/tests/test_recompute_rank.py b/addons/gamification/tests/test_recompute_rank.py new file mode 100644 index 00000000000..c82fb4759fd --- /dev/null +++ b/addons/gamification/tests/test_recompute_rank.py @@ -0,0 +1,128 @@ +# -*- coding: utf-8 -*- +# Part of Odoo. See LICENSE file for full copyright and licensing details. + +from odoo.tests import common +from unittest.mock import patch + + +class TestComputeRankCommon(common.TransactionCase): + + def setUp(self): + super(TestComputeRankCommon, self).setUp() + + def _patched_send_mail(*args, **kwargs): + pass + + self.User = self.env['res.users'].with_context(no_reset_password=True, mail_create_nosubscribe=True) + self.users = self.User + + patch_email = patch('odoo.addons.mail.models.mail_template.MailTemplate.send_mail', _patched_send_mail) + patch_email.start() + for k in range(-5, 1030, 30): + self.users += self.User.create({ + 'name': str(k), + 'login': "test_recompute_rank_%s" % k, + 'karma': k, + }) + + self.env['gamification.karma.rank'].search([]).unlink() + + self.rank_1 = self.env['gamification.karma.rank'].create({ + 'name': 'rank 1', + 'karma_min': 0, + }) + + self.rank_2 = self.env['gamification.karma.rank'].create({ + 'name': 'rank 2', + 'karma_min': 250, + }) + + self.rank_3 = self.env['gamification.karma.rank'].create({ + 'name': 'rank 3', + 'karma_min': 500, + }) + self.rank_4 = self.env['gamification.karma.rank'].create({ + 'name': 'rank 4', + 'karma_min': 1000, + }) + + patch_email.stop() + + +class test_recompute(TestComputeRankCommon): + + def test_00_initial_compute(self): + + self.assertEqual(len(self.users), 35) + + self.assertEqual( + len(self.rank_1.user_ids & self.users), + len([u for u in self.users if u.karma >= self.rank_1.karma_min and u.karma < self.rank_2.karma_min]) + ) + self.assertEqual( + len(self.rank_2.user_ids & self.users), + len([u for u in self.users if u.karma >= self.rank_2.karma_min and u.karma < self.rank_3.karma_min]) + ) + self.assertEqual( + len(self.rank_3.user_ids & self.users), + len([u for u in self.users if u.karma >= self.rank_3.karma_min and u.karma < self.rank_4.karma_min]) + ) + self.assertEqual( + len(self.rank_4.user_ids & self.users), + len([u for u in self.users if u.karma >= self.rank_4.karma_min]) + ) + + def test_01_switch_rank(self): + + self.assertEqual(len(self.users), 35) + + self.rank_3.karma_min = 100 + # rank_1 -> rank_3 -> rank_2 -> rank_4 + + self.assertEqual( + len(self.rank_1.user_ids & self.users), + len([u for u in self.users if u.karma >= self.rank_1.karma_min and u.karma < self.rank_3.karma_min]) + ) + self.assertEqual( + len(self.rank_3.user_ids & self.users), + len([u for u in self.users if u.karma >= self.rank_3.karma_min and u.karma < self.rank_2.karma_min]) + ) + self.assertEqual( + len(self.rank_2.user_ids & self.users), + len([u for u in self.users if u.karma >= self.rank_2.karma_min and u.karma < self.rank_4.karma_min]) + ) + self.assertEqual( + len(self.rank_4.user_ids & self.users), + len([u for u in self.users if u.karma >= self.rank_4.karma_min]) + ) + + def test_02_update_rank_without_switch(self): + number_of_users = False + + def _patched_recompute_rank(_self, *args, **kwargs): + nonlocal number_of_users + number_of_users = len(_self & self.users) + + patch_bulk = patch('odoo.addons.gamification.models.res_users.Users._recompute_rank', _patched_recompute_rank) + patch_bulk.start() + self.rank_3.karma_min = 700 + self.assertEqual(number_of_users, 7, "Should just recompute for the 7 users between 500 and 700") + patch_bulk.stop() + + def test_03_test_bulk_call(self): + self.assertEqual(len(self.users), 35) + + def _patched_check_in_bulk(*args, **kwargs): + raise + + patch_bulk = patch('odoo.addons.gamification.models.res_users.Users._recompute_rank_bulk', _patched_check_in_bulk) + patch_bulk.start() + + # call on 5 users should not trigger the bulk function + self.users[0:5]._recompute_rank() + + # call on 50 users should trigger the bulk function + with self.assertRaises(Exception): + self.users[0:50]._recompute_rank() + + patch_bulk.stop()