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

Regression: sidebar sorting was being wrong in some cases where the rooms records were returned before the subscriptions #11273

Merged
merged 4 commits into from
Jun 27, 2018

Conversation

ggazzo
Copy link
Member

@ggazzo ggazzo commented Jun 27, 2018

Closes #ISSUE_NUMBER

@engelgabriel engelgabriel temporarily deployed to rocket-chat-pr-11273 June 27, 2018 15:36 Inactive
@ggazzo ggazzo requested a review from sampaiodiego June 27, 2018 15:36
@ggazzo ggazzo temporarily deployed to rocket-chat-pr-11273 June 27, 2018 17:41 Inactive
@ggazzo ggazzo temporarily deployed to rocket-chat-pr-11273 June 27, 2018 17:47 Inactive
const sub = RocketChat.models.Subscriptions.findOne({ rid: room._id });
if (!sub) {
return;
return room;
Copy link
Member

Choose a reason for hiding this comment

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

Why return something?

Copy link
Member Author

Choose a reason for hiding this comment

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

actually there are no difference (callbacks use the last value returned !== undefined, just to avoid some misunderstanding.

const sub = RocketChat.models.Subscriptions.findOne({ rid: room._id });
if (!sub) {
return;
return room;
}
const $set = {lastMessage : room.lastMessage, lm: room._updatedAt, ...getLowerCaseNames(room, sub.name)};
Copy link
Member

Choose a reason for hiding this comment

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

Please break this object in multiple lines and remove the space before the first colon.

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm ok with the spaces (our linter could help with that) but I dont like create a tabbed simple querys =/

const sub = RocketChat.models.Subscriptions.findOne({ rid: room._id });
if (!sub) {
return;
return room;
}
const $set = {lastMessage : room.lastMessage, lm: room._updatedAt, ...getLowerCaseNames(room, sub.name)};
RocketChat.models.Subscriptions.update({ rid: room._id }, {$set});
Copy link
Member

Choose a reason for hiding this comment

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

Why not declare the set here instead of have a variable and pass it here?

@ggazzo ggazzo temporarily deployed to rocket-chat-pr-11273 June 27, 2018 20:44 Inactive
@@ -0,0 +1,5 @@
import redisMq from 'mqemitter-redis';
Copy link
Member

Choose a reason for hiding this comment

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

Please remove this changes

@rodrigok rodrigok changed the title [FIX] sort sidebar Regression: sidebar sorting was being wrong in some cases where the rooms records were returned before the subscriptions Jun 27, 2018
@ggazzo ggazzo temporarily deployed to rocket-chat-pr-11273 June 27, 2018 22:32 Inactive
rodrigok
rodrigok previously approved these changes Jun 27, 2018
@ggazzo ggazzo temporarily deployed to rocket-chat-pr-11273 June 27, 2018 22:51 Inactive
@rodrigok rodrigok merged commit 204baf6 into develop Jun 27, 2018
@rodrigok rodrigok added this to the 0.66.0 milestone Jun 27, 2018
@rodrigok rodrigok added this to Review/QA in June/2018 via automation Jun 27, 2018
@rodrigok rodrigok deleted the fix-sort-list branch June 27, 2018 23:20
@rodrigok rodrigok mentioned this pull request Jun 28, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
June/2018
  
Review/QA
Development

Successfully merging this pull request may close these issues.

None yet

3 participants