-
-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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] [MIG] fetchmail_notify_error_to_sender #1485
[12.0] [MIG] fetchmail_notify_error_to_sender #1485
Conversation
2f7d07e
to
5b9e995
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.
Check travis
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.
Fix travis.
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 5b9e995#diff-64c32bfdead5f1c8738a8c486826fcb7R63, you should change "mail.test" by "mail.test.simple".
In 5b9e995#diff-64c32bfdead5f1c8738a8c486826fcb7R51, change "'[email protected]'
" by "formataddr((partner_1.name, partner_1.email))
", and before that you should include:
partner_1 = self.env['res.partner'].with_context(self._quick_create_ctx).create({
'name': 'Valid Lelitre',
'email': '[email protected]',
})
730994d
to
2b0c6f7
Compare
@mreficent @yajo , any activity on this pull request? We can help in migration. Because we also have migrated version of this module that is actively used by our customer in production, but we are sending it as ZIP archive =) It is better if we can just give direct link to OCA. So how can we speed up? Taking into account that another person working on it |
It seems to be waiting for odoo/odoo#31799 to be merged. |
odoo/odoo#31799 has been merged 😋. Tomorrow I will update this PR. |
af678cc
to
d0389dc
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.
@yajo please remove "needs fixing" tag.
Ready to merge 👍 🍏
'license': 'AGPL-3', | ||
'depends': [ | ||
'fetchmail', | ||
'test_mail' |
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 find it weird to require installing a module that is only meant for testing purposes. It would clutter a production database with useless models, possibly adding some weight for migrations. I'm not saying it's wrong, but do you think there could be a better approach? 🤔 I have seen a couple of interesting clues:
- The
hr
module uses this one for tests without adding it to its dependencies. Would this work? - In the case of the
mass_mailing
addon, there's a separate addon for doing the tests, which depends on the addon being tested (mass_mailing
) and the addon providing the test structure (test_mail
). What do you think about doing it like that?
@@ -14,7 +14,7 @@ | |||
'license': 'AGPL-3', | |||
'depends': [ | |||
'fetchmail', | |||
'test_mail' | |||
# 'test_mail', |
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.
Why don't you just delete the line 😄
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.
😑 👍
c43de86
to
5bd1364
Compare
Hey, the tests seem aren't working. What we do with the travis-ci? :S |
I guess there's no other option than to depend on test_mail. OK, could you please squash migration commits? |
* [FIX] auth_from_http_remote_user - Lint * [FIX] server_environment - Lint * [FIX] base_module_doc_rst - Lint * [FIX] fetchmail_notify_error_to_sender - Fix XML view, it was the 'active' field from 'fetchmail_attach_from_folder' module which was targeted by mistake (belonging to another data model)
…template tech. msg issue
a996691
to
d4f6f99
Compare
@yajo done! 🍏 |
Syncing from upstream OCA/server-tools (10.0)
Standard migration