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

Fixed product url preview #8438

Merged
merged 1 commit into from Oct 25, 2017

Conversation

Projects
None yet
4 participants
@mickaelandrieu
Contributor

mickaelandrieu commented Oct 24, 2017

Questions Answers
Branch? develop
Description? The preview button wasn't working
Type? bug fix
Category? BO
BC breaks? no
Deprecations? no
Fixed ticket? http://forge.prestashop.com/browse/BOOM-4112
How to test? When a product is enabled or disabled, click on "preview" button.

@PrestaShop/prestashop-product-team someone from QA team: would you mind to add one more integration test to ensure the preview link is working and will work forever please? ❤️


This change is Reviewable

@Quetzacoalt91

This comment has been minimized.

Show comment
Hide comment
@Quetzacoalt91

Quetzacoalt91 Oct 24, 2017

Member

We could have tested it with the functional tests ... If we didn't open a new tab. I'm afraid we could lose the focus on the tab we want.

Member

Quetzacoalt91 commented Oct 24, 2017

We could have tested it with the functional tests ... If we didn't open a new tab. I'm afraid we could lose the focus on the tab we want.

@LittleBigDev

This comment has been minimized.

Show comment
Hide comment
@LittleBigDev

LittleBigDev Oct 24, 2017

Contributor

:lgtm:


Reviewed 2 of 2 files at r1.
Review status: all files reviewed at latest revision, all discussions resolved.


Comments from Reviewable

Contributor

LittleBigDev commented Oct 24, 2017

:lgtm:


Reviewed 2 of 2 files at r1.
Review status: all files reviewed at latest revision, all discussions resolved.


Comments from Reviewable

@LittleBigDev

Thanks @mickaelandrieu . Looks good to me.

@mickaelandrieu

This comment has been minimized.

Show comment
Hide comment
@mickaelandrieu

mickaelandrieu Oct 24, 2017

Contributor

@Quetzacoalt91 I've discussed this point with @fatmaBouchekoua and I'll plan a meeting with @eternoendless asap about how we could improve the QA testsuite and never have to fix this behavior, again ;)

Contributor

mickaelandrieu commented Oct 24, 2017

@Quetzacoalt91 I've discussed this point with @fatmaBouchekoua and I'll plan a meeting with @eternoendless asap about how we could improve the QA testsuite and never have to fix this behavior, again ;)

@Quetzacoalt91 Quetzacoalt91 referenced this pull request Oct 25, 2017

Merged

Test preview button #58

@Quetzacoalt91

This comment has been minimized.

Show comment
Hide comment
@Quetzacoalt91

Quetzacoalt91 Oct 25, 2017

Member

Okay, but I already have an improvement in the repository PSFunctionalTests.

I relaunch the tests on your PR to check if everything is still okay.

Member

Quetzacoalt91 commented Oct 25, 2017

Okay, but I already have an improvement in the repository PSFunctionalTests.

I relaunch the tests on your PR to check if everything is still okay.

@marionf

This comment has been minimized.

Show comment
Hide comment
@marionf

marionf Oct 25, 2017

Contributor

Hello @mickaelandrieu
I have a strange behavior on this PR
First, it wasn't working I was in debug mode.
I tried to change for production mode and it was working.
I rechanged for debug mode and now it's working fine.

Contributor

marionf commented Oct 25, 2017

Hello @mickaelandrieu
I have a strange behavior on this PR
First, it wasn't working I was in debug mode.
I tried to change for production mode and it was working.
I rechanged for debug mode and now it's working fine.

@mickaelandrieu

This comment has been minimized.

Show comment
Hide comment
@mickaelandrieu

mickaelandrieu Oct 25, 2017

Contributor

@marionf this don't surprise me as it's a javascript change.. I don't know what is the best option to clear cache except the option "disable cache" from Google debug toolbar.

Anyway, the test added by the QA team fails so I guess this issue is not solved :)

Thanks for your reviews 👍

Contributor

mickaelandrieu commented Oct 25, 2017

@marionf this don't surprise me as it's a javascript change.. I don't know what is the best option to clear cache except the option "disable cache" from Google debug toolbar.

Anyway, the test added by the QA team fails so I guess this issue is not solved :)

Thanks for your reviews 👍

@Quetzacoalt91

This comment has been minimized.

Show comment
Hide comment
@Quetzacoalt91

Quetzacoalt91 Oct 25, 2017

Member

I guess we should test this PR with the branch test-preview-button of https://github.com/PrestaShop/PSFunctionalTests.

Merging the test in the master branch crashed every other PRs opened on PrestaShop

Member

Quetzacoalt91 commented Oct 25, 2017

I guess we should test this PR with the branch test-preview-button of https://github.com/PrestaShop/PSFunctionalTests.

Merging the test in the master branch crashed every other PRs opened on PrestaShop

@mickaelandrieu

This comment has been minimized.

Show comment
Hide comment
@mickaelandrieu

mickaelandrieu Oct 25, 2017

Contributor

@Quetzacoalt91 testing my PR with the test of preview merged (before your revert) works in local on my computer. Sad story.

To be clear about the situation, when clicking on the button:

  • At first creation:
    • in dev mode => there is a debug error page with the right link
    • in prod mode => we are redirected to the right page
  • After:
    • in both dev and prod modes, we are redirected to the right page

This is not the best, but it's not a regression /c @vincentbz.

So... can we consider this issue fixed? :)

Contributor

mickaelandrieu commented Oct 25, 2017

@Quetzacoalt91 testing my PR with the test of preview merged (before your revert) works in local on my computer. Sad story.

To be clear about the situation, when clicking on the button:

  • At first creation:
    • in dev mode => there is a debug error page with the right link
    • in prod mode => we are redirected to the right page
  • After:
    • in both dev and prod modes, we are redirected to the right page

This is not the best, but it's not a regression /c @vincentbz.

So... can we consider this issue fixed? :)

@Quetzacoalt91

This comment has been minimized.

Show comment
Hide comment
@Quetzacoalt91

Quetzacoalt91 Oct 25, 2017

Member

I'm good to merge the PR as soon as the travis check is green. I kept the PR on the tests opened in PrestaShop/PSFunctionalTests#60, as it will prevent the future regressions.

Member

Quetzacoalt91 commented Oct 25, 2017

I'm good to merge the PR as soon as the travis check is green. I kept the PR on the tests opened in PrestaShop/PSFunctionalTests#60, as it will prevent the future regressions.

@Quetzacoalt91 Quetzacoalt91 added this to the 1.7.3.0 milestone Oct 25, 2017

@Quetzacoalt91 Quetzacoalt91 merged commit 91d3c3a into PrestaShop:develop Oct 25, 2017

3 checks passed

codacy/pr Good work! A positive pull request.
Details
code-review/reviewable 2 files reviewed
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment