Replace old selectors with smart objects selector widget in the administration UI

Review Request #10215 — Created Oct. 10, 2018 and updated

skaefer143
Review Board
master
10214
f3c4f3a...
reviewboard, students

Now that RelatedObjectedWidget is in Djblets, we import
it from there and extend it in the RelatedUserWidget.

Created RelatedRepositoryWidget and RelatedGroupWidget, along with
relevant HTML and JS files, which extends the RelatedObjectsWidget.
RelatedGroupWidget has the ability to only display groups that are
invite only. All three widgets, for exampe, can be viewed on
/admin/db/reviews/defaultreviewer/add/, and they all function.

See /r/10214.

JS Tests are currently incomplete.

Ran ./tests/runtests.py and no errors were thrown. Ran the js-tests, and
no errors were thrown.

New unit tests were created in reviewboard.admin.tests that test
the functionality of the widgets. The test classes are:
RelatedUserWidgetTestCase, RelatedRepositoryWidgetTestCase and,
RelatedGroupWidgetTestCase.

Other tests in reviewboard.notifications.tests.test_admin,
reviewboard.reviews.tests.test_admin, and
reviewboard.oauth.tests test that forms which utilize these new widgets
can properly save data.

Navigated to /admin/db/reviews/defaultreviewer/add/ and the
RelatedUserWidget, RelatedRepositoryWidget and
RelatedGroupWidget function as intended.

Loading file attachments...

  • 0
  • 0
  • 106
  • 17
  • 123
Description From Last Updated
brennie
  1. 
      
  2. Instead of "See patch #10214" you can do:

    See /r/10214/.
    

    and it will be linked correctly. (e.g. /r/10214/)

  3. You also need to migrate the template.

  4. 
      
skaefer143
skaefer143
skaefer143
skaefer143
brennie
  1. 
      
  2. This doesn't need to live on the prototype. It can live as a global (const searchPlaceholderText = ...) since you're wrapping everything in an IIFE.

  3. 
      
skaefer143
Review request changed

Change Summary:

The RelatedRepositoryWidget functions and renders responses now. It is also extending Djblets.RelatedObjectSelectorView now, instead of using RB..

Commit:

-d5e82c3e4862c66cbb0f0510996466c03b931848
+64a92a43ad1e5ee9b9799e0ddbd1f3c3fd3b2036

Diff:

Revision 6 (+235 -370)

Show changes

Checks run (1 failed, 1 succeeded)

flake8 passed.
JSHint failed.

JSHint

skaefer143
skaefer143
Review request changed

Change Summary:

Add the relatedGroupWidget, and replace the default Group selector widget. It uses /api/groups/ to search for groups.

Description:

   

[Work in Progress]

   
~  

Moved RelatedObjectWidget into Djblets. The widget is imported from Djblets

  ~

Moved RelatedObjectWidget into Djblets. The widget is imported from Djblets

    now, and extended by RelatedUserWidget.

   
~  

Created RelatedRepositoryWidget, along with related HTML and JS files, which

~   extends the RelatedObjectsWidget. Currently displaying on
~   /admin/db/reviews/defaultreviewer/add/, but doesn't function.

  ~

Created RelatedRepositoryWidget and RelatedGroupWidget, along with

  ~ relevant HTML and JS files, which extends the RelatedObjectsWidget.
  ~ RelatedGroupWidget has the ability to only display groups that are
  + invite only. All three widgets are displaying on
  + /admin/db/reviews/defaultreviewer/add/, and they all function.

   
   

See /r/10214.

Testing Done:

~  

Ran ./tests/runtests.py and no errors were thrown.

  ~

Ran ./tests/runtests.py and no errors were thrown. Ran the js-tests, and

  + no errors were thrown.

   
   

Navigated to /admin/db/reviews/defaultreviewer/add/ and the Smart

~   Select User Widget works as normal. The RelatedRepositoryWidget
~   doesn't currently function.

  ~ Select User Widget works as normal. Both the RelatedRepositoryWidget
  ~ and the RelatedGroupWidget also function as intended.

Commit:

-c383d3c2a24f1172a5f28b72419239fabf9ecfd1
+a519a7a8f71fcac0c694ad8042ae18c9e6259e1e

Diff:

Revision 8 (+499 -373)

Show changes

Checks run (1 failed, 1 succeeded)

flake8 passed.
JSHint failed.

JSHint

skaefer143
skaefer143
Review request changed

Change Summary:

Adding skeleton code for python unit and JS tests.

Commit:

-80dc7cb7f71f1edd9d50f90e48b14e64dd13665a
+34186bab1c2ff4dc94ec8f36e6e68076b16e7080

Diff:

Revision 10 (+554 -373)

Show changes

Checks run (1 failed, 1 succeeded)

flake8 passed.
JSHint failed.

JSHint

skaefer143
cdkushni
  1. 
      
  2. reviewboard/admin/form_widgets.py (Diff revision 11)
     
     

    Initalize->Initialize Typo.

  3. reviewboard/admin/tests.py (Diff revision 11)
     
     

    Did this work for you? I found that when running tests, pdb would interrupt control flow but I wouldn't be able to step through code or do anything other than continue the test.

  4. 
      
skaefer143
skaefer143
praiseA
  1. 
      
  2. reviewboard/admin/form_widgets.py (Diff revision 9)
     
     

    I was wondering why you removed the init method. do you instantiate the local_site_name and multivalued variables else where? or you don't need them anymore as well

    1. I actually move this code into a class RelatedObjectWidget, and I extend upon that class. RelatedObjectWidget is in my Django patch, and must be used for this patch to work.

  3. 
      
