Summary: New implicit fallthrough linter detected a few issues; none of these have behavioral impacts but they can clearly be tightened up. See D1824.
Test Plan: Lint; inspection.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran, epriestley
Differential Revision: https://secure.phabricator.com/D1825
Summary:
The general idea here is to build a Differential-like dashboard which shows all
the things you need to audit and all the things that other people have raised
issues with, so you have a one-stop "what do I need to deal with?" interface.
- Add problem commits to the "active" view of /audit/.
- Add problem commits to homepage.
- Add commit browsing interfaces to /audit/.
- Add an "Audit" app button.
Test Plan: Looked at homepage, commit filters. Audited commits, verified state
changes reflected properly.
Reviewers: btrahan, jungejason
Reviewed By: btrahan
CC: aran, epriestley
Maniphest Tasks: T904
Differential Revision: https://secure.phabricator.com/D1712
Summary:
Improve the custom query interface:
- Allow search for tasks not in projects.
- Allow search for tasks with no projects.
- Allow custom search to include author/owner constraints.
Test Plan: Searched for various sorts of tasks.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran, epriestley
Maniphest Tasks: T911
Differential Revision: https://secure.phabricator.com/D1722
Summary:
- Use $this->linkTo($phid) to render all links.
- Simplify code.
Test Plan: Public feed renders with 'target="_top"' links. Nonpublic feed
doesn't. Looked at a bunch of feed stories, none seem broken.
Reviewers: btrahan, aran, nh, jungejason, ide
Reviewed By: btrahan
CC: aran, epriestley
Maniphest Tasks: T453
Differential Revision: https://secure.phabricator.com/D1514
Summary: getBestURI() = best URI
Test Plan:
It says "best" in the name so it must be the best!
Also in Maniphest emails we'll link you to /view/ even for binaries and other
non-viewable content.
Reviewers: btrahan, jungejason
Reviewed By: btrahan
CC: anjali, aran
Differential Revision: https://secure.phabricator.com/D1461
Summary:
We currently allow you to assign code review to disabled users, but
should not.
Test Plan:
- Created revisions with no reviewers and only disabled reviewers, was
appropriately warned.
- Looked at a disabled user handle link, was clearly informed.
- Tried to create a new revision with a disabled reviewer, was rebuffed.
Reviewers: btrahan, jungejason
Reviewed By: btrahan
CC: aran
Differential Revision: https://secure.phabricator.com/D1429
Summary:
we used to need this function for security purposes, but no longer need
it. remove it so that some call sites can be optimized via smarter data
fetching, and so the whole codebase can have one less thing in it.
Test Plan:
verified the images displayed properly for each of the following
- viewed a diff with added images.
- viewed a user feed
- viewed a user profile
- viewed all image macros
- viewed a paste and clicked through "raw link"
weakness in testing around proxy files and transformed files. not sure what
these are. changes here are very programmatic however.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, btrahan, epriestley
Maniphest Tasks: T672
Differential Revision: https://secure.phabricator.com/D1354
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:
add "Maniphest Task:" or "Maniphest Tasks:" followed by text that has TX in it.
foreach TX the task will be attached to the revision and the revision will be
attached to the task. parsing is pretty... ummm, robust such that it will pick
up any TX substring and parse that as a Maniphest Task just fine. it errors
out if there is not an actual task for TX and otherwise churns along pretty
nicely.
Also, make sure the PhabricatorObjectHandle loads the task ID as the alternateID
since we need that here and it should be that way anyhoo.
Test Plan:
made a diff and in the commit message added Maniphest Task(s): TX combination.
Tried various combinations of TX -- single, multiple with commas, multiple many
lines, single bad, multiple bad, multiple mix of bad and good. verified that the
good tasks were attached to the diff and diff was attached to the good tasks.
Maniphest Tasks: T137
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, btrahan, epriestley
Differential Revision: 1165
Summary: See T625. Facebook's REST-based MTA layer had a check for this so I
overlooked it in porting it out. We should not attempt to deliver email to
disabled users.
Test Plan:
Used MetaMTA console to send email to:
- No users: received "no To" exception.
- A disabled user: received "all To disabled" exception.
- A valid user: received email.
- A valid user and a disabled user: received email to valid user only.
(Note that you can't easily send to disabled users directly since they don't
appear in the typeahead, but you can prefill it and then disable the user by
hitting "Send".)
Reviewers: btrahan, jungejason, nh, tuomaspelkonen, aran
Reviewed By: aran
CC: skrul, aran, epriestley
Differential Revision: 1120
Summary: This method relies on 64-bit math being available, which isn't a safe
assumption. Use the builtin bc functions instead for arbitrarily large integers.
Test Plan: @skrul, can you apply this locally and let me know if it works?
Reviewers: skrul, hunterbridges, jungejason, nh, tuomaspelkonen, aran
Reviewed By: skrul
CC: aran, skrul, epriestley
Differential Revision: 912
Summary:
The CSRF changes meant that we can't generate a file URI with just its PHID
anymore, and converted a mathematical function into a service call.
Unfortunately, this caused massive perf problems in some parts of the
application, critically handles, where loading N users became N single gets.
Derp derp derp. Remedy this by doing a single multiget. This substantially
improves performance of many interfaces, particularly the Maniphest task list.
I need to go through the rest of the PhabricatorFileURI callsites and get rid of
them, but I think this is the most substantive one.
Test Plan: Profiled Maniphest task list, queries went from >100 to a handful.
Explosion of multiderp. :/ Looked at some views with profile photos to verify
they still render accurately.
Reviewers: jungejason, nh, tuomaspelkonen, aran
Reviewed By: aran
CC: aran
Differential Revision: 921
Summary:
We use ObjectHandles as proxy objects which can refer to any other object in the
system. Add the concept of the underlying object's "status" (e.g., open, closed
or busy).
This allows us to render completed tasks and revisions with strikethrough. In
the future, if we implement OOO or something, we could render users with a
"busy" status if they're on vacation, etc.
Test Plan: Viewed a task with closed revisions and dependencies:
https://secure.phabricator.com/file/view/PHID-FILE-6183e81286fa3288d33d/
Reviewed By: codeblock
Reviewers: codeblock, hunterbridges, jungejason, tuomaspelkonen, aran
CC: aran, codeblock
Differential Revision: 772
Summary:
I want clean this up enough that I can land D595 without making a complete mess,
here's a small simplification.
Move object load logic into PhabricatorObjectHandleData.
Test Plan: Attached tasks and revisions, merged tasks.
Reviewed By: aran
Reviewers: tuomaspelkonen, jungejason, aran
CC: aran
Differential Revision: 724
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
Summary: Index Phriction documents in search, and allow
PhabricatorObjectHandleData to load them.
Test Plan: Searched for "pokemons", got my wiki page as a search result.
Reviewed By: hsb
Reviewers: hsb, codeblock, jungejason, tuomaspelkonen, aran
CC: aran, hsb
Differential Revision: 648
Summary:
fix two issues. First is that the field in the query is not
correct; the other is to make indexing handle deleted repository.
Test Plan: indexed some commits and the search result looks correct.
Reviewed By: epriestley
Reviewers: epriestley, tuomaspelkonen
CC: aran, epriestley
Differential Revision: 633
Summary:
We deleted a repository (I don't remember which one). Some
differential revisions where committed to this repository, and opening
these revisions show a blank page, because the repository_id is not in
the database anymore.
This causes the 'ERROR 8: Undefined index: 12 at
[phabricator/src/applications/phid/handle/data/PhabricatorObjectHandleData.php:209]
' in our log.
Test Plan:
Opened a revision which where committed to multiple repositories including
the deleted one. Made sure that the page was rendered correctly and there
were no messages in the error log.
Reviewed By: epriestley
Reviewers: epriestley, jungejason
CC: aran, epriestley
Differential Revision: 598
Summary:
when a repository was deleted,
PhabricatorObjectHandleData::loadHandles() is throwing exception because
it assumes that the repository for the commit exists.
Test Plan: try an revision whose repo was deleted and it renders.
Reviewed By: epriestley
Reviewers: epriestley, andrewjcg
CC: aran, epriestley
Differential Revision: 576
Summary:
Make it prettier, paginate, add user pictures, show document types, clean some
stuff up a little. Plenty of room for improvement but this should make it a lot
more useful.
Test Plan:
Here's what the new one looks like:
https://secure.phabricator.com/file/view/PHID-FILE-edce2b83c2e3a121c2b7/
Reviewed By: jungejason
Reviewers: tomo, jungejason, aran, tuomaspelkonen, mroch
Commenters: tomo
CC: aran, tomo, jungejason, epriestley
Differential Revision: 545
Summary:
Provide a base PhabricatorMailReplyHandler class which handles the plumbing for
multiplexing email if necessary and supporting public and private reply handler
addressses. DifferentialReplyHandler now extends it, and a new
ManiphestReplyHandler also does.
The general approach here is that we have three supported cases:
- no reply handler, default config, same as what we're doing now
- public reply handler, requires overriding classes but just sets "reply-to"
to some address the install generates and still sends only one email
- private reply handler, provides a default generation mechanism or you can
override it and splits mail apart so we send one to each recipient
Test Plan:
Sent email from Maniphest and Differential with and without
reply-handler-domains set.
Reviewed By: aran
Reviewers: jungejason, tuomaspelkonen, aran
CC: aran, epriestley
Differential Revision: 254
Summary:
differential.getrevision now returns commits for a given revision.
URI is also return in differential.getrevision.
Test Plan:
Tested from Conduit Console UI that the calls were working
correctly.
Reviewed By: jungejason
Reviewers: jungejason
CC: epriestley, simpkins, dpepper, jungejason
Differential Revision: 163
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:
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
