Conversation
e497f5e to
0704efa
Compare
HekkiMelody
left a comment
There was a problem hiding this comment.
Code and functional review, LGTM
Module was originally proposed for v16 here: #753
SirPyTech
left a comment
There was a problem hiding this comment.
Thanks for the PR!
This is good, I suggested some improvements but nothing worth blocking from merge in my opinion.
| <field name="interval_type">weeks</field> | ||
| <field | ||
| name="nextcall" | ||
| eval="(DateTime.today() + relativedelta(weekday=6)).strftime('%Y-%m-%d')" |
There was a problem hiding this comment.
suggestion: strftime might not be needed.
Moreover, why postponing the first call?
There was a problem hiding this comment.
The idea is to send the reminder on a Sunday, containing the activities for the coming week.
There was a problem hiding this comment.
nextcall field being a datetime type removed using strftime as datetime object is accepted by field
| _inherit = "res.users" | ||
|
|
||
| @api.model | ||
| def cron_send_crm_reminder_activities(self): |
There was a problem hiding this comment.
suggestion: Please consider moving these methods to crm.lead: if everyone did this, the res.users model would be full of methods for every kind of model.
There was a problem hiding this comment.
Using crm.lead model as suggested
| @api.model | ||
| def cron_send_crm_reminder_activities(self): | ||
| users = self._get_crm_activities().mapped("user_id") | ||
| mail_template = self.env.ref( |
There was a problem hiding this comment.
suggestion: Please handle the case where the template does not exist, this might be a good opportunity to add a test for this piece of code.
There was a problem hiding this comment.
Updated handling of case without template and added testcase as suggested
| for user in users: | ||
| mail_template.send_mail(user.id) | ||
|
|
||
| def _get_crm_activities(self, user=None): |
There was a problem hiding this comment.
suggestion: Please consider using self for the user, instead of user since we are in the res.users model.
There was a problem hiding this comment.
As 'crm.lead' model is used, changes are taken care accordingly
| from odoo.tests.common import TransactionCase, new_test_user | ||
|
|
||
|
|
||
| class TestEmailReminderPlannedActivity(TransactionCase): |
There was a problem hiding this comment.
suggestion: We could use BaseCommon to reduce the mail logs as suggested in https://github.com/OCA/maintainer-tools/wiki/Migration-to-version-18.0:
evaluate the use of
BaseCommonas base test class if you need to setup company, currency, users and groups in a special way)
There was a problem hiding this comment.
Inherited from Basecommon and included tracking_disable to env context
There was a problem hiding this comment.
note: Please clarify in the PR description that this is porting an unmerged module: from the point of view of the reviewer this changes the approach.
When reviewing a migration of a merged module the reviewer just has to check that the module is correctly adapted to the target version; if the module was never merged, it has to be reviewed in full like a brand new module.
There was a problem hiding this comment.
Agree, PR related information and unmerged related comments are updated
| t-value="object.env[activity.res_model].browse(activity.res_id)" | ||
| /> | ||
| <p style="margin-bottom: 5px">Deadline: | ||
| <span t-out="activity.date_deadline" /> |
There was a problem hiding this comment.
suggestion: Consider formatting this date in the user's locale, it might suffice using t-field instead of t-out.
0704efa to
20bfb76
Compare
Using "crm.lead" as model instead of "res.users" and modified module accordingly.Introduced testcase to verify sending of mail for crm activities when related mail template is not available.
20bfb76 to
bff303c
Compare


Migrating below PR which contains unmerged module
crm_reminder_email_activitiesfrom 16.0 to 18.0Related 16.0 PR: #753