FIX: Better error message when username change fails.

https://meta.discourse.org/t/500-error-on-username-edit/64064

app/assets/javascripts/discourse/controllers/preferences/username.js.es6

@@ -1,11 +1,12 @@
+import { default as computed, observes } from 'ember-addons/ember-computed-decorators';
 import { setting, propertyEqual } from 'discourse/lib/computed';
 import DiscourseURL from 'discourse/lib/url';
 import { userPath } from 'discourse/lib/url';
+import { popupAjaxError } from 'discourse/lib/ajax-error';
 
 export default Ember.Controller.extend({
   taken: false,
   saving: false,
-  error: false,
   errorMessage: null,
   newUsername: null,
 
@@ -15,35 +16,40 @@ export default Ember.Controller.extend({
   saveDisabled: Em.computed.or('saving', 'newUsernameEmpty', 'taken', 'unchanged', 'errorMessage'),
   unchanged: propertyEqual('newUsername', 'username'),
 
-  checkTaken: function() {
-    if( this.get('newUsername') && this.get('newUsername').length < this.get('minLength') ) {
+  @observes("newUsername")
+  checkTaken() {
+    let newUsername = this.get('newUsername');
+
+    if (newUsername && newUsername.length < this.get('minLength')) {
       this.set('errorMessage', I18n.t('user.name.too_short'));
     } else {
-      var self = this;
       this.set('taken', false);
       this.set('errorMessage', null);
+
       if (Ember.isEmpty(this.get('newUsername'))) return;
       if (this.get('unchanged')) return;
-      Discourse.User.checkUsername(this.get('newUsername'), undefined, this.get('content.id')).then(function(result) {
+
+      Discourse.User.checkUsername(newUsername, undefined, this.get('content.id')).then(result => {
         if (result.errors) {
-          self.set('errorMessage', result.errors.join(' '));
+          this.set('errorMessage', result.errors.join(' '));
         } else if (result.available === false) {
-          self.set('taken', true);
+          this.set('taken', true);
         }
       });
     }
-  }.observes('newUsername'),
+  },
 
-  saveButtonText: function() {
-    if (this.get('saving')) return I18n.t("saving");
+  @computed('saving')
+  saveButtonText(saving) {
+    if (saving) return I18n.t("saving");
     return I18n.t("user.change");
-  }.property('saving'),
+  },
 
   actions: {
     changeUsername() {
       if (this.get('saveDisabled')) { return; }
 
-      return bootbox.confirm(I18n.t("user.change_username.confirm"), 
+      return bootbox.confirm(I18n.t("user.change_username.confirm"),
                              I18n.t("no_value"),
                              I18n.t("yes_value"), result => {
         if (result) {
@@ -51,7 +57,7 @@ export default Ember.Controller.extend({
           this.get('content').changeUsername(this.get('newUsername')).then(() => {
             DiscourseURL.redirectTo(userPath(this.get('newUsername').toLowerCase() + "/preferences"));
           })
-          .catch(() => this.set('error', true))
+          .catch(popupAjaxError)
           .finally(() => this.set('saving', false));
         }
       });
@@ -59,5 +65,3 @@ export default Ember.Controller.extend({
   }
 
 });
-
-

app/assets/javascripts/discourse/templates/preferences-username.hbs

@@ -7,14 +7,6 @@
       </div>
     </div>
 
-    {{#if error}}
-      <div class="control-group">
-        <div class="instructions">
-          <div class='alert alert-error'>{{i18n 'user.change_username.error'}}</div>
-        </div>
-      </div>
-    {{/if}}
-
     <div class="control-group">
       <label class="control-label">{{i18n 'user.username.title'}}</label>
       <div class="controls">

app/controllers/users_controller.rb

@@ -127,14 +127,13 @@ class UsersController < ApplicationController
     user = fetch_user_from_params
     guardian.ensure_can_edit_username!(user)
 
-    # TODO proper error surfacing (result is a Model#save call)
     result = UsernameChanger.change(user, params[:new_username], current_user)
-    raise Discourse::InvalidParameters.new(:new_username) unless result
 
-    render json: {
-      id: user.id,
-      username: user.username
-    }
+    if result
+      render json: { id: user.id, username: user.username }
+    else
+      render_json_error(user.errors.full_messages.join(','))
+    end
   end
 
   def check_emails

config/locales/client.en.yml

@@ -689,7 +689,6 @@ en:
         title: "Change Username"
         confirm: "If you change your username, all prior quotes of your posts and @name mentions will be broken. Are you absolutely sure you want to?"
         taken: "Sorry, that username is taken."
-        error: "There was an error changing your username."
         invalid: "That username is invalid. It must only include numbers and letters"
 
       change_email:

spec/controllers/users_controller_spec.rb

@@ -867,7 +867,7 @@ describe UsersController do
 
   end
 
-  context '.username' do
+  context '#username' do
     it 'raises an error when not logged in' do
       expect { xhr :put, :username, username: 'somename' }.to raise_error(Discourse::NotLoggedIn)
     end
@@ -894,10 +894,17 @@ describe UsersController do
         expect(user.reload.username).to eq(old_username)
       end
 
-      # Bad behavior, this should give a real JSON error, not an InvalidParameters
       it 'raises an error when change_username fails' do
-        User.any_instance.expects(:save).returns(false)
-        expect { xhr :put, :username, username: user.username, new_username: new_username }.to raise_error(Discourse::InvalidParameters)
+        xhr :put, :username, username: user.username, new_username: '@'
+
+        expect(response).to_not be_success
+
+        body = JSON.parse(response.body)
+
+        expect(body['errors'].first).to include(I18n.t(
+          'user.username.short', min: User.username_length.begin
+        ))
+
         expect(user.reload.username).to eq(old_username)
       end