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

Migration of Advanced Parameters -> Webservice -> listing #10208

Merged
merged 36 commits into from Sep 3, 2018

Conversation

Projects
None yet
7 participants
@tomas862
Contributor

tomas862 commented Aug 30, 2018

Questions Answers
Branch? develop
Description? Migration of Advanced Parameters -> Webservice -> grid migration.
Page can be reached via admin-dev/index.php/configure/advanced/webservice/
Type? new feature
Category? BO
BC breaks? no
Deprecations? no
Fixed ticket? no
How to test? Compare to the legacy Webservice controller list, the list should work exactly the same - edit and add buttons will redirect to legacy form due to they are not migrated yet.

This change is Reviewable

@tomas862 tomas862 changed the title from [WIP] Migration of Advanced Parameters -> Webservice -> listing to Migration of Advanced Parameters -> Webservice -> listing Aug 31, 2018

@tomas862

This comment has been minimized.

Show comment
Hide comment
@tomas862

tomas862 Aug 31, 2018

Contributor

The webservice list is ready for review :)

Contributor

tomas862 commented Aug 31, 2018

The webservice list is ready for review :)

@mickaelandrieu

Minor changes, but good for QA review

Show outdated Hide outdated ...restaShopBundle/Resources/config/services/adapter/data_configuration.yml Outdated
@@ -98,3 +98,8 @@ services:
class: 'PrestaShop\PrestaShop\Core\Form\ChoiceProvider\ModuleByNameChoiceProvider'
arguments:
- '@=service("prestashop.core.admin.module.repository").getInstalledModulesCollection()'
prestashop.core.form.choice_provider.status:

This comment has been minimized.

@mickaelandrieu

mickaelandrieu Aug 31, 2018

Contributor

check all your service declarations please to apply the convention

@mickaelandrieu

mickaelandrieu Aug 31, 2018

Contributor

check all your service declarations please to apply the convention

Show outdated Hide outdated ...staShopBundle/Resources/views/Admin/Common/Grid/Columns/toggle.html.twig Outdated
@sarjon

some minor comments. 👍

@@ -31,40 +31,48 @@ const $ = global.$;
*/
class ColumnToggling {

This comment has been minimized.

@sarjon

sarjon Sep 2, 2018

Member

admin-dev/themes/new-theme/js/app/utils/column-toggling.js is not the right place for this file, everything under admin-dev/themes/new-theme/js/app is Vue related stuff.

@sarjon

sarjon Sep 2, 2018

Member

admin-dev/themes/new-theme/js/app/utils/column-toggling.js is not the right place for this file, everything under admin-dev/themes/new-theme/js/app is Vue related stuff.

* @throws \PrestaShopDatabaseException
* @throws \PrestaShopException
*/
public function toggleMultipleStatus(array $columnIds, $status)

This comment has been minimized.

@sarjon

sarjon Sep 2, 2018

Member

this method does not toggle status, it sets status, even your docblock says that. :)

@sarjon

sarjon Sep 2, 2018

Member

this method does not toggle status, it sets status, even your docblock says that. :)

[],
'Admin.Notifications.Error'
) .
' <b>' . WebserviceKey::$definition['table'] . '</b> ' .

This comment has been minimized.

@sarjon

sarjon Sep 2, 2018

Member

dont use html in php.

@sarjon

sarjon Sep 2, 2018

Member

dont use html in php.

'Admin.Notifications.Error'
) .
' <b>' . WebserviceKey::$definition['table'] . '</b> ' .
$this->translator->trans('(cannot load object)', array(), 'Admin.Notifications.Error');

This comment has been minimized.

@sarjon

sarjon Sep 2, 2018

Member

you can use short syntax array

@sarjon

sarjon Sep 2, 2018

Member

you can use short syntax array

/**
* Class WebserviceDefinitionFactory is responsible for creating grid definition for Webservice grid.
*/
final class WebserviceDefinitionFactory extends AbstractGridDefinitionFactory

This comment has been minimized.

@sarjon

sarjon Sep 2, 2018

Member

I think its WebserviceKeyDefinitionFactory as you are creating grid for WebserviceKey object.

@sarjon

sarjon Sep 2, 2018

Member

I think its WebserviceKeyDefinitionFactory as you are creating grid for WebserviceKey object.

*
* @return RedirectResponse
*/
public function listCreateAction()

This comment has been minimized.

@sarjon

sarjon Sep 2, 2018

Member

maybe createAction()?

@sarjon

sarjon Sep 2, 2018

Member

maybe createAction()?

*/
public function processFormAction(Request $request)
public function processSettingsFormAction(Request $request)

This comment has been minimized.

@sarjon

sarjon Sep 2, 2018

Member

it is common to name it as processFormAction()

@sarjon

sarjon Sep 2, 2018

Member

it is common to name it as processFormAction()

@@ -216,3 +216,11 @@ services:
- '@prestashop.adapter.data_provider.language'
- '@prestashop.adapter.cache_clearer'
- '@=service("prestashop.adapter.legacy.configuration").get("_PS_TRANSLATIONS_DIR_")'
prestashop.adapter.webservice.webservice_account_eraser:

This comment has been minimized.

@sarjon

sarjon Sep 2, 2018

Member

how it is a data configuration? i think this service should be declared under webservice.yml.

@sarjon

sarjon Sep 2, 2018

Member

how it is a data configuration? i think this service should be declared under webservice.yml.

prestashop.adapter.webservice.webservice_account_eraser:
class: 'PrestaShop\PrestaShop\Adapter\Webservice\WebserviceAccountEraser'
prestashop.adapter.webservice.webservice_account_status_modifier:

This comment has been minimized.

@sarjon

sarjon Sep 2, 2018

Member

same as above.

@sarjon

sarjon Sep 2, 2018

Member

same as above.

<div class="row justify-content-center">
{{ include('@PrestaShop/Admin/Configure/AdvancedParameters/WebservicePage/webservice_settings.twig') }}
</div>
{{ form_rest(form) }}

This comment has been minimized.

@sarjon

sarjon Sep 2, 2018

Member

wrap form_rest into twig block for easier form customization

@sarjon

sarjon Sep 2, 2018

Member

wrap form_rest into twig block for easier form customization

@marionf marionf self-assigned this Sep 3, 2018

@marionf

This comment has been minimized.

Show comment
Hide comment
@marionf

marionf Sep 3, 2018

Contributor

Hello @tomas862

This page shouldn't be accessible when I click on Advanced parameters > Webservice ? I still see the legacy page
When I add a webservice key and click on Save, I am redirected to the legacy page

When I try to make a bulk action, two strings are not translated

capture d ecran_224

Contributor

marionf commented Sep 3, 2018

Hello @tomas862

This page shouldn't be accessible when I click on Advanced parameters > Webservice ? I still see the legacy page
When I add a webservice key and click on Save, I am redirected to the legacy page

When I try to make a bulk action, two strings are not translated

capture d ecran_224

@sarjon

This comment has been minimized.

Show comment
Hide comment
@sarjon

sarjon Sep 3, 2018

Member

hey @LouiseBonnard is domain Admin.Actions good for Enable selection & Disable selection actions? previously both keys were in messages domain.

Member

sarjon commented Sep 3, 2018

hey @LouiseBonnard is domain Admin.Actions good for Enable selection & Disable selection actions? previously both keys were in messages domain.

@LouiseBonnard

This comment has been minimized.

Show comment
Hide comment
@LouiseBonnard

LouiseBonnard Sep 3, 2018

Contributor

Hey @sarjon, it would be awesome to have those keys in Admin.Actions, thanks for asking. ;-)

Contributor

LouiseBonnard commented Sep 3, 2018

Hey @sarjon, it would be awesome to have those keys in Admin.Actions, thanks for asking. ;-)

@tomas862

This comment has been minimized.

Show comment
Hide comment
@tomas862

tomas862 Sep 3, 2018

Contributor

Hi @marionf ,

I think I was unclear in this pull request description - this pull request contains only the list migration.
For this reason this page is not reachable via Advanced parameters > Webservice and the ''add webservice account" and "edit" buttons points to legacy form which is not migrated yet. Every other part of the list should work as it is on legacy page.

About the two non translated strings:
These two string have been migrated to new domain Admin.Actions - in this case they have lost existing translation, See @LouiseBonnard answer above ;)

Thank you!

Contributor

tomas862 commented Sep 3, 2018

Hi @marionf ,

I think I was unclear in this pull request description - this pull request contains only the list migration.
For this reason this page is not reachable via Advanced parameters > Webservice and the ''add webservice account" and "edit" buttons points to legacy form which is not migrated yet. Every other part of the list should work as it is on legacy page.

About the two non translated strings:
These two string have been migrated to new domain Admin.Actions - in this case they have lost existing translation, See @LouiseBonnard answer above ;)

Thank you!

@marionf

This comment has been minimized.

Show comment
Hide comment
@marionf

marionf Sep 3, 2018

Contributor

All good, thank you @tomas862 for these details :)

Contributor

marionf commented Sep 3, 2018

All good, thank you @tomas862 for these details :)

@marionf marionf added QA ✔️ and removed waiting for author labels Sep 3, 2018

@marionf marionf removed their assignment Sep 3, 2018

@mickaelandrieu

This comment has been minimized.

Show comment
Hide comment
@mickaelandrieu

mickaelandrieu Sep 3, 2018

Contributor

Reviewed and validated by @marionf => LGTM

Good job @tomas862

Contributor

mickaelandrieu commented Sep 3, 2018

Reviewed and validated by @marionf => LGTM

Good job @tomas862

@mickaelandrieu mickaelandrieu added this to the 1.7.5.0 milestone Sep 3, 2018

@mickaelandrieu mickaelandrieu merged commit 13f232f into PrestaShop:develop Sep 3, 2018

2 checks passed

Codacy/PR Quality Review Up to standards. A positive pull request.
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@sarjon

This comment has been minimized.

Show comment
Hide comment
@sarjon

sarjon Sep 4, 2018

Member

@mickaelandrieu wasnt it too early to merge? there are was quite a few unresolved comments. :/

Member

sarjon commented Sep 4, 2018

@mickaelandrieu wasnt it too early to merge? there are was quite a few unresolved comments. :/

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