Summary:
This doesn't cover every case exhaustively (see comments) but should cover like
98% of the practical cases.
This makes one workflow modification: willWriteRevision() was previously
guaranteed to have a revisionID / revisionPHID and no longer is. I verified that
no field implementations depend on this behavior. Fields which depend on IDs
should be using didWriteRevision() instead.
Test Plan: Inserted a "throw" into the middle of the transactions and created
revisions; they didn't orphan. Created revisions normally, they worked
correctly.
Reviewers: btrahan, nh
Reviewed By: btrahan
CC: aran, epriestley
Maniphest Tasks: T605
Differential Revision: https://secure.phabricator.com/D1541
Summary: There are lots of callsites to $changeset->getFilename() so it seemed
easier to rename getFileName() to getFilename() even if it includes database
change. Plus I think that getFilename() is better.
Test Plan:
Alter database.
Open revision.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran
Differential Revision: https://secure.phabricator.com/D1437
Summary:
See T730 and the slightly-less-pretty version of this in D1398.
When a user takes an action in Differential that has no effect (for instance,
accepting an already-accepted revision), prompt them:
Action Has No Effect
You can not accept this revision because it has already been accepted.
Do you want to post the feedback anyway, as a normal comment?
[Cancel] [Post as Comment]
If they have no comment text, the dialog only says "Cancel".
I think this is probably the best way to balance all the concerns here -- it
might occasionally be a little annoying, but that should be rare, and it should
never be confusing (the current workflow is extremely confusing).
This also fixes the issue where you can add all sorts of CCs who are already
part of the revision, either explicitly or via mentions.
Test Plan:
Posted some has-effect and has-no-effect comments, made different
choices in the dialog, everything seems to work OK?
Reviewers: vrana, btrahan, jungejason
Reviewed By: vrana
CC: aran, vrana
Maniphest Tasks: T730
Differential Revision: https://secure.phabricator.com/D1403
Summary:
Commenting on a diff causes adding the writer to the CCs. It doesn't make much
sense if the writer is author or reviewer who get all the copies anyway.
I've also moved the decision to DifferentialCommentEditor.
Test Plan:
Comment on a diff where I am author
Comment on a diff where I am reviewer
Comment on a diff where I am neither
Explicitely Add CCs where I am author
Reviewers: epriestley
Reviewed By: epriestley
CC: jungejason, aran, epriestley
Differential Revision: https://secure.phabricator.com/D1397
Summary: See T773 and the explanatory inline comment.
Test Plan: Made no-action comments and comments that did something (reject, plan
changes) to revisions. Saw them always jump to the top of the action list.
Reviewers: jungejason, simpkins, btrahan
Reviewed By: jungejason
CC: aran, jungejason
Maniphest Tasks: T773
Differential Revision: https://secure.phabricator.com/D1386
Phabricator
Summary: ...this breaks without D1328. Used good ole "codemod" to do this
work, with lots of manual edits around 80 chars.
Test Plan: clicked around phabricator tool suite, particular differential, a
bunch
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, epriestley
Differential Revision: https://secure.phabricator.com/D1351
Summary: Some fields need this data in some circumstances in order to validate
-- see D1153.
Test Plan: Ran "arc diff" against local, no longer got an exception for access
of this field from the 'Reviewers' validator.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran, btrahan
Differential Revision: 1160
Summary:
See T643. We have some hard-coded checks in Arcanist for the existence of
'testPlan' and 'title', and don't properly validate those fields on the server.
Add a validation pass in the Conduit-based edit pathway.
In particular, this means that if you disable the "Test Plan" field, Arcanist
won't block you anymore.
Test Plan: Disabled Arcanist checks and ran "arc diff"; got blocked on the
server side.
Reviewers: btrahan, jungejason
Reviewed By: btrahan
CC: aran, btrahan
Differential Revision: 1153
Summary:
`arc commit` and `arc mark-committed` would only add comments <author> committed
this revision, since now everyone can run this commands it makes more sense to
show the actual committer instead of the author.
Test Plan:
Commit (or mark committed) not your revision, Phabricator should add <you>
committed this revision comment instead of <author> committed this revision.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, epriestley
Differential Revision: 1067
Test Plan:
Login as an admin, go to a revision that you don't own - you should be able to
abandon this revision.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, epriestley
Differential Revision: 1048
Summary:
Still some rough edges, but this adds a table of open revisions to Diffusion.
See T262.
I'll make this a little better (e.g., "see all.." instead of arbitrary 10 cap,
or maybe move to top-level nav?) but I think I have to refactor some other stuff
first. This should let us root out any major issues, at least.
NOTE: You must associate Arcanist Projects with Repositories (in Repositories ->
Arcanist Projects -> Edit) for this to work!
Also made paths include all parent paths so that browse views of directories
will work.
Test Plan: Uploaded a diff which affected "/blah", it appeared when browsing "/"
and "/blah".
Reviewers: jungejason, nh, tuomaspelkonen, aran
Reviewed By: jungejason
CC: aran, jungejason
Differential Revision: 979
Summary: See T262. This creates the index on the Differential side which we need in order to execute this query efficiently on the Diffusion side.
Also renames "DiffusionGitPathIDQuery" to "DiffusionPathIDQuery", this query object has nothing to do with git.
Test Plan: Attached top-level and sub-level diffs to revisions and verified they populated the table with sensible data.
Reviewers: bmaurer, aravindn, fmoo, jungejason, nh, tuomaspelkonen, aran
CC:
Differential Revision: 931
Summary:
These fields use auxiliary storage now. Migrate the data and get rid of the
columns in the main table.
- This might take a little while to run, although there are <500k rows so
probably not too long.
- Maybe grab a backup of the table first, if I screwed something up this will
delete the data in these fields.
Test Plan:
- Ran migration locally.
- Browsed Differential.
- Grepped for "revertPlan" and "blameRevision".
Reviewers: jungejason, tuomaspelkonen, aran
Reviewed By: jungejason
CC: aran, jungejason, epriestley
Differential Revision: 832
Summary:
When an object is updated, record the content source for the update. This mostly
isn't terribly useful but one concrete thing I want to do with it is let admins
audit via-email replies more easily since there are a bunch of options which let
you do hyjinx if you intentionally configure them insecurely. I think having a
little more auditability around this feature is generally good. At some point
I'm going to turn this into a link admins can click to see details.
It also allows us to see how frequently different mechanisms are used, and lets
you see if someone is at their desk or on a mobile or whatever, at least
indirectly.
The "tablet" and "mobile" sources are currently unused but I figured I'd throw
them in anyway. SMS support should definitely happen at some point.
Not 100% sure about the design for this, I might change it to plain text at some
point.
Test Plan: Updated objects and saw update sources rendered.
Reviewers: jungejason, tuomaspelkonen, aran
Reviewed By: jungejason
CC: aran, epriestley, jungejason
Differential Revision: 844
Summary: When a user stores the empty string in an auxiliary field, simply don't
store it, and delete it if it already exists.
Test Plan: Edited a revision with an empty "Quack" field, got an empty row in
the DB. Applied patch, edited empty again, row went away. Edited empty again,
still no row. Edited and put something in the field, got a row.
Reviewers: jungejason, tuomaspelkonen, aran
Reviewed By: jungejason
CC: aran, jungejason
Differential Revision: 865
Summary:
After D814 and D829, you should be able to implement this logic in the
didWriteRevision() method of the field.
Note that the attacher is still referenced in
ConduitAPI_differential_updatetaskrevisionassoc_Method. This method should
probably be moved to facebook/ since it's pretty Facebook-specific.
No rush on any of this, it's not hurting anything.
Test Plan:
- Hit differential.getcommitmessage
- Ran 'arc diff'
Reviewers: jungejason, tuomaspelkonen, aran
Reviewed By: jungejason
CC: aran, jungejason
Differential Revision: 830
Differential comments
Summary: If you @mention several users, at least one of which is already CC'd,
we unset all the CCs and don't attach the "Added CCs: ..." block to the comment.
Test Plan: @mentioned two users, one of whom was already CC'd.
Reviewers: jungejason, tuomaspelkonen, aran
Reviewed By: jungejason
CC: aran, jungejason
Differential Revision: 827
Summary:
When rendering commit messages, drive all the logic through field specification
classes instead of the hard-coded DifferentialCommitMessageData class. This
removes DifferentialCommitMessageData and support classes.
Note that this effectively reverts D546, and will cause a minor break for
Facebook (Task IDs will no longer render in commit messages generated by "arc
amend", and will not be editable via "arc diff --edit"). This can be resolved by
implementing the feature as a custom field. While I've been able to preserve the
task ID functionality elsewhere, I felt this implementation was too complex to
reasonably leave hooks for, and the break is pretty minor.
Test Plan:
- Made numerous calls to differential.getcommitmessage across many diffs in
various states, with and without 'edit' and with and without various field
overrides.
- General behavior seems correct (messages look accurate, and have the
expected information). Special fields like "Reviewed By" and "git-svn-id" seem
to work correctly.
- Edit behavior seems correct (edit mode shows all editable fields, hides
fields like "Reviewed By").
- Field overwrite behavior seems correct (overwritable fields show the correct
values when overwritten, ignore provided values otherwise).
Reviewed By: jungejason
Reviewers: jungejason, tuomaspelkonen, aran
CC: aran, jungejason
Differential Revision: 814
Summary:
See T354. List every rule which has ever been applied in X-Herald-Rules, not
just the ones which most recently triggered.
Also some random fixes while I was debugging this:
- When conduit methods throw non-conduit exceptions, make sure they get
logged.
- Trigger the Facebook "tasks" backcompat block only if we were going to fail
(this should reduce the shakniess of the transition).
- Fix some log spew from the new field stuff.
Test Plan:
- Created a rule (ID #3) "No Zebras" which triggers for revisions without
"zebra" in the title.
- Created a revision without "zebra" in the title, got X-Herald-Rules: <2>,
<3>
- Updated revision to have "zebra" in the title, verified rule did not trigger
in Herald transcript.
- Verified X-Herald-Rules is still: <2>, <3>
Reviewed By: aran
Reviewers: aran, jungejason, tuomaspelkonen
CC: aran, epriestley
Differential Revision: 817
Summary:
When we create or update a revision, we use a parsed commit message dictionary
to edit its fields. Drive consumption of the dictionary through custom fields
instead of hardcoding.
This requires adding some fields which don't really do anything right now to
cover fields which appear only in the commit message.
Test Plan: "arc diff"'d this revision against localhost, "arc diff"'d again to
update.
Reviewed By: jungejason
Reviewers: jungejason, tuomaspelkonen, aran
CC: aran, jungejason
Differential Revision: 811
Summary:
Move all the rest of the fields into the custom field schema, for revision
views.
I left a couple of stubs in here (willWriteRevision, didWriteRevision) since I'd
planned to do edits here too, but this diff is sort of big-ish already. I'll do
all the edit fields in the next revision.
Depends on D808.
Test Plan: Viewed, edited and conduit'ed some revisions.
Reviewed By: jungejason
Reviewers: jungejason, tuomaspelkonen, aran
CC: aran, jungejason, epriestley
Differential Revision: 809
Summary: Depends on D798. Extends custom fields and makes the vaguely useful:
they can appear on the edit and view interfaces. This does not integrate them
with commit messages yet; that's more complicated but I plan to do it shortly.
Test Plan: Implemented a custom field per P123, it correctly appears on the edit
interface, persists, validates, and shows up when viewing the revision.
Reviewed By: jungejason
Reviewers: jungejason, tuomaspelkonen, aran
CC: aran, epriestley, jungejason
Differential Revision: 800
Summary: Basic integration between Phriction and feed.
Test Plan: Created and edited some documents, they published to feed.
Reviewed By: hsb
Reviewers: hsb, codeblock, jungejason, tuomaspelkonen, aran
CC: aran, hsb
Differential Revision: 653
Summary:
This thing services every app but it lives inside Differential right now. Pull
it out, and separate the factory interfaces per-application.
This will let us accommodate changes we need to make for Phriction to support
wiki linking.
Test Plan: Tested remarkup in differential, diffusion, maniphest, people,
slowvote.
Reviewed By: hsb
Reviewers: hsb, codeblock, jungejason, tuomaspelkonen, aran
CC: aran, hsb
Differential Revision: 646
Summary: Basic hookup for Differential -> Feed. Also introduces "one-line"
stories for less-important stuff.
Test Plan: Interacted with some revisions, got feed stories out of it.
Reviewed By: jungejason
Reviewers: jungejason, aran, tuomaspelkonen, codeblock
CC: aran, jungejason
Differential Revision: 632
Summary:
When a user gets @mentioned in Differential, add them as a CC.
No Maniphest hookup yet since I want to make that one a little more formal.
Depends on D518.
Test Plan:
@mentioned a user and they were added as a CC.
Reviewed By: jungejason
Reviewers: tomo, mroch, jsp, jungejason, aran, tuomaspelkonen
CC: aran, jungejason
Differential Revision: 519
Summary:
We currently have only an "Add reviewers" action, add "Add CCs". This can also
be accomplished less-discoverably with mentions.
Test Plan:
Added reviewers and CCs to revisions. Toggled display between reviewers and CCs.
Reviewed By: jungejason
Reviewers: tomo, mroch, jsp, jungejason, aran, tuomaspelkonen
CC: aran, jungejason
Differential Revision: 521
Summary:
See T251. In Gmail, conversations split if you reply to them and the next email
does not "In-Reply-To" your message ID. When an action is triggered by an email,
carry its Message-ID through the stack and use it for "In-Reply-To" and
"References" on the subsequent message.
Test Plan:
Live-patched phabricator.com and replied to a Maniphest thread in Gmail without
disrupting the thread. Locally replied to Maniphest and Differential threads and
verified Message-ID was carried across the reply boundary.
Reviewed By: rm
Reviewers: tcook, jungejason, aran, tuomaspelkonen, rm
CC: aran, epriestley, rm
Differential Revision: 498
Summary:
create the event
Test Plan:
checked the timeline event was consumed successfully by
facebook daemon when the revision is created from the webUI or arc
command line.
Reviewed By: epriestley
Reviewers: epriestley, aran
CC: aran, epriestley
Differential Revision: 478
Summary:
The owners of a revision are only really the reviewers when the revision is in
NEEDS_REVIEW.
Also build a raw indexed document viewer so you can look at the index of a
document from the web interface.
Finally, reindex revisions when comments are added, not just when the revision
itself is edited.
Test Plan:
Toggled abandon/reclaim on a revision and verified the relationships indexed
properly.
Reviewed By: jungejason
Reviewers: aran, jungejason, tuomaspelkonen
CC: aran, epriestley, jungejason
Differential Revision: 470
reviewers as metadata
Summary:
The "Add reviewer" implementation is super lazy right now since I didn't want to
do a schema change. Man up and add a column. I also plan to store "via"
information here (e.g., via email or via mobile).
NOTE: This schema change may take a while since the comment table is pretty big
in Facebook's install.
This needs a little CSS work but I think it's reasonable for now.
Test Plan:
Made comments on revisions and tasks. Added reviewers to a revision, got linked
names instead of a blob of text.
Reviewed By: aran
Reviewers: tomo, jungejason, tuomaspelkonen, aran
Commenters: tomo
CC: aran, tomo, epriestley
Differential Revision: 394
Summary:
- added a new config class for representing the kind of repetition a rule has
(once, every time, first time only)
- added an email action to herald rules for differential to allow someone to get
an email but only the first one
- changed the herald rule ui to allow a user to pick the amount of repetition
Test Plan:
created a test rule and ran it over and over
Reviewed By: epriestley
Reviewers: epriestley, tuomaspelkonen
CC: aran, epriestley, gc3
Revert Plan:
Tags:
- begin *PUBLIC* platform impact section -
Bugzilla: #
- end platform impact -
Differential Revision: 357
Summary:
See task. Allows users to unsubscribe via email.
Test Plan:
Used mail receiver to unsubscribe from a revision. Tested subscribe/unsubscribe
buttons. Verified "!unsubscribe" appears as an avilable action in email.
Reviewed By: ola
Reviewers: ola, mroch, jungejason, tuomaspelkonen, aran
CC: aran, epriestley, ola
Differential Revision: 385
Summary:
This isn't terribly elegant but it solves the problem without loss of
generality. We can pursue a more finessed solution later if it seems prudent.
Test Plan:
Created a revision matched by a blanket herald rule, and then commented on it.
Comment email had X-Herald-Rules header in it.
Reviewed By: aran
Reviewers: aran, tuomaspelkonen, jungejason
CC: aran
Differential Revision: 218
Summary:
DifferentialRevision stores this field as a dictionary but the
Editor incorrectly passed it to Herald as a raw value array. Ideally the
property should be called unsubscribedDict or something but I'm increasingly
thinking we're going to centralize subscriptions for Adjutant or some similar
system so I'm disinclined to pursue the schema change just yet. I provided an
explicit raw-value-oriented API, at least.
Test Plan:
With two accounts, A and B: created universal Herald CC rule with
user B, created a revision with user A, had user B unsubscribe, had user A
update the revision. User B was not resubscribed.
Reviewed By: aran
Reviewers: jungejason, tuomaspelkonen, aran
CC: ola, aran, epriestley
Differential Revision: 188
Summary:
Phabricator did not support giving the task ids in the commit message.
Currently there is no default implementation for this, but there is a
Facebook specific implementation. At some point default implementation
for Maniphest tasks to revisions will be added.
Test Plan:
Tested with the Facebook specific implementation that task ids are
recognized in the commit message and tasks are automatically attached
to revisions.
The task attached to this revision was added from the commit message.
Reviewed By: jungejason
Reviewers: jungejason, epriestley
CC: dpepper, edward, gpatangay, tuomaspelkonen, jungejason
Differential Revision: 240996
Summary: to enable publishing to internfeed, we create timeline events
for the comments.
Test Plan: create a test revision, comments on it and check if it
creates a timeline event with correct data at /daemon/timeline.
Reviewers: epriestley,tuomaspelkonen
CC:
Differential Revision: 144
Summary:
Old differential allowed users to plan changes for their own revisions.
This feature is now available in Phabricator version of differential.
Test Plan:
Tested by selecting "Plan Changes" for one of my own aceepted revisions.
Reviewed By: epriestley
Reviewers: epriestley
CC: jungejason, epriestley
Differential Revision: 130
