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

2.2.6 Use batches and direct queries to fix sales address upgrade #19098

Merged
merged 8 commits into from
Mar 14, 2019
Merged

2.2.6 Use batches and direct queries to fix sales address upgrade #19098

merged 8 commits into from
Mar 14, 2019

Conversation

rikwillems
Copy link
Contributor

Description (*)

Magento Commerce compatible fix for sales address upgrade script.

Fixed Issues (if relevant)

  1. [update] enhance performance on large catalog #16570: enhance performance on large catalog
    (comment)
  2. 2.2.6 Use batches and direct queries to fix sales address upgrade #18945

@magento-engcom-team
Copy link
Contributor

Hi @rikwillems. Thank you for your contribution
Here is some useful tips how you can test your changes using Magento test environment.
Add the comment under your pull request to deploy test or vanilla Magento instance:

  • @magento-engcom-team give me test instance - deploy test instance based on PR changes
  • @magento-engcom-team give me 2.2-develop instance - deploy vanilla Magento instance

For more details, please, review the Magento Contributor Assistant documentation

@ihor-sviziev ihor-sviziev self-assigned this Nov 7, 2018
@ihor-sviziev ihor-sviziev changed the title Combined commit 2.2.6 Use batches and direct queries to fix sales address upgrade Nov 7, 2018
@ihor-sviziev
Copy link
Contributor

ihor-sviziev commented Nov 7, 2018

Also looks like CLA showing as "not signed", so probably we have some issue there. Could you contact me in Slack to get it resolved faster?

@orlangur
Copy link
Contributor

orlangur commented Nov 7, 2018

@ihor-sviziev is this not relevant or already delivered to 2.3?

@ihor-sviziev
Copy link
Contributor

ihor-sviziev commented Nov 7, 2018

@magento-cicd2
Copy link
Contributor

magento-cicd2 commented Nov 8, 2018

CLA assistant check
All committers have signed the CLA.

@rikwillems
Copy link
Contributor Author

This CLA is giving me a headache. I tried a new PR fix this as I cannot modify existing commits with new emailaddress. #19114

@orlangur
Copy link
Contributor

orlangur commented Nov 8, 2018

@rikwillems,

cannot modify existing commits with new email address

git reset --soft HEAD^
git commit -m'Message'
git force push

should do the trick for arbitrary amount of last commits.

@ihor-sviziev
Copy link
Contributor

ihor-sviziev commented Nov 21, 2018

Hi @rikwillems,
Could you update your PR by squashing all commits into single one and force push, as @orlangur showed above?

Also please create PR to 2.3-develop branch, as we should accept it at first according to contributor guide

@rikwillems
Copy link
Contributor Author

@ihor-sviziev I tried this now. Please see it that works.

@ihor-sviziev
Copy link
Contributor

ihor-sviziev commented Nov 21, 2018

Hi @rikwillems,
Looks like you pushed without force option, as result - it merged changes.
Use git push origin <your branch> -f

@rikwillems
Copy link
Contributor Author

@ihor-sviziev this is quote a pain. Not sure why it is not working out. Tried it again.

@ihor-sviziev
Copy link
Contributor

@rikwillems, now you force-pushed, but pushed the same commits as before, but we want to have one squashed commit from you.
In order to verify if you did everything is ok - on the commits section all commit authors should be clickable, now we have two commits from email not linked to your github account
https://github.com/magento/magento2/pull/19098/commits

@rikwillems
Copy link
Contributor Author

@ihor-sviziev this should do it...

Copy link
Contributor

@ihor-sviziev ihor-sviziev left a comment

Choose a reason for hiding this comment

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

Issues with commit author is gone, that's good 👍
One small thing still there. Please check it.
Also please create PR to 2.3-develop branch, because it should be merged at first

app/code/Magento/Sales/Setup/UpgradeData.php Outdated Show resolved Hide resolved
@ihor-sviziev
Copy link
Contributor

Hi @rikwillems,
Are you interested in finalizing this PR?

@magento-engcom-team
Copy link
Contributor

Hi @ihor-sviziev, thank you for the review.
ENGCOM-4344 has been created to process this Pull Request

@magento-engcom-team
Copy link
Contributor

@rikwillems thank you for contributing. Please accept Community Contributors team invitation here to gain extended permissions for this repository.

@p-bystritsky
Copy link
Contributor

@rikwillems @ihor-sviziev there is not such column as "orderAddressId" in "sales_order_address" table, used in 'where' clause in https://github.com/magento/magento2/pull/19098/files#diff-0815805b2d40fcf0d22ebd639e0d73c4R247. Which will lead to an error during update.

'quote_address_id' => $quoteAddresses[$orderAddress['quote_id']]['address_id'] ?? null,
];
$where = [
'orderAddressId' => $orderAddress['entity_id']
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
'orderAddressId' => $orderAddress['entity_id']
'entity_id' => $orderAddress['entity_id']

@magento-engcom-team
Copy link
Contributor

Hi @ihor-sviziev, thank you for the review.
ENGCOM-4344 has been created to process this Pull Request

@magento-engcom-team
Copy link
Contributor

Hi @ihor-sviziev, thank you for the review.
ENGCOM-4344 has been created to process this Pull Request

@p-bystritsky p-bystritsky self-assigned this Mar 13, 2019
@magento-engcom-team magento-engcom-team merged commit df36443 into magento:2.2-develop Mar 14, 2019
@ghost
Copy link

ghost commented Mar 14, 2019

Hi @rikwillems, thank you for your contribution!
Please, complete Contribution Survey, it will take less than a minute.
Your feedback will help us to improve contribution process.

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

Successfully merging this pull request may close these issues.

7 participants