Skip to content
This repository has been archived by the owner on Apr 3, 2019. It is now read-only.

feat(unblock): add unblock rate limits #131

Merged
merged 1 commit into from
Oct 5, 2016
Merged

feat(unblock): add unblock rate limits #131

merged 1 commit into from
Oct 5, 2016

Conversation

seanmonstar
Copy link
Contributor

@seanmonstar seanmonstar commented Sep 25, 2016

}

EmailRecord.parse = function (object) {
var rec = new EmailRecord()
object = object || {}
rec.bk = object.bk // timestamp when the account was banned
rec.rl = object.rl // timestamp when the account was rate-limited
rec.xs = object.xs || [] // timestamps when emails were sent
rec.xs = object.xs || rec.xs // timestamps when emails were sent
Copy link
Contributor

Choose a reason for hiding this comment

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

Does this fix a bug, or just for consistency?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I just noticed that an Array is already initialized in the constructor, why create a new one. One might hope that V8 can initialize arrays for nothing, but 🤷

Copy link
Contributor

Choose a reason for hiding this comment

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

👍 ; mostly just confirming it wasn't a bug :-)

@rfk
Copy link
Contributor

rfk commented Sep 25, 2016

@seanmonstar as an unsolicited f?, this LGTM at a high-level - it's more ad-hoc coupling between auth-server and customs-server APIs, but we're well down that road already, so 👍

Copy link
Contributor

@rfk rfk left a comment

Choose a reason for hiding this comment

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

This LGTM apart from test coverage of the payload stuff.

return {
block: true,
retryAfter: ipRecord.retryAfter()
}
}


var blockEmail = emailRecord.update(action)
var wantsUnblock = req.body.payload && req.body.payload.unblockCode
var blockEmail = emailRecord.update(action, !!wantsUnblock)
Copy link
Contributor

Choose a reason for hiding this comment

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

I think it would be worth adding a remote test exercises this payload-checking logic

@rfk rfk removed the shipit label Oct 4, 2016
@seanmonstar seanmonstar force-pushed the unblock branch 4 times, most recently from 351f72a to 48664c4 Compare October 5, 2016 02:12
@seanmonstar
Copy link
Contributor Author

@rfk added a remote test that payload.unblockCode affects the EmailRecord in memcached.

@rfk rfk merged commit 03c8c02 into master Oct 5, 2016
@rfk rfk removed the waffle:review label Oct 5, 2016
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants