Skip to content

MA-1985 add analytics event to thread/comment vote #11450

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.

Sign up for GitHub

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

Merged
wajeeha-khalid merged 3 commits into from
Feb 12, 2016
Merged

MA-1985 add analytics event to thread/comment vote #11450

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
fe9b1a0
add analytics event to thread/comment vote
wajeeha-khalid Feb 4, 2016
3312eb2
refactored code into exclusive functions
wajeeha-khalid Feb 10, 2016
72aeb2c
implemented test event data for thread/comment vote
wajeeha-khalid Feb 11, 2016
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
69 changes: 46 additions & 23 deletions lms/djangoapps/discussion_api/api.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,11 @@
get_initializable_thread_fields,
)
from discussion_api.serializers import CommentSerializer, ThreadSerializer, get_context
from django_comment_client.base.views import track_comment_created_event, track_thread_created_event
from django_comment_client.base.views import (
track_comment_created_event,
track_thread_created_event,
track_voted_event,
)
from django_comment_common.signals import (
thread_created,
thread_edited,
Expand Down Expand Up @@ -496,7 +500,7 @@ def _check_editable_fields(cc_content, data, context):
)


def _do_extra_actions(api_content, cc_content, request_fields, actions_form, context):
def _do_extra_actions(api_content, cc_content, request_fields, actions_form, context, request):

@jcdyer jcdyer Feb 8, 2016

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This function is getting unweildy. Consider extracting methods from it that describe what it does. Something like:

_handle_following_field 
_handle_abuse_flagged_field
_handle_voted_field

Also, the else: assert field == 'voted' bit is weird. That should probably be an elif field == 'voted' with a new else block that raises a more appropriate exception (maybe a KeyError).

@wajeeha-khalid wajeeha-khalid Feb 10, 2016

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jcdyer i wonder why the assert is there or why should we put any keyerror while this function only does extra actions required after a thread/comment is created or updated and all the field validations are already made before entring to this function. So i would like to remove that on whole. Does it make sense?

@symbolist symbolist Feb 11, 2016

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@wajeeha-khalid I am not familiar with this code but generally speaking the purpose would be to ensure that any callers of this function should know if they accidentally pass in an action that this function does not handle. This pattern is commonly used in logic which does some kind of dispatch.

@symbolist symbolist Feb 11, 2016

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Lets retain that existing behaviour by replacing the assert with a KeyError.

"""
Perform any necessary additional actions related to content creation or
update that require a separate comments service request.
Expand All @@ -505,25 +509,44 @@ def _do_extra_actions(api_content, cc_content, request_fields, actions_form, con
if field in request_fields and form_value != api_content[field]:
api_content[field] = form_value
if field == "following":
if form_value:
context["cc_requester"].follow(cc_content)
else:
context["cc_requester"].unfollow(cc_content)
_handle_following_field(form_value, context["cc_requester"], cc_content)
elif field == "abuse_flagged":
if form_value:
cc_content.flagAbuse(context["cc_requester"], cc_content)
else:
cc_content.unFlagAbuse(context["cc_requester"], cc_content, removeAll=False)
_handle_abuse_flagged_field(form_value, context["cc_requester"], cc_content)
elif field == "voted":
_handle_voted_field(form_value, cc_content, api_content, request, context)
else:
assert field == "voted"
signal = thread_voted if cc_content.type == 'thread' else comment_voted
signal.send(sender=None, user=context["request"].user, post=cc_content)
if form_value:
context["cc_requester"].vote(cc_content, "up")
api_content["vote_count"] += 1
else:
context["cc_requester"].unvote(cc_content)
api_content["vote_count"] -= 1
raise ValidationError({field: ["Invalid Key"]})


def _handle_following_field(form_value, user, cc_content):
"""follow/unfollow thread for the user"""
if form_value:
user.follow(cc_content)
else:
user.unfollow(cc_content)


def _handle_abuse_flagged_field(form_value, user, cc_content):
"""mark or unmark thread/comment as abused"""
if form_value:
cc_content.flagAbuse(user, cc_content)
else:
cc_content.unFlagAbuse(user, cc_content, removeAll=False)


def _handle_voted_field(form_value, cc_content, api_content, request, context):
"""vote or undo vote on thread/comment"""
signal = thread_voted if cc_content.type == 'thread' else comment_voted
signal.send(sender=None, user=context["request"].user, post=cc_content)
if form_value:
context["cc_requester"].vote(cc_content, "up")
api_content["vote_count"] += 1
else:
context["cc_requester"].unvote(cc_content)
api_content["vote_count"] -= 1
track_voted_event(
request, context["course"], cc_content, vote_value="up", undo_vote=False if form_value else True
)


def create_thread(request, thread_data):
Expand Down Expand Up @@ -568,7 +591,7 @@ def create_thread(request, thread_data):
cc_thread = serializer.instance
thread_created.send(sender=None, user=user, post=cc_thread)
api_thread = serializer.data
_do_extra_actions(api_thread, cc_thread, thread_data.keys(), actions_form, context)
_do_extra_actions(api_thread, cc_thread, thread_data.keys(), actions_form, context, request)

track_thread_created_event(request, course, cc_thread, actions_form.cleaned_data["following"])

Expand Down Expand Up @@ -609,7 +632,7 @@ def create_comment(request, comment_data):
cc_comment = serializer.instance
comment_created.send(sender=None, user=request.user, post=cc_comment)
api_comment = serializer.data
_do_extra_actions(api_comment, cc_comment, comment_data.keys(), actions_form, context)
_do_extra_actions(api_comment, cc_comment, comment_data.keys(), actions_form, context, request)

track_comment_created_event(request, context["course"], cc_comment, cc_thread["commentable_id"], followed=False)

Expand Down Expand Up @@ -646,7 +669,7 @@ def update_thread(request, thread_id, update_data):
# signal to update Teams when a user edits a thread
thread_edited.send(sender=None, user=request.user, post=cc_thread)
api_thread = serializer.data
_do_extra_actions(api_thread, cc_thread, update_data.keys(), actions_form, context)
_do_extra_actions(api_thread, cc_thread, update_data.keys(), actions_form, context, request)
return api_thread


Expand Down Expand Up @@ -690,7 +713,7 @@ def update_comment(request, comment_id, update_data):
serializer.save()
comment_edited.send(sender=None, user=request.user, post=cc_comment)
api_comment = serializer.data
_do_extra_actions(api_comment, cc_comment, update_data.keys(), actions_form, context)
_do_extra_actions(api_comment, cc_comment, update_data.keys(), actions_form, context, request)
return api_comment


Expand Down
39 changes: 37 additions & 2 deletions lms/djangoapps/discussion_api/tests/test_api.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2107,7 +2107,8 @@ def test_following(self, old_following, new_following):

@ddt.data(*itertools.product([True, False], [True, False]))
@ddt.unpack
def test_voted(self, current_vote_status, new_vote_status):
@mock.patch("eventtracking.tracker.emit")
def test_voted(self, current_vote_status, new_vote_status, mock_emit):
"""
Test attempts to edit the "voted" field.

Expand Down Expand Up @@ -2144,6 +2145,22 @@ def test_voted(self, current_vote_status, new_vote_status):
expected_request_data["value"] = ["up"]
self.assertEqual(actual_request_data, expected_request_data)

event_name, event_data = mock_emit.call_args[0]
self.assertEqual(event_name, "edx.forum.thread.voted")
self.assertEqual(
event_data,
{
'undo_vote': not new_vote_status,
'url': '',
'target_username': self.user.username,
'vote_value': 'up',
'user_forums_roles': [FORUM_ROLE_STUDENT],
'user_course_roles': [],
'commentable_id': 'original_topic',
'id': 'test_thread'
}
)

@ddt.data(*itertools.product([True, False], [True, False], [True, False]))
@ddt.unpack
def test_vote_count(self, current_vote_status, first_vote, second_vote):
Expand Down Expand Up @@ -2499,7 +2516,8 @@ def test_endorsed_access(self, role_name, is_thread_author, thread_type, is_comm

@ddt.data(*itertools.product([True, False], [True, False]))
@ddt.unpack
def test_voted(self, current_vote_status, new_vote_status):
@mock.patch("eventtracking.tracker.emit")
def test_voted(self, current_vote_status, new_vote_status, mock_emit):
"""
Test attempts to edit the "voted" field.

Expand Down Expand Up @@ -2539,6 +2557,23 @@ def test_voted(self, current_vote_status, new_vote_status):
expected_request_data["value"] = ["up"]
self.assertEqual(actual_request_data, expected_request_data)

event_name, event_data = mock_emit.call_args[0]
self.assertEqual(event_name, "edx.forum.response.voted")

self.assertEqual(
event_data,
{
'undo_vote': not new_vote_status,
'url': '',
'target_username': self.user.username,
'vote_value': 'up',
'user_forums_roles': [FORUM_ROLE_STUDENT],
'user_course_roles': [],
'commentable_id': 'dummy',
'id': 'test_comment'
}
)

@ddt.data(*itertools.product([True, False], [True, False], [True, False]))
@ddt.unpack
def test_vote_count(self, current_vote_status, first_vote, second_vote):
Expand Down
1 change: 1 addition & 0 deletions lms/djangoapps/discussion_api/tests/utils.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -358,6 +358,7 @@ def make_minimal_cs_comment(overrides=None):
ret = {
"type": "comment",
"id": "dummy",
"commentable_id": "dummy",
"thread_id": "dummy",
"parent_id": None,
"user_id": "0",
Expand Down