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

Entries required before viewing data doesn't work

    Details

    • Type: Bug
    • Status: Closed
    • Priority: Critical
    • Resolution: Fixed
    • Affects Version/s: 1.8.7, 1.9.3
    • Fix Version/s: 1.8.8, 1.9.4
    • Labels:
      None
    • Affected Branches:
      MOODLE_18_STABLE, MOODLE_19_STABLE
    • Fixed Branches:
      MOODLE_18_STABLE, MOODLE_19_STABLE

      Description

      Entries required before viewing doesn't work.

      Student can see other students entries without creating any entry.

        Gliffy Diagrams

        1. MDL-16999_1.8_second.patch
          8 kB
          Jérôme Mouneyrac
        2. MDL-16999_1.8.patch
          8 kB
          Jérôme Mouneyrac
        3. MDL-16999_1.9_second.patch
          8 kB
          Jérôme Mouneyrac
        4. MDL-16999_1.9.patch
          8 kB
          Jérôme Mouneyrac
        5. MDL-16999_2.0.patch
          8 kB
          Jérôme Mouneyrac
        6. MDL-16999.xls
          100 kB
          Jérôme Mouneyrac
        1. databasewarningmessage.png
          213 kB

          Issue Links

            Activity

            Hide
            stronk7 Eloy Lafuente (stronk7) added a comment -

            Assigning to Jerome and addressing to 1.9.4. Thanks!

            Show
            stronk7 Eloy Lafuente (stronk7) added a comment - Assigning to Jerome and addressing to 1.9.4. Thanks!
            Hide
            jerome Jérôme Mouneyrac added a comment -

            No problem, I have a look to this
            About Missing Test Steps: http://docs.moodle.org/en/How_to_add_steps_to_reproduce_to_a_bug_report

            Show
            jerome Jérôme Mouneyrac added a comment - No problem, I have a look to this About Missing Test Steps: http://docs.moodle.org/en/How_to_add_steps_to_reproduce_to_a_bug_report
            Hide
            jerome Jérôme Mouneyrac added a comment -

            I confirm something is not consistent. after talking with Helen and Martin, here is the expected behavior:

            'Required Entries': display a specific warning message till the student add the required number of entries. The student still see others entries. it's a kind of reminder.
            'Required Entries Before Viewing': display a specific warning message till the student add the required number of entries. The student doesn't see others entries till he add the required number of entries.

            Show
            jerome Jérôme Mouneyrac added a comment - I confirm something is not consistent. after talking with Helen and Martin, here is the expected behavior: 'Required Entries': display a specific warning message till the student add the required number of entries. The student still see others entries. it's a kind of reminder. 'Required Entries Before Viewing': display a specific warning message till the student add the required number of entries. The student doesn't see others entries till he add the required number of entries.
            Hide
            jerome Jérôme Mouneyrac added a comment -

            Also need to be fixed:

            Show
            jerome Jérôme Mouneyrac added a comment - Also need to be fixed: help message for these two options wiki documentation: http://docs.moodle.org/en/Adding/editing_a_database
            Hide
            jerome Jérôme Mouneyrac added a comment -

            I attached a patch fixing it

            Show
            jerome Jérôme Mouneyrac added a comment - I attached a patch fixing it
            Hide
            jerome Jérôme Mouneyrac added a comment -

            Here a little table about the situation:

            1.74 1.75 1.76 Current Patch (CVS Moodle Version)
            Entries Required X ND ND ND D
            Entries Required Before Viewing ND ND X X ND

            ------------
            Legend:
            ------------
            D = Entries of other participants are displayed
            ND = Entries of other participants are not displayed
            X = The functionality is not been implemented in view.php

            => when we'll apply the patch, we need to upgrade the database. We'll copy the Entries Required number ("requiredentries" database table) into the Entries Required Before Viewing ("requiredentriestoview"), and set the Entries Required number to 0.

            Show
            jerome Jérôme Mouneyrac added a comment - Here a little table about the situation: 1.74 1.75 1.76 Current Patch (CVS Moodle Version) Entries Required X ND ND ND D Entries Required Before Viewing ND ND X X ND ------------ Legend: ------------ D = Entries of other participants are displayed ND = Entries of other participants are not displayed X = The functionality is not been implemented in view.php => when we'll apply the patch, we need to upgrade the database. We'll copy the Entries Required number ("requiredentries" database table) into the Entries Required Before Viewing ("requiredentriestoview"), and set the Entries Required number to 0.
            Hide
            jerome Jérôme Mouneyrac added a comment -

            I did a search on Moodle code, this change will only impact this part of code into data_document.php:

            //minimum records to view check
            // trap if too few records
            // TODO : report a potential capability lack of : mod/data:viewhiddenentries
            $recordsAmount = count_records('data_records', 'dataid', $data->id);
            if ($data->requiredentriestoview > $recordsAmount && !isteacher($data->course) && !has_capability('mod/data:manageentries', $context)) {
            if (!empty($CFG->search_access_debug)) echo "search reject : not enough records to view ";
            return false;
            }

            It would imply that this search will not display the same result after the patch is apply. However this bit of code is included into data_check_text_access() function, function that is called nowhere...

            Show
            jerome Jérôme Mouneyrac added a comment - I did a search on Moodle code, this change will only impact this part of code into data_document.php: //minimum records to view check // trap if too few records // TODO : report a potential capability lack of : mod/data:viewhiddenentries $recordsAmount = count_records('data_records', 'dataid', $data->id); if ($data->requiredentriestoview > $recordsAmount && !isteacher($data->course) && !has_capability('mod/data:manageentries', $context)) { if (!empty($CFG->search_access_debug)) echo "search reject : not enough records to view "; return false; } It would imply that this search will not display the same result after the patch is apply. However this bit of code is included into data_check_text_access() function, function that is called nowhere...
            Hide
            jerome Jérôme Mouneyrac added a comment -

            I attached the upgrade patch for the database

            Show
            jerome Jérôme Mouneyrac added a comment - I attached the upgrade patch for the database
            Hide
            jerome Jérôme Mouneyrac added a comment -

            Hi Eloy,
            I assign this issue to you for reviewing. It's not really complex but I do a change on the database table that will impact everybody using database, so I prefer to have a check on this one. Once you're ok, assign it back to me I'll commit these patchs. Thank you.

            Show
            jerome Jérôme Mouneyrac added a comment - Hi Eloy, I assign this issue to you for reviewing. It's not really complex but I do a change on the database table that will impact everybody using database, so I prefer to have a check on this one. Once you're ok, assign it back to me I'll commit these patchs. Thank you.
            Hide
            stronk7 Eloy Lafuente (stronk7) added a comment -

            Hi Jerome,

            first of all, I must confess that I don't understand your "table" above at all :-P, so I've gone to code and here there are my conclusions. Please, correct me if I'm wrong.

            1) Right now, requiredentries shows and incorrect message, telling that you won't be able to view other entries, while it should only display a "reminder" showing how many entries are lefting to complete the activity.
            2) The requiredentriestoview isn't checked at all.

            Based in those premises your patch to module seems correct: You've fixed the messages to be correct and have added the requiredentriestoview check. Perfect BUT one small detail... I'd leave the "entrieslefttoadd" unmodified and will create a new "entrieslefttoaddremind" for the requiredentries check. This because old sites (not upgraded) will continue having old approach and lang packs cannot change the meaning of strings.

            And then the upgrade bits... this is the part I don't understand at all. Why do you want to move all the contents of requiredentries to requiredentriestoview? Because you're asuming that everybody was using requiredentries as requiredentriestoview? What about sites having both properly (separately) defined and haven't detected this bug? You'll be resetting requiredentries for all sites and I'm not sure if that's correct. Isn't enough to fix functionality without changing DB? Also... doing that update in 19_STABLE sounds a bit risky... because.. how do you merge that to HEAD? If the execution is duplicated in HEAD... everybody will end with both fields set to 0 !!!

            Ciao

            Show
            stronk7 Eloy Lafuente (stronk7) added a comment - Hi Jerome, first of all, I must confess that I don't understand your "table" above at all :-P, so I've gone to code and here there are my conclusions. Please, correct me if I'm wrong. 1) Right now, requiredentries shows and incorrect message, telling that you won't be able to view other entries, while it should only display a "reminder" showing how many entries are lefting to complete the activity. 2) The requiredentriestoview isn't checked at all. Based in those premises your patch to module seems correct: You've fixed the messages to be correct and have added the requiredentriestoview check. Perfect BUT one small detail... I'd leave the "entrieslefttoadd" unmodified and will create a new "entrieslefttoaddremind" for the requiredentries check. This because old sites (not upgraded) will continue having old approach and lang packs cannot change the meaning of strings. And then the upgrade bits... this is the part I don't understand at all. Why do you want to move all the contents of requiredentries to requiredentriestoview? Because you're asuming that everybody was using requiredentries as requiredentriestoview? What about sites having both properly (separately) defined and haven't detected this bug? You'll be resetting requiredentries for all sites and I'm not sure if that's correct. Isn't enough to fix functionality without changing DB? Also... doing that update in 19_STABLE sounds a bit risky... because.. how do you merge that to HEAD? If the execution is duplicated in HEAD... everybody will end with both fields set to 0 !!! Ciao
            Hide
            jerome Jérôme Mouneyrac added a comment - - edited

            Hi Eloy,
            You understood well the problem.

            About the upgrade, here is the functionality history:
            In the version 1.74 "Entries Required" didn't exist. Only "Entries Required Before Viewing" existed and was working fine.
            In the version 1.75 "Entries Required" was created but had exactly the same behavior.
            In the version 1.76 "Entries Required Before Viewing" functionality process has been removed (I think because it didn't make any sens to have twice the same function). However the functionality was still available on the user interface. It just didn't do anything.
            This is still the same in the current version.
            The patch does what you describe in your comment.

            So if ever I apply the patch it is going to create a problem as people would have been using "Entries Required" instead of "Entries Required Before Viewing". If you apply the patch without changing the database values, then users will see entries of other participants when the administrator probably set them up to not to be shown (because "Entries Required" works currently as "Entries Required Before Viewing").

            In conclusion:
            If I commit the database patch people upgrading from 1.75 and before will lost their settings
            If I don't commit the database patch people upgrading from 1.76 and after will also need to change their settings

            I could make a patch testing the Moodle version... However note that any people with the 1.75 version will be a problem because we can not guess which functionality they used. So the best way would be to have during Moodle upgrade a special config, when at least one of these two fields is set.

            Show
            jerome Jérôme Mouneyrac added a comment - - edited Hi Eloy, You understood well the problem. About the upgrade, here is the functionality history: In the version 1.74 "Entries Required" didn't exist. Only "Entries Required Before Viewing" existed and was working fine. In the version 1.75 "Entries Required" was created but had exactly the same behavior. In the version 1.76 "Entries Required Before Viewing" functionality process has been removed (I think because it didn't make any sens to have twice the same function). However the functionality was still available on the user interface. It just didn't do anything. This is still the same in the current version. The patch does what you describe in your comment. So if ever I apply the patch it is going to create a problem as people would have been using "Entries Required" instead of "Entries Required Before Viewing". If you apply the patch without changing the database values, then users will see entries of other participants when the administrator probably set them up to not to be shown (because "Entries Required" works currently as "Entries Required Before Viewing"). In conclusion: If I commit the database patch people upgrading from 1.75 and before will lost their settings If I don't commit the database patch people upgrading from 1.76 and after will also need to change their settings I could make a patch testing the Moodle version... However note that any people with the 1.75 version will be a problem because we can not guess which functionality they used. So the best way would be to have during Moodle upgrade a special config, when at least one of these two fields is set.
            Hide
            jerome Jérôme Mouneyrac added a comment -

            I attached a Matrix where you can see the history of this functionality

            Show
            jerome Jérôme Mouneyrac added a comment - I attached a Matrix where you can see the history of this functionality
            Hide
            jerome Jérôme Mouneyrac added a comment -

            Note when I'm speaking about 1.75, I'm speaking about the CVS version of the view.php file. I'm not speaking about the Moodle version.

            After having a better look to the user interface history, and the database structure history, the two fields have always been existing. See the attached excel file.

            In order to fix database values, we could check the CVS version in the top line in the view.php file:
            1. if version <= 1.75 do not change database
            2. if version >= 1.76 swap the two fields values and set "Required Entries" to 0
            3. Need to alert people using the 1.75 version that they need to check their settings.

            Show
            jerome Jérôme Mouneyrac added a comment - Note when I'm speaking about 1.75, I'm speaking about the CVS version of the view.php file. I'm not speaking about the Moodle version. After having a better look to the user interface history, and the database structure history, the two fields have always been existing. See the attached excel file. In order to fix database values, we could check the CVS version in the top line in the view.php file: 1. if version <= 1.75 do not change database 2. if version >= 1.76 swap the two fields values and set "Required Entries" to 0 3. Need to alert people using the 1.75 version that they need to check their settings.
            Hide
            stronk7 Eloy Lafuente (stronk7) added a comment - - edited

            Well,

            I don't think we can really compute the best settings based in file version (1.75, 1.76...). Because it's current status (and you don't know when that status was acquired). Is possible that someone running 1.76 now has been running 1.75 for months and just updated 1 week ago, for example. And all sort of combinations.

            So, definitively: -1 to make cvs version based checks IMO.

            I think that we should, simply:

            1) Apply the module patch. This way both settings will be working ok in the future.
            2) Comment in the database module forum.
            3) In upgrade, query DB to see if there were activities using only one of the settings. Those are the sites that will need review. Warn in upgrade (perhaps showing list).

            That way, functionality will be working ok in all sites. And sites will have a chance to review their configuration in some activities.

            I cannot imagine another way to compute the different versions mesh

            Show
            stronk7 Eloy Lafuente (stronk7) added a comment - - edited Well, I don't think we can really compute the best settings based in file version (1.75, 1.76...). Because it's current status (and you don't know when that status was acquired). Is possible that someone running 1.76 now has been running 1.75 for months and just updated 1 week ago, for example. And all sort of combinations. So, definitively: -1 to make cvs version based checks IMO. I think that we should, simply: 1) Apply the module patch. This way both settings will be working ok in the future. 2) Comment in the database module forum. 3) In upgrade, query DB to see if there were activities using only one of the settings. Those are the sites that will need review. Warn in upgrade (perhaps showing list). That way, functionality will be working ok in all sites. And sites will have a chance to review their configuration in some activities. I cannot imagine another way to compute the different versions mesh
            Hide
            dougiamas Martin Dougiamas added a comment -

            +1 for what Eloy said!

            Show
            dougiamas Martin Dougiamas added a comment - +1 for what Eloy said!
            Hide
            jerome Jérôme Mouneyrac added a comment -

            No problem Eloy, I'll follow these instructions

            Show
            jerome Jérôme Mouneyrac added a comment - No problem Eloy, I'll follow these instructions
            Hide
            jerome Jérôme Mouneyrac added a comment -

            I attached a screenshot of the message displayed during the environment check

            Show
            jerome Jérôme Mouneyrac added a comment - I attached a screenshot of the message displayed during the environment check
            Hide
            jerome Jérôme Mouneyrac added a comment -

            I updated the warning text with Tim's help.

            Show
            jerome Jérôme Mouneyrac added a comment - I updated the warning text with Tim's help.
            Hide
            jerome Jérôme Mouneyrac added a comment -

            I attached a patch generating this screenshot during upgrade

            Show
            jerome Jérôme Mouneyrac added a comment - I attached a patch generating this screenshot during upgrade
            Hide
            jerome Jérôme Mouneyrac added a comment -

            I posted a comment into the database module forum: http://moodle.org/mod/forum/discuss.php?d=110928

            Show
            jerome Jérôme Mouneyrac added a comment - I posted a comment into the database module forum: http://moodle.org/mod/forum/discuss.php?d=110928
            Hide
            jerome Jérôme Mouneyrac added a comment -

            Eloy, I'll commit tomorrow, I'll let you one day to -1 me

            Show
            jerome Jérôme Mouneyrac added a comment - Eloy, I'll commit tomorrow, I'll let you one day to -1 me
            Hide
            tsala Helen Foster added a comment -

            http://docs.moodle.org/en/Adding/editing_a_database updated to reflect settings after the fix.

            Jerome, is it only 1.9.3 which is affected by this bug?

            Show
            tsala Helen Foster added a comment - http://docs.moodle.org/en/Adding/editing_a_database updated to reflect settings after the fix. Jerome, is it only 1.9.3 which is affected by this bug?
            Hide
            jerome Jérôme Mouneyrac added a comment -

            Hi Helen, thank you for noticing it, I forgot to mention it: the fix will be applied to 1.9 and 1.8.

            Show
            jerome Jérôme Mouneyrac added a comment - Hi Helen, thank you for noticing it, I forgot to mention it: the fix will be applied to 1.9 and 1.8.
            Hide
            jerome Jérôme Mouneyrac added a comment -

            I attached three new patches for 1.8, 1.9 and 2.0. They contains all change (fix + display alert once only during upgrade only)

            Show
            jerome Jérôme Mouneyrac added a comment - I attached three new patches for 1.8, 1.9 and 2.0. They contains all change (fix + display alert once only during upgrade only)
            Hide
            jerome Jérôme Mouneyrac added a comment -

            Fix and warning committed in 1.8, 1.9 and 2.0
            The fix has an impact on any Moodle site using database module with "Required Entries" settings. Please test carefully.
            Thanks

            Show
            jerome Jérôme Mouneyrac added a comment - Fix and warning committed in 1.8, 1.9 and 2.0 The fix has an impact on any Moodle site using database module with "Required Entries" settings. Please test carefully. Thanks
            Hide
            skodak Petr Skoda added a comment -

            1/ please do not use FROM {$CFG->prefix}data instead use

            {data}

            in HEAD

            2/ does this affect visibility of entries in recent activity block and related code?

            Show
            skodak Petr Skoda added a comment - 1/ please do not use FROM {$CFG->prefix}data instead use {data} in HEAD 2/ does this affect visibility of entries in recent activity block and related code?
            Hide
            jerome Jérôme Mouneyrac added a comment -

            I don't think there is an impact on recent activity block, the user will just see "Updated database".
            I'll fix the bad SQL, thanks Petr.

            Show
            jerome Jérôme Mouneyrac added a comment - I don't think there is an impact on recent activity block, the user will just see "Updated database". I'll fix the bad SQL, thanks Petr.
            Hide
            jerome Jérôme Mouneyrac added a comment -

            fix code in HEAD (retested)

            Show
            jerome Jérôme Mouneyrac added a comment - fix code in HEAD (retested)
            Hide
            stronk7 Eloy Lafuente (stronk7) added a comment -

            Hi Jerome... little comment, yesterday we agrred about to store "requiredentriesfixflag" flag in config_plugins table and it seems that you are storing it in central config table instead.

            That means 1 more record will be read from DB in each request...so I'ld suggest to change that to use config_plugins table instead by using:

            get_config('data', 'requiredentriesfixflag')
            set_config('requiredentriesfixflag, 1, 'data')

            Just to save 1 extra-record, yup, but...

            Ciao

            Show
            stronk7 Eloy Lafuente (stronk7) added a comment - Hi Jerome... little comment, yesterday we agrred about to store "requiredentriesfixflag" flag in config_plugins table and it seems that you are storing it in central config table instead. That means 1 more record will be read from DB in each request...so I'ld suggest to change that to use config_plugins table instead by using: get_config('data', 'requiredentriesfixflag') set_config('requiredentriesfixflag, 1, 'data') Just to save 1 extra-record, yup, but... Ciao
            Hide
            jerome Jérôme Mouneyrac added a comment -

            No problem, it's fixed. All versions (1.8, 1.9 and 2.0) now use config_plugins table.

            Show
            jerome Jérôme Mouneyrac added a comment - No problem, it's fixed. All versions (1.8, 1.9 and 2.0) now use config_plugins table.
            Hide
            howardsmiller Howard Miller added a comment -

            I just upgraded a 1.9 site to latest CVS and got hit with this warning. There's no help attached, so what does it mean? Do I have a problem, or is this bug not fixed? Any help appreciated. Let me know if you want more info.

            Show
            howardsmiller Howard Miller added a comment - I just upgraded a 1.9 site to latest CVS and got hit with this warning. There's no help attached, so what does it mean? Do I have a problem, or is this bug not fixed? Any help appreciated. Let me know if you want more info.
            Hide
            howardsmiller Howard Miller added a comment - - edited

            In fact I'm reopening it. I don't understand what's going on here at all However, you can't generate a "mystery" message like this on the Environment page with no information about how to fix it!! So even if it's right it's still wrong

            admin/environment/custom check/check required entries fields
            From MoodleDocs
            This page does not exist yet. You are welcome to create it.

            Show
            howardsmiller Howard Miller added a comment - - edited In fact I'm reopening it. I don't understand what's going on here at all However, you can't generate a "mystery" message like this on the Environment page with no information about how to fix it!! So even if it's right it's still wrong admin/environment/custom check/check required entries fields From MoodleDocs This page does not exist yet. You are welcome to create it.
            Hide
            howardsmiller Howard Miller added a comment -

            Ahh... that's interesting, when I refreshed the page the check had gone. Is that it then - the check actually fixes the problem and you don't really have to worry? If so, that's different to the way everything else works on this page and will confuse people.

            Show
            howardsmiller Howard Miller added a comment - Ahh... that's interesting, when I refreshed the page the check had gone. Is that it then - the check actually fixes the problem and you don't really have to worry? If so, that's different to the way everything else works on this page and will confuse people.
            Hide
            jerome Jérôme Mouneyrac added a comment -

            Hi Howard, thanks for your feedback. I'll update the warning message. However we need to alert all people once during the upgrade that they need to check their database settings. This page sounds a good place to do.

            Show
            jerome Jérôme Mouneyrac added a comment - Hi Howard, thanks for your feedback. I'll update the warning message. However we need to alert all people once during the upgrade that they need to check their database settings. This page sounds a good place to do.
            Hide
            jerome Jérôme Mouneyrac added a comment -

            I attached a new screen shot of the warning message.

            Howard:
            the Moodledoc link looked working. Anyway I updated the text as well.

            As you already updated your Moodle, If you wanna retest, you will need to:

            • remove the database entry "requiredentriesfixflag" from config_plugins table
            • change for a same higher version number into your version.php and into the function mod/data/lib.php/check_required_entries_fields()
            • do not process the upgrade entirely, and revert the version number (so you don't break futur moodle upgrade)
            Show
            jerome Jérôme Mouneyrac added a comment - I attached a new screen shot of the warning message. Howard: the Moodledoc link looked working. Anyway I updated the text as well. As you already updated your Moodle, If you wanna retest, you will need to: remove the database entry "requiredentriesfixflag" from config_plugins table change for a same higher version number into your version.php and into the function mod/data/lib.php/check_required_entries_fields() do not process the upgrade entirely, and revert the version number (so you don't break futur moodle upgrade)
            Hide
            jerome Jérôme Mouneyrac added a comment -

            Warning message update committed.

            Show
            jerome Jérôme Mouneyrac added a comment - Warning message update committed.
            Hide
            jerome Jérôme Mouneyrac added a comment -

            Howard let me know what is not clear, I'll explain here and I'll add some information on the forum. Thank you.

            Show
            jerome Jérôme Mouneyrac added a comment - Howard let me know what is not clear, I'll explain here and I'll add some information on the forum. Thank you.
            Hide
            howardsmiller Howard Miller added a comment -

            I'd seen the yellow box in your screen shot above. If you are suggesting that I should have got that then I didn't - I just got the check message.

            Show
            howardsmiller Howard Miller added a comment - I'd seen the yellow box in your screen shot above. If you are suggesting that I should have got that then I didn't - I just got the check message.
            Hide
            howardsmiller Howard Miller added a comment -

            I'm guessing that the docs information should be here....

            http://docs.moodle.org/en/admin/environment/custom_check/check_required_entries_fields

            It's empty though. It would be better if somebody who understood this problem (i.e. not me) put something in there.

            Show
            howardsmiller Howard Miller added a comment - I'm guessing that the docs information should be here.... http://docs.moodle.org/en/admin/environment/custom_check/check_required_entries_fields It's empty though. It would be better if somebody who understood this problem (i.e. not me) put something in there.
            Hide
            jerome Jérôme Mouneyrac added a comment -

            Hi Howard,
            it is not normal if you saw a check message without a yellow box. Which Moodle version are you using?

            I didn't notice the automatic link to the Moodledocs (probably too obvious :o), I'm adding something to this page. Thank you for your feedback.

            Show
            jerome Jérôme Mouneyrac added a comment - Hi Howard, it is not normal if you saw a check message without a yellow box. Which Moodle version are you using? I didn't notice the automatic link to the Moodledocs (probably too obvious :o), I'm adding something to this page. Thank you for your feedback.
            Hide
            skodak Petr Skoda added a comment - - edited

            This was not a nice solution:
            1/ see regression in MDL-17405
            2/ Data mod string should not pollute admin lang file
            3/ environment test can not change database
            4/ upgrade check can not be executed repeatedly in this case - the warning is there only during the first execution
            5/ required entries tests should be in SQL, there is no need to loop all databases
            6/ the $result->setFeedbackStr should be done outside of for loop
            7/ 3rd party mods can not use environment.xml, why should Data mod be allowed to do that?

            I have created a new issue MDL-17427 which should help in similar cases when we need to inform admins about upgrade related issues.

            Show
            skodak Petr Skoda added a comment - - edited This was not a nice solution: 1/ see regression in MDL-17405 2/ Data mod string should not pollute admin lang file 3/ environment test can not change database 4/ upgrade check can not be executed repeatedly in this case - the warning is there only during the first execution 5/ required entries tests should be in SQL, there is no need to loop all databases 6/ the $result->setFeedbackStr should be done outside of for loop 7/ 3rd party mods can not use environment.xml, why should Data mod be allowed to do that? I have created a new issue MDL-17427 which should help in similar cases when we need to inform admins about upgrade related issues.
            Hide
            stronk7 Eloy Lafuente (stronk7) added a comment -

            Yes, really... talking with Petr... and re-reading this... I don't get the point where we switched from upgrade (original proposal) to environmental check.

            Agree about environment not being the best place for this sort of stuff. Bad abuse IMO. Let's rethink the thing.

            Ciao

            Show
            stronk7 Eloy Lafuente (stronk7) added a comment - Yes, really... talking with Petr... and re-reading this... I don't get the point where we switched from upgrade (original proposal) to environmental check. Agree about environment not being the best place for this sort of stuff. Bad abuse IMO. Let's rethink the thing. Ciao
            Hide
            jerome Jérôme Mouneyrac added a comment - - edited

            After talking with Tim we though it was a good location for checking as there were similar check done for Question.
            I did check installation process on 1.9 to see if it was still working, and expected wrongly it would work on 2.0...
            I'm having another look at all that...

            Show
            jerome Jérôme Mouneyrac added a comment - - edited After talking with Tim we though it was a good location for checking as there were similar check done for Question. I did check installation process on 1.9 to see if it was still working, and expected wrongly it would work on 2.0... I'm having another look at all that...
            Hide
            skodak Petr Skoda added a comment -

            the quiz and questions is a different problem, ideally questions should know nothing about quiz. So verifying questions in environment.xml might be considered core stuff == ok, but I do not think anything related to activities including quiz should be done there.

            Show
            skodak Petr Skoda added a comment - the quiz and questions is a different problem, ideally questions should know nothing about quiz. So verifying questions in environment.xml might be considered core stuff == ok, but I do not think anything related to activities including quiz should be done there.
            Hide
            jerome Jérôme Mouneyrac added a comment -

            No problem Petr, I'll move it to upgrade as Eloy suggested I'll also fix what can be fixed in your list. The warning should not be displayed more than once I think.

            Show
            jerome Jérôme Mouneyrac added a comment - No problem Petr, I'll move it to upgrade as Eloy suggested I'll also fix what can be fixed in your list. The warning should not be displayed more than once I think.
            Hide
            tsala Helen Foster added a comment -

            After re-reading everyone's comments and also http://docs.moodle.org/en/admin/environment/custom_check/check_required_entries_fields I'm finding it difficult to understand why this issue has to be so complicated.

            Why isn't it just a simple case of fixing the 'Required entries' and 'Entries required before viewing' so that they behave the way described in the help files?

            Sorry I don't understand why admins should need to check database activity settings after upgrading. Sorry if I'm missing something really obvious, but the paragraph in http://docs.moodle.org/en/admin/environment/custom_check/check_required_entries_fields describing the worst that could happen if people don't check their database activities settings doesn't sound bad to me at all!

            Show
            tsala Helen Foster added a comment - After re-reading everyone's comments and also http://docs.moodle.org/en/admin/environment/custom_check/check_required_entries_fields I'm finding it difficult to understand why this issue has to be so complicated. Why isn't it just a simple case of fixing the 'Required entries' and 'Entries required before viewing' so that they behave the way described in the help files? Sorry I don't understand why admins should need to check database activity settings after upgrading. Sorry if I'm missing something really obvious, but the paragraph in http://docs.moodle.org/en/admin/environment/custom_check/check_required_entries_fields describing the worst that could happen if people don't check their database activities settings doesn't sound bad to me at all!
            Hide
            skodak Petr Skoda added a comment -

            jerome: please have a look at MDL-17405 I already did some changes in HEAD

            Show
            skodak Petr Skoda added a comment - jerome: please have a look at MDL-17405 I already did some changes in HEAD
            Hide
            jerome Jérôme Mouneyrac added a comment -

            Helen: a teacher could have created some entries that he only wants to be seen by students once they submit few entries. After upgrade, the students will see these previously "restricted" entries.
            Petr: I have a look.

            Show
            jerome Jérôme Mouneyrac added a comment - Helen: a teacher could have created some entries that he only wants to be seen by students once they submit few entries. After upgrade, the students will see these previously "restricted" entries. Petr: I have a look.
            Hide
            jerome Jérôme Mouneyrac added a comment -

            Petr: in the comment 28/Nov/08 07:21 AM - 4/ do you suggest to display the warning message many time?
            PS: I attached a beta patch (above question needs to be clearly defined) and screenshot

            Show
            jerome Jérôme Mouneyrac added a comment - Petr: in the comment 28/Nov/08 07:21 AM - 4/ do you suggest to display the warning message many time? PS: I attached a beta patch (above question needs to be clearly defined) and screenshot
            Hide
            jerome Jérôme Mouneyrac added a comment -

            I attached a patch that I'm going to commit on 1.9 in order to move the warning message into upgrade pages. This warning message will not be displayed if the administrator did previously upgrade. However the administrator will process to an "empty" upgrade.

            Show
            jerome Jérôme Mouneyrac added a comment - I attached a patch that I'm going to commit on 1.9 in order to move the warning message into upgrade pages. This warning message will not be displayed if the administrator did previously upgrade. However the administrator will process to an "empty" upgrade.
            Hide
            jerome Jérôme Mouneyrac added a comment -

            I attached a patch for 1.8 as well

            Show
            jerome Jérôme Mouneyrac added a comment - I attached a patch for 1.8 as well
            Hide
            jerome Jérôme Mouneyrac added a comment -

            Petr, I'm leaving in few minutes for Europe, so I assign this issue to you. I don't want to commit it without being reviewed in 1.9 and 1.8 Stable. The two patches to review and commit are:
            MDL-16999_1.8_second.patch
            MDL-16999_1.9_second.patch
            Thank you.

            Show
            jerome Jérôme Mouneyrac added a comment - Petr, I'm leaving in few minutes for Europe, so I assign this issue to you. I don't want to commit it without being reviewed in 1.9 and 1.8 Stable. The two patches to review and commit are: MDL-16999 _1.8_second.patch MDL-16999 _1.9_second.patch Thank you.
            Hide
            jerome Jérôme Mouneyrac added a comment -

            I moved the warning message from the environment check page to a normal upgrade page

            Nicolas Martignoni: you have to change the French language pack again (the string has been moved from admin.php to data.php), thanks

            Show
            jerome Jérôme Mouneyrac added a comment - I moved the warning message from the environment check page to a normal upgrade page Nicolas Martignoni: you have to change the French language pack again (the string has been moved from admin.php to data.php), thanks

              People

              • Votes:
                3 Vote for this issue
                Watchers:
                11 Start watching this issue

                Dates

                • Created:
                  Updated:
                  Resolved:
                  Fix Release Date:
                  28/Jan/09