[OAuth2Provider] Append OAuth2 token management page to API Token management for account preference
Review Request #7941 — Created Feb. 3, 2016 and discarded
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
Manualy tested on browser
| 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
|
|
|
Most of this can be combined into a single statement without losing readability. Given my suggestion below, I'd say keep … |
chipx86
|
|
|
Fetching the application relation for each token will result in a lot of queries. We want to fetch them in … |
chipx86
|
|
|
Can you move these (and GroupsForm) so that they're in alphabetical order? By the way, I keep running across code … |
chipx86
|
|
|
Space before { Same below. |
chipx86
|
|
|
These rules are all the same. You can combine them. (Is the first one meant to do that, or meant … |
chipx86
|
|
|
2 space indentation. |
chipx86
|
|
|
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
|
|
|
Trailing commas in JavaScript will break some browsers. |
chipx86
|
|
|
We don't want to hard-code URLs in JavaScript, so the Python side must be responsible for passing in the URLs … |
chipx86
|
|
|
This should go first, and should do .apply(this, arguments) instead of .call(this). |
chipx86
|
|
|
No trailing comma. |
chipx86
|
|
|
No trailing comma. |
chipx86
|
|
|
No trailing comma. |
chipx86
|
|
|
Missing a period. |
chipx86
|
|
|
No trailing comma. |
chipx86
|
|
|
No trailing comma. |
chipx86
|
|
|
We shouldn't be inserting buttons here. Instead, we should register actions. That way, the UI is consistent, and we don't … |
chipx86
|
|
|
No trailing comma. |
chipx86
|
|
|
The second line here should be removed. |
chipx86
|
|
|
Blank line between these. |
chipx86
|
|
|
Space after if. We can't check the text of the button, because we need to localize text. We need to … |
chipx86
|
|
|
Each user-visible string needs to be localized by doing: gettext('Show scopes') |
chipx86
|
|
|
We should only query these once. |
chipx86
|
|
|
This should be called template, and should use _.template. |
chipx86
|
|
|
No trailing comma. |
chipx86
|
|
|
Needs a "Returns:" section. |
chipx86
|
|
|
Blank line should go after the load, not before. |
chipx86
|
|
|
1 space indentation for HTML. |
chipx86
|
|
|
In some places, we say "OAuth2." In others, "OAuth 2." We'll need to be sure we're consistent everywhere. (I prefer … |
chipx86
|
|
|
Similar comments to my other change, in terms of wording, and that this doesn't return a list. Also, missing a … |
chipx86
|
|
|
I mention this in a previous review, but we use 2 space indentation in .less files. |
chipx86
|
|
|
"string", lowercase. |
chipx86
|
|
|
Blank line between these. Missing periods. |
chipx86
|
|
|
"Initialize." Same comment as this chagne's parent's review about "Returns" vs "Return", "Initializes" vs "Initialize", etc. Only for functions/methods, though. … |
chipx86
|
|
|
"object", lowercase. Same below. |
chipx86
|
|
|
"Scopes" needs to be localized. |
chipx86
|
|
|
Same comments as my previous review. Strings wrapped in gettext() cannot wrap. |
chipx86
|
|
|
render functions must return this, for chaining. This should also be documented. |
chipx86
|
|
|
Two blank lines between these. |
chipx86
|
|
|
Needs docs. So do the functions. |
chipx86
|
|
|
I don't think OAuth2 tokens have a policy concept, do they? Maybe I'll find this out in one of the … |
chipx86
|
|
|
Missing trailing comma. |
 brennie
|
|
|
object, not dict. |
brennie
|
|
|
This should be isExpired. JS properties all use camelCase. |
brennie
|
|
|
/** |
brennie
|
|
|
/** * Return API URL to access the resource. * * Extended description here. * * Returns: * string: ... … |
brennie
|
-
-
"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 -
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.
-
Fetching the
applicationrelation 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') -
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? -
-
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?)
-
-
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.)
-
-
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.
-
-
-
-
-
-
-
-
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.
-
-
-
-
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.
-
-
-
-
-
-
-
LE
- Change Summary:
-
Update CR, get ready for webAPI integration
- Depends On:
-
- Diff:
Revision 2 (+302 -3)
- 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
-
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
-
-
In some places, we say "OAuth2." In others, "OAuth 2." We'll need to be sure we're consistent everywhere. (I prefer "OAuth2".)
-
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.
-
-
-
-
"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.
-
-
-
-
-
-
-
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.
-
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
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