Skip to content

Fix: Multiple relation requests from a user to same member #994

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
Open
wants to merge 2 commits into
base: develop
Choose a base branch
from
Open

Fix: Multiple relation requests from a user to same member #994

wants to merge 2 commits into from

Conversation

decon-harsh
Copy link
Member

@decon-harsh decon-harsh commented Feb 3, 2021 •
edited
Loading

Description

A user was able to send multiple mentorship request to the same member.

So, I added a check on relation.state and returned HTTPS.CONFLICT
Fixes #866

Type of Change:

  • Code

Code/Quality Assurance Only

  • Bug fix (non-breaking change which fixes an issue)

How Has This Been Tested?

Manually through postman
Ran all tests
Screenshot from 2021-02-03 22-08-14

Mocks

Screenshot from 2021-02-03 22-09-34

Checklist:

  • My PR follows the style guidelines of this project
  • I have performed a self-review of my own code or materials

Code/Quality Assurance Only

  • My changes generate no new warnings

@codecov
Copy link

codecov bot commented Feb 3, 2021 •
edited
Loading

Codecov Report

Merging #994 (01393c4) into develop (8f1906e) will decrease coverage by 3.26%.
The diff coverage is 83.33%.

Impacted file tree graph

@@             Coverage Diff             @@
##           develop     #994      +/-   ##
===========================================
- Coverage    96.14%   92.88%   -3.27%     
===========================================
  Files           96       38      -58     
  Lines         5399     2079    -3320     
===========================================
- Hits          5191     1931    -3260     
+ Misses         208      148      -60
Impacted Files Coverage Δ
app/api/dao/mentorship_relation.py 95.65% <75.00%> (-0.46%) ⬇️
app/messages.py 100.00% <100.00%> (ø)
app/api/resources/mentorship_relation.py 96.51% <0.00%> (-1.24%) ⬇️
app/api/dao/user.py 85.82% <0.00%> (-0.06%) ⬇️
app/api/models/user.py 100.00% <0.00%> (ø)
app/api/api_extension.py 100.00% <0.00%> (ø)
app/api/jwt_extension.py 100.00% <0.00%> (ø)
app/api/models/mentorship_relation.py 100.00% <0.00%> (ø)
app/schedulers/delete_unverified_users_cron_job.py 100.00% <0.00%> (ø)
... and 73 more

@decon-harsh
Copy link
Member Author

@vj-codes Please have a look

@decon-harsh decon-harsh mentioned this pull request Feb 4, 2021
Open
6 tasks
@vj-codes vj-codes added the Status: Needs Review PR needs an additional review or a maintainer's review. label Feb 5, 2021
@epicadk
Copy link
Member

epicadk commented Apr 13, 2021 •
edited
Loading

Please add releavant tests as well

@epicadk epicadk 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 Apr 13, 2021
@vj-codes
Copy link
Member

vj-codes commented May 5, 2021

@decon-harsh any updates here?

@vj-codes
Copy link
Member

@decon-harsh any updates?

epicadk
epicadk reviewed May 18, 2021
View reviewed changes
app/api/dao/mentorship_relation.py
return messages.MENTOR_ALREADY_IN_A_RELATION, HTTPStatus.BAD_REQUEST
elif (
Copy link
Member

Choose a reason for hiding this comment

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

Nit: you don't really need elif over 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 didn't understand why isn't it required?

Copy link
Member

Choose a reason for hiding this comment

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

You can just use another if statement right ? No need for the elif

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 mean I can but why, if the relation.state is ACCEPTED it cannot be PENDING then , only one prevails at a time. Do you still want me to use if instead of elif?

Copy link
Member

Choose a reason for hiding this comment

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

I agree with what @decon-harsh says here. @epicadk why we need an if here?

@decon-harsh decon-harsh requested a review from isabelcosta as a code owner May 27, 2021 06:56
@vj-codes vj-codes added Status: Needs Review PR needs an additional review or a maintainer's review. and removed Status: Changes Requested Changes are required to be done by the PR author. labels Jul 24, 2021
vj-codes
vj-codes reviewed Jan 17, 2023
View reviewed changes
Copy link
Member

@vj-codes vj-codes left a comment

Choose a reason for hiding this comment

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

Closing due to inactivity, thank you for your contribution:)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@devkapilbansal devkapilbansal devkapilbansal left review comments

@epicadk epicadk epicadk left review comments

@vj-codes vj-codes vj-codes left review comments

@isabelcosta isabelcosta Awaiting requested review from isabelcosta isabelcosta is a code owner

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Labels
Status: Needs Review PR needs an additional review or a maintainer's review.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

bug: A user is able to send multiple relation requests to the same member
4 participants
@decon-harsh @epicadk @vj-codes @devkapilbansal