Summary

File Provider framework integration with My Account page

Review Request #7144 — Created April 1, 2015 and updated May 20, 2015, 6:26 p.m.

Information

Owner

VTL-Developer

Repository

Review Board

Branch

master

Bugs

Depends On

7140

Commit

634c34a...

Reviewers

Groups

reviewboard, students

People

Description

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.

Testing Done

Manual testing, and debugging at each part.

Issues

Description	From	Last Updated
"view"	brennie	April 1, 2015, 8:45 p.m.
"Return data for the JavaScript view."	brennie	April 1, 2015, 8:45 p.m.
"Return"	brennie	April 10, 2015, 7:21 a.m.
You can just do {'name': provider.name} here becuase we are only using one key-value pair per element.	brennie	April 1, 2015, 8:48 p.m.
Could you please alphabetize this entire group of imports?	brennie	April 7, 2015, 7:24 a.m.
"This page allows the creation..."	brennie	April 7, 2015, 7:24 a.m.
Can you undo this and just add an extra if check at the end to call register_account_page_class on the extra …	brennie	April 7, 2015, 7:24 a.m.
We don't want to show the file providers accounts page, even if there are no file providers? Shouldn't that page …	brennie	April 7, 2015, 6:29 p.m.
Can we save the value of this selector to a variable so we don't have to look it up twice?	brennie	April 7, 2015, 7:24 a.m.
What is mesasge contains arbitrary HTML? How do we know it is escaped? Also, it looks like you should be …	brennie	April 7, 2015, 7:24 a.m.
Can you make this a constant on the object? Its cleaner and then we don't have to build the string …	brennie	April 10, 2015, 7:22 a.m.
Same concerns re: escaping and templating as above.	brennie	April 7, 2015, 7:24 a.m.
This too	brennie	April 10, 2015, 7:22 a.m.
You can use inline js with <%- %> so you can just do <%- gettext('Create File Provider Account') %>. The …	brennie	April 7, 2015, 7:28 a.m.
No blank line here.	brennie	April 7, 2015, 6:56 a.m.
Are these expected to be public or private? If they are (pseudo-)private, then they should have a leading underscore (e.g., …	brennie	April 10, 2015, 7:22 a.m.
There doesn't seem to be a reason to use the overhead of template processing here.	brennie	April 7, 2015, 6:56 a.m.
Instead of using a template, use $('<html... />')	brennie	April 7, 2015, 6:56 a.m.
All var statements should be at the top of the function.	brennie	April 1, 2015, 8:43 p.m.
Can you unify this with the declaration up top?	brennie	April 10, 2015, 7:22 a.m.
This should end in a semicolon.	david	May 20, 2015, 6:26 p.m.
In javascript, gettext strings have to be one big string (the scanner that extracts the strings doesn't understand concatenation).	david	May 20, 2015, 6:26 p.m.
The text for this has to be passed in to the template invocation because the scanner won't know to extract …	david	May 20, 2015, 6:26 p.m.
Same here.	david	May 20, 2015, 6:26 p.m.
Same here.	david	May 20, 2015, 6:26 p.m.
Same here.	david	May 20, 2015, 6:26 p.m.
This should be run through gettext.	david	May 20, 2015, 6:26 p.m.
This string needs to be all in one piece.	david	May 20, 2015, 6:26 p.m.

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

Summary:: File Provider framework integration with My Account page
[WIP] File Provider framework integration with My Account page
Groups:: students

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:

[WIP] File Provider framework integration with My Account page

File Provider framework integration with My Account page

Commit:

4a5bafa6717bad1ab2728273a36daf4450efb861

59760ce518fedad622411f97eb3352d5c293c64b

Diff:

Revision 2 (+467 -3)

Show changes

	reviewboard/staticbundles.py
	reviewboard/accounts/pages.py
	reviewboard/accounts/forms/pages.py
	reviewboard/static/rb/css/pages/my-account.less
	reviewboard/static/rb/js/accountPrefsPage/views/fileProviderAccountsView.js

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

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:

59760ce518fedad622411f97eb3352d5c293c64b

0a2ae12f47aaf09aef8ba8ccdc3d7a7cacebca71

Diff:

Revision 3 (+460 -3)

Show changes

	reviewboard/staticbundles.py
	reviewboard/accounts/pages.py
	reviewboard/accounts/forms/pages.py
	reviewboard/static/rb/css/pages/my-account.less
	reviewboard/static/rb/js/accountPrefsPage/views/fileProviderAccountsView.js

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)
The issue has been resolved. Show all issues
```
"view"
```
reviewboard/accounts/forms/pages.py (Diff revision 3)
The issue has been resolved. Show all issues
```
"Return data for the JavaScript view."
```

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

The issue has been resolved. Show all issues

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)

