diff --git a/app/assets/javascripts/discourse/controllers/preferences/username.js.es6 b/app/assets/javascripts/discourse/controllers/preferences/username.js.es6 index f6cfa7be70e..6317b07d67e 100644 --- a/app/assets/javascripts/discourse/controllers/preferences/username.js.es6 +++ b/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({ } }); - - diff --git a/app/assets/javascripts/discourse/templates/preferences-username.hbs b/app/assets/javascripts/discourse/templates/preferences-username.hbs index ae27149d9d2..fc68eccedc8 100644 --- a/app/assets/javascripts/discourse/templates/preferences-username.hbs +++ b/app/assets/javascripts/discourse/templates/preferences-username.hbs @@ -7,14 +7,6 @@ - {{#if error}} -
-
-
{{i18n 'user.change_username.error'}}
-
-
- {{/if}} -
diff --git a/app/controllers/users_controller.rb b/app/controllers/users_controller.rb index b41acf4597d..8b17c46177e 100644 --- a/app/controllers/users_controller.rb +++ b/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 diff --git a/config/locales/client.en.yml b/config/locales/client.en.yml index 1c782f51d44..1a4cd78fd8b 100644 --- a/config/locales/client.en.yml +++ b/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: diff --git a/spec/controllers/users_controller_spec.rb b/spec/controllers/users_controller_spec.rb index 37303fb7686..9b4962b81ad 100644 --- a/spec/controllers/users_controller_spec.rb +++ b/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