WIP: Base: Review UI URLs

Review Request #10758 — Created Oct. 20, 2019 and updated

nicolelisa
Review Board
master
reviewboard, students
set up base for review ui urls


Summary Author
set up base for review ui urls
nicolelisa
wip for url rendering with ajax started
nicolelisa
add testing and fix patterns
nicolelisa
reviewbot changes
nicolelisa
Description From Last Updated

E402 module level import not at top of file

reviewbotreviewbot

E501 line too long (82 > 79 characters)

reviewbotreviewbot

F401 'django.conf.urls.include' imported but unused

reviewbotreviewbot

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

reviewbotreviewbot

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

reviewbotreviewbot

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

reviewbotreviewbot

F401 'django.core.urlresolvers.reverse_lazy' imported but unused

reviewbotreviewbot

F401 'django.core.urlresolvers.reverse' imported but unused

reviewbotreviewbot

F401 'django.http.Http404' imported but unused

reviewbotreviewbot

F401 'django.core.urlresolvers.resolve' imported but unused

reviewbotreviewbot

F401 'django.db.models.Q' imported but unused

reviewbotreviewbot

F401 'django.http.Http404' imported but unused

reviewbotreviewbot

F401 'django.http.HttpResponseForbidden' imported but unused

reviewbotreviewbot

F401 'django.core.urlresolvers.resolve' imported but unused

reviewbotreviewbot

F401 'django.db.models.Q' imported but unused

reviewbotreviewbot

F401 'django.shortcuts.get_object_or_404' imported but unused

reviewbotreviewbot

F401 'reviewboard.reviews.views.ReviewFileAttachmentView' imported but unused

reviewbotreviewbot

This shouldn't be staticmethod and should be the get method, ie def get(self, ...):

brenniebrennie

Once ReviewFileAttachmentViewMixin is not using staticmethods, this should just be self.get_attachments, etc.

brenniebrennie

Here too

brenniebrennie

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

reviewbotreviewbot

W293 blank line contains whitespace

reviewbotreviewbot

This should be TextBasedReviewUIView.as_view()

brenniebrennie

These should not be staticmethods; these should be regular methods instead.

brenniebrennie

These should not be staticmethods; these should be regular methods instead.

brenniebrennie

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

reviewbotreviewbot

E303 too many blank lines (2)

reviewbotreviewbot

In addition to these new tests, you should also have tests that assert that unregistering the review UI has the ...

amalik2amalik2

This import statement should probably be moved to the top of this file for consistency.

amalik2amalik2

Is there any way this can be done without modifying a global variable in another file? If there isn't, then ...

amalik2amalik2

This import statement should probably be moved to the top of this file for consistency.

amalik2amalik2

F401 'django.http.HttpResponse' imported but unused

reviewbotreviewbot

F401 'djblets.util.decorators.augment_method_from' imported but unused

reviewbotreviewbot

F401 'djblets.webapi.resources.WebAPIResource' imported but unused

reviewbotreviewbot

F401 'reviewboard.reviews.models.ReviewRequest' imported but unused

reviewbotreviewbot

F401 'reviewboard.reviews.views.ReviewFileAttachmentView' imported but unused

reviewbotreviewbot

F401 'django.views.generic.base.ContextMixin' imported but unused

reviewbotreviewbot

F401 'django.views.generic.base.TemplateView' imported but unused

reviewbotreviewbot

F401 'django.views.generic.base.RedirectView' imported but unused

reviewbotreviewbot

F401 'reviewboard.webapi.decorators.webapi_check_local_site' imported but unused

reviewbotreviewbot

You should write some tests to make sure that this method works as expected

amalik2amalik2

E501 line too long (81 > 79 characters)

reviewbotreviewbot

E126 continuation line over-indented for hanging indent

reviewbotreviewbot

Is kwargs meant to be used by individual review UIs in the future? I think all of the query parameter ...

amalik2amalik2

Just curious about this, how can specific review UIs like the JSON or XML ones render text in different ways ...

amalik2amalik2

E501 line too long (84 > 79 characters)

reviewbotreviewbot

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

reviewbotreviewbot

E303 too many blank lines (2)

reviewbotreviewbot

F401 'django.db.models.Q' imported but unused

reviewbotreviewbot

F401 'django.shortcuts.get_object_or_404' imported but unused

reviewbotreviewbot

F401 'reviewboard.reviews.views_mixins.ReviewRequestViewMixin' imported but unused

reviewbotreviewbot

E126 continuation line over-indented for hanging indent

reviewbotreviewbot

F401 'django.http.Http404' imported but unused

reviewbotreviewbot

F401 'django.shortcuts.render_to_response' imported but unused

reviewbotreviewbot

F401 'reviewboard.reviews.views.ReviewFileAttachmentView' imported but unused

reviewbotreviewbot

F401 'django.views.generic.base.View' imported but unused

reviewbotreviewbot

F811 redefinition of unused 'ReviewFileAttachmentViewMixin' from line 76

reviewbotreviewbot

E126 continuation line over-indented for hanging indent

reviewbotreviewbot

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

reviewbotreviewbot

F401 'django.http.HttpResponseNotFound' imported but unused

reviewbotreviewbot

F401 'django.http.HttpResponse' imported but unused

reviewbotreviewbot

F401 'django.http.HttpResponseBadRequest' imported but unused

reviewbotreviewbot

F401 'django.shortcuts.get_list_or_404' imported but unused

reviewbotreviewbot

E501 line too long (86 > 79 characters)

reviewbotreviewbot

E116 unexpected indentation (comment)

reviewbotreviewbot

W391 blank line at end of file

reviewbotreviewbot

F401 'django.db.models.Q' imported but unused

reviewbotreviewbot

F401 'django.shortcuts.get_object_or_404' imported but unused

reviewbotreviewbot

F401 'reviewboard.reviews.views_mixins.ReviewRequestViewMixin' imported but unused

reviewbotreviewbot

E126 continuation line over-indented for hanging indent

reviewbotreviewbot

F401 'django.http.HttpResponse' imported but unused

reviewbotreviewbot

F401 'django.http.Http404' imported but unused

reviewbotreviewbot

F401 'django.shortcuts.render_to_response' imported but unused

reviewbotreviewbot

E126 continuation line over-indented for hanging indent

reviewbotreviewbot

F401 'django.http.HttpResponse' imported but unused

reviewbotreviewbot

F401 'django.http.HttpResponseBadRequest' imported but unused

reviewbotreviewbot

F401 'django.http.HttpResponseNotFound' imported but unused

reviewbotreviewbot

F401 'django.shortcuts.get_list_or_404' imported but unused

reviewbotreviewbot

E116 unexpected indentation (comment)

reviewbotreviewbot

E501 line too long (86 > 79 characters)

reviewbotreviewbot

W391 blank line at end of file

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

flake8

nicolelisa
Review request changed

Commits:

Summary Author
-
set up base for review ui urls
nicolelisa
+
set up base for review ui urls
nicolelisa
+
add testing and fix patterns
nicolelisa

Diff:

Revision 2 (+290 -22)

Show changes

Checks run (1 failed, 1 succeeded)

flake8 failed.
JSHint passed.

flake8

nicolelisa
nicolelisa
Review request changed

Commits:

Summary Author
-
set up base for review ui urls
nicolelisa
-
add testing and fix patterns
nicolelisa
-
reviewbot changes
nicolelisa
+
add get_render_mode
nicolelisa
+
set up base for review ui urls
nicolelisa
+
add testing and fix patterns
nicolelisa
+
reviewbot changes
nicolelisa

Diff:

Revision 4 (+375 -39)

Show changes

Checks run (1 failed, 1 succeeded)

flake8 failed.
JSHint passed.

flake8

nicolelisa
Review request changed

Commits:

Summary Author
-
add get_render_mode
nicolelisa
-
set up base for review ui urls
nicolelisa
-
add testing and fix patterns
nicolelisa
-
reviewbot changes
nicolelisa
+
set up base for review ui urls
nicolelisa
+
add get_render_mode
nicolelisa
+
add testing and fix patterns
nicolelisa
+
reviewbot changes
nicolelisa

Diff:

Revision 5 (+563 -119)

Show changes

Checks run (1 failed, 1 succeeded)

flake8 failed.
JSHint passed.

flake8

brennie
  1. 
      
  2. reviewboard/reviews/ui/text.py (Diff revision 5)
     
     
     
     

    This shouldn't be staticmethod and should be the get method, ie

    def get(self, ...):
    
  3. reviewboard/reviews/ui/text.py (Diff revision 5)
     
     

    Once ReviewFileAttachmentViewMixin is not using staticmethods, this should just be self.get_attachments, etc.

  4. reviewboard/reviews/ui/text.py (Diff revision 5)
     
     

    Here too

  5. reviewboard/reviews/ui/text.py (Diff revision 5)
     
     

    This should be TextBasedReviewUIView.as_view()

  6. reviewboard/reviews/views.py (Diff revision 5)
     
     
     

    These should not be staticmethods; these should be regular methods instead.

  7. reviewboard/reviews/views.py (Diff revision 5)
     
     
     

    These should not be staticmethods; these should be regular methods instead.

  8. 
      
nicolelisa
Review request changed

Commits:

Summary Author
-
set up base for review ui urls
nicolelisa
-
add get_render_mode
nicolelisa
-
add testing and fix patterns
nicolelisa
-
reviewbot changes
nicolelisa
+
set up base for review ui urls
nicolelisa
+
add get_render_mode
nicolelisa
+
add testing and fix patterns
nicolelisa
+
reviewbot changes
nicolelisa

Diff:

Revision 6 (+605 -119)

Show changes

Checks run (1 failed, 1 succeeded)

flake8 failed.
JSHint passed.

flake8

amalik2
  1. 
      
  2. In addition to these new tests, you should also have tests that assert that unregistering the review UI has the expected behaviour.

    1. Hm. I had sworn i wrote this but I must have forgot. thanks for pointing this out!

  3. reviewboard/reviews/ui/base.py (Diff revision 6)
     
     

    This import statement should probably be moved to the top of this file for consistency.

    1. This was the workaround for the cyclic import statement discussed in the meeting. If you have any other ideas on how to fix this though, that would be really helpful.

  4. reviewboard/reviews/ui/base.py (Diff revision 6)
     
     

    Is there any way this can be done without modifying a global variable in another file? If there isn't, then for encapsulation purposes it may be better to have a method in the urls.py file that takes in a urlpatterns argument, and appends it to the dyanmic URL resolver. Something like this:

    def add_dynamic_review_ui_urls(urlpatterns):
    ....

  5. reviewboard/reviews/ui/base.py (Diff revision 6)
     
     

    This import statement should probably be moved to the top of this file for consistency.

  6. reviewboard/reviews/ui/text.py (Diff revision 6)
     
     

    You should write some tests to make sure that this method works as expected

    1. Structuring of this is still a WIP but that's on the todo list

  7. reviewboard/reviews/ui/text.py (Diff revision 6)
     
     

    Is kwargs meant to be used by individual review UIs in the future? I think all of the query parameter values are available in the request object, which already gets passed to the review UI

    1. Changing the structure of this part but will take a look at this when it's moved around.

  8. reviewboard/reviews/ui/text.py (Diff revision 6)
     
     

    Just curious about this, how can specific review UIs like the JSON or XML ones render text in different ways based on the query parameter values? Where should the code that accesses them actually be (i.e. would it be in get_render_lines? If so, wouldn't that function need to be modified to take in a request argument?)

    1. Yeah, we've been discussing this in the #students channel. Moving a bunch of stuff around to hopefully address this.

  9. 
      
nicolelisa
Review request changed

Commits:

Summary Author
-
set up base for review ui urls
nicolelisa
-
add get_render_mode
nicolelisa
-
add testing and fix patterns
nicolelisa
-
reviewbot changes
nicolelisa
+
set up base for review ui urls
nicolelisa
+
add get_render_mode
nicolelisa
+
add testing and fix patterns
nicolelisa
+
reviewbot changes
nicolelisa

Diff:

Revision 7 (+1241 -631)

Show changes

Checks run (1 failed, 1 succeeded)

flake8 failed.
JSHint passed.

flake8

nicolelisa
Review request changed

Commits:

Summary Author
-
set up base for review ui urls
nicolelisa
-
add get_render_mode
nicolelisa
-
add testing and fix patterns
nicolelisa
-
reviewbot changes
nicolelisa
+
set up base for review ui urls
nicolelisa
+
wip for url rendering with ajax started
nicolelisa
+
add testing and fix patterns
nicolelisa
+
reviewbot changes
nicolelisa

Diff:

Revision 8 (+1269 -649)

Show changes

Checks run (1 failed, 1 succeeded)

flake8 failed.
JSHint passed.

flake8

Loading...