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

[16][ADD][sale_partner_pricelist] New field allowed pricelist in sale/customer to as… #2620

Merged
merged 1 commit into from
Jan 22, 2024

Conversation

oscarjarsa
Copy link
Contributor

…sign multiple rates a client

@oscarjarsa oscarjarsa force-pushed the 15.0_add_new_fiel_allowed_pricelist branch 22 times, most recently from de6ecc0 to dd8776d Compare August 1, 2023 14:27
@alan196 alan196 force-pushed the 15.0_add_new_fiel_allowed_pricelist branch 2 times, most recently from 5ed09d9 to 24bd5d0 Compare August 1, 2023 15:51
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 & Functional review 👍

@alan196 alan196 force-pushed the 15.0_add_new_fiel_allowed_pricelist branch from 24bd5d0 to 6ee975c Compare August 1, 2023 16:17
@YahairaCVJarsa
Copy link

image

Functional review

@EduardoJarsa
Copy link

Funtional review, everything looks working.

Copy link
Member

@rafaelbn rafaelbn left a comment

Choose a reason for hiding this comment

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

LGTM 👍🏼


{
"name": "Sale Partner Pricelist",
"version": "16.0.1.0.0",
Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Contributor

@rousseldenis rousseldenis left a comment

Choose a reason for hiding this comment

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

Code review

class ResPartnerr(models.Model):
_inherit = "res.partner"

allowed_pricelist_ids = fields.Many2many(
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm wondering if default values could come from a configuration on partner category to ease user day-to-day job ? Maybe in a ROADMAP ?

Copy link
Contributor

Choose a reason for hiding this comment

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

I changed the behavior of the constraint, if the field is empty, it will allow all the pricelist.

With this change, we don't need to define a default value for this field


@api.constrains("property_product_pricelist")
def _check_allowed_pricelist(self):
test_mode = self.env.registry.in_test_mode() or getattr(
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't really like this kind of code. I'd prefer adding a configuration setting to enable the feature.

@oscarjarsa oscarjarsa force-pushed the 15.0_add_new_fiel_allowed_pricelist branch 9 times, most recently from 1696f8e to ede8b34 Compare September 7, 2023 21:36
@alan196 alan196 force-pushed the 15.0_add_new_fiel_allowed_pricelist branch 2 times, most recently from c55b093 to 6dbfdeb Compare September 10, 2023 11:29
@alan196
Copy link
Contributor

alan196 commented Sep 10, 2023

@rafaelbn @rousseldenis I take in consideration your comments.

I make some changes in the module and I think is ready to merge.

Could you help me to review 🙏

…ricelist to customers and show only that pricelists in the sale order
@alan196 alan196 force-pushed the 15.0_add_new_fiel_allowed_pricelist branch from 6dbfdeb to c96ae1b Compare September 10, 2023 12:00
@dreispt
Copy link
Member

dreispt commented Jan 22, 2024

/ocabot merge nobump

@OCA-git-bot
Copy link
Contributor

On my way to merge this fine PR!
Prepared branch 16.0-ocabot-merge-pr-2620-by-dreispt-bump-nobump, awaiting test results.

OCA-git-bot added a commit that referenced this pull request Jan 22, 2024
Signed-off-by dreispt
@OCA-git-bot
Copy link
Contributor

It looks like something changed on 16.0 in the meantime.
Let me try again (no action is required from you).
Prepared branch 16.0-ocabot-merge-pr-2620-by-dreispt-bump-nobump, awaiting test results.

OCA-git-bot added a commit that referenced this pull request Jan 22, 2024
Signed-off-by dreispt
@OCA-git-bot
Copy link
Contributor

It looks like something changed on 16.0 in the meantime.
Let me try again (no action is required from you).
Prepared branch 16.0-ocabot-merge-pr-2620-by-dreispt-bump-nobump, awaiting test results.

OCA-git-bot added a commit that referenced this pull request Jan 22, 2024
Signed-off-by dreispt
@OCA-git-bot
Copy link
Contributor

It looks like something changed on 16.0 in the meantime.
Let me try again (no action is required from you).
Prepared branch 16.0-ocabot-merge-pr-2620-by-dreispt-bump-nobump, awaiting test results.

@OCA-git-bot OCA-git-bot merged commit 0197925 into OCA:16.0 Jan 22, 2024
6 of 7 checks passed
@OCA-git-bot
Copy link
Contributor

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

@alan196 alan196 deleted the 15.0_add_new_fiel_allowed_pricelist branch May 7, 2024 18:15
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.