Summary:
This is mostly ripped from D2721, but doesn't implement the T945 part.
After we parse a commit message, give DifferentialFieldSpecifications an opportunity to react to the message as well (e.g., by updating related objects).
Test Plan: Impelmented a var_dump() body, ran `reparse.php` on a commit.
Reviewers: vrana, 20after4, btrahan, edward
Reviewed By: 20after4
CC: aran
Maniphest Tasks: T945, T1544
Differential Revision: https://secure.phabricator.com/D3444
Summary:
Without this, the https redirect doesn't work if you're not logged in, because
the login check in willBeginExecution happens before the redirect controller
can redirect. This also has the unpleasant effect of the login page on http
(when it should have redirected to https) not having any css or js.
Test Plan:
With the https redirect enabled, loaded phabricator without being logged in,
and saw that I got redirected to the https login page instead of seeing a
http login page.
Reviewers: epriestley, vrana
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D3438
Summary: AphrontSideNavView is oldschool and I want to kill it.
Test Plan: Viewed Ponder, clicked both nav options.
Reviewers: pieter, vrana, btrahan
Reviewed By: pieter
CC: aran
Differential Revision: https://secure.phabricator.com/D3430
Summary:
I want to implement a `{P123}` rule to embed pastes, but we need viewers everywhere before it will work with privacy.
This is not exhaustive; many Remarkup callsites haven't been converted to `PhabricatorMarkupInterface` yet.
Test Plan: Looked at Maniphest, Differential, Diffusion, Phriction; added markup, made edits and hit previews.
Reviewers: vrana, btrahan
Reviewed By: vrana
CC: aran
Differential Revision: https://secure.phabricator.com/D3428
Summary:
We have lots of info about unit tests.
This allows linking them from Unit field.
Test Plan: Monkey patched `$test['link']`, clicked on it.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D3434
Summary:
I've replaced all `id(new PhabricatorObjectHandleData(...))->loadHandles()` by `$this->loadViewerHandles(...)`.
Lint caught one usage in a static method.
Test Plan: Displayed revision with sporadic author.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D3432
Summary: I've done D3432 in the hope that it will fix also this...
Test Plan: Blamed sporadic author.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D3433
Summary: Rehash of D3411. In cgi/fcgi setups we have no idea if the request is HTTP or HTTPS as far as I can tell, so make this config-triggered again. Also handle @vrana's "off" case.
Test Plan: Set this flag, observed redirect to https when `$_SERVER['HTTPS']` was absent.
Reviewers: nh, vrana
Reviewed By: nh
CC: aran
Differential Revision: https://secure.phabricator.com/D3420
Summary:
See discussion in D3340. Some configurations set only a search attribute because their records are indexed by username (this is probably not quite the correct LDAP terminology). Other configurations use one attribute to search and a different attribute to select usernames.
After D3340, installs which set only a search attribute broke. Instead, fall back to the search attribute if no username attribute is present.
Test Plan: Successfully logged in on my test slapd.
Reviewers: yunake, voldern, briancline
Reviewed By: voldern
CC: aran
Differential Revision: https://secure.phabricator.com/D3406
Summary: It would be best to add this everywhere at once but I'm too lazy for it.
Test Plan: Displayed package list and detail and revision comment with away users.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D3419
Summary:
Currently, if some page /x/ has a grandchild at /x/y/z/ but /x/y/ does not exist, we insert a ghost entry for it in the "Document Hierarchy". However, we don't sort the hierarchy properly after inserting ghosts, so they always appear at the bottom and in an unuseful order.
Instead, sort children again after any ghost insertions.
Test Plan: Created /x/a/, /y/a/, /z/a/, verified ghosts sorted incorrectly before this patch and correctly afterward.
Reviewers: btrahan, vrana
Reviewed By: vrana
CC: aran
Maniphest Tasks: T1750
Differential Revision: https://secure.phabricator.com/D3418
Summary: Replaces the full names after D3413.
Test Plan: See screenshot.
Reviewers: vrana
Reviewed By: vrana
CC: aran
Differential Revision: https://secure.phabricator.com/D3414
Summary:
There is basically no reason for anyone to ever use the uncollapsed mode for more than the first 2 minutes of using the tool.
Delete all code related to collapse/expand.
(I'm going to add tooltips next.)
Also move the drag bar a few pixels to the right, so it does not overlap with the scrollbar on the "local" nav if there is one.
Test Plan: Viewed in desktop/tablet/phone modes.
Reviewers: vrana
Reviewed By: vrana
CC: aran
Differential Revision: https://secure.phabricator.com/D3413
Summary:
See some discussion in D1673.
- There's a concrete (if minor) problem with this in Firefox with wrapping search.
- People complain about how we're stealing all their pixels.
- There isn't much of a functional purpose to it since all the operations are fairly rare.
- This addresses the aesthetic purpose of the fixed-position nav (not making the side nav ugly) by making the side nav scroll up 44px and then stop.
Test Plan: Scrolled in desktop, tablet modes.
Reviewers: vrana
Reviewed By: vrana
CC: aran
Differential Revision: https://secure.phabricator.com/D3412
Summary: We need to do something here.
Test Plan: None.
Reviewers: epriestley, nh
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D3415
Summary:
If the phabricator.base-uri is set up with https and phabricator receives
a request that isn't over ssl, we will issue a redirect response to the user
to force them to use https.
Test Plan:
With this disabled, verified that pages still load correctly over http.
Enabled it; verified I get redirected to the same path but on https when I
make an http request; verified https requests get served without a redirect.
Reviewers: epriestley, vrana
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D3411
Summary: Previously, the identification string was thrown at the server long before you were connected, I've moved this to the end of the motd raw, and now errthangz gud
Test Plan: Register an account for your bot to use, give your bot the correct nick and password, then watch
Reviewers: epriestley
CC: aran
Differential Revision: https://secure.phabricator.com/D3410
Summary: This isn't very obvious, provide some more specific instructions.
Test Plan: Followed the instructions on my Windows machine, got a working `php`.
Reviewers: btrahan, vrana, jungejason
Reviewed By: vrana
CC: aran
Differential Revision: https://secure.phabricator.com/D2555
Summary:
Some fields require displayed diff (e.g. Lines), some require last manual diff (Lint, Unit).
Lint requires both - it needs to find if the line with the error is displayed or not.
Test Plan: Displayed committed diff with lint errors, clicked on them, got last manual diff at the correct line.
Reviewers: epriestley, btrahan
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D3331
Summary:
Ball is more obvious and visible than I thought.
Delete the status word and display until date in title.
Also display the until date in revision list.
Also display near future dates with DoW instead of year.
Test Plan: Displayed revision and revision list with away reviewers.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D3407
Summary:
We have some complaints on this feature:
- It's not clear that the displayed code is context from gap.
- It would be better if the context would be displayed with its real indentation.
- It's not clear how far the context is from the displayed code.
- Links revealing gap aren't on consistent place.
This solves all these problems and introduces new one:
It now seems that the reveal links works only with the left side.
Anyway, I think that this is better overall.
I don't want to put the context on a separate line to not waste space.
Test Plan: Displayed various contexts, revealed context.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin, bh, jwatzman
Differential Revision: https://secure.phabricator.com/D3404
Summary: the former is self explanatory. the latter is necessary for installations that require email verification. since many system agents are given bogus email address there can become a problem where these accounts can't be verified
Test Plan: created system agent account from scratch. edited user and toggled system agent accountness. created system agent with unverified email address and verified it.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Maniphest Tasks: T1656
Differential Revision: https://secure.phabricator.com/D3401
Summary:
I have a chain of revisions sometimes.
I want to see also revisions depending on me.
Test Plan: Displayed revision in the middle of chain.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D3399
Summary: this is useful for certain workflows, typically where the reviewer is a gatekeeper of sorts who does the acutal commit. Special thanks to D2900 which made this relatively brain-dead to code up.
Test Plan: set to "true" in my local development environment and verified test user "xerxes" could close my stuff
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Maniphest Tasks: T1732
Differential Revision: https://secure.phabricator.com/D3398
Summary: said differently, if the user included another to address or one or more cc's, don't send the error message email.
Test Plan: played around in the metamta test console and verified that blank replies generated the error handler.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Maniphest Tasks: T1643
Differential Revision: https://secure.phabricator.com/D3345
Summary:
Currently, CelerityController extends AphrontController, not PhabricatorController. (I think I imagined Celerity being somewhat stand-alone and didn't want to create a dependency.)
This creates a concrete problem if a static resource is missing, since we throw an exception, but the higher-level exception handlers depend on the User existing in order to show an appropriate response page. This is the only controller which doesn't extend PhabricatorController, and it doesn't seem worthwhile to make a weird edge case out of it.
Specific repro case is:
- Remove `externals/javelin/` (or forget to run `git submodule update --init`).
- Load a static resource.
- Get "[Rendering Exception] Argument 1 passed to PhabricatorMainMenuView::setUser() must be an instance of PhabricatorUser, null given, called in /services/apache/phabricator/phabricator/src/view/page/PhabricatorStandardPageView.php on line 435 and defined"
Test Plan:
- Followed above steps, no more fataling.
- Verified this is the only weird controller.
Reviewers: voldern, vrana, btrahan
Reviewed By: voldern
CC: aran
Differential Revision: https://secure.phabricator.com/D3389
Summary:
We have `max_allowed_packet` 1 GiB but our replication dies if the query is longer than unknown value (it dies with 293 MB long query).
Anyway, there's no reason why we should not save the cache if you have small `max_allowed_packet`.
Test Plan: Lowered `$size` to 100, deleted cache from DB, displayed changeset, verified issued queries in DarkConsole, verified DB.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D3390
Summary:
I like systems that just work. It is possible to store files larger than max_allowed_packet in MySQL and we shouldn't demand it.
It also fixes a problem when file was smaller than `storage.mysql-engine.max-size` but its escaped version was larger than `max_allowed_packet`.
Test Plan: Reduced the size to 5e4, uploaded 90 kB file, checked the queries in DarkConsole, downloaded the file.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D3392
Summary: Also allow left nav to hide.
Test Plan:
# Resize left nav.
# Shrink browser width to switch device.
# Increase the width again.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D3383
Summary: In my haste, I forgot a trailing ?
Test Plan: Try both "Where is Derp?" and "Where in the world is Derp?"
Reviewers: epriestley
CC: aran
Differential Revision: https://secure.phabricator.com/D3387
Summary: We recently opted for 'security.alternate-file-domain' and we have some hotlinks to the original domain.
Test Plan: Enabled 'security.alternate-file-domain', observed redirect.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D3380
Summary:
This does a few things:
- Allows you to flag pastes. This is straightforward.
- Allows Applications to register event listeners.
- Makes object action lists emit a 'didrenderactions' event, so other applications can add more actions. The Flags application injects its action in this way. This should generally make it much easier to add actions to objects when we add new applications, with less code duplication and better modularity. We have a really hacky version of this in Differential that I want to get rid of in lieu of this more general approach. I'm going to make object lists do the same thing, so any application can jump in and add stuff.
Test Plan: Flagged and unflagged pastes. Viewed home page, differential, flags list.
Reviewers: vrana, btrahan
Reviewed By: btrahan
CC: aran
Differential Revision: https://secure.phabricator.com/D3377
Summary: Permits the name and langauge of a paste to be edited. This will eventually allow the visibility policy to be edited as well.
Test Plan: Edited name/langauge of some pastes. Tried to edit a paste I didn't own, was harshly rebuffed.
Reviewers: vrana, btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T1690
Differential Revision: https://secure.phabricator.com/D3376
Summary:
We have this hybrid "create / last few pastes" landing screen right now but I ~never use the list at the bottom and it makes the controller kind of complicated. I want to let you edit pastes too, and this generally simplifies things.
Also makes the textarea monospaced and cleans up the fork logic a bit.
Test Plan: Created, forked pastes. Viewed paste lists. Viewed pastes.
Reviewers: vrana, btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T1690
Differential Revision: https://secure.phabricator.com/D3375
Summary:
When logging in as an LDAP user for the first time (thus registering), a DAO exception was being thrown because PhabricatorLDAPRegistrationController wasn't passing in a username to PhabricatorUser::setUsername().
Somewhat separately, since either the PHP LDAP extension's underlying library or Active Directory are returning attributes with lowercased key names, I have to search on sAMAccountName and look for the key samaccountname in the results; this is fine since the config allows these to be defined separately. However I found that PhabricatorLDAPProvider::retrieveUserName() was attempting to use the search attribute rather than the username attribute. This resolves.
Test Plan: Tested registration and login against our internal AD infrastructure; worked perfectly. Need help from someone with access to a functional non-AD LDAP implementation; I've added the original author and CCs from D2722 in case they can help test in this regard.
Reviewers: epriestley, voldern
Reviewed By: voldern
CC: voldern, aran, Korvin, auduny, svemir
Differential Revision: https://secure.phabricator.com/D3340
Summary:
See T1602#15.
I don't intend to land this right away, because I'm not sure I won't
need another change or two to the rendering code. Keeping it here so I
won't forget about it.
Test Plan: iiam
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Maniphest Tasks: T1602
Differential Revision: https://secure.phabricator.com/D3240
Summary: See comment inline. We should fix this properly but it goes faily deep so I just stopped the bleeding for now. It's OK if we end up with a silly-looking file tree view for bizarre edge cases.
Test Plan: Created a problem diff as described, verified it no longer threw.
Reviewers: vrana, btrahan
Reviewed By: vrana
CC: aran, Avish
Maniphest Tasks: T1702
Differential Revision: https://secure.phabricator.com/D3373
Summary: If the commit has a known author but that author is different from the revision author or the revision doesn't exist, we'll throw away the commit author and then raise an audit for "Owners Not Involved". Instead, we should just use the commit author in all cases.
Test Plan:
Debugged this with Zor in IRC, he reported it fixed his issue.
Before:
http://pastie.org/4574680
("Author" is empty.)
After:
http://pastie.org/4574712
("Author" is correctly recognized.)
Reviewers: jungejason, nh, vrana, btrahan
Reviewed By: vrana
CC: aran
Differential Revision: https://secure.phabricator.com/D3374
Summary:
I quite often wonder what's inside these gaps displayed in changeset. In which method I am? Is it a while loop or a foreach? Maybe I'm in a class but the project doesn't have a sctrict policy of one class per file so what's the name of the class?
I've experimented with bunch of rules:
- Always display 0 indentation: useless for one class per file.
- Always display 1 indentation: weird inside global functions.
- Display closest lower indentation: works best.
I'm not sure about highlighting the context. I like highlighting but maybe in this case subtler monochrome text will work better.
Test Plan: Browsed bunch of diffs, loved it.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D3371
array_diff(): Argument #1 is not an array at [/var/www/phabricator/phabricator/src/applications/feed/PhabricatorFeedStoryPublisher.php:109]
Auditors: alanh
Summary:
Actions you made no longer show up in the lighting-bolt
dropdown. I didn't touch realtime notifications but they're transient
enough that it shouldn't matter too much?
I wonder, though, whether it would be more useful to have the
notifications still present but automatically marked read.
Test Plan:
Create notifications; muck around in the database; check that
the dropdown and list pages are displaying correctly.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Maniphest Tasks: T1402
Differential Revision: https://secure.phabricator.com/D3360
Summary: It seemed like a good idea at the time.
Test Plan: Uh huh.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D3352
Summary:
Just a bunch of copy-pasta from D2884. I suppose this calls for
a refactoring at some point...
Test Plan:
Make a bunch of updates, some from different users; check
notifications dropdown and list.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D3361
Summary: This has been deprecated for quite a while and I'm pretty sure there are no callsites in the wild since this tool doesn't get much use outside of Facebook.
Test Plan: grep
Reviewers: vrana, btrahan, meitros
Reviewed By: vrana
CC: aran
Maniphest Tasks: T603
Differential Revision: https://secure.phabricator.com/D3195
Summary:
Adds a flexible navigation menu to diffs that shows you your current position in the diff.
Anticipating some "this is the best thing ever" and some "this is the wosrt thing ever" on this, but let's see how much pushback we get? It seems pretty good to me.
Test Plan: Will attach screenshots.
Reviewers: vrana, btrahan
Reviewed By: vrana
CC: aran
Maniphest Tasks: T1633, T1591
Differential Revision: https://secure.phabricator.com/D3355
Summary:
We have some issues with Elastic search (or maybe it's SMC) causing that indexing sporadically doesn't work.
Throwing in indexing stops the workflow and is annoying.
Not indexing doesn't have fatal consequences for the user and we can (and probably should) postpone it.
Test Plan: Thrown, looked at log.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D3350
Summary: I had no idea what checkered is.
Test Plan: Flagged revision, flagged task.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D3346
Summary:
More and more relations are going under edges and I can't work with them from Relatives framework.
This doesn't have the nice transitive property of normal relatives (loading relative objects from relatives loads all of them at once) but I can add it when I need it.
I plan to use it in D3085 (after converting relationships to edges).
Test Plan:
$task = id(new ManiphestTask())
->loadOneWhere('phid = %s', $phid);
print_r($task->loadRelativeEdges(4));
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D3344
Summary:
We have /differential/filter/drafts/ but nobody knows about it.
This diff displays the draft only if there is no flag to not waste space.
Test Plan: /differential/filter/revisions/
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin, alanh
Differential Revision: https://secure.phabricator.com/D3324
Summary:
Rendering method shouldn't load data.
The view probably shouldn't load data either because it is a job for component (object that both loads data and displays them) but we don't have that concept in Phabricator.
This at least improves the architecture a little bit.
Test Plan: /differential/
Reviewers: epriestley
Reviewed By: epriestley
CC: alanh, aran, Korvin
Differential Revision: https://secure.phabricator.com/D3325
Summary:
This is another experiment for reducing reviewers response time.
I stole the idea (and colors) from [[ http://www.reviewboard.org/media/screenshots/2009/02/02/dashboard.png | ReviewBoard ]].
I actually quite like it (except when everything is red) and I can image that people will review just to have better color balance.
The code is not production ready for these reasons:
- We load holidays again and again for each revision. I couldn't cache it to static variable because it could persist multiple requests, right?
- I don't know how to expand height to the whole cell (I'm really bad in CSS).
- CSS rules are probably in wrong file.
- We probably want to use different colors.
This is how it looks:
{F16406}
Test Plan: Displayed revision list.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D3190
- Use a gradient on the main menu.
- Slightly darken the application menu.
- Use kerned logo text.
- Use cleaner logo image.
- Adjust search input colors to fit the darker scheme better.
Summary:
- Fix width, corresponding to wider sprites.
- Sprite the "Audit" icon.
- Mark the meta-application as device-ready.
- Fix some collapse/expand bugs with the draggable local navs.
- Add texture to local nav.
- Darken the application nav to make it more cohesive with the main nav. I think this is an improvement?
Test Plan: See screenshots.
Reviewers: chad, btrahan
Reviewed By: btrahan
CC: aran, netfoxcity
Maniphest Tasks: T1569
Differential Revision: https://secure.phabricator.com/D3338
Summary: See T1677. I think wanting bots to be able to post comments without sending email is a pretty reasonable use case. Eventually we should probably support this more broadly and maybe protect it with permissions (normal users maybe shouldn't be able to do this?) but we can wait for use cases.
Test Plan: Made comments with and without "silent". Verified that the non-silent comment sent email, and the silent comment did not.
Reviewers: btrahan, vrana
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T1677
Differential Revision: https://secure.phabricator.com/D3341
Summary: Currently, if no placeholder is configured we always move "Cc" up to "To", even if we have a valid "To". Instead, move "Cc" to "To" only if there's no "To" and no placeholder.
Test Plan: Sent email with "to" and "cc", email with "cc" only with a placeholder, and email with "cc" only without a placeholder. Verified recipients ended up in the right location in all cases.
Reviewers: nh, btrahan
Reviewed By: btrahan
CC: klimek, aran
Differential Revision: https://secure.phabricator.com/D3342
Summary:
The logic here was swapped - new file should be on the right side.
Plus we had a fatal for VS = -1 where new file should be on left.
Test Plan:
Downloaded raw diff of:
- base VS change
- change VS change
- change VS change with unmodified file
Reviewers: epriestley, btrahan
Reviewed By: btrahan
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D3333
Summary:
I need to visit Phabricator homepage (usually to read the docs) quite often.
This is also kind of a signature.
Test Plan: Clicked it.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D3335
Summary: If I click on some file in ToC and then go back in browser history then it currently does nothing.
Test Plan: Collapsed file, jumped on it in ToC, collapsed it again, jumped to inline comment in it, went back in history.
Reviewers: alanh
Reviewed By: alanh
CC: aran, epriestley
Differential Revision: https://secure.phabricator.com/D3328
Summary:
Currently, when a user runs "arc diff" and the diff exceeds PHP's 'post_max_size', they get a very confusing and irrelevant error about a missing Conduit session token. The reason for this is that 'post_max_size' doesn't build $_POST, so //all// the data is missing.
We try to detect this, but currently only do so effectively for specific file upload forms. Broaden the detection to cover all cases.
Previously, we ran into an issue where Firefox + HTML5 drag-and-drop uploads would get a false positive on this detection. I dug into this and added the Content-Type checks, which correctly handle that case.
Test Plan: With small and large 'post_max_size', ran small and large normal, HTML5 and multipart/form-data POST requests against Phabricator in Safari and Firefox. Got desired beahviors.
Reviewers: vrana, btrahan
Reviewed By: btrahan
CC: tido, aran
Differential Revision: https://secure.phabricator.com/D3320
Summary: We need to use commit diff in some links and manual diff in some others.
Test Plan: Displayed revision, verified that the action link uses commit diff.
Reviewers: epriestley, nh
Reviewed By: nh
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D3300
Summary: We use numbers here and I see no reason for strings.
Test Plan:
$ bin/storage upgrade
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D3303
Summary: Added match to the novel statement: Where in the world is derp?
Test Plan: Say something like "Where in the world is CarmenSandiego?"
Reviewers: epriestley
CC: aran
Differential Revision: https://secure.phabricator.com/D3318
Summary:
See D3126, T1667, T1658. Prior to D3126, `phd` did not use MySQL directly. Now that it does, there are at least two specific problems (see inline comment).
In the long term, we should probably break this dependency and use Conduit. However, we don't currently have access to the daemon log ID and getting it is a mess (the overseer generates it), and I think I want to rewrite how all this works at some point anyway (the daemon calls are currently completely unauthenticated, which is silly -- we should move them to an authenticated channel at some point, I think).
Test Plan: Ran `phd stop` with a bad MySQL config against a non-running daemon, didn't get a query error.
Reviewers: nh, vrana, btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T1667, T1658
Differential Revision: https://secure.phabricator.com/D3314
Summary: We need to open the envelope here.
Test Plan: Ran `bin/storage dump` without errors.
Reviewers: vrana, btrahan
Reviewed By: vrana
CC: aran
Differential Revision: https://secure.phabricator.com/D3315
Summary: See T1665. If you have a directory named 'readme', we try to read it as a README.
Test Plan: Created a directory named 'readme', hit a similar fatal to the one in T1665, applied this patch, everything worked great.
Reviewers: btrahan, vrana
Reviewed By: vrana
CC: aran
Maniphest Tasks: T1665
Differential Revision: https://secure.phabricator.com/D3312
Summary:
If I use my own selector then it doesn't respect Phabricator config.
Also I hated this method.
Test Plan: Used default selector, displayed revision.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D3307
Summary:
It's kind of nice to type `s explode` in jump nav and have it
just work. This involves weakening a bunch of the request parameter
checks, but the only real extra assumption is that language defaults to
PHP... which is not that big of a stretch, and it's not like we know
about any other languages' documentation.
It'd be better to have builtins be more first-class and less awkward
hack, but that seems hard.
Test Plan: Search for symbols.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D3302
Summary:
The "Running Daemons" tab in `/daemon` links to
`/daemon/log?show=running/`, which doesn't work. No other side nav tries
to link to a URL with a query string, and I don't know if this ever
worked. This fixes that in a minimally invasive way.
NOTE: Daemons run when a good man goes to war.
Test Plan: View `/daemon` and click on tabs.
Reviewers: epriestley, nh
Reviewed By: nh
CC: aran, Korvin, nh
Differential Revision: https://secure.phabricator.com/D3301
Summary:
This is the first time I've ever had CSS actually work like it promises it does (i.e., markup the "right" way and then you don't have to change the markup later).
Since I laboriously laid this whole thing out with <divs> originally, I was able to just override some of the styles and make the layout reasonable for devices.
The only differences for existing forms are:
- No colon after labels (looks cleaner anyway).
- Non-error required text is no longer a red star but a the grey word "Required" (this is clearer).
Test Plan:
Viewed paste form on a phone.
Viewed ~20 other forms on the site to verify that I didn't break anything.
Reviewers: btrahan, chad, vrana
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T1569
Differential Revision: https://secure.phabricator.com/D3298
Summary:
I just put them in the property table instead of a list at the foot, they looked weird down there and were too bulky relative to their importance.
This won't scale great if someone forks a paste ten thousand times or whatever, but we can deal with that when we get there.
Also clean up a few things and tweak some styles,
Test Plan: Looked at forked, unforked pastes.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Differential Revision: https://secure.phabricator.com/D3295
Summary:
- Add a PhabricatorApplication.
- Make most of the views work well on tablets / phones. The actual "Create" form doesn't, but everything else is good -- need to make device-friendly form layouts before I can do the form.
Test Plan: Will attach screenshots.
Reviewers: btrahan, chad, vrana
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T1569
Differential Revision: https://secure.phabricator.com/D3293
Summary:
This case is unusually complicated because there are more rules than most objects will have.
- Edits are either "joins", "leaves" or "other edits".
- "Joins" require "can join" or "can edit".
- "Leaves" don't require any policy.
- "Other edits" require "can edit".
- You can't edit away your ability to edit.
- You //can// leave a project that you wouldn't be able to rejoin.
Things I'm going to add:
- Global log of policy changes.
- `bin/policy` script for undoing policy changes.
- Test coverage for these rules.
Test Plan: Made various project visibility edits with various users, joined / left projects, etc. I'll add more complete coverage in the next diff.
Reviewers: btrahan, vrana
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T603
Differential Revision: https://secure.phabricator.com/D3270
Summary: See D3291.
Test Plan: Ran `phpast.getast` via API.
Reviewers: alanh
Reviewed By: alanh
CC: aran
Differential Revision: https://secure.phabricator.com/D3292
Summary:
You can now embed countdowns in Remarkup! Not sure what it's
useful for, but there you have it.
Also I may have made a hash of the markup code; I don't really know what
I'm doing.
Test Plan: Make a new countdown, put `{C###}` in a Differential comment.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Maniphest Tasks: T1053
Differential Revision: https://secure.phabricator.com/D3290
Summary:
Create `phpast.{version,getast}` methods for calling xhpast
with `--version` and with source code as input, respectively.
Test Plan:
Run `arc call-conduit` a bunch of times; delete xhpast; run
it a bunch more times.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Maniphest Tasks: T1534
Differential Revision: https://secure.phabricator.com/D3289
Summary:
We will need to process them.
Maybe it will fit better in Repository application but we don't have it yet.
Test Plan:
$ ./fact cursors
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin, royw
Differential Revision: https://secure.phabricator.com/D3287
Summary: This is arguably a more useful view than listing all daemons.
Test Plan: Looked at list, only saw daemons that haven't exited
Reviewers: vrana, epriestley
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D3286
Summary:
We spend 6.37 s in this condition on a big diff.
By adding the 'S' flag, the time is down to 2.15 s.
Test Plan: `DifferentialRevisionEditor::newRevisionFromConduitWithDiff()`
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D3284
Summary:
See D3277, D3278.
- Sprite all the menu icons.
- Delete the unsprited versions.
- Notification bolt now uses the same style as everything else.
Test Plan: Looked at page, hovered, clicked things.
Reviewers: btrahan, chad, vrana
Reviewed By: vrana
CC: aran
Maniphest Tasks: T1569
Differential Revision: https://secure.phabricator.com/D3279
Summary:
Trigger the crossreference behavior on code blocks. Limited to
Differential, where we know what the project is, but includes regular
comments, inline comments, and previews of both.
(Hopefully event handlers on deleted elements also get deleted, so we
don't leak memory? Also, caching is a problem, and I didn't find a way
to mark existing cache entries as stale, like
`DifferentialChangesetParser::CACHE_VERSION`...)
Test Plan:
Load Differential revision, make lots of comments, click on
things.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Maniphest Tasks: T1602
Differential Revision: https://secure.phabricator.com/D3283
Summary: no need to get all O(N) up in this when we can do constant time of "8"
Test Plan: arc lint
Reviewers: vrana, epriestley
Reviewed By: vrana
CC: aran, Korvin
Maniphest Tasks: T891
Differential Revision: https://secure.phabricator.com/D3271
Summary:
Many images in Differential changesets are icons designed for
use on dark backgrounds. This makes them invisible on Differential's
white background. This adds an option to use a darker background
instead so you can see the images.
Currently this behavior is triggered on hover. (Also, it's a rather
garish fuchsia.) It seems fine UX-wise but I'm not totally sure of it.
Test Plan:
Load diff containing grippy_texture. Marvel at the grippy
fuchsia.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D3272
Summary: ...just in case that stuff happens in the "wild". also cleaned up the logic here since we no longer have the conduit conditionality.
Test Plan: made sure I didn't break JS on the site. reasoned about logic of my function and asking people PHP typing questions in job interviews.
Reviewers: epriestley, vrana
Reviewed By: epriestley
CC: aran, Korvin
Maniphest Tasks: T891
Differential Revision: https://secure.phabricator.com/D3269
Summary:
- Add getHelpURI() to PhabricatorApplication for application user guides.
- Add a new "help" icon menu item and skeletal Diviner application.
- Move help tabs to Applications where they exist, document the other ones that don't exist yet.
- Grep for all tab-related stuff and delete it.
Test Plan: Clicked "help" for some apps. Clicked around randomly in a bunch of other apps.
Reviewers: btrahan, vrana
Reviewed By: btrahan
CC: aran
Differential Revision: https://secure.phabricator.com/D3267
Summary: Make it possible to get to stuff that used to be in tabs.
Test Plan: Clicked links and such.
Reviewers: btrahan, vrana
Reviewed By: btrahan
CC: aran
Differential Revision: https://secure.phabricator.com/D3264
Summary: Move to application navigation, make it possible to get to /logs/ from the navigation.
Test Plan: Hit all interfaces, verified email.
Reviewers: btrahan, vrana
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T1569, T631
Differential Revision: https://secure.phabricator.com/D3261
Summary: 'cuz we don't need it and it's lame complexity for API clients of all kinds. Rip the band-aid off now.
Test Plan: used conduit console and verified no more shield. also did some JS stuff around the suite to verify I didn't kill JS
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Maniphest Tasks: T891
Differential Revision: https://secure.phabricator.com/D3265
Summary: We should probably do this also in `differential.creatediff` but it's not a big deal because we later call `differential.updaterevision` which does this using `DifferentialRevisionEditor`.
Test Plan: None.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D3263
Summary: default check the system send prefernce for immediateness and add more direct text about dameons, with a link to help.
Test Plan: looks good
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Maniphest Tasks: T726
Differential Revision: https://secure.phabricator.com/D3262
Summary:
Currently, we have a hard-coded list of settings panels. Make them a bit more modular.
- Allow new settings panels to be defined by third-party code (see {D2340}, for example -- @ptarjan).
- This makes the OAuth stuff more flexible for {T887} / {T1536}.
- Reduce the number of hard-coded URIs in various places.
Test Plan: Viewed / edited every option in every panel. Grepped for all references to these URIs.
Reviewers: btrahan, vrana, ptarjan
Reviewed By: btrahan
CC: aran
Differential Revision: https://secure.phabricator.com/D3257
Summary:
We currently have two relatively distinct applications, "People" and "Settings", living in /people/. Move settings to its own directory.
This renames a couple of classes but makes no real code changes.
Test Plan: Browsed /settings/, changed some settings.
Reviewers: btrahan, vrana
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T1569, T631
Differential Revision: https://secure.phabricator.com/D3256
Summary:
- Add an Application.
- Move routes to the application.
- Move nav out of tabs (which no longer exist).
- Fix a couple of random things.
Test Plan: Viewed sent/received mail logs. Performed send/receive tests. Viewed email details.
Reviewers: btrahan, vrana
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T631, T1569
Differential Revision: https://secure.phabricator.com/D3255
Summary: Add some code from a random guy on the Internet.
Test Plan: Upload a PNG file with alpha; check thumbnail.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Maniphest Tasks: T1646
Differential Revision: https://secure.phabricator.com/D3258
Summary:
Fact engines loading dependent objects are super slow because they load them one by one.
This diff put each page in a Lisk set allowing engines to use `loadRelatives()`.
It also introduces `clearSet()` method which is somewhat neccessary in PHP < 5.3 or with disabled cyclic [[ http://php.net/gc | GC ]].
Test Plan:
$iterator = new PhabricatorFactUpdateIterator(new DifferentialRevision());
foreach ($iterator as $revision) {
$diffs = $revision->loadRelatives(new DifferentialDiff(), 'revisionID');
echo memory_get_usage() . "\n";
}
Experienced not-steadily-increasing memory usage and much faster loading.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D3247
Summary:
There's currently no way to get here from the UI since nav tabs don't exist anymore. It's also always been hard to find this feature even when we had the tabs, since it's surprising that it's inside "MetaMTA".
- Move mailing lists to a separate application.
- Add `buildApplicationPage()`, since we don't really need `buildStandardPageResponse()` any more -- we can infer all the information from `PhabricatorApplication`. This will let us get rid of a lot of the `PhabricatorXXXController` classes which just define application information.
- Add `getApplicationURI()` to reduce code duplication, and in case we want to let you move applications around some day.
Test Plan: Looked/edited/saved mailing lists.
Reviewers: btrahan, vrana
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T631
Differential Revision: https://secure.phabricator.com/D3248
Summary:
If a repository is configured with a custom encoding, it wasn't respected by DiffusionGitFileContentQuery making all views with
non-UTF8 characters fail. Check if we have a custom encoding and encode if any it set.
NOTE: This only works for Git repositories.
Test Plan: Browsed a repository with custom encoding before and after this patch.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Maniphest Tasks: T452
Differential Revision: https://secure.phabricator.com/D3251
Summary: See D3252.
Test Plan: This one is nasty to test, I'm going to make some coffee first.
Reviewers: davidreuss, vrana, btrahan
Reviewed By: davidreuss
CC: aran
Maniphest Tasks: T452
Differential Revision: https://secure.phabricator.com/D3254
Summary:
Add a new left-side application menu. This menu shows which application you're in and provides a quick way to get to other applications.
On desktops, menus are always shown but the app menu can be collapsed to be very small.
On tablets, navigation buttons allow you to choose between the menus and the content.
On phones, navigation buttons allow you to choose between the app menu, the local menu, and the content.
This needs some code and UI cleanup, but has no effect yet so I think it's okay to land as-is, I'll clean it up a bit as I start integrating it. I want to play around with it a bit and see if it's good/useful or horrible anyway.
Test Plan: Will include screenshots.
Reviewers: vrana, btrahan, chad
Reviewed By: btrahan
CC: aran, alanh
Maniphest Tasks: T1569
Differential Revision: https://secure.phabricator.com/D3223
Summary:
Currently, we're showing projets in reverse order (Z..A) because most cursor pagers go from high IDs to low IDs.
Allow sequence to be reversed; reverse it.
Also simplify some query/paging stuff.
Test Plan: Set page size to 1, paged back and forth.
Reviewers: vrana, btrahan
Reviewed By: vrana
CC: aran
Maniphest Tasks: T603
Differential Revision: https://secure.phabricator.com/D3221
Summary:
- In ProjectQuery, always load the viewer's membership in the project because we need it to perform a CAN_VIEW test.
- Add storage for the view, edit and join policies.
- A user can always view a project if they are a member.
- A user can always join a project if they can edit it.
- Editing a project requires both "view" and "edit" permissions, and edit does not imply view.
- This has no effect on the application yet.
Test Plan: See next diff.
Reviewers: vrana, btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T603
Differential Revision: https://secure.phabricator.com/D3219
Summary:
- Allow PolicyQuery to require specific sets of capabilities other than "CAN_VIEW", like edit, etc. The default set is "view".
- Add some convenience methods to PolicyFilter to test for capabilities.
Test Plan: Viewed pastes, projects, etc. Used other stuff in future diff.
Reviewers: vrana, btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T603
Differential Revision: https://secure.phabricator.com/D3212
Summary:
I ran into a case where a commit isn't "new" but hasn't been closed. I
think the check on the status of the differential revision should be
enough and this check isn't needed.
Test Plan:
used the reparse.php script to close a revision that previously wouldn't
close.
Reviewers: epriestley, vrana
Reviewed By: vrana
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D3232
Summary:
- Use @chad's nice gradient overlay icons.
- Show selected states.
- Use profile picture for profile item (not sure about this treatment?)
- Workflow the logout link
Test Plan: Will add screenshots.
Reviewers: alanh, btrahan, chad
Reviewed By: btrahan
CC: aran
Differential Revision: https://secure.phabricator.com/D3225
Summary:
There's some good feedback from Wikimedia here:
http://lists.wikimedia.org/pipermail/wikitech-l/2012-August/062252.html
Try to improve on some of it. In particular:
- Make it clear that /arcanist/ is not where you should be (D3235).
- Provide better connections from "Arcanist User Guide" to other documents.
- Provide a "Quick Start" guide with a simpler set of instructions that links to richer documentation.
- Reorganize the project setup guide to put more important things earlier on.
- Make it clear that you should commit `.arcconfig`.
- Provide more hints for initial setup.
- Describe and organize advanced configuration/extension documentation as more clearly separate from basic setup/install documentation.
Test Plan: Generated, read docs.
Reviewers: vrana, btrahan
Reviewed By: vrana
CC: aran
Differential Revision: https://secure.phabricator.com/D3236
Summary:
We currently cache all connections in LiskDAO so we can roll back transactions when fixtured unit tests complete.
Since we establish a new connection wrapper each time we establish a global lock, this cache currently grows without bound.
Instead, pool global lock connections so we never have more than the largest number of locks we've held open at once (in PullLocalDaemon, always 1).
Another way to solve this is probably to add an "onclose" callback to `AphrontDatabaseConnection` so that it can notify any caches that it been closed. However, we currently allow a connection to be later reopened (which seeems reasonable) so we'd need a callback for that too. This is much simpler, and this use case is unusual, so I'd like to wait for more use cases before pursing a more complicated fix.
Test Plan:
Ran this in a loop:
while (true) {
for ($ii = 0; $ii < 100; $ii++) {
$lock = PhabricatorGlobalLock::newLock('derp');
$lock->lock();
$lock->unlock();
}
$this->sleep(1);
}
Previously it leaked ~100KB/sec, now has stable memory usage.
Reviewers: vrana, nh, btrahan
Reviewed By: vrana
CC: aran
Maniphest Tasks: T1636
Differential Revision: https://secure.phabricator.com/D3239
Summary:
Ponder is similar in spirit to the Wiki tool, but uses a Q&A
format and up/downvotes to signal user sentiment. Popular
questions are moved to the top of the feed on a 5-minute
cycle based on age (younger is better) and vote count (higher
is better).
Pre-apologies for noob diff.
Test Plan:
- `./bin/phd list` Should include `PonderHeatDaemon`; phd launch it
if necessary.
- Navigate to /ponder/ ; observe sanity when adding questions,
voting on them, and adding answers.
- Confirm that questions and answers are linkable using Q5 / Q5#A5 formatted object links.
- Confirm that searching for Ponder Questions works using built-in
search.
Feedback on code / schema / whatever organization very welcome.
Reviewers: nh, vrana, epriestley
Reviewed By: epriestley
CC: gmarcotte, aran, Korvin, starruler
Differential Revision: https://secure.phabricator.com/D3136
Summary:
We can use `.gitattributes` instead but there's no way how to set repository config for all users in Git, right?
So provide a script writting to `.git/info/attributes` instead so that we don't have to .gitignore `.gitattributes`.
Test Plan:
$ scripts/celerity/install_merge.sh
$ git pull # with merge conflict in Celerity map
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D3228
Summary: Apparently I am not qualified to do basic math.
Test Plan: Unit test.
Reviewers: vrana
Reviewed By: vrana
CC: aran
Differential Revision: https://secure.phabricator.com/D3218
Test Plan:
$ ./reindex_all_users.php
Search for me in open documents.
Search for @epriestley in open documents.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D3216
Summary: Make the policy control accept a more sensible set of inputs. (This currently has no callsites.)
Test Plan: Used in future diff.
Reviewers: vrana, btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T603
Differential Revision: https://secure.phabricator.com/D3211
Summary:
I need this to set "disabled" on some menu items that are policy-restricted.
NOTE: This is getting gross and I promise to clean it up with the new side nav stuff.
Test Plan: Added "Disabled" to some items, they became disabeld.
Reviewers: vrana, btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T603
Differential Revision: https://secure.phabricator.com/D3203
Summary: rather than showing an erroneous "we still parsing" message.
Test Plan: for each version control system, typed in a garbage URL and got a 4oh4. (note this actually fails for SVN -- see comment about how my code fails atm -- and DiffusionGitRequest seems to pick off this error in advance and returns an AphrontUsageException.)
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Maniphest Tasks: T1624
Differential Revision: https://secure.phabricator.com/D3201
Summary: See discussion in D3078 for why I've separated this. Pretty sure it's not quite ready yet -- I want to build a couple of things on it so we have a better idea of what we need (autoincrement ID? <factType, objectA, epoch> primary key? objectB column? valueZ?) and don't need to do a ton of schema patches.
Test Plan: Applied patches, ran D3078.
Reviewers: vrana, btrahan, majak
Reviewed By: vrana
CC: aran
Maniphest Tasks: T1581, T1562
Differential Revision: https://secure.phabricator.com/D3088
Summary: I think this is simpler? Includes test cases.
Test Plan: Ran tests. Loaded /paste/.
Reviewers: vrana, nh
Reviewed By: vrana
CC: aran
Differential Revision: https://secure.phabricator.com/D3209
Summary: See title. Adds features needed for D3136.
Test Plan:
Observe sanity (or run D3136 in a sandbox
and observe that voting works).
Reviewers: epriestley
Reviewed By: epriestley
CC: gmarcotte, aran, Korvin
Differential Revision: https://secure.phabricator.com/D3205
Summary:
Put the function in the base class so all the Diffusion views
can use it. Also use shinier tooltips.
Test Plan: Browse Diffusion.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D3206
Summary: We managed to move enough Owners stuff aside to make this reasonable; make projects implement the policy interface and projectquery use cursor-based paging.
Test Plan:
- Grepped for ProjectQuery callsites.
- Created an audit comment.
- Used `project.query` to query projects.
- Loaded homepage.
- Viewed Maniphest task list, grouped by project.
- Viewed project list.
- Created / edited project.
- Browsed Owners.
Reviewers: vrana, btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T603
Differential Revision: https://secure.phabricator.com/D3200
Summary: A cursory look at DifferentialReviewer suggests the primary reviewer doesn't actually have to be among the reviewers? Uploading this so bug reporter can patch and see if it helps.
Test Plan: Nope.
Reviewers: epriestley
Reviewed By: epriestley
CC: szymon, aran, Korvin
Maniphest Tasks: T1625
Differential Revision: https://secure.phabricator.com/D3198
Summary:
This is a step toward unearthing Project queries enough that I can make them policy-aware. Right now, some ProjectQuery callsites do not have reasonable access to the viewer. In particular, Owners packages need to issue Project queries because we allow projects to own packages and resolve project members inside of some package queries.
Currently, we have a very unmodern approach to querying packages, with a large number of one-off static load methods:
PhabricatorOwnersPackage::loadAffectedPackages()
PhabricatorOwnersPackage::loadOwningPackages()
PhabricatorOwnersPackage::loadPackagesForPaths()
PhabricatorOwnersOwner::loadAllForPackages()
PhabricatorOwnersOwner::loadAffiliatedUserPHIDs()
PhabricatorOwnersOwner::loadAffiliatedPackages()
ConduitAPI_owners_query_Method::queryAll()
ConduitAPI_owners_query_Method::queryByOwner()
ConduitAPI_owners_query_Method::queryByAffiliatedUser()
ConduitAPI_owners_query_Method::queryByPath()
We should replace `PhabricatorOwnersOwner` with an Edge and move all of these calls to a Query class. I'm going to try to do as little of this work as I can for now since I'm much more interested in getting a functional policy implementation into other applications, but ProjectQuery needs to be policy-aware before I can do that and I need to dig some at least some of the callsites out enough that I can get a viewer in there without making the code worse than it is.
This adds a PhabricatorOwnersPackageQuery class and removes one callsite of one of those static methods.
I also intend to dissolve the two separate concepts of an "owner" (direct owner) and an "affiliated user" (indirect owner via project membership) since I think we're always fine with "affiliated users" owners.
Test Plan: Loaded home page / audit tool, which use the modified path. Ran queries manually via script. Made sure results included directly owned packages and packages owned through project membership.
Reviewers: vrana, btrahan, meitros
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T603
Differential Revision: https://secure.phabricator.com/D3193
Summary:
A few goals here:
- Slightly simplify the Query classtree -- it's now linear: `Query` -> `OffsetPagedQuery` (adds offset/limit) -> `PolicyQuery` (adds policy filtering) -> `CursorPagedPolicyQuery` (adds cursors).
- Allow us to move from non-policy queries to policy queries without any backward compatibility breaks, e.g. Conduit methods which accept 'offset'.
- Separate the client limit ("limit") from the datafetch hint limit ("rawresultlimit") so we can make the heurstic smarter in the future if we want. Some discussion inline.
Test Plan: Expanded unit tests to cover offset behaviors.
Reviewers: vrana, btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T603
Differential Revision: https://secure.phabricator.com/D3192
Summary:
- Commit detail view
- List of projects
- "edit" action which takes the user to a simple form where they can only add / remove projects.
- Integrated the project relationship into the commit search indexer
- fixed a bug from D790; it seems you must select the column if you're going to join against it later. Without this change searching for author or projectfails 100% for me.
Test Plan: added and removed projects. verified appropriate projects showed up in detail and edit view. searched for commits by project and found the ones I was supposed to...!
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Maniphest Tasks: T1614
Differential Revision: https://secure.phabricator.com/D3189
Summary:
When viewing Maniphest tasks grouped by project, there's this
weird algorithm that involves generating strings to use as sort keys.
It's pretty clearly wrong in several cases; this aims to fix it.
Test Plan:
Open Maniphest and try to sort by things. Unfortunately, I
don't have access to a decent Maniphest database, so I'm not certain it
works as it should.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin, mikaaay
Maniphest Tasks: T1595
Differential Revision: https://secure.phabricator.com/D3142
Summary:
In a revision with multiple diffs like
https://secure.phabricator.com/D3168?vs=6094&id=6095
clicking "Show Raw File (Left)" while comparing diffs 1 and 2 brings up
the version from base, instead of from diff 1. This is because the hunks
are stored as diffs between base and diff X, and the raw file is
generated from the hunks. This introduces a hack which is probably not
actually correct but seems to work for the 90% case.
(The "Show Raw File (Right)" button was and remains correct.)
Test Plan: Click raw file buttons while comparing different diffs.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D3169
Summary:
Put flag note in a Javelin tooltip on the flag, and extra
reviewers in a Javelin tooltip on the (+N). This is a bit more expensive
because we have to fetch all of their usernames but I'm hoping that's
okay?
Test Plan:
Load a bunch of revision lists. Mouse on. Mouse off. Mouse
on. Mouse off.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D3187
Summary:
- Store project members in edges.
- Migrate existing members to edge storage.
- Delete PhabricatorProjectAffiliation.
- I left the actual underlying data around just in case something goes wrong; we can delete it evenutally.
Test Plan:
- Ran migration.
- Created a new project.
- Joined and left a project.
- Added and removed project members.
- Manually called PhabricatorOwnersOwner::loadAffiliatedUserPHIDs() to verify its behavior.
Reviewers: vrana, btrahan
Reviewed By: vrana
CC: aran
Maniphest Tasks: T603
Differential Revision: https://secure.phabricator.com/D3186
Summary:
These are currently useless and confusing (they have no application impact), and should be migrated to edges if we want to restore them in some form.
I left the actual storage so this doesn't destroy any data, it just removes all traces of this feature from the UI.
Test Plan: Looked at and edited projects.
Reviewers: vrana, btrahan
Reviewed By: vrana
CC: aran
Maniphest Tasks: T603
Differential Revision: https://secure.phabricator.com/D3183
Summary:
I want to:
- move the membership storage to edges
- remove the concepts of "roles" (which are decorative text only) and "owners" (which will be replaced with policy-based controls)
This moves us a step closer to that by reducing the use of ProjectAffiliation outside of the class.
Test Plan: Loaded project profile. Called `project.query`. Joined and left a project.
Reviewers: vrana, btrahan
Reviewed By: vrana
CC: aran
Maniphest Tasks: T603
Differential Revision: https://secure.phabricator.com/D3182
Summary: This is clearer and more consistent with other Query classes.
Test Plan: Used home page, conduit api, project list, other interfaces.
Reviewers: vrana, btrahan
Reviewed By: vrana
CC: aran
Maniphest Tasks: T603
Differential Revision: https://secure.phabricator.com/D3179
Summary: I'm deprecating the concept of "owners" (which currently has no meaning in the actual application) in favor of policy-based controls. Remove the ability to query by it.
Test Plan: Grepped for setOwners(), no relevant hits. Browsed the project listing.
Reviewers: vrana, btrahan
Reviewed By: vrana
CC: aran
Maniphest Tasks: T603
Differential Revision: https://secure.phabricator.com/D3178
Summary:
I'm trying to make progress on the policy/visibility stuff since it's a blocker for Wikimedia.
First, I want to improve Projects so they can serve as policy groups (e.g., an object can have a visibility policy like "Visible to: members of project 'security'"). However, doing this without breaking anything or snowballing into a bigger change is a bit awkward because Projects are name-ordered and we have a Conduit API which does offset paging. Rather than breaking or rewriting this stuff, I want to just continue offset paging them for now.
So I'm going to make PhabricatorPolicyQuery extend PhabricatorOffsetPagedQuery, but can't currently since the `executeWithPager` methods would clash. These methods do different things anyway and are probably better with different names.
This also generally improves the names of these classes, since cursors are not necessarily IDs (in the feed case, they're "chronlogicalKeys", for example). I did leave some of the interals as "ID" since calling them "Cursor"s (e.g., `setAfterCursor()`) seemed a little wrong -- it should maybe be `setAfterCursorPosition()`. These APIs have very limited use and can easily be made more consistent later.
Test Plan: Browsed around various affected tools; any issues here should throw/fail in a loud/obvious way.
Reviewers: vrana, btrahan
Reviewed By: vrana
CC: aran
Maniphest Tasks: T603
Differential Revision: https://secure.phabricator.com/D3177
Summary:
Behave like Differential.
Also save one path ID query.
Test Plan: Displayed very large commit, clicked in ToC, clicked on Load.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D3176
Summary: Looks weird with long paths.
Test Plan: Displayed diff with moved code.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D3173
Summary:
- import_project_symbols supports an optional extra field, which is the
context of the symbol.
- Symbol query can take a symbol argument, either as a parameter or a
URL component (so you can now jump nav to `s Zerg/rush`, for
example).
- Conduit method not yet updated. Will do that later.
NOTE: Not providing a context is distinct from providing an empty
context, because an empty context stands for top-level context, i.e.
functions and classes for PHP. It will not find class methods, etc. It
is possible that we should use some weird token that could not normally
be a context name to stand in for empty context.
Test Plan: Do a bunch of symbol searches.
Reviewers: epriestley
Reviewed By: epriestley
CC: nh, aran, Korvin
Maniphest Tasks: T1602
Differential Revision: https://secure.phabricator.com/D3148
Summary:
In D3144, I made us look in application maps to find routing rules. However, we don't process //all// the maps when we 404 and try to do a "/" redirect. Process all of the maps.
Additionally, in D3146 I made the menu items application-driven. However, some pages (like 404) don't have a controller. Drop the requirement that the controller be nonnull.
Test Plan:
- Visited "/maniphest", got a redirect after this patch.
- Visited "/asldknfalksfn", got a 404 after this patch.
Reviewers: davidreuss, vrana, btrahan
Reviewed By: davidreuss
CC: aran
Maniphest Tasks: T1607
Differential Revision: https://secure.phabricator.com/D3158
Summary:
See T1602.
This is just the minimal functional patch; the scripts will continue
working because of the `DEFAULT ''`.
Test Plan:
Can't fully test this until I get more code working, but
nothing broke horribly yet.
Reviewers: epriestley
Reviewed By: epriestley
CC: nh, aran, Korvin
Maniphest Tasks: T1602
Differential Revision: https://secure.phabricator.com/D3147
Summary: This is a tax for internationalization.
Test Plan: Displayed a revision with added and moved files.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D3166
Summary: Nobody knows that it's possible to highlight more lines because there's no interface for it.
Test Plan:
Highlighted:
- single line
- top to bottom
- bottom to top
- inside to outside
Reviewers: Korvin, epriestley
Reviewed By: epriestley
CC: aran, epriestley
Differential Revision: https://secure.phabricator.com/D3150
Summary: Allow custom LDAP port to be defined in config file
Test Plan: Test login works by specifying a custom port
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D3153
Summary:
This makes no changes, it just moves the menu icons to the applications instead of hard-coded on the page.
I'm going to try to address some of the angst in T1593 next...
Test Plan: Loaded logged-in / logged out pages. Clicked menu items. Looked at /applications/.
Reviewers: vrana, btrahan
Reviewed By: vrana
CC: aran
Maniphest Tasks: T1593, T1569
Differential Revision: https://secure.phabricator.com/D3146
Summary:
When we match an application route, select it as the current application and store it on the controller.
Move routes for the major applications into their PhabricatorApplication classes so this works properly.
Test Plan: Added a var_dump() and made sure we picked the right app for all these applications.
Reviewers: vrana, btrahan
Reviewed By: vrana
CC: aran
Maniphest Tasks: T1569
Differential Revision: https://secure.phabricator.com/D3144
Summary: The Log and PID directory should be separable in the config file
Test Plan: Start the daemons, and check if the pid and log files are stored in directories that were specified in the config file.
Reviewers: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D3149
Summary:
Show modified date on the right of the list view (which I guess
is also the created date, since pastes are immutable?)
Test Plan: Browse through the many volumes of untitled masterworks.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Maniphest Tasks: T1600
Differential Revision: https://secure.phabricator.com/D3145
Summary:
- Put the code to generate informational dicts about flags into the
base class.
- Update flag.delete to accept an object PHID in order to delete the
flag on that object, since currently the model is that each object
may have at most one flag, and each flag has exactly one object,
although the former is not enforced.
- Add flag.edit, which creates or updates a flag, optionally with the
given color and note.
Test Plan:
Spend endless hours repeatedly running arc call-conduit and
arc flag.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D3141
Summary: as title
Test Plan: tested without params. Tested with single known path
Reviewers: epriestley, vrana, nh
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D3139
Summary:
Allow sorting tasks by title in addition to priority, updated,
created.
Test Plan:
Load Maniphest, click between order buttons, note that tasks
are being ordered correctly, as if by magic.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Maniphest Tasks: T1592
Differential Revision: https://secure.phabricator.com/D3137
Summary:
- Adds a new "Applications" application.
- Builds an application list via application config instead of via hard-coding, so we can move toward better concepts of installing/uninstalling applications, etc.
- Applications indicate that they need attention with notice counts and brief status messages rathern than 50 giant tables of all sorts of app data.
I want to try replacing the home screen with this screen, pretty much. Not sure if this is totally crazy or not. What does everyone else think?
Test Plan: Will add screenshots.
Reviewers: btrahan, chad, vrana, alanh
Reviewed By: vrana
CC: aran, davidreuss, champo
Maniphest Tasks: T1569
Differential Revision: https://secure.phabricator.com/D3129
Summary:
Unlike (all? most?) other tables, the flag table has two wide
columns: the object name and the flag note. This fiddles with the
classes so neither gets squished too much by the other.
This is kind of a hack and I don't even know if it's cross-browser
compatible because I only have WebKit here. But maybe it's fine.
Test Plan:
View Differential home page while changing revision names and
flag notes to weird things.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Maniphest Tasks: T1586
Differential Revision: https://secure.phabricator.com/D3128
Summary:
This lets you delete inlines from the preview at the bottom of
the page, instead of hunting for them through the diffs.
There is not yet a keyboard shortcut.
The mechanism for updating the inlines in the diffs is kind of a hack
and I'm sure I'm special-casing way too much, but at least it works.
Test Plan:
Load revision with many diffs. Create inlines all over the
place. Delete them all. Mwahaha.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Maniphest Tasks: T1433, T1431
Differential Revision: https://secure.phabricator.com/D3131
Summary:
If the user has an editor configured, an Edit link appears next
to the History link.
Somewhat suboptimally, the column is still there if there are no edit
links, it's just empty. I don't know if it matters...
Test Plan: Load Diffusion pages while changing editor setting in preferences.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin, avivey
Maniphest Tasks: T1558
Differential Revision: https://secure.phabricator.com/D3124
Summary:
Firefox has a limit of ~500,000 elements which can be passed in literal array.
This amount of data is meaningless anyway because even Retina displays don't have such resolution.
Limit the amount of data to mitigate browser limitations and also reduce the page size.
Ensure that first and last element is passed.
I considered also reducing the granularity to days but I want new repositories to have nice precise graph.
Test Plan: Displayed the chart in Firefox.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D3134
Summary: See D3125#3
Test Plan:
Well, apparently the FB test Phabricator has no other flags
in it, so I modified the database by hand... the changed revision was
not marked as flagged in Differential.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Maniphest Tasks: T1557
Differential Revision: https://secure.phabricator.com/D3130
Summary:
To make it easier to monitor daemons, let's store their current state
(running, died, exited, or unknown) to the db. The purpose of this is to
provide more information on the daemon console about the status of daemons,
especially when they are running on multiple machines. This is mostly backend
work, with only a few frontend changes. (It is also dependent on a change
to libphutil.)
These changes will make dead or stuck daemons more obvious, and will allow
more work on the frontend to hide daemons (and logs) that have exited cleanly,
i.e. ones we don't care about any more.
Test Plan:
- run db migration, check in db that all daemons were marked as exited
- start up a daemon, check in db that it is marked as running
- open web interface, check that daemon is listed as running
- after daemon has been running for a little bit, check in db that dateModified
is being updated (indicating daemon is properly sending heartbeat)
- kill -9 daemon (but don't run bin/phd yet), and check that db still shows it
as running
- edit daemon db entry to show it as being on a different host, and backdate
dateModified field by 3 minutes, and check the web ui to show that the status
is unknown.
- change db entry to have proper host, check in web ui that daemon status is
displayed as dead. Check db to see that the status was saved.
- run bin/phd stop, and see that the formerly dead daemon is now exited.
Reviewers: epriestley, vrana
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D3126
Summary:
The tables are currently kind of wide, and the emails for
non-recognized users seem like the least useful parts, so I put them in
tooltips so they're only visible if you want them. On the other hand, it
means you can only view one at a time, and if you're on mobile you can't
see them at all. Overall, not sure whether this is a good idea.
Test Plan:
Load Diffusion pages with some recognized and some
unrecognized users. Hover over the latter and see that emails appear.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D3123
Summary:
Currently, on secure.phabricator.com, if you type "e" we generate about 600 users and ship them over the wire. This takes ~300ms.
Instead, limit the results to a superset of what the client will actually show.
Test Plan: Ran user typeahead queries, tweaked limit to 1.
Reviewers: vrana, btrahan
Reviewed By: vrana
CC: aran
Differential Revision: https://secure.phabricator.com/D3121
Summary:
Put a flag icon to the left of each flagged revision in the
Differential summary tables. Flag is colored correctly and when hovered
reveals the note in a tooltip.
As epriestley specifically notes that the Flagged Revisions table should
only be shown when a user is looking at their own revisions, maybe this
ought to be limited by what controller is using this view, or something.
Test Plan:
View Differential main page. Check that flags appear
correctly if some revisions are flagged.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin, avivey
Maniphest Tasks: T1557
Differential Revision: https://secure.phabricator.com/D3125
Summary:
- Include symbols in main typeahead results.
- Simplify the symbol query a bit and extend PhabricatorOffsetPagedQuery. There was some stuff around language ranking that I got rid of, I think the theory there was that mapping file extensions to languages might not work in general but I think it works fine in practice, and we have more config stuff now around guessing languages and getting the mappings right.
- Make it easier to debug the typeahead by showing the results in page format for non-ajax requests.
- When we have too many results, show only the top few of each type.
Depends on D3116, D3117
Test Plan: Used typeahead, got symbols in results. Hit endpoint with non-ajax, got useful debug view.
Reviewers: btrahan, vrana
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T1569
Differential Revision: https://secure.phabricator.com/D3118
Summary:
Always order applications first, then users, then other results (currently, there are no other results, of course).
(This is similar to the general ordering algorithm used in JX.Prefab but has enough current/future differences that I split it rather than trying to share them.)
Test Plan: Queried results, verified order.
Reviewers: vrana
Reviewed By: vrana
CC: aran
Maniphest Tasks: T1569
Differential Revision: https://secure.phabricator.com/D3117
Summary: If a change copies some file `A` to `B` and also edits `A`, we currently record this as an indirect change and don't show the edits to `A` in the diff. Instead, record these as direct changes.
Test Plan: Created two commits, one which copied `A` to `B` without modifying `A` and one which copied `A` to `B` and modified A. Viewed both commits in Diffusion. The unmodified commit did not show `A`, and the modified commit did (with the correct changes).
Reviewers: btrahan, vrana
Reviewed By: vrana
CC: champo, aran
Differential Revision: https://secure.phabricator.com/D3120
Summary:
This allows the nav to be laid out with divs instead of tables and for the navigation column to be made flexible. Design is non-final, this is just a step toward reactive menus that work on tablets/phones and an application menu.
I'm going to play around with flexible nav and document navigation and see if that goes anywhere.
Test Plan: Will attach screenshots.
Reviewers: btrahan, vrana, chad
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T1569
Differential Revision: https://secure.phabricator.com/D3114
Summary:
Added the boxes.
NOTE: I am not sure how to deal with the user choosing a minimum higher than the maximum; it causes an empty result set, but if we can avoid allowing it, that'd be better, I think.
Test Plan: See the boxes there, not filtering. Change them, see them filtering.
Reviewers: epriestley
CC: aran, Korvin
Maniphest Tasks: T1565
Differential Revision: https://secure.phabricator.com/D3109
Summary: See discussion in D3103. We don't need this for now; if we do in the future we should probably use an alternate implementation.
Test Plan: Grepped for 'placeholder', viewed UI examples.
Reviewers: vrana
Reviewed By: vrana
CC: aran
Maniphest Tasks: T1569
Differential Revision: https://secure.phabricator.com/D3115
Summary:
This needs a bunch of refinement but pretty much works. Currently shows only users and applications. Plans:
- Show actual search results too.
- Clean up the datasource endpoint so it's less of a mess.
- Make other typeaheads look more like this one.
- Improve sorting.
- Make object names hit the named objects as the first match.
Test Plan: Will attach screenshots.
Reviewers: btrahan, vrana, chad
Reviewed By: vrana
CC: aran
Maniphest Tasks: T1569
Differential Revision: https://secure.phabricator.com/D3110
Summary:
Each query takes over 2 seconds in FBCODE.
I didn't find a way how to speed them up.
There's also no easy way how to parallelize them at least.
So AJAX is the last instance.
Test Plan: Loaded commit with one branch and no tag.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D3112
Summary:
We have some false positives on commit changes checker.
I'm not sure if the reason is a difference between `git diff` and `svn diff` or something else but making this more robust doesn't harm anything.
We couldn't make the files from the whole changeset because I want to ignore context bigger than `$num_lines` to reduce rebase noise.
Test Plan:
Ran the method on diff which had false positive previously.
Ran the method on a diff with real change.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D3107
Summary:
- Looks better (can probably still use some tweaks), especially search.
- Moves logout from weird footer location to main menu.
- Reactive: on tablets and phones, the menu adjusts to remain useful.
- Fixed position on desktops for future side nav changes.
- Adds an icon header thing that's currently hard-coded but will be application-driven soon.
Test Plan: Used menu on desktop, tablet, phone, logged in / logged out, toggled darkconsole. Will add some screenshots.
Reviewers: btrahan, chad
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T1569
Differential Revision: https://secure.phabricator.com/D3105
Summary: The new menu stuff needs this but it was easy to pull out on its own.
Test Plan: Cliked UI example buttons.
Reviewers: btrahan, chad
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T1569
Differential Revision: https://secure.phabricator.com/D3104
Summary:
Support placeholder text for inputs. We currently don't use this because it requires JS and doesn't degrade (no JS means you have zero idea what the input is for if it isn't separately labeled) but there are some cases where intent is obvious from context (for example, the search input in the menu bar, which is fairly obvious on its own and will soon have a magnifying glass icon) and in such cases it's much prettier and saves a bunch of space over an explicit label. Add a behavior so we can add placeholders where they make sense.
This implementation is somewhat sanity-checked agianst the two jQuery placeholder implementations I was able to google:
https://github.com/danielstocks/jQuery-Placeholder/https://github.com/mathiasbynens/jquery-placeholder
Since we don't currently have any uses cases, I haven't included support for making JS access to the `value` work, for password inputs, or for dynamically altering the placeholder.
Test Plan: Played around with the placeholder in the UI example in various browsers and couldn't break it.
Reviewers: btrahan, chad
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T1569
Differential Revision: https://secure.phabricator.com/D3103
Summary: You need to use -- to separate arguments for phd and the daemon.
Test Plan: Ran with the extra --.
Reviewers: epriestley, vrana
Reviewed By: vrana
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D3106
Summary:
Add a 2x ("retina") sprite sheet with icons that I gave some hover/active effects. I'm just doing one sheet rather than separate 1x and 2x sheets, we can muck with it later but I don't think anyone's going to go over their bandwidth cap.
@chad, I'll put the PSD on the Dropbox too if you have a chance to give it a once-over.
Test Plan: Built menu on this, all the icons work.
Reviewers: btrahan, vrana, chad
Reviewed By: chad
CC: aran
Maniphest Tasks: T1569
Differential Revision: https://secure.phabricator.com/D3102
Summary:
- These don't fit anywhere in the new design.
- Even if we figure out how to fit them in, 220px logos definitely won't fit on the 320px iPhone screen so anyone who has a custom logo will have to rework them anyway.
- Kill it for now, and once we get the new design in and working maybe we can restore it somehow.
Test Plan: Loaded local install, no logo. Grepped for config.
Reviewers: btrahan, vrana
Reviewed By: vrana
CC: aran
Maniphest Tasks: T1569
Differential Revision: https://secure.phabricator.com/D3101
Summary: For any count fact, allow a chart to be drawn. INCREDIBLY POWERFUL DATA ANALYSIS PLATFORM.
Test Plan: Drew a chart of object counts. Drew the Maniphest burn chart.
Reviewers: vrana, btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T1562
Differential Revision: https://secure.phabricator.com/D3099
Summary:
- Add PhabricatorApplication. This is a general class that I have grand designs for, but used here to allow applications to provide objects for analysis by the facts appliction.
- Add FactCursors, to keep track of where iterators are.
- Make the daemon do something sort of useful.
- Add `bin/fact cursors` for showing and managing objects and cursors.
- Add some options to `bin/fact analyze`.
Test Plan:
- `bin/fact cursors`, `bin/fact cursors --reset DifferentialRevision`, `bin/fact cursors --reset X`
- `bin/fact analyze`, `bin/fact analyze --all`, `bin/fact analyze --iterator DifferentialRevision --skip-aggregates`
- `bin/phd debug fact`
Reviewers: vrana, btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T1562
Differential Revision: https://secure.phabricator.com/D3098
Summary: helping noobs help themselves
Test Plan: set $rows = array() and verified the txt. also threw a false && for my isAdmin conditional to check the other txt
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Maniphest Tasks: T1086, T1360
Differential Revision: https://secure.phabricator.com/D3100
After this commit: d9296638cd
I started getting this error:
Unhandled Exception ("Exception")
Bad getter call: getURIObject
It turns out that getURIObject just needed to be getRemoteURIObject and then the problem goes away.
Summary: Not totally sure about this but I think it's okay?
Test Plan: Loaded /fact/, got a more readable page.
Reviewers: vrana, btrahan
Reviewed By: vrana
CC: aran
Maniphest Tasks: T1562
Differential Revision: https://secure.phabricator.com/D3090
Summary:
In D3063, we stopped converting "user@domain:path" git-style URIs, but this broke the SSH-detection code and I missed that in my test plan because my test case uses natural SSH keys so the omission of SSH flags didn't cause failures.
This code is a bit of a mess anyway. Consolidate and refactor it to be a bit simpler, and add test coverage.
Test Plan: Ran unit tests. Ran "test_connection.php" in SSH and non-SSH modes, verified SSH modes generated appropriate ssh-agent commands around the git remote commands.
Reviewers: vrana, btrahan, tberman
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T1529
Differential Revision: https://secure.phabricator.com/D3093
Summary: Currently, if you have a task with project "X" and you apply a batch edit to it to remove "X", the action has no effect because we incorrectly skip the edit as a no-op. Instead, don't perform this check for edge edits.
Test Plan: Batch removed a project from several tasks with only one project.
Reviewers: vrana, btrahan
Reviewed By: vrana
CC: aran
Maniphest Tasks: T1566
Differential Revision: https://secure.phabricator.com/D3092
Summary:
Some facts are aggregations of other facts. For example, we may compute how many times each macro is used in each object as a "raw fact":
Dnnn uses macro "psyduck" 6 times.
But we want to present this data in aggregate form, e.g. "order macros by popularity". We can do this at runtime and it probably won't be too awful a query, but we can also aggregate it cheaply:
Macro "psyduck" is used 3920 times across all objects.
...and then do a query like "select macros ordered by usage".
"Aggregate" facts support facts like this. The aggregate facts I've implemented are:
- Count of all objects.
- Count of objects of type X.
- Last time facts were updated.
These clearly fit the "aggregate" facts template well. I'm not 100% sure macros do. We can use this table to answer a question like "What are the most popular macros, ordered by use?" We can also use it to answer a question like "What are the most popular macros in the last 6 months?", if we build a specific fact for that. But we can't use it to answer a question like "What are the most popular macros between times X and Y?". Maybe that's important; maybe not.
This seems like a good fit for at least some types of facts.
I'll de-magic the keys a bit in the next diff.
Test Plan: Ran the engines and got some aggregated facts about other facts.
Reviewers: vrana, btrahan
Reviewed By: vrana
CC: aran
Maniphest Tasks: T1562
Differential Revision: https://secure.phabricator.com/D3089
Test Plan: Jumped on correct line in SVN, Git and HG repos.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D3084
Summary:
Basic "Fact" application with some storage, part of a daemon, and a control binary.
= Goals =
The general idea is that we have various statistics we'd like to compute, like the frequency of image macros, reviewer responsiveness, task close rates, etc. Computing these on page load is expensive and messy. By building an ETL pipeline and running it in a daemon, we can precompute statistics and just pull them out of "stats" tables.
One way to do this is just to completely hard-code everything, e.g. have a daemon that runs every hour which issues a big-ass query and dumps results into a table per-fact or per fact-group. But this has a bunch of drawbacks: adding new stuff to the pipeline is a pain, various fact aggregators can't share much code, updates are slow and expensive, we can never build generic graphs on top of it, etc.
I'm hoping to build an ETL pipeline which is generic enough that we can use it for most things we're interested in without needing schema changes, and so that installs can use it also without needing schema changes, while still being specific enough that it's fast and we can build useful stuff on top of it. I'm not sure if this will actually work, but it would be cool if it does so I'm starting pretty generally and we'll see how far I get. I haven't built this exact sort of thing before so I might be way off.
I'm basing the whole thing on analyzing entire objects, not analyzing changes to objects. So each part of the pipeline is handed an object and told "analyze this", not handed a change. It pretty much deletes all the old data about that thing and then writes new data. I think this is simpler to implement and understand, and it protects us from all sorts of weird issues where we end up with some kind of garbage in the DB and have to wipe the whole thing.
= Facts =
The general idea is that we extract "facts" out of objects, and then the various view interfaces just report those facts. This change has on type of fact, a "raw fact", which is directly derived from an object. These facts are concerete and relate specifically to the object they are derived from. Some examples of such facts might be:
D123 has 9 comments.
D123 uses macro "psyduck" 15 times.
D123 adds 35 lines.
D123 has 5 files.
D123 has 1 object.
D123 has 1 object of type "DREV".
D123 was created at epoch timestamp 89812351235.
D123 was accepted by @alincoln at epoch timestamp 8397981839.
The fact storage looks like this:
<factType, objectPHID, objectA, valueX, valueY, epoch>
Currently, we supprot one optional secondary key (like a user PHID or macro PHID), two optional integer values, and an optional timestamp. We might add more later. Each fact type can use these fields if it wants. Some facts use them, others don't. For instance, this diff adds a "N:*" fact, which is just the count of total objects in the system. These facts just look like:
<"N:*", "PHID-xxxx-yyyy", ...>
...where all other fields are ignored. But some of the more complex facts might look like:
<"DREV:accept", "PHID-DREV-xxxx", "PHID-USER-yyyy", ..., ..., nnnn> # User 'yyyy' accepted at epoch 'nnnn'.
<"FILE:macro", "PHID-DREV-xxxx", "PHID-MACR-yyyy", 17, ..., ...> # Object 'xxxx' uses macro 'yyyy' 17 times.
Facts have no uniqueness constraints. For @vrana's reviewer responsiveness stuff, we can insert multiple rows for each reviewer, e.g.
<"DREV:reviewed", "PHID-DREV-xxxx", "PHID-USER-yyyy", nnnn, ..., mmmm> # User 'yyyy' reviewed revision 'xxxx' after 'nnnn' seconds at 'mmmm'.
The second value (valueY) is mostly because we need it if we sample anything (valueX = observed value, valueY = sample rate) but there might be other uses. We might need to add "objectB" at some point too -- currently we can't represent a fact like "User X used macro Y on revision Z", so it would be impossible to compute macro use rates //for a specific user// based on this schema. I think we can start here though and see how far we get.
= Aggregated Facts =
These aren't implemented yet, but the idea is that we can then take the "raw facts" and compute derived/aggregated/rollup facts based on the raw fact table. For example, the "count" fact can be aggregated to arrive at a count of all objects in the system. This stuff will live in a separate table which does have uniqueness constraints, and come in the next diff.
We might need some kind of time series facts too, not sure about that. I think most of our use cases today are covered by raw facts + aggregated facts.
Test Plan: Ran `bin/fact` commands and verified they seemed to do reasonable things.
Reviewers: vrana, btrahan
Reviewed By: vrana
CC: aran, majak
Maniphest Tasks: T1562
Differential Revision: https://secure.phabricator.com/D3078
Summary: Some people don't like these, so they should be able to turn them off.
Test Plan:
Toggled the setting on and off; loaded a page in diffusion and differential
that should have symbol cross-references, and saw that they weren't linked
when I had the setting disabled. I also checked that the symbols are still
linked when the setting hasn't been touched.
Reviewers: epriestley, vrana
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D3069
Summary: I will need this for tracking line number in Blame previous revision.
Test Plan:
$ hg diff --rev 0:1
$ svn diff -r 64382:64383 README
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D3083
Summary:
See title - This simply adds a checkbox to the "Edit User" page in the
admin view, to allow an administrator to re-send the "Welcome to Phabricator"
email.
Test Plan:
Sent myself another welcome email using the checkbox.
Created a new user using the admin panel, to make sure emails still get
sent for new users.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Maniphest Tasks: T1524
Differential Revision: https://secure.phabricator.com/D3081
Summary:
We need `$this->old` and `$this->new` in `renderShield()`.
Broken since D2358.
Test Plan: Loaded contents of file with lots of changes.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D3080
Summary:
The filename field and the checkbox to select the default image were
overlapping in Firefox on Linux on both the Project Edit page and the
Profile Edit page.
Test Plan: Looked at both of the pages and saw that they rendered better.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D3079
Summary: This iterator processes objects that have been updated.
Test Plan:
Ran this test script:
$cursor = null;
$table = new DifferentialRevision();
while (true) {
$iterator = new PhabricatorFactsUpdateIterator($table, $cursor);
foreach ($iterator as $new_cursor => $update) {
echo "{$new_cursor} => D".$update->getID()."\n";
$cursor = $new_cursor;
}
echo "Zzz...\n";
sleep(5);
}
Verified it iterated over every object and then stopped. Made a comment on a differenial revision, verified it iterated over the object after 15 seconds.
Reviewers: vrana, btrahan
Reviewed By: vrana
CC: aran
Maniphest Tasks: T1562
Differential Revision: https://secure.phabricator.com/D3077
Summary: PhutilBufferedIterator now implements all the nonspecific logic here.
Test Plan:
Created a test script like this:
$iterator = new LiskMigrationIterator(new DifferentialRevision());
$iterator->setPageSize(3);
foreach ($iterator as $key => $rev) {
echo "{$key}: ".$rev->getID()."\n";
}
Ran it and verified sensible iteration results.
Reviewers: vrana, btrahan
Reviewed By: vrana
CC: aran
Maniphest Tasks: T1562
Differential Revision: https://secure.phabricator.com/D3076
Summary: Also move the other tests up so they'll trigger when this stuff is touched.
Test Plan: liberate
Reviewers: nh, btrahan, vrana
Reviewed By: nh
CC: aran
Maniphest Tasks: T1283
Differential Revision: https://secure.phabricator.com/D3074
Summary:
- the current LDAP auth flow expects a DN to look like
cn=ossareh,ou=Users,dc=example,dc=com
- however many LDAP setups have their dn look something like
cn=Mike Ossareh,ou=Users,dc=example,dc=com
Test Plan:
Test if logins work with a LDAP setup which has cn=Full Name
instead of cn=username.
To test you should ensure you set the properties needed to
trigger the search before login as detailed in conf/default.conf.php
Reviewers: epriestley
CC: mbeck, aran, Korvin
Differential Revision: https://secure.phabricator.com/D3072
Summary: Currently, the logout link is this awkward form in the footer since we have to CSRF it. Enable a GET + dialog + confirm workflow instead so the logout link can just be a link instead of this weird mess.
Test Plan: Went to /logout/, logged out.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Differential Revision: https://secure.phabricator.com/D3066
Summary: I was poking around to see how this class worked and noticed this variable does nothing.
Test Plan: Careful inspection and reasoning that unused private member variable can be deleted.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D3068
Summary:
As we work through @chad's redesign, one thing I want to do is improve the tablet/mobile experience.
Add a "device" behavior which sets a "device-phone", "device-tablet" or "device-desktop" class on the root div. The behavior (device names, width triggers) is mostly based on Bootstrap.
Also adds a preview viewport=meta tag, which makes the iPhone not scale the page like crazy and is a desirable end state, but currently makes the app less usable since things get cut off.
Test Plan:
Added some classes like this:
.device-desktop {
background: blue;
}
.device-tablet {
background: orange;
}
.device-phone {
background: yellow;
}
...and loaded the site on a desktop, iPad and iPhone. Resized the window. Got the right background color in all cases.
Reviewers: btrahan, chad
Reviewed By: chad
CC: aran
Differential Revision: https://secure.phabricator.com/D3063
Summary:
This allows getting stats for any arbitrary period, e.g.
- everything
- last week
- week before last week
Test Plan: Ran the script for last week.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D3064
Summary:
The final goal is to display reviewers response time on homepage.
This is a building block for it.
The algorithm is quite strict - it doesn't count simple comment as response because reviewers would be able to cheat with comments like "I'm overwhelmed right now and will review next week".
We are more liberate in Phabricator where reviewers response with comments without changing the status quite often but I'm not trying to improve response times in Phabricator so this is irrelevant.
Reviewers in Facebook changes status more often (to clean their queue) so I follow this approach.
There is currently no way to track reviewers silently added and removed in Edit Revision but it's not a big deal.
The algorithm doesn't track commandeered revision, there's a TODO for it.
Response times are put in two buckets: `$reviewed` and `$not_reviewed`.
`$reviewed` contains reviewers who took action, `$not_reviewed` contains reviewers who didn't respond on time.
I will probably compute average time from `$reviewed` and raise it for those `$not_reviewed` that are higher than this average.
The idea is to not favor reviewers who were only lucky for being in a group with someone fast.
Test Plan: Passed test.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D3062
Summary: I want to link this page from outside.
Test Plan: /phid/?phids=PHID-USER-gsraq7yc66r4stl4c6u3
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D3060
Summary:
Currently, we have this cumbersome `PhabricatorRepositoryCommitMessageDetailParser` hook. This is really old and outdated; I want to just use the Differential custom field parser. See T945 for a specific application.
However, it allows installs to override author/committer association. Instead, provide an event hook for doing this.
Test Plan: Added a listener, made every commit resolve to "turtle", parsed some commits, verified the events looked sane and they now correctly were all attributed to "turtle".
Reviewers: btrahan, vrana, nh
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T1337
Differential Revision: https://secure.phabricator.com/D3040
Summary: Currently, MySQL/MySQLi connections store passwords in plain text on the object. Allow them to be stored in PhutilOpaqueEnvelopes instead. See D3053.
Test Plan: Loaded site.
Reviewers: vrana, btrahan
Reviewed By: vrana
CC: aran
Differential Revision: https://secure.phabricator.com/D3054
Summary: We pull "retries" and a doc link from PhabricatorEnv directly. Break these dependencies so the classes can move to libphutil.
Test Plan: Browsed site, triggered a schema exception and verified I still got the useful footer text.
Reviewers: btrahan, vrana
Reviewed By: vrana
CC: aran
Maniphest Tasks: T1283
Differential Revision: https://secure.phabricator.com/D3053
Summary: Add an explicit close() method to connections and call it in GlobalLock.
Test Plan:
Wrote a script like this:
$lock = PhabricatorGlobalLock::newLock('test');
echo "LOCK";
$lock->lock();
sleep(10);
echo "UNLOCK";
$lock->unlock();
sleep(9999);
Using `SHOW FULL PROCESSLIST`, verified the connection closed after 10 seconds with both the "MySQL" and "MySQLi" implementations.
Reviewers: btrahan, vrana
Reviewed By: vrana
CC: aran
Maniphest Tasks: T1470
Differential Revision: https://secure.phabricator.com/D3035
Summary:
The URL /jump/?jump=%s now does the same thing as the POST version,
except that if it couldn't jump to anything, it loads /jump/ with the
query filled in so you can just press enter (we don't save searches
without a CSRF token).
(hsb: Sorry for stealing your task! It hadn't been updated in two months
so I figured you were likely not actively working on it.)
Test Plan:
Loaded given URL with different queries (including various
flavors of nothing). Search worked as expected.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Maniphest Tasks: T1036
Differential Revision: https://secure.phabricator.com/D3047
Summary: Our auditors requested displaying this field and I can image that it can be useful.
Test Plan: /audit/
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D3044
Summary:
I want to move queryfx() and family to libphutil, for @chad and others (see T1283). We need to break a few dependencies to do this.
Since AphrontWriteGuard is independently useful, I broke the dependency between it and AphrontRequest rather than between Connection and WriteGuard. I'll move its implementation to libphutil in a future diff.
Test Plan: Loaded site, submitted CSRF form successfully, monkeyed with CSRF token, submitted CSRF form, got error.
Reviewers: btrahan, vrana
Reviewed By: vrana
CC: aran
Maniphest Tasks: T1283
Differential Revision: https://secure.phabricator.com/D3042
Summary: I changed this from `getName` to `getFullName` to make attached revisions, etc., render with "Dnnn", but accidentally made all users render as "username (Full Name)". Be a little more surgical in application of full names.
Test Plan: Created a task and attached a CC, a task and a revision. Verified the task and revision rendered with "Tnn", "Dnn" but the CC rendered as "username".
Reviewers: btrahan, vrana
Reviewed By: btrahan
CC: aran
Differential Revision: https://secure.phabricator.com/D3041
Summary: We currently omit email from Git author/committer lookups, which gives us some bad results when identify commit authors. Include email. Also simplify this block a little bit.
Test Plan: Ran "reparse.php --message" on several commits, verified that the author/committer seemed reasonable with var_dump()s.
Reviewers: btrahan, vrana, nh
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T1337
Differential Revision: https://secure.phabricator.com/D3039
Summary: This returns a list of PHIDs, not objects which need to be pulled.
Test Plan:
Repro'd fatal locally, verified it was fixed.
Repro steps are:
- Create an arc project associated with a repository, with indexed language(s) and subprojects.
- View a file in that repository.
Reviewers: nh, btrahan
Reviewed By: btrahan
CC: aran
Differential Revision: https://secure.phabricator.com/D3038
Summary:
See D3033, T1529. We currently transform "scp-style" `user@host:path` URIs into normal `ssh://user@host/path` URIs. This is undesirable for two reasons:
- The paths aren't always equivalent. They are for GitHub, which is why I missed this originally, but in the general case the ":path" is resolved relatively and the "/path" is resolved absolutely. So this transformation can break things.
- It confuses users, who do not think of "git@host:path" URIs as SSH URIs even though the SSH protocol is implied.
So stop using them, and just use the "git@host:path" URIs instead. This is a bit messy since we have some validation built up on top of URIs. Hopefully we can get rid of more of this in the future as we simplify repository management.
Test Plan: Unit tests cover this stuff pretty well. Made a new git repository with a "git@host:path" style URI and did pull/discover on it, verified the right URI was used.
Reviewers: btrahan, vrana
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T1529
Differential Revision: https://secure.phabricator.com/D3036
Summary: MetaMTA + daemons used to be pretty hard but @nh landed some patches a while ago that make it way eaiser. Back off the "ooh scary config" text in the documentation, since this option will just work for ~every install now.
Test Plan: Read it.
Reviewers: btrahan, vrana, nh
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T1525
Differential Revision: https://secure.phabricator.com/D3037
Summary: created a PhabricatorInlineCommentPreviewController so controllers in Diffusion and Differential respectively just have to handle the URI mapping and data loading like good little controllers.
Test Plan:
left inline comments on commits, deleted inline commits, submitted inline comments -- all worked well
did the same on some diffs
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Maniphest Tasks: T1176
Differential Revision: https://secure.phabricator.com/D3034
Summary:
- The most common workflow complaint I've seen recently is something like "how do I use Differential with a branch full of random code that me and several other developers all commit to"? There are some okay answers ("commandeer") but I think the best answer is "don't do that". Add a document explaining how development works at Facebook (and many other companies) without the use of feature branching, why it's better, and how you can lay the technical groundwork you need to to stop doing this.
- Add a general "smaller commits are better" and "your commit messsage should provide context" document.
- Minor updates to other stuff as my understanding of Mercurial has been refined.
Test Plan: Generated and read documentation.
Reviewers: btrahan, vrana, schrockn
Reviewed By: btrahan
CC: aran
Differential Revision: https://secure.phabricator.com/D3025
Summary: See detailed discussion in T1543.
Test Plan:
- Enabled multiplexing.
- Set user A to "enable Re".
- Created a task owned by user B with user A cc'd.
- Verified A got no "Re:" before this patch.
- Applied patch.
- Verified A got "Re:" after this patch.
Reviewers: nh, btrahan, vrana
Reviewed By: nh
CC: aran
Maniphest Tasks: T1543
Differential Revision: https://secure.phabricator.com/D3031
Summary: ...basically by pounding the DOM a bit to be a little closer to differential. I also make the "add comment" UI show up if and only if the commit is rendering properly.
Test Plan: prezzed "Z" on diffusion and noted it was like differential now
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Maniphest Tasks: T1177
Differential Revision: https://secure.phabricator.com/D3027
Summary: ...also swapped "status" and "order" so "status" is first, as in my testing it was sub-optimal to specifiy status (more of "what i want") after order ("how I want it")
Test Plan: ran various queries on my test instance via conduit console and the results all seem correct
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Maniphest Tasks: T1381
Differential Revision: https://secure.phabricator.com/D3028
Summary:
Various text boxes have a documentation link below them.
Make the Differential inline comment box one of them.
Test Plan:
Loaded Differential revision in sandbox. Played around
with inline comments.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Maniphest Tasks: T1513
Differential Revision: https://secure.phabricator.com/D3024
Summary:
- Add edges for this relationship.
- Use edges to store this data.
- Migrate old data.
- Fix some warnings with generating feed stories about Aux and Edge transactions.
- Fix a task-task edge issue with "Create Subtask".
Test Plan:
- Migrated data, verified reivsions showed up.
- Attached and detached tasks to revisions and vice versa.
- Created a new revision with attached tasks.
- Created a subtask.
Reviewers: btrahan, vrana
Reviewed By: btrahan
CC: aran
Differential Revision: https://secure.phabricator.com/D3018
Summary:
We do this in differential. To do this in diffusion, we need to know the
arcanist project (which I do by loading all possible projects for the
repository) and the language.
Test Plan:
load a php file in diffusion to see crossreferences; load a text file and
check darkconsole that it didn't try to crossreference.
Reviewers: epriestley, vrana, jungejason
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D3020
Summary: Theses are sort of silly anyway since they should all have the actor in them rather than being sentence fragments, but make them work OK for English at least. See D3013.
Test Plan:
Ran:
echo pht('added %d dependencie(s): %s', 1, 'derp')."\n";
echo pht('added %d dependencie(s): %s', 2, 'derp, derp')."\n";
Got:
added dependency: derp
added dependencies: derp, derp
Reviewers: vrana, btrahan
Reviewed By: vrana
CC: aran
Differential Revision: https://secure.phabricator.com/D3015
Summary: I spend most time in software development by being lazy.
Test Plan: Changed view, reloaded page, viewed the file without sending the form.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D3012
Summary:
introduced in D3006, D3007. we need a list of phids for revision and now that its attachments its always two way.
without this patch revisions don't show up on maniphest and attaching from either mani or diffu only has the attachment show up where you did it. (since two_way = false)
Test Plan: attached revisions and tasks to one another and verified things were showing up where they should
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D3011
Summary:
blogs are collections of posts. a blog also has metadata like a name, description and "bloggers" that can edit the metadata of the blog and contribute posts.
changes include the post edit flow where bloggers can now select which blogs to publish to. also made various small tweaks throughout the UI to make things sensical and clean as the concept of blogs is introduced.
there's edges powering this stuff. bloggers <=> blogs and posts <=> blogs in particular.
Test Plan:
made blogs, deleted blogs, tried to make blogs with no bloggers. all went well.
verified ui to publish only showed up for public posts, published posts to blogs, un-published posts to blogs, re-published posts to blogs, deleted posts and verified they disappeared from blogs.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Maniphest Tasks: T1373
Differential Revision: https://secure.phabricator.com/D3003
Summary: See D3006. Move this data to the edge store.
Test Plan:
- Created dependencies, migrated, verified dependencies were preserved.
- Added new dependencies, they worked.
Reviewers: vrana, btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T1162
Differential Revision: https://secure.phabricator.com/D3007
Summary:
- Use edges to store "X depends on Y" information in Maniphest.
- Show both "Depends On" and "Dependent Tasks".
- Migrate all the old edges.
Test Plan:
- Added some relationships, migrated, verified they were preserved.
- Added some new valid relationships, verified tasks got updated with sensible transactions and sent reasonable emails.
- Tried to add a cycle, got an ugly but effective error.
Reviewers: vrana, btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T1162
Differential Revision: https://secure.phabricator.com/D3006
Summary: This should simplify a bunch of stuff in D3006 and D3003.
Test Plan: Will update D3006.
Reviewers: btrahan, vrana
Reviewed By: btrahan
CC: aran
Differential Revision: https://secure.phabricator.com/D3010
Summary: See D3006, D3007. Make it easier to do migrations like that without holding all results in memory.
Test Plan:
Ran this code with an artificially small page size (2):
foreach (new LiskMigrationIterator(new DifferentialRevision()) as $rev) {
echo "Revision ".$rev->getID()."\n";
}
Verified each revision as loaded and processed.
Reviewers: vrana, btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T1162
Differential Revision: https://secure.phabricator.com/D3008
Summary:
This doesn't indicate which path is part of which package - I think it would be too heavy.
It just highlights the paths in a similar way as audits are highlighted.
Maybe we can use different colors for highlighting different packages and use them also in paths. We can mix the colors if one path is part of more packages :-).
Test Plan:
Viewed commit with 9 files and 4 packages where I am responsible only for one of them.
Verified that the only file in my package is highlighted.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin, haugen
Maniphest Tasks: T1226
Differential Revision: https://secure.phabricator.com/D2982
Summary: In order to perform the searches on Windows 2003 Server Active Directory you have to set the LDAP_OPT_REFERRALS option to 0
Test Plan: Test if LDAP works with Windows 2003 AD
Reviewers: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D3004
Summary: This keeps people in the correct To or CC field on multiplexed messages.
Test Plan:
with multiplexing on, checked that I received an email with me in the CC
field instead of the To field for a diff I'm CC'd on.
Reviewers: epriestley, jungejason, vrana
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D2999
Summary: The Graph API exposes a new field, security_settings, which allows applications to see whether a user has enabled Secure Browsing. This diff adds a configuration setting to Phabricator which forces users to have Secure Browsing enabled when logging in via Facebook.
Test Plan: With the configuration setting off, verify that secure browsing does not affect the ability to log in. With the configuration setting on and secure browsing off, verify that the login attempts is rejected. Then verify that the login attempt succeeds when secure browsing is enabled.
Reviewers: epriestley
Reviewed By: epriestley
CC: arice, aran, Korvin
Maniphest Tasks: T1487
Differential Revision: https://secure.phabricator.com/D2964
Summary: Blame can be slow.
Test Plan:
Viewed a file with no preference, saw blame.
Changed view, saw it.
Viewed a file, saw the changed view.
Viewed a file as raw document.
Reviewers: Two9A, epriestley
Reviewed By: epriestley
CC: aran, epriestley, Korvin, btrahan
Maniphest Tasks: T1278
Differential Revision: https://secure.phabricator.com/D3000
Summary:
- LDAP import needs to use envelopes.
- Use ldap_sprintf().
Test Plan: Configured an LDAP server. Added an account. Imported it; logged in with it. Tried to login with accounts like ",", etc., got good errors.
Reviewers: vrana, btrahan
Reviewed By: btrahan
CC: aran
Differential Revision: https://secure.phabricator.com/D2995
Summary:
See D2991 / T1526. Two major changes here:
- PHP just straight-up logs passwords on ldap_bind() failures. Suppress that with "@" and keep them out of DarkConsole by enabling discard mode.
- Use PhutilOpaqueEnvelope whenever we send a password into a call stack.
Test Plan:
- Created a new account.
- Reset password.
- Changed password.
- Logged in with valid password.
- Tried to login with bad password.
- Changed password via accountadmin.
- Hit various LDAP errors and made sure nothing appears in the logs.
Reviewers: vrana, btrahan
Reviewed By: vrana
CC: aran
Differential Revision: https://secure.phabricator.com/D2993
Summary:
Currently, it's hard to debug performance issues on POST pages. Add flags to stop redirects and always collect profiles.
Also fix an issue with "all" profiles. This feature is mostly just for profiling DarkConsole itself and is rarely used, I think it's been broken for some time. There's no way to get to it with the UI.
NOTE: Some JS workflows don't stop on redirect because they use JS/AJAX redirects.
Test Plan: Enabled options, browsed, got stopped on redirects and had profiles generated. Disabled options and verified redirects and profiles work normally.
Reviewers: vrana, btrahan
Reviewed By: vrana
CC: aran
Differential Revision: https://secure.phabricator.com/D2990
Summary:
- Share code between `createinline` and `getcomments`
- Make them both extend the base class.
- Sync up the parameters (this is more or less nonbreaking since the call is ~6 hours old).
Test Plan: Created and fetched inlines via the API.
Reviewers: alanh, vrana, btrahan
Reviewed By: vrana
CC: aran
Maniphest Tasks: T1500
Differential Revision: https://secure.phabricator.com/D2988
'phd status' should have a stable result when invoked multiple times.
Automatically removing PID files for dead daemons every time 'phd status' is
invoked prevents tools from noticing that a daemon has died if something
happens to invoke 'phd status' before the tool looks. This affects Puppet
noticably, since it probably runs the status command every half hour.
'phd status' may be invoked by tools (such as puppet) which need to make
automated decisions about whether to start/restart the daemon. To enable this,
'phd status' now exits with 0 if all daemons are running, 1 if no daemons are
running, and 2 if some (but not all) daemons are running.
Summary:
See T931. LLVM users would also prefer simpler mail, and have similarly harsh opinions about the current state of affairs.
Allow installs to disable the hint blocks if they don't want them.
Test Plan: Sent myself mail with these settings on/off, got mail with/without the blocks.
Reviewers: btrahan, vrana
Reviewed By: vrana
CC: aran
Maniphest Tasks: T931
Differential Revision: https://secure.phabricator.com/D2968
Summary: This is a minor quality-of-life improvement to prevent D2968 from being as nasty as it is.
Test Plan: Ran unit tests; generated Differential, Maniphest and Diffusion emails and verified the bodies looked sensible.
Reviewers: btrahan, vrana
Reviewed By: vrana
CC: aran
Maniphest Tasks: T931
Differential Revision: https://secure.phabricator.com/D2986
Summary: Paths autocompleter sometimes omits `/`.
Test Plan:
# Go to https://secure.phabricator.com/owners/new/.
# Write `/specs/h` to path.
# Delete the second slash resulting in `/specsh`.
# Don't find `/specshistorytest.html` in autocomplete.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D2981
Summary: a silly thing because I was bored
Test Plan: and I said "arc call-conduit", and there were comments
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Maniphest Tasks: T1500
Differential Revision: https://secure.phabricator.com/D2971
Summary:
Lisk currently behaves in two different ways if you call it like `load("cow")` (throws) versus `load(99999999)` (returns null), where neither ID exists.
This was intended to catch programming errors as distinct from missing data, but in practice the former is very rare and you have to handle the latter in most cases anyway. The case where you pass "0" is particularly confusing. See D2971 for an example.
On the balance, I think this ends up being far more confusing than helpful. Instead, just return NULL if we're sure there's no such object.
Test Plan: Reasoned about program behavior.
Reviewers: alanh, btrahan, vrana
Reviewed By: vrana
CC: aran
Differential Revision: https://secure.phabricator.com/D2977
Summary:
See T1501. When users mash "save", stop them if they didn't change anything.
Also, don't default-fill the "edit notes" field with the previous notes. This is meant to be more like a commit message for your changes.
Test Plan: Edited a document with no changes, got a dialog. Edited a document with a title change only and a description change only, things worked. Edited a document with a previous "Edit notes", got a blank default fill.
Reviewers: alanh, btrahan, vrana
Reviewed By: vrana
CC: aran
Maniphest Tasks: T1501
Differential Revision: https://secure.phabricator.com/D2978
Summary: For "accept" and "reject" action, find the time between the action and last time the revision was updated by a new diff. Show it as the responsiveness row.
Test Plan: view it for a couple of engineers.
Reviewers: epriestley, vii
Reviewed By: epriestley
CC: vrana, nh, aran, Korvin
Differential Revision: https://secure.phabricator.com/D2970
Summary: Add active-directory domain-based ldap authentication support
Test Plan: Tested on a live install against Active Directory on a Windows Server
Reviewers: epriestley
CC: aran, epriestley
Maniphest Tasks: T1496
Differential Revision: https://secure.phabricator.com/D2966
Summary: I missed this callsite in D2946. Transition it to the new markup cache.
Test Plan: Clicked "show change" on a description edit transaction, got the change instead of a fatal.
Reviewers: alanh, btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T1502
Differential Revision: https://secure.phabricator.com/D2972
Summary:
See D818 for an older attempt at this. Support code has matured to the point where the patch is pretty straightforward.
@tido, this was a long-standing request from Aditya back in the day.
Test Plan: Used `reparse.php --herald` to send myself a bunch of emails with various patch configurations. Confirmed that limits are respected, reasonable errors arise when they're violated, etc. (Timeout is a little funky but that's out of scope here, I think.)
Reviewers: btrahan
Reviewed By: btrahan
CC: tido, aran
Maniphest Tasks: T456
Differential Revision: https://secure.phabricator.com/D2967
Summary:
- Assigning $cc_phids clobbers the correct value assigned on line 80.
- Remove stack trace noise, the trace is always meaningless and well-known.
- Actually show the original body.
Test Plan: Piped mail to the mail receiver and verified the errors didn't CC revision CCs, no longer had traces, and included the original raw text body.
Reviewers: btrahan, jungejason
Reviewed By: jungejason
CC: aran
Differential Revision: https://secure.phabricator.com/D2969
Summary: See D2955, D2956, D2957.
Test Plan: Read documentation. Ran all these commands from Git Bash and cmd.exe and used the specified editors to edit blocks of text.
Reviewers: btrahan, jungejason
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T1309
Differential Revision: https://secure.phabricator.com/D2958
Summary:
Still not 100% sure what the repro case here is, but we do something similar on line 438 above. One case could be an SVN repo with a subpath specified, I think.
In any case, don't fatal if we're missing the commit.
Test Plan: Loaded some browse views, although I don't have a repro.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Differential Revision: https://secure.phabricator.com/D2959
Summary: http://www.networksorcery.com/enp/protocol/irc.htm
Test Plan: augment code with an additional debug line (phlog('hi');) so I can see my case was trigged and it will fall through. setup an ill-configured IRC server with ngircd. Configure an ircbot to connect to said ill-configured IRC server. verify ircbot connected to channel. verify in irc bot logs that debug line was invoked.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Maniphest Tasks: T1452
Differential Revision: https://secure.phabricator.com/D2962
Summary: Allow the GC daemon to collect the new markup cache.
Test Plan: Ran gc daemon in "debug" mode, saw it collect cache entries.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Differential Revision: https://secure.phabricator.com/D2947
Summary:
- See D2945.
- Drop `cache` field from ManiphestTransaction.
- Render task descriptions and transactions through PhabricatorMarkupEngine.
- Also pull the list of macros more lazily.
Test Plan:
- Verified transactions and transaction preview work correctly and interact with cache correctly.
- Verified tasks descriptions and task preview work correctly.
- Verified we don't hit the imagemacro table when we're rendering everything from cache anymore.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Differential Revision: https://secure.phabricator.com/D2946
Test Plan:
Created diff with a postponed linter and the lint status set to such
via arcanist. Verified lint status showed as postponed in diff view.
Reviewers: epriestley, vrana
Reviewed By: epriestley
CC: aran, Korvin
Maniphest Tasks: T1332
Differential Revision: https://secure.phabricator.com/D2932
Summary:
- Old versions of Mercurial give different output for `hg log -- ''` and `hg log`. Just use `hg log`.
- Branch names with spaces can't be specified in `--rev`. I talked with hstuart in #mercurial and apparently am not crazy.
Test Plan:
- Viewed history of a repository.
- Viewed history of a file.
- Viewed branch `m m m m m 2:ffffffffffff (inactive)`
- Learned that you checkout this branch with `hg checkout ':m m m m m 2:ffffffffffff (inactive)'`
Reviewers: dschleimer, btrahan
Reviewed By: dschleimer
CC: aran
Maniphest Tasks: T1268
Differential Revision: https://secure.phabricator.com/D2950
Summary:
The immediate issue this addresses is T1366, adding a rendering cache to Phriction. For wiki pages with code blocks especially, rerendering them each time is expensive.
The broader issue is that out markup caches aren't very good right now. They have three major problems:
**Problem 1: the data is stored in the wrong place.** We currently store remarkup caches on objects. This means we're always loading it and passing it around even when we don't need it, can't genericize cache management code (e.g., have one simple script to drop/GC caches), need to update authoritative rows to clear caches, and can't genericize rendering code since each object is different.
To solve this, I created a dedicated cache database that I plan to move all markup caches to use.
**Problem 2: time-variant rules break when cached.** Some rules like `**bold**` are time-invariant and always produce the same output, but some rules like `{Tnnn}` and `@username` are variant and may render differently (because a task was closed or a user is on vacation). Currently, we cache the raw output, so these time-variant rules get locked at whatever values they had when they were first rendered. This is the main reason Phriction doesn't have a cache right now -- I wanted `{Tnnn}` rules to reflect open/closed tasks.
To solve this, I split markup into a "preprocessing" phase (which does all the parsing and evaluates all time-invariant rules) and a "postprocessing" phase (which evaluates time-variant rules only). The preprocessing phase is most of the expense (and, notably, includes syntax highlighting) so this is nearly as good as caching the final output. I did most of the work here in D737 / D738, but we never moved to use it in Phabricator -- we currently just do the two operations serially in all cases.
This diff splits them apart and caches the output of preprocessing only, so we benefit from caching but also get accurate time-variant rendering.
**Problem 3: cache access isn't batched/pipelined optimally.** When we're rendering a list of markup blocks, we should be able to batch datafetching better than we do. D738 helped with this (fetching is batched within a single hunk of markup) and this improves batching on cache access. We could still do better here, but this is at least a step forward.
Also fixes a bug with generating a link in the Phriction history interface ($uri gets clobbered).
I'm using PHP serialization instead of JSON serialization because Remarkup does some stuff with non-ascii characters that might not survive JSON.
Test Plan:
- Created a Phriction document and verified that previews don't go to cache (no rows appear in the cache table).
- Verified that published documents come out of cache.
- Verified that caches generate/regenerate correctly, time-variant rules render properly and old documents hit the right caches.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T1366
Differential Revision: https://secure.phabricator.com/D2945
Summary:
Currently, a change may affect a very large number of paths. When we run the OwnersWorker on it, we'll execute a query which looks up packages for the paths. This may exceed "max_allowed_packet". Instead, break the list of paths into smaller chunks.
This is mostly to unblock r4nt / llvm, I'm going to add a more finessed approach to array_chunk() shortly.
Test Plan: Ran `reparse.php --owners` on a revision which affected packages, verified results are the same before and after the change. Set chunk size to 1, verified query results aggregated properly.
Reviewers: btrahan, jungejason, nh
Reviewed By: jungejason
CC: aran
Differential Revision: https://secure.phabricator.com/D2943
Summary:
Certain types of things we should be storing in edges (notably, Task X depends on Task Y) should always be acyclic. Allow `PhabricatorEdgeEditor` to enforce this, since we can't correctly enforce it outside of the editor without being vulnerable to races.
Each edge type can be marked acyclic. If an edge type is acyclic, we perform additional steps when writing new edges of that type:
- We acquire a global lock on the edge type before performing any reads or writes. This ensures we can't produce a cycle as a result of a race where two edits add edges which independently do not produce a cycle, but do produce a cycle when combined.
- After performing writes but before committing transactions, we load the edge graph for each acyclic type and verify that it is, in fact, acyclic. If we detect cycles, we abort the edit.
- When we're done, we release the edge type locks.
This is a relatively high-complexity change, but gives us a simple way to flag an edge type as acyclic in a robust way.
Test Plan: Ran unit tests.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T1162
Differential Revision: https://secure.phabricator.com/D2940
Summary:
Currently, multiple unit tests that acquire global locks will interfere with each other. Namespace the locks so they don't.
(Possibly we should also rename this to PhabricatorStorageNamespaceLock or something since it's not really global any more, but that's kind of unwieldy...)
Test Plan: Acquired locks with --trace and verified they were namespaced properly.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T1162
Differential Revision: https://secure.phabricator.com/D2939
Summary:
In unit tests which use fixtures, we open transactions on every connection we establish. However, since we don't track connections that are established with "$force_new" (currently, only GlobalLock connections) we never close these transactions normally.
Instead of not tracking these connections, track them using unique keys so we'll never get a cache hit on them.
Test Plan: Built unit tests on top of this, had them stop dying from unclosed transactions.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T1162
Differential Revision: https://secure.phabricator.com/D2938
Summary:
A later diff adds unit tests against edges, but we need real objects to connect with edges. Add some trivial objects to the Harbormaster database to compliment the similar HarbormasterScratchTable.
On its own, this does nothing interesting.
Test Plan: Built unit tests on this in a followup.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T1162
Differential Revision: https://secure.phabricator.com/D2937
Summary: GitHub moved these pages around and made it easier to find your application list.
Test Plan: Read docs, verified links are correct.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Differential Revision: https://secure.phabricator.com/D2941
Summary:
We have a bit more copy-paste than we need, consolidate a bit.
(Also switch Mercurial to download git diffs, which it handles well; we use them in "arc patch".)
Test Plan:
- Downloaded a raw diff from Differential.
- Downloaded a raw change from Diffusion.
- Downloaded a raw file from Diffusion.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Differential Revision: https://secure.phabricator.com/D2942
Summary: This is a fairly contentious default that we can easily move to configuration.
Test Plan: Changed the default, changed my user setting, reverted my user setting, verified the "settings" page.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Differential Revision: https://secure.phabricator.com/D2935
Summary: Currently, Diffusion supports highlighting of line ranges passed in the URI. It would be helpful to be able to highlight multiple line ranges.
Test Plan: Accessed directly via URL in my sandbox. Seems to work. I'm not sure what other components use this functionality, but this change should be backwards compatible.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, epriestley, Korvin
Differential Revision: https://secure.phabricator.com/D2921
Summary:
Brazenly copied from differential.close. Ulterior motive
is to be able to automate discarding of useless commits from
arcanist testing.
Test Plan:
Called method in phabricator sandbox. Revisions were
successfully abandoned.
Reviewers: epriestley
Reviewed By: epriestley
CC: nh, aran, Korvin
Differential Revision: https://secure.phabricator.com/D2928
Summary: Depends on D2926. Adds a simple CLI client for Aphlict to make it easier to debug stuff.
Test Plan: Ran client, saw debug messages.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Differential Revision: https://secure.phabricator.com/D2927
Summary:
See discussion here:
https://secure.phabricator.com/chatlog/channel/%23phabricator/?at=21186
Basically, MySQL usually raises a good error if we exceed "max_allowed_packet":
EXCEPTION: (AphrontQueryException) #1153: Got a packet bigger than 'max_allowed_packet' bytes
But sometimes it gives us a #2006 instead. This is documented, at least:
>"With some clients, you may also get a Lost connection to MySQL server during query error if the communication packet is too large."
http://dev.mysql.com/doc/refman//5.5/en/packet-too-large.html
Try to improve the error message to point at this as a possible explanation.
Test Plan: Faked an error, had it throw, read exception message. See also chatlog.
Reviewers: btrahan, skrul
Reviewed By: skrul
CC: aran
Differential Revision: https://secure.phabricator.com/D2923
Summary: See D2924.
Test Plan: Ran locks with blocking timeouts.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Differential Revision: https://secure.phabricator.com/D2925
Summary:
accessibility covers not only a given post but also the various "published" views.
to keep the code relative clean, this diff also splits up the post list controller logic quite a bit. this also feels like good preparation for some other work around introducing "blogs" which are collections of published posts from bloggers with some fancy features around that.
Test Plan: clicked around various parts of the Phame application as a logged in user, a logged in user with no personal posts, and without any user logged in at all. various views all seemed reasonable.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Maniphest Tasks: T1373
Differential Revision: https://secure.phabricator.com/D2898
Summary:
See T1448. If this file isn't present, just move on instead of failing, since it's a (sort of) legitimate repository state.
Also fix some silliness a little later that got introduced in refactoring, I think.
Test Plan: Added an external to my test repo and removed ".gitmodules". Verified that the directory is now viewable after this patch.
Reviewers: btrahan, davidreuss, jungejason
Reviewed By: davidreuss
CC: aran
Maniphest Tasks: T1448
Differential Revision: https://secure.phabricator.com/D2922
Summary:
binary_safe_diff is needed in arcanist too. Moved it over to
arcanist. See D2915.
Test Plan: diffusion page rendered correctly on binary file.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D2916
Summary: Did exactly what @epriestley suggested in T1428#2.
Test Plan: Turn it on in your config, post a revision, accept it. Turn it off in your config, post a revision, can't accept it.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D2900
Summary: See D2906. This just adds text so they render pretty.
Test Plan:
Got pretty emails and rendered transactions.
{F13706}
Reviewers: btrahan, davidreuss
Reviewed By: btrahan
CC: aran
Differential Revision: https://secure.phabricator.com/D2907
Summary:
- See D2741.
- When EdgeEditor performs edits, emit events.
- Listen for Maniphest edge events and save the changes as transactions.
- Do all this in a reasonably generic way that won't take too much rewriting as we use edges more generally.
Test Plan: Attached and detached commits from tasks, saw reasonable-looking transactions spring into existence.
Reviewers: btrahan, davidreuss
Reviewed By: btrahan
CC: aran
Differential Revision: https://secure.phabricator.com/D2906
Summary: Simplify FeedQuery by making it extend from PhabricatorIDPagedPolicyQuery
Test Plan: Looked at feed on home, projects, user profile, and called `feed.query`.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Differential Revision: https://secure.phabricator.com/D2905
Summary: This code is duplicated in two places; share it.
Test Plan: Looked at feed and notifications.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Differential Revision: https://secure.phabricator.com/D2901
Summary: This is both only partially complete (supports Maniphest only) and somewhat overcomplicated (includes support for applying similar algorithms to Feed), but provides runtime aggregation of notifications.
Test Plan: {F13502}
Reviewers: btrahan, jungejason
Reviewed By: btrahan
CC: aran
Differential Revision: https://secure.phabricator.com/D2884
Summary: Explain how these work. Let me know if there's a clearer way to explain "arc:bookmark".
Test Plan: Generated / viewed documentation.
Reviewers: dschleimer
Reviewed By: dschleimer
CC: aran
Maniphest Tasks: T1233
Differential Revision: https://secure.phabricator.com/D2868
Summary:
Oh man, after the explode/map, the output when no references is actually
array(1) {
[0]=> string(0) ""
}
.. making the falsey check fail to work as expected.
Test Plan: same as D2892, but with a little more scrutiny :p
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D2893
Summary:
When there are no other references (and there will be none ususally,
if D2891 is accepted), we would show "References:" with empty contents.
Avoid that by testing for empty output after applying stupid filtering.
Test Plan: Looked in a standard repository with no special refs.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D2892
Summary:
Diff of diffs display changes between new versions of file.
This is bad after rebase because there can be many unrelated changes so it is hard to spot the real change.
This diff unhighlights the lines that were added or removed in rebase.
The changes are still visible (they can be sometimes relevant) but very subtle.
Test Plan:
# Add, change and delete line. Display diff.
# Add and change some lines in parent. Rebase. Display diff. Display diff of diff.
# Change and add some lines. Display diff. Display diff to first diff. Display diff to second diff.
Reviewers: epriestley, jungejason
Reviewed By: jungejason
CC: jungejason, aran, Korvin
Differential Revision: https://secure.phabricator.com/D2761
Summary:
The locks held by read-only pullLocal daemons were causing our discovery instance
to not get the lock and fail at discovery. We don't need to hold the lock while
pulling (only while discovering), so this moves the lock to the appropriate place.
Test Plan: tested in production
Reviewers: jungejason, epriestley, vrana
Reviewed By: jungejason
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D2890
Summary: They just beg to be clicked.
Test Plan: clicked furiously with my mouse until i got tired - went expected places.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D2889
Summary:
Even though `--encoding` is passed to the command, git still fails
in some cases to correctly convert the output. Attempt the conversion
ourselves if it's non UTF-8.
Test Plan: Reparsed message in a repository with ISO-8859-1 encoded commit messages.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Maniphest Tasks: T452
Differential Revision: https://secure.phabricator.com/D2888
Summary:
- Allow clients to query for specific closed statuses (invalid, resolved, wontfix, etc), not just "closed" tasks.
- Rename this method to maniphest.query and deprecate maniphest.find as an alias to maniphest.query, for API consistency.
Test Plan: Ran queries for all tasks, "wontfix" tasks, closed tasks.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Differential Revision: https://secure.phabricator.com/D2887
Summary:
This adds:
1) A new "arc:lint-postponed" diff property which stores a list of
lint names that are postponed and a finishpostponedlinters conduit
method which removes linters from this list. Postponed linters are
shown in the lint details.
2) A updatelintmessages conduit message, which adds additional lint
messages to the "arc:lint" diff property.
In combination, this provides very basic support for running
asynchronous static analysis tools. When the diff is being created,
a list of asynchronous static analysis runs can be added to the
diff's postponed linters list. As these postponed linters finish
up, then can report new lint messages back to the diff then mark
themselves as complete.
The client is currently responsible for filtering the lint messages
by things like affected lines and files.
Test Plan:
Used conduit call API to add lint messages and remove postponed
linters from a test diff.
Reviewers: epriestley, vrana, nh, jungejason
Reviewed By: epriestley
CC: aran, Korvin
Maniphest Tasks: T1332
Differential Revision: https://secure.phabricator.com/D2792
Summary: people want to get the info about how long the revision has been active since the last time the authoer ran 'arc diff'.
Test Plan: call it from conduit console
Reviewers: vrana, epriestley
Reviewed By: epriestley
CC: Girish, aran, epriestley
Differential Revision: https://secure.phabricator.com/D2885
Summary: Line numbers and file paths may be different in current version.
Test Plan: Disabled editor, issued exception, clicked on the link.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D2886
Summary:
We have a race condition right now, where we may insert a commit without `seenOnBranches`. This means shouldAutocloseCommit will return false (since it's not on any autoclose branches) if the message parser runs fast enough, causing it to associate the commit but not close the revision. This happened for D2851.
Also prompt the user to repair broken repositories.
Test Plan: Ran discovery / repair. Ran discovery on new commits. Verified 'seenOnBranches' value. Deleted some data, verified "repair" error. Repaired repository.
Reviewers: jungejason, nh, vrana, btrahan
Reviewed By: btrahan
CC: aran
Differential Revision: https://secure.phabricator.com/D2858
Summary:
- Provide a filter to show just unread notifications.
- Visually show which notifications are unread.
- Style tweaks to make notifications more readable.
Test Plan: {F13494}
Reviewers: btrahan, jungejason
Reviewed By: btrahan
CC: aran
Differential Revision: https://secure.phabricator.com/D2883
Summary:
They were shifted by one because of calling `$day->setTime(24, 0, 0)`.
I've tried to clone the date and get the epoch from the clone but it was crashing for some reason.
Test Plan: /calendar/
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D2882
Summary: this lets people download diffs easily in case arc export, arc patch, etc isn't to their liking or whatever.
Test Plan: "Download Raw Diff" a few times with various things toggled in the "Revision Update History" widget
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Maniphest Tasks: T1294
Differential Revision: https://secure.phabricator.com/D2855
Summary: Allow multiple daemons to run without contention.
Test Plan: Ran multiple daemons simultaneously in "debug" mode, observed them acquiring (and sometimes failing to acquire) locks.
Reviewers: btrahan, jungejason, nh
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T1400
Differential Revision: https://secure.phabricator.com/D2877
Summary:
These are currently not available via Conduit.
Also fix a bug where bad JSON input triggers an error about undefined `$metadata`.
Test Plan: Ran 'repository.create' with and without a description and with and without autoclose. Verified the created repositories had the requested attributes.
Reviewers: btrahan, vrana
Reviewed By: btrahan
CC: aran
Differential Revision: https://secure.phabricator.com/D2881
Summary: This error usually doesn't occur because we have only one hunk per changeset most of the time.
Test Plan:
$hunk = new ArcanistDiffHunk();
$hunk->setAddLines(1);
$hunk->setDelLines(1);
$change = new ArcanistDiffChange();
$change->addHunk($hunk);
$change->addHunk($hunk);
$diff = DifferentialDiff::newFromRawChanges(array($change));
var_dump($diff->getLineCount());
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D2878
Summary: Implementation is a little crazy but this seems to work as advertised.
Test Plan: Acquired locks with "lock.php". Verified they held as long as the process reamined open and released properly on kill -9, ^C, etc.
Reviewers: nh, jungejason, vrana, btrahan, Girish, edward
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T1400
Differential Revision: https://secure.phabricator.com/D2864
Summary: I will also need `getRemovedLines()` so refactor this first.
Test Plan:
New test case.
Viewed uncached diff.
Verified that the only callsite of `getAddedLines()` trims lines.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D2875
Summary: When the commandeerer was a reviewer, after the commandeering, he stayed as a reviewer. He can no longer amend the diff. He has to go 'Edit Revision' to remove himself/herself. The fix is to remove it automatically.
Test Plan:
comamndeereded a revision and the behavior is correct now:
- I was removed from the revision list
- the comment transaction shows one more entry that I removed myself as a reviewer
Reviewers: epriestley, vrana
Reviewed By: vrana
CC: nh, vrana, aran, Korvin
Maniphest Tasks: T1225
Differential Revision: https://secure.phabricator.com/D2872
Summary: Related to D2873.
Test Plan: Specified it and verified that highlighting still works.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D2874
Summary:
Simpler fix for D2572. Not entirely sure why Firebug is crashing Firefox. It appears to be callstack depth related, possibly? You can sort of reproduce this like this:
>>> var f = function(n) { n && f(n - 1); }
>>> f(10000); // Takes a few ms to run.
>>> f(40000); // Takes a few ms to run.
>>> f(50000); // Hangs Firefox.
If there are 2,000 files, we currently hit a stack depth of around 4,000 with the pass() rules, so it seems like we should be 10x short of exploding.
Anyway, this keeps us from increasing stack depth for menus that aren't currently open and stops Firebug from crashing.
Test Plan: Clicked 2000-diff revision in Firefox.
Reviewers: vrana
Reviewed By: vrana
CC: aran
Differential Revision: https://secure.phabricator.com/D2870
Summary:
When the absolute path is used for load file (loadFileContent(()), it fails in git. For example:
/var/repo/page_admin_app
> git cat-file blob '4d6c03923006d6c444660f2c734fe03e10fd20bd':'/ios/PageAdminApp/Resources/splash/De fault-Portrait@2x~ipad.png'
fatal: Not a valid object name 4d6c03923006d6c444660f2c734fe03e10fd20bd:/ios/PageAdminApp/Resources/s plash/Default-Portrait@2x~ipad.png
This is breaking the auto-closing for about 8 revisions like
https://phabricator.fb.com/rPPA4d6c03923006d6c444660f2c734fe03e10fd20bd ...
https://phabricatorcator.fb.com/rPPA51acb7e482aab0c491b530ed19dddc741d50f673 ...
Test Plan:
- reparsed https://phabricator.fb.com/rPPA4d6c03923006d6c444660f2c734fe03e10fd20bd successfully with corresponding differential revision being closed.
- verified that without leading '/', loadFileContent for svn still works. Both of the following commands worked (note the double '/' right before 'tfb':
svn cat svn+ssh://svn.vip.facebook.com/svnroot//tfb/trunk/www/flib/intern/cachearchiver/regenerators/wurfl/CacheArchiveWurflRegenerator.php@579700
svn cat svn+ssh://svn.vip.facebook.com/svnroot/tfb/trunk/www/flib/intern/cachearchiver/regenerators/wurfl/CacheArchiveWurflRegenerator.php@579700
Reviewers: vrana
Reviewed By: vrana
CC: nh, aran, epriestley
Differential Revision: https://secure.phabricator.com/D2847
Summary:
Sometimes people create their account in Phabricator using some OAuth provider, forget about it and then tries to login with another provider.
Provide this information to them.
Test Plan: Tried to link an account linked to already existing user.
Reviewers: epriestley
Reviewed By: epriestley
CC: robarnold, aran, Korvin
Differential Revision: https://secure.phabricator.com/D2857
Summary: See T693. To do "arc branch" performantly in immutable history repositories, we need to be able to do a single query to identify revisions related by hash. Return hash information to enable this.
Test Plan: Ran `differential.query`.
Reviewers: btrahan, vrana
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T693
Differential Revision: https://secure.phabricator.com/D2859
Summary: show project profile image on pertinent edit page. also add a "Use Default Image" checkbox for both project and user profiles. Also added a function for projects to get the profile picture to prevent some copy + paste action.
Test Plan: set my user profile and project profile image. clicked "Use Default Image" and got the default image back.
Reviewers: epriestley, floatinglomas
Reviewed By: floatinglomas
CC: aran, Korvin
Maniphest Tasks: T1307
Differential Revision: https://secure.phabricator.com/D2852
Summary:
Improve performance of large discovery tasks in Git by using subprocess streaming, like we do for Mercurial.
Basically, we save the cost of running many `git log` commands by running one big `git log` command but only parsing as much of it as we need to. This is pretty complicated, but we more or less need it for mercurial (which has ~100ms of 'hg' overhead instead of ~5ms of 'git' overhead) so we're already committed to most of the complexity costs. The git implementation is much simpler than the hg implementation because we don't need to handle all the weird parent rules (git gives us to them easily).
Test Plan:
Before, `discover --repair` on Phabricator took 35s:
real 0m35.324s
user 0m13.364s
sys 0m21.088s
Now 7s:
real 0m7.236s
user 0m2.436s
sys 0m3.444s
Note that most of the time is spent inserting rows after discover, the actual speedup of the git discovery part is much larger (subjectively, it runs in less than a second now, from ~28 seconds before).
Also ran discover/pull on single new commits in normal cases to verify that nothing broke in the common case.
Reviewers: jungejason, nh, vrana
Reviewed By: vrana
CC: aran
Maniphest Tasks: T1401
Differential Revision: https://secure.phabricator.com/D2851
Summary:
If a repository is missing commits because they mysteriously vanished, there's no reasonable way to get them back right now. Provide a way to ignore the state in the database and rediscover the entire repository unconditionally.
We don't queue any reparses or anything, but when I move reparse into this script we can hook things up or something. This generally shouldn't be too important anyway.
Test Plan: Ran `repository discover --repair` on Phabricator.
Reviewers: jungejason, nh, vrana
Reviewed By: vrana
CC: aran
Differential Revision: https://secure.phabricator.com/D2850
Summary:
Nothing new or exciting here yet, just moving the random scripts/repositories/ things to bin/repository. Also add `repository list`.
(Console stuff comes from D2841.)
Test Plan: Ran `repository list`, `repository pull`, `repository discover`, `repository discover --verbose`, `repository help`.
Reviewers: jungejason, vrana
Reviewed By: vrana
CC: aran
Differential Revision: https://secure.phabricator.com/D2849
Summary: Add verbose logging. This logging is activated by setting "phd.verbose" in the config, running "phd debug", or explicitly in scripts/repository/pull.php and scripst/repository/discover.php
Test Plan:
>>> orbital ~/devtools/phabricator $ ./scripts/repository/discover.php GTEST
Discovering 'GTEST'...
<VERB> PhabricatorRepositoryPullLocalDaemon Discovering commits in repository 'GTEST'...
<VERB> PhabricatorRepositoryPullLocalDaemon Examining branch '()_+abcd$100', at a37bc285a12efa7224fe19f3df54cd90fa2b897a.
<VERB> PhabricatorRepositoryPullLocalDaemon Skipping, HEAD is known.
<VERB> PhabricatorRepositoryPullLocalDaemon Examining branch '+abcd$100', at a37bc285a12efa7224fe19f3df54cd90fa2b897a.
<VERB> PhabricatorRepositoryPullLocalDaemon Skipping, HEAD is known.
<VERB> PhabricatorRepositoryPullLocalDaemon Examining branch '_+abcd$100', at a37bc285a12efa7224fe19f3df54cd90fa2b897a.
<VERB> PhabricatorRepositoryPullLocalDaemon Skipping, HEAD is known.
<VERB> PhabricatorRepositoryPullLocalDaemon Examining branch 'abcd$100', at a37bc285a12efa7224fe19f3df54cd90fa2b897a.
<VERB> PhabricatorRepositoryPullLocalDaemon Skipping, HEAD is known.
<VERB> PhabricatorRepositoryPullLocalDaemon Examining branch 'arcpatch', at 774c7737b2d560a291697126bf4513204ccf661a.
<VERB> PhabricatorRepositoryPullLocalDaemon Skipping, HEAD is known.
<VERB> PhabricatorRepositoryPullLocalDaemon Examining branch 'arcpatch-1', at dc97539bee07293f95990d71f4638335a2531d69.
<VERB> PhabricatorRepositoryPullLocalDaemon Skipping, HEAD is known.
<VERB> PhabricatorRepositoryPullLocalDaemon Examining branch 'arcpatch-2', at 1acfaec313c46dd3caa90448800181fb91b0270f.
Reviewers: jungejason
Reviewed By: jungejason
CC: aran
Differential Revision: https://secure.phabricator.com/D2843
Summary:
See D2855 for usage.
This has a drawback that inlines without a comment (synthetic comments) are not attached anywhere.
I don't like adding more and more methods so I've chosen this solution.
Plus comments and inline comments are often useful together.
Test Plan: Called the method on a revision with inline comments.
Reviewers: epriestley
Reviewed By: epriestley
CC: vii, aran, Korvin
Differential Revision: https://secure.phabricator.com/D2844
Test Plan:
Displayed repository.
Displayed repository history.
Wondered that we actually have bunch of commits without a revision.
Displayed blame.
Didn't display merge commit.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D2840
Summary: in svn and hg (for now), no branch used.
Test Plan: will test live
Reviewers: epriestley
CC: nh, vrana, aran, Korvin
Differential Revision: https://secure.phabricator.com/D2839
Summary:
This allows translations to specify custom date and time format.
We will need to call `setlocale()` or build system independent on it at some point but I guess it is good for now.
Test Plan: Translated 'D, M j, g:i A', saw it in diff.
Reviewers: btrahan, epriestley
Reviewed By: btrahan
CC: aran, Korvin
Maniphest Tasks: T1139, T1321
Differential Revision: https://secure.phabricator.com/D2809
Summary: I need this much more than commit but it can be useful too.
Test Plan: Displayed blame, clicked on the link.
Reviewers: epriestley
Reviewed By: epriestley
CC: john, aran, Korvin, phunt
Differential Revision: https://secure.phabricator.com/D2820
Summary: People want to create filters checking if they are in CC which is almost impossible with multiplexing in Outlook.
Test Plan: Sent e-mail with multiple CCs, verified headers.
Reviewers: epriestley, nh
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D2829
Summary:
Currently, notifications aren't z-indexed so they can end up underneath some other elements:
{F13144}
Fix this so they float correctly.
Test Plan: {F13166}
Reviewers: jungejason, btrahan
Reviewed By: jungejason
CC: aran
Maniphest Tasks: T1398
Differential Revision: https://secure.phabricator.com/D2834
Summary: it's calling pull daemon's failure. This is actually Nick's fix.
Test Plan: Nick already manually ran it on daemon machine.
Reviewers: vrana, nh
Reviewed By: vrana
CC: aran, epriestley
Differential Revision: https://secure.phabricator.com/D2828
Summary: documentation changes for D2822
Test Plan:
Generated docs, but failed to read them because no one at Facebook
remembers how to get docs working in our sandbox.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Maniphest Tasks: T1233
Differential Revision: https://secure.phabricator.com/D2827
Summary: Add a `notification.debug` setting that shows debug info in the browser. Also improve some logging/error handling stuff and fix a bug with host names.
Test Plan: {F13098}
Reviewers: jungejason, btrahan, vrana
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T944
Differential Revision: https://secure.phabricator.com/D2810
Summary:
Currently, when rendering the panel we count the number of unread notifications in the last 15, but this means we can never render a number larger than 15. If the user has more unread notifications than that, or unread notifications older than the most recent 15, there will be a flash of the higher number and then it will update to the lower number afterward.
Instead, count all unread notifications. This uses the same method used to render both numbers.
Test Plan: Loaded a page, checked the menu, nothing exploded.
Reviewers: btrahan, jungejason, vrana
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T974
Differential Revision: https://secure.phabricator.com/D2811
Summary:
"differential.creatediff" requires a mostly-parsed diff, but there's no reason we can't make DifferentialDiffs out of raw diffs.
This mainly serves to lower the adoption barrier if getting "arc" distributed is too much of a hassle.
Test Plan: Made a diff out of a raw block of diff text.
Reviewers: ffx, vrana, btrahan
Reviewed By: btrahan
CC: aran
Differential Revision: https://secure.phabricator.com/D2751
Summary: D2230 did the heavy lifting spoken of in the comment this diff deletes. Also remove 'macro' => false.
Test Plan: Added an image macro to a wiki document and it showed up
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Maniphest Tasks: T1023
Differential Revision: https://secure.phabricator.com/D2668
Summary: This prevents "8 active days" for last week.
Test Plan: `strtotime('1 week ago 24:00')`
Reviewers: john, epriestley
Reviewed By: epriestley
CC: aran, epriestley
Differential Revision: https://secure.phabricator.com/D2794
Summary: Hopefully this is helpful? Also fixed a thing that wasn't using config.
Test Plan: Read documentation. Sent myself a notification over the server.
Reviewers: jungejason
Reviewed By: jungejason
CC: aran
Maniphest Tasks: T944
Differential Revision: https://secure.phabricator.com/D2804
Summary: The current behavior is that, when the dropdown menu is clicked for a binary file, it navigates to the current revision page directly without showing the dropdown meu. The fix is to do nothing when there is no file displayed.
Test Plan:
- checked a diff with binary file
- checked a diff with normal files
Reviewers: vrana, epriestley
Reviewed By: epriestley
CC: epriestley, aran
Differential Revision: https://secure.phabricator.com/D2801
Summary:
The autoclose logic is currently doing a little too much work. We want to parse each commit at most twice:
# When it first appears in the repository.
# When it first appears on an autoclose branch.
These two events might not be distinct (i.e., it might first appear on an autoclose branch).
Currently, to discover commits initially appearing on autoclose branches, we check each branch, determine if it's an autoclose branch or not, and determine if the HEAD is already a known commit on an autoclose branch. This is correct so far, and allows us to ignore branches which either haven't changed or have commits at HEAD which we've already examined.
However, if an autoclose branch has a new commit, we start working backward through it. Prior to this patch, we only stop when we hit commits that we've already discovered lie on this branch. If the branch is new, none of the commits will be discovered on it (they're discovered in general, and likely discovered on other autoclose branches, but not discovered on this branch), so we'll parse all the way back to the root.
Instead, we want to stop when we hit commits that we've already discovered on //any// autoclose branch.
So do that.
Test Plan: Pushed a new branch, then pushed a new commit at HEAD. Ran discovery, verified we rediscovered only 1 commit, not every commit in history.
Reviewers: vrana, jungejason, aurelijus
Reviewed By: vrana
CC: aran
Maniphest Tasks: T1389
Differential Revision: https://secure.phabricator.com/D2798
Summary:
Support the `--verbose` flag added in D2795 in `phd`. See T1389.
Also simplify argument generation a little bit.
Test Plan: Ran "nice" daemon with debug, daemon + verbose, daemon + no verbose.
Reviewers: vrana, jungejason, edward, aurelijus
Reviewed By: aurelijus
CC: aran
Maniphest Tasks: T1389
Differential Revision: https://secure.phabricator.com/D2797
Summary:
Array-like fields in HerladCommitAdapter should always return things of
type array. If they return array(...) or null, then array_*() functions
sporadically break.
Test Plan:
Ran a PhabricatorRepositoryCommitHeraldWorker for a troublesome commit.
Failed before; fails no more.
Reviewers: epriestley, jungejason
Reviewed By: epriestley
CC: aran, Korvin, vdt
Maniphest Tasks: T1385
Differential Revision: https://secure.phabricator.com/D2793
Summary: I tried also filling the column by empty space but this looks better.
Test Plan: Displayed a comment both with visible and hidden inlines.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Maniphest Tasks: T1216
Differential Revision: https://secure.phabricator.com/D2789
Summary: It requires `allow_url_fopen` which we don't check in setup and our installation is about to disable it.
Test Plan:
Login with OAuth.
/oauth/facebook/diagnose/
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D2787
Summary:
Show all notifications, but make the non-reload ones transient.
Depends on D2781, D2780
Test Plan: {F12986}
Reviewers: jungejason, vrana
Reviewed By: jungejason
CC: aran
Maniphest Tasks: T944
Differential Revision: https://secure.phabricator.com/D2782
Summary: Use the features from D2758.
Test Plan: Updated T1 with two browser windows pointing at it, verified reload appeared, only one reload, and it appeared with 'alert' style.
Reviewers: jungejason, vrana
Reviewed By: jungejason
CC: aran
Maniphest Tasks: T944
Differential Revision: https://secure.phabricator.com/D2781
Summary: Add a "View All Notifications" link and page.
Test Plan: Viewed all notifications
Reviewers: jungejason, vrana
Reviewed By: jungejason
CC: aran
Maniphest Tasks: T974
Differential Revision: https://secure.phabricator.com/D2780
Summary: `array_fill()`, contrary to `range()`, doesn't accept the last element but the number of elements.
Test Plan: Reparsed commit not changed after the last diff but rebased which was previously reported as changed.
Reviewers: jungejason, epriestley
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D2783
Summary:
Works this way:
- Select users' language with multiplexing.
- Select default language otherwise (it can be different from current user's language).
- Build body and subject for each user individually.
- Set the original language after sending the mails.
Test Plan:
- Comment on a diff of user with custom translation.
- Set default to a custom translation. Comment on a diff of user with default translation.
- Set default to a default translation. Comment on a diff of user with default translation.
Repeat with/without multiplexing.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Maniphest Tasks: T1139
Differential Revision: https://secure.phabricator.com/D2774
Summary:
Diffusion page is sharing the keyboard shortcuts code with
Differential page. But since the toc (Changes) panel doesn't have id
'differential-review-toc', the 'jumping to toc' doesn't work. The fix is
to add the ID. I don't like adding 'Differential' to the Diffusion page.
Later we should refactor the code to extract the shared components out of Differential.
Test Plan:
verified that 't' worked on the diffusion commit page.
Reviewers: epriestley, nh
Reviewed By: epriestley
CC: hwang, aran, Korvin
Differential Revision: https://secure.phabricator.com/D2500
Summary: Avoid requesting a non-existent repository...
Test Plan: Delete a repo that has an associated owners package. Then verify that the owners list page no longer throws an exception.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Maniphest Tasks: T1372
Differential Revision: https://secure.phabricator.com/D2776
Summary:
- Allow more than one notification to be shown.
- Allow notifications to be customized with extra classes.
Test Plan: {F12776}
Reviewers: allenjohnashton, ddfisher, keebuhm, jungejason
Reviewed By: jungejason
CC: aran
Maniphest Tasks: T944
Differential Revision: https://secure.phabricator.com/D2758
Summary:
- Add a /notification/status/ page which shows server status.
- Remove various test controllers and routes.
- Make the "no notifications" message look better.
- Move port/URI configuration to config file.
Test Plan: Started server, hit /notification/status/, saw server status.
Reviewers: allenjohnashton, ddfisher, keebuhm, jungejason
Reviewed By: jungejason
CC: aran
Maniphest Tasks: T944
Differential Revision: https://secure.phabricator.com/D2756
Summary:
Sorry this took so long, had a bunch of stuff going on today.
Separate the actual core part of making conduit calls from the controller, so the application can make conduit calls without needing to invoke HTTP or redo auth. Generally, this lets us build more parts of the application on top of Conduit, as appropriate.
This diff can be simplified, but I wanted to unblock you guys first. I'll followup with a cleanup patch once I have a chance.
Test Plan: Ran unit tests, ran calls from the conduit API console, and ran calls over arc.
Reviewers: nodren, 20after4, btrahan, vrana
Reviewed By: 20after4
CC: aran, svemir
Maniphest Tasks: T945
Differential Revision: https://secure.phabricator.com/D2718
Summary: I forgot to handle the case where there was no task ids entered in D2771, but this corrects that issue.
Test Plan: Search with no task ids...
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Maniphest Tasks: T1365
Differential Revision: https://secure.phabricator.com/D2773
Test Plan: Created action link using VS diff, selected VS diff.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D2770
Test Plan: search in task ids for `t123, 456, pickles` (assuming 123 and 456 exist), and you will see 123 and 456 listed. This is done in the buildQueryFromRequest function, so it would process a hand-written GET string just fine too.
Reviewers: epriestley
CC: aran, Korvin
Maniphest Tasks: T1365
Differential Revision: https://secure.phabricator.com/D2771
Summary:
Allow administrators to delete accounts if they jump through enough hoops.
Also remove bogus caption about usernames being uneditable since we let admins edit those too now.
Test Plan: Tried to delete myself. Deleted a non-myself user.
Reviewers: csilvers, vrana
Reviewed By: csilvers
CC: aran
Maniphest Tasks: T1184
Differential Revision: https://secure.phabricator.com/D2767
Summary: See discussion in rP2f138d0501887fd0aca0f8536176f092880f662c.
Test Plan: Ran `user.query` on verified and unverified users.
Reviewers: csilvers, vrana
Reviewed By: csilvers
CC: aran
Differential Revision: https://secure.phabricator.com/D2768
Summary: This has been a point of some confusion, make the messages more explicit.
Test Plan:
Added var_dump() stuff and ran on some commits:
$ ./scripts/repository/reparse.php --message rP9fc54f4dfb61f7338cb1cfe819bc72d2a3404264
Running 'PhabricatorRepositoryGitCommitMessageParserWorker'...
string(58) "Closed by commit rP9fc54f4dfb61 (authored by @epriestley)."
$ ./scripts/repository/reparse.php --message rP444c634b6c6612fc7b36ddffab8023ef67372ab9
Running 'PhabricatorRepositoryGitCommitMessageParserWorker'...
string(83) "Closed by commit rP444c634b6c66 (authored by Ben Rogers, committed by @epriestley)."
$ ./scripts/repository/reparse.php --message rP22d12fe499e3ecb62392397f2ac2a91768c974aa
Running 'PhabricatorRepositoryGitCommitMessageParserWorker'...
string(52) "Closed by commit rP22d12fe499e3 (authored by vrana)."
$ ./scripts/repository/reparse.php --message rPe51958159483cd0acf00adcff51edf8717b4a23b
Running 'PhabricatorRepositoryGitCommitMessageParserWorker'...
string(85) "Closed by commit rPe51958159483 (authored by David Fisher, committed by @epriestley)."
Reviewers: csilvers, vrana
Reviewed By: csilvers
CC: aran
Differential Revision: https://secure.phabricator.com/D2765
Summary: We need it.
Test Plan: Ran 'differential.getdiff', saw it.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D2766
Test Plan:
Verified that the method retuns the same output.
Verified the number of SQL queries.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D2764
Summary: Allow fulltext search on custom query screen using the same fulltext search as the search page.
Test Plan: Enter search terms - with and without additional filters - see the expected results. Don't enter search terms - with or without additional filters - and see the expected results.
Reviewers: epriestley
CC: aran, Korvin
Maniphest Tasks: T1305
Differential Revision: https://secure.phabricator.com/D2763
Summary: Implemented it how it was suggested in ticket comments
Test Plan: create a revision in a branch, push that branch up, verify it's visible in diffusion and also that revision is not closed, then merge and push to master, verify that revision closed
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin, 20after4
Maniphest Tasks: T1210
Differential Revision: https://secure.phabricator.com/D2706
Summary:
This is to allow conservative people to bring back the old manners.
NOTE: It doesn't mark all occurrences but I think that's good enough. We will eventually mark them all.
Test Plan: Translated 'closed', displayed closed revision.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin, asukhachev
Maniphest Tasks: T1139
Differential Revision: https://secure.phabricator.com/D2760
Test Plan:
Altered database.
Wrote a custom translation and selected it in preferences.
Verified that the text is custom translated.
Set language back to default.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Maniphest Tasks: T1139
Differential Revision: https://secure.phabricator.com/D2757
Summary:
This is the first step in Phabricator internationalization.
It adds a translation selector and calls it at startup.
Installations can add custom selectors to override some texts.
We can add official translations in future.
Next step is to allow user to choose his translation which will override the global one.
This is currently used only for English plurals.
Test Plan: Displayed a diff with unit test error, verified that it says 'Detail' or 'Details' and not 'Detail(s)'.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Maniphest Tasks: T1139
Differential Revision: https://secure.phabricator.com/D2753
Summary: We currently match "any PHP file path", which is wrong, since it will match things like `/diffusion/path/to/some/source/code.php`. Match only "index.php".
Test Plan: This is the config secure.phabricator.com / local / etc run, we just had out of date documentation.
Reviewers: Korvin, vrana, btrahan, jungejason
Reviewed By: jungejason
CC: aran
Maniphest Tasks: T1323
Differential Revision: https://secure.phabricator.com/D2754
Summary:
Some e-mail clients display this header and it needs to be constant.
This is somehow involved but I doubt that there is a simpler solution.
Test Plan:
Applied SQL patch.
Commented on revision, commented on commit, changed package.
Verified that the `Thread-Topic` has constant and human readable value.
Reviewers: epriestley
Reviewed By: epriestley
CC: ola, aran, Korvin
Differential Revision: https://secure.phabricator.com/D2745
Summary:
I am a fancy designer!
{F12665} {F12666}
Test Plan: Opened/closed menu. Viewed with-notification-count and without-notification count states.
Reviewers: allenjohnashton, ddfisher, keebuhm
Reviewed By: ddfisher
CC: aran, chad, joe
Maniphest Tasks: T974
Differential Revision: https://secure.phabricator.com/D2735
Summary:
- Move to port 22280 by default.
- Warn when running as non-root.
- Allow subscription and publish/admin ports to be configured.
- Allow server to drop root after binding to 843.
- Allow log path to be configured.
- Add /status/ admin URI which shows server status.
- Return HTTP 400 Bad Request for other requests, instead of hanging.
- Minor formatting cleanup.
Test Plan:
Ran without root:
$ node aphlict_server.js
...got a good error message. Ran with --user:
$ sudo node aphlict_server.js --user=epriestley
...verified server dropped permissions. Ran with --port / --admin. Hit /status/ with GET, got status. Hit other URLs with GET, got 400.
Reviewers: allenjohnashton, ddfisher, keebuhm
Reviewed By: ddfisher
CC: aran
Differential Revision: https://secure.phabricator.com/D2737
Summary:
Based off D2704. Adds humane.js and a bit of plumbing. Currently does
not seem to load notification.css (which causes notifications not to display)
for reasons entirely opaque to me.
Test Plan:
tried locally. currently works except for the actual display due to
css loading difficulties
Reviewers: epriestley
Reviewed By: epriestley
CC: allenjohnashton, keebuhm, aran, Korvin
Differential Revision: https://secure.phabricator.com/D2705
Summary:
Added a dropdown menu button and the keyboard shortcut 'h' to the
web diff view. These hide or show the annotated code display.
Test Plan:
Viewed an example diff that changed a large number of source files
and played around with keyboard shortcuts. Everything seemed to
work as expected.
Reviewers: nh, epriestley
Reviewed By: epriestley
CC: aran, epriestley, Korvin
Differential Revision: https://secure.phabricator.com/D2714
Summary: This is //extremely// basic but dead simple and should cover us for v1, I think. Let me know what features you need.
Test Plan: Used UI example page.
Reviewers: allenjohnashton, ddfisher, keebuhm
Reviewed By: ddfisher
CC: aran, ender
Maniphest Tasks: T944
Differential Revision: https://secure.phabricator.com/D2732
Summary:
In D1460, we added initial-connection retries and additional logging in response to network/router instability at Facebook.
One of these logs occurs normally, though, when daemons get disconnected by 'wait_timeout' settings. This is noisy for a lot of installs and a few people have asked me about it.
I assume the situation has cleared up a bit at Facebook, so we can remove the "business as usual" log and keep error logs quieter for general installs?
Test Plan: Weathered the support storm.
Reviewers: nh, vrana
Reviewed By: nh
CC: Makinde, aran
Differential Revision: https://secure.phabricator.com/D2736
Summary: Made it possible to link and unlink LDAP accounts with Phabricator accounts.
Test Plan:
I've tested this code locally and in production where I work.
I've tried creating an account from scratch by logging in with LDAP and linking and unlinking an LDAP account with an existing account. I've tried to associate the same LDAP account with different Phabricator accounts and it failed as expected.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin, auduny, svemir
Maniphest Tasks: T742
Differential Revision: https://secure.phabricator.com/D2722
Summary: We allow ".", "_" and "-" in usernames now, but not in the route.
Test Plan: Went to /p/.-_/, got 404'd at the route level before and now make it to the profile controller.
Reviewers: vrana, btrahan
Reviewed By: vrana
CC: aran
Maniphest Tasks: T1348
Differential Revision: https://secure.phabricator.com/D2729
Summary: GitHub dropped support for the v2 API today, which breaks login and registration. Use the v3 API instead.
Test Plan: Registered and logged in with GitHub. Verified process pulled email/photo/name/username correctly.
Reviewers: vrana, btrahan
Reviewed By: vrana
CC: aran
Differential Revision: https://secure.phabricator.com/D2726
Summary: NOTE: This can break current ongoing conversations.
Test Plan: Commented on a revision and checked the header in the e-mail.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Maniphest Tasks: T1340
Differential Revision: https://secure.phabricator.com/D2723
Summary:
Text in the header changes appearance shortly after page loads. This
is due - for some reason - to adding the flash object. Making the width and
height of the flash object 0 solves this.
Test Plan: viewed locally, problem gone
Reviewers: epriestley
Reviewed By: epriestley
CC: allenjohnashton, keebuhm, aran, Korvin
Differential Revision: https://secure.phabricator.com/D2719
Summary:
It seems that Outlook and Mail.app mostly ignores the threading headers and thread primarily by subject.
They are also very picky about the Re: part in the header.
I guess that's because users of these clients often hit Reply when they want to create a new message to the sender of an e-mail.
We need both of these applications to work with the same setting because we don't use multiplexing to prevent sending multiple e-mails to people in lists.
I also believe that the default behavior should just work in most setups.
I've tried several different combinations of putting "Re:" and none of them seems to always work in both clients.
This diff at least adds more abstraction to the code which should prevent copy/paste errors (two fixed by this diff!).
Test Plan: Sent several e-mails with varying subject, verified that they look as before in Outlook and Mail.app.
Reviewers: epriestley, nh
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D2709
Summary:
Adds the node.js Aphlict server, the flash Aphlict client, and some
supporting javascript. Built on top of - and requires - D2703 (which is still
in progress). Will likely work with no modification on top of the final
version, though.
The node server is currently run with
sudo node support/aphlict/server/aphlict_server.js
Test Plan: tested locally
Reviewers: epriestley
Reviewed By: epriestley
CC: allenjohnashton, keebuhm, aran, Korvin
Differential Revision: https://secure.phabricator.com/D2704
Summary:
It's possible to have a tag with no tagger (or git used to allow this),
so some tags (like 26791a8bcf0e6d33f43aef7682bdb555236d56de in the linux
kernel) were causing trouble.
Test Plan:
opened diffusion to a page where I was previously getting a red box complaining
about being unable to parse the output of git for-each-ref.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin, vrana
Differential Revision: https://secure.phabricator.com/D2711
Summary: D2703#13 is confusing - it looks like that @allenjohnashton took the action but it was @epriestley.
Test Plan: I don't have a repro so I tested this block standalone.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D2707
Test Plan: Displayed e-mail preferences with and without multiplexing.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D2699
Summary:
Add a dropdown to display notificaitons. Right now
there is nothing real time about it, but we do update the panel
when the user clicks. This panel is only displayed if the
install has notifications enabled and you have them enabled in
your preferences (not using them by default).
Test Plan: Turn off notifications for user1, left them on for user2. Did things from user1 and from user2 on task both were cc'd on. user2 recieved all notifications, user1 recieved nothing. Made new user, made sure everything was switched off by default.
Reviewers: epriestley, btrahan
Reviewed By: epriestley
CC: keebuhm, ddfisher, aran, Korvin
Differential Revision: https://secure.phabricator.com/D2703
Summary:
Previously, the comment and/or summary would be added to the title. This
is incorrect behavior.
Test Plan: observed change
Reviewers: epriestley
Reviewed By: epriestley
CC: allenjohnashton, keebuhm, aran, Korvin
Differential Revision: https://secure.phabricator.com/D2701
Summary:
Most notifications are not implemented, a fallback message should
be displayed for these cases.
Test Plan: Commented out renderNotificationView in PhabricatorFeedStoryManiphest and watched the notifications render.
Reviewers: epriestley, btrahan
Reviewed By: epriestley
CC: ddfisher, keebuhm, aran, Korvin
Differential Revision: https://secure.phabricator.com/D2700
Summary:
Added `renderNotificationView()` abstract function to `PhabricatorFeedStory` base class.
Fixed duplicate line in `PhabricatorFeedStoryManiphest` class.
Fixed spacing/formatting in `ManiphestTransactionEditor`.
Test Plan: No functional changes
Reviewers: epriestley, btrahan
Reviewed By: epriestley
CC: allenjohnashton, ddfisher, aran, Korvin
Differential Revision: https://secure.phabricator.com/D2698
Summary: The notification implementation has been extended to Differential. Appropriate changes have been made to the Differential editors and Differential feed story.
Test Plan: Tested out various actions available for Differential and confirmed that the notifications get delivered correctly and feed is generated.
Reviewers: epriestley, btrahan
Reviewed By: epriestley
CC: allenjohnashton, ddfisher, aran, Korvin
Differential Revision: https://secure.phabricator.com/D2696
Summary:
HPHP doesn't like resolved symlinks.
Also I like this code better.
Test Plan: Used and not used custom Celerity map.
Reviewers: epriestley, nh
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D2694
Summary: After D2571, feed for maniphest task creation was being generated non-full-size. Fixed this by properly getting the maniphest action from story data.
Test Plan: Feed seems to work fine and the feed for task creation is full-sized.
Reviewers: epriestley, btrahan
Reviewed By: epriestley
CC: allenjohnashton, ddfisher, aran, Korvin
Differential Revision: https://secure.phabricator.com/D2691
Summary: Moved the files in notification one level up to match the rest of code base. And ran arc liberate.
Test Plan: Made sure __phutil_library_map__ points to the right files.
Reviewers: epriestley, btrahan
Reviewed By: epriestley
CC: allenjohnashton, aran, Korvin
Differential Revision: https://secure.phabricator.com/D2688
Summary: Added `PhabricatorBuiltinPatchList` entry so that "storage upgrade" will update the database. Renamed and numbered the notification.sql patch.
Test Plan: Drop phabricator_feed.feed_storynotification table if it exists and run bin/storage upgrade to check if the patch is correctly applied.
Reviewers: epriestley, btrahan, allenjohnashton
Reviewed By: epriestley
CC: ddfisher, aran, Korvin
Differential Revision: https://secure.phabricator.com/D2687
Summary:
Default of 300 seconds is more than likely too much in most cases.
Provide the option to override.
Test Plan:
Blocked ElasticSearch with iptables
Set timeout to 5 seconds and make sure we error early
Reviewers: epriestley, vrana
Reviewed By: vrana
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D2678
Summary:
TransactionType gives us more information than
update, open, close, assign. We can display those in feed/notifications along with and comments on the actions.
Test Plan: did on local machine tested out.
Reviewers: epriestley, btrahan
Reviewed By: epriestley
CC: ddfisher, keebuhm, aran, Korvin
Differential Revision: https://secure.phabricator.com/D2683
Summary: First diff in a series of diffs to add notifications to Phabricator. This is the notification application ONLY. This commit does not include the changes to other applications that makes them add notifications. As such, no notifications will be generated beyond the initial database import.
Test Plan: This is part of the notifications architecture which has been running on http://theoryphabricator.com for the past several months.
Reviewers: epriestley, btrahan, ddfisher
Reviewed By: epriestley
CC: allenjohnashton, keebuhm, aran, Korvin, jungejason, nh
Maniphest Tasks: T974
Differential Revision: https://secure.phabricator.com/D2571
Summary:
I need this information quite often.
I don't know how many people are working on a single project and this information will be useless for them but I guess it won't hurt much?
Test Plan: Commented on accepted revision.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D2674
Summary: These columns holds the same value in most cases which irritates me.
Test Plan: Displayed history with same author and committer, emulated different committer.
Reviewers: hsb, epriestley
Reviewed By: epriestley
CC: aran, epriestley
Differential Revision: https://secure.phabricator.com/D2671
Summary: As per title: when browsing files in Diffusion, set "Highlighted with blame" as the default view instead of falling back to "Highlighted".
Test Plan: Tested view with a local Diffusion setup, defaults correctly.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Maniphest Tasks: T1278
Differential Revision: https://secure.phabricator.com/D2669
Summary:
This also changes some stuff:
- Reviewers used to be at top, now they are under comments.
- Primary reviewer is now rendered as first.
Test Plan: Added `renderValueForMail()` to a custom field, created diff, commented on it and verified e-mails.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D2664
Summary:
- The config is called "resource-path" and the script references "resource-path", but the actual value checked for is "resource-map".
- Use nonempty(), since defaulting with getEnvConfig() will give you null if the setting exists but is set to null. This default is nearly useless so maybe we should change it to use coalesce().
- Remove Celerity map initialization from warmup. We don't currently initialize the environment in warmup, and Celerity initialization now depends on the environment.
Test Plan: Ran patch locally and on FPM-Warmup.
Reviewers: vrana, btrahan
Reviewed By: vrana
CC: hsb, aran
Differential Revision: https://secure.phabricator.com/D2662
Summary:
Give them a big essay about how it's dangerous, but allow them to do it formally.
Because the username is part of the password salt, users must change their passwords after a username change.
Make password reset links work for already-logged-in-users since there's no reason not to (if you have a reset link, you can log out and use it) and it's much less confusing if you get this email and are already logged in.
Depends on: D2651
Test Plan: Changed a user's username to all kinds of crazy things. Clicked reset links in email. Tried to make invalid/nonsense name changes.
Reviewers: btrahan, vrana
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T1303
Differential Revision: https://secure.phabricator.com/D2657
Summary:
See T1303, which presents a reasonable case for inclusion of these characters in valid usernames.
Also, unify username validity handling.
Test Plan: Created a new user with a valid name. Tried to create a new user with an invalid name. Ran unit tests.
Reviewers: btrahan, vrana
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T1303
Differential Revision: https://secure.phabricator.com/D2651
Summary:
See https://github.com/facebook/phabricator/issues/117
- The $user save can hit a duplicate key exception like the email, but we don't handle it correctly.
- When the $user saves but the $email does not, the $user is left with a (rolled-back, invalid) ID. This makes the UI glitch out a bit. Wipe the ID if we abort the transaction.
- We show the "Required" star marker even if the email is filled in.
The ID issue is sort of a general problem, but I think it's fairly rare: you must be doing inserts on related objects and the caller must catch the transaction failure and attempt to handle it in some way.
I can think of three approaches:
- Manually "roll back" the objects inside the transaction, as here. Seems OK if this really is a rare problem.
- Automatically roll back the 'id' and 'phid' columns (if they exist). Seems reasonable but maybe more complicated than necessary. Won't get every case right. For instance, if we inserted a third object here and that failed, $email would still have the userPHID set.
- Automatically roll back the entire object. We can do this by cloning all the writable fields. Seems like it might be way too magical, but maybe the right solution? Might have weird bugs with nonwritable fields and other random stuff.
We can trigger the rollback by storing objects we updated on the transaction, and either throwing them away or rolling them back on saveTransaction() / killTransaction().
These fancier approaches all seem to have some tradeoffs though, and I don't think we need to pick one yet, since this has only caused problems in one case.
Test Plan: Tried to create a new user (via People -> Create New User) with a duplicate username. Got a proper UI message with no exception and no UI glitchiness.
Reviewers: btrahan, vrana, hgrimberg, hgrimberg01
Reviewed By: hgrimberg01
CC: aran
Differential Revision: https://secure.phabricator.com/D2650
Summary: replace differential.find with differential.query and display in requested order
Test Plan: say D444D222D443D442 and ensure they are in the correct order
Reviewers: epriestley
Reviewed By: epriestley
CC: Mnkras, aran
Differential Revision: https://secure.phabricator.com/D2656
Summary:
We have custom static resources.
We currently include them in Phabricator's celerity resource map which is causing some pain - we need to regenerate the file without our custom resources before pushing upstream, we need to discard our changes before pulling from upstream and we need to rebuild with our changes to run Phabricator.
This diff allows writing and reading the map in other location.
The plan is this - I will run `celerity_mapper.php` twice - once to build Phabricator-only resources (to push to upstream) and once to build Phabricator + ours resoruces to put in our directory.
Better solution would be to create a map just with our resources and read and combine it with Phabricator resources.
But it is complicated because we have dependencies on Phabricator resources.
Test Plan:
`celerity_mapper.php webroot`
`celerity_mapper.php webroot ../facebook/src/__celerity_resource_map__.php`
Delete Phabricator's celerity map, set 'celerity.resource-path' and successfully load Phabricator.
Reviewers: epriestley, edward
Reviewed By: epriestley
CC: aran, Koolvin
Maniphest Tasks: T721
Differential Revision: https://secure.phabricator.com/D2630
Summary:
We need to generate the URI dynamically.
This code is also generally better.
Test Plan: Created custom search selector passing the custom URI to engine.
Reviewers: epriestley, edward
Reviewed By: epriestley
CC: aran, Koolvin
Differential Revision: https://secure.phabricator.com/D2655
Summary: If, e.g., $PATH is broken we may not be able to run "ps". We'll explode pretty hard, currently. Instead, just show a harsher warning.
Test Plan: Changed "ps auxwww" to "psq", which doesn't exist on my system. Loaded page, got warning instead of explosion.
Reviewers: nathanws, vrana, btrahan
Reviewed By: btrahan
CC: aran
Differential Revision: https://secure.phabricator.com/D2624
Summary:
"configuration/default" moved to "default". Rename it to "configuration" and put config stuff back there.
Put writeguard into writeguard/.
Test Plan: tools
Reviewers: vrana
Reviewed By: vrana
CC: aran
Maniphest Tasks: T1103
Differential Revision: https://secure.phabricator.com/D2639
Summary: These were in an unusual location, but are better back in policy/
Test Plan: implicit arc unit
Reviewers: vrana
Reviewed By: vrana
CC: aran
Maniphest Tasks: T1103
Differential Revision: https://secure.phabricator.com/D2638
Summary:
- `kill_init.php` said "Moving 1000 files" - I hope that this is not some limit in `FileFinder`.
- [src/infrastructure/celerity] `git mv utils.php map.php; git mv api/utils.php api.php`
- Comment `phutil_libraries` in `.arcconfig` and run `arc liberate`.
NOTE: `arc diff` timed out so I'm pushing it without review.
Test Plan:
/D1234
Browsed around, especially in `applications/repository/worker/commitchangeparser` and `applications/` in general.
Auditors: epriestley
Maniphest Tasks: T1103
Summary: They were only displaying seconds. I found a function in viewutils.php that allowed for single-unit precision formatting, but I wanted more, so I wrote another function to allow more detail.
Test Plan: [site]/mail, and watch it work. It's a new function, so it shouldn't break anything else.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Koolvin
Maniphest Tasks: T1296
Differential Revision: https://secure.phabricator.com/D2616
Summary: Explain what lint is, how to configure it, what we ship with, etc. Also wax philosophical.
Test Plan: Generated docs, read "carefully".
Reviewers: csilvers, btrahan, vrana
Reviewed By: vrana
CC: aran
Differential Revision: https://secure.phabricator.com/D2628
Summary: These fail on 32-bit machines and seem not valuable enough to try to make work with `bc`, etc.
Test Plan: Ran tests (depends on D2622).
Reviewers: floatinglomas, vrana, btrahan
Reviewed By: vrana
CC: aran
Maniphest Tasks: T1298
Differential Revision: https://secure.phabricator.com/D2623
Summary: Mark these actions with the same markers we use in Differential.
Test Plan: {F12094}
Reviewers: csilvers, jungejason, btrahan
Reviewed By: csilvers
CC: aran
Maniphest Tasks: T1289
Differential Revision: https://secure.phabricator.com/D2601
Summary: D2216 tried to ask the user, this one is explicit.
Test Plan: Click the button
Reviewers: epriestley, lucian
Reviewed By: epriestley
CC: aran, Koolvin
Differential Revision: https://secure.phabricator.com/D2600
Summary: This is an example of code simplification with D2557.
Test Plan: Display user list, verify the SQL queries.
Reviewers: epriestley, btrahan
Reviewed By: epriestley
CC: aran, Koolvin
Differential Revision: https://secure.phabricator.com/D2558
Summary:
Some lint errors (e.g. Javelin) don't have a line number.
Put them on the first line.
Putting them above the first line would be even nicer but much more complicated.
Test Plan: Display diff with lint error on line 0 (D2583).
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Koolvin
Differential Revision: https://secure.phabricator.com/D2599
Summary: Also fix the notice text.
Test Plan:
Display diff with inline comments and lint errors.
Click on inline comment and lint link.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Koolvin
Differential Revision: https://secure.phabricator.com/D2594
Summary:
Allow allowed email addresses to be restricted to certain domains. This implies email must be verified.
This probably isn't QUITE ready for prime-time without a few other tweaks (better administrative tools, notably) but we're nearly there.
Test Plan:
- With no restrictions:
- Registered with OAuth
- Created an account with accountadmin
- Added an email
- With restrictions:
- Tried to OAuth register with a restricted address, was prompted to provide a valid one.
- Tried to OAuth register with a valid address, worked fine.
- Tried to accountadmin a restricted address, got blocked.
- Tried to accountadmin a valid address, worked fine.
- Tried to add a restricted address, blocked.
- Tried to add a valid address, worked fine.
- Created a user with People with an invalid address, got blocked.
- Created a user with People with a valid address, worked fine.
Reviewers: btrahan, csilvers
Reviewed By: csilvers
CC: aran, joe, csilvers
Maniphest Tasks: T1184
Differential Revision: https://secure.phabricator.com/D2581
Summary:
This adds a link to [Closed] e-mail if it detects some changes.
It compares added and removed lines with 3 lines context.
The subtle form of informing is permissive to false negatives and positives.
I have an e-mail filter for [Closed] e-mails so I wouldn't personally notice this change - we should probably promote this feature a little bit.
Test Plan:
Reparse a diff with a change after last update.
Reparse a diff without a change after last update.
Reviewers: jungejason, epriestley
Reviewed By: jungejason
CC: aran, Koolvin, btrahan
Maniphest Tasks: T201
Differential Revision: https://secure.phabricator.com/D2540
Summary:
Our code is quite complex in areas where we prevents the 1+N queries problem explained in [[ http://www.phabricator.com/docs/phabricator/article/Performance_N+1_Query_Problem.html | a performance chapter ]].
This diff adds an abstraction for preventing this code.
Test Plan:
Run all examples mentioned in the doc-comments with logging the queries.
Generate and read docs.
Reviewers: epriestley, btrahan
Reviewed By: epriestley
CC: aran, Koolvin
Differential Revision: https://secure.phabricator.com/D2557
Summary:
I thought about it a little bit and this makes the most sense for me:
# Original author usually writes at least something and commander only updates it.
# There's a creation date of revision (= first diff) by these comments. I don't want to change this date because I use this information. Author should correspond to this date.
# It solves all our repros.
Test Plan: Display commandeered revision.
Reviewers: nh, epriestley
Reviewed By: epriestley
CC: aran, epriestley
Differential Revision: https://secure.phabricator.com/D2577
Summary: this section gets updated for each and every request. clicking a given entry updates the larger dark-console area to have the information from that request
Test Plan: clicked around in maniphest and observed request log populating correctly. clicked a few entries in request log and saw it updated properly. clicked a different tab in the dark-console and it worked. clicked a different request log entry and it opened the dark console to the proper request on the proper tab.
Reviewers: epriestley, vrana
Reviewed By: epriestley
CC: aran, Koolvin
Maniphest Tasks: T1136
Differential Revision: https://secure.phabricator.com/D2574
Summary:
- We currently have some bugs in account creation due to nontransactional user/email editing.
- We save $user, then try to save $email. This may fail for various reasons, commonly because the email isn't unique.
- This leaves us with a $user with no email.
- Also, logging of edits is somewhat inconsistent across various edit mechanisms.
- Move all editing to a `PhabricatorUserEditor` class.
- Handle some broken-data cases more gracefully.
Test Plan:
- Created and edited a user with `accountadmin`.
- Created a user with `add_user.php`
- Created and edited a user with People editor.
- Created a user with OAuth.
- Edited user information via Settings.
- Tried to create an OAuth user with a duplicate email address, got a proper error.
- Tried to create a user via People with a duplicate email address, got a proper error.
Reviewers: btrahan, vrana, jungejason
Reviewed By: btrahan
CC: tberman, aran
Maniphest Tasks: T1184
Differential Revision: https://secure.phabricator.com/D2569
Summary: Better solution would be to reload the page for user with valid token and all data he inserted but I guess that we don't have enough infrastructure for this.
Test Plan: Mangle token and send form.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Koolvin
Differential Revision: https://secure.phabricator.com/D2570
Summary:
Unittest databases are not always destroyed in our setup.
It could be caused by `__destruct()` not called in case of a fatal error.
Test Plan:
arc unit src/applications/calendar/storage/holiday
Reviewers: edward, epriestley
Reviewed By: epriestley
CC: aran, Koolvin
Differential Revision: https://secure.phabricator.com/D2568
Summary:
- Add "role" information, so clients can identify disabled users.
- Formally deprecate `user.info`
Test Plan: Ran "user.query" and "user.whoami", inspected output. Verified "user.info" appears as deprecated in method list and console.
Reviewers: csilvers, btrahan
Reviewed By: csilvers
CC: aran
Differential Revision: https://secure.phabricator.com/D2565
Summary:
Instead of assuming the test plan and summary are written by the author
of the differential revision, let's assume they are written by the author
of the latest differential diff.
Test Plan: viewed a drev that had been commandeered but not updated to check authors
Reviewers: epriestley, jungejason, vrana
Reviewed By: epriestley
CC: aran, Koolvin
Maniphest Tasks: T1235
Differential Revision: https://secure.phabricator.com/D2550
Summary: Just because I like it more.
Test Plan: View diff with comment from disabled user.
Reviewers: epriestley, btrahan
Reviewed By: epriestley
CC: aran, Koolvin
Differential Revision: https://secure.phabricator.com/D2525
Summary:
It's also more readable so I think it's OK.
I've also filed a bug for HHVM.
Test Plan: `arc unit` in HHVM
Reviewers: epriestley, jungejason
Reviewed By: jungejason
CC: aran, Koolvin
Differential Revision: https://secure.phabricator.com/D2551
Summary:
- If you have an unverified primary email, we show a disabled "Primary" button right now in the "Status" column. Instead we should show an enabled "Verify" button, to allow you to re-send the verification email.
- Sort addresses in a predictable way.
Test Plan:
- Added, verified and removed a secondary email address.
- Resent verification email for primary address.
- Changed primary address.
Reviewers: btrahan, csilvers
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T1184
Differential Revision: https://secure.phabricator.com/D2548
Summary: also makes the UI more general for this username + password business.
Test Plan:
- configure a phabricator repository from the svn server @asherwin provided which is configured for svn protocol with SASL
- observed phabricator failing without my patch
- upgraded my SVN client to support SASL (protip for mac users - http://www.wandisco.com/subversion/download#osx)
- applied patch to phabricator
- restarted daemons
- noted daemon success - diffusion populating nicely
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Koolvin
Maniphest Tasks: T1260
Differential Revision: https://secure.phabricator.com/D2549
Summary:
Git and hg (supposedly..) differentiate between an author (who wrote the patch)
and a committer (who applied the patch).
This patch allows Phabricator to note when a patch is committed
by someone other than the Author.
Test Plan:
Created 2 accounts,
- U (Account with a PHID)
- U' (Account without a PHID)
and had them create and commit commits
testing if their username/real name would be displayed correctly in Diffusion,
- BrowserTable
- HistoryTable
- Code revision
Teztz,
A(uthor)/C(ommitter)
If it's A/A then Author committed
UL = User link (<a href="/p/username">username</a>)
UN = User name ("Firstname Lastname")
Tezt | Expected in table | Got
-------------------------------------------
A/A | UL/UL | UL/UL
A'/C | UN/UL | UN/UL
A/C' | UL/UN | UL/UN
A'/C' | UN/UN | UN/UN
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Koolvin
Maniphest Tasks: T688
Differential Revision: https://secure.phabricator.com/D2541
Summary:
If someone typos one name in a cc or reviewer list, it would be nice if we
display all of the valid names in the field when running arc diff (in addition
to the error message).
Test Plan:
used the conduit console to check that calling differential.parsecommitmessage
with a list of some valid and some invalid ccs returns a result with both an
error and a list of some ccs. Also ran arc diff with that list of ccs to check
for the correct user experience.
Reviewers: epriestley, jungejason, vrana
Reviewed By: epriestley
CC: aran, Koolvin
Differential Revision: https://secure.phabricator.com/D2542
Summary:
This attaches commit diff to its associated revision as any other diff.
The consequence is that the revision page now shows the actual commit instead of the last diff. It may be disturbing but it is desired.
Another consequence is that lint and unit results are displayed as skipped after the revision was committed. I want to fix it somehow.
My next plan is to automatically diff against the last normal diff and include the link to this diff in commit e-mail if non-empty.
Test Plan:
reparse.php
Diff against last normal diff.
Reviewers: epriestley, jungejason
Reviewed By: jungejason
CC: aran, Koolvin, jungejason
Maniphest Tasks: T201
Differential Revision: https://secure.phabricator.com/D2530
Summary:
Allow callers to find all packages/paths owned by a given
user/project.
Test Plan:
Used conduit api page with --
user owner: PHID-USER-6ce1c976b86e5f3c34f6 (tried both loadPackagesFromProjects
true/false)
proj owner: PHID-PROJ-r5wnmmaawqsn4tvjmqm4
repo/path: E, /tfb/trunk/www/flib/privacy. Checked both path.getowners
and owners.query
(all of these are defined internally at fb)
Reviewers: epriestley, btrahan, nh
Reviewed By: btrahan
CC: aran, Koolvin
Differential Revision: https://secure.phabricator.com/D2482
Summary:
Mercurial renamed "--only-branch" to "--branch" about two years ago. "-b" exists in both versions.
http://www.selenic.com/pipermail/mercurial-devel/2010-April/020469.html
We have a few other cases where we use features that exist only in recent Mercurial (notably, 'ancestors' in log) but we can work around this one easily.
Test Plan: Looked at a Mercurial repo in Diffusion, verified that "log -b" commands issued and that the output was correct.
Reviewers: btrahan, Makinde, ipalaus
Reviewed By: ipalaus
CC: aran
Maniphest Tasks: T1264
Differential Revision: https://secure.phabricator.com/D2533
Summary: Add support for "tests", "testplan" and "tested" as alias of "Test Plan".
Test Plan: Created a diff with test plan specified in "tests".
Reviewers: csilvers, btrahan
Reviewed By: csilvers
CC: aran
Differential Revision: https://secure.phabricator.com/D2531
Summary:
It's currently possible to configure Phabricator to send mail to some address it recognizes as relating to an object.
When we receive mail from Phabricator, drop it unconditionally.
Test Plan: Wrote two emails, one with the header and one without. Piped them to `mail_handler.php`, one was dropped immediately.
Reviewers: btrahan, nh, mikaaay, jungejason
Reviewed By: jungejason
CC: aran
Differential Revision: https://secure.phabricator.com/D2529
Summary:
Since user emails aren't in the user table, we had to do extra data fetching
for handles, and the emails are only used in MetaMTA, so we move the email
code into MetaMTA and remove it from handles.
Test Plan: send test emails
Reviewers: jungejason, vrana, epriestley
Reviewed By: epriestley
CC: aran, Koolvin
Differential Revision: https://secure.phabricator.com/D2494
Summary:
We introduced a "user.query" call recently which is only about two weeks old. Bump versions so users get a forced upgrade.
Also, we raise a fairly confusing message when the user calls a nonexistent method. This is not the intent; `class_exists()` throws. Tailor this exception more carefully.
Test Plan:
- Ran `echo {} | arc call-conduit derp.derp`, got a better exception.
- Bumped version, ran `arc list`, got told to upgrade.
Reviewers: indiefan, nh, vrana, btrahan, jungejason, Makinde
Reviewed By: vrana
CC: aran
Differential Revision: https://secure.phabricator.com/D2527
Summary: as title
Test Plan: no
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Koolvin
Differential Revision: https://secure.phabricator.com/D2526
Summary:
The current state is very confusing:
{F11734, size=full}
Test Plan: Display calendar for user with two different events in the same week.
Reviewers: epriestley, btrahan
Reviewed By: epriestley
CC: aran, Koolvin
Differential Revision: https://secure.phabricator.com/D2522
Summary:
Allow installs to require users to verify email addresses before they can use Phabricator. If a user logs in without a verified email address, they're given instructions to verify their address.
This isn't too useful on its own since we don't actually have arbitrary email registration, but the next step is to allow installs to restrict email to only some domains (e.g., @mycompany.com).
Test Plan:
- Verification
- Set verification requirement to `true`.
- Tried to use Phabricator with an unverified account, was told to verify.
- Tried to use Conduit, was given a verification error.
- Verified account, used Phabricator.
- Unverified account, reset password, verified implicit verification, used Phabricator.
- People Admin Interface
- Viewed as admin. Clicked "Administrate User".
- Viewed as non-admin
- Sanity Checks
- Used Conduit normally from web/CLI with a verified account.
- Logged in/out.
- Sent password reset email.
- Created a new user.
- Logged in with an unverified user but with the configuration set to off.
Reviewers: btrahan, vrana, jungejason
Reviewed By: btrahan
CC: aran, csilvers
Maniphest Tasks: T1184
Differential Revision: https://secure.phabricator.com/D2520
Summary: Ideally there should be a "send epriestley this profile" button but this is a reasonable step forward. Add a "download .xhprof profile" button to profiles, since walking through these things remotely is pretty awkward. Also expand "Excl" and "Incl" acronyms.
Test Plan: Clicked download button.
Reviewers: csilvers, btrahan, vrana
Reviewed By: csilvers
CC: aran
Differential Revision: https://secure.phabricator.com/D2523
Summary:
This is a rough cut, but gets some of the basics at least. Here's what it looks like:
{F11690}
Some things that would be nice for future diffs:
- Different colors for different event types (tasks? MEETINGS?!)
- When events span across multiple days, keep them in the same row.
- Switch which month you're looking at.
- Show specific users instead of all.
- etc etc etc
Test Plan: See screenshot.
Reviewers: btrahan, vrana
Reviewed By: btrahan
CC: aran
Differential Revision: https://secure.phabricator.com/D2514
Summary:
we were parsing the git log output slightly incorrectly and over-exploding on spaces. we also needed to escape the path %20 stuff`.
Not sure if there's something fancy to do given folks should reparse their repos if they are impacted by this issue.
Test Plan:
made a directory with spaces and some dummy revisions. observed diffs wouldn't load and links broken.
with patch, ran scripts/reparse.php for pertinent revisions and diffs loaded and links weren't broken.
Reviewers: floatinglomas, epriestley
Reviewed By: epriestley
CC: aran, Koolvin
Maniphest Tasks: T1252
Differential Revision: https://secure.phabricator.com/D2510
Summary: See T1254, until this gets paginated properly we can at least show more results. 25 is pretty anemic.
Test Plan: Tweaked limit, verified result count was affected.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T1254
Differential Revision: https://secure.phabricator.com/D2513
Summary: If you have an empty value saved in the "default branch" field, we default to empty string (or null, or whatever) instead of the correct default.
Test Plan:
- Looked at a Git repo with an empty default branch, got a default to "master" instead of an error.
- Looked at a Mercurial repo with an empty default branch, got a default to "default" instead of an error.
Reviewers: btrahan, csilvers
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T1237
Differential Revision: https://secure.phabricator.com/D2512
Summary:
- We currently write every PHID we generate to a table. This was motivated by two concerns:
- **Understanding Data**: At Facebook, the data was sometimes kind of a mess. You could look at a random user in the ID tool and see 9000 assocs with random binary data attached to them, pointing at a zillion other objects with no idea how any of it got there. I originally created this table to have a canonical source of truth about PHID basics, at least. In practice, our data model has been really tidy and consistent, and we don't use any of the auxiliary data in this table (or even write it). The handle abstraction is powerful and covers essentially all of the useful data in the app, and we have human-readable types in the keys. So I don't think we have a real need here, and this table isn't serving it if we do.
- **Uniqueness**: With a unique key, we can be sure they're unique, even if we get astronomically unlucky and get a collision. But every table we use them in has a unique key anyway. So we actually get pretty much nothing here, except maybe some vague guarantee that we won't reallocate a key later if the original object is deleted. But it's hard to imagine any install will ever have a collision, given that the key space is 36^20 per object type.
- We also currently use PHIDs and Users in tests sometimes. This is silly and can break (see D2461).
- Drop the PHID database.
- Introduce a "Harbormaster" database (the eventual CI tool, after Drydock).
- Add a scratch table to the Harbormaster database for doing unit test meta-tests.
- Now, PHID generation does no writes, and unit tests are isolated from the application.
- @csilvers: This should slightly improve the performance of the large query-bound tail in D2457.
Test Plan: Ran unit tests. Ran storage upgrade.
Reviewers: btrahan, vrana, jungejason
Reviewed By: btrahan
CC: csilvers, aran, nh, edward
Differential Revision: https://secure.phabricator.com/D2466
Summary: These patterns are hard-coded, allow them to match case-insenstiviely.
Test Plan: Typed "d3" and "D3", got the right object in the attach dialog.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T1253
Differential Revision: https://secure.phabricator.com/D2511
Summary: ...they were broken...
Test Plan: clicked links for both SVN and Git repos and got working results
Reviewers: vrana, floatinglomas, 20after4
Reviewed By: floatinglomas
CC: aran, epriestley
Maniphest Tasks: T1250
Differential Revision: https://secure.phabricator.com/D2505
Summary:
only show the blank, "create new" wiki page for the project if the project actually exists; only allow edit if the project actually exists.
Small wrinkle here is not checking if the project actually exists if the page already exists.
Test Plan:
- viewed a project wiki page
- viewed a prokect wiki page for a fake project and got a 404
- edited a project wiki page
- edited a project wiki page for a fake project and got a 404
Reviewers: epriestley, jacktrades
Reviewed By: epriestley
CC: aran, Koolvin
Maniphest Tasks: T1248
Differential Revision: https://secure.phabricator.com/D2506
Summary: we weren't actually removing any edges. now we do.
Test Plan: had a commit associated with task x; removed association. had a commit associated with task x; removed association while adding a different one.
Reviewers: floatinglomas, epriestley
Reviewed By: epriestley
CC: aran, Koolvin
Maniphest Tasks: T1256
Differential Revision: https://secure.phabricator.com/D2509
Summary: we need a user (the viewer in this case) for the status to render correctly with respect to timezone
Test Plan: my profile no longer fatals with an away status
Reviewers: davidreuss, vrana
CC: aran, epriestley
Differential Revision: https://secure.phabricator.com/D2504
Summary: D2490 was not my finest hour and I incorrectly thought it was a null value from error. In reality this error is impossible and its just a valid empty array so instead use the empty predicate to bail.
Test Plan: with our logic combined, this be tested
Reviewers: vrana, epriestley
Reviewed By: epriestley
CC: aran, Koolvin
Differential Revision: https://secure.phabricator.com/D2503
Summary: see T1241, T1242, T1244 for some examples of crud getting saved
Test Plan: threw some crud in my conduit console and got reasonable errors back
Reviewers: mikaaay, epriestley
Reviewed By: epriestley
CC: aran, Koolvin
Maniphest Tasks: T1241, T1242, T1244
Differential Revision: https://secure.phabricator.com/D2487
Summary: D2492#6
Test Plan:
`user.query` of user which is away.
`user.query` of user which is not away.
`user.whoami` - no information there.
Reviewers: epriestley
Reviewed By: epriestley
CC: btrahan, aran, Koolvin
Differential Revision: https://secure.phabricator.com/D2502
Summary: there were 6 values, but we only need 4. delete the two empty ones as the others are all stylistically valid.
Test Plan: ...looks good!
Reviewers: ric03uec, vrana, epriestley
Reviewed By: epriestley
CC: aran, epriestley
Maniphest Tasks: T1247
Differential Revision: https://secure.phabricator.com/D2498
Summary: There's no method for enabling users somewhat intentionally.
Test Plan: Disable myself (oops, this is probably my last diff ever).
Reviewers: btrahan
Reviewed By: btrahan
CC: aran, epriestley
Differential Revision: https://secure.phabricator.com/D2497
Test Plan: Display revision list both with last reviewer and without.
Reviewers: epriestley, btrahan
Reviewed By: btrahan
CC: aran, Koolvin
Differential Revision: https://secure.phabricator.com/D2495
Summary:
This is not so general as `getRequiredHandlePHIDs()`.
It allows bulk loading of user statuses only in revision list.
It also loads data in `render()`. I'm not sure if it's OK.
Maybe we can use the colorful point here.
Or maybe some unicode symbol?
Test Plan: {F11451, size=full}
Reviewers: btrahan, epriestley
Reviewed By: btrahan
CC: aran, Koolvin
Differential Revision: https://secure.phabricator.com/D2484
Summary:
I want to use this to warn user if he specifies reviewers that are away.
We can also implement a general query method but I think that this usage is the
most useful not only for me but also in general case.
Test Plan:
Call the method for user which is away and which is not away.
Add user status through Conduit.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran, epriestley
Differential Revision: https://secure.phabricator.com/D2492
Summary: 'cuz github issue 114 came into existence. instead, just return false early here. note i am not sure if I should phlog that this is happening or not but its not exception worthy IMO.
Test Plan: lint-only 'cuz i don't want to setup an IRC server locally / somehow get my local phabricator instance accessible out there. happy to test end to end if there's an easier way...!
Reviewers: vrana, epriestley
Reviewed By: vrana
CC: aran, Koolvin
Differential Revision: https://secure.phabricator.com/D2490
Summary:
I've tried colors, italics, strike-through and this is a compromise between me and @leebyron.
NOTE: I don't want to disturb @epriestley from playing Diablo 3.
Test Plan: {F11439, size=full}
Reviewers: btrahan
Reviewed By: btrahan
CC: aran, epriestley, leebyron
Differential Revision: https://secure.phabricator.com/D2481
Summary:
Occasionally a bot will get subscribed to a differential revision; when
this happens we shouldn't send them an email, because otherwise the
author of the diff will get a bounce email if the bot has an invalid
emal address.
Test Plan:
re-sent a message that had a bot on the cc list, saw that it was no longer
included when it got sent.
Reviewers: epriestley, jungejason, vrana
CC: aran, Koolvin
Differential Revision: https://secure.phabricator.com/D2479
Summary: the existing, more custom text doesn't make sense when viewing someone else's revisions AND in someplaces its the less interesting "no data found".
Test Plan: clicked around the various http://phabricator.dev/differential/filter/* pages with various users.
Reviewers: epriestley, ry
Reviewed By: epriestley
CC: aran, Koolvin
Maniphest Tasks: T1222
Differential Revision: https://secure.phabricator.com/D2475
Summary:
D2470 added Package mailhandler, which was configured incorrectly in the getRequiredClasses function. this makes it like the other mail handlers
Reported at https://github.com/facebook/phabricator/issues/112
Test Plan: setup mode no longer fails
Reviewers: epriestley, jungejason, royklopper
Reviewed By: royklopper
CC: aran, Koolvin
Differential Revision: https://secure.phabricator.com/D2476
Summary:
D2457 and D2459 made some changes here. This little guy just needed to be touched so it could be arc lint'd to have the proper updated include paths
Sorry to spam reviewers; Evan is in Diablo 3 somewhere. :D
Test Plan: no more fatal on test everything implemented (arc unit src/infrastructure/__tests__/)
Reviewers: epriestley, jungejason, nh, csilvers
CC: aran, Koolvin
Differential Revision: https://secure.phabricator.com/D2474
Summary:
For package creation and deletion, send email to all the owners For
package modification, detect important fields such as owners and paths, and then
send out emails to all owners (including deleted owners and current owners)
Also start using transaction for package creation/deletion/modification.
Test Plan:
- tested mail creation and deletion
- tested modification to auditing enabled, primary owners, owners, paths
Reviewers: epriestley, nh, vrana
Reviewed By: epriestley
CC: prithvi, aran, Koolvin
Differential Revision: https://secure.phabricator.com/D2470
Summary:
Required in order to run tests successfully in the HipHop interpreter.
Similar to D2362.
Test Plan: Run the tests in an HipHop runtime.
Reviewers: epriestley
Reviewed By: epriestley
CC: jungejason, aran, Koolvin, vrana
Differential Revision: https://secure.phabricator.com/D2365
Summary: We had a real issue where revision was marked as accepted but the comment wasn't saved.
Test Plan: Reclaim abandoned revision.
Reviewers: epriestley
Reviewed By: epriestley
CC: jungejason, aran, Koolvin
Differential Revision: https://secure.phabricator.com/D2463
Summary: Prelminary, I want to test this a bit more when I'm better rested. Provide a "bulk" import mode for Mercurial so we can do initial discovery more quickly.
Test Plan: Imported the Mercurial repository in 2m45s, blocked on MySQL I/O rather than Mercurial I/O.
Reviewers: csilvers, btrahan
Reviewed By: csilvers
CC: aran
Differential Revision: https://secure.phabricator.com/D2457
Summary:
Also add some formatting and links.
Also fix test broken by D2393.
Test Plan:
diviner .
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Koolvin
Differential Revision: https://secure.phabricator.com/D2461
Summary: This could use some additional explanation.
Test Plan: Read text.
Reviewers: btrahan, aurelijus
Reviewed By: aurelijus
CC: aran
Maniphest Tasks: T1204
Differential Revision: https://secure.phabricator.com/D2446
Summary: For git workflows where developers push personal feature branches to the origin, this is a quick workaround until T1210 is implemented properly. I'll also mention this in D2446.
Test Plan:
- Committed a revision in an "autoclose" repository; it was autoclosed. Committed a revision in a no-autoclose repository, it was not autoclosed.
- Edited repositories, saving the "autoclose" setting as enabled/disabled.
Reviewers: aurelijus, btrahan
Reviewed By: aurelijus
CC: aran
Maniphest Tasks: T1210
Differential Revision: https://secure.phabricator.com/D2448
Summary:
- Support offset/limit as added by D2442, for Mercurial.
- We just list everything and slice, no clear way to do better and this isn't a major perf issue.
- No clear way to easily get by-update sorting, we can implement this by looking up revs if someone asks.
- Bury some of the Git implementation details inside the Git query.
Test Plan:
- Looked at a Git repo, clicked "View All Branches".
- Looked at a Hg repo, clicked "View All Branches".
- Limited page size to 1, made sure it limited properly.
Reviewers: aurelijus, btrahan, vrana
Reviewed By: aurelijus
CC: aran
Differential Revision: https://secure.phabricator.com/D2453
Summary:
Sorting by last commit date
Branch view limit to 25 branches
All branches table page with pagination on Git
Test Plan:
* Check repository view for expected behavior on branch table view
* Check all branches page & test pagination
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Koolvin
Maniphest Tasks: T1200
Differential Revision: https://secure.phabricator.com/D2442
Summary: Revisions don't necessarily have a backing repository, as in "--raw".
Test Plan: Looked at a "--raw" revision in accepted and closed states.
Reviewers: nh, btrahan
Reviewed By: nh
CC: aran
Differential Revision: https://secure.phabricator.com/D2444
Summary:
The highlighting is distracting according to Nick Shrock and others.
Real designer, Lee Byron, helped me with this.
It also gives us unagressive target for jumping to the source line in future.
Another feature I will probably implement is highlighting also the source of copies/moves.
I will use the right side of the left column for it.
Test Plan:
Hover copied notifier.
Hover coverage notifier.
I've also checked that this doesn't break our super-flaky old/new code JavaScript detector.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Koolvin, leebyron, schrockn
Differential Revision: https://secure.phabricator.com/D2443
Summary:
- When you have an un-cloned repository, we currently throw random-looking Git/Hg exception. Instead, throw a useful error.
- When you have a cloned but undiscovered repository, we show no commits. This is crazy confusing. Instead, show commits as "importing...".
- Fix some warnings and errors for empty path table cases, etc.
Test Plan:
- Wiped database.
- Added Mercurial repo without running daemons. Viewed in Diffusion, got a good exception.
- Pulled Mercurial repo without discovering it. Got "Importing...".
- Discovered Mercurial repo without parsing it. Got "Importing..." plus date information.
- Parsed Mercurial repo, got everything working properly.
- Added Git repo without running daemons, did all the stuff above, same results.
- This doesn't improve SVN much but that's a trickier case since we don't actually make SVN calls and rely only on the parse state.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T776
Differential Revision: https://secure.phabricator.com/D2439
Summary:
- Merge CommitTask daemon into PullLocal daemon. This is another artifact of past instability (and order-dependent parsers). We still publish to the timeline, although this was the last consumer. Long term we'll probably delete timeline and move to webhooks, since everyone who has asked about this stuff has been eager to trade away the durability and ordering of the timeline for the ease of use of webhooks. There's also no reason to timeline this anymore since parsing is no longer order-dependent.
- Add `phd start` to start all the daemons you need. Add `phd restart` to restart all the daemons you need. So cool~
- Simplify and improve phd and Diffusion daemon documentation.
Test Plan:
- Ran `phd start`.
- Ran `phd restart`.
- Generated/read documentation.
- Imported some stuff, got clean parses.
Reviewers: btrahan, csilvers
Reviewed By: csilvers
CC: aran, jungejason, nh
Differential Revision: https://secure.phabricator.com/D2433
Summary: We raise an improved exception for missing tables/columns, but not databases.
Test Plan: Hit a "no such database error", got a better error message pointing me at storage upgrades.
Reviewers: btrahan, jungejason
Reviewed By: btrahan
CC: aran
Differential Revision: https://secure.phabricator.com/D2429
Summary: When the server version is ahead of the client version, send a more exciting error!!!
Test Plan: omg~~~
Reviewers: nh, Makinde, btrahan, jungejason
Reviewed By: nh
CC: aran
Differential Revision: https://secure.phabricator.com/D2436
Summary: Add history column & history link per branch on branch table view, also later add some more features like last commit date, etc.
Test Plan: Checked if History column is in browser table view & history links are properly linked.
Reviewers: epriestley
Reviewed By: epriestley
CC: davidreuss, aran, Koolvin
Maniphest Tasks: T1201, T1202, T1200
Differential Revision: https://secure.phabricator.com/D2432
Summary:
See D2418. This merges the commit discovery daemon into the same single daemon, and applies all the same rules to it.
There are relatively few implementation changes, but a few things did change:
- I simplified/improved Mercurial importing, by finding full branch tip hashes with "--debug branches" and using "parents --template {node}" so we don't need to do separate "--debug id" calls.
- Added a new "--not" flag to exclude repositories, since I switched to real arg parsing anyway.
- I removed a web UI notification that you need to restart the daemons, this is no longer true.
- I added a web UI notification that no pull daemon is running on the machine.
NOTE: @makinde, this doesn't change anything from your perspective, but it something breaks this is the likely cause.
This implicitly resolves T792, because discovery no longer runs before pulling.
Test Plan:
- Swapped databases to a fresh install.
- Ran "pulllocal" in debug mode. Verified it correctly does nothing (fixed a minor issue with min() on empty array).
- Added an SVN repository. Verified it cloned and discovered correctly.
- Added a Mercurial repository. Verified it cloned and discovered correctly.
- Added a Git repository. Verified it cloned and discovered correctly.
- Ran with arguments to verify behaviors: "--not MTEST --not STEST", "P --no-discovery", "P".
Reviewers: btrahan, csilvers, Makinde
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T792
Differential Revision: https://secure.phabricator.com/D2430
Summary:
also fix some bugs where we weren't properly capturing the expiry value or scope of access tokens.
This code isn't the cleanest as some providers don't confirm what scope you've been granted. In that case, assume the access token is of the minimum scope Phabricator requires. This seems more useful to me as only Phabricator at the moment really easily / consistently lets the user increase / decrease the granted scope so its basically always the correct assumption at the time we make it.
Test Plan: linked and unlinked Phabricator, Github, Disqus and Facebook accounts from Phabricator instaneces
Reviewers: epriestley
Reviewed By: epriestley
CC: zeeg, aran, Koolvin
Maniphest Tasks: T1110
Differential Revision: https://secure.phabricator.com/D2431
Summary:
For most actions (like "accept"), we write a row only if you aren't acting on behalf of anything else. This avoids cases like every accept causing two relationships:
Some Project | Accept
Some User | Accept
For "Resign", we must always write the row. Break the logic out and handle it separately.
Test Plan: Poked it locally, but let me know if this fixes things?
Reviewers: 20after4, btrahan
Reviewed By: 20after4
CC: aran
Differential Revision: https://secure.phabricator.com/D2423
Summary: Mention that you don't actually need an `.arcconfig` any more.
Test Plan: Read documentation.
Reviewers: btrahan, csilvers
Reviewed By: csilvers
CC: aran
Differential Revision: https://secure.phabricator.com/D2425
Summary:
Allow the pull daemon to take a list of repositories. By default, pull all repositories.
Make some effort to respect pull frequencies, although we'll necessarily suffer a bit if running with only one process.
NOTE: We still launch one discovery daemon per working copy, so this only cuts the daemon count in half.
Test Plan:
- Ran `phd debug pulllocal`, verified behavior.
- Ran `pull.php P MTEST SVNTEST --trace`, verified it pulled the repos and ran the right commands.
- Ran `phd repository-launch-master`, verified the right daemons launched, checked daemon console.
- Ran `phd repository-launch-readonly`, verified the right daemon launched, checked daemon console.
Reviewers: btrahan, csilvers, davidreuss
Reviewed By: csilvers
CC: aran
Differential Revision: https://secure.phabricator.com/D2418
Summary: specify user table to make things not ambiguous
Test Plan: conduit console still works, including ID query...!
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Koolvin
Maniphest Tasks: T1075
Differential Revision: https://secure.phabricator.com/D2419
Summary:
When you create a new task, the UI gives you the option to create another similar task. We copy some fields, but not others.
Currently, the field list is hard-coded and excludes auxiliary fields. Instead, allow auxiliary fields to elect to be copied.
Test Plan:
- Created a new task, verified appropriate field defuaults.
- Created a new "similar" task, verified 'copy' fields copied in.
- Edited an existing task, verified appropriate values.
- Edited-with-errors, verified new values didn't get reverted in the form.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T1193
Differential Revision: https://secure.phabricator.com/D2410
Summary: The PHID list is a list, not a map -- I must have broken this in refactoring or something, since everything else works fine. See D2013.
Test Plan: Viewed a resignable revision, saw "Resign" (new after this commit), resignd.
Reviewers: 20after4, btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T904
Differential Revision: https://secure.phabricator.com/D2417
Summary:
- Move email to a separate table.
- Migrate existing email to new storage.
- Allow users to add and remove email addresses.
- Allow users to verify email addresses.
- Allow users to change their primary email address.
- Convert all the registration/reset/login code to understand these changes.
- There are a few security considerations here but I think I've addressed them. Principally, it is important to never let a user acquire a verified email address they don't actually own. We ensure this by tightening the scoping of token generation rules to be (user, email) specific.
- This should have essentially zero impact on Facebook, but may require some minor changes in the registration code -- I don't exactly remember how it is set up.
Not included here (next steps):
- Allow configuration to restrict email to certain domains.
- Allow configuration to require validated email.
Test Plan:
This is a fairly extensive, difficult-to-test change.
- From "Email Addresses" interface:
- Added new email (verified email verifications sent).
- Changed primary email (verified old/new notificactions sent).
- Resent verification emails (verified they sent).
- Removed email.
- Tried to add already-owned email.
- Created new users with "accountadmin". Edited existing users with "accountadmin".
- Created new users with "add_user.php".
- Created new users with web interface.
- Clicked welcome email link, verified it verified email.
- Reset password.
- Linked/unlinked oauth accounts.
- Logged in with oauth account.
- Logged in with email.
- Registered with Oauth account.
- Tried to register with OAuth account with duplicate email.
- Verified errors for email verification with bad tokens, etc.
Reviewers: btrahan, vrana, jungejason
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T1184
Differential Revision: https://secure.phabricator.com/D2393
Summary: The various interfaces here are in conflict about what a role is and isn't. Make them all consistent.
Test Plan: Edited some users into various roles, verified they reported correctly.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T1190
Differential Revision: https://secure.phabricator.com/D2415
Summary:
- In practice, 'edit' has two modes, 'create' and 'edit'. These seem like they should map to "create a revision" and "update a revision", but they are completely different.
- We use the "create" mode:
- When creating a message from the working copy.
- When creating a message from a file.
- When creating a message from a commit.
- When creating a message from a user template.
- When creating a message from an "--edit"!
- We use the "edit" mode:
- ONLY when updating a revision with `arc diff --verbatim`.
- The only difference is in which fields may be overwritten. Under "create", all fields may be overwritten. Under "edit", only safe fields may be overwritten.
- The "Differential Revision" field currently does not render in either edit mode. This is wrong. Even though it can not be updated in the "edit" mode, it should still render in both modes. This is the only material change this revision makes.
- Without this change, when we "create" a new message from a working copy and the working copy has a "Differential Revision" field, we incorrectly discard it.
- The only field which does not render on edit modes now is "Reviewed by" (not "Reviewers"), which is correct, since we do not read the value.
Test Plan: Ran "arc diff" to create/update revisions. Ran "arc diff --verbatim" to create/update revisions with implicit edits (with D2411). Ran "arc diff --edit" to update revisions with explicit edits.
Reviewers: jungejason, btrahan
Reviewed by: jungejason
CC: vrana, aran
Differential Revision: https://secure.phabricator.com/D2412
Summary:
- When a user uploads an oversized file, throw an exception.
- When an uncaught exception occurs during a Conduit request, return a Conduit response.
- When an uncaught exception occurs during a non-workflow Ajax request, return an Ajax response.
Test Plan:
- Uploaded overlarge files.
- Hit an exception page with ?__ajax__=1 and ?__conduit__=1
Reviewers: btrahan, vrana, jungejason
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T875, T788
Differential Revision: https://secure.phabricator.com/D2385
Summary:
- We used to have connection-level caching, so we needed getTransactionKey() to make sure there was one transaction state per real connection. We now cache in Lisk and each Connection object is guaranteed to represent a real, unique connection, so we can make this a non-static.
- I kept the classes separate because it was a little easier, but maybe we should merge them?
- Also track/implement read/write locking.
- (The advantage of this over just writing LOCK IN SHARE MODE is that you can use, e.g., some Query class even if you don't have access to the queries it runs.)
Test Plan: Can you come up with a way to write unit tests for this? It seems like testing that it works requires deadlocking MySQL if the test is running in one process.
Reviewers: vrana, btrahan
Reviewed By: vrana
CC: aran
Differential Revision: https://secure.phabricator.com/D2398
Summary:
The highlighting is distracting according to Nick Shrock and others.
Real designer, Lee Byron, helped me with this.
It also gives us unagressive target for jumping to the source line in future.
Another feature I will probably implement is highlighting also the source of copies/moves.
I will use the right side of the left column for it.
Test Plan:
Hover copied notifier.
Hover coverage notifier.
I've also checked that this doesn't break our super-flaky old/new code JavaScript detector.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Koolvin, leebyron, schrockn
Differential Revision: https://secure.phabricator.com/D2403
Summary: We currently try to do "app login" for all OAuth providers, but not all of them support it in a meaningful way. Particularly, it always fails for Google.
Test Plan: Ran google diagnostics on a working config, no longer got a diagnostic failure.
Reviewers: btrahan, vrana, csilvers
Reviewed By: csilvers
CC: aran
Differential Revision: https://secure.phabricator.com/D2377
Summary: Primarily for @csilvers who has 92 million repositories or something. This is a touch hacky, but movitated by pragmatism.
Test Plan:
- Ran "repository.create" to create repositories, "repository.query" to list them.
- Tested most or maybe all of the error conditions, probably.
Reviewers: btrahan, vrana, csilvers
Reviewed By: csilvers
CC: aran
Differential Revision: https://secure.phabricator.com/D2396
Summary: Generally moves us toward having a sane approach to transaction handling.
Test Plan: See test case, which fails before this patch and passes afterwards.
Reviewers: vrana, btrahan, jungejason
Reviewed By: vrana
CC: aran
Differential Revision: https://secure.phabricator.com/D2394
Summary:
It saves some time on non-highlighting generated and other not interesting code.
The code is quite complex (300 lines methods) so I'm not sure if everything is moved correctly.
P.S. I hope that moved code detector will work...
Test Plan:
Display generated file with all whitespace, verify that it is not highlighted.
Display normal file.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Koolvin
Maniphest Tasks: T1134
Differential Revision: https://secure.phabricator.com/D2358
Summary:
I wanted to point someone on a file uploaded to Phabricator and the normal link is just too long.
I guess that this also improves security. Because pointing someone to the file directly reveals the secret key used in /data/ and it can be served without auth?
We already use `{F123}` so there will be no conflicts in future because we wouldn't want to reuse it for something else.
I promote the link on /file/ - it adds one redirect but I think it's worth it. I also considered making the link from the File ID column but there are already too many links (with some duplicity).
Test Plan:
/file/
/F123 (redirect)
/F9999999999 (404)
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Koolvin
Differential Revision: https://secure.phabricator.com/D2380
Summary:
I will use it for highlighting users which are not currently available.
Maybe I will also use it in the nagging tool.
I don't plan creating a UI for it as API is currently enough for us.
Maybe I will visualize it at /calendar/ later.
I plan creating `user.deletestatus` method when this one will be done.
Test Plan:
`storage upgrade`
Call Conduit `user.addstatus`.
Verify DB.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Koolvin
Differential Revision: https://secure.phabricator.com/D2382
Summary: I have a patch which makes uploads all fancy and adds progress bars, but document the landscape first since it's quite complicated.
Test Plan: Generated, read docs. Configured `storage.upload-size-limit` to various values.
Reviewers: btrahan, vrana
Reviewed By: vrana
CC: aran
Maniphest Tasks: T875
Differential Revision: https://secure.phabricator.com/D2381
lighttpd could support rules like this, to add efficiency:
# Aggressively cache all static files
$HTTP["url"] =~ "\.(jpg|gif|png|css|js|htc)" {
expire.url = ( "" => "access 1 years" )
}
# Compress files for faster transfer
compress.filetype = (
"text/plain",
"text/html",
"text/javascript",
"text/css",
"text/xml"
)
compress.cache-dir = <would need to set to something>?
I don't know if that is necessary or useful. Probably not a good idea
at this point, where the code is changing so rapidly: a 1 year cache
of javascript code could cause trouble. And i think the default
lighttpd.conf already compresses text/html, text/plain, text/css, and
application/x-javascript by default, so we're ok there (could add
text/javascript and text/xml, I guess).
Summary: We currently make a ludicrously gigantic permission request to do Google auth (read/write access to the entire address book), since I couldn't figure out how to do a more narrowly tailored request when I implemented it. @csilvers pointed me at some much more sensible APIs; we can now just ask for user ID, name, and email address.
Test Plan: Created a new account via Google Oauth. Linked/unlinked an existing account. Verified diagnostics page still works correctly. Logged in with a pre-existing Google account created with the old API (to verify user IDs are the same through both methods).
Reviewers: btrahan, vrana, csilvers, Makinde
Reviewed By: csilvers
CC: aran
Differential Revision: https://secure.phabricator.com/D2378
Summary: I will need it for nagging tool.
Test Plan:
None yet.
Please suggest me how to create a testing database (I need to insert some data in the table). I guess that it is now possible?
There is also probably some bug in `arc unit` - `setEnvConfig()` is not called before `getEnvConfig()` resulting in fatal error.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Koolvin
Differential Revision: https://secure.phabricator.com/D2376
Summary:
We will need it for two purposes:
- Status tool.
- Nagging tool - @aran suggested using "3 business days" and I don't want it to fall on New Year's Eve or such.
I don't plan working on any interface for editing this as this kind of data should be always imported.
Test Plan:
`bin/storage upgrade`
`scripts/calendar/import_us_holidays.php`
/calendar/
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Koolvin
Differential Revision: https://secure.phabricator.com/D2375
Summary: This allows writing inline comments and reduces different behavior between normal and very large diffs.
Test Plan:
Verify that normal diff works.
Verify that very large diff works.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Koolvin
Differential Revision: https://secure.phabricator.com/D2361
Summary:
This adds support to differential fields to display warnings before a revision
gets accepted. Since lint and unit are differential fields, the code for their
warnings was moved into their respective field specification classes, so there
is only one code path for warnings (lint, unit, or custom).
Test Plan:
Select 'Accept' on a revision with lint/unit warnings and see messages appear
like they used to. Change it back to 'Comment' and they go away. Repeat with
a revision without lint/unit warnings and see no warnings appear. Checked
darkconsole to see no errors due to this.
Reviewers: jungejason, epriestley, vrana
Reviewed By: epriestley
CC: aran, Koolvin
Differential Revision: https://secure.phabricator.com/D2363
Summary: NOTE: `renderViewOptionsDropdown()` adds unnecessary parameters to URL but the link just redirects anyway.
Test Plan:
Show Raw File (Left and Right) in SVN and Git.
Verify also Added and Deleted files.
Reviewers: epriestley, aran
Reviewed By: epriestley
CC: Koolvin
Differential Revision: https://secure.phabricator.com/D2370
Summary:
- This is only slightly useful for updating Differential, since DiffQuery (vs RawDiffQuery) already gets you most of what you need. The only thing is that DiffQuery returns the diff for one path only right now(and the SVN version is very "special"). Should be easy to fix in the Git/HG cases at least, though (or maybe just use RawDiffQuery to avoid the SVN mess).
- Added a "download raw diff" link.
Test Plan: Viewed Diffusion and raw commits for SVN, Mercurial and Git repositories.
Reviewers: vrana, btrahan, jungejason
Reviewed By: vrana
CC: aran
Differential Revision: https://secure.phabricator.com/D2350
Summary:
- Unit tests can request storage fixtures.
- We build one fixture across all tests in the process, which can quickstart (takes roughly 1s to build, 200ms to destroy for me). This is a one-time cost for running an arbitrary number of fixture-based tests.
- We isolate all the connections inside transactions for each test, so individual tests don't affect one another.
Test Plan: Ran unit tests, which cover the important properties of fixtures.
Reviewers: btrahan, vrana, jungejason, edward
Reviewed By: btrahan
CC: aran, davidreuss
Maniphest Tasks: T140
Differential Revision: https://secure.phabricator.com/D2345
Summary:
When choosing a verb to show with a closed differential revision, choose the
verb based on the upstream vcs, not the vcs used to create the diff, since these
are not the same thing. I also updated the documentation for the next step for
an accepted diff for the case where the local vcs and backing vcs aren't the
same (since arc land doesn't work for those).
Test Plan:
Loaded a committed diff and an accepted diff from fbcode and www to check that
they show the correct thing.
Reviewers: jungejason, epriestley
Reviewed By: epriestley
CC: aran, Koolvin
Maniphest Tasks: T1118
Differential Revision: https://secure.phabricator.com/D2360
Summary: Only inlines were indexed (contrary to what comment claims).
Test Plan: Index one revision, check database.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Koolvin
Differential Revision: https://secure.phabricator.com/D2359
Summary:
I think this improves things, let me know if you have feedback.
Also addresses T840.
Test Plan: See screenshots...
Reviewers: vrana, btrahan, jungejason
Reviewed By: vrana
CC: aran, zeeg
Maniphest Tasks: T840
Differential Revision: https://secure.phabricator.com/D2357
Summary: basically by validating we have good user data when we set the user data.
Test Plan: simulated a failure from a phabricator on phabricator oauth scenario. viewed ui that correctly told me there was an error with the provider and to try again.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Koolvin
Maniphest Tasks: T1077
Differential Revision: https://secure.phabricator.com/D2337
Summary: 'cuz it looks dumb to use a URI slug
Test Plan: viewed a post liked the title
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Koolvin
Differential Revision: https://secure.phabricator.com/D2338
Summary:
Before: {F10754}
After: {F10753}
Test Plan:
View revision with lint warnings and unit errors.
Click on Details.
Click on Details.
Click on Details.
Click on Details.
Reviewers: asukhachev, epriestley
Reviewed By: epriestley
CC: aran, Koolvin
Differential Revision: https://secure.phabricator.com/D2351
Summary:
- Currently, connections are responsible for connection caching. However, I want unit tests to be able to say "throw away the entire connection cache" with storage fixtures, and this is difficult/impossible when connections are responsible for the cache.
- The only behavioral change is that previously we would use the same connection for read-mode and write-mode queries. We'll now establish two connections. No installs actually differentiate between the modes so it isn't particularly relevant what we do here. In the long term, we should probably check the "w" cache before building a new "r" connection, so transactional code which involves reads and writes works (we don't have any such code right now).
Test Plan: Loaded pages, verified only one connection was established per database. Ran unit tests.
Reviewers: btrahan, vrana, jungejason, edward
Reviewed By: vrana
CC: aran
Maniphest Tasks: T140
Differential Revision: https://secure.phabricator.com/D2342
Summary: Allow the default namespace to be set in configuration, so you can juggle multiple copies of sandbox test data or whatever.
Test Plan: Changed default namespace, verified web UI and "storage" script respect it.
Reviewers: btrahan, vrana, jungejason
Reviewed By: vrana
CC: aran
Maniphest Tasks: T345
Differential Revision: https://secure.phabricator.com/D2341
Summary:
Also reduce the memory usage a little bit (before increasing it again).
I use the same CSS class as for the copied code.
Test Plan: Parsed 100 diffs and checked about 10 of them - looks good.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Koolvin
Differential Revision: https://secure.phabricator.com/D2339
Summary:
This addresses three issues with the current patch management system:
# Two people developing at the same time often pick the same SQL patch number, and then have to go rename it. The system catches this, but it's silly.
# Second/third-party developers can't use the same system to manage auxiliary storage they may want to add.
# There's no way to build mock databases for unit tests that need to do reads.
To resolve these things, you can now name your patches whatever you want and conflicts are just merge conflicts, which are less of a pain to fix than filename conflicts.
Dependencies are now a DAG, with implicit dependencies created on the prior patch if no dependencies are specified. Developers can add new concrete subclasses of `PhabricatorSQLPatchList` to add storage management, and define the dependency branchpoint of their patches so they apply in the correct order (although, generally, they should not depend on the mainline patches, presumably).
The commands `storage upgrade --namespace test1234` and `storage destroy --namespace test1234` will allow unit tests to build and destroy MySQL storage.
A "quickstart" mode allows an upgrade from scratch in ~1200ms. Destruction takes about 200ms. These seem like fairily reasonable costs to actually use in tests. Building from scratch patch-by-patch takes about 6000ms.
Test Plan:
- Created new databases from scratch with and without quickstart in a separate test namespace. Pointed the webapp at the test namespaces, browsed around, everything looked good.
- Compared quickstart and no-quickstart dump states, they're identical except for mysqldump timestamps and a few similar things.
- Upgraded a legacy database to the new storage format.
- Destroyed / dumped storage.
Reviewers: edward, vrana, btrahan, jungejason
Reviewed By: btrahan
CC: aran, nh
Maniphest Tasks: T140, T345
Differential Revision: https://secure.phabricator.com/D2323
Summary:
- Show README on the repository screen.
- Move README to the bottom of the page for both repository and browse screens.
- Support "README.rainbow".
Test Plan: Looked at repository, browse screens. Made a "README.rainbow".
Reviewers: btrahan, vrana, jungejason
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T1104
Differential Revision: https://secure.phabricator.com/D2336
Summary: The color used for this feature is pretty important and I am bad with colors.
Test Plan:
View diff created by D2320 with some copied lines and one line changed:
{F10604, size=full}
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Koolvin
Differential Revision: https://secure.phabricator.com/D2321
Summary:
Required for D2321.
Deprecates D2320.
Uses algorithm described at D2320#16.
Complexity of this algorithm would be `O(N)` (`N` stands for number of lines) in most cases.
The worst case is `O(A*F)` (`A` stands for number of added lines, `F` for number of colliding lines) but it should be pretty rare. Real-world example is 100 modified files with moved license block (15 lines) in each. This will require 1500*100 comparisons because the algorithm will be trying to find the longest block in each file.
Test Plan:
`arc diff --only` on commit with copied code.
More tests on standalone algorithm.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Koolvin
Differential Revision: https://secure.phabricator.com/D2333
Summary:
Many times when I'm reading a big diff, I want to go to the
TOC. Add it.
Test Plan:
can navigate with 't'. It also shows up in '?'
Revert Plan:
Reviewers: epriestley, vrana
Reviewed By: epriestley
CC: nh, aran, Koolvin
Differential Revision: https://secure.phabricator.com/D2335
Summary:
We added the ability to delete files a while ago, but this interface isn't happy about it.
I still render the macro so you can see/delete it, e.g.
Test Plan: Viewed a deleted macro page, got a page instead of an error. Also verified that the actual remarkup part doesn't have issues.
Reviewers: btrahan, vrana, jungejason
Reviewed By: jungejason
CC: aran
Differential Revision: https://secure.phabricator.com/D2328
Summary:
This is mostly intended to simplify D2323.
We currently allow users to edit and customize the links on the homepage, but as far as I know no one actually does this (no one complained when we redid the homepage earlier this year) and it creates a lot of mess in the database patches and quickstart dump. After D2331, this is the only data we load in the patch files. The patch files are also a mess with respect to this data and have various different versions of it.
Also the current UI is just kind of bad, it stretches stuff across too many screens and is generally ungood. Nuking this lets us nuke a lot of code in general.
(In the long term, I think we'll move toward an "application" model anyway, and this stuff will go away sooner or later.)
I'll add a drop-database patch some time later, just in case anyone does actually use this, so they can get their data out of MySQL.
Test Plan: Looked at home page, clicked "More Stuff", got a single list of other apps/things.
Reviewers: btrahan, vrana, edward, jungejason
Reviewed By: jungejason
CC: aran
Maniphest Tasks: T345
Differential Revision: https://secure.phabricator.com/D2332
Summary:
This is mostly in an effort to simplify D2323. Currently, we load one image into the database by default. This is a weird special case that makes things more complicated than necessary.
Instead, use a disk-based default avatar.
Test Plan: Verified that a user without an image appears with the default avatar as a handle, in profile settings, and on their person page.
Reviewers: btrahan, vrana, edward, jungejason
Reviewed By: vrana
CC: aran
Maniphest Tasks: T345
Differential Revision: https://secure.phabricator.com/D2331
Summary:
Also couple of small changes:
- Add method name to title.
- 404 for /conduit/method/x/.
- Remove utilities from side panel.
- Remove side panel from log.
Test Plan:
/conduit/
/conduit/method/x/
/conduit/method/user.whoami/
/conduit/log/
Reviewers: btrahan, epriestley
Reviewed By: epriestley
CC: aran, Koolvin
Differential Revision: https://secure.phabricator.com/D2326
Summary: We now allow symbolic commits, so let them through the pipeline.
Test Plan: {F10571}
Reviewers: davidreuss, btrahan, vrana
Reviewed By: davidreuss
CC: aran
Maniphest Tasks: T1130
Differential Revision: https://secure.phabricator.com/D2315
Summary:
This is somewhat controversial but push date is usually more useful than commit date (which can be for example a month before other people can see the commit).
We can also store both dates.
Test Plan:
git log --pretty="%ct %at"
Reviewers: epriestley
Reviewed By: epriestley
CC: nh, aran, Koolvin
Differential Revision: https://secure.phabricator.com/D2319
Summary: Inspired by D2242.
Test Plan:
Select text in left pane.
Select text in right pane.
Select all.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Koolvin
Differential Revision: https://secure.phabricator.com/D2249
Summary: I thought that this will be fun but the elasticsearch API is horrible and the documentation is poor.
Test Plan:
Search for:
- string
- author
- author, owner
- string, author
- open
- string, open, author
- string, exclude
- several authors, several owners
- nothing
- probably all other combinations
Normally, such an exhaustive test plan wouldn't be required but each combination requires a completely different query.
Reviewers: epriestley, jungejason
Reviewed By: epriestley
CC: aran, Koolvin, btrahan
Differential Revision: https://secure.phabricator.com/D2298
Summary:
- When viewing a commit, show its tags.
- For commits with many tags, show a list of all tags on the tag list interface.
- Improve some handling of symbolic references.
- When tags contain content, show it on the browse view reached by clicking the tag name.
Test Plan: Looked at commits with and without tags, clicked "More tags...", clicked tag names.
Reviewers: btrahan, vrana, davidreuss, jungejason
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T1130
Differential Revision: https://secure.phabricator.com/D2290
Summary:
- Track + message through file moves.
- Stop + message on file create.
- Stop + message on first commit.
Test Plan:
- Tested blaming through a move, through a create, and through the first commit.
- Verified this doesn't break anything in SVN / Mercurial.
Reviewers: vrana, btrahan, jungejason
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T1091
Differential Revision: https://secure.phabricator.com/D2295
Summary: "Committed" is SVN-specific language, and confusing in Git and Mercurial. Use neutral language instead.
Test Plan: Inspection.
Reviewers: btrahan, Makinde, vrana, jungejason
Reviewed By: vrana
CC: aran
Maniphest Tasks: T909
Differential Revision: https://secure.phabricator.com/D2087
Test Plan:
Added CC's/Auditors, clicked the form elements, and saw correct
behaviour. Verified that metadata was present in the detail table.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, epriestley, 20after4, Koolvin
Maniphest Tasks: T904
Differential Revision: https://secure.phabricator.com/D2002
Summary: we were using the "path" as the next_uri and that drops some delicious get parameters
Test Plan: see T1140; basically re-ran the steps listed there and they passed!
Reviewers: epriestley, njhartwell
Reviewed By: epriestley
CC: aran, Koolvin
Maniphest Tasks: T1140, T1009
Differential Revision: https://secure.phabricator.com/D2299
Summary:
I have no idea what I'm doing, but here's part of an elasticsearch engine. These things work:
- Indexing stuff (??)
- Searching for text/type?
- Reconstructing things??
All the complicated stuff doesn't work. I'm having a hard time figuring out the best way to model things because elasticsearch's documentation is not exactly the most complete or illuminating.
@amckinley, does this look sane-ish so far? Particularly, the /phabricator/<type>/<phid>/ URI scheme and how I've set up the relationships and fields in the documents?
How should I model the relationship and field queries? I want, like, an "equal" query but it seems like I've got "text" or "term" to work with and neither are exact match? And "term" doesn't consider PHIDs to be terms since they have hyphens in them?
I'll keep kind of slogging my way forward here but if you have valuable wisdom to share it would probably get me to a better end state much faster. The whole query construction phase is pretty much black magic to me.
Test Plan: nyancat
Reviewers: amckinley, vrana
Reviewed By: vrana
CC: jungejason, tuomaspelkonen, aran, 20after4, vrana
Differential Revision: https://secure.phabricator.com/D790
Summary:
We will need it for intl.
I've put it to User instead of UserProfile to be easier accessible.
Test Plan:
Apply SQL patch.
Change sex to Male.
Change sex to Unknown.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Koolvin
Maniphest Tasks: T1139
Differential Revision: https://secure.phabricator.com/D2287
Summary:
This is slightly more complicated for this reason:
- We don't set `dateCommitted` for normal commits, only for markcommitted.
-- We need to add this date to old revisions now.
Test Plan:
Reparse a revision - commit date was set.
Conduit `markcommitted` - commit date was set.
Run SQL script.
Display closed revision.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Koolvin
Differential Revision: https://secure.phabricator.com/D2282
Summary:
PHP has this crazy [[ http://php.net/arg_separator.output | arg_separator.output ]] INI setting which allows setting different string for URL parameters separator instead of `&` (e.g. in `?a=1&b=2`).
Don't use it for external URLs.
Test Plan: Log in through OAuth.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Koolvin
Differential Revision: https://secure.phabricator.com/D2284
Test Plan:
Ctrl+click on Show Diff in Chrome - button is not grayed-out, new tab is opened.
Click on it - button is grayed out.
Repeat in Firefox.
Reviewers: epriestley, tuomaspelkonen
Reviewed By: epriestley
CC: aran, Koolvin
Maniphest Tasks: T1137
Differential Revision: https://secure.phabricator.com/D2278
Summary:
These are explicit copies of implicitly-generated Lisk methods.
See brief discussion in rPdec8bac3a3af6065166d485db80fffa70dc2abe3.
Test Plan: Looked at a diff in Differential.
Reviewers: vrana, btrahan
Reviewed By: vrana
CC: aran
Differential Revision: https://secure.phabricator.com/D2283
Summary:
This is better than writing "(UNSTABLE!!!)" in front of the text description.
I'll add a wiki to keep track of API changes, too.
See also D2087, which motivates this.
Test Plan: Browsed console, saw "deprecated" and "unstable" on appropriate methods.
Reviewers: btrahan, vrana, jungejason
Reviewed By: vrana
CC: aran
Maniphest Tasks: T909
Differential Revision: https://secure.phabricator.com/D2271
Test Plan:
Click on "passing a null index to idx()" in DarkConsole.
Click on entry in stack trace.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Koolvin
Differential Revision: https://secure.phabricator.com/D2275
Summary:
Ctrl+click opens the link to a new tab in most browsers.
Shift+click to a new window.
Alt+click or Meta+click downloads the target.
This diff respects these conventions by disabling JX.Workflow for these modifiers.
Test Plan:
Click Flag Task - inline dialog.
Ctrl+click Flag Task - new tab with standalone dialog.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Koolvin
Differential Revision: https://secure.phabricator.com/D2276
Summary:
move up the panels which generally has short length.
Test Plan:
view the page.
Reviewers: blair, vrana
Reviewed By: vrana
CC: epriestley, aran
Differential Revision: https://secure.phabricator.com/D2228
Summary: This is not very nice.
Test Plan: /P1
Reviewers: codeblock
Reviewed By: codeblock
CC: aran, epriestley
Differential Revision: https://secure.phabricator.com/D2267
Summary:
We are marking disabled buttons with 'disabled' class in `behavior-form`.
But we ignore `JX.Workflow` there because it has its own handling.
But this handling doesn't set class so the button is disabled but it is not indicated to user.
It causes troubles in Clowncopterize where users report that browser freezes before doing anything after clicking it. It probably happens also on other places.
This diff solves it by using CSS3 selector on attribute (contrary to explicitly setting class in JX.Workflow).
Test Plan:
Add `sleep(3)` to `DifferentialCommentSaveController`.
Clowncopterize empty comment.
Reviewers: epriestley, tuomaspelkonen
Reviewed By: epriestley
CC: aran, Koolvin
Differential Revision: https://secure.phabricator.com/D2264
Summary:
Please review carefully, me not very well on English.
I am just guessing what were the design decisions in most parts of this document.
Feel free to correct me or add more information.
Test Plan:
`diviner .`
/docs/
/docs/article/Database_Schema.html
Copy the text to Word and proofread.
Reviewers: epriestley, btrahan
Reviewed By: epriestley
CC: nh, jungejason, aran, Koolvin
Differential Revision: https://secure.phabricator.com/D2258
Summary: NOTE: This is starting to be too hacky.
Test Plan:
View revision with inline diffs, verify that Reply is there.
View standalone - no Reply.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Koolvin
Differential Revision: https://secure.phabricator.com/D2263
Summary: Lists the 25 most recent tags on the "Repository" page.
Test Plan: Looked at a git repository with a tag, saw it. Looked at HG/SVN repos, they didn't break.
Reviewers: davidreuss, 20after4, btrahan, vrana, jungejason
Reviewed By: davidreuss
CC: aran
Maniphest Tasks: T1130
Differential Revision: https://secure.phabricator.com/D2255
Summary: Also link to `D1?id=` instead of `?id=` because some IE versions linked to root in this case.
Test Plan: Click on old diff's inline comment link on large revision.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Koolvin
Differential Revision: https://secure.phabricator.com/D2260
Summary:
- Adds "Commandeer Revision", to allow you to plunder revisions from those lost to sea (e.g., interns who have left or co-workers who are dealing with a family emergency).
- Removes admin-abandon to simplify things, since you can just Commandeer + Abandon now.
- There are other workarounds available but this is the natural/expected workflow (and the one everyone always asks for) and there's no real reason not to allow it.
Test Plan: Swashbuckled.
Reviewers: cpiro, btrahan, vrana, jungejason
Reviewed By: cpiro
CC: aran, zeeg
Differential Revision: https://secure.phabricator.com/D2257
Summary:
If I have Pygments enabled in config but `pygmentize` doesn't work then unhighlighted source is stored to cache.
If I later make `pygmentize` work then the unhighlighted source is still loaded from the cache.
Test Plan:
Break `pygmentize`.
View a diff with JS files.
Fix `pygmenize`.
View the diff again.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, gatos99, Koolvin
Differential Revision: https://secure.phabricator.com/D2227
Summary:
Couple of small improvements:
- Delete `randomon` macro.
- Make name unique (deleting current conflicts randomly).
- Image macro must be alone on the line.
- Filter by name.
Test Plan:
Run SQL.
/file/macro/
/file/macro/?name=imagemacro
Try to create conflicting name.
Write this comment:
Test imagemacro.
imagemacro
Reviewers: aran, epriestley
Reviewed By: epriestley
CC: epriestley, Koolvin
Differential Revision: https://secure.phabricator.com/D2230
Summary:
This event is fired after a task is created and assigned with an id.
Use case is sending an email notification to everyone in a project when a new task is
submitted to said project.
Test Plan:
Implement the event listener, submit a new task to a project, see if the project members
receive an email notification. I will submit the event handler in a separate diff once it's a bit
prettier and tested more thoroughly.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, jungejason
Differential Revision: https://secure.phabricator.com/D2159
Summary:
- Add an "Administrators" policy.
- Allow "Public" to be completely disabled in configuration.
- Simplify unit tests, and cover the new policies.
Test Plan: Ran unit tests.
Reviewers: btrahan, vrana, jungejason
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T603
Differential Revision: https://secure.phabricator.com/D2238
Summary: Introduces a scope-guarded way to override the env config, for unit tests which are sensitive to config values.
Test Plan: Ran unit tests.
Reviewers: btrahan, vrana, jungejason
Reviewed By: btrahan
CC: aran
Differential Revision: https://secure.phabricator.com/D2237
Summary: This appears to sometimes be effective (for MS clients), and we've seen it in the wild on inbound mail.
Test Plan: Sent myself some mail, verified it had the right header.
Reviewers: btrahan, vrana, jungejason
Reviewed By: vrana
CC: aran
Maniphest Tasks: T571
Differential Revision: https://secure.phabricator.com/D2241
Summary:
- For line numbers, use "user-select: none" to make them unselectable. This provides a stronger visual cue that copy/paste is enchanted.
- In Paste, make it look sensible again after the blame-on-blame refactor in Diffusion. See also TODO to share this code formally.
- In Diffusion, use the "phabricator-oncopy" behavior.
NOTE: I left blame/commit columns selectable in Diffusion, since you might reasonably want to copy/paste them?
NOTE: In Differential, the left side of the diff still highlights, even though it will be copied only if you select part of a line on the left and nothing else. But this seemed like a reasonable behavior, so I left it.
Test Plan:
- Looked at Paste. Saw a nice line number column. Selected text, got the expected selection. Copied text, got the expected copy.
- Looked at Diffusion. Saw a nice line number column, still. Selected text, got expected selection. Copied text, got expected copy.
- Looked at Differential. Highlighted stuff, got expected results. Copied stuff, got expected results.
Reviewers: btrahan, vrana, jungejason
Reviewed By: vrana
CC: aran
Maniphest Tasks: T1123
Differential Revision: https://secure.phabricator.com/D2242
Test Plan:
Comment `JX.DOM.remove(pre)` to better see the problem in old code.
Apply this diff and verify that the pre is not shown.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran
Differential Revision: https://secure.phabricator.com/D2245
Summary: See rP23f25edd97f052ff4c1c5d8c4be962b4da149bca.
Test Plan: RAN LINT AND UNIT TESTS. VERIFIED THERE ARE NO SYNTAX ERRORS.
Reviewers: vrana
Reviewed By: vrana
CC: aran
Differential Revision: https://secure.phabricator.com/D2240
Summary:
With invalid session (which happens for me when I change production and dev db but can of course happen in other cases), Phabricator displays an ugly unhandled exception dialog suggesting to logging in again.
But there's no login dialog on that page.
This also changes how users with invalid session are treated on pages not requiring logging.
Previously, an exception was thrown on them. Now they are treated as unlogged users.
Test Plan: Corrupt session, go to /, login.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran
Differential Revision: https://secure.phabricator.com/D2236
Summary:
This continues work started at D2215.
Files moved from deleted directory were marked as Copied Here instead of Moved Here.
Test Plan: Reparsed two commits which was previously wrong, now correct.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran
Maniphest Tasks: T1114
Differential Revision: https://secure.phabricator.com/D2229
Summary:
Provides a basic start for access policies. Objects expose various capabilities, like CAN_VIEW, CAN_EDIT, etc., and set a policy for each capability. We currently implement three policies, PUBLIC (anyone, including logged-out), USERS (any logged-in) and NOONE (nobody). There's also a way to provide automatic capability grants (e.g., the owner of an object can always see it, even if some capability is set to "NOONE"), but I'm not sure how great the implementation feels and it might change.
Most of the code here is providing a primitive for efficient policy-aware list queries. The problem with doing queries naively is that you have to do crazy amounts of filtering, e.g. to show the user page 6, you need to filter at least 600 objects (and likely more) before you can figure out which ones are 500-600 for them. You can't just do "LIMIT 500, 100" because that might have only 50 results, or no results. Instead, the query looks like "WHERE id > last_visible_id", and then we fetch additional pages as necessary to satisfy the request.
The general idea is that we move all data access to Query classes and have them do object filtering. The ID paging primitive allows efficient paging in most cases, and the executeOne() method provides a concise way to do policy checks for edit/view screens.
We'll probably end up with mostly broader policy UIs or configuration-based policies, but there are at least a few cases for per-object privacy (e.g., marking tasks as "Security", and restricting things to the members of projects) so I figured we'd start with a flexible primitive and the simplify it in the UI where we can.
Test Plan: Unit tests, played around in the UI with various policy settings.
Reviewers: btrahan, vrana, jungejason
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T603
Differential Revision: https://secure.phabricator.com/D2210
Summary:
...pretty sure the JS is too hack-tastic but it works...! :D
also fixed a small error from assert_instances_of change where a null value is all errors and what have you
Test Plan: played around with tasks in firefox and safari. made cc, owner, and project changes, as well as priority, etc.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran
Maniphest Tasks: T1074
Differential Revision: https://secure.phabricator.com/D2234
Summary: use arc layout, include about the __tests__ folder, upsell unit testing
Test Plan: read the docs!
Reviewers: epriestley
Reviewed By: epriestley
CC: aran
Differential Revision: https://secure.phabricator.com/D2235
Summary: tried to cover the basics and sprinkle in lots of class references, etc. would really appreciate feedback...! :D
Test Plan: read the docs!
Reviewers: epriestley, vrana
Reviewed By: vrana
CC: aran
Maniphest Tasks: T359
Differential Revision: https://secure.phabricator.com/D2223
Summary:
missed doing this for phame, so i poked around a bit and added it to
similar verbage as well as to a few "Next Steps" where I thought the feedback
might be provocative.
Test Plan: read the docs
Reviewers: vrana, epriestley
Reviewed By: epriestley
CC: aran
Differential Revision: https://secure.phabricator.com/D2220
Summary: This is not perfect. Moved files are reported as deleted but I'm happy with it.
Test Plan: Reparsed two commits which was previously wrong, now semi-correct.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran
Maniphest Tasks: T1114
Differential Revision: https://secure.phabricator.com/D2215
Summary:
'cuz we need to be phamous!
V1 feature set
- posts
-- standard thing you'd expect - a title and a remarkup-powered body and...
-- "phame" title - a short string that can be used to reference the story. this gets auto-updated when you mess with the title.
-- configuration - for now, do you want Facebook, Disqus or no comments? this is a per-post thing but feeds from an instance-wide configuration
Please do toss out any must have features or changes.
Test Plan: played around with this bad boy like whoa
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, vrana
Maniphest Tasks: T1111
Differential Revision: https://secure.phabricator.com/D2202
Summary:
- Add an explicit multiplexing option, and enable it by default. This is necessary for Mail.app to coexist with other clients ("Re:" breaks outlook at the very least, and generally sucks in the common case), and allows users with flexible clients to enable subject variance.
- Add an option for subject line variance. Default to not varying the subject, so mail no longer says [Committed], [Closed], etc. This is so the defaults thread correctly in Gmail (not entirely sure this actually works).
- Add a preference to enable subject line variance.
- Unless all mail is multiplexed, don't enable or respect the "Re" or "vary subject" preferences. These are currently shown and respected in non-multiplex cases, which creates inconsistent results.
NOTE: @jungejason @nh @vrana This changes the default behavior (from non-multiplexing to multiplexing), and might break Facebook's integration. You should be able to keep the same behavior by setting the options appropriately, although if you can get the new defaults working they're probably better.
Test Plan:
Send mail from Maniphest, Differential and Audit. Updated preferences. Enabled/disabled multiplexing. Things seem OK?
NOTE: I haven't actually been able to repro the Gmail threading issue so I'm not totally sure what's going on there, maybe it started respecting "Re:" (or always has), but @cpiro and @20after4 both reported it independently. This fixes a bunch of bugs in any case and gives us more conservative set of defaults.
I'll see if I can buff out the Gmail story a bit but every client is basically a giant black box of mystery. :/
Reviewers: btrahan, vrana, jungejason, nh
Reviewed By: btrahan
CC: cpiro, 20after4, aran
Maniphest Tasks: T1097, T847
Differential Revision: https://secure.phabricator.com/D2206
Summary: Sometimes we get a lowercase "Meddelelse" in Danish outlook. Relax the patterns since the risk of hitting false positives here is essentially nonexistant.
Test Plan: Unit tests.
Reviewers: davidreuss, btrahan, vrana
Reviewed By: vrana
CC: aran
Differential Revision: https://secure.phabricator.com/D2205
Summary:
Add <optgroup> style selects, if the array of options is actually an
array-of-arrays.
Test Plan: Made one, it looked OK.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran
Differential Revision: https://secure.phabricator.com/D2177
Summary:
Owners field is filled by Primary Owner which is required.
So that it is not neccessary to require filling Owners explicitly.
Test Plan: Don't fill Owners and successfully save the form //before// this change.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran
Differential Revision: https://secure.phabricator.com/D2201
Summary:
Some Differential fields are not nullable; when Test Plan is switched to non-required mode we can end up trying to save a null value to a non-nullable column (see D2193).
(I should probably just alter the schema to make these fields nullable, but that might have farther-reaching effects.)
Test Plan: Reproduced error, applied patch, no more error.
Reviewers: btrahan, vrana, jungejason
Reviewed By: btrahan
CC: aran
Differential Revision: https://secure.phabricator.com/D2200
Summary: Partially broken by D2166.
Test Plan:
Hover line number in revision.
Hover line number in standalone view.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran
Differential Revision: https://secure.phabricator.com/D2196
Summary:
This is a somewhat common request, and far more difficult than necessary currently.
I think the field is useful enough to leave it default-enabled, but there's wide diversity in testing philosophy.
Test Plan: Verified "test plan" field appeared. Disabled config. Verified "test plan" field vanished.
Reviewers: btrahan, vrana, jungejason
Reviewed By: vrana
CC: aran, asouza
Differential Revision: https://secure.phabricator.com/D2193
Summary: We'll get a typehint warning on the repository if there's no repository. Check outside the method instead.
Test Plan: Loaded page, no warning.
Reviewers: btrahan, vrana, jungejason
Reviewed By: vrana
CC: aran
Differential Revision: https://secure.phabricator.com/D2194
Summary:
- We currently post-filter by branches, but should do this in SQL. See T799.
- We currently identify branch-name-matches as being in the working copy even if they belong to a different project (e.g., two different projects with commits on the branch "master"). See T1100.
- Denormalize branch and project information into DifferentialRevision.
- Expose project information in the API.
Test Plan: Ran conduit API queries with branches and arc project IDs, got reasonable results.
Reviewers: btrahan, vrana, jungejason
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T1100, T799
Differential Revision: https://secure.phabricator.com/D2190
Summary:
Otherwise useless query is executed:
lang=sql
SELECT c.*
FROM `repository_commit` c
ORDER BY c.epoch DESC
Test Plan: /diffusion/X/browse/x
Reviewers: epriestley
Reviewed By: epriestley
CC: aran
Differential Revision: https://secure.phabricator.com/D2186
Test Plan:
/rX1
Browse in Diffusion
Open in Editor
Reviewers: epriestley
Reviewed By: epriestley
CC: aran
Differential Revision: https://secure.phabricator.com/D2180
Summary:
There have been a couple of requests for this since bookmarks are "out this year like woah" and "totally uncool dude".
Allow users to save named custom queries and make them the /maniphest/ default if they so desire.
A little messy. :/
Test Plan: Saved, edited, deleted custom queries. Made custom query default; made 'no default' default. Verified default behavior. Issued a modified search from a custom query.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran, epriestley, 20after4
Maniphest Tasks: T923, T1034
Differential Revision: https://secure.phabricator.com/D1964
Summary:
When Phabricator is configured to generate patch email, we'll fatal if the patch contains binaries and is generating to Git because ArcanistBundle can't load the binary data. Provide a callback to load the data. See D2174.
(This may cause us to generate absolutely enormous emails, but you get what you asked for...)
Test Plan: Created a diff with an image under "send git patches" email configuration.
Reviewers: Makinde, btrahan, vrana, jungejason
Reviewed By: Makinde
CC: aran
Differential Revision: https://secure.phabricator.com/D2175
Summary:
PHP arrays have an internal "current position" marker. (I think because foreach() wasn't introduced until PHP 4 and there was no way to get rid of it by then?)
A few functions affect the position of the marker, like reset(), end(), each(), next(), and prev(). A few functions read the position of the marker, like each(), next(), prev(), current() and key().
For the most part, no one uses any of this because foreach() is vastly easier and more natural. However, we sometimes want to select the first or last key from an array. Since key() returns the key //at the current position//, and you can't guarantee that no one will introduce some next() calls somewhere, the right way to do this is reset() + key(). This is cumbesome, so we introduced head_key() and last_key() (like head() and last()) in D2161.
Switch all the reset()/end() + key() (or omitted reset() since I was feeling like taking risks + key()) calls to head_key() or last_key().
Test Plan: Verified most of these by visiting the affected pages.
Reviewers: btrahan, vrana, jungejason, Koolvin
Reviewed By: jungejason
CC: aran
Differential Revision: https://secure.phabricator.com/D2169
Summary:
There was a typo:
`PHID-!!!!-NO_PROJECT` instead of
`PHID-!!!!-NO-PROJECT`
Also use `<em>` to differentiate from project named "(No Project)".
Test Plan:
/maniphest/report/project/
Click on (No Project).
Reviewers: epriestley
Reviewed By: epriestley
CC: aran
Differential Revision: https://secure.phabricator.com/D2167
Test Plan: I didn't repro it probably because of custom rules.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran
Maniphest Tasks: T1090
Differential Revision: https://secure.phabricator.com/D2150
Summary: See T1021. Raise configuration or implementation exceptions immediately. When all engines fail, raise an aggregate exception with details.
Test Plan: Forced all engines to fail, received an aggregate exception. Forced an engine to fail with a config exception, recevied it immediately.
Reviewers: btrahan, vrana, jungejason
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T1021
Differential Revision: https://secure.phabricator.com/D2157
Summary: In Safari, Firefox and Chrome, respect cursor position and selection ranges.
Test Plan: Dragged-and-dropped files into the middle of text, end of text, and a selected text range in Safari, Firefox and Chrome. Copy/pasted files into similar cases in Chrome. Got expected, normal behavior in all cases.
Reviewers: btrahan, vrana, jungejason
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T1016
Differential Revision: https://secure.phabricator.com/D2155
Summary: See discussion in T789. Covered the obvious cases, at least. We can refine this as we get a larger sample size.
Test Plan: Unit test coverage.
Reviewers: btrahan, vrana, jungejason
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T789
Differential Revision: https://secure.phabricator.com/D2154
Summary: See T993. MySQL fails very very softly if you request an engine which does not exist. Detect and fail if the InnoDB engine is missing or broken.
Test Plan: Faked InnoDB missing, got a failure. Ran normally, got success.
Reviewers: btrahan, vrana, jungejason
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T993
Differential Revision: https://secure.phabricator.com/D2160
Summary:
We don't use versioned URIs for images, so when they change users may get old versions.
This was a particular issue with the recent logo change, which several users reported cache-related issues from.
Instead, use Celerity to manage image URI versions in addition to CSS/JS.
This is complicated, because we need to rewrite image URIs inside of CSS, which means the hash of a CSS file has to be derived from the current image data. Otherwise, when we updated an image the CSS wouldn't update, so we wouldn't be any better off.
So basically we:
- Find all the "raw" files, and put them into the map.
- Find all the CSS/JS, perform content-altering transformations on it (i.e., not minification) based on the partial map, and then put it into the map based on transformed hashes.
(If we wanted, we could now do CSS variables or whatever for "free", more or less.)
Test Plan:
- Regenerated celerity map, browsed site, verified images generated with versioned URIs.
- Moved "blue" flag image over "green" flag image, regenerated map, verified "green" flag image and the associated CSS changed hashes.
- Added transformation unit tests; ran unit tests.
Reviewers: btrahan, vrana, jungejason
Reviewed By: vrana
CC: aran
Maniphest Tasks: T1073
Differential Revision: https://secure.phabricator.com/D2146
Summary: These elements look heavy and out of place right now.
Test Plan: Looked at error views in uiexample page.
Reviewers: btrahan, vrana, jungejason
Reviewed By: jungejason
CC: aran
Differential Revision: https://secure.phabricator.com/D2144
Summary: Currently, we show them everything. Instead, show them an explicit notice.
Test Plan: Looked at "My Projects" feed with no projects.
Reviewers: btrahan, vrana, jungejason
Reviewed By: jungejason
CC: aran
Maniphest Tasks: T1015
Differential Revision: https://secure.phabricator.com/D2143
Summary:
- Make some effort to simplify the code.
- Make "Skip Past This Commit" work in Git and Mercurial.
- Make blame work in Mercurial.
- Add tooltip hover state to show more information about commits.
Test Plan: Viewed blame views in SVN, Git, Hg. Clicked line numbers, hovered/clicked commits, hovered/clicked "blame past..."
Reviewers: btrahan, vrana, jungejason
Reviewed By: vrana
CC: aran
Maniphest Tasks: T378
Differential Revision: https://secure.phabricator.com/D2142
Summary: See D2080. The introduction of `arc land`, defaulting to `origin/master`, and --auto enormously simplifies the documentation.
Test Plan: Read documentation.
Reviewers: btrahan
Reviewed By: btrahan
CC: 20after4, aran
Maniphest Tasks: T894
Differential Revision: https://secure.phabricator.com/D2082
Summary:
The older logic was incorrect:
- It chose `change.left` for `data.on_right` being true.
- 'O' and 'N' mean 'old' and 'new', not 'left' and 'right'. In diff-of-diffs, both sides are 'N'.
So, select the changeset ID correctly (pick the right side one for on_right), and select the new file prefix correctly (N for new, O for old).
Test Plan: Waved my mouse over some inline comments in a diff-of-diffs, got reasonable-looking reticles.
Reviewers: vrana, btrahan
Reviewed By: vrana
CC: aran
Maniphest Tasks: T1076
Differential Revision: https://secure.phabricator.com/D2138
I think the issue is that we don't set the left-side changesetID correctly. This seems to work correctly locally, but I'm not sure I got a good repro. Pushing to verify the production test cases provided in T1076.
Auditors: vrana, btrahan
Summary:
The audit tools has many false positive about Author Not
Matching with Revision. The fix is to set the authorPHID which was
missing in the existing code
Test Plan:
run reparse.php and it doesn't generate false positive result
anymore.
Reviewers: nh, epriestley
Reviewed By: epriestley
CC: aran
Differential Revision: https://secure.phabricator.com/D2119
Summary: renderMiniPanel() renders the entire <p>.
Test Plan: Looked at page source for homepage, verified there was no double </p>.
Reviewers: vrana, btrahan
Reviewed By: vrana
CC: aran
Maniphest Tasks: T1079
Differential Revision: https://secure.phabricator.com/D2128
Summary: This separates common MySQL stuff (identifiers and comments escaping, error codes, connection retries) from PHP extension specific stuff (connect, query, fetch, errors, escape string).
Test Plan:
/
Use `AphrontMySQLiDatabaseConnection` in `PhabricatorLiskDAO`, load homepage, edit task, save task.
Reviewers: epriestley
Reviewed By: epriestley
CC: nh, aran
Differential Revision: https://secure.phabricator.com/D2113
Summary: I've found it useful mainly on smaller screen or with lots of comments.
Test Plan: Show Diff
Reviewers: epriestley
Reviewed By: epriestley
CC: aran
Differential Revision: https://secure.phabricator.com/D2122
Summary:
This introduces some boundary checking for
PhabricatorOwnersOwner::loadAffiliatedUserPHIDs() if it gets passed an empty
array, which happened when I ran arc diff and it called
differential.createrevision.
Test Plan: ran arc diff
Reviewers: epriestley, meitros, jungejason
CC: aran
Differential Revision: https://secure.phabricator.com/D2112
Summary:
MySQL doesn't treat `\` as escaping character in ##``##.
This isn't probably SQL injection hole because I've found no calls of this method with user input.
But better safe than sorry.
See also [[http://dev.mysql.com/doc/refman/5.1/en/server-sql-mode.html#sqlmode_no_backslash_escapes | NO_BACKSLASH_ESCAPES]].
Test Plan:
lang=sql
SELECT `a\`b`; -- Throws: Syntax error near '`'.
-- Should throw: Unknown column 'a`b'.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran
Differential Revision: https://secure.phabricator.com/D2109
Summary:
I've considered that user may have set editor but not checked out Phabricator repositories.
But stack trace is useful mainly for developers.
Test Plan:
Click on path in Unhandled Exception.
Repeat with disabled editor.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran
Differential Revision: https://secure.phabricator.com/D2107
Summary: ...also make the pager usage in ChatLog use the nice formatWhereClause functionality
Test Plan: set $page_size = 2 and paged around the data a bit
Reviewers: epriestley
Reviewed By: epriestley
CC: aran
Maniphest Tasks: T905
Differential Revision: https://secure.phabricator.com/D2106
Summary: Use Edges to attach Commits and Tasks. Note, no "edit attached commits" interface from tasks yet since the search backend needs a little work to list commits in a sensible way.
Test Plan: Attached commits to tasks. Looked at commits, looked at tasks.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T904
Differential Revision: https://secure.phabricator.com/D2105
Summary:
various stripe stuff, including
- external stripe library
- payment form
- test controller to play with payment form, sample business logic
My main questions / discussion topics are...
- is the stripe PHP library too big? (ie should I write something more simple just for phabricator?)
-- if its cool, what is the best way to include the client? (ie should I make it a submodule rather than the flat copy here?)
- is the JS I wrote (too) ridiculous?
-- particularly unhappy with the error message stuff being in JS *but* it seemed the best choice given the most juicy error messages come from the stripe JS such that the overall code complexity is lowest this way.
- how should the stripe JS be included?
-- flat copy like I did here?
-- some sort of external?
-- can we just load it off stripe servers at request time? (I like that from the "if stripe is down, stripe is down" perspective)
- wasn't sure if the date control was too silly and should just be baked into the form?
-- for some reason I feel like its good to be prepared to walk away from Stripe / switch providers here, though I think this is on the wrong side of pragmatic
Test Plan: - played around with sample client form
Reviewers: epriestley
Reviewed By: epriestley
CC: aran
Differential Revision: https://secure.phabricator.com/D2096
Summary:
We have a lot of cases where we store object relationships, but it's all kind of messy and custom. Some particular problems:
- We go to great lengths to enforce order stability in Differential revisions, but the implementation is complex and inelegant.
- Some relationships are stored on-object, so we can't pull the inverses easily. For example, Maniphest shows child tasks but not parent tasks.
- I want to add more of these and don't want to continue building custom stuff.
- UIs like the "attach stuff to other stuff" UI need custom branches for each object type.
- Stuff like "allow commits to close tasks" is notrivial because of nonstandard metadata storage.
Provide an association-like "edge" framework to fix these problems. This is nearly identical to associations, with a few differences:
- I put edge metadata in a separate table and don't load it by default, to keep edge rows small and allow large metadata if necessary. The on-edge metadata seemed to get abused a lot at Facebook.
- I put a 'seq' column on the edges to ensure they have an explicit, stable ordering within a source and type.
This isn't actually used anywhere yet, but my first target is attaching commits to tasks for T904.
Test Plan: Made a mock page that used Editor and Query. Verified adding and removing edges, overwriting edges, writing and loading edge data, sequence number generation.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran, 20after4
Differential Revision: https://secure.phabricator.com/D2088
Summary: NOTE: This is not produced by a script so there might be errors. Please review carefully.
Test Plan: Browse around Differential.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran
Differential Revision: https://secure.phabricator.com/D2103
Summary: I looooove JS! It makes me giddy with glee!
Test Plan: Picked dates. See screenshots.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Differential Revision: https://secure.phabricator.com/D2086
Summary:
When system agent adds a comment then he is added to CC.
When I amend and update then I get message "Commit message references nonexistent ..."
Test Plan: Update revision with system agent in CC.
Reviewers: epriestley
Reviewed By: epriestley
CC: michalburger1, aran
Differential Revision: https://secure.phabricator.com/D2100
Test Plan:
Jump to head.
Go to doctor.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran
Differential Revision: https://secure.phabricator.com/D2097
Summary: NOTE: This is not produced by a script so there might be errors. Please review carefully.
Test Plan: Browse around Diffusion.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran
Differential Revision: https://secure.phabricator.com/D2094
Summary:
- The UI is pretty straightforward, since Handle just works (tm)
- Added two methods to the owners object to handle the new layer of
indirection. Then ran git grep PhabricatorOwnersOwner and changed
callsites as appropriate.
Sending this to get a round of feedback before I test the non-trivial
changes in this diff.
Test Plan:
- owners tool: edit, view, list for basic functionality.
- phlog for the two new methods I added
Reviewers: epriestley, blair, jungejason
CC: aran
Differential Revision: https://secure.phabricator.com/D2079
Summary: NOTE: This is not produced by a script so there might be errors. Please review carefully.
Test Plan: Browse around.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran
Differential Revision: https://secure.phabricator.com/D2091
Summary: Delete some dead code in Feed along the way.
Test Plan:
/feed/
/search/
Reviewers: epriestley
Reviewed By: epriestley
CC: aran
Differential Revision: https://secure.phabricator.com/D2092
Summary:
Most setters returns `$this` but some don't.
I guess it's not by purpose.
Test Plan:
arc lint
Reviewers: epriestley
Reviewed By: epriestley
CC: aran
Differential Revision: https://secure.phabricator.com/D2085
Summary: As title
Test Plan: hit diffcamp, owners to test HandleData
Reviewers: epriestley, btrahan
CC: aran
Differential Revision: https://secure.phabricator.com/D2063
Summary:
I wanted to search for D1234 in texts of other documents.
But search tool always redirects me.
I've left the redirect behavior for simple search forms (header and home) and removed it from full search form.
I don't consider this complete because the first result in search for D1234 should be of course D1234 which is not the case currently.
I am not sure how to solve it:
- We can display a special result in this case.
- We can index the documents so that they will be searchable also for short strings.
I tend to use the first solution because revisions can be truncated at arbitrary length (rX1f1f1f should display revision rX1f1f1f1f1f1f1f).
Test Plan: Search for D1234, rX123, T4.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, epriestley, ddfisher
Differential Revision: https://secure.phabricator.com/D1905
Summary: Added a regex to remove the text
Test Plan: Tested a few messages, from mail application them gmail, both seemed fine, will add unit tests
Reviewers: epriestley
Reviewed By: epriestley
CC: aran
Differential Revision: https://secure.phabricator.com/D2078
Summary:
Show application names, then a human-readable description of what they're for.
Eventually we'll have better help / tutorial / onboarding / etc systems too.
Test Plan: See screenshot.
Reviewers: btrahan, mgummelt
Reviewed By: btrahan
CC: aran, davidreuss
Differential Revision: https://secure.phabricator.com/D2075
