Skip to content

Conversation

Kajol-Kumari
Copy link
Member

@Kajol-Kumari Kajol-Kumari commented May 9, 2020

fix: #2785

  • Added unique url using slug for All_submissions of a particular phase.

  • Add unique url for get submissions under my_submissions tab

  • Add the required tests.

GIF for feature added:
ezgif com-video-to-gif (1)

@Kajol-Kumari
Copy link
Member Author

Kajol-Kumari commented May 9, 2020

@RishabhJain2018 can u please let me know if it's OK.

@RishabhJain2018 RishabhJain2018 requested a review from Sanji515 May 12, 2020 04:57
@Sanji515
Copy link
Member

@Kajol-Kumari can you please fix the build here?

@codecov-io
Copy link

codecov-io commented May 12, 2020

Codecov Report

Merging #2786 into master will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master    #2786   +/-   ##
=======================================
  Coverage   72.51%   72.51%           
=======================================
  Files          85       85           
  Lines        5513     5513           
=======================================
  Hits         3998     3998           
  Misses       1515     1515           

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 6238d29...6238d29. Read the comment docs.

),
url(
r"^challenge/(?P<challenge_pk>[0-9]+)/challenge_phase/(?P<challenge_phase_pk>[0-9]+)/count$",
r"^challenge/(?P<challenge_pk>[0-9]+)/challenge_phase/(?P<version>(v1|v2))/(?P<challenge_phase_pk_or_slug>[-a-zA-Z0-9_]+)/count$",
Copy link
Member

Choose a reason for hiding this comment

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

Hey are we required to do this API also using slug in this PR?

Copy link
Member Author

Choose a reason for hiding this comment

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

yes, as in vm.getResults() we are calling both submissions as well as count. So, both will be called using the slug only.

vm.isResult = true;
if (phaseId !== undefined) {
vm.phaseId = phaseId;
vm.mySubmissionPhaseSlug = phaseId;
Copy link
Member

Choose a reason for hiding this comment

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

@Kajol-Kumari I think you have to work a liitle bit more here as now from the function getResults, its argument coming as slug not phase id in variable phaseId so you need to stop setting to controller variable vm.phaseId in line 813 and also make sure that it is not breaking other functionalities where we might not able to get vm.phaseId after the removal of line 813.

@codecov-commenter
Copy link

Codecov Report

Merging #2786 into master will increase coverage by 2.82%.
The diff coverage is 30.43%.

@@            Coverage Diff             @@
##           master    #2786      +/-   ##
==========================================
+ Coverage   72.93%   75.75%   +2.82%     
==========================================
  Files          83       20      -63     
  Lines        5368     2924    -2444     
==========================================
- Hits         3915     2215    -1700     
+ Misses       1453      709     -744     
Impacted Files Coverage Δ
frontend/src/js/controllers/permissionCtrl.js 36.36% <22.22%> (-63.64%) ⬇️
frontend/src/js/controllers/challengeCtrl.js 68.47% <28.66%> (-5.23%) ⬇️
...ntend/src/js/controllers/challengeHostTeamsCtrl.js 70.50% <66.66%> (-1.18%) ⬇️
frontend/src/js/controllers/teamsCtrl.js 71.17% <66.66%> (ø)
frontend/src/js/controllers/dashCtrl.js 98.93% <100.00%> (+0.01%) ⬆️
apps/hosts/serializers.py
apps/web/views.py
apps/web/serializers.py
scripts/workers/submission_worker.py
... and 42 more
Impacted Files Coverage Δ
frontend/src/js/controllers/permissionCtrl.js 36.36% <22.22%> (-63.64%) ⬇️
frontend/src/js/controllers/challengeCtrl.js 68.47% <28.66%> (-5.23%) ⬇️
...ntend/src/js/controllers/challengeHostTeamsCtrl.js 70.50% <66.66%> (-1.18%) ⬇️
frontend/src/js/controllers/teamsCtrl.js 71.17% <66.66%> (ø)
frontend/src/js/controllers/dashCtrl.js 98.93% <100.00%> (+0.01%) ⬆️
apps/hosts/serializers.py
apps/web/views.py
apps/web/serializers.py
scripts/workers/submission_worker.py
... and 42 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a2d1a19...cd09e9f. Read the comment docs.

@RishabhJain2018 RishabhJain2018 self-requested a review June 21, 2020 17:07
Copy link
Member

@Ram81 Ram81 left a comment

Choose a reason for hiding this comment

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

@Kajol-Kumari instead of passing one field for both slug and phase pk can't you just pass 2 separate fields slug & challenge_phase_pk as request param and use one of those based on the API being called on the backend side. That way would be much cleaner to write the backend code. Let me know if you need any help with that.

return Response(response_data, status=status.HTTP_404_NOT_FOUND)
else:
try:
challenge_phase = ChallengePhase.objects.get(
Copy link
Member

Choose a reason for hiding this comment

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

@Kajol-Kumari you can use get_challenge_phase_model here right?

return Response(response_data, status=status.HTTP_404_NOT_FOUND)
else:
try:
challenge_phase = ChallengePhase.objects.get(
Copy link
Member

Choose a reason for hiding this comment

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

same here?

return Response(response_data, status=status.HTTP_400_BAD_REQUEST)
else:
try:
challenge_phase = ChallengePhase.objects.get(
Copy link
Member

Choose a reason for hiding this comment

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

same here?

@RishabhJain2018
Copy link
Member

@Ram81 Can you please review this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Create unique URL for each phase under My Submission and All Submissions Page
6 participants