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

chore(rethinkdb): OrganizationUser: Phase 1 #9952

Merged
merged 66 commits into from
Jul 11, 2024
Merged
Show file tree
Hide file tree
Changes from 63 commits
Commits
Show all changes
66 commits
Select commit Hold shift + click to select a range
bfa5c21
chore: read ReflectionGroups from PG
mattkrick May 29, 2024
7ac086e
Merge branch 'master' into chore/retrogroups3
mattkrick May 29, 2024
9844960
Merge branch 'master' into chore/retrogroups3
mattkrick May 30, 2024
62e123c
chore: add PG RetroReflection table
mattkrick May 30, 2024
c38f86c
chore: write to PG RetroReflection table
mattkrick Jun 3, 2024
d318560
fixup: inserts and updates
mattkrick Jun 4, 2024
16843dc
Merge branch 'master' into retroReflection-phase1e
mattkrick Jun 4, 2024
abb3bdf
fix: move to record literal types
mattkrick Jun 4, 2024
9911448
fix types
mattkrick Jun 4, 2024
af03b31
chore: migrate reflections to PG
mattkrick Jun 6, 2024
2a3c0ca
Merge branch 'master' into retroReflection-phase1e
mattkrick Jun 6, 2024
8dfa870
fix: remove hard delete of inactive groups
mattkrick Jun 6, 2024
3b48d22
Merge branch 'retroReflection-phase1e' into chore/retroReflection-phase2
mattkrick Jun 6, 2024
24da04b
fixup: remove unused var
mattkrick Jun 6, 2024
c60e502
Merge branch 'retroReflection-phase1e' into chore/retroReflection-phase2
mattkrick Jun 6, 2024
0d2c7d7
handle escape chars and commas
mattkrick Jun 6, 2024
9ef9346
chore: begin building equality checker
mattkrick Jun 6, 2024
76aeff9
fixup: account for lots of formatting in content
mattkrick Jun 7, 2024
0830c8e
Merge branch 'retroReflection-phase1e' into chore/retroReflection-phase2
mattkrick Jun 24, 2024
a12b08a
fix: rename extra spaces for conversion from plaintext to content
mattkrick Jun 24, 2024
a732657
fix: rename Reactji composite type attributes
mattkrick Jun 24, 2024
954503d
fix: constructor prop in lookup table
mattkrick Jun 25, 2024
c7775ad
handle reactji migration
mattkrick Jun 25, 2024
d53aca4
change dataloaders to pg
mattkrick Jun 25, 2024
6689e39
fix: self-review
mattkrick Jun 25, 2024
a6fd0d3
Merge branch 'master' into chore/retroReflection-phase2
mattkrick Jun 25, 2024
00964a8
Merge branch 'chore/retroReflection-phase2' into chore/retroReflectio…
mattkrick Jun 25, 2024
50ca56b
fix: migration order rename
mattkrick Jun 25, 2024
b220f22
Merge branch 'chore/retroReflection-phase2' into chore/retroReflectio…
mattkrick Jun 25, 2024
22ca4f3
chore: write to PG
mattkrick Jun 25, 2024
9c55acd
chore: migrate old records to PG
mattkrick Jun 25, 2024
9dfd35c
fix: add teamid FK
mattkrick Jun 25, 2024
654aa42
Merge branch 'chore/timeline-phase1' into chore/timeline-phase2
mattkrick Jun 25, 2024
ebbea29
chore: read from pg
mattkrick Jun 25, 2024
7dbea0d
Merge branch 'master' into chore/timeline-phase2
mattkrick Jun 26, 2024
3fd2236
Merge branch 'chore/timeline-phase2' into chore/timeline-phase3
mattkrick Jun 26, 2024
c552ea6
fix bad merge
mattkrick Jun 26, 2024
50d9e77
Merge branch 'chore/timeline-phase2' into chore/timeline-phase3
mattkrick Jun 26, 2024
b7c092d
add migration
mattkrick Jun 26, 2024
fe9efe9
write to pg
mattkrick Jun 27, 2024
4d838f3
Merge branch 'master' into chore/organization-phase1
mattkrick Jun 27, 2024
34b05bf
Merge branch 'master' into chore/organization-phase1
mattkrick Jul 1, 2024
02aa23d
Merge branch 'master' into chore/organization-phase1
mattkrick Jul 1, 2024
dfba81e
Merge branch 'master' into chore/organization-phase1
mattkrick Jul 1, 2024
81d68b7
Merge branch 'master' into chore/organization-phase1
mattkrick Jul 3, 2024
f4f5ede
fix: array append/remove
mattkrick Jul 3, 2024
dcbcd78
chore: migrate existing orgs to PG
mattkrick Jul 3, 2024
b329fd3
remove RethinkDB.Organization
mattkrick Jul 4, 2024
0a549d6
Merge branch 'master' into chore/organization-phase2
mattkrick Jul 4, 2024
5f6bdfc
Merge branch 'chore/organization-phase2' into chore/organization-phase3
mattkrick Jul 4, 2024
21876d9
Merge branch 'master' into chore/organization-phase3
mattkrick Jul 4, 2024
af856f2
fix: isRequestToJoin tests
mattkrick Jul 8, 2024
129d1a9
fix: isOrgVerified
mattkrick Jul 8, 2024
6e368a4
fix deleteUser test
mattkrick Jul 8, 2024
27b0cf2
fix: comment
mattkrick Jul 9, 2024
b5422dd
Merge branch 'master' into chore/organization-phase3
mattkrick Jul 9, 2024
3c061e2
fix: remove newUserUntil
mattkrick Jul 9, 2024
1cdf979
chore: replace half of OrganizationUser instances
mattkrick Jul 9, 2024
473c0c5
remove newUserUntil on client
mattkrick Jul 10, 2024
49be1bc
fix: update all instances of OrganizationUser
mattkrick Jul 10, 2024
65614c6
Merge branch 'master' into chore/organizationUser-phase1
mattkrick Jul 10, 2024
ca01882
fix: bad merge
mattkrick Jul 10, 2024
44d153e
re-add rethinkdb to server tests
mattkrick Jul 10, 2024
3c3b46c
fix tests
mattkrick Jul 10, 2024
3e3634c
fix: use test schema
mattkrick Jul 10, 2024
8d33999
fix: safer queries
mattkrick Jul 10, 2024
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 3 additions & 12 deletions packages/client/components/BillingLeaderActionMenu.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,6 @@ const BillingLeaderActionMenu = (props: Props) => {
graphql`
fragment BillingLeaderActionMenu_organization on Organization {
id
billingTier
}
`,
organizationRef
Expand All @@ -45,7 +44,6 @@ const BillingLeaderActionMenu = (props: Props) => {
graphql`
fragment BillingLeaderActionMenu_organizationUser on OrganizationUser {
role
newUserUntil
user {
id
}
Expand All @@ -54,9 +52,9 @@ const BillingLeaderActionMenu = (props: Props) => {
organizationUserRef
)
const atmosphere = useAtmosphere()
const {id: orgId, billingTier} = organization
const {id: orgId} = organization
const {viewerId} = atmosphere
const {newUserUntil, role, user} = organizationUser
const {role, user} = organizationUser
const isBillingLeader = role === 'BILLING_LEADER'
const {id: userId} = user

Expand All @@ -82,14 +80,7 @@ const BillingLeaderActionMenu = (props: Props) => {
<MenuItem label='Leave Organization' onClick={toggleLeave} />
)}
{viewerId !== userId && (
<MenuItem
label={
billingTier === 'team' && new Date(newUserUntil) > new Date()
? 'Refund and Remove'
: 'Remove from Organization'
}
onClick={toggleRemove}
/>
<MenuItem label={'Remove from Organization'} onClick={toggleRemove} />
)}
</Menu>
</>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,6 @@ import RowInfoHeader from '../../../../components/Row/RowInfoHeader'
import RowInfoHeading from '../../../../components/Row/RowInfoHeading'
import RowInfoLink from '../../../../components/Row/RowInfoLink'
import BaseTag from '../../../../components/Tag/BaseTag'
import EmphasisTag from '../../../../components/Tag/EmphasisTag'
import InactiveTag from '../../../../components/Tag/InactiveTag'
import RoleTag from '../../../../components/Tag/RoleTag'
import {MenuPosition} from '../../../../hooks/useCoords'
Expand Down Expand Up @@ -126,24 +125,21 @@ interface UserInfoProps {
isBillingLeader: boolean
isOrgAdmin: boolean
inactive: boolean | null
newUserUntil: string
}

const UserInfo: React.FC<UserInfoProps> = ({
preferredName,
email,
isBillingLeader,
isOrgAdmin,
inactive,
newUserUntil
inactive
}) => (
<StyledRowInfo>
<RowInfoHeader>
<RowInfoHeading>{preferredName}</RowInfoHeading>
{isBillingLeader && <RoleTag>Billing Leader</RoleTag>}
{isOrgAdmin && <BaseTag className='bg-gold-500 text-white'>Org Admin</BaseTag>}
{inactive && !isBillingLeader && !isOrgAdmin && <InactiveTag>Inactive</InactiveTag>}
{new Date(newUserUntil) > new Date() && <EmphasisTag>New</EmphasisTag>}
</RowInfoHeader>
<RowInfoLink href={`mailto:${email}`} title='Send an email'>
{email}
Expand Down Expand Up @@ -258,7 +254,6 @@ const OrgMemberRow = (props: Props) => {
preferredName
}
role
newUserUntil
...BillingLeaderActionMenu_organizationUser
...OrgAdminActionMenu_organizationUser
}
Expand All @@ -269,7 +264,6 @@ const OrgMemberRow = (props: Props) => {
const {isViewerBillingLeader, isViewerOrgAdmin} = organization

const {
newUserUntil,
user: {email, inactive, picture, preferredName},
role
} = organizationUser
Expand All @@ -291,7 +285,6 @@ const OrgMemberRow = (props: Props) => {
isBillingLeader={isBillingLeader}
isOrgAdmin={isOrgAdmin}
inactive={inactive}
newUserUntil={newUserUntil}
/>
<UserActions
isViewerOrgAdmin={isViewerOrgAdmin}
Expand Down
39 changes: 23 additions & 16 deletions packages/server/billing/helpers/adjustUserCount.ts
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
import {sql} from 'kysely'
import {InvoiceItemType} from 'parabol-client/types/constEnums'
import getRethink from '../../database/rethinkDriver'
import {RDatum} from '../../database/stricterR'
Expand Down Expand Up @@ -36,7 +37,7 @@ const maybeUpdateOrganizationActiveDomain = async (
return

// don't modify if we can't guess the domain or the domain we guess is the current domain
const domain = await getActiveDomainForOrgId(orgId)
const domain = await getActiveDomainForOrgId(orgId, dataLoader)
if (!domain || domain === activeDomain) return
organization.activeDomain = domain
const pg = getKysely()
Expand All @@ -52,13 +53,19 @@ const changePause = (inactive: boolean) => async (_orgIds: string[], user: IUser
email,
isActive: !inactive
})
return Promise.all([
await Promise.all([
updateUser(
{
inactive
},
userId
),
getKysely()
.updateTable('OrganizationUser')
.set({inactive})
.where('userId', '=', userId)
.where('removedAt', 'is', null)
.execute(),
Comment on lines +63 to +68
Copy link
Contributor

Choose a reason for hiding this comment

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

Ensure transactional integrity.

Consider wrapping the database update operations in a transaction to ensure atomicity.

+  const trx = await r.transaction()
+  try {
    await Promise.all([
      updateUser(
        {
          inactive
        },
        userId
      ),
      getKysely()
        .updateTable('OrganizationUser')
        .set({inactive})
        .where('userId', '=', userId)
        .where('removedAt', 'is', null)
        .execute(),
      r
        .table('OrganizationUser')
        .getAll(userId, {index: 'userId'})
        .filter({removedAt: null})
        .update({inactive})
        .run()
    ])
+    await trx.commit()
+  } catch (error) {
+    await trx.rollback()
+    throw error
+  }
Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
getKysely()
.updateTable('OrganizationUser')
.set({inactive})
.where('userId', '=', userId)
.where('removedAt', 'is', null)
.execute(),
const trx = await r.transaction()
try {
await Promise.all([
updateUser(
{
inactive
},
userId
),
getKysely()
.updateTable('OrganizationUser')
.set({inactive})
.where('userId', '=', userId)
.where('removedAt', 'is', null)
.execute(),
r
.table('OrganizationUser')
.getAll(userId, {index: 'userId'})
.filter({removedAt: null})
.update({inactive})
.run()
])
await trx.commit()
} catch (error) {
await trx.rollback()
throw error
}

r
.table('OrganizationUser')
.getAll(userId, {index: 'userId'})
Expand All @@ -73,33 +80,28 @@ const addUser = async (orgIds: string[], user: IUser, dataLoader: DataLoaderWork
const r = await getRethink()
const [rawOrganizations, organizationUsers] = await Promise.all([
dataLoader.get('organizations').loadMany(orgIds),
r
.table('OrganizationUser')
.getAll(userId, {index: 'userId'})
.orderBy(r.desc('newUserUntil'))
.coerceTo('array')
.run()
dataLoader.get('organizationUsersByUserId').load(userId)
])
dataLoader.get('organizationUsersByUserId').clear(userId)
Copy link
Contributor

Choose a reason for hiding this comment

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

Clear the cache after modification.

Ensure that the data loader cache is cleared after modifying the data to avoid stale data issues.

+  dataLoader.get('organizations').clearMany(orgIds)

Committable suggestion was skipped due to low confidence.

const organizations = rawOrganizations.filter(isValid)
const docs = orgIds.map((orgId) => {
const oldOrganizationUser = organizationUsers.find(
(organizationUser) => organizationUser.orgId === orgId
)
const organization = organizations.find((organization) => organization.id === orgId)!
// continue the grace period from before, if any OR set to the end of the invoice OR (if it is a free account) no grace period
const newUserUntil =
(oldOrganizationUser && oldOrganizationUser.newUserUntil) ||
organization.periodEnd ||
new Date()
return new OrganizationUser({
id: oldOrganizationUser?.id,
orgId,
userId,
newUserUntil,
tier: organization.tier
})
})

await getKysely()
.insertInto('OrganizationUser')
.values(docs)
.onConflict((oc) => oc.doNothing())
.execute()
Comment on lines +100 to +104
Copy link
Contributor

Choose a reason for hiding this comment

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

Ensure transactional integrity.

Consider wrapping the database update operations in a transaction to ensure atomicity.

+  const trx = await getKysely().transaction()
+  try {
    await getKysely()
      .insertInto('OrganizationUser')
      .values(docs)
      .onConflict((oc) => oc.doNothing())
      .execute()
    await trx.commit()
  } catch (error) {
    await trx.rollback()
    throw error
  }
Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
await getKysely()
.insertInto('OrganizationUser')
.values(docs)
.onConflict((oc) => oc.doNothing())
.execute()
const trx = await getKysely().transaction()
try {
await getKysely()
.insertInto('OrganizationUser')
.values(docs)
.onConflict((oc) => oc.doNothing())
.execute()
await trx.commit()
} catch (error) {
await trx.rollback()
throw error
}

await r.table('OrganizationUser').insert(docs, {conflict: 'replace'}).run()
await Promise.all(
orgIds.map((orgId) => {
Expand All @@ -111,7 +113,13 @@ const addUser = async (orgIds: string[], user: IUser, dataLoader: DataLoaderWork
const deleteUser = async (orgIds: string[], user: IUser) => {
const r = await getRethink()
orgIds.forEach((orgId) => analytics.userRemovedFromOrg(user, orgId))
return r
await getKysely()
.updateTable('OrganizationUser')
.set({removedAt: sql`CURRENT_TIMESTAMP`})
.where('userId', '=', user.id)
.where('orgId', 'in', orgIds)
.execute()
Comment on lines +116 to +121
Copy link
Contributor

Choose a reason for hiding this comment

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

Ensure transactional integrity.

Consider wrapping the database update operations in a transaction to ensure atomicity.

+  const trx = await getKysely().transaction()
+  try {
    await getKysely()
      .updateTable('OrganizationUser')
      .set({removedAt: sql`CURRENT_TIMESTAMP`})
      .where('userId', '=', user.id)
      .where('orgId', 'in', orgIds)
      .execute()
    await trx.commit()
  } catch (error) {
    await trx.rollback()
    throw error
  }
Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
await getKysely()
.updateTable('OrganizationUser')
.set({removedAt: sql`CURRENT_TIMESTAMP`})
.where('userId', '=', user.id)
.where('orgId', 'in', orgIds)
.execute()
const trx = await getKysely().transaction()
try {
await getKysely()
.updateTable('OrganizationUser')
.set({removedAt: sql`CURRENT_TIMESTAMP`})
.where('userId', '=', user.id)
.where('orgId', 'in', orgIds)
.execute()
await trx.commit()
} catch (error) {
await trx.rollback()
throw error
}

await r
.table('OrganizationUser')
.getAll(user.id, {index: 'userId'})
.filter((row: RDatum) => r.expr(orgIds).contains(row('orgId')))
Expand Down Expand Up @@ -152,7 +160,6 @@ export default async function adjustUserCount(

const dbAction = dbActionTypeLookup[type]
await dbAction(orgIds, user, dataLoader)

const auditEventType = auditEventTypeLookup[type]
await insertOrgUserAudit(orgIds, userId, auditEventType)

Expand Down
14 changes: 5 additions & 9 deletions packages/server/billing/helpers/generateInvoice.ts
Original file line number Diff line number Diff line change
@@ -1,7 +1,6 @@
import {InvoiceItemType} from 'parabol-client/types/constEnums'
import Stripe from 'stripe'
import getRethink from '../../database/rethinkDriver'
import {RDatum} from '../../database/stricterR'
import Coupon from '../../database/types/Coupon'
import Invoice, {InvoiceStatusEnum} from '../../database/types/Invoice'
import {InvoiceLineItemEnum} from '../../database/types/InvoiceLineItem'
Expand Down Expand Up @@ -353,16 +352,13 @@ export default async function generateInvoice(
invoice.status === 'paid' && invoice.status_transitions.paid_at
? fromEpochSeconds(invoice.status_transitions.paid_at)
: undefined
const [organization, billingLeaderIds] = await Promise.all([
const [organization, orgUsers] = await Promise.all([
dataLoader.get('organizations').loadNonNull(orgId),
r
.table('OrganizationUser')
.getAll(orgId, {index: 'orgId'})
.filter({removedAt: null})
.filter((row: RDatum) => r.expr(['BILLING_LEADER', 'ORG_ADMIN']).contains(row('role')))
.coerceTo('array')('userId')
.run() as any as string[]
dataLoader.get('organizationUsersByOrgId').load(orgId)
])
const billingLeaderIds = orgUsers
.filter(({role}) => role && ['BILLING_LEADER', 'ORG_ADMIN'].includes(role))
.map(({userId}) => userId)

const billingLeaders = (await dataLoader.get('users').loadMany(billingLeaderIds)).filter(isValid)
const billingLeaderEmails = billingLeaders.map((user) => user.email)
Expand Down
Original file line number Diff line number Diff line change
@@ -1,5 +1,3 @@
import getRethink from '../../database/rethinkDriver'
import {RDatum} from '../../database/stricterR'
import {DataLoaderWorker} from '../../graphql/graphql'
import {OrganizationSource} from '../../graphql/public/types/Organization'
import {analytics} from '../../utils/analytics/analytics'
Expand All @@ -9,31 +7,23 @@ const sendEnterpriseOverageEvent = async (
organization: OrganizationSource,
dataLoader: DataLoaderWorker
) => {
const r = await getRethink()
const manager = getStripeManager()
const {id: orgId, stripeSubscriptionId} = organization
if (!stripeSubscriptionId) return
const [orgUserCount, subscriptionItem] = await Promise.all([
r
.table('OrganizationUser')
.getAll(orgId, {index: 'orgId'})
.filter({removedAt: null, inactive: false})
.count()
.run(),
const [orgUsers, subscriptionItem] = await Promise.all([
dataLoader.get('organizationUsersByOrgId').load(orgId),
manager.getSubscriptionItem(stripeSubscriptionId)
])
const activeOrgUsers = orgUsers.filter(({inactive}) => !inactive)
const orgUserCount = activeOrgUsers.length
if (!subscriptionItem) return

const quantity = subscriptionItem.quantity
if (!quantity) return
if (orgUserCount > quantity) {
const billingLeaderOrgUser = await r
.table('OrganizationUser')
.getAll(orgId, {index: 'orgId'})
.filter({removedAt: null})
.filter((row: RDatum) => r.expr(['BILLING_LEADER', 'ORG_ADMIN']).contains(row('role')))
.nth(0)
.run()
const billingLeaderOrgUser = orgUsers.find(
({role}) => role && ['BILLING_LEADER', 'ORG_ADMIN'].includes(role)
Copy link
Contributor

Choose a reason for hiding this comment

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

Ensure proper error handling in promise chain.

The catch block in the sendEnterpriseOverageEvent invocation is empty. It is advisable to handle errors or log them for debugging purposes.

-    sendEnterpriseOverageEvent(org, dataLoader).catch()
+    sendEnterpriseOverageEvent(org, dataLoader).catch((error) => {
+      console.error(`Failed to send overage event for org ${org.id}:`, error);
+    });
Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
const billingLeaderOrgUser = orgUsers.find(
const billingLeaderOrgUser = orgUsers.find(
// Other code here
sendEnterpriseOverageEvent(org, dataLoader).catch((error) => {
console.error(`Failed to send overage event for org ${org.id}:`, error);
});

)!
const {id: userId} = billingLeaderOrgUser
const user = await dataLoader.get('users').loadNonNull(userId)
analytics.enterpriseOverUserLimit(user, orgId)
Expand Down
14 changes: 14 additions & 0 deletions packages/server/billing/helpers/teamLimitsCheck.ts
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,13 @@ import sendTeamsLimitEmail from './sendTeamsLimitEmail'

const enableUsageStats = async (userIds: string[], orgId: string) => {
const pg = getKysely()
await pg
.updateTable('OrganizationUser')
.set({suggestedTier: 'team'})
.where('orgId', '=', orgId)
.where('userId', 'in', userIds)
.where('removedAt', 'is', null)
.execute()
Comment on lines +24 to +30
Copy link
Contributor

Choose a reason for hiding this comment

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

Ensure transactional integrity.

Consider wrapping the database update operations in a transaction to ensure atomicity.

+  const trx = await pg.transaction()
+  try {
    await pg
      .updateTable('OrganizationUser')
      .set({suggestedTier: 'team'})
      .where('orgId', '=', orgId)
      .where('userId', 'in', userIds)
      .where('removedAt', 'is', null)
      .execute()
    await pg
      .updateTable('User')
      .set({featureFlags: sql`arr_append_uniq("featureFlags", 'insights')`})
      .where('id', 'in', userIds)
      .execute()
+    await trx.commit()
+  } catch (error) {
+    await trx.rollback()
+    throw error
+  }

Committable suggestion was skipped due to low confidence.

await r
.table('OrganizationUser')
.getAll(r.args(userIds), {index: 'userId'})
Expand Down Expand Up @@ -87,6 +94,13 @@ export const maybeRemoveRestrictions = async (orgId: string, dataLoader: DataLoa
.set({tierLimitExceededAt: null, scheduledLockAt: null, lockedAt: null})
.where('id', '=', orgId)
.execute(),
pg
.updateTable('OrganizationUser')
.set({suggestedTier: 'starter'})
.where('orgId', '=', orgId)
.where('userId', 'in', billingLeadersIds)
.where('removedAt', 'is', 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.

added this because it was erroneously missing in the rethinkdb query

.execute(),
Comment on lines +97 to +103
Copy link
Contributor

Choose a reason for hiding this comment

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

Ensure transactional integrity.

Similar to the previous comment, consider wrapping the database update operations in a transaction to ensure atomicity.

+  const trx = await pg.transaction()
+  try {
    await pg
      .updateTable('Organization')
      .set({tierLimitExceededAt: null, scheduledLockAt: null, lockedAt: null})
      .where('id', '=', orgId)
      .execute()
    await pg
      .updateTable('OrganizationUser')
      .set({suggestedTier: 'starter'})
      .where('orgId', '=', orgId)
      .where('userId', 'in', billingLeadersIds)
      .where('removedAt', 'is', null)
      .execute()
    await trx.commit()
  } catch (error) {
    await trx.rollback()
    throw error
  }
Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
pg
.updateTable('OrganizationUser')
.set({suggestedTier: 'starter'})
.where('orgId', '=', orgId)
.where('userId', 'in', billingLeadersIds)
.where('removedAt', 'is', null)
.execute(),
const trx = await pg.transaction()
try {
await pg
.updateTable('Organization')
.set({tierLimitExceededAt: null, scheduledLockAt: null, lockedAt: null})
.where('id', '=', orgId)
.execute()
await pg
.updateTable('OrganizationUser')
.set({suggestedTier: 'starter'})
.where('orgId', '=', orgId)
.where('userId', 'in', billingLeadersIds)
.where('removedAt', 'is', null)
.execute()
await trx.commit()
} catch (error) {
await trx.rollback()
throw error
}

r
.table('OrganizationUser')
.getAll(r.args(billingLeadersIds), {index: 'userId'})
Expand Down
21 changes: 18 additions & 3 deletions packages/server/billing/helpers/updateSubscriptionQuantity.ts
Original file line number Diff line number Diff line change
Expand Up @@ -11,9 +11,10 @@ import {getStripeManager} from '../../utils/stripe'
*/
const updateSubscriptionQuantity = async (orgId: string, logMismatch?: boolean) => {
const r = await getRethink()
const pg = getKysely()
Copy link
Contributor

Choose a reason for hiding this comment

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

Avoid redundant database connections

The code is opening a connection to RethinkDB but doesn't seem to use it. Consider removing the redundant connection.

-  const r = await getRethink()

Committable suggestion was skipped due to low confidence.

const manager = getStripeManager()

const org = await getKysely()
const org = await pg
.selectFrom('Organization')
.selectAll()
.where('id', '=', orgId)
Expand All @@ -34,15 +35,29 @@ const updateSubscriptionQuantity = async (orgId: string, logMismatch?: boolean)
return
}

const [orgUserCount, teamSubscription] = await Promise.all([
const [orgUserCountRes, orgUserCount, teamSubscription] = await Promise.all([
pg
.selectFrom('OrganizationUser')
.select(({fn}) => fn.count<number>('id').as('count'))
.where('orgId', '=', orgId)
.where('removedAt', 'is', null)
.where('inactive', '=', false)
.executeTakeFirstOrThrow(),
Comment on lines +38 to +45
Copy link
Contributor

Choose a reason for hiding this comment

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

Remove redundant RethinkDB query

The code is querying both PostgreSQL and RethinkDB for the same data. Since the transition is towards using PostgreSQL, remove the RethinkDB query.

    const [orgUserCountRes, teamSubscription] = await Promise.all([
      pg
        .selectFrom('OrganizationUser')
        .select(({fn}) => fn.count<number>('id').as('count'))
        .where('orgId', '=', orgId)
        .where('removedAt', 'is', null)
        .where('inactive', '=', false)
        .executeTakeFirstOrThrow(),
-      r
-        .table('OrganizationUser')
-        .getAll(orgId, {index: 'orgId'})
-        .filter({removedAt: null, inactive: false})
-        .count()
-        .run(),
      manager.getSubscriptionItem(stripeSubscriptionId)
    ])
Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
const [orgUserCountRes, orgUserCount, teamSubscription] = await Promise.all([
pg
.selectFrom('OrganizationUser')
.select(({fn}) => fn.count<number>('id').as('count'))
.where('orgId', '=', orgId)
.where('removedAt', 'is', null)
.where('inactive', '=', false)
.executeTakeFirstOrThrow(),
const [orgUserCountRes, teamSubscription] = await Promise.all([
pg
.selectFrom('OrganizationUser')
.select(({fn}) => fn.count<number>('id').as('count'))
.where('orgId', '=', orgId)
.where('removedAt', 'is', null)
.where('inactive', '=', false)
.executeTakeFirstOrThrow(),
manager.getSubscriptionItem(stripeSubscriptionId)
])

r
.table('OrganizationUser')
.getAll(orgId, {index: 'orgId'})
.filter({removedAt: null, inactive: false})
.count()
.run(),
await manager.getSubscriptionItem(stripeSubscriptionId)
manager.getSubscriptionItem(stripeSubscriptionId)
])
if (orgUserCountRes.count !== orgUserCount) {
sendToSentry(new Error('OrganizationUser count mismatch'), {
tags: {
orgId
}
})
}
if (
teamSubscription &&
teamSubscription.quantity !== undefined &&
Expand Down
20 changes: 3 additions & 17 deletions packages/server/database/types/OrganizationUser.ts
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,6 @@ export type OrgUserRole = 'BILLING_LEADER' | 'ORG_ADMIN'
interface Input {
orgId: string
userId: string
newUserUntil?: Date
id?: string
inactive?: boolean
joinedAt?: Date
Expand All @@ -20,36 +19,23 @@ export default class OrganizationUser {
suggestedTier: TierEnum | null
inactive: boolean
joinedAt: Date
newUserUntil: Date
orgId: string
removedAt: Date | null
role: OrgUserRole | null
userId: string
tier: TierEnum | null
tier: TierEnum
trialStartDate?: Date | null

constructor(input: Input) {
const {
suggestedTier,
userId,
id,
removedAt,
inactive,
orgId,
joinedAt,
newUserUntil,
role,
tier
} = input
const {suggestedTier, userId, id, removedAt, inactive, orgId, joinedAt, role, tier} = input
this.id = id || generateUID()
this.suggestedTier = suggestedTier || null
this.inactive = inactive || false
this.joinedAt = joinedAt || new Date()
this.newUserUntil = newUserUntil || new Date()
this.orgId = orgId
this.removedAt = removedAt || null
this.role = role || null
this.userId = userId
this.tier = tier || null
this.tier = tier || 'starter'
}
}
Loading
Loading