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

Add Behat tests for Customer commands #14670

Merged
merged 2 commits into from Jul 25, 2019

Conversation

@sarjon
Copy link
Member

commented Jul 16, 2019

Questions Answers
Branch? develop
Description? Adds Behat tests for Setting private note about customer & setting required fields.
Type? improvement
Category? TE
BC breaks? no
Deprecations? no
Fixed ticket? Partially #14480
How to test? Build should be green.

This change is Reviewable

@sarjon sarjon requested a review from PrestaShop/prestashop-core-developers as a code owner Jul 16, 2019

@sarjon

This comment has been minimized.

Copy link
Member Author

commented Jul 16, 2019

@matks a few tests, if build is green I think you could merge it if no code changes are needed. :)

class CustomerFeatureContext extends AbstractDomainFeatureContext
{
/**
* @Given /^"(Partner offers)" is "(required|not required)"$/

This comment has been minimized.

Copy link
@matks

matks Jul 17, 2019

Contributor

The value Partner offers is hardcoded, which makes this step not reusable for other fields 🤔 ?

This comment has been minimized.

Copy link
@sarjon

sarjon Jul 18, 2019

Author Member

Not really, it's the only field that is available for customers, there might be more in future, so I left it like so.

@@ -102,6 +103,40 @@ public function setCurrentCustomer($customerName)
Context::getContext()->updateCustomer($this->customers[$customerName]);
}
/**
* @Given private note is not set about customer :reference

This comment has been minimized.

Copy link
@matks

matks Jul 17, 2019

Contributor
Suggested change
* @Given private note is not set about customer :reference
* @Given there is no private note registered for customer :reference

This comment has been minimized.

Copy link
@sarjon

sarjon Jul 18, 2019

Author Member

I'm afraid your wording suggestion is a bit wrong, there is not command for "register private note for customer" nor it is written like so in BO, it's more like "setting private note about customer" instead. 🤔

This comment has been minimized.

Copy link
@matks

matks Jul 25, 2019

Contributor

I'm just trying to remove the "set" word that is very generic, in order to find something more relatable or more pictureable 😅

Any other idea ?

This comment has been minimized.

Copy link
@sarjon

sarjon Jul 25, 2019

Author Member

Yes, "set", like "add", "edit" and other words is "generic", but is it bad? Even command is called SetPrivateNoteAboutCustomerCommand. 🤔

}
/**
* @Then customer :reference private note should be :privateNote

This comment has been minimized.

Copy link
@matks

matks Jul 17, 2019

Contributor
Suggested change
* @Then customer :reference private note should be :privateNote
* @Then customer :reference private note content should be :privateNote

Maybe we should use a Bheat pystring here in order to allow more complex notes to be specified ?

This comment has been minimized.

Copy link
@sarjon

sarjon Jul 18, 2019

Author Member

Not sure, note is a simple string.

Regarding your wording, do we really need to point out "content"? There is nothing else (header, footer & etc) about it but note. 🤔

This comment has been minimized.

Copy link
@matks

matks Jul 25, 2019

Contributor

Not sure, note is a simple string.
Then we can use a single-column-single-row Behat TableNode

* @Then customer :reference private note content should be :
| Hey this is a not I wrote for Sarunas ! |

Regarding your wording, do we really need to point out "content"? There is nothing else (header, footer & etc) about it but note. 🤔

Maybe it was overthinking 😺 thanks for pointing this out

}
/**
* @When I set :privateNote private note about customer :reference

This comment has been minimized.

Copy link
@matks

matks Jul 17, 2019

Contributor
Suggested change
* @When I set :privateNote private note about customer :reference
* @When I write a private note for customer :reference with the content :content

Let's aim for scenarios that sound, as much as possible, feature-oriented 😉

I know this is hard because the BO is basically a big CRUD. But we must try to avoid using generic CRUD-related words like "save", "set", "edit", "modify".

Example: when we migrate Customer Service page, instead of writing "I save a message for customer X" we can use "I answer to customer X message" 😉 this sentence carries the meaning, the goal, rather than the action.

This comment has been minimized.

Copy link
@sarjon

sarjon Jul 18, 2019

Author Member

words like "save", "set", "edit", "modify".

I did use "set". 😄

Actually, I've had discussion quite some time ago with Pablo about this private note and we settled with "set private note about customer". Using "save/set private note for customer" would be wrong, because it is not visible for customer itself, only for BO admins, that's why "about customer" was kept, as it is private note. This is very feature-oriented imo. :)

This comment has been minimized.

Copy link
@matks

matks Jul 25, 2019

Contributor

OK for the "about", what about the "write" then 😄 ?

This comment has been minimized.

Copy link
@sarjon

sarjon Jul 25, 2019

Author Member

Hmm, the use case is to set private note, the command says "set", why add new wording. 🤔

@@ -0,0 +1,12 @@
# ./vendor/bin/behat -c tests/Integration/Behaviour/behat.yml -s customer
@reset-database-before-feature

This comment has been minimized.

Copy link
@matks

matks Jul 17, 2019

Contributor

I'm not sure we should write 1 dedicated file for it 🤔 are we not going to create a lot of folders then ?

I understand you want to split the scenarios, but if a file contains only 1 scenario, it looks like too much ?

This comment has been minimized.

Copy link
@sarjon

sarjon Jul 18, 2019

Author Member

Yeah, I agree, but these are "feature" files for a reason, we can't put everything into customer management (which is basically CRUD feature) or some other feature file, do we only have 1 feature about Customers?

How can we improve that? 🤔

I see that Sylius, which heavily depends on Behat are splitting their features into a lot of different feature-files. One of examples https://github.com/Sylius/Sylius/tree/master/features/payment/managing_payments Some of them have 1 scenario, others more.

This comment has been minimized.

Copy link
@matks

matks Jul 25, 2019

Contributor

Mmmm

All right ☺️

Even if later we find we have written too many files, we can merge them because tests do not fall under the SemVer "no BC !" rule

This comment has been minimized.

Copy link
@sarjon

sarjon Jul 25, 2019

Author Member

Exactly. 👍

@sarjon

This comment has been minimized.

Copy link
Member Author

commented Jul 23, 2019

Tests needs to be rerun.

@PierreRambaud

This comment has been minimized.

Copy link
Contributor

commented Jul 23, 2019

@sarjon Done, waiting for CI.

@sarjon sarjon force-pushed the sarjon:customer-behat-tests branch from aed8e8c to fbded9f Jul 25, 2019

@matks

matks approved these changes Jul 25, 2019

@matks matks added this to the 1.7.7.0 milestone Jul 25, 2019

@matks

This comment has been minimized.

Copy link
Contributor

commented Jul 25, 2019

Thank you @sarjon

@matks matks merged commit 28854c2 into PrestaShop:develop Jul 25, 2019

2 checks passed

PrettyCI Code formatting
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@sarjon sarjon deleted the sarjon:customer-behat-tests branch Jul 25, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.