Uploaded image for project: 'Moodle'
  1. Moodle
  2. MDL-17955

Timed posts should not be marked as experimental

    Details

    • Type: Improvement
    • Status: Closed
    • Priority: Minor
    • Resolution: Fixed
    • Affects Version/s: 1.8.10, 1.9.6, 2.0, 2.8, 3.1
    • Fix Version/s: 3.1
    • Component/s: Forum
    • Labels:
    • Environment:
      en
    • Database:
      Any
    • Testing Instructions:
      Hide
      1. Create a new site
        1. Confirm that timed posts are enabled by default
      2. Take an upgraded site
        1. Confirm that timed posts were not enabled

      Note: we're talking about the admin setting 'forum_enabletimedposts'.

      Show
      Create a new site Confirm that timed posts are enabled by default Take an upgraded site Confirm that timed posts were not enabled Note: we're talking about the admin setting 'forum_enabletimedposts' .
    • Affected Branches:
      MOODLE_18_STABLE, MOODLE_19_STABLE, MOODLE_20_STABLE, MOODLE_28_STABLE, MOODLE_31_STABLE
    • Fixed Branches:
      MOODLE_31_STABLE
    • Pull Master Branch:
      MDL-17955-master

      Description

      Setting to enable in admin console suggests they are. Needs updating if all OK with this feature.

        Gliffy Diagrams

          Issue Links

            Activity

            ray Ray Lawrence created issue -
            ray Ray Lawrence made changes -
            Field Original Value New Value
            Link This issue has been marked as being related by MDL-17956 [ MDL-17956 ]
            stronk7 Eloy Lafuente (stronk7) made changes -
            Assignee moodle.com [ moodle.com ] Petr ?koda [ skodak ]
            Hide
            stronk7 Eloy Lafuente (stronk7) added a comment -

            Assigning to Petr... do you know the status of this?

            Show
            stronk7 Eloy Lafuente (stronk7) added a comment - Assigning to Petr... do you know the status of this?
            Hide
            skodak Petr Skoda added a comment -

            They are still experimental, if I remember it correctly there are still some issues left to be solved...

            Show
            skodak Petr Skoda added a comment - They are still experimental, if I remember it correctly there are still some issues left to be solved...
            Hide
            ray Ray Lawrence added a comment -

            I tried this out extensively recently all seemed well: visibility was correct based on the relevant capability inc. for groups. Emails were sent at appropriate times.

            There was no duration (period) option as noted on MDL-17955

            Show
            ray Ray Lawrence added a comment - I tried this out extensively recently all seemed well: visibility was correct based on the relevant capability inc. for groups. Emails were sent at appropriate times. There was no duration (period) option as noted on MDL-17955
            Hide
            srmi Darrel Tenter added a comment -

            We've just started using this recently and have noticed a bug. The ability to view a post that has a future display start date is limited to the person that made the post.

            If I as admin make a post with a future availability date, an instructor can see the post listed on the forum page. However if they try to view it they get "Discussion ID was incorrect or no longer exists" as if the post had been removed. Likewise if an instructor makes a similar post I as admin get the same error when trying to view it.

            Permissions for both Roles are set to Allow for viewing hidden timed posts.

            Show
            srmi Darrel Tenter added a comment - We've just started using this recently and have noticed a bug. The ability to view a post that has a future display start date is limited to the person that made the post. If I as admin make a post with a future availability date, an instructor can see the post listed on the forum page. However if they try to view it they get "Discussion ID was incorrect or no longer exists" as if the post had been removed. Likewise if an instructor makes a similar post I as admin get the same error when trying to view it. Permissions for both Roles are set to Allow for viewing hidden timed posts.
            Hide
            ray Ray Lawrence added a comment -

            Darrel,

            Is the behaviour the same if the poster and viewer both have "normal" course only role assignments?

            Show
            ray Ray Lawrence added a comment - Darrel, Is the behaviour the same if the poster and viewer both have "normal" course only role assignments?
            Hide
            ecastro Enrique Castro added a comment - - edited

            At ULPGC have enabled timed post for the full academic year. The feature was well received by teachers and used in some courses.

            We have found only three "bugs" or inconvenient behaviours that require changes in code

            a) No indication of the time-restriction on the discussion once published
            This is detailed in issue MDL-9070
            Once the post is saved the author can see it, but not the students (if out of time interval), but there is absolutely NO indication of that fact. I propose to print "invisible/hidden" discussions as DIMMED, like activities in course page.
            This would imply to modify function forum_print_discussion_header() in forum/lib.php to check timestart and timeend properties and set DIMMED class.

            b) A mechanism to change dates or REMOVE time limitation AFTER the publication of the post.
            Nos if the teacher erroneously save a post with time limitation cannot make it visible for all afterwards. Teh only way is to modify DB directly. A post in a news forum can be deleted and re-posted without limitation. But this is NOT possible for a whole discussion that was started by a time-restricted post

            c) Error "Discussion ID was incorrect or no longer exists" in NEWS forums
            Due to a) we have experienced the same confusing bug seen by Darrel Tenter
            In a NEWS forum an people with permissions (teachers and admins) can see ALL discussions, but if you try to view a discusion you have not authored you are thrown that error "Discussion ID was incorrect or no longer exists"

            I have traced the bug to some lines at the start of discussion.php script.
            There, by line 45, you can find

                if ($forum->type == 'news') {
                    if (!($USER->id == $discussion->userid || (($discussion->timestart == 0
                        || $discussion->timestart <= time())
                        && ($discussion->timeend == 0 || $discussion->timeend > time())))) {
                        print_error('invaliddiscussionid', 'forum', "$CFG->wwwroot/mod/forum/view.php?f=$forum->id");
                    }
                }

            (similar in 1.9 version with message "Discussion ID was incorrect or no longer exists" hardcoded)

            This is a limitation specific for NEWS forum that is quite nonsense. Students CANNOT see the hidden discussions, those are not printed in the list. This condition is affecting to teachers and admins, both with legitimate rights to access to those time-limited discussions.
            In my opinon, either the condition is eliminated completely or the regular checks for applicable permissions are added.

            Show
            ecastro Enrique Castro added a comment - - edited At ULPGC have enabled timed post for the full academic year. The feature was well received by teachers and used in some courses. We have found only three "bugs" or inconvenient behaviours that require changes in code a) No indication of the time-restriction on the discussion once published This is detailed in issue MDL-9070 Once the post is saved the author can see it, but not the students (if out of time interval), but there is absolutely NO indication of that fact. I propose to print "invisible/hidden" discussions as DIMMED, like activities in course page. This would imply to modify function forum_print_discussion_header() in forum/lib.php to check timestart and timeend properties and set DIMMED class. b) A mechanism to change dates or REMOVE time limitation AFTER the publication of the post. Nos if the teacher erroneously save a post with time limitation cannot make it visible for all afterwards. Teh only way is to modify DB directly. A post in a news forum can be deleted and re-posted without limitation. But this is NOT possible for a whole discussion that was started by a time-restricted post c) Error "Discussion ID was incorrect or no longer exists" in NEWS forums Due to a) we have experienced the same confusing bug seen by Darrel Tenter In a NEWS forum an people with permissions (teachers and admins) can see ALL discussions, but if you try to view a discusion you have not authored you are thrown that error "Discussion ID was incorrect or no longer exists" I have traced the bug to some lines at the start of discussion.php script. There, by line 45, you can find if ($forum->type == 'news') { if (!($USER->id == $discussion->userid || (($discussion->timestart == 0 || $discussion->timestart <= time()) && ($discussion->timeend == 0 || $discussion->timeend > time())))) { print_error('invaliddiscussionid', 'forum', "$CFG->wwwroot/mod/forum/view.php?f=$forum->id"); } } (similar in 1.9 version with message "Discussion ID was incorrect or no longer exists" hardcoded) This is a limitation specific for NEWS forum that is quite nonsense. Students CANNOT see the hidden discussions, those are not printed in the list. This condition is affecting to teachers and admins, both with legitimate rights to access to those time-limited discussions. In my opinon, either the condition is eliminated completely or the regular checks for applicable permissions are added.
            ecastro Enrique Castro made changes -
            Affects Version/s 1.8.10 [ 10350 ]
            Affects Version/s 1.9.6 [ 10340 ]
            Affects Version/s 2.0 [ 10122 ]
            Affects Version/s 1.9.3 [ 10290 ]
            ecastro Enrique Castro made changes -
            Link This issue blocks MDL-9070 [ MDL-9070 ]
            ecastro Enrique Castro made changes -
            Link This issue blocks MDL-9070 [ MDL-9070 ]
            ecastro Enrique Castro made changes -
            Link This issue has been marked as being related by MDL-9070 [ MDL-9070 ]
            ecastro Enrique Castro made changes -
            Comment [ Display invisible discussions as DIMMED text for teachers and admins ]
            ecastro Enrique Castro made changes -
            Comment [ Display "invisible" discussion header as dimmed text ]
            Hide
            skodak Petr Skoda added a comment -

            reassigning to HQ

            Show
            skodak Petr Skoda added a comment - reassigning to HQ
            skodak Petr Skoda made changes -
            Assignee Petr ?koda (skodak) [ skodak ] moodle.com [ moodle.com ]
            dougiamas Martin Dougiamas made changes -
            Workflow jira [ 30322 ] MDL Workflow [ 44380 ]
            dougiamas Martin Dougiamas made changes -
            Workflow MDL Workflow [ 44380 ] MDL Full Workflow [ 72756 ]
            Hide
            amanda.doughty Amanda Doughty added a comment - - edited

            I have tested this in 2.2 and found that if display is set to more than than two days in advance then email notifications are never sent. This is by design as posts with a creation date longer than two days ago are marked as sent. See forum/lip.php line 2119:

             
            /**
             * Marks posts before a certain time as being mailed already
             *
             * @global object
             * @global object
             * @param int $endtime
             * @param int $now Defaults to time()
             * @return bool
             */
            function forum_mark_old_posts_as_mailed($endtime, $now=null) {
                global $CFG, $DB;
                if (empty($now)) {
                    $now = time();
                }
             
                if (empty($CFG->forum_enabletimedposts)) {
                    return $DB->execute("UPDATE {forum_posts}
                                           SET mailed = '1'
                                         WHERE (created < ? OR mailnow = 1)
                                               AND mailed = 0", array($endtime));
             
                } else {
                    return $DB->execute("UPDATE {forum_posts}
                                           SET mailed = '1'
                                         WHERE discussion NOT IN (SELECT d.id
                                                                    FROM {forum_discussions} d
                                                                   WHERE d.timestart > ?)
                                               AND (created < ? OR mailnow = 1)
                                               AND mailed = 0", array($now, $endtime));
                }
            }

            Show
            amanda.doughty Amanda Doughty added a comment - - edited I have tested this in 2.2 and found that if display is set to more than than two days in advance then email notifications are never sent. This is by design as posts with a creation date longer than two days ago are marked as sent. See forum/lip.php line 2119: /** * Marks posts before a certain time as being mailed already * * @global object * @global object * @param int $endtime * @param int $now Defaults to time() * @return bool */ function forum_mark_old_posts_as_mailed($endtime, $now=null) { global $CFG, $DB; if (empty($now)) { $now = time(); }   if (empty($CFG->forum_enabletimedposts)) { return $DB->execute("UPDATE {forum_posts} SET mailed = '1' WHERE (created < ? OR mailnow = 1) AND mailed = 0", array($endtime));   } else { return $DB->execute("UPDATE {forum_posts} SET mailed = '1' WHERE discussion NOT IN (SELECT d.id FROM {forum_discussions} d WHERE d.timestart > ?) AND (created < ? OR mailnow = 1) AND mailed = 0", array($now, $endtime)); } }
            Hide
            hrynkiw Donna Hrynkiw added a comment -

            We've just turned on this feature for our faculty. Our testing teacher requests that the original posting date* be replaced with the "Display from" date – as if the teacher had just actually posted.

            * as displayed to the students and is included in e-mail if they are subscribed to the forum.

            Show
            hrynkiw Donna Hrynkiw added a comment - We've just turned on this feature for our faculty. Our testing teacher requests that the original posting date* be replaced with the "Display from" date – as if the teacher had just actually posted. * as displayed to the students and is included in e-mail if they are subscribed to the forum.
            dobedobedoh Andrew Nicols made changes -
            Epic Link MDL-48287 [ 82187 ]
            Hide
            dobedobedoh Andrew Nicols added a comment -

            This will also need to be changed to a setting on a per-forum basis and the associated upgrade handled with it.

            Show
            dobedobedoh Andrew Nicols added a comment - This will also need to be changed to a setting on a per-forum basis and the associated upgrade handled with it.
            dobedobedoh Andrew Nicols made changes -
            Status Open [ 1 ] Open [ 1 ]
            Summary Forum timed posts - still experimental? Remove experimental flag from forum settings
            Affects Version/s 2.8 [ 13150 ]
            Labels triaged
            Assignee moodle.com [ moodle.com ]
            Component/s Administration [ 10050 ]
            Component/s Language [ 10089 ]
            Hide
            hitteshahuja Hittesh added a comment - - edited

            I cant seem to replicate error (c) or (b) listed by Enrique Castro:
            b) A mechanism to change dates or REMOVE time limitation AFTER the publication of the post.

            • Dates can be changes or disabled easily using the tick boxes

            c) Error "Discussion ID was incorrect or no longer exists" in NEWS forums

            • When Teacher 'A' posts a timed discussion under the default News forum, teacher 'B' can look at it absolutely fine without any errors.
            Show
            hitteshahuja Hittesh added a comment - - edited I cant seem to replicate error (c) or (b) listed by Enrique Castro : b) A mechanism to change dates or REMOVE time limitation AFTER the publication of the post. Dates can be changes or disabled easily using the tick boxes c) Error "Discussion ID was incorrect or no longer exists" in NEWS forums When Teacher 'A' posts a timed discussion under the default News forum, teacher 'B' can look at it absolutely fine without any errors.
            Hide
            dobedobedoh Andrew Nicols added a comment -

            Thanks all,

            I think that all of the blockers to this have been integrated and are available in 3.1.

            I'm going to propose that we go ahead with this issue soon.

            As mentioned before, this probably needs to become a per-forum setting when it is removed from experimental, but perhaps others disagree?

            Show
            dobedobedoh Andrew Nicols added a comment - Thanks all, I think that all of the blockers to this have been integrated and are available in 3.1. I'm going to propose that we go ahead with this issue soon. As mentioned before, this probably needs to become a per-forum setting when it is removed from experimental, but perhaps others disagree?
            dobedobedoh Andrew Nicols made changes -
            Status Open [ 1 ] Open [ 1 ]
            Affects Version/s 3.1 [ 15060 ]
            Sprint candidate Now [ 10240 ]
            dobedobedoh Andrew Nicols made changes -
            dobedobedoh Andrew Nicols made changes -
            Assignee Andrew Nicols [ dobedobedoh ]
            Hide
            dobedobedoh Andrew Nicols added a comment -

            I'm going to put this issue up for review now as all blockers have been completed.

            At this time I'm simply going to remove the experimental flag from the language string, and change the default to be enabled. I do not think that we should change the value for existing sites from disabled to enabled.

            After further thought I don't think that there's any benefit to making this a per-forum setting. The functionality is only available to teachers (not students).

            Show
            dobedobedoh Andrew Nicols added a comment - I'm going to put this issue up for review now as all blockers have been completed. At this time I'm simply going to remove the experimental flag from the language string, and change the default to be enabled. I do not think that we should change the value for existing sites from disabled to enabled. After further thought I don't think that there's any benefit to making this a per-forum setting. The functionality is only available to teachers (not students).
            dobedobedoh Andrew Nicols made changes -
            Status Open [ 1 ] Waiting for peer review [ 10012 ]
            Testing Instructions # Create a new site
            ## *Confirm that timed posts are enabled by default*
            cibot CiBoT made changes -
            Labels triaged ci triaged
            Hide
            cibot CiBoT added a comment -

            Code verified against automated checks.

            Checked MDL-17955 using repository: git://github.com/andrewnicols/moodle.git

            More information about this report

            Show
            cibot CiBoT added a comment - Code verified against automated checks. Checked MDL-17955 using repository: git://github.com/andrewnicols/moodle.git master (0 errors / 0 warnings) [branch: MDL-17955-master | CI Job ] More information about this report
            lameze Simey Lameze made changes -
            Remaining Estimate 0 minutes [ 0 ]
            Status Waiting for peer review [ 10012 ] Peer review in progress [ 10013 ]
            Peer reviewer Simey Lameze [ lameze ]
            Hide
            lameze Simey Lameze added a comment -

            Thanks for work on this Andrew, as you've said all issues on the epic were done. So I don't see why we should keep that setting as experimental.

            Feel free to push for integration review.

            Thanks.

            Show
            lameze Simey Lameze added a comment - Thanks for work on this Andrew, as you've said all issues on the epic were done. So I don't see why we should keep that setting as experimental. Feel free to push for integration review. Thanks.
            lameze Simey Lameze made changes -
            Status Peer review in progress [ 10013 ] Development in progress [ 3 ]
            Hide
            dobedobedoh Andrew Nicols added a comment -

            Thanks Simey,

            Pushing for Integration.

            Show
            dobedobedoh Andrew Nicols added a comment - Thanks Simey, Pushing for Integration.
            dobedobedoh Andrew Nicols made changes -
            Status Development in progress [ 3 ] Waiting for integration review [ 10010 ]
            Testing Instructions # Create a new site
            ## *Confirm that timed posts are enabled by default*
            # Create a new site
            ## *Confirm that timed posts are enabled by default*
            # Take an upgraded site
            ## *Confirm that timed posts were not enabled*
            cibot CiBoT made changes -
            Status Waiting for integration review [ 10010 ] Waiting for integration review [ 10010 ]
            Integration priority 0
            Original Estimate 0 minutes [ 0 ]
            Hide
            poltawski Dan Poltawski added a comment -

            The main moodle.git repository has just been updated with latest weekly modifications. You may wish to rebase your PULL branches to simplify history and avoid any possible merge conflicts. This would also make integrator's life easier next week.
            TIA and ciao

            Show
            poltawski Dan Poltawski added a comment - The main moodle.git repository has just been updated with latest weekly modifications. You may wish to rebase your PULL branches to simplify history and avoid any possible merge conflicts. This would also make integrator's life easier next week. TIA and ciao
            Hide
            cibot CiBoT added a comment -

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

            Show
            cibot CiBoT added a comment - Moving this issue to current integration cycle, will be reviewed soon. Thanks for the hard work!
            cibot CiBoT made changes -
            Status Waiting for integration review [ 10010 ] Waiting for integration review [ 10010 ]
            Currently in integration Yes [ 10041 ]
            cibot CiBoT made changes -
            Labels ci triaged triaged
            cibot CiBoT made changes -
            Labels triaged ci triaged
            Hide
            cibot CiBoT added a comment -

            Code verified against automated checks.

            Checked MDL-17955 using repository: git://github.com/andrewnicols/moodle.git

            More information about this report

            Show
            cibot CiBoT added a comment - Code verified against automated checks. Checked MDL-17955 using repository: git://github.com/andrewnicols/moodle.git master (0 errors / 0 warnings) [branch: MDL-17955-master | CI Job ] More information about this report
            poltawski Dan Poltawski made changes -
            Status Waiting for integration review [ 10010 ] Integration review in progress [ 10004 ]
            Integrator Dan Poltawski [ poltawski ]
            Hide
            poltawski Dan Poltawski added a comment -

            Thanks Andrew, integrated to master

            Show
            poltawski Dan Poltawski added a comment - Thanks Andrew, integrated to master
            poltawski Dan Poltawski made changes -
            Status Integration review in progress [ 10004 ] Waiting for testing [ 10005 ]
            Fix Version/s 3.1 [ 15060 ]
            rajeshtaneja Rajesh Taneja made changes -
            Tester Frédéric Massart [ fred ]
            fred Frédéric Massart made changes -
            Status Waiting for testing [ 10005 ] Testing in progress [ 10011 ]
            fred Frédéric Massart made changes -
            Testing Instructions # Create a new site
            ## *Confirm that timed posts are enabled by default*
            # Take an upgraded site
            ## *Confirm that timed posts were not enabled*
            # Create a new site
            ## *Confirm that timed posts are enabled by default*
            # Take an upgraded site
            ## *Confirm that timed posts were not enabled*

            _Note: we're talking about the admin setting 'forum_enabletimedposts'_.
            Hide
            fred Frédéric Massart added a comment -

            Passing.

            Note, it's not related to this issue but I find it hard to get the meaning of the setting.

            Show
            fred Frédéric Massart added a comment - Passing. Note, it's not related to this issue but I find it hard to get the meaning of the setting.
            fred Frédéric Massart made changes -
            Status Testing in progress [ 10011 ] Tested [ 10006 ]
            Hide
            poltawski Dan Poltawski added a comment -

            Thanks for your contributions! This change is now available from the main moodle.git repository and will shortly be available on download.moodle.org.

            Are you quite sure that all those bells and whistles, all those wonderful facilities of your so called powerful programming languages, belong to the solution set rather than the problem set?
            – Edsger W. Dijkstra

            Show
            poltawski Dan Poltawski added a comment - Thanks for your contributions! This change is now available from the main moodle.git repository and will shortly be available on download.moodle.org. Are you quite sure that all those bells and whistles, all those wonderful facilities of your so called powerful programming languages, belong to the solution set rather than the problem set? – Edsger W. Dijkstra
            poltawski Dan Poltawski made changes -
            Status Tested [ 10006 ] Closed [ 6 ]
            Integration date 07/Apr/16
            Currently in integration Yes [ 10041 ]
            Resolution Fixed [ 1 ]
            poltawski Dan Poltawski made changes -
            Summary Remove experimental flag from forum settings Timed posts should not be marked as experimental
            tsala Helen Foster made changes -
            Labels ci triaged ci docs_required triaged
            Hide
            marycooch Mary Cooch added a comment -
            Show
            marycooch Mary Cooch added a comment - Removing docs_required label as this is documented here https://docs.moodle.org/31/en/Using_Forum#For_teachers and here https://docs.moodle.org/31/en/Forum_settings#Timed_forum_posts .
            marycooch Mary Cooch made changes -
            Labels ci docs_required triaged ci triaged

              People

              • Votes:
                15 Vote for this issue
                Watchers:
                16 Start watching this issue

                Dates

                • Created:
                  Updated:
                  Resolved:
                  Fix Release Date:
                  23/May/16