-
-
Notifications
You must be signed in to change notification settings - Fork 45
[19.0][MIG] edi_mail_import_oca #229
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
base: 19.0
Are you sure you want to change the base?
Conversation
c598e9c to
9b4f48e
Compare
9b4f48e to
a88067b
Compare
simahawk
left a comment
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.
Interesting approach! Thanks!
|
|
||
| class EdiExchangeType(models.Model): | ||
| _name = "edi.exchange.type" | ||
| _inherit = ["edi.exchange.type", "mail.alias.mixin"] |
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.
Thinking out loud: instead of polluting the exchange type model, wouldn't be interesting to have a specific record for this? Eg: "EDI mail gateway" or something similar.
Easier to maintain and to configure for the end user probably. WDYT?
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.
Or maybe using edi.backend. At the end, that is the goal of backends, isn't it?
c4338c9 to
c52a31e
Compare
Remove unnecessary sudo
8b6c81b to
82e92fb
Compare
|
@simahawk After your comments, I do agree that the best approach would be to set everything on the backend and do the same thing we do with Storage. Also, I improved REadmes and information inside the backend to make it easier to handle (actually, it would be nice to include similar information in other modules to make it easier for users. Thanks for your comments, IMO they improved the solution.
|
| return values | ||
|
|
||
| def _mail_exchange_type_pending_input_domain(self): | ||
| """Domain for retrieving input exchange types for emails.""" |
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 would add some more conventional constraint here.
What if we add specific type of backend Email and we rely on the fact that exc types refer to that type.
I would make it more clear from the configuration point of view.
| not custom_values.get("type_id") | ||
| and not backend.full_mail_exchange_type_id | ||
| ): | ||
| types = self.env["edi.exchange.type"].search( |
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.
if we rely on the fact that we can have multiple files in an email and multiple types configured, we should support multiple creation of records.
Hence, here we should collect all new record values and call super on each set of values at the end.
Also, each record should be related to a single parent record (conventionally I would use the 1st one).
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, I understand the idea, however, we cannot do that, as the message_new should return just one record and is attaching some extra information... however, I will find a solution for that. don't worry 😉
82e92fb to
422b3b0
Compare

Depends on #215