Remembering previously entered values (auto-fill) #2882

cooperka · 2019-02-15T18:08:14Z

~~Depends on getodk/javarosa#406~~ (merged and snapshotted)

Allows form designers to refer to the last-saved instance of this particular form in order to retrieve answers from it. Currently intended for text-based answers rather than media. Simply add an external instance to your form:

<instance id="last-saved" src="jr://instance/last-saved"/>

The instance id can be anything you want.

Then you can access the external data as usual, for example:

<setvalue event="xforms-ready" ref="/data/foo" value="instance('last-saved')/data/foo"/>

⚠️ xforms-ready is being deprecated, see here for more info. TL;DR: Use odk-model-first-load now instead.

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

Extensive testing with modified All Widgets.xml: filling blank forms, saving forms, filling new blank forms, finalizing, exiting without saving, etc.

Addtional testing with encryption to verify last-saved.xml is deleted when the form is finalized. Valid sample encryption key you can test with:

<submission base64RsaPublicKey="MIGfMA0GCSqGSIb3DQEBAQUAA4GNADCBiQKBgQCnTXfMaLqcrbxgCA3T2bg4RcugjW8EjuyLttzIGsqX1wcazHLMwW8iGGREfMIM22M1MiRE5+1EygNG+rz2MR1o1YjPyULgzi38KFyLkPJ96lq23kCMzh1wPrXRE7Hhu3C/eojc33RNtT2g9xpwPRAaY1v+X+Fypsq4GckMKNLfrQIDAQAB"/>

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

See extensive discussion at getodk/roadmap#30. The result of the discussion there was this approach. It makes use of existing logic to accomplish this feature without needing too much new code.

Collect caches the last-saved instance to the form's media directory formName-media/last-saved.xml on each save. This file will stay around even if the original saved instance is lost, which is probably what users would expect. The form's media directory is deleted when the blank form is deleted from the device, which is also probably what users would expect.

When parsing new FormDefs, lastSavedSrc will be passed to JavaRosa as either "jr://file/last-saved.xml" or null if no file exists because the form has never been saved. A null value means no instance src is provided, meaning the instance will be treated as internal instead of external. The internal <instance id="last-saved"/> has no children and thus will provide empty values for anything referring to it.

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?

Form designers will be able to designate questions as "auto-filled" based on the last-saved value for that particular question. I don't anticipate much regression risk here since the old logic isn't significantly modified.

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

Linked above.

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

Yes [TODO file issue].

It will also require an update to ODK Build if they want to support this [TODO file issue].

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

ggalmazor

Everything is looking great. I have some comments about the general design of the changes this PR introduces, but nothing that should block the PR, IMO.

Before merging, I think other users with more experience with the codebase should review the PR as well.

collect_app/src/main/java/org/odk/collect/android/tasks/FormLoaderTask.java

collect_app/src/main/java/org/odk/collect/android/provider/FormsProvider.java

cooperka · 2019-02-21T04:24:15Z

@opendatakit-bot label "needs review"

collect_app/src/main/java/org/odk/collect/android/tasks/SaveToDiskTask.java

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

collect_app/src/main/java/org/odk/collect/android/upload/InstanceGoogleSheetsUploader.java

zestyping · 2019-03-06T01:46:13Z

Nice and simple solution! LGTM!

yanokwa · 2019-03-11T17:21:50Z

@grzesiek2010 I think this is probably ready for testing. Can you do a review and tag it for testing when you're satisfied?

yanokwa · 2019-03-12T14:03:50Z

@cooperka Can you remove the empty commit so we have a cleaner commit history? You should be able to do the rebuild from your CircleCI account.

grzesiek2010 · 2019-03-14T20:16:21Z

I tried the form you attached and got this:

E/SaveToDiskTask: java.io.FileNotFoundException: /storage/emulated/0/odk/forms/autoFillTest-media/last-saved.xml: open failed: ENOENT (No such file or directory)
        at libcore.io.IoBridge.open(IoBridge.java:456)
        at java.io.RandomAccessFile.<init>(RandomAccessFile.java:117)
        at org.odk.collect.android.tasks.SaveToDiskTask.writeFile(SaveToDiskTask.java:409)
        at org.odk.collect.android.tasks.SaveToDiskTask.exportData(SaveToDiskTask.java:278)
        at org.odk.collect.android.tasks.SaveToDiskTask.doInBackground(SaveToDiskTask.java:126)
        at org.odk.collect.android.tasks.SaveToDiskTask.doInBackground(SaveToDiskTask.java:53)
        at android.os.AsyncTask$2.call(AsyncTask.java:288)
        at java.util.concurrent.FutureTask.run(FutureTask.java:237)
        at android.os.AsyncTask$SerialExecutor$1.run(AsyncTask.java:231)

