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.

Sign up for GitHub

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

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

Open
wants to merge 6 commits into
base: master
Choose a base branch
from
Open

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

Changes from 1 commit
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
7d5bb4a
Added the :selected option with the default tax category
MrBowmanXD Dec 1, 2024
d95bb77
Fixed the logic and moved the code to a helper file
MrBowmanXD Dec 1, 2024
980f86c
Fixed the lint errors
MrBowmanXD Dec 2, 2024
3199118
Removed the :selected attribute when the tax category is not required…
MrBowmanXD Dec 3, 2024
5def2b5
Added a spec file in order to test the tax categories helper
MrBowmanXD Dec 8, 2024
0d4904d
Small change in spec file
MrBowmanXD Dec 9, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
27 changes: 20 additions & 7 deletions app/helpers/spree/admin/tax_categories_helper.rb
View file
Open in desktop
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 ⭐

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hello, thank you. I will check it out.

Original file line number Diff line number Diff line change
@@ -1,8 +1,21 @@
module Spree::Admin::TaxCategoriesHelper
def tax_category_dropdown_options(require_tax_category)
{
:include_blank => Spree::Config.products_require_tax_category ? false : t(:none),
selected: Spree::Config.products_require_tax_category ? Spree::TaxCategory.find_by(is_default: true)&.id : nil
}
# frozen_string_literal: true

module Spree
module Admin
module TaxCategoriesHelper
def tax_category_dropdown_options(require_tax_category)
if require_tax_category
{
include_blank: false,
selected: Spree::TaxCategory.find_by(is_default: true)&.id
}
else
{
include_blank: t(:none),
selected: nil
}
end
end
end
end
end
end
Loading