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

Add API to support saving local only resources #2178

Draft
wants to merge 26 commits into
base: master
Choose a base branch
from

Conversation

ndegwamartin
Copy link
Collaborator

IMPORTANT: All PRs must be linked to an issue (except for extremely trivial and straightforward changes).

Fixes #2123

Description
Clear and concise code change description.
See #2123

Alternative(s) considered
Have you considered any alternatives? And if so, why have you chosen the approach in this PR?
See #2123

Type
Choose one: (Bug fix | Feature | Documentation | Testing | Code health | Builds | Releases | Other)
Feature

Screenshots (if applicable)
N/A

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 added a commit to opensrp/android-fhir that referenced this pull request Sep 18, 2023
  - With unmerged PR #1
  - With unmerged PR google#1669
  - With unmerged PR google#2178
  - With unmerged PR #9
  - With unmerged PR #10
ndegwamartin added a commit to opensrp/android-fhir that referenced this pull request Sep 18, 2023
  - With unmerged PR #1
  - With unmerged PR google#1669
  - With unmerged PR google#2178
  - With unmerged PR #9
  - With unmerged PR #10
ndegwamartin added a commit to opensrp/android-fhir that referenced this pull request Sep 20, 2023
This reverts commit 7a45e18.

The changes will be introduced by the PR google#2178
ndegwamartin added a commit to opensrp/android-fhir that referenced this pull request Sep 21, 2023
      - With unmerged PR google#1669
      - Wup google#2178
      - Wup #9
      - Wup #10
      - Wup google#2076
ndegwamartin added a commit to opensrp/android-fhir that referenced this pull request Sep 21, 2023
  - With unmerged PR #11
  - Wup google#1669
  - Wup google#2076
  - Wup #9
  - Wup google#2178
  - Wup #10
Copy link
Collaborator

@aditya-07 aditya-07 left a comment

Choose a reason for hiding this comment

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

IMO, adding a default value to FhirEngine.create to specify whether the resources are meant to be local only is a better design choice.

Additionally, we would want all the updates to the resource to be local only as well. Therefore, we should mark the resource as local only in the db itself.

Comment on lines 35 to 42
suspend fun create(vararg resource: Resource): List<String>

/**
* Creates one or more remote FHIR [resource]s in the local storage without flagging as local
* changes.
*/
suspend fun createRemote(vararg resource: Resource)

Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
suspend fun create(vararg resource: Resource): List<String>
/**
* Creates one or more remote FHIR [resource]s in the local storage without flagging as local
* changes.
*/
suspend fun createRemote(vararg resource: Resource)
/**
* Creates one or more FHIR [resource]s in the local storage.
*
* @param isLocalOnly - Setting the value to [true] instructs engine that the resource and its subsequent updates should never be synced to the server.
* @return the logical IDs of the newly created resources.
*/
suspend fun create(vararg resource: Resource, isLocalOnly: Boolean = false): List<String>

The default value can be provided in the interface i.e FhirEngine.kt and the FhirEngineImpl.kt would be called with that value if no value is provided by the caller.

Comment on lines 47 to 50
override suspend fun createRemote(vararg resource: Resource) {
return database.insertRemote(*resource)
}

Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
override suspend fun createRemote(vararg resource: Resource) {
return database.insertRemote(*resource)
}

insertRemote would just add the resource in the db without creating a LocalChange counterintuitively marking the change as local only.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Looking at the test, createRemote is doing what it is supposed to do. But, the name createRemote is counterintuitive and suggests that the resource has something to do with the remote server. A more intutive name could have be createLocal ?

@ndegwamartin
Copy link
Collaborator Author

@aditya-07 could you have a second look to see if this is what you had in mind?

@ndegwamartin
Copy link
Collaborator Author

@jingtang10 you had a concern with the updates for the related resources processed by the implementation here, could you share that here or on the ticket #2123 ?

ndegwamartin added a commit to opensrp/android-fhir that referenced this pull request Oct 13, 2023
  - With unmerged PR #11
  - Wup google#1669
  - Wup #9
  - Wup google#2178
  - Wup #10
  - Wup google#2230
  - Wup google#2262
