[WIP] Checklist Extension

Review Request #4131 — Created May 9, 2013 and discarded

Information

rb-extension-pack

Reviewers

First iteration of Checklist extension

Milestone: Checklist on own page, can add/delete items and check items off
           Changes persist in database

Includes:

Extension configuration files
Django models
Backbone models/views

Remaining work:

Currently hardcoded to request ChecklistInstance with id=1. Need to figure out how to create or retrieve a ChecklistInstance associated with the current Review(Draft)? in progress.
Currently need to hardcode first Checklist / ChecklistInstance in, with the JSON = [] (empty list)

Went to home page, extension page: checklist does not appear
Go to a review request: checklist appears. Should be visible on all review views.
Add items by filling in textfield and hitting enter
Update items by double-clicking their text, editing the text, and hitting enter
Delete item same way as updating, but set text to be empty
Check and uncheck items
Refresh or leave the review, then come back and confirm changes persist.
Drag the dialog around. Scroll the page. Dialog should stay in place.
Attached a screenshot and opened it. Checklist is still visible.
Description From Last Updated

Col: 5 E128 continuation line under-indented for visual indent

reviewbotreviewbot

Col: 1 E302 expected 2 blank lines, found 1

reviewbotreviewbot

Col: 35 W291 trailing whitespace

reviewbotreviewbot

Col: 13 E128 continuation line under-indented for visual indent

reviewbotreviewbot

Col: 1 E302 expected 2 blank lines, found 1

reviewbotreviewbot

Col: 42 W291 trailing whitespace

reviewbotreviewbot

Col: 1 E302 expected 2 blank lines, found 1

reviewbotreviewbot

Col: 42 W291 trailing whitespace

reviewbotreviewbot

Col: 80 E501 line too long (100 > 79 characters)

reviewbotreviewbot

Col: 101 W292 no newline at end of file

reviewbotreviewbot

Col: 1 E302 expected 2 blank lines, found 1

reviewbotreviewbot

Col: 17 E251 no spaces around keyword / parameter equals

reviewbotreviewbot

Col: 19 E251 no spaces around keyword / parameter equals

reviewbotreviewbot

Col: 1 W293 blank line contains whitespace

reviewbotreviewbot

Col: 67 W292 no newline at end of file

reviewbotreviewbot

Col: 1 E302 expected 2 blank lines, found 1

reviewbotreviewbot

Col: 11 W292 no newline at end of file

reviewbotreviewbot

Col: 2 W292 no newline at end of file

reviewbotreviewbot

Col: 1 E302 expected 2 blank lines, found 1

reviewbotreviewbot

Col: 5 W191 indentation contains tabs

reviewbotreviewbot

Col: 5 E101 indentation contains mixed spaces and tabs

reviewbotreviewbot

Col: 2 W292 no newline at end of file

reviewbotreviewbot

Col: 5 E128 continuation line under-indented for visual indent

reviewbotreviewbot

Col: 1 E302 expected 2 blank lines, found 1

reviewbotreviewbot

Col: 35 W291 trailing whitespace

reviewbotreviewbot

Col: 13 E128 continuation line under-indented for visual indent

reviewbotreviewbot

Col: 1 E302 expected 2 blank lines, found 1

reviewbotreviewbot

Col: 42 W291 trailing whitespace

reviewbotreviewbot

Col: 1 E302 expected 2 blank lines, found 1

reviewbotreviewbot

Col: 42 W291 trailing whitespace

reviewbotreviewbot

Col: 80 E501 line too long (100 > 79 characters)

reviewbotreviewbot

Col: 101 W292 no newline at end of file

reviewbotreviewbot

Col: 1 E302 expected 2 blank lines, found 1

reviewbotreviewbot

Col: 17 E251 no spaces around keyword / parameter equals

reviewbotreviewbot

Col: 19 E251 no spaces around keyword / parameter equals

reviewbotreviewbot

Col: 1 W293 blank line contains whitespace

reviewbotreviewbot

Col: 67 W292 no newline at end of file

reviewbotreviewbot

Col: 1 E302 expected 2 blank lines, found 1

reviewbotreviewbot

Col: 11 W292 no newline at end of file

reviewbotreviewbot

Col: 2 W292 no newline at end of file

reviewbotreviewbot

Col: 1 E302 expected 2 blank lines, found 1

reviewbotreviewbot

Col: 5 W191 indentation contains tabs

reviewbotreviewbot

Col: 5 E101 indentation contains mixed spaces and tabs

reviewbotreviewbot

Col: 2 W292 no newline at end of file

reviewbotreviewbot

Col: 5 E128 continuation line under-indented for visual indent

reviewbotreviewbot

Col: 1 E302 expected 2 blank lines, found 1

reviewbotreviewbot

Col: 35 W291 trailing whitespace

reviewbotreviewbot

Col: 13 E128 continuation line under-indented for visual indent

reviewbotreviewbot

Col: 1 E302 expected 2 blank lines, found 1

reviewbotreviewbot

Col: 42 W291 trailing whitespace

reviewbotreviewbot

Col: 1 E302 expected 2 blank lines, found 1

reviewbotreviewbot

Col: 42 W291 trailing whitespace

reviewbotreviewbot

Col: 80 E501 line too long (100 > 79 characters)

reviewbotreviewbot

Col: 101 W292 no newline at end of file

reviewbotreviewbot

Col: 1 E302 expected 2 blank lines, found 1

reviewbotreviewbot

Col: 17 E251 no spaces around keyword / parameter equals

reviewbotreviewbot

Col: 19 E251 no spaces around keyword / parameter equals

reviewbotreviewbot

Col: 1 W293 blank line contains whitespace

reviewbotreviewbot

Col: 67 W292 no newline at end of file

reviewbotreviewbot

Col: 1 E302 expected 2 blank lines, found 1

reviewbotreviewbot

Col: 11 W292 no newline at end of file

reviewbotreviewbot

Col: 2 W292 no newline at end of file

reviewbotreviewbot

Col: 1 E302 expected 2 blank lines, found 1

reviewbotreviewbot

Col: 5 W191 indentation contains tabs

reviewbotreviewbot

Col: 5 E101 indentation contains mixed spaces and tabs

reviewbotreviewbot

Col: 13 E126 continuation line over-indented for hanging indent

reviewbotreviewbot

Col: 9 E128 continuation line under-indented for visual indent

reviewbotreviewbot

4 spaces here.

mike_conleymike_conley

Col: 80 E501 line too long (85 > 79 characters)

reviewbotreviewbot

Break this up at the second comma.

mike_conleymike_conley

Why the massive indents? 4 spaces should be enough (or maybe we do 2 for CSS? I don't remember...look through …

mike_conleymike_conley

What's with the hardcoded ID?

mike_conleymike_conley

Lowercase "f" in fixed.

mike_conleymike_conley

var before pk

mike_conleymike_conley

var before items and max. Semicolon after max = 0.

mike_conleymike_conley

Spaces on either side of the = and <

mike_conleymike_conley

Spaces on either side of the +

mike_conleymike_conley

Spaces on either side of =, <

mike_conleymike_conley

var items

mike_conleymike_conley

Space on either side of = and <

mike_conleymike_conley

var item

mike_conleymike_conley

Not sure if one-liners are desired... might as well put these in curly braces, like this: if (e.keyCode == 13) …

mike_conleymike_conley

This comment doesn't make a whole lot of sense...

mike_conleymike_conley

Switch these two for alphabetical order.

mike_conleymike_conley

Switch these two lines.

mike_conleymike_conley

Put the imports in alphabetical order - so webapi_check_local_site before webapi_check_login_required.

mike_conleymike_conley

This should be with the rest of the djblets.webapi imports.

mike_conleymike_conley

Col: 17 E251 no spaces around keyword / parameter equals

reviewbotreviewbot

Col: 19 E251 no spaces around keyword / parameter equals

reviewbotreviewbot

All of this dead code should be removed.

mike_conleymike_conley

What's eventually going to go here? What kind of configuration will be necessary?

mike_conleymike_conley

Col: 5 E128 continuation line under-indented for visual indent

reviewbotreviewbot

Col: 13 E127 continuation line over-indented for visual indent

reviewbotreviewbot

Col: 13 E126 continuation line over-indented for hanging indent

reviewbotreviewbot

Col: 35 W291 trailing whitespace

reviewbotreviewbot

Col: 48 W291 trailing whitespace

reviewbotreviewbot

Col: 1 E303 too many blank lines (3)

reviewbotreviewbot

Col: 5 E128 continuation line under-indented for visual indent

reviewbotreviewbot

Col: 13 E127 continuation line over-indented for visual indent

reviewbotreviewbot

Col: 13 E126 continuation line over-indented for hanging indent

reviewbotreviewbot

Just curious - why are we making the client responsible for setting the IDs? That sounds like something the database …

mike_conleymike_conley

I think we try to avoid one-liners, so like: if (e.keyCode != 13 || !this.input.val()) { return; }

mike_conleymike_conley

Col: 5 E128 continuation line under-indented for visual indent

reviewbotreviewbot

Col: 7 E128 continuation line under-indented for visual indent

reviewbotreviewbot

Col: 13 E126 continuation line over-indented for hanging indent

reviewbotreviewbot

Col: 5 E128 continuation line under-indented for visual indent

reviewbotreviewbot

Col: 7 E128 continuation line under-indented for visual indent

reviewbotreviewbot

Col: 13 E126 continuation line over-indented for hanging indent

reviewbotreviewbot

Col: 5 E128 continuation line under-indented for visual indent

reviewbotreviewbot

Col: 5 E128 continuation line under-indented for visual indent

reviewbotreviewbot

Col: 13 E126 continuation line over-indented for hanging indent

reviewbotreviewbot
reviewbot
  1. This is a review from Review Bot.
      Tool: PEP8 Style Checker
      Processed Files:
        rbchecklist/rbchecklist/models.py
        rbchecklist/rbchecklist/admin_urls.py
        rbchecklist/rbchecklist/extension.py
        rbchecklist/setup.py
        rbchecklist/rbchecklist/resources.py
        rbchecklist/rbchecklist/templatetags/checklisttags.py
        rbchecklist/rbchecklist/urls.py
        rbchecklist/rbchecklist/views.py
      Ignored Files:
        rbchecklist/rbchecklist/htdocs/js/views/checklistItemView.js
        rbchecklist/rbchecklist/htdocs/js/views/checklistInstanceView.js
        rbchecklist/rbchecklist/htdocs/js/models/checklistItemModel.js
        rbchecklist/rbchecklist/templates/rbchecklist/configure.html
        rbchecklist/rbchecklist/templates/rbchecklist/scripts.html
        rbchecklist/rbchecklist/templates/rbchecklist/base.html
        rbchecklist/rbchecklist/templatetags/checklisttags.pyc
        rbchecklist/rbchecklist/htdocs/js/checklists.js
        rbchecklist/rbchecklist/htdocs/js/models/checklistInstanceModel.js
        rbchecklist/rbchecklist/templatetags/__init__.pyc
        rbchecklist/rbchecklist/templates/rbchecklist/checklist.html
    
    
  2. rbchecklist/rbchecklist/admin_urls.py (Diff revision 1)
     
     
    Col: 5
     E128 continuation line under-indented for visual indent
    
  3. rbchecklist/rbchecklist/extension.py (Diff revision 1)
     
     
    Col: 1
     E302 expected 2 blank lines, found 1
    
  4. rbchecklist/rbchecklist/extension.py (Diff revision 1)
     
     
    Col: 35
     W291 trailing whitespace
    
  5. rbchecklist/rbchecklist/extension.py (Diff revision 1)
     
     
    Col: 13
     E128 continuation line under-indented for visual indent
    
  6. rbchecklist/rbchecklist/models.py (Diff revision 1)
     
     
    Col: 1
     E302 expected 2 blank lines, found 1
    
  7. rbchecklist/rbchecklist/models.py (Diff revision 1)
     
     
    Col: 42
     W291 trailing whitespace
    
  8. rbchecklist/rbchecklist/models.py (Diff revision 1)
     
     
    Col: 1
     E302 expected 2 blank lines, found 1
    
  9. rbchecklist/rbchecklist/models.py (Diff revision 1)
     
     
    Col: 42
     W291 trailing whitespace
    
  10. rbchecklist/rbchecklist/models.py (Diff revision 1)
     
     
    Col: 80
     E501 line too long (100 > 79 characters)
    
  11. rbchecklist/rbchecklist/models.py (Diff revision 1)
     
     
    Col: 101
     W292 no newline at end of file
    
  12. rbchecklist/rbchecklist/resources.py (Diff revision 1)
     
     
    Col: 1
     E302 expected 2 blank lines, found 1
    
  13. rbchecklist/rbchecklist/resources.py (Diff revision 1)
     
     
    Col: 17
     E251 no spaces around keyword / parameter equals
    
  14. rbchecklist/rbchecklist/resources.py (Diff revision 1)
     
     
    Col: 19
     E251 no spaces around keyword / parameter equals
    
  15. rbchecklist/rbchecklist/resources.py (Diff revision 1)
     
     
    Col: 1
     W293 blank line contains whitespace
    
  16. rbchecklist/rbchecklist/resources.py (Diff revision 1)
     
     
    Col: 67
     W292 no newline at end of file
    
  17. Col: 1
     E302 expected 2 blank lines, found 1
    
  18. Col: 11
     W292 no newline at end of file
    
  19. rbchecklist/rbchecklist/urls.py (Diff revision 1)
     
     
    Col: 2
     W292 no newline at end of file
    
  20. rbchecklist/rbchecklist/views.py (Diff revision 1)
     
     
    Col: 1
     E302 expected 2 blank lines, found 1
    
  21. rbchecklist/rbchecklist/views.py (Diff revision 1)
     
     
    Col: 5
     W191 indentation contains tabs
    
  22. rbchecklist/rbchecklist/views.py (Diff revision 1)
     
     
    Col: 5
     E101 indentation contains mixed spaces and tabs
    
  23. rbchecklist/setup.py (Diff revision 1)
     
     
    Col: 2
     W292 no newline at end of file
    
  24. 
      
PH
reviewbot
  1. This is a review from Review Bot.
      Tool: PEP8 Style Checker
      Processed Files:
        rbchecklist/rbchecklist/admin_urls.py
        rbchecklist/rbchecklist/models.py
        rbchecklist/rbchecklist/extension.py
        rbchecklist/setup.py
        rbchecklist/rbchecklist/resources.py
        rbchecklist/rbchecklist/templatetags/checklisttags.py
        rbchecklist/rbchecklist/urls.py
        rbchecklist/rbchecklist/views.py
      Ignored Files:
        rbchecklist/rbchecklist/htdocs/js/views/checklistInstanceView.js
        rbchecklist/rbchecklist/htdocs/js/views/checklistItemView.js
        rbchecklist/rbchecklist/htdocs/js/models/checklistItemModel.js
        rbchecklist/rbchecklist/templates/rbchecklist/configure.html
        rbchecklist/rbchecklist/templates/rbchecklist/scripts.html
        rbchecklist/rbchecklist/templates/rbchecklist/base.html
        rbchecklist/rbchecklist/htdocs/js/checklists.js
        rbchecklist/rbchecklist/htdocs/js/models/checklistInstanceModel.js
        rbchecklist/rbchecklist/templates/rbchecklist/checklist.html
    
    
  2. rbchecklist/rbchecklist/admin_urls.py (Diff revision 2)
     
     
    Col: 5
     E128 continuation line under-indented for visual indent
    
  3. rbchecklist/rbchecklist/extension.py (Diff revision 2)
     
     
    Col: 1
     E302 expected 2 blank lines, found 1
    
  4. rbchecklist/rbchecklist/extension.py (Diff revision 2)
     
     
    Col: 35
     W291 trailing whitespace
    
  5. rbchecklist/rbchecklist/extension.py (Diff revision 2)
     
     
    Col: 13
     E128 continuation line under-indented for visual indent
    
  6. rbchecklist/rbchecklist/models.py (Diff revision 2)
     
     
    Col: 1
     E302 expected 2 blank lines, found 1
    
  7. rbchecklist/rbchecklist/models.py (Diff revision 2)
     
     
    Col: 42
     W291 trailing whitespace
    
  8. rbchecklist/rbchecklist/models.py (Diff revision 2)
     
     
    Col: 1
     E302 expected 2 blank lines, found 1
    
  9. rbchecklist/rbchecklist/models.py (Diff revision 2)
     
     
    Col: 42
     W291 trailing whitespace
    
  10. rbchecklist/rbchecklist/models.py (Diff revision 2)
     
     
    Col: 80
     E501 line too long (100 > 79 characters)
    
  11. rbchecklist/rbchecklist/models.py (Diff revision 2)
     
     
    Col: 101
     W292 no newline at end of file
    
  12. rbchecklist/rbchecklist/resources.py (Diff revision 2)
     
     
    Col: 1
     E302 expected 2 blank lines, found 1
    
  13. rbchecklist/rbchecklist/resources.py (Diff revision 2)
     
     
    Col: 17
     E251 no spaces around keyword / parameter equals
    
  14. rbchecklist/rbchecklist/resources.py (Diff revision 2)
     
     
    Col: 19
     E251 no spaces around keyword / parameter equals
    
  15. rbchecklist/rbchecklist/resources.py (Diff revision 2)
     
     
    Col: 1
     W293 blank line contains whitespace
    
  16. rbchecklist/rbchecklist/resources.py (Diff revision 2)
     
     
    Col: 67
     W292 no newline at end of file
    
  17. Col: 1
     E302 expected 2 blank lines, found 1
    
  18. Col: 11
     W292 no newline at end of file
    
  19. rbchecklist/rbchecklist/urls.py (Diff revision 2)
     
     
    Col: 2
     W292 no newline at end of file
    
  20. rbchecklist/rbchecklist/views.py (Diff revision 2)
     
     
    Col: 1
     E302 expected 2 blank lines, found 1
    
  21. rbchecklist/rbchecklist/views.py (Diff revision 2)
     
     
    Col: 5
     W191 indentation contains tabs
    
  22. rbchecklist/rbchecklist/views.py (Diff revision 2)
     
     
    Col: 5
     E101 indentation contains mixed spaces and tabs
    
  23. rbchecklist/setup.py (Diff revision 2)
     
     
    Col: 2
     W292 no newline at end of file
    
  24. 
      
PH
PH
reviewbot
  1. This is a review from Review Bot.
      Tool: PEP8 Style Checker
      Processed Files:
        rbchecklist/rbchecklist/models.py
        rbchecklist/rbchecklist/admin_urls.py
        rbchecklist/rbchecklist/extension.py
        rbchecklist/setup.py
        rbchecklist/rbchecklist/resources.py
        rbchecklist/rbchecklist/templatetags/checklisttags.py
        rbchecklist/rbchecklist/urls.py
        rbchecklist/rbchecklist/views.py
      Ignored Files:
        rbchecklist/rbchecklist/htdocs/js/views/checklistItemView.js
        rbchecklist/rbchecklist/htdocs/js/views/checklistInstanceView.js
        rbchecklist/rbchecklist/htdocs/js/models/checklistItemModel.js
        rbchecklist/rbchecklist/templates/rbchecklist/configure.html
        rbchecklist/rbchecklist/templates/rbchecklist/scripts.html
        rbchecklist/rbchecklist/templates/rbchecklist/base.html
        rbchecklist/rbchecklist/htdocs/js/checklists.js
        rbchecklist/rbchecklist/htdocs/js/models/checklistInstanceModel.js
        rbchecklist/rbchecklist/templates/rbchecklist/checklist.html
    
    
  2. rbchecklist/rbchecklist/admin_urls.py (Diff revision 3)
     
     
    Col: 5
     E128 continuation line under-indented for visual indent
    
  3. rbchecklist/rbchecklist/extension.py (Diff revision 3)
     
     
    Col: 1
     E302 expected 2 blank lines, found 1
    
  4. rbchecklist/rbchecklist/extension.py (Diff revision 3)
     
     
    Col: 35
     W291 trailing whitespace
    
  5. rbchecklist/rbchecklist/extension.py (Diff revision 3)
     
     
    Col: 13
     E128 continuation line under-indented for visual indent
    
  6. rbchecklist/rbchecklist/models.py (Diff revision 3)
     
     
    Col: 1
     E302 expected 2 blank lines, found 1
    
  7. rbchecklist/rbchecklist/models.py (Diff revision 3)
     
     
    Col: 42
     W291 trailing whitespace
    
  8. rbchecklist/rbchecklist/models.py (Diff revision 3)
     
     
    Col: 1
     E302 expected 2 blank lines, found 1
    
  9. rbchecklist/rbchecklist/models.py (Diff revision 3)
     
     
    Col: 42
     W291 trailing whitespace
    
  10. rbchecklist/rbchecklist/models.py (Diff revision 3)
     
     
    Col: 80
     E501 line too long (100 > 79 characters)
    
  11. rbchecklist/rbchecklist/models.py (Diff revision 3)
     
     
    Col: 101
     W292 no newline at end of file
    
  12. rbchecklist/rbchecklist/resources.py (Diff revision 3)
     
     
    Col: 1
     E302 expected 2 blank lines, found 1
    
  13. rbchecklist/rbchecklist/resources.py (Diff revision 3)
     
     
    Col: 17
     E251 no spaces around keyword / parameter equals
    
  14. rbchecklist/rbchecklist/resources.py (Diff revision 3)
     
     
    Col: 19
     E251 no spaces around keyword / parameter equals
    
  15. rbchecklist/rbchecklist/resources.py (Diff revision 3)
     
     
    Col: 1
     W293 blank line contains whitespace
    
  16. rbchecklist/rbchecklist/resources.py (Diff revision 3)
     
     
    Col: 67
     W292 no newline at end of file
    
  17. Col: 1
     E302 expected 2 blank lines, found 1
    
  18. Col: 11
     W292 no newline at end of file
    
  19. rbchecklist/rbchecklist/urls.py (Diff revision 3)
     
     
    Col: 2
     W292 no newline at end of file
    
  20. rbchecklist/rbchecklist/views.py (Diff revision 3)
     
     
    Col: 1
     E302 expected 2 blank lines, found 1
    
  21. rbchecklist/rbchecklist/views.py (Diff revision 3)
     
     
    Col: 5
     W191 indentation contains tabs
    
  22. rbchecklist/rbchecklist/views.py (Diff revision 3)
     
     
    Col: 5
     E101 indentation contains mixed spaces and tabs
    
  23. rbchecklist/setup.py (Diff revision 3)
     
     
    Col: 13
     E126 continuation line over-indented for hanging indent
    
  24. 
      
PH
PH
reviewbot
  1. This is a review from Review Bot.
      Tool: PEP8 Style Checker
      Processed Files:
        rbchecklist/rbchecklist/admin_urls.py
        rbchecklist/rbchecklist/models.py
        rbchecklist/rbchecklist/extension.py
        rbchecklist/setup.py
        rbchecklist/rbchecklist/resources.py
        rbchecklist/rbchecklist/templatetags/checklisttags.py
        rbchecklist/rbchecklist/urls.py
        rbchecklist/rbchecklist/views.py
      Ignored Files:
        rbchecklist/rbchecklist/htdocs/js/views/checklistItemView.js
        rbchecklist/rbchecklist/htdocs/js/views/checklistInstanceView.js
        rbchecklist/rbchecklist/htdocs/js/models/checklistItemModel.js
        rbchecklist/rbchecklist/templates/rbchecklist/configure.html
        rbchecklist/rbchecklist/htdocs/css/checklists.css
        rbchecklist/rbchecklist/htdocs/js/checklists.js
        rbchecklist/rbchecklist/htdocs/js/models/checklistInstanceModel.js
    
    
  2. rbchecklist/rbchecklist/admin_urls.py (Diff revision 4)
     
     
    Col: 9
     E128 continuation line under-indented for visual indent
    
  3. rbchecklist/rbchecklist/extension.py (Diff revision 4)
     
     
    Col: 80
     E501 line too long (85 > 79 characters)
    
  4. rbchecklist/rbchecklist/resources.py (Diff revision 4)
     
     
    Col: 17
     E251 no spaces around keyword / parameter equals
    
  5. rbchecklist/rbchecklist/resources.py (Diff revision 4)
     
     
    Col: 19
     E251 no spaces around keyword / parameter equals
    
  6. rbchecklist/rbchecklist/urls.py (Diff revision 4)
     
     
    Col: 5
     E128 continuation line under-indented for visual indent
    
  7. rbchecklist/rbchecklist/urls.py (Diff revision 4)
     
     
    Col: 13
     E127 continuation line over-indented for visual indent
    
  8. rbchecklist/setup.py (Diff revision 4)
     
     
    Col: 13
     E126 continuation line over-indented for hanging indent
    
  9. 
      
PH
mike_conley
  1. Hey Felix,
    
    This looks pretty good. Please address these issues, along with the ones ReviewBot picked up (or drop the ones that don't make sense, along with an explanation on why you dropped it).
    
    You also need to fill in the "Testing Done" field of this review request. Tell us what scenarios you've tested.
    
    I've also not tried the extension, but I've seen the video...is the checklist available while viewing diffs, screenshots and other binary files?
    
    -Mike
    
    1. Filled in the testing section.
      
      The checklist is visible as long as the global variable gReviewRequest is active, so it's available while viewing screenshots, diffs, and binary files.
  2. rbchecklist/rbchecklist/admin_urls.py (Diff revision 4)
     
     
    4 spaces here.
  3. rbchecklist/rbchecklist/extension.py (Diff revision 4)
     
     
    Break this up at the second comma.
  4. rbchecklist/rbchecklist/htdocs/css/checklists.css (Diff revision 4)
     
     
     
     
     
     
     
     
     
     
    Why the massive indents? 4 spaces should be enough (or maybe we do 2 for CSS? I don't remember...look through some of our Review Board CSS for the correct format).
  5. What's with the hardcoded ID?
  6. Lowercase "f" in fixed.
  7. var before items and max. Semicolon after max = 0.
  8. Spaces on either side of the = and <
  9. Spaces on either side of the +
  10. Spaces on either side of =, <
  11. Space on either side of = and <
  12. Why not || these?
  13. Not sure if one-liners are desired... might as well put these in curly braces, like this:
    
    if (e.keyCode == 13) {
      this.close();
    }
  14. rbchecklist/rbchecklist/models.py (Diff revision 4)
     
     
    This comment doesn't make a whole lot of sense...
  15. rbchecklist/rbchecklist/models.py (Diff revision 4)
     
     
    These two lines of documentation aren't really clear... should this be:
    
    Checklist items for this template are stored as a JSON string in the following format:
  16. rbchecklist/rbchecklist/resources.py (Diff revision 4)
     
     
     
    Switch these two for alphabetical order.
  17. rbchecklist/rbchecklist/resources.py (Diff revision 4)
     
     
     
    Switch these two lines.
  18. rbchecklist/rbchecklist/resources.py (Diff revision 4)
     
     
     
    Put the imports in alphabetical order - so webapi_check_local_site before webapi_check_login_required.
  19. rbchecklist/rbchecklist/resources.py (Diff revision 4)
     
     
     
    This should be with the rest of the djblets.webapi imports.
  20. rbchecklist/rbchecklist/resources.py (Diff revision 4)
     
     
     
     
     
     
     
     
     
     
     
     
     
    All of this dead code should be removed.
  21. What's eventually going to go here? What kind of configuration will be necessary?
    1. Removed this, don't actually have anything in mind.
  22. 
      
PH
PH
PH
reviewbot
  1. This is a review from Review Bot.
      Tool: PEP8 Style Checker
      Processed Files:
        rbchecklist/rbchecklist/models.py
        rbchecklist/setup.py
        rbchecklist/rbchecklist/extension.py
        rbchecklist/rbchecklist/urls.py
        rbchecklist/rbchecklist/resources.py
      Ignored Files:
        rbchecklist/rbchecklist/htdocs/js/views/checklistInstanceView.js
        rbchecklist/rbchecklist/htdocs/js/views/checklistItemView.js
        rbchecklist/rbchecklist/htdocs/js/models/checklistItemModel.js
        rbchecklist/rbchecklist/htdocs/js/checklists.js
        rbchecklist/rbchecklist/htdocs/css/checklists.css
        rbchecklist/rbchecklist/htdocs/js/models/checklistInstanceModel.js
        rbchecklist/rbchecklist/templates/rbchecklist/checklist_scripts.html
    
    
  2. rbchecklist/rbchecklist/extension.py (Diff revision 5)
     
     
    Col: 35
     W291 trailing whitespace
    
  3. rbchecklist/rbchecklist/extension.py (Diff revision 5)
     
     
    Col: 48
     W291 trailing whitespace
    
  4. rbchecklist/rbchecklist/resources.py (Diff revision 5)
     
     
    Col: 1
     E303 too many blank lines (3)
    
  5. rbchecklist/rbchecklist/urls.py (Diff revision 5)
     
     
    Col: 5
     E128 continuation line under-indented for visual indent
    
  6. rbchecklist/rbchecklist/urls.py (Diff revision 5)
     
     
    Col: 13
     E127 continuation line over-indented for visual indent
    
  7. rbchecklist/setup.py (Diff revision 5)
     
     
    Col: 13
     E126 continuation line over-indented for hanging indent
    
  8. 
      
PH
reviewbot
  1. This is a review from Review Bot.
      Tool: PEP8 Style Checker
      Processed Files:
        rbchecklist/rbchecklist/models.py
        rbchecklist/setup.py
        rbchecklist/rbchecklist/extension.py
        rbchecklist/rbchecklist/urls.py
        rbchecklist/rbchecklist/resources.py
      Ignored Files:
        rbchecklist/rbchecklist/htdocs/js/views/checklistInstanceView.js
        rbchecklist/rbchecklist/htdocs/js/views/checklistItemView.js
        rbchecklist/rbchecklist/htdocs/js/models/checklistItemModel.js
        rbchecklist/rbchecklist/htdocs/js/checklists.js
        rbchecklist/rbchecklist/htdocs/css/checklists.css
        rbchecklist/rbchecklist/htdocs/js/models/checklistInstanceModel.js
        rbchecklist/rbchecklist/templates/rbchecklist/checklist_scripts.html
    
    
  2. rbchecklist/rbchecklist/urls.py (Diff revision 6)
     
     
    Col: 5
     E128 continuation line under-indented for visual indent
    
  3. rbchecklist/rbchecklist/urls.py (Diff revision 6)
     
     
    Col: 7
     E128 continuation line under-indented for visual indent
    
  4. rbchecklist/setup.py (Diff revision 6)
     
     
    Col: 13
     E126 continuation line over-indented for hanging indent
    
  5. 
      
mike_conley
  1. Hey Felix,
    
    Looking really good - just two more points during this last pass.
    
    -Mike
  2. Just curious - why are we making the client responsible for setting the IDs? That sounds like something the database could / should probably take care of automatically, no?
    1. This is the item's index within the checklist. All the items in a checklist are bundled into a JSON string, and the JSON string is what is sent to the server, so the database won't be able to see the item or set an ID.
      
      The column "template_json" in the database is storing something like '[{id:1, text:"This is a checklist item"}, {id:2, text:"This is another checklist item"}]'
    2. Ah, ok - feel free to drop this one.
      
      Another question though - it seems a bit strange to recalculate that value each time an item is added. Can't we calculate it once and cache it?
      
      -Mike
    3. You're right. I rewrote ChecklistInstance model to store the maxIndex and use it when adding items.
  3. I think we try to avoid one-liners, so like:
    
    if (e.keyCode != 13 || !this.input.val()) {
      return;
    }
  4. 
      
PH
reviewbot
  1. This is a review from Review Bot.
      Tool: PEP8 Style Checker
      Processed Files:
        rbchecklist/rbchecklist/models.py
        rbchecklist/setup.py
        rbchecklist/rbchecklist/extension.py
        rbchecklist/rbchecklist/urls.py
        rbchecklist/rbchecklist/resources.py
      Ignored Files:
        rbchecklist/rbchecklist/htdocs/js/views/checklistInstanceView.js
        rbchecklist/rbchecklist/htdocs/js/views/checklistItemView.js
        rbchecklist/rbchecklist/htdocs/js/models/checklistItemModel.js
        rbchecklist/rbchecklist/htdocs/js/checklists.js
        rbchecklist/rbchecklist/htdocs/css/checklists.css
        rbchecklist/rbchecklist/htdocs/js/models/checklistInstanceModel.js
        rbchecklist/rbchecklist/templates/rbchecklist/checklist_scripts.html
    
    
  2. rbchecklist/rbchecklist/urls.py (Diff revision 7)
     
     
    Col: 5
     E128 continuation line under-indented for visual indent
    
  3. rbchecklist/rbchecklist/urls.py (Diff revision 7)
     
     
    Col: 7
     E128 continuation line under-indented for visual indent
    
  4. rbchecklist/setup.py (Diff revision 7)
     
     
    Col: 13
     E126 continuation line over-indented for hanging indent
    
  5. 
      
PH
reviewbot
  1. This is a review from Review Bot.
      Tool: PEP8 Style Checker
      Processed Files:
        rbchecklist/rbchecklist/models.py
        rbchecklist/setup.py
        rbchecklist/rbchecklist/extension.py
        rbchecklist/rbchecklist/urls.py
        rbchecklist/rbchecklist/resources.py
      Ignored Files:
        rbchecklist/rbchecklist/htdocs/js/views/checklistInstanceView.js
        rbchecklist/rbchecklist/htdocs/js/views/checklistItemView.js
        rbchecklist/rbchecklist/htdocs/js/models/checklistItemModel.js
        rbchecklist/rbchecklist/htdocs/js/checklists.js
        rbchecklist/rbchecklist/htdocs/css/checklists.css
        rbchecklist/rbchecklist/htdocs/js/models/checklistInstanceModel.js
        rbchecklist/rbchecklist/templates/rbchecklist/checklist_scripts.html
    
    
  2. rbchecklist/rbchecklist/urls.py (Diff revision 8)
     
     
    Col: 5
     E128 continuation line under-indented for visual indent
    
  3. rbchecklist/rbchecklist/urls.py (Diff revision 8)
     
     
    Col: 5
     E128 continuation line under-indented for visual indent
    
  4. rbchecklist/setup.py (Diff revision 8)
     
     
    Col: 13
     E126 continuation line over-indented for hanging indent
    
  5. 
      
PH
Review request changed

Status: Discarded

Loading...