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

          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