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

      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.

        Gliffy Diagrams

          Issue Links

            Activity

            Hide
            tsala 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
            tsala 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
            quen 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
            quen 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
            oljapetrovic 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
            oljapetrovic 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
            sry_not4sale 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
            sry_not4sale 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
            quen 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
            quen 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
            quen Sam Marshall added a comment -

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

            Show
            quen Sam Marshall added a comment - New test script describing how it's supposed to work (patch coming soon).
            Hide
            quen 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
            quen 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
            quen 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
            quen 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
            mudrd8mz David Mudrak added a comment -

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

            Show
            mudrd8mz David Mudrak added a comment - Thanks Sam. I'll look at your solution and will leave a comment here. Enjoy the holiday!
            Hide
            mudrd8mz 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
            mudrd8mz 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
            mudrd8mz 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
            mudrd8mz 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
            quen 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
            quen 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
            poltawski 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
            poltawski 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
            quen Sam Marshall added a comment -

            Rebased both branches.

            Show
            quen Sam Marshall added a comment - Rebased both branches.
            Hide
            stronk7 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
            stronk7 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 CiBoT added a comment -

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

            Show
            cibot CiBoT added a comment - Moving this reopened issue out from current integration. Please, re-submit it for integration once ready.
            Hide
            quen 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
            quen 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
            poltawski Dan Poltawski added a comment -

            Integrated to 23 and master.

            Thanks Sam!

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

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

            Show
            abgreeve Adrian Greeve added a comment - Tested on the 2.3 and master integration branches. Everything worked as described. Test passed.
            Hide
            nebgor 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
            nebgor 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:
                  Fix Release Date:
                  12/Nov/12