Setting the Title as Unique fails if the Source wraps the content in quotes.

In my case, I have two Feed-Types importing into a single Content-Type. Each Feed-Type imports a different source, each containing the same Title. The titles may contain commas, and so the CSV export wraps them in Quotes.

CommentFileSizeAuthor
#7 feeds-csv-machine-name-test-3021269-7.patch5.54 KBmegachriz
#4 interdiff-3021269-2-4.txt3.91 KBmegachriz
#4 feeds-csv-machine-name-test-3021269-4.patch7.44 KBmegachriz
#2 feeds-csv-machine-name-test-3021269-2.patch4.07 KBmegachriz
feeds_import_feeds_import.tar_.gz271 bytesrovo

Comments

rovo created an issue. See original summary.

megachriz’s picture

megachriz
Primary language Dutch
commented
Status: Active » Needs review
StatusFileSize
new feeds-csv-machine-name-test-3021269-2.patch4.07 KB

Thanks for reporting. I think reverting #2997951: Custom mapping source should use value as item getter instead of machine name? could have fixed this issue. To ensure that, we need to add test coverage for this issue.

I start with forcing tests to pass machine names to the method ::createFeedTypeForCsv().

Status: Needs review » Needs work

The last submitted patch, 2: feeds-csv-machine-name-test-3021269-2.patch, failed testing. View results

megachriz’s picture

megachriz
Primary language Dutch
commented
Status: Needs work » Needs review
StatusFileSize
new feeds-csv-machine-name-test-3021269-4.patch7.44 KB
new interdiff-3021269-2-4.txt3.91 KB

This adds a test for importing a CSV with spaces, uppercase characters and special characters in the column names. I think I've fixed the test failure for #2 too.

  • MegaChriz committed f5cc0ee on 8.x-3.x 
    Issue #3021269 by MegaChriz, rovo: Added test coverage for importing CSV...
megachriz’s picture

megachriz
Primary language Dutch
commented
Status: Needs review » Fixed

Yay! The test passes. Now we have test coverage for the bug reported in this issue!

Committed #4.

megachriz’s picture

megachriz
Primary language Dutch
commented
Status: Fixed » Needs review
StatusFileSize
new feeds-csv-machine-name-test-3021269-7.patch5.54 KB

Spoke too soon, #2997951-9: Custom mapping source should use value as item getter instead of machine name? - which introduced the bug reported in this issue - still passes.

The changed test also tests updating items. I forgot that the issue reported here was specifically to do with updating existing items. I also added a test for altering parsed CSV data. This would cover the case that broke Feeds Tamper.

I now ensured that the changes in these test don't pass with #2997951-9: Custom mapping source should use value as item getter instead of machine name? applied.

  • MegaChriz committed d6301cb on 8.x-3.x 
    Issue #3021269 by MegaChriz, rovo: Added test coverage for updating and...
megachriz’s picture

megachriz
Primary language Dutch
commented
Status: Needs review » Fixed

Committed #7.

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.