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

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

Minh Le Hoang
Review Board
master
7900
7964, 7993, 7997, 7996
reviewboard
brennie, chipx86
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

Loading file attachments...

  • 0
  • 0
  • 42
  • 5
  • 47
Description From Last Updated
Review Bot
  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. 
      
Minh Le Hoang
Christian Hammond
  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. 
      
Minh Le Hoang
Review Bot
  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. 
      
Minh Le Hoang
Review Bot
  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. 
      
Christian Hammond
  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. 
      
Minh Le Hoang
Review Bot
  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. 
      
Barret Rennie
  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. 
      
Barret Rennie
Review request changed

Status: Discarded

Loading...