1
0
Fork 0
mirror of https://we.phorge.it/source/phorge.git synced 2024-11-25 00:02:41 +01:00
Commit graph

6170 commits

Author SHA1 Message Date
cpettet
0420754d73 allow setting view and edit policy with maniphest.createtask
Summary: allowing explicit maniphest policy on api creation

Test Plan:
tested with arc cli

Example:

> echo '{"title": "mc6", "editPolicy": "users", "viewPolicy": "users"}' | \
> arc call-conduit maniphest.createtask

Reviewers: epriestley, #blessed_reviewers

Reviewed By: epriestley, #blessed_reviewers

Subscribers: epriestley, Korvin

Differential Revision: https://secure.phabricator.com/D10243
2014-08-12 14:07:27 -07:00
epriestley
c443913c0b Allow users to set notifications to "Email", "Notification", or "Ignore"
Summary:
Ref T5861. Ref T5769. If users don't care at all about something, allow them to ignore it.

We have some higher-volume notifications either built now (column changes) or coming (mentions) which users might reasonably want to ignore completely.

Test Plan:
Ignored some notifications, then took appropraite actions. Saw my user culled from the notification subscriber list.

{F189531}

Reviewers: chad, btrahan

Reviewed By: btrahan

Subscribers: epriestley

Maniphest Tasks: T5769, T5861

Differential Revision: https://secure.phabricator.com/D10240
2014-08-12 12:29:03 -07:00
epriestley
f6f9d78f3a Modularize mail tags
Summary:
Ref T5861. Currently, mail tags are hard-coded; move them into applications. Each Editor defines its own tags.

This has zero impact on the UI or behavior.

Test Plan:
  - Checked/unchecked some options, saved form.
  - Swapped back to `master` and saw exactly the same values.

Reviewers: chad, btrahan

Reviewed By: btrahan

Subscribers: epriestley

Maniphest Tasks: T5861

Differential Revision: https://secure.phabricator.com/D10238
2014-08-12 12:28:41 -07:00
epriestley
d011f8fdc6 Add a setting to disable all notification email
Summary: Ref T5861. Adds an option to opt out of all notification email. We'll still send you password resets, email verifications, etc.

Test Plan:
{F189484}

  - Added unit tests.
  - With preference set to different things, tried to send myself mail. Mail respected preferences.
  - Sent password reset email, which got through the preference.

Reviewers: btrahan, chad

Reviewed By: chad

Subscribers: rush898, epriestley

Maniphest Tasks: T5861

Differential Revision: https://secure.phabricator.com/D10237
2014-08-12 12:28:29 -07:00
epriestley
0196f53f9d Separate email formatting options into a new panel
Summary:
Ref T5861. These two options are complex, rarely useful, and not directly related to controlling what mail you receive.

Move them to a separate panel to make way for more stuff on the preferences panel. We'll probably add an "HTML" option to this new panel eventually, too.

Test Plan:
{F189474}

  - Used both panels.
  - Tested with multiplexing off.

Reviewers: btrahan, chad

Reviewed By: chad

Subscribers: epriestley

Maniphest Tasks: T5861

Differential Revision: https://secure.phabricator.com/D10236
2014-08-12 12:28:17 -07:00
epriestley
500506bfef Persist excluded recipients when saving mail
Summary:
Fixes T5185. The fundamental issue is that this `excludePHIDs` property was not saved, so the logic went like this:

  - Generate `excludePHIDs` correctly.
  - Pass `excludePHIDs` through the stack.
  - Perform some other computations correctly.
  - Queue the mail for the daemons, throwing it away. {icon bomb}
  - Daemons process mail with empty `excludePHIDs` list.

Store it in the persistent properties array instead.

Also remove the "override self mail" thing, since it's only used by `bin/mail send-test` and suffers from the same issue. I think it's too useless to fix, since even if you get caught by it, `bin/mail` makes it clear why the message was dropped.

Test Plan:
Notable:

  - `exclude` present in properties
  - Exclusion reason under RECIPIENTS header

{P1229}

Reviewers: btrahan

Reviewed By: btrahan

Subscribers: epriestley

Maniphest Tasks: T5185

Differential Revision: https://secure.phabricator.com/D10234
2014-08-12 12:28:07 -07:00
epriestley
c9835c4492 Publish stories about objects in projects as related to projects
Summary:
Fixes T5456. We lost this logic in the transition to applicationtransactions.

When publishing a feed story, mark all of the object's projects as related, so the project filter in feed works.

Test Plan: Made a comment on a task associated with a project, saw the story in filtered feed.

Reviewers: btrahan

Reviewed By: btrahan

Subscribers: timor, epriestley

Maniphest Tasks: T5456

Differential Revision: https://secure.phabricator.com/D10233
2014-08-12 12:27:24 -07:00
epriestley
e4049e8797 Fix verbose email addresses being passed to mail adapters
Summary:
Fixes T5233.

  - The mail adapter API currently expects plain addresses (like `a@b.com`) in `addTos()`, and some adapters can not accept fancy verbose addresses (like `"name" <a@b.com>`).
  - When we try to send error email, we pass the entire "From" header into the API. This is incorrect.
  - Since it would be nice to make this just work in the future, fix it inside the API.
  - Specifically, this is reached with: send email -> generates error -> we try to send you an email back -> we send it to your "From" -> some mailers choke on the fancy name if you have one.

Test Plan: Processed an errorneous email with a fancy "From", got a response error.

Reviewers: btrahan

Reviewed By: btrahan

Subscribers: epriestley

Maniphest Tasks: T5233

Differential Revision: https://secure.phabricator.com/D10232
2014-08-12 12:27:13 -07:00
epriestley
dedcfd0c91 Fix broken handle rendering in Ponder
Summary: Ref T5817. This just fixes the markup in emails, the overall behavior still isn't great. I don't want to spend to much time on Ponder until it ends up somewhere nearer the top of the priority queue.

Test Plan: Viewed feed stories and emails, no stray/clearly-broken HTML.

Reviewers: btrahan

Reviewed By: btrahan

Subscribers: epriestley

Maniphest Tasks: T5817

Differential Revision: https://secure.phabricator.com/D10231
2014-08-12 12:25:58 -07:00
epriestley
a3a72c1c7d Use transactions properly when building tasks from email
Summary: Fixes T5859. This doesn't change much, but makes the transaction record a little more accurate and activates stuff like `#hashtags` and `{F123}` causing policy associations.

Test Plan: Used `bin/mail receive-test` and mail receiver script to send bug mail, saw hashtags imply projects.

Reviewers: btrahan

Reviewed By: btrahan

Subscribers: epriestley

Maniphest Tasks: T5859

Differential Revision: https://secure.phabricator.com/D10229
2014-08-12 12:25:47 -07:00
epriestley
ec9eaabfbd Allow repo updates to recover after force push + garbage collection
Summary:
Fixes T5839. If a repository has been force pushed and garbage collected, we might have a ref cursor in the database which still points at the old commit (which no longer exists).

We'll then run a command like `git log <new hash> --not <old hash>` to figure out which commits are newly pushed, and this will bomb out because `<old hash>` is invalid.

Instead, validate all the `<old hash>` values before we try to make use of them.

Test Plan:
  - Forced a repository into a bad state by mucking with the datbase, generating a reproducible failure similar to the one in T5839.
  - Applied patch.
  - `bin/repository update <callsign> --trace` filtered the bad commit and put the repository into the right state.
  - Saw new commits recognized correctly.
  - Ran `bin/repository update <callsign>` for a Mercurial and SVN repo as a sanity check.

Reviewers: btrahan

Reviewed By: btrahan

Subscribers: epriestley

Maniphest Tasks: T5839

Differential Revision: https://secure.phabricator.com/D10226
2014-08-12 12:25:24 -07:00
epriestley
394250397e Improve "unblock task" feed stories
Summary:
Fixes T5184. Fixes T5008. Three issues with stories/notifications about changing the status of tasks which block other tasks:

**Bad Feed Stories**

  - Problem: Feed story rendering was confusing (T5184).
  - Solution: fix it to provide context.

**Too Many Feed Stories**

  - Problem: Feed gets a story for the original task's close ("a closed x"), and a story for each blocked task ("a closed x, a task blocking y").
  - "Solution": Punt. These are redundant in the full feed but not in filtered feeds. Right solution is display-time aggregation. No users have really complained about this.

**Too Many Notifications**

  - Problem: Users subscribed to both tasks get notified about the clsoe, and also about the unblocked task. These notifications are redundant.
  - "Solution": Punt. This is easy to fix by silencing notifications for the sub-editor, but I'm worried it would be confusing. Users haven't complained. Display-time aggregation might be a better fix.

Test Plan: {F189463}

Reviewers: btrahan, chad

Reviewed By: chad

Subscribers: epriestley

Maniphest Tasks: T5008, T5184

Differential Revision: https://secure.phabricator.com/D10235
2014-08-12 09:27:26 -07:00
epriestley
94389fcd9f Allow projects to be filtered by icon and color
Summary: Ref T5819. Implements basic icon and color filtering for projects.

Test Plan: {F189350}

Reviewers: btrahan, chad

Reviewed By: chad

Subscribers: epriestley

Maniphest Tasks: T5819

Differential Revision: https://secure.phabricator.com/D10230
2014-08-12 08:04:38 -07:00
epriestley
9309723ac4 Send graceful shutdown signals to daemons in Phabricator
Summary:
Fixes T5855. Adds a `--graceful N` flag to `phd stop` and `phd restart`.

`phd` will send SIGINT, wait `N` seconds, SIGTERM, wait 15 seconds, and SIGKILL. By default, `N` is 15.

Test Plan:
  - Ran `bin/phd debug ...` and used `^C` to interrupt daemons. Saw graceful shutdown behavior, and abrupt termination on multiple `^C`.
  - Ran `bin/phd start`, `bin/phd stop` and `bin/phd restart` with `--graceful` set to various things, notably `0`. Saw graceful shutdowns on the CLI and in the web UI. With `0`, abrupt shutdowns.

Reviewers: btrahan, hach-que

Reviewed By: hach-que

Subscribers: epriestley

Maniphest Tasks: T5855

Differential Revision: https://secure.phabricator.com/D10228
2014-08-11 20:18:31 -07:00
James Rhodes
aab0ed1c50 Implement artifact release for Harbormaster
Summary: Resolves T5836.  This automatically releases artifacts when Harbormaster builds finish (either passing or failing).  This allows Harbormaster to release the Drydock leases it has for hosts.

Test Plan: Tested it with a build plan that passes and fails; tested it with lots of builds running in parallel.

Reviewers: #blessed_reviewers, epriestley

Reviewed By: #blessed_reviewers, epriestley

Subscribers: epriestley, Korvin

Maniphest Tasks: T5836

Differential Revision: https://secure.phabricator.com/D10208
2014-08-12 09:15:16 +10:00
James Rhodes
d2111214f2 Allow timeouts to be specified on Drydock SSH connections
Summary: This allows timeouts to be specified on SSH connections that Drydock makes.  Used in the EC2 allocator to poll for the SSH server starting.

Test Plan: Used in EC2 allocator diff.

Reviewers: epriestley, #blessed_reviewers

Reviewed By: epriestley, #blessed_reviewers

Subscribers: epriestley, Korvin

Differential Revision: https://secure.phabricator.com/D10225
2014-08-12 09:14:47 +10:00
James Rhodes
e48aaa563a Allow Drydock blueprints to define and use custom fields
Summary: This allows Drydock blueprints to define custom fields for blueprint settings.

Test Plan: Pulled out of EC2 allocator diff.

Reviewers: epriestley, #blessed_reviewers

Reviewed By: epriestley, #blessed_reviewers

Subscribers: epriestley, Korvin

Differential Revision: https://secure.phabricator.com/D10224
2014-08-12 08:39:00 +10:00
James Rhodes
efc82c727b Measure how long build targets take in Harbormaster
Summary:
Ref T1049.  This keeps track of how long a build target takes to execute in Harbormaster and displays it in the build view page.  I'm not sure whether "Started" is really that useful once the target has completed?

Also, I change the name of the time taken depending on whether or not the target has completed; if it's still in progress it's called "Elapsed" and if it's completed then it's "Duration".  The primary reason for this is that "Duration" sounds like post tense, whereas "Elapsed" is current tense.  I'm not sure whether this is okay or not?

Test Plan: Ran a Sleep build step and saw the target dates / times appear correctly.

Reviewers: epriestley, #blessed_reviewers

Reviewed By: epriestley, #blessed_reviewers

Subscribers: talshiri, epriestley, Korvin

Maniphest Tasks: T5824, T1049

Differential Revision: https://secure.phabricator.com/D10174
2014-08-12 08:34:43 +10:00
Caleb Anderson
60fca1d7f4 Updated PhabricatorManiphestTaskTestDataGenerator to assign projects to the generated tasks.
Summary: To assist with {T5245}, I have added projects back into the lipsum maniphest generator with the edge infrastructure.

Test Plan: Run the lipsum script for PhabricatorManiphestTaskTestDataGenerator and make sure it generates project data.

Reviewers: #blessed_reviewers, epriestley

Reviewed By: #blessed_reviewers, epriestley

Subscribers: epriestley, Korvin

Differential Revision: https://secure.phabricator.com/D10202
2014-08-11 13:27:33 -07:00
epriestley
0292793d4d Account for preempting events on the Phrequent list view
Summary: Fixes T5850. Also fixes some logic where the wrong preempting events could be attached during a bulk query.

Test Plan: Phrequent list now shows preemption-aware times.

Reviewers: hach-que, btrahan

Reviewed By: btrahan

Subscribers: epriestley

Maniphest Tasks: T5850

Differential Revision: https://secure.phabricator.com/D10223
2014-08-11 12:30:48 -07:00
epriestley
fc814647e6 Improve usability of Phrequent "Stop Time" dialog
Summary:
Fixes T5848.

  - Disallow tracking negative time.
  - Preserve note if there's an error with the time selection.
  - Show start time and duration.
  - Slightly better error messages.

Test Plan: Started and stopped time. Tried to select future/negative ranges.

Reviewers: hach-que, btrahan

Reviewed By: btrahan

Subscribers: epriestley

Maniphest Tasks: T5848

Differential Revision: https://secure.phabricator.com/D10218
2014-08-11 12:30:01 -07:00
epriestley
2b909fb3a1 Minor, fix one more Actor -> Acting As PHID
Summary: See D10221, I missed one of these callsites.

Auditors: btrahan
2014-08-11 12:20:25 -07:00
epriestley
6232e9676c Don't send reset links to unverified addresses on accounts with verified addresses
Summary:
Via HackerOne. If a user adds an email address and typos it, entering `alinculne@gmailo.com`, and it happens to be a valid address which an evil user controls, the evil user can request a password reset and compromise the account.

This strains the imagination, but we can implement a better behavior cheaply.

  - If an account has any verified addresses, only send to verified addresses.
  - If an account has no verified addresses (e.g., is a new account), send to any address.

We've also received several reports about reset links not being destroyed as aggressively as researchers expect. While there's no specific scenario where this does any harm, revoke all outstanding reset tokens when a reset link is used to improve the signal/noise ratio of the reporting channel.

Test Plan:
  - Tried to send a reset link to an unverified address on an account with a verified address (got new error).
  - Tried to send a reset link to a verified adddress on an account with a verified address (got email).
  - Tried to send a reset link to an invalid address (got old error).
  - Tried to send a reset link to an unverified address on an account with only unverified addresses -- a new user (got email).
  - Requested several reset links, used one, verified all the others were revoked.

Reviewers: btrahan

Reviewed By: btrahan

Subscribers: epriestley

Differential Revision: https://secure.phabricator.com/D10206
2014-08-11 12:13:09 -07:00
epriestley
7513c70e2c Show a very basic purchase history in Phortune
Summary: Ref T2787. This is very basic and just helps me know that the data is inserting correctly.

Test Plan: {F187765}

Reviewers: btrahan

Reviewed By: btrahan

Subscribers: epriestley

Maniphest Tasks: T2787

