Summary:
I pretty shortsightedly made sending a side effect of save() in the case that a
server is configured for immediate sending. Move this out, make it explicit, and
get rid of all the tangles surrounding it.
The web tool now ignores the server setting and only repsects the checkbox,
which makes far more sense.
Test Plan:
Sent mails from Maniphest, Differential, and the web console. Also ran all the
unit tests. Verified headers from Maniphest.
Reviewed By: rm
Reviewers: aran, rm
CC: tuomaspelkonen, rm, jungejason, aran
Differential Revision: 200
Summary:
Amazon SES does not allow us to set a Message-ID header, which means
that threads are incorrect in Mail.app (and presumably other applications
which respect In-Reply-To and References) because the initial email does not
have anything which attaches it to the rest of the thread. To fix this, never
rely on Message-ID if the mailer doesn't support Message-ID.
(In the Amazon SES case, Amazon generates its own Message-ID which we can't
know ahead of time).
I additionally used all the Lisk isolation from the other tests to make this
testable and wrote tests for it.
I also moved the idea of a thread ID lower in the stack and out of
DifferentialMail, which should not be responsible for implementation details.
NOTE: If you push this, it will cause a one-time break of threading for
everyone using Outlook since I've changed the seed for generating Thread-Index.
I feel like this is okay to avoid introducing more complexity here.
Test Plan:
Created and then updated a revision, messages delivered over Amazon
SES threaded correctly in Mail.app. Verified headers. Unit tests.
Reviewed By: rm
Reviewers: aran, tuomaspelkonen, jungejason, rm
Commenters: aran
CC: aran, rm, epriestley
Differential Revision: 195
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:
Thanks to erling for the report. This was XSSable, although you could
only get yourself.
Test Plan:
Made a comment like "</textarea><h1>" and edited it before and after
the patch. Proper behavior with this patch.
Reviewed By: aran
Reviewers: erling, jungejason, tuomaspelkonen, aran
CC: aran
Differential Revision: 187
Summary:
If you don't have a custom renderer, this variable does not get set
and emits a warning when you try to read it.
Test Plan:
Loaded page before and after change, warnings went away (lines
145 and 154 in old file).
Reviewed By: aran
Reviewers: tuomaspelkonen, aran
CC: jungejason, aran, epriestley
Differential Revision: 186
Summary:
Fixed buggy and incomplete logic for handling IGNORE ALL mode properly.
A subparser is used to parse the non-ws-ignoring changeset while a
ws-ignoring changeset is handed off to the original parser. At a later
step, the original parser queries the subparser for its lines of text
(which are formatted properly due to being in non-ws-ignoring mode) and
uses them to replace the text in the ws-ignoring diff.
Task ID: 549940
Test Plan:
-turn off caching temporarily (the cached view is still indented
improperly)
-visit http://phabricator.dev1943.facebook.com/D242591
-note aligned, but not completely highlighted, indentation right above the
comments complaining about
indentation issues
Reviewed By: aran
Reviewers: tuomaspelkonen, jungejason, aran
Commenters: epriestley
CC: aran, epriestley, grglr
Differential Revision: 174
Summary:
This adds a new view to differential called Updates.
The high-level goal of Updates is to enabled differential to be
effectively used without email notifications. I've tried doing things
like automatically deleting differential emails where I'm in the 'to'
line since they show up on the main diffential page but then there's
always the chance an important diff flies by without me seeing it. Also,
sometimes someone comments on a diff post-commit but differential
doesn't surface those diffs.
I re-created a test db on my devserver using mysqldump to get data on
revs > 230000 so I would have some test data. We need to add a simple
viewtime table but I didn't want to do that in production. Here's the
table:
CREATE TABLE differential_viewtime (
viewerPHID varchar(64) not null,
objectPHID varchar(64) not null,
viewTime int unsigned not null,
PRIMARY KEY (viewerPHID, objectPHID)
);
Issues:
-Once we turn this on, all diffs will be 'unviewed'. What do you think
about a 'Clear All' button or something?
-Maybe we should add a pager
This feature would be insanely useful, let me know what you think.
Test Plan:
Loaded Updates in my sandbox
http://phabricator.dev1577.snc6.facebook.com/differential/filter/updates/
Clicked a diff, then went back, made sure diff disappeared from Updates
list
Reviewed By: tuomaspelkonen
Reviewers: epriestley, jungejason, tuomaspelkonen
Commenters: epriestley
CC: epriestley, elynde, tuomaspelkonen
Differential Revision: 169
Summary:
Take Tuomas's first diff. Sorry, my bad on diff feedback.
Test Plan:
Load D240353 without any options and make sure 'ignore-trailing'
selected.
Reviewed By: tuomaspelkonen
Reviewers: tuomaspelkonen
CC: epriestley, aran, tuomaspelkonen
Differential Revision: 172
Summary:
'ignore-all' is very confusing, especially for Python, because the indentation
is incorrect.
Test Plan:
Tested loading D240353 without any options and made sure 'ignore-trailing' was
selected and the diff looked correct.
Reviewed By: aran
Reviewers: jungejason, simpkins, aran, dpepper
Commenters: dpepper
CC: grglr, aran, dpepper, tuomaspelkonen
Differential Revision: 171
Summary: add a conduit method to enable querying revisions' phid from
their revision_IDs, and another one to update the task->revision assoc.
Test Plan: for querying revision_phid method, tested empty, one, and two
revisions in the query. For the one to update the task->revision assoc,
I have another diff in facebook which verified it add and remove assoc
correctly.
Reviewers: tuomaspelkonen, epriestley
CC:
Differential Revision: 165
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:
Currently if I go to leave an inline comment and then decide
against it and hit <tab>-<enter>, the inline comment edit dialog closes
but the overlay doesn't disappear due to a JS error around trying to
access the markup property of null. This fixes that by setting the
payload to an array with empty markup so that the JS works and the
overlay disappears.
Test Plan:
clicked on the line number column, then pressed ok and
confirmed the dialog closed, the overlay disappeared and there were no
JS errors. also tested that for clicking the reply link and when editing
an existing inline comment.
Reviewed By: aran
Reviewers: epriestley, aran
CC: aran, bh
Revert Plan:
OK
Differential Revision: 154
Summary:
Whitespace options could not be selected and the 'ignore_all' was not working.
Test Plan:
Made sure that whitespace is ignored correctly and selected option is handled
correctly.
Reviewed By: jungejason
Reviewers: jungejason
CC: epriestley, jungejason, tuomaspelkonen
Differential Revision: 149
Summary:
Replies to comments went always to the left side of the diff. There
was a confusion between 'is_new' and 'on_right'. Database wants the
information if the reply is on the left or on the right. The database
does not care if the comment is a reply or an original comment.
The code looks a bit confusing, because the database field is called
'isNewFile' and that is used to determine, which side the comment
applies to.
Test Plan:
Tested that every of combination of new comment/reply to the left/right side
worked when editing and after submitting the comment.
Reviewed By: jungejason
Reviewers: jungejason
CC: epriestley, jungejason
Differential Revision: 148
Summary:
Differential used to only show diffs for the first 100 files. Now all
the files are shown in the table of contents and there is a link to
the standalone view for every file. The inline diffs can still be
seen, if user clicks "Show All Files Inline".
Inline comments can also be added in the standalone view, but there is
no form to submit them. The revision page must be reloaded to able to
submit the inline comment.
Test Plan:
Changed the limit to three for testing purposes and checked that a diff of
mine with 5 files had the links to the standalone views. Made sure that
adding a comment in a standalone view worked and that after reloading the
revision page the comment was visible. Changed the limit back to 100 and
made sure that my diff had all the files inline and that the anchor links
were working.
Reviewed By: jungejason
Reviewers: jungejason
CC: epriestley, simpkins, jungejason, tuomaspelkonen
Differential Revision: 147
Summary:
The existing message is confusing and might cause people to think
that there is something wrong in the field name, e.g., 'CC' instead
of the value.
Test Plan:
Tested by putting random crap in CC and Reviewers and made sure the
error message was using the new format.
Reviewed By: jungejason
Reviewers: epriestley, jungejason
CC: mdevine, jungejason
Differential Revision: 145
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:
Inline comment anchros were present for all different diffs inside a
revision. They were only working for the current diff. Removed the
links that were for different diffs.
I couldn't get the automatic linking to other diffs working, probably
because the anchors didn't work when the page was reloaded. This is also
a bit confusing if the diff changes when clicking on anchor.
We might want to carry all the comments along in the future, but I
don't think it's needed at the moment.
Test Plan:
Tested on a revision, which had inline comments for differerent diffs
that only the comments for the latest diff had anchors when the page
was loaded. Changed the diff manually on the page and made sure the
anchors for that diff were working correctly and the anchors for the
latest diff were not available.
Reviewed By: epriestley
Reviewers: epriestley
CC: jungejason, epriestley
Differential Revision: 142
Summary:
When quoting someone's text in differential, the text was not properly
rendered. Now the quoted text is rendered nicely and it's easy to locate.
Test Plan:
Disabled the inline cache, Checked that quotes in D211086 are rendered
correctly. Started writing a new comment starting with '>' and made
sure the new comment was rendered correctly while writing it.
Reviewed By: epriestley
Reviewers: epriestley
CC: jungejason, tuomaspelkonen, epriestley
Differential Revision: 138
Summary: Is this an appropriate place to say "herp derp?"
Test Plan: Check for Resign as Reviewer for the following table
waiting for review | accepted | committed
Am actually reviewer
Am not reviewer but CC'ed
Unrelated
Reviewers: epriestley
CC:
Differential Revision: 140
Summary:
Added long waited image macro support for differential and others.
Test Plan:
Tried a couple of different macros and made sure they appear nicely
in the comment preview. Made sure that the normal comments are shown
correctly.
Reviewed By: epriestley
Reviewers: epriestley
CC: jungejason, tuomaspelkonen, epriestley
Differential Revision: 129
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
Summary:
No workflow dialog when subscribing or unsubscribing from a Differential
Revision
Test Plan:
Repeatedly subscribe and unsubscribe to a revision. Enter refractory period due
to
speed of page gen.
Reviewed By: epriestley
Reviewers: epriestley
CC: epriestley
Differential Revision: 128
Summary:
There was a need to add old facebook specific action links and properties
back to differential.
Test Plan:
Tested that all the facebook specific links work for multiple
different revisions.
Reviewed By: epriestley
Reviewers: epriestley
CC: jungejason, tuomaspelkonen, epriestley
Differential Revision: 127
Summary:
Enable "Resign as Reviewer" from Differential Revision View UI
Test Plan:
Look at revision that I am a reviewer on and that I am not.
Reviewed By: epriestley
Reviewers: epriestley
CC: epriestley
Differential Revision: 120
Summary:
add filtering for MetaMTA transcripts, add Herald
transcripts, also fixed PhabricatorObjectHandleData to support commits.
Note that paging in the transcripts pages will be in a different diff.
Test Plan:
test the transcripts for both MetaMTA and Herald.
Reviewed By: epriestley
Reviewers: epriestley, tuomaspelkonen
CC: jungejason, epriestley
Differential Revision: 114
Summary:
Internal tools, e.g., differential and diffusion have user defined
preferences for monospaced font and the option for showing either the
name of the tool or the glyph of the tool in the title.
These preferences were ported to phabricator. These preferences can be
modified in /preferences/ and they both affect diffusion and differential
at the moment.
Test Plan:
* Created an empty database
* Loaded /preferences/ and modified the monospaced font and clicked save
* Confirmed that the same page was loaded with the message that preferences
have been saved and that the example text used the user defined font
* in /preferences/ changed the option to show tool names as plain text and
clicked save
* Confirmed that the same page was loaded with '[Preferences]' in the title
instead of a glyph
* These same tests were also executed for differential and diffusion
Reviewers: epriestley
CC: jungejason
Differential Revision: 91
Summary:
This makes some of the line spacing, paragraph spacing and layout
less terrible. In particular, fixes code blocks inside Differential inline
comments.
Test Plan:
Looked at Maniphest Tasks, Differential Revisions and Differential
inline comments with various flavors of remarkup in them.
Reviewed By: jungejason
Reviewers: tuomaspelkonen, jungejason
CC: jungejason
Differential Revision: 89
Summary:
Back to the authentic roots.
Test Plan:
Verified that the button had the correct text in the UI.
Reviewed By: epriestley
Reviewers: epriestley
CC: epriestley
Differential Revision: 86
Summary:
As we've discussed the check is not needed.
Test Plan:
- php -l
Reviewed By: epriestley
Reviewers: jungejason, epriestley
CC: epriestley
Revert Plan:
sure
Other Notes:
Differential Revision: 81
Summary:
add a constants module
src/applications/phid/constants/PhabricatorPHIDConstants.
Test Plan:
Execute applications which were using the hard-coded string.
Differential Revision: 44
Reviewed By: epriestley
Reviewers: epriestley
CC: epriestley
Summary:
- Prevent long comments from expanding the inline box.
- Make anchor links to inlines work properly.
- Get rid of "pre" white-space formatting in inline comments.
Test Plan: Viewed a revision with crazy comments in it.
Reviewers:
CC:
Summary: Autolink Differential and Maniphest objects.
Test Plan: Typed "D12345" and "T12345" into the Differential comment preview,
got links. Typed "http://www.elsewhere.com/D12345" and got a single link to
that URI, not a mess where the D12345 part linked incorrectly.
Reviewers: aran
CC:
Differential Revision: 35
