Summary

Add a support status banner to the admin dashboard

Review Request #8064 — Created March 17, 2016 and submitted March 29, 2016, 7:57 p.m.

Information

Owner

brennie

Repository

Review Board

Branch

release-2.5.x

Bugs

Depends On

~~8065~~

Reviewers

Groups

reviewboard

People

Description

The support banner shows the current support status, via
beanbaginc.com's API. While the data is being retrieved, a spinner is
being shown.

Testing Done

Tested the banner with:

an active support contract;
an inactive support contract; and
no support contract.

Files

Issues

Description	From	Last Updated
Style-wise, it'd be nice to actually have this appear like a draft banner. Something flush against the left, top, and …	chipx86	March 23, 2016, 12:02 p.m.
Undo this change.	david	March 18, 2016, 4:35 p.m.
Add another blank line here.	david	March 18, 2016, 4:35 p.m.
Alignment is a little funny here. Since there's only one item in the object, can you put that all on …	david	March 18, 2016, 4:35 p.m.
Doc comment?	david	March 18, 2016, 4:33 p.m.
Doc comments throughout?	david	March 18, 2016, 4:36 p.m.
Can you use $.param to format the querystring args?	david	March 18, 2016, 4:29 p.m.
Can you assert that instance is null?	david	March 18, 2016, 4:28 p.m.
You can flip this around and default options into your new object, which makes it so you can skip the …	david	March 18, 2016, 4:27 p.m.
Since this is global, instead of assigning to var, we probably should be doing window.SUPPORT_DATA = ....	david	March 18, 2016, 4:37 p.m.
Alphabetical order.	chipx86	March 22, 2016, 8:03 p.m.
Maybe I'm thinking a different language, but I thought you had to wrap new Blah in parens if you wanted …	chipx86	March 22, 2016, 8:04 p.m.
Rather than doing this and building the script yourself, you can just do: $.ajax({ url: RB.SupportBannerView.supportURL, data: { 'support-data': SUPPORT_DATA, …	chipx86	March 22, 2016, 8:06 p.m.
This isn't a boolean. If we want to check it against an empty string, we should do that explicitly.	chipx86	March 22, 2016, 8:07 p.m.
You can just do var SUPPORT_DATA, rather than explicitly attaching it to window.	chipx86	March 22, 2016, 8:07 p.m.
Typo. =	chipx86	March 22, 2016, 8:08 p.m.
Is the alignment a bit off here? Shouldn't it be 2 spaces in, not 4?	imadueme	March 28, 2016, 1:22 p.m.
Is the alignment a bit off here? Shouldn't it be 2 spaces in, not 4?	imadueme	March 28, 2016, 1:25 p.m.
Doesn't look like you added this, but, are vim preferences supposed to be in here?	imadueme	March 28, 2016, 1:22 p.m.
Blank line in between these.	david	March 29, 2016, 7:56 p.m.

Tool: Pyflakes
Processed Files:
    reviewboard/admin/views.py
    reviewboard/staticbundles.py

Ignored Files:
    reviewboard/static/rb/js/admin/admin.js
    reviewboard/static/rb/css/pages/admin-dashboard.less
    reviewboard/static/rb/js/admin/views/webhookFormView.js
    reviewboard/static/rb/js/admin/models/supportContractModel.js
    reviewboard/static/lib/js/backbone-1.0.0.min.js
    reviewboard/templates/admin/dashboard.html
    reviewboard/static/rb/js/admin/views/supportBannerView.js
    reviewboard/static/rb/js/admin/repositoryform.js



Tool: PEP8 Style Checker
Processed Files:
    reviewboard/admin/views.py
    reviewboard/staticbundles.py

Ignored Files:
    reviewboard/static/rb/js/admin/admin.js
    reviewboard/static/rb/css/pages/admin-dashboard.less
    reviewboard/static/rb/js/admin/views/webhookFormView.js
    reviewboard/static/rb/js/admin/models/supportContractModel.js
    reviewboard/static/lib/js/backbone-1.0.0.min.js
    reviewboard/templates/admin/dashboard.html
    reviewboard/static/rb/js/admin/views/supportBannerView.js
    reviewboard/static/rb/js/admin/repositoryform.js

Description:: The support banner shows the current support status, via
beanbaginc.com's API. While the data is being retrieved, a spinner is
being shown.
+
+
This patch also reorganizes all the admin JavaScript into the
+ js/admin/ directory.

Can you move the admin.js refactor into a separate change?

It also looks like backbone-1.0.0.min.js is no longer a minified file. What happened there?

brennie

March 18, 2016, 4:36 p.m.
```
That was an accident.
```

reviewboard/admin/views.py (Diff revision 1)
The issue has been resolved. Show all issues
```
Undo this change.
```
reviewboard/static/rb/css/pages/admin-dashboard.less (Diff revision 1)
The issue has been resolved. Show all issues
```
Add another blank line here.
```

reviewboard/static/rb/js/admin/admin.js (Diff revision 1)

The issue has been resolved. Show all issues

Alignment is a little funny here. Since there's only one item in the object, can you put that all on one line?

reviewboard/static/rb/js/admin/models/supportContractModel.js (Diff revision 1)
The issue has been resolved. Show all issues
```
Doc comment?
```
reviewboard/static/rb/js/admin/views/supportBannerView.js (Diff revision 1)
The issue has been resolved. Show all issues
```
Doc comments throughout?
```
reviewboard/static/rb/js/admin/views/supportBannerView.js (Diff revision 1)
The issue has been resolved. Show all issues
```
Can you use $.param to format the querystring args?
```
reviewboard/static/rb/js/admin/views/supportBannerView.js (Diff revision 1)
The issue has been resolved. Show all issues
```
Can you assert that instance is null?
```

reviewboard/static/rb/js/admin/views/supportBannerView.js (Diff revision 1)

The issue has been resolved. Show all issues

You can flip this around and default options into your new object, which makes it so you can skip the || {} part.

reviewboard/templates/admin/dashboard.html (Diff revision 1)

The issue has been resolved. Show all issues

Since this is global, instead of assigning to var, we probably should be doing window.SUPPORT_DATA = ....

Description:

		The support banner shows the current support status, via
		beanbaginc.com's API. While the data is being retrieved, a spinner is
		being shown.
-
-		This patch also reorganizes all the admin JavaScript into the
-		`js/admin/` directory.

Depends On:

~~8065 - Refactor admin JS~~

Diff:

Revision 2 (+105 -3)

Show changes

	reviewboard/staticbundles.py
	reviewboard/admin/views.py
	reviewboard/static/rb/css/pages/admin-dashboard.less
	reviewboard/static/rb/js/admin/admin.js
	reviewboard/static/rb/js/admin/views/supportBannerView.js

Tool: PEP8 Style Checker
Processed Files:
    reviewboard/admin/views.py
    reviewboard/staticbundles.py

Ignored Files:
    reviewboard/static/rb/js/admin/admin.js
    reviewboard/static/rb/css/pages/admin-dashboard.less
    reviewboard/static/rb/js/admin/views/supportBannerView.js



Tool: Pyflakes
Processed Files:
    reviewboard/admin/views.py
    reviewboard/staticbundles.py

Ignored Files:
    reviewboard/static/rb/js/admin/admin.js
    reviewboard/static/rb/css/pages/admin-dashboard.less
    reviewboard/static/rb/js/admin/views/supportBannerView.js

Diff:

Revision 3 (+122 -7)

Show changes

	reviewboard/staticbundles.py
	reviewboard/admin/views.py
	reviewboard/static/rb/css/pages/admin-dashboard.less
	reviewboard/static/rb/js/admin.js
	reviewboard/static/rb/js/repositoryform.js
	reviewboard/static/rb/js/admin/views/supportBannerView.js
	reviewboard/static/rb/js/webhooksAdmin/views/webhookFormView.js
	reviewboard/templates/admin/dashboard.html

Tool: Pyflakes
Processed Files:
    reviewboard/admin/views.py
    reviewboard/staticbundles.py

Ignored Files:
    reviewboard/static/rb/js/admin/admin.js
    reviewboard/static/rb/css/pages/admin-dashboard.less
    reviewboard/static/rb/js/admin/views/webhookFormView.js
    reviewboard/templates/admin/dashboard.html
    reviewboard/static/rb/js/admin/views/supportBannerView.js
    reviewboard/static/rb/js/admin/repositoryform.js



Tool: PEP8 Style Checker
Processed Files:
    reviewboard/admin/views.py
    reviewboard/staticbundles.py

Ignored Files:
    reviewboard/static/rb/js/admin/admin.js
    reviewboard/static/rb/css/pages/admin-dashboard.less
    reviewboard/static/rb/js/admin/views/webhookFormView.js
    reviewboard/templates/admin/dashboard.html
    reviewboard/static/rb/js/admin/views/supportBannerView.js
    reviewboard/static/rb/js/admin/repositoryform.js

Did you not post with the right parent commit?

Change Summary:

Upload the correct set of commits

Diff:

Revision 4 (+118 -3)

Show changes

	reviewboard/staticbundles.py
	reviewboard/admin/views.py
	reviewboard/static/rb/css/pages/admin-dashboard.less
	reviewboard/static/rb/js/admin/admin.js
	reviewboard/static/rb/js/admin/views/supportBannerView.js
	reviewboard/templates/admin/dashboard.html

Tool: Pyflakes
Processed Files:
    reviewboard/admin/views.py
    reviewboard/staticbundles.py

Ignored Files:
    reviewboard/static/rb/js/admin/admin.js
    reviewboard/static/rb/css/pages/admin-dashboard.less
    reviewboard/templates/admin/dashboard.html
    reviewboard/static/rb/js/admin/views/supportBannerView.js



Tool: PEP8 Style Checker
Processed Files:
    reviewboard/admin/views.py
    reviewboard/staticbundles.py

Ignored Files:
    reviewboard/static/rb/js/admin/admin.js
    reviewboard/static/rb/css/pages/admin-dashboard.less
    reviewboard/templates/admin/dashboard.html
    reviewboard/static/rb/js/admin/views/supportBannerView.js

reviewboard/static/rb/css/pages/admin-dashboard.less (Diff revision 4)

The issue has been resolved. Show all issues

Alphabetical order.

reviewboard/static/rb/js/admin/admin.js (Diff revision 4)

The issue has been resolved. Show all issues

Maybe I'm thinking a different language, but I thought you had to wrap new Blah in parens if you wanted to immediately call a function on it?
I think I'd prefer the standard thing of two statements: One instantiating, one rendering. Easier to debug if things go wrong, too.

reviewboard/static/rb/js/admin/views/supportBannerView.js (Diff revision 4)

The issue has been resolved. Show all issues

Rather than doing this and building the script yourself, you can just do:

$.ajax({
    url: RB.SupportBannerView.supportURL,
    data: {
        'support-data': SUPPORT_DATA,
    },
    dataType: 'jsonp',
    jsonpCallback: 'RB.SupportBannerView.instance.receive'
});

That will handle all the work of doing the JSONP stuff, along with handling cache busting.

reviewboard/static/rb/js/admin/views/supportBannerView.js (Diff revision 4)
The issue has been resolved. Show all issues
```
This isn't a boolean. If we want to check it against an empty string, we should do that explicitly.
```
reviewboard/templates/admin/dashboard.html (Diff revision 4)
The issue has been resolved. Show all issues
```
You can just do var SUPPORT_DATA, rather than explicitly attaching it to window.
```
reviewboard/templates/admin/dashboard.html (Diff revision 4)
The issue has been resolved. Show all issues
```
Typo. =
```

basic.png (Revision 1)

The issue has been resolved. Show all issues

Style-wise, it'd be nice to actually have this appear like a draft banner. Something flush against the left, top, and right sides. No header.

Diff:

Revision 5 (+202 -8)

Show changes

	reviewboard/staticbundles.py
	reviewboard/admin/views.py
	reviewboard/static/rb/css/defs.less
	reviewboard/static/rb/css/pages/admin-dashboard.less
	reviewboard/static/rb/css/ui/boxes.less
	reviewboard/static/rb/js/admin/admin.js
	reviewboard/static/rb/js/admin/views/supportBannerView.js
	reviewboard/templates/admin/dashboard.html

Removed Files:

Added Files:

Tool: Pyflakes
Processed Files:
    reviewboard/admin/views.py
    reviewboard/staticbundles.py

Ignored Files:
    reviewboard/static/rb/js/admin/admin.js
    reviewboard/static/rb/css/pages/admin-dashboard.less
    reviewboard/static/rb/css/ui/boxes.less
    reviewboard/templates/admin/dashboard.html
    reviewboard/static/rb/js/admin/views/supportBannerView.js
    reviewboard/static/rb/css/defs.less



Tool: PEP8 Style Checker
Processed Files:
    reviewboard/admin/views.py
    reviewboard/staticbundles.py

Ignored Files:
    reviewboard/static/rb/js/admin/admin.js
    reviewboard/static/rb/css/pages/admin-dashboard.less
    reviewboard/static/rb/css/ui/boxes.less
    reviewboard/templates/admin/dashboard.html
    reviewboard/static/rb/js/admin/views/supportBannerView.js
    reviewboard/static/rb/css/defs.less

Added Files:

reviewboard/static/rb/css/pages/admin-dashboard.less (Diff revision 5)
The issue has been dropped. Show all issues
```
Is the alignment a bit off here? Shouldn't it be 2 spaces in, not 4?
```
reviewboard/static/rb/css/pages/admin-dashboard.less (Diff revision 5)
The issue has been resolved. Show all issues
```
Is the alignment a bit off here? Shouldn't it be 2 spaces in, not 4?
```
reviewboard/static/rb/css/pages/admin-dashboard.less (Diff revision 5)
The issue has been dropped. Show all issues
```
Doesn't look like you added this, but, are vim preferences supposed to be in here?
```
1. brennie
  
  March 28, 2016, 1:22 p.m.
  I'm just gonna leave it in.

Change Summary:

Address Israel's issues.

Diff:

Revision 6 (+123 -34)

Show changes

	reviewboard/static/rb/css/defs.less
	reviewboard/static/rb/css/pages/admin-dashboard.less
	reviewboard/static/rb/css/ui/boxes.less
	reviewboard/static/rb/js/admin/views/supportBannerView.js

Tool: PEP8 Style Checker
Ignored Files:
    reviewboard/static/rb/css/pages/admin-dashboard.less
    reviewboard/static/rb/js/admin/views/supportBannerView.js
    reviewboard/static/rb/css/defs.less
    reviewboard/static/rb/css/ui/boxes.less



Tool: Pyflakes
Ignored Files:
    reviewboard/static/rb/css/pages/admin-dashboard.less
    reviewboard/static/rb/js/admin/views/supportBannerView.js
    reviewboard/static/rb/css/defs.less
    reviewboard/static/rb/css/ui/boxes.less

Diff:

Revision 7 (+123 -34)

Show changes

	reviewboard/static/rb/css/defs.less
	reviewboard/static/rb/css/pages/admin-dashboard.less
	reviewboard/static/rb/css/ui/boxes.less
	reviewboard/static/rb/js/admin/views/supportBannerView.js

Tool: Pyflakes
Ignored Files:
    reviewboard/static/rb/css/pages/admin-dashboard.less
    reviewboard/static/rb/js/admin/views/supportBannerView.js
    reviewboard/static/rb/css/defs.less
    reviewboard/static/rb/css/ui/boxes.less



Tool: PEP8 Style Checker
Ignored Files:
    reviewboard/static/rb/css/pages/admin-dashboard.less
    reviewboard/static/rb/js/admin/views/supportBannerView.js
    reviewboard/static/rb/css/defs.less
    reviewboard/static/rb/css/ui/boxes.less

Ship it!

reviewboard/static/rb/css/pages/admin-dashboard.less (Diff revision 7)
The issue has been resolved. Show all issues
```
Blank line in between these.
```

Status:: Completed
Change Summary:: Pushed to release-2.5.x (6a85766)