File Provider framework integration with My Account page

Review Request #7144 — Created April 1, 2015 and updated

Information

Review Board
master
634c34a...

Reviewers

If a user wants to use an online file provider to upload files to a review request, they need to be able to create accounts and make changes as necessary. This would be allowed through the My Account page of a user. On this page, users would be able to see a list of all of their file provider accounts and delete any as necessarry. There is also a form for the user to create a new file provider account by select a provider name from the drop-menu and typing in an account name to associate with it.

All of the creations and deletions are done through web api calls, instead of form submissions to the server. As such, the calls would have error or success messages on screen.

Manual testing, and debugging at each part.

Description From Last Updated

"view"

brenniebrennie

"Return data for the JavaScript view."

brenniebrennie

"Return"

brenniebrennie

You can just do {'name': provider.name} here becuase we are only using one key-value pair per element.

brenniebrennie

Could you please alphabetize this entire group of imports?

brenniebrennie

"This page allows the creation..."

brenniebrennie

Can you undo this and just add an extra if check at the end to call register_account_page_class on the extra …

brenniebrennie

We don't want to show the file providers accounts page, even if there are no file providers? Shouldn't that page …

brenniebrennie

Can we save the value of this selector to a variable so we don't have to look it up twice?

brenniebrennie

What is mesasge contains arbitrary HTML? How do we know it is escaped? Also, it looks like you should be …

brenniebrennie

Can you make this a constant on the object? Its cleaner and then we don't have to build the string …

brenniebrennie

Same concerns re: escaping and templating as above.

brenniebrennie

This too

brenniebrennie

You can use inline js with <%- %> so you can just do <%- gettext('Create File Provider Account') %>. The …

brenniebrennie

No blank line here.

brenniebrennie

