-
-
Notifications
You must be signed in to change notification settings - Fork 100
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(user): add display name history to profiles for privileged roles #3144
base: master
Are you sure you want to change the base?
feat(user): add display name history to profiles for privileged roles #3144
Conversation
->orderBy('approved_at', 'desc') | ||
->get() | ||
->map(function($request) { | ||
return $request->username . ' (' . $request->approved_at->format('F j, Y') . ')'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It feels like the dates should be shifted one row back.
When viewing a user who has only had one approved change, the tooltip shows their original username and the date they joined. There's no way to see which date the username was changed on.
Similarly, after a second change, the rows are "previous name (when it was approved)" and "original name (date joined)".
What I really want to see is "previous name (when it was changed to current name)" and "original name (when it was changed to previous name)".
However, that doesn't read well. You could have "previous name (until [date])" and "original name (until [date])", but it might be easier to just include the current name in the list: "current name (approval date)", "previous name (approval date)", "original name (creation date)".
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think I have this squared away now in latest, but let me know. It's possible I've misinterpreted the feedback.
To make things a little more manageable, I've also extracted the logic responsible for building the tooltip into an action.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Almost. With the current implementation I'm seeing:
* CurrentUsername (approved_at)
* PreviousUsername (approved_at)
* OriginalUsername (PreviousUsername approved_at)
OriginalUsername should show account creation date.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm starting to wonder if this should just be another tab on the filament page for a user.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could be, one big challenge there is we'd need to grant several roles the ability to "manage" users in order to open the resource up. I'm not sure what the implications are of doing this, which is why I've held off on it up to now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I still lean towards not opening up the user resource in Filament at this time.
Though, I think it would be reasonable to have some new kind of tool which allows users to search for a display name and find all associated accounts.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OriginalUsername should show account creation date.
Addressed in latest.
Role::EVENT_MANAGER, | ||
Role::NEWS_MANAGER, | ||
Role::ENGINEER, | ||
Role::GAME_EDITOR, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can see why Administrator, Moderator, and Event Manager are in this list, but why would the other roles need that information? And why not Dev Compliance or Quality Assurance?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
News manager and engineer are probably good to remove.
Do you think it may be desirable for Developer in case they need to hunt down a user for credit? (same question for Game Editor)
For DevC/QAM, them being a developer is probably implied. Though if all devs don't need it, maybe we can only give access to those roles. Now that I think of it, CR may want it too in case they need to see a username history trail for someone who was previously a Junior Developer.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you think it may be desirable for Developer in case they need to hunt down a user for credit? (same question for Game Editor)
Developers can't award manual unlocks (at this time). And even if they did, we'd probably restrict it to doing so from a ticket where they'd only be able to do it for the user and achievement associated to the ticket.
The main reason this needs to be exposed outside of moderation is for tracking data externally. We know Event Manager has a valid use case (though that could just be handled by exposing the ULID).
Now that I think of it, CR may want it too in case they need to see a username history trail for someone who was previously a Junior Developer.
Once they're no longer a Junior Developer, does CR even care about them? They become DevC/QAM's problem at that point. And that might be a valid case to see the username history. Although, at that point, maybe we should just expose it to everyone. If SomeBadDeveloper changes their username to hide the stigma they've collected, shouldn't anyone familiar with SomeBadDeveloper's unstable code be able to quickly tell that some new set was created by the same user and have similar issues? I guess the user would just report the issues against the new username and DevC/QAM could deal with the history.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Another role that should have access to this is Cheat Investigator. Though, they probably need something a bit more sophisticated. If a report comes in for "PreviousUsername", and they've already changed their display name to "CurrentUsername", they'll need somewhere to find which account(s) were associated to "PreviousUsername". That functionality can probably be rolled into the existing admin tools.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In latest, I've set roles to:
return $user->hasAnyRole([
Role::ADMINISTRATOR,
Role::MODERATOR,
Role::DEV_COMPLIANCE,
Role::QUALITY_ASSURANCE,
Role::EVENT_MANAGER,
Role::GAME_EDITOR,
Role::CHEAT_INVESTIGATOR,
]);
I agree that cheat investigator needs something more sophisticated. It should probably be a new tool that lives in Filament.
https://discord.com/channels/310192285306454017/1017568867574362283/1333436729247399936
If a user is an admin, moderator, developer, event manager, news manager, engineer, or game editor, they can see a user's display name history on their user profile:
The underline visual indicator is intentionally very subtle and appears on desktop only.
This is currently implemented using a simple
title
attribute. When profiles are migrated to React, this can use a fancier popover component.