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