Skip to content

chore: Chore team metadata table + isOrganization migration from metadata#12828

Merged
keithwillcode merged 59 commits into
mainfrom
chore-team-metadata-table
Jan 16, 2024
Merged

chore: Chore team metadata table + isOrganization migration from metadata#12828
keithwillcode merged 59 commits into
mainfrom
chore-team-metadata-table

Conversation

@sean-brydon
Copy link
Copy Markdown
Member

@sean-brydon sean-brydon commented Dec 16, 2023

What does this PR do?

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)
  • Tests (Unit/Integration/E2E or any other test)
  • This change requires a documentation update

How should this be tested?

  1. Seed the database
  2. Connect to DB
  3. UPDATE "Team" SET "metadata" = '{"isOrganization": "true", "isOrganizationConfigured": "true", "orgAutoAcceptEmail": "example@example.com", "isOrganizationVerified": "false"}'
    WHERE "id" in (1,2,3);
    UPDATE "Team" SET "isOrganization" = true WHERE "id" in (1,2,3);
  4. Run SQL above to set metadata
  5. Run migrations - ensure migrations have ran successfully and each team in the above query has a OrganizationSettings table associated with it

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
  • 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

@vercel
Copy link
Copy Markdown

vercel Bot commented Dec 16, 2023

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

Name Status Preview Comments Updated (UTC)
api ✅ Ready (Inspect) Visit Preview 💬 Add feedback Jan 16, 2024 10:08am
dev ✅ Ready (Inspect) Visit Preview 💬 Add feedback Jan 16, 2024 10:08am
5 Ignored Deployments
Name Status Preview Comments Updated (UTC)
ai ⬜️ Ignored (Inspect) Visit Preview Jan 16, 2024 10:08am
cal ⬜️ Ignored (Inspect) Visit Preview Jan 16, 2024 10:08am
cal-demo ⬜️ Ignored (Inspect) Visit Preview Jan 16, 2024 10:08am
qa ⬜️ Ignored (Inspect) Visit Preview Jan 16, 2024 10:08am
ui ⬜️ Ignored (Inspect) Visit Preview Jan 16, 2024 10:08am

@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented Dec 16, 2023

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

@github-actions github-actions Bot added the ❗️ migrations contains migration files label Dec 16, 2023
@zomars zomars added the core area: core, team members only label Dec 16, 2023
Comment on lines +24 to +45
-- Set team field to notify if it is an organization -> Easier than metadata.parse(X).isOrganization
UPDATE "Team"
SET "isOrganization" = (metadata ->> 'isOrganization')::BOOLEAN
WHERE
metadata ->> 'isOrganization' = 'true';

-- Insert data into org settings
INSERT INTO "OrganizationSettings" ("teamId", "isOrganizationConfigured", "orgAutoAcceptEmail", "isOrganizationVerified")
SELECT
t.id,
(t.metadata ->> 'isOrganizationConfigured')::BOOLEAN AS "isOrganizationConfigured",
t.metadata ->> 'orgAutoAcceptEmail' AS "orgAutoAcceptEmail",
(t.metadata ->> 'isOrganizationVerified')::BOOLEAN AS "isOrganizationVerified"
FROM (
SELECT
id,
metadata
FROM
"Team"
WHERE
metadata ->> 'isOrganization' = 'true'
) AS t; No newline at end of file
Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

important:

Here we are just updating a table - we are not removing or deleting this data from metadata FOR NOW.

Once we have ran this custom migration we can consider removing the existing fields in prod db to ensure we dont have duplicated data.

@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented Dec 16, 2023

📦 Next.js Bundle Analysis for @calcom/web

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

Three Pages Changed Size

The following pages changed size from the code in this PR compared to its base branch:

Page Size (compressed) First Load % of Budget (350 KB)
/insights 465.62 KB 630.43 KB 180.12% (🟢 -2.15%)
/settings/teams/[id]/members 372.63 KB 537.43 KB 153.55% (🟢 -1.96%)
/workflows 278.42 KB 443.22 KB 126.64% (🟢 -2.13%)
Details

Only the gzipped size is provided here based on an expert tip.

First Load is the size of the global bundle plus the bundle for the individual page. If a user were to show up to your website and land on a given page, the first load size represents the amount of javascript that user would need to download. If next/link is used, subsequent page loads would only need to download that page's bundle (the number in the "Size" column), since the global bundle has already been downloaded.

Any third party scripts you have added directly to your app using the <script> tag are not accounted for in this analysis

The "Budget %" column shows what percentage of your performance budget the First Load total takes up. For example, if your budget was 100kb, and a given page's first load size was 10kb, it would be 10% of your budget. You can also see how much this has increased or decreased compared to the base branch of your PR. If this percentage has increased by 20% or more, there will be a red status indicator applied, indicating that special attention should be given to this. If you see "+/- <0.01%" it means that there was a change in bundle size, but it is a trivial enough amount that it can be ignored.

Comment on lines -52 to -55
metadata: {
...metaDataParsed,
isOrganizationVerified: true,
orgAutoAcceptEmail: acceptedEmailDomain,
},
Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

We no longer need to update metadata for this as it is handled in organizationSettings

Comment on lines +36 to +38
const team = await getTeamOrThrow(input.teamId);

const isOrg = team.isOrganization;
Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Switched this order around so we can get if it is an organization based on teamId

@ThyMinimalDev can you confirm this is the correct logic here?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

makes sense, probably better to check if the team is an org from database rathe than relying on input parameter

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

@ThyMinimalDev given the changes in inviteHandler do these changes look fine to you?

@sean-brydon sean-brydon requested review from emrysal and zomars January 8, 2024 11:26

const nonOrgTeams = user.teams
.filter((membership) => !isOrganization({ team: membership.team }))
.filter((membership) => !membership.team.isOrganization)
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I don't see a reason to switch away from isOrganization

Suggested change
.filter((membership) => !membership.team.isOrganization)
.filter((membership) => !isOrganization({team:membership.team})

Comment thread packages/trpc/server/routers/viewer/organizations/adminUpdate.handler.ts Outdated
Comment thread packages/trpc/server/routers/viewer/organizations/adminUpdate.handler.ts Outdated
sean-brydon and others added 2 commits January 16, 2024 22:39
…handler.ts

Co-authored-by: Hariom Balhara <hariombalhara@gmail.com>
…handler.ts

Co-authored-by: Hariom Balhara <hariombalhara@gmail.com>
Comment thread packages/trpc/server/routers/viewer/organizations/create.handler.ts Outdated
hariombalhara
hariombalhara previously approved these changes Jan 16, 2024
Copy link
Copy Markdown
Member

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

Awesome work @sean-brydon 🙏 Thanks for the PR

@zomars
Copy link
Copy Markdown
Contributor

zomars commented Jan 16, 2024

Heads up. the migration in this PR is failing on prod. Blocking the latest release.

CREATE UNIQUE INDEX "OrganizationSettings_organizationId_key" ON "OrganizationSettings"("organizationId");

-- CreateIndex
CREATE UNIQUE INDEX "Team_slug_isOrganization_key" ON "Team"("slug", "isOrganization");
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

ERROR: could not create unique index "Team_slug_isOrganization_key"
DETAIL: Key (slug, "isOrganization")=(advisors, f) is duplicated.

@zomars
Copy link
Copy Markdown
Contributor

zomars commented Jan 16, 2024

@calcom/foundation @sean-brydon had to revert this PR due to migration not being compatible with actual prod data. Causing it to fail.

@zomars
Copy link
Copy Markdown
Contributor

zomars commented Jan 16, 2024

From internal chat:

... parentId isn't being taking into account for the new unique constrain
just slug and isOrganization
So one of those is a subteam and another one is a stantalone team (non-org) with slug "advisors". Both have metadata→isOrganization as null/false. The former because is a subteam in an org and the latter because is not on an org. Hence the conflict.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

5 points Created by SyncLinear.com consumer core area: core, team members only Medium priority Created by Linear-GitHub Sync ❗️ migrations contains migration files

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants