-
Notifications
You must be signed in to change notification settings - Fork 15
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
Sort dataset files by size #2080
Sort dataset files by size #2080
Conversation
Hi @luistoptal, |
Hi @rija Nothing is preventing, there are about 18 pending PRs, I keep ready for review only a few at a time and set for review more of them as the existing ones are merged I will rebase this one and make ready |
…t-take-units-into-account
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @luistoptal,
praise: The sorting based on size is working on my dev
issue: There should be acceptance for it
suggestion: Maybe the acceptance test could be as the followings:
Scenario: Files tab can be sort by size in ascending order
Given I have not signed in
And I am on "/dataset/100035"
And I follow "Files"
And I follow "1"
And I should see ""
When I click "Size"
Then I should see ""
Scenario: Files tab can be sort by size in descending order
Given I have not signed in
And I am on "/dataset/100035"
And I follow "Files"
And I follow "1"
And I should see ""
When I click "Size"
And I click "size"
Then I should see ""
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @luistoptal,
praise: functionality works locally and on my AWS deployment
praise: great that they are additional test scenarios that cover the functionality, with one of them capable of surfacing the bug raised in the associated issue
praise: all tests are passing locally and on CI
Based on that, I'm happy to approve this PR
…units-into-account
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @luistoptal,
praise: The acceptance tests are created and passing.
Based on the above and given the PR conflict is fixed, happy to approve.
…units-into-account
Pull request for issue: #2067
How to test?
How have functionalities been implemented?
customized the sorting function for the size column
Any issues with implementation?
--
Any changes to automated tests?
--