Skip to content

Issues 13769. Fix wrong info about sent email in order sender. #13878

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

Conversation

pawcioma
Copy link
Contributor

@pawcioma pawcioma commented Feb 28, 2018

Description

Fixes wrong info about sent email in Order Sender. Order sender when send method got exception then checkAndSend should return false.

Fixed Issues (if relevant)

  1. Order Email Sender #13769: Order Email Sender

Manual testing scenarios

  1. If we want check issue we should have wrong config sendmail in server.
  2. We should place order.
  3. We had wrong configuration in sendmail, so email not sent.
  4. In database in sales_order we should have email_send and send_email as false.

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)

@magento-cicd2
Copy link
Contributor

magento-cicd2 commented Feb 28, 2018

CLA assistant check
All committers have signed the CLA.

@pawcioma pawcioma force-pushed the issues-13769-info-about-sent-email branch from bf1dce3 to 1c7e3a1 Compare February 28, 2018 08:33
@sidolov sidolov self-assigned this Feb 28, 2018
@sidolov sidolov self-requested a review March 1, 2018 08:06
@magento-engcom-team magento-engcom-team added this to the March 2018 milestone Mar 1, 2018
@magento-engcom-team
Copy link
Contributor

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

@magento-engcom-team
Copy link
Contributor

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

@TomashKhamlai
Copy link
Contributor

TomashKhamlai commented Mar 5, 2018

Hello @pawcioma!
I tried to check if the issue is fixed and it looks like not. Considering that I have no rich experience with Sendmail I thought that it is a good idea to acquire some details from you. I tried this sequence of steps:

  1. Uncomment any MAILER directive, for example, MAILER(local')dnland place anyFEATUREdirective below in config for exampleFEATURE(always_add_domain')dnl Why this should work
  2. Create customer and place an order as an authorized customer
  3. Check your inbox there shouldn't be sales notification
  4. Check the database

✅ Expected result:

  • before fixing
    mysql -uroot -p123123q -e"select entity_id,state,status,customer_email,email_sent,send_email from develop.sales_order;"
mysql: [Warning] Using a password on the command line interface can be insecure.
+-----------+-------+---------+-------------------+------------+------------+
| entity_id | state | status  | customer_email    | email_sent | send_email |
+-----------+-------+---------+-------------------+------------+------------+
|         1 | new   | pending | [email protected] |          1 |          1 |
+-----------+-------+---------+-------------------+------------+------------+
  • after fixing
    mysql -uroot -p123123q -e"select entity_id,state,status,customer_email,email_sent,send_email from pr.sales_order;"
mysql: [Warning] Using a password on the command line interface can be insecure.
+-----------+-------+---------+--------------------------+------------+------------+
| entity_id | state | status  | customer_email           | email_sent | send_email |
+-----------+-------+---------+--------------------------+------------+------------+
|         1 | new   | pending | [email protected] |          0 |          0 |
+-----------+-------+---------+--------------------------+------------+------------+

❌ Actual result:

  • before fixing
    mysql -uroot -p123123q -e"select entity_id,state,status,customer_email,email_sent,send_email from develop.sales_order;"
mysql: [Warning] Using a password on the command line interface can be insecure.
+-----------+-------+---------+-------------------+------------+------------+
| entity_id | state | status  | customer_email    | email_sent | send_email |
+-----------+-------+---------+-------------------+------------+------------+
|         1 | new   | pending | [email protected] |          1 |          1 |
+-----------+-------+---------+-------------------+------------+------------+
  • after fixing
    mysql -uroot -p123123q -e"select entity_id,state,status,customer_email,email_sent,send_email from pr.sales_order;"
mysql: [Warning] Using a password on the command line interface can be insecure.
+-----------+-------+---------+--------------------------+------------+------------+
| entity_id | state | status  | customer_email           | email_sent | send_email |
+-----------+-------+---------+--------------------------+------------+------------+
|         1 | new   | pending | [email protected] |          1 |          1 |
+-----------+-------+---------+--------------------------+------------+------------+

I see that there is no difference between before and after fixing: email_sent and send_email are equal to 1. Would you mind to share parts of your Sendmail configuration that makes the whole configuration to be broken?

@sidolov
Copy link
Contributor

sidolov commented Mar 8, 2018

Hi @TomashKhamlai , one of the possible options how to broke mail service - invalid sendmail configuration in your php.ini:
sendmail_path=/invalid/path/to/sendmail
after that fix, you can reproduce described behavior.

@pawcioma , I verified your solution - it works as expected, thank you for your contribution!

@magento-engcom-team magento-engcom-team merged commit 1c7e3a1 into magento:2.2-develop Mar 8, 2018
@ihor-sviziev
Copy link
Contributor

Hi @pawcioma, thank you for your contribution! Could you create new Pull Request to 2.3-develop branch with the same changes?

@pawcioma
Copy link
Contributor Author

@ihor-sviziev Ok I will prepare

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.

6 participants