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

[BUU] Variants with "none" tax category have an invisible error #12480

Closed
dacook opened this issue May 15, 2024 · 8 comments · Fixed by #12486
Closed

[BUU] Variants with "none" tax category have an invisible error #12480

dacook opened this issue May 15, 2024 · 8 comments · Fixed by #12486
Assignees

Comments

@dacook
Copy link
Member

dacook commented May 15, 2024

Description

When this option is ticked:
Screenshot 2024-05-15 at 4 52 04 pm

But variants still have "None" selected, they are in an invalid state.
The BUU PRoducts screen shows a summary that they are "modified", yet shows no indication of what's wrong. When attempting save, they are counted as erorrs, but don't show any messages explaining why.

image

Discovered in:

Expected Behavior

I don't think they should show "modified" at all.

When saving, an error is expected for each variant that has "None" selected. There should be a message appearing below the Tax Category dropdown.

Actual Behaviour

Steps to Reproduce

as above

Animated Gif/Screenshot

Workaround

Severity

Your Environment

  • Version used:
  • Browser name and version:
  • Operating System and version (desktop or mobile):

Possible Fix

@dacook
Copy link
Member Author

dacook commented May 15, 2024

For reference, this is what happens on the Variant Edit screen: the error message appears at the top, but not at the field. That's probably why we dont' see it on BUU. Hopefully we can fix this at the model level.

Screenshot 2024-05-15 at 5 17 03 pm

@RachL
Copy link
Contributor

RachL commented May 15, 2024

@dacook when product require tax category "none" is an option in the dropdown, so I'm not sure I understand why this would give an invalid state? We need to be able to select "none" even if product require tax category.

@dacook
Copy link
Member Author

dacook commented May 16, 2024

Hi @RachL , the "None" option is a special blank option which saves as empty (no tax category).

When "products require tax category" is ticked, the blank "None" option is not valid. Why don't we hide it in that case? I had a look but it looked complicated to hide from the dropdown.

So instead I went ahead and fixed the fields to properly show the error messages, see PR.

I also had a look at the default tax category. I think we need to improve how this is handled (e.g. in this case perhaps the system could automatically save it with the default tax category if there is one. I think we discussed this before but I can't remember exactly...

@dacook
Copy link
Member Author

dacook commented May 16, 2024

According to @mkllnk, when you select "products require tax category", you must also select a default tax category.

In that case, we should never allow "None" selected, and instead select the default. I think we need to discuss this more..

@dacook
Copy link
Member Author

dacook commented May 16, 2024

I think my PR is worth proceeding with for now.
For the future, I've suggested how we can improve the settings screen to avoid bad config:
https://openfoodnetwork.slack.com/archives/C01T75H6G0Z/p1715839142389029

@RachL
Copy link
Contributor

RachL commented May 16, 2024

@dacook on staging FR (and in production as well) I'm able to select "none" on variants even though the tax category is required 🤷 I thought it maybe had something to do with enterprise settings, but no.

is there anything else that comes into play?

@RachL
Copy link
Contributor

RachL commented May 16, 2024

we should never allow "None" selected

FYi not only do we let people choose "none", but it's also the default option selected when creating new products.

@dacook
Copy link
Member Author

dacook commented May 20, 2024

I'm able to select "none" on variants even though the tax category is required

Yes it seems strange, we should change that! The empty "None" option may need to still appear in case it is already selected on the variant. But I think it could be greyed out and disabled, preventing you from saving it.

is there anything else that comes into play?

Looking at the code, it looks like it should select the "default" tax category when creating a new variant. But strangely that's not working in my testing..

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

Successfully merging a pull request may close this issue.

2 participants