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 feature to start worker and notify host on challenge approval. #2807

Merged
merged 32 commits into from
Jun 28, 2020

Conversation

KhalidRmb
Copy link
Member

@KhalidRmb KhalidRmb commented May 22, 2020

Pending:

  • - Creating email template on Sendgrid.
  • - Fill in template details in backend.

apps/challenges/challenge_notification_util.py Outdated Show resolved Hide resolved
apps/challenges/challenge_notification_util.py Outdated Show resolved Hide resolved
apps/challenges/challenge_notification_util.py Outdated Show resolved Hide resolved
@codecov-commenter
Copy link

codecov-commenter commented May 27, 2020

Codecov Report

Merging #2807 into master will increase coverage by 2.82%.
The diff coverage is 28.57%.

@@            Coverage Diff             @@
##           master    #2807      +/-   ##
==========================================
+ 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 Δ
...ontend/src/js/controllers/featuredChallengeCtrl.js 68.99% <ø> (ø)
frontend/src/js/controllers/challengeCtrl.js 68.46% <22.22%> (-5.24%) ⬇️
frontend/src/js/controllers/permissionCtrl.js 36.36% <22.22%> (-63.64%) ⬇️
frontend/src/js/controllers/authCtrl.js 65.90% <25.00%> (-0.96%) ⬇️
frontend/src/js/controllers/challengeListCtrl.js 94.68% <50.00%> (ø)
...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/changePwdCtrl.js 100.00% <100.00%> (ø)
frontend/src/js/controllers/dashCtrl.js 99.00% <100.00%> (+0.07%) ⬆️
... and 38 more
Impacted Files Coverage Δ
...ontend/src/js/controllers/featuredChallengeCtrl.js 68.99% <ø> (ø)
frontend/src/js/controllers/challengeCtrl.js 68.46% <22.22%> (-5.24%) ⬇️
frontend/src/js/controllers/permissionCtrl.js 36.36% <22.22%> (-63.64%) ⬇️
frontend/src/js/controllers/authCtrl.js 65.90% <25.00%> (-0.96%) ⬇️
frontend/src/js/controllers/challengeListCtrl.js 94.68% <50.00%> (ø)
...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/changePwdCtrl.js 100.00% <100.00%> (ø)
frontend/src/js/controllers/dashCtrl.js 99.00% <100.00%> (+0.07%) ⬆️
... and 38 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 f75f559...a75bc75. Read the comment docs.

@deshraj deshraj requested a review from Ram81 May 31, 2020 04:44
@deshraj
Copy link
Member

deshraj commented May 31, 2020

Generally looks good to me. @Ram81 lets get this merged if your comments are incorporated.

@Ram81
Copy link
Member

Ram81 commented May 31, 2020

@KhalidRmb for now we can use count instead of success flags. You can make changes for the API in a new PR.

@RishabhJain2018
Copy link
Member

Hey @Ram81 Can you please test this PR?

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.

@KhalidRmb Minor change. Let me know once you verify the case for notification signal for the unapproval case too. Rest of it LGTM

apps/challenges/challenge_notification_util.py Outdated Show resolved Hide resolved
@KhalidRmb
Copy link
Member Author

@KhalidRmb Minor change. Let me know once you verify the case for notification signal for the unapproval case too. Rest of it LGTM

Done.

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.

LGTM 👍 . Can you just share a screenshot of email template for any challenge. I didn't have aws dev & sendgrid credentials so couldn't test it.

Copy link
Member

@RishabhJain2018 RishabhJain2018 left a comment

Choose a reason for hiding this comment

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

@RishabhJain2018
Copy link
Member

@KhalidRmb Can you please fix the travis build here?

@KhalidRmb
Copy link
Member Author

@KhalidRmb Can you please fix the travis build here?

Done!

@RishabhJain2018
Copy link
Member

@KhalidRmb Can you please resolve the conflicts here.

@KhalidRmb
Copy link
Member Author

@KhalidRmb Can you please resolve the conflicts here.

Done,

apps/challenges/models.py Outdated Show resolved Hide resolved
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.

minor change. Rest of it LGTM 👍

@RishabhJain2018
Copy link
Member

@KhalidRmb Please resolve the conflicts here.

@RishabhJain2018
Copy link
Member

@KhalidRmb Please fix the build here.

@RishabhJain2018 RishabhJain2018 merged commit 3a4d2cc into Cloud-CV:master Jun 28, 2020
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.

5 participants