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 <me@citizen428.net>

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

app/javascript/onboarding/__tests__/Onboarding.test.jsx

@@ -133,7 +133,7 @@ describe('<Onboarding />', () => {
     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('<Onboarding />', () => {
     // 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');

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 (
-        <div role="alert" class="crayons-notice crayons-notice--danger">
-          An error occurred: {errorMessage}
-        </div>
-      );
-    }
-
     const sections = groups.map((group) => {
       return (
         <div key={group.id} class="onboarding-profile-sub-section">
@@ -195,6 +195,11 @@ export class ProfileForm extends Component {
             slidesCount={slidesCount}
             currentSlideIndex={currentSlideIndex}
           />
+          {error && (
+            <div role="alert" class="crayons-notice crayons-notice--danger m-2">
+              An error occurred: {errorMessage}
+            </div>
+          )}
           <div className="onboarding-content about">
             <header className="onboarding-content-header">
               <h1 id="title" className="title">
@@ -219,9 +224,19 @@ export class ProfileForm extends Component {
                 />
               </figure>
               <h3>{name}</h3>
-              <p>{username}</p>
             </div>
-            <div>{sections}</div>
+            <div className="onboarding-profile-sub-section">
+              <TextInput
+                field={{
+                  attribute_name: 'username',
+                  label: 'Username',
+                  default_value: username,
+                  required: true,
+                }}
+                onFieldChange={this.handleFieldChange}
+              />
+            </div>
+            {sections}
           </div>
         </div>
       </div>

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 (
     <FormField>
@@ -36,9 +43,11 @@ export function TextInput(props) {
       <input
         class="crayons-textfield"
         placeholder={placeholder_text}
+        defaultValue={default_value}
         name={attribute_name}
         id={attribute_name}
         onChange={onFieldChange}
+        required={required ? 'required' : ''}
       />
       {description && <p class="crayons-field__description">{description}</p>}
     </FormField>

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();
   });
 });

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