-
-
Notifications
You must be signed in to change notification settings - Fork 191
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
[12.0] Add module account_invoice_overdue_reminder #69
Conversation
I'm working on backporting this module to v10. Where should I propose it ? There isn't any 10.0 branch on this project (starts at 11.0)... |
6be2747
to
49f3c7e
Compare
49f3c7e
to
ba70743
Compare
FYI, PR for v10 is here: #77 |
def name_get(self): | ||
res = [] | ||
for rec in self: | ||
name = _('%s Reminder %d') % (rec.invoice_id.number, rec.counter) |
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.
name = _('%s Reminder %d') % (rec.invoice_id.number, rec.counter) | |
name = _("{} Reminder {}".format(rec.invoice_id.number, rec.counter)) |
I think a new format is better. What do you think about it?
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 is the real advantage of it ? The result is the same, no ?
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.
In this place, the result is the same but the new format string syntax has become more powerful without complicating the simpler use cases. You can see other reasons in https://realpython.com/python-string-formatting/#which-string-formatting-method-should-you-use.
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.
Thanks to inform us about string formatting. As it's python 3.6+ I wonder if it works like that
name = _(f'{rec.invoice_id.number} Reminder {rec.counter}')
If yes the syntax is really readable
name = _('%s, Reminder %s') % ( | ||
action.commercial_partner_id.display_name, action.date) |
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.
name = _('%s, Reminder %s') % ( | |
action.commercial_partner_id.display_name, action.date) | |
name = _("{}, Reminder {}".format(action.commercial_partner_id.display_name, action.date)) |
Same comments as above.
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.
Functional Test, LGTM 👍
and code reviewed:
@alexis-via
File name (,py and .xml) that depend core should be used name same as a core odoo
i.e.
company.py should be res_company.py
config_settings.py should be res_config_settings.py
partner.py should be res_partner.py
|
||
{ | ||
'name': 'Overdue Invoice Reminder', | ||
'version': '12.0.2.0.0', |
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.
'version': '12.0.2.0.0', | |
'version': '12.0.1.0.0', |
Module should be start at version 12.0.1.0.0
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.
yes
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.
Héhé, this module already had a "long" life, is already used in production ! And there are migration scripts associated with this version number (due to changes in the datamodel), so I won't change it!
<odoo noupdate="1"> | ||
|
||
|
||
<record id="voicemail" model="overdue.reminder.result"> |
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.
<record id="voicemail" model="overdue.reminder.result"> | |
<record id="voicemail" model="overdue.reminder.result"> |
Could you tab before code, it'll look good.
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.
When you have large spaced files begin at the first char make it more readable avoiding too many break lines
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.
In v13, I'll apply black formatting. In the meantime, let's focus on the important things please.
@Saran440 Thanks for your review. IMHO, renaming company.py (or other) to res_company.py is useless. |
…CA#69 : - It added in our fork because in this time was not merged in OCA.
…update your mail templates) Add ability to add contacts as Cc of the reminder email (added to the Cc of the mail template) Add partner_policy with 3 options to give some choice about which contact should be selected to send reminders Access reminders from partner via Action menu
Advantages: 1) easy access to the history in the chatter of the partner. 2) no problem to access the email by other users (avoid "The requested operation cannot be completed due to security restrictions. Please contact your system administrator.")
account_invoice_overdue_reminder/wizard/overdue_reminder_wizard.py
Outdated
Show resolved
Hide resolved
Co-authored-by: ypapouin <y.papouin@dec-industrie.com>
Co-authored-by: ypapouin <y.papouin@dec-industrie.com>
@ypapouin Thanks for your review; nice catch for the selection field. |
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.
LGTM
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.
Tested in production
We just got an issue in production, I'm not sure if its an odoo bug or a misusing of transient model and attachments. This is my analysis:
This is my technical analysis:
Note that the deletion works perfectly when called from the auto-vacuum cron since So my conclusion:
The traceback:
|
@alexis-via any chance to merge in main branch ? Thanks for this great module. |
@ypapouin Strange... I don't see why you would have an ir.attachment on overdue.reminder.step. I don't see anywhere in the code where I would create an attachment on overdue.reminder.step. |
@api.depends('type', 'state', 'date_due') | ||
def _compute_overdue(self): | ||
today = fields.Date.context_today(self) | ||
for inv in self: |
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.
for inv in self: | |
for inv in self.filtered('date_due'): |
Because inv.date_due < today
will fail if date_due
is False
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.
I wasn't aware that date_due could be False on an open invoice. When does it happen ?
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.
When property_payment_term_id
is not set on a partner, it is not filled when a new invoice in created so no date_due
.
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.
I'm sorry, but that's not true. When the payment term is empty, date_due = date_invoice. Here is the relevant code in the account module: https://github.com/odoo/odoo/blob/12.0/addons/account/models/account_invoice.py#L1287
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.
Indeed you are right, the problem comes from open invoices created in OpenERP 6.1 before the migration.
Sorry for the buzz.
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.
Functional test ok
Hi @alexis-via thanks for this! |
@eLBati it's ready to merge, of course. And also for the other versions. |
@OCA/accounting-maintainers could you merge? Thanks |
/ocabot merge nobump |
Hey, thanks for contributing! Proceeding to merge this for you. |
Congratulations, your PR was merged at 3609369. Thanks a lot for contributing to OCA. ❤️ |
No description provided.