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 customer address books in order creation view | Add webpack #184

Open
wants to merge 22 commits into
base: main
Choose a base branch
from

Conversation

BartoszWojdalowicz
Copy link

PR include behat tests.

BartoszWojdalowicz and others added 22 commits January 18, 2022 10:48
Now created `Order` passed to email template to be able to render breakdown / user can see what he pays for
When dealing with virtual products or  ( when only one shipping method is enabled and channel skipping_shipping_step_allowed )
@Loocos
Copy link

Loocos commented Apr 12, 2023

It could be great to merge this feature

@@ -0,0 +1,8 @@
import 'semantic-ui-css/components/dropdown';
Copy link
Member

Choose a reason for hiding this comment

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

--- src/Resources/assets/admin/js/index.js
+++ src/Resources/private/admin/js/app.js

I'm not sure if it's part of new bundle notation, that allows this Resource/assets path, but it should be either Resources/private or /assets in the root if plugin's symfony version allows it

Choose a reason for hiding this comment

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

AFAIR it doesn't change anything as the paths are configured in the webpack.config.js. We can go with Resource/private (as we do in Sylius) and change it to the /assets once the plugin structure updated.

@@ -0,0 +1,4 @@
{
Copy link
Member

Choose a reason for hiding this comment

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

  1. Built assets shouldn't be included in git
  2. public/build shouldn't exists in plugin unless it's a test app

Copy link

@jakubtobiasz jakubtobiasz left a comment

Choose a reason for hiding this comment

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

  1. Rebase is needed
  2. /public and /src/Resources/public should be removed
  3. Including assets should be done with using template events
  4. The installation guide (at least) should be updated, due to added webpack

Comment on lines +50 to +52
"symfony/web-profiler-bundle": "^4.4 || ^5.4",
"symfony/webpack-encore-bundle": "^1.12",
"symfony/web-server-bundle": "^4.4 || ^5.4"

Choose a reason for hiding this comment

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

It should be ^5.4 || ^6.0

Comment on lines +19 to +32
Scenario: Creating an order with both addresses from address books
When I create a new order for "[email protected]" and channel "United States"
And I add "Stark Coat" to this order
And I see the address book shipping address select
And I see the address book billing address select
And I select first address in shipping address book with name "Jon Snow"
And I select first address in billing address book with name "Ned Stark"
And I select "Free" shipping method
And I select "Cash on Delivery" payment method
And I place and confirm this order
Then I should be notified that order has been successfully created
And this order shipping address should be "Jon Snow", "Frost Alley", "90210", "Ankh-Morpork", "United States"
And this order billing address should be "Ned Stark", "Banana Street", "90232", "New York", "United States"
And there should be one not paid nor shipped order with channel "United States" for "[email protected]" in the registry

Choose a reason for hiding this comment

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

Suggested change
Scenario: Creating an order with both addresses from address books
When I create a new order for "[email protected]" and channel "United States"
And I add "Stark Coat" to this order
And I see the address book shipping address select
And I see the address book billing address select
And I select first address in shipping address book with name "Jon Snow"
And I select first address in billing address book with name "Ned Stark"
And I select "Free" shipping method
And I select "Cash on Delivery" payment method
And I place and confirm this order
Then I should be notified that order has been successfully created
And this order shipping address should be "Jon Snow", "Frost Alley", "90210", "Ankh-Morpork", "United States"
And this order billing address should be "Ned Stark", "Banana Street", "90232", "New York", "United States"
And there should be one not paid nor shipped order with channel "United States" for "[email protected]" in the registry
Scenario: Creating an order with both addresses from address books
When I create a new order for "[email protected]" and channel "United States"
And I add "Stark Coat" to this order
And I select first address in shipping address book with name "Jon Snow"
And I select first address in billing address book with name "Ned Stark"
And I select "Free" shipping method
And I select "Cash on Delivery" payment method
And I place and confirm this order
Then I should be notified that order has been successfully created
And this order shipping address should be "Jon Snow", "Frost Alley", "90210", "Ankh-Morpork", "United States"
And this order billing address should be "Ned Stark", "Banana Street", "90232", "New York", "United States"
And there should be one not paid nor shipped order with channel "United States" for "[email protected]" in the registry

If we don't see the address book select field, we can't click it. So, in my opinion, these two steps seem redundant. Same below.

Comment on lines +20 to +21
And I should not see the address book shipping address select
And I should not see the address book billing address select

Choose a reason for hiding this comment

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

Not needed as well. If there's some kind of "wait" we should move it to the specify step.

@@ -0,0 +1,8 @@
import 'semantic-ui-css/components/dropdown';

Choose a reason for hiding this comment

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

AFAIR it doesn't change anything as the paths are configured in the webpack.config.js. We can go with Resource/private (as we do in Sylius) and change it to the /assets once the plugin structure updated.

Comment on lines +1 to +2
{{ encore_entry_script_tags('admin-entry', null, 'admin') }}
{{ encore_entry_script_tags('sylius-orderCreation-admin', null, 'orderCreation_admin') }}

Choose a reason for hiding this comment

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

It should be done with using template events:

  • sylius.admin.layout.stylesheets
  • sylius.admin.layout.javascripts
  • sylius.shop.layout.stylesheets
  • sylius.shop.layout.javascripts

In such case, we don't require additional steps while installing the plugin. It applies to all 4 modified files.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants