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

Create Insights screen and show user info, practitioner details and sync statistics #2873

Merged
merged 58 commits into from
Dec 7, 2023

Conversation

brandyodhiambo
Copy link
Contributor

@brandyodhiambo brandyodhiambo commented Nov 21, 2023

IMPORTANT: Where possible all PRs must be linked to a Github issue

Fixes #2857

Engineer Checklist

  • I have written Unit tests for any new feature(s) and edge cases for bug fixes
  • I have added any strings visible on UI components to the strings.xml file
  • I have updated the CHANGELOG.md file for any notable changes to the codebase
  • I have run ./gradlew spotlessApply and ./gradlew spotlessCheck to check my code follows the project's style guide
  • I have built and run the FHIRCore app to verify my change fixes the issue and/or does not break the app
  • I have checked that this PR does NOT introduce breaking changes that require an update to Content and/or Configs? If it does add a sample here or a link to exactly what changes need to be made to the content.

Code Reviewer Checklist

  • I have verified Unit tests have been written for any new feature(s) and edge cases
  • I have verified any strings visible on UI components are in the strings.xml file
  • I have verifed the CHANGELOG.md file has any notable changes to the codebase
  • I have verified the solution has been implemented in a configurable and generic way for reuseable components
  • I have built and run the FHIRCore app to verify the change fixes the issue and/or does not break the app

Copy link

codecov bot commented Nov 21, 2023

Codecov Report

Merging #2873 (6479f45) into main (ad3a737) will increase coverage by 2.3%.
Report is 116 commits behind head on main.
The diff coverage is 65.4%.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff            @@
##              main   #2873     +/-   ##
=========================================
+ Coverage     64.5%   66.9%   +2.3%     
- Complexity    1075    1103     +28     
=========================================
  Files          218     223      +5     
  Lines         9635   10434    +799     
  Branches      1897    1920     +23     
