Skip to content

Sort invite roles list by application display name - #357

Open
kayjoosten wants to merge 3 commits into
mainfrom
343-sort-invite-roles
Open

kayjoosten wants to merge 3 commits into
mainfrom
343-sort-invite-roles

Conversation

@kayjoosten

Copy link
Copy Markdown
Contributor

Summary

The list of invite roles shown under /invite-roles was not sorted, so every visit could show the roles in a different, unpredictable order.

Changes

  • Added InviteRoleList::sortByApplicationDisplayName(string $locale), which sorts roles alphabetically (case insensitively) by their application display name, falling back to the role name for roles without applications. This mirrors the sorting approach already used for /my-services.
  • Updated InviteRolesController to sort the list using the current locale before rendering.
  • Added unit tests covering sorting, case insensitivity, and roles without applications.

Testing

Ran the full QA suite locally (composer check): composer-validate, phplint, phpmd, phpcs, license-headers, phpunit (147 tests, 281 assertions), phpstan. All green.

Closes #343

# If applied, this commit will...
Sort the list of roles shown on the invite-roles page by application
display name.

# Why is this change needed?
Prior to this change, the list of invite roles shown under
/invite-roles was not sorted. Every request could return the roles in
a different order, making the menu feel unpredictable and hard to
scan.

# How does it address the issue?
This change adds a sortByApplicationDisplayName method to
InviteRoleList that orders roles alphabetically and case
insensitively by their application display name, falling back to the
role name when a role has no applications. The InviteRolesController
now sorts the list using the current locale before rendering it,
matching the approach already used for the my-services overview.

# Provide links to any relevant tickets, articles or other resources
Closes #343
* Sorts the roles alphabetically (case-insensitively) by the display name of their
* first application, falling back to the role name for roles without applications.
*/
public function sortByApplicationDisplayName(

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Consideration: This currently changes the value of the value object.
I think it would be nicer if the initializeWith cals this function. That has two advantages: The value object state does not need to be changed anymore (one could argue that if the state needs to be changed it is not valid in the first place). And 2, the sortByApplicationDisplayName can become a pure function.

@johanib johanib left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

If point below is fixed, please merge.

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.

List of roles is not sorted

2 participants