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

Fix grid ids in migrated JS #10656

Merged
merged 3 commits into from Sep 25, 2018

Conversation

Projects
None yet
7 participants
@sarjon
Member

sarjon commented Sep 24, 2018

Questions Answers
Branch? 1.7.5.x
Description? Fix Grid ids in js. It's related to #10569
Type? bug fix
Category? BO
BC breaks? no
Deprecations? no
Fixed ticket? n/a
How to test? Grid ("Show SQL query", "Refresh list" & etc.) and Bulk ("Delete selected" & etc.) actions of migrated list should work as they did before.

This change is Reviewable

@sarjon

This comment has been minimized.

Show comment
Hide comment
@sarjon

sarjon Sep 24, 2018

Member

@PierreRambaud any ideas whats wrong with unit tests? :/

Member

sarjon commented Sep 24, 2018

@PierreRambaud any ideas whats wrong with unit tests? :/

@PierreRambaud

This comment has been minimized.

Show comment
Hide comment
@PierreRambaud

PierreRambaud Sep 24, 2018

Contributor

@sarjon Done mate ;)

Contributor

PierreRambaud commented Sep 24, 2018

@sarjon Done mate ;)

@eternoendless eternoendless added this to the 1.7.5.0 milestone Sep 24, 2018

@marionf

This comment has been minimized.

Show comment
Hide comment
@marionf

marionf Sep 24, 2018

Contributor

Hello @sarjon

I have an exception when I try to delete in bulk action on the Traffic & SEO page

capture d ecran_318

Contributor

marionf commented Sep 24, 2018

Hello @sarjon

I have an exception when I try to delete in bulk action on the Traffic & SEO page

capture d ecran_318

@@ -55,7 +55,7 @@ public function present(GridInterface $grid)
$searchCriteria = $grid->getSearchCriteria();
$data = $grid->getData();
$presentedGrid = [
'id' => $definition->getId(),
'id' => strtolower($definition->getId()),

This comment has been minimized.

@sarjon

sarjon Sep 24, 2018

Member

i dont like this workaround, but its related to this issue #10412 :(

@sarjon

sarjon Sep 24, 2018

Member

i dont like this workaround, but its related to this issue #10412 :(

@matks

This comment has been minimized.

Show comment
Hide comment
@matks

matks Sep 24, 2018

Contributor

I agree with this change but this is a major technical setting so we need eternoendless to agree with it.

@eternoendless can you review #10412 and sarjon latest commit to fix it ?

Contributor

matks commented Sep 24, 2018

I agree with this change but this is a major technical setting so we need eternoendless to agree with it.

@eternoendless can you review #10412 and sarjon latest commit to fix it ?

@marionf

This comment has been minimized.

Show comment
Hide comment
@marionf

marionf Sep 24, 2018

Contributor

It doesn't fix the issues, maybe an assets problem ?

Contributor

marionf commented Sep 24, 2018

It doesn't fix the issues, maybe an assets problem ?

@sarjon

This comment has been minimized.

Show comment
Hide comment
@sarjon

sarjon Sep 24, 2018

Member

maybe an assets problem

yes, im waiting for feedback on this solution before rebasing & building assets. :/

Member

sarjon commented Sep 24, 2018

maybe an assets problem

yes, im waiting for feedback on this solution before rebasing & building assets. :/

@matks

This comment has been minimized.

Show comment
Hide comment
@matks

matks Sep 24, 2018

Contributor

@sarjon Solution approved by eternoendless . We're good here ! Please rebase & rebuild assets

Contributor

matks commented Sep 24, 2018

@sarjon Solution approved by eternoendless . We're good here ! Please rebase & rebuild assets

@sarjon

This comment has been minimized.

Show comment
Hide comment
@sarjon

sarjon Sep 25, 2018

Member

rebase done.

i noticed that we need to change more things in order to fix everything. :/ what if we keep grid ids as they where before and only update how hooks are executed? For example instead of this:

$this->hookDispatcher->dispatchWithParameters('action' . $definition->getId() . 'GridDefinitionModifier', [
    'definition' => $definition,
]);

we keep same ids and do this:

$this->hookDispatcher->dispatchWithParameters('action' . SomeHelper::toCamelCase($definition->getId()) . 'GridDefinitionModifier', [
    'definition' => $definition,
]);

so key email_logs would become EmailLogs?

@matks wdyt?

Member

sarjon commented Sep 25, 2018

rebase done.

i noticed that we need to change more things in order to fix everything. :/ what if we keep grid ids as they where before and only update how hooks are executed? For example instead of this:

$this->hookDispatcher->dispatchWithParameters('action' . $definition->getId() . 'GridDefinitionModifier', [
    'definition' => $definition,
]);

we keep same ids and do this:

$this->hookDispatcher->dispatchWithParameters('action' . SomeHelper::toCamelCase($definition->getId()) . 'GridDefinitionModifier', [
    'definition' => $definition,
]);

so key email_logs would become EmailLogs?

@matks wdyt?

@matks

matks approved these changes Sep 25, 2018

@eternoendless

This comment has been minimized.

Show comment
Hide comment
@eternoendless

eternoendless Sep 25, 2018

Member

Broken tests unrelated to PR

Member

eternoendless commented Sep 25, 2018

Broken tests unrelated to PR

@eternoendless

This comment has been minimized.

Show comment
Hide comment
@eternoendless
Member

eternoendless commented Sep 25, 2018

Thank you @sarjon

@eternoendless eternoendless merged commit 973a5b4 into PrestaShop:1.7.5.x Sep 25, 2018

0 of 2 checks passed

Codacy/PR Quality Review Codacy was unable to analyse your pull request.
Details
continuous-integration/travis-ci/pr The Travis CI build failed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment