Display Monitor Tags on Status Page #815

Fallstop · 2021-10-28T02:24:57Z

Description

Displays the tags from the dashboard on the public status page, this makes the internal dashboard and public status to be more consistent. For example we use the tagging system to show which services depend on which, and which team is responsible for the service.

Type of change

User Interface
New feature (non-breaking change which adds functionality)

Checklist

My code follows the style guidelines of this project
I ran ESLint and other linters for modified files
I have performed a self-review of my own code and test it
I have commented my code, particularly in hard-to-understand areas
My changes generate no new warnings
My code needed automated testing. I have added them (this is optional task)

Screenshots

Desktop Testing:

Mobile Testing:

deefdragon · 2021-10-28T09:16:06Z

I think this needs a setting for if it should be enabled or not. And or a setting on each tag as to include it or not.

I don't want someone to have private information in the tags, to only then have it exposed on the status page on accident after upgrading (it would be a useful option into the future as well.).

Saibamen · 2021-10-28T14:45:58Z

server/routers/api-router.js

@@ -141,9 +141,14 @@ router.get("/api/status-page/monitor-list", cache("5 minutes"), async (_request,
        await checkPublished();
        const publicGroupList = [];
        let list = await R.find("group", " public = 1 ORDER BY weight ");
-


Whoops accidental change, just didn't notice I delated it in the diff.

louislam · 2021-10-28T17:11:14Z

I think this needs a setting for if it should be enabled or not. And or a setting on each tag as to include it or not.

I don't want someone to have private information in the tags, to only then have it exposed on the status page on accident after upgrading (it would be a useful option into the future as well.).

Thank you for the reminding, it maybe a kind of breaking change. A toggle button is definitely needed.

Fallstop · 2021-10-28T21:07:57Z

I think this needs a setting for if it should be enabled or not. And or a setting on each tag as to include it or not.

I don't want someone to have private information in the tags, to only then have it exposed on the status page on accident after upgrading (it would be a useful option into the future as well.).

Great idea, I'll look into adding a setting that defaults to off.

Fallstop · 2021-10-29T05:25:51Z

I've added the toggle in now, have a look.

server/routers/api-router.js

Co-authored-by: Adam Stachowicz <saibamenppl@gmail.com>

Fallstop · 2021-10-30T08:21:45Z

I've rebased and tested it on the newest master commit, is there anything left before this can be merged?

deefdragon

A few minor formatting things, one potential inefficiency, and some requests for the front end.

deefdragon · 2021-10-31T09:28:27Z

src/pages/StatusPage.vue

@@ -77,6 +77,16 @@
                    <font-awesome-icon icon="save" />
                    {{ $t("Switch to Dark Theme") }}
                </button>
+
+                <button v-if="tagsVisible == 'hidden'" class="btn btn-secondary me-2" @click="changeTagsVisibilty('visible')">


So, if someone is using tabbing to navigate the UI, having 2 separate buttons for the options may cause some strange behavior. Can you combine these into one button that determines what is shown/done based on the current state?

Also, if possible, please add a tool tip that says "currently displaying tags/not displaying tags". Different UIs treat what the icon represents differently. IIRC Most use it as "make it go to this state" but some others do "it is currently in this state". The button text should cover it, but having a tool tip basically eliminates the ambiguity all together.

having 2 separate buttons for the options may cause some strange behaviour

I also thought that at first glance, but it doesn't doesn't because v-if doesn't hide the element, it completely removes it from the DOM. v-show might cause this behaviour.

But yeah, I agree with combining the buttons, I was just trying to maintain consistency with the existing code-base, and the theme button is laid out like that. I wonder if I should also refactor the theme button to the same style while I'm at it?

add a tool tip

Yup, great idea, but none of the other buttons have tool tips, might be a good idea to make that a separate PR?

IIRC Most use it as "make it go to this state"

Yeah, the "make it go to this state" is much more common and accepted, but it is always good to clear ambiguity.

Here is the new combined button (tagsVisible is going to be a bool now):

<button class="btn btn-secondary me-2" @click="changeTagsVisibilty(!tagsVisible)"> <template v-if="tagsVisible"> <font-awesome-icon icon="eye-slash" /> {{ $t("Hide Tags") }} </template> <template v-else> <font-awesome-icon icon="eye" /> {{ $t("Show Tags") }} </template> </button>

I think it getting removed from the DOM would throw off the tab highlighting as there was nothing left to highlight, which was the problem. Its fixed now regardless.

As for the tool-tips it's up to @louislam, but if he is ok with us having them, IMO we should add them right away, and then whenever touching old code (mind, only when reasonable, like with adding tests) instead of as one big PR.

src/pages/StatusPage.vue

deefdragon · 2021-10-31T09:38:28Z

server/routers/api-router.js

+            if ((await getSettings("statusPage")).statusPageTags=="visible") {
+                monitorGroup.monitorList = await Promise.all(monitorGroup.monitorList.map( async (monitor)=>{
+                    // Includes tags as an array in response, allows for tags to be displayed on public status page
+                    let tags = await R.getAll("SELECT mt.monitor_id,mt.value, tag.name, tag.color FROM monitor_tag mt JOIN tag ON mt.tag_id = tag.id WHERE mt.monitor_id = ?", [monitor.id]);
+                    return {...monitor,tags: tags}
+                }))
+            }


Logically it looks pretty sound, but is there any chance you can get this to format a little cleaner? It was a bit of a pain to read through.

the if clause on 150 I kept misinterpreting the parens

the SQL could be formatted for clarity

the callback chaining could likely do with some newlines and indenting

Also, wont this getSettings for every groupBean? I don't think that is necessary.

Also, wont this getSettings for every groupBean?

Good catch, I've cleaned up the formatting and loop in the latest commit so it should be quite a lot more readable.

Co-authored-by: deef <deef551@gmail.com>

Also to note: due to the transition of tagsVisible this breaks compatibility with the previous commits, delete the tagsVisible setting in the database to fix.

chakflying

Fix Grammar

src/pages/StatusPage.vue

Co-authored-by: Nelson Chan <chakflying@hotmail.com>

server/routers/api-router.js

src/pages/StatusPage.vue

Co-authored-by: Adam Stachowicz <saibamenppl@gmail.com>

louislam added this to the 1.11.0 milestone Oct 28, 2021

Saibamen reviewed Oct 28, 2021

View reviewed changes

Saibamen reviewed Oct 29, 2021

View reviewed changes

server/routers/api-router.js Outdated Show resolved Hide resolved

Fallstop and others added 6 commits October 30, 2021 21:16

Most hacked in POC

5e6d945

Styling for tags

9143b73

Match lint settings

348c5ec

Smarter CSS to fix Mobile alignment

24664cd

Added toggle for tag visibility

b32bfb3

Remove debug statement in server/routers/api-router.js

74688e6

Co-authored-by: Adam Stachowicz <saibamenppl@gmail.com>

Fallstop force-pushed the tags-on-status branch from 3d97b08 to 74688e6 Compare October 30, 2021 08:19

deefdragon suggested changes Oct 31, 2021

View reviewed changes

Fallstop and others added 2 commits November 1, 2021 12:52

Small formatting changes from code-review

4004926

Co-authored-by: deef <deef551@gmail.com>

Merged buttons, cleaned up SS tag retrieval and made tagsVisible a bool.

12ef9f3

Also to note: due to the transition of tagsVisible this breaks compatibility with the previous commits, delete the tagsVisible setting in the database to fix.

chakflying requested changes Nov 1, 2021

View reviewed changes

src/pages/StatusPage.vue Outdated Show resolved Hide resolved

Fix grammer in comment

191b81e

Co-authored-by: Nelson Chan <chakflying@hotmail.com>

Saibamen reviewed Nov 1, 2021

View reviewed changes

server/routers/api-router.js Outdated Show resolved Hide resolved

server/routers/api-router.js Outdated Show resolved Hide resolved

src/pages/StatusPage.vue Outdated Show resolved Hide resolved

src/pages/StatusPage.vue Outdated Show resolved Hide resolved

Fallstop and others added 2 commits November 3, 2021 11:46

Apply formatting suggestions from code review

e5d6410

Co-authored-by: Adam Stachowicz <saibamenppl@gmail.com>

Merge branch 'louislam:master' into tags-on-status

e450696

louislam merged commit f952d28 into louislam:master Nov 7, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Display Monitor Tags on Status Page #815

Display Monitor Tags on Status Page #815

Fallstop commented Oct 28, 2021

deefdragon commented Oct 28, 2021

Saibamen Oct 28, 2021

Fallstop Oct 28, 2021

louislam commented Oct 28, 2021

Fallstop commented Oct 28, 2021

Fallstop commented Oct 29, 2021

Fallstop commented Oct 30, 2021

deefdragon left a comment

deefdragon Oct 31, 2021

Fallstop Oct 31, 2021 •

edited

Loading

deefdragon Nov 1, 2021

deefdragon Oct 31, 2021

Fallstop Nov 1, 2021

chakflying left a comment

Display Monitor Tags on Status Page #815

Display Monitor Tags on Status Page #815

Conversation

Fallstop commented Oct 28, 2021

Description

Type of change

Checklist

Screenshots

deefdragon commented Oct 28, 2021

Saibamen Oct 28, 2021

Choose a reason for hiding this comment

Fallstop Oct 28, 2021

Choose a reason for hiding this comment

louislam commented Oct 28, 2021

Fallstop commented Oct 28, 2021

Fallstop commented Oct 29, 2021

Fallstop commented Oct 30, 2021

deefdragon left a comment

Choose a reason for hiding this comment

deefdragon Oct 31, 2021

Choose a reason for hiding this comment

Fallstop Oct 31, 2021 • edited Loading

Choose a reason for hiding this comment

deefdragon Nov 1, 2021

Choose a reason for hiding this comment

deefdragon Oct 31, 2021

Choose a reason for hiding this comment

Fallstop Nov 1, 2021

Choose a reason for hiding this comment

chakflying left a comment

Choose a reason for hiding this comment

Fallstop Oct 31, 2021 •

edited

Loading