- Download Diff

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.
'register_email_hook' imported but unused	reviewbot	March 20, 2017, 5:29 p.m.
'unregister_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
```
 'register_email_hook' imported but unused
```
reviewboard/notifications/email/__init__.py (Diff revision 1)
Show all issues
```
 'unregister_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)

You have a pending review.

Review Board 4.0 alpha 0 (dev)

Refactor notifications.email into a module

Change Summary:

Description:

Commit:

Diff:

Change Summary:

Commit:

Diff:

Checks run (2 succeeded, 1 failed with error)

Change Summary:

Diff:

Checks run (2 succeeded, 1 failed with error)

Diff:

Checks run (2 succeeded, 1 failed with error)

Status: Closed (submitted)

Change Summary: