Summary

[OAuth2Provider] Append OAuth2 token management page to API Token management for account preference

Review Request #7941 — Created Feb. 3, 2016 and discarded Sept. 7, 2017, 9 p.m.

Information

Owner

lehoangm

Repository

Review Board

Branch

master

Bugs

Depends On

~~7900~~

Blocks

7964, 7993, 7997, 7996

Reviewers

Groups

reviewboard

People

brennie, chipx86

Description

User need a page to manage list of all OAuth2 tokens that have been authorized.
The OAuth2 managment is added as an extension to the current API token management page
However, it stay as a separete dialog box to be clear

Testing Done

Manualy tested on browser

Files

Issues

Description	From	Last Updated
"Return", not "Get". This also needs to be in the proper doc format, providing a Returns: section. See https://www.reviewboard.org/docs/codebase/dev/docs/writing-codebase-docs/#standard-documentation-format	chipx86	Feb. 13, 2016, 9:46 p.m.
Most of this can be combined into a single statement without losing readability. Given my suggestion below, I'd say keep …	chipx86	Feb. 13, 2016, 9:47 p.m.
Fetching the application relation for each token will result in a lot of queries. We want to fetch them in …	chipx86	Feb. 13, 2016, 9:48 p.m.
Can you move these (and GroupsForm) so that they're in alphabetical order? By the way, I keep running across code …	chipx86	Feb. 13, 2016, 8:20 p.m.
Space before { Same below.	chipx86	Feb. 13, 2016, 9:48 p.m.
These rules are all the same. You can combine them. (Is the first one meant to do that, or meant …	chipx86	Feb. 13, 2016, 9:48 p.m.
2 space indentation.	chipx86	Feb. 13, 2016, 9:48 p.m.
You'll need a "Model Attributes:" section. See https://www.reviewboard.org/docs/codebase/dev/docs/writing-codebase-docs/#documenting-models (Note that other models don't have this yet. We're working on standardizing …	chipx86	Feb. 13, 2016, 11:28 p.m.
Trailing commas in JavaScript will break some browsers.	chipx86	Feb. 13, 2016, 9:49 p.m.
We don't want to hard-code URLs in JavaScript, so the Python side must be responsible for passing in the URLs …	chipx86	Feb. 13, 2016, 9:49 p.m.
This should go first, and should do .apply(this, arguments) instead of .call(this).	chipx86	Feb. 13, 2016, 9:49 p.m.
No trailing comma.	chipx86	Feb. 13, 2016, 9:49 p.m.
No trailing comma.	chipx86	Feb. 13, 2016, 9:49 p.m.
No trailing comma.	chipx86	Feb. 13, 2016, 9:49 p.m.
Missing a period.	chipx86	Feb. 13, 2016, 9:50 p.m.
No trailing comma.	chipx86	Feb. 13, 2016, 9:50 p.m.
No trailing comma.	chipx86	Feb. 13, 2016, 9:50 p.m.
We shouldn't be inserting buttons here. Instead, we should register actions. That way, the UI is consistent, and we don't …	chipx86	Feb. 13, 2016, 11:29 p.m.
No trailing comma.	chipx86	Feb. 13, 2016, 9:51 p.m.
The second line here should be removed.	chipx86	Feb. 13, 2016, 9:51 p.m.
Blank line between these.	chipx86	Feb. 13, 2016, 11:22 p.m.
Space after if. We can't check the text of the button, because we need to localize text. We need to …	chipx86	Feb. 13, 2016, 11:21 p.m.
Each user-visible string needs to be localized by doing: gettext('Show scopes')	chipx86	Feb. 13, 2016, 11:30 p.m.
We should only query these once.	chipx86	Feb. 13, 2016, 11:30 p.m.
This should be called template, and should use _.template.	chipx86	Feb. 13, 2016, 9:53 p.m.
No trailing comma.	chipx86	Feb. 13, 2016, 9:53 p.m.
Needs a "Returns:" section.	chipx86	Feb. 13, 2016, 11:29 p.m.
Blank line should go after the load, not before.	chipx86	Feb. 13, 2016, 9:54 p.m.
1 space indentation for HTML.	chipx86	Feb. 13, 2016, 9:54 p.m.
In some places, we say "OAuth2." In others, "OAuth 2." We'll need to be sure we're consistent everywhere. (I prefer …	chipx86	March 3, 2016, 11:20 a.m.
Similar comments to my other change, in terms of wording, and that this doesn't return a list. Also, missing a …	chipx86	March 4, 2016, 9:03 p.m.
I mention this in a previous review, but we use 2 space indentation in .less files.	chipx86	March 3, 2016, 11:21 a.m.
"string", lowercase.	chipx86	March 4, 2016, 9:05 p.m.
Blank line between these. Missing periods.	chipx86	March 4, 2016, 9:05 p.m.
"Initialize." Same comment as this chagne's parent's review about "Returns" vs "Return", "Initializes" vs "Initialize", etc. Only for functions/methods, though. …	chipx86	March 4, 2016, 9:06 p.m.
"object", lowercase. Same below.	chipx86	March 4, 2016, 9:06 p.m.
"Scopes" needs to be localized.	chipx86	March 4, 2016, 9:54 p.m.
Same comments as my previous review. Strings wrapped in gettext() cannot wrap.	chipx86	March 4, 2016, 9:54 p.m.
render functions must return this, for chaining. This should also be documented.	chipx86	March 5, 2016, 12:12 a.m.
Two blank lines between these.	chipx86	March 4, 2016, 9:56 p.m.
Needs docs. So do the functions.	chipx86	March 5, 2016, 12:11 a.m.
I don't think OAuth2 tokens have a policy concept, do they? Maybe I'll find this out in one of the …	chipx86	March 4, 2016, 10:11 p.m.
Missing trailing comma.	brennie	March 15, 2016, 9:05 a.m.
object, not dict.	brennie	March 15, 2016, 8:54 a.m.
This should be isExpired. JS properties all use camelCase.	brennie	March 15, 2016, 8:54 a.m.
/**	brennie	March 15, 2016, 8:54 a.m.
/** * Return API URL to access the resource. * * Extended description here. * * Returns: * string: ... …	brennie	March 15, 2016, 9:04 a.m.

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

Ignored Files:
    reviewboard/templates/oauth2_provider/authorized-token-delete.html
    reviewboard/static/rb/js/accountPrefsPage/views/oauth2TokensView.js
    reviewboard/static/rb/css/pages/my-account.less



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

Ignored Files:
    reviewboard/templates/oauth2_provider/authorized-token-delete.html
    reviewboard/static/rb/js/accountPrefsPage/views/oauth2TokensView.js
    reviewboard/static/rb/css/pages/my-account.less

Change Summary:

Remove WIP prefix for RC summary

Summary:

-	[WIP][OAuth2Provider] Append OAuth2 token management page to API Token management for account preference
+	[OAuth2Provider] Append OAuth2 token management page to API Token management for account preference

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

Show all issues

"Return", not "Get".

This also needs to be in the proper doc format, providing a Returns: section. See https://www.reviewboard.org/docs/codebase/dev/docs/writing-codebase-docs/#standard-documentation-format

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

Show all issues

Most of this can be combined into a single statement without losing readability. Given my suggestion below, I'd say keep the query separate, but merge the serialized bits.

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

Show all issues

Fetching the application relation for each token will result in a lot of queries. We want to fetch them in the initial query above, so you'll want to do:

oauth2_tokens = self.user.accesstoken_set.select_related('application')

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

Show all issues

Can you move these (and GroupsForm) so that they're in alphabetical order?

By the way, I keep running across code that has come from another of your changes, but I'm having trouble tracking which review request it comes from. For instance, OAuth2AppListForm. Can you make sure that every review request has the Depends On field set appropriately?

reviewboard/static/rb/css/pages/my-account.less (Diff revision 1)
Show all issues
```
Space before {
Same below.
```

reviewboard/static/rb/css/pages/my-account.less (Diff revision 1)

Show all issues

These rules are all the same. You can combine them.

(Is the first one meant to do that, or meant to apply only to elements with both of those classes?)

reviewboard/static/rb/css/pages/my-account.less (Diff revision 1)
Show all issues
```
2 space indentation.
```

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

Show all issues

You'll need a "Model Attributes:" section. See https://www.reviewboard.org/docs/codebase/dev/docs/writing-codebase-docs/#documenting-models

(Note that other models don't have this yet. We're working on standardizing on this format.)

reviewboard/static/rb/js/accountPrefsPage/views/oauth2TokensView.js (Diff revision 1)
Show all issues
```
Trailing commas in JavaScript will break some browsers.
```

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

Show all issues

We don't want to hard-code URLs in JavaScript, so the Python side must be responsible for passing in the URLs that are needed.

reviewboard/static/rb/js/accountPrefsPage/views/oauth2TokensView.js (Diff revision 1)
Show all issues
```
This should go first, and should do .apply(this, arguments) instead of .call(this).
```
reviewboard/static/rb/js/accountPrefsPage/views/oauth2TokensView.js (Diff revision 1)
Show all issues
```
No trailing comma.
```
reviewboard/static/rb/js/accountPrefsPage/views/oauth2TokensView.js (Diff revision 1)
Show all issues
```
No trailing comma.
```
reviewboard/static/rb/js/accountPrefsPage/views/oauth2TokensView.js (Diff revision 1)
Show all issues
```
No trailing comma.
```
reviewboard/static/rb/js/accountPrefsPage/views/oauth2TokensView.js (Diff revision 1)
Show all issues
```
Missing a period.
```
reviewboard/static/rb/js/accountPrefsPage/views/oauth2TokensView.js (Diff revision 1)
Show all issues
```
No trailing comma.
```
reviewboard/static/rb/js/accountPrefsPage/views/oauth2TokensView.js (Diff revision 1)
Show all issues
```
No trailing comma.
```

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

Show all issues

We shouldn't be inserting buttons here. Instead, we should register actions. That way, the UI is consistent, and we don't have to inject custom HTML here.

LE

lehoangm Feb. 13, 2016, 11:53 p.m.
```
I remove the toogle button for now
```

reviewboard/static/rb/js/accountPrefsPage/views/oauth2TokensView.js (Diff revision 1)
Show all issues
```
No trailing comma.
```
reviewboard/static/rb/js/accountPrefsPage/views/oauth2TokensView.js (Diff revision 1)
Show all issues
```
The second line here should be removed.
```
reviewboard/static/rb/js/accountPrefsPage/views/oauth2TokensView.js (Diff revision 1)
Show all issues
```
Blank line between these.
```

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

Show all issues

Space after if.

We can't check the text of the button, because we need to localize text. We need to track the state instead.

reviewboard/static/rb/js/accountPrefsPage/views/oauth2TokensView.js (Diff revision 1)
Show all issues
```
Each user-visible string needs to be localized by doing: gettext('Show scopes')
```
1. LE
  
  lehoangm Feb. 13, 2016, 11:53 p.m.
  I removed the scope toogle button for now
reviewboard/static/rb/js/accountPrefsPage/views/oauth2TokensView.js (Diff revision 1)
Show all issues
```
We should only query these once.
```
1. LE
  
  lehoangm Feb. 13, 2016, 11:53 p.m.
  I removed the scope toogle button for now
reviewboard/static/rb/js/accountPrefsPage/views/oauth2TokensView.js (Diff revision 1)
Show all issues
```
This should be called template, and should use _.template.
```
reviewboard/static/rb/js/accountPrefsPage/views/oauth2TokensView.js (Diff revision 1)
Show all issues
```
No trailing comma.
```

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

Show all issues

Needs a "Returns:" section.

reviewboard/templates/oauth2_provider/authorized-token-delete.html (Diff revision 1)
Show all issues
```
Blank line should go after the load, not before.
```

reviewboard/templates/oauth2_provider/authorized-token-delete.html (Diff revision 1)

Show all issues

1 space indentation for HTML.

Change Summary:

Update CR, get ready for webAPI integration

Depends On:

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

Diff:

Revision 2 (+302 -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/oauth2TokensView.js
	reviewboard/static/rb/js/resources/models/oauth2TokenModel.js

Removed Files:

Added Files:

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

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

Change Summary:

Update rebasing

Diff:

Revision 3 (+300 -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/oauth2TokensView.js
	reviewboard/static/rb/js/resources/models/oauth2TokenModel.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/oauth2TokensView.js
    reviewboard/static/rb/js/resources/models/oauth2TokenModel.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/oauth2TokensView.js
    reviewboard/static/rb/js/resources/models/oauth2TokenModel.js
    reviewboard/static/rb/css/pages/my-account.less

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

Show all issues

In some places, we say "OAuth2." In others, "OAuth 2." We'll need to be sure we're consistent everywhere. (I prefer "OAuth2".)

lehoangm March 10, 2016, 7:36 a.m.

Ops, "OAuth 2" cases must be mistakes. I like "OAuth2" and use "OAuth2" everywhere else.

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

Show all issues

Similar comments to my other change, in terms of wording, and that this doesn't return a list.
Also, missing a period. Same with ones below.

reviewboard/static/rb/css/pages/my-account.less (Diff revision 3)

Show all issues

I mention this in a previous review, but we use 2 space indentation in .less files.

reviewboard/static/rb/js/accountPrefsPage/views/oauth2TokensView.js (Diff revision 3)
Show all issues
```
"string", lowercase.
```
reviewboard/static/rb/js/accountPrefsPage/views/oauth2TokensView.js (Diff revision 3)
Show all issues
```
Blank line between these.
Missing periods.
```

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

Show all issues

"Initialize."

Same comment as this chagne's parent's review about "Returns" vs "Return", "Initializes" vs "Initialize", etc.

Only for functions/methods, though. Not classes.

reviewboard/static/rb/js/accountPrefsPage/views/oauth2TokensView.js (Diff revision 3)
Show all issues
```
"object", lowercase.
Same below.
```
reviewboard/static/rb/js/accountPrefsPage/views/oauth2TokensView.js (Diff revision 3)
Show all issues
```
"Scopes" needs to be localized.
```

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

Show all issues

Same comments as my previous review. Strings wrapped in gettext() cannot wrap.

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

Show all issues

render functions must return this, for chaining. This should also be documented.

reviewboard/static/rb/js/accountPrefsPage/views/oauth2TokensView.js (Diff revision 3)
Show all issues
```
Two blank lines between these.
```
reviewboard/static/rb/js/resources/models/oauth2TokenModel.js (Diff revision 3)
Show all issues
```
Needs docs.
So do the functions.
```

reviewboard/static/rb/js/resources/models/oauth2TokenModel.js (Diff revision 3)

Show all issues

I don't think OAuth2 tokens have a policy concept, do they? Maybe I'll find this out in one of the other changes. The API tokens have policy, because that's a concept we created.

LE

lehoangm March 10, 2016, 7:36 a.m.
```
Copy and paste from API token. Removed
```

Change Summary:

Update cr; Add token expiry date with style

Diff:

Revision 4 (+323 -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/oauth2TokensView.js
	reviewboard/static/rb/js/resources/models/oauth2TokenModel.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/oauth2TokensView.js
    reviewboard/static/rb/js/resources/models/oauth2TokenModel.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/oauth2TokensView.js
    reviewboard/static/rb/js/resources/models/oauth2TokenModel.js
    reviewboard/static/rb/css/pages/my-account.less

reviewboard/accounts/forms/pages.py (Diff revision 4)
Show all issues
```
Missing trailing comma.
```
reviewboard/static/rb/js/accountPrefsPage/views/oauth2TokensView.js (Diff revision 4)
Show all issues
```
object, not dict.
```
reviewboard/static/rb/js/accountPrefsPage/views/oauth2TokensView.js (Diff revision 4)
Show all issues
```
This should be isExpired. JS properties all use camelCase.
```
reviewboard/static/rb/js/resources/models/oauth2TokenModel.js (Diff revision 4)
Show all issues
```
/**
```

reviewboard/static/rb/js/resources/models/oauth2TokenModel.js (Diff revision 4)

Show all issues

/**
 * Return API URL to access the resource.
 *
 * Extended description here.
 *
 * Returns:
 *     string: ...
 */

Same for other doc comments

Review Board 6.0.1 alpha 0 (dev)

[OAuth2Provider] Append OAuth2 token management page to API Token management for account preference

Information

Reviewers

Change Summary:

Summary:

Change Summary:

Depends On:

Diff:

Removed Files:

Added Files:

Change Summary:

Diff:

Change Summary:

Diff:

Status: Discarded