diff --git a/app/controllers/client_api/users/users_controller.rb b/app/controllers/client_api/users/users_controller.rb index 98c663c40..89d148d76 100644 --- a/app/controllers/client_api/users/users_controller.rb +++ b/app/controllers/client_api/users/users_controller.rb @@ -57,19 +57,21 @@ module ClientApi end def update - user_service = ClientApi::UserService.new( + service = ClientApi::Users::UpdateService.new( current_user: current_user, params: user_params ) - if user_service.update_user! + result = service.execute + + if result[:status] == :success bypass_sign_in(current_user) success_response else - unsuccess_response(current_user.errors.full_messages, - :unprocessable_entity) + error_response( + message: result[:message], + details: service.user.errors + ) end - rescue CustomUserError => error - unsuccess_response(error.to_s) end private @@ -84,22 +86,35 @@ module ClientApi :system_message_email_notification) end - def success_response(template = nil, locals = nil) + def success_response(args = {}) + template = args.fetch(:template) { nil } + locals = args.fetch(:locals) { {} } + details = args.fetch(:details) { {} } + respond_to do |format| format.json do - if template && locals - render template: template, status: :ok, locals: locals + if template + render template: template, + status: :ok, + locals: locals else - render json: {}, status: :ok + render json: { details: details }, status: :ok end end end end - def unsuccess_response(message, status = :unprocessable_entity) + def error_response(args = {}) + message = args.fetch(:message) { t('client_api.generic_error_message') } + details = args.fetch(:details) { {} } + status = args.fetch(:status) { :unprocessable_entity } + respond_to do |format| format.json do - render json: { message: message }, + render json: { + message: message, + details: details + }, status: status end end diff --git a/app/javascript/src/components/validation/components/ValidatedErrorHelpBlock.jsx b/app/javascript/src/components/validation/components/ValidatedErrorHelpBlock.jsx index 1dc37a432..2e9b1afa8 100644 --- a/app/javascript/src/components/validation/components/ValidatedErrorHelpBlock.jsx +++ b/app/javascript/src/components/validation/components/ValidatedErrorHelpBlock.jsx @@ -30,7 +30,6 @@ class ValidatedErrorHelpBlock extends Component { super(props); this.cleanProps = this.cleanProps.bind(this); - this.cleanProps = this.cleanProps.bind(this); } cleanProps() { diff --git a/app/javascript/src/components/validation/components/ValidatedForm.jsx b/app/javascript/src/components/validation/components/ValidatedForm.jsx index 7005decdc..5b15a251a 100644 --- a/app/javascript/src/components/validation/components/ValidatedForm.jsx +++ b/app/javascript/src/components/validation/components/ValidatedForm.jsx @@ -4,6 +4,14 @@ import PropTypes from "prop-types"; import _ from "lodash"; class ValidatedForm extends Component { + static parseErrors(errors) { + // This method is quite smart, in the sense that accepts either + // errors in 3 shapes: localized error messages ({}), + // unlocalized error messages ({}), or mere strings (unlocalized) + const arr = _.isString(errors) ? [errors] : errors; + return arr.map((el) => _.isString(el) ? { message: el } : el); + } + constructor(props) { super(props); @@ -34,19 +42,17 @@ class ValidatedForm extends Component { } setErrors(errors) { - // This method is quite smart, in the sense that accepts either - // errors in 3 shapes: localized error messages ({}), - // unlocalized error messages ({}), or mere strings (unlocalized) const newState = {}; _.entries(errors).forEach(([key, value]) => { - const arr = _.isString(value) ? [value] : value; - newState[key] = arr.map((el) => _.isString(el) ? { message: el } : el); + newState[key] = ValidatedForm.parseErrors(value); }); this.setState(newState); } setErrorsForTag(tag, errors) { - const newState = update(this.state, { [tag]: { $set: errors } }); + const newState = update(this.state, { + [tag]: { $set: ValidatedForm.parseErrors(errors) } + }); this.setState(newState); } diff --git a/app/javascript/src/components/validation/components/ValidatedFormControl.jsx b/app/javascript/src/components/validation/components/ValidatedFormControl.jsx index c6c54dc72..6297b4ce7 100644 --- a/app/javascript/src/components/validation/components/ValidatedFormControl.jsx +++ b/app/javascript/src/components/validation/components/ValidatedFormControl.jsx @@ -16,7 +16,7 @@ class ValidatedFormControl extends Component { const value = e.target.value; // Pass-through "original" onChange - if (_.has(this.props, "onChange")) { + if (_.has(this.props, "onChange") && this.props.onChange !== undefined) { this.props.onChange(e); } diff --git a/app/javascript/src/components/validation/validators/text_validators.js b/app/javascript/src/components/validation/validators/text_validators.js index 2a6c61aa1..d2ad5233f 100644 --- a/app/javascript/src/components/validation/validators/text_validators.js +++ b/app/javascript/src/components/validation/validators/text_validators.js @@ -2,15 +2,18 @@ import _ from "lodash"; import { NAME_MIN_LENGTH, NAME_MAX_LENGTH, - TEXT_MAX_LENGTH + TEXT_MAX_LENGTH, + PASSWORD_MIN_LENGTH, + PASSWORD_MAX_LENGTH, + USER_INITIALS_MAX_LENGTH } from "../../../config/constants/numeric"; +import { EMAIL_REGEX } from "../../../config/constants/strings"; export const nameMinLengthValidator = (value, messageIds = {}) => { const messageId = _.has(messageIds, "text_too_short") ? messageIds.text_too_short : "validators.text_validators.text_too_short"; - if (value.length < NAME_MIN_LENGTH) { return [{ intl: true, @@ -44,6 +47,20 @@ export const nameLengthValidator = (value, messageIds = {}) => { return nameMaxLengthValidator(value, messageIds); }; +export const textBlankValidator = (value, messageIds = {}) => { + const messageId = _.has(messageIds, "text_blank") ? + messageIds.text_blank : + "validators.text_validators.text_blank"; + + if (value.length === 0) { + return [{ + intl: true, + messageId + }]; + } + return []; +} + export const textMaxLengthValidator = (value, messageIds = {}) => { const messageId = _.has(messageIds, "text_too_long") ? messageIds.text_too_long : @@ -57,4 +74,59 @@ export const textMaxLengthValidator = (value, messageIds = {}) => { }]; } return []; +}; + +export const passwordLengthValidator = (value, messageIds = {}) => { + const messageIdTooShort = _.has(messageIds, "text_too_short") ? + messageIds.text_too_short : + "validators.text_validators.text_too_short"; + const messageIdTooLong = _.has(messageIds, "text_too_long") ? + messageIds.text_too_long : + "validators.text_validators.text_too_long"; + + if (value.length < PASSWORD_MIN_LENGTH) { + return [{ + intl: true, + messageId: messageIdTooShort, + values:{ min_length: PASSWORD_MIN_LENGTH } + }]; + } else if (value.length > PASSWORD_MAX_LENGTH) { + return [{ + intl: true, + messageId: messageIdTooLong, + values:{ max_length: PASSWORD_MAX_LENGTH } + }]; + } + return []; +}; + +export const userInitialsMaxLengthValidator = (value, messageIds = {}) => { + const messageId = _.has(messageIds, "text_too_long") ? + messageIds.text_too_long : + "validators.text_validators.text_too_long"; + + if (value.length > USER_INITIALS_MAX_LENGTH) { + return [{ + intl: true, + messageId, + values: { max_length: USER_INITIALS_MAX_LENGTH } + }]; + } + return []; +}; + +export const emailValidator = (value, messageIds = {}) => { + const res = textBlankValidator(value, messageIds); + if (res.length > 0) { + return res; + } + + const messageId = _.has(messageIds, "invalid_email") ? + messageIds.invalid_email : + "validators.text_validators.invalid_email"; + + if (!EMAIL_REGEX.test(value)) { + return [{ intl: true, messageId }]; + } + return []; }; \ No newline at end of file diff --git a/app/javascript/src/config/locales/messages.js b/app/javascript/src/config/locales/messages.js index fa87b5bf9..3cbbdeb25 100644 --- a/app/javascript/src/config/locales/messages.js +++ b/app/javascript/src/config/locales/messages.js @@ -19,7 +19,9 @@ export default { validators: { text_validators: { text_too_short: "is too short (minimum is {min_length} characters)", - text_too_long: "is too long (maximum is {max_length} characters)" + text_too_long: "is too long (maximum is {max_length} characters)", + text_blank: "can't be blank", + invalid_email: "invalid email" } }, error_messages: { diff --git a/app/javascript/src/scenes/SettingsPage/scenes/profile/components/InputEnabled.jsx b/app/javascript/src/scenes/SettingsPage/scenes/profile/components/InputEnabled.jsx index 04f087031..7efacc543 100644 --- a/app/javascript/src/scenes/SettingsPage/scenes/profile/components/InputEnabled.jsx +++ b/app/javascript/src/scenes/SettingsPage/scenes/profile/components/InputEnabled.jsx @@ -4,29 +4,35 @@ import { string, func } from "prop-types"; import styled from "styled-components"; import { FormattedMessage, FormattedHTMLMessage } from "react-intl"; import { - FormGroup, - FormControl, ControlLabel, Button, ButtonToolbar, - HelpBlock } from "react-bootstrap"; +import update from "immutability-helper"; import { updateUser } from "../../../../../services/api/users_api"; import { transformName } from "../../../../../services/helpers/string_helper"; import { addAlert } from "../../../../../components/actions/AlertsActions"; import { BORDER_LIGHT_COLOR, - COLOR_APPLE_BLOSSOM } from "../../../../../config/constants/colors"; import { ENTER_KEY_CODE, - USER_INITIALS_MAX_LENGTH, - NAME_MAX_LENGTH, - PASSWORD_MAX_LENGTH, - PASSWORD_MIN_LENGTH } from "../../../../../config/constants/numeric"; -import { EMAIL_REGEX } from "../../../../../config/constants/strings"; +import { + ValidatedForm, + ValidatedFormGroup, + ValidatedFormControl, + ValidatedErrorHelpBlock, + ValidatedSubmitButton +} from "../../../../../components/validation"; +import { + textBlankValidator, + nameMaxLengthValidator, + passwordLengthValidator, + userInitialsMaxLengthValidator, + emailValidator +} from "../../../../../components/validation/validators/text_validators"; const StyledInputEnabled = styled.div` border: 1px solid ${BORDER_LIGHT_COLOR}; @@ -38,10 +44,6 @@ const StyledInputEnabled = styled.div` } `; -const StyledHelpBlock = styled(HelpBlock)` - color: ${COLOR_APPLE_BLOSSOM}; -`; - class InputEnabled extends Component { constructor(props) { super(props); @@ -49,30 +51,16 @@ class InputEnabled extends Component { this.state = { value: this.props.inputValue === "********" ? "" : this.props.inputValue, current_password: "", - password_confirmation: "", - errorMessage: "" + password_confirmation: "" }; - this.handleChange = this.handleChange.bind(this); - this.handlePasswordConfirmation = this.handlePasswordConfirmation.bind( - this - ); this.handleKeyPress = this.handleKeyPress.bind(this); - this.confirmationField = this.confirmationField.bind(this); - this.handleSubmit = this.handleSubmit.bind(this); - this.getValidationState = this.getValidationState.bind(this); - this.handleFullNameValidation = this.handleFullNameValidation.bind(this); - this.handleEmailValidation = this.handleEmailValidation.bind(this); - this.handleInitialsValidation = this.handleInitialsValidation.bind(this); - this.handlePasswordConfirmationValidation = this.handlePasswordConfirmationValidation.bind( - this - ); + this.handleChange = this.handleChange.bind(this); this.handleCurrentPassword = this.handleCurrentPassword.bind(this); - this.handleFileChange = this.handleFileChange.bind(this); - } - - getValidationState() { - return this.state.errorMessage.length > 0 ? "error" : null; + this.handlePasswordConfirmation = this.handlePasswordConfirmation.bind(this); + this.handleSubmit = this.handleSubmit.bind(this); + this.inputField = this.inputField.bind(this); + this.confirmationField = this.confirmationField.bind(this); } handleKeyPress(event) { @@ -83,170 +71,30 @@ class InputEnabled extends Component { } handleChange(event) { - event.preventDefault(); - switch (this.props.dataField) { - case "full_name": - this.handleFullNameValidation(event); - break; - case "email": - this.handleEmailValidation(event); - break; - case "initials": - this.handleInitialsValidation(event); - break; - case "password": - this.handlePasswordValidation(event); - break; - case "avatar": - this.handleFileChange(event); - break; - default: - this.setState({ value: event.target.value, errorMessage: "" }); - } - } - - handleFileChange(event) { - this.setState({ value: event.currentTarget.files[0], errorMessage: "" }); - } - - handlePasswordConfirmation(event) { - const { value } = event.target; - if (value.length === 0) { - this.setState({ - password_confirmation: value, - errorMessage: - }); - } - this.setState({ password_confirmation: value }); - } - - handleFullNameValidation(event) { - const { value } = event.target; - if (value.length > NAME_MAX_LENGTH) { - this.setState({ - value, - errorMessage: ( - - ) - }); - } else if (value.length === 0) { - this.setState({ - value, - errorMessage: - }); + let newVal; + if (this.props.dataField === "avatar") { + newVal = event.currentTarget.files[0]; } else { - this.setState({ value, errorMessage: "" }); - } - } - - handleEmailValidation(event) { - const { value } = event.target; - if (!EMAIL_REGEX.test(value)) { - this.setState({ - value, - errorMessage: - }); - } else if (value.length === 0) { - this.setState({ - value, - errorMessage: - }); - } else { - this.setState({ value, errorMessage: "" }); - } - } - - handleInitialsValidation(event) { - const { value } = event.target; - if (value.length > USER_INITIALS_MAX_LENGTH) { - this.setState({ - value, - errorMessage: ( - - ) - }); - } else if (value.length === 0) { - this.setState({ - value, - errorMessage: - }); - } else { - this.setState({ value, errorMessage: "" }); - } - } - - handlePasswordValidation(event) { - const { value } = event.target; - if (value.length > PASSWORD_MAX_LENGTH) { - this.setState({ - value, - errorMessage: ( - - ) - }); - } else if (value.length < PASSWORD_MIN_LENGTH) { - this.setState({ - value, - errorMessage: ( - - ) - }); - } else { - this.setState({ value, errorMessage: "" }); - } - } - - handlePasswordConfirmationValidation(event) { - const { value } = event.target; - if (value !== this.state.value) { - this.setState({ - password_confirmation: value, - errorMessage: ( - - ) - }); - } else { - this.setState({ password_confirmation: value, errorMessage: "" }); + newVal = event.target.value; } + const newState = update(this.state, { + value: { $set: newVal } + }); + this.setState(newState); } handleCurrentPassword(event) { - const { value } = event.target; - if (value.length > PASSWORD_MAX_LENGTH) { - this.setState({ - current_password: value, - errorMessage: ( - - ) - }); - } else if (value.length < PASSWORD_MIN_LENGTH) { - this.setState({ - current_password: value, - errorMessage: ( - - ) - }); - } else { - this.setState({ current_password: value, errorMessage: "" }); - } + const newState = update(this.state, { + current_password: { $set: event.target.value } + }); + this.setState(newState); + } + + handlePasswordConfirmation(event) { + const newState = update(this.state, { + password_confirmation: { $set: event.target.value } + }); + this.setState(newState); } handleSubmit(event) { @@ -292,7 +140,7 @@ class InputEnabled extends Component { } }) .catch(({ response }) => { - this.setState({ errorMessage: response.data.message.toString() }); + this.form.setErrors(response.data.details); }); } @@ -301,108 +149,154 @@ class InputEnabled extends Component { if (type === "email") { return ( -
- - + + + + -
+ + ); } return ""; } inputField() { - const { inputType } = this.props; + const { inputType, dataField } = this.props; + + let validatorsOnChange = []; + if (dataField === "full_name") { + validatorsOnChange = [textBlankValidator, nameMaxLengthValidator]; + } else if (dataField === "initials") { + validatorsOnChange = [textBlankValidator, userInitialsMaxLengthValidator]; + } else if (dataField === "email") { + validatorsOnChange = [emailValidator]; + } + if (inputType === "password") { return (
- - - - - - - - - - + + + + + + + + + + + + + + + + + + + + +
); } if (inputType === "file") { return ( - + + + + ); } return ( - + + + + ); } render() { return ( - -
- -

-   - -

- {this.props.labelValue !== "none" && ( - - - - )} - {this.inputField()} - {this.confirmationField()} - {this.state.errorMessage} - - - - -
-
+ + { this.form = f; }} + > +

+   + +

+ {this.props.labelValue !== "none" && ( + + + + )} + {this.inputField()} + {this.confirmationField()} + + + + + + +
); } diff --git a/app/services/client_api/user_service.rb b/app/services/client_api/user_service.rb deleted file mode 100644 index 15d87f27f..000000000 --- a/app/services/client_api/user_service.rb +++ /dev/null @@ -1,28 +0,0 @@ -module ClientApi - class UserService < BaseService - def update_user! - error = I18n.t('client_api.user.password_invalid') - raise CustomUserError, error unless check_current_password - @params.delete(:current_password) # removes unneeded element - @current_user.update(@params) - end - - private - - def check_current_password - return true unless @params[:email] || @params[:password] - pass_blank_err = I18n.t('client_api.user.blank_password_error') - pass_match_err = I18n.t('client_api.user.passwords_dont_match') - current_password = @params[:current_password] - raise CustomUserError, pass_blank_err unless current_password - raise CustomUserError, pass_match_err unless check_password_confirmation - @current_user.valid_password? current_password - end - - def check_password_confirmation - return true if @params[:email] - @params[:password] == @params[:password_confirmation] - end - end - CustomUserError = Class.new(StandardError) -end diff --git a/app/services/client_api/users/update_service.rb b/app/services/client_api/users/update_service.rb new file mode 100644 index 000000000..161976f4b --- /dev/null +++ b/app/services/client_api/users/update_service.rb @@ -0,0 +1,53 @@ +module ClientApi + module Users + class UpdateService < BaseService + attr_accessor :user + + def execute + @user = @current_user + + if current_password_valid? && + password_confirmation_valid? && + @user.update(@params.except(:current_password)) + success + else + error(@user.errors.full_messages.uniq.join('. ')) + end + end + + private + + def current_password_valid? + # Only check for current_password when updating + # email or password + return true unless @params[:email] || @params[:password] + + if @user.valid_password?(@params[:current_password]) + return true + else + @user.errors.add( + :current_password, + I18n.t('client_api.user.current_password_invalid') + ) + return false + end + end + + def password_confirmation_valid? + # Only check for password_confirmation when + # updating password + return true unless @params[:password] + + if @params[:password] == @params[:password_confirmation] + return true + else + @user.errors.add( + :password_confirmation, + I18n.t('client_api.user.password_confirmation_not_match') + ) + return false + end + end + end + end +end diff --git a/config/locales/en.yml b/config/locales/en.yml index 533c8df61..d1ac3eff6 100644 --- a/config/locales/en.yml +++ b/config/locales/en.yml @@ -1822,9 +1822,7 @@ en: leave_team_error: "An error occured." leave_flash: "Successfuly left team %{team}." user: - blank_password_error: "Password can't be blank!" - passwords_dont_match: "Passwords don't match" - password_invalid: "Password is invalid!" - avatar_too_big: "Avatar file size must be less than 0.2 MB" + current_password_invalid: "incorrect password" + password_confirmation_not_match: "doesn't match" invite_users: permission_error: "You don't have permission to invite additional users to team. Contact its administrator/s." diff --git a/features/settings_page/profile.feature b/features/settings_page/profile.feature index 6ba58a9b2..e8c470614 100644 --- a/features/settings_page/profile.feature +++ b/features/settings_page/profile.feature @@ -103,7 +103,7 @@ Scenario: Unsuccessful Password Change, current password is invalid And I fill in "mypassword5678" in New password field And I fill in "mypassword5678" in New password confirmation field Then I click "Update" button - And I should see "Password is invalid!" + And I should see "incorrect password" @javascript Scenario: Successful Password Change diff --git a/spec/services/client_api/user_service_spec.rb b/spec/services/client_api/user_service_spec.rb deleted file mode 100644 index 2cc665785..000000000 --- a/spec/services/client_api/user_service_spec.rb +++ /dev/null @@ -1,80 +0,0 @@ -require 'rails_helper' - -describe ClientApi::UserService do - let(:user) do - create :user, - full_name: 'User One', - initials: 'UO', - email: 'user@happy.com', - password: 'asdf1234', - password_confirmation: 'asdf1234' - end - - describe '#update_user!' do - it 'should update user email if the password is correct' do - email = 'new_user@happy.com' - params = { email: email, current_password: 'asdf1234' } - user_service = ClientApi::UserService.new(current_user: user, - params: params) - user_service.update_user! - expect(user.email).to eq(email) - end - - it 'should raise CustomUserError error if the password is not correct' do - email = 'new_user@happy.com' - params = { email: email, current_password: 'banana' } - user_service = ClientApi::UserService.new(current_user: user, - params: params) - expect { - user_service.update_user! - }.to raise_error(ClientApi::CustomUserError) - end - - it 'should update initials and full name without password confirmation' do - full_name = 'Happy User' - initials = 'HU' - user_service = ClientApi::UserService.new( - current_user: user, - params: { full_name: full_name, initials: initials } - ) - user_service.update_user! - expect(user.full_name).to eq(full_name) - expect(user.initials).to eq(initials) - end - - it 'should raise an error if current password not present' do - user_service = ClientApi::UserService.new( - current_user: user, - params: { password: 'hello1234', password_confirmation: 'hello1234' } - ) - expect { - user_service.update_user! - }.to raise_error(ClientApi::CustomUserError) - end - - it 'should raise an error if password_confirmation don\'t match' do - user_service = ClientApi::UserService.new( - current_user: user, - params: { password: 'hello1234', - password_confirmation: 'hello1234567890', - current_password: 'asdf1234' } - ) - - expect { - user_service.update_user! - }.to raise_error(ClientApi::CustomUserError, 'Passwords don\'t match') - end - - it 'should update the password' do - new_password = 'hello1234' - user_service = ClientApi::UserService.new( - current_user: user, - params: { password: new_password, - password_confirmation: new_password, - current_password: 'asdf1234' } - ) - user_service.update_user! - expect(user.valid_password?(new_password)).to be(true) - end - end -end diff --git a/spec/services/client_api/users/update_service_spec.rb b/spec/services/client_api/users/update_service_spec.rb new file mode 100644 index 000000000..81998a9e0 --- /dev/null +++ b/spec/services/client_api/users/update_service_spec.rb @@ -0,0 +1,79 @@ +require 'rails_helper' + +include ClientApi::Users + +describe ClientApi::Users::UpdateService do + let(:user) do + create :user, + full_name: 'User One', + initials: 'UO', + email: 'user@happy.com', + password: 'asdf1234', + password_confirmation: 'asdf1234' + end + + it 'should update user email if the password is correct' do + email = 'new_user@happy.com' + params = { email: email, current_password: 'asdf1234' } + service = UpdateService.new(current_user: user, + params: params) + result = service.execute + expect(result[:status]).to eq :success + expect(user.email).to eq(email) + end + + it 'should raise CustomUserError error if the password is not correct' do + email = 'new_user@happy.com' + params = { email: email, current_password: 'banana' } + service = UpdateService.new(current_user: user, + params: params) + result = service.execute + expect(result[:status]).to eq :error + end + + it 'should update initials and full name without password confirmation' do + full_name = 'Happy User' + initials = 'HU' + service = UpdateService.new( + current_user: user, + params: { full_name: full_name, initials: initials } + ) + result = service.execute + expect(result[:status]).to eq :success + expect(user.full_name).to eq(full_name) + expect(user.initials).to eq(initials) + end + + it 'should raise an error if current password not present' do + service = UpdateService.new( + current_user: user, + params: { password: 'hello1234', password_confirmation: 'hello1234' } + ) + result = service.execute + expect(result[:status]).to eq :error + end + + it 'should raise an error if password_confirmation don\'t match' do + service = UpdateService.new( + current_user: user, + params: { password: 'hello1234', + password_confirmation: 'hello1234567890', + current_password: 'asdf1234' } + ) + result = service.execute + expect(result[:status]).to eq :error + end + + it 'should update the password' do + new_password = 'hello1234' + service = UpdateService.new( + current_user: user, + params: { password: new_password, + password_confirmation: new_password, + current_password: 'asdf1234' } + ) + result = service.execute + expect(result[:status]).to eq :success + expect(user.valid_password?(new_password)).to be(true) + end +end