Skip to content

Conversation

@mirjharr2
Copy link
Contributor

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!

etvonn and others added 3 commits March 12, 2025 19:43
Refactored code to adhere to single responsibility principle a bit better, form input is encapsulated in a class now. Should also make things a bit easier to read/maintain.
Corrected form input so that switch input functions as intended, had to make light controller modifications.
@welcome
Copy link

welcome bot commented Apr 6, 2025

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.

@julianguyen
Copy link
Member

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

def create
@moment = current_user.moments.build(moment_params)
@moment.published_at = Time.zone.now if publishing?
@moment.published_at = params[:moment][:publishing] == '1' ? Time.zone.now : nil # Use unfiltered params
Copy link
Member

Choose a reason for hiding this comment

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

I'm curious why we're using params[:moment][:publishing] instead of publishing??

Copy link

Choose a reason for hiding this comment

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

It's been a while, but iirc I made the change because I was struggling to get the save as draft switch to function correctly, as it was essentially either always a draft or always publishing otherwise. I just went back for a closer look since our tests didn't pass, and now it isn't functional so I must've failed to push something at some point. I'll be going back to see if I can get it working again when I get a chance tonight.

def update
if (publishing? && !@moment.published?) || saving_as_draft?
@moment.published_at = !saving_as_draft? && Time.zone.now
if params[:moment][:publishing]
Copy link
Member

Choose a reason for hiding this comment

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

What about saving_as_draft??

etvonn and others added 2 commits April 23, 2025 18:12
@vkaraujo
Copy link

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants