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

Review Request #7900 — Created Jan. 20, 2016 and discarded

Information

Review Board
master

Reviewers

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

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.


Description From Last Updated

How about OAuth2AppForm?

brenniebrennie

"This form"

brenniebrennie

"view"

brenniebrennie

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

brenniebrennie

"Get" -> "Return" Missing a Returns block, e.g. ```python """... Returns: dict: An explanation of the return value. """

brenniebrennie

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 …

brenniebrennie

How about serialized_apps ?

brenniebrennie

_serialize_app? Needs a docstring.

brenniebrennie

How about OAuth2Page ?

brenniebrennie

Needs a doc-comment.

brenniebrennie

Needs a doc comment.

brenniebrennie

Needs a doc comment.

brenniebrennie

Needs a doc comment.

brenniebrennie

Needs a doc comment.

brenniebrennie

This would be better formatted as: ```javascript this.actions = [ { id: 'remove', label: gettext('Remove') }, { id: 'edit', label: …

brenniebrennie

Two lines between top-level declarations/definitions.

brenniebrennie

Needs a doc comment.

brenniebrennie

Needs a doc comment.

brenniebrennie

Needs a doc comment.

brenniebrennie

Two blank lines.

brenniebrennie

Needs a doc comment.

brenniebrennie

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

brenniebrennie

Should use dashes instead of underscores.

brenniebrennie

Needs a doc comment.

brenniebrennie

Needs a doc comment.

brenniebrennie

Needs a doc comment.

brenniebrennie

This is returning a dict.

chipx86chipx86

This can be one statement without impacting readability.

chipx86chipx86

This is returning a dict.

chipx86chipx86

No blank line here.

chipx86chipx86

Should be in alphabetical order.

chipx86chipx86

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

chipx86chipx86

No trailing comma.

chipx86chipx86

Should use .apply(this, arguments).

chipx86chipx86

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

chipx86chipx86

No traiing comma.

chipx86chipx86

Best to put a big /* XXX This is for testing */ or something here and below, so it doesn't …

chipx86chipx86

No trailing comma.

chipx86chipx86

The href should have a value. At the very least, "#".

chipx86chipx86

Text needs to be localized.

chipx86chipx86

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

chipx86chipx86

Needs a "Returns:" section.

chipx86chipx86

"Return a list of a user's ..."

chipx86chipx86

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

chipx86chipx86

This is wrapping too early.

chipx86chipx86

Missing a period.

chipx86chipx86

Missing a period.

chipx86chipx86

Missing a period.

chipx86chipx86

Space before {. Same below.

chipx86chipx86

Indentation levels in .less files should be 2 spaces.

chipx86chipx86

Ideally, these should be in alphabetical order.

chipx86chipx86

"an OAuth2"

chipx86chipx86

"string", lowercase.

chipx86chipx86

Missing a period.

chipx86chipx86

.extend isn't the right thing here.

chipx86chipx86

"Initialize."

chipx86chipx86

"object", lowercase.

chipx86chipx86

Missing a period.

chipx86chipx86

No blank line here.

chipx86chipx86

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

chipx86chipx86

"object"

chipx86chipx86

Missing period. Check further docs for this as well.

chipx86chipx86

Including templates in this way can be very dangerous, and isn't something we want to do. Looking through those templates, …

chipx86chipx86

Should be removed.

chipx86chipx86

This should be wrapped in gettext().

chipx86chipx86

This should be wrapped in gettext().

chipx86chipx86

Due to how the gettext localization string gathering works, you can't actually split lines and concatenate them. So they'll need …

chipx86chipx86

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

chipx86chipx86

No blank line.

chipx86chipx86

Space before {.

chipx86chipx86

Should be wrapped with gettext().

chipx86chipx86

Two blank lines between these.

chipx86chipx86

This needs a doc header. Same with the functions.

chipx86chipx86

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

chipx86chipx86

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

chipx86chipx86

IDs should be in the same form as class names, rather than CamelCase. Also, we're assigning too much meaning to …

chipx86chipx86
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/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
    
    
  2. 
      
brennie
  1. Since this is a WIP, please put [WIP] in the summary.

    1. what does WIP really mean ?

    2. WIP means "Work in Progress"

    3. When we use WIP and when we dont use it ?

  2. reviewboard/accounts/forms/pages.py (Diff revision 1)
     
     
    Show all issues

    How about OAuth2AppForm?

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

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

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

    "This form"

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

    "view"

  5. reviewboard/accounts/forms/pages.py (Diff revision 1)
     
     
    Show all issues

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

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

    2. I think we are not using API for now

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

  7. 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
    ]
    
  8. reviewboard/accounts/forms/pages.py (Diff revision 1)
     
     
    Show all issues

    How about serialized_apps ?

  9. reviewboard/accounts/forms/pages.py (Diff revision 1)
     
     
    Show all issues

    _serialize_app?

    Needs a docstring.

  10. reviewboard/accounts/pages.py (Diff revision 1)
     
     
    Show all issues

    How about OAuth2Page ?

    1. Let's consider OAuth2AppListingPage, similar to the one above

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

  11. Show all issues

    Needs a doc-comment.

  12. Show all issues

    Needs a doc comment.

  13. Show all issues

    Needs a doc comment.

  14. Show all issues

    Needs a doc comment.

  15. Show all issues

    Needs a doc comment.

  16. Show all issues

    This would be better formatted as:

    ```javascript
    this.actions = [
    {
    id: 'remove',
    label: gettext('Remove')
    },
    {
    id: 'edit',
    label: gettext('Edit')
    }
    ];

  17. Show all issues

    Two lines between top-level declarations/definitions.

  18. Show all issues

    Needs a doc comment.

  19. Show all issues

    Needs a doc comment.

  20. Show all issues

    Needs a doc comment.

  21. Show all issues

    Two blank lines.

  22. Show all issues

    Needs a doc comment.

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

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

    2. Instead of:

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

      do:

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

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

  24. Show all issues

    Should use dashes instead of underscores.

  25. Show all issues

    Needs a doc comment.

  26. Show all issues

    Needs a doc comment.

  27. Show all issues

    Needs a doc comment.

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

    This is returning a dict.

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

    This can be one statement without impacting readability.

  4. reviewboard/accounts/forms/pages.py (Diff revision 3)
     
     
    Show all issues

    This is returning a dict.

  5. reviewboard/accounts/forms/pages.py (Diff revision 3)
     
     
     
     
    Show all issues

    No blank line here.

  6. reviewboard/accounts/pages.py (Diff revision 3)
     
     
     
     
    Show all issues

    Should be in alphabetical order.

  7. Show all issues

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

  8. Show all issues

    No trailing comma.

  9. Show all issues

    Should use .apply(this, arguments).

    1. Removing this to implement the webapi

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

  11. Show all issues

    No traiing comma.

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

  13. Show all issues

    No trailing comma.

  14. Show all issues

    The href should have a value. At the very least, "#".

  15. Show all issues

    Text needs to be localized.

  16. Show all issues

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

  17. Show all issues

    Needs a "Returns:" section.

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

    "Return a list of a user's ..."

  3. reviewboard/accounts/forms/pages.py (Diff revision 7)
     
     
     
     
     
    Show all issues

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

  4. reviewboard/accounts/forms/pages.py (Diff revision 7)
     
     
     
    Show all issues

    This is wrapping too early.

  5. reviewboard/accounts/forms/pages.py (Diff revision 7)
     
     
    Show all issues

    Missing a period.

  6. reviewboard/accounts/forms/pages.py (Diff revision 7)
     
     
    Show all issues

    Missing a period.

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

    Missing a period.

  8. Show all issues

    Space before {.

    Same below.

  9. Show all issues

    Indentation levels in .less files should be 2 spaces.

  10. Show all issues

    Ideally, these should be in alphabetical order.

  11. Show all issues

    "an OAuth2"

  12. Show all issues

    "string", lowercase.

  13. Show all issues

    Missing a period.

  14. Show all issues

    .extend isn't the right thing here.

    1. I changed to prototype.defaults

  15. Show all issues

    "Initialize."

  16. Show all issues

    "object", lowercase.

  17. Show all issues

    Missing a period.

  18. Show all issues

    No blank line here.

  19. Show all issues

    "Create".

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

  20. Show all issues

    "object"

  21. Show all issues

    Missing period.

    Check further docs for this as well.

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

  23. Show all issues

    Should be removed.

  24. Show all issues

    This should be wrapped in gettext().

  25. Show all issues

    This should be wrapped in gettext().

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

  27. Show all issues

    render functions must return this, and should document that in the function docs.

    Same with others below.

  28. Show all issues

    No blank line.

  29. Show all issues

    Space before {.

  30. Show all issues

    Should be wrapped with gettext().

  31. Show all issues

    Two blank lines between these.

  32. Show all issues

    This needs a doc header.

    Same with the functions.

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

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

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

  36. 
      
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/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
    
    
  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/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
    
    
  2. 
      
LE
brennie
Review request changed
Status:
Discarded