Use Djblets' dynamic config pages for account pages

Review Request #7828 — Created Dec. 24, 2015 and submitted

Information

Review Board
release-2.6.x

Reviewers

Account pages now use the ConfigPageRegistry and
DynamicConfigPageMixin from Djblets instead of using custom logic.
The external API has been kept the same, but it has been rewritten to
use Registry methods instead. Unit test logic has been updated to
check for the right exceptions.

  • Ran unit tests.
Description From Last Updated

Swap these.

chipx86chipx86

Yielding here is going to be slower than just doing: return (AccountSettingsPage, APITokensPage, ...) We're already building the tuple for …

chipx86chipx86

Maybe just set this directory below in AccountPage, and access it later as AccountPage.registry (similar to how we do ReviewRequest.objects)?

chipx86chipx86

Blank line between these.

chipx86chipx86

Are we aiming to deprecate these, or to keep them as the official API? I ask because it seems it'd …

chipx86chipx86

I think you can just return iter(pages).

chipx86chipx86

No need for parens.

chipx86chipx86

We should use AccountPage.registry instead of pages.

chipx86chipx86

I think you just changed the order of how things are shown on the user settings page.

daviddavid
david
  1. Ship It!
  2. 
      
chipx86
  1. 
      
  2. reviewboard/accounts/pages.py (Diff revision 1)
     
     
     
    Show all issues

    Swap these.

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

    Yielding here is going to be slower than just doing:

    return (AccountSettingsPage, APITokensPage, ...)
    

    We're already building the tuple for the for loop, and generators have additional expense to them, often being slower for smaller lists of static things.

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

    Maybe just set this directory below in AccountPage, and access it later as AccountPage.registry (similar to how we do ReviewRequest.objects)?

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

    Blank line between these.

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

    Are we aiming to deprecate these, or to keep them as the official API? I ask because it seems it'd be nice to exclusively use AccountPage.registry.blah() for calls, but I know some of these wrap the registry calls in more detailed ways (logging exceptions, for instance).

    I wonder if we should move all the detail of these global functions into the registry subclass, mark all these as deprecated, and aim to use those exclusively.

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

    I think you can just return iter(pages).

  8. reviewboard/accounts/tests.py (Diff revision 1)
     
     
    Show all issues

    No need for parens.

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

    We should use AccountPage.registry instead of pages.

  10. 
      
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        reviewboard/accounts/tests.py
        reviewboard/accounts/pages.py
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/accounts/tests.py
        reviewboard/accounts/pages.py
    
    
  2. 
      
brennie
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        reviewboard/accounts/tests.py
        reviewboard/accounts/pages.py
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/accounts/tests.py
        reviewboard/accounts/pages.py
    
    
  2. 
      
david
  1. 
      
  2. reviewboard/accounts/pages.py (Diff revision 2)
     
     
     
    Show all issues

    I think you just changed the order of how things are shown on the user settings page.

  3. 
      
brennie
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        reviewboard/accounts/tests.py
        reviewboard/accounts/pages.py
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/accounts/tests.py
        reviewboard/accounts/pages.py
    
    
  2. 
      
david
  1. Given the issue with the page ordering, how about some functional testing? I'd like to hear how you've exercised the UI.

  2. 
      
brennie
Review request changed

Status: Closed (submitted)

Change Summary:

Pushed to release-2.6.x (3bd1e9e)
Loading...