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 Shipping block of new Order View page #16065

Merged
merged 10 commits into from Nov 6, 2019

Conversation

@sarjon
Copy link
Contributor

sarjon commented Oct 22, 2019

Questions Answers
Branch? develop
Description? Migrates Shipping block to new Order View page.
Type? improvement
Category? BO
BC breaks? no
Deprecations? no
Fixed ticket? #15824
How to test? Shipping block should behave the same as in legacy.

This change is Reviewable

@sarjon sarjon requested a review from PrestaShop/prestashop-core-developers as a code owner Oct 22, 2019
@sarjon sarjon added this to the 1.7.7.0 milestone Oct 22, 2019
@sarjon

This comment has been minimized.

Copy link
Contributor Author

sarjon commented Oct 23, 2019

@LouiseBonnard both suggestions accepted, thanks! :)

Copy link
Contributor

jolelievre left a comment

Nice job @sarjon Just a couple questions to be sure and we're good

</div>
{% endif %}

<div class="form-group">

This comment has been minimized.

Copy link
@jolelievre

jolelievre Oct 24, 2019

Contributor

I guess you can't use ps.form_group_row inside a modal?

@sarjon sarjon force-pushed the sarjon:m/orders/carriers branch from 2fc6a96 to 9308f51 Oct 25, 2019
@sarjon

This comment has been minimized.

Copy link
Contributor Author

sarjon commented Oct 25, 2019

Rebased.

Copy link
Contributor

jolelievre left a comment

Thanks @sarjon

@Robin-Fischer-PS

This comment has been minimized.

Copy link

Robin-Fischer-PS commented Oct 29, 2019

Hi @sarjon !

I've two bugs on this PR :

  • When trying to edit a shipping line, there is an error, and changes are not saved

  • When editing a shipping line, the "Carrier" field should have the actual carrier selected by default. Instead, it's the first value in the dropdown that is selected (on the video, it should be "My carrier", but it's "Youpila" instead) :

You can see both errors on this screen record :
https://drive.google.com/file/d/1RaPssdHAn9rnapI2za0jLHl4lSp5i6BU/view?usp=sharing

Furthermore, I saw two bugs that were already present in Legacy. Would it be easy to fix for you ? (of course, it absolutely not mandatory, since these are no regression)

  • When a product has a specific carrier selected (in product -> Shipping), on the shipping block, it should not be possible to choose another carrier (it's actually possible).

  • When a carrier has max size limitation, on the shipping block, it should not be possible to choose this carrier when a product exceed this limit (it's actually possible).

If you need more info on these bugs, don't hesitate to ping me 😄

Thanks !

@sarjon

This comment has been minimized.

Copy link
Contributor Author

sarjon commented Oct 30, 2019

Hi @Robin-Fischer-PS ,

When trying to edit a shipping line, there is an error, and changes are not saved

I cannot reproduce it. 😕 Is there anything specific to this order? Maybe you need to rebuild assets?

When editing a shipping line, the "Carrier" field should have the actual carrier selected by default. Instead, it's the first value in the dropdown that is selected (on the video, it should be "My carrier", but it's "Youpila" instead) :

Fixed.

Furthermore, I saw two bugs that were already present in Legacy. Would it be easy to fix for you ?

I've checked those bugs, but it seems they are out of scope for this PR, I guess you could open an issue so they won't be forget. :)

@sarjon sarjon force-pushed the sarjon:m/orders/carriers branch from 9308f51 to 15f8877 Oct 30, 2019
@sarahdib

This comment has been minimized.

Copy link

sarahdib commented Oct 30, 2019

Hello @sarjon

Weird for the first issue I reproduce it. When I tried to edit the shipping line, there is an error, and changes are not saved

@matks

This comment has been minimized.

Copy link
Contributor

matks commented Oct 30, 2019

@sarahdib Just checked, I have the error too, it is "An error occurred while sending an email to the customer." . It seems that because SMTP is not enabled in our test shops, we cannot modify shipping informations for an order 😅 as PS attempts to notify the user by mail.

@sarjon sarjon force-pushed the sarjon:m/orders/carriers branch from f67d330 to 4148b56 Oct 31, 2019
@matks

This comment has been minimized.

Copy link
Contributor

matks commented Oct 31, 2019

@sarahdib You should have a more detailed error message in your logs (I guess you're in DEV environment, so this means the file var/logs/dev.log). It should be the line with a php.CRITICAL: Uncaught Exception:

@Robin-Fischer-PS

This comment has been minimized.

Copy link

Robin-Fischer-PS commented Oct 31, 2019

Hi @sarjon ! Yes, we tried with and without rebuilding assets. @matks I'm looking at the log.

@matks

This comment has been minimized.

Copy link
Contributor

matks commented Oct 31, 2019

It was a "carrier ID invalid" exception 🤔 I'm going to try to understand

@matks

This comment has been minimized.

Copy link
Contributor

matks commented Oct 31, 2019

@sarjon Looks like $updateOrderShippingForm is initialized withouth a current_order_carrier_id parameter, consequently UpdateOrderShippingDetailsHandler::handle() crashes when calling

$orderCarrier = new OrderCarrier($command->getCurrentOrderCarrierId());

because $command->getCurrentOrderCarrierId() is null

@sarjon

This comment has been minimized.

Copy link
Contributor Author

sarjon commented Oct 31, 2019

@matks does this happen for virtual order? Can you reproduce it on demo orders as well? If so, which one?

@matks

This comment has been minimized.

Copy link
Contributor

matks commented Oct 31, 2019

I see it for demo orders with IDs 4 and 5 🤔

@sarjon

This comment has been minimized.

Copy link
Contributor Author

sarjon commented Oct 31, 2019

I see it for demo orders with IDs 4 and 5

That is so weird. I reinstalled PS, rebuilt assets (make sure hard refresh browser) and still cannot reproduce this issue. 🙀

@matks

This comment has been minimized.

Copy link
Contributor

matks commented Oct 31, 2019

Indeed after a full reinstall + assets rebuilt I dont see the issue anymore.
@Robin-Fischer-PS can you check with

  • checkout the branch
  • rebuild assets
  • install the shop
@Robin-Fischer-PS

This comment has been minimized.

Copy link

Robin-Fischer-PS commented Oct 31, 2019

Hi again @sarjon , @matks .

So it seems that this bug does not appear when trying with a dev core near me, I think it's some kind of voodoo bug.

Anyway, it now works fine, I don't know why.

I've detected a new bug : When editing a tracking number, it does not appear on the block (but it's OK on Legacy page, so the edit works fine).

See video :
https://drive.google.com/file/d/1nbaM2aefZylc0WAvlt0ujTZY4PpQdOLp/view?usp=sharing

And @sarjon , it's been a pleasure to work with you, I wish you all the best for your next job 😄

@matks

This comment has been minimized.

Copy link
Contributor

matks commented Nov 6, 2019

@Robin-Fischer-PS OK, I checked the code. The tracking number was not displayed because no tracking URL was provided for the selected carrier.

I improved the behavior:

  • when a tracking number is available, but no tracking URL, I show the number
  • when both are available, I show the number + the link
@matks
matks approved these changes Nov 6, 2019
@Robin-Fischer-PS

This comment has been minimized.

Copy link

Robin-Fischer-PS commented Nov 6, 2019

Hi @matks , thanks for the fix and the small improvement ! It's ✔️ :)

@matks

This comment has been minimized.

Copy link
Contributor

matks commented Nov 6, 2019

Thank you @sarjon for the PR and @Robin-Fischer-PS for the validation

@matks matks merged commit cbc15a1 into PrestaShop:develop Nov 6, 2019
2 checks passed
2 checks passed
PrettyCI Code formatting
Details
Travis CI - Pull Request Build Passed
Details
@sarjon sarjon deleted the sarjon:m/orders/carriers branch Nov 6, 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.