Page MenuHomePhabricator

DRAFT, recurring events need optional end dates
ClosedPublic

Authored by lpriestley on May 31 2015, 11:02 PM.

Details

Reviewers
epriestley
Group Reviewers
Blessed Reviewers
Maniphest Tasks
T8357: Add recurrence end date
Commits
Restricted Diffusion Commit
rP95551a1a5a6e: DRAFT, recurring events need optional end dates
Summary

Ref T8357, DRAFT, recurring events need optional end dates

Test Plan

Edit recurring event, set end date, save, recurring ghosts should not generate after end date

Diff Detail

Repository
rP Phabricator
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

lpriestley retitled this revision from to DRAFT, recurring events need optional end dates.
lpriestley updated this object.
lpriestley edited the test plan for this revision. (Show Details)
lpriestley added a reviewer: epriestley.
lpriestley edited edge metadata.

brokenz update

Fixing editor. Seems to work, although not respecting recurrence end date yet.

epriestley edited edge metadata.
This revision is now accepted and ready to land.Jun 1 2015, 1:46 PM

(Not sure if you just wanted to kick this in as-is or wait to actually respect the setting, but this looks fine so far.)

Oh thanks. I will add to this a bit more before it's ready, but thanks for looking at it :3

lpriestley edited edge metadata.

Actually respect recurrence end date

One possible issue inline, the rest looks good to me.

src/applications/calendar/query/PhabricatorCalendarEventQuery.php
163

I think this change isn't correct:

  • Create a daily event a year ago.
  • View this month.

I'd expect $sequence_start to be about 365, and $instance_count to be about 30. So this will evaluate to:

for ($index = 365; $index < 30; $index++) {

...and not enter the loop body, because $index < 30 is never true.

Create a daily event a year ago.

Er, create a "daily" recurring event with the first instance starting a year ago.

src/applications/calendar/query/PhabricatorCalendarEventQuery.php
163

Just tested, and this actually works as expected, but it surfaced a small bug - recurrence end date always gets set. Will fix.

Disabled recurrence end date should not save

src/applications/calendar/controller/PhabricatorCalendarEventEditController.php
193–195

debugging code

src/applications/calendar/query/PhabricatorCalendarEventQuery.php
152–153

It looks like $instance_count is the index number of the last index here

158

It looks like $instance_count is a count here. I think one of these is wrong? As written, probably this one should be $sequence_start + $this->getRawResultLimit() and the variable should be $sequence_end?

lpriestley marked 3 inline comments as done.

Correct edit options for recurring events v ghost events

lpriestley edited edge metadata.
epriestley edited edge metadata.
epriestley added inline comments.
src/applications/calendar/controller/PhabricatorCalendarEventEditController.php
145

Seems slightly weird that we clone $start_value here and $end_value above?

This revision is now accepted and ready to land.Jun 2 2015, 1:50 AM
lpriestley edited edge metadata.

Using end_value as default recurrence end date instead of start value

This revision was automatically updated to reflect the committed changes.