Well, based on the title, I guess that is expected. But for example the price "4 000" I would expect I could import. However, this is currently not the case.

The price "one hundred eirik dollars" should probably not be allowed to import ;)

However, if one decides to import such a price, it should not crash the other prices we are trying to import. Super annoying if price number 80000 crashes the import, and you have to start over.

CommentFileSizeAuthor
#10 commerce_pricelist-3087390-10.patch6.54 KBjsacksick
#10 interdiff_9-10.txt411 bytesjsacksick
#9 commerce_pricelist-3087390-9.patch6.54 KBjsacksick
#6 3087390-6.patch4.84 KBeiriksm
#2 3087390.patch4.82 KBeiriksm
#2 3087390-test-only.patch3.06 KBeiriksm
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

eiriksm created an issue. See original summary.

eiriksm’s picture

eiriksm
he/him
Primary language Norwegian Bokmål
Location Norway
CreditAttribution: eiriksm at Ny Media AS, Violinist, Foreningen Drupal Norge commented
Status: Active » Needs review
FileSize
3087390-test-only.patch3.06 KB
3087390.patch4.82 KB
eiriksm’s picture

eiriksm
he/him
Primary language Norwegian Bokmål
Location Norway
CreditAttribution: eiriksm at Ny Media AS, Violinist, Foreningen Drupal Norge commented

So basically this fixes three issues in one:

- Make it possible to import prices with a space in them
- Make sure the import does not crash if a price is invalid in one of the rows
- Make sure the import does not crash (but rather skip the list price) on invalid list price fields.

I could make separate issues for all of them, but it was convenient for me.

The last submitted patch, 2: 3087390-test-only.patch, failed testing. View results

jsacksick’s picture

jsacksick CreditAttribution: jsacksick at Centarro commented 
+++ b/src/Form/PriceListItemImportForm.php
@@ -360,6 +360,11 @@ class PriceListItemImportForm extends FormBase {
+        $row['price'] = str_replace(' ', '', $row['price']);

I don't think the if is necessary, and we should probably do that for the list price as well no?

eiriksm’s picture

eiriksm
he/him
Primary language Norwegian Bokmål
Location Norway
CreditAttribution: eiriksm at Ny Media AS, Violinist, Foreningen Drupal Norge commented
FileSize
3087390-6.patch4.84 KB

Good point!

Updated patch

jsacksick’s picture

jsacksick CreditAttribution: jsacksick at Centarro commented

Sorry for the late feedback, we already have functional tests for the import, and these are in PriceListItemTest, not PriceListTest.

Could you actually add a method there? (We already have testImportPriceListItemsWithDelete and testImportPriceListItemsWithUpdate).

jsacksick’s picture

jsacksick CreditAttribution: jsacksick at Centarro commented
Status: Needs review » Needs work

The patch needs to be rerolled now that #3095942: Move the row for building the CSV header & rows to helper methods was committed.

jsacksick’s picture

jsacksick CreditAttribution: jsacksick at Centarro commented
Status: Needs work » Needs review
FileSize
commerce_pricelist-3087390-9.patch6.54 KB

Ok, I moved the test to PriceListItemTest, and refactored it a little, tests are passing for me locally.

jsacksick’s picture

jsacksick CreditAttribution: jsacksick at Centarro commented
FileSize
interdiff_9-10.txt411 bytes
commerce_pricelist-3087390-10.patch6.54 KB

Removing the empty useless line in the CSV file.

  • jsacksick committed 58233b2 on 8.x-2.x authored by eiriksm 
    Issue #3087390 by eiriksm, jsacksick: Badly formatted prices can crash...
jsacksick’s picture

jsacksick CreditAttribution: jsacksick at Centarro commented
Status: Needs review » Fixed

Tests are passing, committed!

Status: Fixed » Closed (fixed)

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