[CHG] Temporary values as JSON

[benwillig]

Fixes #126.

@sylvain-garancher With this way, we can add as much temporary values as we want.

[rousseldenis]

LGTM.

@benwillig Just a test for empty_scanner_values() ?

[benwillig]

@rousseldenis Done

[rousseldenis]

👍

[Garamotte]

@benwillig This means that any module that defines properties cannot be installed with queue_job ?
In this case, it's an issue of the queue_job module, IMO.

Also, removing the properties will make our scenarios fail when we'll update the module... :(

Anyway, you should split the tests, at least one per method, instead of a big single method, to be able to see which test fails.

[benwillig]

Having properties on a model is not the problem. The error which is raised on startup (or during test) is caused by the self.ensure_one() defined at the beginning of the method. This is why I'm not sure which module we should fix, as I already said.

I can write a migration script which stores the value of tmp_val (if it's a valid json) in tmp_values['json_tmp_val<nr>'] or tmp_values['tmp_val<nr>']. But I can't create the property json_tmp_val<nr> because of the ensure_one.

[Garamotte]

I understand the issue, but removing the properties will need to adapt scenarios with a simple update of the module.

I made some tests, and I can confirm that any module with properties will crash if they contain self.ensure_one(). And ensuring we have a single record is better when we call a property. This is due to Odoo's models desin (because self can be nothing, a single record, or a collection of records).

Even if it's a bit ugly, what di you think about adding a warning in the properties, and remove them for the next version ?

    @property
    @api.multi
    def json_tmp_val1(self):
        if not self:
            logger.warning('DEPRECATED: This attribute will be removed in the next version.')
            return

        self.ensure_one()
        return json.loads(self.tmp_val1 or 'null')

IMO, the best solution would be to find a way to list methods without calling the properties in queue_job, to avoid later conflicts with any module that declares properties, but I don't know the inspect module enough to change that.

[benwillig]

I thought about this solution but as you said, it's a bit ugly. But I think it's the only workaround at the moment. I will change this PR to readd thoses properties with the warning.

[Garamotte]

@benwillig I finally found how to fix queue_job:

diff --git queue_job/models/base.py queue_job/models/base.py
index 5ded5c66ebf6..4ab252281fc5 100644
--- queue_job/models/base.py
+++ queue_job/models/base.py
@@ -23,7 +23,7 @@ def _register_hook(self):
             if attr_name == '_cache':
                 continue
             try:
-                attr = getattr(self, attr_name)
+                attr = getattr(self.__class__, attr_name)
             except AttributeError:
                 continue
             if inspect.ismethod(attr) and getattr(attr, 'delayable', None):

The issue was caused by using a recordset (eg. an instance of the class) instead of the class itself.
By doing this, properties are evaluated, instead of being returned directly.

Uing a Serialized field is a great idea, and I'll hapilly approve the PR, but this is not related to the bug anymore :)

[benwillig]

Sorry I didn't have time to readd properties. The code you gave comes from v11, so we need to backport thoses commits to 10.0, then I will be able to add your properties without the warning.

[Garamotte]

I'm currently creating PRs on queue_job for both versions ;)

Edit: OCA/queue#49 (10.0) and OCA/queue#50 (11.0)

[benwillig]

Updated my PR. Thanks again @sylvain-garancher

[Garamotte]

Thanks.
If I'm not wrong, there is no test for the default argument of get_tmp_value.

Also, could you split the test in several smaller tests ?
This allows to have a better overview of tests when there are fails.

[benwillig]

Spllited and added a test.

[rousseldenis]

LGTM

[LoisRForgeFlow]

LGTM