|
[
Permalink
| « Hide
]
Eloy Lafuente (stronk7) added a comment - 25/Oct/08 01:56 AM
Assigning to Jerome and addressing to 1.9.4. Thanks!
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  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. Also need to be fixed:
I attached a patch fixing it
Here a little table about the situation:
1.74 1.75 1.76 Current Patch (CVS Moodle Version) ------------ => 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. 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 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... I attached the upgrade patch for the database
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. 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. 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 Hi Eloy,
You understood well the problem. About the upgrade, here is the functionality history: 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: 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. I attached a Matrix where you can see the history of this functionality
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: 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. 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 No problem Eloy, I'll follow these instructions
I attached a screenshot of the message displayed during the environment check
I updated the warning text with Tim's help.
I attached a patch generating this screenshot during upgrade
I posted a comment into the database module forum: http://moodle.org/mod/forum/discuss.php?d=110928
Eloy, I'll commit tomorrow, I'll let you one day to -1 me
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? Hi Helen, thank you for noticing it, I forgot to mention it: the fix will be applied to 1.9 and 1.8.
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)
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 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? 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. fix code in HEAD (retested)
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') Just to save 1 extra-record, yup, but... Ciao No problem, it's fixed. All versions (1.8, 1.9 and 2.0) now use config_plugins table.
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.
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 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.
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.
I attached a new screen shot of the warning message.
Howard: As you already updated your Moodle, If you wanna retest, you will need to:
Warning message update committed.
Howard let me know what is not clear, I'll explain here and I'll add some information on the forum. Thank you.
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.
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. 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. This was not a nice solution:
1/ see regression in 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 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 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... 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.
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. 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 jerome: please have a look at
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. 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 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.
I attached a patch for 1.8 as well
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 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Bug
Resolved
Critical
Entries required before viewing data doesn't work
 |
|
