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 2.4 Branch:
    • Pull Master Branch:
    • Rank:
      48706

      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

        Issue Links

          Activity

          Hide
          Eloy Lafuente (stronk7) added a comment -

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

          Show
          Eloy Lafuente (stronk7) added a comment - This os going to be built on top of MDL-37033 , so adding dependency.
          Hide
          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
          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
          Aparup Banerjee added a comment -

          looks like MDL-37033 is integrated

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

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

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

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

          Show
          Eloy Lafuente (stronk7) added a comment - grrr, indentation changed somehow between my commit in MDL-37033 and this. Fixing...
          Hide
          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
          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
          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
          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
          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
          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
          David Monllaó added a comment -

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

          Show
          David Monllaó added a comment - It passes, tested in master, 24, 23 and 22
          Hide
          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 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
          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
          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
          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
          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
          Aparup Banerjee added a comment -

          \o/ awesome you are

          Show
          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: