WIP: Dynamic Review UI Rendering
Review Request #10758 — Created Oct. 20, 2019 and updated
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 forTextBasedReviewUI. This would ultimately take atype=argument specifying (sourceorrendered), which could be passed by the caller. Assigned to Nicole.
3. Text(2): Rendering Options Template Block - Updatereviewboard/templates/reviews/ui/text.htmltemplate 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 - WrapRB.apiCallin 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:
- Ran JavaScript unit tests.
- Tested manually to make sure that the element to update actually gets
updated by using a mock endpoint.- Tested manually to make sure comments are correctly reattached after
a request is finished.
| Summary | ID | Author |
|---|---|---|
| 8321ecb908f20b49eb2268770c82695e3369ddb0 | nicolelisa | |
| db70ed57fcd4ad6e53ed9d7d60050b3fd55334c9 | nicolelisa | |
| e11c287f2f0ee9d18dbb15b0d8b7a92adb78b025 | nicolelisa | |
| 9b5766e66ce9613b4aae5a0c7c3466bdb5edba0d | nicolelisa | |
| ac92785154c3b11ffca422967cc093a3196da6e8 | nicolelisa | |
| bbac6f795332fbebc83b8392ecc3213b86f58ab2 | nicolelisa | |
| 179716bc369fae4f98b62ebd30ef2f858e87f974 | nicolelisa | |
| cfebbba9ebc36d2fad99b5d2f88844892a0ec284 | nicolelisa | |
| 20b38c6b129876ce9714e3a13672182c88fa8866 | nicolelisa | |
| fad657c963f5bdf9b98f38eaa802fa033591c203 | nicolelisa | |
| b2bef6b73d110507856bf81618e48a846ccc79ec | nicolelisa | |
| 8db350412c01e143b1d21654493f7667b80ea218 | nicolelisa |
| Description | From | Last Updated |
|---|---|---|
|
E402 module level import not at top of file |
 reviewbot
|
|
|
E501 line too long (82 > 79 characters) |
reviewbot
|
|
|
F401 'django.conf.urls.include' imported but unused |
reviewbot
|
|
|
E123 closing bracket does not match indentation of opening bracket's line |
reviewbot
|
|
|
E123 closing bracket does not match indentation of opening bracket's line |
reviewbot
|
|
|
E123 closing bracket does not match indentation of opening bracket's line |
reviewbot
|
|
|
F401 'django.core.urlresolvers.reverse_lazy' imported but unused |
reviewbot
|
|
|
F401 'django.core.urlresolvers.reverse' imported but unused |
reviewbot
|
|
|
F401 'django.http.Http404' imported but unused |
reviewbot
|
|
|
F401 'django.core.urlresolvers.resolve' imported but unused |
reviewbot
|
|
|
F401 'django.db.models.Q' imported but unused |
reviewbot
|
|
|
F401 'django.http.Http404' imported but unused |
reviewbot
|
|
|
F401 'django.http.HttpResponseForbidden' imported but unused |
reviewbot
|
|
|
F401 'django.core.urlresolvers.resolve' imported but unused |
reviewbot
|
|
|
F401 'django.db.models.Q' imported but unused |
reviewbot
|
|
|
F401 'django.shortcuts.get_object_or_404' imported but unused |
reviewbot
|
|
|
F401 'reviewboard.reviews.views.ReviewFileAttachmentView' imported but unused |
reviewbot
|
|
|
This shouldn't be staticmethod and should be the get method, ie def get(self, ...): |
 brennie
|
|
|
Once ReviewFileAttachmentViewMixin is not using staticmethods, this should just be self.get_attachments, etc. |
brennie
|
|
|
Here too |
brennie
|
|
|
F841 local variable 'review_ui' is assigned to but never used |
reviewbot
|
|
|
W293 blank line contains whitespace |
reviewbot
|
|
|
This should be TextBasedReviewUIView.as_view() |
brennie
|
|
|
These should not be staticmethods; these should be regular methods instead. |
brennie
|
|
|
These should not be staticmethods; these should be regular methods instead. |
brennie
|
|
|
F841 local variable 'review_ui' is assigned to but never used |
reviewbot
|
|
|
E303 too many blank lines (2) |
reviewbot
|
|
|
In addition to these new tests, you should also have tests that assert that unregistering the review UI has the … |
 amalik2
|
|
|
This import statement should probably be moved to the top of this file for consistency. |
amalik2
|
|
|
Is there any way this can be done without modifying a global variable in another file? If there isn't, then … |
amalik2
|
|
|
This import statement should probably be moved to the top of this file for consistency. |
amalik2
|
|
|
F401 'django.http.HttpResponse' imported but unused |
reviewbot
|
|
|
F401 'djblets.util.decorators.augment_method_from' imported but unused |
reviewbot
|
|
|
F401 'djblets.webapi.resources.WebAPIResource' imported but unused |
reviewbot
|
|
|
F401 'reviewboard.reviews.models.ReviewRequest' imported but unused |
reviewbot
|
|
|
F401 'reviewboard.reviews.views.ReviewFileAttachmentView' imported but unused |
reviewbot
|
|
|
F401 'django.views.generic.base.ContextMixin' imported but unused |
reviewbot
|
|
|
F401 'django.views.generic.base.TemplateView' imported but unused |
reviewbot
|
|
|
F401 'django.views.generic.base.RedirectView' imported but unused |
reviewbot
|
|
|
F401 'reviewboard.webapi.decorators.webapi_check_local_site' imported but unused |
reviewbot
|
|
|
You should write some tests to make sure that this method works as expected |
amalik2
|
|
|
E501 line too long (81 > 79 characters) |
reviewbot
|
|
|
E126 continuation line over-indented for hanging indent |
reviewbot
|
|
|
Is kwargs meant to be used by individual review UIs in the future? I think all of the query parameter … |
amalik2
|
|
|
Just curious about this, how can specific review UIs like the JSON or XML ones render text in different ways … |
amalik2
|
|
|
E501 line too long (84 > 79 characters) |
reviewbot
|
|
|
F841 local variable 'review_ui' is assigned to but never used |
reviewbot
|
|
|
E303 too many blank lines (2) |
reviewbot
|
|
|
F401 'django.db.models.Q' imported but unused |
reviewbot
|
|
|
F401 'django.shortcuts.get_object_or_404' imported but unused |
reviewbot
|
|
|
F401 'reviewboard.reviews.views_mixins.ReviewRequestViewMixin' imported but unused |
reviewbot
|
|
|
E126 continuation line over-indented for hanging indent |
reviewbot
|
|
|
F401 'django.http.Http404' imported but unused |
reviewbot
|
|
|
F401 'django.shortcuts.render_to_response' imported but unused |
reviewbot
|
|
|
F401 'reviewboard.reviews.views.ReviewFileAttachmentView' imported but unused |
reviewbot
|
|
|
F401 'django.views.generic.base.View' imported but unused |
reviewbot
|
|
|
F811 redefinition of unused 'ReviewFileAttachmentViewMixin' from line 76 |
reviewbot
|
|
|
E126 continuation line over-indented for hanging indent |
reviewbot
|
|
|
F841 local variable 'is_enabled_for' is assigned to but never used |
reviewbot
|
|
|
F401 'django.http.HttpResponseNotFound' imported but unused |
reviewbot
|
|
|
F401 'django.http.HttpResponse' imported but unused |
reviewbot
|
|
|
F401 'django.http.HttpResponseBadRequest' imported but unused |
reviewbot
|
|
|
F401 'django.shortcuts.get_list_or_404' imported but unused |
reviewbot
|
|
|
E501 line too long (86 > 79 characters) |
reviewbot
|
|
|
E116 unexpected indentation (comment) |
reviewbot
|
|
|
W391 blank line at end of file |
reviewbot
|
|
|
F401 'django.db.models.Q' imported but unused |
reviewbot
|
|
|
F401 'django.shortcuts.get_object_or_404' imported but unused |
reviewbot
|
|
|
F401 'reviewboard.reviews.views_mixins.ReviewRequestViewMixin' imported but unused |
reviewbot
|
|
|
E126 continuation line over-indented for hanging indent |
reviewbot
|
|
|
F401 'django.http.HttpResponse' imported but unused |
reviewbot
|
|
|
F401 'django.http.Http404' imported but unused |
reviewbot
|
|
|
F401 'django.shortcuts.render_to_response' imported but unused |
reviewbot
|
|
|
E126 continuation line over-indented for hanging indent |
reviewbot
|
|
|
F401 'django.http.HttpResponse' imported but unused |
reviewbot
|
|
|
F401 'django.http.HttpResponseBadRequest' imported but unused |
reviewbot
|
|
|
F401 'django.http.HttpResponseNotFound' imported but unused |
reviewbot
|
|
|
F401 'django.shortcuts.get_list_or_404' imported but unused |
reviewbot
|
|
|
E116 unexpected indentation (comment) |
reviewbot
|
|
|
E501 line too long (86 > 79 characters) |
reviewbot
|
|
|
W391 blank line at end of file |
reviewbot
|
|
|
F401 'django.db.models.Q' imported but unused |
reviewbot
|
|
|
F401 'django.shortcuts.get_object_or_404' imported but unused |
reviewbot
|
|
|
F401 'reviewboard.reviews.views_mixins.ReviewRequestViewMixin' imported but unused |
reviewbot
|
|
|
E126 continuation line over-indented for hanging indent |
reviewbot
|
|
|
F401 'django.http.HttpResponse' imported but unused |
reviewbot
|
|
|
F401 'django.http.Http404' imported but unused |
reviewbot
|
|
|
F401 'django.shortcuts.render_to_response' imported but unused |
reviewbot
|
|
|
E126 continuation line over-indented for hanging indent |
reviewbot
|
|
|
F401 'django.http.HttpResponse' imported but unused |
reviewbot
|
|
|
F401 'django.http.HttpResponseBadRequest' imported but unused |
reviewbot
|
|
|
F401 'django.http.HttpResponseNotFound' imported but unused |
reviewbot
|
|
|
F401 'django.shortcuts.get_list_or_404' imported but unused |
reviewbot
|
|
|
E116 unexpected indentation (comment) |
reviewbot
|
|
|
E501 line too long (86 > 79 characters) |
reviewbot
|
|
|
W391 blank line at end of file |
reviewbot
|
|
|
E302 expected 2 blank lines, found 1 |
reviewbot
|
|
|
E302 expected 2 blank lines, found 0 |
reviewbot
|
|
|
E265 block comment should start with '# ' |
reviewbot
|
|
|
This should be removed, I've removed it from my AJAX function review request now as well |
amalik2
|
|
|
This should be removed, I've removed it from my AJAX function review request now as well |
amalik2
|
|
|
This should be removed, I've removed it from my AJAX function review request now as well |
amalik2
|
|
|
This should be removed, I've removed it from my AJAX function review request now as well |
amalik2
|
|
|
All changes to this file should be reverted since they were just for testing purposes |
amalik2
|
|
|
F401 'django.http.HttpResponse' imported but unused |
reviewbot
|
|
|
F401 'django.utils.safestring.mark_safe' imported but unused |
reviewbot
|
|
|
F401 'djblets.util.compat.django.template.loader.render_to_string' imported but unused |
reviewbot
|
|
|
This test code should be removed |
amalik2
|
|
|
This test code should be removed |
amalik2
|
|
|
This isn't too important but technically this should be an HTTP 400 error. 404 indicates the resource was not found, … |
amalik2
|
|
|
This will need to be slightly modified to handle diff chunks, since those aren't stored using context['rendered_lines'] or context['text_lines'] |
amalik2
|
|
|
In my XML review request I had to set context['lines'] to the result of the generate_render() function to bypass the … |
amalik2
|
|
|
This should be context['lines'] = context['text_lines'] |
amalik2
|
|
|
This test code should be removed |
amalik2
|
|
|
This should either be implemented and used in some way or removed |
amalik2
|
|
|
Maybe a different name for this key would be more suitable? It may not be that intuitive to others what … |
amalik2
|
|
|
This comment should be removed |
amalik2
|
|
|
This comment should be removed |
amalik2
|
|
|
This isn't important for this review request but in my AJAX function review request, there was a check for whether … |
amalik2
|
|
|
This test code should be removed |
amalik2
|
|
|
This test code should be removed |
amalik2
|
|
|
This test code should be removed |
amalik2
|
|
|
This test code should be removed |
amalik2
|
|
|
The docstring should be modified to include the 'renderType' parameter since you are using it |
amalik2
|
|
|
This should be removed |
amalik2
|
|
|
This test code should be removed |
amalik2
|
|
|
This test code should be removed |
amalik2
|
|
|
This test code should be removed |
amalik2
|
|
|
This test code should be removed |
amalik2
|
|
|
This test code should be removed |
amalik2
|
|
|
This test code should be removed |
amalik2
|
|
|
This test code should be removed |
amalik2
|
|
|
This test code should be removed |
amalik2
|
|
|
This test code should be removed |
amalik2
|
|
|
This test code should be removed |
amalik2
|
|
|
This test code should be removed |
amalik2
|
|
|
This test code should be removed |
amalik2
|
- Commits:
-
Summary ID Author 8321ecb908f20b49eb2268770c82695e3369ddb0 nicolelisa 8321ecb908f20b49eb2268770c82695e3369ddb0 nicolelisa db70ed57fcd4ad6e53ed9d7d60050b3fd55334c9 nicolelisa
Checks run (1 failed, 1 succeeded)
flake8
- Commits:
-
Summary ID Author 8321ecb908f20b49eb2268770c82695e3369ddb0 nicolelisa db70ed57fcd4ad6e53ed9d7d60050b3fd55334c9 nicolelisa 8321ecb908f20b49eb2268770c82695e3369ddb0 nicolelisa db70ed57fcd4ad6e53ed9d7d60050b3fd55334c9 nicolelisa e11c287f2f0ee9d18dbb15b0d8b7a92adb78b025 nicolelisa
Checks run (2 succeeded)
- Commits:
-
Summary ID Author 8321ecb908f20b49eb2268770c82695e3369ddb0 nicolelisa db70ed57fcd4ad6e53ed9d7d60050b3fd55334c9 nicolelisa e11c287f2f0ee9d18dbb15b0d8b7a92adb78b025 nicolelisa 8321ecb908f20b49eb2268770c82695e3369ddb0 nicolelisa db70ed57fcd4ad6e53ed9d7d60050b3fd55334c9 nicolelisa e11c287f2f0ee9d18dbb15b0d8b7a92adb78b025 nicolelisa 3f69a1f00c9303084d1d89fe47bc622472df4c71 nicolelisa
- Commits:
-
Summary ID Author 8321ecb908f20b49eb2268770c82695e3369ddb0 nicolelisa db70ed57fcd4ad6e53ed9d7d60050b3fd55334c9 nicolelisa e11c287f2f0ee9d18dbb15b0d8b7a92adb78b025 nicolelisa 3f69a1f00c9303084d1d89fe47bc622472df4c71 nicolelisa 8321ecb908f20b49eb2268770c82695e3369ddb0 nicolelisa db70ed57fcd4ad6e53ed9d7d60050b3fd55334c9 nicolelisa e11c287f2f0ee9d18dbb15b0d8b7a92adb78b025 nicolelisa 9c9d767c43a41e701165bb4506f2b3a11eec5b49 nicolelisa
Checks run (1 failed, 1 succeeded)
flake8
- Commits:
-
Summary ID Author 8321ecb908f20b49eb2268770c82695e3369ddb0 nicolelisa db70ed57fcd4ad6e53ed9d7d60050b3fd55334c9 nicolelisa e11c287f2f0ee9d18dbb15b0d8b7a92adb78b025 nicolelisa 9c9d767c43a41e701165bb4506f2b3a11eec5b49 nicolelisa 8321ecb908f20b49eb2268770c82695e3369ddb0 nicolelisa db70ed57fcd4ad6e53ed9d7d60050b3fd55334c9 nicolelisa e11c287f2f0ee9d18dbb15b0d8b7a92adb78b025 nicolelisa d71396a9f3efab7725275c2172429aef3e48778b nicolelisa
Checks run (1 failed, 1 succeeded)
flake8
-
-
In addition to these new tests, you should also have tests that assert that unregistering the review UI has the expected behaviour.
-
-
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):
.... -
-
-
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
-
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?)
- Commits:
-
Summary ID Author 8321ecb908f20b49eb2268770c82695e3369ddb0 nicolelisa db70ed57fcd4ad6e53ed9d7d60050b3fd55334c9 nicolelisa e11c287f2f0ee9d18dbb15b0d8b7a92adb78b025 nicolelisa d71396a9f3efab7725275c2172429aef3e48778b nicolelisa 8321ecb908f20b49eb2268770c82695e3369ddb0 nicolelisa db70ed57fcd4ad6e53ed9d7d60050b3fd55334c9 nicolelisa e11c287f2f0ee9d18dbb15b0d8b7a92adb78b025 nicolelisa d87233dcbfd3bc2acf76d71c38c5afa54f20af0c nicolelisa
Checks run (1 failed, 1 succeeded)
flake8
- Commits:
-
Summary ID Author 8321ecb908f20b49eb2268770c82695e3369ddb0 nicolelisa db70ed57fcd4ad6e53ed9d7d60050b3fd55334c9 nicolelisa e11c287f2f0ee9d18dbb15b0d8b7a92adb78b025 nicolelisa d87233dcbfd3bc2acf76d71c38c5afa54f20af0c nicolelisa 8321ecb908f20b49eb2268770c82695e3369ddb0 nicolelisa db70ed57fcd4ad6e53ed9d7d60050b3fd55334c9 nicolelisa e11c287f2f0ee9d18dbb15b0d8b7a92adb78b025 nicolelisa 9b5766e66ce9613b4aae5a0c7c3466bdb5edba0d nicolelisa - Diff:
-
Revision 8 (+1269 -649)
Checks run (1 failed, 1 succeeded)
flake8
- Commits:
-
Summary ID Author 8321ecb908f20b49eb2268770c82695e3369ddb0 nicolelisa db70ed57fcd4ad6e53ed9d7d60050b3fd55334c9 nicolelisa e11c287f2f0ee9d18dbb15b0d8b7a92adb78b025 nicolelisa 9b5766e66ce9613b4aae5a0c7c3466bdb5edba0d nicolelisa 8321ecb908f20b49eb2268770c82695e3369ddb0 nicolelisa db70ed57fcd4ad6e53ed9d7d60050b3fd55334c9 nicolelisa e11c287f2f0ee9d18dbb15b0d8b7a92adb78b025 nicolelisa 9b5766e66ce9613b4aae5a0c7c3466bdb5edba0d nicolelisa ac92785154c3b11ffca422967cc093a3196da6e8 nicolelisa
Checks run (1 failed, 1 succeeded)
flake8
- Commits:
-
Summary ID Author 8321ecb908f20b49eb2268770c82695e3369ddb0 nicolelisa db70ed57fcd4ad6e53ed9d7d60050b3fd55334c9 nicolelisa e11c287f2f0ee9d18dbb15b0d8b7a92adb78b025 nicolelisa 9b5766e66ce9613b4aae5a0c7c3466bdb5edba0d nicolelisa ac92785154c3b11ffca422967cc093a3196da6e8 nicolelisa 8321ecb908f20b49eb2268770c82695e3369ddb0 nicolelisa db70ed57fcd4ad6e53ed9d7d60050b3fd55334c9 nicolelisa e11c287f2f0ee9d18dbb15b0d8b7a92adb78b025 nicolelisa 9b5766e66ce9613b4aae5a0c7c3466bdb5edba0d nicolelisa ac92785154c3b11ffca422967cc093a3196da6e8 nicolelisa ac31a894d15d2097bfad47231f80cd6226536581 nicolelisa
Checks run (2 succeeded)
- Commits:
-
Summary ID Author 8321ecb908f20b49eb2268770c82695e3369ddb0 nicolelisa db70ed57fcd4ad6e53ed9d7d60050b3fd55334c9 nicolelisa e11c287f2f0ee9d18dbb15b0d8b7a92adb78b025 nicolelisa 9b5766e66ce9613b4aae5a0c7c3466bdb5edba0d nicolelisa ac92785154c3b11ffca422967cc093a3196da6e8 nicolelisa ac31a894d15d2097bfad47231f80cd6226536581 nicolelisa 8321ecb908f20b49eb2268770c82695e3369ddb0 nicolelisa db70ed57fcd4ad6e53ed9d7d60050b3fd55334c9 nicolelisa e11c287f2f0ee9d18dbb15b0d8b7a92adb78b025 nicolelisa 9b5766e66ce9613b4aae5a0c7c3466bdb5edba0d nicolelisa ac92785154c3b11ffca422967cc093a3196da6e8 nicolelisa bbac6f795332fbebc83b8392ecc3213b86f58ab2 nicolelisa 179716bc369fae4f98b62ebd30ef2f858e87f974 nicolelisa - Diff:
-
Revision 11 (+1708 -706)
- Summary:
-
WIP: Base: Review UI URLsWIP: Dynamic Review UI Rendering
- Description:
-
~ set up base for review ui urls
~ 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 forTextBasedReviewUI. This would ultimately take atype=argument specifying (sourceorrendered), which could be passed by the caller. Assigned to Nicole.+ 3. Text(2): Rendering Options Template Block - Update reviewboard/templates/reviews/ui/text.htmltemplate 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.apiCallin a new method to provide Review UIs with the ability to make requests for custom rendering options and backend rendering implementations. Assigned to Adil - Testing Done:
-
+ 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.
- Description:
-
+ 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. ~ 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 forTextBasedReviewUI. This would ultimately take atype=argument specifying (sourceorrendered), which could be passed by the caller. Assigned to Nicole.3. Text(2): Rendering Options Template Block - Update reviewboard/templates/reviews/ui/text.htmltemplate 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.apiCallin a new method to provide Review UIs with the ability to make requests for custom rendering options and backend rendering implementations. Assigned to Adil - Testing Done:
-
~ 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 ~ Adil's Testing for Tasks 3 and 4:
~ ~ - Ran JavaScript unit tests.
~ - Tested manually to make sure that the element to update actually gets
updated by using a mock endpoint.
~ - Tested manually to make sure comments are correctly reattached after
a request is finished.
- a request is finished.
- Commits:
-
Summary ID Author 8321ecb908f20b49eb2268770c82695e3369ddb0 nicolelisa db70ed57fcd4ad6e53ed9d7d60050b3fd55334c9 nicolelisa e11c287f2f0ee9d18dbb15b0d8b7a92adb78b025 nicolelisa 9b5766e66ce9613b4aae5a0c7c3466bdb5edba0d nicolelisa ac92785154c3b11ffca422967cc093a3196da6e8 nicolelisa bbac6f795332fbebc83b8392ecc3213b86f58ab2 nicolelisa 179716bc369fae4f98b62ebd30ef2f858e87f974 nicolelisa 8321ecb908f20b49eb2268770c82695e3369ddb0 nicolelisa db70ed57fcd4ad6e53ed9d7d60050b3fd55334c9 nicolelisa e11c287f2f0ee9d18dbb15b0d8b7a92adb78b025 nicolelisa 9b5766e66ce9613b4aae5a0c7c3466bdb5edba0d nicolelisa ac92785154c3b11ffca422967cc093a3196da6e8 nicolelisa bbac6f795332fbebc83b8392ecc3213b86f58ab2 nicolelisa 179716bc369fae4f98b62ebd30ef2f858e87f974 nicolelisa cfebbba9ebc36d2fad99b5d2f88844892a0ec284 nicolelisa 20b38c6b129876ce9714e3a13672182c88fa8866 nicolelisa fad657c963f5bdf9b98f38eaa802fa033591c203 nicolelisa b2bef6b73d110507856bf81618e48a846ccc79ec nicolelisa 8db350412c01e143b1d21654493f7667b80ea218 nicolelisa - Diff:
-
Revision 12 (+2092 -764)
-
-
-
-
-
-
-
-
-
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)
-
This will need to be slightly modified to handle diff chunks, since those aren't stored using context['rendered_lines'] or context['text_lines']
-
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
-
-
-
-
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?
-
-
-
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.
-
-
-
-
-
-
-
-
-
-
-
-
-
-
-
-
-
-