This repository has been archived by the owner on Apr 29, 2019. It is now read-only.

import-export-improvements #82 : super attribute error message improvements #115

Merged

magento-engcom-team merged 8 commits into magento-engcom:2.3-develop from tadhgbowe:import-export-issue-82-error-messaging

Jul 21, 2018

tadhgbowe commented Jun 28, 2018

Improvement to product import error handling for configurable variations column super attribute codes.

Description

If the standard product import detects that a configurable_variations attribute code is not super one the following error messages will be displayed:

Attribute code does not exist or is not in chosen attribute set.
Attribute code needs to have an Input Type of "Dropdown", "Visual Swatch" or Text Swatch"
Attribute code needs to have a Scope of "Global".

This covers all the reasons why it may not be a not super attribute. Displaying "Attribute with this code is not super" is not helpful enough.

Fixed Issues (if relevant)

#82
Please see the above issue as there is a full comment trace.

Manual testing scenarios

Try import a configurable product with a super attribute code inside configurable_variations that:

doesn't exist
exists but is not in the target attribute set
is not global scope
Is not of type "dropdown", "visual swatch" or "text swatch"

Contribution checklist

[ x ] Pull request has a meaningful description of its purpose
[ x ] 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)


          import-export-improvements magento-engcom#82 : configurable variation…

e99c99e

…s - not a super attribute error message improvements

Contributor

magento-cicd2 commented Jun 28, 2018 •

edited

Loading

All committers have signed the CLA.

Tadhg Bowe added 3 commits

June 28, 2018 16:07


          import-export-improvements magento-engcom#82 : configurable variation…

65bce0a

…s - not a super attribute error message improvements - travis ci build code fix


          import-export-improvements magento-engcom#82 : configurable variation…

7f9f6db

…s - not a super attribute error message improvements - travis ci build code style fix


          import-export-improvements magento-engcom#82 : configurable variation…

a76e3a3

…s - not a super attribute error message improvements - travis ci build code style fixes

magento-engcom-team assigned dmanners

magento-engcom-team added this to the Release: 2.3.0 milestone

magento-engcom-team added 2.3-develop Component: ConfigurableImportExport labels

Contributor

magento-engcom-team commented Jul 3, 2018

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

dmanners suggested changes

View reviewed changes

Contributor

dmanners left a comment

Hey, thanks for your PR. I have added a few comments on issues. I am happy with you adding more detail for the user. Good job.

app/code/Magento/ConfigurableImportExport/Model/Import/Product/Type/Configurable.php Outdated

+                   * @param int $rowNum
+                   * @return bool
+                   */
+                  protected function _identifySuperAttributeError($superAttrCode, $rowNum)

Contributor

dmanners Jul 3, 2018

I think we could probably make this method private and remove the _ in the name. For me since this method is only being used in this class making it private would work fine. Do you think that makes sense or would you think it should be open for extension? In that case I would make it public as we are trying to stay away from protected methods.

app/code/Magento/ConfigurableImportExport/Model/Import/Product/Type/Configurable.php Outdated

+                      $codeNotGlobal = false;
+                      $codeNotTypeSelect = false;
+                      // Does this attribute code exist? Does is have the correct settings?
+                      $commonAttributes = self::$commonAttributesCache;

Contributor

dmanners Jul 3, 2018

Do we need this assignment or could we just loop through the cache directly?

app/code/Magento/ConfigurableImportExport/Model/Import/Product/Type/Configurable.php Outdated

