WIP: Dynamic Review UI Rendering

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

nicolelisa
Review Board
master
10713
reviewboard, students

Overview:

Add in functionality to allow review UIs to provide a list of (relative) URLs they want to register, along with the methods that will be handling requests to those URLs.

Task specifications can be found here

This task has been separated into 4 subtasks:

1. Base(1): Review UI URLs - Adding the ability for a ReviewUI subclass to provide URLs they want to make available. Assigned to Nicole.
2. Text(1): Built-In Render URLs - Adding a standard _render/ URL for TextBasedReviewUI. This would ultimately take a type= argument specifying (source or rendered), which could be passed by the caller. Assigned to Nicole.
3. Text(2): Rendering Options Template Block - Update reviewboard/templates/reviews/ui/text.html template to provide a {% block "render_options" %} that subclasses of the template can use to add new UI. This would be blank by default, meaning it's entirely optional, and can be used only by the review UIs that need it. Assigned to Adil.
4. Text(3): Render Requests in JavaScript - Wrap RB.apiCall in a new method to provide Review UIs with the ability to make requests for custom rendering options and backend rendering implementations. Assigned to Adil

Adil's Testing for Tasks 3 and 4:

  1. Ran JavaScript unit tests.
  2. Tested manually to make sure that the element to update actually gets
    updated by using a mock endpoint.
  3. Tested manually to make sure comments are correctly reattached after
    a request is finished.
Summary Author
Add in Adil's 'Added function to reload render options with AJAX' - Review Request #10749
nicolelisa
Changed enpoint for ajax request to point to TextBasedReviewUIView url
nicolelisa
set up base for review ui urls
nicolelisa
Add dummy features for the Ajax functionality
nicolelisa
wip for url rendering with ajax started
nicolelisa
revert html changes to rendering
nicolelisa
Added in Review Request #10731 changes completed by Adil
nicolelisa
reviewbot changes
nicolelisa
reviewbot changes
nicolelisa
add testing and fix patterns
nicolelisa
reviewbot changes
nicolelisa
Renamed TextBasedReviewUIView to TextBasedReviewUITextView
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

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

E302 expected 2 blank lines, found 1

reviewbotreviewbot

E302 expected 2 blank lines, found 0

reviewbotreviewbot

E265 block comment should start with '# '

reviewbotreviewbot

This should be removed, I've removed it from my AJAX function review request now as well

amalik2amalik2

This should be removed, I've removed it from my AJAX function review request now as well

amalik2amalik2

This should be removed, I've removed it from my AJAX function review request now as well

amalik2amalik2

This should be removed, I've removed it from my AJAX function review request now as well

amalik2amalik2

All changes to this file should be reverted since they were just for testing purposes

amalik2amalik2

F401 'django.http.HttpResponse' imported but unused

reviewbotreviewbot

F401 'django.utils.safestring.mark_safe' imported but unused

reviewbotreviewbot

F401 'djblets.util.compat.django.template.loader.render_to_string' imported but unused

reviewbotreviewbot

This test code should be removed

amalik2amalik2

This test code should be removed

amalik2amalik2

This isn't too important but technically this should be an HTTP 400 error. 404 indicates the resource was not found, ...

amalik2amalik2

This will need to be slightly modified to handle diff chunks, since those aren't stored using context['rendered_lines'] or context['text_lines']

amalik2amalik2

In my XML review request I had to set context['lines'] to the result of the generate_render() function to bypass the ...

amalik2amalik2

This should be context['lines'] = context['text_lines']

amalik2amalik2

This test code should be removed

amalik2amalik2

This should either be implemented and used in some way or removed

amalik2amalik2

Maybe a different name for this key would be more suitable? It may not be that intuitive to others what ...

amalik2amalik2

This comment should be removed

amalik2amalik2

This comment should be removed

amalik2amalik2

This isn't important for this review request but in my AJAX function review request, there was a check for whether ...

amalik2amalik2

This test code should be removed

amalik2amalik2

This test code should be removed

amalik2amalik2

This test code should be removed

amalik2amalik2

This test code should be removed

amalik2amalik2

The docstring should be modified to include the 'renderType' parameter since you are using it

amalik2amalik2

This should be removed

amalik2amalik2

This test code should be removed

amalik2amalik2

This test code should be removed

amalik2amalik2

This test code should be removed

amalik2amalik2

This test code should be removed

amalik2amalik2

This test code should be removed

amalik2amalik2

This test code should be removed

amalik2amalik2

This test code should be removed

amalik2amalik2

This test code should be removed

amalik2amalik2

This test code should be removed

amalik2amalik2

This test code should be removed

amalik2amalik2

This test code should be removed

amalik2amalik2

This test code should be removed

amalik2amalik2
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

nicolelisa
Review request changed

Commits:

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
+
set up base for review ui urls
nicolelisa
+
wip for url rendering with ajax started
nicolelisa
+
revert html changes to rendering
nicolelisa
+
add testing and fix patterns
nicolelisa
+
reviewbot changes
nicolelisa

Diff:

Revision 9 (+1239 -679)

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
-
wip for url rendering with ajax started
nicolelisa
-
reviewbot changes
nicolelisa
-
revert html changes to rendering
nicolelisa
-
add testing and fix patterns
nicolelisa
-
reviewbot changes
nicolelisa
+
Add in Adil's 'Added function to reload render options with AJAX' - Review Request #10749
nicolelisa
+
set up base for review ui urls
nicolelisa
+
wip for url rendering with ajax started
nicolelisa
+
revert html changes to rendering
nicolelisa
+
reviewbot changes
nicolelisa
+
add testing and fix patterns
nicolelisa
+
reviewbot changes
nicolelisa

Diff:

Revision 11 (+1708 -706)

Show changes

Checks run (1 failed, 1 succeeded)

flake8 failed.
JSHint passed.

flake8

nicolelisa
nicolelisa
nicolelisa
Review request changed

Commits:

Summary Author
-
Add in Adil's 'Added function to reload render options with AJAX' - Review Request #10749
nicolelisa
-
set up base for review ui urls
nicolelisa
-
wip for url rendering with ajax started
nicolelisa
-
revert html changes to rendering
nicolelisa
-
reviewbot changes
nicolelisa
-
add testing and fix patterns
nicolelisa
-
reviewbot changes
nicolelisa
+
Add in Adil's 'Added function to reload render options with AJAX' - Review Request #10749
nicolelisa
+
Changed enpoint for ajax request to point to TextBasedReviewUIView url
nicolelisa
+
set up base for review ui urls
nicolelisa
+
Add dummy features for the Ajax functionality
nicolelisa
+
wip for url rendering with ajax started
nicolelisa
+
revert html changes to rendering
nicolelisa
+
Added in Review Request #10731 changes completed by Adil
nicolelisa
+
reviewbot changes
nicolelisa
+
reviewbot changes
nicolelisa
+
add testing and fix patterns
nicolelisa
+
reviewbot changes
nicolelisa
+
Renamed TextBasedReviewUIView to TextBasedReviewUITextView
nicolelisa

Diff:

Revision 12 (+2092 -764)

Show changes

Checks run (1 failed, 1 succeeded)

flake8 failed.
JSHint passed.

flake8

amalik2
  1. 
      
  2. reviewboard/admin/urls.py (Diff revision 12)
     
     

    This should be removed, I've removed it from my AJAX function review request now as well

  3. reviewboard/admin/urls.py (Diff revision 12)
     
     

    This should be removed, I've removed it from my AJAX function review request now as well

  4. reviewboard/admin/urls.py (Diff revision 12)
     
     

    This should be removed, I've removed it from my AJAX function review request now as well

  5. reviewboard/admin/urls.py (Diff revision 12)
     
     

    This should be removed, I've removed it from my AJAX function review request now as well

  6. reviewboard/reviews/ui/markdownui.py (Diff revision 12)
     
     

    All changes to this file should be reverted since they were just for testing purposes

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

    This test code should be removed

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

    This test code should be removed

  9. reviewboard/reviews/ui/text.py (Diff revision 12)
     
     

    This isn't too important but technically this should be an HTTP 400 error. 404 indicates the resource was not found, while 400 indicates that the client sent a bad request (in this case, an invalid render_type value counts as a bad request)

  10. reviewboard/reviews/ui/text.py (Diff revision 12)
     
     

    This will need to be slightly modified to handle diff chunks, since those aren't stored using context['rendered_lines'] or context['text_lines']

  11. reviewboard/reviews/ui/text.py (Diff revision 12)
     
     

    In my XML review request I had to set context['lines'] to the result of the generate_render() function to bypass the caching that occurs in get_rendered_lines(), I think you'll have to do that here as well

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

    This should be context['lines'] = context['text_lines']

  13. reviewboard/reviews/ui/text.py (Diff revision 12)
     
     

    This test code should be removed

  14. reviewboard/reviews/ui/text.py (Diff revision 12)
     
     

    This should either be implemented and used in some way or removed

  15. reviewboard/reviews/ui/text.py (Diff revision 12)
     
     

    Maybe a different name for this key would be more suitable? It may not be that intuitive to others what 'reviewUIId' actually means. 'reviewUiType' perhaps?

  16. reviewboard/reviews/views_mixins.py (Diff revision 12)
     
     

    This comment should be removed

  17. reviewboard/reviews/views_mixins.py (Diff revision 12)
     
     

    This comment should be removed

  18. This isn't important for this review request but in my AJAX function review request, there was a check for whether a comment should be rendered or not here. Since that will likely be useful for your JSON review UI if you rebase on top of this review request, you should remember to add that here in your JSON code.

  19. This test code should be removed

  20. This test code should be removed

  21. This test code should be removed

  22. This test code should be removed

  23. The docstring should be modified to include the 'renderType' parameter since you are using it

  24. This should be removed

  25. This test code should be removed

  26. This test code should be removed

  27. This test code should be removed

  28. This test code should be removed

  29. This test code should be removed

  30. This test code should be removed

  31. This test code should be removed

  32. This test code should be removed

  33. This test code should be removed

  34. This test code should be removed

  35. This test code should be removed

  36. This test code should be removed

  37. 
      
Loading...