Skip to content
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 Order message page #14970

Merged
merged 30 commits into from Oct 9, 2019

Conversation

@sarjon
Copy link
Contributor

sarjon commented Aug 2, 2019

Questions Answers
Branch? develop
Description? This PR migrates "Sell > Customer Service > Order Messages".
Type? refacto
Category? CO
BC breaks? yes, removes legacy controller
Deprecations? no
Fixed ticket? #10555
How to test? ⚠️ Build assets before testing ⚠️ Page should work the same as legacy.

This change is Reviewable

@sarjon sarjon requested a review from PrestaShop/prestashop-core-developers as a code owner Aug 2, 2019
@sarjon sarjon changed the title Migration of Order message page [WIP] Migration of Order message page Aug 2, 2019
@sarjon sarjon changed the title [WIP] Migration of Order message page Migration of Order message page Aug 5, 2019
@sarjon

This comment has been minimized.

Copy link
Contributor Author

sarjon commented Aug 5, 2019

Waiting for review.

/**
* @When I specify :propertyName :propertyValue in default language for order message :reference
*/
public function specifyPropertyInDefaultLanguage(string $propertyName, string $propertyValue, string $reference)

This comment has been minimized.

Copy link
@matks

matks Aug 16, 2019

Contributor

I'm not a fan 😐 of these functions because you "stack" property specifications until you perform the persistence call using addWithSpecifiedProperties. This can lead to debugging issues when addWithSpecifiedProperties is called too soon, or called by another function (without you aware of it) or called too late and leading to incomplete / invalid data set.

You really dont want to use the TableNode provided by Behat 😛 ?

    When I add order message "OM1" with specified properties for default language:
 | name       | Out of stock                       |
 | message | Products are out of stock |

This comment has been minimized.

Copy link
@matks

matks Aug 16, 2019

Contributor

Note : this is not a blocking item, I'm not going to bother you too much on behat tests. Adding integration tests is already one hundred times better than migrating pages without them 😛 so if you tell me you really feel like if it's not an issue, I'm fine 👍

This comment has been minimized.

Copy link
@sarjon

sarjon Aug 19, 2019

Author Contributor

Well, as I said before, for me it seems like more readable format. For example, imagine having this

| active | true |

instead of this

And I specify "Displayed" is "enabled"

But if you feel it's better to use tables, I can do that in next PRs. :)

@sarjon

This comment has been minimized.

Copy link
Contributor Author

sarjon commented Aug 21, 2019

@matks comments addressed. :)

@matks

This comment has been minimized.

Copy link
Contributor

matks commented Aug 21, 2019

Going for QA 💪

@sarahdib sarahdib self-assigned this Aug 22, 2019
@matks matks referenced this pull request Sep 9, 2019
1 of 28 tasks complete
@sarjon sarjon force-pushed the sarjon:m/order-message/form branch from 6ed4adf to 51c4124 Sep 30, 2019
@sarjon

This comment has been minimized.

Copy link
Contributor Author

sarjon commented Sep 30, 2019

Rebased & still waiting for QA.

@sarahdib

This comment has been minimized.

Copy link

sarahdib commented Oct 8, 2019

Hello @sarjon

There is a little problem with wording :
Capture d’écran 2019-10-08 à 15 18 12
Capture d’écran 2019-10-08 à 15 18 36

@sarjon sarjon force-pushed the sarjon:m/order-message/form branch from 51c4124 to 30f48e1 Oct 8, 2019
@sarjon

This comment has been minimized.

Copy link
Contributor Author

sarjon commented Oct 8, 2019

Sorry @sarahdib, that was copy/paste mistake. 😅 Should be fixed now. 👍

@sarahdib sarahdib added QA ✔️ and removed waiting for QA labels Oct 9, 2019
@sarahdib sarahdib added this to the 1.7.7.0 milestone Oct 9, 2019
@matks
matks approved these changes Oct 9, 2019
@matks

This comment has been minimized.

Copy link
Contributor

matks commented Oct 9, 2019

Thank you @sarjon

@matks matks merged commit 512d0d6 into PrestaShop:develop Oct 9, 2019
2 checks passed
2 checks passed
PrettyCI Code formatting
Details
Travis CI - Pull Request Build Passed
Details
@sarjon sarjon deleted the sarjon:m/order-message/form branch Oct 9, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.