Instead of "See patch #10214" you can do:

See /r/10214/.


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

You also need to migrate the template.

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.

Initalize->Initialize Typo.

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.

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

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

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

There should be one class per widget we are testing.

These aren't necessary

These aren't necessary.

No period, as unit tests are printed as:

TEST_DOCSTRING ... OK


Same for all other tests.

Single quotes throughout the file.

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)

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

Same here re: string types and primary keys.

Single quotes.

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

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

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

Hello!

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

This should be this._inviteOnly.

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

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

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

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

Here too.

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

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

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

Can you show before/after screenshots of each usage?

These should be in the same import group.

These should be in alphabetical order.

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

This can easily be:

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

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.

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

"invite-only"

Same comment as above.

These should be in alphabetical order.

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

A better way to wrap would be:

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


Same with tests below.

This can be one request, like:

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


Same with tests below.

These should be in alphabetical order.

These should be in alphabetical order.

Can you make sure that the label is still appropriate?

Two blank lines between these.

Two blank lines between these.

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

This should be called inviteOnly in JavaScript.

"Local Site"

Typo: "multuple" -> "multiple"

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

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.

djblets-widgets should be first.

This can just be <script> these days.

As above, this should be inviteOnly.

This can just be <script> these days.

I think these should be in aphabetical order

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.

Blank line between these.

attrs (dict, optional):

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

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

attrs (dict, optional):

Missing a docstring, which would look something like:



Unit tests for RelatedUserWidget.

On previous line. Same below.

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.

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

Could you re-flow this as:

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


Same below.

Missing class-level docstring.

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

We generally prefer ) not on its own line.

Missing class-level docstring.

Missing class level docstring.

Missing module-level docstring, e.g.


Unit tests for reviewboard.notifications.models

This file should be called test_admin.

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

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

This docstring is wrong. It should be:


Tests for reviewboard.oauth.admin.

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

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.

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

{% load djblets_js i18n %}

{{input_html}}

<script>

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

{% load djblets_js i18n %}

{{input_html}}

<script>

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

{% load djblets_js i18n %}

{{input_html}}

<script>