Fixed notifications reload error

[imfarhan0105-bits]

Fixes #7354

What was happening

I found that when a user unfollows someone, the Follow record gets deleted but the follower notification can stick around. Since the notification is still there but its associated record is gone, opening the notifications page ends up throwing a NoMethodError.

While looking into this, I also noticed the same kind of issue with GpxSuccessNotifier when the associated trace is deleted.

What I changed

I updated the cleanup so that notifications are removed along with their associated Follow or Trace records. I also changed the unfollow flow to use destroy_all so the cleanup callbacks actually run.

For older notifications that were already left orphaned, I added a guard so they are simply skipped instead of crashing the page. Record-less notifications that are valid by design, like GPX import failures, are still shown normally.

Tests

I added regression tests for the different cases, including:

• follower notifications after unfollowing
• existing orphaned follower notifications
• GPX success notifications after deleting a trace
• existing orphaned GPX success notifications
• GPX failure notifications without an associated record

All the relevant tests are passing locally.

[tomhughes]

Well this is the fundamental question - what is the correct fix here?

Is the correct fix to remove the notification? To keep it but to render it specially without any details? Something else?

[imfarhan0105-bits]

Well this is the fundamental question - what is the correct fix here?

Is the correct fix to remove the notification? To keep it but to render it specially without any details? Something else?

Personally, I feel like removing the notification is the better way to handle it. The notification is tied to the actual Follow/Trace record, so once that record is gone, I don't think there's much point in keeping the notification around either.

For example, if someone unfollows a user, having a “new follower” notification still sitting there afterwards feels a bit weird, since that relationship doesn't exist anymore.

[gravitystorm]

On the other hand, if I get email notifications saying that someone has started following me, I would expect the site notifications to say the same thing. If the user unfollows me, that doesn't make the notification invalid - they still did follow at a particular time, and I should be able to click through to see their user etc.

[tomhughes]

Also adding code now to remove notifications when a follow is cancelled certainly won't work to fix any existing cases like the one that triggered this so if we do go this way we'll need a task or migration to fixup existing notifications.

[imfarhan0105-bits]

Yeah, I agree with this. Thinking about it again, the notification should really represent something that happened, rather than the current state of the relationship. If someone followed me and I received a notification for it, that event is still valid even if they later decide to unfollow me. Removing it afterwards would effectively erase that history, and I agree that being able to click through to their profile still makes the notification useful.

I also hadn't considered the email notification angle initially. If the email tells me that someone followed me, I'd expect the corresponding site notification to remain as well.

And you're right about the existing orphaned notifications. My current approach only handles them at render time, while the cleanup would only apply going forward. So it doesn't really address the existing cases that led to this issue in the first place.

I'll rethink the approach properly. Thanks for pointing this out.

[pablobm]

Echoing what has already been said, I think that we should not delete the notifications when the original record is gone. There's the mentioned "email trail" issue, but that's not my only concern.

I feel uneasy about coupling notifications with other aspects of the application, at least for the moment. Having Follow (for example) being aware of notifications and responsible for cleaning them up smells wrong to me. Not that it could not happen in the future, but that there should be a good reason for it.

So I think we should be showing some sort of "notification was here, but the content was gone, sorry" message, perhaps in a subdued/muted design, and move on. Something like this:

Incidentally, there shouldn't be "fixup commits" like your second one. For that sort of thing, please fold the commits together to present a clean history, as per the contributor guidelines.

[pablobm]

Initially you create the notification programmatically, and I see no reason why it should be deleted interactively as that is quite costly in terms of speed of tests. You do the right thing later in the GPX tests.

[pablobm]

A specific problem with this is that it messes up with the counts. If we show up 20 notifications per page, but one is skipped, then there are 19 notifications on the page. At the moment we are not showing counts, but we will and the problem will compound. Also users will not be able to delete/discard these notifications when this becomes possible.

[imfarhan0105-bits]

Hi @pablobm ,

I’ve gone through the discussion on the issue and the PR comments again, and I think I have a much better understanding of the direction now.

My understanding is that notifications should remain even if the associated Follow/Trace is deleted, and in that case we show a muted message saying that the content is no longer available, rather than deleting or skipping the notification.

One thing I’m still unsure about: if, say, 4 people follow a user and 2 of them later unfollow, should all 4 notifications remain, with the 2 deleted follows shown as unavailable?

Also, thinking about the UI/UX side of this, wouldn’t the notifications tab become quite cluttered for someone who has a lot of daily interaction on the app, with potentially many “content is no longer available” notifications?

After the discussion on the issue, I was thinking of putting together a small proper proposal covering the expected behaviour, UI/UX considerations, edge cases, and implementation approach, and then reworking the PR based on that. Would that be a good way to proceed?

Thanks!

[pablobm]

One thing I’m still unsure about: if, say, 4 people follow a user and 2 of them later unfollow, should all 4 notifications remain, with the 2 deleted follows shown as unavailable?

That's right. The problem is that the Follow record is deleted for the 2 people who unfollowed and _new_follower.html.erb can't be rendered for those. However the 2 other people who still follow will have a Follow record and those notifications can be rendered.

Also, thinking about the UI/UX side of this, wouldn’t the notifications tab become quite cluttered for someone who has a lot of daily interaction on the app, with potentially many “content is no longer available” notifications?

It could potentially happen, but my guess is that it's uncommon. Not many people will delete recent GPX traces or quickly follow-unfollow that this will be a frequent occurrence. If we find that this is not true, we can adjust.

After the discussion on the issue, I was thinking of putting together a small proper proposal covering the expected behaviour, UI/UX considerations, edge cases, and implementation approach, and then reworking the PR based on that. Would that be a good way to proceed?

My feeling is that this issue is a bit pressing at the moment, as people affected by it won't be able to see their notification pages at all, getting a 500 response. Therefore I would lean towards a simple but decent solution now, and then it's ok if you want to come back with a more thorough study.

Also note that there's other work going on around this area, currently #7316, so decisions there may impact your study.