Summary: Ref T5791. This edge table grows 2+X faster than the corresponding mail table depending on usage. Ergo, lets make sure to clean that up too in the delete code.
Test Plan: careful thought
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: epriestley, Korvin
Maniphest Tasks: T5791
Differential Revision: https://secure.phabricator.com/D13408
Summary:
Ref T8493. Diffusion is probably the strongest upstream use case we have for Spaces right now, so I want to get us on it to kick the tires a bit.
Small amount of hackiness around the multi-page form thing but it shouldn't create any problems.
Test Plan:
- Created a new repo.
- Edited a repo.
- Tried invalid edits, saw value preserved.
- Viewed edit full detail screen, saw space info.
- Viewed repo detail view, saw space.
- Viewed repo list view, saw space.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T8493
Differential Revision: https://secure.phabricator.com/D13414
Summary: Ref T5791. Makes the result page a little less ugly and adds the time created. Makes the detail page include more useful information about the message.
Test Plan: viewed results page and detail page and they looked better
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: epriestley, Korvin
Maniphest Tasks: T5791
Differential Revision: https://secure.phabricator.com/D13410
Summary:
Ref T8637. This does nothing interesting, just has empty scaffolding for a bulk job queue.
Basic idea is that when you do something like a batch edit in Maniphest, we:
- Create a BulkJob with all the details.
- Queue a worker to start the job.
- Send you to a progress bar page for the job.
In the background:
- The "start job" worker creates a ton of Task objects, then queues worker tasks to do the work.
In the foreground:
- Fancy ajax animates the progress bar and it goes wooosh.
In general:
- Big jobs actually work.
- Jobs get logged.
- You can monitor jobs.
- Terrible junk like T8637 should be much harder to write and much easier to catch and diagnose.
Test Plan:
No interesting code/beahavior yet. Clean `storage adjust`.
{F526411}
Reviewers: chad, btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T8637
Differential Revision: https://secure.phabricator.com/D13392
Summary:
Ref T8096. Various tweaks here:
- Sort result lists by importance (even lint -- "errors first" seems better than "alphabetical by file", I think?).
- Do sane stuff with display limits.
- Add a "view all" view.
- Don't show a huge table of passing tests in Differential.
- Link to full results.
Test Plan: See screenshots.
Reviewers: chad, btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T8096
Differential Revision: https://secure.phabricator.com/D13407
Summary: Ref T8096. We don't currently link to the buildable, which I think contributes to Harbormaster feeling a little scattered.
Test Plan: {F528095}
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T8096
Differential Revision: https://secure.phabricator.com/D13405
Summary: Ref T8096. No functional changes, just a bit less code.
Test Plan: Viewed some revisions, saw the same stuff as before.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T8096
Differential Revision: https://secure.phabricator.com/D13404
Summary:
Ref T5791. This diff adds a "sensitive" flag to `PhabricatorMetaMTAMail`, defaults it to true in the constructor, and then sets it to false in teh application transaction editor. Assumption here is that sensitive emails are basically all the emails that don't flow through the application transaction editor.
This diff also gets a basic "mail view" page up and going.
This diff also fixes a bug writing recipient edges; the actor was being included.
This bug also fixes a querying bug; we shouldn't do the automagic join of $viewer is recipient or $viewer is actor if folks are querying for recipients or actors already. The bug manifested itself as having the "inbox" be inbox + outbox.
Test Plan: viewd list of messages. viewed message detail.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: epriestley, Korvin
Maniphest Tasks: T5791
Differential Revision: https://secure.phabricator.com/D13406
Summary:
Ref T5791. This diff does a few things...
- Adds code to write recipients to edges on save
- Makes Query performance for policy filtering okay-ish
- Adds a Search Engine for PhabricatorMetaMTAMail
- Adds "working" List Controller
- Inbox and Outbox both work
- Adds stub View Controller
Test Plan: ran `./bin/storage upgrade` and saw my inbox and outbox start getting data. played with application and saw new entries in inbox and outbox
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: epriestley, Korvin
Maniphest Tasks: T5791
Differential Revision: https://secure.phabricator.com/D13397
Summary: Fixes T8095. Still needs UI/UX work (see T8096) but this has all the core features now.
Test Plan: Saw Harbormaster lint/unit data as though it was Differential lint-unit data.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T8095
Differential Revision: https://secure.phabricator.com/D13401
Summary: Ref T8096. Show basic lint/unit info on builds. This is still pretty rough.
Test Plan: {F524839}
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T8096
Differential Revision: https://secure.phabricator.com/D13383
Summary:
Ref T8096. Fixes a few bugs and glitches.
- Set build completion time when handling a message.
- Format duration information in a more human-readable way.
- Use a table for build variables.
- Fix up container PHIDs on diffs (a touch hacky, should be OK for now though).
Test Plan: Browsed around the UI.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T8096
Differential Revision: https://secure.phabricator.com/D13382
Summary:
Ref T8095. When build results are reported for a target, allow them to include unit and lint results.
There is no real way to see this stuff in the UI yet, either in Harbormaster or Differential.
Test Plan: Manually called this method with some results, saw Harbormaster update appropriately.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T8095
Differential Revision: https://secure.phabricator.com/D13380
Summary: Ref T8095. This weird grey table has no remaining callsites and can be removed.
Test Plan: Grepped for symbols.
Reviewers: chad, btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T8095
Differential Revision: https://secure.phabricator.com/D13379
Summary:
Ref T8095. Same as D13377, but for unit results.
This is a bit rough and there's some duplication between this and unit results. I'll likely merge them later, but I think some of it is superficial since these iterations are still a little crude.
Test Plan: {F523499}
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T8095
Differential Revision: https://secure.phabricator.com/D13378
Summary:
Ref T8095. Render lint results in a future-ready way.
This makes the renderer accept `HarbormasterBuildLintMessage` objects. If we have legacy data instead, it converts it into `HarbormasterBuildLintMessage` objects.
Design is a bit rough but will be cleaned up later after T7739.
This moves away from "postponed linters", which are obsolete after Harbormaster (and were only ever used by Facebook).
Test Plan: {F523429}
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T8095
Differential Revision: https://secure.phabricator.com/D13377
Summary: closes T8486
Test Plan: Create a pholio mock. In an already existing object mention the pholio mock. Verify that the reference to the object exists in the pholio mock's timeline.
Reviewers: lpriestley, #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: epriestley, Korvin
Maniphest Tasks: T8486
Differential Revision: https://secure.phabricator.com/D13402
Summary: Ref T8099, This adds a consistent background color to object and policy tags, and highlights them when they deviate from the normal. Still likely worth revamping 'closed' and 'review' state colors.
Test Plan: Review lots of diffs and tasks.
Reviewers: btrahan, epriestley
Reviewed By: epriestley
Subscribers: epriestley, Korvin
Maniphest Tasks: T8099
Differential Revision: https://secure.phabricator.com/D13399
Summary: Closes T8639, Formatting event dates in list view
Test Plan: List view should show dates in wide and narrow lists.
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: epriestley, Korvin
Maniphest Tasks: T8639
Differential Revision: https://secure.phabricator.com/D13398
Summary: Ref T5791. This should make performance snappy wrt policy checks in some future diff where the Query is updated and in use somewhere in the application.
Test Plan: ran `./bin/storage upgrade`. commented on a task and saw actorPHID populated correctly in underlying MetaMTAMail object database entry
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: epriestley, Korvin
Maniphest Tasks: T5791
Differential Revision: https://secure.phabricator.com/D13396
Summary: Fixes T8642, This is a table, but not returned as one. Set it to Content for now with a Collapsed layout.
Test Plan: Test /people/logs/
Reviewers: btrahan, epriestley
Reviewed By: epriestley
Subscribers: epriestley, Korvin
Maniphest Tasks: T8642
Differential Revision: https://secure.phabricator.com/D13395
Summary:
Ref T5791. This does a few bits there. Namely:
- Adds PHID column to PhabricatorMetaMTAMail
- Implements a PhabricatorMetaMTAMailPHIDType
- Script to backpopulate them.
- Makes PhabricatorMetaMTAMail implement PolicyInterface.
- View policy is NOONE and the author and recipients have automatic view capabilities
- No edit capability.
- Adds a PhabricatorMetaMTAMailQuery for PhabricatorMetaMTAMail.
Test Plan: ran `./bin/storage upgrade` successfully. commented on a maniphest task and verifed the metamta mail object in the database was created successfully with a shiny new phid
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: epriestley, Korvin
Maniphest Tasks: T5791
Differential Revision: https://secure.phabricator.com/D13394
Summary: Ref T7950, Refactor Calendar Search, and implement Projects on events
Test Plan: Verify that all queries in Calendar search still work, and that events can now have associated Projects that you can search by in Calendar Search.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: epriestley, Korvin
Maniphest Tasks: T7950
Differential Revision: https://secure.phabricator.com/D13393
Summary:
Fixes T4139. Adds a "Desktop Notifications" panel to settings. For now, we start with "Send Desktop Notifications Too" functionality. We can try to be fancy later and only send desktop notifications if the web app doesn't have focus, etc.
Test Plan:
Made some comments as a test user on a task and got purdy desktop notifications using Chrome. Then did it again with Firefox.
Played around with permissions form with Chrome and got helpful information about what was up. Played around with Firefox and got similar results, except canceling the dialogue didn't invoke my handler code somehow. Oh Firefox!
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: rbalik, tycho.tatitscheff, joshuaspence, epriestley, Korvin
Maniphest Tasks: T4139
Differential Revision: https://secure.phabricator.com/D13219
Summary: Ref T8637. This class has some really old parameter handling which can send `withIDs(array())` down a "fetch everything" pathway. Clean up most of it.
Test Plan: Issued every ApplicationSearch query.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T8637
Differential Revision: https://secure.phabricator.com/D13390
Summary:
Ref T8637. If a user tries to batch edit a list of tasks which can't be edited, we fall through to `withIDs(array())`, which can affect //everything//.
Explicitly stop batch editing if we don't have valid IDs or valid tasks.
The UI sort-of warns you that something is wrong, but this is ultimately a pretty severe UX issue. I'll fix the underlying Query in the next diff.
Test Plan: Tried to batch edit a list of tasks I didn't have permission to edit.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: lloyd.oliver, epriestley
Maniphest Tasks: T8637
Differential Revision: https://secure.phabricator.com/D13388
Summary:
Fixes T6787. I'm kind of cheating a little bit here by not unifying default selection with `initializeNew(...)` methods, but I figure we can let this settle for a bit and then go do that later. It's pretty minor.
Since we're not doing templates I kind of want to swap the `'template'` key to `'type'` so maybe I'll do that too at some point.
@chad, freel free to change these, I was just trying to make them pretty obvious. I //do// think it's good for them to stand out, but my approach is probably a bit inconsistent/heavy-handed in the new design.
Test Plan:
{F525024}
{F525025}
{F525026}
{F525027}
Reviewers: btrahan, chad
Reviewed By: btrahan
Subscribers: johnny-bit, joshuaspence, chad, epriestley
Maniphest Tasks: T6787
Differential Revision: https://secure.phabricator.com/D13387
Summary:
Fixes T5135. Currently, when you create a credential, we default the policies to your PHID. This means we can't have an application-level configurable default because there's no way to select "the actor's PHID" as a policy.
Start tracking the credential author's PHID and add an object policy for it, so there is such a setting.
Then, add policy defaults.
This mostly unblocks T6787. This obsoletes T6860.
Test Plan:
- Created a credential with "Credential Author" policy.
- Verified I can see/edit it, but other users can not.
- Changed default policies to something else.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T5135
Differential Revision: https://secure.phabricator.com/D13385
Summary: Prepares for bringing spaces and a new object policy here.
Test Plan: Used all search controls dozens of times.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Differential Revision: https://secure.phabricator.com/D13384
Summary:
Ref T8095. Two general problems:
- I want Harbormaster to own all lint and unit test results.
- I don't want users to have to configure anything for `arc` to keep working automatically.
These are in conflict because generic lint/unit test ownership in Harbormaster requires that build targets exist which we can attach build results to. However, we can't currently create build targets on demand: Harbormaster assumes it is responsible for creating targets, then running code or making third-party service calls to actually run the builds.
I considered two broad approaches to let `arc` push results into Harbormaster without requiring administrators to configure some kind of "arc results" build plan:
# Add magic target PHIDs like `PHID-MAGIC-this-is-really-arc-unit`.
# Add new code to build real targets with real PHIDs.
(1) is probably a bit less work to get off the ground, but I think it's worse overall and very likely to create more problems in the long run. I particularly worry that it will lead to a small amount of special casing in a very large number of places, which seems more fragile.
(2) is more work upfront but I think does a better job of putting all the special casing in one place that we can, e.g., more reasonably unit test, and letting the rest of the code rarely/never care about this case since it's just dealing with normal plans/steps/targets as far as it can tell.
This diff introduces "autoplans", which are source templates for plans/steps. This let us "push" these targets into Harbormaster. Hypthetically, any process "like" arc can use autoplans to upload test/lint/etc results. In practice, probably only `arc` will ever use this, but I think it's still quite a bit cleaner than the alternative despite all the generality.
Workflow is basically:
- `arc` creates a diff.
- `arc` calls `harbormaster.queryautotargets`, passing the diff PHID and saying "I have some lint and unit results I want to stick on this thing".
- Harbormaster builds the plan, steps, and targets (if any of them don't already exist), and hands back the target PHIDs so `arc` has a completely standard-looking place to put results.
- `arc` uploads the test results to the right targets, as though Harbormaster had asked it to run unit/lint in the first place.
(This doesn't actually do any of that yet, just sets things up.)
I'll maybe doc turn that ^^^^^^ into a doc for posterity since I think it's hard to guess what an "autotarget" is, but I'm going to grab some lunch first.
Test Plan:
- Added unit tests to make sure we can build these things properly.
- Used `harbormaster.queryautotargets` to build autotargets for a bunch of diffs.
- Verified targets come up in "waiting for message" state.
- Verified plans and steps are not editable.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: hach-que, epriestley
Maniphest Tasks: T8095
Differential Revision: https://secure.phabricator.com/D13345
Summary:
Ref T8095. This is just general groundwork for more exciting changes:
- Use more modern conventions around controllers, UI elements, and dialogs.
- Provide real CAN_EDIT policies and policy checks (they just don't do anything yet).
Test Plan:
- Used all affected controllers.
- Faked CAN_EDIT to POLICY_NOONE and verified everything was greyed out and unselectable.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T8095
Differential Revision: https://secure.phabricator.com/D13344
Summary:
Ref T8095.
Harbormaster has a `BuildItem` class, but it has no table and is unused. This was an earlier idea about representing lint/unit results and some other possible types of messages, but I think we want to be more specific than this.
Remove `BuildItem` and add `Lint` and `Unit` storage. These tables roughly parallel how we store lint/unit messages today, with some guesses about how where they'll go in the future.
Test Plan: Ran `bin/storage upgrade` and got a clean adjust out of it.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: hach-que, epriestley
Maniphest Tasks: T8095
Differential Revision: https://secure.phabricator.com/D13329
Summary: Ref T8099, Now that we have cleaner headers, we can add more pop to important items, like the state of an Object. This makes it easier to just note the color and generally understand the state of the object (closed, returned, accepted, open). @epriestley, I think you previously thought this was a bug, but if it still feels bad, let me know.
Test Plan:
Review Differential, Maniphest, Polls for various object states.
{F520973}
Reviewers: btrahan, epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T8099
Differential Revision: https://secure.phabricator.com/D13369
Summary: Ref T8099, Adds more infomation to the icon in Maniphest Task Lists.
Test Plan: Hover over icons in a Maniphest Task
Reviewers: btrahan, epriestley
Reviewed By: epriestley
Subscribers: epriestley, Korvin
Maniphest Tasks: T8099
Differential Revision: https://secure.phabricator.com/D13367
Summary: Fixes T8626. I guess that `$this->getViewer()->renderHandle(null)` renders as "Unknown Object", which is somewhat odd to me.
Test Plan: Viewed an atom from a book without a repository linked to it.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: epriestley, Korvin
Maniphest Tasks: T8626
Differential Revision: https://secure.phabricator.com/D13370
Summary: Not sure if we want this, but it seems to work fine.
Test Plan: {F516736}
Reviewers: joshuaspence, chad
Reviewed By: joshuaspence, chad
Subscribers: joshuaspence, epriestley
Differential Revision: https://secure.phabricator.com/D13363
Summary: Ref T8617. Provide general documentation with tools for debugging hangs and slow pages. Update DarkConsole docs and discuss how to use Services and XHProf. Explain what Multimeter is for and how to use it. Update XHProf docs and provide some usage hints.
Test Plan: Read documentation.
Reviewers: joshuaspence, btrahan
Reviewed By: joshuaspence, btrahan
Subscribers: joshuaspence, epriestley
Maniphest Tasks: T8617
Differential Revision: https://secure.phabricator.com/D13359
Summary: Fixes T6839. Sometimes, worker tasks go astray for whatever reason. This automates the step of `bin/repository importing | xargs | mangle mangle | bin/repostiory reparse`.
Test Plan: Ran various flavors of the command, got good looking results.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T6839
Differential Revision: https://secure.phabricator.com/D13362
Summary: Ref T8099, uses original bgcolor when dark headers are used.
Test Plan: Switch between light and dark Phabricator, see new colors.
Reviewers: btrahan, epriestley
Reviewed By: epriestley
Subscribers: epriestley, Korvin
Maniphest Tasks: T8099
Differential Revision: https://secure.phabricator.com/D13361
Summary: Ref T8096. This shows the build plan name on the Harbormaster build plan view controller. Without this, the name is not displayed anywhere on the page when you're viewing a build plan's configuration (which makes things confusing if you're updating a bunch of build plans at once).
Test Plan: Viewed a build plan, saw the build plan name on the page.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: epriestley, Korvin
Projects: #harbormaster
Maniphest Tasks: T8096
Differential Revision: https://secure.phabricator.com/D13356
Summary: Ref T8099, Cleans up profile photos around the site in a more consistent way.
Test Plan: grep for 'background-size: 100%'
Reviewers: btrahan, epriestley
Reviewed By: epriestley
Subscribers: epriestley, Korvin
Maniphest Tasks: T8099
Differential Revision: https://secure.phabricator.com/D13360
Ref T8618. I think this was just typoed in D13070.
Immediate issue is that we're looking for symbols with the book name as a "phid".
Auditors: joshuaspence
Summary: Closes T8032
Test Plan: Verify that when editing a calendar event's description, there is a UI bar helping with adding markup to the description. Also verify that markup is displayed correctly on the event page once the event has been updated.
Reviewers: lpriestley, #blessed_reviewers, epriestley
Reviewed By: lpriestley, #blessed_reviewers, epriestley
Subscribers: epriestley, Korvin
Maniphest Tasks: T8032
Differential Revision: https://secure.phabricator.com/D13355
Summary:
Minor layout updates to Ref T8099
- Timeline tweaks
- Use Lato headers in Document Obj Headers
- Minor Remarkup
- Add Audit Icons
(Unclear if Audit is "correct", ie Status vs. Commit Status) But added icons anyways if needed.
Test Plan: Review each of the updated elements
Reviewers: btrahan, epriestley
Reviewed By: epriestley
Subscribers: epriestley, Korvin
Maniphest Tasks: T8099
Differential Revision: https://secure.phabricator.com/D13357
Summary: Ref T8099. This adds a new class which all search engines return for layout. I thought about this a number of ways, and I think this is the cleanest path. Each Engine can return whatever UI bits they needs, and AppSearch or Dashboard picks and lays the bits out as needed. In the AppSearch case, interfaces like Notifications, Calendar, Legalpad all need more custom layouts. I think this also leaves a resonable path forward for NUX as well. Also, not sure I implemented the class correctly, but assume thats easy to fix?
Test Plan: Review and do a search in each application changed. Grep for all call sites.
Reviewers: btrahan, epriestley
Reviewed By: epriestley
Subscribers: epriestley, Korvin
Maniphest Tasks: T8099
Differential Revision: https://secure.phabricator.com/D13332
Summary: Fixes T8352. Associate Diviner books and atoms with a repository. This relationship is not really surfaced anywhere in the UI but provides metadata that contextualises search results. Depends on D13091.
Test Plan: Ran `diviner generate --repository ARC` and then went to `/diviner/book/arcanist/`.
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T7703, T8352
Differential Revision: https://secure.phabricator.com/D13070
Summary: This drops the Windows-specific escaping code for the creation of directories when acquiring a lease. This is basically the change from D10378 without the other, no longer necessary changes.
Test Plan: This code hasn't been run in a production environment for a while (any instances of Phabricator using Drydock / Harbormaster with Windows have had this code removed by the D10378 patch for a while).
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: epriestley, Korvin
Projects: #drydock
Differential Revision: https://secure.phabricator.com/D13341
Summary: Ref T2015. This code is only relevant when attempting to run commands on a Windows host over SSH. Since SSH on Windows is extremely fragile and hard to maintain, and WinRM is a better long-term solution, drop this code (which will end up being unused when later diffs introduce the WinRM command interface).
Test Plan: This code won't be used when D10495 lands.
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: epriestley, Korvin
Projects: #drydock
Maniphest Tasks: T2015
Differential Revision: https://secure.phabricator.com/D13340
Summary: Ref T8320. Fixes T8427. This is still a little funky because Owners has weird name rules, but should fix the bugs (unselectable packages) in T8427.
Test Plan: Browsed Owners typaheads, used various search functions.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T8320, T8427
Differential Revision: https://secure.phabricator.com/D13349
Summary:
Fixes T8601. To reproduce the problem:
- Set your time preference to `""` (the empty string). This isn't possible from the modern UI, but can be done with "Right Click > Inspect Element", or users may have carried it forward from an older setting (this is the case with me and @hach-que on this install).
- Load Calendar with some events.
- This parses an epoch, which sets `valueTime` to `""` (since there are no format characters in the preference) and then `getEpoch()` fails because `strlen($time)` is 0.
- Since `getEpoch()` failed, `getDateTime()` also fails.
To fix this:
- Only permit the date and time preferences to have valid values.
Test Plan:
- Loaded page before patch, saw fatal.
- Applied patch.
- No more fatal.
- Viewed tooltips, dates/times, dates/times in other apps.
- Changed my preferences, saw them respected.
Reviewers: lpriestley
Reviewed By: lpriestley
Subscribers: epriestley, hach-que
Maniphest Tasks: T8601
Differential Revision: https://secure.phabricator.com/D13346
Summary:
Fixes T8597. Second issue there is that if you look at a huge file in Diffusion (like `/path/to/300MB.pdf`) we pull the whole thing over Conduit upfront, then try to shove it into file storage.
Instead, pull no more than the chunk limit (normally 4MB) and don't spend more than 10s pulling data.
If we get 4MB of data and/or time out, just fail with a message in the vein of "this is a really big file".
Eventually, we could improve this:
- We can determine the //size// of very large files correctly in at least some VCSes, this just takes a little more work. This would let us show the true filesize, at least.
- We could eventually stream the data out of the VCS, but we can't stream data over Conduit right now and this is a lot of work.
This is just "stop crashing".
Test Plan: Changed limits to 0.01 seconds and 8 bytes and saw reasonable errors. Changed them back and got normal beahvior.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T8597
Differential Revision: https://secure.phabricator.com/D13348
Summary: Fixes T8603. For automatic 'order' parameters provided by infrastructure en route to T7715, clarify that they are optional (we will use the default builtin order for the underlying Query if an order is not provided).
Test Plan: Used web UI to see "optional" hint.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T8603
Differential Revision: https://secure.phabricator.com/D13342
Summary:
Ref T8597. If a page issues a large query (like inserting a blob into file storage), we may try to utf8ize the entire thing. This is slow and pointless.
Instead, truncate tab data after 4096 bytes before sanitizing.
Test Plan: Adjusted limit to 256 bytes, saw long queries get truncated reasonably.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T8597
Differential Revision: https://secure.phabricator.com/D13347
Summary: Closes T8050, Format Calendar list objects
Test Plan: Open Calendar list, check that new formatting is true to mocks.
Reviewers: chad, #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: epriestley, Korvin
Maniphest Tasks: T8050
Differential Revision: https://secure.phabricator.com/D13318
Summary: Ref T8577, More correct fix - set default value array() for `getParameter` call.
Test Plan: Make sure saved queries like Month View, Day View, and Upcoming Events, still work.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: epriestley, Korvin
Maniphest Tasks: T8577
Differential Revision: https://secure.phabricator.com/D13330
Summary: Fixes T8589. Adds a bunch of new languages to the syntax highlighting config options so that they are supported by #paste.
Test Plan: Saw new filetypes in Paste.
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: epriestley, Korvin
Maniphest Tasks: T8589
Differential Revision: https://secure.phabricator.com/D13337
Summary:
Ref T8449. Before we show a policy hint in the header of an object, compare it to the space policy (if one exists).
If the space policy is strictly stronger (more restrictive -- for example, the Space policy is 'members of X', and the object policy is 'public'), show the space policy instead.
See discussion on T8376.
Test Plan: {F509126}
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T8449
Differential Revision: https://secure.phabricator.com/D13328
Summary:
Ref T8575. We run a big "(A) UNION (B)" query on the home page and on the main Differential page.
"A" can always be improved by using `%Ls`, so it can use the second half of the `(authorPHID, status)` key.
"B" can sometimes be improved if the fraction of open revisions is smaller than the fraction of revisions you are reviewing. This is true for me on secure.phabricator.com (I'm a reviewer, either directly or via 'Blessed Reviewers', on about 80% of revisions, but <5% are open). In these cases, a `(status, phid)` key is more efficient.
Test Plan: Tweaked queries and added keys on this server, saw less derpy query plans and performance.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T8575
Differential Revision: https://secure.phabricator.com/D13325
Summary:
Ref T8575. Although we cache spaces as a whole, we don't cache viewer spaces. This can still do a lot of work if they have complex policies.
Instead, cache them in the request cache.
Test Plan: Saw this account for 37% of a page in produciton (303ms on on 835ms).
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T8575
Differential Revision: https://secure.phabricator.com/D13324
Summary: These class properties don't seem to be used.
Test Plan: Eyeball it.
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: epriestley, Korvin
Differential Revision: https://secure.phabricator.com/D13295
Summary: Ref T8575. We check if users can see applications frequently, and caching on the Query isn't especially effective. Use the new Request cache instead.
Test Plan:
- Saw `/feed/` drop 7% (from ~830ms to ~770ms) on profiles.
Reviewers: btrahan, avivey
Reviewed By: avivey
Subscribers: avivey, epriestley
Maniphest Tasks: T8575
Differential Revision: https://secure.phabricator.com/D13321
Summary:
Ref T8574. Currently, failures during mail body construction, feed publishing, or search indexing could cause us to retry the publishing task and potentially send duplicate mail.
Instead, build (but do not send) the mail first, then send all the mail at the very end.
This isn't completley perfect, but should make it enormously harder for duplicate mail to be generated.
Test Plan: Sent some mail, ran the daemons, saw it show up normally in the outbound queue.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T8574
Differential Revision: https://secure.phabricator.com/D13320
Summary:
Ref T8574. This could fail because the target is disabled (as here), or doesn't have access to the API, or the API request needs to be satisfied by a different host which isn't available.
In any of these cases, just show the failure and continue generating the mail. This field isn't important enough to block the mail, and many of these errors are permanent.
(I'll follow up on T8574 with some more permanent ideas to address this class of issue.)
Test Plan: Faked API call failure, generated mail, saw clean generation of mail with a failure message.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T8574
Differential Revision: https://secure.phabricator.com/D13319
Summary: Encountered this playing with T8402 on my test instance. I think warning the user about adding a trailing "/" is unnecessary so don't do it. I think its confusing to not call out spaces / to lump them in with special characters so call out the sapces.
Test Plan: moved a page around and verified no warning if the slug is missing a "/" as user specified and that a change to spaces is called out
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: epriestley, Korvin
Differential Revision: https://secure.phabricator.com/D13316
Summary: Fixes T8346. Also gets rid of the air quotes on Auth and uses the "Appname Application" style we use elsewhere.
Test Plan: slapped an "|| true" in the checking logic and verified the link was rendered in a usable fashion just below the error message.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: epriestley, Korvin
Maniphest Tasks: T8346
Differential Revision: https://secure.phabricator.com/D13315
Summary: Fixes T8518. Defaulting $events to null creates carnage further down the stack. Instead, default it to array() so foreach, array_select_keys, etc. don't barf.
Test Plan: logic and will ask the reporting user to verify since they can repro 100%
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: epriestley, Korvin
Maniphest Tasks: T8518
Differential Revision: https://secure.phabricator.com/D13314
Summary: Ref T4558. If a Diviner atom is a ghost (i.e. the underlying source code has been removed), mark it as closed in the search index.
Test Plan: Searched for a ghost atom in global searcn and saw the results show "Closed".
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: epriestley, Korvin
Maniphest Tasks: T4558
Differential Revision: https://secure.phabricator.com/D13297
Summary: Ref T4558. Give `DivinerAtomPHIDType` and `DivinerBookPHIDType` a font icon.
Test Plan: Saw icons in global search.
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: epriestley, Korvin
Maniphest Tasks: T4558
Differential Revision: https://secure.phabricator.com/D13304
Summary: Fixes T8524, T8550. These were both missing "subscribers" and Phriction was also the only application with no "other".
Test Plan:
- Project
- set user A to notify only for project subscriber changes
- made a project with user A
- subscribed user B to project
- verified notification sent to user A
- used ./bin/mail to make sure no mail was sent to user A because "This mail has tags which control which users receive it, and this recipient has not elected to receive mail with any of the tags on this message (Settings > Email Preferences)."
- Phriction
- set user A to notify only for phriction document subscriber changes
- made a document with user A
- subscribed user B to document
- verified notification sent to user A
- used ./bin/mail to make sure no mail was sent to user A because "This mail has tags which control which users receive it, and this recipient has not elected to receive mail with any of the tags on this message (Settings > Email Preferences)."
- observed option for "other" in email preferences
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: epriestley, Korvin
Maniphest Tasks: T8524, T8550
Differential Revision: https://secure.phabricator.com/D13313
Summary:
Ref T4558. This diff modernizes the #diviner application. Basically:
- Add an edit controller, accessible at `/book/$BOOK/edit/`.
- Add edit/view policies.
- Added an action menu to the `DivinerBookController` to expose the edit interface.
- Allows projects to be associated with books.
- Implement edges and transactions.
- Implemented `PhabricatorApplicationTransactionInterface` in `DivinerLiveBook`.
Test Plan:
- Generated a Diviner book with `./bin/diviner generate`.
- Added projects to a book and ensured that they persisted.
- Changed the view policy on a book and made sure it was effective.
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T4558
Differential Revision: https://secure.phabricator.com/D13091
Summary: Closes T8481.
Test Plan: Verify that in Passphrase > Create an option to create a Note credential exists and credentials of type Note are createable.
Reviewers: epriestley, #blessed_reviewers, eadler, lpriestley
Reviewed By: eadler
Subscribers: joshuaspence, epriestley, Korvin
Maniphest Tasks: T8481
Differential Revision: https://secure.phabricator.com/D13261
Summary: Closes T8460.
Test Plan: In the Calendar application, create a recursive event. After editing a recursion of the event, verify that the "Recurrence of Event" property is of the form "<event sequence number> of <parent event>".
Reviewers: lpriestley, epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: epriestley, Korvin
Maniphest Tasks: T8460
Differential Revision: https://secure.phabricator.com/D13307
Summary: Closes T8046, Convert "Created By" and "Invited" inputs in Calendar search to use `PhabricatorPeopleUserFunctionDatasource`
Test Plan: {nav Calendar > Advanced Search}, search for "members:Calendar", created by and invited. Should autofill and search correctly.
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: epriestley, Korvin
Maniphest Tasks: T8046
Differential Revision: https://secure.phabricator.com/D13311
Summary: Ref T8099, Adds ability to set hidden form open, which leaves form open on searches and moves anchor point.
Test Plan: Run saved searches, advanced searches, and searches from main search ui.
Reviewers: btrahan, epriestley
Reviewed By: epriestley
Subscribers: epriestley, Korvin
Maniphest Tasks: T8099
Differential Revision: https://secure.phabricator.com/D13308
Summary:
Ref T8099, these should sit in a 4px gutter.
Also:
- remove bad route on feed
- Fix issue with custom logos
- Fix unit error
Test Plan: Review on dashboards and on Maniphest homepage
Reviewers: btrahan, epriestley
Reviewed By: epriestley
Subscribers: epriestley, Korvin
Maniphest Tasks: T8099
Differential Revision: https://secure.phabricator.com/D13310
Summary: This should default to light, also made the icons on the dark headers more transparent.
Test Plan: Tested light, dark, blue, blindigo, and default.
Reviewers: btrahan, epriestley
Reviewed By: epriestley
Subscribers: epriestley, Korvin
Differential Revision: https://secure.phabricator.com/D13306
Summary:
Fixes T5138. Some of the "revision" properties are really "diff" properties, but we only show the properties for the most recent / current diff.
- Immediately, this makes it hard or impossible to review, e.g., lint/unit results for older diffs.
- Longer-term, these limits will become more problematic with more data on diffs after Harbormaster.
Instead, separate "revision" from "diff" properties.
(In the long term, it might make sense to show more diffs in this panel -- e.g., tabs for the 8 most recent updates or something -- but I went with the simplest approach for now since I don't have a clean way to deal with 100-update revisions offhand.)
Test Plan: {F500480}
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: cburroughs, epriestley
Maniphest Tasks: T5138
Differential Revision: https://secure.phabricator.com/D13282
Summary: Ref T8449. When an object is in a space, explain that clearly in the policy description dialog.
Test Plan: {F496126}
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T8449
Differential Revision: https://secure.phabricator.com/D13264
Summary:
- Give preference to Segoe on Windows
- Move to Lato headers except documents
- Fix diviner issues
- Fix feed overwriting line-height
- Fix apps launcher
- Fix infoview + listview
- Make tall headers less tall
- Legalpad, Diviner tweaks
Test Plan: Random surfing. @epriestley can you just commit this for me, connection generally shitty.
Reviewers: btrahan, epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Differential Revision: https://secure.phabricator.com/D13305
Summary: Ref T7458. Only index documentable atoms in the search index. In particular, this prevents files and methods from being returned in search results (clicking on these search results doesn't actually work anyway).
Test Plan: Actually, to get this to work I had to destroy the search index and recreate it... is this expected?
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: epriestley, Korvin
Maniphest Tasks: T7458
Differential Revision: https://secure.phabricator.com/D13298
Summary:
Fixes T8547. I wasn't immediately able to reproduce this locally (although I didn't try too hard), but I think the issue is that when atoms extend ghosts (probably they are usually ghosts themselves?), we try to check the ghost language and fatal.
Instead, don't match ghosts when figuring out what an atom extends.
This could maybe be a little cleaner (match the ghosts, at lower priority, and show that they're ghosts?) but I'm not sure there's a real product use case for it, and this looks like a safer way to stop the bleeding for now.
Test Plan: Poked around Diviner locally.
Reviewers: joshuaspence
Reviewed By: joshuaspence
Subscribers: epriestley
Maniphest Tasks: T8547
Differential Revision: https://secure.phabricator.com/D13300
Summary:
Fixes T8548. This needs to be cleaned up more generally at some point, but stop the bleeding for now.
If a thread member is invited to an event with a non-thread-member host, we try to render the host but don't have their handle (this is a holdover from the bygone days of events as statuses).
For now, just don't render anything. In the future, it might be nice to render (some of?) the attendees who are thread members, but I suspect we may revisit this widget more generally.
Test Plan:
- As a non thread-member, created an event and invited a thread member.
- Viewed thread as thread-member.
- Saw widget fatal.
- Applied patch.
- As thread-member, saw widget render with just "9AM-10AM", instead of "host, 9AM-10AM".
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T8548
Differential Revision: https://secure.phabricator.com/D13299
Summary: Fixes T8551, Creating a recurring event should save it as a recurring event
Test Plan: Before patch: -create an event -flag as recurring -save -Result: event is not recurring -After patch Result: event saves as recurring.
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: epriestley, Korvin
Maniphest Tasks: T8551
Differential Revision: https://secure.phabricator.com/D13302
Summary: Ref T8362, Add date format preference and respect it in date selection controls
Test Plan: Set date format preference in the user settings panels, create new event, select new start date in the correct format.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: jasonrumney, eadler, epriestley, Korvin
Maniphest Tasks: T8362
Differential Revision: https://secure.phabricator.com/D13262
Summary: I think that I've caught the bulk of these issues now.
Test Plan: Eyeball it.
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: epriestley, Korvin
Differential Revision: https://secure.phabricator.com/D13296
Summary: All classes should extend from some other class. See D13275 for some explanation.
Test Plan: `arc unit`
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: epriestley, Korvin
Differential Revision: https://secure.phabricator.com/D13283
Summary:
This translation string is wrong and causes the following warning when running unit tests:
```
[2015-06-15 16:03:41] ERROR 2: vsprintf(): Too few arguments at [/home/joshua/workspace/github.com/phacility/libphutil/src/internationalization/PhutilTranslator.php:95]
arcanist(head=master, ref.master=956bfa701c36), phabricator(head=master, ref.master=80f11427e576), phutil(head=master, ref.master=3ff84448a916)
#0 vsprintf(string, array) called at [<phutil>/src/internationalization/PhutilTranslator.php:95]
#1 PhutilTranslator::translate(string)
#2 call_user_func_array(array, array) called at [<phutil>/src/internationalization/pht.php:17]
#3 pht(string) called at [<phabricator>/src/applications/auth/controller/PhabricatorAuthStartController.php:75]
#4 PhabricatorAuthStartController::handleRequest(AphrontRequest) called at [<phabricator>/src/aphront/AphrontController.php:69]
#5 AphrontController::delegateToController(PhabricatorAuthStartController) called at [<phabricator>/src/applications/base/controller/PhabricatorController.php:213]
#6 PhabricatorController::willBeginExecution() called at [<phabricator>/src/applications/base/controller/__tests__/PhabricatorAccessControlTestCase.php:270]
#7 PhabricatorAccessControlTestCase::checkAccess(string, PhabricatorTestController, AphrontRequest, array, array) called at [<phabricator>/src/applications/base/controller/__tests__/PhabricatorAccessControlTestCase.php:112]
#8 PhabricatorAccessControlTestCase::testControllerAccessControls()
#9 call_user_func_array(array, array) called at [<arcanist>/src/unit/engine/phutil/PhutilTestCase.php:492]
#10 PhutilTestCase::run() called at [<arcanist>/src/unit/engine/PhutilUnitTestEngine.php:65]
#11 PhutilUnitTestEngine::run() called at [<arcanist>/src/workflow/ArcanistUnitWorkflow.php:186]
#12 ArcanistUnitWorkflow::run() called at [<arcanist>/scripts/arcanist.php:382]
```
Test Plan: `arc lint`
Reviewers: epriestley, #blessed_reviewers, chad
Reviewed By: #blessed_reviewers, chad
Subscribers: epriestley, Korvin
Differential Revision: https://secure.phabricator.com/D13292
Summary:
Ref T4345. This error is per object-type in the query implementations, not a mail/permissions issue.
Without `didRejectResult()`, we can't distinguish between "restricted" and "unknown" for objects filtered by `willFilterPage()`.
- Call `didRejectResult()` on commits.
- Make `didRejectResult()` handle both existing policy exceptions and filtering.
- Recover from partial objects (like commits) which are missing attached data required to figure out policies.
Test Plan: Saw "Restricted Diffusion Commit" instead of "Unknown Object (Diffusion Commit)" when viewing nonvisible commit handle in Maniphest.
Reviewers: btrahan, joshuaspence
Reviewed By: joshuaspence
Subscribers: epriestley
Maniphest Tasks: T4345
Differential Revision: https://secure.phabricator.com/D13289
Summary: This `break` statement causes `./bin/hunks migrate` to only migrate one-hunk-at-a-time, which is unnecessary and slightly misleading. Instead, allow the script to migrate //all// legacy hunks to modern storage. In particular, this means that we can recommend that installs run this command sometime before D13222 is landed.
Test Plan: It's a pain to setup the data necessary to test this, but this is identical to the change that I made on our production install when I migrated our hunk storage.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: epriestley, Korvin
Differential Revision: https://secure.phabricator.com/D13288
Summary: Remove backticks from SQL statements for consistency. In //most// places, we don't use backticks around table/field names, so at least be consistent about this.
Test Plan: Learned what backticks are used for in MySQL.
Reviewers: eadler, epriestley, #blessed_reviewers
Reviewed By: eadler, epriestley, #blessed_reviewers
Subscribers: epriestley
Differential Revision: https://secure.phabricator.com/D13267
Summary: This should (hopefully) be the last one of these since D13185 has landed.
Test Plan: `arc unit`
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: epriestley, Korvin
Differential Revision: https://secure.phabricator.com/D13284
Summary: I don't believe that any subclass should override these methods.
Test Plan: `arc unit`
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: epriestley, Korvin
Differential Revision: https://secure.phabricator.com/D13265
Summary: Couple of reasons here. Mainly, we're looking at Serif fonts for Phacility, so having headers tie back seems worth exploring. Also Oswals was a bit heavy, and in trying to find a replacement with similiar distinction, was unable to find anything suitable. Trying out Slabo here and have been pleasantly surprised how well the Serif/Sans-Serif combination works here, typographically.
Test Plan:
Test Objects, Documents, and Dashboards.
{F500207}
{F500208}
Reviewers: btrahan, epriestley
Reviewed By: epriestley
Subscribers: epriestley, Korvin
Differential Revision: https://secure.phabricator.com/D13279
Summary: Ref T4558. When publishing a new Diviner book, use the most-open policy instead of `PhabricatorPolicies::POLICY_USER`.
Test Plan: Ran `diviner generate` in a directory which didn't have published Diviner documentation.
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: epriestley, Korvin
Maniphest Tasks: T4558
Differential Revision: https://secure.phabricator.com/D13254
Summary:
Ref T5681. Getting this to work correctly is a bit tricky, mostly because of the policy checks we do prior to applying an edit.
I think I came up with a mostly-reasonable approach, although it's a little bit gross. It uses `spl_object_hash()` so it shouldn't be able to do anything bad/dangerous (the hints are strictly bound to the hinted object, which is a clone that we destroy moments later).
Test Plan:
- Added + ran a unit test.
- Created a task with a "Subscribers" policy with me as a subscriber (without the hint stuff, this isn't possible: since you aren't a subscriber *yet*, you get a "you won't be able to see it" error).
- Unsubscribed from a task with a "Subscribers" policy, was immediately unable to see it.
- Created a task with a "subscribers" policy and a project subscriber with/without me as a member (error / success, respectively).
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T5681
Differential Revision: https://secure.phabricator.com/D13259
Summary:
Ref T8488. Ref T5681.
Now you can just use `id(new ConpherenceThreadMembersPolicyRule())->getObjectPolicyFullKey()` as a policy.
Added tests for TaskAuthor + ThreadMembers.
Test Plan:
- Ran tests.
- Set a thread policy to "Members of thread'.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T5681, T8488
Differential Revision: https://secure.phabricator.com/D13258
Summary:
Ref T5681. Ref T8488. This allows policy rules to provide "Object Policies", which are similar to the global/basic policies:
- They show up directly in the dropdown (you don't have to create a custom rule).
- They don't need to create or load anything in the database.
To implement one, you just add a couple methods on an existing PolicyRule that let Phabricator know it can work as an object policy rule.
{F494764}
These rules only show up where they make sense. For example, the "Task Author" rule is only available in Maniphest, and in "Default View Policy" / "Default Edit Policy" of the Application config.
This should make T8488 easier by letting us set the default policies to "Members of Thread", without having to create a dedicated custom policy for every thread.
Test Plan:
- Set tasks to "Task Author" policy.
- Tried to view them as other users.
- Viewed transaction change strings.
- Viewed policy errors.
- Set them as default policies.
- Verified they don't leak into other policy controls.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T5681, T8488
Differential Revision: https://secure.phabricator.com/D13257
Summary:
Ref T5681. Policy rules can now select objects they can apply to, so a rule like "task author" only shows up where it makes sense (when defining task policies).
This will let us define rules like "members of thread" in Conpherence, "subscribers", etc., to make custom policies more flexible.
Notes:
- Per D13251, we need to do a little work to get the right options for policies like "Maniphest > Default View Policy". This should allow "task" policies.
- This implements a "task author" policy as a simple example.
- The `willApplyRule()` signature now accepts `$objects` to support bulk-loading things like subscribers.
Test Plan:
- Defined a task to be "visible to: task author", verified author could see it and other users could not.
- `var_dump()`'d willApplyRule() inputs, verified they were correct (exactly the objects which use the rule).
- Set `default view policy` to a task-specific policy.
- Verified that other policies like "Can Use Bulk Editor" don't have these options.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T5681
Differential Revision: https://secure.phabricator.com/D13252
Summary: Working towards a more unified look and feel. This brings in Lato as a complete base font over Helvetica Neue, as well as removing Source Sans Pro from DocumentView and Conpherence. Design-wise Lato provides the nice readability at larger font sizes that Source Sans Pro did, with the ability to scale down to tables and UI widgets with ease. This gives us one font instead of two, and now Object descriptions and Timeline posts all can benefit from a consistent, readable font.
Test Plan:
Test main UI, smaller elements like tables, menus, DocumentViews, Previews, Conpherence.
{F498135}
{F498136}
Reviewers: btrahan, epriestley
Reviewed By: epriestley
Subscribers: epriestley, Korvin
Differential Revision: https://secure.phabricator.com/D13276
Summary: Self-explanatory.
Test Plan: Viewed the project page as a user without edit permissions and saw the link greyed out.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: epriestley, Korvin
Differential Revision: https://secure.phabricator.com/D13255
Summary:
Ref T5681. Ref T6860. This doesn't do anything interesting on its own, just makes the next diff smaller.
In the next diff, policies become aware of the types of objects they're acting on. We need to specify which object type all the "Default View/Edit" settings are for so they get the right rules.
For example, a rule like "Allow task author" is OK for "View Policy" on a task, and also OK for "Default View Policy" on ManiphestApplication. But it's not OK for "Can Create Tasks" on ManiphestApplication.
So annotate all the "template"/"default" policies with their types. The next diff will use these to let you select appropriate rules for the given object type.
Test Plan:
- Used `grep` to find these.
- This change has no effect.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T5681, T6860
Differential Revision: https://secure.phabricator.com/D13251
Summary: Ref T8362, Fix query frequency unit and change time preference from input to dropdown.
Test Plan: Change user time preference in Date Time Settings panel, open feed, observe new time stamps.
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: epriestley, Korvin
Maniphest Tasks: T8362
Differential Revision: https://secure.phabricator.com/D13236
Summary: Ref T8498. This editor is an artifact of the Old World at this point, but it still works fine.
Test Plan: Moved tasks between spaces using the batch editor.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T8498
Differential Revision: https://secure.phabricator.com/D13249
Summary:
Ref T8498. Allow ApplicationEmail addresses to be put into spaces:
- You can only see and send to addresses in Spaces you have access to.
- Objects are created into the same space their address is associated with.
Test Plan:
- Used `bin/mail receive-test` to send mail to various `xyz-bugs@...` addresses.
- Saw objects created in the proper space.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T8498
Differential Revision: https://secure.phabricator.com/D13247
Summary: Fixes T8503. On 32-bit systems, these 64-bit integers will be mangled by the '%d' conversion implied by the 'int' type.
Test Plan: Somewhat guessing here since I don't have a 32-bit system handy, but this strongly echoes similar issues in the past. Feed works fine locally. Looked at the generated queries.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T8503
Differential Revision: https://secure.phabricator.com/D13250
Summary:
Ref T8498. I want to add Spaces to these, and the logic for getting Spaces right is a bit tricky, so swap these to ApplicationTransactions.
One new piece of tech: made it easier for Editors to raise DuplicateKeyException as a normal ValidationException, so callers don't have to handle this case specially.
One behavioral change: we no longer require these addresses to be at the `auth.email-domains` domains -- I think this wasn't quite right in the general case. It's OK to require users to have `@mycompany.com` addresses but add `@phabricator.mycompany-infrastructure.com` addresses here if you want.
Test Plan:
- Tried to create a duplicate email.
- Tried to create an empty email.
- Tried to create an invalid email.
- Created a new email.
- Deleted an email.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T8498
Differential Revision: https://secure.phabricator.com/D13246
Summary: Ref T8449. Get rid of the `S123` stuff in headers, it feels like clutter.
Test Plan: Looked at objects in Spaces.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T8449
Differential Revision: https://secure.phabricator.com/D13243
Summary: Ref T8498. Allow Herald rules to act on the Space which contains an object.
Test Plan:
- Wrote a "Space is any of..." rule, created tasks that matched and failed the rule.
- Also created a Pholio rule with the "Space..." condition.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T8498
Differential Revision: https://secure.phabricator.com/D13242
Summary:
Ref T8377. This adds a standard disable/enable feature to Spaces, with a couple of twists:
- You can't create new stuff in an archived space, and you can't move stuff into an archived space.
- We don't show results from an archived space by default in ApplicationSearch queries. You can still find these objects if you explicitly search for "Spaces: <the archived space>".
So this is a "put it in a box in the attic" sort of operation, but that seems fairly nice/reasonable.
Test Plan:
- Archived and activated spaces.
- Used ApplicationSearch, which omitted archived objects by default but allowed searches for them, specifically, to succeed.
- Tried to create objects into an archived space (this is not allowed).
- Edited objects in an archived space (this is OK).
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T8377
Differential Revision: https://secure.phabricator.com/D13238
Summary: Fixes T8464. We could lose the additional users from "Send an email..." rules //if// Herald did not apply any other transactions to the task.
Test Plan:
- Destroyed all Herald rules.
- Created a single "Send an email to..." rule.
- Created a task.
- Saw target get an email.
Reviewers: btrahan, chad
Reviewed By: chad
Subscribers: epriestley
Maniphest Tasks: T8464
Differential Revision: https://secure.phabricator.com/D13245
Summary:
Ref T8493. Tricks:
- "Create Similar Task" and "Create Subtask" should copy the parent's Space.
- Normal list view + workboard card view.
Test Plan:
- Created a task, edited space, etc.
- Viewed tasks with different users.
- Created a "Similar Task" (saw proper Space).
- Created a subtask (saw proper Space).
- Viewed workboard.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T8493
Differential Revision: https://secure.phabricator.com/D13232
Summary:
Ref T8493. Add Spaces support to Pholio.
This is straightforward; Pholio has no clone/copy/fork or weird parent/child stuff going on.
Test Plan: Created a mock, put it in a space, looked at it as another user, searched for stuff in spaces, viewed Macros.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T8493
Differential Revision: https://secure.phabricator.com/D13231
Summary:
Ref T8449. Try out some more subtle behaviors:
- Make the "Space" control part of the policy control, so the UI shows "Visible To: [Space][Policy]". I think this helps make the role of spaces more clear. It also makes them easier to implement.
- Don't show the default space in headers: instead, show nothing.
- If the user has access to only one space, pretend spaces don't exist (no edit controls, no header stuff).
This might be confusing, but I think most of the time it will all align fairly well with user expectation.
Test Plan:
- Viewed a list of pastes (saw Space with non-default space, no space with default space, no space with user in only one space).
- Viewed a paste (saw Space with non-default space, saw no space with default space, saw no space with user in only one space).
- Edited spaces on objects (control as privileged user, no control as locked user).
- Created a new paste in a space (got space select as privileged user, no select as locked user).
Reviewers: chad, btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T8449
Differential Revision: https://secure.phabricator.com/D13229
Summary: Ref T8472, Fix missing recurrence end date control
Test Plan: Create new event, recurrence end date should be available.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: epriestley, Korvin
Maniphest Tasks: T8472
Differential Revision: https://secure.phabricator.com/D13241
Summary: Ref T8496. In D13123, the condition for establishing a web session was made too strict: we need to let non-activated users establish web sessions in order to see "you are a bad disabled person" or "your account needs approval" messages. The previous behavior let them in, the new behavior incorrectly locks them out.
Test Plan: Enabled login approvals and registered a new account with username/password auth.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: joshuaspence, epriestley
Maniphest Tasks: T8496
Differential Revision: https://secure.phabricator.com/D13239
Summary: Fixes T8473. Adds garbage collection to the `phabricator_feed.feed_storynotification` table.
Test Plan: Reduced the TTL to a really small value and ran the trigger daemon. Saw feed notifications removed from the database.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: epriestley, Korvin
Maniphest Tasks: T8473
Differential Revision: https://secure.phabricator.com/D13233
Summary:
If you have a saved query with an order alias, we currently apply the order correctly but don't show the right value in the UI.
Map any saved value to the canoncial value when rendering the control.
Test Plan: Added some `var_dump()` and verified order key was getting mapped forward correctly.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Differential Revision: https://secure.phabricator.com/D13227
Summary:
Fixes T8483. I did this incorrectly in D13159, by doing it correctly first and then editing it carelessly. For most transaction types, it didn't matter, but did for inline state.
Also, clean up any bad inline state transactions.
Test Plan:
- Ran migration, bad transactions vanished.
- Marked some inline comments as done.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T8483
Differential Revision: https://secure.phabricator.com/D13226
Summary:
Ref T8441. Ref T7715. Move Maniphest to SearchFields.
The only new tech here is hiding fields, which we use to hide some fields on the dashboard query UI.
Test Plan:
- Queried by each field, including custom fields.
- Used some standrad queries.
- Used dashboards, used standard + custom queries.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T7715, T8441
Differential Revision: https://secure.phabricator.com/D13225
Summary: Ref T8478. This is easier than I thought, let's see if it's conspicious?
Test Plan:
- Embedded `{Z2}` and saw "Private Correspondence".
- Still saw normal stuff with useful participant lists/titles in all main UIs.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T8478
Differential Revision: https://secure.phabricator.com/D13220
Summary: Ref T8472, Add helper method on event object to determine if event is the parent of a recurrence.
Test Plan: No user facing change.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: epriestley, Korvin
Maniphest Tasks: T8472
Differential Revision: https://secure.phabricator.com/D13221
Summary:
We can end up here with a stack trace like this, while rendering an embedded Slowvote trying to publish a Feed story:
```
Daemon 43450 STDE [Mon, 08 Jun 2015 22:49:57 +0000] [2015-06-08 22:49:57] EXCEPTION: (PhutilProxyException) Error while executing Task ID 830591. {>} (PhabricatorDataNotAttachedException) Attempting to access attached data on PhabricatorUser (via getAlternateCSRFString()), but the data is not actually attached. Before accessing attachable data on an object, you must load and attach it.
Daemon 43450 STDE [Mon, 08 Jun 2015 22:49:57 +0000]
Daemon 43450 STDE [Mon, 08 Jun 2015 22:49:57 +0000] Data is normally attached by calling the corresponding needX() method on the Query class when the object is loaded. You can also call the corresponding attachX() method explicitly. at [<phabricator>/src/infrastructure/storage/lisk/PhabricatorLiskDAO.php:166]
Daemon 43450 STDE [Mon, 08 Jun 2015 22:49:57 +0000] arcanist(head=master, ref.master=7d15b85a1bc0), phabricator(head=master, ref.master=929f5f22acef), phutil(head=master, ref.master=92882eb9404d)
Daemon 43450 STDE [Mon, 08 Jun 2015 22:49:57 +0000] #0 <#2> PhabricatorLiskDAO::assertAttached(string) called at [<phabricator>/src/applications/people/storage/PhabricatorUser.php:556]
Daemon 43450 STDE [Mon, 08 Jun 2015 22:49:57 +0000] #1 <#2> PhabricatorUser::getAlternateCSRFString() called at [<phabricator>/src/applications/people/storage/PhabricatorUser.php:432]
Daemon 43450 STDE [Mon, 08 Jun 2015 22:49:57 +0000] #2 <#2> PhabricatorUser::generateToken(integer, integer, string, integer) called at [<phabricator>/src/applications/people/storage/PhabricatorUser.php:344]
Daemon 43450 STDE [Mon, 08 Jun 2015 22:49:57 +0000] #3 <#2> PhabricatorUser::getRawCSRFToken() called at [<phabricator>/src/applications/people/storage/PhabricatorUser.php:357]
Daemon 43450 STDE [Mon, 08 Jun 2015 22:49:57 +0000] #4 <#2> PhabricatorUser::getCSRFToken() called at [<phabricator>/src/infrastructure/javelin/markup.php:91]
Daemon 43450 STDE [Mon, 08 Jun 2015 22:49:57 +0000] #5 <#2> phabricator_form(PhabricatorUser, array, array) called at [<phabricator>/src/applications/slowvote/view/SlowvoteEmbedView.php:169]
Daemon 43450 STDE [Mon, 08 Jun 2015 22:49:57 +0000] #6 <#2> SlowvoteEmbedView::render() called at [<phabricator>/src/view/AphrontView.php:175]
Daemon 43450 STDE [Mon, 08 Jun 2015 22:49:57 +0000] #7 <#2> AphrontView::producePhutilSafeHTML() called at [<phutil>/src/markup/render.php:133]
Daemon 43450 STDE [Mon, 08 Jun 2015 22:49:57 +0000] #8 <#2> phutil_escape_html(SlowvoteEmbedView)
Daemon 43450 STDE [Mon, 08 Jun 2015 22:49:57 +0000] #9 <#2> array_map(string, array) called at [<phutil>/src/markup/engine/remarkup/PhutilRemarkupBlockStorage.php:56]
Daemon 43450 STDE [Mon, 08 Jun 2015 22:49:57 +0000] #10 <#2> PhutilRemarkupBlockStorage::restore(PhutilSafeHTML, integer) called at [<phutil>/src/markup/engine/PhutilRemarkupEngine.php:299]
Daemon 43450 STDE [Mon, 08 Jun 2015 22:49:57 +0000] #11 <#2> PhutilRemarkupEngine::restoreText(PhutilSafeHTML, integer) called at [<phutil>/src/markup/engine/PhutilRemarkupEngine.php:295]
Daemon 43450 STDE [Mon, 08 Jun 2015 22:49:57 +0000] #12 <#2> PhutilRemarkupEngine::postprocessText(array) called at [<phabricator>/src/infrastructure/markup/PhabricatorMarkupEngine.php:138]
Daemon 43450 STDE [Mon, 08 Jun 2015 22:49:57 +0000] #13 <#2> PhabricatorMarkupEngine::process() called at [<phabricator>/src/applications/feed/story/PhabricatorFeedStory.php:167]
Daemon 43450 STDE [Mon, 08 Jun 2015 22:49:57 +0000] #14 <#2> PhabricatorFeedStory::loadAllFromRows(array, PhabricatorUser) called at [<phabricator>/src/applications/feed/query/PhabricatorFeedQuery.php:37]
Daemon 43450 STDE [Mon, 08 Jun 2015 22:49:57 +0000] #15 <#2> PhabricatorFeedQuery::willFilterPage(array) called at [<phabricator>/src/infrastructure/query/policy/PhabricatorPolicyAwareQuery.php:237]
Daemon 43450 STDE [Mon, 08 Jun 2015 22:49:57 +0000] #16 <#2> PhabricatorPolicyAwareQuery::execute() called at [<phabricator>/src/infrastructure/query/policy/PhabricatorPolicyAwareQuery.php:168]
Daemon 43450 STDE [Mon, 08 Jun 2015 22:49:57 +0000] #17 <#2> PhabricatorPolicyAwareQuery::executeOne() called at [<phabricator>/src/applications/feed/worker/FeedPushWorker.php:12]
Daemon 43450 STDE [Mon, 08 Jun 2015 22:49:57 +0000] #18 <#2> FeedPushWorker::loadFeedStory() called at [<phabricator>/src/applications/feed/worker/FeedPublisherWorker.php:6]
Daemon 43450 STDE [Mon, 08 Jun 2015 22:49:57 +0000] #19 <#2> FeedPublisherWorker::doWork() called at [<phabricator>/src/infrastructure/daemon/workers/PhabricatorWorker.php:91]
Daemon 43450 STDE [Mon, 08 Jun 2015 22:49:57 +0000] #20 <#2> PhabricatorWorker::executeTask() called at [<phabricator>/src/infrastructure/daemon/workers/storage/PhabricatorWorkerActiveTask.php:162]
Daemon 43450 STDE [Mon, 08 Jun 2015 22:49:57 +0000] #21 <#2> PhabricatorWorkerActiveTask::executeTask() called at [<phabricator>/src/infrastructure/daemon/workers/PhabricatorTaskmasterDaemon.php:22]
Daemon 43450 STDE [Mon, 08 Jun 2015 22:49:57 +0000] #22 PhabricatorTaskmasterDaemon::run() called at [<phutil>/src/daemon/PhutilDaemon.php:185]
Daemon 43450 STDE [Mon, 08 Jun 2015 22:49:57 +0000] #23 PhutilDaemon::execute() called at [<phutil>/scripts/daemon/exec/exec_daemon.php:125]
```
Just return `null`.
Test Plan: Will check that tasks clear in production.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Differential Revision: https://secure.phabricator.com/D13218
Summary:
Ref T8478. I think the cycle is:
- Conpherence Thread > Loads handle for participant > loads file for profile image > loads attached files to check visibility > loads conpherence thread > ...
So, specifically, someone attached their profile image to a thread or message somewhere.
This breaks the cycle by stopping the attached-files visibility check from happening, since we don't need it. This seemed like the easiest link in the chain to break.
//Ideally//, I think the longer-term and more complete fix here is to stop Conpherence from requiring handles in order to load thread handles (and, generally, having a "handles must not load other handles" rule), but that's not trivial and might not be especially practical.
Test Plan: Will test in production.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T8478
Differential Revision: https://secure.phabricator.com/D13216
Summary: Now that Users implement PhabricatorApplicationTransactionInterface, we try to write an inverse edge. At least for now, we should retain the old behavior instead.
Test Plan:
- Unit tests which cover this stuff pass again.
- Grepped for other `instanceof PhabricatorApplicationTransactionInterface`, the all seemed either benign or irrelevant.
Reviewers: joshuaspence, btrahan
Reviewed By: btrahan
Subscribers: epriestley
Differential Revision: https://secure.phabricator.com/D13215
Summary: See also D13186.
Test Plan: Ran `arc unit --everything`
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: Korvin, epriestley
Differential Revision: https://secure.phabricator.com/D13201
Summary: Right now, "Publish" workers for user profile edits (title / blub) can get gummed up in the daemons. Implement the interfaces and provide a Query so they can go through.
Test Plan:
- Made a profile "Title" edit.
- Used `bin/worker execute --id <id>` to see task fail.
- Applied patch.
- Saw task work.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Differential Revision: https://secure.phabricator.com/D13213
Summary: Ref T8441. Ref T7715. Ref T7909. Clean up all the ordering and grouping hacks in Maniphest so we can drive it through normal infrastructure, move it to SearchField, introduce Spaces, and eventually modernize the Conduit API.
Test Plan:
- Executed all grouping/ordering queries, including custom queries.
- Forced execution with old aliases; got modern results.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T7909, T7715, T8441
Differential Revision: https://secure.phabricator.com/D13197
Summary: Ref T8441. Ref T7715. I'm primarily clearing callers to `saveQueryOrder()` so I can get rid of it.
Test Plan: Used all Macro search features.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T7715, T8441
Differential Revision: https://secure.phabricator.com/D13194
Summary:
Ref T8441. Ref T7715. Automatically generate a modern "Order" control in ApplicationSearch for engines which fully support SearchField.
Notably, this allows the standard "Order" control to automatically support custom field orders. We do this in Maniphest today, but in an ad-hoc way.
Test Plan: Performed order-by queries in Almanac (Services), Pholio, Files, People, Projects, and Paste.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T7715, T8441
Differential Revision: https://secure.phabricator.com/D13193
Summary:
Ref T8441. Ref T7715. For modern Query classes, automatically make subscriber queries and SearchField integrations work.
In particular, we can just drive this query with EdgeLogic and don't need to do anything specific on these Query classes beyond making sure they're implemented in a way that picks up all of the EdgeLogic clauses.
Test Plan:
- Searched for subscribers in Pholio, Files, Paste, and Projects.
- Searched for all other fields in Projects to check that Query changes are OK.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T7715, T8441
Differential Revision: https://secure.phabricator.com/D13191
Summary:
Ref T8441. Ref T7715. This is the second of three ApplicationSearch + CustomField use cases (Maniphest is the third).
Also add a way to set a default ordering for the fields.
Test Plan:
- Performed searches with each field.
- Added a custom field and searched for it.
- Observed desired ordering.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T7715, T8441
Differential Revision: https://secure.phabricator.com/D13190
Summary:
Ref T8441. Ref T7715. This is mostly about getting SearchFields + CustomFields working.
(This includes a couple of SearchFields which aren't used quite yet.)
Test Plan:
- Used all search controls.
- Defined custom fields and searched for them.
- Created an old saved search which searches on custom fields on master, switched to this patch, search worked exaclty as written.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T7715, T8441
Differential Revision: https://secure.phabricator.com/D13189
Summary: Ref T8357, Check if recurrence end date is disabled before saving it.
Test Plan: Create new event, before saving, leave "recurrence end date" unchecked, save, should not get an error.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: epriestley, Korvin
Maniphest Tasks: T8357
Differential Revision: https://secure.phabricator.com/D13212
Summary: Fixes T8459, Correctly display event id and sequence in crumb and page title on eventviewcontroller
Test Plan: Open `E111/3`, crumb and title should display '`E111 (3)`' instead of '`E111`'.
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T8459
Differential Revision: https://secure.phabricator.com/D13211
Summary:
Fixes T8464. We could incorrectly use a cached value when computing CC's.
Just load a fresh value. There are no other callers that would benefit from this cache, so it's more complicated to reload it correctly prior to publishing than to just skip it.
Also make the PHID headers unique.
Test Plan:
- Verified that users received mail about the transactions which caused them to be added to an object.
- Veirfied that headers no longer have redundant values.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T8464
Differential Revision: https://secure.phabricator.com/D13206
Summary:
Fixes an issue where mentioning an event name (like `E999`) in a revision summary (and probably elsewhere) would produce an error like this:
```
2015/06/08 17:30:22 [error] 27702#0: *307450 FastCGI sent in stderr: "PHP message: [2015-06-08 17:30:22] EXCEPTION: (Exception) Transaction ("PHID-XACT-CEVT-zglgzy36aote5ja", of type "core:edge") requires a handle ("PHID-DREV-4m6vorimvg4bm3ltskca") that it did not load. at [<phabricator>/src/applications/transactions/storage/PhabricatorApplicationTransaction.php:285]
PHP message: arcanist(head=master, ref.master=8c589f1f759f), phabricator(head=master, ref.master=b4de79741ceb), phutil(head=master, ref.master=4a0e1b47a584)
PHP message: #0 <#2> PhabricatorApplicationTransaction::getHandle(string) called at [<phabricator>/src/applications/transactions/storage/PhabricatorApplicationTransaction.php:474]
PHP message: #1 <#2> PhabricatorApplicationTransaction::shouldHide() called at [<phabricator>/src/applications/calendar/storage/PhabricatorCalendarEventTransaction.php:82]
PHP message: #2 <#2> PhabricatorCalendarEventTransaction::shouldHide() called at [<phutil>/src/utils/utils.php:428]
PHP message: #3 <#2> mfilter(array, string, boolean) called at [<phabricator>/src/applications/calendar/editor/PhabricatorCalendarEventEditor.php:350]
PHP message: #4 <#2> PhabricatorCalendarEventEditor::shouldSendMail(PhabricatorCalendarEvent, array) called at [<phabricator>/src/applications/transactions/editor/PhabricatorApplicationTransactionEditor.php:990]
PHP message: #5 <#2> PhabricatorApplicationTransactionEditor::applyTransactions(PhabricatorCalendarEvent, array) called at [<phabricator>/src/applications/transactions/editor/PhabricatorApplicationTransactionEditor.php:2937]
PHP message: #6 <#2> PhabricatorApplicationTransactionEditor::applyInverseEdgeTransactions(DifferentialRevision, DifferentialTransaction, integer) called at [<phabricator>/src/applications/transactions/editor/PhabricatorApplicationTransactionEditor.php:602]
...
```
This is similar to rP98aae51c / rP0fc0af64, but for Calendar. (Likely, I copy/pasted the Editor from Maniphest a while ago.)
We don't need to do this filtering here because we do it later before sending mail. Additionally, because some transactions may hide or show depending on the viewer, it's strictly incorrect to do it here.
Test Plan:
- Created a revision which mentioned a bunch of events.
- Grepped for other implementations of `shouldSendMail()` and verified that none try to perform similar filtering.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley, lpriestley
Differential Revision: https://secure.phabricator.com/D13210
Summary:
Ref T8455. Use standard effects for revisions, instead of a custom effect.
This fixes the major issue (conduit error) in T8455 because the standard effect now performs PHID type filtering.
This retains other behaviors (in particular: not re-CC'ing explicitly removed CCs).
Test Plan:
- With a Herald rule that adds a mailing list as a CC, created a revision before the change and hit the error in T8455. After the change, saw correct behavior.
- Wrote a normal Herald rule to add CCs and created a revision, saw it fire properly.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T8455
Differential Revision: https://secure.phabricator.com/D13183
Summary: Ref T8455. Use the standard effect in task rules, instead of a custom effect.
Test Plan: Wrote a Maniphest CC rule, updated a task, saw rule activate properly.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T8455
Differential Revision: https://secure.phabricator.com/D13182
Summary: Ref T8455. Use the standard actions in commit rules, instead of a custom action.
Test Plan: Wrote an "add cc" Herald rule for commits, pushed a commit, saw the rule fire correctly.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T8455
Differential Revision: https://secure.phabricator.com/D13181
Summary: Ref T8455. Instead of using an ad-hoc subscribers effect in Phriction, use the new standard one (introduced previously; this depends on D13178).
Test Plan:
- Wrote an "add subscribers" Herald rule, updated a Phriction document, saw it apply its effect.
- Observed availability of "remove" subscribers actions.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T8455
Differential Revision: https://secure.phabricator.com/D13180
Summary:
Ref T8455. Begins consolidating the code for applying these effects:
- Makes Add/Remove subscribers a standard effect, and uses it in Pholio.
- This includes the "don't re-subscribe users who have explicitly unsubscribed" logic from Differential in the standard effect. I think this rule is always desirable.
- This adds new filtering of invalid PHID types to resolve the `arc diff` issue in T8455 once Differential uses this standard effect.
- Added "Remove Subscribers" to MockAdapter in order to test that it works.
- Relabeled "CC" in Pholio to "Subscribers" for consistency.
Test Plan:
- Created several rules which add subscribers to (and remove subscribers from) mocks.
- Updated mocks, changing properties and adding and removing subscribers.
- Observed transactions applying and aggregating properly.
- Observed add/remove rules each working correctly.
- Observed the "don't re-add unsubscribed users" condition acting on subscribers who had previously been added but explicitly removed/unsubscribed.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T8455
Differential Revision: https://secure.phabricator.com/D13179
Summary:
Ref T8455. The Herald code in general isn't nearly as modular as it should be, and the subscriber code particularly has some legacy cruft. This is making it fragile and causing the issue described in T8455.
Currently, each Herald adapter has essentially identical code which it uses to determine which users are subscribed to an object. Instead, share code between object types.
I removed "explicitCCs":
- The value was always identical to doing the query in the common/standard way.
- They were only used to print a diagnostic message on transcripts, which I think is no longer relevant.
- I believe it predates transactions, so when it was added you couldn't figure out the old object state by looking at the transaction history. Now, CC changes are recorded there, so there's no need to restate the CC state on the transcript.
- Even if we do want to restore this (or something similar), we can do it directly from Herald now.
Test Plan:
- Created rules that use the "CCs" field in Herald, Pholio, Maniphest and Differential.
- Updated objects in each application.
- Observed valid field reads in the tranascript.
- Grepped for `FIELD_CC`.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T8455
Differential Revision: https://secure.phabricator.com/D13177
Summary: Ref T8463.
Test Plan:
- Created a new revision via web UI with a username `@mention` in the summary and no repository.
- Prior to patch, hit a "not attached" error.
- After patch, no error.
- Created a new web UI revision, as above, but with a repository; saw repository work fine.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T8463
Differential Revision: https://secure.phabricator.com/D13205
Summary: Fixes T8433. Add an index to the `phabricator_feed.feed_storynotification` table. Primarily, this speeds up object destruction.
Test Plan: Ran `./bin/storage upgrade` and saw the migration apply cleanly.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: Korvin, epriestley
Maniphest Tasks: T8433
Differential Revision: https://secure.phabricator.com/D13164
Summary: Remove the `*TransactionType` classes and define the constants in the corresponding `*Transaction` class instead.
Test Plan: `grep`
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: Korvin, epriestley
Differential Revision: https://secure.phabricator.com/D13188
Summary: See D13185, the `testMethodVisibility()` test was not being executed.
Test Plan: `arc unit`
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: Korvin, epriestley
Differential Revision: https://secure.phabricator.com/D13186
Summary: Fixes T8458, Show informative errors when attempting to set a recurrence end date on a non-recurring event.
Test Plan: Create new event, set recurrence end date via date-picker without setting the "is recurring" checkbox, and attempt to save. Should get error saying there cannot be a recurrence end date on a non-recurring event.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: Korvin, epriestley
Maniphest Tasks: T8458
Differential Revision: https://secure.phabricator.com/D13192
Summary:
Ref T8441. Ref T7715.
- These are obsolete after the Viewer/HandlePool changes.
- These are unused after the typeahead parameterization changes.
Test Plan: `grep`, poked around.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T7715, T8441
Differential Revision: https://secure.phabricator.com/D13176
Ref T8454. This got gummed up in swapping between double negatives like "noDisabled".
Viewed queue, saw "Hide Disabled Users" instead of "Show Only Disabled Users".
Auditors: joshuaspence, btrahan
Summary: Ref T8441. Does what it says, provided other conditions (like using the new SearchField stuff) are fulfilled.
Test Plan:
{F473836}
{F473837}
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T8441
Differential Revision: https://secure.phabricator.com/D13171
Summary: Ref T8441. Ref T7715. Let SearchFields do some value massaging before buiding queries so we don't have to work as hard in each SearchEngine. Particularly, they can handle evaluateTokens() from Tokenizers.
Test Plan: Paste automatically gained access to `viewer()`, etc.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T7715, T8441
Differential Revision: https://secure.phabricator.com/D13170
Summary:
Ref T8441. Ref T7715. Add a layer of indirection to fields in search engines. This will allow us to:
- Simplify SearchEngine code, which has collected a lot of duplication around expressing what is effectively field types.
- Automatically add fields like "Spaces" and "Projects" (primary driver for T8441).
- Reorder or hide fields (not sure if we really want to do this, but it seems plausible, and this will let us play around with it, at least).
- Drive Conduit Query methods via SearchEngines, so the same code specifies both the search UI and the `application.query` endpoint (primary driver in T7715).
Test Plan:
- Searched for stuff in Paste, everything behaved exaclty like it used to (except that I removed the "no language" checkbox, which seemed like fluff from a bygone era).
- Searched for stuff in other applications, saw no changes.
- Hit date field errors.
- Used query strings to specify values.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T7715, T8441
Differential Revision: https://secure.phabricator.com/D13169
Summary: Ref T8441. I want to use `PhabricatorSearchField` for a better, more useful object.
Test Plan: `grep`, `arc lint`
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T8441
Differential Revision: https://secure.phabricator.com/D13168
Summary:
Ref T8434. Hard-codes form sources as a complaint form.
This form is close to perfect and I'm not actually sure we need to let users customize it at all.
Test Plan: {F473587}
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T8434
Differential Revision: https://secure.phabricator.com/D13166
Summary: Ref T8434. Throw these together.
Test Plan: Created a new Queue.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T8434
Differential Revision: https://secure.phabricator.com/D13163
Summary: Ref T8434. Now you can list them.
Test Plan: Listed sources.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T8434
Differential Revision: https://secure.phabricator.com/D13162
Summary:
Ref T8434. Minor cleanup/modernization. I made type selection modal (like Herald, Auth, etc) so we can render the form on the next screen based on the type.
{F472519}
Test Plan: Created a new source, edited an existing source.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T8434
Differential Revision: https://secure.phabricator.com/D13161
Summary: Ref T8434. Touch things up a bit. This also cleans up the missing ApplicationTransactionInterface for T6367.
Test Plan: Didn't create a new queue. Didn't edit an existing queue.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: jeremyb, epriestley
Maniphest Tasks: T8434
Differential Revision: https://secure.phabricator.com/D13160
Summary:
Ref T8424. This adds crude integration with Paste's edit/view workflows: you can change the space a Paste appears in, see transactions, and get a policy callout.
Lots of rough edges and non-obviousness but it pretty much works.
Test Plan:
- Created and updated Pastes.
- Moved them between spaces, saw policy effects.
- Read transactions.
- Looked at feed.
- Faked query to return no spaces, saw control and other stuff vanish.
- Faked query to return no spaces, created pastes.
- Tried to submit bad values and got errors.
Reviewers: chad, btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T8424
Differential Revision: https://secure.phabricator.com/D13159
Test Plan: Submitted a form - saw nothing out of ordinary.
Reviewers: #blessed_reviewers
Subscribers: epriestley
Differential Revision: https://secure.phabricator.com/D13167
Summary: Ref T4558. Allow ghost atoms to be rendered in #diviner. This functionality didn't exist previously, but was hinted at by the TODO comments.
Test Plan: Generated #diviner documentation for rARC and then removed a class (before re-generating the documentation). Navigated to the documentation for the removed class and saw "This atom no longer exists".
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: epriestley
Maniphest Tasks: T4558
Differential Revision: https://secure.phabricator.com/D13114
Summary:
Ref T8424. No UI or interesting behavior yet, but integrates Spaces checks:
- `PolicyFilter` now checks Spaces.
- `PolicyAwareQuery` now automatically adds Spaces constraints.
There's one interesting design decision here: **spaces are stronger than automatic capabilities**. That means that you can't see a task in a space you don't have permission to access, //even if you are the owner//.
I //think// this is desirable. Particularly, we need to do this in order to exclude objects at the query level, which potentially makes policy filtering for spaces hugely more efficient. I also like Spaces being very strong, conceptually.
It's possible that we might want to change this; this would reduce our access to optimizations but might be a little friendlier or make more sense to users later on.
For now, at least, I'm pursuing the more aggressive line. If we stick with this, we probably need to make some additional UI affordances (e.g., show when an owner can't see a task).
This also means that you get a hard 404 instead of a policy exception when you try to access something in a space you can't see. I'd slightly prefer to show you a policy exception instead, but think this is generally a reasonable tradeoff to get the high-performance filtering at the Query layer.
Test Plan:
- Added and executed unit tests.
- Put objects in spaces and viewed them with multiple users.
- Made the default space visible/invisible, viewed objects.
- Checked the services panel and saw `spacePHID` constraints.
- Verified that this adds only one query to each page.
Reviewers: btrahan, chad
Reviewed By: btrahan
Subscribers: chad, epriestley
Maniphest Tasks: T8424
Differential Revision: https://secure.phabricator.com/D13156
Summary:
Ref T8424. When users are rejected because they can't see the space an object is in, this isn't really a capability rejection. Don't require a capability when rejecting objects.
This mostly simplifies upcoming changes.
Test Plan:
- Viewed a capability exception dialog, it looked the same as always.
- (After additional changes, viewed a space exception dialog.)
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T8424
Differential Revision: https://secure.phabricator.com/D13155
Summary:
Ref T8424. I'm using Paste as a testbed application because Spaces make some degree of sense for it but it's also flat/simple.
This doesn't do anything interesting or useful and mostly just making the next (more interesting) diff smaller.
Test Plan:
- Ran `bin/storage upgrade -f`.
- Browsed pastes.
- Created a paste.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T8424
Differential Revision: https://secure.phabricator.com/D13154
Summary:
Ref T8424. This adds a standard KeyValueCache to serve as a request cache.
In particular, I need to cache Spaces (they are frequently accessed, sometimes by multiple viewers) but not have them survive longer than the scope of one request.
This request cache is explicitly destroyed by each web request and each daemon request.
In the very long term, building this kind of construct supports reusing PHP interpreters to run web requests (see some discussion in T2312).
Test Plan:
- Added and executed unit tests.
- Ran every daemon.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T8424
Differential Revision: https://secure.phabricator.com/D13153
Summary:
Fixes T8326. This removes calls to PhabricatorStartup from places that daemons may access.
This salt doesn't need to be global; it's embedded in the token we return. It's fine if we use a different salt every time. In practice, we always use the same viewer, so this change causes little or no behavioral change.
Ref T8424. For Spaces, I need a per-request cache for all spaces, because they have unusual access patterns and require repeated access, in some cases by multiple viewers.
We don't currently have a per-request in-process cache that we, e.g., clear in the daemons.
We do have a weak/theoretical/forward-looking attempt at this in `PhabricatorStartup::getGlobal()` but I'm going to throw that away (it's kind of junky, partly because of T8326) and replace it with a more formal mechanism.
Test Plan:
- Submitted some forms.
- Grepped for `csrf.salt`.
- Viewed page source, saw nice CSRF tokens with salt.
- All the salts are still the same on every page I checked, but it doesn't matter if this isn't true everywhere.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T8326, T8424
Differential Revision: https://secure.phabricator.com/D13151
Summary: Fixes T7458. Integrates #diviner into #applicationsearch by indexing `DivinerLiveBook` and `DivinerLiveSymbol` search documents. Depends on D13157.
Test Plan: Ran `./bin/search index --all --type BOOK` and `./bin/search index --all --type ATOM` and then searched for various symbols via global search.
Reviewers: epriestley, #blessed_reviewers
Subscribers: Korvin, epriestley
Maniphest Tasks: T7458
Differential Revision: https://secure.phabricator.com/D13090
Summary: I missed these in D13157.
Test Plan: Browsed around #diviner.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: Korvin, epriestley
Differential Revision: https://secure.phabricator.com/D13158
Summary: Fixes T8401. Change `withIncludeGhosts()` to `withExcludeGhosts()` and `withIncludeUndocumentable()` to `withExcludeDocumentable()`. In particular, this allows querying for atoms by PHID to work as expected.
Test Plan: I got confused with double negatives so I might have gotten some of these wrong... I poked around Diviner and re-generated documentation to verify that this is working as expected.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: Korvin, epriestley
Maniphest Tasks: T8401
Differential Revision: https://secure.phabricator.com/D13157
Summary: Ref T8099, hashtag#yolo. Adds back the original gradients plus a 'light' theme. Unclear which should be default, but we can play with it until a decision needs to be made.
Test Plan: Change colors a lot, turn on durable column.
Reviewers: btrahan, epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T8099
Differential Revision: https://secure.phabricator.com/D13146
This fixes this, which only crops up some of the time:
```
Daemon 39827 STDE [Thu, 04 Jun 2015 03:03:11 +0000] [2015-06-04 03:03:10] EXCEPTION: (PhutilProxyException) Error while executing Task ID 902271. {>} (PhabricatorDataNotAttachedException) Attempting to access attached data on PhabricatorRepositoryCommit (via getCommitData()), but the data is not actually attached. Before accessing attachable data on an object, you must load and attach it.
Daemon 39827 STDE [Thu, 04 Jun 2015 03:03:11 +0000]
Daemon 39827 STDE [Thu, 04 Jun 2015 03:03:11 +0000] Data is normally attached by calling the corresponding needX() method on the Query class when the object is loaded. You can also call the corresponding attachX() method explicitly. at [<phabricator>/src/infrastructure/storage/lisk/PhabricatorLiskDAO.php:166]
Daemon 39827 STDE [Thu, 04 Jun 2015 03:03:11 +0000] arcanist(head=master, ref.master=8c589f1f759f), phabricator(head=master, ref.master=4a45620b0458), phutil(head=master, ref.master=afc05a9a7f00)
Daemon 39827 STDE [Thu, 04 Jun 2015 03:03:11 +0000] #0 <#2> PhabricatorLiskDAO::assertAttached(string) called at [<phabricator>/src/applications/repository/storage/PhabricatorRepositoryCommit.php:129]
Daemon 39827 STDE [Thu, 04 Jun 2015 03:03:11 +0000] #1 <#2> PhabricatorRepositoryCommit::getCommitData() called at [<phabricator>/src/applications/audit/editor/PhabricatorAuditEditor.php:669]
Daemon 39827 STDE [Thu, 04 Jun 2015 03:03:11 +0000] #2 <#2> PhabricatorAuditEditor::buildMailBody(PhabricatorRepositoryCommit, array) called at [<phabricator>/src/applications/transactions/editor/PhabricatorApplicationTransactionEditor.php:2178]
Daemon 39827 STDE [Thu, 04 Jun 2015 03:03:11 +0000] #3 <#2> PhabricatorApplicationTransactionEditor::sendMailToTarget(PhabricatorRepositoryCommit, array, PhabricatorMailTarget) called at [<phabricator>/src/applications/transactions/editor/PhabricatorApplicationTransactionEditor.php:2152]
Daemon 39827 STDE [Thu, 04 Jun 2015 03:03:11 +0000] #4 <#2> PhabricatorApplicationTransactionEditor::sendMail(PhabricatorRepositoryCommit, array) called at [<phabricator>/src/applications/transactions/editor/PhabricatorApplicationTransactionEditor.php:998]
Daemon 39827 STDE [Thu, 04 Jun 2015 03:03:11 +0000] #5 <#2> PhabricatorApplicationTransactionEditor::publishTransactions(PhabricatorRepositoryCommit, array) called at [<phabricator>/src/applications/transactions/worker/PhabricatorApplicationTransactionPublishWorker.php:21]
Daemon 39827 STDE [Thu, 04 Jun 2015 03:03:11 +0000] #6 <#2> PhabricatorApplicationTransactionPublishWorker::doWork() called at [<phabricator>/src/infrastructure/daemon/workers/PhabricatorWorker.php:91]
Daemon 39827 STDE [Thu, 04 Jun 2015 03:03:11 +0000] #7 <#2> PhabricatorWorker::executeTask() called at [<phabricator>/src/infrastructure/daemon/workers/storage/PhabricatorWorkerActiveTask.php:162]
Daemon 39827 STDE [Thu, 04 Jun 2015 03:03:11 +0000] #8 <#2> PhabricatorWorkerActiveTask::executeTask() called at [<phabricator>/src/infrastructure/daemon/workers/PhabricatorTaskmasterDaemon.php:20]
Daemon 39827 STDE [Thu, 04 Jun 2015 03:03:11 +0000] #9 PhabricatorTaskmasterDaemon::run() called at [<phutil>/src/daemon/PhutilDaemon.php:185]
Daemon 39827 STDE [Thu, 04 Jun 2015 03:03:11 +0000] #10 PhutilDaemon::execute() called at [<phutil>/scripts/daemon/exec/exec_daemon.php:125]
```
Auditors: btrahan
Fixes this, which only triggers on some kinds of mail:
```
Daemon 39819 STDE [Thu, 04 Jun 2015 02:56:38 +0000] [2015-06-04 02:56:38] EXCEPTION: (PhutilProxyException) Error while executing Task ID 902251. {>} (PhabricatorDataNotAttachedException) Attempting to access attached data on DifferentialRevision (via getActiveDiff()), but the data is not actually attached. Before accessing attachable data on an object, you must load and attach it.
Daemon 39819 STDE [Thu, 04 Jun 2015 02:56:38 +0000]
Daemon 39819 STDE [Thu, 04 Jun 2015 02:56:38 +0000] Data is normally attached by calling the corresponding needX() method on the Query class when the object is loaded. You can also call the corresponding attachX() method explicitly. at [<phabricator>/src/infrastructure/storage/lisk/PhabricatorLiskDAO.php:166]
Daemon 39819 STDE [Thu, 04 Jun 2015 02:56:38 +0000] arcanist(head=master, ref.master=8c589f1f759f), phabricator(head=master, ref.master=6dede2e2c513), phutil(head=master, ref.master=afc05a9a7f00)
Daemon 39819 STDE [Thu, 04 Jun 2015 02:56:38 +0000] #0 <#2> PhabricatorLiskDAO::assertAttached(string) called at [<phabricator>/src/applications/differential/storage/DifferentialRevision.php:158]
Daemon 39819 STDE [Thu, 04 Jun 2015 02:56:38 +0000] #1 <#2> DifferentialRevision::getActiveDiff() called at [<phabricator>/src/applications/differential/customfield/DifferentialBranchField.php:72]
Daemon 39819 STDE [Thu, 04 Jun 2015 02:56:38 +0000] #2 <#2> DifferentialBranchField::updateTransactionMailBody(PhabricatorMetaMTAMailBody, DifferentialTransactionEditor, array) called at [<phabricator>/src/applications/transactions/editor/PhabricatorApplicationTransactionEditor.php:2481]
Daemon 39819 STDE [Thu, 04 Jun 2015 02:56:38 +0000] #3 <#2> PhabricatorApplicationTransactionEditor::addCustomFieldsToMailBody(PhabricatorMetaMTAMailBody, DifferentialRevision, array) called at [<phabricator>/src/applications/differential/editor/DifferentialTransactionEditor.php:1208]
Daemon 39819 STDE [Thu, 04 Jun 2015 02:56:38 +0000] #4 <#2> DifferentialTransactionEditor::buildMailBody(DifferentialRevision, array) called at [<phabricator>/src/applications/transactions/editor/PhabricatorApplicationTransactionEditor.php:2178]
Daemon 39819 STDE [Thu, 04 Jun 2015 02:56:38 +0000] #5 <#2> PhabricatorApplicationTransactionEditor::sendMailToTarget(DifferentialRevision, array, PhabricatorMailTarget) called at [<phabricator>/src/applications/transactions/editor/PhabricatorApplicationTransactionEditor.php:2152]
Daemon 39819 STDE [Thu, 04 Jun 2015 02:56:38 +0000] #6 <#2> PhabricatorApplicationTransactionEditor::sendMail(DifferentialRevision, array) called at [<phabricator>/src/applications/transactions/editor/PhabricatorApplicationTransactionEditor.php:998]
Daemon 39819 STDE [Thu, 04 Jun 2015 02:56:38 +0000] #7 <#2> PhabricatorApplicationTransactionEditor::publishTransactions(DifferentialRevision, array) called at [<phabricator>/src/applications/transactions/worker/PhabricatorApplicationTransactionPublishWorker.php:21]
Daemon 39819 STDE [Thu, 04 Jun 2015 02:56:38 +0000] #8 <#2> PhabricatorApplicationTransactionPublishWorker::doWork() called at [<phabricator>/src/infrastructure/daemon/workers/PhabricatorWorker.php:91]
Daemon 39819 STDE [Thu, 04 Jun 2015 02:56:38 +0000] #9 <#2> PhabricatorWorker::executeTask() called at [<phabricator>/src/infrastructure/daemon/workers/storage/PhabricatorWorkerActiveTask.php:162]
Daemon 39819 STDE [Thu, 04 Jun 2015 02:56:38 +0000] #10 <#2> PhabricatorWorkerActiveTask::executeTask() called at [<phabricator>/src/infrastructure/daemon/workers/PhabricatorTaskmasterDaemon.php:20]
Daemon 39819 STDE [Thu, 04 Jun 2015 02:56:38 +0000] #11 PhabricatorTaskmasterDaemon::run() called at [<phutil>/src/daemon/PhutilDaemon.php:185]
Daemon 39819 STDE [Thu, 04 Jun 2015 02:56:38 +0000] #12 PhutilDaemon::execute() called at [<phutil>/scripts/daemon/exec/exec_daemon.php:125]
```
Auditors: btrahan
Summary: Ref T6367.
Test Plan:
- Added and executed unit tests.
- Sent mail to A (en_US) and B (en_A*).
- Got one mail in English and one mail in ENGLISH.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T6367
Differential Revision: https://secure.phabricator.com/D13142
Summary:
Ref T6367. Removes `multiplexMail()`!
We can't pass a single body into a function which splits it anymore: we need to split recipients first, then build bodies for each recipient list. This lets us build separate bodies for each recipient's individual translation/access levels.
The new logic does this:
- First, split recipients into groups called "targets".
- Each target corresponds to one actual mail we're going to build.
- Each target has a viewer (whose translation / access levels will be used to generate the mail).
- Each target has a to/cc list (the users who we'll ultimately send the mail to).
- For each target, build a custom mail body based on the viewer's access levels and settings (language prefs not actually implemented).
- Then, deliver the mail.
Test Plan:
- Read new config help.
Then did a bunch of testing, primarily with `bin/mail list-outbound` and `bin/mail show-outbound` (to review generated mail), `bin/phd debug taskmaster` (to run daemons freely) and `bin/worker execute --id <id>` (to repeatedly test a specific piece of code after identifying an issue).
With `one-mail-per-recipient` on (default):
- Sent mail to multiple users.
- Verified mail showed up in `mail list-outbound`.
- Examined mail with `mail show-outbound`.
- Added a project that a subscriber could not see.
- Verified it was not present in `X-Phabricator-Projects`.
- Verified it was rendered as "Restricted Project" for the non-permissioned viewer.
- Added a subscriber, then changed the object policy so they could not see it and sent mail.
- Verified I received mail but the other user did not.
- Enabled public replies and verified mail generated with public addresses.
- Disabld public replies and verified mail generated with private addresses.
With `one-mail-per-recipient` off:
- Verified that one mail is sent to all recipients.
- Verified users who can not see the object are still filtered.
- Verified that partially-visible projects are completely visible in the mail (this violates policies, as documented, as the best available compromise).
- Enabled public replies and verified the mail generated with "Reply To".
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: carlsverre, epriestley
Maniphest Tasks: T6367
Differential Revision: https://secure.phabricator.com/D13131
Summary:
Ref T6367. Do all mail, feed, notification and search stuff from the daemons, in all editors.
There are four relatively-stateful editors (Audit, Differential, Phriction, PhortuneCart) which needed special care to move state into the daemons properly.
Beyond that, I moved mailTo/mailCC/feedRelated/feedNotify to be computed before we enter the worker:
- This is simpler, since a lot of editors rely on being able to call `$object->getReviewers()` or similar to compute them.
- This is more correct, since we want to freeze the lists at this moment in time.
Finally, I renamed `loadEdges` to `willPublish` and made it a slightly more general hook.
---
This is a bit fragile and I'm not //thrilled// about it.
It would probably be cleaner to have separate Editor and Publisher classes (something like @fabe's D11329 did). However, I think that's quite a lot of work, and I'd like to see stronger motivation for it (either in this actually being more fragile than I think, or there being other things we get out of it). Overall, I'm comfortable with this change, just definitely not a big fan of the "save" + "load" pattern since I think it's really fragile, nonobvious, hard to debug/predict, etc.
Test Plan:
Directly updated editors:
- Created a new Phriction page, saw "Document Content".
- Edited a Phriction page, saw "Document Diff".
- Edited a revision, got normal looking mail.
- Faked in `changedPriorToCommitURI` and verified it survived the state boundary.
- Sent Audit mail.
- Sent invoice mail.
Indirect editors - for these, I just made a change and made sure the mail generated:
- Updated a paste.
- Updated an event.
- Updated a thread.
- Updated a task.
- Updated a mock.
- Updated a question.
- Updated a project.
- Updated a file.
- Updated an initiative.
- Updated a Legalpad document.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley, fabe
Maniphest Tasks: T6367
Differential Revision: https://secure.phabricator.com/D13115
Summary:
Ref T6367. This is similar to D11329, but not quite as ambitious.
Allow Editors to implement `supportsWorkers()` and move their publishing work into a daemon. So far, only Paste supports this.
Most of the complexity here is saving and restoring state across the barrier between the web process and the worker process, but I think this is ~90% of it and then we'll pick up a couple of random things in applications.
I'm primarily trying to keep this as gradual as possible.
Test Plan:
- Published transactions with and without daemon support.
- Looked at mail, feed.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: fabe, epriestley
Maniphest Tasks: T6367
Differential Revision: https://secure.phabricator.com/D13107
Summary:
Ref T7703. See that task and inline for a bunch of discussion.
Briefly, when we run implicit policy rules ("to see a revision, you must also be able to see its repository") at query time, they don't apply to other viewers we might check later.
We do this very rarely, but when we do we're often doing it for a bunch of different viewers (for example, in Herald) so I don't want to just reload the object a million times.
Test Plan:
- Added and executed unit tests.
- Wrote a "flag everything" Herald rule, as in the original report in T7703, and no longer got "Unknown Object" flags on revisions.
- Rigged up a lot of cases in the web UI and couldn't find any inconsistencies, although this case is normally very hard to hit.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T7703
Differential Revision: https://secure.phabricator.com/D13104
Summary:
Fixes T8387. This completes conversion of lists into users.
These settings allow administrators to reduce the amount of mail delivered to lists, ahead of T5791.
Test Plan: {F468206}
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T8387
Differential Revision: https://secure.phabricator.com/D13138
Summary: Ref T8387. Mostly completeness, but you might want to choose html vs text mail.
Test Plan: {F468203}
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T8387
Differential Revision: https://secure.phabricator.com/D13137
Summary:
Ref T8387. Ref T6367. This allows selection of a language, which will be respected in email delievered to the list users.
For example, you could have a German list that gets mail in German or something. I don't know that the feature is really useful, it's mostly just for completeness.
I also supported it for bots, mostly so their pronouns can be configured.
Test Plan: {F468186}
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T6367, T8387
Differential Revision: https://secure.phabricator.com/D13136
Summary: Ref T8377. These were picked up by tests.
Test Plan: `arc unit --everything`
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T8377
Differential Revision: https://secure.phabricator.com/D13130
Summary: Ref T8387. This is now completely obsoleted by mailing list users.
Test Plan: Grepped for `mailinglist` and related symbols.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: eadler, epriestley
Maniphest Tasks: T8387
Differential Revision: https://secure.phabricator.com/D13129
Summary: Ref T8387. Mostly just modernizes this panel to work for viewers vs users. Auto-verify these edits since they aren't otherwise verifiable.
Test Plan:
- Added, changed, removed addresses for a list.
- Used panel normally for my own account.
- Verified bots don't get a panel.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: eadler, epriestley
Maniphest Tasks: T8387
Differential Revision: https://secure.phabricator.com/D13125
Summary:
Ref T8387. Adds new mailing list users.
This doesn't migrate anything yet. I also need to update the "Email Addresses" panel to let administrators change the list address.
Test Plan:
- Created and edited a mailing list user.
- Viewed profile.
- Viewed People list.
- Searched for lists / nonlists.
- Grepped for all uses of `getIsDisabled()` / `getIsSystemAgent()` and added relevant corresponding behaviors.
- Hit the web/api/ssh session blocks.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: eadler, tycho.tatitscheff, epriestley
Maniphest Tasks: T8387
Differential Revision: https://secure.phabricator.com/D13123
Summary:
Ref T8387. This mostly merges D10565 + D10480. I'm going to touch this to add mailing list stuff shortly so I wanted to clean those up.
This isn't super pretty but is fully flexible and consistent with other modern query UIs.
This should be more-or-less backward compatible.
Test Plan: Fiddled with the new options.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: eadler, cburroughs, epriestley
Maniphest Tasks: T8044, T8387
Differential Revision: https://secure.phabricator.com/D13122
Summary:
Ref T8387. This describes changes I haven't made yet, but plan to make.
Also removes the long-deprecated actAsUser capability so I can remove the caveat about it from the documentation.
Test Plan: `grep`, reading
Reviewers: btrahan, eadler
Reviewed By: btrahan, eadler
Subscribers: eadler, epriestley
Maniphest Tasks: T8387
Differential Revision: https://secure.phabricator.com/D13120
Summary: This got removed by accident in D12933.
Test Plan: Editing names works now.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Differential Revision: https://secure.phabricator.com/D13144
Summary: When destroying an object, also remove all associated flags and tokens.
Test Plan: Awarded a Maniphest task with a token and flagged it as well. Destroyed it with `./bin/remove destroy` and saw flag and token removed from the database.
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: Korvin, epriestley
Differential Revision: https://secure.phabricator.com/D13112
Summary:
This simply adds colors, icons to ProjectQueryConduitAPIMethod and
ProjectCreateConduitAPIMethod.
Additionally, adds 'tags' to ProjectCreateConduitAPIMethod
Change-Id: Ic6332fe174a59ecfd60cea281ccb0ed938136141
Test Plan:
create a new project with project.create, specify icon, color and tags
call project.query with colors and icons args, check for results
containing matching projects.
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: legoktm, aklapper, Korvin, epriestley, valhallasw, qgil
Differential Revision: https://secure.phabricator.com/D13098
Summary: Ref T8099, I want to live with this for a few weeks. I've grow to like that it gives more distinction to ObjectBox Headers and fits very nicely as headers in DocumentView. Even if we don't use them for all headers, Documents are much nicer to me at least with a more visually impactful font (Source Sans doesn't make a great header).
Test Plan:
Review dashboards, objects, Diviner, Phriction, Legalpad, and misc other pages.
{F466410}
Reviewers: btrahan, epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T8099
Differential Revision: https://secure.phabricator.com/D13127
Summary: Closes T8371, Query for recurring event parents should accept a "not a child of anything" clause
Test Plan: querying for a list of events with recurring events should still work the same.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: Korvin, epriestley
Maniphest Tasks: T8371
Differential Revision: https://secure.phabricator.com/D13140
Summary: This class is no longer used since D10792.
Test Plan: `grep`
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: Korvin, epriestley
Differential Revision: https://secure.phabricator.com/D13116
Summary: Ref T8371, Cancelled recurring events should propogate to real child events
Test Plan: Create recurring event, create and exception to a ghost event, cancel recurring event, real ghost event should be treated as cancelled while the recurring event remains cancelled.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: Korvin, epriestley
Maniphest Tasks: T8371
Differential Revision: https://secure.phabricator.com/D13121
Summary: Ref T8394, List view should only allow 100 ghost events per recurring event
Test Plan: Open list view with many recurring events. Shouldn't hang.
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T8394
Differential Revision: https://secure.phabricator.com/D13132
Summary: Ref T8099, Moves AphrontPagerView to PHUIPagerView, converts to standard PHUIButtons and adds some additional features for icon placement on buttons.
Test Plan: Tested Advanced Search and Searching files in Diffusion. Works as expected.
Reviewers: btrahan, epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T8342, T8099
Differential Revision: https://secure.phabricator.com/D13092
Summary: This class is no longer used.
Test Plan: `grep`
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: Korvin, epriestley
Differential Revision: https://secure.phabricator.com/D13117
Summary: This class is no longer used after D13032.
Test Plan: `grep`
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: Korvin, epriestley
Differential Revision: https://secure.phabricator.com/D13118
Summary: This class is no longer used after D12850.
Test Plan: `grep`
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: Korvin, epriestley
Differential Revision: https://secure.phabricator.com/D13119
Summary: Fixes T8382, Edit link of an exception to a ghost event should not create new event
Test Plan: Create recurring event, open ghost, edit, save, edit again. Should edit event created after first edit, not new event.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: Korvin, epriestley
Maniphest Tasks: T8382
Differential Revision: https://secure.phabricator.com/D13108
Summary: Now that `ManiphestTask` implements `PhabricatorApplicationTransactionInterface`, the transaction removal happens automatically.
Test Plan: Used `./bin/remove destroy` to delete a `ManiphestTask` and saw related transactions removed as well.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: Korvin, epriestley
Differential Revision: https://secure.phabricator.com/D13110
Summary: Ref T8357, DRAFT, recurring events need optional end dates
Test Plan: Edit recurring event, set end date, save, recurring ghosts should not generate after end date
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T8357
Differential Revision: https://secure.phabricator.com/D13088
Summary: got some errors in my sandbox when I updated. this fixes them and I think is the right naming convention.
Test Plan: /settings/panel/emailpreferences/ started working again! /spaces/ also loaded unit tests passed without errors
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T8377
Differential Revision: https://secure.phabricator.com/D13103
Summary: Ref T8099, Mostly a Feed cleanup, removing old CSS, relying on modern display objects, adds back the feed to profile (I miss it, but maybe you don't).
Test Plan: Visit Feed on Profiles, Projects, Feed, and Dashboards. Same UI Everywhere. TODO, "Public Feed".
Reviewers: btrahan, epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T8099
Differential Revision: https://secure.phabricator.com/D13101
Summary:
Fixes T8372. We currently store the entire resource path in the cachekey; shorten it to a maximum of 64 characters.
The key size is 128 and T8372 had an example of a 112-byte key which could run over 128 with a slightly-long cache namespace.
Test Plan:
- Not 100% sure this is the issue, but it seems likely.
- Dumped the cachekey before and after the change.
- Saw values like `celerity:/res/1432859262T/phabricator/e2460e8f/core.pkg.css-6eGjUCJORU3j` now.
Reviewers: btrahan, chad
Reviewed By: chad
Subscribers: epriestley
Maniphest Tasks: T8372
Differential Revision: https://secure.phabricator.com/D13095
Summary: Fixes T7603. This application is no longer used.
Test Plan: Went to `/repository/` and got a 404.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: epriestley
Maniphest Tasks: T7603
Differential Revision: https://secure.phabricator.com/D12734
Summary: Fixes T8369, Both ghost instances and exceptions to generated ghosts should be editable
Test Plan: Create recurring event, open /E{id}/1, edit, save. New exception should not be "recurring", and should be editable, as the regular ghost events.
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T8369
Differential Revision: https://secure.phabricator.com/D13089
Summary: Ref T8099, We can just set the no data string on the ObjectList directly.
Test Plan: Run a bum query, see better UI.
Reviewers: btrahan, epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T8099
Differential Revision: https://secure.phabricator.com/D13087
Summary: Ref T7604. Remove these "arcanist project" fields from the `LiskDAO` classes, but leave the data intact (there are no more read/writes to this data).
Test Plan:
- Grepped for calls to these methods.
- Ran `./bin/storage adjust` to check for schema issues.
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T7604
Differential Revision: https://secure.phabricator.com/D13012
Summary: I think the language here is inverted? On a commit which is reverted, the text states `$author added a reverted commit: $commit`. In reality, `$commit` is the //reverting// commit, not the //reverted// commit.
Test Plan: Think about English.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: Korvin, epriestley
Differential Revision: https://secure.phabricator.com/D13067
Summary: Closes T8358, Edit recurring ghost events, not recurring event.
Test Plan: Create recurring event, original event should not be editable, but ghost events should be editable.
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T8358
Differential Revision: https://secure.phabricator.com/D13075
Summary: Ref T8099, Adds a bit more color/contract to Paste.
Test Plan: Review a list of Pastes, embedded Pastes, and the main Paste UI.
Reviewers: btrahan, epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T8099
Differential Revision: https://secure.phabricator.com/D13083
Summary: Ref T8099, Adds ability to set header as "tall" and provide better control over height of actual header text. Also fixed some color issues with multiple object boxes.
Test Plan: Review a commit, review dashboards, review a diff, review home.
Reviewers: btrahan, epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T8099
Differential Revision: https://secure.phabricator.com/D13077
Summary: Ref T8099. Makes subheaders in object lists darker with a background. Moves to lighter icons in the header with a dark instead of blue hover state. Gives a nicer blue selected state for sidenavs.
Test Plan: Just eyeballing a lot of this, but with some level of consistency.
Reviewers: btrahan, epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T8099
Differential Revision: https://secure.phabricator.com/D13065
Summary: Ref T8099, No specific reason for these to be small buttons.
Test Plan: Test desktop and mobile layouts.
Reviewers: btrahan, epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T8099
Differential Revision: https://secure.phabricator.com/D13081
Summary: Ref T8099, Adds a border to ConpherenceView, but not other Views. Removed some old styles.
Test Plan: Visit Conpherence Room, Thread, and Search.
Reviewers: btrahan, epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T8099
Differential Revision: https://secure.phabricator.com/D13062
Summary: If a host artifact exists in a Harbormaster build, where the Drydock lease no longer exists, then an error will be raised because of the attempt to access an undefined index. This changes the code to use `idx()` so that it correctly returns null instead.
Test Plan: Tested in production.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: joshuaspence, Korvin, epriestley
Differential Revision: https://secure.phabricator.com/D13074
Summary:
Ref T7984. With this, an install can add an ExternalSymbolsSource to src/extensions, which will include whatever
source they have.
Test Plan: search for php and python builtins.
Reviewers: joshuaspence, epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: Korvin, epriestley
Maniphest Tasks: T7984
Differential Revision: https://secure.phabricator.com/D13036
Summary: Fixes T8347, Generated ghost events should start generating as early as the range start minus the duration of the event.
Test Plan: Create a year long weekly event, a year later, each day should have about 52 events.
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T8347
Differential Revision: https://secure.phabricator.com/D13069
Summary: Closes T8356, Ghost events should provide a link to original event
Test Plan: Open an instance at /calendar/event/{id}/{index}. It should have a link to /calendar/event/{id}. Original recurring event should not link to itself.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: Korvin, epriestley
Maniphest Tasks: T8356
Differential Revision: https://secure.phabricator.com/D13068
Summary: Fixes T8329. I was able to figure out a reasonable way to have the full conpherence default to the email settings panel. I think this is cleaner than making things a dialogue as I rambled about in the description for T8329.
Test Plan: using /bin/mail to verify correct email links were generated for conpherence notifications and maniphest (general) notifications.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T8329
Differential Revision: https://secure.phabricator.com/D13058
Summary:
Fixes T8154. Now you can tell when a Conpherence was started by mousing over the date. I think this is sufficient.
This ends up cleaning up the code significantly; it used to inherit from some transaction view class and had built up lots of cludge over time. Much cleaner now.
Test Plan: moused over tooltips in both durable column and full conpherence and saw correct full dates. clicked on a timestamp in durable column and went to right place in full conpherence.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T8154
Differential Revision: https://secure.phabricator.com/D13057
Summary: Ref T8099, Fixes T8341. Switches all remaining callsites to setBarColor to use setStatusIcon (sans workboards).
Test Plan: Test each of the applications I changed as I could (not Releeph).
Reviewers: btrahan, epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T8341, T8099
Differential Revision: https://secure.phabricator.com/D13059
Summary: Ref T8099, adds StatusIcons in place of barColor. May need to revisit icons. Also fixed incorrect icons used in Drydock.
Test Plan: Visit Harbormaster, Drydock, see proper icons.
Reviewers: btrahan, epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T8099
Differential Revision: https://secure.phabricator.com/D13054
Summary: Ref T8099, minor spacing improvements and more icons for feed.
Test Plan:
Test feed in Feed, in a Dashboard, Profile, and Project
{F443366}
Reviewers: btrahan, epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T8099
Differential Revision: https://secure.phabricator.com/D13045
Summary: Ref T8099. I use this app daily, so 30 minutes of love is good enough for me.
Test Plan:
Tested with a fake chat.
{F443522}
Reviewers: btrahan, epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T8099
Differential Revision: https://secure.phabricator.com/D13046
Summary: Ref T8099, Sets a StatusIcon instead of a barColor on config status lists.
Test Plan: Review ignored and non-ignored status items.
Reviewers: btrahan, epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T8341, T8099
Differential Revision: https://secure.phabricator.com/D13050
Summary: Ref T8099, updates table layouts and StatusIcons in the Daemons application.
Test Plan: Click on lots of different things in Daemons.
Reviewers: btrahan, epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T8099
Differential Revision: https://secure.phabricator.com/D13052
Summary: Ref T8099, Updates Drydock with new StatusIcon over barColor. Making a guess on best icons, feel free to change.
Test Plan: Review Drydock UI in sandbox.
Reviewers: btrahan, epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T8341, T8099
Differential Revision: https://secure.phabricator.com/D13053
Summary: These could cause F442758.
Test Plan: {F442779}
Reviewers: btrahan, joshuaspence
Reviewed By: joshuaspence
Subscribers: epriestley
Differential Revision: https://secure.phabricator.com/D13042
Summary: Ref T8099. Adds back basic header color options, which change the logo color instead. Also RAINBOW.
Test Plan:
tested each of the new colors.
{F442284}
Reviewers: btrahan, epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T8099
Differential Revision: https://secure.phabricator.com/D13040
Summary: Ref T8099, I failed to test "Advanced Search" which disappeared during the last commit.
Test Plan: Test built-in, saved, and advanced search filters.
Reviewers: btrahan, epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T8099
Differential Revision: https://secure.phabricator.com/D13035
Summary: Ref T8026, Create button should be a dropdown with public and private options
Test Plan: Create both a public and private event. Confirm view policies apply correctly.
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T8026
Differential Revision: https://secure.phabricator.com/D13033
Summary:
Ref T8320. Fixes T8317. Fixes T2831. Fixes T8073. Fixes T7127.
There was a bug with this line:
for ($ii = 0; $ii < count($paths); $ii++) {
...because the array may be sparse if there have been deletes, so `count($paths)` might be 3, but the real keys could be `1`, `5` and `6`. I think this was the primary issue behind T7127.
The old Editor did a lot of work to try to validate paths. When a path failed to validate, it silently discarded it. This was silly and pointless: it's incredibly bad UX; and it's totally fine if users saves "invalid" paths. This was likely the cause of T8317, and probably the cause of T8073.
T2831 I'm less sure about, but I can't reproduce it and I rewrote all the logic so I suspect it's gone.
This also records and shows edits, so if stuff does keep happening it should be more clear what's going on.
I removed some adjacent stuff:
- I removed the ability to delete packages. I'll add "disable" in a future diff, plus `bin/remove destroy`, like other objects. Getting rid of this now let me get rid of all the mail stuff.
- I removed "path validation" where packages would try to automatically update in response to commits. This doesn't necessarily make sense in Git/Mercurial, is sketchy, could easily have been the source of T2831, and seems generally complicated and not very valuable. We could maybe restore it some day, but I'd like to get Owners stable before trying to do crazy stuff like that.
Test Plan: {F437687}
Reviewers: chad, btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T8317, T8073, T7127, T2831, T8320
Differential Revision: https://secure.phabricator.com/D13032
Summary:
Ref T8320. There's currently one enormous form; split it into a general information form (name, description, owners) and a paths form.
I think this is a little more manageable from both a UX point of view and from an "I have to convert this to use ApplicationTransactions" point of view.
Test Plan:
- Edited paths.
- Edited non-path information.
- Created new packages.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T8320
Differential Revision: https://secure.phabricator.com/D13026
Summary:
Ref T8320. Modernize search and major interfaces.
This slightly regresses some list view and search features; I'll probably restore some later (once the Query has proper `needX(...)` methods) and drop the rest.
Test Plan: Browsed, edited, deleted, and created packages.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T8320
Differential Revision: https://secure.phabricator.com/D13024
Summary:
Ref T7447. Implements per-viewer comment hiding. Once a comment is obsolete or uninteresting, you can hide it completely.
This is sticky per-user.
My hope is that this will strike a better balance between concerns than some of the other approaches (conservative porting, summarization, hide-all).
Specifically, this adds a new action here:
{F435621}
Clicking it completely collapses the comment into a small icon on the previous line, and saves the comment state as hidden for you:
{F435626}
You can click the icon to reveal all hidden comments below the line.
Test Plan:
- Hid comments.
- Showed comments.
- Created, edited, deleted and submitted comments.
- Used Diffusion comments (hiding is not implemented there yet, but I'd plan to bring it there eventually if it works out in Differential).
Reviewers: btrahan, chad
Reviewed By: btrahan
Subscribers: jparise, yelirekim, epriestley
Maniphest Tasks: T7447
Differential Revision: https://secure.phabricator.com/D13009
Summary:
Ref T8238. This allows configuration of a "staging area" for Git repositories, which is the URI to some Git repository (possibly the same repository).
If a staging area is configured, `arc` will push a copy of anything it creates a diff for there (see next revision). This primarily makes handoff to build systems easier.
This is a bit leaky and I intend for it to eventually be positioned as a less-preferred solution, but from the perspective of build systems it's the same as the real (virtual ref) solution that I want to build.
Test Plan: Ran `arc diff` with various flags, saw appropriate changes copied into the staging area. See also discussion in T8238.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: cburroughs, epriestley
Maniphest Tasks: T8238
Differential Revision: https://secure.phabricator.com/D13019
Summary: Ref T8099, adds 'show/hide' functions to PHUIObjectBoxView, rolls them out in Application Search for trial.
Test Plan:
Test doing a couple searches in Projects, Audit, etc. Seems to work ok. Design might need more?
{F437207}
Reviewers: btrahan, epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T8099
Differential Revision: https://secure.phabricator.com/D13027
Summary: Fixes T8147, All day events should obey selected query range in viewer timezone.
Test Plan: Create all day event May 25, query for events May 26-27. All day event should not be part of the query results list.
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T8147
Differential Revision: https://secure.phabricator.com/D13023
Summary:
We don't want to support this right now, so disable it, similar to "Land To Hosted" feature.
Keep to the code as an example for advanced installs.
Ref T182, T8313
Test Plan: load a diff.
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: epriestley
Maniphest Tasks: T182, T8313
Differential Revision: https://secure.phabricator.com/D13022
Summary: Ref T7603. Ref T7604. There is no need for these controllers anymore as the "Arcanist Project" data is not used anywhere. Depends on D12894 and D12898.
Test Plan: Went to `/repository/project/edit/2/` and got a 404.
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T7603, T7604
Differential Revision: https://secure.phabricator.com/D12897
Summary: Ref T7604. Remove arcanist projects from #releeph.
Test Plan: I don't really know how to use Releeph but I clicked around and nothing seemed too broken.
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T7604
Differential Revision: https://secure.phabricator.com/D12898
Summary:
- Give the fields names and descriptions.
- When new, default-disabled fields are added, disable them by default even if there's already a config.
- Be a bit less hacky about `$faux_spec`.
Test Plan: {F432383}
Reviewers: joshuaspence, fabe
Reviewed By: fabe
Subscribers: epriestley
Differential Revision: https://secure.phabricator.com/D13006
Summary: Ref T7604. Mark the `arcanist.projectinfo` #conduit method as deprecated.
Test Plan: Went to the `/config/issue/` page and saw the `Deprecated Conduit Method "arcanist.projectinfo" In Use` setup issue.
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T7604
Differential Revision: https://secure.phabricator.com/D12970
Summary: This is vaguely related to T5791. Add a "REPOSITORY" header to audit emails so that they can be filtered in Gmail.
Test Plan: Commented on an audit and used `./bin/mail show-outbound` to inspect outbound email.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: fabe, Korvin, epriestley
Differential Revision: https://secure.phabricator.com/D12997
Summary:
Includes a new Block in Herad emails which tells the user about which commits were merged in a merge commit
Otherwise the email would just say "Merge branch XYZ". Ref T8295
Test Plan: imported various commits (and merges) and watched resulting herald emails for all of them
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T8295
Differential Revision: https://secure.phabricator.com/D12993
Summary: These format strings use `%d` instead of `%s`.
Test Plan: Eyeball it.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: Korvin, epriestley
Differential Revision: https://secure.phabricator.com/D12996
Summary: Ref T7604. Add `encoding` information to the `repository.query` #conduit query.
Test Plan: Changed repository encoding from the UI and saw the encoding returned by the Conduit call.
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T7604
Differential Revision: https://secure.phabricator.com/D12992
Summary:
Make Herald conditions and actions more resilient (see discussion in D12896). This protects against invalid rules, which may have been valid in the past but are no longer valid. Specifically:
- If a rule has an invalid field, the conditions fail and the actions do not execute.
- The transcript shows that the rule failed because of an invalid field, and points at the issue.
- If a rule has an invalid action, that action fails but other actions execute.
- The transcript shows that the action failed.
- Everything else (particularly, other rules) continues normally in both cases.
- The edit interface is somewhat working when editing an invalid rule, but it could use some further improvements.
Test Plan:
# Ran this rule on a differential revision and saw the rule fail in the transcript.
# Was able to submit a differential without receiving an `ERR-CONDUIT-CORE`.
# Edited the Herald rule using the UI and was able to save the rule succesfully.
# Ran this rule on a differential revision and saw one success and one failure in the transcript.
# Was able to submit a differential without receiving an `ERR-CONDUIT-CORE`.
# Edited the Herald rule using the UI. Clicking save caused a `HeraldInvalidActionException` to be thrown, but maybe this is okay.
Differential Revision: http://phabricator.local/D41
Summary: Ref T8300, clicking in day view should create new event
Test Plan: Open day view, click in an empty slot, new event modal should open.
Reviewers: epriestley, #blessed_reviewers, chad
Reviewed By: epriestley, #blessed_reviewers
Subscribers: Korvin, epriestley
Maniphest Tasks: T8300
Differential Revision: https://secure.phabricator.com/D12990
Summary:
Ref T8099. In most cases we return either an ObjectList or AphrontTable, and can pretty up the UI in ApplicationSearch. There are a few edge cases, like PeopleUserLog, that can be cleanup up individually in the future, but look fine for now.
Also added 'setNotice' for AphrontTable for a few cases where we want to convey addtional information.
TODO: Seems we always pass a Pager Object, which tries to get displayed, I'll redesign that interaction in the future, probably by passing the Pager to the ObjectBox
Test Plan: Went throught most/all ApplicationSearch panels I could find, even edge cases look better.
Reviewers: btrahan, epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T8099
Differential Revision: https://secure.phabricator.com/D12989
Summary: Ref T8300, Rescheduling events by dragging them in day view
Test Plan: Open day view, drag events, observe them reschedule.
Reviewers: chad, epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: Korvin, epriestley
Maniphest Tasks: T8300
Differential Revision: https://secure.phabricator.com/D12988
Summary:
In the great `pht()` conversion, some strings like "123,456" are now being printed as numbers with "%d". These come out as "123" instead of "123,456".
Use "%s" and "PhutilNumber" to present numbers with comma groupings.
Test Plan:
- Viewed DarkConsole.
- Viewed conduit logs.
- Viewed daemon logs.
- Grepped for `%d ms` and `%d us`.
Reviewers: btrahan, joshuaspence
Reviewed By: joshuaspence
Subscribers: epriestley
Differential Revision: https://secure.phabricator.com/D12979
Summary: Ref T8099, Quick descent pass at making header, object lists, tables, filter view, mobile friendly.
Test Plan:
Test home, differential diff, maniphest task, new task, search, and a few other views.
{F414034}
Reviewers: btrahan, epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T8099
Differential Revision: https://secure.phabricator.com/D12984
Summary: Ref T8099, I //think// this was just a bum merge, but maybe I missed a commit too.
Test Plan: Poked around a number of new headers and styles, uiexamples, dialogs, etc.
Reviewers: btrahan, epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T8099
Differential Revision: https://secure.phabricator.com/D12982
Summary: Ref T8099, minor adds back the border, makes blue highlight {$blue}
Test Plan: Hover over new crumbs, match header hovers. Check Phriction for border
Reviewers: btrahan, epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T8099
Differential Revision: https://secure.phabricator.com/D12959
Summary: Ref T8099, adds an action icon similar to dashboards for taking people to the search.
Test Plan: Click on each icon, verify it takes me to correct URL.
Reviewers: btrahan, epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T8099
Differential Revision: https://secure.phabricator.com/D12974
Summary: Closes T8041, Calendar query results should be ordered from first start date to last start date
Test Plan: Open "Upcoming Events" in Calendar, verify that the event with the first start date is first and that events are ordered in ascending start dates.
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T8041
Differential Revision: https://secure.phabricator.com/D12968
Summary: Fixes T8277. Transforming files can race; resolve the race after we lose.
Test Plan:
- Added `sleep(10)` near the bottom of the transform controller.
- Transformed a file in two browser windows at the same time; got something like this (exception corresponds to the loser of the race):
{F412526}
- Applied patch.
- Repeated process, got this:
{F412527}
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T8277
Differential Revision: https://secure.phabricator.com/D12965
Summary: Fixes T8283. The code in `FileController` assumes this value is an array.
Test Plan: Loaded any file in Diffusion in unconfigured repository, no more warning.
Reviewers: avivey, joshuaspence, btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T8283
Differential Revision: https://secure.phabricator.com/D12964
Summary:
fixes T8260. Only turn on symbol links if:
- The repository has any configuration about symbols, or
- There actually are symbols in the repository.
Test Plan: Look at revisions and files in various states of configurations and having symbols.
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: joshuaspence, Korvin, epriestley
Maniphest Tasks: T8260
Differential Revision: https://secure.phabricator.com/D12946
Summary: Ref T8031, Calendar event datepicker should auto-complete end time to be an hour from start time unless end time has been edited
Test Plan: Create event, edit start time, end time should adjust to an hour from start time.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: Korvin, epriestley
Maniphest Tasks: T8031
Differential Revision: https://secure.phabricator.com/D12960
Summary: Ref T8099, this removes the gradient headers and their edge cases in Pinboard View, Setup Issues, Exceptions, and Document Heiriarchy.
Test Plan:
Tested each of these, grep for "gradient" and verify all states are gone.
{F410708}
{F410709}
{F410710}
{F410714}
Reviewers: btrahan, epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T8099
Differential Revision: https://secure.phabricator.com/D12944
Summary: Fixes T8274. That report is very light on details, but I think the issue is that their Elasticsearch is misconfigured and the new setup warning dies a little too hard if the server is just completely dead.
Test Plan: Set `search.elastic.host` to an invalid server, got a similar-looking exception (?), applied patch, got a setup warning instead.
Reviewers: btrahan, joshuaspence
Reviewed By: btrahan, joshuaspence
Subscribers: epriestley
Maniphest Tasks: T8274
Differential Revision: https://secure.phabricator.com/D12948
Summary: Fixes T5703. These have been unused in production for a while and the new stuff seems good.
Test Plan: Mostly `grep`.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T5703
Differential Revision: https://secure.phabricator.com/D12949
Summary:
Make sure we're 100% clear that this is really, truly not recommended.
Also improve the text itself, and show the objects which are being destroyed more clearly.
Test Plan: Removed objects with `bin/remove destroy`.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Differential Revision: https://secure.phabricator.com/D12952
Summary: Fixes T6403. Remaining built-ins were already built-in effectively so this is a small re-factor plus some docs. I probably wouldn't have written anything if not for the TODO so please feel free to tell me to write something else or what have you.
Test Plan: NA since this didn't actually change anything.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T6403
Differential Revision: https://secure.phabricator.com/D12937
Summary: Converts most all tables to be directly set via `setTable` to an ObjectBox. I think this path is more flexible design wise, as we can change the box based on children, and not just CSS. We also already do this with PropertyList, Forms, ObjectList, and Header. `setCollapsed` is added to ObjectBox to all children objects to bleed to the edges (like diffs).
Test Plan: I did a grep of `appendChild($table)` as well as searches for `PHUIObjectBoxView`, also with manual opening of hundreds of files. I'm sure I missed 5-8 places. If you just appendChild($table) nothing breaks, it just looks a little funny.
Reviewers: epriestley, btrahan
Subscribers: Korvin, epriestley
Differential Revision: https://secure.phabricator.com/D12955
Summary: Ref T8031, Make default start value of new events to be the next whole hour and the end value to be the next whole hour after that.
Test Plan: Create new event at, say, 10:30am. Event default times should be 11:00 AM - 12:00 PM
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: Korvin, epriestley
Maniphest Tasks: T8031
Differential Revision: https://secure.phabricator.com/D12954
Summary: This data is available and loaded, just not used.
Test Plan: {F411442}
Reviewers: chad
Reviewed By: chad
Subscribers: epriestley
Differential Revision: https://secure.phabricator.com/D12951
Summary: Ref T8099, this removes the gradient headers and their edge cases in Pinboard View, Setup Issues, Exceptions, and Document Heiriarchy.
Test Plan:
Tested each of these, grep for "gradient" and verify all states are gone.
{F410708}
{F410709}
{F410710}
{F410714}
Reviewers: btrahan, epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T8099
Differential Revision: https://secure.phabricator.com/D12944
Summary: Closes T7936, Calendar event icons show up on month and day views
Test Plan: Calendar month and day views should show event icons
Reviewers: chad, #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T7936
Differential Revision: https://secure.phabricator.com/D12943
Summary: Removes setting colors on the headers and adds setting colors on an ObjectBoxView (which sets the headers). Ref T8099
Test Plan:
Tested each color, fixed workboard colors, added color to important setup issues.
{F410658}
{F410659}
Reviewers: btrahan, epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T8099
Differential Revision: https://secure.phabricator.com/D12942
Summary: Fixes T8264. Broken in D12929. Sweep all the applyBuiltin implementations and always break; rather than return
Test Plan: added myself to a project successfully (showed up as a member)
Reviewers: epriestley, chad
Subscribers: Korvin, epriestley
Maniphest Tasks: T6403, T8264
Differential Revision: https://secure.phabricator.com/D12940
Summary: Ref XXXXX. I broke things a bit in XXXXX in that if the TYPE_EDGE had an inverse transaction, we weren't correctly "doing nothing" and instead were falling back to our old every editor has to implement a no-op ways... Fix things by putting the TYPE_EDGE code in the handle external builtin function like it belongs.
Test Plan: made a comment on a task referencng a commit successfully
Reviewers: epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T6403
Differential Revision: https://secure.phabricator.com/D12939
Summary: Remove the `PhabricatorDefaultSearchEngineSelector` class. This is quite similar to D12053.
Test Plan: Went to `/view/PhabricatorSearchApplication/` and saw the storage engine configuration. Set `search.elastic.host` and saw the highlighted storage engine change.
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: Korvin, epriestley
Differential Revision: https://secure.phabricator.com/D12670
Summary: Fixes T8251. I think this used to be "lucky handle" when it was all one giant terrible function, so restore previous functionality and have the title be the viewer's name in this case.
Test Plan: rocked a solo conpherence and saw my username as the title
Reviewers: chad, epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T8251
Differential Revision: https://secure.phabricator.com/D12935
Summary: Ref T6403. This was actually simple stuff.
Test Plan: changed the edit policy of a paste. changed the edit and join policy of a phame blog.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T6403
Differential Revision: https://secure.phabricator.com/D12933
Summary: Ref T6403. Conpherence keeps track of comments for message counts so we needed some special attention there. Otherwise, straight-forward.
Test Plan: left a comment on a diff with inline comments. sent messages in conpherence successfully. verified unread count incremented correctly for sent messages for users.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T6403
Differential Revision: https://secure.phabricator.com/D12932
Summary:
Fixes T8150. Small generalization:
- Timeline has a dropdown with no special logic; make that generic.
- Use it in Phriction.
Test Plan:
- Used Phriction dropdown on desktop and mobile.
- Used timeline dropdown.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T8150
Differential Revision: https://secure.phabricator.com/D12931
Summary: Ref T6403. This one was pretty easy since no one does anything custom with subscribers.
Test Plan: subscribed / unscribed to a random commit ("audit"). joined / left, watched / unwatched a project
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T6403
Differential Revision: https://secure.phabricator.com/D12930
Summary: Ref T6403. This does TYPE_EDGE since I just had to deal with T8252. Look like this fixes a few editors (maybe) that would have had fatals with mentions like slowvote and ponder.
Test Plan: made a phame post mentioning a task and it worked! joined / left a project, watched / unwatched a project and that worked! blind faith for other sites.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T6403
Differential Revision: https://secure.phabricator.com/D12929
Summary: Fixes T8252. See T6403 for longer term fix.
Test Plan: made a post with a task mention in it and it worked
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T8252
Differential Revision: https://secure.phabricator.com/D12926
Summary:
Changed "Active Prodcuts" to "Active Products" on Releeph Product Query dropdown.
(Incidentally, this is my first diff on Phabricator proper, so I hope I got it right. I'm used to SVN, not Git.)
Test Plan: Make sure the change is there and didn't bork anything else.
Reviewers: chad, #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: Korvin, epriestley
Differential Revision: https://secure.phabricator.com/D12904
Summary: Use a consistent blue across all anchors. Slightly lighter than current blue. Ref T8099
Test Plan: Visit Phriction, Remarkup, various apps.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T8099
Differential Revision: https://secure.phabricator.com/D12918
Summary: Fixes T8242. D12833 removed the title as well as the blurb from people hovercards. When re-adding the title don't bother throwing things through pht since that seems like not something you translate exactly and also lose the flavor text which most users end up having since title is rarely set (at least on this install).
Test Plan: viewed hovercards and saw title and blurb again as appropos relative to the data being set
Reviewers: chad, epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T8242
Differential Revision: https://secure.phabricator.com/D12915
Summary: Fixes T8217. Broken circa D12760. Some browsers rendered nothing while others rendered a ",".
Test Plan: added folks to conpherences and saw people widget update correctly. added files to conpherence and saw file widget update correctly. Tested on Chrome and Firefox.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T8217
Differential Revision: https://secure.phabricator.com/D12912
Summary: Fix some method signatures so that arguments with default values are at the end of the argument list (see D12418).
Test Plan: Eyeballed the callsites.
Reviewers: epriestley, #blessed_reviewers, hach-que
Reviewed By: epriestley, #blessed_reviewers, hach-que
Subscribers: hach-que, Korvin, epriestley
Differential Revision: https://secure.phabricator.com/D12782
Summary:
Fixes T8230. I think I broke this in D12354 since I missed the fact that this could filter results, because it's constructed in a sort of weird way. Try to make the logic a little more clear, while retaining the relevant properties:
- Pastes with unloadable content should be discarded;
- the input order should be retained in the result set.
Test Plan: Faked content misses and saw things work instead of fatal.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T8230
Differential Revision: https://secure.phabricator.com/D12909
Summary:
Fixes T7923.
Prevent the user from finding tasks that they can't edit in merge workflows. Also ensure that we query properly on final merge action just in case.
Test Plan: Tried to find a task I couldn't edit in various searches under the "merge" dialogue and couldn't find the task. Removed this big of code and tried to merge in a task and after hitting "merge" observed the page reloaded with no task merged in.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T7923
Differential Revision: https://secure.phabricator.com/D12872
Summary: Fixes T8237. This table is unusual and doesn't have an `id` column, so `delete()` doesn't actually know how to delete records and fails.
Test Plan:
- Used `bin/remove destroy` to destroy an object with notifications, as per T8237.
- Applied patch.
- Used `bin/remove destroy` to get a clean delete.
- Verified related notifications vanished from notification menu.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T8237
Differential Revision: https://secure.phabricator.com/D12907
Summary: Ref T7769. It might possibly make sense to generate these some day, but for now they aren't payable and we have no use for them.
Test Plan:
- Generated $0.00 invoices, saw code decline to invoice.
- Generated a $7.99 invoice, got an invoice.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T7769
Differential Revision: https://secure.phabricator.com/D12878
Summary: This adds minor functionality to PHUIHeaderView (icons, tags) that were previously on in ActionHeader. This allows us to remove the PHUIActionHeaderView outright in favor of just one Header class.
Test Plan: Tested each callsite, workboards, hovercards, conpherence.
Reviewers: btrahan, epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Differential Revision: https://secure.phabricator.com/D12902
Summary: Ref T7604. Change `DiffusionLintSaveRunner` to use repositories instead of Arcanist Projects.
Test Plan: Ran the `save_lint.php` script and queried results using the `diffusion.getlintmessages` Conduit endpoint.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: Korvin, epriestley
Maniphest Tasks: T7604
Differential Revision: https://secure.phabricator.com/D12893
Summary:
Fixes T7977.
- Move Indexed Languages and See Symbols From config to Repository
- Make symbol search skip projects
This also makes the default languages to Everything instead of Nothing.
Test Plan:
- Browse files, click symbols.
- Use quick search to find symbols
- Browse revision, click symbols
Reviewers: joshuaspence, epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: Korvin, epriestley
Maniphest Tasks: T7977
Differential Revision: https://secure.phabricator.com/D12687
Summary: Fixes T8215. When adding a revert edge, only consider commits within the same repository.
Test Plan: Imported the arcanist repository three times and inspected the reverts relationships on rARC490984936bb7ddfc589ef3d376ec2ac2365f070d.
Reviewers: hach-que, epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: Korvin, epriestley
Maniphest Tasks: T8215
Differential Revision: https://secure.phabricator.com/D12875
Test Plan: If a repository's tracking flag is toggled while it's in queue, we may loop indefinitely.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: Korvin, epriestley
Differential Revision: https://secure.phabricator.com/D11296
Summary: Currently adding auditors in commit message only works with the plural. Make "Auditor: bla" work.
Test Plan: Unit tests. I don't know if I am supposed to add a test.
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: joshuaspence, epriestley
Differential Revision: https://secure.phabricator.com/D10636
Summary: Moves Dashboard Panels to use PHUIObjectBoxView and PHUIHeaderView
Test Plan:
Tested the most common dashboards, Differential, Maniphest, Projects, Feed, Audit. Some edge cases (Legalpad, Macro) still are in progress. Tested laying out a new Dashboard, removing panels, moving panels.
{F406170}
Reviewers: btrahan, epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T8099
Differential Revision: https://secure.phabricator.com/D12873
Summary: Closes T8176, Add a setting so user can choose when Calendar weeks start
Test Plan: Open account settings, go to "Date and Time", change week start day, open Calendar, observe that month view responds to new week start day.
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T8176
Differential Revision: https://secure.phabricator.com/D12884
Summary: Ref T8176, Split up settings into "Date and Time Settings" and everything else.
Test Plan: Open /settings, Account Settings should now have two tabs: "Account" and "Date and Time"
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T8176
Differential Revision: https://secure.phabricator.com/D12881
Summary: Shares more CSS between header styles, unifies the look. Ref T8099
Test Plan: Open and Close Durable Column a lot. Inspect pixels.
Reviewers: btrahan, epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T8099
Differential Revision: https://secure.phabricator.com/D12877
Summary:
...which lets all the fancy settings for Email | Notify | Off be possible. Fixes T8164. Wasn't too sure the best way to break things up but members vs watchers felt meaningful to break out to me.
Also fixes a small bug where we were generating bad slug updated stories by messing with the signature of the slug data. Perhaps this fix isn't even good enough (the array_keys()) call and instead we'll need to implement transaction has effect and do a sort?
Test Plan: used ./bin/mail list-outbound and ./bin/mail show-outbound --id XX to verify reasonable emails were being generated. saw new preferences in settings.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T8164
Differential Revision: https://secure.phabricator.com/D12868
Summary:
Fixes T6956. Before this change, we called PhabricatorUser::getOmnipotentUser in the various delete methods to query the data. Now, we use $engine->getViewer(), since its always a good thing to have less calls to PhabricatorUser::getOmnipotentUser thrown around the codebase.
I used the "codemod" tool to audit the existing calls to PhabricatorDestructorEngine (all of them) so ostensibly this gets all the spots. If I missed something though, its still going to work, so this change is very low risk.
Test Plan: ./bin/remove destroy P1; visit P1 and get a 404
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T6956
Differential Revision: https://secure.phabricator.com/D12866
Summary:
New, cleaner, ObjectItemLists. Lots of minor style tweaks, basic overview:
- Remove FootIcons
- Remove Stackable
- Remove Plain List
- Add StatusIcon
- Add setting ObjectList to an ObjectBox
- Minor retouches to Headers
Mostly, this should give us an idea of life with the new Object Lists. I'll take another application by application pass down the road. This mostly looks at implementation in Maniphest, Differential, Audit, Workboards. Checked a few other areas and dialogs while testing, and everything looks square.
Test Plan: Maniphest, Differential, Homepage, Audit, People, and other applications. Drag reorder, etc.
Reviewers: btrahan, epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Differential Revision: https://secure.phabricator.com/D12865
Summary: This adds the bare minimum transaction set for editing posts. Fixes T7626 because now files will be correctly attached to phame posts on subsequent edits. Future work here should be adding transaction types like `TYPE_BLOG` for when posts are moved between blogs, `TYPE_VISIBILITY` for when posts are moved to published, etc. Nothing too tricky there but keeping this diff relatively small seems prudent.
Test Plan: made posts successfully. also made errors like no title, no phame title, and duplicate phame title and got correct errors. added a file to a post and verified file has phame post in "attached" tab, which should fix T7626.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T7626
Differential Revision: https://secure.phabricator.com/D12864
Summary: Ref T8183. The list view still used this field to choose a red/orange bar color.
Test Plan: Viewed event list view.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T8183
Differential Revision: https://secure.phabricator.com/D12857
Summary: Closes T8189, Calendar events should be green if viewer is invited, and grey if not.
Test Plan: Check that month and day views display events that viewer is invited to as green and grey otherwise, including month day badges on mobile monthly view.
Reviewers: chad, #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T8189
Differential Revision: https://secure.phabricator.com/D12850
Summary: Ref T7626. Modernizes the code a bit here so we can eventually make progress on T7626 and other stuff.
Test Plan: made a blog, edited a blog, made errors - stuff looked good
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T7626
Differential Revision: https://secure.phabricator.com/D12849
Summary:
Both user.whoami and user.query call the same wire formatting code, but expect different data.
Don't try to add availability data to user.whoami.
Stop adding email data to user.query. We've added it since D11791, but my intent was for it to be exposed //only// via user.whoami (i.e., expose your address, not others').
Test Plan:
- Called both methods.
- Saw emails on user.whoami.
- Saw availability on user.query.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Differential Revision: https://secure.phabricator.com/D12848
