Merge lp://staging/~newz/awstrial/3rdlevel-domain-blacklisting into lp://staging/awstrial
Proposed by
Matthew Nuzum
| Status: | Merged |
|---|---|
| Approved by: | Matthew Nuzum |
| Approved revision: | 299 |
| Merged at revision: | 296 |
| Proposed branch: | lp://staging/~newz/awstrial/3rdlevel-domain-blacklisting |
| Merge into: | lp://staging/awstrial |
| Diff against target: |
128 lines (+76/-16) 2 files modified
awstrial/trial/auth.py (+19/-6) awstrial/trial/tests.py (+57/-10) |
| To merge this branch: | bzr merge lp://staging/~newz/awstrial/3rdlevel-domain-blacklisting |
| Related bugs: |
| Reviewer | Review Type | Date Requested | Status |
|---|---|---|---|
| Māris Fogels (community) | Approve | ||
| Matthew Nuzum (community) | Needs Resubmitting | ||
|
Review via email:
|
|||
Commit message
This updates the blacklisted_
Description of the change
This updates the blacklisted_
For example:
Free mail service xyz.com is often used for abusers but email addresses are in the form of
<email address hidden>
Now, simply blocking xyz.com will also block randombits.xyz.com.
To post a comment you must log in.
Preview Diff
[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
The implementation itself looks OK. The tests need some work:
'settings' is a global variable and fiddling with it is bad. It is better to have a single test that asserts that the setting is true and take the check out of the individual tests.
You don't need to check the log message in every test. You need two separate tests just for logging: one that checks that the log message is present for blacklisted addresses, one that tests that it is absent for valid addresses. If you have the two logging tests, then you can remove all the log check lines from the new tests you wrote.
That logger line is old code, but it's terrible. Please change it to the standard logging pattern using logging.