Page MenuHomePhabricator

T5464, ApplicationTransactions for calendar events
ClosedPublic

Authored by lpriestley on Apr 28 2015, 4:17 AM.
Tags
None
Referenced Files
F13182808: D12586.id30224.diff
Fri, May 10, 3:35 AM
F13182806: D12586.id30225.diff
Fri, May 10, 3:34 AM
F13182805: D12586.id30220.diff
Fri, May 10, 3:34 AM
F13178149: D12586.diff
Wed, May 8, 8:13 PM
Unknown Object (File)
Thu, Apr 25, 1:33 AM
Unknown Object (File)
Thu, Apr 18, 4:33 PM
Unknown Object (File)
Thu, Apr 11, 9:07 AM
Unknown Object (File)
Apr 2 2024, 5:10 PM

Details

Summary

Closes T5464, Implement ApplicationTransactions in Calendar.

Test Plan

Create a calendar event, update calendar event, detail view of event should show update history.

Diff Detail

Repository
rP Phabricator
Branch
calendarapplicationtransactions
Lint
Lint Passed
Unit
Tests Passed
Build Status
Buildable 5557
Build 5576: [Placeholder Plan] Wait for 30 Seconds

Event Timeline

lpriestley retitled this revision from to T5464, ApplicationTransactions for calendar events.
lpriestley updated this object.
lpriestley edited the test plan for this revision. (Show Details)
lpriestley added a reviewer: epriestley.
epriestley edited edge metadata.

Couple of minor inlines. Also:

  • Looks like the .sql files didn't get added -- maybe a weird ~/.gitignore rule?
src/applications/calendar/controller/PhabricatorCalendarEventEditController.php
103

For consistency, prefer getID() over getId().

src/applications/calendar/editor/PhabricatorCalendarEventEditor.php
105–107

Just remove this stuff for now.

src/applications/calendar/storage/PhabricatorCalendarEventTransaction.php
12–15

These probably shouldn't be four separate mail tags. Mail tags are used to offer preferences in SettingsEmail Preferences so users can get email about more important events (like a task being opened or closed or commented on) but not be emailed about less important events (like priority changes or project adjustments).

It doesn't seem very useful to set different preferences for the start and end times of an event. Even though "start time" is arguably a bit more important than "end time", I'd guess that almost all users probably always want to be notified in the same way about any scheduling change. I'd maybe provide two tags here instead:

  • MAILTAG_CONTENT - The content of an event changes (start date, end date, description).
  • MAILTAG_OTHER - Other information about an event changes (status).
26

Should be PhabricatorCalendarEventTransactionComment.

This revision now requires changes to proceed.Apr 28 2015, 10:53 AM
lpriestley edited edge metadata.
lpriestley marked 4 inline comments as done.

Addressing code review and adding fixed the sql .gitignore rule.

epriestley edited edge metadata.
epriestley added inline comments.
src/applications/calendar/storage/PhabricatorCalendarEventTransaction.php
23

Then add "Event" here too.

195–205

This still uses the old tag constants.

src/applications/calendar/storage/PhabricatorCalendarTransactionComment.php
3

Oh, this should be PhabricatorCalendarEventTransactionComment for consistency with the Transaction class and the table name (that is, add "Event").

This revision now requires changes to proceed.Apr 28 2015, 1:07 PM
lpriestley edited edge metadata.
lpriestley marked 3 inline comments as done.

Renaming PhabricatorCalendarTransactionComment.php to PhabricatorCalendarEventTransactionComment.php

This revision is now accepted and ready to land.Apr 28 2015, 1:22 PM
This revision was automatically updated to reflect the committed changes.