elight
Feature/nested destruction
The fix introduced in #38 for destruction of nested comments does not work for us. Two problems persist when deleting a record with a nested structure of comments:
- not all comments are deleted: only the roots; direct children, grand children are not touched (see spec in attached PR)
- with same structure, but creating comments through
Comment.build_from(... parent: abc): AACWT still tries to delete a non-existing comment
case 2 I failed to reproduce outside of our app. Are you interested?
We have to continue to use the "fix" in #32.
Nitpick but could you move the two method definitions up to the top of the describe block? They're referenced in the code before they're defined.
Also, suggest renaming "answer_to" to "create_reply_to" as a Comment is being created.
I always get antsy when a test isn't understandable without opening another file or even reading another part of the same file (with exception to the behavior under test). Keeping tests painfully simple is the only way I know for them to be effective documentation.
On Thu, Feb 6, 2014 at 6:06 AM, Niklas H [email protected] wrote:
rebased & force-pushed. Sorry for the delay.
Reply to this email directly or view it on GitHub: https://github.com/elight/acts_as_commentable_with_threading/pull/52#issuecomment-34317001
For me the custom methods usually come after the standard rspec blocks to keep surprises away, but tastes differ. I am not a native.en speaker, so reply and answer were synonymous for me.
Will fix both.
Thanks, Nilklas! I only meant my remarks as constructive criticism. I'm grateful for your submission!
I agree Rspec can make these things awkward. Describe blocks are really anonymous classes but Rspec hides that by being DSLy. If I had to do it over again, I'd use MiniTest.
On Thu, Feb 6, 2014 at 12:29 PM, Niklas H [email protected] wrote:
For me the custom methods usually come after the standard rspec blocks to keep surprises away, but tastes differ. I am not a native.en speaker, so reply and answer were synonymous for me.
Will fix both.
Reply to this email directly or view it on GitHub: https://github.com/elight/acts_as_commentable_with_threading/pull/52#issuecomment-34354206
I did not take any offense ;-) I like Rspec because it is DSLy (TM) - I could not write the above test cleaner in any other existing test framework. But this may be me :cat:
Do you have an idea why the deep deletion does not hit all the comments?
Oops didn't notice this was a FAILING test. ;-)
On Thu, Feb 6, 2014 at 2:20 PM, Niklas H [email protected] wrote:
I did not take any offense ;-) I like Rspec because it is DSLy (TM) - I could not write the above test cleaner in any other existing test framework. But this may be me :cat:
Do you have an idea why the deep deletion does not hit all the comments?
Reply to this email directly or view it on GitHub: https://github.com/elight/acts_as_commentable_with_threading/pull/52#issuecomment-34366200
Huh. before_destroy is destroying the root. I believe the Comment should instead be recursively destroying its children through the before_destroy.
On Thu, Feb 6, 2014 at 2:20 PM, Niklas H [email protected] wrote:
I did not take any offense ;-) I like Rspec because it is DSLy (TM) - I could not write the above test cleaner in any other existing test framework. But this may be me :cat:
Do you have an idea why the deep deletion does not hit all the comments?
Reply to this email directly or view it on GitHub: https://github.com/elight/acts_as_commentable_with_threading/pull/52#issuecomment-34366200
That said, I don't recall if my initial intent was to prevent the destruction of subtrees and instead reparent the children to the parent of the deleted node. Though that wouldn't work if you deleted the root...
From seeing other usages of threaded comments (see Reddit), I'd be inclined to instead delete the comment body and mark the Comment (not the thread) as deleted but leaving the Comment otherwise intact. This retains the structure of the tree without the hassle of worrying about reparenting.
If we did this, it could be an API breaking change for some and so a major release.
Thoughts?
/cc @petergoldstein
On Thu, Feb 6, 2014 at 2:20 PM, Niklas H [email protected] wrote:
I did not take any offense ;-) I like Rspec because it is DSLy (TM) - I could not write the above test cleaner in any other existing test framework. But this may be me :cat:
Do you have an idea why the deep deletion does not hit all the comments?
Reply to this email directly or view it on GitHub: https://github.com/elight/acts_as_commentable_with_threading/pull/52#issuecomment-34366200
While marking a Comment as deleted and leaving its children intact is a nice feature to have, I think destroying it should actually clear them from the database, with all its replies.
For example. as an admin I want to get rid of spam - no need to keep the replies.
