ifme icon indicating copy to clipboard operation
ifme copied to clipboard

Refactor moments_form_helper.rb

Open mirjharr2 opened this issue 8 months ago • 3 comments

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, FormInput will handle the input properties of the moments form.
  • Within MomentsFormHelper, methods including moments_input_props, moment_publishing, moment_bookmarked, and moment_display_resources will be updated to create a new FormInput with 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.
  • MomentsFormHelper now only handles business logic and not form input construction.
  • The FormInput object 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!

mirjharr2 avatar Apr 06 '25 22:04 mirjharr2

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.

welcome[bot] avatar Apr 06 '25 22:04 welcome[bot]

I'm seeing there were test failures introduced here. I would look into this! Let me know if you need help!

julianguyen avatar Apr 09 '25 15:04 julianguyen

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:

  1. Move FormInput into its own file, ideally at app/forms/form_input.rb, following Rails convention (one class per file). This will make it easier to reuse and maintain.

  2. 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
  1. 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

vkaraujo avatar Apr 29 '25 20:04 vkaraujo