Summary:
Ref T2222. Ref T3886. Medium term goal is to remove `DifferentialRevisionEditor`.
This temporarily reduces a couple of pieces of functionality unique to the RevisionEditor, but I'm going to go clean those up in the next couple diffs.
Test Plan: Used `arc diff --create` to create several revisions with different data.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T3886, T2222
Differential Revision: https://secure.phabricator.com/D8452
Summary: Ref T2222. Ref T3794. Medium term goal is to remove `DifferentialRevisionEditor`. This removes one of two callsites.
Test Plan: Used `arc diff --edit` to repeatedly update a revision, making changes to various fields.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T3794, T2222
Differential Revision: https://secure.phabricator.com/D8451
Summary: Ref T2222. Ref T3886. This is a little early for general use, but the message parse/generate stuff requires CustomFields and FieldSpecifications to be closely aligned, so this provides at least a plausbile approach for any installs that run into trouble.
Test Plan: Viewed config; reordered fields.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T2222, T3886
Differential Revision: https://secure.phabricator.com/D8450
Summary: Ref T2222. Ref T3886. Converts parsing and construction of commit messages to be driven by CustomField.
Test Plan:
This is a huge, messy change. I've made an effort to test it exhasutively, but suspect I probably missed a few behaviors. Roughly:
- Enumerted all current fields (fields implementing `shouldAppearOnCommitMessage()`) and tried to test them one by one.
- Used `arc diff --edit` repeatedly to manipulate each field (this workflow hits both the parse and construct steps).
- Used `arc amend --show` to examine construct output (this does not activate the "edit" mode).
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T3886, T2222
Differential Revision: https://secure.phabricator.com/D8449
Summary: When we fail to render a feed story because something is broken, just break that story, not the entire feed.
Test Plan: {F125898}
Reviewers: btrahan, chad
Reviewed By: chad
CC: aran
Differential Revision: https://secure.phabricator.com/D8488
Summary:
- Render functions as `func()` for consistency/clarity.
- Sort articles first.
- Sort case insensitively.
- Label the "no group" symbols.
Test Plan: Regenerated and examined docs.
Reviewers: btrahan, chad
Reviewed By: chad
CC: aran
Differential Revision: https://secure.phabricator.com/D8480
Summary:
adds project images. Also fiddles with HTML + CSS just a bit so we have a "picture" column and a "details" column if a picture exists.
This keeps the details all in a nice column even if there are many details that end up being taller than the picture UI.
Fixes T3991.
Test Plan: looked at a task (no pic), project (pic w/ no details), and user (pic w/ many details) hovercard and all looked good on Chrome and Safari
Reviewers: epriestley, chad
CC: chad, Korvin, epriestley, aran
Maniphest Tasks: T3991
Differential Revision: https://secure.phabricator.com/D8483
Summary: Via HackerOne. I don't think this is a security vulnerability, but it is inconsistent. There's no reason to prefill this, and I think the code was just lazy.
Test Plan:
- Hit this page with `?email=xyz` in a GET request, no more prefill.
- Looped the page with bad addresses, appropriate prefill.
- Added an address.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Differential Revision: https://secure.phabricator.com/D8458
Summary:
Couple of tweaks:
- If a conpherence has no participants, we fail to `attachParticipants()`. This can happen if you leave a Conpherence as the last participant, then visit the URI again explicitly.
- If you can't load any transactions (usually, because you don't have permission to view a thread's transactions), we try to attach `null` instead of `array()`. This can happen if you attempt to view a thread you don't have permission to see. A more general fix would be to tweak the load/filtering order, but I'm leaving that for another time since it's more involved and only gives us a small performance gain in unusual sitautions.
- `initializeNewThread()` should be declared `static`.
Test Plan:
- Viewed a thread with no participants, got proper policy error.
- Viewed a thread I couldn't see, got proper policy error.
- Grepped for `initializeNewThread()`.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Differential Revision: https://secure.phabricator.com/D8467
Summary:
Via HackerOne. This defuses an attack which allows users to steal OAuth tokens through a clever sequence of steps:
- The attacker begins the OAuth workflow and copies the Facebook URL.
- The attacker mutates the URL to use the JS/anchor workflow, and to redirect to `/phame/live/X/` instead of `/login/facebook:facebook.com/`, where `X` is the ID of some blog they control. Facebook isn't strict about paths, so this is allowed.
- The blog has an external domain set (`blog.evil.com`), and the attacker controls that domain.
- The user gets stopped on the "live" controller with credentials in the page anchor (`#access_token=...`) and a message ("This blog has moved...") in a dialog. They click "Continue", which POSTs a CSRF token.
- When a user POSTs a `<form />` with no `action` attribute, the browser retains the page anchor. So visiting `/phame/live/8/#anchor` and clicking the "Continue" button POSTs you to a page with `#anchor` intact.
- Some browsers (including Firefox and Chrome) retain the anchor after a 302 redirect.
- The OAuth credentials are thus preserved when the user reaches `blog.evil.com`, and the attacker's site can read them.
This 302'ing after CSRF post is unusual in Phabricator and unique to Phame. It's not necessary -- instead, just use normal links, which drop anchors.
I'm going to pursue further steps to mitigate this class of attack more thoroughly:
- Ideally, we should render forms with an explicit `action` attribute, but this might be a lot of work. I might render them with `#` if no action is provided. We never expect anchors to survive POST, and it's surprising to me that they do.
- I'm going to blacklist OAuth parameters (like `access_token`) from appearing in GET on all pages except whitelisted pages (login pages). Although it's not important here, I think these could be captured from referrers in some cases. See also T4342.
Test Plan: Browsed all the affected Phame interfaces.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran, arice
Differential Revision: https://secure.phabricator.com/D8481
Summary:
The People application shows users awaiting approval, but incorrectly counts disabled users (i.e., users who were not approved).
Instead, count only non-disabled, non-approved users.
Test Plan: My homepage count dropped from 4 to 1, corresponding to the actual number of accounts.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran, spicyj
Differential Revision: https://secure.phabricator.com/D8486
Summary:
There are quite a few tests in Arcanist, libphutil and Phabricator that do something similar to `$this->assertEqual(false, ...)` or `$this->assertEqual(true, ...)`.
This is unnecessarily verbose and it would be cleaner if we had `assertFalse` and `assertTrue` methods.
Test Plan: I contemplated adding a unit test for the `getCallerInfo` method but wasn't sure if it was required / where it should live.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley
CC: Korvin, epriestley, aran
Differential Revision: https://secure.phabricator.com/D8460
Summary:
I was trying to set up a http hook, but despite setting the config,
the endpoint wasn't getting a request. I was advised on IRC by balpert to
restart my daemons and it worked great after I did that.
Since this information isn't in the documentation, I am adding it to the
description of the option, so it helps the next person.
Test Plan: N/A
Reviewers: #blessed_reviewers, epriestley
Reviewed By: epriestley
CC: Korvin, epriestley, aran, spicyj
Differential Revision: https://secure.phabricator.com/D8447
Summary:
Ref T2222. Currently, Differential has a fairly hairy piece of logic to parse object lists, like `Reviewers: alincoln, htaft`. Extract, generalize, and cover this.
- Some of the logic can be simplified with modern ObjectQuery stuff.
- Make `@username` the formal monogram for users.
- Make `list@domain.com` the formal monogram for mailing lists.
- Add test coverage.
Test Plan:
- Ran unit tests.
- Called `differential.parsecommitmessage` with a bunch of real-world inputs and got sensible results.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T2222
Differential Revision: https://secure.phabricator.com/D8445
Summary: Ref T2222. We have a hunk of logic that purely does text parsing here; separate it and get coverage on it.
Test Plan:
- Ran new unit tests.
- Used `differential.parsecommitmessage`.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T2222
Differential Revision: https://secure.phabricator.com/D8444
Summary: Ref T2222. These no longer have any callsites. Also got rid of a little bit of other code which also no longer has callsites.
Test Plan: `grep`
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T2222
Differential Revision: https://secure.phabricator.com/D8443
Summary: Ref T2222. Update this callsite; pretty straightforward.
Test Plan: Used Conduit to take actions and saw their effects in Differential.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T2222
Differential Revision: https://secure.phabricator.com/D8442
Summary: Ref T2222. When we discover a commit associated with a revision, close it using modern transactions.
Test Plan: {F123848}
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T2222
Differential Revision: https://secure.phabricator.com/D8441
Summary: This can be a command, which might be arbitrarily long, but the column is VARCHAR(255).
Test Plan: `grep`
Reviewers: dctrwatson, btrahan
Reviewed By: btrahan
CC: aran
Differential Revision: https://secure.phabricator.com/D8446
Summary: Ref T4570. Add trivial assertions to tests which fail-by-exploding so we can fail tests with no assertions.
Test Plan: Ran `arc unit --everything` with Arcanist patched to fail with no assertions.
Reviewers: leebyron, btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T4570
Differential Revision: https://secure.phabricator.com/D8436
Summary:
Adding the Author to the home page and Audit overview page,
so that at a glance you can see who authored the commits
that need to be audited
Test Plan: View home page and audit overview page and see that author is shown
Reviewers: #blessed_reviewers, epriestley
Reviewed By: epriestley
CC: Korvin, epriestley, aran
Differential Revision: https://secure.phabricator.com/D8438
Summary: Ref T2222. Moves this instance of CommentEditor to TransactionEditor.
Test Plan: Used `bin/mail receive-test` to test receiving comment mail and action mail.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T2222
Differential Revision: https://secure.phabricator.com/D8427
Summary: Ref T2222. For now, I'm just dropping this rather than updating it since I'll need to come back here later for `DifferentialRevisionEditor` anyway, and no users rely on this functionality.
Test Plan: Static checks; this isn't user-facing.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T2222
Differential Revision: https://secure.phabricator.com/D8426
Summary: Ref T2222. Straightforward update to new stuff.
Test Plan:
- Tried to close an uncloseable revision.
- Closed a closeable revision.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T2222
Differential Revision: https://secure.phabricator.com/D8425
Summary: Ref T2222. Primary goal is to remove this callsite for `DifferentialCommentEditor`, but rather than updating it I'm just nuking this method since it's been deprecated for more than a year (more than two years?)
Test Plan: Reloaded Conduit method list.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T2222
Differential Revision: https://secure.phabricator.com/D8424
Summary:
Via HackerOne. We're missing this permissions check, so you can sneak around it with URL editing right now.
I checked the other queries in this application and they seem OK.
Test Plan: Tried to post to a blog I had no permission to join.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Differential Revision: https://secure.phabricator.com/D8423
Summary:
Unwinds the mess I made in D8422 / D8430:
- Remove `'fonts'`, since individual fonts can be included via Celerity now.
- Include Source Sans from the local source when a document uses it as a fontkit.
Test Plan: Browsed Diviner, saw Source Sans.
Reviewers: chad
Reviewed By: chad
CC: aran
Differential Revision: https://secure.phabricator.com/D8431
Summary: We have a dozen users who has `(...)` in their 'real name', like 'Jimmy (He) Zhang', and it's causing the diffusion file browser problems when blame is enabled. The parser does not expect those parenthesis and the lines of code will be empty if they were last touched by a user like that.
Test Plan: Try it
Reviewers: wez, lifeihuang, JoelB, #blessed_reviewers, epriestley
Reviewed By: epriestley
CC: Korvin, epriestley, aran
Differential Revision: https://secure.phabricator.com/D8429
Summary:
- Allow Celerity to map and serve WOFF files.
- Add Source Sans Pro, Source Sans Pro Bold, and the corresponding LICENSE.
- Add a `font-source-sans-pro` resource for the font.
Test Plan:
- Changed body `font-face` to `'Source Sans Pro'`.
- Added `require_celerity_resource('font-source-sans-pro')` in StandardPageView.
Works in Firefox/Chrome/Safari, at least:
{F123296}
{F123297}
{F123298}
Reviewers: btrahan, chad
Reviewed By: chad
CC: chad, aran
Differential Revision: https://secure.phabricator.com/D8430
Summary: Fixes T4553, T4407.
Test Plan: created tasks and they showed up in the proper column. edited task priority and they moved about sensically.
Reviewers: chad, epriestley
Reviewed By: epriestley
CC: Korvin, epriestley, aran
Maniphest Tasks: T4553, T4407
Differential Revision: https://secure.phabricator.com/D8420
Summary: Ref T988. This layout got mucked up a while ago, restore it to some semblance of sanity and give it a couple of basic search options.
Test Plan: Searched for stuff. Woooo~~
Reviewers: btrahan, chad
Reviewed By: chad
CC: aran
Maniphest Tasks: T988
Differential Revision: https://secure.phabricator.com/D8419
Summary: Ref T988. Instead of hard-coding the application landing page, make the Diviner root show books if any have been generated. Otherwise, show a helpful message about how to generate documentation locally.
Test Plan:
{F122723}
{F122724}
Reviewers: chad, btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T988
Differential Revision: https://secure.phabricator.com/D8416
Summary: Ref T988. This makes it easier to generate documentation.
Test Plan: Ran with and without `--book`. Examined CLI output.
Reviewers: chad, btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T988
Differential Revision: https://secure.phabricator.com/D8415
Summary: Ref T988. This is primarily intended to let us add the "HEY! THIS ISN'T USER DOCUMENTATION" notices to the arcanist and libphutil technical docs.
Test Plan: Added some prefaces, generated docs, looked at them.
Reviewers: chad, btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T988
Differential Revision: https://secure.phabricator.com/D8410
Summary:
Ref T988. When the user clicks a link we haven't explicitly resolved before, we send them to the `/find/` endpoint, but currently just 404 if we can't find the relevant documentation.
Instead, display a more user-friendly error message, since we're probably going to have some of these. Also, make the page title much worse.
Test Plan: Hit a 404 via `/find/`, got a nicer page.
Reviewers: chad, btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T988
Differential Revision: https://secure.phabricator.com/D8408
Summary:
Ref T988. This fixes the biggest current problem with Diviner, which is dead links to articles.
In the new Diviner, articles can have both a "name" (derived from the file name, and used in the URI) and a "title" (optional, specified explicitly). For example, we have one document with the name "feedback" and the title "Give Feedback! Get Support!".
On disk, we want to use the name for the actual file where the text lives ("feedback.diviner"). We also want to use the name in the URI, to generate a clean URI and to allow us to retitle the document slightly without breaking links to it (for example, we renamed the "Backup" document to "Backups and Migrations").
However, when displaying the article we want to use the title.
Currently, you can //only// link to the name, not the title. This is inconvenient:
- We have a bunch of existing docs which link to titles.
- It's natural/intuitive to link to titles.
- Linking to titles makes it easier/cheaper to generate documentation, because we don't need to be able to resolve things at render time.
To remedy this, allow links to target either names or titles. If we miss on a name query, we'll do a title query. This is implemented with a slug hash to allow approximately correct titles (wrong case/spacing/punctuation, e.g.) and sidestep all the UTF8/column length issues.
(In the long run, atom resolution should theoretically be more sophistiated than it is now, and we should do render-time lookups on at least some documents to catch bad links. However, this is fairly complicated and a relatively advanced feature, and I think allowing links to titles is desirable no matter what.)
Test Plan: The user documentation book now has valid links to articles when the titles and names differ.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T988
Differential Revision: https://secure.phabricator.com/D8407
Summary:
Ref T2222. Ref T4484. See D8404 for discussion.
When a revision is updated with the new Editor, apply Herald rules. Additionally, apply them in a modern way which generates transactions.
Test Plan: {F122299}
Reviewers: btrahan
Reviewed By: btrahan
CC: aran, chad
Maniphest Tasks: T2222, T4484
Differential Revision: https://secure.phabricator.com/D8405
Summary:
Ref T2222. Ref T4484. This is a stepping stone to getting Herald supported in the new Differental code. Generally:
- Instead of an Editor either supporting or not supporting Herald, let it choose based on transactions. Specifically, Differential only runs rules on revision creation and diff updates.
- Optionally, allow an Editor to return some transactions to apply instead of having to apply everything itself. This lets us make it clear why changes happend in the transaction log, and share more code.
- I updated only one transaction type (owners in Maniphest) since it was the easiest and cleanest to update and test. Everything else still works like it used to, it just won't generate a transaction record yet.
- The transaction records are a touch rough, but we can clean them up later.
Test Plan: {F122282}
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T4484, T2222
Differential Revision: https://secure.phabricator.com/D8404
Summary: Ref T2222. Ref T4481. This fixes the issue where "Plan Changes" could immediately trigger a state change (e.g., back to accepted) because of state-based transitions out of the NEEDS_REVISION state.
Test Plan: Planned changes an "accepted" revision, it didn't immediately return to being accepted.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T2222, T4481
Differential Revision: https://secure.phabricator.com/D8403
Summary:
Ref T2222. Ref T4481. Specifically:
- When a revision is updated, change all "Reject" reviewers to "Reject Prior".
- Change status to "Needs Review".
- Update the state logic to account for this properly.
Test Plan:
- Created a revision as user A, with B as a reviewer.
- Rejected as B.
- Updated the revision as A.
- Saw revision in "needs review" state, with B as a "Rejected Prior" reviewer.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T4481, T2222
Differential Revision: https://secure.phabricator.com/D8402
Summary:
Ref T2222. Five very small improvements:
- I hit this exception and it took a bit to understand which transaction was causing problems. Add an `Exception` subclass which does a better job of making the message debuggable.
- The `oldValue` of a transaction may be `null`, legitimately (for example, changing the `repositoryPHID` for a revision from `null` to some valid PHID). Do a check to see if `setOldValue()` has been called, instead of a check for a `null` value.
- Add an additional check for the other case (shouldn't have a value, but does).
- When we're not generating a value, don't bother calling the code to generate it. The best case scenario is that it has no effect; any effect it might have (changing the value) is always wrong.
- Maniphest didn't fall back to the parent correctly when computing this flag, so it got the wrong result for `CustomField` transactions.
Test Plan: Resolved the issue I was hitting more easily, made updates to a `null`-valued custom field, and applied other normal sorts of transactions successfully.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T4557, T2222
Differential Revision: https://secure.phabricator.com/D8401
Summary: Moves Browse to "View All" and makes "My Events" the default on Calendar.
Test Plan: Browse both pages.
Reviewers: btrahan, epriestley
Reviewed By: epriestley
CC: Korvin, epriestley, aran
Differential Revision: https://secure.phabricator.com/D8397
Summary:
Fixes T4550 by changing supportsFeed to shouldPublishFeedStory, so things can be more granular like that are with mail. Attempts to fix things generally too, filtering out xactions that have no business in feed, etc.
Also return an updated Task HTML representation on drag and drop moves, etc. This is important so if the priority changes you can see it reflected in the UI.
Test Plan: dragged tasks around. observed no feed stories on subpriority drags. observed feed stories and updated color bars on stories that changed priority
Reviewers: epriestley, chad
Reviewed By: epriestley
CC: Korvin, epriestley, aran
Maniphest Tasks: T4550
Differential Revision: https://secure.phabricator.com/D8399
Summary:
A user reported this stack trace:
http://pastebin.com/6auGbZsE
...on this GitHub issue:
https://github.com/facebook/phabricator/issues/389#issuecomment-36612511
The problem is similar to the original report, but not identical. In this case, we're following a sequence of steps like:
- Run setup checks.
- Check for enabled providers, in order to raise "no providers configured yet" warning.
- Try to generate login/redirect URIs.
- Build the request.
- Set the default base URI.
- Run normal code.
Since we try to generate URIs before we provide a default, this fatals. Instead, don't try to build objects.
An alternative fix might be to try to set defaults earlier, but we depend on some config and on building the Request in order to be able to figure out if a request is HTTP or HTTPS right now. We could assume one, or guess, or use protocol-relative URIs (`///host.com`), but I think this fix is a little cleaner overall. If we keep hitting similar stuff, we could look into alternate fixes.
We could also set some kind of "setup mode" flag and make `getURI()` if it's called during setup mode to detect these during testing. I'd like to hit one more of these before doing that, though.
Test Plan: Reproduced the issue, applied the patch, verified this fixes it.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Differential Revision: https://secure.phabricator.com/D8395
Summary: looks better, more useful
Test Plan: looked better, was more useful when I observed my feed
Reviewers: chad, epriestley
Reviewed By: epriestley
CC: Korvin, epriestley, aran
Differential Revision: https://secure.phabricator.com/D8394
Summary: This wasn't working. Create a little JS handler and server-side support for returning the Task in the "project card" format.
Test Plan: Edited tasks from the board - they worked!
Reviewers: epriestley
Reviewed By: epriestley
CC: Korvin, epriestley, aran, chad
Differential Revision: https://secure.phabricator.com/D8392
Summary:
If the first non-null entry in the params array is falsey, the request bombs.
Something like {"id":279,"projectPHIDs":[],"status":"0","ownerPHID":"PHID-USER-on3xxsnaljmfn36d4b7a"}
Test Plan:
Before:
echo '{"id":279,"projectPHIDs":[],"status":"0","ownerPHID":"PHID-USER-cj3cpuh7oorbmnn2pl5g"}' | arc call-conduit maniphest.update
{"error":"ERR-NO-EFFECT","errorMessage":"ERR-NO-EFFECT: Update has no effect.","response":null}
After:
echo '{"id":279,"projectPHIDs":[],"status":"0","ownerPHID":"PHID-USER-cj3cpuh7oorbmnn2pl5g"}' | arc call-conduit maniphest.update
{"error":null,"errorMessage":null,"response":{"id":"279","phid":"PHID-TASK-lbwcq3pmur2c5fuqqhlx"...
Reviewers: garoevans, epriestley, #blessed_reviewers
Reviewed By: epriestley
CC: Korvin, epriestley, aran
Differential Revision: https://secure.phabricator.com/D8391
Summary:
adds ManiphestTransaction::TYPE_PROJECT_COLUMN and makes it work. Had to clean up the Javascript ever so slightly as it was sending up the string "null" when it should just omit the data in these cases. Ref T4422.
NOTE: this is overall a bit buggy - e.g. move a task Foo from column A to top of column B; refresh; task Foo is at bottom of column B and should be at top of column B - but I plan on additional diff or three to clean this up.
Test Plan: dragged tasks around columns. clicked on those tasks and saw some nice transactions.
Reviewers: epriestley
CC: Korvin, epriestley, aran
Maniphest Tasks: T4422
Differential Revision: https://secure.phabricator.com/D8366
Summary:
Ref T2222.
- Restore mail tags for ApplicationTransactions mail.
- Restore subject line verbs.
- Denormalize line count and repository PHID.
- Fix an issue with the mailgun adapter where headers weren't attached properly.
Test Plan: Sent some mail, verified it had correct subjects and tags.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T2222
Differential Revision: https://secure.phabricator.com/D8378
Summary:
A few minor fixes:
- When we build a tag with `"meta" => null`, strip the attribute like we do for all other attributes. Previously, we would actually set the metadata to `null`. This happened with the Conpherence form.
- Just respond to the draft request with an empty (but valid) response, instead of building a dialog.
- `PhabricatorShapedRequest` is confusingly named and I should have caught this in review, but the basic shape of it is:
- You make one object.
- You call `trigger()` when stuff changes (e.g., a keystroke).
- It manages making a small number of requests (e.g., one request after the user stops typing for a moment).
- The way it was being used previously would incorrectly send a request for every keystroke.
I think I'm going to simplify `ShapedRequest` and merge it into some larger queue for T430.
Test Plan: Typed some text, no longer saw a flurry of requests. Reloaded page, still saw draft text.
Reviewers: btrahan, chad
Reviewed By: chad
CC: aran, chad
Differential Revision: https://secure.phabricator.com/D8380
Summary: Ref T2222. Remove the old controller and swap in the new ApplicationTransactions one.
Test Plan: Made a pile of edits.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T2222
Differential Revision: https://secure.phabricator.com/D8377
Summary:
Ref T2222. Make the "EditPro" controller accommodate diff updates, and support the transaction type. This one is pretty straightforward.
Also make `revisionPHID` in the comments table nullable to fix the "Edit" action.
Test Plan:
- Created new revision.
- Updated revision.
- Tried to do some invalid stuff.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T2222
Differential Revision: https://secure.phabricator.com/D8376
Summary:
Ref T2222. Differential has certain "words of power" (like `Ref T123` or `Depends on D345`) which should expand into a separate transaction when they appear anywhere in text.
Currently, they're respected in only some fields. I'm expanding them to work in any remarkup field, including comments and inline comments.
This partially generalizes transaction expansion/extraction in comments. Eventually, I'll probably implement some very soft sort of reference edge for T4036, maybe.
Test Plan: {F119368}
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T2222
Differential Revision: https://secure.phabricator.com/D8369
Summary:
For imported SVN repositories with an "Import Only" path, we produce a `/path/to/root/` URI, but should produce `/path/to/root/then/to/import/only/`.
As it is, the URI instructs the user to check out the whole repository.
Also, don't show the "Clone As" fragment in the URI for remote repositories, and prevent it from being edited for nonhosted repositories. This is generally more consistent with user expectation.
Test Plan:
- Created a remote SVN repository with "Import Only", saw path include it.
- Verified no "Clone As" options, no "Clone As" in URI.
- Switched it to hosted, saw "Clone As" options appear and work properly.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran, staticshock
Differential Revision: https://secure.phabricator.com/D8375
Summary:
Fixes T4414. Currently, when we discover a new repository, we do something like this:
foreach (branch) {
foreach (commit on this branch) {
do_something();
}
}
In cases where there are a lot of branches which mostly just branch `master`, this leads to us doing roughly `O(branches * commits)` work.
We have a `commitCache` to prevent this, but it has two problems:
- It only fills out of the DB, and we do this whole thing before writing to the DB, which is the entire point.
- It has a fixed size (2048) and on initial discovery we're usually dealing with way more commits than that.
Instead, also stop doing work if we hit a commit which is known already.
Test Plan:
- Added `print` on the number of discovered refs and number of unique refs.
- Ran `bin/repository discover --repair X` on a repo with several branches.
- Before the patch, got 397 refs and 135 unique refs.
- After the patch, got 135 refs and 135 unique refs.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T4414
Differential Revision: https://secure.phabricator.com/D8374
Summary:
See IRC. I'm having trouble figuring out what's going on with b4taylor's report, but fix two possible issues:
# The commit query is missing a `repositoryID`, which could cause issues if you import two copies of the same repository.
# I think we may try to close commits on untracked branches right now, as long as they aren't excluded by other autoclose rules.
Test Plan: Ran `bin/repository refs` on a few repos.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran, brennantaylor
Differential Revision: https://secure.phabricator.com/D8373
Summary: Better aligns the text area when leaving an inline comment. Also, phts
Test Plan: reload page, view new padding.
Reviewers: epriestley
Reviewed By: epriestley
CC: Korvin, epriestley, aran
Differential Revision: https://secure.phabricator.com/D8370
Summary: Ref T2222. This probably doesn't get everything, but should improve many of the newer transactions.
Test Plan: Looked at feed after making some edits.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T2222
Differential Revision: https://secure.phabricator.com/D8368
Summary: Ref T2222. This should help new mail thread properly with old mail.
Test Plan: Will push.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T2222
Differential Revision: https://secure.phabricator.com/D8367
Summary: Although the defaults don't require a verified email address, it's easy to lock yourself out by accident by configuring `auth.require-email-verification` or `auth.email-domains` before setting up email. Just force-verify the initial/setup account's address.
Test Plan: Went through setup on a fresh install, saw address verify.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Differential Revision: https://secure.phabricator.com/D8365
Summary: Ref T2222. This updates the new JIRA field to be editable.
Test Plan: Used `/editpro/` to edit associated JIRA issues.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T2222
Differential Revision: https://secure.phabricator.com/D8364
Summary: Ref T2222. This will probabaly have a few rough edges too, but seems to work well.
Test Plan:
- Made a bunch of comments while building this.
- Made some new comments.
- Verified that the Asana/JIRA integration is only a little bit janky, not completely broken.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T2222
Differential Revision: https://secure.phabricator.com/D8362
Summary: Ref T2222. This will probably have some rough edges for a bit (e.g., weird cases I didn't remember or think of), but there's no change to the underlying data and we can easily revert if things get too messy.
Test Plan: Looked at a variety of revisions and saw sensible output.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T2222
Differential Revision: https://secure.phabricator.com/D8361
Summary:
Ref T2222. These are pretty straightforward.
For these fields and a few others, the existing code shows the value for the "current/manual" diff (i.e., the diff selected in the diff selection table), not the "active" diff (i.e., the most recent diff attached to the revision). I'm going to drop that for now (always showing the most recent diff instead) and then reevaluate it once we're switched over. In 95% of cases these are the same, anyway.
Test Plan: Looked at fields; this diff changes nothing on its own.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T2222
Differential Revision: https://secure.phabricator.com/D8359
Summary:
...this was nice to do for boards, since this diff also starts calling this code in the board move case. The big trick is to use the new expandTransactions hook to expand the subpriority transaction into a priority transaction if pertinent. The other stuff is just about hiding these new subpriority extractions.
...also removes the "edit" UI from the default board since we can't actually edit anything and it thus is buggy.
Ref T4422. Next step is to move board edits into the editor with their own little transaction.
Test Plan: re-orded tasks on a maniphest query, reloaded, and noted re-order stuck.
Reviewers: epriestley
Reviewed By: epriestley
CC: Korvin, epriestley, aran
Maniphest Tasks: T4422
Differential Revision: https://secure.phabricator.com/D8358
Summary:
Ref T2222. Ref T3886. Gets the storage-based fields working.
This requires future changes to actually do anything, all this code is inactive.
Test Plan: {F118863}
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T3886, T2222
Differential Revision: https://secure.phabricator.com/D8357
Summary: Ref T2222. Ref T3886. Differential has a legacy storage table for auxiliary fields; move the data to modern storage.
Test Plan:
- Ran migration.
- Verified fields still worked properly afterward (view, edit, etc).
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T3886, T2222
Differential Revision: https://secure.phabricator.com/D8355
Summary:
Ref T2222. This isn't complete and doesn't change runtime behavior yet (the new fields are not glued to the interface), but implements many fields.
(The remaining fields have something weird going on with them, for the most part.)
Test Plan:
With additional changes, rendered most fields sensibly:
{F118834}
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T2222
Differential Revision: https://secure.phabricator.com/D8354
Summary:
The intent of getOrBuildEngine() is to save some boilerplate in cases where we're just using a standard engine, but it didn't get cached so we'd rebuilt it over and over again.
This was especially bad in Differential with a large number of inlines.
Test Plan: "Query" tab of services is no longer quite so ridiculous in Differential.
Reviewers: btrahan, dctrwatson
Reviewed By: btrahan
CC: aran
Differential Revision: https://secure.phabricator.com/D8352
Summary:
Currently, the linter raises `XHP29` warnings for these files because they are not abstract or final.
I guess there are two possibly solutions, either making the classes final or marking them as `@concrete-extensible`. Given that there are no subclasses of these classes in the `phabricator`, `arcanist` and `libphutil` repositories... I opted to declare the classes as final.
Test Plan:
The following linter warnings are gone:
```
>>> Lint for src/aphront/configuration/AphrontDefaultApplicationConfiguration.php:
Warning (XHP29) Class Not abstract Or final
This class is neither 'final' nor 'abstract', and does not have a
docblock marking it '@concrete-extensible'.
3 /**
4 * @group aphront
5 */
>>> 6 class AphrontDefaultApplicationConfiguration
7 extends AphrontApplicationConfiguration {
8
9 public function __construct() {
>>> Lint for src/applications/differential/mail/DifferentialReplyHandler.php:
Warning (XHP29) Class Not abstract Or final
This class is neither 'final' nor 'abstract', and does not have a
docblock marking it '@concrete-extensible'.
1 <?php
2
>>> 3 class DifferentialReplyHandler extends PhabricatorMailReplyHandler {
4
5 private $receivedMail;
6
```
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley
CC: Korvin, epriestley, aran
Differential Revision: https://secure.phabricator.com/D8347
Summary: This currently raises a linter `XHP37` warning.
Test Plan: The file now lints okay.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley
CC: Korvin, epriestley, aran
Differential Revision: https://secure.phabricator.com/D8349
Summary:
Ref T2222. Ref T3886. Ref T418. A few changes:
- CustomField can now index into global search.
- Use CustomField fields instead of older custom fields for Differential global search. (This slightly breaks any custom fields which exist, but they are presumably very rare, and probably do not exist; this break is also very mild.)
- Automatically perform CustomField and Subscribable indexing on applicable object types.
Test Plan: Used `bin/search index` to reindex a bunch of stuff, then searched for it. Debug-dumped abstract documents to inspect them.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T418, T3886, T2222
Differential Revision: https://secure.phabricator.com/D8346
Summary: If an attacker somehow intercepts a verification URL for an email address, they can hypothetically CSRF the account owner into verifying it. What you'd do before (how do you get the link?) and after (why do you care that you tricked them into verifying) performing this attack is unclear, but in theory we should require a CSRF submission here; add one.
Test Plan: {F118691}
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Differential Revision: https://secure.phabricator.com/D8351
Summary: Ref T2222. This enriches mail a little bit to get these rendering pretty much like they do now.
Test Plan: {F118255}
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T2222
Differential Revision: https://secure.phabricator.com/D8343
Summary: Fixes T4376. Only thing I don't like in the current implementation is clicking "Done" doesn't refresh the page so you don't see the viewed secret transaction until you reload. Also made the textarea read-only as when I was playing with this for the first time I assumed I could also edit from the view secret side of things.
Test Plan: Viewed some secrets, saw some transactions.
Reviewers: epriestley
Reviewed By: epriestley
CC: Korvin, epriestley, aran
Maniphest Tasks: T4376
Differential Revision: https://secure.phabricator.com/D8345
Summary: D8341 was a good start. However, I was looping through all the statuses each time, when I should only deal with a given status once. Instead, unset() a status from the list of statuses once we handled it. Also, delete the last old $key thing, which interfered with my chosen strategy.
Test Plan: made a two day event and verified it showed up in just those two days. (will push and test again just in case but this should be it)
Reviewers: epriestley, chad
Reviewed By: epriestley
CC: Korvin, epriestley, aran
Differential Revision: https://secure.phabricator.com/D8342
Summary:
...maybe anyway because I can't reproduce it live. This diff does two things that should help with bugginess though - uses $viewer rather than $user (...$user is who we are looking at...) *AND* upgrades a Conpherence util class to Calendar, and said util class has unit tests and came about from fixing a similar bug in Conpherence back in the day.
Wrote some comments in the util class because I think it has a tendency to trip people up. These comments are not partciularly good however.
Test Plan: viewed user profile - looked good. viewed conpherence - looked good. ran unit tests - they passed. (note I would also like to push this live and verify Chad's profile is fixed on secure.phabricator.com)
Reviewers: epriestley, chad
Reviewed By: epriestley
CC: Korvin, epriestley, aran
Differential Revision: https://secure.phabricator.com/D8341
Summary:
Ref T2222. This requires one new trick:
- When merging edge transactions which both add/update an edge, the Editor gets to control how the edge data is merged.
Specifically, we pick the "strongest" state to keep, so "accept + comment" leaves you with an accept instead of a comment.
Test Plan: Accepted, commented on, and comment + accepted revisions. Added some debugging dumps to verify that the merging was getting hit and working correctly.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T2222
Differential Revision: https://secure.phabricator.com/D8340
Summary:
Ref T2222. This doesn't feel super clean, but doesn't feel too bad either.
Basically, Differential transactions can have secondary state-based effects (changing the overall revision status) when reviewers resign, are removed, accept, or reject revisions.
To deal with this in ApplicationTransactions, I did this:
- `applyFinalEffects()` can now alter the transaction set (notably, add new ones). This mostly matters for email, notifications and feed.
- In Differential, check for an overall revision state transition in `applyFinalEffects()` (e.g., your reject moving the revision to a rejected state).
- I'm only writing the transaction if the transition is implied and indirect.
- For example, if you "Plan Changes", that action changes the state on its own so there's no implicit state change transaction added.
The transactions themselves are kind of fluff, but it seems useful to keep a record of when state changes occurred in the transaction log. If people complain we can hide/remove them.
Test Plan: {F118143}
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T2222
Differential Revision: https://secure.phabricator.com/D8339
Summary: Ref T2222. This mostly makes Accept/Reject work. The big missing piece is that overall revision status does not yet update properly. I need to think about how I want that to work a little bit more.
Test Plan: Accepted and rejected some stuff.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T2222
Differential Revision: https://secure.phabricator.com/D8333
Summary: Ref T2222. This is obsolete and no longer used. We could deduce it from transactions or commits in modern Phabricator if we wanted it. We may implement a more general mechanism for T4434.
Test Plan: `grep`
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T2222
Differential Revision: https://secure.phabricator.com/D8330
Summary:
Ref T2222. This introduces two small new concepts:
- `expandTransactions()`: allows a transaction to expand into several transactions. For example, "resign" adds a "remove reviewers" transaction.
- We have some other cases which could use this, but currently hard-code things outside of the `Editor`.
- One example is that in Maniphest, closing a task implies claiming it if it is unowned.
- `setIgnoreOnNoEffect()`: The whole Editor can be set to continue or stop if any transactions have no effect, but this allows the behavior to be refined at the individual transaction level. This is primarily to make the UX less confusing, so the user gets only a single relevant error instead of one for each expanded transaction.
Otherwise, this is pretty straightforward.
Test Plan:
Rigged comment form to use SavePro controller, enabled resign action, then tried to resign from a bunch of stuff.
{F117743}
Reviewers: btrahan
Reviewed By: btrahan
CC: chad, aran
Maniphest Tasks: T2222
Differential Revision: https://secure.phabricator.com/D8328
Summary: We store the `arc` commandline in this 255-character column, but it can be more than 255-characters long. If it's huge, truncate it.
Test Plan:
Executed:
arc list --conduit-uri=http://local.aphront.com:8080/ --conduit-version 6.aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa
Works fine after this patch.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Differential Revision: https://secure.phabricator.com/D8327
Summary: Hardens up the logic for DST and makes them easier to access elsewhere.
Test Plan: view sample events, all day and multiday, in my sandbox
Reviewers: epriestley
Reviewed By: epriestley
CC: Korvin, epriestley, aran
Differential Revision: https://secure.phabricator.com/D8332
Summary: Ref T2222. Implements the simpler actions (abandon, reclaim, close, reopen, plan changes, request review) in a transactional way with validation and effect checks.
Test Plan:
- Rigged submissions to point at the Pro controller.
- Rigged dropdown to have all these options all the time.
- Tried to apply about 20-30 of these operations to various revisions and always got the expected result (success, error, or no-op).
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T2222
Differential Revision: https://secure.phabricator.com/D8307
Summary: Ref T2222. Makes the "pro" controller work with inlines.
Test Plan: Added a bunch of inlines and saved them with the "pro" controller.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T2222
Differential Revision: https://secure.phabricator.com/D8306
Summary: Ref T2222. Adds a mostly-functional "Pro" comment controller. This does the core stuff, but does not yet do actions (accept, reject, etc.) or inline comments.
Test Plan: Changed the `if (false)` to an `if (true)`, then made some comments, etc. This is normally unreachable.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T2222
Differential Revision: https://secure.phabricator.com/D8304
Summary: Ref T2222. Adds basic support for email.
Test Plan: Received an email via `/editpro/`.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T2222
Differential Revision: https://secure.phabricator.com/D8303
Summary:
@mbishopim3 reported an issue in IRC:
> mbishopim3: epriestley: "Error updating working copy: Commit "" has not been discovered yet! Run discovery before updating refs." any ideas?
I can't reproduce it and it went away for him, but one theory is that we're getting here and git/hg are spitting out nothing, which we incorrectly parse as `array("")` when we intend `array()`.
Test Plan:
Pushed some new commits, ran `bin/repositoy refs X`, got expected results.
I can't actually reproduce the bug, but this might fix it and appears to make the code more correct.
Reviewers: btrahan
Reviewed By: btrahan
CC: mbishopim3, aran
Differential Revision: https://secure.phabricator.com/D8326
