Feature/editing forms with data#23
Conversation
…ectly. Main problem currently is that behavior after deleting a field is somewhat inconsistent and should be tested more.
…kend still need to be informed of what this means.
…ion, remove unnecessary var_dump
…to feature/editing_forms_with_data
…to feature/editing_forms_with_data
if $ele = $this->get_element($element_name) returns FALSE, we don't want to check for props
laupow
left a comment
laupow
left a comment
There was a problem hiding this comment.
Nice work. There are a few tweaks, but otherwise, I think it's looking good.
I'll email you two screenshots to make sure there isn't a stray HTML element somewhere causing some font size problems. It might be an anomaly on my end and not related to your changes, but let's check.
| $this->_transform_upload($node, $disco_obj); | ||
| } elseif ($node->tagName == 'event_tickets') { | ||
| $this->_transform_event_tickets($node, $disco_obj); | ||
| if (!$node->tagAttrs['deleted']) |
There was a problem hiding this comment.
We'll probably want to add a check to avoid the warning message below.
Notice: Undefined index: deleted at …/thor/thor.php:184(when deleted isn't an attr on a $node.)
| $this->show_error_jumps = false; | ||
| } | ||
| $data_manager_link = $this->admin_page->make_link(array('cur_module' => 'ThorData')); | ||
| $publish_status_text .= '<p><strong>This form has stored data. </strong><a href="' . $data_manager_link . '">Manage stored data</a><br>You can edit the form, but if you remove a field, it will remain in the database to prevent unintentional data loss. You will not be able to change the type of existing fields.</p>'; |
There was a problem hiding this comment.
A message tweak to make for $publish_status_text:
"You can add new form fields and modify existing fields, but if you remove a field, the removed field's data will persist until you delete the stored data for this form. Changing the type of an existing form field is unsupported."
| if (empty($emailNode)) { | ||
| $this->set_error('thor_content', "An event ticket form requires a short text input with the exact label 'Your Email'. Please add that element or change the email field label to 'Your Email'."); | ||
| } | ||
| $eventTicketsClose = $xml->xpath("/*/event_tickets")[0]['event_close_datetime']; |
There was a problem hiding this comment.
Could you test this on a form with multiple Events? I know we talked about it with the [0] syntax, but looking at it now I think it's gonna break when forms have multiple event ticket fields (they usually do). Thx.
| $this->_transform_upload($node, $disco_obj); | ||
| } elseif ($node->tagName == 'event_tickets') { | ||
| $this->_transform_event_tickets($node, $disco_obj); | ||
| if (!$node->tagAttrs['deleted']) { |
There was a problem hiding this comment.
There's a PHP notice that pops up on forms that exist today and don't have the deleted attr.
Notice: Undefined index: deleted at …/thor/thor.php:184
Could you tweak the line so it ensures the key 'deleted' exists in tagAttrs, first?
| $data_comment.='<p>To edit this form, you will first need to delete the stored data.</p></div>'; | ||
| $this->change_element_type('thor_comment','comment',array('text'=>$data_comment)); | ||
| $data_manager_link = $this->admin_page->make_link(array('cur_module' => 'ThorData')); | ||
| $data_comment = '<div id="manageDataNote"><p><strong>This form has stored data, and uses a custom thor view. </strong><a href="' . $data_manager_link . '">Manage stored data</a></p>'; |
There was a problem hiding this comment.
Change "uses a custom thor view" to "has custom form logic installed."
…nathanielwarner/reasoncms into feature/editing_forms_with_data # Conflicts: # reason_4.0/lib/core/content_managers/thor.php3
…than one associated event
Rather than a straight echo, which spit out the string before the html doctype
Old Thor XML won't have this attr, so we have to detect its presence before testing its value.
The previous implementation wasn't deleting all elements where deleted=true
mryand
left a comment
mryand
left a comment
There was a problem hiding this comment.
I note one edge case that could be handled better.
Step 1: Set up a form with checkboxes, and make it editable by the submitter
Step 2: Fill out this form, checking the checkboxes
Step 3: Edit the form as an administrator, changing one of the labels on the checkboxes
Step 4: Edit the submission you sent before the form change.
Note that the option you previously checked is not visible. Once the form is saved, the changed checkbox column is overwritten either with an empty string or with the new value. The old value is not saved.
I know this is an edge case, but I could see it biting people.
A preferred behavior would be more similar to the way radio buttons and dropdowns work, where the option selected before the change is added as an additional option.
Similarly, it would be ideal if an additional checkbox option with the stored label could be added.
| $this->show_error_jumps = false; | ||
| } | ||
| $data_manager_link = $this->admin_page->make_link(array('cur_module' => 'ThorData')); | ||
| $publish_status_text .= '<p><strong>This form has stored data. </strong><a href="' . $data_manager_link . '">Manage stored data</a><br>You can add new form fields and modify existing fields, but if you remove a field, the removed field\'s data will persist until you delete the stored data for this form. Changing the type of an existing field is unsupported.</p>'; |
There was a problem hiding this comment.
I don't think we need to specify what's the same as when you don't have stored data, just what's different. Suggested text edit:
If you remove a field, the field's data will remain available until you delete the stored data for this form. Changing existing fields' types is not supported.
There was a problem hiding this comment.
I just saw Doug's suggestion, which is better. So the message would be:
If you remove a field, the field's data will remain available until you delete the stored data for this form. You cannot change the type of existing fields.
|
This looks to be ready to merge barring the comment from @mryand about labels on checkboxes (Feb 2). I think that change should be made before we roll this out to production. @nathanielwarner or @laupow can you speak to that? |
…with_data * production: Improved description of Type Usage module Added support for reporting on module usage Added get_modules_to_page_types() and get_all_modules() methods Added Type Usage admin module Added type usage report Added detail/example view to sharing stats module Bumped version number for media_api.js Switched to using style attribute instead of deprecated presentational attributes on media iframes Fixed error that was causing video.js to not work on crossdomain iframes Added ability to specify only live sites in get_site_id_from_url() Added ability to exclude restricted works from the feed Added batch select/deselect for events Improved labeling of the share site access module Added reading ease notifier and new setting to make it easy to turn grade levels and reading ease on and off Fixed input limiter bug - js was not finding proper url for checking count Updated to use the proper remote now that the maintainer has merged in our PR Added sanitize_slug() added publication_related_under_nav and publication_related_under_nav_sidebar_blurbs
…re/editing_forms_with_data
4 participants
No description provided.