Summary: Applied some more linter fixes that I previously missed because my global `arc` install was out-of-date.
Test Plan: Will run `arc unit` on another host.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: epriestley, Korvin
Differential Revision: https://secure.phabricator.com/D9443
Summary: Ran `arc lint --apply-patches --everything` over rP, mainly to change double quotes to single quotes where appropriate. These changes also validate that the `ArcanistXHPASTLinter::LINT_DOUBLE_QUOTE` rule is working as expected.
Test Plan: Eyeballed it.
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: epriestley, Korvin, hach-que
Differential Revision: https://secure.phabricator.com/D9431
Summary: Some actions (notably, `!accept`) require more information than we currently load.
Test Plan: Piped in some `!accept` mail using `bin/mail receive-test`.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Differential Revision: https://secure.phabricator.com/D8844
Summary:
Ref T4371. Ref T4699. Fixes T3994.
Currently, we're very conservative about sending errors back to users. A concern I had about this was that mistakes could lead to email loops, massive amounts of email spam, etc. Because of this, I was pretty hesitant about replying to email with more email when I wrote this stuff.
However, this was a long time ago. We now have Message-ID deduplication, "X-Phabricator-Sent-This-Mail", generally better mail infrastructure, and rate limiting. Together, these mechanisms should reasonably prevent anything crazy (primarily, infinite email loops) from happening.
Thus:
- When we hit any processing error after receiving a mail, try to send the author a reply with details about what went wrong. These are limited to 6 per hour per address.
- Rewrite most of the errors to be more detailed and informative.
- Rewrite most of the errors in a user-facing voice ("You sent this mail..." instead of "This mail was sent..").
- Remove the redundant, less sophisticated code which does something similar in Differential.
Test Plan:
- Using `scripts/mail/mail_receiver.php`, artificially received a pile of mail.
- Hit a bunch of different errors.
- Saw reasonable error mail get sent to me.
- Saw other reasonable error mail get rate limited.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T3994, T4371, T4699
Differential Revision: https://secure.phabricator.com/D8692
Summary:
Ref T2222. Brings the major mail features (affected files, patches) forward.
This drops some of the minor integrations which just show object state (like "Maniphest Tasks") since I think they're not very important. I'll put them back if users miss them.
Test Plan: Sent mail with inline/attached patches.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T2222
Differential Revision: https://secure.phabricator.com/D8459
Summary: Ref T2222. These no longer have any callsites. Also got rid of a little bit of other code which also no longer has callsites.
Test Plan: `grep`
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T2222
Differential Revision: https://secure.phabricator.com/D8443
Summary: Ref T2222. Moves this instance of CommentEditor to TransactionEditor.
Test Plan: Used `bin/mail receive-test` to test receiving comment mail and action mail.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T2222
Differential Revision: https://secure.phabricator.com/D8427
Summary:
Currently, the linter raises `XHP29` warnings for these files because they are not abstract or final.
I guess there are two possibly solutions, either making the classes final or marking them as `@concrete-extensible`. Given that there are no subclasses of these classes in the `phabricator`, `arcanist` and `libphutil` repositories... I opted to declare the classes as final.
Test Plan:
The following linter warnings are gone:
```
>>> Lint for src/aphront/configuration/AphrontDefaultApplicationConfiguration.php:
Warning (XHP29) Class Not abstract Or final
This class is neither 'final' nor 'abstract', and does not have a
docblock marking it '@concrete-extensible'.
3 /**
4 * @group aphront
5 */
>>> 6 class AphrontDefaultApplicationConfiguration
7 extends AphrontApplicationConfiguration {
8
9 public function __construct() {
>>> Lint for src/applications/differential/mail/DifferentialReplyHandler.php:
Warning (XHP29) Class Not abstract Or final
This class is neither 'final' nor 'abstract', and does not have a
docblock marking it '@concrete-extensible'.
1 <?php
2
>>> 3 class DifferentialReplyHandler extends PhabricatorMailReplyHandler {
4
5 private $receivedMail;
6
```
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley
CC: Korvin, epriestley, aran
Differential Revision: https://secure.phabricator.com/D8347
Summary: Ref T2222. Adds a mostly-functional "Pro" comment controller. This does the core stuff, but does not yet do actions (accept, reject, etc.) or inline comments.
Test Plan: Changed the `if (false)` to an `if (true)`, then made some comments, etc. This is normally unreachable.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T2222
Differential Revision: https://secure.phabricator.com/D8304
Summary: Ref T2222. Ref T1790. I partially modernized this recently, but bring it to the mail version too.
Test Plan: See screenshots.
Reviewers: btrahan
Reviewed By: btrahan
CC: zeeg, aran
Maniphest Tasks: T1790, T2222
Differential Revision: https://secure.phabricator.com/D8294
Summary: Ref T2222. This is a `tmp.differential`-only issue. Inline comment transactions now have content, so we treat them like body text. We also render them separately as inline text. This produces mail where inlines are rendered twice.
Test Plan: Sent myself mail, saw only one copy of inlines.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T2222
Differential Revision: https://secure.phabricator.com/D8216
Summary:
See D8200. Ref T2222. Instead of writing one comment which can have a ton of different effects, write a series of one-effect comments. These will be easier to convert into ApplicationTransactions.
This has a minor user-facing effect of making these multiple-action comments render separately:
{F111919}
Once the migration completes, they should automatically merge together nicely again.
Test Plan: Made a bunch of comments and took a bunch of actions, all of which worked normally except that they rendered as several things instead of just one.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran, FacebookPOC
Maniphest Tasks: T2222
Differential Revision: https://secure.phabricator.com/D8201
Summary:
Ref T2222. Currently, one `DifferentialComment` can do a lot of things (add ccs, reviewers, comments, inline comments, and perform state changes). In the future, each `ApplicationTransaction` does only one thing. This is the primary piece of complexity which makes the upcoming migration risky, because each comment needs to migrate into multiple transactions.
I want to mitigate this complexity as much as possible before the migration itself happens. One approach I'm going to use to do that is to start writing one comment per effect now, so the mapping is more direct when the migration itself happens and the write code can be straightforward (one row per save()) after the migration.
This tackles a small piece of that, which is the mail Differential sends. Currently, Differential mail acts on a single comment. Instead, allow it to act on a list of comments, but always give it one comment for now. In the future, we can hand it several comments instead and still get the expected behavior.
This change should have no impact on any application behaviors.
Test Plan:
- Commented;
- commented with inline;
- added reviewers;
- added CCs;
- added CCs via mentions;
- updated revision;
- looked at all the mail, all of which seemed sane.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T2222
Differential Revision: https://secure.phabricator.com/D8199
Summary:
Via Asana. The tags on Differential mail are wrong in two cases:
- Transactions which submit inline comments but no comment text are not labeled as "comments", but should be.
- Non-close, non-comment transactions are not labeled at all, but should be labeled "other".
Test Plan: Submitted a no-comments, inlines-only transaction and got a message with proper `X-Phabricator-Mail-Tags` header.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Differential Revision: https://secure.phabricator.com/D7912
Summary: Fixes T4001. I broke this some time ago and no one has complained. I don't think it gets much use, and we haven't added it for the newer apps. Just get rid of it rather than adapt the URIs for ApplicationSearch.
Test Plan: Unit tests, sent myself some email.
Reviewers: zeeg, btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T4001
Differential Revision: https://secure.phabricator.com/D7355
Test Plan:
Enable inline patches:
```
bin/config set metamta.differential.patch-format 'unified'
bin/config set metamta.differential.inline-patches 100000000
```
Create a new diff and confirm it renders correctly via email.
Reviewers: epriestley
Reviewed By: epriestley
CC: Korvin, aran
Differential Revision: https://secure.phabricator.com/D7198
Summary: I removed the only callsite in D7179, but forgot to remove this code.
Test Plan: Grepped for callsites.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Differential Revision: https://secure.phabricator.com/D7194
Summary: Ref T603. Clean these up and move them to a single place.
Test Plan:
- Downloaded a raw diff.
- Enabled "attach diffs", created a revision, got an email with a diff.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T603
Differential Revision: https://secure.phabricator.com/D7179
Summary: Ref T603. Swaps out most `PhabricatorFile` loads for `PhabricatorFileQuery`.
Test Plan:
- Viewed Differential changesets.
- Used `file.info`.
- Used `file.download`.
- Viewed a file.
- Deleted a file.
- Used `/Fnnnn` to access a file.
- Uploaded an image, verified a thumbnail generated.
- Created and edited a macro.
- Added a meme.
- Did old-school attach-a-file-to-a-task.
- Viewed a paste.
- Viewed a mock.
- Embedded a mock.
- Profiled a page.
- Parsed a commit with image files linked to a revision with image files.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T603
Differential Revision: https://secure.phabricator.com/D7178
Summary: I kind of made a mess of the API doing T2784. I figure just adding this is fine but LMK if you'd prefer something like diffquery got cleaned up more to handle this. Also adds an idx() call as I was getting errors looking at old diffs. Fixes T3823.
Test Plan: used the new api via test console - great success.
Reviewers: epriestley
Reviewed By: epriestley
CC: Korvin, aran
Maniphest Tasks: T3823
Differential Revision: https://secure.phabricator.com/D6966
Summary: Ref T603. Killing this class is cool because the classes that replace it are policy-aware. Tried to keep my wits about me as I did this and fixed a few random things along the way. (Ones I remember right now are pulling a query outside of a foreach loop in Releeph and fixing the text in UIExample to note that the ace of hearts if "a powerful" card and not the "most powerful" card (Q of spades gets that honor IMO))
Test Plan: tested the first few changes (execute, executeOne X handle, object) then got real mechanical / careful with the other changes.
Reviewers: epriestley
Reviewed By: epriestley
CC: Korvin, aran, FacebookPOC
Maniphest Tasks: T603
Differential Revision: https://secure.phabricator.com/D6941
Summary: Ref T2715. Had to start loading status information in the query class. Debated trying to clean up some of the attach / load stuff but decided to just add status under the new paradigm for now.
Test Plan: phid.query also made a status and checked that out. also played in conpherence.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Maniphest Tasks: T2715
Differential Revision: https://secure.phabricator.com/D6585
Summary:
Fixes T3030. T1977 attempted to fix this but either didn't work (I think this is the case) or was broken later. We don't send `DifferentialCommentMail` on a create or update; we send `DifferentialReviewRequestMail`.
Also update the details to be more clear.
Test Plan:
Verified review request mail is marked undeliverable:
```
$ ./bin/mail show-outbound --id 6644
...
PARAMETERS
...
mailtags: ["differential-review-request"]
...
subject: D922: asdf
subject-prefix: [Differential]
vary-subject-prefix: [Request, 100 lines]
...
RECIPIENTS
! duck (duck)
- This mail has tags which control which users receive it, and this recipient has not elected to receive mail with any of the tags on this message (Settings > Email Preferences).
BODY
epriestley requested code review of "asdf".
...
```
Verified update mail is marked undeliverable:
```
$ ./bin/mail show-outbound --id 6646
...
Message: Message has no valid recipients: all To/Cc are disabled, invalid, or configured not to receive this mail.
PARAMETERS
...
mailtags: ["differential-updated"]
...
subject: D922: asdf
subject-prefix: [Differential]
vary-subject-prefix: [Updated, 100 lines]
...
RECIPIENTS
! duck (duck)
- This mail has tags which control which users receive it, and this recipient has not elected to receive mail with any of the tags on this message (Settings > Email Preferences).
BODY
epriestley updated the revision "asdf".
...
```
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T3030
Differential Revision: https://secure.phabricator.com/D6518
Summary:
Moves all remaining mail handling into ReplyHandlers.
Farewell, `getPhabricatorToInformation()`! You were a bad method and no one liked you.
Ref T1205.
Test Plan:
- Used test console to send mail to Revisions, Tasks, Conpherences and Commits (these all actually work).
- Used test console to send mail to Requests, Macros, Questions and Mocks (these accept the mail but don't do anything with it, but didn't do anything before either).
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T1205
Differential Revision: https://secure.phabricator.com/D5953
Summary: This doesn't do anything, but touches a bunch of files so I split it out to reduce the size of the next diff. Basically, make `MailReceiver` classes responsible for loading their application objects. Ref T1205.
Test Plan: Inspection / next diff / code is not reached.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T1205
Differential Revision: https://secure.phabricator.com/D5941
Summary:
Ref T1205. Continuation of D5915.
Currently, `PhabricatorMetaMTAReceivedMail` has //all// the logic for routing mail. In particular:
- New mail receivers in applications must edit it.
- Mail receivers don't drop out when applications are uninstalled.
Applications have some logic in subclasses of `PhabricatorMailReplyHandler`, but this class is a bit of a mess. It is also heavily based on the assumption that mail receivers are objects (like revisions), but this is not true in at least two cases today (creating new tasks with `bugs@`, creating a new Conpherence thread) and likely other cases in the future (e.g., revision-by-mail).
Move this logic into a new `PhabricatorMailReceiver` classtree. This is similar to `PhabricatorMailReplyHandler` but a bit cleaner and more general. I plan to heavily reduce the responsibilities of `PhabricatorMailReplyHandler` or possibly eliminate it entirely.
For now, the new classtree doesn't do much of interest. The only behavioral change this diff causes is that Phabricator will now reject mail to an application when that application is uninstalled.
I also moved all the `ReplyHandler` classes into `mail/` directories in their respective applications.
Test Plan: Unit tests, used receive test to route mail to various objects.
Reviewers: btrahan
Reviewed By: btrahan
CC: Afaque_Hussain, edward, aran
Maniphest Tasks: T1205
Differential Revision: https://secure.phabricator.com/D5922
Summary:
Unmuck almost all of the we-sort-of-have-viewers-some-of-the-time mess.
There are a few notable cases here:
- I used Omnipotent users when indexing objects for search. I think this is correct; we do policy filtering when showing results.
- I cheated in a bad way in the Remarkup object rule, but fixing this requires fixing all the PhabricatorRemarkupEngine callsites (there are 85). I'll do that in the next diff.
- I cheated in a few random places, like when sending mail about package edits. These aren't a big deal.
Test Plan:
- Grepped for all PhabricatorObjectHandleData references.
- Gave them viewers.
Reviewers: vrana
Reviewed By: vrana
CC: aran, edward
Maniphest Tasks: T603
Differential Revision: https://secure.phabricator.com/D5151
Summary: This triggers mainly for SVN branch copy.
Test Plan: Ran this code separately.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D5126
Summary:
T2345
getConfig throws an Exception when the key does not exist.
Also removes dead code that throws an Exception.
Test Plan:
Reloaded the Phabricator home page. In the process, found
2 Exceptions thrown due to nonexistent keys. After addressing these problems,
the home page loads without Exceptions.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D4541
Summary: introducing a new friend called DifferentialHunkParser. Sort of like the DifferentialChangesetParser but works with hunks only. tried to grab hunk parsing type things from across the code base and move them into this new class.
Test Plan: unit tests and played around in Differential a bit.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Maniphest Tasks: T2009
Differential Revision: https://secure.phabricator.com/D4351
Summary:
This commit doesn't change license of any file. It just makes the license implicit (inherited from LICENSE file in the root directory).
We are removing the headers for these reasons:
- It wastes space in editors, less code is visible in editor upon opening a file.
- It brings noise to diff of the first change of any file every year.
- It confuses Git file copy detection when creating small files.
- We don't have an explicit license header in other files (JS, CSS, images, documentation).
- Using license header in every file is not obligatory: http://www.apache.org/dev/apply-license.html#new.
This change is approved by Alma Chao (Lead Open Source and IP Counsel at Facebook).
Test Plan: Verified that the license survived only in LICENSE file and that it didn't modify externals.
Reviewers: epriestley, davidrecordon
Reviewed By: epriestley
CC: aran, Korvin
Maniphest Tasks: T2035
Differential Revision: https://secure.phabricator.com/D3886
Summary: this makes notifications work better for folks who choose to handle things in Phabricator and not over email
Test Plan: had my test account and "real" account battle each other on a few tasks and divs. Noted that I received emails appropos to the respective settings.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Maniphest Tasks: T1977
Differential Revision: https://secure.phabricator.com/D3856
Summary: See D3789. Same thing for Differential.
Test Plan: Created a new revision and made a comment. Verified reviewer got popup notifications but the in-app notifications were delivered already marked as read.
Reviewers: vrana, btrahan
Reviewed By: vrana
CC: aran
Maniphest Tasks: T1403
Differential Revision: https://secure.phabricator.com/D3790
Summary: When we receive an email, figure out if any of the other tos and ccs are users. If they are, pass their phids through the stach as "exclude phids" and exclude them from getting the email.
Test Plan: used the various applications (audit, differential, maniphest) and noted emails were sent as expected.
Reviewers: epriestley, vrana
Reviewed By: vrana
CC: aran, Korvin, vrana
Maniphest Tasks: T1676
Differential Revision: https://secure.phabricator.com/D3645
Summary: This is a minor quality-of-life improvement to prevent D2968 from being as nasty as it is.
Test Plan: Ran unit tests; generated Differential, Maniphest and Diffusion emails and verified the bodies looked sensible.
Reviewers: btrahan, vrana
Reviewed By: vrana
CC: aran
Maniphest Tasks: T931
Differential Revision: https://secure.phabricator.com/D2986
Summary:
- Assigning $cc_phids clobbers the correct value assigned on line 80.
- Remove stack trace noise, the trace is always meaningless and well-known.
- Actually show the original body.
Test Plan: Piped mail to the mail receiver and verified the errors didn't CC revision CCs, no longer had traces, and included the original raw text body.
Reviewers: btrahan, jungejason
Reviewed By: jungejason
CC: aran
Differential Revision: https://secure.phabricator.com/D2969
Summary: People want to create filters checking if they are in CC which is almost impossible with multiplexing in Outlook.
Test Plan: Sent e-mail with multiple CCs, verified headers.
Reviewers: epriestley, nh
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D2829
Summary:
Works this way:
- Select users' language with multiplexing.
- Select default language otherwise (it can be different from current user's language).
- Build body and subject for each user individually.
- Set the original language after sending the mails.
Test Plan:
- Comment on a diff of user with custom translation.
- Set default to a custom translation. Comment on a diff of user with default translation.
- Set default to a default translation. Comment on a diff of user with default translation.
Repeat with/without multiplexing.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Maniphest Tasks: T1139
Differential Revision: https://secure.phabricator.com/D2774
Summary:
This is the first step in Phabricator internationalization.
It adds a translation selector and calls it at startup.
Installations can add custom selectors to override some texts.
We can add official translations in future.
Next step is to allow user to choose his translation which will override the global one.
This is currently used only for English plurals.
Test Plan: Displayed a diff with unit test error, verified that it says 'Detail' or 'Details' and not 'Detail(s)'.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Maniphest Tasks: T1139
Differential Revision: https://secure.phabricator.com/D2753
Summary:
Some e-mail clients display this header and it needs to be constant.
This is somehow involved but I doubt that there is a simpler solution.
Test Plan:
Applied SQL patch.
Commented on revision, commented on commit, changed package.
Verified that the `Thread-Topic` has constant and human readable value.
Reviewers: epriestley
Reviewed By: epriestley
CC: ola, aran, Korvin
Differential Revision: https://secure.phabricator.com/D2745
Summary: NOTE: This can break current ongoing conversations.
Test Plan: Commented on a revision and checked the header in the e-mail.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Maniphest Tasks: T1340
Differential Revision: https://secure.phabricator.com/D2723
