Moodle
  1. Moodle
  2. MDL-5583

allow teachers to specify certain fields in database activity as required

    Details

    • Testing Instructions:
      Hide

      Note, all parts of the test need to be completed. Behat is currently unable to fill in some of the database fields we use (e.g. Checkbox).

      1. Run behat tests
      2. Create a new Database activity
      3. Add required fields for:
        1. checkbox (with one option only)
        2. checkbox (with multiple options)
        3. file
        4. latlong
        5. menu
        6. number
        7. picture
        8. radio
        9. text
        10. textarea
        11. url
        12. multimenu (with one option only)
        13. multimenu (with multiple options)
      4. Create templates
      5. Go to add a new entry
      6. Check that only the checkboxes and multimenus with multiple options have "You must select at least one." displayed. The others should just have the required * icon
      7. Fill in all fields
      8. Save and view
      9. Confirm that the entry was added
      10. Add required fields for:
        1. checkbox
        2. file
        3. latlong
        4. menu
        5. number
        6. picture
        7. radio
        8. text
        9. textarea
        10. url
        11. multimenu
      11. Add another entry
      12. Don't fill in any of the fields
      13. Ensure that an error message appears next to all of the required fields, but not any of the others.
      14. Delete all of the required fields
      15. Add another entry
      16. Don't fill in any of the fields
      17. Confirm that the entry was added
      18. Backup the activity
      19. Restore the activity
      20. Confirm that the required state for your fields was copied across
      Show
      Note, all parts of the test need to be completed. Behat is currently unable to fill in some of the database fields we use (e.g. Checkbox). Run behat tests Create a new Database activity Add required fields for: checkbox (with one option only) checkbox (with multiple options) file latlong menu number picture radio text textarea url multimenu (with one option only) multimenu (with multiple options) Create templates Go to add a new entry Check that only the checkboxes and multimenus with multiple options have "You must select at least one." displayed. The others should just have the required * icon Fill in all fields Save and view Confirm that the entry was added Add required fields for: checkbox file latlong menu number picture radio text textarea url multimenu Add another entry Don't fill in any of the fields Ensure that an error message appears next to all of the required fields, but not any of the others. Delete all of the required fields Add another entry Don't fill in any of the fields Confirm that the entry was added Backup the activity Restore the activity Confirm that the required state for your fields was copied across
    • Affected Branches:
      MOODLE_16_STABLE, MOODLE_17_STABLE, MOODLE_18_STABLE, MOODLE_19_STABLE, MOODLE_22_STABLE, MOODLE_28_STABLE
    • Pull from Repository:
    • Pull Master Branch:
      MDL-5583-master
    • Story Points (Obsolete):
      20
    • Sprint:
      FRONTEND Sprint 14, Team A Sprint 1, Team '; drop tables Sprint 2, Team ';drop tables Sprint 3, Team ';drop tables Sprint 4
    • Issue size:
      Large

      Description

      When creating a database, allow the teacher to specify whether a field must be filled in or not.

        Gliffy Diagrams

          Issue Links

            Activity

            Hide
            Laurent Domenech added a comment -

            This zip file contains the files that I modified to fix this, as well as entry MDL-6821

            Show
            Laurent Domenech added a comment - This zip file contains the files that I modified to fix this, as well as entry MDL-6821
            Hide
            Laurent Domenech added a comment -

            This file contains the fixes (for just this issue) made and tested on the latest 1.7.

            There is a cosmetic issue with the checkbox itself. It is centered on the Edit Field form. For some fields, it is not a problem, but for the fields with options, it will not be aligned with the other options. Aligning all options requires deeper changes in the code that I didn't feel confortable doing at this stage.

            Show
            Laurent Domenech added a comment - This file contains the fixes (for just this issue) made and tested on the latest 1.7. There is a cosmetic issue with the checkbox itself. It is centered on the Edit Field form. For some fields, it is not a problem, but for the fields with options, it will not be aligned with the other options. Aligning all options requires deeper changes in the code that I didn't feel confortable doing at this stage.
            Hide
            Anthony Borrow added a comment -

            Adding the ability to set a database activity field as required would be consistent behavior with other places in Moodle where such functionality is common place (user fields, etc.). I think this requires adding a required field to the database. One of the param fields could be used but I would not recommend it as they would be different depending on the field types. Peace - Anthony

            Show
            Anthony Borrow added a comment - Adding the ability to set a database activity field as required would be consistent behavior with other places in Moodle where such functionality is common place (user fields, etc.). I think this requires adding a required field to the database. One of the param fields could be used but I would not recommend it as they would be different depending on the field types. Peace - Anthony
            Hide
            Anthony Borrow added a comment -

            adding later versions to keep this issue from falling through the cracks

            Show
            Anthony Borrow added a comment - adding later versions to keep this issue from falling through the cracks
            Hide
            Sophia N added a comment -

            affects 2.0 as well.

            Show
            Sophia N added a comment - affects 2.0 as well.
            Hide
            Helen Foster added a comment -

            Thanks Sophia, adding the latest Moodle version as an affected version to keep this issue in the spotlight.

            Show
            Helen Foster added a comment - Thanks Sophia, adding the latest Moodle version as an affected version to keep this issue in the spotlight.
            Hide
            Nadav Kavalerchik added a comment -

            Any news regarding an implementation of this feature in Moodle 2.x ?

            Show
            Nadav Kavalerchik added a comment - Any news regarding an implementation of this feature in Moodle 2.x ?
            Hide
            Helen Foster added a comment -

            Hi Nadav,

            I haven't heard anything however I suggest we encourage more people to vote for it as the 2.4 roadmap includes 'many of your most-voted fixes from the tracker' http://docs.moodle.org/dev/Roadmap

            Show
            Helen Foster added a comment - Hi Nadav, I haven't heard anything however I suggest we encourage more people to vote for it as the 2.4 roadmap includes 'many of your most-voted fixes from the tracker' http://docs.moodle.org/dev/Roadmap
            Hide
            Damyon Wiese added a comment -

            I have implemented this for a client and am posting the patch here for master.

            This patch depends on MDL-32401

            Show
            Damyon Wiese added a comment - I have implemented this for a client and am posting the patch here for master. This patch depends on MDL-32401
            Hide
            Andrew Nicols added a comment -

            Updated the patch from Damyon, removing the dependency and rebasing on master. The patch now complies with coding standards.

            I've pushed but haven't written behat or testing instructions yet.

            Show
            Andrew Nicols added a comment - Updated the patch from Damyon, removing the dependency and rebasing on master. The patch now complies with coding standards. I've pushed but haven't written behat or testing instructions yet.
            Hide
            CiBoT added a comment -

            Fails against automated checks.

            Checked MDL-5583 using repository: git://github.com/andrewnicols/moodle.git

            More information about this report

            Show
            CiBoT added a comment - Fails against automated checks. Checked MDL-5583 using repository: git://github.com/andrewnicols/moodle.git master (branch: MDL-5583-master | CI Job ) Coding style problems found More information about this report
            Hide
            Sam Hemelryk added a comment -

            Hi Andrew,

            Thanks for reviving this.

            I've been looking at the code and have made the following notes:

            1. db/install.xml The comment, precision, and use of UNSIGNED all need to be fixed. The install file version should also to be bumped as XMLDB editor would
            2. db/upgrade.php Again XMLDB code should be used here, there are a couple of problems with that upgrade block.
            3. lang/en/data.php
              1. There are two identical strings being added fieldrequired and requiredfield
              2. I couldn't see where the new 'required' string was being used.
              3. The requiredentries string has been added but already exists (so its there twice now).
              4. It appears requiredfieldhint is always concatenated, if so we should make the additional content a variable I think.
            4. lib.php
              1. data_process_submission several uses of \stdClass, that preceeding slash is not allowed (not in a namespaced area)
              2. data_process_submission:3774 phpdoc needs to be updated.
              3. data_process_submission:3775 unused global $DB
              4. data_process_submission:3808 $submitteddata[$fieldid] is used as an array without being defined as one.
              5. data_process_submission:3816 unused variable $fieldname
              6. data_process_submission:3820 That has to be wrong? $submitteddata is an array of arrays, there is a level missing in the logic in this code block by the looks. Additionally we could break from that foreach by the looks.
            5. edit.php
              1. This script now more heavily relies on $rid which is not validated before being fetched, there should be better validation. I know its not directly a problem introduced by these changes but perhaps it could be quickly fixed now.
              2. 185 should be using MUST_EXIST when fetching $rid and either joining to database id or validating earlier as noted above.
            6. The date, multimenu fields can't be marked required?
            7. Extending the behat tests to cover a successful adding of an entry with required fields would also be great.

            When running behat tests there was also an error:

            Warning: Creating default object from empty value in /var/www/integration/mod/data/edit.php on line 181

            Cheers
            Sam

            Show
            Sam Hemelryk added a comment - Hi Andrew, Thanks for reviving this. I've been looking at the code and have made the following notes: db/install.xml The comment, precision, and use of UNSIGNED all need to be fixed. The install file version should also to be bumped as XMLDB editor would db/upgrade.php Again XMLDB code should be used here, there are a couple of problems with that upgrade block. lang/en/data.php There are two identical strings being added fieldrequired and requiredfield I couldn't see where the new 'required' string was being used. The requiredentries string has been added but already exists (so its there twice now). It appears requiredfieldhint is always concatenated, if so we should make the additional content a variable I think. lib.php data_process_submission several uses of \stdClass, that preceeding slash is not allowed (not in a namespaced area) data_process_submission:3774 phpdoc needs to be updated. data_process_submission:3775 unused global $DB data_process_submission:3808 $submitteddata [$fieldid] is used as an array without being defined as one. data_process_submission:3816 unused variable $fieldname data_process_submission:3820 That has to be wrong? $submitteddata is an array of arrays, there is a level missing in the logic in this code block by the looks. Additionally we could break from that foreach by the looks. edit.php This script now more heavily relies on $rid which is not validated before being fetched, there should be better validation. I know its not directly a problem introduced by these changes but perhaps it could be quickly fixed now. 185 should be using MUST_EXIST when fetching $rid and either joining to database id or validating earlier as noted above. The date, multimenu fields can't be marked required? Extending the behat tests to cover a successful adding of an entry with required fields would also be great. When running behat tests there was also an error: Warning: Creating default object from empty value in /var/www/integration/mod/data/edit.php on line 181 Cheers Sam
            Hide
            Helen Foster added a comment -

            Hi Andrew,

            Just wondering whether we really need a help pop-up for requiredfield, or whether the name 'Required field' describes things sufficiently?

            Comparing with elsewhere in Moodle, the feedback activity has a required field setting and it doesn't have a help pop-up.

            Show
            Helen Foster added a comment - Hi Andrew, Just wondering whether we really need a help pop-up for requiredfield, or whether the name 'Required field' describes things sufficiently? Comparing with elsewhere in Moodle, the feedback activity has a required field setting and it doesn't have a help pop-up.
            Hide
            Andrew Nicols added a comment -

            1) fixed
            2) fixed
            3-1) fixed
            3-2) fixed
            3-3) fixed
            3-4) no idea what this means
            4-1) fixed - didn't know that this was a rule, and don't understand why it is
            4-2) updated
            4-3) removed
            4-4) it is defined as one... a few lines above
            4-5) removed
            4-6) that foreach is looping over the subarray already... it discards the key because we don't really need it; but it is looping through all elements of both dimensions. Not sure what you mean about breaking either.
            5-1) Added
            5-2) Removed
            6) Nope... as Damyon said in the original, sometimes non-entry is required.
            7) I tried... tried... tried. Behat cannot cope with these forms.

            Helen: I removed that - good point

            Show
            Andrew Nicols added a comment - 1) fixed 2) fixed 3-1) fixed 3-2) fixed 3-3) fixed 3-4) no idea what this means 4-1) fixed - didn't know that this was a rule, and don't understand why it is 4-2) updated 4-3) removed 4-4) it is defined as one... a few lines above 4-5) removed 4-6) that foreach is looping over the subarray already... it discards the key because we don't really need it; but it is looping through all elements of both dimensions. Not sure what you mean about breaking either. 5-1) Added 5-2) Removed 6) Nope... as Damyon said in the original, sometimes non-entry is required. 7) I tried... tried... tried. Behat cannot cope with these forms. Helen: I removed that - good point
            Hide
            CiBoT added a comment -

            Fails against automated checks.

            Checked MDL-5583 using repository: git://github.com/andrewnicols/moodle.git

            More information about this report

            Show
            CiBoT added a comment - Fails against automated checks. Checked MDL-5583 using repository: git://github.com/andrewnicols/moodle.git master (branch: MDL-5583-master | CI Job ) Coding style problems found More information about this report
            Hide
            Damyon Wiese added a comment -

            I guess I can peer review this - it's been long enough

            Show
            Damyon Wiese added a comment - I guess I can peer review this - it's been long enough
            Hide
            Damyon Wiese added a comment -

            I think Sams 3-4 comment means to make the lang string like this:
            $string['requiredfieldhint'] = '{$a} (required field)';

            so the translator can control exactly how the strings are combined.

            Show
            Damyon Wiese added a comment - I think Sams 3-4 comment means to make the lang string like this: $string ['requiredfieldhint'] = '{$a} (required field)'; so the translator can control exactly how the strings are combined.
            Hide
            Damyon Wiese added a comment -

            I don't think we need this one:
            $str .= '<span>foo</span>';

            I get this error when editing an existing item (this was a single checkbox field)
            Notice: Undefined property: stdClass::$id in /home/damyonw/Documents/Moodle/integration/master/moodle/mod/data/tabs.php on line 40

            And this error when i go to save my edits:
            Coding error detected, it must be fixed by a programmer: moodle_database::update_record_raw() id field must be specified.

            I tried to edit an entry with an image field and got this:
            Debug info: ERROR: invalid input syntax for integer: ""
            SELECT f.id AS id, f.contenthash, f.pathnamehash, f.contextid, f.component, f.filearea, f.itemid, f.filepath, f.filename, f.userid, f.filesize, f.mimetype, f.status, f.source, f.author, f.license, f.timecreated, f.timemodified, f.sortorder, f.referencefileid, r.repositoryid AS repositoryid, r.reference AS reference, r.lastsync AS referencelastsync
            FROM mdl26_files f
            LEFT JOIN mdl26_files_reference r
            ON f.referencefileid = r.id
            WHERE f.contextid = $1
            AND f.component = $2
            AND f.filearea = $3
            AND f.itemid = $4 ORDER BY itemid, filepath, filename
            [array (
            0 => 5,
            1 => 'user',
            2 => 'draft',
            3 => '',
            )]
            Error code: dmlreadexception

            So there seems to be some regressions with editing existing entries.

            The code changes looks well done - but it is hard to follow exactly - just because of the code around it.

            that's enough for tonight /review

            Thanks for picking this up again Andrew.

            Show
            Damyon Wiese added a comment - I don't think we need this one: $str .= '<span>foo</span>'; I get this error when editing an existing item (this was a single checkbox field) Notice: Undefined property: stdClass::$id in /home/damyonw/Documents/Moodle/integration/master/moodle/mod/data/tabs.php on line 40 And this error when i go to save my edits: Coding error detected, it must be fixed by a programmer: moodle_database::update_record_raw() id field must be specified. I tried to edit an entry with an image field and got this: Debug info: ERROR: invalid input syntax for integer: "" SELECT f.id AS id, f.contenthash, f.pathnamehash, f.contextid, f.component, f.filearea, f.itemid, f.filepath, f.filename, f.userid, f.filesize, f.mimetype, f.status, f.source, f.author, f.license, f.timecreated, f.timemodified, f.sortorder, f.referencefileid, r.repositoryid AS repositoryid, r.reference AS reference, r.lastsync AS referencelastsync FROM mdl26_files f LEFT JOIN mdl26_files_reference r ON f.referencefileid = r.id WHERE f.contextid = $1 AND f.component = $2 AND f.filearea = $3 AND f.itemid = $4 ORDER BY itemid, filepath, filename [array ( 0 => 5, 1 => 'user', 2 => 'draft', 3 => '', )] Error code: dmlreadexception So there seems to be some regressions with editing existing entries. The code changes looks well done - but it is hard to follow exactly - just because of the code around it. that's enough for tonight /review Thanks for picking this up again Andrew.
            Hide
            Andrew Nicols added a comment -

            Thanks Damyon,

            I think I've fixed all of that. Just running through tests now. I'd inadvertently put the MUST_EXIST in the wrong position for the get_record.

            I'll put it up for peer review after I've tested editing existing entries, etc.

            Show
            Andrew Nicols added a comment - Thanks Damyon, I think I've fixed all of that. Just running through tests now. I'd inadvertently put the MUST_EXIST in the wrong position for the get_record. I'll put it up for peer review after I've tested editing existing entries, etc.
            Hide
            Andrew Nicols added a comment -

            FIxed all that - up for peer review again since it sounds like you have more in mind.

            Show
            Andrew Nicols added a comment - FIxed all that - up for peer review again since it sounds like you have more in mind.
            Hide
            Damyon Wiese added a comment -

            If those are all now fixed I think this is ready for integration.

            Thanks - pushing the button...

            Show
            Damyon Wiese added a comment - If those are all now fixed I think this is ready for integration. Thanks - pushing the button...
            Hide
            Dan Poltawski added a comment -

            The main moodle.git repository has just been updated with latest weekly modifications. You may wish to rebase your PULL branches to simplify history and avoid any possible merge conflicts. This would also make integrator's life easier next week.

            TIA and ciao

            Show
            Dan Poltawski added a comment - The main moodle.git repository has just been updated with latest weekly modifications. You may wish to rebase your PULL branches to simplify history and avoid any possible merge conflicts. This would also make integrator's life easier next week. TIA and ciao
            Hide
            CiBoT added a comment -

            Moving this issue to current integration cycle, will be reviewed soon. Thanks for the hard work!

            Show
            CiBoT added a comment - Moving this issue to current integration cycle, will be reviewed soon. Thanks for the hard work!
            Hide
            Sam Hemelryk added a comment -

            Backup and restore needs to be done still - should be pretty easy I imagine - will you have time to look at that in the next day or two Andrew or shall I reopen this?

            Show
            Sam Hemelryk added a comment - Backup and restore needs to be done still - should be pretty easy I imagine - will you have time to look at that in the next day or two Andrew or shall I reopen this?
            Hide
            Andrew Nicols added a comment -

            Just sorting it now Sam. Thanks for pointing out my foolish oversight.
            Pretty sure it's just a one-line fix, but I'm testing it now.

            Show
            Andrew Nicols added a comment - Just sorting it now Sam. Thanks for pointing out my foolish oversight. Pretty sure it's just a one-line fix, but I'm testing it now.
            Hide
            Andrew Nicols added a comment -

            Huzzah - fixed. As predicted, a one line change.
            Testing instructions updated accordingly.

            Show
            Andrew Nicols added a comment - Huzzah - fixed. As predicted, a one line change. Testing instructions updated accordingly.
            Hide
            Helen Foster added a comment -

            This issue now has its very own QA test: MDLQA-7165. Please shout if the test needs amending or expanding.

            Show
            Helen Foster added a comment - This issue now has its very own QA test: MDLQA-7165 . Please shout if the test needs amending or expanding.
            Hide
            Sam Hemelryk added a comment -

            Hi Andrew,

            I've been having a play with these changes this morning and I feel that the user interface still needs a bit of work sorry.
            I'm going to reopen it for this week and continue my review, once I've got full notes I'll share them with you and we'll come up with a plan for this.
            I would like to think that this can still land providing the interface woes can be dealt with.

            Cheers
            Sam

            Show
            Sam Hemelryk added a comment - Hi Andrew, I've been having a play with these changes this morning and I feel that the user interface still needs a bit of work sorry. I'm going to reopen it for this week and continue my review, once I've got full notes I'll share them with you and we'll come up with a plan for this. I would like to think that this can still land providing the interface woes can be dealt with. Cheers Sam
            Hide
            CiBoT added a comment -

            Moving this reopened issue out from current integration. Please, re-submit it for integration once ready.

            Show
            CiBoT added a comment - Moving this reopened issue out from current integration. Please, re-submit it for integration once ready.
            Hide
            Helen Foster added a comment -

            Just noting that the QA test MDLQA-7165 has temporarily been removed from MDLQA-1 as this feature is not yet integrated.

            Show
            Helen Foster added a comment - Just noting that the QA test MDLQA-7165 has temporarily been removed from MDLQA-1 as this feature is not yet integrated.
            Hide
            moodle.com added a comment -

            Hey Andrew, someone from the other team will pick this one up

            Show
            moodle.com added a comment - Hey Andrew, someone from the other team will pick this one up
            Hide
            John Okely added a comment - - edited

            Sam Hemelryk (long shot) did you happen to still have those notes?

            I rebased and got the branch going again, but I'll need some specific info on what needs improving in the interface

            cc Andrew Nicols, Damyon Wiese

            Show
            John Okely added a comment - - edited Sam Hemelryk (long shot) did you happen to still have those notes? I rebased and got the branch going again, but I'll need some specific info on what needs improving in the interface cc Andrew Nicols , Damyon Wiese
            Hide
            Sam Hemelryk added a comment -

            Haha nope sorry John, I can't recall what was there and while I have a backup of my chat logs and may have something in there I think the best bet would be to just reassess the user interface and determine what if anything needs doing.

            Show
            Sam Hemelryk added a comment - Haha nope sorry John, I can't recall what was there and while I have a backup of my chat logs and may have something in there I think the best bet would be to just reassess the user interface and determine what if anything needs doing.
            Hide
            John Okely added a comment -

            Thanks anyway Sam! Yep we'll do that

            Show
            John Okely added a comment - Thanks anyway Sam! Yep we'll do that
            Hide
            John Okely added a comment -

            Summary of remaining work:

            • Check the labels for all the field types with a screen reader – compare to mform
            • Add javascript validation, so when you tab to the next field the javascript error will pop up
            • Perhaps say "You must supply a value" instead
            • Use the asterisk image instead of the character *
            • Asterisk is on the right side at the end of each field input, which is inconsistent with the rest of moodle:
              • Fields with a label: Put the asterisk on the right of the label
              • Fields with a hidden label. We should put label in a div before the input and hide all the text except the "required image"
              • Radio buttons. We should auto-select the first item (then it's always going to have a value)
              • Checkboxes + Multimenu. Put a text instruction "At least one checkbox is required" in a div before the checkboxes.
            • Update behat tests
            Show
            John Okely added a comment - Summary of remaining work: Check the labels for all the field types with a screen reader – compare to mform Add javascript validation, so when you tab to the next field the javascript error will pop up Perhaps say "You must supply a value" instead Use the asterisk image instead of the character * Asterisk is on the right side at the end of each field input, which is inconsistent with the rest of moodle: Fields with a label: Put the asterisk on the right of the label Fields with a hidden label. We should put label in a div before the input and hide all the text except the "required image" Radio buttons. We should auto-select the first item (then it's always going to have a value) Checkboxes + Multimenu. Put a text instruction "At least one checkbox is required" in a div before the checkboxes. Update behat tests
            Hide
            CiBoT added a comment -

            Fails against automated checks.

            Checked MDL-5583 using repository: git://github.com/xow/moodle.git

            More information about this report

            Show
            CiBoT added a comment - Fails against automated checks. Checked MDL-5583 using repository: git://github.com/xow/moodle.git master (63 errors / 4 warnings) [branch: MDL-5583-master | CI Job ] phplint (0/0) , php (50/4) , js (0/0) , css (0/0) , phpdoc (11/0) , commit (2/0) , savepoint (0/0) , thirdparty (0/0) , More information about this report
            Hide
            CiBoT added a comment -

            Fails against automated checks.

            Checked MDL-5583 using repository: git://github.com/xow/moodle.git

            More information about this report

            Show
            CiBoT added a comment - Fails against automated checks. Checked MDL-5583 using repository: git://github.com/xow/moodle.git master (36 errors / 1 warnings) [branch: MDL-5583-master | CI Job ] phplint (0/0) , php (25/1) , js (0/0) , css (0/0) , phpdoc (11/0) , commit (0/0) , savepoint (0/0) , thirdparty (0/0) , More information about this report
            Hide
            CiBoT added a comment -

            Fails against automated checks.

            Checked MDL-5583 using repository: git://github.com/xow/moodle.git

            More information about this report

            Show
            CiBoT added a comment - Fails against automated checks. Checked MDL-5583 using repository: git://github.com/xow/moodle.git master (28 errors / 0 warnings) [branch: MDL-5583-master | CI Job ] phplint (0/0) , php (17/0) , js (0/0) , css (0/0) , phpdoc (11/0) , commit (0/0) , savepoint (0/0) , thirdparty (0/0) , More information about this report
            Hide
            John Okely added a comment -

            I'm putting this up for peer review. Let me know if I should keep or squash Damyons commit (which is partially Andrews work too).

            There are currently three problems remaining with the usability/interface:

            • Someone can satisfy the required check for the location by inserting a value into latitude but none into logitude or vice versa. However this will require a not insignificant refactor of how mod/data considers something empty in order to fix
            • Right now the teacher can add a required field that's a checkbox, menu or multimenu with no options available. Basically this means that students will not be able to submit anything because they can never fill in the required field. But this will also require significant changes to mod_data. And technically this is no different to when a teacher adds a field that has no items that isn't required.. what is the point of a field that will always be empty? So in talking with others we came to the conclusion that the teacher should no what they are doing and if not the students filling out the form will discover the problem pretty quickly.
            • Something else that I'd like to do but isn't technically part of this issue is hide all the legends (on checkbox, file, location, image, radio). They cause the required asterisks to appear too low, and also make horizontal rules appear in random places (since not every question type has one). But this is an existing mod_data UI issue, made more pronounced by this change.

            CIBot:

            • "Function display_add_field is not documented" If the parent has docblock and the meaning of the function and the arguments/return values are the same, then the child doesn't need any more docblock (link to docs about this)
            • "Phpdocs for function data_process_submission has incomplete parameters list" Not sure what's going on there, might be because of the extended explanation of the return type. Let me know if I should change it
            • "Visibility must be declared on method "notemptyfield" didn't add any of these, keeping consistent with code. Happy to change them if you think I should

            Thanks!

            Show
            John Okely added a comment - I'm putting this up for peer review. Let me know if I should keep or squash Damyons commit (which is partially Andrews work too). There are currently three problems remaining with the usability/interface: Someone can satisfy the required check for the location by inserting a value into latitude but none into logitude or vice versa. However this will require a not insignificant refactor of how mod/data considers something empty in order to fix Right now the teacher can add a required field that's a checkbox, menu or multimenu with no options available. Basically this means that students will not be able to submit anything because they can never fill in the required field. But this will also require significant changes to mod_data. And technically this is no different to when a teacher adds a field that has no items that isn't required.. what is the point of a field that will always be empty? So in talking with others we came to the conclusion that the teacher should no what they are doing and if not the students filling out the form will discover the problem pretty quickly. Something else that I'd like to do but isn't technically part of this issue is hide all the legends (on checkbox, file, location, image, radio). They cause the required asterisks to appear too low, and also make horizontal rules appear in random places (since not every question type has one). But this is an existing mod_data UI issue, made more pronounced by this change. CIBot: "Function display_add_field is not documented" If the parent has docblock and the meaning of the function and the arguments/return values are the same, then the child doesn't need any more docblock (link to docs about this) "Phpdocs for function data_process_submission has incomplete parameters list" Not sure what's going on there, might be because of the extended explanation of the return type. Let me know if I should change it "Visibility must be declared on method "notemptyfield" didn't add any of these, keeping consistent with code. Happy to change them if you think I should Thanks!
            Hide
            CiBoT added a comment -

            Fails against automated checks.

            Checked MDL-5583 using repository: git://github.com/xow/moodle.git

            More information about this report

            Show
            CiBoT added a comment - Fails against automated checks. Checked MDL-5583 using repository: git://github.com/xow/moodle.git master (28 errors / 0 warnings) [branch: MDL-5583-master | CI Job ] phplint (0/0) , php (17/0) , js (0/0) , css (0/0) , phpdoc (11/0) , commit (0/0) , savepoint (0/0) , thirdparty (0/0) , More information about this report

              People

              • Votes:
                46 Vote for this issue
                Watchers:
                26 Start watching this issue

                Dates

                • Created:
                  Updated:

                  Agile