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] product_variant_default_code: recurrent prefix #338

Conversation

chienandalu
Copy link
Member

With a non automatic reference mask that has a code prefix, when that reference mask is recomputed, we could be repeating the prefix without noticing it. For instance, when we change an product.attribute name and that triggers all the default codes recomputations.

cc @Tecnativa TT48322

please review @pedrobaeza @victoralmau

@OCA-git-bot
Copy link
Contributor

Hi @Kev-Roche,
some modules you are maintaining are being modified, check this out!

@pedrobaeza pedrobaeza added this to the 15.0 milestone Mar 12, 2024
@pedrobaeza
Copy link
Member

Do you have deterministic steps to reproduce the problem? Are they reflected in the test as regression case?

@chienandalu
Copy link
Member Author

Do you have deterministic steps to reproduce the problem? Are they reflected in the test as regression case?

Yes, it's stated in the description (non automatic mask template and a code prefix and having a trigger in the mask recomputation, like changing an attribute name) and a new test step is added to assert the fix

@pedrobaeza
Copy link
Member

Well, that are the conditions to trigger the problem, but not the steps in the sense of (create a product this way, do this and do that). Pre-commit is red.

@Kev-Roche
Copy link
Contributor

Is it the same problem describe here ? #293
If so, a cherry pick could be enough like here #300 ?

@chienandalu
Copy link
Member Author

Thanks for the heads up @Kev-Roche

I think in any case that those fixes could be problematic, as it could happen that the prefix could match with a part inside the reference_mask. That's why I used startswith so I could restrict the issue as much as possible.

Moreover, what if the mask should be legitly updated? That case would be ignored!

I'm going to add a test for such case

Addressing @pedrobaeza request for steps:

In a product template with variants (i.e.: Conference Chair):

  • You'll have a reference mask ([Legs])
  • Add a prefix: cc

--> The prefix is duplicated in the reference mask: cccc[Legs]

  • Now you can delete de prefix manually from the mask and the resulting mask will be right: cc[Legs]
  • Now change the name of the attribute Legs to something else.

--> The mask computation is retriggered and we get a duplicated prefix again cccc[Legs].

An extra issue would be that the mask isn't recomputed with the new attribute name, although that would need some extra work.

@pedrobaeza
Copy link
Member

Please reflect that steps to reproduce in the commit message, and it's good to go.

@chienandalu chienandalu force-pushed the 15.0-fix-product_variant_default_code-recurrent-prefix branch from 7d5324d to adbfa0e Compare March 15, 2024 07:53
@chienandalu
Copy link
Member Author

Commit message amended

With a non automatic reference mask that has a code prefix, when that
reference mask is recomputed, we could be repeating the prefix without
noticing it. For instance, when we change an product.attribute name and
that triggers all the default codes recomputations.

In a product template with variants (i.e.: Conference Chair)

- You'll have a reference mask ([Legs])
- Add a prefix: cc

--> The prefix is duplicated in the reference mask: cccc[Legs]

- Now you can delete de prefix manually from the mask and the resulting mask
  will be right: cc[Legs]
- Now change the name of the attribute Legs to something else.

--> The mask computation is retriggered and we get a duplicated prefix again cccc[Legs].

An extra issue would be that the mask isn't recomputed with the new attribute
name, although that would need some extra work.

TT48322
@chienandalu chienandalu force-pushed the 15.0-fix-product_variant_default_code-recurrent-prefix branch from adbfa0e to ed1c0e2 Compare March 15, 2024 08:19
@chienandalu
Copy link
Member Author

CI Green

@pedrobaeza
Copy link
Member

@Kev-Roche is it OK for you?

Copy link
Contributor

@Kev-Roche Kev-Roche left a comment

Choose a reason for hiding this comment

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

LGTM

@pedrobaeza
Copy link
Member

/ocabot merge patch

@OCA-git-bot
Copy link
Contributor

On my way to merge this fine PR!
Prepared branch 15.0-ocabot-merge-pr-338-by-pedrobaeza-bump-patch, awaiting test results.

@OCA-git-bot OCA-git-bot merged commit 78ccb13 into OCA:15.0 Mar 15, 2024
7 checks passed
@OCA-git-bot
Copy link
Contributor

@pedrobaeza The merge process could not be finalized, because command git push origin 15.0-ocabot-merge-pr-338-by-pedrobaeza-bump-patch:15.0 failed with output:

To https://github.com/OCA/product-variant
 ! [rejected]        15.0-ocabot-merge-pr-338-by-pedrobaeza-bump-patch -> 15.0 (fetch first)
error: failed to push some refs to 'https://***@github.com/OCA/product-variant'
hint: Updates were rejected because the remote contains work that you do
hint: not have locally. This is usually caused by another repository pushing
hint: to the same ref. You may want to first integrate the remote changes
hint: (e.g., 'git pull ...') before pushing again.
hint: See the 'Note about fast-forwards' in 'git push --help' for details.

@OCA-git-bot
Copy link
Contributor

Congratulations, your PR was merged at b632eb2. 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