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

Add meta-review to gamification #190

Closed
wants to merge 1 commit into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
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
5 changes: 5 additions & 0 deletions gamification/data/config.py
Original file line number Diff line number Diff line change
Expand Up @@ -76,6 +76,8 @@ def create_badge_activities():
name='Solved a area/genericbears bear proposal issue'),
BadgeActivity(
name='Solved a area/lintbears bear proposal issue'),
BadgeActivity(
name='Did a meta-review or received a meta-review'),
]
BadgeActivity.objects.bulk_create(b_activity__object_list)

Expand Down Expand Up @@ -241,9 +243,12 @@ def add_activities_to_badges():
name='Solved a difficulty/newcomer issue')
all_rounder_activity2 = BadgeActivity.objects.get(
name='Solved a difficulty/low issue')
all_rounder_activity3 = BadgeActivity.objects.get(
name='Did a meta-review or received a meta-review')
all_rounder_activities_list = [
all_rounder_activity1,
all_rounder_activity2,
all_rounder_activity3,
]
all_rounder_badge.b_activities.add(
*all_rounder_activities_list)
4 changes: 4 additions & 0 deletions gamification/management/commands/update_participants_data.py
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,8 @@
update_participants_data_with_issue,
get_participant_objects,
award_badges,
get_meta_review_participant_objects,
update_participants_data_with_meta_review,
)


Expand All @@ -31,3 +33,5 @@ def handle(self, *args, **options):
update_participants_data_with_issue(issue)
for participant in get_participant_objects():
award_badges(participant)
for participant in get_meta_review_participant_objects():
update_participants_data_with_meta_review(participant.login)
41 changes: 41 additions & 0 deletions gamification/process/update.py
Original file line number Diff line number Diff line change
Expand Up @@ -4,12 +4,14 @@
MergeRequest,
Issue,
)
from data.newcomers import active_newcomers
from gamification.process.activity_points import (
get_activity_with_points
)
from gamification.labels import NEGATIVE_POINT_LABELS
from gamification.data.points import MERGE_REQUEST_CLOSED_WITHOUT_MERGE
from gamification.models import Participant
from meta_review.models import Reaction, Participant as meta_review_participants


def get_mr_objects():
Expand Down Expand Up @@ -135,6 +137,45 @@ def get_participant_objects():
return participants


def get_meta_review_participant_objects():
return meta_review_participants.objects.all()


meta_review_completed_list = []


def update_participants_data_with_meta_review(meta_review_participant):
"""
Update participants based on meta-review

This method updates every participant based on the meta-review
received or given. If the meta-review participant is
in the active newcomers list then it will check if the meta-review
is complete or not and update the activity accordingly.
"""
active_newcomers_list = active_newcomers()
if (meta_review_participant.login in active_newcomers_list and
meta_review_participant.login not in meta_review_completed_list):
# Get the corresponding gamification participant
gamification_participant = Participant.objects.get(
username=meta_review_participant.login)
reaction = Reaction.objects.get(id=meta_review_participant.login)
Copy link
Member

Choose a reason for hiding this comment

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

This is definitely wrong. I checked the output you print out https://pastebin.com/6X6y5Kfx. You have the illusion that this works, because your test is wrong. In your test, both participant and reaction share the same id (primary_key), which gives you the wrong belief that you are correct.

That's why I said you should run .ci/build.sh and show the log to prove this works. You can use that for debugging, but you don't need to prove anymore as I am pretty confident now this is wrong.

Copy link
Member Author

Choose a reason for hiding this comment

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

@li-boxuan Thanks for your valuable input.

Can you please tell me what do I need to do in order to fix this? This thing is taking way much more time than I expected to. 😞

Copy link
Member

Choose a reason for hiding this comment

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

First you need to fix the test case. See #190 (comment) so that your test won't pass (which is good coz now you know your code is wrong).

Then read the definition of Reaction model. Understand the relationships between those models.

Copy link
Member Author

@shikharvaish28 shikharvaish28 Nov 14, 2018

Choose a reason for hiding this comment

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

In order to test things, I used my user handle shikharvaish28. Ideally, I should be able to get reaction using id which is the primary key of Reaction model.

test_meta_review_participant = meta_review_participant.objects.get(login='shikharvaish28')
reaction = Reaction.objects.get(id=test_meta_review_participant.login)

Instead, I am getting DoesNotExist: Reaction matching query does not exist.Can you please tell me why is it so?

Since login is the primary key of Participant, I am expecting it to be linked in someway to id of Reaction. I want to do this because if I am somehow able to get the corresponding participant for Reaction, everything would be sorted.

PS: I'm running these commands in django shell to get a gist of things.

Copy link
Member

Choose a reason for hiding this comment

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

Since login is the primary key of Participant, I am expecting it to be linked in someway to id of Reaction.

Your assumption is wrong. Django does not work like that.

I strongly recommend you to read https://docs.djangoproject.com/en/2.1/topics/db/queries/, esepecially understand how Foreign Key works.

if (meta_review_participant.pos_in > 0 or
meta_review_participant.pos_out > 0 or
meta_review_participant.neg_in > 0 or
meta_review_participant.neg_out > 0):
# Add a meta-review activity to the participant
activity = 'Did a meta-review or received a meta-review'
created_at = reaction.created_at
updated_at = None
gamification_participant.add_activity(0, activity, created_at,
updated_at)
meta_review_completed_list.append(meta_review_participant.login)
logger = logging.getLogger(__name__)
logger.info(meta_review_participant.login,
' has completed the meta-review activity')


def award_badges(participant):
activities = participant.activities.values('name')
participant.add_badges(activities)
10 changes: 5 additions & 5 deletions gamification/tests/test_management_commands.py
Original file line number Diff line number Diff line change
Expand Up @@ -24,10 +24,10 @@ def test_command_create_config_data(self):
self.assertEquals(levels.count(), 8)

b_activities = BadgeActivity.objects.all()
# There must be 26 badge activities created
# as there are 26 badge_activities listed in
# There must be 27 badge activities created
# as there are 27 badge_activities listed in
# file config.py
self.assertEquals(b_activities.count(), 26)
self.assertEquals(b_activities.count(), 27)

badges = Badge.objects.all()
# There must be 8 badges created
Expand Down Expand Up @@ -61,7 +61,7 @@ def test_command_create_config_data(self):

# The All-Rounder badge must have two activities
badge7 = Badge.objects.get(name='The All-Rounder')
self.assertEquals(badge7.b_activities.count(), 2)
self.assertEquals(badge7.b_activities.count(), 3)


class CreateParticipantsTest(TestCase):
Expand Down Expand Up @@ -97,4 +97,4 @@ def test_command_update_particiapants_data(self):
self.assertEquals(current_level, 'Intermediate-II')

number_of_badges = participant.badges.all().count()
self.assertEquals(number_of_badges, 2)
self.assertEquals(number_of_badges, 1)
69 changes: 69 additions & 0 deletions gamification/tests/test_models.py
Original file line number Diff line number Diff line change
@@ -1,5 +1,7 @@
from django.test import TestCase

from data.newcomers import active_newcomers

from gamification.models import (
Activity,
Level,
Expand All @@ -8,6 +10,12 @@
Participant,
)

from gamification.process.update import (
update_participants_data_with_meta_review
)

from meta_review.models import Reaction, Participant as meta_review_participant


class ActivityModelTest(TestCase):

Expand Down Expand Up @@ -341,3 +349,64 @@ def test_add_badges_method(self):

# After applying add_badge method
self.assertEquals(test_participant.badges.count(), 0)

def test_update_participants_data_with_meta_review_complete(self):
# Test the participant who has completed the meta-review activity
active_newcomers_list = active_newcomers()
active_newcomers_list.extend(['test2', 'test3'])
# meta-review participant
meta_review_participant.objects.create(login='test2')
test_meta_review_participant = meta_review_participant.objects.get(
login='test2')
Copy link
Member

Choose a reason for hiding this comment

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

Unnecessary. meta_review_participant.objects.create(login='test2') will return the instance.


Reaction.objects.create(id='test2')
Copy link
Member

Choose a reason for hiding this comment

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

Well, this is definitely inappropriate and prone to error. You are creating a reaction with id also equals to test2, but the participant you created is also with id test2.

reaction = Reaction.objects.get(
id=test_meta_review_participant.login)
# corresponding gamification participant for 'test2'
Participant.objects.create(username='test2')
test_gamification_participant = Participant.objects.get(
username='test2')

test_meta_review_participant.pos_in = 2
reaction.created_at = '2018-10-25 14:30:59'
# Before applying update_participants_data_with_meta_review method
self.assertEquals(test_gamification_participant.activities.count(), 0)
# After applying update_participants_data_with_meta_review method
update_participants_data_with_meta_review(test_meta_review_participant)
self.assertEquals(test_gamification_participant.activities.count(), 1)
self.assertEquals(
test_gamification_participant.activities.all()[0].name,
'Did a meta-review or received a meta-review')
# Check if multiple activities are not adding up for the same
# participant 'test2' already has meta-review added so check
# if activity is not added again
update_participants_data_with_meta_review(test_meta_review_participant)
self.assertEquals(test_gamification_participant.activities.count(), 1)

def test_update_participants_data_with_meta_review_incomplete(self):
# Test the participant who has zero meta-review activity
meta_review_participant.objects.create(login='test3')
test_meta_review_participant = meta_review_participant.objects.get(
login='test3')
Reaction.objects.create(id='test3')
reaction = Reaction.objects.get(
id=test_meta_review_participant.login)
# The corresponding gamification participant
Participant.objects.create(username='test3')
test_gamification_participant = Participant.objects.get(
username='test3')

reaction.created_at = '2018-10-25 14:30:59'
update_participants_data_with_meta_review(test_meta_review_participant)
self.assertEquals(test_gamification_participant.activities.count(), 0)

# if (meta_review_participant.login in active_newcomers_list): is false
meta_review_participant.objects.create(login='test4')
test_meta_review_participant = meta_review_participant.objects.get(
login='test4')
# The corresponding gamification participant for 'test4'
Participant.objects.create(username='test4')
test_gamification_participant = Participant.objects.get(
username='test4')
update_participants_data_with_meta_review(test_meta_review_participant)
self.assertEquals(test_gamification_participant.activities.count(), 0)