Skip to content
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

Fix product import on existing, empty unit_type and variant_unit #8876

Merged

Conversation

SarvarKh
Copy link
Contributor

What? Why?

Closes #8773

Importing product file with empty unit_type andvariant_unit_name columns on already existing products was failing without any error notice to the user.

What should we test?

  • Visit /admin/product_import page.
  • Fill out product list template (.csv) leaving unit_type andvariant_unit_name columns empty and product name column to already existing product name.
  • Select your .csv file, upload and import.

Corresponding error notification appears.

PR_import

Release notes

Changelog Category: User facing changes

Copy link
Member

@mkllnk mkllnk left a comment

Choose a reason for hiding this comment

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

Great!

Would you like to write a spec, too? You could improve spec/models/product_import/entry_validator_spec.rb.

@SarvarKh
Copy link
Contributor Author

Thank you @mkllnk
I noticed only the inventory validation spec was tested.
To test product validation, I have been trying to mock tax_and_shipping_validation for validate_all without success so far.

In short, its arguments are throwing an error:

  1) ProductImport::EntryValidator product validation products exist validates a spreadsheet entry in kg
     Failure/Error: tax_and_shipping_validation(entry, 'tax', entry.tax_category, @spreadsheet_data.tax_index)
       #<Double :spreadsheet_data> received unexpected message :tax_index with (no args)

Tried to do something like:

allow(entry_validator).to receive(:tax_and_shipping_validation).with(entry_product, true, entry_product.tax_category, true).and_return({something: 'testing'})

@mkllnk
Copy link
Member

mkllnk commented Feb 16, 2022

It's not a great spec. It uses a lot of mocked data. But I created an example to show how you can reach the code you fixed:

+  describe "variant validation" do
+    let(:potatoes) {
+      create(
+        :simple_product,
+        supplier: enterprise,
+        on_hand: '100',
+        name: 'Potatoes',
+        unit_value: 1000,
+        variant_unit_scale: 1000,
+        variant_unit: 'weight'
+      )
+    }
+    let(:potato_variant) do
+      ProductImport::SpreadsheetEntry.new(
+        unscaled_units: "1",
+        units: "1",
+        unit_type: "kg",
+        name: potatoes.name,
+        display_name: 'Pink Potatoes',
+        enterprise: enterprise,
+        enterprise_id: enterprise.id,
+        producer: enterprise,
+        producer_id: enterprise.id,
+      )
+    end
+
+    before do
+      allow(import_settings).to receive(:dig)
+      allow(spreadsheet_data).to receive(:tax_index)
+      allow(spreadsheet_data).to receive(:shipping_index)
+      allow(entry_validator).to receive(:enterprise_validation)
+      allow(entry_validator).to receive(:tax_and_shipping_validation)
+      allow(entry_validator).to receive(:variant_of_product_validation)
+    end
+
+    it "validates a product" do
+
+      entries = [potato_variant]
+      entry_validator.validate_all(entries)
+
+      expect(potato_variant.errors.count).to eq 0
+    end
+  end

There are still errors on the entry. This is not a complete test case but maybe a start for you?

@SarvarKh SarvarKh force-pushed the product-import-validation branch from 39a46cd to 22de758 Compare February 16, 2022 09:20
@SarvarKh
Copy link
Contributor Author

The given example was a great help, thanks.

Copy link
Contributor

@jibees jibees left a comment

Choose a reason for hiding this comment

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

👌 🙏

@filipefurtad0
Copy link
Contributor

Hey @SarvarKh ,

I could reproduce the error on the freezing progress bar, it seems the same as the one reported here:
#5363 (comment)

image

After your PR this is fixed:

The user sees why the file is not uploaded, and the error is not seen on the Response tab:

image

Updating other fields like stock, but having unit_type = g (in this case) works as before 👍

So, this closes both #5363 and #8773 - awesome!!! 💪 🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Uploading a product import file with empty unit type fails without any clue on the UI
4 participants