Summary: This adds back the title to the header link and scans through the codebase for instances where
Test Plan: Tested as many ObjectItemLists as I could find (each app homepage), there may be outliers, but can resolve those individually.
Reviewers: btrahan, epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Differential Revision: https://secure.phabricator.com/D10961
Summary: Ref T3669. Probably. Adds a yellow warning at the top of the Diff View and makes the comment draft icon yellow on lists of revisions.
Test Plan:
Test a diff with many warnings, see warning. Test a diff with draft comments, see warning. Test new icon in list view.
{F230133}
{F230134}
Reviewers: btrahan, epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T3669
Differential Revision: https://secure.phabricator.com/D10789
Summary: Fixes T6699. We need to "loadInlineComments" consistently, though for an unexpected reason - this mutates the $changesets to include all $changesets that have an associated inline comment, which is necessary to make them render properly.
Test Plan: Took a diff with inline comments and updated it, noting the inline comments disappeared. applied this patch and the inlines reappeared.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T6699
Differential Revision: https://secure.phabricator.com/D10935
Summary: Fixes T6693.
Test Plan:
Made a bunch of comments on a diff with differential, being sure to leave inlines here and there. This reproduced the issue in T6693. With this patch this issue no longer reproduces!
Successfully "showed older changes" in Maniphest too.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T6693
Differential Revision: https://secure.phabricator.com/D10931
Summary:
Ref T4712. Specifically...
- Differential
- needed getApplicationTransactionViewObject() implemented
- Audit
- needed getApplicationTransactionViewObject() implemented
- Repository
- one object needed PhabricatorApplicationTransactionInterface implemented
- setShouldTerminate(true)
- Ponder
- BONUS BUG FIX - leaving a comment on an answer had a bad redirect URI
- both PonderQuestion and PonderAnswer needed PhabricatorApplicationTransactionInterface implemented
- setShouldTerminate(true) on both "history" controllers
- left a "TODO" on buildAnswers on the question view controller, which is non-standard and should be re-written eventually
- Phortune
- BONUS BUG FIX - fix new user "createNewAccount" code to not fatal
- PhortuneAccount, PhortuneMerchant, and PhortuneCart needed PhabricatorApplicationTransactionInterface implemented
- setShouldTerminate(true) on Account view, merchant view, and cart view controller
- Fund
- Legalpad
- Nuance
- NuanceSource needed PhabricatorApplicationTransactionInterface implemented
- Releeph (this product is kind of a mess...)
- HACKQUEST - had to manually create an arcanist project to even be able to make a "product" and get started...!
- BONUS BUG FIX - make sure to "setName" on product edit
- ReleephProject (should be ReleepProduct...?), ReleephBranch, and ReleepRequest needed PhabricatorApplicationTransactionInterface implemented
- Harbormaster
- HarbormasterBuildable, HarbormasterBuild, HarbormasterBuildPlan, and HarbormasterBuildStep all needed PhabricatorApplicationTransactionInterface implemented
- setShouldTerminate(true) all over the place
Test Plan: foreach application, viewed the timeline(s) and made sure they still rendered
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T4712
Differential Revision: https://secure.phabricator.com/D10925
Summary: I think this was a "hacked" sub thing that never got updated when we switched to a real editor? I am not 100% sure how these methods are used, so please let me know if I should expand my test plan. Fixes T6659.
Test Plan: made a diff from the web ui, looked up the phid from mysql, ran bin/remove destroy <phid>, and it worked!
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T6659
Differential Revision: https://secure.phabricator.com/D10911
Summary: Fixes T6658.
Test Plan: made a diff with no repository and default policy and it worked!
Reviewers: epriestley, chad
Reviewed By: chad
Subscribers: Korvin, epriestley
Maniphest Tasks: T6658
Differential Revision: https://secure.phabricator.com/D10910
Summary: See <https://phabricator.wikimedia.org/T906>. This behavior is a bug; we should remove the button if the user can't use the application.
Test Plan:
- With Macro uninstalled, did these things verifying the button vanished:
- Sent a user a message.
- Edited a revision.
- Edited repository basic information.
- Edited an initiative.
- Edited a Harbormaster build step.
- Added task comments.
- Edited profile blurb.
- Edited blog description.
- Commented on Pholio mock.
- Uploaded Pholio image.
- Edited Phortune merchant.
- Edited Phriction document.
- Edited Ponder answer.
- Edited Ponder question.
- Edited Slowvote poll.
- Edited a comment.
- Reinstalled Macro and saw button come back.
- Used button to put silly text on a funny picture.
Reviewers: btrahan, chad
Reviewed By: chad
Subscribers: epriestley
Differential Revision: https://secure.phabricator.com/D10900
Summary: Fixes T6152, T6237. This introduces a viewPolicy column to the DifferentialDiff, and re-jiggers the DifferentialDiff policy implementation such that things behave as before once associated with a revision, else use the DifferentialDiff policy column value.
Test Plan: made a diff with a non-standard view policy and noted that policy was still selected in the revision step. arc lint.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T6237, T6152
Differential Revision: https://secure.phabricator.com/D10875
Summary: Fixes T6200. Ref T6237. When creating a diff from the web view, allow the user to select the repository at that time. When viewing a diff that has no associated revision and then creating a revision, pass along the repository phid to the create revision controller. Within the create revision controller, default the repository selector to this repository phid. Finally, in the editor, stop aggressively resetting the repository phid for every TYPE_UPDATE; rather, do so if its not a new object -- the diff should reign supreme in that case -- or if there's no repository -- let the diff be the guide.
Test Plan:
- made a diff with an associated repo, made a revision from the diff, saw the associated repo and it stuck on save!
- made a diff with an associated repo, made a revision from the diff but changed the repo and it stuck on save!
- made a diff with an associated repo, made a revision from the diff but changed the repo to nothing and it stuck on save!
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T6237, T6200
Differential Revision: https://secure.phabricator.com/D10872
Summary: Ref T6237. This sets us up for some future work like T6152, T6200 and generally cleaning up this workflow a bit. Tried to do as little as possible so not exposing transaction view yet. (Though that timeline is going to be a little funky in the common case of just the lone create transaction.)
Test Plan: made a diff from web ui and it worked. made a herald rule to block certain diffs then tried to make such a diff and saw UI letting me know i was blocked
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T6237
Differential Revision: https://secure.phabricator.com/D10869
Summary: The shield is just confusing. In one case it doesn't work, and in the other case it just shows you a copy of the file you can see just below except in red. Fixes T4599, T1211. Note T1211 proposed not showing the "move away" file **at all** but I think removing the shield fixes the source of confusion. The code here is a bit if / else if / else if... heavy but this is logically sound.
Test Plan: made a diff where i moved a file then edited it in the new location. viewed diff, saw confusing shield, dropped caches, applied patch, viewed diff and saw no shield. made a diff where I moved a file and didn't edit in new location and saw similar shield disappearness.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T1211, T4599
Differential Revision: https://secure.phabricator.com/D10865
Summary: This upgrades 1up view from "does not work" back to "barely works".
Test Plan: view diff, 1up and 2up.
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: epriestley
Differential Revision: https://secure.phabricator.com/D10854
Summary: Fixes T3942, turns the load links into buttons.
Test Plan: Set my limit to 1, test page.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T3942
Differential Revision: https://secure.phabricator.com/D10775
Summary: Fixes T6343. Grepped for all callsites and added addLinkSection where needed.
Test Plan: Tested Differential, Maniphest, Conpherence, Ponder and Macro. Inspect HTML mail for anchor tags. Inspect text mails for non-disruption.
Reviewers: epriestley, btrahan
Reviewed By: btrahan
Subscribers: talshiri, Korvin, epriestley
Maniphest Tasks: T6343
Differential Revision: https://secure.phabricator.com/D10762
Summary: Ref T6345, This adds more consistent color choices to match how Phabricator generally works across Differential/Diffusion per user statuses.
Test Plan: Review a few Audits in my sandbox.
Reviewers: epriestley, btrahan
Reviewed By: btrahan
Subscribers: Korvin, epriestley
Maniphest Tasks: T6345
Differential Revision: https://secure.phabricator.com/D10726
Summary:
Ref T5702. This is a forward-looking change which provides some very broad API improvements but does not implement them. In particular:
- Controllers no longer require `$request` to construct. This is mostly for T5702, directly, but simplifies things in general. Instead, we call `setRequest()` before using a controller. Only a small number of sites activate controllers, so this is less code overall, and more consistent with most constructors not having any parameters or effects.
- `$request` now offers `getURIData($key, ...)`. This is an alternate way of accessing `$data` which is currently only available on `willProcessRequest(array $data)`. Almost all controllers which implement this method do so in order to read one or two things out of the URI data. Instead, let them just read this data directly when processing the request.
- Introduce `handleRequest(AphrontRequest $request)` and deprecate (very softly) `processRequest()`. The majority of `processRequest()` calls begin `$request = $this->getRequest()`, which is avoided with the more practical signature.
- Provide `getViewer()` on `$request`, and a convenience `getViewer()` on `$controller`. This fixes `$viewer = $request->getUser();` into `$viewer = $request->getViewer();`, and converts the `$request + $viewer` two-liner into a single `$this->getViewer()`.
Test Plan:
- Browsed around in general.
- Hit special controllers (redirect, 404).
- Hit AuditList controller (uses new style).
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T5702
Differential Revision: https://secure.phabricator.com/D10698
Summary: pre-patch, we match on things like https / http and port... just match domains. Fixes T5693.
Test Plan: arc diff -> arc land and the diff was closed correctly
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T5693
Differential Revision: https://secure.phabricator.com/D10701
Summary:
I am not sure how valuable this is *as is* - I think it needs different explanations for what happened in mercurial or subversion? I do not know what those explanations are.
Made an error in D10485 - the $hashes that were saved is an array of objects, so it ends up turning into garbage via the wonders of serialization and de-serialization. Fix that by explicitly saving the tree hash.
I would like to make this work for the other VCS types we support, add the "undo / nope" button and call it fixed.
Ref T3686.
Test Plan: clicked "explan why" and saw why
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: epriestley, Korvin
Maniphest Tasks: T5693, T3686
Differential Revision: https://secure.phabricator.com/D10489
Summary: Default $phids to array() and update it if getValue() has something pertinent... Fixes T6292.
Test Plan: just used the ole logic noodle on this one.
Reviewers: chad, epriestley
Reviewed By: chad, epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T6292
Differential Revision: https://secure.phabricator.com/D10697
Summary:
Ref T1191. Now that the whole database is covered, we don't need to do as much work to build expected schemata. Doing them database-by-database was helpful in converting, but is just reudndant work now.
Instead of requiring every application to build its Lisk objects, just build all Lisk objects.
I removed `harbormaster.lisk_counter` because it is unused.
It would be nice to autogenerate edge schemata, too, but that's a little trickier.
Test Plan: Database setup issues are all green.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley, hach-que
Maniphest Tasks: T1191
Differential Revision: https://secure.phabricator.com/D10620
Summary:
Ref T1191. Ref T6203. While generating expected schemata, I ran into these columns which seem to have sketchy nullability.
- Mark most of them for later resolution (T6203). They work fine today and don't need to block T1191. Changing them can break the application, so we can't autofix them.
- Forgive a couple of them that are sort-of reasonable or going to get wiped out.
Test Plan: Saw 94 remaining warnings.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: hach-que, epriestley
Maniphest Tasks: T1191, T6203
Differential Revision: https://secure.phabricator.com/D10593
Summary: Fixes T6201. This stuff didn't fully get updated for ApplicationTransactions. Get it working again (notably, make inline comment text publish) and clean it up a little bit.
Test Plan:
- Published a Differential feed story into Asana with comment text.
- Pulbished a Diffusion feed story into Asana with comment text.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T6201
Differential Revision: https://secure.phabricator.com/D10584
Summary: Fixes T6184. On a Revision page we don't show the date as an important piece of information, so it's also not likely useful on a Hovercard (and confusing as to what the date means).
Test Plan: Hover over a linked Diff
Reviewers: btrahan, epriestley
Reviewed By: epriestley
Subscribers: epriestley, Korvin
Maniphest Tasks: T6184
Differential Revision: https://secure.phabricator.com/D10579
Summary: Fixes T6176. Language here is a bit awkard but I wanted to use the verb "removed" *and* still have the object first, so I ended up adding the before details parenthetically.
Test Plan: story no longer fatal'd in my feed
Reviewers: epriestley, chad
Reviewed By: chad
Subscribers: epriestley, Korvin
Maniphest Tasks: T6176
Differential Revision: https://secure.phabricator.com/D10549
Summary:
Ref T1191. This lays some groundwork for generating the expected schemata, so we can compare them to the actual schemata and produce a meaningful diff.
- In general, each application will subclass `PhabricatorConfigSchemaSpec` and provide a definition of the tables it expects.
- This class has helper methods to mostly-automatically build table definitions for Lisk and (in the future) edges.
- When building expected schema, we specify a "data type", like "epoch". This is the type of data the application stores in the column, from the application's point of view. The SchemaSpec converts this into the best avilable storage type: for example, "text" will translate to `utf8mb4` if it's availalbe, or `binary` if not. This gives us a layer of indirection to insulate us from craziness.
Test Plan: See screenshots.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T1191
Differential Revision: https://secure.phabricator.com/D10497
Summary: Fixes T5536. Some bonus pht in there.
Test Plan: made a diff hovered over the stars and saw my new text.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: epriestley, Korvin
Maniphest Tasks: T5536
Differential Revision: https://secure.phabricator.com/D10487
Summary:
Implements a new transaction - still TYPE_ACTION - but using a new DifferentialAction::ACTION_COMMIT_CLOSE. Augment rendering as necessary to display this new transaction. Saves enough information so T3686 is possible but stops short of implementing a popup to display this information. Fixes T5875. Ref T3686.
One small display oddity - this new transaction now renders at the top of the transaction group whereas when it was a comment it was on the bottom. I think this is basically okay but if not how fix? (Playing with the "strength" of these actions will mess up the email too?)
Test Plan: made a diff X that fixed task Y. committed. checked diff X, task Y, and the commit pages for proper transactions and all looked good.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: epriestley, Korvin
Maniphest Tasks: T3686, T5875
Differential Revision: https://secure.phabricator.com/D10485
Summary: Fixes T4036. Now if you say something on diff X like "This reminds me of Tx and Dy and commitHashFoo and Px." each of those objects gets a little visible transaction that the mention occurred. No feed, email, or notifications.
Test Plan: made a comment like above and verified transactions. also submitted a diff that "Fixes Tx" and Tx did not get the transaction as expected.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: joshuaspence, epriestley, Korvin
Maniphest Tasks: T4036
Differential Revision: https://secure.phabricator.com/D10451
Summary:
Ref T6013. A very long time ago, edges were less clearly low-level infrastructure, and some user-aware stuff got built around edge edits.
This was kind of a mess and I eventually removed it, during or prior to T5245. The big issue was that control flow was really hard to figure out as things went all the way down to the deepest level of infrastructure and then came back up the stack to events and transactions. The new stuff is more top-down and generally seems a lot easier and cleaner.
Consequently, actors are no longer required for edge edits. Remove the parameter.
Test Plan: Poked around; ran unit tests.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley, hach-que
Maniphest Tasks: T6013
Differential Revision: https://secure.phabricator.com/D10412
Summary: Ref T3307. Only one I thought was tricky was Excel; I went with bytes there like it was email.
Test Plan: played around on a few endpoints but mostly thought carefully
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: epriestley, Korvin
Maniphest Tasks: T3307
Differential Revision: https://secure.phabricator.com/D10392
Summary: Fixes T5915. Occasionally, users derp up and diff private key material. Adding a pre-write Herald phase enables configuration of a partial layer of protection that will reject these changes before they hit disk, provided they can be detected by, e.g., filename.
Test Plan:
- Added a rule with checks on every field, verified they looked fine in the transcript.
- Created some revisions to test those changes (I have a bunch of revision rules locally).
- Verified rejects don't write transcripts to the database.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T5915
Differential Revision: https://secure.phabricator.com/D10305
Summary: Ref T5894. We have a couple more similar cases. Make them all do a decision-based redirect for now.
Test Plan: Did "View Raw File" and such, and also made sure thumbnails still work.
Reviewers: btrahan, chad
Reviewed By: chad
Subscribers: epriestley
Maniphest Tasks: T5894
Differential Revision: https://secure.phabricator.com/D10301
Summary:
When enabled, this will show the full history of review comments in an
email-compatible threading-view.
Test Plan: Sending emails with the option on and off.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: epriestley, Korvin
Differential Revision: https://secure.phabricator.com/D10146
Summary:
Added support for side-by-side HTML and plaintext email building.
We can control if the HTML stuff is sent by by a new config, metamta.html-emails
Test Plan:
Been running this in our deployment for a few months now.
====Well behaved clients====
- Gmail
- Mail.app
====Bad clients====
- [[ http://airmailapp.com/ | Airmail ]]. They confuse Gmail too, though.
====Need testing====
- Outlook (Windows + Mac)
Reviewers: chad, #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: webframp, taoqiping, chad, epriestley, Korvin
Maniphest Tasks: T992
Differential Revision: https://secure.phabricator.com/D9375
Summary: Fixes T2101. When viewing an image change, show image dimensions, MIME type, and filesize.
Test Plan:
{F190189}
{F190190}
very utility
such wow
Reviewers: mailson, btrahan, chad
Reviewed By: chad
Subscribers: epriestley, Korvin, aran
Maniphest Tasks: T2101
Differential Revision: https://secure.phabricator.com/D5206
Summary:
Ref T5861. Currently, mail tags are hard-coded; move them into applications. Each Editor defines its own tags.
This has zero impact on the UI or behavior.
Test Plan:
- Checked/unchecked some options, saved form.
- Swapped back to `master` and saw exactly the same values.
Reviewers: chad, btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T5861
Differential Revision: https://secure.phabricator.com/D10238
Summary:
- Fixes T5851. Currently, if a commit has `Fixes T123`, we generate an email with just that before generating the commit email. Don't send/publish transactions about a commit before it imports (this is a tiny bit hacky, but well-contained and I don't think it causes any problems).
- Fixes T4864. Currently, we try to parse Differential information even if Differential is not installed. Instead, do this only if Differential is installed.
- Fixes T5771. Currently, if we can't figure out who the committer/author of a commit is, we don't publish a `Fixes T123` transaction. Instead, fall back to acting as "Diffusion" if we can't find a better actor. Most of this diff expands the role of application actors. The existing application actors (Herald and Harbormaster) seem to be working well.
Test Plan:
- Pushed a commit with `Fixes T123` and verified it did not generate email directly. (The task half of the transaction still does, correctly.)
- Uninstalled Differential and pushed a commit, got a clean import instead of an exception.
- Commented out author/committer PHIDs and pushed stuff, saw a "Diffusion" actor.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T5771, T4864, T5851
Differential Revision: https://secure.phabricator.com/D10221
Summary: This fix is wrong - should be load and not get - but moreover this is actually correctly set as the reply handler is instantiated inside the DifferentialRevisionMailReceiver correctly; $this->getExclude was correct. Ref T5185.
Test Plan: this shall stop the fatal in production.
Reviewers: chad, epriestley
Reviewed By: epriestley
Subscribers: epriestley, Korvin
Maniphest Tasks: T5185
Differential Revision: https://secure.phabricator.com/D10101
Summary: Ref T5185. By code inspection, I am pretty sure before this patch it was doing a set of a get on itself which does nothing. Now, being careful not to break Facebook we get the proper exclusion phids. I am pretty sure the folks in T5185 are experiencing this in Differential only.
Test Plan: Get some folks on T5185 to play with this
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: epriestley, Korvin
Maniphest Tasks: T5185
Differential Revision: https://secure.phabricator.com/D10087
Summary:
This Fixes T5737. Apparently the functionality to search by different
statuses in differential was already there, but the options weren't
exposed in the frontend. I can't think of any reason why this should've
been the case, so I just added the other options.
Test Plan: Tested against some local diffs to match new query option.
Reviewers: joshuaspence, epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: epriestley, Korvin
Maniphest Tasks: T5737
Differential Revision: https://secure.phabricator.com/D10076
Summary: Instead of implementing the `getCapabilityKey` method in all subclasses of `PhabricatorPolicyCapability`, provide a `final` implementation in the base class which uses reflection. See D9837 and D9985 for similar implementations.
Test Plan: N/A
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: epriestley, Korvin, hach-que
Differential Revision: https://secure.phabricator.com/D10039
Summary: Ref T5655. Rename `PhabricatorPHIDType` subclasses for clarity (see discussion in D9839). I'm not too keen on some of the resulting class names, so feel free to suggest alternatives.
Test Plan: Ran unit tests.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: epriestley, Korvin, hach-que
Maniphest Tasks: T5655
Differential Revision: https://secure.phabricator.com/D9986
Summary: Provide an implementation for the `getName` method rather than automagically determining the application name.
Test Plan: Saw reasonable application names in the launcher.
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: epriestley, Korvin
Differential Revision: https://secure.phabricator.com/D10027
Summary: Ref T5655. Some discussion in D9839. Generally speaking, `Phabricator{$name}Application` is clearer than `PhabricatorApplication{$name}`.
Test Plan:
# Pinned and uninstalled some applications.
# Applied patch and performed migrations.
# Verified that the pinned applications were still pinned and that the uninstalled applications were still uninstalled.
# Performed a sanity check on the database contents.
Reviewers: btrahan, epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: hach-que, epriestley, Korvin
Maniphest Tasks: T5655
Differential Revision: https://secure.phabricator.com/D9982
Summary: Instead of implementing the `getTypeConstant` method in all subclasses of `PhabricatorPHIDType`, provide a `final` implementation in the base class which uses reflection. See D9837 for a similar implementation.
Test Plan: Ran `arc unit`.
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: epriestley, Korvin, hach-que
Differential Revision: https://secure.phabricator.com/D9985
Summary: Ref T5655. The `PhabricatorDestructibleInterface` interface is misspelled as `PhabricatorDestructableInterface`. Fix the spelling mistake.
Test Plan: `grep`. Seeing as this interface is fairly recent, I don't expect that this would cause any widespread breakages.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: epriestley, Korvin
Maniphest Tasks: T5655
Differential Revision: https://secure.phabricator.com/D9988
Summary: Ref T4420. These are used for some stuff like "reviewer".
Test Plan:
- Edited "reviewers" in differential edit.
- Edited "reviewers" in differential search.
- Edited "reviewers" in Differential "add reviewers..." action on detail page.
- Edited a "reviewers" field in a herald rule.
- Edited "owner" in owners search.
- Edited "primary owner", "owners" on owners edit.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T4420
Differential Revision: https://secure.phabricator.com/D9887
Summary: Ref T5245. Updates the project/object edge to use a modern class definition. Moves further toward real edges.
Test Plan: Added projects to some objects, viewed transactions in transaction record.
Reviewers: chad, btrahan, joshuaspence
Reviewed By: joshuaspence
Subscribers: epriestley
Maniphest Tasks: T5245
Differential Revision: https://secure.phabricator.com/D9849
Summary:
Ref T5245. These were a bad idea.
We no longer need actors for edge edits either, so remove those. Generally, edges have fit into the policy model as pure/low-level infrastructure, and they do not have any policy or capability information in and of themselves.
Test Plan: `grep`
Reviewers: chad, btrahan, joshuaspence
Reviewed By: joshuaspence
Subscribers: epriestley
Maniphest Tasks: T5245
Differential Revision: https://secure.phabricator.com/D9840
Summary:
Ref T5245. See some discussion in D9838.
When we attach object A to object B, we'd like to write transactions on both sides but only write the actual edges once.
To do this, allow edge types to `shouldWriteInverseTransactions()`. When an edge type opts into this, have editors apply the inverse transactions before writing the edge. These inverse transactions don't actually apply effects, they just show up in the transaction log.
Test Plan: Attached and detached revisions from tasks, saw transactions appear on both sides of the operation.
Reviewers: chad, btrahan, joshuaspence
Reviewed By: btrahan, joshuaspence
Subscribers: epriestley
Maniphest Tasks: T5245
Differential Revision: https://secure.phabricator.com/D9839
Test Plan: Queried a revision that had a repository attached, got the PHID; queried one that didn't, got null.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: epriestley, Korvin
Differential Revision: https://secure.phabricator.com/D9928
Summary: Ref T4420. Update "projects" source.
Test Plan:
- Edited projects on a Differential revision.
- Edited projects on a commit.
- Edited projects on a repository.
- Edited projects in feed search.
- Edited projects in a Herald rule field.
- Edited projects in a Herald rule action.
- Edited projects in Maniphest batch editor.
- Edited projects on Maniphest task.
- Edited projects in "Associate Projects..." action in Maniphest.
- Edited projects on Maniphest search in "all projects", "any project" and "not projects" fields.
- Edited projects on a Paste.
- Edited projects on a Pholio mock.
- Edited projects on a custom policy rule.
- Edited projects on a Ponder question.
- Edited projects on a Diffusion search query.
- Edited projects on a global search query.
- Edited projects on a slowvote.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T4420
Differential Revision: https://secure.phabricator.com/D9884
Summary:
Ref T4420.
- Allow tokenizers to accept either a `Datasource` object (new style) or a URI (old style).
- Read URI and placeholder text from object, if available.
- Swap the "repositories" datasource (which seemed like the simplest one) over to the new stuff.
- Tweak/update the repo tokens a little bit.
Test Plan:
- Used tokenizer in Herald, Differential (search), Differential (edit), Push Logs.
- Grepped for other callsites.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T4420
Differential Revision: https://secure.phabricator.com/D9874
Summary:
Fixes T3732. Ref T1205. Ref T3116.
External accounts (like emails used as identities, Facebook accounts, LDAP accounts, etc.) are stored in "ExternalAccount" objects.
Currently, we have a very restrictive `CAN_VIEW` policy for ExternalAccounts, to add an extra layer of protection to make sure users can't use them in unintended ways. For example, it would be bad if a user could link their Phabricator account to a Facebook account without proper authentication. All of the controllers which do sensitive things have checks anyway, but a restrictive CAN_VIEW provided an extra layer of protection. Se T3116 for some discussion.
However, this means that when grey/external users take actions (via email, or via applications like Legalpad) other users can't load the account handles and can't see anything about the actor (they just see "Restricted External Account" or similar).
Balancing these concerns is mostly about not making a huge mess while doing it. This seems like a reasonable approach:
- Add `CAN_EDIT` on these objects.
- Make that very restricted, but open up `CAN_VIEW`.
- Require `CAN_EDIT` any time we're going to do something authentication/identity related.
This is slightly easier to get wrong (forget CAN_EDIT) than other approaches, but pretty simple, and we always have extra checks in place anyway -- this is just a safety net.
I'm not quite sure how we should identify external accounts, so for now we're just rendering "Email User" or similar -- clearly not a bug, but not identifying. We can figure out what to render in the long term elsewhere.
Test Plan:
- Viewed external accounts.
- Linked an external account.
- Refreshed an external account.
- Edited profile picture.
- Viewed sessions panel.
- Published a bunch of stuff to Asana/JIRA.
- Legalpad signature page now shows external accounts.
{F171595}
Reviewers: chad, btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T3732, T1205, T3116
Differential Revision: https://secure.phabricator.com/D9767
Summary: I'm pretty sure that `@group` annotations are useless now... see D9855. Also fixed various other minor issues.
Test Plan: Eye-ball it.
Reviewers: #blessed_reviewers, epriestley, chad
Reviewed By: #blessed_reviewers, epriestley
Subscribers: epriestley, Korvin, hach-que
Differential Revision: https://secure.phabricator.com/D9859
Summary: This supplements the footer warning and makes it more visible for authors.
Test Plan: {F173277}
Reviewers: btrahan, chad
Reviewed By: chad
Subscribers: epriestley
Differential Revision: https://secure.phabricator.com/D9794
Summary:
Ref T5495. We currently show one warning in revision headers, about not having any reviewers.
I want to add a second warning (for missing Legalpad signatures). At least one install would like to add custom warnings (see T5495) which are so specific that we can't reasonably cover them in the upstream.
Generalize these header warnings by moving them to CustomField, so I can implement the Legalpad stuff without making a mess and the install in T5495 can use an extension.
Test Plan:
Hit all three header states, they look exactly like they did before this change:
{F173265}
Reviewers: btrahan, chad
Reviewed By: chad
Subscribers: epriestley
Maniphest Tasks: T5495
Differential Revision: https://secure.phabricator.com/D9793
Summary: Fixes T5503. We incorrectly render an encoding note for empty files. Only render an encoding note for text changes with at least one hunk.
Test Plan:
- Viewed empty file, no note.
- Viewed nonempty file with altered encoding, saw note.
Reviewers: btrahan, joshuaspence
Reviewed By: joshuaspence
Subscribers: epriestley
Maniphest Tasks: T5503
Differential Revision: https://secure.phabricator.com/D9780
Summary:
Ref T3116. Add a Herald action "Require legal signatures" which requires revision authors to accept legal agreements before their revisions can be accepted.
- Herald will check which documents the author has signed, and trigger a "you have to sign X, Y, Z" for other documents.
- If the author has already signed everything, we don't spam the revision -- basically, this only triggers when signatures are missing.
- The UI will show which documents must be signed and warn that the revision can't be accepted until they're completed.
- Users aren't allowed to "Accept" the revision until documents are cleared.
Fixes T1157. The original install making the request (Hive) no longer uses Phabricator, and this satisfies our requirements.
Test Plan:
- Added a Herald rule.
- Created a revision, saw the rule trigger.
- Viewed as author and non-author, saw field UI (generic for non-author, specific for author), transaction UI, and accept-warning UI.
- Tried to accept revision.
- Signed document, saw UI update. Note that signatures don't currently //push// an update to the revision, but could eventually (like blocking tasks work).
- Accepted revision.
- Created another revision, saw rules not add the document (since it's already signed, this is the "no spam" case).
Reviewers: btrahan, chad
Reviewed By: chad
Subscribers: asherkin, epriestley
Maniphest Tasks: T1157, T3116
Differential Revision: https://secure.phabricator.com/D9771
Summary: Ref T5137. A slight modification to D9609, such that the repository is always included in Differential emails. Otherwise "Accepted", "Closed" and "Requested Changes To" emails don't include the repository.
Test Plan: Not tested.
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: epriestley, Korvin
Maniphest Tasks: T5137
Differential Revision: https://secure.phabricator.com/D9728
Summary:
Ref T5446.
- For all callsites which do not specify a value, set `false` explicitly.
- Make `true` the default.
Test Plan: Used `grep`, then manually went through everything.
Reviewers: chad
Reviewed By: chad
Subscribers: epriestley
Maniphest Tasks: T5446
Differential Revision: https://secure.phabricator.com/D9687
Summary:
add looksoon call after every attempt at landing.
This includes failed attempts, to elevate "not a fast-forward" issues, although there are probably smarter things to be done about that.
Test Plan: Land, look at logs.
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: epriestley, Korvin
Differential Revision: https://secure.phabricator.com/D9518
Summary: Ref T5179. Ref T4045. Ref T832. We can now write non-utf8 hunks into the database, so try to do more reasonable things with them in the UI.
Test Plan: (See screenshots...)
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T832, T4045, T5179
Differential Revision: https://secure.phabricator.com/D9294
Summary: Ref T1049. This moves the declaration of build variables onto HarbormasterBuildableInterface, allowing new classes implementing HarbormasterBuildableInterface to declare their own variables.
Test Plan: Implemented it on another class, saw the build variables appear.
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: epriestley, Korvin
Maniphest Tasks: T1049
Differential Revision: https://secure.phabricator.com/D9618
Summary: Ref T5137. Listing the repository in Differential emails makes it easy to filter.
Test Plan: Eye-ball it.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: young_hwi, epriestley, Korvin
Maniphest Tasks: T5137
Differential Revision: https://secure.phabricator.com/D9609
Summary: Ref T4986. Instead of requiring users to know the name of an application search engine class, let them select from a list.
Test Plan:
Created a new panel.
{F165468}
Reviewers: chad
Reviewed By: chad
Subscribers: epriestley
Maniphest Tasks: T4986
Differential Revision: https://secure.phabricator.com/D9500
Summary: Fixes T5309. Modernize this callsite to use ChangesetQuery and pick up attached objects.
Test Plan: Clicked "Download Raw Diff" in Differential.
Reviewers: btrahan, chad
Reviewed By: chad
Subscribers: epriestley
Maniphest Tasks: T5309
Differential Revision: https://secure.phabricator.com/D9461
Summary: This trailing whitespace is meaningful for these files. Also, exclude test data from linting.
Test Plan: Ran unit tests.
Reviewers: hach-que, joshuaspence
Reviewed By: joshuaspence
Subscribers: epriestley
Differential Revision: https://secure.phabricator.com/D9462
Summary: Fixes T5303. Individual diffs can have public access policies.
Test Plan: Viewed a public diff while logged out.
Reviewers: btrahan, chad
Reviewed By: chad
Subscribers: epriestley
Maniphest Tasks: T5303
Differential Revision: https://secure.phabricator.com/D9452
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: This went smoother than expeced. Makes the rounded Card the default, also tweaked selected state a little.
Test Plan:
Test UIExamples, Maniphest, Home, Differential, Harbormaster, Audit. Everything seems normal
{F163971}
{F163973}
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: epriestley, Korvin
Differential Revision: https://secure.phabricator.com/D9408
Summary:
Can't say I know what I'm doing here, but this fixes an the upgrade-scope flow for landing-to-github.
Without this change, it looks like the submit button makes the browser (Chrome and msie) make the call in the background, instead of hijacking the window.
With it, it works like it should.
Test Plan: try to land with weak token, click "Refresh..", see GitHub button.
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: epriestley, Korvin
Differential Revision: https://secure.phabricator.com/D9407
Summary: Fixes T5262. This branch is overzealous, and causes us to fail to load changeses if `metamta.differential.unified-comment-context` is off. It was on for me locally for testing, which is why I missed this.
Test Plan: No more exception.
Reviewers: chad
Reviewed By: chad
Subscribers: richardvanvelzen, epriestley
Maniphest Tasks: T5262
Differential Revision: https://secure.phabricator.com/D9376
Summary: Fix size and spacing of file icons in diffs, update with new types, consistency.
Test Plan: Tested a diff in differential
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: epriestley, Korvin
Differential Revision: https://secure.phabricator.com/D9372
Summary: Ref T4045. Ref T5179. Send all new writes into the modern store.
Test Plan:
- Created a diff.
- Verified it went to the modern store.
- Destroyed a revision, verified hunks were destroyed.
- Also unit tests.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T4045, T5179
Differential Revision: https://secure.phabricator.com/D9293
Summary: Ref T4045. Ref T5179. When saving a modern hunk, deflate it if we have the function and deflating it will save a nontrivial number of bytes.
Test Plan:
- Used `bin/hunks migrate` to move some hunks over, saw ~70-80% compression on most standard hunks.
- Viewed changesets using compressed hunks.
- Profiled `gzinflate()` and verified the cost is trivial (<< 1ms) at least for normal diffs.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T4045, T5179
Differential Revision: https://secure.phabricator.com/D9292
Summary:
Ref T4045. Ref T5179. While we'll eventually need to force a migration, we can let installs (particularly large installs) do an online migration for now. This moves hunks to the new storage format one at a time.
(Note that nothing writes to the new store yet, so this is the only way to populate it.)
WARNING: Installs, don't run this yet! It won't compress the data. Wait until it can also do compression.
Test Plan: Added a `break;` after migrating one row and moved a few rows over. Spot checked them in the database and viewed the affected diffs.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T4045, T5179
Differential Revision: https://secure.phabricator.com/D9291
Summary:
Ref T4045. Ref T5179. Hunk storage has two major issues:
- It's utf8, but actual diffs are binary.
- It's huge and can't be compressed or archived.
This introduces a second datastore which solves these problems: by recording hunk encoding, supporting compression, and supporting alternate storage. There's no actual compression or storage support yet, but there's space in the table for them.
Since nothing actually uses hunk IDs, it's fine to have these tables exist at the same time and use the same IDs. We can migrate data between the tables gradually without requiring downtime or disrupting installs.
Test Plan:
- There are no writes to the new table yet.
- The only effect this has is making us issue one extra query when looking for hunks.
- Observed the query issue, but everything else continue working fine.
- Created a new diff.
- Ran unit tests.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T4045, T5179
Differential Revision: https://secure.phabricator.com/D9290
