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

[feature] Limit consecutive SMS tokens #481 #485

Merged
merged 2 commits into from
Aug 1, 2023

Conversation

pandafy
Copy link
Member

@pandafy pandafy commented Jul 25, 2023

@pandafy pandafy force-pushed the issues/481-limit-sms-tokens branch from 63a3a9e to 202d328 Compare July 26, 2023 11:35
@@ -53,6 +53,7 @@ jobs:
run: |
pip install -e .[saml,openvpn_status]
pip install ${{ matrix.django-version }}
pip install --force-reinstall --no-deps "https://github.com/openwisp/openwisp-utils/tarball/fallback-positiveintegerfield"
Copy link
Member Author

Choose a reason for hiding this comment

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

TODO: Remove this before merging.

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 switch back to master

@coveralls
Copy link

coveralls commented Jul 26, 2023

Coverage Status

coverage: 98.817% (+0.005%) from 98.812% when pulling efbce3a on issues/481-limit-sms-tokens into 0cfe071 on master.

@pandafy pandafy force-pushed the issues/481-limit-sms-tokens branch from 202d328 to 8ffd9c5 Compare July 26, 2023 12:08
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.

See my comments below.

@@ -53,6 +53,7 @@ jobs:
run: |
pip install -e .[saml,openvpn_status]
pip install ${{ matrix.django-version }}
pip install --force-reinstall --no-deps "https://github.com/openwisp/openwisp-utils/tarball/fallback-positiveintegerfield"
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 switch back to master

which have :ref:`enabled the SMS verification option
<openwisp_radius_sms_verification_enabled>`.

``OPENWISP_RADIUS_SMS_REQUEST_TIMEOUT``
Copy link
Member

Choose a reason for hiding this comment

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

Time out is not a good name for this.

I would change it to OPENWISP_RADIUS_SMS_COOLDOWN.

@@ -571,6 +571,7 @@ class OrganizationRadiusSettingsInline(admin.StackedInline):
'sms_sender',
'sms_message',
'allowed_mobile_prefixes',
'sms_timeout',
Copy link
Member

Choose a reason for hiding this comment

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

same here

@@ -1109,6 +1110,16 @@ class AbstractOrganizationRadiusSettings(UUIDModel):
),
fallback=app_settings.SMS_MESSAGE_TEMPLATE,
)
sms_timeout = FallbackPositiveIntegerField(
Copy link
Member

Choose a reason for hiding this comment

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

same here


This setting is applicable only for organizations
which have :ref:`enabled the SMS verification option
<openwisp_radius_sms_verification_enabled>`.
Copy link
Member

Choose a reason for hiding this comment

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

is this note referred to OPENWISP_RADIUS_SMS_COOLDOWN? If so, please move it after the heading, not before.

Copy link
Member Author

Choose a reason for hiding this comment

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

This note is for OPENWISP_RADIUS_ALLOWED_MOBILE_PREFIXES

@nemesifier nemesifier merged commit 6f31d23 into master Aug 1, 2023
20 checks passed
@nemesifier nemesifier deleted the issues/481-limit-sms-tokens branch August 1, 2023 21:38
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.

[feature] Limit consecutive SMS tokens (backend REST API)
3 participants