Summary

Support not sending e-mails on WebAPI token creation

Review Request #9258 — Created Oct. 10, 2017 and submitted Nov. 1, 2017, 2:25 p.m.

Information

Owner

brennie

Repository

Review Board

Branch

release-3.0.x

Bugs

Depends On

~~9275~~

Blocks

9259

Commit

de65698...

Reviewers

Groups

reviewboard

People

Description

Currently when a WebAPI token is created, we always send an e-mail
notifying the user of its creation. However, this is not always ideal.
Some extensions and/or integrations require a WebAPI token in order to
communicate with Review Board from some external service (such as
CircleCI). In this case, we do not need to bother the user with a
notification that the token was created.

Djblets now has support for token update and create signals (via
/r/9275). We use these signals to determine if the token generated was
auto-generated. The notifications module has been modified to listen
to the new signals (instead of the generic post_save signal it was
listening to before) and not send e-mails when the token in question
is auto-generated.

Testing Done

Ran unit tests.

Issues

Description	From	Last Updated
F841 local variable 'webapi_token' is assigned to but never used	reviewbot	Oct. 10, 2017, 4 p.m.
F401 'reviewboard.webapi.signals.webapi_token_created' imported but unused	reviewbot	Oct. 10, 2017, 4 p.m.
At this point it might be nice to turn this into a table with signal, handler, sender and then iterate …	david	Oct. 16, 2017, 4:02 p.m.
We could just have a signal for this in djblets. It's definitely possible that other consumers of djblets.webapi will have …	david	Oct. 17, 2017, noon
F401 'reviewboard.webapi.managers.WebAPITokenManager' imported but unused	reviewbot	Oct. 17, 2017, 12:07 p.m.
F401 'reviewboard.webapi.signals.webapi_token_updated' imported but unused	reviewbot	Oct. 17, 2017, 12:07 p.m.
Maybe a list of tuples?	david	Oct. 20, 2017, 12:46 p.m.
This docstring needs to be updated now.	david	Oct. 20, 2017, 12:46 p.m.
The outbox should default to [] at the beginning of every test. I think you can get rid of this …	david	Oct. 20, 2017, 12:47 p.m.
I think other things assertEqual(len(mail.outbox), 0)	david	Oct. 20, 2017, 12:47 p.m.
This can fit on one line.	chipx86	Oct. 23, 2017, 2:07 p.m.
"WebAPIToken"	chipx86	Oct. 23, 2017, 2:08 p.m.

flake8 failed.

JSHint passed.

flake8

reviewboard/notifications/tests.py (Diff revision 1)
The issue has been resolved. Show all issues
```
F841 local variable 'webapi_token' is assigned to but never used
```
reviewboard/webapi/models.py (Diff revision 1)
The issue has been resolved. Show all issues
```
F401 'reviewboard.webapi.signals.webapi_token_created' imported but unused
```

Commit:

ae136263f6f7459e3ce78b48e25d0026b5b6586c

630e90eb4669427a9cc8bfd06de47b092062fe91

Diff:

Revision 2 (+149 -14)

Show changes

	reviewboard/notifications/tests.py
	reviewboard/notifications/email/__init__.py
	reviewboard/notifications/email/signal_handlers.py
	reviewboard/webapi/managers.py
	reviewboard/webapi/models.py
	reviewboard/webapi/signals.py

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

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

The issue has been resolved. Show all issues

At this point it might be nice to turn this into a table with signal, handler, sender and then iterate that to do all the connections.

reviewboard/webapi/managers.py (Diff revision 2)

The issue has been resolved. Show all issues

We could just have a signal for this in djblets. It's definitely possible that other consumers of djblets.webapi will have similar wants.

brennie Oct. 16, 2017, 3:56 p.m.

I feel like the wanting email or not is kinda specific to RB itself?

david Oct. 16, 2017, 4:32 p.m.

I mean the created signal, not the handler.

brennie Oct. 16, 2017, 9:16 p.m.

Yeah but we are adding the signal basically only for this reason. We have to have the args the signal will accept ahead of time, and having send_email be one of those is weird. Perhaps it would be better to rename this to autogenerated? I would be comfortable with that in djblets.

Change Summary:

Use a table of signals and loop through it

Commit:

630e90eb4669427a9cc8bfd06de47b092062fe91

5be1a7f85d2dcfc9176275ce31195acb8f272eac

Diff:

Revision 3 (+156 -23)

Show changes

	reviewboard/notifications/tests.py
	reviewboard/notifications/email/__init__.py
	reviewboard/notifications/email/signal_handlers.py
	reviewboard/webapi/managers.py
	reviewboard/webapi/models.py
	reviewboard/webapi/signals.py

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

Change Summary:

Move signals into /r/9275/

Description:

		Currently when a WebAPI token is created, we always send an e-mail
		notifying the user of its creation. However, this is not always ideal.
		Some extensions and/or integrations require a WebAPI token in order to
		communicate with Review Board from some external service (such as
		CircleCI). In this case, we do not need to bother the user with a
		notification that the token was created.

~		To accomplish this, we now have specific WebAPI creation/update signals,
~		as well as a more specialized manager for the `WebAPIToken` class so
~		that we may specify at creation time whether or not an e-mail will be
~		sent. Currently the only way to not send an e-mail is via this manager.
~
~		The notifications module has been modified to listen to these new
	~	Djblets now has support for token update and create signals (via
	~	/r/9275). We use these signals to determine if the token generated was
	~	auto-generated. The notifications module has been modified to listen
	~	to the new signals (instead of the generic `post_save` signal it was
	~	listening to before) and not send e-mails when the token in question
	~	is auto-generated.
-		signals (instead of the generic `post_save` signal it was listening to
-		before) and conditionally not send e-mails when requested.
-
-		Testing done:
-		Ran unit tests.

Commit:

5be1a7f85d2dcfc9176275ce31195acb8f272eac

23b1d7a8157d2c7c4840d9506fe3b52286a7729a

Diff:

Revision 4 (+56 -23)

Show changes

	reviewboard/notifications/tests.py
	reviewboard/notifications/email/__init__.py
	reviewboard/notifications/email/signal_handlers.py
	reviewboard/webapi/models.py

Checks run (1 failed, 1 succeeded)

flake8 failed.

JSHint passed.

flake8

reviewboard/webapi/models.py (Diff revision 4)
The issue has been resolved. Show all issues
```
F401 'reviewboard.webapi.managers.WebAPITokenManager' imported but unused
```
reviewboard/webapi/models.py (Diff revision 4)
The issue has been resolved. Show all issues
```
F401 'reviewboard.webapi.signals.webapi_token_updated' imported but unused
```

Commit:

23b1d7a8157d2c7c4840d9506fe3b52286a7729a

40d45803bf5ab164b6373048e5d08f9d825df73c

Diff:

Revision 5 (+54 -23)

Show changes

	reviewboard/notifications/tests.py
	reviewboard/notifications/email/__init__.py
	reviewboard/notifications/email/signal_handlers.py

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

Depends On:: ~~9275 - Add signals for token creation and update~~

reviewboard/notifications/email/__init__.py (Diff revision 5)
The issue has been resolved. Show all issues
```
Maybe a list of tuples?
```
reviewboard/notifications/tests.py (Diff revision 5)
The issue has been resolved. Show all issues
```
This docstring needs to be updated now.
```

reviewboard/notifications/tests.py (Diff revision 5)

The issue has been resolved. Show all issues

The outbox should default to [] at the beginning of every test. I think you can get rid of this line.

reviewboard/notifications/tests.py (Diff revision 5)
The issue has been resolved. Show all issues
```
I think other things assertEqual(len(mail.outbox), 0)
```