Skip to content

Comments

fix: use case insensitive sorting for name of users, labs and orgs#267

Merged
PurcyTwoBulls merged 4 commits intomainfrom
fix/EG-591_table_sort_order
Aug 28, 2024
Merged

fix: use case insensitive sorting for name of users, labs and orgs#267
PurcyTwoBulls merged 4 commits intomainfrom
fix/EG-591_table_sort_order

Conversation

@PurcyTwoBulls
Copy link
Contributor

Overrides the sort function to apply a case insensitive sort (with a case sensitive sort for consistent results if they match).

This is applied to all sortable text fields in tables.

@PurcyTwoBulls PurcyTwoBulls requested a review from simondib August 22, 2024 18:40
Comment on lines 4 to 19
export function caseInsensitiveSortFn(a: any, b: any, direction: 'asc' | 'desc') {
if (a.toLowerCase() === b.toLowerCase()) {
// If idenical ignoring case, sort by case for consistency of results
if (direction === 'asc') {
return a < b ? -1 : 1;
} else {
return a > b ? -1 : 1;
}
return 0;
}
if (direction === 'asc') {
return a.toLowerCase() < b.toLowerCase() ? -1 : 1;
} else {
return a.toLowerCase() > b.toLowerCase() ? -1 : 1;
}
}
Copy link
Contributor

@zigtan zigtan Aug 23, 2024

Choose a reason for hiding this comment

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

Thanks @PurcyTwoBulls for working on this fix. This function needs some improvement.

Organization Names, Laboratory Names, and also User Emails are enforced to be unique within their respective DynamoDB tables with the use of the supporting unique-reference-table DynamoDB table.

Have a look through the unique-reference-table DynamoDB table records to see the unique references - you will see that Laboratory Names are unique within their parent Organization.

So, if there is an existing Laboratory named 'ABC' the backend logic will never allow a new Laboratory named 'abc' to be created - as the lower-case uniqueness record already exists in the unique-reference-table.

As a result, this function can be simplified to:

export function caseInsensitiveSortFn(a: string, b: string, direction: 'asc' | 'desc'): number {
  const orderAB = a.toLowerCase() < b.toLowerCase() ? -1 : 1;
  return direction === 'desc' ? orderAB * -1 : orderAB; // If desc, invert orderAB
}

Also we should not ever need to use the any type, as we are using Typescript instead of Javascript so we should be explicitly defining the types.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks @zigtan

For users, emails have uniqueness but the users first and last name and combinations do not. If we want to make the decision that we are not tackling that edge case then we don't need to, but given it is user entry it is probable edge case.

I have updated the function signature.

Copy link
Contributor

@zigtan zigtan Aug 26, 2024

Choose a reason for hiding this comment

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

@PurcyTwoBulls , a User's names (FirstName, LastName) do not have to be unique in the system.

For all User's their FirstName, LastName are just their personal details.

Internally we reference all User's by their primary UUID.

The User's Email is used as a Global Secondary Index (GSI) for efficient lookups in DynamoDB.

The rule to ensure each User's Email is unique is a logical security design and also support the multi-tenant architecture where a User can invited to one or more Organizations (using the same email).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I understand that, but what is being used here for displaying user is their display name or their email address, so we are using First and Last name in these sorted fields.

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks @PurcyTwoBulls for highlighting the First Name and Last Name sorting issue you are trying to tackle.

I think this is a deeper problem seeming from our UI design.

We will need to circle back to improve the UI design to separately list Users' Email, and Full Name in separate columns and have each be sortable. The current design of showing a single column for Email or Full Name complicates the sorting behaviour and solution.

Copy link
Contributor

@zigtan zigtan left a comment

Choose a reason for hiding this comment

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

Thanks @PurcyTwoBulls . Just need to update the FE sort-utils caseInsensitiveSortFn() logic.

@zigtan zigtan self-requested a review August 27, 2024 01:25
@PurcyTwoBulls PurcyTwoBulls merged commit a0fac08 into main Aug 28, 2024
@zigtan zigtan deleted the fix/EG-591_table_sort_order branch September 4, 2024 11:34
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