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

Fix sorting search result by health #5880

Merged
merged 3 commits into from
Dec 24, 2020

Conversation

kozlovsky
Copy link
Contributor

This PR fixes a problem when folders are not displayed in the channel content view after sorting by "health".

The essence of the fix is in replacing INNER JOIN to LEFT JOIN in SQL query, so ChannelNode objects without TorrentState corresponding objects are still returned by the query.

Also, in this PR I add default ordering by rowid to all queries which return ChannelNode objects, to have guaranteed stable sorting order in tests.

Some tests are marked with FIXME as they are not returning totally correct results. Namely, full-text search queries don't return more than one folder. This is a separate bug in full-text search queries and will be fixed in a separate PR.

@kozlovsky kozlovsky requested a review from ichorid December 23, 2020 22:02
@kozlovsky
Copy link
Contributor Author

retest this please

@sonarqubecloud
Copy link

Kudos, SonarCloud Quality Gate passed!

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 7 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication

@@ -450,6 +450,132 @@ def test_get_channels(metadata_store):
assert len(channels) == 5


@db_session
Copy link
Contributor

Choose a reason for hiding this comment

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

👍 Wow! Quite a test 👏

@ichorid ichorid merged commit 257a396 into Tribler:devel Dec 24, 2020
@kozlovsky kozlovsky deleted the fix/fix-sorting-by-health branch August 20, 2021 06:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants