Details

    • Type: Sub-task Sub-task
    • Status: Closed
    • Priority: Minor 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:
    • Rank:
      46003

      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.

        Issue Links

          Activity

          Hide
          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
          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
          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
          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
          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
          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
          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
          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
          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
          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
          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
          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
          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
          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
          Tim Hunt added a comment -

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

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

          Thanks Colin, i've integrated this to master.

          Show
          Dan Poltawski added a comment - Thanks Colin, i've integrated this to master.
          Hide
          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
          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
          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
          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
          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
          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: