Skip to content

Conversation

PhilBastian
Copy link
Contributor

@PhilBastian PhilBastian commented Mar 14, 2025

In this PR

  • The "All Messages" menu displays a list of all messages - both successfully processed audit messages as well as error messages
  • Display messages details in a grid (i.e. HTML table, as per existing ServicePulse functionality). Columns shown:
    • Status (Icon, not text, see below)
    • Message Id
    • Message Type
    • Time Sent
    • Processing Time
  • Toggle switch for 'Auto refresh' with an input box to take in the refresh time period in seconds.
  • Pagination of messages
  • Clicking on a message takes the user to a details view page ( draft )

@PhilBastian PhilBastian self-assigned this Mar 14, 2025
Copy link
Member

@johnsimons johnsimons left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nothing major, just a few comments that can be looked at in subsequent PRs

Comment on lines +38 to +42
<div>
<OnOffSwitch :id="id" @toggle="toggleRefresh" :value="autoRefresh" />
</div>
<input type="number" v-model="refreshTimeout" min="1" max="600" v-on:change="updateTimeout" />
<span class="unit">s</span>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could a dropdown with pre-set refresh ratings including zero be better option?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this works, and looks fine...

<i class="fa fa-lg fa-refresh" />
</button>
<span>|</span>
<label>Auto-Refresh:</label>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would be nice to have some kind of visual indication that we are fetching new data - https://fontawesome.com/icons/spinner?f=classic&s=solid

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

added as idea

alias?: string;
redirect?: string;
title: string;
component: RouteComponent | (() => Promise<RouteComponent>);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it may be better to introduce a new type, e.g:

export interface RedirectRouteItem {
  path: string;
  title: string;
}

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this type reflects what VueRouter expects


const dataRetriever = useAutoRefresh(async () => {
try {
const [response, data] = await useTypedFetchFromServiceControl<Message[]>(
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It may be good to introduce a typed client.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i don't understand what you mean

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

look at throughputclient

Comment on lines +52 to +58
function friendlyTypeName(messageType: string) {
if (messageType == null) return null;
const typeClass = messageType.split(",")[0];
const typeName = typeClass.split(".").reverse()[0];
return typeName.replace(/\+/g, ".");
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This can be problematic, it may be worth adding a card to the board.

Copy link
Member

@johnsimons johnsimons Mar 17, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In other places, we show the whole type name

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

added card

@johnsimons johnsimons changed the title migrate SI message list Add all messages list to ServicePulse Mar 17, 2025
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was thinking that it may also make sense to save in local storage the current setting, so it won't reset

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

added as idea


<template>
<div class="refresh-config">
<button class="fa" title="refresh" @click="() => emit('manualRefresh')">
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
<button class="fa" title="refresh" @click="() => emit('manualRefresh')">
<button class="btn btn-secondary btn-sm" title="refresh" @click="() => emit('manualRefresh')">

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why? I don't want this to look like other buttons in the system

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why should this button be different?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

because of the layout

@PhilBastian PhilBastian enabled auto-merge (squash) March 18, 2025 06:54
# Conflicts:
#	src/Frontend/src/components/failedmessages/MessageRedirectForBackwardsCompatibility.vue
@PhilBastian PhilBastian merged commit e14777f into master Mar 18, 2025
5 checks passed
@PhilBastian PhilBastian deleted the si_integration branch March 18, 2025 08:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants