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 the new eCBIS flavor | main #2610

Open
wants to merge 20 commits into
base: main
Choose a base branch
from

Conversation

dubdabasoduba
Copy link
Member

@dubdabasoduba dubdabasoduba commented Jul 20, 2023

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

  • This addition will allow our client to install apps targetting the preview server and the prod server on the same device.
  • The logos on the preview APK have been changed for clarity on what each APK does

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

@dubdabasoduba dubdabasoduba requested a review from pld July 20, 2023 19:28
Comment on lines 177 to 181
create("ecbis_liberia") {
dimension = "apps"
applicationIdSuffix = ".ecbis_liberia"
versionNameSuffix = "-ecbis_liberia"
manifestPlaceholders["appLabel"] = "MOH eCBIS"
Copy link
Member

Choose a reason for hiding this comment

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

ah so we're going to make the current one preview and have the new one be production? I think we can put "production" in the name instead of "liberia" for clarity

Copy link
Member Author

Choose a reason for hiding this comment

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

Awesome!

Copy link
Contributor

Choose a reason for hiding this comment

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

I think we should do the reverse since the production app is what the client will interact with even on Playstore and should be concise, that is

  • For prod - the flavor should be ecbis and the suffix .ecbis
  • For preview - the flavor should be ecbis_preview and the suffix .ecbis_preview

Copy link
Member Author

Choose a reason for hiding this comment

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

@ndegwamartin The thinking on the change was to force them to download the new prod APK. If we use the same package name they will just update which means they don't clear the existing preview data on the device. I am also not sure if the server URL will be correctly updated.

Copy link
Contributor

Choose a reason for hiding this comment

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

If we are unable to enforce the integrity for a fresh install then I suppose we can follow that approach. We will however need a new Playstore release APK upload and verification process for the new application id cc @pld

Copy link
Member Author

Choose a reason for hiding this comment

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

That's a nice catch. Does the mean we need to de-list the current app?

Copy link
Member

Choose a reason for hiding this comment

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

k so then we'd have 2 releases on play store the prod and preview one? I'm fine w/that, I'd worry a bit that if we have to de-list the current app, we take the risk of delay on getting up a new app

@codecov
Copy link

codecov bot commented Jul 20, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 60.6%. Comparing base (ac82739) to head (52cb3b6).
Report is 89 commits behind head on main.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##              main   #2610      +/-   ##
==========================================
+ Coverage     29.6%   60.6%   +30.9%     
- Complexity     658    1067     +409     
==========================================
  Files          239     247       +8     
  Lines        11204    9979    -1225     
  Branches      1948    1778     -170     
==========================================
+ Hits          3323    6049    +2726     
+ Misses        7447    2897    -4550     
- Partials       434    1033     +599     
Flag Coverage Δ
geowidget 18.7% <ø> (-28.5%) ⬇️

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

see 93 files with indirect coverage changes

@dubdabasoduba dubdabasoduba changed the title Add the new eCBIS flavor Add the new eCBIS flavor | main Jul 21, 2023
@dubdabasoduba dubdabasoduba self-assigned this Jul 21, 2023
Copy link
Member

Choose a reason for hiding this comment

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

shouldn't we use the ecbis logo w/a banner over it or something

Copy link
Member Author

Choose a reason for hiding this comment

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

We wanted the distinction on the installation hence the need to use different icons.

I will be updating the configs on preview to change the app name on the UI to read preview too

@ellykits
Copy link
Collaborator

@dubdabasoduba Do we still need to merge this PR?

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

4 participants