Summary:
Fixes T9019. Pretty much ripped from D14467. I added the "policy hint" stuff so that you can create a project with this policy immediately.
I really dislike how the "hint" code works, but we //almost// never need to use it and the badness feels fairly well-contained.
Also pick up a quick feedback fix from D14863.
Test Plan:
- Added test coverage, got it to pass.
- Created a project with "Visible To: Project Members".
Reviewers: joshuaspence, chad
Reviewed By: chad
Maniphest Tasks: T9019
Differential Revision: https://secure.phabricator.com/D14869
Summary: Ref T8509. We currently give you a fairly obtuse error when trying to name a project something like "!!". The error is correct, but not as helpful as it could be. Give users a more specific, more helpful error.
Test Plan: {F1042883}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T8509
Differential Revision: https://secure.phabricator.com/D14872
Summary:
Fixes T8509. Changes these behaviors:
- If you create a project named "QQQ" and add "qqq" as a hashtag at the same time, it fails in an unhelpful way. (Now: succeeds.)
- If you add "qqq" as a hashtag to a project with primary hashtag "qqq", it fails in a correct but probably unnecessary way (Now: just works).
We could make one or both of these behaviors show the user an error instead, but I think it's likely that this behavior is just what they always want.
Test Plan:
- Added failing tests and made them pass.
- Executed both scenarios described above from the web UI.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T8509
Differential Revision: https://secure.phabricator.com/D14871
Summary:
Ref T8509. This fixes three issues:
- Adding a slug like `UPPERCASE` would not give you a normalized slug. (Now: normalizes as `uppercase`.)
- Adding a slug like `UPPERCASE` would allow you to give two different projects the different tags `UPPERCASE` and `uppercase` (and `UpPeRcAsE`, etc). (Now: second tag is rejected as a duplicate.)
- Adding multiple identical or similar slugs would produce a duplicate key exception. (Now: ignores the duplicates.)
Test Plan:
- Added test coverage.
- Made tests pass.
- Hit these cases in the UI.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T8509
Differential Revision: https://secure.phabricator.com/D14870
Summary: See D14467. Just teasing this apart so I can be a little more confident in my commandeering. These are the unambiguous cleanup changes from D14467.
Test Plan: inspection / clicked stuff / no impact
Reviewers: chad, joshuaspence
Reviewed By: joshuaspence
Differential Revision: https://secure.phabricator.com/D14868
Summary: I feel like the daemon configuration should be locked from editing from the web UI, given that much of it won't work unless the daemons are restarted anyway.
Test Plan: N/A
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D14867
Summary:
Ref T10010. Subprojects have the following general membership rule: if you are a member of a subproject ("Engineering > Backend"), you are also a member of the parent project.
It would be unreasonably difficult to implement this rule directly in SQL when querying `withMemberPHIDs()`, because we'd have to do an arbitrarily large number of arbitrarily deep joins, or fetch and then requery a lot of data.
Instead, introduce "materailized members", which are just a copy of all the effective members of a project. When a subproject has a membership change, we go recompute the effective membership of all the parent projects. Then we can just JOIN to satisfy `withMemberPHIDs()`.
Having this process avialable will also be useful in the future, when a project's membership might be defined by some external source.
Also make milestones mostly work like we'd expect them to with respect to membership and visibility.
Test Plan:
- Added and executed unit tests.
- Changed project members, verified materialized members populated correctly in the database.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10010
Differential Revision: https://secure.phabricator.com/D14863
Summary:
Ref T10010. This adds infrastructure for querying projects by type, depth, parent and ancestor.
I needed to revise the "extended policy check" cycle detection rules. When, e.g., querying a grandchild, they incorrectly detected a cycle because both the child and grandchild needed to check the policy of the grandparent.
Instead, simplify it to just do a basic runaway calldepth check. There are many other safety mechanisms to make it so this can't ever occur.
(Cycle detection does have existing test coverage, and those tests still pass, it just takes a little longer to detect the cycle internally.)
There is still no way to create subprojects in the UI.
Test Plan: Added and executed unit tests.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10010
Differential Revision: https://secure.phabricator.com/D14862
Summary:
Ref T10010. This implements technical groundwork for subprojects. Specifically, it implements policy rules like Phriction:
- to see a project, you must be able to see all of its parents (and the project itself).
- you can edit a project if you can edit any of its parents (or the project itself).
To facilitiate this, we load all project ancestors when querying projects so we can do the view/edit checks.
This does NOT yet implement:
- proper membership rules for these projects (up next);
- any kind of UI to let users create subprojects.
Test Plan:
- Added unit tests.
- Executed unit tests.
- Browsed Projects (no change in behavior is expected).
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10010
Differential Revision: https://secure.phabricator.com/D14861
Summary:
Ref T10010. Currently, we do an unusual JOIN to make testing for viewer membership in projects a little cheaper.
This won't work as-is once we have subprojects, so standardize, simplify, and cover it with more tests for now. (I may be able to get a similar optimization later, but want a correct implementation first.)
Test Plan:
This change should create no behavioral differences.
- Added tests.
- Ran tests.
- Viewed projects.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10010
Differential Revision: https://secure.phabricator.com/D14859
Summary: Ref T5240. We don't currently use any of these options and I don't think we have any plans to use them. Strip them out for now to make fixing drag-and-drop stuff easier.
Test Plan: Grepped for removed stuff, no hits.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T5240
Differential Revision: https://secure.phabricator.com/D14864
Summary:
Ref T10004. This could sometimes pass `false`, which counts as disabled.
Instead, pass `null` explicitly.
Test Plan: Edited default space on an EditEngine form.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10004
Differential Revision: https://secure.phabricator.com/D14858
Summary: Currently we do not show node IDs in this view, but do show token IDs in the stream view. Given that this view facilitates testing various XHPAST functionality, it would be useful to add this information.
Test Plan: Saw node IDs in XHPAST.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D14697
Summary:
Fixes T1895. Now that we have on-demand prviews, we can use them on mobile. On mobile:
- don't show live previews;
- only save drafts every 10 seconds.
Also, show fewer remarkup buttons on mobile to try to make sure the more important ones (preview, e.g.) fit.
Test Plan:
- Made window narrower and wider to trigger preview/no-preview behavior.
- Used DarkConsole to verify request rate.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T1895
Differential Revision: https://secure.phabricator.com/D14856
Summary:
Ref T3967. This gives us a reasonable baseline for doing remarkup previews inline in all contexts, and works in weird/constrained context including:
- inline comments;
- conpherence; and
- custom fields.
It would be nicer to go beyond this in contexts like Phame posts, but this is a start, at least.
Test Plan:
{F1040877}
{F1040878}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T3967
Differential Revision: https://secure.phabricator.com/D14855
Summary: Adds a basic nux for `/` and not found documents. Ref T10023
Test Plan: Visit a clean install, see Welcome page. Visit a non built page, see not found UX
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Maniphest Tasks: T10023
Differential Revision: https://secure.phabricator.com/D14854
Summary: These transactions (when a user subscribes or unsubscribes only themselves) are universally uninteresting.
Test Plan:
- Subscribed/unsubscribed, saw transactions but no feed/mail.
- Commented, got implicitly subscribed, saw only comment in feed/mail, saw both transasctions on task.
Reviewers: chad
Reviewed By: chad
Differential Revision: https://secure.phabricator.com/D14853
Summary:
- Phurl is missing a ReplyHandler / MailReceiver (all of this code should get cleaned up eventually, but I don't plan to get to it for a while).
- Badges has a bad call.
This should clean up some bad daemon tasks.
Test Plan: Saw fewer daemon errors after these changes.
Reviewers: chad
Reviewed By: chad
Differential Revision: https://secure.phabricator.com/D14852
Summary:
Ref T9979. This uses ngrams (specifically, trigrams) to build a reasonably efficient index for substring matching. Specifically, for a package like "Example", with ID 123, we store rows like this:
```
< ex, 123>
<exa, 123>
<xam, 123>
<amp, 123>
<mpl, 123>
<ple, 123>
<le , 123>
```
When the user searches for `exam`, we join this table for packages with tokens `exa` and `xam`. MySQL can do this a lot more efficiently than it can process a `LIKE "%exam%"` query against a huge table.
When the user searches for a one-letter or two-letter string, we only search the beginnings of words. This is probably what they want, the only thing we can do quickly, and a reasonable/expected behavior for typeaheads.
Test Plan:
- Ran storage upgrades and search indexer.
- Searched for stuff with "name contains".
- Used typehaead and got sensible results.
- Searched for `aabbccddeeffgghhiijjkkllmmnnooppqqrrssttuuvvwwxxyyzz` and saw only 16 joins.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T9979
Differential Revision: https://secure.phabricator.com/D14846
Summary: Adds a no visible blogs and no posts nux state using new UI. Ref T10032
Test Plan: Archived all my blogs, got no posts fallback. Test a New Blog, got create a post, logged out, saw no create button.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Maniphest Tasks: T10032
Differential Revision: https://secure.phabricator.com/D14848
Summary:
Fixes T10037. When we're building commit `aabbccdd`, we currently do this to check it out:
git reset --hard aabbccdd
However, this has an undesirable side effect of moving the current branch pointer to point at `aabbccdd`. The current branch pointer may be some totally different branch which `aabbccdd` is not part of, so this is confusing and misleading.
Instead, use `git reset --hard HEAD` to get the primary effect we want (destroying staged changes) and then `git checkout aabbccdd` to checkout the commit in a detached HEAD state.
Test Plan:
- Ran a build (a commit-focused operation) successfully.
- Verified working copy was pointed at a detached HEAD afterward:
```
builder@sbuild001:/var/drydock/workingcopy-167/repo/git-test-ii$ git status
HEAD detached at ffc7635
nothing to commit, working directory clean
```
- Ran a land (a branch-foused operation) successfully.
- Verified working copy was pointed at a branch afterward:
```
builder@sbuild001:/core/data/drydock/workingcopy-168/repo/git-test$ git status
On branch master
Your branch is up-to-date with 'origin/master'.
nothing to commit, working directory clean
```
Reviewers: chad
Reviewed By: chad
Subscribers: yelirekim
Maniphest Tasks: T10037
Differential Revision: https://secure.phabricator.com/D14850
Summary:
Ref T6884. Ref T10004. For various reasons we previously didn't publish these transactions, but now do. This is probably a better behavior overall, but we didn't have reasonable strings for them.
Parent tasks now show "alice created blocking task Txxx.".
Feed now shows nothing, since "alice created task Txxx." is right next to any story we would show and showing them both seems silly.
Test Plan:
- Created subtasks.
- Viewed parent tasks.
- Viewed feed.
- Saw pretty reasonable strings/stories.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T6884, T10004
Differential Revision: https://secure.phabricator.com/D14849
Summary:
Fixes T9890. This allows IndexExtensions to emit an object version.
Before we build indexes, we check if the indexed version is the same as the current version. If it is, we just don't call that extension.
T9890 has a case where this is useful: a script went crazy and posted thousands of comments to a single task.
Without versioning, that results in the same comments being indexed over and over again. With versioning, most of the queue could just exit without doing any work.
Test Plan:
- Added a `sleep(1)` to the actual indexing, used `bin/search index --background` to queue up a lot of tasks, ran them with `bin/phd debug task`, saw them complete very quickly with only one actual index operation performed.
- Used `bin/search index --trace` and `bin/search index --trace --background` to observe the behavior of queries against the index version store, which looked sensible.
- Made comments/transactions, saw versions update.
- Used `bin/remove destroy`, verified index versions were purged.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T9890
Differential Revision: https://secure.phabricator.com/D14845
Summary:
Ref T9979. I picked this name long before the advent of modern "Engine" architecture and it ended up being pretty confusing.
Rename "SearchEngine" (currently: mysql or elasticsearch, used to store and query fulltext indexes) to "FulltextStorageEngine" to make it more clear what it does and disambituate it from ApplicationSearch, which also has a bunch of stuff called "SearchEngine", "SearchEngineExtension", etc.
Test Plan: Grepped for `phabricatorsearchengine`.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T9979
Differential Revision: https://secure.phabricator.com/D14843
Summary: Ref T9979. This simplifies/standardizes the code a bit, but mostly gives us more consistent class names and structure.
Test Plan:
- Used `bin/search index --type ...` to index documents of every indexable type.
- Searched for documents by unique text, found them.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T9979
Differential Revision: https://secure.phabricator.com/D14842
Summary:
Ref T9979. There are currently some hacks around Conpherence indexing: it does not really use the fulltext index, but its own specialized index. However, it's kind of hacked up so it can get reindexed by the normal indexing pipeline.
Lift it up into IndexEngine, instead of FulltextEngine. Specifically, the new stuff is going to look like this:
- IndexEngine: Rebuild all indexes.
- ConpherenceIndexExtension: Rebuild thread indexes.
- ProjectMemberIndexExtension: Rebuild project membership views.
- NgramIndexExtension: Rebuild ngram indexes.
- FulltextIndexExtension / FulltextEngine: Rebuild fulltext indexes, a special type of index.
- FulltextCommentExtension: Rebuild comment fulltext indexes.
- FulltextProjectExtension: Rebuild project fulltext indexes.
- etc.
Most of this is at least sort-of-in-place as of this diff, although some of the part in the middle is still pretty rough.
Test Plan:
- Made a unique comment in a Conpherence thread.
- Used `bin/search index --force` to rebuild the index.
- Searched for the comment.
- Found the thread.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T9979
Differential Revision: https://secure.phabricator.com/D14841
Summary:
Ref T9979. This is currently hard-coded but can be done in a generic way.
This has one minor behavioral changes: answer text is no longer included in the question text index in Ponder. I'm not planning to accommodate that for now since I don't want to dig this hole any deeper than I already have. This behavior should be different anyway (e.g., index the answer, then show the question in the results or something).
Test Plan:
- Put a unique word in a Maniphest comment.
- Searched for the word.
- Found the task.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T9979
Differential Revision: https://secure.phabricator.com/D14837
Summary:
Ref T9979. This event had one weird callsite and no known third-party callers. It can be done more cleanly as an extension, now.
This index is used to allow us to "Group By: Project" in Maniphest without joining into the Projects database.
Test Plan:
- Ran a query with "Group By: Project" in Maniphest.
- Renamed project "Apples" to "Zebras".
- Reloaded page.
- UI properly moved "Zebras" tasks to the bottom of the list.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T9979
Differential Revision: https://secure.phabricator.com/D14836
Summary: Ref T9979. This is going to become `FulltextEngine`, but pave the way for that by pulling extensions out of it.
Test Plan:
{F1036624}
- Used `bin/search index Txxx`, saw projects, subscribers and custom fields rebuild in the index.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T9979
Differential Revision: https://secure.phabricator.com/D14835
Summary:
Ref T9890. Ref T9979. Several adjacent goals:
- The `SearchEngine` vs `ApplicationSearchEngine` thing is really confusing. There are also a bunch of confusing class names and class relationships within the fulltext indexing. I want to rename these classes to be more standard (`IndexEngine`, `IndexEngineExtension`, etc). Rename `SearchIndexer` to `IndexEngine`. A future change will rename `SearchEngine`.
- Add the index locks described in T9890.
- Structure things a little more normally so future diffs can do the "EngineExtension" thing more cleanly.
Test Plan:
Indexing:
- Renamed a task to have a unique word in the title.
- Ran `bin/search index Txxx`.
- Searched for unique word.
- Found task.
Locking:
- Added a `sleep(10)` after the `lock()` call.
- Ran `bin/search index Txxx` in two windows.
- Saw first one lock, sleep 10 seconds, index.
- Saw second one give up temporarily after failing to grab the lock.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T9890, T9979
Differential Revision: https://secure.phabricator.com/D14834
Summary: Ref T9979. Convert all DestructionEngine behaviors to extensions.
Test Plan:
{F1033244}
Destroyed an object, verifying:
- Herald transcripts were destroyed;
- edges were destroyed;
- flags were destroyed;
- tokens were destroyed;
- transactions were destroyed;
- worker tasks were cancelled.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T9979
Differential Revision: https://secure.phabricator.com/D14832
Summary:
Ref T9979. The general shape of "engine" code feels pretty good, and I plan to move indexing to be more in line with other modern engines, with the ultimate goal of supporting subprojects (T10010) and several intermediate goals.
Before moving indexing, clean up Destruction, since some of the new indexes will need destruction hooks and destruction currently has a lot of `instanceof` stuff that should be easy to fix by applying more modern approaches.
Test Plan:
- Used `bin/remove destroy` to destory an Almanac device.
- Verified that properties for the device were destroyed.
- Viewed module panel in UI.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T9979
Differential Revision: https://secure.phabricator.com/D14831
Summary: Ref T10032, adds "Basic" NUX to more applications.
Test Plan: Visit each with ?nux=true and click on the create link. T10032 is tracking which apps need general modernization to pick up these changes.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Maniphest Tasks: T10032
Differential Revision: https://secure.phabricator.com/D14847
Summary: Adds basic NUX to Dashboards, Herald, Repositories, Maniphest. Note Herald and Dashboard Panels don't fine the nux for some reason, assume they will when modernized?
Test Plan: Read text, click buttons.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D14844
Summary:
Ref T3462. If someone works directly on `master`, we currently show "Branch: master (branched from master)" in the UI.
Although this is sort of technically accurate, it is confusing.
Instead, just show "Branch: master" in this situation.
Test Plan: Saw "master" instead of "master (branched from master)".
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T3462
Differential Revision: https://secure.phabricator.com/D14829
Summary:
This is just putting a hook in that pretty much works. Behavior:
- If you visit `/maniphest/?nux=true`, it always shows NUX for testing.
- Otherwise, it shows NUX if there are no objects in the application yet.
Test Plan: {F1031846}
Reviewers: chad
Reviewed By: chad
Differential Revision: https://secure.phabricator.com/D14828
Summary: Currently it's difficult to notice posts may be much longer than the summary (in most cases, actually), Adds a consistent "Read More" link to the full post. Also made sure "violet" was more consistantly used.
Test Plan:
Review summaries in Phame
{F1031799}
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D14827
Summary: Ref T9967
Test Plan:
Ran migrations.
Verified database populated properly with PHIDs (SELECT * FROM auth_sshkey;).
Ran auth.querypublickeys conduit method to see phids show up
Ran bin/remove destroy <phid>.
Viewed the test key was gone.
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: Korvin
Maniphest Tasks: T9967
Differential Revision: https://secure.phabricator.com/D14823
Summary:
Ref T10010. This does some cleanups on the schema:
- `viewPolicy`, `editPolicy` and `joinPolicy` were nullable, but should never be `null`. Set them to defaults if they're null, then make the column non-nullable.
- Rename `phrictionSlug` to `primarySlug` and stop adding and removing trailing slashes from it.
- Add new columns to support milestones and non-milestone subprojects.
- Drop very old subprojectPHIDs column. This hasn't done anything in the UI for years and years, and isn't particularly realistic to migrate forward.
The new columns aren't reachable from the UI.
Test Plan:
- Applied patches.
- Grepped for `phrictionSlug`.
- Grepped for `subprojectPHIDs`.
- Created tasks.
- Edited tasks.
- Verified existing tasks still had primary slugs.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10010
Differential Revision: https://secure.phabricator.com/D14825
Summary:
Fixes T10012. The permissions here are little weird: you need edit permission on the //configurations//, not the //engines//. I was checking edit permission on the engines only.
I should possibly make this a bit more consistent, the engine edit permission is just very convenient to use to enforce object create permission right now. I'll likely clean this up after T9789.
Test Plan:
- Tried to reorder forms as a less-privileged user, got proper policy errors.
- Reordered forms normally as a regular user.
Reviewers: chad
Reviewed By: chad
Subscribers: Luke081515.2
Maniphest Tasks: T10012
Differential Revision: https://secure.phabricator.com/D14824
Summary: Ref T10004. This lost a couple of fields when I rearranged how descriptions work. Restore them.
Test Plan:
- Viewed "Using HTTP Parameters".
- Everything had nice descriptions.
- No more weird phantom/misleading 'comment' transaction in UI.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10004
Differential Revision: https://secure.phabricator.com/D14822
Summary: Ref T10004. I missed these previously, so they didn't work quite right. Restore them to glory.
Test Plan: Edited remarkup and text custom fields on an Owners package.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10004
Differential Revision: https://secure.phabricator.com/D14821
Summary:
Ref T10004. This restores "alice created this task." transactions, but in a generic way so we don't have to special case one of the other edits with an old `null` value.
In most cases, creating an object now shows only an "alice created this thing." transaction, unless nonempty defaults (usually, policy or spaces) were adjusted.
Test Plan: Created pastes, tasks, blogs, packages, and forms. Saw a single "alice created this thing." transaction.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10004
Differential Revision: https://secure.phabricator.com/D14820
Summary:
Fixes T9994. Currently, when Drydock can't allocate a new resource because some limit has been reached, it waits patiently for a resource to become available.
It is possible that no resource will ever become available. Particularly with "Working Copy" resources, the new lease may want a copy of `rB`, but the resource may already be maxed out on `rA`.
Right now, no process exists to automatically reclaim the unused `rA`.
When we encounter this situation, try to reclaim one of the other resources if it is just sitting there unused.
Specifically:
- Add a "reclaim" command which means "release this resource //if// it is completely unused".
- Add a `bin/drydock reclaim` to send this command to every active resource.
- When we try to acquire a resource and can't, but only because of some kind of limit / utilization problem, try to release an unused resource to free up some room.
Test Plan:
- Set "Working Copy" resource limit to 1.
- Ran "Test Configuration" in `rA`, which worked.
- Ran "Test Configuration" in `rB`, which hung forever.
- Applied patch.
- Ran "Test Configuration" in `rB`, saw it reclaim the `rA` resource, use the slot, then succeed.
- Ran "Test Configuration" in `rA` again, saw it grab the slot back.
- Ran `bin/drydock reclaim` and saw it reclaim a bunch of old orphaned resources.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T9994
Differential Revision: https://secure.phabricator.com/D14819
Summary:
Ref T9994. This fixes the first issue discussed on that task, which is that when a merge fails after "arc land", we would not clean up all the leases properly.
Specifically, when a merge fails, we use `queueTask()` to schedule a followup task. This followup destroys the lease and frees the underlying resource.
However, the default behavior of `queueTask()` is to //not queue tasks// if the parent task fails. This is a reasonable, safe behavior that was originally introduced in D8774, where it kept us from sending too much mail if a task did "send some mail" and then failed a little later on and got retried.
Since I think the default behavior is correct, I just special cased the behavior for Drydock to make it queue even on failure. These are the only types of followup tasks we currently want to queue on main task failure.
(It's possible that future Blueprints might want some kind of more specialized behavior, where some tasks queue only on success, but we can cross that bridge when we come to it.)
Test Plan:
- See T9994#149878 for test case setup.
- I ran that test case again with this patch, and saw the followup task queue properly in the `--trace` log, a correspoinding update task show up in `/daemon/`, and the lease get destroyed when I ran it a moment later.
{F1029915}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T9994
Differential Revision: https://secure.phabricator.com/D14818
Summary: Ref T10003. Give installs more warning about these changes.
Test Plan: Changed configuration, saw warning. Reset configuration, no warning.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10003
Differential Revision: https://secure.phabricator.com/D14817
Summary: Ref T10004. Happy to take another approach here or just not bother, this just struck me as a little ambiguous/confusing.
Test Plan:
Before, not necessarily clear that the "Create Task" header only applies to the first few items.
{F1029126}
After, more clear:
{F1029127}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10004
Differential Revision: https://secure.phabricator.com/D14815
Summary: Fixes T9871. Ref T10004. These won't win any awards but it fixes them being incredibly weird and confusing.
Test Plan:
{F1029090}
- Tried to use controls, got reasonable behavior.
- Used normal controls to make sure I didn't break anything.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T9871, T10004
Differential Revision: https://secure.phabricator.com/D14814
Summary:
At least for now, the "Space" field is just a subfield of the "Visible To" field, so:
- it doesn't get any separate settings; and
- it always uses the "Visible To" settings.
Test Plan:
- Created a form with a hidden view policy field.
- Created stuff with no "you must pick a space" errors.
- Created stuff with a normal form.
- Prefilled "Space" on a noraml form.
- Verified that trying to prefill "Space" on a form with "Visible To" hidden does nothing.
Reviewers: chad
Reviewed By: chad
Differential Revision: https://secure.phabricator.com/D14812
Summary:
Ref T10004. Fixes T9527. Currently, we render two kinds of bad policy/space transactions during object creation.
First, we render a transaction showing a change from the default policy/space to the selected policy/space:
> alice shifted this object from space S1 Default to space S2 Secret.
This is a //good transaction// (it's showing that the default was changed, which could be important for policy stuff!) but it's confusing because it makes it sound like the object briefly existed in space S1, when it did not.
Instead, render this:
> alice created this object in space S2 Secret.
This retains the value (show that the object was created in an unusual space) without the confusion.
Second, when you create a "New Bug Report", we render a transaction like this:
> alice changed the visibility of this task from "All Users" to "Community".
This is distracting and not useful, becasue it's a locked default of the form. This was essentially fixed by D14810. The new behavior is to show this, //only// if the value was changed from the form value:
> alice created this object with visibility "Administrators".
This should reduce confusion, reduce fluff in the default cases, and do a better job of calling out important changes (basically, unusual spaces/policies).
Test Plan:
- Created an edit form with a default space and policies.
- Used that form to create task with:
- same values as form;
- different values from form.
When I changed the form value, I got transactions. When I left it the same, I didn't.
The transactions rendered in the non-confusing "created with ..." variant.
Editing the values created normal transactions with "changed policy from X to Y".
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T9527, T10004
Differential Revision: https://secure.phabricator.com/D14811
Summary:
Fixes T7661. Ref T9527.
When you create a task, especially with an EditEngine form, you currently get more noise than is useful. For example:
> alice created this task.
> alice changed the edit policy from "All Users" to "Community (Project)".
> alice added projects: Feature Request, Differential.
> alice added a subscriber: alice.
Transaction (1) is a little useful, since it saves us from a weird empty state and shows the object creation time.
Transaction (2) is totally useless (and even misleading) because that's the default policy for the form.
Transaction (3) isn't //completely// useless but isn't very interesting, and probably not worth the real-estate.
Transaction (4) is totally useless.
(These transactions are uniquely useless when creating objects -- when editing them later, they're fine.)
This adds two new rules to hide transactions:
- Hide transactions from object creation if the old value is empty (e.g., set title, set projects, set subscribers).
- Hide transactions from object creation if the old value is the same as the form default value (e.g., set policy to default, set priorities to default, set status to default).
NOTE: These rules also hide the "created this object" transaction, since it's really one of those transaction types in all cases. I want to keep that around in the long term, but just have it be a separate `TYPE_CREATE` action -- currently, it is this weird, inconsistent action where we pick some required field (like title) and special-case the rendering if the old value is `null`. So fixing that is a bit more involved. For now, I'm just dropping these transactions completely, but intend to restore them later.
Test Plan:
- Created objects.
- Usually saw no extra create transactions.
- Saw extra create transactions when making an important change away from form defaults (e.g., overriding form policy).
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T7661, T9527
Differential Revision: https://secure.phabricator.com/D14810
Summary:
Ref T10004. Fixes T5158. There was a long-standing issue with defaults not working properly, but EditEngine has made it more obvious because it's a lot easier to set defaults now.
The issue is basically that the defaults are getting set as the field's real value early on, so when we go to generate the transaction "old value" later, we build a transaction that uses the //new// value as both the "new value" and "old value". Then the engine says "you didn't change anything, so I'm going to ignore this" and drops it.
To fix this, return `null` as the "old value" by default, and add a call to overwrite that after we load a legitimate old value.
This fix is a touch iffy, but I have some grand plans to clean up the CustomField stuff more broadly later on.
Test Plan:
- Set config defaults on select/typeahead fields, created and edited tasks.
- Set form defaults on select/typehaead fields, created and edited tasks.
- In all cases, transactions and state accurately reflected edits.
- Set defaults on //hidden// fields, verified forms respected them correctly.
- This does generate some fluffy transactions, but I'll deal with those in T7661.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T5158, T10004
Differential Revision: https://secure.phabricator.com/D14809
Summary:
Ref T10004. Tweaks some of the UX a little to be more intuitive/inviting?
- Button says "Configure Form" instead of "Actions".
- Root list is less "developer-ey" and more "explain what this is for-ey".
Test Plan:
{F1028928}
{F1028929}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10004
Differential Revision: https://secure.phabricator.com/D14808
Summary:
Ref T10004. Currently, when a logged-out user visits an application like Maniphest, we show them a disabled "Create Task" button with no dropdown menu.
This is technically correct in some sense because none of the items in the menu will work, but we can be more helpful and show the items, just in a disabled state:
{F1028903}
When the user clicks these, they'll be pushed through the login flow and (after D14804) end up on the same page they were on when they selected the item. From here, they can proceed normally.
I changed "...to continue." to "...to take this action." to hopefully be a little more clear. In particular, we do not //continue// the action after you log in: you end up back on the same page you started on. For example, if you clicked "Create New Bug" from the list view, you end up back on the list view and need to click "Create New Bug" again. If you clicked "Edit Task" from some task detail page, you end up on the task detail page and have to click "Edit Task" again.
I think this behavior is always very good. I think it is often the best possible behavior: for actions like "Edit Blocking Tasks" and "Merge Duplicates In", the alternatives I can see are:
- Send user back to task page (best?)
- Send user to standalone page with weird dialog on it and no context (underlying problem behavior all of this is tackling, clearly not good)
- Send user back to task page, but with dialog open (very complicated, seems kind of confusing/undesirable?)
For actions like "Create New Bug" or "Edit Task", we have slightly better options:
- Send user back to task page (very good?)
- Send user to edit/create page (slightly better?)
However, we have no way to tell if a Workflow "makes sense" to complete in a standalone way. That is, we can't automatically determine which workflows are like "Edit Task" and which workflows are like "Merge Duplicates In".
Even within an action, this distinction is not straightforward. For example, "Create Task" can standalone from the Maniphest list view, but should not from a Workboard. "Edit Task" can standalone from the task detail page, but should not from an "Edit" pencil action on a list or a workboard.
Since the simpler behavior is easy, very good in all cases, often the best behavior, and never (I think?) confusing or misleading, I don't plan to puruse the "bring you back to the page, with the dialog open" behavior at any point. I'm theoretically open to discussion here if you REALLY want the dialogs to pop open magically but I think it's probably a lot of work.
Test Plan: As a logged out user, clicked "Create Task". Got a dropdown showing the options available to me if I log in. Clicked one, logged in, ended up in a reasonable place (the task list page where I'd started).
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10004
Differential Revision: https://secure.phabricator.com/D14806
Summary:
Ref T10004. After D14804, we get this behavior by default and no longer need to set it explicitly.
(If some endpoint did eventually need to set it explicitly, it could just change what it passes to `setHref()`, but I believe we currently have no such endpoints and do not foresee ever having any.)
Test Plan:
- As a logged out user, clicked various links in Differential, Maniphest, Files, etc., always got redirected to a sensible place after login.
- Grepped for `setObjectURI()`, `getObjectURI()` (there are a few remaining callsites, but to a different method with the same name in Doorkeeper).
Reviewers: chad
Reviewed By: chad
Subscribers: hach-que
Maniphest Tasks: T10004
Differential Revision: https://secure.phabricator.com/D14805
Summary:
Ref T10004. After a user logs in, we send them to the "next" URI cookie if there is one, but currently don't always do a very good job of selecting a "next" URI, especially if they tried to do something with a dialog before being asked to log in.
In particular, if a logged-out user clicks an action like "Edit Blocking Tasks" on a Maniphest task, the default behavior is to send them to the standalone page for that dialog after they log in. This can be pretty confusing.
See T2691 and D6416 for earlier efforts here. At that time, we added a mechanism to //manually// override the default behavior, and fixed the most common links. This worked, but I'd like to fix the //default// beahvior so we don't need to remember to `setObjectURI()` correctly all over the place.
ApplicationEditor has also introduced new cases which are more difficult to get right. While we could get them right by using the override and being careful about things, this also motivates fixing the default behavior.
Finally, we have better tools for fixing the default behavior now than we did in 2013.
Instead of using manual overrides, have JS include an "X-Phabricator-Via" header in Ajax requests. This is basically like a referrer header, and will contain the page the user's browser is on.
In essentially every case, this should be a very good place (and often the best place) to send them after login. For all pages currently using `setObjectURI()`, it should produce the same behavior by default.
I'll remove the `setObjectURI()` mechanism in the next diff.
Test Plan: Clicked various workflow actions while logged out, saw "next" get set to a reasonable value, was redirected to a sensible, non-confusing page after login (the page with whatever button I clicked on it).
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10004
Differential Revision: https://secure.phabricator.com/D14804
Summary:
Fixes T6864. This creates a sort of busy menu but I think that's proably fine -- users are opting into activating these fields for search anyway.
In the future, we could refine this as, e.g.:
- don't show these options in the dropdown;
- do show them on some new "http prefilling" sort of page;
- then you access them as an advanced user with `?order=secret-magic`.
But I'm not going to bother for now.
Test Plan: Ordered by an int field, then reversed the order.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T6864
Differential Revision: https://secure.phabricator.com/D14800
Summary: This seems to work, but I couldn't figure out how to pass over a Caption for a text field.
Test Plan: New blog, Edit blog.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D14770
Summary:
Ref T9992. This is a step on the path to getting EditEngine working in Badges, Projects and Calendar.
This doesn't add a new `EditField` for icons yet, just standardizes the old stuff. New stuff is more general and I saved 150 lines of code.
I put the endpoint in Files because the similar "choose a profile picture" endpoint will definitely go there, and this endpoint might eventually feature, like, "draw your own icon~~" or something.
Test Plan:
- Created events, projects and badges with custom icons.
- Edited events, projects and badges, changing their icons.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T9992
Differential Revision: https://secure.phabricator.com/D14799
Summary: Ref T9964. Create some docuemntation for this stuff, and clean up the *.edit endpoints a bit.
Test Plan: Read documentation.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T9964
Differential Revision: https://secure.phabricator.com/D14798
Summary:
Ref T9964. Three goals here:
- Make it easier to supply Conduit documentation.
- Make automatic documentation for `*.edit` endpoints more complete, particularly for custom fields.
- Allow type resolution via Conduit types, so you can pass `["alincoln"]` to "subscribers" instead of needing to use PHIDs.
Test Plan:
- Viewed and used all search and edit endpoints, including custom fields.
- Used parameter type resolution to set subscribers to user "dog" instead of "PHID-USER-whatever".
- Viewed HTTP parameter documentation.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T9964
Differential Revision: https://secure.phabricator.com/D14796
Summary:
See T9905#148799. The CommentEditField generated empty comment transactions; these are dropped later, but before they are dropped they would trigger implicit CCs.
The implicit CC rule should probably be narrower, but we shouldn't be generating these transactions in the first place.
Test Plan: No longer implicitly CC'd on a task when doing something minor like changing projects.
Reviewers: chad
Reviewed By: chad
Subscribers: avivey
Differential Revision: https://secure.phabricator.com/D14795
Summary:
Ref T9964.
- New mechanism for rich documentation on unusual/complicated edits.
- Add some docs to `paths.set` since it's not self-evident what you're supposed to pass in.
Test Plan: {F1027177}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T9964
Differential Revision: https://secure.phabricator.com/D14791
Summary: Ref T9964. Fixes T9752. Provides API access to enable/disable packages and change their paths.
Test Plan:
- Changed status via Conduit.
- Changed paths via Conduit.
- Tried to change a path use a nonsense/bogus repository PHID, got an error.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T9752, T9964
Differential Revision: https://secure.phabricator.com/D14790
Summary:
Ref T9908. Fixes T6205.
This is largely some refactoring to improve the code. The new structure is:
- Each EditField has zero or one "submit" (normal edit form) controls.
- Each EditField has zero or one "comment" (stacked actions) controls.
- If we want more than one in the future, we'd just add two fields.
- Each EditField can have multiple EditTypes which provide Conduit transactions.
- EditTypes are now lower-level and less involved on the Submit/Comment pathways.
Test Plan:
- Added and removed projects and subscribers.
- Changed task statuses.
- In two windows: added some subscribers in one, removed different ones in the other. The changes did not conflict.
- Applied changes via Conduit.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T6205, T9908
Differential Revision: https://secure.phabricator.com/D14789
Summary:
Fixes T9997. This was in the database since v0, I just never hooked up the UI since it wasn't previously meaningful.
However, it now makes sense to have a provider like Asana with login disabled and use it only for integrations.
Test Plan: Disabled login on a provider, verified it was no longer available for login/registration but still linkable.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T9997
Differential Revision: https://secure.phabricator.com/D14794
Summary:
Fixes T9995. I think letting users customize slugs is not a hugely compelling as a product feature, and this fixes the issue with slugs that have "/" characters in them and makes the move to EditEngine easier since I don't have to deal with the weird JS thing.
Instead, just generate slugs automatically. No more JS, no more separate field, things automatically update if you rename a blog, and now that URIs have IDs in them the old URI will still work after a rename.
Test Plan:
- Applied migration.
- Created new posts.
- Edited existing posts.
- Visited various posts.
- Created a post with a bunch of "/" in the title, things still worked fine.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T9995
Differential Revision: https://secure.phabricator.com/D14792
Summary: Ref T9964. Add a `setIsConduitOnly()` method so we can mark a field as API-only.
Test Plan:
- Created and edited pastes via web UI (no status field).
- Adjusted status via web UI action.
- Adjusted status via Conduit API.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T9964
Differential Revision: https://secure.phabricator.com/D14788
Summary: Fixes T9988. This logic got inverted by accident at some point.
Test Plan:
- Edited a task, shifting spaces.
- Created a task in default space.
- Created a task in custom space.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T9988
Differential Revision: https://secure.phabricator.com/D14787
Summary: Ref T9983. This method is spelled wrong.
Test Plan: Hit this case, got a dialog instead of a fatal.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T9983
Differential Revision: https://secure.phabricator.com/D14786
Summary:
Fixes T9984. When a tokenizer only allows one selection (like "Task Owner:" or "Land Onto Branch:"), keep the browse button active but have it //replace// values.
Also, have "Create Subtask" default to the system default status, so subtasks of closed tasks are not also closed.
Test Plan:
- Browsed an empty limit=1 tokenizer.
- Replaced a full limit=1 tokenizer.
- Browsed an empty no-limit tokenizer.
- Browsed more tokens into the no-limit tokenizer.
- Typed some tokens normally.
- Created a subtask of a closed task.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T9984
Differential Revision: https://secure.phabricator.com/D14785
Summary:
Ref T9980. No magic here, just write a little bit about how to find outdated callers. Update the technical doc.
Also:
- Fix an unrelated bug where you couldn't leave comments if an object had missing, required, custom fields.
- Restore the ConduitConnectionLog table so `bin/storage adjust` doesn't complain.
Test Plan: Read docs.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T9980
Differential Revision: https://secure.phabricator.com/D14784
Summary:
Ref T9908. These meta-edit-engines are used to generate the main editengine UIs, but they're also editable.
Fix an exception when trying to edit the meta editengine.
Test Plan: Edited editengineconfiguration editengine.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T9908
Differential Revision: https://secure.phabricator.com/D14783
Summary:
Ref T9980. By default, show the viewer //their// calls.
Make it easy to find their own deprecated calls.
I don't like the word "My" but couldn't come up with anything better that didn't feel like a big loss of clarity.
The permissions on this log are also a little weird: non-admins can see everyone else's calls.
I think we should eventually lock that down, but plan to keep it this way for now:
First, a lot of your calls end up with no caller set right now, because we don't set the caller early enough in the process so a lot differnet types of errors can leave us with no user on the log. Fixing that isn't trivial, and users may reasonably want to access to these "no caller" logs to check for errors or debug stuff.
Second, none of it is really that sensitive?
Third, it's reasonable for users to want to look at bots?
I'd plan to maybe do this eventually:
- Make the caller get populated more often after auth code is simplified.
- Only let users look at their calls and maybe bot calls and anonymous calls.
- Let admins look at everything.
But for now everyone can see everything.
Test Plan: {F1025867}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T9980
Differential Revision: https://secure.phabricator.com/D14782
Summary: Ref T9980. This makes it much easier to look for calls to deprecated methods.
Test Plan: {F1025851}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T9980
Differential Revision: https://secure.phabricator.com/D14781
Summary:
Ref T5955, T9980, T9982.
We currently store two types of Conduit logs: //connection// logs and //method// logs.
Originally, Conduit worked like web logins: you'd call `conduit.connect` and then get a session back. This approach still works, but new clients don't use it and it will probably stop working eventually after T5955 is further along.
There was no real reason for things to work like this and no other API in the world does, I think it was just slightly easier to implement back in 2011.
This table was used to group up related calls in a UI long ago, I think, but that got deleted at some point. In any case, it serves no purpose in modern Phabricator.
Test Plan: `grep`
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T5955, T9980, T9982
Differential Revision: https://secure.phabricator.com/D14780
Summary:
Ref T9980. Start making this UI more useful and powerful so we can give administrators a better toolset for reacting to API changes.
Fixes T9755. We were logging the caller, just not rendering it properly.
Test Plan: {F1025799}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T9755, T9980
Differential Revision: https://secure.phabricator.com/D14779
Summary: Ref T9980. I don't think this is actually useful, and plan to give users and administrators more powerful tools instead.
Test Plan: Loaded setup warnings.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T9980
Differential Revision: https://secure.phabricator.com/D14778
Summary: Ref T9964. Priorities and statuses have metadata (colors, names, etc) which we can reasonably just return from API callers. This is so ligthweight that I think it doesn't really make sense to put in an attachment. We also use this information in `arc tasks`.
Test Plan: Called API, got sensible looking status and priority data back.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T9964
Differential Revision: https://secure.phabricator.com/D14777
Summary: Ref T9964. This just adds more structure to application fields, to make it harder to make typos and easier to validate them later.
Test Plan: Viewed APIs, called some APIs, saw good documentation and correct results.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T9964
Differential Revision: https://secure.phabricator.com/D14776
Summary:
Ref T9964.
- Add a "paths" attachment for fetching paths.
- Always load owners. We will need this to do policy checks in the future, anyway, and this data is not large, is very useful, and is reasonable to load unconditionally.
Test Plan:
- Queried packages via API.
- Edited packages (paths, owners).
- Created a package.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T9964
Differential Revision: https://secure.phabricator.com/D14775
Summary: Ref T9964. Builds on D14772. Allows callers to get the raw content of pastes as an attachment.
Test Plan:
- Read docs.
- Executed attachment query.
- Saw raw paste content.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T9964
Differential Revision: https://secure.phabricator.com/D14774
Summary: Ref T9964. Builds on D14772. Allows callers to request project PHIDs for objects.
Test Plan: {F1025468}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T9964
Differential Revision: https://secure.phabricator.com/D14773
Summary:
Ref T9964. We have various kinds of secondary data on objects (like subscribers, projects, paste content, Owners paths, file attachments, etc) which is somewhat slow, or somewhat large, or both.
Some approaches to handling this in the API include:
- Always return all of it (very easy, but slow).
- Require users to make separate API calls to get each piece of data (very simple, but inefficient and really cumbersome to use).
- Implement a hierarchical query language like GraphQL (powerful, but very complex).
- Kind of mix-and-match a half-power query language and some extra calls? (fairly simple, not too terrible?)
We currently mix-and-match internally, with `->needStuff(true)`. This is not a general-purpose, full-power graph query language like GraphQL, and it occasionally does limit us.
For example, there is no way to do this sort of thing:
$conpherence_thread_query = id(new ConpherenceThreadQuery())
->setViewer($viewer)
// ...
->setNeedMessages(true)
->setWhenYouLoadTheMessagesTheyNeedProfilePictures(true);
However, we almost never actually need to do this and when we do want to do it we usually don't //really// want to do it, so I don't think this is a major limit to the practical power of the system for the kinds of things we really want to do with it.
Put another way, we have a lot of 1-level hierarchical queries (get pictures or repositories or projects or files or content for these objects) but few-to-no 2+ level queries (get files for these objects, then get all the projects for those files).
So even though 1-level hierarchies are not a beautiful, general-purpose, fully-abstract system, they've worked well so far in practice and I'm comfortable moving forward with them in the API.
If we do need N-level queries in the future, there is no technical reason we can't put GraphQL (or something similar) on top of this eventually, and this would represent a solid step toward that. However, I suspect we'll never need them.
Upshot: I'm pretty happy with "->needX()" for all practical purposes, so this is just adding a way to say "->needX()" to the API.
Specifically, you say:
```
{
"attachments": {
"subscribers": true,
}
}
```
...and get back subscriber data. In the future (or for certain attachments), `true` might become a dictionary of extra parameters, if necessary, and could do so without breaking the API.
Test Plan:
- Ran queries to get attachments.
{F1025449}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T9964
Differential Revision: https://secure.phabricator.com/D14772
Summary:
Ref T9964. I added several hacks to get these working. Clean them up and pull this into a proper extension.
The behavior in the web UI is:
- they work in all applications; but
- they only show up in the UI if a value is specified.
So if you visit `/view/?ids=1,2` you get the field, but normally it's not present. We could refine this later. I'm going to add documentation about how to prefill these forms regardless, which should make this discoverable by reading the documentation.
There's one teensey weensey hack: in the API, I push these fields to the top of the table. That one feels OK, since it's purely a convenience/display adjustment.
Test Plan: Queried by IDs, reviewed docs.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T9964
Differential Revision: https://secure.phabricator.com/D14769
Summary:
Ref T9964. Building tables in Remarkup is kind of neat-ish but ends up feeling kind of hacky, and requires weird workarounds if any of the values have `|` in them.
Switch to normal elements instead.
Also move the magic "ids" and "phids" to be more like real fields. I'll clean this up fully in a diff or two, it's just a little tricky because Maniphest has an "ids" field.
Test Plan: {F1024294}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T9964
Differential Revision: https://secure.phabricator.com/D14768
Summary: Ref T9964. I left a couple of these unsupported for now since they're weird in some way.
Test Plan: {F1024031}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T9964
Differential Revision: https://secure.phabricator.com/D14767
Summary:
Ref T9964. Fill in more parameter types and descriptions.
(No date support yet since it's a bit more involved.)
Test Plan: {F1024022}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T9964
Differential Revision: https://secure.phabricator.com/D14766
Summary: Ref T9964. This fills in types and descriptions for ApplicationSearch fields in Paste.
Test Plan:
Got this nice table now:
{F1023999}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T9964
Differential Revision: https://secure.phabricator.com/D14765
Summary:
Ref T9964. ApplicationSearch currently has a bunch of hard-coded `if ($object instanceof thing)` stuff.
Pull that out so it can live in extensions.
Test Plan:
- Searched by spaces, subscribers, projects.
{F1023921}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T9964
Differential Revision: https://secure.phabricator.com/D14764
Summary:
Ref T9964. I want to show users what we're expecting in "constraints", and let constraints like "authors=epriestley" work to make things easier.
I'm generally very happy with the "HTTPParameterType" stuff from EditEngine, so add a parallel set of "ConduitParameterType" classes. These are a little simpler than the HTTP ones, but have a little more validation logic.
Test Plan:
This is really just a proof of concept; some of these fields are now filled in:
{F1023845}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T9964
Differential Revision: https://secure.phabricator.com/D14763
Summary:
Ref T9964. The new `*.search` and `*.edit` methods generate documentation which depends on the viewer.
For example, the `*.search` methods show a reference table of the keys for all your saved queries.
Give them a real viewer to work with.
During normal execution, just populate this viewer with the request's viewer, so `$request->getViewer()` and `$this->getViewer()` both work and mean the same thing.
Test Plan: {F1023780}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T9964
Differential Revision: https://secure.phabricator.com/D14761
Summary: Ref T9964. This is a basic implementation of the new "maniphest.search" endpoint.
Test Plan: Clicked the button in the web UI, got meaningful results back.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T9964
Differential Revision: https://secure.phabricator.com/D14760
Summary:
Ref T9964. Adds a new-style "owners.search" endpoint, and an extension for customfields.
Puts enough indirection in place to give us nice, consistent "custom.key" user-facing keys instead of "std:custom:owners:na0shf9a8dfdsafl" junk.
Test Plan:
- Searched Owners via API.
- Searched by ID.
- Ordered by custom fields.
- Reviewed API docs.
- Used normal search with ordering.
- Viewed custom field values in search results.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T9964
Differential Revision: https://secure.phabricator.com/D14758
Summary: Adds a list of your drafts. Fixes T9927y
Test Plan:
Load up home, see my drafts. Fake 0 drafts, see fallback message.
{F1023139}
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D14756
Summary:
Ref T9360. Fixes the double-rendering of post bodies in feed stories.
Downside is that 'publish' (on its own) no longer shows a body, but that seems fine.
Test Plan:
- Got some double bodies.
- Applied patch.
- No more double bodies.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T9360
Differential Revision: https://secure.phabricator.com/D14748
Summary: Ref T9968. Some of the crumb/route handling wasn't quite tight enough and could hit a fatal.
Test Plan: Hit previously-fataling URI, got a 404 instead.
Reviewers: chad
Reviewed By: chad
Subscribers: starruler
Maniphest Tasks: T9968
Differential Revision: https://secure.phabricator.com/D14747
Summary: Ref T9927. Adds a "Blogs" section to PhameHome. Removes "New Post" Controller. Adds flipped layout for PHUITwoColumnView
Test Plan:
Test PhameHome, Ponder, New Post, etc. Mobile and Desktop states.
{F1022080}
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: johnny-bit, Korvin
Maniphest Tasks: T9927
Differential Revision: https://secure.phabricator.com/D14744
Summary: Fixes T9966. In this unusual, difficult-to-reach case, we throw `Exception` (which has no censoring) instead of `CommandException` (which has censoring). Throw `CommandException` instead.
Test Plan:
- Hacked up a bunch of stuff in order to hit this: disabled origin validation, origin correction, and pointed repository at a bad domain.
- Verified message is now censored correctly.
{F1022217}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T9966
Differential Revision: https://secure.phabricator.com/D14745
Summary:
Ref T9964. See that task for some context and discussion.
Ref T7715, which has the bigger picture here.
Basically, I want Conduit read endpoints to be full-power, ApplicationSearch-driven endpoints, so that applications can:
- Write one EditEngine and get web + conduit writes for free.
- Write one SearchEngine and get web + conduit reads for free.
I previously made some steps toward this, but this puts more of the structure in place.
Test Plan:
Viewed API console endpoint and read 20 pages of docs:
{F1021961}
Made various calls: with query keys, constraints, pagination, and limits.
Viewed new {nav Config > Modules} page.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T7715, T9964
Differential Revision: https://secure.phabricator.com/D14743
Summary: Ref T9897. If you visit `/post/123/spoderman/` it will try to redirect you to `/post/123/spiderman/`, but currently only internal views work because these redirects aren't marked as safe/external.
Test Plan: Visited a misspelled/out-of-date URI on an external blog view, got a good redirect.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T9897
Differential Revision: https://secure.phabricator.com/D14741
Summary:
Ref T9897. Purge a bunch of stuff:
- Remove skins.
- Remove all custom sites for skin resources.
- Remove "framed", "notlive", "preview", separate "live" controllers (see below).
- Merge "publish" and "unpublish" controllers into one.
New behavior:
- Blogs and posts have three views:
- "View": Internal view URI, which is a normal detail page.
- "Internal Live": Internal view URI which is a little prettier.
- "External Live": External view URI for an external domain.
Right now, the differences are pretty minor (basically, different crumbs/chrome). This mostly gives us room to put some milder flavor of skins back later (photography or more "presentation" elements, for example).
This removes 9 million lines of code so I probably missed a couple of things, but I think it's like 95% of the way there.
Test Plan:
Here are some examples of what the "view", "internal" and "external" views look like for blogs (posts are similar):
"View": Unchanged
{F1021634}
"Internal": No chrome or footer. Still write actions (edit, post commments). Has crumbs to get back into Phame.
{F1021635}
"External": No chrome or footer. No write actions. No Phabricator crumbs. No policy/status information.
{F1021638}
I figure we'll probably tweak these a bit to figure out what makes sense (like: maybe no actions on "internal, live"? and "external, live" probably needs a way to set a root "Company >" crumb?) but that they're reasonable-ish as a first cut?
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T9897
Differential Revision: https://secure.phabricator.com/D14740
Summary: edit forms in yo' edit forms
Test Plan: ~(o.o)~
Reviewers: chad
Reviewed By: chad
Differential Revision: https://secure.phabricator.com/D14739
Summary:
Ref T9952. Ref T3462. After D14736, if we have information about the target/"onto" branch, use it in the UI:
- Show "feature (branched from master)" instead of "feature".
- Default "Land Revision" to hit the correct branch.
Test Plan:
- Branched from `test` with branch tracking.
- Diffed.
- Saw "feature (branched from test)" in UI.
- Saw "test" fill as default in "Land Revision", despite the repository having a different default branch.
{F1020587}
{F1020588}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T3462, T9952
Differential Revision: https://secure.phabricator.com/D14737
Summary:
Ref T9952. Default the branch target in the dialog to be whatever branch is the default branch for the repository.
This will be correct for repositories like ours (which land everything into `master`) and correct most of the time for repositories which have some other "primary" branch (maybe `development`).
It won't be great if there are multiple open lines of development in a repository (for example, some changes go to `newdesignpro` and some changes go to `legacy-1.2`). I'll do work in T3462 next to improve those cases so we can pick a better default.
Test Plan:
- Saw dialog default to "master".
- Changed repo default branch, saw it default to "notmaster" instead.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T9952
Differential Revision: https://secure.phabricator.com/D14734
Summary:
Ref T9952. This adds a typeahead so you can pick a branch to target.
It does not choose a default branch, the user must pick a branch explicitly.
Test Plan:
- Landed rGITTESTd587fada48fc to `master` (by typing "master").
- Landed rGITTEST86c339b2ef01 to `notmaster` (by typing "notmaster").
{F1020531}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T9952
Differential Revision: https://secure.phabricator.com/D14733
Summary: Ref T9952. This will let me put a "Branch: [____]" control on the "Land Revision" dialog so users can choose a branch to target.
Test Plan: Used `/typeahead/class/` to vet basic behavior.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T9952
Differential Revision: https://secure.phabricator.com/D14732
Summary: Ref T9952. See discussion there. This change is primarily aimed at letting me build a typeahead of branches in a repository so that we can land to arbitrary branches a few diffs from now.
Test Plan:
- Ran migrations.
- Verified database populated properly with PHIDs (`SELECT * FROM repository_refcursor;`).
- Ran `bin/repository update`.
- Viewed a Git repository in Diffusion.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T9952
Differential Revision: https://secure.phabricator.com/D14731
Summary: If you archive a badge, remove it's presence in the main Phabricator UI. These are still accessible from `/badges/` for properity. Ref T9944
Test Plan: Archive a badge, weep uncontrollably.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Maniphest Tasks: T9944
Differential Revision: https://secure.phabricator.com/D14730
Summary:
Fixes T9945. This is straightforward.
The two sub-object types are very lightweight so I just deleted them directly instead of loading + delete()'ing (or implementing DestructibleInterface on them, which would require they have PHIDs).
Also improve a US English localization.
Test Plan:
- Used `bin/remove destroy PHID-... --trace` to destroy a package.
- Verified it was gone.
- Inspected the SQL in the log for general reasonableness.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T9945
Differential Revision: https://secure.phabricator.com/D14729
Summary: Allows closing a mock from the action list. Ref T9414
Test Plan: New Mock, Edit Mock, Close Mock, Open Mock
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Maniphest Tasks: T9414
Differential Revision: https://secure.phabricator.com/D14726
Summary: Makes this more consistent. Also clean up spacing. Ref T9414
Test Plan: Archive/Activate Paste, Edit Paste
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Maniphest Tasks: T9414
Differential Revision: https://secure.phabricator.com/D14724
Summary: Fixes T9919. We were missing feed strings and US English localizations for some of this stuff.
Test Plan:
Before:
{F1018877}
After:
{F1018879}
{F1018880}
{F1018881}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T9919
Differential Revision: https://secure.phabricator.com/D14721
Summary: Fixes T9941. I think someone from Perforce emailed us about 10 years ago and I added this link in response, but I haven't seen other interest in Perforce since then. Link is now dead.
Test Plan:
- {nav Diffusion > Create Repository > Import Existing}, no more Perforce link.
- Grepped for `perforce`.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T9941
Differential Revision: https://secure.phabricator.com/D14720
Summary:
Ref T9132. I think the featureset is approximatley stable, so here's some documentation.
I also cleaned up a handful of things in the UI and tried to make them more obvious or more consistent.
Test Plan: Read documentation.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T9132
Differential Revision: https://secure.phabricator.com/D14718
Summary: Fixes T9851. I'll hold this for a while to give users some time to update per T9860.
Test Plan:
Edited a task via:
- Conduit
- Comments field
- Edit form
- New task form
Reviewers: chad
Reviewed By: chad
Subscribers: Krenair
Maniphest Tasks: T9851
Differential Revision: https://secure.phabricator.com/D14576
Summary:
Fixes T9920. When hiding changes, tell users why so they can learn the comment rule (usually, "Changes from before your most recent comment are hidden."; sometimes they're hidden for pagination reasons).
Also use "Show Older Comments" instead of "Show older comments." for the action since I think it's a little more consistent to use title case for links/actions?
Test Plan:
- Viewed a task with a lot of comments, saw a "most recent comment" element.
- Artificially set page size to 3, saw a "lots of changes" hide.
- Grepped for removed string.
- Clicked both "show older stuff" links.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T9920
Differential Revision: https://secure.phabricator.com/D14719
Summary: Ref T9908. Move all the "pro" stuff into the old locations.
Test Plan: Created/edited tasks, looked at URIs, saw non-pro ones. Grepped for `editpro`.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T9908
Differential Revision: https://secure.phabricator.com/D14717
Summary:
Ref T9908. No more callsites. Also:
- Phurl a couple of documentation URIs.
- Get rid of "task:" in the global search since it doesn't really make sense anymore.
Test Plan: `grep`, edited/created tasks.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T9908
Differential Revision: https://secure.phabricator.com/D14716
Summary: Ref T9908. This is the last of the things that need to swap over.
Test Plan:
- Created tasks from a workboard.
- Created tasks in different columns.
- Edited tasks.
- Used `?parent=..`.
- Verified that default edit form config now affects comment actions.
- No more weird comment thing on forms, at least for now.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T9908
Differential Revision: https://secure.phabricator.com/D14715
Summary: Ref T9908. This drives workboard card edits through the new stuff. Bit of copy-paste but the old one will get deleted soon.
Test Plan:
- Edited some cards.
- Changed priority on a priority-sorted board, saw proper re-sort
- Removed board project, saw card vanish properly.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T9908
Differential Revision: https://secure.phabricator.com/D14712
Summary: Ref T9908. Fixes T8903. This moves the inline edit from task lists (but not from workboards) over to editengine.
Test Plan:
- Edited a task from a draggable list.
- Edited a task from an undraggable list.
- Edited a task, changed projects, saw refresh show correct projects.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T8903, T9908
Differential Revision: https://secure.phabricator.com/D14711
Summary: Normalize "getViewURI" and "getLiveURI" for PhameBlog and PhamePost. Use pretty URIs in PhamePost
Test Plan: View Recent, View posts, edit post, new post, move post, view live.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D14713
Summary: Ref T9908. This fixes "Create Subtask" so it works with the new stuff. Mostly straightforward.
Test Plan: Created some subtasks.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T9908
Differential Revision: https://secure.phabricator.com/D14706
Summary: Ref T9908. This form has a reasonable behavior now after the global reordering stuff.
Test Plan: Clicked "Edit Task".
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T9908
Differential Revision: https://secure.phabricator.com/D14708
Summary:
A guide to basic skills every software professional should have.
This is so fundamental that I don't think the document is actually helpful, but we can try it I guess.
Test Plan: Reading?
Reviewers: chad
Reviewed By: chad
Subscribers: Shredder121
Differential Revision: https://secure.phabricator.com/D14707
Summary:
Ref T9908. This removes the "Create Another Empty Task", "Create A Similar Task" and "Create Another Subtask of Same Parent Task" workflow callouts on the task detail page, which are actions that show up after creating a task or creating a subtask.
- I think "Create Empty" isn't relevant now that we have "Create Task" nearby and the quick create menu?
- I'm not sure if "Create Similar" is worth keeping. If we do want to retain it, I'd maybe like to find a way to do it generically.
- I'm likewise not sure if "Create another subtask" is worth keeping.
Overall, these actions are weird/unusual and I'm not sure how valuable they are. I'm open to keeping "Similar" and/or "Subtask" but I'd like to verify that they're still valuable and make sure we have a reasonable design for them if we retain them.
For example, if we want to retain "Similar", maybe a better approach is just to add "Create Similar Object" to every action menu (which is now possible for EditEngine applications)? There's at least some interest in "Create Similar Repository" in Diffusion.
Also removes a very very old piece of "attached files" code.
Test Plan: Created and viewed some tasks.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T9908
Differential Revision: https://secure.phabricator.com/D14705
Summary:
Ref T9132. Ref T9908. Puts reordering UI in place:
- For create forms, this just lets you pick a UI display order other than alphabetical. Seems nice to have.
- For edit forms, this lets you create a hierarchy of advanced-to-basic forms and give them different visibility policies, if you want.
Test Plan:
{F1017842}
- Verified that "Edit Thing" now takes me to the highest-ranked edit form.
- Verified that create menu and quick create menu reflect application order.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T9132, T9908
Differential Revision: https://secure.phabricator.com/D14704
Summary:
Ref T9132. Ref T9908. This attempts to move us forward on answering this question:
> Which form gets used when a user clicks "Edit Task"?
One answer is "the same form that was used to create the task". There are several problems with that:
- The form might not exist anymore.
- The user might not have permission to see it.
- Some of the fields might be hidden, essentially preventing them from being edited.
- We have to store the value somewhere and old tasks won't have a value.
- Any instructions on the form probably don't apply to edits.
One answer is "force the default, full form". That's not as problematic, but it means we have no ability to create limited access users who see fewer fields.
The answer in this diff is:
- Forms can be marked as "edit forms".
- We take the user to the first edit form they have permission to see, from a master list.
This allows you to create several forms like:
- Advanced Edit Form (say, all fields -- visible to administrators).
- Basic Edit Form (say, no policies -- visible to trusted users).
- Noob Edit Form (say, no policies, priorities, or status -- visible to everyone).
Then you can give everyone access to "noob", some people access to "basic", and a few people access to "advanced".
This might only be part of the answer. In particular, you can still //use// any edit form you can see, so we could do these things in the future:
- Give you an option to switch to a different form if you want.
- Save the form the task was created with, and use that form by default.
If we do pursue those, we can fall back to this behavior if there's a problem with them (e.g., original form doesn't exist or wasn't recorded).
There's also no "reorder" UI yet, that'll be coming in the next diff.
I'm also going to try to probably make the "create" and "edit" stuff a little more consistent / less weird in a bit.
Test Plan: Marked various forms as edit forms or not edit forms, made edits, hit permissions errors, etc.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T9132, T9908
Differential Revision: https://secure.phabricator.com/D14702
Summary: Fixes T9928. Not sure if this is best mechanic or add new methods to PhamePostQuery (a join?)
Test Plan: Archive a blog, don't see posts on PhameHome
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Maniphest Tasks: T9928
Differential Revision: https://secure.phabricator.com/D14701
Summary:
Ref T9908. Simplify some of the policies here:
- If you can edit an application (currently, always "Administrators"), you can view and edit all of its forms.
- You must be able to edit an application to create new forms.
- Improve some error messages.
- Get about halfway through letting users reorder forms in the "Create" menu if they want to sort by something weird since it'll need schema changes and I can do them all in one go here.
Test Plan:
- Tried to create and edit forms as an unprivileged user.
- Created and edited forms as an administrator.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T9908
Differential Revision: https://secure.phabricator.com/D14700
Summary: Moves New Post and Move Post to be separate Controllers with Dialogs. Ref T9897
Test Plan: Move a post to a new blog, see message and see post. Click New Post, get dialog, pick blog, edit new post.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Maniphest Tasks: T9897
Differential Revision: https://secure.phabricator.com/D14698
Summary:
Ref T9132. Ref T9908. Fixes T5622. This allows you to copy some fields (projects, subscribers, custom fields, some per-application) from another object when creating a new object by passing the `?template=xyz` parameter.
Extend "copy" support to work with all custom fields.
Test Plan:
- Created new pastes, packages, tasks using `?template=...`
- Viewed new template docs page.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T5622, T9132, T9908
Differential Revision: https://secure.phabricator.com/D14699
Summary:
Ref T9908. We can get a double-header with this ("Quick Create", "Create Task") which looks weird.
The behavior of this menu is probably obvious enough on its own from context and the "+" icon.
Test Plan: Opened menu, no more "Quick Create" item.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T9908
Differential Revision: https://secure.phabricator.com/D14696
Summary:
Ref T9908. This is a behavioral change:
- Old behavior: "Subscribers" field is default-populated with author.
- New behavior: this transaction is just created no matter what.
The new behavior is much easier to make work with form defaults, hidden fields, etc. For example, on the "Create Bug" form, I've hidden "Subscribers", but I still want the author to be subscribed.
And if a user sets the default value of "Subscribers:" to "Alice, Bob", they almost certainly mean "Alice, Bob, and the task author".
And I ended up deleting myself by accident way more often than I deleted myself on purpose -- especially with "Create Similar task", I'd sometimes delete all the CCs and delete myself by accident and then have to put myself back.
Finally, technically speaking, restoring the old behavior is kind of hard/messy and this is much easier.
Test Plan:
- Created a task.
- Was automatically added as a subscriber.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T9908
Differential Revision: https://secure.phabricator.com/D14694
Summary:
Ref T9908. When there are custom / renamed / policy considerations for applications, respect them in the quick create menu.
This has some performance implications, in that it makes every page slower by two queries (and potentially more, soon), which is quite bad. I have some ideas to mitigate this, but it's not the end of the world to eat these queries for now.
Test Plan: {F1017316}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T9908
Differential Revision: https://secure.phabricator.com/D14693
Summary: Ref T9908. Now that you can submit multiple actions, you can "Open + Assign/Claim" a closed task, which is a reasonable action.
Test Plan: Assign/claim'd a closed task.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T9908
Differential Revision: https://secure.phabricator.com/D14692
Summary:
Ref T9908.
- You should not need edit permission on a task in order to comment on it.
- At least for now, ignore any customization in Conduit and Stacked Actions. These UIs always use the full edit form as it's written in the application.
Test Plan:
- Verified a non-editor can now comment on tasks they can see.
- Verified a user still can't use an edit form they can't see.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T9908
Differential Revision: https://secure.phabricator.com/D14691
Summary: Fixes T6132. We currently allow invalid configuration here; validate it.
Test Plan: Tried to save invalid config (negative priorities, string priorities, etc).
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T6132
Differential Revision: https://secure.phabricator.com/D14689
Summary:
Fixes T8707. See that task for discussion. Browser behavior is apparently to ignore a newline immediately following a `<textarea>`, and ostensibly has been since the early 1800s.
This is the same fix Rails used when it encountered this issue in 2011, which gives me some confidence it is correct.
Test Plan:
- Edited a task with leading newlines in the description.
- Newlines were preserved correctly across multiple edits.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T8707
Differential Revision: https://secure.phabricator.com/D14688
Summary: I wrote this earlier in D14680 but have now realized that it's the same sentence twice when read carefully.
Test Plan: read more carefully
Reviewers: chad
Reviewed By: chad
Differential Revision: https://secure.phabricator.com/D14687
Summary: Update Ponder Questions and Answers to render Remarkup in Feed
Test Plan: New Question, Edit Question, New Answer, Edit Answer, New Comment. See //remarkup// in Feed.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Maniphest Tasks: T9825
Differential Revision: https://secure.phabricator.com/D14648
Summary:
- ipull there is wrong
- The `+` wasn't doing what I thought it was doing.
- I already forgot what that detour was doing, so I wrote it down.
Test Plan: Load Versions page, see no error log.
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: epriestley
Differential Revision: https://secure.phabricator.com/D14686
Summary: Fixes T5788. We already have this as a pre-commit field, add it as a post-commit field too.
Test Plan: Ran this rule on a merge commit. Also ran it on a non-merge commit. Both got the correct value.
Reviewers: avivey, chad
Reviewed By: avivey, chad
Subscribers: avivey
Maniphest Tasks: T5788
Differential Revision: https://secure.phabricator.com/D14685
Summary:
Fixes T9206. This was also blocked on tokenizers being weird.
Also clean up some rendering stuff from the earlier changes.
Test Plan:
- Added an "unassign" rule by typing "None", per instructions in the placeholder text.
- Ran the rule.
- Task got unassigned.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T9206
Differential Revision: https://secure.phabricator.com/D14684
Summary:
Ref T7848. This is a companion to "Change status to: ...".
(I'm pretty sure the only reason I didn't originally write these was the tokenizer bug in D14682, I just forgot about it).
This is basically a copy/paste of the "status" action.
Test Plan:
- Wrote a rule to change task priorities.
- Edited a task.
- Saw rule fire properly.
- Tokens also stick around correctly.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T7848
Differential Revision: https://secure.phabricator.com/D14683
Summary:
Fixes T7848. @jasonfsmitty discussed an issue in great detail there and in D14359, and I completely missed it. Specifically:
- If you save a "Change status to: Open" rule in Maniphest, and then edit it again, the token shows "Unknown Object (???)" instead of the correct token.
- That's because loadHandles() has no idea what to do with the value "open", since it's not a real PHID.
The way we render tokenizer tokens in Herald is quite hacky right now. Fortunately, I wrote a //slightly// better way for EditEngine yesterday or the day before. Use the slightly better way to fix the issue with D14359.
This could still be better than it is, but the badness is mostly hidden now and can be cleaned up later without impacting anything.
Test Plan: Edited a Herald rule with projects and status changes, saw proper tokens.
Reviewers: chad
Reviewed By: chad
Subscribers: jasonfsmitty
Maniphest Tasks: T7848
Differential Revision: https://secure.phabricator.com/D14682
Summary: Fixes T9496. If you have some statuses or priorities you don't need, allow users to disable them to stop the bleeding.
Test Plan:
- Set task to status X and priority Y.
- Disabled X and Y using config.
- Verified task still had old status/priority.
- Verified new task could not be created/edited into those settings.
- Verified task/priority appeared in typeahead, but were marked as disabled.
- Viewed email command docs.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T9496
Differential Revision: https://secure.phabricator.com/D14681
Summary:
Ref T7848. This patch is incomplete and has the following issues:
- Multiple statuses can be entered on the edit rule page (only the first one is used).
- Statuses are not rendered correctly when re-editing a rule.
Test Plan: Applied to our local phab instance and verified it works with our task workflow.
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: joshuaspence, revi, epriestley, jsmith
Maniphest Tasks: T7848
Differential Revision: https://secure.phabricator.com/D14359
Summary:
Fixes T7250. Currently, if a display group of transactions (multiple transactions by the same author in a short period of time with no intervening comments) has several transactions of similar strength (e.g., several status change transactions) we can end up displaying them in reverse chronological order, which is confusing.
Instead, make sure transactions of the same type/strength are always in logical order.
Test Plan:
- Merged a task into another task, then reopened the merged task.
- Before patch: merge/reopen showed in wrong order.
{F1014954}
- After patch: merge/reopen show in correct order.
{F1014955}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T7250
Differential Revision: https://secure.phabricator.com/D14680
Summary: For consistency, plus I ignore these. Ref T9897
Test Plan: Change to notify, log into notchad, subscribe, change back, see notification instead of email.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Maniphest Tasks: T9909, T9897
Differential Revision: https://secure.phabricator.com/D14676
Summary: This just pretties up some config like `maniphest.custom-field-definitions` which I noticed was kind of hard to read while chasing down other stuff.
Test Plan: {F1014940}
Reviewers: chad
Reviewed By: chad
Differential Revision: https://secure.phabricator.com/D14679
Summary: Ref T9132. See T9908#147038.
Test Plan:
- As user A, created a new task.
- As user B, started typing a comment on it (with no prior activity).
- Users A and B must be different.
Before patch: preview/draft don't work, trace in error log (see above).
After patch: preview/draft work.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T9132
Differential Revision: https://secure.phabricator.com/D14678
Summary:
Ref T9132. Fixes T4580. Thhat might actually have been fixed a while ago or something since it describes a buggy/bad interaction which doesn't reproduce for me at HEAD.
This saves and restores all the stacked actions (subscribers, projects, etc) so that you don't lose anything if you close a window by accident.
Test Plan:
Added a bunch of actions in various states, reloaded the page, draft stuck around.
Submitted form, actions didn't stick around anymore.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T4580, T9132
Differential Revision: https://secure.phabricator.com/D14675
Summary: Ref T9132. I can't actually get rid of the EditController yet since a few weird things still use it, but I think I can swap this button out without breaking anything. This will let us do "New Feature Request" / "New Bug" / "Advanced Task Creation" on secure and start playing with this stuff sooner.
Test Plan: Clicked "Create Task", got sent to new form.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T9132
Differential Revision: https://secure.phabricator.com/D14673
Summary:
Ref T9132. We currently have an old preview/draft behavior and a new actions behavior.
Let the actions behavior do drafts/previews too, so we can eventually throw away the old thing.
This is pretty much just copying the old behavior into the new one, but with a few tweaks. The major change is that we submit all the stacked actions behavior now, so the preview reflects everything the change will do (and, soon, we can save it in the draft in a consistent way).
Also includes one hack-fix that I'll clean up at some point.
Test Plan: Added a bunch of stacked actions and observed meaningful previews.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T9132
Differential Revision: https://secure.phabricator.com/D14672
Summary:
Ref T9132. Open to discussion here since it's mostly product stuff, but here's my gut on this:
- Change Maniphest behavior to stop assigning tasks if they're unassigned when closed. I think this behavior often doesn't make much sense. We'll probably separately track "who closed this" in T4434 eventually.
- Only add the actor as a subscriber if they comment, like in other applications. Previously, we added them as a subscriber for other types of changes (like priority and status changes). This is more consistent, but open to retaining the old behavior or some compromise between the two.
- Retain the "when changing owner, subscribe the old owner" behavior.
Test Plan:
- Added a comment, got CC'd.
- Changed owners, saw old owner get CC'd.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T9132
Differential Revision: https://secure.phabricator.com/D14670
Summary:
Ref T9132. This is kind of a mess because the tokenizer rewrite left rendering tokenizers in Javascript a little rough. This causes bugs like icons not showing up on tokens in the "Policy" dialog, which there's a task for somewhere I think.
I think I've fixed it enough that the beahavior is now correct (i.e., icons show up properly), but some of the code is a bit iffy. I'll eventually clean this up properly, but it's fairly well contained for now.
Test Plan:
- Reassigned a task.
- Put a task up for grabs.
- No reassign on closed tasks.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T9132
Differential Revision: https://secure.phabricator.com/D14669
Summary: Ref T9132. Supports selects in stacked actions and adds "Change Status" + "Change Priority".
Test Plan: Changed status and priority from stacked actions.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T9132
Differential Revision: https://secure.phabricator.com/D14667
Summary:
Ref T9132. Only allow a task to have a single owner in the UI.
In Conduit, make this field appear and behave as "phid" instead of "list<phid>".
Test Plan: Edited a task with new fancy form, got limited to one owner. Assigned/unassigned. Used Conduit to assign/unassign.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T9132
Differential Revision: https://secure.phabricator.com/D14666
Summary: Ref T9132. This makes the "Quote" action on comments work properly in these applications.
Test Plan: Quoted text in each application.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T9132
Differential Revision: https://secure.phabricator.com/D14665
Summary: Ref T9132. Shhh this never happened shhhhhhh.
Test Plan: Selected multiple actions, saw them add at the bottom.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T9132
Differential Revision: https://secure.phabricator.com/D14664
Summary:
Ref T9132. Like D14659, I'll hold this until after the cut.
This swaps commenting in Maniphest over to EditEngine / stackable actions. New code doesn't have parity yet, although none of the things we're missing should technically be //strictly mandatory//. There's a list inline. I'll restore these in the next diffs.
Briefly -- comments, subscribers and projects work. Status, owners and priority do not yet.
Test Plan:
- Made comments and added subscribers and projects.
- Read through the old code to look for missing features and tried to document them all.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T9132
Differential Revision: https://secure.phabricator.com/D14663
Summary:
Ref T9132. I'm going hold this until after the release cut since it isn't going to land completely smoothly, but I think I can prep it today/tomorrow and hopefully get it close enough to working to put in HEAD on Saturday after the push.
This adds the basics: new EditEngine, new EditController, and new `maniphest.edit` API endpoint.
I put the new stuff at `editpro/` for now until it works a little better.
Some notes on stuff this is dropping/changing/not-working-yet:
- Preview for the description. I'd rather solve this by putting a "Preview" button on every Remarkup area if we want to retain it. Particularly, it does not generalize to adding custom remarkup fields in its current form. See also T3967.
- Per-field policies are no longer enforced. They were never truly enforced anyway (for example, any user who can edit a task has always been able to edit every field via Conduit or email actions or Herald, where Herald supports things), and only really served as a hint to users. I think we can obsolete this by having installs hide/lock these fields instead. This is a desirable outcome for me, since I don't like retaining these policies and the idea of truly enforcing them properly is worrisome. These were originally added for Uber as an onboarding sort of thing. I'll prepare users for this in greater detail in the documentation.
- Couple of minor bugs with ordering / defaults / only-one-owner-allowed in this diff that I'll clean up in future diffs before this stuff lands.
- I don't have a concrete plan on "Create Similar Task" / "Clone" yet (do you have thoughts? Is this worth trying to do in every application?). I'll probably just mostly mimic the current behavior.
Test Plan: I'll vet this more thoroughly in followups, just banged around some tasks for now and created/edited via the API.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T9132
Differential Revision: https://secure.phabricator.com/D14659
Summary: Provides a real 2x avatar and offers new built in images for profile pictures.
Test Plan: reload profile, see sharper image, pick eevee, see eevee
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D14668
Summary: Use Profile Image, remove skin, show domain info better, add New Post button. Ref T9897
Test Plan: Test new buttons, see new images.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Maniphest Tasks: T9897
Differential Revision: https://secure.phabricator.com/D14671
Summary: Make this function re-usable in other views. Ref T9897
Test Plan: View a blog, see the same information
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Maniphest Tasks: T9897
Differential Revision: https://secure.phabricator.com/D14658
Summary: Color nodata as nodata, fix picture redirect, give hints when items aren't set on blogs. Ref T9897
Test Plan: Tested each of these items.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Maniphest Tasks: T9897
Differential Revision: https://secure.phabricator.com/D14657
Summary: I spent way to long to arrive at this solution. Ref T9360
Test Plan: Publish a new post, see time, unpublish post, see draft. Start a new post, wait 10 minutes, publish, see "now".
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Maniphest Tasks: T9360
Differential Revision: https://secure.phabricator.com/D14656
Summary: Ref T9132. This still has a lot of rough edges but the basics seem to work OK.
Test Plan: {F1012627}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T9132
Differential Revision: https://secure.phabricator.com/D14653
Summary:
Ref T9132. Fixes T5031. This approximately implements the plan described in T5031#67988:
When we recieve a preview request, don't write a draft if the form is from a version of the object before the last update the viewer made.
This should fix the race-related (?) zombie draft comments that sometimes show up.
I just added a new object for this stuff to make it easier to do stacked actions (or whatever we end up with) a little later, since I needed to do some schema adjustments anyway.
Test Plan:
- Typed some text.
- Reloaded page.
- Draft stayed there.
- Tried real hard to get it to ghost by submitting stuff in multiple windows and typing a lot and couldn't, although I didn't bother specifically narrowing down the race condition.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T5031, T9132
Differential Revision: https://secure.phabricator.com/D14640
Summary:
Ref T9132. When I've touched `PhabricatorApplication` I keep hitting this bad `pht()` junk.
The warning is correct, these strings are not extactable and can not be translated.
Fix it so they can be extracted and translated.
Broadly, in all cases we want to render one of these:
> 95 Things (for fewer than some limit)
> 99+ Things (when we hit the limit)
Test Plan: Looked at homepage status counts, moused over them, saw reasonable strings. Grepped for removed method.
Reviewers: chad
Reviewed By: chad
Subscribers: joshuaspence
Maniphest Tasks: T9132
Differential Revision: https://secure.phabricator.com/D14638
Summary:
Ref T9132. This just replaces the "Add Comment" form in Paste with a generic flow in EditEngine.
No actual field-awareness or action stacking or anything quite yet, but that will come in a bit. This mildly regresses drafts (which don't seem like a big deal for Pastes). I'll hook those up again in the next diff, but I want to build them in a better way that will work with multiple actions in a generic way, and solve T5031.
Big practical advantage here is that applications don't need copy/pasted preview controllers.
Test Plan:
- Saw previews.
- Added comments.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T9132
Differential Revision: https://secure.phabricator.com/D14637
Summary: Seeing if this is the correct path, then will apply in Pholio, Ponder.
Test Plan: epriestley
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: joshuaspence, Korvin
Maniphest Tasks: T9825
Differential Revision: https://secure.phabricator.com/D14646
Summary: This adds a separate Publish/Unpublish step aside from Preview in Phame Posts. This allows easier access to publishing without previewing, though I left publish in tact on the preview page. Also cleaned up some minor transaction issues with mail.
Test Plan: New Post, Publish Post, Preview Post. Check mail logs. Get mail upon publish.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D14642
Summary:
Ref T9132. Give most standard custom fields reasonable Conduit support so you can use the new `application.x` endpoints to set them.
Major missing field type is dates, again.
Test Plan: Used Conduit to set various custom fields on a package.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T9132
Differential Revision: https://secure.phabricator.com/D14635
Summary:
Ref T9132. This allows you to prefill custom fields with `?custom.x.y=value`, for most types of custom fields.
Dates (which are substantially more complicated) aren't supported. I'll just do those once the dust settles. Other types should work, I think.
Test Plan:
- Verified custom fields appear on "HTTP Parameters" help UI.
- Used `?x=y` to prefill custom fields on edit form.
- Performed various normal edits.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T9132
Differential Revision: https://secure.phabricator.com/D14634
Summary:
Fixes T9874.
- Stop using the phrase "restart your webserver". Instead, say "restart Phabricator".
- Write a document explaining that "Restart Phabricator" means to restart all of the server processes, depending on how your configuration is set up, and approximately how to do that.
- Link to this document.
- In places where we are not specifically giving instructions and the user isn't expected to do anything, be intentionally vague so as to avoid being misleading.
Test Plan:
- Read document.
- Hit "exetnsion" and "PHP config" setup checks, got "restart Phabricator" with documentation links in both cases.
Reviewers: chad
Maniphest Tasks: T9874
Differential Revision: https://secure.phabricator.com/D14636
Summary:
Ref T9132. This isn't perfect, but doesn't break any existing functionality. This stuff works:
- Editing values.
- Reordering fields.
- All builtin field tyepes.
This stuff may not work yet:
- Assigning custom field defaults.
- Some conduit stuff.
- Fully custom fields?
- Locking/hiding fields? Didn't actually test this one.
I'll keep chipping away at that stuff. In some cases, it may be easier to convert all the CustomField apps first, although Differential might be a fair bit of work.
Test Plan:
Created a bunch of custom fields of every avialable type and edited them.
{F1008789}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T9132
Differential Revision: https://secure.phabricator.com/D14617
Summary:
Ref T9132. Paste is in fairly good shape so Owners is up next. Reasoning:
- One install wants API access for it.
- It's a simple application for getting CustomFields working with EditEngine.
This only does the EditEngine part, so CustomFields are no longer editable until I make that work. That will be up next, and I'll hold this until that's ready.
Test Plan:
- Created and edited packages via web UI.
- Created and edited package editing forms via web UI.
- Created and edited packages via Conduit.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T9132
Differential Revision: https://secure.phabricator.com/D14598
Summary:
Fixes T9881. This one had Quicksand spelled as "QuickSand" (with capital "S") so it probably didn't get hit by `grep`.
Didn't need to do any special magic with the footer, as far as I can tell.
Test Plan: Loaded project board view, seemed to work OK (no footer, nav works, title works, mobile menu sane).
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T9881
Differential Revision: https://secure.phabricator.com/D14626
Summary: Fixes T9715. Adds a MySQL-based lock to ensure that schema migrations are not applied on multiple hosts simultaneously.
Test Plan: Ran `./bin/storage upgrade` concurrently. One invocation was successful whilst the other hit a `PhutilLockException`.
Reviewers: #blessed_reviewers, epriestley
Subscribers: Korvin
Maniphest Tasks: T9715
Differential Revision: https://secure.phabricator.com/D14463
Summary: Removes all calls to addExtraQuicksandConfig Ref T9690
Test Plan: grep for addExtraQuicksandConfig, view a Pholio Page with and without chatbar, edit a pholio mock, save mock.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Maniphest Tasks: T9690
Differential Revision: https://secure.phabricator.com/D14622
Summary: These weren't open to the public even if the blog was public.
Test Plan: View a blog post, blog view, and blog manage page while logged out.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D14619
Summary: Sends `/phame/` to PhameHomeController, which is all published posts. Still some rough edges to work out for new posts, new blogs, but I think this is the right direction.
Test Plan:
go to Phame, see most recent posts, no drafts. click on find posts, see post list, click on find blogs, see blogs.
{F1008800}
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Maniphest Tasks: T9742
Differential Revision: https://secure.phabricator.com/D14618
Summary: Provides more information that a post is a draft.
Test Plan:
Add a draft post, see new style. Check Blog as non-editor, don't see draft post.
{F1008655}
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Maniphest Tasks: T9360
Differential Revision: https://secure.phabricator.com/D14613
Summary:
Ref T9132. This is a bit more cleanup to make adding CustomField support easier.
Right now, both `EditField` and `EditType` can actually generate a transaction. This doesn't matter too much in practice today, but gets a little more complicated a couple of diffs from now with CustomField stuff.
Instead, always use `EditType` to generate the transaction. In the future, this should give us less total code and make more things work cleanly by default.
Test Plan: Used web UI and Conduit to make various edits to pastes, including doing race-condition tests on "Projects".
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T9132
Differential Revision: https://secure.phabricator.com/D14607
Summary: Ref T9132. I had some hacks in place for dealing with Edge/Subscribers stuff. Clean that up so it's structured a little better.
Test Plan:
- Edited subscribers and projects.
- Verified things still show up in Conduit.
- Made concurrent edits (added a project in one window, removed it in another window, got a clean result with a correct merge of the two edits).
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T9132
Differential Revision: https://secure.phabricator.com/D14601
Summary:
Ref T9132. This is a quality-of-life improvement for new `application.edit` endpoints.
Instead of strictly requiring PHIDs, allow IDs or monograms. This primarily makes these endpoints easier to test and use.
Test Plan: Edited objects via API by passing IDs, PHIDs and monograms.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T9132
Differential Revision: https://secure.phabricator.com/D14600
Summary:
Ref T9132. Currently, EditEngine had some branchy-`instanceof` code like this:
```
if ($object instanceof Whatever) {
do_magic();
}
if ($object instanceof SomethingElse) {
do_other_magic();
}
```
...where `Whatever` and `SomethingElse` are first-party applications like ProjectsInterface and SubscribersInterface.
This kind of code is generally bad because third-parties can't add new stuff, and it suggest something is kind of hacky in its architecture. Ideally, we would eventually get rid of almost all of this.
T9789 is a similar discussion of this for the next layer down (`TransactionEditor`) and plans to get rid of branchy-instanceofs there too.
Since I'm about to add more stuff here (for Custom Fields), split it out first so I'm not digging us any deeper than I already dug us.
Broadly, this allows third-party extensions to add fields to every EditEngine UI if they want, like we do for Policies, Subscribers, Projects and Comments today (and CustomFields soon).
Test Plan:
{F1007575}
- Observed that all fields still appear on the form and seem to work correctly.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T9132
Differential Revision: https://secure.phabricator.com/D14599
Summary: Creates a new PhameBlogView which is more of a blog landing page with the latest posts. Management has moved to PhameManageController with a new timeline.
Test Plan:
Edit Blog, Publish, Subscribe, view posts.
{F1008400}
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Maniphest Tasks: T9360
Differential Revision: https://secure.phabricator.com/D14608
Summary: Updates Herald to use modern methods.
Test Plan: View List, View Test Console, Run a test, View Results, View Rules, New Rule, Edit Rule, Check mobile menus.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Maniphest Tasks: T9690
Differential Revision: https://secure.phabricator.com/D14596
Summary: Use modern methods in Pholio
Test Plan: View list, create mock, edit mock, view mobile menu
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Maniphest Tasks: T9690
Differential Revision: https://secure.phabricator.com/D14595
Summary: Fixes T9869. This specific transaction endpoint was missing `shouldAllowPublic()`. Also modernize things a little.
Test Plan: Viewed a policy change by clicking the policy name from the transaction record on a public object while logged out.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T9869
Differential Revision: https://secure.phabricator.com/D14606
Test Plan: warning is not in warnings list.
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: epriestley
Differential Revision: https://secure.phabricator.com/D14594
Summary:
Ref T9132.
Let configurations be enabled/disabled. This doesn't do much right now.
Let configurations be marked as default entries in the application "Create" menu. This makes them show up in the application in a dropdown, so you can replace the default form and/or provide several forms.
In Maniphest, we'll do this to provide a menu something like this:
- New Bug Report
- New Feature Request
- ADVANCED TASK CREATION!!11~ (only available for Community members)
Test Plan: {F1005679}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T9132
Differential Revision: https://secure.phabricator.com/D14584
Summary:
Ref T9845. In Differential, this is not a remarkup block -- it's a mail section. `addTextSection()` has special magic behavior when handed a prebuilt section since D9375.
Swapping to `addRemarkupSection()` causes the error in T9845 and renders nothing in the comment section.
Even if it were a block of text, it would not be appropriate to add it as remarkup. This would incorrectly render comments in files like `__init__.py`, which are common on Python (the filename would render as "__init__.py"). Okay that's a bad example since it works fine but, uh, a file named `T123` would be no good or whatever.
I'll realign T9845 to clean this up and fix it more durably.
Test Plan: Sent myself some mail with inline comments, saw them in the mail.
Reviewers: joshuaspence, chad
Reviewed By: chad
Maniphest Tasks: T9845
Differential Revision: https://secure.phabricator.com/D14589
Summary: Will use these more in the upcoming unbeta design of PhameBlog, likely. Also curious how this works.
Test Plan: Add an image to a blog, remove an image from a blog.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D14587
Summary: Allows Blogs and Posts to be destroyed. Fixes T9756
Test Plan: Test `bin/remove destroy POST` and `bin/remove destroy BLOG` to great success.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Maniphest Tasks: T9756
Differential Revision: https://secure.phabricator.com/D14586
Summary: This makes document views a little more automatic, and a little more style to the page. The Document itself remains on a pure white centered background, but footer and preceeding objects go back to the original body color. This provides a bit more depth and separation over content and definitions/comments.
Test Plan:
Tested Phriction, Diviner, Legalpad, Phame, Email Commands, HTTP Commands, with and without a footer.
{F1005853}
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D14582
Summary: Ref T9756, removes the ability to delete a PhamePost
Test Plan: See link removed, unpublish post, publish post, new post.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Maniphest Tasks: T9756
Differential Revision: https://secure.phabricator.com/D14581
Summary: Caught these while re-reading.
Test Plan: Reading?
Reviewers: chad
Reviewed By: chad
Differential Revision: https://secure.phabricator.com/D14580
Summary: Provide a long-form description of why we require a CLA and the distinction between the individual and corporate CLAs. See Q219 and Q97.
Test Plan: Reading.
Reviewers: chad
Reviewed By: chad
Differential Revision: https://secure.phabricator.com/D14578
Summary: Ref T9851. See T9860. This adds a missing capability to custom HeraldActions, to pave the way for removing the obsolete/undesirable WILLEDITTASK and DIDEDITTASK events.
Test Plan: See T9860 for a replacement action.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T9851
Differential Revision: https://secure.phabricator.com/D14575
Summary: Column status cannot be null fix.
Test Plan: Create a new blog.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D14574
Summary:
Fixes T9850. The `getComment()` test should be a `hasComment()` test, in order to discard empty comments.
Also backport a couple of future fixes which can get you into trouble if you reconfigure forms in awkward ways.
Test Plan: Created a new paste without a comment.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T9850
Differential Revision: https://secure.phabricator.com/D14571
Summary: Also increase the timeout for the external process to complete the transform.
Test Plan: Careful inspection
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: joshuaspence, cburroughs, chad, Korvin
Differential Revision: https://secure.phabricator.com/D14528
Summary: Moves to showing Legalpad previews using PHUIDocumentViewPro
Test Plan: Create a new document, edit an existing document
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D14550
Summary: New icons
Test Plan: Use new icons
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D14568
Summary: Ref T992. I noticed that `ManiphestTask` mail doesn't render Remarkup properly (instead, it renders Remarkup literally). I //think// this is because the code calls `addTextSection()` rather than `addRemarkupSection()`.
Test Plan: Created a new Maniphest Task and saw Remarkup in the generated self-email (inspect the email contents with `./bin/mail show-outbound`). I didn't test the other affected applications.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: Korvin
Maniphest Tasks: T992
Differential Revision: https://secure.phabricator.com/D14511
Summary: Reuse PHUIMarkupPreviewView in Phame for consistency, less custom code. Also, doesn't work (JS issue).
Test Plan: New Post, Edit Post, Save Post
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D14552
Summary:
Fixes T9832. We currently refuse to recognize project hashtags in remarkup if they begin with a digit. This is motivated by attempting to not recognize them if they contain only digits.
I don't think we really gain anything by this. Although most `#123` in text are probably not project references, the cost of doing a lookup for them is quite small, and //some// of them are.
In cases where users use `#123` to refer to tasks in an external system, they can use a rule for that with higher precedence than this one or not give their projects conflicting hashtags.
Test Plan:
- This is well-covered by unit tests.
- Referenced a `#3u1`, per T9832.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T9832
Differential Revision: https://secure.phabricator.com/D14547
Summary: These constants are incorrect.
Test Plan: Archive a blog, see feed story. Publish a blog, see another feed story.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D14545
Summary: See D14536. Some time ago, linters changed to no longer receive these special/unusual file types as inputs by default.
Test Plan:
- Read new docs.
- Attempted to grep for other similar lies, although it's possible I missed some. I didn't find anything.
Reviewers: bgamari, chad
Reviewed By: chad
Differential Revision: https://secure.phabricator.com/D14543
Summary: This logic is flipped.
Test Plan:
- Before change: ran `bin/phd debug task`, saw queries to the config table every second.
- After change: ran `bin/phd debug task`, saw queries to the config table every 10 seconds.
Reviewers: chad, joshuaspence
Reviewed By: chad, joshuaspence
Differential Revision: https://secure.phabricator.com/D14542
Summary: Removes the exception, maybe there is a better way, but landing this for now. Fixes T9829
Test Plan: Test pages with and without a table of contents
Reviewers: epriestley, avivey
Subscribers: epriestley
Maniphest Tasks: T9829
Differential Revision: https://secure.phabricator.com/D14546
Summary:
Ref T9132. This adds an automatic "Comments" field, like the Subscribers/Projects/Policy fields.
The primary goals here are:
- Allow users to make comments via Conduit.
- In the future, get stackable action support.
As a side effect, this also allows you to put comments on create forms. This is a little silly but seems fine, and may be relevant on edit forms (which I'm not 100% sure how I want to handle yet). I've just hidden them by default for now.
Test Plan:
{F976036}
{F976037}
{F976038}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T9132
Differential Revision: https://secure.phabricator.com/D14515
