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:
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
Summary:
Like the title says, similar to Facebook Tasks.
Not sure how I really feel about this, but I guess it's kind of OK? I never used
this feature in Facebook Tasks but I think some people like it.
The drag-and-drop to repri across priorities feels okayish.
Because subpriority is a double and we just split the difference when
reprioritizing, you lose ~a bit of precision every time you repri two tasks
against each other and so you can break it by swapping the priorities of two
tasks ~50 times. This case is pretty silly and pathological. We can add some
code to deal with this at some point if necessary.
I think this also fixes the whacky task layout widths once and for all.
(There are a couple of minor UI glitches like headers not vanishing and header
counts not updating that I'm not fixing because I am lazy.)
Test Plan: Dragged and dropped tasks around.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran, epriestley, mgummelt
Maniphest Tasks: T859
Differential Revision: https://secure.phabricator.com/D1731
Summary:
We'll incorrectly send CCWelcome mail to users who would be added as CCs but are blocked by the new "$dont_add" stuff, for
example when a revision is updated and the user has a Herald rule which triggers them getting CC'd. See D2057.
Potentially a better fix for this would be to have "addCCs" return a list of the CCs it actually added, rather than duplicating the
logic of removing CCs in two places. However, that's not trivial since it's just a wrapper around alterRelationships() which is nasty
and would need a more complicated return type. I think this whole thing will get a refactoring pass at some point -- I want to build a
more generic "associations"-like datastore and replace some of the ad-hoc associations with it. So maybe I can clean it up when that
happens. For now, this should fix the immediate problem.
Test Plan: Updated a revision, didn't get CC welcomed.
Reviewers: vrana, btrahan
Reviewed By: vrana
CC: aran
Differential Revision: https://secure.phabricator.com/D2072
Summary:
- Remove the "Priority" column, since this is indicated by the color swatch, to save space.
- Reduce the "Updated" column from datetime to date only, since time isn't incredibly useful, to save space.
- Show the first two projects a task is associated with, and "..." if there are more.
- Show "None" (for "no owner") in a lighter color.
Test Plan: Looked at tasks on homepage and in Maniphest.
Reviewers: btrahan, 20after4
Reviewed By: btrahan
CC: aran, edward
Maniphest Tasks: T967
Differential Revision: https://secure.phabricator.com/D2065
Summary:
Format a date as 'today', 'yesterday', or 'Mar 27 2012'. Optionally,
the final example can be rendered 'on Mar 27 2012' for things like:
$excuse =
'I fell out of a window '.
phabricator_on_rel_date($time, $me);
Test Plan: Tested in my sandbox!!!!
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, epriestley
Differential Revision: https://secure.phabricator.com/D2035
Summary:
Boolean search supports operators, such as phrase search.
It can be further improved by setting [[http://dev.mysql.com/doc/mysql/en/server-system-variables.html#sysvar_ft_boolean_syntax | ft_boolean_syntax]] to `' |-><()~*:""&^'` (note the leading space):
Default value uses no operator for "optional word" and `+` for "mandatory word".
This value uses no operator for "mandatory word" and `|` for "optional word".
Test Plan: Search for "Enter the name" (with quotes).
Reviewers: epriestley
Reviewed By: epriestley
CC: aran
Differential Revision: https://secure.phabricator.com/D2064
Summary:
- Differential, Maniphest and Diffusion use slightly different styles for the object detail panels.
- Instead, use the same styles and CSS.
- Add object actions to Diffusion, including "Flag".
Test Plan: Looked at revisions, tasks and commit. Flagged and unflagged commits.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran, epriestley
Maniphest Tasks: T1041
Differential Revision: https://secure.phabricator.com/D2062
Summary:
**Who can delete global rules?**: I discussed this with @jungejason. The current behavior is that the rule author or any administrator can delete a global rule, but this
isn't consistent with who can edit a rule (anyone) and doesn't really make much sense (it's an artifact of the global/personal split). I proposed that anyone can delete a
rule but we don't actually delete them, and log the deletion. However, when it came time to actually write the code for this I backed off a bit and continued actually
deleting the rules -- I think this does a reasonable job of balancing accountability with complexity. So the new impelmentation is:
- Personal rules can be deleted only by their owners.
- Global rules can be deleted by any user.
- All deletes are logged.
- Logs are more detailed.
- All logged actions can be viewed in aggregate.
**Minor Cleanup**
- Merged `HomeController` and `AllController`.
- Moved most queries to Query classes.
- Use AphrontFormSelectControl::renderSelectTag() where appropriate (this is a fairly recent addition).
- Use an AphrontErrorView to render the dry run notice (this didn't exist when I ported).
- Reenable some transaction code (this works again now).
- Removed the ability for admins to change rule authors (this was a little buggy, messy, and doesn't make tons of sense after the personal/global rule split).
- Rules which depend on other rules now display the right options (all global rules, all your personal rules for personal rules).
- Fix a bug in AphrontTableView where the "no data" cell would be rendered too wide if some columns are not visible.
- Allow selectFilter() in AphrontNavFilterView to be called without a 'default' argument.
Test Plan:
- Browsed, created, edited, deleted personal and gules.
- Verified generated logs.
- Did some dry runs.
- Verified transcript list and transcript details.
- Created/edited all/any rules; created/edited once/every time rules.
- Filtered admin views by users.
Reviewers: jungejason, btrahan
Reviewed By: btrahan
CC: aran, epriestley
Differential Revision: https://secure.phabricator.com/D2040
Summary:
Herald rules are adding CC also for Author and Reviewer.
See also D1397.
I was considering also just don't displaying the extra CC but this is probably better.
There are still cases where there could be reviewer in CC (e.g. by making reviewer from CC or by direct edit) but I think it's not a big problem.
Beeing both Reviewer and CC can be actually useful (e.g. if you resign than you still are in CC) but it's not that useful to justify this:
Author: vrana
Reviewers: epriestley
CCs: vrana, epriestley
Test Plan: Comment on revision where I am author.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, epriestley
Differential Revision: https://secure.phabricator.com/D2057
Summary:
I think this feature is probably good, but Differential is also really starting to get a lot of stuff which is not the diff in it. Not sure how best to deal with that.
The mixed table styles are also pretty ugly.
So I guess this is more feedback / proof-of-concept, I think I want to try to improve it somehow before I land it.
Test Plan: Looked at some diffs, some had an awkward, ugly list of diffs affecting the same files.
Reviewers: bill, aran, btrahan
Reviewed By: aran
CC: aran, epriestley
Maniphest Tasks: T829
Differential Revision: https://secure.phabricator.com/D2027
Summary:
These are the issues identified by the linter in D2052. I don't think any cause bugs, but they are all reasonable errors to raise and the linter correctly
detected that they are suspicious.
Test Plan: Mostly inspection.
Reviewers: vrana, btrahan
Reviewed By: vrana
CC: aran, epriestley
Differential Revision: https://secure.phabricator.com/D2053
Summary: We may overwrite $comment as a side effect of iteration.
Test Plan: Made some audit comments as different users.
Reviewers: vrana, btrahan
Reviewed By: vrana
CC: aran, epriestley
Differential Revision: https://secure.phabricator.com/D2050
Summary:
When there is a single line Test Plan (or anything else) then `arc amend` puts it on the same line as label.
It is a problem with indented line (as in this diff) because next run of `arc diff` will trim the leading spaces.
Test Plan: arc amend
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, epriestley
Differential Revision: https://secure.phabricator.com/D2054
Summary:
When there is a single line Test Plan (or anything else) then `arc amend` puts it on the same line as label.
It is a problem with indented line (as in this diff) because next run of `arc diff` will trim the leading spaces.
Test Plan: arc amend
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, epriestley
Differential Revision: https://secure.phabricator.com/D2054
Summary:
The full path field of the DiffusionRepositoryPath object is used by the
DiffusionBrowseController when viewing a directory with a readme file, so
we should set this field.
Test Plan: loaded a directory containing a readme in a svn repo
Reviewers: epriestley, jungejason, emiraga
Reviewed By: epriestley
CC: aran, epriestley
Differential Revision: https://secure.phabricator.com/D2045
Test Plan: Looked at /differential/ with and without flags.
Reviewers: nh, btrahan
Reviewed By: nh
CC: aran, epriestley
Maniphest Tasks: T1055
Differential Revision: https://secure.phabricator.com/D2044
Summary:
- When an inline comment preview corresponds to an inline comment on the page, link to it. Just punt in the tough case where the inline is on some other page.
- In "haunted" mode, "z" now toggles through three modes: normal, comment area only, and comment + previews.
Test Plan:
- Viewed visible and not-visible inline comment previews, clicked "View" links.
- Tapped "z" a bunch to toggle haunt modes.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran, epriestley
Maniphest Tasks: T517, T214
Differential Revision: https://secure.phabricator.com/D2041
Summary: 'cuz I miss out on chat room goodness and can't paginate around in the current version
Test Plan: setup a phabot and spammed it in phabot-test. with new test data, set $page_limit = 1 and paged about -- looks good!
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, epriestley
Maniphest Tasks: T990
Differential Revision: https://secure.phabricator.com/D2032
Summary:
Flags are a personal collection of things you want to take a look at later. You can use several different colors and add notes.
Not really sure if this is actually a good idea or not but it was easy to build.
Planned features:
- Allow Herald rules to add flags.
- In the "edit flag" dialog, have a "[x] Subscribe Me" checkbox that CCs you.
- Support Diffusion.
- Support Phriction.
- Always show flags on an object if you have them (in every view)?
- Edit dialog feels a little heavy?
- More filtering in /flag/ tool.
- Add a top-level links somewhere?
Test Plan: Added, edited and removed flags from things. Viewed flags in flag view.
Reviewers: aran, btrahan
Reviewed By: btrahan
CC: aran, epriestley, Koolvin
Maniphest Tasks: T1041
Differential Revision: https://secure.phabricator.com/D2024
Summary:
If a file was last modified at revision 50 and you look at revision 55, we currently 404. Instead, always identify the last modification.
Also simplify some of the query objects.
Test Plan: Viewed after-modification revisions for several files in SVN.
Reviewers: vrana, btrahan
Reviewed By: vrana
CC: aran, epriestley
Maniphest Tasks: T851
Differential Revision: https://secure.phabricator.com/D2028
Summary: See T955. We jump to an awkard place right now; jump above the comment instead.
Test Plan: Clicked inline comment anchor links.
Reviewers: Makinde, btrahan
Reviewed By: Makinde
CC: aran, epriestley
Maniphest Tasks: T955
Differential Revision: https://secure.phabricator.com/D2029
Summary:
- Still really really rough.
- Adds a full synchronous mode for debugging.
- Adds some logging.
- It can now allocate EC2 machines and put webroots on them in a hacky, terrible way.
- Adds a base query class.
Test Plan: oh hey look a test page? http://ec2-50-18-65-151.us-west-1.compute.amazonaws.com:2011/
Reviewers: btrahan
Reviewed By: btrahan
CC: aran, epriestley
Maniphest Tasks: T1049
Differential Revision: https://secure.phabricator.com/D2026
Summary: Show parent commit information to make it easier to understand merges.
Test Plan: Looked at commits in SVN, hg, git.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran, epriestley
Maniphest Tasks: T961
Differential Revision: https://secure.phabricator.com/D2021
Summary: COMPLETELY ORIGINAL IDEA
Test Plan: Browsed around Phabricator, got helpful readmes in some cases.
Reviewers: davidreuss, btrahan
Reviewed By: btrahan
CC: aran, epriestley
Differential Revision: https://secure.phabricator.com/D2022
Summary: This header allows recipients to distinguish between CCs generated by Herald and CCs generated by humans.
Test Plan: Created a Herald rule to add a bunch of CC's to every revision. Created a revision. Added some CCs manually. Verified that only manual CCs appeared in the "Explicit" header.
Reviewers: btrahan, vrana
Reviewed By: btrahan
CC: aran, epriestley
Maniphest Tasks: T808
Differential Revision: https://secure.phabricator.com/D2018
Summary:
See some discussion in D2002. Add two new actions:
- Resign: (auditor only) closes your open request (user request ONLY) by putting it in a "resigned" state.
- Close: (author only) closes all open requests by putting them in a "closed" state.
@davidreuss, this is probably conflict-city with D2002 -- I'll wait for you to land first and then handle the merge on my end.
Test Plan: Resigned from and closed audits.
Reviewers: 20after4, davidreuss, btrahan
Reviewed By: btrahan
CC: aran, epriestley
Maniphest Tasks: T904
Differential Revision: https://secure.phabricator.com/D2013
Summary: Adds an "allactive" filter in addition to the all projects filter.
Test Plan:
visit All Active view (/project/filter/allactive/) and
see that it lists all projects except those which have been archived.
Visit other filter views to be sure nothing else got broken.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, epriestley
Differential Revision: https://secure.phabricator.com/D2010
Summary:
$stdout from the previous run would be reused if an exception
occurred
Test Plan: that's a negative, ghostrider.
Reviewers: epriestley, jungejason
Reviewed By: epriestley
CC: aran, epriestley
Differential Revision: https://secure.phabricator.com/D2008
Summary:
When a commit is a merge, show what it merged.
Also fix some bugs:
- Mercurial queries may contain ":", but mercurial rev ranges may also contain ":". A rev range with a branch that has a ":" in it is ambigiuous, e.g. branch "a:b" might appear in a rev range like "a🅱️0", which can not be parsed. Use stable commit names instead.
- Mercurial stable commit name implementation was broken, fix it.
- Extend DiffusionHistoryQuery from DiffusionQuery to share code.
- Fix a bug where Mercurial's main browse list would not show the most recent commit if it was a merge commit.
Test Plan: Generated a bunch of mercurial/git merge commits and looked at them, they seemed to accurately represent the repository state.
Reviewers: btrahan, Makinde
Reviewed By: btrahan
CC: aran, epriestley
Maniphest Tasks: T961
Differential Revision: https://secure.phabricator.com/D2005
Summary:
- Show the canonical (i.e., shorter) commit identifier in the subject.
- For commits without a revision, put the commit summary in the subject.
Test Plan: Ran "scripts/repository/reparse.php <commit> --herald" for a number of different commits (with revision, without revision); got more useful email subjects.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran, epriestley
Maniphest Tasks: T1028
Differential Revision: https://secure.phabricator.com/D2004
Summary: $link gets reused later in the function, use a different variable name to avoid broken nonsense.
Test Plan: Clicked users/projects links.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran, epriestley
Maniphest Tasks: T1038
Differential Revision: https://secure.phabricator.com/D2003
Summary:
In the Differential revision list views:
- Allow you to filter by mailables (notably, mailing lists).
- Allow you to filter by user (including disabled users).
Test Plan: Filtered by a mailing list.
Reviewers: btrahan, nh
CC: aran, epriestley
Maniphest Tasks: T1031
Differential Revision: https://secure.phabricator.com/D1994
Summary: I'll mark this one up inline since it's all separate bugs.
Test Plan:
- Created a diff with eight changes: (newline absent -> newline present, newline present -> newline absent, newline present -> newline present, newline absent -> newline absent) x (short file with change near end, long file with change near middle).
- Viewed diff in Ignore All, Ignore Most, Ignore Trailing and Show All whitespace modes.
- All 32 results seemed sensible.
- Really wish this stuff was better factored and testable. Need to fix it. :(
Reviewers: vrana, btrahan
Reviewed By: btrahan
CC: aran, epriestley
Maniphest Tasks: T1030
Differential Revision: https://secure.phabricator.com/D1992
Summary: Apparently I spent like a good month copy/pasting slightly different versions of this logic all over the codebase.
Test Plan: Selected "View Options -> Browse in Diffusion" for a chagneset, got a URI with a branch name in it under Git.
Reviewers: vrana, btrahan
Reviewed By: btrahan
CC: aran, epriestley
Differential Revision: https://secure.phabricator.com/D1993
Summary:
- We need to sort Projects explicitly because we go through task-by-task which ruins the ordering. My test case was just small enough not to notice.
- Push "No Project" to the bottom explicitly.
- Simplify/fix the pull of "Unassigned" to the top.
- Fix a return type.
Test Plan: Tested the various sorting cases against a larger test data set.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran, epriestley
Differential Revision: https://secure.phabricator.com/D1988
Summary:
turns out both github and Phabricator fall back to if the user already has a login session when accessing the pertinent profile picture data. Facebook on the other hand is a stingy bastard about have an actual access token. Ergo, in production (once I could test Facebook) this button failed.
The patch sets the access token properly such that the provider can use it properly when retrieving the profile image.
Test Plan: re-did my meta-Phabricator test and it still passed. setup my phabricator dev instance for Facebook OAuth (created a test app and everything... :/ ) and it worked end to end.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, epriestley
Maniphest Tasks: T870
Differential Revision: https://secure.phabricator.com/D1986
Summary:
Currently, we use "git log" to detect the change list for all commits, but this produces no output for merge commits.
Instead, parse them as changes against the first parent (the merge destination). This produces generally sensible/expected behavior, and is consistent with what GitHub does.
We need to special-case the first commit because it doesn't have parents.
NOTE: This is a parser change so you need to run `./scripts/repository/reparse.php --all <callsign> --change` to reparse merge commits in already-imported repositories after updating.
Test Plan: Reparsed a merge commit, a non-merge commit, and the first commit in the Phabricator repository.
Reviewers: btrahan, gschmidt
Reviewed By: btrahan
CC: aran, epriestley
Maniphest Tasks: T961
Differential Revision: https://secure.phabricator.com/D1985
Summary: We need to build a request in order to pick up an appropriate default branch name, instead of using the raw static generator.
Test Plan: Clicked a symbol link, got /master/path/blahblah
Reviewers: vrana, btrahan
Reviewed By: vrana
CC: aran, epriestley
Differential Revision: https://secure.phabricator.com/D1982
Summary:
It is now possible to set config setting requiring class of certain implementation to something completely else.
The consequence is that your Phabricator may stop working after update because you didn't implement some new method.
This diff validates the class upon usage.
It throws exception which is better than fatal thrown currently after calling undefined method.
Better solution would be to validate classes when setting the config but it would be too expensive - respective class definitions would have to be loaded and checked by reflection.
I was also thinking about some check script but nobody would run it after changing config.
The same behavior should be implemented for these settings:
- metamta.mail-adapter
- metamta.maniphest.reply-handler
- metamta.differential.reply-handler
- metamta.diffusion.reply-handler
- storage.engine-selector
- search.engine-selector
- differential.field-selector
- maniphest.custom-task-extensions-class
- aphront.default-application-configuration-class
- controller.oauth-registration
Test Plan:
Send comment, verify that it pass.
Change `metamta.differential.reply-handler` to incompatible class, verify that sending comment shows nice red exception.
Set `metamta.differential.reply-handler` to empty string, verify that it throws.
Reviewers: epriestley, btrahan
Reviewed By: epriestley
CC: aran, epriestley
Differential Revision: https://secure.phabricator.com/D1919
Summary:
- Feature request from Airtime that I missed in the feedback notes, came up yesterday.
- Identify git submodules as "FILE_SUBMODULE", not "FILE_NORMAL".
- Link git submodules to an external resolver endpoint, which tries to find commits in tracked repositories.
- Identify git symlinks as "FILE_SYMLINK", not "FILE_NORMAL".
- Add folder, file, symlink and externals icons.
Test Plan:
- externals/javelin is now identified as a submoudule and links to Javelin, not identified as a file and links to error.
- bin/phd is now identified as a symlink.
- Interfaces have pretty icons.
Reviewers: btrahan, cpiro, ddfisher, keebuhm, allenjohnashton
Reviewed By: btrahan
CC: aran, epriestley
Differential Revision: https://secure.phabricator.com/D1975
Summary: Add @20after4's new rule to the Phabricator engine.
Test Plan: Wrote some ~~deleted~~ text.
Reviewers: 20after4, btrahan
Reviewed By: btrahan
CC: aran, epriestley
Differential Revision: https://secure.phabricator.com/D1972
Summary: I think these are all the actions which make any sense.
Test Plan:
- Performed and verified each action through the batch editor.
- Performed a large batch edit which applied each action type multiple times and verified the aggregate behavior was correct.
Reviewers: btrahan, cpiro
Reviewed By: btrahan
CC: aran, epriestley
Maniphest Tasks: T441
Differential Revision: https://secure.phabricator.com/D1971
Test Plan:
Search for some symbol. Click on the result. Verify that there is not // in URL.
Click on the link from generated exception.
View history in Diffusion, click on Browse.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, epriestley
Differential Revision: https://secure.phabricator.com/D1979
Test Plan: Clicked on `Exception` in `throw new Exception`.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, epriestley
Differential Revision: https://secure.phabricator.com/D1978
Summary:
A bug was introduced in
rP30ae22bfcff71fa3f14e38fd3cd597448df501c3 which caused commits to start
being parsed as if the repository callsign was empty. This caused errors
and problems and much unhappiness. Example error:
EXCEPTION: (Exception) No such repository ''. at [.../phabricator/src/applications/diffusion/request/base/DiffusionRequest.php:130]
Test Plan: Saw that commit parsing was breaking. Applied fix. Saw that commits parsed again.
Reviewers: epriestley
CC: aran, epriestley
Differential Revision: https://secure.phabricator.com/D1974
Summary:
Aggregate multiple add/remove transactions so we don't restore removed projects for a (remove + add) batch edit.
(Possibly we should do this in the TransactionEditor as well / instead, but it's fairly easy here and this is the only possible case currently.)
Test Plan: Performed a remove + add batch edit without issues.
Reviewers: btrahan
Reviewed By: btrahan
CC: cpiro, aran, epriestley
Maniphest Tasks: T985
Differential Revision: https://secure.phabricator.com/D1967
Summary:
1. Created a filter for user comments that are not submitted yet
2. surface this information to the user by providing a "Draft revisions" link on the differential home page.
Test Plan: tested on one of the diffs
Reviewers: nh, epriestley
Reviewed By: epriestley
CC: aran, epriestley, jungejason
Differential Revision: https://secure.phabricator.com/D1927
Summary:
Depends on D1921. Depends on D1899.
Add the "View Options" dropdown menu to Diffusion, with options like "show standalone", "show raw file", "show all", etc.
Test Plan: Viewed commits in Differential and Diffusion.
Reviewers: davidreuss, nh, btrahan
Reviewed By: btrahan
CC: aran, epriestley
Maniphest Tasks: T904
Differential Revision: https://secure.phabricator.com/D1932
Summary:
Depends on D1929. In emails, notify recipients that inlines are attached.
Vaguely copy/pastey from Differential but they only share like six lines and this seems like a random piece of code to pull out.
Test Plan: Added inline comments, got email mentioning them
Reviewers: davidreuss, nh, btrahan
Reviewed By: davidreuss
CC: aran, epriestley
Maniphest Tasks: T904
Differential Revision: https://secure.phabricator.com/D1930
Summary: Depends on D1928. Uses the new UI element to display inlines in Diffusion.
Test Plan: Looked at a commit with inline comments, saw them in the summaries.
Reviewers: davidreuss, nh, btrahan
Reviewed By: davidreuss
CC: aran, epriestley
Maniphest Tasks: T904
Differential Revision: https://secure.phabricator.com/D1929
Summary:
This is in preparation for getting the "View Options" dropdown working on audits.
- Use Files to serve raw data so we get all the security benefits of the alternate file domain. Although the difficulty of exploiting this is high (you need commit access to the repo) there's no reason to leave it dangling.
- Add a "contentHash" to Files so we can lookup files by content rather than adding some weird linker table. We can do other things with this later, potentially.
- Don't use 'data' URIs since they're crazy and we can just link to the file URI.
- When showing a binary file or an image, don't give options like "show highlighted text with blame" or "edit in external editor" since they don't make any sense.
- Use the existing infrastructure to figure out if things are images or binaries instead of an ad-hoc thing in this class.
Test Plan: Looked at text, image and binary files in Diffusion. Verified we reuse existing files if we've already generated them.
Reviewers: btrahan, vrana
Reviewed By: btrahan
CC: aran, epriestley
Maniphest Tasks: T904
Differential Revision: https://secure.phabricator.com/D1899
Summary:
Diffusion request/uri handling is currently a big, hastily ported mess. In particular, it has:
- Tons and tons of duplicated code.
- Bugs with handling unusual branch and file names.
- An excessively large (and yet insufficiently expressive) API on DiffusionRequest, including a nonsensical concrete base class.
- Other tools were doing hacky things like passing ":" branch names.
This diff attempts to fix these issues.
- Make the base class abstract (it was concrete ONLY for "/diffusion/").
- Move all URI generation to DiffusionRequest. Make the core static. Add unit tests.
- Delete the 300 copies of URI generation code throughout Diffusion.
- Move all URI parsing to DiffusionRequest. Make the core static. Add unit tests.
- Add an appropriate static initializer for other callers.
- Convert all code calling `newFromAphrontRequestDictionary` outside of Diffusion to the new `newFromDictionary` API.
- Refactor static initializers to be sensibly-sized.
- Refactor derived DiffusionRequest classes to remove duplicated code.
- Properly encode branch names (fixes branches with "/", see <https://github.com/facebook/phabricator/issues/100>).
- Properly encode path names (fixes issues in D1742).
- Properly escape delimiter characters ";" and "$" in path names so files like "$100" are not interpreted as "line 100".
- Fix a couple warnings.
- Fix a couple lint issues.
- Fix a bug where we would not parse filenames with spaces in them correctly in the Git browse query.
- Fix a bug where Git change queries would fail unnecessarily.
- Provide or improve some documentation.
This thing is pretty gigantic but also kind of hard to split up. If it's unreasonably difficult to review, let me know and I can take a stab at it though.
This supplants D1742.
Test Plan:
- Used home, repository, branch, browse, change, history, diff (ajax), lastmodified (ajax) views of Diffusion.
- Used Owners typeaheads and search.
- Used diffusion.getrecentcommitsbypath method.
- Pushed a change to an absurdly-named file on an absurdly-named branch, everything worked properly.
{F9185}
Reviewers: nh, vrana, btrahan
Reviewed By: btrahan
CC: aran, epriestley
Differential Revision: https://secure.phabricator.com/D1921
Summary: Allow AphrontTableView to render with sort indicators and links in its columns.
Test Plan: Looked at UI example.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran, epriestley, sandra
Maniphest Tasks: T994
Differential Revision: https://secure.phabricator.com/D1946
Summary:
Allow tasks to be grouped by project. Since this is many-to-many and we're a little deficient on indexes for doing this on the database, we pull all matching tasks and group them in PHP. This shouldn't be a huge issue for any existing installs, though, and we can add keys when we run into one.
- When a task is in multiple projects, it appears under multiple headers.
- When a query has a task filter, those projects are omitted from the grouping (they'd always show everything, which isn't useful). Notably, if you search for "Differential", you can now see "Bugs", "Feature Requests", etc.
Test Plan: Selected "Group by: Project".
Reviewers: btrahan, Josereyes
Reviewed By: btrahan
CC: aran, epriestley
Maniphest Tasks: T923
Differential Revision: https://secure.phabricator.com/D1953
Summary: Part of a user feature request, see T994.
Test Plan: Looked at data in columns, seemed to line up with reality.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran, epriestley, sandra
Maniphest Tasks: T994
Differential Revision: https://secure.phabricator.com/D1944
Summary:
- Affects the "Inline Comments" summary table which appears in comments that have attached inlines in the discussion threads in Differential.
- Prepares for inclusion in Diffusion.
- No application changes (minor CSS), just factors code better.
- Simplify/separate CSS.
Test Plan: Looked at on-diff and off-diff comment summaries in Differential, display looked correct.
Reviewers: davidreuss, nh, btrahan
Reviewed By: davidreuss
CC: aran, epriestley
Maniphest Tasks: T904
Differential Revision: https://secure.phabricator.com/D1928
Summary:
See <https://github.com/facebook/phabricator/issues/102>. Commit data may not be available for unpared commits, but we'll raise a warning about $commit_data in that case (the UI correctly handles missing $commit_data).
Also some minor cleanup / UI fixes.
Test Plan: Browsed around hg / git repos, including unparsed commits.
Reviewers: btrahan, killermonk
Reviewed By: btrahan
CC: aran, epriestley
Differential Revision: https://secure.phabricator.com/D1961
Summary:
See <https://github.com/facebook/phabricator/issues/102>. Between Feb 1 and Mar 1, the hg released changed the exit code behavior of "hg pull". This broke us mildly (and a bunch of other applications more severely, which is why it was reverted).
Detect the common case of this (english) and don't fail.
Test Plan: @killermonk, can you try applying this? I'll try to do an upgrade to 2.1 and see if I can also do a proper test.
Reviewers: Makinde, btrahan, killermonk
Reviewed By: btrahan
CC: killermonk, aran, epriestley
Differential Revision: https://secure.phabricator.com/D1948
Summary:
- We incorrectly count resolution changes and other noise as opens / closes.
- Show one graph: open bugs over time (red line minus green line). This and its derivative are the values you actually care about. It is difficult to see the derivative with both lines, but easy with one line.
Test Plan: Looked at burnup chart. Saw charty things. Verified resolution changes no longer make the line move.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran, epriestley
Maniphest Tasks: T923, T982
Differential Revision: https://secure.phabricator.com/D1945
Summary:
Split from D1921.
- DifferentialChangesetParser doesn't have this property declared.
- We weren't providing a markup engine, which caused some warnings.
- This would cause failures if comment caches weren't present. Currently, they always will be though unless someone has wiped them explicitly in the DB.
Test Plan: Viewed a diff with inline comments, didn't get any warnings in the log.
Reviewers: nh, vrana, btrahan
Reviewed By: btrahan
CC: aran, epriestley
Differential Revision: https://secure.phabricator.com/D1925
Summary:
Split from D1921. We'll explode each line into too many parts currently, if the filename contains spaces.
Also use -z to get \0 newlines.
Test Plan: Browsed a directory containing files with spaces in their names, links etc were correct.
Reviewers: nh, vrana, btrahan
Reviewed By: btrahan
CC: aran, epriestley
Differential Revision: https://secure.phabricator.com/D1924
Summary:
In D1926 this got converted to use the new InsetView, but we lost the 'id="resources"' on the hidden input which is required by the JS.
Just use phutil_render_tag() to make sure the `id` shows up.
Test Plan: Edited a project without bumping into an exception.
Reviewers: hsb, btrahan
Reviewed By: hsb
CC: aran, epriestley
Differential Revision: https://secure.phabricator.com/D1957
Summary:
If the 'Summary' is not present and not inferred to be empty from a newline after the title with no explicit 'Summary' field, we'll copy all the field values from the revision (including NULL), not overwrite the 'Summary' value from the message (since it's not present) and then write the NULL back to the revision.
Instead, string cast the read from the Revision so we write back empty string in the not-provided case.
Test Plan: Ran "arc diff --create --use-commit-message HEAD" with P336 in HEAD, didn't fail (previously, it failed).
Reviewers: btrahan, 20after4
Reviewed By: 20after4
CC: aran, epriestley
Maniphest Tasks: T1019
Differential Revision: https://secure.phabricator.com/D1956
Summary: This diff is really complicated, only a master programmer like myself could have accomplished it.
Test Plan: derrrrrp
Reviewers: davidreuss, nh, btrahan, jungejason
Reviewed By: jungejason
CC: aran, epriestley
Maniphest Tasks: T904
Differential Revision: https://secure.phabricator.com/D1936
Summary: Works in Safari, Firefox, Chrome.
Test Plan: Copied some text, threw up a little in my mouth.
Reviewers: aran, tuomaspelkonen, tomo, rstout, btrahan
Reviewed By: aran
CC: aran, epriestley, ddfisher
Maniphest Tasks: T145, T995
Differential Revision: https://secure.phabricator.com/D244
Summary:
T937 suggests 'inset' could have its own view controller.
It has the following methods:
- setTitle for title
- setRightbutton if you have to place something (preferably a button)
on the right side of the form
- setDescription if you want to describe what it does
- setContent for the main content
- addDivAttributes REALLY not sure about this one but it had to be included
because of a single controller (see owners/controller/edit/PhabricatorOwnersEditController.php:238)
- appendChild works as usual if your form is complex but you still want to remove
->appendChild('<div class..') ->appendChild('</div>');
It might be an overkill so maybe some could be dropped:
- addDivAttributes() and just rewrite how PhabricatorOwnersEditController.php works
- setContent() and use appendChild for the main content?
Test Plan:
- Looked at the controllers in phabricator
- Changed the controller
- Opened the page in another tab
- If something didnd't look the same I fixed it.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, epriestley
Differential Revision: https://secure.phabricator.com/D1926
Test Plan:
type /a<enter> - should jump to audits
type /f<enter> - should jump to feed
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, epriestley
Differential Revision: https://secure.phabricator.com/D1823
Summary: Facebook extends this, just unfinal it for now and we can figure out the right long-term fix at our leisure.
Test Plan: Inspection.
Reviewers: vrana, btrahan
Reviewed By: vrana
CC: aran, epriestley
Differential Revision: https://secure.phabricator.com/D1917
Summary:
It is currently not possible to select source code covered by reticle when creating comment.
This diff hides reticle on mouseout from reply area.
Test Plan:
Hover inline comment, verify that reticle is displayed.
Reply, verify that reticle is displayed when mouseover reply, hidden otherwise.
Repeat for create.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, epriestley
Differential Revision: https://secure.phabricator.com/D1851
Summary: In very large diffs.
Test Plan:
Display very large diff.
Display normal diff.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, epriestley
Differential Revision: https://secure.phabricator.com/D1918
Summary:
It should be possible to write `D1234` and others without making links from them.
Depends on D1913.
Test Plan:
`D1234`
`<a href="">`
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, epriestley
Differential Revision: https://secure.phabricator.com/D1915
Summary:
If we can't look up the name for a mailing list, don't put 'Unknown Mailing
List' in the commit message - it will confuse things later down the road.
Surely there is a better way of doing this than checking the name of the
handle for the mailing list.
Test Plan:
called differential.getcommitmessage on a revision that had an invalid mailing
list phid.
Reviewers: epriestley, schrockn, jungejason
Reviewed By: epriestley
CC: aran, epriestley
Differential Revision: https://secure.phabricator.com/D1910
Summary: I've done the same stupid copy/paste mistake twice in one day :-(.
Test Plan: Display diff with no newline at end of file in new file.
Reviewers: davidreuss, epriestley
Reviewed By: davidreuss
CC: aran, epriestley
Differential Revision: https://secure.phabricator.com/D1911
Summary:
Resolves T989
- users can now disable the '/' keyboard shortcut which focuses the
search box
- users can now disable the jump nav functionality of the search box
Test Plan:
- verified that the '/' keyboard shortcut works with preference enabled
or unset
- verified that '/' no longer has any effect and disappears from
keyboard shortcuts help overlay with preference disabled
- verified that search boxes have jump nav capabilities with jump nav
functionality preference unset or enabled
- verified that search boxes do not jump with jump nav preference
disabled
- verified that the jump nav still works as a jump nav with jump nav
preference disabled
Reviewers: epriestley
Reviewed By: epriestley
CC: simpkins, aran, epriestley, vrana
Maniphest Tasks: T989
Differential Revision: https://secure.phabricator.com/D1902
Summary: Text "No newline at end of file" was sent to highlighter causing error with most languages.
Test Plan: Display diff containing file with no newline at end of file.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, epriestley
Differential Revision: https://secure.phabricator.com/D1903
Summary:
- Adds "User Projects" filter to Maniphest.
- The filter expands the user(s) specified into a set of projects and issues a query on that project set.
- "View All Triage" button in Tactical Command's Needs Triage panel now points to User Projects-Need Triage filter.
Test Plan: - User Projects filter correctly displays only the tasks in projects the specified users are involved in.
Reviewers: epriestley
Reviewed By: epriestley
CC: keebuhm, ddfisher, allenjohnashton, epriestley, aran
Differential Revision: https://secure.phabricator.com/D1901
Summary:
It looks like there is really this text written e.g. at https://secure.phabricator.com/D1896#0a6a1957
I am not sure that it is the only place which needs to be fixed.
Test Plan: Display diff with no newline at end of file in Differential.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, epriestley
Differential Revision: https://secure.phabricator.com/D1900
Summary:
- Add inline comments to Audits, like Differential.
- Creates new storage for the comments in the Audits database.
- Creates a new PhabricatorAuditInlineComment class, similar to DifferentialInlineComment.
- Defines an Interface which Differential and Audit comments conform to.
- Makes consumers of DifferentialInlineComments consume objects which implement that interface instead.
- Adds save
NOTE: Some features are still missing! Wanted to cut this off before it got crazy:
- Inline comments aren't shown in the main comment list.
- Inline comments aren't shown in the emails.
- Inline comments aren't previewed.
I'll followup with those but this was getting pretty big.
@vrana, does the SQL change look correct?
Test Plan:
- Created, edited, deleted, replied to, reloaded and saved inline comments in Diffusion, on the left and right side of diffs.
- Created, edited, deleted, replied to, reloaded and saved inline comments in Differentila, on the left and right side of primary and diff-versus-diff diffs.
Reviewers: btrahan, vrana
Reviewed By: btrahan
CC: aran, epriestley
Maniphest Tasks: T904
Differential Revision: https://secure.phabricator.com/D1898
Summary:
We give you a pretty bad error right now if your server doesn't have, say, png support, saying "only png is supportd loololloo".
Instead, show you which formats are supported in the error messsage, and tell you upfront.
Test Plan: Tried to upload supported and unsupported images, got appropriate errors and supported format text.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran, epriestley
Maniphest Tasks: T981
Differential Revision: https://secure.phabricator.com/D1894
Summary:
It is misplaced from the beginning but it got worse after some CSS tweaks.
I was also thinking about using the same DropdownMenu as in Differential but I don't feel strongly about it to do it myself.
Test Plan:
Display file in Diffusion.
Repeat with disabled Editor.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, epriestley
Differential Revision: https://secure.phabricator.com/D1892
Summary: I missed this URI when grepping for old links.
Test Plan: Clicked the link, didn't 404.
Reviewers: davidreuss, btrahan
Reviewed By: davidreuss
CC: aran, epriestley
Differential Revision: https://secure.phabricator.com/D1895
Summary: Last of the big final patches. Left a few debatable classes (12 out of about 400) that I'll deal with individually eventually.
Test Plan: Ran testEverythingImplemented.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran, epriestley
Maniphest Tasks: T795
Differential Revision: https://secure.phabricator.com/D1881
Summary: svn cat returns a non-zero exit status when trying to cat a directory.
Test Plan: Browsed diffusion in my sandbox
Reviewers: epriestley, jungejason
Reviewed By: epriestley
CC: aran, epriestley
Differential Revision: https://secure.phabricator.com/D1882
Summary: No big surprises here, delted the unused "DarkConsole" class.
Test Plan: Ran 'testEverythingImplemented' to verify I wasn't finalizing anything we extend.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran, epriestley
Maniphest Tasks: T795
Differential Revision: https://secure.phabricator.com/D1876
Summary:
- Order audits by id desc so they tend to be in descending time order, like other content.
- In audit tables without commit context (audit tool, home page) show commit descriptions.
- Correctly hide pagers on "active" audit filter.
- Make pagers work correctly on commit / audit views.
Test Plan: Looked at homepage, audit, owners, differential. Paginated relevant interfaces.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran, epriestley
Maniphest Tasks: T904
Differential Revision: https://secure.phabricator.com/D1875
Summary: Remove these from typeaheads, since "archive" basically means "delete".
Test Plan: Tried to typeahead an archived project.
Reviewers: btrahan, skrul
Reviewed By: skrul
CC: aran, epriestley
Differential Revision: https://secure.phabricator.com/D1872
Summary:
- Show a tip in the margin about what coverage colors mean.
- Highlight the line when mousing over coverage.
- Randomly change the colors to different colors.
- Fix a bug with "show more" that I introduced with the other coverage diff (oops!)
Test Plan: Moused over coverage things.
Reviewers: tuomaspelkonen, btrahan
Reviewed By: tuomaspelkonen
CC: aran, epriestley
Maniphest Tasks: T965
Differential Revision: https://secure.phabricator.com/D1874
Summary: There are a few things we can improve with tooltips.
Test Plan: Moused over all the stuff on the test page.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran, epriestley
Maniphest Tasks: T965
Differential Revision: https://secure.phabricator.com/D1870
Summary:
Currently, we mark some lines that otherwise count as "unchanged" to be "changed" when they have internal whitespace changes.
However, we've already excluded them from the intra-line diff algorithm. Unset the flag so they can get intra-line diffed.
Test Plan: Viewed a change with internal whitespace changes, internal whitespace changes were intraline diffed.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran, epriestley
Maniphest Tasks: T970
Differential Revision: https://secure.phabricator.com/D1871
Summary:
We used to have "ignore all", but it became "ignore most". It does not ignore in-line whitespace changes or whitespace changes in language where whitespace is marked semantic.
Add an explicit "ignore all" option.
Test Plan: Used "ignore all" to view a change with heuristically-detected semantic whitespace, the change was ignored.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran, epriestley
Maniphest Tasks: T970
Differential Revision: https://secure.phabricator.com/D1865
Summary:
svn 1.7 changed their xml format slightly, they now have a
##<?xml version="1.0" encoding="UTF-8"?>## tag instead of
##<?xml version="1.0"?>##. This relaxes matching this tag.
Test Plan: ./scripts/repository/reparse.php rE521979 --change
Reviewers: epriestley, jungejason
Reviewed By: epriestley
CC: aran, epriestley
Differential Revision: https://secure.phabricator.com/D1866
Summary: We've hit a couple of these in the wild, raise better error messages when the local repo is toast / broken / nonsense.
Test Plan: Broke my local repo in all of the different ways we test for, verified I got an error message in each case.
Reviewers: btrahan, abirchall
Reviewed By: btrahan
CC: aran, epriestley
Maniphest Tasks: T964, T924
Differential Revision: https://secure.phabricator.com/D1855
Summary:
These are all unambiguously unextensible. Issues I hit:
- Maniphest Change/Diff controllers, just consolidated them.
- Some search controllers incorrectly extend from "Search" but should extend from "SearchBase". This has no runtime effects.
- D1836 introduced a closure, which we don't handle correctly (somewhat on purpose; we target PHP 5.2). See T962.
Test Plan: Ran "testEverythingImplemented" unit test to identify classes extending from `final` classes. Resolved issues.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran, epriestley
Maniphest Tasks: T795
Differential Revision: https://secure.phabricator.com/D1843
Summary: We now support an optional coverage parameter.
Test Plan: ??? iiam
Reviewers: tuomaspelkonen, btrahan
Reviewed By: tuomaspelkonen
CC: aran, epriestley
Differential Revision: https://secure.phabricator.com/D1850
Summary:
Adds a macro handler that spams your channel with macros. Config is:
- macro.size: scale macros to this size before rasterizing
- macro.sleep: sleep this many seconds between lines (evade flood protection)
Test Plan: derpderp
Reviewers: kdeggelman, btrahan
Reviewed By: btrahan
CC: aran, epriestley
Differential Revision: https://secure.phabricator.com/D1838
Summary:
I'm on a host where I have the PhabricatorRepositoryPullLocalDaemons
tracking a remote repo. In my case, these end up as local git repos in
/var/repo/$name.
I'm working on another daemon that is going to automatically make
changes and commit them back upstream. I figured it would be best to do
this in a new local repo. I'll put these in /var/repo-clones/$name.
It's nice to use the exec*() functions in PhabricatorRepository, so the
approach I thought of was to load the PhabricatorRepository object from
the database, then change its localPath to point at the
/var/repo-clones/$name directory instead.
I didn't really want to change the local-path detail with setDetail(),
as that risks committing the change upstream. It's nice to use the
repo's execLocalCommand() methods though, hence wanting to change the
local path.
Test Plan: None yet.
Reviewers: epriestley
CC: aran, epriestley
Differential Revision: https://secure.phabricator.com/D1821
Summary: We'll keep deleted branches around right now because Git's behavior is not to remove them without --prune.
Test Plan: Ran "git fetch --all --prune" to make sure it at least ostensibly works.
Reviewers: btrahan, 20after4
Reviewed By: 20after4
CC: aran, epriestley
Differential Revision: https://secure.phabricator.com/D1833
Summary: New implicit fallthrough linter detected a few issues; none of these have behavioral impacts but they can clearly be tightened up. See D1824.
Test Plan: Lint; inspection.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran, epriestley
Differential Revision: https://secure.phabricator.com/D1825
Summary:
More complicated updates deserves title and explanation.
This diff uses only the first line from comment in diff's description.
It also removes the truncating at 80 chars which looks as an error.
Test Plan:
var_dump(preg_replace('/\n.*/s', '', "abc"));
var_dump(preg_replace('/\n.*/s', '', "abc\ndef"));
var_dump(preg_replace('/\n.*/s', '', "abc\ndef"));
var_dump(preg_replace('/\n.*/s', '', "abc\ndef\nghi"));
var_dump(preg_replace('/\n.*/s', '', "\nabc")); // empty string
Display revision with 255 chars description in update history - it looks OK.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, epriestley
Differential Revision: https://secure.phabricator.com/D1812
Summary: We currently parse these as directory changes and discard them. Instead, parse them as a new "SUBMODULE" type of change.
Test Plan:
- Reparsed a commit which changes submodules and verified it parses correctly.
- Reparsed a commit which adds submodules and verified it parses correctly.
Reviewers: btrahan, kdeggelman
Reviewed By: btrahan
CC: aran, epriestley
Differential Revision: https://secure.phabricator.com/D1815
Summary:
They end up in "CCs:" fields where they can't be parsed.
Not bothering to migrate since I think only Dropbox has hit this.
Also improved another error condition's handling.
Test Plan: Tried to save a mailing list with spaces and commas in the name.
Reviewers: btrahan, Makinde
Reviewed By: btrahan
CC: aran, epriestley
Maniphest Tasks: T947
Differential Revision: https://secure.phabricator.com/D1813
Summary:
Currently, we sort all results alphabetically. This isn't ideal. Instead, sort them like this:
- If the viewing user appears in the list, always sort them first. This is common in a lot of contexts and some "Ben Evans" guy is sorting first on secure.phabricator.com and causing me no end of aggravation.
- If the tokens match a "priority" component (e.g., username), sort that before results which do not have a "priority" match.
- Within a group (self, priority, everything else) sort tokens alphabetically.
NOTE: I need to go add setUser() to all the tokenizers to make the "self" rule work, but that's trivial so I figured I'd get this out first.
Test Plan:
https://secure.phabricator.com/file/data/4s2a72l5hhyyqqkq4bnd/PHID-FILE-x2r6ubk7s7dz54kxmtwx/Screen_Shot_2012-03-07_at_9.18.03_AM.png
Previously, "aaaaaepriestley" (first alphabetic match) would sort before "epriestley" (the viewing user). Now, "epriestley" sorts first because that is the viewer.
https://secure.phabricator.com/file/data/rmnxgnafz42f23fsjwui/PHID-FILE-yrnn55jl3ysbntldq3af/Screen_Shot_2012-03-07_at_9.18.09_AM.png
Previously, "aaaagopher" (first alphabetic match) would sort before "banana" (the "priority" match). Now, "banana" sorts first because it priority matches on username.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran, epriestley
Maniphest Tasks: T946
Differential Revision: https://secure.phabricator.com/D1807
Summary:
- Enforce proper workflow rules.
- Fix a derp-bug with patches.
Test Plan:
- Tried to mark a revision I didn't own.
- Tried to mark a revision already marked committed.
- Tried to mark a revision otherwise not accepted.
- Verified daemon can override workflow rules and mark from arbitrary states.
Reviewers: btrahan, Makinde
Reviewed By: Makinde
CC: aran, epriestley
Maniphest Tasks: T948
Differential Revision: https://secure.phabricator.com/D1809
Summary: When a revision is accepted, allow users to manually mark it committed if there's a daemon/tracking problem. This shouldn't happen in most installs but we have less-robust support for Mercurial and some installs may not be fully configured.
Test Plan: Marked a revision committed.
Reviewers: btrahan, Makinde
Reviewed By: Makinde
CC: aran, epriestley
Maniphest Tasks: T948
Differential Revision: https://secure.phabricator.com/D1808
Summary: Provide a reasonable JS API for the Aphlict client. Provide an example behavior to invoke it.
Test Plan:
Ran "aphlict_server.js" with:
$ sudo node aphlict_server.js
Loaded /aphlict/. Opened console. Got "hello" from the server every second.
Got reasonable errors with the server not present ("Security exception", but this is because it can't connect to port 843 to access the policy server).
Reviewers: ddfisher, keebuhm, allenjohnashton, btrahan
Reviewed By: btrahan
CC: aran, epriestley
Maniphest Tasks: T944
Differential Revision: https://secure.phabricator.com/D1800
Summary:
- Use a join to effect this query.
- Fixes a bug where packages with no commits would raise an exception because of the awkward query construction.
- Fixes a bug on audit views.
Test Plan:
- Viewed a package with no commits.
- Altered audit filters.
Reviewers: btrahan, 20after4
Reviewed By: 20after4
CC: aran, epriestley
Maniphest Tasks: T904
Differential Revision: https://secure.phabricator.com/D1801
Summary: Conduit access for open audits.
Test Plan: Used test console to run some queries.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran, epriestley
Maniphest Tasks: T904
Differential Revision: https://secure.phabricator.com/D1792
Summary:
Adds an optional "Auditors" field (like "Reviewers") to commit messages which gives installs a zero-config method for making audit requests.
This field does not appear on templates unless set, and is mostly ignored (but validated and preserved) by Differential.
It is then parsed by the daemons if present, and audit requests are pushed to valid users.
Test Plan: Made an "Auditors" commit and verified it was retained with "arc amend --show". Pushed it and verified the audit was triggered.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran, epriestley
Maniphest Tasks: T904, T880
Differential Revision: https://secure.phabricator.com/D1793
Summary: We render these in a realtively unreadable way right now; allow customization and provide reasonable defaults.
Test Plan: Looked at some tasks with custom fields on them.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran, epriestley
Maniphest Tasks: T930
Differential Revision: https://secure.phabricator.com/D1790
Summary:
Caption Submit is quite confusing because it doesn't actually make the data visible to other users.
Especially when comment confirm button is also called Submit in serious mode.
Test Plan: Add inline comment.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, epriestley
Differential Revision: https://secure.phabricator.com/D1796
Summary:
- all search boxes are now jump navs (old functionality retained if none
of the jump nav patterns match)
- added global keyboard shortcut '/' to focus the search box in the upper
right
Test Plan:
- pressed '/' and noticed the search box gains keyboard focus
- triggered jump nav functionality from search box and saw it worked
- did a search which did not match a jump nav pattern and saw it worked
(and searched in the selected context)
NOTE: The search box on the /search/ page is also changed to have jump
nav functionality. Old functionality is not impared. Still, this may not
be desirable.
Reviewers: epriestley, btrahan
Reviewed By: epriestley
CC: aran, epriestley
Differential Revision: https://secure.phabricator.com/D1794
Summary:
- Remove "0.5%" padding which makes Safari flip out and render every row differently sometimes.
- Remove list padding from ManiphestTaskListView, put it in the controller composition instead.
Test Plan: Viewed all places where task lists appear.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran, epriestley
Differential Revision: https://secure.phabricator.com/D1788
Summary:
Filter Revisions button currently resets Status and Order fields.
I've rewritten it to GET form because it doesn't perform any action.
It fixed the problem along the way.
Test Plan:
/differential/filter/revisions/
Status: Open.
Filter Revisions.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, epriestley
Differential Revision: https://secure.phabricator.com/D1771
Summary:
- Move table to Repository, since we have no Owners joins in the application anymore but would like to do a Repository join.
- Rename "packagePHID" to "auditorPHID", since this column may contain package, project, or user PHIDs.
Test Plan:
- Browsed Owners, Audit, and Differential interfaces to the Audit tool.
- Made comments and state changes.
- Ran "reparse.php --herald --owners" on several commits.
Reviewers: btrahan, jungejason
Reviewed By: btrahan
CC: aran, epriestley, nh, vrana
Maniphest Tasks: T904
Differential Revision: https://secure.phabricator.com/D1787
Summary:
Show the retry count in the meta mta view (in addition to the list of
messages) - I find this info useful when I'm trying to debug what's going on
with mail failures.
Task ID: #
Blame Rev:
Test Plan:
loaded /mail/view/NNNNN/ and saw the retry count
Revert Plan:
Tags:
Reviewers: epriestley, btrahan, jungejason
Reviewed By: epriestley
CC: aran, epriestley
Differential Revision: https://secure.phabricator.com/D1782
Summary:
Added a query option of status for the MetaMTA list controller. There currently
isn't a ui for accessing this.
Task ID: #
Blame Rev:
Test Plan:
loaded /mail/, /mail/?status=queued, /mail?phid=PHID...&status=...
each request returned a sane list of data
Revert Plan:
Tags:
Reviewers: epriestley, btrahan, jungejason
Reviewed By: epriestley
CC: aran, epriestley
Differential Revision: https://secure.phabricator.com/D1784
Summary:
Displaying reviewer who was by coincidence listed first is quite confusing, especially for committed revisions.
This displays the one who really reviewed the revision if available.
This implementation is pretty bad from performance perspective - O(N) queries to retrieve all comments.
The page load still feels quite fast.
Test Plan: /differential/filter/revisions/
Reviewers: epriestley
CC: aran, epriestley
Differential Revision: https://secure.phabricator.com/D1772
Summary:
- Owners has "by user" commit views, but these are supplanted by the Audit views. Just nuke them.
- Owners has "by package" commit views; consolidate these onto the package detail pages and link into Audit for full details.
Test Plan: Browsed all the Owners interfaces, clicked "View All ... Commits" buttons.
Reviewers: btrahan, jungejason
Reviewed By: btrahan
CC: aran, epriestley
Maniphest Tasks: T904
Differential Revision: https://secure.phabricator.com/D1764
Summary:
- Users may elect to receive an initial notification about a commit; allow it to be replied to in order to interact with the object.
- Share thread headers between emails.
- Add the "REPLY HANDLER ACTIONS" section to both emails.
Test Plan:
- Used "reparse.php --herald" to trigger herald emails, verified reply-to and email body.
- Made audit comments, verified body.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran, epriestley
Maniphest Tasks: T904
Differential Revision: https://secure.phabricator.com/D1762
Summary:
We already generate patches, but currently attach them. Allow them to be inlined instead (optionally, up to a certain size).
Also allow selection between unified and git patches.
Test Plan: Set these options in my local config, sent out a diff.
Reviewers: btrahan, Makinde
Reviewed By: Makinde
CC: aran, epriestley
Maniphest Tasks: T874
Differential Revision: https://secure.phabricator.com/D1759
Summary:
I use a smart editor which wraps words by itself so that I don't need them to be
wrapped by actual newlines.
Curent state disallows me adding or removing words later without uglying the
formatting.
Also the wordwrapped message looks ugly in Phabricator.
I am not sure how the commit message would look like on other places (such as
GitHub) but all reasonable tools should be able to wrap the text by itself.
Test Plan: arc amend --show # on a diff with long lines
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, epriestley
Differential Revision: https://secure.phabricator.com/D1755
Summary:
This does two things:
- don't fill '-' for columns where there are data from previous week
- completele hide columns if there are now new data so that noobs don't have a
55 years of history
Test Plan:
View my commits stats.
View my requested changes stats.
View epriestley FB stats.
Reviewers: vii, epriestley
Reviewed By: epriestley
CC: aran, epriestley
Differential Revision: https://secure.phabricator.com/D1750
Summary:
This will allow sending mail to be done by task workers. See T750.
Task ID: #
Blame Rev:
Test Plan:
- started taskmaster daemon in test env
- used "send new test message" feature in MetMTA (with send now unchecked)
- confirmed receipt of 1 email
- repeated 2 & 3 with send now checked
Revert Plan:
Tags:
Reviewers: epriestley, jungejason
Reviewed By: epriestley
CC: aran, epriestley
Maniphest Tasks: T388, T750
Differential Revision: https://secure.phabricator.com/D1723
Summary: add a big ole HELP tab and make "scope" link to the specific
sub-section about scope
Test Plan:
read my doc a few times, it basically english
verified links looked correct and should work right once this is all in
production
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, epriestley
Maniphest Tasks: T910
Differential Revision: https://secure.phabricator.com/D1752
Summary: Sandra had trouble opening the Spreadsheet_Excel_Writer ones so use
PHPExcel, which is way better, just a bit more complicated.
Test Plan:
- Generated modern Excel 2007 .xslx sheets.
- Opened them in Excel in Office Mac 2011.
- Opened them in Apple Numbers from the app store.
Reviewers: btrahan, jungejason
Reviewed By: jungejason
CC: aran, epriestley
Maniphest Tasks: T911
Differential Revision: https://secure.phabricator.com/D1744
Summary:
- We have a lot of headers now; document them.
- Remove the one random protip from like 3 years ago from all Differential
mail.
Test Plan: generated; read documentation
Reviewers: btrahan
Reviewed By: btrahan
CC: aran, epriestley
Maniphest Tasks: T931
Differential Revision: https://secure.phabricator.com/D1748
Summary:
- Update the Javelin submodule to pick up recent fixes (like D1749).
- Update the package definitions do do a slightly better job of packaging
resources.
Test Plan:
Up and down work in tokenizers now. Pages load slightly fewer
resources.
Reviewers: vrana, btrahan
Reviewed By: btrahan
CC: aran, epriestley
Maniphest Tasks: T927
Differential Revision: https://secure.phabricator.com/D1751
