Includes a new Block in Herad emails which tells the user about which commits were merged in a merge commit
Otherwise the email would just say "Merge branch XYZ". Ref T8295
Details
Details
- Reviewers
epriestley - Group Reviewers
Blessed Reviewers - Maniphest Tasks
- T8295: Show which commits were included in a merge commit in Herald emails
- Commits
- Restricted Diffusion Commit
rPf0d16b3047ff: show merged commits in herald emails for merge commits
imported various commits (and merges) and watched resulting herald emails for all of them
Diff Detail
Diff Detail
- Repository
- rP Phabricator
- Branch
- mergeCommits
- Lint
Lint Passed - Unit
Test Failures - Build Status
Buildable 6243 Build 6265: [Placeholder Plan] Wait for 30 Seconds
| Time | Test | |
|---|---|---|
| 8,442 ms | PhabricatorCelerityTestCase::testCelerityMaps | |
| 1,396 ms | PhabricatorConduitTestCase::testConduitMethods | |
| 724 ms | PhabricatorInfrastructureTestCase::testApplicationsInstalled | |
| 844 ms | PhabricatorInfrastructureTestCase::testRejectMySQLNonUTF8Queries | |
| 1,441 ms | PhabricatorLibraryTestCase::testEverythingImplemented | |
| View Full Test Results (1 Failed · 6 Passed) |
Event Timeline
Comment Actions
Some minor stuff inline, this generally looks good to me.
| src/applications/repository/customfield/PhabricatorCommitMergedCommitsField.php | ||
|---|---|---|
| 33 | This should be the actual viewer -- I think $this->getViewer() will work. | |
| 34 | I think this is no longer used for anything and has no effect (we determine it automatically in all cases, now). | |
| 40 | Using ConduitCall directly (as in PhabricatorCommitBranchesField) might be a little simpler than building a DiffusionRequest. | |
| 41 | Use real viewer. | |
| 52–53 | Use pht(). | |
| 57–61 | Maybe simpler with foreach ($merges as $merge). | |
| 68–69 | Maybe put $ex->getMessage() in the mail body? So we end up with something like:
That seems more obvious/helpful than just dumping it to the webserver error log. | |