T2715 [MIG] interaction_resume: Migration to 18.0 #2061
Conversation
Summary of ChangesHello @SamBachmann, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly enhances partner communication tracking by migrating and expanding the Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
|
/gemini review |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request migrates the interaction_resume module, which introduces a timeline view of all communications with a partner. The changes include adding the new module with its models, views, wizards, and JS assets. My review focuses on the new interaction_resume module. I've found a couple of critical issues related to background job scheduling that could cause crashes, and a logic issue in determining interaction direction. I've also noted some minor issues in the JavaScript code.
| res.mapped("partner_id").with_delay( | ||
| channel="root.partner_communication", | ||
| priority=100, | ||
| identity_key=self.partner_id._name | ||
| + ".fetch_interactions." | ||
| + str(self.partner_id.id), | ||
| ).fetch_interactions() |
There was a problem hiding this comment.
The identity_key for the with_delay call is constructed incorrectly. In the create method, self is an empty recordset, so self.partner_id is also empty, and self.partner_id.id will raise an error. Additionally, calling with_delay on a recordset with a static identity_key will result in all jobs having the same key, which is likely not the intended behavior.
To fix this, you should iterate over the partners and schedule a job for each with a unique identity_key.
for partner in res.mapped("partner_id"):
partner.with_delay(
channel="root.partner_communication",
priority=100,
identity_key=f"res.partner.fetch_interactions.{partner.id}",
).fetch_interactions()| self.partner_id.with_delay( | ||
| channel="root.partner_communication", | ||
| priority=100, | ||
| identity_key=self.partner_id._name | ||
| + ".fetch_interactions." | ||
| + str(self.partner_id.id), | ||
| ).fetch_interactions() |
There was a problem hiding this comment.
The identity_key for the with_delay call is constructed incorrectly. The message_post method can be called on a recordset of crm.claim records. In that case, self.partner_id would be a recordset of partners, and self.partner_id.id would raise an error.
To fix this, you should iterate over the claims and schedule a job for each partner.
| self.partner_id.with_delay( | |
| channel="root.partner_communication", | |
| priority=100, | |
| identity_key=self.partner_id._name | |
| + ".fetch_interactions." | |
| + str(self.partner_id.id), | |
| ).fetch_interactions() | |
| for claim in self: | |
| if claim.partner_id: | |
| claim.partner_id.with_delay( | |
| channel="root.partner_communication", | |
| priority=100, | |
| identity_key=f"{claim.partner_id._name}.fetch_interactions.{claim.partner_id.id}", | |
| ).fetch_interactions() |
| user = message.author_id.user_ids[:1] or message.create_uid or self.env.user | ||
| if user.share: | ||
| user = self.env.user | ||
| interactions += interactions.with_user(user).create( | ||
| { | ||
| "partner_id": partner_id, | ||
| "date": message.date, | ||
| "communication_type": "Email", | ||
| "subject": message.subject, | ||
| "body": message.body, | ||
| "other_type": "Direct message converted to interaction", | ||
| "direction": "out" if message.author_id.id != partner_id else "in", | ||
| } | ||
| ) |
There was a problem hiding this comment.
The logic to determine the interaction direction is a bit too simplistic. It only checks if the author is the partner for whom the interaction is being logged. This doesn't account for other contacts of the partner or other scenarios. A more robust approach would be to check if the author is one of the partner's related contacts, similar to the logic in crm_request.py.
partner = self.env["res.partner"].browse(partner_id)
partners = partner | partner.other_contact_ids
if partner.email:
partners |= self.env["res.partner"].with_context(
active_test=False
).search([("email", "=", partner.email)])
user = message.author_id.user_ids[:1] or message.create_uid or self.env.user
if user.share:
user = self.env.user
interactions += interactions.with_user(user).create(
{
"partner_id": partner_id,
"date": message.date,
"communication_type": "Email",
"subject": message.subject,
"body": message.body,
"other_type": "Direct message converted to interaction",
"direction": "in" if message.author_id in partners else "out",
}
)| var self = this; | ||
| var res_ids = this.model.get(this.handle).res_ids; | ||
| if (!res_ids.length) { | ||
| console.log("No records to refresh"); |
There was a problem hiding this comment.
This file contains several console.log and console.error statements (e.g., lines 22, 36, 44, 58, 67, 86, 89). These are useful for debugging but should be removed from production code to avoid cluttering the browser console. Please remove all of them.
There was a problem hiding this comment.
Code Review
This pull request introduces a new module interaction_resume to display a timeline of communications with partners. It also includes formatting fixes across several README.rst files. The new module is well-structured and covers various interaction sources like emails, phone calls, and SMS.
My review focuses on the new module. I've found a couple of important issues related to job queuing and data pagination that could lead to incorrect behavior. I've also suggested an improvement in the JavaScript code for better error handling and user experience.
Overall, this is a great addition. Once the identified issues are addressed, the module should be solid.
| def create(self, vals_list): | ||
| res = super().create(vals_list) | ||
| res.mapped("partner_id").with_delay( | ||
| channel="root.partner_communication", | ||
| priority=100, | ||
| identity_key=self.partner_id._name | ||
| + ".fetch_interactions." | ||
| + str(self.partner_id.id), | ||
| ).fetch_interactions() | ||
| return res |
There was a problem hiding this comment.
The identity_key for the delayed job is not generated correctly when creating multiple interactions at once. It uses self.partner_id, but self is an empty recordset in the context of the create method, resulting in a static key like 'res.partner.fetch_interactions.False'. This can cause jobs to be deduplicated incorrectly, leading to missed interaction updates for some partners if they are created in the same transaction.
To fix this, you should iterate over the created records and generate a unique identity_key for each partner.
| def create(self, vals_list): | |
| res = super().create(vals_list) | |
| res.mapped("partner_id").with_delay( | |
| channel="root.partner_communication", | |
| priority=100, | |
| identity_key=self.partner_id._name | |
| + ".fetch_interactions." | |
| + str(self.partner_id.id), | |
| ).fetch_interactions() | |
| return res | |
| def create(self, vals_list): | |
| res = super().create(vals_list) | |
| for partner in res.mapped("partner_id"): | |
| partner.with_delay( | |
| channel="root.partner_communication", | |
| priority=100, | |
| identity_key=f"{partner._name}.fetch_interactions.{partner.id}", | |
| ).fetch_interactions() | |
| return res |
| for model in models: | ||
| self.env[model].fetch_interaction(self, since, until) | ||
| self.last_interaction_fetch_date = fields.Datetime.now() | ||
| self.last_interaction_fetch_page += page |
There was a problem hiding this comment.
The pagination logic for fetching more interactions is incorrect. self.last_interaction_fetch_page += page will cause the page number to grow exponentially, leading to incorrect pagination and skipping of historical data. It should be set to the current page number being fetched to ensure linear pagination.
| self.last_interaction_fetch_page += page | |
| self.last_interaction_fetch_page = page |
| .catch(function () { | ||
| console.log("Error refreshing record"); | ||
| }); |
There was a problem hiding this comment.
The error handling in this method uses console.log, which is not visible to end-users. It's better to use Odoo's notification service to display a user-friendly error message. This improves the user experience significantly.
Please apply this pattern to the _onFetchMore and _onLogInteraction methods as well.
.catch(function (error) {
self.displayNotification({ message: "Error refreshing record", type: "danger" });
console.error("Error refreshing record:", error);
});| .catch(function () { | ||
| console.log("Error fetching more records"); | ||
| }); |
There was a problem hiding this comment.
Similar to the _onRefresh method, the error handling here uses console.log. Please use the notification service to inform the user about the error, and log the error object to the console for debugging purposes.
.catch(function (error) {
self.displayNotification({ message: "Error fetching more records", type: "danger" });
console.error("Error fetching more records:", error);
});| .catch(function (error) { | ||
| console.error("Error executing action:", error); | ||
| }); |
There was a problem hiding this comment.
For consistency and better user experience, please use the notification service to display an error message to the user in case the action fails.
.catch(function (error) {
self.displayNotification({ message: "Error executing action", type: "danger" });
console.error("Error executing action:", error);
});
SamBachmann
force-pushed
the
sBachmann/18.0-interaction_resume
branch
from
216161f to
63290e2
Compare
|
@ecino As the idea is to simply migrate the module and not to totally refactor it, should I address gemini' review issues ? |
SamBachmann
changed the title
[MIG] T2715 interaction_resume manual update of templates [MIG] T2715 interaction_resume manual update of backend logic [MIG] T2715 precommit [MIG] T2715 REVERT changes in other folder
ecino
force-pushed
the
sBachmann/18.0-interaction_resume
branch
from
63290e2 to
96b3733
Compare
|
@SamBachmann I fixed some migration issues that prevented the installation (I forced pushed on the branch, you can fetch and reset your local branch to get the latest state). Now it hase OWL errors when trying to load the view which you can check. The module is adding the "Interaction Resume" button inside the partner view, which displays the history of all exchanged messages. |
This PR migrates the interaction_resume module from Odoo v14 to v18.
Key changes:
I have successfully resolved all migration-related conflicts and ensured the module installs without manifest errors. However, I have not yet been able to fully verify the functional behavior in a live v18 environment due to local setup complexities. @ecino I would appreciate if you give me more info about the usage of this module.