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

show follow notification in tab #1727

Open
wants to merge 25 commits into
base: main
Choose a base branch
from
Open

Conversation

pelumy
Copy link
Contributor

@pelumy pelumy commented Jan 8, 2025

Issues covered

127
P/S: This needs the backend to send follow notifications as a silent one and not an alert. This needs to be taken into consideration before merging.

Description

I wasn't expecting this PR to turn out this huge, but it did 😿. It contains the following changes:

  • Creates a new core data model version. The NosNotification entity needed a relationship with the Event entity.
  • Refactors the NosNotificationViewModel and NotificationView.
  • Creates a NosNotification and local notification for a silent follow notification.
  • Redefines the NotificationView to shows 3 tabs: "Follows", "In Network" and "Out of Network".
  • Deleting NosNotification reference when an Event is deleted in the DatabaseCleaner. Thanks for fixing this @mplorentz.

How to test

  1. Build the app.
  2. Select the Notifications option in the bottom tabs.
    • You should now see three tabs "Follows", "In Network" and "Out of Network".
  3. Please clone this to send a silent push notification. You can change the follower's npub in the json, this example uses Josh's pub. Don't forget to copy your device token and update that part in the notification console. This is a notion doc on how to send push notifications from the console.
    • You should see the notification appear in the Follows tab and also in your iPhone's home Screen.

Screenshots/Video

Post screenshots or video showing your changes, ideally showing how the app worked before and after these changes. Delete this section if this PR contains no visual changes.

Notification tab view

Before After
Before After
Before After
Before After

Video showing follows notification, inNetwork and outOfNetwork.
https://github.com/user-attachments/assets/a08cab0c-0a66-4099-9b8c-3b31b3bee6eb
N/B: I updated the UI after making the video. The screenshot above is the correct UI.

@pelumy pelumy marked this pull request as draft January 8, 2025 21:22
@pelumy pelumy changed the title Ishow follow notification in tab show follow notification in tab Jan 8, 2025
@pelumy pelumy marked this pull request as ready for review January 13, 2025 21:24
@joshuatbrown
Copy link
Contributor

👀

@joshuatbrown
Copy link
Contributor

Unfortunately, I'm getting some errors in my console that I think are the root cause of my old notifications not showing up. Here's what I see frequently in the console:

RelayService: Error parsing events: Could not merge changes.

I know I dealt with this when I added the new AuthorList entity, and I assume the issue here is similar. I don't entirely remember what the solution was there, but I'd be happy to debug with you if you'd like.

@pelumy
Copy link
Contributor Author

pelumy commented Jan 16, 2025

Unfortunately, I'm getting some errors in my console that I think are the root cause of my old notifications not showing up. Here's what I see frequently in the console:

RelayService: Error parsing events: Could not merge changes.

I know I dealt with this when I added the new AuthorList entity, and I assume the issue here is similar. I don't entirely remember what the solution was there, but I'd be happy to debug with you if you'd like.

Thanks @joshuatbrown. I will love to debug with you today.

@pelumy
Copy link
Contributor Author

pelumy commented Jan 17, 2025

To avoid over complicating things, after debugging with @joshuatbrown, we deem it best to fetch Events for inNetwork and outOfNetwork, then fetch NosNotifications for followEvents. We don't think performance wise, it is okay to convert 300 old notifications from Events to NosNotifications on app launch or when the user taps the notification tab button.

Given this change, I think it is fine to revert to the previous database definition we had. Thoughts?

cc: @mplorentz

@joshuatbrown
Copy link
Contributor

Reverting sounds fine to me, but I'd love to hear Matt's thoughts.

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.

2 participants