1
0
Fork 0
mirror of https://we.phorge.it/source/phorge.git synced 2024-11-11 09:22:40 +01:00
Commit graph

107 commits

Author SHA1 Message Date
epriestley
c29982acb9 Fix phid accumulation for handles
Summary: I goofed this, $phids was already being populated and I changed the
meaning. This causes a fatal if you filter the list by a user who is not an
author or first reviewer for any of the revisions (e.g., no open revisions).

Test Plan: Looked at the list of a user with no revisions.

Reviewers: codeblock, jungejason

Reviewed By: codeblock

CC: aran, codeblock, jungejason

Differential Revision: 989
2011-10-07 12:58:16 -07:00
epriestley
bea4795575 Separate revision list rendering logic into a RevisionListView
Summary: I want to throw this in Diffusion as part of T262, but it's embedded in
the controller right now. Split it out.

Test Plan: Looked at various revision list views, no changes.

Reviewers: jungejason, nh, tuomaspelkonen, aran

Reviewed By: jungejason

CC: aran, jungejason

Differential Revision: 977
2011-10-06 10:26:47 -07:00
epriestley
76ac8b4196 Display local commit information in Differential
Summary:
After D857, we try to attach local commit information to revisions. If this
information is available, display it on the revision.

Design on this is a little rough, I might try to combine this into the revision
update view or something like that since we're starting to take up a lot of real
estate for metadata.

Test Plan: Local diffed this and got some commit info.

Reviewers: jungejason, tuomaspelkonen, aran

Reviewed By: jungejason

CC: aran, epriestley, jungejason

Differential Revision: 872
2011-08-31 13:49:50 -07:00
epriestley
69445222f7 Track content sources (email, web, conduit, mobile) for replies
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
2011-08-30 11:08:27 -07:00
Jason Ge
4693ffa82b Deprecate generateProperties
Summary:
deprecate generateProperties() from class
DifferentialRevisionDetailRenderer. Custom fields now provides a much
more powerful version of generateProperties().

Depends on D814.

Test Plan:
implemented facebook task field with custom field and
verified it worked.

Reviewers: epriestley, tuomaspelkonen

Reviewed By: epriestley

CC: aran, jungejason, epriestley

Differential Revision: 826
2011-08-18 11:33:10 -07:00
epriestley
6dc193d3d9 Fully update library map. 2011-08-18 09:52:36 -07:00
epriestley
2d22226ff0 Unguard the Differential update time write on GET. 2011-08-16 13:50:47 -07:00
epriestley
a869dbf45b Implement all field edit interfaces on the custom field schema
Summary:
Moves the revision edit controller to be completely schema-driven.

Depends on D810.

Test Plan: Edited revisions. Entered intentionally invalid values to trigger
error conditions.
Reviewed By: jungejason
Reviewers: jungejason, tuomaspelkonen, aran
CC: aran, jungejason, epriestley
Differential Revision: 810
2011-08-15 10:21:00 -07:00
epriestley
442d1dbeaa Move Differential's remaining field views to extensble field schema
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
2011-08-15 10:20:46 -07:00
epriestley
5038b26018 Move Differential's read-only fields to the extensible field schema
Summary:
Move additional fields (which rely on loading handles) to the extensible field
classes and out of hardcoding in the controller.

Depends on D807.

Test Plan: Viewed, edited, and hit conduit for revisions.
Reviewed By: jungejason
Reviewers: jungejason, tuomaspelkonen, aran
CC: aran, jungejason, epriestley
Differential Revision: 808
2011-08-15 08:39:58 -07:00
epriestley
52ec6c02ee Move Differential's simple fields to the extensible field schema
Summary:
Differential has a bunch of display-only fields, implement them all as field
specifications instead of hard-coded fields.

Also add some more documentation and fix redundant string constants in blame
rev/revert plan fields.

Test Plan: Viewed, edited, and hit conduit for revisions.
Reviewed By: jungejason
Reviewers: jungejason, tuomaspelkonen, aran
CC: aran, jungejason, epriestley
Differential Revision: 807
2011-08-15 08:39:48 -07:00
epriestley
9b3370368d Allow Differential custom fields to appear on edit and view interfaces
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
2011-08-14 10:04:37 -07:00
epriestley
f49e35deaf Basic task dependencies for Maniphest
Summary:
This allows you to edit dependencies. It is a better patch than it used to be.
It depends on D725.

  - If you create a cycle, it just throws an exception and aborts the workflow.
It should not do this.
  - Tasks which depend on the current task aren't shown in the UI. Need to add a
new table for this.
  - Transaction text says "attached Task" but should probably say "added a
dependency on task".

Test Plan: Created valid and invalid dependencies between tasks. Created valid
and invalid dependencies between revisions.
Reviewed By: tuomaspelkonen
Reviewers: davidreuss, jungejason, tuomaspelkonen, aran
Commenters: codeblock
CC: aran, codeblock, tuomaspelkonen, epriestley
Differential Revision: 595
2011-08-02 11:16:31 -07:00
epriestley
c0ae2f6289 Show change diffs in Phriction
Summary:
This is really rough and needs work (particularly, there's some diff code I
really need to refactor since I sort-of-copy-pasted it) but basically
functional.

Show text changes between diffs and allow users to revert to earlier versions.

Differential's line-oriented diff style isn't ideal for large blocks of text but
I'm betting this is probably good enough in most cases. We can see how bad it is
in practice and then fix it if needbe.

I added a bunch of support for "description" but didn't add the feature in this
diff, I'll either follow up or task it out since it should be a pretty
straightforward change.

Test Plan: Looked at history for several Phriction documents, clicked "previous
change" / "next change", clicked revert buttons.
Reviewed By: hsb
Reviewers: hsb, codeblock, jungejason, tuomaspelkonen
CC: aran, hsb, epriestley
Differential Revision: 687
2011-07-18 08:46:45 -07:00
epriestley
5e00d00cf7 Show more information on revision views
Summary:
Show line count, arcanist project and base revision.

This adds a little clutter but I think we're still okay and I can play around
with it later.

Test Plan: Looked at a couple of revisions. I'm actually not 100% sure about the
SVN logic but maybe I will test that before committing.
Reviewed By: tomo
Reviewers: tomo, jungejason, tuomaspelkonen, aran
CC: aran, tomo
Differential Revision: 685
2011-07-16 18:54:13 -07:00
epriestley
a49138defd Generalize the markup engine factory
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
2011-07-11 16:36:30 -07:00
epriestley
c9acc5b8e9 Allow comment panel to be stuck/unstuck to the bottom of the display
Summary:
See T303. Enable comment panel haunting.

I hid the preview for the sticky panel, which I think is reasonable?

Test Plan:
https://secure.phabricator.com/file/view/PHID-FILE-64713fa8a7c2a22e5b93/
Reviewed By: broofa
Reviewers: broofa, jungejason, aran, tomo, tuomaspelkonen
CC: aran, broofa
Differential Revision: 615
2011-07-08 13:24:20 -07:00
epriestley
2b8116d7ae Fix error cues on "Edit Revision" interface
Summary: The field hints on this interface don't behave correctly. Particularly, when you add yourself as a reviewer you aren't pointed at the issue.

Test Plan: Edited a revision and tried to save invalid changes, including self-reviewership.

Reviewers: moskov, jungejason, tuomaspelkonen, aran

CC:

Differential Revision: 565
2011-06-30 13:53:22 -07:00
epriestley
bb4cf7d6b3 Add an "Add CCs" action to Differential
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
2011-06-28 06:41:38 -07:00
epriestley
74d57b0a42 Use phabricator_ time functions in more places
Summary:
Replace some more date() calls with locale-aware calls.

Also, at least on my system, the DateTimeZone / DateTime stuff didn't actually
work and always rendered in UTC. Fixed that.

Test Plan:
Viewed daemon console, differential revisions, files, and maniphest timestamps
in multiple timezones.

Reviewed By: toulouse
Reviewers: toulouse, fratrik, jungejason, aran, tuomaspelkonen
CC: aran, toulouse
Differential Revision: 530
2011-06-26 10:38:25 -07:00
epriestley
4e75080b66 Make naming, titles and layout more consistent between Maniphest and
Differential

Summary:
Make some display stuff more consistent.

Test Plan:
Looked at a task and a revision.

Reviewed By: jungejason
Reviewers: aran, jungejason, tuomaspelkonen
CC: aran, jungejason
Differential Revision: 462
2011-06-14 20:45:43 -07:00
epriestley
4ec31ef75c Provide basic capabilities to make Differential column width flexible
Summary:
- Make wrap width settable in PHP.
  - Dynamically generate max-width based on configurable maximum width.
  - Constrain non-diff elements to standard width.
  - Provide a configuration setting.

Test Plan:
Set various things to 100 / 120, as far as I could tell everything seemed to
render sensibly? This should have no effect on 80-col changes.

Reviewed By: jdperlow
Reviewers: jdperlow, tuomaspelkonen, jungejason, aran
CC: aran, jdperlow
Differential Revision: 413
2011-06-09 12:01:11 -07:00
epriestley
4d3ef5ee0b Remove nonfunctional links from Differential inline comment previews
Summary:
They currently have "Next", "Previous" and "Reply" links which don't work. Don't
render these links.

Test Plan:
Looked at inline previews, didn't see any silly/nonfunctional links.

Reviewed By: tuomaspelkonen
Reviewers: aran, jungejason, tuomaspelkonen
CC: aran, epriestley, tuomaspelkonen
Differential Revision: 419
2011-06-09 11:22:07 -07:00
tuomaspelkonen
2521621074 Added a conduit call to update arc unit results for a postponed test.
Summary:
It was not possible before to update arc unit results for a postponed
test.  This change makes it possible. Also the number of postponed tests
are shown in differential.

Let me know if this looks too Facebook specific.

Test Plan:
Tested the conduit call manually from Conduit Console and updated test
results for a diff that had 20 postponed tests.

Reviewed By: jungejason
Reviewers: epriestley, jungejason
Commenters: epriestley
CC: slawekbiel, aran, tuomaspelkonen, jungejason, epriestley
Differential Revision: 416
2011-06-09 10:45:58 -07:00
epriestley
2e8990e9e0 Store metadata with Differential and Maniphest comments, and store added
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
2011-06-09 10:43:25 -07:00
epriestley
94d0adb140 Add "Undo" for editing Differential inline comments
Summary:
When a user hits 'cancel' on a 'new', 'edit', or 'reply' operation, add a little
"Changes discarded. __Undo__" insert so they can get their change back. No undo
for delete since there's an explicit prompt. Once this lands we can make
'escape' work again to close dialogs.

This change started feeling really good when I was merging all the duplicate
code and making things more consistent, but by the time I started writing client
rendering it felt gross. I'm not really thrilled with it but I guess it's a step
forward? The feature seems pretty OK in practice. Let me know how much barfing
this causes and I can try to remedy the most acute concerns.

This also fixes a bug where replies always (?) appear on the 'new' side of the
diff (I think?).

Test Plan:
Applied 'new', 'edit', 'delete' and 'reply' operations, pressed 'cancel' and
'okay' in each case, with and without changing text where relevant. All
behaviors seem to conform with expectations, except that canceling out of 'edit'
without changing the text gives you an option to undo when it shouldn't really.
There's no super easy way to get at the original text right now.

Reviewed By: aran
Reviewers: aran, jungejason, tuomaspelkonen
CC: simpkins, aran, epriestley
Differential Revision: 406
2011-06-08 10:44:01 -07:00
epriestley
112346ee61 Validate inline comment changeset IDs and revision IDs
Summary:
Historically, we had a bug at some point which caused inline comments to get
associated with changeset 0. Prevent that explicitly. See T108.

Test Plan:
Set "$changeset = 0" in the endpoint and got an exception.

Reviewed By: aran
Reviewers: aran
CC: aran
Differential Revision: 374
2011-06-07 19:58:45 -07:00
epriestley
74a2953ffd Support "!unsubscribe" in Differential reply handler
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
2011-05-31 15:19:55 -07:00
epriestley
4b92b2cead Allow revisions to be edited from Maniphest
Summary:
You can currently attach tasks to revisions from Differential, but not revisions
to tasks from Maniphest. Allow editing from either side.

This logic is kind of tricky but the alternative was massive code duplication.

Test Plan:
Added and removed revisions from maniphest. Added and removed tasks from
differential.

This should have no impact on the Facebook install since none of this is used
there.

Reviewed By: aran
Reviewers: tomo, tuomaspelkonen, jungejason, aran
CC: aran, epriestley
Differential Revision: 288
2011-05-16 15:31:46 -07:00
tuomaspelkonen
70807fb4e2 Fixed for /differential/diff
Summary:
It was broken

Test Plan:
Sandbox

Reviewed By: grglr
Reviewers: grglr, epriestley
CC: aran, grglr
Differential Revision: 280
2011-05-13 12:29:47 -07:00
epriestley
54154e4f48 Move "Rendering References" to the DifferentialChangesetParser level
Summary:
Separates changeset IDs from rendering. Now each changeset has a "rendering
reference" which is basically a description of what the ajax endpoint should
render. For Differential, it's in the form "id/vs". For Diffusion,
"branch/path;commit".

I believe this fixes pretty much all of the bugs related to "show more" breaking
in various obscure ways, although I never got a great repro for T153.

