Moodle
  1. Moodle
  2. MDL-34476

Completion block's "full course report" is unavailable to Teachers

    Details

    • Testing Instructions:
      Hide
      1. Enable course completion on a course
      2. Add the course completion block to the course page
      3. Login as a teacher and ensure you can see "View Course report" in addition to the normal course completion details
      4. Login as an admin who is not enrolled in the course and ensure you are told you aren't enrolled in the course, but can see the "View Course report"
      5. Login as an student and ensure you can see the normal course completion details
      Show
      Enable course completion on a course Add the course completion block to the course page Login as a teacher and ensure you can see "View Course report" in addition to the normal course completion details Login as an admin who is not enrolled in the course and ensure you are told you aren't enrolled in the course, but can see the "View Course report" Login as an student and ensure you can see the normal course completion details
    • Affected Branches:
      MOODLE_23_STABLE
    • Fixed Branches:
      MOODLE_22_STABLE, MOODLE_23_STABLE
    • Pull from Repository:
    • Pull Master Branch:
      wip-MDL-34476-master
    • Rank:
      42892

      Description

      Only unenrolled users (Managers and Administrators) can see full course completion report
      Teachers, that need it most, are unable to see the report since they are enrolled into the course.
      Surely this is not the intended behavior (Am i right?)

      blocks/completionstatus/block_completionstatus.php

      // Check this user is enroled
      if (!$info->is_tracked_user($USER->id)) {
      

      lib/completionlib.php

          public function is_tracked_user($userid) {
              return is_enrolled(context_course::instance($this->course->id), $userid, '', true);
          }
      

      Looking into the above code, It seems there should be a distinction between students and all the rest (Teachers and Administration stuff) regarding "Who should be able to see the complete course completion report"

        Activity

        Hide
        Mike Walters added a comment -

        I'd like to add my voice to this one as it is a big problem for me at the moment.

        I have checked permissions in the course and my teachers say that "report/completion:view" is "yes". But they still do not have access.

        I have checked as many of the capability and role overrides I can think of and still cannot get the report visible.

        The site I am testing this on is the latest 2.3.1 weekly build. As a second test I also took the "coursestatus" block code from a 2.2 site where the same block is working fine and replaced the 2.3.1 code. The error persisted.

        Please feel free to contact me if you need me to test or try anything in a production site to try and resolve this issue.

        Cheers,
        Mike

        Show
        Mike Walters added a comment - I'd like to add my voice to this one as it is a big problem for me at the moment. I have checked permissions in the course and my teachers say that "report/completion:view" is "yes". But they still do not have access. I have checked as many of the capability and role overrides I can think of and still cannot get the report visible. The site I am testing this on is the latest 2.3.1 weekly build. As a second test I also took the "coursestatus" block code from a 2.2 site where the same block is working fine and replaced the 2.3.1 code. The error persisted. Please feel free to contact me if you need me to test or try anything in a production site to try and resolve this issue. Cheers, Mike
        Hide
        Nadav Kavalerchik added a comment -

        +1 (like) to Mike's comment

        Show
        Nadav Kavalerchik added a comment - +1 (like) to Mike's comment
        Hide
        Nadav Kavalerchik added a comment -

        What seems to workaround for us (for now) is:

        // If not enrolled, but are can view the report:
        if (has_capability('report/completion:view', get_context_instance(CONTEXT_COURSE, $COURSE->id))) {
            $this->content->text = '<a href="'.$CFG->wwwroot.'/report/completion/index.php?course='.$COURSE->id.
        	'">'.get_string('viewcoursereport', 'completion').'</a>';
            return $this->content;
        }
        
        // Check this user is enroled
        if (!$info->is_tracked_user($USER->id)) {
            // Otherwise, show error
            $this->content->text = get_string('notenroled', 'completion');
            return $this->content;
        }
        
        Show
        Nadav Kavalerchik added a comment - What seems to workaround for us (for now) is: // If not enrolled, but are can view the report: if (has_capability('report/completion:view', get_context_instance(CONTEXT_COURSE, $COURSE->id))) { $ this ->content->text = '<a href="'.$CFG->wwwroot.'/report/completion/index.php?course='.$COURSE->id. '">'.get_string('viewcoursereport', 'completion').'</a>'; return $ this ->content; } // Check this user is enroled if (!$info->is_tracked_user($USER->id)) { // Otherwise, show error $ this ->content->text = get_string('notenroled', 'completion'); return $ this ->content; }
        Hide
        Mike Walters added a comment -

        Love it! worked a treat

        Shame it has meant a mod to the core code (will make updating a little more painful) but it has solved the problem.

        Thanks for the workaround. Have you submitted this for inclusion in core for the next release?

        Cheers,
        Mike

        Show
        Mike Walters added a comment - Love it! worked a treat Shame it has meant a mod to the core code (will make updating a little more painful) but it has solved the problem. Thanks for the workaround. Have you submitted this for inclusion in core for the next release? Cheers, Mike
        Hide
        Nadav Kavalerchik added a comment -

        We are using GIT to manage our code (you are advised to do so,too).
        So this patch (on our servers) is currently on a "dev" branch,
        which allows us to seperate the upstream Moodle.org changes ("MOODLE_23_STABLE" branch) from the changes we made. And "git rebase" our changes over the new updates we get from upstream Moodle.org.
        When these changes get integrated we will discard our local branch

        I see Sam Marshall in assigned for this issue. so i guess he will find the right time to test/review/integrate this patch into the core. (or suggest a different patch, if needed)

        Show
        Nadav Kavalerchik added a comment - We are using GIT to manage our code (you are advised to do so,too). So this patch (on our servers) is currently on a "dev" branch, which allows us to seperate the upstream Moodle.org changes ("MOODLE_23_STABLE" branch) from the changes we made. And "git rebase" our changes over the new updates we get from upstream Moodle.org. When these changes get integrated we will discard our local branch I see Sam Marshall in assigned for this issue. so i guess he will find the right time to test/review/integrate this patch into the core. (or suggest a different patch, if needed)
        Hide
        Mike Walters added a comment -

        Sadly I am not lucky enough to have my own server yet , and I don't think I can get GIT installed on my shared hosting.
        But this isn't the place to discuss that! Just glad I found a solution so quickly.

        Show
        Mike Walters added a comment - Sadly I am not lucky enough to have my own server yet , and I don't think I can get GIT installed on my shared hosting. But this isn't the place to discuss that! Just glad I found a solution so quickly.
        Hide
        CLAIRE BROWNE added a comment -

        Hey, thanks for the code but what file do I change these statements in?

        Show
        CLAIRE BROWNE added a comment - Hey, thanks for the code but what file do I change these statements in?
        Hide
        Mike Walters added a comment -

        Hi Claire,

        I changed it in block_completionstatus.php and that did the trick.

        Show
        Mike Walters added a comment - Hi Claire, I changed it in block_completionstatus.php and that did the trick.
        Hide
        Maryel Mendiola added a comment -

        Thanks a lot Nadav!!
        I've applied the patch and now it works OK

        Show
        Maryel Mendiola added a comment - Thanks a lot Nadav!! I've applied the patch and now it works OK
        Hide
        CLAIRE BROWNE added a comment -

        Hi, I have applied the patch but my teacher is still regarded as a student in Moodle. I have made sure that my teacher has all the viewing permission capabilities for the course completion status block. Is there something I am missing? What are other peoples set up?

        Show
        CLAIRE BROWNE added a comment - Hi, I have applied the patch but my teacher is still regarded as a student in Moodle. I have made sure that my teacher has all the viewing permission capabilities for the course completion status block. Is there something I am missing? What are other peoples set up?
        Hide
        Mike Walters added a comment -

        I think I have found another problem here
        If I switch on manual completion by teacher etc. required. When I tick the box in the report to approve the completion the screen goes blank.

        Has anyone else seen this? I've tried it on two different sites with the same result.

        Show
        Mike Walters added a comment - I think I have found another problem here If I switch on manual completion by teacher etc. required. When I tick the box in the report to approve the completion the screen goes blank. Has anyone else seen this? I've tried it on two different sites with the same result.
        Hide
        Doug Moody added a comment -

        I want to add that the number of days that this block reports is limited to 30 days, no matter how far into the future you set your expected completion date. The dropdown only allows 30 days. Plus, even if you select 30 days or less, the reported time in the block is not computer correctly. It counts down. it ought to count up, as in "x days out of y days required till completion"
        The math is screwed up on this, so my students don't know how many days are left till the course is over.

        Show
        Doug Moody added a comment - I want to add that the number of days that this block reports is limited to 30 days, no matter how far into the future you set your expected completion date. The dropdown only allows 30 days. Plus, even if you select 30 days or less, the reported time in the block is not computer correctly. It counts down. it ought to count up, as in "x days out of y days required till completion" The math is screwed up on this, so my students don't know how many days are left till the course is over.
        Hide
        Jason Fowler added a comment -

        The logic in that patch is rather sketchy, not sure why it wasn't working before, looking at the latest master code, there shouldn't be a problem with it ...

        Show
        Jason Fowler added a comment - The logic in that patch is rather sketchy, not sure why it wasn't working before, looking at the latest master code, there shouldn't be a problem with it ...
        Hide
        Jason Fowler added a comment -

        Looking at the existing code, I see why the patch works, I've applied a modified version of it, and will backport it if it passes peer review

        Show
        Jason Fowler added a comment - Looking at the existing code, I see why the patch works, I've applied a modified version of it, and will backport it if it passes peer review
        Hide
        Frédéric Massart added a comment - - edited

        Hi Jason,

        your patch looks good and will work, the only thing that tickles me would be the case when a user is enrolled and has the capability, then the content of the block would only be the link to the permission report page.

        The current behaviour was to display to teachers and managers their completion status as well, which does not make much sense, but was happening. If we agree that you can have multiple roles then the capability to see the completion report should not overwrite the content of the block.

        Apart from that it's all good, although you might want to fix the typo in your commit message while backporting .

        Cheers!

        (Edit: Some testing instructions to ensure that the different roles can or can't see the link to the completion report would be nice)

        Show
        Frédéric Massart added a comment - - edited Hi Jason, your patch looks good and will work, the only thing that tickles me would be the case when a user is enrolled and has the capability, then the content of the block would only be the link to the permission report page. The current behaviour was to display to teachers and managers their completion status as well, which does not make much sense, but was happening. If we agree that you can have multiple roles then the capability to see the completion report should not overwrite the content of the block. Apart from that it's all good, although you might want to fix the typo in your commit message while backporting . Cheers! (Edit: Some testing instructions to ensure that the different roles can or can't see the link to the completion report would be nice)
        Hide
        Jason Fowler added a comment -

        After discussing it with Fred, it seems it would be better to have the initial content included when needed, with the report link appended

        Show
        Jason Fowler added a comment - After discussing it with Fred, it seems it would be better to have the initial content included when needed, with the report link appended
        Hide
        Jason Fowler added a comment -

        Changed to match the recommendations of Fred - sorry for the horrible diff this produces, it's mostly whitespace changes...

        Show
        Jason Fowler added a comment - Changed to match the recommendations of Fred - sorry for the horrible diff this produces, it's mostly whitespace changes...
        Hide
        Jason Fowler added a comment -

        just moved the check for enrolment to simplify the changes being made

        Show
        Jason Fowler added a comment - just moved the check for enrolment to simplify the changes being made
        Hide
        Frédéric Massart added a comment -

        Looks good! Feel free to push it further!

        Show
        Frédéric Massart added a comment - Looks good! Feel free to push it further!
        Hide
        Jason Fowler added a comment -

        Pushing for integration

        Show
        Jason Fowler added a comment - Pushing for integration
        Hide
        Sam Hemelryk added a comment -

        Thanks everyone involved with this change, it has been integrated now

        Show
        Sam Hemelryk added a comment - Thanks everyone involved with this change, it has been integrated now
        Hide
        David Monllaó added a comment - - edited

        It passes on master but fails on 22; it's only a var initialization, I've seen it with Jason and the fix is quick.

        Show
        David Monllaó added a comment - - edited It passes on master but fails on 22; it's only a var initialization, I've seen it with Jason and the fix is quick.
        Hide
        Jason Fowler added a comment -

        I've pushed a fix for this to the 2.2 branch, David has made sure it works, so once that has been integrated, this can be reset for testing.

        Show
        Jason Fowler added a comment - I've pushed a fix for this to the 2.2 branch, David has made sure it works, so once that has been integrated, this can be reset for testing.
        Hide
        David Monllaó added a comment -

        Thanks Jason; sorry I forgot to explain where it fails; when I add the course completion status block I receive a fatal error (Coding error detected, it must be fixed by a programmer: PHP catchable fatal error) related with an uninitiated $context var

        Show
        David Monllaó added a comment - Thanks Jason; sorry I forgot to explain where it fails; when I add the course completion status block I receive a fatal error (Coding error detected, it must be fixed by a programmer: PHP catchable fatal error) related with an uninitiated $context var
        Hide
        Sam Hemelryk added a comment -

        Thanks for getting a quick fix up for that guys, I've pulled it in now and its ready for testing again.

        Show
        Sam Hemelryk added a comment - Thanks for getting a quick fix up for that guys, I've pulled it in now and its ready for testing again.
        Hide
        David Monllaó added a comment -

        I haven't begun with the testing but looking at 23 code it seems that we will have the same problem...

        Show
        David Monllaó added a comment - I haven't begun with the testing but looking at 23 code it seems that we will have the same problem...
        Hide
        David Monllaó added a comment -

        Thanks Sam and Jason, after the 23 fix it passes in master, 22 and 23.

        Show
        David Monllaó added a comment - Thanks Sam and Jason, after the 23 fix it passes in master, 22 and 23.
        Hide
        Dan Poltawski added a comment -

        Congratulations, you've done it!

        Nf n erjneq sbe fhpprfshy vagrtengvba vagb guvf jrrxf eryrnfr, V pna abj qvfpybfr gb lbh gur rkvfgnapr bs shapgvba fge_ebg13(), gb tb va lbhe gbbyxvg nybat jvgu uggc://cuc.arg/znahny/ra/shapgvba.tmtrgff.cuc

        Cyrnfr qb abg nyybj guvf vasbezngvba gb cnff shegure.

        Show
        Dan Poltawski added a comment - Congratulations, you've done it! Nf n erjneq sbe fhpprfshy vagrtengvba vagb guvf jrrxf eryrnfr, V pna abj qvfpybfr gb lbh gur rkvfgnapr bs shapgvba fge_ebg13(), gb tb va lbhe gbbyxvg nybat jvgu uggc://cuc.arg/znahny/ra/shapgvba.tmtrgff.cuc Cyrnfr qb abg nyybj guvf vasbezngvba gb cnff shegure.

          People

          • Votes:
            7 Vote for this issue
            Watchers:
            7 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved: