Moodle
  1. Moodle
  2. MDL-1626

Forum: Per-discussion subscription

    Details

    • Type: New Feature New Feature
    • Status: Closed
    • Priority: Critical Critical
    • Resolution: Fixed
    • Affects Version/s: 1.9, 2.0.1, 2.6, 2.8
    • Fix Version/s: 2.8
    • Component/s: Forum
    • Labels:
    • Environment:
      All
    • Testing Instructions:
      Hide

      There are fairly extensive unit tests which accompany this issue, and behat tests which cover a selection of it.

      The tests currently do not cover:

      • moving discussions between forums; or
      • backup/restore.
      Moving discussions
      1. create a forum with a subscription mode of Auto and a name of "Auto forum"
      2. post a discussion in the forum with the subject "Automatic discussion"
        • Confirm that you are subscribed to the post
      3. create a forum with a subscription mode of Optional and a name of "Optional forum"
      4. post a discussion in the forum with the subject "Optional discussion"
        • Confirm that you are not subscribed to the post
      5. View the "Optional discussion" discussion
      6. Move the discussion to the Automatic forum
      7. View the Automatic forum threadlist
        • Confirm that you are:
        • *subscribed to the "Automatic discussion"
        • *unsubscribed to the "Optional discussion"
      8. View "Automatic discussion"
      9. Move the discussion to the Optional forum
      10. View the Optional forum threadlist
        • *Confirm that you are subscribed to "Automatic discussion"
      11. View "Automatic discussion" again
      12. Move the discussion back to the Automatic forum
      13. View the Automatic forum threadlist
        • Confirm that you are:
        • *subscribed to the "Automatic discussion"
        • *unsubscribed to the "Optional discussion"
      14. View the "Optional discussion" discussion
      15. Move the discussion back to the Optional forum
      16. View the Optional forum threadlist
        • *Confirm that you are unsubscribed from "Optional discussion"
      17. Subscribe yourself to the "Optional discussion" discussion
      18. View the Automatic forum threadlist
        • *Confirm that you are subscribed to "Automatic discussion"
      19. Unsubscribe yourself from the "Automatic discussion" discussion
      20. View the "Optional discussion" discussion (which you're now subscribed to)
      21. Move the discussion to the Automatic forum
      22. View the Automatic forum threadlist
        • Confirm that you are:
        • *unsubscribed from the "Automatic discussion"
        • *subscribed to the "Optional discussion"
      23. View "Automatic discussion" (which you're no longer subscribed to)
      24. Move the discussion to the Optional forum
      25. View the Optional forum threadlist
        • *Confirm that you are now unsubscribed from "Automatic discussion"
      26. View "Automatic discussion" again
      27. Move the discussion back to the Automatic forum
      28. View the Automatic forum threadlist
        • Confirm that you are:
        • *unsubscribed from the "Automatic discussion"
        • *subscribed to the "Optional discussion"
      Backup/restore
      1. Create a forum in optional subscription mode
      2. Create two discussions within it
      3. As student1:
        • Subscribe to discussion 1
      4. As student2:
        • Subscribe to discussion 2
      5. As student3:
        • Subscribe to the forum
      6. As student4:
        • Subscribe to the forum
        • Unsubscribe from discussion 1
      7. As student5:
        • Subscribe to the forum
        • Unsubscribe from discussion 2
      8. As admin, backup the forum including user data
      9. Restore the forum
      10. For each of the users above: Confirm that the subscription data was copied over correctly

      You should also move around the UI to check that it behaves as expected. Pay particular notice to:

      • the discussion list - you should be able to toggle the discussion subscription from the discussions and have them update from one to other
      • writing a new post - this displays information about your current subscription. It also defaults to subscribing you to the discussion when you post any message to that discussion
      Show
      There are fairly extensive unit tests which accompany this issue, and behat tests which cover a selection of it. The tests currently do not cover: moving discussions between forums; or backup/restore. Moving discussions create a forum with a subscription mode of Auto and a name of "Auto forum" post a discussion in the forum with the subject "Automatic discussion" Confirm that you are subscribed to the post create a forum with a subscription mode of Optional and a name of "Optional forum" post a discussion in the forum with the subject "Optional discussion" Confirm that you are not subscribed to the post View the "Optional discussion" discussion Move the discussion to the Automatic forum View the Automatic forum threadlist Confirm that you are: *subscribed to the "Automatic discussion" *unsubscribed to the "Optional discussion" View "Automatic discussion" Move the discussion to the Optional forum View the Optional forum threadlist *Confirm that you are subscribed to "Automatic discussion" View "Automatic discussion" again Move the discussion back to the Automatic forum View the Automatic forum threadlist Confirm that you are: *subscribed to the "Automatic discussion" *unsubscribed to the "Optional discussion" View the "Optional discussion" discussion Move the discussion back to the Optional forum View the Optional forum threadlist *Confirm that you are unsubscribed from "Optional discussion" Subscribe yourself to the "Optional discussion" discussion View the Automatic forum threadlist *Confirm that you are subscribed to "Automatic discussion" Unsubscribe yourself from the "Automatic discussion" discussion View the "Optional discussion" discussion (which you're now subscribed to) Move the discussion to the Automatic forum View the Automatic forum threadlist Confirm that you are: *unsubscribed from the "Automatic discussion" *subscribed to the "Optional discussion" View "Automatic discussion" (which you're no longer subscribed to) Move the discussion to the Optional forum View the Optional forum threadlist *Confirm that you are now unsubscribed from "Automatic discussion" View "Automatic discussion" again Move the discussion back to the Automatic forum View the Automatic forum threadlist Confirm that you are: *unsubscribed from the "Automatic discussion" *subscribed to the "Optional discussion" Backup/restore Create a forum in optional subscription mode Create two discussions within it As student1: Subscribe to discussion 1 As student2: Subscribe to discussion 2 As student3: Subscribe to the forum As student4: Subscribe to the forum Unsubscribe from discussion 1 As student5: Subscribe to the forum Unsubscribe from discussion 2 As admin, backup the forum including user data Restore the forum For each of the users above: Confirm that the subscription data was copied over correctly You should also move around the UI to check that it behaves as expected. Pay particular notice to: the discussion list - you should be able to toggle the discussion subscription from the discussions and have them update from one to other writing a new post - this displays information about your current subscription. It also defaults to subscribing you to the discussion when you post any message to that discussion
    • Affected Branches:
      MOODLE_19_STABLE, MOODLE_20_STABLE, MOODLE_26_STABLE, MOODLE_28_STABLE
    • Fixed Branches:
      MOODLE_28_STABLE
    • Pull Master Branch:
      MDL-1626-master
    • Story Points (Obsolete):
      100
    • Sprint:
      FRONTEND Sprint 12

      Description

      I am finding that my inbox is absolutely flooded with unwanted moodle mails off the forum script. This is very annoying and I know it will similarly annoy my users. In fact it will put them off the system.

      The problem arises because unlike other discussion boards moodle messages are emailed to me from the entire forum, not just the thread I have started or responded to. The overwhelming of people's inboxes is a serious business: it could be said that moodle is generating spam because the email feature is so unfocused.

      Is there any chance this is going to be improved please?

        Gliffy Diagrams

        1. 1626.gitpatch
          18 kB
          Sam Hemelryk
        2. coverage.zip
          871 kB
          Andrew Nicols
        3. moodle 1.9.3 (chk)-diff-2009-01-31-20-20-44.patch
          55 kB
          John White
        4. moodle 1.9.4 (chk)-diff-2009-02-21-16-14-30.patch
          111 kB
          John White
        5. moodle 1.9.4 (chk)-diff-2009-02-23-00-00-16.patch
          111 kB
          John White
        6. Per-forum subscription.pdf
          48 kB
          Andrew Nicols
        7. thread_subscribe1.bmml
          12 kB
          John White
        8. thread_subscribe1.bmml
          12 kB
          John White
        9. threadsubscribingforum_2.5.0.patch
          101 kB
          John White
        10. ThreadSubscriptions.pdf
          338 kB
          John White
        1. discussion-view.png
          30 kB
        2. Per-forum subscription.png
          87 kB
        3. subscribed-forum-newpost.png
          58 kB
        4. thread_subscribe1.png
          122 kB
        5. threadlist.png
          26 kB
        6. unsubscribed-forum-newpost.png
          58 kB

          Issue Links

            Activity

            Hide
            Mary Cooch added a comment -

            Removing the docs_required label as this is documented in https://docs.moodle.org/28/en/Forum_settings and https://docs.moodle.org/28/en/Forum_FAQ

            Show
            Mary Cooch added a comment - Removing the docs_required label as this is documented in https://docs.moodle.org/28/en/Forum_settings and https://docs.moodle.org/28/en/Forum_FAQ
            Hide
            Mary Cooch added a comment - - edited

            Removing the qa_test_required label as there are now 3 basic manual QA tests MDLQA-7153 and MDLQA-7155 and MDLQA-7168

            Show
            Mary Cooch added a comment - - edited Removing the qa_test_required label as there are now 3 basic manual QA tests MDLQA-7153 and MDLQA-7155 and MDLQA-7168
            Hide
            Dan Poltawski added a comment -

            This issue added upgrade steps using 2014051202 - which is not diverged from the stable version number on 2.7 - which is currently 2014051201.

            This means that if anyone increments the version number on 27_STABLE for mod_forum, the discussion subscriptions stuff won't upgrade properly at all (funnily enough its the exact scenario mentioned in https://docs.moodle.org/dev/Moodle_versions#Why_we_branch_versions).

            I have created MDL-46885 to address this.

            Show
            Dan Poltawski added a comment - This issue added upgrade steps using 2014051202 - which is not diverged from the stable version number on 2.7 - which is currently 2014051201. This means that if anyone increments the version number on 27_STABLE for mod_forum, the discussion subscriptions stuff won't upgrade properly at all (funnily enough its the exact scenario mentioned in https://docs.moodle.org/dev/Moodle_versions#Why_we_branch_versions ). I have created MDL-46885 to address this.
            Hide
            Eloy Lafuente (stronk7) added a comment -

            It's always a pleasure to close these issues. Their fixes are now upstream. Great work!

            Ciao

            Show
            Eloy Lafuente (stronk7) added a comment - It's always a pleasure to close these issues. Their fixes are now upstream. Great work! Ciao
            Hide
            Jetha Chan added a comment -

            Thanks for working on this, Andrew! Tests passed on master.

            Show
            Jetha Chan added a comment - Thanks for working on this, Andrew! Tests passed on master.
            Hide
            Marina Glancy added a comment -

            Thanks Andrew, integrated, back to testing

            Show
            Marina Glancy added a comment - Thanks Andrew, integrated, back to testing
            Hide
            Andrew Nicols added a comment -

            I've pushed a fix for that. It was a mysqlism - I'd referenced u.id in a subquery where u is a table in the parent query. It should have been u2.id.

            Pushed an additional commit to the same branch.

            Thanks,

            Andrew

            Show
            Andrew Nicols added a comment - I've pushed a fix for that. It was a mysqlism - I'd referenced u.id in a subquery where u is a table in the parent query. It should have been u2.id. Pushed an additional commit to the same branch. Thanks, Andrew
            Hide
            Marina Glancy added a comment -

            Andrew, there are (somehow, despite all our runs) multiple phpunit failures, see
            http://integration.moodle.org/job/20.%20Run%20phpunit%20UnitTests%20%28master%29/3029/

            Show
            Marina Glancy added a comment - Andrew, there are (somehow, despite all our runs) multiple phpunit failures, see http://integration.moodle.org/job/20.%20Run%20phpunit%20UnitTests%20%28master%29/3029/
            Hide
            Marina Glancy added a comment -

            Thanks Andrew, integrated in master

            Show
            Marina Glancy added a comment - Thanks Andrew, integrated in master
            Hide
            Andrew Nicols added a comment -

            Okay, thanks Marina. I've force-pushed an update to the branch.

            1) As discussed I've not made this change for the moment. I think this should be done in a separate issue as there could be places we call some of these functions when we don't have the full course object (e.g. get_unsubscribable_forums did not contain the course field).
            2) Again, I'll defer that to a separate issue as the code in question has just been moved into a new class and renamed
            3) Same again... (sorry)
            4) Good catch - I've sorted that string out, and added an AMOS MOV command for the rename
            5) As mentioned, I've discussed this at length with Sam. For the most part, the cache is rarely touched by ordinary users. When viewing a forum it is used a little, when viewing a discussion it is barely touched. The big wins are in:

            • moving discussions between forums - this saves 2*user count queries as I recall; and
            • cron.
              For the first, then yes... a request cache may make some sense but I'm still not convinced;
              For the cron, then no... a request cache is bad. We'd have to keep fetching the cache and querying it which gets very expensive when considering larger forums.
              Additionally any change to forum subscriptions would get extremely expensive too.
              Unit tests are not a case for using MUC.

            Hope that's all good. Thanks!

            Andrew

            Show
            Andrew Nicols added a comment - Okay, thanks Marina. I've force-pushed an update to the branch. 1) As discussed I've not made this change for the moment. I think this should be done in a separate issue as there could be places we call some of these functions when we don't have the full course object (e.g. get_unsubscribable_forums did not contain the course field). 2) Again, I'll defer that to a separate issue as the code in question has just been moved into a new class and renamed 3) Same again... (sorry) 4) Good catch - I've sorted that string out, and added an AMOS MOV command for the rename 5) As mentioned, I've discussed this at length with Sam. For the most part, the cache is rarely touched by ordinary users. When viewing a forum it is used a little, when viewing a discussion it is barely touched. The big wins are in: moving discussions between forums - this saves 2*user count queries as I recall; and cron. For the first, then yes... a request cache may make some sense but I'm still not convinced; For the cron, then no... a request cache is bad. We'd have to keep fetching the cache and querying it which gets very expensive when considering larger forums. Additionally any change to forum subscriptions would get extremely expensive too. Unit tests are not a case for using MUC. Hope that's all good. Thanks! Andrew
            Hide
            Marina Glancy added a comment -

            Andrew pointed out to me the Sam's comment above about using MUC. Well, up to you guys. From my point of view forum code is so performance-unwise in terms of unnecessary and duplicating DB queries, that MUC static cache access overhead would be absolutely invisible but make the code more readable. But I'm not going to hold integration because of that.

            Show
            Marina Glancy added a comment - Andrew pointed out to me the Sam's comment above about using MUC. Well, up to you guys. From my point of view forum code is so performance-unwise in terms of unnecessary and duplicating DB queries, that MUC static cache access overhead would be absolutely invisible but make the code more readable. But I'm not going to hold integration because of that.
            Hide
            Marina Glancy added a comment -

            ok, I continue

            4. https://github.com/andrewnicols/moodle/commit/0b6f20849ae885a30bb0ed1927e4b508c14f9ac4 - please add AMOS tag for renaming string; also disallowsubscription_help duplicates disallowsubscription
            5. you really should not be using static variables and use MUC instead. There is an option there for request cache which is performance-wise identical to the static variable. You won't need setup/teardown in unittests then. Grep code for cache_store::MODE_REQUEST

            I did not check every line of code, it's too big. But I really like the way forum code looks now and good test coverage!

            Show
            Marina Glancy added a comment - ok, I continue 4. https://github.com/andrewnicols/moodle/commit/0b6f20849ae885a30bb0ed1927e4b508c14f9ac4 - please add AMOS tag for renaming string; also disallowsubscription_help duplicates disallowsubscription 5. you really should not be using static variables and use MUC instead. There is an option there for request cache which is performance-wise identical to the static variable. You won't need setup/teardown in unittests then. Grep code for cache_store::MODE_REQUEST I did not check every line of code, it's too big. But I really like the way forum code looks now and good test coverage!
            Hide
            Marina Glancy added a comment -

            Right, you did such a big reshuffling of the code that I keep reviewing the old code thinking it's new

            Show
            Marina Glancy added a comment - Right, you did such a big reshuffling of the code that I keep reviewing the old code thinking it's new
            Hide
            Andrew Nicols added a comment -

            1) That can be arranged, but should possibly be a different issue so it can be backported?
            2) admins are not enrolled in the course so cannot subscribe
            3) True, though again a different issue perhaps?

            Show
            Andrew Nicols added a comment - 1) That can be arranged, but should possibly be a different issue so it can be backported? 2) admins are not enrolled in the course so cannot subscribe 3) True, though again a different issue perhaps?
            Hide
            Marina Glancy added a comment -

            Hi Andrew, thanks for fixing conflicts.
            You did a great job! It's a lot of code to review, so I might throw my comments here several at a time:

            1. Funciton \mod_forum\subscriptions::is_subscribed() should call get_coursemodule_from_instance() with courseid when it is available in $forum->course, it will be much faster.
            2. Not related to your change but it looks like has_capability('mod/forum:allowforcesubscribe', context_module::instance($cm->id), $userid) should have $doanything=false, or otherwise there is no options for admins to opt-out. But it's out of scope I guess
            3. get_unsubscribable_forums() - again, I can see that this is old code. Very strange check for course module visibility, it should be either excluded or $cm->uservisible should be checked.

            Show
            Marina Glancy added a comment - Hi Andrew, thanks for fixing conflicts. You did a great job! It's a lot of code to review, so I might throw my comments here several at a time: 1. Funciton \mod_forum\subscriptions::is_subscribed() should call get_coursemodule_from_instance() with courseid when it is available in $forum->course, it will be much faster. 2. Not related to your change but it looks like has_capability('mod/forum:allowforcesubscribe', context_module::instance($cm->id), $userid) should have $doanything=false, or otherwise there is no options for admins to opt-out. But it's out of scope I guess 3. get_unsubscribable_forums() - again, I can see that this is old code. Very strange check for course module visibility, it should be either excluded or $cm->uservisible should be checked.
            Hide
            Andrew Nicols added a comment -

            FYI, mod_forum phpunit coverage report attached.

            Show
            Andrew Nicols added a comment - FYI, mod_forum phpunit coverage report attached.
            Hide
            Andrew Nicols added a comment -

            Thanks Marina,

            I've fixed that now. I messed up the function signature when resolving a rebase conflict, and I'd not run the full unit test suite for a few days and had inadvertently deleted the s of delete_records somehow.

            Have run the full phpunit suite, and all mod_forum behat tests - all now passing,

            Andrew

            Show
            Andrew Nicols added a comment - Thanks Marina, I've fixed that now. I messed up the function signature when resolving a rebase conflict, and I'd not run the full unit test suite for a few days and had inadvertently deleted the s of delete_records somehow. Have run the full phpunit suite, and all mod_forum behat tests - all now passing, Andrew
            Hide
            Marina Glancy added a comment -

            ... and behat error

            (::) failed steps (::)
             
            01. PHP debug message/s found:
                
                Notice: Undefined variable: forum in /home/marina/repositories/master/moodle/mod/forum/lib.php on line 6813
                
                
                Notice: Trying to get property of non-object in /home/marina/repositories/master/moodle/mod/forum/classes/subscriptions.php on line 150
                In step `Given I follow "Test forum name"'.                   # behat_general::click_link()
                From scenario `Add a forum and a discussion attaching files'. # /home/marina/repositories/master/moodle/mod/forum/tests/behat/add_forum.feature:8
                Of feature `Add forum activities and discussions'.            # /home/marina/repositories/master/moodle/mod/forum/tests/behat/add_forum.feature
            
            

            Show
            Marina Glancy added a comment - ... and behat error (::) failed steps (::)   01. PHP debug message/s found: Notice: Undefined variable: forum in /home/marina/repositories/master/moodle/mod/forum/lib.php on line 6813 Notice: Trying to get property of non-object in /home/marina/repositories/master/moodle/mod/forum/classes/subscriptions.php on line 150 In step `Given I follow "Test forum name"'. # behat_general::click_link() From scenario `Add a forum and a discussion attaching files'. # /home/marina/repositories/master/moodle/mod/forum/tests/behat/add_forum.feature:8 Of feature `Add forum activities and discussions'. # /home/marina/repositories/master/moodle/mod/forum/tests/behat/add_forum.feature
            Hide
            Marina Glancy added a comment -

            Hi Andrew,
            I'm still reviewing but I already have a phpunit error:

            PHP Fatal error:  Call to undefined method pgsql_native_moodle_database::delete_record() in /home/marina/repositories/int_master/moodle/mod/forum/lib.php on line 4312
            

            Show
            Marina Glancy added a comment - Hi Andrew, I'm still reviewing but I already have a phpunit error: PHP Fatal error: Call to undefined method pgsql_native_moodle_database::delete_record() in /home/marina/repositories/int_master/moodle/mod/forum/lib.php on line 4312
            Hide
            CiBoT added a comment -

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

            Show
            CiBoT added a comment - Moving this issue to current integration cycle, will be reviewed soon. Thanks for the hard work!
            Hide
            Andrew Nicols added a comment -

            Submitting for integration.

            Show
            Andrew Nicols added a comment - Submitting for integration.
            Hide
            Sam Hemelryk added a comment -

            Just chiming in after Andrew messaged me in regards to caching as the singleton.

            First up caching. Andrew is spot on in summarising my viewing on using MUC for this.
            Using MUC for request caches adds overhead and must come with immediate value. That value usually comes in the form of a cache that is shared amongst API's, or may require invalidation in more than one API in such a way that it cannot be assumed or controlled from any single place.
            Last I looked at the caches here that wasn't the cache.
            On a very general note: cache event invalidation can be a risky thing with older parts of Moodle. I'm not sure how forum stands truthfully, but normally before going down this road I would strongly suggest only invalidating by events from API's. All interaction with something through an API passes through a single point somewhere in the process which is where you would trigger your invalidation. Without the API you may find that its impossible to be sure you trigger the invalidation in all places, especially for older code like forum where it was originally acceptable to assume forum was present and interact with them in other code (obviously not the case now).

            In regards to the singleton idea what I said to Andrew could be summarised by saying it is just a matter of time and effort.
            I agree that if time wasn't an issue a better approach would be to look at singleton class for the core forum concepts and supporting static helper classes for bulk actions.
            However I believe we are trying to get forum improvements achieved within sprints and refrain from significant modification and Petr'esq rewriting of the Forum at this stage.
            The inconsistency between forumid and forum caught my eye while reviewing this as well, however when I followed back the methods I understood why Andrew has done what he has done.
            It may not be the perfect approach but that in my mind would be a serious investment, and what has been done is at least logical.
            On the plus side when Forum finally does get some serious loving, or a replacement is found it should be easy to map these methods to the new structures.
            By no means is my answer definitive, but the current approach gets a +1 from me.

            Cheers
            Sam

            Show
            Sam Hemelryk added a comment - Just chiming in after Andrew messaged me in regards to caching as the singleton. First up caching. Andrew is spot on in summarising my viewing on using MUC for this. Using MUC for request caches adds overhead and must come with immediate value. That value usually comes in the form of a cache that is shared amongst API's, or may require invalidation in more than one API in such a way that it cannot be assumed or controlled from any single place. Last I looked at the caches here that wasn't the cache. On a very general note: cache event invalidation can be a risky thing with older parts of Moodle. I'm not sure how forum stands truthfully, but normally before going down this road I would strongly suggest only invalidating by events from API's. All interaction with something through an API passes through a single point somewhere in the process which is where you would trigger your invalidation. Without the API you may find that its impossible to be sure you trigger the invalidation in all places, especially for older code like forum where it was originally acceptable to assume forum was present and interact with them in other code (obviously not the case now). In regards to the singleton idea what I said to Andrew could be summarised by saying it is just a matter of time and effort. I agree that if time wasn't an issue a better approach would be to look at singleton class for the core forum concepts and supporting static helper classes for bulk actions. However I believe we are trying to get forum improvements achieved within sprints and refrain from significant modification and Petr'esq rewriting of the Forum at this stage. The inconsistency between forumid and forum caught my eye while reviewing this as well, however when I followed back the methods I understood why Andrew has done what he has done. It may not be the perfect approach but that in my mind would be a serious investment, and what has been done is at least logical. On the plus side when Forum finally does get some serious loving, or a replacement is found it should be easy to map these methods to the new structures. By no means is my answer definitive, but the current approach gets a +1 from me. Cheers Sam
            Hide
            Andrew Nicols added a comment -

            Thanks again:
            1) fixed
            2) good plan, but perhaps we should move all of the subscription related functions to the new class and mark this as part of the deprecation?
            3) yes. We do indeed call it in a loop, but it's a considered thought though. If you look higher up, we retrieve the full record for the first 5,000 users. After that we start to only store the id and then call the get_record here if idnumber wasn't retrieved (not sure why we check idnumber but hey...) so as not to run out of memory so I guess that's a won't change. We could theoretically change the sorting (at present they're by email) such that they're ordered by the number of posts each user will receive, but that's difficult to calculate. We could also potentially store them in a request cache, but we'd still have to monitor the size of that cache because the default cache is still memory so it doesn't really help us.
            4) Good catch. Done
            5) Thanks yes - I'll double check who that was and split the commit. Apologies for the rudeness, it was unintentional.

            I'm afraid I don't have a solution to the per-forum instance thoughts.

            Show
            Andrew Nicols added a comment - Thanks again: 1) fixed 2) good plan, but perhaps we should move all of the subscription related functions to the new class and mark this as part of the deprecation? 3) yes. We do indeed call it in a loop, but it's a considered thought though. If you look higher up, we retrieve the full record for the first 5,000 users. After that we start to only store the id and then call the get_record here if idnumber wasn't retrieved (not sure why we check idnumber but hey...) so as not to run out of memory so I guess that's a won't change. We could theoretically change the sorting (at present they're by email) such that they're ordered by the number of posts each user will receive, but that's difficult to calculate. We could also potentially store them in a request cache, but we'd still have to monitor the size of that cache because the default cache is still memory so it doesn't really help us. 4) Good catch. Done 5) Thanks yes - I'll double check who that was and split the commit. Apologies for the rudeness, it was unintentional. I'm afraid I don't have a solution to the per-forum instance thoughts.
            Hide
            Frédéric Massart added a comment -

            Thanks Andrew,

            Here are some other things I noticed:

            1. Both IDs are the same here (mod/forum/discuss.php:111)

                      // Firstly for the forum being moved to.
                      \mod_forum\subscriptions::fill_subscription_cache($forumto->id);
                      // And also for the discussion being moved.
                      \mod_forum\subscriptions::fill_subscription_cache($forumto->id);
              

            2. In regard with the DB query in forum_is_forcesubscribed, you could add a debugging message, and so we can improve master before it lands.
            3. Can we potentially do a query in a loop here? I do not see the result of the query being cached somewhere:

              // Not your code: forum/lib.php:639
                                   if (!isset($userfrom->idnumber)) {
                                       // Minimalised user info, fetch full record.
                                       $userfrom = $DB->get_record('user', array('id' => $userfrom->id));
                                       forum_cron_minimise_user_record($userfrom);
                                   }
              // Maybe this is intentional...
              

            4. forum_print_latest_dscussions: If you are adding space before and after the equal character, you might as well make it consistent with the rest of the parameters.
            5. You might want to consider giving credit and/or authorship to the person who designed the icon in a commit.

            You are right about the forum cron, the per-forum instance might not be a very good idea. Maybe it's just that the cron needs to be different and works its own way. I don't have any strong opinion on this, and with all the time and effort you've already put on this issue, I am confident that you considered it and made the right decision, I just wanted to raise the point.

            Good work!

            Cheers,
            Fred

            Show
            Frédéric Massart added a comment - Thanks Andrew, Here are some other things I noticed: Both IDs are the same here (mod/forum/discuss.php:111) // Firstly for the forum being moved to. \mod_forum\subscriptions::fill_subscription_cache($forumto->id); // And also for the discussion being moved. \mod_forum\subscriptions::fill_subscription_cache($forumto->id); In regard with the DB query in forum_is_forcesubscribed, you could add a debugging message, and so we can improve master before it lands. Can we potentially do a query in a loop here? I do not see the result of the query being cached somewhere: // Not your code: forum/lib.php:639 if (!isset($userfrom->idnumber)) { // Minimalised user info, fetch full record. $userfrom = $DB->get_record('user', array('id' => $userfrom->id)); forum_cron_minimise_user_record($userfrom); } // Maybe this is intentional... forum_print_latest_dscussions: If you are adding space before and after the equal character, you might as well make it consistent with the rest of the parameters. You might want to consider giving credit and/or authorship to the person who designed the icon in a commit. You are right about the forum cron, the per-forum instance might not be a very good idea. Maybe it's just that the cron needs to be different and works its own way. I don't have any strong opinion on this, and with all the time and effort you've already put on this issue, I am confident that you considered it and made the right decision, I just wanted to raise the point. Good work! Cheers, Fred
            Hide
            Andrew Nicols added a comment -

            Thanks for the in-depth comments Fred, I look forward to the rest!

            1: fixed and pushed
            2: could, but is there any benefit? I kept it in the same guise as other events. Would be interested to know your rationale here.
            3: good catch - I should have thought about that
            4: I discussed this with SamH over IM and decided that it was probably not the best approach in this case. The setup costs for such caches can be quite high, especially if a cache is only used the once.
            5: fixed - thanks. I introduced those today /o\
            6: changed to protected
            7: Maybe... My original thinking was from the point-of-view of cron. Most of the interactions a user has with the forum when these functions are used involve maybe two, or three calls. Conversely when cron runs you're looking at an equation taking into account the number of unique forums, unique discussions, unique posts, unique recipients, subscriptions, and discussion preferences. A per-forum singleton could work, but I'd be worried about the memory footprint in cron. Having a singleton per forum with a subscription cache, and discussion preference cache could work here as we would be able to throw away an entire forum if memory became tight. That said, the way that the forum cron code is currently written is user oriented. We loop through each user and send all of their forum posts in one go and that's how the current cache is also written. If we moved to a per-forum class structure then the nature of the caching would become forum oriented and when selecting all posts for a user we'd have to build each forum in turn and load it's preferences. If memory were to become an issue, we would not be able to flush parts of the cache in the knowledge that we would not need them again. Perhaps we should discuss this more tomorrow when you've had more of a look through the code.
            8: Not sure whether we can do much about that, though actually we should move that up one to the is_subscribed() function because if the discussionid is specified, then the force subscription check is bypassed. I've now made the move (and confirmed that doing so doesn't break unit tests).

            Cheers,

            Andrew

            Show
            Andrew Nicols added a comment - Thanks for the in-depth comments Fred, I look forward to the rest! 1: fixed and pushed 2: could, but is there any benefit? I kept it in the same guise as other events. Would be interested to know your rationale here. 3: good catch - I should have thought about that 4: I discussed this with SamH over IM and decided that it was probably not the best approach in this case. The setup costs for such caches can be quite high, especially if a cache is only used the once. 5: fixed - thanks. I introduced those today /o\ 6: changed to protected 7: Maybe... My original thinking was from the point-of-view of cron. Most of the interactions a user has with the forum when these functions are used involve maybe two, or three calls. Conversely when cron runs you're looking at an equation taking into account the number of unique forums, unique discussions, unique posts, unique recipients, subscriptions, and discussion preferences. A per-forum singleton could work, but I'd be worried about the memory footprint in cron. Having a singleton per forum with a subscription cache, and discussion preference cache could work here as we would be able to throw away an entire forum if memory became tight. That said, the way that the forum cron code is currently written is user oriented. We loop through each user and send all of their forum posts in one go and that's how the current cache is also written. If we moved to a per-forum class structure then the nature of the caching would become forum oriented and when selecting all posts for a user we'd have to build each forum in turn and load it's preferences. If memory were to become an issue, we would not be able to flush parts of the cache in the knowledge that we would not need them again. Perhaps we should discuss this more tomorrow when you've had more of a look through the code. 8: Not sure whether we can do much about that, though actually we should move that up one to the is_subscribed() function because if the discussionid is specified, then the force subscription check is bypassed. I've now made the move (and confirmed that doing so doesn't break unit tests). Cheers, Andrew
            Hide
            Frédéric Massart added a comment -

            Hi Andrew,

            as per your request I had a look at this, here are some comments/suggestions.

            1. Missing period in events::get_url() PHP Docs.
            2. Custom validation of events could use else if - Does not matter
            3. I would recommend using get_recordset where possible, especially in fill_discussion_subscription_cache, imagine the number of preferences that one could have on Moodle.org, that is a lot to load in memory.
            4. Could we use caching rather than static caches? There are cache events to invalidate the data, and perhaps that can be linked to a user, or a forum ID. Preventing the invalidation of all the data as soon as someone changes a preference.
            5. I saw references to \mod_forum\subscriptions::FORUM_DISCUSSION_xxx in that same class, self:: would be enough.
            6. Is it really necessary to set the variables and the methods as private? Would there be a usecase for partners to play override/re-use those? I usually tend to go for protected except in some cases.
            7. As most of the logic include static methods, so this is just a set of functions, would that make sense to get an instance per forum? So you do not need to pass forum around any more. It could be a singleton of course. This would help solving the inconsistencies in method parameters where some are $forum, some are $forumid, and when they are $forum I did not notice much usage of other properties. Maybe that would help for caching purposes too. As code speaks better than my English, I mean this: (food for thoughts)

              $forumsub = \mod_forum\subscriptions::get($forumid);
              $forumsub->is_subscribed($userid);
              $forumsub->subscribe($userid);
              $forumsub->subscribe_to($userid, $discussionid);
               
              // Maybe we need a forum object too... but not now ;)
              

            8. I had to dive in quite a lot to understand why you needed the key forcesubscribe when subscribing a user. This is something we will always forget - And if we do, one more query. Stacktrace: Observer > ::subscribe_user() > ::is_subscribed() > forum_is_forcesubscribed() > SQL Query or not.

            I'll stop here today, and I'll resume tomorrow.

            Cheers,
            Fred

            Show
            Frédéric Massart added a comment - Hi Andrew, as per your request I had a look at this, here are some comments/suggestions. Missing period in events::get_url() PHP Docs. Custom validation of events could use else if - Does not matter I would recommend using get_recordset where possible, especially in fill_discussion_subscription_cache, imagine the number of preferences that one could have on Moodle.org, that is a lot to load in memory. Could we use caching rather than static caches? There are cache events to invalidate the data, and perhaps that can be linked to a user, or a forum ID. Preventing the invalidation of all the data as soon as someone changes a preference. I saw references to \mod_forum\subscriptions::FORUM_DISCUSSION_xxx in that same class, self:: would be enough. Is it really necessary to set the variables and the methods as private? Would there be a usecase for partners to play override/re-use those? I usually tend to go for protected except in some cases. As most of the logic include static methods, so this is just a set of functions, would that make sense to get an instance per forum? So you do not need to pass forum around any more. It could be a singleton of course. This would help solving the inconsistencies in method parameters where some are $forum, some are $forumid, and when they are $forum I did not notice much usage of other properties. Maybe that would help for caching purposes too. As code speaks better than my English, I mean this: (food for thoughts) $forumsub = \mod_forum\subscriptions::get($forumid); $forumsub->is_subscribed($userid); $forumsub->subscribe($userid); $forumsub->subscribe_to($userid, $discussionid);   // Maybe we need a forum object too... but not now ;) I had to dive in quite a lot to understand why you needed the key forcesubscribe when subscribing a user. This is something we will always forget - And if we do, one more query. Stacktrace: Observer > ::subscribe_user() > ::is_subscribed() > forum_is_forcesubscribed() > SQL Query or not. I'll stop here today, and I'll resume tomorrow. Cheers, Fred
            Hide
            Andrew Davis added a comment -

            $string['discussionsubscribestop'] = 'I don\'t want email copies of posts to this discussion';

            I'm not entirely sure what you should change it to but forum notifications may not be received by email (although they generally are). For example, a user may be notified by Jabber.

            Aside from that I think you might be ready to go. Well done with all the automated testing.

            Show
            Andrew Davis added a comment - $string['discussionsubscribestop'] = 'I don\'t want email copies of posts to this discussion'; I'm not entirely sure what you should change it to but forum notifications may not be received by email (although they generally are). For example, a user may be notified by Jabber. Aside from that I think you might be ready to go. Well done with all the automated testing.
            Hide
            Andrew Nicols added a comment -

            Taking this out of peer review as I'm trying to optimise the cron code further.

            Show
            Andrew Nicols added a comment - Taking this out of peer review as I'm trying to optimise the cron code further.
            Hide
            Andrew Nicols added a comment -

            I've made the suggested changes for the context and comments and pushed a fix.

            Show
            Andrew Nicols added a comment - I've made the suggested changes for the context and comments and pushed a fix.
            Hide
            Andrew Nicols added a comment -

            Cheers Andrew,

            I've added both of those tags as you suggest.
            I'll also make the comments changes for those unit tests, and the table name.

            I won't make that change to line 4042. That's a bug in github, not our code and is entirely unrelated to my current changeset in any case. It's perfectly valid code. Please raise a bug report against GitHub.

            I was just wondering about that same context check myself so I'll add that now.

            Show
            Andrew Nicols added a comment - Cheers Andrew, I've added both of those tags as you suggest. I'll also make the comments changes for those unit tests, and the table name. I won't make that change to line 4042. That's a bug in github, not our code and is entirely unrelated to my current changeset in any case. It's perfectly valid code. Please raise a bug report against GitHub. I was just wondering about that same context check myself so I'll add that now.
            Hide
            Andrew Davis added a comment -

            A few quick things right off the bat.

            Assuming you agree it is required, add the docs_required label. http://docs.moodle.org/dev/Tracker_issue_labels Presumably there will be some user docs that require updating for this new functionality.

            The qa_test_required label should probably also be on here. If nothing else someone will need to review the forum QA tests and make sure that they cover new functionality and haven't been made to fail by the changes.

            In mod/forum/tests/mail_test.php helper_run_cron_check_count() there is what appears to be an incorrect comment.

            +        // There should be two messages.
            +        $this->assertEquals($expected, count($messages));
            

            Typo in mod/forum/db/install.xml. In the comment presumably one of those "subscribe"s should be "unsubscribe".

            <TABLE NAME="forum_discussion_subs" COMMENT="Users may choose to subscribe and subscribe from specific discussions.">
            

            Its pretty trivial but have a look at https://github.com/andrewnicols/moodle/blob/5e8de81b082abefc3c3acff40fe900570ce90c2f/mod/forum/lib.php line 4042. Notice how everything below that is not coloured correctly? The code appears to be correct but github has problems with it. If its all the same to you would you mind replacing the inner quotes with single quotes. I think that will fix it.

            You didn't add it but In mod/forum/classes/subscriptions.php there appears to be some copy pasted code. ctrl+f for "// Find out forum context. First try to take current page context to save on DB query." and note the if else structure that follows it. The if condition is certainly non-trivial so its probably worth shifting the whole thing into a function.

            Show
            Andrew Davis added a comment - A few quick things right off the bat. Assuming you agree it is required, add the docs_required label. http://docs.moodle.org/dev/Tracker_issue_labels Presumably there will be some user docs that require updating for this new functionality. The qa_test_required label should probably also be on here. If nothing else someone will need to review the forum QA tests and make sure that they cover new functionality and haven't been made to fail by the changes. In mod/forum/tests/mail_test.php helper_run_cron_check_count() there is what appears to be an incorrect comment. + // There should be two messages. + $this->assertEquals($expected, count($messages)); Typo in mod/forum/db/install.xml. In the comment presumably one of those "subscribe"s should be "unsubscribe". <TABLE NAME="forum_discussion_subs" COMMENT="Users may choose to subscribe and subscribe from specific discussions."> Its pretty trivial but have a look at https://github.com/andrewnicols/moodle/blob/5e8de81b082abefc3c3acff40fe900570ce90c2f/mod/forum/lib.php line 4042. Notice how everything below that is not coloured correctly? The code appears to be correct but github has problems with it. If its all the same to you would you mind replacing the inner quotes with single quotes. I think that will fix it. You didn't add it but In mod/forum/classes/subscriptions.php there appears to be some copy pasted code. ctrl+f for "// Find out forum context. First try to take current page context to save on DB query." and note the if else structure that follows it. The if condition is certainly non-trivial so its probably worth shifting the whole thing into a function.
            Hide
            Andrew Davis added a comment -

            The peer review torch has been passed.

            Show
            Andrew Davis added a comment - The peer review torch has been passed.
            Hide
            Andrew Nicols added a comment -

            Unit test is at 100% for the new class, the new events, the deprecated components, and in a few other places
            Behat tests cover most things, but are currently unable to test the moving forums action. I've also deferred the backup/restore test to a manual test for the moment too, and I've written some fairly comprehensive test instructions for those two areas.

            Show
            Andrew Nicols added a comment - Unit test is at 100% for the new class, the new events, the deprecated components, and in a few other places Behat tests cover most things, but are currently unable to test the moving forums action. I've also deferred the backup/restore test to a manual test for the moment too, and I've written some fairly comprehensive test instructions for those two areas.
            Hide
            CiBoT added a comment -

            Results for MDL-1626

            • Remote repository: git://github.com/andrewnicols/moodle.git
            Show
            CiBoT added a comment - Results for MDL-1626 Remote repository: git://github.com/andrewnicols/moodle.git Remote branch MDL-1626 -master to be integrated into upstream master Executed job http://integration.moodle.org/job/Precheck%20remote%20branch/4023 Details: http://integration.moodle.org/job/Precheck%20remote%20branch/4023/artifact/work/smurf.html
            Hide
            Andrew Nicols added a comment -

            Hi Mark Pearson,

            This issue is actually a part of MDL-39707. The priority on that issue is this issue and MDL-41773 (which deals with the layout rewrite) and we're trying to get those in for 2.8.

            Jason Fowler and I have started to look at how we can make the required changes for the layout issue too. It just happened that in writing out the specification for this issue I was considering the optimal way of writing this functionality and wrote a proof-of-concept which ticked all of the boxes.

            Andrew

            Show
            Andrew Nicols added a comment - Hi Mark Pearson , This issue is actually a part of MDL-39707 . The priority on that issue is this issue and MDL-41773 (which deals with the layout rewrite) and we're trying to get those in for 2.8. Jason Fowler and I have started to look at how we can make the required changes for the layout issue too. It just happened that in writing out the specification for this issue I was considering the optimal way of writing this functionality and wrote a proof-of-concept which ticked all of the boxes. Andrew
            Hide
            Mark Pearson added a comment -

            Andrew,
            I'm trying to figure out the status of MDL-39707 (Update mod_forum to add additional functionality, and improve accessibility and performance) relative to what you're doing. I'm worried that patching the current Forum code will mean that nothing will happen in terms of modernising the overall look, feel & functionality of this most basic of Moodle's features. It's rather frustrating to see overall progress on MDL-39707 completely stalled.

            Show
            Mark Pearson added a comment - Andrew, I'm trying to figure out the status of MDL-39707 (Update mod_forum to add additional functionality, and improve accessibility and performance) relative to what you're doing. I'm worried that patching the current Forum code will mean that nothing will happen in terms of modernising the overall look, feel & functionality of this most basic of Moodle's features. It's rather frustrating to see overall progress on MDL-39707 completely stalled.
            Hide
            Andrew Nicols added a comment -

            Pushed an updated version with:

            • unit tests for the events;
            • unit tests for 95% of the \mod_forum\subscriptions class
            • behat tests for some of the subscription stuff:
              • change in forum subscription + discussion subscripion + their effect on one another

            Still to do:

            • behat testing for more of the subscription stuff:
              • change in module-level forum subscription model (by admin)'s effect on user subscriptions
              • backup/restore tests

            And deferred because other issues need to fix things:

            • behat tests for moving discussions - we need to address accesibility issues here first so manual testing will be required

            Will write more tests next week (yay).

            Show
            Andrew Nicols added a comment - Pushed an updated version with: unit tests for the events; unit tests for 95% of the \mod_forum\subscriptions class behat tests for some of the subscription stuff: change in forum subscription + discussion subscripion + their effect on one another Still to do: behat testing for more of the subscription stuff: change in module-level forum subscription model (by admin)'s effect on user subscriptions backup/restore tests And deferred because other issues need to fix things: behat tests for moving discussions - we need to address accesibility issues here first so manual testing will be required Will write more tests next week (yay).
            Hide
            Sam Hemelryk added a comment -

            Oops forgot to push finish review sorry Andrew.

            Show
            Sam Hemelryk added a comment - Oops forgot to push finish review sorry Andrew.
            Hide
            CiBoT added a comment -

            Results for MDL-1626

            • Remote repository: git://github.com/andrewnicols/moodle.git
            Show
            CiBoT added a comment - Results for MDL-1626 Remote repository: git://github.com/andrewnicols/moodle.git Remote branch MDL-1626 -master to be integrated into upstream master Executed job http://integration.moodle.org/job/Precheck%20remote%20branch/3974 Details: http://integration.moodle.org/job/Precheck%20remote%20branch/3974/artifact/work/smurf.html
            Hide
            Andrew Nicols added a comment -

            Helps if I push the right branch when requesting testing. Sorry about the CI noise.

            Show
            Andrew Nicols added a comment - Helps if I push the right branch when requesting testing. Sorry about the CI noise.
            Hide
            CiBoT added a comment -

            Results for MDL-1626

            • Remote repository: git://github.com/andrewnicols/moodle.git
            Show
            CiBoT added a comment - Results for MDL-1626 Remote repository: git://github.com/andrewnicols/moodle.git Remote branch MDL-1626 -master to be integrated into upstream master Executed job http://integration.moodle.org/job/Precheck%20remote%20branch/3973 Details: http://integration.moodle.org/job/Precheck%20remote%20branch/3973/artifact/work/smurf.html
            Hide
            Andrew Nicols added a comment -

            mod/forum/classes/subscriptions.php
            1: Not sure of a more appropriate return value. This is used for pre-seeding the cache. We need to return but there is no data to return. Okay, so I've created a new set of functions: One to fetch the cache, and one to fill it. Places which need to preseed the cache now fill it, but elsewhere it's fetched (which calls fill first).
            2: Thanks, wrapped in an appropriate check.
            3: Thanks - optimised
            4: Will look into that. It's part of the original forum code but it would be good to improve it in this change. I'm not quite sure where we'd get the cmid from as the function is passed the $forum object and it's a public API. I'm not quite sure where we'd get the cmid from as the function is passed the $forum object and it's a public API.
            5: Thanks, wrapped in an appropriate check.

            mod/forum/lib.php
            1: Good point. I'll fix those.
            2: Yeah - it's weird and we shouldn't fail a delete because subscription removal has failed but as you point out it returns true anyway. I have moved the field to above the forum discussion removal because this has FK constraints and would fail there. I've also moved the digests and the standard subs, and I've taken out the bonkers results.
            3: The coding style needs to clarify this really. Have changed the indentation.
            4: The coding style needs to clarify this really. Have changed the indentation.
            5: The & isn't redundant - we need to pass the value by reference here to update the cache. Have updated the docs to reflect this.
            6: Good luck. Pen and paper required!
            7: Yup - thanks. I've added a call there which doesn't affect the return value. A failure to remove subscriptions will break the discussion removal anyway, and delete_records always return true.
            8: Tah - removed.
            9: Tah - removed.
            10: Good catch - that was from a first attempt

            toggle.js
            1: Yup

            lang/en/forum.php
            1: Okay sure thing.
            2: I'll make that change, but this is probably one for Helen?

            General suggestions/ideas/wants:
            1: Behat. Yes. I know I need to write these. I was just valueing my sanity a little longer before delving into that area.
            2: Yup - agree. I'll write tests for the new stuff in deprecated. Actually turns out that I had written some, but at some point when I was checking I'd converted every call to the newly deprecated functions, I removed said tests. Added back with additional comments and th
            e such.
            3: Ack. I hate events
            4: Agree. I was just being sleep-deprived and lazy.

            I've taken a fresh look at cron and optimised it further - I was indeed doing something dumb.

            WRT backup/restore, I'll try and write an appropriate behat test here. It would be nice longer term to automate testing of backup/restore for all modules in behat.
            WRT table indexes, at present they're all FK constrains, and I couldn't get XMLDB editor to add additional indices on the table.

            Also going to write more unit test coverage.

            Show
            Andrew Nicols added a comment - mod/forum/classes/subscriptions.php 1: Not sure of a more appropriate return value. This is used for pre-seeding the cache. We need to return but there is no data to return. Okay, so I've created a new set of functions: One to fetch the cache, and one to fill it. Places which need to preseed the cache now fill it, but elsewhere it's fetched (which calls fill first). 2: Thanks, wrapped in an appropriate check. 3: Thanks - optimised 4: Will look into that. It's part of the original forum code but it would be good to improve it in this change. I'm not quite sure where we'd get the cmid from as the function is passed the $forum object and it's a public API. I'm not quite sure where we'd get the cmid from as the function is passed the $forum object and it's a public API. 5: Thanks, wrapped in an appropriate check. mod/forum/lib.php 1: Good point. I'll fix those. 2: Yeah - it's weird and we shouldn't fail a delete because subscription removal has failed but as you point out it returns true anyway. I have moved the field to above the forum discussion removal because this has FK constraints and would fail there. I've also moved the digests and the standard subs, and I've taken out the bonkers results. 3: The coding style needs to clarify this really. Have changed the indentation. 4: The coding style needs to clarify this really. Have changed the indentation. 5: The & isn't redundant - we need to pass the value by reference here to update the cache. Have updated the docs to reflect this. 6: Good luck. Pen and paper required! 7: Yup - thanks. I've added a call there which doesn't affect the return value. A failure to remove subscriptions will break the discussion removal anyway, and delete_records always return true. 8: Tah - removed. 9: Tah - removed. 10: Good catch - that was from a first attempt toggle.js 1: Yup lang/en/forum.php 1: Okay sure thing. 2: I'll make that change, but this is probably one for Helen? General suggestions/ideas/wants: 1: Behat. Yes. I know I need to write these. I was just valueing my sanity a little longer before delving into that area. 2: Yup - agree. I'll write tests for the new stuff in deprecated. Actually turns out that I had written some, but at some point when I was checking I'd converted every call to the newly deprecated functions, I removed said tests. Added back with additional comments and th e such. 3: Ack. I hate events 4: Agree. I was just being sleep-deprived and lazy. I've taken a fresh look at cron and optimised it further - I was indeed doing something dumb. WRT backup/restore, I'll try and write an appropriate behat test here. It would be nice longer term to automate testing of backup/restore for all modules in behat. WRT table indexes, at present they're all FK constrains, and I couldn't get XMLDB editor to add additional indices on the table. Also going to write more unit test coverage.
            Hide
            Sam Hemelryk added a comment -

            Attached coding style cleanup patch "1626.gitpatch"
            Can be applied with "git apply"

            Show
            Sam Hemelryk added a comment - Attached coding style cleanup patch "1626.gitpatch" Can be applied with "git apply"
            Hide
            Sam Hemelryk added a comment -

            Thanks for working on this Andrew - you are a brave, dedicated man.
            I am really looking forward to this arriving to Moodle.org!

            As I've gone through your patch I've made several codign style fixes, I'll attach a patch file immediately after I post this so that you can just pull them in if you like. If you'd rather do it yourself no probs - the code checker and moodle check turn up 90% of what I fixed. The other 10% is things like unused globals, missing globals, unused variables, typos.

            mod/forum/classes/subscriptions.php

            1. 174 inconsistent return.
            2. 346 $discussionsubscriptions is not guaranteed to exist, must fix.
            3. 375 long line with possible optimisation, it looks like the same params on that line are used within all three database queries in that method and could be separated out into a single var and reused.
            4. 398 I can't help but think a database query could be saved here if this was one database call rather than two. Really not important as this is a rare operation, just noting as it catches my eye every time I scroll past it.
            5. 412 $discussionsubscriptions is not guaranteed to exist, must fix.

            mod/forum/lib.php

            1. It strikes me that in other defines we've not shortend SUBSCRIBE to SUB, perhaps for consistency and sort of in line with coding style those defines should be FORUM_DISCUSSION_SUBSCRIBE and FORUM_DISCUSSION_UNSUBSCRIBE.
            2. 297 [forum_delete_instance] just noting that the if statement there is redundant, delete_records always returns true and throws exceptions on fail. What you've done is consistent so no problem really - just noting so that it has being mentioned.
            3. 654 Wierd indenting
            4. 689 ditto
            5. 1030 [forum_cron_setup_user] Redundant & on $userto, noting in the phpdoc that this function modifies the given object would be a better appraoch (assuming you've put that there to try to signify that it is going to be modified).
            6. Cron code in general, I need to take a real close look at, performance needs to be considered, and there is something that doesn't match up there in my head yet.
            7. 4512 [forum_delete_discussion] does this function need to delete forum_discussion_subs?
            8. 4797 [forum_post_subscription] unused variable $action
            9. 4798 [forum_post_subscription] unused variable $subscribed resulting from function call.
            10. 5346 [forum_print_latest_discussions] you added a new variable that is not used.

            toogle.js

            1. Just the general docs stuff really, I imagine you're well aware of this anyway, just mentioning for completeness.

            lang/en/forum.php

            1. clicktosubscribe, and clicktounsubscribe - I don't think "currently" is needed in those strings.
            2. also with clicktounsubscribe: when observing the tooltip on the icon for a discussion I wonder if it would be clearer to have something like "Not subscribed: click to subscribe" so that it can be easily differentiated at a glance.

            General suggestions/ideas/wants:

            1. Behat tests, really not fun perhaps, but really essential as this is the prime oppertunity to use some tests testing subscriptions.
            2. You had introduced fatal errors in the deprecated methods which raises a good point. We should be unit testing those.
            3. New events should really be accompanied by tests.
            4. Test instructions on this issue really need to be stronger.
              • Steps to test cron and mail out would of course be essential, perhaps important to have multiple users subscribed to a discusion.
              • Moving a descussion

            Things I've still to look at and test, Andrew I wouldn't mind chatting to you about a couple of these when you are online next:

            1. Cron
            2. Backup/Restore
            3. Table indexes

            Just noting that there is perhaps a raft of features that people would appreciate after this has landed.
            Those additional new features should have issues created after this lands if there isn't already an issue for them.

            Cheers
            Sam

            Show
            Sam Hemelryk added a comment - Thanks for working on this Andrew - you are a brave, dedicated man. I am really looking forward to this arriving to Moodle.org! As I've gone through your patch I've made several codign style fixes, I'll attach a patch file immediately after I post this so that you can just pull them in if you like. If you'd rather do it yourself no probs - the code checker and moodle check turn up 90% of what I fixed. The other 10% is things like unused globals, missing globals, unused variables, typos. mod/forum/classes/subscriptions.php 174 inconsistent return. 346 $discussionsubscriptions is not guaranteed to exist, must fix. 375 long line with possible optimisation, it looks like the same params on that line are used within all three database queries in that method and could be separated out into a single var and reused. 398 I can't help but think a database query could be saved here if this was one database call rather than two. Really not important as this is a rare operation, just noting as it catches my eye every time I scroll past it. 412 $discussionsubscriptions is not guaranteed to exist, must fix. mod/forum/lib.php It strikes me that in other defines we've not shortend SUBSCRIBE to SUB, perhaps for consistency and sort of in line with coding style those defines should be FORUM_DISCUSSION_SUBSCRIBE and FORUM_DISCUSSION_UNSUBSCRIBE. 297 [forum_delete_instance] just noting that the if statement there is redundant, delete_records always returns true and throws exceptions on fail. What you've done is consistent so no problem really - just noting so that it has being mentioned. 654 Wierd indenting 689 ditto 1030 [forum_cron_setup_user] Redundant & on $userto, noting in the phpdoc that this function modifies the given object would be a better appraoch (assuming you've put that there to try to signify that it is going to be modified). Cron code in general, I need to take a real close look at, performance needs to be considered, and there is something that doesn't match up there in my head yet. 4512 [forum_delete_discussion] does this function need to delete forum_discussion_subs? 4797 [forum_post_subscription] unused variable $action 4798 [forum_post_subscription] unused variable $subscribed resulting from function call. 5346 [forum_print_latest_discussions] you added a new variable that is not used. toogle.js Just the general docs stuff really, I imagine you're well aware of this anyway, just mentioning for completeness. lang/en/forum.php clicktosubscribe, and clicktounsubscribe - I don't think "currently" is needed in those strings. also with clicktounsubscribe: when observing the tooltip on the icon for a discussion I wonder if it would be clearer to have something like "Not subscribed: click to subscribe" so that it can be easily differentiated at a glance. General suggestions/ideas/wants: Behat tests, really not fun perhaps, but really essential as this is the prime oppertunity to use some tests testing subscriptions. You had introduced fatal errors in the deprecated methods which raises a good point. We should be unit testing those. New events should really be accompanied by tests. Test instructions on this issue really need to be stronger. Steps to test cron and mail out would of course be essential, perhaps important to have multiple users subscribed to a discusion. Moving a descussion Things I've still to look at and test, Andrew I wouldn't mind chatting to you about a couple of these when you are online next: Cron Backup/Restore Table indexes Just noting that there is perhaps a raft of features that people would appreciate after this has landed. Those additional new features should have issues created after this lands if there isn't already an issue for them. Cheers Sam
            Hide
            CiBoT added a comment -

            Results for MDL-1626

            • Remote repository: git://github.com/andrewnicols/moodle.git
            Show
            CiBoT added a comment - Results for MDL-1626 Remote repository: git://github.com/andrewnicols/moodle.git Remote branch MDL-1626 -master to be integrated into upstream master Executed job http://integration.moodle.org/job/Precheck%20remote%20branch/3912 Error: The MDL-1626 -master branch at git://github.com/andrewnicols/moodle.git exceeds the maximum number of commits ( 17 > 15)
            Hide
            Andrew Nicols added a comment -

            I've just pushed some additional changes. I was looking to handle moving the subscriptions to a discussion when that discussion has moved. As a result of this I needed to implement additional caching and to do so cleanly, I started to move the subscription functions to a new class, implemented as static functions.

            I also started to restrict when we accept a forum ID, and when we accept an object. There were several locations where we were fetching a forum record if an ID was provided and then providing the ID when we already had the record when subsequently calling that function. The most obvious of these was forum_is_subscribed() which we frequently call in loops.

            As a result, of these changes, we can now query the subscription status of every user in a forum, in a single query. We can also query the status of every discussion in a forum with a single query too.

            I felt that this was a blocker for moving of discussions - imagine querying every subscription status on the Using Moodle forum on moodle.org...

            Still waiting for PR.

            Show
            Andrew Nicols added a comment - I've just pushed some additional changes. I was looking to handle moving the subscriptions to a discussion when that discussion has moved. As a result of this I needed to implement additional caching and to do so cleanly, I started to move the subscription functions to a new class, implemented as static functions. I also started to restrict when we accept a forum ID, and when we accept an object. There were several locations where we were fetching a forum record if an ID was provided and then providing the ID when we already had the record when subsequently calling that function. The most obvious of these was forum_is_subscribed() which we frequently call in loops. As a result, of these changes, we can now query the subscription status of every user in a forum, in a single query. We can also query the status of every discussion in a forum with a single query too. I felt that this was a blocker for moving of discussions - imagine querying every subscription status on the Using Moodle forum on moodle.org... Still waiting for PR.
            Hide
            CiBoT added a comment -

            Results for MDL-1626

            • Remote repository: git://github.com/andrewnicols/moodle.git
            Show
            CiBoT added a comment - Results for MDL-1626 Remote repository: git://github.com/andrewnicols/moodle.git Remote branch MDL-1626 -master to be integrated into upstream master Executed job http://integration.moodle.org/job/Precheck%20remote%20branch/3833 Details: http://integration.moodle.org/job/Precheck%20remote%20branch/3833/artifact/work/smurf.html
            Hide
            CiBoT added a comment -

            Results for MDL-1626

            • Remote repository: git://github.com/andrewnicols/moodle.git
            Show
            CiBoT added a comment - Results for MDL-1626 Remote repository: git://github.com/andrewnicols/moodle.git Remote branch MDL-1626 -master to be integrated into upstream master Executed job http://integration.moodle.org/job/Precheck%20remote%20branch/3831 Details: http://integration.moodle.org/job/Precheck%20remote%20branch/3831/artifact/work/smurf.html
            Hide
            Andrew Nicols added a comment -

            Attaching some screenshots of the UI in it's current form

            Show
            Andrew Nicols added a comment - Attaching some screenshots of the UI in it's current form
            Hide
            Andrew Nicols added a comment -

            Here's my first attempt at doing this. Things to note:

            • it is still not possible to subscribe to a discussion if subscription is disabled;
            • it is still not possible to unsubscribe from a discussion if subscription is forced;
            • when a user changes their own forum subscription, all of their per-discussion subscription preferences for that forum are removed;
            • when a user changes the subscription method for the forum in a way which does affect all particiapnts (e.g. from optional to automatic), this will affect the forum discussions for all users but not the discussion subscriptions. This enables a user who had previously opted out of the forum but into certain discussions to revert the module-level change (and vice-versa) and keep their per-discussion preference
            • If a user has already posted in a forum and set their per-discussion preference, we respect this preference. Otherwise we default to suggesting that the user subscribes to the discussion.
            Show
            Andrew Nicols added a comment - Here's my first attempt at doing this. Things to note: it is still not possible to subscribe to a discussion if subscription is disabled; it is still not possible to unsubscribe from a discussion if subscription is forced; when a user changes their own forum subscription, all of their per-discussion subscription preferences for that forum are removed; when a user changes the subscription method for the forum in a way which does affect all particiapnts (e.g. from optional to automatic), this will affect the forum discussions for all users but not the discussion subscriptions. This enables a user who had previously opted out of the forum but into certain discussions to revert the module-level change (and vice-versa) and keep their per-discussion preference If a user has already posted in a forum and set their per-discussion preference, we respect this preference. Otherwise we default to suggesting that the user subscribes to the discussion.
            Hide
            Andrew Nicols added a comment -

            Hi Elizabeth,

            That's the intention. Sorry for not updating this issue that much, but I'm aware that there are 71 people watching this issue who will receive all updates in their inbox.

            I'm currently in the process of writing unit tests and making sure that it works as we expect. I'm hoping that we'll see this land early in the 2.8 lifecycle.

            Andrew

            Show
            Andrew Nicols added a comment - Hi Elizabeth, That's the intention. Sorry for not updating this issue that much, but I'm aware that there are 71 people watching this issue who will receive all updates in their inbox. I'm currently in the process of writing unit tests and making sure that it works as we expect. I'm hoping that we'll see this land early in the 2.8 lifecycle. Andrew
            Hide
            Elizabeth Dalton added a comment -

            So... any chance we'll see this in 2.8?

            Show
            Elizabeth Dalton added a comment - So... any chance we'll see this in 2.8?
            Hide
            Andrew Nicols added a comment -

            It's worth bearing in mind the suggestions in MDL-621 for this issue: When moving a discussion between forums, we should attempt to copy the subscription data too.

            Show
            Andrew Nicols added a comment - It's worth bearing in mind the suggestions in MDL-621 for this issue: When moving a discussion between forums, we should attempt to copy the subscription data too.
            Hide
            Andrew Nicols added a comment -

            I'm starting to look at how we may implement this feature for Moodle 2.7. As such, I've started a new discussion in the Forums forum at https://moodle.org/mod/forum/discuss.php?d=243959

            Show
            Andrew Nicols added a comment - I'm starting to look at how we may implement this feature for Moodle 2.7. As such, I've started a new discussion in the Forums forum at https://moodle.org/mod/forum/discuss.php?d=243959
            Hide
            Andrew Nicols added a comment -

            Attaching a potential workflow explaining how discussion (thread) subscription could fit in with the existing digest options.

            Show
            Andrew Nicols added a comment - Attaching a potential workflow explaining how discussion (thread) subscription could fit in with the existing digest options.
            Hide
            Derek Chirnside added a comment -

            Just adding this link for the sake of completeness. There seems to be the possibility of progress here: This from the forums, https://moodle.org/mod/forum/discuss.php?d=237515#p1032149

            Show
            Derek Chirnside added a comment - Just adding this link for the sake of completeness. There seems to be the possibility of progress here: This from the forums, https://moodle.org/mod/forum/discuss.php?d=237515#p1032149
            Hide
            John White added a comment -

            Attached is the file threadsubscribingforum_2.5.0.patch which should contain everything needed to make thread-based subscription work, at least on Moodle 2.5.0, without causing much extra load on standard forums. I would be grateful for any feedback on this.
            -John

            Show
            John White added a comment - Attached is the file threadsubscribingforum_2.5.0.patch which should contain everything needed to make thread-based subscription work, at least on Moodle 2.5.0, without causing much extra load on standard forums. I would be grateful for any feedback on this. -John
            Hide
            John White added a comment -

            Hi all,
            Having not contributed to this discussion much since I first created a fix for the problem in Moodle 1.9, I have now revisited this and completed a substantial solution in version 2.5 for which I will attach a patch file or zipped folder shortly.
            But to explain what this fix achieves and how it operates I'm first attaching this file (ThreadSubscriptions.pdf) for your interest and comment.

            • John
            Show
            John White added a comment - Hi all, Having not contributed to this discussion much since I first created a fix for the problem in Moodle 1.9, I have now revisited this and completed a substantial solution in version 2.5 for which I will attach a patch file or zipped folder shortly. But to explain what this fix achieves and how it operates I'm first attaching this file (ThreadSubscriptions.pdf) for your interest and comment. John
            Hide
            Derek Chirnside added a comment -

            Question for Martin or Helen. This is progress with Moodle forums and I wish the coders well. It will certainly help with Moodle.org and Moodle based MOOCs.
            We now move on from ForumNG I guess. Close https://tracker.moodle.org/browse/MDL-21538 as "evaluated"?

            There are other aspects of forums there is interest in: the display reply in context, drafts, 1<>tutor interactions, read later, drag and drop for images into the edit area (etc). Some of these are here in the tracker, some are not. What would you suggest: visit some of these languishing tracker items and freshen them up and see what interest there is? Or do you have some other plans for forum improvements, and as such, this tidy up activity is premature? Is https://tracker.moodle.org/browse/MDL-39707 where all the action will be at?

            -Derek

            Show
            Derek Chirnside added a comment - Question for Martin or Helen. This is progress with Moodle forums and I wish the coders well. It will certainly help with Moodle.org and Moodle based MOOCs. We now move on from ForumNG I guess. Close https://tracker.moodle.org/browse/MDL-21538 as "evaluated"? There are other aspects of forums there is interest in: the display reply in context, drafts, 1<>tutor interactions, read later, drag and drop for images into the edit area (etc). Some of these are here in the tracker, some are not. What would you suggest: visit some of these languishing tracker items and freshen them up and see what interest there is? Or do you have some other plans for forum improvements, and as such, this tidy up activity is premature? Is https://tracker.moodle.org/browse/MDL-39707 where all the action will be at? -Derek
            Hide
            Derek Chirnside added a comment -

            I've been trying to follow the diagram above.

            Here's what would suit some cases:
            *'follow all new discussions', and be able to opt out of getting notifications in future from thread with one click.
            *'follow all new posts', and be able to opt IN with one click to get future notifications of this thread.
            *If I was a tutor to be able to push out a specific post to everyone subscribed to the forum in spite of their setting. Takes away the need for a specific NEWS forum.
            *Tick follow or unfollow for any existing discussion

            -Derek

            Show
            Derek Chirnside added a comment - I've been trying to follow the diagram above. Here's what would suit some cases: *'follow all new discussions', and be able to opt out of getting notifications in future from thread with one click. *'follow all new posts', and be able to opt IN with one click to get future notifications of this thread. *If I was a tutor to be able to push out a specific post to everyone subscribed to the forum in spite of their setting. Takes away the need for a specific NEWS forum. *Tick follow or unfollow for any existing discussion -Derek
            Hide
            Christopher Fuhrman added a comment -

            I took an online coursera.org MOOC and the default subscription for the thread (when asking a new question in a thread or responding in a thread). It just "worked". I was disappointed to find on Moodle.org's forums it doesn't work that way (I'm getting all new threads sent to me and I'm not interested in that). This is absolutely necessary where message volume is high! How much energy/time spent so far debating this? It could have already been implemented? 132+ votes for this. Insert Nike slogan here.

            Show
            Christopher Fuhrman added a comment - I took an online coursera.org MOOC and the default subscription for the thread (when asking a new question in a thread or responding in a thread). It just "worked". I was disappointed to find on Moodle.org's forums it doesn't work that way (I'm getting all new threads sent to me and I'm not interested in that). This is absolutely necessary where message volume is high! How much energy/time spent so far debating this? It could have already been implemented? 132+ votes for this. Insert Nike slogan here.
            Hide
            Elizabeth Dalton added a comment -

            Great! Can we hope for this in 2.6?

            Show
            Elizabeth Dalton added a comment - Great! Can we hope for this in 2.6?
            Hide
            Martin Dougiamas added a comment -

            Adding this to the queue for FRONTEND.

            The way forward is indeed tweaking the existing forum, and we'll be looking at Advanced Forums to look how they added this feature.

            Show
            Martin Dougiamas added a comment - Adding this to the queue for FRONTEND. The way forward is indeed tweaking the existing forum, and we'll be looking at Advanced Forums to look how they added this feature.
            Hide
            Derek Chirnside added a comment -

            Olja, Check out https://moodle.org/mod/forum/discuss.php?d=195094
            See http://tracker.moodle.org/browse/MDL-34144 also, and make sure you vote where you wish.

            You say "what is the future of this idea?". Helen is right, it is ForumNG, and I guess the only forseeable solution is to use this as a plugin or one of the regular forum plugins. (And I realise this could be a problem)

            I doubt if there will be any tweaking of the existing forum. My take (from what Sam, Martin and Dan says) is that it is just priorities. They are doing other things. So we just wait.

            -Derek

            Show
            Derek Chirnside added a comment - Olja, Check out https://moodle.org/mod/forum/discuss.php?d=195094 See http://tracker.moodle.org/browse/MDL-34144 also, and make sure you vote where you wish. You say "what is the future of this idea?". Helen is right, it is ForumNG, and I guess the only forseeable solution is to use this as a plugin or one of the regular forum plugins. (And I realise this could be a problem) I doubt if there will be any tweaking of the existing forum. My take (from what Sam, Martin and Dan says) is that it is just priorities. They are doing other things. So we just wait. -Derek
            Hide
            Helen Foster added a comment -

            Hi Olja,

            As far as I know, the reason why this feature has not yet been implemented is because we're hoping to replace the forum module with the OU's ForumNG module which includes thread subscription. Please see MDL-21538 for further details, and let's hope ForumNG makes it into Moodle 2.5!

            Show
            Helen Foster added a comment - Hi Olja, As far as I know, the reason why this feature has not yet been implemented is because we're hoping to replace the forum module with the OU's ForumNG module which includes thread subscription. Please see MDL-21538 for further details, and let's hope ForumNG makes it into Moodle 2.5!
            Hide
            Olja Petrovic added a comment -

            Any news on this?

            I'm trying to figure out from forums and tracker what is the future of this idea.

            It's so important! I tried to subscribe to forums but it just fills out my mailbox, it's quicker to go directly to the recent posts page and have the same thing!

            It's not about being selfish (like some discussions suggest) but about

            1) not wasting time clicking on all the threads you are participating in to see if there's any response (a lot of the times people ask questions but never bother to come back to the thread)

            2) I think thread abandonment level would drop if people would know that you responded to them, like maybe the novice forum poster who doesn't subscribe to the forum and stops checking up on the thread after a few days gets a response but never knows about it

            It't the opposite of selfish! I like to respond in many different forums, but it makes no sense for me to subscribe to all of them, it gives me no improvement over just visiting the recent posts page once a day. I don't want my mailbox to be an exact copy of the forum's recent posts page! Makes no sense!

            Please do something about it, I see it as crucial to the well being of the Moodle community and collaboration!

            Olja

            Show
            Olja Petrovic added a comment - Any news on this? I'm trying to figure out from forums and tracker what is the future of this idea. It's so important! I tried to subscribe to forums but it just fills out my mailbox, it's quicker to go directly to the recent posts page and have the same thing! It's not about being selfish (like some discussions suggest) but about 1) not wasting time clicking on all the threads you are participating in to see if there's any response (a lot of the times people ask questions but never bother to come back to the thread) 2) I think thread abandonment level would drop if people would know that you responded to them, like maybe the novice forum poster who doesn't subscribe to the forum and stops checking up on the thread after a few days gets a response but never knows about it It't the opposite of selfish! I like to respond in many different forums, but it makes no sense for me to subscribe to all of them, it gives me no improvement over just visiting the recent posts page once a day. I don't want my mailbox to be an exact copy of the forum's recent posts page! Makes no sense! Please do something about it, I see it as crucial to the well being of the Moodle community and collaboration! Olja
            Hide
            Derek Chirnside added a comment -

            I apologise to those who will get this message twice since I also posted here: http://tracker.moodle.org/browse/MDL-21538 which is a tracker item to suggest including ForumNG in core.
            ForumNG has the functionality in this tracker item (subscription at the discussion level,avoiding the term 'thread') Plus a lot more.

            It is a complex issue. Dan has asked a question here: http://moodle.org/mod/forum/discuss.php?d=195094#p851412 "a question which we have been discussing, what are the compelling features in ForumNG which you are keen to see in core?" I've had my say (long and rambly sorry). In summary: I'd like to see major improvements in the forum in the core.

            Some of you watching this tracker item may like to have a say in answer to Dan's question.

            -Derek

            Show
            Derek Chirnside added a comment - I apologise to those who will get this message twice since I also posted here: http://tracker.moodle.org/browse/MDL-21538 which is a tracker item to suggest including ForumNG in core. ForumNG has the functionality in this tracker item (subscription at the discussion level,avoiding the term 'thread') Plus a lot more. It is a complex issue. Dan has asked a question here: http://moodle.org/mod/forum/discuss.php?d=195094#p851412 "a question which we have been discussing, what are the compelling features in ForumNG which you are keen to see in core?" I've had my say (long and rambly sorry). In summary: I'd like to see major improvements in the forum in the core. Some of you watching this tracker item may like to have a say in answer to Dan's question. -Derek
            Hide
            Juan Leyva added a comment -

            I finally finished a local plugin that implements this feature.

            The plugin is not so much tested (I'm the only one tester) but it works fine for me.

            https://github.com/jleyva/moodle-local_dsubscription/zipball/master

            http://docs.moodle.org/22/en/Forum_discussion_subscription_plugin

            Show
            Juan Leyva added a comment - I finally finished a local plugin that implements this feature. The plugin is not so much tested (I'm the only one tester) but it works fine for me. https://github.com/jleyva/moodle-local_dsubscription/zipball/master http://docs.moodle.org/22/en/Forum_discussion_subscription_plugin
            Hide
            Martin Dougiamas added a comment -

            I can absolutely guarantee that this feature will be in the rewrite of forum when it happens.

            Show
            Martin Dougiamas added a comment - I can absolutely guarantee that this feature will be in the rewrite of forum when it happens.
            Hide
            John White added a comment -

            Hi again all. It's been two years since I commented on and worked this issue, and that time has gone in a flash for me!
            But I am really keen to help move this on. I now have one site on 2.0 (.2 very shortly), I can move a test site to the same. I'm expected to take a 3 million records site up to 2.0+ in Summer.
            I could re-write all my code edits to conform with Moodle 2 and make that work in each environment in turn (though they all use MySQL).

            But I gather a major re-write of Forum module is on the cards, so do I need to:
            . wait and do nothing because it out of my hands
            . wait and do nothing because we may move to ForumNG (OU)
            . re-write for 2.0.2 knowing that 2.1 will need yet another re-write, but at least helping to INFORM that revamp
            . work with developers who are looking at 2.1 forum

            I do have a few comments to put into the melting pot...
            1. That at least the Forum module works and has some strong functionality (the same can't be said for NWiki in 2.0 regrettably - pity the poor souls who thought moving to NWiki in 1.9 would help the transition!)
            2. That my mods would at least give administrators / teachers / users OPTIONS about subscriptions
            3. That we CAN keep the interface simple by hiding most of the new subscription models in Advanced settings, so that they can be ignored
            4. That it's in moodle.org that these subscription functions are most needed... I can't hack 500 emails a month just because I want to try answering someone's post!

            I'll be pleased to help in whatever way I can. I thought I'd retired once ... Ha ha!
            As always

            Show
            John White added a comment - Hi again all. It's been two years since I commented on and worked this issue, and that time has gone in a flash for me! But I am really keen to help move this on. I now have one site on 2.0 (.2 very shortly), I can move a test site to the same. I'm expected to take a 3 million records site up to 2.0+ in Summer. I could re-write all my code edits to conform with Moodle 2 and make that work in each environment in turn (though they all use MySQL). But I gather a major re-write of Forum module is on the cards, so do I need to: . wait and do nothing because it out of my hands . wait and do nothing because we may move to ForumNG (OU) . re-write for 2.0.2 knowing that 2.1 will need yet another re-write, but at least helping to INFORM that revamp . work with developers who are looking at 2.1 forum I do have a few comments to put into the melting pot... 1. That at least the Forum module works and has some strong functionality (the same can't be said for NWiki in 2.0 regrettably - pity the poor souls who thought moving to NWiki in 1.9 would help the transition!) 2. That my mods would at least give administrators / teachers / users OPTIONS about subscriptions 3. That we CAN keep the interface simple by hiding most of the new subscription models in Advanced settings, so that they can be ignored 4. That it's in moodle.org that these subscription functions are most needed... I can't hack 500 emails a month just because I want to try answering someone's post! I'll be pleased to help in whatever way I can. I thought I'd retired once ... Ha ha! As always
            Hide
            Helen Foster added a comment -

            Setting a fix version of DEV backlog for consideration for inclusion in next Moodle version.

            Show
            Helen Foster added a comment - Setting a fix version of DEV backlog for consideration for inclusion in next Moodle version.
            Hide
            Tim Hunt added a comment -

            Note that the OU's Forum NG already has this, and other goodness: http://moodle.org/mod/data/view.php?d=13&rid=2927. (Feel free to have a look and vote for MDL-21538, but note that ForumNG is still only beta release until about June.)

            Show
            Tim Hunt added a comment - Note that the OU's Forum NG already has this, and other goodness: http://moodle.org/mod/data/view.php?d=13&rid=2927 . (Feel free to have a look and vote for MDL-21538 , but note that ForumNG is still only beta release until about June.)
            Hide
            Martin Dougiamas added a comment -

            Really sorry but this has to wait until 2.1 and the big forum revamp

            Show
            Martin Dougiamas added a comment - Really sorry but this has to wait until 2.1 and the big forum revamp
            Hide
            Ray Lawrence added a comment -

            It doesn't seem that bad to me

            • As the number of places that can be subscribed will dramatically increase as a result of this change, it will become difficult for a user to know which items they are subscribed to. This knowledge is needed for example if the user wants to unsubscribe from all items in a specific moodle site.

            Solution: Provide unsubscribe from all forums in this course option. Do we need to change the current option?

            • Subscribing to a forum and subscribing to a thread within that forum are mutually exclusive. It may be difficult for a user to understand this intricacy unique to Moodle. Special care must be taken that users understand the consequences of unsubscribing from a forum when they still have thread subscriptions within that forum, or vice versa.

            1. I'ts not unique to Moodle, I can subsribe to single discussions in all manner of public fora.

            2. Solution: When user unsubscribes from forum they also unsubscribe from all discussions - existing and future. Being subscribed to a forum will mean automatic subscription to initial post in discussion, and the option to sunscribe to future posts.

            Easy

            Show
            Ray Lawrence added a comment - It doesn't seem that bad to me As the number of places that can be subscribed will dramatically increase as a result of this change, it will become difficult for a user to know which items they are subscribed to. This knowledge is needed for example if the user wants to unsubscribe from all items in a specific moodle site. Solution: Provide unsubscribe from all forums in this course option. Do we need to change the current option? Subscribing to a forum and subscribing to a thread within that forum are mutually exclusive. It may be difficult for a user to understand this intricacy unique to Moodle. Special care must be taken that users understand the consequences of unsubscribing from a forum when they still have thread subscriptions within that forum, or vice versa. 1. I'ts not unique to Moodle, I can subsribe to single discussions in all manner of public fora. 2. Solution: When user unsubscribes from forum they also unsubscribe from all discussions - existing and future. Being subscribed to a forum will mean automatic subscription to initial post in discussion, and the option to sunscribe to future posts. Easy
            Hide
            Olli Savolainen added a comment -

            Usability risks:

            • As the number of places that can be subscribed will dramatically increase as a result of this change, it will become difficult for a user to know which items they are subscribed to. This knowledge is needed for example if the user wants to unsubscribe from all items in a specific moodle site.
            • Subscribing to a forum and subscribing to a thread within that forum are mutually exclusive. It may be difficult for a user to understand this intricacy unique to Moodle. Special care must be taken that users understand the consequences of unsubscribing from a forum when they still have thread subscriptions within that forum, or vice versa.
            Show
            Olli Savolainen added a comment - Usability risks: As the number of places that can be subscribed will dramatically increase as a result of this change, it will become difficult for a user to know which items they are subscribed to. This knowledge is needed for example if the user wants to unsubscribe from all items in a specific moodle site. Subscribing to a forum and subscribing to a thread within that forum are mutually exclusive. It may be difficult for a user to understand this intricacy unique to Moodle. Special care must be taken that users understand the consequences of unsubscribing from a forum when they still have thread subscriptions within that forum, or vice versa.
            Hide
            Martin Dougiamas added a comment -

            My +1 for that! (Subscribing to discussion starters, and then being able to individually subscribe to any replies for each discussion).

            Show
            Martin Dougiamas added a comment - My +1 for that! (Subscribing to discussion starters, and then being able to individually subscribe to any replies for each discussion).
            Hide
            David Mudrak added a comment -

            Sorry - I have not read the comments above, so maybe it has already been mentioned. Just for the reference: from the today's Moodle Developers jabber chat

            (18:46:21) david: mahara allows you to subscribe to a given thread and it is very nice feature. What I would really love is being subscribed to any new discussion (ie the initial post) and I can choose to subscribe to a whole thread then
            (18:46:47) Eloy: yes, I love that too. Exactly implemented that way.

            Show
            David Mudrak added a comment - Sorry - I have not read the comments above, so maybe it has already been mentioned. Just for the reference: from the today's Moodle Developers jabber chat (18:46:21) david: mahara allows you to subscribe to a given thread and it is very nice feature. What I would really love is being subscribed to any new discussion (ie the initial post) and I can choose to subscribe to a whole thread then (18:46:47) Eloy: yes, I love that too. Exactly implemented that way.
            Hide
            Elena Ivanova added a comment -

            Hi John,
            re the last one.
            The buttons will take more space, but potentially they may state the same as links do: Subscribe (No) and Unsubscribe (Yes). Track vs Untrack. hmm...

            Show
            Elena Ivanova added a comment - Hi John, re the last one. The buttons will take more space, but potentially they may state the same as links do: Subscribe (No) and Unsubscribe (Yes). Track vs Untrack. hmm...
            Hide
            John White added a comment -

            Mark (and all),

            Thanks for the help.
            I am now well underway with the changes and refinements I should be able to post up a new version next week, but at the moment I have family over from Oz!

            I'm in accord with all your comments, I will add the 'hide' subscription button functionality, which is not difficult or code-heavy to implement. And I have now taken out the 'nudge'.

            The only thing I feel needs seeing - in the flesh - to appreciate to value of is the issue over the alt/help text. From my point of view the current state (1.9.4) is deeply confusing to newer users (old hands never read that text because the don't need to hang around long enough). By heading the column Subscribed? or Thread subscribed? as appropriate, the button state answers the question Yes or No, but without the help telling you what clicking does the users asks: do I click Yes when I mean Yes, or do I click Yes when I mean No? !

            More info soon I promise!

            John

            Show
            John White added a comment - Mark (and all), Thanks for the help. I am now well underway with the changes and refinements I should be able to post up a new version next week, but at the moment I have family over from Oz! I'm in accord with all your comments, I will add the 'hide' subscription button functionality, which is not difficult or code-heavy to implement. And I have now taken out the 'nudge'. The only thing I feel needs seeing - in the flesh - to appreciate to value of is the issue over the alt/help text. From my point of view the current state (1.9.4) is deeply confusing to newer users (old hands never read that text because the don't need to hang around long enough). By heading the column Subscribed? or Thread subscribed? as appropriate, the button state answers the question Yes or No, but without the help telling you what clicking does the users asks: do I click Yes when I mean Yes, or do I click Yes when I mean No? ! More info soon I promise! John
            Hide
            Mark Pearson added a comment -

            John,
            A bit late in commenting – hope not too late.
            Re comment of March-18:
            >I have also been much more POSITIVE about the button alt/help line, the 'No' button reads 'CLICK to ALLOW emails from this discussion' - so you know you will switch to 'Yes'.
            SHOULD THE SAME BE TRUE OF THE FORUM Yes/No BUTTON TEXT?

            My opinion (and it is only that) is that the rollover for the buttons should display the current state and not what would happen if you changed that state. Thus, rather than 'click this to do something else' it should state "emails currently enabled / emails currently disabled" and if the user wants to change the state they click the button. Similarly with the Forum Yes/No button. I believe this is the way other buttons work in Moodle (though I could be badly wrong). I think the advantage is that it easier for the programmer and the user to relate to a current state than a future potential change and extrapolate from that.

            > "ARE WE ALL AGREED ABOUT THIS?"
            Yes I agree. Simple is good. And your logic is impeccable.

            >My own view is that the 'ratings nudge' is a code-extravagant luxury and should be removed!
            I think the 'count posts' option is much neater, addresses concerns (documented above) and only invokes code if enabled, so it can always be ignored not heavy sites.
            IS THERE A CONCENSUS ABOUT THIS?

            I consense with your position here too.

            >IS IT BETTER TO GO DOWN ELOY'S SUGGESTED ROUTE, OR NOT, OR BOTH?
            Nope – I think your route is well argued.

            >At this time I don't see any way of having a list of subscribers fixed by admin or teacher. This would hide the 'Subscribe to this forum'-type links and drop-downs, which is all that is necessary!
            I would encourage you to add this feature. I think that it would be useful.

            Thanks for all the work you're doing on this John.

            Show
            Mark Pearson added a comment - John, A bit late in commenting – hope not too late. Re comment of March-18: >I have also been much more POSITIVE about the button alt/help line, the 'No' button reads 'CLICK to ALLOW emails from this discussion' - so you know you will switch to 'Yes'. SHOULD THE SAME BE TRUE OF THE FORUM Yes/No BUTTON TEXT? My opinion (and it is only that) is that the rollover for the buttons should display the current state and not what would happen if you changed that state. Thus, rather than 'click this to do something else' it should state "emails currently enabled / emails currently disabled" and if the user wants to change the state they click the button. Similarly with the Forum Yes/No button. I believe this is the way other buttons work in Moodle (though I could be badly wrong). I think the advantage is that it easier for the programmer and the user to relate to a current state than a future potential change and extrapolate from that. > "ARE WE ALL AGREED ABOUT THIS?" Yes I agree. Simple is good. And your logic is impeccable. >My own view is that the 'ratings nudge' is a code-extravagant luxury and should be removed! I think the 'count posts' option is much neater, addresses concerns (documented above) and only invokes code if enabled, so it can always be ignored not heavy sites. IS THERE A CONCENSUS ABOUT THIS? I consense with your position here too. >IS IT BETTER TO GO DOWN ELOY'S SUGGESTED ROUTE, OR NOT, OR BOTH? Nope – I think your route is well argued. >At this time I don't see any way of having a list of subscribers fixed by admin or teacher. This would hide the 'Subscribe to this forum'-type links and drop-downs, which is all that is necessary! I would encourage you to add this feature. I think that it would be useful. Thanks for all the work you're doing on this John.
            Hide
            John White added a comment -

            Hi all again,

            I want to add one other issue that could readily be solved at the same time as any implementation of thread-based subscriptions.
            This I have documented in MDL-18581. At this time I don't see any way of having a list of subscribers fixed by admin or teacher, so that students are unable to opt in or out, except the 'Yes, forever' option where the list is ALL those who have the initialsubscriptions capability set. So it CAN be done by adding a special role and unsetting the initialsubscriptions from the student role, but it could be done by adding a 'Fixed subscriber list' option in the forum force-subscribe settings. This would hide the 'Subscribe to this forum'-type links and drop-downs, which is all that is necessary!

            If such an item is to be added in it would be logical to decide on this and make it item 4 of the force-subscribe list.

            Regards,

            John

            Show
            John White added a comment - Hi all again, I want to add one other issue that could readily be solved at the same time as any implementation of thread-based subscriptions. This I have documented in MDL-18581 . At this time I don't see any way of having a list of subscribers fixed by admin or teacher, so that students are unable to opt in or out, except the 'Yes, forever' option where the list is ALL those who have the initialsubscriptions capability set. So it CAN be done by adding a special role and unsetting the initialsubscriptions from the student role, but it could be done by adding a 'Fixed subscriber list' option in the forum force-subscribe settings. This would hide the 'Subscribe to this forum'-type links and drop-downs, which is all that is necessary! If such an item is to be added in it would be logical to decide on this and make it item 4 of the force-subscribe list. Regards, John
            Hide
            John White added a comment -

            Ray, & ALL,

            There were so many typos in my last comment I must apologize for.
            I hope its just about readable.

            Cheers,
            John

            Show
            John White added a comment - Ray, & ALL, There were so many typos in my last comment I must apologize for. I hope its just about readable. Cheers, John
            Hide
            Ray Lawrence added a comment -

            Hi John,

            Phew. Lot's to digest.

            I just wanted to flag my concern on this quickly. I think brevity is important and that the wording needs to be ultra clear. I need a little time to think about this and I don't have that time today. Will be back soon...

            Show
            Ray Lawrence added a comment - Hi John, Phew. Lot's to digest. I just wanted to flag my concern on this quickly. I think brevity is important and that the wording needs to be ultra clear. I need a little time to think about this and I don't have that time today. Will be back soon...
            Hide
            John White added a comment -

            Ray,

            Thanks for the feedback. You are right that I have been liberal in my use of 'threads' in place of 'discussions', although I see them as equivalent as both can be used (recursively) to mean the whole or part of such (Wikipedia). Indeed where there is plenty of room for explanatory text I have preferred to use 'discussion threads' as this leaves none in doubt, I would hope.
            Where I am a bit concerned is where brevity is important, so should be column heading be 'discussion subscribed?' in place of 'thread subscribed?', and should the button be 'Show subscribed discussions first', not 'Show subscribed threads first'?
            I don't really feel that there is any confusion here - but if you and others tell me otherwise I will change it certainly.
            SO, DO I DROP THE EXPRESSION 'threads' ENTIRELY?

            You may also note another, more subtle measure here... I made the column 'thread subscribed?' with a ?, and changed the equivalent column in the forum index page to 'subscribed?' with a ? [programmers note: I have added an extra language string (one of many) 'subscribedq' just for that, not messed with the existing one],
            I did this because the Yes/No buttons, being a button i.e it implies you click to make it act, made little sense otherwise. I have also been much more POSITIVE about the button alt/help line, the 'No' button reads 'CLICK to ALLOW emails from this discussion' - so you know you will switch to 'Yes'.
            SHOULD THE SAME BE TRUE OF THE FORUM Yes/No BUTTON TEXT?

            As you know, when the user is allowed thread/discussion subscribe, I currently add 2 extra items in the subscription drop-down on the edit page.
            I NOW THINK THIS IS A MISTAKE!
            Its confusing, AND it causes an extra unnecessary weight of code in validation, in these circumstances we should only offer the new two...
            'Send me email copies of posts to this discussion', and
            'I don't want email copies of posts to this discussion'.
            ...because you always have the option for subscribing to the whole forum at the forum level.
            So I propose editing the code again.
            ARE WE ALL AGREED ABOUT THIS?

            Finally, I add two extra features which were not subscription issues using settings:
            $CFG->forum_ratingsnudge==1 and
            $CFG->forum_countposts==1
            described as items (8) and (9) above, WELL above!
            Do these say in?
            My own view is that the 'ratings nudge' is a code-extravagant luxury and should be removed!
            I think the 'count posts' option is much neater, addresses concerns (documented above) and only invokes code if enabled, so it can always be ignored not heavy sites.
            IS THERE A CONCENSUS ABOUT THIS?

            And finally finally, IS IT BETTER TO GO DOWN ELOY'S SUGGESTED ROUTE, OR NOT, OR BOTH?

            Regards to all,
            John

            Sorry about the capitals but I note that this issue has been raised to Critical, so their are things to resolve lest in delay 2.0 development.

            Show
            John White added a comment - Ray, Thanks for the feedback. You are right that I have been liberal in my use of 'threads' in place of 'discussions', although I see them as equivalent as both can be used (recursively) to mean the whole or part of such (Wikipedia). Indeed where there is plenty of room for explanatory text I have preferred to use 'discussion threads' as this leaves none in doubt, I would hope. Where I am a bit concerned is where brevity is important, so should be column heading be 'discussion subscribed?' in place of 'thread subscribed?', and should the button be 'Show subscribed discussions first', not 'Show subscribed threads first'? I don't really feel that there is any confusion here - but if you and others tell me otherwise I will change it certainly. SO, DO I DROP THE EXPRESSION 'threads' ENTIRELY? You may also note another, more subtle measure here... I made the column 'thread subscribed?' with a ?, and changed the equivalent column in the forum index page to 'subscribed?' with a ? [programmers note: I have added an extra language string (one of many) 'subscribedq' just for that, not messed with the existing one] , I did this because the Yes/No buttons, being a button i.e it implies you click to make it act, made little sense otherwise. I have also been much more POSITIVE about the button alt/help line, the 'No' button reads 'CLICK to ALLOW emails from this discussion' - so you know you will switch to 'Yes'. SHOULD THE SAME BE TRUE OF THE FORUM Yes/No BUTTON TEXT? As you know, when the user is allowed thread/discussion subscribe, I currently add 2 extra items in the subscription drop-down on the edit page. I NOW THINK THIS IS A MISTAKE! Its confusing, AND it causes an extra unnecessary weight of code in validation, in these circumstances we should only offer the new two... 'Send me email copies of posts to this discussion', and 'I don't want email copies of posts to this discussion'. ...because you always have the option for subscribing to the whole forum at the forum level. So I propose editing the code again. ARE WE ALL AGREED ABOUT THIS? Finally, I add two extra features which were not subscription issues using settings: $CFG->forum_ratingsnudge==1 and $CFG->forum_countposts==1 described as items (8) and (9) above, WELL above! Do these say in? My own view is that the 'ratings nudge' is a code-extravagant luxury and should be removed! I think the 'count posts' option is much neater, addresses concerns (documented above) and only invokes code if enabled, so it can always be ignored not heavy sites. IS THERE A CONCENSUS ABOUT THIS? And finally finally, IS IT BETTER TO GO DOWN ELOY'S SUGGESTED ROUTE, OR NOT, OR BOTH? Regards to all, John Sorry about the capitals but I note that this issue has been raised to Critical, so their are things to resolve lest in delay 2.0 development.
            Hide
            Ray Lawrence added a comment -

            Just a quick comment on the terms in the proposed interface.

            Typically "threads" are named "discussions" in Moodle.

            Suggest use "discussions" instead of "threads" in drop downs.

            Show
            Ray Lawrence added a comment - Just a quick comment on the terms in the proposed interface. Typically "threads" are named "discussions" in Moodle. Suggest use "discussions" instead of "threads" in drop downs.
            Hide
            John White added a comment -

            ...thinking about this more overnight, I believe it the mark/star implementation will require another table, with
            id,
            userid,
            post,
            mark,
            applytodiscussion (default:0).

            Theoretically you could leave out the mark column as any entry in the table would mean that a post is so marked - or, if post value is the initial post in a discussion and applytodiscussion=1 then the whole discussion is so marked. That way your need to star a discussion could be mated with the inevitable request to star a post.

            But my earlier suggestion of using the rating system would be quite wrong!!! This would limit the use of ratings further [I already wonder that it ought to be possible to apply more than one rating scale in a forum, e.g. ratings on two different criteria (scales)], so using up the rating to facilitate this star-marking would be retrograde!

            And having decided that this is independent of ratings then it is easier to conceive using AJAX to apply a star/mark/tag to either a discussion or a post, say, at the end of the title. At the Post level this would be binary (star off/star on), but at the discussion level this would be tri-state (no star/star on a post/star on this discussion) all held in the one table. It would be nice to use the extended character set here, but it this is going to cause problems in other languages, it could be 3 little gifs.

            I would be very happy to set about writing this - if enough people want it. Or to hand it over to you (Eloy), or another. But I would re-iterate that this looks like functionality independent of thread subscriptions to me.

            Dan Poltawski made a comment about Petr attacking subscriptions for 2.0 already, but I saw nothing about this in the roadmap.

            Regards,
            John

            Show
            John White added a comment - ...thinking about this more overnight, I believe it the mark/star implementation will require another table, with id, userid, post, mark, applytodiscussion (default:0). Theoretically you could leave out the mark column as any entry in the table would mean that a post is so marked - or, if post value is the initial post in a discussion and applytodiscussion=1 then the whole discussion is so marked. That way your need to star a discussion could be mated with the inevitable request to star a post. But my earlier suggestion of using the rating system would be quite wrong!!! This would limit the use of ratings further [I already wonder that it ought to be possible to apply more than one rating scale in a forum, e.g. ratings on two different criteria (scales)] , so using up the rating to facilitate this star-marking would be retrograde! And having decided that this is independent of ratings then it is easier to conceive using AJAX to apply a star/mark/tag to either a discussion or a post, say, at the end of the title. At the Post level this would be binary (star off/star on), but at the discussion level this would be tri-state (no star/star on a post/star on this discussion) all held in the one table. It would be nice to use the extended character set here, but it this is going to cause problems in other languages, it could be 3 little gifs. I would be very happy to set about writing this - if enough people want it. Or to hand it over to you (Eloy), or another. But I would re-iterate that this looks like functionality independent of thread subscriptions to me. Dan Poltawski made a comment about Petr attacking subscriptions for 2.0 already, but I saw nothing about this in the roadmap. Regards, John
            Hide
            John White added a comment -

            Eloy,

            Thanks for your input. I certainly appreciate what you say about wanting to mark, tag or star discussions. But I also think that more clarity is needed.

            First of all there is a great desire to choose to subscribe to some discussion threads and not others, whether this is achieved by yes/no or by a star (which then might have other uses) is in itself immaterial. But note, we either add a column in forum_subscriptions for thread or for star/mark but not for both. If we add both we for a matrix in which it is NOT possible to both STAR a discussion and NOT SUBSCRIBE to the forum. Of course the 'stars' should not be in this table, they do not belong here. But there is a problem putting them in forum_ratings too, since this refers to POSTS not discussions. So this would suggest that it requires a new table with id, userid, discussion, starmark - but this too will generate a problem further down the line, as someone if bound to ask to be able to star (in this sense) posts rather than discussions. And this fits better (I think) with your point about teachers putting a private mark on discussions, because surely this might pertain to POSTS instead.

            Added to this, if I want to starmark posts, and/or subscribe to said discussions, does this mean that when I star a post I subscribe to it, and if I have a setting in my profile to say 'Yes, subscribe to starred discussions' does that mean that I will NEVER want to star a post but not subscribe to that discussion? In short, whilst I wholly applaud the idea of 'starring' a discussion (privately) for whatever purpose I wish, AND I would CERTAINLY want to then 'show only starred discussions', I think this is almost independent of the subscription issue!

            Instead I want to suggest that we use the forum_ratings table for starmarks (unless we are serious about adding a whole new table) and the forum_subscriptions table for thread-based subscriptions (by adding 'thread') as I have done. In forum_ratings we need to add a boolean column for 'private' (default:0), which could be enough to do the trick, Then, if in the rating system, we have a very restricted type of rating (ONE STAR) that is allowed be Private (and that ratings can't be private and public at a whim - can you imagine the mess if the teacher didn't realize this was public or private!), then you could allow a one star rating to appear on the forum view page (if any post is starred), and that might be sufficient.

            Finally, however you implement a starmark system, in a new table or in ratings, if you have to collect the star data and check on a user profile setting, to see who is subscribed to a discussion then we could run into a worse issue with the program load than Dan Poltawski has expressed concern over at: http://moodle.org/mod/forum/discuss.php?d=118619#p520820 .

            Does this take us further?

            John

            Show
            John White added a comment - Eloy, Thanks for your input. I certainly appreciate what you say about wanting to mark, tag or star discussions. But I also think that more clarity is needed. First of all there is a great desire to choose to subscribe to some discussion threads and not others, whether this is achieved by yes/no or by a star (which then might have other uses) is in itself immaterial. But note, we either add a column in forum_subscriptions for thread or for star/mark but not for both. If we add both we for a matrix in which it is NOT possible to both STAR a discussion and NOT SUBSCRIBE to the forum. Of course the 'stars' should not be in this table, they do not belong here. But there is a problem putting them in forum_ratings too, since this refers to POSTS not discussions. So this would suggest that it requires a new table with id, userid, discussion, starmark - but this too will generate a problem further down the line, as someone if bound to ask to be able to star (in this sense) posts rather than discussions. And this fits better (I think) with your point about teachers putting a private mark on discussions, because surely this might pertain to POSTS instead. Added to this, if I want to starmark posts, and/or subscribe to said discussions, does this mean that when I star a post I subscribe to it, and if I have a setting in my profile to say 'Yes, subscribe to starred discussions' does that mean that I will NEVER want to star a post but not subscribe to that discussion? In short, whilst I wholly applaud the idea of 'starring' a discussion (privately) for whatever purpose I wish, AND I would CERTAINLY want to then 'show only starred discussions', I think this is almost independent of the subscription issue! Instead I want to suggest that we use the forum_ratings table for starmarks (unless we are serious about adding a whole new table) and the forum_subscriptions table for thread-based subscriptions (by adding 'thread') as I have done. In forum_ratings we need to add a boolean column for 'private' (default:0), which could be enough to do the trick, Then, if in the rating system, we have a very restricted type of rating (ONE STAR) that is allowed be Private (and that ratings can't be private and public at a whim - can you imagine the mess if the teacher didn't realize this was public or private!), then you could allow a one star rating to appear on the forum view page (if any post is starred), and that might be sufficient. Finally, however you implement a starmark system, in a new table or in ratings, if you have to collect the star data and check on a user profile setting, to see who is subscribed to a discussion then we could run into a worse issue with the program load than Dan Poltawski has expressed concern over at: http://moodle.org/mod/forum/discuss.php?d=118619#p520820 . Does this take us further? John
            Hide
            Eloy Lafuente (stronk7) added a comment -

            Hi John,

            threaded subscriptions (or starred discussions, as I use to call them in my mind) are definitively a good thing. And I think that having them in forums would be awesome! B-) Great work!

            Anyway, I'd want to comment here some basic concepts that have been in my mind since time ago related to this functionality:

            1) The concept itself: Some lines above I've commented that always I've seen this as "starred discussions", and IMO that's the key concept: To be able to mark (star) discussions. Nothing else.

            2) And then, the second (and more interesting) part. To be able to use those simple stars (marks) for a bunch of things. Obviously, the most prominent thing is to be able to receive personalised mails, containing only the starred discussions. But also can imagine the possibility of viewing (in the forum discussions page) only those starred discussions (instead of all them), or track only starred discussions, or as a tool for teachers to mark discussions privately, ... or whatever anybody more imaginative than me can do with those nice stars.

            But I'd separate 1) and 2) completely from start. First let's allow forum to have their discussion starred (really easy to do). We only need one configuration at global module level to enable/disable and another one at activity level (and probably one capability to decide who can "use" those little marks and everything associated with them).

            And then the second part, which complexity is bigger, specially when talking about subscriptions / mailouts, where there are a lot of settings to control (digest types, auto-subscription when posting, manual subscription, forced subscription...) needed to be integrated in a clever way if one forum has "stars" enabled and the user has permissions to use them. Sure it can be done, in fact John has done a great job thinking and implementing about that.

            But, once more, and as summary... I'd separated the "mark" thing from the subscription functionality. Simple but can give more power to the starred discussions in the future IMO.

            My 0.5 cents... ciao

            Show
            Eloy Lafuente (stronk7) added a comment - Hi John, threaded subscriptions (or starred discussions, as I use to call them in my mind) are definitively a good thing. And I think that having them in forums would be awesome! B-) Great work! Anyway, I'd want to comment here some basic concepts that have been in my mind since time ago related to this functionality: 1) The concept itself: Some lines above I've commented that always I've seen this as "starred discussions", and IMO that's the key concept: To be able to mark (star) discussions. Nothing else. 2) And then, the second (and more interesting) part. To be able to use those simple stars (marks) for a bunch of things. Obviously, the most prominent thing is to be able to receive personalised mails, containing only the starred discussions. But also can imagine the possibility of viewing (in the forum discussions page) only those starred discussions (instead of all them), or track only starred discussions, or as a tool for teachers to mark discussions privately, ... or whatever anybody more imaginative than me can do with those nice stars. But I'd separate 1) and 2) completely from start. First let's allow forum to have their discussion starred (really easy to do). We only need one configuration at global module level to enable/disable and another one at activity level (and probably one capability to decide who can "use" those little marks and everything associated with them). And then the second part, which complexity is bigger, specially when talking about subscriptions / mailouts, where there are a lot of settings to control (digest types, auto-subscription when posting, manual subscription, forced subscription...) needed to be integrated in a clever way if one forum has "stars" enabled and the user has permissions to use them. Sure it can be done, in fact John has done a great job thinking and implementing about that. But, once more, and as summary... I'd separated the "mark" thing from the subscription functionality. Simple but can give more power to the starred discussions in the future IMO. My 0.5 cents... ciao
            Hide
            Eloy Lafuente (stronk7) added a comment -

            Deleting My mockup! I thought I was in test server. Apologises by the noise!

            Show
            Eloy Lafuente (stronk7) added a comment - Deleting My mockup! I thought I was in test server. Apologises by the noise!
            Hide
            Eloy Lafuente (stronk7) added a comment -

            Edited UI Mockup <Starred Discussions Mockup1>: some clarification in user / forum interaction

            Show
            Eloy Lafuente (stronk7) added a comment - Edited UI Mockup <Starred Discussions Mockup1>: some clarification in user / forum interaction
            Hide
            Eloy Lafuente (stronk7) added a comment -

            Edited UI Mockup <Starred Discussions Mockup1>: Fixing some strings

            Show
            Eloy Lafuente (stronk7) added a comment - Edited UI Mockup <Starred Discussions Mockup1>: Fixing some strings
            Hide
            Eloy Lafuente (stronk7) added a comment -

            Added UI Mockup: <Starred Discussions Mockup1>

            Show
            Eloy Lafuente (stronk7) added a comment - Added UI Mockup: <Starred Discussions Mockup1>
            Hide
            John White added a comment -

            Added UI Mockup: <thread_subscribe1>

            Show
            John White added a comment - Added UI Mockup: <thread_subscribe1>
            Hide
            John White added a comment -

            In case that wasn't clear. The above problem is fully fixed in the file...

            moodle 1.9.4 (chk)-diff-2009-02-23-00-00-16.patch (111 kB)

            ...posted above on 23rd Feb.

            Show
            John White added a comment - In case that wasn't clear. The above problem is fully fixed in the file... moodle 1.9.4 (chk)-diff-2009-02-23-00-00-16.patch (111 kB) ...posted above on 23rd Feb.
            Hide
            John White added a comment -

            I've found the little bug I left the the 1.9.4 version

            Technical:
            In order for the buttons to run most efficiently (and compactly) when AJAX is enabled, each button holds only minimal information (the threadid and subscribe or not), the forumid is held in the overarching 'ajaxform' form which calls to 'yesno.php' or 'yesno_ajax.php'.
            But without AJAX enabled each button becomes its own mini form, because it contains all the data including the forumid (wasteful, but I think necessary - especially if we have to provide for the no Javascript in the browser case).
            However, I left the overarching ajaxform form in place (in this scenario its redundant), and this messes up the straight href call in the FIRST button ONLY (I presume that it reacts to the nearest </form> tag, not being happy with the fact that these forms are nested).

            Fix:
            In mod/forum/lib.php,
            in function forum_print_latest_discussions(...),
            the start and end of the form are wrapped by the AJAX enabled condition:

            Thus...
            About line 5347:
            if (!empty($CFG->forum_ajaxrating))

            { echo '<form id="ajaxform" method="post" action="yesno.php">'; echo '<input type="hidden" id="forumid" name="forumid" value="'.$forum->id.'">'; }

            About line 5537:
            if (!empty($CFG->forum_ajaxrating))

            { echo '</form>'; }

            Now all four scenarios work, for ALL buttons:
            AJAX enabled at server; Javascript enabled in browser
            AJAX enabled at server; Javascript disabled in browser
            AJAX disabled at server; Javascript enabled in browser
            AJAX disabled at server; Javascript disabled in browser

            I will post an amended patch shortly.

            Regards,
            John

            Show
            John White added a comment - I've found the little bug I left the the 1.9.4 version Technical: In order for the buttons to run most efficiently (and compactly) when AJAX is enabled, each button holds only minimal information (the threadid and subscribe or not), the forumid is held in the overarching 'ajaxform' form which calls to 'yesno.php' or 'yesno_ajax.php'. But without AJAX enabled each button becomes its own mini form, because it contains all the data including the forumid (wasteful, but I think necessary - especially if we have to provide for the no Javascript in the browser case). However, I left the overarching ajaxform form in place (in this scenario its redundant), and this messes up the straight href call in the FIRST button ONLY (I presume that it reacts to the nearest </form> tag, not being happy with the fact that these forms are nested). Fix: In mod/forum/lib.php, in function forum_print_latest_discussions(...), the start and end of the form are wrapped by the AJAX enabled condition: Thus... About line 5347: if (!empty($CFG->forum_ajaxrating)) { echo '<form id="ajaxform" method="post" action="yesno.php">'; echo '<input type="hidden" id="forumid" name="forumid" value="'.$forum->id.'">'; } About line 5537: if (!empty($CFG->forum_ajaxrating)) { echo '</form>'; } Now all four scenarios work, for ALL buttons: AJAX enabled at server; Javascript enabled in browser AJAX enabled at server; Javascript disabled in browser AJAX disabled at server; Javascript enabled in browser AJAX disabled at server; Javascript disabled in browser I will post an amended patch shortly. Regards, John
            Hide
            Daniele Cordella added a comment -

            Waiting and hoping...

            Show
            Daniele Cordella added a comment - Waiting and hoping...
            Hide
            John White added a comment -

            Daniele,

            Following Martin's enthusiasm for the idea, expressed above, and the other feedback I have received so far, it would seem that setting 'forcesubscribe' to option 5: 'Automatic thread-based subscriptions' in the new schema would seem to address this very issue best.
            This will still allow users to decide they don't wish to thread-subscribe, or conversely, to subscribe to the entire forum, but the default position is that if you contribute to a thread you become subscribed to it. With this setting new users are no longer run over by the email traffic, so they are perhaps less likely to unsubscribe! And old users (that's a self-deprication) get email responses to their posts, and just leave a tab open to General Problems to check what else is coming up perhaps.

            So, if this were running on Moodle.org, then personally - as a user, I would choose to accept the auto thread-subscription in the General Problems forum (immediately solving the problem you refer to), but I might subscribe to the entire 'Forum Module' forum, so that I can see if any related issues as they are raised. Useful flexibility.

            Regards,

            John

            Show
            John White added a comment - Daniele, Following Martin's enthusiasm for the idea, expressed above, and the other feedback I have received so far, it would seem that setting 'forcesubscribe' to option 5: 'Automatic thread-based subscriptions' in the new schema would seem to address this very issue best. This will still allow users to decide they don't wish to thread-subscribe, or conversely, to subscribe to the entire forum, but the default position is that if you contribute to a thread you become subscribed to it. With this setting new users are no longer run over by the email traffic, so they are perhaps less likely to unsubscribe! And old users (that's a self-deprication) get email responses to their posts, and just leave a tab open to General Problems to check what else is coming up perhaps. So, if this were running on Moodle.org, then personally - as a user, I would choose to accept the auto thread-subscription in the General Problems forum (immediately solving the problem you refer to), but I might subscribe to the entire 'Forum Module' forum, so that I can see if any related issues as they are raised. Useful flexibility. Regards, John
            Hide
            Daniele Cordella added a comment -

            Please let me add that I am still missing comments from people replying to my answers and asking for clarifications. (See: http://moodle.org/mod/forum/discuss.php?d=116610) This is bad... but I can't read each post of an entire forum to look for the message the maybe was sent to me. Moodlers remain without answers and this is not good at all. I believe this problem is matter of all moodlers not only a my personal problem.

            Show
            Daniele Cordella added a comment - Please let me add that I am still missing comments from people replying to my answers and asking for clarifications. (See: http://moodle.org/mod/forum/discuss.php?d=116610 ) This is bad... but I can't read each post of an entire forum to look for the message the maybe was sent to me. Moodlers remain without answers and this is not good at all. I believe this problem is matter of all moodlers not only a my personal problem.
            Hide
            John White added a comment -

            I hope developers will try this out and report back here if they succeed in uncovering any bugs - or have any other comments to make.

            I hope developers will comment on this addition, in due course.

            For my part I have found one bug that needs fixing - but superficially (at least) it is a surprisingly odd one!

            This is to do with the Yes/No buttons in the 'Thread subscribed?' column - as shown at http://moodle.org/mod/forum/discuss.php?d=115149#p505241

            In circumstances where the thread subscriptions are allowed and forum AJAX is switched on, these buttons use AJAX to subscribe/unsubscribe the user from the relevant threads perfectly, just as Eloy's AJAX ratings work! Even if Javascript is DISABLED in the browser (have you ever tried Google Maps without Javascript???) the buttons still perform correctly in their non-AJAX state (naturally this clunks, but thats life!).

            HOWEVER, if thread subscriptions are allowed, but AJAX is switched OFF, then the FIRST yes/no button in the list does not perform its no-AJAX duty! ONLY THE FIRST ONE!!!
            That's why I didn't spot it.

            The cause is associated with the way the browser (well, Firefox in particularly) deals will one FORM inside another (which structure does not occur with AJAX enabled).
            Now that I know the systems and the cause, I will find the solution soon enough.

            Meantime, this has no bearing on the functionality with AJAX enabled.

            Show
            John White added a comment - I hope developers will try this out and report back here if they succeed in uncovering any bugs - or have any other comments to make. I hope developers will comment on this addition, in due course. For my part I have found one bug that needs fixing - but superficially (at least) it is a surprisingly odd one! This is to do with the Yes/No buttons in the 'Thread subscribed?' column - as shown at http://moodle.org/mod/forum/discuss.php?d=115149#p505241 In circumstances where the thread subscriptions are allowed and forum AJAX is switched on, these buttons use AJAX to subscribe/unsubscribe the user from the relevant threads perfectly, just as Eloy's AJAX ratings work! Even if Javascript is DISABLED in the browser (have you ever tried Google Maps without Javascript???) the buttons still perform correctly in their non-AJAX state (naturally this clunks, but thats life!). HOWEVER, if thread subscriptions are allowed, but AJAX is switched OFF, then the FIRST yes/no button in the list does not perform its no-AJAX duty! ONLY THE FIRST ONE!!! That's why I didn't spot it. The cause is associated with the way the browser (well, Firefox in particularly) deals will one FORM inside another (which structure does not occur with AJAX enabled). Now that I know the systems and the cause, I will find the solution soon enough. Meantime, this has no bearing on the functionality with AJAX enabled.
            Hide
            John White added a comment -

            This patch should behave better than the previous one, in that I have done more to try to make the patch created on a local CVS work without objection, though you may find you have to use the --strip option (or supply the file location of each file).

            If 3 files yesno.php, yesno_ajax.php, yesno_ajax.js, appear in you top level directory move these to mod/forum.

            You still require the extra field in table mdl_forum_subscriptions shown in this discussion.

            Show
            John White added a comment - This patch should behave better than the previous one, in that I have done more to try to make the patch created on a local CVS work without objection, though you may find you have to use the --strip option (or supply the file location of each file). If 3 files yesno.php, yesno_ajax.php, yesno_ajax.js, appear in you top level directory move these to mod/forum. You still require the extra field in table mdl_forum_subscriptions shown in this discussion.
            Hide
            John White added a comment -

            At last I have finished the patch for 1.9.4, ready to attach here.

            This addresses quite a number of issues or developments that I commented on above, (as well as running correctly onto 1.9.4 which the patch for 1.9.3 would not)...

            For example,
            (1) The teacher switching the forum to 'force-subscribed' does not remove prior thread-subscriptions from the database (there is no need to remove these because they become irrelevant and invisible), thus they will return in the forum is switched back, but

            (2) the user clicking the 'Subscribe to this forum' link would remove their own thread-subscriptions, and is now warned:

            'You are currently subscribed to one or more discussion threads in this forum!

            If you wish to DELETE all such subscriptions so as to replace these with a subscription to the entire forum, click Yes.

            Otherwise click No.'

            ...but this warning only appears if the user has prior thread-subscriptions.

            (3) Similarly, if the teacher changes the forum settings to 'Subscriptions not allowed', when there were prior thread-subscriptions for some uses, she is warned:

            'There are already users subscribed to this forum or its discussion threads!

            If you wish to DELETE all such subscriptions, click Yes
            You will then need to save the forum settings again.

            Otherwise click No'

            ... I also believe that this overcomes a possible residual bug whereby it may previously have been possible for emails to creep through under circumstances where forum subscriptions existed before such a change - unless, of course, this was INTENTIONAL, in which case I had better modify a call to forum_unsubscribe_all() in mod_form.php!

            (4) It would be a disadvantage for teachers not to be able to see whether users were subscribed to at least some forum threads, and would so easily result in the teacher forcing a subscription to the whole forum in the view/edit subscribers panel. So now, if some users have thread subscriptions a warning to that effect appears at the top of this view/edit panel (in edit mode), and the number of theard subscriptions appear in parenthesis after each username.

            (5) I have given the user a button option to sort their subscribed threads to the top of the discussions listing if they so wish (view.php). This naturally only appears if 'forcesubscribe' is set to any of the new options 4 to 6.

            This one was potentially the messiest of all the changes, so I have kept this simple, and left a lengthy comment in mod/forum/lib.php (marked with a TODO) as to the method chosen, and why, and two other processes considered - both of which require doing more than a few minor tweaks + functions (or code blocks) added - which is really all I've done up to now.

            (6) I have modified Eloy's lovely AJAX to run the Yes/No buttons for thread subscription - no flashing back to refresh every time!!! But a method that works if Javascript is disabled in the local browser has been retained as well.

            (7) Emails needed to be modified both in terms of who gets what, and in the unsubscribe links in the footer, which now reflect whether the user is subscribe to the forum or a thread.

            (8) I have added in two features that might be considered completely superfluous (and hence need removing). The first of these is a 'ratings reminder', it would like this: If $CFG->forum_ratingsnudge==1 and ratings of some kind are enabled in the forum, then when the discussion ORIGINATOR chooses to unsubscribe from the thread they created, but that thread has been replied to by one of more users, if the originator has not rated any posts in the thread, they see a polite reminder.

            (9) and, in a similar vein, if $CFG->forum_countposts==1 then the count of the number of forum posts a user has contributed in that course appears in parenthesis after the user's name. This latter one was in response to http://moodle.org/mod/forum/discuss.php?d=116200#p513188.

            It is notable that (8) & (9) are both switched on or off globally, for all forums, as I have only provided control in mod/forum/settings.php (i.e. the admin settings page), realistically these might be more useful (or at least more acceptable) if they were controlled for the forum's own settings page, which of course would require new fields in the database!

            Regards,
            John

            Show
            John White added a comment - At last I have finished the patch for 1.9.4, ready to attach here. This addresses quite a number of issues or developments that I commented on above, (as well as running correctly onto 1.9.4 which the patch for 1.9.3 would not)... For example, (1) The teacher switching the forum to 'force-subscribed' does not remove prior thread-subscriptions from the database (there is no need to remove these because they become irrelevant and invisible), thus they will return in the forum is switched back, but (2) the user clicking the 'Subscribe to this forum' link would remove their own thread-subscriptions, and is now warned: 'You are currently subscribed to one or more discussion threads in this forum! If you wish to DELETE all such subscriptions so as to replace these with a subscription to the entire forum, click Yes. Otherwise click No.' ...but this warning only appears if the user has prior thread-subscriptions. (3) Similarly, if the teacher changes the forum settings to 'Subscriptions not allowed', when there were prior thread-subscriptions for some uses, she is warned: 'There are already users subscribed to this forum or its discussion threads! If you wish to DELETE all such subscriptions, click Yes You will then need to save the forum settings again. Otherwise click No' ... I also believe that this overcomes a possible residual bug whereby it may previously have been possible for emails to creep through under circumstances where forum subscriptions existed before such a change - unless, of course, this was INTENTIONAL, in which case I had better modify a call to forum_unsubscribe_all() in mod_form.php! (4) It would be a disadvantage for teachers not to be able to see whether users were subscribed to at least some forum threads, and would so easily result in the teacher forcing a subscription to the whole forum in the view/edit subscribers panel. So now, if some users have thread subscriptions a warning to that effect appears at the top of this view/edit panel (in edit mode), and the number of theard subscriptions appear in parenthesis after each username. (5) I have given the user a button option to sort their subscribed threads to the top of the discussions listing if they so wish (view.php). This naturally only appears if 'forcesubscribe' is set to any of the new options 4 to 6. This one was potentially the messiest of all the changes, so I have kept this simple, and left a lengthy comment in mod/forum/lib.php (marked with a TODO) as to the method chosen, and why, and two other processes considered - both of which require doing more than a few minor tweaks + functions (or code blocks) added - which is really all I've done up to now. (6) I have modified Eloy's lovely AJAX to run the Yes/No buttons for thread subscription - no flashing back to refresh every time!!! But a method that works if Javascript is disabled in the local browser has been retained as well. (7) Emails needed to be modified both in terms of who gets what, and in the unsubscribe links in the footer, which now reflect whether the user is subscribe to the forum or a thread. (8) I have added in two features that might be considered completely superfluous (and hence need removing). The first of these is a 'ratings reminder', it would like this: If $CFG->forum_ratingsnudge==1 and ratings of some kind are enabled in the forum, then when the discussion ORIGINATOR chooses to unsubscribe from the thread they created, but that thread has been replied to by one of more users, if the originator has not rated any posts in the thread, they see a polite reminder. (9) and, in a similar vein, if $CFG->forum_countposts==1 then the count of the number of forum posts a user has contributed in that course appears in parenthesis after the user's name. This latter one was in response to http://moodle.org/mod/forum/discuss.php?d=116200#p513188 . It is notable that (8) & (9) are both switched on or off globally, for all forums, as I have only provided control in mod/forum/settings.php (i.e. the admin settings page), realistically these might be more useful (or at least more acceptable) if they were controlled for the forum's own settings page, which of course would require new fields in the database! Regards, John
            Hide
            John White added a comment -

            I have almost completed a patch for 1.9.4. I will post this up in the next couple of days.
            I not only patches 1.9.4 correctly, but also has virtually al of the features that I mentioned as 'to do' items above.

            Regards,
            John

            Show
            John White added a comment - I have almost completed a patch for 1.9.4. I will post this up in the next couple of days. I not only patches 1.9.4 correctly, but also has virtually al of the features that I mentioned as 'to do' items above. Regards, John
            Hide
            John White added a comment -

            ...and further to the above comment, an even more pressing requirement would be a confirmation page that is required if ever the teacher/administrator switches from any of options 4 to 6 down to any of options 0 to 3, AND there ALREADY EXIST some thread subscriptions.
            Similar (though more robust) wording would be needed here:

            'You are about to remove thread-based subscription from this forum.
            This will mean wiping the current thread subscriptions your users
            already have. If you change back to thread-based subscriptions at
            a future date, doing so will not restore those users' subscriptions
            automatically; users will have to do this manually.
            Do you want to proceed with this?
            Yes / No'
            ...

            I'm sure other issues will be thought of too - but all of this is eminently doable!

            John

            Show
            John White added a comment - ...and further to the above comment, an even more pressing requirement would be a confirmation page that is required if ever the teacher/administrator switches from any of options 4 to 6 down to any of options 0 to 3, AND there ALREADY EXIST some thread subscriptions. Similar (though more robust) wording would be needed here: 'You are about to remove thread-based subscription from this forum. This will mean wiping the current thread subscriptions your users already have. If you change back to thread-based subscriptions at a future date, doing so will not restore those users' subscriptions automatically; users will have to do this manually. Do you want to proceed with this? Yes / No' ... I'm sure other issues will be thought of too - but all of this is eminently doable! John
            Hide
            John White added a comment -

            Martin,

            Thanks. And I too have this squarely aimed at moodle.org, but I can see it also running internal teacher support help on larger sites perhaps.
            Or homework support 'clubs' and the like.

            The most significant difference in the interface, but a modest one at that, is the extra column that any (subscribable) user sees when options 4 to 6 are used.
            I have placed a sample of what this would look like at: http://moodle.org/mod/forum/discuss.php?d=115149#p505235 along with the mtrace() output in the cron.
            As noted above, when option 6 is chosen (if ever) the yes/no buttons shown in that screen shot for option 4 or 5, become just plain text yes/no, no longer links.

            I have a couple more screen clips I can post up in the same place, though these are very simple: the drop-down the teacher sees in the forum settings, and the drop-down all users see when posting.

            I have got one more development that I have spotted as necessary...
            When the user clicks: 'Subscribes to this forum', but is already subscribed by thread, all their previous thread subscriptions have no meaning (and are therefore wiped), so there needs to be a confirmation page (that ONLY shows if you have 1 or more thread subscriptions) stating something like:

            'You are about to subscribe to this forum as a whole.
            This will mean losing your current subscriptions to
            separate threads. Do you want to proceed with this?
            Yes / No'
            ...

            But let's firstly see if what we have to date behaves itself.

            And whether I have the patch IN THE RIGHT FORMAT!!!

            Regards,

            John

            Show
            John White added a comment - Martin, Thanks. And I too have this squarely aimed at moodle.org, but I can see it also running internal teacher support help on larger sites perhaps. Or homework support 'clubs' and the like. The most significant difference in the interface, but a modest one at that, is the extra column that any (subscribable) user sees when options 4 to 6 are used. I have placed a sample of what this would look like at: http://moodle.org/mod/forum/discuss.php?d=115149#p505235 along with the mtrace() output in the cron. As noted above, when option 6 is chosen (if ever) the yes/no buttons shown in that screen shot for option 4 or 5, become just plain text yes/no, no longer links. I have a couple more screen clips I can post up in the same place, though these are very simple: the drop-down the teacher sees in the forum settings, and the drop-down all users see when posting. I have got one more development that I have spotted as necessary... When the user clicks: 'Subscribes to this forum', but is already subscribed by thread, all their previous thread subscriptions have no meaning (and are therefore wiped), so there needs to be a confirmation page (that ONLY shows if you have 1 or more thread subscriptions) stating something like: 'You are about to subscribe to this forum as a whole. This will mean losing your current subscriptions to separate threads. Do you want to proceed with this? Yes / No' ... But let's firstly see if what we have to date behaves itself. And whether I have the patch IN THE RIGHT FORMAT!!! Regards, John
            Hide
            Martin Dougiamas added a comment - - edited

            Thanks so much, John. This will make a very big difference for moodle.org especially (I maintain that getting email you did not specifically expect can be a useful thing in a smaller constructionist learning environment).

            I've not looked at the patch yet, but would also really appreciate feedback from others about the usability of the interface. Perhaps someone could post some screenshots too?

            Show
            Martin Dougiamas added a comment - - edited Thanks so much, John. This will make a very big difference for moodle.org especially (I maintain that getting email you did not specifically expect can be a useful thing in a smaller constructionist learning environment). I've not looked at the patch yet, but would also really appreciate feedback from others about the usability of the interface. Perhaps someone could post some screenshots too?
            Hide
            John White added a comment -

            I thought I would add a little about what it is you are meant to see with this patch...

            Obviously in the Forum settings panel the teacher/administrator has 3 extra options in the 'Force everyone to be subscribed?' drop-down.
            4. Allow thread or forum subscriptions
            5. Automatic thread-based subscriptions
            6. Force thread-based subscriptions

            Also in the Forum page itself the t/a sees an extra fast link 'Automatic thread-based subscriptions' (if not already chosen), below 'Force everyone to be subscribed'.

            • This seems to be to be the most likely option to want.

            When option 4 is chosen 'This forum allows everyone to choose whether to subscribe or not' appears as a reminder to all users.
            With option 5 the text is 'This forum provides thread-based subscriptions automatically'
            and with option 6 the text is 'This forum forces thread-based subscriptions'

            With either option 4 or 5 all users (with a subscribable role) see an extra column in the discussion list: 'Thread subscribed?'
            This contains yes/no buttons for instant thread subscription.
            Neither option stops the user from subscribing to the entire forum.

            Switch to option 6 and these buttons become yes/no text only, because you can only be subscribed to a thread by posting in it (this state does not recurse backwards to posts you have already made if the forum settings are changed).

            None of these settings stops the t/a from subscribing users to the forum, such that they appear in the forum subscribers list (which is unchanged), but if this is done the 'Thread subscribed?' column disappears as it is irrelevant!

            With options 4 or 5 the user can unsubscribe from the forum (because that is not forced) and then the yes/no thread subscription buttons appear, though if you move from forum-subscribed to forum-not-subscribed, all the buttons will be in the No state, and the user can change any.
            [Note: It could be argued that for state 5 these should all default to Yes, see below.]

            In state 4 or 5 you also get a fast link for 'Subscribe to this discussion thread' or 'Unsubscribe form this discussion thread' on the discussion page,
            and 2 extra options in the 'Subscription' drop-down on the posting page:
            'Send me email copies of posts to this discussion', and
            'I don't want email copies of posts to this discussion'.

            The difference between these to states is that whereas in state 4, the user's profile setting for autosubscribe is used to pre-load this 'subscription' setting,
            in state 5 it is always pre-loaded to 'Send me email copies of posts to this discussion'.
            So state 5 is automatic, not forced!.

            In state 6 this option box disappears, as do the fast links and buttons for subscribing and unsubscribing.
            In place you find that the user is subscribed to any thread they post in.

            But what if all students have been pre-subscribed to the forum by the t/a with thread subscriptions in state 6?
            Then the user receives mail from all threads until he/she posts to a discussion (new or old), then their forum subscription drops out and they are subscribed to the newly posted thread only.
            The principle of this one is that whereas the teacher may wish to hail all course participants with the merits of their forum, NOT removing the forum subscription would render it the same as Option 1 'Yes, forever'!!!

            The result of all of this is that mdl_forum_subscriptions now keeps track of who is subscribed and to what forum/thread. The value thread=0 means the whole forum, which is why this must be the default setting for column 'thread' so as to keep all existing subscriptions intact.

            Then the cron has an additional task to do. As well as collecting all forum subscribers, it must collect all thread subscribers but it must do so in such a way that the user profile data is collected only once for all cases; and this it does.
            Then a very small change to the condition that allows emails to be generated is all that is required.

            So far my tests appear good, but I have to admit to two untested areas,
            1. Does read-tracking upset this regimen - I think that unlikely, and
            2. What happens where groups are used for subscriptions.

            On these, or any other matters, I am keen to receive feedback.

            Regards,
            John

            Show
            John White added a comment - I thought I would add a little about what it is you are meant to see with this patch... Obviously in the Forum settings panel the teacher/administrator has 3 extra options in the 'Force everyone to be subscribed?' drop-down. 4. Allow thread or forum subscriptions 5. Automatic thread-based subscriptions 6. Force thread-based subscriptions Also in the Forum page itself the t/a sees an extra fast link 'Automatic thread-based subscriptions' (if not already chosen), below 'Force everyone to be subscribed'. This seems to be to be the most likely option to want. When option 4 is chosen 'This forum allows everyone to choose whether to subscribe or not' appears as a reminder to all users. With option 5 the text is 'This forum provides thread-based subscriptions automatically' and with option 6 the text is 'This forum forces thread-based subscriptions' With either option 4 or 5 all users (with a subscribable role) see an extra column in the discussion list: 'Thread subscribed?' This contains yes/no buttons for instant thread subscription. Neither option stops the user from subscribing to the entire forum. Switch to option 6 and these buttons become yes/no text only, because you can only be subscribed to a thread by posting in it (this state does not recurse backwards to posts you have already made if the forum settings are changed). None of these settings stops the t/a from subscribing users to the forum, such that they appear in the forum subscribers list (which is unchanged), but if this is done the 'Thread subscribed?' column disappears as it is irrelevant! With options 4 or 5 the user can unsubscribe from the forum (because that is not forced) and then the yes/no thread subscription buttons appear, though if you move from forum-subscribed to forum-not-subscribed, all the buttons will be in the No state, and the user can change any. [Note: It could be argued that for state 5 these should all default to Yes, see below.] In state 4 or 5 you also get a fast link for 'Subscribe to this discussion thread' or 'Unsubscribe form this discussion thread' on the discussion page, and 2 extra options in the 'Subscription' drop-down on the posting page: 'Send me email copies of posts to this discussion', and 'I don't want email copies of posts to this discussion'. The difference between these to states is that whereas in state 4, the user's profile setting for autosubscribe is used to pre-load this 'subscription' setting, in state 5 it is always pre-loaded to 'Send me email copies of posts to this discussion'. So state 5 is automatic, not forced!. In state 6 this option box disappears, as do the fast links and buttons for subscribing and unsubscribing. In place you find that the user is subscribed to any thread they post in. But what if all students have been pre-subscribed to the forum by the t/a with thread subscriptions in state 6? Then the user receives mail from all threads until he/she posts to a discussion (new or old), then their forum subscription drops out and they are subscribed to the newly posted thread only. The principle of this one is that whereas the teacher may wish to hail all course participants with the merits of their forum, NOT removing the forum subscription would render it the same as Option 1 'Yes, forever'!!! The result of all of this is that mdl_forum_subscriptions now keeps track of who is subscribed and to what forum/thread. The value thread=0 means the whole forum, which is why this must be the default setting for column 'thread' so as to keep all existing subscriptions intact. Then the cron has an additional task to do. As well as collecting all forum subscribers, it must collect all thread subscribers but it must do so in such a way that the user profile data is collected only once for all cases; and this it does. Then a very small change to the condition that allows emails to be generated is all that is required. So far my tests appear good, but I have to admit to two untested areas, 1. Does read-tracking upset this regimen - I think that unlikely, and 2. What happens where groups are used for subscriptions. On these, or any other matters, I am keen to receive feedback. Regards, John
            Hide
            John White added a comment -

            Patch as described in comment by John White

            Show
            John White added a comment - Patch as described in comment by John White
            Hide
            John White added a comment -

            Hi all.

            I had for quite a time felt that this was a significant shortfall in the forum module, which most particularly shows up in the General Problems forum where it is quite plain to most regular contributors that new users are completely shocked and overwhelmed by the email storm that results from their first post, such that they hurriedly switch of their subscription to the forum and never get to see the responses they may have had!

            It is almost equally a problem if you unsubscribe and just keep a browser tab open and refresh it periodically - if you have answered half-a-dozen queries, then within a few days you can have forgotten which titles/users you replied to (would I EVER write that down?), they become so spaced out in the list, you may never get to see that you were asked a follow-up question! This is a less than fabulous situation.

            However, instead of complaining, I have, as I have posted in moodle.org forum and the Forum module forum, done something about it.

            I am posting up a patch (originally on 1.9.3) that implements subscription by discussion thread (discussion <=> thread here). Tim Hunt will I hope, in due course, check the patch to make sure I have got that part right - I believe so!

            To work this patch you need an extra column in the table (mdl_)forum_subscriptions, so you must first run the sql statement:

            ALTER TABLE `mdl_forum_subscriptions` ADD COLUMN `thread` BIGINT(10) UNSIGNED NOT NULL DEFAULT 0 AFTER `forum`, ADD INDEX `mdl_forusubs_thr_ix` (`thread`);

            I have also left off a tiny (and non-critical) bit from theme/standard/styles_layout.css...

            #mod-forum-discuss .subscription

            { float: right; text-align:right; white-space: nowrap; }

            But for the rest - the changes in 11 files - are in the patch.

            I have annotated all changes, and you can search to find each marked by the initials JWC (later we will of course want to remove such egotism!!)

            I have tested it as thoroughly as I can - it is implemented on a training site in the UK, and I've got a team of people roped in to comment on it, but I am very open to suggestions for improvements. I can think of one myself: on a big site you might appreciate a check box to 'show me discussions I'm subscribed to' on the forum page, or even 'show subscribed discussions first' to alter the listing order!

            I too agree that this could all seem a bit overwhelming to course leader and to users. For that reason I have tried to keep the options as simple as possible and to give useful (but brief) information in the help. In short I have given three extra options for the forum itself, and what the user can see is based on which of these is chosen:

            4. Allow thread or forum subscriptions
            5. Automatic thread-based subscriptions
            6. Force thread-based subscriptions

            I'm sure there needs to be a debate about these settings to. You need to consider, particularly, what effect each would have if the teacher subscribes all course users to a the forum, when the default setting is one of the above. This issue has been considered and I hope you will appreciate the subtlety of the results.

            Regards,
            John

            Show
            John White added a comment - Hi all. I had for quite a time felt that this was a significant shortfall in the forum module, which most particularly shows up in the General Problems forum where it is quite plain to most regular contributors that new users are completely shocked and overwhelmed by the email storm that results from their first post, such that they hurriedly switch of their subscription to the forum and never get to see the responses they may have had! It is almost equally a problem if you unsubscribe and just keep a browser tab open and refresh it periodically - if you have answered half-a-dozen queries, then within a few days you can have forgotten which titles/users you replied to (would I EVER write that down?), they become so spaced out in the list, you may never get to see that you were asked a follow-up question! This is a less than fabulous situation. However, instead of complaining, I have, as I have posted in moodle.org forum and the Forum module forum, done something about it. I am posting up a patch (originally on 1.9.3) that implements subscription by discussion thread (discussion <=> thread here). Tim Hunt will I hope, in due course, check the patch to make sure I have got that part right - I believe so! To work this patch you need an extra column in the table (mdl_)forum_subscriptions, so you must first run the sql statement: ALTER TABLE `mdl_forum_subscriptions` ADD COLUMN `thread` BIGINT(10) UNSIGNED NOT NULL DEFAULT 0 AFTER `forum`, ADD INDEX `mdl_forusubs_thr_ix` (`thread`); I have also left off a tiny (and non-critical) bit from theme/standard/styles_layout.css... #mod-forum-discuss .subscription { float: right; text-align:right; white-space: nowrap; } But for the rest - the changes in 11 files - are in the patch. I have annotated all changes, and you can search to find each marked by the initials JWC (later we will of course want to remove such egotism!!) I have tested it as thoroughly as I can - it is implemented on a training site in the UK, and I've got a team of people roped in to comment on it, but I am very open to suggestions for improvements. I can think of one myself: on a big site you might appreciate a check box to 'show me discussions I'm subscribed to' on the forum page, or even 'show subscribed discussions first' to alter the listing order! I too agree that this could all seem a bit overwhelming to course leader and to users. For that reason I have tried to keep the options as simple as possible and to give useful (but brief) information in the help. In short I have given three extra options for the forum itself, and what the user can see is based on which of these is chosen: 4. Allow thread or forum subscriptions 5. Automatic thread-based subscriptions 6. Force thread-based subscriptions I'm sure there needs to be a debate about these settings to. You need to consider, particularly, what effect each would have if the teacher subscribes all course users to a the forum, when the default setting is one of the above. This issue has been considered and I hope you will appreciate the subtlety of the results. Regards, John
            Hide
            Jeff Sherk added a comment -

            I would also like to see the ability to subscribe to a specific thread within a forum, instead of the whole forum.

            Show
            Jeff Sherk added a comment - I would also like to see the ability to subscribe to a specific thread within a forum, instead of the whole forum.
            Hide
            Artem Andreev added a comment -

            I think "rss for thread" may be good alternative solution? And it is easy to implement...

            Show
            Artem Andreev added a comment - I think "rss for thread" may be good alternative solution? And it is easy to implement...
            Hide
            Derek Chirnside added a comment -

            I think this is a much needed enhancement.

            Where are we at now?
            Do we need more clarification of a best implementation? I can see a workable solution in the comments above.

            I notice it says "Fix version 2.0" Does that mean it is in the 2.0 plan?

            Show
            Derek Chirnside added a comment - I think this is a much needed enhancement. Where are we at now? Do we need more clarification of a best implementation? I can see a workable solution in the comments above. I notice it says "Fix version 2.0" Does that mean it is in the 2.0 plan?
            Hide
            Mark Penny added a comment -

            I'm coming in a bit late, but I'm all for minimal defaults and maximum options. It's a bit irking to post a focused request or response (Moodle forums) and wind up getting daily updates on everything else in the place. On the other hand, there are times when you want to be kept abreast of the whole big discussion.

            Grats and kudos to everybody trying to help with this.

            Show
            Mark Penny added a comment - I'm coming in a bit late, but I'm all for minimal defaults and maximum options. It's a bit irking to post a focused request or response (Moodle forums) and wind up getting daily updates on everything else in the place. On the other hand, there are times when you want to be kept abreast of the whole big discussion. Grats and kudos to everybody trying to help with this.
            Hide
            Ray Lawrence added a comment -

            Thanks. The effect I was trying to get to was a simple method of allowing users to manage subscription to individual discussions but retain a simple way of alerting them to new discussions (which they could then opt to subscribe to).

            Although it may be desirable to only subscribe to reply to posts of one's own a student may do this inadvertently and receive a very partial view of the discussion. And of course it may not be what the teacher intends.

            + 1 for being able to disable subscription emails and not others. Has this been logged a feature request before?

            Show
            Ray Lawrence added a comment - Thanks. The effect I was trying to get to was a simple method of allowing users to manage subscription to individual discussions but retain a simple way of alerting them to new discussions (which they could then opt to subscribe to). Although it may be desirable to only subscribe to reply to posts of one's own a student may do this inadvertently and receive a very partial view of the discussion. And of course it may not be what the teacher intends. + 1 for being able to disable subscription emails and not others. Has this been logged a feature request before?
            Hide
            Matt Gibson added a comment -

            It does seem complicated, but I do agree with Alejo that it is needed. I really want to be able to subscribe to individual threads and not necessarily forums as a whole, especially on moodle.org. An additional option would be to have an automatic thread subscription if you post to a forum, and maybe a separate option to not subscribe to the forum, or the thread as a whole, but only to any replies to a specific post made by you.

            Interface wise, I think checkboxes/toggle links at the top of forums, threads and own-posts, as well as a whole site subscriptions interface, showing forums and threads you are subscribed to in tree format (course-forum-threads) so you can unsubscribe to many at once.

            It would also be very useful to have a block showing recent posts only in my subscribed forums/threads, which would suit me better than the emails to be honest. It would be good if I could turn off subscription emails without turning off all emails and still see subscribed threads amalgamated on site somewhere

            Show
            Matt Gibson added a comment - It does seem complicated, but I do agree with Alejo that it is needed. I really want to be able to subscribe to individual threads and not necessarily forums as a whole, especially on moodle.org. An additional option would be to have an automatic thread subscription if you post to a forum, and maybe a separate option to not subscribe to the forum, or the thread as a whole, but only to any replies to a specific post made by you. Interface wise, I think checkboxes/toggle links at the top of forums, threads and own-posts, as well as a whole site subscriptions interface, showing forums and threads you are subscribed to in tree format (course-forum-threads) so you can unsubscribe to many at once. It would also be very useful to have a block showing recent posts only in my subscribed forums/threads, which would suit me better than the emails to be honest. It would be good if I could turn off subscription emails without turning off all emails and still see subscribed threads amalgamated on site somewhere
            Hide
            Ray Lawrence added a comment -

            This seems quite complicated. Would a simpler solution be:

            • Fourm subscription settings as now.
            • If subscribed to a forum the user receives the initial post of every new dicussion and subsequent replies until they unsubscribe.
            • The subscription links in emails of posts, the drop down in the post interface and user profile change to manage subscription to discussions not forums.
            • Unsubscribe forums links on forum page and forums index page remain as now.
            Show
            Ray Lawrence added a comment - This seems quite complicated. Would a simpler solution be: Fourm subscription settings as now. If subscribed to a forum the user receives the initial post of every new dicussion and subsequent replies until they unsubscribe. The subscription links in emails of posts, the drop down in the post interface and user profile change to manage subscription to discussions not forums. Unsubscribe forums links on forum page and forums index page remain as now.
            Hide
            Alejo Becerra Díaz added a comment -

            Since my first contact with Moodle, a couple of years ago, one of the things that surprised me the most was Forum subscription instead of the omnipresent thread subscription. I can see pedagogical and technical advantages of course subscription, but thread subscription is a must in certain situation.

            Technically, I think that thread subscription settings should be the value to check when sending emails. Forum subscription and unsubscription choices should reset the values of all threads and be the default value for new threads in that forum, but this could be overridden later by particular thread choices (unless forced forum subscription, of course).

            Regarding the interface, apart from looking at consolidated forum systems (phpbb and the like), here are my suggestions.
            Generally speaking, there should be duplicated options for subscribing/unsubscribing to/from a thread or the entire forum.
            In the profile, for example, the "Forum auto-subscribe" setting option should have 3 options, adding one named "Yes, when I post subscribe me to that thread" and rewording the negative one "No, don't automatically subscribe me".
            The settings in the new post form should be similar, with 4 options:

            • "Send me email copies of posts to this forum"
            • "Send me email copies of posts to this thread"
            • "I don't want email copies of posts to this thread"
            • "I don't want email copies of posts to this forum"

            The default value should be the current option (in this order: no subscription to thread, forum subscription enabled, thread subscription enabled; no subscription to forum should always be explicitly choiced, giving its consequences). The profile setting should only be used for the first post to a forum.

            The same duplication philosophy should apply to top-right options while viewing a forum, and to mail unsubscribe options.

            There should be new options while viewing a discussion, similar to the top-right option while viewing a forum, to subscribe/unsubscribe to/from the thread.
            There should be a new column in the "forum view" page for each discussion, "Subscribed", with the same behaviour that the same column in the "forums index" page, but, of course, at a thread level.

            I hope this help.

            Show
            Alejo Becerra Díaz added a comment - Since my first contact with Moodle, a couple of years ago, one of the things that surprised me the most was Forum subscription instead of the omnipresent thread subscription. I can see pedagogical and technical advantages of course subscription, but thread subscription is a must in certain situation. Technically, I think that thread subscription settings should be the value to check when sending emails. Forum subscription and unsubscription choices should reset the values of all threads and be the default value for new threads in that forum, but this could be overridden later by particular thread choices (unless forced forum subscription, of course). Regarding the interface, apart from looking at consolidated forum systems (phpbb and the like), here are my suggestions. Generally speaking, there should be duplicated options for subscribing/unsubscribing to/from a thread or the entire forum. In the profile, for example, the "Forum auto-subscribe" setting option should have 3 options, adding one named "Yes, when I post subscribe me to that thread" and rewording the negative one "No, don't automatically subscribe me". The settings in the new post form should be similar, with 4 options: "Send me email copies of posts to this forum" "Send me email copies of posts to this thread" "I don't want email copies of posts to this thread" "I don't want email copies of posts to this forum" The default value should be the current option (in this order: no subscription to thread, forum subscription enabled, thread subscription enabled; no subscription to forum should always be explicitly choiced, giving its consequences). The profile setting should only be used for the first post to a forum. The same duplication philosophy should apply to top-right options while viewing a forum, and to mail unsubscribe options. There should be new options while viewing a discussion, similar to the top-right option while viewing a forum, to subscribe/unsubscribe to/from the thread. There should be a new column in the "forum view" page for each discussion, "Subscribed", with the same behaviour that the same column in the "forums index" page, but, of course, at a thread level. I hope this help.
            Hide
            Martin Dougiamas added a comment -

            Assigning to me temporarily because Vy-Shane no longer works for Moodle HQ.

            Show
            Martin Dougiamas added a comment - Assigning to me temporarily because Vy-Shane no longer works for Moodle HQ.
            Hide
            D.I. von Briesen added a comment -

            I'd put forth that this whole issue, which is a big one (spam vs. keeping in touch) can be resolved by subscribing users to the threads to which they post. Otherwise, it'd inherit their settings.

            In both cases, their choices should be a bit less implied. Currently there's a profile setting (auto subscribe when i post) that most don't know about, and it catches folks off guard.

            There's an option at the bottom of a post, and other places where you can subscribe/unsubscribe, but why not include that within the text of the button- so you could have two buttons:

            [post this message and email me responses to it] [post this message but don't send me related emails]
            you could even have another that says [post this message and email me all discussions on this FORUM]

            for layout, you could simplify by having a message:
            click one of the buttons below to post your message and set your email subscription options. ( ? ) <-- help button
            [email me responses to my message][don't send me related emails][don't send me any emails from this whole forum]

            Or you could have 3 radio buttons and a [submit post] button - the radio button could inherit from a setting profile.

            The 2nd option here would unusbscribe someone to a thread where they were already subscribed, and would not be available when the admin had forced subscription on all.

            Show
            D.I. von Briesen added a comment - I'd put forth that this whole issue, which is a big one (spam vs. keeping in touch) can be resolved by subscribing users to the threads to which they post. Otherwise, it'd inherit their settings. In both cases, their choices should be a bit less implied. Currently there's a profile setting (auto subscribe when i post) that most don't know about, and it catches folks off guard. There's an option at the bottom of a post, and other places where you can subscribe/unsubscribe, but why not include that within the text of the button- so you could have two buttons: [post this message and email me responses to it] [post this message but don't send me related emails] you could even have another that says [post this message and email me all discussions on this FORUM] for layout, you could simplify by having a message: click one of the buttons below to post your message and set your email subscription options. ( ? ) <-- help button [email me responses to my message] [don't send me related emails] [don't send me any emails from this whole forum] Or you could have 3 radio buttons and a [submit post] button - the radio button could inherit from a setting profile. The 2nd option here would unusbscribe someone to a thread where they were already subscribed, and would not be available when the admin had forced subscription on all.
            Hide
            Martin Dougiamas added a comment -

            From Martin Dougiamas (martin at moodle.com) Thursday, 8 July 2004, 09:42 AM:

            Perhaps you have some good ideas about how the interface should work.

            From Jurgis Pralgauskis (jpral at yahoo.com) Thursday, 23 September 2004, 08:47 AM:

            I would say subscribe per topic, as thread imight be too dynamic..

            in my post http://moodle.org/mod/forum/discuss.php?d=11448 I said little more (bout interface <- phpbb

            From Martin Dougiamas (martin at moodle.com) Wednesday, 29 September 2004, 07:04 PM:

            discussion topic = thread (in Moodle). Same thing.

            Interface ideas still wanted (there are none at the previously mentioned discussion). What we need are specific clean ideas for making it work within the Moodle form framework.

            My brain is tired so I can't even get started thinking about it.

            From jaime alamo (jaime.alamo at uv.es) Monday, 13 December 2004, 11:53 PM:

            As some forums are overposted thread subcription, instead of forum subcription, should be more effective for personal interests and server load.

            From jaime alamo (jaime.alamo at uv.es) Tuesday, 14 December 2004, 12:17 AM:

            1. Add to the forum list of subscribers a thread list of subscribers for each thread.

            2. In each post add links to subscribe to this thread and unsubscribe to this thread

            3. In the liked page, check the user is not already subscribed to the forum or the thread and proceed accordingly. People wanting thread mails should unsubscribe to that forum.

            4. For each new post, the mailer should send mails to both lists

            From brian king (free.as.in.speech at gmail.com) Thursday, 8 December 2005, 08:34 PM:

            I've implemented this for a moodle 1.5.3 installation, and would be very happy to share it - especially if it gets implemented on moodle.org!

            Personally, it's always something that bothered me about the moodle.org forums - there are certain discussions that I'd like to follow, but not necessarily all discussions in a single forum.

            How I implemented it:

            When creating / editing a forum, I added a question whether or not to allow subscriptions to single discussions.

            If discussion subscription has been allowed for a forum, then there is a (un)subcribe to this discussion link at the top of each discussion. Entire forum subscriptions work as they did before; I didn't change how they function.

            Technical details:

            • added a 'discussion' column to the forum_subscriptions table
            • added an 'allowthreadsubscription' column to the forum table
            • minor changes made to the files:

            lang/en/forum.php

            mod/forum/mod.html

            mod/forum/lib.php

            mod/forum/discuss.php

            mod/forum/subscribe.php

            From Brian Sea (sea at umr.edu) Monday, 12 December 2005, 03:29 AM:

            This should be added to CVS, IMO. I don't see how providing the option can hurt, especially since it's coded.

            From Robert Barreca (rob at electronicinsight.com) Monday, 3 April 2006, 06:23 AM:

            Could you upload a patch for this? I'd like to implement it for School Engine.

            From Martin Dougiamas (martin at moodle.com) Monday, 3 April 2006, 10:49 AM:

            Yes, I'll implement this if we get a patch

            From brian king (free.as.in.speech at gmail.com) Wednesday, 5 April 2006, 04:35 PM:

            as i last left the code, there was still some buggy behavior. seeing as people are interested, i'll dig out the code and debug it - and should have a patch in the next couple of weeks.

            Show
            Martin Dougiamas added a comment - From Martin Dougiamas (martin at moodle.com) Thursday, 8 July 2004, 09:42 AM: Perhaps you have some good ideas about how the interface should work. From Jurgis Pralgauskis (jpral at yahoo.com) Thursday, 23 September 2004, 08:47 AM: I would say subscribe per topic, as thread imight be too dynamic.. in my post http://moodle.org/mod/forum/discuss.php?d=11448 I said little more (bout interface <- phpbb From Martin Dougiamas (martin at moodle.com) Wednesday, 29 September 2004, 07:04 PM: discussion topic = thread (in Moodle). Same thing. Interface ideas still wanted (there are none at the previously mentioned discussion). What we need are specific clean ideas for making it work within the Moodle form framework. My brain is tired so I can't even get started thinking about it. From jaime alamo (jaime.alamo at uv.es) Monday, 13 December 2004, 11:53 PM: As some forums are overposted thread subcription, instead of forum subcription, should be more effective for personal interests and server load. From jaime alamo (jaime.alamo at uv.es) Tuesday, 14 December 2004, 12:17 AM: 1. Add to the forum list of subscribers a thread list of subscribers for each thread. 2. In each post add links to subscribe to this thread and unsubscribe to this thread 3. In the liked page, check the user is not already subscribed to the forum or the thread and proceed accordingly. People wanting thread mails should unsubscribe to that forum. 4. For each new post, the mailer should send mails to both lists From brian king (free.as.in.speech at gmail.com) Thursday, 8 December 2005, 08:34 PM: I've implemented this for a moodle 1.5.3 installation, and would be very happy to share it - especially if it gets implemented on moodle.org! Personally, it's always something that bothered me about the moodle.org forums - there are certain discussions that I'd like to follow, but not necessarily all discussions in a single forum. How I implemented it: When creating / editing a forum, I added a question whether or not to allow subscriptions to single discussions. If discussion subscription has been allowed for a forum, then there is a (un)subcribe to this discussion link at the top of each discussion. Entire forum subscriptions work as they did before; I didn't change how they function. Technical details: added a 'discussion' column to the forum_subscriptions table added an 'allowthreadsubscription' column to the forum table minor changes made to the files: lang/en/forum.php mod/forum/mod.html mod/forum/lib.php mod/forum/discuss.php mod/forum/subscribe.php From Brian Sea (sea at umr.edu) Monday, 12 December 2005, 03:29 AM: This should be added to CVS, IMO. I don't see how providing the option can hurt, especially since it's coded. From Robert Barreca (rob at electronicinsight.com) Monday, 3 April 2006, 06:23 AM: Could you upload a patch for this? I'd like to implement it for School Engine. From Martin Dougiamas (martin at moodle.com) Monday, 3 April 2006, 10:49 AM: Yes, I'll implement this if we get a patch From brian king (free.as.in.speech at gmail.com) Wednesday, 5 April 2006, 04:35 PM: as i last left the code, there was still some buggy behavior. seeing as people are interested, i'll dig out the code and debug it - and should have a patch in the next couple of weeks.

              Dates

              • Created:
                Updated:
                Resolved:

                Agile