Test Plan:
Clicked "show more" in diffusion change and commit views and differential diff,
diff-of-diff, standalone-diff, standalone-diff-of-diff views. Verified refs and
'whitespace' were always sent correctly.

Made inline comments on diffs and diffs-of-diffs. Used "Reply".

Reviewed By: tuomaspelkonen
Reviewers: tuomaspelkonen, jungejason, aran
CC: aran, tuomaspelkonen, epriestley
Differential Revision: 274
2011-05-13 06:10:57 -07:00
tuomaspelkonen
43f6cc75f6 Added 'Next' and 'Previous' links to differential
Summary:
Browsing comments was a bit difficult without the possibllity to jump
between comments. These links will make the browsing easier.

Test Plan:
Tested on multiple diffs that the links were working correctly.

Reviewed By: epriestley
Reviewers: epriestley
CC: jungejason, aran, tuomaspelkonen, epriestley
Differential Revision: 266
2011-05-11 14:46:00 -07:00
tuomaspelkonen
e4f42dcd7d Correct whitespace option is passed to 'Show All Lines' request.
Summary:
Expanding lines duplicated some lines occasionally, because whitespace
option was different for the original request and the following request.

Test Plan:
Tested that the broken changeset was correct now.

Reviewed By: epriestley
Reviewers: epriestley
CC: jungejason, aran, epriestley
Differential Revision: 263
2011-05-10 18:05:13 -07:00
epriestley
3ab334af93 Fix "reply" link in Differential
Summary:
Fixes the issue caused by rPa0af5b66437719dba6136579c051982ab275e6a0. Prior to
that patch, isCommentInNewFile() returned $comment->getIsNewFile(). While this
was often the wrong value, it came from the database and was the integer 1 if
true.

After the patch, the function returns 'true' as a boolean, which is passed to JS
and then back to PHP, interpreted as an integer, and evaluates to 0.

To avoid this issue in general, provide an isBool() method on AphrontRequest
which interprets this correctly.

I will also revert the revert of rPa0af5b66437719dba6136579c051982ab275e6a0 when
I land this.

Test Plan:
Clicked "reply" on the right hand side of a diff, got a right-hand-side inline
comment.

Reviewed By: rm
Reviewers: tuomaspelkonen, jungejason, aran, rm
CC: simpkins, aran, epriestley, rm
Differential Revision: 250
2011-05-08 21:23:15 -07:00
epriestley
3ee7b5380e Revert "Revert "Make DifferentialChangesetParser explicitly map display to storage for comments""
This reverts commit 1c2222f26f.
2011-05-08 21:22:25 -07:00
tuomaspelkonen
1c2222f26f Revert "Make DifferentialChangesetParser explicitly map display to storage for comments"
This reverts commit a0af5b6643.
2011-05-06 18:32:28 -07:00
epriestley
a0af5b6643 Make DifferentialChangesetParser explicitly map display to storage for comments
Summary:
This is a followup to D228. Basically, we use "changeset" (and, implicitly,
changesetID) for way too much stuff now.

One thing it can't possibly capture is the complete, arbitrary mapping between
the left and right sides of the displayed diff and the places we want to store
the left and right side comments. This causes a bunch of bugs; basically adding
inline comments is completely broken in diff-of-diff views prior to this patch.
Make this mapping explicit.

Note that the renderer already passes this mapping to
DifferentialChangesetParser which is why there are no changes outside this file,
I just didn't finish the implementation during the port.

This has the nice side-effect of fixing T132 and several other bugs.

Test Plan:
Made new-file and old-file comments on a normal diff; reloaded page, verified
comments didn't do anything crazy.

Expanded text on a normal diff, made new-file and old-file comments; reloaded
page, verified comments.

Repeated these steps for a previous diff in the same revision; verified
comments.

Loaded diff-of-diffs and verified expected comments appeared. Made new left and
right hand side comments, which almost work, see below.

NOTE: There is still a bug where comments made in the left-display-side of a
diff-of-diffs will incorrectly be written to the right-storage-side of the
right-display-side diff. However, this is an issue with the JS (the PHP is
correct) so I want to pull it out of scope for this patch since I think I need
to fix some other JS stuff too and this improves the overall state of the world
even if not everything is fixed.

Reviewed By: tuomaspelkonen
Reviewers: jungejason, tuomaspelkonen, aran, ola
CC: aran, tuomaspelkonen, epriestley
Differential Revision: 237
2011-05-06 15:01:52 -07:00
epriestley
dd8232b766 Hint 'arc' commands in Differential UI
Summary:
Point users toward 'arc amend', 'arc commit', 'arc patch' and 'arc export' since
no one is going to read 'arc help'.

