added ability to edit associations #230
Conversation
| * The drupal form state. | ||
| * @param string $form_name | ||
| * The name of an XML form. | ||
| * |
There was a problem hiding this comment.
@param for $id is missing here
builder/xml_form_builder.module
Outdated
| * | ||
| * @return DOMDocument | ||
| * Transformed document on success, original on failure. | ||
| */ | ||
| function xml_form_builder_transform_metadata_datastream(DOMDocument $source_document, $transform = NULL) { | ||
| function xml_form_builder_transform_metadata_datastream(DOMDocument $source_document, xslt $transform = NULL) { |
There was a problem hiding this comment.
if xslt really a data type/class? Not sure about that. Where is that defined so? Seems to me that array_key_exists($transform, $transforms) denotes that $transform is just an array key value, so type hinting there should be wrong and also annotation @params for $transform?
|
I tested this from the front end and it looks good. @jordandukart @DiegoPino are you satisfied with your code review? |
|
Hey @jordandukart 😄 checking on this pull, do you feel @ajstanley solved your concerns or still something missing? Thanks a lot! |
| } | ||
| } | ||
| $options = xml_form_builder_get_title_options($form_name); | ||
| $search = "['" . implode("']['", $association['title_field']) . "']"; |
There was a problem hiding this comment.
If there is no title_field set it's stored as FALSE and this implode will break.
|
@jordandukart @ajstanley hi. Anything missing here? Are you all ok on the changes or issues pending? |
|
Its all good AFAIK, Diego |
|
@jordandukart will ping you again about the issues you found originally. Are they addressed? @ajstanley says its all good /thanks for the patience on this, and I really feel this is a good candidate for our incoming release. Thanks! |
|
@jordandukart ping. Just need to be sure Concerns are addressed, don't want to make @ajstanley wait (more) but if there are pending things here lets discuss then. Thanks! 😄 |
|
Will try go get this today. |
|
Our PHPCS is behind a bit (think this is on the radar to update but): |
| $id = $form_state['values']['id']; | ||
| $form_name = $form_state['values']['form_name']; | ||
| $template = ''; | ||
| $file_uploaded = isset($form_state['values']['template_file']); |
There was a problem hiding this comment.
This pops warnings as the file is managed the value is a fid. So when the fid isn't 0 it'll have been uploaded.
Notice: Trying to get property of non-object in xml_form_builder_edit_association_form_submit() (line 426 of /var/www/drupal7/sites/all/modules/islandora_xml_forms/builder/includes/associations.form.inc).
Warning: DOMDocument::load(): Empty string supplied as input in load() (line 426 of /var/www/drupal7/sites/all/modules/islandora_xml_forms/builder/includes/associations.form.inc).
Could just do if ($form_state['values']['template_file'] && !$form_state['values']['remove_transform']) and remove the $file_uploaded var.
|
Not a blocking problem but a general comment that the auto-formatting of things that weren't directly changed makes reading these diffs a pain. |
|
I had some inconsistent warnings with coder spacings, but it should all be good now - Apologies about the auto-formatting. |
|
@ajstanley sorry for making you wait so long. Will test tonight and merge tomorrow. Thanks |
|
@rosiel I tested this long time ago and forgot probably. Can't be sure. If you want to merge and you tested, deeply go for it. |
JIRA Ticket: (https://jira.duraspace.org/browse/ISLANDORA-1765)
What does this Pull Request do?
Adds the ability to alter existing form associations
What's new?
Added a link under operations to move to a form allowing changes to be made.
How should this be tested?
Navigate to existing editable for association,
There will be an Edit button next to the Delete button
Clicking Edit will take you to an edit form.
Change one or more values in this association.
Save, and check to see that changes persisited
Interested parties
Tag (@Islandora/7-x-1-x-committers)