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

deshraj · 2017-01-10T06:43:13Z

No description provided.

dexter1691 · 2017-01-10T07:15:33Z

apps/jobs/models.py

@@ -36,8 +40,10 @@ class Submission(TimeStampedModel):
        (FINISHED, FINISHED),
    )

-    participant_team = models.ForeignKey(ParticipantTeam, related_name='submissions')
-    challenge_phase = models.ForeignKey(ChallengePhase, related_name='submissions')
+    participant_team = models.ForeignKey(


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.

dexter1691 · 2017-01-10T07:17:33Z

apps/jobs/urls.py


 urlpatterns = [
+    url(r'challenge/(?P<challange_id>[0-9]+)/'
+        r'challenge_phase/(?P<challange_phase_id>[0-9]+)/',


Shouldn't this be appended with a submission end_point?

How do you update / delete / get the submission given submission id

Yeah, we will add that functionality soon. This PR mainly focuses on creating the Submission only since this is a major feature.

Okay, but it still feels odd to not have a submission end_point appended after challenge_phase_id. The post request should be sent to /challenge/<challenge_id>/challenge_phase<phase_id>/submission right?

So, since it is in the jobs app, so the url would be /api/jobs/challenge/<challenge_id>/challenge_phase<phase_id>/submission. Does that sound good?

dexter1691 · 2017-01-10T07:23:19Z

apps/jobs/views.py

+
+from challenges.models import (
+    ChallengePhase,
+    Challenge,)


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

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

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

Sure, will look into it.

dexter1691 · 2017-01-10T07:29:57Z

apps/participants/utils.py

+def get_participant_team_id_of_a_user_for_a_challenge(user, challenge_id):
+    """Returns the participant team object for a particular user for a particular challenge"""
+    participant_teams = Participant.object.filter(user=user).values_list('teams', flat=True)
+    for participant_team in participant_teams:


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

Yup, I am figuring that out.

dexter1691 · 2017-01-10T07:34:20Z

apps/participants/utils.py

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


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.

dexter1691 · 2017-01-10T07:36:28Z

I am assuming this is gonna handle only the file upload part! actual update of the submission status will be done through channels?

dexter1691 · 2017-01-10T07:38:20Z

apps/jobs/models.py

+                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'
+                    raise PermissionDenied(


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.

I am not sure what kind of exception we can get there? Can you please elaborate 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.

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

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.

dexter1691 · 2017-01-10T07:41:45Z

apps/jobs/models.py

+                participant_team=self.participant_team,
+                status=Submission.FAILED).count()
+
+            offset_submission_count = self.submission_number - failed_count


can we rename this to successful_count?
Also we are going to use success_count more, so might be better to explicitly save that and not save failed_count?

So, we are not storing the successful or failure counts since we can calculate that by seeing the status of the submissions made by the user.

taranjeet · 2017-01-10T07:08:32Z

apps/jobs/admin.py

+
+
+@admin.register(Submission)
+class SubmissioneAdmin(TimeStampedAdmin):


typo, SubmissioneAdmin --> SubmissionAdmin

taranjeet · 2017-01-10T07:09:38Z

apps/jobs/admin.py

+                    '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',


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

taranjeet · 2017-01-10T07:22:37Z

apps/jobs/models.py

 from django.db import models
+from django.db.models import Max


can you please order the import alphabetically ?

taranjeet · 2017-01-10T07:31:35Z

apps/jobs/models.py

+            offset_submission_count = self.submission_number - failed_count
+
+            if (offset_submission_count > self.challenge_phase.max_submissions):
+                print "Checking to see if the offset_submission_count (%d) ", \


this print statement can be more meaningful if we log additional details like submission id, participant team id or phase id. can you please add accordingly here and for the else block also

taranjeet · 2017-01-10T07:42:07Z

apps/jobs/models.py

+
+            self.status = Submission.SUBMITTED
+
+        saved_model = super(Submission, self).save(*args, **kwargs)


i think this variable name should be renamed to saved_model_instance. It actually makes more sense.

taranjeet · 2017-01-10T09:00:21Z

apps/jobs/views.py

+
+from accounts.permissions import HasVerifiedEmail
+
+from participants.utils import (


can you please reorder imports alphabetically ?

taranjeet · 2017-01-10T09:01:10Z

apps/jobs/views.py

+    # check if the challenge exists or not
+    try:
+        challenge = Challenge.objects.get(pk=challenge_id)
+        if not challenge.is_active():


what if take is_active check out of try-catch

taranjeet · 2017-01-10T09:11:45Z

apps/jobs/views.py

+        return Response(response_data, status=status.HTTP_401_UNAUTHORIZED)
+
+    # check if challenge is public and accepting solutions
+    if not challenge.is_public:


why querying for phase, when challenge is not active, should be after challenge.is_public check

taranjeet · 2017-01-10T09:41:41Z

apps/jobs/models.py

+
+                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'


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?

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

this is still pending.

taranjeet · 2017-01-10T09:43:35Z

apps/jobs/models.py

+                        (self.challenge_phase.max_submissions_per_day == 0)):
+                    print 'PERMISSION DENIED'
+                    raise PermissionDenied(
+                        "The maximum number of submissions this day have been reached.")


I think it should be The maximum number of submissions that can be made today has reached or The maximum number of submission for today has been reached.

taranjeet · 2017-01-10T09:51:33Z

apps/jobs/models.py

+                    print 'PERMISSION DENIED'
+                    raise PermissionDenied(
+                        "The maximum number of submissions this day have been reached.")
+            else:


I really didn't get this else here. What is it for? Can you explain me more clearly?

I just realized that it's not required here according to our use case. I will remove it.

dexter1691 · 2017-01-11T04:40:02Z

apps/jobs/models.py

+                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'
+                    raise PermissionDenied(


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.

deshraj · 2017-01-11T13:38:44Z

@trojan can you please review the PR again? I have added the tests.

taranjeet · 2017-01-10T12:10:38Z

apps/jobs/models.py

+            successful_count = self.submission_number - failed_count
+
+            if (successful_count > self.challenge_phase.max_submissions):
+                print "Checking to see if the successful_count (%d) ", \


the print message is fine, but what if we write it like this(suggestion)

print 'Submission request is submitted by user {user_id} from participant team {} with successful count {} for challenge phase {} with max submission {}'.format(user_id=, ....)

This way it will be easier for us to grep from logs in case of error.

taranjeet · 2017-01-10T12:13:25Z

apps/jobs/models.py

+                raise PermissionDenied(
+                    "The maximum number of submissions has been reached.")
+            else:
+                print "Submission is below for user (%d) form participant_team (%d) for challenge_phase (%d)" % (


i guess if we print the above message, then this print is absolutely not required.

taranjeet · 2017-01-10T12:22:34Z

apps/jobs/views.py

+    try:
+        challenge = Challenge.objects.get(pk=challenge_id)
+    except Challenge.DoesNotExist:
+        response_data = {'error': 'Challenge does not exist!'}


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

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

taranjeet · 2017-01-11T17:33:48Z

apps/jobs/models.py

+            successful_count = self.submission_number - failed_count
+
+            if successful_count > self.challenge_phase.max_submissions:
+                print "Checking to see if the successful_count {0} ", \


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.

taranjeet · 2017-01-11T17:35:02Z

apps/jobs/models.py

+
+                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'


this is still pending.

taranjeet · 2017-01-11T17:38:56Z

apps/jobs/views.py

+    if serializer.is_valid():
+        serializer.save()
+        response_data = serializer.data
+        print response_data


i think this print statement should be not there.

taranjeet · 2017-01-11T17:39:46Z

apps/jobs/views.py

+    try:
+        participant_team = ParticipantTeam.objects.get(pk=participant_team_id)
+    except ParticipantTeam.DoesNotExist:
+        response_data = {'error': 'You haven\'t participated in the challenge'}


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

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

taranjeet · 2017-01-11T17:44:23Z

tests/unit/jobs/test_views.py

+from django.contrib.auth.models import User
+from django.utils import timezone
+
+


this is a extra newline.

taranjeet · 2017-01-11T19:54:56Z

apps/participants/utils.py

+    return Challenge.objects.filter(pk=challenge_id, participant_teams__in=participant_teams).exists()
+
+
+def get_participant_team_id_of_user_for_a_challenge(user, challenge_id):


see if this helps here ?

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.

deshraj · 2017-01-13T00:58:44Z

@trojan can you please check and merge it so that we can start integrating the worker?

taranjeet · 2017-01-13T05:41:32Z

@deshraj : looks good, can be merged now

…#396) * Challenges: property methods added to Challenge and ChallengePhase Model * Docstring added to each method * Tests added for Challenge model * More test cases added * Jobs: Added API endpoint for making submission to a Challenge Phase * Minor fixes incorporating the changes suggested * API endpoint for submission ready * Unused imports removed * Minor additions in the business logic * Tests added for Challenge Submission API * Unused imports removed from jobs app * Removed unused modules from tests * Removed unused modules from views * Logging statments added for jobs models * Tests modified for Jobs app

deshraj added 5 commits January 8, 2017 20:04

Challenges: property methods added to Challenge and ChallengePhase Model

d95d2c7

Docstring added to each method

0b6c965

Tests added for Challenge model

30b21f7

More test cases added

9abf0ad

Jobs: Added API endpoint for making submission to a Challenge Phase

bc379b1

deshraj requested review from dexter1691 and taranjeet January 10, 2017 06:43

Branch updated with the master branch

b33f640

deshraj changed the title ~~Jobs: Added API endpoint for making submission to a Challenge Phase~~ [In-Progress] Jobs: Added API endpoint for making submission to a Challenge Phase Jan 10, 2017

dexter1691 requested changes Jan 10, 2017

View reviewed changes

dexter1691 reviewed Jan 10, 2017

View reviewed changes

taranjeet reviewed Jan 10, 2017

View reviewed changes

deshraj added 4 commits January 10, 2017 04:58

Minor fixes incorporating the changes suggested

c6fdaf6

Branch updated with master

1e3120d

API endpoint for submission ready

f9f17a3

Unused imports removed

d643578

dexter1691 approved these changes Jan 11, 2017

View reviewed changes

deshraj added 3 commits January 11, 2017 05:46

Branch updated with master

b8ff4c3

Minor additions in the business logic

ffec7dd

Tests added for Challenge Submission API

739c880

deshraj changed the title ~~[In-Progress] Jobs: Added API endpoint for making submission to a Challenge Phase~~ Jobs: Added API endpoint for making submission to a Challenge Phase Jan 11, 2017

deshraj changed the title ~~Jobs: Added API endpoint for making submission to a Challenge Phase~~ Jobs: Added API endpoint for creating submission to a Challenge Phase Jan 11, 2017

deshraj and others added 4 commits January 11, 2017 08:41

Unused imports removed from jobs app

114d4d8

Merge branch 'master' into submission

ad83504

Removed unused modules from tests

7592a2f

Removed unused modules from views

084fbdf

taranjeet reviewed Jan 11, 2017

View reviewed changes

deshraj added 3 commits January 12, 2017 01:14

Logging statments added for jobs models

3f5f60f

Merge conflict resolved

77200e3

Tests modified for Jobs app

ab28b43

Merge branch 'master' into submission

0c1c86b

deshraj merged commit 0bf97ba into master Jan 13, 2017

deshraj deleted the submission branch January 13, 2017 05:44

deshraj mentioned this pull request Jan 13, 2017

Create the job submission API #387

Closed



		@admin.register(Submission)
		class SubmissioneAdmin(TimeStampedAdmin):

		from django.db import models
		from django.db.models import Max


		self.status = Submission.SUBMITTED

		saved_model = super(Submission, self).save(args, *kwargs)


		from accounts.permissions import HasVerifiedEmail

		from participants.utils import (

		from django.contrib.auth.models import User
		from django.utils import timezone

		return Challenge.objects.filter(pk=challenge_id, participant_teams__in=participant_teams).exists()


		def get_participant_team_id_of_user_for_a_challenge(user, challenge_id):

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

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

Conversation

deshraj commented Jan 10, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dexter1691 Jan 10, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dexter1691 commented Jan 10, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

deshraj commented Jan 11, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

deshraj commented Jan 13, 2017 • edited

taranjeet commented Jan 13, 2017

dexter1691 Jan 10, 2017 •

edited

deshraj commented Jan 13, 2017 •

edited