• 
      

    Rework review request fields.

    Review Request #9144 — Created Aug. 22, 2017 and submitted

    Information

    Review Board
    release-3.0.x
    5dc6c78...

    Reviewers

    This change does a fairly large cleanup to the JavaScript side of the review
    request fields. The old way was pretty hard-coded into the
    ReviewRequestEditorView: each of the builtin fields had an entry which defined
    some metadata about it, and the editor would scan through those, and then scan
    through any remaining fields and pull (more limited) metadata out of the
    markup.

    With the new model introduced in this change, each (editable) field has a
    javascript view of its own which handles its interaction. There are base
    classes for the existing types of fields, including plain text fields,
    comma-separated multivalue fields, and multiline text fields. By default, each
    of the field types defined in fields.py maps to one of these base classes,
    which will fall back on the old behavior in order to maintain compatibility
    with existing extensions that use them. If those field view types are
    subclasses, they can add additional features such as autocomplete or
    specialized saving mechanics. There's no longer any special casing between
    "built-in" and "other" fields, aside from the fact that the built-in field
    views specify useExtraData: false.

    While I was in here, I cleaned up several related issues:
    - Both the "close description" and "change description" fields used an element
    with the ID #field_changedescription.
    - We no longer hard-code #field_changedescription in the CSS.
    - Some documentation updates and tweaks.

    There's still a lot more cleanup that can be done, moving functionality from
    the ReviewRequestEditorModel and ReviewRequestEditorView into the field
    classes, but this change seemed like it was big enough already.

    • Ran unit tests.
    • Ran js-tests.
    • Manually tested all fields on the review request page, including rich text
      and CSV-type fields that linkified their values in various ways.
    • Manually tested an extension that added various field types.
    Description From Last Updated

    The unit testing could use more fleshing out. I know we have existing tests for the built-in fields, but we …

    chipx86 chipx86

    Col: 2 Expected a string and instead saw %.

    reviewbot reviewbot

    Col: 4 Expected ':' and instead saw 'load'.

    reviewbot reviewbot

    Col: 9 Expected a JSON value.

    reviewbot reviewbot

    Col: 9 Expected '}' and instead saw 'reviewtags'.

    reviewbot reviewbot

    Col: 9 Unrecoverable syntax error. (4% scanned).

    reviewbot reviewbot

    "JavaScript" Here and all the others.

    chipx86 chipx86

    Description is intended 1 space too far.

    chipx86 chipx86

    This kinda reads weird. Maybe: The way that the review request should be closed. This can be one of <values>. …

    chipx86 chipx86

    So this broke when originally converted to ES6. The newline shouldn't have been there, as that adds unwanted whitespace to …

    chipx86 chipx86

    Same here. There shouldn't be any newline involved.

    chipx86 chipx86

    You can pass this.showBanner in directly. No need for the wrapping function.

    chipx86 chipx86

    I'd like to avoid using for .. of. Babel turns it into a whole lot of pretty expensive JavaScript. Can …

    chipx86 chipx86

    Missing trailing period.

    chipx86 chipx86

    One-line summary.

    chipx86 chipx86

    This should be using the standard function doc format.

    chipx86 chipx86

    "Python", "JavaScript"

    chipx86 chipx86

    Second sentence should be its own paragraph. One line summary.

    chipx86 chipx86

    One-line summary, standard function doc format (since we don't have properties in JavaScript and shouldn't pretend).

    chipx86 chipx86

    One-line summary.

    chipx86 chipx86

    Needs to return this.

    chipx86 chipx86

    What if we don't want auto-complete for the text field? Can this be optional?

    chipx86 chipx86

    Can you put the property on the next line? Reads a bit better.

    chipx86 chipx86

    This can use template literals.

    chipx86 chipx86

    JSON.parse?

    brennie brennie

    Can you expand this to say more about what this does? How it formats it?

    chipx86 chipx86

    Looks like we only use draft this once. We can get rid of that variable.

    chipx86 chipx86

    For each of the built-in field descriptions, can you use the label (like "Branch", "Change Description", etc.) in the doc …

    chipx86 chipx86

    "Array"

    chipx86 chipx86

    Can you use data=[] in the function definition?

    chipx86 chipx86

    "Array"

    chipx86 chipx86

    Can this be data=[]?

    chipx86 chipx86

    "Array"

    chipx86 chipx86

    data=[]?

    chipx86 chipx86

    "Array"

    chipx86 chipx86

    data=[]?

    chipx86 chipx86

    afterEach needs to do RB.DnDUploader.instance = null. Not sure whether this change came before my cleanup, so maybe this is …

    chipx86 chipx86

    The include should be indented 2 more spaces.

    chipx86 chipx86

    The include should be indented 1 more space.

    chipx86 chipx86

    Col: 9 Expected '}' and instead saw 'reviewtags'.

    reviewbot reviewbot

    Col: 9 Expected a JSON value.

    reviewbot reviewbot

    Col: 9 Unrecoverable syntax error. (4% scanned).

    reviewbot reviewbot

    Col: 4 Expected ':' and instead saw 'load'.

    reviewbot reviewbot

    Col: 2 Expected a string and instead saw %.

    reviewbot reviewbot

    Still needed?

    chipx86 chipx86

    Line's too long now. Review Bot's going to yell forever :/ Would be pretty cool to have dedent allow for …

    chipx86 chipx86

    As discovered recently, we'll need to wrap the file in (function() { ... })();

    chipx86 chipx86

    This might be faster as: this._fieldName = this.fieldID.replace(/_(.)/g, (m, c) => c.toUpperCase()); Assuming you want to strip away all underscores …

    chipx86 chipx86

    Missing "Returns" This should also go after useEditIconOnly.

    chipx86 chipx86

    Typo in "Markdown".

    chipx86 chipx86

    Col: 9 Unrecoverable syntax error. (5% scanned).

    reviewbot reviewbot

    Col: 9 Expected '}' and instead saw 'reviewtags'.

    reviewbot reviewbot

    Col: 9 Expected a JSON value.

    reviewbot reviewbot

    Col: 4 Expected ':' and instead saw 'load'.

    reviewbot reviewbot

    Col: 2 Expected a string and instead saw %.

    reviewbot reviewbot

    Col: 4 Expected ':' and instead saw 'load'.

    reviewbot reviewbot

    Col: 9 Expected a JSON value.

    reviewbot reviewbot

    Col: 9 Expected '}' and instead saw 'reviewtags'.

    reviewbot reviewbot

    Col: 9 Unrecoverable syntax error. (5% scanned).

    reviewbot reviewbot

    Col: 2 Expected a string and instead saw %.

    reviewbot reviewbot
    Checks run (1 failed, 1 succeeded)
    flake8 passed.
    JSHint failed.

    JSHint

    david
    Review request changed
    Change Summary:

    Shuffle a bit of code up to base classes.

    Commit:
    1205a491b672242e49ba4ac750364c6735d51e5d
    e5d36b83ab0cf97039c84ef95dd072eb1dcce924

    Checks run (1 failed, 1 succeeded)

    flake8 passed.
    JSHint failed.

    JSHint

    brennie
    1. 
        
    2. Show all issues

      JSON.parse?

    3. 
        
    chipx86
    1. Looks great overall! Love where this is going.

      A lot of my comments actually have to do with the docs not being consistent with standards. I've marked those. Also have comments about testing.

    2. Show all issues

      The unit testing could use more fleshing out. I know we have existing tests for the built-in fields, but we really should have tests for all the new registration support and the options for each field to ensure that the base foundation is working correctly. I know that's a lot more work, but it'll be worth it.

      1. I agree that we should have more tests for all that but must it block this change? This is pretty big already.

      2. It can wait. Just something for the radar.

    3. reviewboard/reviews/builtin_fields.py (Diff revision 2)
       
       
      Show all issues

      "JavaScript"

      Here and all the others.

    4. Show all issues

      Description is intended 1 space too far.

    5. Show all issues

      This kinda reads weird. Maybe:

      The way that the review request should be closed. This can be one of <values>.
      
      This is only used if ``fieldName`` is ``closeDescription``.
      
      1. This is obsoleted by /r/9150/

    6. Show all issues

      So this broke when originally converted to ES6. The newline shouldn't have been there, as that adds unwanted whitespace to be beginning. Mind fixing it while you're here? <%- describeText %> should be flush up against the prior <label>.

    7. Show all issues

      Same here. There shouldn't be any newline involved.

    8. Show all issues

      You can pass this.showBanner in directly. No need for the wrapping function.

    9. Show all issues

      I'd like to avoid using for .. of. Babel turns it into a whole lot of pretty expensive JavaScript. Can you use a standard for .. in or _.each() instead?

    10. Show all issues

      Missing trailing period.

    11. Show all issues

      One-line summary.

    12. Show all issues

      This should be using the standard function doc format.

    13. Show all issues

      "Python", "JavaScript"

    14. Show all issues

      Second sentence should be its own paragraph. One line summary.

    15. Show all issues

      One-line summary, standard function doc format (since we don't have properties in JavaScript and shouldn't pretend).

    16. Show all issues

      One-line summary.

    17. Show all issues

      Needs to return this.

    18. Show all issues

      What if we don't want auto-complete for the text field? Can this be optional?

      1. I'm not sure what you mean by this comment, especially on these lines. Auto-complete is only set up if the autocomplete attribute is defined.

      2. I have no idea what I was reading when I wrote that. Ignore!

    19. Show all issues

      Can you put the property on the next line? Reads a bit better.

    20. Show all issues

      This can use template literals.

    21. Show all issues

      Can you expand this to say more about what this does? How it formats it?

    22. Show all issues

      Looks like we only use draft this once. We can get rid of that variable.

    23. Show all issues

      For each of the built-in field descriptions, can you use the label (like "Branch", "Change Description", etc.) in the doc summary?

    24. Show all issues

      "Array"

    25. Show all issues

      Can you use data=[] in the function definition?

    26. Show all issues

      "Array"

    27. Show all issues

      Can this be data=[]?

    28. Show all issues

      "Array"

    29. Show all issues

      data=[]?

    30. Show all issues

      "Array"

    31. Show all issues

      data=[]?

      1. That only works if data is undefined. We want to be able to handle the case where it's also null.

    32. Show all issues

      afterEach needs to do RB.DnDUploader.instance = null. Not sure whether this change came before my cleanup, so maybe this is working around issues fixed there, but for reference, see commit 528b268.

      1. This came before your cleanup, so it just goes away.

    33. Show all issues

      The include should be indented 2 more spaces.

    34. Show all issues

      The include should be indented 1 more space.

    35. Show all issues

      Still needed?

    36. 
        
    david
    Review request changed
    Commit:
    e5d36b83ab0cf97039c84ef95dd072eb1dcce924
    a588a33c2be12b1f99a85a2335cdbaeb10ebf8ab

    Checks run (1 failed, 1 succeeded)

    flake8 passed.
    JSHint failed.

    JSHint

    chipx86
    1. Only a few comments. Looks good to me otherwise. I'll do a final pass after this.

    2. Show all issues

      Line's too long now. Review Bot's going to yell forever :/ Would be pretty cool to have dedent allow for some character at the end of a line that says "Join the next line and ignore any leading spaces of that line."

      Maybe we can wrap right after <%-? Kinda ugly but... Less invasive.

      1. Review Bot doesn't check line length in JS files, and if it did, there would be a bunch of things that wouldn't pass muster (given that gettext calls can't be wrapped). Given that, I'd prefer to leave this as-is.

    3. Show all issues

      As discovered recently, we'll need to wrap the file in (function() { ... })();

    4. reviewboard/static/rb/js/views/reviewRequestFieldViews.es6.js (Diff revision 3)
       
       
       
       
       
       
       
       
      Show all issues

      This might be faster as:

      this._fieldName = this.fieldID.replace(/_(.)/g, (m, c) => c.toUpperCase());
      

      Assuming you want to strip away all underscores and not just ones precedencing a letter.

    5. reviewboard/static/rb/js/views/reviewRequestFieldViews.es6.js (Diff revision 3)
       
       
       
       
       
       
       
       
      Show all issues

      Missing "Returns"

      This should also go after useEditIconOnly.

    6. Show all issues

      Typo in "Markdown".

    7. 
        
    david
    Review request changed
    Commit:
    a588a33c2be12b1f99a85a2335cdbaeb10ebf8ab
    5dc6c78eddba49221314a452f362df67aadd8c3d

    Checks run (1 failed, 1 succeeded)

    flake8 passed.
    JSHint failed.

    JSHint

    chipx86
    1. Ship It!
    2. 
        
    david
    Review request changed
    Status:
    Completed
    Change Summary:
    Pushed to release-3.0.x (c240e78)