feat: Add user's avatar to POST and PATCH requests in users endpoints by HamzaFouad · Pull Request #11187 · calcom/cal.diy

HamzaFouad · 2023-09-06T21:40:40Z

What does this PR do?

Adds avatar to POST and PATCH requests in users endpoint
Update users docs

Requirement/Documentation

If there is a requirement document, please, share it here.
If there is ab UI/UX design document, please, share it here.

Type of change

New feature (non-breaking change which adds functionality)

How should this be tested?

Are there environment variables that should be set?
What are the minimal test data to have?
What is expected (happy path) to have (input and output)?
Any other important info that could help to test that PR

Mandatory Tasks

Make sure you have self-reviewed the code. A decent size PR without self-review might be rejected.

Checklist

I haven't added tests that prove my fix is effective or that my feature works
I haven't checked if new and existing unit tests pass locally with my changes

vercel · 2023-09-06T21:40:45Z

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

Name	Status	Preview	Comments	Updated (UTC)
ui	✅ Ready (Inspect)	Visit Preview	💬 Add feedback	Oct 9, 2023 4:37am

vercel · 2023-09-06T21:40:46Z

@HamzaFouad is attempting to deploy a commit to the cal Team on Vercel.

A member of the Team first needs to authorize it.

CLAassistant · 2023-09-06T21:40:47Z

All committers have signed the CLA.

github-actions · 2023-09-06T21:40:52Z

Thank you for following the naming conventions! 🙏 Feel free to join our discord and post your PR link to collect XP and win prizes!

github-actions · 2023-09-06T21:48:16Z

📦 Next.js Bundle Analysis for @calcom/web

This analysis was generated by the Next.js Bundle Analysis action. 🤖

This PR introduced no changes to the JavaScript bundle! 🙌

github-actions · 2023-09-22T00:15:44Z

This PR is being marked as stale due to inactivity.

Stale

ThyMinimalDev · 2023-10-12T12:37:06Z

+  try {
+    let base64Data = input;
+    // Check if the input starts with a data URI scheme (data:content/type;base64,...)
+    if (input.startsWith("data:")) {
+      const parts = input.split(",");
+      if (parts.length === 2) {
+        // metadata and base64Data
+        base64Data = parts[1]; // Extract the base64 data from the second part
+      }
+    }
+    const buffer = Buffer.from(base64Data, "base64").toString("base64");
+    return buffer === base64Data;
+  } catch (error) {
+    return false;
+  }


couldn't we replace this whole thing with a regexp ?

Also I think the naming of the function is unclear, we are here checking if the data inside of the data-uri string is encoded in base64.

Sure we can. but why do yo prefer using regex over a native lib?

I tried to name the extension method to be as generic as possible to accommodate all base64-encoding-string checks, not just for images.

HamzaFouad · 2023-10-22T12:06:37Z

@alishaz-polymath Could you please take the initiative and help this PR to move forward? It has been stale for very long time with no progress in reviews.

alishaz-polymath · 2023-10-22T13:24:45Z

@alishaz-polymath Could you please take the initiative and help this PR to move forward? It has been stale for very long time with no progress in reviews.

I'll review it this week. Thank you @HamzaFouad 🙏

alishaz-polymath

Requested a few changes to improve strictness and efficiency.

Please do test locally to confirm they work and adjust accordingly. The rest looks good to me 🎉 , but I'll wait on the requested changes. 🙏

alishaz-polymath · 2023-10-25T11:20:58Z

