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: UTC time to active importers #70

Merged
merged 8 commits into from
Jun 26, 2024

Conversation

gildub
Copy link
Contributor

@gildub gildub commented Jun 24, 2024

Add /src/app/dayjs.ts with common options
Add UTC time to importers
Add formatDateTime function

Fix #68

Screenshot from 2024-06-24 15-22-21

@gildub gildub requested a review from carlosthe19916 June 24, 2024 13:29
@gildub gildub self-assigned this Jun 24, 2024
Copy link
Collaborator

@helio-frota helio-frota left a comment

Choose a reason for hiding this comment

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

lgtm

Copy link
Member

@carlosthe19916 carlosthe19916 left a comment

Choose a reason for hiding this comment

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

Minor comments, please take a look

client/src/app/dayjs.ts Outdated Show resolved Hide resolved
configValues?.disabled == false &&
dayjs(item.report?.startDate)
.utc()
.format("YYYY-MM-DD HH:mm:ss")}
Copy link
Member

Choose a reason for hiding this comment

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

Just an idea:

  • The current code in the main branch has a function formatDate() that can be reused everywhere.
  • what about having something like formatDateTime so we can reuse it too?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@carlosthe19916,

Well I suppose this a question of reuse vs performance.
I'm not against a format function here but what's do we gain as the call to dayjs format is straightforward here.

Copy link
Member

Choose a reason for hiding this comment

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

Having formats in a single place will help us to change the formats in a single shot when needed. So far this is the only place where we use this format but in the case of formatDate if UX decides the format needs to change We will need to only change that function and the whole app will have the desired format rather than changing .format(ABCAD) in every single file

@helio-frota
Copy link
Collaborator

main branch has a function formatDate() that can be reused everywhere.

The current function export const formatDate = (value: Date, includeTime = true) is unused and using Intl.DateTimeFormat

trustify-ui git:(main) rg formatDate -A2
client/src/app/utils/utils.ts
32:export const formatDate = (value: Date, includeTime = true) => {
33-  const timeZone = Intl.DateTimeFormat().resolvedOptions().timeZone;
34-  const dateOptions: Intl.DateTimeFormatOptions = {

We can see a good amount of dayjs usage without calling a specific function

client/src/app/pages/vulnerability-list/vulnerability-list.tsx
177:                        {/* {dayjs(item.date_discovered).format(RENDER_DATE_FORMAT)} */}
185:                        {/* {dayjs(item.date_discovered).format(RENDER_DATE_FORMAT)} */}

client/src/app/pages/vulnerability-details/vulnerability-details.tsx
26:import dayjs from "dayjs";
100:                    dayjs(vulnerability.published).format(RENDER_DATE_FORMAT)}
109:                    dayjs(vulnerability.modified).format(RENDER_DATE_FORMAT)}

client/src/app/pages/vulnerability-details/related-sboms.tsx
4:import dayjs from "dayjs";
135:                    {dayjs(item.published).format(RENDER_DATE_FORMAT)}

client/src/app/pages/vulnerability-details/related-advisories.tsx
4:import dayjs from "dayjs";
123:                    {dayjs(item.published).format(RENDER_DATE_FORMAT)}
130:                    {dayjs(item.modified).format(RENDER_DATE_FORMAT)}

client/src/app/pages/importer-list/importer-list.tsx
56:import dayjs from "dayjs";
241:                            dayjs(item.report.startDate).format(
251:                            dayjs(item.report.endDate).format(

client/src/app/pages/advisory-list/advisory-list.tsx
5:import dayjs from "dayjs";
212:                        {dayjs(item.published).format(RENDER_DATE_FORMAT)}
219:                        {dayjs(item.modified).format(RENDER_DATE_FORMAT)}

client/src/app/pages/sbom-list/sbom-list.tsx
4:import dayjs from "dayjs";
202:                        {dayjs(item.published).format(RENDER_DATE_FORMAT)}

client/src/app/pages/advisory-details/vulnerabilities.tsx
4:import dayjs from "dayjs";
54:      published: vuln ? dayjs(vuln.published).millisecond() : 0,
55:      modified: vuln ? dayjs(vuln.modified).millisecond() : 0,
146:                        dayjs(item.published).format(RENDER_DATE_FORMAT)} */}
151:                        dayjs(item.modified).format(RENDER_DATE_FORMAT)} */}

client/src/app/pages/advisory-details/overview.tsx
17:import dayjs from "dayjs";
46:                        {dayjs(advisory.published).format(RENDER_DATE_FORMAT)}
52:                        {dayjs(advisory.modified).format(RENDER_DATE_FORMAT)}
131:                        {dayjs(
141:                        {dayjs(

client/src/app/pages/package-details/related-cves.tsx
4:import dayjs from "dayjs";
128:                    {dayjs(item.published).format(RENDER_DATE_FORMAT)}

client/src/app/pages/sbom-details/overview.tsx
13:import dayjs from "dayjs";
58:              {dayjs(sbom.published).format(RENDER_DATE_FORMAT)}

client/src/app/pages/sbom-details/cves.tsx
3:import dayjs from "dayjs";
144:                    {dayjs(item.published).format(RENDER_DATE_FORMAT)}

I think that makes more sense:

  • Delete the unused formatDate from client/src/app/utils/utils.ts
  • Merge this PR
  • Refactor later in future PRs if the idea is to have a central function to format dates.

@carlosthe19916
Copy link
Member

@helio-frota your report is not accurate. Pull the latest changes of the main branch

@helio-frota
Copy link
Collaborator

@carlosthe19916 that's correct sorry for the noise, the function is being used and using dayjs.

in this case I'm +1 having a formatDateTime

@helio-frota helio-frota self-requested a review June 26, 2024 11:39
Copy link
Collaborator

@helio-frota helio-frota left a comment

Choose a reason for hiding this comment

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

There is a function inside client/src/app/utils/utils.ts that makes use of dayjs already.

export const formatDate = (value?: string) => {
return value ? dayjs(value).format(RENDER_DATE_FORMAT) : undefined;
};

so I'm +1 to @carlosthe19916 suggestion

@gildub
Copy link
Contributor Author

gildub commented Jun 26, 2024

@carlosthe19916, @helio-frota, thanks for your help.
Could you please review latest which contains the formatDateTime function ?

Comment on lines 133 to 135
{!!item.non_normative.discovered
? formatDate(item.non_normative.discovered)
: null}
Copy link
Member

Choose a reason for hiding this comment

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

why the unnecessary change of this block of code? what problem are we solving with this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@carlosthe19916, It's because I've made changes to https://github.com/trustification/trustify-ui/pull/70/files#diff-93d1e679f3a44569699332c49a4e0cf4e30ada958d50d897b3764cf2e20dd6f7R37-R39 where it makes more sense to not return undefined and therefore check for such case before calling formatDate.

Copy link
Member

Choose a reason for hiding this comment

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

  • Sorry but this change is replacing a simple line of code with a complex conditional longer code. This is is a step back in my opinion.
  • What specific problem is this change solving that the previous code was not covering?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@carlosthe19916,

formatDate(item.non_normative.discovered) what happens when formatDate returns undefined ?
React might break in such case and what should be done is to trap such case and return null.

Copy link
Member

Choose a reason for hiding this comment

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

  • formatDate is already returning undefined in multiple cases.
const myVar = formatDate(undefined); // Then myVar will be undefined

<>
{myVar}
</>

How is the previous code breaking reactjs? Can you give me a real concrete example and not suppositions?

  • I think the case is simple: Case1 vs Case2, let's explain why:
{!!item.non_normative.discovered
                        ? formatDate(item.non_normative.discovered)
                        : null}

is better than

{formatDate(item.non_normative.released)}

Why if item.non_normative.released is undefined it will break the code?

  • Couldn't we just do the following?:
export const formatDate = (value?: string) => {
  return value ? dayjs(value).format(RENDER_DATE_FORMAT) : null;
};

we are returning null as you want.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@carlosthe19916 , sorry for the confusion, what I mean by breaking the code is not reactjs friendly when the code renders {undefined}.
Well it seems that React 18 handles undefined better. But that shouldn't really happen anyway :
https://blog.saeloun.com/2022/04/14/react-18-allows-components-to-render-undfined/

Let's use your latest example and make sure formatDate and formatDateTime return null if undefined.

@gildub gildub requested a review from carlosthe19916 June 26, 2024 14:52
@carlosthe19916 carlosthe19916 merged commit ad16201 into trustification:main Jun 26, 2024
1 check passed
@gildub gildub deleted the show-time branch June 27, 2024 07:19
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.

Show time for importer runs
3 participants