Moodle
  1. Moodle
  2. MDL-16999

Entries required before viewing data doesn't work

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Critical 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
    • Rank:
      30308

      Description

      Entries required before viewing doesn't work.

      Student can see other students entries without creating any entry.

      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
          Eloy Lafuente (stronk7) added a comment -

          Assigning to Jerome and addressing to 1.9.4. Thanks!

          Show
          Eloy Lafuente (stronk7) added a comment - Assigning to Jerome and addressing to 1.9.4. Thanks!
          Hide
          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
          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
          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
          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
          Jérôme Mouneyrac added a comment -

          Also need to be fixed:

          Show
          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
          Jérôme Mouneyrac added a comment -

          I attached a patch fixing it

          Show
          Jérôme Mouneyrac added a comment - I attached a patch fixing it
          Hide
          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
          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
          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
          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
          Jérôme Mouneyrac added a comment -

          I attached the upgrade patch for the database

          Show
          Jérôme Mouneyrac added a comment - I attached the upgrade patch for the database
          Hide
          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
          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
          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
          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
          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
          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
          Jérôme Mouneyrac added a comment -

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

          Show
          Jérôme Mouneyrac added a comment - I attached a Matrix where you can see the history of this functionality
          Hide
          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
          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
          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
          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
          Martin Dougiamas added a comment -

          +1 for what Eloy said!

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

          No problem Eloy, I'll follow these instructions

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

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

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

          I updated the warning text with Tim's help.

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

          I attached a patch generating this screenshot during upgrade

          Show
          Jérôme Mouneyrac added a comment - I attached a patch generating this screenshot during upgrade
          Hide
          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
          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
          Jérôme Mouneyrac added a comment -

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

          Show
          Jérôme Mouneyrac added a comment - Eloy, I'll commit tomorrow, I'll let you one day to -1 me
          Hide
          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
          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
          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
          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
          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
          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
          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
          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
          Petr Škoda 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
          Petr Škoda 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
          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
          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
          Jérôme Mouneyrac added a comment -

          fix code in HEAD (retested)

          Show
          Jérôme Mouneyrac added a comment - fix code in HEAD (retested)
          Hide
          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
          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
          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
          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
          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
          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
          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
          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
          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
          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
          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
          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
          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
          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
          Jérôme Mouneyrac added a comment -

          Warning message update committed.

          Show
          Jérôme Mouneyrac added a comment - Warning message update committed.
          Hide
          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
          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
          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
          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
          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
          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
          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
          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
          Petr Škoda 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
          Petr Škoda 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
          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
          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
          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
          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
          Petr Škoda 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
          Petr Škoda 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
          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
          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
          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
          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
          Petr Škoda added a comment -

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

          Show
          Petr Škoda added a comment - jerome: please have a look at MDL-17405 I already did some changes in HEAD
          Hide
          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
          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
          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
          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
          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
          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
          Jérôme Mouneyrac added a comment -

          I attached a patch for 1.8 as well

          Show
          Jérôme Mouneyrac added a comment - I attached a patch for 1.8 as well
          Hide
          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
          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
          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
          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: