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

[NEW] code to get the updated messages #8857

Merged
merged 2 commits into from
Nov 16, 2017
Merged

[NEW] code to get the updated messages #8857

merged 2 commits into from
Nov 16, 2017

Conversation

ggazzo
Copy link
Member

@ggazzo ggazzo commented Nov 14, 2017

@RocketChat/core

@ggazzo ggazzo requested a review from rodrigok November 14, 2017 16:28
@engelgabriel engelgabriel temporarily deployed to rocket-chat-pr-8857 November 14, 2017 16:28 Inactive
Copy link
Contributor

@graywolf336 graywolf336 left a comment

Choose a reason for hiding this comment

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

Is there a rest api that can be added to implement this as well?

@rafaelks
Copy link
Contributor

@graywolf336 Yes, please. :-)

@ggazzo ggazzo temporarily deployed to rocket-chat-pr-8857 November 14, 2017 20:22 Inactive
@ggazzo
Copy link
Member Author

ggazzo commented Nov 14, 2017

image
@rafaelks @graywolf336 @rodrigok

@ggazzo ggazzo temporarily deployed to rocket-chat-pr-8857 November 14, 2017 20:50 Inactive
@rodrigok
Copy link
Member

@graywolf336 can you review it again?

@ggazzo
Copy link
Member Author

ggazzo commented Nov 14, 2017

as @rafaelks suggested I changed the REST endpoint to chat.syncMessages and the param name to lastUpdate

@rafaelks
Copy link
Contributor

My suggestion was actually don't return two different types of objects on the same API and don't replicate what we have in channels.history. About the name, if we want to follow some pattern, could be something like: channels.history.sync (one per type of room).

@rodrigok rodrigok merged commit 3e41dec into develop Nov 16, 2017
@rodrigok rodrigok deleted the messages-get branch November 16, 2017 18:33
@graywolf336
Copy link
Contributor

graywolf336 commented Nov 16, 2017

The api method doesn't conform to the rest of the room and messages apis, there should have been something like @rafaelks said. That's why I didn't review it again, because there were changes needed that were suggested.

@rafaelks
Copy link
Contributor

I agree. Will we update the name of the method, or keep the same? @graywolf336 @ggazzo @rodrigok

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

Successfully merging this pull request may close these issues.

None yet

5 participants