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: allow users to change their names #3071

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

Conversation

wescopeland
Copy link
Member

This PR gives users the ability to request name changes.

How it works

A new form has been added to /settings:
Screenshot 2025-01-17 at 10 01 51 PM

When the user attempts to submit the form, they get a confirm with this warning:
Screenshot 2025-01-17 at 10 02 37 PM

If the user confirms, the request is submitted and lives in the user_usernames table:
Screenshot 2025-01-17 at 10 03 10 PM

Screenshot 2025-01-17 at 10 03 35 PM
(note the new approved_at column)

In the management app, a new tool has been added:
Screenshot 2025-01-17 at 10 03 55 PM

The (1) badge indicates there is a pending username change request.

As an admin or moderator, I can view the list of pending username change requests:
Screenshot 2025-01-17 at 10 04 25 PM

If I click approve, the user's display_name value will be changed, something will be recorded to their account's audit log, and an email will be fired off:
Screenshot 2025-01-17 at 10 05 08 PM

Screenshot 2025-01-17 at 10 05 26 PM

This is the happy path case.

Alternatively, after 30 days, the username change request will "expire". It will no longer appear in the list of active requests, and the user will be able to request a new username change.

Other notes:

  • After setting a display_name, users do have the ability to request reverting back to their original username (they can bypass this uniqueness check).
  • Muted users cannot change their display_name.
  • After a display_name change is approved, users are put on a 30 day cooldown before they can request another change.

@wescopeland wescopeland requested a review from a team January 18, 2025 03:09
app/Filament/Resources/UserUsernameResource.php Outdated Show resolved Hide resolved
public/request/auth/register.php Show resolved Hide resolved
->requiresConfirmation()
->modalDescription("Are you sure you'd like to do this? The username change will go into effect immediately.")
->color('success')
->icon('heroicon-o-check'),
Copy link
Member

Choose a reason for hiding this comment

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

There doesn't seem to be any way to "Disapprove" a request. If the user wants to be something inappropriate, do we just ignore it for 30 days?

Copy link
Member

Choose a reason for hiding this comment

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

Deleting the request (which is only available through the bulk actions menu) immediately lets the user try a different username, despite the warning they get when requesting one:
image

Copy link
Member Author

@wescopeland wescopeland Jan 19, 2025

Choose a reason for hiding this comment

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

denied_at column added in latest. When a request is denied, the user is put on a 30 day cooldown.

I've removed the bulk action in latest - I suspect bulk approve/deny is undesirable given whoever is doing approvals will need to check if Discord aliases need to be updated.

Copy link
Member

Choose a reason for hiding this comment

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

Should we also email the user if their request is denied?

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 not sure. I see both pros and cons of sending a decline notification.

On one hand, it would be nice to notify them so they know not to submit the same request again.
On the other hand, I'm worried it might foment resentment from the user towards staff, especially given they have to wait potentially weeks to try again.

Copy link
Member

Choose a reason for hiding this comment

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

They'd have to wait weeks anyway. At least this way they know we aren't just ignoring them.

Copy link
Member Author

Choose a reason for hiding this comment

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

Added a decline email in latest.

@@ -366,6 +366,9 @@ protected function mapWebRoutes(): void
Route::put('password', [UserSettingsController::class, 'updatePassword'])->name('api.settings.password.update');
Route::put('email', [UserSettingsController::class, 'updateEmail'])->name('api.settings.email.update');

Route::post('username-change-request', [UserSettingsController::class, 'storeUsernameChangeRequest'])
->name('api.settings.username-change-request.store');
Copy link
Member

Choose a reason for hiding this comment

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

Validation errors aren't reported correctly.
image

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 having trouble reproducing this:

Screenshot 2025-01-19 at 11 06 13 AM

I did make one minor change to client-side validation in latest (users cannot request a change to the name they are currently set to). But, even before this change, I was seeing server-side validation messages appear in the toast.

Copy link
Member

Choose a reason for hiding this comment

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

Try with invalid characters. I think I was using "It's-a-me, Mario" as my 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.

That worked, thanks.

I've made an update in latest, but it's client-side. I think for these particular requests, it may be best to catch them before they get sent to the server:

Screenshot 2025-01-19 at 12 04 51 PM

Copy link
Member

Choose a reason for hiding this comment

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

Suggest changing message to match sign up.
image

vs
image

Copy link
Member Author

Choose a reason for hiding this comment

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

Updated these client-side validation messages in latest.

app/Policies/UserUsernamePolicy.php Outdated Show resolved Hide resolved
default => $query,
};
})
->default('pending'),
Copy link
Member

@Jamiras Jamiras Jan 19, 2025

Choose a reason for hiding this comment

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

It seems unlikely, but if a request is not addressed within 30 days, it falls off the default filter and the user is allowed to make new requests. The existing request is still present and can be accessed (and approved/denied) by removing this filter.

To test: update the created_at of a pending request to be more than 30 days ago.

Copy link
Member Author

Choose a reason for hiding this comment

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

In latest, the table will only show non-expired rows.

->requiresConfirmation()
->modalDescription("Are you sure you'd like to do this? The username change will go into effect immediately.")
->color('success')
->icon('heroicon-o-check'),
Copy link
Member

Choose a reason for hiding this comment

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

Should we also email the user if their request is denied?

@@ -366,6 +366,9 @@ protected function mapWebRoutes(): void
Route::put('password', [UserSettingsController::class, 'updatePassword'])->name('api.settings.password.update');
Route::put('email', [UserSettingsController::class, 'updateEmail'])->name('api.settings.email.update');

Route::post('username-change-request', [UserSettingsController::class, 'storeUsernameChangeRequest'])
->name('api.settings.username-change-request.store');
Copy link
Member

Choose a reason for hiding this comment

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

Suggest changing message to match sign up.
image

vs
image

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.

2 participants