@@ -0,0 +1,17 @@
+export function isValidBase64(input: string): boolean {
+  try {


Suggested change

try {

const base64Regex = /^(?:[A-Za-z0-9+/]{4})*(?:[A-Za-z0-9+/]{2}==|[A-Za-z0-9+/]{3}=)?$/;

try {

Can potentially use a regex here for strict checking for base64
This pattern assumes a character set of A-Z, a-z, 0-9, +, / and possibly ending with = or ==.

alishaz-polymath · 2023-10-25T11:24:41Z

+      if (parts.length === 2) {
+        // metadata and base64Data
+        base64Data = parts[1]; // Extract the base64 data from the second part
+      }


Suggested change

if (parts.length === 2) {

// metadata and base64Data

base64Data = parts[1]; // Extract the base64 data from the second part

}

if (parts.length !== 2) return false;

base64Data = parts[1]; // Extract the base64 data from the second part

We can return false when parts length !== 2 to not go about further computations as it would be false already.

I made this condition to not just cover base64-images, but also to cover any base64 string checks you might have in the app, so the string may be a single part or multiple part based on the usecase.

if we are only going to make this method exclusive to base64-images then there is no need for the above check
and we can tweak the regex you implemented and implement the method similar to that:

export function isValidBase64Image(input: string): boolean { const regex = /^data:image\/[^;]+;base64,(?:[A-Za-z0-9+/]{4})*(?:[A-Za-z0-9+/]{2}==|[A-Za-z0-9+/]{3}=)?$/; return regex.test(input); }

we will also rename the method to be image exclusive isValidBase64Image

alishaz-polymath · 2023-10-25T11:30:34Z

+        base64Data = parts[1]; // Extract the base64 data from the second part
+      }
+    }
+    const buffer = Buffer.from(base64Data, "base64").toString("base64");


Suggested change

const buffer = Buffer.from(base64Data, "base64").toString("base64");

if (!base64Regex.test(base64Data)) return false;

Buffer.from(base64Data, "base64");

return true;

We can use regex to test and return false if it doesn't adhere to the regex pattern. The next line would fail if not base64, at which point it should return false thanks to catch.

I think we no longer need to use this line too anymore, right?
Buffer.from(base64Data, "base64");

as the condition before it should be enough to cover base64 validity

Actually we still do.

The regex test checks if the string looks like valid base64

The buffer conversion checks if the string can be successfully decoded as base64

how so?
if it is a valid base64 string, then by definition, it can be decoded into binary data. Or am I missing something?

if we needed to use Buffer.from(base64Data, "base64") then why do we need the regex check at all.
we could have passed the string to the buffer, as I did at first, and if it is decodable then it passes the validation and if not we catch the exception and return false.

@ThyMinimalDev
What do you think?
The regex is fairly strict so there shouldn't be many strings that pass the regex and fail Base64, but what is the safer approach here?

I Think using a regexp is the best approach here, I don't think there is a bulletproof way to make sure that the base64 string is actually a valid image, but as long as it's a valid base64 it is fine.

alishaz-polymath · 2023-10-25T11:31:01Z

+      }
+    }
+    const buffer = Buffer.from(base64Data, "base64").toString("base64");
+    return buffer === base64Data;


Suggested change

return buffer === base64Data;

Don't need to recheck it now that all checks are already done, this is redundant.

…HamzaFouad/cal.com into api/user-avatar-post-and-patch-requests

alishaz-polymath

LGTM 🚀 🚀

ThyMinimalDev · 2023-11-07T09:59:08Z

 import { _UserModel as User } from "@calcom/prisma/zod";
 import { iso8601 } from "@calcom/prisma/zod-utils";

+import { isValidBase64Image } from "~/lib/utils/isValidBase64";


missed this during review, this file is now named isValidBase64Image.ts not isValidBase64.ts

…#11187) Co-authored-by: Syed Ali Shahbaz <52925846+alishaz-polymath@users.noreply.github.com> Co-authored-by: Peer Richelsen <peeroke@gmail.com> ## What does this PR do?  Fix wrong import introduced in this pr: #11187  ## Requirement/Documentation  - If there is a requirement document, please, share it here. - If there is ab UI/UX design document, please, share it here. ## Type of change  - [ ] Bug fix (non-breaking change which fixes an issue) - [ ] Chore (refactoring code, technical debt, workflow improvements) - [ ] New feature (non-breaking change which adds functionality) - [ ] Breaking change (fix or feature that would cause existing functionality to not work as expected) - [ ] This change requires a documentation update ## How should this be tested?  - Are there environment variables that should be set? - What are the minimal test data to have? - What is expected (happy path) to have (input and output)? - Any other important info that could help to test that PR ## Mandatory Tasks - [ ] Make sure you have self-reviewed the code. A decent size PR without self-review might be rejected. ## Checklist  - I haven't read the [contributing guide](https://github.com/calcom/cal.com/blob/main/CONTRIBUTING.md) - My code doesn't follow the style guidelines of this project - I haven't commented my code, particularly in hard-to-understand areas - I haven't checked if my PR needs changes to the documentation - I haven't checked if my changes generate no new warnings - I haven't added tests that prove my fix is effective or that my feature works - I haven't checked if new and existing unit tests pass locally with my changes

…#11187) (#12256) Co-authored-by: Syed Ali Shahbaz <52925846+alishaz-polymath@users.noreply.github.com> Co-authored-by: Peer Richelsen <peeroke@gmail.com> ## What does this PR do?  Fix wrong import introduced in this pr: #11187  ## Requirement/Documentation  - If there is a requirement document, please, share it here. - If there is ab UI/UX design document, please, share it here. ## Type of change  - [ ] Bug fix (non-breaking change which fixes an issue) - [ ] Chore (refactoring code, technical debt, workflow improvements) - [ ] New feature (non-breaking change which adds functionality) - [ ] Breaking change (fix or feature that would cause existing functionality to not work as expected) - [ ] This change requires a documentation update ## How should this be tested?  - Are there environment variables that should be set? - What are the minimal test data to have? - What is expected (happy path) to have (input and output)? - Any other important info that could help to test that PR ## Mandatory Tasks - [ ] Make sure you have self-reviewed the code. A decent size PR without self-review might be rejected. ## Checklist  - I haven't read the [contributing guide](https://github.com/calcom/cal.com/blob/main/CONTRIBUTING.md) - My code doesn't follow the style guidelines of this project - I haven't commented my code, particularly in hard-to-understand areas - I haven't checked if my PR needs changes to the documentation - I haven't checked if my changes generate no new warnings - I haven't added tests that prove my fix is effective or that my feature works - I haven't checked if new and existing unit tests pass locally with my changes

…#11187) Co-authored-by: Syed Ali Shahbaz <52925846+alishaz-polymath@users.noreply.github.com> Co-authored-by: Peer Richelsen <peeroke@gmail.com>

…#11187) (#12256) Co-authored-by: Syed Ali Shahbaz <52925846+alishaz-polymath@users.noreply.github.com> Co-authored-by: Peer Richelsen <peeroke@gmail.com> ## What does this PR do?  Fix wrong import introduced in this pr: #11187  ## Requirement/Documentation  - If there is a requirement document, please, share it here. - If there is ab UI/UX design document, please, share it here. ## Type of change  - [ ] Bug fix (non-breaking change which fixes an issue) - [ ] Chore (refactoring code, technical debt, workflow improvements) - [ ] New feature (non-breaking change which adds functionality) - [ ] Breaking change (fix or feature that would cause existing functionality to not work as expected) - [ ] This change requires a documentation update ## How should this be tested?  - Are there environment variables that should be set? - What are the minimal test data to have? - What is expected (happy path) to have (input and output)? - Any other important info that could help to test that PR ## Mandatory Tasks - [ ] Make sure you have self-reviewed the code. A decent size PR without self-review might be rejected. ## Checklist  - I haven't read the [contributing guide](https://github.com/calcom/cal.com/blob/main/CONTRIBUTING.md) - My code doesn't follow the style guidelines of this project - I haven't commented my code, particularly in hard-to-understand areas - I haven't checked if my PR needs changes to the documentation - I haven't checked if my changes generate no new warnings - I haven't added tests that prove my fix is effective or that my feature works - I haven't checked if new and existing unit tests pass locally with my changes

HamzaFouad added 2 commits September 7, 2023 00:33

add avatar in user's post and patch requests

952a460

update users api docs

81f4b7e

HamzaFouad changed the title ~~Api/user avatar post and patch requests~~ feat: Add User's avatar to POST and PATCH requests in users endpoints Sep 6, 2023

HamzaFouad changed the title ~~feat: Add User's avatar to POST and PATCH requests in users endpoints~~ feat: Add user's avatar to POST and PATCH requests in users endpoints Sep 6, 2023

vercel Bot deployed to Preview – ui September 6, 2023 21:44 View deployment

Merge branch 'main' into api/user-avatar-post-and-patch-requests

0019bc2

vercel Bot deployed to Preview – ui September 6, 2023 21:56 View deployment

PeerRich added ✨ feature New feature or request Low priority Created by Linear-GitHub Sync labels Sep 7, 2023

Merge branch 'main' into api/user-avatar-post-and-patch-requests

0e19f19

vercel Bot deployed to Preview – ui September 7, 2023 17:00 View deployment

github-actions Bot added the Stale label Sep 22, 2023

keithwillcode previously requested changes Sep 26, 2023

View reviewed changes

Comment thread apps/api/lib/validations/user.ts Outdated

github-actions Bot removed the Stale label Sep 27, 2023

validate that avatar is a base64-string

c674e78

vercel Bot deployed to Preview – ui September 27, 2023 08:32 View deployment

cleanup

561cfdf

vercel Bot deployed to Preview – ui September 27, 2023 08:39 View deployment

Merge branch 'main' into api/user-avatar-post-and-patch-requests

46e0804

vercel Bot deployed to Preview – ui September 27, 2023 11:14 View deployment

