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.

Sign up for GitHub

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

Jump to bottom

change responses of resend_email and adding testcases #765

Closed
wants to merge 2 commits into from
Closed

change responses of resend_email and adding testcases #765

wants to merge 2 commits into from

Conversation

saikirankv
Copy link
Contributor

@saikirankv saikirankv commented Aug 28, 2020 •
edited by isabelcosta
Loading

Description

Fixes #720

Type of Change:

  • Code
  • Quality Assurance
  • User Interface
  • Outreach
  • Documentation

Code/Quality Assurance Only

  • Bug fix (non-breaking change which fixes an issue)
  • This change requires a documentation update (software upgrade on readme file)
  • New feature (non-breaking change which adds functionality pre-approved by mentors)

How Has This Been Tested?

Checklist:

  • My PR follows the style guidelines of this project
  • I have performed a self-review of my own code or materials
  • I have commented my code or provided relevant documentation, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • Any dependent changes have been merged
  • Update Postman API at /docs folder
  • Update Swagger documentation and the exported file at /docs folder
  • Update requirements.txt

Code/Quality Assurance Only

  • My changes generate no new warnings
  • My PR currently breaks something (fix or feature that would cause existing functionality to not work as expected)
  • I have added tests that prove my fix is effective or that my feature works
  • New and existing unit tests pass locally with my changes
  • Any dependent changes have been published in downstream modules

@codecov
Copy link

codecov bot commented Aug 28, 2020 •
edited
Loading

Codecov Report

Merging #765 into develop will increase coverage by 0.47%.
The diff coverage is 97.50%.

Impacted file tree graph

@@             Coverage Diff             @@
##           develop     #765      +/-   ##
===========================================
+ Coverage    95.84%   96.31%   +0.47%     
===========================================
  Files           95       97       +2     
  Lines         5200     5322     +122     
===========================================
+ Hits          4984     5126     +142     
+ Misses         216      196      -20
Impacted Files Coverage Δ
tests/users/test_api_resend_email.py 97.14% <97.14%> (ø)
app/api/resources/user.py 94.23% <100.00%> (+5.28%) ⬆️
app/database/models/user.py 98.57% <0.00%> (-0.03%) ⬇️
app/api/models/user.py 100.00% <0.00%> (ø)
app/api/api_extension.py 100.00% <0.00%> (ø)
app/api/resources/admin.py 88.13% <0.00%> (ø)
tests/user_journey/test_happy_path_1.py 98.86% <0.00%> (ø)
app/api/dao/user.py 85.48% <0.00%> (+0.40%) ⬆️
app/api/resources/mentorship_relation.py 97.74% <0.00%> (+0.75%) ⬆️
... and 3 more

@saikirankv saikirankv mentioned this pull request Aug 28, 2020
Open
@saikirankv
Copy link
Contributor Author

@isabelcosta please review my PR

@SanketDG SanketDG added the Status: Needs Review PR needs an additional review or a maintainer's review. label Aug 28, 2020
@isabelcosta
Copy link
Member

isabelcosta commented Sep 4, 2020 •
edited
Loading

@saikirankv can you please mention in the PR description which issue you are solving?
I just updated your description. Could you add some description to the PR, following the template :)

isabelcosta
isabelcosta requested changes Sep 4, 2020
View reviewed changes
Copy link
Member

@isabelcosta isabelcosta left a comment

Choose a reason for hiding this comment

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

I requested a very tiny change.
You did very good work with the new test suite! 👏 👏 @saikirankv
Thank you for your contribution!

app/api/resources/user.py Outdated
@@ -320,7 +320,7 @@ def get(cls, token):

@users_ns.route("user/resend_email")
@users_ns.response(HTTPStatus.OK, "%s" % messages.EMAIL_VERIFICATION_MESSAGE)
@users_ns.response(HTTPStatus.BAD_REQUEST, "Invalid input.")
@users_ns.response(HTTPStatus.BAD_REQUEST, "%s" % messages.EMAIL_INPUT_BY_USER_IS_INVALID)
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
@users_ns.response(HTTPStatus.BAD_REQUEST, "%s" % messages.EMAIL_INPUT_BY_USER_IS_INVALID)
@users_ns.response(HTTPStatus.BAD_REQUEST, f"{messages.EMAIL_INPUT_BY_USER_IS_INVALID})

I would suggest this, because in this project we are trying to be consistent with f-strings :)

@isabelcosta isabelcosta requested a review from SanketDG September 4, 2020 23:58
@saikirankv
Copy link
Contributor Author

@SanketDG can you review this PR?

SanketDG
SanketDG approved these changes Sep 19, 2020
View reviewed changes
Copy link
Contributor

@SanketDG SanketDG left a comment

Choose a reason for hiding this comment

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

LGTM!

@vj-codes
Copy link
Member

@saikirankv please resolve the merge conflicts thanks

@vj-codes vj-codes added Status: Changes Requested Changes are required to be done by the PR author. and removed Status: Needs Review PR needs an additional review or a maintainer's review. labels Feb 21, 2021
@vj-codes
Copy link
Member

@saikirankv closing due to inactivity
Let us know if you would like to continue again and thank you for your contribution :)

@vj-codes vj-codes closed this Mar 17, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@isabelcosta isabelcosta isabelcosta requested changes

@SanketDG SanketDG SanketDG approved these changes

Assignees
No one assigned
Labels
Status: Changes Requested Changes are required to be done by the PR author.
Projects
None yet
Milestone
No milestone
4 participants
@saikirankv @isabelcosta @vj-codes @SanketDG