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

Emit Sync Job Finished Status After Sync Completion #1907

Closed
wants to merge 4 commits into from

Conversation

ndegwamartin
Copy link
Collaborator

@ndegwamartin ndegwamartin commented Mar 10, 2023

Description
@jingtang10 @aditya-07 I think we should emit the Success status as well. Currently once its done downloading we don't notify dependent apps.

Checklist

  • I have read and acknowledged the Code of conduct.
  • I have read the Contributing page.
  • I have signed the Google Individual CLA, or I am covered by my company's Corporate CLA.
  • I have discussed my proposed solution with code owners in the linked issue(s) and we have agreed upon the general approach.
  • I have run ./gradlew spotlessApply and ./gradlew spotlessCheck to check my code follows the style guide of this project.
  • I have run ./gradlew check and ./gradlew connectedCheck to test my changes locally.
  • I have built and run the demo app(s) to verify my change fixes the issue and/or does not break the demo app(s).

@ndegwamartin ndegwamartin changed the title Emit Sync Success Status After Sync Completion Emit Sync Job Finished Status After Sync Completion Mar 14, 2023
omarismail94
omarismail94 previously approved these changes Mar 17, 2023
Copy link
Contributor

@omarismail94 omarismail94 left a comment

Choose a reason for hiding this comment

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

@ndegwamartin please update your PR with master! Ive approved it, but it needs to be up to date with master

@omarismail94 omarismail94 enabled auto-merge (squash) March 17, 2023 11:45
@ndegwamartin
Copy link
Collaborator Author

@omarismail94 thank you

@aditya-07 aditya-07 disabled auto-merge March 17, 2023 13:09
@@ -116,6 +116,7 @@ internal class FhirSynchronizer(
}
}
return if (exceptions.isEmpty()) {
setSyncState(SyncJobStatus.Finished())
Copy link
Collaborator

Choose a reason for hiding this comment

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

SyncJobStatus.Finished signifies that the sync has completed/finished successfully and this change is setting it early irrespective of whether upload is successful or not.
That's why FhirSynchronizer.kt#L91 is the correct place to set the overall success status.

If we need the individual success status for upload and download, we can maybe think of adding new status values.

Copy link
Contributor

@omarismail94 omarismail94 left a comment

Choose a reason for hiding this comment

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

Good catch @aditya-07 ! Yes, this is correct

@omarismail94 omarismail94 dismissed their stale review March 17, 2023 13:25

Aditya corrected my assumption

@ndegwamartin
Copy link
Collaborator Author

Good catch @aditya-07 ! Yes, this is correct

Noted

@ndegwamartin
Copy link
Collaborator Author

Closing this for now

ndegwamartin added a commit to opensrp/android-fhir that referenced this pull request May 11, 2023
  - With unmerged PR #1
  - With unmerged PR google#1917
  - With unmerged PR google#1978
  - With unmerged PR google#1964
  - With unmerged PR google#1963
  - With unmerged PR google#1907
ndegwamartin added a commit to opensrp/android-fhir that referenced this pull request May 23, 2023
  - With unmerged PR #1
  - With unmerged PR google#1917
  - With unmerged PR google#1978
  - With unmerged PR google#1964
  - With unmerged PR google#1907
ndegwamartin added a commit to opensrp/android-fhir that referenced this pull request Jun 15, 2023
  - With unmerged PR #1
  - With unmerged PR google#1917
  - With unmerged PR google#1978
  - With unmerged PR google#1907
  - With unmerged PR google#2016
  - With unmerged PR google#2032
ndegwamartin added a commit to opensrp/android-fhir that referenced this pull request Jun 26, 2023
      - With unmerged PR #1
      - With unmerged PR google#1917
      - With unmerged PR google#1978
      - With unmerged PR google#1907
      - With unmerged PR google#2016
      - With unmerged PR google#2032
      - With unmerged PR google#1669
ndegwamartin added a commit to opensrp/android-fhir that referenced this pull request Jun 26, 2023
   - With unmerged PR #1
   - With unmerged PR google#1917
   - With unmerged PR google#1978
   - With unmerged PR google#1907
   - With unmerged PR google#2032
   - With unmerged PR google#1669
   - With unmerged PR google#2047
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants