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

fix: check for phone identity on Phone Change #1819

Draft
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

J0
Copy link
Contributor

@J0 J0 commented Oct 28, 2024

What kind of change does this PR introduce?

Allows user to change phone number to that of another unconfirmed phone number in they system

What is the current behavior?

Currently, it is not possible to use a phone number which exists in the system but is unconfirmed. Consider the following scenario:

User A signs up with phone number PN but doesn't confirm the number.
User B signs up with phone number PN2 and confirms it.
User B tries to update their phone number to PN -> this fails because User A has signed up with it already.

What is the new behavior?

Check is done via phone identity, it is possible to change phone number to one that is tagged to another user but uncofirmed.

Additional context

  • Need to see if we can use this method in the admin route and remove IsDuplciatedPhone
  • Think and check if it's worthwhile to write a new model method and condense the FindUser and FindIdentity into a single function

internal/models/user.go Outdated Show resolved Hide resolved
@coveralls
Copy link

Pull Request Test Coverage Report for Build 11568181912

Warning: This coverage report may be inaccurate.

This pull request's base commit is no longer the HEAD commit of its target branch. This means it includes changes from outside the original pull request, including, potentially, unrelated coverage changes.

Details

  • 12 of 14 (85.71%) changed or added relevant lines in 2 files are covered.
  • 71 unchanged lines in 5 files lost coverage.
  • Overall coverage increased (+0.01%) to 57.175%

Changes Missing Coverage Covered Lines Changed/Added Lines %
internal/models/user.go 11 13 84.62%
Files with Coverage Reduction New Missed Lines %
internal/models/user.go 1 75.67%
internal/api/helpers.go 5 80.49%
internal/security/captcha.go 11 68.52%
internal/api/admin.go 23 65.81%
internal/api/middleware.go 31 80.52%
Totals Coverage Status
Change from base Build 11548689337: 0.01%
Covered Lines: 9574
Relevant Lines: 16745

💛 - Coveralls

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.

2 participants