ndegwamartin added a commit to opensrp/android-fhir that referenced this pull request Oct 30, 2023
ndegwamartin added a commit to opensrp/android-fhir that referenced this pull request Oct 30, 2023
@ndegwamartin
Copy link
Collaborator Author

Changing this to Draft for now since a workaround solution for the initial problem was put forward

ndegwamartin added a commit to opensrp/android-fhir that referenced this pull request Mar 27, 2024
ndegwamartin added a commit to opensrp/android-fhir that referenced this pull request Mar 27, 2024
ndegwamartin added a commit to opensrp/android-fhir that referenced this pull request Apr 19, 2024
ndegwamartin added a commit to opensrp/android-fhir that referenced this pull request Apr 23, 2024
ndegwamartin added a commit to opensrp/android-fhir that referenced this pull request Apr 29, 2024
ndegwamartin added a commit to opensrp/android-fhir that referenced this pull request May 8, 2024
         - With unmerged PR #9
         - WUP PR google#2178
         - WUP PR google#2511
ndegwamartin added a commit to opensrp/android-fhir that referenced this pull request May 14, 2024
   - With unmerged PR #9
   - WUP PR google#2178
   - WUP PR google#2511
ndegwamartin added a commit to opensrp/android-fhir that referenced this pull request May 28, 2024
             - With unmerged PR #9
             - WUP PR google#2178
             - WUP PR google#2511
             - WUP PR google#2537
ndegwamartin added a commit to opensrp/android-fhir that referenced this pull request Jun 4, 2024
       - With unmerged PR #9
       - WUP PR google#2178
       - WUP PR google#2511
       - WUP PR google#2464
ndegwamartin added a commit to opensrp/android-fhir that referenced this pull request Jun 10, 2024
           - With unmerged PR #9
           - WUP PR google#2178
           - WUP PR google#2511
ndegwamartin added a commit to opensrp/android-fhir that referenced this pull request Jun 11, 2024
- With unmerged PR #9
- WUP PR google#2178
- WUP PR google#2511
- WUP PR google#2537
- WUP PR google#2511
ndegwamartin added a commit to opensrp/android-fhir that referenced this pull request Jun 26, 2024
    - With unmerged PR #9
    - WUP PR google#2178
    - WUP PR google#2511
    - WUP PR google#2511
ndegwamartin added a commit to opensrp/android-fhir that referenced this pull request Jun 26, 2024
    - With unmerged PR #9
    - WUP PR google#2178
    - WUP PR google#2511
    - WUP PR google#2511
    - WUP PR google#2544
ndegwamartin added a commit to opensrp/android-fhir that referenced this pull request Jul 9, 2024
        - With unmerged PR #9
        - WUP PR google#2178
        - WUP PR google#2544
        - WUP PR google#2607
ndegwamartin added a commit to opensrp/android-fhir that referenced this pull request Jul 30, 2024
    - With unmerged PR #9
    - WUP PR google#2178

- WUP google#2652
- WUP google#2521
- WUP google#2645
- WUP google#2648
- WUP google#2650
@ndegwamartin
Copy link
Collaborator Author

This is blocked by/Superseded by this discussion - #2625

ndegwamartin added a commit to opensrp/android-fhir that referenced this pull request Sep 4, 2024
ndegwamartin added a commit to opensrp/android-fhir that referenced this pull request Sep 10, 2024
FORK
         - With unmerged PR #9
            - WUP  #13

SDK
            - WUP google#2178
            - WUP google#2650
            - WUP google#2663
PERF
- WUP google#2669
- WUP google#2565
- WUP google#2561
- WUP google#2535
ndegwamartin added a commit to opensrp/android-fhir that referenced this pull request Sep 10, 2024
@jingtang10
Copy link
Collaborator

This is blocked by/Superseded by this discussion - #2625

why is this blocked by custom resources?

ndegwamartin added a commit to opensrp/android-fhir that referenced this pull request Oct 2, 2024
    FORK
             - With unmerged PR #9
                - WUP  #13

    SDK
                - WUP google#2178
                - WUP google#2650
                - WUP google#2663
    PERF
    - WUP google#2669
    - WUP google#2565
    - WUP google#2561
    - WUP google#2535
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: PR under Review
Development

Successfully merging this pull request may close these issues.

New API to support saving local only resources
7 participants