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

Error 500 when product category left blank #12591

Closed
filipefurtad0 opened this issue Jun 20, 2024 · 3 comments · Fixed by #12671
Closed

Error 500 when product category left blank #12591

filipefurtad0 opened this issue Jun 20, 2024 · 3 comments · Fixed by #12671
Assignees
Labels
bug-s3 The bug is stopping a critical or non-critical feature but there is a usable workaround. good first issue regression Tagging any identified regressions

Comments

@filipefurtad0
Copy link
Contributor

Description

This seems to be a somewhat recent regression:
leaving the product category empty while creating a product returns an error 500.

Expected Behavior

leaving the product category empty while creating should display a message to the user, no error 500 should be seen - See pic below, taken from staging-UK which has not been updated for quite a while now:

image

Actual Behaviour

Leaving the product category empty while creating a product returns an error 500.

Steps to Reproduce

  1. Log in as an enterprise user
  2. Visit the products page and click "New Product"
  3. Fill in all the mandatory fields except Product Category
  4. Click "Create"

Animated Gif/Screenshot

Screencast.from.19-06-24.14_47_17.webm

Workaround

Fill in product category before clicking "Create"

Severity

bug-s3: a feature is broken but there is a workaround

Your Environment

  • Version used: v4.4.52
  • Browser name and version: Firefox 126.0.2
  • Operating System and version (desktop or mobile): Ubuntu 22.04

Possible Fix

@filipefurtad0 filipefurtad0 added the bug-s3 The bug is stopping a critical or non-critical feature but there is a usable workaround. label Jun 20, 2024
@github-project-automation github-project-automation bot moved this to All the things 💤 in OFN Delivery board Jun 20, 2024
filipefurtad0 added a commit to filipefurtad0/openfoodnetwork that referenced this issue Jun 20, 2024
@filipefurtad0 filipefurtad0 added the regression Tagging any identified regressions label Jun 20, 2024
@github-project-automation github-project-automation bot moved this to To triage (By the maintainers) in Welcome New Developers! Jun 20, 2024
rioug added a commit that referenced this issue Jun 24, 2024
@RachL RachL moved this from To triage (By the maintainers) to Frontend Easy in Welcome New Developers! Jul 9, 2024
@wandji20
Copy link
Contributor

Hi
After giving this one a try I noticed we can create products without passing values for product supplier or category and would not get the mentioned 500 error.
I am not sure if I am missing something, but it makes sense that the request is successful since the ensure_standard_variant callback in app/models/spree/product.rb is only triggered after successfully creating a product object.
However, this callback fails to create the product variant when the supplier or category is missing in the product create params.
We can later create a variant for the product from the products table.

Ideally, I would change the callback to be called before the product is saved but I see we have a different approach to setting variant supplier and category (primary_taxon) when doing imports.

  • self.import_product in engines/dfc_provider/app/services/supplied_product_builder.rb
  • save_new_product in app/models/product_import/entry_processor.rb
  • mark_as_new_product in app/models/product_import/entry_validator.rb

This approach may need some changes everywhere we are creating products and I don't know if it's important to have a variant created when creating a product.

Another possible solution will be to conditionally perform validation (presence) of supplier_id and primary_taxon_id at the controller level to include errors in the product error object.
This will only work when creating a product from the admin dashboard.

@rioug
Copy link
Collaborator

rioug commented Jul 16, 2024

Thanks for looking in to this @wandji20 . If you want a bit of context you can look at #9069 .In short we are simplifying our data model, we are moving pretty much every thing to the variant. But we are yet to change the UI/UX, hence why we are creating a variant when creating a new product. But there is nothing enforcing that a product has to have at least one variant.

Ideally, I would change the callback to be called before the product is saved

You can't really do that because you need a product to create a variant. But I think we could do something similar by having a after_validation callback on create that would manually check if we have a supplier_id and a primary_taxon_id and raise an error if anything is missing.
Would do you like to give that ago ?
It might have some unforeseen consequences and break a lot of things, but if this is the case we can revisit and maybe use your last suggestion, ie: doing the validation at the controller level.

@wandji20
Copy link
Contributor

Thanks for explaining @rioug
Seems to me it will make sense to use (your suggestion) an after_validation callback on create that would manually check if we have a supplier_id and a primary_taxon_id since it will be line this plan for the product model.

I will give it a shot and reach out if I doubt anything

@dacook dacook moved this from Frontend Easy to In progress in Welcome New Developers! Jul 17, 2024
@dacook dacook moved this from All the things 💤 to In Progress ⚙ in OFN Delivery board Jul 17, 2024
@github-project-automation github-project-automation bot moved this from In progress to Done in Welcome New Developers! Aug 6, 2024
@github-project-automation github-project-automation bot moved this from In Progress ⚙ to Done in OFN Delivery board Aug 6, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug-s3 The bug is stopping a critical or non-critical feature but there is a usable workaround. good first issue regression Tagging any identified regressions
Projects
Status: Done
4 participants