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

Add Flashduty notification #3475

Merged
merged 10 commits into from
Jul 31, 2023
Merged

Add Flashduty notification #3475

merged 10 commits into from
Jul 31, 2023

Conversation

guguji5
Copy link
Contributor

@guguji5 guguji5 commented Jul 24, 2023

Tick the checkbox if you understand [x]:

  • I have read and understand the pull request rules.

Description

Add notification provider of Flashduty (which is a Saas monitor noise reduction platform in China.)

Type of change

Please delete any options that are not relevant.

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

Screenshots



src/components/notifications/FlashDuty.vue Outdated Show resolved Hide resolved
src/components/notifications/FlashDuty.vue Outdated Show resolved Hide resolved
src/components/notifications/FlashDuty.vue Outdated Show resolved Hide resolved
server/notification-providers/flashduty.js Show resolved Hide resolved
server/notification-providers/flashduty.js Outdated Show resolved Hide resolved
server/notification-providers/flashduty.js Outdated Show resolved Hide resolved
server/notification-providers/flashduty.js Outdated Show resolved Hide resolved
server/notification-providers/flashduty.js Outdated Show resolved Hide resolved
server/notification-providers/flashduty.js Outdated Show resolved Hide resolved
server/notification-providers/flashduty.js Outdated Show resolved Hide resolved
@CommanderStorm
Copy link
Collaborator

Could you also add a screenshot of the notification provider working? ^^
(This is standard procedure for regional notification providers, as testing them is sadly basically impossible due to region locks and language bariers)

Copy link
Collaborator

@chakflying chakflying left a comment

Choose a reason for hiding this comment

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

Missing space.

src/components/NotificationDialog.vue Outdated Show resolved Hide resolved
@guguji5
Copy link
Contributor Author

guguji5 commented Jul 25, 2023

Could you also add a screenshot of the notification provider working? ^^ (This is standard procedure for regional notification providers, as testing them is sadly basically impossible due to region locks and language bariers)

Because of the network, may I use the screenshot hosted by imgur? (Currently, most of Flashduty users are in China, so the screenshot has some Chinese words.)

@guguji5 guguji5 requested a review from CommanderStorm July 25, 2023 06:54
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.

about the screenshots: Could you attach a screenshot showing how the Test button looks when pressed?
(I am a bit concearned that specifing it as a url => using it as a resource in postNotification might not work)

server/notification-providers/flashduty.js Outdated Show resolved Hide resolved
server/notification-providers/flashduty.js Outdated Show resolved Hide resolved
server/notification-providers/flashduty.js Outdated Show resolved Hide resolved
@guguji5
Copy link
Contributor Author

guguji5 commented Jul 26, 2023

about the screenshots: Could you attach a screenshot showing how the Test button looks when pressed? (I am a bit concearned that specifing it as a url => using it as a resource in postNotification might not work)

Yep, you are right. there is a bug when fire a 'Test' request. I fix it by adding '?' before the monitorInfo (which means when the monitorInfo is undefined, abandon the logic behind and return undefined.)


@guguji5 guguji5 requested a review from CommanderStorm July 26, 2023 02:42
@guguji5 guguji5 requested a review from chakflying July 27, 2023 06:09
Copy link
Collaborator

@chakflying chakflying left a comment

Choose a reason for hiding this comment

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

I think including zh-HK and zh-TW translation is unnecessary. Otherwise looks okay.

@guguji5
Copy link
Contributor Author

guguji5 commented Jul 28, 2023

@CommanderStorm is it ready to merge this PR ? or anything to improve ?

@CommanderStorm
Copy link
Collaborator

As indicated by my aproval-review:
from my point, no, but only louis vote counts. See our contribution guide: https://github.com/louislam/uptime-kuma/blob/master/CONTRIBUTING.md
The point of reviews from me and Nelson is to reduce unnessesary go-arounds from Louis, not to give any indicaor of timeline/mergability/...

@guguji5
Copy link
Contributor Author

guguji5 commented Jul 28, 2023

As indicated by my aproval-review: from my point, no, but only louis vote counts. See our contribution guide: https://github.com/louislam/uptime-kuma/blob/master/CONTRIBUTING.md The point of reviews from me and Nelson is to reduce unnessesary go-arounds from Louis, not to give any indicaor of timeline/mergability/...

thank you for your reply. I will wait louis's feedback.

@louislam louislam added this to the 1.23.0 milestone Jul 31, 2023
@louislam louislam added the question Further information is requested label Jul 31, 2023
@louislam
Copy link
Owner

I think including zh-HK and zh-TW translation is unnecessary. Otherwise looks okay.

Same thought, just need to change zh-CN.json, as the others are using Traditional Chinese.

Other code changes look good to me.

@guguji5
Copy link
Contributor Author

guguji5 commented Jul 31, 2023

I think including zh-HK and zh-TW translation is unnecessary. Otherwise looks okay.

Same thought, just need to change zh-CN.json, as the others are using Traditional Chinese.

Other code changes look good to me.

@chakflying @louislam ok, I revert the changes in zh-HK and zh-TW and fix the conflicts in en.json

@louislam louislam removed the question Further information is requested label Jul 31, 2023
@louislam louislam merged commit 587d9e4 into louislam:master Jul 31, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants