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

instance destination doesn't do anything #66

Closed
getodk-bot opened this issue Dec 28, 2016 · 14 comments
Closed

instance destination doesn't do anything #66

getodk-bot opened this issue Dec 28, 2016 · 14 comments
Assignees
Milestone

Comments

@getodk-bot
Copy link
Member

Issue by mitchellsundt
Thursday Jul 09, 2015 at 18:08 GMT
Originally opened as getodk/getodk#288 (1 comment(s))


Originally reported on Google Code with ID 287

instance destination seems to bind to the prompt of whatever thing you type there. if
that thing doesn't exist, it doesn't create it. this usually results in a broken form.

more broadly, it'd be great to understand what it's supposed to do.

Reported by yanokwa on 2011-08-03 22:26:52

@getodk-bot
Copy link
Member Author

Comment by mitchellsundt
Thursday Jul 09, 2015 at 18:08 GMT


Reported by yanokwa on 2012-05-25 20:10:52

@issa-tseng
Copy link
Member

Yeah, instance destination should probably create the requested custom destination node in the instance tag rather than the default. Someone looking to fix this would do well to search for instance.children.push within public/javascripts/data.js.

@issa-tseng issa-tseng self-assigned this May 2, 2017
@issa-tseng issa-tseng modified the milestone: 0.3.1 May 2, 2017
@issa-tseng
Copy link
Member

My test case file

issa-tseng added a commit to issa-tseng/odkbuild that referenced this issue May 10, 2017
* it still doesn’t support `..`, but it does support nested paths.
* resolves getodk#66.
@yanokwa
Copy link
Member

yanokwa commented May 18, 2017

I know I filled this issue, but I have no idea what someone would use instance destination for. I think this might have been used as a hack to rename the data name? Any ideas why we had this in the first place, @clint-tseng?

@issa-tseng
Copy link
Member

shrug. Seemed like elementary XForms functionality, so this feature has existed, albeit somewhat broken, since the very first imagining of this application. If you'd rather just strip it out, I can take a look at existing forms to see if we can safely deprecate.

@yanokwa
Copy link
Member

yanokwa commented May 19, 2017

I legit can't think of why we had it as a thing in the first place. Summoning @MartijnR in case he knows.

Either way, I agree that a good next step is to see how existing forms use it.

@MartijnR
Copy link

I don't know what it is either. A form control without a corresponding primary instance node would crash a form in Enketo.

@issa-tseng
Copy link
Member

@MartijnR I think the question is whether there is a strong case for being able to customize the exact instance destination node path; eg you want to build your own XML structure, vs rely on the automagic one we infer.

@yanokwa
Copy link
Member

yanokwa commented May 22, 2017

I can't think of a strong case for being able to customize the exact instance destination node. Or rather, in my many years of form design, it has never crossed my mind to want that feature and I haven't seen someone ask for it on the mailing list. For those reasons, I'm voting for a deprecation, but I think it'd be prudent to wait for the review of the existing forms.

@MartijnR
Copy link

MartijnR commented May 22, 2017 via email

@issa-tseng
Copy link
Member

Okay. For now, I will remove this feature from the 0.3.1 release slate, and we'll look at deprecation possibilities instead for a future release.

@issa-tseng issa-tseng removed this from the 0.3.1 milestone May 22, 2017
@issa-tseng issa-tseng added this to the 0.3.3 milestone Jun 12, 2017
@issa-tseng
Copy link
Member

Putting this in the 0.3.3 milestone to investigate whether anyone is using this feature and make a final decision on merging the fix or cutting the feature entirely.

@issa-tseng
Copy link
Member

I performed a review of this feature's usage. Only ~0.4% of forms attempt to use instance destination, and of those that do the vast majority don't know what they're doing with it. There are a lot of filesystem paths, URLs, and email addresses, along with relevance/calculation expressions, names with spaces, repeated values that would overwrite the same path repeatedly, and other sundry.

Having reviewed every instance of this feature's use, it seems deprecation would disappoint roughly 3 people, who were probably already disappointed when their careful use of the feature failed to work without PR #139.

Deprecating.

issa-tseng added a commit to issa-tseng/odkbuild that referenced this issue Jul 6, 2017
* See getodk#66 (comment)
  for deprecation rationale.
* This deprecation does not attempt to prune the now-dormant data being
  carried around on the internal JSON payload. This is largely harmless,
  and can be done at a later date should a desire arise.
* Any form that attempted to use this feature would have behaved
  unexpectedly without PR getodk#139; the only behavioural change these forms
  will see is that binding will point at valid inferred XML nodes rather
  than probably-nonexistent custom ones.
lognaturel added a commit that referenced this issue Jul 18, 2017
control/rm #66: remove instance destination feature entirely.
@issa-tseng
Copy link
Member

Resolved by deletion (#167).

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

Successfully merging a pull request may close this issue.

6 participants