-
-
Notifications
You must be signed in to change notification settings - Fork 200
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
[IMP+FIX] account_credit_control: traceable communications #86
[IMP+FIX] account_credit_control: traceable communications #86
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.
Maybe you can add little checks in tests for count or existence of the records.
string='Credit Lines', | ||
) | ||
contact_address = fields.Many2one( | ||
contact_address_id = fields.Many2one( |
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.
There's a chance that the records on the table are not vacuum, so better to rename these fields in scripts for not losing references. Shouldn't this field be required by the way?
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.
There's a chance that the records on the table are not vacuum, so better to rename these fields in scripts for not losing references
Good catch, but actually in that case, this table would become problematic, because it would contain data that was not meant to be persistent. What I'm going to do is force a vacuum in the pre-migration.
Shouldn't this field be required by the way?
It's used automatically when the action is executed, and in those cases is always filled automatically. From then, the only thing that actually matters is message followers, so I think it is OK as it is.
@@ -86,6 +90,9 @@ def _compute_credit_control_count(self): | |||
for data in fetch_data} | |||
for rec in self: | |||
rec.credit_control_count = result.get(rec.id, 0) | |||
rec.credit_control_communication_count = len( |
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.
Please don't add this non optimized way. Can you do a read_group
?
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.
Not here, because I'd need to count distinct values for the communication_id
field. The only possibly optimized way to get that AFAIK is raw SQL. Since this is only to be executed in form views, which shouldn't have many values, I didn't see this as a big problem.
Should I do it through SQL?
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.
Well, you could add the one2many to communication and add the credit_control_run_id on communication
I forgot the runbot warning:
|
4b4ec7a
to
5894c52
Compare
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.
All attended/responded.
string='Credit Lines', | ||
) | ||
contact_address = fields.Many2one( | ||
contact_address_id = fields.Many2one( |
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.
There's a chance that the records on the table are not vacuum, so better to rename these fields in scripts for not losing references
Good catch, but actually in that case, this table would become problematic, because it would contain data that was not meant to be persistent. What I'm going to do is force a vacuum in the pre-migration.
Shouldn't this field be required by the way?
It's used automatically when the action is executed, and in those cases is always filled automatically. From then, the only thing that actually matters is message followers, so I think it is OK as it is.
@@ -86,6 +90,9 @@ def _compute_credit_control_count(self): | |||
for data in fetch_data} | |||
for rec in self: | |||
rec.credit_control_count = result.get(rec.id, 0) | |||
rec.credit_control_communication_count = len( |
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.
Not here, because I'd need to count distinct values for the communication_id
field. The only possibly optimized way to get that AFAIK is raw SQL. Since this is only to be executed in form views, which shouldn't have many values, I didn't see this as a big problem.
Should I do it through SQL?
89ec75c
to
36eadc6
Compare
cc/ @qgroulard |
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.
Most welcome improvements, thank you! (code review)
Before this patch, `credit.control.communication` was a `TransientModel` that sent emails. This produced that, when someone responded to one of those emails, Odoo detected it was a response but couldn't know where to store it, so it was lost. Also, users with credit control access got a 500 error when clicking on the "View" button in the email. Now, `credit.control.communication` is a normal `Model` which inherits from `mail.thread` and `mail.activity.mixin`. This makes more sense, because when you are tracking credit control communications, you usually communicate with a customer regarding several lines at once (no sense on asking a customer to pay each line separately). More changes: - `credit.control.run` now displays an additional smart button that leads users to generated communications. - `credit.control.line` now is linked to a communication instead of a simple message (the old link is preserved in a hidden legacy column). - `credit.control.line` has a new state "queued", which indicates that the communication is executed but still not sent. When sent, it's changed to "done" or "email_error". - Comunication emails have a specific subtype. - Some fields are renamed, as they didn't follow guidelines. - Some menu names are less verbose now. - Mail templates code is migrated. @Tecnativa TT24841
36eadc6
to
f9053c5
Compare
New changes:
@rafaelbn review again please. |
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.
After reviewing with @chienandalu this PR need some fixes for who are currently using it as when you send the emails, templates fails. Don't fail in runbot just for existing databases.
9ed247f
to
9159920
Compare
- Template translations migration scripts - Template recipients
9159920
to
9a9a5fe
Compare
If state is todo and user is manager then allow change state to draft
In the last test this are the final points. Thanks |
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 think this is a great improvement!!! Can we help with tests or something?
@yajo please check latest comments and let's merge this. |
Fixed with last commit. You just have to go to Action > Change Lines' State |
It seems the message subtype must be That's not very nice because it means that I couldn't use the However, the fact it appears as grayed-out doesn't mean it's really a note. Maybe it's just Odoo distinguishing between messages written by users from messages written by the system, isn't it? |
It doesn't sound to me that Odoo grays out that. Anyway, it should be non gray for proper UX. Can you please investigate the reason? Also the subtype can be important to be respected. |
The gray is defined on: The white is set if is_discussion or is_notification are true. So, at the end, the subtype needs to be comment (as is_discussion will not be an option, as there is model and resource) or the function must be inherited and modifed 🤔 |
Thanks @etobella, it confirms what I said in #86 (comment). So this leaves us 2 options:
I think the correct option is 2. At least for this PR. |
But can we put any subtype that puts things as not grey as Odoo does with mail templates manually sent for example? |
Update patch imported from OCA#86 to new style guidelines.
Adapt imported patch OCA#86 to v13. @Tecnativa TT24841
Update patch imported from OCA#86 to new style guidelines.
Adapt imported patch OCA#86 to v13. @Tecnativa TT24841
Update patch imported from OCA#86 to new style guidelines.
Adapt imported patch OCA#86 to v13. @Tecnativa TT24841
Update patch imported from OCA#86 to new style guidelines.
Adapt imported patch OCA#86 to v13. @Tecnativa TT24841
Update patch imported from OCA#86 to new style guidelines.
Adapt imported patch OCA#86 to v13. @Tecnativa TT24841
Update patch imported from OCA#86 to new style guidelines.
Adapt imported patch OCA#86 to v13. @Tecnativa TT24841
Update patch imported from OCA#86 to new style guidelines.
Adapt imported patch OCA#86 to v13. @Tecnativa TT24841
Update patch imported from OCA#86 to new style guidelines.
Adapt imported patch OCA#86 to v13. @Tecnativa TT24841
Update patch imported from OCA#86 to new style guidelines.
Adapt imported patch OCA#86 to v13. @Tecnativa TT24841
Update patch imported from OCA#86 to new style guidelines.
Adapt imported patch OCA#86 to v13. @Tecnativa TT24841
Update patch imported from OCA#86 to new style guidelines.
Adapt imported patch OCA#86 to v13. @Tecnativa TT24841
Update patch imported from OCA#86 to new style guidelines.
Adapt imported patch OCA#86 to v13. @Tecnativa TT24841
Update patch imported from OCA#86 to new style guidelines.
Adapt imported patch OCA#86 to v13. @Tecnativa TT24841
Update patch imported from OCA#86 to new style guidelines.
Adapt imported patch OCA#86 to v13. @Tecnativa TT24841
Before this patch,
credit.control.communication
was aTransientModel
that sent emails. This produced that, when someone responded to one of those emails, Odoo detected it was a response but couldn't know where to store it, so it was lost. Also, users with credit control access got a 500 error when clicking on the "View" button in the email.Now,
credit.control.communication
is a normalModel
which inherits frommail.thread
andmail.activity.mixin
. This makes more sense, because when you are tracking credit control communications, you usually communicate with a customer regarding several lines at once (no sense on asking a customer to pay each line separately).More changes:
credit.control.run
now displays an additional smart button that leads users to generated communications.credit.control.line
now is linked to a communication instead of a simple message (the old link is preserved in a hidden legacy column).@Tecnativa TT24841