ifme
ifme copied to clipboard
Refactor moments_form_helper.rb
Description
This pull request aims to improve the architecture of the current codebase. The class moments_form_helper.rb violates the Single Responsibility Principle because it handles multiple responsibilities.
Key Changes
- A new class,
FormInputwill handle the input properties of the moments form. - Within
MomentsFormHelper, methods includingmoments_input_props,moment_publishing,moment_bookmarked, andmoment_display_resourceswill be updated to create a newFormInputwith all the information instead of formatting the input within the methods.
More Details
This is important because:
- It is easier to test the form input properties on their own.
MomentsFormHelpernow only handles business logic and not form input construction.- The
FormInputobject can be used in other classes as well, making the codebase more extendable. - Single Responsibility Principle is adhered to, improving the overall architecture of the codebase.
Corresponding Issue
#2350
Reviewing this pull request? Check out our Code Review Practices guide if you haven't already!
Thank you for opening this pull request with us! Be sure to follow our Pull Request Practices. Let us know if you have any questions on Slack.
I'm seeing there were test failures introduced here. I would look into this! Let me know if you need help!
Hey guys,i'm looking for a place to contribute and ended here. Great work on the refactor! This definitely improves the code structure and makes the helper easier to maintain.
I have three suggestions to finalize this:
-
Move
FormInputinto its own file, ideally atapp/forms/form_input.rb, following Rails convention (one class per file). This will make it easier to reuse and maintain. -
Add unit tests for
FormInput, for methods like.with_value,.with_attributes, and.empty. Example:
# spec/forms/form_input_spec.rb
require 'rails_helper'
RSpec.describe FormInput, type: :model do
it 'builds with basic attributes' do
input = FormInput.new(id: 'test', type: 'text', name: 'form[test]', label: 'Test')
expect(input.to_h).to include(id: 'test', type: 'text', name: 'form[test]', label: 'Test')
end
it 'returns a new input with additional attributes' do
input = FormInput.new(id: 'test', type: 'text', name: 'form[test]', label: 'Test')
updated_input = input.with_attributes(placeholder: 'Enter text')
expect(updated_input.to_h[:placeholder]).to eq('Enter text')
end
it 'provides an empty input' do
empty_input = FormInput.empty
expect(empty_input.to_h.values).to all(be_nil)
end
end
- Update the failing helper specs: Files and lines: spec/helpers/moments_form_helper_spec.rb, at 35, 276, 534, and 813. The failures are because the new form structure now correctly generates hidden fields for switches.
Example of what needs to be added:
Before (old expected input):
{
id: 'moment_comment',
type: 'switch',
name: 'moment[comment]',
label: 'Allow Comments?',
value: true
}
Now (new structure):
{
id: 'moment_comment_hidden',
type: 'hidden',
name: 'moment[comment]',
value: '0'
},
{
id: 'moment_comment',
type: 'switch',
name: 'moment[comment]',
label: 'Allow Comments?',
unchecked_value: '1',
value: '0',
dark: true,
info: 'Only you and viewers can comment'
}
Once these adjustments are made, the helper specs should pass. Hope this helps. Also sorry if I am overstepping somehow, I am looking for projects to contribute as a study practice and I thought it would be easier to start with PR reviews