Added the :selected option with the default tax category #12989 #13009

MrBowmanXD · 2024-12-01T15:26:56Z

This pull request solves the default tax category not showing problem when you create a new product in the admin panel

Closes Default tax category is not set #12989

Added the following code:

selected: Spree::TaxCategory.find_by(is_default: true)&.id

in the _tax_category_form.html.haml file.

What should we test?

1.Open product creation screen.
2.See the tax category is the one that is set to default.

Release notes

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.

Dependencies

Documentation updates

chahmedejaz

Thank you so much for working on this. I was wondering if we could handle the use case shared below.

chahmedejaz · 2024-12-01T19:04:01Z

app/views/spree/admin/products/_tax_category_form.html.haml

@@ -1,5 +1,5 @@
 = f.field_container :tax_category_id do
  = f.label :tax_category_id, t(:tax_category)
  %br
-  = f.collection_select(:tax_category_id, Spree::TaxCategory.all, :id, :name, {:include_blank => Spree::Config.products_require_tax_category ? false : t(:none)}, {:class => "select2 fullwidth"})
+  = f.collection_select(:tax_category_id, Spree::TaxCategory.all, :id, :name, {:include_blank => Spree::Config.products_require_tax_category ? false : t(:none), selected: Spree::TaxCategory.find_by(is_default: true)&.id}, {:class => "select2 fullwidth"})


This looks right, however, I was wondering if this way users might end up unintentionally selecting the tax category when it's not required. To prevent this, I think nothing should be selected if the products don't require a tax category (Spree::Config.products_require_tax_category = false). Otherwise, the default category should be selected.

Given that we are incorporating the above logic, is it possible we could move this logic to the view helper to clean up the logic from the view? Thanks.

I have the logic requested above (in case Spree::Config.products_require_tax_category = false it shows nothing, otherwise it selects the default category). Don't know where to put the code in the helper tho. Can i put it in tax_helper.rb file in the helpers folder?

Moved to a new file (app/helpers/spree/admin/tax_categories_helper.rb) the new logic requested, hope it's within the structure of the project.

chahmedejaz · 2024-12-01T19:12:40Z

P.S. I think some specs are failing due to this change, can you please look into those as well? Thanks.

MrBowmanXD · 2024-12-01T20:02:17Z

I think the failing spec might be related to the tax category existence so maybe the updated logic might solve it, need to check this idea.

MrBowmanXD · 2024-12-02T13:36:03Z

I think the failing spec might be related to a new :selected attribute added to the view (selected: Spree::TaxCategory.find_by(is_default: true)&.id)

chahmedejaz

We are getting there :)
Apparently, we have lint issues with the newly added helper, are you happy to look into these? Thanks.

chahmedejaz · 2024-12-02T14:24:58Z

I think the failing spec might be related to a new :selected attribute added to the view (selected: Spree::TaxCategory.find_by(is_default: true)&.id)

Yes, however, we need to validate the specs whether the code is breaking some usecase in the specs or the specs need to be updated accordingly :)

MrBowmanXD · 2024-12-02T20:53:46Z

Let me check these lint issues.

MrBowmanXD · 2024-12-02T21:23:40Z

We are getting there :) Apparently, we have lint issues with the newly added helper, are you happy to look into these? Thanks.

Was able to resolve the lint issues in the code.

MrBowmanXD · 2024-12-03T08:39:23Z

I could remove :selected from the else clause to pass the failing spec. (Because before my solution there was no :Selected attribute)

… (like original version)

chahmedejaz

Thanks for working through this. 👍🏻
Now we will wait for the 2nd review. 🙂

rioug · 2024-12-04T00:33:12Z

app/helpers/spree/admin/tax_categories_helper.rb

It's a good practise to add unit test for your code, even if the logic seems trivial. Could you add some test for the new helper ? Have a look here for some inspiration https://github.com/openfoodfoundation/openfoodnetwork/blob/master/spec/helpers/admin/products_helper_spec.rb

I can add some tests, sorry if i did not start with a test, i am still learning Rails.

I would like to ask if you guys know any good resource for rspec in rails? i will search for it as well, thanks in advance

No worries 🙂
I find this playlist helpful, maybe it would help:
https://www.youtube.com/watch?v=2jX-FLcznDE&list=PLS6F722u-R6Ik3fbeLXbSclWkT6Qsp9ng

Please let us know you face any issues, no worries ⭐

Hello, thank you. I will check it out.

rioug

Thanks for adding the spec ! Just one last change and we should be good to go.

rioug · 2024-12-08T22:28:40Z

spec/helpers/spree/admin/tax_categories_helper_spec.rb

+    context 'when products do not require a tax category' do
+      it 'returns include_blank as the translated "none" string' do
+        options = helper.tax_category_dropdown_options(false)
+        expect(options[:include_blank]).to eq(I18n.t(:none))


It's better to actually check the translation is working,ie:

Suggested change

expect(options[:include_blank]).to eq(I18n.t(:none))

expect(options[:include_blank]).to eq("None")

chahmedejaz

Thanks for your hardwork, @MrBowmanXD. Looks good to me. 👍

Now we will have a round of manual testing by our testing team before the merge. Thanks.

Added the :selected option with the default tax category

7d5bb4a

github-actions bot assigned MrBowmanXD Dec 1, 2024

chahmedejaz requested changes Dec 1, 2024

View reviewed changes

Fixed the logic and moved the code to a helper file

d95bb77

chahmedejaz requested changes Dec 2, 2024

View reviewed changes

Fixed the lint errors

980f86c

Removed the :selected attribute when the tax category is not required…

3199118

… (like original version)

MrBowmanXD requested a review from chahmedejaz December 3, 2024 10:08

chahmedejaz approved these changes Dec 3, 2024

View reviewed changes

rioug added the user facing changes Thes pull requests affect the user experience label Dec 4, 2024

rioug self-requested a review December 4, 2024 00:29

rioug requested changes Dec 4, 2024

View reviewed changes

Added a spec file in order to test the tax categories helper

5def2b5

rioug requested changes Dec 8, 2024

View reviewed changes

Small change in spec file

0d4904d

sigmundpetersen requested a review from rioug December 9, 2024 09:08

rioug approved these changes Dec 9, 2024

View reviewed changes

rioug requested a review from chahmedejaz December 9, 2024 22:47

chahmedejaz approved these changes Dec 10, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added the :selected option with the default tax category #12989 #13009

Added the :selected option with the default tax category #12989 #13009

MrBowmanXD commented Dec 1, 2024 •

edited by chahmedejaz

Loading

chahmedejaz left a comment •

edited

Loading

chahmedejaz Dec 1, 2024 •

edited

Loading

MrBowmanXD Dec 1, 2024

MrBowmanXD Dec 1, 2024

chahmedejaz commented Dec 1, 2024

MrBowmanXD commented Dec 1, 2024

MrBowmanXD commented Dec 2, 2024

chahmedejaz left a comment

chahmedejaz commented Dec 2, 2024

MrBowmanXD commented Dec 2, 2024

MrBowmanXD commented Dec 2, 2024

MrBowmanXD commented Dec 3, 2024 •

edited

Loading

chahmedejaz left a comment

rioug Dec 4, 2024

MrBowmanXD Dec 4, 2024

MrBowmanXD Dec 4, 2024

chahmedejaz Dec 4, 2024

MrBowmanXD Dec 4, 2024

rioug left a comment

rioug Dec 8, 2024

chahmedejaz left a comment •

edited

Loading

	expect(options[:include_blank]).to eq(I18n.t(:none))
	expect(options[:include_blank]).to eq("None")

Added the :selected option with the default tax category #12989 #13009

Are you sure you want to change the base?

Added the :selected option with the default tax category #12989 #13009

Conversation

MrBowmanXD commented Dec 1, 2024 • edited by chahmedejaz Loading

What should we test?

Release notes

Dependencies

Documentation updates

chahmedejaz left a comment • edited Loading

Choose a reason for hiding this comment

chahmedejaz Dec 1, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chahmedejaz commented Dec 1, 2024

MrBowmanXD commented Dec 1, 2024

MrBowmanXD commented Dec 2, 2024

chahmedejaz left a comment

Choose a reason for hiding this comment

chahmedejaz commented Dec 2, 2024

MrBowmanXD commented Dec 2, 2024

MrBowmanXD commented Dec 2, 2024

MrBowmanXD commented Dec 3, 2024 • edited Loading

chahmedejaz 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

rioug left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chahmedejaz left a comment • edited Loading

Choose a reason for hiding this comment

MrBowmanXD commented Dec 1, 2024 •

edited by chahmedejaz

Loading

chahmedejaz left a comment •

edited

Loading

chahmedejaz Dec 1, 2024 •

edited

Loading

MrBowmanXD commented Dec 3, 2024 •

edited

Loading

chahmedejaz left a comment •

edited

Loading