Islandora-Collaboration-Group / islandora_webform

islandora_webform
GNU General Public License v3.0
6 stars 8 forks source link

tags inside tabsets aren't saved during iwi ingest #23

Closed patdunlavey closed 8 years ago

patdunlavey commented 8 years ago

Using a mods form with the tags inside of tabsets, values entered into the tag fields are not saved during ingest.

Here is what my MODS schema looks like: screen shot 2015-11-06 at 3 45 54 pm

I can map values to the tags field. I can also add and remove tags in the ingest preview form. But submitting the form does not result in them being saved in the MODS datastream. Everything else saves fine.

If I edit the MODS data after the object has been ingested, using the regular MODS form, that works fine.

DiegoPino commented 8 years ago

@patdunlavey, new form? can you send me that one to debug?Also using which branch(for alter hook, issue-14?) or the main one? because a lot changed since then.

patdunlavey commented 8 years ago

Just documenting what we discussed offline: I encountered the bug on my local install, which is using the branch "feature/ingest_form_alter_hook". I just tested on unbound-dev.williams.edu, which is using "master" branch, and the bug is there as well.

And here's the form: MODS Universal Form Faculty Publications.xml.txt

DiegoPino commented 8 years ago

@patdunlavey, resolved. It's related to having multiple XML forms (very different ones) associated to the same DSID. I managed to solve this without touching the hash function at all, only modifying some vars we are passing to the submit hander. Will pull tomorrow morning. Works 100%

patdunlavey commented 8 years ago

@DiegoPino - a couple issues:

First, when a tab is cloned in iwi_set_form_element_byhash, it clones the tags that may have been added to it. Though it sets all their values to NULL, it retains the tag elements. This results in each tab having a series of tags with no value accumulated from previously created tabs. Took me a while to track that down! My possible solution can be seen in this commit: https://github.com/patdunlavey/islandora_webform/commit/d746ec8058743df971f9f52270c21f3f72e78add. Does that make sense to you? I'm not 100% sure if it's valid to assume that any special element-type children in a tabset should not be cloned. But I can't think of another way to prevent this problem.

Second problem is that any time I try one of the ajax functions on the ingest form, I get "The form has become outdated. Copy any unsaved work in the form below and then reload this page." I think that's new, but I'm not sure. It happens whether or not I'm doing hook_iwi_set_form_element_alter, so I don't think it's related to that.

I'll test on some other MODS forms to see if it's just our weird form that's causing the problem!

Thanks, Pat

patdunlavey commented 8 years ago

Yep, it works fine on the Basic Image MODS form.

DiegoPino commented 8 years ago

@patdunlavey, i would need to re-review the code again, since this pull was 29 days ago, but in my personal opinion those issues are not issues at all/or related to this module in particular.

The first issue is expected. There is no way we can know(in 7.x-1.6) which tags are new or which where there by default. The clone function (islandora xml forms) clones the element and it's child. There is a new merge in xml forms that marks original ones (versus cloned) ones but it's in 7.x of xml forms + objective forms. https://github.com/Islandora/islandora_xml_forms/pull/211 https://github.com/Islandora/objective_forms/pull/40

The second one can't be reproduced on my side, so there must be some not-this-module custom code trying to mess with the form (there is a '#token' inside drupal that is used to check a forms consistency to avoid hijacking) or even some invalid action in that particular form that writes xml elements -> form elements outside the expected dom breaking drupal's consistency check.

Best

Diego

patdunlavey commented 8 years ago

Thanks @DiegoPino. I'm willing to consider making 7.x of islandora_xml_forms and objective_forms a dependency, though I'd rather not. (Incidentally, it occurs to me that dependency management is a reason why numbered releases may be a good thing.)

Are we already assuming something about the cloned tab when we set the value of all tags inside it to NULL? I don't see how removing all but one child tag, and setting the value of the remaining tag to NULL, is inconsistent with that. Along with this, is it unrealistic to assume that tabs or tags with an index of 0 are original (since we're setting the value to NULL anyway, it actually doesn't actually matter which tag was original anyway).

What I think is clearly not right is to leave a bunch of tags in place with no value.

Another thought: Would it be outrageous to have some kind of argument that can be passed in to iwi_set_form_element_byhash that instructs it to remove all but one child tag and to set the remaining tag to NULL. Something like 'reset_tags' = TRUE.

I'll look into what branches have been merged into what to see if I can find the cause of the '#token' problem.

Thanks so much! -Pat

DiegoPino commented 8 years ago

@patdunlavey, i'm not saying it's a desired functionality, but it's expected. Moreover since normal xml forms work that way. I'm not against doing whatever tag removal strategy you want or implementing the code you already wrote for this. But i would prefer if this would be another ticket and this one closed, because this one is resolved with my pull

Thanks!

patdunlavey commented 8 years ago

Ok, sorry about that.

Separate issue created: https://github.com/commonmedia/islandora_webform/issues/27