Show any tax category errors when saving variants #12486

dacook · 2024-05-16T01:30:37Z

What? Why?

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

If the tax category is required, but not chosen on a variant, the error is now clearer so you know what's wrong:

on bulk products screen: an error message now appears below the dropdown
on variant edit screen: the label is highlighted red.

I now realise this was due to bad setup (see below), and would like to update the interface to avoid the root cause. But until then at least we have better error messaging.

It would have been nice to highlight the Tomselect and Select2 dropdowns too, but there's a couple of problems with that..
.. And we'll have to fix unit value another day.

What should we test?

Setup
I now realise that this is bad setup, but it's currently possible, and it's not intuitive:

Visit /admin/tax_settings/edit, select "products require tax category" and save
Visit /admin/tax_categories and ensure there is no "default"

Test with admin_style_v3 enabled
4. Visit /admin/products
5. select "None" for tax category on a variant, and attempt save
6. Error message should explain why it did not save.

Try both: with admin_style_v3 enabled, also without.
7. Visit /admin/products/x/variants/x/edit
8. select "None" for tax category, and attempt save.

Release notes

It's only really noticeable on admin_style_v3

Changelog Category (reviewers may add a label for the release notes):

User facing changes
API changes (V0, V1, DFC or Webhook)
Technical changes only
Feature toggled

The title of the pull request will be included in the release notes.

Documentation updates

The config requirements may already be documented in the guide, I don't know. But we really need to make the app more intuitive anyway.

Now you can click on the text to tick/untick the box. I've been wanting to do that for ages.

dacook · 2024-05-16T01:47:56Z

Need to fix this spec:
https://github.com/openfoodfoundation/openfoodnetwork/blob/master/engines/dfc_provider/spec/requests/supplied_products_spec.rb#L89

I think this spec is expecting the tax category to be chosen automatically, but I think my change here revealed that it's not.

Hmm, it is "chosen" here: https://github.com/openfoodfoundation/openfoodnetwork/blob/master/app/models/spree/variant.rb#L172-L174

    def tax_category
      super || TaxCategory.find_by(is_default: true)
    end

This is why the spec was passing before. But I guess it's not saved yet.

mkllnk

I think this spec is expecting the tax category to be chosen automatically, but I think my change here revealed that it's not.

No, the default tax category is not saved on the variant. But it's loaded when needed. Before, the validation was checking that it had a tax category. And it did "have" the default tax category without being associated to it. But it doesn't have a tax category id. I know it's confusing. I worked on it last and just kept the existing behaviour because I was worried about breaking anything.

mkllnk · 2024-05-16T03:24:19Z

app/models/spree/variant.rb

@@ -64,7 +64,7 @@ class Variant < ApplicationRecord
    validates_lengths_from_database
    validate :check_currency
    validates :price, numericality: { greater_than_or_equal_to: 0 }, presence: true
-    validates :tax_category, presence: true,
+    validates :tax_category_id, presence: true,


That's a shame. This format has a few downsides, for example a new tax category can't be associated to a variant before it's saved. So instead of letting Rails save the record, we have to save it first. That shouldn't be in issue in our model but may be for test data.

Yes, I thought it was a shame too, but I thought it's also more compatible with rails forms which is a good thing.
The test data issue is the only downside I can think of, can you think of any others?

mkllnk · 2024-05-16T03:32:38Z

app/views/admin/products_v3/_variant_row.html.haml

@@ -48,6 +48,7 @@
      selected_option: variant.primary_taxon_id,
      aria_label: t('.category_field_name'),
      placeholder_value: t('admin.products_v3.filters.search_for_categories')))
+  = error_message_on variant, :primary_taxon_id


We don't have a spec on this, do we?

🙈 You're right, I need to add that.

Wait, for primary_taxon_id, I don't think there is a way to trigger an error.
I put this in for future-proofing, which I realise is generally a bad idea, but it seemed likely that if someone added a validation condition, they wouldn't think to add this.
It's important on this screen, because we don't have an error summary at the top of the screen.

Given that it's untested, there's a very real possibility it doesn't work (eg, any validations would be on the association, not the id!).
So I will remove this.

mkllnk · 2024-05-16T03:53:10Z

app/views/admin/products_v3/_variant_row.html.haml

@@ -56,6 +57,7 @@
      include_blank: t('.none_tax_category'),
      aria_label: t('.tax_category_field_name'),
      placeholder_value: t('.search_for_tax_categories')))
+  = error_message_on variant, :tax_category_id


I just had a play with this and it seemed to work to go back to validating tax_category and just changing the error message here, without changing the input above:

Suggested change

= error_message_on variant, :tax_category_id

= error_message_on variant, :tax_category

If I require a tax category but don't have a default then this will display an error.

The grey zone is to require a tax category, set a default and then select "None". Then it will look like it doesn't have a tax category but the default will be applied during checkout. That's another problem though.

Nice work, I didn't think of that.

It won't work for the above select (ie <select name="...variant[tax_category_id]">). That means we won't get the field_with_errors wrapper class to add a red border on the field.
But that's an acceptable tradeoff 👍

Note to self: also add spec for this error.

dacook

Thanks for your help, I'll try proceeding with error_message_on variant, :tax_category

dacook · 2024-05-16T04:00:15Z

app/views/admin/products_v3/_variant_row.html.haml

@@ -48,6 +48,7 @@
      selected_option: variant.primary_taxon_id,
      aria_label: t('.category_field_name'),
      placeholder_value: t('admin.products_v3.filters.search_for_categories')))
+  = error_message_on variant, :primary_taxon_id


Wait, for primary_taxon_id, I don't think there is a way to trigger an error.
I put this in for future-proofing, which I realise is generally a bad idea, but it seemed likely that if someone added a validation condition, they wouldn't think to add this.
It's important on this screen, because we don't have an error summary at the top of the screen.

dacook · 2024-05-16T04:04:16Z

app/views/admin/products_v3/_variant_row.html.haml

@@ -56,6 +57,7 @@
      include_blank: t('.none_tax_category'),
      aria_label: t('.tax_category_field_name'),
      placeholder_value: t('.search_for_tax_categories')))
+  = error_message_on variant, :tax_category_id


Nice work, I didn't think of that.

It won't work for the above select (ie <select name="...variant[tax_category_id]">). That means we won't get the field_with_errors wrapper class to add a red border on the field.
But that's an acceptable tradeoff 👍

dacook · 2024-05-16T04:05:08Z

app/views/admin/products_v3/_variant_row.html.haml

@@ -56,6 +57,7 @@
      include_blank: t('.none_tax_category'),
      aria_label: t('.tax_category_field_name'),
      placeholder_value: t('.search_for_tax_categories')))
+  = error_message_on variant, :tax_category_id


Note to self: also add spec for this error.

The validation message is on "tax_category", so labels and error messages can use that to show the error state. But the select field has to be "tax_category_id" to work.

I developed this while going down a slightly different path. I tested it visually, but the case I tested doesn't exist. I'm confident it will work if we ever have an error on another select though.

mkllnk

Nice!

rioug

Nice !

filipefurtad0 · 2024-05-21T20:18:51Z

Hey @dacook ,

Thank you for working on this one.

First, I've followed your suggested tests. After staging the PR, and with a Requires tax category option ticked:

And no default tax category (all set to false), and

with admin_style_v3 enabled:

Visit /admin/products
Select "None" for tax category on a variant, and attempt save
Error message should explain why it did not save

This works as expected: the changed variant is highlighted. In fact, there are several other variants with None as a tax category, and these are displayed as well:

Clicking on Discard changes reverts the change for the edited variant, and the error messages disappear. It should be noticed though, that the other 7 variants are left unchanged (they are still having a None tax category).

Then, repeated this for admin_style_v3 disabled:

In this case, the only offending variant is the edited variant, although other variants also have None as a Tax Category.

Summary
With admin_style_v3 enabled -> 8 offending variants.
With admin_style_v3 disabled -> 1 offending variants.

I think this is because of variants on a previous state, which I'm not sure about.

So, after this test, I've tried to look for the related scenario, in which we had the warning about edited variants, but no possibility to remove the banner. This corresponds to bug #12473, i.e., with admin_style_v3 enabled.

With a default tax category -> no change in behavior

Going through pagination I spotted the Discard changes warning is still appears and the responsible variants are not highlighted (in this case, I could figure out which one was responsible for the error, although there are other variants with None ):

Clicking Discard changes reloads the page (after confirmation, as before) but does not remove the warning. . Is this not a legitimate setup in production? I can still find plenty of such variants on staging-AU, and am unclear on the root cause. To put it in another way, I'm not sure #12473 should be closed?

Without a default tax category -> a change is observed

After setting all tax categories as non-default, indeed there is a bug fix:

we can see the highlighted fields, showing which tax category needs to be updated
we can discard changes: the button works, at least in the sense that it removes the banner. However, all variants are left unchanged, i.e., there is no revert to a previous / allowed state.

