Uploaded image for project: 'Moodle'
  1. Moodle
  2. MDL-35027

Hidden forum's subscribers list shows participants with no access to forum

    Details

    • Type: Bug
    • Status: Waiting for integration review
    • Priority: Major
    • Resolution: Unresolved
    • Affects Version/s: 2.2.4, 2.3.2, 2.7.4, 2.9
    • Fix Version/s: STABLE backlog
    • Component/s: Forum, Roles / Access
    • Labels:
    • Testing Instructions:
      Hide
      • Prerequisites: A course with a teacher (T) and a student (S) enrolled.
      • Create a forum in the course and set its subscription mode to "forced subscription". The forum should be set to "Visible: Show".
      • Go to the forum and click "Show/edit current subscribers".
      • VERIFY: Both T and S are shown in the list.
      • VERIFY: There is no button "Turn editing on" on the top-right of the page.
      • Go to "Forum administration -> Edit settings" and set the forum to "Visible: Hide".
      • Go to the forum and click "Show/edit current subscribers".
      • VERIFY: Only T is shown in the list (not S).
      • VERIFY: There is no button "Turn editing on" on the top-right of the page.
      Show
      Prerequisites: A course with a teacher (T) and a student (S) enrolled. Create a forum in the course and set its subscription mode to "forced subscription". The forum should be set to "Visible: Show". Go to the forum and click "Show/edit current subscribers". VERIFY: Both T and S are shown in the list. VERIFY: There is no button "Turn editing on" on the top-right of the page. Go to "Forum administration -> Edit settings" and set the forum to "Visible: Hide". Go to the forum and click "Show/edit current subscribers". VERIFY: Only T is shown in the list (not S). VERIFY: There is no button "Turn editing on" on the top-right of the page.
    • Affected Branches:
      MOODLE_22_STABLE, MOODLE_23_STABLE, MOODLE_27_STABLE, MOODLE_29_STABLE
    • Pull 2.8 Branch:
      MDL-35027-B-28
    • Pull 2.9 Branch:
      MDL-35027-B-29
    • Pull Master Branch:

      Description

      If a forum is set to be restricted to only users with the teacher role, the students can't see the forum. That works fine. But if the forum is set to "Forced subscription", then all the course participants are included in the subscribers list (mod/forum/subscribers.php).

      This is quite surprising for the teacher. Mailings do not actually go out to users who can't see the forum, but the teachers gets the impression that they do.

      The subscribers list should accurately reflect the participants who can access the forum.

      Replication steps:

      1. Log in as admin/teacher
      2. Navigate to a course that has students and teachers enrolled.
      3. Create a forum
        • set the subscription mode to Forced subscription
        • set the visibility to hidden
        • click Save and display
      4. Navigate to Settings > Forum admin > Show/edit current subscribers

      Expected result: Users unable to view the forum should be absent/separate/differentiated.

      Actual result: All enrolled users are subscribed and displayed in the same way.

        Gliffy Diagrams

          Issue Links

            Activity

            Hide
            salvetore Michael de Raadt added a comment -

            Hi.

            It is true that when a forum is created using 'Auto subscribe', all enrolled users will be subscribed to the forum (including later enrolments).

            However, messages are not sent to people who are not able to see the forum when it is hidden. Here is the result of running cron manually to process a post in a forum that is hidden...

            Processing module function forum_cron ...Processing user 3
            user 3 can not see 10
            Processing user 4
            user 4 can not see 10
            Processing user 5
            user 5 can not see 10
            Processing user 15
            Sending post 10: Another post
            1 users were sent post 10, 'Another post'

            So I'm confident that the messages being sent match the visibility of the forum at the time the messages are being sent.

            Show
            salvetore Michael de Raadt added a comment - Hi. It is true that when a forum is created using 'Auto subscribe', all enrolled users will be subscribed to the forum (including later enrolments). However, messages are not sent to people who are not able to see the forum when it is hidden. Here is the result of running cron manually to process a post in a forum that is hidden... Processing module function forum_cron ...Processing user 3 user 3 can not see 10 Processing user 4 user 4 can not see 10 Processing user 5 user 5 can not see 10 Processing user 15 Sending post 10: Another post 1 users were sent post 10, 'Another post' So I'm confident that the messages being sent match the visibility of the forum at the time the messages are being sent.
            Hide
            per.. Per Hessellund Laursen added a comment -

            Hi Michael,
            That's nice to know that the sending of mail respect the user role. But I still think that the subscribers list should match the mailing list. It makes no sense that the subscribers list don't show who will get the message by mail. What else is the list good for?
            Actually it gave me a huge chock to see subscribers list
            BR Per

            Show
            per.. Per Hessellund Laursen added a comment - Hi Michael, That's nice to know that the sending of mail respect the user role. But I still think that the subscribers list should match the mailing list. It makes no sense that the subscribers list don't show who will get the message by mail. What else is the list good for? Actually it gave me a huge chock to see subscribers list BR Per
            Hide
            salvetore Michael de Raadt added a comment -

            The problem is that the visibility of the activity can change over time. If you were to hide and then unhide the forum, you would loose the subscription information. I think this needs to be the case.

            What could help improve this would be to show more accurate information on the subscribers list page. Perhaps users who are subscribed but cannot see the forum should be displayed separately or in a different way. I will alter this issue to that effect.

            Show
            salvetore Michael de Raadt added a comment - The problem is that the visibility of the activity can change over time. If you were to hide and then unhide the forum, you would loose the subscription information. I think this needs to be the case. What could help improve this would be to show more accurate information on the subscribers list page. Perhaps users who are subscribed but cannot see the forum should be displayed separately or in a different way. I will alter this issue to that effect.
            Hide
            kwiliarty Kevin Wiliarty added a comment -

            In my testing I find that this bug, while still affecting the 2.7 branch, has been fixed in 2.8 and 2.9dev.

            Show
            kwiliarty Kevin Wiliarty added a comment - In my testing I find that this bug, while still affecting the 2.7 branch, has been fixed in 2.8 and 2.9dev.
            Hide
            rex Rex Lorenzo added a comment -

            Does MDL-36460 relate to this bug and does it fix this issue? In MDL-36460 it says it has been fixed in 2.7.3.

            Show
            rex Rex Lorenzo added a comment - Does MDL-36460 relate to this bug and does it fix this issue? In MDL-36460 it says it has been fixed in 2.7.3.
            Hide
            bostelm Henning Bostelmann added a comment -

            From my own tests, the problem is still present in 2.9+ (Build: 20150625).

            Show
            bostelm Henning Bostelmann added a comment - From my own tests, the problem is still present in 2.9+ (Build: 20150625).
            Hide
            bostelm Henning Bostelmann added a comment -

            After some investigation, the restrictions seem to be implemented in the current master as follows:

            • Subscriber lists, as displayed via mod/forum/subscribers.php, are filtered for some availability restrictions, including group/grouping restrictions.
            • They are not filtered for other availability restrictions, including date restrictions.
            • They are not filtered for the status of the "eye" icon (activity show/hide).
              The rationale seems to be that some of these restrictions can change very frequently (and one shouldn't filter for these) whereas others change seldomly (and for these filtering make sense). Note that this doesn't affect the actual notifications sent out, but only the way in which the subscriber list is displayed.

            For the expected behaviour, from my point of view: The current behaviour causes confusion for users at least where forums are permanently hidden ("teacher forums", usually force-subscribed, where the subscriber list is never edited manually). In these cases at least, users who can't see hidden activities should not be displayed in the subscriber list. In other cases, it may also be helpful to display subscribers who can't actually access the forum in a different way, say, grayed out.

            Proposed solution:

            (a) Minimal: Change the behaviour for force-subscribed hidden forums so that users unable to see hidden activities are not displayed in the subscriber list. This should be easy to implement and have no significant performance impact.

            (b) Complete: In addition, gray out any users on the subscriber list that are not currently able to access the forum, whatever the reason. This may have a significant performance impact, as it will likely involve complex per-user queries, but on the other hand this affects a page that is probably used very seldomly.

            I'm willing to develop a patch here, but before that, I would like to know what other people in this thread think. Looking forward to your comments...

            Show
            bostelm Henning Bostelmann added a comment - After some investigation, the restrictions seem to be implemented in the current master as follows: Subscriber lists, as displayed via mod/forum/subscribers.php, are filtered for some availability restrictions, including group/grouping restrictions. They are not filtered for other availability restrictions, including date restrictions. They are not filtered for the status of the "eye" icon (activity show/hide). The rationale seems to be that some of these restrictions can change very frequently (and one shouldn't filter for these) whereas others change seldomly (and for these filtering make sense). Note that this doesn't affect the actual notifications sent out, but only the way in which the subscriber list is displayed. For the expected behaviour, from my point of view: The current behaviour causes confusion for users at least where forums are permanently hidden ("teacher forums", usually force-subscribed, where the subscriber list is never edited manually). In these cases at least, users who can't see hidden activities should not be displayed in the subscriber list. In other cases, it may also be helpful to display subscribers who can't actually access the forum in a different way, say, grayed out. Proposed solution: (a) Minimal: Change the behaviour for force-subscribed hidden forums so that users unable to see hidden activities are not displayed in the subscriber list. This should be easy to implement and have no significant performance impact. (b) Complete: In addition, gray out any users on the subscriber list that are not currently able to access the forum, whatever the reason. This may have a significant performance impact, as it will likely involve complex per-user queries, but on the other hand this affects a page that is probably used very seldomly. I'm willing to develop a patch here, but before that, I would like to know what other people in this thread think. Looking forward to your comments...
            Hide
            bostelm Henning Bostelmann added a comment -

            I'm splitting this issue in two parts (along the lines described above).

            (1) For force-subscribed forums, the behaviour is clearly a bug. Displaying all users in the list, whether they can see the forum or not, is confusing and has no value to the teacher.

            I will handle this aspect in this issue; patch to follow in a moment.

            (2) For other modes of subscription, it's still confusing, but displaying the full list may be useful in some cases, for example if the teacher wants to edit the subscriber list before making the forum visible. In this case, it would be better to display a user as grayed-out (or similar) if they currently can't access the forum.

            I've opened a new improvement for this aspect (MDL-50857).

            Show
            bostelm Henning Bostelmann added a comment - I'm splitting this issue in two parts (along the lines described above). (1) For force-subscribed forums, the behaviour is clearly a bug. Displaying all users in the list, whether they can see the forum or not, is confusing and has no value to the teacher. I will handle this aspect in this issue; patch to follow in a moment. (2) For other modes of subscription, it's still confusing, but displaying the full list may be useful in some cases, for example if the teacher wants to edit the subscriber list before making the forum visible. In this case, it would be better to display a user as grayed-out (or similar) if they currently can't access the forum. I've opened a new improvement for this aspect ( MDL-50857 ).
            Hide
            bostelm Henning Bostelmann added a comment -

            I've added a patch and am asking for peer review.

            This patch concerns only forced-subscribed forums, and will limit the subscriber list of hidden forums to those users with the "moodle/course:viewhiddenactivies" capability.

            I've not added a behat test for the moment, but can do that if the functionality as such is deemed to be OK.

            Show
            bostelm Henning Bostelmann added a comment - I've added a patch and am asking for peer review. This patch concerns only forced-subscribed forums, and will limit the subscriber list of hidden forums to those users with the "moodle/course:viewhiddenactivies" capability. I've not added a behat test for the moment, but can do that if the functionality as such is deemed to be OK.
            Hide
            cibot CiBoT added a comment -
            Show
            cibot CiBoT added a comment - Fails against automated checks. Checked MDL-35027 using repository: https://github.com/bostelm/moodle.git master (1 errors / 0 warnings) [branch: MDL-35027 | CI Job ] phplint (0/0) , php (1/0) , js (0/0) , css (0/0) , phpdoc (0/0) , commit (0/0) , savepoint (0/0) , thirdparty (0/0) , More information about this report
            Hide
            bostelm Henning Bostelmann added a comment -

            CiBot, it wasn't me who introduced that method call...

            Show
            bostelm Henning Bostelmann added a comment - CiBot, it wasn't me who introduced that method call...
            Hide
            lameze Simey Lameze added a comment -

            Hi Henning Bostelmann, thanks for work on this.

            I'm still getting students and teacher on the forum subscribers list even after apply your patch. Am I missing something here? Perhaps more detailed testing instructions covering all possible scenarios would be appreciate (With or Without the hidden visibility). I guess would be nice to get a behat for it as well.

            Thanks.

            Show
            lameze Simey Lameze added a comment - Hi Henning Bostelmann , thanks for work on this. I'm still getting students and teacher on the forum subscribers list even after apply your patch. Am I missing something here? Perhaps more detailed testing instructions covering all possible scenarios would be appreciate (With or Without the hidden visibility). I guess would be nice to get a behat for it as well. Thanks.
            Hide
            bostelm Henning Bostelmann added a comment -

            Hello Simey

            thanks for checking the patch, and sorry that it didn't work out for you. When I follow the testing instructions, it works for me, though. The testing instructions - please see the very top of the page - cover both relevant cases: A hidden forum and a visible forum.

            At the moment, I can think only of two reasons why you see a different behaviour:

            (a) You might have chosen the default subscription mode instead of "Forced subscription". The current patch applies to "Forced subscription" mode only, cf. testing instructions.

            (b) You might have viewed the subscriber list with editing turned on, while I tested with editing turned off. There was in fact a bug around here, to the effect that in editing mode, the wrong subscriber list was shown. I have updated the code to fix this: Editing mode is now disabled in force-subscribed forums (as the subscriber list in such forums can never be edited).

            Also, I have now added a new scenario in the behat test "forum_subscriptions_availability.feature".

            If the patch still doesn't work for you, could you attach a screenshot of the forum subscriber list that you see, as well as of the forum settings screen?

            Best wishes
            Henning

            Show
            bostelm Henning Bostelmann added a comment - Hello Simey thanks for checking the patch, and sorry that it didn't work out for you. When I follow the testing instructions, it works for me, though. The testing instructions - please see the very top of the page - cover both relevant cases: A hidden forum and a visible forum. At the moment, I can think only of two reasons why you see a different behaviour: (a) You might have chosen the default subscription mode instead of "Forced subscription". The current patch applies to "Forced subscription" mode only, cf. testing instructions. (b) You might have viewed the subscriber list with editing turned on, while I tested with editing turned off. There was in fact a bug around here, to the effect that in editing mode, the wrong subscriber list was shown. I have updated the code to fix this: Editing mode is now disabled in force-subscribed forums (as the subscriber list in such forums can never be edited). Also, I have now added a new scenario in the behat test "forum_subscriptions_availability.feature". If the patch still doesn't work for you, could you attach a screenshot of the forum subscriber list that you see, as well as of the forum settings screen? Best wishes Henning
            Hide
            lameze Simey Lameze added a comment - - edited

            Hi Henning, thanks for your reply.
            Also, I would like to use:

            \mod_forum\subscriptions::is_forcesubscribed($forum) === false

            It's easier to understand and it's more strict than just use not

            !

            .
            I'm adding Andrew Nicols as he's the component lead, might have some feedback about your suggestion.

            Thanks.

            Show
            lameze Simey Lameze added a comment - - edited Hi Henning, thanks for your reply. Also, I would like to use: \mod_forum\subscriptions::is_forcesubscribed($forum) === false It's easier to understand and it's more strict than just use not ! . I'm adding Andrew Nicols as he's the component lead, might have some feedback about your suggestion. Thanks.
            Hide
            bostelm Henning Bostelmann added a comment -

            Hi Simey, I've updated the code accordingly. I've also added Andrew Nicols as watcher, as you suggested.

            Show
            bostelm Henning Bostelmann added a comment - Hi Simey, I've updated the code accordingly. I've also added Andrew Nicols as watcher, as you suggested.
            Hide
            dobedobedoh Andrew Nicols added a comment -

            Hi both,

            Thanks for working on this issue - I'd seen it popping by in my e-mail feed over the past week but hadn't yet had a chance to comment.

            For some reason, I had it in my mind that we'd already done this, but so far it's only completed for availability changes. Marina Glancy raised MDL-48625 to handle this in a core location, and this issue should ideally use that functionality, but it isn't complete and Marina doesn't think that she'll have any time to work on it any time soon.

            I'm in two minds as to whether this is the correct solution at present:
            On the one hand, it does solve the issue and can the code change in get_potential_subscribers() can eventually be replaced with a call to the code not-yet released in MDL-48625 (just missing a TODO) and we're all good.
            On the other hand, I feel that the filtering of users in this particular manner does not belong in the mod_forum\subscription class at all. Technically the users are subscribed to the forum, but they cannot yet see it. This is evident in the fact that we want to only filter the list when in view mode, and we want the unfiltered list when editing. In this regard, we want to apply general filtering outside of the subscription code. There's currently one place in mod_forum\subscriptions() that we do check the visibility (get_unsubscribable_forums), and I don't feel that it really should be there.

            I'm afraid that I'm leaning more to the other hand. There is only one time that we filter here, and this kind of filtering really does belong in core - not in a module. It technically is not related to subscription itself, but more the expected view of the subscription in certain circumstances.

            So in summary, the change as a whole looks reasonable, but I'm not convinced that it is quite right. I feel that this filtering should be performed outside of the subscriptions class, and ideally should use a core function (as discussed in MDL-48625).

            I think that moving forward we should create a new function within mod/forum/subscribers.php which takes the $cm, the $context, and the list of users and spits out a filtered list of users. This removes the need to make any API changes to the mod_forum\subscription() API. It should ideally include a TODO to change the call once MDL-48625 has been integrated (with a note in the issue description for that issue too).

            What do you both think?

            Hope that I haven't complicated things too much - thank you for working on this issue!

            Andrew

            Show
            dobedobedoh Andrew Nicols added a comment - Hi both, Thanks for working on this issue - I'd seen it popping by in my e-mail feed over the past week but hadn't yet had a chance to comment. For some reason, I had it in my mind that we'd already done this, but so far it's only completed for availability changes. Marina Glancy raised MDL-48625 to handle this in a core location, and this issue should ideally use that functionality, but it isn't complete and Marina doesn't think that she'll have any time to work on it any time soon. I'm in two minds as to whether this is the correct solution at present: On the one hand, it does solve the issue and can the code change in get_potential_subscribers() can eventually be replaced with a call to the code not-yet released in MDL-48625 (just missing a TODO) and we're all good. On the other hand, I feel that the filtering of users in this particular manner does not belong in the mod_forum\subscription class at all. Technically the users are subscribed to the forum, but they cannot yet see it. This is evident in the fact that we want to only filter the list when in view mode, and we want the unfiltered list when editing. In this regard, we want to apply general filtering outside of the subscription code. There's currently one place in mod_forum\subscriptions() that we do check the visibility (get_unsubscribable_forums), and I don't feel that it really should be there. I'm afraid that I'm leaning more to the other hand. There is only one time that we filter here, and this kind of filtering really does belong in core - not in a module. It technically is not related to subscription itself, but more the expected view of the subscription in certain circumstances. So in summary, the change as a whole looks reasonable, but I'm not convinced that it is quite right. I feel that this filtering should be performed outside of the subscriptions class, and ideally should use a core function (as discussed in MDL-48625 ). I think that moving forward we should create a new function within mod/forum/subscribers.php which takes the $cm, the $context, and the list of users and spits out a filtered list of users. This removes the need to make any API changes to the mod_forum\subscription() API. It should ideally include a TODO to change the call once MDL-48625 has been integrated (with a note in the issue description for that issue too). What do you both think? Hope that I haven't complicated things too much - thank you for working on this issue! Andrew
            Hide
            bostelm Henning Bostelmann added a comment -

            Hi Andrew

            I agree that this functionality should be provided in core, but certainly didn't want to touch the core availability API for this change.

            Your solution sounds fine and I can implement that.

            Just to confirm, do you agree that in a force-subscribed forum the teacher should not be able to navigate to the "edit subscriber list" screen?

            Show
            bostelm Henning Bostelmann added a comment - Hi Andrew I agree that this functionality should be provided in core, but certainly didn't want to touch the core availability API for this change. Your solution sounds fine and I can implement that. Just to confirm, do you agree that in a force-subscribed forum the teacher should not be able to navigate to the "edit subscriber list" screen?
            Hide
            dobedobedoh Andrew Nicols added a comment -

            Hi Henning,

            Thanks for continuing to work on this

            I agree with the idea - there's very little benefit to showing the selection list when you can't make any changes.

            Thanks,

            Andrew

            Show
            dobedobedoh Andrew Nicols added a comment - Hi Henning, Thanks for continuing to work on this I agree with the idea - there's very little benefit to showing the selection list when you can't make any changes. Thanks, Andrew
            Hide
            bostelm Henning Bostelmann added a comment -

            Hi Andrew

            I've made the changes as discussed - please have a look.

            Show
            bostelm Henning Bostelmann added a comment - Hi Andrew I've made the changes as discussed - please have a look.
            Hide
            lameze Simey Lameze added a comment - - edited

            Hi Henning Bostelmann, I had a second look on your patch and spoke with Andrew about the current patch. I think that placement of that mod_forum_filter_hidden_users() really wired, specially for those who's not familiar with that area.
            I would suggest that you move that function to the bottom of the page.

            Could you please change that so we can move this issue for integration?

            Thanks.

            Show
            lameze Simey Lameze added a comment - - edited Hi Henning Bostelmann , I had a second look on your patch and spoke with Andrew about the current patch. I think that placement of that mod_forum_filter_hidden_users() really wired, specially for those who's not familiar with that area. I would suggest that you move that function to the bottom of the page. Could you please change that so we can move this issue for integration? Thanks.
            Hide
            bostelm Henning Bostelmann added a comment -

            Thanks Simey Lameze. I've moved the function to the end.

            If backports of the bugfix are needed for integration, please let me know.

            Show
            bostelm Henning Bostelmann added a comment - Thanks Simey Lameze . I've moved the function to the end. If backports of the bugfix are needed for integration, please let me know.
            Hide
            lameze Simey Lameze added a comment -

            Thanks for change that Henning, would be good if you could provide branches for 2.9 and 2.8 as well.

            Show
            lameze Simey Lameze added a comment - Thanks for change that Henning, would be good if you could provide branches for 2.9 and 2.8 as well.
            Hide
            bostelm Henning Bostelmann added a comment -

            Added backports for 2.8 and 2.9.

            Show
            bostelm Henning Bostelmann added a comment - Added backports for 2.8 and 2.9.
            Hide
            dobedobedoh Andrew Nicols added a comment -

            Thanks Henning, you're a star

            Show
            dobedobedoh Andrew Nicols added a comment - Thanks Henning, you're a star
            Hide
            lameze Simey Lameze added a comment -

            Well done Henning Bostelmann, I'm pushing for integration review now!

            Thanks.

            Show
            lameze Simey Lameze added a comment - Well done Henning Bostelmann , I'm pushing for integration review now! Thanks.

              People

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

                Dates

                • Created:
                  Updated: