Add a user-editable OAuth2 Application form

Review Request #9013 - Created June 13, 2017 and submitted

Barret Rennie
Review Board
release-3.0.x
9002, 9040, 9031
reviewboard

The UserApplicationForm is identical to the ApplicationForm, except
that it stops the user from directly editing special fields, such as:

  • local_site, since we do not expose Local Site behaviour by default;
  • user, so they cannot re-assign the application to another users;
  • extra_data, since this is only meant for internal use; and
  • skip_authorization, since this is only intended for trusted apps.

This form is accessible via the user's profile under the "OAuth2
Applications" page.

  • Ran unit tests.
  • Created, edited, deleted applicaitons.
Loading file attachments...

  • 0
  • 0
  • 60
  • 11
  • 71
Description From Last Updated
Checks run (1 failed, 1 succeeded)
flake8 failed.
JSHint passed.

flake8

Barret Rennie
David Trowbridge
  1. 
      
  2. Can we have some kind of empty state here that says there are no configured applications?

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

    Docstring?

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

    Docstring?

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

    Needs to include the local_site_name parameter.

    1. No it does not. The account page view is not mounted per-local-site. Im just using this instead of also importing reverse.

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

    Needs Returns:

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

    Docstring?

  8. reviewboard/accounts/views.py (Diff revision 2)
     
     

    Docstring?

  9. reviewboard/admin/utils.py (Diff revision 2)
     
     

    Module docstring?

  10. reviewboard/admin/utils.py (Diff revision 2)
     
     

    Docstring?

  11. We should use </div> instead of the XHTML syntax.

  12. 
      
Barret Rennie
Review request changed

Change Summary:

  • Address David's issues.
  • Disable skip_authorization field.
  • Improve the UI.

Description:

   

The UserApplicationForm is identical to the ApplicationForm, except

    that it stops the user from directly editing special fields, such as:

   
   
  • local_site, since we do not expose Local Site behaviour by default;
~  
  • user, so they cannot re-assign the application to another user; and
~  
  • extra_data, since this is only meant for internal use.
  ~
  • user, so they cannot re-assign the application to another users;
  ~
  • extra_data, since this is only meant for internal use; and
  +
  • skip_authorization, since this is only intended for trusted apps.
   
   

This form is accessible via the user's profile under the "OAuth2

    Applications" page.

Diff:

Revision 3 (+735 -7)

Show changes

Removed Files:

Added Files:

Checks run (1 failed, 1 succeeded)

flake8 failed.
JSHint passed.

flake8

Barret Rennie
Barret Rennie
Barret Rennie
Review request changed

Change Summary:

Add setup dialog

Diff:

Revision 6 (+286 -24)

Show changes

Removed Files:

Added Files:

Checks run (1 failed, 1 succeeded)

flake8 passed.
JSHint failed.

JSHint

Barret Rennie
Barret Rennie
Christian Hammond
  1. 
      
  2. reviewboard/accounts/forms/pages.py (Diff revision 7)
     
     

    This URL isn't on a Local Site, and we're not providing Local Site information, so reverse is fine.

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

    We should provide the request as an argument, so feature checkers can factor it in.

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

    It's not immediately clear what this is for. Can you elaborate?

  5. reviewboard/accounts/urls.py (Diff revision 7)
     
     
     
     
     
     
     

    This can be condensed:

    url(r'^preferences/oauth2-application/((?P<app_id>[0-9]+)/)?$',
        accounts_views.edit_oauth_app,
        name='edit-oauth-app'),
    
  6. reviewboard/accounts/views.py (Diff revision 7)
     
     
     
     
     

    Alphabetical order.

  7. reviewboard/accounts/views.py (Diff revision 7)
     
     
     
     
     

    Two blank lines.

  8. reviewboard/accounts/views.py (Diff revision 7)
     
     
     

    Swap these. We want to test for login first.

  9. reviewboard/accounts/views.py (Diff revision 7)
     
     
     
     
     
     
     
     
     

    You should use get_object_or_404 instead.

  10. reviewboard/accounts/views.py (Diff revision 7)
     
     

    This and the other Local Site-related code below will never work. This URL isn't under a /s/<localsite>/ URL, so there's never going to be a Local Site listed.

    I think what's going to have to happen is you'll have to fetch without the Local Site, then check if there is a Local Site and that the user has access to it (covering the case where a user has created one but since left the company -- we don't want them sabotaging something).

    Then we'll have to include the local_site in the form data, but validate that the user has access to it.

  11. reviewboard/accounts/views.py (Diff revision 7)
     
     

    This doesn't seem to match what's in the form.

    Let's import the form and make sure we're always using the value it defines.

    1. The ID comes off the page, not the form. But yes, I agree, we should import the page.

  12. reviewboard/accounts/views.py (Diff revision 7)
     
     
     
     

    No blank line here.

  13. reviewboard/accounts/views.py (Diff revision 7)
     
     

    No trailing comma after form. Or if this is meant to be a tuple, it should be wrapped in parens.

  14. reviewboard/admin/utils.py (Diff revision 7)
     
     

    This is probably best moved to djblets.forms somewhere.

    We have a get_fieldsets template tag as well, but it's a bit different in what it returns.

  15. reviewboard/admin/utils.py (Diff revision 7)
     
     

    We don't always necessarily want this. This should be up to callers.

  16. reviewboard/admin/utils.py (Diff revision 7)
     
     
     

    I don't think we want to assume that all collapsed content should be excluded. Seems like that should be a flag, like ignore_collapsed.

    Also, tuple() can just be ().

  17. reviewboard/oauth/forms.py (Diff revision 7)
     
     
     

    Swap these.

  18. reviewboard/oauth/forms.py (Diff revision 7)
     
     
  19. reviewboard/oauth/forms.py (Diff revision 7)
     
     
     
     
     
     
     
     
     

    How about:

    instance = super(UserApplicationForm, self).save(commit=False)
    
    if not self.instance.pk:
        instance.user = self.user
    
    if commit:
        instance.save()
    
  20. reviewboard/oauth/tests.py (Diff revision 7)
     
     

    No parens.

  21. This is being overrided below.

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

    Why do we need to override this? Input classes should generally be left alone.

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

    Similarly here (and for other common styles), we ideally shouldn't have versions specific to the OAuth2 page. We should have consistent definitions that apply to all relevant pages.

  24. render() chains, so you can include this in the chain. We usually do it as one entry as render().$el.

  25. This is really going to be tough to maintain. Can we split across lines? We can use {% spaceless %}.

  26. reviewboard/templates/accounts/edit_oauth_app.html (Diff revision 7)
     
     
     
     
     
     
     
     
     
     
     
     

    So ideally we'd use the djblets_forms/admin/form_fieldsets.html template here. There are two things that prevent this, though:

    1. It uses the get_fieldsets filter, which doesn't exclude or remove collapsed entries.
    2. It doesn't use fieldset.name, since get_fieldsets returns a tuple of (name, fieldset).

    I think the best route to go (since I really don't want to duplicate fieldset rendering logic) is to split the fieldset rendering of form_fieldsets.html into a form_fieldset.html. Then you could easily use it with:

    {% for fieldset in fieldsets %}
    {%  include "djblets_forms/admin/form_fieldset.html" with fieldset_title=fieldset.name %} 
    {% endfor %}
    

    (Also, can we change fieldset.name to fieldset.title? It's more correct.)

  27. Save and Cancel need to be localized.

  28. This doesn't match the anchor used in the form. We should provide that to the template.

  29. No blank line needed.

  30. 
      
Barret Rennie
Barret Rennie
Barret Rennie
Review request changed

Checks run (1 failed, 1 succeeded)

flake8 failed.
JSHint passed.

flake8

Barret Rennie
Barret Rennie
Review request changed
David Trowbridge
  1. 
      
  2. reviewboard/accounts/pages.py (Diff revision 10)
     
     
     
     
     
     
     
     

    Mind making this alphabetical?

  3. reviewboard/accounts/views.py (Diff revision 10)
     
     
     

    e before o

  4. reviewboard/staticbundles.py (Diff revision 10)
     
     

    I don't see this file in your change.

  5. 
      
Barret Rennie
Review request changed

Checks run (1 failed, 1 succeeded)

flake8 passed.
JSHint failed.

JSHint

Barret Rennie
Review request changed

Checks run (1 failed, 1 succeeded)

flake8 passed.
JSHint failed.

JSHint

Barret Rennie
Barret Rennie
David Trowbridge
  1. Ship It!
  2. 
      
Barret Rennie
Review request changed

Status: Closed (submitted)

Change Summary:

Pushed to release-3.0.x (34c7e08)
Loading...