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

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

skaefer143
Review Board
master
10214
11714f4...
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.

Created relatedUserSelectorTests.es6.js,
relatedRepoSelectorTests.es6.js, and
relatedGroupSelectorTests.es6.js to test the widgets
javascript functionality.

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.

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

Loading file attachments...

Description From Last Updated

Instead of "See patch #10214" you can do: See /r/10214/. and it will be linked correctly. (e.g. /r/10214/)

brenniebrennie

You also need to migrate the template.

brenniebrennie

These tests does not actually test rendering at all, even though they . If it is testing rendering, I would ...

brenniebrennie

Can you show before/after screenshots of each usage?

chipx86chipx86

In your description it says: Moved RelatedObjectWidget into Djblets. This patch doesn't actually do that (since it doesn't affect Djblets.) ...

brenniebrennie

This doesn't need to live on the prototype. It can live as a global (const searchPlaceholderText = ...) since you're ...

brenniebrennie

Col: 37 Missing semicolon.

reviewbotreviewbot

Col: 41 Expected '===' and instead saw '=='.

reviewbotreviewbot

I was wondering why you removed the init method. do you instantiate the local_site_name and multivalued variables else where? or ...

praiseApraiseA

Col: 42 Missing semicolon.

reviewbotreviewbot

Col: 3 Missing semicolon.

reviewbotreviewbot

Initalize->Initialize Typo.

cdkushnicdkushni

Did this work for you? I found that when running tests, pdb would interrupt control flow but I wouldn't be ...

cdkushnicdkushni

These should be in the same import group.

chipx86chipx86

These should be in alphabetical order.

chipx86chipx86

who's already in the list I think this was left over from copy & paste, as it should say: which ...

brenniebrennie

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

chipx86chipx86

This can easily be: repo_data = [ { 'id': repo.pk, 'name': repo.name, } for repo in self.existing_repos ]

chipx86chipx86

It'd be better to have the template take the HTML for the input element and have it take care of ...

chipx86chipx86

Can you instead do: The list of IDs of :py:class:`~reviewboard.scmtools.models.Repository` objects. Wrapping appropriately. (You can wrap at a . inside ...

chipx86chipx86

"invite-only"

chipx86chipx86

Same comment as above.

chipx86chipx86

These should be in alphabetical order.

chipx86chipx86

There should be one class per widget we are testing.

brenniebrennie

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

chipx86chipx86

These aren't necessary

brenniebrennie

These aren't necessary.

brenniebrennie

No period, as unit tests are printed as: TEST_DOCSTRING ... OK Same for all other tests.

brenniebrennie

Single quotes throughout the file.

brenniebrennie

A better way to wrap would be: user_widget = \ response.context['adminform'].form.fields['user'].widget.widget Same with tests below.

chipx86chipx86

This can be one request, like: response = self.client.post( '.....', { ... }) Same with tests below.

chipx86chipx86

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

brenniebrennie

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

brenniebrennie

Same here re: string types and primary keys.

brenniebrennie

Single quotes.

brenniebrennie

Same comment here about string types. test_repo.pk is not correct. This should be WebHookTarget.objects.latest().pk

brenniebrennie

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 ...

brenniebrennie

These should be in alphabetical order.

chipx86chipx86

These should be in alphabetical order.

chipx86chipx86

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

brenniebrennie

Can you make sure that the label is still appropriate?

chipx86chipx86

Two blank lines between these.

chipx86chipx86

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

brenniebrennie

Two blank lines between these.

chipx86chipx86

The string needs to be wrapped with gettext(...). Does this need to be here? Why isn't it just being set ...

chipx86chipx86

This should be called inviteOnly in JavaScript.

chipx86chipx86

This should be this._inviteOnly.

brenniebrennie

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

brenniebrennie

const if the variable is not being rebound, let otherwise. Also, this should be localInviteOnly. JS uses lowerCamelCase.

brenniebrennie

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

brenniebrennie

this mapping function is happening twice, so we should pull it out into e.g. this._resultToItem or similar. However, if we ...

brenniebrennie

Here too.

brenniebrennie

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

brenniebrennie

"Local Site"

chipx86chipx86

Typo: "multuple" -> "multiple"

chipx86chipx86

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

chipx86chipx86

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

brenniebrennie

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

brenniebrennie

This is technically an API breakage. Any extensions pulling in widgets will now fail. I assume this is due to ...

chipx86chipx86

djblets-widgets should be first.

chipx86chipx86

This can just be <script> these days.

chipx86chipx86

As above, this should be inviteOnly.

chipx86chipx86

This can just be <script> these days.

chipx86chipx86

I think these should be in aphabetical order

bolariinwabolariinwa

F401 'reviewboard.admin.form_widgets.RelatedUserWidget' imported but unused

reviewbotreviewbot

F401 'reviewboard.admin.form_widgets.RelatedRepositoryWidget' imported but unused

reviewbotreviewbot

F401 'reviewboard.admin.form_widgets.RelatedGroupWidget' imported but unused

reviewbotreviewbot

Blank line between these.

brenniebrennie

attrs (dict, optional):

brenniebrennie

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

brenniebrennie

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

brenniebrennie

attrs (dict, optional):

brenniebrennie

Missing a docstring, which would look something like: Unit tests for RelatedUserWidget.

brenniebrennie

On previous line. Same below.

brenniebrennie

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

brenniebrennie

E501 line too long (104 > 79 characters)

reviewbotreviewbot

E501 line too long (104 > 79 characters)

reviewbotreviewbot

E501 line too long (105 > 79 characters)

reviewbotreviewbot

Could you re-flow this as: value = ( my_form.fields['...'] .widget .value_from_datadict( {'people': ['1', '2']}, {}, 'people') ) Same below.

brenniebrennie

Missing class-level docstring.

brenniebrennie

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

brenniebrennie

We generally prefer ) not on its own line.

brenniebrennie

Missing class-level docstring.

brenniebrennie

Missing class level docstring.

brenniebrennie

Missing module-level docstring, e.g. Unit tests for reviewboard.notifications.models This file should be called test_admin.

brenniebrennie

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

brenniebrennie

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

brenniebrennie

This docstring is wrong. It should be: Tests for reviewboard.oauth.admin.

brenniebrennie

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 ...

brenniebrennie

Docstrings should be of the form: """Single line description Multi-line summary. """ In this case, the docstring should be updated ...

brenniebrennie

{% load %} should be the first line of the file, e.g. {% load djblets_js i18n %} {{input_html}} <script>

brenniebrennie

{% load %} should be the first line of the file, e.g. {% load djblets_js i18n %} {{input_html}} <script>

brenniebrennie

{% load %} should be the first line of the file, e.g. {% load djblets_js i18n %} {{input_html}} <script>

brenniebrennie

E501 line too long (120 > 79 characters)

reviewbotreviewbot

E501 line too long (120 > 79 characters)

reviewbotreviewbot

E501 line too long (121 > 79 characters)

reviewbotreviewbot

E501 line too long (85 > 79 characters)

reviewbotreviewbot

E501 line too long (120 > 79 characters)

reviewbotreviewbot

E501 line too long (120 > 79 characters)

reviewbotreviewbot

E501 line too long (121 > 79 characters)

reviewbotreviewbot

E501 line too long (120 > 79 characters)

reviewbotreviewbot

E501 line too long (120 > 79 characters)

reviewbotreviewbot

E501 line too long (121 > 79 characters)

reviewbotreviewbot

Col: 17 'view' is defined but never used.

reviewbotreviewbot

Why are we now saving this into self ?

brenniebrennie

Why are we saving this into self?

brenniebrennie

dict

brenniebrennie

E501 line too long (120 > 79 characters)

reviewbotreviewbot

E501 line too long (120 > 79 characters)

reviewbotreviewbot

E501 line too long (121 > 79 characters)

reviewbotreviewbot

""" on next line

brenniebrennie

""" on next line.

brenniebrennie

