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

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
@seluianova
Copy link
Contributor

seluianova commented Feb 4, 2025

"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

🙃 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

🙃 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

"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

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

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.

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

Successfully merging this pull request may close these issues.

Activity Log Unit test
2 participants