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.

Sign up for GitHub

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

Jump to bottom

[16.0][IMP] product_variant_configurator: allow force variant creation #397

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

[16.0][IMP] product_variant_configurator: allow force variant creation #397

wants to merge 1 commit into from

Conversation

LauraCForgeFlow
Copy link

No description provided.

JordiMForgeFlow
JordiMForgeFlow approved these changes Feb 25, 2025
View reviewed changes
Copy link

@JordiMForgeFlow JordiMForgeFlow 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

AaronHForgeFlow
AaronHForgeFlow approved these changes Feb 25, 2025
View reviewed changes
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.

LGTM!

rousseldenis
rousseldenis approved these changes Feb 25, 2025
View reviewed changes
Copy link

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

Even if I understand the goal here, I recommend to simplify this function.

The test_enable case should be avoided by adding a parameter that enables the specific behavior.

@rousseldenis rousseldenis added this to the 16.0 milestone Feb 25, 2025
JoanSForgeFlow
JoanSForgeFlow approved these changes Feb 25, 2025
View reviewed changes
Copy link

@JoanSForgeFlow JoanSForgeFlow 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. LGTM!

@LauraCForgeFlow
Copy link
Author

@rousseldenis Okay, thanks for the review! :)
We will try to implement the changes you suggest in separate PR soon, when we have some time.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rousseldenis rousseldenis rousseldenis approved these changes

@AaronHForgeFlow AaronHForgeFlow AaronHForgeFlow approved these changes

@JoanSForgeFlow JoanSForgeFlow JoanSForgeFlow approved these changes

@JordiMForgeFlow JordiMForgeFlow JordiMForgeFlow approved these changes

Assignees
No one assigned
Labels
approved enhancement
Projects
None yet
Milestone
16.0
Development

Successfully merging this pull request may close these issues.
6 participants
@LauraCForgeFlow @rousseldenis @AaronHForgeFlow @JoanSForgeFlow @JordiMForgeFlow @OCA-git-bot