Summary:
Ref T2783. ConduitCall currently has logic to pick a random remote server, but this is ultimately not appropriate: we always want to send requests to a specific server. For example, we want to send repository requests to a server which has that repository locally. The repository tier is not homogenous, so we can't do this below the call level.
Make ConduitCall always-local; logic above it will select ConduitCall for an in-process request or do service selection for an off-host request via ConduitClient.
Test Plan:
- Browsed some pages using ConduitCall, everything worked.
- Grepped for removed stuff.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T2783
Differential Revision: https://secure.phabricator.com/D10959
Summary: This adds back the title to the header link and scans through the codebase for instances where
Test Plan: Tested as many ObjectItemLists as I could find (each app homepage), there may be outliers, but can resolve those individually.
Reviewers: btrahan, epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Differential Revision: https://secure.phabricator.com/D10961
Summary: Fixes T6562, Title/Description querying for Passphrase
Test Plan: Open Passphrase, open advanced queries, enter a title and/or description. Search results should show credentials matching the search.
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T6562
Differential Revision: https://secure.phabricator.com/D10953
Summary: Ref T6713. We were dropping latest transaction ID. This should fix the "easy" part of T6713.
Test Plan: tried to add participants and it worked! (removing participants only allows yourself, but that worked too.)
Reviewers: chad, epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T6713
Differential Revision: https://secure.phabricator.com/D10952
Summary: Fixes T6719. At some point, we added automagical subscriptions via @mentions, and these were failing in project descriptions from a lack of an implementation in the editor. Said "implementation" is to do nothing, but it needs to be there nonetheless.
Test Plan: updated a project mentioning someone in the description and it worked. also saw a 'subscriber added' transaction
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T6719
Differential Revision: https://secure.phabricator.com/D10951
Summary: Cleans up spacing, updates to fonts instead of images. Fixed some mobile issues.
Test Plan:
Test with and without counts on desktop, tablet, mobile. Test layout in FF, Chrome, IE.
{F246745}
{F246746}
Reviewers: btrahan, epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Differential Revision: https://secure.phabricator.com/D10948
Summary: Fixes T6664, clicking search icon in empty search field should link to advanced search
Test Plan: navigate to home page, click search icon or click into search box and hit enter. Advanced search page should open.
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T6664
Differential Revision: https://secure.phabricator.com/D10947
Summary:
The `$timeline` variable is undefined. I was seeing the following error in the logs:
```
EXCEPTION: (RuntimeException) Undefined variable: timeline at [<phutil>/src/error/PhutilErrorHandler.php:210]
#0 PhutilErrorHandler::handleError(integer, string, string, integer, array) called at [<phabricator>/src/applications/harbormaster/controller/HarbormasterStepEditController.php:205]
#1 HarbormasterStepEditController::processRequest() called at [<phabricator>/src/aphront/AphrontController.php:33]
#2 AphrontController::handleRequest(AphrontRequest) called at [<phabricator>/webroot/index.php:103]
```
Test Plan: Created a build step without a fatal error.
Reviewers: btrahan, hach-que, epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: Korvin, epriestley
Differential Revision: https://secure.phabricator.com/D10941
Summary:
Fixes T6702. Ref T3554. Currently, tasks can be cancelled, retried and freed from the web UI by any logged in user.
This isn't appreciably dangerous (I can't come up with a way that a user could do anything security-affecting), but I think I probably intended this to be admin-only, but these actions should move to the CLI anyway.
Move them to the CLI. Lay some groundwork for some future `bin/worker cancel --class SomeTaskClass`, but don't implement that yet.
Test Plan: Used `cancel`, `retry` and `free` from the CLI. Hit all the error/success states.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T3554, T6702
Differential Revision: https://secure.phabricator.com/D10939
Summary: Ref T3669. Probably. Adds a yellow warning at the top of the Diff View and makes the comment draft icon yellow on lists of revisions.
Test Plan:
Test a diff with many warnings, see warning. Test a diff with draft comments, see warning. Test new icon in list view.
{F230133}
{F230134}
Reviewers: btrahan, epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T3669
Differential Revision: https://secure.phabricator.com/D10789
Summary: Fixes T6699. We need to "loadInlineComments" consistently, though for an unexpected reason - this mutates the $changesets to include all $changesets that have an associated inline comment, which is necessary to make them render properly.
Test Plan: Took a diff with inline comments and updated it, noting the inline comments disappeared. applied this patch and the inlines reappeared.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T6699
Differential Revision: https://secure.phabricator.com/D10935
Summary: Change icon for Settings app to more match previous. Also align plus icon a little better.
Test Plan: Lots of staring.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Differential Revision: https://secure.phabricator.com/D10934
Summary: Fixes T6694. Ref T4712. Turns out the logic here was slightly incorrect; we don't want to use the id of the last thing we hid but rather the first thing we show. I had garbage test data ("asdsadsadsa", etc) I guess so I didn't notice this.
Test Plan: made a new task where user a and user b alternated 3 comments each, cooperatively numbering them from 1 - 20. as both users, showed older transactions. pre-patch the issue described in T6694 occurred and post patch I saw the entire counting sequence.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T4712, T6694
Differential Revision: https://secure.phabricator.com/D10933
Summary: Fixes T6693.
Test Plan:
Made a bunch of comments on a diff with differential, being sure to leave inlines here and there. This reproduced the issue in T6693. With this patch this issue no longer reproduces!
Successfully "showed older changes" in Maniphest too.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T6693
Differential Revision: https://secure.phabricator.com/D10931
Summary: Updates header to use font-icons instead of images.
Test Plan: Test desktop and mobile layouts, Chrome, FF, Safari, IE.
Reviewers: btrahan, epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Differential Revision: https://secure.phabricator.com/D10930
Summary: Only necessary for edits, only bother if the comment version is greater than 1. Ref T6690. This is another way to fix T6690 -- this check will never run since you can't edit a conpherence comment -- **but** the fix already applied should happen too to future proof Conpherence.
Test Plan: made a comment on a diff - success. edited the comment and mentions were generated.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T6690
Differential Revision: https://secure.phabricator.com/D10928
Summary: Fixes T6690. The editor innards end up loading the conpherence object, whose policy is dictated by these participation objects.
Test Plan: pre patch could not create new conpherences. post patch I can create conpherences! i can also add people to conpherences and it works.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T6690
Differential Revision: https://secure.phabricator.com/D10927
Summary:
Ref T4712. Specifically...
- Differential
- needed getApplicationTransactionViewObject() implemented
- Audit
- needed getApplicationTransactionViewObject() implemented
- Repository
- one object needed PhabricatorApplicationTransactionInterface implemented
- setShouldTerminate(true)
- Ponder
- BONUS BUG FIX - leaving a comment on an answer had a bad redirect URI
- both PonderQuestion and PonderAnswer needed PhabricatorApplicationTransactionInterface implemented
- setShouldTerminate(true) on both "history" controllers
- left a "TODO" on buildAnswers on the question view controller, which is non-standard and should be re-written eventually
- Phortune
- BONUS BUG FIX - fix new user "createNewAccount" code to not fatal
- PhortuneAccount, PhortuneMerchant, and PhortuneCart needed PhabricatorApplicationTransactionInterface implemented
- setShouldTerminate(true) on Account view, merchant view, and cart view controller
- Fund
- Legalpad
- Nuance
- NuanceSource needed PhabricatorApplicationTransactionInterface implemented
- Releeph (this product is kind of a mess...)
- HACKQUEST - had to manually create an arcanist project to even be able to make a "product" and get started...!
- BONUS BUG FIX - make sure to "setName" on product edit
- ReleephProject (should be ReleepProduct...?), ReleephBranch, and ReleepRequest needed PhabricatorApplicationTransactionInterface implemented
- Harbormaster
- HarbormasterBuildable, HarbormasterBuild, HarbormasterBuildPlan, and HarbormasterBuildStep all needed PhabricatorApplicationTransactionInterface implemented
- setShouldTerminate(true) all over the place
Test Plan: foreach application, viewed the timeline(s) and made sure they still rendered
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T4712
Differential Revision: https://secure.phabricator.com/D10925
Summary: Ref T4712. These are a bit time consuming to test so might as well send off a batch now and again.
Test Plan: foreach impacted controller, made sure the timeline rendered as it did before. for project column and config, noted the "should terminate" UI was also rendered unlike before.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T4712
Differential Revision: https://secure.phabricator.com/D10923
Summary: Fixes T6648. We do some automagical hotness based on the text you enter in remarkup textareas - e.g. adding projects or mentioning other objects. Refine the code here so that even when just editing a comment we build these transactions and apply them.
Test Plan: edited a comment and noted new mentions and projects showed up appropriately...!
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T6648
Differential Revision: https://secure.phabricator.com/D10922
Summary:
Ref T4712. Thus far, it seems that most "non-standard" things can be done pretty easily in the controller. Aside from deploying, this diff had to fix a few bugs / missing implementations of stuff.
(Notably, PhabricatorAuthProviderConfig, HeraldRule, PhabricatorSlowvotePoll, and AlmanacNetwork needed to implement PhabricatorApplicationTransactionInterface, PhabricatorAuthAuthProviderPHIDType had to be added, and a rendering bug in transactions of type PhabricatorOAuth2AuthProvider had to be fixed.)
Test Plan: Almanac - looked at binding, device, network, and service view controllers and verified timeline displayed properly. Herald - looked at a rule and verified timeline. Slowvote - looked at a vote and verified timeline. Auth - looked at an auth provider (Facebook) and verified proper display of transactions within timeline.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T4712
Differential Revision: https://secure.phabricator.com/D10921
Summary: These have all been modernized.
Test Plan: Browse Diffusion on a narrow screen.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Differential Revision: https://secure.phabricator.com/D10920
Summary: Ref T4712. This adds pagination. Future diffs will need to deploy `buildTransactionTimeline` everywhere and massage this stuff as necessary if we hit any special cases.
Test Plan: Set page size to "5" to make it need to paginate often. Verified proper transactions loaded in and the javascript actions worked.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T4712
Differential Revision: https://secure.phabricator.com/D10887
Summary:
Fixes T6619. In `{Xnnn key=value, key=value}` we did not require a separator between the object and the key-value part. This could lead to `{rX11aaa}` being parsed as `{rX11 aaa}`, i.e. a reference to `rX11` with parameter `aaa` set.
Instead, require a space or comma before we'll parse key-value parts of embedded objects.
Test Plan:
Added and executed unit tests.
{F242002}
Reviewers: chad, btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T6619
Differential Revision: https://secure.phabricator.com/D10915
Summary: I think this was a "hacked" sub thing that never got updated when we switched to a real editor? I am not 100% sure how these methods are used, so please let me know if I should expand my test plan. Fixes T6659.
Test Plan: made a diff from the web ui, looked up the phid from mysql, ran bin/remove destroy <phid>, and it worked!
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T6659
Differential Revision: https://secure.phabricator.com/D10911
Summary: Fixes T6658.
Test Plan: made a diff with no repository and default policy and it worked!
Reviewers: epriestley, chad
Reviewed By: chad
Subscribers: Korvin, epriestley
Maniphest Tasks: T6658
Differential Revision: https://secure.phabricator.com/D10910
Summary: See <https://phabricator.wikimedia.org/T906>. This behavior is a bug; we should remove the button if the user can't use the application.
Test Plan:
- With Macro uninstalled, did these things verifying the button vanished:
- Sent a user a message.
- Edited a revision.
- Edited repository basic information.
- Edited an initiative.
- Edited a Harbormaster build step.
- Added task comments.
- Edited profile blurb.
- Edited blog description.
- Commented on Pholio mock.
- Uploaded Pholio image.
- Edited Phortune merchant.
- Edited Phriction document.
- Edited Ponder answer.
- Edited Ponder question.
- Edited Slowvote poll.
- Edited a comment.
- Reinstalled Macro and saw button come back.
- Used button to put silly text on a funny picture.
Reviewers: btrahan, chad
Reviewed By: chad
Subscribers: epriestley
Differential Revision: https://secure.phabricator.com/D10900
Summary: Fixes positioning issues by creating another container to hold the abs. positioned arrows. (Issues primarily presented on Workboards).
Test Plan: Test UI arrows on a workboard, applciation launcher, and in UIExamples.
Reviewers: btrahan, epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Differential Revision: https://secure.phabricator.com/D10897
Summary: Helpful shortcut, I make quite a few at least
Test Plan: Click on dropdown, then Project. Get to Project Create page
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Differential Revision: https://secure.phabricator.com/D10893
Summary: Fixes T6567, lets admins set a default policy for new Projects.
Test Plan: Changed the default Policy, created a new Project. Project had correct defaults.
Reviewers: btrahan, epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T6567
Differential Revision: https://secure.phabricator.com/D10890
Summary: **WIP** This adds default capability to the Files application
Test Plan: Set default to public, go to Files page, see public preset. Upload File. Doesn't work.
Reviewers: btrahan, epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T6564
Differential Revision: https://secure.phabricator.com/D10888
Summary:
Ref T6240. Some discussion in that task. In instance/cluster environments, daemons need to make Conduit calls that bypass policy checks.
We can't just let anyone add SSH keys with this capability to the web directly, because then an adminstrator could just add a key they own and start signing requests with it, bypassing policy checks.
Add a `bin/almanac trust-key --id <x>` workflow for trusting keys. Only trusted keys can sign requests.
Test Plan:
- Generated a user key.
- Generated a device key.
- Trusted a device key.
- Untrusted a device key.
- Hit the various errors on trust/untrust.
- Tried to edit a trusted key.
{F236010}
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T6240
Differential Revision: https://secure.phabricator.com/D10878
Summary: Fixes T4652, adding workboard link to emails
Test Plan: Move a task in a workboard from one column to another. Email notification should contain "WORKBOARD" section with link to that workboard
Reviewers: chad, epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: Korvin, epriestley
Maniphest Tasks: T4652
Differential Revision: https://secure.phabricator.com/D10889
Summary: For actions like "Close" that are in theory stopping the timeline, we should display some disruption to the line itself.
Test Plan:
Tested in UIExamples
{F236077}
Reviewers: btrahan, epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Differential Revision: https://secure.phabricator.com/D10884
Summary: Ref T1217, Add link to email preferences to email template
Test Plan: Add comment to object like Maniphest task, check that email has a footer with a link to email preferences.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: Korvin, epriestley
Maniphest Tasks: T1217
Differential Revision: https://secure.phabricator.com/D10883
Summary: Uses the check icon for closed, which is the primary action taken.
Test Plan:
Close as a duplicate, seen new icon.
{F236048}
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Differential Revision: https://secure.phabricator.com/D10882
Summary: Finishes up work on PHUIInsetForm, cleans up the UI a bit when editing multiple tasks
Test Plan:
Edit multiple tasks, apply some new projects, still works as expected.
{F236044}
Reviewers: btrahan, epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Differential Revision: https://secure.phabricator.com/D10881
Summary: Ref T5833. We try to load bindings even if we get no services back.
Test Plan: Queried invalid results.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T5833
Differential Revision: https://secure.phabricator.com/D10880
Summary: Fixes T3189. Now if you say #projects in a commit message they will associate nicely with the commit. Also we record transactions about all this project editing fun.
Test Plan: tested migration by associating some projects with commits and verifying they still showed up post migration. tested adding / removing projects by doing so from the UI, noting transactions written nicely as well
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Projects: #projects
Maniphest Tasks: T3189
Differential Revision: https://secure.phabricator.com/D10877
Summary:
When a logged-out user views the home page, we currently compute a meaningless "assigned tasks" count which just counts every open task. Don't do this.
Ideally `ManiphestTaskQuery` should get some tightening up too (that is, `withOwners(array(null))` should not select every task), but that might affect other stuff and the performance implications of counting every open task are affecting WMF, so just fix the immediate issue for now.
Test Plan:
- Viewed homepage as a logged out user, no assigned task count.
- Viewed homepage as a logged-in user, still saw assigned task count.
Reviewers: btrahan, chad, chasemp
Reviewed By: chasemp
Subscribers: 20after4, epriestley
Differential Revision: https://secure.phabricator.com/D10876
Summary: Fixes T6152, T6237. This introduces a viewPolicy column to the DifferentialDiff, and re-jiggers the DifferentialDiff policy implementation such that things behave as before once associated with a revision, else use the DifferentialDiff policy column value.
Test Plan: made a diff with a non-standard view policy and noted that policy was still selected in the revision step. arc lint.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T6237, T6152
Differential Revision: https://secure.phabricator.com/D10875
Summary: Fixes T6200. Ref T6237. When creating a diff from the web view, allow the user to select the repository at that time. When viewing a diff that has no associated revision and then creating a revision, pass along the repository phid to the create revision controller. Within the create revision controller, default the repository selector to this repository phid. Finally, in the editor, stop aggressively resetting the repository phid for every TYPE_UPDATE; rather, do so if its not a new object -- the diff should reign supreme in that case -- or if there's no repository -- let the diff be the guide.
Test Plan:
- made a diff with an associated repo, made a revision from the diff, saw the associated repo and it stuck on save!
- made a diff with an associated repo, made a revision from the diff but changed the repo and it stuck on save!
- made a diff with an associated repo, made a revision from the diff but changed the repo to nothing and it stuck on save!
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T6237, T6200
Differential Revision: https://secure.phabricator.com/D10872
Summary: Ref T6237. This sets us up for some future work like T6152, T6200 and generally cleaning up this workflow a bit. Tried to do as little as possible so not exposing transaction view yet. (Though that timeline is going to be a little funky in the common case of just the lone create transaction.)
Test Plan: made a diff from web ui and it worked. made a herald rule to block certain diffs then tried to make such a diff and saw UI letting me know i was blocked
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T6237
Differential Revision: https://secure.phabricator.com/D10869
Summary: Ref T6343, adding HTMLMailMode to remarkup, and most objects should now be processed and appear pretty in emails.
Test Plan: Add a comment to a Maniphest task containing a mention of an object like '{T1}' or 'T1'. Emails should show a styled version of the object similar to how the object looks in the context of the Maniphest task in the UI.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: chad, Korvin, epriestley
Maniphest Tasks: T6343, T2617
Differential Revision: https://secure.phabricator.com/D10859
Summary: The shield is just confusing. In one case it doesn't work, and in the other case it just shows you a copy of the file you can see just below except in red. Fixes T4599, T1211. Note T1211 proposed not showing the "move away" file **at all** but I think removing the shield fixes the source of confusion. The code here is a bit if / else if / else if... heavy but this is logically sound.
Test Plan: made a diff where i moved a file then edited it in the new location. viewed diff, saw confusing shield, dropped caches, applied patch, viewed diff and saw no shield. made a diff where I moved a file and didn't edit in new location and saw similar shield disappearness.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T1211, T4599
Differential Revision: https://secure.phabricator.com/D10865
Summary: Ref T6576. This avoids generating almost-empty HTML mail bodies for mail which incorrectly has no HTML body.
Test Plan: Generated some mail locally; the specific hook case is a pain for me to hit right now. Will push and dig in if that doesn't fix it.
Reviewers: btrahan, chad
Reviewed By: chad
Subscribers: epriestley
Maniphest Tasks: T6576
Differential Revision: https://secure.phabricator.com/D10863
Summary:
Ref T4209. Depends on D10402.
This updates Conduit to support authenticating calls from other servers by signing the request parameters with the sending server's private key and verifying it with the public key stored in the database.
Test Plan:
- Made like 500 bad calls using the stuff in D10402.
- Made a few valid calls using the stuff in D10402.
Reviewers: hach-que, btrahan, #blessed_reviewers
Reviewed By: btrahan, #blessed_reviewers
Subscribers: epriestley, Korvin
Maniphest Tasks: T6240, T4209
Differential Revision: https://secure.phabricator.com/D10401
Summary:
Ref T5833. Just building one query for now which returns the whole binding + interface + network + device tree. Maybe this will get split up in the future.
This will allow web hosts to call the central Almanac and pull instance configuration, authenticating with SSH.
Test Plan: {F234443}
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: chad, epriestley
Maniphest Tasks: T5833
Differential Revision: https://secure.phabricator.com/D10862
Summary: Fixes T6525, adds cc and tos to html emails
Test Plan: send html and plain emails, see new stuff
Reviewers: btrahan, epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T6525
Differential Revision: https://secure.phabricator.com/D10857
Summary: Fixes T6243.
Test Plan: simulated having this on by adding 'ONLY_FULL_GROUP_BY' to the $modes array. verified the error message looked nice {F233954}.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T6243
Differential Revision: https://secure.phabricator.com/D10856
Summary: Fixes T4946. Theoretically.
Test Plan:
iiam
also unit tests.
also
```
cd /var/repo/X
git remote remove origin # simulates origin-missing clone under 1.7.1
cd /path/to/phabricator
./bin/repository pull X
```
and observed no errors
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T4946, T5938
Differential Revision: https://secure.phabricator.com/D10855
Summary: This upgrades 1up view from "does not work" back to "barely works".
Test Plan: view diff, 1up and 2up.
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: epriestley
Differential Revision: https://secure.phabricator.com/D10854
Summary: Fixes T3046. Git has no minimum or bad versions; svn has no minimum and the bad version of 1.7.1, and mercurial has a minimum of 1.9 and bad versions 2.2 and 2.1. These error messages are specific about the problem but do not include the details as to what specifically is bad about a given version. I don't think that's really necessary - the details don't help solve the problem and its probably booooooring at best to the average user. The details about a bad version are included in the code however.
Test Plan: hardcoded a VCS array to let me test all the VCS stuff. added some phlog() calls to make sure the VCS version parsing stuff was working correctly relative to the version(s) on my system. played around with setting minimum versions and bad versions and saw good results depending on what I set.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T3046
Differential Revision: https://secure.phabricator.com/D10852
Summary:
Ref T6238. I'm building the instance management application now, but not putting it in the upstream -- I think the only use case for it is to build SAAS. If someone comes up with a use case (maybe a college course that wants to create an instance per-class or something?) we could open it up eventually, but it seems cleaner to keep it out of the upstream until we have such a use case.
I need to add schema patches. Make it easier for a subclass to just "add all the patches in this directory", like "autopatches/" works.
Test Plan:
- Ran `bin/storage status`, saw all normal patches still valid.
- In some future diff, the instances application will use this to apply patches.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T6238
Differential Revision: https://secure.phabricator.com/D10848
Summary: Fixes T6472. I was curious to see how many reports we'd get but we didn't get any I know of... Still, this is a nice convenient link.
Test Plan:
edited a wiki doc title + body. ran /bin/mail to see the body and liked what I saw
```TEXT BODY
btrahan changed the title from "new title is a good thing" to "new title is a good thing asdsadsadsadsada".
btrahan edited the document content.
DOCUMENT DIFF
http://phalanx.dev/phriction/diff/36/?l=3&r=4
DOCUMENT DETAIL
http://phalanx.dev/w/```
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T6472
Differential Revision: https://secure.phabricator.com/D10850
Summary: Fixes T1768. This is mostly a data cleanliness issue as duplicate rows don't really do anything, but let's clear it up now.
Test Plan: made some duplicate rows by adding the same auditor multiple times. ran ./bin/storage upgrade and it worked perfectly!
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T1768
Differential Revision: https://secure.phabricator.com/D10849
Summary: ...way way down in PhabricatorController. Use it on ManiphestTaskDetailController to test it. Ref T4712. I think the pager logic to be added as part of T4712 can safely reside entirely within this method. As I said earlier, 5 parameters is a lot, so I don't really want to add more. Next diff would do the pagination logic and the diff after that would deploy it everywhere. If while deploying it everywhere I find something off, that will be a different diff.
Test Plan: viewed maniphest tasks and they looked as spiffy as ever.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T4712
Differential Revision: https://secure.phabricator.com/D10844
Summary: Fixes T5015, Allow Herald rules for Maniphest to act on task status changes.
Test Plan: Create Herald rule for Maniphest tasks to flag a task with status "wontfix". Change status of Maniphest task to "wontfix". Task should be flagged.
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T5015
Differential Revision: https://secure.phabricator.com/D10842
Summary: Fixes T6175. This particular option is only editable via the Applications application so let the user know that.
Test Plan: saw error message and clicked through to applications app. tried another locked option and saw old message
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T6175
Differential Revision: https://secure.phabricator.com/D10841
Summary: ...if pertinent environment variables are set that is... Fixes T4151. This is the last piece in making repository creation somewhat easier.
Test Plan: made a new repo and noted that http serving was on r/w and ssh serving was still off, as expected for my environment configuration
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T4151
Differential Revision: https://secure.phabricator.com/D10839
Summary:
Fixes T5492. I figured this would be easier to just fix than write a guide for; it actually took me an hour, but I spent like 75% of that futzing with my editor.
- The Move controller currently accepts either a slug or an ID. I can't find any callsites which pass a slug, and this doesn't make sense. Pretty sure this was copy/pasted from Edit or something. Only accept IDs.
- Slightly modernize the Move controller (newDialog(), handleRequest(), $viewer).
- When the user enters a bad slug, warn them that we're going to fix it for them and let them accept or reject the changes.
- Don't prefill the edit note (this feels inconsistent/unusual).
- On the form, label the input "Path" instead of "URI".
- Show the old path, to help remind the user what the input should look like.
- When a user tries to do a no-op move, show a more tailored message.
- When the user tries to do an overwriting move, explain how they can fix it.
- When normalizing a slug like `/question/???/mark/`, make it normalize to `/question/_/mark`.
Test Plan:
- Tried to move a document to itself.
- Tried to overwrite a document.
- Did a bad-path move, accepted corrected path.
- Did a good-path move.
- Did a path move with a weird component like `/???/`.
- Added and executed unit tests.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T5492
Differential Revision: https://secure.phabricator.com/D10838
Summary:
- Order checks used `=` but intended `==`. We could probably write a lint rule for this.
- Selecting `*` with a join could pick (for example) `id` columns from both the document and content tables and end up using the wrong one.
- `%Q` expects a string and chokes on `null`.
Auditors: btrahan
Summary: Fixes T4666, add Herald rules to Phriction Documents
Test Plan: add Herald rule to flag if title contains "xyz", create Phriction Document with title "xyz". Phriction Document should be flagged.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: Korvin, epriestley
Maniphest Tasks: T4666
Differential Revision: https://secure.phabricator.com/D10830
Summary: When merging tasks, the corresponding transaction on the merged task should be black, and the transaction on the ultimate task should be green.
Test Plan: Create two tasks, merge one into the other, merged task transaction is black, the surviving task should show a green transaction.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: chad, Korvin, epriestley
Maniphest Tasks: T6500
Differential Revision: https://secure.phabricator.com/D10827
Summary: when creating new documents the policy object wasn't being initialized properly. update the code to use the new handy initializeNewDocument method. Fixes T6527.
Test Plan: viewed a doc at /w/asdsadsadsdas/ and saw the correct policy setting
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T6527
Differential Revision: https://secure.phabricator.com/D10837
Summary: i think way back in D10490 I didn't incorporate feedback correctly. make this code right as it fatals in this codepath as is. Fixes T6508.
Test Plan: @joshuaspence to the rescue (I remain unable to test this effectively with my baby-clean installation.)
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley, joshuaspence
Maniphest Tasks: T6508
Differential Revision: https://secure.phabricator.com/D10833
Summary: Fixes T6495. convert ad hoc query to a PhrictionDocumentQuery, thus enforcing view permissions
Test Plan: noted my test user a had a great wiki while test user b couldn't see most things.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T6495
Differential Revision: https://secure.phabricator.com/D10822
Summary:
Ref T5833. I want to add SSH keys to Almanac devices, but the edit workflows for them are currently bound tightly to users.
Instead, decouple key management from users and the settings panel.
Test Plan:
- Uploaded, generated, edited and deleted SSH keys.
- Hit missing name, missing key, bad key format, duplicate key errors.
- Edited/generated/deleted/etc keys for a bot user as an administrator.
- Got HiSec'd on everything.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T5833
Differential Revision: https://secure.phabricator.com/D10824
Summary:
- The icon CSS tag is transformed through the new function
PhabricatorProjectIcon::getAPIName($key), which returns
a name without fa-.
- Color is a trivial lookup
- Profile image returns the PHID or null if not available
Test Plan:
- Create two projects, with different icon and color,
one with and one without profile image.
- Request information on both using project.query
Then:
[ ] Confirm icon and colors are correct for both projects
[ ] Confirm image PHID is correct
[ ] Confirm image PHID is null for the project without image
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: yuvipanda, Korvin, legoktm, epriestley
Maniphest Tasks: T6501
Differential Revision: https://secure.phabricator.com/D10823
Summary: ...how do you lock down entire areas otherwise? Fixes T6496.
Test Plan: used user 1 to create x/y that user 2 can't edit. tried to create x/y/z as user 2 and got a big ole error dialogue.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T6496
Differential Revision: https://secure.phabricator.com/D10819
Summary: 'cuz the wiki don't play that no more. Fixes T6497. This is mainly important to fix an incorrect policy filtering issue where a project policy can incorrectly override a document policy. Otherwise, it makes things nice and clean.
Test Plan:
- viewed the wiki - success.
- viewed wiki document list under "index" and tried a few different queries
- grep'd for things like "hasProject" and "getProject" and saw no phriction-related results
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T6497
Differential Revision: https://secure.phabricator.com/D10818
Summary: Allow `./bin/diviner generate` to continue even if there is an exception throw processing an atom. This allows Diviner documentation to be generated for PHP source code that cannot be parsed with XHPAST.
Test Plan: Ran `./bin/diviner generate` on a PHP repository which previously throw an `XHPASTSyntaxErrorException`.
Reviewers: btrahan, epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: Korvin, epriestley
Differential Revision: https://secure.phabricator.com/D10803
Summary: Fixes T5387. I broke this in D8805. Fix it by using the application search parameters that D8805 introduced.
Test Plan: verified that the two links mentioned in T5387 worked for me. Also tried manual links on secure.phabricator.com and those showed the right data even...!
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T5387
Differential Revision: https://secure.phabricator.com/D10817
Summary:
Ref T4029. Fixes T6034.
Various front-end miscellania here. See D10814#96251. This more or less makes policy work but I am not going to call it "fixed" here since we need D10814 to be deployed too and will do that manually.
Test Plan:
- changed document policy from web ui and changes persisted
- changed document policy from web and had form error and changes persisted
- created a structure like users/users/justmyuserpolicy and made sure another user could delete the users/users/ doc
- moved a doc from a to b and verified policy persisted
- verified stub documents inherited policy of the document that stub them...!
- uploaded a file and verified that it 1) had the permissions of the page it was added to and 2) had an "attached" tab linking back to the page on the file page (this means T6034 is fixed with this)
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T6034, T4029
Differential Revision: https://secure.phabricator.com/D10816
Summary: Ref T4029. this diff makes the pertinent database changes AND adds the migration script. This is important to get the data backend straightened away before we fully ship T4029. Next diff will expose the edit controls for these policies and whatever else work is needed to get that part done right.
Test Plan: made sure the lone project page on my wiki had a project with restrictive view policy. Post migration verified correct policy applied to this lone project page AND most open policy applied to the others
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T4029
Differential Revision: https://secure.phabricator.com/D10814
Summary:
Ref T5833. This fixes a few weird things with this table:
- A bunch of columns were nullable for no reason.
- We stored an MD5 hash of the key (unusual) but never used it and callers were responsible for manually populating it.
- We didn't perform known-key-text lookups by using an index.
Test Plan:
- Ran migrations.
- Faked duplicate keys, saw them clean up correctly.
- Added new keys.
- Generated new keys.
- Used `bin/auth-ssh` and `bin/auth-ssh-key`.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T5833
Differential Revision: https://secure.phabricator.com/D10805
Summary: Missed this in previous pass. Send these as links in HTML emails.
Test Plan: Register a new user that nees approval.
Reviewers: btrahan, epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Differential Revision: https://secure.phabricator.com/D10815
Summary: Fixes T6262. Ref T4029. Also gets us ready for T5873 for these end points. I can file something new about someday adding phriction.query, etc but I think we'll remember and can look at that post T5873.
Test Plan: made a document via conduit and it worked!
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T6262, T4029
Differential Revision: https://secure.phabricator.com/D10813
Summary: Ref T4029. Even more code consolidation and cleanup for the long term benefits!
Test Plan: moved a page successfully. tried to move a page to an existing page and got an error. did the two tab trick to try to move a deleted page and got an error.
Reviewers: chad, epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T4029
Differential Revision: https://secure.phabricator.com/D10812
Summary: Ref T4029. More cleanup and code consolidation for the long terms benefits.
Test Plan: found a document and opened up two browser tabs. Loaded delete dialog on both. Completed delete in one tab and noted document was properly deleted. Tried to complete delete in tab 2 and got an error message.
Reviewers: chad, epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T4029
Differential Revision: https://secure.phabricator.com/D10809
Summary: Ref T1191. Same deal as D10786. These were previously case-insensitive, but changed to a case-sensitive column type.
Test Plan: Ran `bin/storage adjust` and got and adjustment.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: webframp, epriestley
Maniphest Tasks: T1191
Differential Revision: https://secure.phabricator.com/D10806
Summary: Fixes T6489. In an earlier diff I forgot to update the document status itself, thinking that the content update percolated up magically. Restore this functionality so the wiki works better.
Test Plan: deleted a document and observed that i did not get the option to delete it again and it disappeared from document index
Reviewers: epriestley, chad
Reviewed By: chad
Subscribers: Korvin, epriestley
Maniphest Tasks: T6489
Differential Revision: https://secure.phabricator.com/D10807
Summary:
Ref T4029. Some business logic lives outside the editor. This revision moves that logic from the edit controller into the editor proper. This makes re-using that business logic across other endpoints - say like a conduit end point - possible. This is also part of the general modernization quest for phriction I am on.
This diff also restores the functionality where you can delete a document by wiping out the content and saving.
Test Plan: tried to make a document with no title or content and saw errors. opened a document for edit with user 1, then made edits with user 2, then saw an error when i made the edit with user 1. clicking "overwrite changes" then worked. deleted a document by wiping out the body and clicking save.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T4029
Differential Revision: https://secure.phabricator.com/D10795
Summary:
Ref T4029. Long live PhrictionTransactionEditor...! this means that all existing functionality runs 100% through the modern transactions + editor framework. this diff does a few things in sum
- kills the old editor
- moves conduit-based edits to new editor
- moves stubbing out documents to new editor
- deletes moving of wiki docs for projects functionality... (T4021#59511 is a better bigger battle plan here.)
Test Plan: edited a phriction document via conduit and it worked. created a new phriction document /that/was/deep/ and verified ancestral documents were properly stubbed out. changed a project name and noted no wiki page moves.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T4029
Differential Revision: https://secure.phabricator.com/D10792
Summary: Ref T5833. Since these will no longer be bound specifically to users, bring them to a more central location.
Test Plan:
- Edited SSH keys.
- Ran `bin/ssh-auth` and `bin/ssh-auth-key`.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T5833
Differential Revision: https://secure.phabricator.com/D10791
Summary:
Ref T5833. Currently, SSH keys are associated only with users, and are a bit un-modern. I want to let Almanac Devices have SSH keys so devices in a cluster can identify to one another.
For example, with hosted installs, initialization will go something like this:
- A request comes in for `company.phacility.com`.
- A SiteSource (from D10787) makes a Conduit call to Almanac on the master install to check if `company` is a valid install and pull config if it is.
- This call can be signed with an SSH key which identifies a trusted Almanac Device.
In the cluster case, a web host can make an authenticated call to a repository host with similar key signing.
To move toward this, put a proper Query class on top of SSH key access (this diff). In following diffs, I'll:
- Rename `userPHID` to `objectPHID`.
- Move this to the `auth` database.
- Provide UI for device/key association.
An alternative approach would be to build some kind of special token layer in Conduit, but I think that would be a lot harder to manage in the hosting case. This gives us a more direct attack on trusting requests from machines and recognizing machines as first (well, sort of second-class) actors without needing things like fake user accounts.
Test Plan:
- Added and removed SSH keys.
- Added and removed SSH keys from a bot account.
- Tried to edit an unonwned SSH key (denied).
- Ran `bin/ssh-auth`, got sensible output.
- Ran `bin/ssh-auth-key`, got sensible output.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T5833
Differential Revision: https://secure.phabricator.com/D10790
Summary: Ref T4214, this breaks the 'eye' out as a separate image 40px x 40px. We also now show the eye on mobile, as we have enough room for both currently.
Test Plan: Tested default and nightmaremoon colors, tested mobile, tablet and desktop layouts.
Reviewers: btrahan, epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T4214
Differential Revision: https://secure.phabricator.com/D10794
Summary: Fixes T6480. @epriestley is very popular and has over 100 messages, thus triggering this issue. fix the typo because bugs are bad.
Test Plan: set limit to 1, observed fatal, applied patch and fatal went away
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T6480
Differential Revision: https://secure.phabricator.com/D10793
Summary:
Ref T5833. Allow services and devices to be tagged with projects.
(These fluff apply implementations are a good example of the issue discussed in T6403.)
Test Plan: {F229569}
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T5833
Differential Revision: https://secure.phabricator.com/D10782
Summary:
Ref T5833. Adds support for arbitrary properites to Almanac devices and bindings.
- For Devices, this allows you to maybe mark what `rack` a server is on, the `serial` number of a router, etc.
- For Bindings, this allows you to maybe mark that a bound device is `active`, provide `credentials`, expose it as `readonly`, etc.
Test Plan: Added properties to Devices and Bindings.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T5833
Differential Revision: https://secure.phabricator.com/D10781
Summary:
Ref T5833. Currently, we have an `AlmanacDeviceProperty`, but it doesn't use CustomFields and is specific to devices. Make this more generic:
- Reuse most of the CustomField infrastructure (so we can eventually get easy support for nice editor UIs, etc).
- Make properties more generic so Services, Bindings and Devices can all have them.
The major difference between this implementation and existing CustomField implementations is that all other implementations are application-authoritative: the application code determines what the available list of fields is.
I want Almanac to be a bit more freeform (basically: you can write whatever properties you want, and we'll put nice UIs on them if we have a nice UI available). For example, we might have some sort of "ServiceTemplate" that says "a database binding should usually have the fields 'writable', 'active', 'credential'", which would do things like offer these as options and put a nice UI on them, but you should also be able to write whatever other properties you want and add services without building a specific service template for them.
This involves a little bit of rule bending, but ends up pretty clean. We can adjust CustomField to accommodate this a bit more gracefully later on if it makes sense.
Test Plan: {F229172}
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T5833
Differential Revision: https://secure.phabricator.com/D10777
Summary:
Ref T4029. Much like D10756, D10761 this does the bare minimum to get things in there. I have a sticky with "TODOs" about moving the error-checking business logic into the editor in all three cases.
Up next - policy...
Test Plan: moved a document and it worked! verified no feed story. verified both documents involved looked good
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: epriestley, Korvin
Maniphest Tasks: T4029
Differential Revision: https://secure.phabricator.com/D10763
Summary:
Ref T4029. Much like D10756 this does the bare minimum to get things in there. I have a sticky with "TODOs" about moving the error-checking business logic into the editor in both cases.
Up next - move actions...
Test Plan: deleted a document and it worked! verified proper feed story.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: shadowhand, Korvin, epriestley
Maniphest Tasks: T4029
Differential Revision: https://secure.phabricator.com/D10761
Summary: Ref T1191. After adjustment, usernames currently end up case-sensitive, which means `alincoln` and `Alincoln` are different users. Make them case-sensitive so these names collie.
Test Plan: Ran `bin/storage adjust`.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T1191
Differential Revision: https://secure.phabricator.com/D10786
Summary: Fixes T6469. Changes the default icon into text instead. Added the text to hidden boards and now display when reordering as well.
Test Plan:
Moved a bunch of columns, tested reordering. Seems more clear.
{F229626}
{F229627}
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T6469
Differential Revision: https://secure.phabricator.com/D10784
Summary:
Ref T1191. Currently if a developer forgot to specify a column type, `storage adjust` aborts explosively mid-stream. Instead:
- Make this a formal error with an unambiugous name/description instead of something you sort of infer by seeing "<unknown>".
- Make this error prevent generation of adjustment warnings, so we don't try to `ALTER TABLE t CHANGE COLUMN c <unknown>`, which is nonsense.
- When schemata errors exist, surface them prominiently in `storage adjust`.
Overall:
- Once `storage upgrade` runs `storage adjust` automatically (soon), this will make it relatively difficult to miss these errors.
- Letting these errors slip through no longer escalates into a more severe issue.
Test Plan:
Commented out the recent `mailKey` spec and ran `storage adjust`:
```
$ ./bin/storage adjust --force
Verifying database schemata...
Found no adjustments for schemata.
Target Error
phabricator2_phriction.phriction_document.mailKey Column Has No Specification
SCHEMATA ERRORS
The schemata have serious errors (detailed above) which the adjustment
workflow can not fix.
If you are not developing Phabricator itself, report this issue to the
upstream.
If you are developing Phabricator, these errors usually indicate that your
schema specifications do not agree with the schemata your code actually
builds.
```
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T1191
Differential Revision: https://secure.phabricator.com/D10771
Summary: Fixes T3942, turns the load links into buttons.
Test Plan: Set my limit to 1, test page.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T3942
Differential Revision: https://secure.phabricator.com/D10775
Summary:
Fixes an issue with T5336 / D9871. We did 99% of the work here but didn't actually turn on the priority sorting. The unit test passed by default, which didn't catch this.
- Fix the unit test (it failed).
- Fix the query (test now passes).
- Add a "Next in Queue" element to the UI to make this kind of thing easier to spot/understand.
Test Plan: Ran unit test. Viewed "Next in Queue". Queued some tasks, flushed the queue. Web UI tracked the state sensibly.
Reviewers: joshuaspence, btrahan
Reviewed By: btrahan
Subscribers: cburroughs, epriestley
Differential Revision: https://secure.phabricator.com/D10766
Summary: Fixes T6436. We subclass the wrong controller and miss the admin-only check.
Test Plan: Ignored / unignored set up issues.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T6436
Differential Revision: https://secure.phabricator.com/D10765
Summary: Fixes T6343. Grepped for all callsites and added addLinkSection where needed.
Test Plan: Tested Differential, Maniphest, Conpherence, Ponder and Macro. Inspect HTML mail for anchor tags. Inspect text mails for non-disruption.
Reviewers: epriestley, btrahan
Reviewed By: btrahan
Subscribers: talshiri, Korvin, epriestley
Maniphest Tasks: T6343
Differential Revision: https://secure.phabricator.com/D10762
Summary:
This implements as little as possible to stick a working transactions + editor codepath in the basic create / edit flow. Aside from the transaction tables, this also required adding a mailKey to a phrictionDocument.
Future work would include adding more transactions types for things like "move" and all the pertinent support. Even future work is to add things like policies which will work easily in the transaction framework. Ref T4029.
Test Plan:
- made a wiki doc
- edit a wiki doc
- had someone subscribe to a wiki doc and edited it
For all three, the edits worked, a reasonable email was sent out, and feed stories were generated.
- made a wiki doc at a /location/like/this
document "stubs" were made as expected in /location and /location/like
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: chad, Korvin, epriestley
Maniphest Tasks: T4029
Differential Revision: https://secure.phabricator.com/D10756
Summary: Fixes T6427.
Test Plan: Log out of sandbox, navigate to public task, click 'See Details' in a transaction. Get Dialog.
Reviewers: btrahan, epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T6427
Differential Revision: https://secure.phabricator.com/D10759
Summary:
Ref T1191. Notable stuff:
- Adds `--disable-utf8mb4` to `bin/storage` to make it easier to test what things will (approximately) do on old MySQL. This isn't 100% perfect but should catch all the major stuff. It basically makes us pretend the server is an old server.
- Require utf8mb4 to dump a quickstart.
- Fix some issues with quickstart generation, notably special casing the FULLTEXT handling.
- Add an `--unsafe` flag to `bin/storage adjust` to let it truncate data to fix schemata.
- Fix some old patches which don't work if the default table charset is utf8mb4.
Test Plan:
- Dumped a quickstart.
- Loaded the quickstart with utf8mb4.
- Loaded the quickstart with `--disable-utf8mb4` (verified that we get binary columns, etc).
- Adjusted schema with `--disable-utf8mb4` (got a long adjustment with binary columns, some truncation stuff with weird edge case test data).
- Adjusted schema with `--disable-utf8mb4 --unsafe` (got truncations and clean adjust).
- Adjusted schema back without `--disable-utf8mb4` (got a long adjustment with utf8mb4 columns, some invalid data on truncated utf8).
- Adjusted schema without `--disable-utf8mb4`, but with `--unsafe` (got truncations on the invalid data).
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T1191
Differential Revision: https://secure.phabricator.com/D10757
Summary: Fixes T6419. Also, there was a question on T6419 about whether this was in a try catch block and it is... Its not clear to me what happens in the "timeout" case though?
Test Plan: looks nice
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T6419
Differential Revision: https://secure.phabricator.com/D10755
Summary:
Fixes T6416. The comment is consistent with intent, but the actual regexp doesn't quite work right. In particular, we incorrectly match `#security.` as `security.` (with a period) instead of `security` (with no period).
Since this stuff is a pain to test and I evidently got it wrong in this case in D8703, make it unit testable.
Test Plan:
Added unit tests. Also:
{F227181}
Reviewers: chad, btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T6416
Differential Revision: https://secure.phabricator.com/D10753
Summary: Ref T5833. Allows you to bind a service (like `db.example.com`) to one or more interfaces (for example, to specify a pool with one read/write host and two read-only hosts). You can't configure which hosts have which properties yet, but you can add all the relevant interfaces to the service. Next diff will start supporting service, binding, and device properties like "is writable", "is active", etc., so that Almanac will be able to express operations like "change which database is writable", "disable writes", "bring a device down", etc.
Test Plan: See screenshots.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T5833
Differential Revision: https://secure.phabricator.com/D10745
Summary: Fixes T6399. This allows you to use global search to find projects by searching for text in their descriptions.
Test Plan: Added a unique word to a project description, reindexed it, searched, got a hit.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T6399
Differential Revision: https://secure.phabricator.com/D10748
Summary: Fixes T6394. See that task for a description. See T6403 for a proposed long-term fix.
Test Plan: Mentioned a task from a dashboard panel.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T6394
Differential Revision: https://secure.phabricator.com/D10750
Summary: Fixes T6372. Apparently ye olde error logs get some crazy spam action as is... Looking around at call sites, we do not specify $config (which could specify the supportage of message id header) so it seems correct to default this to something. I went with "true" as the spot we use this seems like pretty easy stuff that will always work.
Test Plan: lots of thinking
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T6372
Differential Revision: https://secure.phabricator.com/D10749
Summary: Fixes T6395. Ref T6350. I guess I missed this code spot in prior testing / I definitely didn't run an empty commit through it. Works now though.
Test Plan: made an empty commit and observed stuck importing status and errors in phd log. applied patch and commit successfully imported with no errors. made another empty commit and it imported as well
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T6350, T6395
Differential Revision: https://secure.phabricator.com/D10746
Summary: Fixes T6386. I missed this callsite in D10698.
Test Plan: Loaded local domained blog, no fatal.
Reviewers: btrahan, chad
Reviewed By: chad
Subscribers: epriestley
Maniphest Tasks: T6386
Differential Revision: https://secure.phabricator.com/D10744
Summary: type "string" should be type "text". Ref T6366.
Test Plan: viewed config and saw appropriate, working example
Reviewers: epriestley, chad
Reviewed By: chad
Subscribers: Korvin, epriestley
Maniphest Tasks: T6366
Differential Revision: https://secure.phabricator.com/D10736
Summary: ...also truncate authorName to 255 so that we don't get database errors. Ref T6350.
Test Plan: see T6350 - mostly doing it live - but I did sanity check and commit something and it worked!
Reviewers: epriestley, chad
Reviewed By: chad
Subscribers: Korvin, epriestley
Maniphest Tasks: T6350
Differential Revision: https://secure.phabricator.com/D10734
Summary: Ref T6350. We build $sql based on whose subscribed. If no one is subscribed, then we have no query to run.
Test Plan: observed one error disappearing from my daemon log. Also, more doing it live.
Reviewers: epriestley, chad
Reviewed By: chad
Subscribers: Korvin, epriestley
Maniphest Tasks: T6350
Differential Revision: https://secure.phabricator.com/D10731
Summary: as opposed to "requested". Also re-jigger how the "reason" works so the herald editor can get more specific data rather than a generic message. Fixes T6345 along with companion diff D10726.
Test Plan: made a herald rule to add auditors to a commit and saw it work!
Reviewers: epriestley, chad
Reviewed By: chad
Subscribers: Korvin, epriestley
Maniphest Tasks: T6345
Differential Revision: https://secure.phabricator.com/D10730
Summary: Ref T6345, This adds more consistent color choices to match how Phabricator generally works across Differential/Diffusion per user statuses.
Test Plan: Review a few Audits in my sandbox.
Reviewers: epriestley, btrahan
Reviewed By: btrahan
Subscribers: Korvin, epriestley
Maniphest Tasks: T6345
Differential Revision: https://secure.phabricator.com/D10726
Summary: Ref T6350. I just set up a repository to import secure.phabricator.com/P and saw an error in my error logs about this, creating a fatal around when we publish feed stories. this is late enough in the editor code path I could see it firing again and again and again...
Test Plan: the first phabricator commit ever only had one duplicated transaction before i applied this patch. its yet to do it again. otherwise, ask some users with the issue to deploy it and see if it fixes things for them too.
Reviewers: epriestley, chad
Reviewed By: chad
Subscribers: Korvin, epriestley
Maniphest Tasks: T6350
Differential Revision: https://secure.phabricator.com/D10729
Summary: Ref T4484
Test Plan: Made a mock. Made a herald rule to subscribe a user if mock had a string in title. Edited mock to have said string. Observed user subscribed correctly.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T4484
Differential Revision: https://secure.phabricator.com/D10725
Summary: Fixes T6261. The performance of asking gravatar for these images is horrible and causing lots of people to have issues with the page.
Test Plan: noted how wildly fast the edit profile picture page loaded
Reviewers: chad, epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T6261
Differential Revision: https://secure.phabricator.com/D10724
Summary: Fixes T6336. Turns out that the function to update the import status updates that database and doesn't update the object. If the object doesn't get the pertinent update AND there's a herald rule that runs, then the object is later re-saved without ever getting the update flag.
Test Plan: logic in the ole sandbox and going to push it to prod and run re-parse on impacted commits
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley, chad
Maniphest Tasks: T6336
Differential Revision: https://secure.phabricator.com/D10723
Summary: Ref T5833. An interface is an IP (maybe v4, maybe v6) and port on a specified network (public internet, VPN, NAT block, etc).
Test Plan: See screenshots.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T5833
Differential Revision: https://secure.phabricator.com/D10718
Summary: Ref T5833. This differentiates address spaces like the public internet from VPNs, so when a service is available at `192.168.0.1`, we'll know it's on some specific NAT block or whatever.
Test Plan: See screenshots.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T5833
Differential Revision: https://secure.phabricator.com/D10715
Summary: Ref T5833. The "uninteresting" part of this object is virtually identical to AlmanacService.
Test Plan: See screenshots.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T5833
Differential Revision: https://secure.phabricator.com/D10714
Summary: Ref T5833. See that task for functional goals and some discussion of design.
Test Plan: See screenshots.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T5833
Differential Revision: https://secure.phabricator.com/D10713
Summary:
Ref T5702. This is a forward-looking change which provides some very broad API improvements but does not implement them. In particular:
- Controllers no longer require `$request` to construct. This is mostly for T5702, directly, but simplifies things in general. Instead, we call `setRequest()` before using a controller. Only a small number of sites activate controllers, so this is less code overall, and more consistent with most constructors not having any parameters or effects.
- `$request` now offers `getURIData($key, ...)`. This is an alternate way of accessing `$data` which is currently only available on `willProcessRequest(array $data)`. Almost all controllers which implement this method do so in order to read one or two things out of the URI data. Instead, let them just read this data directly when processing the request.
- Introduce `handleRequest(AphrontRequest $request)` and deprecate (very softly) `processRequest()`. The majority of `processRequest()` calls begin `$request = $this->getRequest()`, which is avoided with the more practical signature.
- Provide `getViewer()` on `$request`, and a convenience `getViewer()` on `$controller`. This fixes `$viewer = $request->getUser();` into `$viewer = $request->getViewer();`, and converts the `$request + $viewer` two-liner into a single `$this->getViewer()`.
Test Plan:
- Browsed around in general.
- Hit special controllers (redirect, 404).
- Hit AuditList controller (uses new style).
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T5702
Differential Revision: https://secure.phabricator.com/D10698
Summary: see title
Test Plan: set config to allow public access and viewed a hovercard uri. saw a hovercard with little info as opposed to login prompt. Fixes T6337.
Reviewers: epriestley, chad
Reviewed By: chad
Subscribers: Korvin, epriestley
Maniphest Tasks: T6337
Differential Revision: https://secure.phabricator.com/D10722
Summary: we don't want to mention these phids... when expanding transactions, build the unmnentionable map and make it so. slightly hairy due to how the editor framework works, but overall i think this is the right place to put these hooks. Fixes T6331.
Test Plan: made a commit with a commit message that had fixes, refs, depends on, and auditors and saw no erroneous mentions
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin, chad, epriestley
Maniphest Tasks: T6331
Differential Revision: https://secure.phabricator.com/D10721
Summary: Fixes T2497. I'm not sure where we are with subscribers and custom vs normal codepath, but the mailtags implementation makes no assumptions and can handle it either way.
Test Plan: made a commit and got some sensible mail tags
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T2497
Differential Revision: https://secure.phabricator.com/D10712
Summary:
Fixes T4896, T6293.
Do most of the work in the editor, but pull the raw patch in the daemon and set that on the editor. This is somewhat of a pre-optimization but it was easy enough to do and makes sense to me.
Test Plan:
made a commit and saw it get parsed.
made a commit with "Auditors: foo" field and saw audit made for foo
turned on inline patch and attach patch and saw the patches
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T6293, T4896
Differential Revision: https://secure.phabricator.com/D10705
Summary: These were missing. Sorry, need to fix this interface someday.
Test Plan: pay for stuff on mobile
Reviewers: btrahan, epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Differential Revision: https://secure.phabricator.com/D10708
Summary: Fixes T6305, sets edit and view uris later in the stack.
Test Plan: Create and edit a project in /project/. Create a project in a dialog. Get redirected to correct place. Verify Cancel send you back to home.
Reviewers: epriestley, btrahan
Reviewed By: btrahan
Subscribers: Korvin, epriestley
Maniphest Tasks: T6305
Differential Revision: https://secure.phabricator.com/D10702
Summary: pre-patch, we match on things like https / http and port... just match domains. Fixes T5693.
Test Plan: arc diff -> arc land and the diff was closed correctly
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T5693
Differential Revision: https://secure.phabricator.com/D10701
Summary:
I am not sure how valuable this is *as is* - I think it needs different explanations for what happened in mercurial or subversion? I do not know what those explanations are.
Made an error in D10485 - the $hashes that were saved is an array of objects, so it ends up turning into garbage via the wonders of serialization and de-serialization. Fix that by explicitly saving the tree hash.
I would like to make this work for the other VCS types we support, add the "undo / nope" button and call it fixed.
Ref T3686.
Test Plan: clicked "explan why" and saw why
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: epriestley, Korvin
Maniphest Tasks: T5693, T3686
Differential Revision: https://secure.phabricator.com/D10489
Summary: missing a setHandles on this codepath I think...? Fixes T6300.
Test Plan: not actually tested - I just think this is the fix since the other renderX methods all do this setHandles thing and I can't figure out how handles get set otherwise...
Reviewers: epriestley, avivey
Reviewed By: epriestley, avivey
Subscribers: avivey, Korvin, epriestley
Maniphest Tasks: T6300
Differential Revision: https://secure.phabricator.com/D10699
Summary: Fixes T6301. Just missed this in building out the page.
Test Plan: Viewed an initiative, verified `pageObjects` populated correctly.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T6301
Differential Revision: https://secure.phabricator.com/D10700
Summary: Default $phids to array() and update it if getValue() has something pertinent... Fixes T6292.
Test Plan: just used the ole logic noodle on this one.
Reviewers: chad, epriestley
Reviewed By: chad, epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T6292
Differential Revision: https://secure.phabricator.com/D10697
Summary: Ref T5702. This primarily gets URI routing out of Aphront and into an Application, for consistency.
Test Plan: Loaded some pages, got static resources.
Reviewers: chad, btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T5702
Differential Revision: https://secure.phabricator.com/D10696
Summary: Ref T5702. Primarily, this gets the custom DarkConsole URI routes out of the Aphront core and into an Application, like almost all other routes.
Test Plan: Used DarkConsole.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T5702
Differential Revision: https://secure.phabricator.com/D10695
Summary: Ref T2787. When order statuses change, send merchants and users email about it.
Test Plan: Used `bin/mail` to review mail.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T2787
Differential Revision: https://secure.phabricator.com/D10694
Summary: Ref T2787. I mostly just want these in place so I can glue emails to them, but they're also useful on their own.
Test Plan: {F216515}
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T2787
Differential Revision: https://secure.phabricator.com/D10688
Summary: Ref T2787. Currently, we show all orders/charges, which won't scale well. Show the 10 most recent and link to full order/charge history.
Test Plan: {F216325}
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T2787
Differential Revision: https://secure.phabricator.com/D10685
Summary: Ref T2787. This stuff is now irrelevant and/or has no callsites.
Test Plan: `grep`, poked around
Reviewers: chad, btrahan
Reviewed By: chad, btrahan
Subscribers: epriestley
Maniphest Tasks: T2787
Differential Revision: https://secure.phabricator.com/D10684
Summary: Ref T5835. Sprinkle `shouldAllowPublic()` around to let logged-out users gain access.
Test Plan: Viewed an initiative while logged out.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T5835
Differential Revision: https://secure.phabricator.com/D10683
Summary: Ref T5835. Dump these into global search so you can find them.
Test Plan: {F216290}
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T5835
Differential Revision: https://secure.phabricator.com/D10682
Summary:
Ref T2787. Make this a little more concrete with explicit membership instead of a general edit policy. In particular, we need to know who to email when orders happen, and can't reasonably do that with an edit policy.
I imagine this might eventually get more nuanced (e.g., users who can only approve orders vs users who can manage the merchant itself) but that's a long ways away.
Test Plan: {F216284}
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T2787
Differential Revision: https://secure.phabricator.com/D10681
Summary:
Fixes T6145, T4016.
Filed T6287 and T6288 for some polish on this.
Test Plan: Made new projects from Maniphest - great success. Made new projects from project / create - also great success.
Reviewers: chad, epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T4016, T6145
Differential Revision: https://secure.phabricator.com/D10679
Summary:
Ref T2787.
- Account members can add and remove other members (major use case is corporate accounts).
- Use a modern edge constant setup.
Test Plan: See screenshots.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T2787
Differential Revision: https://secure.phabricator.com/D10678
Summary: Ref T5835. Make fund stories publish to feed and send email.
Test Plan: Made edits, etc., saw them in feed and outbound email.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T5835
Differential Revision: https://secure.phabricator.com/D10677
Summary: Ref T5835. Show backing amounts in transactions. Account for and show refunds.
Test Plan: {F215869}
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T5835
Differential Revision: https://secure.phabricator.com/D10676
Summary: Ref T2787. Allow merchants to flag orders for review. For now, all orders are flagged for review. Eventually, I could imagine Herald rules for coarse things (e.g., require review of all orders over $1,000, or require review of all orders by users not on a whitelist) and maybe examining fraud data for the providers which support it.
Test Plan: {F215848}
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T2787
Differential Revision: https://secure.phabricator.com/D10675
Summary: Ref T2787. Support multiple payment accounts so you can have personal vs company payment accounts.
Test Plan: See screenshots.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T2787
Differential Revision: https://secure.phabricator.com/D10673
Summary:
Ref T2787. Currently, we dump the user back into the application. Instead, give them a confirmation screen and then let them continue.
Also fix a couple of unit tests I adjusted the underlying behavior of somewhat-recently in libphutil.
Test Plan: {F215498}
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T2787
Differential Revision: https://secure.phabricator.com/D10672
Summary: Ref T2787. These don't necessarily do a ton yet, but we can get PayPal out of hold, at least.
Test Plan: Updated charges from all providers. Cleared a PayPal hold.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T2787
Differential Revision: https://secure.phabricator.com/D10670
Summary:
Ref T2787. When Paypal comes back to us with funds on hold, dead-end the transaction but handle it properly.
Generally, smooth out the user interaction on weird states.
Implement refudnds/cancels for Paypal.
Test Plan: {F215230}
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T2787
Differential Revision: https://secure.phabricator.com/D10667
Summary: We were saying "Object Restricted Object"; instead say "Restricted Object". Fixes T6104.
Test Plan: made a restricted paste and a restricted task and saw good error messages. {F215281} {F215282}
Reviewers: epriestley, chad
Reviewed By: chad
Subscribers: Korvin, epriestley
Maniphest Tasks: T6104
Differential Revision: https://secure.phabricator.com/D10668
Summary:
Ref T2787. Currently, we kill a cart and dead-end the workflow on a charge failure.
Instead, fail the charge and reset the cart so the user can try using a valid payment instrument like a normal checkout workflow would.
Some shakiness/smoothing on WePay for the moment; PayPal is still made up since we don't have a "Hold" state yet.
Test Plan: {F215214}
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T2787
Differential Revision: https://secure.phabricator.com/D10666
Summary: Fixes T4018. Basically hits the bullet points in that task description except the "ideally" one.
Test Plan:
ran bin/config migrate and saw sensible output.
```
~> ./bin/config migrate
Migrating file-based config to more modern config...
Skipping config of source type PhabricatorConfigDatabaseSource...
Skipping config of source type PhabricatorConfigLocalSource...
Skipping config of source type PhabricatorConfigDefaultSource...
Done. Migrated 0 keys.
```
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: hach-que, epriestley, Korvin
Maniphest Tasks: T4018
Differential Revision: https://secure.phabricator.com/D10490
Summary:
Ref T2787. This has some rough edges but basically works.
- Users can cancel orders that are in incomplete states (or in complete states, if the application allows them to -- for example, some future application might allow cancellation of billed-but-not-shipped orders).
- Merchant controllers can partially or fully refund orders from any state after payment.
Test Plan: This is still rough around the edges, but issued Stripe and WePay refunds.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: chad, epriestley
Maniphest Tasks: T2787
Differential Revision: https://secure.phabricator.com/D10664
Summary:
Ref T2787.
- Allow merchants to disable payment providers.
- Show more useful information about providers on the payments page.
- Make test vs live more clear.
- Show merchant status.
- Add a description to merchants to flesh them out a bit -- the merchant areas of responsibilities seem to be fitting well with accounts, etc.
Test Plan: {F215109}
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T2787
Differential Revision: https://secure.phabricator.com/D10662
Summary: Ref T2787. Uses the real icons. Straightens out the add payment flow a tiny bit.
Test Plan: {F214922}
Reviewers: btrahan, chad
Reviewed By: chad
Subscribers: epriestley
Maniphest Tasks: T2787
Differential Revision: https://secure.phabricator.com/D10654
Summary: Fixes T6252
Test Plan: Test project query from conduit app, see no errors in log.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T6252
Differential Revision: https://secure.phabricator.com/D10655
Summary: Ref T6256, this prevents more installs from getting in this weird state. We'll have to follow up if possible to "fix" the issue retroactively.
Test Plan: Test moving a backlog column to new position, hiding rest of other panels.
Reviewers: btrahan, epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T6256
Differential Revision: https://secure.phabricator.com/D10651
Summary:
Ref T2787. Builds on D10649 by rebining existing objects (carts, charges, etc) to merchantPHIDs and providerPHIDs instead of an implicit global merchant and weird global artifacts (providerType / providerKey).
Basically:
- When you create something that users can pay for, you specify a merchant to control where the payment goes.
- Accounts are install-wide, but payment methods are bound to merchants. This seems to do a reasonable job of balancing usability and technical concerns.
- Replace a bunch of weird links between objects with standard PHIDs.
- Improve "add payment method" flow.
Test Plan: Went through the Fund flow with Stripe and WePay, funding an initiative.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T2787
Differential Revision: https://secure.phabricator.com/D10652
Summary:
Ref T2787. Instead of making providers global configuration, make them a thing on merchants with web configuration.
Payment methods and some of the pyament workflow needs to be retooled a bit after this, but this seemed like a reasonable cutoff point for this diff.
Test Plan: See screenshots.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T2787
Differential Revision: https://secure.phabricator.com/D10649
Summary:
Ref T2787. Currently, you add payment providers (Stripe, Paypal, etc) in global configuration.
Generally, this approach is cumbersome, limiting, and often hard for users to figure out. It also doesn't provide a natural way to segment payment receivers or provide web access to administrative payment functions like issuing refunds, canceling orders, etc. I think that stuff definitely needs to be in the web UI, and the rule for access to it can't reasonably just be "all administrators" in a lot of reasonable cases.
The only real advantage is that it prevents an attacker from adjusting settings and pointing something at an account they control. But this attack can be mitigated through notifications, some sort of CLI-only merchant lock, payment accounts being relatively identifiable, etc.
So introduce "merchants", which are basically payable entities. An individual merchant will have attached Paypal, Stripe, etc., accounts, and access rules. When you buy something in an application, the merchant to pay is also specified. They also provide an umbrella for dealing with permissions down the line.
This may get a //little// cumbersome because if there are several merchants your saved card information is not shared across them. I think that will be fine in the normal case (most installs will have only one merchant). Even if it isn't and we leave providers global, I think introducing this is the right call from a web UI / permissions point of view. I'll play around with it in the next couple of diffs and figure out exactly where the line goes.
Test Plan: Listed, created, edited, viewed merchants.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T2787
Differential Revision: https://secure.phabricator.com/D10648
Summary:
Ref T2787. These were still stuck in the stone ages.
(The handles are pretty skeletal but most aren't used anywehre.)
Test Plan: Funded an initiative without anything breaking. Grepped for removed constants.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T2787
Differential Revision: https://secure.phabricator.com/D10647
Summary: Ref T2787. Like Stripe, this one is pretty easy to get working correctly on the "good" path and fataling out in a safe way on bad paths.
Test Plan: Funded an initiative with Balanced.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T2787
Differential Revision: https://secure.phabricator.com/D10645
Summary:
Ref T2787. For test charges, Paypal is putting the charge in a "payment review" state. Dealing with this state requires way more infrastructure than other providers: we're supposed to pause delivery, then poll Paypal every 6 hours to see if the review has resolved.
Since I can't seem to generate normal test charges, I can't test Paypal for now. Disable it until we have more infrastructure.
(This diff gets us further along, up to the point where I hit this issue.)
Test Plan: Read documentation, rolled eyes.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T2787
Differential Revision: https://secure.phabricator.com/D10644
Summary:
Ref T2787. This basically already works correctly since the hard logic is external to the provider on API providers. Tweak a couple of things.
Failures still just fail the cart completely, for now.
Test Plan: Completed a charge with Stripe.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T2787
Differential Revision: https://secure.phabricator.com/D10640
Summary:
Ref T2787. This doesn't get all the edge cases quite correct, but is generally a safe, complete payment workflow:
- Shares the actual charging state logic.
- Makes it appropriately stateful with locking and transactions.
- Gets the main flow correct.
- Detects failure cases, just tends to blow up rather than help the user resolve them.
Test Plan:
- Charged with WePay.
- Charged with Infinite Free Money.
- Resumed an abandoned cart.
- Hit all failure states where we just dead-end the cart. Not ideal, but (seemingly) complete/safe/correct.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T2787
Differential Revision: https://secure.phabricator.com/D10639
Summary: Ref T2787. Similar to D10634, give applications more control over the cart workflow. For now this just means they get to pick exit URIs, but in the future they can manage more details of cart behavior.
Test Plan: Funded an initiative and got returned to the initiative instead of dead-ending in Phortune.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T2787
Differential Revision: https://secure.phabricator.com/D10638
Summary: Fixes T6254 and renames status as string. Though maybe this should go through `formatStringConstants`?
Test Plan: Reload Conduit page, see new text.
Reviewers: btrahan, epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T6254
Differential Revision: https://secure.phabricator.com/D10637
Summary:
Ref T2787. When a user purchases a product in Phortune, transition the cart through a purchased state and invoke product callbacks so applications can respond to the workflow.
Also shore up some stuff like preventing negative amounts of funding.
Test Plan: Backed an initiative and saw it show up on the initiative after completing the purcahsing workflow.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T2787
Differential Revision: https://secure.phabricator.com/D10635
Summary: Ref T2787. `Product` is currently a fairly heavy object, but as Phortune develops it makes a lot of sense to make it a lighter object and put more product logic in applications. Convert it into a fairly lightweight reference to applications. The idea is that Phortune is mostly providing a cart flow, and applications manage the details of products.
Test Plan: Funded an initiative for $1.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T2787
Differential Revision: https://secure.phabricator.com/D10634
Summary:
Ref T2787. Phortune currently stores a bunch of stuff as `...inUSDCents`. This ends up being pretty cumbersome and I worry it will create a huge headache down the road (and possibly not that far off if we do Coinbase/Bitcoin soon). Even now, it's more of a pain than I figured it would be.
Instead:
- Provide an application-level serialization mechanism.
- Provide currency serialization.
- Store currency in an abstract way (currently, as "1.23 USD") that can handle currencies in the future.
- Change all `...inUSDCents` to `..asCurrency`.
- This generally simplifies all the application code.
- Also remove some columns which don't make sense or don't make sense anymore. Notably, `Product` is going to get more abstract and mostly be provided by applications.
Test Plan:
- Created a new product.
- Purchased a product.
- Backed an initiative.
- Ran unit tests.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T2787
Differential Revision: https://secure.phabricator.com/D10633
Summary:
Ref T4209. This creates storage for public keys against authorized hosts, such that servers can be authorized to make Conduit calls as the omnipotent user.
Servers are registered into this system by running the following command once:
```
bin/almanac register
```
NOTE: This doesn't implement authorization between servers, just the storage of public keys.
Placing this against Almanac seemed like the most sensible place, since I'm imagining in future that the `register` command will accept more information (like the hostname of the server so it can be found in the service directory).
Test Plan: Ran `bin/almanac register` and saw the host (and public key information) appear in the database.
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: epriestley, Korvin
Maniphest Tasks: T4209
Differential Revision: https://secure.phabricator.com/D10400
Summary: Fixes T6119. This is a little fuzzy, but generally bumping up `innodb_buffer_pool_size` to something bigger than the default (which is often anemic, at `8M`) is desriable, and it seems like it will fix the specific issue a user encountered in T6119.
Test Plan: {F211855}
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T6119
Differential Revision: https://secure.phabricator.com/D10630
Summary:
Ref T1191. Although I fixed some of the mutations earlier (in D10598), I missed the column mutations under old versions of MySQL. In particular, this isn't valid:
- `ALTER TABLE ... MODIFY columnName VARCHAR(64) COLLATE binary`
Issue the permitted version of this instead, which is:
- `ALTER TABLE ... MODIFY columnName VARBINARY(64)`
Also fixed an issue where a clean schema had the wrong nullability for a column in the draft table. Force it to the expected nullability.
The other trick here is around the one column with a FULLTEXT index on it, which needs a little massaging.
Test Plan:
- Forced my local install to return `false` for utf8mb4 support.
- Did a clean adjust into `binary` columns.
- Poked around, added emoji to things.
- Reverted the fake check and did a clean adjust into `utf8mb4` columns.
- Emoji survived.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: fabe, epriestley
Maniphest Tasks: T1191
Differential Revision: https://secure.phabricator.com/D10627
Summary: thanks mailbox
Test Plan: unit tests
Reviewers: btrahan, chad
Reviewed By: chad
Subscribers: epriestley
Differential Revision: https://secure.phabricator.com/D10629
Summary:
Ref T1191. Now that the whole database is covered, we don't need to do as much work to build expected schemata. Doing them database-by-database was helpful in converting, but is just reudndant work now.
Instead of requiring every application to build its Lisk objects, just build all Lisk objects.
I removed `harbormaster.lisk_counter` because it is unused.
It would be nice to autogenerate edge schemata, too, but that's a little trickier.
Test Plan: Database setup issues are all green.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley, hach-que
Maniphest Tasks: T1191
Differential Revision: https://secure.phabricator.com/D10620
Summary: Ref T1191. The index's case sensitivity depends on the column type. Using `text` makes the search case-sensitive, which is not desirable.
Test Plan: After adjustment, searched for "PROJECTS" and found hits against "projects".
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T1191
Differential Revision: https://secure.phabricator.com/D10619
Summary: Fixes T6211. This gives Herald rules an explicit execution order, which seems generally good. See some discussion on T6211 and inline.
Test Plan:
- Added unit test.
- Dry ran rules and saw rules appear in the expected order in the transcript.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T6211
Differential Revision: https://secure.phabricator.com/D10624
Summary: Fixes T6210. The current messaging may be confusing if `pygmentize` is available but broken.
Test Plan: Faked the binary names and hit the errors, which seemed helpful.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T6210
Differential Revision: https://secure.phabricator.com/D10626
Summary: Ref T6201. This isn't quite perfect but should be good enough. At some point far in the future I plan to revamp feed rendering a bit. This should possibly become a real ApplicationTransaction story eventually, too.
Test Plan: {F211777}
Reviewers: btrahan, chad
Reviewed By: chad
Subscribers: epriestley
Maniphest Tasks: T6201
Differential Revision: https://secure.phabricator.com/D10625
Summary: Ref T1191. Similar issue to D10613. This column usually has a hash exactly 12 bytes long, but sometimes stores an internal builtin query name like "open", "all", etc. It might be nice to promote those to 12-byte hashes of a consistent length eventually, but for now just make this a variable-length column.
Test Plan: Ran migration, no longer saw issues with reordering builtin saved searches.
Reviewers: btrahan
Subscribers: epriestley
Maniphest Tasks: T1191
Differential Revision: https://secure.phabricator.com/D10614
Summary:
Ref T1191. The `bytes` types are BINARY(...), which is fixed-length and zero-pads. These hashes are not 64 characters long, so migrating them to `binary` ends up with a bunch of zero-padding.
Instead, migrate them to `text` so we drop the zero padding. It would be vaguely nice to either introduce a `varbytes` type (ick) or change the hash size to a standard size (nicer) eventually, but this isn't very important.
Test Plan: Will adjust `secure.phabricator.com`.
Reviewers: btrahan
Subscribers: epriestley
Maniphest Tasks: T1191
Differential Revision: https://secure.phabricator.com/D10613
Summary:
Ref T1191. When changing the column type of an AUTO_INCREMENT column, we currently may lose the autoincrement attribute.
Instead, support it. This is a bit messy because AUTO_INCREMENT columns interact with PRIMARY KEY columns (tables may only have one AUTO_INCREMENT column, and it must be a primary key). We need to migrate in more phases to avoid this issue.
Introduce new `auto` and `auto64` types to represent autoincrement IDs.
Test Plan:
- Saw autoincrement show up correctly in web UI.
- Fixed an autoincrement issue on the XHProf storage table with `bin/storage adjust` safely.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T1191
Differential Revision: https://secure.phabricator.com/D10607
Summary:
Ref T1191. Currently, the `quickstart.sql` gets generated in a pretty manual fashion. This is a pain, and will become more of a pain in the world of utf8mb4.
Provide a workflow which does upgrade + adjust + dump + destroy, then massages the output to produce a workable `quickstart.sql`.
Test Plan: Inspected output; I'll test this more throughly before actually generating a new quickstart, but that's some ways away.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T1191
Differential Revision: https://secure.phabricator.com/D10603
Summary:
Ref T1191. For most text columns, we either don't care if "a" and "A" are the same, or we expect them to be different (for example: keys, domains, secrets, etc). Default text columns to the `_bin` collation so they are compared by strict character value. This is safer in cases where we aren't sure.
For some text columns, we allow the user to sort by the column in the UI (like Maniphest task titles) or we do care that "A" and "a" are the same (for example: project names). Introduce a new class of virtual data types, the "sort..." types, to cover these columns. These are like the "text..." types but use sorting collations which treat "A" and "a" the same.
Test Plan:
- Made an effort to identify all columns where the UI relies on database collation.
- Ran `bin/storage adjust` and cleared all warnings.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: beng, epriestley
Maniphest Tasks: T1191
Differential Revision: https://secure.phabricator.com/D10602
Summary:
Ref T1191. This was useful for annotating everything but we no longer need it; there are just two types of issues now:
- Error: stuff we can't fix (missing or surplus tables/database/columns, bad column nullability).
- Warning: stuff we can fix (column types, character sets, collations, missing or surplus keys, incorrectly defined keys, bad key uniqueness).
Test Plan: Saw 3,399 warnings and 0 errors.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T1191
Differential Revision: https://secure.phabricator.com/D10594
Summary:
Ref T1191. Ref T6203. While generating expected schemata, I ran into these columns which seem to have sketchy nullability.
- Mark most of them for later resolution (T6203). They work fine today and don't need to block T1191. Changing them can break the application, so we can't autofix them.
- Forgive a couple of them that are sort-of reasonable or going to get wiped out.
Test Plan: Saw 94 remaining warnings.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: hach-que, epriestley
Maniphest Tasks: T1191, T6203
Differential Revision: https://secure.phabricator.com/D10593
Summary:
Ref T1191. We have several keys on `<x, y, id>`. When `id` is an auto-increment primary key, I believe this is exactly equivalent to a key on `<x, y>`, because the leaf nodes are implicitly sorted by `id`. We omit the implicit `id` elsewhere.
It would be nice to drop the `id` bit for consistency, but it's not doing any harm and this doesn't need to block the primary work of T1191.
Test Plan: Saw slightly fewer warnings.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T1191
Differential Revision: https://secure.phabricator.com/D10592
Summary:
Ref T1191. This destroys surplus columns:
- Pholio's transaction comments have a `mockID` column, but this is not used. The `imageID` column is used instead.
- Phragment has an unused `description` column.
- Releeph has an unused `summary` column.
Test Plan:
- Grepped for usage of these columns.
- Checked that these exist in production, too.
- Ran upgrades.
- Added Pholio inline comments.
- Saw fewer warnings.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley, hach-que
Maniphest Tasks: T1191
Differential Revision: https://secure.phabricator.com/D10591
Summary:
Ref T1191.
- Adds definitions for missing keys and keys with wrong uniqueness. Generally, I defined these before fixing the key query to actually pull all keys and support uniqueness.
- Moves "key uniqueness" to note severity; this is fixable (probably?) and there are no remaining issues.
- Moves "Missing Key" to note severity; missing keys are fixable and all remaining missing keys are really missing (either missing edge keys, or missing PHID keys):
{F210089}
- Moves "Surplus Key" to note seveirty; surplus keys are fixable all remaining surplus keys are really surplus (duplicate key in Harbormaster, key on unused column in Worker):
{F210090}
Test Plan:
- Vetted missing/surplus/unique messages.
- 146 issues remaining.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T1191
Differential Revision: https://secure.phabricator.com/D10590
Summary:
Ref T1191. Notable:
- Drops a very old saved query table. See comments inline: plan was to remove it after a year. It's been ~a year and two weeks.
- This has our only fulltext index. I'm not supporting that formally for now, but left a note.
- This has our only MyISAM table. I'm not supporting that explicitly for now, but it shouldn't affect anything. I may deal with this in the future.
- These tables don't actually write directly via Lisk, so there's some fiddling to get the schemata right.
Test Plan: Down to ~250 warnings. No more surplus databases or tables.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T1191
Differential Revision: https://secure.phabricator.com/D10589
Summary:
Ref T1191. Notable:
- `HeraldApplyTranscript` is not actually a DAO and has no table (it is serialized into HeraldTranscript).
Test Plan: Down to fewer than 300 issues.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T1191
Differential Revision: https://secure.phabricator.com/D10588
Summary:
Ref T1191. Nothing too notable here:
- Allow a Lisk object to specify that there's no expectation that a table exists. We have one Harbormaster object and one Token object like this.
- Removed BuildPlanTransactionComment because it's currently unused.
Test Plan:
- Saw ~200 fewer warnings; just ~800 left.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T1191
Differential Revision: https://secure.phabricator.com/D10583
Summary:
Ref T1191.
- Removes ponder comment table. This was migrated a very long time ago.
Test Plan:
- Grepped for removed table.
- Saw ~100 fewer issues in web UI.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T1191
Differential Revision: https://secure.phabricator.com/D10582
Summary:
Ref T1191. Notes:
- Drops the project affiliation table. This is a very old membership table which was migrated to edges.
- Drops the subproject table. This is a very old table for a removed feature.
Test Plan:
- Grepped for dropped tables.
- Saw ~100 fewer setup issues.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T1191
Differential Revision: https://secure.phabricator.com/D10581
Summary:
Ref T1191. Some notes here:
- Drops the old LDAP and OAuth info tables. These were migrated to the ExternalAccount table a very long time ago.
- Separates surplus/missing keys from other types of surplus/missing things. In the long run, my plan is to have only two notice levels:
- Error: something we can't fix (missing database, table, or column; overlong key).
- Warning: something we can fix (surplus anything, missing key, bad column type, bad key columns, bad uniqueness, bad collation or charset).
- For now, retaining three levels is helpful in generating all the expected scheamta.
Test Plan:
- Saw ~200 issues resolve, leaving ~1,300.
- Grepped for removed tables.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T1191
Differential Revision: https://secure.phabricator.com/D10580
Summary: Fixes T6201. This stuff didn't fully get updated for ApplicationTransactions. Get it working again (notably, make inline comment text publish) and clean it up a little bit.
Test Plan:
- Published a Differential feed story into Asana with comment text.
- Pulbished a Diffusion feed story into Asana with comment text.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T6201
Differential Revision: https://secure.phabricator.com/D10584
Summary: Ref T1191. This actually works without T1191, but makes emoji use on the desktop easier.
Test Plan: {F210416}
Reviewers: chad, btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T1191
Differential Revision: https://secure.phabricator.com/D10605
Summary:
Ref T1191.
- This drops two tables.
- Both tables were migrated to transactions a very long time ago and no longer have readers or writers.
Test Plan: Saw ~150 fewer warnings.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T1191
Differential Revision: https://secure.phabricator.com/D10576
Summary: Fixes T6184. On a Revision page we don't show the date as an important piece of information, so it's also not likely useful on a Hovercard (and confusing as to what the date means).
Test Plan: Hover over a linked Diff
Reviewers: btrahan, epriestley
Reviewed By: epriestley
Subscribers: epriestley, Korvin
Maniphest Tasks: T6184
Differential Revision: https://secure.phabricator.com/D10579
Summary: Fixes T6199, checks if Calendar is installed and displays if so.
Test Plan: Turned Calendar on and off, tested both layouts.
Reviewers: btrahan, epriestley
Reviewed By: epriestley
Subscribers: epriestley, Korvin
Maniphest Tasks: T6199
Differential Revision: https://secure.phabricator.com/D10574
Summary: Fixes T6189. We currently don't raise these to the editor level, so files, mentions, and project stuff get ignored.
Test Plan: Verified that files added to question and answer bodies end up attached to the relevant objects.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T6189
Differential Revision: https://secure.phabricator.com/D10564
Summary: Ref T6185. Although it seems that we can't easily defuse or mitigate this, we can at least warn administrators.
Test Plan: Ran on my (unpatched, local) system, got a setup warning.
Reviewers: btrahan, chad
Reviewed By: chad
Subscribers: epriestley
Maniphest Tasks: T6185
Differential Revision: https://secure.phabricator.com/D10561
Summary: Take my secrets on the road
Test Plan: View Passphrase on mobile device, see action list.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: epriestley, Korvin
Differential Revision: https://secure.phabricator.com/D10562
Summary:
Ref T2015. This increases the Drydock worker lease time to 24 hours. We noticed that some leases took longer than 2 hours when leasing from AWS (the actual resource was successfully leased at around 2 hours, 19 minutes).
24 hours should be plenty enough time to actually lease anything from EC2 (or any other leases during builds).
Test Plan: Have not yet tested this.
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: epriestley, Korvin
Maniphest Tasks: T2015
Differential Revision: https://secure.phabricator.com/D10544
Summary: This fixes a unit test failure that started occurring due to the new membership locking feature.
Test Plan: Ran the unit tests.
Reviewers: btrahan, epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: epriestley, Korvin
Differential Revision: https://secure.phabricator.com/D10546
Summary: Ref T1191. Handful of minor things here (T6150, T6149, T6148, T6147, T6146) but nothing very noteworthy.
Test Plan: Viewed web UI, saw fewer errors.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T1191
Differential Revision: https://secure.phabricator.com/D10527
Summary: Fixes T6177. Now that we've reframed "Beta" into "Prototype", there's no reason this needs to be in a separate super-hidden class of application anymore.
Test Plan: Saw Releeph available as a normal Prototype application.
Reviewers: btrahan, chad
Reviewed By: chad
Subscribers: epriestley
Maniphest Tasks: T6177
Differential Revision: https://secure.phabricator.com/D10550
Summary: Fixes T6176. Language here is a bit awkard but I wanted to use the verb "removed" *and* still have the object first, so I ended up adding the before details parenthetically.
Test Plan: story no longer fatal'd in my feed
Reviewers: epriestley, chad
Reviewed By: chad
Subscribers: epriestley, Korvin
Maniphest Tasks: T6176
Differential Revision: https://secure.phabricator.com/D10549
Summary:
Ref T1191.
- Adds support for custom fields.
- Adds support for partial indexes (indexes on a prefix of a column).
- Drops old auxiliary storage table: this was moved to custom field storage about a year ago.
- Drops old project table: this was moved to edges about two months ago.
Test Plan:
- Viewed web UI, saw fewer issues.
- Used `grep` to verify no readers/writers for storage or project table.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T1191
Differential Revision: https://secure.phabricator.com/D10526
Summary:
Ref T1191. Three parts:
- The old way of getting key information only got primary / unique / foreign keys, not all keys. Use `SHOW INDEXES` to get all keys instead.
- Track key uniqueness and raise warnings about it.
- Add a new "all issues" view to show an expanded, flat view of all issues. This is just an easier way to get a list so you don't have to dig around in the hierarchical view.
Test Plan:
{F206351}
{F206352}
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T1191
Differential Revision: https://secure.phabricator.com/D10525
Summary:
Ref T1191.
- There was a varchar(50) column. I changed it to `text64`, since this length is unusual.
- There was an int(3) column. I changed it to `int32`, since this length is unusual.
Test Plan: Ran migrations, saw warnings disappear from config tool.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T1191
Differential Revision: https://secure.phabricator.com/D10524
Summary: Ref T1191. This was migrated to transactions a very long time ago.
Test Plan: Ran migration, grepped, left comments in Slowvote.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T1191
Differential Revision: https://secure.phabricator.com/D10523
Summary: T1191. Nothing very notable here.
Test Plan: Saw more blue in web UI.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Differential Revision: https://secure.phabricator.com/D10522
Summary: Ref T1191. Nothing too exciting in these.
Test Plan: Saw more blue in UI.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T1191
Differential Revision: https://secure.phabricator.com/D10521
Summary:
Ref T1191. Notable:
- Allowed objects to remove default columns (some feed tables have no `id`).
- Added a "note" severity and moved all the charset stuff down to that to make progress more clear.
Test Plan:
Trying to make the whole thing blue...
{F205970}
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T1191
Differential Revision: https://secure.phabricator.com/D10519
Summary: Ref T1191. Fills in some more of the databases. Nothing very notable here. I didn't encounter any issues or overlong keys.
Test Plan: Used web UI to click around and verify expected schemata match up against actual schemata well.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T1191
Differential Revision: https://secure.phabricator.com/D10516
Summary:
Fixes T5603. Puts the toggling of locking membership into the editor so we get exceptions and all that.
I think the dialogue when you try to leave a project that is locked could be a little better maybe? Right now it just says "You can't leave" and "The membership is locked" more or less; should I surface a link to the policy stuff there too?
Test Plan:
- made a project, toggled the "lock" setting, observed stickiness and good transactions being made
- locked a project and tried to leave as a non-editor - got a dialogue letting me know i couldn't
- locked a project and tried to leave as an editor - left successfully
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: epriestley, Korvin
Maniphest Tasks: T5603
Differential Revision: https://secure.phabricator.com/D10508
Summary: Ref T1191. This fills in some more features and gets audit and auth nearly generating reasonable expected schemata.
Test Plan: See screenshots.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T1191
Differential Revision: https://secure.phabricator.com/D10500
Summary:
Ref T1191. The major issue motivation here is that InnoDB keys have a maximum length of 767 bytes. When we move `utf8` colums to `utf8mb4` columns, they'll jump from 3 bytes per character to 4 bytes per character, which may make some indexes too long. Add key schema to help spot this.
Also add nullability since it doesn't hurt.
Test Plan: See screenshots.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T1191
Differential Revision: https://secure.phabricator.com/D10499
Summary:
Ref T1191. This lays some groundwork for generating the expected schemata, so we can compare them to the actual schemata and produce a meaningful diff.
- In general, each application will subclass `PhabricatorConfigSchemaSpec` and provide a definition of the tables it expects.
- This class has helper methods to mostly-automatically build table definitions for Lisk and (in the future) edges.
- When building expected schema, we specify a "data type", like "epoch". This is the type of data the application stores in the column, from the application's point of view. The SchemaSpec converts this into the best avilable storage type: for example, "text" will translate to `utf8mb4` if it's availalbe, or `binary` if not. This gives us a layer of indirection to insulate us from craziness.
Test Plan: See screenshots.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T1191
Differential Revision: https://secure.phabricator.com/D10497
Summary:
Ref T1191. This builds on the "view of the database as it exists" by building a view of the database as it is expected to exist (this is mostly empty for now) and comparing the two. We now render a view of the "comparison schema", which is the actual schema merged with the expected schema and annotated with the differences.
(I'm merging them like this because it makes it easier to handle both "missing" and "surpulus" warnings in a consistent way. If we tried to annotate just the actual or expected schema, the absence of components which are expected to exist is messy to handle.)
Test Plan: See screenshots.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T1191
Differential Revision: https://secure.phabricator.com/D10496
Summary:
Ref T1191. Plan here is:
- Build a tool showing the current schemata status (this diff).
- Have it compare the current status to the desired status (partly here, mostly in future diffs).
- Then add a migration tool, and eventually a setup issue to tell people to run it.
Test Plan:
Reviewed current schemata.
{F204492}
{F204493}
{F204494}
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T1191
Differential Revision: https://secure.phabricator.com/D10494
Summary: See rP8806fb0296c2.
Test Plan:
me fail english
with bonus!
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Differential Revision: https://secure.phabricator.com/D10514
Summary:
Fixes T6084. Changes:
- Rename `phabricator.show-beta-applications` to `phabricator.show-prototypes`, to reinforce that these include early-development applications.
- Migrate the config setting.
- Add an explicit "no support" banner to the config page.
- Rename "Beta" to "Prototype" in the UI.
- Use "bomb" icon instead of "half star" icon.
- Document prototype applications in more detail.
- Explicitly document that we do not support these applications.
Test Plan:
- Ran migration.
- Resolved "obsolete config" issue.
- Viewed config setting.
- Browsed prototypes in Applications app.
- Viewed documentation.
Reviewers: chad, btrahan
Reviewed By: btrahan
Subscribers: epriestley, hach-que
Maniphest Tasks: T6084
Differential Revision: https://secure.phabricator.com/D10493
Summary: ...also re-jiggers all the anchor stuff to use $xaction ID. This seemed like the simplest way once I got in the code, as well as having nice properties for if / when we want to re-add some ajax stuff since the ID is a pretty solid piece of data to key off. Fixes T6083.
Test Plan: mentioned DX in private DX+1. Could see on DX the mention as me and not as the other user. For transactions, I left a comment on Paste and it worked, and I edited an existing transaction and it worked.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: epriestley, Korvin
Maniphest Tasks: T6083
Differential Revision: https://secure.phabricator.com/D10488
Summary: Fixes T5536. Some bonus pht in there.
Test Plan: made a diff hovered over the stars and saw my new text.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: epriestley, Korvin
Maniphest Tasks: T5536
Differential Revision: https://secure.phabricator.com/D10487
Summary:
Implements a new transaction - still TYPE_ACTION - but using a new DifferentialAction::ACTION_COMMIT_CLOSE. Augment rendering as necessary to display this new transaction. Saves enough information so T3686 is possible but stops short of implementing a popup to display this information. Fixes T5875. Ref T3686.
One small display oddity - this new transaction now renders at the top of the transaction group whereas when it was a comment it was on the bottom. I think this is basically okay but if not how fix? (Playing with the "strength" of these actions will mess up the email too?)
Test Plan: made a diff X that fixed task Y. committed. checked diff X, task Y, and the commit pages for proper transactions and all looked good.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: epriestley, Korvin
Maniphest Tasks: T3686, T5875
Differential Revision: https://secure.phabricator.com/D10485
Summary:
Ref T5835. This is still completely made up (no payment integration), but you can "back" an initiative, type a number in the box, and generate a database row. You can then seach for backers and things you've backed and such.
Notable changes:
- Renamed "FundBacking" to "FundBacker". The former name was sort of because you can back things multiple times, but stuff like `$backings` was just too weird.
- I think that's it?
Test Plan:
- Backed an initiative.
- Viewed that I became a backer.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T5835
Differential Revision: https://secure.phabricator.com/D10486
Summary:
Ref T5835. This is all pretty boilerplate, and does not interact with Phortune at all yet.
You can create "Initiatives", which have a title and description, and support most of the expected infrastructure (policies, transactions, mentions, edges, appsearch, remakrup, etc).
Only notable decisions:
- Initiatives have an explicit owner. I think it's good to have a single clearly-responsible user behind an initiative.
- I think that's it?
Test Plan:
- Created an initiative.
- Edited an initiative.
- Changed application policy defaults.
- Searched for initiatives.
- Subscribed to an initiative.
- Opened/closed an initiative.
- Used `I123` and `{I123}` in remarkup.
- Destroyed an initiative.
Reviewers: chad, btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T5835
Differential Revision: https://secure.phabricator.com/D10481
Summary:
Fixes T6044. We've had two cases (both the same install, coincidentally) where pages got hung doing too much data fetching.
When pages hang, we don't get a useful stack trace out of them, since nginx, php-fpm, or PHP eventually terminates things in a non-useful way without any diagnostic information.
The second time (the recent Macros issue) I was able to walk the install through removing limits on nginx, php-fpm, php, and eventually getting a profile by letting the page run for several minutes until the request completed. However, this install is exceptionally technically proficient and this was still a big pain for everyone, and this approach would not have worked if the page actually looped rather than just taking a long time.
Provide `debug.time-limit`, which should give us a better tool for reacting to this situation: by setting it to a small value (like 10), we'll kill the page after 10 seconds with a trace, before nginx/php-fpm/php/etc can kill it uselessly. Hopefully that will be enough information to find the issue (generally, getting a trace has been 95% of the problem in the two cases we've encountered).
Test Plan: Set this option to `3` and added a sleep loop, saw a termination after 3 seconds with a useful trace.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: csilvers, joshuaspence, epriestley
Maniphest Tasks: T6044
Differential Revision: https://secure.phabricator.com/D10465
Summary: Fixes T6052. Allow installs to link to legal documents, etc., in the page footer.
Test Plan:
- Configured a footer.
- Viewed workboards (no footer).
- Viewed Conpherence (no apparent disruption, I think everything z-indexes over the footer).
- Viewed stuff on mobile (seems OK).
- Viewed login page (saw footer).
{F201718}
Reviewers: btrahan, chad
Reviewed By: chad
Subscribers: epriestley
Maniphest Tasks: T6052
Differential Revision: https://secure.phabricator.com/D10466
Summary: Fixes T6059.
Test Plan: Made a comment on TX mentioning TX and TX+1. TX did not get a "mentioned" transaction while TX+1 did.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: epriestley, Korvin
Maniphest Tasks: T6059
Differential Revision: https://secure.phabricator.com/D10464
Summary: Some versions of PHP aren't very happy about both interfaces and superclasses defining a method. Just remove it from MentionableInterface and leave it as implicit.
Auditors: btrahan
Summary: Fixes T4036. Now if you say something on diff X like "This reminds me of Tx and Dy and commitHashFoo and Px." each of those objects gets a little visible transaction that the mention occurred. No feed, email, or notifications.
Test Plan: made a comment like above and verified transactions. also submitted a diff that "Fixes Tx" and Tx did not get the transaction as expected.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: joshuaspence, epriestley, Korvin
Maniphest Tasks: T4036
Differential Revision: https://secure.phabricator.com/D10451
Summary:
Fixes T6056. This documentation is out of date and not very useful.
We could probably fold this option into `maniphest.priorities` at some point.
Test Plan: Read documentation, clicked link.
Reviewers: btrahan, chad
Reviewed By: chad
Subscribers: epriestley
Maniphest Tasks: T6056
Differential Revision: https://secure.phabricator.com/D10450
Summary: see title. Ref T5875.
Test Plan: Merged one task into another task - verified transactions on both tasks. Merged two tasks into another task - verified transactions on all three tasks. Checked out my feed and saw MERGE_INTO stories and MERGE_FROM stories.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: epriestley, Korvin
Maniphest Tasks: T5875
Differential Revision: https://secure.phabricator.com/D10427
Summary:
Ref T6013. I accidentally made this cost explosviely huge when fixing macros for logged out users in D10411.
Specifically, we'd load all the macros, which would load all the files, which would load all the macros (to do policy checks), which would fill out of cache I think (but maybe only some of the time?). Anyway, bad news.
Instead, only load the files if we need them.
Test Plan: Viewed macro main page, macro detail, used a macro, used a meme, edited a macro, edited audio.
Reviewers: btrahan, csilvers
Reviewed By: csilvers
Subscribers: epriestley, spicyj
Maniphest Tasks: T6013
Differential Revision: https://secure.phabricator.com/D10428
Summary:
Ref T2783. Fixes T6039.
- Provide `authorPHID` and `committerPHID` to resolve T6039.
- In message parser, store author/email strings.
- In cached results, emit author/email strings.
Test Plan: Called method with and without bypassCache. Used `reparse.php` to repopulate data on an old commit.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T2783, T6039
Differential Revision: https://secure.phabricator.com/D10424
Summary: Fixes T6037. We don't currently write the "this file is attached to such-and-such object" edge on comment edits.
Test Plan: Edited a comment, adding `{Fnnn}`. Verified file was not attached before the edit, but was afterward.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T6037
Differential Revision: https://secure.phabricator.com/D10423
Summary:
Ref T5968. Issues we've seen from users include:
- Concern about severity ("... Need Restarting").
- Reduce severity of explanatory text ("Different Config", "not severe").
- Explain consequences in more detail.
- In D10420, make "Ignore" easier to find.
- Scope language for the multi-machine case ("at least one daemon").
- Confusion about why daemons need restarting.
- Unbury the lede ("Daemons and Web Have Different Config").
- Make it clear that the root cause is a different checksum by showing the checksum. (This just hammers home that we're comparing checksums and this issue is about config checksums and we're not making it up, the checksums probably aren't that useful on their own.)
- Difficulty understanding how to proceed when restarting does not resolve the issue:
- Call out steps to take on the daemon console explicitly.
- Walk through troubleshooting PHABRICATOR_ENV.
- Walk through troubleshooting multiple `local.json`.
Test Plan: {F199245}
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T5968
Differential Revision: https://secure.phabricator.com/D10421
Summary:
Ref T4331. Ref T5968. Users sometimes have trouble figuring out how to ignore issues. The option is a bit hard to spot, especially if you aren't familiar with interfaces yet.
Make it a button on the issue page itself instead.
Test Plan:
Normal issue:
{F199225}
Ignored issue:
{F199226}
Fatal issue:
{F199227}
Reviewers: btrahan, chad
Reviewed By: chad
Subscribers: epriestley
Maniphest Tasks: T4331, T5968
Differential Revision: https://secure.phabricator.com/D10420
Summary: pre-patch, these fatal, since we overwrite $content to be just a string so methods fail later in the code. Instead, write a $content_str to keep $content as the proper data.
Test Plan: editing a document and on save it showed me the view page! (as opposed to fataling and staying on the eidt page)
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: epriestley, Korvin
Differential Revision: https://secure.phabricator.com/D10426
Summary:
Ref T6013. A very long time ago, edges were less clearly low-level infrastructure, and some user-aware stuff got built around edge edits.
This was kind of a mess and I eventually removed it, during or prior to T5245. The big issue was that control flow was really hard to figure out as things went all the way down to the deepest level of infrastructure and then came back up the stack to events and transactions. The new stuff is more top-down and generally seems a lot easier and cleaner.
Consequently, actors are no longer required for edge edits. Remove the parameter.
Test Plan: Poked around; ran unit tests.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley, hach-que
Maniphest Tasks: T6013
Differential Revision: https://secure.phabricator.com/D10412
Summary:
Fixes T6013. Old image macros/memes never had the file edge written.
We also never wrote file edges for audio.
Finally, the meme controller didn't allow public access.
Write edges for images and audio, perform a migration to populate the historic ones, and make the Editor keep them up to date going forward.
Test Plan:
- Updated image, saw new image attach and old image detach.
- Updated audio, saw new audio attach and old audio detach.
- Ran migration.
- Viewed memes as a logged-out user.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T6013
Differential Revision: https://secure.phabricator.com/D10411
Summary:
Ref T6013. Currently, when we create a thumbnail, it gets its own (default) file visibility policy.
In particular, this causes the issue in T6013: thumbnails get "all users" visibility, which does not include logged-out users.
Instead, a thumbnail should just have the same visibility as the original file does. Enforce this:
- When loading thumbnails, reject thumbnails with invisible originals.
- When filtering thumbnails, permit thumbnails with visible originals.
Test Plan: As a logged-out user, thumbnails are now visible when the original files are attached to visible objects.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T6013
Differential Revision: https://secure.phabricator.com/D10410
Summary: Fixes T6011. See that task for discussion. We can detect when `memory_limit` will be the limiting factor for drag-and-drop uploads and warn administrators about it.
Test Plan: Fiddled configuration values and hit, then resolved, the issue.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T6011
Differential Revision: https://secure.phabricator.com/D10413
Summary: Fixes T6001. We currently don't allow empty secrets, but accounts with no password are occasionally used in the wild.
Test Plan:
- Created a credential with an empty secret.
- Revealed secret, saw empty message.
- Edited it (no form changes), saw secret unchanged.
- Changed it to a nonempty secret.
- Revealed nonempty secret.
- Edited it (no form changes), saw secret unchanged.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T6001
Differential Revision: https://secure.phabricator.com/D10414
Summary: Fixes T5982. Probably. I'm just guessing here but like 95% sure this will fix it and 99% sure it won't hurt/break anything.
Test Plan: Still works on my 64-bit install, for what little that's worth.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T5982
Differential Revision: https://secure.phabricator.com/D10415
Summary: Fixes T5993. Now that we have a context menu we can make some edit operations easier to access.
Test Plan: Toggled column visibility. Verified board state (columns shown/hidden, ordering) was retained.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T5993
Differential Revision: https://secure.phabricator.com/D10417
Summary: make it use the value of the revision before any post-commit magic has occurred. Fixes T4754
Test Plan: made a herald rule that said "if revision exists, and revision accept does not exists, block push". tried to push a commit that had a revision that wasn't accepted and I was blocked.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: mbishopim3, epriestley, Korvin
Maniphest Tasks: T4754, T4574
Differential Revision: https://secure.phabricator.com/D10393
Summary:
Ref T2374. Fixes T5988.
Keep track of what's been killed and not been killed, and surface that maybe you need sudo if things don't get killed with --force
...also basically make this force thing work. I managed to convinced myself stuff was getting killed with --force when it mostly wasn't. Make sure the --force parameter gets pushed as low as it needs to go to have things get killed.
Test Plan:
- `sudo ./bin/phd restart`
- `rm -rf /var/tmp/phd/pid/*`
- `./bin/phd stop` --> get warning about rogue daemons
- `./bin/phd stop X` --> get warning about no running daemons
- `./bin/phd stop --force` --> get warning about not being able to kill daemons
- `sudo ./bin/phd stop --force` --> kill daemons successfully
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: epriestley, Korvin
Maniphest Tasks: T2374, T5988
Differential Revision: https://secure.phabricator.com/D10386
Summary: Ref T6031. I figure its totally cool to include the user creating the task as a subscriber, even if from the template case, so just do that there too. Code is written such that if the user wasn't already in the subscriber case they end up being the last person in the tokenizer. Theoretically this should make any users who didn't want to be automagically subscribed via the create from template case to remove themselves.
Test Plan: made a template from a task that didn't have me as a subscriber initially and observed i was a subscriber.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: epriestley, Korvin
Maniphest Tasks: T6031
Differential Revision: https://secure.phabricator.com/D10408
Summary: Fixes T6029. We should append custom fields last so they show up after things like projects, tokens, etc that render via UI events.
Test Plan: viewed a task with custom fields and projects was last
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: epriestley, Korvin
Maniphest Tasks: T6029
Differential Revision: https://secure.phabricator.com/D10407
Summary:
Ref T2783. This populates the following fields in DiffusionQueryCommitsConduitAPIMethod using DiffusionLowLevelCommitQuery when `bypassCache` is set to true:
* `authorName`
* `authorEmail`
* `committerName`
* `committerEmail`
* `message`
* `hashes`
The original outline called for `authorPHID` and `committerPHID` as well (but no `message` field). As far as I can tell, the PHIDs aren't actual a property on `DiffusionCommitRef`, and since the intention of this is to be able to populate a `DiffusionCommitRef`, I haven't included them. Let me know if we really do need the PHIDs here.
Test Plan: Tested using 3 Phabricator instances (one web, one taskmaster and one storage). The web and taskmaster tiers are directed at the Conduit API of the storage tier. Made a `diffusion.querycommits` from the Conduit app on the web tier instance and saw the data populated from the raw VCS data (located on the storage tier).
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: epriestley, Korvin
Maniphest Tasks: T2783
Differential Revision: https://secure.phabricator.com/D10399
Summary:
Fixes T5956. We changed the default mail encoding to `quoted-printable` to fix delivery via SendGrid via SMTP, but this broke multiple other mailers.
- Change the default back to 8bit (which works everywhere except SendGrid).
- Add a configuration setting for selecting `quoted-printable`.
- Document this issue.
- Discourage use of SendGrid in documentation.
(IMPORTANT) @klimek @nickz This reverts the `quoted-printable` fix for SendGrid. You will need to adjust your configurations (set `phpmailer.smtp-encoding` to `quoted-printable`) and restart your daemons or mail will get double newlines again.
Test Plan:
- Sent mail via SendGrid with various `phpmailer.smtp-encoding` settings, saw mail arrive with specified encoding.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: klimek, nickz, epriestley
Maniphest Tasks: T5956
Differential Revision: https://secure.phabricator.com/D10397
Summary: Ref T3307. Only one I thought was tricky was Excel; I went with bytes there like it was email.
Test Plan: played around on a few endpoints but mostly thought carefully
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: epriestley, Korvin
Maniphest Tasks: T3307
Differential Revision: https://secure.phabricator.com/D10392
Summary: purple != violet, and in our CSS we call these things by the fanciest of terms. Fixes T5995.
Test Plan: flagged something purple and saw that the "remove purple flag" flag was indeed purple. quickly tested other colors and they all seem good too.
Reviewers: epriestley, chad
Reviewed By: chad
Subscribers: epriestley, Korvin
Maniphest Tasks: T5995
Differential Revision: https://secure.phabricator.com/D10389
Summary: we did some security lock down on URI stuff and I think this was a casualty. Fixes T5992.
Test Plan: left a comment, got redirected. no more 500 response.
Reviewers: chad, epriestley
Reviewed By: epriestley
Subscribers: epriestley, Korvin
Maniphest Tasks: T5992
Differential Revision: https://secure.phabricator.com/D10388
Summary:
Ref T5405.
- `--limit` wasn't actually used anywhere.
- Make it mean "the N newest lines".
Test Plan: Ran `bin/phd log`, `bin/phd log --limit 3`.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T5405
Differential Revision: https://secure.phabricator.com/D10385
Summary:
Resolves T5987. This build step was at some point converted to use yielding, which meant that whenever the build step executes it will create a new log. This checks to see if there is an existing log before creating a new one and uses that instead.
Long term we're going to need some way of attaching data to `PhabricatorWorkerYieldException` that can be read when the build step starts again; this will allow us to move more build steps off `while (...) { ... sleep(X); }` loops and onto yielding.
Test Plan: Tested locally.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: epriestley
Maniphest Tasks: T5987
Differential Revision: https://secure.phabricator.com/D10383
Summary: Fixes T4387.
Test Plan: Setup a mercurial repository for rabbitmq-server. Browsed around it and things looked good.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: epriestley, Korvin
Maniphest Tasks: T4387
Differential Revision: https://secure.phabricator.com/D10380
Summary: Looks like I missed this when implementing custom actions and hence you can't currently use custom actions on the pre-commit adapters.
Test Plan: Added a custom action to a pre-commit Herald rule.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: epriestley, Korvin
Differential Revision: https://secure.phabricator.com/D10316
Summary: Ref T1049. This messages is always printed to standard error now that the known hosts file is set to /dev/null. This hides the warning so that we'll be able to parse stderr for Windows hosts (where Powershell decides to output XML...)
Test Plan: Tested locally and verified the warning no longer appears.
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: epriestley, Korvin
Maniphest Tasks: T1049
Differential Revision: https://secure.phabricator.com/D10362
Summary: Ref T1049. Because we no longer destroy artifacts when builds are restarted, we need the build generation number to be part of the artifact key, otherwise we get collisions when restarting builds that contain build steps that emit artifacts.
Test Plan: Ran it with a build plan of "Lease Host" and "Run Command", no longer got an artifact key crash.
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: epriestley, Korvin
Maniphest Tasks: T1049
Differential Revision: https://secure.phabricator.com/D10336
Summary: This prevents crashes when looking at builds, where the build steps have been deleted on the build plan since the build was run. Currently the only information that's pulled from the build step is the description (because this was too large to copy to every target).
Test Plan: Tested it locally.
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: epriestley, Korvin
Differential Revision: https://secure.phabricator.com/D10361
Summary: Ref T2374. While building D10367 I noticed that phd was finding rogue daemons way more than it should be. Re-jigger this code path so rogue daemons are checked for *after* we've dealt with known daemons. This keeps the logic pretty simple overall.
Test Plan: phd start; kill pid files; phd stop and get the right warning; phd stop --force and it kills the rogue demons. phd stop in normal conditions no longer reporting rogue daemons erroneously
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: epriestley, Korvin
Maniphest Tasks: T2374
Differential Revision: https://secure.phabricator.com/D10368
Summary: D10281 upgraded us to modern infrastructure but I think forget to set this little helper to return true. Fixes T5975.
Test Plan: paged through notifications with glee
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: epriestley, Korvin
Maniphest Tasks: T5975
Differential Revision: https://secure.phabricator.com/D10369
Summary: Shows the UI everywhere. Also asort() the keys before calculating the environment hash as that is probably an issue for someone at some point we just don't need to have. Ref T5968.
Test Plan: Viewed the setup check and saw a link to the daemon console. Viewed the daemon console and saw the various stale config daemons. Clicked a daemon and saw a "stale config" header icon where expected. Restarted daemons and all of this went away.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: epriestley, Korvin
Maniphest Tasks: T5968
Differential Revision: https://secure.phabricator.com/D10367
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: 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: 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: $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:
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 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
