Skip to content
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

[PS-2120] Forcing vault to refresh when the vault is purged or new items are imported #4380

Merged
merged 2 commits into from
Jan 11, 2023

Conversation

aj-rosado
Copy link
Contributor

@aj-rosado aj-rosado commented Jan 4, 2023

Type of change

- [x] Bug fix
- [ ] New feature development
- [ ] Tech debt (refactoring, code cleanup, dependency upgrades, etc)
- [ ] Build/deploy pipeline (DevOps)
- [ ] Other

Objective

There are some cases that the vault is not synced after a successful call to purge or import. Added a flag to force it.

Code changes

  • sync.service.ts: Added a public flag that allows components to force the service to execute the FullSync method until the end.
  • purge-vault-component.cs, import.component.cs: Setting the ShouldForceSync flag to true, after a successful call to import or purge.

Before you submit

  • Please add unit tests where it makes sense to do so (encouraged but not required)
  • If this change requires a documentation update - notify the documentation team
  • If this change has particular deployment requirements - notify the DevOps team

@aj-rosado aj-rosado requested a review from a team January 4, 2023 19:24
Copy link
Member

@MGibson1 MGibson1 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think this is the right way to go about this.

The SyncService (and most services) are singletons and setting this at the service level will cause all syncs to be forced for the entire application.

this needs to be solved on an interaction-by-interaction basis. Why is this.syncService.fullSync(true) not sufficient?

@aj-rosado
Copy link
Contributor Author

aj-rosado commented Jan 5, 2023

I don't think this is the right way to go about this.

The SyncService (and most services) are singletons and setting this at the service level will cause all syncs to be forced for the entire application.

this needs to be solved on an interaction-by-interaction basis. Why is this.syncService.fullSync(true) not sufficient?

After the next successful sync the variable was set to false again, so the FullSync would be forced only on the next call. The idea is after the purge or import is completed, the next call to FullSync should always run, because we know there are changes.
When the user navigates to the vault (after purge or import he is navigating there) it will call the FullSync to check changes so was avoiding an extra call to FullSync, but calling this.syncService.fullSync(true) after successful purge or import should be sufficient.

PS: After some tests calling the this.syncService.fullSync(true) after the import/purge. Seems to be working fine and the number of calls haven't increased (as the previous FullSync call is still running) so I'm simplifying this to simply call the this.syncService.fullSync(true)

@aj-rosado aj-rosado added the hold do not merge, do not approve yet label Jan 10, 2023
@MGibson1 MGibson1 added needs-qa Marks a PR as requiring QA approval and removed hold do not merge, do not approve yet labels Jan 11, 2023
@MGibson1
Copy link
Member

this is the route we want to go forward with on PS-2120

@github-actions github-actions bot temporarily deployed to Web Vault - QA January 11, 2023 21:01 Inactive
@MGibson1 MGibson1 removed the needs-qa Marks a PR as requiring QA approval label Jan 11, 2023
@MGibson1 MGibson1 merged commit 51ead2e into master Jan 11, 2023
@MGibson1 MGibson1 deleted the bug/PS-2120-purge-vault-not-refreshing-items branch January 11, 2023 22:47
MGibson1 pushed a commit that referenced this pull request Jan 11, 2023
…ems are imported (#4380)

* [PS-2120] Forcing vault to refresh when the vault is purged or new items are imported

* [PS-2120] Forcing vault refresh by calling fullSync with force as true

(cherry picked from commit 51ead2e)
djsmith85 added a commit that referenced this pull request May 23, 2023
Analog to #4380 we also need to add a full sync to the import.command, so that when a user calls `bw list items` those are populated.
djsmith85 added a commit that referenced this pull request May 30, 2023
Analog to #4380 we also need to add a full sync to the import.command, so that when a user calls `bw list items` those are populated.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants