Skip to content
Open
Show file tree
Hide file tree
Changes from 17 commits
Commits
Show all changes
21 commits
Select commit Hold shift + click to select a range
accbd5a
Build basic form for interview surveys
JoshDevHub Apr 12, 2025
95f4fbf
Add slim-select library for multi select
JoshDevHub Apr 12, 2025
06a98b4
Implement multi select for interview survey form
JoshDevHub Apr 12, 2025
cc59dc5
Add `has_many` in user model for interview surveys
JoshDevHub May 3, 2025
b72d570
Add `inverse_of` option to association
JoshDevHub May 3, 2025
8227369
Add form for filling out interview surveys
JoshDevHub May 3, 2025
7afb207
Use better stimulus semantics for multi select controller
JoshDevHub May 3, 2025
70e75e8
Use `pluck` in select input query
JoshDevHub May 3, 2025
4cff548
Add inverse association for interview_concepts
JoshDevHub May 3, 2025
40c75f8
Add system test for interview surveys with existing concepts
JoshDevHub Jun 1, 2025
30b5109
Refactor feature flag gating in controllers
JoshDevHub Jun 1, 2025
eb49abe
Remove test on `get interview_surves/new`
JoshDevHub Jun 1, 2025
5a48fa3
Add system test for interview surveys with new concepts
JoshDevHub Jun 1, 2025
0bdea06
Satisfy JS and ERB linters
JoshDevHub Jun 1, 2025
ef9a636
Adjust virtual attribute logic
JoshDevHub Jun 7, 2025
293a9e3
Make custom validation method `private`
JoshDevHub Jun 7, 2025
8b32d9c
Merge branch 'main' into interview_survey_multiselect_form
JoshDevHub Jun 8, 2025
93ed0ff
Add disconnect method to destroy slim select
JoshDevHub Jul 15, 2025
b214030
Remove CDN link for slim select styles
JoshDevHub Jul 20, 2025
a4c1bfe
Use proper validation handling with interview survey form
JoshDevHub Jul 20, 2025
890aa80
Merge branch 'main' into interview_survey_multiselect_form
JoshDevHub Jul 20, 2025
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions app/controllers/application_controller.rb
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
class ApplicationController < ActionController::Base
include CurrentTheme
include Pagy::Backend
extend RequiresFeature

before_action :configure_permitted_parameters, if: :devise_controller?
before_action :set_sentry_user, if: :current_user
Expand Down
12 changes: 12 additions & 0 deletions app/controllers/concerns/requires_feature.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
module RequiresFeature
def requires_feature(name, **)
Copy link
Contributor Author

@JoshDevHub JoshDevHub Jun 7, 2025

Choose a reason for hiding this comment

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

I moved the feature flag stuff to this because I think it makes it much easier to use compared to filling up controller actions with Flipper checks. Also means the diff for removing a feature flag is basically deleting the requires_feature line in the relevant controller(s) and a little test cleanup.

Forwards keyword args so you can still do something like requires_feature :feat_name, only: :create or something if the developer wanted.

Copy link
Member

Choose a reason for hiding this comment

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

Love this ❤️

before_action(
lambda do
return if Flipper.enabled?(name, current_user)

redirect_to dashboard_path, notice: 'Feature not enabled'
end,
**
)
end
end
25 changes: 20 additions & 5 deletions app/controllers/interview_surveys_controller.rb
Original file line number Diff line number Diff line change
@@ -1,12 +1,27 @@
class InterviewSurveysController < ApplicationController
before_action :authenticate_user!
requires_feature :survey_feature

def new
unless Feature.enabled?(:survey_feature, current_user)
redirect_to dashboard_path, notice: 'Feature not enabled'
end
@interview_survey = InterviewSurvey.new
end

def create
# puts params[:survey]
redirect_to dashboard_path, notice: 'Survey Submitted'
@interview_survey = current_user.interview_surveys.build(interview_survey_params)

if @interview_survey.save!
redirect_to dashboard_path, notice: 'Survey Submitted'
else
render :new, status: :unprocessable_entity
end
end
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Reminder to self to change this to #save instead of #save! and also write a new system spec that exercises the validation failure code path here.


private

def interview_survey_params
params.require(:interview_survey).permit(
:interview_date,
interview_concept_names: []
)
end
end
14 changes: 14 additions & 0 deletions app/javascript/controllers/multi_select_controller.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,14 @@
import { Controller } from '@hotwired/stimulus'
import SlimSelect from 'slim-select'

export default class MultiSelectController extends Controller {
connect () {
// eslint-disable-next-line no-new
new SlimSelect({
select: this.element,
events: {
addable: function (value) { return value }
}
})
}
}
3 changes: 2 additions & 1 deletion app/models/interview_concept.rb
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
class InterviewConcept < ApplicationRecord
has_many :interview_survey_concepts, dependent: :destroy
has_many :interview_survey_concepts, dependent: :destroy, inverse_of: :interview_concept
has_many :interview_surveys, through: :interview_survey_concepts, inverse_of: :interview_concepts

validates :name, presence: true
validates :name, length: { minimum: 2, maximum: 25 }
Expand Down
17 changes: 16 additions & 1 deletion app/models/interview_survey.rb
Original file line number Diff line number Diff line change
@@ -1,10 +1,25 @@
class InterviewSurvey < ApplicationRecord
belongs_to :user
has_many :interview_survey_concepts, dependent: :destroy
has_many :interview_survey_concepts, dependent: :destroy, inverse_of: :interview_survey
has_many :interview_concepts,
through: :interview_survey_concepts,
inverse_of: :interview_surveys

validates :interview_date, presence: true
validate :interview_date_must_be_in_the_past

def interview_concept_names
interview_concepts.pluck(:name)
end

def interview_concept_names=(names)
self.interview_concepts = names.compact_blank.map do |name|
InterviewConcept.find_or_create_by(name:)
Copy link
Member

Choose a reason for hiding this comment

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

Nit: Would it be worth normalizing the name here to help avoid duplicates?

Suggested change
InterviewConcept.find_or_create_by(name:)
InterviewConcept.find_or_create_by(name: name.downcase)

end
end
Comment on lines +11 to +19
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is one of the things that's maybe subject to change. Lots of different ways to approach this, but I ended up just using this virtual attribute way because it's very simple (and naturally keeps the controller very simple).

There are a few things to worry about with this though. Validating the interview concepts is one. Another is potential performance issues (and maybe race condition issues) around find_or_createing over a list of values (each one will be its own DB call). And also this should probably be wrapped in a transaction.

Might be better to kick this to a form object or think of some way to use accepts_nested_attributes_for, but I found that very unnatural for this specific usecase.

Copy link
Member

Choose a reason for hiding this comment

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

I really like this approach. Simple and clean!

You're right about accepts_nested_attributes_for not being right for this. I think needing to go through the join table is whats making it feel unnatural, I didn't really appreciate or take that fully into account that when we talked about the approaches before.

I think whether we stick with this approach or move toward a form object depends on how we want to handle validations. Form objects should give us more control over that, but it really comes down to what kinds of validations we think we should enforce or display.

Do you have thoughts on what those might be? Comboboxes seem tricky to validate to me - though maybe I’m just not being imaginative enough.

It could also be enough to just limit the input to 25 characters to match the limit we're setting in the backend validation.

Copy link
Member

@KevinMulhern KevinMulhern Jul 20, 2025

Choose a reason for hiding this comment

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

Have you given any thought to this @JoshDevHub?

I think we'll need to either give feedback or prevent users from being able to submit concepts that have less than 3 characters or more than 25 based on the validations in InterviewConcept.

Would we be able to set character limits in SlimSelect?


private

def interview_date_must_be_in_the_past
return if interview_date.present? && interview_date <= Time.zone.today

Expand Down
1 change: 1 addition & 0 deletions app/models/user.rb
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@ class User < ApplicationRecord
has_many :notifications, as: :recipient, dependent: :destroy
has_many :announcements, dependent: nil
has_many :likes, dependent: :destroy
has_many :interview_surveys, dependent: :destroy

belongs_to :path, optional: true, counter_cache: true

Expand Down
16 changes: 13 additions & 3 deletions app/views/interview_surveys/new.html.erb
Original file line number Diff line number Diff line change
Expand Up @@ -4,8 +4,18 @@
<div class="page-container">
<h1 class="page-heading-title">Interview Survey</h1>

<div class='max-w-xl mx-auto'>
<%# where form will be rendered %>
</div>
<%= form_with model: @interview_survey, builder: TailwindFormBuilder do |form| %>
<%= form.label :interview_date %>
<%= form.date_field :interview_date %>

<%= form.label :interview_concept_names %>
<%= form.select :interview_concept_names,
InterviewConcept.pluck(:name, :name),
{ multiple: true },
{ data: { controller: 'multi-select' } } %>

<%= form.submit 'Submit', class: 'button button--primary w-full text-sm' %>
<% end %>

</div>
</div>
1 change: 1 addition & 0 deletions app/views/layouts/application.html.erb
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@
<link href="https://fonts.googleapis.com/css2?family=Inter:wght@100;200;300;400;500;600;700;800;900&display=swap" rel="stylesheet">

<%= stylesheet_link_tag 'application', 'main', media: 'all', data: { turbo_track: 'reload' } %>
<link rel="stylesheet" href="https://unpkg.com/slim-select@latest/dist/slimselect.css" />
<%= render 'shared/sentry' %>
<%= render 'shared/analytics' %>
<%= javascript_include_tag 'main', defer: true, data: { turbo_track: 'reload' } %>
Expand Down
2 changes: 1 addition & 1 deletion db/schema.rb

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

1 change: 1 addition & 0 deletions package.json
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,7 @@
"mermaid": "^11.6.0",
"postcss": "^8.5.3",
"prismjs": "^1.30.0",
"slim-select": "^2.11.0",
"sortablejs": "^1.15.6",
"stimulus-use": "^0.52.3",
"tailwindcss": "4.1.7",
Expand Down
1 change: 1 addition & 0 deletions spec/models/interview_concept_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@
subject(:interview_concept) { create(:interview_concept) }

it { (is_expected.to have_many(:interview_survey_concepts)) }
it { (is_expected.to have_many(:interview_surveys)) }
it { is_expected.to validate_presence_of(:name) }
it { is_expected.to validate_length_of(:name).is_at_least(2).is_at_most(25) }
end
1 change: 1 addition & 0 deletions spec/models/user_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@
it { is_expected.to have_many(:notifications) }
it { is_expected.to have_many(:likes).dependent(:destroy) }
it { is_expected.to belong_to(:path).optional(true) }
it { is_expected.to have_many(:interview_surveys).dependent(:destroy) }

context 'when user is created' do
let!(:default_path) { create(:path, default_path: true) }
Expand Down
26 changes: 24 additions & 2 deletions spec/system/interview_survey_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -10,11 +10,33 @@
context 'when the feature is enabled' do
before do
Flipper.enable(:survey_feature)
create(:interview_concept, name: 'Rails routing')
end

it 'shows the survey' do
it 'creates an interview survey with existing concepts' do
visit new_interview_survey_path
expect(page).to have_content('Interview Survey')

fill_in :interview_survey_interview_date, with: Date.current

find('div[aria-label="Combobox"]').click
find('div[class="ss-option"]', text: 'Rails routing').click
Comment on lines +21 to +22
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Unfortunately have to use this very manual flow to find and click elements. Maybe there's a way to add test ids to elements generated by slim select, but I couldn't immediately figure it out.

Copy link
Member

Choose a reason for hiding this comment

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

I think this is fine Josh. It's difficult to do anything else with libs that generate HTML internally.


click_on 'Submit'

expect(page).to have_content('Survey Submitted')
end

it 'creates an interview survey with new concepts' do
visit new_interview_survey_path

fill_in :interview_survey_interview_date, with: Date.current

find('div[aria-label="Combobox"]').click
find('input[type="search"]').set('React props').send_keys(:return)

click_on 'Submit'

expect(page).to have_content('Survey Submitted')
end
end

Expand Down
8 changes: 8 additions & 0 deletions yarn.lock

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.