Summary: Make things a bit easier to understand, for the start.
Test Plan: Check return values for their types and classes.
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/D25901
Summary:
Include task title and metadata in Conduit's `maniphest.gettasktransactions` output.
This data can for example be used by IRC bots to improve their output (e.g. conditions which changes [not] to emit or text coloring based on metadata).
Upstreaming from https://phabricator.wikimedia.org/rPHABf2fd14dc1edeb41aa2874336548cfaa7fa0e87a0
NOTE: this specific API is in "frozen" status. This method is frozen and will eventually be deprecated. New code should use "transaction.search" instead.
Closes T15916
Test Plan: Call `maniphest.gettasktransactions` on a task, see two additional fields.
Reviewers: O1 Blessed Committers, valerio.bozzolan
Reviewed By: O1 Blessed Committers, valerio.bozzolan
Subscribers: tobiaswiese, valerio.bozzolan, Matthew, Cigaryno
Maniphest Tasks: T15916
Differential Revision: https://we.phorge.it/D25777
Summary:
Due to code additions in rPcb957f8d and rPadbd7d4f required due to rPd321cc81, the code intertwines handling legacy data with handling/creating modern data.
Make things more understandable by clearly separating between both (handle one after the other) and by renaming some variables for clarity, so it will become slightly easier in the future to investigate this bottleneck (it is the only code querying the ManiphestTransaction table, leading to timeouts in large Phorge installations).
Also add a specific reference to the corresponding code change in a code comment, instead of a vague "late 2017".
Also, don't use the variable name `$table` for two different things (database vs AphrontTableView) in the same function.
Test Plan: Carefully read the code. Optionally, play with http://phorge.localhost/maniphest/report/burn/ with and without setting a project filter having tasks created in the codebase before 2017-11-22, and compare that the output is still the same.
Reviewers: O1 Blessed Committers, 20after4
Reviewed By: O1 Blessed Committers, 20after4
Subscribers: 20after4, tobiaswiese, valerio.bozzolan, Matthew, Cigaryno
Differential Revision: https://we.phorge.it/D25828
Summary:
Implicitly nullable parameter declarations are deprecated in PHP 8.4:
https://php.watch/versions/8.4/implicitly-marking-parameter-type-nullable-deprecated
The proposed syntax was introduced in PHP 7.1.
Note: This code change does *not* fix all occurrences but only allows passing `./bin/storage upgrade`.
Refs T15935
Test Plan: Try to successfully run `./bin/storage upgrade` with PHP 8.4
Reviewers: O1 Blessed Committers, chris
Reviewed By: O1 Blessed Committers, chris
Subscribers: tobiaswiese, valerio.bozzolan, Matthew, Cigaryno
Maniphest Tasks: T15935
Differential Revision: https://we.phorge.it/D25814
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:
`strlen()` was used in Phabricator to check if a generic value is a non-empty string.
This behavior is deprecated since PHP 8.1. Phorge adopts `phutil_nonempty_string()` as a replacement.
Note: this may highlight other absurd input values that might be worth correcting
instead of just ignoring. If phutil_nonempty_string() throws an exception in your
instance, report it to Phorge to evaluate and fix that specific corner case.
```
ERROR 8192: strlen(): Passing null to parameter #1 ($string) of type string is deprecated at [/var/www/html/phorge/phorge/src/applications/maniphest/xaction/ManiphestTaskPointsTransaction.php:85]
```
Closes T15902
Test Plan: Unclear due to lack of steps to reproduce.
Reviewers: O1 Blessed Committers, valerio.bozzolan
Reviewed By: O1 Blessed Committers, valerio.bozzolan
Subscribers: tobiaswiese, valerio.bozzolan, Matthew, Cigaryno
Maniphest Tasks: T15902
Differential Revision: https://we.phorge.it/D25765
Summary:
Add OGP <meta> tags to Maniphest task pages when the task is publicly accessible and anonymously accessed. See https://ogp.me/
Based on rP2c72c2b924ffa3f8a49dbec636a2cdca3bae004f reverted in rP49b57eae7df52c189aef1d973823c697fc97fd4b.
Closes T15472
Test Plan:
* Use the default Phorge logo, open a Maniphest task, look at the headers in its HTML.
* Set a custom Phorge logo via `config/edit/ui.logo/`.
* Access a task with "View Policy: All Users" while logged in: No OGP headers included.
* Access a task with "View Policy: Public" while logged in: No OGP headers included.
* Access a task with "View Policy: All Users" while logged out: No OGP headers included; "Access Denied: Restricted Maniphest Task" displayed.
* Access a task with "View Policy: Public" while logged out: OGP headers included.
* Access a task with "View Policy: Public" while logged out with a task description and a task without a task description: OGP headers included.
Reviewers: O1 Blessed Committers, valerio.bozzolan
Reviewed By: O1 Blessed Committers, valerio.bozzolan
Subscribers: avivey, tobiaswiese, valerio.bozzolan, Matthew, Cigaryno
Maniphest Tasks: T15472
Differential Revision: https://we.phorge.it/D25668
Summary:
A task title is supposed to summarize a task. For long novels, there is the task description.
Closes T15898
Test Plan: Go to http://phorge.localhost/maniphest/task/edit/form/default/ and enter a task title with multi-byte characters above the threshold, press "Enter".
Reviewers: O1 Blessed Committers, valerio.bozzolan
Reviewed By: O1 Blessed Committers, valerio.bozzolan
Subscribers: tobiaswiese, valerio.bozzolan, Matthew, Cigaryno
Maniphest Tasks: T15898
Differential Revision: https://we.phorge.it/D25759
Summary:
Display a placeholder text in the text comment field of a Maniphest task if the task status has been set to Duplicate.
This makes it clearer to users (who may have not checked the task status at the top of the page) not to fragment conversations.
Closes T15749
Test Plan:
* Be logged in and go to a task which is closed as a duplicate and see the placeholder text in the field to add a comment.
* Be logged in and go to tasks which are not closed as a duplicate and see no placeholder text in the field to add a comment.
* Go to other places whose code calls a `PhabricatorApplicationTransactionCommentView` constructor and check that it still renders correctly, for example Ponder in http://phorge.localhost/Q1, Slowvote in http://phorge.localhost/V1, Differential in http://phorge.localhost/D1
Reviewers: O1 Blessed Committers, valerio.bozzolan, avivey
Reviewed By: O1 Blessed Committers, valerio.bozzolan, avivey
Subscribers: avivey, tobiaswiese, valerio.bozzolan, Matthew, Cigaryno
Maniphest Tasks: T15749
Differential Revision: https://we.phorge.it/D25546
Summary:
After removing or resetting a custom date field value or points value initially set at task creation there is no way to find and restore the previous value.
As Phorge adds a transaction entry anyway, include the previous value in the transaction title.
This makes it easier to revert an accidental removal of such data and is also more aligned with the behavior of other custom field types.
Closes T15846
Test Plan:
* Define a custom date field in Maniphest via http://phorge.localhost/config/edit/maniphest.custom-field-definitions/ and also enable http://phorge.localhost/config/edit/maniphest.points/
* Create a task with those fields set to non-custom values via a create task form which allows editing/changing these values
* Edit the task and remove/reset the values of these custom fields
* Look at the task and its transaction entries
Reviewers: O1 Blessed Committers, avivey
Reviewed By: O1 Blessed Committers, avivey
Subscribers: tobiaswiese, valerio.bozzolan, Matthew, Cigaryno
Maniphest Tasks: T15846
Differential Revision: https://we.phorge.it/D25678
Summary:
Nothing in the codebase calls or reads this function.
If it was ever used in some implementation of a custom field, then it was likely in context of the `Similar Task` functionality removed in 52f7446eea.
Test Plan: Grep the codebase.
Reviewers: O1 Blessed Committers, avivey
Reviewed By: O1 Blessed Committers, avivey
Subscribers: tobiaswiese, valerio.bozzolan, Matthew, Cigaryno
Differential Revision: https://we.phorge.it/D25677
Summary:
Maniphest expects the values of the `ids` URL parameter to be integers. Example: http://phorge.localhost/maniphest/?ids=1,2,3,4 to show a list of tasks with those IDs.
When passing monograms instead (`T` prefix, like `T123` instead of `123`), a cryptic message `Expected a numeric scalar or null for %Ld conversion` is shown.
Thus check if `$this->taskIDs` consists of integers only; if not throw a `PhutilSearchQueryCompilerSyntaxException` with an explanatory error message.
Closes T15838
Test Plan: Go to http://phorge.localhost/maniphest/?ids=T1,T2,T3,T4 before and after applying the patch.
Reviewers: O1 Blessed Committers, avivey
Reviewed By: O1 Blessed Committers, avivey
Subscribers: avivey, tobiaswiese, valerio.bozzolan, Matthew, Cigaryno
Maniphest Tasks: T15838
Differential Revision: https://we.phorge.it/D25669
Summary:
Pull the names of Priority field values instead of hardcoding them.
Closes T15799
Test Plan:
* As an admin, go to `/config/edit/maniphest.priorities/` and change the value of `"name"` of a Priority value with a value < `50` (e.g.: Low, Wishlist)
* Open `/maniphest/report/project/`
* Hover over the `Oldest (Pri)` column and check the tooltip text before and after applying this patch
Reviewers: O1 Blessed Committers, valerio.bozzolan
Reviewed By: O1 Blessed Committers, valerio.bozzolan
Subscribers: tobiaswiese, valerio.bozzolan, Matthew, Cigaryno
Maniphest Tasks: T15799
Differential Revision: https://we.phorge.it/D25602
Summary:
Allow PHP-coded Custom Fields to show things in Lists.
Also add Repository to Revision List and Flags to Maniphest lists.
Closes T15133. Ref T15512, T15750
Test Plan: Look at Repository List and Task lists that have flags.
Reviewers: O1 Blessed Committers, aklapper, valerio.bozzolan
Reviewed By: O1 Blessed Committers, aklapper, valerio.bozzolan
Subscribers: aklapper, tobiaswiese, valerio.bozzolan, Matthew, Cigaryno
Maniphest Tasks: T15750, T15512, T15133
Differential Revision: https://we.phorge.it/D25548
Summary:
Convert the "Page Size" field from PhabricatorSearchTextField to PhabricatorSearchIntField as its input should be an integer value (the number of tasks to display in the query results).
Closes T15723
Test Plan:
* Go to `/maniphest/query/advanced/`
* enter random values in the `Page Size` field and run random Maniphest queries (example values: `13`, `0`, `-13`, `foobar`, `foo13bar`, `-foo1313`, `13.5`, `-13.5`, `13,5`, `-13,5`, `-`, `-0`).
Reviewers: O1 Blessed Committers, 20after4
Reviewed By: O1 Blessed Committers, 20after4
Subscribers: tobiaswiese, valerio.bozzolan, Matthew, Cigaryno
Maniphest Tasks: T15723
Differential Revision: https://we.phorge.it/D25527
Summary:
After this change, the cover image is finally always visible to others (to people who can see the Task)
instead of being visible only to yourself.
Example conversation before this change:
- Anna: (uploads a nice Cover Image on the Task, using drag & drop from the Workboard)
- Bob: Thanks! But I cannot see that image. Please change visibility.
- Anna: Ouch! How?
- Bob: I don't know. Try Files > your File > Edit > Set Visibility > Something that includes "Bob".
- Anna: Done. Do you see my image now? (Spoiler: it's a green pepper)
- Bob: Yes and no. I mean, I can see the original file, but not from the Workboard.
- Anna: Uh? Have you tried refreshing your cache?
- Bob: Yes. Still invisible from Workboard.
- Anna: Maybe you have to change Defaults Permissions of the "Files" Application, to have "All Users"
- Bob: That is a bit invasive. Let's try. I changed the Files policy globally. Try re-uploading your image again.
- Anna: Done. Do you see it now?
- Bob: Yes! I can finally see it now, also from the Workboard.
Example conversation after this change:
- Anna: (uploads a nice Cover Image on the Task, using drag & drop from the Workboard)
Done! So, after this change, everything works as expected. Every task participant can see that image,
and nobody starts a nonsense conversation, and nobody needs to do random experiments with
default application policies.
So, after this change:
- the original file is attached to the Task
- the cover image is not orphan anymore, but mentioned in the "Transforms" of the original file
- the cover image so now inherits the original file policies
So, Task participants can finally see that cover image (transform), since it comes from a file,
and that file is attached to the Task, and since they can see that Task.
For a micro-optimization, this change refactors a bit PhabricatorFile.
You can still work in object-oriented:
lang=php
$file->attachToObject($task_phid);
But now you can also work directly with PHIDs, if necessary:
lang=php
PhabricatorFile::attachFileToObject($file_phid, $task_phid)
This patch somehow improves T15768.
Closes T15163
Closes T15703
Test Plan:
Visit a Workboard with a Column and a Task. Then:
1. Drag & Drop GNU.png from your computer to that Task
2. Drag & Drop GNU.png again there
3. Drag & Drop Linux.png to that Task
4. Drag & Drop Linux.png again
5. No nuclear implosion.
6. In all cases Task participants can see your Cover Image.
Reviewers: O1 Blessed Committers, avivey
Reviewed By: O1 Blessed Committers, avivey
Subscribers: avivey, speck, tobiaswiese, Matthew, Cigaryno
Maniphest Tasks: T15163, T15703
Differential Revision: https://we.phorge.it/D25475
Summary: T15772.
Test Plan: Add a `throw` statement in graph calculation, load a task that has a graph, see message.
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/D25570
Summary:
Sometime you just want to flag something as Resolved and keep that task claimed by nobody.
But, there are some task statuses that can auto-claim, and "Resolved" is one of these.
So, if you "Resolved", Phorge tries to set yourself as claimer.
Keeping that "claimed by nobody" is a bit tricky and also generates a confusing warning.
In fact, after you "Resolved", you can override the defaults with:
- Add Action > Assign / Claim > (nobody)
The problem is, on saving, the above action causes this warning:
> **Action With No Effect**
> One of your actions has no effect:
> The task already has the selected owner.
> Apply remaining actions?
> [ Cancel ] [ Apply Remaining Actions ]
That warning "The task already has the selected owner" really means
"The task is already claimed by nobody" and, indeed, that is exactly what the user wants.
This patch intercepts the above action, and prevents the related confusing "non-effect" warning.
Thanks to hard troubleshooting from user https://we.phorge.it/p/aklapper/
See also https://we.phorge.it/D25476
Closes T15164
Test Plan:
Task 1 open unassigned:
1. Change Status to Resolved
2. Preview yourself as Claimer
3. Add Action > Assign / Claim, and **set <nothing>**
4. Save and, instead of any confusing warning, only the Status changes.
Task 2 open unassigned:
1. Change Status to Resolved
2. Preview yourself as Claimer
3. Add Action > Assign / Claim, keep it as-is
4. Save and, it works as expected (just like before)
Task 3 open unassigned:
1. Change Status to Resolved
2. Preview yourself as Claimer
3. Add Action > Assign / Claim, set to somebody else
4. Save and, it works as expected (just like before)
Reviewers: O1 Blessed Committers, aklapper
Reviewed By: O1 Blessed Committers, aklapper
Subscribers: tobiaswiese, Matthew, Cigaryno
Maniphest Tasks: T15164
Differential Revision: https://we.phorge.it/D25550
Summary:
`::class` is available since PHP 5.5 (5.5 is a minimal requirement by Phorge): https://www.php.net/manual/en/language.oop5.basic.php#language.oop5.basic.class.class
It makes finding code using IDEs easier; see discussion in D25500.
Thus replace all string return values with returning the `::class` constant instead, with one exception: 'PhabricatorSettingApplication' in `PhabricatorUserPreferencesSearchEngine.php` does not exist and makes arc lint fail so this string remained unchanged.
Also note that two occurrences were wrapped in `pht()` for reasons I do not know.
List of functions whose return value get updated in this code change:
* getApplicationClassName()
* getAdapterApplicationClass()
* getDatasourceApplicationClass()
* getEditorApplicationClass()
* getEngineApplicationClass()
* getPHIDTypeApplicationClass()
* getQueryApplicationClass()
cf. T15158
Test Plan: Too broad - click around, basically.
Reviewers: O1 Blessed Committers, valerio.bozzolan
Reviewed By: O1 Blessed Committers, valerio.bozzolan
Subscribers: tobiaswiese, valerio.bozzolan, Matthew, Cigaryno
Maniphest Tasks: T15158
Differential Revision: https://we.phorge.it/D25524
Summary:
Add `getQueryApplicationClass()` to all `*TransactionQuery.php` classes similar to other `*Query.php` classes having the same function, and make the parent function in `PhabricatorApplicationTransactionQuery.php` abstract.
In the future, this will enable excluding transaction query results based on their underlying application (for example if an application has been uninstalled) to mitigate the problem of overheated search results. See https://we.phorge.it/T15642 for context.
The only callers of `getQueryApplicationClass()` are in `src/infrastructure/query/policy/PhabricatorPolicyAwareQuery.php` and `src/applications/policy/__tests__/PhabricatorPolicyTestCase.php`.
See T15642
Test Plan:
Patch changes only one existing code place, thus check if related pages still work as expected:
* Go to http://phorge.localhost/feed/
* Go to http://phorge.localhost/feed/transactions/
* On http://phorge.localhost/feed/transactions/ , click `Edit Query` and set `Object Types` to `Application` etc.
Reviewers: O1 Blessed Committers, valerio.bozzolan
Reviewed By: O1 Blessed Committers, valerio.bozzolan
Subscribers: tobiaswiese, valerio.bozzolan, Matthew, Cigaryno
Maniphest Tasks: T15642
Differential Revision: https://we.phorge.it/D25500
Summary:
Update the Application Detail view and List View to show a unified set of Badges (Deprecated, etc.), show PHIDs and Monograms on the Application Detail view, allow Applications to register Monograms.
Example of the page /applications/view/PhabricatorDiffusionApplication/:
{F393393}
T15568
Test Plan:
1. Visit /applications/ and see Deprecated badges etc.
2. Visit various Configure buttons from that list and see Monograms, Badges, PHIDs etc.
3. Enjoy screenshots in the comments of this Diff
Reviewers: O1 Blessed Committers, valerio.bozzolan, avivey, speck
Reviewed By: O1 Blessed Committers, valerio.bozzolan, avivey, speck
Subscribers: avivey, speck, tobiaswiese, valerio.bozzolan, Cigaryno
Differential Revision: https://we.phorge.it/D25362
Summary:
`strlen()` was used in Phabricator to check if a generic value is a non-empty string.
This behavior is deprecated since PHP 8.1. Phorge adopts `phutil_nonempty_string()` as a replacement.
Note: this may highlight other absurd input values that might be worth correcting
instead of just ignoring. If phutil_nonempty_string() throws an exception in your
instance, report it to Phorge to evaluate and fix that specific corner case.
```
EXCEPTION: (RuntimeException) strlen(): Passing null to parameter #1 ($string) of type string is deprecated at [<arcanist>/src/error/PhutilErrorHandler.php:261]
arcanist(head=master, ref.master=df6c315ace5f), phorge(head=master, ref.master=7040bd525764)
#0 <#2> PhutilErrorHandler::handleError(integer, string, string, integer) called at [<phorge>/src/applications/maniphest/query/ManiphestTaskQuery.php:1039]
```
Closes T15604
Test Plan: Have more than 100 tasks, run a broad search with more than 100 results, try to go to next page of results.
Reviewers: O1 Blessed Committers, valerio.bozzolan
Reviewed By: O1 Blessed Committers, valerio.bozzolan
Subscribers: tobiaswiese, valerio.bozzolan, Matthew, Cigaryno
Maniphest Tasks: T15604
Differential Revision: https://we.phorge.it/D25392
Summary:
Since PHP 8.1, passing a null string to `trim()` is deprecated.
Thus first check that $row['oldValue'] is not null before trimming it.
Closes T15392
Test Plan: Applied this change; afterwards "Burnup Rate" page at `/maniphest/report/burn/` got correctly rendered in web browser.
Reviewers: O1 Blessed Committers, valerio.bozzolan
Reviewed By: O1 Blessed Committers, valerio.bozzolan
Subscribers: speck, tobiaswiese, valerio.bozzolan, Matthew, Cigaryno
Maniphest Tasks: T15392
Differential Revision: https://we.phorge.it/D25224
Summary:
`strlen()` was used in Phabricator to check if a generic value is a non-empty string.
This behavior is deprecated since PHP 8.1. Here we adopt `phutil_string_cast()` to
reply to the question "is this an empty string?".
Note: this may highlight other absurd input values that might be worth correcting
instead of just ignoring. If phutil_string_cast() throws an exception in your
instance, report it to Phorge to evaluate and fix that specific corner case.
Closes T15390
Test Plan:
Enable `maniphest.points` in settings, for example with:
```
./bin/config set maniphest.points --stdin <<< '{"enabled":true}'
```
Then try to create a Task: it does not explode anymore in PHP 8.1+.
Also try to set various possible values from the Conduit API method "maniphest.edit".
Reviewers: O1 Blessed Committers, valerio.bozzolan
Reviewed By: O1 Blessed Committers, valerio.bozzolan
Subscribers: speck, tobiaswiese, valerio.bozzolan, Matthew, Cigaryno
Maniphest Tasks: T15390
Differential Revision: https://we.phorge.it/D25222
Summary: Ref T13658.
Test Plan:
This test plan is non-exhaustive.
- Ran `bin/mail`.
- Uninstalled and reinstalled an application.
Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam
Maniphest Tasks: T13658
Differential Revision: https://secure.phabricator.com/D21770
Summary: Ref T13661. These constants are no longer used.
Test Plan:
For a blog with a nonempty description, viewed the "Manage" page and the home page; saw properly rendered description.
Grepped for removed constants, no hits.
Maniphest Tasks: T13661
Differential Revision: https://secure.phabricator.com/D21749
Summary:
Ref T13628. Currently, Differential has a "Commandeer" action, but no way to edit the author otherwise.
This is largely archaic: there is no reason to prevent editing the author, and this makes it difficult to undo mistakes if you commandeer by accident.
Instead, provide a normal "Author" field and a "Foist Upon" action, similar to the "Owner" and "Claim/Assign" fields in Maniphest.
Test Plan:
- Applied author transactions as the old author ("foisted"), the new author ("commandeerd"), and an arbitrary third party ("changed author").
- Tried to unassign author, etc.
- Viewed stories in feed and transaction timeline.
- Saw sensible automatic reviewer changes.
- Used existing "Commandeer" action, which is unchanged.
- Called "transaction.search" and saw reasonable transaction values.
Maniphest Tasks: T13628
Differential Revision: https://secure.phabricator.com/D21591
Summary:
Ref T13602. When rendering a user hovercard, pass the object on which the reference appears. If the user can't see the object, make it clear on the hovecard.
Restyle the "nopermission" markup in mentions to make it more obvious what the style means: instead of grey text, use red with an explicit icon.
Test Plan: {F8430398}
Maniphest Tasks: T13602
Differential Revision: https://secure.phabricator.com/D21554
Summary: Ref T13577. After the fix in D21453, lint identifies additional static errors in Phabricator; fix them.
Test Plan: Ran `arc lint`; these messages are essentially all very obscure.
Subscribers: hach-que, yelirekim, PHID-OPKG-gm6ozazyms6q6i22gyam
Maniphest Tasks: T13577
Differential Revision: https://secure.phabricator.com/D21457
Summary:
See PHI1714. This code is incorrectly rendering the chart panel twice, sort of, and passing a non-View object to rendering.
After D21044, this fatals by raising an exception in rendering.
Test Plan: Loaded page, no more exception.
Differential Revision: https://secure.phabricator.com/D21185
Summary:
Ref T13511. Ferret functions currently define "aliases", and some applications override the default aliases.
This probably isn't really the right model, since it means the available function aliases in global search depend on the types of documents you're searching for. This isn't fundamentally unworkable but is kind of weird.
Regardless, these don't actually work. Searching for "description:x" is a syntax error.
Since they don't work, it's a good bet no one is relying on them. Just get rid of them until there's a clearer argument for the feature.
Test Plan: Grepped for "getFunctionMap", got no other hits. Ran some queries with the alias functions, got syntax errors.
Maniphest Tasks: T13511
Differential Revision: https://secure.phabricator.com/D21130
Summary:
Depends on D20966. Ref T13486. Curtains currently render subscribers in a plain text list, but the new ref list element is a good fit for this.
Also, improve the sorting and ordering behavior.
This makes the subscriber list take up a bit more space, but it should make it a lot easier to read at a glance.
Test Plan: Viewed object subscriber lists at varying limits and subscriber counts, saw sensible subscriber lists.
Maniphest Tasks: T13486
Differential Revision: https://secure.phabricator.com/D20967
Summary:
Ref T13486. When a curtain element like "Author" in Maniphest has a very long username, the wrapping and overflow behavior is poor: the date is obscured.
Adjust curtain elements which contain lists of references to other objects to improve wrapping behavior (put the date on a separate line) and overflow behavior (so we get a "..." when a name overflows).
Test Plan: {F7179376}
Maniphest Tasks: T13486
Differential Revision: https://secure.phabricator.com/D20966
Summary: Maniphest object has `getURI` method, let's use it
Test Plan: Create event in task - URI generated as expected in email notification
Reviewers: epriestley, Pawka, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D20935
Summary: Ref T13440. Give the table more obvious visual structure and get rid of the largely useless header columns.
Test Plan: Viewed table, saw a slightly cleaner result.
Maniphest Tasks: T13440
Differential Revision: https://secure.phabricator.com/D20885
Summary: Depends on D20883. Ref T13440. In most cases, all changes belong to the same repository, which makes the "Repository" column redundant and visually noisy. Show repository information in a section header.
Test Plan: {F6989932}
Maniphest Tasks: T13440
Differential Revision: https://secure.phabricator.com/D20884
Summary: Depends on D20882. Ref T13440. Instead of lists of "Differential Revisions" and "Commits", show all changes related to the task in a tabular view.
Test Plan: {F6989816}
Maniphest Tasks: T13440
Differential Revision: https://secure.phabricator.com/D20883
Summary: Ref T13440. This feature is used in only one interface which I'm about to rewrite, so throw it away.
Test Plan: Grepped for all affected symbols, didn't find any hits anywhere.
Maniphest Tasks: T13440
Differential Revision: https://secure.phabricator.com/D20882
Summary: Ref T13279. Facts is still fairly rough, but not broken/policy-violating, so it can be unprototyped to fix the issue where Maniphest reports (which are now driven by Facts) don't work if prototypes are disabled.
Test Plan: Viewed Maniphest reports and Project reports with prototypes on/off and Fact installed/uninstalled.
Subscribers: yelirekim
Maniphest Tasks: T13279
Differential Revision: https://secure.phabricator.com/D20822
Summary:
Fixes T13415. Provide a way for subtypes to customize the behavior of "Change Subtype" actions that appear above comment areas.
Subtypes may disable this action by specifying `"mutations": []`, or provide a list of subtypes.
The bulk editor and API can still perform any change.
Test Plan:
- Tried to define an invalid "mutations" list with a bad subtype, got a sensible error.
- Specified a limited mutations list and an empty mutations list, verified that corresponding tasks got corresponding actions.
- Used the bulk editor to perform a freeform mutation.
- Verified that tasks of a subtype with no "mutations" still work the same way they used to (allow mutation into any subtype).
Maniphest Tasks: T13415
Differential Revision: https://secure.phabricator.com/D20810
Summary: See PHI1434. For objects that support subtypes and have subtypes configured, allow Herald rules to act on subtypes.
Test Plan:
- Configured task and project subtypes, wrote Herald rules, saw "Subtypes" as an option, saw appropriate typeahead values and detail page rendering.
- Unconfigured project subtypes, saw field vanish from UI for new rules.
- Wrote a "subtype"-depenent rule that added a comment, interacted with tasks of that subtype and a different subtype. Saw Herald act only on tasks with the correct subtype.
Differential Revision: https://secure.phabricator.com/D20809
Summary: See PHI1396. Ideally this would be some kind of general-purpose tie-in to object relationships, but see D18456 for precedent.
Test Plan: Used `maniphest.edit` to edit associated commits for a task.
Differential Revision: https://secure.phabricator.com/D20731
Summary:
Fixes T13356. This option is supported and works fine, it just isn't documented.
Add documentation and fix the config option to actually link to it to make life a little easier.
Test Plan: Read documentation.
Maniphest Tasks: T13356
Differential Revision: https://secure.phabricator.com/D20691
Summary: Ref T4900. We may execute a bad query here if the task has no projects at all.
Test Plan: Edited a task with no new or old projects. Instead of an exception, things worked.
Maniphest Tasks: T4900
Differential Revision: https://secure.phabricator.com/D20689
Summary:
Ref T4900. When a card is edited, we currently emit an update notification for all the projects the task is tagged with. This isn't quite the right set:
- We want to emit notifications for projects the task //was previously// tagged with, so it can be removed from boards it should no longer be part of.
- We want to emit notifications for ancestors of projects the task is or was tagged with, so parent project boards can be updated.
- However, we don't need to emit notifications for projects that don't actually have workboards.
Adjust the notification set to align better to these rules.
Test Plan:
- Removal of Parent Project: Edited a task on board "A > B", removing the "B" project tag. Saw board A update in another window.
- Normal Update: Edited a task title on board X, saw board X update in another window.
- Used `bin/aphlict debug` to inspect the notification set, saw generally sensible-seeming data going over the wire.
Reviewers: amckinley
Maniphest Tasks: T4900
Differential Revision: https://secure.phabricator.com/D20680
Summary:
Depends on D20654. Ref T4900. When a task is edited, emit a "workboards" event for all boards it appears on (in a future change, this should also include all boards it //previously// appeared on, and all parents of both sets of boards -- but I'm just getting things working for now).
When we receive a "workboards" event, check if the visible board should be updated.
Aphlict has a complicated intra-window leader/follower election system which could let us process this update event exactly once no matter how many windows a user has open with the same workboard. I'm not trying to do any of this since it seems fairly rare. It makes sense for events like "you have new notifications" where we don't want to generate 100 Ajax calls if the user has 100 windows open, but very few users seem likely to have 100 copies of the same workboard open.
Test Plan:
- Ran `bin/aphlict debug`.
- Opened workboard A in two windows, X and Y.
- Edited and moved tasks in window X.
- Saw "workboards" messages in the Aphlict log.
- Saw window Y update in nearly-real-time (locally, this is fast enough that it feels instantaneous).
Then:
- Stopped the Aphlcit server.
- Edited a task.
- Started the Aphlict server.
- Saw window Y update after a few moments (i.e., update in response to a reconnect).
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T4900
Differential Revision: https://secure.phabricator.com/D20656
Summary:
Depends on D20639. Ref T4900. Currently, "BoardResponseEngine" has a `setObjectPHID()` method. This is called after edit operations to mean "we just edited object X, so we know it needs to be updated".
Move toward `setUpdatePHIDs(...)` in all cases, with `setUpdatePHIDs(array(the-object-we-just-edited))` as a special case of that. After this change, callers pass:
- An optional list of PHIDs they know need to be updated on the client. Today, this is always be a card we just edited (on edit/move flows), or a sort of made-up list of PHIDs for the moment (when you press "R"). In the future, the "R" endpoint will do a better job of figuring out a more realistic update set.
- An optional list of PHIDs currently visible on the client. This is used to update ordering details and mark cards for removal. This is currently passed by edit/move, but not by pressing "R" (it will be in the future).
- An optional list of objects. The "R" workflow has to load these anyway, so we can save a couple queries by letting callers pass them. For now, the edit/move flows still rely on the engine to figure out what it needs to load.
This does very little to actually change client behavior, it mostly just paves the way for the next update to the "R" workflow to make it handle add/remove cases properly.
Test Plan:
- Edited and moved cards on a workboard.
- Pressed "R" to reload a workboard.
Neither of these operations seem any worse off than they were before. They still don't fully work:
- When you edit a card and delete the current workboard project from it, it remains visible. This is also the behavior on `master`. This is sort of intentional since we don't necessarily want to make these cards suddenly disappear? Ideally, we would probably have some kind of "tombstone" state where the card can still be edited but can't be dragged, and the next explicit user interaction would clean up old tombstones. This interaction is very rare and I don't think it's particularly important to specialize.
- When a card is removed from the board, "R" can't currently figure out that it should be removed from the client. This is because the client does not yet pass a "visiblePHIDs" state. It will in an upcoming change.
- The "R" flow always sends a full set of card updates, and can not yet detect that some cards have not changed.
- There's a TODO, but some ordering stuff isn't handled yet.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T4900
Differential Revision: https://secure.phabricator.com/D20652
