Moodle

Deprecated implemention breaks subscription service when using custom roles.

Details

  • Type: Bug Bug
  • Status: Closed Closed
  • Priority: Critical Critical
  • Resolution: Fixed
  • Affects Version/s: 1.9.1
  • Fix Version/s: 1.9.4
  • Component/s: Forum
  • Labels:
    None
  • Database:
    MySQL
  • Affected Branches:
    MOODLE_19_STABLE
  • Fixed Branches:
    MOODLE_19_STABLE

Description

in mod/forum/subscription.php §144 uses

$users = get_course_users($course->id, 'firstname ASC, lastname ASC', $subscriberlist);

deprecated call. When using non standard roles that have capabilities for forum, no users are shown in subscription list.

following code :

$coursecontext = get_context_instance(CONTEXT_COURSE, $COURSE->id);
$users = get_users_by_capability($coursecontext, 'mod/forum:initialsubscriptions', '');

fixes the issue, at least as expected behaviour, but may have functionnal side effects I do not imagine.

do the get_group_users generate same problem ?

Issue Links

This issue will help resolve:
MDLSITE-618 dev.moodle.org needs update to fix forum email problems Critical Resolved
MDL-17673 No emails sent after upgrading to 1.9.3, I had to downgrade Blocker Closed
MDLSITE-590 no email from moodle.org for 24 hours and counting .. Major Closed

Activity

Ascending order - Click to sort in descending order
Hide
Permalink
Petr Škoda (skodak) added a comment -

The problem is that we should display only users with course:view privilege here, the patch would show also users not subscribed to course

Show
Petr Škoda (skodak) added a comment - The problem is that we should display only users with course:view privilege here, the patch would show also users not subscribed to course
Hide
Permalink
Valery Fremaux added a comment -

a double check of both capability check could'nt resolve that restriction ?

Show
Valery Fremaux added a comment - a double check of both capability check could'nt resolve that restriction ?
Hide
Permalink
Petr Škoda (skodak) added a comment -

checking course:view would solve it too, but it would be slow

Show
Petr Škoda (skodak) added a comment - checking course:view would solve it too, but it would be slow
Hide
Permalink
Tim Hunt added a comment -

The right file is actually mod/forum/subscribers.php, and in the latest 1.9.x it is line 145.

Show
Tim Hunt added a comment - The right file is actually mod/forum/subscribers.php, and in the latest 1.9.x it is line 145.
Hide
Permalink
Tim Hunt added a comment -

And to further clarify, this is to do with the interface where teachers can edit the list of subscribers.

Why are we talking about moodle:course/view here?

Surely, the logical thing is to list everyone in the 'Potential users' box who is allowed to subscribe to the forum. That would be everyone with mod/forum:viewdiscussion and without moodle/legacy:guest.

Hmm. OK, I can see the objection to that. It would piss of admins when teachers do Select all, Subscribe. Admins would end up subscribed to forums they don't care about.

So actually, this interface is really about teachers being able to force certain students to be subscribed - in a sense, it is a more selective version of 'Force everyone to be subscribed'. With that in mind, I think that Valery is right. Well, almost, the list of potential subscribers should be:

get_users_by_capability($FORUMcontext, 'mod/forum:initialsubscriptions', ..., $doanything = false);

OK, so that solves subscribing. Now what about un-subscribing? With the above policy, if an admin was subscribed to the forum, a teacher would be able to unsubscribe them, but not resubscribe them. (So would not be able to undo mistakes.)

Mind you, if an admin is subscribed to a forum, and then the teacher forces everyone to be subscribed, the the admin will stop receiving forum emails. What a mess.

Show
Tim Hunt added a comment - And to further clarify, this is to do with the interface where teachers can edit the list of subscribers. Why are we talking about moodle:course/view here? Surely, the logical thing is to list everyone in the 'Potential users' box who is allowed to subscribe to the forum. That would be everyone with mod/forum:viewdiscussion and without moodle/legacy:guest. Hmm. OK, I can see the objection to that. It would piss of admins when teachers do Select all, Subscribe. Admins would end up subscribed to forums they don't care about. So actually, this interface is really about teachers being able to force certain students to be subscribed - in a sense, it is a more selective version of 'Force everyone to be subscribed'. With that in mind, I think that Valery is right. Well, almost, the list of potential subscribers should be: get_users_by_capability($FORUMcontext, 'mod/forum:initialsubscriptions', ..., $doanything = false); OK, so that solves subscribing. Now what about un-subscribing? With the above policy, if an admin was subscribed to the forum, a teacher would be able to unsubscribe them, but not resubscribe them. (So would not be able to undo mistakes.) Mind you, if an admin is subscribed to a forum, and then the teacher forces everyone to be subscribed, the the admin will stop receiving forum emails. What a mess.
Hide
Permalink
Tim Hunt added a comment -

OK, so elsewhere in the forum code, get_course_users was changed to get_users_by_capability($COURSEcontext, 'mod/forum:initialsubscriptions', ...).

I guess we should just update the other bits of forum to be consistent with that. (After first deciding what the correct context is.)

The other place affected is forum_add_instance in forum/lib.php.

Show
Tim Hunt added a comment - OK, so elsewhere in the forum code, get_course_users was changed to get_users_by_capability($COURSEcontext, 'mod/forum:initialsubscriptions', ...). I guess we should just update the other bits of forum to be consistent with that. (After first deciding what the correct context is.) The other place affected is forum_add_instance in forum/lib.php.
Hide
Permalink
Tim Hunt added a comment -

The other place this was changed was for MDL-12979, and Dan says the context was just a mistake, not something subtle, so I will fix.

Show
Tim Hunt added a comment - The other place this was changed was for MDL-12979, and Dan says the context was just a mistake, not something subtle, so I will fix.
Hide
Permalink
Tim Hunt added a comment -

Done, at least as much as can be for now.

Show
Tim Hunt added a comment - Done, at least as much as can be for now.
Hide
Permalink
Martin Dougiamas added a comment -

I'm pretty sure this fix caused a regression which means forum mail is not being sent MDL-17673 and MDLSITE-590

Show
Martin Dougiamas added a comment - I'm pretty sure this fix caused a regression which means forum mail is not being sent MDL-17673 and MDLSITE-590
Hide
Permalink
Petr Škoda (skodak) added a comment -

testing a fix...

Show
Petr Škoda (skodak) added a comment - testing a fix...
Hide
Permalink
Eloy Lafuente (stronk7) added a comment -

re-updated moodle.org to latest 19_STABLE form CVS (with Petr's patch applied). Monitored cron execution. Mailout happened ok. Cool!

Ciao

Show
Eloy Lafuente (stronk7) added a comment - re-updated moodle.org to latest 19_STABLE form CVS (with Petr's patch applied). Monitored cron execution. Mailout happened ok. Cool! Ciao
Hide
Permalink
Eloy Lafuente (stronk7) added a comment -

Resolving as fixed. Ciao

Show
Eloy Lafuente (stronk7) added a comment - Resolving as fixed. Ciao
Hide
Permalink
Martin Dougiamas added a comment -

I also bumped the MOODLE_19_WEEKLY tag on mod/forum/lib.php and rebuilt the weeklies from that tag.

Show
Martin Dougiamas added a comment - I also bumped the MOODLE_19_WEEKLY tag on mod/forum/lib.php and rebuilt the weeklies from that tag.
Hide
Permalink
Tim Hunt added a comment -

Sorry everyone. Thank you for fixing my mess.

Show
Tim Hunt added a comment - Sorry everyone. Thank you for fixing my mess.

Dates

  • Created:
    Updated:
    Resolved:
Atlassian JIRA (v4.4.3#663-r165197) | Report a problem
Powered by a free Atlassian JIRA open source license for Moodle. Try JIRA - bug tracking software for your team.