Differential Revision: https://secure.phabricator.com/D10205
2014-08-11 12:10:09 -07:00
epriestley
d38e89ef6b Fix several issues with application interactions while importing commits
Summary:
  - Fixes T5851. Currently, if a commit has `Fixes T123`, we generate an email with just that before generating the commit email. Don't send/publish transactions about a commit before it imports (this is a tiny bit hacky, but well-contained and I don't think it causes any problems).
  - Fixes T4864. Currently, we try to parse Differential information even if Differential is not installed. Instead, do this only if Differential is installed.
  - Fixes T5771. Currently, if we can't figure out who the committer/author of a commit is, we don't publish a `Fixes T123` transaction. Instead, fall back to acting as "Diffusion" if we can't find a better actor. Most of this diff expands the role of application actors. The existing application actors (Herald and Harbormaster) seem to be working well.

Test Plan:
  - Pushed a commit with `Fixes T123` and verified it did not generate email directly. (The task half of the transaction still does, correctly.)
  - Uninstalled Differential and pushed a commit, got a clean import instead of an exception.
  - Commented out author/committer PHIDs and pushed stuff, saw a "Diffusion" actor.

Reviewers: btrahan

Reviewed By: btrahan

Subscribers: epriestley

Maniphest Tasks: T5771, T4864, T5851

Differential Revision: https://secure.phabricator.com/D10221
2014-08-11 12:08:24 -07:00
epriestley
d09d7ffe1f Fix string construction in Conduit exceptions
Summary:
Fixes T5838.

  - We currently try to use a `ConduitAPIMethod` object as a string.
  - We then pass that string to the parent's `__construct()` method as `$message`.

Test Plan: Uninstalled Maniphest, then tried to execute `maniphest.createtask`. Got a useful exception message instead of an error during message construction.

Reviewers: joshuaspence, btrahan

Reviewed By: btrahan

Subscribers: epriestley

Maniphest Tasks: T5838

Differential Revision: https://secure.phabricator.com/D10211
2014-08-11 12:08:06 -07:00
epriestley
aa67a5ffc8 Make payment method management somewhat more reasonable in Phortune
Summary: Ref T2787. Shows somewhat-useful information, allows payment methods to be disabled and renamed.

Test Plan: Created, renamed, disabled payment methods.

Reviewers: btrahan

Reviewed By: btrahan

Subscribers: epriestley

Maniphest Tasks: T2787

Differential Revision: https://secure.phabricator.com/D10203
2014-08-11 12:07:35 -07:00
epriestley
eb9dcd6fba Consolidate handling of special properties for newly uploaded files
Summary:
Fixes T5849. When a new file is created, we might have to actually write the data to a storage engine, or we might be able to just point at data which is already there.

Currently, these two paths handle `$params` with different code and mild behavioral differences. Instead, have them call the same code so they get the same behavior.

Test Plan:
  - Uploaded the same file multiple times to home page.
  - Uploaded the same file multiple times as profile picture.
  - Generated files via Diffusion.
  - All the files got the expected properties, whether they were reusing data or not.

Reviewers: btrahan, 20after4

Reviewed By: 20after4

Subscribers: epriestley

Maniphest Tasks: T5849

Differential Revision: https://secure.phabricator.com/D10216
2014-08-11 09:39:40 -07:00
epriestley
a9f2c07345 Generate a 403 page with a nice dialog when a file token is invalid
Summary:
Ref T5685. Currently we just 403 on an invalid token, but we can be a little more helpful.

The issues here are:

  - If we **do** redirect you on this page and something goes wrong, you might get stuck in a redirect loop.
  - If we **don't** redirect you, copy/pasting the link to someone (or reloading the page) gives them a pretty confusing result, since the link doesn't work any more. Prior to this diff, they get a 403.

To mitigate this, do a little better than a bare 403: give them a link to auth and generate a new URI for the file.

If this is still confusing, the next best thing I can come up with is something like this:

  - Put some modulous of the timestamp in the URI.
  - If the current time is within 2 seconds of the generation time, show this dialog.
  - Otherwise, redirect.

That seems like it would be okay, but I worry that "2" has to be small (so links you copy/paste -> chat -> click still work) and a small value means that a small amount of clock skew breaks things. We could use the database clock, but ehhh.

Other ideas:

  - Put a hash of the remote IP in the URI, redirect if it doesn't match. Fails for companies behind a NAT gateway but should work in a lot of other cases.
  - Just redirect always, there's no reason it should ever loop and browsers don't really do anything bad when there's a loop (they'll show an error after too many redirects).

I'm leaning toward letting this stabilize in the wild for a bit, then trying "always redirect".

Test Plan: {F188914}

Reviewers: btrahan, 20after4

Reviewed By: 20after4

Subscribers: epriestley

Maniphest Tasks: T5685

Differential Revision: https://secure.phabricator.com/D10215
2014-08-11 09:39:25 -07:00
epriestley
5a630f84de Show file cacheability in Files application
Summary: Ref T5685. We've added a new `canCDN` flag to control whether or not files can be cached and delivered over a CDN. Show this flag in the UI.

Test Plan: Viewed several files, saw correct/expected UI values.

Reviewers: btrahan, 20after4

Reviewed By: 20after4

Subscribers: epriestley

Maniphest Tasks: T5685

Differential Revision: https://secure.phabricator.com/D10213
2014-08-11 09:39:06 -07:00
Mukunda Modell
25ae4c458d Protect file data with a one-time-token
Test Plan: currently untested work in progress

Reviewers: #blessed_reviewers, epriestley

Subscribers: rush898, aklapper, Korvin, epriestley

Projects: #wikimedia

Maniphest Tasks: T5685

Differential Revision: https://secure.phabricator.com/D10054
2014-08-11 07:32:17 -07:00
epriestley
c0919be0ec Fix dashboard list if there are no results
Summary:
We'll fire a bad query if there are no dashboards in the result list, see:

http://pastie.org/private/j0f8tzbdahwragxjsk8qxq

Test Plan: Viewed result list with no dashboards.

Reviewers: btrahan, chad

Reviewed By: chad

Subscribers: epriestley

Differential Revision: https://secure.phabricator.com/D10207
2014-08-09 19:08:16 -07:00
epriestley
c0dc5ca898 Clean up remaining default column logic for boards
Summary: See D10189. We should never hit this anymore, so clean it up.

Test Plan:
  - Reloaded a board, saw everything stay where it was before the change.
  - Added a new task to the project, saw it show up in backlog.

Reviewers: btrahan

Reviewed By: btrahan

Subscribers: epriestley

Differential Revision: https://secure.phabricator.com/D10200
2014-08-08 16:22:11 -07:00
epriestley
664507e450 Use ManiphestTaskQuery instead of ad-hoc load in Maniphest reports
Summary: Fixes T5829. This stuff is old and busted, but keep it working for now.

Test Plan: No more fatal when there are recently closed tasks.

Reviewers: btrahan

Reviewed By: btrahan

Subscribers: epriestley

Maniphest Tasks: T5829

Differential Revision: https://secure.phabricator.com/D10201
2014-08-08 16:21:53 -07:00
epriestley
24a6eeb8d8 Allow the workboard backlog column to be reordered
Summary:
Fixes T5677.

  - Instead of using `sequence == 0` to mean "this is the backlog column", flag the column explicitly.
  - Migrate existing sequence 0 columns to have the flag.
  - Add the flag when initializing or copying a board.
  - Remove special backlog logic when reordering columns.

Test Plan:
  - Migrated columns, viewed some boards, they looked identical.
  - Reordered the backlog column a bunch of times (first, last, middle, dragged other stuff around).
  - Added tasks to a project, saw them show up in the reordered backlog.
  - Initialized a new board and saw a backlog column show up.
  - Copied an existing board and saw the backlog column come over.
  - Tried to hide a backlog column.

Reviewers: btrahan

Reviewed By: btrahan

Subscribers: epriestley

Maniphest Tasks: T5677

Differential Revision: https://secure.phabricator.com/D10189
2014-08-08 15:50:36 -07:00
epriestley
0e7b4b0277 Fix "legalpad documents" typeahead dataousource
Summary: This slipped through the datasource modernization stuff.

Test Plan: Used search UI.

Reviewers: rush898, btrahan, chad

