Page MenuHomePhabricator

Fix calendar part 2
ClosedPublic

Authored by btrahan on Feb 25 2014, 9:57 PM.
Tags
None
Referenced Files
F14056453: D8342.diff
Sat, Nov 16, 8:39 PM
F14003731: D8342.id19836.diff
Sat, Oct 26, 10:01 AM
F13982375: D8342.id19835.diff
Oct 19 2024, 10:45 PM
Unknown Object (File)
Sep 19 2024, 7:16 AM
Unknown Object (File)
Sep 19 2024, 7:05 AM
Unknown Object (File)
Sep 19 2024, 7:05 AM
Unknown Object (File)
Sep 17 2024, 5:30 PM
Unknown Object (File)
Sep 8 2024, 6:30 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