Skip to content

#12808 Add support for 'Skip errors' validation strategy #12867

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

Closed
wants to merge 3 commits into from
Closed

#12808 Add support for 'Skip errors' validation strategy #12867

wants to merge 3 commits into from

Conversation

JustBeYou
Copy link

@JustBeYou JustBeYou commented Dec 23, 2017

Description

I added in the validation phase of CVS importing some checks about validation strategy. Now, if the validation strategy is chosen to be 'Skip errors', the CVS file will be imported if the invalid rows number is smaller or equal to the number of allowed errors. In the previous versions, even if this option is selected, the data validation will fail if any invalid row is found.

Fixed Issues (if relevant)

  1. Magento 2.2.2-dev - CSV Import, skip errors not working #12808: Magento 2.2.2-dev - CSV Import, skip errors not working

Manual testing scenarios

  1. Download this CSV https://transfer.sh/PGiN8/catalog_product.csv
  2. Go to Import section
  3. Select 'Products' and 'Add/Update'
  4. Select 'Skip Errors' and type '5' as 'Allowed Error Count'
  5. Import the downloaded CSV and look how the validation pass even that there are 2 invalid rows

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-cicd2
Copy link
Contributor

magento-cicd2 commented Dec 23, 2017

CLA assistant check
All committers have signed the CLA.

@magento-engcom-team magento-engcom-team added 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 Reproduced on 2.3.x The issue has been reproduced on latest 2.3 release labels Dec 23, 2017
@dmanners dmanners self-assigned this Jan 2, 2018
@dmanners dmanners added this to the January 2018 milestone Jan 2, 2018
@dmanners
Copy link
Contributor

dmanners commented Jan 2, 2018

Hey @JustBeYou thank you for this Pull Request. I will review and process this now. If I need anything I will communicate that with you.

*
* @return string
*/
public function getValidationStrategy();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unfortunately in this type of release (a PR to 2.2-develop is considered a Patch release) we cannot change interface classes because of our BiC guidelines. It would be a good idea to add a new interface and migrate any similar methods to this new interface (see http://devdocs.magento.com/guides/v2.2/contributor-guide/backward-compatible-development/index.html#introduction-of-a-method-to-a-class-or-interface for more information)

@dmanners
Copy link
Contributor

dmanners commented Feb 7, 2018

Hey @JustBeYou are you still interested in working on this Pull Request? If so could you have a look into the requested changes with regards to backwards compatibility.

Thanks

@okorshenko okorshenko modified the milestones: January 2018, February 2018 Feb 7, 2018
@dmanners
Copy link
Contributor

Hi @JustBeYou I am going to close this pull request due to inactivity. Thank you for your effort so far, if you would like to come back to this at a later date feel free to open a new pull request with the requested changes or reach out of one of the team to help out.

Thank you.

@dmanners dmanners closed this Feb 26, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Component: ImportExport Progress: needs update 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 Reproduced on 2.3.x The issue has been reproduced on latest 2.3 release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants