-
-
Notifications
You must be signed in to change notification settings - Fork 9.8k
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
Improve timezone information in login notification #6309
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @ShiinaKin , thank you for your contribution!
我这里有一个建议:无论用户使用哪个时区,都建议在邮件内容中包含时区信息。
希望呈现的是 亦或者都可以呢 |
另外,这里的登录成功提示和单个用户相关,每个用户都应该配置自己所在的时区。直接获取系统时区似乎不太合理。 |
实际上,默认输出的时间格式就是 UTC,不过是 ISO-8601 格式。具体可参考:https://docs.oracle.com/en/java/javase/17/docs/api/java.base/java/time/Instant.html#toString()。 如果允许每个用户配置自己的时区就更好了。例如 https://github.blog/changelog/2022-09-23-local-timezones-available-on-profiles/。 |
想复杂了 Hi @ShiinaKin ,不建议去改代码,直接在邮件通知模板中使用 |
@guqing 我现在修改模板为:
但该如何测试呢,触发一次事件吗 |
是的 |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #6309 +/- ##
============================================
+ Coverage 54.51% 58.29% +3.78%
- Complexity 3523 3764 +241
============================================
Files 646 646
Lines 21862 21978 +116
Branches 1528 1545 +17
============================================
+ Hits 11917 12813 +896
+ Misses 9328 8546 -782
- Partials 617 619 +2 ☔ View full report in Codecov by Sentry. |
Hi @guqing ,你可能没有理解我的意思。登录成功的消息中的时区信息应该是和用户绑定在一起的,而不是直接适用系统时间或者 UTC。目前本身就是 UTC 时间,换一个格式输出似乎没有太大的意义。 |
issue #6256 主要反映的是时间的可读性 |
我认为当前问题只需要让日期格式根据默认时区可读就行,如果要根据用户设置的时区来需要多个 PR 的支持:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Make sense to me.
/approve
application/src/main/resources/extensions/notification-templates.yaml
Outdated
Show resolved
Hide resolved
application/src/main/resources/extensions/notification-templates.yaml
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/approve
/retitle Improve timezone information in login notification
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/ping @halo-dev/sig-halo
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/lgtm
Thank @ShiinaKin for your contribution!
/hold |
application/src/main/resources/extensions/notification-templates.yaml
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/lgtm
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: JohnNiang The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
/unhold |
/area core
/kind improvement
Fixes #6256