There's kind of a tradeoff here where we're wasting a fair amount of UI space
for expert users with the patch/export hints but I think it's probably okay
since there's really no other way to figure out that these features exist.

Note that the "export" command given isn't complete (it needs --git or
--unified), but it will give you a useful error message when you run it, telling
you to specify --git or --unified. If it turns out users get confused by this,
let me know.

Test Plan:
Loaded a revision and looked at it. Faked it into 'accepted' status.

Reviewed By: tuomaspelkonen
Reviewers: tuomaspelkonen, aran, jungejason
CC: aran, tuomaspelkonen, epriestley
Differential Revision: 242
2011-05-06 13:59:45 -07:00
epriestley
7ebd0d1efe Provide a web interface to view raw source text in Differential
Summary:
Add links to the 'standalone view' to grab the raw source text. I think this
operation is rare enough that it's okay to hide it like this. I changed
'Standalone View' to 'View Standalone / Raw' to improve discoverability.

This also fixes the broken Standalone View links in Diffusion by no longer
rendering them.

Test Plan:
viewed old and new sources for a changeset

Reviewed By: tuomaspelkonen
Reviewers: tuomaspelkonen, jungejason, aran
CC: aran, tuomaspelkonen
Differential Revision: 243
2011-05-06 13:36:59 -07:00
epriestley
af06bfb1cc Make Changeset ID for render cache explicit
Summary:
DifferentialChangesetParser currently takes the Changeset object to mean a bunch
of different and mutually conflicting things implicitly:

  - Changeset ID is used to access the render cache.
  - Changeset ID is also used to tell the ajax endpoint what to render when
clicking "show more".
  - Changeset object has the actual changes.
  - Changeset ID and "oldChangesetID" are used to choose where to show inline
comments and how to attach new ones.

This indirectly causes a bunch of problems, like T141 and T132. Move toward
making all these separate things explicit. I want to have the changeset object
only mean the actual changes to display.

Test Plan:
Looked at changesets and verified the render cache was accessed correctly (and
not accessed in other cases).

Reviewed By: tuomaspelkonen
Reviewers: jungejason, tuomaspelkonen, aran
CC: aran, epriestley, tuomaspelkonen
Differential Revision: 228
2011-05-05 11:12:50 -07:00
epriestley
85b09c5ccb Make "Ignore All" the default whitespace mode
Summary:
This mode was fixed in D174 by grglr and is the best mode for almost all changes
once nonbroken, so make it the default.

This is also the mode which takes advantage of the render cache.

Test Plan:
Loaded a revision, got "Ignore All" as the default mode, and revision rendering
wasn't silly/broken.

Reviewed By: tuomaspelkonen
Reviewers: grglr, jungejason, tuomaspelkonen, aran
CC: aran, tuomaspelkonen
Differential Revision: 229
2011-05-05 11:12:09 -07:00
tuomaspelkonen
8c031db32b Added a link from a diff view to diff's revision if there is one.
Summary:
The diff view page should point to the revision, if the diff has
already been attached to one. The form to select the revision was also
removed in this case.

Let's me know if it should be possible to reattach a diff to a different
revision.

Test Plan:
Tested that a new diff created with '--preview' option was not attached to
any revision. After attaching the diff manually, made sure that the diff view
page
showed the link to the revision correctly.

Reviewed By: epriestley
Reviewers: jungejason, epriestley
CC: aran, epriestley
Differential Revision: 216
2011-05-02 18:34:33 -07:00
epriestley
864e0d8a2f Fix XSS hole in inline comment editing
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
2011-04-29 20:27:25 -07:00
epriestley
78d33b1771 Silence an undeclared variable warning
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
2011-04-29 20:27:02 -07:00
elynde
6fb24ba4a4 Merge branch 'master' of github.com:facebook/phabricator 2011-04-28 14:45:30 -07:00
elynde
bd0a4c0d04 Differential Updates View
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
2011-04-28 14:40:41 -07:00
adonohue
f910c379ce Modify whitespace default implementation
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
2011-04-28 09:22:02 -07:00
tuomaspelkonen
2b0b39c4e4 Differential defaults to 'ignore-trailing' until indentation issues are solved.
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
2011-04-27 18:55:46 -07:00
bhiller
15e71e9bf4 [differential] Prevent un-closeable overlay
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
2011-04-20 18:50:57 -07:00
tuomaspelkonen
a6c770217d Fixed whitespace options in Differential.
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
2011-04-19 23:07:44 -07:00