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

[15.0][FIX] sale_comment_template: fix action to allow to see templates in sales #260

Open
wants to merge 1 commit into
base: 15.0
Choose a base branch
from

Conversation

GasparJarsa
Copy link

This commit addresses the following issues:
Resolved an issue where users were unable to view templates in the comment list due to an incorrect domain configuration in the action.
Updated the context to set the sale order as the default option.

@GasparJarsa
Copy link
Author

@alan196 @SirAionTech Could you please help me checking this PR?

Copy link
Contributor

@alan196 alan196 left a comment

Choose a reason for hiding this comment

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

Technical and functional review 👍

@pedrobaeza pedrobaeza added this to the 15.0 milestone Apr 3, 2024
Copy link

@SirAionTech SirAionTech left a comment

Choose a reason for hiding this comment

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

Thanks for the PR!

name="context"
eval="{'default_model_ids': [(4, ref('sale.model_sale_order'))]}"
/>
<field name="domain">[('models', '=', 'sale.order')]</field>

Choose a reason for hiding this comment

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

This would only find templates that are only for sale.orders.

According to OCA/purchase-reporting@84a78d6, this

Suggested change
<field name="domain">[('models', '=', 'sale.order')]</field>
<field name="domain">[('model_ids', '=', 'sale.order')]</field>

should allow to find sale.order's templates when they are listed along with other models' templates (as it should have been before this change), would you mind including it?
In the commit you also have an example of how to implement a test case, it would be great if you could include that too.

eval="{'default_model_ids': [(4, ref('sale.model_sale_order'))]}"
/>
<field name="domain">[('models', '=', 'sale.order')]</field>
<field name="context" eval="{'default_models': 'sale.order'}" />

Choose a reason for hiding this comment

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

Suggested change
<field name="context" eval="{'default_models': 'sale.order'}" />
<field name="context">{'default_models': 'sale.order'}</field>

eval was needed for evaluating ref; it works either way so this is not blocking.

@alan196
Copy link
Contributor

alan196 commented May 7, 2024

@SirAionTech Thanks for your review! I apply your suggestion and rebase the code, could you help me to review 🙏

@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). 🤖

Copy link

@AaronHForgeFlow AaronHForgeFlow left a comment

Choose a reason for hiding this comment

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

Functional 👍

@AaronHForgeFlow
Copy link

I think this can be merged

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.

6 participants