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

Jump to bottom

Webservice symfony less for multilang entities and category creation #27770

Merged
merged 3 commits into from Mar 24, 2022
Merged

Webservice symfony less for multilang entities and category creation #27770

merged 3 commits into from Mar 24, 2022

Conversation

jolelievre
Copy link
Contributor

@jolelievre jolelievre commented Feb 22, 2022 •
edited

Questions Answers
Branch? develop
Description? This PR reverts the integration of Symfony kernel in the webservice dispatcher And it includes an alternative fix to the original issue related to multi lang entities And another one for category creation, we rely more on ugly legacy code but at least we don't create a Symfony dependency in an environment that shouldn't have one
Type? bug fix
Category? WS
BC breaks? no
Deprecations? no
Fixed ticket? Fixes #20691 and #27411
How to test? We should ensure that #20691 and #27411 are still behaving as expected in API
Possible impacts? ~

This change is Reviewable

@jolelievre jolelievre requested a review from a team as a code owner February 22, 2022 13:19
@prestonBot prestonBot added develop Branch Bug fix Type: Bug fix labels Feb 22, 2022
@jolelievre jolelievre mentioned this pull request Feb 22, 2022
Closed
@jolelievre jolelievre mentioned this pull request Feb 22, 2022
Merged
matks
matks approved these changes Feb 24, 2022
View reviewed changes
Copy link
Contributor

@matks matks left a comment

Choose a reason for hiding this comment

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

CI passes now 😄

@matks matks added the Waiting for QA Status: action required, waiting for test feedback label Feb 24, 2022
@Progi1984 Progi1984 added this to the 8.0.0 milestone Feb 24, 2022
@matks matks added the Key feature Notable feature to be highlighted label Feb 28, 2022
@khouloudbelguith khouloudbelguith self-assigned this Mar 22, 2022
Amoifr
Amoifr reviewed Mar 24, 2022
View reviewed changes
classes/Category.php
@@ -1730,9 +1728,7 @@ public function updateGroup($list)
}
$this->cleanGroups();
if (empty($list)) {
$sfContainer = SymfonyContainer::getInstance();
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not very confortable with using service into an entity class.
On my point of view is not the responsability of the entity to do that. The entity role is about throwing an exception if the param is empty. The caller have to prevent this.

khouloudbelguith
khouloudbelguith approved these changes Mar 24, 2022
View reviewed changes
Copy link
Contributor

@khouloudbelguith khouloudbelguith left a comment

Choose a reason for hiding this comment

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

Hi @jolelievre,

It is ok ✔️

  • Add a category from BO > Categories page
  • Add a root category from BO > Categories page
  • Add a category from the Product page
  • Import category
  • Category created from importing a product
Untitled_.Mar.24.2022.6_13.PM.mp4
  • Add a category from API => ok
  • Update a category from API => ok
Untitled_.Mar.24.2022.6_20.PM.mp4

With API not an empty shop, I tried to:

  • Add a product
  • Add a cart rule
  • Add a file
  • Add a customer
  • Add a group customer
  • Add a category
  • Remove a carrier
  • Update the status of an order
  • Add an order status
Untitled_.Mar.24.2022.6_34.PM.mp4

Shop No data after fresh install

  • Add category
  • Update a category
  • Add a product
  • Update the status of an order
  • Add a customer
  • Add an order status
  • Add a cart rule
  • Add a file
  • Remove a carrier
Untitled_.Mar.24.2022.6_45.PM.mp4

I tried automatic tests => OK
Mochawesome Report_cat.pdf

Thanks!

@khouloudbelguith khouloudbelguith added QA ✔️ Status: check done, code approved and removed Waiting for QA Status: action required, waiting for test feedback labels Mar 24, 2022
@jolelievre
Copy link
Contributor Author

Thanks @khouloudbelguith

@jolelievre jolelievre merged commit 2631cbb into PrestaShop:develop Mar 24, 2022
@jolelievre jolelievre deleted the webservice-symfony-less branch March 24, 2022 18:08
@awitkutarahil
Copy link
Contributor

Nice work!

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

@Amoifr Amoifr Amoifr left review comments

@PierreRambaud PierreRambaud PierreRambaud approved these changes

@atomiix atomiix atomiix approved these changes

@matks matks matks approved these changes

@tegbessou tegbessou tegbessou approved these changes

@khouloudbelguith khouloudbelguith khouloudbelguith approved these changes

Assignees

@khouloudbelguith khouloudbelguith

Labels
Bug fix Type: Bug fix develop Branch Key feature Notable feature to be highlighted QA ✔️ Status: check done, code approved
Projects
None yet
Milestone
8.0.0
Development

Successfully merging this pull request may close these issues.

When creating category, select all groups by default
10 participants
@jolelievre @awitkutarahil @PierreRambaud @atomiix @matks @tegbessou @khouloudbelguith @Amoifr @Progi1984 @prestonBot