search

elight / acts_as_commentable_with_threading

Similar to acts_as_commentable; however, utilizes awesome_nested_set to provide threaded comments
MIT License
671 stars 168 forks source link

Feature/nested destruction #52

Open niklas opened 11 years ago

niklas commented 11 years ago

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:

1) not all comments are deleted: only the roots; direct children, grand children are not touched (see spec in attached PR) 2) 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.

petergoldstein commented 10 years ago

@niklas Can you rebase this branch and we'll take a look? Thanks.

niklas commented 10 years ago

rebased & force-pushed. Sorry for the delay.

elight commented 10 years ago

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 notifications@github.com 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

niklas commented 10 years ago

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.

elight commented 10 years ago

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 notifications@github.com 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

niklas commented 10 years ago

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?

elight commented 10 years ago

Oops didn't notice this was a FAILING test. ;-)

On Thu, Feb 6, 2014 at 2:20 PM, Niklas H notifications@github.com 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

elight commented 10 years ago

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 notifications@github.com 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

elight commented 10 years ago

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 notifications@github.com 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

niklas commented 10 years ago

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.

jrasanen commented 9 years ago

What's the status with this pull request? :)