From dd3121e91ca0ad61de240ae819d52eabdee746a8 Mon Sep 17 00:00:00 2001 From: Jacob Herrington Date: Wed, 24 Feb 2021 10:48:31 -0600 Subject: [PATCH] Add username to onboarding (#12697) * Add username to onboarding * Add margin to profile form error alert * Update user controller for onboarding Co-authored-by: Michael Kohl --- app/controllers/users_controller.rb | 35 +++++---- .../onboarding/__tests__/Onboarding.test.jsx | 8 +- .../onboarding/components/ProfileForm.jsx | 77 +++++++++++-------- .../components/ProfileForm/TextInput.jsx | 13 +++- .../components/__tests__/ProfileForm.test.jsx | 48 ++---------- spec/requests/users_onboarding_spec.rb | 22 +++++- 6 files changed, 108 insertions(+), 95 deletions(-) diff --git a/app/controllers/users_controller.rb b/app/controllers/users_controller.rb index 7b86b939c..02d2490b0 100644 --- a/app/controllers/users_controller.rb +++ b/app/controllers/users_controller.rb @@ -10,7 +10,7 @@ class UsersController < ApplicationController before_action :set_suggested_users, only: %i[index] before_action :initialize_stripe, only: %i[edit] - ALLOWED_USER_PARAMS = %i[last_onboarding_page].freeze + ALLOWED_USER_PARAMS = %i[last_onboarding_page username].freeze INDEX_ATTRIBUTES_FOR_SERIALIZATION = %i[id name username summary profile_image].freeze private_constant :INDEX_ATTRIBUTES_FOR_SERIALIZATION @@ -165,19 +165,20 @@ class UsersController < ApplicationController end def onboarding_update - if params[:user] - sanitize_user_params - current_user.assign_attributes(params[:user].permit(ALLOWED_USER_PARAMS)) - current_user.profile_updated_at = Time.current - end - - if current_user.save && params[:profile] - update_result = Profiles::Update.call(current_user, { profile: profile_params }) - end - - current_user.saw_onboarding = true authorize User - render_update_response(update_result&.success?) + user_params = { saw_onboarding: true } + + if params[:user] + if params.dig(:user, :username).blank? + return render_update_response(false, "Username cannot be blank") + end + + sanitize_user_params + user_params.merge!(params[:user].permit(ALLOWED_USER_PARAMS)) + end + + update_result = Profiles::Update.call(current_user, { user: user_params, profile: profile_params }) + render_update_response(update_result.success?, update_result.errors_as_sentence) end def onboarding_checkbox_update @@ -313,11 +314,11 @@ class UsersController < ApplicationController recent_suggestions.presence || default_suggested_users end - def render_update_response(success) - outcome = success ? "updated successfully" : "update failed" + def render_update_response(success, errors = nil) + status = success ? 200 : 422 respond_to do |format| - format.json { render json: { outcome: outcome } } + format.json { render json: { errors: errors }, status: status } end end @@ -376,7 +377,7 @@ class UsersController < ApplicationController end def profile_params - params[:profile].permit(Profile.attributes) + params[:profile] ? params[:profile].permit(Profile.attributes) : nil end def password_params diff --git a/app/javascript/onboarding/__tests__/Onboarding.test.jsx b/app/javascript/onboarding/__tests__/Onboarding.test.jsx index c18fecfab..b739ae809 100644 --- a/app/javascript/onboarding/__tests__/Onboarding.test.jsx +++ b/app/javascript/onboarding/__tests__/Onboarding.test.jsx @@ -133,7 +133,7 @@ describe('', () => { termsCheckbox.click(); // click to next step - const nextButton = await findByText(/continue/i); + let nextButton = await findByText(/continue/i); fetch.mockResponse(fakeEmptyResponse); nextButton.click(); @@ -148,10 +148,10 @@ describe('', () => { // we should be on the Profile Form step await findByTestId('onboarding-profile-form'); - // click on skip for now - skipButton = getByText(/Skip for now/i); + // click on continue without adjusting form fields + nextButton = getByText(/Continue/i); fetch.mockResponse(fakeEmptyResponse); - skipButton.click(); + nextButton.click(); // we should be on the Follow Users step await findByTestId('onboarding-follow-users'); diff --git a/app/javascript/onboarding/components/ProfileForm.jsx b/app/javascript/onboarding/components/ProfileForm.jsx index eb4e0981b..c70cdb149 100644 --- a/app/javascript/onboarding/components/ProfileForm.jsx +++ b/app/javascript/onboarding/components/ProfileForm.jsx @@ -1,7 +1,7 @@ import { h, Component } from 'preact'; import PropTypes from 'prop-types'; -import { userData, getContentOfToken, updateOnboarding } from '../utilities'; +import { userData, updateOnboarding } from '../utilities'; import { Navigation } from './Navigation'; import { ColorPicker } from './ProfileForm/ColorPicker'; @@ -22,8 +22,8 @@ export class ProfileForm extends Component { this.user = userData(); this.state = { groups: [], - formValues: {}, - canSkip: true, + formValues: { username: this.user.username }, + canSkip: false, last_onboarding_page: 'v2: personal info form', }; } @@ -47,26 +47,34 @@ export class ProfileForm extends Component { } } - onSubmit() { - const csrfToken = getContentOfToken('csrf-token'); + async onSubmit() { const { formValues, last_onboarding_page } = this.state; - fetch('/onboarding_update', { - method: 'PATCH', - headers: { - 'X-CSRF-Token': csrfToken, - 'Content-Type': 'application/json', - }, - body: JSON.stringify({ - user: { last_onboarding_page }, - profile: { ...formValues }, - }), - credentials: 'same-origin', - }).then((response) => { - if (response.ok) { - const { next } = this.props; - next(); + const { username, ...newFormValues } = formValues; + try { + const response = await request('/onboarding_update', { + method: 'PATCH', + body: { + user: { last_onboarding_page, username }, + profile: { ...newFormValues }, + }, + }); + if (!response.ok) { + throw response; } - }); + const { next } = this.props; + next(); + } catch (error) { + Honeybadger.notify(error.statusText); + let errorMessage = 'Unable to continue, please try again.'; + if (error.status === 422) { + // parse validation error messages from UsersController#onboarding_update + const errorData = await error.json(); + errorMessage = errorData.errors; + this.setState({ error: true, errorMessage }); + } else { + this.setState({ error: true, errorMessage }); + } + } } handleFieldChange(e) { @@ -153,14 +161,6 @@ export class ProfileForm extends Component { const { profile_image_90, username, name } = this.user; const { canSkip, groups = [], error, errorMessage } = this.state; - if (error) { - return ( - - ); - } - const sections = groups.map((group) => { return (
@@ -195,6 +195,11 @@ export class ProfileForm extends Component { slidesCount={slidesCount} currentSlideIndex={currentSlideIndex} /> + {error && ( + + )}

@@ -219,9 +224,19 @@ export class ProfileForm extends Component { />

{name}

-

{username}

-
{sections}
+
+ +
+ {sections}
diff --git a/app/javascript/onboarding/components/ProfileForm/TextInput.jsx b/app/javascript/onboarding/components/ProfileForm/TextInput.jsx index 7cbfce59f..c0fd7d363 100644 --- a/app/javascript/onboarding/components/ProfileForm/TextInput.jsx +++ b/app/javascript/onboarding/components/ProfileForm/TextInput.jsx @@ -25,8 +25,15 @@ import PropTypes from 'prop-types'; import { FormField } from '@crayons'; export function TextInput(props) { - const { onFieldChange } = props; - const { attribute_name, placeholder_text, description, label } = props.field; + const { onFieldChange, field } = props; + const { + attribute_name, + placeholder_text, + default_value, + description, + label, + required, + } = field; return ( @@ -36,9 +43,11 @@ export function TextInput(props) { {description &&

{description}

} diff --git a/app/javascript/onboarding/components/__tests__/ProfileForm.test.jsx b/app/javascript/onboarding/components/__tests__/ProfileForm.test.jsx index a6feec190..9b334e820 100644 --- a/app/javascript/onboarding/components/__tests__/ProfileForm.test.jsx +++ b/app/javascript/onboarding/components/__tests__/ProfileForm.test.jsx @@ -1,5 +1,5 @@ import { h } from 'preact'; -import { render, fireEvent } from '@testing-library/preact'; +import { render } from '@testing-library/preact'; import fetch from 'jest-fetch-mock'; import '@testing-library/jest-dom'; import { axe } from 'jest-axe'; @@ -130,14 +130,17 @@ describe('ProfileForm', () => { const { findByLabelText } = renderProfileForm(); const field1 = await findByLabelText(/Education/i); - const field2 = await findByLabelText(/Name/i); + const field2 = await findByLabelText(/^Name/i); const field3 = await findByLabelText(/Website URL/i); + const field4 = await findByLabelText(/Username/i); expect(field1).toBeInTheDocument(); expect(field2).toBeInTheDocument(); expect(field2.getAttribute('placeholder')).toEqual('John Doe'); expect(field3).toBeInTheDocument(); expect(field3.getAttribute('placeholder')).toEqual('https://yoursite.com'); + expect(field4).toBeInTheDocument(); + expect(field4.getAttribute('value')).toEqual('username'); }); it('should render a stepper', () => { @@ -152,44 +155,9 @@ describe('ProfileForm', () => { expect(queryByTestId('back-button')).toBeDefined(); }); - it('should update the text on the forward button', async () => { - const { - getByLabelText, - getByText, - queryByText, - findByLabelText, - findByText, - } = renderProfileForm(); + it('should not be skippable', async () => { + const { getByText } = renderProfileForm(); - // input the bio - const field2 = await findByLabelText(/Name/i); - expect(field2.value).toEqual(''); - getByText(/skip for now/i); - expect(queryByText(/continue/i)).toBeNull(); - - fireEvent.keyDown(field2, { - key: 'Enter', - keyCode: 13, - which: 13, - target: { value: 'Hong Kong Fuey' }, - }); - expect(field2.value).toEqual('Hong Kong Fuey'); - - // input the location too (since we're using firevent and it doesn't call the focus events - // that will trigger the continue ) - let field3 = getByLabelText(/Website URL/i); - expect(field3.value).toEqual(''); - fireEvent.keyDown(field3, { - key: 'Enter', - keyCode: 13, - which: 13, - target: { value: 'www.website.com' }, - }); - - field3 = await findByLabelText(/Website URL/i); - - expect(field3.value).toEqual('www.website.com'); - - findByText(/continue/i); + expect(getByText(/continue/i)).toBeInTheDocument(); }); }); diff --git a/spec/requests/users_onboarding_spec.rb b/spec/requests/users_onboarding_spec.rb index 73d1a2e77..41998a183 100644 --- a/spec/requests/users_onboarding_spec.rb +++ b/spec/requests/users_onboarding_spec.rb @@ -13,12 +13,32 @@ RSpec.describe "UsersOnboarding", type: :request do end it "updates the user's last_onboarding_page attribute" do - params = { user: { last_onboarding_page: "v2: personal info form" } } + params = { user: { last_onboarding_page: "v2: personal info form", username: "test" } } expect do patch "/onboarding_update.json", params: params end.to change(user, :last_onboarding_page) end + it "updates the user's username attribute" do + params = { user: { username: "WilhuffTarkin" } } + expect do + patch "/onboarding_update.json", params: params + end.to change(user, :username).to("wilhufftarkin") + end + + it "returns a 422 error if the username is blank" do + params = { user: { username: "" } } + patch "/onboarding_update.json", params: params + expect(response).to have_http_status(:unprocessable_entity) + end + + it "updates the user's profile" do + params = { profile: { employer_name: "Galatic Empire" } } + expect do + patch "/onboarding_update.json", params: params + end.to change(user.profile, :employer_name).to("Galatic Empire") + end + it "does not update the user's last_onboarding_page if it is empty" do params = { user: { last_onboarding_page: "" } } expect do