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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Bug: Avatar is either not shown or is not the user #1529

Closed
1 of 2 tasks
adiati98 opened this issue Aug 8, 2023 · 7 comments 路 Fixed by #1537
Closed
1 of 2 tasks

Bug: Avatar is either not shown or is not the user #1529

adiati98 opened this issue Aug 8, 2023 · 7 comments 路 Fixed by #1537

Comments

@adiati98
Copy link
Member

adiati98 commented Aug 8, 2023

Describe the bug

I opened the notification and the avatars of the same user are either empty or not the user.

Screenshot

avatar

(I'm sure the first avatar is not @BekahHW 馃榿)

Steps to reproduce

  1. Click the notification icon.
  2. See the avatars of the users.

Browsers

Chrome

Additional context (Is this in dev or production?)

No response

Code of Conduct

  • I agree to follow this project's Code of Conduct

Contributing Docs

  • I agree to follow this project's Contribution Docs
@adiati98 adiati98 added 馃悰 bug Something isn't working 馃憖 needs triage labels Aug 8, 2023
@adiati98 adiati98 changed the title Bug: Avatar either not shown or is not the user Bug: Avatar is either not shown or is not the user Aug 8, 2023
@jpmcb
Copy link
Member

jpmcb commented Aug 9, 2023

In your case, I think what's happening is that Bekah is reacting to different highlights (notice the different numbers) you've posted but since the profile pictures aren't being loaded, it looks odd.

I noticed something similar where people's profile pictures aren't being loaded even though I know they have images (like bdougie, martin, etc)

Screenshot 2023-08-08 at 8 10 59 PM

@bdougie
Copy link
Member

bdougie commented Aug 9, 2023

This error may due to the notification meta_id being used here. We have access to the user id, but we cache users based on usernames.

We will probably need an API update to include username.

https://github.com/open-sauced/insights/pull/1478/files#diff-2524a786a0bcb5deadfbb244b5d5ef18304b923e055b9f64c1dd9fd0c695a592R158

We will also don't need to add the meta_id to the message. Weird seeing 239 BekahHW did a thing

@takanome-dev
Copy link
Contributor

We will probably need an API update to include username.

Agree

We will also don't need to add the meta_id to the message. Weird seeing 239 BekahHW did a thing

Indeed, but we need to include the highlight_id though. Should we keep meta_id which can be the username or highlight_id and add a username property?

cc @brandonroberts

@bdougie
Copy link
Member

bdougie commented Aug 9, 2023

In my example above 239 is the highlight ID. The user does not need to see that in the text, to be clear keeping that in the response from the API is expected.

There is no value with having 239 in my notification. That needs to be removed in the UI.

This should be a straight forward API update. Could you open the issue @takanome-dev so we discuss there.

@brandonroberts
Copy link
Contributor

In my example above 239 is the highlight ID. The user does not need to see that in the text, to be clear keeping that in the response from the API is expected.

There is no value with having 239 in my notification. That needs to be removed in the UI.

This should be a straight forward API update. Could you open the issue @takanome-dev so we discuss there.

Yes, we can add "from" user information to each notification in the API.

Here is an example
https://github.com/open-sauced/api/blob/beta/src/user/user.entity.ts#L522
https://github.com/open-sauced/api/blob/beta/src/user/entities/user-collaboration.entity.ts#L53

The meta_id field should be used along with the type to build the URL of where it should link to, which is what we had in the UI before with the notification bell.

@open-sauced
Copy link

open-sauced bot commented Aug 15, 2023

馃帀 This issue has been resolved in version 1.60.0-beta.10 馃帀

The release is available on GitHub release

Your semantic-release bot 馃摝馃殌

@open-sauced
Copy link

open-sauced bot commented Aug 16, 2023

馃帀 This issue has been resolved in version 1.60.0 馃帀

The release is available on GitHub release

Your semantic-release bot 馃摝馃殌

@open-sauced open-sauced bot added the released label Aug 16, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants