Details

    • Type: Sub-task
    • Status: Closed
    • Priority: Minor
    • Resolution: Fixed
    • Affects Version/s: 2.4
    • Fix Version/s: 2.5
    • Component/s: Questions
    • Labels:
    • Testing Instructions:
      Hide

      Create and edit a range of numerical questions, then preview them to ensure they work. In particular test editing the unit options, including puttin in invalid data and ensuring it is rejected.

      Show
      Create and edit a range of numerical questions, then preview them to ensure they work. In particular test editing the unit options, including puttin in invalid data and ensuring it is rejected.
    • Affected Branches:
      MOODLE_24_STABLE
    • Fixed Branches:
      MOODLE_25_STABLE
    • Pull from Repository:
    • Pull Master Branch:

      Description

      The numerical question type has fields for each unit e.g. Unit 1 /Unit/Multiplier. Group these together so they all appear on one line.

        Gliffy Diagrams

          Issue Links

            Activity

            Hide
            colchambers Colin Chambers added a comment -

            To make the numerical questiontype easier to edit The fields for each unit option have now been grouped together to appear on one line.

            Show
            colchambers Colin Chambers added a comment - To make the numerical questiontype easier to edit The fields for each unit option have now been grouped together to appear on one line.
            Hide
            timhunt Tim Hunt added a comment -

            I think that the changes are basically right, but there are a few tedious technical issues with the code that we should get right before we submit this for integration:

            1. I think the way you have done the language strings will cause problems for translators. I think it would be better to make new strings like

            $string['acceptederrormustbenumeric'] = 'Accepted error must be a number.';

            (then delete the old strings is they are no longer used.)

            2. I think this change of whitespace is wrong: https://github.com/colchambers/moodle/commit/MDL-36534#L2L142 - or perhaps it is just that the way the diff looks is confusing.

            3. I don't understand the line if ($mform->elementExists('units[0]')) {. If it is necessary, I think you need to add a comment explaining. Oh. I see. That logic was there already (using multiplier[0]). I still don't understand why! Anyway, it would be nice to tidy up that chunk of code if possible.

            4. I'm not sure if $textboxgroup is the best variable name. $unitgroup might be better.

            5. qtype_numeric cannot depend on qtype_calculated, so https://github.com/colchambers/moodle/commit/MDL-36534#L2L348 is wrong. Oh! I see that again that is an existing problem.

            6. I think that the changes in question/type/calculatedmulti/edit_calculatedmulti_form.php are not really related to this bug, so I think it would be better if they were not part of this commit.

            I know that some of these are existing problems, but it would be nice if we could fix them now, and leave the code in a better state than we found it.

            Also, please can you write some testing instructions.

            Show
            timhunt Tim Hunt added a comment - I think that the changes are basically right, but there are a few tedious technical issues with the code that we should get right before we submit this for integration: 1. I think the way you have done the language strings will cause problems for translators. I think it would be better to make new strings like $string ['acceptederrormustbenumeric'] = 'Accepted error must be a number.'; (then delete the old strings is they are no longer used.) 2. I think this change of whitespace is wrong: https://github.com/colchambers/moodle/commit/MDL-36534#L2L142 - or perhaps it is just that the way the diff looks is confusing. 3. I don't understand the line if ($mform->elementExists('units [0] ')) {. If it is necessary, I think you need to add a comment explaining. Oh. I see. That logic was there already (using multiplier [0] ). I still don't understand why! Anyway, it would be nice to tidy up that chunk of code if possible. 4. I'm not sure if $textboxgroup is the best variable name. $unitgroup might be better. 5. qtype_numeric cannot depend on qtype_calculated, so https://github.com/colchambers/moodle/commit/MDL-36534#L2L348 is wrong. Oh! I see that again that is an existing problem. 6. I think that the changes in question/type/calculatedmulti/edit_calculatedmulti_form.php are not really related to this bug, so I think it would be better if they were not part of this commit. I know that some of these are existing problems, but it would be nice if we could fix them now, and leave the code in a better state than we found it. Also, please can you write some testing instructions.
            Hide
            timhunt Tim Hunt added a comment -

            Re 3. deep git archeology has found https://github.com/moodle/moodle/commit/26b266625bd74f9794e24d9cbfdffe76bb998240 which refers to MDL-15159. OK. so that if is still necessary, but we should add a comment referencing MDL-15159.

            Show
            timhunt Tim Hunt added a comment - Re 3. deep git archeology has found https://github.com/moodle/moodle/commit/26b266625bd74f9794e24d9cbfdffe76bb998240 which refers to MDL-15159 . OK. so that if is still necessary, but we should add a comment referencing MDL-15159 .
            Hide
            colchambers Colin Chambers added a comment -

            Thanks Tim. Rebased on latest master. Added comment re 3. Latest is at https://github.com/colchambers/moodle/compare/master...MDL-36534

            Would you mind addressing #1

            Show
            colchambers Colin Chambers added a comment - Thanks Tim. Rebased on latest master. Added comment re 3. Latest is at https://github.com/colchambers/moodle/compare/master...MDL-36534 Would you mind addressing #1
            Hide
            timhunt Tim Hunt added a comment -

            It turns out that the lang strings were a real mess. Now they are slighly less messy.

            Submitting for integration now.

            Show
            timhunt Tim Hunt added a comment - It turns out that the lang strings were a real mess. Now they are slighly less messy. Submitting for integration now.
            Hide
            stronk7 Eloy Lafuente (stronk7) 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
            stronk7 Eloy Lafuente (stronk7) 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
            colchambers Colin Chambers added a comment -

            Appears I didn't push my branch last week. Surprise!! and apologies. So I've just pulled latest and rebased then pushed. I can see the changes on https://github.com/colchambers/moodle/compare/master...MDL-36534

            HTH. Of course I won't have any lang changes from Tim.

            I only added the following comment to edit_numerical_form.php at line 160 anyway. So you may just want to copy it.
            // The following strange-looking if statement is to do with when the
            // form is used to move questions between categories. See MDL-15159.

            Show
            colchambers Colin Chambers added a comment - Appears I didn't push my branch last week. Surprise!! and apologies. So I've just pulled latest and rebased then pushed. I can see the changes on https://github.com/colchambers/moodle/compare/master...MDL-36534 HTH. Of course I won't have any lang changes from Tim. I only added the following comment to edit_numerical_form.php at line 160 anyway. So you may just want to copy it. // The following strange-looking if statement is to do with when the // form is used to move questions between categories. See MDL-15159 .
            Hide
            timhunt Tim Hunt added a comment -

            Integration branch updated to include the latest version of Colin's commit.

            Show
            timhunt Tim Hunt added a comment - Integration branch updated to include the latest version of Colin's commit.
            Hide
            poltawski Dan Poltawski added a comment -

            Thanks Colin, i've integrated this to master.

            Show
            poltawski Dan Poltawski added a comment - Thanks Colin, i've integrated this to master.
            Hide
            phalacee Jason Fowler added a comment -

            Even though this passed the testing instructions, I noticed a HUGE error with the "Units are optional" setting. I set up metric units for the answers, and when I entered 1mile as the answer, it was marked as write.

            There needs to be another option, "Units are optional, but must be one of those listed to be correct"

            Show
            phalacee Jason Fowler added a comment - Even though this passed the testing instructions, I noticed a HUGE error with the "Units are optional" setting. I set up metric units for the answers, and when I entered 1mile as the answer, it was marked as write. There needs to be another option, "Units are optional, but must be one of those listed to be correct"
            Hide
            timhunt Tim Hunt added a comment -

            Remarkably, that is expected behaviour for the "Units are optional" setting. It is bloody weird, but few people would use that setting for a new question. It is only there to be backwards compatible with the bloody weird way numerical questions worked up to and including Moodle 1.9.

            Show
            timhunt Tim Hunt added a comment - Remarkably, that is expected behaviour for the "Units are optional" setting. It is bloody weird, but few people would use that setting for a new question. It is only there to be backwards compatible with the bloody weird way numerical questions worked up to and including Moodle 1.9.
            Hide
            poltawski Dan Poltawski added a comment -

            Hurray! We did it! Thanks to all the reporters, testers, user and watchers for a bumper week of Moodling!

            Show
            poltawski Dan Poltawski added a comment - Hurray! We did it! Thanks to all the reporters, testers, user and watchers for a bumper week of Moodling!

              People

              • Votes:
                0 Vote for this issue
                Watchers:
                5 Start watching this issue

                Dates

                • Created:
                  Updated:
                  Resolved:
                  Fix Release Date:
                  14/May/13