Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

added Incident report History #4943

Open
wants to merge 12 commits into
base: master
Choose a base branch
from

Conversation

MasonColoretti
Copy link

@MasonColoretti MasonColoretti commented Jul 15, 2024

⚠️⚠️⚠️ Since we do not accept all types of pull requests and do not want to waste your time. Please be sure that you have read pull request rules:
https://github.com/louislam/uptime-kuma/blob/master/CONTRIBUTING.md#can-i-create-a-pull-request-for-uptime-kuma

Tick the checkbox if you understand [x]:

  • I have read and understand the pull request rules.

Description

Added Incident Report History for the #1100
Fixes #1100

Type of change

Feature addition
Please delete any options that are not relevant.

  • New feature (non-breaking change which adds functionality)
  • Other
  • This change requires a documentation update

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 tested it
  • I have commented my code, particularly in hard-to-understand areas (including JSDoc for methods)
  • My changes generates no new warnings
  • My code needed automated testing. I have added them (this is optional task)

Screenshots (if any)

Incident report history list

image
image

Incident report history button

image

@brianmiller

This comment was marked as spam.

@joebnb
Copy link

joebnb commented Jul 16, 2024

if Incident history configurable to be visible on the status page would be even better

@MasonColoretti
Copy link
Author

if Incident history configurable to be visible on the status page would be even better

definitely able to add this later on just trying to ge tthe base feature out

Copy link
Collaborator

@CommanderStorm CommanderStorm left a comment

Choose a reason for hiding this comment

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

The status page currently manages incidents.
I would prefer to leave this as a status page concept and add the history there instead.

Having this as a concept like maintenance has advantages and downsides. Without the infra needed, this is currently somewhat confusing to new users as they have likely not figured out where incidents can be created.
If you want to add this to the admin UI as well, this needs better UX from this side. (split error+empty, add a helptext that incidents can be created on status pages)

package.json Outdated
@@ -49,7 +49,7 @@
"build-docker-nightly-local": "npm run build && docker build -f docker/dockerfile -t louislam/uptime-kuma:nightly2 --target nightly .",
"build-docker-pr-test": "docker buildx build -f docker/dockerfile --platform linux/amd64,linux/arm64 -t louislam/uptime-kuma:pr-test2 --target pr-test2 . --push",
"upload-artifacts": "docker buildx build -f docker/dockerfile --platform linux/amd64 -t louislam/uptime-kuma:upload-artifact --build-arg VERSION --build-arg GITHUB_TOKEN --target upload-artifact . --progress plain",
"setup": "git checkout 1.23.13 && npm ci --production && npm run download-dist",
"setup": "npm ci --production && npm run download-dist",
Copy link
Collaborator

Choose a reason for hiding this comment

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

please revert this change. the git checkout is intentional

this.isLoading = false;
}
},
formatDate(dateString) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

async fetchIncidentReports() {
this.isLoading = true;
try {
const response = await fetch("/api/incident-reports"); // Replace with your API endpoint
Copy link
Collaborator

Choose a reason for hiding this comment

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

The rest of the admin UI uses socketio communication.
I don't see why this part of the UI should differ from that pattern => please use socketio as well for this ^^

Depending on what this page should do (main discussion), either a general handler or a dedicated handler is the way to go.

@@ -0,0 +1,87 @@
<template>
<div>
<h1>Incident Reports</h1>
Copy link
Collaborator

Choose a reason for hiding this comment

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

Currently, our transaltors could not translate this string.
Please use {{ $t("Incident Reports") }} instead and add this key to the en.json.

There are a few other cases of this as well

@CommanderStorm CommanderStorm added area:dashboard The main dashboard page where monitors' status are shown area:incidents Incidents shown in the status pages pr:please address review comments this PR needs a bit more work to be mergable labels Jul 16, 2024
@MasonColoretti
Copy link
Author

The status page currently manages incidents.

I would prefer to leave this as a status page concept and add the history there instead.

Having this as a concept like maintenance has advantages and downsides. Without the infra needed, this is currently somewhat confusing to new users as they have likely not figured out where incidents can be created.

If you want to add this to the admin UI as well, this needs better UX from this side. (split error+empty, add a helptext that incidents can be created on status pages)

ok understandable i can do all of this however im not a ui-ux designer would you be able to more in depth describe how this should look

@CommanderStorm
Copy link
Collaborator

There are a few designs in

@MasonColoretti
Copy link
Author

There are a few designs in

* [[Feature]: Incident Timeline #862](https://github.com/louislam/uptime-kuma/issues/862)

* (also in this one) [Feature Req: Multiple Incidents on Status Page #579](https://github.com/louislam/uptime-kuma/issues/579)

alot of enterprise solutions look something along the lines of this https://jira-service-management.status.atlassian.com/ would you be willing to use this or no?

@CommanderStorm
Copy link
Collaborator

alot of enterprise solutions look something

I think you are talking about the difference of having the incident history on a separate page vs on the same page but below.
I don't think that matters that much.

I would prefer that to be tucked away on a separeate page (I suspect that the regular user does not need to have this avaliable) and linked from the page, but if it is below that is fine by me.

@MasonColoretti
Copy link
Author

alot of enterprise solutions look something

I think you are talking about the difference of having the incident history on a separate page vs on the same page but below. I don't think that matters that much.

I would prefer that to be tucked away on a separeate page (I suspect that the regular user does not need to have this avaliable) and linked from the page, but if it is below that is fine by me.

sounds good i will fix it up and put it on the status page instead

@MasonColoretti
Copy link
Author

@CommanderStorm what do we think of it now... good to merge?

@MasonColoretti
Copy link
Author

whats the status of this pr im considering closing and reopening

@CommanderStorm
Copy link
Collaborator

CommanderStorm commented Aug 30, 2024

Sorry
I did not re-review this earlier, because this does not seem done.
You also did not address some of the other changes I requested. For example the request to do i18n...

Also: the API you are calling does not exist => have you tested the code?...

If you are asking about a merge or reject decision, that would be close, but things are not final.. the issues left are fixable, but require work..
I can't nurture this to health in master (among other things, because I am going on vacation next month) or fix the things that are left via code reviews.

@MasonColoretti
Copy link
Author

Sorry
I did not re-review this earlier, because this does not seem done.
You also did not address some of the other changes I requested. For example the request to do i18n...

Also: the API you are calling does not exist => have you tested the code?...

If you are asking about a merge or reject decision, that would be close, but things are not final.. the issues left are fixable, but require work..
I can't nurture this to health in master (among other things, because I am going on vacation next month) or fix the things that are left via code reviews.

ok i will fix this up again thank you very much for understanding

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:dashboard The main dashboard page where monitors' status are shown area:incidents Incidents shown in the status pages needs:resolve-merge-conflict pr:please address review comments this PR needs a bit more work to be mergable
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Is there a way to view incident history?
4 participants