Summary:
Ref T3092.
Releeph's objects basically go like this:
- At the top level, we have Projects (like "www" or "libphutil")
- Each project has Branches (like "LATEST" or "v1.1.3")
- Each branch has Requests (like pull requests, e.g. "please merge commit X into branch Y (in project Z)")
Currently, there's no real "project detail" or "branch detail" page. Instead, we have a search results page for their contained objects. That is, the "project detail" page shows a list of branches in the project, using ApplicationSearch.
This means that operations like "edit" and "deactivate" are one level up, on the respective list pages.
Instead, move details onto the detail pages. This gives us more room for actions and information, and simplifies the list views.
Basically, these are "detail pages" where the object content is a search interface. We do something simliar to this in Phame right now, although it's messier there (no ApplicationSearch yet).
@chad, you might have some ideas here. Roughly, the design question is "How should we present an object's detail view when its content is really a search interface (Phame Blog for Posts, Releeph Project for Branches)?"
I think the simple approach I've taken here (see screenshot) gives us reasonable results, but overall it's something we haven't done much or done too much thinking about, I think.
Test Plan: {F54774}
Reviewers: btrahan
Reviewed By: btrahan
CC: chad, aran
Maniphest Tasks: T3092
Differential Revision: https://secure.phabricator.com/D6771
Summary: Depends on D6769, removes 'dust' and uses a similar color background.
Test Plan: Review colors in sandbox.
Reviewers: epriestley
Reviewed By: epriestley
CC: Korvin, aran
Differential Revision: https://secure.phabricator.com/D6772
Summary:
^\s+(['"])dust\1\s*=>\s*true,?\s*$\n
Test Plan: Looked through the diff.
Reviewers: chad
Reviewed By: chad
CC: aran
Differential Revision: https://secure.phabricator.com/D6769
Summary: Fixes T2836
Test Plan: make a diff, get it approved, arc land, verify things okay. ask users on T2836 to try.
Reviewers: epriestley
Reviewed By: epriestley
CC: Korvin, aran
Maniphest Tasks: T2836
Differential Revision: https://secure.phabricator.com/D6770
Summary:
This diff accomplishes this task by adding an arbitrary metadata store to PhabricatorObjectHandle. This seemed like it would be "necessary eventually"; for example if / when we decide we want to show images in these stories we'd need to add some more arbitrary data. A point of debate is this technique will yield the _current_ data and not the data at the time the transaction was originally made. I can see this being both desirable and non-desirable.
Otherwise, the best way to do this is to make a new transaction type specifically for create and store exactly what data we think we would need.
(and there's probably many other ways but they require much more work...)
Test Plan: viewed some pholio create stories and yes, they had the description showing.
Reviewers: epriestley
Reviewed By: epriestley
CC: Korvin, aran
Maniphest Tasks: T3685
Differential Revision: https://secure.phabricator.com/D6767
Summary: Ref T2852. Asana is launching some kind of silent follow thing today; I don't know what the API is but it's probably something like this. I'll update this to actually make the right call once the call exists, this is mostly just a placeholder so I don't forget about it.
Test Plan: None yet, this API isn't documented or live and doesn't work yet so it can't be tested.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran, moskov
Maniphest Tasks: T2852
Differential Revision: https://secure.phabricator.com/D6740
Summary: Ref T3092. Fixes T3724. Use modern/flexible UI for these interfaces. Removes the ability to retarget an existing branch (you can just close it and open a new one if you made a mistake).
Test Plan: {F54437} {F54438}
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T3092, T3724
Differential Revision: https://secure.phabricator.com/D6765
Summary:
Releeph branch lists in project views have a bunch of custom UI right now; give them more standard UI and ApplicationSearch.
This drops a small piece of functionality: we now show only a total open request count instead of a detailed enumeration of each request status. I assume this is reasonable (that is, the important piece is "is there something to do on this branch?"), but we can muck with it if the more detailed status is important.
Test Plan: {F54344}
Reviewers: btrahan
Reviewed By: btrahan
CC: LegNeato, aran
Maniphest Tasks: T3656
Differential Revision: https://secure.phabricator.com/D6764
Summary: Ref T2766. Does the integration via ApplicationTransactionsEditor. Only did addCC and Flag for proof of concept.
Test Plan: Made a rule to cc, made a rule to flag. They worked! (will attach screens to diff)
Reviewers: epriestley
Reviewed By: epriestley
CC: Korvin, aran
Maniphest Tasks: T2766
Differential Revision: https://secure.phabricator.com/D6766
Summary: Oops -- forgot to do this in D6759.
Test Plan: `git status`
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Differential Revision: https://secure.phabricator.com/D6760
Summary: We currently die if an event listener throws when registering (e.g., because it is misconfigured), but this prevents you from running `bin/config` to fix it, which is a bit silly.
Test Plan: Created this revision with an invalid listener in config.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Differential Revision: https://secure.phabricator.com/D6761
Summary: somewhere along the line this broke. Before this patch we fail the visibility check since its based on Conpherence Participants which don't get created and attached until applyExternalEffects. Believe it or not, this was the least gross fix I could come up with; since the permission check is done SO early most other ideas I had involved creating a dummy participant object to pass the check then handling things for real later on... Ref T3723.
Test Plan: created a conpherence with myself - great success
Reviewers: epriestley
Reviewed By: epriestley
CC: chad, Korvin, aran
Maniphest Tasks: T3723
Differential Revision: https://secure.phabricator.com/D6762
Summary:
Ref T3721. Releeph currently attempts to implement a flexible, field-driven search for branches, but it's building all of its own infrastructure and it ends up heading down some weird paths. In particular, it loads **every** request and then makes calls into fields to filter them. It also tries to be very very general, which isn't really necessary (for example, I think it's reasonable for us to assume that we won't let you disable the "requestor" field).
ApplicationSearch and CustomField provide more scalable approaches to this problem; move search on top of them. The query still ends up doing some filtering in-process, but it's now far more limited in scope and can be denormalized later.
Test Plan: {F54304}
Reviewers: btrahan
Reviewed By: btrahan
CC: chad, aran
Maniphest Tasks: T3721
Differential Revision: https://secure.phabricator.com/D6758
Summary: I added this easier-extension mechanism a while ago but only added the actual directories to libphutil and arcanist. This one should work, it just wasn't checked into the repository.
Test Plan: Added a file with an exception in it to the directory, verified the exception was thrown at runtime.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Differential Revision: https://secure.phabricator.com/D6759
Summary:
Ref T3718. This moves custom field rendering on the edit screen to PhabricatorCustomField and makes all the APIs conformant.
We still run through edit with both old-school and new-school sets of fields, because the actual editing isn't on the new stuff yet. That will happen in a diff or two.
Test Plan: Edited a request; intentionally introduced errors and verified the form behaved as expected.
Reviewers: btrahan, testuser1122344
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T3718
Differential Revision: https://secure.phabricator.com/D6756
Summary: Defaults hovercards off everywhere feed stories are shown. I tried to find where to put this in so /feed/ could display them, but got horribly lost and confused in SearchQueryLandView
Test Plan: turn hovercards on and off, inspect elements.
Reviewers: epriestley, btrahan
Reviewed By: epriestley
CC: Korvin, aran
Differential Revision: https://secure.phabricator.com/D6757
Summary: Ref T3718. This is not used and does not seem particularly useful.
Test Plan: Grep.
Reviewers: btrahan
Reviewed By: btrahan
CC: LegNeato, aran
Maniphest Tasks: T3718
Differential Revision: https://secure.phabricator.com/D6755
Summary: Currently, we check that the user can view and edit their own transaction, which is always true. Instead, check that they can view the object. I'll fix this with a more tailored check against the EDIT capability that's per-transaction later.
Test Plan: Applying no transactions no longer fatals with undefined `$xaction`.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Differential Revision: https://secure.phabricator.com/D6754
Summary:
Ref T1703. Ref T3718. This introduces `PhabricatorCustomFieldAttachment`, which is just a fancy `array()`. The goal here is to simplify `PhabricatorCustomFieldInterface` as much as possible.
In particular, it can now use common infrastructure (`assertAttached()`) and is more difficult to get wrong.
Test Plan: Edited custom fields on profile.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T1703, T3718
Differential Revision: https://secure.phabricator.com/D6752
Summary:
Ref T1703. Ref T3718. The `PhabricatorCustomFieldList` seems like a pretty good idea. Move more code into it to make it harder to get wrong.
Also the sequencing on old/new values for these transactions was a bit off; fix that up.
Test Plan: Edited standard and custom profile fields.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T1703, T3718
Differential Revision: https://secure.phabricator.com/D6751
Summary:
Fixes T3661. Ref T3718. This makes Releeph custom fields extend PhabricatorCustomField so we can start moving over other pieces of infrastructure (rendering, storage, etc) to run through the same pathways. It's roughly the minimum amount of work required to be able to move forward.
NOTE: This removes per-project custom field selectors. Fields are now configured for an entire install. My understanding is that Facebook does not use this feature, and modern field infrastructure has moved away from selectors.
Test Plan: Viewed and edited projects, branches, and requests in Releeph. Grepped for removed config. Grepped for `field_selector`.
Reviewers: btrahan
Reviewed By: btrahan
CC: LegNeato, aran
Maniphest Tasks: T3661, T3718
Differential Revision: https://secure.phabricator.com/D6750
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
