Details

    • Database:
      Any
    • Testing Instructions:
      Hide

      Test difficulty: Irritating and pernickety

      Background

      For this test you will need:

      • a working mail configuration
      • four users enrolled in a courses. Each must have their own address
      • four forums (perhaps create Auto Subscribed?)
      • one editing teacher (can be one of the four users, but may get confusing)
      • and a partridge in a pair tree

      Setup

        Forum 1 Forum 2 Forum 3 Forum 4 Profile digest setting
      User A Default Digest Subject Digest Complete No Digest No Digest
      User B No Digest Default Digest Subject Digest Complete Digest Complete
      User C Digest Complete No Digest Default Digest Subject Digest Subject
      User D Digest Subject Digest Complete No Digest Default No Digest

      Note: At present, there aren't many links to /mod/forum/index.php to
      control the per-forum digest settings. This patch does not seek to
      address this as I think that we need to consider how this settings page
      fits into other proposed changes (advanced forums, and allowing
      unsubscription from forced forums)
      Therefore you can access the page from /mod/forum/index.php?id=[courseid].
      It is linked from forum e-mails, and the activities block.

      Instructions

      Note: In these test instructions, the following acronyms are used:

      • DC = Daily Digest with complete contents
      • DS = Daily Digest listing subjects only
      • Post two messages per forum
      • Run the attached CLI script with the --digest argument (this calls the mod_forum component of the moodle cron and should make your life simpler for this testing)
      • Confirm that the following matrix of e-mail was received:

        Single messages

        User Forum 1 Forum 2 Forum 3 Forum 4
        User A 2
        2
        User B 2
        User C
        2
        User D
        2 2

        Digest messages

        User Forum 1 Forum 2 Forum 3 Forum 4
        User A
        2 x DS 2 x DC
        User B
        2 x DC 2 x DS 2 x DC
        User C 2 x DC
        2 x DS 2 x DS
        User D 2 x DS 2 x DC
      • Run the CLI script with*out* the --digest argument
      • Confirm that the following matrix of e-mail was received:

        Single messages

        User Forum 1 Forum 2 Forum 3 Forum 4
        User A 2
        2
        User B 2
        User C
        2
        User D
        2 2

        Digest messages

        User Forum 1 Forum 2 Forum 3 Forum 4
        User A
        User B
        User C
        User D
      • Post two more messages per forum - |
      • Run the CLI script with the --digest argument
      • Confirm that the following matrix of e-mail was received:

        Single messages

        User Forum 1 Forum 2 Forum 3 Forum 4
        User A 2
        2
        User B 2
        User C
        2
        User D
        2 2

        Digest messages

        User Forum 1 Forum 2 Forum 3 Forum 4
        User A
        4 x DS 4 x DC
        User B
        4 x DC 4 x DS 4 x DC
        User C 4 x DC
        4 x DS 4 x DS
        User D 4 x DS 4 x DC

      Backup/Restore test

      • Create a backup of the course including user data
      • Restore the backup into a new course
      • For each user, check that the digest settings were copied over.
      Show
      Test difficulty: Irritating and pernickety Background For this test you will need: a working mail configuration four users enrolled in a courses. Each must have their own address four forums (perhaps create Auto Subscribed?) one editing teacher (can be one of the four users, but may get confusing) and a partridge in a pair tree Setup   Forum 1 Forum 2 Forum 3 Forum 4 Profile digest setting User A Default Digest Subject Digest Complete No Digest No Digest User B No Digest Default Digest Subject Digest Complete Digest Complete User C Digest Complete No Digest Default Digest Subject Digest Subject User D Digest Subject Digest Complete No Digest Default No Digest Note: At present, there aren't many links to /mod/forum/index.php to control the per-forum digest settings. This patch does not seek to address this as I think that we need to consider how this settings page fits into other proposed changes (advanced forums, and allowing unsubscription from forced forums) Therefore you can access the page from /mod/forum/index.php?id= [courseid] . It is linked from forum e-mails, and the activities block. Instructions Note: In these test instructions, the following acronyms are used: DC = Daily Digest with complete contents DS = Daily Digest listing subjects only Post two messages per forum Run the attached CLI script with the --digest argument (this calls the mod_forum component of the moodle cron and should make your life simpler for this testing) Confirm that the following matrix of e-mail was received : Single messages User Forum 1 Forum 2 Forum 3 Forum 4 User A 2 2 User B 2 User C 2 User D 2 2 Digest messages User Forum 1 Forum 2 Forum 3 Forum 4 User A 2 x DS 2 x DC User B 2 x DC 2 x DS 2 x DC User C 2 x DC 2 x DS 2 x DS User D 2 x DS 2 x DC Run the CLI script with*out* the --digest argument Confirm that the following matrix of e-mail was received : Single messages User Forum 1 Forum 2 Forum 3 Forum 4 User A 2 2 User B 2 User C 2 User D 2 2 Digest messages User Forum 1 Forum 2 Forum 3 Forum 4 User A User B User C User D Post two more messages per forum - | Run the CLI script with the --digest argument Confirm that the following matrix of e-mail was received : Single messages User Forum 1 Forum 2 Forum 3 Forum 4 User A 2 2 User B 2 User C 2 User D 2 2 Digest messages User Forum 1 Forum 2 Forum 3 Forum 4 User A 4 x DS 4 x DC User B 4 x DC 4 x DS 4 x DC User C 4 x DC 4 x DS 4 x DS User D 4 x DS 4 x DC Backup/Restore test Create a backup of the course including user data Restore the backup into a new course For each user, check that the digest settings were copied over.
    • Affected Branches:
      MOODLE_16_STABLE, MOODLE_25_STABLE
    • Fixed Branches:
      MOODLE_26_STABLE
    • Pull Master Branch:
    • Rank:
      3176

      Description

      At present, a user can make a choice on whether they receive a daily digest of their forum posts, or one e-mail per post; but this setting is a per-user setting. If a user wishes to keep up with a forum, but is less interested in that forum as others, they cannot change one forum to be a daily digest, and leave their other forums to receive an e-mail per post.

      This feature request seeks to add per-forum digest settings with a fallback to the user's profile setting.

      Original description follows:

      Would it be possible to add an option where your could receive a daily digest of all posts to a particular forum? Currently, you get a digest of all forums for which you are subscribed in an entire course.

      Sample of the conversation raised by this issue:

      http://moodle.org/mod/forum/discuss.php?d=34491

      Request to file this in the bug tracker:

      http://moodle.org/mod/forum/discuss.php?d=41178#190271

      1. 4908.php
        0.8 kB
        Andrew Nicols

        Issue Links

          Activity

          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
          Andrew Nicols added a comment -

          Martin, would you mind if I stole this off you? We're looking at implementing this at the moment.
          Discussion in http://docs.moodle.org/dev/mod_forum-subscriptions

          Show
          Andrew Nicols added a comment - Martin, would you mind if I stole this off you? We're looking at implementing this at the moment. Discussion in http://docs.moodle.org/dev/mod_forum-subscriptions
          Hide
          Andrew Nicols added a comment -

          Need to write up some testing instructions for this - it's not the easiest thing in the world to test I'm afraid.

          Show
          Andrew Nicols added a comment - Need to write up some testing instructions for this - it's not the easiest thing in the world to test I'm afraid.
          Hide
          Andrew Nicols added a comment -

          I'm going to request a peer review early on this as I'd like to get some feedback on the code whilst I finish writing test instructions, and checking everything with a fined-tooth comb.

          Show
          Andrew Nicols added a comment - I'm going to request a peer review early on this as I'd like to get some feedback on the code whilst I finish writing test instructions, and checking everything with a fined-tooth comb.
          Hide
          Jason Fowler added a comment -

          Seems these two issues MDL-4908 and MDL-1626 are talking about changes to the forums that may intersect at some point. Just adding a link between them.

          Show
          Jason Fowler added a comment - Seems these two issues MDL-4908 and MDL-1626 are talking about changes to the forums that may intersect at some point. Just adding a link between them.
          Hide
          Andrew Davis added a comment -

          As previously mentioned this needs testing instructions.

          "$generaltable->align[] = 'center';" is being done twice in mod/forum/index.php

          Very minor but there should be a space either side of => in forum_unsubscribe()
          return $DB->delete_records("forum_digests", array("userid"=>$userid, "forum"=>$forumid));

          phpdocs for forum_set_user_maildigest().
          "@return stdClass The subscription setting now set."
          Can you reword this? Actually is there any point in the return value? It is just the value passed in.

          Add a @throws to the php docs for forum_set_user_maildigest(). http://docs.moodle.org/dev/Coding_style#.40throws

          Instead of @package mod @subpackage forum you just want @package mod_forum. http://docs.moodle.org/dev/Coding_style#.40package

          Am I correct in thinking that the setting on the user profile now acts a default that the user can then modify on a per forum basis?

          Show
          Andrew Davis added a comment - As previously mentioned this needs testing instructions. "$generaltable->align[] = 'center';" is being done twice in mod/forum/index.php Very minor but there should be a space either side of => in forum_unsubscribe() return $DB->delete_records("forum_digests", array("userid"=>$userid, "forum"=>$forumid)); phpdocs for forum_set_user_maildigest(). "@return stdClass The subscription setting now set." Can you reword this? Actually is there any point in the return value? It is just the value passed in. Add a @throws to the php docs for forum_set_user_maildigest(). http://docs.moodle.org/dev/Coding_style#.40throws Instead of @package mod @subpackage forum you just want @package mod_forum. http://docs.moodle.org/dev/Coding_style#.40package Am I correct in thinking that the setting on the user profile now acts a default that the user can then modify on a per forum basis?
          Hide
          Andrew Nicols added a comment -

          Hi Andrew Davis,

          Thanks for the feedback - I'll get on to those changes as soon as possible.

          Yes you are correct, but it's a default which follows the profile setting rather than only being set at creation of a forum. I've added a -1 value which means 'do whatever the profile setting says'. This has the net effect of maintaining existing behaviour for a most users, whilst allowing some (power?) users to override their maildigest setting on a per-forum basis as required. Ultimately, we may wish to reword the forum e-mail footer to add further information informing users that they can do this.

          I would ideally like to add a single view listing all forum subscriptions across Moodle, but I suspect that this will have some potential performance issues, especially on larger sites. I'll be looking at that as part of a separate issue though.

          Cheers,

          Andrew

          Show
          Andrew Nicols added a comment - Hi Andrew Davis , Thanks for the feedback - I'll get on to those changes as soon as possible. Yes you are correct, but it's a default which follows the profile setting rather than only being set at creation of a forum. I've added a -1 value which means 'do whatever the profile setting says'. This has the net effect of maintaining existing behaviour for a most users, whilst allowing some (power?) users to override their maildigest setting on a per-forum basis as required. Ultimately, we may wish to reword the forum e-mail footer to add further information informing users that they can do this. I would ideally like to add a single view listing all forum subscriptions across Moodle, but I suspect that this will have some potential performance issues, especially on larger sites. I'll be looking at that as part of a separate issue though. Cheers, Andrew
          Hide
          Andrew Davis added a comment -

          Sounds good. The main thing is that Moodle acts the same before Vs after getting this code ie the email I receive as a student shouldn't suddenly change because the admin upgraded Moodle.

          >I would ideally like to add a single view listing all forum subscriptions across Moodle

          Historically that kind of thing has been tough primarily because of all the capability checks required. Certainly not impossible however.

          Show
          Andrew Davis added a comment - Sounds good. The main thing is that Moodle acts the same before Vs after getting this code ie the email I receive as a student shouldn't suddenly change because the admin upgraded Moodle. >I would ideally like to add a single view listing all forum subscriptions across Moodle Historically that kind of thing has been tough primarily because of all the capability checks required. Certainly not impossible however.
          Hide
          Andrew Nicols added a comment -

          I can't see where the align is set twice - could you point it out to me perchance?

          Show
          Andrew Nicols added a comment - I can't see where the align is set twice - could you point it out to me perchance?
          Hide
          Andrew Davis added a comment -

          Line 108 and line 111 of mod/forum/index.php

          Show
          Andrew Davis added a comment - Line 108 and line 111 of mod/forum/index.php
          Hide
          Andrew Nicols added a comment -

          They're both needed. It's an array with one align setting per column.
          108 belongs to $strsubscribed
          111 belongs to $stremaildigest . ' ' . $OUTPUT->help_icon('emaildigesttype', 'mod_forum');

          Show
          Andrew Nicols added a comment - They're both needed. It's an array with one align setting per column. 108 belongs to $strsubscribed 111 belongs to $stremaildigest . ' ' . $OUTPUT->help_icon('emaildigesttype', 'mod_forum');
          Hide
          Andrew Nicols added a comment -

          I've force pushed an updated branch addressing all of those issues. I'll have another go at writing some test instructions to cover everything too.

          I agree, the issue with showing all forums will be things like capability checks. I suspect it may be a new function specifically for the purpose.

          Show
          Andrew Nicols added a comment - I've force pushed an updated branch addressing all of those issues. I'll have another go at writing some test instructions to cover everything too. I agree, the issue with showing all forums will be things like capability checks. I suspect it may be a new function specifically for the purpose.
          Hide
          Andrew Nicols added a comment -

          Hi Andrew,

          Would you mind re-peer reviewing this for me? I have:

          • added testing instructions of doom
          • rebased against weekly (trivial)
          • added help text to the use profile to explain things
          • added links to the forum-digest settings in all e-mails
          • corrected a bug where all digest mails were going with the user's default maildigest setting instead

          Enjoyski!

          Show
          Andrew Nicols added a comment - Hi Andrew, Would you mind re-peer reviewing this for me? I have: added testing instructions of doom rebased against weekly (trivial) added help text to the use profile to explain things added links to the forum-digest settings in all e-mails corrected a bug where all digest mails were going with the user's default maildigest setting instead Enjoyski!
          Hide
          Andrew Davis added a comment - - edited
          +$forums = $DB->get_records_sql("
          +    SELECT
          +        f.*,
          +        d.maildigest
          +    FROM {forum} f
          +    LEFT JOIN {forum_digests} d ON d.forum = f.id AND d.userid = ?
          +    WHERE f.course = ?
          +    ", array($USER->id, $course->id)); 
          

          This could do with being reformatted a little. http://docs.moodle.org/dev/SQL_coding_style

          Funny indent on line 1179

              $posttext .= ": {$CFG->wwwroot}/mod/forum/index.php?id={$forum->course}\n"; 
          
          $footerlinks[] = "<a href='{$CFG->wwwroot}/mod/forum/index.php?id={$forum->course}'>" . get_string("digestmailpost", "forum") . '</a>';
          +    $posthtml .= '<hr /><div class="mdl-align unsubscribelink">' . implode('&nbsp;', $footerlinks) . '</div>';
          

          Should this be using the html_writer or similar?

          I'm nit picking here though. This is fine

          Show
          Andrew Davis added a comment - - edited +$forums = $DB->get_records_sql(" + SELECT + f.*, + d.maildigest + FROM {forum} f + LEFT JOIN {forum_digests} d ON d.forum = f.id AND d.userid = ? + WHERE f.course = ? + ", array($USER->id, $course->id)); This could do with being reformatted a little. http://docs.moodle.org/dev/SQL_coding_style Funny indent on line 1179 $posttext .= ": {$CFG->wwwroot}/mod/forum/index.php?id={$forum->course}\n" ; $footerlinks[] = "<a href='{$CFG->wwwroot}/mod/forum/index.php?id={$forum->course}'>" . get_string( "digestmailpost" , "forum" ) . '</a>'; + $posthtml .= '<hr /><div class= "mdl-align unsubscribelink" >' . implode('&nbsp;', $footerlinks) . '</div>'; Should this be using the html_writer or similar? I'm nit picking here though. This is fine
          Hide
          Andrew Nicols added a comment -

          I've made the indentation fixes, but I don't think there's any benefit to moving to use html_writer for just that one part of the e-mail.

          btw, that SQL indentation coding style is INSANE!!

          Show
          Andrew Nicols added a comment - I've made the indentation fixes, but I don't think there's any benefit to moving to use html_writer for just that one part of the e-mail. btw, that SQL indentation coding style is INSANE!!
          Hide
          Sam Hemelryk added a comment -

          Hi Andrew,

          Other than backup and restore this looks just about perfect so far.
          The only thing I noted was ridiculously trivial, there are two spaces in the emaildigest_1 string.

          There were a couple of things that sprung to mind as needing to happen after this change:

          1. We need to be able to control this through some means other than forum/index.php, either through the navigation or perhaps a context based preferences page or something (not the first time the preferences page has come up)
          2. Get rid of the user setting and move it either to a preference or to be somehow stored in the digest table. This would be yet another improvement and again aid us in "unwiring" core and the forum module.

          Many thanks
          Sam

          Show
          Sam Hemelryk added a comment - Hi Andrew, Other than backup and restore this looks just about perfect so far. The only thing I noted was ridiculously trivial, there are two spaces in the emaildigest_1 string. There were a couple of things that sprung to mind as needing to happen after this change: We need to be able to control this through some means other than forum/index.php, either through the navigation or perhaps a context based preferences page or something (not the first time the preferences page has come up) Get rid of the user setting and move it either to a preference or to be somehow stored in the digest table. This would be yet another improvement and again aid us in "unwiring" core and the forum module. Many thanks Sam
          Hide
          Sam Hemelryk added a comment -

          Reopening now

          Show
          Sam Hemelryk added a comment - Reopening now
          Hide
          CiBoT added a comment -

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

          Show
          CiBoT added a comment - Moving this reopened issue out from current integration. Please, re-submit it for integration once ready.
          Hide
          Andrew Nicols added a comment -

          Cheers for that Sam,

          I've corrected the string, and added the missing backup/restore steps.

          Putting back up for integration.

          Show
          Andrew Nicols added a comment - Cheers for that Sam, I've corrected the string, and added the missing backup/restore steps. Putting back up for integration.
          Hide
          Petr Škoda added a comment -

          hi:
          hi, a few notes:

          1/ forum_digests is missing foreign userid key - there should be no need for index if you add key, Eloy would know this best
          2/ if (is_int($forum)) { check is not ok because there might be numbers as strings, we have is_number() function for this in moodle, or use is_object() instead in forum_set_user_maildigest()
          3/ forum_set_user_maildigest() has parameter $user, but it is not used in require_capability('mod/forum:viewdiscussion', $context);
          4/ if ($maildigest === -1) { this would not work if the value came from DB, better use == for numbers
          5/ forum_set_user_maildigest() seems to be using $USER instead $user when inserting to $DB
          6/ mod/forum/maildigest.php() does not require login - that works around all our access control which is very bad
          7/ maybe it would be good to not mix ' " in get_string() calls
          8/ where is '/mod/forum/maildigest_ajax.php' referenced from mod/forum/maildigest.php?
          9/ ksort($digestoptions); after manually constructing array with numeric keys seems funny in forum_get_user_digest_options()
          10/ why $user parameter in forum_get_user_digest_options() ?
          11/ why default 0 for forum and user columns in forum_digests() table?

          Show
          Petr Škoda added a comment - hi: hi, a few notes: 1/ forum_digests is missing foreign userid key - there should be no need for index if you add key, Eloy would know this best 2/ if (is_int($forum)) { check is not ok because there might be numbers as strings, we have is_number() function for this in moodle, or use is_object() instead in forum_set_user_maildigest() 3/ forum_set_user_maildigest() has parameter $user, but it is not used in require_capability('mod/forum:viewdiscussion', $context); 4/ if ($maildigest === -1) { this would not work if the value came from DB, better use == for numbers 5/ forum_set_user_maildigest() seems to be using $USER instead $user when inserting to $DB 6/ mod/forum/maildigest.php() does not require login - that works around all our access control which is very bad 7/ maybe it would be good to not mix ' " in get_string() calls 8/ where is '/mod/forum/maildigest_ajax.php' referenced from mod/forum/maildigest.php? 9/ ksort($digestoptions); after manually constructing array with numeric keys seems funny in forum_get_user_digest_options() 10/ why $user parameter in forum_get_user_digest_options() ? 11/ why default 0 for forum and user columns in forum_digests() table?
          Hide
          Andrew Nicols added a comment -

          Hi Petr,

          Thanks for the additional review.

          1) Have added the FK and removed the index
          2) Thanks - change to use is_number
          3) Corrected to user $user
          4) Good thinking - changed to use non type-strict test
          5) Corrected
          6) I've added a call to require_course_login($course). Thanks for picking up.
          7) I don't see anywhere I'm doing that, but I've been through and tried to change all get_string calls to use ' instead of ". The only exception to this is where I've added calls to existing code (the mail processing primarily) where I've left them in the same style as the surrounding code
          8) Thanks - corrected that. I was trying to make this an ajax request, but our current implementation of the single_select doesn't make this easy at present without rewriting the single_select a bit. Basically, we should change them to use the data- attributes rather than calling a function for each select and modifying them DOM on load each time. It's possible now, but I'm concerned about a course with many forums causing performance woes to the browser
          9) This was the only sensible way I could find of sorting the 'Default' option before the others. Since the default option indicates what the current profile default as part of it's get_string content, we need to know what those existing ones are, and this was the easiest way I foudn of doing so. An alternative would be to create a second array, and them re-merge but this felt cleaner. If you have a better/cleaner idea I'd be all ears
          10) Future proofing - not required for the moment, but we may have a reason to force a users selection as an administrator at some point in the future
          11) IIRC, the XMLDB editor wouldn't let me create this without a default. I've removed them now and all is well with the world.

          Andrew

          Show
          Andrew Nicols added a comment - Hi Petr, Thanks for the additional review. 1) Have added the FK and removed the index 2) Thanks - change to use is_number 3) Corrected to user $user 4) Good thinking - changed to use non type-strict test 5) Corrected 6) I've added a call to require_course_login($course). Thanks for picking up. 7) I don't see anywhere I'm doing that, but I've been through and tried to change all get_string calls to use ' instead of ". The only exception to this is where I've added calls to existing code (the mail processing primarily) where I've left them in the same style as the surrounding code 8) Thanks - corrected that. I was trying to make this an ajax request, but our current implementation of the single_select doesn't make this easy at present without rewriting the single_select a bit. Basically, we should change them to use the data- attributes rather than calling a function for each select and modifying them DOM on load each time. It's possible now, but I'm concerned about a course with many forums causing performance woes to the browser 9) This was the only sensible way I could find of sorting the 'Default' option before the others. Since the default option indicates what the current profile default as part of it's get_string content, we need to know what those existing ones are, and this was the easiest way I foudn of doing so. An alternative would be to create a second array, and them re-merge but this felt cleaner. If you have a better/cleaner idea I'd be all ears 10) Future proofing - not required for the moment, but we may have a reason to force a users selection as an administrator at some point in the future 11) IIRC, the XMLDB editor wouldn't let me create this without a default. I've removed them now and all is well with the world. Andrew
          Hide
          Sam Hemelryk added a comment -

          Thanks Andrew - this has been integrated now

          Show
          Sam Hemelryk added a comment - Thanks Andrew - this has been integrated now
          Hide
          Petr Škoda added a comment -

          6/ require_course_login() is for read actions only, all write actions should use require_login()

          Show
          Petr Škoda added a comment - 6/ require_course_login() is for read actions only, all write actions should use require_login()
          Hide
          Damyon Wiese added a comment -

          Failing this re Petrs comment about require_login().

          Andrew can you provide a commit to update this?

          I'll keep testing anyway as this change is unlikely to affect anything else.

          Show
          Damyon Wiese added a comment - Failing this re Petrs comment about require_login(). Andrew can you provide a commit to update this? I'll keep testing anyway as this change is unlikely to affect anything else.
          Hide
          Damyon Wiese added a comment - - edited

          The test script also does not trigger digest emails even with "--digest"

          Also - the matrices in the test instructions are wrong

          User 2 should receive 2 emails from forum 2 (and does)
          User 3 should receive 2 emails from forum 3 (and does)

          Finally - this took longer to setup and test than it would have to write the unit test. And more sadly, once this issue is integrated - this will not be tested again.

          Some sample code for testing messages sent by cron:

          $sink = $this->redirectMessages();
                  cron_setup_user();
                  $this->expectOutputRegex('/Done processing 1 assignment submissions/');
                  assign::cron();
          
                  $messages = $sink->get_messages();
                  $this->assertEquals(1, count($messages));
          
          

          I am not sure if the lack of digest email is a problem with this patch or a problem with the test script.

          Show
          Damyon Wiese added a comment - - edited The test script also does not trigger digest emails even with "--digest" Also - the matrices in the test instructions are wrong User 2 should receive 2 emails from forum 2 (and does) User 3 should receive 2 emails from forum 3 (and does) Finally - this took longer to setup and test than it would have to write the unit test. And more sadly, once this issue is integrated - this will not be tested again. Some sample code for testing messages sent by cron: $sink = $ this ->redirectMessages(); cron_setup_user(); $ this ->expectOutputRegex('/Done processing 1 assignment submissions/'); assign::cron(); $messages = $sink->get_messages(); $ this ->assertEquals(1, count($messages)); I am not sure if the lack of digest email is a problem with this patch or a problem with the test script.
          Hide
          Andrew Nicols added a comment -

          I've pushed an additional commit to the MDL-4908-m branch with the course_login change.

          Looking into the testing further.

          I did start to try and write the unit tests for this. At present there aren't really any phpunit tests for mod_forum.

          Show
          Andrew Nicols added a comment - I've pushed an additional commit to the MDL-4908 -m branch with the course_login change. Looking into the testing further. I did start to try and write the unit tests for this. At present there aren't really any phpunit tests for mod_forum.
          Hide
          Damyon Wiese added a comment -

          Thanks Andrew - the change to course_login will be enough to pass testing (with updated test instructions) - but if you have the time - the unit tests would be much better.

          Show
          Damyon Wiese added a comment - Thanks Andrew - the change to course_login will be enough to pass testing (with updated test instructions) - but if you have the time - the unit tests would be much better.
          Hide
          Andrew Nicols added a comment -

          I'm currently trying to write the unit tests now - bulk cron mail sending is not the easiest of things to test!

          Show
          Andrew Nicols added a comment - I'm currently trying to write the unit tests now - bulk cron mail sending is not the easiest of things to test!
          Hide
          Andrew Nicols added a comment -

          I've added some additional unit tests now to the same branch. They're blocked by MDL-41196.

          Show
          Andrew Nicols added a comment - I've added some additional unit tests now to the same branch. They're blocked by MDL-41196 .
          Hide
          Andrew Nicols added a comment -

          Oh, and by the way, don't be surprised if you see sporadic testcase failures because of a bug in convert when converting from ascii to utf-8. See MDL-41197 for info.

          Show
          Andrew Nicols added a comment - Oh, and by the way, don't be surprised if you see sporadic testcase failures because of a bug in convert when converting from ascii to utf-8. See MDL-41197 for info.
          Hide
          Damyon Wiese added a comment -

          Retested - (only up to commit 8d75829) - the other unit tests will have to be split into a separate issues because of the blocker.

          Unit tests pass.

          Manual testing passes except:

          User 2 receives digest complete messages from Forum 2. The test is wrong here (will change the instructions).

          This is fine to pass once the commit is pulled into integration.

          Show
          Damyon Wiese added a comment - Retested - (only up to commit 8d75829) - the other unit tests will have to be split into a separate issues because of the blocker. Unit tests pass. Manual testing passes except: User 2 receives digest complete messages from Forum 2. The test is wrong here (will change the instructions). This is fine to pass once the commit is pulled into integration.
          Hide
          Sam Hemelryk added a comment -

          Thanks Damyon - I've integrated those two extra commits now.

          Sending back to testing to be passed

          Show
          Sam Hemelryk added a comment - Thanks Damyon - I've integrated those two extra commits now. Sending back to testing to be passed
          Hide
          Damyon Wiese added a comment -

          Yep - the commits are the same ones tested and the test passes.

          Thanks for working on this Andrew - and thanks for the new unit tests.

          Show
          Damyon Wiese added a comment - Yep - the commits are the same ones tested and the test passes. Thanks for working on this Andrew - and thanks for the new unit tests.
          Hide
          Dan Poltawski added a comment -

          Cảm ơn!

          Your changes have now been merged upstream in git and will be available on the Moodle download sites shortly!

          Một hai ba, yo

          Show
          Dan Poltawski added a comment - Cảm ơn! Your changes have now been merged upstream in git and will be available on the Moodle download sites shortly! Một hai ba, yo
          Hide
          Mary Cooch added a comment -

          Removing docs_required label as this is documented here (#20) http://docs.moodle.org/26/en/Forum_FAQ

          Show
          Mary Cooch added a comment - Removing docs_required label as this is documented here (#20) http://docs.moodle.org/26/en/Forum_FAQ

            People

            • Votes:
              1 Vote for this issue
              Watchers:
              9 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved: