[WIP] Extension handler app: New eggs can be installed on the fly with a remote URL.

Review Request #3795 — Created Feb. 2, 2013 and discarded

ayrus
Djblets
master
djblets
Started on the extension handler module for djblets. Christian suggested that the extension browse/install/remove be a new app. These changes bring in a new app to djblets called "exthandler".

Currently, you can visit <rb>/admin/extensions/manager to view the changes. For now, you can install an extension on the fly. Point the URL field to an archive containing a RB extension (or any python egg for that matter), provide the package name and it'll be installed in the background. Once done, you can enable the newly installed extension at /admin/extensions. 

Known issues/limitations:
-If you try to install an extension that has previously been installed, it doesn't handle it quite well. Some searching ( http://trac.edgewall.org/ticket/7014 ) shows that it's a known issue. Since the already installed extension is being imported by the code elsewhere, the request fails. I'm yet to fix this.
-The archive you input has to have the setup.py file directly within; not in a subdirectory.
-If you enter a malformed URL (say a random page which leads to a 404) it isn't handled well. I'm yet to fix this too.


Some changes are dependant on reviewboard codebase too -- a new URL is added to the RB url.py that handles all /admin/extensions/manager/* urls to exthandler and the app exthandler itself has to be added to RB's settings.py under INSTALLED_APPS otherwise the templating fails.

Also fixed entry_points issue on the main extension module and hence the changes to djblets/extensions/base.py.
Tested with two RB extensions.

To test make sure you *don't* have reviewbot and/or rbxmlreview installed and they don't show up at /admin/extensions/

1) Test with rbxmlreview. Provide "http://andromeda.ayrus.net/rbxmlreview.zip" as the URL and "rbxmlreview" as the package name. A success message is expected. Click on the activate link and rbxmlreview should be added to the extensions list on the interface.

2) Test with Review Bot. Provide "http://andromeda.ayrus.net/extension.zip" as the URL and "review-bot-extension" as the package name. A success message is expected. Click on the activate link and Review Bot should be added to the extensions list on the interface.
Description From Last Updated

Col: 1 W191 indentation contains tabs

reviewbotreviewbot

Col: 1 W191 indentation contains tabs

reviewbotreviewbot

Col: 1 W191 indentation contains tabs

reviewbotreviewbot

Col: 5 E901 IndentationError

reviewbotreviewbot

Col: 1 E101 indentation contains mixed spaces and tabs

reviewbotreviewbot

Col: 80 E501 line too long (83 > 79 characters)

reviewbotreviewbot

Col: 80 E501 line too long (83 > 79 characters)

reviewbotreviewbot

We should avoid the tag. It's rarely needed, and implies more about the spacing and style than the HTML should. ...

chipx86chipx86

Docstrings should always be in this format: """One-line summary without wrapping. Description. """

chipx86chipx86

Should be extension_url.

chipx86chipx86

No need for the blank line between these. You will need to localize these. If you look at other files, ...

chipx86chipx86

No blank line after the 1 space indentation in HTML.

chipx86chipx86

No spaces in the {{..}}

chipx86chipx86

We apply indentation to our template tags inside the {% .. %}, not outside. So: {% if user_message %} {% ...

chipx86chipx86

Indentation is off. This should be like: urlpatterns = patterns(..., url(...), ) Note also that you should be using url() ...

chipx86chipx86

Imports should be in alphabetical order.

chipx86chipx86

Missing a docstring.

chipx86chipx86

These blank lines should be gone.

chipx86chipx86

This one too.

chipx86chipx86

And this.

chipx86chipx86

These too (these statements are all related, so they should be grouped).

chipx86chipx86

Put this inside the except.

chipx86chipx86

No blank line here.

chipx86chipx86

locals() should never be passed in. Instead, you should pass in a RequestContext(request, {...}) as the second parameter and be ...

chipx86chipx86

Col: 80 E501 line too long (83 > 79 characters)

reviewbotreviewbot

Goes inside the except:

chipx86chipx86

No blank line.

chipx86chipx86

The template should be passed the exception as an error, and should render the text itself, rather than being provided ...

chipx86chipx86

Same comment about locals.

chipx86chipx86

Col: 80 E501 line too long (83 > 79 characters)

reviewbotreviewbot

Not sure what this is? Probably can go away if we do the above (passing in the error and having ...

chipx86chipx86

These blank lines should go away.

chipx86chipx86

Same comment here. It occurs to me that we're rendering the same template over and over. We should ideally do ...

chipx86chipx86

Col: 80 E501 line too long (83 > 79 characters)

reviewbotreviewbot

Col: 80 E501 line too long (83 > 79 characters)

reviewbotreviewbot

Col: 5 E128 continuation line under-indented for visual indent

reviewbotreviewbot

Col: 15 E271 multiple spaces after keyword

reviewbotreviewbot

Col: 13 E126 continuation line over-indented for hanging indent

reviewbotreviewbot

Col: 9 E123 closing bracket does not match indentation of opening bracket's line

reviewbotreviewbot

Col: 5 E128 continuation line under-indented for visual indent

reviewbotreviewbot
reviewbot
  1. This is a review from Review Bot.
      Tool: PEP8 Style Checker
      Processed Files:
        djblets/extensions/base.py
        djblets/exthandler/urls.py
        djblets/settings.py
        djblets/exthandler/forms.py
        djblets/exthandler/views.py
      Ignored Files:
        djblets/extensions/templates/extensions/extension_list.html
        .gitignore
        djblets/exthandler/templates/exthandler/extension_manager.html
    
    
  2. djblets/exthandler/forms.py (Diff revision 1)
     
     
    Col: 1
     W191 indentation contains tabs
    
  3. djblets/exthandler/forms.py (Diff revision 1)
     
     
    Col: 1
     W191 indentation contains tabs
    
  4. djblets/exthandler/forms.py (Diff revision 1)
     
     
    Col: 1
     W191 indentation contains tabs
    
  5. djblets/exthandler/forms.py (Diff revision 1)
     
     
    Col: 5
     E901 IndentationError
  6. djblets/exthandler/forms.py (Diff revision 1)
     
     
    Col: 1
     E101 indentation contains mixed spaces and tabs
    
  7. djblets/exthandler/views.py (Diff revision 1)
     
     
    Col: 80
     E501 line too long (83 > 79 characters)
    
  8. djblets/exthandler/views.py (Diff revision 1)
     
     
    Col: 80
     E501 line too long (83 > 79 characters)
    
  9. 
      
AY
reviewbot
  1. This is a review from Review Bot.
      Tool: PEP8 Style Checker
      Processed Files:
        djblets/extensions/base.py
        djblets/exthandler/urls.py
        djblets/settings.py
        djblets/exthandler/forms.py
        djblets/exthandler/views.py
      Ignored Files:
        djblets/extensions/templates/extensions/extension_list.html
        .gitignore
        djblets/exthandler/templates/exthandler/extension_manager.html
    
    
  2. djblets/exthandler/views.py (Diff revision 2)
     
     
    Col: 80
     E501 line too long (83 > 79 characters)
    
  3. djblets/exthandler/views.py (Diff revision 2)
     
     
    Col: 80
     E501 line too long (83 > 79 characters)
    
  4. 
      
AY
AY
chipx86
  1. This is a great first stab. There's going to be a lot of comments in here though about fixing style issues and being more in line with the rest of our codebase in ways.
    
    I'd suggest a change in the organization of the code. When I was discussing adding a new module, my thoughts were that we'd have one that's specific to browsing for remote extensions (communicating with a remote Extension Store). I'd suggest naming this module "extensionbrowser".
    
    I consider things like installing an extension to be more base-level extension functionality, which should live in ExtensionManager.
    
    The form you have is probably best suited for extensionbrowser (since eventually we'd have a more full UI on top of it), and it would call out to, say, ExtensionBrowser.install_extension_url.
  2. We should avoid the <br /> tag. It's rarely needed, and implies more about the spacing and style than the HTML should.
    
    Instead, there should be something like a <ul class="actions">, with this being one of them.
    
    Instead of hard-coding a URL, we want to use Django's URL resolution stuff. So, the URL registration for the Add Extension page would have a name (say, "add-extension"), and this would reference it with: 
    
       {% url "add-extension" %}
  3. djblets/exthandler/forms.py (Diff revision 2)
     
     
     
     
    Docstrings should always be in this format:
    
    """One-line summary without wrapping.
    
    Description.
    """
  4. djblets/exthandler/forms.py (Diff revision 2)
     
     
     
    Should be extension_url.
  5. djblets/exthandler/forms.py (Diff revision 2)
     
     
     
     
    No need for the blank line between these.
    
    You will need to localize these. If you look at other files, you should see where "_" is imported and used like _("Extension URL"). These mark the strings for translation and handle looking up from the appropriate language bundle.
  6. djblets/exthandler/templates/exthandler/extension_manager.html (Diff revision 2)
     
     
     
     
     
     
     
     
     
    No blank line after the <div>
    
    1 space indentation in HTML.
  7. We apply indentation to our template tags inside the {% .. %}, not outside. So:
    
    {% if user_message %}
    {%  if user_message == 1 %}
    
    etc.
    
    These should also be indented by one more, since they're inside the {% block %}.
    
    In this case, though, we should do what I said in another comment and check if an error is passed, and then operate based on whether that has a value.
  8. djblets/exthandler/urls.py (Diff revision 2)
     
     
     
     
    Indentation is off. This should be like:
    
        urlpatterns = patterns(...,
            url(...),
        )
    
    Note also that you should be using url() instead of just a tuple. This will allow you to give it a name (url(..., name="add-extension")) for referencing in the template (see my other comment on this).
  9. djblets/exthandler/views.py (Diff revision 2)
     
     
     
     
    Imports should be in alphabetical order.
  10. djblets/exthandler/views.py (Diff revision 2)
     
     
     
    Missing a docstring.
  11. djblets/exthandler/views.py (Diff revision 2)
     
     
     
     
     
     
    These blank lines should be gone.
  12. djblets/exthandler/views.py (Diff revision 2)
     
     
     
     
    This one too.
  13. djblets/exthandler/views.py (Diff revision 2)
     
     
     
     
    And this.
  14. djblets/exthandler/views.py (Diff revision 2)
     
     
     
     
     
     
     
    These too (these statements are all related, so they should be grouped).
  15. djblets/exthandler/views.py (Diff revision 2)
     
     
     
    Put this inside the except.
  16. djblets/exthandler/views.py (Diff revision 2)
     
     
     
     
    No blank line here.
  17. djblets/exthandler/views.py (Diff revision 2)
     
     
    locals() should never be passed in. Instead, you should pass in a RequestContext(request, {...}) as the second parameter and be explicit about the variables you provide. The context_instance= shouldn't be there. Just this format:
    
        return render_to_response(template_name, RequestContext(request, {
            'blah': 'blah',
            ...
        })
  18. djblets/exthandler/views.py (Diff revision 2)
     
     
    Goes inside the except:
  19. djblets/exthandler/views.py (Diff revision 2)
     
     
     
     
    No blank line.
  20. djblets/exthandler/views.py (Diff revision 2)
     
     
    The template should be passed the exception as an error, and should render the text itself, rather than being provided the text.
  21. djblets/exthandler/views.py (Diff revision 2)
     
     
    Same comment about locals.
  22. djblets/exthandler/views.py (Diff revision 2)
     
     
    Not sure what this is? Probably can go away if we do the above (passing in the error and having the template conditionally check it and display its own thing).
  23. djblets/exthandler/views.py (Diff revision 2)
     
     
     
     
     
     
    These blank lines should go away.
  24. djblets/exthandler/views.py (Diff revision 2)
     
     
    Same comment here.
    
    It occurs to me that we're rendering the same template over and over. We should ideally do it only once at the end, and prepare the contents for it above.
  25. 
      
AY
reviewbot
  1. This is a review from Review Bot.
      Tool: PEP8 Style Checker
      Processed Files:
        djblets/extensions/base.py
        djblets/exthandler/urls.py
        djblets/settings.py
        djblets/exthandler/forms.py
        djblets/exthandler/views.py
      Ignored Files:
        djblets/extensions/templates/extensions/extension_list.html
        .gitignore
        djblets/exthandler/templates/exthandler/extension_manager.html
    
    
  2. djblets/exthandler/views.py (Diff revision 3)
     
     
    Col: 80
     E501 line too long (83 > 79 characters)
    
  3. djblets/exthandler/views.py (Diff revision 3)
     
     
    Col: 80
     E501 line too long (83 > 79 characters)
    
  4. 
      
AY
reviewbot
  1. This is a review from Review Bot.
      Tool: PEP8 Style Checker
      Processed Files:
        djblets/extensionbrowser/urls.py
        djblets/extensionbrowser/forms.py
        djblets/extensions/base.py
        djblets/settings.py
        djblets/extensionbrowser/views.py
      Ignored Files:
        djblets/extensions/templates/extensions/extension_list.html
        .gitignore
        djblets/extensionbrowser/templates/extensionbrowser/extension_manager.html
    
    
  2. djblets/extensionbrowser/urls.py (Diff revision 4)
     
     
    Col: 5
     E128 continuation line under-indented for visual indent
    
  3. djblets/extensionbrowser/views.py (Diff revision 4)
     
     
    Col: 15
     E271 multiple spaces after keyword
    
  4. djblets/extensionbrowser/views.py (Diff revision 4)
     
     
    Col: 13
     E126 continuation line over-indented for hanging indent
    
  5. djblets/extensionbrowser/views.py (Diff revision 4)
     
     
    Col: 9
     E123 closing bracket does not match indentation of opening bracket's line
    
  6. 
      
AY
reviewbot
  1. This is a review from Review Bot.
      Tool: PEP8 Style Checker
      Processed Files:
        djblets/extensionbrowser/urls.py
        djblets/extensionbrowser/forms.py
        djblets/extensions/base.py
        djblets/settings.py
        djblets/extensionbrowser/views.py
      Ignored Files:
        djblets/extensions/templates/extensions/extension_list.html
        .gitignore
        djblets/extensionbrowser/templates/extensionbrowser/extension_manager.html
    
    
  2. djblets/extensionbrowser/urls.py (Diff revision 5)
     
     
    Col: 5
     E128 continuation line under-indented for visual indent
    
  3. 
      
AY
Review request changed

Status: Discarded

Loading...