Moodle
  1. Moodle
  2. MDL-26778

Conditional activities: Activities conditional on automatic completion do not update immediately

    Details

    • Database:
      MySQL
    • Difficulty:
      Moderate
    • Affected Branches:
      MOODLE_20_STABLE
    • Rank:
      16394

      Description

      Initially completion tracking and conditional activities are activated on the system. I created the following course structure: (1)Resource Page ( conditional activity: student has to visit/view this page to complete it) ; (2) Quiz (restrcit access: (1) must be marked as completed; condition: student has to get a grade) ; (3) Resource Page / Embedded a video file (restrict access: (2) must be marked as completed; condition: student has to visit/view this page to complete it) ; (4) Quiz (restrcit access: (3) must be marked as completed; condition: student has to get a grade)Quiz; (5) Questionnaire (restrict access: (4) must be marked as completed).
      Completion tracking is set to: Enabled, controled via completion and activity settings
      Completion setting begin on enrolment: set

      If i log in as a student, and visit the first page, the navigation block is not updated immediately, so i can't navigate to the next activity. i have to return to the course overview (here i use topic format, so i've got to go to the topic outline) or hit the page refresh button (f5). there i see (2) is now activated. This issue fit to all page resources. the navigation block is never updated immediately when the condition is set, that a student have to view this page, to get the activity completed.

      May be it could be the same issue as in : http://tracker.moodle.org/browse/MDL-17915

        Issue Links

          Activity

          Hide
          Charles Fulton added a comment -

          Sounds similar to MDL-26507: caching issue?

          Show
          Charles Fulton added a comment - Sounds similar to MDL-26507 : caching issue?
          Hide
          Sam Marshall added a comment -

          This is not directly related to MDL-17915 (which was fixed) or MDL-26507 (which related to changing completion conditions by somebody else). The code is actually behaving as expected: at the end of the page, it updates the 'viewed' flag to yes. In any future request it will correctly get the 'yes' value.

          The problem is that this is not reflected in the navigation block, within the same request.

          Basically this code was originally written in Moodle 1.9 where there wasn't a navigation block so you had to go back to the course page to see the next activity, so it didn't matter...

          There are two possibilities to solve this bug:

          1) Find out if it is possible to update completion data before the navigation block is drawn. (For example, in completion::set_viewed or whatever that function is, make it clear the modinfo cache so that next time modinfo is requested for the nav block, it has correct data. This might work so long as nav block code runs after the set_viewed code, which I'm not sure.)

          2) If this is difficult for some reason, make it redirect to reload the page when completion changes (this is obviously really sucky).

          Hopefully I will look at this soon (i.e. in the next week or so). If I don't, feel free to post a comment to prod me.

          Show
          Sam Marshall added a comment - This is not directly related to MDL-17915 (which was fixed) or MDL-26507 (which related to changing completion conditions by somebody else). The code is actually behaving as expected: at the end of the page, it updates the 'viewed' flag to yes. In any future request it will correctly get the 'yes' value. The problem is that this is not reflected in the navigation block, within the same request. Basically this code was originally written in Moodle 1.9 where there wasn't a navigation block so you had to go back to the course page to see the next activity, so it didn't matter... There are two possibilities to solve this bug: 1) Find out if it is possible to update completion data before the navigation block is drawn. (For example, in completion::set_viewed or whatever that function is, make it clear the modinfo cache so that next time modinfo is requested for the nav block, it has correct data. This might work so long as nav block code runs after the set_viewed code, which I'm not sure.) 2) If this is difficult for some reason, make it redirect to reload the page when completion changes (this is obviously really sucky). Hopefully I will look at this soon (i.e. in the next week or so). If I don't, feel free to post a comment to prod me.
          Hide
          Ramo Karahasan added a comment - - edited

          Wouldn't it be possible to add any ajax server push events on this issue, so that the navigation block is updated. the page resource could check with javascript if the condition is met (page is viewed) and trigger an ajax event to update the navigation block. i'm sorry if this proposal is non-sense, but i'm not really 100% aware of the moodle architecture.

          Show
          Ramo Karahasan added a comment - - edited Wouldn't it be possible to add any ajax server push events on this issue, so that the navigation block is updated. the page resource could check with javascript if the condition is met (page is viewed) and trigger an ajax event to update the navigation block. i'm sorry if this proposal is non-sense, but i'm not really 100% aware of the moodle architecture.
          Hide
          Sam Marshall added a comment -

          I'm pretty sure that wouldn't be possible without a substantial rewrite of almost everything

          Show
          Sam Marshall added a comment - I'm pretty sure that wouldn't be possible without a substantial rewrite of almost everything
          Hide
          Ramo Karahasan added a comment -

          Hi Sam,

          the week is still young, but i thought to start prodding you . How is the status? Will you have time to fix this issue this week?

          Show
          Ramo Karahasan added a comment - Hi Sam, the week is still young, but i thought to start prodding you . How is the status? Will you have time to fix this issue this week?
          Hide
          Sam Marshall added a comment -

          Nope, sorry. I think at current state it's looking unlikely that I will be able to fix this issue before the first week in April (I have a deadline April 1st, so I mean, after my deadline).

          This is unless somebody from Moodle HQ decides this is a priority that needs fixing earlier in order to get into 2.0.3 or whatever, in which case I can possibly find time to fix it earlier.

          Show
          Sam Marshall added a comment - Nope, sorry. I think at current state it's looking unlikely that I will be able to fix this issue before the first week in April (I have a deadline April 1st, so I mean, after my deadline). This is unless somebody from Moodle HQ decides this is a priority that needs fixing earlier in order to get into 2.0.3 or whatever, in which case I can possibly find time to fix it earlier.
          Hide
          Ramo Karahasan added a comment -

          Hi Sam,

          how is the status about this bug? Do you have time to fix it and provide a patch for 2.0.2?

          Show
          Ramo Karahasan added a comment - Hi Sam, how is the status about this bug? Do you have time to fix it and provide a patch for 2.0.2?
          Hide
          Sam Marshall added a comment -

          I don't have time for anything! Hoping to look at this anyway though.

          Show
          Sam Marshall added a comment - I don't have time for anything! Hoping to look at this anyway though.
          Hide
          Sam Marshall added a comment -

          I have a fix for this now - the easy option worked, yay - will file PULL requests as soon as this week's weekly comes out.

          Show
          Sam Marshall added a comment - I have a fix for this now - the easy option worked, yay - will file PULL requests as soon as this week's weekly comes out.
          Hide
          Ramo Karahasan added a comment -

          Great,

          can you tell me, in which file you fixed this problem or better, could you send me the file to ramo.karahasan@gmail.com?

          Cheers,
          Ramo

          Show
          Ramo Karahasan added a comment - Great, can you tell me, in which file you fixed this problem or better, could you send me the file to ramo.karahasan@gmail.com? Cheers, Ramo
          Hide
          Sam Marshall added a comment -

          Filed PULL-595, PULL-596 (Ramo - you can see the new code from there, e.g. look at the difference url).

          This change is trivial: there is a part where it already adjusts the in-memory completion state, if setting completion for current user. In that case I made it also clear the modinfo cache.

          Test steps:

          0. You must have completion and conditional availability enabled (a) in general, (b) for the test course.

          1. Set up a Page activity in a course week, called 'Page 1'. Set it to be automatically marked complete when it is viewed.

          2. Set up another one in the same week called 'Page 2'. Set it to be available only when Page 1 is complete.

          3. Using a student account, visit the course page. Note that Page 1 link is available but the Page 2 link is greyed out. In the navigation block, expand the relevant week. Verify that Page 1 is shown in the navigation, but Page 2 is not.

          4. Visit Page 1. Look at the navigation block. Verify that Page 2 is now visible in the navigation block.

          Before this bug was fixed, in step 4, page 2 would not be visible in the navigation block (however it would become visible if you reloaded the page).

          NOTE: Once it is marked complete you can't easily uncomplete it, so in order to repeat this test you have to literally repeat the whole thing (create new pages) or else use a different student account.

          Show
          Sam Marshall added a comment - Filed PULL-595, PULL-596 (Ramo - you can see the new code from there, e.g. look at the difference url). This change is trivial: there is a part where it already adjusts the in-memory completion state, if setting completion for current user. In that case I made it also clear the modinfo cache. Test steps: 0. You must have completion and conditional availability enabled (a) in general, (b) for the test course. 1. Set up a Page activity in a course week, called 'Page 1'. Set it to be automatically marked complete when it is viewed. 2. Set up another one in the same week called 'Page 2'. Set it to be available only when Page 1 is complete. 3. Using a student account, visit the course page. Note that Page 1 link is available but the Page 2 link is greyed out. In the navigation block, expand the relevant week. Verify that Page 1 is shown in the navigation, but Page 2 is not. 4. Visit Page 1. Look at the navigation block. Verify that Page 2 is now visible in the navigation block. Before this bug was fixed, in step 4, page 2 would not be visible in the navigation block (however it would become visible if you reloaded the page). NOTE: Once it is marked complete you can't easily uncomplete it, so in order to repeat this test you have to literally repeat the whole thing (create new pages) or else use a different student account.
          Hide
          Sam Marshall added a comment -

          Eloy pointed out that this change works for Page (happens to be the module I tested on) but not for some other modules, see comments in PULL-595. I have made the requested changes and submitted new PULL requests.

          Because the change involved modifying a stack of other modules, I have made a test course that includes all of those modified modules (with view completion turned on) so that we can check it is not broken. The test course is included in the attached backup (mbz file).

          Show
          Sam Marshall added a comment - Eloy pointed out that this change works for Page (happens to be the module I tested on) but not for some other modules, see comments in PULL-595. I have made the requested changes and submitted new PULL requests. Because the change involved modifying a stack of other modules, I have made a test course that includes all of those modified modules (with view completion turned on) so that we can check it is not broken. The test course is included in the attached backup (mbz file).
          Hide
          Ramo Karahasan added a comment -

          Hi Sam,
          ok.i've just tested it for page. Can you reference to the new PULLs? I can't see them in http://tracker.moodle.org/browse/PULL-595 nor the backup files for testing.

          Show
          Ramo Karahasan added a comment - Hi Sam, ok.i've just tested it for page. Can you reference to the new PULLs? I can't see them in http://tracker.moodle.org/browse/PULL-595 nor the backup files for testing.
          Hide
          Eloy Lafuente (stronk7) added a comment -

          New pull requests are, for reference:

          • PULL-645, for master (aka. 2.1dev).
          • PULL-646, for 20_STABLE (aka. 2.0.x+)

          Ciao

          Show
          Eloy Lafuente (stronk7) added a comment - New pull requests are, for reference: PULL-645, for master (aka. 2.1dev). PULL-646, for 20_STABLE (aka. 2.0.x+) Ciao
          Hide
          Sam Marshall added a comment -

          thanks eloy! Ramo, the backup file is attached to this bug, see up above & see PULL-645 etc for how it can be used to test. Also the PULL requests are linked up top of this bug as well.

          Show
          Sam Marshall added a comment - thanks eloy! Ramo, the backup file is attached to this bug, see up above & see PULL-645 etc for how it can be used to test. Also the PULL requests are linked up top of this bug as well.
          Hide
          Ramo Karahasan added a comment -

          Yeah,
          i guess i was to impatient . Thanks!

          Show
          Ramo Karahasan added a comment - Yeah, i guess i was to impatient . Thanks!
          Hide
          Carson Tam added a comment -

          Hi Sam,

          When I run the unit tests with this commit '6553cda7c9b9d9ae5439a22841f7986c9b67bc36' , I get these errors:

          set_module_viewed must be called before header is printed
          
              * line 609 of \lib\completionlib.php: call to debugging()
              * line 255 of \lib\simpletest\testcompletionlib.php: call to completion_info->set_module_viewed()
              * line 72 of \lib\simpletestlib\invoker.php: call to completionlib_test->test_set_module_viewed()
              * line 137 of \lib\simpletestlib\invoker.php: call to SimpleInvoker->invoke()
              * line 53 of \lib\simpletestlib\errors.php: call to SimpleInvokerDecorator->invoke()
              * line 137 of \lib\simpletestlib\invoker.php: call to SimpleErrorTrappingInvoker->invoke()
              * line 43 of \lib\simpletestlib\exceptions.php: call to SimpleInvokerDecorator->invoke()
              * line 148 of \lib\simpletestlib\test_case.php: call to SimpleExceptionTrappingInvoker->invoke()
              * line 617 of \lib\simpletestlib\test_case.php: call to SimpleTestCase->run()
              * line 620 of \lib\simpletestlib\test_case.php: call to TestSuite->run()
              * line 37 of \admin\report\unittest\ex_simple_test.php: call to TestSuite->run()
              * line 144 of \lib\simpletestcoveragelib.php: call to AutoGroupTest->run()
              * line 100 of \admin\report\unittest\index.php: call to autogroup_test_coverage->run()
          

          Let me know if you need more info. Thanks.

          Show
          Carson Tam added a comment - Hi Sam, When I run the unit tests with this commit '6553cda7c9b9d9ae5439a22841f7986c9b67bc36' , I get these errors: set_module_viewed must be called before header is printed * line 609 of \lib\completionlib.php: call to debugging() * line 255 of \lib\simpletest\testcompletionlib.php: call to completion_info->set_module_viewed() * line 72 of \lib\simpletestlib\invoker.php: call to completionlib_test->test_set_module_viewed() * line 137 of \lib\simpletestlib\invoker.php: call to SimpleInvoker->invoke() * line 53 of \lib\simpletestlib\errors.php: call to SimpleInvokerDecorator->invoke() * line 137 of \lib\simpletestlib\invoker.php: call to SimpleErrorTrappingInvoker->invoke() * line 43 of \lib\simpletestlib\exceptions.php: call to SimpleInvokerDecorator->invoke() * line 148 of \lib\simpletestlib\test_case.php: call to SimpleExceptionTrappingInvoker->invoke() * line 617 of \lib\simpletestlib\test_case.php: call to SimpleTestCase->run() * line 620 of \lib\simpletestlib\test_case.php: call to TestSuite->run() * line 37 of \admin\report\unittest\ex_simple_test.php: call to TestSuite->run() * line 144 of \lib\simpletestcoveragelib.php: call to AutoGroupTest->run() * line 100 of \admin\report\unittest\index.php: call to autogroup_test_coverage->run() Let me know if you need more info. Thanks.
          Hide
          Sam Marshall added a comment -

          I'm not sure why this issue was still marked as open because the PULL requests were integrated and closed. Closing it now.

          Carson - thanks for report - in future if there are bugs with a change that was already committed, please submit as a new bug (mention original bug number if you know what caused it, like in this case). I have filed this one as MDL-27391. Thanks.

          Show
          Sam Marshall added a comment - I'm not sure why this issue was still marked as open because the PULL requests were integrated and closed. Closing it now. Carson - thanks for report - in future if there are bugs with a change that was already committed, please submit as a new bug (mention original bug number if you know what caused it, like in this case). I have filed this one as MDL-27391 . Thanks.
          Hide
          Carson Tam added a comment -

          Thank you Sam. I will do that in the future.

          Show
          Carson Tam added a comment - Thank you Sam. I will do that in the future.

            People

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

              Dates

              • Created:
                Updated:
                Resolved: