magento-engcom/import-export-improvements#30: Validation should fail when a required field is supplied but is empty after trimming

[pogster]

Description

As the issue describes, when on customer import the first name or last name are supplied with a whitespace value, validation will not fail as opposed to editing a customer on the backend. The file succeeds to validate and import. This should not be the case. Furthermore, when both first and last name are invalidated, both validation failures should show on the import messages.

As a solution, I moved the required field validation before the attribute validation and added a check for empty strings after trimming.

Additionally, with that solution, only a validation error for the first failed field would show. This is caused by the error code valueIsRequired / line 1 (for example) is used for each field, but the ProcessingErrorAggregator only allows one error of the same code per line. I did not find out in which scenario this would be relevant and removed the code, undoing an earlier commit.

Fixed Issues (if relevant)

1. [MAGETWO-1802] Import of file with spaces in required attributes is allowed #30

Manual testing scenarios

Test data for csv:

email	_website	firstname	group_id	lastname
test_ifebgl@unknown-domain.com	base		1

Steps to reproduce

1. Open Backend -> System -> Import/Export -> Import
2. Select Entity Type: Customers
3. Select Import Behaviour: Append Complex Data
4. Select Import Format Version: Magento 1.7 format
5. Select test file
6. Click Check Data button.

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)

[dmanners]

Hi @pogster do you think you can have a look over the travis failures and see if you can solve them? If you have any issues fixing these feel free to drop me a message.

[ulftietze]

How about this way to write? I think it's more readable ^^ Logically it's okay i think.

$attributeCodeNotSetAndNoCustomerId = !isset($rowData[$attributeCode]) && !$this->_getCustomerId($email, $website);
$attributeCodeSetAndValueIsEmptyString = isset($rowData[$attributeCode])  && '' === trim($rowData[$attributeCode]);

if (
    $attributeParams['is_required']
    && ($attributeCodeNotSetAndNoCustomerId || $attributeCodeSetAndValueIsEmptyString)
) {

[pogster]

Agreed, better readable, do they do that in Magento core nowadays?
Short circuit evaluation might be a reason to not do it.

[pogster]

@dmanners managed to. Yaay! I'm impressed by the unit tests actually.

[dmanners]

Could you also take a look at the static tests:

1) Magento\Test\Php\LiveCodeTest::testCodeStyle
PHP Code Sniffer detected 1 violation(s): 
FILE: ...provements/app/code/Magento/CustomerImportExport/Model/Import/Customer.php
--------------------------------------------------------------------------------
FOUND 2 ERRORS AFFECTING 2 LINES
--------------------------------------------------------------------------------
 598 | ERROR | Line exceeds maximum limit of 120 characters; contains 133
     |       | characters
 599 | ERROR | Line exceeds maximum limit of 120 characters; contains 125
     |       | characters
--------------------------------------------------------------------------------

[pogster]

Yeah my refactoring screwed them up. I will look for something to support the M2 coding standards. And run the functional tests.

[pogster]

@dmanners it seems the integration tests are timing out? is that a known issue or something?

[dmanners]

@pogster let me check on those timing out tests for you.

[magento-engcom-team]

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