File Provider framework integration with My Account page

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

VTL-Developer
Review Board
master
7140
634c34a...
reviewboard, students

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)
     
     
  3. reviewboard/accounts/forms/pages.py (Diff revision 3)
     
     

    "Return data for the JavaScript view."

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

    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)
     
     
     
     
     
     
     
     

    Could you please alphabetize this entire group of imports?

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

    "This page allows the creation..."

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

    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)
     
     
     

    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. Can we save the value of this selector to a variable so we don't have to look it up twice?

  10. 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. Same concerns re: escaping and templating as above.

  12. 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. There doesn't seem to be a reason to use the overhead of template processing here.

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

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

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

    "Return"

  3. 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. Are these expected to be public or private? If they are (pseudo-)private, then they should have a leading underscore (e.g., this._$accountName).

  5. Can you unify this with the declaration up top?

  6. 
      
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

Diff:

Revision 5 (+451 -2)

Show changes

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. In javascript, gettext strings have to be one big string (the scanner that extracts the strings doesn't understand concatenation).

  3. 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.

  4. This should be run through gettext.

  5. This string needs to be all in one piece.

  6. 
      
Loading...