• 
      

    [checklist] Refactoring of checklist

    Review Request #7176 — Created April 7, 2015 and submitted

    Information

    rb-extension-pack
    2124492...

    Reviewers

    Refactoring includes

    • Use of request.user instead of passing user_id in every HTTP request
    • Updating of the view only after successful API response
    • Renamed status of each item from finished to checked
    • Use of ChecklistItem non-database resource API instead of using HTTP request parameters
    • Moving of front-end API logic from view to model
    • CSS hyphens everywhere instead of underscores
    • Improving Python code (renaming variables, removing redundant code)
    • Start checklist item IDs from 0 instead of 1.

    Basic CRUD functionality remains. Ensured that the views will not update unless there is a successful response from the server.

    Description From Last Updated

    'resources' imported but unused

    reviewbotreviewbot

    Col: 25 E261 at least two spaces before inline comment

    reviewbotreviewbot

    local variable 'item' is assigned to but never used

    reviewbotreviewbot

    'webapi_response_errors' imported but unused

    reviewbotreviewbot

    'ChecklistItem' imported but unused

    reviewbotreviewbot

    Col: 53 E261 at least two spaces before inline comment

    reviewbotreviewbot

    The first line of this file should be: from __future__ import unicode_literals

    brenniebrennie

    Can you put these into alphabetical order?

    brenniebrennie

    Undo this change.

    brenniebrennie

    The first line of this file should be: from __future__ import unicode_literals

    brenniebrennie

    Undo this change. django, djblets, and reviewboard are all third party imports with repsect to rb-extension-pack.

    brenniebrennie

    Docstrings should be of the following form: """Single line summary Multi-line description. """

    brenniebrennie

    If we're going to put a code example in the docstring, we should be using a restructured text code block.

    brenniebrennie

    "A JSON blob"

    brenniebrennie

    This should probably be six.text_type (from django.utils.six), not str. If this should be a byte string type, use six.binary_type instead.

    brenniebrennie

    Same comment about six.text_type vs str applies here.

    brenniebrennie

    You don't need pass if the class has a docstring.

    brenniebrennie

    Same comment here about unicode_literals.

    brenniebrennie

    Same comment about unicode_literals as above.

    brenniebrennie

    Blank line between these.

    brenniebrennie

    Shouldn't this be int ? The model indicates that ids are integers.

    brenniebrennie

    Missing trailing comma.

    brenniebrennie

    This should be six.text_type.

    brenniebrennie

    Missing trailing comma.

    brenniebrennie

    Missing trailing comma.

    brenniebrennie

    Missing trailing comma.

    brenniebrennie

    Can you catch CheckListItem.DoesNotExist here instead?

    brenniebrennie

    six.text_type, not str

    brenniebrennie

    six.text_type

    brenniebrennie

    Can you catch CheckListItem.DoesNotExist here instead?

    brenniebrennie

    six.text_type

    brenniebrennie

    Should have a trailing comma.

    brenniebrennie

    Trailing comma

    brenniebrennie

    Trailing comma.

    brenniebrennie

    Can you catch CheckListItem.DoesNotExist here instead?

    brenniebrennie

    You can add checklist_item_id as a paramter to the function and just refer to it that way. Its cleaner than …

    brenniebrennie

    This shoudl have from __future__ import unicode_literals

    brenniebrennie

    "ID"

    brenniebrennie

    Is this left over from debugging?

    brenniebrennie

    This should be formatted the same as a Python docstring. This applies to all methods.

    brenniebrennie

    Can we just use response.checklist_item.keys() here?

    brenniebrennie

    I feel like since we're using this all over the file, and we always have access to the Checklist object, …

    brenniebrennie

    "The"

    brenniebrennie

    review_request_id shouldn't be quoted.

    brenniebrennie

    How about _addItemToView ?

    brenniebrennie

    I think it would be ok to include the keys we expect in there (id, checked, description ?).

    brenniebrennie

    Won't this start indexing from 1? Shouldn't we index from 0?

    brenniebrennie

    I don't believe you should be using a dummy model for the web API. Models are not required.

    brenniebrennie

    This will end up in public-facing documentation.

    brenniebrennie

    We don't really need this.

    brenniebrennie

    Can we put a doc comment on this?

    brenniebrennie

    Doc comments should be of the form /* * Single line summary. * * Multi-line description. */ Same elsewhere.

    brenniebrennie

    Needs a doc comment.

    brenniebrennie

    Needs long-form doc comment.

    brenniebrennie

    It technically isn't a dictionary -- its a JS object. You could call it a mapping, instead.

    brenniebrennie

    Needs doc comment.

    brenniebrennie

    Needs doc comment.

    brenniebrennie

    Should use long form doc comment. Same elsewhere.

    brenniebrennie

    If you are referring to Python data types, this is actually a six.text_type.

    brenniebrennie

    Same here.

    brenniebrennie

    Can you fix up this docstring?

    brenniebrennie
    SU
    reviewbot
    1. Tool: PEP8 Style Checker
      Processed Files:
          checklist/checklist/models.py
          checklist/checklist/resources/checklist_item.py
          checklist/checklist/extension.py
          checklist/checklist/resources/__init__.py
          checklist/checklist/resources/checklist_resource.py
      
      Ignored Files:
          checklist/checklist/static/js/views/checklistView.js
          checklist/checklist/static/js/models/checklistAPI.js
          checklist/checklist/static/css/style.less
          checklist/checklist/static/js/models/checklist.js
          checklist/checklist/templates/checklist/template.html
      
      
      
      Tool: Pyflakes
      Processed Files:
          checklist/checklist/models.py
          checklist/checklist/resources/checklist_item.py
          checklist/checklist/extension.py
          checklist/checklist/resources/__init__.py
          checklist/checklist/resources/checklist_resource.py
      
      Ignored Files:
          checklist/checklist/static/js/views/checklistView.js
          checklist/checklist/static/js/models/checklistAPI.js
          checklist/checklist/static/css/style.less
          checklist/checklist/static/js/models/checklist.js
          checklist/checklist/templates/checklist/template.html
      
      
    2. Show all issues
       'resources' imported but unused
      
    3. Show all issues
      Col: 25
       E261 at least two spaces before inline comment
      
    4. Show all issues
       local variable 'item' is assigned to but never used
      
    5. Show all issues
       'webapi_response_errors' imported but unused
      
    6. Show all issues
       'ChecklistItem' imported but unused
      
    7. Show all issues
      Col: 53
       E261 at least two spaces before inline comment
      
    8. 
        
    SU
    reviewbot
    1. Tool: Pyflakes
      Processed Files:
          checklist/checklist/models.py
          checklist/checklist/resources/checklist_item.py
          checklist/checklist/extension.py
          checklist/checklist/resources/__init__.py
          checklist/checklist/resources/checklist_resource.py
      
      Ignored Files:
          checklist/checklist/static/js/views/checklistView.js
          checklist/checklist/static/js/models/checklistAPI.js
          checklist/checklist/static/css/style.less
          checklist/checklist/static/js/models/checklist.js
          checklist/checklist/templates/checklist/template.html
      
      
      
      Tool: PEP8 Style Checker
      Processed Files:
          checklist/checklist/models.py
          checklist/checklist/resources/checklist_item.py
          checklist/checklist/extension.py
          checklist/checklist/resources/__init__.py
          checklist/checklist/resources/checklist_resource.py
      
      Ignored Files:
          checklist/checklist/static/js/views/checklistView.js
          checklist/checklist/static/js/models/checklistAPI.js
          checklist/checklist/static/css/style.less
          checklist/checklist/static/js/models/checklist.js
          checklist/checklist/templates/checklist/template.html
      
      
    2. 
        
    brennie
    1. <p>I'm confused. The model has the id fields for the checklist items as integers, but you convert them to strings all over the place. Why aren't we just using integers?</p>

      1. Sorry, the comment in the model is incorrect, checklist item IDs should be strings at the moment. It was already a str when I took over this project. I plan on keeping the string type for IDs especially for nested items. For example, id: '2_1' will mean that it's an item 1 nested under an item 2. Alternatively, IDs as integers work too, but I will not rule out the possibility of using strings yet.

      2. Ah! This makes sense. Strings will work fine in this case.

    2. checklist/checklist/extension.py (Diff revision 3)
       
       
      Show all issues

      The first line of this file should be:

      from __future__ import unicode_literals

    3. checklist/checklist/extension.py (Diff revision 3)
       
       
      Show all issues

      Can you put these into alphabetical order?

    4. checklist/checklist/extension.py (Diff revision 3)
       
       
      Show all issues

      Undo this change.

    5. checklist/checklist/models.py (Diff revision 3)
       
       
      Show all issues

      The first line of this file should be:

      from __future__ import unicode_literals

    6. checklist/checklist/models.py (Diff revision 3)
       
       
      Show all issues

      Undo this change. django, djblets, and reviewboard are all third party imports with repsect to rb-extension-pack.

    7. checklist/checklist/models.py (Diff revision 3)
       
       
       
       
       
       
       
       
       
       
       
       
       
      Show all issues

      Docstrings should be of the following form:

      """Single line summary

      Multi-line description.
      """

    8. checklist/checklist/models.py (Diff revision 3)
       
       
       
       
       
       
       
       
       
      Show all issues

      If we're going to put a code example in the docstring, we should be using a restructured text code block.

    9. checklist/checklist/models.py (Diff revision 3)
       
       
      Show all issues

      "A JSON blob"

    10. checklist/checklist/models.py (Diff revision 3)
       
       
       
      Show all issues

      This should probably be six.text_type (from django.utils.six), not str. If this should be a byte string type, use six.binary_type instead.

    11. checklist/checklist/models.py (Diff revision 3)
       
       
       
      Show all issues

      Same comment about six.text_type vs str applies here.

    12. checklist/checklist/models.py (Diff revision 3)
       
       
      Show all issues

      You don't need pass if the class has a docstring.

    13. Show all issues

      Same comment here about unicode_literals.

    14. Show all issues

      Same comment about unicode_literals as above.

    15. Show all issues

      Blank line between these.

    16. Show all issues

      Shouldn't this be int ?

      The model indicates that ids are integers.

    17. Show all issues

      Missing trailing comma.

    18. Show all issues

      This should be six.text_type.

    19. Show all issues

      Missing trailing comma.

    20. Show all issues

      Missing trailing comma.

    21. Show all issues

      Missing trailing comma.

    22. Show all issues

      Can you catch CheckListItem.DoesNotExist here instead?

      1. Just using this comment as a reminder (checklist item? hah) that this should probably be Checklist.DoesNotExist.

    23. Show all issues

      six.text_type, not str

    24. Show all issues

      six.text_type

    25. Show all issues

      Can you catch CheckListItem.DoesNotExist here instead?

    26. Show all issues

      six.text_type

    27. Show all issues

      Should have a trailing comma.

    28. Show all issues

      Trailing comma

    29. Show all issues

      Trailing comma.

    30. Show all issues

      Can you catch CheckListItem.DoesNotExist here instead?

    31. Show all issues

      You can add checklist_item_id as a paramter to the function and just refer to it that way. Its cleaner than indexing into kwargs. Same elsewhere.

    32. Show all issues

      This shoudl have from __future__ import unicode_literals

    33. Show all issues

      "ID"

    34. Show all issues

      Is this left over from debugging?

      1. I meant for this to be an assertion, although it does not check if this.collection is null, which sometimes causes the statement to error instead of being asserted. Apart from that, is the assertion being used correctly in this case?

      2. If its sometimes causing with an error with this.collection being null, that should probably be checked, too.

        I'm not sure if we leave asserts in our client JS code; you should probably ask David or Christian about this.

        If this does get left in, make sure to move the var statement above it; the actual assignment can be done below it.

      3. I will assume that asserts are allowed in client JS code for now.

    35. Show all issues

      This should be formatted the same as a Python docstring. This applies to all methods.

    36. Show all issues

      Can we just use response.checklist_item.keys() here?

      1. The map function returns the values instead of keys. Which also means I can use _.values(response.checklist_item) from Underscore.js. /reminder

    37. Show all issues

      I feel like since we're using this all over the file, and we always have access to the Checklist object, that this should be a constant on the parent Checklist object.

    38. Show all issues

      "The"

    39. Show all issues

      review_request_id shouldn't be quoted.

    40. Show all issues

      How about _addItemToView ?

    41. 
        
    SU
    reviewbot
    1. Tool: Pyflakes
      Processed Files:
          checklist/checklist/models.py
          checklist/checklist/resources/checklist_item.py
          checklist/checklist/extension.py
          checklist/checklist/resources/__init__.py
          checklist/checklist/resources/checklist_resource.py
      
      Ignored Files:
          checklist/checklist/static/js/views/checklistView.js
          checklist/checklist/static/js/models/checklistAPI.js
          checklist/checklist/static/css/style.less
          checklist/checklist/static/js/models/checklist.js
          checklist/checklist/templates/checklist/template.html
      
      
      
      Tool: PEP8 Style Checker
      Processed Files:
          checklist/checklist/models.py
          checklist/checklist/resources/checklist_item.py
          checklist/checklist/extension.py
          checklist/checklist/resources/__init__.py
          checklist/checklist/resources/checklist_resource.py
      
      Ignored Files:
          checklist/checklist/static/js/views/checklistView.js
          checklist/checklist/static/js/models/checklistAPI.js
          checklist/checklist/static/css/style.less
          checklist/checklist/static/js/models/checklist.js
          checklist/checklist/templates/checklist/template.html
      
      
    2. 
        
    SU
    reviewbot
    1. Tool: PEP8 Style Checker
      Processed Files:
          checklist/checklist/models.py
          checklist/checklist/resources/checklist_item.py
          checklist/checklist/extension.py
          checklist/checklist/resources/__init__.py
          checklist/checklist/resources/checklist_resource.py
      
      Ignored Files:
          checklist/checklist/static/js/views/checklistView.js
          checklist/checklist/static/js/models/checklistAPI.js
          checklist/checklist/static/css/style.less
          checklist/checklist/static/js/models/checklist.js
          checklist/checklist/templates/checklist/template.html
      
      
      
      Tool: Pyflakes
      Processed Files:
          checklist/checklist/models.py
          checklist/checklist/resources/checklist_item.py
          checklist/checklist/extension.py
          checklist/checklist/resources/__init__.py
          checklist/checklist/resources/checklist_resource.py
      
      Ignored Files:
          checklist/checklist/static/js/views/checklistView.js
          checklist/checklist/static/js/models/checklistAPI.js
          checklist/checklist/static/css/style.less
          checklist/checklist/static/js/models/checklist.js
          checklist/checklist/templates/checklist/template.html
      
      
    2. 
        
    brennie
    1. 
        
    2. checklist/checklist/models.py (Diff revision 5)
       
       
      Show all issues

      I think it would be ok to include the keys we expect in there (id, checked, description ?).

    3. checklist/checklist/models.py (Diff revision 5)
       
       
       
       
       
       
       
       
      Show all issues

      Won't this start indexing from 1? Shouldn't we index from 0?

    4. checklist/checklist/models.py (Diff revision 5)
       
       
       
      Show all issues

      I don't believe you should be using a dummy model for the web API. Models are not required.

    5. Show all issues

      This will end up in public-facing documentation.

    6. Show all issues

      We don't really need this.

    7. Show all issues

      Can we put a doc comment on this?

    8. Show all issues

      Doc comments should be of the form

      /*
       * Single line summary.
       * 
       * Multi-line description.
       */
      

      Same elsewhere.

    9. Show all issues

      Needs a doc comment.

    10. Show all issues

      Needs long-form doc comment.

    11. Show all issues

      It technically isn't a dictionary -- its a JS object. You could call it a mapping, instead.

    12. Show all issues

      Needs doc comment.

    13. Show all issues

      Needs doc comment.

    14. Show all issues

      Should use long form doc comment. Same elsewhere.

    15. 
        
    SU
    reviewbot
    1. Tool: Pyflakes
      Processed Files:
          checklist/checklist/models.py
          checklist/checklist/resources/checklist_item.py
          checklist/checklist/extension.py
          checklist/checklist/resources/__init__.py
          checklist/checklist/resources/checklist_resource.py
      
      Ignored Files:
          checklist/checklist/static/js/views/checklistView.js
          checklist/checklist/static/js/models/checklistAPI.js
          checklist/checklist/static/css/style.less
          checklist/checklist/static/js/models/checklist.js
          checklist/checklist/templates/checklist/template.html
      
      
      
      Tool: PEP8 Style Checker
      Processed Files:
          checklist/checklist/models.py
          checklist/checklist/resources/checklist_item.py
          checklist/checklist/extension.py
          checklist/checklist/resources/__init__.py
          checklist/checklist/resources/checklist_resource.py
      
      Ignored Files:
          checklist/checklist/static/js/views/checklistView.js
          checklist/checklist/static/js/models/checklistAPI.js
          checklist/checklist/static/css/style.less
          checklist/checklist/static/js/models/checklist.js
          checklist/checklist/templates/checklist/template.html
      
      
    2. 
        
    brennie
    1. 
        
    2. checklist/checklist/models.py (Diff revision 6)
       
       
      Show all issues

      If you are referring to Python data types, this is actually a six.text_type.

    3. checklist/checklist/models.py (Diff revision 6)
       
       
      Show all issues

      Same here.

    4. 
        
    SU
    reviewbot
    1. Tool: Pyflakes
      Processed Files:
          checklist/checklist/models.py
          checklist/checklist/resources/checklist_item.py
          checklist/checklist/extension.py
          checklist/checklist/resources/__init__.py
          checklist/checklist/resources/checklist_resource.py
      
      Ignored Files:
          checklist/checklist/static/js/views/checklistView.js
          checklist/checklist/static/js/models/checklistAPI.js
          checklist/checklist/static/css/style.less
          checklist/checklist/static/js/models/checklist.js
          checklist/checklist/templates/checklist/template.html
      
      
      
      Tool: PEP8 Style Checker
      Processed Files:
          checklist/checklist/models.py
          checklist/checklist/resources/checklist_item.py
          checklist/checklist/extension.py
          checklist/checklist/resources/__init__.py
          checklist/checklist/resources/checklist_resource.py
      
      Ignored Files:
          checklist/checklist/static/js/views/checklistView.js
          checklist/checklist/static/js/models/checklistAPI.js
          checklist/checklist/static/css/style.less
          checklist/checklist/static/js/models/checklist.js
          checklist/checklist/templates/checklist/template.html
      
      
    2. 
        
    brennie
    1. 
        
    2. checklist/checklist/models.py (Diff revision 7)
       
       
       
       
      Show all issues

      Can you fix up this docstring?

    3. 
        
    SU
    reviewbot
    1. Tool: Pyflakes
      Processed Files:
          checklist/checklist/models.py
          checklist/checklist/resources/checklist_item.py
          checklist/checklist/extension.py
          checklist/checklist/resources/__init__.py
          checklist/checklist/resources/checklist_resource.py
      
      Ignored Files:
          checklist/checklist/static/js/views/checklistView.js
          checklist/checklist/static/js/models/checklistAPI.js
          checklist/checklist/static/css/style.less
          checklist/checklist/static/js/models/checklist.js
          checklist/checklist/templates/checklist/template.html
      
      
      
      Tool: PEP8 Style Checker
      Processed Files:
          checklist/checklist/models.py
          checklist/checklist/resources/checklist_item.py
          checklist/checklist/extension.py
          checklist/checklist/resources/__init__.py
          checklist/checklist/resources/checklist_resource.py
      
      Ignored Files:
          checklist/checklist/static/js/views/checklistView.js
          checklist/checklist/static/js/models/checklistAPI.js
          checklist/checklist/static/css/style.less
          checklist/checklist/static/js/models/checklist.js
          checklist/checklist/templates/checklist/template.html
      
      
    2. 
        
    david
    1. Ship It!
    2. 
        
    SU
    Review request changed
    Status:
    Completed
    Change Summary:
    Pushed to master (27afa78)