-
Notifications
You must be signed in to change notification settings - Fork 2
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
Added Rest APIs to add/remove moderators in a channel #148
Conversation
Codecov Report
@@ Coverage Diff @@
## master #148 +/- ##
==========================================
+ Coverage 96.31% 96.34% +0.03%
==========================================
Files 118 118
Lines 2876 2929 +53
Branches 70 73 +3
==========================================
+ Hits 2770 2822 +52
Misses 96 96
- Partials 10 11 +1
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I know this is WIP, just doing a preemptive review for a couple things
channels/api.py
Outdated
try: | ||
user = User.objects.get(username=moderator_name) | ||
except User.DoesNotExist: | ||
raise Exception("User {} does not exist".format(moderator_name)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you change this to use NotFound
similar to how it's used in https://github.com/mitodl/open-discussions/pull/140/files?
channels/urls.py
Outdated
@@ -20,6 +22,16 @@ | |||
name='post-list', | |||
), | |||
url( | |||
r'^api/v0/channels/(?P<channel_name>[A-Za-z0-9_]+)/moderators/$', | |||
ModeratorListView.as_view(), | |||
name='moderators-list', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you change this to moderator-list
and the below line to moderator-detail
?
65f0792
to
c77f9dd
Compare
channels/api.py
Outdated
|
||
new_moderator = Api(user) | ||
new_moderator.accept_invite(channel_name) | ||
del new_moderator |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
FYI You generally should never need to do this, it will get deleted when the local variable goes out of scope (when the reference count is 0)
channels/api.py
Outdated
@@ -434,9 +434,20 @@ def add_moderator(self, moderator_name, channel_name): | |||
raise NotFound("User {} does not exist".format(moderator_name)) | |||
|
|||
self.get_channel(channel_name).moderator.add(user) | |||
|
|||
new_moderator = Api(user) | |||
new_moderator.accept_invite(channel_name) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Did you talk to @giocalitri about this approach?
channels/serializers.py
Outdated
def validate_moderator_name(self, value): | ||
"""Validates the moderator name""" | ||
if not isinstance(value, str): | ||
raise ValidationError("contributor name must be a string") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you change this to moderator name must be a string
? And also modify the next validation error below
channels/api.py
Outdated
@@ -1,4 +1,5 @@ | |||
"""Channels APIs""" | |||
# pylint: disable=R0904 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you change this to the written out version for clarity?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I mean change to disable=too-many-public-methods
channels/api.py
Outdated
channel = self.get_channel(channel_name) | ||
if moderator_name not in channel.moderator(): | ||
channel.moderator.add(user) | ||
Api(user).accept_invite(channel_name) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks like an error is raised if there is no invite already. Can you wrap this in a try: except APIException:
block and log the exception if one happens?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great 👍
What are the relevant tickets?
Fix #128
What's this PR do?
Adds REST API to add and remove moderators in a channel.
How should this be manually tested?
Login as a moderator, create a user and make sure that the user exists on reddit, try adding and removing this user as a moderator.