Don't let user save invalid preferences

[seadowg]

Closes #2086

This blocks a user from saving preferences from a QR code where the value type doesn't match the default for that key.

There is also a second change that warns the user that they currently have invalid preferences saved. This prevents a crash that was occurring and let's them know they need to re import. This was a minor change just to account for anyone currently stuck in the scenario discovered in #2086.

What has been done to verify that this works as intended?

Robolectric level tests added for GeneralSharedPreferences and AggregatePreferencesAdder. Manually verified using a device and both a valid and invalid QR code.

I'd like for there to be more tests around the error handling at a Fragment level but I ran into problems with PreferencesFragment and Robolectric` not playing nice. I'm up for digging into this further but didn't want to hold up a fix to the issue.

Why is this the best possible solution? Were any other approaches considered?

I did consider reworking GeneralSharedPreferences so that individual preferences have type safety and (i.e. there is a Preference object or a setX method). I'm probably going to look at this next but again it felt like a big change to make before getting a fix in.

I also think it would be good to try and remove the singletons stored at a process level (both GeneralSharedPreferences and GENERAL_KEYS). Generally I've found that having singletons that live beyond the Application lifecycle creates problems with testing as both Robolectric and plain ol' JUnit tests can run into weird scenarios with test pollution. Again I'd like to potentially look at this next.

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?

The only major change is it blocking them from saving "invalid" preferences. I'd definitely like a sense check on if there are any major risks around that!

Do we need any specific form for testing your changes? If so, please attach one.

N/A

Does this change require updates to documentation? If so, please file an issue here and include the link below.

Before submitting this PR, please make sure you have:

• run ./gradlew checkAll and confirmed all checks still pass OR confirm CircleCI build passes and run ./gradlew connectedDebugAndroidTest locally.
• verified that any code or assets from external sources are properly credited in comments and/or in the about file.
• verified that any new UI elements use theme colors. UI Components Style guidelines

[seadowg]

My bad. Misread the first checkbox and ran all the tests but not the PMD check. Will fix now.

[lognaturel]

A few surface comments, suggestions and questions but overall it looks great to me!

[lognaturel]

Just remembered you had some notes in the PR description I wanted to respond to.

I also think it would be good to try and remove the singletons stored at a process level (both GeneralSharedPreferences and GENERAL_KEYS).

Agreed. Some discussion around the best way to do this would definitely be appreciated. @shobhitagarwal1612 has been wanting to discuss #1871 and other alternative approaches.

[kkrawczyk123]

I can see a crash when rotate device on Fill Blank Form view on all my Android devices from 4.1 to 8.1.

I scanned code attached in the issue, I saw the toast telling me that QR code does not contain valid settings, but does that mean that none settings are imported at all? When I am using dark mode n Androids 6.0 and 7.0, I am losing it after scanning that code. Go out of admin settings and open fill blank form or any other option - light mode is visible. When I kill an app and open again I have light mode (Android 7.0) and on Android 6.0 I am losing dark mode immediately. It makes me feel that something is exported anyway. @seadowg, could you please investigate that?

@opendatakit-bot unlabel "needs testing"

[seadowg]

@kkrawczyk123 hey! So I'm not able to reproduce the crash. I'm pretty new to the app so maybe some step by step instructions to reproduce would be helpful?

In terms of it saving some settings and not the invalid ones I think that is what happens. It actually is a little strange:

1. QR code is scanned
2. App looks at for for each key it might expect to be in the data (i.e. "server_url" or "password") from the QR code and then:
   • If it is: set that value in settings
   • If it is but the value is invalid: don't set the value and go straight to 3
   • If it isn't: reset the value to the default
3. Return to home if successful or show an error if not

I guess that this means depending on what key is invalid it might save some settings, reset others but then fail before resetting others to their default value. It's a pretty confusing scenario for the user I think. Does it make sense do you think to reset all the values in settings to their default if importing fails?

Sorry if none of that makes any sense I found it quite hard to describe!

[shobhitagarwal1612]

Just an idea:
Since we have a QRCode of currently applied settings (saved in directory), we can use that as a fallback whenever the scanned QRCode is faulty to revert back to the last known good state.
I think that's what people in general would expect whenever something bad happens.

[seadowg]

@shobhitagarwal1612 that also works as a fallback!

[seadowg]

@kkrawczyk123 this should be good for testing again. I haven't had a chance myself as I need access to a device and had to leave my office early today. If you get a chance though let me know!

[kkrawczyk123]

Tested with success!
Verified on Androids: 4.1, 4.2, 4.4, 6.0, 7.0 and 8.1.

@opendatakit-bot label "behavior verified"

[seadowg]

@kkrawczyk123 🙌. Thanks so much!

[zestyping]

This looks all set and ready to go! @yanokwa, want to approve?

[seadowg]

@shobhitagarwal1612 ah sorry should have caught those. Been on vacation but will take a look at updating when I get a chance

[seadowg]

@shobhitagarwal1612 that's fixed now! Sorry for the delay: just got back from vacation 🏝

[shobhitagarwal1612]

Thank you so much @seadowg 🎉 🎉