Skip to content

Comments

[fix] Different types of phone numbers are allowed, but only mobile types should#487

Merged
nemesifier merged 2 commits intoopenwisp:masterfrom
ujjwalkirti:master
Sep 1, 2023
Merged

[fix] Different types of phone numbers are allowed, but only mobile types should#487
nemesifier merged 2 commits intoopenwisp:masterfrom
ujjwalkirti:master

Conversation

@ujjwalkirti
Copy link
Contributor

@ujjwalkirti ujjwalkirti commented Aug 13, 2023

I changed the code to check whether phone_number provided is a Mobile number and raise expception if not. (#484 )
Below is the function code that I have altered.

 def validate_phone_number(self, phone_number):
        org = self.context['view'].organization
        if get_organization_radius_settings(org, 'sms_verification'):
            if phonenumbers.phonenumberutil.number_type(phone_number) != 1:
                raise serializers.ValidationError(_('Landline numbers can\'t be used to send SMS.'))
            if not phone_number:
                raise serializers.ValidationError(_('This field is required.'))
            mobile_prefixes = org.radius_settings.allowed_mobile_prefixes_list
            if not self.is_prefix_allowed(phone_number, mobile_prefixes):
                raise serializers.ValidationError(
                    _('This international mobile prefix is not allowed.')
                )
            if User.objects.filter(phone_number=phone_number).exists():
                raise serializers.ValidationError(
                    _('A user is already registered with this phone number.')
                )
        else:
            # Phone number should not be stored if sms verification is disabled
            phone_number = None
        return phone_number

Closes #484

Edited by @pandafy

@ujjwalkirti ujjwalkirti marked this pull request as draft August 13, 2023 15:42
@ujjwalkirti ujjwalkirti marked this pull request as ready for review August 13, 2023 15:47
@ujjwalkirti
Copy link
Contributor Author

@nemesifier can you please review this PR for the issue #484

Copy link
Member

@nemesifier nemesifier left a comment

Choose a reason for hiding this comment

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

@ujjwalkirti thanks for contributing! We need a failing unit test which replicates the bug before we can merge a fix. Do you know how to write tests? If you look at https://github.com/openwisp/openwisp-radius/blob/master/openwisp_radius/tests/test_api/test_phone_verification.py you can find plenty of examples.

org = self.context['view'].organization
if get_organization_radius_settings(org, 'sms_verification'):
if phonenumbers.phonenumberutil.number_type(phone_number) != 1:
raise serializers.ValidationError(_('Landline numbers can\'t be used to send SMS.'))
Copy link
Member

Choose a reason for hiding this comment

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

I'd write that only mobile phone numbers are allowed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Okay, I will make the required changes.

@pandafy
Copy link
Member

pandafy commented Aug 31, 2023

Hey @ujjwalkirti!! Thank for contributing. I will be taking over this PR to complete it.

@nemesifier nemesifier merged commit 80a4c47 into openwisp:master Sep 1, 2023
@nemesifier nemesifier changed the title Bug Fix: Different types of phone numbers are allowed, but only mobile types should [fix] Different types of phone numbers are allowed, but only mobile types should Sep 1, 2023
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.

[bug] Different types of phone numbers are allowed, but only mobile types should

3 participants