Summary: Ref T11114. Obsoleted by Modular Transactions + EditEngine + CommitMessageField + we just "hard code" the title of revisions into the page because we're craaazy.
Test Plan:
- Made an edit on `stable`.
- Viewed the edit on this change, it still had the proper UI strings.
- Edited/created/updated revisions.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T11114
Differential Revision: https://secure.phabricator.com/D17083
Summary: Ref T11114. This makes the unusual stored custom fields ("Blame Rev", "Revert Plan", etc) work somewhat correctly (?) with EditEngine.
Test Plan:
- Created, updated and edited revisions with unusual stored custom fields like "Blame Rev".
- Observed that these fields now populate in "differential.revision.edit" when available.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T11114
Differential Revision: https://secure.phabricator.com/D17068
Summary:
Ref T11114. This creates `differential.revision.edit` (a modern, v3 API method) and redefines the existing methods in terms of it.
Both `differential.createrevision` and `differential.updaterevision` are now internally implemented by building a `differential.revision.edit` API call and then executing it.
I //think// this covers everything except custom fields, which need some tweaking to work with EditEngine. I'll clean that up in the next change.
Test Plan:
- Created, updated, and edited revisions via `arc`.
- Called APIs manually via test console.
- Stored custom fields ("Blame Rev", "Revert Plan") aren't exposed yet.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T11114
Differential Revision: https://secure.phabricator.com/D17067
Summary: Allows you to set forms via typeahead
Test Plan: `/typeahead/browse/PhabricatorEditEngineDatasource/`
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D17065
Summary: This is just some housekeeping - see note in D16287. Basically, "isTextMode" doesn't convey enough information.
Test Plan: `git grep isTextMode | grep -v Remarkup`, and visit all callsites; There are 4 of them left.
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D17063
Summary: Ref T11114. Much of this is around making the "comment-while-updating" flow work correctly.
Test Plan:
- Created new diffs by copy/pasting, then:
- used one to create a new revision;
- used one to update an existing revision, with a comment.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T11114
Differential Revision: https://secure.phabricator.com/D17053
Summary: We're throwing here when we actually want to return `null` so we make it into custom field handling code. See Conpherence.
Test Plan: Found a failing task and re-executed it with `bin/worker execute --id <id>`; after this change, it didn't fatal.
Reviewers: chad
Reviewed By: chad
Differential Revision: https://secure.phabricator.com/D17051
Summary: Ref T11114. This one is a bit more complex, but I think I covered everything.
Test Plan:
- Added reviewers.
- Removed reviewers.
- Made reviewers blocking.
- Made reviewers nonblocking.
- Tried to make the author a reviewer.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T11114
Differential Revision: https://secure.phabricator.com/D17050
Summary:
Ref T11114. Currently, all of Differential is extremely custom CustomFields. I want to back away from that somewhat and leverage more EditEngine / ModularTransactions infrastructure.
This allows EditEngine, ModularTransactions, and CustomFields to coexist in an uneasy peace. The "EditPro" controller applies a //different edit// than the CustomFields do, but everything works out in the end. I think.
Hopefully the horrible mess I am creating here will be short-lived.
Test Plan:
- Edited a revision with the normal editor.
- Edited a revision with the pro editor.
- Created a revision with `arc diff`.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T11114
Differential Revision: https://secure.phabricator.com/D17044
Summary:
Ref T11954. This is kind of complex and I'm not sure I want to actually land it, but it gives us a fairly good improvement for clustered repositories so I'm leaning toward moving forward.
When we make (or receive) clustered repository requests, we must first load a bunch of stuff out of Almanac to figure out where to send the request (or if we can handle the request ourselves).
This involves several round trip queries into Almanac (service, device, interfaces, bindings, properties) and generally is fairly slow/expensive. The actual data we get out of it is just a list of URIs.
Caching this would be very easy, except that invalidating the cache is difficult, since editing any binding, property, interface, or device may invalidate the cache for indirectly connected services and repositories.
To address this, introduce `PhabricatorCacheEngine`, which is an extensible engine like `PhabricatorDestructionEngine` for propagating cache updates. It has two modes:
- Discover linked objects (that is: find related objects which may need to have caches invalidated).
- Invalidate caches (that is: nuke any caches which need to be nuked).
Both modes are extensible, so third-party code can build repository-dependent caches or whatever. This may be overkill but even if Almanac is the only thing we use it for it feels like a fairly clean solution to the problem.
With `CacheEngine`, make any edit to Almanac stuff propagate up to the Service, and then from the Service to any linked Repositories.
Once we hit repositories, invalidate their caches when Almanac changes.
Test Plan:
- Observed a 20-30ms performance improvement with `ab -n 100`.
- (The main page making Conduit calls also gets a performance improvement, although that's a little trickier to measure directly.)
- Added debugging code to the cache engine stuff to observe the linking and invalidation phases.
- Made invalidation throw; verified that editing properties, bindings, etc, properly invalidates the cache of any indirectly linked repositories.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T11954
Differential Revision: https://secure.phabricator.com/D17000
Summary:
Ref T11816. Currently, if someone in California creates an event and then someone in New York edits it, we generate a no-op "<user> changed the start time from 3PM to 3PM." transaction.
This is because the internal timezone of the event is changing, but the actual absolute time is not.
Instead, when an edit wouldn't reschedule an event and would only change the internal timezone, ignore the edit.
Test Plan:
- Edited non-all-day events in PST / EST with out making changes (ignored).
- Edited non-all-day events in PST / EST with changes (changes worked).
- Performed the same edits with all-day events, which also were ignored and worked, respectively.
- Pulled events in and out of all-day mode in different timezones, behavior seemeed reasonable.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T11816
Differential Revision: https://secure.phabricator.com/D16955
Summary: Basic work in progress, but should show timeline comments for files when in lightbox mode. Looks reasonable.
Test Plan: click on images, see comments from timeline.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Maniphest Tasks: T3612
Differential Revision: https://secure.phabricator.com/D16896
Summary:
See downstream issue here: <https://phabricator.wikimedia.org/T150992>
In at least one case (project milestones) we have a locked, non-lockable field. This means "this is locked, and you can't change the fact that it is locked".
At least for now, preserve this behavior.
Test Plan: Created a new milestone of an existing project. This worked correctly with the patch.
Reviewers: chad
Reviewed By: chad
Differential Revision: https://secure.phabricator.com/D16895
Summary: This code should go inside the field-locking loop. Otherwise, it only applies to the last field, and fatals if there are no fields.
Test Plan: Carefuller inspection.
Reviewers: chad
Reviewed By: chad
Subscribers: 20after4
Differential Revision: https://secure.phabricator.com/D16889
Summary:
This has been replaced by `PolicyCodex` after D16830. Also:
- Rebuild Celerity map to fix grumpy unit test.
- Fix one issue on the policy exception workflow to accommodate the new code.
Test Plan:
- `arc unit --everything`
- Viewed policy explanations.
- Viewed policy errors.
Reviewers: chad
Reviewed By: chad
Subscribers: hach-que, PHID-OPKG-gm6ozazyms6q6i22gyam
Differential Revision: https://secure.phabricator.com/D16831
Summary: Redesign the action comment box for better use in two column, mobile, nuance.
Test Plan: Test in mobile / desktop / tablet, adding and removing actions. Actionless comment boxes, etc.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: cspeckmim, Korvin
Differential Revision: https://secure.phabricator.com/D16811
Summary:
Ref T7643. When you do something like this:
- Edit a task description.
- Click "Show Details" on the resulting transaction.
- Get a prose diff dialog showing the change.
...now add some "Old" and "New" tabs. These are useful for:
- reverting to the old text by copy/pasting;
- reading just the new/old text if the diff is noisy;
- sometimes just nice to have?
(This looks a little rough but I didn't want to put a negative margin on tab groups inside dialogs? Not sure what the best fix here is.)
Test Plan: {F1909390}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T7643
Differential Revision: https://secure.phabricator.com/D16817
Summary: Ref T7643. When we send mail about a change to a package description, allow it to say "CHANGES TO PACKAGE DESCRIPTION" instead of "EDIT DETAILS". Smooth!
Test Plan: {F1909417}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T7643
Differential Revision: https://secure.phabricator.com/D16818
Summary:
Fixes T11812.
- Pull the logic for building the "Create Whatever" dropdown out.
- Use it to generate NUX buttons, too.
- Use the new logic in Paste and Maniphest.
Test Plan:
- Viewed Paste NUX, button worked.
- Viewed Maniphest NUX with multiple create forms, button worked.
Reviewers: chad, avivey
Reviewed By: avivey
Subscribers: avivey
Maniphest Tasks: T11812
Differential Revision: https://secure.phabricator.com/D16797
Summary:
Ref T11809.
- Allow users to remove the "Until" date from recurring events.
- When removing "Until", show a sensible string ("...set this event to repeat forever.")
- When users go through the "Make Recurring" workflow, don't require them to explicitly select "Recurring: Recurring" from the dropdown. This intent is clear from clicking "Make Recurring".
- When editing "All Future Events", don't literally apply date changes to them, since that doesn't make sense. We update the template, then reschedule any events which haven't been edited already. I think this is what users probably mean if they make this edit.
- When creating an event with a non-default icon, don't show "alice changed the icon from Default to Party.".
- Hide the "recurring mode" transaction, which had no string ("alice edited this Event.") and was redundant anyway.
- Also, add a little piece of developer text to make hunting these things down easier.
Test Plan: Edited various events, parents, children, made events recur, set until, unset until, viewed transactions, rescheduled parents, rescheduled children.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T11809
Differential Revision: https://secure.phabricator.com/D16796
Summary:
Ref T11809. Currently, commenting on a recurring event hits the same "one or all?" dialog that other edits do.
For comments and edits submitted via the comment widget, we can safely assume that you mean "just this one", since it doesn't really make sense to try to bulk-edit an event from that UI.
Test Plan: Commented on a recurring event parent and an event in the series.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T11809
Differential Revision: https://secure.phabricator.com/D16795
Summary:
Fixes T11805. Depends on D16785. This generally tries to smooth out transactions:
- All-day stuff now says "Nov 3" instead of "Nov 3 12:00:00 AM".
- Fewer weird bugs / extra transactions.
- No more silly extra "yeah, you definitely set that event time" transaction on create.
Test Plan: Edited events; changed from all-day to not-all-day and back again, viewed transaction log.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T11805
Differential Revision: https://secure.phabricator.com/D16786
Summary:
Fixes T11804. This probably isn't perfect but seems to work fairly reasonably and not be as much of a weird nonsense mess like the old behavior was.
When a user edits a recurring event, we ask them what they're trying to do. Then we more or less do that.
Test Plan:
- Edited an event in the middle of a series.
- Edited the first event in a series.
- Edited "just this" and "all future" events in various places in a series.
- Edited normal events.
- Cancelled various events.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T11804
Differential Revision: https://secure.phabricator.com/D16782
Summary:
This feels a little cleaner:
- Clean up transaction log a bit.
- Use a checkbox instead of a two-option dropdown.
This is a little messy because the browser doesn't send anything if the user submits a form with an un-clicked checkbox.
We now send a dummy value ("Hey, there's definitely a checkbox in this form!") so the server can figure out what to do.
Test Plan:
- Edited all-dayness of an event.
- Viewed transaction log.
Reviewers: chad
Reviewed By: chad
Differential Revision: https://secure.phabricator.com/D16776
Summary:
This fixes the permissions issue with D16750, which is actually not really a permissions issue, exactly.
This is the only place anywhere that we use a tokenizer field //and// give it a default value which is not the same as the object value (when creating a merchant, we default it to the viewer).
In other cases (like Maniphest) we avoid this because you can edit the form to have defaults, which would collide with whatever default we provide. Some disucssion in T10222.
Since we aren't going to let you edit these forms for the forseeable future, this behavior is reasonable here though.
However, it triggered a sort-of-bug related to conflict detection for these fields (see T4768). These fields actually have two values: a hidden "initial" value, and a visible edited value.
When you submit the form, we compute your edit by comparing the edited value to the initial value, then applying adds/removes, instead of just saying "set value equal to new value". This prevents issues when two people edit at the same time and both make changes to the field.
In this case, the initial value was being set to the display value, so the field would say "Value: [(alincoln x)]" but internally have that as the intitial value, too. When you submitted, it would see "you didn't change anything", and thus not add any members.
So the viewer wouldn't actually be added as a member, then the policy check would correctly fail.
Note that there are still some policy issues here (you can remove yourself from a Merchant and lock yourself out) but they fall into the realm of stuff discussed in D16677.
Test Plan: Created a merchant account with D16750 applied.
Reviewers: chad
Reviewed By: chad
Differential Revision: https://secure.phabricator.com/D16764
Summary:
Fixes T11638.
- Fix a regression: I broke this "round to the nearest hour" code a while ago while fiddling with datetimes.
- Improve a beahvior: from the day view, make the menu-bar "Create Event" button default to creating an event on the day you were viewing.
Test Plan: Created events from month and day views, got nice round numbers and proper day suggestions.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T11638
Differential Revision: https://secure.phabricator.com/D16754
Summary: Fixes T11733. This fixes the issue by working around it, but it isn't useful to set these fields to a default value anyway.
Test Plan: Created a default Calendar form, set some other defaults, created an event, stuff no longer exploded.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T11733
Differential Revision: https://secure.phabricator.com/D16753
Summary: Accept Conduit parameter values as strings (e.g. from `curl`) and convert to required type.
Test Plan:
Call conduit method with int/bool parameter iusing `curl` and make sure it does not result in validation error, e.g.
```
$ curl http://$PHABRICATOR_HOST/api/maniphest.search -d api.token=$CONDUIT_TOKEN -d constraints[modifiedEnd]=$(date +%s) -d constraints[hasParents]=true -d limit=1
```
Fixes T10456.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: Korvin
Maniphest Tasks: T10456
Differential Revision: https://secure.phabricator.com/D16694
Summary: Ref T10747. This barely works, but can technically import some event data.
Test Plan: Used import flow to import a ".ics" document.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10747
Differential Revision: https://secure.phabricator.com/D16699
Summary:
Ref T7924. This:
- Adds support for remarkup block changes to Modular Transactions.
- Exposes remarkup changes from the Calendar event "Description" transaction.
This makes stuff like mentions and file embeds work properly.
Test Plan:
Mentioned a task in an event description, saw a mention appear on the task.
Uploaded a file to an event description, saw the file become "Attached" to the event.
(Neither of these worked properly before.)
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T7924
Differential Revision: https://secure.phabricator.com/D16481
Summary: Ref T11132, significantly cleans up the Config app, new layout, icons, spacing, etc. Some minor todos around re-designing "issues", mobile support, and maybe another pass at actual Group pages.
Test Plan: Visit and test every page in the config app, set new items, resolve setup issues, etc.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam, Korvin
Maniphest Tasks: T11132
Differential Revision: https://secure.phabricator.com/D16468
Summary: Switches over to new property UI boxes, splits core and apps into separate pages. Move Versions into "All Settings". I think there is some docs I likely need to update here as well.
Test Plan: Click on each item in the sidebar, see new headers.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D16429
Summary:
Ref T11326. When an event is all-day, hide the time controls for the start/end dates. These aren't used and aren't helpful/useful.
This got a little more complicated than it used to be because EditEngine forms may have only some of these controls present.
Test Plan: Edited an all-day event; edited a normal event; swapped an event between normal and all-day.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T11326
Differential Revision: https://secure.phabricator.com/D16327
Summary:
Fixes T10909. I think this is a generally reasonable sort of capability to expose, although I've made it edit-only for now (when creating an event, you're always the host).
Also clean up some minor leftovers in the code, and a couple of little bugs with recurrence frequencies.
Test Plan: Created an event, edited the host of an event.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10909
Differential Revision: https://secure.phabricator.com/D16292
Summary:
Fixes T10633. When generating email about a transaction which adjusts a date, render the offset explicitly (like "UTC-7").
This makes it more clear in cases like this:
- mail is being sent to multiple users, and not necessarily using the viewer's settings;
- you get some mail while travelling and aren't sure which timezone setting it generated under.
Test Plan: Rendered in text mode, saw UTC offset.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10633
Differential Revision: https://secure.phabricator.com/D16287
Summary:
Ref T9275. Swaps Calendar over to modular transactions. Theoretically, this has almost no effect on anything.
Ref T10633. I didn't actually do anything here yet, but this gets us ready to put timestamps in email.
Test Plan: Created and edited a bunch of events, nothing seemed catastrophically broken.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T9275, T10633
Differential Revision: https://secure.phabricator.com/D16286
Summary: Ref T9275. This gets things roughly into shape for a cutover to EditEngine, mostly by fixing some problems with "recurrence end date" not being nullable while editing events.
Test Plan: Edited events with EditPro controller, nothing was obviously broken.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T9275
Differential Revision: https://secure.phabricator.com/D16282
Summary:
Ref T9275. This still has a number of rough edges and other minor problems (no JS on the controls, some date handling control bugs) but I'll smooth those over in future changes.
It does make all the editable transaction types available from EditEngine, technically speaking.
Test Plan: Created and edited events with the "pro" controller, which mostly worked.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T9275
Differential Revision: https://secure.phabricator.com/D16281
Summary: fix T11290.
Test Plan: Paste language type, view in web and in emails (It uses quotes in HTML emails, which I think is something else).
Reviewers: epriestley, chad, #blessed_reviewers
Reviewed By: chad, #blessed_reviewers
Subscribers: Korvin
Maniphest Tasks: T11290
Differential Revision: https://secure.phabricator.com/D16252
Summary:
This is the quickest and dirtiest fix I could come up with.
`PhabricatorApplicationTransaction::getTitleForMail()` is using `clone $this`, which doesn't actually effect `implementation`.
Ref T9789.
Test Plan: update paste comment, get plaintext mail.
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: epriestley
Maniphest Tasks: T9789
Differential Revision: https://secure.phabricator.com/D16251
Test Plan: Tested with a transactionType from an extension.
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: Korvin
Maniphest Tasks: T9789
Differential Revision: https://secure.phabricator.com/D16236
Summary:
Ref T4788. Fixes T7820. This updates the "Merge Duplicates In" interaction, and adds a "Close as Duplicate" action.
These are the last interactions that were using the old code, so it removes that code.
Merges are now recorded as real edges, so we can show them in the UI later on (originally from T9390, etc).
Also provides more general support for relationships which need EDIT permission, not-undoable relationships like merges, preventing relating an object to itself, and relationship side effects like merges.
Finally, fixes a couple of behaviors around typing an exact object name (like `T123`) to find the related object.
Test Plan:
- Merged tasks into the current task.
- Closed the current task as a duplicate of another task.
- Edited other relationships.
- Searched for tasks, commits, etc., by object monogram.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T4788, T7820
Differential Revision: https://secure.phabricator.com/D16196
Summary: Fixes T11166. Adds some class, and space to the preview widget.
Test Plan: Test Maniphest, Ponder, etc, without a footer.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Maniphest Tasks: T11166
Differential Revision: https://secure.phabricator.com/D16168
Summary:
Ref T11179. This splits "Edit Blocking Tasks" into two options now that we have more room ("Edit Parent Tasks", "Edit Subtasks").
This also renames "Blocking" tasks to "Subtasks", and "Blocked" tasks to "Parent" tasks. My goals here are:
- Make the relationship direction more clear: it's more clear which way is up with "parent" and "subtask" at a glance than with "blocking" and "blocked" or "dependent" and "dependency".
- Align language with "Create Subtask".
- To some small degree, use more flexible/general-purpose language, although I haven't seen any real confusion here.
Fixes T6815. I think I narrowed this down to two issues:
- Just throwing a bare exeception (we now return a dialog explicitly).
- Not killing open transactions when the cyclec check fails (we now kill them).
Test Plan:
- Edited parent tasks.
- Edited subtasks.
- Tried to introduce graph cycles, got a nice error dialog.
{F1697087}
{F1697088}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T6815, T11179
Differential Revision: https://secure.phabricator.com/D16166
Summary: Ref T9789. Falling back to `parent::` is better, and fixes older-style feed stories for Pastes, like "added a comment".
Test Plan: Viewed a comment feed story about a paste.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T9789
Differential Revision: https://secure.phabricator.com/D16114
Summary:
Ref T9789. `Transaction` and `Editor` classes are the last major pieces of infrastructure that haven't been fully modularized.
Some of the specific issues are:
- `Editor` classes rely on a bunch of `instanceof` stuff in the base class to pick up transaction types like "subscribe", "projects", etc. Instead, applications should be adding these, and third-party applications should be able to add them.
- Code is spread across `Transaction` and `Editor` classes somewhat oddly. For example, generating old/new values would probably make more sense at the `Transaction` level, but it currently exists at the `Editor` level.
- Both types of classes have a lot of functions based on `switch()` statements, which require a ton of boilerplate and are just generally kind of hard to work with.
This creates classes for each type of transaction, and moves almost all of the logic to them. These classes are simpler and more focused than the old stuff was, and can organize related code better.
This starts inching toward defining `CoreTransactions` for features shared across applications. It only defines the "Create" transaction so far, but at some point I plan to move all the other shared transactions to Core and let them control which objects they're available for.
Test Plan:
- Created pastes with web UI and API.
- Edited all paste properites.
- Archived/activated.
- Verified files got reasonable names.
- Reviewed timeline and feed.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T9789
Differential Revision: https://secure.phabricator.com/D16111
Summary:
Ref T11035. This only fixes half of the issue: comment editing has been fixed, but normal transactions which edit things like descriptions haven't yet.
The normal edits aren't fixed because the "oldValues" are populated too late. The code should start working once they get populated sooner, but I don't want to jump the gun on that since it'll probably have some spooky effects. I have some other transaction changes coming down the pipe which should provide a better context for testing "oldValue" population order.
Test Plan:
- Mentioned `@dog` in a comment.
- Removed `@dog` as a subscriber.
- Edited the comment, adding some unrelated text at the end (e.g., fixing a typo).
- Before change: `@dog` re-added as subscriber.
- After change: no re-add.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T11035
Differential Revision: https://secure.phabricator.com/D16108
Summary:
Ref T7643. When a large block of prose text is edited (like a wiki page), summarize the diff when sending mail.
For now, I'm still showing the whole thing in the web UI, since it's a bit more manageable there.
Also try to fix newlines in Airmail.
Test Plan:
This web diff:
{F1682591}
..became this mail diff:
{F1682592}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T7643
Differential Revision: https://secure.phabricator.com/D16098
Summary:
Ran into this while fixing T11098#179088.
The "Transaction Type" details in the conduit autogenerated documentation for `*.edit` endpoints still wraps incorrectly.
Test Plan: Purged remarkup cache, reloaded page, got full-width text.
Reviewers: avivey, chad
Reviewed By: chad
Differential Revision: https://secure.phabricator.com/D16065
Summary:
Ref T7643.
- When a transaction edits a text block, add a link to the changes (for HTML mail).
- Also, inline the changes in the mail (for HTML mail).
- Do nothing for text mail since I don't think we really have room? And I don't know how we can make the diff look any good.
Test Plan:
Edited a task description, generated mail, examined mail.
- It contained a link leading to a prose diff.
- It had a more-or-less reasonable inline text diff.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T7643
Differential Revision: https://secure.phabricator.com/D16063
Summary:
Ref T3353. This hooks the prose engine up to the UI and throws away the hard-wrapping hacks.
These are likely still very rough in many cases, but are hopefully a big step forward from the old version in the vast majority of cases.
Test Plan: {F1677809}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T3353
Differential Revision: https://secure.phabricator.com/D16056
Summary:
Fixes T11088. When a task is removed from a project, we don't normally delete its column positions. If you accidentally remove a project and then restore the project, it's nice for the task to stay where you put it.
However, we do need to remove its positions in proxy columns to avoid the issue in T11088.
Test Plan:
- Added a failing unit test, made it pass.
- Added a task to "X > Milestone 1", loaded workboard, used "Edit Projects" to move it to "X" instead, loaded workboard.
- Before, it stayed in the "Milestone 1" column.
- After, it moves to the "Backlog" column.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T11088
Differential Revision: https://secure.phabricator.com/D16052
Summary:
Ref T11098.
- Allow "Editor" to be set to the empty string.
- Don't match a validation error to a field unless the actual settings for the field and error match.
Test Plan:
- Tried to set "Editor" to "", success.
- Tried to set "Editor" to "javascript://", only that field got marked "Invalid".
Reviewers: avivey, chad
Reviewed By: chad
Maniphest Tasks: T11098
Differential Revision: https://secure.phabricator.com/D16051
Summary:
Ref T4103. This just adds a single global default setting group, not full profiles.
Primarily, I'm not sure how administrators are supposed to set profiles for users, since most ways user accounts get created don't really support setting roles.. When we figure that out, it should be reasonably easy to extend this. There also isn't much of a need for this now, since pretty much everyone just wants to turn off mail.
Test Plan:
- Edited personal settings.
- Edited global settings.
- Edited a bot's settings.
- Tried to edit some other user's settings.
- Saw defaults change appropriately as I edited global and personal settings.
{F1677266}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T4103
Differential Revision: https://secure.phabricator.com/D16048
Summary:
Ref T10077. Currently, we issue 6+ queries on every page to build this menu, since the menu is built application-by-application.
Build the menu with dedicated modules instead so a single "EditEngine" module can provide all of them with one query.
I'd like to reduce this to 0 queries but I'm not totally sure what we want to do with this menu.
This change removes these items, because EditEngine can not currently provide them:
- Calendar: Eventually via EditEngine eventually.
- Conpherence: Probably via EditEngine, doesn't seem too important.
- People: Maybe via EditEngine, doesn't seem too important? "Welcome" is likely better?
- Pholio: Eventually via EditEngine.
It adds a bunch of other items as a side effect:
{F1677151}
This reduces the queries issued on every page by ~5.
This also makes quick create actions visible while logged out (see T7073).
Test Plan:
- Viewed menu while logged in.
- Viewed menu while logged out.
- Viewed standalone version of menu.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10077
Differential Revision: https://secure.phabricator.com/D16045
Summary:
Ref T4103. Ref T10078. This puts a user cache in front of notification and message counts.
This reduces the number of queries issued on every page by 4 (2x building the menu, 2x building Quicksand data).
Also fixes some minor issues:
- Daemons could choke on sending mail in the user's translation.
- No-op object updates could fail in the daemons.
- Questionable data access pattern in the file query coming out of the profile file cache.
Test Plan:
- Sent myself notifications. Saw count go up.
- Cleared them by visiting objects and clearing all notifications. Saw count go down.
- Sent myself messages. Saw count go up.
- Cleared them by visiting threads. Saw count go down.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T4103, T10078
Differential Revision: https://secure.phabricator.com/D16041
Summary:
Ref T4103. This pretty much replaces these panels in-place with similar looking ones that go through EditEngine.
This has a few rough edges but they're pretty minor and/or hard to hit (for example, when editing another user's settings, the crumbs have a redundant link in them).
Test Plan:
- Edited my own settings.
- Edited a bot user's settings.
- Tried to edit another user's settings (failed).
{F1674465}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T4103
Differential Revision: https://secure.phabricator.com/D16017
Summary:
Fixes T10381. When we converted to `PHUIRemarkupView`, some instructional text got linebreaks added when it shouldn't have them (the source is written in PHP and wrapped at 80 characters, but the output should flow naturally).
Fix this so we don't preserve linebreaks.
This also makes `PHUIRemarkupView` a little more powerful and inches us toward fixing Phame/CORGI remarkup issues, getting rid of `PhabricatorMarkupInterface` / `PhabricatorMarkupOneOff`, and dropping all the application hard-coding in `PhabricatorMarkupEngine`.
Test Plan:
- Grepped for all callsites, looking for callsites which accept remarkup written in `<<<HEREDOC` format.
- Viewed form instructions, Conduit API methods, HTTP parameter edit instructions.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10381
Differential Revision: https://secure.phabricator.com/D15963
Summary: Converts to table so text wraps on long strings well, button always stays top right, better spacing underneath.
Test Plan: Mail, Gmail, mobile
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D15955
Summary:
Ref T10694. If this feels good, I'd plan to eventually add something similar to other applications ("View Task", etc).
Not sure if we should keep the object link later in the mail body or not. I left it for now.
Test Plan: {F1307256, size=full}
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Maniphest Tasks: T10694
Differential Revision: https://secure.phabricator.com/D15884
Summary: Fixes T10493. See that task and inline comments for discussion.
Test Plan:
Created an object with some projects, saw the transaction in resulting mail:
{F1600496}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10493
Differential Revision: https://secure.phabricator.com/D15942
Summary: Fixes T10972. Nothing actually updates this anymore, and only repositories ever did (e.g., Harbormaster and Drydock have never tracked it). Keeping track of this is more trouble than it's worth.
Test Plan: Grepped for constants, viewed a passphrase credential.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10972
Differential Revision: https://secure.phabricator.com/D15932
Summary:
Ref T10939. Fixes T8887. This enables and implements the "review" and "blocking review" options for packages.
This is a bit copy-pastey from `DifferentialReviewersHeraldAction`, which doesn't feel awesome. I think the right fix is Glorious Infrasturcture, though -- I filed T10967 to track that.
Test Plan:
- Set package autoreveiw to "Review".
- Updated, got a reveiwer.
- Set autoreview to "blocking".
- Updated, got a blocking reviewer.
{F1311720}
{F1311721}
{F1311722}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T8887, T10939
Differential Revision: https://secure.phabricator.com/D15916
Summary:
Ref T10939. Ref T8887. This moves toward letting packages automatically become reviewers or blocking reviewers of owned code.
This change adds an "Auto Review" option to packages. Because adding reviewers/blocking reviewers is a little tricky, it doesn't actually have these options yet -- just a "subscribe" option. I'll do the reviewer work in the next update.
Test Plan:
Created a revision in a package with "Auto Review: Subscribe to Changes". The package got subscribed.
{F1311677}
{F1311678}
{F1311679}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T8887, T10939
Differential Revision: https://secure.phabricator.com/D15915
Summary: Fixes T10952. Fixes T10930. I didn't implement this method correctly when I expanded this field for repositories.
Test Plan: Edited a paste without warnings.
Reviewers: avivey, chad
Reviewed By: chad
Maniphest Tasks: T10930, T10952
Differential Revision: https://secure.phabricator.com/D15892
Summary: Ref T10923. This provides a little guidance about hosted vs observed, and points at the `diffusion.ssh-*` options.
Test Plan: Poked around in the web UI, saw useful guidance.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10923
Differential Revision: https://secure.phabricator.com/D15872
Summary:
Ref T10923. Paging wasn't being applied correctly when creating //new// repositories after API changes.
Also, the first redirect after creation wasn't sending users to the right place.
Test Plan:
- Created a new repostiory, got redirected properly.
- Verified that new repostiory flow has the correct fields (name, callsign, etc) and Conduit API has the correct fields (everything).
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10923
Differential Revision: https://secure.phabricator.com/D15865
Summary:
Ref T10748. This needs more extensive testing and is sure to have some rough edges, but seems to basically work so far.
Throwing this up so I can work through it more deliberately and make notes.
Test Plan:
- Ran migration.
- Used `bin/repository list` to list existing repositories.
- Used `bin/repository update <repository>` to update various repositories.
- Updated a migrated, hosted Git repository.
- Updated a migrated, observed Git repository.
- Converted an observed repository into a hosted repository by toggling the I/O mode of the URI.
- Conveted a hosted repository into an observed repository by toggling it back.
- Created and activated a new empty hosted Git repository.
- Created and activated an observed Git repository.
- Updated a mirrored repository.
- Cloned and pushed over HTTP.
- Tried to HTTP push a read-only repository.
- Cloned and pushed over SSH.
- Tried to SSH push a read-only repository.
- Updated several Mercurial repositories.
- Updated several Subversion repositories.
- Created and edited repositories via the API.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10748
Differential Revision: https://secure.phabricator.com/D15842
Summary:
Ref T10748. This allows an EditEngine form to be broken up into pages.
This is less powerful than `PHUIPagedFormView`, because the pages are not sequential / stateful. Each form saves immediately once it's submitted, and can not take you to a new form or back/forward in a series of forms.
For example, you can't create a workflow where the user fills out 5 pages of information before we create an object, like the current repository workflow does.
However, the only place we've ever wanted to do this is repositories and it's fairly bad there, so I feel reasonably confident we aren't going to miss this in the future.
(We do "choose a type of service/repository/rule -> fill out one page of info" fairly often, but can do this without the full-power paging stuff.)
Test Plan:
- Created a repository usin the new Manage UI, filling out only a handful of fields.
- Edited a repository using the new Manage UI.
- All forms are now EditEngine forms offering paged views of the big huge underlying form:
{F1254371}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10748
Differential Revision: https://secure.phabricator.com/D15832
Summary: Ref T10748. This brings the "Actions" items (publish/notify + autoclose enabled) into the new UI.
Test Plan:
- Edited this stuff via EditEngine and Conduit.
- Viewed via new Manage UI.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10748
Differential Revision: https://secure.phabricator.com/D15811
Summary: Ref T10748. Makes a "Branches" panel, enables these transactions in the EditEngine.
Test Plan:
- Edited via EditEngine + Conduit.
- Viewed via manage UI.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10748
Differential Revision: https://secure.phabricator.com/D15809
Summary: Ref T10748. Brings this forward in the UI and EditEngine.
Test Plan:
- Edited via Conduit.
- Viewed via Manage UI.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10748
Differential Revision: https://secure.phabricator.com/D15805
Summary:
Fixes T8952. When Herald changes subscribers, it is zzzzz very boring.
When users change subscribers, it is still super boring (more boring than a merge, for example).
Test Plan: Viewed feed, saw fewer Herald stories.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T8952
Differential Revision: https://secure.phabricator.com/D15774
Summary:
Ref T8952. Currently, when an application (most commonly Herald, but sometimes Drydock, Diffusion, etc) publishes a feed story, we get an empty grey box for it in feed.
Instead, give the story a little application icon kind of "profile picture"-like thing.
Test Plan:
Here's how it looks:
{F1239003}
Feel free to tweak/counter-diff.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T8952
Differential Revision: https://secure.phabricator.com/D15773
Summary: This is completely obsoleted by `owners.search`. See D15472.
Test Plan: Viewed API method in UI console.
Reviewers: avivey, chad
Reviewed By: chad
Differential Revision: https://secure.phabricator.com/D15769
Summary: Fixes T10684. Fixes T10520. This primarily implements a date/epoch field, and then does a bunch of standard plumbing.
Test Plan:
- Created countdowns.
- Edited countdowns.
- Used HTTP prefilling.
- Created a countdown ending on "Christmas Morning", etc.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10520, T10684
Differential Revision: https://secure.phabricator.com/D15655
Summary: Ref T6027. We got a not-very-user-friendly default string before.
Test Plan: Selected "Move", didn't change the dropdown, hit submit. Now, got a nice human-readable description of the issue.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T6027
Differential Revision: https://secure.phabricator.com/D15649
Summary:
Ref T10262. Files have an internal secret key which is partially used to control access to them, and determines part of the URL you need to access them. Scramble (regenerate) the secret when:
- the view policy for the file itself changes (and the new policy is not "public" or "all users"); or
- the view policy or space for an object the file is attached to changes (and the file policy is not "public" or "all users").
This basically means that when you change the visibility of a task, any old URLs for attached files stop working and new ones are implicitly generated.
Test Plan:
- Attached a file to a task, used `SELECT * FROM file WHERE id = ...` to inspect the secret.
- Set view policy to public, same secret.
- Set view policy to me, new secret.
- Changed task view policy, new secret.
- Changed task space, new secret.
- Changed task title, same old secret.
- Added and ran unit tests which cover this behavior.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10262
Differential Revision: https://secure.phabricator.com/D15641
Summary:
Ref T6027. Normally, actions use the same order as the form, but in some cases (like moving stuff on workboards) it makes sense to reorder them explicitly.
Pin "Move on board" near the bottom, and "projects/subscribers" at the bottom. I think these are generally reasonable rules in all cases.
Test Plan: Opened menu, saw slightly better action order.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T6027
Differential Revision: https://secure.phabricator.com/D15639
Summary:
Ref T6027. Try this out and see how it feels? Clear issues:
- This definitely shouldn't be at the top.
- You should probably be able to select it multiple times?
- Some of the "which columns show up" rules might need adjustment?
- Diamond marker maybe not great?
Not sure I love this but it doesn't feel //terrible//...
Test Plan: {F1207891}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T6027
Differential Revision: https://secure.phabricator.com/D15638
Summary: Ref T5214. Fixes T10486. Ref T6027. This exposes the `TYPE_COLUMNS` transaction in a usable way via API, and fixes the interactions via prefilling.
Test Plan:
- Created tasks directly into columns via API.
- Moved tasks between columns via API.
- Used `?column=...` to try to create a template task with valid and bogus column PHIDs.
Reviewers: chad
Reviewed By: chad
Subscribers: AmyLewis
Maniphest Tasks: T5214, T6027, T10486
Differential Revision: https://secure.phabricator.com/D15636
Summary: Ref T6027. This adds human-readable rendering for the new `TYPE_COLUMNS` core transactions.
Test Plan: {F1207784}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T6027
Differential Revision: https://secure.phabricator.com/D15635
Summary:
Ref T6027. We currently have two different transaction types:
- `TYPE_PROJECT_COLUMNS` does most of the work, but has a sort of weird structure and isn't really suitable for API use.
- `TYPE_COLUMN` is this weird, junk transaction which mostly just creates the other transaction.
Merge them into a single higher-level `TYPE_COLUMNS` transaction which works properly and has a sensible structure and comprehensive error checking.
Remaining work here:
- I've removed the old rendering logic, but not yet added new logic. I need to migrate the old transaction types and add new rendering logic.
- Although the internal representation is now //suitable// for use in the API, it isn't properly exposed yet.
Test Plan:
- Created tasks into a column.
- Ran unit tests.
- Moved tasks between columns.
- Will perform additional testing in followups.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T6027
Differential Revision: https://secure.phabricator.com/D15634
Summary: View various conduit pages and update to new UI and add calls to newPage
Test Plan: View list, view method, make a call.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D15613
Summary: Cleans up EditEngine, adds new layout to EditEngine and descendents
Test Plan: Test creating a new form, reordering, marking and unmarking defaults. View new forms.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D15531
Summary:
Ref T10537. For Nuance, I want to introduce new sources (like "GitHub" or "GitHub via Nuance" or something) but this needs to modularize eventually.
Split ContentSource apart so applications can add new content sources.
Test Plan:
This change has huge surface area, so I'll hold it until post-release. I think it's fairly safe (and if it does break anything, the breaks should be fatals, not anything subtle or difficult to fix), there's just no reason not to hold it for a few hours.
- Viewed new module page.
- Grepped for all removed functions/constants.
- Viewed some transactions.
- Hovered over timestamps to get content source details.
- Added a comment via Conduit.
- Added a comment via web.
- Ran `bin/storage upgrade --namespace XXXXX --no-quickstart -f` to re-run all historic migrations.
- Generated some objects with `bin/lipsum`.
- Ran a bulk job on some tasks.
- Ran unit tests.
{F1190182}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10537
Differential Revision: https://secure.phabricator.com/D15521
Summary: See D15432. There, we can use this test to check if the user //could// reassign the task by using "Edit Form" or the stacked actions, so any dedicated "claim" element is consistent with the other permissions.
Test Plan:
- Added a `var_dump($can_reassign)` after the call.
- Saw `true`.
- Edited the edit form, locked and disabled "Assigned To".
- Saw `false`.
Reviewers: chad
Reviewed By: chad
Differential Revision: https://secure.phabricator.com/D15433
Summary:
Fixes T10519. Two issues:
First, the acting user wasn't explicitly included in the mail. This usually didn't matter, but could matter if you unsubscribed and then interacted.
Second, we had some logic which tried to hide redundant "added inline comment" transactions, but could hide them inappropriately. In particular, if another action (like a subscribe) was present in the same group, we could hide the inlines because of that other transaction, then //also// hide the subscribe. This particular issue is likely an unintended consequence of hiding self-subscribes.
Instead of hiding inlines if //anything else// happened, hide them only if:
- there is another "added a comment" transaction; or
- there is another "added an inline comment" transaction.
This prevents the root issue in T10519 (incorrectly hiding every transaction, and thus not sending the mail) and should generally make behavior a little more consistent and future-proof.
Test Plan:
- Submitted //only// an inline comment on a commit I had not previously interacted with.
- Before patch: no mail was generated (entire mail was improperly hidden).
- After patch: got some mail with my comment.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10519
Differential Revision: https://secure.phabricator.com/D15407
Summary:
Ref T10457. Fixes T10024. This primarily just modernizes blueprints to use EditEngine.
This also fixes T10024, which was an issue with stored properties not being flagged correctly.
Also slightly improves typeaheads for blueprints (more information, disabled state).
Test Plan:
- Created and edited various types of blueprints.
- Set and removed limits.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10024, T10457
Differential Revision: https://secure.phabricator.com/D15390
Summary: Full new UI, testing some upcoming treatments for consideration in other View controllers. Small tweaks to allow PHUITwoColumnView to have fixed and fluid width, and let TransactionCommentView go fullWidth.
Test Plan:
Tested a number of Ponder cases, New Question, with and without summary, with and without answers, with and without comments. Mobile, Tablet, and Desktop layouts. Verify Project and Profile UI's still in tact.
{F1120961}
{F1120962}
{F1120963}
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D15315
Summary:
Fixes T10410. Immediate impact of this is that you can now actually delete properties from Almanac services, devices and bindings.
The meat of the change is switching from CustomField to EditEngine for most of the actual editing logic. CustomField creates a lot of problems with using EditEngine for everything else (D15326), and weird, hard-to-resolve bugs like this one (not being able to delete stuff).
Using EditEngine to do this stuff instead seems like it works out much better -- I did this in ProfilePanel first and am happy with how it looks.
This also makes the internal storage for properties JSON instead of raw text.
Test Plan:
- Created, edited and deleted properties on services, devices and bindings.
- Edited and reset builtin properties on repository services.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10410
Differential Revision: https://secure.phabricator.com/D15327
Summary:
WMF ran into this after their update. Here's the setup:
- When you enable Spaces, we leave all existing objects set to `null`, which means "these belong to the default space". This is so we don't have to go update a trillion objects.
- New objects get set to the default space explicitly (`PHID-SPCE-...`) but older ones stay with `null`.
- If you edit an older object (like a task) from the time before Spaces, //and// the form doesn't have a Visbility/Spaces control, we would incorrectly poplate the value with `null` when the effective value should be the default space PHID.
- This caused a "You must choose a space." error in the UI.
Instead, populate the control with the effective value instead of the literal database value. This makes the edit go through cleanly.
Also add a note about this for future-me.
Test Plan:
- Disabled "Visibility" control in task edit form.
- Edited an old task which had `null` as a `spacePHID` in the database.
- Before patch: UI error about selecting a Space.
- After patch: edit goes through cleanly.
Reviewers: chad, 20after4
Reviewed By: chad, 20after4
Subscribers: 20after4, aklapper
Differential Revision: https://secure.phabricator.com/D15306
Summary: Moves all the one off object calls to PHUIRemarkupView, adds a "Document" call as well (future plans).
Test Plan: Visited most pages I could get access to, but may want extra careful eyes on this diff.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D15281
Summary:
Ref T10349. These got sort of half-weirded-up before I separated subscriptions and watching fully. New rules are:
- You can watch whatever you want.
- Watching a parent watches everything inside it.
- If you're watching "Stonework" and go to "Stonework > Masonry", you'll see a "Watching Ancestor" hint to let you know you're already watching a parent or ancestor.
Test Plan:
- Watched and unwatched "Stonework".
- Watched and unwatched "Stonework > Iteration IV".
- While watching "Stonework", visited "Iteration IV" and saw "Watching Ancestor" hint.
- Created a task tagged "Stonework > Iteration IV". Got notified about it because I watch "Stonework".
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10349
Differential Revision: https://secure.phabricator.com/D15280
Summary: Fixes T10347. In the long run maybe we'll try to guess this better, but for now get rid of the "Milestone X" hardcode and just show what the last one was called.
Test Plan:
- Created the first milestone for a project.
- Created the nth milestone for a project.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10347
Differential Revision: https://secure.phabricator.com/D15262
Summary:
Ref T4427.
- New config option for labels, enabling, etc., but no UI/niceness yet.
- When enabled, add a field.
- Allow nonnegative values, including fractional values.
- EditEngine is nice and Conduit / actions basically just work with a tiny bit of extra support code.
Test Plan:
- Edited points via "Edit".
- Edited points via Conduit.
- Edited points via stacked actions.
- Tried to set "zebra" points.
- Tried to set -1 points.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T4427
Differential Revision: https://secure.phabricator.com/D15220
Summary:
Ref T10010. When you try to add "Sprint 35" to a task, remove "Sprint 34", etc. Briefly:
- A task can't be in Sprint 3 and Sprint 4.
- A task can't be in "A" and "A > B" (but "A > B" and "A > C" are fine).
- When a user makes an edit which would violate one of these rules, preserve the last tag in each group of conflicts.
Test Plan:
- Added fairly comprehensive tests.
- Added a bunch of different tags to things, saw them properly exclude conflicting tags.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10010
Differential Revision: https://secure.phabricator.com/D15167
Summary: Mostly for consistency, we're not using other forms of icons and this makes all classes that use an icon call it in the same way.
Test Plan: tested uiexamples, lots of other random pages.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D15125
Summary: Ref T10168. When we render this control, we currently don't put commas into the value correctly if there are multiple alternative hashtags.
Test Plan: Edited a project with multiple alternate hashtags. Before change: they all got smushed together. After change: properly comma-separated.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10168
Differential Revision: https://secure.phabricator.com/D15045
Summary:
Ref T10054. This does a big chunk of the legwork to let users reconfigure profile menus (currently, just project menus).
This includes:
- Editing builtin items (e.g., you can rename the default items).
- Creating new items (for now, only links are available).
This does not yet include:
- Hiding items.
- Reordering items.
- Lots of fancy types of items (dashboards, etc).
- Any UI changes.
- Documentation (does feature: TODO link for documentation).
Test Plan:
{F1060695}
{F1060696}
{F1060697}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10054
Differential Revision: https://secure.phabricator.com/D15010
Summary:
Fixes T10117.
- I accidentally broke setting `null` to unassign tasks at some point when I added richer validation.
- Raise a better error if the user passes junk.
Test Plan:
- Unassigned a task via API and web UI.
- Reassigned a task via API and web UI.
- Tried to do an invalid assign via API, got a sensible error.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10117
Differential Revision: https://secure.phabricator.com/D14992
Summary:
Ref T10010.
- Default name to "Milestone X".
- Remove policy controls, which have no effect.
- Don't generate slugs for milestones since this is a big pain where they all generate as `#milestone_1` by default (you can add one if you want). I plan to add some kind of syntax like `#parent/32` to mean "Milestone 32 in Parent" later.
- Don't require projects to have unique names (again, 900 copies of "Milestone X"). I think we can trust users to sort this out for themselves since modern Phabricator has "Can Create Projects" permission, etc.
Test Plan: Created some milestones, had a less awful experience.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10010
Differential Revision: https://secure.phabricator.com/D14909
Summary:
Ref T9897. This one is a little more involved because of how getting a post on a blog works.
I also changed moving posts to be a real transaction (which shows up in history, now).
Test Plan: Created posts from web UI and conduit.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T9897
Differential Revision: https://secure.phabricator.com/D14902
Summary: Ref T10010. This is pretty straightforward with a couple of very minor new behaviors, like the icon selector edit field.
Test Plan:
- Created projects.
- Edited projects.
- Saw "Create Project" in quick create menu.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10010
Differential Revision: https://secure.phabricator.com/D14896
Summary: Ref T10004. This primarily supports moving Phame to EditEngine.
Test Plan: {F1045166}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10004
Differential Revision: https://secure.phabricator.com/D14887
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: 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:
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 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:
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. 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: 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:
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. 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 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: 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 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:
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 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. 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 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: 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. 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: 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. 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:
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:
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. 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.
- 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: 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:
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:
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. 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. 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. 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:
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:
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. 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: 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
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: 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 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:
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:
Fixes T9787. Currently, file PHID extraction logic happens very early, before we normalize/merge/etc the transactions.
In D14390, I changed how the CONTENT transaction works: before, callers would pass in a file PHID. Afterward, they just pass in the content.
Passing in the content is generaly easier and feels more correct, but inadvertenly broke PHID extraction because converting the content into a file PHID now happened after we extracted the PHID. So we'd extract the entire text of the paste as a "file PHID", which wouldn't work.
Instead, extract file PHIDs later. This impacts a couple of other applications (Conpherence, Pholio) which receive an object or have an unusual file-oriented transaction.
Test Plan:
- Made a new paste, verified the raw file attached to it properly.
- Made and updated a mock, verified all the files attached properly.
- Updated a Conpherence room image, verified the files attached properly.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T9787
Differential Revision: https://secure.phabricator.com/D14494
Summary:
Fixes T9799. Currently, if you can't see an application like Paste, we fatal when trying to generate a result for `conduit.query`, because the new EditEngine-based `paste.edit` method doesn't "know" that it's a "Paste" method.
Straighten this out, and use policies and queries a little more correctly/consistently.
Test Plan:
- Called `conduit.query` as a user who does not have permission to use Paste.
- Before change: fatal.
- After change: results, excluding "paste.*" methods.
Reviewers: chad
Reviewed By: chad
Subscribers: cburroughs
Maniphest Tasks: T9799
Differential Revision: https://secure.phabricator.com/D14492
Summary: Fixes T9772. We now need an EditEngineConfiguration to do interesting things with EditEngine, but this public API wasn't properly making sure we have one.
Test Plan: Called `conduit.query` from web console. Fatal prior to patch; success afterward.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T9772
Differential Revision: https://secure.phabricator.com/D14475
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:
Fixes T9735. I changed how the TYPE_LANGUAGE transction works a little but that accidentally tripped an error condition in `paste.create`.
- Don't bail on no-effect transactions to `paste.create` (like not setting a language).
- When a transaction type has no tailored UI message, make it easier to figure out which transaction is problematic.
Test Plan: Ran `arc paste ...` locally. Got an error before the patch, clean paste creation afterward.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T9735
Differential Revision: https://secure.phabricator.com/D14440
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 T9132. This just moves code around, breaks it up into some smaller chunks, tries to reduce duplication, and adds a touch of documentation.
Test Plan: Created and edited pastes.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T9132
Differential Revision: https://secure.phabricator.com/D14398
Summary: Ref T8992, Validate alias text field length.
Test Plan: Create Phurl with alias of more than 64 characters. Get error. Reduce length of alias to successfully save Phurl.
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: Korvin
Maniphest Tasks: T8992
Differential Revision: https://secure.phabricator.com/D14403
Summary: Use in MailCommands and HTTP Parameters
Test Plan: Tested MailCommands in Paste, HTTP Parameters in Paste, Legalpad, Diviner. Mobile and Desktop breakpoints.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D14397
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
Summary: Ref T9352. See D13635. Build targets can have variables already, but let builds have them too. This mostly enables future use cases (sub-builds, more sophisticated build triggers).
Test Plan: With a custom Herald rule + action like the one in T9352, updated a revision and saw it generate multiple builds with varying parameters.
Reviewers: chad, hach-que
Reviewed By: hach-que
Maniphest Tasks: T9352
Differential Revision: https://secure.phabricator.com/D14222
Summary: Ref T9272. This doesn't fix anything, just a little cleanup while I was looking at it.
Test Plan: Clicked "Show Details" on a couple description changes, got the same effect for less code.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T9272
Differential Revision: https://secure.phabricator.com/D14168
Summary: Fixes T9369.
Test Plan:
- Sent a mail with Mail.app to `bugs@local.phacility.com`.
- Used "View Raw Mail", copy-pasted it into `mail.txt` on disk.
- Ran `cat mail.txt | ./scripts/mail/manage_mail.php --process-duplicates`.
- Saw task get created and me get added as CC.
- Changed "To" to include another user, ran command again, saw task get created and other user get added as CC.
Reviewers: chad
Reviewed By: chad
Subscribers: Korvin
Maniphest Tasks: T9369
Differential Revision: https://secure.phabricator.com/D14086
Summary:
Ref T8672. Ref T9187. Root issue in at least one case is:
- User makes a commit including a file with some non-UTF8 text (say, a Japanese file full of Shift-JIS).
- We pass the file to the TransactionEditor so it can inline or attach the patch if the server is configured for these things.
- When inlining patches, we convert them to UTF8 before inlining. We must do this since the rest of the mail is UTF8.
- When attaching patches, we send them in the original encoding (as file attachments). This is correct, and means we need to give the worker the raw patch in whatever encoding it was originally in: we can't just convert it to utf8 earlier, or we'd attach the wrong patch in some cases.
- TransactionEditor does its thing (e.g., creates the commit), then gets ready to send mail about whatever it did.
- The publishing work now happens in the daemon queue, so we prepare to queue a PublishWorker and pass it the patch (with some other data).
- When we queue workers, we serialize the state data with JSON.
So far, so good. But this is where things go wrong:
- JSON can't encode binary data, and can't encode Shift-JIS. The encoding silently fails and we ignore it.
Then we get to the worker, and things go wrong-er:
- Since the data is bad, we fatal. This isn't a permanent failure, so we continue retrying the task indefinitely.
This applies several fixes:
# When queueing tasks, fail loudly when JSON encoding fails.
# In the worker, fail permanently when data can't be decoded.
# Allow Editors to specify that some of their data is binary and needs special handling.
This is fairly messy, but some simpler alternatives don't seem like good ways forward:
- We can't convert to UTF8 earlier, because we need the original raw patch when adding it as an attachment.
- We could encode //only// this field, but I suspect some other fields will also need attention, so that adding a mechanism will be worthwhile. In particular, I suspect filenames //may// be causing a similar problem in some cases.
- We could convert task data to always use a serialize()-based binary safe encoding, but this is a larger change and I think it's correct that things are UTF8 by default, even if it makes a bit of a mess. I'd rather have an explicit mess like this than a lot of binary data floating around.
The change to make `LiskDAO` will almost certainly catch some other problems too, so I'm going to hold this until after `stable` is cut. These problems were existing problems (i.e., the code was previously breaking or destroying data) so it's definitely correct to catch them, but this will make the problems much more obvious/urgent than they previously were.
Test Plan:
- Created a commit with a bunch of Shift-JIS stuff in a file.
- Tried to import it.
Prior to patch:
- Broken PublishWorker with distant, irrelevant error message.
With patch partially applied (only new error checking):
- Explicit, local error message about bad key in serialized data.
With patch fully applied:
- Import went fine and mail generated.
Reviewers: chad
Reviewed By: chad
Subscribers: devurandom, nevogd
Maniphest Tasks: T8672, T9187
Differential Revision: https://secure.phabricator.com/D13939
