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

fix(web-push-notifications): fix favourite push notifications #23286

Conversation

elizabeth-dev
Copy link
Contributor

@elizabeth-dev elizabeth-dev commented Jan 28, 2023

Fix a bug where clicking in a favourite push notification would result in a 404 (not found) error, since we were redirecting the user to the wrong URL (we were redirecting to /@<user who favourited>/<favourited status id>, when it should be /@<favourited status author>/<favourited status id>)

@elizabeth-dev
Copy link
Contributor Author

elizabeth-dev commented Jan 28, 2023

I could have nested another ternary on line 93, but it felt a bit too much, and I saw the overwriting approach was already being used below, at line 103.

Copy link
Contributor

@ClearlyClaire ClearlyClaire left a comment

Choose a reason for hiding this comment

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

Looks good to me, although I think a more generic fix wouldn't be to special-case favourite, but simply use notification.status.account.acct whenever notification.status exists.

@elizabeth-dev
Copy link
Contributor Author

elizabeth-dev commented Jan 30, 2023

@ClearlyClaire I thought about that, but in the case of boosts, notification.status exists and we still redirect to /@<user who favourited>/<favourited status id> (for some reason). And it works, so I'm not sure about changing that existing behavior.

@ClearlyClaire
Copy link
Contributor

@ClearlyClaire I thought about that, but in the case of boosts, notification.status exists and we still redirect to /@<user who favourited>/<favourited status id> (for some reason). And it works, so I'm not sure about changing that existing behavior.

Are you sure about that? I can actually experience the issue for both favourites and reblogs.

@elizabeth-dev
Copy link
Contributor Author

elizabeth-dev commented Jan 30, 2023

@ClearlyClaire You do? I never had any issue with boost notifications, and I did test that behavior when writing this PR

Example (look at the URL vs the user in the post): https://tech.lgbt/@E_net4@hachyderm.io/109768855342795754

Maybe it's something specific to some instances?

Tbh for me it makes a lot more sense the way you suggested, since having the user who boosted in the URL does nothing at first glance. So, if you are okay with changing that, I'll be happy to update the PR.

@ClearlyClaire
Copy link
Contributor

@ClearlyClaire You do? I never had any issue with boost notifications, and I did test that behavior when writing this PR

Example (look at the URL vs the user in the post): https://tech.lgbt/@E_net4@hachyderm.io/109768855342795754

Maybe it's something specific to some instances?

Actually, this seems like a weird edge case with remote VS local users. I'll look into it but it's not directly related to the issue at hand.

Tbh for me it makes a lot more sense the way you suggested, since having the user who boosted in the URL does nothing at first glance. So, if you are okay with changing that, I'll be happy to update the PR.

Sure!

@elizabeth-dev
Copy link
Contributor Author

@ClearlyClaire I see, you're right. I'll update the PR later this morning.

Fix a bug where clicking in a favourite push notification would result in a 404 (not found) error, since we were redirecting the user to the wrong URL (we were redirecting to /@<user who favourited>/<favourited status id>, when it should be /@<favourited status author>/<favourited status id>)
@elizabeth-dev elizabeth-dev force-pushed the feature/fix-favourite-notifications branch from efd6988 to d88f4fe Compare February 2, 2023 17:36
@elizabeth-dev
Copy link
Contributor Author

@ClearlyClaire Hi, sorry for the delay, I've been busier than I expected. I updated the PR.

I could have brought back the ternary that was there before, but it felt less cluttered this way.

@elizabeth-dev elizabeth-dev requested review from ClearlyClaire and removed request for nightpool February 2, 2023 17:38
@ClearlyClaire ClearlyClaire merged commit 7e04b15 into mastodon:main Feb 3, 2023
tmorio pushed a commit to Otaku-Social/otadon that referenced this pull request Feb 7, 2023
…on#23286)

Fix a bug where clicking in a favourite push notification would result in a 404 (not found) error, since we were redirecting the user to the wrong URL (we were redirecting to /@<user who favourited>/<favourited status id>, when it should be /@<favourited status author>/<favourited status id>)
btrd pushed a commit to btrd/mastodon that referenced this pull request Feb 22, 2023
…on#23286)

Fix a bug where clicking in a favourite push notification would result in a 404 (not found) error, since we were redirecting the user to the wrong URL (we were redirecting to /@<user who favourited>/<favourited status id>, when it should be /@<favourited status author>/<favourited status id>)
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.

None yet

3 participants