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

Provide better error messages if slug contains illegal characters #78

Open
benjaminwil opened this issue Aug 24, 2021 · 3 comments
Open
Labels

Comments

@benjaminwil
Copy link
Contributor

benjaminwil commented Aug 24, 2021

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
Copy link
Member

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
Copy link
Contributor Author

I think that would be a good solution.

@stale
Copy link

stale bot commented Oct 30, 2021

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.

@stale stale bot added the stale label Oct 30, 2021
@jarednorman jarednorman added pinned and removed stale labels Nov 1, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

3 participants