Summary

[OAuth2Provider] Add OAuth2 client app listing page and form into account preference page

Review Request #7900 — Created Jan. 20, 2016 and discarded Sept. 7, 2017, 9 p.m.

Information

Owner

lehoangm

Repository

Review Board

Branch

master

Bugs

Depends On

~~7897~~

Blocks

7941

Reviewers

Groups

reviewboard

People

brennie, chipx86

Description

Add a starting point for user to view all of their OAuth2 client apps.
Similar to the group page, user can search for certain OAuth2 client
rapplications. The page will ask for user confirmation if user want to
remove a client app. The page also display a nonfunctional form for user to edit
and add new application. There is no backend support for all operations

Testing Done

Manually open the page with 1 OAuth client app and type to filter it out.

I cannot find any automate test for the page and form as well as the BackboneJS part.

Files

Issues

Description	From	Last Updated
How about OAuth2AppForm?	brennie	Jan. 22, 2016, 9:29 p.m.
"This form"	brennie	Jan. 21, 2016, 8:40 p.m.
"view"	brennie	Jan. 21, 2016, 8:40 p.m.
Mention that the JS will be responsible for doing this over the API?	brennie	Feb. 13, 2016, 6:02 p.m.
"Get" -> "Return" Missing a Returns block, e.g. ```python """... Returns: dict: An explanation of the return value. """	brennie	Jan. 21, 2016, 8:56 p.m.
We can express this in a more Pythonic fashion by using a list comprehension: serialized_oauth2_client_apps = [ self._serialize_oauth2_client_app(client_app) for client_app …	brennie	Jan. 21, 2016, 9 p.m.
How about serialized_apps ?	brennie	Jan. 21, 2016, 9 p.m.
_serialize_app? Needs a docstring.	brennie	Jan. 21, 2016, 9:04 p.m.
How about OAuth2Page ?	brennie	Jan. 22, 2016, 9:30 p.m.
Needs a doc-comment.	brennie	Jan. 21, 2016, 9:27 p.m.
Needs a doc comment.	brennie	Jan. 21, 2016, 9:27 p.m.
Needs a doc comment.	brennie	Jan. 21, 2016, 9:27 p.m.
Needs a doc comment.	brennie	Jan. 21, 2016, 9:27 p.m.
Needs a doc comment.	brennie	Jan. 21, 2016, 9:27 p.m.
This would be better formatted as: ```javascript this.actions = [ { id: 'remove', label: gettext('Remove') }, { id: 'edit', label: …	brennie	Jan. 21, 2016, 9:27 p.m.
Two lines between top-level declarations/definitions.	brennie	Jan. 21, 2016, 9:29 p.m.
Needs a doc comment.	brennie	Jan. 21, 2016, 9:31 p.m.
Needs a doc comment.	brennie	Jan. 21, 2016, 9:31 p.m.
Needs a doc comment.	brennie	Jan. 21, 2016, 9:31 p.m.
Two blank lines.	brennie	Jan. 21, 2016, 9:31 p.m.
Needs a doc comment.	brennie	Jan. 21, 2016, 9:33 p.m.
If you're not actually doing any substitution, it will be faster to just make this a string instead of using …	brennie	Jan. 22, 2016, 9:31 p.m.
Should use dashes instead of underscores.	brennie	Jan. 21, 2016, 9:33 p.m.
Needs a doc comment.	brennie	Jan. 21, 2016, 9:33 p.m.
Needs a doc comment.	brennie	Jan. 21, 2016, 9:33 p.m.
Needs a doc comment.	brennie	Jan. 21, 2016, 9:32 p.m.
This is returning a dict.	chipx86	Feb. 12, 2016, 10:51 p.m.
This can be one statement without impacting readability.	chipx86	Feb. 12, 2016, 10:52 p.m.
This is returning a dict.	chipx86	Feb. 12, 2016, 10:52 p.m.
No blank line here.	chipx86	Feb. 12, 2016, 10:53 p.m.
Should be in alphabetical order.	chipx86	Feb. 12, 2016, 10:54 p.m.
This should document the model attributes. See https://www.reviewboard.org/docs/codebase/dev/docs/writing-codebase-docs/#documenting-models	chipx86	Feb. 13, 2016, 5:04 p.m.
No trailing comma.	chipx86	Feb. 13, 2016, 5:05 p.m.
Should use .apply(this, arguments).	chipx86	Feb. 13, 2016, 6:05 p.m.
This is a no-op. We already have it set, since that's how we're setting this again. Let's remove this.	chipx86	Feb. 13, 2016, 5:06 p.m.
No traiing comma.	chipx86	Feb. 13, 2016, 5:06 p.m.
Best to put a big /* XXX This is for testing */ or something here and below, so it doesn't …	chipx86	Feb. 13, 2016, 5:06 p.m.
No trailing comma.	chipx86	Feb. 13, 2016, 5:06 p.m.
The href should have a value. At the very least, "#".	chipx86	Feb. 13, 2016, 6:02 p.m.
Text needs to be localized.	chipx86	Feb. 13, 2016, 5:38 p.m.
Should document the options. See https://www.reviewboard.org/docs/codebase/dev/docs/writing-codebase-docs/#documenting-function-options	chipx86	Feb. 13, 2016, 6:04 p.m.
Needs a "Returns:" section.	chipx86	Feb. 13, 2016, 6:04 p.m.
"Return a list of a user's ..."	chipx86	March 3, 2016, 9:57 a.m.
Descriptions are saying list, but it's not a list.	chipx86	March 3, 2016, 9:57 a.m.
This is wrapping too early.	chipx86	March 3, 2016, 9:58 a.m.
Missing a period.	chipx86	March 3, 2016, 9:58 a.m.
Missing a period.	chipx86	March 3, 2016, 9:58 a.m.
Missing a period.	chipx86	March 3, 2016, 9:58 a.m.
Space before {. Same below.	chipx86	March 3, 2016, 10:57 a.m.
Indentation levels in .less files should be 2 spaces.	chipx86	March 3, 2016, 10:01 a.m.
Ideally, these should be in alphabetical order.	chipx86	March 3, 2016, 10:07 a.m.
"an OAuth2"	chipx86	March 3, 2016, 10:07 a.m.
"string", lowercase.	chipx86	March 3, 2016, 10:07 a.m.
Missing a period.	chipx86	March 3, 2016, 10:08 a.m.
.extend isn't the right thing here.	chipx86	March 3, 2016, 10:09 a.m.
"Initialize."	chipx86	March 3, 2016, 10:10 a.m.
"object", lowercase.	chipx86	March 3, 2016, 10:11 a.m.
Missing a period.	chipx86	March 3, 2016, 10:11 a.m.
No blank line here.	chipx86	March 3, 2016, 10:11 a.m.
"Create". Function/method comments should be in the form of "Create", "Return", "Set", etc., rather than "Creates", "Returns", "Sets", etc.	chipx86	March 3, 2016, 10:12 a.m.
"object"	chipx86	March 3, 2016, 10:12 a.m.
Missing period. Check further docs for this as well.	chipx86	March 3, 2016, 10:14 a.m.
Including templates in this way can be very dangerous, and isn't something we want to do. Looking through those templates, …	chipx86	March 3, 2016, 10:57 a.m.
Should be removed.	chipx86	March 3, 2016, 10:14 a.m.
This should be wrapped in gettext().	chipx86	March 3, 2016, 10:15 a.m.
This should be wrapped in gettext().	chipx86	March 3, 2016, 10:15 a.m.
Due to how the gettext localization string gathering works, you can't actually split lines and concatenate them. So they'll need …	chipx86	March 3, 2016, 10:15 a.m.
render functions must return this, and should document that in the function docs. Same with others below.	chipx86	March 3, 2016, 10:18 a.m.
No blank line.	chipx86	March 3, 2016, 10:18 a.m.
Space before {.	chipx86	March 3, 2016, 10:18 a.m.
Should be wrapped with gettext().	chipx86	March 3, 2016, 10:18 a.m.
Two blank lines between these.	chipx86	March 3, 2016, 10:18 a.m.
This needs a doc header. Same with the functions.	chipx86	March 3, 2016, 11:14 a.m.
If this isn't needed in this change, it should be removed and implemented only in the change where it'll be …	chipx86	March 3, 2016, 11:14 a.m.
If these aren't needed in this change, they should be removed.	chipx86	March 3, 2016, 10:35 a.m.
IDs should be in the same form as class names, rather than CamelCase. Also, we're assigning too much meaning to …	chipx86	March 3, 2016, 10:35 a.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/oauth2ClientAppsView.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/oauth2ClientAppsView.js

Since this is a WIP, please put [WIP] in the summary.

LE

lehoangm Jan. 21, 2016, 9:54 p.m.
```
what does WIP really mean ?
```
brennie Jan. 21, 2016, 10:23 p.m.
```
WIP means "Work in Progress"
```

lehoangm Jan. 22, 2016, 6:24 a.m.

When we use WIP and when we dont use it ?

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

Show all issues

How about OAuth2AppForm?

lehoangm Jan. 21, 2016, 9:53 p.m.

Let's consider OAuth2AppListingForm since OAuth2AppForm is not clear what the form does. Please let me know what you think :)

brennie Jan. 21, 2016, 10:23 p.m.

Hmm, how about OAuth2AppForm or OAuth2AppListForm? I think that reads more easily.

reviewboard/accounts/forms/pages.py (Diff revision 1)
Show all issues
```
"This form"
```
reviewboard/accounts/forms/pages.py (Diff revision 1)
Show all issues
```
"view"
```

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

Show all issues

Mention that the JS will be responsible for doing this over the API?

lehoangm Jan. 21, 2016, 9:53 p.m.

I am not sure what you mean. I believe this requires discussion with Christian

LE

lehoangm Jan. 26, 2016, 10:55 p.m.
```
I think we are not using API for now
```

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

Show all issues

"Get" -> "Return"
Missing a Returns block, e.g.
```python

"""...
Returns:

    dict: An explanation of the return value.

"""

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

Show all issues

We can express this in a more Pythonic fashion by using a list comprehension:

serialized_oauth2_client_apps = [
     self._serialize_oauth2_client_app(client_app)
     for client_app in oauth2_client_apps
]

reviewboard/accounts/forms/pages.py (Diff revision 1)
Show all issues
```
How about serialized_apps ?
```
reviewboard/accounts/forms/pages.py (Diff revision 1)
Show all issues
```
_serialize_app?
Needs a docstring.
```

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

Show all issues

How about OAuth2Page ?

lehoangm Jan. 21, 2016, 9:53 p.m.

Let's consider OAuth2AppListingPage, similar to the one above

brennie Jan. 21, 2016, 10:23 p.m.

Likewise, I think this should be OAuth2AppPage or OAuth2AppListPage. It reads nicer.

reviewboard/static/rb/js/accountPrefsPage/views/oauth2ClientAppsView.js (Diff revision 1)
Show all issues
```
Needs a doc-comment.
```
reviewboard/static/rb/js/accountPrefsPage/views/oauth2ClientAppsView.js (Diff revision 1)
Show all issues
```
Needs a doc comment.
```
reviewboard/static/rb/js/accountPrefsPage/views/oauth2ClientAppsView.js (Diff revision 1)
Show all issues
```
Needs a doc comment.
```
reviewboard/static/rb/js/accountPrefsPage/views/oauth2ClientAppsView.js (Diff revision 1)
Show all issues
```
Needs a doc comment.
```
reviewboard/static/rb/js/accountPrefsPage/views/oauth2ClientAppsView.js (Diff revision 1)
Show all issues
```
Needs a doc comment.
```

reviewboard/static/rb/js/accountPrefsPage/views/oauth2ClientAppsView.js (Diff revision 1)

Show all issues

This would be better formatted as:
```javascript

this.actions = [

    {

        id: 'remove',

        label: gettext('Remove')

    },

    {

        id: 'edit',

        label: gettext('Edit')

    }

];

reviewboard/static/rb/js/accountPrefsPage/views/oauth2ClientAppsView.js (Diff revision 1)
Show all issues
```
Two lines between top-level declarations/definitions.
```
reviewboard/static/rb/js/accountPrefsPage/views/oauth2ClientAppsView.js (Diff revision 1)
Show all issues
```
Needs a doc comment.
```
reviewboard/static/rb/js/accountPrefsPage/views/oauth2ClientAppsView.js (Diff revision 1)
Show all issues
```
Needs a doc comment.
```
reviewboard/static/rb/js/accountPrefsPage/views/oauth2ClientAppsView.js (Diff revision 1)
Show all issues
```
Needs a doc comment.
```
reviewboard/static/rb/js/accountPrefsPage/views/oauth2ClientAppsView.js (Diff revision 1)
Show all issues
```
Two blank lines.
```
reviewboard/static/rb/js/accountPrefsPage/views/oauth2ClientAppsView.js (Diff revision 1)
Show all issues
```
Needs a doc comment.
```

reviewboard/static/rb/js/accountPrefsPage/views/oauth2ClientAppsView.js (Diff revision 1)

Show all issues

If you're not actually doing any substitution, it will be faster to just make this a string instead of using _.template.

lehoangm Jan. 21, 2016, 9:53 p.m.

What should i do here ? I dont know how to use string in this context

brennie Jan. 21, 2016, 10:23 p.m.

Instead of:
template: _.template([
    '...',
    '...',
    '...'
].join(''))

do:
content: [
    '...',
    '...',
    '...'
].join('')),


and then instead of using this.template() later, just use this.content.

reviewboard/static/rb/js/accountPrefsPage/views/oauth2ClientAppsView.js (Diff revision 1)
Show all issues
```
Should use dashes instead of underscores.
```
reviewboard/static/rb/js/accountPrefsPage/views/oauth2ClientAppsView.js (Diff revision 1)
Show all issues
```
Needs a doc comment.
```
reviewboard/static/rb/js/accountPrefsPage/views/oauth2ClientAppsView.js (Diff revision 1)
Show all issues
```
Needs a doc comment.
```
reviewboard/static/rb/js/accountPrefsPage/views/oauth2ClientAppsView.js (Diff revision 1)
Show all issues
```
Needs a doc comment.
```

Change Summary:

Update review, add css, reply comments

Diff:

Revision 2 (+268 -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/oauth2ClientAppsView.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/oauth2ClientAppsView.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/oauth2ClientAppsView.js
    reviewboard/static/rb/css/pages/my-account.less

Change Summary:

Add WIP

Summary:

-	Add OAuth2 client application listing page into account preference page
+	[WIP] Add OAuth2 client application listing page into account preference page

Change Summary:

Update review

Diff:

Revision 3 (+268 -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/oauth2ClientAppsView.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/oauth2ClientAppsView.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/oauth2ClientAppsView.js
    reviewboard/static/rb/css/pages/my-account.less

Change Summary:

remove WIP

Summary:

-	[WIP] Add OAuth2 client application listing page into account preference page
+	Add OAuth2 client application listing page into account preference page

Change Summary:

Add reviewer + Add prefix for RC summary

Summary:

-	Add OAuth2 client application listing page into account preference page
+	[OAuth2Provider] Add OAuth2 client application listing page into account preference page

People:

+	chipx86
+	brennie

reviewboard/accounts/forms/pages.py (Diff revision 3)
Show all issues
```
This is returning a dict.
```

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

Show all issues

This can be one statement without impacting readability.

reviewboard/accounts/forms/pages.py (Diff revision 3)
Show all issues
```
This is returning a dict.
```
reviewboard/accounts/forms/pages.py (Diff revision 3)
Show all issues
```
No blank line here.
```
reviewboard/accounts/pages.py (Diff revision 3)
Show all issues
```
Should be in alphabetical order.
```

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

Show all issues

This should document the model attributes. See https://www.reviewboard.org/docs/codebase/dev/docs/writing-codebase-docs/#documenting-models

reviewboard/static/rb/js/accountPrefsPage/views/oauth2ClientAppsView.js (Diff revision 3)
Show all issues
```
No trailing comma.
```
reviewboard/static/rb/js/accountPrefsPage/views/oauth2ClientAppsView.js (Diff revision 3)
Show all issues
```
Should use .apply(this, arguments).
```
1. LE
  
  lehoangm Feb. 13, 2016, 7:53 p.m.
  Removing this to implement the webapi

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

Show all issues

This is a no-op. We already have it set, since that's how we're setting this again. Let's remove this.

reviewboard/static/rb/js/accountPrefsPage/views/oauth2ClientAppsView.js (Diff revision 3)
Show all issues
```
No traiing comma.
```

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

Show all issues

Best to put a big /* XXX This is for testing */ or something here and below, so it doesn't slip through the cracks.

If these functions aren't used yet, it's probably best to not include them in the change.

Also, models should never know or care about UI, so they shouldn't be raising alerts or editing anything.

reviewboard/static/rb/js/accountPrefsPage/views/oauth2ClientAppsView.js (Diff revision 3)
Show all issues
```
No trailing comma.
```
reviewboard/static/rb/js/accountPrefsPage/views/oauth2ClientAppsView.js (Diff revision 3)
Show all issues
```
The href should have a value. At the very least, "#".
```
reviewboard/static/rb/js/accountPrefsPage/views/oauth2ClientAppsView.js (Diff revision 3)
Show all issues
```
Text needs to be localized.
```

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

Show all issues

Should document the options. See https://www.reviewboard.org/docs/codebase/dev/docs/writing-codebase-docs/#documenting-function-options

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

Show all issues

Needs a "Returns:" section.

Change Summary:

Add a different listing page with remove confirmation, and client app form

Summary:

-	[OAuth2Provider] Add OAuth2 client application listing page into account preference page
+	[OAuth2Provider] [WIP] Add OAuth2 client app listing page and form into account preference page

Description:

		Add a starting point for user to view all of their OAuth2 client apps.
		Similar to the group page, user can search for certain OAuth2 client
~		applications. Apart from the filtering, there is no interaction on the page yet
	~	rapplications. The page will ask for user confirmation if user want to
	+	remove a client app. The page also display a nonfunctional form for user to edit
	+	and add new application. There is no backend support for all operations

Depends On:

+	~~7897 - [OAuth2Provider] Add dependencies and urls to provide OAuth2 support for external services~~

Diff:

Revision 4 (+530 -1)

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/oauth2ClientAppsView.js
	reviewboard/static/rb/js/resources/models/oauth2ClientAppModel.js
	reviewboard/templates/accounts/oauth2_client_application.html

Added Files:

Tool: PEP8 Style Checker
Processed Files:
    reviewboard/accounts/pages.py
    reviewboard/accounts/forms/pages.py
    reviewboard/staticbundles.py

Ignored Files:
    reviewboard/templates/accounts/oauth2_client_application.html
    reviewboard/static/rb/js/resources/models/oauth2ClientAppModel.js
    reviewboard/static/rb/js/accountPrefsPage/views/oauth2ClientAppsView.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/templates/accounts/oauth2_client_application.html
    reviewboard/static/rb/js/resources/models/oauth2ClientAppModel.js
    reviewboard/static/rb/js/accountPrefsPage/views/oauth2ClientAppsView.js
    reviewboard/static/rb/css/pages/my-account.less

Change Summary:

Simplify the CSS

Diff:

Revision 5 (+533 -1)

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/oauth2ClientAppsView.js
	reviewboard/static/rb/js/resources/models/oauth2ClientAppModel.js
	reviewboard/templates/accounts/oauth2_client_application.html

Tool: Pyflakes
Processed Files:
    reviewboard/accounts/pages.py
    reviewboard/accounts/forms/pages.py
    reviewboard/staticbundles.py

Ignored Files:
    reviewboard/templates/accounts/oauth2_client_application.html
    reviewboard/static/rb/js/resources/models/oauth2ClientAppModel.js
    reviewboard/static/rb/js/accountPrefsPage/views/oauth2ClientAppsView.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/templates/accounts/oauth2_client_application.html
    reviewboard/static/rb/js/resources/models/oauth2ClientAppModel.js
    reviewboard/static/rb/js/accountPrefsPage/views/oauth2ClientAppsView.js
    reviewboard/static/rb/css/pages/my-account.less

Change Summary:

Fix search bar issue

Diff:

Revision 6 (+533 -1)

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/oauth2ClientAppsView.js
	reviewboard/static/rb/js/resources/models/oauth2ClientAppModel.js
	reviewboard/templates/accounts/oauth2_client_application.html

Tool: Pyflakes
Processed Files:
    reviewboard/accounts/pages.py
    reviewboard/accounts/forms/pages.py
    reviewboard/staticbundles.py

Ignored Files:
    reviewboard/templates/accounts/oauth2_client_application.html
    reviewboard/static/rb/js/resources/models/oauth2ClientAppModel.js
    reviewboard/static/rb/js/accountPrefsPage/views/oauth2ClientAppsView.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/templates/accounts/oauth2_client_application.html
    reviewboard/static/rb/js/resources/models/oauth2ClientAppModel.js
    reviewboard/static/rb/js/accountPrefsPage/views/oauth2ClientAppsView.js
    reviewboard/static/rb/css/pages/my-account.less

Change Summary:

Update from master

Diff:

Revision 7 (+485 -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/oauth2ClientAppsView.js
	reviewboard/static/rb/js/resources/models/oauth2ClientAppModel.js
	reviewboard/templates/accounts/oauth2_client_application.html

Tool: Pyflakes
Processed Files:
    reviewboard/accounts/pages.py
    reviewboard/accounts/forms/pages.py
    reviewboard/staticbundles.py

Ignored Files:
    reviewboard/templates/accounts/oauth2_client_application.html
    reviewboard/static/rb/js/resources/models/oauth2ClientAppModel.js
    reviewboard/static/rb/js/accountPrefsPage/views/oauth2ClientAppsView.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/templates/accounts/oauth2_client_application.html
    reviewboard/static/rb/js/resources/models/oauth2ClientAppModel.js
    reviewboard/static/rb/js/accountPrefsPage/views/oauth2ClientAppsView.js
    reviewboard/static/rb/css/pages/my-account.less

reviewboard/accounts/forms/pages.py (Diff revision 7)
Show all issues
```
"Return a list of a user's ..."
```

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

Show all issues

Descriptions are saying list, but it's not a list.

reviewboard/accounts/forms/pages.py (Diff revision 7)
Show all issues
```
This is wrapping too early.
```
reviewboard/accounts/forms/pages.py (Diff revision 7)
Show all issues
```
Missing a period.
```
reviewboard/accounts/forms/pages.py (Diff revision 7)
Show all issues
```
Missing a period.
```
reviewboard/accounts/forms/pages.py (Diff revision 7)
Show all issues
```
Missing a period.
```
reviewboard/static/rb/css/pages/my-account.less (Diff revision 7)
Show all issues
```
Space before {.
Same below.
```
reviewboard/static/rb/css/pages/my-account.less (Diff revision 7)
Show all issues
```
Indentation levels in .less files should be 2 spaces.
```

reviewboard/static/rb/js/accountPrefsPage/views/oauth2ClientAppsView.js (Diff revision 7)

Show all issues

Ideally, these should be in alphabetical order.

reviewboard/static/rb/js/accountPrefsPage/views/oauth2ClientAppsView.js (Diff revision 7)
Show all issues
```
"an OAuth2"
```
reviewboard/static/rb/js/accountPrefsPage/views/oauth2ClientAppsView.js (Diff revision 7)
Show all issues
```
"string", lowercase.
```
reviewboard/static/rb/js/accountPrefsPage/views/oauth2ClientAppsView.js (Diff revision 7)
Show all issues
```
Missing a period.
```
reviewboard/static/rb/js/accountPrefsPage/views/oauth2ClientAppsView.js (Diff revision 7)
Show all issues
```
.extend isn't the right thing here.
```
1. LE
  
  lehoangm March 8, 2016, 6:22 a.m.
  I changed to prototype.defaults
reviewboard/static/rb/js/accountPrefsPage/views/oauth2ClientAppsView.js (Diff revision 7)
Show all issues
```
"Initialize."
```
reviewboard/static/rb/js/accountPrefsPage/views/oauth2ClientAppsView.js (Diff revision 7)
Show all issues
```
"object", lowercase.
```
reviewboard/static/rb/js/accountPrefsPage/views/oauth2ClientAppsView.js (Diff revision 7)
Show all issues
```
Missing a period.
```
reviewboard/static/rb/js/accountPrefsPage/views/oauth2ClientAppsView.js (Diff revision 7)
Show all issues
```
No blank line here.
```

reviewboard/static/rb/js/accountPrefsPage/views/oauth2ClientAppsView.js (Diff revision 7)

Show all issues

"Create".

Function/method comments should be in the form of "Create", "Return", "Set", etc., rather than "Creates", "Returns", "Sets", etc.

reviewboard/static/rb/js/accountPrefsPage/views/oauth2ClientAppsView.js (Diff revision 7)
Show all issues
```
"object"
```
reviewboard/static/rb/js/accountPrefsPage/views/oauth2ClientAppsView.js (Diff revision 7)
Show all issues
```
Missing period.
Check further docs for this as well.
```

reviewboard/static/rb/js/accountPrefsPage/views/oauth2ClientAppsView.js (Diff revision 7)

Show all issues

Including templates in this way can be very dangerous, and isn't something we want to do.
Looking through those templates, we'd be much better off doing what other similar views do. Each of these views should define the HTML needed inline, rather than assuming the page has outputted it to some hidden element.
By defining inline, we only have it when we need it, and we have an easier way to unit test it (since the unit tests don't need to duplicate the template here).
Also, important note: Using $('...').text() will filter out tags, and return content that may contain things like the word <script>. That would then be turned into HTML, resulting in a security vulnerability. It's actually very, very dangerous to do this kind of thing.
Same below.

reviewboard/static/rb/js/accountPrefsPage/views/oauth2ClientAppsView.js (Diff revision 7)
Show all issues
```
Should be removed.
```
reviewboard/static/rb/js/accountPrefsPage/views/oauth2ClientAppsView.js (Diff revision 7)
Show all issues
```
This should be wrapped in gettext().
```
reviewboard/static/rb/js/accountPrefsPage/views/oauth2ClientAppsView.js (Diff revision 7)
Show all issues
```
This should be wrapped in gettext().
```

reviewboard/static/rb/js/accountPrefsPage/views/oauth2ClientAppsView.js (Diff revision 7)

Show all issues

Due to how the gettext localization string gathering works, you can't actually split lines and concatenate them. So they'll need to be one long string, each. This only needs to happen in JavaScript code.

This violates the standard rule of fitting content into 80 columns, but for gettext strings, we make an exception.

reviewboard/static/rb/js/accountPrefsPage/views/oauth2ClientAppsView.js (Diff revision 7)

Show all issues

render functions must return this, and should document that in the function docs.
Same with others below.

reviewboard/static/rb/js/accountPrefsPage/views/oauth2ClientAppsView.js (Diff revision 7)
Show all issues
```
No blank line.
```
reviewboard/static/rb/js/accountPrefsPage/views/oauth2ClientAppsView.js (Diff revision 7)
Show all issues
```
Space before {.
```
reviewboard/static/rb/js/accountPrefsPage/views/oauth2ClientAppsView.js (Diff revision 7)
Show all issues
```
Should be wrapped with gettext().
```
reviewboard/static/rb/js/accountPrefsPage/views/oauth2ClientAppsView.js (Diff revision 7)
Show all issues
```
Two blank lines between these.
```
reviewboard/static/rb/js/resources/models/oauth2ClientAppModel.js (Diff revision 7)
Show all issues
```
This needs a doc header.
Same with the functions.
```

reviewboard/static/rb/js/resources/models/oauth2ClientAppModel.js (Diff revision 7)

Show all issues

If this isn't needed in this change, it should be removed and implemented only in the change where it'll be used.

Best to keep individual changes to only the code needed in that change.

reviewboard/static/rb/js/resources/models/oauth2ClientAppModel.js (Diff revision 7)

Show all issues

If these aren't needed in this change, they should be removed.

reviewboard/templates/accounts/oauth2_client_application.html (Diff revision 7)

Show all issues

IDs should be in the same form as class names, rather than CamelCase.
Also, we're assigning too much meaning to backbone-template. It's a vague class name with a very specific purpose, but different ways it could be interpreted. However, with my suggestions above, you won't need this anymore.

Change Summary:

Update cr: remove html , adjust minor comments

Diff:

Revision 8 (+478 -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/oauth2ClientAppsView.js
	reviewboard/static/rb/js/resources/models/oauth2ClientAppModel.js

Tool: Pyflakes
Processed Files:
    reviewboard/accounts/pages.py
    reviewboard/accounts/forms/pages.py
    reviewboard/staticbundles.py

Ignored Files:
    reviewboard/static/rb/js/resources/models/oauth2ClientAppModel.js
    reviewboard/static/rb/js/accountPrefsPage/views/oauth2ClientAppsView.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/resources/models/oauth2ClientAppModel.js
    reviewboard/static/rb/js/accountPrefsPage/views/oauth2ClientAppsView.js
    reviewboard/static/rb/css/pages/my-account.less

Change Summary:

Update cr and use oauth2_oauth2clientapplication instead of oauth2_provider_application. Add period and clien_id

Diff:

Revision 9 (+483 -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/oauth2ClientAppsView.js
	reviewboard/static/rb/js/resources/models/oauth2ClientAppModel.js

Tool: Pyflakes
Processed Files:
    reviewboard/accounts/pages.py
    reviewboard/accounts/forms/pages.py
    reviewboard/staticbundles.py

Ignored Files:
    reviewboard/static/rb/js/resources/models/oauth2ClientAppModel.js
    reviewboard/static/rb/js/accountPrefsPage/views/oauth2ClientAppsView.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/resources/models/oauth2ClientAppModel.js
    reviewboard/static/rb/js/accountPrefsPage/views/oauth2ClientAppsView.js
    reviewboard/static/rb/css/pages/my-account.less

Change Summary:

Remove WIP status

Summary:

-	[OAuth2Provider] [WIP] Add OAuth2 client app listing page and form into account preference page
+	[OAuth2Provider] Add OAuth2 client app listing page and form into account preference page

Review Board 6.0.3 alpha 0 (dev)

[OAuth2Provider] Add OAuth2 client app listing page and form into account preference page

Information

Reviewers

Change Summary:

Diff:

Change Summary:

Summary:

Change Summary:

Diff:

Change Summary:

Summary:

Change Summary:

Summary:

People:

Change Summary:

Summary:

Description:

Depends On:

Diff:

Added Files:

Change Summary:

Diff:

Change Summary:

Diff:

Change Summary:

Diff:

Change Summary:

Diff:

Change Summary:

Diff:

Change Summary:

Summary:

Status: Discarded