Add ability to specify non-existent field for combobox - #28
Conversation
pranavbabu
changed the title
josefarias
left a comment
josefarias
left a comment
There was a problem hiding this comment.
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.
|
Should it be system test? |
|
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! |
josefarias
left a comment
josefarias
left a comment
There was a problem hiding this comment.
Thanks for adding the tests! Left some comments after a first pass.
|
|
||
| class HotwireCombobox::ComponentTest < ApplicationViewTestCase | ||
| setup do | ||
| @view = ActionView::Base.new(ActionController::Base.view_paths, {}, ActionController::Base.new) |
There was a problem hiding this comment.
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", |
There was a problem hiding this comment.
Instead of asserting all attrs, let's just assert that the attr hash includes the correct name and value.
| assert_match /<#{tag_name}.* #{attrs}/, tag | ||
| end | ||
|
|
||
| def mock_form(form_object: OpenStruct.new) |
There was a problem hiding this comment.
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 No newline at end of file |
| existent_field_on_model = "Peter" | ||
| field_name = :name |
There was a problem hiding this comment.
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 👍
|
Also, please rebase or merge |
Co-authored-by: Jose Farias <31393016+josefarias@users.noreply.github.com>
Co-authored-by: Jose Farias <31393016+josefarias@users.noreply.github.com>
2 participants
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 hereAt the moment calling this fails