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

TNO-2534 sort by string #1755

Merged
merged 10 commits into from
May 2, 2024
Merged

TNO-2534 sort by string #1755

merged 10 commits into from
May 2, 2024

Conversation

kkwangsir
Copy link
Contributor

@kkwangsir kkwangsir commented Apr 29, 2024

screen

I've modified many other parts, as shown in the screenshot, which works. However, I'd like you to test this natural sort first before I push the other changes

I found some issues.
https://github.com/bcgov/tno/blob/dev/app/editor/src/features/content/list-view/hooks/useColumns.tsx

from the code block.

      accessor: 'section',
      sort: (row) => {
        return `${row.original.page ? row.original.page : ''}:${
          row.original.section ? row.original.section : ''
        }`;
      },

we pass a function into sort, but we never run it.

Because if the accessor is a function, the return is accessor, each time we just sort the"section" which came from the col accessor value.

https://github.com/bcgov/tno/dev/libs/npm/core/src/components/table/FlexboxTable.tsx

      col.sort ?? typeof col.accessor === 'function'
        ? undefined
        : col.accessor,

but as the code below from https://github.com/bcgov/tno/blob/dev/libs/npm/core/src/components/table/utils/sortRows.ts
"I think we expect to sort by accessor function, however, if we change the accessor to any function, it causes a 500 error in Elastic Search.

I modified a couple of places to use sortBy.sort === 'function', but I'm not sure if this is necessary.

      if (sortBy) {
        if (typeof sortBy.sort === 'function') {
          const aVal = sortBy.sort(a) ?? '';
          const bVal = sortBy.sort(b) ?? '';
          return (aVal < bVal ? -1 : 1) * (sortBy.isSortedDesc ? -1 : 1);
        }

        if (typeof sortBy.sort === 'string') {
          const aVal = a.original[sortBy.sort as keyof T] ?? '';
          const bVal = b.original[sortBy.sort as keyof T] ?? '';
          return (aVal < bVal ? -1 : 1) * (sortBy.isSortedDesc ? -1 : 1);
        }

        // Use the accessor for the search column.
        const accessor = a.columns[sortBy.index].accessor;
        if (typeof accessor === 'function') {
          const aVal = accessor(a.original) ?? '';
          const bVal = accessor(b.original) ?? '';
          return (aVal < bVal ? -1 : 1) * (sortBy.isSortedDesc ? -1 : 1);
        }
      }

@Fosol Fosol added the bug Something isn't working label Apr 29, 2024
// Replace each segment of digits and non-digits with formatted strings
// Digits are padded with zeros to the left to ensure correct natural sorting
// Non-digits are left as is
return pageSectionValue.replace(/(\d+)|(\D+)/g, (_, $1, $2) =>
Copy link
Collaborator

Choose a reason for hiding this comment

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

If I'm reading this correct you're extracting the decimal value from the string, and then replacing it by padding the number with 10 zeros?

What I don't understand is how this will be applied to the list-view? I'm not seeing the code that links this. How would this be tested currently?

Copy link
Contributor Author

@kkwangsir kkwangsir Apr 29, 2024

Choose a reason for hiding this comment

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

Hi Jeremy.
It added zero to number to help sort.

> testData = {
...   original: {
...     page: 'Page2',
...     section: 'sport'
...   }
... };
{ original: { page: 'Page2', section: 'sport' } }
> testData2 = {
...   original: {
...     page: 'Page02',
...     section: 'sport'
...   }
... };
{ original: { page: 'Page02', section: 'sport' } }
>
> naturalSortValue(testData)
'page0000000002:sport'
> naturalSortValue(testData2)
'page0000000002:sport'

so the "page2" and "page02" can convert to a same string.

The problem I'm currently facing is:

A. Normally, I cannot pass a function into the sort function. This cannot be executed, and sort will be undefined in the following logical judgment. to use sort as a function this involves modifying the code in tno-core.

First, I modified the code in useColumns.tsx the editor list view:

Passing a function to sort:

accessor: 'section', sort: (row) => naturalSortValue(row)

In flexboxTable.tsx, from tno-core

  col.sort ?? typeof col.accessor === 'function'
    ? undefined
    : col.accessor,

I replace it by the below

const determineSortValue = (col: ITableInternalHeaderColumn<T>) => {
    // If 'col.sort' is defined and not null, use it directly for sorting
    if (col.sort !== undefined && col.sort !== null) {
      console.log('col.sort', col.sort);
      return col.sort;
    }
    // If 'col.sort' is undefined or null and 'accessor' is a function,
    // it indicates that 'accessor' cannot be used for sorting directly (e.g., dynamic values)
    if (typeof col.accessor === 'function') {
      console.log('col.accessor', col.accessor);
      return undefined;
    }
    // If 'col.sort' is undefined or null, and 'accessor' is not a function,
    // use 'accessor' as the field name for sorting
    return col.accessor;
}

sort: determineSortValue(col)

This let sort can be a function, and also allow us to use sort function in sortRows.ts

to execute the following code block :


      if (sortBy) {
        if (typeof sortBy.sort === 'function') {
          const aVal = sortBy.sort(a) ?? '';
          const bVal = sortBy.sort(b) ?? '';
          return (aVal < bVal ? -1 : 1) * (sortBy.isSortedDesc ? -1 : 1);
        }



But I'm not sure if we want to make these changes. it may affect other parts of the code.

B. If I pass a function into the accessor, it will cause an Elasticsearch error later on, but you still can see the sort function works for the results.

such as simply replace accessor useColumns.tsx the editor list view:

accessor: 'section' ===> accessor : (row) => naturalSortValue(row)

in fact, using any function here will always cause an error

Copy link
Collaborator

Choose a reason for hiding this comment

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

Elasticsearch will not understand any of this fancy sorting. The solution that will work for us is to simply sort the results in the UI after Elasticsearch returns the results.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure I understand how this would help in the front end either. If you did that page number padding/manipulation you have indirectly suggested, wouldn't it need to be applied retroactively in elastic search to ALL prior content?

Copy link
Collaborator

Choose a reason for hiding this comment

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

It would be a good idea to apply to new content coming in, but isn't critical for this fix. Applying the sort to the UI should resolve the issue 99% of the time.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In fact, you can test it. I think it's fine in most cases. Now it can sort resource first then page and section

Copy link
Collaborator

@Fosol Fosol left a comment

Choose a reason for hiding this comment

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

I tested your changes locally, but they don't appear to work.
image
image

@kkwangsir
Copy link
Contributor Author

I tested your changes locally, but they don't appear to work. image image

I think I made it like this You will see the page sorted within the same source. Perhaps I'm overthinking it, but if we only want to sort the page, I can quickly make the change.

@Fosol
Copy link
Collaborator

Fosol commented May 1, 2024

The default sort (before clicking on any of the sort columns) should be Source.SortOrder, Source.Code, Page, Section. When a user clicks the sorting columns the sort is applied in the order they manually select them.

Looking at the results you can see for some reason CP News was first even though the page was A18. This would indicate that it was also sorting on the source code. When clicking the sorting columns you can see I don't have the source column sorted. Therefore the sort should only be Page, Section.

@Fosol
Copy link
Collaborator

Fosol commented May 1, 2024

I've published tno-core:0.1.57 for you. Make sure you rebase and add the package to both the Editor and Subscriber. Don't forget to yarn install on both of them.

@kkwangsir
Copy link
Contributor Author

image test it with the package, sorting works as we want

@Fosol
Copy link
Collaborator

Fosol commented May 1, 2024

You'll need to update the package.json files for the Editor and Subscriber apps and update your PR.

Copy link
Collaborator

@Fosol Fosol left a comment

Choose a reason for hiding this comment

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

Need to rebase and resolve conflicts

@Fosol Fosol merged commit d98ddb6 into bcgov:dev May 2, 2024
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants