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 don't update permalinks with name changes #3234

Open
GeorgeColdham opened this issue Jun 19, 2019 · 5 comments
Open

Notifications don't update permalinks with name changes #3234

GeorgeColdham opened this issue Jun 19, 2019 · 5 comments
Assignees
Labels
area: comments & notifications area concerning interactions or notifications with user bug always open for contribution external contributors welcome contribution is welcome!

Comments

@GeorgeColdham
Copy link

Describe the bug
When a notification is received, the link to the article/comment is using the same permalink as can be used to reference it.
If a user changes their name this permalink is broken and leads to a 404 page.

To Reproduce

  1. Generate a notification somehow
  2. Change the username of the notification sender
  3. Try following the link

Expected behavior
The link should still work and lead to the Article or comment.

Screenshots
Notification with url
Screenshot 2019-06-19 at 12 42 19
Following the notification
Screenshot 2019-06-19 at 12 45 16
Comment on article. NOTE: Username change
Screenshot 2019-06-19 at 12 44 29
Permalink url of comment after name change
Screenshot 2019-06-19 at 12 44 50

Desktop (please complete the following information):

  • OS: Mac OS Mojave 10.14.5
  • Browser: Google Chrome
  • Version: 75

Suggestion
My suggestion for a fix, would be to have accounts linked to a uniquely generated id, allowing the names to be changed independently of any permalinks. The username could redirect to the unique id version of the url.

@Zhao-Andy
Copy link
Contributor

Thanks for the bug report! I think I'll most likely work on this. Our current approach is to show static data to prevent excessive queries to the database, and my guess is that the notification data is not up-to-date after the user changes their username.

@jessleenyc jessleenyc added the external contributors welcome contribution is welcome! label Jul 30, 2019
@citizen428
Copy link
Contributor

@Zhao-Andy Did you ever get around to work on this? If not, do you want me to take over?

@Zhao-Andy
Copy link
Contributor

Ah nope, feel free to go ahead! I believe this is still an issue but not 100% sure

@cmgorton cmgorton added this to To do in OSS Rotation: Triage stale issues/pr via automation Nov 30, 2020
@ludwiczakpawel ludwiczakpawel moved this from To do to Keep this issue/pr in OSS Rotation: Triage stale issues/pr Jan 25, 2021
@cmgorton cmgorton removed this from Keep this issue/pr in OSS Rotation: Triage stale issues/pr Apr 1, 2021
@rhymes rhymes self-assigned this Apr 8, 2021
@rhymes
Copy link
Contributor

rhymes commented Apr 12, 2021

When notifications are created, we store some information in the notification object, as a JSON payload:

json_data = {
user: user_data(comment.user),
comment: comment_data(comment)
}

This information contains user_data and comment_data. user_data in particular contains information about the user's name and username, the comment's and article's paths:

name: user.name,
username: user.username,

path: comment.path,

path: comment.commentable.path,

This information, stored in notification.json_data is then used to display the HTML of the notification in /notifications:

see https://github.com/forem/forem/blob/master/app/views/notifications/_comment.html.erb

I imagine that when a user changes username, all the notifications they are the source of are not updated with the changed username, and as a consequence, the paths of all the articles.

@rhymes
Copy link
Contributor

rhymes commented May 7, 2021

Hi @cmgorton, I'm reopening this based on #13663 and the PR that was submit for it: #13689

#13663 and its PR only focus on the dashboard list of articles, not the full experience (notifications included)

@rhymes rhymes reopened this May 7, 2021
@cmgorton cmgorton added area: comments & notifications area concerning interactions or notifications with user and removed area: notifications labels Nov 9, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: comments & notifications area concerning interactions or notifications with user bug always open for contribution external contributors welcome contribution is welcome!
Projects
None yet
Development

No branches or pull requests

7 participants