+                      $codeNotTypeSelect = false;
+                      // Does this attribute code exist? Does is have the correct settings?
+                      $commonAttributes = self::$commonAttributesCache;
+                      foreach ($commonAttributes as $attributeRow) {

Contributor

dmanners Jul 3, 2018

Is there no way to access the attribute from the cache via the code rather than looping through all attributes. This could be a big performance hit if you have a large amount of attributes.

app/code/Magento/ConfigurableImportExport/Model/Import/Product/Type/Configurable.php Outdated

+                          }
+                      }
+                      if ($codeExists == false) {

Contributor

dmanners Jul 3, 2018

Since we are setting these attributes let's do a === check here just to be sure we are dealing with booleans.

app/code/Magento/ConfigurableImportExport/Model/Import/Product/Type/Configurable.php Outdated

-                              // check attribute superity
-                              $this->_entityModel->addRowError(self::ERROR_ATTRIBUTE_CODE_IS_NOT_SUPER, $rowNum, $superAttrCode);
+                              // Identify reason why attribute is not super:
+                              if (!$this->_identifySuperAttributeError($superAttrCode, $rowNum)) {

Contributor

dmanners Jul 3, 2018

Overall I like adding in this new check to give some more useful information to the user.

app/code/Magento/ConfigurableImportExport/Model/Import/Product/Type/Configurable.php Outdated

                    */
                   protected $_messageTemplates = [
-                      self::ERROR_ATTRIBUTE_CODE_IS_NOT_SUPER => 'Attribute with code "%s" is not super',
-                      self::ERROR_INVALID_OPTION_VALUE => 'Invalid option value for attribute "%s"',
+                      self::ERROR_ATTRIBUTE_CODE_DOES_NOT_EXIST => 'Column configurable_variations: Attribute with code "%s" does not exist or is missing from product attribute set',

Contributor

dmanners Jul 3, 2018

I have seen a few of these line issues before and people will format the array so the key and value are on two different lines. Would that make lines under 120 characters? Also does that make sense from your point of view?


          import-export-improvements magento-engcom#82 : configurable variation…

3a0599d

…s - not a super attribute error message improvements - code styling fixes

Contributor

dmanners commented Jul 6, 2018

@tadhgbowe there seem to be a could of static test failures now.

1) Magento\Test\Php\LiveCodeTest::testCodeStyle
PHP Code Sniffer detected 2 violation(s): 
FILE: ...agento/ConfigurableImportExport/Model/Import/Product/Type/Configurable.php
--------------------------------------------------------------------------------
FOUND 1 ERROR AFFECTING 1 LINE
--------------------------------------------------------------------------------
 342 | ERROR | [x] Expected 1 space after USE keyword; found 0
--------------------------------------------------------------------------------
PHPCBF CAN FIX THE 1 MARKED SNIFF VIOLATIONS AUTOMATICALLY
--------------------------------------------------------------------------------
Failed asserting that 2 matches expected 0.
/home/travis/build/magento-engcom/import-export-improvements/dev/tests/static/testsuite/Magento/Test/Php/LiveCodeTest.php:269
2) Magento\Test\Php\LiveCodeTest::testCodeMess
PHP Code Mess has found error(s):
/home/travis/build/magento-engcom/import-export-improvements/app/code/Magento/ConfigurableImportExport/Model/Import/Product/Type/Configurable.php:331	The method identifySuperAttributeError() has a Cyclomatic Complexity of 11. The configured cyclomatic complexity threshold is 10.

The first one seems like not a big problem to update. The second might take a bit of refactoring on the method to see if it can be decreased a bit. If you need some help, feel free to reach out to me on that.


          import-export-improvements magento-engcom#82 : configurable variation…

6a5b15d

…s - not a super attribute error message improvements - code styling Travis CI build fixes

Author

tadhgbowe commented Jul 6, 2018

@dmanners - thanks for the update earlier. Yep I noticed it too. I pushed up some enhancements (i.e. tidy up!) earlier on. Removing the foreach introduced this little beauty. There were some cyclomatic complexities too. I'm on the case... :-)

ps. hope you've been out for fresh air ;-)

Tadhg Bowe added 2 commits

July 7, 2018 12:38


          import-export-improvements magento-engcom#82 : configurable variation…

b98f417

…s - not a super attribute error message improvements - code styling Travis CI build fixes


          import-export-improvements magento-engcom#82 : configurable variation…

8db836b

…s - not a super attribute error message improvements - code styling Travis CI build fixes

Contributor

dmanners commented Jul 9, 2018

Hi @tadhgbowe looking good for the changes, I will check why the integration tests are timing out and get back to you on the next steps for this pr. Weekend involved plenty of fresh air and family time.

Author

tadhgbowe commented Jul 9, 2018

Morning @dmanners. #NoMagentoSunday working nicely for me. Football fever over here!
Yes, I've seen the timing out thing on other pull requests. So I hope it's just a temporary blip. Grand. T

Contributor

dmanners commented Jul 9, 2018

So I have run the internal tools and it all seems fine from the testing side, think it might just be travis. Will pass this onto our QA team now @tadhgbowe and keep you updated.

Author

tadhgbowe commented Jul 9, 2018

@dmanners - That's great. Thanks for the update. I've got a few other issues to get on with. Cheers. T

magento-engcom-team merged commit 8db836b into magento-engcom:2.3-develop

magento-engcom-team pushed a commit that referenced this pull request


          ENGCOM-2174: import-export-improvements #82 : super attribute error m…

1aefab8

…essage improvements #115

dmanners mentioned this pull request

Improve Errormessage #82

Closed

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

2.3-develop Component: ConfigurableImportExport