Summary

Refactor notifications.email into a module

Review Request #8829 — Created March 20, 2017 and submitted May 22, 2017, 10:39 p.m.

Information

Owner

brennie

Repository

Review Board

Branch

release-3.0.x

Bugs

Depends On

Blocks

8889

Commit

72646df...

Reviewers

Groups

reviewboard

People

Description

email.py was getting long and it is past due that it was split up.
This patch is in preparation for another change that will add more
e-mail-specific code.

Testing Done

Ran unit tests.

Issues

Description	From	Last Updated
These modules need to be listed in the codebase docs.	chipx86	April 17, 2017, 4:30 p.m.
'unregister_email_hook' imported but unused	reviewbot	March 20, 2017, 5:29 p.m.
'register_email_hook' imported but unused	reviewbot	March 20, 2017, 5:29 p.m.
undefined name 'logging'	reviewbot	March 20, 2017, 5:29 p.m.
I think connect_signals should still live in the top-level email module.	david	April 4, 2017, 12:29 p.m.
Blank line after this.	david	April 4, 2017, 12:29 p.m.
File docstring?	david	April 4, 2017, 12:30 p.m.
Blank line after this?	david	April 4, 2017, 12:30 p.m.
Blank line after this?	david	April 4, 2017, 12:30 p.m.
Blank line after this?	david	April 4, 2017, 12:30 p.m.
There's 3 blank lines here.	chipx86	April 17, 2017, 4:31 p.m.
No blank line.	chipx86	April 17, 2017, 4:31 p.m.
We usually list this last in the file. This doesn't contain connect_signals. This is also most commonly a list (all …	chipx86	April 17, 2017, 4:39 p.m.
Two blank lines.	chipx86	April 17, 2017, 4:34 p.m.
"Extension hooks for augmenting e-mails" will be more useful in the codebase docs.	chipx86	April 17, 2017, 4:34 p.m.
Should be two blank lines.	chipx86	April 17, 2017, 4:34 p.m.
This loses the information we had in the original docstring.	chipx86	April 17, 2017, 4:37 p.m.
I guess we can skip this one, since it's purely internal callbacks.	chipx86	April 18, 2017, 9:10 p.m.
Too many blank lines.	chipx86	April 18, 2017, 9:10 p.m.
Inconsistent alignment.	chipx86	April 18, 2017, 9:11 p.m.
Kinda wonder if this should just live in callbacks.py. Maybe rename that to something like signal_handlers.py and have the base …	chipx86	April 18, 2017, 9:17 p.m.
tuple should be on its own line.	chipx86	April 18, 2017, 9:18 p.m.
tuple should be on its own line.	chipx86	April 18, 2017, 9:18 p.m.
list should be on its own line.	chipx86	April 18, 2017, 9:19 p.m.

Tool: Pyflakes
Processed Files:
    reviewboard/notifications/tests.py
    reviewboard/notifications/email/callbacks.py
    reviewboard/notifications/email/__init__.py
    reviewboard/notifications/email/message.py
    reviewboard/accounts/forms/pages.py
    reviewboard/notifications/email/utils.py
    reviewboard/notifications/email/hooks.py
    reviewboard/notifications/__init__.py



Tool: PEP8 Style Checker
Processed Files:
    reviewboard/notifications/tests.py
    reviewboard/notifications/email/callbacks.py
    reviewboard/notifications/email/__init__.py
    reviewboard/notifications/email/message.py
    reviewboard/accounts/forms/pages.py
    reviewboard/notifications/email/utils.py
    reviewboard/notifications/email/hooks.py
    reviewboard/notifications/__init__.py

reviewboard/notifications/email/__init__.py (Diff revision 1)
Show all issues
```
 'unregister_email_hook' imported but unused
```
reviewboard/notifications/email/__init__.py (Diff revision 1)
Show all issues
```
 'register_email_hook' imported but unused
```
reviewboard/notifications/email/utils.py (Diff revision 1)
Show all issues
```
 undefined name 'logging'
```

Change Summary:

Reflow description

Description:

~		`email.py` was getting long and it is past due that it
~		was split up.
	~	`email.py` was getting long and it is past due that it was split up.
	~	This patch is in preparation for another change that will add more
	+	e-mail-specific code.

Commit:

-	2adbe94cd68af145c690060b296188390dc087c7
+	ef0d1721cb5b44898335b5c576d39ecbbe637989

Diff:

Revision 2 (+425 -388)

Show changes

	reviewboard/accounts/forms/pages.py
	reviewboard/notifications/__init__.py
	reviewboard/notifications/tests.py
	reviewboard/notifications/email/__init__.py
	reviewboard/notifications/email.py
	reviewboard/notifications/email/hooks.py
	reviewboard/notifications/email/message.py
	reviewboard/notifications/email/utils.py

Tool: Pyflakes
Processed Files:
    reviewboard/notifications/tests.py
    reviewboard/notifications/email/callbacks.py
    reviewboard/notifications/email/__init__.py
    reviewboard/notifications/email/message.py
    reviewboard/accounts/forms/pages.py
    reviewboard/notifications/email/utils.py
    reviewboard/notifications/email/hooks.py
    reviewboard/notifications/__init__.py



Tool: PEP8 Style Checker
Processed Files:
    reviewboard/notifications/tests.py
    reviewboard/notifications/email/callbacks.py
    reviewboard/notifications/email/__init__.py
    reviewboard/notifications/email/message.py
    reviewboard/accounts/forms/pages.py
    reviewboard/notifications/email/utils.py
    reviewboard/notifications/email/hooks.py
    reviewboard/notifications/__init__.py

reviewboard/notifications/__init__.py (Diff revision 2)

Show all issues

I think connect_signals should still live in the top-level email module.

reviewboard/notifications/email/callbacks.py (Diff revision 2)
Show all issues
```
Blank line after this.
```
reviewboard/notifications/email/__init__.py (Diff revision 2)
Show all issues
```
File docstring?
```
reviewboard/notifications/email/hooks.py (Diff revision 2)
Show all issues
```
Blank line after this?
```
reviewboard/notifications/email/message.py (Diff revision 2)
Show all issues
```
Blank line after this?
```
reviewboard/notifications/email/utils.py (Diff revision 2)
Show all issues
```
Blank line after this?
```

Change Summary:

Addressed David's comments.

Commit:

-	ef0d1721cb5b44898335b5c576d39ecbbe637989
+	72646df8b30cae6c7c869235cc49c8c73d91c604

Diff:

Revision 3 (+450 -393)

Show changes

	reviewboard/accounts/forms/pages.py
	reviewboard/notifications/tests.py
	reviewboard/notifications/email/__init__.py
	reviewboard/notifications/email.py
	reviewboard/notifications/email/hooks.py
	reviewboard/notifications/email/message.py
	reviewboard/notifications/email/utils.py

Checks run (2 succeeded, 1 failed with error)

JSHint passed.

PEP8 Style Checker internal error.

Pyflakes passed.

Show all issues

These modules need to be listed in the codebase docs.

reviewboard/notifications/email/callbacks.py (Diff revision 3)
Show all issues
```
There's 3 blank lines here.
```
reviewboard/notifications/email/__init__.py (Diff revision 3)
Show all issues
```
No blank line.
```

reviewboard/notifications/email/__init__.py (Diff revision 3)

Show all issues

We usually list this last in the file.

This doesn't contain connect_signals.

This is also most commonly a list (all Python docs, PEP8, etc. all show as a list instead of a tuple). The rest of our code uses a list as well, so let's change this one.

david April 17, 2017, 4:32 p.m.

I think people are slowly migrating towards using tuples because they're immutable.

reviewboard/notifications/email/__init__.py (Diff revision 3)
Show all issues
```
Two blank lines.
```

reviewboard/notifications/email/hooks.py (Diff revision 3)

Show all issues

"Extension hooks for augmenting e-mails" will be more useful in the codebase docs.

reviewboard/notifications/email/hooks.py (Diff revision 3)
Show all issues
```
Should be two blank lines.
```
reviewboard/notifications/email/message.py (Diff revision 3)
Show all issues
```
This loses the information we had in the original docstring.
```

Change Summary:

Addressed Christian's issues.

Diff:

Revision 4 (+463 -394)

Show changes

	docs/manual/extending/coderef/index.rst
	reviewboard/accounts/forms/pages.py
	reviewboard/notifications/tests.py
	reviewboard/notifications/email/__init__.py
	reviewboard/notifications/email.py
	reviewboard/notifications/email/hooks.py
	reviewboard/notifications/email/message.py
	reviewboard/notifications/email/utils.py

Checks run (2 succeeded, 1 failed with error)

JSHint passed.

PEP8 Style Checker internal error.

Pyflakes passed.

Some of this is from code that was already there, but while we're splitting into a new module...

docs/manual/extending/coderef/index.rst (Diff revision 4)

Show all issues

I guess we can skip this one, since it's purely internal callbacks.

reviewboard/notifications/email/callbacks.py (Diff revision 4)
Show all issues
```
Too many blank lines.
```
reviewboard/notifications/email/__init__.py (Diff revision 4)
Show all issues
```
Inconsistent alignment.
```

reviewboard/notifications/email/__init__.py (Diff revision 4)

Show all issues

Kinda wonder if this should just live in callbacks.py. Maybe rename that to something like signal_handlers.py and have the base notifications/__init__.py import that directly? What do you think?

brennie April 18, 2017, 9:17 p.m.

That makes the import of connect_signals from each module (webhooks, email) less symmetric. I think this is good as is, but Im cool with the rename.

reviewboard/notifications/email/hooks.py (Diff revision 4)
Show all issues
```
tuple should be on its own line.
```
reviewboard/notifications/email/utils.py (Diff revision 4)
Show all issues
```
tuple should be on its own line.
```
reviewboard/notifications/email/utils.py (Diff revision 4)
Show all issues
```
list should be on its own line.
```

Diff:

Revision 5 (+464 -394)

Show changes

	docs/manual/extending/coderef/index.rst
	reviewboard/accounts/forms/pages.py
	reviewboard/notifications/tests.py
	reviewboard/notifications/email/__init__.py
	reviewboard/notifications/email/hooks.py
	reviewboard/notifications/email/message.py
	reviewboard/notifications/email.py
	reviewboard/notifications/email/utils.py

Checks run (2 succeeded, 1 failed with error)

JSHint passed.

PEP8 Style Checker internal error.

Pyflakes passed.

Ship it!

```
Ship It!
```

Status: Closed (submitted)

Change Summary:

Pushed to release-3.0.x (468c1a6)