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

[APIV4] GET /users/{user_id}/status - user status endpoint for apiV4 #5824

Merged
merged 2 commits into from Mar 24, 2017
Merged

[APIV4] GET /users/{user_id}/status - user status endpoint for apiV4 #5824

merged 2 commits into from Mar 24, 2017

Conversation

cpanato
Copy link
Contributor

@cpanato cpanato commented Mar 21, 2017

Summary

This PR implements the GET /users/{user_id}/status - user status endpoint for apiV4

Ticket Link

n/a

Checklist

api4/status.go Outdated
c.Err = err
return
} else {
resp["status"] = statusMap[0].Status
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we need to check if len(statusMap) == 0 first and return a 404 NotFound error if it is. Also can we use statusMap[0].ToJson() as the response body?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ah! true!

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

api4/status.go Outdated
} else {
if len(statusMap) == 0 {
c.Err = model.NewLocAppError("UserStatus", "api.status.user_not_found.app_error", nil, "")
c.Err.StatusCode = http.StatusNotFound
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Super minor but can we do c.Err = model.NewAppError("UserStatus", "api.status.user_not_found.app_error", nil, "", http.StatusNotFound) instead? Minor difference between NewAppError and NewLocAppError :)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

@@ -22,7 +22,7 @@ type Status struct {
Status string `json:"status"`
Manual bool `json:"manual"`
LastActivityAt int64 `json:"last_activity_at"`
ActiveChannel string `json:"active_channel" db:"-"`
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks like it'll remove the active_channel field from the existing API. What are the consequences of this from a backwards compatibility point of view?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I will revert, since I revert the manual as well

Copy link
Member

@jwilander jwilander Mar 24, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is not returned in APIv3 at all, so backwards compatibility is no issue. We should not return ActiveChannel to the client since it doesn't need it and because it would be leaking channel IDs

@jwilander
Copy link
Member

Please see my comment, we should switch back to

ActiveChannel  string `json:"-" db:"-"`

@grundleborg grundleborg merged commit 5bf6ae0 into mattermost:master Mar 24, 2017
@cpanato cpanato deleted the apiV4_userStatus branch March 24, 2017 17:24
@jasonblais jasonblais added Changelog/Not Needed Does not require a changelog entry Docs/Done Required documentation has been written Tests/Not Needed New release tests not required labels Mar 27, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
2: Dev Review Requires review by a developer Changelog/Not Needed Does not require a changelog entry Docs/Done Required documentation has been written Tests/Not Needed New release tests not required
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants