1
0
Fork 0
mirror of https://we.phorge.it/source/phorge.git synced 2024-12-05 05:02:44 +01:00
Commit graph

857 commits

Author SHA1 Message Date
Valerio Bozzolan
7fe622e5d3 Auto-subscription: little less verbose
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
2023-06-15 11:52:02 +02:00
Andre Klapper
72fdf232b4 Fix PHP 8.1 "strlen(null)" exception which blocks rendering Conduit's dashboard.panel.edit page
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.

Closes T15425

Test Plan: Applied this change; afterwards `/conduit/method/dashboard.panel.edit/` correctly renders in browser.

Reviewers: O1 Blessed Committers, valerio.bozzolan

Reviewed By: O1 Blessed Committers, valerio.bozzolan

Subscribers: speck, tobiaswiese, valerio.bozzolan, Matthew, Cigaryno

Maniphest Tasks: T15425

Differential Revision: https://we.phorge.it/D25257
2023-05-30 10:57:43 +02:00
Edward Betts
4862eada5c Correct spelling mistakes
Test Plan: Only changes to spelling.

Reviewers: O1 Blessed Committers, valerio.bozzolan

Reviewed By: O1 Blessed Committers, valerio.bozzolan

Subscribers: speck, tobiaswiese, valerio.bozzolan, Matthew, Cigaryno

Differential Revision: https://we.phorge.it/D25238
2023-05-27 22:19:06 +01:00
Andre Klapper
bff0f810a7 Fix PHP 8.1 "strlen(null)" exception which blocks task Bulk Editor page
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.

Closes T15391

Test Plan:
Applied this change and afterwards, "Bulk Editor" page on `/maniphest/bulk/` rendered correctly.

To test that:

- visit a list of some Tasks (like `/maniphest/`)
- filter something (this can be done in multiple ways)
- select Tasks
- click on Bulk Edit Selected Tasks

Reviewers: O1 Blessed Committers, valerio.bozzolan

Reviewed By: O1 Blessed Committers, valerio.bozzolan

Subscribers: speck, tobiaswiese, valerio.bozzolan, Matthew, Cigaryno

Maniphest Tasks: T15391

Differential Revision: https://we.phorge.it/D25223
2023-05-20 11:46:38 +02:00
Valerio Bozzolan
2df7ea13a3 PhabricatorModularTransactionType: fix regression
Summary:
Fix a regression in this specific point:

    phutil_nonempty_string(integer) called at [<phorge>/src/applications/transactions/storage/PhabricatorModularTransactionType.php:342]

This regression was causing a broken Almanac page and maybe others.

Note: The function phutil_nonempty_string() is well-known to be very angry and
throws for any alien value. This is by design, and in many cases
this is appropriate. But not here.

The business logic here handles very generic types like integers
and really probably whatever scalar value coming from an user input
and then normalized to something else, not necessarily a string, but definitely
something that can be cast to string.

If you have better ideas about how to handle these cases, please join T15190.

Closes T15385

Test Plan:
To test Almanac:

1. go to `/almanac/network/` and create at least one network (example: "foo")
2. go to `/almanac/device/` and create at least one device (example: "bar")
3. visit that Device
4. Add Interface
    - test the creation of an empty Interface
    - test the creation of a right Interface (example: Network "foo", Address 127.0.0.1, Port 80)
    - nothing esplodes anymore

Reviewers: arnold, O1 Blessed Committers, avivey

Reviewed By: arnold, O1 Blessed Committers, avivey

Subscribers: avivey, speck, tobiaswiese, Matthew, Cigaryno

Maniphest Tasks: T15385

Differential Revision: https://we.phorge.it/D25220
2023-05-12 21:37:41 +02:00
Andre Klapper
1b08be518e Fix PHP 8.1 "strlen(null)" exception which blocks rendering error pages for Packages' Create Package and Fund's Create Initiative
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.

Closes T15365

Test Plan: Applied this change and `/packages/package/edit/form/default/` and `/fund/create/` finally rendered in web browser, showing the expected error messages about not having entered any data in the creation page.

Reviewers: O1 Blessed Committers, valerio.bozzolan

Reviewed By: O1 Blessed Committers, valerio.bozzolan

Subscribers: speck, tobiaswiese, valerio.bozzolan, Matthew, Cigaryno

Maniphest Tasks: T15365

Differential Revision: https://we.phorge.it/D25202
2023-05-08 21:43:17 +02:00
Andre Klapper
6e88528370 Fix PHP 8.1 "strlen(null)" exception which blocks rendering list of Active/All projects
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.

Closes T15312

Test Plan:
Applied this change (on top of D25144, D25145, D25146, D25147, D25151, D25152, D25153 and `/project/query/active/` rendered correctly in web browser.

Tested surfing various pages with and without Maniphest's `maniphest.subtypes`.

If you need some example custom fields, this is an example:

```
[
    {
      "key": "default",
      "name": "Task"
    },
    {
      "key": "bug",
      "name": "Bug"
    },
    {
      "key": "log",
      "name": "LogSpam"
    },
    {
      "key": "deadline",
      "name": "Deadline"
    }
  ]
```

Reviewers: O1 Blessed Committers, valerio.bozzolan

Reviewed By: O1 Blessed Committers, valerio.bozzolan

Subscribers: speck, tobiaswiese, valerio.bozzolan, Matthew, Cigaryno

Maniphest Tasks: T15312

Differential Revision: https://we.phorge.it/D25162
2023-05-01 15:55:42 +02:00
Andre Klapper
313d3b7bf2 Fix PHP 8.1 "strlen(null)" exceptions which block rendering the Maniphest task creation page
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.

Closes T15302

Test Plan:
Applied these five changes (on top of D25144, D25145, D25146, D25147, D25151)
and `/maniphest/task/edit/form/default/` finally 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: T15302

Differential Revision: https://we.phorge.it/D25152
2023-05-01 15:52:23 +02:00
Andre Klapper
5a0d82df7f Fix PHP 8.1 "strlen(null)" exception which blocks rendering creation form pages for Legalpad, Macro, Settings
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.

Closes T15309

Test Plan:
Applied this change (on top of `D25144`, `D25145`, `D25146`, `D25147`, `D25150`,
`D25151`, `D25152`, `D25153`, `D25154`, `D25155`, `D25156`, `D25158` locally applied)
and the pages `/legalpad/edit/`, `/macro/create/`, `/settings/builtin/global/`
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: T15309

Differential Revision: https://we.phorge.it/D25159
2023-04-30 19:42:43 +02:00
Andre Klapper
ac99285c57 Fix numerous PHP 8.1 "strlen(null)" exceptions trying to create a project
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 your phutil_nonempty_string() throws an exception, just
report it to Phorge to evaluate and fix together that specific corner case.

Closes T15286

Test Plan: Applied these five changes and `/project/view/1/` finally 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: T15286

Differential Revision: https://we.phorge.it/D25140
2023-04-29 14:41:20 +02:00
Valerio Bozzolan
3a8ee271d9 Render all Removed Comments a little more docile
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
2023-04-06 14:46:35 +02:00
epriestley
9426765a2c Flatten "RemarkupValue" objects when setting field defaults for custom forms
Summary:
Ref T13685. "RemarkupEditField" uses a "RemarkupValue" internally, and it currently attempts to serialize into the database unsuccessfully.

Instead, flatten the value before returning it for storage.

Test Plan:
  - Edited the default description of a task in a custom form.

Maniphest Tasks: T13685

Differential Revision: https://secure.phabricator.com/D21854
2022-06-14 09:37:35 -07:00
epriestley
835cd7f270 Improve remarkup metadata robustness against debris in the database
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
2022-05-28 06:29:43 -07:00
epriestley
5aa159a830 Pass "object" to modular transaction methods explicitly
Summary:
Ref T13682. When editing a comment, the edit pathway may invoke file PHID extraction without first formalizing an object on the Editor.

Pass the "object" explicitly to generally improve the robustness of this pathway.

Test Plan: Edited a comment.

Maniphest Tasks: T13682

Differential Revision: https://secure.phabricator.com/D21839
2022-05-25 10:25:18 -07:00
epriestley
8e703c8c35 Provide a default "loadPage()" implementation on "CursorPagedPolicyAwareQuery"
Summary: Ref T13682. Many subclasses of "CursorPagedPolicyAwareQuery" have the same implementation of "loadPage()", and this is a sensible default behavior.

Test Plan: Examined changes to verify that all removed methods have the same behavior.

Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam

Maniphest Tasks: T13682

Differential Revision: https://secure.phabricator.com/D21838
2022-05-24 10:18:53 -07:00
epriestley
5493f028dc Provide a simple "Attach File" explicit workflow for files referenced but not attached
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
2022-05-24 09:49:59 -07:00
epriestley
5033ef6f88 Give "FileAttachment" policy support and a query object
Summary: Ref T13682. This supports an "Attached Files" curtain UI element.

Test Plan: See next change.

Maniphest Tasks: T13682

Differential Revision: https://secure.phabricator.com/D21835
2022-05-23 14:08:15 -07:00
epriestley
2a0feb3de0 Only attach files that are both referenced in Remarkup and attached by explicit metadata
Summary:
Ref T13682. When a user uploads a file, then changes their mind and deletes the reference to the file, we don't actually want to attach the file.

When choosing which files to attach, only attach files which are both referenced in Remarkup and explicitly attached in remarkup metadata.

Test Plan:
  - Dropped a file into a comment, submitted it, saw it attach normally.
  - Dropped a file into a comment, deleted the reference, submitted it, saw no attachment.

Maniphest Tasks: T13682

Differential Revision: https://secure.phabricator.com/D21832
2022-05-23 14:08:14 -07:00
epriestley
8cd02e6727 Provide missing default attachment list for Files transactions
Summary: Ref T13682. This property may not exist, and should defualt to an empty array if not present.

Test Plan: Created a new object.

Maniphest Tasks: T13682

Differential Revision: https://secure.phabricator.com/D21834
2022-05-23 09:24:38 -07:00
epriestley
42876de60d Generate file attachment transactions for explicit Remarkup attachments on common edit pathways
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
2022-05-19 13:21:04 -07:00
epriestley
fee8297121 Remove file attachment edge types, including API access
Summary:
Ref T13603. This removes the file attachment edge types, which no longer have readers or writers.

It does not delete the underlying data.

This indirectly removes API access to this edge. As far as I know, this was only used by one customer (see D21480) who has migrated away from Phabricator.

Test Plan: Grepped for edge constants.

Maniphest Tasks: T13603

Differential Revision: https://secure.phabricator.com/D21823
2022-05-19 13:21:04 -07:00
epriestley
c25a8fabfc Remove all "ObjectHasFile" edge reads and writes
Summary: Ref T13603. Migrate all code which interacts with the "ObjectHasFile" edge to use the "Attachments" table instead.

Test Plan:
  - Edited a paste's view policy, confirmed associated file secret was scrambled.
  - Verified I could still view paste content as a user who could not naturally view the underlying file.

Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam

Maniphest Tasks: T13603

Differential Revision: https://secure.phabricator.com/D21819
2022-05-19 13:21:04 -07:00
epriestley
d017f3f210 Double-write file attachment to old "edge" storage and new "attachment" storage
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
2022-05-19 13:21:03 -07:00
epriestley
6fea5e5ce7 Do not extract file PHIDs from remarkup blocks for attachment
Summary: Ref T13603. When processing transactions that include remarkup blocks, stop extracting file references from them for attachment.

Test Plan: Submitted a remarkup block with a file reference, no longer saw automatic file attachment.

Maniphest Tasks: T13603

Differential Revision: https://secure.phabricator.com/D21812
2022-05-19 13:21:02 -07:00
epriestley
7693a711a7 Persist remarkup metadata in "VersionedDrafts" and record explicit file uploads
Summary:
Ref T13603. Allow "VersionedDraft" to persist remarkup comment area metadata from stacked actions controls.

When files are dragged and dropped, record them as explicit uploads in comment metadata.

Test Plan: Dragged and dropped files into Remarkup stacked action text areas (e.g., in Maniphest), reloaded page, saw metadata persist across reloads.

Maniphest Tasks: T13603

Differential Revision: https://secure.phabricator.com/D21828
2022-05-19 12:03:18 -07:00
epriestley
00a20d3cdc Fix various "strlen(null)" PHP 8.1 issues on "bin/phd" and "bin/drydock" pathways
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
2022-05-03 15:57:15 -07:00
epriestley
57469db22e Remove product literal strings in "pht()", part 9
Summary: Ref T13658.

Test Plan:
This test plan is non-exhaustive.

  - Ran `bin/storage databases`.
  - Viewed Badges UI exmaples page.
  - Used eval rule for `strings.platform.server.name`, got "Phabricator".

Maniphest Tasks: T13658

Differential Revision: https://secure.phabricator.com/D21773
2022-04-25 12:22:29 -07:00
epriestley
4d3f0dc7c7 Remove product literal strings in "pht()", part 8
Summary: Ref T13658.

Test Plan:
This test plan is non-exhaustive.

  - Viewed "remarkup.process" Conduit method API page.
  - Viewed URIs in a Diffusion repository.
  - Viewed editor protocol configuration in Settings.

Maniphest Tasks: T13658

Differential Revision: https://secure.phabricator.com/D21772
2022-04-25 12:22:29 -07:00
epriestley
21b1a304b6 Improve some PHP 8.1 behavior in "bin/auth recover"
Summary: Ref T13588. Fix a couple of argument parsing issues here.

Test Plan: Ran "bin/auth recover" under PHP 8.1.

Maniphest Tasks: T13588

Differential Revision: https://secure.phabricator.com/D21757
2022-04-19 14:55:19 -07:00
epriestley
a9822a37aa Fix a PHP 8.1 unit test failure in Projects
Summary: Ref T13588. This field may be "null" (and is probably never the empty string, but that's a more ambitious fix).

Test Plan: Ran unit tests, got a pass.

Maniphest Tasks: T13588

Differential Revision: https://secure.phabricator.com/D21752
2022-04-01 12:52:57 -07:00
epriestley
c25595417f Give Phame blogs mutable interact policies
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
2022-04-01 12:52:56 -07:00
Dmitri Iouchtchenko
1965b78b34 Fix subtype extension support check
Summary: Fixes T13663. `supportsSubtypes` tries to create an editable object, but this isn't always valid for `PhabricatorCalendarImport`. Use `instanceof` instead.

Test Plan:
  - Edited calendar import, tasks (2 different subtypes), and projects (2 different subtypes).
  - Changed task subtypes using {nav Change Subtype} action and batch editor.
  - Changed task and project subtypes using Conduit.

Reviewers: epriestley, #blessed_reviewers

Reviewed By: epriestley, #blessed_reviewers

Subscribers: Korvin

Maniphest Tasks: T13663

Differential Revision: https://secure.phabricator.com/D21714
2021-08-20 18:49:35 -04:00
epriestley
3df1e17527 Add a side nav to Conduit API method console pages
Summary: Ref T13072. Make large Conduit doc pages a bit more navigable. This prepares for updating "harbormaster.sendmessage" to support sending messages to builds.

Test Plan: Viewed various Conduit API documentation pages, clicked links.

Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam

Maniphest Tasks: T13072

Differential Revision: https://secure.phabricator.com/D21696
2021-07-21 14:16:59 -07:00
epriestley
afdef332fb Allow "transaction.search" to be called on an object type
Summary:
Ref T13631. This supports a more robust version of "poll for updates by using dateModified window queries" that uses transactions as a logical clock.

This is particularly relevant for commits, since they don't have a "dateModified" at time of writing.

Test Plan:
  - Queried for transactions by type and object.
  - Issued various invalid transaction queries, got appropriate errors.

Maniphest Tasks: T13631

Differential Revision: https://secure.phabricator.com/D21601
2021-03-10 10:20:03 -08:00
epriestley
e9804bb7e5 Provide hovercards for generic edge stories, and include more message information in commit hovercards
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
2021-02-25 10:29:58 -08:00
epriestley
58bbd6ee88 Propagate the "ContextObject" to Remarkup rendering in timelines
Summary:
Ref T13602. Currently, timeline comment rendering does not (by default) propagate the context object to the rendering layer.

This means that `@mentions` of users who can't see the object aren't rendered properly (currently: they show up as blue, but should show up as grey).

Pass the context down the stack and into the remarkup engine.

Test Plan: {F8382905}

Maniphest Tasks: T13602

Differential Revision: https://secure.phabricator.com/D21548
2021-02-13 13:37:36 -08:00
epriestley
9502312b60 Remove "final" from "private" methods in Phabricator
Summary:
Ref T13588. This has never been meaningful, but a "final private" method is specifically forbidden in PHP8.

Remove meaningless "final" from these methods, per new lint checks.

Test Plan: Ran `arc lint --everything` to identify affected methods, then `... | xargs -n1 arc lint --apply-patches`.

Maniphest Tasks: T13588

Differential Revision: https://secure.phabricator.com/D21540
2021-02-03 14:13:29 -08:00
epriestley
058d2489e7 Expose the "file attached to object" and "object attached to file" edges via "edge.search"
Summary:
See PHI1901. An install would like improved support for identifying files related to an object (like a task or revision) for retention/archival/backup/migration/snapshotting purposes.

The "attachment" edge is not really user-level: it just means "if you can see the object, that allows you to see the file". This set includes files that users may not think of as "attached", like thumbnails and internal objects which are attached for technical reasons.

However, this is generally an appropriate relationship to expose for retention purposes.

Test Plan: Used "edge.search" to find files attached to a revision and objects attached to a file.

Differential Revision: https://secure.phabricator.com/D21480
2020-10-16 13:45:35 -07:00
epriestley
6e1b5da112 Fix additional "xprintf()"-class static parameter lint errors
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
2020-09-08 11:45:48 -07:00
epriestley
6e85b521fe Don't raise the "Subscribers Won't Be Notified" draft warning if you aren't adding any non-you subscribers
Summary:
Currently, adding subscribers to a draft revision raises a warning that they won't get an email/notification.

This warning has some false positives:

  - it triggers on any subscriber change, including removing subscribers; and
  - it triggers if you're only adding yourself as a subscriber.

Narrow the scope of the warning so it is raised only if you're adding a subscriber other than yourself.

Test Plan:
  - Added a non-self subscriber, got the warning as before.
  - Added self as a subscriber, no warning (previously: warning).
  - Removed a subscriber, no warning (previously: warning).

Differential Revision: https://secure.phabricator.com/D21402
2020-07-09 14:20:51 -07:00
epriestley
a529efa5b8 Fix an issue where inline comments with only edit suggestions are considered empty
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
2020-05-23 08:24:57 -07:00
epriestley
c063e0e5ec Add "View Raw Remarkup" to inline comments
Summary: Ref T13513. Ref T11401. Support viewing raw remarkup for inlines.

Test Plan: Viewed raw remarkup on inlines.

Maniphest Tasks: T13513, T11401

Differential Revision: https://secure.phabricator.com/D21246
2020-05-13 17:14:20 -07:00
epriestley
94a95efa05 Replace "loadUnsubmittedInlineComments()" with a modern "DiffQuery"
Summary: Ref T13513. All queries now go through a reasonably minimal set of pathways and should have consistent behavior.

Test Plan:
- Loaded a revision with inlines.
- Created a new empty inline, reloaded page, saw it vanish.
- Created a new empty inline, typed draft text, did not save, reloaded page, saw draft present.
- Created a new empty inline, typed draft text. Submitted feedback, got prompt, answered "Y", saw draft text submit.
- Created a new empty inline, typed draft text, scrolled down to bottom of page, typed non-draft text, saw preview include draft text.
- Marked and submitted "Done".
- Used hide/show on inlines, verified state persisted.
- Did much of the same stuff in Diffusion, where it all works the same way (except: there's no prompt when submitting draft is-editing inlines).

Maniphest Tasks: T13513

Differential Revision: https://secure.phabricator.com/D21234
2020-05-07 16:11:02 -07:00
epriestley
c1f1345cc0 Make InlineCommentQueries more robust/consistent
Summary:
Ref T13513. Improve consistency and robustness of the "InlineComment" queries.

The only real change here is that these queries now implicitly add a clause for selecting inlines ("pathID IS NULL" or "changesetID IS NULL").

Test Plan: Browed, created, edited, and submitted inlines.

Maniphest Tasks: T13513

Differential Revision: https://secure.phabricator.com/D21227
2020-05-07 16:00:28 -07:00
epriestley
1656a2ff08 Allow inline comment storage objects to generate their own runtime objects
Summary:
Ref T13513. Currently, inline storage objects ("TransactionComment") can't directly generate a runtime object ("InlineComment").

Allow this transformation to be performed in a genric way so clunky code which does it per-object-type can be removed, lifted, or simplified.

Simplify an especially gross callsite in preview code.

Test Plan: Previewed inline comments.

Maniphest Tasks: T13513

Differential Revision: https://secure.phabricator.com/D21226
2020-05-07 15:57:49 -07:00
epriestley
f5ef341c9e Don't publish "empty" inline comments
Summary:
Ref T13513. Currently, if you start an inline and then submit overall comments, we publish an empty inline. This is literally faithful to what you did, but almost certainly not the intent.

Instead, simply ignore empty inlines at publishing time (and ignore "done" state changes for those comments).

We could delete them outright, but if we do, they'll break if you have another window open with the empty inline (since the stored comment won't exist anymore). At least for now, leave them in place.

Test Plan: Created empty inlines, submitted comments, no longer saw them publish.

Maniphest Tasks: T13513

Differential Revision: https://secure.phabricator.com/D21211
2020-05-04 13:14:04 -07:00
epriestley
67da18e374 When users submit "editing" inlines, warn them that their inlines will be saved
Summary: Ref T13513. This slightly expands the existing-but-hacky "warning" workflow to cover both "mentions on draft" and "submitting inlines being edited".

Test Plan:
  - Submitted changes to a revision with mentions on a draft, inlines being edited, both, and neither.
  - Got sensible warnings in the cases where warnings were appropriate.

Maniphest Tasks: T13513

Differential Revision: https://secure.phabricator.com/D21191
2020-05-04 13:13:15 -07:00
epriestley
b48a22bf50 Make "editing" state persistent for inline comments
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
2020-05-04 13:10:30 -07:00
epriestley
b5bed7b0fa Make omitting "value" from a transaction description an explicit error
Summary: See PHI1710. Until D21044, some transactions could omit "value" and apply correctly. This now throws an exception when accessing `$xaction['value']`. All transactions are expected to have a "value" key, so require it explicitly rather than implicitly.

Test Plan: Submitted a transaction with a "type" but no "value". After D21044, got a language-level exception. After this change, got an explicit exception.

Differential Revision: https://secure.phabricator.com/D21176
2020-04-27 13:16:00 -07:00
epriestley
d2572f8b33 Refine more Differential review state behaviors
Summary:
Ref T13516.

- Add an "Add Comment" navigation anchor.
- Make selection state more clear.
- Make hidden state tidier and more clear.
- Hide "View Options" in the hidden state to dodge all the weird behaviors it implies.
- Click to select/deselect changesets.
- When you open the view dropdown menu, then press "h", close the dropdown menu.

Test Plan: Fiddled with all these behaviors.

Maniphest Tasks: T13516

Differential Revision: https://secure.phabricator.com/D21161
2020-04-23 10:14:52 -07:00