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

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

Minh Le Hoang
Review Board
master
7897
7941
reviewboard
brennie, chipx86

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.

Loading file attachments...

  • 0
  • 0
  • 75
  • 1
  • 76
Description From Last Updated
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/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. 
      
Barret Rennie
  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)
     
     

    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)
     
     

    "This form"

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

    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)
     
     

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

    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)
     
     

    How about serialized_apps ?

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

    _serialize_app?

    Needs a docstring.

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

    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. This would be better formatted as:

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

  12. Two lines between top-level declarations/definitions.

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

  14. Should use dashes instead of underscores.

  15. 
      
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/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. 
      
Minh Le Hoang
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/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. 
      
Minh Le Hoang
Minh Le Hoang
Christian Hammond
  1. 
      
  2. reviewboard/accounts/forms/pages.py (Diff revision 3)
     
     

    This is returning a dict.

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

    This can be one statement without impacting readability.

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

    This is returning a dict.

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

    No blank line here.

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

    Should be in alphabetical order.

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

  8. Should use .apply(this, arguments).

    1. Removing this to implement the webapi

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

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

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

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

  13. Needs a "Returns:" section.

  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/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. 
      
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/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. 
      
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/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. 
      
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/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. 
      
Christian Hammond
  1. 
      
  2. reviewboard/accounts/forms/pages.py (Diff revision 7)
     
     

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

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

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

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

    This is wrapping too early.

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

    Missing a period.

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

    Missing a period.

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

    Missing a period.

  8. Space before {.

    Same below.

  9. Indentation levels in .less files should be 2 spaces.

  10. Ideally, these should be in alphabetical order.

  11. .extend isn't the right thing here.

    1. I changed to prototype.defaults

  12. "Create".

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

  13. Missing period.

    Check further docs for this as well.

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

  15. This should be wrapped in gettext().

  16. This should be wrapped in gettext().

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

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

    Same with others below.

  19. Should be wrapped with gettext().

  20. Two blank lines between these.

  21. This needs a doc header.

    Same with the functions.

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

  23. reviewboard/static/rb/js/resources/models/oauth2ClientAppModel.js (Diff revision 7)
     
     
     
     
     
     
     
     
     
     

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

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

  25. 
      
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/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. 
      
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/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. 
      
Minh Le Hoang
Barret Rennie
Review request changed

Status: Discarded

Loading...