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

Improve timezone information in login notification #6309

Merged
merged 6 commits into from
Jul 17, 2024

Conversation

ShiinaKin
Copy link
Contributor

@ShiinaKin ShiinaKin commented Jul 10, 2024

/area core
/kind improvement

Fixes #6256

格式化新设备登录邮件通知内的登录时间为系统时区

@f2c-ci-robot f2c-ci-robot bot added area/core Issues or PRs related to the Halo Core release-note Denotes a PR that will be considered when it comes time to generate release notes. kind/improvement Categorizes issue or PR as related to a improvement. labels Jul 10, 2024
@f2c-ci-robot f2c-ci-robot bot requested review from ruibaby and wan92hen July 10, 2024 22:41
@ShiinaKin ShiinaKin changed the title feat: convert UTC to System TimeZone feat: convert new device notication loginTime from UTC to System TimeZone Jul 10, 2024
Copy link
Member

@JohnNiang JohnNiang left a 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!

我这里有一个建议:无论用户使用哪个时区,都建议在邮件内容中包含时区信息。

@ShiinaKin
Copy link
Contributor Author

Hi @ShiinaKin , thank you for your contribution!

我这里有一个建议:无论用户使用哪个时区,都建议在邮件内容中包含时区信息。

希望呈现的是 yyyy/MM/dd hh:mm:ss UTC±N 还是 yyyy/MM/dd hh:mm:ss Asia/Shanghai

亦或者都可以呢

@JohnNiang
Copy link
Member

另外,这里的登录成功提示和单个用户相关,每个用户都应该配置自己所在的时区。直接获取系统时区似乎不太合理。

@ShiinaKin
Copy link
Contributor Author

另外,这里的登录成功提示和单个用户相关,每个用户都应该配置自己所在的时区。直接获取系统时区似乎不太合理。

直接使用UTC时间呢,只是对原生的toString进行格式化如何
image

@JohnNiang
Copy link
Member

实际上,默认输出的时间格式就是 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/

@guqing
Copy link
Member

guqing commented Jul 15, 2024

实际上,默认输出的时间格式就是 UTC,不过是 ISO-8601 格式。具体可参考:docs.oracle.com/en/java/javase/17/docs/api/java.base/java/time/Instant.html

如果允许每个用户配置自己的时区就更好了。例如 github.blog/changelog/2022-09-23-local-timezones-available-on-profiles

想复杂了

Hi @ShiinaKin ,不建议去改代码,直接在邮件通知模板中使用 ${#temporals.format(temporal, 'dd/MMM/yyyy HH:mm')} 传入需要的格式即可 参考 https://www.thymeleaf.org/doc/tutorials/3.1/usingthymeleaf.html

@ShiinaKin
Copy link
Contributor Author

ShiinaKin commented Jul 15, 2024

Hi @ShiinaKin ,不建议去改代码,直接在邮件通知模板中使用 ${#temporals.format(temporal, 'dd/MMM/yyyy HH:mm')}
传入需要的格式即可 参考 thymeleaf.org/doc/tutorials/3.1/usingthymeleaf.html

@guqing 我现在修改模板为:

   rawBody: |
        时间:[(${#temporals.format(loginTime, 'yyyy/MM/dd HH:mm:ss')})] UTC
    htmlBody: |
            <p th:text="|时间: ${#temporals.format(loginTime, 'yyyy/MM/dd HH:mm:ss')} UTC|"></p>

但该如何测试呢,触发一次事件吗

@guqing
Copy link
Member

guqing commented Jul 15, 2024

但该如何测试呢,触发一次事件吗

是的

@ShiinaKin ShiinaKin requested a review from JohnNiang July 15, 2024 08:35
@ShiinaKin ShiinaKin changed the title feat: convert new device notication loginTime from UTC to System TimeZone feat: convert new device notication loginTime from ISO8601 to customized Jul 15, 2024
Copy link

codecov bot commented Jul 15, 2024

Codecov Report

Attention: Patch coverage is 50.00000% with 2 lines in your changes missing coverage. Please review.

Project coverage is 58.29%. Comparing base (bc10336) to head (31f9e83).
Report is 78 commits behind head on main.

Files Patch % Lines
...lo/app/security/device/NewDeviceLoginListener.java 50.00% 2 Missing ⚠️
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.
📢 Have feedback on the report? Share it here.

@JohnNiang
Copy link
Member

Hi @guqing ,你可能没有理解我的意思。登录成功的消息中的时区信息应该是和用户绑定在一起的,而不是直接适用系统时间或者 UTC。目前本身就是 UTC 时间,换一个格式输出似乎没有太大的意义。

@ShiinaKin
Copy link
Contributor Author

issue #6256 主要反映的是时间的可读性
对于允许每个用户配置自己的时区,是否应该先通过一个pr在uc里添加相关设置项,再修改这里的时间,使粒度更细呢

@guqing
Copy link
Member

guqing commented Jul 16, 2024

Hi @guqing ,你可能没有理解我的意思。登录成功的消息中的时区信息应该是和用户绑定在一起的,而不是直接适用系统时间或者 UTC。目前本身就是 UTC 时间,换一个格式输出似乎没有太大的意义。

我认为当前问题只需要让日期格式根据默认时区可读就行,如果要根据用户设置的时区来需要多个 PR 的支持:

  1. 首先需要为每个用户增加个性化设置
  2. Mono<Locale> getLocaleFromSubscriber(Subscriber subscriber) {
    中增加一个类似的方法根据订阅者获取时区
  3. 需要为 temporals 表达式的创建传入时区参数,
    https://github.com/thymeleaf/thymeleaf/blob/db314973254ca8d9ee8661cf13c680b93349ee59/lib/thymeleaf/src/main/java/org/thymeleaf/standard/expression/StandardExpressionObjectFactory.java#L223-L225
    目前 thymeleaf 只从 context 中获取了 Locale,时区使用的 jvm 默认时区,但表达式对象是支持传入时区的
    https://github.com/thymeleaf/thymeleaf/blob/db314973254ca8d9ee8661cf13c680b93349ee59/lib/thymeleaf/src/main/java/org/thymeleaf/expression/Temporals.java#L60
    ,因此期望给 thymeleaf 提 issue 让其支持读取 context 中的时区,或者考虑到 thymeleaf 的活跃度可以提供一个自己的 Temporals 类来支持

Copy link
Member

@JohnNiang JohnNiang left a 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

@f2c-ci-robot f2c-ci-robot bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jul 16, 2024
@ShiinaKin ShiinaKin requested a review from guqing July 16, 2024 05:25
Copy link
Member

@JohnNiang JohnNiang left a 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

@f2c-ci-robot f2c-ci-robot bot changed the title feat: convert new device notication loginTime from ISO8601 to customized Improve timezone information in login notification Jul 16, 2024
Copy link
Member

@JohnNiang JohnNiang left a 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

Copy link
Member

@JohnNiang JohnNiang left a 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!

@f2c-ci-robot f2c-ci-robot bot added the lgtm Indicates that a PR is ready to be merged. label Jul 16, 2024
@guqing
Copy link
Member

guqing commented Jul 16, 2024

/hold

@f2c-ci-robot f2c-ci-robot bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jul 16, 2024
@f2c-ci-robot f2c-ci-robot bot removed the lgtm Indicates that a PR is ready to be merged. label Jul 17, 2024
Copy link
Member

@JohnNiang JohnNiang left a comment

Choose a reason for hiding this comment

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

/lgtm

@f2c-ci-robot f2c-ci-robot bot added the lgtm Indicates that a PR is ready to be merged. label Jul 17, 2024
Copy link

f2c-ci-robot bot commented Jul 17, 2024

[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 /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@ruibaby
Copy link
Member

ruibaby commented Jul 17, 2024

/unhold

@f2c-ci-robot f2c-ci-robot bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jul 17, 2024
@f2c-ci-robot f2c-ci-robot bot merged commit 2a807b7 into halo-dev:main Jul 17, 2024
7 checks passed
@ruibaby ruibaby added this to the 2.18.0 milestone Jul 29, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. area/core Issues or PRs related to the Halo Core kind/improvement Categorizes issue or PR as related to a improvement. lgtm Indicates that a PR is ready to be merged. release-note Denotes a PR that will be considered when it comes time to generate release notes.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

用户登录邮件时间异常
4 participants