psybuzz / goldenconversations

0 stars 0 forks source link

First pass at implementing archiving functionality #43

Closed nikilselvam closed 9 years ago

nikilselvam commented 9 years ago

This PR looks at implementing 'archive conversation' functionality. The first commit sets up the front-end logic for archiving and links in a POST request to allow the front-end logic to trigger the back-end logic. The second commit is my first pass at filling out the back-end logic.

The back-end logic needs some more work for the functionality to be complete. @psybuzz would you mind taking a look and helping me out with this part in particular? I wrote the parts that I could in exports.archive of routes/conversation.js using exports.leave of the same file as an example, but I wasn't sure how to incorporate in promises as we do for leave functionality (or if that's even necessary here). Moreover, I included psuedo-code for how we would actually modify the hallOfFame boolean for the given conversation but wasn't sure how to actually modify the DB entry properly. Any comments you can provide here would be great!

psybuzz commented 9 years ago

Hey @nikilselvam, of course! The client-side code you wrote looks good! I'll post some further comments on lines in the server-side code. You're on the right track :)

On promises, the main benefit is that they're supposed to reduce the depth of callbacks that we need to write, but in our case, I'm starting to feel like Q promises have made our codebase a lot harder to read, and might even be more confusing if we try and update code in the future. In the spirit of keeping things maintainable, it might be good for us, in general, to start looking at other libraries that might better suit our asynchronous needs.

nikilselvam commented 9 years ago

@psybuzz I've made changes to update the correct participant's isThrilled attribute to true within the conversation object like you suggested above. Thanks for your help on that! Let me know what you think of the changes.

One thing I noticed is that the current conversation entries in our database don't include isThrilled values for each person in the conversation's participant list. Could this mean that we currently don't set the isThrilled value to false when we add people to conversations? If so, should we add this to our logic so that our database is more explicit in whether its participants are thrilled or not about the conversation?

I also noticed one other potential issue that I'll address in the Files Changed section of this PR.

psybuzz commented 9 years ago

To answer your question about setting isThrilled to false, you're right in that the values should be false, but they don't show up in the database. I'm not sure why this is the case, because in routes/conversation.js, around line 70, there is a for loop that should be setting the isThrilled value to false for all the people.

I wonder if MongoDB might actually store all boolean-marked values as false by default. If we put a console.log(people[i].isThrilled) inside that loop, it correctly prints false for all of them, so it seems that the behavior is correct, even though it doesn't show up in the DB. As such, I think it's safe to just keep it as it is.

Overall, I had a couple minor comments, but other than that, your code looks good!

nikilselvam commented 9 years ago

@psybuzz I've addressed your comments in the latest commit. Thanks for your help! I'm learning a lot about back-end programming through this.

If the isThrilled values seem to be printing out correctly, then I agree that we're probably safe for now. Let's keep an eye on it when we start archiving conversations to ensure that the isThrilled boolean changes appropriately.

psybuzz commented 9 years ago

Alright @nikilselvam ! I made a couple minor comments on lines using reject(..., but other than that, everything looks great! After those are changed to return resError, this archiving backend functionality LGTM!

nikilselvam commented 9 years ago

Thanks @psybuzz ! I've made those changes and then added two other return resError change earlier in the file in the last two commit. Would you mind taking one last look at the final two changes and make sure the extra return resErrors look okay?

@psybuzz @tonyxj Feel free to take one last look at this one. Once I get the okay from both of you I'll go ahead and merge this in!

tonyxj commented 9 years ago

The code looks sublime @nikilselvam! It's amazing to see you guys go back and forth discussing various components. This is the foundation work for the Hall of Fame UI working coming up. I isThrilled. Merge it!

psybuzz commented 9 years ago

Agreed, LGTM!

nikilselvam commented 9 years ago

Great! Thanks guys. Ready to merge.