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

Fixes for avatar payloads #5921

Merged
merged 1 commit into from
Dec 8, 2022
Merged

Fixes for avatar payloads #5921

merged 1 commit into from
Dec 8, 2022

Conversation

zomars
Copy link
Member

@zomars zomars commented Dec 7, 2022

What does this PR do?

Builds upon #5299

  • Removes data encoded images form viewer.me
  • Refactors and simplifies /api/avatar endpoint
  • Adds new query for rawAvatar to use on update avatar forms

Type of change

  • Bug fix (non-breaking change which fixes an issue)

How should this be tested?

  • Go to Settings > Profile
  • Play around with the form
  • Update avatar

@vercel
Copy link

vercel bot commented Dec 7, 2022

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

Name Status Preview Updated
cal ✅ Ready (Inspect) Visit Preview Dec 7, 2022 at 11:39PM (UTC)

@zomars zomars requested review from alannnc, hariombalhara and a team and removed request for alannnc December 7, 2022 23:34
Copy link
Member Author

@zomars zomars 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

| "timeFormat"
| "metadata"
>;
user: Pick<User, "username" | "metadata">;
Copy link
Member Author

Choose a reason for hiding this comment

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

Picking only needed fields

| "timeFormat"
| "metadata"
>;
user: Pick<User, "username" | "metadata">;
Copy link
Member Author

Choose a reason for hiding this comment

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

Same here

import { getPlaceholderAvatar } from "@calcom/lib/getPlaceholderAvatar";
import prisma from "@calcom/prisma";

import { defaultAvatarSrc } from "@lib/profile";

export default async function handler(req: NextApiRequest, res: NextApiResponse) {
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 endpoint was pretty hard to read, I hope this improves legibility a bit.

const { data: user, isLoading } = trpc.viewer.me.useQuery();
const { data: avatar, isLoading: isLoadingAvatar } = trpc.viewer.avatar.useQuery();
Copy link
Member Author

Choose a reason for hiding this comment

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

We fetch raw avatar in separate endpoint

Comment on lines +82 to +84
utils.viewer.me.invalidate();
utils.viewer.avatar.invalidate();
setTempFormValues(null);
Copy link
Member Author

Choose a reason for hiding this comment

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

To avoid stale data

// TODO: Setting avatar value to /avatar.png(which is a dynamic route) would actually reset the avatar because /avatar.png is supposed to return the value of user.avatar
// if (user.avatar) user.avatar = `${CAL_URL}/${user.username}/avatar.png`;
const avatar = user.avatar || defaultAvatarSrc({ email });
user.avatar = rawAvatar ? `${WEBAPP_URL}/${user.username}/avatar.png` : defaultAvatarSrc({ email });
Copy link
Member Author

Choose a reason for hiding this comment

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

Hopefully this set the nail in the coffin from #5299

@@ -160,7 +158,7 @@ const loggedInViewerRouter = router({
locale: user.locale,
timeFormat: user.timeFormat,
timeZone: user.timeZone,
avatar: `${CAL_URL}/${user.username}/avatar.png`,
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 is already handled in createContext

Comment on lines +179 to +181
avatar: authedProcedure.query(({ ctx }) => ({
avatar: ctx.user.rawAvatar,
})),
Copy link
Member Author

Choose a reason for hiding this comment

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

Basic query to fetch raw avatar data.

@@ -1,12 +1,12 @@
import React, { forwardRef, ReactElement, ReactNode, Ref, useCallback, useId, useState } from "react";
import { Edit2, Eye, EyeOff } from "react-feather";
Copy link
Member Author

Choose a reason for hiding this comment

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

Unused

import { FieldValues, FormProvider, SubmitHandler, useFormContext, UseFormReturn } from "react-hook-form";

import classNames from "@calcom/lib/classNames";
import { getErrorFromUnknown } from "@calcom/lib/errors";
import { useLocale } from "@calcom/lib/hooks/useLocale";

import { Alert, showToast, Icon, Skeleton, Tooltip, UnstyledSelect } from "../../..";
import { Alert, Icon, showToast, Skeleton, Tooltip, UnstyledSelect } from "../../..";
Copy link
Member Author

Choose a reason for hiding this comment

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

Sorted imports

@emrysal emrysal merged commit 3c17bc2 into main Dec 8, 2022
@emrysal emrysal deleted the fixes/avatar-payload branch December 8, 2022 09:50
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.

2 participants