Summary:
Static code analysis can detect `Unreachable statement - code above always terminates.`
The vast majority of occurrences in the Phorge codebase are due to an unreachable `break` within a `case` after a `return` or after an all-covering `if/else`.
All this noise makes it harder to spot real logic issues (there are some!), thus fix these trivial cases.
Test Plan: Examine the code; run static code analysis.
Reviewers: O1 Blessed Committers, valerio.bozzolan
Reviewed By: O1 Blessed Committers, valerio.bozzolan
Subscribers: tobiaswiese, valerio.bozzolan, Matthew, Cigaryno
Differential Revision: https://we.phorge.it/D25802
Summary:
Add variable names (`$varname` in `@param type $varname explanation`) to PHPDoc method headers, for fun and profit.
Closes T15923
Test Plan:
* Read the method signatures and their corresponding PHPDoc headers at your fireplace
* Still run `./bin/diviner generate` without explosions (though it is very lenient anyway?)
Reviewers: O1 Blessed Committers, valerio.bozzolan
Reviewed By: O1 Blessed Committers, valerio.bozzolan
Subscribers: tobiaswiese, valerio.bozzolan, Matthew, Cigaryno
Maniphest Tasks: T15923
Differential Revision: https://we.phorge.it/D25794
Summary:
When an admin removes a comment (e.g. spam), the admin gets subscribed to the task. This is usually unwanted as the removal action does not imply that the admin is interested in receiving future notifications about the task, in contrast to e.g. adding a comment to a discussion in the task.
Any transaction of a comment (add, edit, remove) is a `"core:comment"` action. The code calls `applyImplicitCC()` which calls `shouldImplyCC()` which returns the bool `$xaction->isCommentTransaction()`. Expand this bool to `$xaction->isCommentTransaction() && !($xaction->getComment()->getIsRemoved())`.
Closes T15899
Test Plan:
* As an admin, go to a task which has comments and to which you are not subscribed
* Click the dropdown for the comment, select Remove comment
* See that you did not get subscribed to the task
Reviewers: O1 Blessed Committers, valerio.bozzolan
Reviewed By: O1 Blessed Committers, valerio.bozzolan
Subscribers: tobiaswiese, valerio.bozzolan, Matthew, Cigaryno
Maniphest Tasks: T15899
Differential Revision: https://we.phorge.it/D25760
Summary:
Explicitly set "Created" as ActionName (to be used as a mail subject line vary prefix) when the transaction type is `PhabricatorTransactions::TYPE_CREATE` to avoid falling back to a generic "Updated" prefix.
Closes T15865
Test Plan: Either create a new task and check the mail subject line prefix (if you have a mail setup in your Phorge instance), or use the debug patch in P45 and check the order of action strengths.
Reviewers: O1 Blessed Committers, valerio.bozzolan
Reviewed By: O1 Blessed Committers, valerio.bozzolan
Subscribers: tobiaswiese, valerio.bozzolan, Matthew, Cigaryno
Maniphest Tasks: T15865
Differential Revision: https://we.phorge.it/D25732
Summary:
If a single transaction performs column moves on multiple different boards (which is permitted in the API), the rendering logic in the feed currently fails. Make it render properly.
Same fix as rPa3f4cbd7484b591425e83bbfc7642bccb04d0d57 for `getTitle()` which left out also fixing `getTitleForFeed()`.
Test Plan: Carefully read the if/else logic.
Reviewers: O1 Blessed Committers, avivey
Reviewed By: O1 Blessed Committers, avivey
Subscribers: avivey, tobiaswiese, valerio.bozzolan, Matthew, Cigaryno
Differential Revision: https://we.phorge.it/D25646
Summary:
When the user "Mrs. Kitten" adds or remove "Mrs. Kitten" as Subscriber,
usually these messages were generated:
Mrs. Kitten added a subscriber: Mrs. Kitten.
Mrs. Kitten removed a subscriber: Mrs. Kitten.
This was a bit like the Spiderman meme self-pointing Spiderman.
After this change, these beautiful messages are generated instead:
Mrs. Kitten subscribed.
Mrs. Kitten unsubscribed.
| Before | After |
| {F286215} | {F286216} |
Closes T15347
Test Plan:
- subscribe on something
- unsubscribe from something
- all other cases remain as-is
Reviewers: O1 Blessed Committers, bfs, speck
Reviewed By: O1 Blessed Committers, bfs, speck
Subscribers: bfs, speck, tobiaswiese, Matthew, Cigaryno
Maniphest Tasks: T15347
Differential Revision: https://we.phorge.it/D25191
Summary:
This small restyle makes any Removed Comment a little less
prominent than normal ones, with the goal of decreasing a
bit your in-page distractions and increase your individual
productivity in your business by at least 250 milliseconds
every 48 hours of hard work in front of your monitor.
| Before | After |
|---------------------|---------------------|
| {F274834,size=full} | {F274835,size=full} |
This implementation (which is called "Kasper on Diet")
contains these specific changes for Removed Comments:
- user icon visibility: reduced by ~50% (-> Kasper)
- black "trash" icon: reduced by ~50% (-> Diet)
- texts: visibility reduced by ~50%
- vertical padding: reduced from 16px down to 4px
Note that if your Phorge is under the Serious Business Mode,
it seems it is still technically possible to manually
activate the "Decaying Curse" proposal mentioned in the Task.
Closes T15192
Test Plan:
- Add a Comment "I love Phorge"
- Add a Comment "I love Phabricator"
- Mark the second Comment as Removed
- Call a person at your desk
- Plug that person to an eyeball tracker
If the general attention focuses first on a normal Comment and then
on the Removed Comment, this change works perfectly.
Reviewers: O1 Blessed Committers, Cigaryno, avivey
Reviewed By: O1 Blessed Committers, Cigaryno, avivey
Subscribers: speck, tobiaswiese, Matthew, Cigaryno
Tags: #comments
Maniphest Tasks: T15192
Differential Revision: https://we.phorge.it/D25096
Summary:
Ref T13682. Versioned drafts may have missing or unexpected metadata:
- versioned drafts from an older version of Phabricator may be missing metadata;
- versioned drafts created by an older UI against a newer version of Phabricator may have `null` metadata.
Generally, make these workflows robust to metadata in unexpected formats, so database debris doesn't break the UI.
Test Plan: Simulated debris, interacted with UI.
Maniphest Tasks: T13682
Differential Revision: https://secure.phabricator.com/D21849
Summary: Ref T13682. Allow users to manually attach files which are referenced (but not attached) via the UI.
Test Plan: Reference files via `{F...}`, then attached them via the UI workflow.
Maniphest Tasks: T13682
Differential Revision: https://secure.phabricator.com/D21837
Summary:
Ref T13603. On common edit pathways, extract explicit file attachments from Remarkup. These pathways are affected:
- Objects that use EditEngine and expose a remarkup area via "RemarkupEditField".
- Objects that use EditEngine to generate a comment area.
This is the vast majority of pathways, but not entirely exhaustive.
Test Plan: Created and commented on a task, explicitly attaching images. Saw images attach properly.
Maniphest Tasks: T13603
Differential Revision: https://secure.phabricator.com/D21830
Summary: Ref T13603. This adds a second write to new "attachment" storage to all writers except one in Paste, which creates the file inline.
Test Plan:
- Updated a macro image, confirmed a write to "attachment" storage (transaction pathway).
- Updated a blog profile image, confirmed a write to "attachment" storage (legacy pathway).
Maniphest Tasks: T13603
Differential Revision: https://secure.phabricator.com/D21816
Summary: Ref T13676. Ref T13588. Fix some issues that prevent "bin/phd" and "bin/drydock" from executing under PHP 8.1, broadly because `null` is being passed to `strlen()`.
Test Plan: Ran `bin/phd debug task` and `bin/drydock ...` under PHP 8.1.
Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam
Maniphest Tasks: T13676, T13588
Differential Revision: https://secure.phabricator.com/D21795
Summary:
Ref T13661.
I'm fairly sure these policies don't actually do anything (you can't "interact" with a blog) but the primarily support a Phame Post object policy of "Same as Parent Blog", which is the "natural" interact policy for a post.
Most of this is infrastructure support for mutable interact policies: today, only Maniphest has interact mutability and only via indirect effects (locking tasks), not through a directly mutable "Can Interact" policy.
Test Plan:
Ran storage upgrade, edited interact policy of a blog, saw appropriate persistence and transactions.
Created and edited a task to make sure there's no weird fallout from increasing what can be done with interact policies.
Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam
Maniphest Tasks: T13661
Differential Revision: https://secure.phabricator.com/D21751
Summary:
Ref T13620.
- Make generic edge stories render links with hovercards. Other story types (like subscriptions) already do this so I'm fairly certain this is just old code from before hovercards.
- Include a longer commit message snippet in hovercards.
Test Plan: {F8465645}
Maniphest Tasks: T13620
Differential Revision: https://secure.phabricator.com/D21574
Summary:
Ref T13513. An inline is not considered empty if it has a suggestion, but some of the shared transaction code doesn't test for this properly.
Update the shared transaction code to be aware that application comments may have more complex emptiness rules.
Test Plan:
- Posted an inline with only an edit suggestion, comment went through.
- Tried to post a normal empty comment, got an appropriate warning.
Maniphest Tasks: T13513
Differential Revision: https://secure.phabricator.com/D21287
Summary:
Ref T13513. This is mostly an infrastructure cleanup change.
In a perfect world, this would be a series of several changes, but they're tightly interconnected and don't have an obvious clean, nontrivial partition (or, at least, I don't see one). Followup changes will exercise this code repeatedly and all of these individual mutations are "obviously good", so I'm not too worried about the breadth of this change.
---
Inline comments are stored as transaction comments in the `PhabricatorAuditTransactionComment` and `DifferentialTransactionComment` classes.
On top of these two storage classes sit `PhabricatorAuditInlineComment` and `DifferentialInlineComment`. Historically, these were an indirection layer over significantly different storage classes, but nowadays both storage classes look pretty similar and most of the logic is actually the same. Prior to this change, these two classes were about 80% copy/pastes of one another.
Part of the reason they're so copy/pastey is that they implement a parent `Interface`. They are the only classes which implement this interface, and the interface does not provide any correctness guarantees (the storage objects are not actually constrained by it).
To simplify this:
- Make `PhabricatorInlineCommentInterface` an abstract base class instead.
- Lift as much code out of the `Audit` and `Differential` subclasses as possible.
- Delete methods which no longer have callers, or have only trivial callers.
---
Inline comments have two `View` rendering classes, `DetailView` and `EditView`. They share very little code.
Partly, this is because `EditView` does not take an `$inline` object. Historically, it needed to be able to operate on inlines that did not have an ID yet, and even further back in history this was probably just an outgrowth of a simple `<form />`.
These classes can be significantly simplified by passing an `$inline` to the `EditView`, instead of individually setting all the properties on the `View` itself. This allows the `DetailView` and `EditView` classes to share a lot of code.
The `EditView` can not fully render its content. Move the content rendering code into the view.
---
Prior to this change, some operations need to work on inlines that don't have an inline ID yet (we assign an ID the first time you "Save" a comment). Since "editing" comments will now be saved, we can instead create a row immediately.
This means that all the inline code can always rely on having a valid ID to work with, even if that ID corresponds to an empty, draft, "isEditing" comment. This simplifies more code in `EditView` and allows the "create" and "reply" code to be merged in `PhabricatorInlineCommentController`.
---
Client-side inline events are currently handled through a mixture of `ChangesetList` listeners (good) and ad-hoc row-level listeners (less good). In particular, the "save", "cancel", and "undo" events are row-level. All other events are list-level.
Move all events to list-level. This is supported by all inlines now having an ID at all stages of their lifecycle.
This allows some of the client behavior to be simplified. It currently depends on binding complex ad-hoc dictionaries into event handlers in `_drawRows()`, but it seems like almost all of this code can be removed. In fact, no more than one row ever seems to be drawn, so this code can probably be simplified further.
---
Finally, save an "isEditing" state. When we rebuild a revision on the client, click the "edit" button if it's in this state. This is a little hacky, but simpler to get into a stable state, since the row layout of an inline depends on a "view row" followed by an "edit row".
Test Plan:
- Created comments on either side of a diff.
- Edited a comment, reloaded, saw edit stick.
- Saved comments, reloaded, saw save stick.
- Edited a comment, typed text, cancelled, "unedited" to get state back.
- Created a comment, typed text, cancelled, "unedited" to get state back.
- Deleted a comment, "undeleted" to get state back.
Weirdness / known issues:
- Drafts don't autosave yet.
- Fixed in D21187:
- When you create an empty comment then reload, you get an empty editor. This is a bit silly.
- "Cancel" does not save state, but should, once drafts autosave.
- Mostly fixed in D21188:
- "Editing" comments aren't handled specially by the overall submission flow.
- "Editing" comments submitted in that state try to edit themselves again on load, which doesn't work.
Subscribers: jmeador
Maniphest Tasks: T13513
Differential Revision: https://secure.phabricator.com/D21186
Summary:
Ref T13411. This cleans up policy name rendering. We ultimately render into three contexts:
- Plain text contexts, like `bin/policy show`.
- Transaction contexts, where we're showing a policy change. In these cases, we link some policies (like project policies and custom policies) but the links go directly to the relevant object or a minimal explanation of the change. We don't link policies like "All Users".
- Capability contexts, where we're describing a capability, like "Can Push" or cases in Applicaitons. In these cases, we link all policies to the full policy explanation flow.
Test Plan:
- Used `bin/policy show` to examine the policy of an object with a project policy, no longer saw HTML.
- Viewed the transaction logs of Applications (ModularTransactions) and Tasks (not ModularTransactions) with policy edits, including project and custom policies.
- Clicked "Custom Policy" in both logs, got consistent dialogs.
- Viewed application detail pages, saw all capabities linked to explanatory capability dialogs. The value of having this dialog is that the user can get a full explanation of special rules even if the policy is something mundane like "All Users".
Maniphest Tasks: T13411
Differential Revision: https://secure.phabricator.com/D20804
Summary:
Fixes T8952. These feed stories are not interesting and tend to be generated as collateral damage when a non-story update is made to an old task and someone has a "subscribe me" Herald rule.
Also clean up some of the Herald field/condition indexing behavior slightly.
Test Plan: Wrote a "Subscribe X" herald rule, made a trivial update to a task. Before: low-value feed story; after: no feed story.
Maniphest Tasks: T8952
Differential Revision: https://secure.phabricator.com/D20797
Summary:
See D20540. I mistakenly multiplied some strenghts by 100 and others by 1000 when converting them to integers for `PhutilSortVector`.
Multiply them all by 100 (that is, divide the ones which were multiplied by 1000 by 10) to put things back the way they were.
Test Plan: quick mafs
Reviewers: amckinley, richardvanvelzen
Reviewed By: richardvanvelzen
Differential Revision: https://secure.phabricator.com/D20622
Summary:
See downstream <https://phabricator.wikimedia.org/T88655>. This is very marginal, but we currently allow comments consisting of //only// whitespace.
These are probably always mistakes, so treat them like completely empty comments.
(We intentionally do not trim leading or trailing whitespace from comments when posting them becuase leading spaces can be used to trigger codeblock formatting.)
Test Plan:
- Posted empty, nonempty, and whitespace-only comments.
- Whitespace-only comments now have the same behavior as truly empty comments (e.g., do not actually generate a transaction).
Reviewers: amckinley
Reviewed By: amckinley
Differential Revision: https://secure.phabricator.com/D20562
Summary:
See PHI1222. When we publish several transactions to feed at once, we sort them by "action strength" to figure out which one gets to be the title story.
This sort currently uses `msort()`, which uses `asort()`, which is not a stable sort and has inconsistent behavior across PHP versions:
{F6463721}
Switch to `msortv()`, which is a stable sort. Previously, see also T6861.
If all transactions have the same strength, we'll now consistently pick the first one.
This probably (?) does not impact anything in the upstream, but is good from a consistency point of view.
Test Plan:
Top story was published after this change and uses the chronologically first transaction as the title story.
Bottom story was published before this change and uses the chronologically second transaction as the title story.
Both stories have two transactions with the same strength ("create" + "add reviewer").
{F6463722}
Reviewers: amckinley
Reviewed By: amckinley
Differential Revision: https://secure.phabricator.com/D20540
Summary:
Ref T13294. An install is interested in a way to easily answer audit-focused questions like "what edits were made to any Herald rule in Q1 2019?".
We can answer this kind of question with a more granular version of feed that focuses on being exhaustive rather than being human-readable.
This starts a rough version of it and deals with the two major tricky pieces: transactions are in a lot of different tables; and paging across them is not trivial.
To solve "lots of tables", we just query every table. There's a little bit of sleight-of-hand to get this working, but nothing too awful.
To solve "paging is hard", we order by "<dateCreated, phid>". The "phid" part of this order doesn't have much meaning, but it lets us put every transaction in a single, stable, global order and identify a place in that ordering given only one transaction PHID.
Test Plan: {F6463076}
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13294
Differential Revision: https://secure.phabricator.com/D20531
Summary:
Ref T13283. See PHI1202. See D20519. When we apply a group of transactions, label all of them with the same "group ID".
This allows other things, notably Herald, to figure out which transactions applied together in a faithful way rather than by guessing, even though the guess was probably pretty good most of the time.
Also expose this to `transaction.search` in case callers want to do something similar. They get a list of transaction IDs from webhooks already anyway, but some callers use `transaction.search` outside of webhooks and this information may be useful.
Test Plan:
- Ran Herald Test Console, saw faithful selection of recent transactions.
- Changed hard limit from 1000 to 1, saw exception. Users should be very hard-pressed to hit this normally (they'd have to add 990-ish custom fields, then edit every field at once, I think) so I'm just fataling rather than processing some subset of the transaction set.
- Called `transaction.search`, saw group ID information available.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13283
Differential Revision: https://secure.phabricator.com/D20524
Summary:
Ref T13276. Previously, these edges were added directly with an `EdgeEditor`, so they did not generate transaction stories.
Now, they're added properly, but they aren't terribly useful in feed/mail. Hide them in those contexts, like we already do with other types of similar edges.
Test Plan: Will verify behavior on `secure`.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13276
Differential Revision: https://secure.phabricator.com/D20491
Summary:
See PHI1134. Generally, "alice added a dependent revision: ..." isn't a very interesting story. This relationship itself is valuable, but the creation of the relationship is usually pretty obvious from context.
In the specific case of PHI1134, various scripts are racing one another, but I don't think this story is of much value in the general case anyway.
Test Plan: Edited parent/child revisions, no more feed stories.
Reviewers: amckinley
Reviewed By: amckinley
Differential Revision: https://secure.phabricator.com/D20437
Summary:
Ref T13244. See PHI1059. When you lock a task, users who can edit the task can currently override the lock by using "Edit Task" if they confirm that they want to do this.
Mark these edits with an emblem, similar to the "MFA" and "Silent" emblems, so it's clear that they may have bent the rules.
Also, make the "MFA" and "Silent" emblems more easily visible.
Test Plan:
Edited a locked task, overrode the lock, got marked for it.
{F6195005}
Reviewers: amckinley
Reviewed By: amckinley
Subscribers: aeiser
Maniphest Tasks: T13244
Differential Revision: https://secure.phabricator.com/D20131
Summary:
Depends on D20115. See <https://discourse.phabricator-community.org/t/transaction-search-endpoint-does-not-work-on-differential-diffs/2369/>.
Currently, `getApplicationTransactionCommentObject()` throws by default. Subclasses must override it to `return null` to indicate that they don't support comments.
This is silly, and leads to a bunch of code that does a `try / catch` around it, and at least some code (here, `transaction.search`) which doesn't `try / catch` and gets the wrong behavior as a result.
Just make it `return null` by default, meaning "no support for comments". Then remove the `try / catch` stuff and all the `return null` implementations.
Test Plan:
- Grepped for `getApplicationTransactionCommentObject()`, fixed each callsite / definition.
- Called `transaction.search` on a diff with transactions (i.e., not a sourced-from-commit diff).
Reviewers: amckinley
Reviewed By: amckinley
Subscribers: jbrownEP
Differential Revision: https://secure.phabricator.com/D20121
Summary:
Fixes T12921. Currently, we call `getTitleForHTMLMail()`, but that calls `getTitleForMail()` which forces us into text rendering mode.
Instead, have `getTitleForHTML/TextMail()` force the rendering mode, then call `getTitleForMail()` with the desired rendering mode.
This causes stories like "epriestely added dependent tasks: x, y." to appear as links in email instead of plain text.
Test Plan: Used `bin/mail show-outbound --id ... --dump-html > out.html` to verify HTML mail.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T12921
Differential Revision: https://secure.phabricator.com/D19968
Summary:
Depends on D19896. Ref T13222. See PHI873. Add a core "Sign With MFA" transaction type which prompts you for MFA and marks your transactions as MFA'd.
This is a one-shot gate and does not keep you in MFA.
Test Plan:
- Used "Sign with MFA", got prompted for MFA, answered MFA, saw transactions apply with MFA metadata and markers.
- Tried to sign alone, got appropriate errors.
- Tried to sign no-op changes, got appropriate errors.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13222
Differential Revision: https://secure.phabricator.com/D19897
Summary:
Depends on D19914. Ref T11351. Some of the Phoilo rabbit holes go very deep.
`PhabricatorApplicationTransactionInterface` currently requires you to implement `willRenderTimeline()`. Almost every object just implements this as `return $timeline`; only Pholio, Diffusion, and Differential specialize it. In all cases, they are specializing it mostly to render inline comments.
The actual implementations are a bit of a weird mess and the way the data is threaded through the call stack is weird and not very modern.
Try to clean this up:
- Stop requiring `willRenderTimeline()` to be implemented.
- Stop requiring `getApplicationTransactionViewObject()` to be implemented (only the three above, plus Legalpad, implement this, and Legalpad's implementation is a no-op). These two methods are inherently pretty coupled for almost any reasonable thing you might want to do with the timeline.
- Simplify the handling of "renderdata" and call it "View Data". This is additional information about the current view of the transaction timeline that is required to render it correctly. This is only used in Differential, to decide if we can link an inline comment to an anchor on the same page or should link it to another page. We could perhaps do this on the client instead, but having this data doesn't seem inherently bad to me.
- If objects want to customize timeline rendering, they now implement `PhabricatorTimelineInterface` and provide a `TimelineEngine` which gets a nice formal stack.
This leaves a lot of empty `willRenderTimeline()` implementations hanging around. I'll remove these in the next change, it's just going to be deleting a couple dozen copies of an identical empty method implementation.
Test Plan:
- Viewed audits, revisions, and mocks with inline comments.
- Used "Show Older" to page a revision back in history (this is relevant for "View Data").
- Grepped for symbols: willRenderTimeline, getApplicationTransactionViewObject, Legalpad classes.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T11351
Differential Revision: https://secure.phabricator.com/D19918
Summary:
Depends on D19861. Ref T13222. See PHI996. Fixes T10743. Currently, notifications only work if a story also has a feed rendering.
Separate "visible in feed" and "visible in notifications", and make notifications query only notifications and vice versa.
Then, set the test notification stories to be visible in notifications only, not feed.
This could be refined a bit (there's no way to have the two views render different values today, for example) but since the only actual use case we have right now is test notifications I don't want to go //too// crazy future-proofing it. I could imagine doing some more of this kind of stuff in Conpherence eventually, though, perhaps.
Test Plan: Sent myself test notifications, saw them appear on my profile timeline and in the JS popup, and in my notifications menu, but not in feed.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13222, T10743
Differential Revision: https://secure.phabricator.com/D19864
Summary:
Depends on D19860. Ref T13222. Ref T10743. See PHI996.
Long ago, there were different types of feed stories. Over time, there was less and less need for this, and nowadays basically everything is a "transaction" feed story. Each story renders differently, but they're fundamentally all about transactions.
The Notification test controller still uses a custom type of feed story to send notifications. Move away from this, and apply a transaction against the user instead. This has the same ultimate effect, but involves less weird custom code from ages long forgotten.
This doesn't fix the actual problem with these things showing up in feed. Currently, stories always use the same rendering for feed and notifications, and there need to be some additional changes to fix this. So no behavioral change yet, just slightly more reasonable code.
Test Plan: Clicked the button and got some test notifications, with Aphlict running.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13222, T10743
Differential Revision: https://secure.phabricator.com/D19861
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 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 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 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:
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: 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: Ref T13053. See PHI126. Add an explicit "Mute" action to kill mail and notifications for a particular object.
Test Plan: Muted and umuted an object while interacting with it. Saw mail route appropriately.
Maniphest Tasks: T13053
Differential Revision: https://secure.phabricator.com/D19033
Summary:
Ref T13057. This makes "reverts" syntax more visible and useful. In particular, you can now `Reverts Dxx` in a revision or commit, and `Reverts <hash>` from a revision.
When you do, the corresponding object will get a more-visible cross-reference marker in its timeline:
{F5405517}
From here, we can look at surfacing revert information more heavily, since we can now query it on revision/commit pages via edges.
Test Plan: Used "reverts <hash>" and "reverts <revision>" in Differential and Diffusion, got sensible results in the timeline.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13057
Differential Revision: https://secure.phabricator.com/D18978
Summary:
Ref T13051. This puts a translation layer between the raw edge data in the transaction table and the UI that uses it.
The intent is to start writing new, more compact data soon. This class give us a consistent API for interacting with either the new or old data format, so we don't have to migrate everything upfront.
Test Plan: Browsed around, saw existing edge transactions render properly in transactions and feed. Added and removed subscribers and projects, saw good transaction rendering.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13051
Differential Revision: https://secure.phabricator.com/D18946
Summary:
Ref T13042. This adds a "silent" edit mechanism which suppresses feed stories, email, and notifications.
The other behaviors here are:
- The transactions are marked as "silent" so we can render a hint in the UI in the future to make it clear to users that they aren't missing email.
- If the editor uses Herald, mail rules are suppressed so they don't fire incorrectly (this mostly affects "the first time this rule matches, send me an email" rules: without this, they'd match "the first time" on the bulk edit, not send email, then never match again since they already matched).
- If the edit queues additional edits, those are applied silently too.
This doesn't (or, at least, shouldn't) actually change any behavior since you can't apply silent edits yet.
Test Plan:
Somewhat theoretical, since this isn't reachable yet. Should get meaningful testing in an upcoming change.
Did a bit of var_dump() / debug poking to attempt to verify that nothing too crazy is happening.
Viewed and edited objects, no changes in behavior.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13042
Differential Revision: https://secure.phabricator.com/D18882
Summary:
Fixes T12502. This transaction probably should not be getting picked for feed rendering, but it currently does get selected in some cases.
This should probably be revisited eventually (e.g., when Maniphest moves to ModularTransactions) but just fix the brokenness for now.
Test Plan:
- Created a task in a space.
- Viewed feed.
- Saw the story render with readable text.
{F4555747}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T12502
Differential Revision: https://secure.phabricator.com/D17609
If we try to render an edge transaction which uses unknown edge constants,
it turns out we fatal. Degrade instead. This happened when viewing very old
badges.
Auditors: chad
Summary:
Ref T12314. If you set a form to have the "plant" subtype, then create a task with it, save "plant" as the task subtype.
For Conduit, the default subtype is used by default, but a new "subtype" transaction is exposed. You can apply this transaction at create time to create an object of a certain subtype, or at any later time to change the subtype of an object.
This still doesn't do anything particularly useful or interesting.
Test Plan:
- Created a non-subtyped object (a Paste).
- Created "task" and "plant" tasks via different forms.
- Created "default" and "plant" tasks via Conduit.
- Changed the subtype of a task via Conduit.
- Tried to set a bad subtype.
{F3492061}
{F3492066}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T12314
Differential Revision: https://secure.phabricator.com/D17443
Summary: Ref T11114. Keep rendering and mail, toss the rest.
Test Plan: Edited and viewed reviewers.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T11114
Differential Revision: https://secure.phabricator.com/D17086
