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

Addresses changes not impacted when creating an order from another order #11200

Merged
merged 3 commits into from Dec 14, 2018

Conversation

Projects
None yet
6 participants
@jf-viguier
Copy link
Contributor

jf-viguier commented Oct 30, 2018

Questions Answers
Branch? develop
Description? In backoffice, on order X, When you change delivery or invoice address, if you create an order Y from order X, the addresses modification are not impacted. The cart associated with the order has to be changed.
Type? bug fix
Category? BO
BC breaks? Does it break backward compatibility? no
Deprecations? Does it deprecate an existing feature? /no
Fixed ticket? Fixes #11199 cf #11199
How to test? Please indicate how to best verify that this PR is correct.

This change is Reviewable

Addresses changes not impacted when creating an order from another order
In backoffice, on order X, When you change delivery or invoice address, if you create an order Y from order X, the addresses modification are not impacted. The cart associated with the order has to be changed.

To Reproduce
Steps to reproduce the behavior:

Go to an order
Change delivery or invoice address
Create a new order based on previous order
See that the suggested addresses are wrong
@mickaelandrieu

This comment has been minimized.

Copy link
Contributor

mickaelandrieu commented Nov 21, 2018

Hello @jf-viguier,

You need to fix the coding styles:

Loaded config default from "/home/travis/build/PrestaShop/PrestaShop/.php_cs.dist".

  1. /home/travis/build/PrestaShop/PrestaShop/controllers/admin/AdminAddressesController.php
    Checked all files in 427.791 seconds, 41.250 MB memory used
    SYNTAX TESTS FAILED

To do it, execute the following command at the root of your shop:

./vendor/bin/php-cs-fixer fix

And commit the results 👍

Thanks!

@jf-viguier

This comment has been minimized.

Copy link
Contributor

jf-viguier commented Nov 21, 2018

I've tried php-cs-fixer fix but it hasn't changed anything. Can you tell me where is the bug ?

@jf-viguier

This comment has been minimized.

Copy link
Contributor

jf-viguier commented Nov 27, 2018

I don't know how to fix the problem, can someone help ?

@matks

This comment has been minimized.

Copy link
Contributor

matks commented Nov 27, 2018

Hi @jf-viguier, can you tell us the output of the command ./vendor/bin/php-cs-fixer fix from your command-line ?

Else, the fix that php-cs-fixer is supposed to do is the following (blank space):

-                if ($address_type=='invoice') {
-                    $cart->id_address_invoice = (int)$this->object->id;

=>

+                if ($address_type == 'invoice') {
+                    $cart->id_address_invoice = (int) $this->object->id;

and

-                    $cart->id_address_delivery = (int)$this->object->id;

=>

+                    $cart->id_address_delivery = (int) $this->object->id;
                 }
@jf-viguier

This comment has been minimized.

Copy link
Contributor

jf-viguier commented Dec 3, 2018

I've commited the changes.
I can't find ./vendor/bin/php-cs-fixer in Prestashop zip nor in github

@matks

This comment has been minimized.

Copy link
Contributor

matks commented Dec 4, 2018

Hi @jf-viguier

Running php-cs-fixer

In order to get the ./vendor/bin/php-cs-fixer binary you need to install Composer dependencies by running command composer install.

This requires you to have installed the Composer binary either globally on your computer or as a stand-alone binary.

Example of how to get Composer binary as stand-alone executable binary:

$ curl -s https://getcomposer.org/installer | php

This will provide you the composer.phar binary in your current directory. Then move it into root folder of Prestashop and run $ php composer.phar install to install PrestaShop composer dependencies, which include php-cs-fixer 😉

Current PR state this does not pass Travis lint check because of a bad empty line (this is my git diff after running php-cs-fixer on your branch) 😭 :

                 //update order shipping cost
                 $order = new Order($id_order);
                 $order->refreshShippingCost();
-
+
                 // update cart
                 $cart = Cart::getCartByOrderId($id_order);
                 if ($address_type == 'invoice') {

Question about submitted code

I can fix it however I have one thing that worries me in the PR:
https://github.com/PrestaShop/PrestaShop/pull/11200/files#diff-af7fb53fcd275a04ffc615dd95cce59bR423
What if it returns false, as the Cart cannot be found ?

@jf-viguier

This comment has been minimized.

Copy link
Contributor

jf-viguier commented Dec 4, 2018

ok, thanks for your help, I've added a cart check and finally figured out how to launch cs fixer.

@matks matks added this to the 1.7.6.0 milestone Dec 4, 2018

@matks

matks approved these changes Dec 4, 2018

Copy link
Contributor

matks left a comment

Bravo @jf-viguier 😄 this pull request goes into QA step

@matks matks added the waiting for QA label Dec 4, 2018

@marionf marionf self-assigned this Dec 4, 2018

@marionf marionf added QA ✔️ and removed waiting for QA labels Dec 12, 2018

@marionf marionf removed their assignment Dec 12, 2018

@Quetzacoalt91 Quetzacoalt91 merged commit 6c313d2 into PrestaShop:develop Dec 14, 2018

2 checks passed

Codacy/PR Quality Review Up to standards. A positive pull request.
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@Quetzacoalt91

This comment has been minimized.

Copy link
Member

Quetzacoalt91 commented Dec 14, 2018

Thank you @jf-viguier

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment