MDL-5583 mod_data: Improve required fields

Fix accidental <tr> in some field modify screens Update mod_data version Change required asterisk to image Improve required error message Fix required icon positions Remove required code from date field Add name in labels for fields Add required field option for multimenu Remove old required field title text modifier Add multimenu to behat Add more comprehensive behat tests Reload old input when an input error occurs Behat grammar fixes Allow location of 0, 0 Use html_writer Fix existing mod_data behat tests
moodle · Mar 9, 2015 · 1c3b205 · 1c3b205
1 parent b89cca1
commit 1c3b205
Show file tree

Hide file tree

Showing 27 changed files with 303 additions and 169 deletions.
diff --git a/mod/data/db/install.xml b/mod/data/db/install.xml
@@ -1,5 +1,5 @@
 <?xml version="1.0" encoding="UTF-8" ?>
-<XMLDB PATH="mod/data/db" VERSION="20141001" COMMENT="XMLDB file for Moodle mod/data"
+<XMLDB PATH="mod/data/db" VERSION="20150205" COMMENT="XMLDB file for Moodle mod/data"
     xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance"
     xsi:noNamespaceSchemaLocation="../../../lib/xmldb/xmldb.xsd"
 >
@@ -54,7 +54,7 @@
         <FIELD NAME="type" TYPE="char" LENGTH="255" NOTNULL="true" SEQUENCE="false"/>
         <FIELD NAME="name" TYPE="char" LENGTH="255" NOTNULL="true" SEQUENCE="false"/>
         <FIELD NAME="description" TYPE="text" NOTNULL="true" SEQUENCE="false"/>
-        <FIELD NAME="required" TYPE="int" LENGTH="1" NOTNULL="true" DEFAULT="0" SEQUENCE="false" COMMENT="Private fields are only visible or editable by users with additional capabilities"/>
+        <FIELD NAME="required" TYPE="int" LENGTH="1" NOTNULL="true" DEFAULT="0" SEQUENCE="false" COMMENT="Required fields must have a value when inserted by a user"/>
         <FIELD NAME="param1" TYPE="text" NOTNULL="false" SEQUENCE="false"/>
         <FIELD NAME="param2" TYPE="text" NOTNULL="false" SEQUENCE="false"/>
         <FIELD NAME="param3" TYPE="text" NOTNULL="false" SEQUENCE="false"/>

diff --git a/mod/data/db/upgrade.php b/mod/data/db/upgrade.php
@@ -133,7 +133,11 @@ function xmldb_data_upgrade($oldversion) {
 
     // Moodle v2.7.0 release upgrade line.
     // Put any upgrade step following this.
-    if ($oldversion < 2014051201) {
+
+    // Moodle v2.8.0 release upgrade line.
+    // Put any upgrade step following this.
+
+    if ($oldversion < 2015022600) {
         // Define field required to be added to data_fields.
         $table = new xmldb_table('data_fields');
         $field = new xmldb_field('required', XMLDB_TYPE_INTEGER, '1', null, XMLDB_NOTNULL, null, '0', 'description');
@@ -143,11 +147,8 @@ function xmldb_data_upgrade($oldversion) {
             $dbman->add_field($table, $field);
         }
 
-        upgrade_mod_savepoint(true, 2014051201, 'data');
+        upgrade_mod_savepoint(true, 2015022600, 'data');
     }
 
-    // Moodle v2.8.0 release upgrade line.
-    // Put any upgrade step following this.
-
     return true;
 }
diff --git a/mod/data/edit.php b/mod/data/edit.php
@@ -330,7 +330,7 @@
                     $errors .= $OUTPUT->notification($notification);
                 }
             }
-            $replacements[] = $errors . $field->display_add_field($rid);
+            $replacements[] = $errors . $field->display_add_field($rid, $datarecord);
         }
 
         // Replace the field id tag.

diff --git a/mod/data/field.php b/mod/data/field.php
@@ -266,11 +266,11 @@
 
         $table = new html_table();
         $table->head = array(
-            get_string('fieldname','data'),
+            get_string('fieldname', 'data'),
             get_string('type', 'data'),
             get_string('required', 'data'),
             get_string('fielddescription', 'data'),
-            get_string('action','data'),
+            get_string('action', 'data'),
         );
         $table->align = array('left','left','left', 'center');
         $table->wrap = array(false,false,false,false);

diff --git a/mod/data/field/checkbox/field.class.php b/mod/data/field/checkbox/field.class.php
@@ -27,7 +27,7 @@ class data_field_checkbox extends data_field_base {
     var $type = 'checkbox';
 
     function display_add_field($recordid = 0, $formdata = null) {
-        global $CFG, $DB;
+        global $CFG, $DB, $OUTPUT;
 
         $content = array();
 
@@ -41,13 +41,18 @@ function display_add_field($recordid = 0, $formdata = null) {
             $content = array();
         }
 
-        $str = '';
+        $str = '<div title="' . s($this->field->description) . '">';
+        $str .= '<fieldset><legend><span class="accesshide">'.$this->field->name;
         if ($this->field->required) {
-            $str .= '<div title="' . get_string('requiredfieldhint', 'data', s($this->field->description)) . '">';
+            $str .= '$nbsp;' . get_string('requiredelement', 'form');
+            $str .= '</span></legend>';
+            $str .= '<div>';
+            $str .= html_writer::img($OUTPUT->pix_url('req'), get_string('requiredelement', 'form'),
+                                     array('class' => 'req', 'title' => get_string('requiredelement', 'form')));
+            $str .= '</div>';
         } else {
-            $str .= '<div title="' . s($this->field->description) . '">';
+            $str .= '</span></legend>';
         }
-        $str .= '<fieldset><legend><span class="accesshide">'.$this->field->name.'</span></legend>';
 
         $i = 0;
         foreach (explode("\n", $this->field->param1) as $checkbox) {

diff --git a/mod/data/field/date/field.class.php b/mod/data/field/date/field.class.php
@@ -51,12 +51,7 @@ function display_add_field($recordid = 0, $formdata = null) {
             $content = time();
         }
 
-        $str = '';
-        if ($this->field->required) {
-            $str .= '<div title="' . get_string('requiredfieldhint', 'data', s($this->field->description)) . '">';
-        } else {
-            $str .= '<div title="' . s($this->field->description) . '">';
-        }
+        $str = '<div title="'.s($this->field->description).'">';
         $dayselector = html_writer::select_time('days', 'field_'.$this->field->id.'_day', $content);
         $monthselector = html_writer::select_time('months', 'field_'.$this->field->id.'_month', $content);
         $yearselector = html_writer::select_time('years', 'field_'.$this->field->id.'_year', $content);

diff --git a/mod/data/field/file/field.class.php b/mod/data/field/file/field.class.php
@@ -39,7 +39,7 @@ function display_add_field($recordid = 0, $formdata = null) {
         if ($formdata) {
             $fieldname = 'field_' . $this->field->id . '_file';
             $itemid = $formdata->$fieldname;
-        } else if ($recordid){
+        } else if ($recordid) {
             if ($content = $DB->get_record('data_content', array('fieldid'=>$this->field->id, 'recordid'=>$recordid))) {
 
                 file_prepare_draft_area($itemid, $this->context->id, 'mod_data', 'content', $content->id);
@@ -65,14 +65,18 @@ function display_add_field($recordid = 0, $formdata = null) {
             $itemid = file_get_unused_draft_itemid();
         }
 
-        $html = '';
         // database entry label
+        $html = '<div title="' . s($this->field->description) . '">';
+        $html .= '<fieldset><legend><span class="accesshide">'.$this->field->name;
+
         if ($this->field->required) {
-            $html .= '<div title="' . get_string('requiredfieldhint', 'data', s($this->field->description)) . '">';
+            $html .= '&nbsp;' . get_string('requiredelement', 'form') . '</span></legend>';
+            $image = html_writer::img($OUTPUT->pix_url('req'), get_string('requiredelement', 'form'),
+                                     array('class' => 'req', 'title' => get_string('requiredelement', 'form')));
+            $html .= html_writer::div($image);
         } else {
-            $html .= '<div title="' . s($this->field->description) . '">';
+            $html .= '</span></legend>';
         }
-        $html .= '<fieldset><legend><span class="accesshide">'.$this->field->name.'</span></legend>';
 
         // itemid element
         $html .= '<input type="hidden" name="field_'.$this->field->id.'_file" value="'.$itemid.'" />';
@@ -90,10 +94,6 @@ function display_add_field($recordid = 0, $formdata = null) {
 
         $output = $PAGE->get_renderer('core', 'files');
         $html .= $output->render($fm);
-
-        if ($this->field->required) {
-            $html .= '<span class="requiredfield">' . get_string('requiredfieldshort', 'data') . '</span>';
-        }
         $html .= '</fieldset>';
         $html .= '</div>';
 

diff --git a/mod/data/field/latlong/field.class.php b/mod/data/field/latlong/field.class.php
@@ -44,7 +44,7 @@ class data_field_latlong extends data_field_base {
     // Other map sources listed at http://kvaleberg.com/extensions/mapsources/index.php?params=51_30.4167_N_0_7.65_W_region:earth
 
     function display_add_field($recordid = 0, $formdata = null) {
-        global $CFG, $DB;
+        global $CFG, $DB, $OUTPUT;
 
         $lat = '';
         $long = '';
@@ -59,25 +59,26 @@ function display_add_field($recordid = 0, $formdata = null) {
                 $long = $content->content1;
             }
         }
-        $str = '';
-        if ($this->field->required) {
-            $str .= '<div title="' . get_string('requiredfieldhint', 'data', s($this->field->description)) . '">';
-        } else {
-            $str .= '<div title="' . s($this->field->description) . '">';
-        }
+        $str = '<div title="'.s($this->field->description).'">';
         $str .= '<fieldset><legend><span class="accesshide">'.$this->field->name.'</span></legend>';
         $str .= '<table><tr><td align="right">';
-        $str .= '<label for="field_'.$this->field->id.'_0">' . get_string('latitude', 'data') . '</label></td><td><input type="text" name="field_'.$this->field->id.'_0" id="field_'.$this->field->id.'_0" value="'.s($lat).'" size="10" />°N</td></tr>';
-        $str .= '<tr><td align="right"><label for="field_'.$this->field->id.'_1">' . get_string('longitude', 'data') . '</label></td><td><input type="text" name="field_'.$this->field->id.'_1" id="field_'.$this->field->id.'_1" value="'.s($long).'" size="10" />°E</td>';
+        $str .= '<label for="field_'.$this->field->id.'_0">' . get_string('latitude', 'data');
+        if ($this->field->required) {
+            $str .= html_writer::img($OUTPUT->pix_url('req'), get_string('requiredelement', 'form'),
+                                     array('class' => 'req', 'title' => get_string('requiredelement', 'form')));
+        }
+        $str .= '</label></td><td><input type="text" name="field_'.$this->field->id.'_0" id="field_'.$this->field->id.'_0" value="';
+        $str .= s($lat).'" size="10" />°N</td></tr>';
+        $str .= '<tr><td align="right"><label for="field_'.$this->field->id.'_1">' . get_string('longitude', 'data');
         if ($this->field->required) {
-            $str .= '<td><span class="requiredfield">' . get_string('requiredfieldshort', 'data') . '</span></td>';
+            $str .= html_writer::img($OUTPUT->pix_url('req'), get_string('requiredelement', 'form'),
+                                     array('class' => 'req', 'title' => get_string('requiredelement', 'form')));
         }
+        $str .= '</label></td><td><input type="text" name="field_'.$this->field->id.'_1" id="field_'.$this->field->id.'_1" value="';
+        $str .= s($long).'" size="10" />°E</td>';
         $str .= '</tr>';
         $str .= '</table>';
         $str .= '</fieldset>';
-        if ($this->field->required) {
-            $str .= get_string('requiredfieldhint', 'data', s($this->field->description));
-        }
         $str .= '</div>';
         return $str;
     }
@@ -240,4 +241,15 @@ function export_text_value($record) {
         return sprintf('%01.4f', $record->content) . ' ' . sprintf('%01.4f', $record->content1);
     }
 
+    /**
+     * Check if a field from an add form is empty
+     *
+     * @param mixed $value
+     * @param mixed $name
+     * @return bool
+     */
+    function notemptyfield($value, $name) {
+        return isset($value) && !($value == '');
+    }
+
 }
diff --git a/mod/data/field/menu/field.class.php b/mod/data/field/menu/field.class.php
@@ -32,18 +32,13 @@ function display_add_field($recordid = 0, $formdata = null) {
         if ($formdata) {
             $fieldname = 'field_' . $this->field->id;
             $content = $formdata->$fieldname;
-        } else if ($recordid){
+        } else if ($recordid) {
             $content = $DB->get_field('data_content', 'content', array('fieldid'=>$this->field->id, 'recordid'=>$recordid));
             $content = trim($content);
         } else {
             $content = '';
         }
-        $str = '';
-        if ($this->field->required) {
-            $str .= '<div title="' . get_string('requiredfieldhint', 'data', s($this->field->description)) . '">';
-        } else {
-            $str .= '<div title="' . s($this->field->description) . '">';
-        }
+        $str = '<div title="' . s($this->field->description) . '">';
 
         $options = array();
         $rawoptions = explode("\n",$this->field->param1);
@@ -54,11 +49,15 @@ function display_add_field($recordid = 0, $formdata = null) {
             }
         }
 
-        $str .= html_writer::label(get_string('menuchoose', 'data'), 'field_'.$this->field->id, false, array('class' => 'accesshide'));
-        $str .= html_writer::select($options, 'field_'.$this->field->id, $content, array(''=>get_string('menuchoose', 'data')), array('id'=>'field_'.$this->field->id));
+        $str .= '<label for="' . 'field_' . $this->field->id . '">';
+        $str .= html_writer::span($this->field->name, 'accesshide');
         if ($this->field->required) {
-            $str .= '<span class="requiredfield">' . get_string('requiredfieldshort', 'data') . '</span>';
+            $image = html_writer::img($OUTPUT->pix_url('req'), get_string('requiredelement', 'form'),
+                                     array('class' => 'req', 'title' => get_string('requiredelement', 'form')));
+            $str .= html_writer::div($image);
         }
+        $str .= '</label>';
+        $str .= html_writer::select($options, 'field_'.$this->field->id, $content, array(''=>get_string('menuchoose', 'data')), array('id'=>'field_'.$this->field->id));
 
         $str .= '</div>';
 

diff --git a/mod/data/field/menu/mod.html b/mod/data/field/menu/mod.html
@@ -7,7 +7,6 @@
         <td class="c0"><label for="description"><?php echo get_string('fielddescription', 'data'); ?></label></td>
         <td class="c1"><input class="fielddescription" type="text" name="description" id="description" value="<?php p($this->field->description);?>" /></td>
     </tr>
-    <tr>
     <tr>
         <td class="c0"><label for="required"><?php echo get_string('requiredfield', 'data'); ?></label></td>
         <td class="c1"><input class="requiredfield" type="checkbox" name="required" id="required" <?php p($this->field->required?"checked=\"checked\"":""); ?>/></td>

diff --git a/mod/data/field/multimenu/field.class.php b/mod/data/field/multimenu/field.class.php
@@ -27,12 +27,16 @@ class data_field_multimenu extends data_field_base {
     var $type = 'multimenu';
 
     function display_add_field($recordid = 0, $formdata = null) {
-        global $DB;
+        global $DB, $OUTPUT;
 
         if ($formdata) {
             $fieldname = 'field_' . $this->field->id;
-            $content = $formdata->$fieldname;
-        } else if ($recordid){
+            if (isset($formdata->$fieldname)) {
+                $content = $formdata->$fieldname;
+            } else {
+                $content = array();
+            }
+        } else if ($recordid) {
             $content = $DB->get_field('data_content', 'content', array('fieldid'=>$this->field->id, 'recordid'=>$recordid));
             $content = explode('##', $content);
         } else {
@@ -41,10 +45,19 @@ function display_add_field($recordid = 0, $formdata = null) {
 
         $str = '<div title="'.s($this->field->description).'">';
         $str .= '<input name="field_' . $this->field->id . '[xxx]" type="hidden" value="xxx"/>'; // hidden field - needed for empty selection
-        $str .= '<label class="accesshide" for="field_' . $this->field->id . '">' . $this->field->name. '</label>';
+
+        $str .= '<label for="field_' . $this->field->id . '">';
+        $str .= html_writer::span($this->field->name, 'accesshide');
+        if ($this->field->required) {
+            $str .= '<div>';
+            $str .= html_writer::img($OUTPUT->pix_url('req'), get_string('requiredelement', 'form'),
+                                     array('class' => 'req', 'title' => get_string('requiredelement', 'form')));
+            $str .= '</div>';
+        }
+        $str .= '</label>';
         $str .= '<select name="field_' . $this->field->id . '[]" id="field_' . $this->field->id . '" multiple="multiple">';
 
-        foreach (explode("\n",$this->field->param1) as $option) {
+        foreach (explode("\n", $this->field->param1) as $option) {
             $option = trim($option);
             $str .= '<option value="' . s($option) . '"';
 
@@ -241,4 +254,16 @@ function display_browse_field($recordid, $template) {
         }
         return false;
     }
+
+    /**
+     * Check if a field from an add form is empty
+     *
+     * @param mixed $value
+     * @param mixed $name
+     * @return bool
+     */
+    function notemptyfield($value, $name) {
+        unset($value['xxx']);
+        return !empty($value);
+    }
 }
diff --git a/mod/data/field/multimenu/mod.html b/mod/data/field/multimenu/mod.html
@@ -7,6 +7,10 @@
         <td class="c0"><label for="description"><?php echo get_string('fielddescription', 'data'); ?></label></td>
         <td class="c1"><input class="fielddescription" type="text" name="description" id="description" value="<?php p($this->field->description); ?>" /></td>
     </tr>
+    <tr>
+        <td class="c0"><label for="required"><?php echo get_string('requiredfield', 'data'); ?></label></td>
+        <td class="c1"><input class="requiredfield" type="checkbox" name="required" id="required" <?php p($this->field->required?"checked=\"checked\"":""); ?>/></td>
+    </tr>
     <tr>
         <td class="c0" valign="top"><label for="param1"><?php echo get_string('fieldoptions', 'data'); ?></label></td>
         <td class="c1"><textarea class="optionstextarea" name="param1" id="param1" cols="80" rows="10"><?php if($this->field->param1) {p($this->field->param1);} ?></textarea></td>

diff --git a/mod/data/field/picture/field.class.php b/mod/data/field/picture/field.class.php
@@ -40,6 +40,10 @@ function display_add_field($recordid = 0, $formdata = null) {
         if ($formdata) {
             $fieldname = 'field_' . $this->field->id . '_file';
             $itemid = $formdata->$fieldname;
+            $fieldname = 'field_' . $this->field->id . '_alttext';
+            if (isset($formdata->$fieldname)) {
+                $alttext = $formdata->$fieldname;
+            }
         } else if ($recordid) {
             if ($content = $DB->get_record('data_content', array('fieldid'=>$this->field->id, 'recordid'=>$recordid))) {
                 file_prepare_draft_area($itemid, $this->context->id, 'mod_data', 'content', $content->id);
@@ -67,13 +71,17 @@ function display_add_field($recordid = 0, $formdata = null) {
         } else {
             $itemid = file_get_unused_draft_itemid();
         }
-        $str = '';
+        $str = '<div title="' . s($this->field->description) . '">';
+        $str .= '<fieldset><legend><span class="accesshide">'.$this->field->name;
+
         if ($this->field->required) {
-            $str .= '<div title="' . get_string('requiredfieldhint', 'data', s($this->field->description)) . '">';
+            $str .= '&nbsp;' . get_string('requiredelement', 'form') . '</span></legend>';
+            $image = html_writer::img($OUTPUT->pix_url('req'), get_string('requiredelement', 'form'),
+                                      array('class' => 'req', 'title' => get_string('requiredelement', 'form')));
+            $str .= html_writer::div($image);
         } else {
-            $str .= '<div title="' . s($this->field->description) . '">';
+            $str .= '</span></legend>';
         }
-        $str .= '<fieldset><legend><span class="accesshide">'.$this->field->name.'</span></legend>';
         $str .= '<noscript>';
         if ($file) {
             $src = file_encode_url($CFG->wwwroot.'/pluginfile.php/', $this->context->id.'/mod_data/content/'.$content->id.'/'.$file->get_filename());
@@ -103,9 +111,6 @@ function display_add_field($recordid = 0, $formdata = null) {
         $str .= '<input type="hidden" name="field_'.$this->field->id.'_file" value="'.$itemid.'" />';
         $str .= '<label for="field_'.$this->field->id.'_alttext">'.get_string('alttext','data') .'</label>&nbsp;<input type="text" name="field_'
                 .$this->field->id.'_alttext" id="field_'.$this->field->id.'_alttext" value="'.s($alttext).'" />';
-        if ($this->field->required) {
-            $str .= '<span class="requiredfield">' . get_string('requiredfieldshort', 'data') . '</span>';
-        }
         $str .= '</div>';
 
         $str .= '</fieldset>';
@@ -311,12 +316,12 @@ function file_ok($path) {
     function notemptyfield($value, $name) {
         global $USER;
 
-        $names = explode('_',$name);
+        $names = explode('_', $name);
         if ($names[2] == 'file') {
             $usercontext = context_user::instance($USER->id);
             $fs = get_file_storage();
             $files = $fs->get_area_files($usercontext->id, 'user', 'draft', $value);
-            return count($files)>=2;
+            return count($files) >= 2;
         }
         return false;
     }