The issue has been resolved. Show all issues

Could you please alphabetize this entire group of imports?

reviewboard/accounts/pages.py (Diff revision 3)
The issue has been resolved. Show all issues
```
"This page allows the creation..."
```

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

The issue has been resolved. 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?

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

The issue has been dropped. 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?

VTL-Developer April 7, 2015, 4:16 p.m.

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.

reviewboard/static/rb/js/accountPrefsPage/views/fileProviderAccountsView.js (Diff revision 3)

The issue has been resolved. Show all issues

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

reviewboard/static/rb/js/accountPrefsPage/views/fileProviderAccountsView.js (Diff revision 3)

The issue has been resolved. 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.

reviewboard/static/rb/js/accountPrefsPage/views/fileProviderAccountsView.js (Diff revision 3)

The issue has been resolved. Show all issues

Same concerns re: escaping and templating as above.

reviewboard/static/rb/js/accountPrefsPage/views/fileProviderAccountsView.js (Diff revision 3)

The issue has been resolved. 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.

reviewboard/static/rb/js/accountPrefsPage/views/fileProviderAccountsView.js (Diff revision 3)
The issue has been resolved. Show all issues
```
No blank line here.
```

reviewboard/static/rb/js/accountPrefsPage/views/fileProviderAccountsView.js (Diff revision 3)

The issue has been resolved. Show all issues

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

reviewboard/static/rb/js/accountPrefsPage/views/fileProviderAccountsView.js (Diff revision 3)
The issue has been resolved. Show all issues
```
Instead of using a template, use $('<html... />')
```
reviewboard/static/rb/js/accountPrefsPage/views/fileProviderAccountsView.js (Diff revision 3)
The issue has been resolved. Show all issues
```
All var statements should be at the top of the function.
```

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:

0a2ae12f47aaf09aef8ba8ccdc3d7a7cacebca71

c332fb8dfe56454f05281b38a27f76e4e25c7b60

Diff:

Revision 4 (+456 -2)

Show changes

	reviewboard/staticbundles.py
	reviewboard/accounts/pages.py
	reviewboard/accounts/forms/pages.py
	reviewboard/static/rb/css/pages/my-account.less
	reviewboard/static/rb/js/accountPrefsPage/views/fileProviderAccountsView.js

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 revisions 3 - 4)
The issue has been resolved. Show all issues
```
"Return"
```

reviewboard/static/rb/js/accountPrefsPage/views/fileProviderAccountsView.js (Diff revisions 3 - 4)

The issue has been resolved. 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.

reviewboard/static/rb/js/accountPrefsPage/views/fileProviderAccountsView.js (Diff revisions 3 - 4)

The issue has been resolved. Show all issues

This too

reviewboard/static/rb/js/accountPrefsPage/views/fileProviderAccountsView.js (Diff revisions 3 - 4)

The issue has been resolved. 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).

reviewboard/static/rb/js/accountPrefsPage/views/fileProviderAccountsView.js (Diff revisions 3 - 4)

The issue has been resolved. Show all issues

Can you unify this with the declaration up top?

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

	reviewboard/staticbundles.py
	reviewboard/accounts/pages.py
	reviewboard/accounts/forms/pages.py
	reviewboard/static/rb/css/pages/my-account.less
	reviewboard/static/rb/js/accountPrefsPage/views/fileProviderAccountsView.js

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

Fix it!

reviewboard/static/rb/js/accountPrefsPage/views/fileProviderAccountsView.js (Diff revision 5)
An issue was opened. Show all issues
```
This should end in a semicolon.
```

reviewboard/static/rb/js/accountPrefsPage/views/fileProviderAccountsView.js (Diff revision 5)

An issue was opened. Show all issues

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

reviewboard/static/rb/js/accountPrefsPage/views/fileProviderAccountsView.js (Diff revision 5)

An issue was opened. 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.

reviewboard/static/rb/js/accountPrefsPage/views/fileProviderAccountsView.js (Diff revision 5)
An issue was opened. Show all issues
```
Same here.
```
reviewboard/static/rb/js/accountPrefsPage/views/fileProviderAccountsView.js (Diff revision 5)
An issue was opened. Show all issues
```
Same here.
```
reviewboard/static/rb/js/accountPrefsPage/views/fileProviderAccountsView.js (Diff revision 5)
An issue was opened. Show all issues
```
Same here.
```
reviewboard/static/rb/js/accountPrefsPage/views/fileProviderAccountsView.js (Diff revision 5)
An issue was opened. Show all issues
```
This should be run through gettext.
```

reviewboard/static/rb/js/accountPrefsPage/views/fileProviderAccountsView.js (Diff revision 5)

An issue was opened. Show all issues

This string needs to be all in one piece.