-
-
Notifications
You must be signed in to change notification settings - Fork 620
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
[16.0][FIX] mail_composer_cc_bcc: adapt to changes upstream #1519
[16.0][FIX] mail_composer_cc_bcc: adapt to changes upstream #1519
Conversation
Hi @hailangvn2023, |
hi @hbrunn
|
@trisdoan thanks, merged |
Both the previous error message and the new changes are a bit cryptic... could you please explain what was happening? |
@yajo they created a test that compares the hash of the source code of a function they override with a list of known hashes, so as soon as this changes in upstream, the test fails and we know we have to check if we need to incorporate any changes from there. Added an error message to this effect in f2e9624 |
Ah wow! That was very non-obvious. Thanks. Would you mind to squash it and put that explanation in the commit message itself? I think it'll be valuable some day when it happens again and we Apart from that, the change LGTM. |
This is a test that compares the hash of the source code of a function they override with a list of known hashes, so as soon as this changes in upstream, the test fails and we know we have to check if we need to incorporate any changes from there.
f2e9624
to
35e89eb
Compare
added the commit message. But what do you want me to squash here? Seems like three logically independent changes to me |
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 the commits said something like "adapt to changes upstream. Changes in odoo/odoo#123123 made the hash change", their messages would be more valuable.
If a single commit contained all those explanation, that'd be equally good to me.
But in any case, it's OK if you don't prefer to squash. At this point it's good enough to me. 😄
Thank you very much!
/ocabot merge patch
This PR looks fantastic, let's merge it! |
Congratulations, your PR was merged at 695fc48. Thanks a lot for contributing to OCA. ❤️ |
that's a nifty way to detect upstream changes and force adapting to it.
I wonder however if we couldn't achieve the same goal without copying so much code by setting some context keys that are picked up in an override of
ir.mail.server#build_email