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

feat: sort table columns #581

Merged
merged 1 commit into from
Sep 13, 2024
Merged

feat: sort table columns #581

merged 1 commit into from
Sep 13, 2024

Conversation

henrychoy
Copy link
Collaborator

This PR adds the ability to sort table columns, specifically name, description, createdOn, and lastModified. Also, jobs are sortable by status and artifacts are sortable by uri. Not every table has all these columns because of space, but this is easily configurable if we want to change the default columns.

Tests were also added for each resource type to verify sort order.

Copy link
Collaborator

@keithmanville keithmanville left a comment

Choose a reason for hiding this comment

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

Sorting tags by lastModifiedOn does not seem to work even though the correct queries appear to be made to the API.

I noticed you've started adding the createdOn and lastModifiedOn columns to tables, but it's not consistent. Is there a reasoning behind which tables have this and which don't?

I confirmed tests are passing

None,
None,
[
"Model artifact.",
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can we used a list of IDs for expected instead of choosing an arbitrary field? This comment applies to the sort tests as well.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Pushed up the fix for tags, I had to reference models.Tag.last_modified_on instead of models.Resource.last_modified_on like all the others.

As for the date columns, I just arbitrarily added them to the tables that were less crowded. We can definitely talk about finalizing the columns or adding some functionality for allowing the user to add/hide columns in a future task.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Can we used a list of IDs for expected instead of choosing an arbitrary field? This comment applies to the sort tests as well.

Sure, just to clarify, we should still sort by name and created_on, but have the expected list be the ID's? So something like this?

@pytest.mark.parametrize(
    "sortBy, descending , expected",
    [
        (None, None, ["1", "2", "3"]),
        ("name", True, ["3", "2", "1"]),
        ("name", False, ["1", "2", "3"]),
        ("createdOn", True, ["3", "2", "1"]),
        ("createdOn", False, ["1", "2", "3"]),
    ],
)

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yup, except they should be ints. You can compare them against resource["id"]. Using the IDs is better since they are unique. It also makes all of these parameterized sorting tests more consistent and less verbose.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@pytest.mark.parametrize(
    "sortBy, descending , expected",
    [
        (None, None, [7, 8, 9]),
        ("name", True, [8, 7, 9]),
        ("name", False, [9, 7, 8]),
        ("createdOn", True, [9, 8, 7]),
        ("createdOn", False, [7, 8, 9]),
    ],
)

I updated and pushed just the queues test above which passes. Is printing out the registered items the only way to find the id's? Also the only drawback I see to using id's is that the id value is unrelated to sorting and if that logic were to change, this would fail. For example, I don't know why the id's start at 7. But let me know if this is fine and I'll copy it to the rest of the tests.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@keithmanville I updated all the sort tests to use IDs as discussed on the call. Let me know if any other issues.

@keithmanville
Copy link
Collaborator

LGTM

rebased on dev and resolved conflict from typo commit.
ran black and isort.

@keithmanville
Copy link
Collaborator

@henrychoy can you rebase / resolve conflicts with dev?

Then I will sqauash and merge.

@henrychoy
Copy link
Collaborator Author

@henrychoy can you rebase / resolve conflicts with dev?

Then I will sqauash and merge.

@keithmanville Ran into some errors with rebase and merge, but got merge to work. Should be all set now.

This commit adds table sorting functionality for all resources to the UI. It adds 'sortBy' and
'descending' query parameters to each of the base GET endpoints and updates the services to handle
changes to the query. It adds unit tests to test the new sorting functionality.
@keithmanville
Copy link
Collaborator

@henrychoy I think I was able to do the rebase properly. We want to rebase on dev instead of merging dev so we can maintain a clean commit history.

@keithmanville keithmanville merged commit 76d5646 into dev Sep 13, 2024
11 checks passed
@keithmanville keithmanville deleted the sort-columns branch September 13, 2024 14:18
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.

As a frontend user I need to be able to sort tables in order to more easily navigate resources
2 participants