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.

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

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

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

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).

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

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.

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.

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

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.

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.

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...

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

Rebased both branches.

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

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

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.

Integrated to 23 and master.

Thanks Sam!

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

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!