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

Unresolvable SMS permission prompt when submitting reports for CHT < 3.9 #322

Open
jkuester opened this issue Jul 19, 2023 · 6 comments
Open
Labels
Type: Bug Fix something that isn't working as intended

Comments

@jkuester
Copy link
Contributor

To recreate the behavior

  • On Android 6+ device, open cht-android 1.0.0+ app using a brand that does not have the SEND_SMS permission enabled
    • Unbranded version does not have SEND_SMS permission enabled
  • Connect to a CHT instance that is < 3.9.0
  • Submit app form that does not have any xml2sms configuration set
  • See prominent disclosure page for SMS permission
  • Press "Allow"
  • No prompt for SMS permission is displayed. Instead the Android system settings for the CHT app are opened.
    • You have to press the back button to return to the CHT app

Basically, the sending report as SMS functionality is getting triggered unintentionally and this is inadvertently triggering the SMS permission request which fails because the app does not have SEND_SMS permission enabled.

Details

Due to medic/cht-core#6162, submitting a report in cht-android for CHT versions < 3.9.0 will always trigger an SMS record for the report to be sent regardless of the xml2sms configuration for the form. In older versions of cht-android, if your brand was not configured with the SEND_SMS this send-attempt would always cause an error that was handled silently by the app (but visible in the adb logs).

In cht-android 1.0.0, we updated handling for the SEND_SMS permission to properly request permission from the user when trying to send a report via sms. BUT this only works when the SEND_SMS permission has been configured for the app brand. The idea is that to send reports via sms this is what is needed:

  1. Publish cht-android app with SEND_SMS permission registered
  2. Configure form with xml2sms value
  3. User approves permission request when prompted by app (should happen the first time the app tries to send a report via sms).

Unfortunately, when connecting to a < 3.9.0 CHT server, #2 is bypassed (because of the above-linked issue). Then, if #1 is not done (since presumably you do not want to use the xml2sms feature), the app cannot actually request the permission from the user in #3. When that logic fails in #3, it falls back to re-directing the user to the Android settings to manually set the permission for the app. However, in this case, without #1, it is actually impossible to set the permission (and no prompt is shown to the user for how to proceed). This will continue to happen each time a report is submitted.

Workaround

The good news is that I have identified a workaround that does not require updates to cht-core or cht-android. To avoid experiencing this issue when using cht-android 1.0.0+ to connect to a CHT server that is < 3.9.0, you can set the xml2sms configuration on all your forms to "xml2sms": "false". (It needs to be the string "false" and not just the boolean primitive false.) This will prevent the behavior from medic/cht-core#6162 and new reports submitted for those forms will NOT trigger the SMS workflow at all.

@jkuester jkuester added the Type: Bug Fix something that isn't working as intended label Jul 19, 2023
@Kymoraa
Copy link

Kymoraa commented Jul 20, 2023

Thank you for looking into this and sharing this feedback @jkuester
It is really useful and helpful

@jkuester
Copy link
Contributor Author

I realized I did not mention this in the original description, but one cht-android code change I have thought of that should address this issue would be to add a check to the SMS permission request workflow to short-circuit the process if the app has not registered the SEND_SMS permission. To even be able to prompt the user for permission to send SMS messages, the app has to have set SEND_SMS in the Manifest. If SEND_SMS, there is no point in even trying to request permission from the user (or re-direct to the Android settings) because the user cannot even manually grant the permission without SEND_SMS.

This code change would be beneficial not only because it would improve compatibility with CHT < 3.9, but also it would help make it easier to debug SMS report issues in the future! If someone is trying to configure xml2sms for some forms, but has forgotten to include SEND_SMS on their cht-android brand they will encounter this behavior even on CHT >= 3.9 (and it will probably not be obvious what it is that they are missing). If we had a check for SEND_SMS, we could at least log a coherent message saying that the SMS workflow was triggered but the permission was missing.

@Kymoraa
Copy link

Kymoraa commented Jul 20, 2023

Thanks for this update @jkuester
This will still require a re-creation and re-distribution of the APKs, right?
Between this and the solution to update all forms json and push the configuration changes which one is a more long term and fail proof way to go?

@jkuester
Copy link
Contributor Author

@Kymoraa correct. Taking advantage of any changes made in cht-android would require re-distribution of the APKs.

Honestly, either of the SEND_SMS apk fix or the xml2sms workaround should reliably address this issue going forwards. The only disadvantage of the xml2sms workaround that I have been able to think of is that you have to make sure to include this config in any new forms that get added to the server. Not a big effort, but does require some awareness in the long-term (but this only has to be done until the server is upgraded to 3.9+). I imagine that we will probably want to include the SEND_SMS apk fix in cht-android one way or the other. So if your current 1.x apk has not been widely distributed yet, you could just update your apk and then not have to think about this again. However, if it is going to be a large effort to re-distribute a new APK, the xml2sms workaround probably makes more sense.

@Kymoraa
Copy link

Kymoraa commented Jul 20, 2023

That makes sense @jkuester
A follow up question: If we later upgrade to a version greater than v3.9 e.g. v4.x, will we need to revert the xml2sms workaround and or set it to "true" for all the workflows?

@jkuester
Copy link
Contributor Author

Technically no, you would not need to make any changes to the xml2sms configuration when upgrading. Having "xml2sms": "false" will continue to signal that there is no sms message to send. In >=3.9 this will behave the same as just not having any xml2sms configuration set at all.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Type: Bug Fix something that isn't working as intended
Projects
None yet
Development

No branches or pull requests

2 participants