Skip to content

Conversation

doshgo
Copy link

@doshgo doshgo commented Jan 11, 2024

Pull-request

Changes

  • Existing code
  • New feature

resolves #867

Description

Invalid discriminator(#0000) shown for users with new username system in uploaded gist by TJ-Bot.

@doshgo doshgo requested review from a team as code owners January 11, 2024 01:44
@CLAassistant
Copy link

CLAassistant commented Jan 11, 2024

CLA assistant check
All committers have signed the CLA.

@ankitsmt211
Copy link
Member

ankitsmt211 commented Jan 11, 2024

Hi, can you also add a screenshot in description after changes? thanks
usually it's a good idea to add something visual, but it's just one line so it's alright.

.createGist()
.public_(false)
.description("Uploaded by " + event.getAuthor().getAsTag());
.description("Uploaded by @ " + event.getAuthor().getAsTag().replace("#0000", " "));
Copy link
Member

Choose a reason for hiding this comment

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

is there any reason not to use event.getAuthor().getName() here, you wouldn't need to replace anything.

There's still room for improvement, imo if we can retrieve user as Member, we can get the EffectiveName of user(nickname of user or username if it's null). Although i don't think event.getMember works here, would need to retrieve user seperately what you think @Zabuzard ?

Copy link
Member

Choose a reason for hiding this comment

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

yes, the correct fix is to not use tags as source for this. tags are not a thing anymore and names are now unique. so we should instead put the name.

Copy link
Member

@Zabuzard Zabuzard Jan 11, 2024

Choose a reason for hiding this comment

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

we shouldnt put the effective name though, cause we need to be able to find users uploading malicious content even after renaming or kicked. the account name is enough (or effective name + id)

@doshgo
Copy link
Author

doshgo commented Jan 12, 2024

Here's an image of what the change will look like.
There is an additional space between the @ and though I am not sure its necessary.
What do you guys think?
Screen-Shot-2024-01-11-at-7-20-39-PM

@ankitsmt211
Copy link
Member

Here's an image of what the change will look like. There is an additional space between the @ and though I am not sure its necessary. What do you guys think? Screen-Shot-2024-01-11-at-7-20-39-PM

is it after you used getName? you don't need to use @ now.
It should look like Uploaded by idkwhatimdoin

@doshgo
Copy link
Author

doshgo commented Jan 16, 2024

@ankitsmt211 The file below is using :
description("Uploaded by " + event.getAuthor().getName());

https://github.com/Together-Java/TJ-Bot/assets/34873349/3475bed6-87ea-4f5c-a027-cc949f429497)

What else needed to be improved?

@ankitsmt211
Copy link
Member

@ankitsmt211 The file below is using : description("Uploaded by " + event.getAuthor().getName());

https://github.com/Together-Java/TJ-Bot/assets/34873349/3475bed6-87ea-4f5c-a027-cc949f429497)

What else needed to be improved?

seems alright, you can push the changes.

@ankitsmt211
Copy link
Member

@doshgo are you still working on this? any update, it's been a month old now.

@vishv843
Copy link
Contributor

Hi, I wanna work on this. Can you assign the issue to me?

@vishv843
Copy link
Contributor

image
This is an image showing the change.

@vishv843
Copy link
Contributor

Please check out my PR #1050

@ankitsmt211
Copy link
Member

#1050 already merged which targets the same issue, closing this one now.

@doshgo doshgo deleted the Feature-/-Modified-discriminator-for-username branch May 16, 2024 11:43
@doshgo doshgo restored the Feature-/-Modified-discriminator-for-username branch May 16, 2024 11:43
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.

Wrong discriminator shown for users with new username system
5 participants