fix:defaults in the url query string will create deprecatedID for new submissions #321
Conversation
|
Thanks for submitting this PR @punkch! I'll take a look. |
There was a problem hiding this comment.
I took some time to track the current behavior, which I'll describe here for posterity/future recall:
- When defaults are present,
enketo-webform.jspasses aninstanceStrwith a representation of the form's model, with those defaults populated. - Before line 63,
data.submittedis undefined. - The check prior to this change only assigns
data.submitted = falsewhen there is a draft record being filled. - When initializing a form,
enketo-core'sFormModelassignsdata.submitted = truewhen it's undefined. - When submitting a record,
FormModelthen checks for bothinstanceStrandsubmitted; if both are truthy, it creates adeprecatedID.
I have validated that this does fix the issue, by ensuring data.submitted = false regardless of whether defaults are present. Thanks @punkch!
|
This breaks edits. I think everyone assumed that somebody else had tried edits and verified they still worked. I made the mistake of saying this felt safe to me based on secondhand analysis rather than looking at it myself. I think it makes sense to spend a little bit to see whether there's actually a low-hanging fix but pretty quickly we should just revert and do another point release. |
|
Looking at it again, it really is surprising that record is set for offline draft edits but not for edits from an edit url. No wonder we all thought this was safe. I’ve reviewed the modes/URLs from https://apidocs.enketo.org/v2 carefully and feel confident that a deprecatedId should should only be generated when an instance was provided for edit. Maybe approaching from that angle is more promising. |
This was a regression in enketo#321. `record` here is populated by `_checkAutoSavedRecord`, which is only available in offline mode. Since that will always be falsey (undefined) in edit mode, `submitted` would be set to false. Since we should only set `deprecatedID` when editing submissions, it made more sense to assign it directly based on the `isEditing` property introduced in enketo#269 (which should also have been added to the JSDoc type at that time).
|
The line being modified here was introduced to make sure that a deprecatedId was not generated for a draft edit. It previously checked for the existence of a record and the record being a draft. Was the draft check necessary? Is there ever a case in which a record is populated and it’s not a draft? I’m asking in the context of thinking about risks around #345 |
This was a regression in #321. `record` here is populated by `_checkAutoSavedRecord`, which is only available in offline mode. Since that will always be falsey (undefined) in edit mode, `submitted` would be set to false. Since we should only set `deprecatedID` when editing submissions, it made more sense to assign it directly based on the `isEditing` property introduced in #269 (which should also have been added to the JSDoc type at that time).
Looking back at the history, it was introduced in 853fb44 to fix enketo/enketo#1137, and as mentioned in my review here/notes in #345, when
When submitting a record in offline mode, without first saving a draft. Technically it is submitted from the auto-saved record, but that should be considered an implementation detail. |
Oh! Surprising to me. Makes #345 even more appealing so we don’t have to reason around when exactly record is set. |
This was a regression in enketo#321. `record` here is populated by `_checkAutoSavedRecord`, which is only available in offline mode. Since that will always be falsey (undefined) in edit mode, `submitted` would be set to false. Since we should only set `deprecatedID` when editing submissions, it made more sense to assign it directly based on the `isEditing` property introduced in enketo#269 (which should also have been added to the JSDoc type at that time).
This seems to fix enketo/enketo#1052 though I don't think it addresses the root cause.