Page MenuHomePhabricator

Fix calendar part 2
ClosedPublic

Authored by btrahan on Feb 25 2014, 9:57 PM.
Tags
None
Referenced Files
Unknown Object (File)
Sun, Sep 8, 6:30 AM
Unknown Object (File)
Sat, Sep 7, 4:59 AM
Unknown Object (File)
Wed, Sep 4, 12:43 PM
Unknown Object (File)
Sat, Aug 31, 5:24 AM
Unknown Object (File)
Wed, Aug 28, 6:46 PM
Unknown Object (File)
Mon, Aug 26, 2:52 AM
Unknown Object (File)
Mon, Aug 19, 1:01 AM
Unknown Object (File)
Sat, Aug 17, 2:38 AM

Details

Reviewers
epriestley
chad
Commits
Restricted Diffusion Commit
rP9b0f906207eb: Fix calendar part 2
Summary

D8341 was a good start. However, I was looping through all the statuses each time, when I should only deal with a given status once. Instead, unset() a status from the list of statuses once we handled it. Also, delete the last old $key thing, which interfered with my chosen strategy.

Test Plan

made a two day event and verified it showed up in just those two days. (will push and test again just in case but this should be it)

Diff Detail

Lint
Lint Skipped
Unit
Tests Skipped

Event Timeline

{F118245}

It was a classy test status

epriestley added inline comments.
src/applications/people/controller/PhabricatorPeopleProfileController.php
185–186

Could we remove this loop part instead? It looks like the logic is:

foreach (day) {
  foreach (event) {
    foreach (future day) {
      put the event on all of the days it goes on
    }
    unset the event;
  }
}

Instead, maybe one of these would be simpler?

foreach (day) {
  foreach (event) {
    maybe put this event on this day
  }
}

Or:

foreach (event) {
  put it on all the days it goes on
}
btrahan updated this revision to Unknown Object (????).Feb 25 2014, 10:17 PM

use foreach (epoch / day) { foreach (event) } algorithm.

(i don't think I can just do foreach (event) as it is about mapping these events to days.)

tested by adding a bunch more events to myself; it works