=========================================
+ Hits          6218    6984    +766     
+ Misses        2234    2224     -10     
- Partials      1183    1226     +43     
Flag Coverage Δ
engine 70.4% <67.9%> (-2.3%) ⬇️
geowidget 65.5% <33.3%> (+1.1%) ⬆️
quest 64.6% <64.2%> (+5.7%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files Coverage Δ
...ngine/configuration/view/CompoundTextProperties.kt 100.0% <ø> (ø)
...gine/configuration/workflow/ApplicationWorkflow.kt 100.0% <100.0%> (ø)
...r/fhircore/engine/domain/model/RoundingStrategy.kt 100.0% <100.0%> (ø)
...martregister/fhircore/engine/sync/AppSyncWorker.kt 100.0% <ø> (ø)
...r/fhircore/engine/task/FhirResourceExpireWorker.kt 100.0% <100.0%> (ø)
...fhircore/engine/task/FhirTaskStatusUpdateWorker.kt 100.0% <100.0%> (ø)
...g/smartregister/fhircore/engine/ui/theme/Colors.kt 100.0% <100.0%> (ø)
...gister/fhircore/engine/util/SharedPreferenceKey.kt 100.0% <100.0%> (ø)
...rcore/engine/util/extension/FhirEngineExtension.kt 80.0% <100.0%> (+2.8%) ⬆️
...fhircore/engine/util/extension/StringExtensions.kt 91.6% <100.0%> (ø)
... and 83 more

... and 28 files with indirect coverage changes

brandyodhiambo and others added 18 commits November 21, 2023 10:34
…ange

# Conflicts:
#	android/quest/src/main/java/org/smartregister/fhircore/quest/ui/usersetting/UserSettingInsightScreen.kt
#	android/quest/src/main/java/org/smartregister/fhircore/quest/ui/usersetting/UserSettingScreen.kt
…een' into feature/user_setting_insight_screen

# Conflicts:
#	android/quest/src/main/java/org/smartregister/fhircore/quest/ui/usersetting/UserInsightScreenFragment.kt
#	android/quest/src/main/java/org/smartregister/fhircore/quest/ui/usersetting/UserSettingInsightScreen.kt
…_setting_screen_modification

# Conflicts:
#	android/quest/src/androidTest/java/org/smartregister/fhircore/quest/integration/ui/usersetting/UserSettingScreenTest.kt
#	android/quest/src/main/java/org/smartregister/fhircore/quest/ui/usersetting/UserSettingScreen.kt
#	android/quest/src/main/java/org/smartregister/fhircore/quest/ui/usersetting/UserSettingViewModel.kt
#	android/quest/src/main/java/org/smartregister/fhircore/quest/ui/usersetting/UserSettingsEvent.kt
@sharon2719
Copy link
Contributor

@brandy-kay Please attach a video with the new changes.

@Raynafs
Copy link
Contributor

Raynafs commented Dec 5, 2023

I have uploaded the video showing the new User Insight Screen view. From the recording there are views that are not visible because we are using a non proxy variant. You can test it using a proxy variant to see those views. @ellykits

Screencast.from.05-12-2023.05.57.12.ALASIRI.webm

@@ -24,7 +24,7 @@
<string name="syncing_down" translatable="false">Syncing down</string>
<string name="syncing_initiated">Sync initiated&#8230;</string>
<string name="sync_failed">Sync failed. Check internet connection or try again later</string>
<string name="sync_completed_with_errors">Sync completed with errors. Retrying…</string>
<string name="sync_completed_with_errors" translatable="false">Sync completed with errors. Retrying…</string>
Copy link
Collaborator

Choose a reason for hiding this comment

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

This string should be translatable. Remove translatable=false property.

Comment on lines 148 to 149
<string name="percentage_progress" translatable="false">%1$d%%</string>
<string name="error_occurred" translatable="false">Something went wrong...</string>
Copy link
Collaborator

Choose a reason for hiding this comment

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

These strings should be translatable. Remove translatable=false property.

Comment on lines 250 to 257
headingOne: String?,
contentOne: String?,
headingTwo: String?,
contentTwo: String?,
headingThree: String?,
contentThree: String?,
headingFour: String?,
contentFour: String?,
Copy link
Collaborator

@ellykits ellykits Dec 6, 2023

Choose a reason for hiding this comment

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

This can be refactored to accept any n number of items. The header + content information can be captured in a Pair class as key (header) and value(content). Have the items in LIST. Iterate through the list to render the desired content. (The field properties like color can be configured). If need be you may create a new data class to capture the required items if a Pair class will not suffice.

@ellykits
Copy link
Collaborator

ellykits commented Dec 6, 2023

For the UI @Raynafs Can you only display a section if there is content below the section header? The header section should not appear if there is no content to display. From the video shared Unsynced Resources is empty and User Info is empty as well. Should we have some defaults to display @HenryRae if no content is found?

@Raynafs
Copy link
Contributor

Raynafs commented Dec 6, 2023

@ellykits Yes sure working on it

@Raynafs
Copy link
Contributor

Raynafs commented Dec 7, 2023

@ellykits

I have edited the screen to show a different view when synced and unsynced. Kindly check.

When all the data is synced
image

When we have unsynced data
image

@murayakarina
Copy link

@ellykits
Can we have a new section called Server info where we can include details like the SERVER URL? We can also include the keycloak realm and the client ID.

@SebaMutuku
Copy link
Contributor

SebaMutuku commented Dec 7, 2023

@karina4050. Thanks for the suggestion.
Can this be handled in phase two as an enhancement. Also, do you mind checking with @HenryRae on the same to do a new design with the updates?

@ellykits
Copy link
Collaborator

ellykits commented Dec 7, 2023

@ellykits Can we have a new section called Server info where we can include details like the SERVER URL? We can also include the keycloak realm and the client ID.

We can include some server information like version etc. Keycloak details are sensitive as they are used during deployment.

Copy link
Collaborator

@ellykits ellykits left a comment

Choose a reason for hiding this comment

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

The PR looks good now. Resolve the issue with the strings so we can merge.

Comment on lines 163 to 176
<string name="select_month" translatable="false">Select Month</string>
<string name="user" translatable="false">User</string>
<string name="team" translatable="false">Team</string>
<string name="locality" translatable="false">Locality</string>
<string name="team_organization" translatable="false">Team(Organization)</string>
<string name="care_team" translatable="false">Care Team</string>
<string name="location" translatable="false">Location</string>
<string name="app_version_code" translatable="false">App version code</string>
<string name="build_date" translatable="false">Build date</string>
<string name="manufacture" translatable="false">Manufacture</string>
<string name="app_versions" translatable="false">App version</string>
<string name="os_version" translatable="false">OS Version</string>
<string name="device_date" translatable="false">Date</string>
<string name="device" translatable="false">Device</string>
Copy link
Collaborator

Choose a reason for hiding this comment

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

These Strings should be translatable. Delete the "translatabale = false" property.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Actually can you deleted this property in all the strings, only the app_name should not be translatable and any branding texts.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes sure. Working on it

Copy link
Contributor Author

Choose a reason for hiding this comment

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

also, when we remove translatable and the strings get highlighted with a red line, does it mean we have to provide the translations on the alternative string resource like that for Swahili and French? @ellykits @SebaMutuku

@Raynafs Raynafs requested a review from ellykits December 7, 2023 14:04
Copy link
Collaborator

@ellykits ellykits left a comment

Choose a reason for hiding this comment

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

LGTM 👍

@Raynafs Raynafs merged commit 8e48e2c into main Dec 7, 2023
@Raynafs Raynafs deleted the feature/users_setting_screen_modification branch December 7, 2023 14:13
@ndegwamartin ndegwamartin mentioned this pull request Apr 12, 2024
1 task
qiarie pushed a commit that referenced this pull request Jan 15, 2025
…ync statistics (#2873)

* Added user first and last name

* fixes the user assigned area

* Changes OPENSRP logo color to match new design (black and white)

* addes location details

* completes the user setting ui editing

* Adds refresh with translations and insights page

* addes the user setting ui tests

* Adds info view

* addes the user setting viewmodel tests

* runs spotless checks

* Creating UserInsight Screen

* addes navigation to insight screen

* minor edits

* designs the user insight screen

* adds user insight fragment test and user viewModel test

* Writes tests for UserInsight Screen

* minor edits

* merges user insight branch

* Setting up unsynced screen

* works on getting database version

* finally the unsynched data is represented on the ui

* writes user setting viewmodel test

* completes user setting viewmodel test and edited user insight screen test

* edits the organization text view

* adds a separator between user info and unsynced resources

* removes hello blank fragment from quest string resource.

* fixes code format, removes unused unsyncedResources,removes unessesary plugin and renames OnOfflinemap to onLaunchOfflineMaps

* Update UserSettingInsightScreen to match design

* edits user insight screen components to use a single view

* fixes Questionnaire activity test

* fixes Questionnaire activity test

* runs spotless check

* runs spotless check

* still runs spotless check

* edits insight info view

* edits insight info view

* Fix UserInsightScreen padding

* Show unsynced and synced views

* Add string translations

* Fix UserInsightScreen

* Remove all unnecessary translatable=false in string.xml

---------

Co-authored-by: Raynafs <[email protected]>
Co-authored-by: Sharon Akinyi <[email protected]>
Co-authored-by: Benjamin Mwalimu <[email protected]>
Co-authored-by: Peter Lubell-Doughtie <[email protected]>
Co-authored-by: Sebastian <[email protected]>
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.

Enhance the Sync Insights UI
9 participants