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.

Sign up for GitHub

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

Jump to bottom

22251 admin order email is now required 1 #24479

Merged
merged 40 commits into from
Mar 19, 2020
Merged

22251 admin order email is now required 1 #24479

merged 40 commits into from
Mar 19, 2020

Conversation

solwininfotech
Copy link
Contributor

Description (*)

Fixed Issues (if relevant)

  1. Admin Order - Email is Now Required - Magento 2.2.6 #22251: Admin Order - Email is Now Required - Magento 2.2.6

Manual testing scenarios (*)

  1. Sales -> Orders
  2. Create New Order
  3. Create New Customer
  4. Select Store View or if Single-Store Mode it brings you to the Order Information Page
  5. Email field is now required

Before Fix:
Customer Email is required in Admin Order Creation.
22251_before
After fixed Customer Email is not required in Admin Order Creation and once order is created customer email will be created automatically by system.
22251_after
22251_dynamic_email_creation

Contribution checklist (*)

  • Pull request has a meaningful description of its purpose
  • All commits are accompanied by meaningful commit messages
  • All new or changed code is covered with unit/integration tests (if applicable)
  • All automated tests passed successfully (all builds on Travis CI are green)

@m2-assistant
Copy link

m2-assistant bot commented Sep 6, 2019

Hi @solwininfotech. 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 give me test instance - deploy test instance based on PR changes
  • @magento give me 2.3-develop instance - deploy vanilla Magento instance

For more details, please, review the Magento Contributor Guide documentation.

@solwininfotech solwininfotech mentioned this pull request Sep 6, 2019
Closed
4 tasks
@solwininfotech
Copy link
Contributor Author

@magento-engcom-team give me test instance

@magento-engcom-team
Copy link
Contributor

Hi @solwininfotech. Thank you for your request. I'm working on Magento instance for you

@magento-engcom-team
Copy link
Contributor

Hi @solwininfotech, here is your new Magento instance.
Admin access: https://pr-24479.instances.magento-community.engineering/admin
Login: admin Password: 123123q

@solwininfotech
Copy link
Contributor Author

Hi,
I have tested the things with magneto instance and its working fine.
Thank you.

swnsma
swnsma requested changes Sep 6, 2019
View reviewed changes
Copy link
Contributor

@swnsma swnsma left a comment

Choose a reason for hiding this comment

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

Hi @solwininfotech,
Thank you for your contribution!
You are working with a really great improvement.

Could you please:

  • check my comments
  • check failed functional tests
  • check failed static tests
  • revert changes which are related to changing rights of files

app/code/Magento/Customer/etc/adminhtml/system.xml Outdated Show resolved Hide resolved
app/code/Magento/Customer/etc/adminhtml/system.xml Outdated Show resolved Hide resolved
app/code/Magento/Sales/Block/Adminhtml/Order/Create/Form/Account.php Outdated Show resolved Hide resolved
app/code/Magento/Sales/Model/AdminOrder/Create.php Outdated Show resolved Hide resolved
@ghost ghost assigned swnsma Sep 6, 2019
swnsma
swnsma requested changes Sep 21, 2019
View reviewed changes
Copy link
Contributor

@swnsma swnsma left a comment

Choose a reason for hiding this comment

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

Hi @solwininfotech,
Thank you for your changes!

Please discard these changes.
image

Please fix failed tests.

app/code/Magento/Customer/i18n/en_US.csv Outdated
@@ -1,23 +1,23 @@
"Sign Out","Sign Out"
Copy link
Contributor

Choose a reason for hiding this comment

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

Please discard these changes.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@swnsma
To discard these files I have run the following command and do git push, Is this the valid thing that I have done? I am worried because of in second last commit i had only one test fall and now its become to eight.

git rm --cached app/etc/NonComposerComponentRegistration.php
Please help

@swnsma
Copy link
Contributor

swnsma commented Sep 21, 2019

It was an issue with rights.
Please return files back. We need them.
image
Rights should be 0644.
Also, please change rights to 0644 for these files:
image

swnsma
swnsma previously approved these changes Feb 25, 2020
View reviewed changes
@magento-engcom-team
Copy link
Contributor

Hi @swnsma, thank you for the review.
ENGCOM-6208 has been created to process this Pull Request

@engcom-Delta
Copy link
Contributor

According comment new behavior Customer Email is not required in Admin Order Creation should be approved by Magento Architects team

@ghost ghost dismissed swnsma’s stale review February 26, 2020 08:04

Pull Request state was updated. Re-review required.

@swnsma swnsma self-requested a review March 5, 2020 17:39
@magento-engcom-team
Copy link
Contributor

Hi @swnsma, thank you for the review.
ENGCOM-6208 has been created to process this Pull Request

@sdzhepa
Copy link
Contributor

sdzhepa commented Mar 6, 2020

Hello @solwininfotech and @swnsma

Thank you for contribution and collaboration!

Let me remind that previously was an attempt to fix #22251 and was created #22648

But such changes were not approved by Magento Product Owner(PO). After couple of additional discussions with PO was found a compromise solution.
Please see this comment for more details and explanation: #22648 (comment)

@engcom-Delta , @engcom-Echo
could you please check is this fix meets requirements described: #22648 (comment) if not in should be "QA Failed" and "Progress: needs update"

@engcom-Delta
Copy link
Contributor

✔️ QA passed

@magento-engcom-team magento-engcom-team mentioned this pull request Mar 19, 2020
Closed
magento-engcom-team pushed a commit that referenced this pull request Mar 19, 2020
@slavvka
ENGCOM-6208: 22251 admin order email is now required 1 #24479
bef9ab7
@magento-engcom-team magento-engcom-team merged commit 79cdb54 into magento:2.4-develop Mar 19, 2020
@m2-assistant
Copy link

m2-assistant bot commented Mar 19, 2020

Hi @solwininfotech, 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.

@solwininfotech
Copy link
Contributor Author

Thank you @swnsma for your lots of help.
Thanks @engcom-Delta
Thanks @engcom-Echo
Thanks @sdzhepa

@sdzhepa sdzhepa mentioned this pull request May 9, 2022
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@swnsma swnsma swnsma approved these changes

@akaplya akaplya Awaiting requested review from akaplya

@YevSent YevSent Awaiting requested review from YevSent

@paliarush paliarush Awaiting requested review from paliarush

@buskamuza buskamuza Awaiting requested review from buskamuza

Assignees

@swnsma swnsma

@engcom-Delta engcom-Delta

@engcom-Echo engcom-Echo

Labels
Auto-Tests: Covered All changes in Pull Request is covered by auto-tests Award: bug fix Award: special achievement Component: Customer Component: Sales Progress: accept Release Line: 2.4
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.