• 
      

    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:
    Completed
    Change Summary:
    Pushed to release-2.6.x (3bd1e9e)