-
-
Notifications
You must be signed in to change notification settings - Fork 61
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
connector_search_engine: improve state update wiz #189
base: 16.0
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice feature, thanks a lot.
LGTM on the code side
bcfc380
to
2d391aa
Compare
_jobify_batch_recompute and batch_recompute now accept a binding_ids param to force the records to be computed.
2d391aa
to
81f8e3c
Compare
Hi @lmignon is it good for you ? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@simahawk 2 little comments but LGTM. Thank you for this improvement.
@@ -216,12 +216,14 @@ def _jobify_batch_sync(self, force_export: bool = False) -> None: | |||
description=description, identity_key=identity_exact | |||
).batch_sync(force_export) | |||
|
|||
def _jobify_batch_recompute(self, force_export: bool = False) -> None: | |||
def _jobify_batch_recompute( | |||
self, force_export: bool = False, binding_ids: list | None = None |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
self, force_export: bool = False, binding_ids: list | None = None | |
self, force_export: bool = False, binding_ids: list[int] | None = None |
@@ -256,11 +258,16 @@ def _get_domain_for_recomputing_binding(self, force_export: bool = False) -> lis | |||
states.append("recomputing") | |||
return [("index_id", "=", self.id), ("state", "in", states)] | |||
|
|||
def batch_recompute(self, force_export: bool = False) -> None: | |||
def batch_recompute( | |||
self, force_export: bool = False, binding_ids: list | None = None |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
self, force_export: bool = False, binding_ids: list | None = None | |
self, force_export: bool = False, binding_ids: list[int] | None = None |
@@ -256,11 +258,16 @@ def _get_domain_for_recomputing_binding(self, force_export: bool = False) -> lis | |||
states.append("recomputing") | |||
return [("index_id", "=", self.id), ("state", "in", states)] | |||
|
|||
def batch_recompute(self, force_export: bool = False) -> None: | |||
def batch_recompute( | |||
self, force_export: bool = False, binding_ids: list | None = None |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What do you think about doing another method? (batch_recompute_bindings_ids
?), and factoring out the common part. Less if's is good, in general.
self.ensure_one() | ||
description = _("Prepare a batch recompute of index '%s'") % self.name | ||
self.with_delay( | ||
description=description, identity_key=identity_exact | ||
).batch_recompute(force_export) | ||
).batch_recompute(force_export, binding_ids=binding_ids) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Strictly speaking this job description is now inaccurate because it is now a partial recompute? So maybe this method should be split too?
There hasn't been any activity on this pull request in the past 4 months, so it has been marked as stale and it will be closed automatically if no further activity occurs in the next 30 days. |
Allow to schedule jobs right away instead of waiting for the cron to pass by.