Moodle
  1. Moodle
  2. MDL-28981

Forum's forum_make_mail_html() doesn't call forum_user_can_post() properly

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Trivial Trivial
    • Resolution: Fixed
    • Affects Version/s: 2.0.4, 2.1.1, 2.2
    • Fix Version/s: 2.0.5, 2.1.2
    • Component/s: Forum
    • Labels:
    • Testing Instructions:
      Hide

      1/ create a custom script that does calls forum_make_mail_html() with and without the extra parameters
      2/ verify the result is the same

      Show
      1/ create a custom script that does calls forum_make_mail_html() with and without the extra parameters 2/ verify the result is the same
    • Difficulty:
      Easy
    • Affected Branches:
      MOODLE_20_STABLE, MOODLE_21_STABLE, MOODLE_22_STABLE
    • Fixed Branches:
      MOODLE_20_STABLE, MOODLE_21_STABLE
    • Pull from Repository:
    • Pull Master Branch:
      w33_MDL-28981_m22_canpost
    • Rank:
      18671

      Description

      I was doing some testing with messaging and when I called forum_make_mail_html() then forum_user_can_post() complained about the $cm and $course not being passed (debug messages). Easy fix, just pass both of those from forum_make_mail_html(), should also help with performance (big win here since these are called on the cron).

        Activity

        Hide
        Martin Dougiamas added a comment -

        Thanks Mark, good catch! Easy one for stable team.

        Show
        Martin Dougiamas added a comment - Thanks Mark, good catch! Easy one for stable team.
        Hide
        Petr Škoda added a comment -

        My fault, fixed.

        To integrators? please cherry pick to all 2.x branches.

        Show
        Petr Škoda added a comment - My fault, fixed. To integrators? please cherry pick to all 2.x branches.
        Hide
        Eloy Lafuente (stronk7) added a comment -

        Integrated, thanks! (backported to 20 and 21 stable)

        Show
        Eloy Lafuente (stronk7) added a comment - Integrated, thanks! (backported to 20 and 21 stable)
        Hide
        Andrew Davis added a comment - - edited

        The point of testing is to check that you didn't unknowingly introduce a problem. Thus the bug fixer's opinion as to whether or not testing is required is irrelevant. Add some testing instructions describing how to exercise the code area you've changed and verify that it is still functional even if you can't see how you could have broken it.

        Show
        Andrew Davis added a comment - - edited The point of testing is to check that you didn't unknowingly introduce a problem. Thus the bug fixer's opinion as to whether or not testing is required is irrelevant. Add some testing instructions describing how to exercise the code area you've changed and verify that it is still functional even if you can't see how you could have broken it.
        Hide
        Aparup Banerjee added a comment -

        I think a test script that tests the code calls (not really meant as unit test) would be a handy test here.

        Show
        Aparup Banerjee added a comment - I think a test script that tests the code calls (not really meant as unit test) would be a handy test here.
        Hide
        Andrew Davis added a comment -

        Not really. This isnt white box testing. If you're going to write test scripts do so as part of development (and possibly turn them into unit tests).

        Petr, all thats required is that you spend a few minutes writing out something like this so that a non-programmer can at least verify that the functionality involved still works. All it takes is something like this:

        1) create a forum if you dont already have one. subscribe. check your user profile and make sure that email format is set to "pretty html format"
        2) check the user's messaging preferences and make sure that they are set up to receive forum post notification via email, both online and offline.
        3) as a second user post in the forum. make a note of what you posted.
        4) wait at least "maxeditingtime" (its a system setting) then run /admin/cron.php
        5) check that the second user receives the html email and that the forum post contains the correct content.

        Show
        Andrew Davis added a comment - Not really. This isnt white box testing. If you're going to write test scripts do so as part of development (and possibly turn them into unit tests). Petr, all thats required is that you spend a few minutes writing out something like this so that a non-programmer can at least verify that the functionality involved still works. All it takes is something like this: 1) create a forum if you dont already have one. subscribe. check your user profile and make sure that email format is set to "pretty html format" 2) check the user's messaging preferences and make sure that they are set up to receive forum post notification via email, both online and offline. 3) as a second user post in the forum. make a note of what you posted. 4) wait at least "maxeditingtime" (its a system setting) then run /admin/cron.php 5) check that the second user receives the html email and that the forum post contains the correct content.
        Hide
        Petr Škoda added a comment -

        You are missing one fact - we are talking about a dead piece of code, nobody should be calling the forum_make_mail_html() except the forum cron which already caches the data, this issue is not for non-programmers.

        In this particular issue no big testing is imo necessary, sorry. In other issues I agree we should write better instructions, on the other hand if you write too much detailed instructions testers will not think for themselves and just do what they are told to do which is very bad. They should think a bit different than the person who implemented the solution. Two people doing the same testing does not make much sense to me. Ideally there should be a unit test and a manual testing instructions that actually verify the unit tests make sense. Anyway the tester has to be able to read the code if necessary and come up with alternative testing procedure...

        Show
        Petr Škoda added a comment - You are missing one fact - we are talking about a dead piece of code, nobody should be calling the forum_make_mail_html() except the forum cron which already caches the data, this issue is not for non-programmers. In this particular issue no big testing is imo necessary, sorry. In other issues I agree we should write better instructions, on the other hand if you write too much detailed instructions testers will not think for themselves and just do what they are told to do which is very bad. They should think a bit different than the person who implemented the solution. Two people doing the same testing does not make much sense to me. Ideally there should be a unit test and a manual testing instructions that actually verify the unit tests make sense. Anyway the tester has to be able to read the code if necessary and come up with alternative testing procedure...
        Hide
        Eloy Lafuente (stronk7) added a comment -

        Looking for all the uses across codebase revealed all them pass always both the course and coursemodule (with the context being optional and calculated within the funciton).

        The create mini-script calling forum_user_can_post() with and without the 2 params. Result was the same, but the one missing the params sent the debugging messages and had to perform extra DB queries.

        So passing, that will keep the debug out & save some precious time and work to the function.

        Show
        Eloy Lafuente (stronk7) added a comment - Looking for all the uses across codebase revealed all them pass always both the course and coursemodule (with the context being optional and calculated within the funciton). The create mini-script calling forum_user_can_post() with and without the 2 params. Result was the same, but the one missing the params sent the debugging messages and had to perform extra DB queries. So passing, that will keep the debug out & save some precious time and work to the function.
        Hide
        Eloy Lafuente (stronk7) added a comment -

        Many thanks for the hard work, this has been sent upstream and is available in all the git and cvs repositories.

        Show
        Eloy Lafuente (stronk7) added a comment - Many thanks for the hard work, this has been sent upstream and is available in all the git and cvs repositories.

          People

          • Votes:
            0 Vote for this issue
            Watchers:
            0 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved: