fix: address pr feedback

RetroAchievements · Jan 22, 2025 · 479f9c9 · 479f9c9
1 parent 36aca35
commit 479f9c9
Show file tree

Hide file tree

Showing 4 changed files with 57 additions and 10 deletions.
diff --git a/app/Community/Concerns/ActsAsCommunityMember.php b/app/Community/Concerns/ActsAsCommunityMember.php
@@ -16,6 +16,7 @@
 use App\Models\UserComment;
 use App\Models\UserGameListEntry;
 use App\Models\UserRelation;
+use Illuminate\Database\Eloquent\Collection;
 use Illuminate\Database\Eloquent\Relations\BelongsToMany;
 use Illuminate\Database\Eloquent\Relations\HasMany;
 use Illuminate\Support\Facades\Auth;
@@ -32,16 +33,19 @@ public static function bootActsAsCommunityMember(): void
     public function getVisibleRoleAttribute(): ?SpatieRole
     {
         // Load the user's displayable roles.
-        if ($this->relationLoaded('displayableRoles')) {
-            $displayableRoles = $this->displayableRoles;
+        if ($this->relationLoaded('roles')) {
+            /** @var Collection<int, SpatieRole> $displayableRoles */
+            $displayableRoles = $this->roles->where('display', '>', 0);
         } else {
-            $displayableRoles = $this->displayableRoles()->orderBy('display')->get();
+            /** @var Collection<int, SpatieRole> $displayableRoles */
+            $displayableRoles = $this->displayableRoles()->get();
         }
 
         // If user has an explicitly set visible_role_id, we'll try to show it.
         // However, we need to verify it's still a valid displayable role for the
         // user (it's possible they lost the role at some point).
         if ($this->visible_role_id !== null) {
+            /** @var SpatieRole|null $explicitRole */
             $explicitRole = $displayableRoles->find($this->visible_role_id);
             if ($explicitRole) {
                 return $explicitRole;
@@ -50,6 +54,7 @@ public function getVisibleRoleAttribute(): ?SpatieRole
 
         // Otherwise, fall back to highest ordered displayable role.
         // For most users, this will return null.
+        /** @var SpatieRole|null */
         return $displayableRoles->first();
     }
 

diff --git a/app/Community/Controllers/UserSettingsController.php b/app/Community/Controllers/UserSettingsController.php
@@ -42,6 +42,10 @@ public function show(): InertiaResponse
         /** @var User $user */
         $user = Auth::user();
 
+        $user->load(['roles' => function ($query) {
+            $query->where('display', '>', 0);
+        }]);
+
         $userSettings = UserData::fromUser($user)->include(
             'apiKey',
             'deleteRequested',
@@ -58,7 +62,7 @@ public function show(): InertiaResponse
         );
 
         /** @var Collection<int, Role> $displayableRoles */
-        $displayableRoles = $user->displayableRoles()->orderBy('display')->orderBy('name')->get();
+        $displayableRoles = $user->roles;
 
         $mappedRoles = $displayableRoles->map(fn ($role) => RoleData::fromRole($role))
             ->values()

diff --git a/resources/js/features/settings/components/ProfileSectionCard/ProfileSectionCard.test.tsx b/resources/js/features/settings/components/ProfileSectionCard/ProfileSectionCard.test.tsx
@@ -231,4 +231,38 @@ describe('Component: ProfileSectionCard', () => {
       visibleRoleId: 2,
     });
   });
+
+  it('given multiple visible roles are available, displays them sorted alphabetically by translated name', async () => {
+    // ARRANGE
+    const displayableRoles: App.Data.Role[] = [
+      { id: 1, name: 'zdev' },
+      { id: 2, name: 'adev' },
+      { id: 3, name: 'mdev' },
+    ];
+
+    render<App.Community.Data.UserSettingsPageProps>(<ProfileSectionCard />, {
+      pageProps: {
+        displayableRoles,
+        auth: {
+          user: createAuthenticatedUser({
+            visibleRole: displayableRoles[0],
+          }),
+        },
+        can: {
+          updateMotto: true,
+        },
+        userSettings: createUser(),
+      },
+    });
+
+    // ACT
+    await userEvent.click(screen.getByRole('combobox', { name: /role/i }));
+
+    // ASSERT
+    const optionEls = screen.getAllByRole('option');
+
+    expect(optionEls[0]).toHaveTextContent(/adev/i);
+    expect(optionEls[1]).toHaveTextContent(/mdev/i);
+    expect(optionEls[2]).toHaveTextContent(/zdev/i);
+  });
 });
diff --git a/resources/js/features/settings/components/ProfileSectionCard/VisibleRoleField.tsx b/resources/js/features/settings/components/ProfileSectionCard/VisibleRoleField.tsx
@@ -46,6 +46,13 @@ export const VisibleRoleField: FC = () => {
     );
   }
 
+  const sortedDisplayableRoles = displayableRoles.sort((a, b) => {
+    const aName = t(a.name as TranslationKey);
+    const bName = t(b.name as TranslationKey);
+
+    return aName.localeCompare(bName);
+  });
+
   return (
     <BaseFormField
       control={form.control}
@@ -64,12 +71,9 @@ export const VisibleRoleField: FC = () => {
                 </BaseSelectTrigger>
 
                 <BaseSelectContent>
-                  {displayableRoles.map((displayableRole) => (
-                    <BaseSelectItem
-                      key={`role-option-${displayableRole.id}`}
-                      value={String(displayableRole.id)}
-                    >
-                      {t(displayableRole.name as TranslationKey)}
+                  {sortedDisplayableRoles.map((role) => (
+                    <BaseSelectItem key={`role-option-${role.id}`} value={String(role.id)}>
+                      {t(role.name as TranslationKey)}
                     </BaseSelectItem>
                   ))}
                 </BaseSelectContent>