Set usesEntities to true if there is an entityList attachment in the manifest #6494
Conversation
| false, | ||
| false, | ||
| new ManifestFile("", List.of( | ||
| new MediaFile("file1", Md5.getMd5Hash(new ByteArrayInputStream("contents-updated".getBytes())), "http://file1", true) |
There was a problem hiding this comment.
The contents don't necessarily need to be updated! It looks like when processing an update, if type=entityList is seen in the manifest, the list is processed to a database even when the contents haven't changed.
There was a problem hiding this comment.
Whenever the branch at https://github.com/getodk/collect/blob/master/collect_app/src/main/java/org/odk/collect/android/formmanagement/ServerFormUseCases.kt#L108 is hit, the form should be marked as entity-related.
There was a problem hiding this comment.
I briefly thought that an alternative might be to move the check for an Entity List up so that it only happens if the media file contents have changed but actually we can't do that right now because the md5 hash will never match the manifest hash. We're relying on HTTP caching to not duplicate downloads. This will be improved as part of broader form download improvements.
There was a problem hiding this comment.
Ah, I looked at the details of how and where entityAttachmentsDetected is set and I believe that will work as desired. The test should be updated to make it clear that a content change is not necessary.
Too bad there are two passes over the manifest but we can likely improve that as part of download improvements as well.
There was a problem hiding this comment.
Ah, I looked at the details of how and where entityAttachmentsDetected is set and I believe that will work as desired. The test should be updated to make it clear that a content change is not necessary.
The logic needs to be updated as well if it should work in this way.
There was a problem hiding this comment.
Got it, that makes sense. I see that you've now added a special branch just for having detected any Entity List which matches https://github.com/getodk/collect/blob/master/collect_app/src/main/java/org/odk/collect/android/formmanagement/ServerFormUseCases.kt#L108
There was a problem hiding this comment.
Too bad there are two passes over the manifest but we can likely improve that as part of download improvements as well.
Now there is one see: 1d92ec3
grzesiek2010
force-pushed
the
COLLECT-6493
branch
from
8512058
to
96419f9
Compare
grzesiek2010
force-pushed
the
COLLECT-6493
branch
from
96419f9
to
1d92ec3
Compare
lognaturel
changed the title
|
We need to test upgrades to verify the issue- do we need the released master apk or some other apk might work? |
|
It would be best to use a master release APK. I’ll make one as soon as I can. Unfortunately I think it will be in a couple of hours. |
Thank you. We'll test other PRs and this one probably tomorrow |
|
I'm not sure if registration form (with an old entity spec) works as expected after upgrading.
When I add a new entity and refresh the list of blank forms only follow-up form gets updated, registration form doesn't get updated cause for that form nothing changed because of the new entity so this means that in case of registration forms publishing the new version of the form would change value to "true". |
|
The As you said, we're not going to detect that a form that only creates/updates Entities is entity-related until we get an update to the form itself. That's ok because forms with the old spec version don't have any offline effects. The only way to get offline effects is to update the Entity spec version. At that point, This PR only targets Entity List attachments. If you think there might be an issue with pure registration forms that don't have Entity List attachments, please file another issue. |
The only crack in that I can see is if someone has a newer spec entity form on their device in v2024.2 and then updates to v2024.3. I think that form wouldn't have opened in v20242 so it's a pretty out there scenario, but I want to acknowledge that in that situation their form wouldn't be blocked during updates until they updated the form itself. |
That's a good point! This is one of the reasons we want to leave at least a month between the Collect and Central releases -- it will reduce the chance of this happening. |
|
Tested with Success! Verified on a device with Android 10 Verified cases:
|
|
Tested with Success! Verified on a device with Android 15 |
Closes #6493
Why is this the best possible solution? Were any other approaches considered?
As discussed in the comments and the issue, we need to mark forms downloaded before the upgrade as using entities if they indeed rely on entities. Doing this when new entities arrive seems to be the simplest and most effective approach.
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?
This update modifies the database (specifically the useEntities column) when new entities are being downloaded to ensure that forms using entities are correctly marked, even if they existed prior to the introduction of this column. I don’t foresee any particular risks or areas needing additional careful testing.
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:
./gradlew connectedAndroidTest(or./gradlew testLab) and confirmed all checks still passDateFormatsTest