-
Notifications
You must be signed in to change notification settings - Fork 121
Variations: Stop generation if there are more than 100 variations to generate #8541
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
Conversation
You can test the changes from this Pull Request by:
|
ealeksandrov
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Loog good! Added 1 non-blocking improvement suggestion.
| let variationsToGenerate = ProductVariationGenerator.generateVariations(for: product, excluding: existingVariations) | ||
| print("Variations to Generate: \(variationsToGenerate.count)") | ||
|
|
||
| // Guard for 100 variation limit |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can't we guard for this overflow early? So we won't actually create a lot of objects before failing? We should be able to quickly count possible combinations by multiplying the amount of options for each attribute.
But I'm not sure if we can easily subtract a number of existing variations - there may be duplicated combinations. Can we subtract just total number of existing variations for pre-generation guard (for a worst case)? And if it <100, we can proceed to existing check to make additional verification of combinations.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think we will hit a performance problem here right now. The plan is to eventually ask core to generate variations so probably not worth spending extra time on performance unless we notice something significant!
Generated by 🚫 dangerJS |
…sion seems to be having some bugs
bd7482e to
150782a
Compare
closes: #8489
Why
This PR makes sure to stop the generation process and inform the merchant when there is more than 100 generations to create.
How
Adde a new
GenerationErrortype with atooManyVariationscase to represent the situation where there are more than 100 variations to create.Integrate the error defined above in the ViewModel and in the ViewController to properly inform the merchant via a notice.
Demo
generation-error.mov
Testing Steps
RELEASE-NOTES.txtif necessary.