Add a support status banner to the admin dashboard

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

brennie
Review Board
release-2.5.x
8065
reviewboard

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.
Loading file attachments...

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)
     
     

    Undo this change.

  3. Add another blank line here.

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

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

  5. Doc comments throughout?

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

  7. Can you assert that instance is null?

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

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

  10. 
      
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)
     
     
     
     
     
     

    Alphabetical order.

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

    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)
     
     
     
     
     
     

    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. This isn't a boolean. If we want to check it against an empty string, we should do that explicitly.

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

  7. 
      
chipx86
  1. 
      
  2. 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. Is the alignment a bit off here? Shouldn't it be 2 spaces in, not 4?

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

  4. 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. Blank line in between these.

  3. 
      
brennie
Review request changed

Status: Closed (submitted)

Change Summary:

Pushed to release-2.5.x (6a85766)
Loading...