Skip to content

Fix: Notifications do not load when switching wallets#3586

Merged
rdig merged 2 commits intomasterfrom
fix/3508-notifications-do-not-load-when-switching-wallets
Nov 6, 2024
Merged

Fix: Notifications do not load when switching wallets#3586
rdig merged 2 commits intomasterfrom
fix/3508-notifications-do-not-load-when-switching-wallets

Conversation

@iamsamgibbs
Copy link
Copy Markdown
Contributor

@iamsamgibbs iamsamgibbs commented Oct 31, 2024

Description

The actual fix is in this auth proxy PR: https://github.com/JoinColony/colonyCDappAuthProxy/pull/33

This PR aims to address an issue where notifications do not load when switching wallets.

This was being caused by the useGetUserNotificationsHmacQuery in the NotificationsDataContextProvider being sent before the auth cookie had been set. Which resulted in the getUserNotificationsHMAC lambda returning undefined as the x-wallet-address header is not set.

My proposed solution here is to change how the auth proxy handles un-auth queries. By default the auth proxy allows all un-auth queries and only blocks mutations. This change will still allow un-auth queries by default, but will restrict certain queries (currently just getUserNotificationsHMAC) from executing when not auth.

This causes the useGetUserNotificationsHmacQuery to be reattempted when the wallet connection has properly finished and notifications will load correctly.

Note: I have tried to avoid making changes to the auth proxy by preventing the useGetUserNotificationsHmacQuery from being triggered until the auth headers are set, but was unable to get this working. If this is the preferred solution and anyone has any ideas on how to resolve this, I'm happy to re-investigate this.

Testing

  • Step 1 - Spin up the dev environment from scratch so that it uses the new auth proxy commit (or stop your auth proxy docker and run the branch separately).

We first want to make sure we have two users with notifications enabled and setup.

  • Step 2 - Connect with dev wallet 1 and go to planex. Refresh to be extra sure the createUserNotificationsData mutation has ran.
  • Step 3 - Disconnect your wallet.
Screenshot 2024-10-31 at 15 17 18
  • Step 4 - Connect with dev wallet 2, you should automatically load into planex. Refresh to be extra sure the createUserNotificationsData mutation has ran.

Now that we have two users with notifications enabled we can test properly.

  • Step 5 - Disconnect your wallet again and connect with dev wallet 1.
  • Step 6 - Create a simple payment action and send tokens to Amy.
  • Step 7 - Check you receive a notification.
  • Step 8 - Disconnect your wallet. Without refreshing, connect with dev wallet 2, you should automatically load into planex.
  • Step 9 - Check you have notifications without refreshing.

Diffs

New stuff

  • Auth proxy can now block non-auth queries as necessary

Resolves #3508

@iamsamgibbs iamsamgibbs self-assigned this Oct 31, 2024
@iamsamgibbs iamsamgibbs marked this pull request as ready for review October 31, 2024 15:27
@iamsamgibbs iamsamgibbs requested review from a team as code owners October 31, 2024 15:27
@mmioana mmioana self-requested a review November 1, 2024 10:38
mmioana
mmioana previously approved these changes Nov 1, 2024
Copy link
Copy Markdown
Contributor

@mmioana mmioana left a comment

Choose a reason for hiding this comment

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

Awesome work on this one @iamsamgibbs 👏 🥇 💯

After connecting the wallets, the magic bell user id was created for both leela and amy
Screenshot 2024-11-01 at 13 00 54
Screenshot 2024-11-01 at 13 01 14

Created a simple payment from leela to amy
Screenshot 2024-11-01 at 13 02 01

leela received notifications
Screenshot 2024-11-01 at 13 02 06

And switched to amy's wallet without a page refresh and the notifications were there
Screenshot 2024-11-01 at 13 02 23

Also the check in auth-proxy for the getUserNotificationsHMAC query has passed ✅
Screenshot 2024-11-01 at 13 05 57
Screenshot 2024-11-01 at 13 06 07

rdig
rdig previously approved these changes Nov 4, 2024
Copy link
Copy Markdown
Member

@rdig rdig left a comment

Choose a reason for hiding this comment

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

All good here, tested and working as you're describing, the notifications show up properly for the second account.

Screenshot from 2024-11-04 15-04-20
Screenshot from 2024-11-04 15-04-34
Screenshot from 2024-11-04 15-04-51

@rdig rdig dismissed stale reviews from mmioana and themself via 240b13b November 6, 2024 13:12
@rdig rdig force-pushed the fix/3508-notifications-do-not-load-when-switching-wallets branch from 69bc3b5 to 240b13b Compare November 6, 2024 13:12
@rdig rdig merged commit c866150 into master Nov 6, 2024
@rdig rdig deleted the fix/3508-notifications-do-not-load-when-switching-wallets branch November 6, 2024 13:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Notifications don't load when switching wallets

3 participants