itpp-labs/sync-addons

sync: multi context

yelizariev opened this issue · 6 comments

Sync Studio can be used to integrate two external system. If we already have integration, say odoo-telegram and odoo-github, we may create integration telegram-github. To do it, we need to combine functions from evaluation context of both existing integrations. Currently, it's not possible to do just via UI and we have to create new eval context _eval_context_telegram_telegram_githtub, that will just call _eval_context_telegram_telegram` and _eval_context_telegram_github`.

To resolve this obstacle, we must allow selecting several evaluation contexts for the project.

eval_context field should be replaced wtih Many2many field to new model sync.project.context (fields name, display_name, description)

Field sync.project.context::description is computed in the same way as current eval_context_description, i.e. by reading method doc strings

eval_context = fields.Selection([], string="Evaluation context")
eval_context_description = fields.Text(compute="_compute_eval_context_description")

def _compute_eval_context_description(self):
for r in self:
if not r.eval_context:
r.eval_context_description = ""
continue
method = getattr(self, EVAL_CONTEXT_PREFIX + r.eval_context)
r.eval_context_description = method.__doc__

new version eval_context_description is a computed field, that combines descriptions of selected context records

The solution must include migration scripts to safely upgrade existing deployments

I'm planning to do a prototype.
One thing to consider is the possibility that one evaluation context may unexpectedly override another context's attribute.
Though, it might actually be an inheritance feature if we add a sequence to the evaluation context list. :)
It probably won't happen if developers use unique attributes, or even better, wrap all the attributes in an AttrDict.
But it might be helpful to check for such unwanted cases when choosing evaluation contexts.

If we change the field signature of eval_context from Selection to Many2many, then Odoo will immediately throw this error if some modules are still using eval_context = fields.Selection(selection_add=[...]).

This particular error is coming from the sync module's sync_project_demo.py, but at least in our system, we have multiple modules acting as execution contexts.

image

AssertionError: sync.project.eval_context: selection_add=[('odoo2odoo', 'Odoo2odoo'), ('telegram_demo', 'Telegram (Demo)'), ('trello_github', 'Trello & Github')] on non-list selection None

Since that's in the code, it's not something that a migration script can fix. I recommend creating a new field called eval_contexts that loads eval_context values for compatibility and deprecating it over a few versions.

The migration script will just copy existing eval_context values into the new eval_contexts field.
However, since a module containing selection_add can still be installed even after the upgrade, we need a hook on ir.model.fields.selection's _reflect_selection to detect new selection_add values and create sync.project.context records accordingly.

We can announce to developers to stop using eval_context and eventually deprecate it.

@yelizariev Shall evaluation context methods be defined on the sync.project.context model instead as well?
To avoid a breaking change, _get_eval_context can look for a match on both sync.project (self) and sync.project.context. If a match is made via sync.project, we can log a deprecation notice.

Here's my prototype with the migration script.

The following preliminary tests were done on a fresh database, but I'll be testing it in our own local database with existing projects.

Test project:
image

Test run:
image

In case your decision is to introduce breaking changes, please feel free to modify the code. The current version seems to satisfy our requirements, but I'm still thinking whether execution context methods should be on sync.project or sync.project.context.

Please note that since parameters and secrets are shared among multiple execution contexts, it might be necessary to distinguish them with prefixes.

@themreza
Thank you for your contribution. I think to add new API on v15, which I'll port soon. This way we don't break existing deployments/modules with such brave change.