Moodle
  1. Moodle
  2. MDL-30377

RSS feed picks up expired (hidden) forum posts

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Critical Critical
    • Resolution: Fixed
    • Affects Version/s: 2.0.1, 2.2.6, 2.3.3
    • Fix Version/s: 2.2.7, 2.3.4
    • Component/s: Forum, RSS
    • Labels:
    • Environment:
      XP
    • Testing Instructions:
      Hide
      1. Visit <yoursite>/admin/settings.php?section=optionalsubsystems and check 'enable RSS feeds'.
      2. Visit <yoursite>/admin/settings.php?section=modsettingforum and set 'Enable RSS feeds' to 'yes' and check 'Timed posts'.
      3. Add a forum activity to a course and set 'RSS feed for this activity' to 'Posts' and 'Number of RSS recent articles' to 10.
      4. Click on the forum activity and create a forum discussion called 'Past' and set it to expire some time in the past.
      5. Create another forum discussion called 'Future' and set it to start some time in the future.
      6. Create a bunch of other forum discussions that do not expire.
      7. Copy the links of the forum discussions that have expired (they may be in the student's browser history so it's possible they know the link)
      8. Log in as student on a separate browser and visit the links you copied, you should get a message saying they can't see them.
      9. Under 'Forum administration' click on 'RSS feed of posts'.
      10. Ensure you can only see the posts that have not expired.
      11. Do this for all the forums, ensure you can not see the 'Past' and 'Future' discussion but the others.
      12. As the administrator turn off the 'Timed posts' setting in Moodle.
      13. As a student ensure you can now see the forum posts you could not previously, also checking that the RSS feeds display correctly.*
      14. Turn it back on, and edit each discussion so that the time is now available and as a student check you can now view them and their corresponding RSS feed.

      *NOTE: The RSS feed gets cached by the browser for a default of 1 hour and in Moodle's cache until the cache is purged. In order for the RSS feed to be updated you need to purge your browser cache AND Moodle's cache since the Moodle cache is only updated if the discussion has been updated with another post.

      Show
      Visit <yoursite>/admin/settings.php?section=optionalsubsystems and check 'enable RSS feeds'. Visit <yoursite>/admin/settings.php?section=modsettingforum and set 'Enable RSS feeds' to 'yes' and check 'Timed posts'. Add a forum activity to a course and set 'RSS feed for this activity' to 'Posts' and 'Number of RSS recent articles' to 10. Click on the forum activity and create a forum discussion called 'Past' and set it to expire some time in the past. Create another forum discussion called 'Future' and set it to start some time in the future. Create a bunch of other forum discussions that do not expire. Copy the links of the forum discussions that have expired (they may be in the student's browser history so it's possible they know the link) Log in as student on a separate browser and visit the links you copied, you should get a message saying they can't see them. Under 'Forum administration' click on 'RSS feed of posts'. Ensure you can only see the posts that have not expired. Do this for all the forums, ensure you can not see the 'Past' and 'Future' discussion but the others. As the administrator turn off the 'Timed posts' setting in Moodle. As a student ensure you can now see the forum posts you could not previously, also checking that the RSS feeds display correctly.* Turn it back on, and edit each discussion so that the time is now available and as a student check you can now view them and their corresponding RSS feed. *NOTE: The RSS feed gets cached by the browser for a default of 1 hour and in Moodle's cache until the cache is purged. In order for the RSS feed to be updated you need to purge your browser cache AND Moodle's cache since the Moodle cache is only updated if the discussion has been updated with another post.
    • Affected Branches:
      MOODLE_20_STABLE, MOODLE_22_STABLE, MOODLE_23_STABLE
    • Fixed Branches:
      MOODLE_22_STABLE, MOODLE_23_STABLE
    • Pull from Repository:
    • Pull Master Branch:
      MDL-30377_master
    • Rank:
      32986

      Description

      The RSS feed is picking up on forum posts that have been set to expire (hidden to non-admin etc) using the timed posts option. When clicking on the RSS link for the expired post you are given an error (unless you have the rights to view hidden posts).

        Issue Links

          Activity

          Hide
          Michael de Raadt added a comment -

          Thanks for reporting this.

          I've put it on our backlog.

          In the meantime adding more information, such as screenshots, a workaround or even a code solution, will help us and other users. If you are able to provide a patch, please add a patch label so we will spot it.

          Show
          Michael de Raadt added a comment - Thanks for reporting this. I've put it on our backlog. In the meantime adding more information, such as screenshots, a workaround or even a code solution, will help us and other users. If you are able to provide a patch, please add a patch label so we will spot it.
          Hide
          Michael de Raadt added a comment -

          Bumping this issue.

          Show
          Michael de Raadt added a comment - Bumping this issue.
          Hide
          Mark Nelson added a comment -

          A note to reviewer and integrator -

          I deprecated the function forum_user_can_view_post in master, and replaced it's functionality in all fixed branches with an existing function. The function was only called once, where as another function forum_user_can_see_post was called everywhere else. It seemed pointless having two functions that achieved the same thing.

          Show
          Mark Nelson added a comment - A note to reviewer and integrator - I deprecated the function forum_user_can_view_post in master, and replaced it's functionality in all fixed branches with an existing function. The function was only called once, where as another function forum_user_can_see_post was called everywhere else. It seemed pointless having two functions that achieved the same thing.
          Hide
          Mark Nelson added a comment -

          Also, I took the group discussion logic from forum_user_can_view_post and put it in the forum_user_can_see_post.

          Show
          Mark Nelson added a comment - Also, I took the group discussion logic from forum_user_can_view_post and put it in the forum_user_can_see_post.
          Hide
          Frédéric Massart added a comment -

          Hi Mark,

          looks good! Few things to mention though:

          • On line mod/forum/lib.php:5332, you are using the deprecated function get_context_instance(), although it might not be deprecated in stable branches.
          • You should not remove strings from the stable branches, even if they're not used any more, that would break older stable branches.
          • I wondered if instead of copying twice the logic about timing and groups, we could use a dedicated function for it. It looks a bit weird to pollute the system with two more functions, but at the same time maintaining the same code at 2 different place could probably lead to confusions/regressions.

          Thanks!

          Show
          Frédéric Massart added a comment - Hi Mark, looks good! Few things to mention though: On line mod/forum/lib.php:5332, you are using the deprecated function get_context_instance(), although it might not be deprecated in stable branches. You should not remove strings from the stable branches, even if they're not used any more, that would break older stable branches. I wondered if instead of copying twice the logic about timing and groups, we could use a dedicated function for it. It looks a bit weird to pollute the system with two more functions, but at the same time maintaining the same code at 2 different place could probably lead to confusions/regressions. Thanks!
          Hide
          Mark Nelson added a comment -

          Hi Fred, thanks for that. I have done all those changes, let me know what you think.

          Show
          Mark Nelson added a comment - Hi Fred, thanks for that. I have done all those changes, let me know what you think.
          Hide
          Frédéric Massart added a comment -

          Looks good to me Mark. Pushing for integration. Thanks!

          Show
          Frédéric Massart added a comment - Looks good to me Mark. Pushing for integration. Thanks!
          Hide
          Dan Poltawski added a comment -

          Hi Mark,

          This looks great, there are just a few small things to fixup:

          1. It looks to me like the check on dates with $forum->type == 'news' in mod/forum/discuss.php happens irrespective of $CFG->forum_enabletimedposts, so you are changing the logic there to prevent timed access (I think).
          2. NULL should be null http://docs.moodle.org/dev/Coding_style#Booleans_and_the_null_value
          3. Because i'm nervous of all forum changes, i'd quite like to see a bit more testing scenarios beefed up on this. There is a lot of chance for regressions. So it'd be good to also cover normal forum discusission viewing with all the different forum types.
          Show
          Dan Poltawski added a comment - Hi Mark, This looks great, there are just a few small things to fixup: It looks to me like the check on dates with $forum->type == 'news' in mod/forum/discuss.php happens irrespective of $CFG->forum_enabletimedposts , so you are changing the logic there to prevent timed access (I think). NULL should be null http://docs.moodle.org/dev/Coding_style#Booleans_and_the_null_value Because i'm nervous of all forum changes, i'd quite like to see a bit more testing scenarios beefed up on this. There is a lot of chance for regressions. So it'd be good to also cover normal forum discusission viewing with all the different forum types.
          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
          Mark Nelson added a comment - - edited

          Hi Dan,

          1) You can't actually set up a forum with time restrictions without forum_enabledtimedposts, so I got rid of this check as it seemed pointless performing it twice. There is a lot of code in the forum module that repeats checks etc so I wasn't surprised by this.
          2) Done. I even did this to the function I copied in the deprecated lib - is this correct?
          3) Will add more testing instructions once I come up with some sensible steps rather than just steps all over the place testing the forum.

          Cheers.

          Show
          Mark Nelson added a comment - - edited Hi Dan, 1) You can't actually set up a forum with time restrictions without forum_enabledtimedposts, so I got rid of this check as it seemed pointless performing it twice. There is a lot of code in the forum module that repeats checks etc so I wasn't surprised by this. 2) Done. I even did this to the function I copied in the deprecated lib - is this correct? 3) Will add more testing instructions once I come up with some sensible steps rather than just steps all over the place testing the forum. Cheers.
          Hide
          Dan Poltawski added a comment -

          Hi Mark,

          Thanks for clarifying those.

          Sorry, missed on the first round. that.. you've put forum_user_can_view_post() into lib/depricatedlib.php, thats not correct as this is a module, despite coming with moodle its not 'moodle core' (same way that db tables go into mod/forum/db/install.xml). So it should stay in mod/forum/lib.php.

          I'll leave this open so you fix that one up.

          Show
          Dan Poltawski added a comment - Hi Mark, Thanks for clarifying those. Sorry, missed on the first round. that.. you've put forum_user_can_view_post() into lib/depricatedlib.php, thats not correct as this is a module, despite coming with moodle its not 'moodle core' (same way that db tables go into mod/forum/db/install.xml). So it should stay in mod/forum/lib.php. I'll leave this open so you fix that one up.
          Hide
          Mark Nelson added a comment -

          Hi Dan, I have made the changes you suggested. Thanks.

          Show
          Mark Nelson added a comment - Hi Dan, I have made the changes you suggested. Thanks.
          Hide
          Dan Poltawski added a comment -

          Integrated to 22, 23 and master. Thanks Mark.

          Show
          Dan Poltawski added a comment - Integrated to 22, 23 and master. Thanks Mark.
          Hide
          Rossiani Wijaya added a comment -

          Removing testing instruction for group discussion as it will be fix through MDL-36741.

          Show
          Rossiani Wijaya added a comment - Removing testing instruction for group discussion as it will be fix through MDL-36741 .
          Hide
          Rossiani Wijaya added a comment -

          Hi Mark,

          This is working as expected.

          Thanks for fixing this.

          Test passed.

          Show
          Rossiani Wijaya added a comment - Hi Mark, This is working as expected. Thanks for fixing this. Test passed.
          Hide
          Dan Poltawski added a comment -

          Congratulations! Another bug solved.. only another 7330 to go, thanks for contributing to contributing to 0.8% of all bugs being fixed this week!

          ciao
          Dan

          Show
          Dan Poltawski added a comment - Congratulations! Another bug solved.. only another 7330 to go, thanks for contributing to contributing to 0.8% of all bugs being fixed this week! ciao Dan

            People

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

              Dates

              • Created:
                Updated:
                Resolved: