Manage a single User and click on Delete User and see the popup
Run a test email and check the output ./bin/mail send-test --to username
Visit /maniphest/, shift+click on at least 1 Task, click on Bulk Edit Selected, Continue, see the popup
See the mentioned documentation with your big eyes. Eyes do not explode \o/

Diff Detail

Repository

rP Phorge

Lint

Lint Not Applicable

Unit

Tests Not Applicable

Event Timeline

waldyrious created this revision.Nov 14 2023, 12:12

Owners added a reviewer: O1: Blessed Committers.Nov 14 2023, 12:12

Herald added subscribers: Cigaryno, Matthew, valerio.bozzolan, tobiaswiese. · View Herald TranscriptNov 14 2023, 12:12

waldyrious requested review of this revision.Nov 14 2023, 12:12

Harbormaster completed remote builds in B933: Diff 1510.Nov 14 2023, 12:12

valerio.bozzolan added inline comments.Nov 14 2023, 13:37

conf/aphlict/README
11	✅ Nice
scripts/init/lib.php
16	✅ Nice. Here we cannot use PlatformSymbols since Arcanist is not loaded yet.
src/applications/config/option/PhabricatorConfigOption.php
80	Maybe here we can adopt the PlatformSymbols thing?
src/applications/metamta/management/PhabricatorMailManagementReceiveTestWorkflow.php
184–186	Maybe here we can adopt the PlatformSymbols thing?
src/applications/metamta/management/PhabricatorMailManagementSendTestWorkflow.php
228–230	Maybe here we can adopt the PlatformSymbols thing?
src/applications/people/controller/PhabricatorPeopleDeleteController.php
29–35	Maybe here we can adopt the PlatformSymbols thing? Also, this is a good place for the `<tt>` thing
src/applications/transactions/bulk/PhabricatorEditEngineBulkJobType.php
33–37	Also here maybe nice to adopt PlatformSymbols, and also try the `<tt>` thing
src/infrastructure/events/PhabricatorAutoEventListener.php
13	✅ Nice

waldyrious added inline comments.Nov 14 2023, 14:28

src/applications/transactions/bulk/PhabricatorEditEngineBulkJobType.php
33–37	Sure. For the record, the "tt thing" refers to D25466. We should keep in mind, though, that the CSS-based trick to make the prompt non-selectable won't work here, because the style in that revision is scoped to setup issue pages (the edited file was specifically `setup-issue.css`).

Use dynamically generated platform path where possible

Harbormaster completed remote builds in B934: Diff 1511.Nov 14 2023, 14:37

valerio.bozzolan edited the summary of this revision. (Show Details)Nov 15 2023, 12:17

valerio.bozzolan added a task: T15006: Re-brand Phorge.

We have just a minor issue but with a potentially working fix, and it's ready to land

src/applications/config/option/PhabricatorConfigOption.php
80	✅ Tested
src/applications/metamta/management/PhabricatorMailManagementReceiveTestWorkflow.php
184–186	✅ Untested but it's the same as `PhabricatorMailManagementSendTestWorkflow` and that works
src/applications/metamta/management/PhabricatorMailManagementSendTestWorkflow.php
228–230	✅ Tested running `./bin/mail send-test --to valerio.bozzolan`
src/applications/people/controller/PhabricatorPeopleDeleteController.php
29–35	✅ Tested. This may deserve your lovely un-selectable thing in the future.
src/applications/transactions/bulk/PhabricatorEditEngineBulkJobType.php
36–37	I don't know why but we probably need to delete the cast to string there, and move that cast above. Or, this does not render correctly.

valerio.bozzolan edited the test plan for this revision. (Show Details)Nov 15 2023, 13:19

So this is the only blocking Test that is failing:

Visit /maniphest/, shift+click on at least 1 Task, click on Bulk Edit Selected, Continue, see the popup

Then, ready to land \o/

This revision now requires changes to proceed.Nov 15 2023, 13:23

In D25473#13673, @valerio.bozzolan wrote:

So this is the only blocking Test that is failing:

Visit /maniphest/, shift+click on at least 1 Task, click on Bulk Edit Selected, Continue, see the popup

Oh, I see, you mean the <tt> is rendered verbatim in the page:

Actually, in this case the entire string is already wrapped in a <tt>, so it's redundant to wrap the prompt as well. I tested changing the wrapping element to a <pre>, but that doesn't improve things because the nice display of pre elements is confined to the setup-issues.css stylesheet (the one modified in D25466).

I mean, technically we could do better by using newDialog()->appendCommand() instead of newDialog()->appendParagraph() — as is done in PhabricatorPeopleDeleteController.php, resulting in the following:

...but that would require refactoring how the bulk job dialog is built; currently there's an array $parts constructed in PhabricatorEditEngineBulkJobType::getDescriptionForConfirm(), and each element of the array is passed to newDialog()->appendParagraph() in PhabricatorDaemonBulkJobMonitorController::handleRequest(). Besides being out of scope for the changes proposed in this revision, I also think that even PhabricatorPeopleDeleteController should be refactored too, to use an actual <pre> element for the commands, as is done for the setup issues pages edited in D25466, rather than a <p class="aphront-dialog-view-paragraph aphront-dialog-view-command">.

So tl;dr: as a minimal fix, I will for now just change the "phabricator" string to be the result of PlatformSymbols::getPlatformServerPath() instead, leaving the entire command wrapped in a single <tt> tag, and later we can think about refactoring these dialogs. The result is therefore this:

Remove extra <tt> in bulk job dialog

Harbormaster completed remote builds in B947: Diff 1529.Nov 18 2023, 08:57

waldyrious added inline comments.Nov 18 2023, 08:59

src/applications/transactions/bulk/PhabricatorEditEngineBulkJobType.php
36–37	I have no idea why the cast to string was here indeed. I've removed it, and the result seems to be fine, but happy to put it back if we prefer a minimal patch just in case.