New Event and Related Subscriptions

[wyntarvox]

I am working on bug 3479. Basically, the suggestion came in to have subsequent notifications for edited comments simply have a message saying that the comment was edited, the time, and the reason, rather than repeating the body of the comment as they currently do now.

I set out to make edited comments an event on its own (well, a sub-event of LJ::Event::JournalNewComment). Notifications are built on-the-fly using the live comment object, so adjusting the display for an edited comment to only have the time/reason, will make the original notification do the same. fu and I bandied about the idea of using the event time and either compare it to the edit time or the original comment time, but both of these had drawbacks.

Event vs. edit time meant only the last notification of an edit would be the short version. Event vs. original comment time meant we'd have to have an amount of room-for-error between the comment being posted and the event firing, which defeats the purpose really. A big reason for the suggestion was to prevent multiple identical notifications when someone posts a comment, notices a typo, edits, all within our window of room-for-error.

So, we came back to it being necessary to have this as a separate event. However, that also means needing a separate subscription for comment edits. There are a few options:

1. Have a separate subscription for comment edits (maybe people would like this and don't care about being notified re: edits?)


2. Special-case comment edits to piggy back off subscriptions for new comment edits


3. Build something more robust to let any event piggy back off the subscriptions of another event

Thoughts, comments, direction? Please?

Comments

[fu]

I think that 2) could be done as a simpler version of 3), so I'd go only between 1) and 2). And then -- for 1, what I'm worried about is eating up people's subscriptions (because they'd need an edit subscription for each comment subscription they have), and the added complexity of having to decide whether you want to see edits or not.

So, in terms of interface/user-friendliness, 2 (and perhaps 3) may be the way to go. I can't actually tell what would go into piggybacking one comment off another though. But if you're going that route, in the absence of any other potential approaches, I'd suggest doing whatever it takes to get 2 to work, even if it's ridiculous/hardcoded all over the place, and then step back, see how you can clean it up, and whether the approach is feasible. And at that point, it may be clear whether a more general 3 is possible, or even cleaner.

[denise]

Yah. 2 is the best option from end-user perspective: I really hate it that we eat up subscriptions with notifications that used to be 'free' before ESN, and anything we can do to keep from doing that is a good thing.

I think, though, that doing 3 (or 2 leading to 3, like Fu says) is a good way towards improving the ESN system really well for future work. There's a bug in somewhere to upgrade to LJ's latest version of ESN (since they redid a bunch of stuff and made it possible to email only comments, not inbox required) that you might also want to look at as a starting place, since I don't know if it would be useful at all.

[wyntarvox]

Yeah, I didn't really think of the how for 2/3. Oops. Haha.

[wyntarvox]

It looks like we already have a 3! When TheSchwartz fires event notifications, it looks for subscriptions by calling $event->subscription. That method in turns build a list of events using $event->related_events and uses that to build its where condition in the subscription list SQL.

my $events_list = join( ",", $self->related_events );
...
my $sql = "SELECT userid, subid, is_dirty, journalid, etypeid, " .
"arg1, arg2, ntypeid, createtime, expiretime, flags " .
"FROM subs WHERE etypeid IN ($events_list) $journal_match $and_enabled $limit_sql";

I was stumped for a while, because I thought that this should doing exactly what I wanted, but it just wanted. Then I realised that I was relying on JournalNewComment->related_events rather than a specific JournalNewComment::Edited->related_events. So I defined JournalNewComment::Edited->related_events to tie it to JournalNewComment and hooray! My Edited notifications come through without having to set up individual subscriptions for them. :)

Does that make sense?

[denise]

It makes logical sense, yes, even if ESN is like unto a black box of Satanic complexity to me. :)

[fu]

Makes perfect sense now that you've pointed it out! I'd still never have thought to do it that way otherwise *grins*