[checklist] Refactoring of checklist

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

Wang Jun Sun
rb-extension-pack
7099
7248
2124492...
rb-extension-pack, students

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.

  • 0
  • 0
  • 60
  • 1
  • 61
Description From Last Updated
Wang Jun Sun
Review Bot
  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.  'resources' imported but unused
    
  3. Col: 25
     E261 at least two spaces before inline comment
    
  4.  local variable 'item' is assigned to but never used
    
  5.  'webapi_response_errors' imported but unused
    
  6.  'ChecklistItem' imported but unused
    
  7. Col: 53
     E261 at least two spaces before inline comment
    
  8. 
      
Wang Jun Sun
Review Bot
  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. 
      
Barret Rennie
  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)
     
     

    The first line of this file should be:

    from __future__ import unicode_literals

  3. checklist/checklist/extension.py (Diff revision 3)
     
     

    Can you put these into alphabetical order?

  4. checklist/checklist/extension.py (Diff revision 3)
     
     

    Undo this change.

  5. checklist/checklist/models.py (Diff revision 3)
     
     

    The first line of this file should be:

    from __future__ import unicode_literals

  6. checklist/checklist/models.py (Diff revision 3)
     
     

    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)
     
     
     
     
     
     
     
     
     
     
     
     
     

    Docstrings should be of the following form:

    """Single line summary

    Multi-line description.
    """

  8. checklist/checklist/models.py (Diff revision 3)
     
     
     
     
     
     
     
     
     

    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)
     
     

    "A JSON blob"

  10. checklist/checklist/models.py (Diff revision 3)
     
     
     

    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)
     
     
     

    Same comment about six.text_type vs str applies here.

  12. checklist/checklist/models.py (Diff revision 3)
     
     

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

  13. Same comment here about unicode_literals.

  14. Same comment about unicode_literals as above.

  15. Blank line between these.

  16. Shouldn't this be int ?

    The model indicates that ids are integers.

  17. Missing trailing comma.

  18. This should be six.text_type.

  19. Missing trailing comma.

  20. Missing trailing comma.

  21. Missing trailing comma.

  22. 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. six.text_type, not str

  24. Can you catch CheckListItem.DoesNotExist here instead?

  25. Should have a trailing comma.

  26. Trailing comma

  27. Trailing comma.

  28. Can you catch CheckListItem.DoesNotExist here instead?

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

  30. This shoudl have from __future__ import unicode_literals

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

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

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

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

  35. review_request_id shouldn't be quoted.

  36. How about _addItemToView ?

  37. 
      
Wang Jun Sun
Review Bot
  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. 
      
Wang Jun Sun
Review Bot
  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. 
      
Barret Rennie
  1. 
      
  2. checklist/checklist/models.py (Diff revision 5)
     
     

    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)
     
     
     
     
     
     
     
     

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

  4. checklist/checklist/models.py (Diff revision 5)
     
     
     

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

  5. This will end up in public-facing documentation.

  6. We don't really need this.

  7. Can we put a doc comment on this?

  8. Doc comments should be of the form

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

    Same elsewhere.

  9. Needs a doc comment.

  10. Needs long-form doc comment.

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

  12. Needs doc comment.

  13. Needs doc comment.

  14. Should use long form doc comment. Same elsewhere.

  15. 
      
Wang Jun Sun
Review Bot
  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. 
      
Barret Rennie
  1. 
      
  2. checklist/checklist/models.py (Diff revision 6)
     
     

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

  3. checklist/checklist/models.py (Diff revision 6)
     
     

    Same here.

  4. 
      
Wang Jun Sun
Review Bot
  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. 
      
Barret Rennie
  1. 
      
  2. checklist/checklist/models.py (Diff revision 7)
     
     
     
     

    Can you fix up this docstring?

  3. 
      
Wang Jun Sun
Review Bot
  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 Trowbridge
  1. Ship It!
  2. 
      
Wang Jun Sun
Review request changed

Status: Closed (submitted)

Change Summary:

Pushed to master (27afa78)
Loading...