[Product Import] Product attributes #2562

Matt-Yorkley · 2018-08-20T16:12:33Z

What? Why?

Adds error messages when updating product-based fields on variants of existing products.

What should we test?

When attempting to update the category, description, unit_type, variant_unit_name, tax_category, or shipping_category fields on an existing product, the user is shown an error message that the field cannot be updated.

Release notes

Added erorrs when user is attempting to update non-updatable fields during product import.

Changelog Category: Changed

myriamboure · 2018-08-21T13:42:18Z

@Matt-Yorkley we said that we wouldn't save any import file until there are errors, as it is pretty confusing to enable saving if some products contain errors, we want to make sure the user is conscious about what she imports... and safe that she is not making mistakes or messing up her catalog. So shouldn't we stick to the same bevahior?
= Display those as error lines and let them make the corrections, either by changing the category on the platform for the given product (if they realize they did a mistake previously) or change it in their file if they realize they did a mistake in their file.
@RachL do you agree?
I probably should have been much more details in expected UX in specifying the issue, sorry!
For the upper warning message, I would move it to the main product import page so users know that before starting to import a file. And only display as errors in this second step if they didn't follow the rule and prevent saving.

And about the small "warning icon" on the item line, I would keep it but as the line would be in error section users will have to fix before being able to save. So it explains them why it is not saving and what they have to fix. Here also it's not clear if the value you see which is going to be imported is the one you want to import or the original one? If it's in error line it is clear that the value displaying the error message is the one you want to import and which is wrong.

Do you agree?

myriamboure · 2018-08-21T13:42:46Z

I put it back to in dev so that we can discuss before it is reviewed ;-)

Matt-Yorkley · 2018-08-24T15:29:57Z

@myriamboure I thought these would be ok as warnings, as they don't actually make any of the products invalid.

The message at the top about product fields only shows when there are changed product fields in the file, so most of the time it will not be there (only when relevant). The information feels like something we should have in the docs as opposed to the first upload page.

myriamboure · 2018-08-24T16:03:36Z

@RachL we need some UX feedback on this ;-) As it is important info I would leave it as a warning in the app as well... but happy to just take it out and leave it only in user guide if you all prefer that.

But on the pass/block saving question, @Matt-Yorkley I insist that if there is a risk of confusion in hub manager mind, it's not desired UX. And if I upload a file of 100 products, the system tells me the warning, but I have no clue how many and which products are concerned, so I have to go through the whole list and see where are the warnings to see if there is a problem or not, etc. It's not good UX to me. If you allow import and user doesn't know clearly what she imports, we need to review UX.

RachL · 2018-08-27T10:41:02Z

@myriamboure if I understand correctly, the message is shown when the user add those column in the file, right ? So it basically tells him that those columns won't be taken into account. I guess that it could be convenient to not block the import for that, so that the user has not to do another step of changing his file.
Moreover, in which case will a user really add those columns as they are not on the list of available columns in template file?

myriamboure · 2018-08-27T10:42:23Z

No it's not that @RachL. In a same import, a use can have new products for which those columns will be taken into account AND existing one for which they won't. And he won't know which one are taken into account or not.

myriamboure · 2018-08-27T12:10:45Z

@Matt-Yorkley we just discussed with @RachL and we'll talk quickly about it in our catchup today, but when discussing we agreed with Rachel on UX side that it would be better to :

Display error and block saving, so that we have a consistent behavior and avoid confusion in users mind about what they changed or didn't change. As any other error, with error message like "this product has conflicting info on main product attribute already in the catalog. Please check and resolve conflicts before submitting again." You can leave some warning icon on the field concerned by the conflict. If user want to import data with conflicting info it means she makes a mistake, either in the file or there is a mistake in the platform. So we want to help her fix her mistakes.
Don’t add notice on main product import page. We agree that it has to be in user guide. If we find lots of users are having issues with this present case we can see how to explain better.
For later, on iteration 2, it would be good to open some "conflict resolution modal (display two info and ask which one to choose)" so that we don't block saving and help resolve conflicts directly.

sauloperez · 2018-08-30T12:06:42Z

app/models/product_import/entry_validator.rb

+    end
+
+    def attributes_blank?(attribute, existing_product, entry)
+      existing_product.send(attribute).blank? && entry.send(attribute).blank?


please, use public_send over send. The latter does not respect private methods.

sauloperez · 2018-08-30T12:08:38Z

app/models/product_import/product_importer.rb

+        return true if entry.errors.messages.values.include? [I18n.t('admin.product_import.model.not_updatable')]
+      end
+      false
+    end


Using any? would be a bit more readable IMO.

To use #any? I could rewrite it like this:

entry.errors.messages.values.any? { |value| value == [I18n.t('admin.product_import.model.not_updatable')] }

but I think that's less clear than just array.include? object

I meant using any? with @entries. If I understood correctly, we're checking if any of the entries has an error regardless of the number of errored ones, right?

sauloperez · 2018-08-30T12:09:43Z

spec/models/product_importer_spec.rb

+        csv << ["Tomato", "And Another Enterprise", "Vegetables", "6", "5.50", "500", "Kg"]
+      end
+      File.write('/tmp/test-m.csv', csv_data)
+      file = File.new('/tmp/test-m.csv')


maiking use of Rails' tmp/ folder seems more appropriate. If something happens and you spot this file in tmp/ you know it's something related to the app.

Product import uses the rails tmp folder for it's destination of uploaded files, but this is purely for the specs to create a CSV file first on the disk, and then "upload" or use it during the test. I thought the system /tmp folder would be better, as it will get cleared automatically regardless of what machine the specs are running on.

What do you think?

I can buy the autoclearing as an added benefit 👍

sauloperez · 2018-08-30T12:17:28Z

app/models/product_import/entry_validator.rb

+    end
+
+    def non_updatable_fields
+      EntryValidator.non_updatable_fields


self can replace EntryValidator here and then I'm not sure the method is worth it.

sauloperez · 2018-08-30T12:21:48Z

app/models/product_import/entry_validator.rb

+        next if attributes_match?(attribute, existing_product, entry) || attributes_blank?(attribute, existing_product, entry)
+        mark_as_invalid(entry, attribute: display_name, error: I18n.t('admin.product_import.model.not_updatable'))
+      end
+    end


I think this could be made simpler by comparing hashes instead of iterating over the fields. You can use slice on product.attributes and then compare the resulting hash against non_updatable_fields.

There are some comparisons that need to be checked here that depend on truthyness as opposed to truth. Comparing 1 == 1.0, for instance. If I use #slice here the comparison will be strict, and won't give the correct result.

I also need to compare across the two values to see if they are both #blank, and I need it to be true if one is nil and the other is an empty string, which gives me the same problem with using #slice here.

luisramos0

👍

luisramos0 · 2018-09-05T21:45:19Z

I have staged this in https://staging.openfoodfrance.org/
For now, if any error, first place to look should be the staging server.

RachL · 2018-09-06T15:13:40Z

@luisramos0 I get the snail once I hit the import button even with a very simple file like here:
https://docs.google.com/document/d/1y_A2w0-yDz28Z4-RH6LfUZJ05zr3o4eBkXJRrFBUTIA/edit#
So, is my test crappy or do we have a pb with the server? Thanks :)

RachL · 2018-09-06T15:42:09Z

@luisramos0 FYI I've used the same file on https://staging1.openfood.com.au and there it works perfectly.

luisramos0 · 2018-09-06T16:10:40Z

I'm having a look right now.

luisramos0 · 2018-09-06T16:27:24Z

I just restarted the server (unicorn) and it's now working, at least the first step. Can you please try again? Thanks!

RachL · 2018-09-06T17:13:52Z

thx @luisramos0 working good :)

I'm not sure I've tested this right. To sum up:

yes users are warned and cannot proceed with import as they need to change file first (although the sentence used is not the one proposed by Myriam, I guess we can change this later ;) ).
I'm confused of the term "attempting to update" : does it mean having the product in the file and that's it? Or if the product has to have the exact info than in DB we are good ? If it's the latter, there is a problem somewhere has the field "description" is not accepted for existing products, even if the content is the same as in DB.
If any import featuring existing products is considered an update, why are fields like category or shipping_category not mentioned in the errors ? Are there because they are mandatory fields?
Thanks for feedback and ping @myriamboure for testing review, maybe I've just misunderstood how it works here :)

Testing notes:
https://docs.google.com/document/d/1y_A2w0-yDz28Z4-RH6LfUZJ05zr3o4eBkXJRrFBUTIA/edit#

myriamboure · 2018-09-06T18:07:27Z

Ok so we just checked again and talked with @RachL and it seems the feature is working as expected for all fields expect Description.
It seems @Matt-Yorkley that the description field is not able to recognize if it is the same or not as in DB, and always send an error if you have a description field in the file for an existing product, even if description is same. So it will prevent imports for any existing product if the column description is used in the file, even is no product-based field is touched.
So we need to fix that before merging or we will just be unable to use any file with a description column and existing products if we merge. Back to in dev @Matt-Yorkley sorry ;-)

myriamboure · 2018-09-17T22:00:07Z

@Matt-Yorkley there has been no activity on this since 11 days. It would be good to prioritize finishing product import stuff so that we can release, over starting bulk invoice printing stuff ;-) Thanks!

Matt-Yorkley · 2018-09-20T09:06:46Z

Staged on: https://staging.openfoodnetwork.org.uk

RachL · 2018-09-20T17:58:33Z

Yes, now it works :) https://docs.google.com/document/d/1y_A2w0-yDz28Z4-RH6LfUZJ05zr3o4eBkXJRrFBUTIA/edit#

@Matt-Yorkley did you install whole production database on UK staging ? Using filter on product list is sometimes very long (or with cache problems maybe....) and there is no loading signal after filtering, so you don't know if the platform is looking for products or just did not find them. IMO We should have a loading signal after filtering as well, and a sentence when no products are found. What do you think ?

Matt-Yorkley · 2018-09-20T19:12:00Z

@RachL yes, we have lots of data on UK staging. Maybe it's worth opening an issue to review the user feedback during loading on that page.

myriamboure · 2018-09-20T20:28:21Z

I have the same experience in France, before we had "no product" now we have nothing but the UX is not better... I experience same issue in France as super admin, that would be worth testing with regular user to see if it's also soooo long to charge.

myriamboure · 2018-09-24T21:46:23Z

Ok so let's merge and we'll open a new issue for loading time and UX, let's merggggge !

Matt-Yorkley force-pushed the pi/product_attributes branch from 721a3df to d554b55 Compare August 21, 2018 00:19

Matt-Yorkley added the pr-wip label Aug 21, 2018

Matt-Yorkley force-pushed the pi/product_attributes branch 2 times, most recently from fdb7c70 to be0a287 Compare August 21, 2018 12:34

Matt-Yorkley removed the pr-wip label Aug 21, 2018

Matt-Yorkley force-pushed the pi/product_attributes branch from be0a287 to ce52c01 Compare August 21, 2018 12:53

mkllnk assigned Matt-Yorkley Aug 22, 2018

Matt-Yorkley force-pushed the pi/product_attributes branch 4 times, most recently from dd36e27 to 013563b Compare August 27, 2018 17:35

sauloperez suggested changes Aug 30, 2018

View reviewed changes

luisramos0 approved these changes Aug 30, 2018

View reviewed changes

Matt-Yorkley force-pushed the pi/product_attributes branch 3 times, most recently from cc24b37 to 19a1bb9 Compare September 1, 2018 19:10

sauloperez approved these changes Sep 3, 2018

View reviewed changes

luisramos0 added the pr-staged-fr staging.coopcircuits.fr label Sep 5, 2018

RachL self-assigned this Sep 6, 2018

luisramos0 removed the pr-staged-fr staging.coopcircuits.fr label Sep 6, 2018

sigmundpetersen changed the title ~~Pi/product attributes~~ [Product Import] Product attributes Sep 17, 2018

Matt-Yorkley added 3 commits September 19, 2018 21:12

Add errors when attempting to update non-updatable fields

b57c6cf

Prefer #public_send over #send

cc98cc8

Remove unneccesary method

ebb18e9

Matt-Yorkley force-pushed the pi/product_attributes branch 2 times, most recently from d9819dd to 2e72447 Compare September 19, 2018 21:09

Add :description to non-updatable attributes list

fc68e28

Matt-Yorkley force-pushed the pi/product_attributes branch from 2e72447 to fc68e28 Compare September 19, 2018 21:30

Matt-Yorkley added the pr-staged-uk staging.openfoodnetwork.org.uk label Sep 20, 2018

RachL removed the pr-staged-uk staging.openfoodnetwork.org.uk label Sep 20, 2018

mkllnk merged commit 51f9a0a into openfoodfoundation:master Sep 25, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Product Import] Product attributes #2562

[Product Import] Product attributes #2562

Matt-Yorkley commented Aug 20, 2018 •

edited

Loading

myriamboure commented Aug 21, 2018

myriamboure commented Aug 21, 2018

Matt-Yorkley commented Aug 24, 2018

myriamboure commented Aug 24, 2018

RachL commented Aug 27, 2018

myriamboure commented Aug 27, 2018

myriamboure commented Aug 27, 2018

sauloperez Aug 30, 2018

sauloperez Aug 30, 2018

Matt-Yorkley Sep 1, 2018

sauloperez Sep 18, 2018

sauloperez Aug 30, 2018

Matt-Yorkley Sep 1, 2018 •

edited

Loading

sauloperez Sep 3, 2018

sauloperez Aug 30, 2018

sauloperez Aug 30, 2018

Matt-Yorkley Sep 1, 2018

Matt-Yorkley Sep 1, 2018

luisramos0 left a comment

luisramos0 commented Sep 5, 2018

RachL commented Sep 6, 2018

RachL commented Sep 6, 2018

luisramos0 commented Sep 6, 2018

luisramos0 commented Sep 6, 2018 •

edited

Loading

RachL commented Sep 6, 2018

myriamboure commented Sep 6, 2018

myriamboure commented Sep 17, 2018

Matt-Yorkley commented Sep 20, 2018

RachL commented Sep 20, 2018

Matt-Yorkley commented Sep 20, 2018

myriamboure commented Sep 20, 2018

myriamboure commented Sep 24, 2018

[Product Import] Product attributes #2562

[Product Import] Product attributes #2562

Conversation

Matt-Yorkley commented Aug 20, 2018 • edited Loading

What? Why?

What should we test?

Release notes

myriamboure commented Aug 21, 2018

myriamboure commented Aug 21, 2018

Matt-Yorkley commented Aug 24, 2018

myriamboure commented Aug 24, 2018

RachL commented Aug 27, 2018

myriamboure commented Aug 27, 2018

myriamboure commented Aug 27, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Matt-Yorkley Sep 1, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

luisramos0 left a comment

Choose a reason for hiding this comment

luisramos0 commented Sep 5, 2018

RachL commented Sep 6, 2018

RachL commented Sep 6, 2018

luisramos0 commented Sep 6, 2018

luisramos0 commented Sep 6, 2018 • edited Loading

RachL commented Sep 6, 2018

myriamboure commented Sep 6, 2018

myriamboure commented Sep 17, 2018

Matt-Yorkley commented Sep 20, 2018

RachL commented Sep 20, 2018

Matt-Yorkley commented Sep 20, 2018

myriamboure commented Sep 20, 2018

myriamboure commented Sep 24, 2018

Matt-Yorkley commented Aug 20, 2018 •

edited

Loading

Matt-Yorkley Sep 1, 2018 •

edited

Loading

luisramos0 commented Sep 6, 2018 •

edited

Loading