Summary:
Ref T4589. When you look at a file, we load attached objects in order to run the "you can see this if you can see any attached object" policy check.
However, right now the subquery inherits the "throw on filter" flag from the parent query. This inheritance makes sense in other cases[1], but because this is an "ANY" rule it does not make sense here. In practice, it means that if the file is attached to several objects, and any of them gets filtered, you can not see the file.
Instead, explicitly drop the flag for this subquery.
[1] Sort of. It doesn't produce wrong results in other cases, but now that I think about it might produce a less-tailored error than it could. I'll look into this the next time I'm poking around.
Test Plan:
- Viewed an "All Users" file attached to a private Mock.
- Prior to this patch, I incorrectly received an exception when the Mock was loaded. This is wrong; I should be able to see the file because the policy is "All Users".
- After the patch, I can correctly view the file, just not the associated mock.
{F127074}
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: 20after4, aran, epriestley
Maniphest Tasks: T4589
Differential Revision: https://secure.phabricator.com/D8498
Summary: Ref T4659. Because we JOIN, tasks with no value are filtered out. Instead, LEFT JOIN.
Test Plan: Issued an "Order by" and got all applicable tasks. Adjusted values and saw order change.
Reviewers: btrahan, hach-que
Reviewed By: hach-que
Subscribers: epriestley
Maniphest Tasks: T4659
Differential Revision: https://secure.phabricator.com/D10119
Summary:
Resolves T4659. This implements support for sorting tasks by custom fields.
Some of this feels hacky in the way it's hooked up to the Maniphest search engine and task query.
Test Plan: Queryed on a custom date field, with a small page size, and moved back and forth through the result set.
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: epriestley, Korvin
Maniphest Tasks: T4659
Differential Revision: https://secure.phabricator.com/D10106
Summary: Initially the change is aimed to solve issue with line breaks being lost in projects descriptions. But it is done in a general place so line breaks behavior is more consistent all over the place.
Test Plan:
- Write a multiline description of the project, using single \n for line breakers.
- View project details in project/view/X?
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: epriestley, Korvin
Differential Revision: https://secure.phabricator.com/D10014
Summary: Fixes T5102. Bumps the version to clear caches and documents the `#` headers.
Test Plan: Read documentation.
Reviewers: asherkin, btrahan, chad
Reviewed By: chad
Subscribers: epriestley
Maniphest Tasks: T5102
Differential Revision: https://secure.phabricator.com/D10046
Summary:
Ref T4420. This was a performance hack introduced long ago to make typeaheads for users a little cheaper. The idea was that you could load some of an object's columns and skip other ones.
We now always load users on demand, so the cost of loading the whole objects is very small. No other use cases ever arose for this, and it seems unlikely that they will in the future. Remove it all.
Test Plan:
- Grepped for `CONFIG_PARTIAL_OBJECTS`.
- Grepped for `dirtyFields`.
- Grepped for `missingFields`.
- Grepped for `resetDirtyFields`.
- Grepped for `loadColumns`.
- Grepped for `loadColumnsWhere`.
- Grepped for `loadRawDataWhere`.
- Loaded and saved some lisk objects.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T4420
Differential Revision: https://secure.phabricator.com/D9895
Summary: Ref T5245. Earlier changes in this stack introduced new edge strings, provide English translations.
Test Plan: This is hard to test exhastively, but I poked around the UI and saw some of these rendering correctly. If I messed anything up it's trivial to fix when someone spots it.
Reviewers: joshuaspence, chad
Reviewed By: joshuaspence, chad
Subscribers: epriestley
Maniphest Tasks: T5245
Differential Revision: https://secure.phabricator.com/D9920
Summary: Ref T5245. Updates the project/object edge to use a modern class definition. Moves further toward real edges.
Test Plan: Added projects to some objects, viewed transactions in transaction record.
Reviewers: chad, btrahan, joshuaspence
Reviewed By: joshuaspence
Subscribers: epriestley
Maniphest Tasks: T5245
Differential Revision: https://secure.phabricator.com/D9849
Summary: Ref T5245. With work elsewhere (notably, D9839) we can remove this TODO and use real transactions.
Test Plan: Pushed a `closes Txxx` commit and got a close + transaction.
Reviewers: chad, btrahan, joshuaspence
Reviewed By: joshuaspence
Subscribers: epriestley
Maniphest Tasks: T5245
Differential Revision: https://secure.phabricator.com/D9848
Summary:
Ref T5245. These were a bad idea.
We no longer need actors for edge edits either, so remove those. Generally, edges have fit into the policy model as pure/low-level infrastructure, and they do not have any policy or capability information in and of themselves.
Test Plan: `grep`
Reviewers: chad, btrahan, joshuaspence
Reviewed By: joshuaspence
Subscribers: epriestley
Maniphest Tasks: T5245
Differential Revision: https://secure.phabricator.com/D9840
Summary:
Ref T5245. See some discussion in D9838.
When we attach object A to object B, we'd like to write transactions on both sides but only write the actual edges once.
To do this, allow edge types to `shouldWriteInverseTransactions()`. When an edge type opts into this, have editors apply the inverse transactions before writing the edge. These inverse transactions don't actually apply effects, they just show up in the transaction log.
Test Plan: Attached and detached revisions from tasks, saw transactions appear on both sides of the operation.
Reviewers: chad, btrahan, joshuaspence
Reviewed By: btrahan, joshuaspence
Subscribers: epriestley
Maniphest Tasks: T5245
Differential Revision: https://secure.phabricator.com/D9839
Summary:
Ref T5245. A very long time ago I had this terrible idea that we'd let objects react to edges being added and insert transactions in response.
This turned out to be a clearly bad idea very quickly, for like 15 different reasons. A big issue is that it inverts the responsibilities of editors. It's also just clumsy and messy.
We now have `PhabricatorApplicationTransactionInterface` instead, which mostly provides a cleaner way to deal with this.
Implement `PhabricatorApplicationTransactionInterface`, implicitly moving all the attach actions (task/task, task/revision, task/commit, task/mock) to proper edge transactions.
The cost of this is that the inverse edges don't write transactions -- if you attach an object to another object, only the object you were acting on posts a transaction record. This is sort of buggy anyway already. I'll fix this in the next diff.
Test Plan: Attached tasks, revisions and mocks to a task, then detached them.
Reviewers: chad, btrahan, joshuaspence
Reviewed By: joshuaspence
Subscribers: epriestley
Maniphest Tasks: T5245
Differential Revision: https://secure.phabricator.com/D9838
Summary:
Ref T5245. I want to add a new capability to edge types, which is a good opportunity to move away from `PhabricatorEdgeConfig`, which isn't modular.
This is basically the same as the modularization of PHID types, which has worked well. Add `PhabricatorEdgeType` and provide an adaption layer for the existing code.
This has no runtime changes, except the fixed edge constant.
Test Plan: Ran `var_dump(PhabricatorEdgeType::getAllTypes())` and got reasonable looking output.
Reviewers: chad, btrahan, joshuaspence
Reviewed By: joshuaspence
Subscribers: epriestley
Maniphest Tasks: T5245
Differential Revision: https://secure.phabricator.com/D9837
Summary: Add a new line for clarity.
Test Plan: Inspected `./resources/celerity/map.php`.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: epriestley, Korvin
Differential Revision: https://secure.phabricator.com/D9931
Summary: `phutil_var_export` only accepts a single parameter. This bug was introduced in D9926.
Test Plan: Eye-ball it.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: epriestley, Korvin
Differential Revision: https://secure.phabricator.com/D9930
Summary: Fixes T5336. Currently, `PhabricatorWorkerLeaseQuery` is basically FIFO. It makes more sense for the queue to be a priority-queue, and to assign higher priorities to alerts (email and SMS).
Test Plan: Created dummy tasks in the queue (with different priorities). Verified that the priority field was set correctly in the DB and that the priority was shown on the `/daemon/` page. Started a `PhabricatorTaskmasterDaemon` and verified that the higher priority tasks were executed before lower priority tasks.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: epriestley, Korvin
Maniphest Tasks: T5336
Differential Revision: https://secure.phabricator.com/D9871
Summary: I'm pretty sure that `@group` annotations are useless now... see D9855. Also fixed various other minor issues.
Test Plan: Eye-ball it.
Reviewers: #blessed_reviewers, epriestley, chad
Reviewed By: #blessed_reviewers, epriestley
Subscribers: epriestley, Korvin, hach-que
Differential Revision: https://secure.phabricator.com/D9859
Summary: The monospaced rule should still have higher precedence than these
rules, so use flat text tests to cover some rule interactions.
Auditors: btrahan
Summary: Remarkup rules can not safely use arbitrary text in tag attributes,
because it may include tokens which are later replaced. Precedence rules
should prevent this in general. Use flat text assertions and adjust precedence
rules in cases where they may not prevent tokens from appearing in attributes.
Auditors: btrahan
Summary:
Ref T3116. Add a Herald action "Require legal signatures" which requires revision authors to accept legal agreements before their revisions can be accepted.
- Herald will check which documents the author has signed, and trigger a "you have to sign X, Y, Z" for other documents.
- If the author has already signed everything, we don't spam the revision -- basically, this only triggers when signatures are missing.
- The UI will show which documents must be signed and warn that the revision can't be accepted until they're completed.
- Users aren't allowed to "Accept" the revision until documents are cleared.
Fixes T1157. The original install making the request (Hive) no longer uses Phabricator, and this satisfies our requirements.
Test Plan:
- Added a Herald rule.
- Created a revision, saw the rule trigger.
- Viewed as author and non-author, saw field UI (generic for non-author, specific for author), transaction UI, and accept-warning UI.
- Tried to accept revision.
- Signed document, saw UI update. Note that signatures don't currently //push// an update to the revision, but could eventually (like blocking tasks work).
- Accepted revision.
- Created another revision, saw rules not add the document (since it's already signed, this is the "no spam" case).
Reviewers: btrahan, chad
Reviewed By: chad
Subscribers: asherkin, epriestley
Maniphest Tasks: T1157, T3116
Differential Revision: https://secure.phabricator.com/D9771
Summary:
Ref T3116. Allow documents to be queried for ones the viewer has signed, and make this the default view.
This also relaxes the versioning stuff a little bit, and stops invalidating signatures on older versions of documents. While I think we should do that eventually, it should be more explicit and have better coordination in the UI. For now, we'll track and show older signatures, but not invalidate them.
I imagine eventually differentiating between "minor edits" (typo / link fixes, for example) and major edits which actually require re-signature.
Test Plan: {F171650}
Reviewers: chad
Reviewed By: chad
Subscribers: epriestley
Maniphest Tasks: T3116
Differential Revision: https://secure.phabricator.com/D9769
Summary: Lightens up the border a little on obj tags
Test Plan: photoshop, homepage
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: epriestley, Korvin
Differential Revision: https://secure.phabricator.com/D9759
Summary: Fixes T5497. Scope these down a little bit so they don't bleed into `{W...}` embeds and such.
Test Plan:
- Viewed a Legalpad document with headers, monospaced stuff, and lists. Looked the same before/after.
- Viewed a comment with headers, monospace, and lists. Looked the same before/after.
- Viewed a `{W..}` embed, now looks sane.
{F171052}
Reviewers: chad
Reviewed By: chad
Subscribers: epriestley
Maniphest Tasks: T5497
Differential Revision: https://secure.phabricator.com/D9757
Summary: The rest of this code works if we hand off `array()`, and fataling here, while more correct, is harder for users to get out of (they have to go manually remove files) and not obvious.
Test Plan: Corrupted pid file and ran `phd stop`.
Reviewers: joshuaspence
Reviewed By: joshuaspence
Subscribers: epriestley
Differential Revision: https://secure.phabricator.com/D9749
Summary: Provides a base set of shaded object tags for use in Phabricator.
Test Plan:
Lots of Photoshop and Chrome.
{F170252, size=full}
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: epriestley, Korvin
Differential Revision: https://secure.phabricator.com/D9737
Summary: Moves PhabricatorActionHeaderView to PHUIActionHeaderView, adds Red, Green, and Violet colors and extend ObjectBox to take colors and action headers.
Test Plan:
Tested new Welcome layout as well as UIExamples, Workboards, and Hovercards
{F169669}
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: epriestley, Korvin
Differential Revision: https://secure.phabricator.com/D9707
Summary: Constructing tables manually just isn't fun.
Test Plan:
```
./bin/storage status
phabricator:db.audit Applied db audit
phabricator:db.calendar Applied db calendar
phabricator:db.chatlog Applied db chatlog
phabricator:db.conduit Applied db conduit
phabricator:db.countdown Applied db countdown
phabricator:db.daemon Applied db daemon
phabricator:db.differential Applied db differential
phabricator:db.draft Applied db draft
phabricator:db.drydock Applied db drydock
phabricator:db.feed Applied db feed
phabricator:db.file Applied db file
phabricator:db.flag Applied db flag
phabricator:db.harbormaster Applied db harbormaster
...
```
This probably isn't ready to land yet, we should fix `PhutilConsoleTable` to truncate columns which would otherwise cause overflow.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: epriestley, Korvin
Differential Revision: https://secure.phabricator.com/D9604
Summary: Ref T5179. Ref T4045. Ref T832. We can now write non-utf8 hunks into the database, so try to do more reasonable things with them in the UI.
Test Plan: (See screenshots...)
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T832, T4045, T5179
Differential Revision: https://secure.phabricator.com/D9294
Summary: Convert `./bin/mail` and a`./bin/sms` to use `PhutilConsoleTable` for formatting output.
Test Plan: I don't actually have mail and SMS setup on my dev box, but this is a pretty straightforward change.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: epriestley, Korvin
Differential Revision: https://secure.phabricator.com/D9621
Summary:
Ref T4209. Unifies the local (`./bin/phd status`) and global (`./bin/phd status --all`) view into a single table. This generally makes it easy to administer daemons running across multiple hosts.
Depends on D9606.
Test Plan:
```
> sudo ./bin/phd status
ID Host PID Started Daemon Arguments
38 localhost 2282 Jun 18 2014, 7:52:56 AM PhabricatorRepositoryPullLocalDaemon
39 localhost 2289 Jun 18 2014, 7:52:57 AM PhabricatorGarbageCollectorDaemon
40 localhost 2294 Jun 18 2014, 7:52:57 AM PhabricatorTaskmasterDaemon
41 localhost 2314 Jun 18 2014, 7:52:58 AM PhabricatorTaskmasterDaemon
42 localhost 2319 Jun 18 2014, 7:52:59 AM PhabricatorTaskmasterDaemon
43 localhost 2328 Jun 18 2014, 7:53:00 AM PhabricatorTaskmasterDaemon
44 localhost 2354 Jun 18 2014, 7:53:08 AM PhabricatorRepositoryPullLocalDaemon X --not Y
```
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: epriestley, Korvin
Maniphest Tasks: T4209
Differential Revision: https://secure.phabricator.com/D9607
Summary: Ref T4209. Currently, `./bin/phd status` prints a table showing the daemons that are executing on the current host. It would be useful to be able to conventiently query the daemons running across all hosts. This would also (theoretically) make it possible to conditionally start daemons on a host depending upon the current state and on the daemons running on other hosts.
Test Plan:
```
> ./bin/phd status --all
ID Host PID Started Daemon Arguments
18 phabricator 6969 Jun 12 2014, 4:44:22 PM PhabricatorTaskmasterDaemon
17 phabricator 6961 Jun 12 2014, 4:44:19 PM PhabricatorTaskmasterDaemon
16 phabricator 6955 Jun 12 2014, 4:44:15 PM PhabricatorTaskmasterDaemon
15 phabricator 6950 Jun 12 2014, 4:44:14 PM PhabricatorTaskmasterDaemon
14 phabricator 6936 Jun 12 2014, 4:44:13 PM PhabricatorGarbageCollectorDaemon
13 phabricator 6931 Jun 12 2014, 4:44:12 PM PhabricatorRepositoryPullLocalDaemon
```
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: epriestley, Korvin
Maniphest Tasks: T4209
Differential Revision: https://secure.phabricator.com/D9497
Summary:
Full screen is a little foobar so disabling it for inline comments
Fixes T5272
Test Plan:
View inline comment after change, make sure full screen option
has gone.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: epriestley, Korvin
Maniphest Tasks: T5272
Differential Revision: https://secure.phabricator.com/D9579
Summary:
Ref T4986. Instead of requiring you to know engine class names and copy/paste URLs, provide select dropdowns that use SCARY JAVASCRIPT to do magical things.
I think this is mostly reasonable, the only issue is that it's hard to create a panel out of a completely ad-hoc query (you'd have to save it, then create a panel out of the saved query, then remove the saved query). Once we develop T5307 we can do a better job of this.
Test Plan: See screenshots.
Reviewers: chad
Reviewed By: chad
Subscribers: epriestley
Maniphest Tasks: T4986
Differential Revision: https://secure.phabricator.com/D9572
Summary: Fixes T4729. This form is a little fluff, but we show it in the URI when you click an anchor on the page, and doing so seems desirable. I think it's reasonable to support this form, given that it appears in the URI.
Test Plan: Wrote some stuff like `M60`, `M60/71`, `M60/72/`, `M60/73/#13` and saw it all get picked up and rendered/linked properly.
Reviewers: chad
Reviewed By: chad
Subscribers: epriestley
Maniphest Tasks: T4729
Differential Revision: https://secure.phabricator.com/D9555
Summary: Sometimes, the `PhabricatorInfrastructureTestCase` //would// fail, but we don't run it with `arc unit` because of the directory structure. See D9556 for an example.
Test Plan: This is essentially the same as D9557.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: epriestley, Korvin
Differential Revision: https://secure.phabricator.com/D9559
Summary:
Ref T5359. When users upload non-image file types (PDFs, text files, whatever), Pholio currently chokes in a few places. Make most of these behaviors more reasonable:
- Provide thumbs in the required sizes.
- Predict the thumb size of these files correctly.
- Disable inline comments.
- Make "View Fullsize" and "Download" into buttons. These mostly-work. Download should probaly really download, but CSRF on forms is a bit of a pain right now.
Test Plan: See screenshots.
Reviewers: chad
Reviewed By: chad
Subscribers: epriestley
Maniphest Tasks: T5359
Differential Revision: https://secure.phabricator.com/D9548
Summary: This ensure that Aphlict is always served on the primary / production domain, because the alternate file domain is intended to prevent Flash execution.
Test Plan: Confirmed this fixes the issue by visiting https://code.redpointsoftware.com.au/ and seeing notification messages.
Reviewers: joshuaspence, #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: epriestley, Korvin
Differential Revision: https://secure.phabricator.com/D9511
Summary: This class has been deprecated for a while now (see rP0a8b0d1392bd79b4e88fbf910b176c960d57b4b4). It should be safe to remove.
Test Plan: N/A
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: epriestley, Korvin
Differential Revision: https://secure.phabricator.com/D9467
Summary: This class has been deprecated for a while now (see rPdad7c65bf56384480be7c18e02fdc01ea67cf1ff). It should be safe to remove.
Test Plan: N/A
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: epriestley, Korvin
Differential Revision: https://secure.phabricator.com/D9468
Summary: This class has been deprecated for a while (see rP574bc3ba31cca2767bafe7844d7f854d90d6be1c). It should be safe to remove now.
Test Plan: N/A
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: epriestley, Korvin
Differential Revision: https://secure.phabricator.com/D9469
Summary: Applied some more linter fixes that I previously missed because my global `arc` install was out-of-date.
Test Plan: Will run `arc unit` on another host.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: epriestley, Korvin
Differential Revision: https://secure.phabricator.com/D9443
Summary: Ran `arc lint --apply-patches --everything` over rP, mainly to change double quotes to single quotes where appropriate. These changes also validate that the `ArcanistXHPASTLinter::LINT_DOUBLE_QUOTE` rule is working as expected.
Test Plan: Eyeballed it.
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: epriestley, Korvin, hach-que
Differential Revision: https://secure.phabricator.com/D9431
Summary:
This currently uses a hard-coded relative path, but should not, especially after D9401.
The major effect of this is that updated .swf files might not be served properly, and we were at the whims of the server configuration for caching/versioning behavior.
Test Plan: Enabled debug notifications, saw .swf load through Celerity.
Reviewers: joshuaspence
Reviewed By: joshuaspence
Subscribers: epriestley
Differential Revision: https://secure.phabricator.com/D9421
Summary:
Replaces the icons with fonts from FontAwesome. Up in the air about the meme icon. Thoughts?
Also removed the second fullscreen/normal state. Seems obvious what it does, but assume someone complained previously?
Test Plan:
Tested all the icon states and made sure they still worked. Test fullscreen and help.
{F163485}
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: epriestley, Korvin
Differential Revision: https://secure.phabricator.com/D9385
Summary: Ref T2628. This makes Transactions understand objects that can have project relationships, extract project mentions, and handle watching.
Test Plan: See next diff.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T2628
Differential Revision: https://secure.phabricator.com/D9340
Summary: Builds a consistent 'selected, hover' state slightly darker than selected states.
Test Plan: Tested Conpherence, Sidenavs
Reviewers: epriestley
Subscribers: epriestley, Korvin
Differential Revision: https://secure.phabricator.com/D9345
Summary: Adds back the power icon
Test Plan: Logged out of local instance, saw icon appear. Click login icon. Logged in. Ate a toast sandwich.
Reviewers: epriestley
Subscribers: epriestley, Korvin
Differential Revision: https://secure.phabricator.com/D9336
Summary: Adds more consistent colors and spacing to notifications, conpherence dropdowns, search dropdowns, and typeaheads.
Test Plan: Tested Notifications, menu and page. Conpherence, menu and page, Search, and Typeaheads.
Reviewers: epriestley
Subscribers: epriestley, Korvin
Differential Revision: https://secure.phabricator.com/D9313
Summary: Removes lightblue app icons, moves the menu ones to menu sprite. Minor CSS updates to apps nav.
Test Plan: Test all sm icons work in new nav, test apps nav.
Reviewers: epriestley, btrahan
Subscribers: epriestley, Korvin, hach-que
Differential Revision: https://secure.phabricator.com/D9302
Summary: Fixes T5021, UI labels for the fields, "Edit Dependencies" in the action list, transaction strings ("added dependent tasks", etc), UI strings in the dependencies dialog (title/submit/etc)
Test Plan: Open task, edit blocks, dialog should have new term, task history should show "blocks" instead of "dependencies"
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: epriestley, Korvin
Maniphest Tasks: T5021
Differential Revision: https://secure.phabricator.com/D9270
Summary:
Ref T4657. Right now, you have to muck with `events.listeners` to install listeners. Instead, automatically install all subclasses of AutoEventListener.
Primarily, this makes it easier to resolve requests with "drop this file in `src/extensions/`, no warranty", which seems to have worked well so far in resolving things like custom remarkup rules, etc.
Test Plan:
- Added such a listener, had it autoregister.
- Clicked around and saw the effects of normal listeners.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T4657
Differential Revision: https://secure.phabricator.com/D9262
Summary:
Fixes T4021. Chooses to keep a "primary" slug based off the name - including all that lovely logic - and allow the user to specify "additional" slugs. Expose these as "hashtags" to the user.
Sets us up for a fun diff where we can delete all the Project => Phriction automagicalness. In terms of this diff, see the TODOs i added.
Test Plan:
added a primary slug as an additional slug - got an error. added a slug in use on another project - got an error. added multiple good slugs and they worked. removed slugs and it worked. made some remark using multiple new slugs and they all linked to the correct project
ran epriestley's case
- Create project "A".
- Give it additional slug "B".
- Try to create project "B".
and i got a nice error about hashtag collision
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: epriestley, Korvin
Maniphest Tasks: T4021
Differential Revision: https://secure.phabricator.com/D9250
Summary:
Fixes T5094. In some cases we do slightly expensive transformations to resources (inlining images, replacing URIs, building packages). We can throw cache in front of them easily since URIs are already permanently associated with a single resource.
Also browse around and move some CSS/JS into packages.
Test Plan:
Added logging to verify the caches are working, saw moderately improved performance.
Browsed around looking at resources tab in developer console, saw fewer total requests.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T5094
Differential Revision: https://secure.phabricator.com/D9175
Summary: placeholder text is pretty useful.
Test Plan: placeholder text is pretty useful. also fully supports not breaking everything.
Reviewers: chad, #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: epriestley, Korvin
Differential Revision: https://secure.phabricator.com/D9223
Summary: Fixes T4735. When running `./bin/phd`, show daemon arguments.
Test Plan:
```
./bin/phd status
PID Started Daemon Arguments
12711 May 20 2014, 9:02:52 AM PhabricatorRepositoryPullLocalDaemon []
12716 May 20 2014, 9:02:52 AM PhabricatorGarbageCollectorDaemon []
12733 May 20 2014, 9:02:53 AM PhabricatorTaskmasterDaemon []
12768 May 20 2014, 9:02:53 AM PhabricatorTaskmasterDaemon []
12775 May 20 2014, 9:02:53 AM PhabricatorTaskmasterDaemon []
12780 May 20 2014, 9:02:54 AM PhabricatorTaskmasterDaemon []
12838 May 20 2014, 9:02:54 AM PhabricatorFactDaemon []
13436 May 20 2014, 9:03:23 AM PhabricatorRepositoryPullLocalDaemon ["X","--not","Y"]
```
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: epriestley, Korvin
Maniphest Tasks: T4735
Differential Revision: https://secure.phabricator.com/D9208
Summary: Ref T4029. When checking the view policy of a document, require the viewer to also be able to see all of the ancestors.
Test Plan:
- Hard-coded `/x/y/` to "no one".
- Checked that `/x/y/` is not visible.
- Checked that `/x/y/z/` is not visible.
- Checked that `/x/`, `/x/q/`, etc., are still visible.
- Tested project pages and sub-pages for project visibility.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T4029
Differential Revision: https://secure.phabricator.com/D9199
Summary:
Ref T4967. Adds a "Watch" relationship to projects, which is stronger than member/subscribed.
Specifically, when a task is tagged with a project, we'll include all project watchers in the email/notifications. Normally we don't include projects unless they're explicitly CC'd, or have some other active role in the object (like being a reviewer or auditor).
This allows you to closely follow a project without needing to write a Herald rule for every project you care about.
Test Plan:
- Watched/unwatched a project.
- Tested the watch/subscribe/member relationships:
- Watching implies subscribe.
- Joining implies subscribe.
- Leaving implies unsubscribe + unwatch.
- You can't unsubscribe until you unwatch (slightly better would be unsubscribe implies unwatch, but this is a bit tricky).
- Watched a project, then recevied email about a tagged task without otherwise being involved.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T4967
Differential Revision: https://secure.phabricator.com/D9185
Summary:
Updates policy, headers, typeaheads to FA over policy icons
Need advice - can't seem to place where icons come from on Typeahead? Wrong icons and wrong colors.... it is late
Test Plan:
- grepped for SPRITE_STATUS
- grepped for sprite-status
- grepped for setStatus for headers
- grepped individual icons names
Browsed numerous places, checked new dropdowns, see pudgy people.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: epriestley, Korvin
Maniphest Tasks: T4739
Differential Revision: https://secure.phabricator.com/D9179
Summary: In general these are fairly readable, but if not it cleans up on hover (and hover card).
Test Plan: tested a closed task in my sandbox
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: epriestley, Korvin
Differential Revision: https://secure.phabricator.com/D9158
Summary:
D9153 fixed half of this, but exposed another issue, which is that we don't actually serve ".eot" and ".ttf" through Celerity right now.
Make sure we include them in the routes.
Test Plan:
- Downloaded CSS, JS, TTF, EOT, WOFF, JPG, etc., through Celerity.
Reviewers: btrahan, chad
Reviewed By: chad
Subscribers: epriestley
Differential Revision: https://secure.phabricator.com/D9154
Summary:
See <6a45b7e670>
These URIs have "?hack=iefix#ieieielol" on them, which the parser doesn't recognize as a known resource, so it errs on the side of caution by not rewriting.
Instead, strip this bit off, attempt to rewrite, then put it back on.
Test Plan: Loaded `font-awesome.css` locally and saw properly rewritten URIs.
Reviewers: btrahan, chad
Reviewed By: chad
Subscribers: epriestley
Differential Revision: https://secure.phabricator.com/D9153
Summary:
Fixes T5069. T2222 mostly-intentionally stopped emitting these.
My sense is that users generally find event listeners (or, really, writing PHP at all) much less preferable to things like Herald rules or HTTP hooks. This is generally good, since those things are way easier to maintain, so I plan to continue moving away from events in cases where we have reasonable alternatives.
We also generally have more and better alternatives now than when these were written.
Test Plan: `grep`
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T5069
Differential Revision: https://secure.phabricator.com/D9151
Summary: Someone stole my bot's name, so the bot couldn't (re)connect. This tries adding some text to the name if it encounters a 433 'nick in use' error
Test Plan: Started a ton of bots: F154744
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: epriestley, Korvin
Differential Revision: https://secure.phabricator.com/D9123
Summary: Did a more exhaustive grep on setIcon and found 99.9% of the icons.
Test Plan: I verified icon names on UIExamples, but unable to test some of the more complex flows visually. Mostly a read and replace.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: epriestley, Korvin, hach-que
Differential Revision: https://secure.phabricator.com/D9088
Summary: Remove white app icons, no longer in use as far as grep/memory serve. These were for list hover states.
Test Plan: Rebuild sprites, celerity. Grep for appIcon use (only feed). Verify all action lists are driven by FontAwesome.
Reviewers: btrahan, epriestley
Reviewed By: epriestley
Subscribers: epriestley, Korvin, hach-que
Differential Revision: https://secure.phabricator.com/D9078
Summary: Ref T2683. Normally not a big deal, but if a readme has some codeblocks missing the cache can slow things down.
Test Plan:
- Verified we hit the cache.
- Verified TOC still works.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T5028, T2683
Differential Revision: https://secure.phabricator.com/D9049
Summary: The removes the sprite sheet 'icons' and replaces it with FontAwesome fonts.
Test Plan:
- Grep for SPRITE_ICONS and replace
- Grep for sprite-icons and replace
- Grep for PhabricatorActionList and choose all new icons
- Grep for Crumbs and fix icons
- Test/Replace PHUIList Icon support
- Test/Replace ObjectList Icon support (foot, epoch, etc)
- Browse as many pages as I could get to
- Remove sprite-icons and move remarkup to own sheet
- Review this diff in Differential
Reviewers: btrahan, epriestley
Reviewed By: epriestley
Subscribers: epriestley, Korvin, hach-que
Differential Revision: https://secure.phabricator.com/D9052
Summary:
Also fix a few other minor issues:
- Use lint config.
- Fix a method signature from `arc unit --everything` (unrelated).
- Add a javelin doc.
Test Plan: Ran `arc lint`, `arc unit`, `arc linters`.
Reviewers: btrahan, joshuaspence
Reviewed By: joshuaspence
Subscribers: epriestley
Differential Revision: https://secure.phabricator.com/D9072
Summary:
Ref T2039. This diff is the equivalent to D9057, but for rP.
Depends on D9066.
Test Plan: Ran `arc lint` and ensure it doesn't complain about the `.arclint` file.
Reviewers: #blessed_reviewers, epriestley
Subscribers: epriestley, Korvin
Maniphest Tasks: T2039
Differential Revision: https://secure.phabricator.com/D9064
Summary:
Provides a working SMS implementation with support for Twilio.
This version doesn't really retry if we get any gruff at all. Future versions should retry.
Test Plan: used bin/sms to send messages and look at them.
Reviewers: chad, epriestley
Reviewed By: epriestley
Subscribers: aurelijus, epriestley, Korvin
Maniphest Tasks: T920
Differential Revision: https://secure.phabricator.com/D8930
Summary: Ref D8930. My "send test" for SMS was failing before this patch, and now it works nicely.
Test Plan: Used new code in D8930 that uses $this->queueTask() to get some work done and it got done in process
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: epriestley, Korvin
Differential Revision: https://secure.phabricator.com/D9018
Summary: Ref T4119. Adds the block rule and makes a faint effort at CSS.
Test Plan: See D8953 for a screenshot.
Reviewers: chad, btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T4119
Differential Revision: https://secure.phabricator.com/D8955
Summary:
Ref T4398. This prompts users for multi-factor auth on login.
Roughly, this introduces the idea of "partial" sessions, which we haven't finished constructing yet. In practice, this means the session has made it through primary auth but not through multi-factor auth. Add a workflow for bringing a partial session up to a full one.
Test Plan:
- Used Conduit.
- Logged in as multi-factor user.
- Logged in as no-factor user.
- Tried to do non-login-things with a partial session.
- Reviewed account activity logs.
{F149295}
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T4398
Differential Revision: https://secure.phabricator.com/D8922
Summary:
Ref T4843. This adds support to `javelin_tag()` for an `aural` attribute. When specified, `true` values mean "this content is aural-only", while `false` values mean "this content is not aural".
- I've attempted to find the best modern approaches for marking this content, but the `aural` attribute should let us change the mechanism later.
- Make the "beta" markers on application navigation visual only (see T4843). This information is of very low importance, the application navigation is accessed frequently, and the information is available on the application list.
- Partially convert the main navigation. This is mostly to test things, since I want to get more concrete feedback about approaches here.
- Add a `?__aural__=1` attribute, which renders the page with aural-only elements visible and visual-only elements colored.
Test Plan: {F146476}
Reviewers: btrahan, scp, chad
Reviewed By: chad
Subscribers: aklapper, qgil, epriestley
Maniphest Tasks: T4843
Differential Revision: https://secure.phabricator.com/D8830
Summary:
Ref T3583. Adds edges, query relationships, etc. Lots of debugging/temporary UI.
My general intent here is to use edges to track where panels appear, and then put additional data on the dashboard itself to control layout, positioning, etc.
Dashboards don't actually render yet so this is still pretty boring.
Test Plan:
{F149175}
{F149176}
{F149177}
Reviewers: chad, btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T3583
Differential Revision: https://secure.phabricator.com/D8916
Summary:
Moderize Inline Comment Display
- Use standard colors
- Better display with/without comment
- OMG Icons
Test Plan:
{F148256}
Test with and without main comment, test with many for few comments on 1-3 files.
Reviewers: epriestley, btrahan
Reviewed By: btrahan
Subscribers: epriestley, Korvin
Differential Revision: https://secure.phabricator.com/D8885
Summary:
In some applications, using `{V2}` syntax to embed a vote throws. The chain of causality looks like this:
- We try to render a `phabricator_form()`.
- This requires a CSRF token.
- We look for a CSRF token on the user.
- It's an omnipotent user with no token, so everything fails.
To resolve this, make sure we always pass the real user in.
Test Plan:
- Lots of `grep`.
- Made a Differential comment with `{V2}`.
- Made a Diffusion comment with `{V2}`.
- Made a Maniphest comment with `{V2}`.
- Replied to a Conpherence thread with `{V2}`.
- Created a Conpherence thread with `{V2}`.
- Used Conduit to update a Conpherence thread with `{V2}`.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley, lkassianik
Differential Revision: https://secure.phabricator.com/D8849
Summary: Ref T3718. Move from unbatched / ad-hoc loading to standard stuff for handles.
Test Plan: Looked at some requests and saw no changes.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T3718
Differential Revision: https://secure.phabricator.com/D8810
Summary: This `%d` should be a `%s`, since the `PhutilNumber` value may get formatted according to locale settings.
Test Plan: will make @zeeg
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley, zeeg
Differential Revision: https://secure.phabricator.com/D8814
Summary:
PHP 5.5 specifies constant PASSWORD_BCRYPT should be used in password_hash()
instead of CRYPT_BLOWFISH. Using CRYPT_BLOWFISH is not supported in either PHP
or HHVM. This constant breaks Username / Password authentication.
Test Plan:
Login using Username/Password with bcrypt hash. Before applying the patch,
No matter what password entered, it will always fail authentication. After this
patch, user should be able to login with bcrypt hash.
Reviewers: btrahan, epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: epriestley, Korvin
Differential Revision: https://secure.phabricator.com/D8808
Summary: This adds in the Glyphicons Halflings Font/Iconset as an option for PHUIIconView along with a standard set of 10 colors. This will be a replacement for the standard action icon set in upcoming diffs, as well as obviously give us more flexibility, less KB, and less design resource time managing images.
Test Plan: UIExamples, Diviner
Reviewers: btrahan, epriestley
Reviewed By: epriestley
Subscribers: epriestley, Korvin
Differential Revision: https://secure.phabricator.com/D8798
Summary:
For Harbormaster tasks which want to poll or wait, this lets them say "try again a little later" without having to sleep and hold a queue slot.
This is basically the same as failing, except that we don't increment the failure counter. Instead, we just set the current lease to the correct length and then exit. The task will be retried after the lease expires.
Test Plan: Using both `bin/harbormaster` and `phd debug taskmaster`, ran a lot of waiting tasks through the queue, faking them to either yield or not yield in a controlled manner. The queue responded as expected, yielding tasks appropraitely and retrying them later.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Differential Revision: https://secure.phabricator.com/D8792
Summary:
Fixes T4802. For context, see T1921.
Originally (in T1921), a developer ran into an issue where rendering `phabricator_form()` with an absolute URI confusingly dropped CSRF tokens, and it wasn't obvious why. This is a security measure, but at the time it wasn't very clear how all the pieces fit together. To make it more clear, we:
# expanded the exception text in developer mode to include a description of this issue; and
# added an exception in developer mode when rendering a form like this.
However, (2) causes some undesirable interactions for file downloads. In particular, if:
- developer mode is on; and
- there's no alternate file domain configured; and
- you try to download a file...
...we produce CDN URIs that are fully-qualified, and you get the exception from (2) above.
This is kind of a mess, and producing fully-qualified CDN URIs in all cases is simple and clear and desirable. To resolve this, just revert (2). We still have the clarification from (1) above and this hasn't caused further issues, so I think that's sufficient. This is a rare issue anyway and not particularly serious or error prone (at worst, a bit confusing and annoying, but hopefully easy to understand and resolve after the changes in (1)).
Test Plan: With develper mode and no alternate file domain, downloaded files from Files.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T4802
Differential Revision: https://secure.phabricator.com/D8776
Summary:
See discussion in D8773. Three small adjustments which should help prevent this kind of issue:
- When queueing followup tasks, hold them on the worker until we finish the task, then queue them only if the work was successful.
- Increase the default lease time from 60 seconds to 2 hours. Although most tasks finish in far fewer than 60 seconds, the daemons are generally stable nowadays and these short leases don't serve much of a purpose. I think they also date from an era where lease expiry and failure were less clearly distinguished.
- Increase the default wait-after-failure from 60 seconds to 5 minutes. This largely dates from the MetaMTA era, where Facebook ran services with high failure rates and it was appropriate to repeatedly hammer them until things went through. In modern infrastructure, such failures are rare.
Test Plan:
- Verified that tasks queued properly after the main task was updated.
- Verified that leases default to 7200 seconds.
- Intentionally failed a task and verified default 300 second wait before retry.
- Removed all default leases shorter than 7200 seconds (there was only one).
- Checked all the wait before retry implementations for anything much shorter than 5 minutes (they all seem reasonable).
Reviewers: btrahan, sowedance
Reviewed By: sowedance
Subscribers: epriestley
Differential Revision: https://secure.phabricator.com/D8774
Summary:
Fixes T4773. For config settings of type `list<string>`, `set`, or `list<regex>`, the "defaults" table and "examples" aren't always in the same format you should actually use when changing the setting.
This is pretty confusing. Instead, always show the settings in the desired format. For example, if the user should enter a newline-separated list, show them a newline separated list.
Test Plan:
- Grepped for `list<string>`, `list<regex>`, and `'set'`; verified all the config had the right example format (most already did).
- Viewed config settings of various kinds, including custom settings.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T4773
Differential Revision: https://secure.phabricator.com/D8725
Summary:
This adds a system which basically keeps a record of recent actions, who took them, and how many "points" they were worth, like:
epriestley email.add 1 1233989813
epriestley email.add 1 1234298239
epriestley email.add 1 1238293981
We can use this to rate-limit actions by examining how many actions the user has taken in the past hour (i.e., their total score) and comparing that to an allowed limit.
One major thing I want to use this for is to limit the amount of error email we'll send to an email address. A big concern I have with sending more error email is that we'll end up in loops. We have some protections against this in headers already, but hard-limiting the system so it won't send more than a few errors to a particular address per hour should provide a reasonable secondary layer of protection.
This use case (where the "actor" needs to be an email address) is why the table uses strings + hashes instead of PHIDs. For external users, it might be appropriate to rate limit by cookies or IPs, too.
To prove it works, I rate limited adding email addresses. This is a very, very low-risk security thing where a user with an account can enumerate addresses (by checking if they get an error) and sort of spam/annoy people (by adding their address over and over again). Limiting them to 6 actions / hour should satisfy all real users while preventing these behaviors.
Test Plan:
This dialog is uggos but I'll fix that in a sec:
{F137406}
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Differential Revision: https://secure.phabricator.com/D8683
Summary: IE won't load background images in a page that are served with the mimetype "image/jpg" as it only recognises the "image/jpeg" mimetype.
Test Plan: Spent an hour or two going back and forth between Linux (to dev) and Windows (to test) to find the source of this issue, then flipped several tables at IE for being terrible.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: epriestley, Korvin
Differential Revision: https://secure.phabricator.com/D8689
Summary: Ref T418. Fixes T4642. The "changes since last update" and "branch" fields got dropped; restore them in a general, field-driven way.
Test Plan:
- Created a revision, got relevant sections in mail.
- Commented on a revision, got relevant sections in mail.
- Updated a revision, got relevant sections in mail.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: spicyj, epriestley
Maniphest Tasks: T418, T4642
Differential Revision: https://secure.phabricator.com/D8657
Summary: Fixes T4683. This was just a missing method implementation. Also provide a couple of translation things.
Test Plan:
- Created a revision from the command line with a nonempty `JIRA Issues:` line, via `arc diff`.
- Looked at the translation strings.
Reviewers: btrahan, chad
Reviewed By: chad
Subscribers: epriestley
Maniphest Tasks: T4683
Differential Revision: https://secure.phabricator.com/D8656
Summary:
Fixes T4677. Implements a "send an email" pre-receive action, which sends push summaries.
For use cases where features are often pushed as a large number of commits (e.g., checkpoint commits are retained), using commit emails means users get a ton of email. Instead, this allows you to get an email about a push, which summarizes what changed.
Overall, this is basically the same as commit email, but more suitable for some workflows.
Test Plan:
Wrote some rules, then made a bunch of pushes. Got email like this:
{F134929}
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T4677
Differential Revision: https://secure.phabricator.com/D8618
Summary: Ref T4590. Ref T1049. This is primarily intended to support HTTP auth in Harbormaster.
Test Plan: Added a field, edited it, etc.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T4590, T1049
Differential Revision: https://secure.phabricator.com/D8607
Summary:
Ref T1049. Fixes T4602. Moves all the funky field stuff to CustomField. Uses ApplicationTransactions to apply and record edits.
This makes "artifact" fields a little less nice (but still perfectly usable). With D8599, I think they're reasonable overall. We can improve this in the future.
All other field types are better (e.g., fixes weird bugs with "bool", fixes lots of weird behavior around required fields), and this gives us access to many new field types.
Test Plan:
Made a bunch of step edits. Here's an example:
{F133694}
Note that:
- "Required" fields work correctly.
- the transaction record is shown at the bottom of the page.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T4602, T1049
Differential Revision: https://secure.phabricator.com/D8600
Summary: Ref T4663. Ref T4659. Allows "date" fields to be filtered with range parameters.
Test Plan:
- Added a custom "date" field with "search".
- Populated some values.
- Searched for dates using new range filters.
- Combined date search with other searches.
- Ran other searches independently.
- Inspected the generated queries.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: shadowhand, epriestley
Maniphest Tasks: T4659, T4663
Differential Revision: https://secure.phabricator.com/D8598
Summary:
- Point them at the new Diviner.
- Make them a little less cumbersome to write.
Test Plan: Found almost all of these links in the UI and clicked them.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Differential Revision: https://secure.phabricator.com/D8553
Summary:
Fixes T3976. Long ago, some applications used "#comment-5" instead of "#5" for transaction/comment anchors. Now everything (I think?) uses "#5"; this is the style used by ApplicationTransactions.
This might break some very old, explcit `T123#comment-5` links, or off-site links to the `comment-N` anchors, but all that stuff generally got renumbered when we migrated anyway and getting you to the right object is like 95% of the job.
Test Plan: Verified that `T123#5` now links to `#5`.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T3976
Differential Revision: https://secure.phabricator.com/D8542
Summary: Ref T4430. This just deploys it on the property lists. (Help on how to do translations better? I tried a more traditional pht('%s, %s, %s, and %d other(s)') but I think the string lookup assumes the %d comes as the second param or something?)
Test Plan: Made a Maniphest Task with a hojillion subscribers and noted the working dialogue. Also made a Pholio Mock with lots of subscribers and it worked.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: aran, epriestley, Korvin, chad
Maniphest Tasks: T4430
Differential Revision: https://secure.phabricator.com/D8525
Summary: Fixes T4614. These don't do anything bad or dangerous, but generate unusable pages.
Test Plan:
- Added and executed unit tests.
- Tried to create pages like `/../`, `/begin/../end/`, etc.
Reviewers: btrahan, chad
Reviewed By: chad
Subscribers: aran, epriestley
Maniphest Tasks: T4614
Differential Revision: https://secure.phabricator.com/D8535
Summary: Ref T4587. Add an option to automatically generate a keypair, associate the public key, and save the private key.
Test Plan: Generated some keypairs. Hit error conditions, etc.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: aran, epriestley
Maniphest Tasks: T4587
Differential Revision: https://secure.phabricator.com/D8513
Summary:
Ref T4588. Request from @zeeg. Adds a "BRANCHES" field to commit emails, so the branches the commit appears on are shown.
I've implemented this with CustomField, but in a very light way.
Test Plan: Used `scripts/repository/reparse.php --herald` to generate mail, got a BRANCHES section where applicable.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: aran, epriestley, zeeg
Maniphest Tasks: T4588
Differential Revision: https://secure.phabricator.com/D8501
Summary: see title. Fixes T4549.
Test Plan: made a readme that had some headers and observed a nice ToC
Reviewers: chad, epriestley
Reviewed By: epriestley
Subscribers: aran, epriestley, Korvin
Maniphest Tasks: T4549
Differential Revision: https://secure.phabricator.com/D8490
Summary:
Ref T2222.
- Removes `DifferentialTasksAttacher`, which has had no callsites for a very long time.
- Moves `differential.getrevisioncomments` off `DifferentialCommentQuery`.
- Moves Releeph churn field off `DifferentialCommentQuery`.
- Removes dead code in `DifferentialRevisionViewController`.
- Removes `DifferentialException` (no references).
- Removes `DifferentialRevision->loadComments()` (no callsites).
- Removes `DifferentialRevision->loadReviewedBy()` (all callsites updated).
- Removes `DifferentialCommentQuery` (all callsites updated).
Test Plan: Mostly a lot of `grep`.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T2222
Differential Revision: https://secure.phabricator.com/D8476
Summary:
Ref T2222. Ref T3886. Medium term goal is to remove `DifferentialRevisionEditor`.
This temporarily reduces a couple of pieces of functionality unique to the RevisionEditor, but I'm going to go clean those up in the next couple diffs.
Test Plan: Used `arc diff --create` to create several revisions with different data.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T3886, T2222
Differential Revision: https://secure.phabricator.com/D8452
Summary:
There are quite a few tests in Arcanist, libphutil and Phabricator that do something similar to `$this->assertEqual(false, ...)` or `$this->assertEqual(true, ...)`.
This is unnecessarily verbose and it would be cleaner if we had `assertFalse` and `assertTrue` methods.
Test Plan: I contemplated adding a unit test for the `getCallerInfo` method but wasn't sure if it was required / where it should live.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley
CC: Korvin, epriestley, aran
Differential Revision: https://secure.phabricator.com/D8460
Summary: Ref T4570. Add trivial assertions to tests which fail-by-exploding so we can fail tests with no assertions.
Test Plan: Ran `arc unit --everything` with Arcanist patched to fail with no assertions.
Reviewers: leebyron, btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T4570
Differential Revision: https://secure.phabricator.com/D8436
Summary:
- Allow Celerity to map and serve WOFF files.
- Add Source Sans Pro, Source Sans Pro Bold, and the corresponding LICENSE.
- Add a `font-source-sans-pro` resource for the font.
Test Plan:
- Changed body `font-face` to `'Source Sans Pro'`.
- Added `require_celerity_resource('font-source-sans-pro')` in StandardPageView.
Works in Firefox/Chrome/Safari, at least:
{F123296}
{F123297}
{F123298}
Reviewers: btrahan, chad
Reviewed By: chad
CC: chad, aran
Differential Revision: https://secure.phabricator.com/D8430
Summary: Adds support for custom SSL certs in the IRC bot config, same as in .arcconfig
Test Plan: Bot wouldn't connect before. Added this code and corresponding line in bot config, now it does.
Reviewers: #blessed_reviewers, epriestley
Reviewed By: epriestley
CC: Korvin, epriestley, aran
Differential Revision: https://secure.phabricator.com/D8393
Summary:
A few minor fixes:
- When we build a tag with `"meta" => null`, strip the attribute like we do for all other attributes. Previously, we would actually set the metadata to `null`. This happened with the Conpherence form.
- Just respond to the draft request with an empty (but valid) response, instead of building a dialog.
- `PhabricatorShapedRequest` is confusingly named and I should have caught this in review, but the basic shape of it is:
- You make one object.
- You call `trigger()` when stuff changes (e.g., a keystroke).
- It manages making a small number of requests (e.g., one request after the user stops typing for a moment).
- The way it was being used previously would incorrectly send a request for every keystroke.
I think I'm going to simplify `ShapedRequest` and merge it into some larger queue for T430.
Test Plan: Typed some text, no longer saw a flurry of requests. Reloaded page, still saw draft text.
Reviewers: btrahan, chad
Reviewed By: chad
CC: aran, chad
Differential Revision: https://secure.phabricator.com/D8380
Summary:
Ref T2222. Differential has certain "words of power" (like `Ref T123` or `Depends on D345`) which should expand into a separate transaction when they appear anywhere in text.
Currently, they're respected in only some fields. I'm expanding them to work in any remarkup field, including comments and inline comments.
This partially generalizes transaction expansion/extraction in comments. Eventually, I'll probably implement some very soft sort of reference edge for T4036, maybe.
Test Plan: {F119368}
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T2222
Differential Revision: https://secure.phabricator.com/D8369
Summary: Better aligns the text area when leaving an inline comment. Also, phts
Test Plan: reload page, view new padding.
Reviewers: epriestley
Reviewed By: epriestley
CC: Korvin, epriestley, aran
Differential Revision: https://secure.phabricator.com/D8370
Summary:
Ref T2222. Ref T3886. Gets the storage-based fields working.
This requires future changes to actually do anything, all this code is inactive.
Test Plan: {F118863}
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T3886, T2222
Differential Revision: https://secure.phabricator.com/D8357
Summary:
Ref T2222. This isn't complete and doesn't change runtime behavior yet (the new fields are not glued to the interface), but implements many fields.
(The remaining fields have something weird going on with them, for the most part.)
Test Plan:
With additional changes, rendered most fields sensibly:
{F118834}
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T2222
Differential Revision: https://secure.phabricator.com/D8354
Summary:
Ref T2222. Ref T3886. Ref T418. A few changes:
- CustomField can now index into global search.
- Use CustomField fields instead of older custom fields for Differential global search. (This slightly breaks any custom fields which exist, but they are presumably very rare, and probably do not exist; this break is also very mild.)
- Automatically perform CustomField and Subscribable indexing on applicable object types.
Test Plan: Used `bin/search index` to reindex a bunch of stuff, then searched for it. Debug-dumped abstract documents to inspect them.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T418, T3886, T2222
Differential Revision: https://secure.phabricator.com/D8346
Summary:
Ref T2222. This introduces two small new concepts:
- `expandTransactions()`: allows a transaction to expand into several transactions. For example, "resign" adds a "remove reviewers" transaction.
- We have some other cases which could use this, but currently hard-code things outside of the `Editor`.
- One example is that in Maniphest, closing a task implies claiming it if it is unowned.
- `setIgnoreOnNoEffect()`: The whole Editor can be set to continue or stop if any transactions have no effect, but this allows the behavior to be refined at the individual transaction level. This is primarily to make the UX less confusing, so the user gets only a single relevant error instead of one for each expanded transaction.
Otherwise, this is pretty straightforward.
Test Plan:
Rigged comment form to use SavePro controller, enabled resign action, then tried to resign from a bunch of stuff.
{F117743}
Reviewers: btrahan
Reviewed By: btrahan
CC: chad, aran
Maniphest Tasks: T2222
Differential Revision: https://secure.phabricator.com/D8328
Summary:
Ref T1191. I believe we only have three meaningful binary fields across all applications:
- The general cache may contain gzipped content.
- The file storage blob may contain arbitrary binary content.
- The Passphrase secret can store arbitrary binary data (although it currently never does).
This adds Lisk config for binary fields, and uses `%B` where necessary.
Test Plan:
- Added and executed unit tests.
- Forced file uploads to use MySQL, uploaded binaries.
- Disabled the CONFIG_BINARY on the file storage blob and tried again, got an appropraite failure.
- Tried to register with an account containing a G-Clef, and was stopped before the insert.
Reviewers: btrahan, arice
Reviewed By: arice
CC: arice, chad, aran
Maniphest Tasks: T1191
Differential Revision: https://secure.phabricator.com/D8316
Summary:
Ref T1191. Test that MySQL's rules match those of `phutil_is_utf8_with_only_bmp_characters()`:
- Build a string with //every// character that we consider to be a BMP character.
- Write it into MySQL.
- Read it back out.
- Make sure MySQL didn't truncate it.
Test Plan: Ran unit test. This test runs pretty quickly (50ms), the string with every character isn't all that enormous.
Reviewers: btrahan, arice
Reviewed By: arice
CC: chad, arice, aran
Maniphest Tasks: T1191
Differential Revision: https://secure.phabricator.com/D8314
Summary:
Ref T3886. Broadly, fields break down into two types right now: fields which store data on the object (like `DifferentialTitleField`) and fields which store data in custom field storage.
The former type generally reads data from the object into local storage prior to editing, then writes it back afterward. Currently, this happens in `didSetObject()`.
However, now that we load and set objects from ApplicationTransactionQuery, we'll do this extra read-field-values on view interfaces too. There, it's unnecessary and sometimes throws data-attached exceptions.
Instead, separate these concepts, and do all the read-from-object / read-from-storage in one logical chunk, separate from `didSetObject()`.
Test Plan:
- Edited Differential revision.
- Edited Maniphest task.
- Edited Project.
- Edited user profile.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T3886
Differential Revision: https://secure.phabricator.com/D8299
Summary:
Ref T3886. Ref T418.
- Adds "View Policy" and "Edit Policy" fields.
- Allows CustomFields to produce arbitrary types of transactions, so these fields can produce standard view/edit policy transactions and get all the strings and validation associated with them.
Test Plan: {F116001}
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T418, T3886
Differential Revision: https://secure.phabricator.com/D8287
Summary:
Ref T3886. Ref T418. For fields like "Summary" and "Test Plan" where changes can't be summarized in one line, allow CustomField to provide a "(Show Details)" link and render a diff.
Also consolidate some of the existing copy/paste, and simplify this featuer slightly now that we've move to dialogs.
Test Plan:
{F115918}
- Viewed "description"-style field changes in phlux, pholio, legalpad, maniphest, differential, ponder (questions), ponder (answers), and repositories.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T3886, T418
Differential Revision: https://secure.phabricator.com/D8284
Summary: Ref T2222. Ref T1790. I partially modernized this recently, but bring it to the mail version too.
Test Plan: See screenshots.
Reviewers: btrahan
Reviewed By: btrahan
CC: zeeg, aran
Maniphest Tasks: T1790, T2222
Differential Revision: https://secure.phabricator.com/D8294
Summary: Fixes T4466. We do an excessively strict effect check now, which means that these fields changing from (string) "16" to (int) 16 generates a transaction. Instead, compare integer values if the field has data in it.
Test Plan:
{F116261}
(Also made updates without changing the number, which did not appear in the transaction log anymore.)
Reviewers: btrahan, richardvanvelzen
Reviewed By: richardvanvelzen
CC: aran
Maniphest Tasks: T4466
Differential Revision: https://secure.phabricator.com/D8292
Summary:
Fixes T4463. When your VCS or account password is not set, we test it for upgrade anyway. This doesn't make sense and throws shortly into the process because the empty hash isn't parseable.
Instead, only show upgrade prompts when the password exists.
Test Plan:
- Added a password to an existing account with no password via password reset.
- Added a VCS password to an existing account with no VCS password.
- Observed no fatals / nonsense behaviors.
Reviewers: btrahan, chad
Reviewed By: chad
CC: aran
Maniphest Tasks: T4463
Differential Revision: https://secure.phabricator.com/D8282
Summary: ...do it somewhat generically, so we could fairly easily add this to other applications. Fixes T3496. I got a wee bit lazy and decided not to migrate existing drafts. My excuses aside from laziness are doing it this way will let us see if anyone complains, we can always do a migration later if people do complain, and there's likely to be a lot of garbage data for older / bigger installs, and the migration didn't seem worth itgiven it would also likely be expensive in these cases.
Test Plan: made a draft inline comment on DX and observed DX had a note icon on Differential home page. made a draft comment on DX and observed DX had a note icon on Differential home page. deleted a draft inline comment and noted icon disappeared from Differential homepage. Submitted a draft comment + inline comment and noted icon disappeared.
Reviewers: epriestley
Reviewed By: epriestley
CC: Korvin, epriestley, aran
Maniphest Tasks: T3496
Differential Revision: https://secure.phabricator.com/D8275
Summary: Fixes T4443. Plug VCS passwords into the shared key stretching. They don't use any real stretching now (I anticipated doing something like T4443 eventually) so we can just migrate them into stretching all at once.
Test Plan:
- Viewed VCS settings.
- Used VCS password after migration.
- Set VCS password.
- Upgraded VCS password by using it.
- Used VCS password some more.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T4443
Differential Revision: https://secure.phabricator.com/D8272
Summary:
Ref T4443. In addition to performing upgrades from, e.g., md5 -> bcrypt, also allow sidegrades from, e.g., bcrypt(cost=11) to bcrypt(cost=12). This allows us to, for example, bump the cost function every 18 months and stay on par with Moore's law, on average.
I'm also allowing "upgrades" which technically reduce cost, but this seems like the right thing to do (i.e., generally migrate password storage so it's all uniform, on average).
Test Plan:
- Fiddled the bcrypt cost function and saw appropriate upgrade UI, and upgraded passwords upon password change.
- Passwords still worked.
- Around cost=13 or 14 things start getting noticibly slow, so bcrypt does actually work. Such wow.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T4443
Differential Revision: https://secure.phabricator.com/D8271
Summary:
Ref T4443.
- Add a `password_hash()`-based bcrypt hasher if `password_hash()` is available.
- When a user logs in using a password, upgrade their password to the strongest available hash format.
- On the password settings page:
- Warn the user if their password uses any algorithm other than the strongest one.
- Show the algorithm the password uses.
- Show the best available algorithm.
Test Plan: As an md5 user, viewed password settings page and saw a warning. Logged out. Logged in, got upgraded, no more warning. Changed password, verified database rehash. Logged out, logged in.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T4443
Differential Revision: https://secure.phabricator.com/D8270
Summary:
Ref T4443. Make hashing algorithms pluggable and extensible so we can deal with the attendant complexities more easily.
This moves "Iterated MD5" to a modular implementation, and adds a tiny bit of hack-glue so we don't need to migrate the DB in this patch. I'll migrate in the next patch, then add bcrypt.
Test Plan:
- Verified that the same stuff gets stored in the DB (i.e., no functional changes):
- Logged into an old password account.
- Changed password.
- Registered a new account.
- Changed password.
- Switched back to master.
- Logged in / out, changed password.
- Switched back, logged in.
- Ran unit tests (they aren't super extensive, but cover some of the basics).
Reviewers: btrahan
Reviewed By: btrahan
CC: aran, kofalt
Maniphest Tasks: T4443
Differential Revision: https://secure.phabricator.com/D8268
Summary: Fixes T3872. Ref T1812. Ref T3886. Modernize the "closes x as y" string parser, and use all the new parsers instead of the old ones.
Test Plan: Made a commit full of a pile of these trigger strings, then used `scripts/repository/reparse.php --message` to reparse it. Verified that parses came back as expected using a bunch of `var_dump()`.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T1812, T3872, T3886
Differential Revision: https://secure.phabricator.com/D8263
Summary:
Ref T3886. See D8261. This brings the "reverts x" phrase to modern infrastructure. It isn't actually called by the real parser yet, I'm going to do that in one go at the end so I can test everything more easily.
This had unit tests; port most of them forward.
Test Plan: Added and executed unit tests.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T3886
Differential Revision: https://secure.phabricator.com/D8262
Summary:
Ref T3886. Ref T3872. Ref T1812. We have several parsers which look for textual references to other objects, like:
Closes Tx.
Depends on Dy.
Reverts Dz.
Currently, these are pretty hard coded, don't get all the edge cases right, and don't generalize well. They're also implemented in the middle of Differential's field code. So I want to:
- Share more code so that, e.g., "Tx, Ty" always works (only some rules support it right now);
- fix bugs in the parser, like T3872;
- make this a modular, extensible process which runs against custom fields, not a builtin part of fields;
- make the internals more flexible to accommodate custom stuff like T1812.
This implements the "Verbs optional-noun Object, Optional Other Objects optional-as-something." grammar in a general way so subclasses can just plug in their keywords. Runtime code doesn't touch this yet.
Test Plan: Ran unit tests.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T3872, T1812, T3886
Differential Revision: https://secure.phabricator.com/D8261
Summary:
Allows to keep really wide graphs inside the div:
dot (width=100%) {{{ }}}
Test Plan:
Created a graph that is wider than a phriction page.
Added `(width=100%)` and now the images stays within the div.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley
CC: Korvin, epriestley, aran
Differential Revision: https://secure.phabricator.com/D8235
Summary:
Ref T2222. This restores the "N older comments are hidden." shield to all ApplicationTransactions applications. Roughly the rule this uses is that transactions older than your most recent comment are hidden, under the assumption that you've already read and dealt with them, since you replied afterward. Then we show your last comment to remind/contextualize you, and anything afterward. We also don't hide transactions if we'd only be hiding a handfull, and we never hide the few most recent transactions.
This might need some #design help.
Test Plan:
The tricky part here is the anchor rule, which deals with the case where you follow a link to `T123#4`, but that would normally be hidden. We simulate a click on "show all" if you hit an anchor which is hidden. Here's what it looks like in Maniphest:
{F112891}
Reviewers: btrahan, chad
Reviewed By: btrahan
CC: chad, aran
Maniphest Tasks: T2222
Differential Revision: https://secure.phabricator.com/D8229
Summary: Fixes T3300. Images under 32KB are inlined automatically into CSS using data URIs; larger images remain as normal links. I picked the 32KB threshold arbitrarily, based on it looking roughly like it got reasonable results on `core.pkg.css` (inlining most of the random stuff, but not inlining all the 2X sprites and such).
Test Plan: Loaded site, browsed around, looked at generated CSS.
Reviewers: btrahan, chad
Reviewed By: chad
CC: aran
Maniphest Tasks: T3300
Differential Revision: https://secure.phabricator.com/D8225
Summary: Ref T4375. We never join this table, so this is a pretty straight find/replace.
Test Plan: Browsed around Calendar, verified that nothing seemed broken. Saw my red dot in other apps.
Reviewers: btrahan, chad
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T4375
Differential Revision: https://secure.phabricator.com/D8145
Summary: Ref T1139. This has some issues and glitches, but is a reasonable initial attempt that gets some of the big pieces in. We have about 5,200 strings in Phabricator.
Test Plan: {F108261}
Reviewers: btrahan
Reviewed By: btrahan
CC: aran, chad
Maniphest Tasks: T1139
Differential Revision: https://secure.phabricator.com/D8138
Summary: `What's new` has been broken for awhile, I've updated it to use the `feed.query` text view.
Test Plan: Start up a bot and say "What's new?"
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley
CC: fas, epriestley, aran, Kage, demo
Differential Revision: https://secure.phabricator.com/D8118
Summary: This adds the app icons, cleans up css Ref T3623
Test Plan: see new icons in dropdown menu
Reviewers: epriestley
Reviewed By: epriestley
CC: Korvin, epriestley, aran
Maniphest Tasks: T3623
Differential Revision: https://secure.phabricator.com/D8124
Summary:
Ref T3583. General idea here is:
- Users will be able to create `DashboardPanel`s, which are things like the jump nav, or a minifeed, or recent assigned tasks, or recent tokens given, or whatever else.
- The `DashboardPanel`s can be combined into `Dashboard`s, which select specific panels and arrange them in some layout (and maybe have a few other options eventually).
- Then, you'll be able to set a specific `Dashboard` for your home page, and maybe for project home pages. But you can also use `Dashboard`s on their own if you just like dashboards.
My plan is pretty much:
- Put in basic infrastructure for dashboards (this diff).
- Add basic create/edit (next few diffs).
- Once dashboards sort of work, do the homepage integration.
This diff does very little: you can't create dashboards or panels yet, and thus there are no dashboards to look at. This is all skeleton code, pretty much.
IMPORTANT: We need an icon bwahahahahaha
Test Plan:
omg si purrfect
{F106367}
Reviewers: chad, btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T3583
Differential Revision: https://secure.phabricator.com/D8109
Summary: Ref T1921. Ref T4339. If you `phabricator_form()` with an absolute URI, we silently drop the CSRF tokens. This can be confusing if you meant to specify `"/some/path"` but ended up specifying `"http://this.install.com/some/path"`. In all current cases that I can think of / am aware of, this indicates an error in the code. Make it more obvious what's happening and how to fix it. The error only fires in developer mode.
Test Plan: Hit this case, also rendered normal forms.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T4339, T1921
Differential Revision: https://secure.phabricator.com/D8044
Summary: we need this for legalese. Ref T3116
Test Plan: made a legalpad document with underlines. also re-gened docs and noted underlines worked
Reviewers: epriestley
Reviewed By: epriestley
CC: Korvin, epriestley, aran
Maniphest Tasks: T3116
Differential Revision: https://secure.phabricator.com/D7996
Summary:
Ref T4327. I want to make change parsing testable; one thing which is blocking this is that the Git discovery process is still part of `PullLocal` daemon instead of being part of `DiscoveryEngine`. The unit test stuff which I want to use for change parsing relies on `DiscoveryEngine` to discover repositories during unit tests.
The major reason git discovery isn't part of `DiscoveryEngine` is that it relies on the messy "autoclose" logic, which we never implemented for Mercurial. Generally, I don't like how autoclose was implemented: it's complicated and gross and too hard to figure out and extend.
Instead, I want to do something more similar to what we do for pushes, which is cleaner overall. Basically this means remembering the old branch heads from the last time we parsed a repository, and figuring out what's new by comparing the old and new branch heads. This should give us several advantages:
- It should be simpler to understand than the autoclose stuff, which is pretty mind-numbing, at least for me.
- It will let us satisfy branch and tag queries cheaply (from the database) instead of having to go to the repository. We could also satisfy some ref-resolve queries from the database.
- It should be easier to extend to Mercurial.
This implements the basics -- pretty much a table to store the cursors, which we update only for Git for now.
Test Plan:
- Ran migration.
- Ran `bin/repository discover X --trace --verbose` on various repositories with branches and tags, before and after modifying pushes.
- Pushed commits to a git repo.
- Looked at database tables.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T4327
Differential Revision: https://secure.phabricator.com/D7982
Summary:
does a few smallish things... Ref T3116
- adds an action to "sign document", thus improving visiblity of this feature from 0 to some value more than 0
- adds a crumb on the edit page to get back to the view page
- warns the user on the edit page IFF signatures exist for the current version that their edits could invalidate those signatures
- adds a "needSignatures" option to the Document Query class
Test Plan: click the new UI elements and they worked. edited a document with signatures, noted warning UI, edited anyway, noted warning UI correctly disappeared on new edit. also verified a single signature had the correct translation
Reviewers: epriestley
Reviewed By: epriestley
CC: Korvin, epriestley, aran
Maniphest Tasks: T3116
Differential Revision: https://secure.phabricator.com/D7983
Summary: This modularizes the rest of the GC submethods. Turned out there was nothing tricky.
Test Plan: Ran `bin/phd debug garbage` and got reasonable looking behavior and output.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Differential Revision: https://secure.phabricator.com/D7971
Summary:
The GC is a big block of hard-coded application GCs right now. Among other things, this means third parties can't tap into the infrastructure.
Modularize it into `GarbageCollector` classes. This implements only one to prove the new stuff works; I'll followup with the rest in the next diff or few depending on how much mess I run into.
Test Plan: Used `bin/phd debug garbage` to run the collector in debug mode, observed reasonable output and behavior.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Differential Revision: https://secure.phabricator.com/D7970
Summary: Fixes T4317. Update the "inline comment" control to a RemarkupControl. This could maybe use some padding/spacing/design touches eventually but seems OK for the moment.
Test Plan: {F101825}
Reviewers: chad, btrahan
Reviewed By: chad
CC: chad, aran
Maniphest Tasks: T4317
Differential Revision: https://secure.phabricator.com/D7969
Summary: Ref T1344. Write edges and read them when reloading the board.
Test Plan: After reload, stuff stays mostly where I put it. In-column order isn't always persisted correctly yet.
Reviewers: chad, btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T1344
Differential Revision: https://secure.phabricator.com/D7944
Summary:
Ref T1344. This fixes two issues with DraggableList:
- In lists which allowed it, you could drag the top item above itself and get a dashed-border ghost item. This didn't make sense and didn't behave well. Just don't treat this operation as valid.
- In lists which allowed it, you could drag any non-top item to the topmost position, then drag it to an invalid position. The dashed-border ghost item would not be removed properly if this happend.
- Also fix some minor leftovers with Celerity.
Test Plan:
- Dragged the first item above itself; now an invalid operation with no ghost.
- Dragged another item to the first position then back to its original position; ghost vanishes.
- Clean lint.
Reviewers: chad, btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T1344
Differential Revision: https://secure.phabricator.com/D7939
Summary:
Currently, to add new migration patches you need to:
- Add a file to `resources/sql/patches/`; then
- add an entry to `src/infrastructure/storage/blahblah/BlahBlahBlah.php`.
The second step isn't actually necessary, and we've been using this system for a long time without any issues arising.
Instead of requiring manual adjustments to the patch list, infer the patch specifications from the files on disk so you don't need to do step 2.
Also, simplify the existing data, which can //mostly// be derived from patch names. There are a few exceptions/errors, noted inline, which are preserved for compatibility.
Test Plan:
- For the new genration of `name` and `type`, I added code to check that the old and new values were the same before converting. This caught the two inline exceptions ("emailtableport", "drydockresouces").
- Added new patches to `autopatches/` and ran `bin/storage status` to verify they got picked up correctly.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Differential Revision: https://secure.phabricator.com/D7894
Summary: Ref T4264. Ref T2628. Ref T3102. Allows you to associate repositories with projects. In the future, you'll be able to write Herald object rules against projects, use Herald fields like "Repository's projects", and search by project.
Test Plan: See screenshots.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T3102, T4264, T2628
Differential Revision: https://secure.phabricator.com/D7881
Summary: Ref T4222. Adds the map name to Celerity resource URIs, so we can serve out of any map.
Test Plan: Poked around, verified URIs have "/phabricator/" in them now.
Reviewers: btrahan, hach-que
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T4222
Differential Revision: https://secure.phabricator.com/D7877
Summary:
Ref T4222. Currently, CelerityResourceResponse holds response resources in flat maps. Instead, specify which map resources appear in.
Also, provide `requireResource()` and `initBehavior()` APIs on the Controller and View base classes. These provide a cleaner abstraction over `require_celerity_resource()` and `Javelin::initBehavior()`, but are otherwise the same. Move a few callsites over.
Test Plan:
- Reloaded pages.
- Browsed around Differential.
Reviewers: btrahan, hach-que
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T4222
Differential Revision: https://secure.phabricator.com/D7876
Summary:
Repositories currently have a no-UI "shortcut" feature which is only used by Facebook (and I'm not sure it's even used). As implemented, this feature is policy-oblivious and kind of nonsensical. Throw it away.
I'm open to reimplementing this, but I want to see some level of interest in it before I do. The new implementation would add shortcuts to each repository, similar to how mirrors work. My original plan was to follow this up with such an implementation (it's half-implemented in my sandbox), but as I worked through it I'm not sure it's really valuable.
Test Plan: Browsed repository list, grep.
Reviewers: btrahan
Reviewed By: btrahan
CC: FacebookPOC, aran
Differential Revision: https://secure.phabricator.com/D7862
Summary:
Ref T4222. Earlier, I adjusted the root from `webroot/` to `webroot/rsrc/`. However, this means that all the `/rsrc/x/y/z.jpg` fragments in CSS are no longer recognized as resource names.
Since we have like 9,000 things in CSS that do `url(/rsrc/xyz.jpg)` and I don't want to fix/test them all, so just make them work as-is. There's no real reason either setting is better than the other.
(Both URLs also work fine, but the parsed one will be better once we have real CDN support.)
Test Plan: Verified CSS gets managed resource URIs transformed into it.
Reviewers: btrahan, hach-que
Reviewed By: hach-que
CC: aran
Maniphest Tasks: T4222
Differential Revision: https://secure.phabricator.com/D7875
Summary: Ref T4222. This doesn't actually support multiple sources yet, but moves us closer by getting rid of some dead and exceedingly-singletoney code.
Test Plan: Browsed around, looked at Phame blogs.
Reviewers: btrahan, hach-que
Reviewed By: hach-que
CC: aran
Maniphest Tasks: T4222
Differential Revision: https://secure.phabricator.com/D7874
Summary:
Ref T4222. This fixes some issues with Phame's resource construction.
Phame requires a fully virtual resource source, and since I want to run wordpress templates unmodified some day I don't want to build resource maps for skins.
Move all the stuff that depends on resource lists being discoverable at build time to `CelerityPhysicalResources`, and only generate maps for subclasses.
The root `CelerityResources` can now construct virtual resources; construct a virtual resource for Phame and use it.
Test Plan: Off-domain blogs work correctly now. On-domain blogs with custom skins work correctly now.
Reviewers: btrahan, hach-que
Reviewed By: hach-que
CC: aran
Maniphest Tasks: T4222
Differential Revision: https://secure.phabricator.com/D7873
Summary:
Ref T4222.
- Removes the old map and changes the CelerityResourceMap API to be entirely driven by the new map.
- The new map is about 50% smaller and organized more sensibly.
- This removes the `/pkg/` URI component. All resources are now required to have unique names, so we can tell if a resource is a package or not by looking at the name.
- Removes some junky old APIs.
- Cleans up some other APIs.
- Added some feedback for `bin/celerity map`.
- `CelerityResourceMap` is still a singleton which is inextricably bound to the Phabricator map; this will change in the future.
Test Plan:
- Reloaded pages.
- Verified packaging works by looking at generated includes.
- Forced minification on and verified it worked.
- Forced no-timestamps on and verified it worked.
- Rebuilt map.
- Ran old script and verified error message.
- Checked logs.
Reviewers: btrahan, hach-que
Reviewed By: hach-que
CC: chad, aran
Maniphest Tasks: T4222
Differential Revision: https://secure.phabricator.com/D7872
Summary: Ref T4222. These are the last two "return a big ball of mud" methods. Make the API stronger so I can swap out the implementations.
Test Plan: Reloaded pages.
Reviewers: btrahan, hach-que
Reviewed By: hach-que
CC: aran
Maniphest Tasks: T4222
Differential Revision: https://secure.phabricator.com/D7871
Summary:
Ref T4222. A few diffs from now, `CelerityResourceMap` will have a `CelerityResources` inside of it:
- Rename `resolvePackage()` to `getResourceNamesForPackageHash()`. This isn't a functional change, it's just making it clear what it does.
- Add `getResourceDataForName()`, to push details about storage into `CelerityResources`.
Test Plan: Reloaded a bunch of pages, rebuilt map.
Reviewers: btrahan, hach-que
Reviewed By: hach-que
CC: aran
Maniphest Tasks: T4222
Differential Revision: https://secure.phabricator.com/D7869
Summary: Ref T4222. Same deal as D7867, but for this other super nebulous "return a blob of stuff" method.
Test Plan: Regenerated map, browsed around, etc.
Reviewers: btrahan, hach-que
Reviewed By: hach-que
CC: aran
Maniphest Tasks: T4222
Differential Revision: https://secure.phabricator.com/D7868
Summary: Ref T4222. Currently, this exposes a bunch of information about the Celerity internals. This information is difficult to preserve exactly with the new maps. Strengthen the API by providing more specific capabilities.
Test Plan: Regenerated map, browsed around.
Reviewers: btrahan, hach-que
Reviewed By: hach-que
CC: aran
Maniphest Tasks: T4222
Differential Revision: https://secure.phabricator.com/D7867
Summary: Ref T4222. Continues porting `scripts/celerity_mapper.php` functionality into `bin/celerity map`. This is pretty much a `1:1` port with no functional changes, but hopefully the code is a little better factored. Particularly, more responsibilities are pluggalbe through `CelerityResources` now.
Test Plan: Ran `bin/celerity map` and inexpected the `var_dump()` output, which appeared to make sense.
Reviewers: btrahan, hach-que
Reviewed By: hach-que
CC: aran
Maniphest Tasks: T4222
Differential Revision: https://secure.phabricator.com/D7865
Summary:
Ref T4222. Moves us toward a more modern Celerity CLI, and moves map discovery into the classtree. This is a little bit bulky (and means you can't ship completely standalone celerity maps) but has the advantage of being completely standard, and we could subclass maps into an auto-discovering map later if we have a need for it.
This doesn't affect the existing Celerity stuff. I'm going to build the new stuff in parallel, and then swap us over at the end.
Test Plan: Ran `bin/celerity map`, got reasonable-looking output.
Reviewers: btrahan, hach-que
Reviewed By: hach-que
CC: aran
Maniphest Tasks: T4222
Differential Revision: https://secure.phabricator.com/D7864
Summary: Ref T4222. This was used by Facebook while developing Releeph, but should no longer be necessary since Releeph is in the upstream. I can't get an answer out of Facebook about whether they still use it or not (see T4227), so nuke it. We're going to replace it with a more general mechanism (see T4222).
Test Plan: Regenerated celerity map. Browsed some pages, still got resources.
Reviewers: btrahan, hach-que
Reviewed By: hach-que
CC: aran
Maniphest Tasks: T4222
Differential Revision: https://secure.phabricator.com/D7863
Summary:
Ref T4264. This gets most of the plumbing in for "object" rules, which will bind to a specific object, like a repository or project.
It does not yet let you actually create these rules.
Test Plan: Ran `storage upgrade`, created/edited rules, browsed Herald.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T4264
Differential Revision: https://secure.phabricator.com/D7847
Summary:
Ref T2015. Not directly related to Drydock, but I've wanted to do this for a bit.
Introduce a common base class for all the workflows in the scripts in `bin/*`. This slightly reduces code duplication by moving `isExecutable()` to the base, but also provides `getViewer()`. This is a little nicer than `PhabricatorUser::getOmnipotentUser()` and gives us a layer of indirection if we ever want to introduce more general viewer mechanisms in scripts.
Test Plan: Lint; ran some of the scripts.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T2015
Differential Revision: https://secure.phabricator.com/D7838
Summary:
Ref T2015. Currently, Drydock has a `wait-for-lease` workflow which is invoked in the background by the `lease` workflow.
The goal of this mechanism is to allow `bin/drydock lease` to print out logs as the lease is acquired. However, this predates the `runAllTasksInProcess` flags, and they provide a simpler and more robust way (potentially with `--trace` and `PhutilConsole`) to do synchronous execution and debug logging.
Simplify this whole mechanism: just run everything in-process in `bin/drydock lease`, and do logging via `--trace`. We could thread a `PhutilConsole` through things too, but this seems good enough for now.
Also various cleanup/etc.
Test Plan: Ran `bin/drydock lease`. Ran `bin/harbormaster build X --plan Y`, for `Y` being a Drydock-dependent build plan.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T2015
Differential Revision: https://secure.phabricator.com/D7835
