chore(rethinkdb): OrganizationUser: Phase 1 #9952
Conversation
Signed-off-by: Matt Krick <[email protected]>
Signed-off-by: Matt Krick <[email protected]>
Signed-off-by: Matt Krick <[email protected]>
Signed-off-by: Matt Krick <[email protected]>
Signed-off-by: Matt Krick <[email protected]>
Signed-off-by: Matt Krick <[email protected]>
Signed-off-by: Matt Krick <[email protected]>
Signed-off-by: Matt Krick <[email protected]>
Signed-off-by: Matt Krick <[email protected]>
Signed-off-by: Matt Krick <[email protected]>
Signed-off-by: Matt Krick <[email protected]>
Signed-off-by: Matt Krick <[email protected]>
Signed-off-by: Matt Krick <[email protected]>
Signed-off-by: Matt Krick <[email protected]>
Signed-off-by: Matt Krick <[email protected]>
Signed-off-by: Matt Krick <[email protected]>
Signed-off-by: Matt Krick <[email protected]>
Signed-off-by: Matt Krick <[email protected]>
Signed-off-by: Matt Krick <[email protected]>
Signed-off-by: Matt Krick <[email protected]>
…n-phase3 Signed-off-by: Matt Krick <[email protected]>
Signed-off-by: Matt Krick <[email protected]>
Signed-off-by: Matt Krick <[email protected]>
Signed-off-by: Matt Krick <[email protected]>
Signed-off-by: Matt Krick <[email protected]>
Signed-off-by: Matt Krick <[email protected]>
Signed-off-by: Matt Krick <[email protected]>
Signed-off-by: Matt Krick <[email protected]>
Signed-off-by: Matt Krick <[email protected]>
Signed-off-by: Matt Krick <[email protected]>
Signed-off-by: Matt Krick <[email protected]>
Signed-off-by: Matt Krick <[email protected]>
WalkthroughThe recent updates focus on removing the Changes
Sequence Diagram(s)sequenceDiagram
participant User as User
participant Frontend as Frontend
participant Backend as Backend
participant DB as Database
User->>Frontend: Initiate Billing Action
Frontend->>Backend: Request Billing Leader Actions
Backend->>DB: Fetch Organization Data
DB-->>Backend: Return Organization Data (without billingTier, newUserUntil)
Backend->>Frontend: Return Billing Actions
Frontend-->>User: Display Billing Actions
Tip You can make your PR comments more assertiveChange the Recent review detailsConfiguration used: CodeRabbit UI Files selected for processing (6)
Files skipped from review as they are similar to previous changes (4)
Additional comments not posted (7)
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
Signed-off-by: Matt Krick <[email protected]>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 26
Outside diff range, codebase verification and nitpick comments (18)
packages/server/utils/getActiveDomainForOrgId.ts (1)
1-2: Add a brief comment for imports.Adding a brief comment explaining the purpose of the imports can improve readability.
+ // Importing DataLoaderInstance for fetching data and getKysely for PostgreSQL operations import {DataLoaderInstance} from '../dataloader/RootDataLoader' import getKysely from '../postgres/getKysely'packages/server/graphql/queries/helpers/countTiersForUserId.ts (1)
1-1: Add a brief comment for import.Adding a brief comment explaining the purpose of the import can improve readability.
+ // Importing DataLoaderInstance for fetching data import {DataLoaderInstance} from '../../../dataloader/RootDataLoader'packages/server/utils/setTierForOrgUsers.ts (1)
15-15: Add a brief comment for import.Adding a brief comment explaining the purpose of the import can improve readability.
+ // Importing getKysely for PostgreSQL operations const pg = getKysely()packages/server/graphql/private/queries/suOrgCount.ts (1)
7-20: PostgreSQL query logic addedThe new PostgreSQL query logic seems correct and improves efficiency by removing the need for direct database queries. However, the
console.log(pgResults)statement should be removed or replaced with proper logging before deploying to production.- console.log(pgResults) + // console.log(pgResults) // Uncomment for debugging purposespackages/server/safeMutations/safeArchiveEmptyStarterOrganization.ts (1)
15-28: PostgreSQL update logic addedThe new PostgreSQL update logic seems to be correctly updating the
removedAtcolumn for the specifiedorgId. However, theconsole.logstatement should be removed or replaced with proper logging before deploying to production.- console.log(pgResults) + // console.log(pgResults) // Uncomment for debugging purposespackages/server/graphql/mutations/helpers/createNewOrg.ts (1)
33-35: PostgreSQL insert logic addedThe new PostgreSQL insert logic seems correct and improves efficiency by removing the need for direct database queries. However, the
await getKysely()statement should be assigned to a variable to improve readability and consistency with other parts of the code.- await getKysely() + const pg = await getKysely()packages/server/graphql/private/queries/suProOrgInfo.ts (1)
9-22: PostgreSQL query logic addedThe new PostgreSQL query logic seems correct and improves efficiency by removing the need for direct database queries. However, the
console.log(pgResults)statement should be removed or replaced with proper logging before deploying to production.- console.log(pgResults) + // console.log(pgResults) // Uncomment for debugging purposespackages/server/graphql/queries/invoices.ts (1)
Line range hint
43-60:
Optimise filtering of active organisation usersThe filtering of
orgUsersto getactiveOrgUserscan be optimised by including theinactivecondition in the initial query to fetchorgUsers.- const [tooManyInvoices, orgUsers] = await Promise.all([ + const [tooManyInvoices, activeOrgUsers] = await Promise.all([ ... - dataLoader.get('organizationUsersByOrgId').load(orgId) + dataLoader.get('organizationUsersByOrgId').load(orgId, {inactive: false}) ]) - const activeOrgUsers = orgUsers.filter(({inactive}) => !inactive) const orgUserCount = activeOrgUsers.lengthpackages/server/graphql/mutations/archiveOrganization.ts (2)
2-2: Add newline after import statements for readability.Consider adding a newline after the import statements to improve readability.
+ import {sql} from 'kysely'
8-8: Add newline after import statements for readability.Consider adding a newline after the import statements to improve readability.
+ import getKysely from '../../postgres/getKysely'packages/server/graphql/mutations/helpers/removeFromOrg.ts (4)
1-1: Add newline after import statements for readability.Consider adding a newline after the import statements to improve readability.
+ import {sql} from 'kysely'
6-6: Add newline after import statements for readability.Consider adding a newline after the import statements to improve readability.
+ import getKysely from '../../../postgres/getKysely'
21-21: Add newline after variable declarations for readability.Consider adding a newline after the variable declarations to improve readability.
+ const pg = getKysely()
47-55: Based on the context provided from the previous successful search, we can see that theremovedAtfield is updated in multiple places in the codebase for PostgreSQL. However, no corresponding updates for RethinkDB were found. Here are the relevant snippets for PostgreSQL:
packages/server/graphql/mutations/helpers/removeFromOrg.tspackages/server/graphql/mutations/archiveOrganization.tspackages/server/safeMutations/safeArchiveEmptyStarterOrganization.tspackages/server/billing/helpers/adjustUserCount.tsSince no RethinkDB updates were found in these contexts, it suggests a potential inconsistency.
Ensure consistency between PostgreSQL and RethinkDB operations.
The code updates the
removedAtfield in PostgreSQL but does not show corresponding updates in RethinkDB. Ensure that the operations are consistent and that both databases reflect the same state.
- Files to check:
packages/server/graphql/mutations/helpers/removeFromOrg.tspackages/server/graphql/mutations/archiveOrganization.tspackages/server/safeMutations/safeArchiveEmptyStarterOrganization.tspackages/server/billing/helpers/adjustUserCount.tsAnalysis chain
Ensure consistency between PostgreSQL and RethinkDB operations.
The code updates the
removedAtfield in both PostgreSQL and RethinkDB. Ensure that the operations are consistent and that both databases reflect the same state.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify consistency between PostgreSQL and RethinkDB for the `removedAt` field. # Test: Check if both databases have the same `removedAt` values for the specified user. # Expect: Consistent state between PostgreSQL and RethinkDB. # Check PostgreSQL pg_result=$(psql -c "SELECT removedAt FROM OrganizationUser WHERE userId = '${userId}' AND orgId = '${orgId}'") # Check RethinkDB r_result=$(rethinkdb --eval "r.db('your_db').table('OrganizationUser').filter({userId: '${userId}', orgId: '${orgId}'}).pluck('removedAt')") # Compare results if [ "$pg_result" != "$r_result" ]; then echo "Inconsistency detected between PostgreSQL and RethinkDB" exit 1 else echo "Consistency verified between PostgreSQL and RethinkDB" exit 0 fiLength of output: 563
Script:
#!/bin/bash # Search for the update operation on the `removedAt` field in PostgreSQL in the codebase rg --context 5 "updateTable('OrganizationUser').set({removedAt: sql`CURRENT_TIMESTAMP`})" # Search for the update operation on the `removedAt` field in RethinkDB in the codebase rg --context 5 "r.db('your_db').table('OrganizationUser').update({removedAt: r.now()})"Length of output: 646
Script:
#!/bin/bash # Search for the update operation on the `removedAt` field in PostgreSQL in the codebase rg --context 5 "updateTable\('OrganizationUser'\)\.set\(\{removedAt: sql`CURRENT_TIMESTAMP`\}\)" # Search for the update operation on the `removedAt` field in RethinkDB in the codebase rg --context 5 "r\.db\('your_db'\)\.table\('OrganizationUser'\)\.update\(\{removedAt: r\.now\(\)\}\)"Length of output: 307
Script:
#!/bin/bash # Search for any occurrence of 'removedAt' being set in PostgreSQL in the codebase rg --context 5 "removedAt: sql" # Search for any occurrence of 'removedAt' being set in RethinkDB in the codebase rg --context 5 "removedAt: r\.now"Length of output: 5952
Script:
#!/bin/bash # Extract surrounding context of PostgreSQL updates to identify if any RethinkDB operations are present or missing rg --context 10 "updateTable\('OrganizationUser'\)\.set\(\{removedAt: sql`CURRENT_TIMESTAMP`\}\)"Length of output: 192
packages/server/graphql/public/mutations/setOrgUserRole.ts (2)
4-4: Add newline after import statements for readability.Consider adding a newline after the import statements to improve readability.
+ import getKysely from '../../../postgres/getKysely'
25-25: Add newline after variable declarations for readability.Consider adding a newline after the variable declarations to improve readability.
+ const pg = getKysely()packages/server/billing/helpers/adjustUserCount.ts (1)
40-40: Consider adding a log entry.Logging the domain update can help in tracking changes and debugging issues.
+ Logger.info(`Updated active domain for org ${orgId} to ${domain}`)packages/server/utils/__tests__/isRequestToJoinDomainAllowed.test.ts (1)
32-32: Typo in comment: "trumps" should be "overrides".Consider replacing "trumps" with "overrides" for clarity and professionalism.
-// Team trumps starter tier with more users org +// Team overrides starter tier with more users org
| const orgUsers = await dataLoader.get('organizationUsersByOrgId').load(orgId) | ||
| const billingLeaderOrgUsers = orgUsers.filter( | ||
| ({role}) => role && ['BILLING_LEADER', 'ORG_ADMIN'].includes(role) | ||
| ) | ||
| const billingLeaderUserIds = billingLeaderOrgUsers.map(({userId}) => userId) |
There was a problem hiding this comment.
Use const for constant variables
The billingLeaderOrgUsers and billingLeaderUserIds variables are not reassigned, so they should be declared with const instead of let.
- let billingLeaderOrgUsers = orgUsers.filter(
+ const billingLeaderOrgUsers = orgUsers.filter(
- let billingLeaderUserIds = billingLeaderOrgUsers.map(({userId}) => userId)
+ const billingLeaderUserIds = billingLeaderOrgUsers.map(({userId}) => userId)Committable suggestion was skipped due to low confidence.
There was a problem hiding this comment.
where are you seeing let?
There was a problem hiding this comment.
@mattkrick It appears there was a misunderstanding. The code already uses const for both billingLeaderOrgUsers and billingLeaderUserIds. No changes are needed.
Apologies for the confusion!
| const orgUsers = await dataLoader.get('organizationUsersByOrgId').load(orgId) | ||
| const activeOrgUsers = orgUsers.filter(({inactive}) => !inactive) | ||
| const quantity = activeOrgUsers.length |
There was a problem hiding this comment.
Optimise filtering of active organisation users
The filtering of orgUsers to get activeOrgUsers can be optimised by including the inactive condition in the initial query to fetch orgUsers.
- const orgUsers = await dataLoader.get('organizationUsersByOrgId').load(orgId)
- const activeOrgUsers = orgUsers.filter(({inactive}) => !inactive)
+ const activeOrgUsers = await dataLoader.get('organizationUsersByOrgId').load(orgId, {inactive: false})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.
| const orgUsers = await dataLoader.get('organizationUsersByOrgId').load(orgId) | |
| const activeOrgUsers = orgUsers.filter(({inactive}) => !inactive) | |
| const quantity = activeOrgUsers.length | |
| const activeOrgUsers = await dataLoader.get('organizationUsersByOrgId').load(orgId, {inactive: false}) | |
| const quantity = activeOrgUsers.length |
| await pg | ||
| .with('Org', (qc) => qc.insertInto('Organization').values(org)) | ||
| .insertInto('OrganizationUser') | ||
| .values(orgUsers) | ||
| .execute() |
There was a problem hiding this comment.
Ensure proper error handling for database operations
The await statements for database operations should have proper error handling to ensure robustness.
- await pg
+ try {
+ await pg
+ .with('Org', (qc) => qc.insertInto('Organization').values(org))
+ .insertInto('OrganizationUser')
+ .values(orgUsers)
+ .execute()
+ } catch (error) {
+ console.error('Error inserting organization users:', error)
+ 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.
| await pg | |
| .with('Org', (qc) => qc.insertInto('Organization').values(org)) | |
| .insertInto('OrganizationUser') | |
| .values(orgUsers) | |
| .execute() | |
| try { | |
| await pg | |
| .with('Org', (qc) => qc.insertInto('Organization').values(org)) | |
| .insertInto('OrganizationUser') | |
| .values(orgUsers) | |
| .execute() | |
| } catch (error) { | |
| console.error('Error inserting organization users:', error) | |
| throw error | |
| } |
| "suggestedTier" "TierEnum", | ||
| "inactive" BOOLEAN NOT NULL DEFAULT FALSE, | ||
| "joinedAt" TIMESTAMP WITH TIME ZONE NOT NULL DEFAULT NOW(), | ||
| "newUserUntil" TIMESTAMP WITH TIME ZONE, |
There was a problem hiding this comment.
Remove deprecated newUserUntil column.
The newUserUntil column is deprecated and should be removed from the table schema.
- "newUserUntil" TIMESTAMP WITH TIME ZONE,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.
| "newUserUntil" TIMESTAMP WITH TIME ZONE, |
| ]) | ||
| dataLoader.get('organizationUsersByUserId').clear(userId) |
There was a problem hiding this comment.
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.
| await getKysely() | ||
| .updateTable('OrganizationUser') | ||
| .set({removedAt: sql`CURRENT_TIMESTAMP`}) | ||
| .where('userId', '=', user.id) | ||
| .where('orgId', 'in', orgIds) | ||
| .execute() |
There was a problem hiding this comment.
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.
| 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 getKysely() | ||
| .insertInto('OrganizationUser') | ||
| .values(docs) | ||
| .onConflict((oc) => oc.doNothing()) | ||
| .execute() |
There was a problem hiding this comment.
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.
| 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 | |
| } |
| getKysely() | ||
| .updateTable('OrganizationUser') | ||
| .set({inactive}) | ||
| .where('userId', '=', userId) | ||
| .where('removedAt', 'is', null) | ||
| .execute(), |
There was a problem hiding this comment.
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.
| 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 | |
| } |
| .set({suggestedTier: 'starter'}) | ||
| .where('orgId', '=', orgId) | ||
| .where('userId', 'in', billingLeadersIds) | ||
| .where('removedAt', 'is', null) |
There was a problem hiding this comment.
added this because it was erroneously missing in the rethinkdb query
| const orgUsers = await dataLoader.get('organizationUsersByOrgId').load(orgId) | ||
| const billingLeaderOrgUsers = orgUsers.filter( | ||
| ({role}) => role && ['BILLING_LEADER', 'ORG_ADMIN'].includes(role) | ||
| ) | ||
| const billingLeaderUserIds = billingLeaderOrgUsers.map(({userId}) => userId) |
There was a problem hiding this comment.
where are you seeing let?
Signed-off-by: Matt Krick <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 2
Outside diff range, codebase verification and nitpick comments (2)
packages/server/dataloader/__tests__/isOrgVerified.test.ts (1)
Line range hint
75-76: Avoid hardcoding tier values.Consider using the
TierEnumfor the tier values to avoid hardcoding and potential typos.- tier: 'starter' as const + tier: TierEnum.STARTERpackages/server/utils/__tests__/isRequestToJoinDomainAllowed.test.ts (1)
75-76: Avoid hardcoding tier values.Consider using the
TierEnumfor the tier values to avoid hardcoding and potential typos.- tier: 'starter' as const + tier: TierEnum.STARTER
Signed-off-by: Matt Krick <[email protected]>
| await getKysely() | ||
| .with('Org', (qc) => qc.insertInto('Organization').values(org)) | ||
| .insertInto('OrganizationUser') | ||
| .values(orgUsers) | ||
| .execute() |
There was a problem hiding this comment.
Ensure transactional integrity for PostgreSQL operations.
Consider wrapping the PostgreSQL operations in a transaction to ensure atomicity. This will prevent partial data insertion in case of an error.
+ const trx = await getKysely().transaction()
+ try {
await getKysely()
.with('Org', (qc) => qc.insertInto('Organization').values(org))
.insertInto('OrganizationUser')
.values(orgUsers)
.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.
| await getKysely() | |
| .with('Org', (qc) => qc.insertInto('Organization').values(org)) | |
| .insertInto('OrganizationUser') | |
| .values(orgUsers) | |
| .execute() | |
| const trx = await getKysely().transaction() | |
| try { | |
| await getKysely() | |
| .with('Org', (qc) => qc.insertInto('Organization').values(org)) | |
| .insertInto('OrganizationUser') | |
| .values(orgUsers) | |
| .execute() | |
| await trx.commit() | |
| } catch (error) { | |
| await trx.rollback() | |
| throw error | |
| } |
Signed-off-by: Matt Krick <[email protected]>
Signed-off-by: Matt Krick <[email protected]>
Description
Add writes to PG
TEST
Summary by CodeRabbit
New Features
newUserUntilreferences and simplifying user and organisation role logic.Bug Fixes
Refactor