-
Notifications
You must be signed in to change notification settings - Fork 2.1k
Feature: Interview survey form with multi select #5018
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Feature: Interview survey form with multi select #5018
Conversation
@@ -0,0 +1,12 @@ | |||
module RequiresFeature | |||
def requires_feature(name, **) |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Love this ❤️
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:) | ||
end | ||
end |
There was a problem hiding this comment.
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_create
ing 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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
find('div[aria-label="Combobox"]').click | ||
find('div[class="ss-option"]', text: 'Rails routing').click |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
find('div[aria-label="Combobox"]').click | ||
find('div[class="ss-option"]', text: 'Rails routing').click |
There was a problem hiding this comment.
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.
@@ -0,0 +1,12 @@ | |||
module RequiresFeature | |||
def requires_feature(name, **) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Love this ❤️
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:) | ||
end | ||
end |
There was a problem hiding this comment.
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.
if @interview_survey.save! | ||
redirect_to dashboard_path, notice: 'Survey Submitted' | ||
else | ||
render :new, status: :unprocessable_entity | ||
end | ||
end |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not a showstopper, but I noticed that we lose the selected tags when submitting the form with validation errors. It could be a bit frustrating for users if they’ve added a lot of tags:
Screen.Recording.2025-07-20.at.22.15.53.mov
I’m not sure how feasible it is to repopulate the tags with SlimSelect, but might be worth exploring.
On a related note, I noticed we’re saving the concepts to the database even if the form validation fails - is that the intended behavior?
|
||
def interview_concept_names=(names) | ||
self.interview_concepts = names.compact_blank.map do |name| | ||
InterviewConcept.find_or_create_by(name:) |
There was a problem hiding this comment.
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?
InterviewConcept.find_or_create_by(name:) | |
InterviewConcept.find_or_create_by(name: name.downcase) |
@@ -171,7 +171,7 @@ | |||
|
|||
create_table "interview_surveys", force: :cascade do |t| | |||
t.bigint "user_id", null: false | |||
t.date "interview_date" | |||
t.date "interview_date", null: false |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: looks like the migration might have accidentally been left out of the commit.
Because
We need a multi select form on interview surveys.
Note that this intentionally has a fairly constrained scope just to make sure we have the general behavior down for what we want. The form still looks kind of bad because little attention was paid to styling, validations aren't completely ironed out, and the feature is still hidden behind a feature flag.
This PR
before_action
wrapperQA
survey_feature
feature flag. One way is to runbundle exec flipper enable survey_feature
[email protected]
withpassword
as the password.localhost:3000/interview_surveys/new
. There's no active link anywhere to this yet, so it must be visited by manually entering into the address barIssue
Closes #4966
Additional Information
Happy path video:
2025-06-07.13-06-26.mp4
Pull Request Requirements
keyword: brief description of change
format, using one of the following keywords:Feature
- adds new or amends existing user-facing behaviorChore
- changes that have no user-facing value, refactors, dependency bumps, etcFix
- bug fixesBecause
section summarizes the reason for this PRThis PR
section has a bullet point list describing the changes in this PRIssue
section