brennie
  1. 
      
  2. These tests does not actually test rendering at all, even though they . If it is testing rendering, I would expect it to test the generated HTML.

    However, I'm not sure how valuable it is to test the rendering of a form since that is likely to change often and be quite fragile.

    So I think we need to just change the names of the tests.

    Perhaps instead of testing specific instances of where they are used, we should just make a test form for each testcase and use that instead:

    e.g.

    class RelatedUserWidgetTestCase(TestCase):
        class TestForm(forms.Form):
            my_multiselect_field = # ...
    
        def test_#...
    
    
    class RelatedRepositoryWidgetTestCase(TestCase):
        class TestForm(forms.Form):
            my_multiselect_field = # ...
    
       def test_#...
    

    In this way the test cases are isolated to specifically the widget and do not have to deal with any details of specific form cases.

    That being said, the test you have written that test the OAuth application form does the correct redirect is still valuable! This is a good test case and I think we should keep it as a separate test case (in a e.g., ApplicationAdminTests in reviewboard/oauth/tests/tests.py -- or at least I think that is where it should go. Maybe ask chipx86 or david to double check?).

    1. Agreed on all fronts. We want a test suite per class being tested. It's also worth noting that the first test is testing too many things. It's testing:

      1. That the widget template is used during a request on the OAuth2 page.
      2. That an OAuth2 application can be added.
      3. That fetching an OAuth2 application entry uses the template.

      The second test repeats these with a different page and a different widget. And then a third.

      The problem is, these are not really testing the widgets. They're testing the specific forms.

      Those sorts of tests can be valuable for those forms, if the general actions aren't already covered (though, they'd need to test more of the results of those objects), but these tests should be different. You should follow the testing pattern for the relevant functions in djblets/forms/tests/test_conditions_widget.py. This covers:

      • value_from_datadict (for each possible condition within)
      • render (checking substrings, like assertIn('multivalued: true', html))

      These should be done with an independent instance of a page, not dependent on any existing form. That way, those forms don't impact these tests (important since this widget is not dependent on the forms, and their tests should not break if those forms break). You can construct an instance of a field as a form would, and then call methods on it.

    2. Oh okay, thanks for the review! I'll get on that.

      So, should I modify the test I wrote, and keep it as tests that test redirects for the forms? Or tests that test that the template is used in the form? Or no, and just keep it to tests that test the functionality of the widget?

      Basically, is there anything worth keeping here?

    3. What I would do is:

      1. Remove from the tests the isinstance(..., RelatedWhateverWidget) stuff.
      2. Rename test_user_widget_render and put it into a new testcase in reviewboard.oauth.tests e.g. OAuthFormsTests
      3. Rename test_repo_widget_Render and put it into a new testcase in reviewboard.notification.tests.test_webhook_target_admin.
      4. Rename test_group_widget_render and put it into a new testcase in reviewboard.reviews.tests.test_default_reviewer_admin.
  3. reviewboard/admin/form_widgets.py (Diff revision 13)
     
     

    who's already in the list

    I think this was left over from copy & paste, as it should say:

    which repositories are already in the list

  4. reviewboard/admin/tests.py (Diff revision 13)
     
     

    There should be one class per widget we are testing.

  5. reviewboard/admin/tests.py (Diff revision 13)
     
     

    These aren't necessary

  6. reviewboard/admin/tests.py (Diff revision 13)
     
     

    These aren't necessary.

  7. reviewboard/admin/tests.py (Diff revision 13)
     
     

    No period, as unit tests are printed as:

    TEST_DOCSTRING ... OK
    

    Same for all other tests.

  8. reviewboard/admin/tests.py (Diff revision 13)
     
     

    Single quotes throughout the file.

  9. reviewboard/admin/tests.py (Diff revision 13)
     
     
     

    You definitely don't want to use str here. str means two differen things depending on your version of Python.

    On Python 2, str is the bytes type, e.g., it represents raw binary data.

    On Python 3, str represents a text type (what was unicode in Python 2).

    However, you can also just do the following and not have to worry about character types:

    self.assertRedirects(response,
                         '/admib/db/oauth/application/%s/'
                         % test_user.pk)
    
  10. reviewboard/admin/tests.py (Diff revision 13)
     
     

    This only works by accident. YOu should be retrieving the latest Application and using its pk here instead, e.g.

    application = Application.objects.latest()
    
    # use application.pk
    
  11. reviewboard/admin/tests.py (Diff revision 13)
     
     
     

    Same here re: string types and primary keys.

  12. reviewboard/admin/tests.py (Diff revision 13)
     
     

    Single quotes.

  13. reviewboard/admin/tests.py (Diff revision 13)
     
     
     

    Same comment here about string types.

    test_repo.pk is not correct. This should be WebHookTarget.objects.latest().pk

  14. reviewboard/admin/tests.py (Diff revision 13)
     
     

    Again, this should use % formatting and this only works as a fluke that test_group.pk == DefaultReviewer.objects.latest().pk. (e.g., they're both 1).

  15. reviewboard/scmtools/forms.py (Diff revision 13)
     
     

    can you use a keyword argument for whatever True is? (Is it required=?).

  16. This should only be indented one space relative to the line above.

  17. This should be this._inviteOnly.

  18. This can just do return optionTemplate(item) since the template shouldn't be modifying the item.

  19. const if the variable is not being rebound, let otherwise.

    Also, this should be localInviteOnly. JS uses lowerCamelCase.

  20. This doesn't bother me, but another mentor is going to tell you to do succes: results => { ... } instead of using the shorthand.

  21. this mapping function is happening twice, so we should pull it out into e.g.

    this._resultToItem or similar. However, if we do that, then we definitely have to use the success: results => { ... } syntax becuase otherwise this will be lost (as the foo() {...} shorthand does not bind the function, but () => {} does).

    1. I understand what you mean with using the arrow functions and why, but I'm not sure why we'd need this._resultToItem. Would something like this be better?

      if (this._inviteOnly === true) {
          results.groups = results.groups.filter(obj => {
              return obj.invite_only;
          });
      }
      callback(results.groups.map(u => ({
          name: u.name,
          display_name: u.display_name,
          id: u.id,
          invite_only: u.invite_only
      })));
      
    2. Looks a-ok to me.

      One note is that for the filter you can do: results.groups.filter(obj => obj.invite_only)

      which is shorthand for what you have.

    3. Ok thanks! I'm going to leave it as is, because otherwise I go over the 79 char limit.

  22. This should only be indented one space relative to the line above.

  23. Not strictly necessary, but I imagine another mentor will tell you not to use shorthand object method syntax here.

    1. Correct. This will result in the wrong context in the function. We want the function to have the correct this, so use => ... instead.

  24. Not strictly necessary, but I imagine another mentor will tell you not to use shorthand object method syntax here.

  25. 
      
chipx86
  1. 
      
  2. Can you show before/after screenshots of each usage?

    1. Would you like this in the Description of this patch? Before and after for each widget?

  3. reviewboard/admin/form_widgets.py (Diff revision 13)
     
     
     
     

    These should be in the same import group.

  4. reviewboard/admin/form_widgets.py (Diff revision 13)
     
     
     

    These should be in alphabetical order.

  5. reviewboard/admin/form_widgets.py (Diff revision 13)
     
     

    No nee for the or None part. None is a value.

  6. reviewboard/admin/form_widgets.py (Diff revision 13)
     
     
     
     
     
     
     
     
     
     

    This can easily be:

    repo_data = [
        {
            'id': repo.pk,
            'name': repo.name,
        }
        for repo in self.existing_repos
    ]
    
  7. reviewboard/admin/form_widgets.py (Diff revision 13)
     
     
     
     
     
     
     
     
     

    It'd be better to have the template take the HTML for the input element and have it take care of positioning, rather than concatenating.

    1. What do you mean by this? I copied this format from the render method in RelatedUserWidget.

    2. I believe he is saying that the related_repo_widget.html template should accept a context var for input_html and splat it into the template itself. e.g.

      {# related_widget.html #}
      {{input_html}
      
      ... rest of template
      

      return render_to_string('admin/related_repo_widget.html', {
          'input_html': mark_safe(input_html),
          'input_id': final_attrs['id'],
          'multivalued': self.multivalued,
          'repos': repo_data,
      })
      
  8. reviewboard/admin/form_widgets.py (Diff revision 13)
     
     

    Can you instead do:

    The list of IDs of :py:class:`~reviewboard.scmtools.models.Repository` objects.
    

    Wrapping appropriately. (You can wrap at a . inside the reference, if needed.)

  9. reviewboard/admin/form_widgets.py (Diff revision 13)
     
     

    "invite-only"

  10. reviewboard/admin/form_widgets.py (Diff revision 13)
     
     

    Same comment as above.

  11. reviewboard/admin/tests.py (Diff revision 13)
     
     
     
     

    These should be in alphabetical order.

  12. reviewboard/admin/tests.py (Diff revision 13)
     
     
     
     
     
     
     
     
     

    Since these aren't doing anything special, you can remove them.

  13. reviewboard/admin/tests.py (Diff revision 13)
     
     
     

    A better way to wrap would be:

    user_widget = \
        response.context['adminform'].form.fields['user'].widget.widget
    

    Same with tests below.

  14. reviewboard/admin/tests.py (Diff revision 13)
     
     
     
     
     
     
     
     
     
     
     
     

    This can be one request, like:

    response = self.client.post(
        '.....',
        {
            ...
        })
    

    Same with tests below.

  15. reviewboard/reviews/forms.py (Diff revision 13)
     
     
     
     

    These should be in alphabetical order.

  16. reviewboard/scmtools/forms.py (Diff revision 13)
     
     
     

    These should be in alphabetical order.

  17. reviewboard/scmtools/forms.py (Diff revision 13)
     
     
     

    Can you make sure that the label is still appropriate?

    1. Image

      This is a screenshot of this code. It still says "Review groups with access". Is that alright?

  18. Two blank lines between these.

  19. Two blank lines between these.

  20. The string needs to be wrapped with gettext(...).

    Does this need to be here? Why isn't it just being set on the object?

  21. This should be called inviteOnly in JavaScript.

  22. Typo: "multuple" -> "multiple"

  23. No need for the _.extend, since there isn't a dictionary to extend. You should be able to just pass item.

  24. reviewboard/staticbundles.py (Diff revision 13)
     
     

    This is technically an API breakage. Any extensions pulling in widgets will now fail. I assume this is due to the addition of the widget in Djblets, and if so, then we should just rename that one.

  25. reviewboard/templates/admin/base_site.html (Diff revision 13)
     
     
     
     

    djblets-widgets should be first.

  26. This can just be <script> these days.

  27. As above, this should be inviteOnly.

  28. This can just be <script> these days.

  29. 
      
skaefer143
skaefer143
bolariinwa
  1. 
      
  2. reviewboard/admin/tests.py (Diff revision 14)
     
     
     
     

    I think these should be in aphabetical order

  3. 
      
skaefer143
Review request changed

Change Summary:

Changed widget tests into form tests, and moved them to proper locations. Fixed a bunch of code style issues.

Commit:

-71783bdf6f38682d5332da0c7047503ac9f5e59b
+b0072ee1df6272dbbce01dc8f91355c4e2c76793

Diff:

Revision 15 (+662 -383)

Show changes

Checks run (1 failed, 1 succeeded)

flake8 failed.
JSHint passed.

flake8

skaefer143
Review request changed

Change Summary:

updated widget unit tests for RelatedUserWidget, RelatedRepositoryWidget and RelatedGroupWidget, based on mentor feedback.

Commit:

-b0072ee1df6272dbbce01dc8f91355c4e2c76793
+9536023a78ca53ac3dae0e6e6dacccb68b151df0

Diff:

Revision 16 (+993 -383)

Show changes

Checks run (1 failed, 1 succeeded)

flake8 failed.
JSHint passed.

flake8

brennie
  1. 
      
  2. In your description it says:

    Moved RelatedObjectWidget into Djblets.

    This patch doesn't actually do that (since it doesn't affect Djblets.) Instead it should say something like:

    Now that RelatedObjectedWidget is in Djblets, we import
    it from there and extend it in the RelatedUserWidget.

  3. reviewboard/admin/form_widgets.py (Diff revision 16)
     
     
     

    Blank line between these.

  4. reviewboard/admin/form_widgets.py (Diff revision 16)
     
     

    attrs (dict, optional):

  5. reviewboard/admin/form_widgets.py (Diff revision 16)
     
     
     

    We are overwriting this just below so this can be removed.

  6. reviewboard/admin/form_widgets.py (Diff revision 16)
     
     

    Missing *args (tuple) and **kwargs (dict).

  7. reviewboard/admin/form_widgets.py (Diff revision 16)
     
     

    attrs (dict, optional):

  8. reviewboard/admin/tests.py (Diff revision 16)
     
     

    Missing a docstring, which would look something like:

    Unit tests for RelatedUserWidget.

  9. reviewboard/admin/tests.py (Diff revision 16)
     
     

    On previous line. Same below.

  10. reviewboard/admin/tests.py (Diff revision 16)
     
     
     
     
     
     
     

    You may want to use self.assertHTMLEqual(a, b) which parses a and b and thencompares the resulting trees for equality, ignoring whitespace and attribute ordering.

    Same goes for all the other tests doing assertIn with HTML.

    1. I just tried that, and tried mixing up some values (like changing the ordering of useAvatars: true and multivalued: true), and self.assertHTMLEqual() raised an assertion. I think it's because it looks at everything between the <script> ... </script> tags to see if any character is different. If somebody were to change the ordering of useAvatars and multivalued, or even added anything new, the test would fail.

      Is that worth the trouble?

      I think what I'll do instead is change my code so that the self.assertIn()'s don't look at commas and such. It will simply check that the value is as expected.

    2. Eh, actaully I'll just use self.assertHTMLEqual(). If anything in the HTML template changes, it really should be changed in the test too.

  11. reviewboard/admin/tests.py (Diff revision 16)
     
     
     
     

    You do not actually need the parentheses here (or below).

  12. reviewboard/admin/tests.py (Diff revision 16)
     
     
     
     
     
     
     
     
     

    Could you re-flow this as:

    value = (
        my_form.fields['...']
        .widget
        .value_from_datadict(
            {'people': ['1', '2']},
            {},
            'people')
    )
    

    Same below.

  13. reviewboard/admin/tests.py (Diff revision 16)
     
     

    Missing class-level docstring.

  14. reviewboard/admin/tests.py (Diff revision 16)
     
     
     
     

    This can go on a single line as {'id': 'repositories'}

  15. reviewboard/admin/tests.py (Diff revision 16)
     
     

    We generally prefer ) not on its own line.

  16. reviewboard/admin/tests.py (Diff revision 16)
     
     

    Missing class-level docstring.

  17. reviewboard/admin/tests.py (Diff revision 16)
     
     

    Missing class level docstring.

  18. Missing module-level docstring, e.g.

    Unit tests for reviewboard.notifications.models

    This file should be called test_admin.

  19. This should be WebhookTargetAdminTests, since it is testing the admin view explicitly.

  20. reviewboard/oauth/tests.py (Diff revision 16)
     
     

    This should be OAuthAdminTests since it is explicitly testing the admin view.

  21. reviewboard/oauth/tests.py (Diff revision 16)
     
     

    This docstring is wrong. It should be:

    Tests for reviewboard.oauth.admin.

  22. Missing module level docstring:

    Unit tests for reviewboard.reviews.admin.

    Also, this file should be renamed to test_admin.py so other reviews.admin functionality can be tested here in the future (as the testcases are quite short).

  23. Docstrings should be of the form:

    """Single line description
    
    Multi-line summary.
    """
    

    In this case, the docstring should be updated to:

    Tests for reviewboard.reviews.admin.DefaultReviewerAdmin.

  24. reviewboard/templates/admin/related_group_widget.html (Diff revision 16)
     
     
     
     
     
     

    {% load %} should be the first line of the file, e.g.

    {% load djblets_js i18n %}
    
    {{input_html}}
    
    <script>
    
  25. reviewboard/templates/admin/related_repo_widget.html (Diff revision 16)
     
     
     
     
     
     

    {% load %} should be the first line of the file, e.g.

    {% load djblets_js i18n %}
    
    {{input_html}}
    
    <script>
    
  26. reviewboard/templates/admin/related_user_widget.html (Diff revision 16)
     
     
     
     
     
     

    {% load %} should be the first line of the file, e.g.

    {% load djblets_js i18n %}
    
    {{input_html}}
    
    <script>
    
  27. 
      
skaefer143
Review request changed

Change Summary:

fixed code style issues, as well as added documentation for tests, based on mentor feedback.

Description:

   

[Work in Progress]

   
~  

Moved RelatedObjectWidget into Djblets. The widget is imported from Djblets

~   now, and extended by RelatedUserWidget.

  ~

Now that RelatedObjectedWidget is in Djblets, we import

  ~ it from there and extend it in the RelatedUserWidget.

   
   

Created RelatedRepositoryWidget and RelatedGroupWidget, along with

    relevant HTML and JS files, which extends the RelatedObjectsWidget.
    RelatedGroupWidget has the ability to only display groups that are
    invite only. All three widgets, for exampe, can be viewed on
    /admin/db/reviews/defaultreviewer/add/, and they all function.

   
   

See /r/10214.

Testing Done:

   

Ran ./tests/runtests.py and no errors were thrown. Ran the js-tests, and

    no errors were thrown.

   
  +

New unit tests were created in reviewboard.admin.tests that test

  + the functionality of the widgets. The test classes are:
  + RelatedUserWidgetTestCase, RelatedRepositoryWidgetTestCase and,
  + RelatedGroupWidgetTestCase.

  +
  +

Other tests in reviewboard.notifications.tests.test_admin,

  + reviewboard.reviews.tests.test_admin, and
  + reviewboard.oauth.tests test that forms which utilize these new widgets
  + can properly save data.

  +
   

Navigated to /admin/db/reviews/defaultreviewer/add/ and the

    RelatedUserWidget, RelatedRepositoryWidget and
    RelatedGroupWidget function as intended.

Commit:

-9536023a78ca53ac3dae0e6e6dacccb68b151df0
+1a2b0d63b7e62d7b522df8cef7d05d540cf79c75

Diff:

Revision 17 (+1074 -384)

Show changes

Checks run (1 failed, 1 succeeded)

flake8 failed.
JSHint passed.

flake8

skaefer143
Review request changed

Checks run (1 failed, 1 succeeded)

flake8 failed.
JSHint passed.

flake8

skaefer143
Review request changed

Change Summary:

Fixed some issues with the admin form tests. Also managed to figure out how to import the correct JS static bundle to use in the JS tests for the new widgets. Cleaned up some javascript bundle import code that wasn't needed anymore, as well as corrected some names.

Commit:

-a7a6f14d11a6caf5e3cc93853b33ad687eacb3ae
+7e36799068d54cae577fd091a3a5097e4ccf2820

Diff:

Revision 19 (+1084 -389)

Show changes

Checks run (2 failed)

flake8 failed.
JSHint failed.

flake8

JSHint

skaefer143
skaefer143
Review request changed

Change Summary:

change form tests to use reverse() instead of hardcoding urls.

Commit:

-7e36799068d54cae577fd091a3a5097e4ccf2820
+b2faf32cfc0ad8a412d06966992cfbb017293d6d

Diff:

Revision 20 (+1098 -389)

Show changes

Checks run (2 failed)

flake8 failed.
JSHint failed.

flake8

JSHint

brennie
  1. 
      
  2. reviewboard/admin/form_widgets.py (Diff revision 20)
     
     

    Why are we now saving this into self ?

    1. Ooo good catch. At first I was saving this into the class, so it was easier to grab the existing objects, but there's no need for that now. I'll change it back.

    2. Actually, I was using the self.existing_whatever in tests. I was using it to easily compare if a group was already selected in a widget. For instance, line 45 in reviewboard/reviews/tests/test_admin.py. Should I assert that a group is already selected in a widget in a different way?

    3. Now that these form tests are actually in /r/10339, this is irrelevant.

  3. reviewboard/admin/form_widgets.py (Diff revision 20)
     
     

    Why are we saving this into self?

  4. reviewboard/admin/form_widgets.py (Diff revision 20)
     
     
  5. """ on next line

  6. reviewboard/oauth/tests.py (Diff revision 20)
     
     

    """ on next line.

  7. reviewboard/reviews/tests/test_admin.py (Diff revision 20)
     
     

    """ on next line.

  8. This test doesn't do anything.

    1. I'll update my patch description, these JS test are still being worked on.

  9. 
      
skaefer143
shoven
  1. 
      
  2. A widget to select related *groups* using search and autocomplete.

  3. Do we need this console.log still? And if we do, since this is an error perhaps console.error(...) would be better?

    1. Thank you, I'll change it to console.error(...)!

  4. Do we need this console.log still? And if we do, since this is an error perhaps console.error(...) would be better?

    1. Thank you, I'll change it to console.error(...)!

  5. 
      
skaefer143
Review request changed

Change Summary:

Address Barret and Sarah's feedback. Got rid of self in "self.existing_object" calls, there was no need for it. Fixed some minor code comments.

Commit:

-b2faf32cfc0ad8a412d06966992cfbb017293d6d
+f3c4f3a9444ed40db1ba4deb6f08bc3301cf3b51

Diff:

Revision 21 (+960 -387)

Show changes

Checks run (2 failed)

flake8 failed.
JSHint failed.

flake8

JSHint

Loading...