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 by 'views' column in Table view #475

Merged
merged 2 commits into from
Dec 14, 2018
Merged

Fix sorting by 'views' column in Table view #475

merged 2 commits into from
Dec 14, 2018

Conversation

srowen
Copy link
Contributor

@srowen srowen commented Dec 13, 2018

Description of changeset:

I believe this could resolve #363
The issue is that index-table.html uses these column names when forming the sort_by query parameter ...

var col_names = ['Title', 'Author', 'CreatedAt', 'UpdatedAt', 'Views', 'Upvotes', 'Comments']

... but posts.py is expecting sort_by columns like ...

    post_properties = {
        "updated_at": Post.updated_at,
        "created_at": Post.created_at,
        "title": Post.title,
    }
    join_order_col = {
        "uniqueviews": func.count(distinct(PageView.user_id)),
        "allviews": func.count(PageView.object_id),
        "upvotes": func.count(Vote.object_id),
        "comments": func.count(Comment.post_id)
    }

views isn't among the options but uniqueviews and allviews are. I think this should be an alias for allviews?

Test Plan:

./run_tests.sh passes, but I am not sure how to run this locally to test the sorting.

Auto-reviewers: @NiharikaRay @matthewwardrop @earthmancash @danfrankj

@AppVeyorBot
Copy link

@matthewwardrop
Copy link
Collaborator

matthewwardrop commented Dec 13, 2018

Thanks for the patch @srowen ! I don't think I've ever touched this piece of code in the knowledge repo since I got involved in the project; I'll take a look soon. From a quick glance, the patch looks good.

Regarding testing it locally, you should be able to run:
./<repo_path>/scripts/knowledge_repo --repo <knowledge repository git repo or folder> runserver --debug --port 7000
which will spin up a local webserver instance at 'http://127.0.0.1:7000'.

@srowen
Copy link
Contributor Author

srowen commented Dec 13, 2018

Thanks @matthewwardrop ! Right that's easy to run locally, thanks (with --repo in front of the repo). I'm glad I did because this wasn't quite enough. The other problem is that the join condition this creates uses object IDs, but the join was on Post.path. Once that was fixed, this works for views and fixes the comments, upvotes columns.

Author still doesn't work actually because the join condition would need to be more complex to match posts to author IDs. Maybe we can fix that separately or else remove sorting by author?

@AppVeyorBot
Copy link

@matthewwardrop
Copy link
Collaborator

Whoops! Yes, you need the --repo flag. I've modified my comment above to include it for posterity.

I'm happy to have the author sorting fixed separately. I've also tossed around the idea of removing this table view, but only after we have proper sorting and filtering of the main index "feed"... which would probably take more time than I have to dedicate to the project at the moment. In the meantime, fixes like these are really valuable in terms of keep the project usable. So thanks again!

This patch looks good now, so I'll merge it in!

@matthewwardrop matthewwardrop merged commit b654895 into airbnb:master Dec 14, 2018
@srowen srowen deleted the Issue363 branch December 14, 2018 15:27
tanuj208 pushed a commit to ElucidataInc/knowledge-repo that referenced this pull request Jun 6, 2019
* Handle sorting by 'views' column

* Fix join to use post ID
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.

Broken Sorting in Table View
3 participants