Fixes #2841: Invalid emails and names in import:sample-data script #3160
Conversation
|
Warning Rate limit exceeded@gautam-divyanshu has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 13 minutes and 8 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (1)
WalkthroughThis pull request includes significant modifications to the Changes
Assessment against linked issues
Possibly related PRs
Suggested labels
Suggested reviewers
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
Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🔭 Outside diff range comments (1)
src/utilities/loadSampleData.ts (1)
1-10: Import Order Suggestion: Sort Import Statements
The import statements are currently unsorted, which might lead to merge conflicts or reduced readability. Consider grouping external modules (e.g., "dotenv", "node:fs/promises", "node:path") before internal modules (like "../drizzle/schema") and sorting them alphabetically per project guidelines.🧰 Tools
🪛 GitHub Actions: Pull request workflow
[error] 1-1: Import statements could be sorted.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
src/utilities/loadSampleData.ts(2 hunks)
🧰 Additional context used
🪛 GitHub Actions: Pull request workflow
src/utilities/loadSampleData.ts
[error] 1-1: Import statements could be sorted.
[error] 14-14: Formatter would have printed the following content.
🔇 Additional comments (10)
src/utilities/loadSampleData.ts (10)
11-12: Environment Configuration Initialization: dotenv.config() Placement
Moving the dotenv.config() call so early in the file ensures environment variables are available to subsequent code. This positioning is good practice; please confirm that every dependent module correctly accesses the intended variables.
16-21: Query Client Configuration Formatting
The properties in the queryClient initialization have now been consistently indented and aligned. This improves readability without affecting functionality. Also, converting the port value to a number explicitly (via Number(process.env.API_POSTGRES_PORT)) is appropriate.
26-29: Interface LoadOptions Reformatting
The adjustments in the formatting of the LoadOptions interface (spacing and indentation) make the code clearer while preserving the original structure.
35-61: Improved Readability in listSampleData Function
The reformatting in the try-catch block—including clearer indentation for variable declarations, file operations, and console logging—enhances maintainability. The logic of listing and displaying sample data remains unchanged.
67-79: Consistent Formatting in formatDatabase Function
The updated indentation for the table array and the for-loop in the formatDatabase function improves clarity. No functional changes have been made.
86-227: Reformatting of insertCollections Function
The insertCollections function and its switch-case blocks (handling "users", "organizations", "posts", and "events") have been reformatted for consistent spacing and alignment. This refactoring improves readability while keeping the underlying logic intact.
105-127: Validation Check: Handling Invalid Email and Name?
The PR title indicates fixes for invalid email and name issues, yet within the "users" case block (lines 105–127) no explicit logic appears to sanitize or validate these fields. Please confirm if the invalid email and name issues are addressed elsewhere or if further modifications are required in this block.
232-260: Formatting Improvements in checkCountAfterImport Function
The consistent reformatting in checkCountAfterImport (including the structured table output for record counts) contributes to improved readability without altering functionality.
264-274: Command-line Options Parsing Formatting
The revisions to the options object and the argument parsing (lines 264–274) ensure consistency and clarity. These changes enhance maintainability of the command-line interface parsing.
276-280: Asynchronous Invocation Clarity
The final async IIFE is succinct and well-formatted, clearly outlining the script’s execution flow (first listing sample data, then inserting collections).
|
@palisadoes, don't merge this PR; I need to make more changes because there are a lot of problems in the script that was merged earlier. |
palisadoes
changed the title
|
@Bittukr7479 has found a solution. Please contact him on slack. |
|
Hii @gautam-divyanshu , let's discuss fixing the problem with the user I'd and email in Slack. |
|
@palisadoes, what else is the problem here? |
|
Check slack |
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (6)
package.json(2 hunks)sample_data/events.json(1 hunks)sample_data/organizations.json(1 hunks)sample_data/posts.json(1 hunks)sample_data/users.json(1 hunks)src/utilities/loadSampleData.ts(2 hunks)
🧰 Additional context used
🪛 Biome (1.9.4)
src/utilities/loadSampleData.ts
[error] 190-190: isNaN is unsafe. It attempts a type coercion. Use Number.isNaN instead.
See the MDN documentation for more details.
Unsafe fix: Use Number.isNaN instead.
(lint/suspicious/noGlobalIsNan)
🪛 GitHub Actions: Pull request workflow
src/utilities/loadSampleData.ts
[warning] 190-190: isNaN is unsafe. It attempts a type coercion. Use Number.isNaN instead.
[warning] 1-1: Import statements could be sorted.
[warning] 14-14: Formatter would have printed the following content.
[warning] 1-1: Formatter would have printed the following content.
[warning] 1-1: Formatter would have printed the following content.
[warning] 1-1: Formatter would have printed the following content.
[warning] 1-1: Formatter would have printed the following content.
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Analyse Code With CodeQL (typescript)
🔇 Additional comments (8)
sample_data/organizations.json (1)
1-30: Consistent Organization Data Update
All organization entries now use new UUIDs and updated ISO timestamp values in a consistent format. These changes look correct and should help ensure that downstream processes (such as the import:sample-data script) receive data in the expected format. Please verify that any references to organization IDs elsewhere in the application match these new UUIDs.sample_data/posts.json (1)
1-33: Updated Post Identifiers and Timestamps
The new UUIDs for the post, creator, organization, and updater fields are formatted correctly and consistently with the rest of the sample data. The unchanged values for caption and timestamps (createdAt, pinnedAt, updatedAt) maintain content integrity. Ensure that the import script that consumes these posts is aware of and compatible with these new identifiers.sample_data/events.json (1)
1-39: Consistent Update for Event Data
Event entries now feature new UUIDs for key identifiers (id, creatorId, organizationId, updaterId) and maintain proper ISO date formatting for all timestamp fields, as well as start and end times. These changes are consistent across the sample data files. Confirm that the adjustments integrate smoothly into the data import process and that any association logic (e.g., linking events to organizations) has been updated accordingly.sample_data/users.json (1)
1-228: Uniform UUID Format for User Entries and Valid Sample Emails
User objects now consistently use UUIDs with hyphens, which improves readability and standardization. The sample emails (using the example.com domain) and names appear valid and correctly formatted for testing purposes. Please ensure that the validation routines in the import:sample-data script recognize these formats and that no downstream processes rely on the old user ID format.src/utilities/loadSampleData.ts (3)
1-22: LGTM! Good improvements to the setup.Moving
dotenv.config()to the top ensures environment variables are loaded before use, and addinginquirerenables better user interaction.🧰 Tools
🪛 GitHub Actions: Pull request workflow
[warning] 1-1: Import statements could be sorted.
[warning] 14-14: Formatter would have printed the following content.
[warning] 1-1: Formatter would have printed the following content.
[warning] 1-1: Formatter would have printed the following content.
[warning] 1-1: Formatter would have printed the following content.
[warning] 1-1: Formatter would have printed the following content.
197-234: LGTM! Good improvements to function return type and output formatting.The boolean return type enables better flow control, and the table-formatted output improves readability.
250-271: LGTM! Excellent addition of user interaction.The confirmation prompt prevents accidental data loss and improves the user experience.
package.json (1)
27-27: Verify inquirer version compatibility.Let's check if version 12.4.1 of inquirer is the latest stable version and if its types package version 9.0.7 is compatible.
Also applies to: 43-43
✅ Verification successful
🌐 Web query:
What is the latest stable version of inquirer and its types package?💡 Result:
The latest stable versions are: - **inquirer**: Version `12.4.1` ([npm][1]), published **5 days ago** (February 2, 2025) - **TypeScript types** (`@types/inquirer`): Version `9.0.7` ([npm][10]), last updated **a year ago** ### Key Details: - **inquirer** ([source][1]): - Legacy version still maintained (newer `@inquirer/prompts` exists but isn't part of this query). - Includes core CLI prompt types like `input`, `confirm`, `list`, and `checkbox`. - **@types/inquirer** ([source][10]): - Type definitions for Inquirer.js (compatible with older versions of `inquirer`). - No newer TypeScript definitions were found in the provided sources. For new projects using modern TypeScript/JavaScript workflows, consider migrating to `@inquirer/prompts` instead of relying on legacy typings[1][10]. Citations: - 1: https://www.npmjs.com/package/inquirer - 2: https://www.jsdocs.io/package/@types/inquirer - 3: https://pypi.org/project/inquirerpy/ - 4: https://pypi.org/project/inquirer/ - 5: https://www.jsdocs.io/package/inquirer - 6: https://github.com/DefinitelyTyped/DefinitelyTyped/issues/36726 - 7: https://python-inquirer.readthedocs.io/en/latest/ - 8: https://www.npmjs.com/package/@types/inquirer/v/0.0.37 - 9: https://python-inquirer.readthedocs.io/en/latest/inquirer.html - 10: https://www.npmjs.com/package/@types/inquirer
Inquirer dependency versions are compatible.
- The dependency "inquirer" is set to version 12.4.1, which is currently the latest stable release.
- The corresponding dev dependency "@types/inquirer" is at version 9.0.7, matching the latest available TypeScript definitions.
|
Biome uses tab separation |
|
Please fix the failing test |
Our Pull Request Approval ProcessThanks for contributing! Testing Your CodeRemember, your PRs won't be reviewed until these criteria are met:
Our policies make our code better. ReviewersDo not assign reviewers. Our Queue Monitors will review your PR and assign them.
Reviewing Your CodeYour reviewer(s) will have the following roles:
CONTRIBUTING.mdRead our CONTRIBUTING.md file. Most importantly:
Other
|
There was a problem hiding this comment.
Actionable comments posted: 5
🔭 Outside diff range comments (2)
sample_data/organizations.json (1)
1-59: Formatting Issue: JSON File Formatting Inconsistent
The pipeline failure indicates that the JSON formatter would have reprinted this file differently. Please run the configured formatter (for example, via Prettier or jq) to standardize spacing and indentation across the file.For instance, you could run:
- npx prettier --write sample_data/organizations.json
🧰 Tools
🪛 GitHub Actions: Pull request workflow
[error] 2-2: Formatter would have printed the following content.
sample_data/organization_memberships.json (1)
1-101: Formatting Issue: JSON File Formatting Inconsistent
The new organization_memberships.json file triggers formatting errors (as noted in the pipeline). Please run the JSON formatter to adjust spacing and indents as per project standards.For example:
- npx prettier --write sample_data/organization_memberships.json
Otherwise, the structure and field consistency (organizationId, memberId, role, createdAt, creatorId) look correct and aligned with the updated organization data.🧰 Tools
🪛 GitHub Actions: Pull request workflow
[error] 1-1: Formatter would have printed the following content.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (3)
sample_data/organization_memberships.json(1 hunks)sample_data/organizations.json(1 hunks)src/utilities/loadSampleData.ts(2 hunks)
🧰 Additional context used
🪛 GitHub Actions: Pull request workflow
sample_data/organization_memberships.json
[error] 1-1: Formatter would have printed the following content.
src/utilities/loadSampleData.ts
[warning] 204-204: isNaN is unsafe. It attempts a type coercion. Use Number.isNaN instead.
[warning] 1-1: Import statements could be sorted.
[warning] 14-14: Formatter would have printed the following content.
sample_data/organizations.json
[error] 2-2: Formatter would have printed the following content.
🪛 Biome (1.9.4)
src/utilities/loadSampleData.ts
[error] 204-204: isNaN is unsafe. It attempts a type coercion. Use Number.isNaN instead.
See the MDN documentation for more details.
Unsafe fix: Use Number.isNaN instead.
(lint/suspicious/noGlobalIsNan)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Analyse Code With CodeQL (typescript)
🔇 Additional comments (12)
src/utilities/loadSampleData.ts (12)
9-10: Great addition of inquirer for user prompts.
Introducing inquirer is a good step toward interactive data management.
35-45: No major issues with listSampleData changes.
The listing logic is straightforward, and the layout for displaying file names and document counts is clear.
47-57: Add specific error handling for JSON parsing.
Catching generic errors is good, but you may want to provide targeted error feedback if JSON parsing fails (e.g., invalid JSON structure).
68-79: formatDatabase implementation is clear.
Looping over all tables and deleting them is fine. Just be mindful of any foreign key constraints in the future.
88-89: insertCollections function signature looks good.
No issues found with the updated parameter list.
102-129: User and organization data insertion verified.
The usage of parseDate for createdAt and updatedAt fields is a clean approach for consistent date handling.
131-143: Organization memberships data insertion confirmed.
Likewise, parseDate usage maintains consistency with other entities.
202-205: Use Number.isNaN instead of isNaN for safer type checking.
Replacing isNaN with Number.isNaN avoids unexpected type coercions.- return isNaN(parsedDate.getTime()) ? null : parsedDate; + return Number.isNaN(parsedDate.getTime()) ? null : parsedDate;🧰 Tools
🪛 Biome (1.9.4)
[error] 204-204: isNaN is unsafe. It attempts a type coercion. Use Number.isNaN instead.
See the MDN documentation for more details.
Unsafe fix: Use Number.isNaN instead.(lint/suspicious/noGlobalIsNan)
🪛 GitHub Actions: Pull request workflow
[warning] 204-204: isNaN is unsafe. It attempts a type coercion. Use Number.isNaN instead.
254-260: Nicely updated collections array.
Adding "organization_memberships" ensures consistency with the newly inserted data.
264-265: Parsing format flag is straightforward.
Your approach for detecting and applying the --format or -f flag is clear.
275-295: User prompt for existing data is well-implemented.
Leveraging inquirer provides a smooth user experience for handling existing data before loading new samples.
211-248: Ensure the boolean return value is utilized.
Returning a boolean from checkCountAfterImport is a useful enhancement. Verify that callers handle the true/false outcome meaningfully.✅ Verification successful
🏁 Script executed:
#!/bin/bash # Searching for usage of checkCountAfterImport to verify the boolean result is being handled properly. rg "checkCountAfterImport" -A 5Length of output: 1271
Boolean return value usage verified in loadSampleData.ts
The boolean value returned by checkCountAfterImport is indeed being used meaningfully in the codebase. In one instance, the result is assigned to the variable existingData, which is then checked in an if condition to determine further actions. Although one call simply awaits the function without capturing the return value, the overall design correctly handles the true/false outcome in contexts where it matters.
Our Pull Request Approval ProcessThanks for contributing! Testing Your CodeRemember, your PRs won't be reviewed until these criteria are met:
Our policies make our code better. ReviewersDo not assign reviewers. Our Queue Monitors will review your PR and assign them.
Reviewing Your CodeYour reviewer(s) will have the following roles:
CONTRIBUTING.mdRead our CONTRIBUTING.md file. Most importantly:
Other
|
Our Pull Request Approval ProcessThanks for contributing! Testing Your CodeRemember, your PRs won't be reviewed until these criteria are met:
Our policies make our code better. ReviewersDo not assign reviewers. Our Queue Monitors will review your PR and assign them.
Reviewing Your CodeYour reviewer(s) will have the following roles:
CONTRIBUTING.mdRead our CONTRIBUTING.md file. Most importantly:
Other
|
Our Pull Request Approval ProcessThanks for contributing! Testing Your CodeRemember, your PRs won't be reviewed until these criteria are met:
Our policies make our code better. ReviewersDo not assign reviewers. Our Queue Monitors will review your PR and assign them.
Reviewing Your CodeYour reviewer(s) will have the following roles:
CONTRIBUTING.mdRead our CONTRIBUTING.md file. Most importantly:
Other
|
|
This PR is ready to merge. I have created the sample data for organization, users, and organization_membership. Other sample data will be created on the way, solving issues to avoid any confusion because right now I can't check data because the UI is not working. |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop-postgres #3160 +/- ##
====================================================
+ Coverage 39.87% 39.90% +0.02%
====================================================
Files 455 455
Lines 33551 33527 -24
Branches 415 415
====================================================
Hits 13378 13378
+ Misses 20173 20149 -24 ☔ View full report in Codecov by Sentry. |
|
@varshith257 Please merge this pr. |
There was a problem hiding this comment.
Actionable comments posted: 5
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (3)
biome.jsonc(1 hunks)docs/docs/docs/getting-started/installation.md(1 hunks)src/utilities/loadSampleData.ts(6 hunks)
🧰 Additional context used
🪛 markdownlint-cli2 (0.17.2)
docs/docs/docs/getting-started/installation.md
346-346: Trailing spaces
Expected: 0 or 2; Actual: 1
(MD009, no-trailing-spaces)
350-350: Bare URL used
null
(MD034, no-bare-urls)
351-351: Bare URL used
null
(MD034, no-bare-urls)
352-352: Bare URL used
null
(MD034, no-bare-urls)
353-353: Bare URL used
null
(MD034, no-bare-urls)
354-354: Bare URL used
null
(MD034, no-bare-urls)
355-355: Bare URL used
null
(MD034, no-bare-urls)
356-356: Bare URL used
null
(MD034, no-bare-urls)
357-357: Bare URL used
null
(MD034, no-bare-urls)
358-358: Bare URL used
null
(MD034, no-bare-urls)
359-359: Bare URL used
null
(MD034, no-bare-urls)
360-360: Bare URL used
null
(MD034, no-bare-urls)
361-361: Bare URL used
null
(MD034, no-bare-urls)
362-362: Bare URL used
null
(MD034, no-bare-urls)
363-363: Bare URL used
null
(MD034, no-bare-urls)
364-364: Bare URL used
null
(MD034, no-bare-urls)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Run tests for talawa api
- GitHub Check: Analyse Code With CodeQL (typescript)
🔇 Additional comments (17)
biome.jsonc (1)
19-20: 🏁 Script executed:#!/bin/bash # Description: Verify sample data files are excluded from Biome formatting # Note: This helps ensure that manual fixes to emails and names in sample data won't be overwritten by the formatter # Find all sample data files echo "Sample data files that should be excluded:" fd . sample_data -t f # Test if Biome will format these files echo -e "\nVerifying Biome exclusion:" biome format --write sample_data 2>&1 | grep -i "ignored\|skipped"Length of output: 347
src/utilities/loadSampleData.ts (16)
4-4: Good addition for environment variable management.
Importing dotenv to load environment variables is a best practice and ensures environment data is accessible without issues.
7-7: Helpful user input integration.
Introducing inquirer to prompt the user for confirmation is a good approach when dealing with data deletion.
11-12: Appropriate placement of dotenv.config().
Calling dotenv.config() at the start of the file ensures environment variables are loaded before usage.
65-65: Clear and concise doc comment.
Documenting the new functionality for selective data deletion is helpful for maintainers.
74-74: Ensures organization memberships are handled.
Adding organizationMembershipsTable to the formatting process keeps the database tables consistent.
115-119: Accurate date parsing for users.
Using parseDate ensures invalid or malformed dates are handled gracefully. Looks good.
125-135: Consistent date handling for organizations.
Applying parseDate to createdAt and updatedAt fields helps maintain consistent date formats across entities.
138-149: Implementing parseDate for organization memberships.
This is consistent with other tables and ensures date validity.
183-183: Updated doc comment for clarified return type.
Reflecting the boolean return type aligns well with the new logic of indicating existing data.
185-185: New return type for checkCountAfterImport.
Switching from void to boolean is a meaningful improvement. The function now clearly signals if data remains.
190-193: Added organization memberships to the record-count check.
Ensuring all relevant tables are counted maintains consistency.
204-205: Simple boolean flag to track existing data.
This approach is clear and efficient.
214-216: Minimal overhead in determining data existence.
Setting dataExists to true upon any record ensures quick detection of prior data.
219-219: Returning data existence status.
This boolean return is straightforward and easy to interpret by downstream callers.
226-226: Default collections expanded to include memberships.
Helps ensure all new tables get populated without extra parameters.
242-242: Pre-check for existing data.
Calling checkCountAfterImport before insertion is a robust way to warn users about existing records.
gautam-divyanshu
added
the
ignore-sensitive-files-pr
|
@coderabbitai approve |
✅ Actions performedComments resolved and changes approved. |
varshith257
merged commit afd21cc
into
PalisadoesFoundation:develop-postgres
What kind of change does this PR introduce?
sample data of users, organizations, organization memberships created
Issue Number:
Fixes #2841
Snapshots/Videos:
If relevant, did you update the documentation?
installation.mdupdated with sample user.Summary
Does this PR introduce a breaking change?
Checklist
CodeRabbit AI Review
Test Coverage
Other information
Have you read the contributing guide?
Summary by CodeRabbit
Summary by CodeRabbit