Add a support status banner to the admin dashboard

Review Request #8064 — Created March 17, 2016 and submitted

Information

Review Board
release-2.5.x

Reviewers

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

Tested the banner with:

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

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 …

chipx86chipx86

Undo this change.

daviddavid

Add another blank line here.

daviddavid

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

daviddavid

Doc comment?

daviddavid

Doc comments throughout?

daviddavid

Can you use $.param to format the querystring args?

daviddavid

Can you assert that instance is null?

daviddavid

You can flip this around and default options into your new object, which makes it so you can skip the …

daviddavid

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

daviddavid

Alphabetical order.

chipx86chipx86

Maybe I'm thinking a different language, but I thought you had to wrap new Blah in parens if you wanted …

chipx86chipx86

Rather than doing this and building the script yourself, you can just do: $.ajax({ url: RB.SupportBannerView.supportURL, data: { 'support-data': SUPPORT_DATA, …

chipx86chipx86

This isn't a boolean. If we want to check it against an empty string, we should do that explicitly.

chipx86chipx86

You can just do var SUPPORT_DATA, rather than explicitly attaching it to window.

chipx86chipx86

Typo. =

chipx86chipx86

Is the alignment a bit off here? Shouldn't it be 2 spaces in, not 4?

imaduemeimadueme

Is the alignment a bit off here? Shouldn't it be 2 spaces in, not 4?

imaduemeimadueme

Doesn't look like you added this, but, are vim preferences supposed to be in here?

imaduemeimadueme

Blank line in between these.

daviddavid
reviewbot
  1. 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
    
    
  2. 
      
brennie
david
  1. 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?

    1. That was an accident.

  2. reviewboard/admin/views.py (Diff revision 1)
     
     
    Show all issues

    Undo this change.

  3. Show all issues

    Add another blank line here.

  4. reviewboard/static/rb/js/admin/admin.js (Diff revision 1)
     
     
     
     
     
    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?

  5. Show all issues

    Doc comment?

  6. Show all issues

    Doc comments throughout?

  7. Show all issues

    Can you use $.param to format the querystring args?

  8. Show all issues

    Can you assert that instance is null?

  9. Show all issues

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

  10. Show all issues

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

  11. 
      
brennie
reviewbot
  1. 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
    
    
  2. 
      
brennie
reviewbot
  1. 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
    
    
  2. 
      
david
  1. Did you not post with the right parent commit?

  2. 
      
brennie
reviewbot
  1. 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
    
    
  2. 
      
chipx86
  1. 
      
  2. reviewboard/static/rb/css/pages/admin-dashboard.less (Diff revision 4)
     
     
     
     
     
     
    Show all issues

    Alphabetical order.

  3. reviewboard/static/rb/js/admin/admin.js (Diff revision 4)
     
     
    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.

  4. reviewboard/static/rb/js/admin/views/supportBannerView.js (Diff revision 4)
     
     
     
     
     
     
    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.

  5. Show all issues

    This isn't a boolean. If we want to check it against an empty string, we should do that explicitly.

  6. Show all issues

    You can just do var SUPPORT_DATA, rather than explicitly attaching it to window.

  7. Show all issues

    Typo. =

  8. 
      
chipx86
  1. 
      
  2. 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.

  3. 
      
brennie
reviewbot
  1. 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
    
    
  2. 
      
brennie
imadueme
  1. 
      
  2. Show all issues

    Is the alignment a bit off here? Shouldn't it be 2 spaces in, not 4?

  3. Show all issues

    Is the alignment a bit off here? Shouldn't it be 2 spaces in, not 4?

  4. Show all issues

    Doesn't look like you added this, but, are vim preferences supposed to be in here?

    1. I'm just gonna leave it in.

  5. 
      
brennie
reviewbot
  1. 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
    
    
  2. 
      
brennie
reviewbot
  1. 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
    
    
  2. 
      
david
  1. 
      
  2. Show all issues

    Blank line in between these.

  3. 
      
brennie
Review request changed
Status:
Completed
Change Summary:
Pushed to release-2.5.x (6a85766)