Fixed detecting duplicated projects

[grzesiek2010]

Closes #4817

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

I tested the fix manually and also added automated tests.

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

The problem took place when we tried to add a Demo project already having a Google Drive project.

• if you have a Google Drive project, server url in preferences is set to an empty string
• if you scan a qr code for a Demo project that qr code does not contain url (because default values are not included during generating qr codes) and also an an empty string is returned

then those urls were compared (and treated as equal of course) and the fact that we had two different types of projects was ignored what caused detecting a duplicated project.

The problem was difficult to test automatically because in SettingsConnectionMatcherTest we use InMemSettings which is a replacement for SharedPreferences but InMemSettings does not return the same defaults as our SharedPreferences and then everything seemed to fork fine (for example we had null instead of an empty string and in our code it was treated as two different values). I had to save those defaults first: https://github.com/getodk/collect/pull/4818/files#diff-19ba577600f3b2f53559efdece854ef897e0c0eb92058fecc6f68867cf7cd693R116
In a perfect world we should return the same defaults for production code and tests but maybe let's figure it out in a separate pr? @seadowg what do you think?

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's a safe fix and we can just test the scenario described in the issue to confirm the bug is fixed.

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.

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

[grzesiek2010]

@kkrawczyk123 @mmarciniak90 I also spotted another bug which gave us the possibility to add multiple demo projects. Both issues should be fixed.

[seadowg]

Ach yeah it's annoying that the SettingsConnectionMatcher needs to know about defaults seeing as the entire concept is kind of "hidden" due to that being a detail of SharedPreferencesSettings. We could have InMemSettings return the same defaults, but I'm a little wary of everything having to reference the same constant. I'm thinking there are a couple of different ways we could go to make things simpler:

• Have our Settings#getX() methods take defaults as a second arg. This would mean that SettingsConnectionMatcher could just not specify defaults when access server URL/protocol and it wouldn't need to know about our ProjectKeys.defaults.
• Add a isDefault method to Settings to expose whether a key's value is a default or not. This would allow SettingsConnectionMatcher to match missing values in the json with default values.

I think we could look at one of those for this PR, but also happy to just go with what we've got for now. I think I prefer the second one. What do you think?

[grzesiek2010]

Add a isDefault method to Settings to expose whether a key's value is a default or not.

to achieve this we would need to have a method like that for every single type (String, Boolean Int etc) because you can't create one isDefault() method and pass a key there as a parameter since you wouldn't know what method you need to call on shared preferences (getString(), getBoolean() etc).

am i right?

Even if we did that am not sure if it would work... comparing that values that come from json with settings that we have saved will always be tricky.
Let's assume we have a default project already saved then the value returned for server url will be: https://demo.getodk.org for shared preferences but might be an empty value for InMemSettings (if it doesn't use the same defaults). At the same time in json we can have nothing for server url (normal case) but I can also imagine cases where users that create qr codes on their own include that https://demo.getodk.org either way. In all the permutations the result of comparing them should be that they are equal.

I wonder if it wouldn't be better to change the whole process of detecting duplicated projects and maybe first save a new project without checking if it's duplicated and then compare all existing projects to detect if the new one is a duplication. If so remove it (if that's what a user wants). Thanks to that we would just compare values from our settings (not from json) what would narrow down that number of permutations). What do you think?

[seadowg]

to achieve this we would need to have a method like that for every single type (String, Boolean Int etc) because you can't create one isDefault() method and pass a key there as a parameter since you wouldn't know what method you need to call on shared preferences (getString(), getBoolean() etc).

I think it'd be possible to grab the value saved as an Object then compare it to the default to see if they're equivalent? It all still feels pretty awkward.

I wonder if it wouldn't be better to change the whole process of detecting duplicated projects and maybe first save a new project without checking if it's duplicated and then compare all existing projects to detect if the new one is a duplication. If so remove it (if that's what a user wants). Thanks to that we would just compare values from our settings (not from json) what would narrow down that number of permutations). What do you think?

Yeah, the whole way our defaults work is kind of ugly for this connection matcher stuff. I do like that we don't have to save defaults for pretty much everything else! Maybe saving a project is the right thing to do here, although it still doesn't feel quite right. I guess thinking along the same lines, we could alternatively convert the existing project settings to JSON and then use that for the comparison?

[grzesiek2010]

I guess thinking along the same lines, we could alternatively convert the existing project settings to JSON and then use that for the comparison?

still not a perfect solution. According to our implementation we do not include settings that are equal to defaults so if you have server url https://demo.getodk.org it won't be included in json that we generate but as I said before I can imagine a case where users that generate qr codes on their own include such defaults. In such a case comparison would return false.

[seadowg]

@grzesiek2010 yup. Seems like this is a really awkward one. I think your current solution is probably the best way to go for now. I might make a few tweaks as I know you're out this week.

[mmarciniak90]

Tested with success

Verified on Android versions: 8.1, 9.0, 10.0

Verified cases:

• Google Drive project existed -> added Demo project
• Central project existed -> added Demo project
• Demo project existed -> tried to add Demo project -> Duplicate project dialog appeared
• Central project existed -> tried to add the same Central project -> Duplicate project dialog appeared
• Google Drive project existed -> tried to add Google Drive project -> Duplicate project dialog appeared
• removed Demo project -> added Demo project

[kkrawczyk123]

Verified also on Androids 5.1 and 11.