Page MenuHomePhabricator

All day events should obey selected query range in viewer timezone.
ClosedPublic

Authored by lpriestley on May 26 2015, 6:52 PM.
Tags
None
Referenced Files
Unknown Object (File)
Thu, Dec 19, 7:02 PM
Unknown Object (File)
Thu, Dec 19, 9:41 AM
Unknown Object (File)
Thu, Dec 19, 6:52 AM
Unknown Object (File)
Mon, Dec 9, 9:48 AM
Unknown Object (File)
Wed, Dec 4, 3:03 AM
Unknown Object (File)
Wed, Dec 4, 3:02 AM
Unknown Object (File)
Wed, Dec 4, 3:02 AM
Unknown Object (File)
Wed, Dec 4, 3:02 AM
Subscribers

Details

Summary

Fixes T8147, All day events should obey selected query range in viewer timezone.

Test Plan

Create all day event May 25, query for events May 26-27. All day event should not be part of the query results list.

Diff Detail

Repository
rP Phabricator
Branch
calendaralldayquery
Lint
Lint Passed
Unit
Tests Passed
Build Status
Buildable 6298
Build 6320: [Placeholder Plan] Wait for 30 Seconds

Event Timeline

lpriestley retitled this revision from to All day events should obey selected query range in viewer timezone..
lpriestley updated this object.
lpriestley edited the test plan for this revision. (Show Details)
lpriestley added a reviewer: epriestley.
epriestley edited edge metadata.
epriestley added inline comments.
src/applications/calendar/query/PhabricatorCalendarEventQuery.php
204–205

If the query has no ranges, or only one end of the range, I think this will incorrectly filter all the events?

(In the general case, like the list view, a query may execute with no range constraints at all.)

206

Consider using foreach ($events as $key => $event) { ... unset($events[$key]); } to remove events (i.e., reject bad events) instead of collecting the events you're going to keep. If there are several filters, it's a little simpler to do:

foreach ($events as $key => $event) {
  if (no_good()) {
    unset($events[$key]);
  }
}

...several times in a row, instead of repeatedly moving everything to a new list of filtered events. This is more consistent with other query classes, which more frequently have several reasons to reject objects.

208

I think the array_values() is unnecessary?

This revision now requires changes to proceed.May 26 2015, 6:57 PM
lpriestley edited edge metadata.
lpriestley marked 3 inline comments as done.

if query has no ranges, don't unset any events

lpriestley edited edge metadata.

pedantic point

epriestley edited edge metadata.
This revision is now accepted and ready to land.May 26 2015, 9:27 PM
This revision was automatically updated to reflect the committed changes.