Summary:
Ref T1702. Ref T3718. There are a couple of things going on here:
**PhabricatorCustomFieldList**: I added `PhabricatorCustomFieldList`, which is just a convenience class for dealing with lists of fields. Often, current field code does something like this inline in a Controller:
foreach ($fields as $field) {
// do some junk
}
Often, that junk has some slightly subtle implications. Move all of it to `$list->doSomeJunk()` methods (like `appendFieldsToForm()`, `loadFieldsFromStorage()`) to reduce code duplication and prevent errors. This additionally moves an existing list-convenience method there, out of `PhabricatorPropertyListView`.
**PhabricatorUserConfiguredCustomFieldStorage**: Adds `PhabricatorUserConfiguredCustomFieldStorage` for storing custom field data (like "ICQ Handle", "Phone Number", "Desk", "Favorite Flower", etc).
**Configuration-Driven Custom Fields**: Previously, I was thinking about doing these with interfaces, but as I thought about it more I started to dislike that approach. Instead, I built proxies into `PhabricatorCustomField`. Basically, this means that fields (like a custom, configuration-driven "Favorite Flower" field) can just use some other Field to actually provide their implementation (like a "standard" field which knows how to render text areas). The previous approach would have involed subclasssing the "standard" field and implementing an interface, but that would mean that every application would have at least two "base" fields and generally just seemed bleh as I worked through it.
The cost of this approach is that we need a bunch of `proxy` junk in the base class, but that's a one-time cost and I think it simplifies all the implementations and makes them a lot less magical (e.g., all of the custom fields now extend the right base field classes).
**Fixed Some Bugs**: Some of this code hadn't really been run yet and had minor bugs.
Test Plan:
{F54240}
{F54241}
{F54242}
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T1702, T1703, T3718
Differential Revision: https://secure.phabricator.com/D6749
Summary: Ref T3656. Releeph denormalizes branch cut point identifiers into Branch objects, but this information isn't useful or used for sorting, filtering, or enforcing unique constraints. Instead, derive it via noramlized pathways from the `cutPointCommitPHID`.
Test Plan: Ran storage upgrade. Ran `releephwork.getbranch` and `releeph.getbranches`. Grepped for `cutPointCommitIdentifier`.
Reviewers: btrahan
Reviewed By: btrahan
CC: LegNeato, aran
Maniphest Tasks: T3656
Differential Revision: https://secure.phabricator.com/D6636
Summary:
Fixes T3660. Releeph Projects currently have an unused one-to-one mapping to Phabricator projects. This isn't consistent with other applications and has no integrations or uses. Get rid of it.
NOTE: Waiting for signoff from @legneato on T3660 before pulling the trigger here.
Test Plan: Created and edited Releeph projects. Grepped for references to project ID; there are a dozen or so but they're all either Releeph projects or Arcanist projects.
Reviewers: btrahan
Reviewed By: btrahan
CC: LegNeato, aran
Maniphest Tasks: T3660
Differential Revision: https://secure.phabricator.com/D6635
Summary: Ref T3655. Depends on D6633. This removes the writes and the column.
Test Plan: Created a project, edited a project. Verified the table doesn't have any keys including this column.
Reviewers: btrahan
Reviewed By: btrahan
CC: LegNeato, aran
Maniphest Tasks: T3655
Differential Revision: https://secure.phabricator.com/D6634
Summary:
Ref T3655. ReleephProject currently has both `repositoryID` and `repositoryPHID`, which point to the same object and are reudundant. Get rid of all reads of `repositoryID`.
NOTE: This makes project loads depend on repository loads. The eventual rule here will be that you must be able to see a repository in order to see projects for that repository, which seems like a reasonable rule. We might need to tailor it more than this (e.g., if there are branch read permissions down the line) but this seems like a reasonable minimum.
Test Plan: Grepped for `repositoryID` in `releeph/`. Called `releeph.getbranches`.
Reviewers: btrahan
Reviewed By: btrahan
CC: LegNeato, aran
Maniphest Tasks: T3655
Differential Revision: https://secure.phabricator.com/D6633
Summary:
Ref T1809. Provide ApplicationSearch to Flags and allow the user to select flags by color.
@chad might have some design feedback on my control.
Test Plan: {F54131}
Reviewers: btrahan, chad
Reviewed By: chad
CC: aran
Maniphest Tasks: T1809
Differential Revision: https://secure.phabricator.com/D6747
Summary:
Ref T1809. Ref T603. Ref T3599. Makes flags policy aware.
This change reduces the utility of flag search/browse; the next change will switch it to ApplicationSearch to restore utility. Representing all that ordering in terms of cursor paging is also a giant pain.
Test Plan: Viewed Differential, Flags, etc. Grepped for all PhabricatorFlagQuery callsites.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T603, T1809, T3599
Differential Revision: https://secure.phabricator.com/D6746
Summary: take epriestley's feedback 'cuz its good
Test Plan: collapse, expand, use undo like a rockstar. observe proper behavior
Reviewers: epriestley
Reviewed By: epriestley
CC: Korvin, aran
Maniphest Tasks: T2258
Differential Revision: https://secure.phabricator.com/D6748
Summary: Fixes T2258.
Test Plan: collapsed and expanded file via the dropdown - good stuff. got the "undo" element into the mix - also good stuff.
Reviewers: epriestley
Reviewed By: epriestley
CC: chad, Korvin, aran
Maniphest Tasks: T2258
Differential Revision: https://secure.phabricator.com/D6742
Summary: Fixes T3715. Makes "visible" global instead of per-menu, so all the menus share a visible state.
Test Plan: For menus A and B, clicked "A, A", "A, B, A", "A, B, B", "A, B, B, A", etc. Couldn't figure out a way to break it.
Reviewers: btrahan, chad
Reviewed By: chad
CC: aran
Maniphest Tasks: T3715
Differential Revision: https://secure.phabricator.com/D6745
Summary: Fixes T2348. We should probably do some of this more broadly, but can tackle them one at a time as they arise, since many fields have no effective length limit.
Test Plan: {F54126}
Reviewers: btrahan, asherkin
Reviewed By: asherkin
CC: aran
Maniphest Tasks: T2348
Differential Revision: https://secure.phabricator.com/D6744
Summary: See IRC. This is dumb but I think we should try to work by default on Debian, and it doesn't cost us too much. See inline comment for more.
Test Plan:
- No `disable_functions`, restarted, worked fine.
- Set `disable_functions = pcntl_derp`, restarted, worked fine.
- Set `disable_functions = derp`, restarted, setup fatal.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Differential Revision: https://secure.phabricator.com/D6741
Summary: Ref T2852. Bleh, gross. Does what it says in the title.
Test Plan: {F54024}
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T2852
Differential Revision: https://secure.phabricator.com/D6735
Summary: Ref T2852. Token given stories currently try to `strip_tags()` a `PHUIFeedView` or similar, which doesn't work. Cast it to a string before stripping. This is super gross but I don't want to clean it up until after ApplicationTransactions so we can really clean up all of Feed.
Test Plan: Ran `bin/feed republish <id>` on a feed story about giving a token to a revision.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T2852
Differential Revision: https://secure.phabricator.com/D6733
Summary: Fixes T3697. Currently, we don't pass "branch" implicitly, so, e.g., when viewing a branch you don't get the right commit hash when looking up the README.
Test Plan: Viewed a non-`master` branch with a README, no fatal. Poked around and couldn't find anything suspicious.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T3697
Differential Revision: https://secure.phabricator.com/D6734
Summary:
Fixes T3709. PHP has two configuration options ('disable_functions', 'disable_classes') which allow functions and classes to be blacklisted at runtime.
Since these break things in an unclear way, raise a setup fatal if they are set.
We take a slightly more tailored approach to these in `phd` already, but I'd rather try just saying "no, this is bad" and see if we can get away with it. I suspect we can, and there's no legitimate reason to blacklist functions given that Phabricator must have access to, e.g., `proc_open()`.
Test Plan: {F54058}
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T3709
Differential Revision: https://secure.phabricator.com/D6739
Summary: Fixes T3710. The text on these options is switched around.
Test Plan: {F54051} {F54052}
Reviewers: btrahan, nmalcolm, chad
Reviewed By: chad
CC: aran
Maniphest Tasks: T3710
Differential Revision: https://secure.phabricator.com/D6737
Summary:
companion diff to D6729. This is the back-end stuff, plus calls the JS in D6729 for when images are removed, un-removed, uploaded, or replaced.
Fixes T3640.
Test Plan: messed around with images. hit save - new order! temporarily showed these stories and got text about re-ordering stuff.
Reviewers: epriestley, chad
Reviewed By: epriestley
CC: Korvin, aran
Maniphest Tasks: T3640
Differential Revision: https://secure.phabricator.com/D6731
Summary: Picked better colors and hover states.
Test Plan: test new colors, stare intently.
Reviewers: epriestley, btrahan
Reviewed By: epriestley
CC: Korvin, aran
Differential Revision: https://secure.phabricator.com/D6730
Summary:
Ref T3640. JS part only, should give you a list in `imageOrder` on the server that you can read with `$request->getStrList('imageOrder')`.
NOTE: You can't drag images into the first position; this is an existing thing that I just need to fix with DraggableList.
@chad might have some design feedback.
Test Plan: Dragged images around, things seemed to work?
Reviewers: btrahan, chad
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T3640
Differential Revision: https://secure.phabricator.com/D6729
Summary: Ref T2852. Currently, we publish commits with no audit requests and reviews with no CCs or reviewers into Asana. This creates undesired notifications, so drop events which would publish an object that doesn't exist yet and has no followers or respible users.
Test Plan: Used `bin/feed republish` to publish a story about an object with no related users, saw the publish abort with the new message. Added a CC, published again, got a publish.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T2852
Differential Revision: https://secure.phabricator.com/D6727
Summary:
We currently check if daemons are running using the filesystem and process list. These checks reach the wrong result for a lot of users because their webservers can't read the filesystem or process list. They also reach the wrong result for daemons running on other machines.
Instead, query the active daemon list to see if daemons are running. This should be significantly more reliable.
(We didn't do this before because the running daemon list mechanism didn't exist when the check was written, and at the time it was more complex than doing a simple filesystem/process list thing.)
Test Plan: Viewed `/repositories/` with and without daemons running, saw appropriate warning or lack of warning.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Differential Revision: https://secure.phabricator.com/D6722
Summary: This moved to CLI.
Test Plan: Read.
Reviewers: btrahan, chad
Reviewed By: chad
CC: aran
Differential Revision: https://secure.phabricator.com/D6724
Summary: Fixes T3703. Clear question notifications when viewing a question.
Test Plan: Gave a question a token, logged in as author, saw notification, viewed question page, notification was marked read.
Reviewers: chad, btrahan
Reviewed By: chad
CC: aran
Maniphest Tasks: T3703
Differential Revision: https://secure.phabricator.com/D6723
Summary:
Fixes T3698. Sometimes views need to render differently depending on whether they contain content or not. The existing approach for this is `isEmptyContent()`, which doesn't work well and is sort of hacky (it implies double-rendering content, which is not always free or side-effect free).
Instead, provide a test for an element without children. This test is powerful enough to catch the easy cases of `null`, etc., and just do the expected thing, but will not catch a View which is reduced upon rendering. Since this is rare and we have no actual need for it today, just accept that as a limitation.
Test Plan:
Viewed Timeline and Feed UI examples. Viewed Feed (feed), Pholio (timelineview), and Differential (old transactionview).
{F53915}
Reviewers: chad, btrahan
Reviewed By: chad
CC: aran
Maniphest Tasks: T3698
Differential Revision: https://secure.phabricator.com/D6718
Summary: This fixed a bug with macros search finding macros flagged by any user. We should only look at flags by the current user.
Test Plan: Verify that no macros flagged by another user show up in macros search.
Reviewers: epriestley
Reviewed By: epriestley
CC: Korvin, aran
Differential Revision: https://secure.phabricator.com/D6717
Summary: Cleaning up my mess, (No Filtering) should be the default selected option in macros search form.
Test Plan: Go to /macro/query/advanced/ and verify that (No Filtering) is the default selected option.
Reviewers: epriestley
Reviewed By: epriestley
CC: Korvin, aran
Maniphest Tasks: T3692
Differential Revision: https://secure.phabricator.com/D6715
Summary: I think we accidentally forgot to include this action in D6660.
Test Plan: verified it showed up in the UI to have the action be an audit
Reviewers: chad, epriestley
Reviewed By: epriestley
CC: Korvin, aran
Differential Revision: https://secure.phabricator.com/D6712
Summary: we get participation data ordered, then query conpherences by phid... be sure to resort the conpherences based on participation data. I missed this in testing 'cuz my test data is so trashy, but it is glaringly obvious in production. :/
Test Plan: replied to a very old conpherence and noted it was first in the notification panel
Reviewers: chad, epriestley
Reviewed By: epriestley
CC: Korvin, aran
Maniphest Tasks: T3641
Differential Revision: https://secure.phabricator.com/D6711
Summary: Fixes T3690. Uses standard colors, smaller borders.
Test Plan: Review Menu with and without a notification
Reviewers: epriestley, btrahan
Reviewed By: btrahan
CC: Korvin, aran
Maniphest Tasks: T3690
Differential Revision: https://secure.phabricator.com/D6710
Summary: Reuse the existing flags functionality for searching macros. Currently implemented as a simple select element (for color).
Test Plan: Flagged some macros and tried searching by them.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D6709
Summary: Fixes T3641. Probably needs some @chad love though on colors and what have you. Technique was to jam this into the existing notifications stuff as much as possible. I think its "okay" but if we were to add more stuff here (like a 3rd application) this could get a quality pass to consolidate even more code.
Test Plan: played with it in Chrome and Safari - looks reasonable
Reviewers: chad, epriestley
Reviewed By: epriestley
CC: aran, Korvin
Maniphest Tasks: T3641
Differential Revision: https://secure.phabricator.com/D6708
Summary:
This is mostly for personal reasons / lols, but they have a perfectly functional OAuth2 API and it takes like 15 minutes to add a provider now and I was in this code anyway...
@chad, we could use JIRA, Twitter and Twitch.tv auth icons if you have a chance.
Test Plan: {F53564}
Reviewers: btrahan, chad
Reviewed By: btrahan
CC: aran
Differential Revision: https://secure.phabricator.com/D6706
Summary: Ref T2852. Asana adds the actor as a follower when they create a task, so subtasks currently have up to two followers (the actor and the reviewer) when they should have only one (the reviewer). Simply removing the actor is an effective remedy for this because unfollowing tasks occurs with sneaky ninja stealth in Asana and doesn't generate notifications or even transaction activity.
Test Plan: Synchronized a revision without this patch, saw two followers on the subtask. Synchronized a revision after this patch, saw the "removeFollowers" fire and only one follower on the subtask, with no record of the removal in notifications or the transaction log.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T2852
Differential Revision: https://secure.phabricator.com/D6700
Summary: Fixes a query in √D6260.
Test Plan: View a Releeph RQ and verify that the "churn" field renders and has the right numbers in it.
Reviewers: epriestley, btrahan
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D6702
Summary: Use standard colors.
Test Plan: create status
Reviewers: epriestley, btrahan
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D6701
Summary: Ref T2769. This isn't a real adapter and its methods are increasingly hacky messes. Make "dry run" a first-class concept on the HeraldEngine instead and remove the adapter.
Test Plan: Ran Herald via test console and via CLI.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T2769
Differential Revision: https://secure.phabricator.com/D6693
Summary:
Ref T2769. This will house the transaction list and replace the "edit log" stuff.
The UI is a little bit rough and can probably share more code with the transaction history, but seems mostly-reasonable.
Test Plan: {F53253}
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T2769
Differential Revision: https://secure.phabricator.com/D6690
Summary: Ref T2769. The `HeraldRule` class has some query logic; move it into `HeraldRuleQuery`. Also some minor cleanup.
Test Plan: Ran test console, created a new revision, used `reparse.php --herald`. Verified rules triggered correctly.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T2769
Differential Revision: https://secure.phabricator.com/D6689
Summary:
Ref T2769. Move all of this stuff into Adapters and get rid of the hard-coded classes.
I cheated in two places.
Test Plan: Edited and activated Herald rules.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran, chad
Maniphest Tasks: T2769
Differential Revision: https://secure.phabricator.com/D6688
Summary: Ref T2769. Moves all traces of HeraldConditionConfig into Adapters.
Test Plan: Edited rules and used Test Console to exercise both affected code paths. Tried to save invalid rules to hit error pat.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T2769
Differential Revision: https://secure.phabricator.com/D6679
Summary: Ref T2769. Get rid of the last use of `HeraldContentTypeConfig` by moving repetition options into Adapters.
Test Plan: Viewed / edited Herald rules.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T2769
Differential Revision: https://secure.phabricator.com/D6664
Summary: Ref T2769. Use Adapters to build all the strings for transcripts, then get rid of the old maps.
Test Plan: Viewed revision and commit transcripts.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T2769
Differential Revision: https://secure.phabricator.com/D6663
Summary: Ref T2769. This cleans up almost every use of the HeraldContentTypeConfig class.
Test Plan: Viewed and edited Herald rules.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T2769
Differential Revision: https://secure.phabricator.com/D6662
Summary: Ref T2769. Shift the bulk of value and action config into Adapters.
Test Plan: Viewed and edited Herald list and rules.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran, vrana
Maniphest Tasks: T2769
Differential Revision: https://secure.phabricator.com/D6660
