This repository has been archived by the owner. It is now read-only.
fix(tests): Update rate limit sms by ip address #191
Merged
Conversation
|
LGTM, r+ with nits |
lib/ip_record.js
Outdated
| this.sms = this._trim(now, this.sms, limits.maxSms) | ||
| if (this.sms.length === 0) { return } | ||
| // xs is naturally ordered from oldest to newest | ||
| // and we only need to keep up to limits.maxEmails + 1 |
rfk
Mar 28, 2017
Member
comment nit: s/maxEmails/maxSms/
comment nit: s/maxEmails/maxSms/
| @@ -236,8 +234,8 @@ module.exports = function (limits, now) { | |||
| } | |||
|
|
|||
| // Increment sms request count and throttle if needed | |||
| if (actions.isSmsSendingAction(action) && phoneNumber) { | |||
| this.addSmsRequest({ phoneNumber: phoneNumber }) | |||
rfk
Mar 28, 2017
Member
The phoneNumber argument appears to be unused after removing these, can we safely remote it or does it need to be kept for consistency with other records?
The phoneNumber argument appears to be unused after removing these, can we safely remote it or does it need to be kept for consistency with other records?
vbudhram
Mar 28, 2017
Author
Contributor
Good catch, it can be removed.
Good catch, it can be removed.
vbudhram
force-pushed the
issue-190-fix-sms-rate-limit
branch
2 times, most recently
from
dc7799b
to
5f49655
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
This PR correctly rate limits when a user has sent more than 5 sms from an ip address. Previously, the the rate limiting was
rate-limit when 5 unique sms requests sent from this ip address. Updated tests to be more reflective of this scenario.Fixes #190
@rfk r?