it's because I just copied the xml file not creating -media dir. I know that if we download a file from aggregate a dir is created automatically but this case should be handled better. Our users can create their own servers which don't do that.

cooperka · 2019-03-18T21:59:35Z

@grzesiek2010 I added a generic fix in writeFile to make sure the path exists. Please tag for testing if you think it's ready.

cooperka · 2019-03-25T14:51:29Z

@grzesiek2010 fixed. The file will always exist now before the form is parsed, even if it's later deleted.

@opendatakit-bot label "needs review"

lognaturel · 2019-03-25T16:02:06Z

I believe this means that a plain-text copy of the latest filled instance will always be available, even in the case of an encrypted form, is that right? If so, perhaps the caching shouldn't happen in the case of an encrypted form?

I only took a quick look at the JR pull request and didn't immediately see whether the last saved XML is always parsed or only if the blank form queries it (as discussed in the spec thread). Either way, I think it would be good to have a record of a performance check with a big form and big filled instance.

cooperka · 2019-03-25T17:42:17Z

Good catch @lognaturel, last-saved instances should definitely be disabled (or encrypted) for encrypted forms. I think I'll go the easy route of disabling it unless you think it would be trivial to support encrypted last-saved instances (I haven't dealt with ODK encryption before).

The last-saved instance form-media/last-saved.xml is always created on save but should only be parsed if explicitly referenced. I'll verify to make sure, and benchmark it.

lognaturel · 2019-03-25T19:24:01Z

you think it would be trivial to support encrypted last-saved instances

The idea with ODK encryption is that access to the device can't compromise access to the data at all. In practice, that means asymmetric encryption with the private key never getting close to the device. So certainly that scheme wouldn't allow for querying the last-saved instance. I think disabling for encrypted form sounds great.

It will always be deleted on save if finalized, along with the rest of the plaintext files. Because of this, InstanceSyncTask doesn't need to delete it (nor does it even know of its existence because it's in a separate directory) so it just passes in `null`.

cooperka · 2019-03-25T22:58:45Z

@lognaturel

Encryption logic: updated
Parse logic: it only parses last-saved if something refers to it in the form ✅ (specifically: if (rawSrcLower.equals("jr://instance/last-saved")))
Benchmark for XFormUtils.getFormFromInputStream:

	WITHOUT last-saved	WITH last-saved
Trial 1 (ms)	627	621
Trial 2 (ms)	631	577
Trial 3 (ms)	623	633
Trial 4 (ms)	580	657
Trial 5 (ms)	559	652
Avg	604	628

I modified this XLS to have over 1000 unique text fields, all with different default values. Here's the final XML with the last-saved instance. It's admittedly not perfect science but it seems pretty clear that the extra instance doesn't significantly affect the total parse time.

I executed adb shell rm -rf /storage/emulated/0/odk/.cache/*.formdef in between each open to get the numbers.

For what it's worth, the main appearance_widgets.xml is 260K and its associated last-saved.xml is only 16K according to ls -lh.

grzesiek2010 · 2019-03-26T14:19:13Z

I think that the solution for encrypted forms with removing last-saved.xml file every time is not good.
The form is removed once you mark one filled form finalized.
Let's imagine a case when someone has an encrypted form and fills one instance without saving as finalized - last-saved.xml file is created and not removed. Then he fills another instance and saves as finalized - last-saved.xml file is removed. But someone may want to keep one not finalized (encrypted) form as a template (with answers) for other instances.
So maybe we should keep last-saved.xml file but not override it when a form is finalized?

cooperka · 2019-03-26T14:31:49Z

I agree that it's not 100% ideal for encrypted forms, but I can't think of a better alternative that's simple enough to be worth implementing, verifying, and maintaining. I personally favor security over ease of use in this case.

As described on the XLSForm spec:

Encryption-enabled forms provide a mechanism to keep finalized records private at all times. This includes the time after a record is marked as final that it is stored on the device ...

I think if someone wants to use last-saved on an encrypted form, they're welcome to, but they should simply do so before finalizing anything. If you have a simple proposal that would keep things secure while also allowing for more intuitive last-saved behavior feel free to suggest!

grzesiek2010 · 2019-03-26T14:54:45Z

Yes, it would be difficult to handle this case. Ok so maybe let's leave it as is but this case should be described in the doc. Please keep it in mind creating an issue.

grzesiek2010 · 2019-03-26T14:56:15Z

@lognaturel if you can't see anything else we should improve please add needs testing label.

kkrawczyk123 · 2019-03-27T16:16:31Z

@cooperka, I do not completely understand why the form which I could use for testing has been removed and form which I cannot use was attached in the comment. Not very tester friendy :( It took me a long long time to change All widgets form so I could use it for testing.
In general, saving data in xml and retrieving them works very good for text, numeric, range, date, geo, single/multiple select widgets. I've checked the behavior for encrypted form too.
I have noticed some differences in the behavior of media widgets like an image, draw, signature - media visible in hierarchy view and not visible on the widget. On the Annotate widget markup button was active, white space visible when clicking on it, on video widget and selfie video play button was active and black space visible when using it, play mode on audio widget unable to use it. File widget did not save anything at all. I am writing because those cases were probably not considered so far, so those behaviors could be unified/improved in the future and maybe media stored somehow in the media folder? I think that those informations might be valuable for @cooperka and other devs.
The form that I used:
All widgets_save.xml.txt
so we have success in here!
@opendatakit-bot unlabel "needs testing"
@opendatakit-bot label "behavior verified"

grzesiek2010 · 2019-03-27T16:23:21Z

@kkrawczyk123 thanks!
I think we can merge and file a separate issue for media widgets. Do you agree @lognaturel?

cooperka · 2019-03-27T16:35:06Z

Sorry about the form being removed @kkrawczyk123, looks like GitHub may have deleted the file because it was uploaded to my fork. Thank you for uploading your own.

This feature is mainly intended for text-based widgets, though as you mentioned some other widgets also work. I'll make this clear in the upcoming documentation. Thanks!

lognaturel · 2019-03-27T16:40:34Z

Merge away, @grzesiek2010!

So sorry you rebuilt the form, @kkrawczyk123! If that happens again, do ask in Slack or a comment to see if someone has a copy!

cooperka mentioned this pull request Feb 15, 2019

Remembering previously entered values (auto-fill) getodk/javarosa#406

Merged

ggalmazor approved these changes Feb 18, 2019

View reviewed changes

yanokwa added this to the v1.21 milestone Feb 20, 2019

getodk-bot added the needs review label Feb 21, 2019

yanokwa requested a review from zestyping March 5, 2019 00:05

zestyping reviewed Mar 6, 2019

View reviewed changes

collect_app/src/main/java/org/odk/collect/android/tasks/SaveToDiskTask.java Show resolved Hide resolved

zestyping reviewed Mar 6, 2019

View reviewed changes

collect_app/src/main/java/org/odk/collect/android/utilities/FileUtils.java Show resolved Hide resolved

zestyping reviewed Mar 6, 2019

View reviewed changes

collect_app/src/main/java/org/odk/collect/android/upload/InstanceGoogleSheetsUploader.java Outdated Show resolved Hide resolved

yanokwa requested a review from grzesiek2010 March 11, 2019 17:17

cooperka force-pushed the autofill branch from b51c2c0 to e6e7cc1 Compare March 12, 2019 14:56

cooperka added 12 commits March 14, 2019 12:39

Pass last-saved instance into JR from FormLoaderTask

e3bdaa1

Refactor a few related FileUtils

27e7b58

Pass last-saved src from InstanceGoogleSheetsUploader also

7f863f8

Update last-saved instance on save

e13eb3c

Move LAST_SAVED_FILENAME to FileUtils

a395953

Make form MEDIA_SUFFIX a constant

cc6806c

Factor out getLastSavedSrcIfExists util

c48b7cb

Rename old occurrences of form fileName => basename

c0eb4f1

Update one other occurrence of -media string

57f3f27

Make MEDIA_SUFFIX private to FormUtils

85258e8

Make LAST_SAVED_PATH private to FormUtils

9359920

Remove temporary variable

ec10c86

cooperka force-pushed the autofill branch from e6e7cc1 to ec10c86 Compare March 14, 2019 16:40

Make sure dirs exist before trying to write

d457b0b

grzesiek2010 removed the needs review label Mar 25, 2019

getodk-bot added the needs review label Mar 25, 2019

Always create last-saved.xml if it doesn't exist

bb19e4a

cooperka force-pushed the autofill branch from 7cd592e to bb19e4a Compare March 25, 2019 14:53

grzesiek2010 approved these changes Mar 26, 2019

View reviewed changes

lognaturel added the needs testing label Mar 26, 2019

grzesiek2010 removed the needs review label Mar 26, 2019

getodk-bot added behavior verified and removed needs testing labels Mar 27, 2019

grzesiek2010 merged commit ab0e320 into getodk:master Mar 27, 2019

cooperka deleted the autofill branch March 27, 2019 17:22

grzesiek2010 mentioned this pull request Mar 28, 2019

Fixed the problem with parsing instances getodk/javarosa#416

Merged

cooperka mentioned this pull request Apr 2, 2019

Document last-saved secondary instances (for auto-fill) getodk/xforms-spec#228

Merged

This was referenced Sep 27, 2019

Remembering previously entered values doesn't work #3363

Closed

Fixed forms with external instances getodk/javarosa#497

Merged

grzesiek2010 mentioned this pull request Nov 21, 2019

View last submitted data at the time of filling up the form #389

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remembering previously entered values (auto-fill) #2882

Remembering previously entered values (auto-fill) #2882

cooperka commented Feb 15, 2019 •

edited

Loading

ggalmazor left a comment

cooperka commented Feb 21, 2019

zestyping commented Mar 6, 2019

yanokwa commented Mar 11, 2019

yanokwa commented Mar 12, 2019

grzesiek2010 commented Mar 14, 2019 •

edited

Loading

cooperka commented Mar 18, 2019

cooperka commented Mar 25, 2019

lognaturel commented Mar 25, 2019

cooperka commented Mar 25, 2019

lognaturel commented Mar 25, 2019

cooperka commented Mar 25, 2019 •

edited

Loading

grzesiek2010 commented Mar 26, 2019 •

edited

Loading

cooperka commented Mar 26, 2019 •

edited

Loading

grzesiek2010 commented Mar 26, 2019

grzesiek2010 commented Mar 26, 2019

kkrawczyk123 commented Mar 27, 2019

grzesiek2010 commented Mar 27, 2019

cooperka commented Mar 27, 2019

lognaturel commented Mar 27, 2019 •

edited

Loading

Remembering previously entered values (auto-fill) #2882

Remembering previously entered values (auto-fill) #2882

Conversation

cooperka commented Feb 15, 2019 • edited Loading

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

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:

ggalmazor left a comment

Choose a reason for hiding this comment

cooperka commented Feb 21, 2019

zestyping commented Mar 6, 2019

yanokwa commented Mar 11, 2019

yanokwa commented Mar 12, 2019

grzesiek2010 commented Mar 14, 2019 • edited Loading

cooperka commented Mar 18, 2019

cooperka commented Mar 25, 2019

lognaturel commented Mar 25, 2019

cooperka commented Mar 25, 2019

lognaturel commented Mar 25, 2019

cooperka commented Mar 25, 2019 • edited Loading

grzesiek2010 commented Mar 26, 2019 • edited Loading

cooperka commented Mar 26, 2019 • edited Loading

grzesiek2010 commented Mar 26, 2019

grzesiek2010 commented Mar 26, 2019

kkrawczyk123 commented Mar 27, 2019

grzesiek2010 commented Mar 27, 2019

cooperka commented Mar 27, 2019

lognaturel commented Mar 27, 2019 • edited Loading

cooperka commented Feb 15, 2019 •

edited

Loading

grzesiek2010 commented Mar 14, 2019 •

edited

Loading

cooperka commented Mar 25, 2019 •

edited

Loading

grzesiek2010 commented Mar 26, 2019 •

edited

Loading

cooperka commented Mar 26, 2019 •

edited

Loading

lognaturel commented Mar 27, 2019 •

edited

Loading