Improved handling errors when starting new forms or editing existing ones

[grzesiek2010]

Work towards #5419

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

I've tested the changes manually with the tester app: https://github.com/grzesiek2010/collectTester and added automated tests.

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

The goal of this issue was to block editing of finalized or sent forms via Content Provider. I've handled that in FormUriActivity.
As we discussed earlier FormUriActivity should be like a gate to form filling where we check if it makes sense to start the process at all (data validation). I've taken the opportunity and improved that class by validating other cases as well so that now the class contains a series of methods asserting that everything is ok and form filling can be started.
I've also made sure FormEntryActivity is not used directly in our app but only by FormUriActivity after validating all cases: 29ebf51
I could remove some duplicate validations from FormEntryActivity now as it is checked in FormUriActivity but there is one problem... FormEntryActivity is an exported activity and can be started from an external app by its name (see the tester app). It shouldn't work like that because now even if we block editing of finalized or sent forms via Content Provider it is still possible using FormEntryActivity directly. I was a little bit afraid of changing this state and making the activity not exported because maybe there are some projects that rely on it so I decided to leave it as is for now and add analytics: a120365

Initially, I wanted to disable editing finalized and sent forms as described in the issue but I ended up with a lot of failing tests and the UI where those finalized forms were still in the list of forms to edit but couldn't be edited. We have a separate issue for that: #5418
Since the pr contains a lot of improvements in FormUriActivity as described above I think it would be good to test it and merge and disable editing finalized forms in a separate one. That's why in the last commit I brought back the original behavior (editing finalized forms).

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?

Please test editing forms using the tester app: https://github.com/grzesiek2010/collectTester. I've implemented some refactoring so it probably would make sense to test starting forms using that app (including some weird scenarios when forms should not be started see what cases should block starting a form FormUriActivityTest)

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

[seadowg]

Initially, I wanted to disable editing finalized and sent forms as described in the issue but I ended up with a lot of failing tests and the UI where those finalized forms were still in the list of forms to edit but couldn't be edited. We have a separate issue for that: #5418

Agreed, this makes sense as a refactor before working on #5419. As you point out, I think it makes sense to work on #5418 before that as well, and the issues are prioritized like that in the backlog.

FormEntryActivity is an exported activity and can be started from an external app by its name (see the tester app)

FormEntryActivity itself isn't exported as far as I can see - there's an activity-alias that is. I think this means that you should be able to change the targetActivity of the alias to FormURIActivity right?

[grzesiek2010]

FormEntryActivity itself isn't exported as far as I can see - there's an activity-alias that is. I think this means that you should be able to change the targetActivity of the alias to FormURIActivity right?

This would cause an infinite loop 😄 FormURIActivity -> FormEntryActivity -> FormURIActivity etc.
[EDIT] It would probably work if I renamed FormEntryActivity to something else like FormFillingActivity so that its name is different than the name of the alias. Do you think it's a good solution?

Generally, I don't know why that alas is there at all... it looks like:

        <activity-alias
            android:name=".activities.FormEntryActivity"
            android:targetActivity=".activities.FormEntryActivity"
            tools:ignore="AppLinkUrlError"
            android:exported="true">
        </activity-alias>

so android:name and android:targetActivity hold the same value and we shouldn't need this alias in this case right?

[seadowg]

This would cause an infinite loop 😄 FormURIActivity -> FormEntryActivity -> FormURIActivity etc.
[EDIT] It would probably work if I renamed FormEntryActivity to something else like FormFillingActivity so that its name is different than the name of the alias. Do you think it's a good solution?

Oh no! I thought that aliases only worked outside of the app, so that wouldn't happen. I agree that just renaming FormEntryActivity would be a good solution here.

[grzesiek2010]

I agree that just renaming FormEntryActivity would be a good solution here.

Done. Thanks to that I was able to remove all the checks (if data is valid) from FormFillingActivity (FormEntryActivity before) and move it to FormUriActivity which reduced the complexity of FormFillingActivity a little bit (always nice!).

I think the next step to make would be to pass the Form/Instance object to FormFillingActivity instead of URI. FormFillingActivity does not have to deal with unpacking URI and fetching those objects from the database we can just pass it to this activity. If you agree I will file a separate issue for that. What do you think?

[seadowg]

I think the next step to make would be to pass the Form/Instance object to FormFillingActivity instead of URI. FormFillingActivity does not have to deal with unpacking URI and fetching those objects from the database we can just pass it to this activity. If you agree I will file a separate issue for that. What do you think?

Maybe just passing in the Form or Instance DB ID would be best? That way can avoid serializing the object or risking anything big getting written out to disk with the rest of the intent. It'd be great to avoid issues for this kind of thing because it's always hard to work out prioritization for refactors vs features. Just go ahead and make the change as a follow-up PR, or include here if you think it's small?

[seadowg]

I'm really liking this change so far. Feels much cleaner. I've still got to have a good look through FormUriActivityTest, but I thought it'd make sense to get these thoughts in early as I think it'll be ready for QA after these few little tweaks.

[seadowg]

I still have to review the one test file (and I probably won't get to it until next week), but this is ready for QA any changes there shouldn't affect behaviour.

All requested changes made, but still more to review.

[dbemke]

Tested with Success!

Verified on device with Android 10

Verified cases:

• reproducing steps from automated tests
• regression checks in Fill Blank Form and Edit Saved Form in Collect
• ODK Collect Intents Testers app

Since most of the tests rely on ODK Collect Intents Testers app which doesn’t work on devices with Android 11 and above, testing was conducted on Android 10. On Android 13 there were only regression checks in the Collect app. Because of that we don’t remove the label needs testing. If you decide that testing with ODK Collect Intents Testers app only on Android 10 is enough, please remove the label.

[dbemke]

After the update of ODK Collect Intents Testers app I reproduced the errors/cases again on Android 10 and everything works well.

[srujner]

Tested with Success!

Verified on device with Android 13

• I also checked once again on Android 13 and everything works correct now, the intents Tester works well after the newest fix.

[seadowg]

My bad on closing! Pro-tip: Android Studio's GitHub plugin has a button that says "Close Pull Request" - it doesn't close the window with the PR info.

[seadowg]

@grzesiek2010 looks like this needs a final conflict resolve before merging