-
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?
Changes from 17 commits
accbd5a
95f4fbf
06a98b4
cc59dc5
b72d570
8227369
7afb207
70e75e8
4cff548
40c75f8
30b5109
eb49abe
5a48fa3
0bdea06
ef9a636
293a9e3
8b32d9c
93ed0ff
b214030
a4c1bfe
890aa80
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,12 @@ | ||
| module RequiresFeature | ||
| def requires_feature(name, **) | ||
| before_action( | ||
| lambda do | ||
| return if Flipper.enabled?(name, current_user) | ||
|
|
||
| redirect_to dashboard_path, notice: 'Feature not enabled' | ||
| end, | ||
| ** | ||
| ) | ||
| end | ||
| end | ||
| 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 | ||
|
||
|
|
||
| private | ||
|
|
||
| def interview_survey_params | ||
| params.require(:interview_survey).permit( | ||
| :interview_date, | ||
| interview_concept_names: [] | ||
| ) | ||
| end | ||
| end | ||
| 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({ | ||
JoshDevHub marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||
| select: this.element, | ||
| events: { | ||
| addable: function (value) { return value } | ||
| } | ||
| }) | ||
| } | ||
| } | ||
| 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:) | ||||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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
|
||||||
| end | ||||||
| end | ||||||
|
Comment on lines
+11
to
+19
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 Might be better to kick this to a form object or think of some way to use
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I really like this approach. Simple and clean! You're right about 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.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 | ||||||
|
|
||||||
|
|
||||||
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -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
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 | ||
|
|
||
|
|
||
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Uh oh!
There was an error while loading. Please reload this page.
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
Flipperchecks. Also means the diff for removing a feature flag is basically deleting therequires_featureline 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: :createor 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 ❤️