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

ISLANDORA-1559 #211

Merged
merged 2 commits into from
Dec 7, 2015
Merged

ISLANDORA-1559 #211

merged 2 commits into from
Dec 7, 2015

Conversation

matthewperry
Copy link
Contributor

Jira: https://jira.duraspace.org/browse/ISLANDORA-1559

Other Related Pull - Objective Forms:

Islandora/objective_forms#40

What does this Pull Request do?

Makes new tabs that are created respect the original form default values.

How should this be tested?

By using a form builder form that has a tabpanel which has a field that uses a default value. This default value should now prepopulate into the newly cloned tab instead of forcing to NULL.

Test the form while ingesting, editing, and using the form in the Form Builder Preview (when editing the form).

Example 1:
Default Form: Collection MODS Form
Within this form the name (tabs) -> 0 (tabpanel) -> type (select) it has the default value set of personal.

The first tab when you load the form uses the correct default but as soon as you add a new tab the newly cloned tab will default to corporate as this is the first option in the select box.

With this change when you add the new tab it will load the default value of the initial "empty/new" form and set the select option to personal, while continuing to force fields without a default value to NULL so they remain empty.

Background context:

Causes issues when using tabs that have set values and when new tabs are created without having the values set can result in missing xml values/attributes.

Tagging: @DiegoPino, @ruebot, @nigelgbanks


Matthew Perry
Developer
discoverygarden inc. | Managing Digital Content

@DiegoPino
Copy link
Contributor

@matthewperry, let me look at this one in detail (1-2 days) and test some scenarios. A first look into the pull makes me feel there could be a simpler way to do this, loading a whole form every time you want to clone a tab can be resource intensive and i'm not fully sure how it handles nested special elements.

Thanks 😄

@matthewperry
Copy link
Contributor Author

@DiegoPino, I've run some testing and you have to load a new form because the way the form builder currently works is when the form is created with a pre-existing form_state the fields default_value is updated to be the values from the form_state.

An example:

When editing a form that has a name field in a tab with the value: Matthew Perry (already saved) but the default is "Anonymous" if I use the current form without loading a new empty form when I add a new tab it will add it with the value set to "Matthew Perry".

@DiegoPino
Copy link
Contributor

@matthewperry, yeah, i got that one, but it's an overkill (lots of ram, processing resources, many things can go wrong) if you call https://github.com/Islandora/islandora_xml_forms/blob/7.x/builder/xml_form_builder.module#L438-L471 on every clone command.

What i would suggest is, if this functionality is really required, to add a new property/method to the element in the registry (objective forms) to keep track of the original values. This way getting that default (the original one) is just one step over the already in $form_state element registry. No need to load the form again. Also easier to maintain. I feel it's almost the same amount of code, but targets the need in a more efficient way. Just my thoughts.

Thanks

@matthewperry
Copy link
Contributor Author

@DiegoPino, I'll look take a look into that.
Thanks

@ruebot
Copy link
Member

ruebot commented Dec 1, 2015

Just a heads up TravisCI is going to fail until we sort this out.

...currently working through it on CLAW here: Islandora/documentation#125 and will do a pull request for 7.x-1.x Islandora as soon as I (or we) figure it out.

@ruebot
Copy link
Member

ruebot commented Dec 1, 2015

OH! I think this explains everything: http://pear.php.net/

screenshot from 2015-12-01 12 06 24

@ruebot
Copy link
Member

ruebot commented Dec 2, 2015

…ll is dependant on an update to Object Forms module.
@matthewperry
Copy link
Contributor Author

@DiegoPino Made some modifications based on feedback, it now requires a change in Objective Forms. Islandora/objective_forms#40

@DiegoPino
Copy link
Contributor

@matthewperry, will test tomorrow AM, commented on the dependant ticket too. Looks nice and clean 👍

Thanks!

@ruebot
Copy link
Member

ruebot commented Dec 6, 2015

Ok y'all, PEAR is back, and testing is working again as it should.

I've restarted all the failed builds, and we should be good to go shortly.

DiegoPino added a commit that referenced this pull request Dec 7, 2015
ISLANDORA-1559
Makes new tabs that are created respect the original form default values.
@DiegoPino DiegoPino merged commit 0b9b18c into Islandora:7.x Dec 7, 2015
@DiegoPino
Copy link
Contributor

@matthewperry, @ruebot. Merged this, also the required parent one Islandora/objective_forms#40

Thanks

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

Successfully merging this pull request may close these issues.

3 participants