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

Add ability to specify non-existent field for combobox #28

Open
wants to merge 7 commits into
base: main
Choose a base branch
from

Conversation

pranavbabu
Copy link
Contributor

@pranavbabu pranavbabu commented Feb 10, 2024

The reason for this PR that sometimes model can have json type fields with nested attributes and calling form.object.public_send(field_name) on it will fail. I used same check as rails do here

At the moment calling this fails

  <%= form.combobox '[existent_field][non_existent_field]' %>

@pranavbabu pranavbabu changed the title Add ability to specify non-exist field for combobox Add ability to specify non-existent field for combobox Feb 10, 2024
Copy link
Owner

@josefarias josefarias left a comment

Choose a reason for hiding this comment

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

Great catch. Thanks! Can you add a test that would fail without these changes and pass with the changes, please?

Happy to merge after that.

app/presenters/hotwire_combobox/component.rb Show resolved Hide resolved
@pranavbabu
Copy link
Contributor Author

Should it be system test?

@josefarias
Copy link
Owner

Either a system or unit/integration test would be fine. I don't think we have tests for the main component. But we do have tests for the listbox option presenter. You can create tests based on that, if you wanted to test the component by itself.

Thanks!

Copy link
Owner

@josefarias josefarias left a comment

Choose a reason for hiding this comment

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

Thanks for adding the tests! Left some comments after a first pass.

test/presenters/hotwire_combobox/component_test.rb Outdated Show resolved Hide resolved

class HotwireCombobox::ComponentTest < ApplicationViewTestCase
setup do
@view = ActionView::Base.new(ActionController::Base.view_paths, {}, ActionController::Base.new)
Copy link
Owner

Choose a reason for hiding this comment

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

Because we're inheriting from ApplicationViewTestCase the view object should be accessible by just calling #view anywhere in the test cases. No need to initialize here.

We use this in the Listbox::Option tests in the #render method.

form: mock_form(form_object: OpenStruct.new("#{field_name}": existent_field_on_model)),
)

assert_equal component.hidden_field_attrs, { id: "#{field_name}_id-hw-hidden-field",
Copy link
Owner

Choose a reason for hiding this comment

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

Instead of asserting all attrs, let's just assert that the attr hash includes the correct name and value.

@@ -10,6 +10,21 @@ def assert_attrs(tag, tag_name: :input, **attrs)
assert_match /<#{tag_name}.* #{attrs}/, tag
end

def mock_form(form_object: OpenStruct.new)
Copy link
Owner

Choose a reason for hiding this comment

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

I understand the instinct to stub this out in the spirit of encapsulation.

In this case I'd rather not stub the form builder methods since they don't "belong" to us.

Let's test the real thing instead. SQLite supports JSON columns. Let's add one to one of the fixtures instead and use that in the tests instead of mocking the form.

value: nil }

end
end
Copy link
Owner

Choose a reason for hiding this comment

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

Suggested change
end
end

Comment on lines +9 to +10
existent_field_on_model = "Peter"
field_name = :name
Copy link
Owner

Choose a reason for hiding this comment

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

I think the test is clear enough that we don't need these explaining variables. Let's inline the values instead. Same goes for the other tests.

It's okay to introduce a bit of harmless repetition in order to keep less things in mind while reading these tests 👍

test/presenters/hotwire_combobox/component_test.rb Outdated Show resolved Hide resolved
@josefarias
Copy link
Owner

Also, please rebase or merge main back in so we can get the changes from your other merged PR

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

Successfully merging this pull request may close these issues.

2 participants