-
Notifications
You must be signed in to change notification settings - Fork 3k
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
[HOLD for payment 2024-04-09] [$500] [MEDIUM] Tag - The list of tags consisting of numbers is not ordered in ascending order. #33650
Comments
Job added to Upwork: https://www.upwork.com/jobs/~01d24e74dadb891171 |
Triggered auto assignment to @sonialiap ( |
Bug0 Triage Checklist (Main S/O)
|
Triggered auto assignment to Contributor-plus team member for initial proposal review - @situchan ( |
ProposalPlease re-state the problem that we are trying to solve in this issue.Numbers sorted in alphabetical order as plain text in tags. What is the root cause of that problem?App/src/libs/OptionsListUtils.js Line 744 in 9e81447
This function "Sorts tags alphabetically by name." This function is used in OptionsListUtils.getFilteredOptions()
What changes do you think we should make in order to solve the problem?We can update it to sort numbers as numbers firstly, and then sort everything else alphabetically. (We need requirements what goes first, numbers or text). |
ProposalPlease re-state the problem that we are trying to solve in this issue.The list of tags consisting of numbers is not ordered in ascending order What is the root cause of that problem?App/src/libs/OptionsListUtils.js Lines 738 to 752 in 9e81447
The sortTags function incorrectly sorts numerical tags as strings, leading to a non-sequential order (e.g., 1, 10, 2). The issue stems from using Lodash's _.sortBy, which sorts elements as strings by default, not accounting for numeric values. What changes do you think we should make in order to solve the problem?Modify the sortTags function to differentiate between numeric and alphanumeric tags. Implement a conditional check within the sorting logic to treat numeric tags as numbers, ensuring correct numerical order. we can use localeCompare with numeric option and with adjustment to make symbols at the end instead of at the begining: function sortTags(tags) {
// Function to check if a string contains symbols
function isSymbol(s) {
const symbolsRegex = /^[^\p{L}\p{N}]+$/u; // Matches symbols
// reference: https://www.regular-expressions.info/unicode.html?ref=bluebirz.net
return symbolsRegex.test(s);
}
const sortedTags = tags.sort((tagA, tagB) => {
// Extract the name property of the tags
const nameA = tagA.name;
const nameB = tagB.name;
// Check if both tag names are symbols
const isSymbolA = isSymbol(nameA);
const isSymbolB = isSymbol(nameB);
// Compare symbols vs. non-symbols
if (isSymbolA && !isSymbolB) {
return 1; // Move symbols to the end
} else if (!isSymbolA && isSymbolB) {
return -1; // Move symbols to the end
}
// If both are symbols or both are non-symbols, use localeCompare for sorting
// reference: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/String/localeCompare
return nameA.localeCompare(nameB, 'en', { numeric: true });
});
return sortedTags;
}
// Example usage
// you can test it in https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/String/localeCompare
const tags = [
{ name: '0' },
{ name: '1' },
{ name: '2' },
{ name: '3' },
{ name: '10' },
{ name: '20' },
{ name: 'a' },
{ name: 'b' },
{ name: 'c' },
{ name: 'a1' },
{ name: 'a20' },
{ name: 'b1' },
{ name: 'b10' },
{ name: '日本' },
{ name: '中国' },
{ name: '!' },
{ name: '@' },
{ name: '#' },
{ name: '$' },
{ name: '0a' },
{ name: '10bc' },
{ name: '20a' }
];
const sortedTags = sortTags(tags);
const tagNames = sortedTags.map(tag => tag.name);
console.log(tagNames); Result:Array ["0", "0a", "1", "2", "3", "10", "10bc", "20", "20a", "a", "a1", "a20", "b", "b1", "b10", "c", "中国", "日本", "!", "@", "#", "$"] What alternative solutions did you explore? (Optional) |
@sonialiap, @situchan Huh... This is 4 days overdue. Who can take care of this? |
1 similar comment
@sonialiap, @situchan Huh... This is 4 days overdue. Who can take care of this? |
📣 It's been a week! Do we have any satisfactory proposals yet? Do we need to adjust the bounty for this issue? 💸 |
@sonialiap, @situchan Still overdue 6 days?! Let's take care of this! |
@situchan what do you think of the above proposals? |
ProposalPlease re-state the problem that we are trying to solve in this issue.Tag - The list of tags consisting of numbers is not ordered in ascending order. What is the root cause of that problem?When sorting the list of tags, we use default sortBy alphabetically so the list of tags consisting of numbers is not ordered in ascending order. App/src/libs/OptionsListUtils.ts Lines 869 to 879 in 1b3939d
What changes do you think we should make in order to solve the problem?We have a new sorting rule function sortTags(tags: Record<string, Tag> | Tag[]) {
const getGroupType = (str: string) => {
// Only Number
// if (/^\d+$/.test(str)) {
// return 0;
// }
// start with a number
if (/^\d/.test(str)) {
return 1;
}
// start with a letter
if (/^[a-zA-Z]/.test(str)) {
return 2;
}
// everything else
return 3;
};
if (Array.isArray(tags)) {
const sortedTags = tags.sort((a, b) => {
const typeA = getGroupType(a.name);
const typeB = getGroupType(b.name);
if (typeA !== typeB) {
// sort by type first
return typeA - typeB;
}
// sort by number value
// if (typeA === 0) {
// return Number(a.name) - Number(b.name);
// }
// perform a regular string comparison
return a.name.localeCompare(b.name);
});
return sortedTags;
}
return sortTags(Object.values(tags));
} What alternative solutions did you explore? (Optional)If we want the sort rule to look like the image attachment in the new rule comment we can update the function sortTags function sortTags(tags: Record<string, Tag> | Tag[]) {
let sortedTags;
if (Array.isArray(tags)) {
sortedTags = tags.sort((a, b) => {
const charCodeA = a.name.charCodeAt(0);
const charCodeB = b.name.charCodeAt(0);
if (charCodeA !== charCodeB) {
return charCodeA - charCodeB;
}
return a.name.localeCompare(b.name);
});
} else {
sortedTags = Object.values(tags).sort((a, b) => {
const charCodeA = a.name.charCodeAt(0);
const charCodeB = b.name.charCodeAt(0);
if (charCodeA !== charCodeB) {
return charCodeA - charCodeB;
}
return a.name.localeCompare(b.name);
});
}
return sortedTags;
} POC Screen.Recording.2024-03-01.at.00.49.59.mov |
ProposalPlease re-state the problem that we are trying to solve in this issue.Tag - The list of tags consisting of numbers is not ordered in ascending order. What is the root cause of that problem?It is impossible to sort strings appropriately using the inbuilt Especially, the strings with different lengths pose issues with the App/src/libs/OptionsListUtils.js Lines 751 to 757 in 0c351a4
What changes do you think we should make in order to solve the problem?We should replace the function
|
I am not sure this is bug. |
📣 It's been a week! Do we have any satisfactory proposals yet? Do we need to adjust the bounty for this issue? 💸 |
Trying to get engineer's thoughts if this is bug or expected 🎀 👀 🎀 |
Triggered auto assignment to @madmax330, see https://stackoverflow.com/c/expensify/questions/7972 for more details. |
@madmax330 @sonialiap @situchan this issue was created 2 weeks ago. Are we close to approving a proposal? If not, what's blocking us from getting this issue assigned? Don't hesitate to create a thread in #expensify-open-source to align faster in real time. Thanks! |
All proposed solutions don't satisfy above case. |
ProposalPlease re-state the problem that we are trying to solve in this issue.The tag sort order in new dot is not consistent with oldDot. What is the root cause of that problem?We are using different sorting algorithms; while oldDot uses underscore What changes do you think we should make in order to solve the problem?To ensure consistency, we can utilize the same
We also need to implement this in
Not sure if the tag is also required to detect locale or not, but we can omit It can also be applied to other fields if necessary and belong to the scope of this issue. What alternative solutions did you explore? (Optional)Change oldDot to use native |
Current assignee @madmax330 is eligible for the choreEngineerContributorManagement assigner, not assigning anyone new. |
📣 @situchan 🎉 An offer has been automatically sent to your Upwork account for the Reviewer role 🎉 Thanks for contributing to the Expensify app! |
📣 @wildan-m 🎉 An offer has been automatically sent to your Upwork account for the Contributor role 🎉 Thanks for contributing to the Expensify app! Offer link |
|
The solution for this issue has been 🚀 deployed to production 🚀 in version 1.4.58-8 and is now subject to a 7-day regression period 📆. Here is the list of pull requests that resolve this issue: If no regressions arise, payment will be issued on 2024-04-09. 🎊 For reference, here are some details about the assignees on this issue:
|
BugZero Checklist: The PR fixing this issue has been merged! The following checklist (instructions) will need to be completed before the issue can be closed:
|
Payment summary:
|
Thanks @sonialiap, usually C+ will suggest conducting the regression test and completing the checklist, Is that correct?. In my opinion, I don't think a regression test is necessary as we have already written unit tests for the sorting function, and the bug does not directly affect a specific flow. |
That's correct, not sure why melvin tagged both you and Situ. Thanks for your thoughts on the regression test. Sounds like we're good on that front ✔️ |
If you haven’t already, check out our contributing guidelines for onboarding and email [email protected] to request to join our Slack channel!
Version Number: 1.4.17-1
Reproducible in staging?: y
Reproducible in production?: y
If this was caught during regression testing, add the test name, ID and link from TestRail:
Email or phone of affected tester (no customers):
Logs: https://stackoverflow.com/c/expensify/questions/4856
Expensify/Expensify Issue URL:
Issue reported by: Applause - Internal Team
Slack conversation:
Action Performed:
Preconditions:
)
Steps:
Expected Result:
The list of tags consisting of numbers should be ordered in ascending order (1,2,3,4,5...).
Actual Result:
The list of tags consisting of numbers is not ordered in ascending order.
Workaround:
Unknown
Platforms:
Which of our officially supported platforms is this issue occurring on?
Screenshots/Videos
Add any screenshot/video evidence
Bug6326715_1703679900523.Recording__991.mp4
View all open jobs on GitHub
Upwork Automation - Do Not Edit
The text was updated successfully, but these errors were encountered: