Fix(notification-aliyun-sms): throw error when sending SMS failed

[polunzh]

Tick the checkbox if you understand [x]:

• I have read and understand the pull request rules.

Description

Throw error when sending Aliyun SMS failed, snapshot below.

Type of change

Please delete any options that are not relevant.

• Bug fix (non-breaking change which fixes an issue)

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 (if any)

Please do not use any external image service. Instead, just paste in or drag and drop the image here, and it will be uploaded automatically.

[CommanderStorm]

I don't quite know why you opened an empty PR (what would you like to discuss?)

Currently the alium-sms notification provider checks for success like this:

uptime-kuma/server/notification-providers/aliyun-sms.js

Lines 73 to 76 in 72741eb

	if (result.data.Message === "OK") {
	return true;
	}
	return false;

[polunzh]

I don't quite know why you opened an empty PR (what would you like to discuss?)

Currently the alium-sms notification provider checks for success like this:

uptime-kuma/server/notification-providers/aliyun-sms.js

Lines 73 to 76 in 72741eb

	if (result.data.Message === "OK") {
	return true;
	}
	return false;

Sorry, I updated the PR.

Original bugs:

• The method this.sendSms() is an async function, so it must be called with await, or it will always be true:
  

  uptime-kuma/server/notification-providers/aliyun-sms.js

  Line 21 in 72741eb

  if (this.sendSms(notification, msgBody)) {

• The method should throw error if response message is not OK instead of return false.

[CommanderStorm]

The same problem (not awaiting the success-determiner) is also be present in these other poviders due to other devs copying code:

• DingDing
• FlashDuty
• PagerDuty
• PagerTree
• Splunk

Could you attach a fix for these as well?

If you do, could you add Resolves #3458 to the description?

[louislam]

The same problem (not awaiting the success-determiner) is also be present in these other poviders due to other devs copying code:

* `DingDing`

* `FlashDuty`

* `PagerDuty`

* `PagerTree`

* `Splunk`

Could you attach a fix for these as well?

If you do, could you add Resolves #3458 to the description?

Maybe do this in new pull request(s). Some of them are regional only notifications, I can't test the other normal cases. Since notification is an important feature in Uptime Kuma, it is required to find someone who actually can test it (up/down/test ok/test fail). Maybe we can reach back the og authors to test them.

[CommanderStorm]

While I don't agree that these changes require extra tests, let's ask the OG authors if they can duplicate+test these fixes across the other providers:

• DingDing: @AnnAngela @dingdayu -> fix(notification-dingding): throw error when failed #3598
• FlashDuty: @guguji5
• PagerDuty: @MarcHagen
• PagerTree: @armiiller
• Splunk: @Joseph-Irving

Currently these monitors don't report erros at all (they always succeed).
Could each of you please add:

• an await statement to the function which your provider does not await
• throwing errors instead of returning false to make them appear in the frontend

and document that these are fixed by attaching a screenshot as above of the monitor failing?

[AnnAngela]

While I don't agree that these changes require extra tests, let's ask the OG authors if they can duplicate+test these fixes across the other providers:

• DingDing: @AnnAngela @dingdayu
• FlashDuty: @guguji5
• PagerDuty: @MarcHagen
• PagerTree: @armiiller
• Splunk: @Joseph-Irving

Currently these monitors don't report erros at all (they always succeed).
Could each of you please add:

• an await statement to the function which your provider does not await
• throwing errors instead of returning false to make them appear in the frontend

and document that these are fixed by attaching a screenshot as above of the monitor failing?

I am not the author of Dingding provider (I only changed the timezone setting of output message), but if @dingdayu don't have time, I can take a look at thursday.

[polunzh]

The same problem (not awaiting the success-determiner) is also be present in these other poviders due to other devs copying code:

• DingDing
• FlashDuty
• PagerDuty
• PagerTree
• Splunk

Could you attach a fix for these as well?

If you do, could you add Resolves #3458 to the description?

Sorry, I can't promise to fix all of these notification issues, but I'll take a look over the weekend if it hasn't been fixed yet.

[guguji5]

@CommanderStorm Sorry to see this issue so late, is there any problem to fix the FlashDuty channel ? or fixed

[AnnAngela]

@CommanderStorm @louislam I have made a pr #3598