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

Get all categories in category tree form (not only enabled ones) #15030

Merged
merged 4 commits into from
Aug 12, 2019

Conversation

matthieu-rolland
Copy link
Contributor

@matthieu-rolland matthieu-rolland commented Aug 8, 2019

Questions Answers
Branch? 1.7.6.x
Description? Include disabled categories in category tree.
Type? bug fix
Category? BO
BC breaks? no
Deprecations? no
Fixed ticket? fixes #14790
How to test? see #14790

This change is Reviewable

@matthieu-rolland matthieu-rolland requested a review from a team as a code owner August 8, 2019 09:46
@matthieu-rolland
Copy link
Contributor Author

matthieu-rolland commented Aug 8, 2019

I could make this simple modification because this CategoryTreeChoiceProvider is only used by one service, which is on only used in the category form.

In the future if we need to have another category tree that doesn't take disabled categories into account, then we'll have to make that optional via the constructor, and configure it in
src/PrestaShopBundle/Resources/config/services/adapter/form.yml

It just felt like premature optimization at this point, don't hesitate to tell me if you think otherwise.

@prestonBot prestonBot added the Bug Type: Bug label Aug 9, 2019
@eternoendless eternoendless added Bug fix Type: Bug fix and removed Bug Type: Bug fix labels Aug 9, 2019
@eternoendless eternoendless changed the title get all categories in category tree form (not only enabled ones) Get all categories in category tree form (not only enabled ones) Aug 9, 2019
@prestonBot prestonBot added the Bug Type: Bug label Aug 9, 2019
@eternoendless
Copy link
Member

eternoendless commented Aug 9, 2019

I bet this will become a problem once we try and use the same provider elsewhere. I think we should add the option in the constructor. Remember that other people may be using this class for purposes unknown to us.

@eternoendless eternoendless added this to the 1.7.6.1 milestone Aug 9, 2019
Copy link
Member

@eternoendless eternoendless left a comment

Choose a reason for hiding this comment

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

Please add the option in the constructor

@matthieu-rolland
Copy link
Contributor Author

done !

matks
matks previously approved these changes Aug 12, 2019
@matks matks dismissed stale reviews from jolelievre and eternoendless August 12, 2019 08:34

Requested change has been applied

@matks
Copy link
Contributor

matks commented Aug 12, 2019

@matthieu-rolland PR logic is ✅but codestyle is broken according to PrettyCI
Can you fix and update the PR ?

@matks matks added the Waiting for QA Status: action required, waiting for test feedback label Aug 12, 2019
@marionf marionf added QA ✔️ Status: check done, code approved and removed Waiting for QA Status: action required, waiting for test feedback labels Aug 12, 2019
@eternoendless
Copy link
Member

Thank you @matthieu-rolland

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
1.7.6.x Branch Bug fix Type: Bug fix Bug Type: Bug QA ✔️ Status: check done, code approved
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants