fix: reviewed-user-sms-lock by TaduJR · Pull Request #16016 · calcom/cal.diy

TaduJR · 2024-07-31T22:44:11Z

What does this PR do?

Fixes [CAL-4077] Already reviewed users/teams shouldn't be marked for review again (SMS sending) #15987
Fixes CAL-4077

Mandatory Tasks (DO NOT REMOVE)

I have self-reviewed the code (A decent size PR without self-review might be rejected).
I have added a Docs issue here if this PR makes changes that would require a documentation change. If N/A, write N/A here and check the checkbox.
I confirm automated tests are in place that prove my fix is effective or that my feature works.

How should this be tested?

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

Checklist

I haven't commented my code, particularly in hard-to-understand areas

vercel · 2024-07-31T22:44:17Z

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

A member of the Team first needs to authorize it.

graphite-app · 2024-07-31T22:45:11Z

Graphite Automations

"Add community label" took an action on this PR • (07/31/24)

1 label was added to this PR based on Keith Williams's automation.

"Add consumer team as reviewer" took an action on this PR • (07/31/24)

1 reviewer was added to this PR based on Keith Williams's automation.

Amit91848

Thanks for the PR @TaduJR , but I guess you are misunderstanding the issue. Your changes doesn't solve the issue.

https://github.com/calcom/cal.com/blob/89c08868f6c069b1d8ceb163e3d9b5e7c24d3879/packages/lib/checkRateLimitAndThrowError.ts#L35-L46

This is where accounts are flagged and set for review. What we want is if an account was previously marked as reviewed and later unlocked by an admin, we do not want to mark it for review again

Amit91848 · 2024-08-01T02:23:30Z

