Summary

[checklist] Allow single level nesting of checklist items

Review Request #7248 — Created April 24, 2015 and discarded Oct. 26, 2021, 12:16 p.m.

Information

Owner

Sunxperous

Repository

rb-extension-pack

Branch

master

Bugs

Depends On

~~7176~~

Blocks

7960

Reviewers

Groups

rb-extension-pack, students

People

Description

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.

Testing Done

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.

Files

Issues

Description	From	Last Updated
Can you indent the nested items a bit more? This is a bit too subtle.	david	Aug. 23, 2015, 10:08 a.m.
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 …	brennie	April 25, 2015, 11:28 a.m.
See previous formatting comment. Since we're doing this in two places, should we just have a helper method to generate …	brennie	April 25, 2015, 11:28 a.m.
This should be else. You shouldn't do is comparisons with strings and '' is false-y. This also doesn't handle the …	brennie	April 25, 2015, 11:28 a.m.
Since you are returning a mutable object ([]), this must be a function that returns a new default object each …	brennie	April 25, 2015, 11:28 a.m.
Needs doc comment.	brennie	April 25, 2015, 11:28 a.m.
Needs doc comment.	brennie	April 25, 2015, 11:28 a.m.
Can this be formatted as: this.$el .children('.checklist-content') .find('.checklist-item-move') .show()	brennie	April 25, 2015, 11:28 a.m.
Can we do: this.$el .children('.checklist-content') .find('.checklist-item-move') .hide() It is easier to read when formatted like this.	brennie	April 25, 2015, 11:28 a.m.
Needs doc comment.	brennie	April 25, 2015, 11:28 a.m.
Make sure to remove commented code from final version.	brennie	April 25, 2015, 11:28 a.m.
Needs a doc comment.	brennie	April 25, 2015, 11:28 a.m.
Make sure to remove commented code from final version.	brennie	April 25, 2015, 11:28 a.m.
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	May 1, 2015, 12:58 p.m.
This should just use a for loop, rather than sticking it in a list comprehension.	david	Aug. 23, 2015, 10:11 a.m.
The r prefix isn't necessary for this because it doesn't have any characters.	david	Aug. 23, 2015, 9:58 a.m.
Blank line between these.	brennie	Aug. 30, 2015, 7:46 p.m.
This should be item.startswith(parent_id). It will end up being faster than checking for all possible substring placements of parent_id in …	brennie	Aug. 30, 2015, 7:46 p.m.
This method should ensure that nest_under isn't a second level element (i.e., '_' not in nest_under). If so, it should …	brennie	Aug. 31, 2015, 9:21 a.m.
Should use two backquotes for reStructured Text code tags.	brennie	Aug. 30, 2015, 7:50 p.m.
No blank line here.	brennie	Aug. 30, 2015, 7:50 p.m.
``checklist_item_id``	brennie	Aug. 30, 2015, 7:50 p.m.
The exception I mentioned should be caught here. When that occurs, it should return an INVALID_FORM_DATA response	brennie	Aug. 31, 2015, 9:06 a.m.
Same here.	brennie	Aug. 31, 2015, 9:07 a.m.
I think Djblets has a .transition() mixin. If it does not, drop this and related issues.	brennie	Aug. 30, 2015, 10:36 p.m.
Same here.	brennie	Aug. 30, 2015, 10:36 p.m.
This should use a .transform() mixin. if Djblets doesnt define one, drop this issue.	brennie	Aug. 30, 2015, 10:36 p.m.
Same here.	brennie	Aug. 30, 2015, 10:36 p.m.
This is pretty important and I feel it should be in the doccomment for the function.	brennie	Aug. 30, 2015, 8:01 p.m.
Same here	brennie	Aug. 30, 2015, 8:02 p.m.
Blank line here.	brennie	Aug. 30, 2015, 8:03 p.m.
Double quotes preferred here.	brennie	Aug. 30, 2015, 8:03 p.m.
You are doing item.get('id') twice. let's break that out into an itemID variable.	brennie	Aug. 30, 2015, 8:04 p.m.
Given that we're pulling out itemID, can we rename this to childIDStart or childStartPos ?	brennie	Aug. 30, 2015, 8:06 p.m.

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

checklist/checklist/models.py (Diff revision 1)

The issue has been resolved. 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.

checklist/checklist/models.py (Diff revision 1)

The issue has been resolved. 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?

checklist/checklist/models.py (Diff revision 1)

The issue has been resolved. 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.

checklist/checklist/static/js/models/checklist.js (Diff revision 1)

The issue has been resolved. Show all issues

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

checklist/checklist/static/js/views/checklistView.js (Diff revision 1)
The issue has been resolved. Show all issues
```
Needs doc comment.
```
checklist/checklist/static/js/views/checklistView.js (Diff revision 1)
The issue has been resolved. Show all issues
```
Needs doc comment.
```

checklist/checklist/static/js/views/checklistView.js (Diff revision 1)

The issue has been resolved. Show all issues

Can this be formatted as:

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

checklist/checklist/static/js/views/checklistView.js (Diff revision 1)

The issue has been resolved. 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.

checklist/checklist/static/js/views/checklistView.js (Diff revision 1)
The issue has been resolved. Show all issues
```
Needs doc comment.
```

checklist/checklist/static/js/views/checklistView.js (Diff revision 1)

The issue has been resolved. Show all issues

Make sure to remove commented code from final version.

checklist/checklist/static/js/views/checklistView.js (Diff revision 1)
The issue has been resolved. Show all issues
```
Needs a doc comment.
```

checklist/checklist/static/js/views/checklistView.js (Diff revision 1)

The issue has been resolved. Show all issues

Make sure to remove commented code from final version.

Change Summary:

Reduce the scope of this review request to just nesting of checklist items.
Now fully functional nesting of items.
Touch up docs and comments.
Improve multi-line statements.

Summary:

[WIP] [checklist] Integrate checklist templates into main checklist

[WIP] [checklist] Allow single level nesting of checklist items

Description:

~		Implement single level nested checklist items.
	~	Implement single level nested checklist items.
		Checklist items can now be dragged into another for nesting.

		Why is this a WIP?
~		Certain features are still in progress, and templates cannot be imported yet.
	~	UI still requires minor touching up.
-		UI also requires minor touching up.

Testing Done:

~		nil, there is always something broken at the moment
	~	Basic CRUD of checklist items remain.
	+
	+	Dragging a checklist item into another nests it, and 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.
	+
	+	When a top level checklist item contains no nested items, it is draggable.
	+
	+	Ensured that checklist item states persist after a refresh.

Commit:

881d4c41dcb4d20752c901758e9393dd7e1b0617

dfeb1f8806dabed3cab213364db59691f63e1425

Diff:

Revision 2 (+294 -56)

Show changes

	checklist/checklist/models.py
	checklist/checklist/resources/checklist_item.py
	checklist/checklist/static/css/style.less
	checklist/checklist/static/js/models/checklist.js
	checklist/checklist/static/js/views/checklistView.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



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

checklist/checklist/models.py (Diff revision 2)

The issue has been resolved. 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
]

Change Summary:

Touch up UI, fix some code

Description:

~		Implement single level nested checklist items.
	~	Implements single level nested checklist items in preparation for integration of checklist templates.
-		Checklist items can now be dragged into another for nesting.

~		Why is this a WIP?
~		UI still requires minor touching up.
	~	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.
	+
	+	Why is this still a WIP?
	+	Checking a parent item should check and uncheck all nested items, and vice versa.
	+	Allow minimizing of nested checklist items within parent checklist items.

Testing Done:

~		Basic CRUD of checklist items remain.
	~	Basic CRUD of non-parent checklist items remain.

~		Dragging a checklist item into another nests it, and the parent checklist item cannot be dragged.
	~	Ensured that CRUD functionality of parent checklist items is as expected - nested items are not affected, except when the parent is deleted.

~		A nested checklist item can also be dragged into another top level checklist item, or back to the main checklist.
	~	Ensured proper draggable and droppable functionality depending on the state of each checklist item.

~		When a top level checklist item contains no nested items, it is draggable.
	~	Also ensured that checklist item states persist after a refresh, for new items, parent items and child items.
-
-		Ensured that checklist item states persist after a refresh.

Branch:

master

Commit:

dfeb1f8806dabed3cab213364db59691f63e1425

9024ac0fe419aeef3a1cf64adcc5868aab72f247

Diff:

Revision 3 (+358 -49)

Show changes

	checklist/checklist/models.py
	checklist/checklist/resources/checklist_item.py
	checklist/checklist/static/css/style.less
	checklist/checklist/static/js/models/checklist.js
	checklist/checklist/static/js/views/checklistView.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



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

Change Summary:

Allow minimizing of nested checklist items within parents

Summary:

[WIP] [checklist] Allow single level nesting of checklist items

[checklist] Allow single level nesting of checklist items

Description:

		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.

~		Why is this still a WIP?
	~	Nested checklist items can be minimized within the parent checklist item.
-		Checking a parent item should check and uncheck all nested items, and vice versa.
-		Allow minimizing of nested checklist items within parent checklist items.

Testing Done:

		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 functionality depending on the state of each checklist item.
	~	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.

Commit:

9024ac0fe419aeef3a1cf64adcc5868aab72f247

b5a9cdbfd7a5ca0f50f8d5f28c5c592a642a6c4e

Diff:

Revision 4 (+433 -62)

Show changes

	checklist/checklist/models.py
	checklist/checklist/resources/checklist_item.py
	checklist/checklist/static/css/style.less
	checklist/checklist/static/js/models/checklist.js
	checklist/checklist/static/js/views/checklistView.js

Added Files:

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

Change Summary:

Prevent minimized nested items from appearing when editing parent description.

Commit:

b5a9cdbfd7a5ca0f50f8d5f28c5c592a642a6c4e

a1a9c6490ab8c43fb124ab0794e9ce0a02b6cdd5

Diff:

Revision 5 (+419 -61)

Show changes

	checklist/checklist/models.py
	checklist/checklist/resources/checklist_item.py
	checklist/checklist/static/css/style.less
	checklist/checklist/static/js/models/checklist.js
	checklist/checklist/static/js/views/checklistView.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



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

Single level nesting (Revision 1)
The issue has been resolved. Show all issues
```
Can you indent the nested items a bit more? This is a bit too subtle.
```
1. SU
  
  Sunxperous
  
  Aug. 23, 2015, 10:17 a.m.
  Added 10px.
2. brennie
  
  Aug. 23, 2015, 10 p.m.
  Can you upload a new screenshot?
3. SU
  
  Sunxperous
  
  Aug. 23, 2015, 10:15 p.m.
  Uploaded!

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

The issue has been resolved. Show all issues

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

checklist/checklist/resources/checklist_item.py (Diff revision 5)
The issue has been resolved. Show all issues
```
The r prefix isn't necessary for this because it doesn't have any  characters.
```

Change Summary:

Add more indentation to nested items, and fix minor code syntax.

Commit:

a1a9c6490ab8c43fb124ab0794e9ce0a02b6cdd5

5a969009d563585bfcb9fa8f233411a697b21fd5

Diff:

Revision 6 (+417 -61)

Show changes

	checklist/checklist/models.py
	checklist/checklist/resources/checklist_item.py
	checklist/checklist/static/css/style.less
	checklist/checklist/static/js/models/checklist.js
	checklist/checklist/static/js/views/checklistView.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



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

Change Summary:: New screenshot!
Added Files:

checklist/checklist/models.py (Diff revision 6)
The issue has been resolved. Show all issues
```
Blank line between these.
```

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

The issue has been resolved. 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.

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

The issue has been resolved. 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.

checklist/checklist/models.py (Diff revision 6)
The issue has been resolved. Show all issues
```
Should use two backquotes for reStructured Text code tags.
```
checklist/checklist/models.py (Diff revision 6)
The issue has been resolved. Show all issues
```
No blank line here.
```
checklist/checklist/resources/checklist_item.py (Diff revision 6)
The issue has been resolved. Show all issues
```
``checklist_item_id``
```

checklist/checklist/resources/checklist_item.py (Diff revision 6)

The issue has been resolved. Show all issues

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

checklist/checklist/resources/checklist_item.py (Diff revision 6)
The issue has been resolved. Show all issues
```
Same here.
```

checklist/checklist/static/css/style.less (Diff revision 6)

The issue has been resolved. Show all issues

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

checklist/checklist/static/css/style.less (Diff revision 6)
The issue has been resolved. Show all issues
```
Same here.
```

checklist/checklist/static/css/style.less (Diff revision 6)

The issue has been resolved. Show all issues

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

checklist/checklist/static/css/style.less (Diff revision 6)
The issue has been resolved. Show all issues
```
Same here.
```
checklist/checklist/static/js/models/checklist.js (Diff revision 6)
The issue has been resolved. Show all issues
```
This is pretty important and I feel it should be in the doccomment for the function.
```
checklist/checklist/static/js/models/checklist.js (Diff revision 6)
The issue has been resolved. Show all issues
```
Same here
```
checklist/checklist/static/js/models/checklist.js (Diff revision 6)
The issue has been resolved. Show all issues
```
Blank line here.
```
checklist/checklist/static/js/views/checklistView.js (Diff revision 6)
The issue has been resolved. Show all issues
```
Double quotes preferred here.
```

checklist/checklist/static/js/views/checklistView.js (Diff revision 6)

The issue has been resolved. Show all issues

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

checklist/checklist/static/js/views/checklistView.js (Diff revision 6)
The issue has been resolved. Show all issues
```
Given that we're pulling out itemID, can we rename this to childIDStart or childStartPos ?
```

Change Summary:

Resolved Brennie's issues and slightly reduced width of nested items by 4px.

Commit:

5a969009d563585bfcb9fa8f233411a697b21fd5

b0f24fc3f77c6b3dcbefb77a8f038a9b8209e16f

Diff:

Revision 7 (+435 -62)

Show changes

	checklist/checklist/models.py
	checklist/checklist/resources/checklist_item.py
	checklist/checklist/static/css/style.less
	checklist/checklist/static/js/models/checklist.js
	checklist/checklist/static/js/views/checklistView.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



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

Status:: Discarded