Moodle
  1. Moodle
  2. MDL-34228

Completion report no longer filter by roles (gradebookroles)

    Details

    • Testing Instructions:
      Hide

      NOTE: This test assumes you create a new course and activity. Alternatively, using existing ones will be fine.

      0. Enable completion tracking at system level if necessary.
      1. Set up a new course and enable completion tracking under 'Student progress'.
      2. Create a new Page and set the completion option to 'manual'.
      3. Enrol two users in the course: one as a student (I'm calling this one U1) and one as a teacher (I'm calling this one U2).
      4. View the activity completion report for the course (under Reports / Activity completion in Navigation).
      + The student U1 should display in the report.
      + The teacher U2 should not display in the report.

      5. Under the course settings, go to Users/Permissions.
      6. Look for 'iscompletiontracked'.
      + Currently (default) it is allocated to Student role.

      7. Click the + icon and add the capability to the Teacher role.
      8. Reload the activity completion reports.
      + Now both users should display on the report.

      Show
      NOTE: This test assumes you create a new course and activity. Alternatively, using existing ones will be fine. 0. Enable completion tracking at system level if necessary. 1. Set up a new course and enable completion tracking under 'Student progress'. 2. Create a new Page and set the completion option to 'manual'. 3. Enrol two users in the course: one as a student (I'm calling this one U1) and one as a teacher (I'm calling this one U2). 4. View the activity completion report for the course (under Reports / Activity completion in Navigation). + The student U1 should display in the report. + The teacher U2 should not display in the report. 5. Under the course settings, go to Users/Permissions. 6. Look for 'iscompletiontracked'. + Currently (default) it is allocated to Student role. 7. Click the + icon and add the capability to the Teacher role. 8. Reload the activity completion reports. + Now both users should display on the report.
    • Affected Branches:
      MOODLE_23_STABLE
    • Fixed Branches:
      MOODLE_23_STABLE
    • Pull Master Branch:
      MDL-34228-master
    • Rank:
      42570

      Description

      Since commit MDL-32107
      https://github.com/moodle/moodle/commit/38d06fea66063ca6574a473eec2904ac11912608
      the completion report has silently stopped filtering by gradebookroles (used to be called progresstrackedroles, it's a configuration setting).
      The reason is that the method get_tracked_user_sql() has been eliminated from the completion_info class in completionlib.php in favor of get_enrolled_sql() in lib/accesslib. But they are different functions.

      Only grade report now filters by gradebookroles.
      If that configuration exists, there is a reason.
      Or not?
      It is useful to me, and maybe other Moodle admins are already using it.
      We need to create precise reports for sending to specific authorities and need to control who is included.

      And however, while it exists, it should be used.
      When it is replaced by something, we can use that alternative.

      I know it has been proposed to create a capability 'info/included_in_reports', but 'being included in reports' is not really a capability, like seeing reports is.

      Plus Moodle functionality shouldn't just silently change.

      I think it would be useful to be able to have a completion report that is consistent with the gradebook report. If people want other roles included in the report, they can always add them to gradebookroles.

      Thank you all for the great work and that you for Moodle.

        Issue Links

          Activity

          Hide
          Helen Foster added a comment -

          Hi Olja,

          Thanks for your report and for highlighting it on moodle.org http://moodle.org/mod/forum/discuss.php?d=206601

          Hopefully our activity completion expert Sam can reply to your comments soon.

          Show
          Helen Foster added a comment - Hi Olja, Thanks for your report and for highlighting it on moodle.org http://moodle.org/mod/forum/discuss.php?d=206601 Hopefully our activity completion expert Sam can reply to your comments soon.
          Hide
          Sam Marshall added a comment -

          Thanks for filing this issue. Here is my take on the issue (slightly expanded version of forum post):

          1) It was pretty sketchy that the completion report previously used an admin setting designed for gradebook.

          2) I don't believe 'list of roles' type admin settings are appropriate. In addition the get_enrolled_sql API does not support them (but it supports capabilities).

          3) My opinion is that using a capability for this would be preferable, i.e. so that users appear on this list if they have a role which contains another capability (and don't have one which prohibits it). This allows more accurate control, e.g. you might have a situation where in some courses you want teachers to show and in others you don't and is generally less crappy.

          The only catch is that semantically, it isn't really a capability - it's not something that you (as a person with that role) can do, it's more just information about a flag that applies to you. Perhaps there should be a convention for capability names when they are not really access control permissions, like e.g. if they start with "info/" or something.

          Original forum discussion was http://moodle.org/mod/forum/discuss.php?d=206215

          Show
          Sam Marshall added a comment - Thanks for filing this issue. Here is my take on the issue (slightly expanded version of forum post): 1) It was pretty sketchy that the completion report previously used an admin setting designed for gradebook. 2) I don't believe 'list of roles' type admin settings are appropriate. In addition the get_enrolled_sql API does not support them (but it supports capabilities). 3) My opinion is that using a capability for this would be preferable, i.e. so that users appear on this list if they have a role which contains another capability (and don't have one which prohibits it). This allows more accurate control, e.g. you might have a situation where in some courses you want teachers to show and in others you don't and is generally less crappy. The only catch is that semantically, it isn't really a capability - it's not something that you (as a person with that role) can do, it's more just information about a flag that applies to you. Perhaps there should be a convention for capability names when they are not really access control permissions, like e.g. if they start with "info/" or something. Original forum discussion was http://moodle.org/mod/forum/discuss.php?d=206215
          Hide
          Olja Petrovic added a comment -

          Thank you both for your timely and detailed replies!
          Whatever you decide is OK by me.
          I have contributed my point of view to the discussion, but am aware that I'm not an expert or a core developer, so the decision is obviously yours.
          I'll keep an eye on this issue, and hope I can help (still haven't learned to use Git, but am working on it... in my free time, that means slowly).
          Greetings,
          Olja

          Show
          Olja Petrovic added a comment - Thank you both for your timely and detailed replies! Whatever you decide is OK by me. I have contributed my point of view to the discussion, but am aware that I'm not an expert or a core developer, so the decision is obviously yours. I'll keep an eye on this issue, and hope I can help (still haven't learned to use Git, but am working on it... in my free time, that means slowly). Greetings, Olja
          Hide
          Aaron Barnes added a comment -

          Hi all, just wanted to chip in my 2c.

          First of all, the use of the gradebook role was originally a simplificiation as completion contained it's own setting for choosing which roles were being tracked. I can't remember who originally made the change, I think it was Petr.

          Secondly, adjusting the tracked users functions in completionlib.php also affects course completion and changes need to be reflected in other places (e.g. the cron and reports) it's used.

          This issue definitely needs some careful thought,
          Aaron

          Show
          Aaron Barnes added a comment - Hi all, just wanted to chip in my 2c. First of all, the use of the gradebook role was originally a simplificiation as completion contained it's own setting for choosing which roles were being tracked. I can't remember who originally made the change, I think it was Petr. Secondly, adjusting the tracked users functions in completionlib.php also affects course completion and changes need to be reflected in other places (e.g. the cron and reports) it's used. This issue definitely needs some careful thought, Aaron
          Hide
          Sam Marshall added a comment -

          After discussion with David Mudrak in chat, we think the best approach is to create a new capability:

          moodle/course:iscompletiontrackeduser

          The 'is' prefix indicates that it's an informational capability.

          This should default to set for student and nobody else. (We could do an upgrade to set up this capability based on gradebookroles, not quite sure if this is worth doing or not.)

          In the long term, there would be a similar capability for gradebook (out of scope of this issue) and gradebookroles setting would be removed.

          I'll hopefully code this soon if I get the time (I'm going on holiday again after this week and there are critical things I need to finish for an internal release so not quite sure if it will happen, but hopefully).

          Show
          Sam Marshall added a comment - After discussion with David Mudrak in chat, we think the best approach is to create a new capability: moodle/course:iscompletiontrackeduser The 'is' prefix indicates that it's an informational capability. This should default to set for student and nobody else. (We could do an upgrade to set up this capability based on gradebookroles, not quite sure if this is worth doing or not.) In the long term, there would be a similar capability for gradebook (out of scope of this issue) and gradebookroles setting would be removed. I'll hopefully code this soon if I get the time (I'm going on holiday again after this week and there are critical things I need to finish for an internal release so not quite sure if it will happen, but hopefully).
          Hide
          Sam Marshall added a comment -

          New test script describing how it's supposed to work (patch coming soon).

          Show
          Sam Marshall added a comment - New test script describing how it's supposed to work (patch coming soon).
          Hide
          Sam Marshall added a comment -

          Requesting peer review. Since David already looked at this I'm hoping he can review it.

          NOTE: I didn't update core version.php in this commit, basically it updates every week anyway so I was a bit lazy about conflicts. If you use this patch you need to update your moodle version in some way just so it gets the capability.

          NOTE 2: I didn't make it automatically acquire the initial value for the setting from the old 'gradebookroles' setting - this would be possible if people feel it's required, but defaulting to student is an awful lot simpler.

          Show
          Sam Marshall added a comment - Requesting peer review. Since David already looked at this I'm hoping he can review it. NOTE: I didn't update core version.php in this commit, basically it updates every week anyway so I was a bit lazy about conflicts. If you use this patch you need to update your moodle version in some way just so it gets the capability. NOTE 2: I didn't make it automatically acquire the initial value for the setting from the old 'gradebookroles' setting - this would be possible if people feel it's required, but defaulting to student is an awful lot simpler.
          Hide
          Sam Marshall added a comment -

          Should mention here since I just submitted this: I am away again for 3 weeks after this week, so if there are issues after that, I won't respond right away - if anybody feels this is urgent and wants to take it over, they can, but otherwise I can handle it when I get back.

          Show
          Sam Marshall added a comment - Should mention here since I just submitted this: I am away again for 3 weeks after this week, so if there are issues after that, I won't respond right away - if anybody feels this is urgent and wants to take it over, they can, but otherwise I can handle it when I get back.
          Hide
          David Mudrak added a comment -

          Thanks Sam. I'll look at your solution and will leave a comment here. Enjoy the holiday!

          Show
          David Mudrak added a comment - Thanks Sam. I'll look at your solution and will leave a comment here. Enjoy the holiday!
          Hide
          David Mudrak added a comment -

          The logic seems OK as discussed in the chat. The only thing that caught my eye is the name of the new capability. I am not sure about our recommendation on these "passive" capabilities but something like "Be shown on completion reports" would make more sense to me. Discussing with Helen might be useful here.

          Show
          David Mudrak added a comment - The logic seems OK as discussed in the chat. The only thing that caught my eye is the name of the new capability. I am not sure about our recommendation on these "passive" capabilities but something like "Be shown on completion reports" would make more sense to me. Discussing with Helen might be useful here.
          Hide
          David Mudrak added a comment -

          Please note that the branch holding this patch is actually called just "MDL-34228". I am fixing this issue report to make the links work.

          Show
          David Mudrak added a comment - Please note that the branch holding this patch is actually called just " MDL-34228 ". I am fixing this issue report to make the links work.
          Hide
          Sam Marshall added a comment -

          Thanks David. I agree with your wording for the capability name, have changed it.

          (Sorry about the branch name - it SHOULD have been called that... I changed the branch name.)

          I rebased and cherry-picked it to 23 as well, will submit for integration, let's see how it goes...

          Show
          Sam Marshall added a comment - Thanks David. I agree with your wording for the capability name, have changed it. (Sorry about the branch name - it SHOULD have been called that... I changed the branch name.) I rebased and cherry-picked it to 23 as well, will submit for integration, let's see how it goes...
          Hide
          Dan Poltawski added a comment -

          The main moodle.git repository has just been updated with latest weekly modifications. You may wish to rebase your PULL branches to simplify history and avoid any possible merge conflicts. This would also make integrator's life easier next week.

          TIA and ciao

          Show
          Dan Poltawski added a comment - The main moodle.git repository has just been updated with latest weekly modifications. You may wish to rebase your PULL branches to simplify history and avoid any possible merge conflicts. This would also make integrator's life easier next week. TIA and ciao
          Hide
          Sam Marshall added a comment -

          Rebased both branches.

          Show
          Sam Marshall added a comment - Rebased both branches.
          Hide
          Eloy Lafuente (stronk7) added a comment - - edited

          Two comments:

          • Why the cap name is "iscompletiontrackeduser" and the description is "Be shown on completion reports" instead of "Be tracked by completion system".
          • More yet... following the same point... if it's about to who is tracked (for completion)... why the capability check is only being applied in that (reporting) place? Perhaps that's enough but... in my mind... it sounds like we should be also checking that cap before adding/calculating any completion info, isn't it? Or is there any interest about doing that for everybody?

          So, basically, or the name is wrong or the implementation is incomplete. I'm reopening this for next week, just to clarify.

          Ciao

          Show
          Eloy Lafuente (stronk7) added a comment - - edited Two comments: Why the cap name is "iscompletiontrackeduser" and the description is "Be shown on completion reports" instead of "Be tracked by completion system". More yet... following the same point... if it's about to who is tracked (for completion)... why the capability check is only being applied in that (reporting) place? Perhaps that's enough but... in my mind... it sounds like we should be also checking that cap before adding/calculating any completion info, isn't it? Or is there any interest about doing that for everybody? So, basically, or the name is wrong or the implementation is incomplete. I'm reopening this for next week, just to clarify. Ciao
          Hide
          CiBoT added a comment -

          Moving this reopened issue out from current integration. Please, re-submit it for integration once ready.

          Show
          CiBoT added a comment - Moving this reopened issue out from current integration. Please, re-submit it for integration once ready.
          Hide
          Sam Marshall added a comment -

          Eloy: I definitely don't think it is appropriate to actually stop it tracking completion for people (apart from that I don't think that's a good idea anyway, it would also make a whole lot more work).

          So agree with rename of capability. The new name is 'isincompletionreports'.

          I have pushed new versions of both branch and am resubmitting for integration next week.

          Show
          Sam Marshall added a comment - Eloy: I definitely don't think it is appropriate to actually stop it tracking completion for people (apart from that I don't think that's a good idea anyway, it would also make a whole lot more work). So agree with rename of capability. The new name is 'isincompletionreports'. I have pushed new versions of both branch and am resubmitting for integration next week.
          Hide
          Dan Poltawski added a comment -

          Integrated to 23 and master.

          Thanks Sam!

          Show
          Dan Poltawski added a comment - Integrated to 23 and master. Thanks Sam!
          Hide
          Adrian Greeve added a comment -

          Tested on the 2.3 and master integration branches.
          Everything worked as described.
          Test passed.

          Show
          Adrian Greeve added a comment - Tested on the 2.3 and master integration branches. Everything worked as described. Test passed.
          Hide
          Aparup Banerjee added a comment -

          Your issue has dug up some gold.
          It works great i've been told.
          Go forth, be brave, be bold.

          yay! "All your thoughts are belong to everyone."

          Thanks and ciao!

          Show
          Aparup Banerjee added a comment - Your issue has dug up some gold. It works great i've been told. Go forth, be brave, be bold. yay! "All your thoughts are belong to everyone." Thanks and ciao!

            People

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

              Dates

              • Created:
                Updated:
                Resolved: