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

Jobs: Added API endpoint for creating submission to a Challenge Phase #396

Merged
merged 21 commits into from Jan 13, 2017
Merged
Show file tree
Hide file tree
Changes from 17 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
16 changes: 15 additions & 1 deletion apps/jobs/admin.py
@@ -1 +1,15 @@
# Register your models here.
from django.contrib import admin

from base.admin import TimeStampedAdmin

from .models import Submission


@admin.register(Submission)
class SubmissionAdmin(TimeStampedAdmin):
list_display = ('participant_team', 'challenge_phase', 'created_by', 'status', 'is_public',
'submission_number', 'submitted_at', 'input_file', 'stdout_file', 'stderr_file')
list_filter = ('participant_team', 'challenge_phase',
'created_by', 'status', 'is_public')
search_fields = ('participant_team', 'challenge_phase',
Copy link
Member

Choose a reason for hiding this comment

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

ideally search field should contains fields which are text or integer(if it is very necessary). Can you please update here ?

'created_by', 'status')
66 changes: 63 additions & 3 deletions apps/jobs/models.py
@@ -1,9 +1,13 @@
from __future__ import unicode_literals

import datetime

from os.path import join

from django.db import models
from django.contrib.auth.models import User
from django.core.exceptions import PermissionDenied
from django.db import models
from django.db.models import Max
Copy link
Member

Choose a reason for hiding this comment

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

can you please order the import alphabetically ?


from base.models import (TimeStampedModel, )
from challenges.models import ChallengePhase
Expand Down Expand Up @@ -44,8 +48,10 @@ class Submission(TimeStampedModel):
(SUBMITTING, SUBMITTING),
)

participant_team = models.ForeignKey(ParticipantTeam, related_name='submissions')
challenge_phase = models.ForeignKey(ChallengePhase, related_name='submissions')
participant_team = models.ForeignKey(
Copy link
Member

Choose a reason for hiding this comment

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

can we make this user? That will store the participant team information as well, right?
It will require one extra join though to query submission per team. Might be good to add both then?

Saying this because then we will loose out information regarding which user submitted. Not super relevant info but still.

ParticipantTeam, related_name='submissions')
challenge_phase = models.ForeignKey(
ChallengePhase, related_name='submissions')
created_by = models.ForeignKey(User)
status = models.CharField(max_length=30, choices=STATUS_OPTIONS)
is_public = models.BooleanField(default=False)
Expand All @@ -66,3 +72,57 @@ def __unicode__(self):
class Meta:
app_label = 'jobs'
db_table = 'submission'

def save(self, *args, **kwargs):

if not self.pk:
sub_num = Submission.objects.filter(
challenge_phase=self.challenge_phase,
participant_team=self.participant_team).aggregate(
Max('submission_number'))['submission_number__max']
if sub_num:
self.submission_number = sub_num + 1
else:
self.submission_number = 1

failed_count = Submission.objects.filter(
challenge_phase=self.challenge_phase,
participant_team=self.participant_team,
status=Submission.FAILED).count()

successful_count = self.submission_number - failed_count

if successful_count > self.challenge_phase.max_submissions:
print "Checking to see if the successful_count {0} ", \
Copy link
Member

Choose a reason for hiding this comment

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

can you combine both the print statements. Also I will prefer that log a print statement containing every required detail just above the if statement and then you can remove print in else statement.

" is greater than the maximum allowed {1}".format(
successful_count, self.challenge_phase.max_submissions)

print "The submission request is submitted by user {0}", \
"from participant_team {1} ".format(
self.created_by.pk, self.participant_team.pk)

raise PermissionDenied(
"The maximum number of submissions has been reached.")
else:
print "Submission is below for user {0} form participant_team {1} for challenge_phase {2}".format(
self.created_by.pk, self.participant_team.pk, self.challenge_phase.pk)

if hasattr(self.challenge_phase, 'max_submissions_per_day'):
submissions_done_today_count = Submission.objects.filter(
challenge_phase__challenge=self.challenge_phase.challenge,
participant_team=self.participant_team,
challenge_phase=self.challenge_phase,
submitted_at__gte=datetime.date.today()).count()

if ((submissions_done_today_count + 1 - failed_count > self.challenge_phase.max_submissions_per_day) or
(self.challenge_phase.max_submissions_per_day == 0)):
print 'PERMISSION DENIED'
Copy link
Member

Choose a reason for hiding this comment

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

also this print statement can be made more meaningful, so that it becomes easier for us to debug any issue(in case if it happens).
One more thing which I am curious about is why are we not using python logging module?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah we will replace all the debugging statements with logging once the pr is ready.

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 still pending.

raise PermissionDenied(
Copy link
Member

Choose a reason for hiding this comment

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

we are not handling this exception in the views.py L78 I guess. We should handle this exception gracefully and send a proper error message.

Copy link
Member Author

Choose a reason for hiding this comment

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

I am not sure what kind of exception we can get there? Can you please elaborate more?

Copy link
Member

Choose a reason for hiding this comment

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

If permission is denied then on save, it will throw an error, right? This I guess will call 500 Internal error. We should catch this exception and throw a 401 error saying that max number of submissions have been made.

Copy link
Member Author

Choose a reason for hiding this comment

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

No, it will give a 403 response so we are good to go with this.

Copy link
Member

Choose a reason for hiding this comment

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

Hmm. Ok for now but minor point is the stack trace will miss the actual endpoint that is called. Can you plz add a simple /TODO just so that we remember to check.

"The maximum number of submission for today has been reached")

self.is_public = (True if self.challenge_phase.leaderboard_public else False)

self.status = Submission.SUBMITTED

submission_instance = super(Submission, self).save(*args, **kwargs)
return submission_instance
24 changes: 24 additions & 0 deletions apps/jobs/serializers.py
@@ -0,0 +1,24 @@
from rest_framework import serializers

from .models import Submission


class SubmissionSerializer(serializers.ModelSerializer):

def __init__(self, *args, **kwargs):
context = kwargs.get('context')
if context:
created_by = context.get('request').user
kwargs['data']['created_by'] = created_by.pk

participant_team = context.get('participant_team').pk
kwargs['data']['participant_team'] = participant_team

challenge_phase = context.get('challenge_phase').pk
kwargs['data']['challenge_phase'] = challenge_phase

super(SubmissionSerializer, self).__init__(*args, **kwargs)

class Meta:
model = Submission
fields = ('participant_team', 'challenge_phase', 'created_by', 'status', 'input_file')
7 changes: 6 additions & 1 deletion apps/jobs/urls.py
@@ -1,4 +1,9 @@
from django.conf.urls import url # noqa
from django.conf.urls import url

from . import views

urlpatterns = [
url(r'challenge/(?P<challenge_id>[0-9]+)/'
r'challenge_phase/(?P<challenge_phase_id>[0-9]+)/submission/',
views.challenge_submission, name='challenge_submission'),
]
75 changes: 74 additions & 1 deletion apps/jobs/views.py
@@ -1 +1,74 @@
# Create your views here.
from rest_framework import permissions, status
from rest_framework.decorators import (api_view,
authentication_classes,
permission_classes,
throttle_classes,)

from rest_framework_expiring_authtoken.authentication import (
ExpiringTokenAuthentication,)
from rest_framework.response import Response
from rest_framework.throttling import UserRateThrottle

from accounts.permissions import HasVerifiedEmail
from challenges.models import (
ChallengePhase,
Challenge,)
Copy link
Member

Choose a reason for hiding this comment

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

comma at the end of a tuple seems inconsistent in the whole file

Copy link
Member Author

Choose a reason for hiding this comment

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

This is the convention that we should ideally follow and we have been maintaining this throughout the project.

Copy link
Member

Choose a reason for hiding this comment

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

Not in this file though. There are a couple of places where there is no comma. Right?

Copy link
Member Author

Choose a reason for hiding this comment

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

Sure, will look into it.

from participants.models import (ParticipantTeam,)
from participants.utils import (
get_participant_team_id_of_user_for_a_challenge,)

from .serializers import SubmissionSerializer


@throttle_classes([UserRateThrottle])
@api_view(['POST'])
@permission_classes((permissions.IsAuthenticated, HasVerifiedEmail))
@authentication_classes((ExpiringTokenAuthentication,))
def challenge_submission(request, challenge_id, challenge_phase_id):
"""API Endpoint for making a submission to a challenge"""

# check if the challenge exists or not
try:
challenge = Challenge.objects.get(pk=challenge_id)
except Challenge.DoesNotExist:
response_data = {'error': 'Challenge does not exist!'}
Copy link
Member

Choose a reason for hiding this comment

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

we should enforce a standard here, some messages have an exclamation at the end, where as others dont.
just a suggestion.

Copy link
Member Author

Choose a reason for hiding this comment

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

Lets remove the exclamation mark. We have been following that convention so far.

return Response(response_data, status=status.HTTP_400_BAD_REQUEST)

# check if the challenge is active or not
if not challenge.is_active:
response_data = {'error': 'Challenge is not active!'}
return Response(response_data, status=status.HTTP_406_NOT_ACCEPTABLE)

# check if the challenge phase exists or not
try:
challenge_phase = ChallengePhase.objects.get(
pk=challenge_phase_id, challenge=challenge)
except ChallengePhase.DoesNotExist:
response_data = {'error': 'Challenge Phase does not exist!'}
return Response(response_data, status=status.HTTP_400_BAD_REQUEST)

# check if challenge phase is public and accepting solutions
if not challenge_phase.is_public:
response_data = {
'error': 'Sorry, cannot accept submissions since challenge phase is not public!'}
return Response(response_data, status=status.HTTP_406_NOT_ACCEPTABLE)

participant_team_id = get_participant_team_id_of_user_for_a_challenge(
request.user, challenge_id)
try:
participant_team = ParticipantTeam.objects.get(pk=participant_team_id)
except ParticipantTeam.DoesNotExist:
response_data = {'error': 'You haven\'t participated in the challenge'}
Copy link
Member

Choose a reason for hiding this comment

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

we can write this string in double quotes, then we wont have to escape t

Copy link
Member Author

Choose a reason for hiding this comment

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

I have used single quotes since we have been following this convention so far.

return Response(response_data, status=status.HTTP_403_FORBIDDEN)

serializer = SubmissionSerializer(data=request.data,
context={'participant_team': participant_team,
'challenge_phase': challenge_phase,
'request': request
})
if serializer.is_valid():
serializer.save()
response_data = serializer.data
print response_data
Copy link
Member

Choose a reason for hiding this comment

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

i think this print statement should be not there.

return Response(response_data, status=status.HTTP_201_CREATED)
return Response(serializer.errors, status=status.HTTP_400_BAD_REQUEST)
28 changes: 28 additions & 0 deletions apps/participants/utils.py
@@ -0,0 +1,28 @@
from challenges.models import Challenge

from .models import Participant


def is_user_part_of_participant_team(user, participant_team_id):
"""Returns boolean if the user belongs to the participant team or not"""
return Participant.objects.filter(user=user, team__id=participant_team_id).exists()


def has_participant_team_participated_in_challenge(participant_team_id, challenge_id):
"""Returns boolean if the Participant Team participated in particular Challenge"""
return Challenge.objects.filter(pk=challenge_id, participant_team__id=participant_team_id).exists()


def has_user_participated_in_challenge(user, challenge_id):
"""Returns boolean if the user has participated in a particular challenge"""
participant_teams = Participant.objects.filter(user=user).values_list('teams', flat=True)
return Challenge.objects.filter(pk=challenge_id, participant_teams__in=participant_teams).exists()
Copy link
Member

Choose a reason for hiding this comment

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

This should essentially use the information from get_participant_team_id_of_a_user_for_a_challenge function.
If participant_team is provided, use the info otherwise call the function to get that info based on user.
Should also rename these functions to make them concise.



def get_participant_team_id_of_user_for_a_challenge(user, challenge_id):
Copy link
Member

Choose a reason for hiding this comment

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

see if this helps here ?

Copy link
Member Author

Choose a reason for hiding this comment

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

I am pretty sure that there is a better way of doing this. Will look into it. For now, I think we are good with this.

"""Returns the participant team object for a particular user for a particular challenge"""
participant_teams = Participant.objects.filter(user=user).values_list('team', flat=True)
for participant_team in participant_teams:
Copy link
Member

Choose a reason for hiding this comment

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

There should be a better way to do this using joins.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yup, I am figuring that out.

if Challenge.objects.filter(pk=challenge_id, participant_teams=participant_team).exists():
return participant_team
return None
Empty file added tests/unit/jobs/__init__.py
Empty file.