File Provider framework integration with My Account page
Review Request #7144 — Created April 1, 2015 and updated
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" |
 brennie
|
|
|
"Return data for the JavaScript view." |
brennie
|
|
|
"Return" |
brennie
|
|
|
You can just do {'name': provider.name} here becuase we are only using one key-value pair per element. |
brennie
|
|
|
Could you please alphabetize this entire group of imports? |
brennie
|
|
|
"This page allows the creation..." |
brennie
|
|
|
Can you undo this and just add an extra if check at the end to call register_account_page_class on the extra … |
brennie
|
|
|
We don't want to show the file providers accounts page, even if there are no file providers? Shouldn't that page … |
brennie
|
|
|
Can we save the value of this selector to a variable so we don't have to look it up twice? |
brennie
|
|
|
What is mesasge contains arbitrary HTML? How do we know it is escaped? Also, it looks like you should be … |
brennie
|
|
|
Can you make this a constant on the object? Its cleaner and then we don't have to build the string … |
brennie
|
|
|
Same concerns re: escaping and templating as above. |
brennie
|
|
|
This too |
brennie
|
|
|
You can use inline js with <%- %> so you can just do <%- gettext('Create File Provider Account') %>. The … |
brennie
|
|
|
No blank line here. |
brennie
|
|
|
Are these expected to be public or private? If they are (pseudo-)private, then they should have a leading underscore (e.g., … |
brennie
|
|
|
There doesn't seem to be a reason to use the overhead of template processing here. |
brennie
|
|
|
Instead of using a template, use $('<html... />') |
brennie
|
|
|
All var statements should be at the top of the function. |
brennie
|
|
|
Can you unify this with the declaration up top? |
brennie
|
|
|
This should end in a semicolon. |
 david
|
|
|
In javascript, gettext strings have to be one big string (the scanner that extracts the strings doesn't understand concatenation). |
david
|
|
|
The text for this has to be passed in to the template invocation because the scanner won't know to extract … |
david
|
|
|
Same here. |
david
|
|
|
Same here. |
david
|
|
|
Same here. |
david
|
|
|
This should be run through gettext. |
david
|
|
|
This string needs to be all in one piece. |
david
|
VT
Summary: |
|
||||
|---|---|---|---|---|---|
Groups: |
|
VT
Change Summary:
Added documentation for the functions and objects in Python and JavaScript files.
Had displayMessage (previously called displayResult) to specify different types of display of messages, such as info, error or warning.
Move the initialization of options and context for Item.deleteAccount to the ItemView._deleteAccount.
Prevent submission of form from pressing the enter key to use the API submission call.
Summary: |
|
||||
|---|---|---|---|---|---|
Commit: |
|
||||
Diff: |
Revision 2 (+467 -3) |
-
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
VT
Change Summary:
Removed the 'deleteAccount' function from FileProviderAccountItem and had '_deleteAccount' in the ItemView call the destroy function on the model.
Changed an equality check from
==to===
Commit: |
|
||||
|---|---|---|---|---|---|
Diff: |
Revision 3 (+460 -3) |
-
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
-
-
-
-
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. -
reviewboard/accounts/pages.py (Diff revision 3) Could you please alphabetize this entire group of imports?
-
-
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_classon the extra page class? -
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?
-
-
What is mesasge contains arbitrary HTML? How do we know it is escaped?
Also, it looks like you should be using
_.templatehere. This allows you to do HTML escaping using<%- value %>tags. -
-
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. -
-
-
-
VT
Change Summary:
Updated with changes in response from reviews.
Updated to use or avoid _.templates as appropriate, and changed some escapes to use gettext instead of variables.
Cached some JQuery variables for better performance.
Fixed documentation and import orders in the changed Python fields.
Reverted the change with the loading of the classes, to iterate through the list once (instead of twice) and load the new one as appropriately.
Commit: |
|
||||
|---|---|---|---|---|---|
Diff: |
Revision 4 (+456 -2) |
-
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
-
-
-
reviewboard/static/rb/js/accountPrefsPage/views/fileProviderAccountsView.js (Diff revisions 3 - 4) 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. -
reviewboard/static/rb/js/accountPrefsPage/views/fileProviderAccountsView.js (Diff revisions 3 - 4) This too
-
reviewboard/static/rb/js/accountPrefsPage/views/fileProviderAccountsView.js (Diff revisions 3 - 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). -
reviewboard/static/rb/js/accountPrefsPage/views/fileProviderAccountsView.js (Diff revisions 3 - 4) Can you unify this with the declaration up top?
VT
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: |
|
||||
|---|---|---|---|---|---|
Diff: |
Revision 5 (+451 -2) |
-
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
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