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

fix(i18n): compatible with timestamp in milliseconds or null values #1775

Open
wants to merge 8 commits into
base: main
Choose a base branch
from

Conversation

Justineo
Copy link
Contributor

@Justineo Justineo commented Nov 11, 2024

Summary

The filter API is returning timestamps in milliseconds instead of seconds and null as updated_at values for those weren't modified since created. The issue affects both filtered services and routes.

Before:

image

After:

image

@Justineo Justineo enabled auto-merge (squash) November 11, 2024 17:15
@Justineo Justineo requested a review from a team as a code owner November 12, 2024 06:55
const invalidDate = 'Invalid Date'
if (!timestamp) {
return invalidDate
return '-'
Copy link
Collaborator

Choose a reason for hiding this comment

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

not saying this is wrong change, but would some places where user expects to see Invalid Date and now sees - be "broken" or "changed without the user's content" ? Also are we sure there are no tests that expects Invalid Date ?
Seems that the change is not related to shameful normalize and can be excluded from this PR, unless it's absolutely needed?

Copy link
Contributor Author

@Justineo Justineo Nov 12, 2024

Choose a reason for hiding this comment

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

I can revert this change, as it was intended to handle potential null values returned by backend APIs, and we are already providing the correct fallback value (use created_at if updated_at is null). I’m just curious to understand if there are any scenarios in our current design where we might expect to see Invalid Date.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't see this string anywhere in the tests. So looks like safe change. If someone complains we just file JIRA and fix :)

Copy link
Contributor Author

@Justineo Justineo Nov 13, 2024

Choose a reason for hiding this comment

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

Sounds good. Can you stamp this one?

const invalidDate = 'Invalid Date'
if (!timestamp) {
return invalidDate
return '-'
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
return '-'
return ''

Can we use emdash instead please?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think I should just remove this. We are already handling null before this.

@@ -52,6 +52,19 @@ export const createI18n = <MessageSource extends Record<string, any>>
const { $t, ...otherProps } = intlOriginal
const intl = otherProps

/**
* Shamefully normalize a timestamp to be in seconds as some APIs are returning timestamps in milliseconds
* TODO: Remove this function once all timestamps are normalized from the backend
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ticket for the backend: MA-3412

@Justineo
Copy link
Contributor Author

I've updated the code and no longer handles null value when formatting the timestamp. PTAL @ValeryG @kaiarrowood

@kongponents-bot
Copy link
Collaborator

Preview components from this PR in consuming application

In consuming application project install preview versions of shared packages generated by this PR:

@kong-ui-public/i18n@pr-1775
@kong-ui-public/analytics-chart@pr-1775
@kong-ui-public/analytics-geo-map@pr-1775
@kong-ui-public/analytics-metric-provider@pr-1775
@kong-ui-public/forms@pr-1775
@kong-ui-public/misc-widgets@pr-1775
@kong-ui-public/entities-shared@pr-1775
@kong-ui-public/document-viewer@pr-1775
@kong-ui-public/spec-renderer@pr-1775
@kong-ui-public/dashboard-renderer@pr-1775
@kong-ui-public/documentation@pr-1775
@kong-ui-public/expressions@pr-1775
@kong-ui-public/entities-certificates@pr-1775
@kong-ui-public/entities-consumer-credentials@pr-1775
@kong-ui-public/entities-consumer-groups@pr-1775
@kong-ui-public/entities-consumers@pr-1775
@kong-ui-public/entities-data-plane-nodes@pr-1775
@kong-ui-public/entities-gateway-services@pr-1775
@kong-ui-public/entities-key-sets@pr-1775
@kong-ui-public/entities-keys@pr-1775
@kong-ui-public/entities-snis@pr-1775
@kong-ui-public/entities-upstreams-targets@pr-1775
@kong-ui-public/entities-vaults@pr-1775
@kong-ui-public/entities-routes@pr-1775
@kong-ui-public/entities-plugins@pr-1775

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants