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

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

    Details

    • 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_28_STABLE, MOODLE_29_STABLE, MOODLE_30_STABLE
    • Fixed Branches:
      MOODLE_28_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.
            Hide
            cibot CiBoT added a comment -

            Moving this issue to current integration cycle, will be reviewed soon. Thanks for the hard work!

            Show
            cibot CiBoT added a comment - Moving this issue to current integration cycle, will be reviewed soon. Thanks for the hard work!
            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 MOODLE_28_STABLE (1 errors / 0 warnings) [branch: MDL-35027-B-28 | CI Job ] phplint (0/0) , php (0/0) , js (0/0) , css (0/0) , phpdoc (1/0) , commit (0/0) , savepoint (0/0) , thirdparty (0/0) , MOODLE_29_STABLE (1 errors / 0 warnings) [branch: MDL-35027-B-29 | CI Job ] phplint (0/0) , php (0/0) , js (0/0) , css (0/0) , phpdoc (1/0) , commit (0/0) , savepoint (0/0) , thirdparty (0/0) , master (1 errors / 0 warnings) [branch: MDL-35027-B | CI Job ] phplint (0/0) , php (0/0) , js (0/0) , css (0/0) , phpdoc (1/0) , commit (0/0) , savepoint (0/0) , thirdparty (0/0) , More information about this report
            Hide
            bostelm Henning Bostelmann added a comment -

            CiBoT found a problem in the phpdoc comment... I think I've fixed that now.

            Show
            bostelm Henning Bostelmann added a comment - CiBoT found a problem in the phpdoc comment... I think I've fixed that now.
            Hide
            cibot CiBoT added a comment -

            Code verified against automated checks.

            Checked MDL-35027 using repository: https://github.com/bostelm/moodle.git

            More information about this report

            Show
            cibot CiBoT added a comment - Code verified against automated checks. Checked MDL-35027 using repository: https://github.com/bostelm/moodle.git MOODLE_28_STABLE (0 errors / 0 warnings) [branch: MDL-35027-B-28 | CI Job ] MOODLE_29_STABLE (0 errors / 0 warnings) [branch: MDL-35027-B-29 | CI Job ] master (0 errors / 0 warnings) [branch: MDL-35027-B | CI Job ] More information about this report
            Hide
            stronk7 Eloy Lafuente (stronk7) added a comment -

            Side comment: While I like the filtering function I've doubts a generic MDL-48625 will be able to do the work (unless we construct module callbacks for it). My rationale is that there are other causes leading to user being filtered out. For example, for forums... users not having 'mod/forum:viewdiscussion' also should be filtered out. Or any other potential reason (that may be different in every module).

            In any case, apart from that side comment, I think that current code looks ok, just looking to it a bit more, running tests and proceeding to integrate it. Thanks!

            Show
            stronk7 Eloy Lafuente (stronk7) added a comment - Side comment: While I like the filtering function I've doubts a generic MDL-48625 will be able to do the work (unless we construct module callbacks for it). My rationale is that there are other causes leading to user being filtered out. For example, for forums... users not having 'mod/forum:viewdiscussion' also should be filtered out. Or any other potential reason (that may be different in every module). In any case, apart from that side comment, I think that current code looks ok, just looking to it a bit more, running tests and proceeding to integrate it. Thanks!
            Hide
            stronk7 Eloy Lafuente (stronk7) added a comment -

            Integrated (28, 29 & master), thanks!

            PS: I've added a extra commit adding one more scenario verifying the "Turn editing on" is not there anymore for forced-subscription forums. At the same time have get rid of the @javascript tag for the other scenario.

            https://git.moodle.org/gw?p=integration.git;a=commitdiff;h=ad9a7604f901f17dba56c0473956a78273e88a10

            Ciao

            Show
            stronk7 Eloy Lafuente (stronk7) added a comment - Integrated (28, 29 & master), thanks! PS: I've added a extra commit adding one more scenario verifying the "Turn editing on" is not there anymore for forced-subscription forums. At the same time have get rid of the @javascript tag for the other scenario. https://git.moodle.org/gw?p=integration.git;a=commitdiff;h=ad9a7604f901f17dba56c0473956a78273e88a10 Ciao
            Hide
            damyon Damyon Wiese added a comment -

            The testing instructions pass, but I'm not going to pass the test until I can get Andrew to comment on this point:

            I have some reservations about why this list is only filtered for forced subscriptions. E.g. can you explain why there is a difference in this list between "auto" and "forced" (and other modes)? Can you write a simple docs page that explains which users will show in this list?

            To me the use case of unsubscribing users before making a forum visible seems minor, and contrived compared to the much simpler to under stand logic of "Show/edit current subscribers list" === "The list of users who will receive posts".

            Show
            damyon Damyon Wiese added a comment - The testing instructions pass, but I'm not going to pass the test until I can get Andrew to comment on this point: I have some reservations about why this list is only filtered for forced subscriptions. E.g. can you explain why there is a difference in this list between "auto" and "forced" (and other modes)? Can you write a simple docs page that explains which users will show in this list? To me the use case of unsubscribing users before making a forum visible seems minor, and contrived compared to the much simpler to under stand logic of "Show/edit current subscribers list" === "The list of users who will receive posts".
            Hide
            damyon Damyon Wiese added a comment -

            Failing this for attention.

            Show
            damyon Damyon Wiese added a comment - Failing this for attention.
            Hide
            dobedobedoh Andrew Nicols added a comment -

            Hi Damyon,

            Thanks for raising this point.

            There are four modes:

            1. disabled -> No standard user can subscribe
            2. optional -> Any standard user can subscribe, but none are subscribed by default
            3. auto -> Any standard user can subscribe, and all are subscribed by default
            4. forced -> All users are subscribed. None can opt out.

            This changeset does two things:

            1. removes the "Turn editing on" button and prevents using editing mode of forced forums; and
            2. filters the list of users.

            I feel that the first change makes sense because the interface did not let you make changes to the list of users anyhow, and I feel that the second change makes sense because it should display a list of users currently subscribed.

            However, after re-reading this issue and playing with it for what feels like the thirtieth time, I've managed to unconvince myself of various bits of it (thanks Damyon Wiese).

            This is a list of forum subscribers, and I feel that although the changes may make sense in the wider scheme of things, on their own they do not.
            Longer term I think we need to display the lists such that:

            1. For all forum types we should list all users
            2. Where users are hidden, this should be indicated in the list
            3. When editing is on, all users should be listed (possibly with an indication)
            4. Editing should be removed where it does not make sense (force suscribed).

            Therefore, I'd say that this issue is partially correct, and partially goes too far.
            Filtering the list of users at this stage is incorrect IMO as we lose some of the information.

            Awaiting Eloy Lafuente (stronk7)'s thoughts too.

            Show
            dobedobedoh Andrew Nicols added a comment - Hi Damyon, Thanks for raising this point. There are four modes: disabled -> No standard user can subscribe optional -> Any standard user can subscribe, but none are subscribed by default auto -> Any standard user can subscribe, and all are subscribed by default forced -> All users are subscribed. None can opt out. This changeset does two things: removes the "Turn editing on" button and prevents using editing mode of forced forums; and filters the list of users. I feel that the first change makes sense because the interface did not let you make changes to the list of users anyhow, and I feel that the second change makes sense because it should display a list of users currently subscribed. However, after re-reading this issue and playing with it for what feels like the thirtieth time, I've managed to unconvince myself of various bits of it (thanks Damyon Wiese ). This is a list of forum subscribers, and I feel that although the changes may make sense in the wider scheme of things, on their own they do not. Longer term I think we need to display the lists such that: For all forum types we should list all users Where users are hidden, this should be indicated in the list When editing is on, all users should be listed (possibly with an indication) Editing should be removed where it does not make sense (force suscribed). Therefore, I'd say that this issue is partially correct, and partially goes too far. Filtering the list of users at this stage is incorrect IMO as we lose some of the information. Awaiting Eloy Lafuente (stronk7) 's thoughts too.
            Hide
            stronk7 Eloy Lafuente (stronk7) added a comment -

            Hi,

            1) About killing the "Turn editing on" button when the forum is forced subscription I think we all agree it's ok, as far as that mode is fully automated and manual edition has no sense.

            2) About hiding users when the forum is forced subscribed and cannot see hidden activities I also think it's ok, mainly because such a technique is used often by teachers to have their private forum in courses. And, those filtered out users really should not be there.

            So, IMO, the 2 changes to this issue had perfect sense for, fully automated, forced subscription forums.

            Said that, while I was integrating this I also had some objections/thoughts:

            A) This is slightly incorrect. To be real... those users should not be subscribed at all. I accept hiding them is a "workaround/visual" solution, but clearly they should not be subscribed.

            B) This is also slightly incomplete. As I commented above there are many other reasons, apart from the forum being hidden, that also should prevent users to become subscribed (say groupings, say access conditions, say permissions...). And none of them is being considered.

            C) Real problem is that we are subscribing basically to all users in the course and IMO automated (auto and forced modes) subscriptions should be more elaborated, always being recalculated when needed (course settings change, mainly). That means that \mod_forum\subscriptions::get_potential_subscribers() should be returning a more accurate list, verifying all the conditions are meet and both subscribing and unsubscribing users as needed (right now Moodle just adds new subscriptions all over the time, never unsubscribes anybody).

            D) As an alternative to C), we could continue using current "basically all users" are auto-subscribed (auto and forced modes) and then use a complete B) hiding/dimming (I don't mind) filtering machinery, considering all the causes for it to happen.

            So... summary:

            I) I think current change is ok. And covers a really common teacher's use case (using hidden forums for private discussions). That was the main principle I applied here to decide.

            II) This should be extended to both a) other subscription modes and b) other "hiding" causes. So, all subscribers lists do behave the same. This will lead to slower listing of subscribers for sure.

            III) I'm neutral about when we should be hiding or dimming those users. Personally I don't think we are missing/losing anything by hiding them. After all, those users are not subscribed in practice. But don't oppose to show them dimmed or so.

            IV) Ideally we should be calculating and applying a more accurate list of potential subscribers and differentiate the automatically subscribed ones and the manual ones in order to be able to unsubscribe them when conditions change. But maybe this is too much work for little practice benefit. Although it's how it should be. Right now we are being "lazy" by deferring the calculations to the "email moment".

            And... one line corollary:

            IMO this issue is a good step in the correct direction for a common use case, so +1 to keep it and also +1 to create a issue to expand this as commented above (I, II and III).

            Show
            stronk7 Eloy Lafuente (stronk7) added a comment - Hi, 1) About killing the "Turn editing on" button when the forum is forced subscription I think we all agree it's ok, as far as that mode is fully automated and manual edition has no sense. 2) About hiding users when the forum is forced subscribed and cannot see hidden activities I also think it's ok, mainly because such a technique is used often by teachers to have their private forum in courses. And, those filtered out users really should not be there. So, IMO, the 2 changes to this issue had perfect sense for, fully automated, forced subscription forums. Said that, while I was integrating this I also had some objections/thoughts: A) This is slightly incorrect. To be real... those users should not be subscribed at all. I accept hiding them is a "workaround/visual" solution, but clearly they should not be subscribed. B) This is also slightly incomplete. As I commented above there are many other reasons, apart from the forum being hidden, that also should prevent users to become subscribed (say groupings, say access conditions, say permissions...). And none of them is being considered. C) Real problem is that we are subscribing basically to all users in the course and IMO automated (auto and forced modes) subscriptions should be more elaborated, always being recalculated when needed (course settings change, mainly). That means that \mod_forum\subscriptions::get_potential_subscribers() should be returning a more accurate list, verifying all the conditions are meet and both subscribing and unsubscribing users as needed (right now Moodle just adds new subscriptions all over the time, never unsubscribes anybody). D) As an alternative to C), we could continue using current "basically all users" are auto-subscribed (auto and forced modes) and then use a complete B) hiding/dimming (I don't mind) filtering machinery, considering all the causes for it to happen. So... summary: I) I think current change is ok. And covers a really common teacher's use case (using hidden forums for private discussions). That was the main principle I applied here to decide. II) This should be extended to both a) other subscription modes and b) other "hiding" causes. So, all subscribers lists do behave the same. This will lead to slower listing of subscribers for sure. III) I'm neutral about when we should be hiding or dimming those users. Personally I don't think we are missing/losing anything by hiding them. After all, those users are not subscribed in practice. But don't oppose to show them dimmed or so. IV) Ideally we should be calculating and applying a more accurate list of potential subscribers and differentiate the automatically subscribed ones and the manual ones in order to be able to unsubscribe them when conditions change. But maybe this is too much work for little practice benefit. Although it's how it should be. Right now we are being "lazy" by deferring the calculations to the "email moment". And... one line corollary: IMO this issue is a good step in the correct direction for a common use case, so +1 to keep it and also +1 to create a issue to expand this as commented above (I, II and III).
            Hide
            bostelm Henning Bostelmann added a comment -

            Thanks Eloy; just to note that an issue for the other subscription modes / reasons for hiding exists: MDL-50857. I split it off from the present one since the solution may have performance impacts.

            Show
            bostelm Henning Bostelmann added a comment - Thanks Eloy; just to note that an issue for the other subscription modes / reasons for hiding exists: MDL-50857 . I split it off from the present one since the solution may have performance impacts.
            Hide
            bostelm Henning Bostelmann added a comment -

            Also, regarding Eloy's point 2), let me note that this use case, and the current behaviour there, is of real concern. We use these hidden "teacher forums" quite often, and I had several enquiries about an "urgent security problem" in Moodle because teachers looked at the subscribers list, and got the impression that students were receiving those confidential messages. The current patch fixes this problem.

            Show
            bostelm Henning Bostelmann added a comment - Also, regarding Eloy's point 2), let me note that this use case, and the current behaviour there, is of real concern. We use these hidden "teacher forums" quite often, and I had several enquiries about an "urgent security problem" in Moodle because teachers looked at the subscribers list, and got the impression that students were receiving those confidential messages. The current patch fixes this problem.
            Hide
            damyon Damyon Wiese added a comment -

            Ok - thanks for the comments - I agree with what was said by everyone above and I'm happy to pass this as it is a common use case - but with the followup issue to make these lists make sense regardless of the subscription type.

            Show
            damyon Damyon Wiese added a comment - Ok - thanks for the comments - I agree with what was said by everyone above and I'm happy to pass this as it is a common use case - but with the followup issue to make these lists make sense regardless of the subscription type.
            Hide
            dobedobedoh Andrew Nicols added a comment -

            Thanks for your contributions! This change is now available from the main moodle.git repository and will shortly be available on download.moodle.org.

            Show
            dobedobedoh Andrew Nicols added a comment - Thanks for your contributions! This change is now available from the main moodle.git repository and will shortly be available on download.moodle.org.

              People

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

                Dates

                • Created:
                  Updated:
                  Resolved:
                  Fix Release Date:
                  14/Sep/15