Fix notification message when Aphlict is not configured
ClosedPublic
Actions

Authored by valerio.bozzolan on Jun 26 2023, 20:07.

Details

Reviewers

avivey

Group Reviewers

O1: Blessed Committers

Maniphest Tasks

T15415: Notifications: Fix message "Notification server not enabled"

Commits

rP1005e381604e: Fix notification message when Aphlict is not configured

Summary

Before this change the footer in the notification bar was a bit
weird on the message "Notification server not enabled":

Before	After

Also a bit wrong on the "Connecting..." message:

Before	After

The fix involves a refactor in the HTML structure in order to
match the graphic rendered when Aphlict is configured and running.

In short, we pass from this:

<span class="connection-status-text aphlict-connection-status-notenabled">
  <span class="visual-only phui-icon-view phui-font-fa fa-circle-o grey"></span>
  Notification server not enabled
</span>

To this:

<span class="visual-only phui-icon-view phui-font-fa fa-circle-o grey"></span>
<span class="connection-status-text aphlict-connection-status-notenabled">Notification server not enabled</span>

See the Task for additional details.

Closes T15415

Test Plan

Follow the official Notification documentation about the config notification.servers,
open the Notification top-bar, and check the message in these conditions:

without the config
with the config
with/without Aphlict running

https://we.phorge.it/book/phorge/article/notifications/

Diff Detail

Repository

rP Phorge

Lint

Lint Not Applicable

Unit

Tests Not Applicable

Event Timeline

valerio.bozzolan created this revision.Jun 26 2023, 20:07

Owners added a reviewer: O1: Blessed Committers.Jun 26 2023, 20:07

Herald added subscribers: Cigaryno, Matthew, tobiaswiese, speck. · View Herald TranscriptJun 26 2023, 20:07

Harbormaster completed remote builds in B605: Diff 1024.Jun 26 2023, 20:07

valerio.bozzolan requested review of this revision.Jun 26 2023, 20:07

Is it legit for this function to return an array instead of a single item? That’s the only structural question I have- everything else looks good and just some nitpicks.

src/applications/notification/view/PhabricatorNotificationStatusView.php
56	“A text” -> “a message”
58	Could you clarify that if multiple it should be space delimited.

In D25312#9149, @speck wrote:

Is it legit for this function to return an array instead of a single item? That’s the only structural question I have- everything else looks good and just some nitpicks.

Yep, interestingly the phutil_tag() runs phutil_escape_html($content):

https://we.phorge.it/source/phorge/browse/master/src/infrastructure/markup/render.php;659e13fa6d0e92f129e4c940e765846dd53c9ad4$97

And, the phutil_escape_html([array]) runs phutil_escape_html($each):

https://we.phorge.it/source/phorge/browse/master/src/infrastructure/markup/render.php;659e13fa6d0e92f129e4c940e765846dd53c9ad4$137-141

avivey subscribed.Jun 27 2023, 06:41

avivey added inline comments.

src/applications/notification/view/PhabricatorNotificationStatusView.php
63	should be called something like "make...View", because it doesn't "get" a thing, and the icon/message are provided to it. Also, I think we have a rule against using `Msg` in method names.