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

top comments based on likes #83

Closed
bitFez opened this issue Sep 23, 2020 · 19 comments · Fixed by #154
Closed

top comments based on likes #83

bitFez opened this issue Sep 23, 2020 · 19 comments · Fixed by #154
Assignees
Labels
Milestone

Comments

@bitFez
Copy link
Contributor

bitFez commented Sep 23, 2020

Is there a way to list for example top 5 comments based on likes on a Post?

@bitFez bitFez added the feature label Sep 23, 2020
@abhiabhi94
Copy link
Collaborator

abhiabhi94 commented Sep 24, 2020

For getting the top comments based on likes, you may use this function:

from comment.models import Comment

from post.models import Post

# whatever model you associate comments with, just get an instance of the model here
post = Post.objects.first()

Comment.objects.filter_comments_by_object(post).order_by('-reaction__likes')

Most likely, you would want to sort the parent comments based on likes, for that you may use this instead

Comment.objects.filter_comments_by_parent(post).order_by('-reaction__likes')

This feature is in the pipeline but hasn't been implemented yet. Most likely, we will try to incorporate it in one of the future releases.

@bitFez
Copy link
Contributor Author

bitFez commented Sep 24, 2020

Thank you again. I'm afraid it's not working still. I will try to sit down again and take a closer look but here is the error message:
image

@bitFez
Copy link
Contributor Author

bitFez commented Sep 24, 2020

This is my views entry for top comments which I pass through as context.

top_comments = Comment.objects.filter_comments_by_object(madde).order_by('-reaction__likes')

@abhiabhi94
Copy link
Collaborator

This is my views entry for top comments which I pass through as context.

top_comments = Comment.objects.filter_comments_by_object(madde).order_by('-reaction__likes')

Sorry i wrote the wrong functions, the correct ones are:

# sort all comments
Comment.objects.all_comments_by_object(post).order_by('-reaction__likes')
# sort all parent comments
Comment.objects.filter_parents_by_object(post).order_by('-reaction__likes')

@abhiabhi94
Copy link
Collaborator

Hey @Radi85, I was thinking of implementing this feature. It would be great to have the option to render comments in order of reaction(likes, dislikes etc) as well as the same in a reverse order. The reverse order thing can also be added with the present way in which we render comments.

Any thoughts from your side would be welcome.

@abhiabhi94 abhiabhi94 self-assigned this Nov 26, 2020
@Radi85
Copy link
Owner

Radi85 commented Dec 6, 2020

Hey @abhiabhi94,

This would be a nice feature. Would you create a setting variable to let the developers choose their desired ordering?
Which milestone do you think that we can assign for this feature?

@abhiabhi94
Copy link
Collaborator

This shouldn't require too much of work so 2.6 should be okay.

Would you create a setting variable to let the developers choose their desired ordering?

A setting variable is not a great option in my view. This might hinder the user if they want different order for comment on different objects.

The raw idea that I have right now in my mind as of now, how the template tags have been written, I think we would require some sort of change or add some template tags, because ordering based upon reactions(like, dislike) would be a bit different from order based on replies or date.

@Radi85
Copy link
Owner

Radi85 commented Dec 6, 2020

You may add an optional argument order_by to render_comments template tag to provide a list of item to be considered for ordering when fetching the comments from DB.

@Radi85 Radi85 added this to the 2.6.0 milestone Dec 6, 2020
@abhiabhi94
Copy link
Collaborator

abhiabhi94 commented Dec 6, 2020

You may add an optional argument order_by to render_comments template tag to provide a list of item to be considered for ordering when fetching the comments from DB.

ordering by the number of replies will be a bit different from ordering by likes, also because we can't just directly add the argument to the order_by function from django.

Ordering with likes

Comment.objects.filter_comments_by_object(post).order_by('-reaction__likes')

Ordering with replies

# will have to add a property reply_count and see if this works or have to use annotation
Comment.objects.filter_comments_by_object(post).order_by('-reply_count')

@Radi85
Copy link
Owner

Radi85 commented Dec 6, 2020

This can be simply documented, so I can pass order_by as follows:

order_by = ['-reaction__likes', '-reply_count', '-posted']

@abhiabhi94
Copy link
Collaborator

Hey @Radi85, I think we can add another setting attribute so that this can be applied globally, if required by the developer. We can probably pass their order to the Meta options inside the comment model.

As of now, I haven't though of any pitfalls or issues that this may cause anything else in the codebase. Any thoughts from your side would be great.

Also, may be at a later stage we can probably think of providing a dropdown inside the templates to sort the comment based upon any of the available parameters.

@Radi85
Copy link
Owner

Radi85 commented Jan 4, 2021

Hey @Radi85, I think we can add another setting attribute so that this can be applied globally, if required by the developer. We can probably pass their order to the Meta options inside the comment model.

Yes. This is how it should be. COMMENT_ORDER_BY in the settings.

As of now, I haven't though of any pitfalls or issues that this may cause anything else in the codebase. Any thoughts from your side would be great.

The issue that can happen is passing a wrong field name. We might test the field names before passing them to meta class or just simply leave the app raising the default error.

Also, may be at a later stage we can probably think of providing a dropdown inside the templates to sort the comment based upon any of the available parameters.

This would be a nice feature.

@abhiabhi94
Copy link
Collaborator

The issue that can happen is passing a wrong field name. We might test the field names before passing them to meta class or just simply leave the app raising the default error.

Yes, I think that would be necessary.

In addition to these, I think we should add similar checks for other setting variables or would that fall into the YAGNI(you aren't going to need it) category?

@Radi85
Copy link
Owner

Radi85 commented Jan 5, 2021

In addition to these, I think we should add similar checks for other setting variables or would that fall into the YAGNI(you aren't going to need it) category?

In fact I would prefer to not check any of settings values provided by the developer, let the app crach and notify the developer. Bcz if I fallback to the default value, that might be unnoticeable and give unexpected result.

Moreover, most of the settings values are boolean and checking them is really YAGNI.

@abhiabhi94
Copy link
Collaborator

Moreover, most of the settings values are boolean and checking them is really YAGNI.

Seems legit.

My only point of concern now is every change in this setting would create a new migration. Would that be a good design strategy?

We can probably bypass this issue by overriding the default queryset by this setting value. What do you think about this?

@Radi85
Copy link
Owner

Radi85 commented Jan 5, 2021

My only point of concern now is every change in this setting would create a new migration. Would that be a good design strategy?

I just realized that ordering is part of the migration, good you mentioned that 😅 . Of course this will be shitty design to change the migration when the user preference changes. DB (tables structure) should not be changed at all based on the user preference.

We can probably bypass this issue by overriding the default queryset by this setting value. What do you think about this?

Yes we can extend the default queryset with the provided ordering.

@abhiabhi94
Copy link
Collaborator

As far as ordering by reply_count is concerned, i am afraid we might have to add a column to the database, because we can't use the order_by attribute for SQL queries with properties(i.e. python code).

An alternative to this would be something along the lines:

import functools

def get_queryset(self):

        @functools.lru_cache(maxsize=None)
        def is_reverse(order):
            if order[0] == '-':
                return True
            return False

        order = settings.COMMENT_ORDER_BY
        # ignore this method for now, it just handles the validation part
        validate_order(order)
        qs = super().get_queryset()
        if order.find('replies') < -1:
            return qs.order_by(order)

        return sorted(qs, key=lambda a: a.replies().count(), reverse=is_reverse(order))

The above can't be used because it returns a list in case of replies and not a queryset, which will create issues with the generic views and hell lot of other stuff.

In case, @Radi85 you have any suggestions to a way around this issue, it would be great to know them.

@Radi85
Copy link
Owner

Radi85 commented Jan 5, 2021

Filtering by replies seems weird for me. We are currently using comment thread with one level of replies, how this can be controlled/calcualted if we want to go more deeper?!

I would suggest to not implement ordering by replies for now and in case we recieve more request on it, we can then implement it and adjust the list views that uses the queryset.

@abhiabhi94
Copy link
Collaborator

abhiabhi94 commented Jan 6, 2021

Filtering by replies seems weird for me. We are currently using comment thread with one level of replies, how this can be controlled/calcualted if we want to go more deeper?!

Just to be on the same page, replies here stands for reply count.

Sorting by reply count can be one of the ways to present more "popular" or "discussed" conversations first. This would only be done for parent comment as of now. This assumption seems reasonable to me.

abhiabhi94 added a commit to abhiabhi94/Comment that referenced this issue Jan 20, 2021
abhiabhi94 added a commit to abhiabhi94/Comment that referenced this issue Jan 20, 2021
abhiabhi94 added a commit to abhiabhi94/Comment that referenced this issue Jan 20, 2021
abhiabhi94 added a commit to abhiabhi94/Comment that referenced this issue Jan 20, 2021
abhiabhi94 added a commit to abhiabhi94/Comment that referenced this issue Jan 20, 2021
abhiabhi94 added a commit to abhiabhi94/Comment that referenced this issue Jan 20, 2021
abhiabhi94 added a commit to abhiabhi94/Comment that referenced this issue Jan 20, 2021
abhiabhi94 added a commit to abhiabhi94/Comment that referenced this issue Jan 20, 2021
abhiabhi94 added a commit to abhiabhi94/Comment that referenced this issue Jan 30, 2021
abhiabhi94 added a commit to abhiabhi94/Comment that referenced this issue Jan 31, 2021
abhiabhi94 added a commit to abhiabhi94/Comment that referenced this issue Feb 4, 2021
Radi85 pushed a commit that referenced this issue Feb 6, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants