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.

Sign up for GitHub

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: [M3-8451] - List events in event landing page in chronological order #11339

Open
wants to merge 4 commits into
base: develop
Choose a base branch
from
+110 −7
Open

fix: [M3-8451] - List events in event landing page in chronological order #11339

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
e0b4ed1
change: [M3-8451] - List events in a chronological order
harsh-akamai Nov 27, 2024
a476c28
code clean up
harsh-akamai Nov 28, 2024
65450bb
Added changeset: Events landing page lists events in wrong order
harsh-akamai Nov 28, 2024
e115ab9
improved unit test coverage
harsh-akamai Nov 29, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions packages/manager/.changeset/pr-11339-fixed-1732776925827.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
"@linode/manager": Fixed
---

Events landing page lists events in wrong order ([#11339](https://github.com/linode/manager/pull/11339))
8 changes: 7 additions & 1 deletion packages/manager/src/features/Events/EventRow.test.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
import * as React from 'react';

import { eventFactory } from 'src/factories';
import { getEventTimestamp } from 'src/utilities/eventUtils';
import {
renderWithTheme,
resizeScreenSize,
Expand All @@ -9,6 +10,7 @@ import {

import { EventRow } from './EventRow';

import type { completionEvent } from './EventRow';
import type { Event } from '@linode/api-v4/lib/account';

describe('EventRow', () => {
Expand All @@ -17,11 +19,15 @@ describe('EventRow', () => {
status: 'notification',
username: 'test_user',
});
const completionEvent: completionEvent = {
...mockEvent,
completionTime: getEventTimestamp(mockEvent),
};

it('displays the correct data', () => {
resizeScreenSize(1600);
const { getByRole } = renderWithTheme(
wrapWithTableBody(<EventRow event={mockEvent} />)
wrapWithTableBody(<EventRow event={completionEvent} />)
);

expect(
Expand Down
10 changes: 6 additions & 4 deletions packages/manager/src/features/Events/EventRow.tsx
View file
Open in desktop
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've extended the Event interface to include completionTime for an event and moved the getEventTimeStamp() to the sorting util function.

Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,6 @@ import { Hidden } from 'src/components/Hidden';
import { TableCell } from 'src/components/TableCell';
import { TableRow } from 'src/components/TableRow';
import { useProfile } from 'src/queries/profile/profile';
import { getEventTimestamp } from 'src/utilities/eventUtils';

import {
formatProgressEvent,
Expand All @@ -18,16 +17,19 @@ import {
} from './utils';

import type { Event } from '@linode/api-v4/lib/account';
import type { DateTime } from 'luxon';

export interface completionEvent extends Event {
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: I think completedEvent sounds a bit better than completionEvent

completionTime: DateTime;
}
interface EventRowProps {
entityId?: number;
event: Event;
event: completionEvent;
}

export const EventRow = (props: EventRowProps) => {
const { event } = props;
const theme = useTheme();
const timestamp = getEventTimestamp(event);
const { action, message, username } = {
action: event.action,
message: getEventMessage(event),
Expand Down Expand Up @@ -85,7 +87,7 @@ export const EventRow = (props: EventRowProps) => {
</TableCell>
<Hidden mdDown>
<TableCell data-qa-event-created-cell parentColumn="Absolute Date">
<DateTimeDisplay value={timestamp.toString()} />
<DateTimeDisplay value={event.completionTime.toString()} />
</TableCell>
</Hidden>
</TableRow>
Expand Down
69 changes: 68 additions & 1 deletion packages/manager/src/features/Events/EventsLanding.test.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,9 +1,11 @@
import { within } from '@testing-library/react';
import { DateTime } from 'luxon';
import React from 'react';

import { eventFactory } from 'src/factories';
import { makeResourcePage } from 'src/mocks/serverHandlers';
import { HttpResponse, http, server } from 'src/mocks/testServer';
import { renderWithTheme } from 'src/utilities/testHelpers';
import { renderWithTheme, resizeScreenSize } from 'src/utilities/testHelpers';

import { EventsLanding } from './EventsLanding';

Expand Down Expand Up @@ -82,4 +84,69 @@ describe('EventsLanding', () => {

await findByText('No more events to show');
});

it('displays the data chronologically', async () => {
resizeScreenSize(1600);
const mockEvent1 = eventFactory.build({
action: 'linode_migrate',
created: DateTime.local().minus({ minutes: 1 }).toISO(),
duration: 30,
entity: {
id: 1,
label: 'linode-1',
},
status: 'finished',
});

const mockEvent2 = eventFactory.build({
action: 'firewall_device_add',
created: DateTime.local().minus({ minutes: 2 }).toISO(),
entity: {
id: 2,
label: 'firewall-1',
type: 'firewall',
},
secondary_entity: {
id: 1,
label: 'linode-1',
type: 'linode',
},
status: 'notification',
});

const eventsResponse = makeResourcePage([mockEvent1, mockEvent2], {
page: 1,
pages: 1,
results: 1,
});

server.use(
http.get('*/events', () => HttpResponse.json(eventsResponse), {
once: true,
}),
// `useEventsInfiniteQuery` needs to make two fetches to know if there are no more events to show
http.get('*/events', () => HttpResponse.json(makeResourcePage([])), {
once: true,
})
);

const { findByRole, findByText } = renderWithTheme(<EventsLanding />);
await findByText('No more events to show');

const table = await findByRole('table');
const tableBodyRows = within(table).getAllByRole('row').slice(1);
expect(tableBodyRows.length).toEqual(2);

const dates = tableBodyRows.map((row) => {
const cells = within(row).getAllByRole('cell');
return cells[3].textContent || '';
});

// util function to check if the dates are in descending order
const areDatesSorted = dates.every(
(date, i, arr) => i === 0 || new Date(arr[i - 1]) >= new Date(date)
);

expect(areDatesSorted).toBe(true);
Comment on lines +140 to +150
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we add a test-id to the Absolute Date table cell and grab the dates that way instead of grabbing the cell at index 3?

We can also just hardcode the date string; I think the test is clearer this way:

const mockEvent1 = eventFactory.build({
  ...
  created: '2024-12-10T16:29:52.973-05:00',
  ...
});
const mockEvent2 = eventFactory.build({
  ...
  created: '2024-12-10T16:28:52.974-05:00',
  ...
});
expect(dates).toStrictEqual(['2024-12-11 05:30', '2024-12-11 05:28']);

To test the sorting function, we can add a separate unit test for the sortEventsChronologically function in the utils test file

});
});
11 changes: 10 additions & 1 deletion packages/manager/src/features/Events/EventsLanding.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,9 @@ import {
StyledTableCell,
StyledTypography,
} from './EventsLanding.styles';
import { sortEventsChronologically } from './utils';

import type { completionEvent } from './EventRow';
import type { Filter } from '@linode/api-v4';

interface Props {
Expand Down Expand Up @@ -49,6 +51,13 @@ export const EventsLanding = (props: Props) => {
isLoading,
} = useEventsInfiniteQuery(filter);

const eventsInSortedOrder: completionEvent[] = React.useMemo(() => {
if (events && events.length > 0) {
return sortEventsChronologically(events);
}
return [];
}, [events]);

Comment on lines +54 to +60
Copy link
Member

Choose a reason for hiding this comment

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

This works! Although, it kind of hurts that we are considering client side sorting after all of the work done to simplify and streamline the events system. 😖

I wonder if we could consider removing getEventTimestamp completely and show explicit "Timestamp" and "Finished" times. No client side sorting changes would be needed and the events should be in order by their "timestamp" in this case (not sorted by "Finished"). Could this solve the problem?

Screenshot 2024-12-11 at 9 13 49 PM

Copy link
Contributor

Choose a reason for hiding this comment

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

that's a sensible solution, and i think with UI help it we could move towards this solution and get rid of client-side sorting. The mockup shown here is enough of a departure from the current UI to warrant UX validation from the team.

it makes clear to the user all events are sorted through the timestamp, however the are a few other things to consider:

  • is Timestamp the right name for a column
  • What happens to relative/absolute dates? (I know there are talk to finesse those columns)
  • The "Finished At" column presents cognitive issues. (aka: does N/A means this event did not finish somehow?) - the concept of a potential "completed" value being null may be quite foreign for a regular user.

I think this could easily be solved by labelling things well and perhaps some contextual help. I'll bring this up to the UX team today

const renderTableBody = () => {
if (isLoading) {
return (
Expand All @@ -70,7 +79,7 @@ export const EventsLanding = (props: Props) => {
} else {
return (
<>
{events?.map((event) => (
{eventsInSortedOrder?.map((event) => (
<EventRow
entityId={entityId}
event={event}
Expand Down
14 changes: 14 additions & 0 deletions packages/manager/src/features/Events/utils.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ import { getEventTimestamp } from 'src/utilities/eventUtils';
import { ACTIONS_WITHOUT_USERNAMES } from './constants';
import { eventMessages } from './factory';

import type { completionEvent } from './EventRow';
import type { Event } from '@linode/api-v4';

type EventMessageManualInput = {
Expand Down Expand Up @@ -137,3 +138,16 @@ export const formatProgressEvent = (event: Event): ProgressEventDisplay => {
showProgress,
};
};

export const sortEventsChronologically = (
events: Event[]
): completionEvent[] => {
return events
?.map((e) => ({
...e,
completionTime: getEventTimestamp(e),
}))
.sort((e1, e2) => {
return e2.completionTime.toMillis() - e1.completionTime.toMillis();
});
};