Skip to content

Conversation

@gabriellsh
Copy link
Member

@gabriellsh gabriellsh commented Apr 5, 2021

Proposed changes (including videos or screenshots)

Fixed some race conditions in admin.

Self DMs used to be created with the userId duplicated. Sometimes rooms can have 2 equal uids, but it's a self DM. Fixed a getter so this isn't a problem anymore.

Issue(s)

Steps to test or reproduce

Further comments

@gabriellsh gabriellsh requested a review from a team April 5, 2021 19:53
Copy link
Member

@dougfabris dougfabris left a comment

Choose a reason for hiding this comment

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

Remove the comments on the code and use this nice information on the PR's description.

@gabriellsh
Copy link
Member Author

Remove the comments on the code and use this nice information on the PR's description.

IMHO, this information is relevant enough to be in the code. I fear someone might want to change this without the proper consideration. WDYT?

I can add it to the PR also though, no problem.

@dougfabris
Copy link
Member

dougfabris commented Apr 5, 2021

Remove the comments on the code and use this nice information on the PR's description.

IMHO, this information is relevant enough to be in the code. I fear someone might want to change this without the proper consideration. WDYT?

I can add it to the PR also though, no problem.

If you think it's a piece of relevant information to be there, go ahead! But remember, if you need to explain what a code does, it isn't good enough. Maybe it's not the case here!

dougfabris
dougfabris previously approved these changes Apr 5, 2021
@gabriellsh
Copy link
Member Author

I agree! We shouldn't need to explain code, I'm all for that. But I think in specific cases it's needed, since we can't fix the "root" problem and this is a workaround. The comments don't explain the code itself, as it's pretty clear what it does. It explains why that was the choice.

I'm not trying to justify the comments BTW, thats just my opinion. Maybe @ggazzo can comment about this too, since both our POVs are relevant in this case.

…user_info

* 'develop' of github.com:RocketChat/Rocket.Chat: (40 commits)
  [FIX] Typos/missing elements in the French translation (#21525)
  [FIX] Archive permissions for room moderator (#21563)
  [FIX] Checking 'start-discussion' Permission for MessageBox Actions (#21564)
  [FIX] Correcting the case there are no result in admin users list (#21556)
  [FIX] Don't allow whitespace on bold, italic and strike (#21483)
  [FIX] Message Block ordering (#21464)
  [IMPROVE] Add proxy for data export (#20998)
  [FIX] Updating a message causing URLs to be parsed even within markdown code (#21489)
  Bump version to 3.13.2
  [FIX] Fix the bugs opening discussions (#21557)
  A React-based replacement for BlazeLayout (#21527)
  Language update from LingoHub 🤖 on 2021-04-12Z (#21530)
  Chore: Increase testing coverage on password policy class (#21482)
  Chore: Meteor update to 2.1.1 (#21494)
  Chore: Do not stop animations on Test Mode (#21484)
  Chore: Remove control character from room model operation (#21493)
  [NEW] New set of rules for client code (#21318)
  Bump version to 3.13.1
  [FIX] Header component breaking if user is not part of teams room (#21465)
  [FIX] Admin Users list pagination (#21469)
  ...
@ggazzo ggazzo merged commit d6ff6bd into develop Apr 16, 2021
@ggazzo ggazzo deleted the fix/user_info branch April 16, 2021 04:38
@sampaiodiego sampaiodiego mentioned this pull request Apr 28, 2021
@dougfabris dougfabris linked an issue Apr 28, 2021 that may be closed by this pull request
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.

[Bug] Users self chat are duplicated

4 participants