Page MenuHomePhabricator

Allow modular transactions to override transaction title and body text in mail

Authored by epriestley on Jan 29 2019, 8:09 PM.



Ref T12921. I'm moving Instances to modular transactions, and we have an "Alert" transaction type used to send notifications ("Your instance is going to be suspended for nonpayment.").

Currently, there's no way to specifically customize mail rendering under modular transactions. Add crude support for it.

Note that (per comment) this is fairly aspirational right now, since we actually always render everything as text (see T12921). But this API should (?) mostly survive intact when I fix this properly, and allows Instances to move to modular transactions so I can fix some more pressing issues in the meantime.

Test Plan

See next diff for Instances.

Diff Detail

rP Phabricator
Automatic diff as part of commit; lint not applicable.
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

epriestley created this revision.Jan 29 2019, 8:09 PM
epriestley requested review of this revision.Jan 29 2019, 8:11 PM
amckinley accepted this revision.Jan 31 2019, 2:26 AM
amckinley added inline comments.

This feels weird... How is this different from if ($comment)?

This revision is now accepted and ready to land.Jan 31 2019, 2:26 AM
epriestley added inline comments.Jan 31 2019, 3:24 AM

Yeah, I'm cheating a bit here:

  • Returning a comment means "that comment".
  • Returning null means "no comment", i.e. "do not render anything in mail". This lets modular types explicitly hide a value that would otherwise be rendered.
  • Returning false means "use default beahvior", i.e. fall back to the older rendering. Some day, this could be cleaned up.

This diff is more of a "try to limit scope creep" sort of change than a crowning jewel of perfect programming.

(if ($comment) is also different for the comment "0".)

In practice, subclasses only ever return null or a comment: the default behavior is "fall back to older behavior", so you only override if you want to do something else. So the "false" magic shouldn't really infect anything.

This revision was automatically updated to reflect the committed changes.