Add auto-complete functionality to the Depends On field

Review Request #8434 — Created Sept. 24, 2016 and submitted

Information

Review Board
release-2.5.x
3aee79d...

Reviewers

For the ReviewRequestEditorView page:
Goal: To add autocomplete functionality to the Depends On field.

This was accomplished by:
Implementing basic autocomplete to the depends on field.
Implementing autocomplete to use the search api.
Properly determining correct data from search results.
Formatting and Displaying correct data to the correct field.

  • Verified results using existing autocomplete tests
  • Added additional tests to verify dependson results
    • Verify existence of autocomplete
    • Verify autocomplete results
  • Addionally tested by hand:
    • Id and Summary tested for Depends On

Description From Last Updated

One comment I've seen repeated is that our request description should kind've "tell a story" with respect to the project. …

RS rswanson

Can you rewrite your summary in the imperitive mood? i.e, "Add auto-complete functionality to the Depends On field"

brenniebrennie

Can you add screenshots?

brenniebrennie

Looking at the code now, I think we can make a major simplification. Right now, we have two iterations over …

daviddavid

This is a good time to ask, if we can leave comments in a request. I think they should be …

LA larmiej

this can use the operator "===" for extra security

LA larmiej

if changing the operator to "===" is okay with you

LA larmiej

Fix indentation.

brenniebrennie

Can you remove the spacing between the parentheses?

brenniebrennie

Undo this change.

brenniebrennie

This can be a one line comment. I'm not sure if its necessary, though.

brenniebrennie

Undo this.

brenniebrennie

Is this intentional?

brenniebrennie

space after colon

brenniebrennie

Undo this.

brenniebrennie

Undo this.

brenniebrennie

Undo this.

brenniebrennie

Undo this.

brenniebrennie

Remove this line.

brenniebrennie

Undo this.

brenniebrennie

Undo this.

brenniebrennie

Remove this line.

brenniebrennie

Is this still WIP

brenniebrennie

This file is all third-party code. I don't think we want to make any style fixes in here, and even …

daviddavid

It's probably a little silly to define two new variables when these are only used once. In fact, this whole …

daviddavid

Please undo this change.

daviddavid

What are these changes?

daviddavid

It's okay to explicitly compare against true in cases where this could take other falsy values, but we should at …

daviddavid

The var statement should be the first thing in the function body.

daviddavid

In es5 code (which this is, since we haven't yet ported it to es6), the var statement should be the …

daviddavid

There's still a bunch of style changes in this file which are obscuring your functional changes. Please go through until …

daviddavid

Multi-line comments should use /* ... */.

daviddavid

Trailing whitespace.

daviddavid

Braces.

brenniebrennie

Prefer: var data; if (!options.matchCase && typeof term === 'string') { term = term.toLowerCase(); } data = cache.load(term); We want …

brenniebrennie

!==, 'string'

brenniebrennie

blank line.

brenniebrennie

This needn't be two conditions: if (options.fieldName === 'review_requests' && !data.public) { return false; }

brenniebrennie

I'm not sure I'm comfortable with this here. The code using autocomplete should be the one responsible for supplying strings.

chipx86chipx86

Similarly here. I don't think we should be enforcing strings here.

chipx86chipx86

No space after function.

chipx86chipx86

A couple of stylistic comments: * Must be 4 space indentation. * The if line is too long. Lines should …

chipx86chipx86

Variables with values should come before variables without.

chipx86chipx86

We don't want to have field-specific logic here. Instead, we want the code registering a field to be able to …

chipx86chipx86

No space after function.

chipx86chipx86

Instead of $($field.children()[0]), you can do $field.children().eq(0), etc. Also, we should fetch $field.children() only once.

chipx86chipx86

A few things here. First, javascript variables should be camelCase. Secondly, combine this into one var statement: var unwrappedData = …

brenniebrennie

We always use /* block comments */.

brenniebrennie

['public'] as public is a reserved word and will break in IE.

brenniebrennie

i--

brenniebrennie

Reformat as: unwrappedData[i].id = unwrappedData[i] .id .toString();

brenniebrennie

No trailing comma (will break IE)

brenniebrennie

We don't need to add this. We should be setting display_name for the results instead in the caller.

chipx86chipx86

Space after :

chipx86chipx86

This function can be written much more simply: function(data, options) { return data.search[options.fieldName] .filter(function(datum) { return !datum['public']; }) .map(function(datum) { …

brenniebrennie

We should never have private reviews here, under any circumstances. The API must return only public results.

chipx86chipx86

We access unwrappedData[i] a lot. let's pull that out once.

chipx86chipx86

if (!unwrappedData[i]['public']) {

brenniebrennie

Once unwrappedData[i] is pulled out, this can/should be one line.

chipx86chipx86

Shouldn't this be return unwrappedData?

daviddavid

Indentation is still wrong.

chipx86chipx86

We shouldn't be touching this code. Only the new field should be modified.

chipx86chipx86

Same here.

chipx86chipx86

You got rid of itemsLen. There's no way this code works. Please make sure this is all very properly tested.

chipx86chipx86

Can we pull out unwrappedData[i] into another variable, say datum and use that here?

brenniebrennie

Can we call it item instead of value?

brenniebrennie

I don't think this extra level of nesting is required. You're not adding beforeEach or afterEach.

brenniebrennie

Indentation is off by 1 space for the for loop here.

daviddavid

Your indentation is still funky here (the for loop and everything in it needs to be indented 1 more space).

daviddavid

One nice way to clean this up would be to make it so options.fieldName could be either a function or …

daviddavid
reviewbot
  1. Tool: Pyflakes
    Ignored Files:
        reviewboard/static/rb/js/views/reviewRequestEditorView.js
        reviewboard/static/lib/js/ui.autocomplete.js
    
    
    
    Tool: PEP8 Style Checker
    Ignored Files:
        reviewboard/static/rb/js/views/reviewRequestEditorView.js
        reviewboard/static/lib/js/ui.autocomplete.js
    
    
  2. 
      
CO
reviewbot
  1. Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/webapi/resources/search.py
    
    Ignored Files:
        reviewboard/static/rb/js/views/reviewRequestEditorView.js
        reviewboard/static/lib/js/ui.autocomplete.js
    
    
    
    Tool: Pyflakes
    Processed Files:
        reviewboard/webapi/resources/search.py
    
    Ignored Files:
        reviewboard/static/rb/js/views/reviewRequestEditorView.js
        reviewboard/static/lib/js/ui.autocomplete.js
    
    
  2. 
      
CO
RS
  1. 
      
  2. Show all issues

    One comment I've seen repeated is that our request description should kind've "tell a story" with respect to the project. It'd be useful to me (and maybe other students) to hear about the purpose of this project and how your changes accomplish that.

    1. Yeah, thats a good idea. I actually just wanted to get this posted before I stopped, so it only has the git commit message. I'll definitely make sure to update it.

  3. 
      
LA
  1. 
      
  2. Show all issues

    This is a good time to ask, if we can leave comments in a request. I think they should be removed so as they are not forgotten and published with it

    1. Probably a good idea. I went back and removed all of them.

  3. Show all issues

    this can use the operator "===" for extra security

  4. Show all issues

    if changing the operator to "===" is okay with you

  5. 
      
CO
reviewbot
  1. Tool: Pyflakes
    Ignored Files:
        reviewboard/static/rb/js/views/reviewRequestEditorView.js
        reviewboard/static/rb/js/views/tests/reviewRequestEditorViewTests.js
        reviewboard/static/lib/js/ui.autocomplete.js
    
    
    
    Tool: PEP8 Style Checker
    Ignored Files:
        reviewboard/static/rb/js/views/reviewRequestEditorView.js
        reviewboard/static/rb/js/views/tests/reviewRequestEditorViewTests.js
        reviewboard/static/lib/js/ui.autocomplete.js
    
    
  2. 
      
CO
brennie
  1. 
      
  2. Show all issues

    Can you rewrite your summary in the imperitive mood? i.e, "Add auto-complete functionality to the Depends On field"

  3. Show all issues

    Can you add screenshots?

  4. reviewboard/static/lib/js/ui.autocomplete.js (Diff revision 3)
     
     
     
     
     
     
    Show all issues

    Fix indentation.

  5. Show all issues

    Can you remove the spacing between the parentheses?

    1. This style appears throughout this entire file. I've gone back and removed all of it.

    2. Apperently reformating this was not the way to go. I've reverted that changes throughout the rest of the file. Should I have this one match for consistency or should I remove the spacing anyways?

  6. Show all issues

    Undo this change.

  7. Show all issues

    This can be a one line comment. I'm not sure if its necessary, though.

  8. Show all issues

    Undo this.

  9. Show all issues

    Is this intentional?

  10. Show all issues

    space after colon

  11. Show all issues

    Undo this.

  12. Show all issues

    Undo this.

  13. Show all issues

    Undo this.

  14. Show all issues

    Undo this.

  15. Show all issues

    Remove this line.

  16. Show all issues

    Undo this.

  17. Show all issues

    Undo this.

  18. Show all issues

    Remove this line.

  19. Show all issues

    Is this still WIP

  20. 
      
CO
reviewbot
  1. Tool: PEP8 Style Checker
    Ignored Files:
        reviewboard/static/rb/js/views/reviewRequestEditorView.js
        reviewboard/static/rb/js/views/tests/reviewRequestEditorViewTests.js
        reviewboard/static/lib/js/ui.autocomplete.js
    
    
    
    Tool: Pyflakes
    Ignored Files:
        reviewboard/static/rb/js/views/reviewRequestEditorView.js
        reviewboard/static/rb/js/views/tests/reviewRequestEditorViewTests.js
        reviewboard/static/lib/js/ui.autocomplete.js
    
    
  2. 
      
CO
david
  1. 
      
  2. Show all issues

    This file is all third-party code. I don't think we want to make any style fixes in here, and even if we did, they should be in a separate change so they don't obscure the few changes you did make to the code.

    That said, I don't think any changes should be required in here. If they are, we definitely need to look at them closely.

  3. Show all issues

    It's probably a little silly to define two new variables when these are only used once. In fact, this whole function could be:

    cmp: function(term, a, b) {
       return b.data.id - a.data.id;
    }
    
  4. Show all issues

    Please undo this change.

  5. Show all issues

    What are these changes?

  6. Show all issues

    It's okay to explicitly compare against true in cases where this could take other falsy values, but we should at least use !== instead of != because != will implicitly cast, which makes the explicit comparison moot.

  7. Show all issues

    The var statement should be the first thing in the function body.

  8. reviewboard/static/rb/js/views/reviewRequestEditorView.js (Diff revision 4)
     
     
     
     
     
     
     
     
     
     
    Show all issues

    In es5 code (which this is, since we haven't yet ported it to es6), the var statement should be the first thing in the function. That means we need to postpone the assignment of a few of these:

    var parsed = [],
        items,
        value,
        i;
    
    if (options.resourceName === 'search') {
        data = data.search;
    }
    
    items = data[options.fieldName];
    
    for (i = 0; i < items.length; i++) {
        ...
    }
    

    Note that I've removed the kind of useless itemsLen variable. I'd also like the "search" special case to be commented explaining why it's there.

  9. 
      
CO
reviewbot
  1. Tool: Pyflakes
    Ignored Files:
        reviewboard/static/rb/js/views/reviewRequestEditorView.js
        reviewboard/static/rb/js/views/tests/reviewRequestEditorViewTests.js
        reviewboard/static/lib/js/ui.autocomplete.js
    
    
    
    Tool: PEP8 Style Checker
    Ignored Files:
        reviewboard/static/rb/js/views/reviewRequestEditorView.js
        reviewboard/static/rb/js/views/tests/reviewRequestEditorViewTests.js
        reviewboard/static/lib/js/ui.autocomplete.js
    
    
  2. 
      
CO
reviewbot
  1. Tool: PEP8 Style Checker
    Ignored Files:
        reviewboard/static/rb/js/views/reviewRequestEditorView.js
        reviewboard/static/rb/js/views/tests/reviewRequestEditorViewTests.js
        reviewboard/static/lib/js/ui.autocomplete.js
    
    
    
    Tool: Pyflakes
    Ignored Files:
        reviewboard/static/rb/js/views/reviewRequestEditorView.js
        reviewboard/static/rb/js/views/tests/reviewRequestEditorViewTests.js
        reviewboard/static/lib/js/ui.autocomplete.js
    
    
  2. 
      
david
  1. 
      
  2. reviewboard/static/lib/js/ui.autocomplete.js (Diff revision 6)
     
     
     
     
    Show all issues

    There's still a bunch of style changes in this file which are obscuring your functional changes. Please go through until the diff has only the changes which actually affect the behavior, and then we can discuss why they're necessary.

  3. Show all issues

    Multi-line comments should use /* ... */.

  4. Show all issues

    Trailing whitespace.

  5. 
      
CO
reviewbot
  1. Tool: PEP8 Style Checker
    Ignored Files:
        reviewboard/static/rb/js/views/reviewRequestEditorView.js
        reviewboard/static/rb/js/views/tests/reviewRequestEditorViewTests.js
        reviewboard/static/lib/js/ui.autocomplete.js
    
    
    
    Tool: Pyflakes
    Ignored Files:
        reviewboard/static/rb/js/views/reviewRequestEditorView.js
        reviewboard/static/rb/js/views/tests/reviewRequestEditorViewTests.js
        reviewboard/static/lib/js/ui.autocomplete.js
    
    
  2. 
      
david
  1. 
      
  2. Can you explain why you need all these new String() calls?

    1. Originally the code was meant to only handle strings. However, review requests can also return integers. I decided simply to cast everything to a string rather than adding additional cases to handle integers.

  3. 
      
brennie
  1. 
      
  2. Show all issues

    Braces.

    1. This file is mostly third-party code, so it's better to match the existing style.

  3. reviewboard/static/lib/js/ui.autocomplete.js (Diff revision 7)
     
     
     
     
    Show all issues

    Prefer:

    var data;
    
    if (!options.matchCase && typeof term === 'string') {
        term = term.toLowerCase();
    }
    
    data = cache.load(term);
    

    We want to use === basically always.

    1. We probably shouldn't make changes to the braces/var location, but we should use ===

  4. reviewboard/static/lib/js/ui.autocomplete.js (Diff revision 7)
     
     
     
     
    Show all issues

    !==, 'string'

  5. Show all issues

    blank line.

  6. reviewboard/static/rb/js/views/reviewRequestEditorView.js (Diff revision 7)
     
     
     
     
     
     
    Show all issues

    This needn't be two conditions:

    if (options.fieldName === 'review_requests' && !data.public) {
        return false;
    }
    
  7. 
      
CO
reviewbot
  1. Tool: Pyflakes
    Ignored Files:
        reviewboard/static/rb/js/views/reviewRequestEditorView.js
        reviewboard/static/rb/js/views/tests/reviewRequestEditorViewTests.js
        reviewboard/static/lib/js/ui.autocomplete.js
    
    
    
    Tool: PEP8 Style Checker
    Ignored Files:
        reviewboard/static/rb/js/views/reviewRequestEditorView.js
        reviewboard/static/rb/js/views/tests/reviewRequestEditorViewTests.js
        reviewboard/static/lib/js/ui.autocomplete.js
    
    
  2. 
      
CO
reviewbot
  1. Tool: PEP8 Style Checker
    Ignored Files:
        reviewboard/static/rb/js/views/reviewRequestEditorView.js
        reviewboard/static/rb/js/views/tests/reviewRequestEditorViewTests.js
        reviewboard/static/lib/js/ui.autocomplete.js
    
    
    
    Tool: Pyflakes
    Ignored Files:
        reviewboard/static/rb/js/views/reviewRequestEditorView.js
        reviewboard/static/rb/js/views/tests/reviewRequestEditorViewTests.js
        reviewboard/static/lib/js/ui.autocomplete.js
    
    
  2. 
      
chipx86
  1. Good job so far. My biggest concern is that there are changes made that are too global.

    I'm not sure you need to make any modifications to ui.autocomplete.js. Instead, the code working with the autocomplete widget should be responsible for returning the appropriate data types. By casting to a string, you can alter behavior that might conceivably exist in other use cases, and make it harder to eventually move away from this widget (as the data type casting behavior is "hidden" from the caller's code).

    I also have some suggestions for the if checks in the autocomplete invocation in reviewRequestEditorView.js, intended to keep from adding field-specific behavior here (fields are pluggable and we don't want to hard-code things if we can help it).

  2. Show all issues

    I'm not sure I'm comfortable with this here. The code using autocomplete should be the one responsible for supplying strings.

  3. reviewboard/static/lib/js/ui.autocomplete.js (Diff revision 9)
     
     
     
     
    Show all issues

    Similarly here. I don't think we should be enforcing strings here.

  4. Show all issues

    No space after function.

  5. Show all issues

    A couple of stylistic comments: * Must be 4 space indentation. * The if line is too long. Lines should not exceed 79 characters. This should wrap after &&.

    The bigger thing is that this begins a precedent of field-specific logic in here. We don't want that. Fields are pluggable, and we want extensions to be able to provide their own fields.

    In this particular case, what we're saying is "hide any private review requests." We shouldn't even be getting these in the result to begin with. What you instead need to do is modify the API request to exclude any private review requests for this field, so they never come through to begin with. We can then safely display anything that we get back.

    This is very important. Imagine if we only get 20 results back and the first 18 are private. That'd mean we only get 2 public review requests to display, instead of 20. That'd be a confusing experience.

  6. Show all issues

    Variables with values should come before variables without.

  7. reviewboard/static/rb/js/views/reviewRequestEditorView.js (Diff revision 9)
     
     
     
     
     
     
    Show all issues

    We don't want to have field-specific logic here. Instead, we want the code registering a field to be able to parse the result.

    If you look at registerField, you'll see where fields get registered. There's an option (which sadly is not documented there, but you can see usage in setupFieldEditor) that indicates if autocomplete behavior should be used. This is a dictionary of things like fieldName, nameKey, etc.

    What I'd recommend is adding another optional thing in that dictionary: parseResultList. This would be a function that takes the data and return the list of items. If that exists in the dictionary, it'd be called, and if not, the default items = data[options.fieldName] would be used.

    Then the Depends On handler could provide its own implementation, removing the hard-coded support for data.search.

  8. Show all issues

    No space after function.

  9. Show all issues

    Instead of $($field.children()[0]), you can do $field.children().eq(0), etc.

    Also, we should fetch $field.children() only once.

  10. 
      
CO
reviewbot
  1. Tool: PEP8 Style Checker
    Ignored Files:
        reviewboard/static/rb/js/views/reviewRequestEditorView.js
        reviewboard/static/rb/js/views/tests/reviewRequestEditorViewTests.js
        reviewboard/static/lib/js/ui.autocomplete.js
    
    
    
    Tool: Pyflakes
    Ignored Files:
        reviewboard/static/rb/js/views/reviewRequestEditorView.js
        reviewboard/static/rb/js/views/tests/reviewRequestEditorViewTests.js
        reviewboard/static/lib/js/ui.autocomplete.js
    
    
  2. 
      
brennie
  1. 
      
  2. Show all issues

    A few things here.

    First, javascript variables should be camelCase.

    Secondly, combine this into one var statement:

    var unwrappedData = data.search[options.fieldName],
        i;
    

    (because of var hoisting)

  3. Show all issues

    We always use /* block comments */.

  4. Show all issues

    ['public'] as public is a reserved word and will break in IE.

  5. Show all issues

    i--

  6. Show all issues

    Reformat as:

    unwrappedData[i].id = unwrappedData[i]
        .id
        .toString();
    
  7. Show all issues

    No trailing comma (will break IE)

  8. 
      
CO
reviewbot
  1. Tool: Pyflakes
    Ignored Files:
        reviewboard/static/rb/js/views/reviewRequestEditorView.js
        reviewboard/static/rb/js/views/tests/reviewRequestEditorViewTests.js
        reviewboard/static/lib/js/ui.autocomplete.js
    
    
    
    Tool: PEP8 Style Checker
    Ignored Files:
        reviewboard/static/rb/js/views/reviewRequestEditorView.js
        reviewboard/static/rb/js/views/tests/reviewRequestEditorViewTests.js
        reviewboard/static/lib/js/ui.autocomplete.js
    
    
  2. 
      
brennie
  1. 
      
  2. reviewboard/static/rb/js/views/reviewRequestEditorView.js (Diff revision 11)
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
    Show all issues

    This function can be written much more simply:

    function(data, options) {
        return data.search[options.fieldName]
            .filter(function(datum) { return !datum['public']; })
            .map(function(datum) {
                datum.id = datum.id.toString();
                return datum;
            });
    });
    
    1. It's simpler, but slower. The current function can be more optimal. filter is going to construct a whole new array that map then has to iterate through. It's better to do this in one pass.

  3. Show all issues

    if (!unwrappedData[i]['public']) {
    
  4. 
      
david
  1. 
      
  2. Show all issues

    Shouldn't this be return unwrappedData?

  3. 
      
chipx86
  1. 
      
  2. reviewboard/static/lib/js/ui.autocomplete.js (Diff revision 11)
     
     
     
    Show all issues

    We don't need to add this. We should be setting display_name for the results instead in the caller.

    1. Hmmm. I didn't test it but the else if block above that one is probably unneeded as well then.

      else if (selected.data.fullname)
          toMatch = toMatch.concat([selected.data.first_name, selected.data.last_name]);
      
    2. Turns out I'm wrong and you need that if you're searching for last names.

  3. Show all issues

    Space after :

  4. Show all issues

    We should never have private reviews here, under any circumstances. The API must return only public results.

  5. reviewboard/static/rb/js/views/reviewRequestEditorView.js (Diff revision 11)
     
     
     
     
     
     
     
     
    Show all issues

    We access unwrappedData[i] a lot. let's pull that out once.

  6. Show all issues

    Once unwrappedData[i] is pulled out, this can/should be one line.

  7. Show all issues

    Indentation is still wrong.

  8. Show all issues

    We shouldn't be touching this code. Only the new field should be modified.

  9. Show all issues

    Same here.

  10. reviewboard/static/rb/js/views/reviewRequestEditorView.js (Diff revision 11)
     
     
     
     
     
     
     
     
    Show all issues

    You got rid of itemsLen. There's no way this code works. Please make sure this is all very properly tested.

  11. 
      
CO
reviewbot
  1. Tool: PEP8 Style Checker
    Ignored Files:
        reviewboard/static/rb/js/views/reviewRequestEditorView.js
        reviewboard/static/rb/js/views/tests/reviewRequestEditorViewTests.js
        reviewboard/static/lib/js/ui.autocomplete.js
    
    
    
    Tool: Pyflakes
    Ignored Files:
        reviewboard/static/rb/js/views/reviewRequestEditorView.js
        reviewboard/static/rb/js/views/tests/reviewRequestEditorViewTests.js
        reviewboard/static/lib/js/ui.autocomplete.js
    
    
  2. 
      
CO
reviewbot
  1. Tool: Pyflakes
    Ignored Files:
        reviewboard/static/rb/js/views/tests/reviewRequestEditorViewTests.js
        reviewboard/static/rb/js/views/reviewRequestEditorView.js
    
    
    
    Tool: PEP8 Style Checker
    Ignored Files:
        reviewboard/static/rb/js/views/tests/reviewRequestEditorViewTests.js
        reviewboard/static/rb/js/views/reviewRequestEditorView.js
    
    
  2. 
      
brennie
  1. 
      
  2. Show all issues

    Can we pull out unwrappedData[i] into another variable, say datum and use that here?

  3. Show all issues

    Can we call it item instead of value?

  4. Show all issues

    I don't think this extra level of nesting is required. You're not adding beforeEach or afterEach.

  5. 
      
david
  1. 
      
  2. reviewboard/static/rb/js/views/reviewRequestEditorView.js (Diff revision 13)
     
     
     
     
     
     
     
     
     
     
    Show all issues

    Indentation is off by 1 space for the for loop here.

  3. 
      
CO
reviewbot
  1. Tool: Pyflakes
    Ignored Files:
        reviewboard/static/rb/js/views/tests/reviewRequestEditorViewTests.js
        reviewboard/static/rb/js/views/reviewRequestEditorView.js
    
    
    
    Tool: PEP8 Style Checker
    Ignored Files:
        reviewboard/static/rb/js/views/tests/reviewRequestEditorViewTests.js
        reviewboard/static/rb/js/views/reviewRequestEditorView.js
    
    
  2. 
      
david
  1. 
      
    1. The issue with this is that the in:

      items = data[options.fieldName];
      
      for (i = 0; i < items.length; i++) {
                              item = items[i];
      
                              if (options.parseItem) {
                                  item = options.parseItem(item);
                              }
      
                              parsed.push({
                                  data: item,
                                  value: item[options.nameKey],
                                  result: item[options.nameKey]
                              });
                          }
      

      The items = data[options.fieldName]; line can cause errors as there may be an extra wrapper on the data, resulting in items being undefined.
      You can see in the current parseResultsList we have unwrappedData = data.search[options.fieldName]; which is where we remove the datawrapper.

      I've fixed this by replacing parseResultsList with a new optional parameter formatData. While this is more efficient, it seems rather convoluted to have two seperate parameters to accomplish the same goal.

  2. Show all issues

    Looking at the code now, I think we can make a major simplification.

    Right now, we have two iterations over the list of items--one in parseResultList, and one in parse. A simpler approach would be to have an optional parseItem method which would modify an individual item in the results, looking something like:

    parseItem: function(item) {
        item.id = item.id.toString();
        item.display_name = item.summary;
        return item;
    }
    

    Inside parse, you'd then do something like:

    for (i = 0; i < items.length; i++) {
        item = items[i];
    
        if (options.parseItem) {
            item = options.parseItem(item);
        }
    
        parsed.push({
            ...
        });
    }
    
  3. reviewboard/static/rb/js/views/reviewRequestEditorView.js (Diff revision 14)
     
     
     
     
     
     
     
     
     
     
     
     
    Show all issues

    Your indentation is still funky here (the for loop and everything in it needs to be indented 1 more space).

  4. 
      
CO
reviewbot
  1. Tool: PEP8 Style Checker
    Ignored Files:
        reviewboard/static/rb/js/views/tests/reviewRequestEditorViewTests.js
        reviewboard/static/rb/js/views/reviewRequestEditorView.js
    
    
    
    Tool: Pyflakes
    Ignored Files:
        reviewboard/static/rb/js/views/tests/reviewRequestEditorViewTests.js
        reviewboard/static/rb/js/views/reviewRequestEditorView.js
    
    
  2. 
      
CO
david
  1. 
      
  2. reviewboard/static/rb/js/views/reviewRequestEditorView.js (Diff revision 15)
     
     
     
     
     
     
    Show all issues

    One nice way to clean this up would be to make it so options.fieldName could be either a function or an actual field name. This would then be:

    if (_.isFunction(options.fieldName)) {
        items = options.fieldName(data);
    } else {
        items = data[options.fieldName];
    }
    

    and the fieldName for the depends_on field would be:

    fieldName: function(data) {
        return data.search.review_requests;
    }
    

    That way we don't have to have both fieldName and the new formatData function.

    1. This works for this case, but in

      url: SITE_ROOT + reviewRequest.get('localSitePrefix') +
                           'api/' + (options.resourceName || options.fieldName) + '/'
      

      if we don't specify a resource name and have fieldName as a function this will throw an error.
      Also this seems less intuitive than just having a formatData field.

    2. Well, we already have the resourceName in there. The most correct thing is to probably never use fieldName for the resource name and always specify what resource we want explicitly, even when it's the same as fieldName.

  3. 
      
CO
reviewbot
  1. Tool: PEP8 Style Checker
    Ignored Files:
        reviewboard/static/rb/js/views/tests/reviewRequestEditorViewTests.js
        reviewboard/static/rb/js/views/reviewRequestEditorView.js
    
    
    
    Tool: Pyflakes
    Ignored Files:
        reviewboard/static/rb/js/views/tests/reviewRequestEditorViewTests.js
        reviewboard/static/rb/js/views/reviewRequestEditorView.js
    
    
  2. 
      
david
  1. Ship It!
  2. 
      
CO
Review request changed
Status:
Completed
Change Summary:
Pushed to release-3.0.x (3cdd33b)