enterprise icon indicating copy to clipboard operation
enterprise copied to clipboard

Datagrid - Mixed Case sort has irregular results

Open bthibodeau7 opened this issue 2 years ago • 9 comments

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.

bthibodeau7 avatar Aug 30 '22 12:08 bthibodeau7

@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

tmcconechy avatar Aug 30 '22 13:08 tmcconechy

@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.

bthibodeau7 avatar Sep 01 '22 20:09 bthibodeau7

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 avatar Sep 02 '22 13:09 tmcconechy

@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

bthibodeau7 avatar Sep 06 '22 20:09 bthibodeau7

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 avatar Sep 07 '22 13:09 tmcconechy

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

bthibodeau7 avatar Sep 07 '22 19:09 bthibodeau7

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

tmcconechy avatar Sep 08 '22 14:09 tmcconechy

@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.

bthibodeau7 avatar Sep 12 '22 16:09 bthibodeau7

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

tmcconechy avatar Sep 12 '22 16:09 tmcconechy

@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

tjamesallen15 avatar Oct 25 '22 02:10 tjamesallen15

@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

tmcconechy avatar Oct 25 '22 14:10 tmcconechy

@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!

tjamesallen15 avatar Oct 26 '22 00:10 tjamesallen15

QA Passed. Verified in https://main-enterprise.demo.design.infor.com/components/datagrid/test-combo-sort. Thanks!

janahintal avatar Nov 02 '22 14:11 janahintal

@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));
};

}, `

aaronpikkarainen avatar Jan 20 '23 14:01 aaronpikkarainen

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?

tmcconechy avatar Jan 20 '23 14:01 tmcconechy

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.

aaronpikkarainen avatar Jan 20 '23 14:01 aaronpikkarainen

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 :)

tmcconechy avatar Jan 20 '23 14:01 tmcconechy

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

aaronpikkarainen avatar Jan 20 '23 15:01 aaronpikkarainen

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

tmcconechy avatar Jan 20 '23 15:01 tmcconechy

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

ericangeles avatar Feb 03 '23 14:02 ericangeles

See if https://github.com/infor-design/enterprise/issues/6787#issuecomment-1398479727 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)

tmcconechy avatar Feb 03 '23 14:02 tmcconechy

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

Much appreciated!

aaronpikkarainen avatar Feb 28 '23 14:02 aaronpikkarainen

QA Passed. Verified in https://main-enterprise.demo.design.infor.com/components/datagrid/example-index.html & https://main-enterprise.demo.design.infor.com/components/datagrid/test-combo-sort

image image

janahintal avatar Mar 03 '23 13:03 janahintal