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

[User] Account renewal feature scenarios #4175

Merged
merged 2 commits into from
Mar 9, 2016

Conversation

CoderMaggie
Copy link
Member

Q A
Bug fix? yes
New feature? no
BC breaks? no
Deprecations? no
Fixed tickets
License MIT

Based on: #4009

@CoderMaggie CoderMaggie force-pushed the account-renewal branch 6 times, most recently from a39224f to 3118008 Compare February 23, 2016 08:41
@CoderMaggie CoderMaggie changed the title [WIP] [User] Account renewal feature scenarios [User] Account renewal feature scenarios Feb 23, 2016
@pjedrzejewski
Copy link
Member

@TheMadeleine Please rebase, thank you!

And he had an order
But his account was deleted

@ui @javascript
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we need @javascript for this scenario?

@CoderMaggie CoderMaggie force-pushed the account-renewal branch 2 times, most recently from e423689 to e04c9d4 Compare February 23, 2016 10:40
@CoderMaggie
Copy link
Member Author

@pjedrzejewski @michalmarcinkowski corrections applied.

@@ -104,4 +106,21 @@ function it_checks_if_account_was_deleted(

$this->accountShouldBeDeleted();
}

function it_tries_to_register_new_account(
$registerPage
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should be in one line. Same as above.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

@@ -19,6 +19,7 @@
<parameter key="sylius.behat.context.setup.payment.class">Sylius\Behat\Context\Setup\PaymentContext</parameter>
<parameter key="sylius.behat.context.setup.security.class">Sylius\Behat\Context\Setup\SecurityContext</parameter>
<parameter key="sylius.behat.context.setup.user.class">Sylius\Behat\Context\Setup\UserContext</parameter>
<parameter key="sylius.behat.context.setup.order.class">Sylius\Behat\Context\Setup\OrderContext</parameter>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why did you add this parameter? It is not used in this file.

@michalmarcinkowski michalmarcinkowski added the Enhancement Minor issues and PRs improving the current solutions (optimizations, typo fixes, etc.). label Feb 25, 2016

if (null === $flashMessage) {
throw new \Exception('Account is not registered.');
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IMO this method should return boolean. return null !== $flashMessage and then in the context expect($this->registerPage->wasRegistrationSuccessful)->toBe(true) like here

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

@CoderMaggie CoderMaggie force-pushed the account-renewal branch 2 times, most recently from 5f5f1a4 to 295d293 Compare February 26, 2016 10:45
{
$registerPage->wasRegistrationSuccessful()->willReturn(false);

$this->shouldThrow('PhpSpec\Exception\Example\FailureException')->during('iShouldBeRegistered');
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FailureException::class

* @param SharedStorageInterface $sharedStorage
* @param UserRepositoryInterface $userRepository
* @param CustomerShowPage $customerShowPage
* @param LoginPage $loginPage
* @param RegisterPage $registerPage
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since we try to introduce new approach it should be type hinted with an interface. Also check this in spec of this class

pjedrzejewski pushed a commit that referenced this pull request Mar 9, 2016
[User] Account renewal feature scenarios
@pjedrzejewski pjedrzejewski merged commit 844ae84 into Sylius:master Mar 9, 2016
@pjedrzejewski
Copy link
Member

Thanks Magda! 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Enhancement Minor issues and PRs improving the current solutions (optimizations, typo fixes, etc.).
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants