Summary

Add event type filters and custom message templates for I Done This.

Review Request #8961 — Created Oct. 19, 2017 and updated Jan. 3, 2018, 12:25 a.m.

Information

Owner

mudaltsov

Repository

rbintegrations

Branch

master

Bugs

Depends On

Commit

e92b70c...

Reviewers

Groups

rbintegrations

People

Description

This change provides some customization for messages posted to I Done This.

Event types can be filtered so that only some kinds of activity get posted,
while others do not. For example, the "Reply published" event can be disabled
to reduce noise in I Done This from every single reply getting published.

The posted messages can also be customized independently for every event type.
An empty message causes a fallback to the default format string, which is shown
below the text field for reference. Parameters such as the review request ID,
summary, and URL are substituted into placeholder strings, or omitted from the
final message. The syntax for placeholders is from string.Template, which is
limited and safer compared to other kinds of string formatting.

New unit tests were added to verify the added functionality, and some existing
tests were updated to test individual event type filtering.

Testing Done

All unit tests pass.

Manually tested event type filtering. Choosing "All events" posts all activity
as before. Choosing a subset of event types only posts those events.

Also specified some custom message template strings and verified that they get
formatted and posted to I Done This, or cause appropriate validation errors.

Files

Issues

Description	From	Last Updated
I don't know that the word hash clarifies anything.	brennie	Oct. 20, 2017, 10:56 p.m.
Do we want to wrap the template strings with <code> e.g. <dt><code>...</code></dt> ?	brennie	Oct. 21, 2017, 12:04 a.m.
W503 line break before binary operator	reviewbot	Oct. 19, 2017, 2:37 a.m.
E251 unexpected spaces around keyword / parameter equals	reviewbot	Oct. 19, 2017, 2:39 a.m.
E251 unexpected spaces around keyword / parameter equals	reviewbot	Oct. 19, 2017, 2:39 a.m.
You could just have entry_type=None in the arguments list instead of using kwargs.pop here.	david	Jan. 2, 2018, 10:10 a.m.
Needs the return type (unicode)	david	Jan. 2, 2018, 10:12 a.m.
Add a blank line after this.	david	Jan. 2, 2018, 10:13 a.m.
Instead of str(e), this should use six.text_type(e). You can get that by adding from django.utils import six in the imports.	david	Jan. 2, 2018, 10:16 a.m.
str -> six.text_type. Also add a blank line after this.	david	Jan. 2, 2018, 10:16 a.m.
Can we rewrap this a bit? return [ RBIntegrationsExtension.instance.get_bundle_id( 'idonethis-integration-config-form-view'), ]	david	Jan. 2, 2018, 10:18 a.m.
This can be dedented 4 spaces (there's only ever one return value).	david	Jan. 2, 2018, 10:18 a.m.
Add a blank line after this.	david	Jan. 2, 2018, 10:35 a.m.
Add a trailing comma on this line?	david	Jan. 2, 2018, 10:35 a.m.
Add a blank line after this.	david	Jan. 2, 2018, 10:35 a.m.
Add a blank line after this.	david	Jan. 2, 2018, 10:35 a.m.
/**	david	Jan. 2, 2018, 10:35 a.m.
/**	david	Jan. 2, 2018, 10:35 a.m.
This should be in the imperative mood ("Render the view"). This also should have a: Returns: IDoneThisIntegrationConfigFormView: This object, for …	david	Jan. 2, 2018, 10:35 a.m.
If #id_events is a child of #content-main form (which I believe it should be), you can use this.$ for a …	david	Jan. 2, 2018, 10:40 a.m.
/**	david	Jan. 2, 2018, 10:40 a.m.
This coment should probably go above the if key.startswith line.	david	Jan. 2, 2018, 11:54 p.m.

flake8 failed.

JSHint passed.

flake8

rbintegrations/idonethis/integration.py (Diff revision 1)
The issue has been resolved. Show all issues
```
W503 line break before binary operator
```
rbintegrations/idonethis/integration.py (Diff revision 1)
The issue has been resolved. Show all issues
```
E251 unexpected spaces around keyword / parameter equals
```
rbintegrations/idonethis/integration.py (Diff revision 1)
The issue has been resolved. Show all issues
```
E251 unexpected spaces around keyword / parameter equals
```

Add new integration config (Revision 1)

The issue has been resolved. Show all issues

I don't know that the word hash clarifies anything.

mudaltsov Oct. 21, 2017, 12:04 a.m.

You're right, that's an implementation detail (in the API they are called hash_id). This really applies to the first review request, so I'll change it there.

rbintegrations/idonethis/forms.py (Diff revision 1)

The issue has been resolved. Show all issues

Do we want to wrap the template strings with <code> e.g. <dt><code>...</code></dt> ?

mudaltsov Oct. 21, 2017, 12:04 a.m.

Sure, it looks better that way. I also changed from dl to table to lay things out horizontally for clarity.

Change Summary:

No longer a draft, ready for final review!

Added and updated unit tests, fixed some issues they uncovered.
Changed the template string documentation to use a table for layout.

Summary:

[DRAFT] Add event type filters and custom message templates for I Done This.

Add event type filters and custom message templates for I Done This.

Description:

		This change provides some customization for messages posted to I Done This.

		Event types can be filtered so that only some kinds of activity get posted,
		while others do not. For example, the "Reply published" event can be disabled
		to reduce noise in I Done This from every single reply getting published.

		The posted messages can also be customized independently for every event type.
		An empty message causes a fallback to the default format string, which is shown
		below the text field for reference. Parameters such as the review request ID,
		summary, and URL are substituted into placeholder strings, or omitted from the
		final message. The syntax for placeholders is from `string.Template`, which is
		limited and safer compared to other kinds of string formatting.
	+
	+	New unit tests were added to verify the added functionality, and some existing
	+	tests were updated to test individual event type filtering.

Testing Done:

~		(Unit tests are not done in this draft yet)
	~	All unit tests pass.

		Manually tested event type filtering. Choosing "All events" posts all activity
		as before. Choosing a subset of event types only posts those events.

		Also specified some custom message template strings and verified that they get
		formatted and posted to I Done This, or cause appropriate validation errors.

Diff:

Revision 2 (+667 -21)

Show changes

	rbintegrations/extension.py
	rbintegrations/idonethis/entries.py
	rbintegrations/idonethis/forms.py
	rbintegrations/idonethis/integration.py
	rbintegrations/idonethis/tests.py
	rbintegrations/static/js/idonethis/integrationConfigFormView.es6.js

File Captions:

1_add_new-integration_config.png:	Add new integration config
2_events_field_required.png:	Events field is required
3_all_events_checked.png:	Checking "All Events" disables other choices
5_message_validation.png:	Validation of template string placeholders

Added Files:

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

These are pretty much all just trivial style issues.

MU

mudaltsov Jan. 2, 2018, 10:50 a.m.
```
Thanks, all fixed.
```

rbintegrations/idonethis/forms.py (Diff revision 2)

The issue has been resolved. Show all issues

You could just have entry_type=None in the arguments list instead of using kwargs.pop here.

rbintegrations/idonethis/forms.py (Diff revision 2)
The issue has been resolved. Show all issues
```
Needs the return type (unicode)
```
rbintegrations/idonethis/forms.py (Diff revision 2)
The issue has been resolved. Show all issues
```
Add a blank line after this.
```

rbintegrations/idonethis/forms.py (Diff revision 2)

The issue has been resolved. Show all issues

Instead of str(e), this should use six.text_type(e). You can get that by adding from django.utils import six in the imports.

rbintegrations/idonethis/forms.py (Diff revision 2)
The issue has been resolved. Show all issues
```
str -> six.text_type. Also add a blank line after this.
```

rbintegrations/idonethis/forms.py (Diff revision 2)

The issue has been resolved. Show all issues

Can we rewrap this a bit?

return [
    RBIntegrationsExtension.instance.get_bundle_id(
        'idonethis-integration-config-form-view'),
]

rbintegrations/idonethis/forms.py (Diff revision 2)
The issue has been resolved. Show all issues
```
This can be dedented 4 spaces (there's only ever one return value).
```
rbintegrations/idonethis/forms.py (Diff revision 2)
The issue has been resolved. Show all issues
```
Add a blank line after this.
```
rbintegrations/idonethis/forms.py (Diff revision 2)
The issue has been resolved. Show all issues
```
Add a trailing comma on this line?
```
rbintegrations/idonethis/integration.py (Diff revision 2)
The issue has been resolved. Show all issues
```
Add a blank line after this.
```
rbintegrations/idonethis/tests.py (Diff revision 2)
The issue has been resolved. Show all issues
```
Add a blank line after this.
```
rbintegrations/static/js/idonethis/integrationConfigFormView.es6.js (Diff revision 2)
The issue has been resolved. Show all issues
```
/**
```
rbintegrations/static/js/idonethis/integrationConfigFormView.es6.js (Diff revision 2)
The issue has been resolved. Show all issues
```
/**
```

rbintegrations/static/js/idonethis/integrationConfigFormView.es6.js (Diff revision 2)

The issue has been resolved. Show all issues

This should be in the imperative mood ("Render the view").

This also should have a:

Returns:
    IDoneThisIntegrationConfigFormView:
    This object, for chaining.

mudaltsov Jan. 2, 2018, 10:50 a.m.

Some older files in Review Board also need to be updated :)
I was using reviewboard/static/rb/js/admin/views/webhookFormView.js as a template for this functionality.

rbintegrations/static/js/idonethis/integrationConfigFormView.es6.js (Diff revision 2)

The issue has been resolved. Show all issues

If #id_events is a child of #content-main form (which I believe it should be), you can use this.$ for a more efficient query.

rbintegrations/static/js/idonethis/integrationConfigFormView.es6.js (Diff revision 2)
The issue has been resolved. Show all issues
```
/**
```

Change Summary:

Addressed David's comments.

Commit:

e0204268bf5e4946c97738dc750453dd5691324d

0c2a1cd434f1bbe365edbd277642d402af387ac9

Diff:

Revision 3 (+680 -21)

Show changes

	rbintegrations/extension.py
	rbintegrations/idonethis/entries.py
	rbintegrations/idonethis/forms.py
	rbintegrations/idonethis/integration.py
	rbintegrations/idonethis/tests.py
	rbintegrations/static/js/idonethis/integrationConfigFormView.es6.js

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

```
One little nit left.
```
rbintegrations/idonethis/forms.py (Diff revision 3)
The issue has been resolved. Show all issues
```
This coment should probably go above the if key.startswith line.
```

Commit:

0c2a1cd434f1bbe365edbd277642d402af387ac9

e92b70c4b8634f96cbbb5eabc27f69d13a046447

Diff:

Revision 4 (+680 -21)

Show changes

	rbintegrations/extension.py
	rbintegrations/idonethis/entries.py
	rbintegrations/idonethis/forms.py
	rbintegrations/idonethis/integration.py
	rbintegrations/idonethis/tests.py
	rbintegrations/static/js/idonethis/integrationConfigFormView.es6.js

Checks run (2 succeeded)

flake8 passed.

JSHint passed.