Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Standardize how error messages are sent from the server #2727

Closed
sampaiodiego opened this issue Mar 31, 2016 · 8 comments · Fixed by #3040
Closed

Standardize how error messages are sent from the server #2727

sampaiodiego opened this issue Mar 31, 2016 · 8 comments · Fixed by #3040

Comments

@sampaiodiego
Copy link
Member

Today error messages thrown from the server are not standardized. For example:

I propose we thrown errors like the following:

  • the "error code" should be a hiphenized translatable message, like: invalid-username, not-authorized. This way our interface will take care of showing the correct translation for the error
  • the error message should be written in english and being human readable. This way, any interface (other than our web interface, like REST APIs) will have a printable message

@RocketChat/core thoughts?

@engelgabriel
Copy link
Member

I agree, just thing that erro codes code start with "error-" so they will be grouped on the i18n.json files.
Does this applies only to erros or to all messages returned to method calls?

@engelgabriel engelgabriel added this to the 0.26.0 milestone Mar 31, 2016
@sampaiodiego
Copy link
Member Author

To any exception thrown with Meteor.Error

@marceloschmidt
Copy link
Member

I agree we start using "error-..." on error codes (eg.: error-invalid-room). Error message should be english and without any reference to the method being called. So, instead of "[method] archiveRoom -> Invalid room.", we should use "Invalid room.".

@engelgabriel
Copy link
Member

If we can pass more debug information as a 3rd parameter, I agree in removing the "[method] name ->" from the API messages.

@marceloschmidt
Copy link
Member

We can pass a 3rd parameter. I'll add { method: 'methodName' } to the error details. Can anyone think of any other info to get into details object? eg.: throw new Meteor.Error('error-not-authorized', 'Not authorized', { method: 'archiveRoom' }).

@engelgabriel engelgabriel modified the milestones: 0.26.0, 0.27.0 Apr 11, 2016
@marceloschmidt
Copy link
Member

We should define some standards, such as what error to throw when user is not logged in, or when the operation is not allowed because the user lacks permission, or when the user doesn't belong to the room, etc.

So, I vote for:

  1. User is not logged in: error-user-logged-out: The user is logged out
  2. Operation is not allowed: error-not-allowed: Not allowed (instead of not-authorized)
  3. Not belonging to room: error-invalid-room: Invalid room

Any other suggestions?

@marceloschmidt
Copy link
Member

Or maybe using proper error codes could be better.
So, instead of throw new Meteor.Error('error-user-logged-out', 'The user is logged out', { method: 'methodName' });, we could call throw new Meteor.Error(403, 'Forbidden', { method: 'methodName' });

That would also be the case for not being allowed or not belonging to room... in whichever case, the user is forbidden to do that action. Any specific errors would still use the error-* pattern, so error-name-is-required would translate to Name is required

@engelgabriel
Copy link
Member

I dont think http error codes are clear enough for our use cases.

@marceloschmidt marceloschmidt modified the milestones: 0.28.0, 0.27.0 Apr 18, 2016
@marceloschmidt marceloschmidt self-assigned this Apr 18, 2016
@engelgabriel engelgabriel modified the milestones: 0.29.0, 0.28.0 Apr 25, 2016
engelgabriel pushed a commit that referenced this issue Apr 26, 2016
* Add function to handle errors

* Delete message errors

* handle error for hideRoom

* Allow returning error instead of calling toastr.error

* Handle error for leaveRoom

* handle error for openRoom

* handleError for toggleFavorite

* handleError in updateMessage

* error for samlLogout

* handleError for assets

* Add global handleError to eslint

* handleError for addOAuthService

* handleError: getUserRoles

* handleError: insertOrUpdateUsere

* handleError: messageDeleting

* handleError: removeUserFromRoles

* handleError: addPermissionToRole

* handleError: addUserToRole

* handleError: deleteRole

* handleError: removeRoleFromPermission

* handleError: removeUserFromRole

* handleError: saveRole

* Return ready on publish without permission

* handleError: channel-settings

* handleError: mailMessages

* handleError: fileUpload

* handleError: rocketchat-importer

* handleError: addIncomingIntegration

* handleError: deleteIncomingIntegration

* handleError: updateIncomingIntegration

* handleError: addOutgoingIntegration

* handleError: deleteOutgoingIntegration

* handleError: updateOutgoingIntegration

* Return ready on publish without permission

* handleError ldap

* remove throw from client code

* handleError: setEmail, slashCommand

* Sort en.i18n.json

* Google translated languages

* Use correct error return from publishes

* RateLimiter.limitFunction

* Fix order of error "500"

* handleError validateEmailDomain

* handleError channelSettings; settings

* handleError livechat

* handleError: Mailer.sendMail

* handleError pinMessage and unpinMessage

* handleError messageStarring

* handleError oauth apps

* handleError: saveNotificationSettings

* handleError getRoomRoles

* handleError: createDirectMessage

* handleError saveUserPreferences

* handleError: saveUserProfile

* handleError sendConfirmationEmail

* Add ecmascript to root

* handleError: avatar

* handleError: getStatistics

* handleError: roomSetting

* handleError: channelSettings

* handleError: sendInvitationEmail

* handleError: addUserToRoom

* handleError: uploadedFilesList

* Change error key on user edit

* handleError: userInfo

* handleError: userRegistration

* handleError: createChannel

* handleError: createPrivateGroup

* handleError: setUserPassword

* handleError setUserActiveStatus

* handleError: accoutns

* A few more errors thrown

* Error: livechat publishes

* Errors in methods

* handleError searchAgent

* Add errors handling

More errors handling

Auto-translation for all languages

* Permalink
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants