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

[Cart][Behat] Logging customer's cart #4044

Conversation

Zales0123
Copy link
Member

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

Based on #4035. Again, please, don't review this until previous PR's will be merged.

@pjedrzejewski pjedrzejewski added the Enhancement Minor issues and PRs improving the current solutions (optimizations, typo fixes, etc.). label Feb 3, 2016
@michalmarcinkowski
Copy link
Contributor

@Zales0123 please rebase.

@Zales0123 Zales0123 force-pushed the apply-correct-taxes–based-on-customers-data branch 3 times, most recently from eb705b3 to 85045fe Compare February 9, 2016 14:38
@Zales0123
Copy link
Member Author

@michalmarcinkowski done ;) And awaiting review, of course.

@Zales0123 Zales0123 changed the title [WIP][Cart][Behat] Logging customer's cart [Cart][Behat] Logging customer's cart Feb 9, 2016
@@ -68,5 +68,6 @@ default:
taxCategoryRepository: "@sylius.repository.tax_category"
zoneRepository: "@sylius.repository.zone"

- Sylius\Behat\Context\Ui\UserContext
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's put : ~ at the end to be sure that context class will be interpreted as a key.

Copy link
Member Author

Choose a reason for hiding this comment

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

👍

@Zales0123 Zales0123 force-pushed the apply-correct-taxes–based-on-customers-data branch from 8ee66ea to 610b1a3 Compare February 9, 2016 15:25
@Zales0123 Zales0123 force-pushed the apply-correct-taxes–based-on-customers-data branch 3 times, most recently from 101ad65 to 23e0ef1 Compare February 10, 2016 12:07
@Zales0123 Zales0123 force-pushed the apply-correct-taxes–based-on-customers-data branch from 23e0ef1 to 362e97e Compare February 10, 2016 12:22
And product "PHP T-Shirt" belongs to "Clothes" tax category
And store ships everything for free
And store allows paying offline
And there is user "john@example.com" identified by "password123", with "Australia" as shipping country
Copy link
Member

Choose a reason for hiding this comment

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

Maybe we can move this to separate step:

And user/customer "john@example.com" has default shipping address to "Australia"

@pjedrzejewski
Copy link
Member

I did not review everything yet because I'd like to start a quick discussion:

  1. I think we are missing one scenario here, when someone is logged in before adding to the cart. The reason is we want to verify that cart is recalculated when someone signs in and has address, but we also want to verify that it is considered at the time of adding to the cart.
  2. More natural scenario (short version), maybe that's even separate feature:
Background:
    I am logged in customer
    And my default shipping address is to "Australia"

Scenario: Adding item to the cart and getting proper tax information
    When I add product ...
    Then ...

E-Mail and password are irrelevant when we are logged in already. What do you think? I know this is something we can add in separate PR but I wanted to start discussion already.

Btw. something I just realized, from scenarios it is perfectly clear what is going on and they are not coupled to UI just like we wanted, but from UI perspective we are checking the "Cart Summary View" right now. We need to remember there is "Checkout Summary View" at the end and we should ensure that correct amounts are displayed there as well. Something to think about as we progress.

@michalmarcinkowski
Copy link
Contributor

  1. 👍
  2. IMO this is the same feature, but we could probably add a concept of default user? E.g. in this scenario I don't care about it's email or password, so I would like to read that there is a customer that is logged in. WDYT? (let's do it in a separate PR)

For checkout the "Checkout Summary View" I would say we could have a separate scenarios for verifying that "Checkout Summary View" displays the same as "Cart Summary View". This way we will not need to check it on every single scenario.

@Zales0123
Copy link
Member Author

  1. What do you mean by "missing scenario, when someone is logged in before adding to cart"? Wasn't all previous scenarios about already logged in customer? 😄 For example in this feature we are checking proper taxes calculation on order for logged in "john@example.com" from "United Kingdom" :)
  2. 👍 for default customer, for cases like this one.

@michalmarcinkowski
Copy link
Contributor

@Zales0123

  1. I would say we should have this specific scenario (defined shipping address) here. In the feature you linked to there is no need to specify the customer address, it should be removed.

UPDATE

IMO in the feature you linked to we shouldn't even be logged in, as it doesn't change anything and we can validate that guest cart works properly :)

@Zales0123
Copy link
Member Author

@michalmarcinkowski Agreed 👍

@michalmarcinkowski
Copy link
Contributor

@Zales0123

These two steps are also irrelevant since we do not even start a checkout there.

@Zales0123
Copy link
Member Author

As we can see, first feature must have some childhood disease 😄
OK, so suma summarum:

  • create And I am logged in customer step with default customer
  • separate step And my default shipping address is to "Australia" from And I am logged in customer
  • move here step for logged in user with already defined shipping country
  • remove redundant steps from here

Am I missing sth?

@michalmarcinkowski
Copy link
Contributor

@Zales0123 👍

But let's merge this and do it all in a separate PRs. /cc @pjedrzejewski

pjedrzejewski pushed a commit that referenced this pull request Feb 10, 2016
…customers-data

[Cart][Behat] Logging customer's cart
@pjedrzejewski pjedrzejewski merged commit 17def2f into Sylius:master Feb 10, 2016
@pjedrzejewski
Copy link
Member

Agreed about separate PR. 👍 Thank you Mateusz!

@Zales0123 Zales0123 deleted the apply-correct-taxes–based-on-customers-data branch March 23, 2016 09:48
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.

5 participants