feat: update user profile #3223
Conversation
Co-authored-by: Ole-Martin Bratteng <[email protected]>
Co-authored-by: Ole-Martin Bratteng <[email protected]>
|
🍹 The Update (preview) for dailydotdev/api/prod (at 508e87c) was successful. Resource Changes Name Type Operation
+ vpc-native-api-db-migration-51f53d00 kubernetes:batch/v1:Job create
~ vpc-native-personalized-digest-deployment kubernetes:apps/v1:Deployment update
~ vpc-native-private-deployment kubernetes:apps/v1:Deployment update
~ vpc-native-deployment kubernetes:apps/v1:Deployment update
~ vpc-native-bg-deployment kubernetes:apps/v1:Deployment update
~ vpc-native-update-tag-recommendations-cron kubernetes:batch/v1:CronJob update
- vpc-native-api-clickhouse-migration-9d29c5e3 kubernetes:batch/v1:Job delete
+- vpc-native-k8s-secret kubernetes:core/v1:Secret create-replacement
~ vpc-native-ws-deployment kubernetes:apps/v1:Deployment update
~ vpc-native-clean-gifted-plus-cron kubernetes:batch/v1:CronJob update
~ vpc-native-hourly-notification-cron kubernetes:batch/v1:CronJob update
~ vpc-native-update-trending-cron kubernetes:batch/v1:CronJob update
~ vpc-native-clean-zombie-images-cron kubernetes:batch/v1:CronJob update
~ vpc-native-calculate-top-readers-cron kubernetes:batch/v1:CronJob update
~ vpc-native-update-current-streak-cron kubernetes:batch/v1:CronJob update
~ vpc-native-personalized-digest-cron kubernetes:batch/v1:CronJob update
~ vpc-native-generate-search-invites-cron kubernetes:batch/v1:CronJob update
~ vpc-native-validate-active-users-cron kubernetes:batch/v1:CronJob update
~ vpc-native-update-source-public-threshold-cron kubernetes:batch/v1:CronJob update
~ vpc-native-clean-stale-user-transactions-cron kubernetes:batch/v1:CronJob update
~ vpc-native-update-views-cron kubernetes:batch/v1:CronJob update
~ vpc-native-clean-zombie-users-cron kubernetes:batch/v1:CronJob update
- vpc-native-api-db-migration-9d29c5e3 kubernetes:batch/v1:Job delete
~ vpc-native-sync-subscription-with-cio-cron kubernetes:batch/v1:CronJob update
~ vpc-native-update-source-tag-view-cron kubernetes:batch/v1:CronJob update
~ vpc-native-post-analytics-history-day-clickhouse-cron kubernetes:batch/v1:CronJob update
~ vpc-native-update-tags-str-cron kubernetes:batch/v1:CronJob update
~ vpc-native-check-analytics-report-cron kubernetes:batch/v1:CronJob update
~ vpc-native-daily-digest-cron kubernetes:batch/v1:CronJob update
~ vpc-native-generic-referral-reminder-cron kubernetes:batch/v1:CronJob update
+ vpc-native-api-clickhouse-migration-51f53d00 kubernetes:batch/v1:Job create
~ vpc-native-clean-zombie-user-companies-cron kubernetes:batch/v1:CronJob update
~ vpc-native-post-analytics-clickhouse-cron kubernetes:batch/v1:CronJob update
~ vpc-native-temporal-deployment kubernetes:apps/v1:Deployment update
~ vpc-native-update-highlighted-views-cron kubernetes:batch/v1:CronJob update
|
|
|
||
| const cloudinaryUrl = process.env.CLOUDINARY_URL || null; | ||
|
|
||
| const [avatar, cover] = await Promise.all([ |
There was a problem hiding this comment.
I've not actually tested that this work, since uploading images locally doesn't work. Not sure what the best way to test this is E2E wise ? 🤔
There was a problem hiding this comment.
Generic question, why are we adding cover and readme to this mutation when we already have separate workable mutations for cover and readme?
There was a problem hiding this comment.
if only to call in a single call, we can merge mutations on client side and use existing ones
mutation {
updateUserProfile() {}
uploadCoverImage() {}
updateReadme() {}
}
There was a problem hiding this comment.
Generic question, why are we adding cover and readme to this mutation when we already have separate workable mutations for cover and readme?
I mention it in the description, but in general the new form is being re-designed with react-hook-form, and I'd like to take the opportunity to build good form practices. It's not great that some parts of the form are "autosave" while others require you to click the save button, all in the same form.
As for your second comment: First of all I didn't know you could combo mutations like that ? 😅 Do we have any samples of this? Wonder what responses and errors look like
But secondly, and more importantly, I think its better and cleaner to just do it all within the same endpoint, since its all handling the same form, rather than multiple.
There was a problem hiding this comment.
But the fact that we already have it I would rather just use what we have, I don't want duplicates when we can easily do it through graphql
There was a problem hiding this comment.
Errors wise you would get errors the same way as you would with single one, errors in gql response are array, so you would get multiple
There was a problem hiding this comment.
@AmarTrebinjac one example of Ante's suggestion is in graphql/users.ts - the query USER_READING_HISTORY_QUERY but this time, just make the query a mutation then it should work.
The response is similar to any query/mutation we have; the object will contain a property for each caller, in the image below, the response would be something like:
interface Response {
userReadingRankHistory: object;
userReadHistory: object;
// and so on..
}
You can even put conditionals if image exists, then you execute upload mutation.
There was a problem hiding this comment.
was this change, to unify cover and readme mentioned anywhere in the RFC?
There was a problem hiding this comment.
What about when the user is just trying to update name? Would this clear the cover? If not, how would you clear the user's cover if the user is trying to clear it?
@sshanzel No, it would not clear it if the user is specifically not clearing it.
The logic for clearing is if there is a coverUpload, or if there's an existing image, but its not in the form, we clear
if ((!data.cover || !!coverUpload) && user.cover) {
filesToClear.push(
clearFile({
referenceId: user.id,
preset: UploadPreset.ProfileCover,
}),
);
}
@capJavert No, it was not in the RFC.
As for the broader question, I concede that, had I known about this graphql pairing, doing it that way might have worked as well. That's on me.
But now that I've updated it this way, I don't really see the benefit of undoing it and going back? This endpoint already handled uploading avatars, I just added the logic for cover + clearing.
Also, wouldn't this multi-request potentially still cause partial updates? The image requests succeed, but the validation of the form fields doesn't, so now the image is updated, but rest of the user is not? Sounds like we'd just be adding more logic to the frontend.
There was a problem hiding this comment.
I don't think you even have to clear it because we replace the file itself. The filename ends up tied to a certain pattern.
AmarTrebinjac
requested review from
ilasw,
nensidosari,
omBratteng,
rebelchris and
sshanzel
and removed request for
a team
src/types.ts
Outdated
| garmr: GarmrService; | ||
| }; | ||
|
|
||
| export type Location = { |
There was a problem hiding this comment.
Location is a reserved word due to window.Location - it might produce some confusion down the line 😞
There was a problem hiding this comment.
Also, I think you can just use the DatasetLocation and pick the props you need.
There was a problem hiding this comment.
yeah, let's name it something like TLocation
|
|
||
| const cloudinaryUrl = process.env.CLOUDINARY_URL || null; | ||
|
|
||
| const [avatar, cover] = await Promise.all([ |
There was a problem hiding this comment.
Generic question, why are we adding cover and readme to this mutation when we already have separate workable mutations for cover and readme?
src/types.ts
Outdated
| garmr: GarmrService; | ||
| }; | ||
|
|
||
| export type Location = { |
There was a problem hiding this comment.
yeah, let's name it something like TLocation
| export type TLocation = { | ||
| id: string; | ||
| country: string; | ||
| subdivision: string | null; | ||
| city: string | null; | ||
| }; |
There was a problem hiding this comment.
Where is this used? Doesn't seem to be applied anywhere.
There was a problem hiding this comment.
In LoggedInBoot
|
Although I would've liked it if we had converted this with Zod. I'm kinda obsessed with it lately. |
| (async () => { | ||
| if (upload && cloudinaryUrl) { | ||
| const file = await upload; | ||
| return (await uploadAvatar(user.id, file.createReadStream())).url; | ||
| } | ||
| return data.image || null; | ||
| })(), | ||
| (async () => { |
There was a problem hiding this comment.
what are these inline async calls, lets change them up, if we are merging mutations at the end you could just export the current mutation logic as helper and then user it here so its streamlined
3 participants
Jira ticket
Made changes to updateUserProfile to conform with the new user profile. This includes the addition of readme, uploading cover photo, as well as logic for deleting removed photos in the same endpoint, rather than being a separate action, as we want the user to hit "save" to actually change their profile, rather than have some actions be auto, others not.
Also added the new
locationIdcolumn to user, andlocationto boot.MI-1067