-
-
Notifications
You must be signed in to change notification settings - Fork 333
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
verify remote collection before local collection deletion #2701
Conversation
Reviewer's Guide by SourceryThis PR enhances the query collection synchronization between local and remote storage by implementing verification steps before local collection deletion and adding support for subcollection creation. The changes also include error handling improvements and code formatting cleanup. Sequence diagram for verifying remote collection before local deletionsequenceDiagram
participant User
participant LocalStorage
participant RemoteStorage
User->>LocalStorage: Request to delete collection
LocalStorage->>RemoteStorage: Create remote collection
alt Remote collection creation failed
RemoteStorage-->>LocalStorage: Error
LocalStorage->>User: Notify failure
else Remote collection created
RemoteStorage-->>LocalStorage: Return collection ID
LocalStorage->>RemoteStorage: Verify remote collection
alt Verification failed
RemoteStorage-->>LocalStorage: Error
LocalStorage->>User: Notify failure
else Verification successful
LocalStorage->>LocalStorage: Delete local collection
LocalStorage->>User: Notify success
end
end
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey @imolorhe - I've reviewed your changes and they look great!
Here's what I looked at during the review
- 🟢 General issues: all looks good
- 🟢 Security: all looks good
- 🟢 Testing: all looks good
- 🟡 Complexity: 1 issue found
- 🟢 Documentation: all looks good
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
@@ -122,7 +138,22 @@ export class QueryCollectionService { | |||
// Create remote collection | |||
// Delete local collection | |||
const localCollection = await this.mustGetLocalCollection(collectionId); | |||
await this.createRemoteCollection(localCollection); | |||
const resId = await this.createRemoteCollection(localCollection); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
issue (complexity): Consider extracting verification and subcollection creation logic into dedicated methods to improve code organization
The code could be simplified while maintaining the same functionality:
- Extract verification logic into a dedicated method:
private async verifyRemoteCollection(remoteId: string, localCollection: IQueryCollection) {
const remote = await this.api.getCollection(remoteId);
if (!remote) {
throw new Error('Failed to retrieve the remote collection');
}
if (remote.queries.length !== localCollection.queries.length) {
throw new Error('Query count mismatch');
}
return remote;
}
- Extract subcollection creation into a focused method:
private async createSubcollections(
parentId: string,
originalCollectionId: string,
workspaceId?: WorkspaceId,
teamId?: TeamId
) {
const subcollections = await this.getSubcollections(originalCollectionId);
await Promise.all(subcollections.map(sub =>
sub?.id && this.api.createQueryCollection(sub, parentId, workspaceId, teamId)
));
}
This simplifies the main methods:
async createRemoteCollection(
collection: CreateDTO<IQueryCollection> | IQueryCollection,
workspaceId?: WorkspaceId,
teamId?: TeamId
) {
if (!(await this.canApplyRemote())) return;
const res = await this.api.createQueryCollection(collection, undefined, workspaceId, teamId);
if (!res) throw new Error('could not create the collection');
if ('id' in collection) {
await this.createSubcollections(res.id, collection.id, workspaceId, teamId);
}
return res.id;
}
async transformCollectionToRemoteCollection(collectionId: CollectionID) {
if (!(await this.canApplyRemote())) return;
const localCollection = await this.mustGetLocalCollection(collectionId);
const remoteId = await this.createRemoteCollection(localCollection);
await this.verifyRemoteCollection(remoteId, localCollection);
await this.deleteLocalCollection(collectionId);
}
Visit the preview URL for this PR (updated for commit 0031c88): https://altair-gql--pr2701-imolorhe-verify-remo-5q80oimj.web.app (expires Sat, 16 Nov 2024 03:09:20 GMT) 🔥 via Firebase Hosting GitHub Action 🌎 Sign: 02d6323d75a99e532a38922862e269d63351a6cf |
Fixes
Checks
yarn test-build
Changes proposed in this pull request:
Summary by Sourcery
Verify the creation of remote collections before deleting local collections to prevent data loss and handle subcollections during remote collection creation.
Bug Fixes:
Enhancements: