[checklist] Allow single level nesting of checklist items

Review Request #7248 — Created April 24, 2015 and discarded

Information

rb-extension-pack
master

Reviewers

Implements single level nested checklist items in preparation for integration of checklist templates.

Checklist items can now be dragged into another for nesting. Order of checklist items is only updated and maintained upon nesting, while rearranging items within the list is not supported yet.

Dragging a checklist item into another nests it, and then the parent checklist item cannot be dragged. A nested checklist item can also be dragged into another top level checklist item, or back to the main checklist.

Once a top level checklist item contains no nested items, it becomes draggable. Deleting a top level checklist item deletes the nested items as well.

Nested checklist items can be minimized within the parent checklist item.

Basic CRUD of non-parent checklist items remain.

Ensured that CRUD functionality of parent checklist items is as expected - nested items are not affected, except when the parent is deleted.

Ensured proper draggable and droppable, and minimizing functionality depending on the state of each checklist item.

Also ensured that checklist item states persist after a refresh, for new items, parent items and child items.


Description From Last Updated

Can you indent the nested items a bit more? This is a bit too subtle.

daviddavid

Can this be formatted as: new_item_id = ('%s_%s' % (nest_under, parent_item['next_item_id'])) You don't have to cast an int to a …

brenniebrennie

See previous formatting comment. Since we're doing this in two places, should we just have a helper method to generate …

brenniebrennie

This should be else. You shouldn't do is comparisons with strings and '' is false-y. This also doesn't handle the …

brenniebrennie

Since you are returning a mutable object ([]), this must be a function that returns a new default object each …

brenniebrennie

Needs doc comment.

brenniebrennie

Needs doc comment.

brenniebrennie

Can this be formatted as: this.$el .children('.checklist-content') .find('.checklist-item-move') .show()

brenniebrennie

Can we do: this.$el .children('.checklist-content') .find('.checklist-item-move') .hide() It is easier to read when formatted like this.

brenniebrennie

Needs doc comment.

brenniebrennie

Make sure to remove commented code from final version.

brenniebrennie

Needs a doc comment.

brenniebrennie

Make sure to remove commented code from final version.

brenniebrennie

Use the preferred list comprehension syntax: [ self.checklist_items.pop(item) for item in list(self.checklist_items.keys()) if parent_id in item ]

SU Sunxperous

This should just use a for loop, rather than sticking it in a list comprehension.

daviddavid

The r prefix isn't necessary for this because it doesn't have any characters.

daviddavid

Blank line between these.

brenniebrennie

This should be item.startswith(parent_id). It will end up being faster than checking for all possible substring placements of parent_id in …

brenniebrennie

This method should ensure that nest_under isn't a second level element (i.e., '_' not in nest_under). If so, it should …

brenniebrennie

Should use two backquotes for reStructured Text code tags.

brenniebrennie

No blank line here.

brenniebrennie

``checklist_item_id``

brenniebrennie

The exception I mentioned should be caught here. When that occurs, it should return an INVALID_FORM_DATA response

brenniebrennie

Same here.

brenniebrennie

I think Djblets has a .transition() mixin. If it does not, drop this and related issues.

brenniebrennie

Same here.

brenniebrennie

This should use a .transform() mixin. if Djblets doesnt define one, drop this issue.

brenniebrennie

Same here.

brenniebrennie

This is pretty important and I feel it should be in the doccomment for the function.

brenniebrennie

Same here

brenniebrennie

Blank line here.

brenniebrennie

Double quotes preferred here.

brenniebrennie

You are doing item.get('id') twice. let's break that out into an itemID variable.

brenniebrennie

Given that we're pulling out itemID, can we rename this to childIDStart or childStartPos ?

brenniebrennie
reviewbot
  1. Tool: PEP8 Style Checker
    Processed Files:
        checklist/checklist/models.py
        checklist/checklist/resources/checklist_item.py
    
    Ignored Files:
        checklist/checklist/static/js/views/checklistView.js
        checklist/checklist/static/css/style.less
        checklist/checklist/static/js/models/checklist.js
    
    
    
    Tool: Pyflakes
    Processed Files:
        checklist/checklist/models.py
        checklist/checklist/resources/checklist_item.py
    
    Ignored Files:
        checklist/checklist/static/js/views/checklistView.js
        checklist/checklist/static/css/style.less
        checklist/checklist/static/js/models/checklist.js
    
    
  2. 
      
brennie
  1. 
      
  2. checklist/checklist/models.py (Diff revision 1)
     
     
     
     
     
    Show all issues

    Can this be formatted as:

                new_item_id = ('%s_%s'
                               % (nest_under, parent_item['next_item_id']))
    

    You don't have to cast an int to a six.text_type to use the %s formatter; it will automatically convert it for you.

    Also, make sure to use single quotes for strings.

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

    See previous formatting comment.

    Since we're doing this in two places, should we just have a helper method to generate the next item id?

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

    This should be else.

    You shouldn't do is comparisons with strings and '' is false-y. This also doesn't handle the case for when nest_under is None.

  5. checklist/checklist/static/js/models/checklist.js (Diff revision 1)
     
     
     
     
     
     
     
     
    Show all issues

    Since you are returning a mutable object ([]), this must be a function that returns a new default object each time.

  6. Show all issues

    Needs doc comment.

  7. Show all issues

    Needs doc comment.

  8. Show all issues

    Can this be formatted as:

    this.$el
         .children('.checklist-content')
         .find('.checklist-item-move')
         .show()
    
  9. Show all issues

    Can we do:

    this.$el
        .children('.checklist-content')
        .find('.checklist-item-move')
        .hide()
    

    It is easier to read when formatted like this.

  10. Show all issues

    Needs doc comment.

  11. checklist/checklist/static/js/views/checklistView.js (Diff revision 1)
     
     
     
     
     
     
     
     
    Show all issues

    Make sure to remove commented code from final version.

  12. Show all issues

    Needs a doc comment.

  13. checklist/checklist/static/js/views/checklistView.js (Diff revision 1)
     
     
     
     
     
     
    Show all issues

    Make sure to remove commented code from final version.

  14. 
      
SU
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        checklist/checklist/models.py
        checklist/checklist/resources/checklist_item.py
    
    Ignored Files:
        checklist/checklist/static/js/views/checklistView.js
        checklist/checklist/static/css/style.less
        checklist/checklist/static/js/models/checklist.js
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        checklist/checklist/models.py
        checklist/checklist/resources/checklist_item.py
    
    Ignored Files:
        checklist/checklist/static/js/views/checklistView.js
        checklist/checklist/static/css/style.less
        checklist/checklist/static/js/models/checklist.js
    
    
  2. 
      
SU
  1. 
      
  2. checklist/checklist/models.py (Diff revision 2)
     
     
     
    Show all issues

    Use the preferred list comprehension syntax:

    [
        self.checklist_items.pop(item)
        for item in list(self.checklist_items.keys())
        if parent_id in item
    ]
    
  3. 
      
SU
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        checklist/checklist/models.py
        checklist/checklist/resources/checklist_item.py
    
    Ignored Files:
        checklist/checklist/static/js/views/checklistView.js
        checklist/checklist/static/css/style.less
        checklist/checklist/static/js/models/checklist.js
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        checklist/checklist/models.py
        checklist/checklist/resources/checklist_item.py
    
    Ignored Files:
        checklist/checklist/static/js/views/checklistView.js
        checklist/checklist/static/css/style.less
        checklist/checklist/static/js/models/checklist.js
    
    
  2. 
      
SU
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        checklist/checklist/models.py
        checklist/checklist/resources/checklist_item.py
    
    Ignored Files:
        checklist/checklist/static/js/views/checklistView.js
        checklist/checklist/static/css/style.less
        checklist/checklist/static/js/models/checklist.js
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        checklist/checklist/models.py
        checklist/checklist/resources/checklist_item.py
    
    Ignored Files:
        checklist/checklist/static/js/views/checklistView.js
        checklist/checklist/static/css/style.less
        checklist/checklist/static/js/models/checklist.js
    
    
  2. 
      
SU
reviewbot
  1. Tool: PEP8 Style Checker
    Processed Files:
        checklist/checklist/models.py
        checklist/checklist/resources/checklist_item.py
    
    Ignored Files:
        checklist/checklist/static/js/views/checklistView.js
        checklist/checklist/static/css/style.less
        checklist/checklist/static/js/models/checklist.js
    
    
    
    Tool: Pyflakes
    Processed Files:
        checklist/checklist/models.py
        checklist/checklist/resources/checklist_item.py
    
    Ignored Files:
        checklist/checklist/static/js/views/checklistView.js
        checklist/checklist/static/css/style.less
        checklist/checklist/static/js/models/checklist.js
    
    
  2. 
      
david
  1. 
      
  2. Show all issues

    Can you indent the nested items a bit more? This is a bit too subtle.

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

    This should just use a for loop, rather than sticking it in a list comprehension.

  4. Show all issues

    The r prefix isn't necessary for this because it doesn't have any characters.

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

    Blank line between these.

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

    This should be item.startswith(parent_id). It will end up being faster than checking for all possible substring placements of parent_id in item.

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

    This method should ensure that nest_under isn't a second level element (i.e., '_' not in nest_under). If so, it should raise an exception.

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

    Should use two backquotes for reStructured Text code tags.

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

    No blank line here.

  7. Show all issues

    ``checklist_item_id``
    
  8. Show all issues

    The exception I mentioned should be caught here. When that occurs, it should return an INVALID_FORM_DATA response

  9. Show all issues

    Same here.

  10. checklist/checklist/static/css/style.less (Diff revision 6)
     
     
     
    Show all issues

    I think Djblets has a .transition() mixin. If it does not, drop this and related issues.

  11. checklist/checklist/static/css/style.less (Diff revision 6)
     
     
     
    Show all issues

    Same here.

  12. checklist/checklist/static/css/style.less (Diff revision 6)
     
     
     
     
    Show all issues

    This should use a .transform() mixin. if Djblets doesnt define one, drop this issue.

  13. checklist/checklist/static/css/style.less (Diff revision 6)
     
     
     
    Show all issues

    Same here.

  14. Show all issues

    This is pretty important and I feel it should be in the doccomment for the function.

  15. Show all issues

    Same here

  16. Show all issues

    Blank line here.

  17. Show all issues

    Double quotes preferred here.

  18. checklist/checklist/static/js/views/checklistView.js (Diff revision 6)
     
     
     
     
     
     
     
     
     
     
     
    Show all issues

    You are doing item.get('id') twice. let's break that out into an itemID variable.

  19. Show all issues

    Given that we're pulling out itemID, can we rename this to childIDStart or childStartPos ?

  20. 
      
SU
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        checklist/checklist/models.py
        checklist/checklist/resources/checklist_item.py
    
    Ignored Files:
        checklist/checklist/static/js/views/checklistView.js
        checklist/checklist/static/css/style.less
        checklist/checklist/static/js/models/checklist.js
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        checklist/checklist/models.py
        checklist/checklist/resources/checklist_item.py
    
    Ignored Files:
        checklist/checklist/static/js/views/checklistView.js
        checklist/checklist/static/css/style.less
        checklist/checklist/static/js/models/checklist.js
    
    
  2. 
      
david
Review request changed
Status:
Discarded