Moodle
  1. Moodle
  2. MDL-37953

replace hard-coded numbers with defined values in mod/forum/lib.php

    Details

    • Type: Improvement Improvement
    • Status: Closed
    • Priority: Minor Minor
    • Resolution: Fixed
    • Affects Version/s: 2.4.1
    • Fix Version/s: 2.5
    • Component/s: Forum
    • Labels:
    • Testing Instructions:
      Hide
      1. Set $CFG->maxeditingtime to 0 in your config.php file.
      2. Set $CFG->forum_enabletimedposts to 1 in your config.php file.
      3. Create a course with two students enrolled.
      4. Create a forum as an admin.
      5. Add a discussion.
      6. Log in as one of the users and write a post to the discussion.
      7. Log in as another user and respond to this post.
      8. Run the cron and ensure an email is sent to the first user telling them about the response to their post.

      Repeat the steps 2-8, except this time set $CFG->forum_enabletimedposts to 0.

      Show
      Set $CFG->maxeditingtime to 0 in your config.php file. Set $CFG->forum_enabletimedposts to 1 in your config.php file. Create a course with two students enrolled. Create a forum as an admin. Add a discussion. Log in as one of the users and write a post to the discussion. Log in as another user and respond to this post. Run the cron and ensure an email is sent to the first user telling them about the response to their post. Repeat the steps 2-8, except this time set $CFG->forum_enabletimedposts to 0.
    • Affected Branches:
      MOODLE_24_STABLE
    • Fixed Branches:
      MOODLE_25_STABLE
    • Pull Master Branch:
      MDL-37953_master
    • Rank:
      47718

      Description

      forum_posts.mailed is set to 0, 1, and 2. These numbers can only be understood if you find the code context where they are set.

      This patch replaces those hard-coded numbers with define()d words: https://github.com/moquist/moodle/commit/0f8fdb3a73d5bd82894c80898c6757f0bd575692

      Since this is really just a change to improve code maintenance and debugging, there's probably no need to apply it to prior versions.

      [EDIT: I updated the commit URL above because I read too fast and misunderstood what 1 and 2 meant on my first time through the code! I contend that this helps emphasize the importance of using words instead of numbers to convey non-numeric meaning...]

        Activity

        Hide
        Mark Nelson added a comment -

        Hi Matt, thanks for your contribution. I am simply going to add another commit on top of yours so that the constants are passed as a second param to get_records and add testing instructions. Thanks again.

        Show
        Mark Nelson added a comment - Hi Matt, thanks for your contribution. I am simply going to add another commit on top of yours so that the constants are passed as a second param to get_records and add testing instructions. Thanks again.
        Hide
        Frédéric Massart added a comment -

        Thanks guys, the patch looks great.

        Can I just ask you Mark to add some more testing instructions for the case of forum_enabletimedposts being enabled (and not) and when there is an error sending the email to the user. Just to confirm that the edited queries will still work.

        Cheers!
        Fred

        Show
        Frédéric Massart added a comment - Thanks guys, the patch looks great. Can I just ask you Mark to add some more testing instructions for the case of forum_enabletimedposts being enabled (and not) and when there is an error sending the email to the user. Just to confirm that the edited queries will still work. Cheers! Fred
        Hide
        Eloy Lafuente (stronk7) added a comment -

        Hey, it looks perfect... just... wouldn't it be better to change NOTYET by PENDING or something like that? Note I'm not an English expert, but "notyet" sounds strange in my mind. Perhaps we are using something similar in other areas? (messaging...)

        I'll keep this open for some hours... FYC, I won't reject it because of that.

        Ciao

        Show
        Eloy Lafuente (stronk7) added a comment - Hey, it looks perfect... just... wouldn't it be better to change NOTYET by PENDING or something like that? Note I'm not an English expert, but "notyet" sounds strange in my mind. Perhaps we are using something similar in other areas? (messaging...) I'll keep this open for some hours... FYC, I won't reject it because of that. Ciao
        Hide
        Matt Oquist added a comment -
        Show
        Matt Oquist added a comment - Heh – as always, Eloy, you are right. https://github.com/moquist/moodle/commit/5776c22c690514394f4827b215e7ec53e0d74d51 --matt
        Hide
        Mark Nelson added a comment -

        Hi Matt, I have made the change in my commit. Your latest commit was conflicting with my most recent changes, so it was easier for me to do a find/replace on my commit and amend. Eloy you can integrate now.

        Show
        Mark Nelson added a comment - Hi Matt, I have made the change in my commit. Your latest commit was conflicting with my most recent changes, so it was easier for me to do a find/replace on my commit and amend. Eloy you can integrate now.
        Hide
        Mark Nelson added a comment -

        I would like to thank Fred for his inspirational commit message, Fred, well done. You have saved me a lot of grief. cries

        Show
        Mark Nelson added a comment - I would like to thank Fred for his inspirational commit message, Fred, well done. You have saved me a lot of grief. cries
        Hide
        Eloy Lafuente (stronk7) added a comment -

        Integrated (master), thanks!

        Show
        Eloy Lafuente (stronk7) added a comment - Integrated (master), thanks!
        Hide
        Rajesh Taneja added a comment -

        Thanks Mark,

        Works as described.

        Show
        Rajesh Taneja added a comment - Thanks Mark, Works as described.
        Hide
        Damyon Wiese added a comment -

        Congratulations this fix has been added to Moodle!

        You may want to dedicate this issue to someone special on this Valentines day.

        Thanks!

        Show
        Damyon Wiese added a comment - Congratulations this fix has been added to Moodle! You may want to dedicate this issue to someone special on this Valentines day. Thanks!

          People

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

            Dates

            • Created:
              Updated:
              Resolved: