Page MenuHomePhabricator

Generate "stub" events earlier, so more infrastructure works with Calendar
ClosedPublic

Authored by epriestley on Jul 7 2016, 5:26 PM.

Details

Summary

Ref T9275. When you create a recurring event which recurs forever, we want to avoid writing an infinite number of rows to the database.

Currently, we write a row to the database right before you edit the event. Until then, we refer to it as E123/999 or whatever ("instance 999 of event 123").

This creates a big mess with trying to make recurring events work with EditEngine, Subscriptions, Projects, Flags, Tokens, etc -- all of this stuff assumes that whatever you're working with has a PHID.

I poked at letting this stuff work without a PHID a little bit, but that looked like a gigantic mess.

Instead, generate an event "stub" a little sooner (when you look at the event detail page). This is basically just an ID/PHID to refer to the instance.

Then, when you edit the stub, "materialize" it into a real event.

This still has some issues, but I think it's more promising than the other approach was.

Also:

  • Removes dead user profile calendar controller.
  • Replaces comments with EditEngine comments.
Test Plan
  • Commented on a recurring event.
  • Awarded tokens to a recurring event.

Diff Detail

Repository
rP Phabricator
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

epriestley updated this revision to Diff 39087.Jul 7 2016, 5:26 PM
epriestley retitled this revision from to Generate "stub" events earlier, so more infrastructure works with Calendar.
epriestley updated this object.
epriestley edited the test plan for this revision. (Show Details)
epriestley added a reviewer: chad.

I'm going to hold this stuff until after the release cut, next few changes are going to be kinda rough for a bit I think.

I also wonder if this is going to create an issue with search engines crawling infinite pages of calendars and creating infinite event stubs. Not sure what to do about that.

epriestley planned changes to this revision.Jul 9 2016, 1:55 PM

For now, I'm just going to prevent logged-out users from generating stubs.

epriestley updated this revision to Diff 39124.Jul 9 2016, 1:58 PM
epriestley edited edge metadata.
  • Simple fix to prevent runaway stub creation if a crawler goes crazy.

Also note that stub creation isn't really a big deal if it does happen somehow: you can safely delete all the stubs. So even if this goes haywire it should be reasonable to clean it up.

chad accepted this revision.Jul 9 2016, 5:15 PM
chad edited edge metadata.
This revision is now accepted and ready to land.Jul 9 2016, 5:15 PM
This revision was automatically updated to reflect the committed changes.
20after4 added a subscriber: 20after4.EditedJul 27 2016, 9:18 AM

@epriestley: Does an object necessarily have to have a unique monogram in order to have a phid? (I don't think so, sort of a rhetorical question)

In my opinion it would make more sense to always refer to the instances of an event by using the same event ID followed by the recurrence number. E.g. E1015r2 (for E1015 recurrence #2) rather than E1040.

As it is now the events get assigned nonsensical and non-sequential IDs. It seems very odd that the order of IDs would be based only on the first time each recurrence happens to be viewed by someone.

I guess what I'm saying is if the E100/4 monogram format was always used then it would at least be consistent and the phid could still be assigned automatically as this revision implements. Maybe I'm missing something - does edit engine absolutely require that every object has a numeric ID as well as a PHID?

We could decouple event monograms from event IDs -- a good example of this today is commits, which do have internal IDs but have monograms like "rXyyyy" instead of "r9999999". Parent and child IDs and sequence numbers are also stable and I think always available, so generating a monogram like E123/7 or E123r7 should be straightforward.

I haven't done this so far because I'm not sure how common using/memorizing event IDs will really be in the final system. If it's pretty rare to refer to events by ID/monogram, it might not be worth the extra work to make them more clearly related. A maybe-sort-of-related example is Phame blogs, where posts get global "Jxxx" mongrams instead of "Jblog-id/sequence-id" monograms. Phame is pretty new, but it feels OK to me so far that two posts on the same blog don't end up with obviously-related monograms.

My guess is that recurring events in Calendar are probably "more-related" to their parent event than posts are to their blog, but maybe "less-related" than commits are to their repository. I'm not sure exactly where on the spectrum they'll end up, though.

I'm also assuming that stub events will mostly end up getting created roughly in chronological order in practice, even though there's no guarantee of that. If that doesn't happen and it's confusing, we could force it to happen: when generating stub X, generate all ungenerated earlier stubs first. Then recurrence N+1 would always have a higher ID than recurrence N. This is likely pretty straightforward if the major issue is just out-of-order IDs and it's less important to know that Exxx is really Eyyy/N at a glance.

20after4 added a comment.EditedJul 27 2016, 10:26 PM

My guess is that recurring events in Calendar are probably "more-related" to their parent event than posts are to their blog, but maybe "less-related" than commits are to their repository. I'm not sure exactly where on the spectrum they'll end up, though.

I'm also assuming that stub events will mostly end up getting created roughly in chronological order in practice, even though there's no guarantee of that. If that doesn't happen and it's confusing, we could force it to happen: when generating stub X, generate all ungenerated earlier stubs first. Then recurrence N+1 would always have a higher ID than recurrence N. This is likely pretty straightforward if the major issue is just out-of-order IDs and it's less important to know that Exxx is really Eyyy/N at a glance.

One advantage to the E123/N format is for navigating from one recurrence to the next. With relative monograms, figuring out the URL for the next recurrence could be as simple as increment or decrement the number in the URL. I'd be willing to work on this if you would be receptive to reviewing a patch, but not right away I've got a lot of other stuff on my plate right now.