Summary

i) I think this PR highlights variants which have a None tax category. So I'd say it's good to merge.
ii) I'm not sure we've totally understood/fixed/prevented #12473, so I'm a bit weary of having this issue closed. I have not checked production servers - I'm not sure this is visible on the data structure - but are we sure we don't have such variants "corrupt" in production and that enabling BUU won't surface the bug?

I'd vote for merging. Let me know what you think @dacook . Thanks! 🙏

dacook · 2024-05-21T23:30:06Z

Hi @filipefurtad0 , thanks for looking into this.

I agree, this change doesn't solve the root cause of #12473.
However it does help discover variants with an invalid tax category, and so helps work around it, in some cases. Clearly not all cases though :(

You also revealed that the old bulk screen only validates changed records, while the new screen validates all records on the screen. I've been wanting to optimise that since the beginning, will add a tech debt issue to track it.

So I think still need to solve that issue #12473. If it is due to corrupt data, then we should still try to handle it gracefully.

I will merge this one.

Associate label with checkbox

412c110

Now you can click on the text to tick/untick the box. I've been wanting to do that for ages.

dacook changed the title ~~Reveal tax category errors~~ Show any tax category errors when saving variants May 16, 2024

dacook self-assigned this May 16, 2024

dacook force-pushed the tax-category-12480 branch from a2cd1a5 to 8f6f85d Compare May 16, 2024 01:47

dacook requested a review from mkllnk May 16, 2024 02:00

mkllnk reviewed May 16, 2024

View reviewed changes

dacook commented May 16, 2024

View reviewed changes

dacook added 2 commits May 16, 2024 14:19

Show error messages and highlight for bulk editing

3e1e7d3

The validation message is on "tax_category", so labels and error messages can use that to show the error state. But the select field has to be "tax_category_id" to work.

Display red border around invalid selects

e0c60aa

I developed this while going down a slightly different path. I tested it visually, but the case I tested doesn't exist. I'm confident it will work if we ever have an error on another select though.

dacook force-pushed the tax-category-12480 branch from 8f6f85d to e0c60aa Compare May 16, 2024 05:20

dacook requested a review from mkllnk May 16, 2024 06:12

mkllnk approved these changes May 16, 2024

View reviewed changes

rioug added the feature toggled These pull requests' changes are invisible by default and are grouped in release notes label May 20, 2024

rioug approved these changes May 20, 2024

View reviewed changes

filipefurtad0 self-assigned this May 21, 2024

filipefurtad0 added the pr-staged-au staging.openfoodnetwork.org.au label May 21, 2024

filipefurtad0 added feedback-needed and removed pr-staged-au staging.openfoodnetwork.org.au labels May 21, 2024

dacook merged commit b2078c2 into openfoodfoundation:master May 21, 2024
56 checks passed

This was referenced May 22, 2024

[BUU] Avoid validating all records when saving #12503

Closed

[BUU] Dismissing changes is not working #12473

Closed

filipefurtad0 removed the feedback-needed label May 23, 2024

dacook mentioned this pull request May 27, 2024

Do not commit to db unchanged products in bulk save #12515

Merged

4 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Show any tax category errors when saving variants #12486

Show any tax category errors when saving variants #12486

dacook commented May 16, 2024 •

edited

dacook commented May 16, 2024 •

edited

mkllnk left a comment

mkllnk May 16, 2024

dacook May 16, 2024

mkllnk May 16, 2024

dacook May 16, 2024

dacook May 16, 2024

dacook May 16, 2024

mkllnk May 16, 2024

dacook May 16, 2024

dacook May 16, 2024

dacook left a comment

dacook May 16, 2024

dacook May 16, 2024

dacook May 16, 2024

mkllnk left a comment

rioug left a comment

filipefurtad0 commented May 21, 2024

dacook commented May 21, 2024

	= error_message_on variant, :tax_category_id
	= error_message_on variant, :tax_category

Show any tax category errors when saving variants #12486

Show any tax category errors when saving variants #12486

Conversation

dacook commented May 16, 2024 • edited

What? Why?

What should we test?

Release notes

Documentation updates

dacook commented May 16, 2024 • edited

mkllnk left a 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

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

dacook left a 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

mkllnk left a comment

Choose a reason for hiding this comment

rioug left a comment

Choose a reason for hiding this comment

filipefurtad0 commented May 21, 2024

dacook commented May 21, 2024

dacook commented May 16, 2024 •

edited

dacook commented May 16, 2024 •

edited