update flag states #35
Conversation
- The comment admin and moderator can now toggle between the flag states of a flagged comment. - Add function to check and update the flag state after each migrate command. - Extend the API to cover all GUI actions
|
|
||
| return super().get_queryset().select_related('flag').annotate( | ||
| flags=models.F('flag__count')).filter(flags__lte=allowed_flags) | ||
| return super().get_queryset().exclude(flag__state__exact=2) |
There was a problem hiding this comment.
Instead of using raw numbers maybe we may use Enum choices or the class variable here.
There was a problem hiding this comment.
Yes I will look at this in future code refactoring, I tried to get Flag.Flagged but got a circular import.
There was a problem hiding this comment.
Yes, that is the reason I had used the statues in managers.
| (UNFLAGGED, 'Unflagged'), | ||
| (FLAGGED, 'Flagged'), | ||
| (REJECTED, 'Flag rejected by the moderator'), | ||
| (RESOLVED, 'Comment modified by the author'), |
There was a problem hiding this comment.
Earlier gettext_lazy was used with the raw message for the state. It would help users from non-english language to easily port this in their language.
from django.utils.translation import gettext_lazy as _
STATE_CHOICES = [
(UNFLAGGED, _('Unflagged')),
(FLAGGED, _('Flagged')),
(REJECTED, _('Flag rejected by the moderator')),
(RESOLVED, _('Comment modified by the author')),
]There was a problem hiding this comment.
You are right. I will open an issue for translating to cover everything.
| if hasattr(self, 'flag'): | ||
| self.flag.refresh_from_db() | ||
| allowed_flags = getattr(settings, 'COMMENT_FLAGS_ALLOWED', 0) | ||
| if allowed_flags: | ||
| return self.flag.count > allowed_flags | ||
| if not self.flag.is_flag_enabled: | ||
| return False | ||
| return self.flag.state != self.flag.UNFLAGGED | ||
| return False |
There was a problem hiding this comment.
This can be changed to:
if self.flag.is_flag_enabled:
return self.flag.state != self.flag.UNFLAGGED
return FalseThere was a problem hiding this comment.
Your suggestion is not valid here because the developer may disable flagging and is_flagged will still be true and the comments will appear as flagged even if the feature is disabled.
There was a problem hiding this comment.
I'm not sure I understand your argument since I've not seen all the changes in depth. What I suggested was a simple refactoring of your code, which does the exact same thing as your code or does it?
There was a problem hiding this comment.
Simply: self.flag.state != self.flag.UNFLAGGED can be False or True, even if the flagging system is disabled.
I want is_flagged to be always false when the flagging system is disabled.
There was a problem hiding this comment.
Okay, now I understand. I had mistakenly added the hasattr check in my comment. I have edited it now. Everything else is fine.
|
Also, I had used 1 one single setting variable for allowing flags and their count. I think you have moved them to separate variables. We can ask the developer to set the value as 0 or False(whatever you feel like) to check if flags are allowed. This will help in moving the changes to one variable. |
The new variable is for new purpose. |
Okay. Suppose if I set allowed flag count to 10, I obviously want the comment below this number to be hidden. Do I miss something here? One use case that I can think of is notifying the moderators after this count for a review (before which the comments should/shouldn't be visible). I don't think we have implemented this till now. |
The comment that has flags count above this number will be hidden and here comes the new variable, maybe the developer doesn't want that comment to be hidden then he can simply set
We haven't. |
|
Okay, One of them is used to enable the flagging system. Also, In my opinion, the migration of comments should ideally be handled via a command to |
command.