Uploaded image for project: '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

      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"

        Gliffy Diagrams

          Activity

          Hide
          redtwenty 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
          redtwenty 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
          nadavkav Nadav Kavalerchik added a comment -

          +1 (like) to Mike's comment

          Show
          nadavkav Nadav Kavalerchik added a comment - +1 (like) to Mike's comment
          Hide
          nadavkav 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
          nadavkav 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
          redtwenty 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
          redtwenty 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
          nadavkav 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
          nadavkav 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
          redtwenty 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
          redtwenty 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
          moodlecvqo CLAIRE BROWNE added a comment -

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

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

          Hi Claire,

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

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

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

          Show
          m Maryel Mendiola added a comment - Thanks a lot Nadav!! I've applied the patch and now it works OK
          Hide
          moodlecvqo 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
          moodlecvqo 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
          redtwenty 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
          redtwenty 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
          moodman 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
          moodman 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
          phalacee 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
          phalacee 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
          phalacee 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
          phalacee 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
          fred 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
          fred 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
          phalacee 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
          phalacee 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
          phalacee 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
          phalacee 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
          phalacee Jason Fowler added a comment -

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

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

          Looks good! Feel free to push it further!

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

          Pushing for integration

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

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

          Show
          samhemelryk Sam Hemelryk added a comment - Thanks everyone involved with this change, it has been integrated now
          Hide
          dmonllao 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
          dmonllao 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
          phalacee 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
          phalacee 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
          dmonllao 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
          dmonllao 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
          samhemelryk 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
          samhemelryk 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
          dmonllao 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
          dmonllao 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
          dmonllao David Monllaó added a comment -

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

          Show
          dmonllao David Monllaó added a comment - Thanks Sam and Jason, after the 23 fix it passes in master, 22 and 23.
          Hide
          poltawski 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
          poltawski 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:
                Fix Release Date:
                12/Nov/12