Summary: Ref T10978. This is just a maintenance convenience script. It can fix up overall commit state after you `bin/audit delete` stuff or nuke a bunch of stuff from the database, as I did on `secure.phabricator.com`.
Test Plan: Ran `bin/audit synchronize`, and `bin/audit update-owners`.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10978
Differential Revision: https://secure.phabricator.com/D17271
Summary: Ref T10978. This code (mostly related to the old ADD_AUDIT transaction and some to the "store English text in the database" audit reasons) is no longer reachable.
Test Plan:
Grepped for removed symbols:
- withAuditStatus
- getActionNameMap (unrelated callsites exist)
- getActionName (unrelated callsites exist)
- getActionPastTenseVerb
- addAuditReason
- getAuditReasons
- auditReasonMap
Also audited some commits.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10978
Differential Revision: https://secure.phabricator.com/D17267
Summary:
Ref T10978. Currently, during commit import, we write an "Audit Not Required" auditor for commits which don't require an audit.
This auditor is used to power the "Commits in this package" query in Owners.
This conflates audits and commit/package membership. I think it might even predate edges. Code needs to dance around this mess and we get the wrong result in some cases, since auditors are now editable.
Instead, write an explicit edge which just says "this commit is part of such-and-such packages". Then use that to run the query. Logical!
I'll issue guidance on this but I'm not migrating it, since it fixes itself going forward and only really affects the UI in Owners.
Test Plan:
- Ran `bin/audit update-owners` with various arguments.
- Viewed packages in web UI, saw them load the proper commits.
- Queried by packages in Diffusion explicitly.
- Clicked the "View All" link in Owners and got to the right search UI.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10978
Differential Revision: https://secure.phabricator.com/D17264
Summary:
Ref T10978. Updates how we implement "Auditors: ..." in commit messages:
- Use the same parsing code as everything else.
- (Also: parse package names.)
- Use the new transaction code.
Also, fix some UI strings.
Test Plan: Used `bin/repository reparse --herald <commit>` to re-run this code on commits with various messages (valid Auditors, invalid Auditors, no Auditors). Saw appropriate auditors added in the UI.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10978
Differential Revision: https://secure.phabricator.com/D17262
Summary:
Ref T10978. Currently, too many "This audit now <something something>" transactions are posting, because this strict `===` check is failing to detect that the audit is already in the same state.
This is because audit states are currently integers, and saving an integer to the database and then reading it back turns it into a string. This is a whole separate can of worms. For now, just weaken the comparison. I'd eventually like to use string constants here instead of integer constants.
Test Plan:
Commented on a "no audit required" commit, didn't see a double "this doesn't need audit" transaction anymore.
Also made a legit state change and did see a state transaction.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10978
Differential Revision: https://secure.phabricator.com/D17258
Summary:
Fixes T2393. This allows authors to explicitly say "I think I fixed everything, please accept my commit now thank you".
Also improves behavior of "re-accept" and "re-reject" after new auditors you have authority over get added.
Test Plan:
- Kicked a commit back and forth between an author and auditor by alternately using "Request Verification" and "Raise Concern".
- Verified it showed up properly in bucketing for both users.
- Accepted, added a project, accepted again (works now; didn't before).
- Audited on behalf of projects / packages.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T2393
Differential Revision: https://secure.phabricator.com/D17252
Summary:
Ref T2393. We had three copies of this code ("which packages/projects can a user accept on behalf of?"). I removed one in D17250. This consolidates the other two.
This still isn't perfect and it should probably live in a Query or something some day, but there's some weird stuff going on with the viewer in the editor context, and at least the code handles the viewer correctly now and isn't living somewhere weird and totally unrelated to auditing, and the callsites don't need to do a bunch of extra work.
This also moves towards fixing the "re-accept if you've already accepted but then a new package you have authority over was added" bug, which we fixed recently in Differential. This should be less common in Audit, but should still be fixed.
Test Plan: Viewed and audited commits with a mixture of user, package, and project auditors. Saw actions apply to the expected set of auditors.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T2393
Differential Revision: https://secure.phabricator.com/D17251
Summary:
Ref T2393. This code is no longer reachable (we never had an API for auditing in Diffusion) and unused. Clean it up before implementing new states/actions.
(Note that code for displaying these transactions still needs to stick around for a bit, we'll just never apply new ones from here on out. They've been replaced with modular transactions.)
Test Plan: Grepped for usage, commentd on / audited a commit.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T2393
Differential Revision: https://secure.phabricator.com/D17250
Summary: Ref T2393. This adds a state-change transaction hint to Audit, like we have in Differential. This is partly for consistency and partly to make it more clear what should happen next.
Test Plan: {F2477848}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T2393
Differential Revision: https://secure.phabricator.com/D17243
Summary:
Fixes T6024. Ref T12121. Currently, we show build status in commit history tables; show audit status alongside it.
Also:
- Change the "Author/Committer" header to just "Author"; I think it's reasonably obvious what "x/y" means (if you can't guess, you can click the commit and likely figure it out) and this gives us a little more space.
- Make the audit list look more like the corresponding list in Differential, with similar formatting.
Test Plan:
- Viewed history of a repostiory, saw audit status.
- Viewed a merge commit, saw audit status in the list of merged commits.
- Viewed a commit search results list.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T12121, T6024
Differential Revision: https://secure.phabricator.com/D17227
Summary: Ref T10978. Handle loads can be batched a bit more efficiently by doing them upfront.
Test Plan: Queries dropped a bit locally, but I mostly have the same autors/auditors. I'm seeing 286 queries on my account in production, so I'll check what happens with that.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10978
Differential Revision: https://secure.phabricator.com/D17225
Summary: Fixes T12117. I typed or copy/pasted this constant wrong while refactoring during T10978.
Test Plan: Called `audit.query`.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T12117
Differential Revision: https://secure.phabricator.com/D17218
Summary: Fixes T6660. Uses the new stuff in Audit to build an EditEngine-aware icon.
Test Plan: {F2364304}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T6660
Differential Revision: https://secure.phabricator.com/D17208
Summary:
Ref T9482. These may need a little more work (feel free to shoot me a counter-diff) but try to:
- Never use only color to distinguish between states (for colorblind, etc users).
- Give the "nothing needs to be done" state a more obvious "okay" icon (instead of a question mark).
Test Plan: Looked at some linked commits in Maniphest, the icons made a bit more sense?
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T9482
Differential Revision: https://secure.phabricator.com/D17203
Summary:
Fixes T9482. Historically, Audit was somewhat confused about whether queries and views should act on the viewer's status or the object's status.
This realigns Audit to work like Differential: we show overall status for the commit, just like we show overall status for revisions. This better aligns with expectation and isn't weird/confusing, and bucketing should handle all the "what do //I// need to do" stuff now (or, at least, seems to have in Differential).
This is also how every other type of object works in every other application, AFAIK (all of them show object status, not viewer's-relationship-to-the-object status).
Test Plan:
- Viewed commit lists in Owners and Audit.
- Saw commit overall statuses, not my personal status.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T9482
Differential Revision: https://secure.phabricator.com/D17195
Summary:
Fixes T9430. Fixes T9362. Fixes T9544. This changes the default view of Audit to work like Differential, where commits you need to audit or respond to are shown in buckets.
This is a bit messy and probably needs some followups. This stuff has changed from a compatibility viewpoint:
- The query works differently now (but in a better, modern way), so existing saved queries will need to be updated.
- I've removed the counters from the home page instead of updating them, since they're going to get wiped out by ProfileMenu soon anyway.
- When bucketed queries return too many results (more than 1,000) we now show a warning about it. This isn't greaaaat but it seems good enough for now.
Test Plan: {F2351123}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T9430, T9362, T9544
Differential Revision: https://secure.phabricator.com/D17192
Summary: Ref T10978. Until T10448 makes mail tags modular, keep the old tags working.
Test Plan: Made some commit edits, ran `bin/phd debug task` to process mail for them.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10978
Differential Revision: https://secure.phabricator.com/D17190
Summary:
Fixes T6630. Long ago, "Audit", "Diffusion" and "Repositories" were three totally separate applications.
This separation isn't useful and the three rapidly became intertwined. Ideally, they would all be one application.
This doesn't take us quite that far, but Audit no longer has any controllers and has little actual behavior.
The "Audit" screen has always just been a SearchEngine view of commits with some filters on it, and this formalizes that and puts a link to it in Diffusion. (This view has other uses, too.)
Test Plan:
- Accessed audit from home page.
- Accessed audit/commits from Diffusion.
- Could no longer uninstall Audit on its own.
- Grepped for `/audit/` and `AuditApplication`.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T6630
Differential Revision: https://secure.phabricator.com/D17186
Summary:
Fixes T8739. Currently, Diffusion inline comments in the timeline are sorted arbitrarily, mostly by creation order.
Instead, sort them by line number, like Differential.
Test Plan:
Made comments in "C", "B", "A" order, saw them in line order after submit:
{F2343032}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T8739
Differential Revision: https://secure.phabricator.com/D17184
Summary: Ref T10978. Ref T7676. Make auditors work more like reviewers, so they can be freely added or removed.
Test Plan:
- Interacted with auditors via "Edit Commit" and API.
- Comment area is still oldschool and doesn't work yet.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10978, T7676
Differential Revision: https://secure.phabricator.com/D17181
Summary:
Fixes T11610. Clean up some sketchy old code from long ago.
If you had rules that use conditions like "Accepted revision exists" and ran them in the test console, we'd never load the "CommitData" and fatal.
Instead, load CommitData in `newTestAdapter()` and generally make these pathways a little more modern.
Test Plan:
- Wrote an "Accepted Revision Exists" rule.
- Ran a commit in the test console.
- Before patch, got fatal from T11610.
- After patch, got clean test result.
- Also pushed a commit and reviewed the transcript to make sure the rule ran properly.
Reviewers: joshuaspence, chad
Reviewed By: chad
Maniphest Tasks: T11610
Differential Revision: https://secure.phabricator.com/D16522
Summary:
Ref T11035. This only fixes half of the issue: comment editing has been fixed, but normal transactions which edit things like descriptions haven't yet.
The normal edits aren't fixed because the "oldValues" are populated too late. The code should start working once they get populated sooner, but I don't want to jump the gun on that since it'll probably have some spooky effects. I have some other transaction changes coming down the pipe which should provide a better context for testing "oldValue" population order.
Test Plan:
- Mentioned `@dog` in a comment.
- Removed `@dog` as a subscriber.
- Edited the comment, adding some unrelated text at the end (e.g., fixing a typo).
- Before change: `@dog` re-added as subscriber.
- After change: no re-add.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T11035
Differential Revision: https://secure.phabricator.com/D16108
Summary:
Fixes T11017. We add packages as "uninteresting" auditors so that we can query commits by package later.
Until recently, this didn't matter because we didn't send mail to packages. But now we do, so stop mailing them when they don't actually need to do anything.
Test Plan:
- Made a commit to a file which was part of a package but which I owned (so it does not trigger auditing).
- `var_dump()`'d mail "To:" PHIDs.
- Before patch: included package.
- After patch: no package.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T11017
Differential Revision: https://secure.phabricator.com/D15970
Summary:
Ref T10939. Fixes T10174. We can currently trigger "uninteresting" auditors in two ways:
- Packages with auditing disabled ("NONE" audits).
- Packages with auditing enabled, but they don't need an audit (e.g., author is a pacakge owner; "NOT REQUIRED" audits).
These audits aren't interesting (we only write them so we can list "commits in this package" from other UIs) but right now they take up the audit slot. In particular:
- They show in the UI, but are generally useless/confusing nowadays. The actual table of contents does a better job of just showing "which packages do these paths belong to" now, and shows all packages for each path.
- They block Herald from adding real auditors.
Change this:
- Don't show uninteresting auditors.
- Let Herald upgrade uninteresting auditors into real auditors.
Test Plan:
- Ran `bin/repository reparse --owners <commit> --force`, and `--herald` to trigger Owners and Herald rules.
- With a package with auditing disabled, triggered a "None" audit and saw it no longer appear in the UI with the patch applied.
- With a package with auditing disabled, added a Herald rule to trigger an audit. With the patch, saw it go through and upgrade the audit to "Audit Required".
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10174, T10939
Differential Revision: https://secure.phabricator.com/D15940
Summary:
Ref T10537. For Nuance, I want to introduce new sources (like "GitHub" or "GitHub via Nuance" or something) but this needs to modularize eventually.
Split ContentSource apart so applications can add new content sources.
Test Plan:
This change has huge surface area, so I'll hold it until post-release. I think it's fairly safe (and if it does break anything, the breaks should be fatals, not anything subtle or difficult to fix), there's just no reason not to hold it for a few hours.
- Viewed new module page.
- Grepped for all removed functions/constants.
- Viewed some transactions.
- Hovered over timestamps to get content source details.
- Added a comment via Conduit.
- Added a comment via web.
- Ran `bin/storage upgrade --namespace XXXXX --no-quickstart -f` to re-run all historic migrations.
- Generated some objects with `bin/lipsum`.
- Ran a bulk job on some tasks.
- Ran unit tests.
{F1190182}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10537
Differential Revision: https://secure.phabricator.com/D15521
Summary: This updates (all?) of Diffusion/Audit to new UI, included edit and other extra form pages. It's fairly complete but I don't know all the nooks and crannies so to speak to fully verify I didn't mess anything up.
Test Plan: Tested creating new repositories, browsing, searching, auditing. Need more eyes.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D15487
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
