Summary:
Ref T11404. This improves things by about 10%:
- Use `PhutilClassMapQuery`, which has slightly better caching.
- Do a little less work to generate pretty error messages.
- Make the "disabled" code a little faster (and sort of clearer, too?) by doing less fancy stuff.
These are pretty minor adjustments and not the sort of optimizations I'd make normally, but this code gets called ~100x (once per revision) and generates ~10 fields normally, so even small savings can amount to something.
(I also want to try to make `arc` faster in the next update, and improving Conduit performance helps with that.)
Test Plan: Ran `differential.revision.search`, saw cost drop from ~195ms to ~170ms locally.
Reviewers: yelirekim, chad
Reviewed By: chad
Maniphest Tasks: T11404
Differential Revision: https://secure.phabricator.com/D16355
Summary:
Ref T11404. On my system, this improves performance by 10-15% for `differential.revision.search`.
`PhutilTypeSpec` provides high quality typechecking and is great for user-facing things that need good error messages.
However, it's also a bit slow, and pointless here (the API is internal and it only has one possible option).
I think I added this after writing `checkMap` just because I wanted to use it more often. My desire is sated after finding many reasonable ways to use it to give users high-quality error messages about things like configuration files.
Test Plan: Profiled `differential.revision.search` before and after change, saw wall time drop from ~220ms to ~195ms.
Reviewers: yelirekim, chad
Reviewed By: chad
Maniphest Tasks: T11404
Differential Revision: https://secure.phabricator.com/D16354
Summary:
Ref T11404. Depends on D16351. Currently, both `differential.query` and `differential.revision.search` issue `2N` queries to fetch:
- dependencies for each revision; and
- projects for each revision.
Fix this:
- Take these custom fields out of Conduit so they don't load this data by default.
- For `differential.query`, put this data back in by hard coding it.
- For `differential.revision.search`, just leave it out. You can already optionally get projects efficiently, and this endpoint is a work in progress. I would tentatively be inclined to expose graph data as a "graph" extension once we need it.
This makes both methods execute in `O(1)` time (which is still 20-30 queries, but at least it's not 320 queries anymore).
Test Plan:
- Ran `differential.query`, observed no change in results but 199 fewer internal queries.
- Ran `differential.revision.search`, observed data gone from results and 200 fewer internal queries.
Reviewers: yelirekim, chad
Reviewed By: chad
Maniphest Tasks: T11404
Differential Revision: https://secure.phabricator.com/D16352
Summary:
Ref T11404. Depends on D16350.
Currently, custom fields can issue "N+1" queries in some cases, so querying 100 revisions issues 100 extra queries.
This affects all `*.search` endpoints for objects with custom fields, and some older endpoints (notably `differential.query`).
This change bulk loads "normal" custom fields, which gets rid of some of these queries. Instead of loading fields for each object, we build a big list of all fields and load them all at once.
The next change will tackle the remaining inefficient edge queries.
Test Plan:
- Configured a custom field with normal database storage in Differential.
- Ran `differential.query`, looking at custom fields in results for correctness.
- Ran `differential.revision.search`, looking at custom fields in results for correctness.
- In both cases, observed queries drop from `3N` to `2N` (all the "normal" custom field stuff got bulk loaded).
Reviewers: yelirekim, chad
Reviewed By: chad
Maniphest Tasks: T11404
Differential Revision: https://secure.phabricator.com/D16351
Summary:
Ref T11404. Currently, SearchEngineAttachments can bulk-load data but SearchEngineExtensions can not.
This leads to poor performance of custom fields. See T11404 for discussion.
This changes the API to support a bulk load + format pattern like the one Attachments use. The next change will use it to bulk-load custom field data.
Test Plan:
- Ran `differential.query`, `differential.revision.search` as a sanity check.
- No behavioral changes are expected
- See next revision.
Reviewers: yelirekim, chad
Reviewed By: chad
Maniphest Tasks: T11404
Differential Revision: https://secure.phabricator.com/D16350
Summary:
We're picking three useful groups of build statuses to provide as default queries:
- Stuff not yet building
- Stuff building
- Stuff which has finished building
These are reasonable buckets for builds since (unlike most objects in phabricatorland) users are generally waiting impatiently for the machine to do something for them, rather than being responsible for doing something with the machine.
Test Plan: clicked around the search engine and enjoyed my defaults
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: epriestley
Differential Revision: https://secure.phabricator.com/D16349
Summary:
This supports a few basic use cases that aren't served by the buildable search engine:
- I'm trying to discover when the last time that this particular build plan failed was.
- I want to know if any builds have deadlocked.
- At a glance, I'm more interested in what build plans are running, not which buildables are being built. This is more often than not the case.
Test Plan: {F1744003}
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: Korvin, epriestley
Differential Revision: https://secure.phabricator.com/D16347
Summary: Fixes T11392. If some tasks are restricted, we only have PHIDs for them, not objects. Just use the PHIDs instead.
Test Plan: {F1741335}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T11392
Differential Revision: https://secure.phabricator.com/D16345
Summary:
Ref T4788. This gives us a new level of graceful degradation, so now we show:
- Zero through 100 connected tasks: whole graph.
- More than 100 connnected tasks, but fewer than 100 direct parents/subtasks: just parents and subtasks, with "..." to hint that the graph is cut off.
- More than 100 parents and children: just the "sorry, too much stuff" error message.
Test Plan: {F1740882}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T4788
Differential Revision: https://secure.phabricator.com/D16344
Summary:
Ref T4788. Add links to jump to search results with a task's parents or subtasks. This allows relationships to remain useful if there are a zillion of them, and you can sort/filter stuff more easily.
Language might need some tweaking at some point, feeling a little un-brainy today with wordstuff.
Test Plan:
{F1740855}
{F1740856}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T4788
Differential Revision: https://secure.phabricator.com/D16343
Summary:
Fixes T11386. Ref T4788.
- Apparently fix weird strikethrough effect? Spooky!
- Provide a little icon hint in the left column about which tasks are direct parents/children, vs just reachable somehow. I don't think this is super useful/important, but seems maybe nice?
Test Plan: {F1740779}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T4788, T11386
Differential Revision: https://secure.phabricator.com/D16342
Summary:
Ref T8126. Ref T4788. This adds a way to query by parent or subtask.
I plan to link to this from the task graph (e.g., {nav View > Search Subtasks} or similar, in a dropdown on the "Task Graph" element) as a way to let us bail out if tasks have 300 subtasks and send the user to a big query result list. That'll give us more flexibility to tailor the UI for reasonable numbers of tasks.
There's no UI for this unless you specify a query yourself, so the only ways to get to it are:
- Manually put `?parentIDs=...` into the URI.
- Use the API.
- Future link from task graphs.
It doesn't seem too useful to me on its own, outside of the context of links from tasks.
Test Plan:
- Manually put `?parentIDs=...` and `?subtaskIDs=...` into Maniphest query UI, got expected results.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T4788, T8126
Differential Revision: https://secure.phabricator.com/D16341
Summary:
Ref T8126. See that task for discussion. This change:
- Updates language to be more consistent ("Parents", "Subtasks") since I moved us away from the often-confusing "Block" language in T4788.
- Fixes bugs with finding the wrong set of tasks if tasks have a mixture of open and closed subtasks or parents.
Test Plan:
- Created four tasks: no subtasks, one closed subtask, one open subtask, mixture of open and closed subtasks.
- Created four more tasks: no parents, one closed parent, one open parent, mixture of open and closed parents.
- Searched for all this stuff, got the proper results:
{F1740683}
{F1740684}
{F1740685}
{F1740686}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T8126
Differential Revision: https://secure.phabricator.com/D16340
Summary: Ref T11326. This adds prev/next links for recurring events (ala D16179) and moves the "accept/decline" buttons closer to the invite list. This might need some fiddling, but should be a little more human-friendly.
Test Plan: {F1740541}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T11326
Differential Revision: https://secure.phabricator.com/D16339
Summary: Ref T8116. Puts a list of packages on the publisher page, and a list of versions on the package page.
Test Plan: Viewed a publisher, saw packages. Viewed a package, saw versions. Looked at list views.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T8116
Differential Revision: https://secure.phabricator.com/D16321
Summary: Ref T8116. Add search-by-name and per-package / per-publisher search to Packages.
Test Plan: Searched publishers, packages, versions by name. Searched packages by publisher. Searched versions by package.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T8116
Differential Revision: https://secure.phabricator.com/D16320
Summary:
Ref T8116. This adds a control for creating publishers (default: administrators) and default publisher/package edit controls.
I've left the edit defaults at "no one" for now to force you to select a policy. This might be something to look at later.
Test Plan: Created publishers, packages. Tried to create publishers with "can create" policy set restrictively.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T8116
Differential Revision: https://secure.phabricator.com/D16319
Summary:
Ref T8116. A version has:
- a package (like "Arcanist") which it belongs to;
- a name (like "v3.1.5").
The name is immutable and unique, like the package key and publisher key.
Policy stuff:
- Versions have the exact same policies as their packages.
- You must be able to edit a package to create new versions of it.
This is still entirely uninteresting.
Test Plan: {F1731703}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T8116
Differential Revision: https://secure.phabricator.com/D16316
Summary:
Ref T8116. A package has:
- a publisher (like "Phacility"), from the previous revision;
- a name (like "Arcanist");
- a package key (like "arcanist").
The package key is immutable, like the publisher key.
This gives a package a full key like "phacility/arcanist".
Policy stuff:
- You must be able to view a publisher to view a package (currently, everyone can always see all publishers).
- You must be able to edit a publisher to create a new package inside it.
- Packages have separate view/edit permissions.
This still does nothing interesting.
Test Plan: {F1731663}
Reviewers: chad
Reviewed By: chad
Subscribers: eadler
Maniphest Tasks: T8116
Differential Revision: https://secure.phabricator.com/D16315
Summary:
Ref T8116. Partially scavenged from D14152. This roughs in a new Packages application for Arcanist extensions and third-party applications, and adds a "Publisher" object.
A "Publisher" represents an individual or entity who is publishing a package, like "Phacility". It's explicitly //not// necessarily the original author -- just the primary entity vouching for the safety of the code.
A publisher just has a name and a unique key for now. For example, Phacility might have "Phacility" and "phacility", respectively.
Unique keys are immutable, e.g., the package "phacility/arcanist" will always be exactly the same package by exactly the same publisher.
Test Plan: {F1731621}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T8116
Differential Revision: https://secure.phabricator.com/D16314
Summary:
Ref T11326. This isn't perfect, but should be a little easier to use and less weird/confusing.
Generally, provide a "Query > Month > Day" crumb on day views, and a "Wed, July 3" header.
Generally, provide a "Query > Month" crumb on month views, and a "July 2019" header.
Also try to fix a bit of padding/spacing on the day view.
Test Plan: {F1739128}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T11326
Differential Revision: https://secure.phabricator.com/D16338
Summary:
Ref T11326. When viewing "February", add a class to dates in January and March to let them be styled a little differently as a UI hint.
For now, I've given them a grey background. (Calendar.app changes the date number color instead.)
Test Plan: {F1738990}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T11326
Differential Revision: https://secure.phabricator.com/D16336
Summary:
Ref T11326. This doesn't go quite as far as the mock in T11326#185932, but gets rid of the easy margins.
Also cleans up some of the border rules so they're simpler and more consistent (no weird ragged edges on the far right).
Test Plan: {F1738951}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T11326
Differential Revision: https://secure.phabricator.com/D16335
Summary:
Ref T11326. Currently, we link Calendar days using hidden DOM nodes.
This is nice because it's simple, and right-clicking a day works properly. However, it's a bit ugly/unintuitive, messy, and unclear. It's especially messy because days are really two different rows, one for events and one for day/week numbers.
Instead, use JS to highlight day cells. You can still right-click by clicking the actual day number, which seems like a reasonable compromise.
Test Plan: {F1738941}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T11326
Differential Revision: https://secure.phabricator.com/D16334
Summary:
Ref T11326. When an event is all-day, hide the time controls for the start/end dates. These aren't used and aren't helpful/useful.
This got a little more complicated than it used to be because EditEngine forms may have only some of these controls present.
Test Plan: Edited an all-day event; edited a normal event; swapped an event between normal and all-day.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T11326
Differential Revision: https://secure.phabricator.com/D16327
Summary: When we have an info view in a column, the css isn't specific enough to override the core info-view css.
Test Plan: Review an importing repository, see info view properly spaced.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D16331
Summary: This padding is a little off / custom. Normalizes it to the form on mobile.
Test Plan: Review some settings forms, save form changes.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D16330
Summary: Mobile forms are super tight, this opens them up a little bit.
Test Plan: Review editing a document, task, mobile, tablet.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D16329
Summary:
Fixes T11375. Some validation code was mishandling raw epoch timestamps.
For numeric values larger than 29999999 (e.g., 2999-12-25, christmas 2999), assume the value is a timestamp.
Test Plan: Used `maniphest.search` to query for `modifiedStart`, got a better result set and saw the `dateModified` constraint in the query.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T11375
Differential Revision: https://secure.phabricator.com/D16326
Summary:
Finishes fixing T11365. rP28199bcb48 added the new numeric entry
control and used it for TOTP setup, but missed the case of entering
a factor when TOTP was already set up.
Test Plan:
Observe behaviour of TOTP setup and subsequent factor entry
in iOS browser, make sure they're consistent.
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: epriestley
Maniphest Tasks: T11365
Differential Revision: https://secure.phabricator.com/D16325
Summary:
Fixes T11365. I tested these variants:
- `<input type="number" />`
- `<input type="text" pattern="\d*" />`
Of these, this one (using `pattern`) appears to have the best behavior: it shows the correct keyboard on iOS mobile and does nothing on desktops.
Using `type="number"` causes unwanted sub-controls to appear in desktop Safari, and a numbers + symbols keyboard to appear on iOS (presumably so users can type "." and "-" and maybe ",").
Test Plan: Tested variants in desktop browsers and iOS simulator, see here and T11365 for discussion.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T11365
Differential Revision: https://secure.phabricator.com/D16323
Summary:
Fix T11339.
Now, old and new are both simple lists of phids, and the rendering should make sense.
Test Plan: Viewed existing transaction with all 3 states.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: Korvin
Maniphest Tasks: T11339
Differential Revision: https://secure.phabricator.com/D16311
Summary:
See accompanying discussion in T11359.
As far as I can tell we aren't vulnerable, but subprocesses could be (now, or in the future). Reject any request which may have a `Proxy:` header.
This will also do a false-positive reject if `HTTP_PROXY` is defined in the environment, but this is likely a misconfiguration (cURL does not read it). I'll provide guidance on this.
Test Plan:
- Made requests using `curl -H Proxy:...`, got rejected.
- Made normal requests, got normal pages.
Reviewers: chad, avivey
Reviewed By: avivey
Differential Revision: https://secure.phabricator.com/D16318
Summary:
`mysql` has the magic feature of ignoring port arguments and using the socket when connecting to localhost.
This flag makes it not do that.
Test Plan: `./bin/storage shell`, execute `status`, see `Connection: localhost via TCP/IP`.
Reviewers: joshuaspence, #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D16317
Summary:
Fixes T11358. Entering a too-long title/subtitle currently raises an unfriendly (database-level) error.
Raise a friendlier error.
Test Plan: {F1731533}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T11358
Differential Revision: https://secure.phabricator.com/D16313
Summary:
Fixes T10750. Files have some outdated cache/key code which prevents recording an edit history on file comments.
Remove this ancient cruft.
(Users must `bin/storage adjust` after upgrading to this patch to reap the benefits.)
Test Plan:
- Ran `bin/storage adjust`.
- Edited a comment in Files.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10750
Differential Revision: https://secure.phabricator.com/D16312
Summary: Ref T11326. This makes it a little easier to jump back up to check out your day.
Test Plan: {F1725575}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T11326
Differential Revision: https://secure.phabricator.com/D16309
Summary: Ref T11326. This just cleans things up a little and removes some of the obvious layout/CSS issues.
Test Plan:
- Viewed day view before/after. Also viewed profile panel.
Before:
{F1725547}
After:
{F1725548}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T11326
Differential Revision: https://secure.phabricator.com/D16308
Summary:
Ref T11326. This just inches things forward a little bit:
- Make it easier to see current day.
- Line-through cancelled events.
- Don't colorize the whole event title, just use an Attending/Invited/Custom icon.
- Slightly subtler treatment for all-day events.
Test Plan: See screenshot in T11326.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T11326
Differential Revision: https://secure.phabricator.com/D16306
Summary:
Ref T11326. Normally, events occur at a specific epoch, independent of the viewer. For example, if we're having a meeting in 35 hours, every user who looks at the event will see that it starts 35 hours from now.
But when an event is "All Day", the start time and end time depend on the //viewer//. A day like "Christmas" does not start at the same time for everyone: it starts sooner if you're in a more-eastern timezone. Baiscally, an event on "July 15th" starts whenever "July 15th" starts for whoever is looking at it.
Previously, we stored these events by using the western-most and eastern-most timezones as the start and end times (the earliest possible start and latest possible end).
This worked OK, but we get into a bunch of trouble with EditEngine, mostly because each field can be updated individually now. We can't easily tell if an event is all-day or not when reading or updating the start time and end time, and making that easier would introduce a huge amount of complexity.
Instead, when we update the start or end time, we write //two// times:
- The epoch timestamp of the time the user entered, which is the start time we will use if the event is a normal event.
- The epoch timestamp of 12:00 AM in UTC on the same date as the //local// date the user entered. This is pretty much like just storing the date the user actually typed. This is what w'ell use if the event is an all-day event.
Then, no matter whether the event is later made all-day or not, we have all the information we need to display it correctly.
Test Plan:
- Created and edited all-day events.
- Migrated existing all-day events, which appeared to survive without problems. (Note that events all-day which were created or edited in the last couple of days `master` won't survive this mutation correctly and will need to be fixed.)
- Created and edited normal, recurring, and recurring all-day events.
- Swapped back to `stable`, created an event, specifically migrated it forward, made sure it survived with times intact.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T11326
Differential Revision: https://secure.phabricator.com/D16305
Summary: Ref T11326. Align this stuff with "Host" and "hostPHID".
Test Plan: Searched for events by host.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T11326
Differential Revision: https://secure.phabricator.com/D16303
Summary:
Ref T11326. This gets rid of the old multi-paged form stuff used in the last version of Diffusion.
This incidentally removes a callsite for a date control to make it a little easier to simplify them.
Test Plan: Grepped for all removed classes, no more callsites.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T11326
Differential Revision: https://secure.phabricator.com/D16302
Summary:
Ref T11326. These are last-generation and neither of these have callsites anymore.
(I nuked these since I'm trying to simplify date handling.)
Test Plan: Grepped for callsites.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T11326
Differential Revision: https://secure.phabricator.com/D16301
Summary: Ref T11326. Use modern methods instead of building this stuff separately.
Test Plan: Used `E123`, `{E123}`, saw references render normally.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T11326
Differential Revision: https://secure.phabricator.com/D16300
