Addingnotefield

[sohilpandya]

@minaorangina chaged it.

[sohilpandya]

@minaorangina made the changes.

[minaorangina]

@sohilpandya Just had a read through the code, a couple of points:

• You're running deleteEvent before getEvent, which I imagine would break? The event won't exist anymore by the time getEvent/deleteEventFromCalendar execute.
• Also, the RSVPs need deleting as well (if any)
• Your implementation of deleteEvenFromCalendar might need to be beefed up a bit. Each execution of deleteEventFromCalendar happens asynchronously, so technically, the function might return before the event has been deleted for all users (remember bug #175 ). Take a look at setHostEventChoices or getRSVPs, you might want to go for that approach.

[sohilpandya]

@minaorangina

1 - We are getting the request from the param payload, so it will not break. 2 - I'll look into it and delete the sets. 3 - Yes, remember the bug, will make the change and make it similar to setHostEventChoices/getRSVPs, although does the delete ones need to be synchronous or can we keep it as is? In the other places we are setting things into the db, here we are just deleting it.

[minaorangina]

@sohilpandya Regarding point 1: I don't follow - when you say "request from param payload", do you mean the eventID is in the request payload? If so, that's all fine, but surely when getEvent executes on line 22, there wouldn't be an event in the database to get anymore, and the response from the database would always be 0?

Point 3: Deleting is fine - it only happens once and has a callback, so we don't have to worry about returning too early. It's when these asynchronous calls need to run more than once that problems might occur

[sohilpandya]

@minaorangina Ahh, I see the issue re. point 1 now. 😨

I'll swap them so that we fetch the event first and then delete the event.

[minaorangina]

@sohilpandya Before fixing, you might want to start off by writing a failing test for it. Obviously your tests passed in order for you to be able to make the PR in the first place, but really we want to make sure our tests are covering for things like this. Important to be able to really trust our server-side code! 😄

[minaorangina]

@sohilpandya Take a look at https://github.com/DRDD2016/app/issues/168, I outlined all the different things to keep in mind in order to satisfy this user story. Treat it as a checklist if you like, there's quite a few bits to remember to do

[sohilpandya]

@minaorangina in #168 you talk about notifications and adding a field in notifications isCancelled But we decided that we are not creating notifications for events that are deleted.

[minaorangina]

@sohilpandya I remember us discussing that, but I don't remember why we came to that conclusion. Maybe we thought it would be a large step, but as I see it now it seems like it's just a matter of adding that isCancelled flag. I think the improvement to the UX is significant enough to go for it. What do you reckon?

[sohilpandya]

@minaorangina I am not sure about this actually.

I don't think we should add another notification for this, an actual addition in the future maybe to delete all the related notifications from the feed view. At the moment, once the event is deleted and they select the event from the notification feed, they will be taken to a page which says the event has been deleted.

Not sure.

[minaorangina]

@sohilpandya, removing all the previous notifications is definitely not happening at this time, as that would require a fair amount of heavylifting. But at a glance, a user would have no idea if an event has been deleted or not until they click it (if they click it). I reckon adding the notification is a good compromise, and doesn't require too much of a change.

It would be ideal to implement it now, as we don't want sprint 2 hanging over our heads. But if you prefer, raise an issue and have a think about it if you like.

I'm heading out now, I'll be around for merging etc tomorrow morning :+1:

[sohilpandya]

@minaorangina I have made all the changes except for the notifications when the event is deleted.

I have created an issue, we can look into it on tuesday, there are quite a few changes that will need to be made in order to accomodate the new isCancelled field, so we could probably add the other additional fields we need to add to the notifications at the same time. i.e. hasRSVP'd, hasEdited etc.

[minaorangina]

Still can't merge yet @sohilpandya. There's a few things:

Function naming

deleteEventFromCalendar and deleteEventFromUserCalendar are very similar names. It's hard for me to know what the difference is between these two functions at a glance. Let's look at this:

• server/db/deleteEventFromUsersCalendar.js holds a function called deleteEvent*s*FromUsersCalendar (plural). The file name should be exactly the same as the function name.
• deleteEvent*s*FromUsersCalendar should not be pluralised. Though it is called more than once by deleteEventFromCalendar, this function itself does one thing, once. The name implies otherwise.
• The outer function's name, deleteEventFromCalendar is a bit of a misnomer - it's job is to delete one event from several calendars, so ought to be pluralised. This name is more suited to the inner function, so you may want to swap
• Swap the names around, then change deleteEventFromUsersCalendar to deleteEventFromUserCalendars. These names would be a bit closer to the truth.
• In deleteEventFromCalendar, you require deleteEventFromUsersCalendar but you call deleteEventFromUserCalendar. This is a bug - you would get: TypeError: undefined is not a function.

Testing

deleteEventFromCalendar.js

• You have written a test for deleteEventFromCalendar, but you're actually testing deleteEvent!
• We need a proper test for deleteEventFromCalendar - to have done so would have caught the bug I mentioned earlier. It's especially crucial to test the functions that run other functions iteratively or recursively, as these can be fragile.

endpoints.js

• You're testing the wrong endpoint... (line 277)
• Line 294 of endpoints.js, you're testing the number 1, doesn't achieve much. If you want to test the success of the get request, have a look inside the response object and look for a 200 response code or something.
• Assertion on line 295 fails, but that relates to the incorrect endpoint being tested.
• No need for t.deepEqual in, both here and in deleteEventFromCalendar.js - t.equal is enough.

initDB.js

• In initDB, line 92, you're putting the wrong eventID into event:500
• Line 94, you're pushing a notification for event:500. I can't see it being removed again in any test teardowns, so they'll build up. Be sure to prevent this.

I'd really recommend using failing tests as your starting point. There's loads going on with this user story and easy for bugs to creep in. If you go test-first here, you have 100% proof that everything is fine and working well 😊

[minaorangina]

👌