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

fix(User): fix bot and system properties being incorrect in some cases #5923

Merged
merged 5 commits into from Jun 27, 2021

Conversation

ImRodry
Copy link
Contributor

@ImRodry ImRodry commented Jun 24, 2021

Please describe the changes this PR makes and why it should be merged:
Due to the system property only being present in user objects when the user is a system user, it would end up always being null instead of false like it should be. This PR fixes that so that it's true for system users, false for non-system users and null for partial users (regardless of whether they're system or not, since we don't know that)
Another issue in the User class was that the bot property would always be false for partial users due to the check typeof this.bot !== 'boolean' which would return true since this.bot was set to null previously. This fixes that so that it's null for partial users, true for bots and false for regular users.

⚠️ I did test these changes but only by creating partial objects myself since I couldn't find a way to easily obtain an actual partial user object. Please let me know if there's something that could go wrong that I didn't pick up in my tests.

Status and versioning classification:

  • Code changes have been tested against the Discord API, or there are no code changes
  • I know how to update typings and have done so, or typings don't need updating

Copy link
Member

@SpaceEEC SpaceEEC left a comment

Choose a reason for hiding this comment

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

When patching an already cached user with partial data (happens in audit logs and wherever we'd get a partial user in the future), the bot and system property would be assigned false.

Can be reproduced by the following steps:

  • Enable the USER partial
  • Add/remove a role from/to a bot
  • Fetch audit log
  • Inspect User#bot of that bot and find false

Same thing should apply to system, but I don't have a system user to assign a role to at hand 😄

The ideal behavior of structure._patch({}) for potentially partial structures would be no changes. (Although this is not yet the case for all instances, but we are slowly getting there.)

@ImRodry
Copy link
Contributor Author

ImRodry commented Jun 25, 2021

When patching an already cached user with partial data (happens in audit logs and wherever we'd get a partial user in the future), the bot and system property would be assigned false.

Can be reproduced by the following steps:

  • Enable the USER partial
  • Add/remove a role from/to a bot
  • Fetch audit log
  • Inspect User#bot of that bot and find false

Same thing should apply to system, but I don't have a system user to assign a role to at hand 😄

The ideal behavior of structure._patch({}) for potentially partial structures would be no changes. (Although this is not yet the case for all instances, but we are slowly getting there.)

I tried to reproduce this and got the same behavior but that's because the user is not classified as a partial since it's username is a string (at least it was in my case).
I believe this happened because bot was not present in the data used to create the user but the partial property returned false so it's technically the expected behavior. I believe partial should have more checks than just the type of the user's username to prevent situations like this.

@ImRodry
Copy link
Contributor Author

ImRodry commented Jun 25, 2021

The previously mentioned issue should be fixed as I made it so that it takes both the received data and the existing data for the user into account. So if one isn't present, the other one will be kept instead of being overwritten with false for non-partial users. Please let me know if there are any other issues with this approach

typings/index.d.ts Show resolved Hide resolved
@iCrawl iCrawl dismissed stale reviews from vladfrangu and SpaceEEC June 25, 2021 16:15

Stale

src/structures/User.js Outdated Show resolved Hide resolved
src/structures/User.js Outdated Show resolved Hide resolved
@iCrawl iCrawl merged commit e44ae96 into discordjs:master Jun 27, 2021
@ImRodry ImRodry deleted the fix-user-properties branch June 27, 2021 16:02
@iCrawl iCrawl added this to the Version 13 milestone Jun 27, 2021
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

7 participants