Add a user-editable OAuth2 Application form

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

Information

Review Board
release-3.0.x

Reviewers

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.

Description From Last Updated

Can we have some kind of empty state here that says there are no configured applications?

daviddavid

W391 blank line at end of file

reviewbotreviewbot

F811 redefinition of unused 'test_set_local_site' from line 155

reviewbotreviewbot

F811 redefinition of unused 'test_set_local_site' from line 201

reviewbotreviewbot

Docstring?

daviddavid

Docstring?

daviddavid

Needs to include the local_site_name parameter.

daviddavid

Needs Returns:

daviddavid

Docstring?

daviddavid

Docstring?

daviddavid

Module docstring?

daviddavid

Docstring?

daviddavid

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

daviddavid

W292 no newline at end of file

reviewbotreviewbot

W292 no newline at end of file

reviewbotreviewbot

Col: 52 Missing semicolon.

reviewbotreviewbot

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

chipx86chipx86

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

chipx86chipx86

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

chipx86chipx86

This can be condensed: url(r'^preferences/oauth2-application/((?P<app_id>[0-9]+)/)?$', accounts_views.edit_oauth_app, name='edit-oauth-app'),

chipx86chipx86

Alphabetical order.

chipx86chipx86

Two blank lines.

chipx86chipx86

Swap these. We want to test for login first.

chipx86chipx86

You should use get_object_or_404 instead.

chipx86chipx86

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

chipx86chipx86

This doesn't seem to match what's in the form. Let's import the form and make sure we're always using the …

chipx86chipx86

No blank line here.

chipx86chipx86

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

chipx86chipx86

This is probably best moved to djblets.forms somewhere. We have a get_fieldsets template tag as well, but it's a bit …

chipx86chipx86

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

chipx86chipx86

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

chipx86chipx86

Swap these.

chipx86chipx86

"or"

chipx86chipx86

How about: instance = super(UserApplicationForm, self).save(commit=False) if not self.instance.pk: instance.user = self.user if commit: instance.save()

chipx86chipx86

No parens.

chipx86chipx86

This is being overrided below.

chipx86chipx86

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

chipx86chipx86

Similarly here (and for other common styles), we ideally shouldn't have versions specific to the OAuth2 page. We should have …

chipx86chipx86

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

chipx86chipx86

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

chipx86chipx86

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

chipx86chipx86

Save and Cancel need to be localized.

chipx86chipx86

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

chipx86chipx86

No blank line needed.

chipx86chipx86

E501 line too long (81 > 79 characters)

reviewbotreviewbot

F841 local variable 'local_site' is assigned to but never used

reviewbotreviewbot

Mind making this alphabetical?

daviddavid

e before o

daviddavid

Doc comment?

daviddavid

Doc comment?

daviddavid

Trailing whitespace.

daviddavid

Trailing whitespace.

daviddavid

Doc comment?

daviddavid

Col: 52 Missing semicolon.

reviewbotreviewbot

Doc comment?

daviddavid

Doc comment?

daviddavid

Doc comment?

daviddavid

Trailing comma.

daviddavid

No blank lines here.

daviddavid

Doc comment?

daviddavid

I don't see this file in your change.

daviddavid

Col: 6 Expected '}' to match '{' from line 211 and instead saw ';'.

reviewbotreviewbot

Col: 2 Missing semicolon.

reviewbotreviewbot

Col: 2 Expected an identifier and instead saw ')'.

reviewbotreviewbot

Col: 2 Expected an assignment or function call and instead saw an expression.

reviewbotreviewbot

Col: 1 Expected ')' and instead saw '}'.

reviewbotreviewbot

Col: 6 Expected '}' to match '{' from line 211 and instead saw ';'.

reviewbotreviewbot

Col: 1 Expected ')' and instead saw '}'.

reviewbotreviewbot

Col: 2 Missing semicolon.

reviewbotreviewbot

Col: 2 Expected an identifier and instead saw ')'.

reviewbotreviewbot

Col: 2 Expected an assignment or function call and instead saw an expression.

reviewbotreviewbot
Checks run (1 failed, 1 succeeded)
flake8 failed.
JSHint passed.

flake8

brennie
david
  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. 
      
brennie
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

brennie
brennie
brennie
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

brennie
brennie
chipx86
  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. 
      
brennie
brennie
brennie
Review request changed

Checks run (1 failed, 1 succeeded)

flake8 failed.
JSHint passed.

flake8

brennie
brennie
Review request changed
david
  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. 
      
brennie
Review request changed

Checks run (1 failed, 1 succeeded)

flake8 passed.
JSHint failed.

JSHint

brennie
Review request changed

Checks run (1 failed, 1 succeeded)

flake8 passed.
JSHint failed.

JSHint

brennie
brennie
david
  1. Ship It!
  2. 
      
brennie
Review request changed

Status: Closed (submitted)

Change Summary:

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