Summary

Add new feature flag for the Unified Review Banner

Review Request #10187 — Created Sept. 27, 2018 and submitted Nov. 2, 2018, 4:10 p.m.

Information

Owner

shoven

Repository

Review Board

Branch

release-4.0.x

Bugs

Depends On

Commit

62d5dae...

Reviewers

Groups

reviewboard, students

People

Description

This is the first step of the larger project of creating the Unified
Review Banner Base Shell. Adding this feature flag will allow us to
toggle between the current banner experience and the new Unified Review
Banner experience to allow development on the Unified Banner to continue
without breaking the current experience.

Out of scope for this patch: using the feature flag to toggle between
the two experiences (will come in the next patch).

Testing Done


Ran all JS Unit tests (all pass)
Set the value in settings_local.py as 

ENABLED_FEATURES = {
    'reviews.unified_banner': True/False
}

and tested output of RB.EnabledFeatures.unifiedBanner in 

ReviewablePage.es6.js file (where the Unified Banner will be 

instantiated)

Issues

Description	From	Last Updated
You can remove the bit about the failing tests, as that isn't relevant context to the patch.	brennie	Sept. 27, 2018, 5:07 p.m.
To do multi-line markdown code segments, the first line (with the triple backticks) needs to be blank ``` like this …	brennie	Sept. 27, 2018, 5:07 p.m.
Can you wrap your summary & testing done at 72 characters?	brennie	Sept. 27, 2018, 5:07 p.m.
Tiny nit: summaries don't need a period.	brennie	Sept. 28, 2018, 3:16 p.m.
Also, your summary should be in the imperitive mood, i.e, Add instead of Adding.	brennie	Sept. 28, 2018, 3:16 p.m.
E302 expected 2 blank lines, found 1	reviewbot	Sept. 27, 2018, 10:47 a.m.
E501 line too long (80 > 79 characters)	reviewbot	Sept. 27, 2018, 10:47 a.m.
This should be two blank lines instead of one.	david	Nov. 1, 2018, 8:50 p.m.

flake8 failed.

JSHint passed.

flake8

reviewboard/reviews/features.py (Diff revision 1)
The issue has been resolved. Show all issues
```
E302 expected 2 blank lines, found 1
```
reviewboard/reviews/features.py (Diff revision 1)
The issue has been resolved. Show all issues
```
E501 line too long (80 > 79 characters)
```

Commit:

-	b12cd63bb886ca64ba1b8ea98e4c538e3394eac0
+	e4b7e431a22342f1d302f1a6ac066ae098a7c3ac

Diff:

Revision 2 (+18 -1)

Show changes

	reviewboard/reviews/features.py
	reviewboard/templates/base.html

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

Some small nits about your summary/testing done, otherwise this looks good

The issue has been resolved. Show all issues

You can remove the bit about the failing tests, as that isn't relevant context to the patch.

The issue has been resolved. Show all issues

To do multi-line markdown code segments, the first line (with the triple backticks) needs to be blank

```
like this
```

Otherwise it interprets:
```foo
bar
```

`` `foo bar` ``

The issue has been resolved. Show all issues

Can you wrap your summary & testing done at 72 characters?

Description:

~		This is the first step of the larger project of creating the Unified Review Banner Base Shell. Adding this feature flag will allow us to toggle between the current banner experience and the new Unified Review Banner experience to allow development on the Unified Banner to continue without breaking the current experience.
	~	This is the first step of the larger project of creating the Unified
	+	Review Banner Base Shell. Adding this feature flag will allow us to
	+	toggle between the current banner experience and the new Unified Review
	+	Banner experience to allow development on the Unified Banner to continue
	+	without breaking the current experience.

~		Out of scope for this patch: using the feature flag to toggle between the two experiences (will come in the next patch).
	~	Out of scope for this patch: using the feature flag to toggle between
	+	the two experiences (will come in the next patch).

Testing Done:

~		Ran all JS Unit tests (all pass except the 6 known to fail in release-4.0.x)
~		Set the value in settings_local.py as `ENABLED_FEATURES = { 'reviews.unified_banner': True/False }` and tested output of `RB.EnabledFeatures.unifiedBanner` in `ReviewablePage.es6.js` file (where the Unified Banner will be instantiated)
	~	Ran all JS Unit tests (all pass)
	~	Set the value in settings_local.py as
	+
	+	`ENABLED_FEATURES = {`
	+	'reviews.unified_banner': True/False
	+	}
	+
	+
	+	and tested output of `RB.EnabledFeatures.unifiedBanner` in
	+	`ReviewablePage.es6.js` file (where the Unified Banner will be
	+	instantiated)

Ship it!

```
Ship It!
```

The issue has been resolved. Show all issues
```
Tiny nit: summaries don't need a period.
```

The issue has been resolved. Show all issues

Also, your summary should be in the imperitive mood, i.e, Add instead of Adding.

Summary:

-	Adding new feature flag for the Unified Review Banner.
+	Add new feature flag for the Unified Review Banner

```
One super tiny nit!
```
reviewboard/reviews/features.py (Diff revision 2)
The issue has been resolved. Show all issues
```
This should be two blank lines instead of one.
```

Commit:

-	e4b7e431a22342f1d302f1a6ac066ae098a7c3ac
+	62d5daebc3635bfa31e9920e740a039376a511b8

Diff:

Revision 3 (+19 -1)

Show changes

	reviewboard/reviews/features.py
	reviewboard/templates/base.html

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

Ship it!

```
Ship It!
```

Status: Closed (submitted)

Change Summary:

Pushed to master (145097d)