Show informative errors when attempting to set a recurrence end date on a non-recurring event.
ClosedPublic
Actions

Authored by lpriestley on Jun 7 2015, 6:00 PM.

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…

Summary

Fixes T8458, Show informative errors when attempting to set a recurrence end date on a non-recurring event.

Test Plan

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

Lint

Lint Not Applicable

Unit

Tests Not Applicable

Event Timeline

lpriestley updated this revision to Diff 31882.Jun 7 2015, 6:00 PM

lpriestley retitled this revision from to Show informative errors when attempting to set a recurrence end date on a non-recurring event..

lpriestley updated this object.

lpriestley edited the test plan for this revision. (Show Details)

lpriestley added a reviewer: epriestley.

lpriestley added a task: T8458: Recurrence end date control should be completely disabled unless the event is a recurring event..

Herald added a reviewer: Blessed Reviewers. · View Herald TranscriptJun 7 2015, 6:00 PM

Herald added subscribers: epriestley, Korvin. · View Herald Transcript

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
304	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.)

This revision now requires changes to proceed.Jun 7 2015, 7:06 PM

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.

epriestley accepted this revision.Jun 7 2015, 10:43 PM

epriestley edited edge metadata.

epriestley added inline comments.

src/applications/calendar/editor/PhabricatorCalendarEventEditor.php
279–280	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).

This revision is now accepted and ready to land.Jun 7 2015, 10:43 PM

switch from end date enabled to just end date. simpler, and more clear.

lpriestley added inline comments.Jun 7 2015, 10:56 PM

src/applications/calendar/editor/PhabricatorCalendarEventEditor.php
279–280	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.