Page MenuHomePhorge

Fix notification message when Aphlict is not configured
ClosedPublic

Authored by valerio.bozzolan on Jun 26 2023, 20:07.
Tags
None
Referenced Files
F2205179: D25312.1716830167.diff
Sun, May 26, 17:16
F2205146: D25312.1716826941.diff
Sun, May 26, 16:22
Unknown Object (File)
Thu, May 23, 11:46
Unknown Object (File)
Thu, May 23, 11:46
Unknown Object (File)
Tue, May 21, 23:37
Unknown Object (File)
Mon, May 20, 02:34
Unknown Object (File)
Sun, May 19, 01:34
Unknown Object (File)
Sun, May 19, 00:23

Details

Summary

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

BeforeAfter
Phorge notification server not enabled Before.png (35×360 px, 2 KB)
Phorge notification server not enabled After.png (35×360 px, 2 KB)

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

BeforeAfter
Phorge notification connecting Before.png (35×360 px, 1 KB)
Phorge notification connecting After.png (35×360 px, 1 KB)

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

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 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.

src/applications/notification/view/PhabricatorNotificationStatusView.php
63

I totally agree on the "get" but I tried to be in line with protected function getTagContent() { just above

src/applications/notification/view/PhabricatorNotificationStatusView.php
63

to be fair, getTagContent does figure out its data on its own...

valerio.bozzolan marked 4 inline comments as done.

following review tips

This revision is now accepted and ready to land.Jun 27 2023, 09:15