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

Review Request #7941 — Created Feb. 3, 2016 and discarded

Information

Review Board
master

Reviewers

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

chipx86chipx86

Most of this can be combined into a single statement without losing readability. Given my suggestion below, I'd say keep …

chipx86chipx86

Fetching the application relation for each token will result in a lot of queries. We want to fetch them in …

chipx86chipx86

Can you move these (and GroupsForm) so that they're in alphabetical order? By the way, I keep running across code …

chipx86chipx86

Space before { Same below.

chipx86chipx86

These rules are all the same. You can combine them. (Is the first one meant to do that, or meant …

chipx86chipx86

2 space indentation.

chipx86chipx86

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 …

chipx86chipx86

Trailing commas in JavaScript will break some browsers.

chipx86chipx86

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

chipx86chipx86

This should go first, and should do .apply(this, arguments) instead of .call(this).

chipx86chipx86

No trailing comma.

chipx86chipx86

No trailing comma.

chipx86chipx86

No trailing comma.

chipx86chipx86

Missing a period.

chipx86chipx86

No trailing comma.

chipx86chipx86

No trailing comma.

chipx86chipx86

We shouldn't be inserting buttons here. Instead, we should register actions. That way, the UI is consistent, and we don't …

chipx86chipx86

No trailing comma.

chipx86chipx86

The second line here should be removed.

chipx86chipx86

Blank line between these.

chipx86chipx86

Space after if. We can't check the text of the button, because we need to localize text. We need to …

chipx86chipx86

Each user-visible string needs to be localized by doing: gettext('Show scopes')

chipx86chipx86

We should only query these once.

chipx86chipx86

This should be called template, and should use _.template.

chipx86chipx86

No trailing comma.

chipx86chipx86

Needs a "Returns:" section.

chipx86chipx86

Blank line should go after the load, not before.

chipx86chipx86

1 space indentation for HTML.

chipx86chipx86

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

chipx86chipx86

Similar comments to my other change, in terms of wording, and that this doesn't return a list. Also, missing a …

chipx86chipx86

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

chipx86chipx86

"string", lowercase.

chipx86chipx86

Blank line between these. Missing periods.

chipx86chipx86

"Initialize." Same comment as this chagne's parent's review about "Returns" vs "Return", "Initializes" vs "Initialize", etc. Only for functions/methods, though. …

chipx86chipx86

"object", lowercase. Same below.

chipx86chipx86

"Scopes" needs to be localized.

chipx86chipx86

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

chipx86chipx86

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

chipx86chipx86

Two blank lines between these.

chipx86chipx86

Needs docs. So do the functions.

chipx86chipx86

I don't think OAuth2 tokens have a policy concept, do they? Maybe I'll find this out in one of the …

chipx86chipx86

Missing trailing comma.

brenniebrennie

object, not dict.

brenniebrennie

This should be isExpired. JS properties all use camelCase.

brenniebrennie

/**

brenniebrennie

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

brenniebrennie
reviewbot
  1. 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
    
    
  2. 
      
LE
chipx86
  1. 
      
  2. reviewboard/accounts/forms/pages.py (Diff revision 1)
     
     

    "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

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

    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.

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

    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')
    
  5. reviewboard/accounts/pages.py (Diff revision 1)
     
     
     
     
     

    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?

  6. Space before {

    Same below.

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

    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?)

  8. 2 space indentation.

  9. 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.)

  10. Trailing commas in JavaScript will break some browsers.

  11. 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.

  12. This should go first, and should do .apply(this, arguments) instead of .call(this).

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

    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.

    1. I remove the toogle button for now

  14. The second line here should be removed.

  15. Blank line between these.

  16. 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.

  17. Each user-visible string needs to be localized by doing: gettext('Show scopes')

    1. I removed the scope toogle button for now

  18. We should only query these once.

    1. I removed the scope toogle button for now

  19. This should be called template, and should use _.template.

  20. Needs a "Returns:" section.

  21. Blank line should go after the load, not before.

  22. 1 space indentation for HTML.

  23. 
      
LE
reviewbot
  1. 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
    
    
  2. 
      
LE
reviewbot
  1. 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
    
    
  2. 
      
chipx86
  1. 
      
  2. reviewboard/accounts/forms/pages.py (Diff revision 3)
     
     

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

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

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

    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.

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

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

  5. Blank line between these.

    Missing periods.

  6. "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.

  7. "object", lowercase.

    Same below.

  8. "Scopes" needs to be localized.

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

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

  11. Two blank lines between these.

  12. Needs docs.

    So do the functions.

  13. 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.

    1. Copy and paste from API token. Removed

  14. 
      
LE
reviewbot
  1. 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
    
    
  2. 
      
brennie
  1. 
      
  2. reviewboard/accounts/forms/pages.py (Diff revision 4)
     
     

    Missing trailing comma.

  3. This should be isExpired. JS properties all use camelCase.

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

    Same for other doc comments

  5. 
      
brennie
Review request changed

Status: Discarded

Loading...