Uploaded image for project: 'Moodle'
  1. Moodle
  2. MDL-38659

Multiple mistakes in feedback element in environment.xml files

    Details

    • Testing Instructions:
      Hide

      master:

      1) Go to admin->server->environment and verify all checks are shown ok.

      2) Edit admin/environment.xml and change any level = "required" test to have an <ON_CHECK> feedback instead of the correct <ON_ERROR>. The environment page must show "Error reading environment data (16)" for that setting.

      3) Edit admin/environment.xml and change any level = "optional" test to have an <ON_ERROR> feedback instead of the correct <ON_CHECK>. The environment page must show "Error reading environment data (17)" for that setting.

      stable branches:

      1) Go to admin->server->environment and verify all checks are shown ok (for current and future versions).

      Show
      master: 1) Go to admin->server->environment and verify all checks are shown ok. 2) Edit admin/environment.xml and change any level = "required" test to have an <ON_CHECK> feedback instead of the correct <ON_ERROR>. The environment page must show "Error reading environment data (16)" for that setting. 3) Edit admin/environment.xml and change any level = "optional" test to have an <ON_ERROR> feedback instead of the correct <ON_CHECK>. The environment page must show "Error reading environment data (17)" for that setting. stable branches: 1) Go to admin->server->environment and verify all checks are shown ok (for current and future versions).
    • Affected Branches:
      MOODLE_23_STABLE, MOODLE_24_STABLE, MOODLE_25_STABLE
    • Fixed Branches:
      MOODLE_23_STABLE, MOODLE_24_STABLE
    • Pull from Repository:
    • Pull Master Branch:

      Description

      While implementing MDL-37033 I've detected that a lot of <FEEDBACK> elements in the environment.xml are wrong.

      This is not critical, because the test is performed ok and the status is correct, but some interesting messages aren't ever shown.

      Basically, this must be observed always:

      • For "required" level tests, only ON_ERROR and ON_OK messages are allowed (for fails and passes respectively).
      • For "optional" level tests, only ON_CHECK and ON_OK messages are allowed (for fails and passes respectively).

      And there are a bunch of tests not observing those rules.

      So I'm going to create a dtd to be able to, easily verify the xml file for correctness (master only, obviously, although changes to the xml will land to all supported versions).

      Ciao

        Gliffy Diagrams

          Attachments

            Issue Links

              Activity

              Hide
              stronk7 Eloy Lafuente (stronk7) added a comment -

              This os going to be built on top of MDL-37033, so adding dependency.

              Show
              stronk7 Eloy Lafuente (stronk7) added a comment - This os going to be built on top of MDL-37033 , so adding dependency.
              Hide
              stronk7 Eloy Lafuente (stronk7) added a comment -

              Sending to integration.

              Note master has 1 extra commit implementing, in environmentlib.php the detection of incorrect level/feedback combinations.

              The other commit is common for all branches and it's the fix of all the current incorrect feedbacks since 2.0.

              Important: This must be integrated after MDL-37033 (is built on top of it).

              Ciao

              Show
              stronk7 Eloy Lafuente (stronk7) added a comment - Sending to integration. Note master has 1 extra commit implementing, in environmentlib.php the detection of incorrect level/feedback combinations. The other commit is common for all branches and it's the fix of all the current incorrect feedbacks since 2.0. Important: This must be integrated after MDL-37033 (is built on top of it). Ciao
              Hide
              nebgor Aparup Banerjee added a comment -

              looks like MDL-37033 is integrated

              Show
              nebgor Aparup Banerjee added a comment - looks like MDL-37033 is integrated
              Hide
              nebgor Aparup Banerjee added a comment -

              oh something has conflicted within admin/environment.xml with integration's one

              Show
              nebgor Aparup Banerjee added a comment - oh something has conflicted within admin/environment.xml with integration's one
              Hide
              stronk7 Eloy Lafuente (stronk7) added a comment -

              grrr, indentation changed somehow between my commit in MDL-37033 and this. Fixing...

              Show
              stronk7 Eloy Lafuente (stronk7) added a comment - grrr, indentation changed somehow between my commit in MDL-37033 and this. Fixing...
              Hide
              stronk7 Eloy Lafuente (stronk7) added a comment -

              but i remember i verified...

              https://github.com/stronk7/moodle/compare/MDL-37033...MDL-38659 works... uhm.

              anyway, nvm!

              Show
              stronk7 Eloy Lafuente (stronk7) added a comment - but i remember i verified... https://github.com/stronk7/moodle/compare/MDL-37033...MDL-38659 works... uhm. anyway, nvm!
              Hide
              stronk7 Eloy Lafuente (stronk7) added a comment -

              aha, Damyon added one commit to each branch, fixing the indentation, hence the conflict here.

              Oki, I've amended the branches and now they should apply clean.

              Ciao

              Show
              stronk7 Eloy Lafuente (stronk7) added a comment - aha, Damyon added one commit to each branch, fixing the indentation, hence the conflict here. Oki, I've amended the branches and now they should apply clean. Ciao
              Hide
              nebgor Aparup Banerjee added a comment - - edited

              Thanks, no wonder i couldn't spot the conflict bah!

              thats been integrated into master.

              ps: i did wonder why some with level="required" ones (json, pcre, hash etc) don't have a message at all but that's not the point of this patch exactly
              pps: http://download.moodle.org/environment/environment.zip gets updated upon major release? (not sure :-D)

              Show
              nebgor Aparup Banerjee added a comment - - edited Thanks, no wonder i couldn't spot the conflict bah! thats been integrated into master. ps: i did wonder why some with level="required" ones (json, pcre, hash etc) don't have a message at all but that's not the point of this patch exactly pps: http://download.moodle.org/environment/environment.zip gets updated upon major release? (not sure :-D)
              Hide
              dmonllao David Monllaó added a comment -

              It passes, tested in master, 24, 23 and 22

              Show
              dmonllao David Monllaó added a comment - It passes, tested in master, 24, 23 and 22
              Hide
              damyon Damyon Wiese added a comment -

              Thanks for your hard work. This issue has been integrated upstream and is now available via git (and in some hours, via mirrors and downloads).

              Show
              damyon Damyon Wiese added a comment - Thanks for your hard work. This issue has been integrated upstream and is now available via git (and in some hours, via mirrors and downloads).
              Hide
              stronk7 Eloy Lafuente (stronk7) added a comment -

              Re, Aparup:

              1) The feedback section is optional and is used when some clarification is needed to the error or warning. Else just "error/warn" (red/yellow) is shown. So it's a decision of the developer if the feedback is used or no. Hence there are some elements missing it. Surely we should be more uniform, but, hehe, that's another story.

              2) Yes, environment information should be refreshed @ download automatically, so people running old versions of Moodle not including the patch will access to it via component download. The same applies to timezones information. In fact that should have happened already (because we always "publish" the version from master). I'll take a look next week, surely we were fetching that from cvs and need some modification to be done from git repos.

              Ciao

              Show
              stronk7 Eloy Lafuente (stronk7) added a comment - Re, Aparup: 1) The feedback section is optional and is used when some clarification is needed to the error or warning. Else just "error/warn" (red/yellow) is shown. So it's a decision of the developer if the feedback is used or no. Hence there are some elements missing it. Surely we should be more uniform, but, hehe, that's another story. 2) Yes, environment information should be refreshed @ download automatically, so people running old versions of Moodle not including the patch will access to it via component download. The same applies to timezones information. In fact that should have happened already (because we always "publish" the version from master). I'll take a look next week, surely we were fetching that from cvs and need some modification to be done from git repos. Ciao
              Hide
              stronk7 Eloy Lafuente (stronk7) added a comment -

              aha, it was working ok, it just happens 0h-36h after master has landed to moodle.git
              (same for TZs)

              Show
              stronk7 Eloy Lafuente (stronk7) added a comment - aha, it was working ok, it just happens 0h-36h after master has landed to moodle.git (same for TZs)
              Hide
              nebgor Aparup Banerjee added a comment -

              \o/ awesome you are

              Show
              nebgor Aparup Banerjee added a comment - \o/ awesome you are

                People

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

                  Dates

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