Skip to content
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

[17.0][MIG] mail_forward: Migration to version 17.0 #1536

Merged
merged 5 commits into from
Jan 22, 2025

Conversation

carlos-lopez-tecnativa
Copy link
Contributor

TT51933

@Tecnativa @pedrobaeza @chienandalu could you please review this

@pedrobaeza
Copy link
Member

/ocabot migration mail_forward

@OCA-git-bot OCA-git-bot added this to the 17.0 milestone Jan 17, 2025
@OCA-git-bot OCA-git-bot mentioned this pull request Jan 17, 2025
32 tasks
Copy link
Member

@pedrobaeza pedrobaeza left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tested on runboat and working. I'm not very strong on JS code, but after a general overview, it looks good.

Copy link
Member

@chienandalu chienandalu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perfect 👍

@OCA-git-bot
Copy link
Contributor

This PR has the approved label and has been created more than 5 days ago. It should therefore be ready to merge by a maintainer (or a PSC member if the concerned addon has no declared maintainer). 🤖

@pedrobaeza
Copy link
Member

/ocabot merge nobump

@OCA-git-bot
Copy link
Contributor

Hey, thanks for contributing! Proceeding to merge this for you.
Prepared branch 17.0-ocabot-merge-pr-1536-by-pedrobaeza-bump-nobump, awaiting test results.

OCA-git-bot added a commit that referenced this pull request Jan 20, 2025
Signed-off-by pedrobaeza
@OCA-git-bot
Copy link
Contributor

@pedrobaeza your merge command was aborted due to failed check(s), which you can inspect on this commit of 17.0-ocabot-merge-pr-1536-by-pedrobaeza-bump-nobump.

After fixing the problem, you can re-issue a merge command. Please refrain from merging manually as it will most probably make the target branch red.

@pedrobaeza
Copy link
Member

/ocabot merge nobump

OCA-git-bot added a commit that referenced this pull request Jan 20, 2025
Signed-off-by pedrobaeza
@OCA-git-bot
Copy link
Contributor

On my way to merge this fine PR!
Prepared branch 17.0-ocabot-merge-pr-1536-by-pedrobaeza-bump-nobump, awaiting test results.

@OCA-git-bot
Copy link
Contributor

@pedrobaeza your merge command was aborted due to failed check(s), which you can inspect on this commit of 17.0-ocabot-merge-pr-1536-by-pedrobaeza-bump-nobump.

After fixing the problem, you can re-issue a merge command. Please refrain from merging manually as it will most probably make the target branch red.

@pedrobaeza
Copy link
Member

@carlos-lopez-tecnativa can you check the error is now in mail_print module?

@carlos-lopez-tecnativa
Copy link
Contributor Author

@carlos-lopez-tecnativa can you check the error is now in mail_print module?

@pedrobaeza I believe this issue stems from an Odoo bug. For more details, please refer to the commit message:
a34b547
Would you agree that we should create a PR to Odoo to address this? Let me know your thoughts!

@pedrobaeza
Copy link
Member

pedrobaeza commented Jan 20, 2025

I can't say. If you are sure that it's the way to go... Can't be patched here in OCA? @chienandalu may suggest something as well.

@pedrobaeza
Copy link
Member

Is this mergeable then?

https://github.com/odoo/odoo/blob/a32626be4cfbaeb21ed64a0abaad298e8e223ea3/addons/mail/static/src/core/common/message.xml#L146-L160 -->
<xpath expr="//DropdownItem//i" position="before">
<t
t-if="action.callComponent"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe I'd add a flag in the action to constrain the behavior to this module.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@chienandalu I tried to add a flag, but it is not possible. The reason is that the action definition is done by adding a new action to messageActionsRegistry:
https://github.com/odoo/odoo/blob/afa014502259eb82ef03eb2d681b606665e85b42/addons/mail/static/src/core/common/message_actions.js#L14

However, this definition is transformed to be passed as XML in the function transformAction, and this function is not exportable, so I cannot inherit it to add a new flag:
https://github.com/odoo/odoo/blob/afa014502259eb82ef03eb2d681b606665e85b42/addons/mail/static/src/core/common/message_actions.js#L104

Please let me know if you have other suggestions or if we can continue with the merge.

@pedrobaeza I created the PR to Odoo. If this PR is merged, the last change can be reverted. But to avoid waiting, I think this PR is ready to merge.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please put here then the TODO that this should be removed or changed to whatever if the PR link is merged.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@pedrobaeza Done. I split the commit to keep this change in a specific commit, which can be reverted if necessary. Please let me know if this works for you. Thanks in advance.

Currently, Odoo only renders the callComponent for quick actions (2 or 3 actions). However, the remaining actions, rendered as DropdownItem, do not invoke the callComponent.
This commit ensures that the rendering is consistent in both cases.

Complementary to: #131426

TODO: This code should be removed once the issue is fixed in the Odoo core.
        odoo/odoo#194643
@pedrobaeza
Copy link
Member

/ocabot merge nobump

@OCA-git-bot
Copy link
Contributor

On my way to merge this fine PR!
Prepared branch 17.0-ocabot-merge-pr-1536-by-pedrobaeza-bump-nobump, awaiting test results.

@OCA-git-bot OCA-git-bot merged commit a6eaa48 into OCA:17.0 Jan 22, 2025
5 of 7 checks passed
@OCA-git-bot
Copy link
Contributor

Congratulations, your PR was merged at ac71ee9. Thanks a lot for contributing to OCA. ❤️

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants