Fixes T8458, Show informative errors when attempting to set a recurrence end date on a non-recurring event.
Details
- Reviewers
epriestley - Group Reviewers
Blessed Reviewers - Maniphest Tasks
- T8458: Recurrence end date control should be completely disabled unless the event is a recurring event.
- Commits
- Restricted Diffusion Commit
rP47051643c76c: Show informative errors when attempting to set a recurrence end date on a non…
Create new event, set recurrence end date via date-picker without setting the "is recurring" checkbox, and attempt to save. Should get error saying there cannot be a recurrence end date on a non-recurring event.
Diff Detail
- Repository
- rP Phabricator
- Branch
- calendarrecurrenceenddate
- Lint
Lint Passed - Unit
Tests Passed - Build Status
Buildable 6606 Build 6628: [Placeholder Plan] Wait for 30 Seconds
Event Timeline
What about just hiding the field if the event isn't recurring (or even making creation of recurring vs non-recurring events modal, so you don't have to do any JS wizardry)?
One inline with a technical issue.
src/applications/calendar/editor/PhabricatorCalendarEventEditor.php | ||
---|---|---|
300 | We may reach this code with $recurrence_end_enabled and $is_recurring not defined. (For example, I'd expect canceling an event to fail with an error here.) |
I'm not sure about making it modal. When the options are private, public, and recurring, it's very ambiguous if recurring will be private or public. I want to think about it some more before committing to that.
Initializing end date and recurring so that they don't throw an error when null in editor.
src/applications/calendar/editor/PhabricatorCalendarEventEditor.php | ||
---|---|---|
278–279 | These might need to be be $object->getIsRecurring() and $object->getRecurrenceEndDate() or similar, you might be able to sneak this through by doing it in two transactions now (remove recurring first, then edit again and add an end date). |
src/applications/calendar/editor/PhabricatorCalendarEventEditor.php | ||
---|---|---|
278–279 | Okay, just switching to recurrence_end instead of recurrence_end_enabled. Seems like that would make more sense anyways, despite the ui preventing edits to recurring events. |