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

Improve routing of PrestaShop application #9137

Merged
merged 12 commits into from Jun 19, 2018

Conversation

Projects
None yet
5 participants
@mickaelandrieu
Contributor

mickaelandrieu commented May 28, 2018

Questions Answers
Branch? develop
Description? Routing current organization has already been improved by @sarjon, let's go deeper for 1.7.5.
Type? improvement
Category? BO
BC breaks? no
Deprecations? no
How to test? All tests should pass as they don't rely on hardcoded URLs, but some of them have been changed to better respect the menu organization (mostly for the first migrated pages). As we use the router to generate URLs and not hardcoded values, this is not a break of API! Also, URLs from API part have not been changed (if we consider them as part of a REST API...).

Want an overview of application routing? => try php bin/console debug:router

admin
api


This change is Reviewable

@@ -33,7 +33,7 @@ export default new VueRouter({
mode: 'history',
base: (() => {
const hasIndex = /(index\.php)/.exec(window.location.href);
return `${window.data.baseUrl}${hasIndex ? '/index.php' : ''}/stock`;
return `${window.data.baseUrl}${hasIndex ? '/index.php' : ''}/sell/stocks`;

This comment has been minimized.

@mickaelandrieu

mickaelandrieu May 28, 2018

Contributor

note that I'm not proud of my fix neither to the current code, but fixing it "the right way" should be done in a specific pull request.

@mickaelandrieu

This comment has been minimized.

Contributor

mickaelandrieu commented May 28, 2018

ping @fatmaBouchekoua can you explain to me how to reproduce the failing test, I can create a product with a price, a reference a description and 2 pictures manually, so I don't really know what bug I have introduced here 😞

@PierreRambaud

This comment has been minimized.

Contributor

PierreRambaud commented May 29, 2018

@mickaelandrieu This selector #growls-default > .growl-notice > .growl-message:not(:empty) isn't found, it can be changed by .growl-message:not(:empty) and the test pass

#
#_shipping:
# resource: "shipping/*.yml"
# prefix: /shipping/

This comment has been minimized.

@PierreRambaud

PierreRambaud May 29, 2018

Contributor

Missing eof

This comment has been minimized.

@mickaelandrieu

mickaelandrieu May 30, 2018

Contributor

fixed, ty

This comment has been minimized.

@PierreRambaud

PierreRambaud Jun 7, 2018

Contributor

Hum, still missing but don't care

This comment has been minimized.

@mickaelandrieu

mickaelandrieu Jun 15, 2018

Contributor

fixed

@mickaelandrieu

This comment has been minimized.

Contributor

mickaelandrieu commented May 29, 2018

This selector #growls-default > .growl-notice > .growl-message:not(:empty) isn't found, it can be changed by .growl-message:not(:empty) and the test pass

Maybe... but why it worked before and not now? I'm not comfortable having to change the tests to make them pass :(

@PierreRambaud

This comment has been minimized.

Contributor

PierreRambaud commented May 29, 2018

Yeah, it's weird, I try this selector directly in chrome extension and it returns nothing :/

@PierreRambaud

This comment has been minimized.

Contributor

PierreRambaud commented Jun 7, 2018

A little update and merge please :D

@@ -0,0 +1,3 @@
_theme_catalog:
resource: "theme_catalog.yml"
prefix: /themes-catalog/

This comment has been minimized.

@PierreRambaud

PierreRambaud Jun 7, 2018

Contributor

Missing new line

This comment has been minimized.

@mickaelandrieu

mickaelandrieu Jun 7, 2018

Contributor

:/

missing CS validator configuration and build in travis.

This is the last time I fix a coding style issue, I don't have time for this /c @eternoendless

methods: [GET]
defaults:
_controller: 'PrestaShopBundle:Admin\Improve\Design\ThemeCatalog:index'
_legacy_controller: AdminThemesCatalog
_legacy_controller: AdminThemesCatalog

This comment has been minimized.

@PierreRambaud

PierreRambaud Jun 7, 2018

Contributor

Missing new line

@mickaelandrieu

This comment has been minimized.

Contributor

mickaelandrieu commented Jun 15, 2018

@eternoendless LGTM? the more we wait, the bigger this pull request will be ^^

@PierreRambaud

This comment has been minimized.

Contributor

PierreRambaud commented Jun 15, 2018

👍

@mickaelandrieu

This comment has been minimized.

Contributor

mickaelandrieu commented Jun 18, 2018

At this point I dunno why e2e are broken and I can't reproduce it :/

@mickaelandrieu

This comment has been minimized.

Contributor

mickaelandrieu commented Jun 18, 2018

@mickaelandrieu This selector #growls-default > .growl-notice > .growl-message:not(:empty) isn't found, it can be changed by .growl-message:not(:empty) and the test pass

this is still the same issue :)

@PierreRambaud

This comment has been minimized.

Contributor

PierreRambaud commented Jun 18, 2018

@mickaelandrieu No prob mate, I will have a check on tomorrow if it's ok for you =)

@Quetzacoalt91

This comment has been minimized.

Member

Quetzacoalt91 commented Jun 19, 2018

We need a rebase before merge, some other PRs have been included here.

@mickaelandrieu

This comment has been minimized.

Contributor

mickaelandrieu commented Jun 19, 2018

@PierreRambaud My HERO <3

@PierreRambaud

This comment has been minimized.

Contributor

PierreRambaud commented Jun 19, 2018

I just update assets after pull upstream develop branch, I don't understand why there is #9198 because it has already been merged ><

@PierreRambaud

This comment has been minimized.

Contributor

PierreRambaud commented Jun 19, 2018

haha, I use rebase commands, that's why there are some commits related to develop branch :)

@Quetzacoalt91 Quetzacoalt91 merged commit cb4a0cd into PrestaShop:develop Jun 19, 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
@Quetzacoalt91

This comment has been minimized.

Member

Quetzacoalt91 commented Jun 19, 2018

@mickaelandrieu mickaelandrieu deleted the mickaelandrieu:improve/routing branch Jun 19, 2018

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