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

Jump to bottom

1847: Activity log tests #1889

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

1847: Activity log tests #1889

wants to merge 2 commits into from

Conversation

f1sh1918
Copy link
Contributor

@f1sh1918 f1sh1918 commented Feb 3, 2025

Short description

Added some tests for the activity log

Proposed changes

  • added function test for ActivityLog.ts (session storage)
  • added component tests for several components
  • created a separate component for the ActivityLogTable to improve testability
  • fix some invalid nesting of html components inside tbody

Side effects

  • none

Testing

  1. npm run test
  2. Since i refactored sth, please test the activity log:
  • Login with nuernberg region admin or region manager
  • "User settings" open activity -> should be empty
  • Create cards and do the step before -> log entries should be visible

Resolved issues

Fixes: #1847

@f1sh1918 f1sh1918 force-pushed the 1847-activity-log-tests branch 3 times, most recently from 8263368 to 7a07802 Compare February 3, 2025 14:35
@f1sh1918 f1sh1918 force-pushed the 1847-activity-log-tests branch from 7a07802 to 2584643 Compare February 3, 2025 14:47
@seluianova
Copy link
Contributor

seluianova commented Feb 4, 2025 •
edited
Loading

"User settings" open activity -> should be empty

off-topic but I haven't tested this functionality before, so just wanted to ask now:
why the activity log was added in the settings section? (if there's nothing to set there)

@seluianova
Copy link
Contributor

seluianova commented Feb 4, 2025 •
edited
Loading

🙃 I compared the view on main and on the current branch, and found this difference:
main
image
feature-branch
image
Not critical but I think there shouldn't be a hover if "Keine Einträge vorhanden".
I also think the alignment of "Keine Einträge vorhanden" looks a bit strange. I think it should be aligned across the entire row, not just in the first column.

@f1sh1918
Copy link
Contributor Author

f1sh1918 commented Feb 4, 2025 •
edited
Loading

🙃 I compared the view on main and on the current branch, and found this difference: main image feature-branch image Not critical but I think there shouldn't be a hover if "Keine Einträge vorhanden". I also think the alignment of "Keine Einträge vorhanden" looks a bit strange. I think it should be aligned across the entire row, not just in the first column.

yea true i will check that. i had to change the structure to fix invalid nesting of html components.

@f1sh1918
Copy link
Contributor Author

f1sh1918 commented Feb 4, 2025 •
edited
Loading

"User settings" open activity -> should be empty

off-topic but I haven't tested this functionality before, so just wanted to ask now: why the activity log was added in the settings section? (if there's nothing to set there)

Because its somehow user related data but not settings, true.
I really think no one uses this feature even it was requested.
We could think about moving that one level higher, makes absolutely sense.
Here in between, f.e.
image
Feel free to create a ticket for that

@f1sh1918
Copy link
Contributor Author

f1sh1918 commented Feb 5, 2025

Alright i removed the hover and set the colspan to 999 to ensure that it will take the whole width in any case for example for other projects we may render more columns.

image

@f1sh1918 f1sh1918 force-pushed the 1847-activity-log-tests branch from 12bad63 to f0f9d68 Compare February 5, 2025 11:03
@seluianova
Copy link
Contributor

One more off-topic question:
Why do we store activity log in the session storage?
If I'm working in two tabs, I can have different activity logs, even if I'm logged in as the same user...
I think it's quite confusing
image

seluianova
seluianova approved these changes Feb 5, 2025
View reviewed changes
Copy link
Contributor

@seluianova seluianova left a comment

Choose a reason for hiding this comment

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

Changes look good to me now, thanks!

@f1sh1918
Copy link
Contributor Author

f1sh1918 commented Feb 5, 2025 •
edited
Loading

One more off-topic question: Why do we store activity log in the session storage? If I'm working in two tabs, I can have different activity logs, even if I'm logged in as the same user... I think it's quite confusing image

The main reason was that the user data (sensitive data) will in any case be cleared and only remain for a particular session.
If we do this with localStorage we would have to ensure that it will be cleared not only on logout but regulary, because the user may only close the tab or sth.

steffenkleinle
steffenkleinle reviewed Feb 8, 2025
View reviewed changes
administration/src/project-configs/nuernberg/__tests__/ActivityLogEntry.test.tsx
Comment on lines +10 to +15
const { getByTestId } = render(<ActivityLogEntry timestamp={new Date()} card={activityLogCardExample} />)
expect(getByTestId('activity-log-entry-timestamp').textContent).toBe('1/1/2024, 12:00:00 AM')
expect(getByTestId('activity-log-entry-fullname').textContent).toBe('Thea Test')
expect(getByTestId('activity-log-entry-pass-id').textContent).toBe('3132222')
expect(getByTestId('activity-log-entry-birthday').textContent).toBe('01.02.2000')
expect(getByTestId('activity-log-entry-expiry').textContent).toBe('01.01.2026')
Copy link
Member

Choose a reason for hiding this comment

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

❓ Why are you testing with test id? That should only be used as last resort if asserting on anything else is not possible. Same for other tests.
See https://testing-library.com/docs/queries/about/#priority

Hmm I see, testing tables is apparently not that easily possible with react testing library. Found some issue/gists here though which sounds like an okay solution. testing-library/dom-testing-library#583 (comment)

Since setting a test id and then matching that the content is correct basically only tests that some specific text is present as well. This still does not assert that the correct text is rendered in the right column (if eg the table header order changes). Personally, I would therefore prefer to avoid the test id and just assert that some text is rendered.

Suggested change
const { getByTestId } = render(<ActivityLogEntry timestamp={new Date()} card={activityLogCardExample} />)
expect(getByTestId('activity-log-entry-timestamp').textContent).toBe('1/1/2024, 12:00:00 AM')
expect(getByTestId('activity-log-entry-fullname').textContent).toBe('Thea Test')
expect(getByTestId('activity-log-entry-pass-id').textContent).toBe('3132222')
expect(getByTestId('activity-log-entry-birthday').textContent).toBe('01.02.2000')
expect(getByTestId('activity-log-entry-expiry').textContent).toBe('01.01.2026')
expect(getByText('1/1/2024, 12:00:00 AM')).toBeTruthy()
expect(getByText('Thea Test')).toBeTruthy()
expect(getByText('3132222')).toBeTruthy()
expect(getByText('01.02.2000')).toBeTruthy()
expect(getByText('01.01.2026')).toBeTruthy()

administration/src/bp-modules/user-settings/__tests__/ActivityLogTable.test.tsx
const { getByText, getByTestId } = renderWithTranslation(
<ActivityLogTable activityLog={[]} activityLogConfig={nuernbergConfig.activityLogConfig!} />
)
expect(getByTestId('activity-log-column-names').textContent).toBe(
Copy link
Member

Choose a reason for hiding this comment

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

🔧 I think the following would more closely resemble how a user would use the table (and avoid cluttering the code with test ids):

Suggested change
expect(getByTestId('activity-log-column-names').textContent).toBe(
expect(queryAllByRole('row')[0].textContent).toBe(nuernbergConfig.activityLogConfig!.columnNames.join(''))

administration/src/bp-modules/user-settings/__tests__/ActivityLogTable.test.tsx
Comment on lines +24 to +28
expect(getByTestId('activity-log-column-names').textContent).toBe(
nuernbergConfig.activityLogConfig!.columnNames.join('')
)
expect(queryByText('Keine Einträge vorhanden')).toBeNull()
expect(getByTestId('activity-log-table-body').children).toHaveLength(2)
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
expect(getByTestId('activity-log-column-names').textContent).toBe(
nuernbergConfig.activityLogConfig!.columnNames.join('')
)
expect(queryByText('Keine Einträge vorhanden')).toBeNull()
expect(getByTestId('activity-log-table-body').children).toHaveLength(2)
expect(queryAllByRole('row')[0].textContent).toBe(nuernbergConfig.activityLogConfig!.columnNames.join(''))
expect(queryByText('Keine Einträge vorhanden')).toBeNull()
expect(queryAllByRole('row')).toHaveLength(3))

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@steffenkleinle steffenkleinle steffenkleinle left review comments

@seluianova seluianova seluianova approved these changes

@ztefanie ztefanie Awaiting requested review from ztefanie ztefanie is a code owner

At least 2 approving reviews are required to merge this pull request.

Assignees
No one assigned
Labels
not-testable
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Activity Log Unit test
3 participants
@f1sh1918 @seluianova @steffenkleinle