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

Unification combination command details #30369

Merged
merged 5 commits into from Nov 22, 2022
Merged

Unification combination command details #30369

merged 5 commits into from Nov 22, 2022

Conversation

jolelievre
Copy link
Contributor

@jolelievre jolelievre commented Nov 21, 2022 •
edited

Questions Answers
Branch? develop
Description? We start the same refacto work for combination commands as we did for the product ones, the idea is to fuse all the small sub domain commands into one unified to decrease the number of DB fetches and updates and simplify the code. We use the same alternative update_product behat suite this way only one will have to be cleaned.
Type? refacto
Category? BO
BC breaks? no
Deprecations? no
Fixed ticket? ~
Related PRs ~
How to test? Refacto PR validated by automatic tests, no new component is integrated in the form for now. The final PR will be functionally tested.
Possible impacts? ~

@jolelievre jolelievre requested a review from a team as a code owner November 21, 2022 17:22
@prestonBot prestonBot added develop Branch Refactoring Type: Refactoring labels Nov 21, 2022
zuk3975
zuk3975 reviewed Nov 21, 2022
View reviewed changes
src/Adapter/Product/Combination/CommandHandler/UpdateCombinationHandler.php Outdated
private $combinationFiller;

/**
* @param CombinationRepository $combinationRepository
Copy link
Contributor

@zuk3975 zuk3975 Nov 21, 2022 •
edited

Choose a reason for hiding this comment

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

add missing param or remove this docblock completely (I've seen we don't require these when all properties are typed anymore any other PR 😄 )

Suggested change
* @param CombinationRepository $combinationRepository
* @param CombinationRepository $combinationRepository
* @param CombinationFillerInterface $combinationFiller

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I removed it 😛

zuk3975
zuk3975 previously approved these changes Nov 21, 2022
View reviewed changes
Copy link
Contributor

@zuk3975 zuk3975 left a comment

Choose a reason for hiding this comment

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

only one docblock missing, but not blocking

@jolelievre jolelievre force-pushed the unification-combination-command-details branch from a1d34dc to 94d6af5 Compare November 21, 2022 17:47
@jolelievre jolelievre merged commit 6d9ff16 into PrestaShop:develop Nov 22, 2022
@jolelievre jolelievre deleted the unification-combination-command-details branch November 22, 2022 13:14
@Progi1984 Progi1984 added this to the 8.1.0 milestone Dec 21, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@matks matks matks approved these changes

@SharakPL SharakPL SharakPL approved these changes

@zuk3975 zuk3975 zuk3975 approved these changes

@yanmakouf yanmakouf yanmakouf approved these changes

Assignees
No one assigned
Labels
develop Branch Refactoring Type: Refactoring
Projects
None yet
Milestone
8.1.0
Development

Successfully merging this pull request may close these issues.

None yet
7 participants
@jolelievre @matks @SharakPL @zuk3975 @yanmakouf @Progi1984 @prestonBot