Are these expected to be public or private? If they are (pseudo-)private, then they should have a leading underscore (e.g., …

brenniebrennie

There doesn't seem to be a reason to use the overhead of template processing here.

brenniebrennie

Instead of using a template, use $('<html... />')

brenniebrennie

All var statements should be at the top of the function.

brenniebrennie

Can you unify this with the declaration up top?

brenniebrennie

This should end in a semicolon.

daviddavid

In javascript, gettext strings have to be one big string (the scanner that extracts the strings doesn't understand concatenation).

daviddavid

The text for this has to be passed in to the template invocation because the scanner won't know to extract …

daviddavid

Same here.

daviddavid

Same here.

daviddavid

Same here.

daviddavid

This should be run through gettext.

daviddavid

This string needs to be all in one piece.

daviddavid
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        reviewboard/accounts/pages.py
        reviewboard/accounts/forms/pages.py
        reviewboard/staticbundles.py
    
    Ignored Files:
        reviewboard/static/rb/js/accountPrefsPage/views/fileProviderAccountsView.js
        reviewboard/static/rb/css/pages/my-account.less
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/accounts/pages.py
        reviewboard/accounts/forms/pages.py
        reviewboard/staticbundles.py
    
    Ignored Files:
        reviewboard/static/rb/js/accountPrefsPage/views/fileProviderAccountsView.js
        reviewboard/static/rb/css/pages/my-account.less
    
    
  2. 
      
VT
VT
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        reviewboard/accounts/pages.py
        reviewboard/accounts/forms/pages.py
        reviewboard/staticbundles.py
    
    Ignored Files:
        reviewboard/static/rb/js/accountPrefsPage/views/fileProviderAccountsView.js
        reviewboard/static/rb/css/pages/my-account.less
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/accounts/pages.py
        reviewboard/accounts/forms/pages.py
        reviewboard/staticbundles.py
    
    Ignored Files:
        reviewboard/static/rb/js/accountPrefsPage/views/fileProviderAccountsView.js
        reviewboard/static/rb/css/pages/my-account.less
    
    
  2. 
      
VT
reviewbot
  1. Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/accounts/pages.py
        reviewboard/accounts/forms/pages.py
        reviewboard/staticbundles.py
    
    Ignored Files:
        reviewboard/static/rb/js/accountPrefsPage/views/fileProviderAccountsView.js
        reviewboard/static/rb/css/pages/my-account.less
    
    
    
    Tool: Pyflakes
    Processed Files:
        reviewboard/accounts/pages.py
        reviewboard/accounts/forms/pages.py
        reviewboard/staticbundles.py
    
    Ignored Files:
        reviewboard/static/rb/js/accountPrefsPage/views/fileProviderAccountsView.js
        reviewboard/static/rb/css/pages/my-account.less
    
    
  2. 
      
brennie
  1. 
      
  2. reviewboard/accounts/forms/pages.py (Diff revision 3)
     
     
    Show all issues

    "view"

  3. reviewboard/accounts/forms/pages.py (Diff revision 3)
     
     
    Show all issues

    "Return data for the JavaScript view."

  4. reviewboard/accounts/forms/pages.py (Diff revision 3)
     
     
     
     
    Show all issues

    You can just do {'name': provider.name} here becuase we are only using one key-value pair per element.

  5. reviewboard/accounts/pages.py (Diff revision 3)
     
     
     
     
     
     
     
     
    Show all issues

    Could you please alphabetize this entire group of imports?

  6. reviewboard/accounts/pages.py (Diff revision 3)
     
     
    Show all issues

    "This page allows the creation..."

  7. reviewboard/accounts/pages.py (Diff revision 3)
     
     
     
     
     
     
     
     
     
    Show all issues

    Can you undo this and just add an extra if check at the end to call register_account_page_class on the extra page class?

  8. reviewboard/accounts/pages.py (Diff revision 3)
     
     
     
    Show all issues

    We don't want to show the file providers accounts page, even if there are no file providers? Shouldn't that page just say there are none?

    1. With the file providers account page, you want to be able to make changes to your account. Whether they are create, delete or edit. However, if there are no file providers supported, you cannot do any of those things and it'll just be a blank page. So removing the page from the UI seems like a good option unless the there's a supported provider for users to create and interact with.

  9. Show all issues

    Can we save the value of this selector to a variable so we don't have to look it up twice?

  10. Show all issues

    What is mesasge contains arbitrary HTML? How do we know it is escaped?

    Also, it looks like you should be using _.template here. This allows you to do HTML escaping using <%- value %> tags.

  11. Show all issues

    Same concerns re: escaping and templating as above.

  12. Show all issues

    You can use inline js with <%- %> so you can just do <%- gettext('Create File Provider Account') %>.

    The <%- %> tags guarantee the value is escaped, so please use those elsewhere.

  13. Show all issues

    No blank line here.

  14. Show all issues

    There doesn't seem to be a reason to use the overhead of template processing here.

  15. Show all issues

    Instead of using a template, use $('<html... />')

  16. Show all issues

    All var statements should be at the top of the function.

  17. 
      
VT
reviewbot
  1. Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/accounts/pages.py
        reviewboard/accounts/forms/pages.py
        reviewboard/staticbundles.py
    
    Ignored Files:
        reviewboard/static/rb/js/accountPrefsPage/views/fileProviderAccountsView.js
        reviewboard/static/rb/css/pages/my-account.less
    
    
    
    Tool: Pyflakes
    Processed Files:
        reviewboard/accounts/pages.py
        reviewboard/accounts/forms/pages.py
        reviewboard/staticbundles.py
    
    Ignored Files:
        reviewboard/static/rb/js/accountPrefsPage/views/fileProviderAccountsView.js
        reviewboard/static/rb/css/pages/my-account.less
    
    
  2. 
      
brennie
  1. 
      
  2. reviewboard/accounts/forms/pages.py (Diff revisions 3 - 4)
     
     
    Show all issues

    "Return"

  3. Show all issues

    Can you make this a constant on the object? Its cleaner and then we don't have to build the string each time we call displayMessage.

  4. Show all issues

    This too

  5. Show all issues

    Are these expected to be public or private? If they are (pseudo-)private, then they should have a leading underscore (e.g., this._$accountName).

  6. Show all issues

    Can you unify this with the declaration up top?

  7. 
      
VT
Review request changed
Change Summary:

Fixed grammar error in documentation.

The function displayMessage has a preconstructed template for outputting the message.

Fixed silly mistake with declaring a variable and assigning the value later.

Commit:
c332fb8dfe56454f05281b38a27f76e4e25c7b60
634c34a74787d06137a56f64f0df5b80133fd26b
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        reviewboard/accounts/pages.py
        reviewboard/accounts/forms/pages.py
        reviewboard/staticbundles.py
    
    Ignored Files:
        reviewboard/static/rb/js/accountPrefsPage/views/fileProviderAccountsView.js
        reviewboard/static/rb/css/pages/my-account.less
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/accounts/pages.py
        reviewboard/accounts/forms/pages.py
        reviewboard/staticbundles.py
    
    Ignored Files:
        reviewboard/static/rb/js/accountPrefsPage/views/fileProviderAccountsView.js
        reviewboard/static/rb/css/pages/my-account.less
    
    
  2. 
      
david
  1. 
      
  2. Show all issues

    This should end in a semicolon.

  3. Show all issues

    In javascript, gettext strings have to be one big string (the scanner that extracts the strings doesn't understand concatenation).

  4. Show all issues

    The text for this has to be passed in to the template invocation because the scanner won't know to extract the string from within a template.

  5. Show all issues

    This should be run through gettext.

  6. Show all issues

    This string needs to be all in one piece.

  7.