Merge branch 'main' into api/user-avatar-post-and-patch-requests

83c15d3

vercel Bot deployed to Preview – ui October 4, 2023 09:05 View deployment

alishaz-polymath requested a review from keithwillcode October 5, 2023 11:01

vercel Bot deployed to Preview – ui October 6, 2023 12:33 View deployment

Merge branch 'main' into api/user-avatar-post-and-patch-requests

48db721

vercel Bot deployed to Preview – ui October 9, 2023 04:37 View deployment

ThyMinimalDev reviewed Oct 12, 2023

View reviewed changes

alishaz-polymath requested changes Oct 25, 2023

View reviewed changes

Merge branch 'main' into api/user-avatar-post-and-patch-requests

bcf42e5

alishaz-polymath requested review from ThyMinimalDev and Udit-takkar October 25, 2023 11:35

HamzaFouad added 3 commits October 27, 2023 00:02

refactor isValidBase64Image to use regex

ac6bdc8

Merge branch 'api/user-avatar-post-and-patch-requests' of github.com:…

f1b7356

…HamzaFouad/cal.com into api/user-avatar-post-and-patch-requests

rename file

7b3d5d4

alishaz-polymath approved these changes Oct 30, 2023

View reviewed changes

Merge branch 'main' into api/user-avatar-post-and-patch-requests

8288ed4

PeerRich enabled auto-merge (squash) October 31, 2023 19:40

PeerRich added 2 commits October 31, 2023 19:44

Merge branch 'main' into api/user-avatar-post-and-patch-requests

12eebe0

Merge branch 'main' into api/user-avatar-post-and-patch-requests

fa4f54c

ThyMinimalDev approved these changes Nov 1, 2023

View reviewed changes

ThyMinimalDev and others added 4 commits November 1, 2023 11:44

Merge branch 'main' into api/user-avatar-post-and-patch-requests

85f07e3

Merge branch 'main' into api/user-avatar-post-and-patch-requests

d9deec6

Merge branch 'main' into api/user-avatar-post-and-patch-requests

7133fc5

Merge branch 'main' into api/user-avatar-post-and-patch-requests

fd00625

PeerRich merged commit e94c030 into calcom:main Nov 7, 2023

ThyMinimalDev reviewed Nov 7, 2023

View reviewed changes

ThyMinimalDev mentioned this pull request Nov 7, 2023

fix: wrong import isValidBase64Image #12256

Merged

6 tasks

		@@ -0,0 +1,17 @@
		export function isValidBase64(input: string): boolean {
		try {

-  try {
+  const base64Regex = /^(?:[A-Za-z0-9+/]{4})*(?:[A-Za-z0-9+/]{2}==|[A-Za-z0-9+/]{3}=)?$/;
+    try {

-    const buffer = Buffer.from(base64Data, "base64").toString("base64");
+    if (!base64Regex.test(base64Data)) return false;
+    Buffer.from(base64Data, "base64");
+    return true;

Conversation

HamzaFouad commented Sep 6, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What does this PR do?

Requirement/Documentation

Type of change

How should this be tested?

Mandatory Tasks

Checklist

Uh oh!

vercel Bot commented Sep 6, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

vercel Bot commented Sep 6, 2023

Uh oh!

CLAassistant commented Sep 6, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

github-actions Bot commented Sep 6, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

github-actions Bot commented Sep 6, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

📦 Next.js Bundle Analysis for @calcom/web

Uh oh!

github-actions Bot commented Sep 22, 2023

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

HamzaFouad Oct 15, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

HamzaFouad commented Oct 22, 2023

Uh oh!

alishaz-polymath commented Oct 22, 2023

Uh oh!

alishaz-polymath left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

HamzaFouad Oct 26, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

HamzaFouad Oct 27, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alishaz-polymath left a comment

Choose a reason for hiding this comment

Uh oh!

ThyMinimalDev Nov 7, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

HamzaFouad commented Sep 6, 2023 •

edited

Loading

vercel Bot commented Sep 6, 2023 •

edited

Loading

CLAassistant commented Sep 6, 2023 •

edited

Loading

github-actions Bot commented Sep 6, 2023 •

edited

Loading

github-actions Bot commented Sep 6, 2023 •

edited

Loading

HamzaFouad Oct 15, 2023 •

edited

Loading

HamzaFouad Oct 26, 2023 •

edited

Loading

HamzaFouad Oct 27, 2023 •

edited

Loading

ThyMinimalDev Nov 7, 2023 •

edited

Loading