fix(#385): Production instance on production app should not have red border #386
Conversation
|
Excellent work tracking this down! I really love how well researched the parent ticket it is. Does this PR allow each flavor/brand of the APK to have it's own |
Yes, you can override it with flavor specific value, similar to other values. If you don't provide, it will default to |
| this.requestWindowFeature(Window.FEATURE_NO_TITLE); | ||
| setContentView(R.layout.main); | ||
|
|
||
| // Add an alarming red border if using configurable (i.e. dev) | ||
| // app with a medic production server. | ||
| if (settings.allowsConfiguration() && appUrl != null && appUrl.contains("app.medicmobile.org")) { | ||
| if (settings.allowsConfiguration() && appUrl != null && appUrl.contains("app.medicmobile.org") && !isProductionApp) { |
There was a problem hiding this comment.
Looking at this and the rest of the context aroud this functionality, I wonder if the core issue is that we should have updated this line when adding support for multiple prod instances so that it does not apply the border when allowCustomHosts = False. Basically, I think setting this would get the behavior you need:
| if (settings.allowsConfiguration() && appUrl != null && appUrl.contains("app.medicmobile.org") && !isProductionApp) { | |
| if (settings.allowCustomHosts() && appUrl != null && appUrl.contains("app.medicmobile.org")) { |
The goal of this red border (as it was originally implemented) was to warn when someone had entered a prod URL into the Custom url box of the Unbranded app. When we did #313, we added a new allowCustomHosts setting to allow for toggling off the custom URL box for apps like moh_kenya_echis which presents a list of instances to choose from, but does not allow for entering a custom URL. As a part of that work, we should have changed this line from allowsConfiguration to allowCustomHosts because now allowsConfiguration just indicates if there is a list of multiple instances to choose from whereas it is allowCustomHosts that would actually show the Custom URL box where a user could enter a prod URL....
There was a problem hiding this comment.
Disabling custom hosts would prevent us from leveraging this feature for production apps. While not a dealbreaker, it does limit the flexibility we could achieve with a single app. For example, it would restrict:
- Allowing users to log into arbitrary test instances for debugging or testing purposes.
- Enabling users to log into new production instances before those instances are formally integrated into the app.
There was a problem hiding this comment.
Okay, so maybe I am misunderstanding the scope of what you are trying to achieve here. Sorry! I thought that the purpose of this PR was just to allow an app with the moh_kenya_echis-style multi-instance list to be able to include app.medicmobile.org URLs in the instance list without triggering the red border.
My understanding is that the goal of the red border, when it was introduced, was to be shown anytime someone entered a "production" URL into the Custom URL box (which at the time was only shown in the Unbranded app). Now, it is technically possible to show the Custom URL option in other app flavors (by setting allowCustomHosts = true and not setting a dedicated app_host.
@binokaryg do you have a new use-case now where you need to allow the Custom URL option in a production app and you do NOT want to see the red border (even if the user enters a "production" URL into the Custom box)? If that is the case, I think we may want to re-think the need for the red border at all, since it seems like the assumptions from the original purpose for the border are no longer holding true.... (And, frankly, using app.medicmobile.org to determine if the URL is "production" is not even applicable for many self-hosted instances anyway.)
There was a problem hiding this comment.
anytime someone entered a "production" URL into the Custom URL
Is this a bad idea?
I'm asking this because that's how majority of the app users are using it here.
@binokaryg do you have a new use-case now where you need to allow the Custom URL option in a production app and you do NOT want to see the red border
Now we want to move to a branded app with a list of instances to choose from. Although it's not a requirement to have Custom URL option, it's still good to have for flexibility.
If we want to have a fine control over which URLs show warning and which don't, shall we move this flag to the individual instances in the instance list?
There was a problem hiding this comment.
I did some digging and it appears this functionality has been basically unchanged since it was added 9 years ago. That PR notes:
Add an alarming red border if using configurable (i.e. dev) app with a medic production server.
Then, as now, the border got triggered when there was not a hard-coded app_host URL, but instead the user could enter a custom URL to connect to an instance.
Based on the above quote, my inference is that folks were using the Unbranded (aka configurable) app to allow their users to connect to testing/training instances. The intention being to use the proper branded app to connect to the actual production instance. Then, in order to prevent test/training data from being accidentally entered into a production instance, this border was added to warn the user that they were connecting to a production instance from an app intended for testing/training.
You mentioned:
Is this a bad idea?
I'm asking this because that's how majority of the app users are using it here.
Technically speaking, there should be no issue with connecting to a production instance via a Custom URL. (Assuming that is intended and there is no danger of test/training data being entered in prod). However, this is definitely not the recommended way to have users connect. Besides just having the extra step of making sure the prod URL is correctly entered for each user, you also will not be able to use any kind of deep links when connecting via a Custom URL.
Now we want to move to a branded app with a list of instances to choose from.
My proposed solution of just changing allowsConfiguration to allowCustomHosts should make it so that the border is not shown when connecting to Medic prod apps chosen from a configured list.
Although it's not a requirement to have Custom URL option, it's still good to have for flexibility.
This is where I think your approach here might differ from the previous thoughts/intentions that went into the red border. That being said, if your existing users have already been connecting via a custom URL in the unbranded app and seeing the red border, then clearly the "warning" is really not providing much value (at least in your case).
In general, I think that the idea we can detect a "production" instance by checking the URL is now fundamentally flawed since there is no way to account for the URLs of self-hosted prod instances.
Suggested next steps
I think that the allowsConfiguration to allowCustomHosts change is the proper minimal fix for the #385 bug and we should make that change to close the bug (either in this PR or I can open a different one).
For the case of your new branded app, I think YAGNI would argue that you should not enable the Custom URL box until it is actually a requirement that you need it.
If you do identify a use case where it is necessary to support a Custom URL in your branded app (or use the Unbranded app in prod), I think we should open a new issue and the specifics of that use case will help direct the best way we can proceed with handling the red border. It is tricky to anticipate potential needs and add logic to address possible cases. However, if/when we have a specific case, that can help us determine the best path forwards. Several possible solutions would be:
- Remove the red border all together. Folks can use the Unbranded app and/or the Custom URL box at their own risk. The red boarder is imperfect anyway since it only flags Medic-hosted prod instances (and not self-hosted ones).
- If you need the Custom URL for connecting to test/training instances, you could instead publish a branded training app with the links to the test instance(s) pre-configured.
- Add support for just disabling the red border for a particular brand (regardless of if the Custom URL box is shown)
- For a multi-instance brand we could add support for configuring the border to show/hide at the instance-by-instance case
Excellent! I see Josh has some more detailed input, but this was my only question - I'll defer to others on the rest of the PR - thanks! |
instead of allowsConfiguration, as per PR feedback Co-authored-by: Joshua Kuestersteffen <[email protected]>
Fixes #385:
productioninsrc/main/res/values/bools.xml.EmbeddedBrowserActivity.javato check the production flag and prevent the display of the red border if the app is in production mode.