Page MenuHomePhabricator

Both ghost instances and exceptions to generated ghosts should be editable
ClosedPublic

Authored by lpriestley on Jun 1 2015, 1:28 AM.
Tags
None
Referenced Files
Unknown Object (File)
Sat, Mar 23, 10:21 PM
Unknown Object (File)
Thu, Mar 21, 11:02 AM
Unknown Object (File)
Fri, Mar 1, 2:06 PM
Unknown Object (File)
Feb 19 2024, 6:06 AM
Unknown Object (File)
Feb 15 2024, 1:22 PM
Unknown Object (File)
Feb 10 2024, 8:16 AM
Unknown Object (File)
Feb 9 2024, 6:18 PM
Unknown Object (File)
Feb 9 2024, 6:18 PM
Subscribers

Details

Summary

Fixes T8369, Both ghost instances and exceptions to generated ghosts should be editable

Test Plan

Create recurring event, open /E{id}/1, edit, save. New exception should not be "recurring", and should be editable, as the regular ghost events.

Diff Detail

Repository
rP Phabricator
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

lpriestley retitled this revision from to Both ghost instances and exceptions to generated ghosts should be editable.
lpriestley updated this object.
lpriestley edited the test plan for this revision. (Show Details)
lpriestley added a reviewer: epriestley.

Clearing the isRecurring flag from these events feels weird to me, especially given that it's a parameter to generateNthGhost(). As a developer, how do I know when to pass the flag or not?

I'd naviely expect all originals, ghosts, and concrete-ghosts to have this flag. Is there a reason we need to clear it instead of changing other logic?

I think it's a bit weird when users edit a specific instance and the "Is Recurring" checkbox is checked. Perhaps we should break that method into two methods that share some code?

Especially since the event view clearly links to the original recurring event (which must not be edited)

Maybe we shouldn't show you the checkbox and shouldn't let you change it after event creation? That is, the checkbox would only show up for new events. When editing an event, we'd either show nothing or maybe show some help text like "This is a recurring event.", but not show an editable checkbox.

If you want to un-recur an event, it seems reasonable to have to cancel it and create a new non-recurring event. (Likewise, converting a non-recurring event into a recurring event doesn't seem like a very important use case, although this is probably a lot easier to support and seems very slightly more likely to be useful to me.)

Exceptions should have link to original recurring event

src/applications/calendar/controller/PhabricatorCalendarEventEditController.php
206–216

Or just if ($this->isCreate()).

src/applications/calendar/storage/PhabricatorCalendarEvent.php
282–283

Hmm, I can't figure out what this is doing. Why do we need it?

lpriestley added inline comments.
src/applications/calendar/storage/PhabricatorCalendarEvent.php
282–283

Sometimes we run through this method if we're generating ghost events in which case $this is the recurring event. Sometimes we're generating an exception where $this is a ghost event with no PHID. I added this to check if there is a PHID, in which case we're cloning a real event, and if not, then we're materializing a ghost.

epriestley edited edge metadata.
This revision is now accepted and ready to land.Jun 1 2015, 3:45 AM
This revision was automatically updated to reflect the committed changes.