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

Use template_card instead of text messages for the WeCom notification provider #4516

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

Conversation

darkclip
Copy link

@darkclip darkclip commented Feb 22, 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

add markdown message support for WeCom notification provider

Type of change

  • 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 generates no new warnings
  • My code needed automated testing. I have added them (this is optional task)

Screenshots (if any)

Event before after
UP before_up after_up
DOWN before_down after_down
Cert-expiry before_expire after_expire
Testing before_testing after_testing

CommanderStorm

This comment was marked as resolved.

@CommanderStorm CommanderStorm marked this pull request as draft February 22, 2024 11:35
@darkclip darkclip marked this pull request as ready for review February 22, 2024 12:13
CommanderStorm

This comment was marked as resolved.

@darkclip
Copy link
Author

darkclip commented Feb 22, 2024

This msgtype is not one which is officially recognised by the api as far as I see it. Am I looking at the wrong docs?

Maybe the English version is outdated. It's documented in this Chinese one:
https://developer.work.weixin.qq.com/document/path/91770

@darkclip

This comment was marked as resolved.

@darkclip darkclip changed the title add markdown message support for WeCom notification provider add template_card message support for WeCom notification provider Feb 22, 2024
@CommanderStorm
Copy link
Collaborator

CommanderStorm commented Feb 22, 2024

Agree, much nicer.
From a UX perspective I feel like switching these two fieds might be benefitial.
What do you think?

image

@CommanderStorm
Copy link
Collaborator

CommanderStorm commented Feb 22, 2024

Another thing that I forgot in the second review (not adding this would also be fine ^^).
Do you think the call-to-action buttons as in buildActions from #3886 might be possible?

@darkclip
Copy link
Author

darkclip commented Feb 22, 2024

From a UX perspective I feel like switching these two fieds might be benefitial. What do you think?

The reason for this arrangement is that this way a quick status will show up in brief view. Other wise you have to click into that message to see the status. Or we can just discard that most up title.

brief

@darkclip
Copy link
Author

Another thing that I forgot in the second review (not adding this would also be fine ^^). Do you think the call-to-action buttons as in buildActions from #3886 might be possible?

There's already a builtin call-to-action function, you click the message it will jump to the monitor url. I just did not find a quick way to get the site url.

@darkclip
Copy link
Author

without the title bar

IMG_2882

@CommanderStorm CommanderStorm changed the title add template_card message support for WeCom notification provider Use template_card instead of text messages for the WeCom notification provider Feb 22, 2024
@CommanderStorm CommanderStorm added the area:notifications Everything related to notifications label Feb 22, 2024
@darkclip
Copy link
Author

I think it looks better without the title bar. Push the change.

@CommanderStorm
Copy link
Collaborator

  • There's already a builtin call-to-action function, you click the message it will jump to the monitor URL.

    I can't read the text, but could/should these buttons be used to link to the different sites?
    image

    See teams (Refactor MS-Teams notification to use AdaptiveCards #4538) for an example in another notification provider
    newUp

    I just did not find a quick way to get the site URL.

    adding the following to server/notification-providers/notification-provider.js should do the trick

    /**
       * Extract the url from the monitor
       * @param {object|null} monitorJSON Monitor details (For Up/Down/Cert-Expiry only)
       * @returns {string|undefined} the address/url/hostname of the monitor
       */
      extractURL(monitorJSON) {
          if (monitorJSON === null) {
              return undefined;
          }
          switch (monitorJSON["type"]) {
              case "ping":
                  return monitorJSON["hostname"];
              case "docker":
                  return monitorJSON["docker_host"];
              case "port":
              case "dns":
              case "gamedig":
              case "steam":
                  if (monitorJSON["port"]) {
                      return monitorJSON["hostname"] + ":" + monitorJSON["port"];
                  }
                  return monitorJSON["hostname"];
              default:
                  if (monitorJSON["url"] !== "https://") {
                      return monitorJSON["url"];
                  }
                  return undefined;
          }
      }

  • could you update the screenshots in the description to make sure that there has not been a regression that we missed?

@CommanderStorm CommanderStorm marked this pull request as draft March 7, 2024 16:25
@darkclip
Copy link
Author

darkclip commented Mar 7, 2024

@CommanderStorm
yes, those buttons could be added as a link, they are called jump_list. The thing is the builtin jump URL is mandatory, in case of no link present, the whole message just wouldn't be sent. The same goes for jump_list. That's my last commit is for. I could add other entries to jump_list, but if there's no URL to be retrieved and it's not properly tested against all edge cases, that's just another failure point. That's the reason I first try to use markdown instead of template_card.

@CommanderStorm
Copy link
Collaborator

CommanderStorm commented Mar 7, 2024

I think that handling is wrong.
In case the url does not exist, the whole card_action should not be added.
The same goes for jump_list: in case no links exist, don't add the list.

Going by the docs, that should work.

@darkclip
Copy link
Author

darkclip commented Mar 7, 2024

I actually agree with you, but as I said, card_action is mandatory for template_card, and URL is mandatory for card_action. I personally think that's pretty stupid, but that's the way of one of the largest tech companies around here.

Co-authored-by: Frank Elsinga <frank@elsinga.de>
@github-actions github-actions bot added needs:resolve-merge-conflict A merge-conflict needs to be addressed before reviewing makes sense again and removed needs:resolve-merge-conflict A merge-conflict needs to be addressed before reviewing makes sense again labels May 19, 2024
@CommanderStorm
Copy link
Collaborator

I just went over the discussion to look if there is something left open and two things are currently blocking a merge:

  • I think this part fell under the table. Is this intentional?

    The same goes for jump_list: in case no links exist, don't add the list.
    Going by the docs, that should work

  • there have been some other changes to wechat in the master branch. could you merge these changes into your PR?

@CommanderStorm CommanderStorm added needs:work this PR needs work and removed needs:review this PR needs a review by maintainers or other community members labels May 23, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:notifications Everything related to notifications needs:resolve-merge-conflict A merge-conflict needs to be addressed before reviewing makes sense again needs:work this PR needs work
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants