-
Notifications
You must be signed in to change notification settings - Fork 577
Translate view: don't try to log UX action for unauthenticated users #3946
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
base: main
Are you sure you want to change the base?
Conversation
Codecov Report❌ Patch coverage is 🚀 New features to boost your workflow:
|
|
I tried to use the data already available in the frontend instead of adding info to the Django template. Just in case, I kept the changes in a separate commit, so it's easy to revert 9f6564b Worth noting that I'm very much hand waving when writing JavaScript, and even more so when writing tests (asked ChatGPT to write one based on test cases). P.S. this seems to add an additional call to |
translate/src/api/uxaction.ts
Outdated
|
|
||
| async function isAuthenticated(): Promise<boolean> { | ||
| if (!userDataPromise) { | ||
| userDataPromise = fetchUserData(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Re-fetching user data for logging like this is not the right solution; the info is available at least via useAppSelector((state) => state[USER].isAuthenticated). Accesssing that might require renaming some functions to be hooks, or to pass the data along in arguments.
|
I think I'm in React/Redux hell. I tried moving the authentication state as a parameter of the existing function, but that required adding it to all props, update calls, tests, etc. This is how far I manage to go. It works, I'm not sure how correct it is in terms of model (e.g. separation). |
| }); | ||
| } | ||
| }, []); | ||
| }, [user.isAuthenticated, unread, log]); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same here: is this unnecessary?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Again, trying to wrap my head about React, and failing 😵💫
I think this was OK before, because the log was sent once at startup (could we potentially log the wrong number of unread notifications though?). But now it depends on authentication status.
The "multiple notifications" was caused by the log changing each render, so the solution is stabilize that across re-renders?
|
I seem to have introduced a bug for "unread notifications", where this action is sent multiple times. |
Fixes #3900
Not sure if there's a better way to do it.