Reviewed By: chad

Subscribers: epriestley

Differential Revision: https://secure.phabricator.com/D10196
2014-08-08 11:38:18 -07:00
epriestley
abfff87f26 Convert workboard column options into a dropdown menu
Summary:
Ref T5024, T4427, T5474, T5523. Instead of separate icons in the column header for "Create Task" and "Edit Column Settings", use a dropdown menu.

  - T5024 will likely add a "View Standalone" option.
  - T4427 needs header space to show a count.
  - T5474 likely needs "Edit Triggers..." (this seems reasonable to separate from editing the name, etc.)
  - T5523 likely adds "Move all tasks..." eventually.

Test Plan: {F187414}

Reviewers: btrahan, chad

Reviewed By: chad

Subscribers: epriestley

Maniphest Tasks: T5523, T5474, T5024, T4427

Differential Revision: https://secure.phabricator.com/D10190
2014-08-08 10:35:51 -07:00
Chad Little
417b6bbe41 Set Flush on a few Dialogs
Summary: Sets layout as flush when rendering diff table or timeline in a Dialog

Test Plan: Tested each

Reviewers: epriestley

Reviewed By: epriestley

Subscribers: epriestley, Korvin

Differential Revision: https://secure.phabricator.com/D10194
2014-08-08 10:21:13 -07:00
Chad Little
42258ce0be Remove "Edit" text on Phame
Summary: Fixes T5731

Test Plan: Load list of self-created posts

Reviewers: btrahan, epriestley

Reviewed By: epriestley

Subscribers: epriestley, Korvin

Maniphest Tasks: T5731

Differential Revision: https://secure.phabricator.com/D10192
2014-08-08 10:02:22 -07:00
epriestley
a1477baa39 Fix Legalpad for logged-out users
Summary: Fixes T5739. I only got D9857 half right: the new method names are correct, but the bodies needed to change too.

Test Plan: Signed a document as an anonymous user.

Reviewers: btrahan, chad

Reviewed By: chad

Subscribers: epriestley

Maniphest Tasks: T5739

Differential Revision: https://secure.phabricator.com/D10191
2014-08-08 09:53:49 -07:00
cpettet
57eb3aecb6 allow phids other than users (i.e. mailinglists)
Summary:
Via the UI adding a mailinglist for CC works, but via
the API currently it shows:

>One or more PHIDs were invalid for ccPHIDS

This removes the user validation check for ccPHIDs.

(I left it in for other things like owner since that seems
still appropriate?)

Test Plan:
used arc locally to add a mailinglist to cc

```echo '{"id": 2, "ccPHIDs": ["PHID-MLST-ohduchbv4dfimk7opt3r"]}' | arc call-conduit maniphest.update```

Reviewers: #blessed_reviewers, epriestley

Reviewed By: #blessed_reviewers, epriestley

Subscribers: epriestley, Korvin

Differential Revision: https://secure.phabricator.com/D10193
2014-08-08 09:53:35 -07:00
epriestley
59a85e8845 Support natural ordering of workboards
Summary:
Ref T4807. This is probably a complete fix, but I'd be surprised if there isn't a little cleanup I missed.

When users drag tasks on a "natural"-ordered workboard, leave things where they put them.

This isn't //too// bad since a lot of the existing work is completely reusable (e.g., we don't need any new JS).

Test Plan:
  - Dragged a bunch of stuff around, it stayed where I put it after dropped and when reloaded.
  - Dragged stuff across priorities, no zany priority changes (in "natural" mode).
  - Created new tasks, they show up at the top.
  - Tagged new tasks, they show up at the top of backlog.
  - Swapped to "priority" mode and got sorting and the old priority-altering reordering.
  - Added tasks in priority mode.
  - Viewed task transactions for correctness/sanity.

Reviewers: btrahan, chad

Reviewed By: chad

Subscribers: chad, epriestley

Maniphest Tasks: T4807

Differential Revision: https://secure.phabricator.com/D10182
2014-08-08 08:11:00 -07:00
epriestley
043e0db8d3 When selecting implicit column positions, actually create them
Summary:
Ref T4807. This is an alternative to D10179. The problem these diffs solve is that I want to be able to reorder a column's positions without having to load the actual objects, but that's difficutl because two positions may have the same sequence number (and I think it's good that we allow that, since it makes a bunch of other stuff way easier).

Instead of using the object ID (e.g., the task ID) to reorder positions with the same sequence, use the position itself. This is a little easier, is less ambiguous if columns eventually have several types of objects, and produces a better behavior when old objects are freshly added to a board. For example, if you tag `T300` with `#project`, this new rule will push it to the top of "Backlog" while the old rule might have buried it deep. I think this behavior is desirable and more "natural".

When creating a group of new rows, we do order the batch by ID, so a group of freshly-tagged objects float to the top togehter in ID order. This seems like the most natural rule, too.

Test Plan:
  - Loaded some boards with implicit objects on them (freshly tagged tasks) and saw rows create.
  - Verified new rows created in the right order.
  - Dragged some tasks around.

Reviewers: btrahan

Reviewed By: btrahan

Subscribers: epriestley

Maniphest Tasks: T4807

Differential Revision: https://secure.phabricator.com/D10180
2014-08-08 08:10:49 -07:00
epriestley
fdf6b56261 Add UI for alternate board ordering rules
Summary:
Ref T4807. This doesn't actually do anything yet, but adds a dropdown menu for choosing an ordering and gets all the UI working correctly.

This also fixes a bug where column hidden state wouldn't persist across filter changes.

(I won't land this until it does something, but the next diff will probably be a mess so this seemed like a clean place to sever things.)

Test Plan:
{F187114}

  - Altered sort ordering.
  - Altered hidden state and filters, verified all states persisted correctly.
  - Added `phlog()` to edit/create and move controllers and verified they receive sort information.

Reviewers: btrahan, chad

Reviewed By: chad

Subscribers: swisspol, chad, epriestley

Maniphest Tasks: T4807

Differential Revision: https://secure.phabricator.com/D10178
2014-08-08 08:10:29 -07:00
Mukunda Modell
12aaa942ac Add a CanCDN flag to uploaded files
Summary:
CanCDN flag indicates that a file can be served + cached
via anonymous content distribution networks.

Once D10054 lands, any files that lack the CanCDN flag
will require a one-time-use token and headers will
prohibit cache to protect sensitive files from
unauthorized access.

This diff separates the CanCDN changes from the code that
enforces these restrictions in D10054 so that the changes
can be tested and refined independently.

Test Plan: Work in progress

Reviewers: #blessed_reviewers, epriestley

Reviewed By: #blessed_reviewers, epriestley

Subscribers: rush898, qgil, epriestley, aklapper, Korvin

Maniphest Tasks: T5685

Differential Revision: https://secure.phabricator.com/D10166
2014-08-07 18:56:20 -07:00
epriestley
c0585b7a34 Fix Phrequent duration accounting
Summary: Fixes T5705. This was just derp; instead of returning the duration of the first slice, return the duration of all the slices.

Test Plan: Added unit tests. Saw reasonable results in the UI.

Reviewers: btrahan, hach-que

Reviewed By: hach-que

Subscribers: epriestley

Maniphest Tasks: T5705

Differential Revision: https://secure.phabricator.com/D10184
2014-08-07 17:05:14 -07:00
lkassianik
7204f9fec2 T5423, "is newly created" herald rule fails on dry runs
Summary: Fixes T5423, "is newly created" herald rule fails on dry runs

Test Plan: Create herald "is newly created" rule, and do a dry run on an existing pholio mock, differential commit, or maniphest task. Should not return an exception.

Reviewers: #blessed_reviewers, epriestley

Reviewed By: #blessed_reviewers, epriestley

Subscribers: epriestley, Korvin

Maniphest Tasks: T5423

Differential Revision: https://secure.phabricator.com/D10187
2014-08-07 17:04:41 -07:00
lkassianik
c6998207ab T5409, allow bin/remove to permanently destroy credential and everything associated with it
Summary: Fixes T5409, bin/remove permanently destroys credential

Test Plan: Add a passphrase, run bin/remove destroy K123 --trace, verify credential no longer exists

Reviewers: #blessed_reviewers, epriestley

Reviewed By: #blessed_reviewers, epriestley

Subscribers: epriestley, Korvin

Maniphest Tasks: T5409

Differential Revision: https://secure.phabricator.com/D10185
2014-08-07 16:33:54 -07:00
epriestley
20d8b1bdd3 Implement PhabricatorProjectInterface on ManiphestTask
Summary:
Ref T5245. This removes some hacks and activates two meaningful interactions:

  - The "projects" field goes through shared code now.
  - Mentioning projects in tasks using hashtags now tags them.

Test Plan:
  - Viewed a task with projects.
  - Viewed a task with no projects.
  - Viewed a task with projects and board positions.
  - Viewed a revision with projects.
  - Made a `#hashtag` comment in Maniphest and got a project association.

Reviewers: btrahan

Reviewed By: btrahan

Subscribers: epriestley

Maniphest Tasks: T5245

Differential Revision: https://secure.phabricator.com/D10177
2014-08-07 15:44:12 -07:00
James Rhodes
3785f8113e Allow build steps to create URI artifacts
Summary: Ref T1049.  This allows build steps to create URI artifacts, which can be used to link to external builds and other resources.

Test Plan: Used a build step in an external library to test the creation of a URI artifact and verified it appeared correctly.

Reviewers: epriestley, #blessed_reviewers

Reviewed By: epriestley, #blessed_reviewers

Subscribers: epriestley, Korvin

Maniphest Tasks: T1049

Differential Revision: https://secure.phabricator.com/D10173
2014-08-08 08:42:36 +10:00
James Rhodes
bc116d7e02 Change "Stop" to "Pause" in Harbormaster build UI
Summary: Resolves T5814.  Ref T1049.  This changes "Stop" to "Pause" in the UI (internally it's still referred to as Stop).

Test Plan: Viewed builds and saw the intended wording.

Reviewers: #blessed_reviewers, epriestley

Reviewed By: #blessed_reviewers, epriestley

Subscribers: epriestley, Korvin

Maniphest Tasks: T1049, T5814

Differential Revision: https://secure.phabricator.com/D10172
2014-08-08 08:25:04 +10:00
Bob Trahan
27d44594dc Transactions - add "view raw" action
Summary: Use cutlery icon for hilarity. Ref T5768.

Test Plan: made something with remarkup in it, used 'view raw' and saw the remarkup raw in a nice little dialogue.

Reviewers: epriestley

Reviewed By: epriestley

Subscribers: epriestley, Korvin

Maniphest Tasks: T5768

Differential Revision: https://secure.phabricator.com/D10183
2014-08-07 15:21:32 -07:00
Bob Trahan
7388351aab Add "Installed" icon to Dashboard list view.
Summary: Fixes T5478. For "personal" installs use the person icon; for global use the global icon. For both providing explanatory tooltip text about what's going on. This will need to be updated if / when we start installing dashboards to other applications. Also, this query isn't 100% optimized but the major part *is* so I think its okay.

Test Plan: Installed a dashboard for personal use and verified correct icon / text showed up. Did the same for global installed dashboard...!

Reviewers: epriestley

Reviewed By: epriestley

Subscribers: epriestley, Korvin

Maniphest Tasks: T5478

Differential Revision: https://secure.phabricator.com/D10181
2014-08-07 14:31:02 -07:00
cpettet
6a69b4699e file.upload set policy explicitly
Summary:
This is pretty basic allowing a user to set the
policy as a valid string ('no-one' or 'users') or
as a valid PHID.  Without an explicit policy
a permissive one is set.

Test Plan:
Tested using the python-phabricator module (very basic api wrapper).

The arc cli syntax was evading me.

```import base64
from phabricator import Phabricator
phab = Phabricator()
with open('mypic.jpg') as f:
    encoded = base64.b64encode(f.read())

//set no-one as viewer which really means author only?
phab.file.upload(name='mypicnoone.jpg',
                 data_base64=encoded,
                 viewPolicy='no-one')

//set a specific phid as policy in this case a project
phab.file.upload(name='mypicphid.jpg',
                 data_base64=encoded,
                 viewPolicy='PHID-PROJ-fgvvnafmhvkgn2d5a4rf')

//no set policy ends up as 'users' i.e. ('all users')
phab.file.upload(name='mypicdefault.jpg', data_base64=encoded)```

Not able to really test canCDN attribute but it should be
fine and I tried to make it all consistent with D10166

Reviewers: 20after4, epriestley, #blessed_reviewers

Reviewed By: epriestley, #blessed_reviewers

Subscribers: 20after4, epriestley, Korvin

Maniphest Tasks: T5685

Differential Revision: https://secure.phabricator.com/D10164
2014-08-07 12:14:17 -07:00
epriestley
57c1e0cc4e Correct typo: security.alter[n]ate-file-domain
Summary: Minor correction to correct spelling of alternate (the 'n' was missing).

Test Plan: reviewer to verify correct spelling

Reviewers: #blessed_reviewers, epriestley

Reviewed By: #blessed_reviewers, epriestley

Subscribers: epriestley

Differential Revision: https://secure.phabricator.com/D10175
2014-08-07 09:41:20 -07:00
Bob Trahan
1bd714f8ac Macro - allow view controller to be viewed publicly
Summary: Fixes T5735, setting up Phacility for huge financial success.

Test Plan:
opened up Safari - who logs in with Safari anyway? - and could still view a macro
could also view the list of macros

Reviewers: epriestley

Reviewed By: epriestley

Subscribers: epriestley, Korvin

Maniphest Tasks: T5735

Differential Revision: https://secure.phabricator.com/D10170
2014-08-06 15:20:33 -07:00
epriestley
09868271bd Move board relationships to dedicated storage
Summary:
Fixes T5476. Using edges to store which objects are on which board columns ends up being pretty awkward. In particular, it makes T4807 very difficult to implement.

Introduce a dedicated `BoardColumnPosition` storage.

This doesn't affect ordering rules (T4807) yet: boards are still arranged by priority. We just read which tasks are on which columns out of a new table.

Test Plan:
  - Migrated data, then viewed some boards. Saw exactly the same data.
  - Dragged tasks from column to column.
  - Created a task directly into a column.

Reviewers: btrahan

Reviewed By: btrahan

Subscribers: epriestley

Maniphest Tasks: T5476

Differential Revision: https://secure.phabricator.com/D10160
2014-08-06 15:09:09 -07:00
Bob Trahan
95ef72e4f9 Events - add a byline to event list
Summary: so you can see who the event is about...! Fixes T5621.

Test Plan: saw the creator of each event on /calendar/event/query/all/

Reviewers: epriestley

Reviewed By: epriestley

Subscribers: epriestley, Korvin

Maniphest Tasks: T5621

Differential Revision: https://secure.phabricator.com/D10169
2014-08-06 15:04:12 -07:00
Bob Trahan
50a393913c Slowvote - add ability to destroy a poll
Summary: Fixes T5773.

Test Plan: Made a poll and voted on it. Deleted it via ./bin/remove destory V1. No errors and the poll is gone.

Reviewers: epriestley

Reviewed By: epriestley

Subscribers: epriestley, Korvin

Maniphest Tasks: T5773

Differential Revision: https://secure.phabricator.com/D10167
2014-08-06 14:19:37 -07:00
Bob Trahan
20a65b21eb Settings - upgrade monospace font regexp to support '.'
Summary: this data is a little weird since its user-entered and we need to put it in a web page un-escaped for the font to load correctly. Ergo, we use a regex to make the input safe / sane, and said regex needs to support a '.'.  Fixes T5810.

Test Plan: added Fixedsys Excelsior 3.01 to my system and was able to set my preference and get the new font

Reviewers: epriestley

Reviewed By: epriestley

Subscribers: dereckson, epriestley, Korvin

Maniphest Tasks: T5810

Differential Revision: https://secure.phabricator.com/D10163
2014-08-06 13:53:30 -07:00
James Rhodes
9c1c4bb5ae Move artifacts and build target messages into tabs
Summary: This moves artifacts and build target messages into tabs.

Test Plan: Viewed build plan, saw the tabs appear when the steps had appropriate artifacts and / or messages.

Reviewers: #blessed_reviewers, epriestley, chad

Reviewed By: #blessed_reviewers, epriestley

Subscribers: epriestley, Korvin

Differential Revision: https://secure.phabricator.com/D10161
2014-08-06 10:34:39 +10:00
James Rhodes
cefe30d737 Hide empty build logs
Summary: This automatically hides any empty build logs from Harbormaster, so that they do not appear.

Test Plan: Viewed a build plan where the logs were empty and didn't see them appear.

Reviewers: chad, #blessed_reviewers, epriestley

Reviewed By: #blessed_reviewers, epriestley

Subscribers: epriestley, Korvin

Differential Revision: https://secure.phabricator.com/D10091
2014-08-06 10:28:13 +10:00
Joshua Spence
4e9746ed4e Rename PhutilKeyValueCache subclasses
Summary: Ref T5655. Depends on D10155.

Test Plan: Ran `arc unit`

Reviewers: epriestley, #blessed_reviewers

Reviewed By: epriestley, #blessed_reviewers

Subscribers: epriestley, Korvin

Maniphest Tasks: T5655

Differential Revision: https://secure.phabricator.com/D10156
2014-08-06 08:12:28 +10:00
Joshua Spence
8fd098329b Rename AphrontQueryException subclasses
Summary: Ref T5655. Depends on D10149.

Test Plan: Ran `arc unit`

Reviewers: epriestley, #blessed_reviewers

Reviewed By: epriestley, #blessed_reviewers

Subscribers: epriestley, Korvin, hach-que

Maniphest Tasks: T5655

Differential Revision: https://secure.phabricator.com/D10150
2014-08-06 07:51:21 +10:00
Bob Trahan
21dca29c5f Workboards - add new "initialization" flow
Summary: Currently, we just create a default "backlog" column if / when you visit a workboard for the first time. Post this patch, instead you see a blocking dialog that lets you either create the default backlog column or import columns from another project. In the case of the latter, the user gets another dialog which lets them select any project of which they are a member that also has columns in it. Note that only not hidden columns get imported. Fixes T4431.

Test Plan:
- made a new workboard and got my new dialog. made a default backlog and it worked!
- made a new workboard again and tried the import flow - it also worked.
- verified projects with no columns do not show up in import dialog
- verified project with / without columns still all show up in maniphest project typeahead

Reviewers: epriestley

Reviewed By: epriestley

Subscribers: epriestley, Korvin

Maniphest Tasks: T4431

Differential Revision: https://secure.phabricator.com/D10153
2014-08-05 13:40:41 -07:00
epriestley
e68b6deccb Remove PHID_TYPE_ACMT
Summary: Ref T4896. This was used by the old audit comment storage, which is now defunct.

Test Plan: Grepped for callsites in the codebase.

Reviewers: joshuaspence, btrahan

Reviewed By: btrahan

Subscribers: epriestley

Maniphest Tasks: T4896

Differential Revision: https://secure.phabricator.com/D10152
2014-08-05 12:02:22 -07:00
Mukunda Modell
5f82705e12 First stab at a conduit method for creating projects.
Summary: This code is mostly lifted from the PhabricatorProjectCreateController.

Test Plan: currently untested

Reviewers: rush898, #blessed_reviewers, epriestley

Reviewed By: #blessed_reviewers, epriestley

Subscribers: epriestley, aklapper, Korvin

Maniphest Tasks: T5691

Differential Revision: https://secure.phabricator.com/D10036
2014-08-05 09:29:30 -07:00
epriestley
1e375c97c5 Normalize project slugs before querying for them
Summary:
Fixes T5728. In particular:

  - `/tag/XYZ/` now works as an alias for `/tag/xyz/`.
  - `arc todo --project ASDF` now works as an alias for `arc todo --project asdf`.

Test Plan: Called `project.query` and visited `/tag/LBHABLHBH/`.

Reviewers: btrahan

Reviewed By: btrahan

Subscribers: aklapper, epriestley

Maniphest Tasks: T5728

Differential Revision: https://secure.phabricator.com/D10144
2014-08-04 16:55:22 -07:00
epriestley
42cf7f6faa Make the current session key a component of the CSRF token
Summary: Fixes T5510. This purely reduces false positives from HackerOne: we currently rotate CSRF tokens, but do not bind them explicitly to specific sessions. Doing so has no real security benefit and may make some session rotation changes more difficult down the line, but researchers routinely report it. Just conform to expectations since the expected behavior isn't bad and this is less work for us than dealing with false positives.

Test Plan:
  - With two browsers logged in under the same user, verified I was issued different CSRF tokens.
  - Verified the token from one browser did not work in the other browser's session.

Reviewers: btrahan

Reviewed By: btrahan

Subscribers: epriestley

Maniphest Tasks: T5510

Differential Revision: https://secure.phabricator.com/D10136
2014-08-04 12:04:47 -07:00
epriestley
95eeffff7e Terminate other sessions on credential changes
Summary:
Fixes T5509. Currently, existing sessions live on even if you change your password.

Over the course of the program, we've recieved a lot of HackerOne reports that sessions do not terminate when users change their passwords. I hold that this isn't a security vulnerability: users can explicitly manage sessions, and this is more general and more powerful than tying session termination to password resets. In particular, many installs do not use a password provider at all (and no researcher has reported this in a general, application-aware way that discusses multiple authentication providers).

That said, dealing with these false positives is vaguely time consuming, and the "expected" behavior isn't bad for users, so just align behavior with researcher expectations: when passwords are changed, providers are removed, or multi-factor authentication is added to an account, terminate all other active login sessions.

Test Plan:
  - Using two browsers, established multiple login sessions.
  - In one browser, changed account password. Saw session terminate and logout in the second browser.
  - In one browser, removed an authentication provider. Saw session terminate and logout in the second browser.
  - In one browser, added MFA. Saw session terminate and logout in the second browser.

Reviewers: btrahan

Reviewed By: btrahan

Subscribers: epriestley

Maniphest Tasks: T5509

Differential Revision: https://secure.phabricator.com/D10135
2014-08-04 12:04:35 -07:00
epriestley
e56dc8f299 Invalidate outstanding password reset links when users adjust email addresses
Summary:
Fixes T5506. Depends on D10133. When users remove an email address or change their primary email address, invalidate any outstanding password reset links.

This is a very small security risk, but the current behavior is somewhat surprising, and an attacker could sit on a reset link for up to 24 hours and then use it to re-compromise an account.

Test Plan:
  - Changed primary address and removed addreses.
  - Verified these actions invalidated outstanding one-time login temporary tokens.
  - Tried to use revoked reset links.
  - Revoked normally from new UI panel.

Reviewers: btrahan

Reviewed By: btrahan

Subscribers: epriestley

Maniphest Tasks: T5506

Differential Revision: https://secure.phabricator.com/D10134
2014-08-04 12:04:23 -07:00
epriestley
30f6405a86 Add an explicit temporary token management page to Settings
Summary:
Ref T5506. This makes it easier to understand and manage temporary tokens.

Eventually this could be more user-friendly, since it's relatively difficult to understand what this screen means. My short-term goal is just to make the next change easier to implement and test.

The next diff will close a small security weakness: if you change your email address, password reset links which were sent to the old address are still valid. Although an attacker would need substantial access to exploit this (essentially, it would just make it easier for them to re-compromise an already compromised account), it's a bit surprising. In the next diff, email address changes will invalidate outstanding password reset links.

Test Plan:
  - Viewed outstanding tokens.
  - Added tokens to the list by making "Forgot your password?" requests.
  - Revoked tokens individually.
  - Revoked all tokens.
  - Tried to use a revoked token.

Reviewers: btrahan

Reviewed By: btrahan

Subscribers: epriestley

Maniphest Tasks: T5506

Differential Revision: https://secure.phabricator.com/D10133
2014-08-04 12:04:13 -07:00
epriestley
e8d272b0da Use standard infrastructure to attach commits to other objects
Summary:
Ref T4896. Now that we have a transaction editor, we can delete a giant block of hacks.

I believe this also resolves the commit/task attachment issues @joshuaspence and @mbishopim3 mentioned.

Test Plan: Attached and detached commits and tasks.

Reviewers: btrahan, joshuaspence, mbishopim3

Reviewed By: mbishopim3

Subscribers: mbishopim3, epriestley, joshuaspence

Maniphest Tasks: T4896

Differential Revision: https://secure.phabricator.com/D10138
2014-08-04 12:03:58 -07:00
epriestley
725e2fa410 Write a "resign" audit relationship even if actor has no relationship
Summary: Ref T4896. I got this logic slightly wrong when porting it over: we always want to write this relationship, to allow members of a project with an audit request against a commit to resign and get it out of their queue.

Test Plan:
  - Resigned from a commit with an existing relationship.
  - Resigned from a commit with no existing relationship, saw one added.

Reviewers: btrahan, joshuaspence, mbishopim3

Reviewed By: mbishopim3

Subscribers: epriestley

Maniphest Tasks: T4896

Differential Revision: https://secure.phabricator.com/D10137
2014-08-04 12:03:48 -07:00
epriestley
86dbf1d17d Recognize Maniphest task description as a remarkup block
Summary:
Ref T4589. We don't recognize task descriptions as remarkup blocks, so `{F...}` references in them do not get attached to the objects, and thus no policy exemption is created.

Recognize them, which activates `{F...}` and `@mentions`.

We probably have a few more of these in other applications, but it's not a big deal to clean them up as they arise.

Test Plan: Uploaded a file to a task in the description field, saw it attach and get a policy exemption.

Reviewers: btrahan

Reviewed By: btrahan

Subscribers: epriestley

Maniphest Tasks: T4589

Differential Revision: https://secure.phabricator.com/D10139
2014-08-04 12:03:36 -07:00
Joshua Spence
f055736eca Rename PhutilRemarkupRule subclasses
Summary: Ref T5655. Depends on D9993.

Test Plan: See D9993.

Reviewers: #blessed_reviewers, epriestley

Reviewed By: #blessed_reviewers, epriestley

Subscribers: epriestley, Korvin

Maniphest Tasks: T5655

Differential Revision: https://secure.phabricator.com/D9994
2014-08-05 00:55:43 +10:00
epriestley
c9fe162470 Fix an issue where file queries would throw incorrectly
Summary:
Ref T4589. When you look at a file, we load attached objects in order to run the "you can see this if you can see any attached object" policy check.

However, right now the subquery inherits the "throw on filter" flag from the parent query. This inheritance makes sense in other cases[1], but because this is an "ANY" rule it does not make sense here. In practice, it means that if the file is attached to several objects, and any of them gets filtered, you can not see the file.

Instead, explicitly drop the flag for this subquery.

[1] Sort of. It doesn't produce wrong results in other cases, but now that I think about it might produce a less-tailored error than it could. I'll look into this the next time I'm poking around.

Test Plan:
  - Viewed an "All Users" file attached to a private Mock.
  - Prior to this patch, I incorrectly received an exception when the Mock was loaded. This is wrong; I should be able to see the file because the policy is "All Users".
  - After the patch, I can correctly view the file, just not the associated mock.

{F127074}

Reviewers: btrahan

Reviewed By: btrahan

Subscribers: 20after4, aran, epriestley

Maniphest Tasks: T4589

Differential Revision: https://secure.phabricator.com/D8498
2014-08-02 14:46:36 -07:00
epriestley
9181929ebc Give files uploaded to objects a very restrictive view policy
Summary:
Fixes T4589. This implements much better policy behavior for files that aligns with user expectations.

Currently, all files have permissive visibility.

The new behavior is:

  - Files uploaded via drag-and-drop to the home page or file upload page get permissive visibility, for ease of quickly sharing things like screenshots.
  - Files uploaded via the manual file upload control get permissive visibility by default, but the user can select the policy they want at upload time in an explicit/obvious way.
  - Files uploaded via drag-and-drop anywhere else (e.g., comments or Pholio) get restricted visibility (only the uploader).
    - When the user applies a transaction to the object which uses the file, we attach the file to the object and punch a hole through the policies: if you can see the object, you can see the file.
    - This rule requires things to use ApplicationTransactions, which is why this took so long to fix.
    - The "attach stuff to the object" code has been in place for a long time and works correctly.

I'll land D8498 after this lands, too.

Test Plan:
  - Uploaded via global homepage upload and file drag-and-drop upload, saw permissive visibility.
  - Uploaded via comment area, saw restricted visibility.
  - After commenting, verified links were established and the file became visible to users who could see the attached object.
  - Verified Pholio (which is a bit of a special case) correctly attaches images.

Reviewers: btrahan

Reviewed By: btrahan

Subscribers: epriestley

Maniphest Tasks: T4589

Differential Revision: https://secure.phabricator.com/D10131
2014-08-02 14:46:13 -07:00
epriestley
1f1828e0c0 Allow users to set an explicit visibility for manual file uploads at creation time
Summary: Ref T4589. Depends on D10129. In addition to letting users change the visibility policy for files, also allow them to choose a policy explicitly when a file is uploaded.

Test Plan: Uploaded several files using the plain old uploader, saw appropriate visibility policies applied.

Reviewers: btrahan

Reviewed By: btrahan

Subscribers: epriestley

Maniphest Tasks: T4589

Differential Revision: https://secure.phabricator.com/D10130
2014-08-02 14:45:59 -07:00
epriestley
4c04d4d019 Allow users to set view policies on files explicitly
Summary: Ref T4589. Allow users to adjust visibility settings on files explicitly. This makes it easier to understand and manage upcoming changes in T4589.

Test Plan: Changed the view policy for a file several times.

Reviewers: btrahan

Reviewed By: btrahan

Subscribers: epriestley

Maniphest Tasks: T4589

Differential Revision: https://secure.phabricator.com/D10129
2014-08-02 14:45:50 -07:00
epriestley
b5750412c7 Apply normal Audit actions directly with Transaction editor
Summary: Ref T4896. This converts the last "CommentEditor" to a transaction editor and removes a large part of the old code.

Test Plan:
  - Added comments.
  - Accepted / added auditors.
  - Added inline comments.

Reviewers: joshuaspence, btrahan

Reviewed By: btrahan

Subscribers: epriestley

Maniphest Tasks: T4896

Differential Revision: https://secure.phabricator.com/D10128
2014-08-02 14:45:39 -07:00
epriestley
25acf5d130 Apply Diffusion reply email directly with transaction editor
Summary: Ref T4896. Invoke the new editor directly instead of in a roundabout way when handling Audit email.

Test Plan: Used `bin/mail receive-test` to simulate mail, saw comment post with proper content source.

Reviewers: joshuaspence, btrahan

Reviewed By: btrahan

Subscribers: epriestley

Maniphest Tasks: T4896

Differential Revision: https://secure.phabricator.com/D10127
2014-08-02 14:45:27 -07:00
epriestley
508260e4fe Apply diffusion.createcomment directly with transaction editor in Audit
Summary: Ref T4896. Use the new transaction-oriented `PhabricatorAuditEditor` directly instead of invoking it via the old editor.

Test Plan: Used Conduit to add a comment, use silent mode, and accept a commit.

Reviewers: joshuaspence, btrahan

Reviewed By: btrahan

Subscribers: epriestley

Maniphest Tasks: T4896

Differential Revision: https://secure.phabricator.com/D10126
2014-08-02 14:45:09 -07:00
epriestley
a297450aa9 Apply "accept" and "resign" actions with transactions
Summary: Ref T4896. Applies these actions using new transaction stuff.

Test Plan:
  - Accepted and raised concern with my own commit, verifying the special project/package behavior.
  - Accepted and raised concern with another author's commit, verifying the authority-over-packages/projects behavior.
  - Accepted a commit I was not affiliated wiht, verifying the "join as an auditor" behavior.

Reviewers: joshuaspence, btrahan

Reviewed By: btrahan

Subscribers: epriestley

Maniphest Tasks: T4896

Differential Revision: https://secure.phabricator.com/D10125
2014-08-02 14:44:57 -07:00
epriestley
78e164aea6 Use transactions to apply "resign" and "close" Audit actions
Summary: Ref T4896. Hook these up with new stuff.

Test Plan:
  - Closed an audit.
  - Resigned from an audit.

Reviewers: joshuaspence, btrahan

Reviewed By: btrahan

Subscribers: epriestley

Maniphest Tasks: T4896

Differential Revision: https://secure.phabricator.com/D10124
2014-08-02 14:44:45 -07:00
epriestley
688f245a95 Use transactions to apply "add auditors" action in Audit
Summary:
Ref T4896. Move the write for "Add Auditors" inside the new Editor.

There are no longer any readers or writers for metadata, so remove the calls for it.

Test Plan: Added auditors from the web UI.

Reviewers: joshuaspence, btrahan

Reviewed By: btrahan

Subscribers: epriestley

Maniphest Tasks: T4896

Differential Revision: https://secure.phabricator.com/D10123
2014-08-02 14:44:35 -07:00
Joshua Spence
c4586664b3 Apply some linter auto-fixes
Summary: A few minor fixes, applied by `arc lint --everything --apply-patches`.

Test Plan: N/A

Reviewers: epriestley, #blessed_reviewers

Reviewed By: epriestley, #blessed_reviewers

Subscribers: epriestley, Korvin

Differential Revision: https://secure.phabricator.com/D10120
2014-08-02 19:03:02 +10:00
epriestley
bb022d2376 Minor, restore Audit getMailThreading method
Summary: This also still has a callsite which I missed.

Auditors: btrahan
2014-08-02 01:26:45 -07:00
James Rhodes
46b4fa85d0 Support custom fields in "Order By" for Maniphest
Summary:
Resolves T4659.  This implements support for sorting tasks by custom fields.

Some of this feels hacky in the way it's hooked up to the Maniphest search engine and task query.

Test Plan: Queryed on a custom date field, with a small page size, and moved back and forth through the result set.

Reviewers: #blessed_reviewers, epriestley

Reviewed By: #blessed_reviewers, epriestley

Subscribers: epriestley, Korvin

Maniphest Tasks: T4659

Differential Revision: https://secure.phabricator.com/D10106
2014-08-02 18:22:16 +10:00
epriestley
950eeef4c0 Minor, restore Audit newReplyHandlerForCommit method
Summary: This still has a callsite which I missed.

Auditors: btrahan
2014-08-02 01:13:29 -07:00
Joshua Spence
955ec1bb9b Fix a file misnaming
Summary: This class was renamed in D9991 but the filename is incorrect.

Test Plan: Eyeball it

Reviewers: epriestley, #blessed_reviewers

Reviewed By: epriestley, #blessed_reviewers

Subscribers: epriestley, Korvin

Differential Revision: https://secure.phabricator.com/D10118
2014-08-02 18:00:41 +10:00
epriestley
49bd5721c5 Use standard infrastructure for Feed in Audit
Summary: Ref T4896. Instead of using custom stuff, use standard stuff.

Test Plan: Viewed a bunch of feed stories and published some over the Asana bridge.

Reviewers: joshuaspence, btrahan

Reviewed By: btrahan

Subscribers: epriestley

Maniphest Tasks: T4896

Differential Revision: https://secure.phabricator.com/D10114
2014-08-02 00:06:56 -07:00
epriestley
64736264a6 Use standard infrastructure for Audit email generation
Summary: Ref T4896. Replace custom stuff with standard stuff.

Test Plan:
  - Sent a bunch of email and it all looked sensible/correct.
  - Made sure to test inlines, specifically, as they're a bit tricky.

Reviewers: joshuaspence, btrahan

Reviewed By: btrahan

Subscribers: epriestley

Maniphest Tasks: T4896

Differential Revision: https://secure.phabricator.com/D10112
2014-08-02 00:06:45 -07:00
epriestley
b787d3ef0d Use standard infrastructure for Audit search indexing
Summary: Ref T4896.

Test Plan: Made an unusual comment, then found it by searching.

Reviewers: btrahan, joshuaspence

Reviewed By: btrahan, joshuaspence

Subscribers: epriestley

Maniphest Tasks: T4896

Differential Revision: https://secure.phabricator.com/D10110
2014-08-02 00:06:35 -07:00
epriestley
5b969fb5b8 Provide a transaction editor to perform Audit row writes
Summary:
Ref T4896. Replaces more custom stuff with standard stuff. In particular:

  - No more fake proxy writes;
  - no more fake detection of `@mentions`.

For now, the old code still applies most of the effects and handles feed and email.

Test Plan:
  - Added comments.
  - Added comments with inline comments.
  - Added just inline comments.
  - Added comments with Conduit.
  - Previewed comments.
  - Added CCs explicitly and with `@mentions`.
  - Added auditors.
  - Accepted a commit.

Reviewers: joshuaspence, btrahan

Reviewed By: btrahan

Subscribers: epriestley

Maniphest Tasks: T4896

Differential Revision: https://secure.phabricator.com/D10109
2014-08-02 00:06:25 -07:00
epriestley
89b942c183 Move Audit to proper Subscriptions
Summary:
Ref T4896. Currently, subscriptions to commits are stored as auditors with a special "CC" type.

Instead, use normal subscriptions storage, reads and writes.

Test Plan:
  - Ran migration and verified data still looked good.
  - Viewed commits in UI and saw "subscribers".
  - Saw "Automatically Subscribed", clicked Subscribe/Unsubscribe on a non-authored commit, saw subscriptions update.
  - Pushed a commit through Herald rules and saw them trigger subscriptions and auditors.
  - Used "Add CCs".
  - Added CCs with mentions.

Reviewers: btrahan, joshuaspence

Reviewed By: btrahan, joshuaspence

Subscribers: epriestley

Maniphest Tasks: T4896

Differential Revision: https://secure.phabricator.com/D10103
2014-08-02 00:06:13 -07:00
Joshua Spence
68f1ca896d Fix misspelled file name
Summary: This class was renamed in D9991, but the filename is incorrect.

Test Plan: Eyeball it

Reviewers: epriestley, #blessed_reviewers

Reviewed By: epriestley, #blessed_reviewers

Subscribers: epriestley, Korvin

Differential Revision: https://secure.phabricator.com/D10117
2014-08-02 17:05:49 +10:00
Bob Trahan
dd918b0d14 Application Search - fix error updating searches from human-readable links
Summary:
Fixes T5666. When we have a pretty link right now it can conflict with form data; e.g. if you have 'statuses=open' in the URI and then uncheck status = open in the UI, you will still get the open status in the next search.

To fix this, set the form action explicitly to lose all the get parameter junk.

Test Plan: tried the test case in T5666 / this description and it no longer failed...!

Reviewers: epriestley

Reviewed By: epriestley

Subscribers: epriestley, Korvin

Maniphest Tasks: T5666

Differential Revision: https://secure.phabricator.com/D10115
2014-08-01 17:22:24 -07:00
Bob Trahan
e50b269416 Notifications - fix race condition around "Mark All Read".
Summary:
pre-patch "Mark All Read" marks *all* unread notifications as read. This is a race condition in that the user is looking at some set of notiifcations and that set may update such that the newest notifications aren't shown. An example might be if sitting on the notifications page or having the menu open while a new notification comes in... Note re-opening the menu would show the latest notifications.

This patch makes it so "Mark All Read" links only marks the notifications currently loaded (and older.) Fixes T5764.

Additionally, if there is nothing to "mark read" the button / link "Mark All Read" will have a disabled style and yield a dialog saying "nothing to mark as read".

Test Plan: carefully tracked ?chronoKey populating correctly in various links. Verified query constructed properly too.

Reviewers: epriestley

Reviewed By: epriestley

Subscribers: epriestley, Korvin

Maniphest Tasks: T5764

Differential Revision: https://secure.phabricator.com/D10113
2014-08-01 16:39:05 -07:00
Bob Trahan
5ccc465798 Workboards - fix broken links on pages accessed via tag
Summary: $this->id wasn't being set in this case so just set it explicitly after we finish loading the project. Fixes T5763.

Test Plan: links were broken no longer!

Reviewers: epriestley

Reviewed By: epriestley

Subscribers: epriestley, Korvin

Maniphest Tasks: T5763

Differential Revision: https://secure.phabricator.com/D10108
2014-08-01 11:06:42 -07:00