feat: Adds support for filtering and sorting by loginUsername with us…#51
feat: Adds support for filtering and sorting by loginUsername with us…#51
Conversation
Coverage Report
|
| for (String prop : property) { | ||
| Object value = PropertyAccessorFactory.forBeanPropertyAccess(obj).getPropertyValue(prop); | ||
| if (value == null) continue; | ||
| if (isNegation) return filter(value, filter); |
There was a problem hiding this comment.
Is this for the case when loginUsername value does not match but userId does i.e. to support filtering on userId even when loginUsername exists? The customer only sees the loginUsername on the web client if it is present, so we shouldn't need to care.
There was a problem hiding this comment.
This code is not correct. userId should only be checked if loginUsername is null. I've updated this part in revision.
| Object propertyValue1; | ||
| Object propertyValue2; | ||
|
|
||
| if ("userId".equalsIgnoreCase(sortToken.getKey())) { |
There was a problem hiding this comment.
Ok yeah we don't need that we can just match it as it expects as "UserId"
| Object propertyValue1; | ||
| Object propertyValue2; | ||
|
|
||
| if ("userId".equalsIgnoreCase(sortToken.getKey())) { |
There was a problem hiding this comment.
Let's declare the string constants at the top of the class
| useEffect(() => { | ||
| const fetchAllUsers = async () => { | ||
| try { | ||
| const response = await dataAccessService.describeUsers({} as DescribeUsersRequestData) |
There was a problem hiding this comment.
We're retrieving all users? There are two issues with this:
- We don't need all users, we need mapping only for the CreatedBy and LastModified userIds on the current page of SessionTemplates table
- We need to paginate through the response to get all the values
We can add this method here
There was a problem hiding this comment.
In fact, we can call the method as part of the useSessionTemplatesService here instead of using a useEffect
There was a problem hiding this comment.
I was getting all users because UsersSharedWith could possibly be any user.
If we only get the users on the current page, the filter would not be right? For example, if all the templates created by user1 are on page 1 and templates by user3 are on page 2 and I want to filter templates created by user3. If only users are fetched that are on the corresponding page we would not be able to filter by user3?
Maybe we could get all users corresponding to CreatedBy, LastModified, and UsersSharedWith and then create the userId -> loginUsername map with just this subset of users instead of all.
There was a problem hiding this comment.
Ok, updated to fetch only users from the current page's CreatedBy/LastModifiedBy fields instead of all users. This builds a map of userId→loginUsername for:
- Displaying loginUsername in table columns (instead of userId)
- Populating the filter dropdown with relevant users
- Showing loginUsername in filter chips (instead of userId)
| } | ||
|
|
||
| const fetchFilteringOptions = async (filteringText: string, filteringProperty: string) => { | ||
| const labelMap = props.propertyValueToLabelMaps?.get(filteringProperty) |
There was a problem hiding this comment.
Wow what's going on here? Could you please help me understand?
There was a problem hiding this comment.
propertyValueToLabelMaps is passed to FilterBar as a prop from SessionTemplatesTable. This map maps the 3 filtering properties CreatedBy, LastModifiedBy and UsersSharedWith to userIdToLoginUsernameMap which maps userIds to loginUsernames for all the users. This map is created when fetching all users.
So when CreatedBy, LastModifiedBy or UsersSharedWith is selected as the filtering property the map is retrieved and the loop takes the users in the map and makes them dropdown options where value is the userId which is sent to the backend when selected and label is the loginUsername shown in the dropdown.
a758a16 to
1b51244
Compare
Coverage Report
|
f2ec97c to
e14e3c0
Compare
Coverage Report
|
Coverage Report
|
| @@ -2029,7 +2029,12 @@ components: | |||
| description: "The entity that represents the data that the user passes for describing the users" | |||
| properties: | |||
| UserIds: | |||
There was a problem hiding this comment.
Should we simplify the filter names? UserIds for internal userIds, LoginUsernames for loginUsernames
…UserIds to LoginUsernames
Coverage Report
|
Coverage Report
|
| return (await this.getUsersApi()).describeUsers(describeUsersRequest) | ||
| } | ||
|
|
||
| public async describeUsersByIds(userIds: string[]): Promise<Map<string, string>> { |
There was a problem hiding this comment.
nit: name the method more appropriately? This returns the map for provided userIds right?
| sessionTemplateId={existingSessionTemplateId!} | ||
| handleUsersChange={(users: [OptionDefinition]) => { | ||
| let userIds: [string] = [] | ||
| let labels: string[] = [] |
There was a problem hiding this comment.
Do we still need userIds[]?
| import {SESSION_TEMPLATES_TABLE_CONSTANTS} from "@/constants/session-templates-table-constants"; | ||
|
|
||
| export default function SessionTemplateOverview({sessionTemplate}: { sessionTemplate: SessionTemplate | undefined }) { | ||
| type Props = { |
There was a problem hiding this comment.
nit: More appropriate naming, follow the pattern for other components and their props
| export default function SessionTemplateOverview({sessionTemplate}: { sessionTemplate: SessionTemplate | undefined }) { | ||
| type Props = { | ||
| sessionTemplate: SessionTemplate | undefined | ||
| userDisplayNames?: Map<string, string> |
There was a problem hiding this comment.
nit: These are loginUsernames right?
mitshiv1905
left a comment
There was a problem hiding this comment.
As discussed offline, let's try to ensure that the number of describeUsers() calls are minimized for SessionTemplate display purposes
|
|
||
| DescribeUsersRequestData userRequest = new DescribeUsersRequestData(); | ||
| userRequest.setLoginUsernames(List.of(new FilterToken().operator(lookupOp).value(filter.getValue()))); | ||
| List<User> users = userService.describeUsers(userRequest).getUsers(); |
There was a problem hiding this comment.
A describeUsers() call per filter is too expensive. Can we group the calls by EQUAL, CONTAINS, NOT_CONTAINS? That way we will only need 3 calls at most.
Coverage Report
|
Coverage Report
|
mitshiv1905
left a comment
There was a problem hiding this comment.
In the interest of time, decided to leave the filtering code in DescribeSessionTemplatesController which should ideally be in a separate component or the SessionTemplatesService.
…erId fallback
Issue #, if available:
Description of changes:
Added support for filtering and sorting users by loginUsername with fallback to userId when loginUsername is null
Backend changes:
Core filtering and sorting logic:
Session template filtering:
API model updates:
Frontend changes:
Session template filtering and display:
Other
Why is this change necessary:
Users are more easily identified by their loginUsername than by userId. Previously, the UI displayed loginUsername in tables, but filtering and sorting still operated on userId, creating a mismatch between what users see and what they can filter/sort by. This change aligns the filtering and sorting behavior with the displayed values, improving usability while maintaining backward compatibility with users who only have userId populated.
How was this change tested:
Manual Test Cases - Filter and sorting changes
Users Table
Users Table - Filter by User ID (shows loginUsername)
Users Table - Sort User ID column
Session Templates Table
Session Templates Table - Details panel
Session Templates Table - Filter by Created By
Session Templates Table - Filter by Last Modified By
Session Templates Table - Filter by Users Shared With
Create template and assign user
Assign Session Templates to Users
Add Users to User Group
Remove Users from User Group
User with no loginUsername
User
no_login_namedoes not have a loginUsername and checked that it was sorted correctly and filterableEmpty filter results
Multiple filters combined
Also tested filtering and sorting for internal auth setup
Any additional information or context required to review the change:
Documentation Checklist:
Compatibility Checklist:
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.