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

Migrates Orders list - part 1 #13988

Merged
merged 46 commits into from Aug 28, 2019

Conversation

@sarjon
Copy link
Member

commented May 27, 2019

Questions Answers
Branch? develop
Description? Migrates orders list.
Type? refacto
Category? CO
BC breaks? no
Deprecations? no
Fixed ticket? Migrate "Orders > Orders" page #10582
How to test? Should work the same as legacy list. ⚠️ Rebuild assets before testing.

This change is Reviewable

$templateVars['{followup}'] = str_replace('@', $order->shipping_number, $carrier->url);
}
if ($history->addWithemail(true, $templateVars)) {

This comment has been minimized.

Copy link
@sarjon

sarjon May 30, 2019

Author Member

@matks this one is very tricky, $history->addWithemail()can return false in two cases:

  1. When OrderHistory was not created (which is OK)
  2. When OrderHistory is created, but email was not sent to customer

I could workaround this by splitting this method into:

if (!$history->add()) {
    // throw saving order exception
}

if (!$history->sendEmail()) {
    // throw email sending exception
}

Unfortunately, it does not solve issue completely. In legacy, if $history->addWithemail() fails, it continues to loop through other orders attempting to change their status and in the end if shows list of errors:

Screenshot (1)

I'm wondering how should we keep the same behavior when using command to change order status? Should we keep track of errors and throw exception with multiple errors? Wdyt?

This comment has been minimized.

Copy link
@matks

matks May 31, 2019

Contributor

The question can be converted in a more global matter: "When performing one bulk action, when one of the actions fail, should we 1) stop there and display the error message to user or should we 2) skip it, attempt to perform the action on the remaining items, and at the end display a list of the failed usecases ?"

So far we have followed the strategy 1) and I like this because one error might be a hint of something wrong happening, so it's good to wait for user to re-try to make sure the bulk action was not triggered by mistake.

However the decision is a Product Choice so ping @colinegin @marionf

This comment has been minimized.

Copy link
@sarjon

sarjon May 31, 2019

Author Member

Should we consider email sending failure an error? This happens pretty often, how do we inform user about it?

This comment has been minimized.

Copy link
@colinegin

colinegin Jun 5, 2019

Collaborator

@MatShir this is related to orders listing, so I let you decide with @marionf ! ;)

This comment has been minimized.

Copy link
@marionf

marionf Jun 11, 2019

Contributor

@matks I think it's better to skip it and perform action in the remaining items, and display the list of the failed usecases at the end
This way if some of remaining items are not failing, the action is done for these items

This comment has been minimized.

Copy link
@matks

matks Jun 12, 2019

Contributor

So it's decided 😉ping @sarjon
And I'll create an issue to apply this strategy to all pages

This comment has been minimized.

Copy link
@matks
'customer' => 'customer',
'osname' => 'osl.name',
'date_add' => 'o.`date_add`',
];

This comment has been minimized.

Copy link
@sarjon

sarjon May 30, 2019

Author Member

Same issue with sorting.

@sarjon sarjon changed the title [WIP] Migrates Orders list Migrates Orders list Jun 3, 2019
'closable': true,
'actions': [{
'type': 'button',
'label': 'Update Order Status'|trans({}, 'Admin.Orderscustomers.Feature'),

This comment has been minimized.

Copy link
@LouiseBonnard

LouiseBonnard Jun 5, 2019

Contributor

What about Update status instead? Do you agree that this will introduce a new behavior since it won't be necessary to go inside an order page to update its status?

This comment has been minimized.

Copy link
@sarjon

sarjon Jun 5, 2019

Author Member

I've just used the same wording as in legacy, if you think it should be changed, it's 👍 for me.

This comment has been minimized.

Copy link
@LouiseBonnard

LouiseBonnard Jun 5, 2019

Contributor

@sarjon, thanks for your quick reply! Am I right about the behavior or is it displayed in another context?

This comment has been minimized.

Copy link
@sarjon

sarjon Jun 5, 2019

Author Member

It's a legacy page, but the context is the same (change order status in bulk action) in new page as well, except it's displayed in modal. https://prnt.sc/nxujkw

This comment has been minimized.

Copy link
@LouiseBonnard

LouiseBonnard Jun 5, 2019

Contributor

Alright. Let's write Update status in this case, localized in Admin.Orderscustomers.Feature as well, please. :-)

This comment has been minimized.

Copy link
@sarjon

sarjon Jun 17, 2019

Author Member

Done. 👍

@marionf marionf removed the waiting for PM label Jun 11, 2019
@sarjon sarjon marked this pull request as ready for review Jun 17, 2019
@sarjon sarjon requested a review from PrestaShop/prestashop-core-developers as a code owner Jun 17, 2019
@matks matks referenced this pull request Jul 4, 2019
4 of 43 tasks complete
Copy link
Contributor

left a comment

Review done 😄 almost nothing to change, close to perfect 👏

@sarjon

This comment has been minimized.

Copy link
Member Author

commented Jul 15, 2019

Review comments addressed.

@matks

This comment has been minimized.

Copy link
Contributor

commented Jul 16, 2019

@sarjon Approved can you rebase, then we got for QA ?

@sarjon sarjon force-pushed the sarjon:m/orders/list branch from 791c37b to 277bbb9 Jul 16, 2019
@matks

This comment has been minimized.

Copy link
Contributor

commented Aug 12, 2019

Let's do this, Travis !

@sarjon

This comment has been minimized.

Copy link
Member Author

commented Aug 12, 2019

Still failing. 😭

@matks

This comment has been minimized.

Copy link
Contributor

commented Aug 12, 2019

Again ...

@sarahdib

This comment has been minimized.

Copy link

commented Aug 20, 2019

Hello @sarjon

  • Missing column using multistore
    When I'm using multistore the column store is display in legacy but not in migrated page.
    image
    image

  • The view action is not working. The action display orders list instead of the order (maybe it's not finish yet)

  • The create order button is missing

image
image

  • The listing is not correct in multishop. If I'm in the BO of shop 2 I do not have to see the order of shop 1
    image
    image

  • If I search all ID who contain "1" I found only the order 1 instead of 1, 10, 11
    image

image

@sarjon

This comment has been minimized.

Copy link
Member Author

commented Aug 21, 2019

Missing column using multistore

Fixed.

The view action is not working. The action display orders list instead of the order (maybe it's not finish yet)

It's part of another PR.

The create order button is missing

It's part of another PR.

The listing is not correct in multishop. If I'm in the BO of shop 2 I do not have to see the order of shop 1

Fixed.

If I search all ID who contain "1" I found only the order 1 instead of 1, 10, 11

Fixed

The search by date is not working

Not related to this PR, it's a global issue being fixed another PR.

@sarahdib let me know if you have any other issues. :)

@sarahdib

This comment has been minimized.

Copy link

commented Aug 23, 2019

Hello @sarjon

Thanks for the fixes and precisions :)

I found a new bug

@sarjon

This comment has been minimized.

Copy link
Member Author

commented Aug 23, 2019

Regarding

When I select all or some order and try to use bulk action to change the order status there is a 500 error (see the video)

and

When I try to download the invoice there is an error it's ok for the delivery slip

It works well for me. 😕 What's you PHP version? Is it on some environment that I could access?

@sarjon

This comment has been minimized.

Copy link
Member Author

commented Aug 26, 2019

ping @sarahdib

@matks
matks approved these changes Aug 26, 2019
@sarahdib

This comment has been minimized.

Copy link

commented Aug 26, 2019

@sarjon

Sorry I'm in the middle of the validation process for 1.7.6.1.

I'm using php 7.1.19 google chrome browser.

@sarahdib

This comment has been minimized.

Copy link

commented Aug 27, 2019

@sarjon I don't know why the problem happen last time.
I re-install prestashop on my local and hosting test and it's ok for the invoice and the bulk action. Weird. I try several combinaison of multishop and it's ok now :)

filter by new client -> OK
filter by id -> OK

Thank you @sarjon

@sarjon

This comment has been minimized.

Copy link
Member Author

commented Aug 27, 2019

I re-install prestashop

In some cases that's all you need. :)

@matks

This comment has been minimized.

Copy link
Contributor

commented Aug 28, 2019

Thank you @sarjon and @sarahdib

@matks matks merged commit d15abd5 into PrestaShop:develop Aug 28, 2019
2 checks passed
2 checks passed
PrettyCI Code formatting
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@sarjon sarjon deleted the sarjon:m/orders/list branch Aug 28, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.