Summary:
Depends on D19858. Ref T13222. See PHI995. In D19635 and related revisions, inline behavior changed to allow you to pre-mark your own inlines as done (as a reviewer) and to pre-mark your inlines for you (as an author).
These actions generate low-value stories in the timeline, like "alice marked 3 comments done." when an author adds some notes to their own revision. These aren't helpful and can be a little misleading.
Instead, just don't count it when someone marks their own inlines as "done". If we throw away all the marks after throwing away the self-marks, hide the whole story.
This happens in three cases:
# You comment on your own revision, and don't uncheck the "Done" checkbox.
# You comment on someone else's revision, and check the "Done" checkbox before submitting.
# You leave a not-"Done" inline on your own revision, then "Done" it later.
Cases (1) and (2) seem unambiguously good/clear. Case (3) is a little more questionable, but I think this still isn't very useful for reviewers.
If there's still a clarity issue around case (3), we could change the story text to "alice marked 3 inline comments by other users as done.", but I think this is probably needlessly verbose and that no one will be confused by the behavior as written here.
(Also note that this story is never shown in feed.)
Test Plan: Created and marked a bunch of inlines as "Done" in Differential and Diffusion, as the author and reviewer/auditor. My own marks didn't generate timeline stories; marking others' comments still does.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13222
Differential Revision: https://secure.phabricator.com/D19859
Summary:
Ref T13222. See PHI995. Before making a change to inline rendering, consolidate this code for generating the "alice added inlines comments." and "alice marked X inlines as done." transactions.
Both Differential and Diffusion have four very similar chunks of code. Merge them into shared methods and reduce code duplication across the methods.
(In the next change, I plan to hide the "done" story when the mark affects your own inline, since users marking their own inlines as "done" is generally not very interesting or useful.)
Test Plan: As author and reviewer/auditor, added inlines in Differential and Diffusion. As author, marked own and others inlines as done and undone. Got sensible transaction rendering and persistence of "Done".
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13222
Differential Revision: https://secure.phabricator.com/D19858
Summary:
Ref T13222. Fixes T12588. See PHI683. In several cases, we present the user with a choice between multiple major options: Alamnac service types, Drydock blueprint types, Repository VCS types, Herald rule types, etc.
Today, we generally do this with radio buttons and a "Submit" button. This isn't terrible, but often it means users have to click twice (once on the radio; once on submit) when a single click would be sufficient. The radio click target can also be small.
In other cases, we have a container with a link and we'd like to link the entire container: notifications, the `/drydock/` console, etc. We'd like to just link the entire container, but this causes some problems:
- It's not legal to link block eleements like `<a><div> ... </div></a>` and some browsers actually get upset about it.
- We can `<a><span> ... </span></a>` instead, then turn the `<span>` into a block element with CSS -- and this sometimes works, but also has some drawbacks:
- It's not great to do that for screenreaders, since the readable text in the link isn't necessarily very meaningful.
- We can't have any other links inside the element (e.g., details or documentation).
- We can `<form><button> ... </button></form>` instead, but this has its own set of problems:
- You can't right-click to interact with a button in the same way you can with a link.
- Also not great for screenreaders.
Instead, try adding a `linked-container` behavior which just means "when users click this element, pretend they clicked the first link inside it".
This gives us natural HTML (real, legal HTML with actual `<a>` tags) and good screenreader behavior, but allows the effective link target to be visually larger than just the link.
If no issues crop up with this, I'd plan to eventually use this technique in more places (Repositories, Herald, Almanac, Drydock, Notifications menu, etc).
Test Plan:
{F6053035}
- Left-clicked and command-left-clicked the new JS fanciness, got sensible behaviors.
Reviewers: amckinley
Reviewed By: amckinley
Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam
Maniphest Tasks: T13222, T12588
Differential Revision: https://secure.phabricator.com/D19855
Summary:
Ref T13222. Ref T12588. See PHI683. After D19853, "Create Subtask" may pop a dialog to let you choose between multiple forms.
Allow users to configure which forms are available by using `maniphest.subtypes` to choose available children for each subtype. Users may either specify particular subtypes or specific forms.
Test Plan: Configured "Quest" tasks to have "Objective" children, got appropriate prompt behavior. Used "subtypes" and "forms" to select forms; used "forms" to reorder forms.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13222, T12588
Differential Revision: https://secure.phabricator.com/D19854
Summary:
Ref T13222. Ref T12588. See PHI683. Currently, "Create Subtask" always uses the first edit form that the user has access to for the same task subtype. (For example, if you "Create Subtask" from a "Bug", you get the first edit form for "Bugs".)
I didn't want to go too crazy with the initial subtype implementation, but it seems like we're generally on firm ground and it's working fairly well: user requests are for more flexibility in using the system as implemented, not changes to the system or confusion/difficulty with any of the tradeoffs. Thus, I'm generally comfortable continuing to build it out in the same direction. To improve flexibility, I want to make the options from "Create Subtask" more flexible/configurable.
I plan to let you specify that a given subtype (say, "Quest") prompts you with creation options for a set of other subtypes (say, "Objective"), or prompts you with a particular set of forms.
If we end up with a single option, we just go into the current flow (directly to the edit form). If we end up with more than one option, we prompt the user to choose between them.
This change is a first step toward this:
- When building "Create Subtask", query for multiple forms.
- The default behavior is now "prompt user to choose among create forms of the same subtype". Previously, it was "use the first edit form of the same subtype". This is a behavioral change.
- The next change will make the selected forms configurable.
- (I also plan to make the dialog itself less rough.)
Test Plan: {F6051067}
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13222, T12588
Differential Revision: https://secure.phabricator.com/D19853
Summary: Ref T13222. Ref T12588. See PHI683. To make "Create Subtask..." fancier, we need slightly more logic around subtype maps. Upgrade the plain old array into a proper object so it can have relevant methods, notably "get a list of valid child subtypes for some parent subtype".
Test Plan: Created and edited tasks, changed task subtypes. Grepped for affected symbols (`newEditEngineSubtypeMap`, `newSubtypeMap`).
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13222, T12588
Differential Revision: https://secure.phabricator.com/D19852
Summary:
Ref T13222. See PHI683. Currently, you can "Change subtype..." via Conduit and the bulk editor, but not via the comment action stack or edit forms.
In PHI683 an install is doing this often enough that they'd like it to become a first-class action. I've generally been cautious about pushing this action to become a first-class action (there are some inevitable rough edges and I don't want to add too much complexity if there isn't a use case for it) but since we have evidence that users would find it useful and nothing has exploded yet, I'm comfortable taking another step forward.
Currently, `EditEngine` has this sort of weird `setIsConduitOnly()` method. This actually means more like "this doesn't show up on forms". Make it better align with that. In particular, a "conduit only" field can already show up in the bulk editor, which is goofy. Change this to `setIsFormField()` and convert/simplify existing callsites.
Test Plan:
There are a lot of ways to reach EditEngine so this probably isn't entirely exhaustive, but I think I got pretty much anything which is likely to break:
- Searched for `setIsConduitOnly()` and `getIsConduitOnly()`, converted all callsites to `setIsFormField()`.
- Searched for `setIsLockable()`, `setIsReorderable()` and `setIsDefaultable()` and aligned these calls to intent where applicable.
- Created an Almanac binding.
- Edited an Almanac binding.
- Created an Almanac service.
- Edited an Almanac service.
- Edited a binding property.
- Deleted a binding property.
- Created and edited a badge.
- Awarded and revoked a badge.
- Created and edited an event.
- Made an event recurring.
- Created and edited a Conpherence thread.
- Edited and updated the diff for a revision.
- Created and edited a repository.
- Created and disabled repository URIs.
- Created and edited a blueprint.
- Created and edited tasks.
- Created a paste, edited/archived a paste.
- Created/edited/archived a package.
- Created/edited a project.
- Made comments.
- Moved tasks on workboards via comment action stack.
- Changed task subtype via comment action stack.
Reviewers: amckinley
Reviewed By: amckinley
Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam
Maniphest Tasks: T13222
Differential Revision: https://secure.phabricator.com/D19842
Summary:
Ref T13216. See PHI970. Ref T13054. See some discussion in T13216.
When a Harbormaster Buildable object is first created for a Diff, it has no `containerPHID` since the revision has not yet been created.
We later (after creating a revision) send the Buildable a message telling it that we've added a container and it should re-link the container object.
Currently, we send this message in `applyExternalEffects()`, which runs inside the Differential transaction. If Harbormaster races quickly enough, it can read the `Diff` object before the transaction commits, and not see the container update.
Add a `didCommitTransaction()` callback after the transactions commit, then move the message code there instead.
Test Plan:
- See T13216 for substantial evidence that this change is on the right track.
- Before change: added `sleep(15)`, reproduced the issue reliably.
- After change: unable to reproduce issue even with `sleep(15)` (the `containerPHID` always populates correctly).
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13216, T13054
Differential Revision: https://secure.phabricator.com/D19807
Summary:
See PHI975. Ref T13216. Ref T2543. Previously, see D19204 and PHI433.
When you're acting on a draft revision, we change the button text to "Submit Quietly" as a hint that your actions don't generate notifications yet.
However, this isn't accurate when one of your actions is "Request Review", which causes the revision to publish.
Allow actions to override the submit button text, and make the "Request Review" action change the button text to "Publish Revision".
The alternative change I considered was to remove the word "Quietly" in all cases.
I'm not //thrilled// about how complex this change is to adjust one word, but the various pieces are all fairly clean individually. I'm not sure we'll ever be able to use it for anything else, but I do suspect that the word "Quietly" was the change in D19204 with the largest effect by far (see T10000).
Test Plan:
- Created a draft revision. Saw "Submit Quietly" text.
- Added a "Request Review" action, saw it change to "Publish Revision".
- Reloaded page, saw stack saved and "Publish Revision".
- Removed action, saw "Submit Quietly".
- Repeated on a non-draft revision, button stayed put as "Submit".
- Submitted the various actions, saw them have the desired effects.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13216, T2543
Differential Revision: https://secure.phabricator.com/D19810
Summary: Depends on D19785. Ref T13217. This converts many of the most common clause construction pathways to the new %Q / %LQ / %LO / %LA / %LJ semantics.
Test Plan: Browsed around a bunch, saw fewer warnings and no obvious behavioral errors. The transformations here are generally mechanical (although I did them by hand).
Reviewers: amckinley
Reviewed By: amckinley
Subscribers: hach-que
Maniphest Tasks: T13217
Differential Revision: https://secure.phabricator.com/D19789
Summary:
Depends on D19743. Ref T13210. Since this command can easily dump a bunch of binary data (or just a huge long blob of nonsense) to stdout, default to requiring "--output <file>".
Using `--output -` will print to stdout.
Test Plan: Ran with: no `--output`, `--output file`, `--output -`, `--output - --overwrite`. Got sensible results or errors in all cases.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13210
Differential Revision: https://secure.phabricator.com/D19744
Summary: Depends on D19738. Ref T13210. Currently, when you use "--overwrite", we just //append// the new content. Instead, actually overwrite the file.
Test Plan: Used `--overwrite`, saw an actual clean overwrite instead of an append.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13210
Differential Revision: https://secure.phabricator.com/D19739
Summary:
Ref T13210. Minor usability improvements to "bin/bulk export":
- Allow `--class task` to work (previously, only `--class ManiphestTaskSearchEngine` worked).
- If you run `--query jXIlzQyOYHPU`, don't require `--class`, since the query identifies the class on its own.
- Allow users to call `--query A --query B --query C` and get a union of all results.
Test Plan:
- Ran `--class task`, `--query A --query B`, `--query X` (with no `--class`), got good results.
- Ran various flavors of bad combinations (queries from different engines, invalid engines, query and class differing, ambiguous/invalid `--class` name) and got sensible errors.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13210
Differential Revision: https://secure.phabricator.com/D19738
Summary:
Ref T13202. In D19660, I added comments to Phriction and tweaked some CSS.
One of these tweaks was getting rid of an extra border which was rendering under the comment area. However, I took off too much and ended up removing borders from other applications.
I think we don't actually need this `setNoBorder()` stuff after all -- a later change was sufficient to stop the actual border I was trying to get rid of from rendering. So this mostly just reverts part of D19660.
This rendering still isn't perfect, but I'm fine leaving that for another day for now.
Test Plan:
- Viewed comment areas in Phriction. Saw correct number of borders (1).
- Viewed comment areas in Maniphest. Saw correct number of borders (1).
- Grepped for extraneous removed classs, no hits.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13202
Differential Revision: https://secure.phabricator.com/D19684
Summary:
Depends on D19659. Fixes T1894. Ref T13077. See PHI840.
- Add an EditEngine, although it currently supports no fields.
- Add (basic, top-level-only) commenting (we already had the table in the database).
This will probably create some issues. I'm most concerned about documents accumulating a ton of old, irrelevant comments over time which are hard to keep track of and no longer relevant. But I think this is probably a step forward in almost all cases, and a good thing on the balance.
This also moves us incrementally toward putting all editing on top of EditEngine.
Test Plan: {F5877347}
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13077, T1894
Differential Revision: https://secure.phabricator.com/D19660
Summary:
Ref T13197. See PHI873. Record when a user has MFA'd and add a little icon to the transaction, similar to the exiting "Silent" icon.
For now, this just makes this stuff more auditable. Future changes may add ways to require MFA for certain specific transactions, outside of the ones that already always require MFA (like revealing credentials).
Test Plan: {F5877960}
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13197
Differential Revision: https://secure.phabricator.com/D19665
Summary: See PHI871. Ref T13197. These sections are only divided visually and don't have textual headers. Add aural headers.
Test Plan: {F5875471}
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13197
Differential Revision: https://secure.phabricator.com/D19654
Summary: Ref T13077. There is no "PHUIDocumentView" so toss the "Pro" suffix from this classname.
Test Plan: Grepped for `PHUIDocumentView` and `PHUIDocumentViewPro`.
Reviewers: amckinley
Maniphest Tasks: T13077
Differential Revision: https://secure.phabricator.com/D19616
Summary:
Depends on D19607. Ref T13189. See PHI642. Ref T13186.
Some transactions can sometimes be applied to objects you can not edit. Currently, using `*.edit` to edit an object always explicitly requires CAN_EDIT.
Now that individual transactions require CAN_EDIT by default and can reduce or replace this requirement, stop requiring CAN_EDIT to reach the editor.
The only expected effect of this change is that low-permission edits (like disabling a user, leaving a project, or leaving a thread) can now work via `*.edit`.
Test Plan:
- Tried to perform a normal edit (changing a task title) against an object with no CAN_EDIT. Still got a permissions error.
- As a non-admin, disabled other users while holding the "Can Disable Users" permission.
- As a non-admin, got a permissions error while trying to disable other users while not holding the "Can Disable Users" permission.
Reviewers: amckinley
Maniphest Tasks: T13189, T13186
Differential Revision: https://secure.phabricator.com/D19608
Summary:
Depends on D19585. Ref T13164.
Almost all transactions require CAN_EDIT on the object, but they generally do not enforce this directly today. Instead, this is effectively enforced by Controllers, API methods, and EditEngine doing a `CAN_EDIT` check when loading the object to be edited.
A small number of transactions do not require CAN_EDIT, and instead require only a weaker/lesser permission. These are:
- Joining a project which you have CAN_JOIN on.
- Leaving a project which isn't locked.
- Joining a Conpherence thread you can see (today, no separate CAN_JOIN permission for Conpherence).
- Leaving a Conpherence thread.
- Unsubscribing.
- Using the special `!history` command from email.
Additionally, these require CAN_INTERACT, which is weaker than CAN_EDIT:
- Adding comments.
- Subscribing.
- Awarding tokens.
Soon, I want to add "disabling users" to this list, so that you can disable users if you have "Can Disable User" permission, even if you can not otherwise edit users.
It's possible this list isn't exhaustive, so this change might break something by adding a policy check to a place where we previously didn't have one. If so, we can go weaken that policy check to the appropriate level.
Enforcement of these special cases is currently weird:
- We mostly don't actually enforce CAN_EDIT in the Editor; instead, it's enforced before you get to the editor (in EditEngine/Controllers).
- To apply a weaker requirement (like leaving comments or leaving a project), we let you get through the Controller without CAN_EDIT, then apply the weaker policy check in the Editor.
- Some transactions apply a confusing/redundant explicit CAN_EDIT policy check. These mostly got cleaned up in previous changes.
Instead, the new world order is:
- Every transaction has capability/policy requirements.
- The default is CAN_EDIT, but transactions can weaken this explicitly they want.
- So now we'll get requirements right in the Editor, even if Controllers or API endpoints make a mistake.
- And you don't have to copy/paste a bunch of code to say "yes, every transaction should require CAN_EDIT".
Test Plan:
- Tried to add members to a Conpherence thread I could not edit (permissions error).
- Left a Conpherence thread I could not edit (worked properly).
- Joined a thread I could see but could not edit (worked properly).
- Tried to join a thread I could not see (permissions error).
- Implemented `requireCapabilites()` on ManiphestTransactionEditor and tried to edit a task (upgrade guidance error).
- Mentioned an object I can not edit on another object (works).
- Mentioned another object on an object I can not edit (works).
- Added a `{F...}` reference to an object I can not edit (works).
- Awarded tokens to an object I can not edit (works).
- Subscribed/unsubscribed from an object I can not edit (works).
- Muted/unmuted an object I can not edit (works).
- Tried to do other types of edits to an object I can not edit (correctly results in a permissions error).
- Joined and left a project I can not edit (works).
- Tried to edit and add members to a project I can not edit (permissions error).
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13164
Differential Revision: https://secure.phabricator.com/D19586
Summary:
Depends on D19594. See PHI823. Ref T13164.
- Add a label for the "X" button in comment areas, like "Remove Action: Change Subscribers".
- Add a label for the floating header display options menu in Differential.
- Add `role="button"` to `PHUIButtonView` objects that we render with an `<a ...>` tag.
Test Plan:
Viewed a revision with `?__aural__=true`:
- Saw "Remove Action: ..." label.
- Saw "Display Options" label.
- Used inspector to verify that some `<a class="button" ...>` now have `<a class="button" role="button" ...>`. This isn't exhaustive, but at least improves things. A specific example is the "edit", "reply", etc., actions on inline comments.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13164
Differential Revision: https://secure.phabricator.com/D19595
Summary:
Ref T13164. See PHI823. (See that issue for some more details and discussion.)
Add aural labels to various buttons which were missing reasonable aural labels.
The "Search" button (magnifying glass in the global search input) had an entire menu thing inside it. I moved that one level up and it doesn't look like it broke anything (?). All the other changes are pretty straightforward.
Test Plan:
{F5806497}
{F5806498}
- Will follow up on the issue to make sure things are in better shape for the reporting user.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13164
Differential Revision: https://secure.phabricator.com/D19594
Summary: Depends on D19584. Ref T13164. This check is an //extra// check: you need EDIT //and// this capability. Thus, we can do it in validation without issues.
Test Plan:
- This code isn't reachable today: all methods of applying this transaction do a separate check for "Can Lock" upfront.
- Commented out the "Can Lock" check in the LockController, tried to lock as a user without permission. Was rejected with a policy exception.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13164
Differential Revision: https://secure.phabricator.com/D19585
Summary:
Ref T13164. See PHI725. For real "*.search" methods, parameters get validated and you get an error if you use an empty list as a constraint.
Since "transaction.search" isn't really a normal "*.search" method, it doesn't benefit from this. Just do the check manually for now.
Test Plan: Made `transaction.search` calls with no constraints (got results); a valid costraint (got fewer results); and an invalid empty constraint (got an exception).
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13164
Differential Revision: https://secure.phabricator.com/D19562
Summary:
See PHI751. Ref T13164. We added a "silent" flag for Editors somewhat recently (currently reachable only for bulk edits with `bin/bulk ...` command).
However, this flag doesn't carry through to the sub-editor when we make inverse edge edits. These are edits like "X is a parent of Y", which cause an implicit "Y is a child of X" edit to occur.
Pass the flag through.
Test Plan:
- Rigged the relationships controller to make silent edits.
- Changed the parents of a revision from the web UI. Saw no mail or feed stories.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13164
Differential Revision: https://secure.phabricator.com/D19543
Summary: See PHI725. Ref T13151. We currently try to load comments unconditionally, but not all objects (like projects) have comments. Only try to load comments if an object actually has comments.
Test Plan: Queried for an object with no comments, like project `#masonry`, via `transaction.search`.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13151
Differential Revision: https://secure.phabricator.com/D19507
Summary:
Ref T13151. See PHI725. By default, "transaction.search" doesn't provide details about transactions because many have bad/weird/policy-violating internal types or fields.
The "create" transaction is simple and straightforward, so label it to allow callers to distinguish it.
Test Plan:
- Created a new task.
- Called `transaction.search` on it.
- Saw the labelled "create" transaction.
Reviewers: amckinley
Reviewed By: amckinley
Subscribers: swisspol
Maniphest Tasks: T13151
Differential Revision: https://secure.phabricator.com/D19505
Summary:
Depends on D19502. Ref T13151. See PHI719. An install ended up with an object with 111,000+ comments on it because someone wrote a script to treat it like a logfile.
Although we seem to do mostly okay with this (locally, it only takes about 30s to index a similar object) we'll hit a wall somewhere (since we need to hold everything in memory), and it's hard to imagine a legitimate object with more than 1,000 comments. Just ignore comments past the first thousand.
(Conpherence threads may legitimately have more than 1,000 comments, but go through a different indexer.)
Test Plan:
- Piped some comments into `maniphest.edit` in a loop to create a task with 100K comments.
- Ran `bin/search index Txxx --force` to reindex it, with `--trace`.
- Before: task indexed in about 30s.
- After: script loaded comments with LIMIT 1000 and indexed in a couple seconds.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13151
Differential Revision: https://secure.phabricator.com/D19503
Summary:
Ref T13151. See PHI683. Ref T12314.
You can currently change object subtypes via Conduit (`maniphest.edit`) but not via the web UI.
Changing object subtypes is inherently a somewhat-perilous operation that likely has a lot of rough edges we'll need to smooth over eventually, mostly around changing an object from subtype X to subtype Y, where some field exists on one but not the other. This isn't a huge issue, just not entirely intuitive.
It should also, in theory, be fairly rare.
As a reasonable middle ground, provide web UI access via the bulk editor. This makes it possible, but doesn't clutter the UI up with a rarely-used option with rough edges.
Test Plan:
- With subtypes not configured, saw a normal bulk editor with no new option.
- With subtypes configured, swapped tasks subtypes via bulk editor.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13151, T12314
Differential Revision: https://secure.phabricator.com/D19490
Summary:
See PHI251. Ref T13137.
- Replace the perplexing text box with a checkbox that explains what it does.
- Mention this feature in the documentation.
Test Plan:
- Clicked/unclicked checkbox.
- Read documentation.
- Used an existing checkbox control in Slowvote to make sure I didn't break it.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13137
Differential Revision: https://secure.phabricator.com/D19433
Summary:
See <https://discourse.phabricator-community.org/t/hidden-description-field-in-maniphest-task-breaks-form/1432>.
If you hide the "Description" field in Maniphest, we still try to render a remarkup preview for it. This causes a JS error and a nonfunctional element on the page.
Instead, hide the preview panel if the field has been locked or hidden.
Test Plan:
- Hid the field, loaded the form, no more preview panel / JS error.
- Used a normal form with the field visible, saw a normal preview.
Reviewers: amckinley
Reviewed By: amckinley
Differential Revision: https://secure.phabricator.com/D19432
Summary:
Depends on D19372. Ref T13124. See PHI505. Currently, if you `!history` a task with a lot of comments, you get output like this:
> alice added a comment.
> bailey added a comment.
> alice added a comment.
> alice added a comment.
>
> AAAA
>
> BBBB
>
> AAAA
>
> AAAA
This is impossible to read. Put the "alice added a comment." headers above the actual comments for comments after the first.
These types of mail messages are unusual, but occur in several cases:
- The new `!history` command.
- Multiple comments on a draft revision before it promotes out of draft.
- (Probably?) Conduit API updates which submit multiple comment transactions for some reason.
Test Plan: Used `bin/mail receive-test` to send a `!history` command to a task, saw a much more readable rendering of the transaction log in the resulting email.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13124
Differential Revision: https://secure.phabricator.com/D19373
Summary:
See PHI505. Ref T13124. If you're an agent of a hostile state trying to exfiltrate corporate secrets, you might find yourself foiled if Phabricator is secured behind a VPN.
To assist users in this situation, provide a "!history" command which will dump the entire history of an object in a nice text format and get through the troublesome VPN.
Some issues with this:
- You currently get all the "X added a comment." up top, and then all the comments below. This isn't terribly useful.
- This goes through the "Must Encrypt" flag, but possibly should not? (On the other hand, this is a pretty willful way to bypass it the flag.)
Test Plan: Used `bin/mail receive-test ...` to send `!history` commands, got somewhat-useful response mail.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13124
Differential Revision: https://secure.phabricator.com/D19372
Summary: Depends on D19337. Ref T13120. Ref T12414. These are slightly more substantive than namespace/network, but pretty much standard fare.
Test Plan:
- Searched for interfaces with "almanac.interface.search".
- Created and edited interfaces with "almanac.interface.edit".
- Created and edited interfaces with web UI since some stuff got tweaked.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13120, T12414
Differential Revision: https://secure.phabricator.com/D19338
Summary: Depends on D19290. Ref T13110. Differential still has some hacks in place which require these methods to "very temporarily" be nonfinal, but the badness can be slightly reduced nowadays.
Test Plan: Loaded some pages, nothing fataled.
Maniphest Tasks: T13110
Differential Revision: https://secure.phabricator.com/D19291
Summary:
Depends on D19279. Ref T13110. This implements the existing publishing logic for buildables, but does so via ModularTransactions instead of a core transaction type.
Since each application is implementing build transactions independently, this removes the core type.
Next, Differential will get a similar treatment.
Test Plan: Used `bin/harbormaster publish` (with some commenting-out-guard-clauses) to publish a commit Buildable; saw unchanged feed behavior.
Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam
Maniphest Tasks: T13110
Differential Revision: https://secure.phabricator.com/D19280
Summary: Ref T13114. See PHI510. Firing Herald on mentioned objects tends to feel arbitrary and can substantially slow down edits which mention many objects.
Test Plan: Mentioned tasks on other tasks; verified that the normal path is hit normally, the new Herald-free path is hit on the mentioned object, and both still work fine and show up in the timeline.
Maniphest Tasks: T13114
Differential Revision: https://secure.phabricator.com/D19263
Summary:
See PHI466. Ref T13108. Somewhat recently, new rules were added so that "Resigning" from a revision takes you off the default recipient list, even if you're still a member of a project or package that is still a reviewer or subscriber.
However, these rules don't currently apply to the similar expansion which occurs in notifications. If you resign from a revision you may still get some notifications (just not email) if a package or project you're a member of is a reviewer or subscriber.
(Possibly these should eventually share more code, but just get things working for now.)
Test Plan:
- Created a revision as A.
- Added B as a reviewer.
- Added a package B is an owner for as a reviewer.
- As B, resigned. (Make sure B is also not an explicit subscriber.)
- Commented on the revision as A.
- Before: B is included in the expanded notification recipient list.
- After: B is no longer included in the expanded notification recipient list.
Maniphest Tasks: T13108
Differential Revision: https://secure.phabricator.com/D19244
Summary: Depends on D19224. Ref T13106. Computing this is expensive and the value is not used. This came from D15432, but we never actually shipped that feature.
Test Plan: Saw local query cost drop from 139 to 110 with no change in functionality. Grepped for removed symbols.
Maniphest Tasks: T13106
Differential Revision: https://secure.phabricator.com/D19225
Summary: See PHI433. Ref T13102. Users in the wild have mixed expecations about exactly what "draft" means. Recent changes have tried to make behavior more clear. As part of clarifying messaging, make it explicit that `@mention` does not work on drafts by showing users a warning when they try to `@mention` a user.
Test Plan:
- Mentioned users on drafts, got a warning.
- Posted normal comments on drafts, no warning.
- Posted normal/mention comments on non-drafts, no warning.
Maniphest Tasks: T13102
Differential Revision: https://secure.phabricator.com/D19210
Summary: See PHI425. See T12678. This should be an integer, but may be a string.
Test Plan: Called `differential.revision.edit`, observed integer in result instead of string.
Differential Revision: https://secure.phabricator.com/D19186
Summary:
Ref T13099. Ref T12787. See PHI417. Differential has new "irresponsible" warnings in the timeline somewhat recently, but these publish feed stories that don't link to the revision or have other relevant details, so they're confusing on the balance.
These have a high strength so they render on top, but we actually just want to hide them from the feed and let "abraham closed Dxyz by committing rXzzz." be the primary story.
Modularize things more so that we can get this behavior. Also, respect `shouldHideForFeed()` at display time, not just publishing time.
Test Plan: Used `bin/differential attach-commit` on a non-accepted revision to "irresponsibly land" a revision. Verified that feed story now shows "closed by commit" instead of "closed irresponsibly".
Maniphest Tasks: T13099, T12787
Differential Revision: https://secure.phabricator.com/D19179
Summary:
Depends on D19119. Ref T13083. This is probably still very buggy, but I'm planning to build support tools to make debugging facts easier shortly.
This generates a large number of datapoints, at least, and can render some charts which aren't all completely broken in an obvious way.
Test Plan: Ran `bin/fact analyze --all`, got some charts with lines that went up and down in the web UI.
Subscribers: yelirekim
Maniphest Tasks: T13083
Differential Revision: https://secure.phabricator.com/D19120
Summary:
Fixes T10969. Ref T13077. When you create a Phriction document with a relative link (`[[ ./path/to/page ]]`) the initial email currently points to the wrong place.
This is because the context object (the page) isn't passed to the markup engine. Without this context, the relative link is rendered as though it appeared somewhere else (like a task or revision) where relative links don't make sense.
Test Plan: Created a new Phriction document with a relative link to `[[ ./porcupine_facts/starmap ]]`, saw a usable link in the resulting email.
Maniphest Tasks: T13077, T10969
Differential Revision: https://secure.phabricator.com/D19105
Summary:
Depends on D19064. Ref T13054. See that task for additional discussion.
When buildables are created by `arc` and have lint/unit messages, they can currently pass or fail before Herald triggers actual builds. This puts them in a pre-build state where they can't complete until Herald says it's okay.
On its own, this change intentionally strands `arc diff --only` diffs in the "PREPARING" stage forever.
Test Plan:
- Ran a build with `bin/harbormaster`, saw it build normally.
- Ran a build with web UI, saw it build normally.
- Ran a build with `arc diff`, saw it build normally.
- Ran a build with `arc diff --only`, saw it hang in "PREPARING" forever.
Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam
Maniphest Tasks: T13054
Differential Revision: https://secure.phabricator.com/D19065
Summary: Depends on D19063. Ref T13054. Prepare for the addition of a new `PREPARING` status by getting rid of the "scattered mess of switch statements" pattern of status management.
Test Plan: Searched/browsed buildables. Viewed buildables. Viewed revisions. Grepped for all affected symbols.
Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam
Maniphest Tasks: T13054
Differential Revision: https://secure.phabricator.com/D19064
Summary: Fixes T13071. See that task for discusison. I think this `<= version` constraint is needless in normal cases (it should match everything in the table anyway), and slightly harmful in bizarre cases where a draft somehow gets a much larger ID than it should have.
Test Plan:
- Gave a draft an unreasonably large ID.
- Pre-patch, observed: submitting comments on the draft's object does not clear the draft.
- Post-patch: submitting comments on the draft's object now clears the draft correctly.
- Also added comments/actions, reloaded pages, saw drafts stick properly.
Maniphest Tasks: T13071
Differential Revision: https://secure.phabricator.com/D19060
Summary: Ref T13054. Fixes T12714. Applies read locks to all transactions instead of only a very select subset (chat messages in Conpherence).
Test Plan: See <T13054#235650> for discussion and testing.
Maniphest Tasks: T13054, T12714
Differential Revision: https://secure.phabricator.com/D19059
