Sync Updates #192
Sync Updates #192
Conversation
- Added filter for the allowDonations - Added Syncing for the Users. - Added name_flags (NEW_, UPDATE_ on name fields) for verification purpose. - origin ttc is added - isMonitored false is handled on dissociation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
WalkthroughThis pull request updates the RO user synchronization process in several areas. The Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant API
participant DB
Note over API: syncROUsers updates
Client->>API: Request to sync RO users
API->>API: Filter projects by allowDonations
API->>API: Aggregate unique RO users into Map
API->>DB: Create new users (with error handling)
API->>DB: Fetch existing RO users, update mapping
API->>DB: Insert/Update projects and sites (prefix NEW_/UPDATE_)
API->>DB: Disassociate sites not in PP API
API-->>Client: Return response
sequenceDiagram
participant Client
participant SyncEndpoint
participant FS as FileSystem
participant DB
Note over SyncEndpoint: New syncROs endpoint flow
Client->>SyncEndpoint: Call syncROs (with CRON key)
SyncEndpoint->>FS: Read projects.json data
SyncEndpoint->>DB: Fetch existing RO users, projects, sites
SyncEndpoint->>SyncEndpoint: Compare projects (compareProjects)
SyncEndpoint->>SyncEndpoint: Compare sites (compareSites)
SyncEndpoint->>DB: Execute create/update/delete operations (commented out)
SyncEndpoint->>SyncEndpoint: Run cleanup (cleanUps)
SyncEndpoint-->>Client: Return success response with project info
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🔇 Additional comments (6)
✨ Finishing Touches
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 using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (5)
apps/server/src/pages/api/cron/sync-ro-users.ts (4)
54-67: Consider using upsert instead of skipDuplicates
UsingskipDuplicates: trueonly avoids inserting exact duplicate data but does not update any partially matching user fields. If fields like name or email can change, consider an upsert-based approach or a separate update step to keep user data current.
302-302: Avoid repeatedly prefixing project names with "UPDATE_"
Appending"UPDATE_"on every change can lead to names like"UPDATE_UPDATE_*". Consider storing a version field or a separate flag to distinguish new vs. updated projects, rather than continually manipulating thename.- name: "UPDATE_" + projectNameFormPP, + name: projectNameFormPP, + // Optionally store a version or set an "isUpdated" flag if needed
334-335: Ensure "NEW_" prefix is truly needed
Prefixing newly created sites with"NEW_"can be helpful for debugging, but confirm you won’t unintentionally retain this prefix in production. Handle edge cases (e.g., empty or undefinedsiteName) gracefully.
353-353: Avoid duplicative "UPDATE_" prefix for site names
Similar to projects, repeatedly prepending"UPDATE_"can produce unwieldy names. Track updates with a timestamp or separate flag instead of chaining prefixes.apps/server/src/pages/api/cron/sync/syncROs.ts (1)
1-220: Complete or remove commented logic and add testing
Most synchronization routines here are commented out, making the file appear like a prototype. Unclear code can cause confusion and lead to maintenance challenges. Consider:
- Activating or removing sections of commented code.
- Adding tests to validate each step in the sync flow (e.g.,
compareProjects,compareSites, etc.).- Ensuring that reading from
projects.jsonis intentional, especially if you plan to fetch data from external services later.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
apps/server/src/pages/api/cron/sync-ro-users.ts(9 hunks)apps/server/src/pages/api/cron/sync/.gitignore(1 hunks)apps/server/src/pages/api/cron/sync/syncROs.ts(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- apps/server/src/pages/api/cron/sync/.gitignore
🔇 Additional comments (3)
apps/server/src/pages/api/cron/sync-ro-users.ts (3)
30-31: Validate data availability and structure before filtering
Ensure thatfetchAllProjectsWithSites()consistently returns an array of objects containingallowDonations. If it can sometimes returnundefinedor an unexpected structure, consider adding guard clauses or fallback logic to prevent runtime errors.
33-52: Confirmproject.tporeliably exists
This block robustly extracts RO users, but it depends onproject.tpoandproject.tpo.id. If any project objects lack those fields, they will be skipped. Verify that this scenario is acceptable or handle it explicitly to avoid missing users. Also confirm prefixing names with"NEW_"fits your naming requirements long-term.
264-265: Verify the necessity of settingisMonitoredto false
When disassociating sites, forcingisMonitoredto false might be desired, but confirm there is no condition under which a site should remain monitored after being removed from the PP API.
| if (!uniqueUsers.has(project.tpo.id)) { | ||
| uniqueUsers.set(project.tpo.id, { | ||
| remoteId: project.tpo.id, | ||
| name: "NEW_" + project.tpo.name, |
There was a problem hiding this comment.
These "NEW_" & "UPDATE_" name flags I have just added for checking Purposes. So If I ran it few passes, I'd be able to check what's change & etc. It will be removed.
| @@ -141,6 +178,8 @@ export default async function syncROUsers(req: NextApiRequest, res: NextApiRespo | |||
| // Add the new site to the array for bulk creation | |||
| newSiteData.push({ | |||
| remoteId: remoteId_PP, | |||
| name: "NEW_" + siteName, | |||
| origin: "ttc", | |||
There was a problem hiding this comment.
Here are 2 things.
name: siteName was not being used here, not sure why. Added it just to keep it uniform.
origin: 'ttc' was not anywhere. Existing Imported Projects have their origin "ttc" so I added.
| @@ -0,0 +1,220 @@ | |||
| // Organizing the Sync Flow | |||
There was a problem hiding this comment.
Ignore this File entirely.
I was trying to rewrite to refactoring the CRON.
added check for user with remoteId before adding it to users, prevents creating multiple user with same remoteId. Checked by grouping by remoteId.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
apps/server/src/pages/api/cron/sync-ro-users.ts (1)
51-86: Consider adding batch size for user creation.When creating multiple users, consider adding a batch size to prevent potential performance issues with large datasets.
if (newUsersToCreate.length > 0) { const result = await prisma.user.createMany({ data: newUsersToCreate, + skipDuplicates: true, });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
apps/server/src/pages/api/cron/sync-ro-users.ts(11 hunks)apps/server/src/pages/api/cron/sync/syncROs.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/server/src/pages/api/cron/sync/syncROs.ts
🔇 Additional comments (3)
apps/server/src/pages/api/cron/sync-ro-users.ts (3)
30-31: LGTM! Good filtering of projects based onallowDonations.The filtering ensures that only projects allowing donations are synchronized, which aligns with the PR objectives.
201-201: LGTM! Origin field added for consistency.Based on your previous comment, adding
origin: 'ttc'maintains consistency with existing imported projects.Also applies to: 354-354
283-295: LGTM! Improved site dissociation logic.Setting
isMonitoredto false when dissociating sites aligns with the PR objectives and ensures proper cleanup.
| console.log(error); | ||
| logger(`Error in transaction: ${error}`, "error"); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Remove console.log statement.
The error is already being logged using the logger utility. Remove the redundant console.log statement.
- console.log(error);
logger(`Error in transaction: ${error}`, "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.
| console.log(error); | |
| logger(`Error in transaction: ${error}`, "error"); | |
| logger(`Error in transaction: ${error}`, "error"); |
| if (!uniqueUsers.has(project.tpo.id)) { | ||
| uniqueUsers.set(project.tpo.id, { | ||
| remoteId: project.tpo.id, | ||
| name: "NEW_" + project.tpo.name, |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Remove temporary name flags after verification.
Based on your previous comment, these "NEW_" and "UPDATE_" name flags were added for verification purposes and should be removed.
Also applies to: 166-166, 200-200, 321-321, 353-353, 372-372
🏁 Script executed:
#!/bin/bash
# Description: Find all occurrences of temporary name flags.
rg -A 1 'name: "(NEW|UPDATE)_"'Length of output: 820
ACTION REQUIRED: Remove Temporary Debug Flags
Our verification confirms that temporary prefixes (e.g., "NEW_") are still present in the file. Please remove these prefixes from all occurrences in apps/server/src/pages/api/cron/sync-ro-users.ts. Specifically:
- Line 40: Remove the prefix from
name: "NEW_" + project.tpo.name - Line 166: Remove the prefix from
name: "NEW_" + projectName - Line 321 & 353: Remove the prefixes from both occurrences of
name: "NEW_" + siteName
Additionally, check any other flagged lines (e.g., lines 200, 372) to ensure that no temporary flags remain in the code. These prefixes were only used for verification purposes and should be removed before merging.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
apps/server/src/pages/api/cron/sync-ro-users.ts (1)
43-43: Simplify the boolean literal in the ternary expression.The ternary expression can be simplified by directly using the condition result.
-isPlanetRO: project.tpo.id.startsWith("tpo_") ? true : false, +isPlanetRO: project.tpo.id.startsWith("tpo_"),🧰 Tools
🪛 Biome (1.9.4)
[error] 43-43: Unnecessary use of boolean literals in conditional expression.
Simplify your code by directly assigning the result without using a ternary operator.
If your goal is negation, you may use the logical NOT (!) or double NOT (!!) operator for clearer and concise code.
Check for more details about NOT operator.
Unsafe fix: Remove the conditional expression with(lint/complexity/noUselessTernary)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
apps/server/src/pages/api/cron/sync-ro-users.ts(11 hunks)
🧰 Additional context used
🪛 Biome (1.9.4)
apps/server/src/pages/api/cron/sync-ro-users.ts
[error] 43-43: Unnecessary use of boolean literals in conditional expression.
Simplify your code by directly assigning the result without using a ternary operator.
If your goal is negation, you may use the logical NOT (!) or double NOT (!!) operator for clearer and concise code.
Check for more details about NOT operator.
Unsafe fix: Remove the conditional expression with
(lint/complexity/noUselessTernary)
🔇 Additional comments (6)
apps/server/src/pages/api/cron/sync-ro-users.ts (6)
6-8: LGTM! Type imports are properly optimized.The use of the
typekeyword for imports helps reduce bundle size by ensuring types are not included in the runtime code.
41-41: Remove temporary name prefix.As previously discussed, the "NEW_" prefix was added for verification purposes and should be removed.
-name: "NEW_" + project.tpo.name, +name: project.tpo.name,
167-167: Remove temporary name prefix.The "NEW_" prefix should be removed as it was only used for verification purposes.
-name: "NEW_" + projectName, +name: projectName,
284-296: LGTM! Proper handling of isMonitored flag during site dissociation.Setting
isMonitoredto false when dissociating a site ensures consistent state management.
407-407: Remove console.log statement.The error is already being logged using the logger utility.
-console.log(error);
321-322: Remove all temporary name prefixes.Remove the "NEW_" and "UPDATE_" prefixes from all site names as they were only used for verification purposes.
-name: "UPDATE_" + projectNameFormPP, +name: projectNameFormPP, -name: "NEW_" + siteName, +name: siteName, -name: "UPDATE_" + siteName, +name: siteName,Also applies to: 353-354, 372-373
| remoteId: project.tpo.id, | ||
| name: "NEW_" + project.tpo.name, | ||
| email: project.tpo.email, | ||
| isPlanetRO: project.tpo.id.startsWith("tpo_") ? true : false, |
There was a problem hiding this comment.
Does this seem right? All profiles (prf_) would be false & tpo_ are in fact RO so only they will be true.
This reverts commit 76f51ea.
Site Sync Updates
@sagararyal
Summary by CodeRabbit
New Features
Chores