WIP: For logged #272
WIP: For logged #272
Conversation
|
What is the use-case for this? I gather this would allow a moderator to restrict access to some topic. But in what case is that needed?
This is what you wrote here. But this use-case seems to be covered by the "private forum" setting. A half private/half public forum would require the group permission feature, to restrict access to an entire category rather than individual topics. |
I didn't know about that setting, I missed it. |
|
If you were planning on setting every topic in a given category as private, then that use-case is better solved by group permissions. The category would be visible to some group(s) as well as all the topics in it. So, categories only visible to logged-in users would be covered by the group permissions feature (i.e: a user must be logged-in and be in an authorized group). |
|
Makes sense, so do you think it's just better to create the whole groups feature rather than doing this "only-for-logged" small one? I will probably keep using this fork and make another fork for the whole "groups-permissions" (if that's what you think is best) |
|
On a second thought, I'm not sure groups would replace this. Since it's a small change, I say we add this but for categories instead of topics. How that sounds? |
| @@ -25,6 +25,9 @@ def visible(self): | |||
|
|
|||
| def opened(self): | |||
| return self.filter(is_closed=False) | |||
|
|
|||
| def for_non_logged_users(self): | |||
| return self.filter(is_for_logged=False) | |||
There was a problem hiding this comment.
I think it may be better to change the visible filter to take a user parameter and do the (conditional) filtering there.
We would otherwise be adding this new filter everywhere where visible is used. Also, since we are breaking the visible API, then the test should fail in all the places where this filtering is required (i.e: the user profile comes to mind).
| @@ -44,6 +44,7 @@ class Topic(models.Model): | |||
| is_globally_pinned = models.BooleanField(_("globally pinned"), default=False) | |||
| is_closed = models.BooleanField(_("closed"), default=False) | |||
| is_removed = models.BooleanField(default=False) | |||
| is_for_logged = models.BooleanField(_("is for logged"), default=False) | |||
There was a problem hiding this comment.
May be a better name would be is_login_required (like the decorator) or is_for_logged_in_users (but it's loooong)`
Don't forget to run the makemigrations command and commit the new migration file.
|
|
||
| ACTION = ( | ||
| (COMMENT, _("comment")), | ||
| (MOVED, _("topic moved")), | ||
| (CLOSED, _("topic closed")), | ||
| (UNCLOSED, _("topic unclosed")), | ||
| (PINNED, _("topic pinned")), | ||
| (UNPINNED, _("topic unpinned"))) | ||
| (UNPINNED, _("topic unpinned")), | ||
| (FOR_LOGGED, _("topic for logged users")), |
There was a problem hiding this comment.
"logged users" -> "logged-in users"
|
I added some comments to the code with changes I would have requested, just in case. |
Yeah, that is definitely a better idea. |
This would allow for topics to be invisible for non logged users.
I opened this PR as work in progress so I can get an idea if this is something that you want to see implemented or not.
If you think it is I could work in tests, upgrade, etc.
The main flaw of the feature as it is now IMO is that the "security" around non logged users to be able to see topics it's a bit useless since there is no control in user's registration.