Minor Update -> reset:data #3304
Conversation
WalkthroughThis pull request removes the Changes
Sequence Diagram(s)sequenceDiagram
participant Caller
participant Main as addSampleData.ts main function
participant DB as Database
Caller->>Main: Trigger add sample data operation
Main->>DB: Connect to DB and insert collections
DB-->>Main: Return success/failure
Main-->>Caller: Return response
sequenceDiagram
participant Caller
participant Main as resetData.ts main function
participant DB as Database
Caller->>Main: Initiate database reset
Main->>DB: Format database (truncate non-admin tables)
DB-->>Main: Confirm formatting
Main->>Main: Log "Administrator preserved."
Main-->>Caller: Operation completed
Possibly related PRs
Suggested labels
Suggested reviewers
Tip CodeRabbit's docstrings feature is now available as part of our Pro Plan! Simply use the command 📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (4)
⏰ Context from checks skipped due to timeout of 90000ms (2)
🔇 Additional comments (7)
✨ 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
Documentation and Community
|
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
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (5)
scripts/dbManagement/addSampleData.ts(1 hunks)scripts/dbManagement/helpers.ts(1 hunks)scripts/dbManagement/resetData.ts(1 hunks)test/scripts/dbManagement/addSampleData.test.ts(0 hunks)test/scripts/dbManagement/resetDB.test.ts(0 hunks)
💤 Files with no reviewable changes (2)
- test/scripts/dbManagement/resetDB.test.ts
- test/scripts/dbManagement/addSampleData.test.ts
⏰ 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 (4)
scripts/dbManagement/resetData.ts (1)
28-28: Confirm actual presence of the admin user
While this message indicates that administrator access is preserved, there's no explicit check to ensure that the admin user exists in the system. IfAPI_ADMINISTRATOR_USER_EMAIL_ADDRESSisn't set or is invalid, this might be misleading. Consider verifying the admin account before displaying this message.scripts/dbManagement/addSampleData.ts (1)
3-3: Removal of ensureAdministratorExists is aligned with the new approach
Dropping the function import is consistent with the PR objective to preserve rather than recreate the admin. This clarifies that administrator creation is no longer a step in addSampleData.scripts/dbManagement/helpers.ts (2)
69-70: Validate the adminEmail usage
Relying onenvConfig.API_ADMINISTRATOR_USER_EMAIL_ADDRESSis fine, but consider enforcing a fail-fast mechanism (e.g., throwing an error) if this variable is missing. Otherwise, you risk deleting all users or preserving none.
84-90: Truncation approach looks solid
UsingRESTART IDENTITY CASCADEhere is a standard way to reset tables while also addressing any foreign key dependencies. The logic is clear and appropriate for a full “clean slate” reinitialization.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop-postgres #3304 +/- ##
=================================================
Coverage 48.56% 48.57%
=================================================
Files 458 458
Lines 34533 34489 -44
Branches 976 971 -5
=================================================
- Hits 16772 16752 -20
+ Misses 17761 17737 -24 ☔ View full report in Codecov by Sentry. |
|
@palisadoes Please ignore this Patch, it is related to helpers file for which tests are not written right now, xoldd told me that if any other file is conflicting in parallel tests with sample data, then it would need to be fixed rather than these helpers since files should be able to test on flexible DB states. I am working on all of this to ensure error free parallel integration tests, will raise related PR soon. Meanwhile this PR is ready for review. |
palisadoes
merged commit 29d92be
into
PalisadoesFoundation:develop-postgres
Overview
Based on discussion about testing the scripts with xoldd, it came into light that we don't need to reset administrator and recreate (similar to what seedInitialData does), we should ignore this since it remains unaffected from users.
This PR introduces minor change in script to skip deleting administrator from DB and skip checking or recreation of administrator in both reset:data or add:sample_data since it is absolute that administrator always exist in any situation even after a db reset.
Related Issue: #3214
Future Steps
Since we are close to resolving issue regarding conflicts that might occur during parallel vitest, this PR will be followed by adding new tests and correcting old ones to avoid conflict while running in parallel.
Summary by CodeRabbit