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: 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:
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 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 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 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 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. 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. 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 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. 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:
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. 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. 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:
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:
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
Summary:
Ref T9132. Allows fields to be locked (shown, but not modifiable) and hidden (not shown).
In both cases, default values are still respected.
This lets you do things like create a form that generates objects with specific projects, policies, etc.
Test Plan:
- Set defaults.
- Locked and hid a bunch of fields.
- Created new objects using the resulting form.
{F975801}
{F975802}
{F975803}
{F975804}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T9132
Differential Revision: https://secure.phabricator.com/D14509
Summary: Ref T9132. Allow form configurations to include defaults (like default projects, spaces, policies, etc).
Test Plan:
Defaulted "Language" to "Rainbow", plus other adjustments:
{F975746}
{F975747}
{F975748}
{F975749}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T9132
Differential Revision: https://secure.phabricator.com/D14508
Summary:
Ref T9132. This just makes edited forms do //something//, albeit not anything very useful yet.
You can now edit a form and:
- Retitle it;
- add a preamble (instructions on top of the form); and
- reorder the form's fields.
Test Plan:
{F974632}
{F974633}
{F974634}
{F974635}
{F974636}
Reviewers: chad
Reviewed By: chad
Subscribers: hach-que
Maniphest Tasks: T9132
Differential Revision: https://secure.phabricator.com/D14503
Summary:
Ref T9132. This diff doesn't do anything interesting, it just lays the groundwork for more interesting future diffs.
Broadly, the idea here is to let you create multiple views of each edit form. For example, we might create several different "Create Task" forms, like:
- "New Bug Report"
- "New Feature Request"
These would be views of the "Create Task" form, but with various adjustments:
- A form might have additional instructions ("how to file a good bug report").
- A form might have prefilled values for some fields (like particular projects, subscribers, or policies).
- A form might have some fields locked (so they can not be edited) or hidden.
- A form might have a different field order.
- A form might have a limited visibility policy, so only some users can access it.
This diff adds a new storage object (`EditEngineConfiguration`) to keep track of all those customizations and represent "a form which has been configured to look and work a certain way".
This doesn't let these configurations do anything useful/interesting, and you can't access them directly yet, it's just all the boring plumbing to enable more interesting behavior in the future.
Test Plan:
ApplicationEditor forms now let you manage available forms and edit the current form:
{F959025}
There's a new (bare bones) list of all available engines:
{F959030}
And if you jump into an engine, you can see all the forms for it:
{F959038}
The actual form configurations have standard detail/edit pages. The edit pages are themselves driven by ApplicationEditor, of course, so you can edit the form for editing forms.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T9132
Differential Revision: https://secure.phabricator.com/D14453
Summary: Ref T9132. This allows you to prefill EditEngine forms with stuff like `?subscribers=epriestley`, and we'll figure out what you mean.
Test Plan:
- Did `/?subscribers=...` with various values (good, bad, mis-capitalized).
- Did `/?projects=...` with various values (good, bad, mis-capitalized).
- Reviewed documentation.
- Reviewed {nav Config > HTTP Parameter Types}.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T9132
Differential Revision: https://secure.phabricator.com/D14404
Summary:
Ref T9132. We have several places in the code that sometimes need to parse complex types. For example, we accept all of these in ApplicationSearch and now in ApplicationEditor:
> /?subscribers=cat,dog
> /?subscribers=PHID-USER-1111
> /?subscribers[]=cat&subscribers[]=PHID-USER-2222
..etc. The logic to parse this stuff isn't too complex, but it isn't trivial either.
Right now it lives in some odd places. Notably, `PhabricatorApplicationSearchEngine` has some weird helper methods for this stuff. Rather than give `EditEngine` the same set of weird helper methods, pull all this stuff out into "HTTPParameterTypes".
Future diffs will add "Projects" and "Users" types where all the custom parsing/lookup logic can live. Then eventually the Search stuff can reuse these.
Generally, this just breaks the code up into smaller pieces that have more specific responsibilities.
Test Plan: {F944142}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T9132
Differential Revision: https://secure.phabricator.com/D14402
Summary:
Ref T5873. Ref T9132. This is really rough and feels pretty flimsy at the edges (missing validation, generality, modularity, clean error handling, etc) but gets us most of the way toward generating plausible "whatever.edit" Conduit API methods from EditEngines.
These methods are full-power methods which can do everything the edit form can, automatically support the same range of operations, and update when new fields are added.
Test Plan:
- Used new `paste.edit` to create a new Paste.
- Used new `paste.edit` to update an existing paste.
- Applied a variety of different transactions.
- Hit a reasonable set of errors.
{F941144}
{F941145}
{F941146}
{F941147}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T5873, T9132
Differential Revision: https://secure.phabricator.com/D14393
Summary:
Ref T9132. Although forms do generally support prefilling right now, you have to guess how to do it.
Provide an explicit action showing you which values are supported and how to prefill them. This is generated automatically when an application switches to ApplicationEditor.
Test Plan:
{F939804}
{F939805}
{F939806}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T9132
Differential Revision: https://secure.phabricator.com/D14392
Summary:
Ref T9132. Ref T4768. This is a rough v0 of ApplicationEditor, which replaces the edit workflow in Paste.
This mostly looks and works like ApplicationSearch, and is heavily modeled on it.
Roughly, we define a set of editable fields and the ApplicationEditor stuff builds everything else.
This has no functional changes, except:
- I removed "Fork Paste" since I don't think it's particularly useful now that pastes are editable. We could restore it if users miss it.
- Subscribers are now editable.
- Form field order is a little goofy (this will be fixed in a future diff).
- Subscribers and projects are now race-resistant.
The race-resistance works like this: instead of submitting just the new value ("subscribers=apple, dog") and doing a set operation ("set subscribers = apple, dog"), we submit the old and new values ("original=apple" + "new=apple, dog") then apply the user's changes as an add + remove ("add=dog", "remove=<none>"). This means that two users who do "Edit Paste" at around the same time and each add or remove a couple of subscribers won't overwrite each other, unless they actually add or remove the exact same subscribers (in which case their edits legitimately conflict). Previously, the last user to save would win, and whatever was in their field would overwrite the prior state, potentially losing the first user's edits.
Test Plan:
- Created pastes.
- Created pastes via API.
- Edited pastes.
- Edited every field.
- Opened a paste in two windows and did project/subscriber edits in each, saved in arbitrary order, had edits respected.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T4768, T9132
Differential Revision: https://secure.phabricator.com/D14390