@@ -16,6 +16,20 @@ type GetOptions = {
 const setSMSLockState = async ({ input }: GetOptions) => {


This mutation is only called inside apps/web/pages/settings/admin/lockedSMS/lockedSMSView.tsx where admin can mark the user or team as LOCKED or UNLOCKED only. These changes aren't required

anikdhabal · 2024-08-02T03:46:04Z

+    const userToUpdate = await prisma.user.findUnique({
+      where: {
+        id: userId,
+      },
+    });
+    if (!userToUpdate) {
+      throw new TRPCError({ code: "BAD_REQUEST", message: "User not found" });
+    }
+    if (userToUpdate.smsLockState === SMSLockState.UNLOCKED) {
+      throw new TRPCError({
+        code: "BAD_REQUEST",
+        message: "Re-review for unlocked user or team is not allowed",
+      });
+    }


Also, this is not correct, you checked whether the user's smslockstate is unlocked or not, but all users smslockstate UNLOCKED by default in the database.

TaduJR · 2024-08-02T11:08:35Z

Hello @Amit91848, and @anikdhabal. Thank you so much for your comments and review. Now I fully understand the requirement. I will work on it.

TaduJR · 2024-08-03T09:19:23Z

Hello @Amit91848. I have made a new push. But I have one question? As you can see I am throwing a trpc error for re-review sms. I think the trpc error is shown as toast on the frontend. Should continue on with the current approach?

Amit91848

Hello @Amit91848. I have made a new push. But I have one question? As you can see I am throwing a trpc error for re-review sms. I think the trpc error is shown as toast on the frontend. Should continue on with the current approach?

Hey @TaduJR , this is close but we don't want to throw errors as it will halt the process. If admin has reviewed and unlocked the team or user, that means it is safe and does not need to be reviewed or locked again.

Amit91848 · 2024-08-04T16:00:08Z

-  smsLockState       SMSLockState        @default(UNLOCKED)
+  OutOfOfficeReasons           OutOfOfficeReason[]
+  smsLockState                 SMSLockState        @default(UNLOCKED)
+  isSMSLockStateReviewedBefore Boolean             @default(false)


NIT: Better variable naming

Suggested change

isSMSLockStateReviewedBefore Boolean @default(false)

reviewedByAdmin Boolean @default(false)

TaduJR · 2024-08-04T19:03:18Z

Thank you so much @Amit91848.

Amit91848

Left some suggestions. Please make changes accordingly @TaduJR

Amit91848 · 2024-08-05T08:05:23Z


  if (userId) {
+    const user = await prisma.user.findUnique({ where: { id: userId, profiles: { none: {} } } });
+    if (status === SMSLockState.REVIEW_NEEDED && user?.smsLockReviewedByAdmin) return;


Suggested change

if (status === SMSLockState.REVIEW_NEEDED && user?.smsLockReviewedByAdmin) return;

if (status === SMSLockState.UNLOCKED && user?.smsLockReviewedByAdmin) return;

If it was unlocked by admin then we don't need to mark it for review or locked again

I think this one needs to be if (user?.smsLockReviewedByAdmin) return; by removing SMSLockState.UNLOCKED condition because as you can see on changeSMSRateLimit function, the status (rate limiting type) is either LOCKED or REVIEW_NEED so no need to check for UNLOCKED.

Amit91848 · 2024-08-05T08:05:38Z

+    const team = await prisma.team.findUnique({
+      where: { id: teamId, parentId: null, isOrganization: false },
+    });
+    if (status === SMSLockState.REVIEW_NEEDED && team?.smsLockReviewedByAdmin) return;


Suggested change

if (status === SMSLockState.REVIEW_NEEDED && team?.smsLockReviewedByAdmin) return;

if (status === SMSLockState.UNLOCKED && team?.smsLockReviewedByAdmin) return;

Same here

Same for this also

Amit91848 · 2024-08-05T08:07:59Z

+    if (!userToUpdate) throw new TRPCError({ code: "BAD_REQUEST", message: "User not found" });
+    const userUpdateInput = {
+      smsLockState: lock ? SMSLockState.LOCKED : SMSLockState.UNLOCKED,
+      ...(userToUpdate.smsLockState === SMSLockState.REVIEW_NEEDED && !lock
+        ? { smsLockReviewedByAdmin: true }
+        : {}),
+    };


This is unnecessary. setSMSLockState is a privileged endpoint only accessible by admin, so you just have to update the reviewedByAdmin to true.

smsLockReviewedByAdmin means the LockState whether Reviewe_Needed, unlocked locked etc is being done by admin, so we will just mark it true no matter the state.

Amit91848 · 2024-08-05T08:10:12Z

-      data: {
-        smsLockState: lock ? SMSLockState.LOCKED : SMSLockState.UNLOCKED,
-      },
+      data: userUpdateInput,


Suggested change

data: userUpdateInput,

data: {

smsLockState: lock ? SMSLockState.LOCKED : SMSLockState.UNLOCKED,

smsLockReviewedByAdmin: true,

},

setSMSLockState is a privileged endpoint only accessible by admin, so you just have to update the reviewedByAdmin to true.

Amit91848 · 2024-08-05T08:13:55Z

+    if (!teamToUpdate) throw new TRPCError({ code: "BAD_REQUEST", message: "Team not found" });
+    const teamUpdateInput = {
+      smsLockState: lock ? SMSLockState.LOCKED : SMSLockState.UNLOCKED,
+      ...(teamToUpdate.smsLockState === SMSLockState.REVIEW_NEEDED && !lock
+        ? { smsLockReviewedByAdmin: true }
+        : {}),
+    };
+    const updatedTeam = await prisma.team.update({
+      where: {
+        id: teamId,
      },
+      data: teamUpdateInput,
    });


Amit91848 · 2024-08-05T08:14:04Z

+    if (!teamToUpdate) throw new TRPCError({ code: "BAD_REQUEST", message: "Team not found" });
+    const teamUpdateInput = {
+      smsLockState: lock ? SMSLockState.LOCKED : SMSLockState.UNLOCKED,
+      ...(teamToUpdate.smsLockState === SMSLockState.REVIEW_NEEDED && !lock
+        ? { smsLockReviewedByAdmin: true }
+        : {}),
+    };
+    const updatedTeam = await prisma.team.update({
+      where: {
+        id: teamToUpdate.id,
+      },
+      data: teamUpdateInput,
+    });
+    return { name: updatedTeam.slug, locked: lock };


Amit91848

LGTM! Tested and works 🚀
Thanks @TaduJR

TaduJR · 2024-08-12T05:56:09Z

My pleasure @Amit91848.

graphite-app Bot added the community Created by Linear-GitHub Sync label Jul 31, 2024

graphite-app Bot requested a review from a team July 31, 2024 22:44

github-actions Bot added teams area: teams, round robin, collective, managed event-types 🐛 bug Something isn't working 🧹 Improvements Improvements to existing features. Mostly UX/UI labels Jul 31, 2024

dosubot Bot added the admin label Jul 31, 2024

TaduJR changed the title ~~fix: inital commit~~ fix: reviewed-user-sms-lock Jul 31, 2024

Amit91848 requested changes Aug 1, 2024

View reviewed changes

anikdhabal reviewed Aug 2, 2024

View reviewed changes

throw trpc error on re-review for sms

0c4ef91

TaduJR force-pushed the fix-reviewed-user-sms-lock branch from 875fbb5 to 0c4ef91 Compare August 3, 2024 08:58

github-actions Bot added the ❗️ migrations contains migration files label Aug 3, 2024

Amit91848 requested changes Aug 4, 2024

View reviewed changes

TaduJR added 2 commits August 4, 2024 21:26

update logic

abbf950

test

6e6866e

TaduJR requested a review from Amit91848 August 4, 2024 19:03

Amit91848 requested changes Aug 5, 2024

View reviewed changes

update logic

6fd6edc

TaduJR requested review from Amit91848 and anikdhabal August 5, 2024 10:54

Amit91848 approved these changes Aug 12, 2024

View reviewed changes

Amit91848 added the ready-for-e2e label Aug 12, 2024

Merge branch 'main' into fix-reviewed-user-sms-lock

1ca5412

CarinaWolli merged commit ad62f9d into calcom:main Aug 12, 2024

		@@ -16,6 +16,20 @@ type GetOptions = {
		const setSMSLockState = async ({ input }: GetOptions) => {

	isSMSLockStateReviewedBefore Boolean @default(false)
	reviewedByAdmin Boolean @default(false)

	if (status === SMSLockState.REVIEW_NEEDED && user?.smsLockReviewedByAdmin) return;
	if (status === SMSLockState.UNLOCKED && user?.smsLockReviewedByAdmin) return;

Conversation

TaduJR commented Jul 31, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What does this PR do?

Mandatory Tasks (DO NOT REMOVE)

How should this be tested?

Checklist

Uh oh!

vercel Bot commented Jul 31, 2024

Uh oh!

graphite-app Bot commented Jul 31, 2024

Graphite Automations

Uh oh!

Amit91848 left a comment • 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!

Amit91848 Aug 1, 2024 • 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!

anikdhabal Aug 2, 2024 • 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!

TaduJR commented Aug 2, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

TaduJR commented Aug 3, 2024

Uh oh!

Amit91848 left a comment • 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!

Uh oh!

TaduJR commented Aug 4, 2024

Uh oh!

Amit91848 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

TaduJR Aug 5, 2024 • 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!

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!

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!

Amit91848 left a comment

Choose a reason for hiding this comment

Uh oh!

TaduJR commented Aug 12, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

TaduJR commented Jul 31, 2024 •

edited

Loading

Amit91848 left a comment •

edited

Loading

Amit91848 Aug 1, 2024 •

edited

Loading

anikdhabal Aug 2, 2024 •

edited

Loading

TaduJR commented Aug 2, 2024 •

edited

Loading

Amit91848 left a comment •

edited

Loading

TaduJR Aug 5, 2024 •

edited

Loading