search

innoq / statuses

statuses
Apache License 2.0
13 stars 14 forks source link

allow deletion of last conversation post (#59) #60

Closed johnswanson closed 11 years ago

johnswanson commented 11 years ago

I was inspired by http://aheusingfeld.github.io/posts/2013/08/06/getting-started-with-clojure/ -- I've been playing with Clojure for a while but wanted to do something more real and collaborative. Very open to feedback from more experienced clojure devs, so let me know if anything needs fixing. Thanks!

aheusingfeld commented 11 years ago

Hi John,

thanks for the pull-request, this looks pretty good IMHO. There is just a small issue: When we remove the post from the conversation, we need to check whether there is only one post left in the conversation. In this case we don't have a conversation anymore and therefore have to delete the conversation and the reference to the conversation in the remaining post.

Would you mind fixing this and updating your pull-request with a second commit? Thanks in advance.

Cheers Alex

johnswanson commented 11 years ago

Ah, good point--a conversation of one doesn't make much sense. Fixed it--thanks!

aheusingfeld commented 11 years ago

Hi John,

first of all thanks for the update, the fix works well!! I did some extensive testing today and unfortunately spotted two points which I'd consider minor bugs:

  1. You can't delete a reply1 to post1 if you already send reply2 to post1. You can only delete reply1 if you first delete reply2 (see the following screenshot for further explanation: ) screenshot-statuses59-bug1
  2. The check for "Is this post the last post of the conversation" neglects the user.

To fix this I'd suggest we check whether the current user's post's id occurs in any other post's in-reply-to attribute. If it doesn't, we can offer the option to delete it. How do you think about it?

Thanks again for all your effort!

johnswanson commented 11 years ago

Hey Alex, that's a much better way to go about it--my mental model of "conversations" was incorrect, since I was looking them as vectors in the DB without thinking about how the vector was actually representing a flattened tree. Thanks for your help, just added another commit!

aheusingfeld commented 11 years ago

Great submission! thanks, John!

@stilkov @pschirmacher I'd like to merge it! Any objections?

stilkov commented 11 years ago

No objections. Thanks John!