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

11110 commits

Author SHA1 Message Date
epriestley
bdecff7d67 Show "objectives" UI only if prototypes are enabled
Summary: See D17955.

Test Plan: Loaded a revision, no longer saw annotations with prototypes off. Still saw annotations with prototypes on.

Reviewers: chad

Reviewed By: chad

Differential Revision: https://secure.phabricator.com/D17983
2017-05-20 07:55:48 -07:00
Chad Little
5f49f9c793 Add sound to logged out Conpherence
Summary: Fixes T12735. Adds a sound if the user is logged out, skips checking a setting.

Test Plan: set participants to null and verify sound plays, no exceptions1111

Reviewers: epriestley

Reviewed By: epriestley

Subscribers: Korvin

Maniphest Tasks: T12735

Differential Revision: https://secure.phabricator.com/D17973
2017-05-20 06:51:33 -07:00
epriestley
1644b45050 Disperse task subpriorities in blocks
Summary:
Ref T7664. The current algorithm for moving task subpriorities can end up stuck in a real sticky swamp in some unusual situations.

Instead, use an algorithm which works like this:

  - When we notice two tasks are too close together, look at the area around those tasks (just a few paces).
  - If things look pretty empty, we can just spread the tasks out a little bit.
  - But, if things are still real crowded, take another look further.
  - Keep doing that until we're looking at a real nice big spot which doesn't have too many tasks in it in total, even if they're all in one place right now.
  - Then, move 'em out!

Also:

  - Just swallow our pride and do the gross `INSERT INTO ... "", "", "", "", "", "", ... ON DUPLICATE KEY UPDATE` to bulk update.
  - Fix an issue where a single move could cause two different subpriority recalculations.

Test Plan:
  - Changed `ManiphesTaskTestCase->testTaskAdjacentBlocks()` to insert 1,000 tasks with identical subpriorities, saw them spread out in 11 queries instead of >1,000.
  - Dragged tons of tasks around on workboards.
  - Ran unit tests.

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T7664

Differential Revision: https://secure.phabricator.com/D17959
2017-05-19 15:45:14 -07:00
epriestley
c6a7bcfe89 Make Pholio description behave as a remarkup field (e.g., subscribe mentioned users)
Summary: Ref T12732. This is pre-existing but fix it since I caught it while banging around.

Test Plan: {F4967442}

Reviewers: chad, amckinley

Reviewed By: chad

Maniphest Tasks: T12732

Differential Revision: https://secure.phabricator.com/D17970
2017-05-19 14:27:09 -07:00
epriestley
bbc5f79227 Make membership lock/unlock feed stories read more naturally
Summary: Ref T12732. This is pre-existing.

Test Plan: {F4967438}

Reviewers: chad, amckinley

Reviewed By: chad

Maniphest Tasks: T12732

Differential Revision: https://secure.phabricator.com/D17969
2017-05-19 14:25:39 -07:00
epriestley
789d57522b Make editing project images redirect to "Manage" more consistently
Summary:
Ref T12732. Currently, different ways of setting a profile image can leave you in different places.

Instead, always send the user back to the "Manage" page.

Test Plan: Used "Current Picture", "use picture", "Build picture" and "upload picture", always ended up in the same spot.

Reviewers: chad, amckinley

Reviewed By: amckinley

Maniphest Tasks: T12732

Differential Revision: https://secure.phabricator.com/D17967
2017-05-19 14:06:48 -07:00
epriestley
10b3879232 Make Project slug/hashtag transactions render a little more nicely
Summary: Ref T12732. Use `renderValue()` to build `renderValueList()` so we get nice fancy text for these.

Test Plan: {F4967410}

Reviewers: chad, amckinley

Reviewed By: amckinley

Maniphest Tasks: T12732

Differential Revision: https://secure.phabricator.com/D17966
2017-05-19 13:59:27 -07:00
Chad Little
abd791889c Update Maniphest title transaction again
Summary: Ref T12732, third time is the charm?

Test Plan: Read

Reviewers: epriestley

Subscribers: Korvin

Maniphest Tasks: T12732

Differential Revision: https://secure.phabricator.com/D17965
2017-05-19 13:46:13 -07:00
Chad Little
5a34b299e4 Update Maniphest title language
Summary: Ref T12732

Test Plan: Read

Reviewers: epriestley

Reviewed By: epriestley

Subscribers: Korvin

Maniphest Tasks: T12732

Differential Revision: https://secure.phabricator.com/D17964
2017-05-19 13:41:29 -07:00
Chad Little
601622013d Clarify milestone/subproject creation language
Summary: Ref T12732

Test Plan: Read new language

Reviewers: epriestley

Reviewed By: epriestley

Subscribers: Korvin

Maniphest Tasks: T12732

Differential Revision: https://secure.phabricator.com/D17961
2017-05-19 13:30:02 -07:00
epriestley
c9889e3d55 Fix an issue in Phriction where moving a document just copied it instead
Summary: Ref T12732. See D17918. With modular transactions, `getCustomTransactionNewValue()` isn't actually called.

Test Plan: Moved document `/x/` to `/y/`, saw document gone at `/x/` instead of copied.

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T12732

Differential Revision: https://secure.phabricator.com/D17963
2017-05-19 13:04:24 -07:00
epriestley
fdf00f6df4 Clean up some minor UI behaviors in Differential
Summary:
Minor UI tweaks:

  - Use the dynamic icon for each file (e.g., image, text), not a hard-coded icon.
  - Render the path (less important) in grey and the filename (more important) in black.

Test Plan: {F4966176}

Reviewers: chad

Reviewed By: chad

Differential Revision: https://secure.phabricator.com/D17957
2017-05-19 12:01:58 -07:00
epriestley
6c46f27d98 Add quest objectives to the minimap
Summary:
Add important objectives (like waygates and quest markers) to the minimap.

This also probably fixes @cspeckmim's bug with the {key @} keyboard shortcut.

Test Plan:
(This is probably easier to undestand if you `arc patch` + click around.)

{F4966037}

Reviewers: chad, amckinley

Reviewed By: chad

Subscribers: cspeckmim

Differential Revision: https://secure.phabricator.com/D17955
2017-05-19 12:01:01 -07:00
Chad Little
d783299a19 Fix Phriction status not set property on new document
Summary: I deleted too many lines of code here and TYPE_MOVE was always being applied when CONTENT was set. This should fix on next document save, but should I write some migration tool anyways?

Test Plan: Create a new document, see document with correct status.

Reviewers: epriestley

Reviewed By: epriestley

Subscribers: Korvin

Differential Revision: https://secure.phabricator.com/D17960
2017-05-19 17:57:38 +00:00
Chad Little
93e28da76e Add more "disabled" UI to PHUIObjectItemView
Summary: Brings more UI tweaks to disabled objects, like projects/people. Also fixes a missing icon in projects.

Test Plan: Application search with people and projects that have disabled results.

Reviewers: epriestley

Reviewed By: epriestley

Subscribers: Korvin

Maniphest Tasks: T12732

Differential Revision: https://secure.phabricator.com/D17962
2017-05-19 17:54:53 +00:00
Austin McKinley
7e46d7ab6a Migrate Project color to modular transactions
Test Plan: Unit tests + changing project colors.

Reviewers: #blessed_reviewers, epriestley

Reviewed By: #blessed_reviewers, epriestley

Subscribers: epriestley

Differential Revision: https://secure.phabricator.com/D17958
2017-05-18 16:46:06 -07:00
Austin McKinley
1bff5309e6 Migrate Project icons to modular transactions
Test Plan: Unit tests pass. Changed some icons, observed expected timeline entries.

Reviewers: #blessed_reviewers, epriestley

Reviewed By: #blessed_reviewers, epriestley

Subscribers: epriestley

Differential Revision: https://secure.phabricator.com/D17956
2017-05-18 16:25:59 -07:00
Austin McKinley
eb84bf98a4 Migrate Project image to modular transactions
Summary: I'm not sure you can actually remove a project's image (maybe via the API?), but I kept the code for rendering the relevant title/feed anyway.

Test Plan: Unit tests + adding/changing project pictures.

Reviewers: #blessed_reviewers, epriestley

Reviewed By: #blessed_reviewers, epriestley

Subscribers: Korvin, epriestley

Differential Revision: https://secure.phabricator.com/D17954
2017-05-18 16:08:37 -07:00
Austin McKinley
f92059d84c Migrate Project status to modular transactions
Test Plan: Unit tests pass. Archived/activated some projects a couple times; observed expected transactions on timeline.

Reviewers: #blessed_reviewers, epriestley

Reviewed By: #blessed_reviewers, epriestley

Subscribers: epriestley

Differential Revision: https://secure.phabricator.com/D17953
2017-05-18 11:36:13 -07:00
Austin McKinley
91eb22cb3a Migrate Project slugs to modular transactions
Test Plan: Unit tests all pass. Added/removed/altered some project hashtags and observed expected transactions in timeline.

Reviewers: #blessed_reviewers, epriestley

Reviewed By: #blessed_reviewers, epriestley

Subscribers: epriestley

Differential Revision: https://secure.phabricator.com/D17952
2017-05-18 11:15:16 -07:00
Chad Little
1599c56217 Add Pinboard Items to Timeline
Summary: This allows adding of pinboard items to a timeline. I'm hoping we can get this in for Maniphest (Pholio, Cover Image) and Macro (because, Macro), but unsure how to scalably do this. Anyways, here's the front end.

Test Plan:
Make some fake timeline items in UIExamples, test mobile, tablet, and desktop breakpoints.

{F4965798}

Reviewers: epriestley

Reviewed By: epriestley

Subscribers: Korvin

Differential Revision: https://secure.phabricator.com/D17950
2017-05-18 10:34:57 -07:00
epriestley
fb9f3cc0b4 Restore the "buoyant" header in Differential
Summary:
Fixes T1591. This was removed long ago because it was a mess to implement and caused a bunch of weird issues, and also my tolerance for dealing with weird JS issues was much, much lower.

I have now survived the fires of JX.Scrollbar and would love to address 200 small nitpicks about obscure browser behaviors on Linux, so open the floodgates again.

A secondary goal here is to create room to add a global view state menu on the right, with 300 options like "hide all inlines", "hide done inlines", "hide collapsed inlines", "hide ghosts", "show ghosts", "enable filetree", "disable filetree", etc, etc. Not sure how much of this I'll actually do. I have one more experiment I want to try first.

Test Plan: {F4963294}

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T1591

Differential Revision: https://secure.phabricator.com/D17945
2017-05-18 10:24:26 -07:00
epriestley
f78ce156f1 Restore "h" to hide or show files, and modernize file visibility toggling
Summary:
Ref T12616. This puts "h" back to collapse or expand the current file.

This removes some very complicated/messy code around following links in the table of contents and getting files auto-expanded. I suspect no one will miss this, but we can restore it if ayone notices.

Test Plan: Pressed "h" to collapse/expand a file. Also used the menu items.

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T12616

Differential Revision: https://secure.phabricator.com/D17940
2017-05-18 10:21:37 -07:00
Austin McKinley
1e3c8df1c8 Migrate Project names to modular transactions
Summary: Also changes access modifiers on `PhabricatorProjectTransactionEditor` and sets up `storage` for `applyExternalEffects`.

Test Plan: Created new projects, attempted to create without name, with too long of a name, and with a name that conflicts with other projects and observed expected errors.

Reviewers: #blessed_reviewers, epriestley

Reviewed By: #blessed_reviewers, epriestley

Subscribers: epriestley

Maniphest Tasks: T12673

Differential Revision: https://secure.phabricator.com/D17947
2017-05-17 17:12:22 -07:00
Austin McKinley
d12be0fc21 Change Pholio editor access modifier
Summary: Used by `PholioImageFileTransaction::mergeTransactions()`. I forgot to test adding multiple images to a Mock at the same time after migrating `mergeTransactions` over to the modular framework.

Test Plan: Added multiple images in a single transaction and didn't get an exception about accessing a protected function.

Reviewers: #blessed_reviewers, epriestley

Reviewed By: #blessed_reviewers, epriestley

Subscribers: epriestley

Differential Revision: https://secure.phabricator.com/D17946
2017-05-17 16:24:40 -07:00
Chad Little
0e8f72a0d9 Clarify Subscription page language
Summary: This says "Edit Subscription" but the page only lets you update the payment method for autopay. I think this is confusing users. I tried to find the best language here, but suggest something else if you prefer.

Test Plan: Review language on sample subscription page.

Reviewers: epriestley

Reviewed By: epriestley

Subscribers: Korvin

Maniphest Tasks: T12451

Differential Revision: https://secure.phabricator.com/D17944
2017-05-17 14:48:22 -07:00
epriestley
9eb285f4aa Fix "left"/"right" changeset ID selection for synthetic deletions
Summary:
Fixes T8323. See that task for a description.

We were using `nonempty()`, but that rule doesn't cover synthetic deletions (file present in an earlier diff, but no longer present in the later diff).

Test Plan: Followed the steps in T8323, got a clean comment.

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T8323

Differential Revision: https://secure.phabricator.com/D17929
2017-05-17 08:54:13 -07:00
epriestley
51df02821b Move the "select a line range" inline code to DiffInline
Summary: Ref T12616. This makes line range selection use the new code, and removes the remainder of the old "hover a line number" / "select a line range" code.

Test Plan: Hovered line numbers; selected line ranges.

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T12616

Differential Revision: https://secure.phabricator.com/D17927
2017-05-17 08:41:26 -07:00
epriestley
422eb9db83 Correct the generation of "<th />" IDs on left-hand-side of image changesets
Summary:
Fixes T7682. The left-hand-side "<th />" row did not generate with the correct ID.

