Skip to content
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

Datagrid - Mixed Case sort has irregular results #6787

Closed
bthibodeau7 opened this issue Aug 30, 2022 · 23 comments · Fixed by #6930 or #7263
Closed

Datagrid - Mixed Case sort has irregular results #6787

bthibodeau7 opened this issue Aug 30, 2022 · 23 comments · Fixed by #6930 or #7263
Assignees
Labels
type: bug 🐛 [3] Velocity rating (Fibonacci)

Comments

@bthibodeau7
Copy link

In TWLOM, when sorting by Carrier the sort results do not make any sense. Do not appear alpha or numeric.

  1. SASTT - Ship Via. Set up the following ship-via's, 01, 07, 09PT, 32, 45PT, 1SHP, 405.

  2. OEET - Enter an order for each one of the Ship-Via's using a product with quantity available. Print to TWL

  3. TWLOM - Select to View All. Search. Sort by Carrier by clicking on the up and down arrow next to Carrier in the grid. Notice the sort is 09PT, 01, 1SHP, 32, 45PT, 07, 405.

The sort should be 01, 07, 09PT, 1SHP, 32, 405, 45PT. Numeric Carrier ID's must sort correctly. Alpha Carrier ID's work correctly.

Version
html lang="en-US" class="is-chrome" data-sohoxi-version="4.3.3"

Screenshots
image

Platform

  • Infor Application/Team Name: SX.enterprise
  • Device: (if applicable) [e.g. N/A
  • OS Version: N/A
  • Browser Name: Chrome and Edge
  • Browser Version: Chrome - Version 104.0.5112.102 (Official Build) (64-bit); Microsoft Edge - Version 104.0.1293.70 (Official build) (64-bit)

Additional context
Note from SX.enterprise Development: Updated a bunch of ordhdr.carrier values in Dev and verified there is no extra trailing spaces. This is standard SOHO grid sorting and we cannot control this in CSD. It looks somewhat correct in Dev sorting so more details would be needed and it would be an Infor SOHO issue as we do not control this unless the data is bad with extra spacing which is not the case in Dev.

@tmcconechy tmcconechy added type: bug 🐛 [2] Velocity rating (Fibonacci) status: clarification Issue needs clarification labels Aug 30, 2022
@tmcconechy
Copy link
Member

@bthibodeau7 I'm afraid i dont know what all these abbreviations mean. Can you put the issue in terms of how our components would understand?

I guess if i understand we should be able to reproduce this if we use the same data and do a sort? If so can we get some sample data in JSON format to use or an example?

I guess we can try to create this.. But it would speed the issue up

@bthibodeau7
Copy link
Author

@tmcconechy I can show you but it is a sort for a alpha-numeric grid. If you want to put something on my calendar. I don't know how to tell you without telling you the functions it is happening in.

@tmcconechy
Copy link
Member

tmcconechy commented Sep 2, 2022

I can "reproduce" it in this example i made...
test-combo-sort.html.zip

But this seems a bit "custom" and I tried the sorting in excel and it works the same as the datagrid
Screen Shot 2022-09-02 at 9 55 02 AM

We do have the ability to make your own sort function https://github.com/infor-design/enterprise/blob/main/app/views/components/datagrid/test-sort-override.html#L77 so maybe you can use that for this case? This is our default sort https://github.com/infor-design/enterprise/blob/main/src/components/datagrid/datagrid.js#L12436

@tmcconechy tmcconechy removed the status: clarification Issue needs clarification label Sep 2, 2022
@bthibodeau7
Copy link
Author

@tmcconechy How do you make your own sort function? This is an example of the way they thing it should sort which is how Excel sorts it:
image

This is what the column is currently doing:
image

@tmcconechy
Copy link
Member

I tried to explain that in the previous (how to make a custom sort).

We do have the ability to make your own sort function https://github.com/infor-design/enterprise/blob/main/app/views/components/datagrid/test-sort-override.html#L77 so maybe you can use that for this case? This is our default sort https://github.com/infor-design/enterprise/blob/main/src/components/datagrid/datagrid.js#L12436

Im wondering if this sort is normal or not? As the data looks pretty irregular. But i think we can investigate further. Im just wondering if this should be custom sort or if it should work with mixes cases this way? I think now its sorting alphabetical

@tmcconechy tmcconechy changed the title TWLOM - Numeric Carrier ID's are alpha numeric and do not sort correctly in the grid Datagrid - Mixed Case sort has irregular results Sep 7, 2022
@bthibodeau7
Copy link
Author

I would think any time a field is alpha numeric it would search the same as Excel does.

@tmcconechy
Copy link
Member

Here is the example test case i came up with
test-combo-sort.html.zip

@bthibodeau7
Copy link
Author

@tmcconechy I am in Support and not a programmer. I just need to know if this can be fixed. I know there is a way to set your sort but that is the issue, sorting by asc or dsnd does not seem to put them in the right order.

@tmcconechy
Copy link
Member

It's probably possible to fix. We have to work through it and make sure. Its accepted into the backlog

@tmcconechy tmcconechy added [3] Velocity rating (Fibonacci) and removed [2] Velocity rating (Fibonacci) labels Sep 14, 2022
@tjamesallen15
Copy link
Contributor

tjamesallen15 commented Oct 25, 2022

@tmcconechy How do you make your own sort function? This is an example of the way they thing it should sort which is how Excel sorts it: image

This is what the column is currently doing: image

Question. In this image, the top image is the expected result? I am confused. When I tried to sort it in Excel I received the sorting like this.
image

@ericangeles @tmcconechy @bthibodeau7

@tmcconechy
Copy link
Member

@tjamesallen15 excel does it like this and i think we should just do the same...

Screen Shot 2022-10-25 at 10 20 58 AM

I think the logic should be if it starts with a number sort by the number then go with the text. i.e. 0123456789abcdefghijk
But i think its really a bug in the existing search function https://github.com/infor-design/enterprise/blob/main/src/components/datagrid/datagrid.js#L12530

Maybe in here it is incorrect for mixed case because of the types https://github.com/infor-design/enterprise/blob/main/src/components/datagrid/datagrid.js#L12540-L12546

@tjamesallen15
Copy link
Contributor

@tjamesallen15 excel does it like this and i think we should just do the same...

Screen Shot 2022-10-25 at 10 20 58 AM

I think the logic should be if it starts with a number sort by the number then go with the text. i.e. 0123456789abcdefghijk But i think its really a bug in the existing search function https://github.com/infor-design/enterprise/blob/main/src/components/datagrid/datagrid.js#L12530

Maybe in here it is incorrect for mixed case because of the types https://github.com/infor-design/enterprise/blob/main/src/components/datagrid/datagrid.js#L12540-L12546

Thanks, Tim!

@janahintal
Copy link
Contributor

@aaronpikkarainen
Copy link
Contributor

@tmcconechy, when our team tried the coding fix for this issue and tested it with our data, we found that sorting is not working properly (in cases involving mixed numeric/text values; looks like other cases too).

I was able to find a good coding solution that imitates how Excel sorts mixed numeric/text values. This is how Excel sorts this kind of data:

  • numeric values are grouped together (sorted numerically)
  • text values are grouped next (sorted alphabetically)
  • blank values are at the bottom of the list (both ascending and descending)

For example, Excel will sort like this:

  • Ascending: 1, 2, 07, 11, 1a, 22a, 2ab, a, B, c
  • Descending: c, B, a, 2ab, 22a, 1a, 11, 07, 2, 1

Below is the coding solution. It is safe because the logic is simply dictating that numeric data is always less than string data (like Excel), and that empty values are always below other values (like Excel).

Can you use this new code for this issue instead? This is the updated "sortFunction" method from datagrid.js with the previous coding change removed and my two small blocks of code added (where you see comments "Imitate how Excel...").

`
sortFunction(id, ascending) {
const column = this.columnById(id);
// Assume the field and id match if no column found
const col = column.length === 0 ? null : column[0];
const field = col === null ? id : col.field;

const self = this;
const primer = function (a) {
  a = (a === undefined || a === null ? '' : a);

  if (typeof a === 'string') {
    a = a.toUpperCase();

    if ($.isNumeric(a)) {
      a = parseFloat(a);
    }
  }
  return a;
};

let key = function (x) { return primer(self.fieldValue(x, field)); };
if (col && col.sortFunction) {
  key = function (x) { return col.sortFunction(self.fieldValue(x, field)); };
}

ascending = !ascending ? -1 : 1;

return function (a, b) {
  a = key(a);
  b = key(b);

  // Imitate how Excel does sorting when comparing numbers with strings (numbers are always less than strings).
  // Note: The above primer function makes the data type of $.isNumeric() values to be number, which is important
  //       in imitating Excel sorting (i.e. the string '5' becomes 5 and is treated as a number in sorting).
  // Example: The following string values will sort in this order (ascending): 1, 2, 07, 11, 1a, 22a, 2ab, a, B, c
  if (typeof a === 'number' && typeof b === 'string' && b !== '') {
    return ascending * -1;
  } else if (typeof a === 'string' && typeof b === 'number' && a !== '') {
    return ascending;
  }

  // Imitate how Excel sorts blank values (always at end of list for both ascending and descending).
  // Note: It is annoying to see a bunch a blank values at the top of the list when trying to see sorted values.
  if (a === '') {
    return b === '' ? 0 : 1; // an empty a always returns 1 (or 0 if equal with b)
  } else if (b === '') {
    return a === '' ? 0 : -1; // an empty b always returns -1 (or 0 if equal with a)
  }

  if (typeof a !== typeof b) {
    a = a.toString().toLowerCase();
    b = b.toString().toLowerCase();
  }

  return ascending * ((a > b) - (b > a));
};

},
`

@tmcconechy
Copy link
Member

Is different than the current / new solution we added here? I.E. does this work better than the issue here and still satisfy this use case?

@aaronpikkarainen
Copy link
Contributor

Yes, the code I provided works better than what is currently in datagrid.js. It sorts properly (following how Excel does it), and it puts empty values at the bottom like Excel (which users prefer when they want to look at sorted results). It also fixes the bad side-effects that I saw with the previous fix.

@tmcconechy
Copy link
Member

ok cool, can you add a new issue as this is closed and confirmed. also can you mention what the bad side-effect was? Also you can do a PR against if if you think its complete :)

@aaronpikkarainen
Copy link
Contributor

That's quite a lot of work. Could we just reopen this issue, since the previous fix against this issue did not fix it properly?

For example, sort the "Id" column on the main Datagrid example (https://design.infor.com/code/ids-enterprise/latest/demo/components/datagrid/example-index.html), and you will see sorting is currently not right. It's not following how Excel sorts those Id values.

image

Excel sorting: "smallest to largest" (ascending)

image

@tmcconechy
Copy link
Member

Not sure what a lot of work for new issue but i can go either way i guess.

@tmcconechy tmcconechy reopened this Jan 20, 2023
@ericangeles
Copy link
Contributor

@tjamesallen15 please review it again. Let me know if you need help on this.

@tmcconechy
Copy link
Member

See if #6787 (comment) the code provided by @aaronpikkarainen works. I trust him that it does but just add it and do your due diligence in testing and updating tests (or adding some)

@aaronpikkarainen
Copy link
Contributor

Thank you, team, for getting this coding change in place. Imitating Excel's sorting will be a good strategy.

Much appreciated!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: bug 🐛 [3] Velocity rating (Fibonacci)
Projects
No open projects
6 participants