Skip to content
This repository has been archived by the owner on Nov 29, 2023. It is now read-only.

Remove online form cache #293

Merged
merged 7 commits into from Sep 10, 2021

Conversation

eyelidlessness
Copy link
Contributor

@eyelidlessness eyelidlessness commented Jul 31, 2021

This change addresses problems introduced in #269:

@@ -200,6 +200,7 @@ function _init( formParts ) {
modelStr: formParts.model,
instanceStr: _prepareInstance( formParts.model, settings.defaults ),
external: formParts.externalData,
survey: formParts,
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

While still untested, the changes in this file restore almost exactly the original behavior prior to #269, the only difference is store.init does still need to be called to support last-saved functionality. The store is otherwise not in use in this code path, but it's worth calling out.

Comment on lines +121 to +127
const uploadQueuedRecord = _uploadRecord;

const uploadRecord = ( survey, record ) => (
setLastSavedRecord( survey, record )
.then( () => _uploadRecord( record ) )
);

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Separating these functions wasn't a strictly necessary change, but uploadRecord's signature has changed to require a survey to support last-saved functionality while online, and it felt like this separation was better than requiring that for offline mode's upload queue where last-saved should not be used anyway. An alternative approach would have been to call setLastSavedRecord in controller-webform.js, but that would have required backfilling a much more elaborate set of tests for that module.

@@ -358,17 +365,11 @@ function getFormParts( props ) {
/** @type {Survey} */
let survey;

/** @type {XMLDocument} */
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The changes below remove last-saved implementation details, which are now implemented in the new last-saved.js module instead.

@@ -14,6 +14,7 @@ import records from './records-queue';
import $ from 'jquery';
import encryptor from './encryptor';
import formCache from './form-cache';
import { getLastSavedRecord, populateLastSavedInstances } from './last-saved';
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tried to keep changes here as minimal as possible, as this module is still untested. The changes here are to support:

  • The signature change to uploadRecord
  • Continuing to ensure last-saved instances are populated when the form is reset after submitting/queueing/saving a draft record

@@ -7,7 +7,10 @@ import events from './event';
import settings from './settings';
import connection from './connection';
import assign from 'lodash/assign';
import encryptor from './encryptor';
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As with many of the changes in connection.js, the changes here remove last-saved implementation details, which are now implemented in last-saved.js.

@@ -0,0 +1,152 @@
import encryptor from './encryptor';
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All of the last-saved implementation now lives here. These changes should mostly look familiar, as it's very similar to the previous implementation. The primary differences are:

  • Last-saved records are no longer stored in surveys, they're written to a new lastSavedRecords store
  • Some code style changes:
    • Named exports declared at the definition site; this improves the ability to follow references through the project, as well as makes it clearer at definition site whether a function is intended to be used outside the module
    • Arrow functions (they're just more idiomatic)
  • Not technically a change, but calling out that if a survey is already cached for offline mode, online mode will still update that cache's last-saved externalData instances when a survey changes. This ensures caches do not store stale last-saved data, particularly in cases where a last-saved instance may have been removed from the form.

@@ -12,6 +12,7 @@ import exporter from './exporter';
import { t } from './translator';
import $ from 'jquery';
import formCache from './form-cache';
import { setLastSavedRecord } from './last-saved';
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The changes here account for the API changes in connection.js.

@@ -945,5 +963,6 @@ export default {
dynamicData: dataStore,
record: recordStore,
flush,
dump
dump,
get lastSavedRecords() { return server.lastSavedRecords; },
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tried to keep the changes to store.js to a minimum, keeping as much last-saved logic in last-saved.js as possible. These minor changes were necessary because the store's schema all has to be defined at initialization time. This breaks a bit with the pattern for other stores, as last-saved.js references the db.js interfaces directly rather than a wrapper around them.

) )
)
),
],
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This change, as well as the one in build.js, addresses another media regression (and potentially several other regressions 😬), this time caused by #289:

  1. enketo-core defines and implements a set of aliased modules whose functionality it depends on
  2. enketo-express also defines those aliases, but overrides those implementations.
  3. When we switched to esbuild, it failed to account for those overrides, causing the enketo-express build to use enketo-core's behavior for those modules.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should consider consolidating relevant portions of these configs, so the two builds don't diverge in the future.

} )
.then( done, done );
} );
} );
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These tests are no longer relevant, because none of the modes access the form cache anymore.

@MartijnR
Copy link
Member

Hi @eyelidlessness,

I had a quick glance and especially at the PR comments you made. Looks good. There is just a merge conflict atm.

I'd like to do some real-life testing in the next few days.

I committed these by mistake in a rush to get the draft PR open
Also brings in the remaining relevant update test from enketo#291
This resolves issues where media was being removed from a form when saving a record in offline mode
- Don't error when submitting an edit
- Don't set/update a last-saved record when submitting an edit
@eyelidlessness
Copy link
Contributor Author

@MartijnR sorry it took a bit to resolve conflicts. Rebase got pretty confusing 😬 but should be good to go now!

@MartijnR MartijnR self-requested a review August 18, 2021 18:31
Copy link
Member

@MartijnR MartijnR left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks. Code looks good to me and I did some real-life testing with regular forms and forms that use the last-saved feature. Online-only views no longer cache the form itself. Database structure update seems to work as desired.

@MartijnR
Copy link
Member

Since some other related issues need fixing before releasing a new version, I will just go ahead and merge.

@MartijnR
Copy link
Member

In a short while if there are no objections raised....

@lognaturel
Copy link
Contributor

We’ve got some quality assurance planned in the next few days but if you’re satisfied with your checks a release would be most appreciated!

@MartijnR
Copy link
Member

Oh, that would be great. Now I am tempted to just wait... :)

@lognaturel
Copy link
Contributor

That sounds fine to me! @eyelidlessness would waiting a few days make more conflicts accumulate? @getodk/testers are back from vacation on Tues of next week.

@eyelidlessness
Copy link
Contributor Author

I hope not but I don't mind dealing with them if they do, if holding off is preferred!

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
4 participants