Opened 9 years ago

Closed 7 years ago

#1860 closed task (fixed)

bridgedb email rate limiting needs review

Reported by: aagbsn Owned by: nickm
Priority: Medium Milestone:
Component: Circumvention/BridgeDB Version:
Severity: Keywords:
Cc: aagbsn@… Actual Points:
Parent ID: #4380 Points:
Reviewer: Sponsor:

Description

Please review bridgedb email rate limiting support:
http://github.com/aagbsn/bridgedb/tree/ratelimit/

Child Tickets

Change History (8)

comment:1 Changed 9 years ago by aagbsn

Status: newneeds_review

comment:2 Changed 9 years ago by aagbsn

Cc: aagbsn@… added

comment:3 Changed 9 years ago by nickm

Looks good: some issues...

  • It's okay to use "git rebase -i" to squash patch series that nobody has looked at before sending them for review.
  • It looks like when you're the EMAIL_MESSAGE_RATELIMIT message, you tell the user that the rate limit is a number of seconds. Most non-programmers won't automatically assume that an integer like this is a number of seconds; probably the message should say something more like "the minimum time between emails is %d seconds" rather than just "the minimum time between emails is %d".
  • You're implementing booleans in the db in a slightly odd way; it looks like you insert the value "True" to indicate true in setWasWarned, but you treat _any_ value as indicating truth in getWasWarned.
  • Also in the sql code, WarnedEmails is a better table name than WasWarned. If we have more than one warning in the future, WasWarned will be a downright odd table name.
  • It looks like warnings stay in the table forever unless the user later sends a message that _doesn't_ get a warning, and the database gets a complete list of all the email addresses it ever sent a warning to without later sending them a non-warning message. That can't be right, can it?

comment:4 Changed 9 years ago by nickm

Oh, one more:

  • It looks like a lot of the new code in the exception handler getMailResponse() is cut-and-pasted from the existing "#Generate the message" block. That's not so good; instead of duplicating code twice, can we turn it into a function?

comment:5 Changed 9 years ago by nickm

Hm. This probably looks like it needs to get revised to follow the same i18n stuff as the rest of the code. :/

Other than that, it looks pretty good. How tested is it?

comment:6 in reply to:  5 Changed 9 years ago by aagbsn

Replying to nickm:

Hm. This probably looks like it needs to get revised to follow the same i18n stuff as the rest of the code. :/

I can coordinate with Kaner to do this.

Other than that, it looks pretty good. How tested is it?

I have tested manually but this should ideally have automatic test cases written - merging this branch with countrycode support into master introduced an easily avoidable bug. Do you have any suggestions for how test cases can be implemented?

comment:7 Changed 8 years ago by karsten

Milestone: BridgeDB Upgrades Phase 1
Parent ID: #4380

Assigning as a child ticket to the project ticket that replaces the "BridgeDB Upgrades Phase 1" milestone.

comment:8 Changed 7 years ago by aagbsn

Resolution: fixed
Status: needs_reviewclosed

Already merged.

Note: See TracTickets for help on using tickets.