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

Fix email not sent when sitemap generation has errors #11320

Conversation

marinagociu
Copy link

Description

This PR changes the logic for handling errors in the sitemap generation cron. If an exception is thrown when trying to generate any of the sitemaps, the processes is not stopped anymore, but instead the errors are sent by email based on the XML Sitemap configuration. The old _translateModel property is not used anymore, and the inline translation is correctly suspended using the inlineTranslation property instead.

Fixed Issues (if relevant)

  1. Fatal error: Call getTranslateInline of null when generating some sitemap with errors #10502: Fatal error: Call getTranslateInline of null when generating some sitemap with errors

Manual testing scenarios

  1. Create a sitemap.
  2. Delete the directory in which the sitemap should be generated, or any other thing that will result in an error when trying to generate the sitemap.
  3. In the XML Sitemap configuration enable the cron for generating the sitemap. Setup an email address to receive the errors.
  4. Let the cron run and try to generate the sitemap.
  5. An email should be received in the configured email address and the cron task should be successful. Before, the cron task went into the status error at the first sitemap that could not be generated and the exception message was saved in on the cron task in the message field.

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)

The lines were a reminiscence from when the translation logic was in the
core module. The logic was replaced with the correct use of the
inlineTranslation property that is also present at the end of the method.
@vkublytskyi
Copy link

@marinagociu Thank you for contribution. Please fix failed unit tests (see Travis builds). Actually, unit tests duplicate the error from code so it expects that exception is thrown.

@marinagociu
Copy link
Author

@vkublytskyi, I think I need you input and help on this as I'm not sure how to proceed. As far as I see it the unit test that fails should be removed since the method should not throw an exception, but instead it should send the exception messages by email. Can you confirm this? Should I remove that test, or replace it with another one? If so, with what kind of test should I replace it? Thanks!

@vkublytskyi
Copy link

@marinagociu It should be enough just to fix failed unit test:

  • remove @expectedException \Exception annotation
  • add some message to exception (e.g. Something goes wrong)
  • ensure that for $this->transportBuilderMock method setTemplateVars is invoked with ['warnings' => 'Something goes wrong'] argument.

You may also face the issue caused by fact that observer use method chaining on transport builder. So you will need to mock all methods to return transportBuilderMock or refactor code to call all methods on $this->_transportBuilder instead of chaining.

@marinagociu
Copy link
Author

@vkublytskyi thank you so much for you help! I've updated the unit test with the suggested changes.

adrian-martinez-interactiv4 added a commit to adrian-martinez-interactiv4/magento2 that referenced this pull request Oct 24, 2017
Magento/Sitemap/Model/Observer.php => will be fixed by open PR magento#11320
@vkublytskyi vkublytskyi added 2.2.x bug report Reproduced on 2.1.x The issue has been reproduced on latest 2.1 release Reproduced on 2.2.x The issue has been reproduced on latest 2.2 release labels Oct 30, 2017
@okorshenko okorshenko modified the milestones: October 2017, November 2017 Nov 1, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug report bugfix Progress: accept Release Line: 2.2 Reproduced on 2.1.x The issue has been reproduced on latest 2.1 release Reproduced on 2.2.x The issue has been reproduced on latest 2.2 release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants