Moodle
  1. Moodle
  2. MDL-36991

Conditional activity restrictions should be displayed as a list

    Details

    • Type: Improvement Improvement
    • Status: Closed
    • Priority: Minor Minor
    • Resolution: Fixed
    • Affects Version/s: 2.5
    • Fix Version/s: 2.5
    • Component/s: Conditional activities
    • Labels:
    • Testing Instructions:
      Hide

      A) create new course
      1. add a Page activity with two conditions: date set to the future and course grade set to more than 50%
      2. add another Page with only one condition: date set to the future
      3. add another one with only one condition (date set to the future), and this time, set it to not display at all until the condition is met.

      B) Look at the conditions in the editing teacher role, verify that:
      1. the first Page shows a bullet list of conditions.
      2. the second Page shows only a single condition (no list).
      3. the third Page also shows only a single condition and has the text 'Restricted (not visible)' or whatever that is to indicate that it isn't shown to students.

      C) Log in with a student account or switch role to Student
      1. the first Page should be a list of conditions
      2. the second Page should show only a single condition (no list).
      3. The third page should not show at all

      Note: the strings displayed to students and editing teachers will probably be slightly different, but the rendering of the conditional activity lists should be the same.

      Show
      A) create new course 1. add a Page activity with two conditions: date set to the future and course grade set to more than 50% 2. add another Page with only one condition: date set to the future 3. add another one with only one condition (date set to the future), and this time, set it to not display at all until the condition is met. B) Look at the conditions in the editing teacher role, verify that: 1. the first Page shows a bullet list of conditions. 2. the second Page shows only a single condition (no list). 3. the third Page also shows only a single condition and has the text 'Restricted (not visible)' or whatever that is to indicate that it isn't shown to students. C) Log in with a student account or switch role to Student 1. the first Page should be a list of conditions 2. the second Page should show only a single condition (no list). 3. The third page should not show at all Note: the strings displayed to students and editing teachers will probably be slightly different, but the rendering of the conditional activity lists should be the same.
    • Affected Branches:
      MOODLE_25_STABLE
    • Fixed Branches:
      MOODLE_25_STABLE
    • Pull Master Branch:
      MDL-36991-master
    • Rank:
      46527

      Description

      conditional activity restrictions are listed as a paragraph with several strong tags separating each activity. this should probably be converted to a list for readability and semantics. Here's an example of the code that's output:

      <div class="availabilityinfo ">Restricted: 'Not available until the activity <strong>Expert Domain Activity on Theology</strong> is marked complete. Not available until the activity <strong>Expert Domains (Theology) upload link</strong> is marked complete. Not available until the activity <strong>Father Zakaria Botros (forum)</strong> is marked complete. Not available until the activity <strong>Foundations of the Faith (Wiki)</strong> is marked complete.'</div>
      

      Something like this would be easier to understand:

      <div class="availabilityinfo">Restricted:  Not available until the activity/ies:
      <ul class="requiredactivities">
      <li>Expert Domain Activity on Theology</li>
      <li>Expert Domains (Theology) upload link</li>
      <li>Father Zakaria Botros (forum)</li>
      <li>Foundations of the Faith (Wiki)</li>
      </ul>
      is/are marked completed.
      </div>
      

        Issue Links

          Activity

          Hide
          Daniel Wahl added a comment -

          Attaching screenshot of the actual output listed in the description

          Show
          Daniel Wahl added a comment - Attaching screenshot of the actual output listed in the description
          Hide
          Michael de Raadt added a comment -

          Thanks for suggesting that.

          Feel free to continue working on the issue. If you can put forward a patch or Git branches, please feel free to do so.

          Show
          Michael de Raadt added a comment - Thanks for suggesting that. Feel free to continue working on the issue. If you can put forward a patch or Git branches, please feel free to do so.
          Hide
          Daniel Wahl added a comment -

          great I'm looking into it - but this is moderately beyond me, so I might end up pestering you a little bit!

          the key seems to lie in /lib/conditionlib.php public function get_full_information($modinfo=null) {}

          Show
          Daniel Wahl added a comment - great I'm looking into it - but this is moderately beyond me, so I might end up pestering you a little bit! the key seems to lie in /lib/conditionlib.php public function get_full_information($modinfo=null) {}
          Hide
          Daniel Wahl added a comment -

          Ok, here's my attempt at solving this: https://github.com/thedannywahl/moodle/compare/MDL-36991

          It works but could probably use the following changes:
          use html_writer::tag('li', $information); instead of 2 contatenations
          add some type of class or style to the list
          should it be an ordered list instead of unordered? Since that is the order that the teacher puts them in - but moodle might not require them to be done in that order. Basically I don't know enough about how conditional activities work.

          Show
          Daniel Wahl added a comment - Ok, here's my attempt at solving this: https://github.com/thedannywahl/moodle/compare/MDL-36991 It works but could probably use the following changes: use html_writer::tag('li', $information); instead of 2 contatenations add some type of class or style to the list should it be an ordered list instead of unordered? Since that is the order that the teacher puts them in - but moodle might not require them to be done in that order. Basically I don't know enough about how conditional activities work.
          Hide
          Sam Marshall added a comment -

          I've done a quick review...

          I agree this is a nice improvement and am happy to submit for integration once concerns are addressed.

          Code looks fine (one line somewhere has indent wrong, and should use html_writer as you mention, but otherwise). I don't think a class attribute is needed on the list as there is probably already a class for the parent so it can be themed if necessary? It might benefit from some style in a CSS file (or might not need this).

          Some possible issues:

          1. I think the display/strings are slightly different for teachers (with editing mode on) and students, did you check both cases?

          2. Is this an improvement for all usage? What about the most common case where an activity only has a single condition (e.g. future date) - it's now going to take up two lines, instead of one, right?

          I.e. unless I'm missing something you've made it better where it was stupid long before, such as your example screenshot, but worse for the normal case? Does it need some kind of special case where it builds an array of these things and only uses a list if there's more than one? I'm not sure about this.

          3. Not an expert on languages but I'm not sure about reusing the existing language string (where it used to have quotes in) - I think this is a semantic change, i.e. all the other languages will need to change their language strings as it will look stupid to have the quotes below, so I'm not sure using the same string is appropriate, it may be necessary to create a new name for these strings.

          4. Just checking, is the HTML (er this area of it) still valid afterwards? I'm not sure what tag the text is in, is it just a div so that you're OK to add a <ul> in there?

          5. There are unit tests in this area (I hope), they may depend on the output text, if so will need changing - once the change is finalised, please rerun unit tests to check.

          And yes unordered list is definitely right for this as the student doesn't have to do the requirements in order.

          Show
          Sam Marshall added a comment - I've done a quick review... I agree this is a nice improvement and am happy to submit for integration once concerns are addressed. Code looks fine (one line somewhere has indent wrong, and should use html_writer as you mention, but otherwise). I don't think a class attribute is needed on the list as there is probably already a class for the parent so it can be themed if necessary? It might benefit from some style in a CSS file (or might not need this). Some possible issues: 1. I think the display/strings are slightly different for teachers (with editing mode on) and students, did you check both cases? 2. Is this an improvement for all usage? What about the most common case where an activity only has a single condition (e.g. future date) - it's now going to take up two lines, instead of one, right? I.e. unless I'm missing something you've made it better where it was stupid long before, such as your example screenshot, but worse for the normal case? Does it need some kind of special case where it builds an array of these things and only uses a list if there's more than one? I'm not sure about this. 3. Not an expert on languages but I'm not sure about reusing the existing language string (where it used to have quotes in) - I think this is a semantic change, i.e. all the other languages will need to change their language strings as it will look stupid to have the quotes below, so I'm not sure using the same string is appropriate, it may be necessary to create a new name for these strings. 4. Just checking, is the HTML (er this area of it) still valid afterwards? I'm not sure what tag the text is in, is it just a div so that you're OK to add a <ul> in there? 5. There are unit tests in this area (I hope), they may depend on the output text, if so will need changing - once the change is finalised, please rerun unit tests to check. And yes unordered list is definitely right for this as the student doesn't have to do the requirements in order.
          Hide
          Daniel Wahl added a comment -

          Keeping a counter would be hard b/c you have 5 arrays that are evaluated with foreach() loops so I'd need to evaluate each one to see if the total count is only 1 and then not add the li tag, but if it is greater than one, then add the li tag. It would require a major rewrite of the function. I might be able to do that but it would take me a long time to figure out how to appropriately do it.

          I don't see anything at W3 for HTML4 or HTML5 Draft that says a list must contain more than 1 list-item, so while possibly silly looking, I don't think it's semantically incorrect:

          http://www.w3.org/TR/html401/struct/lists.html
          http://www.w3.org/TR/2012/WD-html5-20121025/the-li-element.html#the-li-element

          1) you're right - it's actually a different function. teachers see the output from get_full_information() and students see the output from is_available(): I have updated the repo with support for is_available()

          2) For students it's still just one line as there's no extra strings, just the conditions, though I agree a list of 1 might seem silly. Maybe style the list so it doesn't have points (e.g. none outside none)

          3) So create a new string, how about a naming convention? I have no idea what's involved in that.

          4) yup. It's a div with a ul inside for students and a div w/ text and a ul for teachers

          5) I'll need help with that one.

          Show
          Daniel Wahl added a comment - Keeping a counter would be hard b/c you have 5 arrays that are evaluated with foreach() loops so I'd need to evaluate each one to see if the total count is only 1 and then not add the li tag, but if it is greater than one, then add the li tag. It would require a major rewrite of the function. I might be able to do that but it would take me a long time to figure out how to appropriately do it. I don't see anything at W3 for HTML4 or HTML5 Draft that says a list must contain more than 1 list-item, so while possibly silly looking, I don't think it's semantically incorrect: http://www.w3.org/TR/html401/struct/lists.html http://www.w3.org/TR/2012/WD-html5-20121025/the-li-element.html#the-li-element 1) you're right - it's actually a different function. teachers see the output from get_full_information() and students see the output from is_available(): I have updated the repo with support for is_available() 2) For students it's still just one line as there's no extra strings, just the conditions, though I agree a list of 1 might seem silly. Maybe style the list so it doesn't have points (e.g. none outside none) 3) So create a new string, how about a naming convention? I have no idea what's involved in that. 4) yup. It's a div with a ul inside for students and a div w/ text and a ul for teachers 5) I'll need help with that one.
          Hide
          Sam Marshall added a comment -

          Thanks for your continued work on this.

          To clarify, I am absolutely fine with lists that only have one item, there's nothing wrong with that and it might even look better, it's just the extra line it takes up on screen. These conditions can already look like a kind of wall of text if you use them on many activities - especially for teachers. Doubling that would probably annoy people.

          Regarding keeping a counter, that doesn't seem like an easy way to do it, as you say. I haven't looked at the code again but there is probably an easier way to do it - work out the result as now, but before outputting it inside the <ul>, do a check on the string. I think the string will always start with <li> so if you do a strpos for <li> starting from position 4 or so (i.e. to skip the first one), then if this === false, you know there is only one <li> in the string, so you COULD do a preg_replace to strip out the starting and ending <li> </li> and output it directly without the <ul>....

          BUT maybe a better way of doing it would be to just add the class 'onerestriction' to the UL if there is only a single restriction. That way it would be possible to use CSS to make the <ul> and <li> display inline, effectively having the same display as before (for one restriction) while the HTML structure stays constant, i.e.

          .whatevertheparentclassis ul.onerestriction,
          .whatevertheparentclassis ul.onerestriction li

          { display: inline; list-style-type: none; }

          What do you think?

          3) So far as I'm aware there is no naming convention for language strings, anything goes. In this case maybe it could be changed from userrestriction_... to just restriction_... assuming those names aren't used.

          5) Regarding phpunit tests, this should be easy to check and fix. Are you set up to run phpunit for moodle? If not, oh dear, that would be the hard bit! The documentation is at: http://docs.moodle.org/dev/PHPUnit - basically once it's installed, just running phpunit will reveal the location of any problems (takes about 15 minutes to do a full run, where it's also possible that somebody else has broken something unrelated... or you can run it on the specific lib/tests/conditionlib_test.php as I doubt it could affect any other test).

          Show
          Sam Marshall added a comment - Thanks for your continued work on this. To clarify, I am absolutely fine with lists that only have one item, there's nothing wrong with that and it might even look better, it's just the extra line it takes up on screen. These conditions can already look like a kind of wall of text if you use them on many activities - especially for teachers. Doubling that would probably annoy people. Regarding keeping a counter, that doesn't seem like an easy way to do it, as you say. I haven't looked at the code again but there is probably an easier way to do it - work out the result as now, but before outputting it inside the <ul>, do a check on the string. I think the string will always start with <li> so if you do a strpos for <li> starting from position 4 or so (i.e. to skip the first one), then if this === false, you know there is only one <li> in the string, so you COULD do a preg_replace to strip out the starting and ending <li> </li> and output it directly without the <ul>.... BUT maybe a better way of doing it would be to just add the class 'onerestriction' to the UL if there is only a single restriction. That way it would be possible to use CSS to make the <ul> and <li> display inline, effectively having the same display as before (for one restriction) while the HTML structure stays constant, i.e. .whatevertheparentclassis ul.onerestriction, .whatevertheparentclassis ul.onerestriction li { display: inline; list-style-type: none; } What do you think? 3) So far as I'm aware there is no naming convention for language strings, anything goes. In this case maybe it could be changed from userrestriction_... to just restriction_... assuming those names aren't used. 5) Regarding phpunit tests, this should be easy to check and fix. Are you set up to run phpunit for moodle? If not, oh dear, that would be the hard bit! The documentation is at: http://docs.moodle.org/dev/PHPUnit - basically once it's installed, just running phpunit will reveal the location of any problems (takes about 15 minutes to do a full run, where it's also possible that somebody else has broken something unrelated... or you can run it on the specific lib/tests/conditionlib_test.php as I doubt it could affect any other test).
          Hide
          Sam Marshall added a comment -

          UPDATE: I checked with the language expert (thanks david). We do NOT need to change the names of the strings because the AMOS translation system automatically detects changes to string values and reports them. This is provided it is going to be for a future version (2.5), you are not allowed to change lang strings at all within a stable version so this change couldn't go into stable anyhow.

          Sorry for the misinformation.

          Show
          Sam Marshall added a comment - UPDATE: I checked with the language expert (thanks david). We do NOT need to change the names of the strings because the AMOS translation system automatically detects changes to string values and reports them. This is provided it is going to be for a future version (2.5), you are not allowed to change lang strings at all within a stable version so this change couldn't go into stable anyhow. Sorry for the misinformation.
          Hide
          Daniel Wahl added a comment -

          ok, I've updated the functions to not output a list if there's only 1 item (used strip_tags() instead of preg_replace()).

          I installed phpunit and 2 tests of 8 are failing, but they don't seem to be related to what I'm doing - or the errors don't make sense to me:

          dannywahlmbp:master danny.wahl$ vendor/bin/phpunit conditionlib_testcase lib/tests/conditionlib_test.php
          PHPUnit 3.7.10 by Sebastian Bergmann.
          
          Configuration read from /Library/Server/Web/Data/Sites/Dev/moodle/master/phpunit.xml
          
          ......FF
          
          Time: 5 seconds, Memory: 91.50Mb
          
          There were 2 failures:
          
          1) conditionlib_testcase::test_is_available
          Failed asserting that '' is true.
          
          /Library/Server/Web/Data/Sites/Dev/moodle/master/lib/tests/conditionlib_test.php:442
          /Library/Server/Web/Data/Sites/Dev/moodle/master/lib/phpunit/classes/advanced_testcase.php:76
          
          To re-run:
           vendor/bin/phpunit conditionlib_testcase lib/tests/conditionlib_test.php
          
          2) conditionlib_testcase::test_section_is_available
          Failed asserting that '' is true.
          
          /Library/Server/Web/Data/Sites/Dev/moodle/master/lib/tests/conditionlib_test.php:590
          /Library/Server/Web/Data/Sites/Dev/moodle/master/lib/phpunit/classes/advanced_testcase.php:76
          
          To re-run:
           vendor/bin/phpunit conditionlib_testcase lib/tests/conditionlib_test.php
          
          FAILURES!
          Tests: 8, Assertions: 52, Failures: 2.
          
          Show
          Daniel Wahl added a comment - ok, I've updated the functions to not output a list if there's only 1 item (used strip_tags() instead of preg_replace()). I installed phpunit and 2 tests of 8 are failing, but they don't seem to be related to what I'm doing - or the errors don't make sense to me: dannywahlmbp:master danny.wahl$ vendor/bin/phpunit conditionlib_testcase lib/tests/conditionlib_test.php PHPUnit 3.7.10 by Sebastian Bergmann. Configuration read from /Library/Server/Web/Data/Sites/Dev/moodle/master/phpunit.xml ......FF Time: 5 seconds, Memory: 91.50Mb There were 2 failures: 1) conditionlib_testcase::test_is_available Failed asserting that '' is true. /Library/Server/Web/Data/Sites/Dev/moodle/master/lib/tests/conditionlib_test.php:442 /Library/Server/Web/Data/Sites/Dev/moodle/master/lib/phpunit/classes/advanced_testcase.php:76 To re-run: vendor/bin/phpunit conditionlib_testcase lib/tests/conditionlib_test.php 2) conditionlib_testcase::test_section_is_available Failed asserting that '' is true. /Library/Server/Web/Data/Sites/Dev/moodle/master/lib/tests/conditionlib_test.php:590 /Library/Server/Web/Data/Sites/Dev/moodle/master/lib/phpunit/classes/advanced_testcase.php:76 To re-run: vendor/bin/phpunit conditionlib_testcase lib/tests/conditionlib_test.php FAILURES! Tests: 8, Assertions: 52, Failures: 2.
          Hide
          Sam Marshall added a comment -

          Thanks! I'm a little doubtful about strip_tags vs. preg_replace to remove only the ones at start and end, but I guess it is currently not possible for there to be any other HTML tags inside the activity names... or is it... hmm... actually I think there might be an option for this in preferences somewhere...

          Regarding the unit tests, those two errors both indicate that it's asserting something is true, which is actually false. To see whether they are related to what you're doing, just switch branch in git to the branch your code is based on (i.e. master, if it's that), and rerun the exact same unit test. If the same errors occur then the errors are not related to your change - in that case please let me know, I'll fix it and we can go from there, the unit tests ought to pass. If they ARE related to what you're doing, look at the line numbers given in the test to see why it's failing. Most likely, the tests have hard-coded the expected result, i.e. it is a plain string and not a list, so they would need to be changed so that the hard-coded version includes <li> and such.

          This is nearly ready to submit for integration. One trivia point about coding standards (the only thing I spotted): there is supposed to be a space after the 'if' command and before the bracket (I don't know why), like 'if (true)' instead of 'if(true)'.

          Show
          Sam Marshall added a comment - Thanks! I'm a little doubtful about strip_tags vs. preg_replace to remove only the ones at start and end, but I guess it is currently not possible for there to be any other HTML tags inside the activity names... or is it... hmm... actually I think there might be an option for this in preferences somewhere... Regarding the unit tests, those two errors both indicate that it's asserting something is true, which is actually false. To see whether they are related to what you're doing, just switch branch in git to the branch your code is based on (i.e. master, if it's that), and rerun the exact same unit test. If the same errors occur then the errors are not related to your change - in that case please let me know, I'll fix it and we can go from there, the unit tests ought to pass. If they ARE related to what you're doing, look at the line numbers given in the test to see why it's failing. Most likely, the tests have hard-coded the expected result, i.e. it is a plain string and not a list, so they would need to be changed so that the hard-coded version includes <li> and such. This is nearly ready to submit for integration. One trivia point about coding standards (the only thing I spotted): there is supposed to be a space after the 'if' command and before the bracket (I don't know why), like 'if (true)' instead of 'if(true)'.
          Hide
          Sam Marshall added a comment -

          btw just in case you're not familiar with regular expressions (apologies if you are), the preg_replace line would probably be (top of my head)

          $value = preg_replace('~^<li>(.*)</li>$~', '$1', $value);
          
          Show
          Sam Marshall added a comment - btw just in case you're not familiar with regular expressions (apologies if you are), the preg_replace line would probably be (top of my head) $value = preg_replace('~^<li>(.*)</li>$~', '$1', $value);
          Hide
          Daniel Wahl added a comment -

          I switched to the preg_replace regex and it seems to be working.

          I found one severe problem now when checking the phpunit tests. If I switch to "student role" as a teacher on master I can see the whole course with the output as you would expect (all topics, etc...). If I do the same thing on my branch I get a "not available" output, which it shouldn't be doing. I can't chase down the string it's outputting or where it's coming from (see screenshot)

          Show
          Daniel Wahl added a comment - I switched to the preg_replace regex and it seems to be working. I found one severe problem now when checking the phpunit tests. If I switch to "student role" as a teacher on master I can see the whole course with the output as you would expect (all topics, etc...). If I do the same thing on my branch I get a "not available" output, which it shouldn't be doing. I can't chase down the string it's outputting or where it's coming from (see screenshot)
          Hide
          Daniel Wahl added a comment -

          login as teacher -> switch role to student -> view course

          Show
          Daniel Wahl added a comment - login as teacher -> switch role to student -> view course
          Hide
          Daniel Wahl added a comment -

          well I found the problem. that's what happens when you return $information instead of $available. I'm now passing all unit tests again!

          dannywahlmbp:master danny.wahl$ vendor/bin/phpunit conditionlib_testcase lib/tests/conditionlib_test.php
          PHPUnit 3.7.10 by Sebastian Bergmann.
          
          Configuration read from /Library/Server/Web/Data/Sites/Dev/moodle/master/phpunit.xml
          
          ........
          
          Time: 6 seconds, Memory: 96.50Mb
          
          OK (8 tests, 118 assertions)
          

          Stick a fork in this one, it's done! By the way I now have a whole new respect for you real Moodle Devs!

          Show
          Daniel Wahl added a comment - well I found the problem. that's what happens when you return $information instead of $available. I'm now passing all unit tests again! dannywahlmbp:master danny.wahl$ vendor/bin/phpunit conditionlib_testcase lib/tests/conditionlib_test.php PHPUnit 3.7.10 by Sebastian Bergmann. Configuration read from /Library/Server/Web/Data/Sites/Dev/moodle/master/phpunit.xml ........ Time: 6 seconds, Memory: 96.50Mb OK (8 tests, 118 assertions) Stick a fork in this one, it's done! By the way I now have a whole new respect for you real Moodle Devs!
          Hide
          Daniel Wahl added a comment -

          I think this is ready for integration review.

          Show
          Daniel Wahl added a comment - I think this is ready for integration review.
          Hide
          Sam Marshall added a comment -

          Thanks! I agree the code is all done. I've put it into my repository to make it easier for integrators, and made the following changes:

          1) There were whitespace errors (space at end of line, some tabs in indent), I fixed.
          2) I added comments on the bits that strip out the list tags if it's only one line.

          I also used the 'right' kind of commit message, and squashed it into a single commit. You can see it by clicking the 'master diff URL' above.

          There's just one thing that needs doing before submitting: it needs a test script so the testers can verify that it actually works. Could you write a test script please? Something like:

          1. create new course
          2. add a Page activity with two conditions: date set to the future and course grade set to more than 50%
          3. add another Page with only one condition: date set to the future
          4. add another one with only one condition (date set to the future), and this time, set it to not display at all until the condition is met.

          Look at the conditions, verify that:

          • the first Page shows a bullet list of conditions.
          • the second Page shows only a single condition (no list).
          • the third Page also shows only a single condition and has the text 'Restricted (not visible)' or whatever that is to indicate that it isn't shown to students.

          5. Log in with a student account

          • display of the first two Page restrictions should be similar, except it doesn't show the initial 'Restricted:' text.
          • the third Page should not show at all (unchanged behaviour).

          It might be that my test script is good enough, but since I wrote it off the top of my head without actually testing, it would be a good idea if somebody actually tests it before the testers get to it, I'm hoping you could do that Also change the script if wrong or not good enough.

          Show
          Sam Marshall added a comment - Thanks! I agree the code is all done. I've put it into my repository to make it easier for integrators, and made the following changes: 1) There were whitespace errors (space at end of line, some tabs in indent), I fixed. 2) I added comments on the bits that strip out the list tags if it's only one line. I also used the 'right' kind of commit message, and squashed it into a single commit. You can see it by clicking the 'master diff URL' above. There's just one thing that needs doing before submitting: it needs a test script so the testers can verify that it actually works. Could you write a test script please? Something like: 1. create new course 2. add a Page activity with two conditions: date set to the future and course grade set to more than 50% 3. add another Page with only one condition: date set to the future 4. add another one with only one condition (date set to the future), and this time, set it to not display at all until the condition is met. Look at the conditions, verify that: the first Page shows a bullet list of conditions. the second Page shows only a single condition (no list). the third Page also shows only a single condition and has the text 'Restricted (not visible)' or whatever that is to indicate that it isn't shown to students. 5. Log in with a student account display of the first two Page restrictions should be similar, except it doesn't show the initial 'Restricted:' text. the third Page should not show at all (unchanged behaviour). It might be that my test script is good enough, but since I wrote it off the top of my head without actually testing, it would be a good idea if somebody actually tests it before the testers get to it, I'm hoping you could do that Also change the script if wrong or not good enough.
          Hide
          Daniel Wahl added a comment -

          Testing Instructions:

          A) create new course
          1. add a Page activity with two conditions: date set to the future and course grade set to more than 50%
          2. add another Page with only one condition: date set to the future
          3. add another one with only one condition (date set to the future), and this time, set it to not display at all until the condition is met.

          B) Look at the conditions in the editing teacher role, verify that:
          1. the first Page shows a bullet list of conditions.
          2. the second Page shows only a single condition (no list).
          3. the third Page also shows only a single condition and has the text 'Restricted (not visible)' or whatever that is to indicate that it isn't shown to students.

          C) Log in with a student account or switch role to Student
          1. the first Page should be a list of conditions
          2. the second Page should show only a single condition (no list).
          3. The third page should not show at all

          Note: the strings displayed to students and editing teachers will probably be slightly different, but the rendering of the conditional activity lists should be the same.

          Show
          Daniel Wahl added a comment - Testing Instructions: A) create new course 1. add a Page activity with two conditions: date set to the future and course grade set to more than 50% 2. add another Page with only one condition: date set to the future 3. add another one with only one condition (date set to the future), and this time, set it to not display at all until the condition is met. B) Look at the conditions in the editing teacher role, verify that: 1. the first Page shows a bullet list of conditions. 2. the second Page shows only a single condition (no list). 3. the third Page also shows only a single condition and has the text 'Restricted (not visible)' or whatever that is to indicate that it isn't shown to students. C) Log in with a student account or switch role to Student 1. the first Page should be a list of conditions 2. the second Page should show only a single condition (no list). 3. The third page should not show at all Note: the strings displayed to students and editing teachers will probably be slightly different, but the rendering of the conditional activity lists should be the same.
          Hide
          Sam Marshall added a comment -

          Thanks, those are great. I put the test instructions in the slot for them, and have now submitted for integration. Let's hope there isn't anything else wrong for the integration reviewer to spot.

          Show
          Sam Marshall added a comment - Thanks, those are great. I put the test instructions in the slot for them, and have now submitted for integration. Let's hope there isn't anything else wrong for the integration reviewer to spot.
          Hide
          Nicolas Connault added a comment -

          Couldn't this be backported to earlier versions?

          Show
          Nicolas Connault added a comment - Couldn't this be backported to earlier versions?
          Hide
          Sam Marshall added a comment -

          Nicolas: It could easily be ported to earlier versions, but it requires a change in language strings. As I understand it (and I might be wrong) we aren't allowed to change language strings in a stable version.

          Show
          Sam Marshall added a comment - Nicolas: It could easily be ported to earlier versions, but it requires a change in language strings. As I understand it (and I might be wrong) we aren't allowed to change language strings in a stable version.
          Hide
          Dan Poltawski added a comment -

          I've integrated it to master only.

          Its true that we can't change the meaning of existing strings on the stable branches (the lang packs need to work the same on 2.4.0 and 2.4.5 so if you change the meaning it could make someone on a older version have confusing interface). You could do it by introducing new strings though (if really want to).

          If this wants backporting we can do it in a new issue.

          Show
          Dan Poltawski added a comment - I've integrated it to master only. Its true that we can't change the meaning of existing strings on the stable branches (the lang packs need to work the same on 2.4.0 and 2.4.5 so if you change the meaning it could make someone on a older version have confusing interface). You could do it by introducing new strings though (if really want to). If this wants backporting we can do it in a new issue.
          Hide
          Rossiani Wijaya added a comment -

          This works expected.

          Test passed.

          Show
          Rossiani Wijaya added a comment - This works expected. Test passed.
          Hide
          Dan Poltawski added a comment -

          Hurray! We did it! Thanks to all the reporters, testers, user and watchers for a bumper week of Moodling!

          Show
          Dan Poltawski added a comment - Hurray! We did it! Thanks to all the reporters, testers, user and watchers for a bumper week of Moodling!

            People

            • Votes:
              0 Vote for this issue
              Watchers:
              5 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved: