From 29a1e80a2a75b22c97b45e7d0bde2da6c5803d75 Mon Sep 17 00:00:00 2001 From: Guewen Baconnier Date: Fri, 18 Sep 2015 12:46:00 +0200 Subject: [PATCH] Show the current or the old value according the change's state The previous implementation had an issue which was that when a revision was created, the partner's value was copied to the revision change and so if the partner was edited, the revision change could show an outdated value. What we want to ensure is to show the actual value when the change is pending and show the old value when the change is done/canceled. --- partner_revision/models/res_partner.py | 7 +- .../models/res_partner_revision.py | 221 ++++++++++++------ partner_revision/tests/__init__.py | 1 + partner_revision/tests/common.py | 21 +- .../tests/test_revision_field_type.py | 7 +- partner_revision/tests/test_revision_flow.py | 2 + .../tests/test_revision_origin.py | 128 ++++++++++ .../views/res_partner_revision_views.xml | 18 +- 8 files changed, 310 insertions(+), 95 deletions(-) create mode 100644 partner_revision/tests/test_revision_origin.py diff --git a/partner_revision/models/res_partner.py b/partner_revision/models/res_partner.py index d7c06d8e2..e428fae5f 100644 --- a/partner_revision/models/res_partner.py +++ b/partner_revision/models/res_partner.py @@ -19,12 +19,17 @@ # # -from openerp import models, api +from openerp import models, fields, api class ResPartner(models.Model): _inherit = 'res.partner' + revision_ids = fields.One2many(comodel_name='res.partner.revision', + inverse_name='partner_id', + string='Revisions', + readonly=True) + @api.multi def write(self, values): if self.env.context.get('__no_revision'): diff --git a/partner_revision/models/res_partner_revision.py b/partner_revision/models/res_partner_revision.py index a012315ea..eb99ae59c 100644 --- a/partner_revision/models/res_partner_revision.py +++ b/partner_revision/models/res_partner_revision.py @@ -26,6 +26,10 @@ from operator import attrgetter from openerp import models, fields, api, exceptions, _ from openerp.osv.orm import setup_modifiers +# sentinel object to be sure that no empty value was passed to +# ResPartnerRevisionChange._value_for_revision +_NO_VALUE = object() + class ResPartnerRevision(models.Model): _name = 'res.partner.revision' @@ -122,6 +126,32 @@ class ResPartnerRevision(models.Model): class ResPartnerRevisionChange(models.Model): + """ Store the change of one field for one revision on one partner + + This model is composed of 3 sets of fields: + + * 'origin' + * 'old' + * 'new' + + The 'new' fields contain the value that needs to be validated. + The 'old' field copies the actual value of the partner when the + change is either applied either canceled. This field is used as a storage + place but never shown by itself. + The 'origin' fields is a related field towards the actual values of + the partner until the change is either applied either canceled, past + that it shows the 'old' value. + The reason behind this is that the values may change on a partner between + the moment when the revision is created and when it is applied. + + On the views, we show the origin fields which represent the actual + partner values or the old values and we show the new fields. + + The 'origin' and 'new_value_display' are displayed on + the tree view where we need a unique of field, the other fields are + displayed on the form view so we benefit from their widgets. + + """ _name = 'res.partner.revision.change' _description = 'Partner Revision Change' _rec_name = 'field_id' @@ -139,8 +169,8 @@ class ResPartnerRevisionChange(models.Model): string='Field Type', readonly=True) - current_value_display = fields.Char( - string='Current', + origin_value_display = fields.Char( + string='Previous', compute='_compute_value_display', ) new_value_display = fields.Char( @@ -148,24 +178,57 @@ class ResPartnerRevisionChange(models.Model): compute='_compute_value_display', ) - current_value_char = fields.Char(string='Current', - readonly=True) - current_value_date = fields.Date(string='Current', - readonly=True) - current_value_datetime = fields.Datetime(string='Current', - readonly=True) - current_value_float = fields.Float(string='Current', + # Fields showing the origin partner's value or the 'old' value if + # the change is applied or canceled. + origin_value_char = fields.Char(compute='_compute_origin_values', + string='Previous', + readonly=True) + origin_value_date = fields.Date(compute='_compute_origin_values', + string='Previous', + readonly=True) + origin_value_datetime = fields.Datetime(compute='_compute_origin_values', + string='Previous', + readonly=True) + origin_value_float = fields.Float(compute='_compute_origin_values', + string='Previous', + readonly=True) + origin_value_integer = fields.Integer(compute='_compute_origin_values', + string='Previous', + readonly=True) + origin_value_text = fields.Text(compute='_compute_origin_values', + string='Previous', + readonly=True) + origin_value_boolean = fields.Boolean(compute='_compute_origin_values', + string='Previous', + readonly=True) + origin_value_reference = fields.Reference( + compute='_compute_origin_values', + string='Previous', + selection='_reference_models', + readonly=True, + ) + + # Fields storing the previous partner's values (saved when the + # revision is applied) + old_value_char = fields.Char(string='Old', + readonly=True) + old_value_date = fields.Date(string='Old', + readonly=True) + old_value_datetime = fields.Datetime(string='Old', + readonly=True) + old_value_float = fields.Float(string='Old', + readonly=True) + old_value_integer = fields.Integer(string='Old', readonly=True) - current_value_integer = fields.Integer(string='Current', - readonly=True) - current_value_text = fields.Text(string='Current', - readonly=True) - current_value_boolean = fields.Boolean(string='Current', + old_value_text = fields.Text(string='Old', + readonly=True) + old_value_boolean = fields.Boolean(string='Old', + readonly=True) + old_value_reference = fields.Reference(string='Old', + selection='_reference_models', readonly=True) - current_value_reference = fields.Reference(string='Current', - selection='_reference_models', - readonly=True) + # Fields storing the value applied on the partner new_value_char = fields.Char(string='New', readonly=True) new_value_date = fields.Date(string='New', @@ -214,51 +277,50 @@ class ResPartnerRevisionChange(models.Model): for suffix, ftypes in _suffix_to_types.iteritems() for ftype in ftypes} - _current_value_fields = ['current_value_%s' % suffix - for suffix in _suffix_to_types] + _origin_value_fields = ['origin_value_%s' % suffix + for suffix in _suffix_to_types] + _old_value_fields = ['old_value_%s' % suffix + for suffix in _suffix_to_types] _new_value_fields = ['new_value_%s' % suffix for suffix in _suffix_to_types] - _value_fields = _current_value_fields + _new_value_fields + _value_fields = (_origin_value_fields + + _old_value_fields + + _new_value_fields) + + @api.one + @api.depends('revision_id.partner_id.*') + def _compute_origin_values(self): + field_name = self.get_field_for_type(self.field_id, 'origin') + if self.state == 'draft': + value = self.revision_id.partner_id[self.field_id.name] + else: + old_field = self.get_field_for_type(self.field_id, 'old') + value = self[old_field] + setattr(self, field_name, value) @api.one @api.depends(lambda self: self._value_fields) def _compute_value_display(self): - for prefix in ('current', 'new'): + for prefix in ('origin', 'new'): value = getattr(self, 'get_%s_value' % prefix)() if self.field_id.ttype == 'many2one' and value: value = value.display_name setattr(self, '%s_value_display' % prefix, value) @api.model - def create(self, vals): - vals = vals.copy() - field = self.env['ir.model.fields'].browse(vals.get('field_id')) - if 'current_value' in vals: - current_value = vals.pop('current_value') - if field: - current_field_name = self.get_field_for_type(field, 'current') - vals[current_field_name] = current_value - if 'new_value' in vals: - new_value = vals.pop('new_value') - if field: - new_field_name = self.get_field_for_type(field, 'new') - vals[new_field_name] = new_value - return super(ResPartnerRevisionChange, self).create(vals) - - @api.model - def get_field_for_type(self, field, current_or_new): - assert current_or_new in ('new', 'current') + def get_field_for_type(self, field, prefix): + assert prefix in ('origin', 'old', 'new') field_type = self._type_to_suffix.get(field.ttype) if not field_type: raise NotImplementedError( 'field type %s is not supported' % field_type ) - return '%s_value_%s' % (current_or_new, field_type) + return '%s_value_%s' % (prefix, field_type) @api.multi - def get_current_value(self): + def get_origin_value(self): self.ensure_one() - field_name = self.get_field_for_type(self.field_id, 'current') + field_name = self.get_field_for_type(self.field_id, 'origin') return self[field_name] @api.multi @@ -267,6 +329,18 @@ class ResPartnerRevisionChange(models.Model): field_name = self.get_field_for_type(self.field_id, 'new') return self[field_name] + @api.multi + def set_old_value(self): + """ Copy the value of the partner to the 'old' field """ + for change in self: + # copy the existing partner's value for the history + old_value_for_write = self._value_for_revision( + change.revision_id.partner_id, + change.field_id.name + ) + old_field_name = self.get_field_for_type(change.field_id, 'old') + change.write({old_field_name: old_value_for_write}) + @api.multi def apply(self): """ Apply the change on the revision's partner @@ -283,10 +357,14 @@ class ResPartnerRevisionChange(models.Model): if change.state in ('cancel', 'done'): continue + field = change.field_id value_for_write = change._convert_value_for_write( change.get_new_value() ) - values[change.field_id.name] = value_for_write + values[field.name] = value_for_write + + change.set_old_value() + changes_ok |= change if not values: @@ -307,7 +385,8 @@ class ResPartnerRevisionChange(models.Model): 'this one.') ) - partner.write(values) + partner.with_context(__no_revision=True).write(values) + changes_ok.write({'state': 'done'}) @api.multi @@ -317,6 +396,7 @@ class ResPartnerRevisionChange(models.Model): raise exceptions.Warning( _('This change has already be applied.') ) + self.set_old_value() self.write({'state': 'cancel'}) @api.model @@ -331,8 +411,18 @@ class ResPartnerRevisionChange(models.Model): return model_field_def.convert_to_write(value) @api.model - def _convert_value_for_revision(self, record, field, value): - field_def = record._fields[field] + def _value_for_revision(self, record, field_name, value=_NO_VALUE): + """ Return a value from the record ready to write in a revision field + + :param record: modified record + :param field_name: name of the modified field + :param value: if no value is given, it is read from the record + """ + field_def = record._fields[field_name] + if value is _NO_VALUE: + # when the value is read from the record, we need to prepare + # it for the write (e.g. extract .id from a many2one record) + value = field_def.convert_to_write(record[field_name]) if field_def.type == 'many2one': # store as 'reference' comodel = field_def.comodel_name @@ -341,7 +431,7 @@ class ResPartnerRevisionChange(models.Model): return value @api.multi - def _prepare_revision_change(self, record, rule, field, value): + def _prepare_revision_change(self, record, rule, field_name, value): """ Prepare data for a revision change It returns a dict of the values to write on the revision change @@ -350,28 +440,15 @@ class ResPartnerRevisionChange(models.Model): :returns: dict of values, boolean """ - field_def = record._fields[field] - # get a ready to write value for the type of the field, - # for instance takes '.id' from a many2one's record (the - # new value is already a value as expected for the - # write) - current_value = field_def.convert_to_write(record[field]) - # get values ready to write as expected by the revision - # (for instance, a many2one is written in a reference - # field) - current_value = self._convert_value_for_revision(record, field, - current_value) - new_value = self._convert_value_for_revision(record, field, value) + new_field_name = self.get_field_for_type(rule.field_id, 'new') + new_value = self._value_for_revision(record, field_name, value=value) change = { - 'current_value': current_value, - 'new_value': new_value, + new_field_name: new_value, 'field_id': rule.field_id.id, } pop_value = False - if not self.env.context.get('__revision_rules'): - # by default always write on partner - change['state'] = 'done' - elif rule.action == 'auto': + if (not self.env.context.get('__revision_rules') or + rule.action == 'auto'): change['state'] = 'done' elif rule.action == 'validate': change['state'] = 'draft' @@ -379,6 +456,18 @@ class ResPartnerRevisionChange(models.Model): elif rule.action == 'never': change['state'] = 'cancel' pop_value = True # change never applied + + if change['state'] in ('cancel', 'done'): + # Normally the 'old' value is set when we use the 'apply' + # button, but since we short circuit the 'apply', we + # directly set the 'old' value here + old_field_name = self.get_field_for_type(rule.field_id, 'old') + # get values ready to write as expected by the revision + # (for instance, a many2one is written in a reference + # field) + origin_value = self._value_for_revision(record, field_name) + change[old_field_name] = origin_value + return change, pop_value def fields_view_get(self, *args, **kwargs): @@ -388,7 +477,7 @@ class ResPartnerRevisionChange(models.Model): return doc = etree.XML(result['arch']) for suffix, ftypes in self._suffix_to_types.iteritems(): - for prefix in ('current', 'new'): + for prefix in ('origin', 'old', 'new'): field_name = '%s_value_%s' % (prefix, suffix) field_nodes = doc.xpath("//field[@name='%s']" % field_name) for node in field_nodes: diff --git a/partner_revision/tests/__init__.py b/partner_revision/tests/__init__.py index b7aa994f5..ea944348b 100644 --- a/partner_revision/tests/__init__.py +++ b/partner_revision/tests/__init__.py @@ -2,3 +2,4 @@ from . import test_revision_flow from . import test_revision_field_type +from . import test_revision_origin diff --git a/partner_revision/tests/common.py b/partner_revision/tests/common.py index be8a4d4c2..7e9f90cbe 100644 --- a/partner_revision/tests/common.py +++ b/partner_revision/tests/common.py @@ -28,7 +28,7 @@ class RevisionMixin(object): The partner should have no prior revision than the one created in the test (so it has exactly 1 revision). - The expected changes are tuples with (field, current_value, + The expected changes are tuples with (field, origin_value, new_value, state) :param partner: record of partner having a revision @@ -45,7 +45,7 @@ class RevisionMixin(object): for expected_change in expected_changes: for change in changes: if (change.field_id, - change.get_current_value(), + change.get_origin_value(), change.get_new_value(), change.state) == expected_change: changes -= change @@ -53,15 +53,15 @@ class RevisionMixin(object): else: missing.append(expected_change) message = u'' - for field, current_value, new_value, state in missing: - message += ("- field: '%s', current_value: '%s', " + for field, origin_value, new_value, state in missing: + message += ("- field: '%s', origin_value: '%s', " "new_value: '%s', state: '%s'\n" % - (field.name, current_value, new_value, state)) + (field.name, origin_value, new_value, state)) for change in changes: - message += ("+ field: '%s', current_value: '%s', " + message += ("+ field: '%s', origin_value: '%s', " "new_value: '%s', state: '%s'\n" % (change.field_id.name, - change.get_current_value(), + change.get_origin_value(), change.get_new_value(), change.state)) if message: @@ -76,17 +76,12 @@ class RevisionMixin(object): """ RevisionChange = self.env['res.partner.revision.change'] get_field = RevisionChange.get_field_for_type - convert = RevisionChange._convert_value_for_revision change_values = [] for field, value, state in changes: - field_def = self.env['res.partner']._fields[field.name] - current_value = field_def.convert_to_write(partner[field.name]) - current_value = convert(partner, field.name, current_value) change = { 'field_id': field.id, # write in the field of the appropriate type for the - # current field (char, many2one, ...) - get_field(field, 'current'): current_value, + # origin field (char, many2one, ...) get_field(field, 'new'): value, 'state': state, } diff --git a/partner_revision/tests/test_revision_field_type.py b/partner_revision/tests/test_revision_field_type.py index bda7e9d69..f25f7488b 100644 --- a/partner_revision/tests/test_revision_field_type.py +++ b/partner_revision/tests/test_revision_field_type.py @@ -24,11 +24,7 @@ from .common import RevisionMixin class TestRevisionFieldType(RevisionMixin, common.TransactionCase): - """ Check how revision are generated and applied based on the rules. - - We do not really care about the types of the fields in this test suite, - but we have to ensure that the general revision flows work as expected. - """ + """ Check that revision changes are stored expectingly to their types """ def _setup_rules(self): RevisionFieldRule = self.env['revision.field.rule'] @@ -256,7 +252,6 @@ class TestRevisionFieldType(RevisionMixin, common.TransactionCase): def test_apply_many2one(self): """ Apply a change on a Many2one field """ - self.s = True self.partner.with_context(__no_revision=True).write({ self.field_many2one.name: self.env.ref('base.fr').id, diff --git a/partner_revision/tests/test_revision_flow.py b/partner_revision/tests/test_revision_flow.py index 76b8ec57a..23eb87d62 100644 --- a/partner_revision/tests/test_revision_flow.py +++ b/partner_revision/tests/test_revision_flow.py @@ -39,6 +39,8 @@ class TestRevisionFlow(RevisionMixin, common.TransactionCase): * apply a revision change writes the value on the partner * apply a whole revision writes all the changes' values on the partner * changes in state 'cancel' or 'done' do not write on the partner + * when all the changes are either 'cancel' or 'done', the revision + becomes 'done' """ def _setup_rules(self): diff --git a/partner_revision/tests/test_revision_origin.py b/partner_revision/tests/test_revision_origin.py new file mode 100644 index 000000000..05bcece4d --- /dev/null +++ b/partner_revision/tests/test_revision_origin.py @@ -0,0 +1,128 @@ +# -*- coding: utf-8 -*- +# +# +# Authors: Guewen Baconnier +# Copyright 2015 Camptocamp SA +# +# This program is free software: you can redistribute it and/or modify +# it under the terms of the GNU Affero General Public License as +# published by the Free Software Foundation, either version 3 of the +# License, or (at your option) any later version. +# +# This program is distributed in the hope that it will be useful, +# but WITHOUT ANY WARRANTY; without even the implied warranty of +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +# GNU Affero General Public License for more details. +# +# You should have received a copy of the GNU Affero General Public License +# along with this program. If not, see . +# +# + +from openerp.tests import common +from .common import RevisionMixin + + +class TestRevisionOrigin(RevisionMixin, common.TransactionCase): + """ Check that origin - old fields are stored as expected. + + 'origin' fields dynamically read fields from the partner when the state + of the change is 'draft'. Once a change becomes 'done' or 'cancel', the + 'old' field copies the value from the partner and then the 'origin' field + displays the 'old' value. + """ + + def _setup_rules(self): + RevisionFieldRule = self.env['revision.field.rule'] + partner_model_id = self.env.ref('base.model_res_partner').id + self.field_name = self.env.ref('base.field_res_partner_name') + RevisionFieldRule.create({ + 'model_id': partner_model_id, + 'field_id': self.field_name.id, + 'action': 'validate', + }) + + def setUp(self): + super(TestRevisionOrigin, self).setUp() + self._setup_rules() + self.partner = self.env['res.partner'].create({ + 'name': 'X', + }) + + def test_origin_value_of_change_with_apply(self): + """ Origin field is read from the parter or 'old' - with apply + + According to the state of the change. + """ + self.partner.with_context(__revision_rules=True).write({ + 'name': 'Y', + }) + revision = self.partner.revision_ids + change = revision.change_ids + self.assertEqual(self.partner.name, 'X') + self.assertEqual(change.origin_value_char, 'X') + self.assertEqual(change.origin_value_display, 'X') + self.partner.write({'name': 'A'}) + self.assertEqual(change.origin_value_char, 'A') + self.assertEqual(change.origin_value_display, 'A') + change.apply() + self.assertEqual(change.origin_value_char, 'A') + self.assertEqual(change.origin_value_display, 'A') + self.partner.write({'name': 'B'}) + self.assertEqual(change.origin_value_char, 'A') + self.assertEqual(change.origin_value_display, 'A') + + def test_origin_value_of_change_with_cancel(self): + """ Origin field is read from the parter or 'old' - with cancel + + According to the state of the change. + """ + self.partner.with_context(__revision_rules=True).write({ + 'name': 'Y', + }) + revision = self.partner.revision_ids + change = revision.change_ids + self.assertEqual(self.partner.name, 'X') + self.assertEqual(change.origin_value_char, 'X') + self.assertEqual(change.origin_value_display, 'X') + self.partner.write({'name': 'A'}) + self.assertEqual(change.origin_value_char, 'A') + self.assertEqual(change.origin_value_display, 'A') + change.cancel() + self.assertEqual(change.origin_value_char, 'A') + self.assertEqual(change.origin_value_display, 'A') + self.partner.write({'name': 'B'}) + self.assertEqual(change.origin_value_char, 'A') + self.assertEqual(change.origin_value_display, 'A') + + def test_old_field_of_change_with_apply(self): + """ Old field is stored when the change is applied """ + self.partner.with_context(__revision_rules=True).write({ + 'name': 'Y', + }) + revision = self.partner.revision_ids + change = revision.change_ids + self.assertEqual(self.partner.name, 'X') + self.assertFalse(change.old_value_char) + self.partner.write({'name': 'A'}) + self.assertFalse(change.old_value_char) + change.apply() + self.assertEqual(change.old_value_char, 'A') + self.partner.write({'name': 'B'}) + self.assertEqual(change.old_value_char, 'A') + + def test_old_field_of_change_with_cancel(self): + """ Old field is stored when the change is canceled """ + self.partner.with_context(__revision_rules=True).write({ + 'name': 'Y', + }) + revision = self.partner.revision_ids + change = revision.change_ids + self.assertEqual(self.partner.name, 'X') + self.assertFalse(change.old_value_char) + self.partner.write({'name': 'A'}) + self.assertFalse(change.old_value_char) + change.cancel() + self.assertEqual(change.old_value_char, 'A') + self.partner.write({'name': 'B'}) + self.assertEqual(change.old_value_char, 'A') diff --git a/partner_revision/views/res_partner_revision_views.xml b/partner_revision/views/res_partner_revision_views.xml index 0f57a70cc..3e0e7d247 100644 --- a/partner_revision/views/res_partner_revision_views.xml +++ b/partner_revision/views/res_partner_revision_views.xml @@ -41,7 +41,7 @@ - + @@ -87,28 +87,28 @@ - + - + - + - + - + - + - + - +