search

solidusio-contrib / solidus_importer

Solidus importer extension to migrate data from other eCommerce systems
BSD 3-Clause "New" or "Revised" License
15 stars 30 forks source link

Provide better error messages if slug contains illegal characters #78

Open benjaminwil opened 3 years ago

benjaminwil commented 3 years ago

Processors::Product finds or initializes a new product based on the @data["Handle"]. Then, later on, it assigns the slug again to the @data["Handle"]. This is normally not a problem. In a recent import, however, we came across a handle/slug data issue that was difficult to debug. We think this is due to how Solidus generates valid slugs from invalid input.

Reproduction steps

Here's how you can reproduce:

  1. In your product import CSV, add a Handle with an invalid character. i.e. arts-&-crafts-box
  2. Run the product import.
  3. Check the slug of the created product. (It should be arts-crafts-box.)
  4. Run the import again.

Current behaviour

A new product is initialized for arts-&-crafts-box, then the slug is re-generated to be arts-crafts-box, and we end up with a ActiveRecord::InvalidRecord Slug has already been taken error.

Expected behaviour

The arts-crafts-box product is found and loaded. No new product is initialized.

Solution

I am not sure what the best solution would be. Perhaps there is a more nuanced problem in Solidus where Product.find_or_initialize_by(slug: slug) is not the best way to find or initialize a product.

I think at the very least we could validate that the slug input in the CSV would be a valid slug as written. In this example, the first import would complete successfully, but the user would expect their product to be at /product/arts-&-crafts-box and they might be confused and think their import didn't work. The user's second import would fail, and it would be hard for them to figure out why.

kennyadsl commented 3 years ago

Hey Ben! What if we just report this as a specific importing error? Users will figure out why this happens and change the CSV based on what they want to do. What do you think?

benjaminwil commented 3 years ago

I think that would be a good solution.

stale[bot] commented 3 years ago

This issue has been automatically marked as stale because it has not had recent activity. It might be closed if no further activity occurs. Thank you for your contributions.