Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remembering previously entered values (auto-fill) #2882

Merged
merged 16 commits into from Mar 27, 2019

Conversation

Projects
None yet
8 participants
@cooperka
Copy link
Collaborator

commented Feb 15, 2019

Closes opendatakit/roadmap#30

Depends on opendatakit/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 opendatakit/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
Copy link
Member

left a comment

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.

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

@cooperka

This comment has been minimized.

Copy link
Collaborator Author

commented Feb 21, 2019

@opendatakit-bot label "needs review"

@yanokwa yanokwa requested a review from zestyping Mar 5, 2019

@zestyping

This comment has been minimized.

Copy link
Collaborator

commented Mar 6, 2019

Nice and simple solution! LGTM!

@yanokwa yanokwa requested a review from grzesiek2010 Mar 11, 2019

@yanokwa

This comment has been minimized.

Copy link
Member

commented Mar 11, 2019

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

@yanokwa

This comment has been minimized.

Copy link
Member

commented Mar 12, 2019

@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.

@cooperka cooperka force-pushed the cooperka:autofill branch from b51c2c0 to e6e7cc1 Mar 12, 2019

@cooperka cooperka force-pushed the cooperka:autofill branch from e6e7cc1 to ec10c86 Mar 14, 2019

@grzesiek2010

This comment has been minimized.

Copy link
Member

commented Mar 14, 2019

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

This comment has been minimized.

Copy link
Collaborator Author

commented Mar 18, 2019

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

@grzesiek2010

This comment has been minimized.

Copy link
Member

commented Mar 22, 2019

There are two problems with this solution:

  1. If someone opens the attached form in v1.20.0 (and older versions) and then upgrade to a version with this fix, it won't work because it's already cached (without the info about last saved instance).
    I know it's rather a rare case so probably we can leave it as is if it's a big problem to fix.

  2. If you use the attached form, then remove last-saved file from media dir manually it won't work anymore. It takes place because if we remove that file a form is cached again. A user would need to clear cache.
    I think this case should be handled.

Merge branch 'master' into autofill
Conflicts:
	collect_app/src/main/java/org/odk/collect/android/logic/FormController.java
	collect_app/src/main/java/org/odk/collect/android/upload/InstanceGoogleSheetsUploader.java
@cooperka

This comment has been minimized.

Copy link
Collaborator Author

commented Mar 25, 2019

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

@opendatakit-bot label "needs review"

@cooperka cooperka force-pushed the cooperka:autofill branch from 7cd592e to bb19e4a Mar 25, 2019

@lognaturel

This comment has been minimized.

Copy link
Member

commented Mar 25, 2019

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

This comment has been minimized.

Copy link
Collaborator Author

commented Mar 25, 2019

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

This comment has been minimized.

Copy link
Member

commented Mar 25, 2019

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.

Ensure last-saved.xml is deleted for encrypted forms
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

This comment has been minimized.

Copy link
Collaborator Author

commented Mar 25, 2019

@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

This comment has been minimized.

Copy link
Member

commented Mar 26, 2019

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

This comment has been minimized.

Copy link
Collaborator Author

commented Mar 26, 2019

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

This comment has been minimized.

Copy link
Member

commented Mar 26, 2019

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

This comment has been minimized.

Copy link
Member

commented Mar 26, 2019

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

@kkrawczyk123

This comment has been minimized.

Copy link

commented Mar 27, 2019

@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

This comment has been minimized.

Copy link
Member

commented Mar 27, 2019

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

@cooperka

This comment has been minimized.

Copy link
Collaborator Author

commented Mar 27, 2019

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

This comment has been minimized.

Copy link
Member

commented Mar 27, 2019

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!

@grzesiek2010 grzesiek2010 merged commit ab0e320 into opendatakit:master Mar 27, 2019

2 checks passed

ci/circleci: build_debug Your tests passed on CircleCI!
Details
ci/circleci: test_unit Your tests passed on CircleCI!
Details

@cooperka cooperka deleted the cooperka:autofill branch Mar 27, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.