Summary: Clicks all the "Show All" links for you at the touch of a button.
Test Plan:
- Used "reveal entire file" on revealable files.
- Opened on already-visible files, got "entire file shown".
- Used other menu options.
- Used normal "show more" links.
Reviewers: btrahan, jungejason
Reviewed By: btrahan
CC: aran, epriestley, btrahan
Maniphest Tasks: T497
Differential Revision: https://secure.phabricator.com/D1331
Summary:
We currently don't link to comments which aren't visible. Link to the
appropriate diff in a new window, indicating where the comment lives.
Test Plan: Clicked visible, not-so-visible comments.
Reviewers: btrahan, jungejason, davidreuss
Reviewed By: btrahan
CC: aran, btrahan, epriestley
Maniphest Tasks: T555, T449
Differential Revision: https://secure.phabricator.com/D1333
Summary: Make it a little easier to create a bunch of accounts if your company
has more than like 5 employees.
Test Plan: Ran "add_user.php" to create new users. Created new users from the
web console.
Reviewers: btrahan, jungejason, rguerin
Reviewed By: btrahan
CC: aran, btrahan, rguerin
Differential Revision: https://secure.phabricator.com/D1336
Summary:
If you try to establish several sessions quickly (e.g., by running several
copies of "arc" at once, as in "arc x | arc y"), the current logic has a high
chance of making them all pick the same conduit session to refresh (since it's
the oldest one when each process selects the current sessions). This means they
all issue updates against "conduit-3" (or whatever) and one ends up with a bogus
session.
Instead, do an update against the table with the session key we read, so only
one process wins the race. If we don't win the race, try again until we do or
have tried every session slot.
Test Plan:
- Wiped conduit sessions, ran arc commands to verify the fresh session case.
- Ran a bunch of arc piped to itself, e.g. "arc list | arc list | arc list |
...". It succeeds up to the session limit, and above that gets failures as
expected.
- Manually checked the session table to make sure things seemed reasonable
there.
- Generally ran a bunch of arc commands.
- Logged out and logged in on the web interface.
Reviewers: btrahan, jungejason
Reviewed By: btrahan
CC: aran, btrahan
Maniphest Tasks: T687
Differential Revision: https://secure.phabricator.com/D1329
Summary:
Provide an easy way to jump to Diffusion from Differential if we have
the data we need to connect them.
Test Plan: Tested menu in linked and unlinked diffs. Used menu item.
Reviewers: btrahan, jungejason
Reviewed By: btrahan
CC: aran, epriestley, btrahan
Maniphest Tasks: T309
Differential Revision: https://secure.phabricator.com/D1326
Summary:
When the user loads a page with an anchor on it like #thing, or clicks a link to
#thing, and #thing doesn't exist, keep trying to navigate to #thing for a few
seconds.
This allows anchors to work when the target is in content which is later ajaxed
in. In particular, this affects inline comments in Differential.
Test Plan: Opened inline comment links in a new tab, was in the right place when
I switched tabs.
Reviewers: nh, btrahan, jungejason
Reviewed By: btrahan
CC: aran, btrahan, epriestley
Maniphest Tasks: T492
Differential Revision: https://secure.phabricator.com/D1327
Summary: These seem to work relatively reasonably and don't have any known
deal-breaking failures.
Test Plan: shrug~
Reviewers: btrahan, jungejason
Reviewed By: btrahan
CC: aran, btrahan
Differential Revision: https://secure.phabricator.com/D1324
corresponding ConduitAPI
Summary: reasonable title... also made this new functionality used by the
repository worker for parsing diffs
Test Plan:
- looked at the conduit console and queried for various types of hashes,
including hashes with no match. got correct results.
- identified a reasonable diff from a local git repo. set the revision status
to 2 (ACCEPTED) in the database. augmented the worker parser code to var_dump
and die after finding revision id. ran scripts/repository/reparse.php
--message rX and verified my var_dumps. removed var_dumps and die and ran
reparse.php again with same paramters. verified revision looked good in
diffusion and there were no errors.
- repeated the above reparse.php jonx for a mercurial repo. note svn isn't in
this hash game so that test was particularly exciting no-op'dness i did not
bother with
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, btrahan, epriestley
Differential Revision: https://secure.phabricator.com/D1315
Summary:
There are several open Differential tasks that are basically blocked on not
having reasonable places in the UI to put things. Replace the "View Standalone /
Raw" button with a "View Options" dropdown menu so we can shove things like
"Expand All", "Fold / Unfold File", and "View in Diffusion" in there.
This doesn't change any behavior, just puts the existing options in a menu.
Test Plan:
- Toggled menu open by clicking button.
- Clicked menu items.
- Toggled menu closed by clicking button.
- Toggled menu closed by clicking document.
- Toggled menu closed by opening another menu.
- Toggled menu closed by selecting an item.
Reviewers: btrahan, jungejason
Reviewed By: btrahan
CC: aran, btrahan
Maniphest Tasks: T497, T309
Differential Revision: https://secure.phabricator.com/D1316
Summary: The recent change to the field causes us to render "http://junk.com/D"
in some cases, just null the field if there's no data.
Test Plan: Ran "arc diff --create".
Reviewers: btrahan, jungejason
Reviewed By: btrahan
CC: aran, btrahan
Differential Revision: https://secure.phabricator.com/D1321
Summary: Preview of Add Reviewers looks silly without actually showing them
Test Plan:
Go to any diff
Leap into action: Add Reviewers
Add some reviewers
Write some comment
Preview including Added reviewers should be displayed
Change action to Comment
Added reviewers should disappear
Repeat with Add CCs
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, epriestley, vrana
Differential Revision: https://secure.phabricator.com/D1276
Summary:
See D1295. $unit_messages may be undefined.
I'll see if I can improve the visibility of warnings, the red dot in DarkConsole
is easy to miss right now. See T734.
Test Plan: Loaded a revision with no unit failures, didn't receive a warning.
Reviewers: nh, btrahan, jungejason
Reviewed By: btrahan
CC: aran, btrahan
Differential Revision: https://secure.phabricator.com/D1306
Summary:
Some installs use Git as the backbone of a CI framework or use a Git remote to
share patches. The tracker scripts currently recognize associated revisions as
"Committed" when they appear in any branch, even if that branch is
"alincoln-personal-development_test_hack" or whatever.
To address the broadest need here, allow Git repositories to be configured to
track only certain branches instead of all branches.
This doesn't allow you to import a branch into Diffusion but ignore it in
Differential. Supporting that is somewhat technically complicated because the
parser currently goes like this:
- Look at HEAD of all branches.
- For any commits we haven't seen before, follow them back to something we
have seen (or the root).
- "Discover" everything new.
Since this doesn't track <branch, commit> pairs, we currently don't have enough
information to tell when a commit appears in a branch for the first time, so we
don't have anywhere we can put a test for whether that branch is tracked and do
the Differential hook only if it is.
However, I think this cruder patch satisfies most of the need and is simple and
obvious in its implementation.
See also D1263.
Test Plan:
- Updated a Git repository with various filters: "", "master, remote", "derp",
" ,,, master ,,,,,"
- Edited SVN and Mercurial repositories to verify they didn't get caught in
the crossfire.
- Ran daemon in debug mode on libphutil with filter "derp", got exception
about no tracked branches. Ran with filter "master", got tracking. Ran with no
filter, got tracking.
- Looked at Diffusion with "derp" and "master", saw no branches and "master"
respectively.
- Added unit tests to cover filtering logic.
Reviewers: btrahan, jungejason, nh, fratrik
Reviewed By: fratrik
CC: aran, fratrik, epriestley
Maniphest Tasks: T270
Differential Revision: https://secure.phabricator.com/D1290
Summary:
- We have a few places where we do some kind of ad-hoc comma list tokenizing,
and I'm adding another one in D1290. Add a helper to the request object.
- Add some unit tests.
Test Plan:
- Ran unit tests.
- Used PHID manager, Maniphest custom view, and Repository project editor.
Reviewers: btrahan, fratrik, jungejason
Reviewed By: btrahan
CC: aran, btrahan, epriestley
Differential Revision: https://secure.phabricator.com/D1302
Summary: This diffs adds support for marking up unittest result messages.
Test Plan: Verified that links in unittest results were markup'd.
Reviewers: epriestley, jungejason
Reviewed By: epriestley
CC: aran, epriestley, zeeg
Differential Revision: https://secure.phabricator.com/D1298
Summary:
D1293 adds support for a literal block in remarkup. This diff enables
it in phabricator with a few basic rules (for line breaks, escaping HTML,
and linkifying URLs).
Test Plan: Tested in sandbox
Reviewers: epriestley, jungejason
Reviewed By: epriestley
CC: aran, epriestley
Differential Revision: https://secure.phabricator.com/D1297
Summary:
When all unit tests pass, a box appears between the unit test results and lint
status (for test failures to go in). This checks if there's anything to put
in that div/ul before putting it on the page.
Test Plan:
Loaded a revision with unit tests OK and saw no box. Loaded a revision with
failing unittests, and saw the same box from before.
Reviewers: tuomaspelkonen, epriestley
Reviewed By: epriestley
CC: jungejason, aran, nh, epriestley
Differential Revision: https://secure.phabricator.com/D1295
Summary: Makes it easier to discover the list of all revisions for a user.
Test Plan:
Opened up /differential/filter/revisions/, and saw that it defaulted
to status of all. Clicked between tabs, and it stayed on all. Selected
open, it only displayed open revisions, including as I switched between
tabs.
Reviewers: epriestley, jungejason
Reviewed By: epriestley
CC: aran, epriestley
Differential Revision: https://secure.phabricator.com/D1278
Summary:
We have a debug interface for sending various sorts of email, but normal users
don't really need to use it. In particular, they can:
- Send arbitrary email to other users;
- Discover other users' email addresses fairly easily (CC everyone);
- Send arbitrary email to arbitrary addresses in conjunction with "Mailing
Lists"
In fact, normal users don't need to get to the MetaMTA web interface at all and
it has some somewhat-sensitive things beacuse it has a lot of detailed
information about mail. For instance, users can look at mail records to discover
things like password reset links and per-user object email addresses.
We should smooth out the UI here but I think I can do something about T21 fairly
soon and cover it then.
Test Plan:
Went to /mail/ with a non-admin, got 404'd. Went to /mail/ with an
admin, everything works, got a red admin header.
Reviewers: jungejason, btrahan
Reviewed By: btrahan
CC: aran, btrahan, jungejason
Maniphest Tasks: T718
Differential Revision: https://secure.phabricator.com/D1292
Summary: makes a nice side filter for most UI elements. only place this getds a
little funky is on the test console; a second, inner filter list appears for the
"affected" filters.
Test Plan: viewed each side filter and verified ui. for each filter, interacted
with the ui and made sure things looked right and there were no errors
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, epriestley
Maniphest Tasks: T631
Differential Revision: https://secure.phabricator.com/D1289
Summary:
See T624. I originally wrote this to require an explicit remote, but this
creates an ugly "origin:" in all the URIs and makes T270 more difficult.
Treat all branch names as implying 'origin/'.
Test Plan:
- Pulled and imported a fresh copy of libphutil without issues.
- Browsed various git repositories.
- Browsed Javelin's various branches.
- Ran upgrade script, got a bunch of clean 'origin/master' -> 'master'
conversions.
- Tried to specify an explicit remote in a default branch name.
- Unit tests.
Reviewers: nh, jungejason, btrahan
Reviewed By: btrahan
CC: aran, btrahan
Maniphest Tasks: T624
Differential Revision: https://secure.phabricator.com/D1269
Summary:
- When changing auxiliary field values, use transactions.
- Clean up some of the load/save logic for auxiliary fields so it's a little
more performant.
NOTE: The transaction display of auxiliary fields is incredibly hacky, I'll
follow up with a more nuanced approach but wanted to limit scope here.
Test Plan: Created and edited tasks with custom fields configured; created and
edited tasks without custom fields configured.
Reviewers: btrahan, jungejason, zeeg
Reviewed By: jungejason
CC: aran, jungejason
Maniphest Tasks: T418
Differential Revision: https://secure.phabricator.com/D1283
Summary: Allow paths to match even if they differ by trailing slashes and
".git".
Test Plan: Ran unit tests.
Reviewers: jungejason, btrahan
Reviewed By: jungejason
CC: aran, jungejason
Maniphest Tasks: T710
Differential Revision: https://secure.phabricator.com/D1286
Summary:
- These never actually did anything.
- I don't even really remember why I built them, maybe the Open Source team
was pushing for more GitHub integration or something? I really have no idea.
- Anyway, repository tailers do everything these could do (and much more).
Test Plan:
- Ran tailers off GitHub for many months without needing post-receive hooks.
- Grepped for relevant strings, couldn't find any references.
- Used "Repository" edit interface for a Git repository.
Reviewers: btrahan, jungejason
Reviewed By: jungejason
CC: aran, jungejason
Maniphest Tasks: T706
Differential Revision: https://secure.phabricator.com/D1273
Summary:
- On the edit view, this is represented as a checkbox.
- On the detail view, it renders with a user-selectable string.
Test Plan: Added a bool field to my local install, checked and unchecked it.
Reviewers: zeeg, jungejason, btrahan
Reviewed By: jungejason
CC: aran, jungejason
Differential Revision: https://secure.phabricator.com/D1277
Summary:
Git accepts either "git@x:/path" or "ssh://git@x.com/path" URIs to mean the
exact same thing, which is causing some false positives and confusion,
particularly because we sometimes mutate URIs.
Since this is just a sanity check, we don't really care about the username,
domain or credentials -- matching the paths is good enough. We're just trying to
make it hard to shoot yourself in the foot by copy-pasting the same local path
into two repositories and forgetting to change one, like I did. :P
Relax the check to only verify the paths are the same.
Test Plan:
- Ran unit tests, which should fully cover things.
- Ran commit discovery daemon in debug mode on incorrectly and correctly
configured repositories.
Reviewers: ajtrichards, jungejason, btrahan
Reviewed By: jungejason
CC: aran, jungejason
Maniphest Tasks: T710
Differential Revision: https://secure.phabricator.com/D1279
layout
Summary:
- Use new less-horrible layout.
- Organize information more completely and sensibly.
Test Plan: Looked at some profiles.
Reviewers: btrahan, jungejason
Reviewed By: jungejason
CC: aran, jungejason
Differential Revision: https://secure.phabricator.com/D1281
Summary:
It used to be more useful for daemons to spew random debugging information, but
features like "phd debug" and some fixes to error reporting like D1101 provide
better ways to debug, test, develop and diagnose daemons.
- Stop writing "." every time MetaMTA sends a message.
- Stop spewing the entire IRC protocol from the IRC bot unless in debug mode.
- Stop writing GC daemon log entries about collecting daemon logs (DURRR)
unless in debug mode.
Test Plan: Ran daemons in debug and non-debug modes, got expected level of
noisiness.
Reviewers: jungejason, nh, btrahan
Reviewed By: jungejason
CC: aran, jungejason
Differential Revision: https://secure.phabricator.com/D1268
Summary: There can be Dxxx, rXXXxxx or even full URL in //Blame Revision// field
so just highlighting it as normal text would work probably best
Test Plan:
Go to https://secure.phabricator.com/D277
You should see a link from //Blame Revision// (if it would be displayed)
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, epriestley
Differential Revision: https://secure.phabricator.com/D1274
Summary:
when a path is '/' in defining a package, D1251 is generating
an extra '//'.
Test Plan: veryfied adding path '/', '/src' and '/src/' all worked.
Reviewers: nh, epriestley
Reviewed By: epriestley
CC: aran, epriestley
Differential Revision: https://secure.phabricator.com/D1266
Summary: I think we only hit this because I mucked around with the database to
recover from the runaway parse of the Diviner repository (now prevented by
D1253), but be more robust against missing data in this interface.
Test Plan: After applying this patch, no longer received a fatal on the commit
history page for users linked to nonexistant/bogus commits.
Reviewers: jack, btrahan, jungejason, aran
Reviewed By: aran
CC: aran
Maniphest Tasks: T701
Differential Revision: https://secure.phabricator.com/D1264
- Use the computed remote URI (which may have an explicit 'ssh://' under Git in some cases).
- Use '$id' correctly rather than casting the URI to an int in the message parser.
Summary: I want to add a command like "where is ArcanistUnitTestEngine" to
phabot. I also want to add a symbol typeahead to Diffusion and generally finish
up that feature since it's useful but only half-implemented. Consolidate the
query logic and expose the data over Conduit.
Test Plan: Used /symbol/ and Conduit to lookup symbols.
Reviewers: btrahan, jungejason
Reviewed By: jungejason
CC: aran, jungejason
Maniphest Tasks: T315
Differential Revision: https://secure.phabricator.com/D1260
Summary: See D1257. Also make the error message more friendly, and remove a very
very old Facebook-specific error.
Test Plan:
- Tried to diff with an older arc.
- Tried to diff with a newer arc.
- Diffed with the right arc.
Reviewers: btrahan, jungejason, aran
Reviewed By: aran
CC: aran, epriestley
Differential Revision: https://secure.phabricator.com/D1258
Summary:
- Previously, used IDs like "33" to match a commit to a Differential revision.
This has a namespacing problem because we now have an arbitrarily large number
of Phabricator installs in the world, and they may want to track commits from
other installs.
- In Differential, parse raw IDs or full URIs. Emit only full URIs.
- In Repositories, parse only full URIs.
- This might cause a few commits to not be picked up in rare circumstances.
Users can fix them with "arc mark-committed". This should be exceedingly rare
because of hash matching.
- There are some caveats for reparsing older repositories, see comments
inline. I don't think there's much broad impact here.
Test Plan:
- Created a new revision, got a full URI.
- Updated revision, worked correctly.
- Ran unit tests.
- Monkeyed with "Differential Revision" field.
- Reviewers: btrahan, jungejason
Reviewers: btrahan, jungejason
Reviewed By: jungejason
CC: aran, epriestley, jungejason
Maniphest Tasks: T54, T692
Differential Revision: 1250
Summary:
Although I couldn't repro the issue in T692, I did manage to point the "Diviner"
repository at the "Phabricator" working copy and screw some stuff up on
secure.phabricator.com.
Before discovering commits in a repository, ensure the 'origin' remote points at
the configured URI. This prevents issues where the working copy gets configured
to point at an existing (but incorrect) checkout.
Test Plan:
- Ran gitcommitdiscovery daemon normally under "phd debug", saw it execute the
"remote show -n" command and then start working.
- Intentionally botched the config, got an exception:
(Exception) Working copy '/INSECURE/repos/phabricator' has origin URL
'ssh://git@github.com/facebook/phabricator.git', but the configured URL
'git://github.com/facebook/diviner.git' is expected. Refusing to proceed.
Reviewers: btrahan, jungejason
Reviewed By: jungejason
CC: aran, jungejason
Maniphest Tasks: T692
Differential Revision: 1253
Summary:
Allow entry of "CC: alincoln" to match user "ALincoln".
Put both variations in the map and try the exact case version first since we'll
also match email addresses and mailables, and theoretically some mailable might
have the same name as a user, as we're effectively abandoning restriction of
which characters can appear in usernames.
Test Plan: Created a local revision with a reviewer in CrAzY CaPs.
Reviewers: jungejason, btrahan
Reviewed By: jungejason
CC: aran, jungejason
Maniphest Tasks: T697
Differential Revision: 1255
Summary:
Paths in owners packages when referring to a directory should always end with
a trailing slash. (Otherwise, some things break, like loading the owning
packages for a path.) With this change, PhabricatorOwnersPackage now requires
that the path provided for a package is valid, and if the path is for a
directory, it adds a trailing slash if one was not provided.
Test Plan:
Edited a path in a package and left off the trailing slash. Saw that the slash
was added. Tried again with the trailing slash, and checked that another slash
was not added. Did this with a path in both a git and svn repository.
Reviewers: epriestley, jungejason
Reviewed By: epriestley
CC: aran, epriestley
Differential Revision: 1251
Summary:
- Old page was useless and dumb.
- New page looks a little less bad, functions a little less poorly.
- Still lots of work to be done.
Test Plan:
- Viewed a project.
- Clicked all the links on the left nav.
- Here is a screenshot:
https://secure.phabricator.com/file/view/PHID-FILE-4buzquotb3fo4dhlicrw/
Reviewers: btrahan, jungejason
Reviewed By: jungejason
CC: aran, jungejason
Maniphest Tasks: T681
Differential Revision: 1246
Summary: Share more code; reduce the number of ad-hoc versions of this rendering
loop.
Test Plan: Clicked all the filters.
Reviewers: btrahan, jungejason
Reviewed By: jungejason
CC: aran, jungejason
Differential Revision: 1247
Summary:
Provide tighter integration between Projects and Phriction. Partly, I have most
of a rewrite for the Projects homepage ready but it's not currently possible to
publish feed stories about a project so all the feeds are empty/boring. This
partly makes them more useful and partly just provides a tool integration point.
- When you create a project, all the wiki pages in projects/<project_name>/*
are associated with it.
- Publish updates to those pages as being related to the project so they'll
show up in project feeds.
- Show a project link on those pages.
This is very "convention over configuration" but I think it's the right
approach. We could provide some sort of, like, "@project=derp" tag to let you
associated arbitrary pages to projects later, but just letting you move pages is
probably far better.
Test Plan:
- Ran upgrade scripts against stupidly named projects ("der", " der", " der
", "der (2)", " der (2) (2)", etc). Ended up with uniquely named projects.
- Ran unit tests.
- Created /projects/ wiki documents and made sure they displayed correctly.
- Verified feed stories publish as project-related.
- Edited projects, including perfomring a name-colliding edit.
- Created projects, including performing a name-colliding create.
Reviewers: btrahan, jungejason
Reviewed By: btrahan
CC: aran, epriestley, btrahan
Maniphest Tasks: T681
Differential Revision: 1231
Summary:
add basic auditing functionalities. For the related commits for a
package, we detect the following conditions which might be suspicious to the
owners of the package:
* no revision specified
* revision not found
* author not match
* reviewedby not match
* owners not involved
* commit author not recognized
The owners of the package can change the status of the audit entries by
accepting it or specify concern.
The owner can turn on/off the auditing for a package.
Test Plan:
* verified that non-owner cannot see the details of the audit and cannot modify
it
* verified that all the audit reasons can be detected
* tested dropdown filtering and package search
* verified really normal change not detected
* verified accept/concern a commit
* tested enable/disable a package for auditing
* verified one audit applies to all <commit, packages> to the packages the
auditor owns
* verified that re-parsing a commit won't have effect if there exists a
relationship for <commit, package> already
Reviewers: epriestley, nh
Reviewed By: epriestley
CC: aran, benmathews, btrahan, mpodobnik, prithvi, TomL, epriestley
Differential Revision: 1242
Summary: This was well-intentioned but has not actually proven to be useful.
Test Plan:
- No list tab shows up anymore.
- Looked up a PHID.
Reviewers: btrahan, jungejason, Girish
Reviewed By: Girish
CC: aran, jungejason, edward, emiraga, Girish, nh, tuomaspelkonen, epriestley
Maniphest Tasks: T631
Differential Revision: 1234
Summary: I didn't get around to this earlier; add Feed/Maniphest integration.
This is partly motivated by wanting Projects to not be terrible. Pretty
straightforward.
Test Plan:
- Created, updated, reassigned and closed a task.
- Verified feed stories render reasonably.
Reviewers: btrahan, jungejason
Reviewed By: btrahan
CC: aran, btrahan
Maniphest Tasks: T681
Differential Revision: 1232
Summary: These are "local" commands, but need remote credentials. If the daemon
runs as a user who does not have credentials, the initial clone will work but
subsequent updates will fail.
Test Plan:
- Nuked a local copy of a Git repo.
- Ran "phd debug fetch <phid>" as root (or any other user with no natural SSH
keys). Verified initial clone worked (since it passes credentials to the command
correctly).
- Killed daemon, re-ran, verified "fetch" failed (no credentials passed).
- Applied this patch.
- Re-ran "phd debug fetch <phid>", verified it passed credentials and
succeeded.
- Did all these steps for a Mercurial repo.
Reviewers: btrahan, jungejason
Reviewed By: btrahan
CC: aran, btrahan
Maniphest Tasks: T686
Differential Revision: 1236
Summary:
I pretty much copy/pasted this code; rather than do that again now that I want
to add feeds to projects, share the code.
This "Builder" is a little weird -- I don't want to call it a "View" because it
does data access. "Builder" seemed okay. We don't really have much code that
does this sort of thing right now, elsewhere.
Test Plan:
- Viewed public feed.
- Viewed private feed.
Reviewers: btrahan, jungejason
Reviewed By: btrahan
CC: aran, btrahan
Maniphest Tasks: T681
Differential Revision: 1233
Summary:
- For context, see T547. This is the last (maybe?) in a series of diffs that
moves us off raw sha1() calls in order to make it easier to audit the codebase
for correct use of hash functions.
- This breaks CSRF tokens. Any open forms will generate an error when
submitted, so maybe upgrade off-peak.
- We now generate HMAC mail keys but accept MAC or HMAC. In a few months, we
can remove the MAC version.
- The only remaining callsite is Conduit. We can't use HMAC since Arcanist
would need to know the key. {T550} provides a better solution to this, anyway.
Test Plan:
- Verified CSRF tokens generate properly.
- Manually changed CSRF to an incorrect value and got an error.
- Verified mail generates with a new mail hash.
- Verified Phabricator accepts both old and new mail hashes.
- Verified Phabricator rejects bad mail hashes.
- Checked user log, things look OK.
Reviewers: btrahan, jungejason, benmathews
Reviewed By: btrahan
CC: aran, epriestley, btrahan
Maniphest Tasks: T547
Differential Revision: 1237
Summary:
Outlook wraps a message in 5 dashes on each side when doing replies.
This strips english and danish versions.
Test Plan:
Tried parsing emails with different messages and saw the
expected behaviour with patch applied. Ran arc unit, and saw test
passed.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, epriestley
Differential Revision: 1239
Summary:
When changing attachments, the removed part is squished together
with the added parts, making it hard to read. This changes the output
so it looks like other changes, seperating each action by a semicolon.
Test Plan:
Viewed a task where i had attached and deleted revisions, and
saw the output look as other changes of same type.
Reviewers: epriestley, btrahan
Reviewed By: epriestley
CC: aran, epriestley
Differential Revision: 1238
Summary:
- Add a "delete" operation. Delete is just a special edit which removes the
page from indexes and shows a notice that the document has been deleted.
- When a user deletes all the content on a page, treat it as a delete.
- When a conduit call deletes all the content on a page, treat it as a delete.
- Add page status to Conduit.
- Add change type field to history.
- Added a couple of constants to support a future 'move' change, which would
move content from one document to another.
Test Plan:
- Verified deleted pages vanish from the document index (and restoring them
puts them back).
- Verified deleted pages show "This page has been deleted...".
- Created, edited and deleted a document via Conduit.
- Deleted pages via "delete" button.
- Deleted pages via editing content to nothing.
Reviewers: btrahan, jungejason
Reviewed By: btrahan
CC: skrul, aran, btrahan, epriestley
Maniphest Tasks: T680
Differential Revision: 1230
Summary: Remove a bunch of relatively useless stuff from the Project list
interfaces.
Test Plan: Looked at project lists, less random busy junk.
Reviewers: btrahan, jungejason, zeeg
Reviewed By: btrahan
CC: aran, btrahan
Maniphest Tasks: T681
Differential Revision: 1229
Summary: Add 'addLines' and 'delLines' properties to differential.getdiff return
dictionary. These properties are aggregated from the changesets.
Test Plan: Issue a differential.getdiff query via conduit and verify that
'addLines' and 'removeLines' properties are included and accurate
Reviewers: epriestley
Reviewed By: epriestley
CC: epriestley, aran, jonathanhester
Differential Revision: 1209
Summary:
This is a needlessly confusing/complex feature that I originally wrote sort of
speculativley. I think we can better serve what little need may exist here with
project feeds.
I'm probably going to get rid of or deemphasize "role" too and just add "Join
Project" and "Leave Project" buttons.
Test Plan: Viewed project list, project profile. Edited project profile and
affiliation.
Reviewers: btrahan, jungejason, zeeg
Reviewed By: btrahan
CC: aran, btrahan
Maniphest Tasks: T681
Differential Revision: 1228
Summary:
- Allow more than the 100 most recent projects to be viewed.
- Provide some useful filters.
- Default the view to your projects, not all projects.
- Put query logic in a query object.
- Put filter view logic in a view object. We can port more stuff to it later.
Test Plan: Looked at active/owned/all projects. Set page size to 5 and paged
through projects.
Reviewers: btrahan, jungejason, zeeg
Reviewed By: btrahan
CC: aran, btrahan
Differential Revision: 1227
Summary:
We have this code in two places; split it into an editor class so we can share
it.
This also fixes some probems with this field not //detaching// tasks properly.
Test Plan:
- Created a revision with no attached tasks.
- Attached it to a task.
- Updated it.
- Detached it.
- Used web UI to attach/detach tasks/revisions.
Reviewers: btrahan, jungejason
Reviewed By: btrahan
CC: aran, btrahan, epriestley
Differential Revision: 1225
updated
Summary:
- If you update a revision with a nonempty "Maniphest Tasks" field, an empty
comment is posted (see T586).
- The transaction email currently says "Attached revision 'Unknown
Differential Revision'", move attaching to "didWriteRevision()" to make sure the
object has been written.
Test Plan: - Attached; updated a revision.
Reviewers: btrahan, jungejason
Reviewed By: btrahan
CC: aran, btrahan
Maniphest Tasks: T685
Differential Revision: 1223
Summary:
- Use DifferentialRevisionQuery, not DifferentialRevisionListData, to select
revisions.
- Make UI simpler (I hope?) and more flexible, similar to Maniphest. It now
shows "Active", "Revisions", "Reviews" and "Subscribed" instead of a hodge-podge
of miscellaneous stuff. All now really has all revisions, not just open
revisions.
- Allow views to be filtered and sorted more flexibly.
- Allow anonymous users to use the per-user views, just don't default them
there.
NOTE: This might have performance implications! I need some help evaluating
them.
@nh / @jungejason / @aran, can one of you run some queries agianst FB's corpus?
The "active revisions" view is built much differently now. Before, we issued two
queries:
- SELECT (open revisions you authored that need revision) UNION ALL (open
revisions you are reviewing that need review)
- SELECT (open revisions you authored that need review) UNION ALL (open
revisions you are reviewing that need revision)
These two queries generate the "Action Required" and "Waiting on Others" views,
and are available in P247.
Now, we issue only one query:
- SELECT (open revisions you authored or are reviewing)
Then we divide them into the two tables in PHP. That query is available in P246.
On the secure.phabricator.com data, this new approach seems to be much better
(like, 10x better). But the secure.phabricator.com data isn't very large. Can
someone run it against Facebook's data (using a few heavy-hitting PHIDs, like
ola or something) to make sure it won't cause a regression?
In particular:
- Run the queries and make sure the new version doesn't take too long.
- Run the queries with EXPLAIN and give me the output maybe?
Test Plan:
- Looked at different filters.
- Changed "View User" PHID.
- Changed open/all.
- Changed sort order.
- Ran EXPLAIN / select against secure.phabricator.com corpus.
Reviewers: btrahan, nh, jungejason
Reviewed By: btrahan
CC: cpiro, aran, btrahan, epriestley, jungejason, nh
Maniphest Tasks: T586
Differential Revision: 1186
Summary:
...except that pesky help tab which remains.
Pertinent bits here...
- move "History" into button "View History" that is grey and next to "Edit Page"
- for history page, add breadcrumb similar to the one on "diff" page. This
unifies the experiencing on history <=> diffs as well as gives the user a link
back to the document, which was a tab on the History page before this diff.
Thoughts for next time...
- I'd like to further unify the breadcrumbs between "View" and "History / Diff".
- The "Document Index" is pretty sweet and feels a bit buried. I wonder if
unifying breadcrumbs is the key here?
Test Plan: clicked around phriction. viewed a document, viewed its history.
verified links in breadcrumbs were correct
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, epriestley
Maniphest Tasks: T631
Differential Revision: 1221
Summary:
kill tabs for Files application. Technique is the "filter list" on the left
hand side, with separation for "Files" versus "Image Macros". UI quirks
include:
- the page title does not change for the 3 files filters while it does change
for each of the two image macro filters.
- standalone "file" pages do not have the filter view
- you can visit /file/upload/ standalone and it doesn't have the pretty filter
list on it
Please do give direction on these quirks if you like. :)
This change also neuters the ?author= functionality for files. The code is
written such that it can easily be brought back.
Test Plan: clicked around on the filters, liked what I saw. uploaded files
fancy-like and basic-like and it worked! made image macros and it worked!
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, btrahan, epriestley
Maniphest Tasks: T631
Differential Revision: 1219
Summary:
For each commit, find the affected packages, and provide a way to
search by package.
Test Plan:
create commits that touch and don't touch two packages, and verify
that they display correctly in all the UI pages.
Reviewers: epriestley, blair, nh, tuomaspelkonen
Reviewed By: epriestley
CC: benmathews, aran, epriestley, btrahan, jungejason, mpodobnik, prithvi
Maniphest Tasks: T83
Differential Revision: 1208
Summary: grab all the files in one big fetch, rather than serially fetching
them. follow up from D1198.
Test Plan: viewed paste and there were no errors!
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, jungejason, btrahan, epriestley
Differential Revision: 1202
Summary:
See D1195, which fataled this daemon.
https://secure.phabricator.com/daemon/log/2966/
Test Plan: Applied this patch to secure.phabricator.com, restarted daemon, it
picked up D1203.
Reviewers: btrahan, jungejason, mareksapota
Reviewed By: btrahan
CC: aran, btrahan, mareksapota
Differential Revision: 1204
Summary: D1174 caught this issue -- we mean to load all //your// rules, but
actually load //all// rules. Use %s correctly.
Test Plan: Hit /herald/rule/ without an exception.
Reviewers: fmoo, btrahan, jungejason
Reviewed By: btrahan
CC: aran, epriestley, btrahan
Differential Revision: 1203
Summary:
merge paste create and paste list into a single controller. Add a "filter list"
to the left hand side and have new "create w/ recent", "my" and "all" views. UI
wrinkle -- "create w/ recent" does not paginate the recent pastes and instead
upsells the user to the new "all" view.
Also includes a business logic clean up or two for simplicity of code.
Test Plan:
- created a paste from the UI
- tried to create a paste with title and no body
- tried to create a paste with no title and no body
- viewed the paste list on "create" view
- viewed the paste list on "author" view
- viewed the paste list on "all" view
- viewed page 2 of the paste list for "author" and "all" views
- "forked" a given paste through completion
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, epriestley, btrahan
Maniphest Tasks: T631
Differential Revision: 1198
Test Plan:
Created a listener that adds some patterns to $matches array, reloaded
Differential, some changesets were not shown as generated.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, epriestley, mareksapota
Differential Revision: 1200
Summary: Add the author PHID to the differential.getrevisionfeedback conduit api
method
Test Plan: issue differential.getrevisionfeedback query via conduit against a
valid revision and verify author phid is included in results
Reviewers: epriestley
Reviewed By: epriestley
CC: epriestley, aran, jungejason, tuomaspelkonen, jonathanhester
Differential Revision: 1190
Test Plan: Commit as not the author and see what shows up.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, epriestley, mareksapota, zeeg
Differential Revision: 1195
Summary:
use the handy DifferentialChangesetParser to do most of the heavy lifting inside
the pertinent view object. update the controller to be aware of the "show
more" calls coming from the new ui and update the transactionID appropriately.
also snuck in a small change to AprontRequest to all getting all the request
data. I used it to debug building this.
Test Plan: made a task and entered a bunch of test data. had descriptions of
various lengths, as well as really long descriptions that i did not change to
much. verified the diff looked correct and various "show more" links worked as
expected
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, btrahan, epriestley
Differential Revision: 1187
couple bug fixes
Summary:
- Add the ability to query for "responsible users" (author or reviewer).
- Add the ability to query for "subscribers" (reviewer or CC).
- Fix an issue where CC and Reviewer used the same join table alias and were
incompatible.
- Remove support for 'paths' for the moment, since each path needs a
repository ID. (There are no clients for this.)
- Remove single withX() methods that have no callsites -- withPath() is
singular because it accepts two arguments and I didn't want to have an ad-hoc
type format, but I think we can get away without these for other conditions.
- Include GROUP BY in more cases where may need it. This doesn't actually
change program behavior since we uniquify in loadFromArray(), it just means less
data over the wire.
These new query classes are to support rewriting the Differential list view on
top of DifferentialRevisionQuery.
Test Plan:
- Issued queries via conduit for "responsible users".
- Issued queries via conduit for "subscribers".
- Issued queries via conduit for "cc" with "reviewer" at the same time.
- Issued queries via conduit for "cc", "reviewer", "responsible users" and
"subscribers" at the same time.
- Issued a "subscribers" and "reviewers" query which returned duplicates;
verified GROUP BY took effect.
Reviewers: nh, btrahan, jungejason
Reviewed By: nh
CC: aran, nh
Differential Revision: 1182
Summary:
Derped this one up; while my testing was successful in preventing runaway
attaching I missed the bit where it doesn't actually work.
This resolves the "Unknown Object" link seen on T661.
Test Plan:
- Created two new revisions, each attached to a local task.
- Verified that they attached additively, Maniphest and Differential were
linked to the right places, and nothign else bad happened.
Reviewers: btrahan, fratrik
Reviewed By: fratrik
CC: aran, fratrik, btrahan
Differential Revision: 1181
Summary:
Prevent keyboard focus of these links so we don't disrupt tab order from
comments to "Submit".
Arguably I should make a "function" for this or something but there's nowhere to
really put it that makes any sense right now.
Test Plan: Verified Firefox skips these links in tab order.
Reviewers: fratrik, btrahan, jungejason
Reviewed By: fratrik
CC: aran, fratrik
Maniphest Tasks: T661
Differential Revision: 1180
Summary:
This landed during my review drama embargo and is a generally good idea but had
some implementation issues.
@elynde reports it has been broken for some time, although it still works on
secure.phabricator.com so I'm guessing it's just taking a zillion years to run
at Facebook. It's up to more than a second for me on secure.phabricator.com:
https://secure.phabricator.com/file/view/PHID-FILE-v4ql4c66u3xnkarmrpm4/
The basic problem is that some of the data architecture around this
implementation is hard to scale. I want to pursue a similar feature eventually,
but drive it off notifications that we'll ship through real-time infrastructure
too.
I'm also trying to get rid of DifferentialRevisionListData and this simplifies
that somewhat.
Test Plan:
- Grepped for table name, table constant, query constant, and class name; no
hits.
- Applied SQL patch.
- Verified that Differential no longer shows "Updated".
Reviewers: elynde, btrahan, jungejason
Reviewed By: elynde
CC: aran, elynde
Differential Revision: 1178
Summary:
Changed cc/reviewer search to be a union/or instead of intersection/and within
each list. Also added support to search for multiple authors (same behavior as
cc/reviewer), and updated conduit call to match. (See discussion on D1158.)
Test Plan:
Used the conduit call to search for revisions with one of 2 people on the cc
list, and checked the results to see that it wasn't constraining to requiring
both be on the cc list.
Reviewers: epriestley, jungejason, btrahan
Reviewed By: epriestley
CC: aran, nh, epriestley
Differential Revision: 1179
Summary: make the change, kill the function. be sure to get a good $user or
$viewer variable
Test Plan:
for each controller or view, look at it in the ui. change timezone, refresh ui
and note change. i did not test the OAuthSettingsPanelController; not sure how
to get to that badboy and i got a bit lazy
Maniphest Tasks: T222
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, btrahan, epriestley
Maniphest Tasks: T222
Differential Revision: 1166
Summary:
To reduce blindness, all textareas with some kind of special syntax should have
an information about this syntax and a link to its documentation. Preview
function is a nice complement but it doesn't replace this information.
I've added this information and the link below the comment field.
Please note that <a target> is a valid attribute in HTML5.
Test Plan:
Go to https://secure.phabricator.com/D1164#comment-content
There should be a link to Remarkup Reference
This link should open Remarkup Reference in a new window (to not discard the
comment)
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, epriestley, vrana
Differential Revision: 1164
Summary:
add "Maniphest Task:" or "Maniphest Tasks:" followed by text that has TX in it.
foreach TX the task will be attached to the revision and the revision will be
attached to the task. parsing is pretty... ummm, robust such that it will pick
up any TX substring and parse that as a Maniphest Task just fine. it errors
out if there is not an actual task for TX and otherwise churns along pretty
nicely.
Also, make sure the PhabricatorObjectHandle loads the task ID as the alternateID
since we need that here and it should be that way anyhoo.
Test Plan:
made a diff and in the commit message added Maniphest Task(s): TX combination.
Tried various combinations of TX -- single, multiple with commas, multiple many
lines, single bad, multiple bad, multiple mix of bad and good. verified that the
good tasks were attached to the diff and diff was attached to the good tasks.
Maniphest Tasks: T137
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, btrahan, epriestley
Differential Revision: 1165
Summary: Some fields need this data in some circumstances in order to validate
-- see D1153.
Test Plan: Ran "arc diff" against local, no longer got an exception for access
of this field from the 'Reviewers' validator.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran, btrahan
Differential Revision: 1160
Summary:
Created a differential.query conduit method that is built on top of
DifferentialRevisionQuery. I also added support for querying by author, ccs, and
reviewers to DifferentialRevisionQuery, so feature parity can be brought up to
match differential.find and its backing class DifferentialRevisionListData.
Test Plan:
Tried a few calls to the conduit call using the web interface, and got back
reasonable looking data.
Reviewers: epriestley, jungejason, btrahan
Reviewed By: epriestley
CC: aran, nh, epriestley
Differential Revision: 1158
Summary:
kill the tabs and make it a create button instead. pertinent notes:
* added a "Filter diffs" button to the form. optional, but i thought it
necessary with the new green button
* linked to Arcanist user guide on the create diff page. somewhat unrelated but
i think create diff will get more traffic now so linking to help seemed like a
reasonable add on here.
Test Plan:
viewed differential homepage
* clicked left hand filter elements. noted "Create Diff" button on filters
within user revisions and no button on filters within all revisions.
* entered another user into Select User UI and viewed their diffs via button and
pressing enter
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, btrahan, epriestley
Differential Revision: 1157
