Skip to content
New issue

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

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

Already on GitHub? # to your account

Display discords timestamps via <:t:timestamp_unix:f> #3719

Closed

Conversation

LifeIsAParadox
Copy link

⚠️⚠️⚠️ 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

Fixes #543

Type of change

Please delete any options that are not relevant.

  • Other
    use Discord timestamp format

https://discord.com/developers/docs/reference#message-formatting-formats

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)

grafik

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.

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.

First of all:

  • please give the PR a descriptive title
  • please actually run the Checklist (not only checking the boxes) ^^

About the functionality:
This kind of undermines the ability to set the timezone of the uptime server and instead hands that off to discord.

  • The problem I see: is this transparent to the users or do we need to change this from "Time" to something like "Time (client-locale)" (I am not super happy with that as well)
  • I have attached two comments which fix the formatting issues raised by check-linters
  • Could you attach a before and after screenshot?
  • Have you made sure that the code you wrote and the previos code is identical?
    From what I looked at, the time of the heartbeatJSON and the current time might be different. Could you use the timestamp all other notification providers use as well?
    The variable you are replacing gets created here:
    heartbeatJSON["timezone"] = await UptimeKumaServer.getInstance().getTimezone();
    heartbeatJSON["timezoneOffset"] = UptimeKumaServer.getInstance().getTimezoneOffset();
    heartbeatJSON["localDateTime"] = dayjs.utc(heartbeatJSON["time"]).tz(heartbeatJSON["timezone"]).format(SQL_DATETIME_FORMAT);

    As I read this code (and the lines previosly) "time" might be slightly different from the time of notification.
    Could you use the same time-reference as well?

server/notification-providers/discord.js Outdated Show resolved Hide resolved
server/notification-providers/discord.js Outdated Show resolved Hide resolved
@louislam
Copy link
Owner

Should convert the time from heartbeatJSON["time"], not Date.now()

@louislam louislam added the question Further information is requested label Sep 10, 2023
use heartbeatJSON["time"] instead of Date.now()
@LifeIsAParadox
Copy link
Author

LifeIsAParadox commented Sep 12, 2023

Please excuse the wrong format, I didn't use ESLint properly, but I've fixed it now.

Here is the before/after pictures:

before after
image image

@LifeIsAParadox
Copy link
Author

Should convert the time from heartbeatJSON["time"], not Date.now()

changed Date.now() to heartbeatJSON["time"]

@louislam louislam added this to the 2.0.0 milestone Oct 8, 2023
@louislam louislam added question Further information is requested and removed question Further information is requested labels Oct 16, 2023
@louislam
Copy link
Owner

I think the time is incorrect

I input <t:1697527649:f> manually, it could show the correct local datetime.

image

@louislam louislam modified the milestones: 2.0.0, 2.1.0 Nov 23, 2023
@CommanderStorm CommanderStorm marked this pull request as draft February 4, 2024 11:34
@CommanderStorm
Copy link
Collaborator

CommanderStorm commented Feb 4, 2024

@LifeIsAParadox can you explain this difference? (I have not looked into this, but it seems a bit strange)

@CommanderStorm CommanderStorm added area:notifications Everything related to notifications type:enhance-existing feature wants to enhance existing monitor help wanted May need your help to test or answer pr:needs review this PR needs a review by maintainers or other community members pr:please address review comments this PR needs a bit more work to be mergable labels May 19, 2024
@CommanderStorm CommanderStorm changed the title Update notification-provider discord.js Display discords timestamps via <:t:timestamp_unix:f> Jun 3, 2024
@CommanderStorm
Copy link
Collaborator

@LifeIsAParadox
Thanks for your work.
Given that louis has not gotten this to work correctly and there has not been any activity after that, I am going to close this PR.
If something changes (f.ex. if the reproduction of the feature starts working because Discord fixed a bug) we can reopen the PR. ^^

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
area:notifications Everything related to notifications help wanted May need your help to test or answer pr:needs review this PR needs a review by maintainers or other community members pr:please address review comments this PR needs a bit more work to be mergable question Further information is requested type:enhance-existing feature wants to enhance existing monitor
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Suggestion] Change time of Discord notification
3 participants