fix: tests & react query cache keys #3540

djabarovgeorge · 2023-05-30T12:25:44Z

What change does this PR introduce?

Fix the test.
Add the query to the cache key on all the cases.

Why was this change needed?

At the moment INFINITE_NOTIFICATIONS_QUERY_KEY could get the query object in useFetchNotifications but in other places, it was missing, which caused issues.

Other information (Screenshots)

djabarovgeorge · 2023-05-30T12:30:52Z

...on-center/src/components/notification-center-web-component/notification-center-component.tsx

+  const { storeQuery } = useNotifications();
+  const { updateAction } = useUpdateAction({ query: storeQuery });


Not sure is it is the best way to do so, but at the moment we need to pass the query to the cache key..

LetItRock · 2023-05-31T08:38:20Z

@djabarovgeorge left you a dm in the Discord, let's not merge this and chat about the changes

…ifier, subscriberId, subscriberHash changes

LetItRock · 2023-06-11T22:40:22Z

.github/workflows/test.yml

@@ -157,7 +157,7 @@ jobs:
      - uses: actions/checkout@v3
      - uses: ./.github/actions/setup-project
      - uses: mansagroup/nrwl-nx-action@v3
-        if: ${{matrix.projectName == '@novu/application-generic' }}
+        if: ${{matrix.projectName == '@novu/application-generic' || matrix.projectName == '@novu/notification-center'}}


when there are the changes in the notification-center package then run the tests pipeline: lint, build, test

LetItRock · 2023-06-11T22:40:58Z

packages/notification-center/package.json

+    "@testing-library/react": "^11.1.0",
+    "@testing-library/dom": "^9.3.0",
+    "@testing-library/user-event": "^12.1.10",
+    "@testing-library/jest-dom": "^4.2.4",


RTL for the notification-center package... I've added a couple of the unit/integration tests

LetItRock · 2023-06-11T22:42:06Z

packages/notification-center/src/components/novu-provider/NovuProvider.tsx

+  const [sessionInfo, setSessionInfo] = useState({
+    isSessionInitialized: false,
+    applicationIdentifier,
+    subscriberId,
+    subscriberHash,
+  });


created the state for the data that is needed to initialize the session...

LetItRock · 2023-06-11T22:43:00Z

packages/notification-center/src/components/novu-provider/NovuProvider.tsx

@@ -81,9 +86,9 @@ export function NovuProvider({
    (newSession: ISession) => {
      applyToken({ apiService, token: newSession.token });
      initializeSocket(newSession);
-      setSessionInitialized(true);
+      setSessionInfo((old) => ({ ...old, isSessionInitialized: true }));


this is the callback called when the session is initialized... setting this flag to true will result in fetching the org, unseen count, and notifications...

LetItRock · 2023-06-11T22:44:02Z

packages/notification-center/src/components/novu-provider/NovuProvider.tsx

+  useEffect(() => {
+    setSessionInfo((old) => ({
+      ...old,
+      isSessionInitialized: false,
+      applicationIdentifier,
+      subscriberId,
+      subscriberHash,
+    }));
+    logout();
+    queryClient.refetchQueries([...SESSION_QUERY_KEY, applicationIdentifier, subscriberId, subscriberHash]);
+  }, [logout, subscriberId, applicationIdentifier, subscriberHash]);


when the props subscriberId, applicationIdentifier, subscriberHash are changing, set the session to isSessionInitialized: false and then reinitialize the session

Do you mean that the reinitialize will be in onSuccessfulSession?

LetItRock · 2023-06-11T22:44:33Z

packages/notification-center/src/hooks/useFeedUnseenCount.ts


 export const useFeedUnseenCount = (
  { query }: { query?: IStoreQuery },
  options: UseQueryOptions<ICountData, Error, ICountData> = {}
 ) => {
  const { apiService, isSessionInitialized } = useNovuContext();
-  const setQueryKey = useSetQueryKey();
+  const feedUnseenCountQueryKey = useFeedUnseenCountQueryKey();


created a few helper hooks that do return the query keys... the hooks are reused in a couple places

LetItRock · 2023-06-11T22:45:57Z

packages/notification-center/src/hooks/useNotifications.test.tsx


+const PROMISE_TIMEOUT = 150;


I don't know why but increasing the timeout helps tests to pass :D

I wonder how hard was figuring it out 😅

I knew that the tests are correct, so wasn't that hard :)

LetItRock · 2023-06-11T22:49:34Z

packages/notification-center/src/store/notifications-provider.context.tsx

+    <StoreProvider stores={stores}>
+      <NotificationsProviderInternal>{children}</NotificationsProviderInternal>
+    </StoreProvider>


basically, I've created the new StoreProvider that allows us to use the storeQuery state inside of any public hooks like useUnseenCountQueryKey... this way we don't require the hook users to pass it through props... it's an internal state info

Amazing reason to refactor :)

packages/notification-center/src/hooks/useNotifications.test.tsx

packages/notification-center/src/shared/interfaces/index.ts

djabarovgeorge

As always amazing work and amazing addition tests left a couple of questions regarding things I was not sure about. @LetItRock

djabarovgeorge · 2023-06-12T12:48:24Z

packages/notification-center/src/components/novu-provider/NovuProvider.tsx

    },
-    [apiService, setSessionInitialized, initializeSocket]
+    [apiService, setSessionInfo, initializeSocket]


What is the difference between adding here setSessionInfo instead of sessionInfo?

setSessionInfo is a state setter ;)

djabarovgeorge · 2023-06-12T13:02:44Z

packages/notification-center/src/components/novu-provider/NovuProvider.tsx

-    setSessionInitialized(false);
-  }, [removeToken, disconnectSocket, apiService]);
+    setSessionInfo((old) => ({ ...old, isSessionInitialized: false }));
+  }, [setSessionInfo, disconnectSocket, apiService]);


