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 Shop parameters -> Traffic & Seo -> Seo & urls page #10234

Merged
merged 74 commits into from Sep 20, 2018

Conversation

Projects
None yet
7 participants
@tomas862
Contributor

tomas862 commented Sep 1, 2018

Questions Answers
Branch? develop
Description? New list and configurations are applied. Page can be reached via admin-dev/index.php/configure/shop/seo-urls/ only due to add/edit page contains legacy form still
Type? new feature
Category? BO
BC breaks? no
Deprecations? no
Fixed ticket?
How to test? Should work identically as Shop parameters -> Traffic & Seo -> Seo & Urls legacy page without add/edit actions because they are pointing to legacy forms
test tips of unique features of this controller:
1. Try to enable shop feature and check (Should be no difference with legacy controller)
2. Enable host mode and check whats changed in settings (Simpliest way is to add define('PS_HOST_MODE', true) somewhere )
3. try to remove .htaccess , robots.txt files or not give permissions to these files

This change is Reviewable

@sarjon

everything is named SeoUrls*, but is it a right naming? in legacy it was called AdminMetaController, so i would prefer either SeoAndUrlsController or keep MetaController for consistency, @mickaelandrieu wdyt?

@sarjon

quick review done

Show outdated Hide outdated admin-dev/themes/new-theme/webpack.config.js Outdated
Show outdated Hide outdated src/Adapter/Meta/ShopUrlDataConfigurator.php Outdated
Show outdated Hide outdated src/Adapter/Shop/ShopUrl.php Outdated
Show outdated Hide outdated src/Adapter/Shop/ShopUrl.php Outdated
* Class HtaccessFileChecker is responsible for checking files validity which are related with
* urls generation (.htaccess) file.
*/
final class HtaccessFileChecker implements UrlFileCheckerInterface

This comment has been minimized.

@sarjon

sarjon Sep 9, 2018

Member

this class is named HtaccessFileChecker , but seems it can check for any file (image, text & etc.) as you pass path as parameters. i think it would be better to inject htaccess file path via constructor.

@sarjon

sarjon Sep 9, 2018

Member

this class is named HtaccessFileChecker , but seems it can check for any file (image, text & etc.) as you pass path as parameters. i think it would be better to inject htaccess file path via constructor.

This comment has been minimized.

@mickaelandrieu

mickaelandrieu Sep 18, 2018

Contributor

make sense

@mickaelandrieu

mickaelandrieu Sep 18, 2018

Contributor

make sense

Show outdated Hide outdated src/Core/File/HtaccessFileFinder.php Outdated
Show outdated Hide outdated ...opBundle/Form/Admin/Configure/ShopParameters/TrafficSeo/SetUpUrlType.php Outdated

@mickaelandrieu mickaelandrieu changed the title from [WIP] Migration of Shop parameters -> Traffic & Seo -> Seo & urls page to Migration of Shop parameters -> Traffic & Seo -> Seo & urls page Sep 18, 2018

@mickaelandrieu

Except for the todos that need to be done, I'm ok with this pull request.

Good job.

Minor comments can be addressed after the freeze.

Show outdated Hide outdated src/Adapter/Meta/SetUpUrlsDataConfiguration.php Outdated
Show outdated Hide outdated src/Adapter/Meta/ShopUrlDataConfiguration.php Outdated
Show resolved Hide resolved src/Adapter/Tools.php
Show outdated Hide outdated src/Adapter/Shop/Context.php Outdated
Show outdated Hide outdated .../Admin/Configure/ShopParameters/TrafficSeo/Meta/MetaFormDataProvider.php Outdated
Show outdated Hide outdated ...pBundle/Form/Admin/Configure/ShopParameters/TrafficSeo/Meta/MetaType.php Outdated
@tomas862

This comment has been minimized.

Show comment
Hide comment
@tomas862

tomas862 Sep 18, 2018

Contributor

Thanks for reviews @mickaelandrieu ! I moved the part with todos (it was a form part) to separate branch and I'll create new PR which includes only form of Seo & Urls page. Everything else regarding Seo & urls page itself is good to go. 👍

Contributor

tomas862 commented Sep 18, 2018

Thanks for reviews @mickaelandrieu ! I moved the part with todos (it was a form part) to separate branch and I'll create new PR which includes only form of Seo & Urls page. Everything else regarding Seo & urls page itself is good to go. 👍

@sarjon

@tomas862 good job 👍 , a few minor comments from me.

Show outdated Hide outdated src/Adapter/Feature/MultiShopFeature.php Outdated
Show outdated Hide outdated src/Adapter/Feature/MultiShopFeature.php Outdated
/**
* Class RobotsTextFileGenerator is responsible for generating robots txt file.
*/
class RobotsTextFileGenerator

This comment has been minimized.

@sarjon

sarjon Sep 19, 2018

Member

you must add interface for it.

@sarjon

sarjon Sep 19, 2018

Member

you must add interface for it.

This comment has been minimized.

@mickaelandrieu

mickaelandrieu Sep 19, 2018

Contributor

agree, it will be done don't worry

@mickaelandrieu

mickaelandrieu Sep 19, 2018

Contributor

agree, it will be done don't worry

/**
* Class HtaccessFileGenerator is responsible for generating htaccess file with its default content.
*/
class HtaccessFileGenerator

This comment has been minimized.

@sarjon

sarjon Sep 19, 2018

Member

you must add an interface for it.

@sarjon

sarjon Sep 19, 2018

Member

you must add an interface for it.

*/
public function __construct(
Configuration $configuration,
HtaccessFileGenerator $htaccessFileGenerator,

This comment has been minimized.

@sarjon

sarjon Sep 19, 2018

Member

if you make an interface for it, you could move this class to Core. 👍

@sarjon

sarjon Sep 19, 2018

Member

if you make an interface for it, you could move this class to Core. 👍

Show outdated Hide outdated ...onfigure/ShopParameters/TrafficSeo/Meta/MetaSettingsFormDataProvider.php Outdated
Show outdated Hide outdated classes/Link.php Outdated
Show outdated Hide outdated classes/Link.php Outdated
Show outdated Hide outdated ...le/Form/Admin/Configure/ShopParameters/TrafficSeo/Meta/UrlSchemaType.php Outdated
_legacy_controller: AdminMeta
admin_meta_list_edit:
path: /edit/{metaId}

This comment has been minimized.

@sarjon

sarjon Sep 19, 2018

Member

i think its better to follow route naming like:
resources/{id}/action, so its like metas/{metaId}/edit, wdyt @matks ?

@sarjon

sarjon Sep 19, 2018

Member

i think its better to follow route naming like:
resources/{id}/action, so its like metas/{metaId}/edit, wdyt @matks ?

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

@mickaelandrieu

This comment has been minimized.

Show comment
Hide comment
@mickaelandrieu

mickaelandrieu Sep 20, 2018

Contributor

Thank you @tomas862 and everyone for review/QA!

Some comments will be addressed later

Contributor

mickaelandrieu commented Sep 20, 2018

Thank you @tomas862 and everyone for review/QA!

Some comments will be addressed later

@mickaelandrieu mickaelandrieu merged commit 4b4fab4 into PrestaShop:develop Sep 20, 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

@tomas862 tomas862 deleted the tomas862:migration/traffic_and_seo branch Sep 20, 2018

@marionf

This comment has been minimized.

Show comment
Hide comment
@marionf
Contributor

marionf commented Sep 24, 2018

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