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

pogster commented 6 years ago

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)

magento-engcom/import-export-improvements#30

Manual testing scenarios

Test data for csv:

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

Steps to reproduce

Open Backend -> System -> Import/Export -> Import
Select Entity Type: Customers
Select Import Behaviour: Append Complex Data
Select Import Format Version: Magento 1.7 format
Select test file
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 commented 6 years ago

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.

pogster commented 6 years ago

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

dmanners commented 6 years ago

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 commented 6 years ago

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

pogster commented 6 years ago

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

dmanners commented 6 years ago

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

magento-engcom-team commented 6 years ago

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