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
Summary: Ref T8700, I don't believe we need to be specific here about the object, since it displays on the object.
Test Plan: Change policy a few times on a task, see new translation
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Maniphest Tasks: T8700
Differential Revision: https://secure.phabricator.com/D13913
Summary: Ref T5791. This is still very basic (no global actions, no support for matching headers/bodies/recipients/etc) but gets the core in.
Test Plan:
{F715209}
{F715211}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T5791
Differential Revision: https://secure.phabricator.com/D13897
Summary: Use `PhutilClassMaQuery` instead of `PhutilSymbolLoader`, mostly for consistency. Depends on D13588.
Test Plan: Poked around a bunch of pages.
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: epriestley, Korvin
Differential Revision: https://secure.phabricator.com/D13589
Summary: Adds additional icon states for subscriber transactions. Also updated "eraser" to "trash" (man that icon is bad).
Test Plan: add a subscriber, remove a subscriber.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D13800
Summary: Ref T8099, Cleans up UI issues, adds `appendList` and renders lists and paragraphs with Remarkup UI.
Test Plan: Test Policy Dialogs, other various dialogs.
Reviewers: btrahan, epriestley
Reviewed By: epriestley
Subscribers: epriestley, Korvin
Maniphest Tasks: T8099
Differential Revision: https://secure.phabricator.com/D13463
Summary: Fixes T8703. The URI handling here was a little unusual.
Test Plan:
- Edited and deleted comments in several applications, including Macro.
- As an admin, deleted others' comments.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T8703
Differential Revision: https://secure.phabricator.com/D13469
Summary:
Ref T5791. This diff adds a "sensitive" flag to `PhabricatorMetaMTAMail`, defaults it to true in the constructor, and then sets it to false in teh application transaction editor. Assumption here is that sensitive emails are basically all the emails that don't flow through the application transaction editor.
This diff also gets a basic "mail view" page up and going.
This diff also fixes a bug writing recipient edges; the actor was being included.
This bug also fixes a querying bug; we shouldn't do the automagic join of $viewer is recipient or $viewer is actor if folks are querying for recipients or actors already. The bug manifested itself as having the "inbox" be inbox + outbox.
Test Plan: viewd list of messages. viewed message detail.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: epriestley, Korvin
Maniphest Tasks: T5791
Differential Revision: https://secure.phabricator.com/D13406
Summary:
Fixes T4139. Adds a "Desktop Notifications" panel to settings. For now, we start with "Send Desktop Notifications Too" functionality. We can try to be fancy later and only send desktop notifications if the web app doesn't have focus, etc.
Test Plan:
Made some comments as a test user on a task and got purdy desktop notifications using Chrome. Then did it again with Firefox.
Played around with permissions form with Chrome and got helpful information about what was up. Played around with Firefox and got similar results, except canceling the dialogue didn't invoke my handler code somehow. Oh Firefox!
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: rbalik, tycho.tatitscheff, joshuaspence, epriestley, Korvin
Maniphest Tasks: T4139
Differential Revision: https://secure.phabricator.com/D13219
Summary:
Ref T8574. Currently, failures during mail body construction, feed publishing, or search indexing could cause us to retry the publishing task and potentially send duplicate mail.
Instead, build (but do not send) the mail first, then send all the mail at the very end.
This isn't completley perfect, but should make it enormously harder for duplicate mail to be generated.
Test Plan: Sent some mail, ran the daemons, saw it show up normally in the outbound queue.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T8574
Differential Revision: https://secure.phabricator.com/D13320
Summary: All classes should extend from some other class. See D13275 for some explanation.
Test Plan: `arc unit`
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: epriestley, Korvin
Differential Revision: https://secure.phabricator.com/D13283
Summary:
Ref T5681. Getting this to work correctly is a bit tricky, mostly because of the policy checks we do prior to applying an edit.
I think I came up with a mostly-reasonable approach, although it's a little bit gross. It uses `spl_object_hash()` so it shouldn't be able to do anything bad/dangerous (the hints are strictly bound to the hinted object, which is a clone that we destroy moments later).
Test Plan:
- Added + ran a unit test.
- Created a task with a "Subscribers" policy with me as a subscriber (without the hint stuff, this isn't possible: since you aren't a subscriber *yet*, you get a "you won't be able to see it" error).
- Unsubscribed from a task with a "Subscribers" policy, was immediately unable to see it.
- Created a task with a "subscribers" policy and a project subscriber with/without me as a member (error / success, respectively).
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T5681
Differential Revision: https://secure.phabricator.com/D13259
Summary:
Ref T5681. Ref T8488. This allows policy rules to provide "Object Policies", which are similar to the global/basic policies:
- They show up directly in the dropdown (you don't have to create a custom rule).
- They don't need to create or load anything in the database.
To implement one, you just add a couple methods on an existing PolicyRule that let Phabricator know it can work as an object policy rule.
{F494764}
These rules only show up where they make sense. For example, the "Task Author" rule is only available in Maniphest, and in "Default View Policy" / "Default Edit Policy" of the Application config.
This should make T8488 easier by letting us set the default policies to "Members of Thread", without having to create a dedicated custom policy for every thread.
Test Plan:
- Set tasks to "Task Author" policy.
- Tried to view them as other users.
- Viewed transaction change strings.
- Viewed policy errors.
- Set them as default policies.
- Verified they don't leak into other policy controls.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T5681, T8488
Differential Revision: https://secure.phabricator.com/D13257
Summary:
Ref T8498. Allow ApplicationEmail addresses to be put into spaces:
- You can only see and send to addresses in Spaces you have access to.
- Objects are created into the same space their address is associated with.
Test Plan:
- Used `bin/mail receive-test` to send mail to various `xyz-bugs@...` addresses.
- Saw objects created in the proper space.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T8498
Differential Revision: https://secure.phabricator.com/D13247
Summary:
Ref T8498. I want to add Spaces to these, and the logic for getting Spaces right is a bit tricky, so swap these to ApplicationTransactions.
One new piece of tech: made it easier for Editors to raise DuplicateKeyException as a normal ValidationException, so callers don't have to handle this case specially.
One behavioral change: we no longer require these addresses to be at the `auth.email-domains` domains -- I think this wasn't quite right in the general case. It's OK to require users to have `@mycompany.com` addresses but add `@phabricator.mycompany-infrastructure.com` addresses here if you want.
Test Plan:
- Tried to create a duplicate email.
- Tried to create an empty email.
- Tried to create an invalid email.
- Created a new email.
- Deleted an email.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T8498
Differential Revision: https://secure.phabricator.com/D13246
Summary:
Ref T8377. This adds a standard disable/enable feature to Spaces, with a couple of twists:
- You can't create new stuff in an archived space, and you can't move stuff into an archived space.
- We don't show results from an archived space by default in ApplicationSearch queries. You can still find these objects if you explicitly search for "Spaces: <the archived space>".
So this is a "put it in a box in the attic" sort of operation, but that seems fairly nice/reasonable.
Test Plan:
- Archived and activated spaces.
- Used ApplicationSearch, which omitted archived objects by default but allowed searches for them, specifically, to succeed.
- Tried to create objects into an archived space (this is not allowed).
- Edited objects in an archived space (this is OK).
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T8377
Differential Revision: https://secure.phabricator.com/D13238
Summary: Fixes T8464. We could lose the additional users from "Send an email..." rules //if// Herald did not apply any other transactions to the task.
Test Plan:
- Destroyed all Herald rules.
- Created a single "Send an email to..." rule.
- Created a task.
- Saw target get an email.
Reviewers: btrahan, chad
Reviewed By: chad
Subscribers: epriestley
Maniphest Tasks: T8464
Differential Revision: https://secure.phabricator.com/D13245
Summary:
Ref T8449. Try out some more subtle behaviors:
- Make the "Space" control part of the policy control, so the UI shows "Visible To: [Space][Policy]". I think this helps make the role of spaces more clear. It also makes them easier to implement.
- Don't show the default space in headers: instead, show nothing.
- If the user has access to only one space, pretend spaces don't exist (no edit controls, no header stuff).
This might be confusing, but I think most of the time it will all align fairly well with user expectation.
Test Plan:
- Viewed a list of pastes (saw Space with non-default space, no space with default space, no space with user in only one space).
- Viewed a paste (saw Space with non-default space, saw no space with default space, saw no space with user in only one space).
- Edited spaces on objects (control as privileged user, no control as locked user).
- Created a new paste in a space (got space select as privileged user, no select as locked user).
Reviewers: chad, btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T8449
Differential Revision: https://secure.phabricator.com/D13229
Summary:
Fixes T8483. I did this incorrectly in D13159, by doing it correctly first and then editing it carelessly. For most transaction types, it didn't matter, but did for inline state.
Also, clean up any bad inline state transactions.
Test Plan:
- Ran migration, bad transactions vanished.
- Marked some inline comments as done.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T8483
Differential Revision: https://secure.phabricator.com/D13226
Summary: Now that Users implement PhabricatorApplicationTransactionInterface, we try to write an inverse edge. At least for now, we should retain the old behavior instead.
Test Plan:
- Unit tests which cover this stuff pass again.
- Grepped for other `instanceof PhabricatorApplicationTransactionInterface`, the all seemed either benign or irrelevant.
Reviewers: joshuaspence, btrahan
Reviewed By: btrahan
Subscribers: epriestley
Differential Revision: https://secure.phabricator.com/D13215
Summary:
Fixes T8464. We could incorrectly use a cached value when computing CC's.
Just load a fresh value. There are no other callers that would benefit from this cache, so it's more complicated to reload it correctly prior to publishing than to just skip it.
Also make the PHID headers unique.
Test Plan:
- Verified that users received mail about the transactions which caused them to be added to an object.
- Veirfied that headers no longer have redundant values.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T8464
Differential Revision: https://secure.phabricator.com/D13206
Summary:
Ref T8424. This adds crude integration with Paste's edit/view workflows: you can change the space a Paste appears in, see transactions, and get a policy callout.
Lots of rough edges and non-obviousness but it pretty much works.
Test Plan:
- Created and updated Pastes.
- Moved them between spaces, saw policy effects.
- Read transactions.
- Looked at feed.
- Faked query to return no spaces, saw control and other stuff vanish.
- Faked query to return no spaces, created pastes.
- Tried to submit bad values and got errors.
Reviewers: chad, btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T8424
Differential Revision: https://secure.phabricator.com/D13159
Summary: Ref T6367.
Test Plan:
- Added and executed unit tests.
- Sent mail to A (en_US) and B (en_A*).
- Got one mail in English and one mail in ENGLISH.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T6367
Differential Revision: https://secure.phabricator.com/D13142
Summary:
Ref T6367. Removes `multiplexMail()`!
We can't pass a single body into a function which splits it anymore: we need to split recipients first, then build bodies for each recipient list. This lets us build separate bodies for each recipient's individual translation/access levels.
The new logic does this:
- First, split recipients into groups called "targets".
- Each target corresponds to one actual mail we're going to build.
- Each target has a viewer (whose translation / access levels will be used to generate the mail).
- Each target has a to/cc list (the users who we'll ultimately send the mail to).
- For each target, build a custom mail body based on the viewer's access levels and settings (language prefs not actually implemented).
- Then, deliver the mail.
Test Plan:
- Read new config help.
Then did a bunch of testing, primarily with `bin/mail list-outbound` and `bin/mail show-outbound` (to review generated mail), `bin/phd debug taskmaster` (to run daemons freely) and `bin/worker execute --id <id>` (to repeatedly test a specific piece of code after identifying an issue).
With `one-mail-per-recipient` on (default):
- Sent mail to multiple users.
- Verified mail showed up in `mail list-outbound`.
- Examined mail with `mail show-outbound`.
- Added a project that a subscriber could not see.
- Verified it was not present in `X-Phabricator-Projects`.
- Verified it was rendered as "Restricted Project" for the non-permissioned viewer.
- Added a subscriber, then changed the object policy so they could not see it and sent mail.
- Verified I received mail but the other user did not.
- Enabled public replies and verified mail generated with public addresses.
- Disabld public replies and verified mail generated with private addresses.
With `one-mail-per-recipient` off:
- Verified that one mail is sent to all recipients.
- Verified users who can not see the object are still filtered.
- Verified that partially-visible projects are completely visible in the mail (this violates policies, as documented, as the best available compromise).
- Enabled public replies and verified the mail generated with "Reply To".
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: carlsverre, epriestley
Maniphest Tasks: T6367
Differential Revision: https://secure.phabricator.com/D13131
Summary:
Ref T6367. Do all mail, feed, notification and search stuff from the daemons, in all editors.
There are four relatively-stateful editors (Audit, Differential, Phriction, PhortuneCart) which needed special care to move state into the daemons properly.
Beyond that, I moved mailTo/mailCC/feedRelated/feedNotify to be computed before we enter the worker:
- This is simpler, since a lot of editors rely on being able to call `$object->getReviewers()` or similar to compute them.
- This is more correct, since we want to freeze the lists at this moment in time.
Finally, I renamed `loadEdges` to `willPublish` and made it a slightly more general hook.
---
This is a bit fragile and I'm not //thrilled// about it.
It would probably be cleaner to have separate Editor and Publisher classes (something like @fabe's D11329 did). However, I think that's quite a lot of work, and I'd like to see stronger motivation for it (either in this actually being more fragile than I think, or there being other things we get out of it). Overall, I'm comfortable with this change, just definitely not a big fan of the "save" + "load" pattern since I think it's really fragile, nonobvious, hard to debug/predict, etc.
Test Plan:
Directly updated editors:
- Created a new Phriction page, saw "Document Content".
- Edited a Phriction page, saw "Document Diff".
- Edited a revision, got normal looking mail.
- Faked in `changedPriorToCommitURI` and verified it survived the state boundary.
- Sent Audit mail.
- Sent invoice mail.
Indirect editors - for these, I just made a change and made sure the mail generated:
- Updated a paste.
- Updated an event.
- Updated a thread.
- Updated a task.
- Updated a mock.
- Updated a question.
- Updated a project.
- Updated a file.
- Updated an initiative.
- Updated a Legalpad document.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley, fabe
Maniphest Tasks: T6367
Differential Revision: https://secure.phabricator.com/D13115
Summary:
Ref T6367. This is similar to D11329, but not quite as ambitious.
Allow Editors to implement `supportsWorkers()` and move their publishing work into a daemon. So far, only Paste supports this.
Most of the complexity here is saving and restoring state across the barrier between the web process and the worker process, but I think this is ~90% of it and then we'll pick up a couple of random things in applications.
I'm primarily trying to keep this as gradual as possible.
Test Plan:
- Published transactions with and without daemon support.
- Looked at mail, feed.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: fabe, epriestley
Maniphest Tasks: T6367
Differential Revision: https://secure.phabricator.com/D13107
Summary: Fixes T8329. I was able to figure out a reasonable way to have the full conpherence default to the email settings panel. I think this is cleaner than making things a dialogue as I rambled about in the description for T8329.
Test Plan: using /bin/mail to verify correct email links were generated for conpherence notifications and maniphest (general) notifications.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T8329
Differential Revision: https://secure.phabricator.com/D13058
Summary: Fixes T6403. Remaining built-ins were already built-in effectively so this is a small re-factor plus some docs. I probably wouldn't have written anything if not for the TODO so please feel free to tell me to write something else or what have you.
Test Plan: NA since this didn't actually change anything.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T6403
Differential Revision: https://secure.phabricator.com/D12937
Summary: Ref XXXXX. I broke things a bit in XXXXX in that if the TYPE_EDGE had an inverse transaction, we weren't correctly "doing nothing" and instead were falling back to our old every editor has to implement a no-op ways... Fix things by putting the TYPE_EDGE code in the handle external builtin function like it belongs.
Test Plan: made a comment on a task referencng a commit successfully
Reviewers: epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T6403
Differential Revision: https://secure.phabricator.com/D12939
Summary: Ref T6403. This was actually simple stuff.
Test Plan: changed the edit policy of a paste. changed the edit and join policy of a phame blog.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T6403
Differential Revision: https://secure.phabricator.com/D12933
Summary: Ref T6403. Conpherence keeps track of comments for message counts so we needed some special attention there. Otherwise, straight-forward.
Test Plan: left a comment on a diff with inline comments. sent messages in conpherence successfully. verified unread count incremented correctly for sent messages for users.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T6403
Differential Revision: https://secure.phabricator.com/D12932
Summary: Ref T6403. This one was pretty easy since no one does anything custom with subscribers.
Test Plan: subscribed / unscribed to a random commit ("audit"). joined / left, watched / unwatched a project
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T6403
Differential Revision: https://secure.phabricator.com/D12930
Summary: Ref T6403. This does TYPE_EDGE since I just had to deal with T8252. Look like this fixes a few editors (maybe) that would have had fatals with mentions like slowvote and ponder.
Test Plan: made a phame post mentioning a task and it worked! joined / left a project, watched / unwatched a project and that worked! blind faith for other sites.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T6403
Differential Revision: https://secure.phabricator.com/D12929
Summary:
...which lets all the fancy settings for Email | Notify | Off be possible. Fixes T8164. Wasn't too sure the best way to break things up but members vs watchers felt meaningful to break out to me.
Also fixes a small bug where we were generating bad slug updated stories by messing with the signature of the slug data. Perhaps this fix isn't even good enough (the array_keys()) call and instead we'll need to implement transaction has effect and do a sort?
Test Plan: used ./bin/mail list-outbound and ./bin/mail show-outbound --id XX to verify reasonable emails were being generated. saw new preferences in settings.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T8164
Differential Revision: https://secure.phabricator.com/D12868
Summary: Use `__CLASS__` instead of hard-coding class names. Depends on D12605.
Test Plan: Eyeball it.
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: hach-que, Korvin, epriestley
Differential Revision: https://secure.phabricator.com/D12806
Summary:
Ref T7708.
This changes things to $viewer->loadHandles where applicable in the durable column render stack. I saw some big wins on my test data like 34 queries => 24 queries on a newly created room as my default thread.
For my test data, the next big perf win would be to change how remarkup rendering works and try to multiload all objects of a certain type in one shot.
e.g. `PhabricatorEmbedFileRemarkupRule` implements `loadObjects` as do all classes which inherit from `PhabricatorObjectRemarkupRule`. This is because `PhabricatorObjectRemarkupRule` implements its `didMarkupText` method using `loadObjects`, and `didMarkupText` gets called per transaction over in `PhabricatorMarkupEngine->process()`. Instead, the `loadObjects` in `didMarkupText` should be hitting some cache, and we should do a bulk load for all `PhabricatorEmbedFileRemarkupRule` that had matches earlier in the rendering stack. ...I think.
Test Plan: carefully looked at "Services" tab in dark console and noted fewer queries with changes post changes versus pre changes
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T7708
Differential Revision: https://secure.phabricator.com/D12780
Summary:
Ref T8021.
- When "All Day" events are loaded, convert them into the viewer's time.
- When "All Day" events are saved, convert them into a +24 hour range.
Test Plan:
- Created and updated "All Day" events.
- Created and updated normal events.
- Changed timezones, edited and viewed "All Day" events and normal events.
- In all cases, "All Day" events appeared to be 12:00AM - 11:59:59PM to the viewer, on the correct day.
- Normal events shifted around properly according to timezones.
Reviewers: lpriestley
Reviewed By: lpriestley
Subscribers: epriestley
Maniphest Tasks: T8021
Differential Revision: https://secure.phabricator.com/D12765
Summary: Closes T8023, Calendar event edit view should validate that start time preceeds end time .
Test Plan: Create Calendar event, add details, make end time be earlier than start time, try to save, get error, make sure all previously entered details are populated correctly.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: Korvin, epriestley
Maniphest Tasks: T8023
Differential Revision: https://secure.phabricator.com/D12656
Summary:
Ref T4100. Ref T5595.
To support a unified "Projects:" query across all applications, a future diff is going to add a set of "Edge Logic" capabilities to `PolicyAwareQuery` which write the required SELECT, JOIN, WHERE, HAVING and GROUP clauses for you.
With the addition of "Edge Logic", we'll have three systems which may need to build components of query claues: ordering/paging, customfields/applicationsearch, and edge logic.
For most clauses, queries don't currently call into the parent explicitly to get default components. I want to move more query construction logic up the class tree so it can be shared.
For most methods, this isn't a problem, but many subclasses define a `buildWhereClause()`. Make all such definitions protected and consistent.
This causes no behavioral changes.
Test Plan: Ran `arc unit --everything`, which does a pretty through job of verifying this statically.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: yelirekim, hach-que, epriestley
Maniphest Tasks: T4100, T5595
Differential Revision: https://secure.phabricator.com/D12453
Summary: Fixes T7756. This is the last little stray bit, though finishing T7757 also helps this feature IMO.
Test Plan: said "ZXX is the best" in comment on DXX and saw proper mention transaction on ZXX
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T7756
Differential Revision: https://secure.phabricator.com/D12405
Summary: Ref T7803. Remove these in favor of more generalized paging and ordering.
Test Plan: Sorted and paged results in various applications.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T7803
Differential Revision: https://secure.phabricator.com/D12378
Summary: This call got renamed but I missed the callsite.
Test Plan: No more fatal when viewing a custom policy from a transaction history page.
Reviewers: btrahan, chad, Mnkras
Reviewed By: chad, Mnkras
Subscribers: epriestley
Differential Revision: https://secure.phabricator.com/D12337
Summary:
Fixes T7731. When a user writes a "Send me an email" rule, always try send them an email, even if their notification settings would normally downgrade it to a notification.
In particular, this is stronger than these downgrades:
- Downgrades due to "self actions";
- downgrades due to "mail tags".
Test Plan:
- Wrote various Herald rules with "Send me an email" rules.
- Used `bin/mail list-outbound` / `show-outbound` to vet generated mail.
- Mail reacted properly to a variety of conditions (disabled accounts, settings, "send me an email" rule, forced delivery).
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T7731
Differential Revision: https://secure.phabricator.com/D12300
Summary: These arrays looks a little odd, most likely due to the autofix applied by `ArcanistXHPASTLinter::LINT_ARRAY_SEPARATOR`. See D12296 in which I attempt to improve the autocorrection from this linter rule.
Test Plan: N/A
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: epriestley
Differential Revision: https://secure.phabricator.com/D12281
Summary:
Ref T7199. This fully modularizes mail command handling in Maniphest.
I had to add a couple of minor not-totally-solid-feeling tricks to deal with the "create" case, but they feel not-too-bad, and a million times better than what came before.
Test Plan: Used all commands with `receive-test`.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T7199
Differential Revision: https://secure.phabricator.com/D12240
Summary: Ref T7199. Everyone can have a mail command! You can have a mail command! You can have a mail command! Mail commands for everyone!
Test Plan: Used `bin/mail receive-test` to issue commands against files and pastes.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T7199
Differential Revision: https://secure.phabricator.com/D12238
Summary:
Ref T7199. Essentially all of the reply handlers now apply transactions to something which implements PhabricatorApplicationTransactionInterface.
We can share code between them by lifting this stuff into a superclass.
First, convert paste. Also rename `PasteMockMailReceiver` to `PasteMailReceiver` (this got mis-copied from Pholio at some point, I think).
Test Plan: Used `bin/mail receive-test` to send comments + `!unsubscribe` to pastes.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T7199
Differential Revision: https://secure.phabricator.com/D12236
Summary:
Ref T7199. Although this is useful for discovery, it's un-useful enough that we already have an option to disable it, and most applications do not provide any meaningful instructions.
Throwing it away makes it easier to move forward and lets us get rid of a config option.
This is becoming a more advanced/power-user feature anyway, and the new syntax will be significantly more complex and hard to explain with a one-liner. I'm currently thinking that I'll maybe make the "help" menu a dropdown and give it some options like:
+---+
| O |
+---+---------------------+
| Maniphest Documentation |
| Maniphest Email Actions |
+-------------------------+
Then you click the "Email Actions" thing and get a runtime-derived list of available options. Not sure if I'll actually build that, but I think we can fairly throw the in-mail instructions away even if we don't go in that specific direction.
Test Plan: Grepped for `replyHandlerInstructions`, got no hits.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T7199
Differential Revision: https://secure.phabricator.com/D12229
Summary:
Ref T7585. This implements everything specified, with a few caveats
- since rooms you have yet to join can't be viewed in the column yet, the column view has some bugs and isn't expected to work.
- the room you're looking at is just pre-pending to the top of the "recent" list
Test Plan: made a room that no one could join. verified when viewing that there was no comment ui. made a room that others could join. verified folks who had yet to join had a "join" button with an area for text. tried joining with / without message text and it worked in both cases
Reviewers: chad, epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T7585
Differential Revision: https://secure.phabricator.com/D12149
Summary:
Ref T1460. Ref T6403. Replace `Diffusion::INLINEDONE` with `Transactions::INLINESTATE` and generalize things enough that we can lift it into core.
The next change will lift Differential's similar implementation into the core.
Also start implementing a fix for T6403, providing an alternate hook for optional builtin transactions.
Test Plan: Changed inline state in Diffusion.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T6403, T1460
Differential Revision: https://secure.phabricator.com/D12129
Summary:
Ref T2009. Ref T1460. The way Diffusion and Differential load inlines is horrible garbage right now:
- Differential does an ad-hoc query to get the PHIDs, then does a real load to policy check.
- Diffusion completely fakes things. In practice this is not a policy violation, but it's dangerous.
Make TransactionCommentQuery extensible so we can subclass it and get the query building correctly in the right Query layer.
Specifically, the Diffusion and Differential subclasses of this Query will add appropriate `withX()` methods to let us express the query in SQL.
Test Plan: Loaded, previewed, edited, and submitted inlines in Differential and Diffusion
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T2009, T1460
Differential Revision: https://secure.phabricator.com/D12026
Summary: This ended up having a different signature; the discrepancy can cause a warning.
Test Plan: No more warning.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Differential Revision: https://secure.phabricator.com/D11971
Summary: The participant list can sometimes be `null`, which fails when we try to `array_fuse()` it.
Test Plan: Created a new thread cleanly.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Differential Revision: https://secure.phabricator.com/D11969
Summary:
Ref T6516. Although this behavior is somewhat-arguable as desirable, I think it's less surprising and more consistent to disable mentions when a repository is publishing.
In particular, if you import a repository developed on another Phabricator install, this stops all the `T123` in commit messages from creating mentions on your unrelated `T123` tasks.
We already disable autoclose, so `Closes T123` and `Ref T123` already have no effect, but a bare `T123` would generate a mention. Likewise, `@epriestley` would generate a mention.
If you import such a repository and then update it periodically, updates will activate autoclose and publishing (if you didn't disable them), but presumably this will hit a couple of tasks and you'll go change the settings if you forgot.
At some point, we may have some kind of use case for separating the "publish" setting into a "publish" setting and a "this is a local repository" setting. For example, if you work at Widget Corp, want to import Phabricator locally, //and// want to write Herald rules against it, you can't currently configure the repository to let you do all of this. But we haven't actually seen a use case for this yet.
Test Plan:
- Pushed some commits with bare `T11`, saw mentions.
- Disabled publishing for the repository, pushed some commits with
- Imported a bunch of commits without seeing pipeline failures.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T6516
Differential Revision: https://secure.phabricator.com/D11966
Summary: Fixes T7232.
Test Plan: Saw "View Raw" gone for removed comment, still present and functional for non-removed comment.
Reviewers: btrahan, chad
Reviewed By: chad
Subscribers: epriestley
Maniphest Tasks: T7232
Differential Revision: https://secure.phabricator.com/D11926
Summary: Ref T7094. I am not sure when this text is legitimately exposed to users - they should be getting an error about not being able to see the object before they get an error about not being able to see a given transaction... That said, I think this text is logically correct at least.
Test Plan: read the text
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T7094
Differential Revision: https://secure.phabricator.com/D11632
Summary:
Ref T7094. We should do a policy query on the files IMO because there exists a scenario where the file gets locked down directly. This requires being a bit more disciplined about setting user, which in turn requires deciding whether or not to show edit / reply links as a separate piece of logic, not conditional on user presence.
This is not the best code but I don't think it gets worse with this and is just some other nuance in any larger cleanup we take on someday.
Test Plan: looked at a revision and noted inline comments rendered correctly with reply / edit actions. looked at a diff standalone and noted no reply / edit actions as expected. looked at a "details" link on a transaction and it rendered correctly. looked at a diff in phriction of page edits and it looked good. grepped around and verified the remaining callsite in diffusion already has the setUser call.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T7094
Differential Revision: https://secure.phabricator.com/D11579
Summary: Fixes T6819. This isn't as useful as you might think and has one horribly buggy behavior - if you edit an object which has a description and a projects field, you can be unable to remove the associated project as the automagic association from the description kicks in. Further, since we've added the ability for applications to create multiple email addresses AND herald can react to those emails - say by programmatically adding projects - the known needs for this feature are basically 0. If this proves to be false we can maybe add some other syntax for these mentions - see T6819 for ideas / discussion.
Test Plan: removed a project from a maniphest task while still mentioning it in the description and it worked!
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T6819
Differential Revision: https://secure.phabricator.com/D11573
Summary: due to typehints, passing null is going to barf here. Ref D11564, ref T5039.
Test Plan: made an edit to a task from the web ui and it didnt fatal
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T5039
Differential Revision: https://secure.phabricator.com/D11571
Summary:
Fixes T5039. The trick / possibly lame part here is we only match 1 application email and its undefined which one. e.g. if a user emails us at address x, y, and z only one of those will pick up the mail. Ergo, don't let users define non-sensical herald conditions like "matches all". Also document what I think was non-intuitive about the code with an inline comment; we have to return an array with just a phid from an object and out of context it feels very "what the...???"
Note this needs to be deployed to other applications still, but I think its okay to close T5039 aggressively here since its done from a user story perspective.
Test Plan: set up a herald rule to flag tasks created as blue via app email x. sent an email to x via `bin/mail receive-test` and verified the task had the blue flag
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T5039
Differential Revision: https://secure.phabricator.com/D11564
Summary: Removes the 1x application icons, and uses the fonticons instead. Feed was only known location.
Test Plan:
feed, dashboards, grep for use
{F275636}
Reviewers: btrahan, epriestley
Reviewed By: epriestley
Subscribers: hach-que, Korvin, epriestley
Differential Revision: https://secure.phabricator.com/D11496
Summary: Fixes T6989. Basically return a nice dialogue like we do for "NoEffect" transactions. This is a little prettier than the other dialogue was. Also, stop adding TYPE_EDGE as a transaction type as we end up having it 2x, which then makes the error get validated 2x.
Test Plan: tried to add myself as a reviewer and got a nice error message.
Reviewers: chad, epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T6989
Differential Revision: https://secure.phabricator.com/D11448
Summary: Ref T6962. Mainly accomplished by re-factoring the base editor `buildMailBody` function and then using it differently in the `DifferentialTransactionEditor`.
Test Plan: commented on a revision leaving inline feedback. inspected via bin/mail and it looked good! also made a maniphest comment and checked that email, which still looked good.
Reviewers: chad, epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T6962
Differential Revision: https://secure.phabricator.com/D11402
Summary: Fixes T6895, When viewing comment edit history, user should not see a dropdown for each comment edit transaction.
Test Plan: Edit task comment, view comment edit history, comment transactions should not provide a dropdown with action items.
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T6895
Differential Revision: https://secure.phabricator.com/D11355
Summary: Fixes T6594, Logged out users should be able to "View Raw" comments in public objects.
Test Plan: Log out, open maniphest task with comments, open dropdown associated with comment, click "View Raw", should be able to see raw comment.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: Korvin, epriestley
Maniphest Tasks: T6594
Differential Revision: https://secure.phabricator.com/D11295
Summary:
Ref T3165. Builds a dedicated index for Conpherence to avoid scale/policy filtering concerns.
- This is pretty one-off but I think it's generally OK.
- There's no UI for it.
- `ConpherenceFulltextQuery` is very low-level. You would need to do another query on the PHIDs it returns to actually show anything to the user.
- The `previousTransactionPHID` is so you can load chat context efficiently. Specifically, if you want to show results like this:
> previous line of context
> **line of chat that matches the query**
> next line of context
...you can read the previous lines out of `previousTransactionPHID` directly, and the next lines by issuing one query with `WHERE previousTransactionPHID IN (...)`.
I'm not 100% sure this is useful, but it seemed like a reasonable thing to provide, since there's no way to query this efficiently otherwise and I figure a lot of chat might make way more sense with a couple of lines of context.
Test Plan:
- Indexed a thread manually (whole thing indexed).
- Indexed a thread by updating it (just the new comment indexed).
- Wrote a hacky test script and got reasonable-looking query results.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T3165
Differential Revision: https://secure.phabricator.com/D11234
Summary: Modernize remaining edges to subclass `PhabricatorEdgeType`. Largely based on D11045.
Test Plan: Browsed around and performed various actions include subscribing, unsubscribing and watching.
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: epriestley
Differential Revision: https://secure.phabricator.com/D11116
Summary:
Removes an unused PhabricatorFeedStory Parameter from all getTitleForFeed() and getApplicationTransactionTitleForFeed() functions.
Ref D11088 Ref T6545
Test Plan: ran all unit tests and viewed some dashboard feeds
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T6545
Differential Revision: https://secure.phabricator.com/D11146
Summary:
Ref T4411
I'm not quite sure if this is the right place for this as it will be difficult to provide proper user feedback of why we removed a particular subscriber.
Is the ApplicationTransactionEditor generally the right place to extract mentioned phids in comments?
On the other hand in some cases we cannot really give user feedback why a user was not subscribed (e.g.: commits & diffs)
Adding a diff to a repo where the user mentioned has no view permissions the subscriber is currently still added. Still would have to find where this is donet...
Any other places?
Unrelated: Is there any way to remove a subscriber from a commit/audit ?
Test Plan:
- Edited tasks with the mentioned user having view permissions to this specific task and without
- Raised concern with a commit and commented on the audit with the user having view permissions to the repo and without
- Added a commit to a repo with and without the mentioned user having permissions
- Mention a user in a task & commit comment with and without permissions
- Mentioning a user in a diff description & comments with and without permissions to the specific diff
Reviewers: chad, #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: chad, Korvin, epriestley
Maniphest Tasks: T4411
Differential Revision: https://secure.phabricator.com/D11049
Summary:
Shows a timeline of all modified settings Fixes T6545
Will show all settings (no pagination, should be not so difficult to add if needed but most installs won't have hundreds of settings changes)
I'm not happy by how the PhabricatorConfigTransaction object is instructed to render the config keys but i don't see any other reasonable way.
We could always show the keys though.
Test Plan: Changed settings and called the history page
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T6545
Differential Revision: https://secure.phabricator.com/D11088
Summary: These classes are named differently from other `PhabricatorEdgeType` subclasses. Rename them for consistency.
Test Plan: I would expect the linter to complain if I missed anything.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: Korvin, epriestley
Differential Revision: https://secure.phabricator.com/D11112
Summary:
...except the transaction class itself, which still needs some knowledge of these transactions for older installs.
Ref T5245. T5604 and T5245 are now in a similar place -- there's an unknown set of bugs introduced from my changes and there's still old display code lying around with some old transactions in the database. I'll stomp out the bugs if / when they surface and data migration is up next.
This revision also adds a "TransactionPreviewString" method to the edge objects so that we can have a prettier "Bob edited associated projects." preview of this transaction.
Test Plan: added a project from task detail and saw correct preview throughout process with correct project added. bulk removed a project from some tasks. added a project from the edit details pane.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T5245
Differential Revision: https://secure.phabricator.com/D11013
Summary:
There's a comma to the lower-left of my profile picture here:
{F248962}
This is on a page like https://secure.phabricator.com/F248948
What's happening is that some `render()` method is returning a valid result like `array($stuff, null)`. This is getting passed to JS as an array, which is implicitly `join()`'ing it into a string, adding a comma.
Instead, make sure we render these to strings on the server side before shipping them to the client.
Test Plan: No more comma on file previews.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Differential Revision: https://secure.phabricator.com/D10974
Summary: Fixes T6731. I don't really understand the intent behind the two view classes here, but to get this to work I need to pass yet more data to the lower-level class.
Test Plan: Viewed a task with many comments. Clicked "show older". Quoted everything I could. Verified for each quote that it quoted correctly, inlcuding linking to the prior transaction.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T6731
Differential Revision: https://secure.phabricator.com/D10973
Summary: Fixes T6727. Repro is: mention a task on another task, in a comment.
The inverse edge editor applying the "alincoln mentioned this in <other task>" transaction doesn't have enough data to execute Herald rules.
Just don't try to execute the rules, since they don't make much sesne from a product perspective and are tricky from a technical perspective.
Test Plan: Commented on `T1` with `T2` in comment body and a Herald rule that examines subscribers.
Reviewers: btrahan
NOTE: Cowboy committing this since any task mention fatals.
Summary: Fixes T6694. Ref T4712. Turns out the logic here was slightly incorrect; we don't want to use the id of the last thing we hid but rather the first thing we show. I had garbage test data ("asdsadsadsa", etc) I guess so I didn't notice this.
Test Plan: made a new task where user a and user b alternated 3 comments each, cooperatively numbering them from 1 - 20. as both users, showed older transactions. pre-patch the issue described in T6694 occurred and post patch I saw the entire counting sequence.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T4712, T6694
Differential Revision: https://secure.phabricator.com/D10933
Summary: Fixes T6693.
Test Plan:
Made a bunch of comments on a diff with differential, being sure to leave inlines here and there. This reproduced the issue in T6693. With this patch this issue no longer reproduces!
Successfully "showed older changes" in Maniphest too.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T6693
Differential Revision: https://secure.phabricator.com/D10931
Summary: Only necessary for edits, only bother if the comment version is greater than 1. Ref T6690. This is another way to fix T6690 -- this check will never run since you can't edit a conpherence comment -- **but** the fix already applied should happen too to future proof Conpherence.
Test Plan: made a comment on a diff - success. edited the comment and mentions were generated.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T6690
Differential Revision: https://secure.phabricator.com/D10928
Summary: Fixes T6648. We do some automagical hotness based on the text you enter in remarkup textareas - e.g. adding projects or mentioning other objects. Refine the code here so that even when just editing a comment we build these transactions and apply them.
Test Plan: edited a comment and noted new mentions and projects showed up appropriately...!
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T6648
Differential Revision: https://secure.phabricator.com/D10922
Summary:
Ref T4712. Thus far, it seems that most "non-standard" things can be done pretty easily in the controller. Aside from deploying, this diff had to fix a few bugs / missing implementations of stuff.
(Notably, PhabricatorAuthProviderConfig, HeraldRule, PhabricatorSlowvotePoll, and AlmanacNetwork needed to implement PhabricatorApplicationTransactionInterface, PhabricatorAuthAuthProviderPHIDType had to be added, and a rendering bug in transactions of type PhabricatorOAuth2AuthProvider had to be fixed.)
Test Plan: Almanac - looked at binding, device, network, and service view controllers and verified timeline displayed properly. Herald - looked at a rule and verified timeline. Slowvote - looked at a vote and verified timeline. Auth - looked at an auth provider (Facebook) and verified proper display of transactions within timeline.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T4712
Differential Revision: https://secure.phabricator.com/D10921
Summary: Ref T4712. This adds pagination. Future diffs will need to deploy `buildTransactionTimeline` everywhere and massage this stuff as necessary if we hit any special cases.
Test Plan: Set page size to "5" to make it need to paginate often. Verified proper transactions loaded in and the javascript actions worked.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T4712
Differential Revision: https://secure.phabricator.com/D10887
Summary: See <https://phabricator.wikimedia.org/T906>. This behavior is a bug; we should remove the button if the user can't use the application.
Test Plan:
- With Macro uninstalled, did these things verifying the button vanished:
- Sent a user a message.
- Edited a revision.
- Edited repository basic information.
- Edited an initiative.
- Edited a Harbormaster build step.
- Added task comments.
- Edited profile blurb.
- Edited blog description.
- Commented on Pholio mock.
- Uploaded Pholio image.
- Edited Phortune merchant.
- Edited Phriction document.
- Edited Ponder answer.
- Edited Ponder question.
- Edited Slowvote poll.
- Edited a comment.
- Reinstalled Macro and saw button come back.
- Used button to put silly text on a funny picture.
Reviewers: btrahan, chad
Reviewed By: chad
Subscribers: epriestley
Differential Revision: https://secure.phabricator.com/D10900
Summary: Ref T1217, Add link to email preferences to email template
Test Plan: Add comment to object like Maniphest task, check that email has a footer with a link to email preferences.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: Korvin, epriestley
Maniphest Tasks: T1217
Differential Revision: https://secure.phabricator.com/D10883
Summary: Ref T6343, adding HTMLMailMode to remarkup, and most objects should now be processed and appear pretty in emails.
Test Plan: Add a comment to a Maniphest task containing a mention of an object like '{T1}' or 'T1'. Emails should show a styled version of the object similar to how the object looks in the context of the Maniphest task in the UI.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: chad, Korvin, epriestley
Maniphest Tasks: T6343, T2617
Differential Revision: https://secure.phabricator.com/D10859
Summary:
Ref T5833. Currently, we have an `AlmanacDeviceProperty`, but it doesn't use CustomFields and is specific to devices. Make this more generic:
- Reuse most of the CustomField infrastructure (so we can eventually get easy support for nice editor UIs, etc).
- Make properties more generic so Services, Bindings and Devices can all have them.
The major difference between this implementation and existing CustomField implementations is that all other implementations are application-authoritative: the application code determines what the available list of fields is.
I want Almanac to be a bit more freeform (basically: you can write whatever properties you want, and we'll put nice UIs on them if we have a nice UI available). For example, we might have some sort of "ServiceTemplate" that says "a database binding should usually have the fields 'writable', 'active', 'credential'", which would do things like offer these as options and put a nice UI on them, but you should also be able to write whatever other properties you want and add services without building a specific service template for them.
This involves a little bit of rule bending, but ends up pretty clean. We can adjust CustomField to accommodate this a bit more gracefully later on if it makes sense.
Test Plan: {F229172}
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T5833
Differential Revision: https://secure.phabricator.com/D10777
Summary: Fixes T6427.
Test Plan: Log out of sandbox, navigate to public task, click 'See Details' in a transaction. Get Dialog.
Reviewers: btrahan, epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T6427
Differential Revision: https://secure.phabricator.com/D10759
Summary: missing a setHandles on this codepath I think...? Fixes T6300.
Test Plan: not actually tested - I just think this is the fix since the other renderX methods all do this setHandles thing and I can't figure out how handles get set otherwise...
Reviewers: epriestley, avivey
Reviewed By: epriestley, avivey
Subscribers: avivey, Korvin, epriestley
Maniphest Tasks: T6300
Differential Revision: https://secure.phabricator.com/D10699
Summary: Fixes T6201. This stuff didn't fully get updated for ApplicationTransactions. Get it working again (notably, make inline comment text publish) and clean it up a little bit.
Test Plan:
- Published a Differential feed story into Asana with comment text.
- Pulbished a Diffusion feed story into Asana with comment text.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T6201
Differential Revision: https://secure.phabricator.com/D10584
Summary:
Ref T1191.
- Adds support for custom fields.
- Adds support for partial indexes (indexes on a prefix of a column).
- Drops old auxiliary storage table: this was moved to custom field storage about a year ago.
- Drops old project table: this was moved to edges about two months ago.
Test Plan:
- Viewed web UI, saw fewer issues.
- Used `grep` to verify no readers/writers for storage or project table.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T1191
Differential Revision: https://secure.phabricator.com/D10526
Summary: Ref T1191. This fills in some more features and gets audit and auth nearly generating reasonable expected schemata.
Test Plan: See screenshots.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T1191
Differential Revision: https://secure.phabricator.com/D10500
Summary: ...also re-jiggers all the anchor stuff to use $xaction ID. This seemed like the simplest way once I got in the code, as well as having nice properties for if / when we want to re-add some ajax stuff since the ID is a pretty solid piece of data to key off. Fixes T6083.
Test Plan: mentioned DX in private DX+1. Could see on DX the mention as me and not as the other user. For transactions, I left a comment on Paste and it worked, and I edited an existing transaction and it worked.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: epriestley, Korvin
Maniphest Tasks: T6083
Differential Revision: https://secure.phabricator.com/D10488
Summary: Fixes T6059.
Test Plan: Made a comment on TX mentioning TX and TX+1. TX did not get a "mentioned" transaction while TX+1 did.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: epriestley, Korvin
Maniphest Tasks: T6059
Differential Revision: https://secure.phabricator.com/D10464
Summary: Some versions of PHP aren't very happy about both interfaces and superclasses defining a method. Just remove it from MentionableInterface and leave it as implicit.
Auditors: btrahan
Summary: Fixes T4036. Now if you say something on diff X like "This reminds me of Tx and Dy and commitHashFoo and Px." each of those objects gets a little visible transaction that the mention occurred. No feed, email, or notifications.
Test Plan: made a comment like above and verified transactions. also submitted a diff that "Fixes Tx" and Tx did not get the transaction as expected.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: joshuaspence, epriestley, Korvin
Maniphest Tasks: T4036
Differential Revision: https://secure.phabricator.com/D10451
Summary: Fixes T6037. We don't currently write the "this file is attached to such-and-such object" edge on comment edits.
Test Plan: Edited a comment, adding `{Fnnn}`. Verified file was not attached before the edit, but was afterward.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T6037
Differential Revision: https://secure.phabricator.com/D10423
Summary:
Fixes T4057. This sort of sidesteps the trickiest (but very rare) case of things like embedded slowvotes. We might be able to refine that later.
In the common bad case (macros, large images) it gets reasonable results by using `overflow: hidden` with `max-height`.
We use `PhabriatorMarkupEngine::summarize()` to try to just render the first paragraph.
Test Plan: {F195093}
Reviewers: chad, btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T4057
Differential Revision: https://secure.phabricator.com/D10355
Summary: its not necessary. Fixes T5906
Test Plan: clicked "Login to Comment" and went straight to the login form, sans intermediary dialogue
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: epriestley, Korvin
Maniphest Tasks: T5906
Differential Revision: https://secure.phabricator.com/D10295
Summary:
Added support for side-by-side HTML and plaintext email building.
We can control if the HTML stuff is sent by by a new config, metamta.html-emails
Test Plan:
Been running this in our deployment for a few months now.
====Well behaved clients====
- Gmail
- Mail.app
====Bad clients====
- [[ http://airmailapp.com/ | Airmail ]]. They confuse Gmail too, though.
====Need testing====
- Outlook (Windows + Mac)
Reviewers: chad, #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: webframp, taoqiping, chad, epriestley, Korvin
Maniphest Tasks: T992
Differential Revision: https://secure.phabricator.com/D9375
Summary: Should help with debugging. In the web UI there is now a link to "View Email". This uses the same debugging output that `./bin/mail show-outbound` uses. Fixes T5768. Code is very defensive as I think these tables may be truncated eventually?
Test Plan: viewed some emails and it worked!
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: epriestley, Korvin
Maniphest Tasks: T5768
Differential Revision: https://secure.phabricator.com/D10244
Summary: Fixes T4973. For `PhabricatorProjectInterface` objects, add a header to let clients do mail filtering.
Test Plan: Saw `X-Phabricator-Projects: <#goat_farm>` in outbound mail.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: rush898, epriestley
Maniphest Tasks: T4973
Differential Revision: https://secure.phabricator.com/D10256
Summary:
Ref T5861. Ref T5769. If users don't care at all about something, allow them to ignore it.
We have some higher-volume notifications either built now (column changes) or coming (mentions) which users might reasonably want to ignore completely.
Test Plan:
Ignored some notifications, then took appropraite actions. Saw my user culled from the notification subscriber list.
{F189531}
Reviewers: chad, btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T5769, T5861
Differential Revision: https://secure.phabricator.com/D10240
Summary:
Ref T5861. Currently, mail tags are hard-coded; move them into applications. Each Editor defines its own tags.
This has zero impact on the UI or behavior.
Test Plan:
- Checked/unchecked some options, saved form.
- Swapped back to `master` and saw exactly the same values.
Reviewers: chad, btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T5861
Differential Revision: https://secure.phabricator.com/D10238
Summary:
Fixes T5456. We lost this logic in the transition to applicationtransactions.
When publishing a feed story, mark all of the object's projects as related, so the project filter in feed works.
Test Plan: Made a comment on a task associated with a project, saw the story in filtered feed.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: timor, epriestley
Maniphest Tasks: T5456
Differential Revision: https://secure.phabricator.com/D10233
Summary:
- Fixes T5851. Currently, if a commit has `Fixes T123`, we generate an email with just that before generating the commit email. Don't send/publish transactions about a commit before it imports (this is a tiny bit hacky, but well-contained and I don't think it causes any problems).
- Fixes T4864. Currently, we try to parse Differential information even if Differential is not installed. Instead, do this only if Differential is installed.
- Fixes T5771. Currently, if we can't figure out who the committer/author of a commit is, we don't publish a `Fixes T123` transaction. Instead, fall back to acting as "Diffusion" if we can't find a better actor. Most of this diff expands the role of application actors. The existing application actors (Herald and Harbormaster) seem to be working well.
Test Plan:
- Pushed a commit with `Fixes T123` and verified it did not generate email directly. (The task half of the transaction still does, correctly.)
- Uninstalled Differential and pushed a commit, got a clean import instead of an exception.
- Commented out author/committer PHIDs and pushed stuff, saw a "Diffusion" actor.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T5771, T4864, T5851
Differential Revision: https://secure.phabricator.com/D10221
Summary: Sets layout as flush when rendering diff table or timeline in a Dialog
Test Plan: Tested each
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: epriestley, Korvin
Differential Revision: https://secure.phabricator.com/D10194
Summary:
Ref T5245. This removes some hacks and activates two meaningful interactions:
- The "projects" field goes through shared code now.
- Mentioning projects in tasks using hashtags now tags them.
Test Plan:
- Viewed a task with projects.
- Viewed a task with no projects.
- Viewed a task with projects and board positions.
- Viewed a revision with projects.
- Made a `#hashtag` comment in Maniphest and got a project association.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T5245
Differential Revision: https://secure.phabricator.com/D10177
Summary: Use cutlery icon for hilarity. Ref T5768.
Test Plan: made something with remarkup in it, used 'view raw' and saw the remarkup raw in a nice little dialogue.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: epriestley, Korvin
Maniphest Tasks: T5768
Differential Revision: https://secure.phabricator.com/D10183
Summary: Ref T4896. Instead of using custom stuff, use standard stuff.
Test Plan: Viewed a bunch of feed stories and published some over the Asana bridge.
Reviewers: joshuaspence, btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T4896
Differential Revision: https://secure.phabricator.com/D10114
Summary:
Ref T4896. Replaces more custom stuff with standard stuff. In particular:
- No more fake proxy writes;
- no more fake detection of `@mentions`.
For now, the old code still applies most of the effects and handles feed and email.
Test Plan:
- Added comments.
- Added comments with inline comments.
- Added just inline comments.
- Added comments with Conduit.
- Previewed comments.
- Added CCs explicitly and with `@mentions`.
- Added auditors.
- Accepted a commit.
Reviewers: joshuaspence, btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T4896
Differential Revision: https://secure.phabricator.com/D10109
Summary: Ref T4896. Depends on D10055. This uses core rendering stuff for audit comments, and fixes all the wonkiness with inlines so we can actually land the migration.
Test Plan: Viewed, previewed and edited various types of comments in Diffusion.
Reviewers: chad, btrahan, joshuaspence
Reviewed By: joshuaspence
Subscribers: epriestley
Maniphest Tasks: T4896
Differential Revision: https://secure.phabricator.com/D10056
Summary: Ref T5655. Rename `PhabricatorPHIDType` subclasses for clarity (see discussion in D9839). I'm not too keen on some of the resulting class names, so feel free to suggest alternatives.
Test Plan: Ran unit tests.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: epriestley, Korvin, hach-que
Maniphest Tasks: T5655
Differential Revision: https://secure.phabricator.com/D9986
Summary: Provide an implementation for the `getName` method rather than automagically determining the application name.
Test Plan: Saw reasonable application names in the launcher.
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: epriestley, Korvin
Differential Revision: https://secure.phabricator.com/D10027
Summary: Ref T5655. Some discussion in D9839. Generally speaking, `Phabricator{$name}Application` is clearer than `PhabricatorApplication{$name}`.
Test Plan:
# Pinned and uninstalled some applications.
# Applied patch and performed migrations.
# Verified that the pinned applications were still pinned and that the uninstalled applications were still uninstalled.
# Performed a sanity check on the database contents.
Reviewers: btrahan, epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: hach-que, epriestley, Korvin
Maniphest Tasks: T5655
Differential Revision: https://secure.phabricator.com/D9982
Summary: Instead of implementing the `getTypeConstant` method in all subclasses of `PhabricatorPHIDType`, provide a `final` implementation in the base class which uses reflection. See D9837 for a similar implementation.
Test Plan: Ran `arc unit`.
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: epriestley, Korvin, hach-que
Differential Revision: https://secure.phabricator.com/D9985
Summary: Ref T5655. The `PhabricatorDestructibleInterface` interface is misspelled as `PhabricatorDestructableInterface`. Fix the spelling mistake.
Test Plan: `grep`. Seeing as this interface is fairly recent, I don't expect that this would cause any widespread breakages.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: epriestley, Korvin
Maniphest Tasks: T5655
Differential Revision: https://secure.phabricator.com/D9988
Summary: These got removed recently but I missed one callsite.
Test Plan: Used `git grep` to double check all other callsites.
Reviewers: joshuaspence
Reviewed By: joshuaspence
Subscribers: epriestley
Differential Revision: https://secure.phabricator.com/D9973
Summary: Ref T5245. Updates the project/object edge to use a modern class definition. Moves further toward real edges.
Test Plan: Added projects to some objects, viewed transactions in transaction record.
Reviewers: chad, btrahan, joshuaspence
Reviewed By: joshuaspence
Subscribers: epriestley
Maniphest Tasks: T5245
Differential Revision: https://secure.phabricator.com/D9849
Summary:
Ref T5245. This hooks up the translation/rendering methods added previously.
These are messy, but now extractable/translatable.
Test Plan: Viewed edge transactions and stories, saw correct strings.
Reviewers: chad, btrahan, joshuaspence
Reviewed By: joshuaspence
Subscribers: epriestley
Maniphest Tasks: T5245
Differential Revision: https://secure.phabricator.com/D9841
Summary:
Ref T5245. These were a bad idea.
We no longer need actors for edge edits either, so remove those. Generally, edges have fit into the policy model as pure/low-level infrastructure, and they do not have any policy or capability information in and of themselves.
Test Plan: `grep`
Reviewers: chad, btrahan, joshuaspence
Reviewed By: joshuaspence
Subscribers: epriestley
Maniphest Tasks: T5245
Differential Revision: https://secure.phabricator.com/D9840
Summary:
Ref T5245. See some discussion in D9838.
When we attach object A to object B, we'd like to write transactions on both sides but only write the actual edges once.
To do this, allow edge types to `shouldWriteInverseTransactions()`. When an edge type opts into this, have editors apply the inverse transactions before writing the edge. These inverse transactions don't actually apply effects, they just show up in the transaction log.
Test Plan: Attached and detached revisions from tasks, saw transactions appear on both sides of the operation.
Reviewers: chad, btrahan, joshuaspence
Reviewed By: btrahan, joshuaspence
Subscribers: epriestley
Maniphest Tasks: T5245
Differential Revision: https://secure.phabricator.com/D9839
Summary:
This was significantly easier than expected. Here's an example of what an extension class might look like:
```
<?php
final class AddRiskReviewHeraldCustomAction extends HeraldCustomAction {
public function appliesToAdapter(HeraldAdapter $adapter) {
return $adapter instanceof HeraldDifferentialRevisionAdapter;
}
public function appliesToRuleType($rule_type) {
return $rule_type == HeraldRuleTypeConfig::RULE_TYPE_GLOBAL ||
$rule_type == HeraldRuleTypeConfig::RULE_TYPE_OBJECT;
}
public function getActionKey() {
return 'custom:add-risk';
}
public function getActionName() {
return 'Add risk rating (JSON)';
}
public function getActionType() {
return HeraldAdapter::VALUE_TEXT;
}
public function applyEffect(
HeraldAdapter $adapter,
$object,
HeraldEffect $effect) {
$key = "phragile:risk-rating";
// Read existing value.
$field_list = PhabricatorCustomField::getObjectFields(
$object,
PhabricatorCustomField::ROLE_VIEW);
$field_list->readFieldsFromStorage($object);
$field_list = mpull($field_list->getFields(), null, 'getFieldKey');
$field = $field_list[$key];
$field->setObject($object);
$field->setViewer(PhabricatorUser::getOmnipotentUser());
$risk = $field->getValue();
$old_risk = $risk; // PHP copies arrays by default!
// Add new value to array.
$herald_args = phutil_json_decode($effect->getTarget());
$risk[$herald_args['key']] = array(
'value' => $herald_args['value'],
'reason' => $herald_args['reason']);
$risk_key = $herald_args['key'];
// Set new value.
$adapter->queueTransaction(
id(new DifferentialTransaction())
->setTransactionType(PhabricatorTransactions::TYPE_CUSTOMFIELD)
->setMetadataValue('customfield:key', $key)
->setOldValue($old_risk)
->setNewValue($risk));
return new HeraldApplyTranscript(
$effect,
true,
pht(
'Modifying automatic risk ratings (key: %s)!',
$risk_key));
}
}
```
Test Plan: Created a custom action for differential revisions, set up a Herald rule to match and trigger the custom action, did 'arc diff' and saw the action trigger in the transcripts.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: locutus, edutibau, ite-klass, epriestley, Korvin
Maniphest Tasks: T4884
Differential Revision: https://secure.phabricator.com/D8784
Summary:
Fixes T5489. Currently, if you make a `#proj` comment on an object already tagged with `#proj`, you get a "no effect" dialog.
Instead, continue if these transactions produce no effect (this is normal/expected, and consistent with `@user`).
Test Plan: Made two `#proj` comments in a row on a revision.
Reviewers: joshuaspence, chad
Reviewed By: chad
Subscribers: epriestley
Maniphest Tasks: T5489
Differential Revision: https://secure.phabricator.com/D9745
Summary:
Fixes T5424.
- One concrete issue: drafts were not being cleared properly because `__draft__` was not set on submission. This (mostly) fixes phantom drafts.
- This ajax comment magic feels weird and floaty and generally has problems. For example, if you add subscribers or inlines, all the stuff on the page which represents those won't update automatically. Instead, just reload. Maybe we'll ajax this stuff some day, but it feels like a net negative for now.
- Also remove it from other applications where it's currently used.
- Fix an issue with inline previews.
Test Plan: Made some comments on a mock, everything worked normally like I expected it to.
Reviewers: chad
Reviewed By: chad
Subscribers: epriestley
Maniphest Tasks: T5424
Differential Revision: https://secure.phabricator.com/D9649
Summary: Ran `arc lint --apply-patches --everything` over rP, mainly to change double quotes to single quotes where appropriate. These changes also validate that the `ArcanistXHPASTLinter::LINT_DOUBLE_QUOTE` rule is working as expected.
Test Plan: Eyeballed it.
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: epriestley, Korvin, hach-que
Differential Revision: https://secure.phabricator.com/D9431
Summary: Ref T2628. This makes Transactions understand objects that can have project relationships, extract project mentions, and handle watching.
Test Plan: See next diff.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T2628
Differential Revision: https://secure.phabricator.com/D9340
Summary:
This does some backend cleanup of the tile stuff, and some general cleanup of other application things:
- Users who haven't customized preferences get a small, specific set of pinned applications: Differential, Maniphest, Diffusion, Audit, Phriction, Projects (and, for administrators, Auth, Config and People).
- Old tile size methods are replaced with `isPinnnedByDefault()`.
- Shortened some short descriptions.
- `shouldAppearInLaunchView()` replaced by less ambiguous `isLaunchable()`.
- Added a marker for third-party / extension applications.
Test Plan: Faked away my preferences and viewed the home page, saw a smaller set of default pins.
Reviewers: chad
Reviewed By: chad
Subscribers: epriestley
Differential Revision: https://secure.phabricator.com/D9358
Summary:
Ref T4967. Adds a "Watch" relationship to projects, which is stronger than member/subscribed.
Specifically, when a task is tagged with a project, we'll include all project watchers in the email/notifications. Normally we don't include projects unless they're explicitly CC'd, or have some other active role in the object (like being a reviewer or auditor).
This allows you to closely follow a project without needing to write a Herald rule for every project you care about.
Test Plan:
- Watched/unwatched a project.
- Tested the watch/subscribe/member relationships:
- Watching implies subscribe.
- Joining implies subscribe.
- Leaving implies unsubscribe + unwatch.
- You can't unsubscribe until you unwatch (slightly better would be unsubscribe implies unwatch, but this is a bit tricky).
- Watched a project, then recevied email about a tagged task without otherwise being involved.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T4967
Differential Revision: https://secure.phabricator.com/D9185
Summary: Fixes T4930. We currently show the edit/quote menu if a transaction group has //inline// comments, but this doesn't make sense and doesn't work properly. Only show this menu if the group has a normal comment.
Test Plan:
Viewed these groups:
- Normal comment (edits fine).
- Just inlines (no more edit menu).
- Inline + comment (edits fine, affects the normal comment properly).
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: bitglue, epriestley
Maniphest Tasks: T4930
Differential Revision: https://secure.phabricator.com/D9180
Summary:
Ref T4968. If you add a comment to revision you aren't currently subscribed to, the email currently chooses "[Changed Subscribers]" as the action title. This is less interesting than "[Commented]", provided the affected subscriber is you (adding other people //is// usually interesting).
In this case, reduce the strength of this action below the strength of "comment".
Test Plan: Made several comments in conjunction with implicit and explicit subscriptions. Saw "[Commented]" for stuff affecting me, and "[Changed Subscribers]" for stuff affecting others.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T4968
Differential Revision: https://secure.phabricator.com/D9168
Summary:
Create transaction, editor, etc, and move command generation over to editor.
Show in a timeline in the buildable page.
Also prevent Engine from creating an empty transaction when build starts (Fixes T4885).
Fixes T4886.
Test Plan: Restart builds and buildables, look at timeline.
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: epriestley, Korvin
Maniphest Tasks: T4885, T4886
Differential Revision: https://secure.phabricator.com/D9110
Summary: The removes the sprite sheet 'icons' and replaces it with FontAwesome fonts.
Test Plan:
- Grep for SPRITE_ICONS and replace
- Grep for sprite-icons and replace
- Grep for PhabricatorActionList and choose all new icons
- Grep for Crumbs and fix icons
- Test/Replace PHUIList Icon support
- Test/Replace ObjectList Icon support (foot, epoch, etc)
- Browse as many pages as I could get to
- Remove sprite-icons and move remarkup to own sheet
- Review this diff in Differential
Reviewers: btrahan, epriestley
Reviewed By: epriestley
Subscribers: epriestley, Korvin, hach-que
Differential Revision: https://secure.phabricator.com/D9052
Summary:
Ref T4119.
- Trim newlines off the quoted text before quoting it; otherwise we can end up with a staircase of ">" at the end of a quote.
- Allow image macros to have leading whitespace, so multiple consecuitive quoted macros work properly.
Test Plan: I QUOTED MACROS A LOT OF TIMES
Reviewers: btrahan, chad
Reviewed By: chad
Subscribers: epriestley
Maniphest Tasks: T4119
Differential Revision: https://secure.phabricator.com/D8983
Summary:
man I sure hate Javascript
I removed the ajax-edit and ajax-remove interactions, becuase they were prohibitively complex to get working given that the entire menu has to change too. Instead, the page just reloads. This works perfectly fine in practice.
If we want to restore these in the future, we should have the server re-render the entire transaction group or something. I think very little is lost here, though.
Test Plan:
- Took all the actions.
- Used existing dropdown menus.
{F150196}
Reviewers: chad, btrahan
Reviewed By: btrahan
Subscribers: epriestley
Differential Revision: https://secure.phabricator.com/D8966
Summary:
Ref T4119. This is ugly for now, but technically works.
The comment area and transaction log don't realy know about each other, so for the moment the linking is a bit manual. Differential/Maniphest are special cases anyway.
Test Plan: {F149992}
Reviewers: chad, btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T4119
Differential Revision: https://secure.phabricator.com/D8957
Summary:
Fixes T4909. Adds a "remove" link next to the edit link, which permanently hides a comment. Addresses two use cases:
- Allowing administrators to clean up spam.
- Allowing users to try to put the genie back in the bottle if they post passwords or sensitive links, etc.
The user who removed the comment is named in the removal text to enforce some level of administrative accountability.
No data is deleted, but there's currently no method to restore these comments. We'll see if we need one.
This is cheating a little bit by storing "removed" as "2" in the isDeleted field. This doesn't seem tooooo bad for now.
Test Plan:
- Removed some of my comments.
- As an administrator, removed other users' comments.
- Failed to view history of a removed comment.
- Failed to edit a removed comment.
- Failed to remove a removed comment.
- Verified feed doesn't show the old comment after comment removal.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: qgil, chad, epriestley
Maniphest Tasks: T4909
Differential Revision: https://secure.phabricator.com/D8945
Summary:
tracing this a bit, in core/behavior-form.js we end up doing form.submit() on this form. Before this patch, the form had no submitURI set so it ended up doing a page load. Now, we set the submitURI and (at least) the form is actually submitted and data is saved. Ref T4669.
Not sure how come the syntheticSubmit even isn't getting picked up though?
Test Plan: hit apple + enter to submit comment edit. page reloaded and my edit persisted!
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: epriestley, Korvin
Maniphest Tasks: T4669
Differential Revision: https://secure.phabricator.com/D8952
Summary: These stories/notifications aren't too useful, just turn them off at least for now.
Test Plan: Will vet this in a sec...
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Differential Revision: https://secure.phabricator.com/D8899
Summary: Fixes T4919. There's some special casing in Diffusion for CAN_PUSH right now, just accommodate that until things get more general.
Test Plan: Viewed a repository edit screen with a custom policy transaction. Clicked the link to view it.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T4919
Differential Revision: https://secure.phabricator.com/D8898
Summary: 'cuz those can be complicated. Fixes T4738. I needed to do a fair amount of heavy lifting to get the policy stuff rendering correctly. For now, I made this end point very one purpose and tried to make that clear.
Test Plan: looked at some custom policies. see screenshots.
Reviewers: chad, epriestley
Reviewed By: epriestley
Subscribers: epriestley, Korvin
Maniphest Tasks: T4738
Differential Revision: https://secure.phabricator.com/D8890
Summary:
In some applications, using `{V2}` syntax to embed a vote throws. The chain of causality looks like this:
- We try to render a `phabricator_form()`.
- This requires a CSRF token.
- We look for a CSRF token on the user.
- It's an omnipotent user with no token, so everything fails.
To resolve this, make sure we always pass the real user in.
Test Plan:
- Lots of `grep`.
- Made a Differential comment with `{V2}`.
- Made a Diffusion comment with `{V2}`.
- Made a Maniphest comment with `{V2}`.
- Replied to a Conpherence thread with `{V2}`.
- Created a Conpherence thread with `{V2}`.
- Used Conduit to update a Conpherence thread with `{V2}`.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley, lkassianik
Differential Revision: https://secure.phabricator.com/D8849
Summary: Throwing this up for testing, swapped out all icons in timeline for their font equivelants. Used better icons where I could as well. We should feel free to use more / be fun with the icons when possible since there is no penalty anymore.
Test Plan: I browsed many, not all, timelines in my sandbox and in IE8. Some of these were just swagged, but I'm expecting we'll do more SB testing before landing.
Reviewers: btrahan, epriestley
Reviewed By: epriestley
Subscribers: epriestley, Korvin
Differential Revision: https://secure.phabricator.com/D8827
Summary: The token transactions can publish empty transaction feed stories.
Stop them from doing that, and make notifications fail more quietly.
Auditors: btrahan
Summary:
Ref T1049. When Harbormaster tests pass, don't bother sending an email about it.
(I tried to implement this earlier but didn't test it entirely properly, and we needed a little more code.)
Test Plan: Used `bin/harbormaster build` to build some junk, got no email about passes.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T1049
Differential Revision: https://secure.phabricator.com/D8813
Summary:
Fixes T4810. When a buildable completes, make an effort to update the corresponding object with a success or failure message. Commits don't support this yet, but revisions do.
{F144614}
Test Plan:
- Used `bin/harbormaster build` and `bin/harbormaster update` to run a pile of builds.
- Tried good/bad builds.
- Sent some normal mail to make sure the mail reentrancy change didn't break stuff.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T4810
Differential Revision: https://secure.phabricator.com/D8803
Summary:
Ref T4810. Ultimate goal is to let Harbormaster post a "build passed/failed" transaction. To prepare for that, implement `PhabricatorApplicationTransactionInterface` in Differential.
To allow Harbormaster to take action on //diffs// but have the transactions apply to //revisions//, I added a new method so that objects can redirect transactions to some other object.
Test Plan:
- Subscribed/unsubscribed/attached/detached from Differential, saw transactions appear properly.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T4810
Differential Revision: https://secure.phabricator.com/D8802
Summary: See IRC. Some users are having difficulty figuring out why Herald is taking some actions. Make it easier to get to the transcript.
Test Plan: {F144622}
Reviewers: btrahan, chad
Reviewed By: chad
Subscribers: dctrwatson, epriestley
Differential Revision: https://secure.phabricator.com/D8804
Summary:
Couple of minor cleanup things here:
- Pass handles to ApplicationTransactions when rendering their stories; this happened implicitly before but doesn't now.
- Add `?text=1` to do ad-hoc rendering of a story in text mode.
- Make Conduit skip unrenderable stories.
- Fix/modernize some text in the Commit story.
Test Plan: Rendered text versions of stories via Conduit and `?text=1`.
Reviewers: btrahan, chad
Reviewed By: chad
Subscribers: zeeg, spicyj, epriestley
Differential Revision: https://secure.phabricator.com/D8793
Summary: ...add a "renderingTarget" to FeedStory and use it as appropos. Overall, not a ton of changes was necessary to make this work. I think this could be made to be even cleaner by going through each and every feed story and re-implementing as necessary with the full toolset available. But this is good enough for now I think, and just something to keep cleaning up when we're in here. Fixes T4630.
Test Plan: made a task. gave it a token. viewed my feed - saw stories. used conduit.feed.query with mode == 'text' and got good readable results.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: spicyj, epriestley, Korvin
Maniphest Tasks: T4630
Differential Revision: https://secure.phabricator.com/D8750
Summary: I also changed PhabricatorApplicationTransactionFeedStory and the TokenGivenFeedStory to include only the title/first line of the feed story, which is more convenient (previously, strip_tags gave a multi-line story without even any linebreaks) and more consistent with the other story types.
Test Plan: Added a requestbin URL to feed.http-hooks, commented on a Differential, and saw storyText equal to "alpert added a comment to D2: c." in the POST data it received.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: epriestley, Korvin
Maniphest Tasks: T4630
Differential Revision: https://secure.phabricator.com/D8710
Summary: ...the key is to move a layer lower and beam down the updated comment. There is a wee bit of Javascript gymnastics going on here. Fixes T4608.
Test Plan: made a comment + resolve. clicked edit and made changes. noted transaction updated correctly and "history" link worked. edited again to a deletion and noted the "this is deleted" looked right and history link still worked
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: epriestley, Korvin, chad
Maniphest Tasks: T4608
Differential Revision: https://secure.phabricator.com/D8702
