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 avatar infinite redirect #5299

Merged
merged 6 commits into from
Nov 1, 2022
Merged

Fix avatar infinite redirect #5299

merged 6 commits into from
Nov 1, 2022

Conversation

hariombalhara
Copy link
Member

@hariombalhara hariombalhara commented Oct 31, 2022

  • Don't set avatar to dynamic avatar endpoint in session because that would endup overriding user avatar
  • Avoid infinite redirect. If avatar is set to the dynamic avatar route, consider serving the default route.

Fixes #5213

Steps to replicate the issue

  • Simply, go to the settings profile page and save without changing the avatar.
    • Because getUserFromSession now returns /avatar.png endpoint, it get's saved to DB.

@vercel
Copy link

vercel bot commented Oct 31, 2022

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Updated
cal ❌ Failed (Inspect) Nov 1, 2022 at 6:25AM (UTC)

Copy link
Member Author

@hariombalhara hariombalhara left a comment

Choose a reason for hiding this comment

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

Self review done

apps/web/pages/api/user/avatar.ts Show resolved Hide resolved
@@ -11,6 +12,7 @@ export default async function handler(req: NextApiRequest, res: NextApiResponse)
const username = req.query.username as string;
const teamname = req.query.teamname as string;
let identity;
let linksToThisRoute = false;
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
let linksToThisRoute = false;
let isAvatarSourceWebAppUrl = false;

Copy link
Member Author

Choose a reason for hiding this comment

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

This might not be a better variable name considering it can be WEBSITE_URL

Copy link
Contributor

@alannnc alannnc left a comment

Choose a reason for hiding this comment

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

Couldn't replicate from instructions, some NIT changes.

@kodiakhq kodiakhq bot merged commit b3ba89c into main Nov 1, 2022
@kodiakhq kodiakhq bot deleted the fix/avatar-infinite-redirect branch November 1, 2022 06:30
@hariombalhara hariombalhara mentioned this pull request Nov 5, 2022
1 task
haffla pushed a commit to tourlane/cal.com that referenced this pull request Nov 22, 2022
* If due to some reason avatar URL is same as route, avoid infinite redirection by serving default

* Fix avatar reverting issue

Co-authored-by: Peer Richelsen <[email protected]>
Co-authored-by: alannnc <[email protected]>
Co-authored-by: kodiakhq[bot] <49736102+kodiakhq[bot]@users.noreply.github.com>
haffla pushed a commit to tourlane/cal.com that referenced this pull request Nov 22, 2022
* If due to some reason avatar URL is same as route, avoid infinite redirection by serving default

* Fix avatar reverting issue

Co-authored-by: Peer Richelsen <[email protected]>
Co-authored-by: alannnc <[email protected]>
Co-authored-by: kodiakhq[bot] <49736102+kodiakhq[bot]@users.noreply.github.com>
@zomars
Copy link
Member

zomars commented Dec 7, 2022

The problem with this is that we're exceeding the payload limits due we're returning the whole image as codified data. We should query another endpoint for avatars specifically.

@zomars zomars mentioned this pull request Dec 7, 2022
1 task
@PeerRich PeerRich added the core area: core, team members only label Jul 24, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
automerge ♻️ autoupdate tells kodiak to keep this branch up-to-date core area: core, team members only
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[CAL-252] Avatar icon not loading properly
5 participants