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

Migrate Sell > Catalog > Attributes & Features > Attributes list #14040

Merged
merged 51 commits into from Oct 11, 2019

Conversation

@zuk3975
Copy link
Contributor

zuk3975 commented May 31, 2019

Questions Answers
Branch? develop
Description? Migrate Sell > Catalog > Attributes & Features > Attributes list. The main Attributes page is managed by AttributeGroupController and the Attribute > View action page is managed by AttributeController. Also migrates both lists filtering, delete and bulk delete actions. In another separate PR's left to implement list actions (edit/create) and positions dragging.
Type? refacto
Category? BO
BC breaks? no
Deprecations? no
Fixed ticket? part of #10511
How to test? Use admin-dev/index.php/sell/catalog/attribute-groups to reach migrated attributes list. The list filtering, view action, delete and bulk delete actions should work as in legacy page. Create/edit/export actions should be implemented in separate PR.

This change is Reviewable

@zuk3975 zuk3975 requested a review from PrestaShop/prestashop-core-developers as a code owner May 31, 2019
@matks matks added the migration label Jun 3, 2019
@zuk3975

This comment has been minimized.

Copy link
Contributor Author

zuk3975 commented Jun 6, 2019

@matks, sorry this has grown to a big PR, but it will be easier to understand the idea with two controllers. It requires a review, i guess ill have something to work on after it.

@zuk3975 zuk3975 force-pushed the zuk3975:m/attributes-list branch from 713a163 to 4031a58 Jun 18, 2019
@zuk3975 zuk3975 changed the title [WIP] Migrate Sell > Catalog > Attributes & Features > Attributes list Migrate Sell > Catalog > Attributes & Features > Attributes list Jun 18, 2019
src/Core/Grid/Column/Type/Attribute/ColorColumn.php Outdated Show resolved Hide resolved
@@ -54,6 +54,7 @@ protected function configureOptions(OptionsResolver $resolver)
->setDefaults([
'confirm_message' => '',
'accessibility_checker' => null,
'extra_route_params' => [],

This comment has been minimized.

Copy link
@matks

matks Jul 12, 2019

Contributor

@zuk3975 Can you explain why you needed this options ? I'm not sure to see it 🤔

@zuk3975

This comment has been minimized.

Copy link
Contributor Author

zuk3975 commented Jul 15, 2019

@matks, regarding #14040 (comment) (I wasn't allowed to quote reply, idk why 😄 )
I use it to pass extra parameters to grid action. I need attributeId and attributeGroupId to be present in action url which is rendered by grid.
You can see the usage example here https://github.com/PrestaShop/PrestaShop/pull/14040/files#diff-22d680d5c47f9296fec1acf8d8f6c067R164

Copy link
Contributor

matks left a comment

Ok, but this seems weird now 🤔 we have parameters:

  • route_param_name
  • route_param_field
  • extra_route_params

So it's redundant 😢

I guess we did a mistake but not going from the very beginning for the full array extra_route_params. We hoped we would never need more than 1 route parameter. We were wrong...

Can you explain this in a comment ? Because without the explanation, developers will be confused, when they have a single parameter in the route, whether to use the first 2 options or the extra_route_params

attributeGroupId: \d+

admin_attributes_search:
path: /

This comment has been minimized.

Copy link
@matks

matks Jul 22, 2019

Contributor

There's 6 spaces here 😄 we want 4 !

This comment has been minimized.

Copy link
@zuk3975

zuk3975 Jul 22, 2019

Author Contributor

Whoops 😅

Copy link
Contributor

sarjon left a comment

Reviewed 4 of 35 files at r1, 8 of 25 files at r2, 10 of 39 files at r3, 34 of 47 files at r4, 5 of 7 files at r5, 4 of 4 files at r6.
Reviewable status: 59 of 61 files reviewed, 31 unresolved discussions (waiting on @eternoendless, @matks, @sarjon, and @zuk3975)


src/Adapter/AttributeGroup/AttributeGroupViewDataProvider.php, line 38 at r6 (raw file):

 * Provides data required for attribute group view action
 */
final class AttributeGroupViewDataProvider

Isn't it something we want to avoid, having service without interface?


src/Adapter/AttributeGroup/AttributeGroupViewDataProvider.php, line 68 at r6 (raw file):

     * @throws AttributeGroupNotFoundException
     */
    public function isColorGroup($attributeGroupId)

How about query GetAttributeGroupIsColor?


src/Adapter/AttributeGroup/AttributeGroupViewDataProvider.php, line 85 at r6 (raw file):

     * @throws AttributeGroupNotFoundException
     */
    public function getAttributeGroupNameById($attributeGroupId)

How about dedicated query as well?


src/Core/Grid/Column/Type/Attribute/ColorColumn.php, line 35 at r4 (raw file):

Previously, matks (Mathieu Ferment) wrote…

I've seen something in one of Sarunas or Tomas PR 🤔let's make sure we dont create a duplicate

Could be related to HighlightedColumn


src/Core/Grid/Definition/Factory/CustomerGridDefinitionFactory.php, line 83 at r6 (raw file):

        HookDispatcherInterface $hookDispatcher,
        $isB2bFeatureEnabled,
        $isMultistoreFeatureEnabledGroup,

Variable name is not clear.


src/PrestaShopBundle/Resources/views/Admin/Sell/Catalog/Attribute/index.html.twig, line 40 at r4 (raw file):

Previously, zuk3975 (Julius Žukauskas) wrote…

When including it i get a weird space before the grid footer.
Include:
Screenshot from 2019-07-15 15-38-40
Embed:
Screenshot from 2019-07-15 15-38-58

Weird, this should be checked why it happens so with include.

@zuk3975

This comment has been minimized.

Copy link
Contributor Author

zuk3975 commented Jul 24, 2019

@sarjon regarding queries,

src/Adapter/AttributeGroup/AttributeGroupViewDataProvider.php, line 68 at r6 (raw file):

* @throws AttributeGroupNotFoundException
*/

public function isColorGroup($attributeGroupId)
How about query GetAttributeGroupIsColor?

When I had dedicated queries, I was suggested to get rid of it and use adapter with object model instead. (#14040 (comment)).

@matks

This comment has been minimized.

Copy link
Contributor

matks commented Jul 24, 2019

@sarjon regarding queries,

src/Adapter/AttributeGroup/AttributeGroupViewDataProvider.php, line 68 at r6 (raw file):

* @throws AttributeGroupNotFoundException
*/

public function isColorGroup($attributeGroupId)
How about query GetAttributeGroupIsColor?

When I had dedicated queries, I was suggested to get rid of it and use adapter with object model instead. (#14040 (comment)).

About this: the reason why we do not use Queries in DataProvider for grids is that we introduced CQRS after having built the "how to create a grid" strategy.

So far it's doing OK 🤔 it works, code is small and located in the right place so I think we can stick to this rule for a while then at some point replace it all with migrated domain code.

I think @mickaelandrieu's plan was to build Grids with neither CQRS nor ObjectModel required, so I guess in the future if we stick to the initial plan, we should replace these Adapters with Doctrine-powered data providers.

@zuk3975 zuk3975 force-pushed the zuk3975:m/attributes-list branch from e702c53 to 9546e0c Jul 25, 2019
@sarjon

This comment has been minimized.

Copy link
Contributor

sarjon commented Aug 5, 2019

Are comments addressed in this PR?

@zuk3975

This comment has been minimized.

Copy link
Contributor Author

zuk3975 commented Aug 5, 2019

Are comments addressed in this PR?

Are comments addressed in this PR?

yes

@matks

This comment has been minimized.

Copy link
Contributor

matks commented Aug 26, 2019

@zuk3975 I checked, and it's a 💯! Let's go for QA !

@sarjon thanks 😉

@matks matks dismissed their stale review via d2e8d7e Oct 11, 2019
@matks matks force-pushed the zuk3975:m/attributes-list branch from e6046ac to d2e8d7e Oct 11, 2019
@matks
matks approved these changes Oct 11, 2019
@matks

This comment has been minimized.

Copy link
Contributor

matks commented Oct 11, 2019

Thank you @zuk3975

@matks matks merged commit f7c3c16 into PrestaShop:develop Oct 11, 2019
2 of 3 checks passed
2 of 3 checks passed
code-review/reviewable 92 files, 31 discussions left (eternoendless, matks, Robin-Fischer-PS, sarjon, zuk3975)
Details
PrettyCI Code formatting
Details
Travis CI - Pull Request Build Passed
Details
@matks matks added this to the 1.7.7.0 milestone Oct 28, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.