Summary:
Ref T4379. When you add a redundant edge, we currently compare the values strictly, using `===`. However, the old and new versions of the edge have slightly different member data, because one has been synthetically constructed and one has been read from the database.
Instead, compare only the things we actually care about:
# Were any destintations added or removed?
# Was any edge data changed?
If the answer to both questions is "no", consider the update a no-op.
Test Plan: In the next diff, I'm making project members use the EDGE transaction type. Before this change, adding an existing project member would generate a transaction with no changes. Now, it is correctly detected as a no-op, while normal transactions continue to work properly.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T4379
Differential Revision: https://secure.phabricator.com/D8166
Summary:
Ref T4379. I want project subscriptions to work like this (yell if this seems whacky, since it makes subscriptions mean somethign a little different for projects than they do for other objects):
- You can only subscribe to a project if you're a project member.
- When you're added as a member, you're added as a subscriber.
- When you're removed as a member, you're removed as a subscriber.
- While you're a member, you can optionally unsubscribe.
From a UI perspective:
- We don't show the subscriber list, since it's going to be some uninteresting subset of the member list.
- We don't show CC transactions in history, since they're an uninteresting near-approximation of the membership transactions.
- You only see the subscription controls if you're a member.
To do this, I've augmented `PhabricatorSubscribableInterface` with two new methods. It would be nice if we were on PHP 5.4+ and could just use traits for this, but we should get data about version usage before we think about this. For now, copy/paste the default implementations into every implementing class.
Then, I implemented the interface in `PhabricatorProject` but with alternate defaults.
Test Plan:
- Used the normal interaction on existing objects.
- This has no actual effect on projects, verified no subscription stuff mysteriously appeared.
- Hit the new error case by fiddling with the UI.
Reviewers: btrahan
Reviewed By: btrahan
CC: chad, aran
Maniphest Tasks: T4379
Differential Revision: https://secure.phabricator.com/D8165
Summary: Add described, simple header and icon with divider.
Test Plan: Tested on an existing mock and created a new mock with and without a description.
Reviewers: epriestley, btrahan
Reviewed By: epriestley
CC: Korvin, epriestley, aran
Differential Revision: https://secure.phabricator.com/D8181
Summary: standard ish. Fixes T4388.
Test Plan: made a comment with L1 and noted L1 linked to L1. Also observed working-ish hovercard.
Reviewers: epriestley
Reviewed By: epriestley
CC: Korvin, epriestley, aran
Maniphest Tasks: T4388
Differential Revision: https://secure.phabricator.com/D8178
Summary: Adds a handy bar full of tiny buttons. Use only when directed. Ref: T4394
Test Plan: View UI Examples.
Reviewers: epriestley, btrahan
Reviewed By: epriestley
CC: Korvin, epriestley, aran
Differential Revision: https://secure.phabricator.com/D8169
Summary: I don't think this is too terrible, and makes the future easier? Maybe?
Test Plan: ALLCAPS translation, Viewed a diff, feed, and notifications.
Reviewers: epriestley, btrahan
Reviewed By: epriestley
CC: Korvin, epriestley, aran
Differential Revision: https://secure.phabricator.com/D8175
Summary: After the recent search changes, the filter here changed from `type` to `types`. Currently, if you click "Attach Differential Revisions", it shows objects of too many types.
Test Plan: Clickced "Attach Differential Revisions" or whatever it's called, just saw revisions.
Reviewers: btrahan, chad
Reviewed By: chad
CC: aran
Differential Revision: https://secure.phabricator.com/D8164
Summary:
Ref T4387. By using `hg locate` to attempt to only list files in the given path
browsing diffusion is a bit faster. In a repo of about 600M it shaves a rough 100ms
off viewing the root of the project.
Test Plan: Looked around in diffusion and saw it showed everything including .files, which was nice
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley
CC: Korvin, epriestley, aran
Maniphest Tasks: T4387
Differential Revision: https://secure.phabricator.com/D8163
Summary: Lets you type any mailable into the "Subscribers" field.
Test Plan: Typed a list, got relevant tasks.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Differential Revision: https://secure.phabricator.com/D8161
Summary:
See <https://github.com/facebook/phabricator/issues/505>. When the status/event table moved, it broke this migration, which implicitly loads statuses by loading events.
Instead, access just the row we care about.
Test Plan: Used `--apply` to apply the new version of the patch.
Reviewers: btrahan, chad
Reviewed By: chad
CC: aran
Differential Revision: https://secure.phabricator.com/D8162
Summary: It used to say "Mark with flag 7" or whatever, and now it says "Mark with flag Checkered"
Test Plan: noted previous rule I made was more understandable
Reviewers: epriestley
Reviewed By: epriestley
CC: Korvin, epriestley, aran
Differential Revision: https://secure.phabricator.com/D8158
Summary: turn on herald rules ability to specify other herald rules. Fixes T4294.
Test Plan: made a rule to be cc'd on new tasks. made another rule to flag a task if it contained "test test" in the title AND the cc'd rule for new tasks matched. Made some new tasks and verified new "test test" tasks were flagged.
Reviewers: epriestley
Reviewed By: epriestley
CC: Korvin, epriestley, aran
Maniphest Tasks: T4294
Differential Revision: https://secure.phabricator.com/D8157
Summary: adds a new FIELD and a new VALUE to support this. Slightly dodgy because priorities do not have phids so we have to special case how we handle this in a few spots. Ref T4294.
Test Plan: made a new rule to get cc'd on unbreak now and wishlist tasks. verified got cc'd correctly and not cc'd correctly.
Reviewers: epriestley
Reviewed By: epriestley
CC: Korvin, epriestley, aran
Maniphest Tasks: T4294
Differential Revision: https://secure.phabricator.com/D8156
Summary: Fixes T4378. I just removed the `<em>` since this element is unusual and it's not convenient to switch it to translatable remarkup.
Test Plan: stared at it real good
Reviewers: btrahan
Reviewed By: btrahan
CC: aran, chrisbolt
Maniphest Tasks: T4378
Differential Revision: https://secure.phabricator.com/D8155
Summary: See IRC. This migration inadvertently depends on the columns in the commit table, because it calls `save()`, and thus broke for installs with data after we added the `importStatus` column. Since that was ~9 months after this patch, probably not many installs are affected.
Test Plan: Ran patch locally with `--apply` on data. Had user verify fix.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Differential Revision: https://secure.phabricator.com/D8152
Summary: Ref T4375. We're going to need these for a bunch of infrastructure to work.
Test Plan: Ran migrations, checked DB, used `phid.query`.
Reviewers: btrahan, chad
Reviewed By: chad
CC: aran
Maniphest Tasks: T4375
Differential Revision: https://secure.phabricator.com/D8151
Summary: Ref T4375. Very basic, but gives us a more standard place to put edit/delete operations.
Test Plan: {F108765}
Reviewers: btrahan, chad
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T4375
Differential Revision: https://secure.phabricator.com/D8149
Summary: Ref T4375. Basic ApplicationSearch integration to power this more flexibly.
Test Plan: {F108762}
Reviewers: btrahan, chad
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T4375
Differential Revision: https://secure.phabricator.com/D8148
Summary: Ref T4375. This doesn't get everything (I figure I'll clean up the actual UI strings when I touch the UIs) but should get the bulk of the URIs and class names and stuff.
Test Plan: Clicked every calendar-related link I could find/grep, they all still seem to work. URIs now say "event".
Reviewers: btrahan, chad
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T4375
Differential Revision: https://secure.phabricator.com/D8147
Summary: Ref T4375. Calendar uses oldschool `loadOneWhere()` calls. Make CalendarEvent policy-aware, do the edit/delete policy checks through the policy framework, and use modern query infrastructure.
Test Plan:
- Viewed calendar;
- created, edited, deleted event;
- viewed calendar tab in Conpherence.
Reviewers: btrahan, chad
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T4375
Differential Revision: https://secure.phabricator.com/D8146
Summary: Ref T4375. We never join this table, so this is a pretty straight find/replace.
Test Plan: Browsed around Calendar, verified that nothing seemed broken. Saw my red dot in other apps.
Reviewers: btrahan, chad
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T4375
Differential Revision: https://secure.phabricator.com/D8145
Summary: Currently, mentioning two projects in a block (`{#a} {#b}`) produces an overzealous parse. Forbid these characters in project monograms.
Test Plan: Got correct markup.
Reviewers: btrahan, dctrwatson, chad
Reviewed By: chad
CC: aran
Differential Revision: https://secure.phabricator.com/D8141
Summary: Ref T1139. This has some issues and glitches, but is a reasonable initial attempt that gets some of the big pieces in. We have about 5,200 strings in Phabricator.
Test Plan: {F108261}
Reviewers: btrahan
Reviewed By: btrahan
CC: aran, chad
Maniphest Tasks: T1139
Differential Revision: https://secure.phabricator.com/D8138
Summary: ...and surface it in all adapters except commit adapters. Values are true or false. Ref T4294
Test Plan: made a herald rule to be cc'd on new tasks. was cc'd on new tasks and not cc'd on updates to existing tasks.
Reviewers: epriestley
Reviewed By: epriestley
CC: Korvin, epriestley, aran
Maniphest Tasks: T4294
Differential Revision: https://secure.phabricator.com/D8142
Summary: I've only seen this be an issue with PhabricatorBot.
Test Plan: Comment on something with characters that are automatically converted like "<>", and see what the irc bot reports
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley
CC: epriestley, aran
Differential Revision: https://secure.phabricator.com/D8140
Summary: Ref T4365. Aligns jump nav with the normal search behavior.
Test Plan: Typed some junk into the jump nav, mashed return.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T4365
Differential Revision: https://secure.phabricator.com/D8136
Summary:
Fixes T4365. See discussion in D8123.
This implements the most conservative solution of approaches discussed in D8123. Basically:
- When you search in primary search, we overwrite "query" in your default (topmost) search filter, and execute that.
This doesn't implement any of the other "sticky" stuff, where the query sticks around. Maybe we'll do that eventually, but it gets messy and could be confusing. Practically, this addresses the major use case in the wild, which is to make the menu bar search mean "Open Tasks" by default.
This also removes the old, obsolete "search scope" stuff. A long time ago, searching from within Maniphest would search tasks, etc., but this was pretty weird and confusing and is no longer used, and no one complained when we got rid of it.
Test Plan: Dragged "Open Tasks" to my top search, searched for "asdf", got "asdf in open tasks" results.
Reviewers: btrahan, chad
Reviewed By: btrahan
CC: bigo, aran
Maniphest Tasks: T4365
Differential Revision: https://secure.phabricator.com/D8135
Summary:
Ref T4365. Drive primary search through ApplicationSearch instead of through a bunch of custom nonsense. Notably, this allows you to save searches, notably.
The one thing this doesn't do -- which I'd like it to -- is carry your query text across searches. When you search for "quack", I want to overwrite the query in your default filter and give you those results, so you can turn the search into an "Open Tasks" search by default by reordering the queries. I'll probably do that next. It feels a little hacky but I want to try it out.
Test Plan: {F106932}
Reviewers: btrahan, chad
Reviewed By: btrahan
CC: aran, bigo
Maniphest Tasks: T4365
Differential Revision: https://secure.phabricator.com/D8123
Summary: Ref T4365. It's not practical to cursor-page all engines; allow main search engines to be offset-paged. Basically, this comes down to setting a flag and then doing a couple of tiny things differently.
Test Plan: Used this two diffs from now.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T4365
Differential Revision: https://secure.phabricator.com/D8121
Summary: Ref T4365. Two diffs from now, I'm changing the UI a bit to let you search for closed and unowned documents more explcitly. To support this in ElasticSearch and more easily in MySQL search, make these explicit, positive relationships.
Test Plan: `bin/search index --all`
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T4365
Differential Revision: https://secure.phabricator.com/D8122
Summary:
Ref T4365. Primary search currently uses `PhabricatorSearchQuery` for storage, which is pretty much the same as `PhabricatorSavedQuery`, except that it's old and not used anywhere else anymore.
Maniphest used to also use this table, but no longer does after Septmeber, 2013. We need to retain the class so the migration can work.
This introduces `PhabricatorSearchApplicationSearchEngine` and `PhabricatorSearchDocumentQuery`, but they're both stubs that I just needed for technical reasons and/or to pass lint. The next couple patches will move logic into them and use ApplicationSearch properly.
Test Plan:
- Searched for stuff.
- Searched for stuff with filters.
- Searched for fulltext in Maniphest.
- Grepped for `PhabricatorSearchQuery`.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T4365
Differential Revision: https://secure.phabricator.com/D8120
Summary: See <https://github.com/facebook/phabricator/issues/501>. I think the issue here is that we created a foreign stub for commit `X-1`, probably because commit `X` was created by running `svn cp y x`.
Test Plan: I'll write a separate test for this before I land it. Huge pain to test.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Differential Revision: https://secure.phabricator.com/D8133
Summary: Ref T3583. This doesn't add any dashboard/panel-specific code beyond headers/titles/buttons/etc., but allows you to create and view dashboards and panel skeletons.
Test Plan: See screenshots.
Reviewers: btrahan, chad
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T3583
Differential Revision: https://secure.phabricator.com/D8131
Summary:
Fixes T4368. This is the last "obvious" table we have which we should be GC'ing but do not. It's about 1/12th of the data on `secure.phabricator.com`.
This table stores logins, account creation, password resets, login attempts, etc, and is primarily useful if something sketchy happens so you can go back and review login activity. This data is not useful indefinitely, and there's no reason to retain it forever. Because you don't always know when something sketchy happened I've given this table a fairly long TTL (180 days), but we don't need limitless amounts of this data.
Test Plan: Ran `phd debug garbage` and saw a reasonable amount of data get GC'd. This table already has an appropriate key.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T4368
Differential Revision: https://secure.phabricator.com/D8128
Summary: Ref T4368. We don't currently GC these tables, and the sent mail table is one of the largest on `secure.phabricator.com`. There's no value in retaining this information indefinitely. Instead, retain it for 90 days, which should be plenty of time to debug/diagnose any issues.
Test Plan: Ran `phd debug garbage`, saw it clean up a reasonable amount of data from these tables.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T4368
Differential Revision: https://secure.phabricator.com/D8127
Summary: `What's new` has been broken for awhile, I've updated it to use the `feed.query` text view.
Test Plan: Start up a bot and say "What's new?"
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley
CC: fas, epriestley, aran, Kage, demo
Differential Revision: https://secure.phabricator.com/D8118
Summary:
Ref T4361. Before we figure out which To/CC are addressable, try to expand To/CC. Specifically, the supported expansion right now is project PHIDs expanding to all their members.
Because of the way multiplexing works, we have to do this in two places: explicitly in `multiplexMail()`, and when sending mail that wasn't multiplexed. This is messy; eventually we can get rid of it (after ApplicationTransactions are everywhere).
This has some rough edges, but should basically give us what we need to make stuff like projects mailable. Particularly, it deals with most issues in D7436:
- I got around the resolution/multiplexing issue by resolving aggregate mailables separately from mailable actors.
- We get to keep the Project PHID as a To/CC/Reviewer/Whatever until the last second.
- Users won't get two emails for being a CC and also a member of a CC'd project.
- We can degrade to the list stuff this way if we want, by having the project aggregate yield a single list PHID.
Test Plan: Added a comment to a revision with a project reviewer, got mail to all the project's members.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T4361
Differential Revision: https://secure.phabricator.com/D8117
Summary:
Fixes T4202. We have old code in MetaMTA which implements gradual backoff and maximum retries.
However, we have more general code in the task queue which does this, too. We can just use the more general stuff in the task queue; it obsoletes the specific stuff in MetaMTA, which is more complex and ran into some kind of issue in T4202.
Remove `retryCount`, `nextRetry` (obsoleted by task queue retry mechanisms) and "simulated failures" (no longer in use).
Generally, modern infrastructure has replaced these mechanisms with more general ones.
Test Plan:
- Sent mail.
- Observed unsendable mail failing in reasonable ways in the queue.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T4202
Differential Revision: https://secure.phabricator.com/D8115
Summary:
Ref T156. @vlada recently implemented filename search in Diffusion, this cleans up the UI a little bit:
- Instead of showing one search box with two different buttons, let the submit buttons appear to the right of the text boxes and separate the search modes.
- Clean up the results a little bit (don't show columns which don't exist).
Test Plan: {F107260}
Reviewers: vlada, btrahan, chad
Reviewed By: chad
CC: vlada, chad, aran
Maniphest Tasks: T156
Differential Revision: https://secure.phabricator.com/D8125
Summary:
Ref T156. Adds basic filename search support for Diffusion,
currently only for Git repositories.
This is preliminary, and it's up for discussion:
- is the UI in the right place;
- what should the search query syntax be (e.g. whether
to put `*`s in the beginning and end of it);
- how to best approach it for Mercurial and/or SVN;
- what's the cleanest result format for `lsquery` (I went
for the minimum necessary change to `DiffusionBrowseSearchController`).
Test Plan:
Browse to a repository in Diffusion, and use both
`Search File Names` and `Search File Content`.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley
CC: Korvin, epriestley, aran
Maniphest Tasks: T156
Differential Revision: https://secure.phabricator.com/D8093
Summary: This adds the app icons, cleans up css Ref T3623
Test Plan: see new icons in dropdown menu
Reviewers: epriestley
Reviewed By: epriestley
CC: Korvin, epriestley, aran
Maniphest Tasks: T3623
Differential Revision: https://secure.phabricator.com/D8124
Summary: This uses the slightly smaller icons. Not sure about the logout icon, will play with it more in the morning.
Test Plan: tested new nav on desktop and mobile.
Reviewers: epriestley
Reviewed By: epriestley
CC: Korvin, epriestley, aran
Differential Revision: https://secure.phabricator.com/D8119
Summary:
Fixes T4358. User request from IRC, but I think this is generally reasonable.
Although we can not prevent users from determining that other user accounts exist in the general case, it does seem reasonable to restrict browsing the user directory to a subset of users.
In our case, I'll probably do this on `secure.phabricator.com`, since it seems a little odd to let Google index the user directory, for example.
Test Plan: Set the policy to "no one" and tried to browse users.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T4358
Differential Revision: https://secure.phabricator.com/D8112
