Disable SMS feature #2892
Disable SMS feature #2892
Conversation
There was a problem hiding this comment.
FYI quoting from this comment:
The goal here isn't to comment the code, but rather the most minimal change to disable the call to the SMS permission
It would be great if all this code could be kept as-is, but put behind a "feature flag" so that if true => SMS works perfectly and if false => SMS permission is not required by the app.
An additional benefit is that we won't have to make any tricky reverts (potentially months from now) in order to get things working again; just flip the switch.
| @@ -648,4 +654,13 @@ public void onChange(boolean selfChange) { | |||
| } | |||
| } | |||
|
|
|||
| private void showSMSFeatureDisabledDialogIfNeeded() { | |||
There was a problem hiding this comment.
Nit: Can we rename to e.g. disableSmsIfNeeded to clarify that this has side-effects?
| @@ -664,4 +664,6 @@ | |||
| <string name="background_location_disabled">This form wants to track your location but tracking is disabled. Please enable in the \u0020\u0020⋮\u0020\u0020 menu above.</string> | |||
| <string name="background_location_enabled">This form tracks your location. You can disable tracking in the \u0020\u0020⋮\u0020\u0020 menu above.</string> | |||
| <string name="open_file">Open file</string> | |||
| <string name="sms_feature_disabled_dialog_title">SMS feature disabled</string> | |||
| <string name="sms_feature_disabled_dialog_message">Sending forms via SMS has been disabled in v1.20.0 due to the new Google\'s permission policy. We will try to bring the feature back soon.</string> | |||
There was a problem hiding this comment.
I wonder if we can be more helpful here, e.g. Sending forms via SMS has been temporarily disabled due to Google\'s new permission policy as of Collect version 1.20.0. We will try to bring the feature back soon. If you depend on this feature, you can downgrade to version 1.19.0
Does ODK host downloadable APKs anywhere for users without access to the play store?
There was a problem hiding this comment.
We did a dialog for the removal of current() that I thought was great. You can find the PR https://github.com/opendatakit/collect/pull/2524/files.
In this case, the title should be "Sending via SMS Disabled" and the message should be "Sending submissions via SMS has been disabled due to Google Play Store restrictions. \n\nPlease make sure the person who configured this device reads the details below and takes action."
Then link to https://forum.opendatakit.org/t/17973 (the ID of a post that I'll write). This will allow us to put whatever actions people need to take there (including linking to APKs).
There was a problem hiding this comment.
Overall, I think this is great, @grzesiek2010. Just the dialog change then I think it should go into testing.
We do lose some data (e.g., people who upgrade will have to re-enable the setting), but it's not the worst thing in the world.
| @@ -664,4 +664,6 @@ | |||
| <string name="background_location_disabled">This form wants to track your location but tracking is disabled. Please enable in the \u0020\u0020⋮\u0020\u0020 menu above.</string> | |||
| <string name="background_location_enabled">This form tracks your location. You can disable tracking in the \u0020\u0020⋮\u0020\u0020 menu above.</string> | |||
| <string name="open_file">Open file</string> | |||
| <string name="sms_feature_disabled_dialog_title">SMS feature disabled</string> | |||
| <string name="sms_feature_disabled_dialog_message">Sending forms via SMS has been disabled in v1.20.0 due to the new Google\'s permission policy. We will try to bring the feature back soon.</string> | |||
There was a problem hiding this comment.
We did a dialog for the removal of current() that I thought was great. You can find the PR https://github.com/opendatakit/collect/pull/2524/files.
In this case, the title should be "Sending via SMS Disabled" and the message should be "Sending submissions via SMS has been disabled due to Google Play Store restrictions. \n\nPlease make sure the person who configured this device reads the details below and takes action."
Then link to https://forum.opendatakit.org/t/17973 (the ID of a post that I'll write). This will allow us to put whatever actions people need to take there (including linking to APKs).
grzesiek2010
force-pushed
the
disable_sms
branch
from
371b968 to
3e9e2a8
Compare
|
Tested with success Verified on Android: 4.2, 4.4, 5.1, 6.0, 7.0, 8.1 Verified cases:
@opendatakit-bot unlabel "needs testing" |
It's a temporarrly change we need to make because of #2738
What has been done to verify that this works as intended?
I tested settings in
General Settings -> Serverto be sure we are not able to have SMS feature enabled.I also tested the dialog which should be displayed if I had SMS feature enabled in the previous version.
Why is this the best possible solution? Were any other approaches considered?
I just disable the feature keeping the whole code in order to bring it back in the feature if possible.
I also added a dialog which is displayed if a user had SMS feature enabled in the previous version.
How does this change affect users? Describe intentional changes to behavior and behavior that could have accidentally been affected by code changes. In other words, what are the regression risks?
It removes SMS options from General Settings -> Server so users are not able to send forms via SMS anymore.
Do we need any specific form for testing your changes? If so, please attach one.
No.
Does this change require updates to documentation? If so, please file an issue here and include the link below.
No, because we haven't documented SMS feature yet getodk/docs#781
Before submitting this PR, please make sure you have:
./gradlew checkAlland confirmed all checks still pass OR confirm CircleCI build passes and run./gradlew connectedDebugAndroidTestlocally.