Simplify code product alert email #16571

arnoudhgz · 2018-07-05T21:33:18Z

The send method was complex and had some duplication in code. This is improved by using private methods for small parts of this method and making the if/else statements more logical (else isn't even used anymore).

As result the PHPMD suppress warning annotations could be removed.

For better string comparison the currently defined types (price and stock) are set as constants.

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-engcom-team · 2018-07-05T21:33:24Z

Hi @arnoudhgz. 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 {$VERSION} instance - deploy vanilla Magento instance

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

orlangur · 2018-07-06T07:27:00Z

app/code/Magento/ProductAlert/Model/Email.php

+            return self::XML_PATH_EMAIL_PRICE_TEMPLATE;
+        }
+
+        return self::XML_PATH_EMAIL_STOCK_TEMPLATE;


Please replace all such conditionals with ternary operator.

@orlangur thank you for your feedback, I changed it...

VladimirZaets · 2018-07-09T08:34:11Z

Hi @arnoudhgz, thank you for collaboration. Please enable strict type mode for this uses declare(strict_types=1) declaration.

VladimirZaets · 2018-07-09T08:35:25Z

app/code/Magento/ProductAlert/Model/Email.php

+     *
+     * @return StoreInterface|Store
+     */
+    private function getStore($customerStoreId)


Please use type hints for the method arguments.

VladimirZaets · 2018-07-09T08:35:32Z

app/code/Magento/ProductAlert/Model/Email.php

+     *
+     * @return string
+     */
+    private function getTemplateConfigPath()


Please use type hints for the method arguments.

VladimirZaets · 2018-07-09T08:35:40Z

app/code/Magento/ProductAlert/Model/Email.php

+     *
+     * @return array
+     */
+    private function getProducts()


Please use type hints for the method arguments.

VladimirZaets · 2018-07-09T08:35:44Z

app/code/Magento/ProductAlert/Model/Email.php

+     *
+     * @return Price|Stock
+     */
+    private function getBlock()


Please use type hints for the method arguments.

arnoudhgz · 2018-07-10T13:56:07Z

@VladimirZaets I have added the strict types and included type hinting where possible.

I saw some existing protected methods like setWebsiteId where I could not define a strict type, because the input can be also string in dependend code....

arnoudhgz · 2018-07-10T21:32:41Z

@VladimirZaets I see that with the strict types give now errors in the unit tests. I will first fix this before you have to check again.

arnoudhgz · 2018-07-10T21:33:38Z

@VladimirZaets I will fix the errors in the unit test first before you have to check again.

VladimirZaets · 2018-07-12T09:55:35Z

Hi @arnoudhgz, maybe you need any help?

arnoudhgz · 2018-07-12T10:19:26Z

@VladimirZaets Yes, I would like that.
I am struggling with the strict type for the setCustomerData method. First I had CustomerInterface but that did not workout, DataObject worked for the unit tests, but now it fails on the integration test.

VladimirZaets · 2018-07-12T10:33:52Z

@arnoudhgz as I see, the $customer should be CustomerInterface type. I searched usages of this method but I can't find any place where was passed param with other type. Can you please explain what the problem when you add CustomerInterface as a type?

arnoudhgz · 2018-07-12T10:48:54Z

@VladimirZaets I had this first as CustomerInterface but than the unit tests said that a DataObject was given. The CustomerRepositoryInterface has a return type of CustomerInterface but it actually returns a DataObject.

That is why I changed it to DataObject.

When using CustomerInterface:

VladimirZaets · 2018-07-12T11:17:53Z

@arnaucoll I think the problem is in the unit test that returns DataObject. What test is failed?

arnoudhgz · 2018-07-12T11:20:27Z

@VladimirZaets please see my updated comment with screenshot ^^

VladimirZaets · 2018-07-12T12:19:59Z

@arnoudhgz The problem is in the test.
In this line uses DataObject instead of CustomerInterface to create the customer mock.

Can you please fix it?

arnoudhgz · 2018-07-14T08:10:11Z

@VladimirZaets it seems solved now. There is only the timeout in Travis.

VladimirZaets · 2018-07-16T09:12:09Z

@arnoudhgz Thanks for fixes

VladimirZaets · 2018-07-23T11:49:22Z

Hi @arnoudhgz, can you please make refactoring and use private properties instead of constants.

arnoudhgz · 2018-07-23T12:11:00Z

@VladimirZaets I can, but it seems wrong, I think the class Magento\ProductAlert\Model\Observer should use EMAIL::TYPE_PRICE abd EMAIL::TYPE_STOCK instead of hardcoded strings in $email->setType('price'); and $email->setType('stock');.

Do you think I should maybe refactor this Observer also to make the code better (remove complexities)?

arnoudhgz · 2018-07-28T13:22:18Z

@VladimirZaets are you really sure I should use private properties? These values are constants imho, they can't be changed with setters or inside other functions.

According to the defnition of constants on the PHP website:

A constant is an identifier (name) for a simple value. As the name suggests, that value cannot change during the execution of the script

VladimirZaets · 2018-08-07T08:33:45Z

Hi @arnoudhgz. I discussed this question and we take decisions what we should do in the similar cases.
We should use constants if value uses in the few classes. If their value uses in the scope of the class we should use private static declaration.

arnoudhgz · 2018-08-07T23:36:17Z

@VladimirZaets thank you for the explanation, after I changed it to private static variables some test failed in Travis. It was about having to many variables on the class (the maximum is 15). So I changed the code to compare on strings (price or stock).

VladimirZaets · 2018-08-10T08:55:01Z

Hi @arnoudhgz. I discussed this issue with my teammates and we take the decision that we can't add strict type and type checking to the already existing files due to backward-compatible reason. It's my fail, sorry. Can you please revert the changes related to strict types. Thanks

arnoudhgz · 2018-08-10T14:10:20Z

@VladimirZaets I will change it back.... 👍

arnoudhgz · 2018-08-10T18:54:30Z

@VladimirZaets I removed the strict types checking + the return types and parameter typehinting for existing functions.

The send method was complex and had some duplication in code. This is improved by using private methods for small parts of this method and making the if/else statements more logical (else isn't even used anymore). As result the PHPMD suppress warning annotations could be removed.

arnoudhgz · 2018-08-16T13:26:31Z

@VladimirZaets do I still need to update the code? Could you provide some feedback?

VladimirZaets · 2018-08-31T08:31:15Z

Hi @arnoudhgz. Sorry for the delay, we took your PR for delivery

magento-engcom-team · 2018-09-12T10:19:05Z

Hi @arnoudhgz. Thank you for your contribution.
We will aim to release these changes as part of 2.3.0.
Please check the release notes for final confirmation.

magento-engcom-team added Partner: MediaCT Pull Request is created by partner MediaCT partners-contribution Pull Request is created by Magento Partner labels Jul 5, 2018

magento-engcom-team added the Component: ProductAlert label Jul 5, 2018

magento-engcom-team assigned VladimirZaets Jul 6, 2018

magento-engcom-team added this to the Release: 2.3.0 milestone Jul 6, 2018

magento-engcom-team added the Release Line: 2.3 label Jul 6, 2018

orlangur reviewed Jul 6, 2018

View reviewed changes

VladimirZaets reviewed Jul 9, 2018

View reviewed changes

VladimirZaets added the Progress: needs update label Jul 9, 2018

arnoudhgz closed this Jul 10, 2018

arnoudhgz reopened this Jul 10, 2018

VladimirZaets removed the Progress: needs update label Jul 16, 2018

VladimirZaets added the Progress: needs update label Jul 23, 2018

arnoudhgz mentioned this pull request Aug 10, 2018

Improve code quality subscriber new action #17232

Merged

4 tasks

magento-engcom-team removed the Progress: needs update label Aug 31, 2018

magento-engcom-team added the Progress: accept label Aug 31, 2018

Simplify code product alert email

316634c

magento-engcom-team merged commit 316634c into magento:2.3-develop Sep 12, 2018

magento-engcom-team pushed a commit that referenced this pull request Sep 12, 2018

ENGCOM-2204: Simplify code product alert email #16571

48dc4c4

Simplify code product alert email #16571

Simplify code product alert email #16571

Uh oh!

Conversation

arnoudhgz commented Jul 5, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Contribution checklist

Uh oh!

magento-engcom-team commented Jul 5, 2018

Uh oh!

orlangur Jul 6, 2018

Choose a reason for hiding this comment

Uh oh!

arnoudhgz Jul 6, 2018

Choose a reason for hiding this comment

Uh oh!

VladimirZaets commented Jul 9, 2018

Uh oh!

VladimirZaets Jul 9, 2018

Choose a reason for hiding this comment

Uh oh!

VladimirZaets Jul 9, 2018

Choose a reason for hiding this comment

Uh oh!

VladimirZaets Jul 9, 2018

Choose a reason for hiding this comment

Uh oh!

VladimirZaets Jul 9, 2018

Choose a reason for hiding this comment

Uh oh!

arnoudhgz commented Jul 10, 2018

Uh oh!

arnoudhgz commented Jul 10, 2018

Uh oh!

arnoudhgz commented Jul 10, 2018

Uh oh!

VladimirZaets commented Jul 12, 2018

Uh oh!

arnoudhgz commented Jul 12, 2018

Uh oh!

VladimirZaets commented Jul 12, 2018

Uh oh!

arnoudhgz commented Jul 12, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

VladimirZaets commented Jul 12, 2018

Uh oh!

arnoudhgz commented Jul 12, 2018

Uh oh!

VladimirZaets commented Jul 12, 2018

Uh oh!

arnoudhgz commented Jul 14, 2018

Uh oh!

VladimirZaets commented Jul 16, 2018

Uh oh!

VladimirZaets commented Jul 23, 2018

Uh oh!

arnoudhgz commented Jul 23, 2018

Uh oh!

arnoudhgz commented Jul 28, 2018

Uh oh!

VladimirZaets commented Aug 7, 2018

Uh oh!

arnoudhgz commented Aug 7, 2018

Uh oh!

VladimirZaets commented Aug 10, 2018

Uh oh!

arnoudhgz commented Aug 10, 2018

Uh oh!

arnoudhgz commented Aug 10, 2018

Uh oh!

arnoudhgz commented Aug 16, 2018

Uh oh!

VladimirZaets commented Aug 31, 2018

Uh oh!

magento-engcom-team commented Sep 12, 2018

Uh oh!

Uh oh!

arnoudhgz commented Jul 5, 2018 •

edited

Loading

arnoudhgz commented Jul 12, 2018 •

edited

Loading