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

BO order creation page - part 2. Handle shipping block #15899

Merged
merged 24 commits into from Oct 17, 2019

Conversation

@zuk3975
Copy link
Contributor

zuk3975 commented Oct 10, 2019

Questions Answers
Branch? develop
Description? Handles shipping block rendering/updating in Create new order page of BO.
Type? refacto
Category? BO
BC breaks? no
Deprecations? no
Fixed ticket? part of #10583
How to test? Nothing to test until all page is not finished. Eases developers cooperation.

This change is Reviewable

@zuk3975 zuk3975 requested a review from PrestaShop/prestashop-core-developers as a code owner Oct 10, 2019
@matks matks added the migration label Oct 10, 2019
@zuk3975 zuk3975 force-pushed the zuk3975:m/orders/create-shipping branch from b69c063 to 6daec9d Oct 11, 2019
@zuk3975 zuk3975 changed the title [WIP] BO order creation page - part 2. Handle shipping block BO order creation page - part 2. Handle shipping block Oct 14, 2019
@zuk3975 zuk3975 changed the title BO order creation page - part 2. Handle shipping block [WIP] BO order creation page - part 2. Handle shipping block Oct 14, 2019
Copy link
Contributor

PierreRambaud left a comment

Reviewed 10 of 23 files at r1, 1 of 1 files at r2, 16 of 16 files at r3.
Reviewable status: all files reviewed, 10 unresolved discussions (waiting on @zuk3975)


admin-dev/themes/new-theme/js/pages/order/create/addresses-renderer.js, line 109 at r3 (raw file):

   */
  _showAddressesBlock() {
    $(createOrderPageMap.addressesBlock).removeClass('d-none');

Could we, one for all use a constant for d-none (or using mapFile), I don't want to be the one who will update all classes when this class will be deleted on bootstrap 5 😭


admin-dev/themes/new-theme/js/pages/order/create/cart-provider.js, line 64 at r3 (raw file):

      method: 'POST',
      data: {
        customer_id: customerId,

If you do this change, I think admin_carts_info and others need it?
Otherwise this is disturbing 😱


admin-dev/themes/new-theme/js/pages/order/create/create-order-page.js, line 222 at r3 (raw file):

   */
  _editDeliveryOption(event) {
    $.ajax(this.router.generate('admin_carts_edit_carrier', {cartId: this.data.cart_id}), {

You're also able to use $.post, $.get like in VueJs context :)


admin-dev/themes/new-theme/js/pages/order/create/create-order-page.js, line 227 at r3 (raw file):

        carrier_id: event.currentTarget.value,
      },
      dataType: 'json',

datatype is not mandatory. Intelligent Guess (xml, json, script, text, html).


admin-dev/themes/new-theme/js/pages/order/create/shipping-renderer.js, line 47 at r3 (raw file):

    if (emptyCart) {
      this._hideContainer();
    } else if (shipping !== null && shipping.length !== 0) {

you should add a check for undefined


src/Adapter/Cart/QueryHandler/GetCartDeliveryOptionsHandler.php, line 64 at r3 (raw file):


        //Check if there is any delivery options available for cart delivery address
        if (array_key_exists($deliveryAddressId, $deliveryOptionsByAddress)) {

Should it be !array_key_exists?


src/PrestaShopBundle/Resources/config/routing/admin/sell/orders/carts.yml, line 61 at r3 (raw file):

/{cartId}/free-shipping

/{cartId}/rules/free-shipping


src/PrestaShopBundle/Resources/config/routing/admin/sell/orders/carts.yml, line 64 at r3 (raw file):

  methods: [POST]
  defaults:
    _controller: PrestaShopBundle:Admin/Sell/Order/Cart:setFreeShipping

If we set a freeshipping for a cart, we can't remove it?


src/PrestaShopBundle/Resources/views/Admin/Sell/Order/Order/Blocks/Create/shipping.html.twig, line 60 at r3 (raw file):

        <div class="col-4">
        <span class="ps-switch">
          <input id="freeShipping_0" class="js-free-shipping-switch" name="free_shipping" value="0" type="radio">

What is the reason to camelCase ids where we use underscore everywhere? (And not only in this file)

Copy link
Contributor Author

zuk3975 left a comment

Reviewable status: 23 of 24 files reviewed, 10 unresolved discussions (waiting on @PierreRambaud and @zuk3975)


admin-dev/themes/new-theme/js/pages/order/create/addresses-renderer.js, line 109 at r3 (raw file):

Previously, PierreRambaud (GoT) wrote…

Could we, one for all use a constant for d-none (or using mapFile), I don't want to be the one who will update all classes when this class will be deleted on bootstrap 5 😭

@PierreRambaud ,
I was wondering if we could only have our css class doing similar thing, but without !important, so it could also work with jquery .show()/.hide();
It could be something simple like .not-displayable { display:none; } in themes.scss or somewhere common.


admin-dev/themes/new-theme/js/pages/order/create/cart-provider.js, line 64 at r3 (raw file):

Previously, PierreRambaud (GoT) wrote…

If you do this change, I think admin_carts_info and others need it?
Otherwise this is disturbing 😱

I change to snake case where its in post data. Others accept id in url.


admin-dev/themes/new-theme/js/pages/order/create/shipping-renderer.js, line 47 at r3 (raw file):

Previously, PierreRambaud (GoT) wrote…

you should add a check for undefined

Do I really need to do that? In successful scenario it wont ever be undefined. And if it is, then it have already failed elsewhere. In this case it would be like checking if my code already failed 😄


src/PrestaShopBundle/Resources/config/routing/admin/sell/orders/carts.yml, line 64 at r3 (raw file):

Previously, PierreRambaud (GoT) wrote…

If we set a freeshipping for a cart, we can't remove it?

yes we can. We set free shipping value. its a boolean. Are you offering to change the naming to something like changeFreeShipping or setFreeShippingValue? (it was editFreeShipping at first, but was weird too.)


src/PrestaShopBundle/Resources/views/Admin/Sell/Order/Order/Blocks/Create/shipping.html.twig, line 60 at r3 (raw file):

Previously, PierreRambaud (GoT) wrote…

What is the reason to camelCase ids where we use underscore everywhere? (And not only in this file)

I am continuing this PR from some unfinished Sarunas work, there was such ids so i just continued. If it really needs snake case, i can change it, but i guess it should better be done later, because this is closely coupled with some other pr's that works on order page.

Copy link
Contributor

PierreRambaud left a comment

Reviewed 3 of 5 files at r4.
Reviewable status: all files reviewed, 10 unresolved discussions (waiting on @PierreRambaud and @zuk3975)


src/PrestaShopBundle/Resources/config/routing/admin/sell/orders/carts.yml, line 64 at r3 (raw file):

Previously, zuk3975 (Julius Žukauskas) wrote…

yes we can. We set free shipping value. its a boolean. Are you offering to change the naming to something like changeFreeShipping or setFreeShippingValue? (it was editFreeShipping at first, but was weird too.)

If it's a boolean, why not useFreeShipping?


src/PrestaShopBundle/Resources/views/Admin/Sell/Order/Order/Blocks/Create/shipping.html.twig, line 60 at r3 (raw file):

Previously, zuk3975 (Julius Žukauskas) wrote…

I am continuing this PR from some unfinished Sarunas work, there was such ids so i just continued. If it really needs snake case, i can change it, but i guess it should better be done later, because this is closely coupled with some other pr's that works on order page.

Better to be consistent. I saw twig files with snake case 🤔

@zuk3975

This comment has been minimized.

Copy link
Contributor Author

zuk3975 commented Oct 15, 2019


src/PrestaShopBundle/Resources/config/routing/admin/sell/orders/carts.yml, line 64 at r3 (raw file):

Previously, PierreRambaud (GoT) wrote…

If it's a boolean, why not useFreeShipping?

is it suitable for controller action name? sounds weird. I mean the prefixed verb should describe the user action, but in this case it describes action for cart.

Copy link
Contributor

PierreRambaud left a comment

Reviewed 1 of 9 files at r5.
Reviewable status: 20 of 28 files reviewed, 11 unresolved discussions (waiting on @matks, @PierreRambaud, and @zuk3975)


admin-dev/themes/new-theme/js/pages/order/create/addresses-renderer.js, line 109 at r3 (raw file):

Previously, zuk3975 (Julius Žukauskas) wrote…

@PierreRambaud ,
I was wondering if we could only have our css class doing similar thing, but without !important, so it could also work with jquery .show()/.hide();
It could be something simple like .not-displayable { display:none; } in themes.scss or somewhere common.

It's overkill to add a new class, when our theme already have the d-none :/


admin-dev/themes/new-theme/js/pages/order/create/cart-editor.js, line 49 at r5 (raw file):

      // this._persistCartInfoData(response);
      EventEmitter.emit('cartAddressesChanged', cartInfo);
    });

Can we add events for these methods in complete and error
EventEmitter.emit('cartAddressesComplete', cartInfo);
EventEmitter.emit('cartAddressesFailed', cartInfo);

for example :)


admin-dev/themes/new-theme/js/pages/order/create/cart-provider.js, line 64 at r3 (raw file):

Previously, zuk3975 (Julius Žukauskas) wrote…

I change to snake case where its in post data. Others accept id in url.

Weird 🤔


admin-dev/themes/new-theme/js/pages/order/create/shipping-renderer.js, line 47 at r3 (raw file):

Previously, zuk3975 (Julius Žukauskas) wrote…

Do I really need to do that? In successful scenario it wont ever be undefined. And if it is, then it have already failed elsewhere. In this case it would be like checking if my code already failed 😄

If it's undefined, shipping.length will raise a js error


src/PrestaShopBundle/Resources/config/routing/admin/sell/orders/carts.yml, line 64 at r3 (raw file):

Previously, zuk3975 (Julius Žukauskas) wrote…

is it suitable for controller action name? sounds weird. I mean the prefixed verb should describe the user action, but in this case it describes action for cart.

Seen with @matks, no problem to use setFreeShipping ^^
Me right now:

Copy link
Contributor

PierreRambaud left a comment

Reviewed 6 of 9 files at r5, 9 of 9 files at r6.
Reviewable status: 33 of 34 files reviewed, 10 unresolved discussions (waiting on @PierreRambaud and @zuk3975)


admin-dev/themes/new-theme/js/pages/order/create/create-order-map.js, line 30 at r6 (raw file):

 */
export default {
  orderCreationContainer: '#order_creation_container',

Use dash instead of underscore, there is no reason to mix them.

@zuk3975 zuk3975 changed the title [WIP] BO order creation page - part 2. Handle shipping block BO order creation page - part 2. Handle shipping block Oct 16, 2019
@zuk3975 zuk3975 force-pushed the zuk3975:m/orders/create-shipping branch from 6cb4f5f to 95a45d3 Oct 16, 2019
/**
* @return string
*/
public function getCarrierDelay(): string

This comment has been minimized.

Copy link
@sarjon

sarjon Oct 16, 2019

Contributor

It's a delivery option, but it says getCarrier* everywhere. 😕

This comment has been minimized.

Copy link
@zuk3975

zuk3975 Oct 16, 2019

Author Contributor

It is what delivery option consist of. Basically it is a carrier when multishipping is gone. Are you suggesting to rename it? @matks, wdyt ? It is really a question on larger scope. After multishipping feature is gone, the delivery option is basically a selection of carrier id. (then somewhere deeper in code the delivery option json is built constisting of carrier_id and delivery_address_id. Should it still be named as delivery option, or just e.g. carrierOption ?

src/Core/Domain/Cart/QueryResult/CartInformation.php Outdated Show resolved Hide resolved
expose: true

admin_carts_set_free_shipping:
path: /{cartId}/rules/free-shipping

This comment has been minimized.

Copy link
@sarjon

sarjon Oct 16, 2019

Contributor

What's with the /rules/? 🤔 Is it part of convention?

This comment has been minimized.

Copy link
@zuk3975
@zuk3975

This comment has been minimized.

Copy link
Contributor Author

zuk3975 commented Oct 16, 2019


admin-dev/themes/new-theme/js/pages/order/create/cart-editor.js, line 49 at r5 (raw file):

Previously, PierreRambaud (GoT) wrote…

Can we add events for these methods in complete and error
EventEmitter.emit('cartAddressesComplete', cartInfo);
EventEmitter.emit('cartAddressesFailed', cartInfo);

for example :)

Hmm, but I don't have a clear vision of where I could use these events now. Maybe I could implement it only when/if needed?

@matks matks dismissed PierreRambaud’s stale review Oct 17, 2019

Changes to be done in another PR once this one is merged. I promise @PierreRambaud

@matks
matks approved these changes Oct 17, 2019
@matks

This comment has been minimized.

Copy link
Contributor

matks commented Oct 17, 2019

@PierreRambaud As this PR needs to move on in order not to slow the whole process, I take responsibility to validate it but also to address all your CR requests in another PR I'll do once this one is merged. You have my word, and you know where to find me if I try to trick you 😄

@matks matks removed the waiting for QA label Oct 17, 2019
@matks

This comment has been minimized.

Copy link
Contributor

matks commented Oct 17, 2019

Merging this PR as it was only a needed merge to be able to optimize the workload
QA and other reviews will happen on the other PRs to come 😄 that will be built on top of it

@matks matks merged commit 5d21798 into PrestaShop:develop Oct 17, 2019
2 of 3 checks passed
2 of 3 checks passed
code-review/reviewable 32 files, 12 discussions left (PierreRambaud, sarjon, zuk3975)
Details
PrettyCI Code formatting
Details
Travis CI - Pull Request Build Passed
Details
@matks matks added this to the 1.7.7.0 milestone Oct 17, 2019
@matks matks referenced this pull request Oct 30, 2019
6 of 33 tasks complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
6 participants
You can’t perform that action at this time.