""" on next line.

brenniebrennie

This test doesn't do anything.

brenniebrennie

Col: 17 'view' is defined but never used.

reviewbotreviewbot

Leftover debug code?

brenniebrennie

A widget to select related *groups* using search and autocomplete.

shovenshoven

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

shovenshoven

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

shovenshoven

E501 line too long (120 > 79 characters)

reviewbotreviewbot

E501 line too long (120 > 79 characters)

reviewbotreviewbot

E501 line too long (121 > 79 characters)

reviewbotreviewbot

Col: 17 'view' is defined but never used.

reviewbotreviewbot

E501 line too long (120 > 79 characters)

reviewbotreviewbot

E501 line too long (120 > 79 characters)

reviewbotreviewbot

E501 line too long (121 > 79 characters)

reviewbotreviewbot

I was wondering why you declared this template outside of the view. Couldn't you declare this inside of the RB.RelatedGroupSelectorView? ...

SudoliciousSudolicious

E501 line too long (120 > 79 characters)

reviewbotreviewbot

E501 line too long (120 > 79 characters)

reviewbotreviewbot

E501 line too long (121 > 79 characters)

reviewbotreviewbot

Col: 20 Unexpected ')'.

reviewbotreviewbot

Col: 20 Expected an identifier and instead saw ')'.

reviewbotreviewbot

Col: 21 Expected ')' and instead saw ';'.

reviewbotreviewbot

Col: 22 Missing semicolon.

reviewbotreviewbot

E501 line too long (120 > 79 characters)

reviewbotreviewbot

E501 line too long (120 > 79 characters)

reviewbotreviewbot

E501 line too long (121 > 79 characters)

reviewbotreviewbot

E501 line too long (120 > 79 characters)

reviewbotreviewbot

E501 line too long (120 > 79 characters)

reviewbotreviewbot

E501 line too long (121 > 79 characters)

reviewbotreviewbot

E501 line too long (120 > 79 characters)

reviewbotreviewbot

E501 line too long (120 > 79 characters)

reviewbotreviewbot

E501 line too long (121 > 79 characters)

reviewbotreviewbot
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

skaefer143
Review request changed

Change Summary:

Add Select Item from Dropdown and Rendering with Inital Options JS test, and rework other relatedUserSelector JS tests

Commit:

-f3c4f3a9444ed40db1ba4deb6f08bc3301cf3b51
+c29a20c8a8137ee6f6e2791ba10f94f2ce124d6b

Diff:

Revision 22 (+1046 -387)

Show changes

Checks run (1 failed, 1 succeeded)

flake8 failed.
JSHint passed.

flake8

Sudolicious
  1. Hey Storm, this is looking really good!

  2. I was wondering why you declared this template outside of the view. Couldn't you declare this inside of the RB.RelatedGroupSelectorView?
    So instead maybe it could be something like this?

    RB.RelatedGroupSelectorView = Djblets.RelatedObjectSelectorView.extend({
    searchPlaceholderText: gettext('Search for groups...'),
    
    optionTemplate = _.template(dedent`
        <div>
        <span class="title"><%- name %> : <%-    display_name %></span>
        </div>
    `);
    
    1. This was actually previous code that I simply modified. You do bring up a good point though, I definitely could put it inside. I'll try it out.

    2. Actually, the reason it's not in the view definition is because we're simply using it as a constant for the renderOption() method. The view.extend() method takes a dictionary argument, where I define and extend callbacks from relatedObjectWidget. So I can't add a optionTemplate = 'anything'; line, because I'm definining a dictionary. It's easier to leave it as is, I think.

  3. 
      
skaefer143
Review request changed

Change Summary:

update a selectize readme, fix a css bug, and add group and repo widget JS tests

Commit:

-c29a20c8a8137ee6f6e2791ba10f94f2ce124d6b
+10a837367fb91686915e8f564b2c9c92be4907f7

Diff:

Revision 23 (+1246 -390)

Show changes

Checks run (2 failed)

flake8 failed.
JSHint failed.

flake8

JSHint

skaefer143
Review request changed

Checks run (1 failed, 1 succeeded)

flake8 failed.
JSHint passed.

flake8

skaefer143
Review request changed

Change Summary:

forgot to define the repo and group JS tests in static bundles, also renamed repo widget test.

Commit:

-7766cd5e06745a871d81f3b7d79b289b0b94234d
+6bd8b826530bbf0d8ce3ece3e56541f44050bb32

Diff:

Revision 25 (+1246 -390)

Show changes

Checks run (1 failed, 1 succeeded)

flake8 failed.
JSHint passed.

flake8

skaefer143
Review request changed

Change Summary:

move selectize.js out of reviewboard, into djblets.

Testing Done:

~  

JS Tests are currently incomplete.

  ~

Created relatedUserSelectorTests.es6.js,

  + relatedRepoSelectorTests.es6.js, and
  + relatedGroupSelectorTests.es6.js to test the widgets
  + javascript functionality.

   
   

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

  ~

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:

-6bd8b826530bbf0d8ce3ece3e56541f44050bb32
+11714f443c8b2834f3dc3f0e9d570808d9c9f09f

Diff:

Revision 26 (+1243 -4448)

Show changes

Checks run (1 failed, 1 succeeded)

flake8 failed.
JSHint passed.

flake8

david
  1. Going to make some small compat updates and land this. Thanks!

  2. 
      
skaefer143
Review request changed

Status: Closed (submitted)

Change Summary:

Pushed to release-4.0.x (7c82cb5)
Loading...