(I couldn't reproduce the exact issue described in T7682, but hovering comments on either side now works properly for me.)

Test Plan: {F4962479}

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T7682

Differential Revision: https://secure.phabricator.com/D17926
2017-05-17 06:26:45 -07:00
epriestley
e4e91ebf6f In Differential, allow "r" to create comments and "R" to quote
Summary:
Ref T11401. Fixes T5232. Ref T12616.

Partly, this moves more code over to the new stuff.

This also allows "r" to work if you have code selected (not just comments). If you "reply" to code, you start a new comment.

You can "R" a comment to quote it. This just starts a new comment normally if you "R" a block of code. This is sort of a power-user version of "quote" since it seems like it probably doesn't really make sense to put it in the UI ever (maybe).

With the new click-to-select, you can click + "R" to reply-with-quote.

Test Plan: Used "r" and "R" to reply to comments and code.

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T12616, T11401, T5232

Differential Revision: https://secure.phabricator.com/D17920
2017-05-16 17:37:54 -07:00
Chad Little
abc9bc77b2 Move Phriction MOVE_TO transaction to Modular Transactions
Summary: Moves this transaction over to modular transactions.

Test Plan: Move a document, re-title a document, try to move over an existing document.

Reviewers: epriestley

Reviewed By: epriestley

Subscribers: Korvin

Differential Revision: https://secure.phabricator.com/D17918
2017-05-16 15:52:35 -07:00
Chad Little
2aa146ffac Set an icon for Maniphest story points
Summary: It's an icon. For story points.

Test Plan: Set some points, see icon.

Reviewers: epriestley

Reviewed By: epriestley

Subscribers: Korvin

Differential Revision: https://secure.phabricator.com/D17915
2017-05-16 12:44:28 -07:00
Austin McKinley
6888472b56 Migrate Pholio inline comments to modular transactions
Summary: Fixes T12626.

Test Plan: Made lots of comments, confirmed no UI changes

Reviewers: #blessed_reviewers, epriestley

Reviewed By: #blessed_reviewers, epriestley

Subscribers: epriestley

Maniphest Tasks: T12626

Differential Revision: https://secure.phabricator.com/D17914
2017-05-16 12:22:46 -07:00
Chad Little
a53d387ea6 Move Phriction Title transaction to Modular Transactions
Summary: Ref T12625. Moves TYPE_TITLE to modular transaction.

Test Plan: New Document, Edit Document, test validation, verify feed stories.

Reviewers: epriestley

Reviewed By: epriestley

Subscribers: Korvin

Maniphest Tasks: T12625

Differential Revision: https://secure.phabricator.com/D17912
2017-05-16 11:08:38 -07:00
Chad Little
fdf001739c Normalize headers and actions in Project sub pages
Summary: Run through all the pages in projects and make sure they all feel similar. Adds back curtain on board manage page, even though it is sad for only having a single action.

Test Plan: Test all pages on a project for consistency in UI.

Reviewers: epriestley

Reviewed By: epriestley

Subscribers: Korvin

Differential Revision: https://secure.phabricator.com/D17909
2017-05-16 09:58:33 -07:00
epriestley
8052ab84bf Remove "^" (Prev) and "V" (Next) actions on Differential inline comments
Summary:
Ref T12616. Fixes T12715. I suspect these are very rarely used. (I think you tried to get rid of them before but I pushed back since we couldn't really offer great alternatives at the time?)

Now that the code is in a better place:

  - Click an inline's header (just the colored part) to select it with the keyboard selection cursor.
  - Click again to deselect it.
  - You can use "n" and "p" to jump to comments, so "click + n" is the same as the old "V" action.
  - This also makes it easier to swap between keyboard and mouse workflows, since you can jump into things with the keyboard at any inline.

Also, make "Reply" render more consistently.

Test Plan:
  - Did all that stuff, things seemed to work OK.

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T12715, T12616

Differential Revision: https://secure.phabricator.com/D17908
2017-05-16 09:44:00 -07:00
Chad Little
29cfcc82ef Update Milestone/Subproject page
Summary: Cleans up the UI, moves details over to curtain, adds some fallback no data strings.

Test Plan: Review with and without subprojects, milestones.

Reviewers: epriestley

Reviewed By: epriestley

Subscribers: Korvin

Differential Revision: https://secure.phabricator.com/D17907
2017-05-16 09:31:25 -07:00
Chad Little
bf753c8b5a Make passing an object to newCurtain optional
Summary: We seem to already support this, just takes it fully there. We don't need to see things like "Flag", etc, on certain subpages of projects/people/etc.

Test Plan: Review Members, Subproject pages, no longer see "Flag for Later" which only is for the Project itself. Check manage, still there.

Reviewers: epriestley

Reviewed By: epriestley

Subscribers: Korvin

Differential Revision: https://secure.phabricator.com/D17897
2017-05-16 08:51:05 -07:00
epriestley
1b5a276a02 Add Differential keyboard shortcuts for "mark done" and "hide/show"
Summary:
Fixes T8130. Allows selected comments to be shown/hidden (with "q") or marked done/not-done (with "w").

(These key selections are because "qwer" are right next to each other on QWERTY keyboards, and now mean "hide, done, edit, reply".)

Also, allow "N" and "P" to do next/previous inline, including hidden inlines. This makes "q" to hide/show a little more powerful and a little easier to undo.

Test Plan: Used "q", "w", "N" and "P" to navigate and interact with comments.

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T8130

Differential Revision: https://secure.phabricator.com/D17906
2017-05-16 08:23:22 -07:00
Chad Little
ef839192aa Only show member/watcher notes on Members page in Projects
Summary: Restricts the view of the membership privileges to just the Members page itself, and not other pages like Home/Details.

Test Plan: Test Home, Test Members, see correct layouts.

Reviewers: epriestley

Reviewed By: epriestley

Subscribers: Korvin

Differential Revision: https://secure.phabricator.com/D17896
2017-05-16 07:17:18 -07:00
Chad Little
1e47ba2481 Use setDrag UI for reordering workboard columns
Summary: This UI can use the setDrag call to reduce clutter on the reodering dialog.

Test Plan:
Reorder some columns, save.

{F4959906}

Reviewers: epriestley

Reviewed By: epriestley

Subscribers: Korvin

Differential Revision: https://secure.phabricator.com/D17898
2017-05-16 07:13:25 -07:00
epriestley
2fb1edfeb1 Restore the Differential "edit" and "reply" keyboard shortcuts
Summary: Ref T12616. This makes "edit" and "reply" work again.

Test Plan:
Used "e" and "r" to edit and reply.

Also used them in bogus ways and got useful UI feedback.

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T12616

Differential Revision: https://secure.phabricator.com/D17895
2017-05-16 06:25:35 -07:00
epriestley
588a66c04d Move most Differetial keyboard shortcuts into DiffChangesetList
Summary: Ref T12616. This moves most keyboard shortcuts into DiffChangesetList. It breaks some shortcuts that I plan to restore later, noted in T12616 (toggle file, edit inline, reply to inline), since I think ripping them out now and rebuilding them in a little bit will make things much simpler.

Test Plan:
  - Used j, k, n, p, J, K shortcuts to navigate a revision.

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T12616

Differential Revision: https://secure.phabricator.com/D17859
2017-05-16 06:24:42 -07:00
epriestley
4fd4ec3d27 Hide inlines one-by-one, instead of in a big group
Summary:
Ref T12616. Fixes T12153. Currently, when you hide inlines, they hide completely and turn into a little bubble on the previous line.

Instead, collapse them to a single line one-by-one. Narrowly, this fixes T12153.

In the future, I plan to make these changes so this feature makes more sense:

  - Introduce global "hide everything" states (T8909) so you can completely hide stuff if you want, and this represents more of a halfway state between "nuke it" and "view it".
  - Make the actual rendering better, so it says "epriestley: blah blah..." instead of just "..." -- and looks less dumb.

The real goal here is to introduce `DiffInline` and continue moving stuff from the tangled jungle of a million top-level behaviors to sensible smooth statefulness.

Test Plan:
  - Hid and revealed inlines in unified and two-up modes.
  - These look pretty junk for now:

{F4948659}

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T12616, T12153

Differential Revision: https://secure.phabricator.com/D17861
2017-05-16 06:19:56 -07:00
epriestley
fe44e987fb Translate "Loading..." text in inline comments
Summary: Ref T12616. This cements the relationship between ChangesetList (parent container) and Changeset (child) and passes translations down so Changeset can use them to translate the text "Loading..."

Test Plan: Viewed loading changes.

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T12616

Differential Revision: https://secure.phabricator.com/D17846
2017-05-16 06:19:01 -07:00
epriestley
64a54aac9d Merge "differential-dropdown-menus" behavior into DiffChangesetList
Summary: Ref T12616. This ends up being a little messy ("one giant function") and maybe I'll clean it up a bit later, but continue consolidating the wild jungle of behaviors into a smaller set of responsible objects.

Test Plan: Clicked all the menu options, saw them work properly. Grepped for removed methods.

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T12616

Differential Revision: https://secure.phabricator.com/D17845
2017-05-16 06:18:26 -07:00
Austin McKinley
545d6347dd Convert remaining Pholio image transactions to modular transaction framework
Summary: Also cleans up now-dead code relating to old transactions

Test Plan: Created lots of mocks, replaced their images, added/removed images, changed the sequence, verified expected DB xaction rows, Mock updates, and correct rendering of timeline.

Reviewers: #blessed_reviewers, epriestley

Reviewed By: #blessed_reviewers, epriestley

Subscribers: epriestley

Differential Revision: https://secure.phabricator.com/D17892
2017-05-15 16:25:34 -07:00
Chad Little
f600bc0811 Clean up watchers and members project page
Summary: Various little fixes, mostly moves information from the "Details" section either into the curtain or into the specific watchers or members list based on user viewership. I think this page is both cleaner and more informative.

Test Plan:
Lock, Unlock, Watch, Join, various projects with multiple users.

{F4959101}

Reviewers: epriestley

Reviewed By: epriestley

Subscribers: Korvin

Differential Revision: https://secure.phabricator.com/D17891
2017-05-15 15:56:14 -07:00
Austin McKinley
78544334cd Fix breakage of Pholio
Summary: Removal of `PholioMockEditor::applyCustomInternalTransaction()` in D17868 broke creation of new mocks in Pholio. Puts the empty method back until we finish migrating Pholio to modular transactions.

Test Plan: Created some mocks, observed lack of unhandled exception.

Reviewers: #blessed_reviewers, epriestley

Reviewed By: #blessed_reviewers, epriestley

Subscribers: epriestley

Differential Revision: https://secure.phabricator.com/D17889
2017-05-15 13:30:13 -07:00
Chad Little
904480dc3c Generate newValue for ManiphestTaskPointTransaction
Summary: I think this is the correct fix, sets a consistent value for transactions, old and new, for Maniphest point values.

Test Plan:
Edit title, see no point feed story, set points, see point story, set points to same value, see no story, remove points, see remove point story.

{F4958233}

Reviewers: epriestley

Reviewed By: epriestley

Subscribers: Korvin

Differential Revision: https://secure.phabricator.com/D17885
2017-05-15 11:18:54 -07:00
Chad Little
d6a620be45 Update Maniphest for modular transactions
Summary: Ref T12671. This modernized Maniphest transactions to modular transactions.

Test Plan:
- Create Task
- Edit Task
- Raise Priority
- Change Status
- Merge as a duplicate
- Create Subtask
- Claim Task
- Assign Project
- Move on Workboard
- Set a cover image
- Assign story points
- Change story points
- Generate lots via lipsum
- Bulk edit tasks
- Leave comments
- Award Token

I'm sure I'm missing something.

Reviewers: epriestley

Reviewed By: epriestley

Subscribers: hazelyang, Korvin

Maniphest Tasks: T12671

Differential Revision: https://secure.phabricator.com/D17844
2017-05-15 10:29:06 -07:00
Chad Little
ea874a2825 Restrict watching and member project display better
Summary: Fixes T12713. We don't need to show watching and member info on other views other than ApplicationSearch (for now) so add a few methods to restrict the calls.

Test Plan: Visit project search, profile, project home, project home with subprojects

Reviewers: epriestley

Reviewed By: epriestley

Subscribers: Korvin

Maniphest Tasks: T12713

Differential Revision: https://secure.phabricator.com/D17883
2017-05-15 09:04:54 -07:00
Chad Little
6fa91caf32 Clean up Project Board Manage / Column Manage pages
Summary: Slightly nicer, more consistent UI. Also removed "Column History" from dropdowns as this is available on the general board manage page.

Test Plan: Review Board and Column management pages.

Reviewers: epriestley

Reviewed By: epriestley

Subscribers: Korvin

Differential Revision: https://secure.phabricator.com/D17881
2017-05-15 07:26:34 -07:00
epriestley
7f54f79fd1 Add required needMembers/needWatchers calls to Project Profile/Subprojects tabs
Summary:
Fixes T12710. See that task for discussion. This is pretty ugly/redundant but not broken.

(Feel free to reject this and pursue something else.)

Test Plan:
  - For a project with active subprojects/milestones, viewed the project profile and subprojects tabs.
  - After patch: they're ugly, but no longer fatal.

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T12710

Differential Revision: https://secure.phabricator.com/D17882
2017-05-15 06:44:18 -07:00
Chad Little
89e567ffd9 Move Board Manage actions up a level
Summary: Moves "reorder columns" and "change background" up a level, redesigns "manage" page to be a little cleaner.

Test Plan: Change colors, reorder columns, manage page, disable board, re-enable board.

Reviewers: epriestley

Reviewed By: epriestley

Subscribers: Korvin

Differential Revision: https://secure.phabricator.com/D17879
2017-05-14 17:48:14 -07:00
Chad Little
a5ad11d2d2 Add Member/Watcher info to search results
Summary: Fixes T12707 Adds additional information to search results for if user is a member or a watcher of a project. Also removed the icon colors, which I'll find a better way to denote in future.

Test Plan: Join a project, watch a project, view results list in /projects/

Reviewers: epriestley

Reviewed By: epriestley

Subscribers: Korvin

Maniphest Tasks: T12707

Differential Revision: https://secure.phabricator.com/D17880
2017-05-14 17:38:37 -07:00
epriestley
c7a6422559 Make a handful of minor Slowvote behaviors more consistent with other applications
Summary:
Ref T12685.

  - Better icon/color/label consistency.
  - Make "0" a valid response.
  - Fix a bug where creating a poll with Quicksand enabled led to bad times (form submitted back to the search screen).

Test Plan: {F4956412}

Reviewers: chad, amckinley

Reviewed By: chad

Maniphest Tasks: T12685

Differential Revision: https://secure.phabricator.com/D17878
2017-05-14 14:01:33 -07:00
epriestley
d2e1dd0ef0 In Applications, always redirect back to the detail page after edits
Summary:
Ref T12685. When you edit an application's policies but don't make any changes, you currently get stuck on the same page. This isn't how other edit screens work, and I think it's a holdover from eras long ago.

Make it consistent with other applications.

Also, fix a missing `pht()`.

Test Plan:
  - Edited an application configuation.
  - Clicked "Save" without making changes.
  - After patch, was redirected back to detail page like in other applications.

Reviewers: chad, amckinley

Reviewed By: chad

Maniphest Tasks: T12685

Differential Revision: https://secure.phabricator.com/D17877
2017-05-14 14:01:01 -07:00
epriestley
95925ad58f Make getApplicationTransactionEditor() in PhabricatorApplication return an editor
Summary:
Ref T12685. I provided this incorrect (`return new` rather than `throw`) implementation earlier; it can now be replaced with a proper implementation.

This caused application policy edits to spew this into the daemon log:

```
[2017-05-14 15:35:27] EXCEPTION: (Error) Call to undefined method PhutilMethodNotImplementedException::setActor() at [<phabricator>/src/applications/transactions/worker/PhabricatorApplicationTransactionPublishWorker.php:69]
```

Test Plan:
  - Used `bin/worker execute --id <id>` to execute a previously-failing task.
  - Saw a feed story publish.

Reviewers: chad, amckinley

Reviewed By: chad

Maniphest Tasks: T12685

Differential Revision: https://secure.phabricator.com/D17876
2017-05-14 14:00:25 -07:00
epriestley
aa81979922 Fix a runaway indentation level in LegalpadDocumentEditEngine
Summary: Ref T12685.

Test Plan: No behavioral changes.

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T12685

Differential Revision: https://secure.phabricator.com/D17875
2017-05-14 13:59:57 -07:00
Chad Little
a25c79bbc0 Add Column Edit / History actions to workboard columns
Summary: This brings up "Edit Column" as an action item under the main column dropdown as well as a "Column History" for completeness. Unsure column history is actually useful, but leaving it in anyways. It might be nice to have some sort of dialog version of a history page.

Test Plan: Make a workboard, add a column, edit column name, stay on workboard.

Reviewers: #blessed_reviewers, epriestley

Reviewed By: #blessed_reviewers, epriestley

Subscribers: epriestley

Differential Revision: https://secure.phabricator.com/D17874
2017-05-14 16:41:20 -04:00
epriestley
11c5638832 Fix some error log issues with uninitialized commit/revision lists
Summary:
Fixes T12679. Reproduction steps appear to be:

  - As a logged-out user, view revision list or commit list.
  - Enable bucketing by action required.
  - Before patch: `foreach (null as ...)` causes error spew.
  - After patch: `foreach (array() as ...)` works great.

Test Plan:
  - Reproduced issue by following steps above in Differential (revisions) and Diffusion (audits/commits).
  - After patches, no more errors in the log.

Reviewers: chad, amckinley

Reviewed By: chad

Maniphest Tasks: T12679

Differential Revision: https://secure.phabricator.com/D17872
2017-05-14 13:28:02 -07:00
Chad Little
db631b423f Add basic Watching filter to /projects/
Summary: Ref T12707. Adds a simple filter for the viewer if logged in.

Test Plan: Watch a project, click on watching list, see project I'm watching.

Reviewers: epriestley

Reviewed By: epriestley

Subscribers: epriestley

Maniphest Tasks: T12707

Differential Revision: https://secure.phabricator.com/D17873
2017-05-14 15:06:04 -04:00
Austin McKinley
74d86f0902 Migrate Pholio image description and mock status to modular transactions
Summary: Also removes more now-dead code from `PholioTransaction`.

Test Plan: opened and closed a bunch of mocks, edited a bunch of image descriptions

Reviewers: #blessed_reviewers, epriestley

Reviewed By: #blessed_reviewers, epriestley

Subscribers: Korvin, epriestley

Differential Revision: https://secure.phabricator.com/D17868
2017-05-12 16:07:18 -07:00
Chad Little
b3692cc12c Fix PonderAnswer transaction callsite
Summary: Caught this in the logs, calling an old transaction, update it.

Test Plan: Answer a question, tail log, see no error.

Reviewers: epriestley

Reviewed By: epriestley

Subscribers: Korvin

Differential Revision: https://secure.phabricator.com/D17870
2017-05-12 19:04:48 +00:00
Chad Little
7aeb498565 Fix missing transaction constants in Phame
Summary: Trailing logs for errors here, picked up some unset constants in Phame.

Test Plan: New Blog, New Post, tail daemon log for errors. See feed stories.

Reviewers: epriestley, amckinley

Reviewed By: epriestley

Subscribers: Korvin

Differential Revision: https://secure.phabricator.com/D17871
2017-05-12 19:04:16 +00:00
Chad Little
28a23e3822 Update Legalpad to use comment transactions
Summary: Updates Legalpad to use comment transactions

Test Plan: Leave some comments.

Reviewers: epriestley

Reviewed By: epriestley

Subscribers: Korvin

Differential Revision: https://secure.phabricator.com/D17867
2017-05-11 10:14:13 -07:00
Chad Little
e1a97616cb Add a large profile picture to Projects
Summary: The ports over a similar "profile image" menu item to Projects. It gives us some room to use the project icon in the sidenav along with a larger photo. It also will open up some room in the sub-page headers for us to focus on that page, and not the identity of the project at hand. Expect a few more project related touch up diffs.

Test Plan:
Review new projects menu on a few projects, update the image, see new image. Great for team photos.

{F4951264}

Reviewers: epriestley, amckinley

Reviewed By: epriestley

Subscribers: Korvin

Differential Revision: https://secure.phabricator.com/D17869
2017-05-11 10:13:38 -07:00
Austin McKinley
fcf64953f0 Migate more of Pholio to Modular Transaction
Summary: Another Franken-transaction. Adds transactions for ImageName and MockName. Adds transaction-level validation for presence of mock name; removed same from edit controller. Removes `PholioTransaction::getRemarkupBodyForFeed()`, which appears to be dead code since that method isn't defined on any other types.

Test Plan: made a bunch of changes to pholio mocks and images and observed expected results

Reviewers: #blessed_reviewers, epriestley

Reviewed By: #blessed_reviewers, epriestley

Subscribers: Korvin, chad, epriestley

Differential Revision: https://secure.phabricator.com/D17865
2017-05-10 15:13:48 -07:00
Chad Little
b32a00dbac Update Legalpad for EditEngine
Summary: Updates Legalpad to use EditEngine, paving the way for //transaction comments//. Spooky.

Test Plan:
- New Document
  - Require signing, Corp - see fail
  - Require signing, Noone - see fail
  - Require signing, Ind - get asked to sign
- Edit Document

Reviewers: epriestley

Reviewed By: epriestley

Subscribers: Korvin

Differential Revision: https://secure.phabricator.com/D17862
2017-05-10 15:11:49 -07:00
Chad Little
bc4edc946b Add Conduit edit endpoint for Macro
Summary: Opens up the edit endpoint for Macros

Test Plan: Review /conduit/method/macro.edit/

Reviewers: epriestley

Reviewed By: epriestley

Subscribers: Korvin

Differential Revision: https://secure.phabricator.com/D17866
2017-05-10 14:54:43 -07:00
Austin McKinley
d39758ca7c Migrate Pholio to Frankenstein transactions
Summary: Begins the process of migrating Pholio to Modular Transactions by starting with the mock's description and changing the base class of PholioTransaction. Expect several more of these diffs to quickly follow. Also changes the icon for description changes to `fa-pencil`; previously it was check or ban, depending on the open/closed state. Looks like an accidental switch fallthrough.

Test Plan: made new mocks, edited their descriptions, observed same UI as previously

Reviewers: #blessed_reviewers, epriestley

Reviewed By: #blessed_reviewers, epriestley

Subscribers: Korvin, epriestley

Differential Revision: https://secure.phabricator.com/D17864
2017-05-10 12:38:49 -07:00
Chad Little
437a843a8e Clean up legalpad transaction copy
Summary: Ref T12685, adds more informative transaction copy, fixes bugs.

Test Plan: Create various documents and edit them.

Reviewers: epriestley

Reviewed By: epriestley

Subscribers: Korvin

Maniphest Tasks: T12685

Differential Revision: https://secure.phabricator.com/D17860
2017-05-09 10:47:24 -07:00
Chad Little
c4aa0a9b9a Fix FundRefund transaction calls
Summary: This function `renderHandleLink` doesn't exist. Update call.

Test Plan: I'm still lost on how to refund a transaction? Existing?

Reviewers: epriestley

Reviewed By: epriestley

Subscribers: Korvin

Differential Revision: https://secure.phabricator.com/D17858
2017-05-09 09:59:41 -07:00
Chad Little
b2cbb8f2ee Update Fund for EditEngine commenting
Summary: Ref T12685. Moves Fund to EditEngine commenting.

Test Plan: View an old Fund, leave a comment, add a subscriber.

Reviewers: epriestley

Reviewed By: epriestley

Subscribers: Korvin

Maniphest Tasks: T12685

Differential Revision: https://secure.phabricator.com/D17857
2017-05-09 09:05:43 -07:00
Chad Little
86f9318052 Update Fund for EditEngine
Summary: Ref T12685, updates fund for edit engine.

Test Plan: Create a Fund, Edit a Fund, wipe out Merchants, check errors for name and missing merchants, back Fund.

Reviewers: epriestley

Reviewed By: epriestley

Subscribers: Korvin

Maniphest Tasks: T12685

Differential Revision: https://secure.phabricator.com/D17855
2017-05-09 08:46:16 -07:00
Chad Little
f717e4b563 Clean up some Fund language
Summary: Ref T12685. Cleans up various Fund language nits.

Test Plan: Read carefully.

Reviewers: epriestley

Reviewed By: epriestley

Subscribers: Korvin

Maniphest Tasks: T12685

Differential Revision: https://secure.phabricator.com/D17856
2017-05-08 21:11:45 -07:00
Chad Little
3dae970129 Clean up some rough Macro transaction edges
Summary: Ref T12685, cleans up various macro issues, remove subscribers, fix feed stories, etc.

Test Plan: Create a new macro, see no subscribers, edit various macros.

Reviewers: epriestley

Reviewed By: epriestley

Subscribers: Korvin

Maniphest Tasks: T12685

Differential Revision: https://secure.phabricator.com/D17848
2017-05-08 16:29:35 -07:00
Chad Little
bf30c07230 Clean up Slowvote transactions a little
Summary: Ref T12685. Moves to `xaction` folder and sets description changes in transaction stories.

Test Plan: Make a poll, edit the description.

Reviewers: epriestley

Reviewed By: epriestley

Subscribers: Korvin

Maniphest Tasks: T12685

Differential Revision: https://secure.phabricator.com/D17849
2017-05-08 16:15:31 -07:00
epriestley
ff205964cf Make macro audio errors more clear
Summary: Ref T12685. I bamboozled myself.

Test Plan: {F4945786}

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T12685

Differential Revision: https://secure.phabricator.com/D17847
2017-05-08 16:01:47 -07:00
Chad Little
32af1d9a9f Restrict PhortuneMerchant creation at EditEngine level
Summary: Ref T12685. Checks merchant capabilities at the edit engine level

Test Plan: Test with and without admin level permissions. Get restricted access if I cheat. Still able to create with admin.

Reviewers: epriestley

Reviewed By: epriestley

Subscribers: Korvin

Maniphest Tasks: T12685

Differential Revision: https://secure.phabricator.com/D17852
2017-05-08 15:56:07 -07:00
Chad Little
c505876d61 Clean up some Passphrase transaction bugs
Summary: Ref T12685. Makes the description field full remarkup and fixes setting a credential secret after destruction.

Test Plan: Change description a lot, set and destroy credentials.

Reviewers: epriestley

Reviewed By: epriestley

Subscribers: Korvin

Maniphest Tasks: T12685

Differential Revision: https://secure.phabricator.com/D17851
2017-05-08 15:55:50 -07:00
Chad Little
ade380530f Clean up some Phame transaction edit bugs
Summary: Ref T12685. Sets required on required fields, cleans up mailtags on PhamePost

Test Plan: Create a new blog, post.

Reviewers: epriestley

Reviewed By: epriestley

Subscribers: Korvin

Maniphest Tasks: T12685

Differential Revision: https://secure.phabricator.com/D17850
2017-05-08 15:55:26 -07:00
Chad Little
0a87881e98 Fix file attach bug in Macro
Summary: This was mis-tested by only using one account, which could always see the image. External transaction moved file attachment to the modular transaction for file and audio instead.

Test Plan: Test adding audio and a macro on a pleb account, visit with normal account and see macro fine.

Reviewers: epriestley, amckinley

Reviewed By: amckinley

Subscribers: Korvin

Differential Revision: https://secure.phabricator.com/D17836
2017-05-05 19:50:12 -07:00
Chad Little
c8c23af840 Enable Feed stories for Fund
Summary: Not sure when these stopped, also fixed mailtag contants.

Test Plan: Close an initiative, see story, fund initiative, see story.

Reviewers: epriestley

Reviewed By: epriestley

Subscribers: Korvin

Differential Revision: https://secure.phabricator.com/D17835
2017-05-05 11:15:10 -07:00
Chad Little
a9604c4604 Update Fund for modular transactions
Summary: Fixes T12627. Updates FundInitiative and FundBacker with modular transactions.

Test Plan: Create an Initiative, back it with fake monies, close initiative, reopen, edit various fields.

Reviewers: epriestley

Reviewed By: epriestley

Subscribers: Korvin

Maniphest Tasks: T12627

Differential Revision: https://secure.phabricator.com/D17782
2017-05-05 11:03:03 -07:00
Austin McKinley
0dc90b891a Reimplement Slowvote transactions using modular transactions
Summary:
Fixes T12623. Adds new modular transactions to Slowvote. Also converts
the `shuffle` column to `bool` for consistency with other boolean-ish columns.

Test Plan:
Create a new vote, modified everything that could be modified from the web UI,
observed expected timeline.

Example timeline: {F4938843}

Example transaction values in DB: {F4938850}

Reviewers: #blessed_reviewers, epriestley

Reviewed By: #blessed_reviewers, epriestley

Subscribers: Korvin, epriestley

Maniphest Tasks: T12623

Differential Revision: https://secure.phabricator.com/D17830
2017-05-04 20:20:00 -07:00
epriestley
836819a0e7 Explicitly quote "From" name part when submitting mail to the Mailgun API
Summary:
We are submitting `epriestley (Evan Priestley) <noreply@meta.phacility.com>`, but should be submitting `"epriestley (Evan Priestley)" <noreply@meta.phacility.com>`.

Add the missing quotes.

Test Plan: Locally, this makes the API calls work against the Mailgun sandbox domain.

Reviewers: chad, amckinley

Reviewed By: chad, amckinley

Differential Revision: https://secure.phabricator.com/D17831
2017-05-04 17:03:26 -07:00
Chad Little
d77a3f8760 Update Spaces for modular transactions
Summary: Updates the Spaces application for modular transactions, seemed easy to bang out.

Test Plan: Create a space, edit a space, archive a space. Verify default space works as intended.

Reviewers: epriestley

Reviewed By: epriestley

Subscribers: Korvin

Differential Revision: https://secure.phabricator.com/D17829
2017-05-04 21:48:13 +00:00
Chad Little
705fd11ba1 Update Legalpad to use modular transactions
Summary: Update Legalpad for modular transactions

Test Plan:
- New Document (no sign)
- New Document (individual)
- New Document (corp)
- Require Signature - get prompted to sign before I can do anything.
- Edit Documents
- Sign Documents
- Comment on Documents

Reviewers: epriestley

Reviewed By: epriestley

Subscribers: Korvin

Differential Revision: https://secure.phabricator.com/D17826
2017-05-04 12:45:02 -07:00
Chad Little
06eae5578b Update Passphrase for modular transactions
Summary: Updates Passphrase for modular transactions.

Test Plan: Create, edit, lock, view, lots of different types of Passphrases. Enable Conduit, Lock Passphrases, Destroy Secrets from the interface and verify from the DB it was eradicated.

Reviewers: epriestley

Reviewed By: epriestley

Subscribers: Korvin

Differential Revision: https://secure.phabricator.com/D17824
2017-05-04 11:31:37 -07:00
Austin McKinley
d34b338f3f Implement modular transactions for application policy changes
Summary: Still needs some cleanup, but ready for review in broad outline form.

Test Plan:
Made lots of policy changes to the Badges application and confirmed expected rows in `application_xactions`, confirmed expected changes to `phabricator.application-settings`.

See example output (not quite working for custom policy objects) here:

{F4922240}

Reviewers: epriestley

Reviewed By: epriestley

Subscribers: Korvin, chad, epriestley

Maniphest Tasks: T11476

Differential Revision: https://secure.phabricator.com/D17757
2017-05-03 17:49:41 -07:00
Chad Little
5307f51170 Update Macro comment form
Summary: Moves over to transaction commenting.

Test Plan: Leave a comment (tested with TYPE_COMMENT still present).

Reviewers: epriestley

Reviewed By: epriestley

Subscribers: Korvin

Differential Revision: https://secure.phabricator.com/D17823
2017-05-03 12:34:21 -07:00
epriestley
60a19e3646 Allow ApplicationTransactionEditor to figure out whether TYPE_COMMENT is supported or not
Summary: See D17812, etc. We can figure this out by looking at the object carefully. We don't need to go delete all the old TYPE_COMMENT (it doesn't hurt anything) but can nuke it when we see it.

Test Plan:
  - Made a comment in Slowvote (supports commenting).
  - Viewed an Almanac device (does not support commenting).

Reviewers: chad

Reviewed By: chad

Differential Revision: https://secure.phabricator.com/D17822
2017-05-03 11:20:57 -07:00
Chad Little
d467a9e337 Modernize PonderQuestion with EditEngine
Summary: Just a small touch up to move this to edit engine.

Test Plan:
- Create a question
- Edit a question
- Close question
- Test NUX state

Reviewers: epriestley

Reviewed By: epriestley

Subscribers: Korvin

Differential Revision: https://secure.phabricator.com/D17812
2017-05-03 11:14:52 -07:00
Chad Little
4aec809b69 Update PhamePost for modular transactions
Summary: Updates PhamePost for modular transactions.

Test Plan:
- Create a post
- Edit a post
- Add a header image
- Delete header image
- Award Token
- Leave comment
- Unpublish post
- Check History page
- Move post
- Archive post

{F4936456}

Reviewers: epriestley

Reviewed By: epriestley

Subscribers: Korvin

Differential Revision: https://secure.phabricator.com/D17818
2017-05-03 11:14:25 -07:00
Chad Little
c878bf5033 Update Macro for EditEngine
Summary: Updates Macro to use EditEngine. Also removes "URL" field for adding a Macro, which I think it's worth pursuing.

Test Plan:
- Create a Macro
- Forget to name it
- Try a PDF
- Use a Macro
- Edit a macro (not working)

Reviewers: epriestley

Reviewed By: epriestley

Subscribers: Korvin

Differential Revision: https://secure.phabricator.com/D17821
2017-05-03 11:11:23 -07:00
Chad Little
21ac196306 Modernize PhameBlog with modular transactions
Summary: Moves PhameBlog over to the wonderful world of modular transactions and the riches that lay beyond...

Test Plan:
- Create Blog
- Edit Blog
- Set Header
- Delete Header
- Add picture
- Archive blog
- Set incorrect domain values
- Be irresponsible with subtitle length
- Activate blog
- Change description

Reviewers: epriestley

Reviewed By: epriestley

Subscribers: Korvin

Differential Revision: https://secure.phabricator.com/D17815
2017-05-02 15:39:56 -07:00
Chad Little
a067040024 Update Macro for Modular Transactions
Summary: Overall, seems to work ok.

Test Plan:
- Add a Macro
- Edit Macro
- Use Macro
- Disable Macro
- Re-enable Macro
- Attach Audio
- Set Audio to loop
- Annoy cats

{F4932069}

Reviewers: epriestley

Reviewed By: epriestley

Subscribers: Korvin

Differential Revision: https://secure.phabricator.com/D17813
2017-05-02 08:41:14 -07:00
Chad Little
eaaba278a9 Convert PonderAnswer to modular transactions
Summary: Fixes T12624. Converts PonderAnswer over to modular transactions.

Test Plan:
- Add an answer
- Edit an answer
- Hide an answer
- Comment on an answer

Reviewers: epriestley

Reviewed By: epriestley

Subscribers: Korvin

Maniphest Tasks: T12624

Differential Revision: https://secure.phabricator.com/D17811
2017-05-01 12:44:30 -07:00
Chad Little
dff028c490 Update PonderQuestion for Modular Transactions
Summary: Ref T12624, this moves PonderQuestion over to modular transactions.

Test Plan:
- Create a question
- Edit a question
- Comment on question
- Answer question
- Edit Answer
- Close Question
- Verify answer count in list views
- Check Question History
- Check Answer History

Reviewers: epriestley

Reviewed By: epriestley

Subscribers: Korvin

Maniphest Tasks: T12624

Differential Revision: https://secure.phabricator.com/D17810
2017-05-01 11:38:25 -07:00
epriestley
0ebe56be40 Fix two Calendar availability cache issues
Summary:
Fixes T12661.

When changing the start date of an event from some time in the past to some time significantly in the future (more than 24 hours), we'd invalidate only future caches and leave users in an "away" state. Instead, just invalidate all past and future caches (this is simpler than trying to figure out a narrower window, and should not make us do too much extra work).

When uninviting users from events, their caches also didn't get cleared correctly. Instead, clear them.

Test Plan:
  - Changed an event from "Apr 1 - June 1" to "May 15 - June 1", saw availablity clear properly.
  - Uninvited user `@dog` from an ongoing event, saw availability clear properly.

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T12661

Differential Revision: https://secure.phabricator.com/D17809
2017-05-01 10:43:51 -07:00
Chad Little
da6d624fe8 Mark ConpherenceCreate and Update conduit calls as frozen
Summary: T12656, mark these methods as frozen and use conpherence.edit instead.

Test Plan: Visit conduit, check status is displayed.

Reviewers: epriestley

Reviewed By: epriestley

Subscribers: Korvin

Differential Revision: https://secure.phabricator.com/D17808
2017-05-01 09:59:38 -07:00
Josh Cox
f2ca348b3a Set project's ObjectName to its PHID when it doesn't have a hashtag
Summary: Fixes T12659. Previously this would lead to an error when trying to run `arc diff` on a revision that had a milestone as a reviewer (or any non-octothorpe'd Object Name)

Test Plan: Followed repro steps in T12659 and didn't get the error described. Also clicked around and didn't notice any obvious regressions in projects or differential

Reviewers: epriestley, #blessed_reviewers

Reviewed By: epriestley, #blessed_reviewers

Subscribers: Korvin, yelirekim

Maniphest Tasks: T12659

Differential Revision: https://secure.phabricator.com/D17807
2017-04-05 06:22:52 -04:00
epriestley
b7c4f60e23 Only recognize "Fixes ..." in main revision content like the Summary or Test Plan
Summary:
Fixes T12642. Currently, writing "Fixes T..." in a comment gets picked up as a formal "fixes".

This is a bit confusing, and can also give you a "no effect" error if you "fixes ..." a task which is already "fixes"'d.

We could make the duplicate action a non-error, but just prevent the text from having an effect instead, which seems cleaner.

Test Plan:
  - Wrote "Fixes ..." in a summary, saw a "fixes" relationship established.
  - Wrote "Fixes ..." in a comment, got a "mention" instead.
  - `var_dump()`'d some stuff as a sanity check, looked reasonable.

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T12642

Differential Revision: https://secure.phabricator.com/D17805
2017-04-30 13:12:28 -07:00
epriestley
89d0c8e388 Use grey dots for disabled users, even if a user is also unverified
Summary: Fixes T12559.

Test Plan:
{F4929988}

{F4929989}

{F4929990}

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T12559

Differential Revision: https://secure.phabricator.com/D17806
2017-04-30 13:10:00 -07:00
Chad Little
608dd06151 Add a lipsum generator for Conpherence Rooms
Summary: Builds a basic room generator for Conpherence, picks a random name, adds 10 random users to it, sets view and edit policy to all users.

Test Plan:
`bin/lipsum generate conpherence`

{F4928815}

Reviewers: epriestley

Reviewed By: epriestley

Subscribers: Korvin

Differential Revision: https://secure.phabricator.com/D17699
2017-04-29 20:08:00 -07:00
Chad Little
d6bce34a5d Update Conpherence to use EditEngine
Summary: Moves Conpherence to use EditEngine. This removes the "First Message" field, but I think that's ok until we have direct messaging of some sort, then maybe have built-ins cover that case.

Test Plan:
 - Visit /new/ and /edit/ for creating new rooms.
 - Edit a room in full conpherence
 - Edit a room in durable column
 - grep for METADATA calls

Reviewers: epriestley

Reviewed By: epriestley

Subscribers: Korvin

Maniphest Tasks: T11729

Differential Revision: https://secure.phabricator.com/D16677
2017-04-29 19:39:12 -07:00
epriestley
d2baa88171 Allow Owners packages to have arbitrarily long names
Summary:
  - Change column type from `sort128` to `sort`.
  - Remove `originalName`. This column is unused. Long ago, we used it to generate a `Thread-Topic` header for mail, but just use PHIDs now (the value just needs to be stable for a given object, users normally don't see it).

Test Plan:
  - Created a package with a beautifully long name. Magnificent!
  - Grepped for `originalName` / `getOriginalName()`, found no Owners hits.
  - Verified that there isn't any name-length validation code to remove.

{F4925637}

Reviewers: chad, amckinley

Reviewed By: chad

Differential Revision: https://secure.phabricator.com/D17798
2017-04-27 18:04:03 -07:00
Chad Little
7824710522 Add an Owners Package hovercard
Summary: Ref T12600. Basically all the property (not path) information on a hovercard for owner packages.

Test Plan:
Create a package with LOTS OF RULES. Test it as open and archived states.

{F4923441}

{F4923444}

Reviewers: epriestley, jmeador

Reviewed By: jmeador

Subscribers: jmeador, Korvin

Maniphest Tasks: T12600

Differential Revision: https://secure.phabricator.com/D17793
2017-04-26 08:45:39 -07:00
Chad Little
d70d09c02c Fix notice spacing on table views
Summary: When a notice is in a table view in a two column layout, reset the margins.

Test Plan: Visit OwnerDetails with no paths set.

Reviewers: epriestley

Reviewed By: epriestley

Subscribers: Korvin

Differential Revision: https://secure.phabricator.com/D17792
2017-04-25 16:46:55 -07:00
Austin McKinley
6bfaf31c0a Spelling fix
Summary: I can't believe I've been staring at this page for so long without noticing this typo

Test Plan: doitlive

Reviewers: #blessed_reviewers, epriestley

Reviewed By: #blessed_reviewers, epriestley

Subscribers: epriestley

Differential Revision: https://secure.phabricator.com/D17791
2017-04-25 12:19:27 -07:00
Chad Little
9509bd1c7f Fix legalpad error display
Summary: Fixes T12636.

Test Plan: Make a from, submit signature without checking box, see correctly aligned error.

Reviewers: epriestley

Reviewed By: epriestley

Subscribers: Korvin

Maniphest Tasks: T12636

Differential Revision: https://secure.phabricator.com/D17789
2017-04-25 09:28:23 -07:00
epriestley
0d5538672c Detect unsynchronizable repositories on multiple cluster hosts
Summary:
Ref T12613. Currently, the SVNTEST and HGTEST repositories are improperly configured on `secure`. These repositories use VCS systems which do not support synchronization, so they can not be served from cluster services with multiple hosts.

However, I've incorrectly configured them the same way as all the Git repositories, which support synchronization. This causes about 50% of requests to randomly fail (when they reach the wrong host).

Detect this issue and warn the user that the configuration is not valid.

It should be exceptionally difficult for normal installs to run into this.

Test Plan:
  - Mostly faked these conditions locally, verified that `secure` really has this configuration.
  - I'll push this, verify that the issue is detected correctly in production, then fix the config which should resolve the intermittent issues with SVNTEST.

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T12613

Differential Revision: https://secure.phabricator.com/D17774
2017-04-24 10:43:05 -07:00
epriestley
41d9453ece When a user changes to a verified primary address, mark their account as verified
Summary:
Ref T12635. See that task for discussion.

You can currently end up with a verified primary address but no "verified" flag on your account through an unusual sequence of address mutations.

Test Plan:
  - Registered without verifying, using address "A".
  - Added a second email address, address "B".
  - Verified B (most easily with `bin/auth verify`).
  - Changed my primary email to B.
  - Before patch: account not verified.
  - After patch: account verified.

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T12635

Differential Revision: https://secure.phabricator.com/D17785
2017-04-24 10:24:21 -07:00
Chad Little
b3c8226cbb New hovercard UI for Maniphest Tasks
Summary: Swaps out hovercard boring view for super cool workboard card view. Will have more diffs to add additional information down the road.

Test Plan: {F4921092}

Reviewers: epriestley

Reviewed By: epriestley

Subscribers: Korvin

Differential Revision: https://secure.phabricator.com/D17769
2017-04-23 18:58:08 -07:00
epriestley
d0e6bf831d Add "%I" (instance name) to application log formats
Summary:
Ref T12611. Currently, the HTTP/SSH logs don't have an option to include the instance name.

Add such an option.

Leave it out of the default logs because most installs don't use this.

Test Plan: See next changes.

Reviewers: chad, amckinley

Reviewed By: chad

Maniphest Tasks: T12611

Differential Revision: https://secure.phabricator.com/D17776
2017-04-23 11:07:19 -07:00
epriestley
bb2b91d28e Add unit tests for file thumbnail generation
Summary: Fixes T12614.

Test Plan:
  - Ran tests, saw them pass.
  - Changed a thumbnail to 800x800, saw tests detect that the default image was missing.

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T12614

Differential Revision: https://secure.phabricator.com/D17773
2017-04-23 11:02:21 -07:00
epriestley
bc9291b327 Fix Conpherence theme variables for both logged-out and logged-in users
Summary: Ref T12622.

Test Plan: As a logged-out and logged-in user, loaded Conpherence threads.

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T12622

Differential Revision: https://secure.phabricator.com/D17768
2017-04-22 09:23:39 -07:00
Chad Little
8d398f6f4a Fix variable name for theme_class
Summary: Fixes T12622. This variable is mis-named.

Test Plan: Visit a room I have not joined.

Reviewers: epriestley, amckinley

Reviewed By: epriestley

Subscribers: Korvin

Maniphest Tasks: T12622

Differential Revision: https://secure.phabricator.com/D17767
2017-04-22 16:01:10 +00:00
epriestley
03d0e3fdbc Fix fatal in Conpherence NUX state
Summary: Fixes T12619.

Test Plan: Faked `return array()` in ConpherneceThreadQuery, got a NUX instead of fatal.

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T12619

Differential Revision: https://secure.phabricator.com/D17764
2017-04-21 14:50:29 -07:00
Chad Little
d3546f94c1 Improve diffusion readme layout
Summary: Uses more standard objects and more padding for reading. Removes the ToC, which is visually broken anyways.

Test Plan: Review a README.md in a local repository.

Reviewers: epriestley

Reviewed By: epriestley

Subscribers: Korvin

Differential Revision: https://secure.phabricator.com/D17752
2017-04-21 11:23:26 -07:00
Chad Little
7c61ace086 Attach Diffusion Pagers to their ObjectBoxView
Summary: Adds the ability to set a pager onto an object box directly and pick up appropriate styles.

Test Plan: grep for renderTablePagerBox, test layouts with and without a pager.

Reviewers: epriestley

Reviewed By: epriestley

Subscribers: Korvin

Maniphest Tasks: T12604

Differential Revision: https://secure.phabricator.com/D17754
2017-04-21 11:22:19 -07:00
Chad Little
ed9afa18cc Allow users to choo choo choose a room color
Summary: This adds some basic per user / per room theming for Conpherence, which should hopefully let users identify rooms from just the sidebar color.

Test Plan: Lots of threads with different colors.

Reviewers: epriestley

Reviewed By: epriestley

Subscribers: Korvin

Differential Revision: https://secure.phabricator.com/D17747
2017-04-20 14:30:59 -07:00
epriestley
7a992b5488 When a package or project has been accepted or rejected, show who did it ("Accepted (by dog)")
Summary: Makes it more clear whose authority actions have been taken under.

Test Plan: {F4916376}

Reviewers: chad

Reviewed By: chad

Differential Revision: https://secure.phabricator.com/D17741
2017-04-20 13:07:08 -07:00
Chad Little
6969e9389f Move room preferences into own controller
Summary: Ref T12591. These preferences are per user and we need to reload the whole UI on a change anyways. Simplifies future expansion.

Test Plan: Option click into new window, set preferences. View in Conpherence, see saved settings, change sound. Hear new sound.

Reviewers: epriestley

Reviewed By: epriestley

Subscribers: Korvin

Maniphest Tasks: T12591

Differential Revision: https://secure.phabricator.com/D17740
2017-04-19 16:48:36 -07:00
epriestley
95dd9dbf43 Make Applications extend LiskDAO
Summary:
Ref T11476. This is a bit hacky, but makes `Application` extend `LiskDAO` so we can apply transactions to it with an `Editor` class.

Also fixes schema stuff so builds should produce a clean bill of health again.

This might only get you slightly further, yell if you run into more trouble.

Test Plan:
  - Ran `bin/storage upgrade -f` and got no warnings.
  - Browsed around, nothing exploded?

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T11476

Differential Revision: https://secure.phabricator.com/D17738
2017-04-19 16:06:14 -07:00
Austin McKinley
d58b808f04 Fixing copy/paste mistake
Test Plan: doitlive

Reviewers: epriestley

Reviewed By: epriestley

Subscribers: Korvin

Differential Revision: https://secure.phabricator.com/D17737
2017-04-19 15:48:59 -07:00
Austin McKinley
febd68039f Add initial infrastructure for adding ModularTransaction support to Application config changes
Summary: Part of the groundwork for T11476.

Test Plan: ran `./bin/storage upgrade` and observed expected DB tables

Reviewers: epriestley

Reviewed By: epriestley

Subscribers: Korvin

Maniphest Tasks: T11476

Differential Revision: https://secure.phabricator.com/D17736
2017-04-19 15:44:57 -07:00
Chad Little
db60af7ea5 Set all room settings at once
Summary: Sets notification and sound preferences in a single array()

Test Plan: Change email preference, save, set sound preference, save. Email preference still OK.

Reviewers: epriestley

Reviewed By: epriestley

Subscribers: Korvin

Differential Revision: https://secure.phabricator.com/D17735
2017-04-19 14:39:17 -07:00
Chad Little
51485a1f82 Add participants ModularTransactions to Conpherence
Summary: Moves participants over to ModularTransactions, simplified a lot of the code. Fixes T12550

Test Plan:
Create a new room with just myself and myself + fake accounts.
Remove a person.
Remove myself.
Edit a room, topic.
Type some messages.
???
Profit

Reviewers: chad

Reviewed By: chad

Subscribers: Korvin

Maniphest Tasks: T12550

Differential Revision: https://secure.phabricator.com/D17685
2017-04-19 14:01:15 -07:00
epriestley
b9868f4f05 Don't require a transaction to mark a participant up-to-date
Summary:
Pathway to D17685. We no longer have "behindTransactionPHID", so we no longer need the latest transaction.

This allows some code to be removed.

Test Plan:
  - Grepped for callsites to `markUpToDate()` and variables used in the calls.
  - Sent messages in a couple threads, viewed them, saw unread counts go away.

Reviewers: chad

Reviewed By: chad

Differential Revision: https://secure.phabricator.com/D17733
2017-04-19 14:00:55 -07:00
epriestley
f46263903c Fix improper filtering behavior in ConpherenceParticipantQuery
Summary:
Pathway to D17685. This fixes an issue idenified in D17731: if any caller ever queried for more than one participant, some results could get thrown away by re-keying the results on thread PHID: two different participants can be members of the same thread!

This also fixes an issue from D17683, where a `needParticipantCache()` callsite was overlooked.

Test Plan:
  - Viewed Conpherence dropdown.
  - Sent messages, saw unread count / thread order still work properly.

Reviewers: chad

Reviewed By: chad

Differential Revision: https://secure.phabricator.com/D17732
2017-04-19 13:59:47 -07:00
epriestley
76d0b67d91 Remove "dateTouched" from ConpherenceParticipant
Summary:
Pathway to D17685. This column is (mostly) a denormalization of `dateModified` on the thread.

Just use a JOIN instead.

This isn't //exactly// the same: we'll bump threads to the top now for non-message changes (e.g., a topic or title change). That seems fine, but we could put a `lastMessageDate` on Thread later if we want to refine it.

Also got rid of a lot of other unused stuff. There's a big garbage TODO here, I'll fix that in the next change.

Test Plan:
  - Grepped for `dateTouched`.
  - Grepped for `participantCursor`.
  - Grepped for `ConpherenceParticipantQuery::LIMIT`.
  - Looked for callsites to `setOrder()`, found none.
  - Added a message to an older thread, saw it bump up to the top.

Reviewers: chad

Reviewed By: chad

Differential Revision: https://secure.phabricator.com/D17731
2017-04-19 13:58:54 -07:00
epriestley
0a335f91cd Remove "participationStatus" from ConpherenceParticipant
Summary:
Pathway to D17685. This column is a very complicated cache of: is participant.messageCount equal to thread.messageCount?

We can just ask this question with a JOIN instead and simplify things dramatically.

Test Plan:
  - Ran migration.
  - Browsed around.
  - Sent a message, saw unread count go up.
  - Read the message, saw unread count go down.

Reviewers: chad

Reviewed By: chad

Differential Revision: https://secure.phabricator.com/D17730
2017-04-19 13:58:42 -07:00
epriestley
c96e17f5ea Remove "behindTransactionPHID" from ConpherenceParticipant
Summary: Pathway to D17685. Nothing reads this field and it has no use or value.

Test Plan:
  - Ran migration.
  - Grepped for `behindTransactionPHID`.

Reviewers: chad

Reviewed By: chad

Differential Revision: https://secure.phabricator.com/D17729
2017-04-19 13:58:29 -07:00
Chad Little
15e7624a17 Add a few more sounds
Summary: A few more mp3s to choose from for Conpherence.

Test Plan: Test each sound in a new room.

Reviewers: epriestley

Reviewed By: epriestley

Subscribers: Korvin

Differential Revision: https://secure.phabricator.com/D17734
2017-04-19 13:47:23 -07:00
Chad Little
8e813f4f1f Add some basic sound preferences
Summary: Ref T7567. This adds some constants (for adding new sounds), global setting for turning on and off sound (setting) and per thread preference for sound choice. Also specc'd out Mentions, if added.

Test Plan: I tested all the preference wiring, but need to set up notifications locally to verify if this works. Feel free to test.

Reviewers: epriestley

Reviewed By: epriestley

Subscribers: amckinley, Korvin

Maniphest Tasks: T7567

Differential Revision: https://secure.phabricator.com/D17726
2017-04-19 13:04:02 -07:00
epriestley
f880000eb0 Stem fulltext tokens before filtering them for stopwords
Summary:
Fixes T12596. A query for a token (like "having") which stems to a stopword (like "have") currently survives filtering. Stem it first so it gets caught.

Also, for InnoDB, a custom stopword table can be configured. If it is, read that instead of the default stopword list (I configured it locally, but the default list is reasonable so we never formally recommended installs configure it).

Test Plan:
Queried for words that stem to stopwords, saw them filtered:

{F4915843}

Queried for the original problem query and saw "having" caught with "have" in the stopword list:

{F4915844}

Fiddled with local InnoDB stopword table config and saw the stopword list get loaded correctly.

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T12596

Differential Revision: https://secure.phabricator.com/D17728
2017-04-19 10:02:21 -07:00
epriestley
b479941ceb Make "Range" HTTP header work for Celerity static resource requests
Summary:
Ref T7567. In T8266 I fixed a bunch of obscure "Range" issues, but only for file downloads -- not for Celerity.

Extend all that stuff to Celerity, which is fortunately much easier.

I believe this will fix Conpherence sounds in Safari.

Test Plan:
  - Wrote out an HTTP request in a text file with `Range: bytes=0-1` and similar, piped it to localhost with `cat request.txt | nc localhost 80`, saw server return appropriate range responses consistent with file behavior after T8266, which all seems to work.
  - Also did that for files to try to make sure I wasn't breaking anything.

Reviewers: chad, amckinley

Reviewed By: chad

Maniphest Tasks: T7567

Differential Revision: https://secure.phabricator.com/D17724
2017-04-18 13:46:27 -07:00
Austin McKinley
ece9579d25 Switch File deletion to use ModularTransactions
Summary: Fixes T12587. Adds a new `PhabricatorFileDeleteTransaction` that enqueues `File` delete tasks.

Test Plan:
  - hack `PhabricatorFileQuery` to ignore isDeleted state
  - stop daemons
  - upload a file, delete it from the UI
  - check that the DB has updated isDeleted = 1
  - check timeline rendering in `File` detail view
  - start daemons
  - confirm rows are deleted from DB

Reviewers: epriestley

Reviewed By: epriestley

Subscribers: Korvin, thoughtpolice

Maniphest Tasks: T12587

Differential Revision: https://secure.phabricator.com/D17723
2017-04-18 13:01:51 -07:00
epriestley
ab2aa74d6e Fix several duplication/replay behaviors in Aphlict
Summary:
Ref T12566. Ref T12563. This fixes three bugs with Aphlict replay stuff:

First, Conphernece would try to repaint the UI even if no thread was open. Only repaint when a thread is open.

Second, although we deduplicate JX.Leader messages, we didn't deduplicate actual notification messages. If you browsed the leader window, then it re-elected itelf as a leader and replayed history, it could rebroadcast notifications and other windows could show doubles. Deduplicate notifications to prevent this.

Third, we always replayed the last 60 seconds of history. When you browsed the leader window, whichever window became the new leader (possibly the one you just browsed) could replay messages from before it had opened, leading to duplicate messages. Particularly, after receiving a message and then browsing you could see that message again. Instead, only replay history as far back as when the window first opened.

Test Plan:
  - Clicked "Repaint" with a thread open, saw a repaint. Clicked "Repaint" with Conpherence open but no thread, no repaint and no 404 request to `/update/null/`.
  - In browser A, opened three windows. In browser B, sent a notification. In browser A, browsed the leader window away twice in a row. Observed that the window which never became a leader doesn't duplicate notifications.
  - In browser A, opened three windows. In browser B, sent a notification. In browser A, browsed the leader window away over and over again. Observed that replay requests issued with appropriate history windows.

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T12566, T12563

Differential Revision: https://secure.phabricator.com/D17722
2017-04-18 12:10:12 -07:00
epriestley
5d55804e3f Play a sound when receiving a new chat message
Summary:
Ref T7567. Nothing fancy yet, just getting this working. Sound is lightly edited version of "Pop 6":

https://www.freesound.org/people/greenvwbeetle/sounds/244656/

Test Plan: Sent chat, heard sounds.

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T7567

Differential Revision: https://secure.phabricator.com/D17721
2017-04-18 11:34:17 -07:00
Austin McKinley
be00264ae7 Make daemons perform file deletion
Summary:
Deletion is a possibly time-intensive process, especially with large
files that are backed by high-latency, chunked storage (such as
S3). Even ~200mb objects take minutes to delete, which makes for an
unhappy experience. Fixes T10828.

Test Plan:
Delete a large file, and stare in awe of the swiftness with
which I am redirected to the main file application.

Reviewers: #blessed_reviewers, epriestley

Reviewed By: #blessed_reviewers, epriestley

Subscribers: thoughtpolice, Korvin

Maniphest Tasks: T10828

Differential Revision: https://secure.phabricator.com/D15743
2017-04-18 11:09:41 -07:00
epriestley
8377bb3637 Raise a tailored error message on "show-outbound --id cat"
Summary: Fixes T12579. Unclear why the user ran this command.

Test Plan: Ran with `--id cat`. Ran with `--id 123`.

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T12579

Differential Revision: https://secure.phabricator.com/D17719
2017-04-18 09:51:26 -07:00
Austin McKinley
b54adc6161 Kick off indexing for File objects on creation
Summary: Ensures that newly-made `File` objects get indexed into the new ngrams index. Fixes T8788.

Test Plan:
  - uploaded a file with daemons stopped; confirmed no new rows in ngrams table
  - started daemons; confirmed indexing of previously-uploaded files happened
  - uploaded a new file with daemons running; confirmed it got added to the index

Not sure how to test the changes to `PhabricatorFileUploadSource->writeChunkedFile()` and `PhabricatorChunkedFileStorageEngine->allocateChunks()`. I spent a few minutes trying to find their callers, but the first looks like it requires a Diffusion repo and the 2nd is only accessible via Conduit. I can test that stuff if necessary, but it's such a small change that I'm not worried about it.

Reviewers: epriestley

Reviewed By: epriestley

Subscribers: Korvin

Maniphest Tasks: T8788

Differential Revision: https://secure.phabricator.com/D17718
2017-04-18 08:38:34 -07:00
Austin McKinley
976fbee877 Implement ngram search for File objects
Summary: Follows the outline in D15656 for implementing ngram search for names of File objects. Also created FileFullTextEngine, because without implementing `PhabricatorFulltextInterface`, `./bin/search` complains that `File` is not an indexable type.

Test Plan:
  - ran `./bin/storage upgrade` to apply the schema change
  - confirmed the presence of a new `file_filename_ngrams` table
  - added a couple file objects
  - ran `bin/search index --type file --force`
  - confirmed the presence of rows in `file_filename_ngrams`
  - did a few keyword searches and saw expected results

Reviewers: epriestley

Reviewed By: epriestley

Subscribers: Korvin

Maniphest Tasks: T8788

Differential Revision: https://secure.phabricator.com/D17702
2017-04-17 17:37:20 -07:00
Chad Little
c98be54bf4 Don't show tag when no topic is set
Summary: Check the strlen of topic before adding a tag to the header in Conpherence.

Test Plan: Remove a topic, no longer see indigo bubble.

Reviewers: epriestley

Reviewed By: epriestley

Subscribers: Korvin

Differential Revision: https://secure.phabricator.com/D17715
2017-04-17 16:15:17 -07:00
epriestley
ffed156981 After a reconnect, repaint Conpherence thread state
Summary: Ref T12566. When we reconnect, refresh the current thread even if we replayed notifications.

Test Plan:
  - Clicked the "Repaint" button, saw the thread refresh.
  - Clicked the "Reconnect" button, saw the thread reresh.
  - Launched `aphlict debug`, killed it, restarted it, saw the thread refresh after reconnect.

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T12566

Differential Revision: https://secure.phabricator.com/D17713
2017-04-17 16:00:32 -07:00
epriestley
eaecf35324 Deduplicate application-level notifications from Aphlict
Summary:
Fixes T12564. We already had some code which seems to deal with this properly, it just wasn't getting used.

Assign each application-level notification a unique ID, then ignore messages with duplicate IDs.

Test Plan:
  - In browser A, loaded `/T123`.
  - In browser B, loaded `/T123`.
  - Made a comment as B.
  - Saw notification as A.
  - Mashed "Replay" a bunch.
  - Before patch: piles of duplicate notifications.
  - After patch: no duplicates.

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T12564

Differential Revision: https://secure.phabricator.com/D17710
2017-04-17 15:55:38 -07:00
epriestley
02194f0fc8 After Aphlict reconnects, ask the server to replay recent messages
Summary:
Fixes T12563. If we've ever seen an "open", mark all future connections as reconnects. When we reconnect, replay recent history.

(Until duplicate messages (T12564) are handled better this may cause some notification duplication.)

Also emit a reconnect event (for T12566) but don't use it yet.

Test Plan: {F4912044}

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T12563

Differential Revision: https://secure.phabricator.com/D17708
2017-04-17 15:54:51 -07:00
epriestley
88157a9442 Hold recent messages in Aphlict so they can be replayed after clients reconnect
Summary:
Ref T12563. Before broadcasting messages from the server, store them in a history buffer.

A future change will let clients retrieve them.

Test Plan:
  - Used the web frontend to look at the buffer, reloaded over time, sent messages. Saw buffer size go up as I sent messages and fall after 60 seconds.
  - Set size to 4 messages, sent a bunch of messages, saw the buffer size max out at 4 messages.

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T12563

Differential Revision: https://secure.phabricator.com/D17707
2017-04-17 15:53:58 -07:00
epriestley
1212047843 Add a "Reconnect" debugging action and show reconnect delays in the console
Summary: Ref T12568. Ref T12567. Allows you to force a reconnect, and shows the reconnect delay on connection close/failure.

Test Plan: {F4911879}

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T12568, T12567

Differential Revision: https://secure.phabricator.com/D17705
2017-04-17 15:51:24 -07:00
epriestley
f394fefe6f Add a very basic "Realtime" log to DarkConsole
Summary: Ref T12568. This begins building toward a more useful realtime debugging console for Leader/Aphlict/general realtime stuff.

Test Plan: {F4911521}

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T12568

Differential Revision: https://secure.phabricator.com/D17701
2017-04-17 15:46:31 -07:00
epriestley
6052bc1933 Extend "fulltext" and "ngrams" interfaces from "indexable" interface
Summary: Ref T8788. See D17702. This allows `bin/search index` to index stuff which only implements `Ngrams`, not `Fulltext`.

Test Plan: Kinda poked around `bin/search index` a bit, yell if you hit more issues deeper down the stack?

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T8788

Differential Revision: https://secure.phabricator.com/D17704
2017-04-17 12:59:41 -07:00
Chad Little
2d00f56837 Use PHUIListItemView in ConpherenceThreadList
Summary: Fixes T12556 Uses more common components in ConpherenceThreadList by moving to PHUIListItemView. Reduces clutter by moving privacy into the header. Gets ride of "See More" double interchanges.

Test Plan:
I need to test this more, doesn't seem to auto-select top room any more, also might build a lipsum generator.

 - Create lots of rooms with various policies
 - Test clicking on policy object
 - Click on different rooms
 - Post in rooms
 - Load up second account, see room numbers
 - Clear room message count by clicking on room

Reviewers: epriestley

Reviewed By: epriestley

Subscribers: Korvin

Maniphest Tasks: T12556

Differential Revision: https://secure.phabricator.com/D17698
2017-04-17 11:21:49 -07:00
Austin McKinley
f801c7ae29 Change PhabricatorPhurlURLViewController to use EditEngine for commenting
Test Plan: Created a phurl, added some comments, confirmed that "Change Subscribers" and "Change Project Tags" are now available in the comment form.

Reviewers: epriestley

Reviewed By: epriestley

Subscribers: chad, Korvin

Maniphest Tasks: T11661

Differential Revision: https://secure.phabricator.com/D17686
2017-04-17 10:19:21 -07:00
Chad Little
a56f9a1a55 Clean up remove participant language in Conpherence
Summary: Updates the language to use "Remove Participant" instead of "Banish User"

Test Plan: Read through the various cases, test them by removing myself or others

Reviewers: epriestley

Reviewed By: epriestley

Subscribers: Korvin

Differential Revision: https://secure.phabricator.com/D17697
2017-04-15 16:07:54 +00:00
Chad Little
c1e8b394cc Fix join and remove policy checks for Conpherence
Summary:
I think these got munged when I removed CAN_JOIN.

 - If you can view the room, you can join it.
 - ~~If you can view the room, you can add others to it.~~ This rule adjustment was removed, see discussion on the revision.
 - If you are a participant in the room, you can remove yourself.
 - If you can edit a room, you can remove anyone.

Test Plan:
Normal feature set:

 - Create a new room that only I can edit, viewable by all users.
 - Leave room (bye k thx)
 - Create another room, myself only
 - Join room from second account
 - See ability to only remove myself
 - Remove myself
 - Rejoin
 - Add third account
 - Log into first account
 - Boot off randos
 - Test joining by green button, message, and by + sign.

Policy consistency:

  - As a user who can not edit the room, tried to add other members. Received policy exception. The `+` button is currently visible and enabled for all users (even users who have not joined the room) but this is pre-existing.

Reviewers: chad

Subscribers: Korvin

Differential Revision: https://secure.phabricator.com/D17696
2017-04-15 06:16:45 -07:00
epriestley
aec19d2acf Reduce code duplication in Phortune account controllers
Summary:
Ref T12451. This is a GREAT comment (A++) but we only need one copy of it.

This uses a pattern similar to Projects, which is a little weird but works well enough.

Test Plan:
  - Viewed all four tabs of an account.
  - Viewed a page with a bad account ID which 404'd properly.

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T12451

Differential Revision: https://secure.phabricator.com/D17694
2017-04-14 10:24:56 -07:00
epriestley
7fbb5f2d92 Reduce some code duplication in PhortuneLandingController
Summary: Ref T12451. This code is the same as the other code.

Test Plan: Went through the default-account case with this code, worked the same as the other code.

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T12451

Differential Revision: https://secure.phabricator.com/D17693
2017-04-14 10:24:32 -07:00
epriestley
505b1d8379 Fix member edit transaction validation so it works for both implicit and explicit account creation
Summary:
Ref T12451. Ref T12484. This should deal with all the `+` / `-` / `=` cases correctly, I think.

Also makes sure that members are real users, not commits or tokens or whatever. And expands the creation test case to make some other basic sanity checks.

Test Plan:
  - Went through implicit first-time creation flow.
  - Went through explicit second-time creation flow.
  - Unit test now passes.

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T12484, T12451

Differential Revision: https://secure.phabricator.com/D17692
2017-04-14 10:24:15 -07:00
epriestley
71d933d496 Add a failing test case for new Phortune account initialization
Summary:
Ref T12451. Ref T12484. I think D17657 fixed this, but caused the bug in D17690. The fix for that causes this bug again.

Put a unit test on it. This test currently fails; I'll correct the bug in the next change.

Test Plan: Ran `arc unit`, saw a failure.

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T12484, T12451

Differential Revision: https://secure.phabricator.com/D17691
2017-04-14 10:23:10 -07:00
epriestley
e1a8b5d3e9 Fix a bug where Phortune accounts created via "Create Account" would not have the viewer added as a member
Summary:
Ref T12451. When you explicitly created a second or third account or whatever, you wouldn't be added as a member.

(The editor sees that you're "already a member", so it doesn't add you.)

Test Plan:
  - Go to `/phortune/`.
  - Click "Switch Accounts".
  - Click "Create Account".
  - Create an account.
  - Before patch: unable to view it since you don't get added as a member.
  - After patch: account created with you as member.
  - Also created an accont with multiple members.
  - Tried to create an account with no members.
  - Tried to create an account with just someone else as a member.

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T12451

Differential Revision: https://secure.phabricator.com/D17690
2017-04-14 10:22:54 -07:00
epriestley
7274e4857c Fix a bug where Phortune could fatal while building crumbs
Summary: Ref T12451. `$this->getAccount()` may not return an account.

Test Plan:
  - Visit `/phortune/X/`, where `X` is the ID of an account you don't have permission to view.
  - Before patch: fatal.
  - After patch: normal policy exception page.

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T12451

Differential Revision: https://secure.phabricator.com/D17689
2017-04-14 10:22:42 -07:00
epriestley
1e43d57c81 When closing tasks with "Fixes xxx", try to act more authentically as the acting user
Summary:
Via HackerOne (<https://hackerone.com/reports/220909>). When we close commits in response to "Fixes Txxx", we currently act as the omnipotent user. This allows users to close tasks they can't see by pushing commits with "Fixes Txxx" in the message.

However, we can't actually tell who authored or committed a change: we're just using the "Author" and "Committer" values from Git in most cases, and anyone can forge those. So we can't really get this right, in a security sense.

(We can tell who //pushed// a change if we host it, but that's often not the right user. If GPG signing was more prevalent, we could use that. In the future, we could use side channels like having `arc land` tell Phabrcator who was pushing changes.)

Since I think the impact of this is fairly minor and this isn't //really// a security issue (more of a confusion/abuse/product issue) I think the behavior is okay more-or-less as-is, but we can do better when we do identify an author: drop permissions, and use their privileges to load the tasks which the commit "fixes".

This effectively implements this rule:

> If we identify the author of a commit as user X, that commit can only affect tasks which user X can see and edit.

Note that:

  - Commits which we can't identify the author for can still affect any task.
  - Any user can forge any other user's identity (or an invalid identity) and affect any task.

So this is just a guard rail to prevent mistakes by good-faith users who type the wrong task IDs, not a real security measure.

Also note that to perform this "attack" you must already have commit access to a repository (or permission to create a repository).

Test Plan:
  - Used `bin/repository reparse --message <commit> --force-autoclose` to run the relevant code.
  - Made the code `throw` before it actually applied the edit.
  - Verified that the edit was rejected if the author was recognized and can not see or could not edit the task.
  - Verified that the edit is accepted if the author can see+edit the task.
  - Verified that the edit is accepted if we can't figure out who the author is.

Reviewers: chad

Reviewed By: chad

Differential Revision: https://secure.phabricator.com/D17688
2017-04-14 08:03:46 -07:00
epriestley
69053a40f9 Dirty the SSH key cache when usernames change
Summary:
Fixes T12554. The SSH key cache contains usernames, but is not currently dirtied on username changes.

An alternative solution would be to use user PHIDs instead of usernames in the file, which would make this unnecessary, but that would make debugging a bit harder. For now, I think this small added complexity is worth the easier debugging, but we could look at this again if cache management gets harder in the future.

Test Plan:
  - Added a key as `ducksey`, ran `bin/ssh-auth`, saw key immediately.
  - Renamed `ducksey` to `ducker`, ran `bin/ssh-auth`, saw username change immediately.
  - Added another key as `ducker`, ran `bin/ssh-auth`, saw key immediately.

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T12554

Differential Revision: https://secure.phabricator.com/D17687
2017-04-14 08:03:00 -07:00
Austin McKinley
980d6cb70b Add validation for config settings of type regex
Summary: Also fixes insufficiently-escaped regex examples

Test Plan: Made several changes to http://local.phacility.com/config/edit/syntax.filemap/ and observed validation failures on malformed regexes, and success on well-formed regexes.

Reviewers: epriestley

Reviewed By: epriestley

Subscribers: Korvin

Maniphest Tasks: T12532

Differential Revision: https://secure.phabricator.com/D17684
2017-04-13 13:57:03 -07:00
Austin McKinley
bfffd807d6 Change syntax highlighting for custom phabricator dot configs
Test Plan:
Created new paste with title '.arcconfig' without choosing a language; observed that the paste gets highlighted as JSON.

JSON mode:
{F4901762}

Javascript mode:
{F4901763}

Reviewers: epriestley

Reviewed By: epriestley

Subscribers: Korvin

Maniphest Tasks: T11667

Differential Revision: https://secure.phabricator.com/D17682
2017-04-13 13:55:33 -07:00
Chad Little
5587abf04c Remove recentParticipants from ConpherenceThread
Summary: We no longer display this any more in the UI, so go ahead and remove the callsites and db column.

Test Plan: New Room, with and without participants.

Reviewers: epriestley

Reviewed By: epriestley

Subscribers: Korvin

Differential Revision: https://secure.phabricator.com/D17683
2017-04-13 13:55:08 -07:00
Austin McKinley
ce06a051a5 Remove old Countdown route
Summary: removes old phabricator.com/countdown/{id} route and code that uses that URL scheme

Test Plan: loaded phabricator.com/countdown, verified that generated links point to phabricator.com/CXXX

Reviewers: epriestley

Reviewed By: epriestley

Subscribers: Korvin

Maniphest Tasks: T12524

Differential Revision: https://secure.phabricator.com/D17681
2017-04-13 13:04:55 -07:00
Chad Little
5c5d3c35a7 Convert date-marker to ModularTransaction in Conpherence
Summary: Swaps this transaction over.

Test Plan: Load up a few rooms with date markers, still render as expected.

Reviewers: epriestley

Reviewed By: epriestley

Subscribers: Korvin

Maniphest Tasks: T12550

Differential Revision: https://secure.phabricator.com/D17680
2017-04-13 13:01:36 -07:00
Austin McKinley
d902d2ac6b Implement countdown.search and countdown.edit
Summary: adds new conduit methods for countdown.edit and countdown.search

Test Plan:
Search: {P2037}
Edit: {P2038}

Reviewers: epriestley

Reviewed By: epriestley

Subscribers: Korvin

Maniphest Tasks: T12524

Differential Revision: https://secure.phabricator.com/D17679
2017-04-13 12:57:10 -07:00
Chad Little
4189eb810b Use violet with not-verified user tags
Summary: Will see how this goes in practice. Uses violet where color is used for non responsive peeps.

Test Plan: Create a user without email verification, test hover card, profile, mentions and lists.

Reviewers: epriestley

Reviewed By: epriestley

Subscribers: Korvin

Differential Revision: https://secure.phabricator.com/D17678
2017-04-13 12:19:49 -07:00
Chad Little
2c5ee2a225 Fix Durable Column CSS-Overload
Summary: This moves the count on the Conpherence Menu Item into a phui-list-item-count, and removes the CSS call to the entire Conphrence stack when durable column is open.

Test Plan: Test with and without the chat column, and a menu with a count

Reviewers: epriestley

Reviewed By: epriestley

Subscribers: Korvin

Differential Revision: https://secure.phabricator.com/D17677
2017-04-13 11:29:30 -07:00
Austin McKinley
9d56a3d86e Reimplement Countdown transactions using Modular Transaction framework
Test Plan: owls

Reviewers: epriestley

Reviewed By: epriestley

Subscribers: Korvin

Differential Revision: https://secure.phabricator.com/D17671
2017-04-13 10:53:57 -07:00
Chad Little
3d6049d0da Remove CAN_JOIN policy from Conpherence
Summary: Fixes T12178, Fixes T11704 Not sure this feature gets any use and I can't find a similar option in other software, so removing it I think simiplifies a number of things. Removes CAN_JOIN and joinable is basically now CAN_VIEW and !$participating. Also removed some old transaction strings for other policies. Don't seem used.

Test Plan: Create a new room, edit room policies, see changes. Log into second account, search for rooms, everything now is visible.

Reviewers: epriestley

Reviewed By: epriestley

Subscribers: Korvin

Maniphest Tasks: T12178, T11704

Differential Revision: https://secure.phabricator.com/D17675
2017-04-13 09:19:50 -07:00
Chad Little
03f2a41b16 Clean up Conpherence Transactions and notifications
Summary: Does a few things. Turns off feed stories (again), removes "action" transactions from notificiations, and only updates message count on actual messages. This feels a bit cleaner and less spammy... I guess... I think @epriestley will really like it and do me a favor or something.

Test Plan: Pull up two windows. test a message, see message count on second screen. Edit a topic or title, get no notification. At all. Ever.

Reviewers: epriestley

Reviewed By: epriestley

Subscribers: Korvin, epriestley

Differential Revision: https://secure.phabricator.com/D17674
2017-04-13 07:20:15 -07:00
epriestley
ada9046e31 Fix a fulltext search issue where finding token length and stopwords could fail
Summary:
Ref T12137. If a database is missing the InnoDB or MyISAM table engines, the big combined query to get both will fail.

Instead, try InnoDB first and then MyISAM.

(I have both engines locally so this worked until I deployed it.)

Test Plan: Faked an InnoDB error like `secure`, got a MyISAM result.

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T12137

Differential Revision: https://secure.phabricator.com/D17673
2017-04-12 19:22:46 -07:00
epriestley
3245e74f16 Show users how fulltext search queries are parsed and executed; don't query stopwords or short tokens
Summary:
Depends on D17670. Fixes T12137. Fixes T12003. Ref T2632.

This shows users a readout of which terms were actually searched for.

This also drops those terms from the query we submit to the backend, dodging the weird behaviors / search engine bugs in T12137.

This might need some design tweaking.

Test Plan: {F4899825}

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T12137, T12003, T2632

Differential Revision: https://secure.phabricator.com/D17672
2017-04-12 19:07:54 -07:00
epriestley
cb49acc2ca Update Phabricator to use intermediate tokens from the query compiler
Summary:
Depends on D17669. Ref T12137. Ref T12003. Ref T2632. Ref T7860.

Converts Phabricator to the new parse + compile workflow with intermediate tokens.

Also fixes a bug where searches for `cat"` or similar (unmatched quotes) wouldn't produce a nice exception.

Test Plan:
  - Fulltext searched.
  - Fulltext searched in Conpherence.
  - Fulltext searched with bad syntax.

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T12137, T12003, T7860, T2632

Differential Revision: https://secure.phabricator.com/D17670
2017-04-12 19:07:33 -07:00
epriestley
4bf968148c Fix pagination of fulltext search results
Summary:
Fixes T8285. Fulltext search relies on an underlying engine which can not realistically use cursor paging. This is unusual and creates some oddness.

Tweak a few numbers -- and how offsets are handled -- to separate the filtered offset and unfiltered offset.

Test Plan:
  - Set page size to 2.
  - Ran a query.
  - Paged forward and backward through results sensibly, seeing the full result set.

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T8285

Differential Revision: https://secure.phabricator.com/D17667
2017-04-12 17:57:46 -07:00
Chad Little
a7ebfc12c0 Modernize Conpherence with Modular Transactions
Summary: Begin converting Conpherence to ModularTransactions, this converts title, topic, and picture to use modular transactions. Participants seems hairy so I'll do that in another diff

Test Plan: Create a room with a topic, change room name, topic. Add people, remove people. Set a room image. Unset topic.

Reviewers: epriestley

Reviewed By: epriestley

Subscribers: Korvin

Differential Revision: https://secure.phabricator.com/D17668
2017-04-12 16:33:57 -07:00
Chad Little
a9845b0b1d Remove picture crop transaction from Conpherence
Summary: Fixes T11730. Removes an old transaction that hasn't been used in a year.

Test Plan: Run sql, check various rooms.

Reviewers: epriestley

Reviewed By: epriestley

Subscribers: Korvin

Maniphest Tasks: T11730

Differential Revision: https://secure.phabricator.com/D17666
2017-04-12 14:18:54 -07:00
Austin McKinley
c6c25b055b Cleanup Countdown manual construction of monograms/uris
Summary: looked for places where Countdown monograms/uris were being constructed by hand, and updated with modern versions

Test Plan: clicked around the Countdown UI, looking for broken links

Reviewers: epriestley

Reviewed By: epriestley

Subscribers: chad, Korvin

Maniphest Tasks: T12524

Differential Revision: https://secure.phabricator.com/D17665
2017-04-12 13:33:19 -07:00
Chad Little
6bf595b951 Check is viewer is a participant before showing count
Summary: In Conpherence ProfileMenuItem we show an unread count if you're a participant, but all message count if you're not. Just remove that.

Test Plan: Log out of room in Conpherence, leave messages on second account, check menu item on both accounts.

Reviewers: epriestley

Reviewed By: epriestley

Subscribers: Korvin

Differential Revision: https://secure.phabricator.com/D17664
2017-04-12 13:27:07 -07:00
Chad Little
75303567b3 Add a Conpherence Profile Menu Item
Summary: Builds a Conpherence Profile Menu Item, complete with counts for the unreads. This allows pinning to home as well as swapping out thread list in Conpherence for pinning eventually.

Test Plan: Add a menu item, chat in room, log into other account, see room count. Room count disappears after viewing.

Reviewers: epriestley

Reviewed By: epriestley

Subscribers: Korvin

Differential Revision: https://secure.phabricator.com/D17662
2017-04-12 13:07:44 -07:00
Chad Little
099c90e7ec Remove "First Message" from New Conpherence Room workflow
Summary: Removes this feature, makes creating a room simpler and less confusing.

Test Plan: Create a room on Conpherence.

Reviewers: epriestley

Reviewed By: epriestley

Subscribers: Korvin

Differential Revision: https://secure.phabricator.com/D17661
2017-04-12 09:15:50 -07:00
Chad Little
cd7547dc57 Update UI for PhortuneAccount
Summary: Primarily, this splits individual sections of the single account page into a more managable and robust sidenav for subscriptions, billing, and managers. The functionality on the subpages is light, but I expect to build on then in coming diffs. This also starts building out a more effective "status" area on the lead page.

Test Plan:
- Load up default account
- Make some edits
- Click on each of the new navigation items
- Verify links to "see all" work
- Test overdue and no payment states for status

{F4337317}

Reviewers: epriestley

Reviewed By: epriestley

Subscribers: Korvin

Differential Revision: https://secure.phabricator.com/D17589
2017-04-11 16:54:58 -07:00
Austin McKinley
6886e9c12d Remove "Destroy" action for Countdown objects
Summary: fixes T12523

Test Plan:
- view Countdown edit screen, Destroy action missing
- checked that `./bin/remove destroy <some-countdown-phid>` removes the DB rows as expected

Reviewers: epriestley

Reviewed By: epriestley

Subscribers: Korvin

Maniphest Tasks: T12523

Differential Revision: https://secure.phabricator.com/D17659
2017-04-11 16:52:35 -07:00
Chad Little
149c1a6de7 Correctly initialize new PhortuneAccount automatically
Summary: There is currently a validation error triggered if you initialize a new account without a member set. I think this is the correct fix, but let me know.

Test Plan: truncate phortune_account database, navigate to phortune, see account automatically created to "Default Account".

Reviewers: epriestley

Reviewed By: epriestley

Subscribers: Korvin

Differential Revision: https://secure.phabricator.com/D17657
2017-04-11 22:46:40 +00:00
epriestley
eeef60a678 Update "bin/policy show" to use PolicyCodex
Summary: Fixes T12541. `describeAutomaticCapability()` is no longer required to implement `PolicyInterface`. Use PolicyCodex instead.

Test Plan: {F4889642}

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T12541

Differential Revision: https://secure.phabricator.com/D17658
2017-04-11 15:21:18 -07:00
Chad Little
58a127f2f9 Update Phortune Merchant UI
Summary: Builds out Phortune Merchant pages to have a sidenav and sub-pages for further expansion. For now this links Orders and Subscriptions to the query engine pages, but could be split out to be more informative (unpaid, upcoming, etc).

Test Plan:
Create a new merchant, edit some information, add a manager in new UI, edit logo, click through to subscriptions, orders.

{F4883013}

Reviewers: epriestley

Reviewed By: epriestley

Subscribers: Korvin

Differential Revision: https://secure.phabricator.com/D17655
2017-04-11 14:36:49 -07:00
Chad Little
5dd18a7ec1 Modernize PhortuneAccount with EditEngine/Modular Transactions
Summary: This updates the backend of PhortuneAccount to use EditEngine and Modular Transactions and updates language to "account manager" for clarity of role.

Test Plan:
- Wiped `phortune_account` table
- Visit Phortune, see new account automatically created.
- Edit name and managers
- Try to set no name or remove myself as a manager, get error messages
- Visit `/phortune/` and create another new account

Reviewers: epriestley

Reviewed By: epriestley

Subscribers: Korvin

Differential Revision: https://secure.phabricator.com/D17585
2017-04-11 12:33:15 -07:00
epriestley
21709a2bbc Remove 'isPartial' parameter with no effect
Summary: Fixes T12536. Nothing reads this parameter; `PhabricatorFile::newChunkedFile` sets the `isPartial` flag automatically.

Test Plan: Grepped for `isPartial`.

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T12536

Differential Revision: https://secure.phabricator.com/D17654
2017-04-11 11:29:23 -07:00
epriestley
af1d494d66 Fix an issue where rejecting reviewers weren't powerful enough
Summary:
Previously, "reject" and "reject older" were separate statuses. Now, they're both shades of "reject".

Set the "older reject" flag properly when we find a non-current reject.

Test Plan:
  - User A accepts a revision.
  - User B rejects it.
  - Author updates it.
  - Before patch: incorrectly transitions to "accepted" ("older" reject is ignored).
  - After patch: correctly transitions to "needs review".

Reviewers: chad

Reviewed By: chad

Differential Revision: https://secure.phabricator.com/D17653
2017-04-11 09:54:34 -07:00
Chad Little
28941b3105 Update PhortuneMerchant to Modular Transactions
Summary: Modernize PhortuneMerchant for Modular Transactions. Also changed the language of "Members" to "Managers", which I think fits better given the power/capability.

Test Plan:
- Create a new Merchant
- Test not filling in a name, see error
- Test removing myself, see error
- Edit an existing Merchant
- Add new managers
- Test removing myself, see error
- Replace Picture
- Update various fields, contact info, email, footer
- Verify transactions are now nice and pretty

Reviewers: epriestley

Reviewed By: epriestley

Subscribers: Korvin

Differential Revision: https://secure.phabricator.com/D17584
2017-04-11 09:32:12 -07:00
epriestley
26d6096e0a When reviewing, always show "Accept" checkboxes for packages/projects, even if there's only one checkbox
Summary: Fixes T12533.

Test Plan: {F4853371}

Reviewers: chad, lvital

Reviewed By: lvital

Maniphest Tasks: T12533

Differential Revision: https://secure.phabricator.com/D17652
2017-04-10 17:28:02 -07:00
epriestley
a7a068f84c Correct two parameter strictness issues with file uploads
Summary:
Fixes T12531. Strictness fallout from adding typechecking in D17616.

  - `chunkedHash` is not a real parameter, so the new typechecking was unhappy about it.
  - `mime-type` no longer allows `null`.

Test Plan:
  - Ran `arc upload --conduit-uri ... 12MB.zero` on a 12MB file full of zeroes.
  - Before patch: badness, failure, fallback to one-shot uploads.
  - After patch: success and glory.

Reviewers: chad

Subscribers: joshuaspence

Maniphest Tasks: T12531

Differential Revision: https://secure.phabricator.com/D17651
2017-04-10 16:01:15 -07:00
epriestley
49132b884b Sell Yellow! Buy Indigo!
Summary: Fixes T12504. Replaces all tags with indigo.

Test Plan: {F4849487}

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T12504

Differential Revision: https://secure.phabricator.com/D17649
2017-04-10 15:01:10 -07:00
Chad Little
4a84954957 Prevent Send on Enter in Fullscreen Remarkup Mode
Summary: Fixes T12138. Test for the presence of being in fullscreen mode, and disable send on enter if present. Side note, I'd love a first class "hasClass" type Javelin function.

Test Plan:
- Go to Conpherence
- Type some smack, see it send on enter
- Go fullscreen like a boss
- Let the words flow
- Close fullscreen, then send on enter.
- (might be nice someday to add a "submit" button to fullscreen editor)

Reviewers: epriestley

Reviewed By: epriestley

Subscribers: Korvin

Maniphest Tasks: T12138

Differential Revision: https://secure.phabricator.com/D17590
2017-04-10 14:39:50 -07:00
epriestley
00a1dec7a6 Render timezones in event reminder mail, and render them more nicely
Summary:
Fixes T12356.

  - In this mail, we currently render "6:00 AM". Instead, render "6:00 AM (PDT)" or similar. This is consistent with times in other modern Transaction mail.
  - Previously, we would render "UTC-7". Render "PDT" instead. For obscure zones with no known timezone abbreviation, fall back to "UTC-7".

Test Plan:
  - Used `bin/calendar notify --minutes X` to trigger notifications, read email bodies.
  - Used this script to list all `T` values and checked them for sanity:

```lang=php
<?php

$now = new DateTime();

$locales = DateTimeZone::listIdentifiers();
foreach ($locales as $locale) {
  $zone = new DateTimeZone($locale);
  $now->setTimeZone($zone);

  printf(
    "%s (%s)\n",
    $locale,
    $now->format('T'));
}
```

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T12356

Differential Revision: https://secure.phabricator.com/D17646
2017-04-10 08:48:37 -07:00
epriestley
50e809e06f Fix an issue where recurring ghost events could go missing if queried with a limit
Summary:
Ref T11816. Depends on D17644. When you executed a query like "upcoming, limit 5 events" you might match some recurring events starting from, say, a year ago and repeating every month.

We'd then generate the first 5 ghosts for these events (say, last January, February, ... May) and later throw them out, so the correct events in the query window (say, this April) would never get generated.

Instead, generate ghosts beginning with the start of the window. The fix in D17644 to number results correctly allows us to do this.

Test Plan:
  - Made a query panel showing 5 events, scheduled an event long in the past, did not visit any of the instances of it so they didn't generate concrete objects.
  - Before the patch, near-future instances failed to show; after the patch, they show.

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T11816

Differential Revision: https://secure.phabricator.com/D17645
2017-04-10 08:48:21 -07:00
epriestley
ab06a9681c Fix two issues with user Calendar event availability cache display
Summary:
Ref T11816. Two minor issues:

  - We used `$event`, not `$next_event`, as the event providing the PHID for "Busy at <event name>". This rendered "Busy at <most future event>" on the profile instead of "Busy at <next upcoming event".
  - The TTL computation used the event start, not the event end, so we could end up rebuilding the cache too often for users busy at an event.

Test Plan:
  - Attended an event in the near future and one later on.
  - Saw profile now say "busy at <near future event>" correctly.
  - In DarkConsole "Services" tab, no longer saw unnecessary cache refills while attending an event.

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T11816

Differential Revision: https://secure.phabricator.com/D17643
2017-04-10 08:47:27 -07:00
epriestley
7707685733 Fix two strings with missing pht()
Summary: Fixes T12517.

Test Plan: Viewed Config application; viewed repository list.

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T12517

Differential Revision: https://secure.phabricator.com/D17639
2017-04-07 10:07:01 -07:00
Rabah Meradi
0bf106eeea Fix 4 typos in code
Summary: Fixes T12516

Test Plan: grep for those typos

Reviewers: #blessed_reviewers, epriestley

Reviewed By: #blessed_reviewers, epriestley

Subscribers: epriestley, PHID-OPKG-gm6ozazyms6q6i22gyam

Maniphest Tasks: T12516

Differential Revision: https://secure.phabricator.com/D17638
2017-04-07 04:09:56 -07:00
epriestley
9856802ba2 Disallow /source/ in robots.txt
Summary: Ref T4245. We disallow `/diffusion/` in robots.txt already because indexers tend to get lost blaming every line of every file throughout history, but didn't update the list for the `/source/` alias. Update it.

Test Plan: Visited `/robots.txt` locally.

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T4245

Differential Revision: https://secure.phabricator.com/D17637
2017-04-06 16:28:09 -07:00
epriestley
3d816e94df Rename "PhabricatorHash::digest()" to "weakDigest()"
Summary: Ref T12509. This encourages code to move away from HMAC+SHA1 by making the method name more obviously undesirable.

Test Plan: `grep`, browsed around.

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T12509

Differential Revision: https://secure.phabricator.com/D17632
2017-04-06 15:43:33 -07:00
epriestley
3a3626834e Replace Remarkup calls to PhabricatorHash::digest() with SHA256
Summary:
Ref T12509. Many of the calls to HMAC+SHA1 are just to compute cachekeys for remarkup objects.

Make these use HMAC+SHA256 instead. There is no downside to swapping these since they just cause a cache miss in the worst case.

I also plan to get rid of `PhabricatorMarkupInterface` eventually, but this doesn't go that far.

Test Plan: Browsed some different types of documents (tasks, legalpad documents, phame blogs / posts, pholio mocks, etc).

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T12509

Differential Revision: https://secure.phabricator.com/D17631
2017-04-06 15:43:18 -07:00
epriestley
d450a08890 Support HMAC+SHA256 with automatic key generation and management
Summary:
Ref T12509. This adds support for HMAC+SHA256 (instead of HMAC+SHA1). Although HMAC+SHA1 is not currently broken in any sense, SHA1 has a well-known collision and it's good to look at moving away from HMAC+SHA1.

The new mechanism also automatically generates and stores HMAC keys.

Currently, HMAC keys largely use a per-install constant defined in `security.hmac-key`. In theory this can be changed, but in practice essentially no install changes it.

We generally (in fact, always, I think?) don't use HMAC digests in a way where it matters that this key is well-known, but it's slightly better if this key is unique per class of use cases. Principally, if use cases have unique HMAC keys they are generally less vulnerable to precomputation attacks where an attacker might generate a large number of HMAC hashes of well-known values and use them in a nefarious way. The actual threat here is probably close to nonexistent, but we can harden against it without much extra effort.

Beyond that, this isn't something users should really have to think about or bother configuring.

Test Plan:
  - Added unit tests.
  - Used `bin/files integrity` to verify, strip, and recompute hashes.
  - Tampered with a generated HMAC key, verified it invalidated hashes.

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T12509

Differential Revision: https://secure.phabricator.com/D17630
2017-04-06 15:42:59 -07:00
epriestley
08a4225437 Provide "bin/files integrity" for debugging, maintaining and backfilling integrity hashes
Summary:
Ref T12470. Provides an "integrity" utility which runs in these modes:

  - Verify: check that hashes match.
  - Compute: backfill missing hashes.
  - Strip: remove hashes. Useful for upgrading across a hash change.
  - Corrupt: intentionally corrupt hashes. Useful for debugging.
  - Overwrite: force hash recomputation.

Users normally shouldn't need to run any of this stuff, but this provides a reasonable toolkit for managing integrity hashes.

I'll recommend existing installs use `bin/files integrity --compute all` in the upgrade guidance to backfill hashes for existing files.

Test Plan:
  - Ran the script in many modes against various files, saw expected operation, including:
  - Verified a file, corrupted it, saw it fail.
  - Verified a file, stripped it, saw it have no hash.
  - Stripped a file, computed it, got a clean verify.
  - Stripped a file, overwrote it, got a clean verify.
  - Corrupted a file, overwrote it, got a clean verify.
  - Overwrote a file, overwrote again, got a no-op.

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T12470

Differential Revision: https://secure.phabricator.com/D17629
2017-04-06 15:42:43 -07:00
epriestley
845a7d8716 Allow the PullLocal daemon to actually hibernate
Summary:
Ref T12298. The PullLocal daemon has had hibernation code for a little while, but it never actually activated because we don't sleep for more than 15 seconds in any case.

Add a maximum sleep instead and use that to control the longest sleep we'll do for hibernation purposes.

Also, when a repository or repository URI is edited, write a NEEDS_UPDATE event into the message table to make sure the daemons de-hibernate.

Test Plan: Used `bin/phd debug pull`, saw the daemon actually hibernate instead of just sleeping for 15 seconds.

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T12298

Differential Revision: https://secure.phabricator.com/D17635
2017-04-06 15:41:19 -07:00
epriestley
f1eeaaf59f Fix scope of "Accept" when you don't check all the "Force Accept" boxes
Summary:
Ref T12272. I wrote this correctly, then broke it by adding the simplification which treats "accept the defaults" as "accept everything".

This simplification lets us render "epriestley accepted this revision." instead of "epriestley accepted this revision onbehalf of: long, list, of, every, default, reviewer, they, have, authority, over." so it's a good thing, but make it only affect the reviewers it's supposed to affect.

Test Plan:
  - Did an accept with a force-accept available but unchecked.
  - Before patch: incorrectly accepted all possible reviewers.
  - After patch: accepted only checked reviewers.
  - Also checked the force-accept box, accepted, got a proper force-accept.

Reviewers: chad, lvital

Reviewed By: lvital

Maniphest Tasks: T12272

Differential Revision: https://secure.phabricator.com/D17634
2017-04-06 15:03:32 -07:00
epriestley
cefbdbcffe Provide a "Reviewers" attachment to "differential.revision.search"
Summary: Allow API callers to retrieve reviewer information via a new "reviewers" attachment.

Test Plan: {F4675784}

Reviewers: chad, lvital

Reviewed By: lvital

Subscribers: lvital

Differential Revision: https://secure.phabricator.com/D17633
2017-04-06 14:46:39 -07:00
epriestley
2f4ff6a850 Fix bad "editPolicy" key in Paste
Summary: Fixes T12508. Files don't have an `editPolicy`, and we started actually checking that the keys are real things in D17616.

Test Plan:
  - Before patch: created a paste, got an "editPolicy" exception.
  - After patch: created a paste that worked properly.

Reviewers: avivey, chad

Reviewed By: avivey

Maniphest Tasks: T12508

Differential Revision: https://secure.phabricator.com/D17628
2017-04-05 13:09:51 -07:00
epriestley
d1a971e221 Support "Range: bytes=123-" requests
Summary:
Ref T12219. We currently only support Range requests like "bytes=123-456", but "bytes=123-", meaning "until end of file", is valid, and Chrome can send these requests.

I suspect this is the issue with T12219.

Test Plan: Used `nc local.phacility.com 80` to pipe raw requests, saw both "bytes=123-456" and "bytes=123-" requests satisfied correctly.

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T12219

Differential Revision: https://secure.phabricator.com/D17626
2017-04-05 11:25:44 -07:00
epriestley
63828f5806 Store and verify content integrity checksums for files
Summary:
Ref T12470. This helps defuse attacks where an adversary can directly take control of whatever storage engine files are being stored in and change data there. These attacks would require a significant level of access.

Such attackers could potentially attack ranges of AES-256-CBC encrypted files by using Phabricator as a decryption oracle if they were also able to compromise a Phabricator account with read access to the files.

By storing a hash of the data (and, in the case of AES-256-CBC files, the IV) when we write files, and verifying it before we decrypt or read them, we can detect and prevent this kind of tampering.

This also helps detect mundane corruption and integrity issues.

Test Plan:
  - Added unit tests.
  - Uploaded new files, saw them get integrity hashes.
  - Manually corrupted file data, saw it fail. Used `bin/files cat --salvage` to read it anyway.
  - Tampered with IVs, saw integrity failures.

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T12470

Differential Revision: https://secure.phabricator.com/D17625
2017-04-05 11:12:31 -07:00
epriestley
45fc4f6f64 Iterate over ranges correctly for encryped files
Summary:
Fixes T12079. Currently, when a file is encrypted and a request has "Content-Range", we apply the range first, //then// decrypt the result. This doesn't work since you can't start decrypting something from somewhere in the middle (at least, not with our cipher selection).

Instead: decrypt the result, //then// apply the range.

Test Plan: Added failing unit tests, made them pass

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T12079

Differential Revision: https://secure.phabricator.com/D17623
2017-04-05 09:56:30 -07:00
epriestley
f70ff34d97 Fix a copy/paste typo with sticky accept
The root issue here is actually just that I cherry-picked stable locally
but did not push it. However, this is a minor issue I also caught while
double-checking things.

Auditors: chad
2017-04-04 18:33:59 -07:00
epriestley
58011a4e8e Upgrade File content hashing to SHA256
Summary:
Ref T12464. This defuses any possible SHA1-collision attacks by using SHA256, for which there is no known collision.

(SHA256 hashes are larger -- 256 bits -- so expand the storage column to 64 bytes to hold them.)

Test Plan:
  - Uploaded the same file twice, saw the two files generate the same SHA256 content hash and use the same underlying data.
  - Tried with a fake hash algorihtm ("quackxyz") to make sure the failure mode worked/degraded correctly if we don't have SHA256 for some reason. Got two valid files with two copies of the same data, as expected.

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T12464

Differential Revision: https://secure.phabricator.com/D17620
2017-04-04 16:23:08 -07:00
epriestley
440ef5b7a7 Remove SHA1 file content hashing and make Files work without any hashing
Summary:
Ref T12464. We currently use SHA1 to detect when two files have the same content so we don't have to store two copies of the data.

Now that a SHA1 collision is known, this is theoretically dangerous. T12464 describes the shape of a possible attack.

Before replacing this with something more robust, shore things up so things work correctly if we don't hash at all. This mechanism is entirely optional; it only helps us store less data if some files are duplicates.

(This mechanism is also less important now than it once was, before we added temporary files.)

Test Plan: Uploaded multiple identical files, saw the uploads work and the files store separate copies of the same data.

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T12464

Differential Revision: https://secure.phabricator.com/D17619
2017-04-04 16:22:10 -07:00
epriestley
1e181f0781 Deprecate "file.uploadhash"
Summary:
Ref T12464. This is a very old method which let you create a file on the server by referring to data which already existed in another file.

Basically, long ago, `arc` could say "Do you already have a file with hash X?" and just skip some work if the server did.

`arc` has not called this method since D13017, in May 2015.

Since it's easy to do so, just make this method pretend that it never has the file. Very old clients will continue to work, since they would expect this response in the common case and continue by uploading data.

Test Plan:
  - Grepped for `uploadhash` in Phabricator and Arcanist.
  - Called the method with the console, verified it returned `null`.

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T12464

Differential Revision: https://secure.phabricator.com/D17618
2017-04-04 16:18:26 -07:00
epriestley
873b39be82 Remove PhabricatorFile::buildFromFileDataOrHash()
Summary:
Ref T12464. This is a very old method which can return an existing file instead of creating a new one, if there's some existing file with the same content.

In the best case this is a bad idea. This being somewhat reasonable predates policies, temporary files, etc. Modern methods like `newFromFileData()` do this right: they share underlying data in storage, but not the actual `File` records.

Specifically, this is the case where we get into trouble:

  - I upload a private file with content "X".
  - You somehow generate a file with the same content by, say, viewing a raw diff in Differential.
  - If the diff had the same content, you get my file, but you don't have permission to see it or whatever so everything breaks and is terrible.

Just get rid of this.

Test Plan:
  - Generated an SSH key.
  - Viewed a raw diff in Differential.
  - (Did not test Phragment.)

Reviewers: chad

Reviewed By: chad

Subscribers: hach-que

Maniphest Tasks: T12464

Differential Revision: https://secure.phabricator.com/D17617
2017-04-04 16:18:00 -07:00
epriestley
45b386596e Make the Files "TTL" API more structured
Summary:
Ref T11357. When creating a file, callers can currently specify a `ttl`. However, it isn't unambiguous what you're supposed to pass, and some callers get it wrong.

For example, to mean "this file expires in 60 minutes", you might pass either of these:

  - `time() + phutil_units('60 minutes in seconds')`
  - `phutil_units('60 minutes in seconds')`

The former means "60 minutes from now". The latter means "1 AM, January 1, 1970". In practice, because the GC normally runs only once every four hours (at least, until recently), and all the bad TTLs are cases where files are normally accessed immediately, these 1970 TTLs didn't cause any real problems.

Split `ttl` into `ttl.relative` and `ttl.absolute`, and make sure the values are sane. Then correct all callers, and simplify out the `time()` calls where possible to make switching to `PhabricatorTime` easier.

Test Plan:
- Generated an SSH keypair.
- Viewed a changeset.
- Viewed a raw diff.
- Viewed a commit's file data.
- Viewed a temporary file's details, saw expiration date and relative time.
- Ran unit tests.
- (Didn't really test Phragment.)

Reviewers: chad

Reviewed By: chad

Subscribers: hach-que

Maniphest Tasks: T11357

Differential Revision: https://secure.phabricator.com/D17616
2017-04-04 16:16:28 -07:00
epriestley
2896da384c Only require POST to fetch file data if the viewer is logged in
Summary:
Ref T11357. In D17611, I added `file.search`, which includes a `"dataURI"`. Partly, this is building toward resolving T8348.

However, in some cases you can't GET this URI because of a security measure:

  - You have not configured `security.alternate-file-domain`.
  - The file isn't web-viewable.
  - (The request isn't an LFS request.)

The goal of this security mechanism is just to protect against session hijacking, so it's also safe to disable it if the viewer didn't present any credentials (since that means there's nothing to hijack). Add that exception, and reorganize the code a little bit.

Test Plan:
  - From the browser (with a session), tried to GET a binary data file. Got redirected.
  - Got a download with POST.
  - From the CLI (without a session), tried to GET a binary data file. Go a download.

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T11357

Differential Revision: https://secure.phabricator.com/D17613
2017-04-04 16:16:01 -07:00
epriestley
2369fa38e1 Provide a modern ("v3") API for querying files ("file.search")
Summary: Ref T11357. Implements a modern `file.search` for files, and freezes `file.info`.

Test Plan: Ran `file.search` from the Conduit console.

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T11357

Differential Revision: https://secure.phabricator.com/D17612
2017-04-04 16:15:36 -07:00
epriestley
260a08a128 Move Files editing and commenting to EditEngine
Summary:
Ref T11357. This moves editing and commenting (but not creation) to EditEngine.

Since only the name is really editable, this is pretty straightforward.

Test Plan: Renamed files; commented on files.

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T11357

Differential Revision: https://secure.phabricator.com/D17611
2017-04-04 16:15:11 -07:00
epriestley
8500f78e45 Move Files to ModularTransactions
Summary: Ref T11357. A lot of file creation doesn't go through transactions, so we only actually have one real transaction type: editing a file name.

Test Plan:
Created and edited files.

{F4559287}

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T11357

Differential Revision: https://secure.phabricator.com/D17610
2017-04-04 10:25:05 -07:00
epriestley
5e44711218 Provide a missing feed transaction string for space creation
Summary:
Fixes T12502. This transaction probably should not be getting picked for feed rendering, but it currently does get selected in some cases.

This should probably be revisited eventually (e.g., when Maniphest moves to ModularTransactions) but just fix the brokenness for now.

Test Plan:
  - Created a task in a space.
  - Viewed feed.
  - Saw the story render with readable text.

{F4555747}

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T12502

Differential Revision: https://secure.phabricator.com/D17609
2017-04-04 10:24:11 -07:00
epriestley
9ebb5f8cda Don't downgrade accepts on update (fix "sticky accept")
Summary:
Fixes T12496. Sticky accept was accidentally impacted by the "void" changes in D17566.

Instead, don't always downgrade all accepts/rejects: on update, we only want to downgrade accepts.

Test Plan:
  - With sticky accept off, updated an accepted revision: new state is "needs review".
  - With sticky accept on, updated an accepted revision: new state is "accepted" (sticky accept working correctly).
  - Did "reject" + "request review" to make sure that still works, worked fine.

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T12496

Differential Revision: https://secure.phabricator.com/D17605
2017-04-03 09:55:22 -07:00
epriestley
163e1ec442 Expose the commit/task/revision relationship edges to "edge.search"
Summary: Fixes T12480.

Test Plan: {F4465908}

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T12480

Differential Revision: https://secure.phabricator.com/D17604
2017-04-02 19:49:55 -07:00
epriestley
009aff1a23 Return task descriptions from "maniphest.search"
Summary:
Fixes T12461. This returns the field as a dictionary with a `"raw"` value, so we could eventually do this if we want without breaking the API:

```
{
  "type": "remarkup",
  "raw": "**raw**",
  "html": "<strong>raw</strong>",
  "text": "raw"
}
```

Test Plan: Called `maniphest.search`, reviewed output.

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T12461

Differential Revision: https://secure.phabricator.com/D17603
2017-04-02 17:44:22 -07:00
epriestley
7e6f37fffb Rename "ElasticSearch" filenames to "Elasticsearch" (2/2)
Sometimes git does some odd magic on case-insensitive filesystems, try to
trick it.

Auditors: chad
2017-04-02 14:59:36 -07:00
epriestley
a9e2732a5c Spell "Elasticsearch" correctly, not "ElasticSearch"
Summary: Ref T12450. These are like 95% my fault, but Elastic appears to spell the name "Elasticsearch" consistently in their branding.

Test Plan: `grep ElasticSearch`

Reviewers: chad, 20after4

Maniphest Tasks: T12450

Differential Revision: https://secure.phabricator.com/D17601
2017-04-02 14:58:59 -07:00
epriestley
0f144d29e9 When "cluster.search" changes, don't trust the old index versions
Summary:
Ref T12450. We track a "document version" for updating search indexes, so that if a document is rapidly updated many times in a row we can skip most of the work.

However, this version doesn't consider "cluster.search" configuration, so if you add a new service (like a new ElasticSearch host) we still think that every document is up-to-date. When you run `bin/search index` to populate the index (without `--force`), we just do nothing.

This isn't necessarily very obvious. D17597 makes it more clear, by printing "everything was skipped and nothing happened" at the end.

Here, fix the issue by considering the content of "cluster.search" when computing fulltext document versions: if you change `cluster.search`, we throw away the version index and reindex everything.

This is slightly more work than we need to do, but changes to "cluster.search" are rare and this is much easier than trying to individually track which versions of which documents are in which services, which probably isn't very useful anyway.

Test Plan:
  - Ran `bin/search index --type project`, saw everything get skipped.
  - Changed `cluster.search`.
  - Ran `search index` again, saw everything get updated.
  - Ran a third time without changing `cluster.search`, everything was properly skipped.

Reviewers: chad, 20after4

Reviewed By: 20after4

Maniphest Tasks: T12450

Differential Revision: https://secure.phabricator.com/D17598
2017-04-02 13:45:48 -07:00
epriestley
bd93978200 Count and report skipped documents from "bin/search index"
Summary:
Ref T12450. There's currently a bad behavior where inserting a document into one search service marks it as up to date everywhere.

This isn't nearly as obvious as it should be because `bin/search index` doesn't make it terribly clear when a document was skipped because the index version was already up to date.

When running `bin/seach index` without `--force` or `--background`, keep track of updated vs not-updated documents and print out some guidance. In other configurations, try to provide more help too.

Test Plan: {F4452134}

Reviewers: chad, 20after4

Reviewed By: 20after4

Maniphest Tasks: T12450

Differential Revision: https://secure.phabricator.com/D17597
2017-04-02 13:45:30 -07:00
epriestley
6d81675032 Remove "url" from Elasticsearch index
Summary:
Ref T12450. This was added a very very long time ago (D2298).

I don't want to put this in the upstream index anymore because I don't want to encourage third parties to develop software which reads the index directly. Reading the index directly is a big skeleton key which bypasses policy checks.

This was added before much of the policy model existed, when that wasn't as much of a concern. On a tecnhnical note, this also doesn't update when `phabricator.base-uri` changes.

This can be written as a search index extension if an install relies on it for some bizarre reason, although none should and I'm unaware of any actual use cases in the wild for it, even at Facebook.

Test Plan: Indexed some random stuff into ElasticSearch.

Reviewers: chad, 20after4

Reviewed By: chad

Maniphest Tasks: T12450

Differential Revision: https://secure.phabricator.com/D17600
2017-04-02 13:26:45 -07:00
epriestley
64234535e3 Remove FIELD_KEYWORDS, index project slugs as body content
Summary:
D17384 added a "keywords" field but only partially implemented it.

  - Remove this field.
  - Index project slugs as part of the document body instead.

Test Plan:
  - Ran `bin/search index PHID-PROJ-... --force`.
  - Found project by searching for a unique slug.

Reviewers: chad, 20after4

Reviewed By: chad

Differential Revision: https://secure.phabricator.com/D17596
2017-04-02 09:36:32 -07:00
Chad Little
7ab4e7dbce Allow Owner Packages to be in a Dashboard Panel
Summary: Ref T12324. Add back Owners.

Test Plan: read carefully

Reviewers: epriestley, eadler

Reviewed By: eadler

Subscribers: Korvin

Maniphest Tasks: T12324

Differential Revision: https://secure.phabricator.com/D17588
2017-03-30 15:13:40 -07:00
Chad Little
eb6f4c4a28 Update PhortuneLanding page UI
Summary: Minor, uses 'user-circle' for account, and merchant logo for merchants in lists.

Test Plan: View the landing page, see updated logos and icons.

Reviewers: epriestley

Reviewed By: epriestley

Subscribers: Korvin

Differential Revision: https://secure.phabricator.com/D17586
2017-03-30 12:27:41 -07:00
Chad Little
86673486c0 Move Phortune Contollers into folders
Summary: Move individual controller files into cooresponding folders. Makes it easier to locate sections and expand without clutter. Also made "chargelist" part of account since it's tied to having an account specifically.

Test Plan: Vist charges, merchants, subscription, accounts, and other pages. No errors from file move.

Reviewers: epriestley

Reviewed By: epriestley

Subscribers: Korvin

Differential Revision: https://secure.phabricator.com/D17587
2017-03-30 12:26:15 -07:00
Mukunda Modell
cb1d904654 Make sure writes go to the right cluster
Summary:
Two little issues

1. there was an extra call to getHostForWrite,
2. The engine instance was shared between multiple service definitions so it
was overwriting the list of writable hosts from one service with hosts from another.

Test Plan:
tested in wikimedia production with multiple services defined like this:

```language=json
 [
        {
          "hosts": [
            {
              "host": "search.svc.codfw.wmnet",
              "protocol": "https",
              "roles": {
                "read": true,
                "write": true
              },
              "version": 5
            }
          ],
          "path": "/phabricator",
          "port": 9243,
          "type": "elasticsearch"
        },
        {
          "hosts": [
            {
              "host": "search.svc.eqiad.wmnet",
              "protocol": "https",
              "roles": {
                "read": true,
                "write": true
              },
              "version": 5
            }
          ],
          "path": "/phabricator",
          "port": 9243,
          "type": "elasticsearch"
        }
      ]
```

Reviewers: #blessed_reviewers, epriestley

Reviewed By: #blessed_reviewers, epriestley

Subscribers: epriestley

Differential Revision: https://secure.phabricator.com/D17581
2017-03-30 18:08:05 +00:00
Mukunda Modell
67a1c40476 Set content-type to application/json
Summary:
Elasticsearch really wants a raw json body and it fails to accept
the request as of es version 5.3

Test Plan:
Tested with elasticsearch 5.2 and 5.3.

Before this change 5.2 worked but 5.3 failed with
`HTTP/406 "Content-Type header [application/x-www-form-urlencoded] is not supported"` [1]

After this change, both worked.

[1] https://phabricator.wikimedia.org/P5158

Reviewers: epriestley, #blessed_reviewers

Reviewed By: epriestley, #blessed_reviewers

Subscribers: Korvin

Differential Revision: https://secure.phabricator.com/D17580
2017-03-30 18:07:47 +00:00
Mukunda Modell
654f0f6043 Make messages translatable and more sensible.
Summary:
These exception messages & comments didn't quite match reality.
Fixed and added pht() around a couple of them.

Test Plan: I didn't test this :P

Reviewers: epriestley, #blessed_reviewers

Reviewed By: epriestley, #blessed_reviewers

Subscribers: Korvin

Differential Revision: https://secure.phabricator.com/D17578
2017-03-28 23:17:35 +00:00
epriestley
5f939dcce0 Re-run config validation from bin/search
Summary:
Ref T12450. Normally, we validate config when:

  - You restart the webserver.
  - You edit it with `bin/config set ...`.
  - You edit it with the web UI.

However, you can also change config by editing `local.json`, `some_env.conf.php`, a `SiteConfig` class, etc. In these cases, you may miss config warnings.

Explicitly re-run search config checks from `bin/search`, similar to the additional database checks we run from `bin/storage`, to try to produce a better error message if the user has made a configuration error.

Test Plan:
```
$ ./bin/search init
Usage Exception: Setting "cluster.search" is misconfigured: Invalid search engine type: elastic. Valid types are: elasticsearch, mysql.
```

Reviewers: chad, 20after4

Reviewed By: 20after4

Maniphest Tasks: T12450

Differential Revision: https://secure.phabricator.com/D17574
2017-03-28 14:53:26 -07:00
epriestley
c22693ff29 Remove PhabricatorSearchEngineTestCase
Summary:
Ref T12450. This is now pointless and just asserts that `cluster.search` has a default value.

We might restore a fancier version of this eventually, but get rid of this for now.

Test Plan: Scruitinized the test case.

Reviewers: chad, 20after4

Reviewed By: 20after4

Maniphest Tasks: T12450

Differential Revision: https://secure.phabricator.com/D17573
2017-03-28 13:57:55 -07:00
epriestley
e7c76d92d5 Make bin/search init messaging a little more consistent
Summary:
Ref T12450. This mostly just smooths out the text a little to improve consistency. Also:

  - Use `isWritable()`.
  - Make the "skipping because not writable" message more clear and tailored.
  - Try not to use the word "index" too much to avoid confusion with `bin/search index` -- instead, talk about "initialize a service".

Test Plan: Ran `bin/search init` with a couple of different (writable / not writable) configs, saw slightly clearer messaging.

Reviewers: chad, 20after4

Reviewed By: 20after4

Maniphest Tasks: T12450

Differential Revision: https://secure.phabricator.com/D17572
2017-03-28 13:57:37 -07:00
Mukunda Modell
699228c73b Address some New Search Configuration Errata
Summary:
  [ ] Write an "Upgrading: ..." guidance task with narrow instructions for installs that are upgrading.
  [ ] Do we need to add an indexing activity (T11932) for installs with ElasticSearch?
  [ ] We should more clearly detail exactly which versions of ElasticSearch are supported (for example, is ElasticSearch <2 no longer supported)? From T9893 it seems like we may //only// have supported ElasticSearch <2 before, so are the two regions of support totally nonoverlapping and all ElasticSearch users will need to upgrade?
  [ ] Documentation should provide stronger guidance toward MySQL and away from Elastic for the vast majority of installs, because we've historically seen users choosing Elastic when they aren't actually trying to solve any specific problem.
  [ ] When users search for fulltext results in Maniphest and hit too many documents, the current behavior is approximately silent failure (see T12443). D17384 has also lowered the ceiling for ElasticSearch, although previous changes lowered it for MySQL search. We should not fail silently, and ideally should build toward T12003.
  [ ] D17384 added a new "keywords" field, but MySQL does not search it (I think?). The behavior should be as consistent across MySQL and Elastic as we can make it. Likely cleaner is giving "Project" objects a body, with "slugs" and "description" separated by newlines?
  [ ] `PhabricatorSearchEngineTestCase` is now pointless and only detects local misconfigurations.
  [ ] It would be nice to build a practical test suite instead, where we put specific documents into the index and then search for them. The upstream test could run against MySQL, and some `bin/search test` could run against a configured engine like ElasticSearch. This would make it easier to make sure that behavior was as uniform as possible across engine implementations.
  [ ] Does every assigned task now match "user" in ElasticSearch?
  [x] `PhabricatorElasticFulltextStorageEngine` has a `json_encode()` which should be `phutil_json_encode()`.
  [ ] `PhabricatorSearchService` throws an untranslated exception.
  [ ] When a search cluster is down, we probably don't degrade with much grace (unhandled exception)?
  [ ] I haven't run bin/search init, but bin/search index doesn't warn me that I may want to. This might be worth adding. The UI does warn me.
  [ ] bin/search init warns me that the index is "incorrect". It might be more clear to distinguish between "missing" and "incorrect", since it's more comforting to users to see "everything is as we expect, doing normal first-time setup now" than "something is wrong, fixing it".
  [ ] CLI message "Initializing search service "ElasticSearch"" does not end with a period, which is inconsistent with other UI messages.
  [ ] It might be nice to let bin/search commands like init and index select a specific service (or even service + host) to act on, as bin/storage --ref ... now does. You can generally get the result you want by fiddling with config.
  [ ] When a service isn't writable, bin/search init reports "Search cluster has no hosts for role "write".". This is accurate but does not provide guidance: it might be more useful to the user to explain "This service is not writable, so we're skipping index check for it.".
  [x] Even with write off for MySQL, bin/search index --type task --trace still updates MySQL, I think? I may be misreading the trace output. But this behavior doesn't make sense if it is the actual behavior, and it seems like reindexAbstractDocument() uses "all services", not "writable services", and the MySQL engine doesn't make sure it's writable before indexing.
  [x] Searching or user fails to find task Grant users tokens when a mention is created, suggesting that stemming is not working.
  [x] Searching for users finds that task, but fails to find a task containing "per user per month" in a comment, also suggesting that stemming is not working.
  [x] Searching for maniphest fails to find task maniphest.query elephant, suggesting that tokenization in ElasticSearch is not as good as the MySQL tokenization for these words (see D17330).
  [x] The "index incorrect" warning UI uses inconsistent title case.
  [x] The "index incorrect" warning UI could format the command to be run more cleanly (with addCommand(), I think).

refs T12450

Test Plan:
* Stared blankly at the code.
* Disabled 'write' role on mysql fulltext service.
* Edited a task, ran search indexer, verified that the mysql index wasn't being updated.

Reviewers: epriestley, #blessed_reviewers

Reviewed By: epriestley, #blessed_reviewers

Subscribers: Korvin

Maniphest Tasks: T12450

Differential Revision: https://secure.phabricator.com/D17564
2017-03-28 20:19:38 +00:00
epriestley
2fbc9a52da Allow users to "Force accept" package reviews if they own a more general package
Summary:
Ref T12272. If you own a package which owns "/", this allows you to force-accept package reviews for packages which own sub-paths, like "/src/adventure/".

The default UI looks something like this:

```
[X] Accept as epriestley
[X] Accept as Root Package
[ ] Force accept as Adventure Package
```

By default, force-accepts are not selected.

(I may do some UI cleanup and/or annotate "because you own X" in the future and/or mark these accepts specially in some way, particularly if this proves confusing along whatever dimension.)

Test Plan: {F4314747}

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T12272

Differential Revision: https://secure.phabricator.com/D17569
2017-03-28 11:51:40 -07:00
epriestley
ddc02ce420 When voiding "Accept" reviews, also void "Reject" reviews
Summary: Ref T10967. This change is similar to D17566, but for rejects.

Test Plan:
  - Create a revision as A, with reviewer B.
  - Reject as B.
  - Request review as A.
  - Before patch: stuck in "rejected".
  - After patch: transitions back to "needs review".

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T10967

Differential Revision: https://secure.phabricator.com/D17568
2017-03-28 11:51:06 -07:00