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

Disable manual sync when auto sync is in progress #1149

Closed
mahalakshme opened this issue Oct 19, 2023 · 9 comments
Closed

Disable manual sync when auto sync is in progress #1149

mahalakshme opened this issue Oct 19, 2023 · 9 comments
Assignees

Comments

@mahalakshme
Copy link
Contributor

mahalakshme commented Oct 19, 2023

Background

Multiple syncs happening from the same phone is causing issues with the integrity of data in the system and also registering errors on Bugsnag. These are the known issues

  1. Duplicate images being saved - (Duplicate images #1132) (Most important)
  2. Item to delete is undefined in entityQueue issue in Bugsnag - [Bugsnag issue] - Item to delete is undefined in entityQueue #843
  3. Multiple rows in entity_approval_status table due to multiple POSTs for approval enabled transactions
  4. Parallel syncs causing increased load on the server

Story

As a user, I dont want the changes I ve made in my app, to be uploaded multiple times, so that the app behaves in an expected manner.

Currently automatic sync is disabled when manual sync is started. But we dont disable the manual sync when automatic sync is in progress.

Tech Recommnedation

  • There is existing state SyncActions.syncing to indicate whether a sync is currently in progress. This is currently not being set when the automatic sync is going on. We can change logic to set this to true even for automatic sync.

Acceptance criteria

  • When automatic background sync is in-progress show a different 'Sync' icon and when someone clicks on it, show the message "Automatic sync is in progress. Try again after sometime."
  • Additionally avoid triggering automatic Background sync, if a sync had been completed within the last half an hour.

Solution Notes

See spike notes here: #1150 (comment) have added the AC below:

@mahalakshme mahalakshme converted this from a draft issue Oct 19, 2023
@mahalakshme mahalakshme moved this from In Analysis to In Analysis Review in Avni Product Oct 20, 2023
@mahalakshme mahalakshme moved this from In Analysis Review to In Analysis in Avni Product Oct 20, 2023
@mahalakshme mahalakshme moved this from In Analysis to In Analysis Review in Avni Product Oct 24, 2023
@mahalakshme mahalakshme moved this from In Analysis Review to Ready in Avni Product Oct 24, 2023
@himeshr himeshr moved this from Ready to In Progress in Avni Product Oct 25, 2023
@himeshr himeshr self-assigned this Oct 25, 2023
@himeshr himeshr moved this from In Progress to Code Review Ready in Avni Product Oct 25, 2023
@himeshr
Copy link
Contributor

himeshr commented Oct 25, 2023

Had tried using the SyncActions.syncing field to implement the fix as suggested in #1150 (comment), but it resulted in issue due to the Background-sync being re-configured at the end of Manual-sync and overriding the SynComponent.ProgressBarView.onPress action.
Therefore, made use of a separate SyncActions.backgroundSyncInProgress field to make this work as required.

Screen.Recording.2023-10-25.at.5.59.43.PM.mov

@himeshr himeshr moved this from Code Review Ready to In Progress in Avni Product Oct 26, 2023
himeshr added a commit that referenced this issue Oct 26, 2023
himeshr added a commit that referenced this issue Oct 26, 2023
@himeshr
Copy link
Contributor

himeshr commented Oct 26, 2023

Made additional code changes to:

  • Skip background-sync if we had recently synced within the last half an hour
  • Show toast message when user clicks on sync button during automatic sync
Screen.Recording.2023-10-26.at.12.36.15.PM.mov
Screen.Recording.2023-10-26.at.12.41.07.PM.mov

@himeshr himeshr moved this from In Progress to Code Review Ready in Avni Product Oct 26, 2023
@himeshr himeshr moved this from Code Review Ready to In Progress in Avni Product Oct 26, 2023
himeshr added a commit that referenced this issue Oct 27, 2023
…d SyncComponent.js for reset after fullSync
@himeshr himeshr moved this from In Progress to Code Review Ready in Avni Product Oct 27, 2023
@petmongrels petmongrels moved this from Code Review Ready to In Code Review in Avni Product Oct 27, 2023
@petmongrels
Copy link
Contributor

SyncService.reset and MenuView.reset code are same. We can move it to GlobalContext.

For QA Purpose (assuming it is developer)

  • do check logs if there are errors of any type

@petmongrels petmongrels moved this from In Code Review to Code Review with Comments in Avni Product Oct 27, 2023
@himeshr
Copy link
Contributor

himeshr commented Oct 27, 2023

SyncService.reset and MenuView.reset code are same. We can move it to GlobalContext.

@petmongrels , i did attempt to move the reset() method to GlobalContext, but the app fails to launch due to cyclic dependency.
I could attempt to make use of the SyncService.reset() in MenuView as well.

himeshr added a commit that referenced this issue Oct 27, 2023
@himeshr himeshr moved this from Code Review with Comments to Code Review Ready in Avni Product Oct 27, 2023
@himeshr
Copy link
Contributor

himeshr commented Oct 27, 2023

@petmongrels , retained only SyncService.reset code and modified menuView to also invoke the same.
Cannot move to GlobalContext due to cyclicDependency issue.

@petmongrels petmongrels moved this from Code Review Ready to In Progress in Avni Product Oct 27, 2023
@petmongrels petmongrels self-assigned this Oct 27, 2023
@petmongrels petmongrels moved this from In Progress to QA Ready in Avni Product Oct 27, 2023
@petmongrels petmongrels removed their assignment Oct 27, 2023
@AchalaBelokar AchalaBelokar moved this from QA Ready to In QA in Avni Product Oct 30, 2023
@AchalaBelokar AchalaBelokar moved this from In QA to QA Ready in Avni Product Oct 30, 2023
@himeshr
Copy link
Contributor

himeshr commented Oct 30, 2023

@mahalakshme
Copy link
Contributor Author

mahalakshme commented Oct 30, 2023

@himeshr why do we need this for testing. when there are media observations, its easily identifiable from the logs and auto sync take a long time to complete and making manual sync to overlap is easy. We should try to do testing without code changes so that we can make sure we are testing with the right apk. I will unmark it as Tech for now. Let me know if you still think otherwise.

@mahalakshme
Copy link
Contributor Author

Have made it back tech card after discussion with Himesh.

@ashusvnath ashusvnath moved this from QA Ready to In QA in Avni Product Oct 31, 2023
@ashusvnath ashusvnath self-assigned this Oct 31, 2023
@ashusvnath
Copy link

Image

Cannot force a manual sync from foreground when a background sync is in progress

@ashusvnath ashusvnath moved this from In QA to Done in Avni Product Oct 31, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

No branches or pull requests

5 participants