Improve editing finalized forms #6698

grzesiek2010 · 2025-04-22T17:24:09Z

Closes #6688
Closes #6593
Closes #6594
Closes #6689
Closes #6686

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

#6688
I've decided to add a new column to instances.db called edit_of. This column will indicate whether a form is an edit of a finalized form, and if so, store the ID of the original form. This change will help address #6594. With this column, I'll be able to identify the latest edit and determine the sequence of edits for any given form. This seems to be everything I will need.

#6593
What initially seemed like a simple task turned out to be more complex than expected. As I mentioned earlier, I thought that the new edit_of column introduced for #6688, together with the existing LAST_STATUS_CHANGE_DATE, would be sufficient to address issues #6593 and #6594. Unfortunately, that assumption was incorrect.

I initially believed that LAST_STATUS_CHANGE_DATE could be used to sort edits and determine both the most recent edit and the sequence number of each edit. However, LAST_STATUS_CHANGE_DATE is updated every time an instance is modified in the database (even for status changes and rightly so). This makes it unreliable for determining the order of edits.

To illustrate the problem, consider the following scenario:

Finalize FormA.
Edit FormA and finalize (Edit 1).
Edit FormA (Edit 1) and finalize (Edit 2).
At this point, we correctly have FormA (Edit 1) and FormA (Edit 2).
Now try to send FormA (Edit 1). Whether the submission succeeds or fails, its status is updated to submitted or submission_failed, and so is its LAST_STATUS_CHANGE_DATE.
As a result, FormA (Edit 1) will now appear newer than FormA (Edit 2), even though it was created earlier.

To address this, I introduced a new column: editNumber. This allows us to store a fixed sequence number for each edit that doesn’t change, ensuring we can accurately determine the order of edits and identify the latest one.

#6594
As previously mentioned, the editNumber column is essential for correctly determining the order of edits. There's nothing more to discuss on this point.

#6689
Nothing important to discuss here - I just fixed how we observe the process of editing a finalized instance.

#6686
Nothing to discuss here.

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?

#6688
This should only prevent the creation or updating of entities when editing a finalized form. However, since the implementation involves changes to the database structure, which can always be tricky, we need to test the upgrade process from an older version to the one introduced in this pull request.
#6593
This change also modifies the database structure, so the upgrade process should be tested as well - something that can conveniently be done in one go since both changes are included in the same pull request.
#6594
Here, it is important to make sure that users are always prompted to open the newest edit no mater how many of them are.
#6689
Here, we can just make sure the issue is no longer visible by reproducing it.
#6686
Please be careful when testing this, as there are many combinations to consider: both "Save as Draft" and "Finalize/Send" buttons visible or only one of them, editing finalized forms enabled or not... It can be confusing. Please make sure that all texts make sense.

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

#6688
Any form that registers/updates entities + is editable (client-editable="true"). I've added two such forms for testis: fcf6926.
#6593 #6594 #6689
Any form that is editable (client-editable="true").
#6686
Both forms that can't be edited after finalization and those that can be edited.

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

Yes: getodk/docs#1941

Before submitting this PR, please make sure you have:

added or modified tests for any new or changed behavior
run ./gradlew connectedAndroidTest (or ./gradlew testLab) and confirmed all checks still pass
added a comment above any new strings describing it for translators
added any new strings with date formatting to DateFormatsTest
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

...t_app/src/main/java/org/odk/collect/android/database/instances/InstanceDatabaseMigrator.java

seadowg

It would be good to add a test that checks that an exception is thrown if you try and save an Instance an editOf with a bogus ID to ensure the foreign key.

...t_app/src/main/java/org/odk/collect/android/database/instances/InstanceDatabaseMigrator.java

collect_app/src/test/java/org/odk/collect/android/database/DatabaseInstancesRepositoryTest.kt

seadowg

#6698 (comment)

seadowg

Loving all the new Kotlin here as well as the fact we're seeing some database integrity creeping into Collect!

seadowg · 2025-04-24T12:59:45Z

@grzesiek2010 I'll leave this unmerged for now seeing as it has migrations, and it'd be annoying to have to unwind them if QA finds something we didn't think of!

collect_app/src/main/java/org/odk/collect/android/instancemanagement/LocalInstancesUseCases.kt

collect_app/src/main/java/org/odk/collect/android/database/DatabaseObjectMapper.kt

...t_app/src/main/java/org/odk/collect/android/database/instances/InstanceDatabaseMigrator.java

..._app/src/main/java/org/odk/collect/android/formlists/savedformlist/SavedFormListViewModel.kt

collect_app/src/main/java/org/odk/collect/android/formmanagement/formmap/FormMapViewModel.kt

seadowg · 2025-04-27T13:02:54Z

collect_app/src/main/java/org/odk/collect/android/instancemanagement/InstanceExt.kt

 fun Instance.showAsEditable(settingsProvider: SettingsProvider): Boolean {
    return isDraft() && settingsProvider.getProtectedSettings()
        .getBoolean(ProtectedProjectKeys.KEY_EDIT_SAVED)
 }

+fun Instance.userVisibleInstanceName(


Agreed! I think it makes a lot more sense for this to be a "display" concern rather than data we store. I'm now wondering if we could also use this method to handle the case where there's no instance name and stop saving the form name instead?

seadowg · 2025-04-27T13:27:44Z

...src/androidTest/java/org/odk/collect/android/feature/instancemanagement/EditSavedFormTest.kt

@@ -114,7 +114,7 @@ class EditSavedFormTest {
            .editForm("One Question Editable")
            .clickOnQuestion("what is your age")
            .answerQuestion("what is your age", "456")
-            .swipeToEndScreen()
+            .swipeToEndScreen("One Question Editable (Edit 1)")


@alyblenkin so this PR also makes the form entry end screen show the "(Edit x)" suffix on the instance name. Do we want that or should that only show in form lists?

Sorry I missed this! We touched on this a little this morning, but I think if we can show the instance name that would be great.

collect_app/src/main/java/org/odk/collect/android/utilities/InstanceUploaderUtils.java

grzesiek2010 · 2025-04-29T00:06:27Z

@seadowg there are new changes for #6689 and #6686

seadowg · 2025-04-29T08:57:11Z

@seadowg there are new changes for #6689 and #6686

Got ya! Let's limit to this set of changes, so things don't get confusing. I think all the DB changes you had planned are no in, and I don't see why we can't merge while we're waiting for Central to fix the bug with updates as long as everything on our side looks good. I'm just going to update the PR title as well as the changes are all linked.

...t_app/src/main/java/org/odk/collect/android/database/instances/InstanceDatabaseMigrator.java

seadowg · 2025-04-30T09:58:00Z

collect_app/src/main/java/org/odk/collect/android/formhierarchy/FormHierarchyFragment.java

+    private void openEditedInstance(long dbId) {
+        Intent intent = FormFillingIntentFactory.editFinalizedFormIntent(requireContext(), currentProjectId, dbId);
+        startActivity(intent);
+        requireActivity().getOnBackPressedDispatcher().onBackPressed();


While we're here: should the set-up for the OnBackPressedCallback callback be in onAttach rather than onViewCreated?

seadowg · 2025-04-30T10:01:47Z

collect_app/src/main/java/org/odk/collect/android/formhierarchy/FormHierarchyFragment.java

@@ -106,7 +111,13 @@ public void onAttach(@NonNull Context context) {
        requireActivity().setTitle(formEntryViewModel.getFormController().getFormTitle());
        startIndex = formEntryViewModel.getFormController().getFormIndex();

-        MaterialProgressDialogFragment.showOn(this, formHierarchyViewModel.isCloning(), getParentFragmentManager(), () -> {
+        formHierarchyViewModel.getInstanceEditResult().observe(this, instanceEditResult -> {


I think there's still going to be a problem here if the user navigates away from Collect and back again while cloning and the Activity gets recreated (clearing view model state). I'm going to test that and update #6691 though, as I think the new cases protecting old edits will handle a lot of this.

seadowg · 2025-04-30T10:16:07Z

collect_app/src/main/java/org/odk/collect/android/formhierarchy/FormHierarchyFragment.java

+                Locale.getDefault()
+        ).format(instance.getLastStatusChangeDate());
+
+        new MaterialAlertDialogBuilder(requireContext())


It'd definitely help us down the line to convert FormHierarchyFragmentHostActivity to using a nav graph and then have the dialogs here just be DialogFragment implementations that are navigated to with the NavigationController. This is something we'll want to do anyway if we want form entry/hierarchy to be one Activity using a nav graph. Is that something you'd be up for trying out as a follow-up?

dbemke · 2025-05-05T12:59:40Z

What is the expected result in this case on Central (how many submissions: 2 submissions or 1 submission with edits)?

Send a editable form.
Go to "Sent" and start editing the form.
Discard the changes.
Go to "Ready to send" and finalize all drafts.
Go to Central and check the submissions (example submission 4, 5 in https://staging.getodk.cloud/projects/152/forms/all-widgets/submissions)

grzesiek2010 · 2025-05-05T13:14:29Z

What is the expected result in this case on Central (how many submissions: 2 submissions or 1 submission with edits)?

There should be 1 submission with one edit. Once you click the edit icon the edit is created so even if you discard changes it is still there. When all drafts are finalized and sent it should be there.

dbemke · 2025-05-06T12:40:26Z

There should be 1 submission with one edit. Once you click the edit icon the edit is created so even if you discard changes it is still there. When all drafts are finalized and sent it should be there.

To summarize our call and discussion on this issue and not to lose information (while I'm testing Central) in this case there are 2 separate submission.

grzesiek2010 · 2025-05-06T12:41:39Z

To summarize our call and discussion on this issue and not to lose information (while I'm testing Central) in this case there are 2 separate submission.

I'm going to try to reproduce this and understand what happens.

dbemke · 2025-05-06T13:40:30Z

I'm going to try to reproduce this and understand what happens.

I just tried a few times and it seems that bulk finalizing might cause 2 submissions- if I open a draft (discarded from edit finalized) and finalize then I get 1 submission as expected.

dbemke · 2025-05-07T06:59:49Z

Crash after trying to delete a form which has edits and is with "sending failed" status. The issue doesn't occur on the master version. The issue also doesn't occur in forms which aren't editable.

Steps to reproduce:

Download a project with a form which is editable.
Send a submission.
Go to "Sent" and edit the form.
Go to "Ready to send" , tap the three dots, "Change View" and select "Show Sent and Unsent Forms".
Select the first version of the form (not the edit) and try to send it (it gets "sending failed" status).
Go to "Delete form" and try to delete the form with "sending failed" status.

dbemke · 2025-05-07T07:23:18Z

The same crash mentioned above occurs in this scenario:

Download a project with an editable form.
Set auto send to "off".
Finalize a form.
Go to "Ready to send" and edit the form and finalize it.
Go to "Delete form" and select both forms and try to delete them.

In this case I haven't noticed the issue after sending a form (not finalizing).

seadowg · 2025-05-07T15:07:03Z

@dbemke both these cases should be fixed by #6703. I'd expect both cases where you delete a form that has edits to cause crashes at the moment.

grzesiek2010 · 2025-05-08T13:54:04Z

I just tried a few times and it seems that bulk finalizing might cause 2 submissions- if I open a draft (discarded from edit finalized) and finalize then I get 1 submission as expected.

It removes deprecatedId, which indicates it is an edit. The same happens on master, so it shouldn't block you. I'm going to file a separate issue and fix it.

seadowg · 2025-05-08T13:56:38Z

It removes deprecatedId, which indicates it is an edit. The same happens on master, so it shouldn't block you. I'm going to file a separate issue and fix it.

Should this case maybe get added to #6691, or is it a different problem?

grzesiek2010 · 2025-05-08T22:46:22Z

Should this case maybe get added to #6691, or is it a different problem?

I think it's a different one, so I filed a new issue. But the fix might be common for both, we will see.

grzesiek2010 added 7 commits April 22, 2025 18:59

Introduced a new column to track edits

3b35c33

Improved tests

667e859

Populate the new editOf column when editing a form

f307d04

Improved tests

e706fc0

Do not create/update entities when an edit is finalized 8000

0401193

Added new tests

fcf6926

Fixed test

52933a1

grzesiek2010 marked this pull request as ready for review April 22, 2025 21:05

grzesiek2010 requested a review from seadowg April 22, 2025 21:05

seadowg requested changes Apr 23, 2025

View reviewed changes

...t_app/src/main/java/org/odk/collect/android/database/instances/InstanceDatabaseMigrator.java Outdated Show resolved Hide resolved

Set FOREIGN KEY

ecdf646

grzesiek2010 force-pushed the COLLECT-6688 branch from 4f8fd41 to ecdf646 Compare April 23, 2025 15:16

grzesiek2010 requested a review from seadowg April 23, 2025 15:41

seadowg requested changes Apr 23, 2025

View reviewed changes

...t_app/src/main/java/org/odk/collect/android/database/instances/InstanceDatabaseMigrator.java Outdated Show resolved Hide resolved

grzesiek2010 added 3 commits April 23, 2025 21:26

Converted DatabaseInstancesRepositoryTest to kotlin

abb78b5

Converted InMemInstancesRepositoryTest to kotlin

3bd0c37

Improved the instances table definition and added tests

a821695

grzesiek2010 requested a review from seadowg April 23, 2025 20:08

seadowg requested changes Apr 24, 2025

View reviewed changes

grzesiek2010 added 2 commits April 24, 2025 12:38

Converted InstancesRepositoryTest to kotlin

9f99338

Improved tests

1cb6bac

grzesiek2010 requested a review from seadowg April 24, 2025 11:36

seadowg requested changes Apr 24, 2025

View reviewed changes

Simplified updating the instances table definition

c2e6295

grzesiek2010 requested a review from seadowg April 24, 2025 12:55

seadowg approved these changes Apr 24, 2025

View reviewed changes

seadowg added the needs testing label Apr 24, 2025

grzesiek2010 added 2 commits April 25, 2025 00:00

Added a new column to track edit number

ccb92ac

Save edit number on editing

e173082

Improved tests

e2f0f9d

grzesiek2010 changed the title ~~Don't create/update entities when editing a finalized form + Highlight edited submissions~~ Don't create/update entities when editing a finalized form + Highlight edited submissions + Block editing out of date submissions Apr 26, 2025

seadowg requested changes Apr 27, 2025

View reviewed changes

grzesiek2010 added 2 commits April 28, 2025 13:38

Simplified saving 'editOf' and 'editNumber'

9565600

Added missing tests

27a46f0

grzesiek2010 force-pushed the COLLECT-6688 branch from 156728d to 27a46f0 Compare April 28, 2025 13:18

Make sure both editOf and editNumber ar null or not null

89dd1a6

grzesiek2010 force-pushed the COLLECT-6688 branch from 8fe56e9 to 89dd1a6 Compare April 28, 2025 13:53

grzesiek2010 added 3 commits April 28, 2025 20:45

Fixed observing editing

202152a

Updated language

17e7103

Update icon according to the state

9e5e164

grzesiek2010 requested a review from seadowg April 29, 2025 00:11

grzesiek2010 commented Apr 29, 2025

View reviewed changes

...t_app/src/main/java/org/odk/collect/android/database/instances/InstanceDatabaseMigrator.java Show resolved Hide resolved

seadowg approved these changes Apr 30, 2025

View reviewed changes

seadowg added the needs testing label Apr 30, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve editing finalized forms #6698

Improve editing finalized forms #6698

Improve editing finalized forms #6698

Are you sure you want to change the base?

Improve editing finalized forms #6698

Conversation

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

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?

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

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:

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment