Add auto-complete functionality to the Depends On field

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

Connor-Y
Review Board
release-2.5.x
3aee79d...
reviewboard, students

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
Loading file attachments...

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

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. 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. 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. this can use the operator "===" for extra security

  4. 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. Can you rewrite your summary in the imperitive mood? i.e, "Add auto-complete functionality to the Depends On field"

  3. Can you add screenshots?

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

    Fix indentation.

  5. 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. Undo this change.

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

  8. Is this intentional?

  9. 
      
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. 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. 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. Please undo this change.

  5. What are these changes?

  6. 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. The var statement should be the first thing in the function body.

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

    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)
     
     
     
     

    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. Multi-line comments should use /* ... */.

  4. 
      
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. 
      
    1. This file is mostly third-party code, so it's better to match the existing style.

  2. reviewboard/static/lib/js/ui.autocomplete.js (Diff revision 7)
     
     
     
     

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

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

    !==, 'string'

  4. blank line.

  5. reviewboard/static/rb/js/views/reviewRequestEditorView.js (Diff revision 7)
     
     
     
     
     
     

    This needn't be two conditions:

    if (options.fieldName === 'review_requests' && !data.public) {
        return false;
    }
    
  6. 
      
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. 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)
     
     
     
     

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

  4. No space after function.

  5. 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. Variables with values should come before variables without.

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

    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. Instead of $($field.children()[0]), you can do $field.children().eq(0), etc.

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

  9. 
      
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. 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. We always use /* block comments */.

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

  5. Reformat as:

    unwrappedData[i].id = unwrappedData[i]
        .id
        .toString();
    
  6. No trailing comma (will break IE)

  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. 
      
brennie
  1. 
      
  2. reviewboard/static/rb/js/views/reviewRequestEditorView.js (Diff revision 11)
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     

    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. if (!unwrappedData[i]['public']) {
    
  4. 
      
david
  1. 
      
  2. Shouldn't this be return unwrappedData?

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

    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. We should never have private reviews here, under any circumstances. The API must return only public results.

  4. reviewboard/static/rb/js/views/reviewRequestEditorView.js (Diff revision 11)
     
     
     
     
     
     
     
     

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

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

  6. Indentation is still wrong.

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

  8. Same here.

  9. reviewboard/static/rb/js/views/reviewRequestEditorView.js (Diff revision 11)
     
     
     
     
     
     
     
     

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

  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. 
      
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. Can we pull out unwrappedData[i] into another variable, say datum and use that here?

  3. Can we call it item instead of value?

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

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

    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)
     
     
     
     
     
     

    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: Closed (submitted)

Change Summary:

Pushed to release-3.0.x (3cdd33b)
Loading...