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

[Notifications] Fix: Notifications should use correct amount #3443

Conversation

iamsamgibbs
Copy link
Contributor

Description

This PR applies the fix introduced in #3307 for the amount in the activity feed and applies it to the notification messages.

Due to the way notification messages are structured, it is very unlikely this would ever actually show up in production as it priorities the custom title, so would only ever show if a custom title was not provided (which is not possible via the UI). However it is such a tiny fix that we will apply it regardless!

Testing

  • Step 1 - Go to src/components/common/Extensions/UserHub/partials/NotificationsTab/partials/Notification/Action/MotionNotificationMessage.tsx and remove actionTitle from the secondPart variable.
const secondPart =
      actionTitle || actionMetadataDescription || formatText(MSG.unknownAction);

Should become:

    const secondPart =
      actionMetadataDescription || formatText(MSG.unknownAction);

This will ensure the custom title is not shown.

  • Step 2 - Install the reputation weighted voting extension
  • Step 3 - Create a simple payment motion
  • Step 4 - Stake to support it fully
  • Step 5 - Check the notification, note the amount should exactly match the amount entered when creating the motion.
Screenshot 2024-10-23 at 17 25 47
  • Step 6 - Run the following mutation in GraphiQL to change the network fee stored in the database:
mutation MyMutation {
  updateCurrentNetworkInverseFee(
    input: {id: "networkInverseFee", inverseFee: "50"}
  ) {
    id
    inverseFee
  }
}
  • Step 7 - Refresh and check the notification again. The amount should have changed to reflect the change in network fee.
Screenshot 2024-10-23 at 17 25 22
  • Step 8 - Forward time with npm run forward-time 1 and finalise the motion.
Screenshot 2024-10-23 at 17 35 01
  • Step 9 - Run another mutation to change the network fee back.
mutation MyMutation {
  updateCurrentNetworkInverseFee(
    input: {id: "networkInverseFee", inverseFee: "100"}
  ) {
    id
    inverseFee
  }
}
  • Step 10 - Refresh and check the notification. The amount change not have changed as the network fee changed after the motion was finalized.
Screenshot 2024-10-23 at 17 35 01

Diffs

Changes 🏗

  • networkInverseFee is passed to actionMetadataDescription for notifications

@iamsamgibbs iamsamgibbs self-assigned this Oct 23, 2024
@iamsamgibbs iamsamgibbs marked this pull request as ready for review October 23, 2024 16:37
@iamsamgibbs iamsamgibbs requested review from a team as code owners October 23, 2024 16:37
@mmioana mmioana self-requested a review October 24, 2024 06:49
Copy link
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.

Thanks for keeping an eye on this @iamsamgibbs and nice work 👏 🙌

Everything works as expected ✨

Applied the code change to not display the custom action title
Screenshot 2024-10-24 at 09 16 32

Installed the extension
Screenshot 2024-10-24 at 09 50 46

Created a simple payment with Reputation
Screenshot 2024-10-24 at 09 51 19
Screenshot 2024-10-24 at 09 51 32
Screenshot 2024-10-24 at 09 51 45

Updated the network fee
Screenshot 2024-10-24 at 09 52 20

And the notifications have updated the displayed amount
Screenshot 2024-10-24 at 09 52 34

Finalised the motion
Screenshot 2024-10-24 at 09 52 50
Screenshot 2024-10-24 at 09 53 11
Screenshot 2024-10-24 at 09 53 31

Reverted the change to the network fee
Screenshot 2024-10-24 at 09 53 47

And the values remained as before 💪
Screenshot 2024-10-24 at 09 54 21

Niiice 🎉

Copy link
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.

Tested this and all good, up until the motion finishes, the value correctly changes based on the network fee.

Once finalized, it's no longer affected by fee changes.

Very nice!

Screenshot from 2024-10-24 13-14-38
Screenshot from 2024-10-24 13-16-05
Screenshot from 2024-10-24 13-17-04
Screenshot from 2024-10-24 13-17-12
Screenshot from 2024-10-24 13-17-18
Screenshot from 2024-10-24 13-17-45
Screenshot from 2024-10-24 13-18-03
Screenshot from 2024-10-24 13-18-10
Screenshot from 2024-10-24 13-18-14
Screenshot from 2024-10-24 13-18-29
Screenshot from 2024-10-24 13-21-28
Screenshot from 2024-10-24 13-21-36
Screenshot from 2024-10-24 13-21-40

One thing I want to point out (as I also mention in the channel), that the landing page navigation sidebar is broken on this branch, but most likely due to a rebase

Screencast.from.2024-10-24.12-44-04.mp4

Copy link
Contributor

@davecreaser davecreaser left a comment

Choose a reason for hiding this comment

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

Great stuff @iamsamgibbs thank you so much for keeping track of this and fixing it! 🚀

It works exactly as you've described:
Screenshot 2024-10-24 at 12 16 13
Screenshot 2024-10-24 at 12 16 58
Screenshot 2024-10-24 at 12 23 08

And after running the final mutation the amount hasn't changed. Nice one!

PS. Sorry, but you'll need to rebase again since I had to rebase the notifications branch while fixing that pesky sidepanel navigation issue.

@iamsamgibbs iamsamgibbs force-pushed the fix/notifications-should-use-correct-amount branch from ff38075 to 37f7021 Compare October 24, 2024 12:11
@iamsamgibbs iamsamgibbs merged commit 57c14c7 into feat/in-app-notifications Oct 24, 2024
2 checks passed
@iamsamgibbs iamsamgibbs deleted the fix/notifications-should-use-correct-amount branch October 24, 2024 12: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.

4 participants