Summary:
Fixes T10519. Two issues:
First, the acting user wasn't explicitly included in the mail. This usually didn't matter, but could matter if you unsubscribed and then interacted.
Second, we had some logic which tried to hide redundant "added inline comment" transactions, but could hide them inappropriately. In particular, if another action (like a subscribe) was present in the same group, we could hide the inlines because of that other transaction, then //also// hide the subscribe. This particular issue is likely an unintended consequence of hiding self-subscribes.
Instead of hiding inlines if //anything else// happened, hide them only if:
- there is another "added a comment" transaction; or
- there is another "added an inline comment" transaction.
This prevents the root issue in T10519 (incorrectly hiding every transaction, and thus not sending the mail) and should generally make behavior a little more consistent and future-proof.
Test Plan:
- Submitted //only// an inline comment on a commit I had not previously interacted with.
- Before patch: no mail was generated (entire mail was improperly hidden).
- After patch: got some mail with my comment.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10519
Differential Revision: https://secure.phabricator.com/D15407
Summary: Ref T4245. These are all straightforward to remove.
Test Plan:
- Edited paths in a package.
- Ran `bin/audit delete --repositories ...` with various identifiers.
- Searched by repository for `R3`, `rAAAA` in Harbormaster.
- Did a Herald dry run on a commit.
- Browsed commits, made comments.
- Viewed a Releeph product list.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T4245
Differential Revision: https://secure.phabricator.com/D14927
Summary: Ref T10032, adds "Basic" NUX to more applications.
Test Plan: Visit each with ?nux=true and click on the create link. T10032 is tracking which apps need general modernization to pick up these changes.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Maniphest Tasks: T10032
Differential Revision: https://secure.phabricator.com/D14847
Summary:
Ref T9132. When I've touched `PhabricatorApplication` I keep hitting this bad `pht()` junk.
The warning is correct, these strings are not extactable and can not be translated.
Fix it so they can be extracted and translated.
Broadly, in all cases we want to render one of these:
> 95 Things (for fewer than some limit)
> 99+ Things (when we hit the limit)
Test Plan: Looked at homepage status counts, moused over them, saw reasonable strings. Grepped for removed method.
Reviewers: chad
Reviewed By: chad
Subscribers: joshuaspence
Maniphest Tasks: T9132
Differential Revision: https://secure.phabricator.com/D14638
Summary:
Ref T9845. In Differential, this is not a remarkup block -- it's a mail section. `addTextSection()` has special magic behavior when handed a prebuilt section since D9375.
Swapping to `addRemarkupSection()` causes the error in T9845 and renders nothing in the comment section.
Even if it were a block of text, it would not be appropriate to add it as remarkup. This would incorrectly render comments in files like `__init__.py`, which are common on Python (the filename would render as "__init__.py"). Okay that's a bad example since it works fine but, uh, a file named `T123` would be no good or whatever.
I'll realign T9845 to clean this up and fix it more durably.
Test Plan: Sent myself some mail with inline comments, saw them in the mail.
Reviewers: joshuaspence, chad
Reviewed By: chad
Maniphest Tasks: T9845
Differential Revision: https://secure.phabricator.com/D14589
Summary: Ref T992. I noticed that `ManiphestTask` mail doesn't render Remarkup properly (instead, it renders Remarkup literally). I //think// this is because the code calls `addTextSection()` rather than `addRemarkupSection()`.
Test Plan: Created a new Maniphest Task and saw Remarkup in the generated self-email (inspect the email contents with `./bin/mail show-outbound`). I didn't test the other affected applications.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: Korvin
Maniphest Tasks: T992
Differential Revision: https://secure.phabricator.com/D14511
Summary: Fixes T9434. I'm not sure exactly what changed behavior here, but we need a `needAuditRequests()`.
Test Plan: Ran a query which hit the exception (empty query was good enough, locally), then applied this patch; saw exception go away.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T9434
Differential Revision: https://secure.phabricator.com/D14162
Summary:
Fixes T9427. Currently, replies to audits/commits go to "Cxxx", but so do replies to countdowns.
There is non real non-disruptive approach available here and this seems least-bad.
Test Plan:
- Made a comment on a commit.
- Fished the reply-to address out of `bin/mail list-oubound` + `bin/mail show-outbound` (it was now "COMMIT...").
- Sent mail to that address.
- Grabbed the raw message and wrote it to `mail.txt`.
- Ran `cat mail.txt | ./scripts/mail/mail_handler.php --process-duplicates`.
- Used `bin/mail list-inbound` + `bin/mail show-inbound` to verify receipt.
- Saw comment appear on audit.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T9427
Differential Revision: https://secure.phabricator.com/D14163
Summary:
Fixes T9279. Modernizes the SearchEngine and Query classes. User-facing changes:
- Added order by commit date, default to order by commit date with newest commits first.
- Added explicit "Needs Audit by".
- Added new `packages(...)` typeahead function.
- Picked up automatic subscribers, projects, and order fields.
This changes behavior a little bit: we previously attempted to exclude, e.g., commits which a package you own needs to audit, but which you have resigned from. This is difficult in general and I think it needs a more comprehensive solution. This shouldn't impact users much, anyway.
Test Plan: {F767628}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T9279
Differential Revision: https://secure.phabricator.com/D14013
Summary:
Ref T8672. Ref T9187. Root issue in at least one case is:
- User makes a commit including a file with some non-UTF8 text (say, a Japanese file full of Shift-JIS).
- We pass the file to the TransactionEditor so it can inline or attach the patch if the server is configured for these things.
- When inlining patches, we convert them to UTF8 before inlining. We must do this since the rest of the mail is UTF8.
- When attaching patches, we send them in the original encoding (as file attachments). This is correct, and means we need to give the worker the raw patch in whatever encoding it was originally in: we can't just convert it to utf8 earlier, or we'd attach the wrong patch in some cases.
- TransactionEditor does its thing (e.g., creates the commit), then gets ready to send mail about whatever it did.
- The publishing work now happens in the daemon queue, so we prepare to queue a PublishWorker and pass it the patch (with some other data).
- When we queue workers, we serialize the state data with JSON.
So far, so good. But this is where things go wrong:
- JSON can't encode binary data, and can't encode Shift-JIS. The encoding silently fails and we ignore it.
Then we get to the worker, and things go wrong-er:
- Since the data is bad, we fatal. This isn't a permanent failure, so we continue retrying the task indefinitely.
This applies several fixes:
# When queueing tasks, fail loudly when JSON encoding fails.
# In the worker, fail permanently when data can't be decoded.
# Allow Editors to specify that some of their data is binary and needs special handling.
This is fairly messy, but some simpler alternatives don't seem like good ways forward:
- We can't convert to UTF8 earlier, because we need the original raw patch when adding it as an attachment.
- We could encode //only// this field, but I suspect some other fields will also need attention, so that adding a mechanism will be worthwhile. In particular, I suspect filenames //may// be causing a similar problem in some cases.
- We could convert task data to always use a serialize()-based binary safe encoding, but this is a larger change and I think it's correct that things are UTF8 by default, even if it makes a bit of a mess. I'd rather have an explicit mess like this than a lot of binary data floating around.
The change to make `LiskDAO` will almost certainly catch some other problems too, so I'm going to hold this until after `stable` is cut. These problems were existing problems (i.e., the code was previously breaking or destroying data) so it's definitely correct to catch them, but this will make the problems much more obvious/urgent than they previously were.
Test Plan:
- Created a commit with a bunch of Shift-JIS stuff in a file.
- Tried to import it.
Prior to patch:
- Broken PublishWorker with distant, irrelevant error message.
With patch partially applied (only new error checking):
- Explicit, local error message about bad key in serialized data.
With patch fully applied:
- Import went fine and mail generated.
Reviewers: chad
Reviewed By: chad
Subscribers: devurandom, nevogd
Maniphest Tasks: T8672, T9187
Differential Revision: https://secure.phabricator.com/D13939
Summary:
Minor layout updates to Ref T8099
- Timeline tweaks
- Use Lato headers in Document Obj Headers
- Minor Remarkup
- Add Audit Icons
(Unclear if Audit is "correct", ie Status vs. Commit Status) But added icons anyways if needed.
Test Plan: Review each of the updated elements
Reviewers: btrahan, epriestley
Reviewed By: epriestley
Subscribers: epriestley, Korvin
Maniphest Tasks: T8099
Differential Revision: https://secure.phabricator.com/D13357
Summary: Ref T8099. This adds a new class which all search engines return for layout. I thought about this a number of ways, and I think this is the cleanest path. Each Engine can return whatever UI bits they needs, and AppSearch or Dashboard picks and lays the bits out as needed. In the AppSearch case, interfaces like Notifications, Calendar, Legalpad all need more custom layouts. I think this also leaves a resonable path forward for NUX as well. Also, not sure I implemented the class correctly, but assume thats easy to fix?
Test Plan: Review and do a search in each application changed. Grep for all call sites.
Reviewers: btrahan, epriestley
Reviewed By: epriestley
Subscribers: epriestley, Korvin
Maniphest Tasks: T8099
Differential Revision: https://secure.phabricator.com/D13332
Summary: All classes should extend from some other class. See D13275 for some explanation.
Test Plan: `arc unit`
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: epriestley, Korvin
Differential Revision: https://secure.phabricator.com/D13283
Summary: Ref T8455. Use the standard actions in commit rules, instead of a custom action.
Test Plan: Wrote an "add cc" Herald rule for commits, pushed a commit, saw the rule fire correctly.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T8455
Differential Revision: https://secure.phabricator.com/D13181
This fixes this, which only crops up some of the time:
```
Daemon 39827 STDE [Thu, 04 Jun 2015 03:03:11 +0000] [2015-06-04 03:03:10] EXCEPTION: (PhutilProxyException) Error while executing Task ID 902271. {>} (PhabricatorDataNotAttachedException) Attempting to access attached data on PhabricatorRepositoryCommit (via getCommitData()), but the data is not actually attached. Before accessing attachable data on an object, you must load and attach it.
Daemon 39827 STDE [Thu, 04 Jun 2015 03:03:11 +0000]
Daemon 39827 STDE [Thu, 04 Jun 2015 03:03:11 +0000] Data is normally attached by calling the corresponding needX() method on the Query class when the object is loaded. You can also call the corresponding attachX() method explicitly. at [<phabricator>/src/infrastructure/storage/lisk/PhabricatorLiskDAO.php:166]
Daemon 39827 STDE [Thu, 04 Jun 2015 03:03:11 +0000] arcanist(head=master, ref.master=8c589f1f759f), phabricator(head=master, ref.master=4a45620b0458), phutil(head=master, ref.master=afc05a9a7f00)
Daemon 39827 STDE [Thu, 04 Jun 2015 03:03:11 +0000] #0 <#2> PhabricatorLiskDAO::assertAttached(string) called at [<phabricator>/src/applications/repository/storage/PhabricatorRepositoryCommit.php:129]
Daemon 39827 STDE [Thu, 04 Jun 2015 03:03:11 +0000] #1 <#2> PhabricatorRepositoryCommit::getCommitData() called at [<phabricator>/src/applications/audit/editor/PhabricatorAuditEditor.php:669]
Daemon 39827 STDE [Thu, 04 Jun 2015 03:03:11 +0000] #2 <#2> PhabricatorAuditEditor::buildMailBody(PhabricatorRepositoryCommit, array) called at [<phabricator>/src/applications/transactions/editor/PhabricatorApplicationTransactionEditor.php:2178]
Daemon 39827 STDE [Thu, 04 Jun 2015 03:03:11 +0000] #3 <#2> PhabricatorApplicationTransactionEditor::sendMailToTarget(PhabricatorRepositoryCommit, array, PhabricatorMailTarget) called at [<phabricator>/src/applications/transactions/editor/PhabricatorApplicationTransactionEditor.php:2152]
Daemon 39827 STDE [Thu, 04 Jun 2015 03:03:11 +0000] #4 <#2> PhabricatorApplicationTransactionEditor::sendMail(PhabricatorRepositoryCommit, array) called at [<phabricator>/src/applications/transactions/editor/PhabricatorApplicationTransactionEditor.php:998]
Daemon 39827 STDE [Thu, 04 Jun 2015 03:03:11 +0000] #5 <#2> PhabricatorApplicationTransactionEditor::publishTransactions(PhabricatorRepositoryCommit, array) called at [<phabricator>/src/applications/transactions/worker/PhabricatorApplicationTransactionPublishWorker.php:21]
Daemon 39827 STDE [Thu, 04 Jun 2015 03:03:11 +0000] #6 <#2> PhabricatorApplicationTransactionPublishWorker::doWork() called at [<phabricator>/src/infrastructure/daemon/workers/PhabricatorWorker.php:91]
Daemon 39827 STDE [Thu, 04 Jun 2015 03:03:11 +0000] #7 <#2> PhabricatorWorker::executeTask() called at [<phabricator>/src/infrastructure/daemon/workers/storage/PhabricatorWorkerActiveTask.php:162]
Daemon 39827 STDE [Thu, 04 Jun 2015 03:03:11 +0000] #8 <#2> PhabricatorWorkerActiveTask::executeTask() called at [<phabricator>/src/infrastructure/daemon/workers/PhabricatorTaskmasterDaemon.php:20]
Daemon 39827 STDE [Thu, 04 Jun 2015 03:03:11 +0000] #9 PhabricatorTaskmasterDaemon::run() called at [<phutil>/src/daemon/PhutilDaemon.php:185]
Daemon 39827 STDE [Thu, 04 Jun 2015 03:03:11 +0000] #10 PhutilDaemon::execute() called at [<phutil>/scripts/daemon/exec/exec_daemon.php:125]
```
Auditors: btrahan
Summary:
Ref T6367. Do all mail, feed, notification and search stuff from the daemons, in all editors.
There are four relatively-stateful editors (Audit, Differential, Phriction, PhortuneCart) which needed special care to move state into the daemons properly.
Beyond that, I moved mailTo/mailCC/feedRelated/feedNotify to be computed before we enter the worker:
- This is simpler, since a lot of editors rely on being able to call `$object->getReviewers()` or similar to compute them.
- This is more correct, since we want to freeze the lists at this moment in time.
Finally, I renamed `loadEdges` to `willPublish` and made it a slightly more general hook.
---
This is a bit fragile and I'm not //thrilled// about it.
It would probably be cleaner to have separate Editor and Publisher classes (something like @fabe's D11329 did). However, I think that's quite a lot of work, and I'd like to see stronger motivation for it (either in this actually being more fragile than I think, or there being other things we get out of it). Overall, I'm comfortable with this change, just definitely not a big fan of the "save" + "load" pattern since I think it's really fragile, nonobvious, hard to debug/predict, etc.
Test Plan:
Directly updated editors:
- Created a new Phriction page, saw "Document Content".
- Edited a Phriction page, saw "Document Diff".
- Edited a revision, got normal looking mail.
- Faked in `changedPriorToCommitURI` and verified it survived the state boundary.
- Sent Audit mail.
- Sent invoice mail.
Indirect editors - for these, I just made a change and made sure the mail generated:
- Updated a paste.
- Updated an event.
- Updated a thread.
- Updated a task.
- Updated a mock.
- Updated a question.
- Updated a project.
- Updated a file.
- Updated an initiative.
- Updated a Legalpad document.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley, fabe
Maniphest Tasks: T6367
Differential Revision: https://secure.phabricator.com/D13115
Summary: Ref T8099, Fixes T8341. Switches all remaining callsites to setBarColor to use setStatusIcon (sans workboards).
Test Plan: Test each of the applications I changed as I could (not Releeph).
Reviewers: btrahan, epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T8341, T8099
Differential Revision: https://secure.phabricator.com/D13059
Summary:
Ref T7447. Implements per-viewer comment hiding. Once a comment is obsolete or uninteresting, you can hide it completely.
This is sticky per-user.
My hope is that this will strike a better balance between concerns than some of the other approaches (conservative porting, summarization, hide-all).
Specifically, this adds a new action here:
{F435621}
Clicking it completely collapses the comment into a small icon on the previous line, and saves the comment state as hidden for you:
{F435626}
You can click the icon to reveal all hidden comments below the line.
Test Plan:
- Hid comments.
- Showed comments.
- Created, edited, deleted and submitted comments.
- Used Diffusion comments (hiding is not implemented there yet, but I'd plan to bring it there eventually if it works out in Differential).
Reviewers: btrahan, chad
Reviewed By: btrahan
Subscribers: jparise, yelirekim, epriestley
Maniphest Tasks: T7447
Differential Revision: https://secure.phabricator.com/D13009
Summary: This data is available and loaded, just not used.
Test Plan: {F411442}
Reviewers: chad
Reviewed By: chad
Subscribers: epriestley
Differential Revision: https://secure.phabricator.com/D12951
Summary: Fixes T8264. Broken in D12929. Sweep all the applyBuiltin implementations and always break; rather than return
Test Plan: added myself to a project successfully (showed up as a member)
Reviewers: epriestley, chad
Subscribers: Korvin, epriestley
Maniphest Tasks: T6403, T8264
Differential Revision: https://secure.phabricator.com/D12940
Summary: Ref T6403. Conpherence keeps track of comments for message counts so we needed some special attention there. Otherwise, straight-forward.
Test Plan: left a comment on a diff with inline comments. sent messages in conpherence successfully. verified unread count incremented correctly for sent messages for users.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T6403
Differential Revision: https://secure.phabricator.com/D12932
Summary: Ref T6403. This one was pretty easy since no one does anything custom with subscribers.
Test Plan: subscribed / unscribed to a random commit ("audit"). joined / left, watched / unwatched a project
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T6403
Differential Revision: https://secure.phabricator.com/D12930
Summary: Ref T6403. This does TYPE_EDGE since I just had to deal with T8252. Look like this fixes a few editors (maybe) that would have had fatals with mentions like slowvote and ponder.
Test Plan: made a phame post mentioning a task and it worked! joined / left a project, watched / unwatched a project and that worked! blind faith for other sites.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T6403
Differential Revision: https://secure.phabricator.com/D12929
Summary: Currently adding auditors in commit message only works with the plural. Make "Auditor: bla" work.
Test Plan: Unit tests. I don't know if I am supposed to add a test.
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: joshuaspence, epriestley
Differential Revision: https://secure.phabricator.com/D10636
Summary:
New, cleaner, ObjectItemLists. Lots of minor style tweaks, basic overview:
- Remove FootIcons
- Remove Stackable
- Remove Plain List
- Add StatusIcon
- Add setting ObjectList to an ObjectBox
- Minor retouches to Headers
Mostly, this should give us an idea of life with the new Object Lists. I'll take another application by application pass down the road. This mostly looks at implementation in Maniphest, Differential, Audit, Workboards. Checked a few other areas and dialogs while testing, and everything looks square.
Test Plan: Maniphest, Differential, Homepage, Audit, People, and other applications. Drag reorder, etc.
Reviewers: btrahan, epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Differential Revision: https://secure.phabricator.com/D12865
Summary: Use `__CLASS__` instead of hard-coding class names. Depends on D12605.
Test Plan: Eyeball it.
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: hach-que, Korvin, epriestley
Differential Revision: https://secure.phabricator.com/D12806
Summary: This class seems to be unused (or maybe I am missing something).
Test Plan: Unit tests still pass.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: Korvin, epriestley
Differential Revision: https://secure.phabricator.com/D12565
Summary:
Ref T7447. This ports comments forward and backward in the best case:
- The old comment is on a changeset with the same filename.
- The old and new files are pretty much the same, line-for-line.
This will fail to port a lot of comments around and probably port a lot of comments into goofy places. We can see how bad it is in practice.
Errata:
- Design is me cobbling something together, probably worth tweaking.
- "Old Comment" should, at a minimum, say "Newer Comment" sometimes, or we should come up with some better name for this stuff.
Test Plan: {F377214}
Reviewers: chad, btrahan
Reviewed By: btrahan
Subscribers: johnny-bit, yelirekim, epriestley
Maniphest Tasks: T7447
Differential Revision: https://secure.phabricator.com/D12484
Summary:
Ref T7803. Ref T5873. I want to drive Conduit through more shared infrastructure, but can't currently add parameters automatically.
Put a `getX()` around the `defineX()` methods so the parent can provide default behaviors.
Also like 60% of methods don't define any special error types; don't require them to implement this method. I want to move away from this in general.
Test Plan:
- Ran `arc unit --everything`.
- Called `conduit.query`.
- Browsed Conduit UI.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: hach-que, epriestley
Maniphest Tasks: T5873, T7803
Differential Revision: https://secure.phabricator.com/D12380
Summary:
...because its always at least the string <ATTACHABLE>... Not sure when we'd hit this / see the TODO about making it better, but its definitely a logic bug right now.
(an update to D12347 helped me notice that this conditional is always hit and may fatal later)
Test Plan: NA
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Differential Revision: https://secure.phabricator.com/D12348
Summary:
Fixes T7731. When a user writes a "Send me an email" rule, always try send them an email, even if their notification settings would normally downgrade it to a notification.
In particular, this is stronger than these downgrades:
- Downgrades due to "self actions";
- downgrades due to "mail tags".
Test Plan:
- Wrote various Herald rules with "Send me an email" rules.
- Used `bin/mail list-outbound` / `show-outbound` to vet generated mail.
- Mail reacted properly to a variety of conditions (disabled accounts, settings, "send me an email" rule, forced delivery).
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T7731
Differential Revision: https://secure.phabricator.com/D12300
Summary:
Fixes T7199. This still isn't a shining example of perfect code, but the raw amount of copy/paste is much lower than it used to be.
- Reduce code duplication between existing receivers.
- Expose receiving objects in help menus where appropriate.
- Connect some "TODO" receivers.
Test Plan:
- Sent mail to every supported object type.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T7199
Differential Revision: https://secure.phabricator.com/D12249
Summary:
Ref T7199. Convert the single help menu item into a dropdown and allow applications to list multiple items there.
When an application has mail command objects, link them in the menu.
Test Plan:
{F355925}
{F355926}
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T7199
Differential Revision: https://secure.phabricator.com/D12244
Summary: Ref T7199. Half of these aren't even reachable, but make some progress toward reducing the amount of nonsense and garbage in mail handling.
Test Plan: Tested all reachable handlers with `bin/mail receive-test`.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T7199
Differential Revision: https://secure.phabricator.com/D12237