removeToken is not needed anymore?

not needed because it's a util that never changes ;)

djabarovgeorge · 2023-06-12T13:43:19Z

packages/notification-center/src/components/novu-provider/NovuProvider.tsx

  );

  useEffect(() => disconnectSocket, [disconnectSocket]);

+  useEffect(() => {
+    setSessionInfo((old) => ({
+      ...old,


do we need ...old,? won't it will be overridden by the following params, or did you add for future proof if we will add more params to setSessionInfo object?

yes, it's always better to do it for the future changes, like this way I won't need to revisit every setSessionInfo call

djabarovgeorge · 2023-06-12T14:01:20Z

packages/notification-center/src/components/novu-provider/NovuProvider.tsx

+  useEffect(() => {
+    setSessionInfo((old) => ({
+      ...old,
+      isSessionInitialized: false,
+      applicationIdentifier,
+      subscriberId,
+      subscriberHash,
+    }));
+    logout();
+    queryClient.refetchQueries([...SESSION_QUERY_KEY, applicationIdentifier, subscriberId, subscriberHash]);
+  }, [logout, subscriberId, applicationIdentifier, subscriberHash]);


Do you mean that the reinitialize will be in onSuccessfulSession?

djabarovgeorge · 2023-06-12T14:24:05Z

packages/notification-center/src/hooks/useFetchUserPreferencesQueryKey.ts

+
+export const useFetchUserPreferencesQueryKey = () => {
+  const setQueryKey = useSetQueryKey();
+  const queryKey = useMemo(() => setQueryKey(USER_PREFERENCES_QUERY_KEY), [setQueryKey]);


Suggested change

const queryKey = useMemo(() => setQueryKey(USER_PREFERENCES_QUERY_KEY), [setQueryKey]);

const queryKey = useMemo(() => setQueryKey([...USER_PREFERENCES_QUERY_KEY]), [setQueryKey]);

djabarovgeorge · 2023-06-12T14:25:56Z

packages/notification-center/src/hooks/useNotifications.test.tsx


+const PROMISE_TIMEOUT = 150;


I wonder how hard was figuring it out 😅

djabarovgeorge · 2023-06-12T14:26:23Z

packages/notification-center/src/hooks/useNotifications.test.tsx

    jest.clearAllMocks();
+    queryClient.clear();


djabarovgeorge · 2023-06-12T14:30:42Z

packages/notification-center/src/store/notifications-provider.context.tsx

+    <StoreProvider stores={stores}>
+      <NotificationsProviderInternal>{children}</NotificationsProviderInternal>
+    </StoreProvider>


Amazing reason to refactor :)

djabarovgeorge · 2023-06-12T14:34:22Z

packages/notification-center/src/components/novu-provider/NovuProvider.test.tsx

djabarovgeorge · 2023-06-14T17:18:07Z

packages/notification-center/src/hooks/useFeedUnseenCountQueryKey.ts

+import { FEED_UNSEEN_COUNT_QUERY_KEY } from './queryKeys';
+import { useSetQueryKey } from './useSetQueryKey';
+
+export const useFeedUnseenCountQueryKey = (query?: IStoreQuery) => {


Not sure I am following up, in this case, we can not get the query inside this hook from the useStore hook?

this hook is used in the useFeedUnseenCount which is used for every feed tab, which passes it's storeQuery to the hook useFeedUnseenCount --> useFeedUnseenCountQueryKey

…tions from the pr #3540

fix: tests & react query cache keys

704522b

github-actions bot added the @novu/notification-center label May 30, 2023

davidsoderberg approved these changes May 30, 2023

View reviewed changes

djabarovgeorge commented May 30, 2023

View reviewed changes

feat(notification-center): reinitialize session when applicationIdent…

c97fcee

…ifier, subscriberId, subscriberHash changes

LetItRock reviewed Jun 11, 2023

View reviewed changes

LetItRock assigned LetItRock and djabarovgeorge Jun 11, 2023

Merge branch 'next' into fix-test-&-react-query-cache-keys

7598a26

LetItRock mentioned this pull request Jun 12, 2023

🐛 Bug Report: Novu notification center "Mark all as read" button not doing anything #3580

Closed

2 tasks

ainouzgali reviewed Jun 12, 2023

View reviewed changes

packages/notification-center/src/hooks/useNotifications.test.tsx Show resolved Hide resolved

ainouzgali approved these changes Jun 12, 2023

View reviewed changes

packages/notification-center/src/shared/interfaces/index.ts Show resolved Hide resolved

djabarovgeorge commented Jun 12, 2023

View reviewed changes

LetItRock added 4 commits June 13, 2023 00:51

feat(notification-center): add some more unit/integration tests

1cd0afc

feat(notification-center): fix failing e2e tests for the widget

f6be2e1

chore: fixing the tests pipeline for the packages

1e2cdaf

chore: fixing the tests pipeline for the packages

19bc37e

LetItRock force-pushed the fix-test-&-react-query-cache-keys branch from 60f2bf4 to 19bc37e Compare June 14, 2023 16:18

djabarovgeorge commented Jun 14, 2023

View reviewed changes

chore(notification-center): fixed failing unit tests and added sugges…

7869b8b

…tions from the pr #3540

github-actions bot added the documentation Improvements or additions to documentation label Jun 14, 2023

LetItRock added this pull request to the merge queue Jun 15, 2023

Merged via the queue into next with commit e10d9af Jun 15, 2023
32 checks passed

LetItRock deleted the fix-test-&-react-query-cache-keys branch June 15, 2023 08:38

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: tests & react query cache keys #3540

fix: tests & react query cache keys #3540

djabarovgeorge commented May 30, 2023 •

edited

djabarovgeorge May 30, 2023

LetItRock commented May 31, 2023

LetItRock Jun 11, 2023

LetItRock Jun 11, 2023

LetItRock Jun 11, 2023

LetItRock Jun 11, 2023

LetItRock Jun 11, 2023

djabarovgeorge Jun 12, 2023

LetItRock Jun 14, 2023

LetItRock Jun 11, 2023

LetItRock Jun 11, 2023

djabarovgeorge Jun 12, 2023

LetItRock Jun 14, 2023

LetItRock Jun 11, 2023

djabarovgeorge Jun 12, 2023

djabarovgeorge left a comment

djabarovgeorge Jun 12, 2023

LetItRock Jun 14, 2023

djabarovgeorge Jun 12, 2023

LetItRock Jun 14, 2023

djabarovgeorge Jun 12, 2023

LetItRock Jun 14, 2023

djabarovgeorge Jun 12, 2023

djabarovgeorge Jun 12, 2023

djabarovgeorge Jun 12, 2023

djabarovgeorge Jun 12, 2023

djabarovgeorge Jun 12, 2023

djabarovgeorge Jun 12, 2023

djabarovgeorge Jun 14, 2023

djabarovgeorge Jun 14, 2023

LetItRock Jun 14, 2023

		const { storeQuery } = useNotifications();
		const { updateAction } = useUpdateAction({ query: storeQuery });

	const queryKey = useMemo(() => setQueryKey(USER_PREFERENCES_QUERY_KEY), [setQueryKey]);
	const queryKey = useMemo(() => setQueryKey([...USER_PREFERENCES_QUERY_KEY]), [setQueryKey]);

fix: tests & react query cache keys #3540

fix: tests & react query cache keys #3540

Conversation

djabarovgeorge commented May 30, 2023 • edited

What change does this PR introduce?

Why was this change needed?

Other information (Screenshots)

Choose a reason for hiding this comment

LetItRock commented May 31, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

djabarovgeorge left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

djabarovgeorge commented May 30, 2023 •

edited