Summary:
Resolves T5817. Continuation of D10231.
This corrects the rendering of the "user answered question" transaction so that it does not incorrectly attempt to render the question handle as HTML in emails if the rendering target is not HTML.
Test Plan: Used `bin/mail show-outbound` to verify that the email didn't contain escaped HTML when answering a question.
Reviewers: #blessed_reviewers, btrahan, epriestley
Reviewed By: #blessed_reviewers, btrahan, epriestley
Subscribers: epriestley, Korvin
Maniphest Tasks: T5817
Differential Revision: https://secure.phabricator.com/D10319
Summary: I derped on this; the SFTP interface doesn't have setWorkingDirectory because it implements DrydockFilesystemInterface and not DrydockCommandInterface. So when you use the Upload File build step, the daemon will crash due to an undefined method.
Test Plan: Tested on my live server.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: epriestley, Korvin
Differential Revision: https://secure.phabricator.com/D10351
Summary: This fixes the ZIP controller redirect in Phragment after the external redirect change.
Test Plan: Tested it on my server.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: epriestley, Korvin, hach-que
Differential Revision: https://secure.phabricator.com/D10350
Summary: Fixes T5958
Test Plan: i just used the ole logic noodle on this one
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: epriestley, Korvin
Maniphest Tasks: T5958
Differential Revision: https://secure.phabricator.com/D10359
Summary:
Fixes T4057. This sort of sidesteps the trickiest (but very rare) case of things like embedded slowvotes. We might be able to refine that later.
In the common bad case (macros, large images) it gets reasonable results by using `overflow: hidden` with `max-height`.
We use `PhabriatorMarkupEngine::summarize()` to try to just render the first paragraph.
Test Plan: {F195093}
Reviewers: chad, btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T4057
Differential Revision: https://secure.phabricator.com/D10355
Summary: Fixes T2564. See screenshot.
Test Plan:
{F194796}
- Made a bunch of valid and invalid adjustments here and verified that the branches table showed autoclose state and branches consistent with the settings.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T2564
Differential Revision: https://secure.phabricator.com/D10349
Summary: Fixes T4769. This is silly and just scratches an itch, but do a better job with navigation sequences.
Test Plan: {F195082}
Reviewers: btrahan, chad
Reviewed By: chad
Subscribers: epriestley
Maniphest Tasks: T4769
Differential Revision: https://secure.phabricator.com/D10353
Summary: Ref T5936. This implements build implementations aborting early when the build has since been restarted. Build steps now periodically poll to see if the build's current generation does not match their generation, and they throw a `HarbormasterBuildAbortedException` if that is the case.
Test Plan: Tested locally on my machine with the sleep build step.
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: epriestley, Korvin
Maniphest Tasks: T5936
Differential Revision: https://secure.phabricator.com/D10322
Summary:
Fixes T4767. I believe 80% of this was actually caused by the author issue fixed in T5771, but this should help make the other 20% debuggable.
- Record why we didn't autoclose a commit when we process it.
- Show branch autoclose status in the main branch table.
- Show commit autoclose status on the edit screen.
- Add documentation about how to find these statuses and what they mean.
Test Plan:
- Read documentation.
- Viewed branches and hovered over the various states.
- Viewed commits in various states and checked the "Autoclose?" field.
- Pushed some commits and saw autoclose activate.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T4767
Differential Revision: https://secure.phabricator.com/D10348
Summary: Fixes T2605. Provide some instructions on configuring RDS properly. The "DB Parameter Group" thing in the web UI seems pretty easy to use, it's just not obvious that it's what you should be using.
Test Plan: Jiggled these warnings to trigger them, viewed the output, saw a table of values and a hint about RDS.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T2605
Differential Revision: https://secure.phabricator.com/D10343
Summary: Ref T992. This makes HTML mail layout more consistent with text mail layout and fixes my greatest annoyance with it.
Test Plan: Used `bin/mail list-outbound --id <id> --dump-html` to view mail in Safari, saw it have a normal amount of whitespace between sections.
Reviewers: btrahan, talshiri, chad
Reviewed By: chad
Subscribers: epriestley
Maniphest Tasks: T992
Differential Revision: https://secure.phabricator.com/D10344
Summary: Ref T5847.
Test Plan: Used `bin/remove destroy` to destroy a question. Saw the question and its answers get wiped out.
Reviewers: btrahan, shadowhand
Reviewed By: shadowhand
Subscribers: shadowhand, epriestley
Maniphest Tasks: T5847
Differential Revision: https://secure.phabricator.com/D10345
Summary:
Ref T2605. For old MySQL, this option is not supported. Catch that and tailor the error.
I couldn't find the first version of MySQL which introduced this optino in order to produce a more useful error. I spent about ~10 minutes looking.
Test Plan: Faked the error, survived setup.
Reviewers: btrahan, chad
Reviewed By: chad
Subscribers: epriestley
Maniphest Tasks: T2605
Differential Revision: https://secure.phabricator.com/D10342
Summary: Add a note about running this manually for troubleshooting.
Test Plan: Read it.
Reviewers: btrahan, chad
Reviewed By: chad
Subscribers: epriestley
Differential Revision: https://secure.phabricator.com/D10341
Summary: be more aggressive about assuming plain-text, use remarkup for no extension, .remarkup, and .md, and last but not least use rainbow for .rainbow. Fixes T5818.
Test Plan: my README rendered just fine post these changes
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: asherkin, epriestley, Korvin
Maniphest Tasks: T5818
Differential Revision: https://secure.phabricator.com/D10340
Summary: Fixes T4881.
Test Plan: made a config change, saw the issue, restarted daemons and it went away
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: epriestley, Korvin
Maniphest Tasks: T4881
Differential Revision: https://secure.phabricator.com/D10339
Summary: Fixes T5943. We do this differently in different places; Audit / Differential do something like this while Pholio expands the "byLine" to include a timestamp. Go with the Audit / Differential approach, as presumably having the date as a top line, easily scannable metadata is the goal here.
Test Plan: viewed a list of pastes and saw a timestamp of creation at the top.
Reviewers: epriestley, chad
Reviewed By: chad
Subscribers: epriestley, Korvin
Maniphest Tasks: T5943
Differential Revision: https://secure.phabricator.com/D10338
Summary: Ref T1049. Set the working directory when executing commands on Drydock hosts. Without this set, they execute in the user's default home directory.
Test Plan: Ran a build and saw the correct working directory when running `pwd`.
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: CanadianBadass, epriestley, Korvin
Maniphest Tasks: T1049
Differential Revision: https://secure.phabricator.com/D10293
Summary:
Fixes T5916. Key insight here is that the screenshot shows a custom "Detail Solution / Notes" field, which is why this mojo doesn't work: custom remarkup fields don't emit their content for mention/file extraction.
Also fix a bug where multiple blocks with file PHIDs could be merged improperly, discarding some file PHIDs.
Test Plan: Added a custom remarkup field, added files to it, saw them attach to the task when changes were saved.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T5916
Differential Revision: https://secure.phabricator.com/D10335
Summary: $email => $e_email. Fixes T5933.
Test Plan: Added an email that was already on another account and got the proper "Duplicate" UI with the duplicate email address still entered
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: epriestley, Korvin
Maniphest Tasks: T5933
Differential Revision: https://secure.phabricator.com/D10334
Summary:
Fixes T5900. We have some very old code here which does not let you update your password if the `account.editable` flag is set.
This was approximately introduced in D890, and I think it was mostly copy/pasted at that point. I'm not sure this ever really made sense. The option is not documented as affecting this, for example. In the modern environment of auth providers, it definitely does not make sense.
Instead, always allow users to change passwords if the install has a password provider configured.
Test Plan:
- Set `account.editable` to false.
- Used a password reset link.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T5900
Differential Revision: https://secure.phabricator.com/D10331
Summary: Fixes T5942. These are external but currently unmarked.
Test Plan: Visited link, got redirected.
Reviewers: btrahan, chad
Reviewed By: chad
Subscribers: epriestley
Maniphest Tasks: T5942
Differential Revision: https://secure.phabricator.com/D10332
Summary:
Primarily, this fixes searching for `F123` in global search.
The info URI is now a better URI than the "best" URI for files, and doesn't have redirect issues.
Test Plan: Searched for `F123` in global search.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Differential Revision: https://secure.phabricator.com/D10330
Summary:
Fixes T5912. When we write files, we attempt to share storage if two files have the same content.
In some cases, we may not share storage. Examples include:
- Files migrated with `bin/files migrate` (it's simpler not to try to dedupe them).
- Old files, from before storage was sharable (the mechanism did not exist).
- Files broken by the bug fixed in T5912.
Add a script to compact files by pointing files with the same content hash at the same file contnet.
In the particular case of files broken by the bug in T5912, we know the hash of the file's content and will only point them at a file that we can load the data for, so this fixes them.
Compaction is not hugely useful in general, but this script isn't too complex and the ability to fix damage from the bug in T5912 is desirable. We could remove this capability eventually.
Test Plan:
- Ran `files compact --all --dry-run` and sanity checked a bunch of the duplicates for actually being duplicates.
- Migrated individual files with `files compact Fnnn --trace` and verified the storage compacted and all files survived the process.
- Verified unused storage was correctly destroyed after removing the last reference to it.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T5912
Differential Revision: https://secure.phabricator.com/D10327
Summary:
Fixes T5926. Fixes T5830. Ref T4767. Users currently sometimes have a hard time understanding repository update frequencies. This is compounded by aggressive backoff and incorrect backoff while importing repositories.
- Don't back off while importing repositories. This prevents us from hanging at 99.99% for inactive repositories while waiting for the next update.
- Back off less aggressively in general, and even more gradually during the first 3 days. This should make behavior around weekends better.
- Show update frequency in the UI.
- Provide an explicit "update now" button to call `diffusion.looksoon` in a more user-friendly way.
- Document how backoff policies work and how to adjust behavior.
Test Plan:
- Ran `bin/phd debug pulllocal` and verified backoff worked correctly from debugging output.
- Clicked "Update Now" to get a hint, reloaded page to see it update.
- Read documentation.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T4767, T5830, T5926
Differential Revision: https://secure.phabricator.com/D10323
Summary:
Fixes T5934. If you hash a password with, e.g., bcrypt, and then lose the bcrypt hasher for some reason, we currently fatal when trying to figure out if we can upgrade.
Instead, detect that the current hasher implementation has vanished and let the user reset their password (for account passwords) or choose a new one (for VCS passwords)>
Test Plan:
Account password:
- Artifically disabled bcrypt hasher.
- Viewed password panel, saw warnings about missing hasher.
- Used password reset workflow to change password, saw iterated MD5 hashed password get set.
- Enabled bcrypt hasher again.
- Saw upgrade warning.
- Upgraded password to bcrypt.
VCS password:
- Artificially disabled bcrypt hasher.
- Viewed password panel, saw warnings about missing hasher.
- Reset password.
- Saw iterated md5 password.
- Reenabled bcrypt.
- Upgraded to bcrypt.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T5934
Differential Revision: https://secure.phabricator.com/D10325
Summary: Ref T4284. This fixes at least one problem which can cause the observed behavior.
Test Plan:
- Before applying patch, used `PHABRICATOR_CREDENTIAL=PHID-CDTL-... bin/ssh-connect` + debugging prints to verify the keyfile was written and cleaned up normally.
- Destroyed the credental, verified the temporary file was not cleand up correctly.
- Applied patch, verified temporary file was not written and command exited with sensible error.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T4284
Differential Revision: https://secure.phabricator.com/D10328
Summary: Clean up some arg handling stuff.
Test Plan: Used this while debugging.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Differential Revision: https://secure.phabricator.com/D10314
Summary: This was broken by rP5ac36e8 by a derpy typo.
Test Plan: Ran dry run against a revision with a a repository, saw the field fill in on the transcript.
Reviewers: nickz, btrahan, chad
Reviewed By: chad
Subscribers: epriestley
Differential Revision: https://secure.phabricator.com/D10326
Summary:
Additional audit states were made queryable for T5871.
Include them in Conduit's audit.query as well. In doing so corrects
references from "status-foo" to "audit-status-foo".
Depends on D10271
Test Plan: with an api, issues queries and got sensible results
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: Korvin, epriestley
Differential Revision: https://secure.phabricator.com/D10290
Summary:
Ref T5932. Ref T5936. This implements build generations in Harbormaster, which provides the infrastructure required to both show users the previous states of restarted builds and to allow users to forcefully abort builds (and their targets).
You can view previous generations of a build by adding `?g=<n>` to the URI, but this isn't exposed in the UI anywhere yet.
Test Plan: Ran a build plan with a Sleep step in it. Reconfigured it for various sleep times and viewed previous generations of the build after restarting it.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: epriestley, Korvin
Maniphest Tasks: T5932, T5936
Differential Revision: https://secure.phabricator.com/D10321
Summary:
Resolves T5937. HTTPS redirects caused by `security.require-https` use a full scheme, domain and port in the URI. Consequently, this causes invocation of the new external redirect logic and prevents redirection from occurring properly when accessing the HTTP version of Phabricator that has `security.require-https` turned on.
I've also fixed the automatic slash redirection logic to add the external flag where appropriate.
Test Plan: Configured SSL on my local machine and turned on `security.require-https`. Observed the "Refusing to redirect" exception on master, while the redirect completed successfully with this patch.
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: epriestley, Korvin
Maniphest Tasks: T5937
Differential Revision: https://secure.phabricator.com/D10318
Summary: Caught this with the new redirect validation logic. The `$return_uri` was being set as just `B123` which is not valid. Prefixing it with `/` (like is done in `HarbormasterBuildActionController` already) gives the correct result of reloading the buildable's page.
Test Plan: Restarted all builds on a buildable, saw the page reload correctly.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: epriestley, Korvin
Differential Revision: https://secure.phabricator.com/D10320
Summary:
Fixes T5912. When migrating files, we try to clean up the old data. However, this code isn't aware of reference counting, and unconditionally destroys the old data.
For example, if you migrate files `F1` and `F2` and they have the same data, we'll delete the shared data when we migrate `F1`. Then you'll get an error when you migrate `F2`.
Since this only affects duplicate files, it primarily hits default profile pictures, which are the most numerous duplicate files on most installs.
Test Plan:
- Verified that the theory was correct by uploading two copies of a file and migrating the first one, before applying the patch. The second one's data was nuked and it couldn't be migrated.
- Applied patch.
- Uploaded two copies of a new file, migrated the first one (no data deletion), migrated the second one (data correctly deleted).
- Uploaded two copies of another new file, `bin/remove destory'd` the first one (no data deletion), then did it to the second one (data correctly deleted).
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T5912
Differential Revision: https://secure.phabricator.com/D10312
Summary: Ref T5915. Make `bin/remove destroy` a bit more thorough, since Herald transcripts can have field information in them.
Test Plan: Used `bin/remove destroy` to nuke revisions, saw their transcripts vanish too.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T5915
Differential Revision: https://secure.phabricator.com/D10306
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:
If daemon data is mangled, `bin/phd restart` will SIGINT process `0`, which kills it.
uh oh T.T so sad
Test Plan: Used `bin/phd start` to start daemons; removed PID information from one; saw `bin/phd stop` shut down cleanly and not kill itself.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: mholden, epriestley
Differential Revision: https://secure.phabricator.com/D10308
Summary: and for bonus, finesse some URIs a tad. Fixes T5922.
Test Plan: viewed F1 logged out and it worked! viewed the ugly URI for F1 and got redirected to the pretty URI.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: epriestley, Korvin
Maniphest Tasks: T5922
Differential Revision: https://secure.phabricator.com/D10309
Summary: its not necessary. Fixes T5906
Test Plan: clicked "Login to Comment" and went straight to the login form, sans intermediary dialogue
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: epriestley, Korvin
Maniphest Tasks: T5906
Differential Revision: https://secure.phabricator.com/D10295
Summary: Fixes T5918.
Test Plan: Verified memes work again.
Reviewers: hach-que, btrahan, chad
Reviewed By: chad
Subscribers: epriestley
Maniphest Tasks: T5918
Differential Revision: https://secure.phabricator.com/D10307
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: Straightforward (this is the one object type we do let you delete from the web UI) implemetation of `PhabricatorDestructibleInterface`.
Test Plan: Used `bin/remove destroy` to destory several files. Used `--trace` to verify they wiped file data.
Reviewers: btrahan, chad
Reviewed By: chad
Subscribers: epriestley
Differential Revision: https://secure.phabricator.com/D10300
Summary: This has never been enabled by default, and isn't safe. Remove it since people can use preallocated or EC2 hosts.
Test Plan: Removed it; didn't see it appear on the "Create Blueprint" page.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: epriestley, Korvin
Differential Revision: https://secure.phabricator.com/D10287
Summary: Fixes T5894. This needs some improvement when we lay in real CDN stuff, but should get all the cases right for now.
Test Plan: Thumbnails work properly again.
Reviewers: btrahan, chad
Reviewed By: chad
Subscribers: epriestley
Maniphest Tasks: T5894
Differential Revision: https://secure.phabricator.com/D10299
Summary: Fixes T5798. We basically weren't using the caching mechanism. Also adds service calls for S3 stuff, and support for seeing a little info like you can for conduit.
Test Plan: uploaded a paste, looked at paste list - no s3 service calls. edited the paste, looked at paste list - no s3 service calls and edited content properly shown
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: epriestley, Korvin
Maniphest Tasks: T5798
Differential Revision: https://secure.phabricator.com/D10294
Summary: Fixes T5871. These queries get to use the actual column on the commit table since they are about the "aggregate" state of different audits.
Test Plan: issues queries and got sensible results.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: epriestley, Korvin
Maniphest Tasks: T5871
Differential Revision: https://secure.phabricator.com/D10271
Summary:
Via HackerOne. Chrome (at least) interprets backslashes like forward slashes, so a redirect to "/\evil.com" is the same as a redirect to "//evil.com".
- Reject local URIs with backslashes (we never generate these).
- Fully-qualify all "Location:" redirects.
- Require external redirects to be marked explicitly.
Test Plan:
- Expanded existing test coverage.
- Verified that neither Diffusion nor Phriction can generate URIs with backslashes (they are escaped in Diffusion, and removed by slugging in Phriction).
- Logged in with Facebook (OAuth2 submits a form to the external site, and isn't affected) and Twitter (OAuth1 redirects, and is affected).
- Went through some local redirects (login, save-an-object).
- Verified file still work.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Differential Revision: https://secure.phabricator.com/D10291
Summary: Ref T2787. This provides a purchase detail screen (which has nothing useful on it yet) and converts a bunch of PHIDs into slightly more useful links.
Test Plan: Browsed around my account.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T2787
Differential Revision: https://secure.phabricator.com/D10284
Summary: Fixes T5471.
Test Plan: Used `bin/remove destroy` to destroy a dashboard and a panel.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T5471
Differential Revision: https://secure.phabricator.com/D10283
Summary: Ref T5891. This just modernizes infrastructure.
Test Plan: Viewed "All" and "Unread" notifications.
Reviewers: btrahan, chad
Reviewed By: chad
Subscribers: epriestley
Maniphest Tasks: T5891
Differential Revision: https://secure.phabricator.com/D10281
Summary:
Fixes T5140. When you ajax-edit a task and we send back a full-size card, we currently always put a drag grip on it.
If you clicked the "edit" thing from a priority-ordered list, this is appropriate. However, if you clicked it from some other type of list, it is not.
Pass the expected grippableness through the call.
Test Plan:
- Edited a task from a reorderable (priority-ordered) view, got grip.
- Edited a task from a nonreorderable (author-ordered) view, got no grip.
Reviewers: btrahan, chad
Reviewed By: chad
Subscribers: epriestley
Maniphest Tasks: T5140
Differential Revision: https://secure.phabricator.com/D10282
Summary:
Fixes T5885. This implements optional soft point limits for workboard columns, per traditional Kanban.
- Allow columns to have a point limit set.
- When a column has a point limit, show it in the header.
- If a column has too many points in it, show the column and point count in red.
@chad, this could probably use some design tweaks. In particular:
- I changed the color of "hidden" columns to avoid confusion with "overfull" columns. We might be able to find a better color.
- UI hints for overfull columns might need adjustment.
(After T4427, we'll let you sum some custom field instead of total number of tasks, which is why this is called "points" rather than "number of tasks".)
Test Plan:
{F190914}
Note that:
- "Pre-planning" has a limit, so it shows "4/12".
- "Planning" has a limit and is overfull, so it shows "5 / 4".
- Other columns do not have limits.
- "Post-planning" is a hidden column. This might be too muted now.
Transactions:
{F190915}
Error messages / edit screen:
{F190916}
Reviewers: btrahan, chad
Reviewed By: btrahan
Subscribers: chad, epriestley
Maniphest Tasks: T5885
Differential Revision: https://secure.phabricator.com/D10276
Summary:
Ref T5885. See D10276.
Currently, ActionHeaders can only have minicons, and we don't use them anywhere and they probably don't make much sense in the product anymore.
Instead, allow them to have font icons. Remove minicons, which have no callsites and probably won't in the future.
Test Plan:
{F190925}
- Grepped for `minicons`.
- Grepped for `setHeaderIcon()`.
Reviewers: btrahan, chad
Reviewed By: chad
Subscribers: epriestley
Maniphest Tasks: T5885
Differential Revision: https://secure.phabricator.com/D10277
Summary: Fixes T5884. Macro images are no longer public on most installs. We could generate tokens for them, but this (using Conduit to pull the file data) is easier and more correct.
Test Plan: Logged a bot into IRC and had it spam part of a macro before being killed for flooding.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T5884
Differential Revision: https://secure.phabricator.com/D10274
Summary: Ref T5884. We migrated with "canCDN" and then had live writes with "cancdn". Move everything to "canCDN" for consistency.
Test Plan: Ran migration, verified DB only has "canCDN" afterward.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T5884
Differential Revision: https://secure.phabricator.com/D10273
Summary:
Ref T992.
- Format text/HTML bodies explicitly in `bin/mail show-outbound`.
- Provide `bin/mail show-outbound --dump-html` so you can do something like `bin/mail show-outbound --dump-html > dump.html; open dump.html` to get a browser preview somewhat easily.
Test Plan: Ran `bin/mail show-outbound` with and without `--dump-html` flag.
Reviewers: talshiri, btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T992
Differential Revision: https://secure.phabricator.com/D10272
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: Ref T4427. This always counts 1 task = 1 point. The tricky bit is making this update in JS.
Test Plan: {F190900}
Reviewers: btrahan, chad
Reviewed By: chad
Subscribers: epriestley
Maniphest Tasks: T4427
Differential Revision: https://secure.phabricator.com/D10275
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 T5575. Moves "All" links into title/header. Mark all read floats left, and connection status sits in footer. Also added hints to enable notifications (it's a cool feature).
Test Plan:
Tested locally both menus.
{F190630}
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: epriestley, Korvin
Maniphest Tasks: T5575
Differential Revision: https://secure.phabricator.com/D10269
Summary: Fixes T5453.
Test Plan: made a remarkup comment that "Q1 is dumb and Q10 is awesome" and only Q10 was linked. changed the new setting to have the value " " and the Q1 also started linking.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: epriestley, Korvin
Maniphest Tasks: T5453
Differential Revision: https://secure.phabricator.com/D10270
Summary: Ref T5862. makes the exception work better
Test Plan: issued some queries from audit ui with and without repos - they worked
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: epriestley, Korvin
Maniphest Tasks: T5862
Differential Revision: https://secure.phabricator.com/D10268
Summary: Fixes T5862. The Diffusion table uses `id` but all the other infrastructure uses `phid` so just do a quick load of the repositories to get the ids. Long term, we should re-key the table by phid I think.
Test Plan: made a query with a repository and got a proper result set
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: epriestley, Korvin
Maniphest Tasks: T5862
Differential Revision: https://secure.phabricator.com/D10245
Summary: Should help with debugging. In the web UI there is now a link to "View Email". This uses the same debugging output that `./bin/mail show-outbound` uses. Fixes T5768. Code is very defensive as I think these tables may be truncated eventually?
Test Plan: viewed some emails and it worked!
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: epriestley, Korvin
Maniphest Tasks: T5768
Differential Revision: https://secure.phabricator.com/D10244
Summary:
Fixes T5727. Updates the regexes to split on '-'. Also changes the editor such that tokens are updated by the larger search process. (Note this means we update this data more often then we need to - for every project transaction.)
Users will need to make an edit to a project -or- run `bin/search index "#project-tag"` to make this actually work.
Test Plan: Made "Frontend-Engineering", "Engineering", and "Backend-Enginering". They all showed up in the typeahead!
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: epriestley, Korvin
Maniphest Tasks: T5727
Differential Revision: https://secure.phabricator.com/D10247
Summary: Fixes T5872. This won't show up in the initial email until T4896 is further along.
Test Plan:
```
RECIPIENTS
discoball (Disco Ball)
BODY
epriestley added a comment.
ffkn
USERS
epriestley (Author)
COMMIT
http://local.aphront.com:8080/rPOEMS165b6c54f487
```
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T5872
Differential Revision: https://secure.phabricator.com/D10266
Summary: Fixes T5883. The first time we hit an error we'll continue forward; we only bail after the second time. Instead, check for an error immediately
Test Plan: HA HA HA DID NOT TEST HA HA HA HA
Reviewers: btrahan, cburroughs
Reviewed By: cburroughs
Subscribers: epriestley
Maniphest Tasks: T5883
Differential Revision: https://secure.phabricator.com/D10265
Summary:
Ref T5884. We migrated to add a `canCDN` flag, but the code looks for a `cancdn` flag.
If this fixes the issue, I'll migrate `cancdn` to `canCDN` in the next diff.
Test Plan: Viewed some files, including old files, and saw the cacheability I expected.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T5884
Differential Revision: https://secure.phabricator.com/D10264
Summary: Fixes T4130. Adds a setup warning when ft_min_word_len is set to the default value.
Test Plan: Hit setup warning; resovled setup warning. Searched for "DOS".
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T4130
Differential Revision: https://secure.phabricator.com/D10259
Summary:
Fixes T3347. We can't really do this one as a config thing since we don't know if the user wants to use LDAP.
Instead, just give them a better message than they otherwise get when they try to install/configure/use LDAP.
Test Plan: Faked it and got a reasonable message.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T3347
Differential Revision: https://secure.phabricator.com/D10260
Summary:
Fixes T2605.
- Add a setup warning about the stopword file.
- Provide a simpler stopword file.
Test Plan:
- Hit setup warning.
- Resolved it according to instructions.
- Added "various" to a task, then searched for it, found the task.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T2605
Differential Revision: https://secure.phabricator.com/D10258
Summary: Fixes T3173. This doesn't actually fix T3173 but I'm going to redirect that. It does make the bot quit IRC gracefully, with a nicer quit message, which can be customized.
Test Plan: Got a bot to quit IRC with nice messages.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T3173
Differential Revision: https://secure.phabricator.com/D10257
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: Fixes T4973. For `PhabricatorProjectInterface` objects, add a header to let clients do mail filtering.
Test Plan: Saw `X-Phabricator-Projects: <#goat_farm>` in outbound mail.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: rush898, epriestley
Maniphest Tasks: T4973
Differential Revision: https://secure.phabricator.com/D10256
Summary: Fixes T1922. When a story is about a primary object associated with projects, show those projects on the feed story.
Test Plan: {F190171}
Reviewers: btrahan, chad
Reviewed By: chad
Subscribers: epriestley
Maniphest Tasks: T1922
Differential Revision: https://secure.phabricator.com/D10255
Summary: Fixes T5579. Modern browsers aggressively autofill credentials, but at least Firefox still behaves slightly better with this flag. Hopefully other browsers will follow suit.
Test Plan: Browsed various interfaces, verifying that login interfaces allow autocomplete while non-login interfaces do not.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T5579
Differential Revision: https://secure.phabricator.com/D10253
Summary: Fixes T5869. Ref T4896. This `setID()` method no longer exists.
Test Plan: (WARNING) This is a pain to reproduce locally so I'm just winging it. I'm 99% sure this ID is only used to generate an anchor link. This is a hack to start with, and T4896 will eventualy clean it up properly.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T4896, T5869
Differential Revision: https://secure.phabricator.com/D10254
Summary: This prevents SSH from saving the host key into known_hosts; StrictHostKeyChecking only prevents it from prompting for unknown hosts, but it will still verify hosts against what it has previously saved.
Test Plan: N/A
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: epriestley, Korvin
Differential Revision: https://secure.phabricator.com/D10252
Summary:
Resolves T5831. This modifies the Drydock SSH interface to execute commands under Powershell when the target host platform is Windows. Powershell is far more featured than cmd.exe, and more closely resembles a UNIX shell.
Currently Powershell outputs stderr as an XML blob on a line, and while this code currently doesn't use that, it will allow us in the future (planned next week) to redirect that output to the stderr log instead of having it all merged in with stdout under cmd (where there is no way to distinguish it).
Test Plan:
Ran various native commands and PowerShell commands from a Harbormaster build, including things like:
```
Write-Host ("my test" + ${build.id})
```
and saw:
```
my test679
```
in the output.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: epriestley, Korvin
Maniphest Tasks: T5831
Differential Revision: https://secure.phabricator.com/D10248
Summary: Fixes T5769. Fixes T5861. Add mail tags for "unblock" and "column change".
Test Plan: Did unblocks and column changes, verified the mail got the right mailtags and recipient nondelivery flags.
Reviewers: chad, btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T5861, T5769
Differential Revision: https://secure.phabricator.com/D10241
Summary:
Ref T5861. Ref T5769. If users don't care at all about something, allow them to ignore it.
We have some higher-volume notifications either built now (column changes) or coming (mentions) which users might reasonably want to ignore completely.
Test Plan:
Ignored some notifications, then took appropraite actions. Saw my user culled from the notification subscriber list.
{F189531}
Reviewers: chad, btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T5769, T5861
Differential Revision: https://secure.phabricator.com/D10240
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: Ref T5861. Adds an option to opt out of all notification email. We'll still send you password resets, email verifications, etc.
Test Plan:
{F189484}
- Added unit tests.
- With preference set to different things, tried to send myself mail. Mail respected preferences.
- Sent password reset email, which got through the preference.
Reviewers: btrahan, chad
Reviewed By: chad
Subscribers: rush898, epriestley
Maniphest Tasks: T5861
Differential Revision: https://secure.phabricator.com/D10237
Summary:
Ref T5861. These two options are complex, rarely useful, and not directly related to controlling what mail you receive.
Move them to a separate panel to make way for more stuff on the preferences panel. We'll probably add an "HTML" option to this new panel eventually, too.
Test Plan:
{F189474}
- Used both panels.
- Tested with multiplexing off.
Reviewers: btrahan, chad
Reviewed By: chad
Subscribers: epriestley
Maniphest Tasks: T5861
Differential Revision: https://secure.phabricator.com/D10236
Summary:
Fixes T5185. The fundamental issue is that this `excludePHIDs` property was not saved, so the logic went like this:
- Generate `excludePHIDs` correctly.
- Pass `excludePHIDs` through the stack.
- Perform some other computations correctly.
- Queue the mail for the daemons, throwing it away. {icon bomb}
- Daemons process mail with empty `excludePHIDs` list.
Store it in the persistent properties array instead.
Also remove the "override self mail" thing, since it's only used by `bin/mail send-test` and suffers from the same issue. I think it's too useless to fix, since even if you get caught by it, `bin/mail` makes it clear why the message was dropped.
Test Plan:
Notable:
- `exclude` present in properties
- Exclusion reason under RECIPIENTS header
{P1229}
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T5185
Differential Revision: https://secure.phabricator.com/D10234
Summary:
Fixes T5456. We lost this logic in the transition to applicationtransactions.
When publishing a feed story, mark all of the object's projects as related, so the project filter in feed works.
Test Plan: Made a comment on a task associated with a project, saw the story in filtered feed.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: timor, epriestley
Maniphest Tasks: T5456
Differential Revision: https://secure.phabricator.com/D10233
Summary:
Fixes T5233.
- The mail adapter API currently expects plain addresses (like `a@b.com`) in `addTos()`, and some adapters can not accept fancy verbose addresses (like `"name" <a@b.com>`).
- When we try to send error email, we pass the entire "From" header into the API. This is incorrect.
- Since it would be nice to make this just work in the future, fix it inside the API.
- Specifically, this is reached with: send email -> generates error -> we try to send you an email back -> we send it to your "From" -> some mailers choke on the fancy name if you have one.
Test Plan: Processed an errorneous email with a fancy "From", got a response error.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T5233
Differential Revision: https://secure.phabricator.com/D10232
Summary: Ref T5817. This just fixes the markup in emails, the overall behavior still isn't great. I don't want to spend to much time on Ponder until it ends up somewhere nearer the top of the priority queue.
Test Plan: Viewed feed stories and emails, no stray/clearly-broken HTML.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T5817
Differential Revision: https://secure.phabricator.com/D10231
Summary: Fixes T5859. This doesn't change much, but makes the transaction record a little more accurate and activates stuff like `#hashtags` and `{F123}` causing policy associations.
Test Plan: Used `bin/mail receive-test` and mail receiver script to send bug mail, saw hashtags imply projects.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T5859
Differential Revision: https://secure.phabricator.com/D10229
Summary:
Fixes T5839. If a repository has been force pushed and garbage collected, we might have a ref cursor in the database which still points at the old commit (which no longer exists).
We'll then run a command like `git log <new hash> --not <old hash>` to figure out which commits are newly pushed, and this will bomb out because `<old hash>` is invalid.
Instead, validate all the `<old hash>` values before we try to make use of them.
Test Plan:
- Forced a repository into a bad state by mucking with the datbase, generating a reproducible failure similar to the one in T5839.
- Applied patch.
- `bin/repository update <callsign> --trace` filtered the bad commit and put the repository into the right state.
- Saw new commits recognized correctly.
- Ran `bin/repository update <callsign>` for a Mercurial and SVN repo as a sanity check.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T5839
Differential Revision: https://secure.phabricator.com/D10226
Summary:
Fixes T5184. Fixes T5008. Three issues with stories/notifications about changing the status of tasks which block other tasks:
**Bad Feed Stories**
- Problem: Feed story rendering was confusing (T5184).
- Solution: fix it to provide context.
**Too Many Feed Stories**
- Problem: Feed gets a story for the original task's close ("a closed x"), and a story for each blocked task ("a closed x, a task blocking y").
- "Solution": Punt. These are redundant in the full feed but not in filtered feeds. Right solution is display-time aggregation. No users have really complained about this.
**Too Many Notifications**
- Problem: Users subscribed to both tasks get notified about the clsoe, and also about the unblocked task. These notifications are redundant.
- "Solution": Punt. This is easy to fix by silencing notifications for the sub-editor, but I'm worried it would be confusing. Users haven't complained. Display-time aggregation might be a better fix.
Test Plan: {F189463}
Reviewers: btrahan, chad
Reviewed By: chad
Subscribers: epriestley
Maniphest Tasks: T5008, T5184
Differential Revision: https://secure.phabricator.com/D10235
Summary: Ref T5819. Implements basic icon and color filtering for projects.
Test Plan: {F189350}
Reviewers: btrahan, chad
Reviewed By: chad
Subscribers: epriestley
Maniphest Tasks: T5819
Differential Revision: https://secure.phabricator.com/D10230
Summary:
Fixes T5855. Adds a `--graceful N` flag to `phd stop` and `phd restart`.
`phd` will send SIGINT, wait `N` seconds, SIGTERM, wait 15 seconds, and SIGKILL. By default, `N` is 15.
Test Plan:
- Ran `bin/phd debug ...` and used `^C` to interrupt daemons. Saw graceful shutdown behavior, and abrupt termination on multiple `^C`.
- Ran `bin/phd start`, `bin/phd stop` and `bin/phd restart` with `--graceful` set to various things, notably `0`. Saw graceful shutdowns on the CLI and in the web UI. With `0`, abrupt shutdowns.
Reviewers: btrahan, hach-que
Reviewed By: hach-que
Subscribers: epriestley
Maniphest Tasks: T5855
Differential Revision: https://secure.phabricator.com/D10228
Summary: Resolves T5836. This automatically releases artifacts when Harbormaster builds finish (either passing or failing). This allows Harbormaster to release the Drydock leases it has for hosts.
Test Plan: Tested it with a build plan that passes and fails; tested it with lots of builds running in parallel.
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: epriestley, Korvin
Maniphest Tasks: T5836
Differential Revision: https://secure.phabricator.com/D10208
Summary: This allows timeouts to be specified on SSH connections that Drydock makes. Used in the EC2 allocator to poll for the SSH server starting.
Test Plan: Used in EC2 allocator diff.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: epriestley, Korvin
Differential Revision: https://secure.phabricator.com/D10225
Summary:
Ref T1049. This keeps track of how long a build target takes to execute in Harbormaster and displays it in the build view page. I'm not sure whether "Started" is really that useful once the target has completed?
Also, I change the name of the time taken depending on whether or not the target has completed; if it's still in progress it's called "Elapsed" and if it's completed then it's "Duration". The primary reason for this is that "Duration" sounds like post tense, whereas "Elapsed" is current tense. I'm not sure whether this is okay or not?
Test Plan: Ran a Sleep build step and saw the target dates / times appear correctly.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: talshiri, epriestley, Korvin
Maniphest Tasks: T5824, T1049
Differential Revision: https://secure.phabricator.com/D10174
Summary: To assist with {T5245}, I have added projects back into the lipsum maniphest generator with the edge infrastructure.
Test Plan: Run the lipsum script for PhabricatorManiphestTaskTestDataGenerator and make sure it generates project data.
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: epriestley, Korvin
Differential Revision: https://secure.phabricator.com/D10202
Summary: Fixes T5850. Also fixes some logic where the wrong preempting events could be attached during a bulk query.
Test Plan: Phrequent list now shows preemption-aware times.
Reviewers: hach-que, btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T5850
Differential Revision: https://secure.phabricator.com/D10223
Summary:
Fixes T5848.
- Disallow tracking negative time.
- Preserve note if there's an error with the time selection.
- Show start time and duration.
- Slightly better error messages.
Test Plan: Started and stopped time. Tried to select future/negative ranges.
Reviewers: hach-que, btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T5848
Differential Revision: https://secure.phabricator.com/D10218
Summary:
Fixes T5837. The problem is that the hash is being recognized as a commit hash. We currently fire the object monogram rules fairly early, but there's no real reason to do this. Move them after all of the hyperlink rules:
0 PhutilRemarkupEscapeRemarkupRule
100 PhutilRemarkupMonospaceRule
150 PhutilRemarkupDocumentLinkRule
175 PhrictionRemarkupRule
<<< OLD OBJECT RULE POSITION
200 PhabricatorIconRemarkupRule
200 PhabricatorMemeRemarkupRule
200 DivinerSymbolRemarkupRule
350 DoorkeeperRemarkupRuleJIRA
350 PhabricatorYoutubeRemarkupRule
350 DoorkeeperRemarkupRuleAsana
400 PhutilRemarkupHyperlinkRule
>>> NEW OBJECT RULE POSITION
500 PhabricatorImageMacroRemarkupRule
500 CustomInlineJIRA5Rule
500 PhabricatorMentionRemarkupRule
500 CustomInlineCodeRule
1000 PhutilRemarkupDelRule
1000 PhutilRemarkupBoldRule
1000 PhutilRemarkupItalicRule
1000 PhutilRemarkupUnderlineRule
- The disadvantage of this approach is that `{F123, alt=go look at http://lol.com/ omg}` will parse the URL first, and then fail to resolve the object embed. This seems very rare / unusual.
- The advantage is that all URLs which happen to have monograms in them work.
In the future, we could refine this by separating the rules, so the embed (`{...}`) versions fired at priority 200, while the normal versions fired at priority 450. We can wait for use cases, though. This is a little messy because the same code implements both rules.
Test Plan:
- Verified example in T5837.
- Marked up object rules like `F123` (works), `[[ asdf | F123 ]]` (works), `{F123, alt=http://example.com}` (does not work).
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T5837
Differential Revision: https://secure.phabricator.com/D10212
Summary:
See some discussion here:
24a6eeb8d8 (commitcomment-7334892)
The `protected $properties;` storage parameter added to `ProjectColumn` is shadowed by `getProperties()` in the base class.
Although this works correctly for me, it's ambiguous and worth fixing. Make the base class methods explicit.
Test Plan: Used `grep` to find callers for both methods and renamed them.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Differential Revision: https://secure.phabricator.com/D10210
Summary:
Via HackerOne. If a user adds an email address and typos it, entering `alinculne@gmailo.com`, and it happens to be a valid address which an evil user controls, the evil user can request a password reset and compromise the account.
This strains the imagination, but we can implement a better behavior cheaply.
- If an account has any verified addresses, only send to verified addresses.
- If an account has no verified addresses (e.g., is a new account), send to any address.
We've also received several reports about reset links not being destroyed as aggressively as researchers expect. While there's no specific scenario where this does any harm, revoke all outstanding reset tokens when a reset link is used to improve the signal/noise ratio of the reporting channel.
Test Plan:
- Tried to send a reset link to an unverified address on an account with a verified address (got new error).
- Tried to send a reset link to a verified adddress on an account with a verified address (got email).
- Tried to send a reset link to an invalid address (got old error).
- Tried to send a reset link to an unverified address on an account with only unverified addresses -- a new user (got email).
- Requested several reset links, used one, verified all the others were revoked.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Differential Revision: https://secure.phabricator.com/D10206
Summary: Ref T2787. This is very basic and just helps me know that the data is inserting correctly.
Test Plan: {F187765}
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T2787
Differential Revision: https://secure.phabricator.com/D10205
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:
Fixes T5838.
- We currently try to use a `ConduitAPIMethod` object as a string.
- We then pass that string to the parent's `__construct()` method as `$message`.
Test Plan: Uninstalled Maniphest, then tried to execute `maniphest.createtask`. Got a useful exception message instead of an error during message construction.
Reviewers: joshuaspence, btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T5838
Differential Revision: https://secure.phabricator.com/D10211
Summary:
Fixes T5849. When a new file is created, we might have to actually write the data to a storage engine, or we might be able to just point at data which is already there.
Currently, these two paths handle `$params` with different code and mild behavioral differences. Instead, have them call the same code so they get the same behavior.
Test Plan:
- Uploaded the same file multiple times to home page.
- Uploaded the same file multiple times as profile picture.
- Generated files via Diffusion.
- All the files got the expected properties, whether they were reusing data or not.
Reviewers: btrahan, 20after4
Reviewed By: 20after4
Subscribers: epriestley
Maniphest Tasks: T5849
Differential Revision: https://secure.phabricator.com/D10216
Summary:
Ref T5685. Currently we just 403 on an invalid token, but we can be a little more helpful.
The issues here are:
- If we **do** redirect you on this page and something goes wrong, you might get stuck in a redirect loop.
- If we **don't** redirect you, copy/pasting the link to someone (or reloading the page) gives them a pretty confusing result, since the link doesn't work any more. Prior to this diff, they get a 403.
To mitigate this, do a little better than a bare 403: give them a link to auth and generate a new URI for the file.
If this is still confusing, the next best thing I can come up with is something like this:
- Put some modulous of the timestamp in the URI.
- If the current time is within 2 seconds of the generation time, show this dialog.
- Otherwise, redirect.
That seems like it would be okay, but I worry that "2" has to be small (so links you copy/paste -> chat -> click still work) and a small value means that a small amount of clock skew breaks things. We could use the database clock, but ehhh.
Other ideas:
- Put a hash of the remote IP in the URI, redirect if it doesn't match. Fails for companies behind a NAT gateway but should work in a lot of other cases.
- Just redirect always, there's no reason it should ever loop and browsers don't really do anything bad when there's a loop (they'll show an error after too many redirects).
I'm leaning toward letting this stabilize in the wild for a bit, then trying "always redirect".
Test Plan: {F188914}
Reviewers: btrahan, 20after4
Reviewed By: 20after4
Subscribers: epriestley
Maniphest Tasks: T5685
Differential Revision: https://secure.phabricator.com/D10215
Summary: Ref T5685. We've added a new `canCDN` flag to control whether or not files can be cached and delivered over a CDN. Show this flag in the UI.
Test Plan: Viewed several files, saw correct/expected UI values.
Reviewers: btrahan, 20after4
Reviewed By: 20after4
Subscribers: epriestley
Maniphest Tasks: T5685
Differential Revision: https://secure.phabricator.com/D10213
Summary: See D10189. We should never hit this anymore, so clean it up.
Test Plan:
- Reloaded a board, saw everything stay where it was before the change.
- Added a new task to the project, saw it show up in backlog.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Differential Revision: https://secure.phabricator.com/D10200
Summary: Fixes T5829. This stuff is old and busted, but keep it working for now.
Test Plan: No more fatal when there are recently closed tasks.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T5829
Differential Revision: https://secure.phabricator.com/D10201
Summary:
Fixes T5677.
- Instead of using `sequence == 0` to mean "this is the backlog column", flag the column explicitly.
- Migrate existing sequence 0 columns to have the flag.
- Add the flag when initializing or copying a board.
- Remove special backlog logic when reordering columns.
Test Plan:
- Migrated columns, viewed some boards, they looked identical.
- Reordered the backlog column a bunch of times (first, last, middle, dragged other stuff around).
- Added tasks to a project, saw them show up in the reordered backlog.
- Initialized a new board and saw a backlog column show up.
- Copied an existing board and saw the backlog column come over.
- Tried to hide a backlog column.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T5677
Differential Revision: https://secure.phabricator.com/D10189
Summary: Just wanted to play with this, removes the gradient 'cards' for a flat design.
Test Plan:
Tested various apps, workboards
{F166127}
Reviewers: btrahan, epriestley
Reviewed By: epriestley
Subscribers: hach-que, epriestley, Korvin
Differential Revision: https://secure.phabricator.com/D9515
Summary: This slipped through the datasource modernization stuff.
Test Plan: Used search UI.
Reviewers: rush898, btrahan, chad
Reviewed By: chad
Subscribers: epriestley
Differential Revision: https://secure.phabricator.com/D10196
Summary:
Ref T5024, T4427, T5474, T5523. Instead of separate icons in the column header for "Create Task" and "Edit Column Settings", use a dropdown menu.
- T5024 will likely add a "View Standalone" option.
- T4427 needs header space to show a count.
- T5474 likely needs "Edit Triggers..." (this seems reasonable to separate from editing the name, etc.)
- T5523 likely adds "Move all tasks..." eventually.
Test Plan: {F187414}
Reviewers: btrahan, chad
Reviewed By: chad
Subscribers: epriestley
Maniphest Tasks: T5523, T5474, T5024, T4427
Differential Revision: https://secure.phabricator.com/D10190
Summary: Sets layout as flush when rendering diff table or timeline in a Dialog
Test Plan: Tested each
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: epriestley, Korvin
Differential Revision: https://secure.phabricator.com/D10194
Summary: Fixes T5739. I only got D9857 half right: the new method names are correct, but the bodies needed to change too.
Test Plan: Signed a document as an anonymous user.
Reviewers: btrahan, chad
Reviewed By: chad
Subscribers: epriestley
Maniphest Tasks: T5739
Differential Revision: https://secure.phabricator.com/D10191
Summary:
Via the UI adding a mailinglist for CC works, but via
the API currently it shows:
>One or more PHIDs were invalid for ccPHIDS
This removes the user validation check for ccPHIDs.
(I left it in for other things like owner since that seems
still appropriate?)
Test Plan:
used arc locally to add a mailinglist to cc
```echo '{"id": 2, "ccPHIDs": ["PHID-MLST-ohduchbv4dfimk7opt3r"]}' | arc call-conduit maniphest.update```
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: epriestley, Korvin
Differential Revision: https://secure.phabricator.com/D10193
Summary:
Ref T4807. This is probably a complete fix, but I'd be surprised if there isn't a little cleanup I missed.
When users drag tasks on a "natural"-ordered workboard, leave things where they put them.
This isn't //too// bad since a lot of the existing work is completely reusable (e.g., we don't need any new JS).
Test Plan:
- Dragged a bunch of stuff around, it stayed where I put it after dropped and when reloaded.
- Dragged stuff across priorities, no zany priority changes (in "natural" mode).
- Created new tasks, they show up at the top.
- Tagged new tasks, they show up at the top of backlog.
- Swapped to "priority" mode and got sorting and the old priority-altering reordering.
- Added tasks in priority mode.
- Viewed task transactions for correctness/sanity.
Reviewers: btrahan, chad
Reviewed By: chad
Subscribers: chad, epriestley
Maniphest Tasks: T4807
Differential Revision: https://secure.phabricator.com/D10182
Summary:
Ref T4807. This is an alternative to D10179. The problem these diffs solve is that I want to be able to reorder a column's positions without having to load the actual objects, but that's difficutl because two positions may have the same sequence number (and I think it's good that we allow that, since it makes a bunch of other stuff way easier).
Instead of using the object ID (e.g., the task ID) to reorder positions with the same sequence, use the position itself. This is a little easier, is less ambiguous if columns eventually have several types of objects, and produces a better behavior when old objects are freshly added to a board. For example, if you tag `T300` with `#project`, this new rule will push it to the top of "Backlog" while the old rule might have buried it deep. I think this behavior is desirable and more "natural".
When creating a group of new rows, we do order the batch by ID, so a group of freshly-tagged objects float to the top togehter in ID order. This seems like the most natural rule, too.
Test Plan:
- Loaded some boards with implicit objects on them (freshly tagged tasks) and saw rows create.
- Verified new rows created in the right order.
- Dragged some tasks around.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T4807
Differential Revision: https://secure.phabricator.com/D10180
Summary:
Ref T4807. This doesn't actually do anything yet, but adds a dropdown menu for choosing an ordering and gets all the UI working correctly.
This also fixes a bug where column hidden state wouldn't persist across filter changes.
(I won't land this until it does something, but the next diff will probably be a mess so this seemed like a clean place to sever things.)
Test Plan:
{F187114}
- Altered sort ordering.
- Altered hidden state and filters, verified all states persisted correctly.
- Added `phlog()` to edit/create and move controllers and verified they receive sort information.
Reviewers: btrahan, chad
Reviewed By: chad
Subscribers: swisspol, chad, epriestley
Maniphest Tasks: T4807
Differential Revision: https://secure.phabricator.com/D10178
Summary:
CanCDN flag indicates that a file can be served + cached
via anonymous content distribution networks.
Once D10054 lands, any files that lack the CanCDN flag
will require a one-time-use token and headers will
prohibit cache to protect sensitive files from
unauthorized access.
This diff separates the CanCDN changes from the code that
enforces these restrictions in D10054 so that the changes
can be tested and refined independently.
Test Plan: Work in progress
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: rush898, qgil, epriestley, aklapper, Korvin
Maniphest Tasks: T5685
Differential Revision: https://secure.phabricator.com/D10166
Summary: Fixes T5705. This was just derp; instead of returning the duration of the first slice, return the duration of all the slices.
Test Plan: Added unit tests. Saw reasonable results in the UI.
Reviewers: btrahan, hach-que
Reviewed By: hach-que
Subscribers: epriestley
Maniphest Tasks: T5705
Differential Revision: https://secure.phabricator.com/D10184
Summary: Fixes T5423, "is newly created" herald rule fails on dry runs
Test Plan: Create herald "is newly created" rule, and do a dry run on an existing pholio mock, differential commit, or maniphest task. Should not return an exception.
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: epriestley, Korvin
Maniphest Tasks: T5423
Differential Revision: https://secure.phabricator.com/D10187
Summary:
Ref T5245. This removes some hacks and activates two meaningful interactions:
- The "projects" field goes through shared code now.
- Mentioning projects in tasks using hashtags now tags them.
Test Plan:
- Viewed a task with projects.
- Viewed a task with no projects.
- Viewed a task with projects and board positions.
- Viewed a revision with projects.
- Made a `#hashtag` comment in Maniphest and got a project association.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T5245
Differential Revision: https://secure.phabricator.com/D10177
Summary: Ref T1049. This allows build steps to create URI artifacts, which can be used to link to external builds and other resources.
Test Plan: Used a build step in an external library to test the creation of a URI artifact and verified it appeared correctly.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: epriestley, Korvin
Maniphest Tasks: T1049
Differential Revision: https://secure.phabricator.com/D10173
Summary: Resolves T5814. Ref T1049. This changes "Stop" to "Pause" in the UI (internally it's still referred to as Stop).
Test Plan: Viewed builds and saw the intended wording.
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: epriestley, Korvin
Maniphest Tasks: T1049, T5814
Differential Revision: https://secure.phabricator.com/D10172
Summary: Use cutlery icon for hilarity. Ref T5768.
Test Plan: made something with remarkup in it, used 'view raw' and saw the remarkup raw in a nice little dialogue.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: epriestley, Korvin
Maniphest Tasks: T5768
Differential Revision: https://secure.phabricator.com/D10183
Summary: Fixes T5478. For "personal" installs use the person icon; for global use the global icon. For both providing explanatory tooltip text about what's going on. This will need to be updated if / when we start installing dashboards to other applications. Also, this query isn't 100% optimized but the major part *is* so I think its okay.
Test Plan: Installed a dashboard for personal use and verified correct icon / text showed up. Did the same for global installed dashboard...!
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: epriestley, Korvin
Maniphest Tasks: T5478
Differential Revision: https://secure.phabricator.com/D10181
Summary:
This is pretty basic allowing a user to set the
policy as a valid string ('no-one' or 'users') or
as a valid PHID. Without an explicit policy
a permissive one is set.
Test Plan:
Tested using the python-phabricator module (very basic api wrapper).
The arc cli syntax was evading me.
```import base64
from phabricator import Phabricator
phab = Phabricator()
with open('mypic.jpg') as f:
encoded = base64.b64encode(f.read())
//set no-one as viewer which really means author only?
phab.file.upload(name='mypicnoone.jpg',
data_base64=encoded,
viewPolicy='no-one')
//set a specific phid as policy in this case a project
phab.file.upload(name='mypicphid.jpg',
data_base64=encoded,
viewPolicy='PHID-PROJ-fgvvnafmhvkgn2d5a4rf')
//no set policy ends up as 'users' i.e. ('all users')
phab.file.upload(name='mypicdefault.jpg', data_base64=encoded)```
Not able to really test canCDN attribute but it should be
fine and I tried to make it all consistent with D10166
Reviewers: 20after4, epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: 20after4, epriestley, Korvin
Maniphest Tasks: T5685
Differential Revision: https://secure.phabricator.com/D10164
Summary: Minor correction to correct spelling of alternate (the 'n' was missing).
Test Plan: reviewer to verify correct spelling
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: epriestley
Differential Revision: https://secure.phabricator.com/D10175
Summary:
Currently, we'll try to publish notifications while running tests. This is at best unnecessary and at worst problematic (we don't stub out the server).
For now, just never publish them.
Test Plan: Ran unit tests with notifications enabled but the server down and didn't get a bunch of warnings.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Differential Revision: https://secure.phabricator.com/D10171
Summary: Fixes T5735, setting up Phacility for huge financial success.
Test Plan:
opened up Safari - who logs in with Safari anyway? - and could still view a macro
could also view the list of macros
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: epriestley, Korvin
Maniphest Tasks: T5735
Differential Revision: https://secure.phabricator.com/D10170
Summary:
Fixes T5476. Using edges to store which objects are on which board columns ends up being pretty awkward. In particular, it makes T4807 very difficult to implement.
Introduce a dedicated `BoardColumnPosition` storage.
This doesn't affect ordering rules (T4807) yet: boards are still arranged by priority. We just read which tasks are on which columns out of a new table.
Test Plan:
- Migrated data, then viewed some boards. Saw exactly the same data.
- Dragged tasks from column to column.
- Created a task directly into a column.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T5476
Differential Revision: https://secure.phabricator.com/D10160
Summary: so you can see who the event is about...! Fixes T5621.
Test Plan: saw the creator of each event on /calendar/event/query/all/
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: epriestley, Korvin
Maniphest Tasks: T5621
Differential Revision: https://secure.phabricator.com/D10169
Summary:
The `##` on the beginning of a line is now a header.
Use more-preferred backticks.
Test Plan: nope lol
Reviewers: joshuaspence
Reviewed By: joshuaspence
Subscribers: epriestley
Differential Revision: https://secure.phabricator.com/D10157
Summary:
Restores functionality for Flowdock->Chatbot adapter.
Most likely the result of API changes in the year since the original patch was contributed,
the flowdock adapter no longer worked.
This makes a few tweaks to both the base streaming adapter class and the flowdock adpater. I took care to not disturb the functionality of the campfire adapter, but I don't have any way to test it.
Test Plan: I am new here and I have no idea what to write other than sarcastic things but I'll most like amend this after review.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: epriestley, Korvin
Differential Revision: https://secure.phabricator.com/D10168
Summary: Fixes T5773.
Test Plan: Made a poll and voted on it. Deleted it via ./bin/remove destory V1. No errors and the poll is gone.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: epriestley, Korvin
Maniphest Tasks: T5773
Differential Revision: https://secure.phabricator.com/D10167
Summary: ...and fix an error where lines that start with ##X## are rendering incorrectly by switching to alternate syntax `X`. Fixes T5806.
Test Plan: read the docs and they looked good
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: epriestley, Korvin
Maniphest Tasks: T5806
Differential Revision: https://secure.phabricator.com/D10165
Summary: this data is a little weird since its user-entered and we need to put it in a web page un-escaped for the font to load correctly. Ergo, we use a regex to make the input safe / sane, and said regex needs to support a '.'. Fixes T5810.
Test Plan: added Fixedsys Excelsior 3.01 to my system and was able to set my preference and get the new font
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: dereckson, epriestley, Korvin
Maniphest Tasks: T5810
Differential Revision: https://secure.phabricator.com/D10163
Summary: This moves artifacts and build target messages into tabs.
Test Plan: Viewed build plan, saw the tabs appear when the steps had appropriate artifacts and / or messages.
Reviewers: #blessed_reviewers, epriestley, chad
Reviewed By: #blessed_reviewers, epriestley
Subscribers: epriestley, Korvin
Differential Revision: https://secure.phabricator.com/D10161
Summary: This automatically hides any empty build logs from Harbormaster, so that they do not appear.
Test Plan: Viewed a build plan where the logs were empty and didn't see them appear.
Reviewers: chad, #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: epriestley, Korvin
Differential Revision: https://secure.phabricator.com/D10091
Summary: Currently, we just create a default "backlog" column if / when you visit a workboard for the first time. Post this patch, instead you see a blocking dialog that lets you either create the default backlog column or import columns from another project. In the case of the latter, the user gets another dialog which lets them select any project of which they are a member that also has columns in it. Note that only not hidden columns get imported. Fixes T4431.
Test Plan:
- made a new workboard and got my new dialog. made a default backlog and it worked!
- made a new workboard again and tried the import flow - it also worked.
- verified projects with no columns do not show up in import dialog
- verified project with / without columns still all show up in maniphest project typeahead
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: epriestley, Korvin
Maniphest Tasks: T4431
Differential Revision: https://secure.phabricator.com/D10153
Summary: Ref T4896. This was used by the old audit comment storage, which is now defunct.
Test Plan: Grepped for callsites in the codebase.
Reviewers: joshuaspence, btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T4896
Differential Revision: https://secure.phabricator.com/D10152
Summary:
Fixes T5728. In particular:
- `/tag/XYZ/` now works as an alias for `/tag/xyz/`.
- `arc todo --project ASDF` now works as an alias for `arc todo --project asdf`.
Test Plan: Called `project.query` and visited `/tag/LBHABLHBH/`.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: aklapper, epriestley
Maniphest Tasks: T5728
Differential Revision: https://secure.phabricator.com/D10144
Summary: Fixes T5510. This purely reduces false positives from HackerOne: we currently rotate CSRF tokens, but do not bind them explicitly to specific sessions. Doing so has no real security benefit and may make some session rotation changes more difficult down the line, but researchers routinely report it. Just conform to expectations since the expected behavior isn't bad and this is less work for us than dealing with false positives.
Test Plan:
- With two browsers logged in under the same user, verified I was issued different CSRF tokens.
- Verified the token from one browser did not work in the other browser's session.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T5510
Differential Revision: https://secure.phabricator.com/D10136
Summary:
Fixes T5509. Currently, existing sessions live on even if you change your password.
Over the course of the program, we've recieved a lot of HackerOne reports that sessions do not terminate when users change their passwords. I hold that this isn't a security vulnerability: users can explicitly manage sessions, and this is more general and more powerful than tying session termination to password resets. In particular, many installs do not use a password provider at all (and no researcher has reported this in a general, application-aware way that discusses multiple authentication providers).
That said, dealing with these false positives is vaguely time consuming, and the "expected" behavior isn't bad for users, so just align behavior with researcher expectations: when passwords are changed, providers are removed, or multi-factor authentication is added to an account, terminate all other active login sessions.
Test Plan:
- Using two browsers, established multiple login sessions.
- In one browser, changed account password. Saw session terminate and logout in the second browser.
- In one browser, removed an authentication provider. Saw session terminate and logout in the second browser.
- In one browser, added MFA. Saw session terminate and logout in the second browser.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T5509
Differential Revision: https://secure.phabricator.com/D10135
Summary:
Fixes T5506. Depends on D10133. When users remove an email address or change their primary email address, invalidate any outstanding password reset links.
This is a very small security risk, but the current behavior is somewhat surprising, and an attacker could sit on a reset link for up to 24 hours and then use it to re-compromise an account.
Test Plan:
- Changed primary address and removed addreses.
- Verified these actions invalidated outstanding one-time login temporary tokens.
- Tried to use revoked reset links.
- Revoked normally from new UI panel.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T5506
Differential Revision: https://secure.phabricator.com/D10134
Summary:
Ref T5506. This makes it easier to understand and manage temporary tokens.
Eventually this could be more user-friendly, since it's relatively difficult to understand what this screen means. My short-term goal is just to make the next change easier to implement and test.
The next diff will close a small security weakness: if you change your email address, password reset links which were sent to the old address are still valid. Although an attacker would need substantial access to exploit this (essentially, it would just make it easier for them to re-compromise an already compromised account), it's a bit surprising. In the next diff, email address changes will invalidate outstanding password reset links.
Test Plan:
- Viewed outstanding tokens.
- Added tokens to the list by making "Forgot your password?" requests.
- Revoked tokens individually.
- Revoked all tokens.
- Tried to use a revoked token.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T5506
Differential Revision: https://secure.phabricator.com/D10133
Summary:
Ref T4896. Now that we have a transaction editor, we can delete a giant block of hacks.
I believe this also resolves the commit/task attachment issues @joshuaspence and @mbishopim3 mentioned.
Test Plan: Attached and detached commits and tasks.
Reviewers: btrahan, joshuaspence, mbishopim3
Reviewed By: mbishopim3
Subscribers: mbishopim3, epriestley, joshuaspence
Maniphest Tasks: T4896
Differential Revision: https://secure.phabricator.com/D10138
Summary: Ref T4896. I got this logic slightly wrong when porting it over: we always want to write this relationship, to allow members of a project with an audit request against a commit to resign and get it out of their queue.
Test Plan:
- Resigned from a commit with an existing relationship.
- Resigned from a commit with no existing relationship, saw one added.
Reviewers: btrahan, joshuaspence, mbishopim3
Reviewed By: mbishopim3
Subscribers: epriestley
Maniphest Tasks: T4896
Differential Revision: https://secure.phabricator.com/D10137
Summary:
Ref T4589. We don't recognize task descriptions as remarkup blocks, so `{F...}` references in them do not get attached to the objects, and thus no policy exemption is created.
Recognize them, which activates `{F...}` and `@mentions`.
We probably have a few more of these in other applications, but it's not a big deal to clean them up as they arise.
Test Plan: Uploaded a file to a task in the description field, saw it attach and get a policy exemption.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T4589
Differential Revision: https://secure.phabricator.com/D10139
Summary:
Ref T4589. When you look at a file, we load attached objects in order to run the "you can see this if you can see any attached object" policy check.
However, right now the subquery inherits the "throw on filter" flag from the parent query. This inheritance makes sense in other cases[1], but because this is an "ANY" rule it does not make sense here. In practice, it means that if the file is attached to several objects, and any of them gets filtered, you can not see the file.
Instead, explicitly drop the flag for this subquery.
[1] Sort of. It doesn't produce wrong results in other cases, but now that I think about it might produce a less-tailored error than it could. I'll look into this the next time I'm poking around.
Test Plan:
- Viewed an "All Users" file attached to a private Mock.
- Prior to this patch, I incorrectly received an exception when the Mock was loaded. This is wrong; I should be able to see the file because the policy is "All Users".
- After the patch, I can correctly view the file, just not the associated mock.
{F127074}
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: 20after4, aran, epriestley
Maniphest Tasks: T4589
Differential Revision: https://secure.phabricator.com/D8498
Summary:
Fixes T4589. This implements much better policy behavior for files that aligns with user expectations.
Currently, all files have permissive visibility.
The new behavior is:
- Files uploaded via drag-and-drop to the home page or file upload page get permissive visibility, for ease of quickly sharing things like screenshots.
- Files uploaded via the manual file upload control get permissive visibility by default, but the user can select the policy they want at upload time in an explicit/obvious way.
- Files uploaded via drag-and-drop anywhere else (e.g., comments or Pholio) get restricted visibility (only the uploader).
- When the user applies a transaction to the object which uses the file, we attach the file to the object and punch a hole through the policies: if you can see the object, you can see the file.
- This rule requires things to use ApplicationTransactions, which is why this took so long to fix.
- The "attach stuff to the object" code has been in place for a long time and works correctly.
I'll land D8498 after this lands, too.
Test Plan:
- Uploaded via global homepage upload and file drag-and-drop upload, saw permissive visibility.
- Uploaded via comment area, saw restricted visibility.
- After commenting, verified links were established and the file became visible to users who could see the attached object.
- Verified Pholio (which is a bit of a special case) correctly attaches images.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T4589
Differential Revision: https://secure.phabricator.com/D10131
Summary: Ref T4589. Depends on D10129. In addition to letting users change the visibility policy for files, also allow them to choose a policy explicitly when a file is uploaded.
Test Plan: Uploaded several files using the plain old uploader, saw appropriate visibility policies applied.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T4589
Differential Revision: https://secure.phabricator.com/D10130
Summary: Ref T4589. Allow users to adjust visibility settings on files explicitly. This makes it easier to understand and manage upcoming changes in T4589.
Test Plan: Changed the view policy for a file several times.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T4589
Differential Revision: https://secure.phabricator.com/D10129
Summary: Ref T4896. This converts the last "CommentEditor" to a transaction editor and removes a large part of the old code.
Test Plan:
- Added comments.
- Accepted / added auditors.
- Added inline comments.
Reviewers: joshuaspence, btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T4896
Differential Revision: https://secure.phabricator.com/D10128
Summary: Ref T4896. Invoke the new editor directly instead of in a roundabout way when handling Audit email.
Test Plan: Used `bin/mail receive-test` to simulate mail, saw comment post with proper content source.
Reviewers: joshuaspence, btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T4896
Differential Revision: https://secure.phabricator.com/D10127
Summary: Ref T4896. Use the new transaction-oriented `PhabricatorAuditEditor` directly instead of invoking it via the old editor.
Test Plan: Used Conduit to add a comment, use silent mode, and accept a commit.
Reviewers: joshuaspence, btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T4896
Differential Revision: https://secure.phabricator.com/D10126
Summary: Ref T4896. Applies these actions using new transaction stuff.
Test Plan:
- Accepted and raised concern with my own commit, verifying the special project/package behavior.
- Accepted and raised concern with another author's commit, verifying the authority-over-packages/projects behavior.
- Accepted a commit I was not affiliated wiht, verifying the "join as an auditor" behavior.
Reviewers: joshuaspence, btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T4896
Differential Revision: https://secure.phabricator.com/D10125
Summary: Ref T4896. Hook these up with new stuff.
Test Plan:
- Closed an audit.
- Resigned from an audit.
Reviewers: joshuaspence, btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T4896
Differential Revision: https://secure.phabricator.com/D10124
Summary:
Ref T4896. Move the write for "Add Auditors" inside the new Editor.
There are no longer any readers or writers for metadata, so remove the calls for it.
Test Plan: Added auditors from the web UI.
Reviewers: joshuaspence, btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T4896
Differential Revision: https://secure.phabricator.com/D10123
Summary: Ref T4659. Because we JOIN, tasks with no value are filtered out. Instead, LEFT JOIN.
Test Plan: Issued an "Order by" and got all applicable tasks. Adjusted values and saw order change.
Reviewers: btrahan, hach-que
Reviewed By: hach-que
Subscribers: epriestley
Maniphest Tasks: T4659
Differential Revision: https://secure.phabricator.com/D10119
Summary:
Resolves T4659. This implements support for sorting tasks by custom fields.
Some of this feels hacky in the way it's hooked up to the Maniphest search engine and task query.
Test Plan: Queryed on a custom date field, with a small page size, and moved back and forth through the result set.
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: epriestley, Korvin
Maniphest Tasks: T4659
Differential Revision: https://secure.phabricator.com/D10106
Summary: This class was renamed in D9991 but the filename is incorrect.
Test Plan: Eyeball it
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: epriestley, Korvin
Differential Revision: https://secure.phabricator.com/D10118
Summary: Ref T4896. Instead of using custom stuff, use standard stuff.
Test Plan: Viewed a bunch of feed stories and published some over the Asana bridge.
Reviewers: joshuaspence, btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T4896
Differential Revision: https://secure.phabricator.com/D10114
Summary: Ref T4896. Replace custom stuff with standard stuff.
Test Plan:
- Sent a bunch of email and it all looked sensible/correct.
- Made sure to test inlines, specifically, as they're a bit tricky.
Reviewers: joshuaspence, btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T4896
Differential Revision: https://secure.phabricator.com/D10112
Summary: Ref T4896.
Test Plan: Made an unusual comment, then found it by searching.
Reviewers: btrahan, joshuaspence
Reviewed By: btrahan, joshuaspence
Subscribers: epriestley
Maniphest Tasks: T4896
Differential Revision: https://secure.phabricator.com/D10110
Summary:
Ref T4896. Replaces more custom stuff with standard stuff. In particular:
- No more fake proxy writes;
- no more fake detection of `@mentions`.
For now, the old code still applies most of the effects and handles feed and email.
Test Plan:
- Added comments.
- Added comments with inline comments.
- Added just inline comments.
- Added comments with Conduit.
- Previewed comments.
- Added CCs explicitly and with `@mentions`.
- Added auditors.
- Accepted a commit.
Reviewers: joshuaspence, btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T4896
Differential Revision: https://secure.phabricator.com/D10109
Summary:
Ref T4896. Currently, subscriptions to commits are stored as auditors with a special "CC" type.
Instead, use normal subscriptions storage, reads and writes.
Test Plan:
- Ran migration and verified data still looked good.
- Viewed commits in UI and saw "subscribers".
- Saw "Automatically Subscribed", clicked Subscribe/Unsubscribe on a non-authored commit, saw subscriptions update.
- Pushed a commit through Herald rules and saw them trigger subscriptions and auditors.
- Used "Add CCs".
- Added CCs with mentions.
Reviewers: btrahan, joshuaspence
Reviewed By: btrahan, joshuaspence
Subscribers: epriestley
Maniphest Tasks: T4896
Differential Revision: https://secure.phabricator.com/D10103
Summary: This class was renamed in D9991, but the filename is incorrect.
Test Plan: Eyeball it
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: epriestley, Korvin
Differential Revision: https://secure.phabricator.com/D10117
Summary:
Fixes T5666. When we have a pretty link right now it can conflict with form data; e.g. if you have 'statuses=open' in the URI and then uncheck status = open in the UI, you will still get the open status in the next search.
To fix this, set the form action explicitly to lose all the get parameter junk.
Test Plan: tried the test case in T5666 / this description and it no longer failed...!
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: epriestley, Korvin
Maniphest Tasks: T5666
Differential Revision: https://secure.phabricator.com/D10115
Summary:
pre-patch "Mark All Read" marks *all* unread notifications as read. This is a race condition in that the user is looking at some set of notiifcations and that set may update such that the newest notifications aren't shown. An example might be if sitting on the notifications page or having the menu open while a new notification comes in... Note re-opening the menu would show the latest notifications.
This patch makes it so "Mark All Read" links only marks the notifications currently loaded (and older.) Fixes T5764.
Additionally, if there is nothing to "mark read" the button / link "Mark All Read" will have a disabled style and yield a dialog saying "nothing to mark as read".
Test Plan: carefully tracked ?chronoKey populating correctly in various links. Verified query constructed properly too.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: epriestley, Korvin
Maniphest Tasks: T5764
Differential Revision: https://secure.phabricator.com/D10113
Summary: $this->id wasn't being set in this case so just set it explicitly after we finish loading the project. Fixes T5763.
Test Plan: links were broken no longer!
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: epriestley, Korvin
Maniphest Tasks: T5763
Differential Revision: https://secure.phabricator.com/D10108
Summary: This moves the status property of the build to the bottom of the property list so that it matches the build targets.
Test Plan: Viewed a build, saw the status in the right position.
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: epriestley, Korvin
Differential Revision: https://secure.phabricator.com/D10096
Summary: This shows the status icon and color along side the build status on the build view controller.
Test Plan: Viewed a build, saw the icon appear.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: epriestley, Korvin
Differential Revision: https://secure.phabricator.com/D10094
Summary:
Ref T1049. This uses tabs on build targets to hide the configuration details and variables by default, instead promoting the target name, it's status and a description of the build step. The description is a new field on each build step.
The primary advantage of having a description on build steps is that DevOps can configure appropriate description information (including any troubleshooting information for build failures) on build steps, and developers who have builds fail against their code review can then look at this information.
Test Plan: Viewed a build plan and saw the appropriate information.
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: epriestley, Korvin
Maniphest Tasks: T1049
Differential Revision: https://secure.phabricator.com/D10093
Summary: Ref T1049. This hides the build target messages area if there are no messages for the target. Since most of the time a build target won't recieve any messages, this area is confusing because it's always empty.
Test Plan: Viewed a build, saw the empty build target message areas disappear.
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: epriestley, Korvin
Maniphest Tasks: T1049
Differential Revision: https://secure.phabricator.com/D10092
Summary: We've received feedback that the "core - exception" is incredibly confusing, to the point where developers see this and write off the build failure as a Phabricator error that is unrelated to their changes.
Test Plan: Ran a build with a `exit 1` run step, didn't see the "core - exception" appear.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: epriestley, Korvin
Differential Revision: https://secure.phabricator.com/D10090
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: This makes input artifacts imply the appropriate build step dependencies in the build plan. That is, if you use a host artifact in a build step, it will then implicitly depend on the 'Lease Host' step.
Test Plan: Viewed the build plan with the artifacts, saw the dependencies. Ran a build, saw everything execute in the correct order.
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: epriestley, Korvin
Differential Revision: https://secure.phabricator.com/D10089
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:
Depends on D9806. This implements the build simulator, which is used to calculate the order of build steps in the plan editor. This includes a migration script to convert existing plans from sequential based to dependency based, and then drops the sequence column.
Because build plans are now dependency based, the grippable and re-order behaviour has been removed.
Test Plan: Tested the migration, saw the dependencies appear correctly.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: epriestley, Korvin
Differential Revision: https://secure.phabricator.com/D9847
Summary: Fixes T5751. Currently, we incorrectly overwrite `$xactions`.
Test Plan: Closed a blocking task, got an email about the correct transaction set ("closed task; added comment") instead of an overwritten transaction set ("closed blocking task").
Reviewers: chad, btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T5751
Differential Revision: https://secure.phabricator.com/D10088
Summary: This was causing a crash according to a conversation with @rfergu in IRC. The issue is that if `$images_to_show` is empty, then `$image` never gets set, and it attempts to call `getID` on a null / unset variable.
Test Plan: @rfergu confirmed this fixed the crash.
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: Korvin, rfergu, epriestley
Differential Revision: https://secure.phabricator.com/D10073
Summary: Fixes T5653.
Test Plan: made a macro with sound and it played. changed it to not play and it did not play. changed it back and it played once more.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: epriestley, Korvin
Maniphest Tasks: T5653
Differential Revision: https://secure.phabricator.com/D10085
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: At least on my install, sorting was pretty borked from a type issue. (e.g. "unbreak now" of 100 sorting as less than "High" of 90). Fix this with some parseInt action. Also support adding new cards with the new colsort stuff. The clever bit here is to include the task ID in the sorting vector because the task ID wins ties at the moment I think / new tasks need to show up before older tasks when they are initially created. Fixes T5716.
Test Plan: added many "normal" priority cards and saw them fly in correctly. changed priority and moved correctly. made no edits and no moves were made correctly.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: epriestley, Korvin
Maniphest Tasks: T5716
Differential Revision: https://secure.phabricator.com/D10081
Summary: been some changes here and this code was broked. turns out we re-assign $action like two lines later and never used the initial value, so we can simply delete the offending line. Fixes T5745.
Test Plan: submitted inline comment pre-patch and fatal. re-submitted post patch and great success!
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: epriestley, Korvin
Maniphest Tasks: T5745
Differential Revision: https://secure.phabricator.com/D10078
Summary: Currently, content runs before header. Instead, run header, content, footer. This makes it easier to use `header.php` for includes, for Phacility.
Test Plan: Made Phacility blog work better, see next diff.
Reviewers: chad
Reviewed By: chad
Subscribers: epriestley
Differential Revision: https://secure.phabricator.com/D10067
Summary:
Ref T4896. Depends on D10056. Moves search indexing to standard infrastructure.
Also, fixes a bug where inline comments would not be indexed.
Test Plan: Used `bin/search index ... --trace` to view index construction of a commit, saw all the comments and inlines get indexed.
Reviewers: btrahan, joshuaspence
Reviewed By: joshuaspence
Subscribers: epriestley
Maniphest Tasks: T4896
Differential Revision: https://secure.phabricator.com/D10057
Summary: Ref T4896. Depends on D10055. This uses core rendering stuff for audit comments, and fixes all the wonkiness with inlines so we can actually land the migration.
Test Plan: Viewed, previewed and edited various types of comments in Diffusion.
Reviewers: chad, btrahan, joshuaspence
Reviewed By: joshuaspence
Subscribers: epriestley
Maniphest Tasks: T4896
Differential Revision: https://secure.phabricator.com/D10056
Summary:
Ref T4896. Depends on D10052. This is the major/scary migration, but not really so bad. It is substantially similar to D8210, but less complex because there are fewer actions here.
This moves `PhabricatorAuditComment` storage to `PhabricatorAuditTransaction`, then reads `PhabricatorAuditComment`s as a proxy around the new objects.
Test Plan:
- Before migrating, browsed around. Nothing appeared broken.
- Migrated cleanly.
- Viewed old transactions (inlines, comments, accept/reject/etc, add auditors, add ccs, implicit CCs).
- Added all of those comment types.
- Edited a draft.
- Deleted a draft.
- Spot checked the database for sanity.
Reviewers: btrahan, joshuaspence
Reviewed By: joshuaspence
Subscribers: epriestley
Maniphest Tasks: T4896
Differential Revision: https://secure.phabricator.com/D10055
Summary:
Ref T4896. Depends on D10023. Prepares the code for the final migration.
The transaction table stores one row per distinct effect (e.g., add CCs) rather than one row per user action (e.g., "add CCs + comment"). We can double-read that table as long as the code doesn't expect transactions/comments to have multiple different effects, and doesn't try to write any such rows.
Everywhere that we were writing a big "X + Y" comment, write two separate "X" and "Y" comments instead. Like D10023, this disrupts the UI a little (you get more boxes), but that will be resolved once the rendering code swaps over. Otherwise, this retains the existing behavior.
Test Plan:
- Used `diffusion.createcomment` to add comments, raise concern, and accept.
- Previewed commenting, adding auditors/ccs, accepting, raising concern.
- Actually performed commenting, adding auditors/ccs, accepting, raising concern.
- Added a user with mentions.
- Added an explicit CC and a mention user.
Reviewers: btrahan, joshuaspence
Reviewed By: joshuaspence
Subscribers: epriestley
Maniphest Tasks: T4896
Differential Revision: https://secure.phabricator.com/D10052
Summary:
Ref T4896. Moves us closer to migrating comments to transactions by building a transaction per inline.
This makes the UI a little wonky, and it will get slightly worse until we swap to the new UI and grouping/collapsing starts working. It's still usable, there's just a box per inline.
Test Plan:
- Added a comment.
- Added an inline comment.
- Added a comment and an inline comment.
Reviewers: btrahan, joshuaspence
Reviewed By: joshuaspence
Subscribers: epriestley
Maniphest Tasks: T4896
Differential Revision: https://secure.phabricator.com/D10023
Summary: Adds Phriction to list of apps that use Source Sans as default font in addition to Legalpad and Diviner.
Test Plan: Tested various layouts imported from secure. Should be reasonably tested, but will follow up on secure.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: epriestley, Korvin
Differential Revision: https://secure.phabricator.com/D10064
Summary:
Fixes T5707. After work on T5245, the `$task` object may not have the correct set of task PHIDs attached to it when Herald is invoked.
Instead, just fetch the authoritative set. This is simple, and Herald is not super performance sensitive (happens on a write) so hitting the DB once is fine.
Test Plan: Created a task with projects, verified the projects field contained the correct values when processed by Herald.
Reviewers: chad, btrahan, joshuaspence
Reviewed By: joshuaspence
Subscribers: epriestley
Maniphest Tasks: T5707
Differential Revision: https://secure.phabricator.com/D10060
Summary: Fixes T5717. Like other partial edits, object links should not be blocked by unrelated missing fields on the object.
Test Plan:
- Linked two objects.
- Verified the inverse editor already sets "continue on missing fields" and "continue on no effect".
Reviewers: chad, btrahan, joshuaspence
Reviewed By: joshuaspence
Subscribers: epriestley
Maniphest Tasks: T5717
Differential Revision: https://secure.phabricator.com/D10059
Summary: Initially the change is aimed to solve issue with line breaks being lost in projects descriptions. But it is done in a general place so line breaks behavior is more consistent all over the place.
Test Plan:
- Write a multiline description of the project, using single \n for line breakers.
- View project details in project/view/X?
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: epriestley, Korvin
Differential Revision: https://secure.phabricator.com/D10014
Summary:
Handling readmes with no extension is a bit of a hack, but seemed like a small cost.
The Big Win here is that you can commit README.remarkup and README.md and have both Phabricator and GitHub render __with__ //all// ##the## ~~pretty~~ **markup**.
Test Plan: Looked at some readme files.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: epriestley, Korvin
Differential Revision: https://secure.phabricator.com/D10047
Summary:
Instead of allowing all routes based on security.alternate-file-domain, now, when security.alternate-file-domain is set, and the request matches this domain, requests are validated against an explicit list. Allowed routes:
- /res/
- /file/data/
- /file/xform/
- /phame/r/
This will be redone by T5702 to be less of a hack.
Test Plan:
- browse around (incl. Phame live) to make sure there is no regression from this when security.alternate-file-domain is not used.
- check that celerity resources and files (incl. previews) are served with security.alternate-file-domain set.
- check that phame live blog is serving its css correctly with security.alternate-file-domain set.
- check that requests outside of the whitelist generate an exception for security.alternate-file-domain
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: epriestley, Korvin
Differential Revision: https://secure.phabricator.com/D10048
Summary: As mentioned on rP8ce35e6b67e7e2a81b274bab7a6dd19dedb4df06, `setConcreteOnly(true)` can be omitted since (lacking magical powers) `loadObjects()` will always only instantiate concrete objects.
Test Plan: N/A
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: epriestley, Korvin
Differential Revision: https://secure.phabricator.com/D10050
Fixes an issue with D9991. A user was hitting the following exception:
```
echo '{}' | arc --conduit-uri='http://phabricator.joshuaspence.com'
call-conduit conduit.query
Waiting for JSON parameters on stdin...
Exception
[HTTP/500] Internal Server Error
>>> UNRECOVERABLE FATAL ERROR <<<
Call to a member function getAPIMethodName() on a non-object
/usr/src/phabricator/src/applications/conduit/method/ConduitQueryConduitAPIMethod.php:34
┻━┻ ︵ ¯\_(ツ)_/¯ ︵ ┻━┻
(Run with --trace for a full exception trace.)
```
Auditors: epriestley
Summary: Fixes T5102. Bumps the version to clear caches and documents the `#` headers.
Test Plan: Read documentation.
Reviewers: asherkin, btrahan, chad
Reviewed By: chad
Subscribers: epriestley
Maniphest Tasks: T5102
Differential Revision: https://secure.phabricator.com/D10046
Summary: Fixes T5695. A Conduit "method does not exist" exception is somewhat expected... there is no need to `phlog` the exception.
Test Plan: Called a non-existent Conduit method. Saw no exceptions in the error logs.
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: epriestley, Korvin
Maniphest Tasks: T5695
Differential Revision: https://secure.phabricator.com/D10042
Summary: Ref T4896. Begins laying groundwork to split comments apart so they behave like transactions, ultimately enabling the migration.
Test Plan: Made several different types of comments, verified resulting email looks OK.
Reviewers: btrahan, joshuaspence
Reviewed By: joshuaspence
Subscribers: epriestley
Maniphest Tasks: T4896
Differential Revision: https://secure.phabricator.com/D10022
Summary:
Ref T4896. This is substantially similar to D8196.
Migrate the comment text out of the `audit_comment` table and into the `audit_transaction_comment` table. Do double reads on `PhabricatorAuditComment` so the APIs aren't disturbed. The old table is still updated.
Test Plan:
- Before applying migration, cleared cache and browsed around. Things looked fine, except no comment text.
- Applied migration.
- Cleared cache, browsed around, saw all my old comments.
- Added some new comments.
- Spot checked migrated and new rows in database.
Reviewers: btrahan, joshuaspence
Reviewed By: joshuaspence
Subscribers: epriestley
Maniphest Tasks: T4896
Differential Revision: https://secure.phabricator.com/D10020
Summary: Ref T4896. Buries all direct access to the table so we can limit the surface area affected by the migration.
Test Plan:
- Grepped for `PhabricatorAuditComment`.
- Grepped for `audit_comment`.
- Viewed a bunch of comments.
- Added a comment.
- Reindexed a commit.
- Searched for unique term in new comment.
Reviewers: btrahan, joshuaspence
Reviewed By: joshuaspence
Subscribers: epriestley
Maniphest Tasks: T4896
Differential Revision: https://secure.phabricator.com/D10019
Summary:
Ref T4896. This is substantially identical to the process which Differential followed, and mostly copied from the original Differential migration and the Differential proxy object.
Basically, we move all the data over but the application can't tell, and the same APIs do reads and writes to the new table.
Test Plan:
- Browsed UI before migrating, everything looked fine (but no inlines).
- Ran migration.
- Verified draft and published comments survived migration.
- Added a draft.
- Previewed draft.
- Submitted draft.
- Viewed standalone with drafts and published comments.
- Sanity checked data in database, didn't see anything unusual.
Reviewers: btrahan, joshuaspence
Reviewed By: joshuaspence
Subscribers: epriestley
Maniphest Tasks: T4896
Differential Revision: https://secure.phabricator.com/D10018
Summary:
Ref T4896. This adds the new storage, without any code changes.
This storage is substantially identical to the Differential storage, except that `changesetID` has been replaced by `pathID`.
I've retained the properties intended to be used to implement T1460. They might not be quite right, but at least we'll be able to make any fixes consistently to both applications. For now, these fields are empty and ignored.
Test Plan: Ran `./bin/storage upgrade`. Nothing calls this code yet.
Reviewers: btrahan, joshuaspence
Reviewed By: joshuaspence
Subscribers: epriestley
Maniphest Tasks: T4896
Differential Revision: https://secure.phabricator.com/D10017
Summary: Ref T4896. Move all direct accesses to the inline comment table behind a small amount of API to make it easier to migrate the table.
Test Plan:
- Grepped for `PhabricatorAuditInlineComment`.
- Grepped for `audit_inlinecomment`.
- Created a draft comment.
- Previewed a draft comment.
- Reloaded page, still saw draft.
- Viewed standalone, still saw draft.
- Made comment, inline published.
- Added a draft, saw both.
- Edited inline comment.
- Reindexed commit.
- Searched for unique word in published comment, found commit.
- Searched for unique word in draft comment, no results.
Reviewers: btrahan, joshuaspence
Reviewed By: joshuaspence
Subscribers: epriestley
Maniphest Tasks: T4896
Differential Revision: https://secure.phabricator.com/D10016
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:
The image was incorrectly served on /rsrc without using celerity. This
is no longer routable with recommendations in D9401.
Test Plan: Browse to home page and inspect javascript link for this resource.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: epriestley, Korvin
Differential Revision: https://secure.phabricator.com/D10040
Summary:
The ability to query by name(s) already exists
but is not bound to the conduit method. This binds it.
Test Plan:
Tested locally
echo '{"names": ["myprojectbyname"]}' | arc call-conduit project.query
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: epriestley, Korvin, 20after4
Differential Revision: https://secure.phabricator.com/D10032
Summary: These files were added in D10001, which was submitted before (but landed after) D9982 had landed.
Test Plan: `arc unit`
Reviewers: epriestley, #blessed_reviewers, chad
Reviewed By: #blessed_reviewers, chad
Subscribers: epriestley, Korvin
Differential Revision: https://secure.phabricator.com/D10033
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: Ref T2787. There were some mega-uggo buttons and such; reduce the uggo-ness by a hair.
Test Plan: {F179686}
Reviewers: btrahan, chad
Reviewed By: chad
Subscribers: epriestley
Maniphest Tasks: T2787
Differential Revision: https://secure.phabricator.com/D10006
Summary:
Ref T2787. Carts need a status so we can tell if they've been purchased. Also kind of get WePay working as a one-time provider, and let charges not have a methodPHID (they won't for one-time providers).
All the status stuff is still super crazy rough and you can do things like start a checkout, add a bunch of stuff to your cart, complete the checkout, and have Phabricator think you paid for all the stuff you added. But this is fine for now since you can't actually edit carts, and also none of this is at all usable anyway. I'll refine some of the workflows in future diffs, for now I'm just getting things hooked up and technically working.
Test Plan:
- Purcahsed a cart and got a sort of status/done screen instead of a "your money is gone" exception.
- Went through the WePay flow and got a successful test checkout.
Reviewers: btrahan, chad
Reviewed By: chad
Subscribers: epriestley
Maniphest Tasks: T2787
Differential Revision: https://secure.phabricator.com/D10003
Summary: Ref T2787. Makes charges a real object, allows providers to apply them. We are now (just barely) capable of stealing users' money.
Test Plan: {F179584}
Reviewers: btrahan, chad
Reviewed By: chad
Subscribers: epriestley
Maniphest Tasks: T2787
Differential Revision: https://secure.phabricator.com/D10002
Summary:
Ref T2787. Make carts and purchases real objects, with storage, that kind-of work.
Roughly, the idea here is that applications create "purchases" (like "1 large t-shirt") and add them to "carts" (a user can have a lot of different carts at the same time), then hand things off to Phortune to deal with actualy charging a card. Roughly this works like Paypal or other similar systems do, except Phortune is the thing the user gets handed off to.
This doesn't do anything interesting/useful yet.
Also fix some bugs and update some UI.
Test Plan: Added a product to a cart, saw it in cart screen.
Reviewers: btrahan, chad
Reviewed By: chad
Subscribers: epriestley
Maniphest Tasks: T2787
Differential Revision: https://secure.phabricator.com/D10001
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. It is superfluous to include "base" in the name of an abstract base class. Furthermore, it is not done consistently within the code base.
Test Plan: Ran `arc unit`.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: epriestley, Korvin
Maniphest Tasks: T5655
Differential Revision: https://secure.phabricator.com/D9989
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: Fixes T5661. We may now pick up a conflicting `dateCreated` field from an edge table join.
Test Plan: Ran a project + dateCreated filtering query, no longer got an exception.
Reviewers: btrahan, joshuaspence
Reviewed By: joshuaspence
Subscribers: epriestley
Maniphest Tasks: T5661
Differential Revision: https://secure.phabricator.com/D9997
Summary: Fixes T5659. When building a token after a user selection, we currently use the `value` as the token text, but sometimes that's an internal name which doesn't make much sense to users. For projects, it is now "sluga slugb Proper Display Name". If available, use `displayName` instead.
Test Plan: Typed some projects into a tokenizer, got display names only.
Reviewers: chad, joshuaspence
Reviewed By: joshuaspence
Subscribers: epriestley
Maniphest Tasks: T5659
Differential Revision: https://secure.phabricator.com/D9996
Summary: Allow `PhrictionDocument` to be permanently deleted with `./bin/remove destroy`.
Test Plan:
Deleted a Phriction document with `./bin/remove` and verified that the database was in the expected state.
```
> ./bin/remove destroy PHID-WIKI-auj57rauigvcqvv5feh6
IMPORTANT: OBJECTS WILL BE PERMANENTLY DESTROYED!
There is no way to undo this operation or ever retrieve this data.
These 1 object(s) will be completely destroyed forever:
- PHID-WIKI-auj57rauigvcqvv5feh6 (PhrictionDocument)
Are you absolutely certain you want to destroy these 1 object(s)? [y/N] y
Destroying objects...
Destroying PhrictionDocument PHID-WIKI-auj57rauigvcqvv5feh6...
Permanently destroyed 1 object(s).
```
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: epriestley, Korvin
Differential Revision: https://secure.phabricator.com/D9976
Summary:
Commits don't support `PhabricatorApplicationTransactionInterface` yet, so the "Edit Maniphest Tasks" dialog from the commit UI currently bombs.
Hard-code it to do the correct writes in a low-level way. After T4896 we can remove this and do `ApplicationTransaction` stuff.
Test Plan: Used the "Edit Maniphest Tasks" UI from Diffusion.
Reviewers: joshuaspence
Reviewed By: joshuaspence
Subscribers: epriestley
Differential Revision: https://secure.phabricator.com/D9975
Summary: Allow `PhabricatorMetaMTAMailingList` to be permanently deleted with `./bin/remove destroy`.
Test Plan:
```
./bin/remove destroy PHID-MLST-nseux3r55escj573shsf
IMPORTANT: OBJECTS WILL BE PERMANENTLY DESTROYED!
There is no way to undo this operation or ever retrieve this data.
These 1 object(s) will be completely destroyed forever:
- PHID-MLST-nseux3r55escj573shsf (PhabricatorMetaMTAMailingList)
Are you absolutely certain you want to destroy these 1 object(s)? [y/N] y
Destroying objects...
Destroying PhabricatorMetaMTAMailingList PHID-MLST-nseux3r55escj573shsf...
Permanently destroyed 1 object(s).
```
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: epriestley, Korvin
Differential Revision: https://secure.phabricator.com/D9979
Summary:
Some mailers remove the duplicate entries themselves, but some (Mailgun) don't.
This affects installations with metamta.one-mail-per-recipient set to false, and will cause
- ugly looking "to" entries. Gmail, for example, collapses to+cc entries to one list, so you get something that looks like "to: me me john"
- It sometimes causes duplicate delivery of the same message when used in conjuction with Google Groups. I suspect that their message de-dup mechanism is confused by it (I fuzzed it directly with Mailgun, and saw the same message delivered twice - once directly through mailgun, and bounced again through Google Groups). This doesn't happen when the entries are not duplicated.
Test Plan: Created some tasks. Added subscribers. Things seem to work reasonably well.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: epriestley, Korvin
Differential Revision: https://secure.phabricator.com/D9978
Summary: These got removed recently but I missed one callsite.
Test Plan: Used `git grep` to double check all other callsites.
Reviewers: joshuaspence
Reviewed By: joshuaspence
Subscribers: epriestley
Differential Revision: https://secure.phabricator.com/D9973
Summary:
Fixes T5614. Ref T4420. Other than the "users" datasource and a couple of others, many datasources ignore what the user typed and just return all results, then rely on the client to filter them.
This works fine for rarely used ("legalpad documents") or always small ("task priorities", "applications") datasets, but is something we should graudally move away from as datasets get larger.
Add a token table to projects, populate it, and use it to drive the datasource query. Additionally, expose it on the applicationsearch UI.
Test Plan:
- Ran migration.
- Manually checked the table.
- Searched for projects by name from ApplicationSearch.
- Searched for projects by name from typeahead.
- Manually checked the typeahead response.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T5614, T4420
Differential Revision: https://secure.phabricator.com/D9896
Summary: Ref T5651. Currently, the Aphlict server returns either `200 OKAY` or `400 Bad Request`. We could return more specific errors in some cases and this may assist with debugging.
Test Plan:
Sent myself a test notification at `/notification/status/` and saw the Aphlict server process the request (running in debug mode). Also poked around with `curl`:
```
> curl http://localhost:22281/
405 Method Not Allowed
> curl http://localhost:22281/ -d ""
400 Bad Request
> curl http://localhost:22281/foobar/
404 Not Found
```
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: epriestley, Korvin
Maniphest Tasks: T5651
Differential Revision: https://secure.phabricator.com/D9967
Summary: Fixes T4420. This isn't perfect but is one step less janky, at least.
Test Plan: Viewed debugging view at `/typeahead/class/`, no unlabeled columns.
Reviewers: btrahan, chad
Reviewed By: chad
Subscribers: epriestley
Maniphest Tasks: T4420
Differential Revision: https://secure.phabricator.com/D9903
Summary: Ref T4420. We don't currently pass placeholder text properly, but should.
Test Plan: Saw placeholder text in Herald.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T4420
Differential Revision: https://secure.phabricator.com/D9901
Summary:
Ref T4420. This was a performance hack introduced long ago to make typeaheads for users a little cheaper. The idea was that you could load some of an object's columns and skip other ones.
We now always load users on demand, so the cost of loading the whole objects is very small. No other use cases ever arose for this, and it seems unlikely that they will in the future. Remove it all.
Test Plan:
- Grepped for `CONFIG_PARTIAL_OBJECTS`.
- Grepped for `dirtyFields`.
- Grepped for `missingFields`.
- Grepped for `resetDirtyFields`.
- Grepped for `loadColumns`.
- Grepped for `loadColumnsWhere`.
- Grepped for `loadRawDataWhere`.
- Loaded and saved some lisk objects.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T4420
Differential Revision: https://secure.phabricator.com/D9895
Summary:
Ref T4420. If a datasource does not specify an icon explicitly, check if the PHID type has a default, and use that.
This leaves us with only Projects and some special stuff setting explicit icons, and reduces code duplication.
Test Plan: Used typeahead to find all affected object types.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T4420
Differential Revision: https://secure.phabricator.com/D9894
Summary: Ref T4420. This one is users plus "upforgrabs". I renamed that to "none" and gave it a special visual style to make it more discoverable. Future diffs will improve this.
Test Plan:
- Used it in global search.
- Used it in batch editor.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T4420
Differential Revision: https://secure.phabricator.com/D9891
Summary: Ref T4420. This is just "project", plus the special "no project" token, but that doesn't actually work. Replace it with a normal project typeahead. This is only used in Maniphest's reports.
Test Plan: Searched for a couple of projects in reports.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T4420
Differential Revision: https://secure.phabricator.com/D9890
Summary: Ref T4420. Bring the global search up to date.
Test Plan: Typed various things into global search.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T4420
Differential Revision: https://secure.phabricator.com/D9889
Summary: Ref T4420. Call this "auditor" since that's what it is.
Test Plan:
- Edited auditors in auditor search.
- Edited auditors in "add auditors" in Diffusion.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T4420
Differential Revision: https://secure.phabricator.com/D9888
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. Earlier changes in this stack introduced new edge strings, provide English translations.
Test Plan: This is hard to test exhastively, but I poked around the UI and saw some of these rendering correctly. If I messed anything up it's trivial to fix when someone spots it.
Reviewers: joshuaspence, chad
Reviewed By: joshuaspence, chad
Subscribers: epriestley
Maniphest Tasks: T5245
Differential Revision: https://secure.phabricator.com/D9920
Summary:
Ref T5245. We'll still display the old ones, but write real edge transactions now -- not TYPE_PROJECTS transactions.
Some code remains to show the existing transactions. The next diff will modernize the old transactions so we can remove this code.
Test Plan:
- Previewed a project-editing comment.
- Submitted a project-editing comment.
- Edited a task's projects.
- Batch edited a task's projects.
Reviewers: joshuaspence, chad, btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T5245
Differential Revision: https://secure.phabricator.com/D9852
Summary: Ref T5245. This property predates edges and is unusual in modern applications. Stop writes to it and populate it implicitly from edges when querying.
Test Plan:
- Viewed task list.
- Created a task.
- Added and removed projects from tasks.
Reviewers: joshuaspence, chad, btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T5245
Differential Revision: https://secure.phabricator.com/D9851
Summary: Ref T5245. This moves the actual storage over and stops reads and writes to the old table.
Test Plan:
- Verified tasks retained projects across the migration.
- Added and removed projects from tasks.
- Searched for: all, any, users' projects, not-in-projects, no-projects.
Reviewers: chad, btrahan, joshuaspence
Reviewed By: joshuaspence
Subscribers: epriestley
Maniphest Tasks: T5245
Differential Revision: https://secure.phabricator.com/D9850
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. With work elsewhere (notably, D9839) we can remove this TODO and use real transactions.
Test Plan: Pushed a `closes Txxx` commit and got a close + transaction.
Reviewers: chad, btrahan, joshuaspence
Reviewed By: joshuaspence
Subscribers: epriestley
Maniphest Tasks: T5245
Differential Revision: https://secure.phabricator.com/D9848
Summary:
Ref T5245. This hooks up the translation/rendering methods added previously.
These are messy, but now extractable/translatable.
Test Plan: Viewed edge transactions and stories, saw correct strings.
Reviewers: chad, btrahan, joshuaspence
Reviewed By: joshuaspence
Subscribers: epriestley
Maniphest Tasks: T5245
Differential Revision: https://secure.phabricator.com/D9841
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
Summary:
Ref T5245. A very long time ago I had this terrible idea that we'd let objects react to edges being added and insert transactions in response.
This turned out to be a clearly bad idea very quickly, for like 15 different reasons. A big issue is that it inverts the responsibilities of editors. It's also just clumsy and messy.
We now have `PhabricatorApplicationTransactionInterface` instead, which mostly provides a cleaner way to deal with this.
Implement `PhabricatorApplicationTransactionInterface`, implicitly moving all the attach actions (task/task, task/revision, task/commit, task/mock) to proper edge transactions.
The cost of this is that the inverse edges don't write transactions -- if you attach an object to another object, only the object you were acting on posts a transaction record. This is sort of buggy anyway already. I'll fix this in the next diff.
Test Plan: Attached tasks, revisions and mocks to a task, then detached them.
Reviewers: chad, btrahan, joshuaspence
Reviewed By: joshuaspence
Subscribers: epriestley
Maniphest Tasks: T5245
Differential Revision: https://secure.phabricator.com/D9838
Summary:
Ref T5245. I want to add a new capability to edge types, which is a good opportunity to move away from `PhabricatorEdgeConfig`, which isn't modular.
This is basically the same as the modularization of PHID types, which has worked well. Add `PhabricatorEdgeType` and provide an adaption layer for the existing code.
This has no runtime changes, except the fixed edge constant.
Test Plan: Ran `var_dump(PhabricatorEdgeType::getAllTypes())` and got reasonable looking output.
Reviewers: chad, btrahan, joshuaspence
Reviewed By: joshuaspence
Subscribers: epriestley
Maniphest Tasks: T5245
Differential Revision: https://secure.phabricator.com/D9837
Summary: Ref T5245. Currently, task/project links rely on side effects in `save()`. Make them more transaction-oriented, with the goal of moving fully to edges a few diffs down the line.
Test Plan:
- Added and removed projects using "Edit Task", "Associate Projects" comment action, and Herald.
- Verified database ended up in the expected state.
Reviewers: chad, btrahan, joshuaspence
Reviewed By: joshuaspence
Subscribers: epriestley
Maniphest Tasks: T5245
Differential Revision: https://secure.phabricator.com/D9833
