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

feat: use display_name everywhere #3017

Open
wants to merge 19 commits into
base: master
Choose a base branch
from

Conversation

wescopeland
Copy link
Member

@wescopeland wescopeland commented Jan 4, 2025

Pre-deploy SQL:

UPDATE UserAccounts 
SET display_name = User 
WHERE display_name IS NULL;

This PR is a precursor to username change functionality. A subsequent PR will add a new panel to the Settings page for users to request a username change, and a new tool in Filament for moderators to approve username changes.

This PR makes the display_name field a first-class citizen on the site, having everywhere a user's name is displayed prefer it over User if a user's display_name value is non-empty.

I have tested:

  • Every legacy PHP page.
  • Every Blade UI page.
  • Every React page.
  • Every endpoint of the web API.
  • The Connect API.
  • Filament.
  • All authentication flows (sign in using display_name instead of username, reset password, etc.).
  • Email notifications.

Even though my manual testing has been extremely exhaustive, I have no doubt there are things here and there I may have missed. However, with the volume of users who are asking for username changes, I'd like to get this functionality done sooner rather than later.

Implementation notes:

  • Pretend my display_name is "wes". This implementation allows you navigate to /user/WCopeland and /user/wes. This does not currently have a self-healing redirect (I don't think this is desirable until the respective pages are converted to React). In other words, {user}'s resolution logic is $user->display_name ?? $user->User.
  • There is a large assumption baked in to this implementation that all usernames and display names are unique. In other words, no one can have a display name that matches an existing username, and vice versa. In the subsequent PR, we could even enforce this at the database level via a migration.

How to test this PR:
Set your display_name value to anything that would be a valid username (min 3 chars, max 20 chars, no special chars). Perform any action anywhere on the site as you normally would.

Screenshot 2025-01-03 at 10 32 39 PM

Screenshot 2025-01-03 at 10 32 48 PM

Screenshot 2025-01-03 at 10 32 51 PM

Screenshot 2025-01-03 at 10 32 57 PM

Screenshot 2025-01-03 at 10 33 03 PM


Next steps:

  • Write a migration to make UserAccounts.display_name non-nullable.

@wescopeland wescopeland requested a review from a team January 4, 2025 03:37
Copy link
Member

@Jamiras Jamiras left a comment

Choose a reason for hiding this comment

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

Going to stop here for today. Have not tested public or connect APIs yet.

resources/views/pages-legacy/userInfo.blade.php Outdated Show resolved Hide resolved
resources/views/pages-legacy/userInfo.blade.php Outdated Show resolved Hide resolved
resources/views/pages/user/[user]/tickets/index.blade.php Outdated Show resolved Hide resolved
resources/views/pages/user/[user]/developer/sets.blade.php Outdated Show resolved Hide resolved
app/Filament/Resources/UserResource.php Outdated Show resolved Hide resolved
public/request/forum-topic-comment/create.php Outdated Show resolved Hide resolved
app/Community/Components/UserProfileMeta.php Show resolved Hide resolved
app/Helpers/database/user-relationship.php Outdated Show resolved Hide resolved
app/Helpers/database/search.php Outdated Show resolved Hide resolved
app/Helpers/database/player-game.php Show resolved Hide resolved
@wescopeland wescopeland added the deployment/sql Includes SQL that needs to be run before/after deployment label Jan 6, 2025
resources/views/pages-legacy/achievementList.blade.php Outdated Show resolved Hide resolved
app/Models/User.php Outdated Show resolved Hide resolved
resources/views/pages-legacy/userInfo.blade.php Outdated Show resolved Hide resolved
Copy link
Member

Choose a reason for hiding this comment

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

achievementwondata, getfriendlist, lbinfo should be modified to return DisplayName in addition to User.

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've made these changes in latest - please give them an extra look. While I've tested these changes, I'm not 100% confident in them. Just want to be extra sure we're okay surfacing the username in all of these.

I've made a change in user-auth.php such that u now accepts both User and display_name.

achievementwondata, getfriendlist, and lbinfo have all been modified to return DisplayName values, ie:

		"Entries": [
			{
				"User": "Bkid",
				"DisplayName": "Bkid",

Copy link
Member

Choose a reason for hiding this comment

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

I've made a change in user-auth.php such that u now accepts both User and display_name.

I think we want the user to always log in with their username, don't we? The problem I reported earlier was that since DisplayName was being returned by the login API, future calls passed it as the user's Username.

Did we change the web login to support display name?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yep, logging in on the site via display_name should already work on this branch.

I figured once a user sets a display_name it would probably be very counterintuitive for them to log in with their original username. For the Connect API it's probably less of a concern past the initial emulator login call, so I'd be happy to revert that if you think we should.

Copy link
Member

Choose a reason for hiding this comment

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

If they can log in to the site using their display name, they'll expect to be able to do so from the emulator too. I was under the impression that the User name was still part of their credentials, and the display name was just something that should be public-facing. Hence, my concerns about still exposing the Username through the avatar images on the webpages.

rcheevos uses the User value returned from the login call as the u parameter for all future calls, so it seems unlikely that the display name would be passed to any of them unless there's some client not using and not mimicking rcheevos.

I guess it doesn't hurt to check both.

Copy link
Member Author

@wescopeland wescopeland Jan 11, 2025

Choose a reason for hiding this comment

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

Implemented number 3 in latest. To reiterate, this means:

  • User is overloaded with the user's display_name in login2, achievementwondata, getfriendlist, and lbinfo.
  • AvatarUrl has been added to all four of those.

I also see we have codenotes2. This is currently returning User and DisplayName - I assume this is used by the toolkit only - not sure if this should be changed or left as-is.

Copy link
Member

Choose a reason for hiding this comment

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

I also see we have codenotes2. This is currently returning User and DisplayName - I assume this is used by the toolkit only - not sure if this should be changed or left as-is.

rcheevos is only processing the User value, and returning it as author. The only thing the toolkit uses is for is the warning that you're overwriting someone else's note. So, I think it's reasonable to return display name there too.

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've verified this is happening in latest:

{
	"User": "WCopeland",
	"DisplayName": "wesweswes",
	"Address": "0x0001a0",
	"Note": "[8-bit] Total Lives Remaining\r\n\r\n02 = Two lives remaining\r\n00 = Player is on last life (not game over)\r\nFD = Game Over \/ Password screen\r\n\r\nSymbol: lives"
},

Copy link
Member

Choose a reason for hiding this comment

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

Sorry, I meant "I think it's reasonable to return display name [in the User field] there too".

Then all the APIs will be overloading User with the display name, and provide an AvatarUrl where we expect the client might want to display one.

Copy link
Member Author

Choose a reason for hiding this comment

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

Ah, I understand now. In latest, I've changed the query to do SELECT ua.display_name AS User, ie:
Screenshot 2025-01-16 at 8 56 38 PM

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
deployment/sql Includes SQL that needs to be run before/after deployment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants