Till/2400 message banner #2421
Till/2400 message banner #2421
Conversation
tillprochaska
force-pushed
the
till/2400-message-banner
branch
2 times, most recently
from
184b85f
to
02c07e1
Compare
| "ui_uri": settings.APP_UI_URL, | ||
| "messages_url": settings.APP_MESSAGES_URL, |
There was a problem hiding this comment.
What do we prefer? URI or URL?
There was a problem hiding this comment.
URL is fine, it seems consistent with the settings portion of the line above, which isn't actually consistent with itself, hmmm.
tillprochaska
force-pushed
the
till/2400-message-banner
branch
from
3d829ca
to
062e083
Compare
There was a problem hiding this comment.
The implementation works and seems to be well developed, it's obvious that you have thoughts about the implementation, there is testing in place which is awesome.
Where I've been struggling with this implementation is in the setting of the different levels. There doesn't seem to be any documentation on the labels I should be setting.
I determined that type:info, type:minor, and type:major all set different levels. But I could not find that documented anywhere. There is also type:announcement and type:scheduled-maintenance which is in our internal documentation on the wiki.
I also found that when I set the label type:major that the UI used the warning color, which was confusing.
I recommend that we take a look at the documentation, and ensure that it is clear how a user/admin can set the various types.
| @@ -34,24 +40,55 @@ import ExportsScreen from 'src/screens/ExportsScreen/ExportsScreen'; | |||
|
|
|||
| import './Router.scss'; | |||
|
|
|||
| const MESSAGES_INTERVAL = 15 * 60 * 1000; // every 15 minutes | |||
There was a problem hiding this comment.
Why a check every 15 minutes? What are the implications of a shorter/longer interval?
There was a problem hiding this comment.
Refreshing the messages more frequently seemed a little wasteful. The worst case scenario here is that users will see an update 15 minutes after it has been published, which seemed acceptable to me.
There was a problem hiding this comment.
Something I noticed when I was playing with the message banner earlier in the week was that updates to the UI were inconsistent. I reduced the refresh to 60 seconds and even accounting for the build process is was really hit and miss as to when the update would show. I also couldn't figure out how long the "fixed" green banner would persist?
That’s a bug! Just pushed a commit to fix it. |
|
Two more things. 1. Convert the review from a draft to a real PR. It's confusing otherwise. 2. Are you really looking to merge this directly into main? Should the endpoint actually be the develop branch? |
Using a light font weight reduces legibility and contrast, which makes it difficult to read some UI texts.
Disatching the action to fetch messages in the router is a little unintuitive to me, but we already do the same for the metadata in that place, so I thought it would be good to be consistent.
There already is a way to set a app-wide message banner via the `ALEPH_APP_BANNER` environment variable. Users will still be able to use that approach and it takes precedences over other messages.
This is another way to display a warning message (only on the home page though). It was introduced in #1907 for a similar purpose (to inform users that anonymous access is temporarily disabled). As we now have two ways to set an app-wide message banner (via the `APP_BANNER` env variable and by configuring a messages endpoint), I think we can now remove this feature.
tillprochaska
force-pushed
the
till/2400-message-banner
branch
from
d29ec37
to
552f312
Compare
I have documented the available message types here: https://github.com/alephdata/status-page-action#status-types (The message banner implemented in Aleph is actually agnostic to the message type displayed. It will simply display a message in a color that is specified in the JSON. The types that are supported, which color should be used to display a message, for how long messages are displayed after they’ve been closed etc. is handled by the GitHub Action. I want to keep those details -- which in part are probably specific to our internal workflows -- out of the Aleph codebase.)
Ah, yes, I forget that every time. So annoying that GitHub has no option to configure the default target branch for PRs. |
ALEPH_APP_MESSAGES_URL) or set to a static value via theALEPH_APP_BANNERvariable.A few things to consider:
The messages JSON endpoint can be configured via an environment variable. The value of that variable is exposed to the frontend via the app metadata. This isn’t optimal for two reasons:
It creates a request waterfall, i.e. the metadata has to be loaded before being able to load messages. This only happens once on app startup and honestly this also isn’t the worst performance bottleneck, so this should be fine.
If the metadata endpoint is unavailable (e.g. because the API service crashed), the frontend won’t be able to load messages, i.e. we can’t display a message banner in case of a full outage.
The only way to change that would be to include the messages endpoint in the frontend image. That means that everyone who’d like to use this feature would have to build a customized UI Docker image. Additionally, configuring this feature would be different from configuring everything else.
I don’t think it’s worth the effort. As an alternative generating a simple, static HTML status page with the same information would be much more sensible. Users would be able to find status information there even in case of a full outage.
I’m unsure about naming. "Messages" is pretty generic. On the other hand, this might be used for things other than incidents/system status (e.g. announcing new features or datasets). We already use the terms alerts, notifications for different concepts.