Moodle

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 from Repository:
  • Pull Master Branch:
    MDL-34228-master
  • Pull Master Diff URL:

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

caused a regression

Bug - A problem which impairs or prevents Moodle from functioning correctly. MDL-37473 Completion blocks do not respect new isincompletionreports capability

  • Major - Major loss of function, incorrect output
  • Closed - The issue is considered finished, the resolution is correct. Issues which are closed should not be reopened.
is duplicated by

Bug - A problem which impairs or prevents Moodle from functioning correctly. MDL-34832 Course Completion Report - Teacher shows up in report

  • Minor - Minor loss of function where workaround is possible
  • Closed - The issue is considered finished, the resolution is correct. Issues which are closed should not be reopened.

Improvement - An enhancement to an existing Moodle feature. MDL-34946 Filter activity completion by roles

  • Minor - Minor loss of function where workaround is possible
  • Closed - The issue is considered finished, the resolution is correct. Issues which are closed should not be reopened.

Activity

Ascending order - Click to sort in descending order
Hide
Permalink
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
Permalink
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
Permalink
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
Permalink
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
Permalink
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
Permalink
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
Permalink
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
Permalink
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
Permalink
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
Permalink
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
Permalink
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
Permalink
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
Permalink
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
Permalink
Sam Marshall added a comment -

Rebased both branches.

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

Vote (8)
Watch (7)

Dates

  • Created:
    Updated:
    Resolved: