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

Link a due date from Forum to the Calendar.

    Details

      Description

      When creating a forum, I would like to see a new item added that states:
      "Show this due date in the Calendar."

      This can go either above or below the "Restrict ratings to items with..." items, but it should not replace these "Restrict ratings..." items. Each will serve its own purpose. Currently, the forum setup webpage is lacking a connection to the calendar.

      With this approach, a forum can remain open even after the due date just in case students want to continue discussion. By also having the "Restrict dates..." field, it becomes obvious when student do not post by the due date. So I believe that all of these fields can work well together. However, maybe this post will stimulate some even better ideas.

        Gliffy Diagrams

        1. MDL-31355-add-duedate-forum-event-calendar.diff
          8 kB
          Isuru Madushanka Weerarathna
        1. 01_create_forum.PNG
          192 kB
        2. 02_fill_fields.PNG
          81 kB
        3. 03_event_created.PNG
          87 kB
        4. 04_link_in_description.PNG
          13 kB
        5. 05_edit_forum.PNG
          88 kB
        6. 06_change_duedate.PNG
          24 kB
        7. 07_event_updated.PNG
          21 kB
        8. 08_disable_duedate.PNG
          20 kB
        9. 09_delete_forum.PNG
          33 kB

          Issue Links

            Activity

            Hide
            salvetore Michael de Raadt added a comment -

            Thanks for suggesting this.

            If you can propose a code solution, that will help others who may have the same need and may that will increase the chance of this improvement/feature coming about sooner. If you are able to provide a patch, please add a patch label so we will spot it. Another way to raise the priority of this would be to suggest it in a forum and ask people to vote for this issue in the tracker.

            Show
            salvetore Michael de Raadt added a comment - Thanks for suggesting this. If you can propose a code solution, that will help others who may have the same need and may that will increase the chance of this improvement/feature coming about sooner. If you are able to provide a patch, please add a patch label so we will spot it. Another way to raise the priority of this would be to suggest it in a forum and ask people to vote for this issue in the tracker.
            Hide
            rjerz Rick Jerz added a comment -

            Michael, I am sorry to say that I cannot propose a code change because I am not a php programmer. It's on my list of things to learn. So we will have to wait for someone else to help.

            Show
            rjerz Rick Jerz added a comment - Michael, I am sorry to say that I cannot propose a code change because I am not a php programmer. It's on my list of things to learn. So we will have to wait for someone else to help.
            Hide
            isuru89 Isuru Madushanka Weerarathna added a comment -

            Hi,

            I would like to help here. I have gone through implementing this requested feature. So far I could add an event to the calendar considering only due date for rating of the forum. That is I could add a check box 'Show due date in calendar' which enables adding the rating deadline as an event in calendar.
            But the thing is that I had to add two columns for the moodle table '[prefix]_forum' to store those necessary data. I would really like to attach a patch here, but since I am new to moodle development community I would like to know how to define my database table changes in a patch file?

            • Thanks.
            Show
            isuru89 Isuru Madushanka Weerarathna added a comment - Hi, I would like to help here. I have gone through implementing this requested feature. So far I could add an event to the calendar considering only due date for rating of the forum. That is I could add a check box 'Show due date in calendar' which enables adding the rating deadline as an event in calendar. But the thing is that I had to add two columns for the moodle table ' [prefix] _forum' to store those necessary data. I would really like to attach a patch here, but since I am new to moodle development community I would like to know how to define my database table changes in a patch file? Thanks.
            Hide
            rjerz Rick Jerz added a comment -

            I appreciate you working on this item.

            I would prefer that another date item titled "Due Date" be added because I believe the "Restrict ratings" dates serves a different purpose (i.e., when posts can be rated, not when they are due). I don't know if a check box is also needed, but maybe so for consistency. If we consider the "Quiz" items, it contains both an "open" and a "close" date, along with enable. So maybe ideally, the forum should read:

            Open the forum (a date field)[]Enable
            Close the forum (a date field) []Enable

            These are suggestions, but I think any progress on this item would be helpful. I like to suggest improvements just in case it is the same amount of programming effort in either case.

            Show
            rjerz Rick Jerz added a comment - I appreciate you working on this item. I would prefer that another date item titled "Due Date" be added because I believe the "Restrict ratings" dates serves a different purpose (i.e., when posts can be rated, not when they are due). I don't know if a check box is also needed, but maybe so for consistency. If we consider the "Quiz" items, it contains both an "open" and a "close" date, along with enable. So maybe ideally, the forum should read: Open the forum (a date field)[]Enable Close the forum (a date field) []Enable These are suggestions, but I think any progress on this item would be helpful. I like to suggest improvements just in case it is the same amount of programming effort in either case.
            Hide
            rjerz Rick Jerz added a comment -

            Well, I thought more about your solution, and saw what you proposed in the moodle.org forums. Your solution would be fine.

            Show
            rjerz Rick Jerz added a comment - Well, I thought more about your solution, and saw what you proposed in the moodle.org forums. Your solution would be fine.
            Hide
            isuru89 Isuru Madushanka Weerarathna added a comment -

            Hi,

            I would like to propose my code solution in abstract manner before I upload it as a patch here. Because, there are some small database changes I had to do to make success in this feature. The changes I have done are indicated as follows.
            1. I had to insert two columns for '[prefix]_forum' to identify whether this forum has a due date event in calendar and the next column will identify corresponding event id.
            2. Additional check box has been provided to enable/disable showing the due date as an event in calendar. If user has unchecked the check box which was previously checked, the corresponding event will be deleted from table '[prefix]_events'. (Need feedback on whether the good solution will be to hide the event rather than deletes it)

            However, I would like to know feasibility of adding those extra column for that moodle table whether it allowed or not. As an alternative I can even create a separate table for each forum to store information regarding events. I really appreciate someone can let me know what could be the best solution here for a database change.

            -Thanks

            Show
            isuru89 Isuru Madushanka Weerarathna added a comment - Hi, I would like to propose my code solution in abstract manner before I upload it as a patch here. Because, there are some small database changes I had to do to make success in this feature. The changes I have done are indicated as follows. 1. I had to insert two columns for ' [prefix] _forum' to identify whether this forum has a due date event in calendar and the next column will identify corresponding event id. 2. Additional check box has been provided to enable/disable showing the due date as an event in calendar. If user has unchecked the check box which was previously checked, the corresponding event will be deleted from table ' [prefix] _events'. (Need feedback on whether the good solution will be to hide the event rather than deletes it) However, I would like to know feasibility of adding those extra column for that moodle table whether it allowed or not. As an alternative I can even create a separate table for each forum to store information regarding events. I really appreciate someone can let me know what could be the best solution here for a database change. -Thanks
            Hide
            isuru89 Isuru Madushanka Weerarathna added a comment -

            Hi,

            I have finished the implementation of this feature. I have attached the diff file. Or you can check it from here.
            https://github.com/isuru89/moodle/tree/MDL31355_forum_rating_event_calendar

            -Thanks

            Show
            isuru89 Isuru Madushanka Weerarathna added a comment - Hi, I have finished the implementation of this feature. I have attached the diff file. Or you can check it from here. https://github.com/isuru89/moodle/tree/MDL31355_forum_rating_event_calendar -Thanks
            Hide
            bltmacomb Barbara Taylor added a comment -

            Our instructors are use to this having this option in 1.9. How soon can we get this for 2.2.3 or 2.3.x? It seems to me that all activities should go to the calendar.

            Show
            bltmacomb Barbara Taylor added a comment - Our instructors are use to this having this option in 1.9. How soon can we get this for 2.2.3 or 2.3.x? It seems to me that all activities should go to the calendar.
            Hide
            rjerz Rick Jerz added a comment -

            I just upgraded to Moodle 2.4, and the new Assignments features made me think about Forums again. Also, I recently spent some time with my university's Blackboard support folks (they are trying to convince me to use Blackboard), and I noticed how disconnected the Calendar is to activities in Blackboard.

            The Moodle Forums remain disconnected to the Calendar, and the priority of getting it connected needs to be raised!

            In Moodle's 2.4 Assignments, there is now a "Cut-off date". The solution for Forums now becomes clear. The same "Cut-off date" method should be implemented for Forums, and this is how. The Forum's "Due Date" should show in the Calendar. The Forum's "Cut-off Date" should turn the rating feature off (which is what the current Due Date does). I believe that this logic provides a consistent logic with Assignments.

            Show
            rjerz Rick Jerz added a comment - I just upgraded to Moodle 2.4, and the new Assignments features made me think about Forums again. Also, I recently spent some time with my university's Blackboard support folks (they are trying to convince me to use Blackboard), and I noticed how disconnected the Calendar is to activities in Blackboard. The Moodle Forums remain disconnected to the Calendar, and the priority of getting it connected needs to be raised! In Moodle's 2.4 Assignments, there is now a "Cut-off date". The solution for Forums now becomes clear. The same "Cut-off date" method should be implemented for Forums, and this is how. The Forum's "Due Date" should show in the Calendar. The Forum's "Cut-off Date" should turn the rating feature off (which is what the current Due Date does). I believe that this logic provides a consistent logic with Assignments.
            Hide
            rjerz Rick Jerz added a comment -

            When I was at MoodleMoot West Coast (Portland) this summer, Martin Dougiamas spoke about how Forums were a high priority for improvement. He made me think about my request to link Forum Due Dates into the Calendar. At MoodleMoot, I encouraged attendees to vote for this tracker item. I am hoping that this request gets some attention.

            Show
            rjerz Rick Jerz added a comment - When I was at MoodleMoot West Coast (Portland) this summer, Martin Dougiamas spoke about how Forums were a high priority for improvement. He made me think about my request to link Forum Due Dates into the Calendar. At MoodleMoot, I encouraged attendees to vote for this tracker item. I am hoping that this request gets some attention.
            Hide
            bltmacomb Barbara Taylor added a comment - - edited

            If the Q&A could be setup better so instructors didn't have to create the post for each group (sometimes up to 20 groups or more in a large lecture class) that would be 2nd for me behind the dates in the calendar. #3 would be the ability to respond privately.

            Show
            bltmacomb Barbara Taylor added a comment - - edited If the Q&A could be setup better so instructors didn't have to create the post for each group (sometimes up to 20 groups or more in a large lecture class) that would be 2nd for me behind the dates in the calendar. #3 would be the ability to respond privately.
            Hide
            beachtch Lisa Beach added a comment -

            Calendar dates and private responses are at the top of my forum-priority list as well.

            Show
            beachtch Lisa Beach added a comment - Calendar dates and private responses are at the top of my forum-priority list as well.
            Hide
            aborrow Anthony Borrow added a comment -

            Rather than creating a calendar event, I am suggesting in MDL-48344 that information about rating restrictions be visible similar to how conditions from conditional activities are displayed. Peace - Anthony

            Show
            aborrow Anthony Borrow added a comment - Rather than creating a calendar event, I am suggesting in MDL-48344 that information about rating restrictions be visible similar to how conditions from conditional activities are displayed. Peace - Anthony
            Hide
            rjerz Rick Jerz added a comment -

            Anthony, I don't agree. A "Forum" is one of the major features of an LMS, so it should have a due date that goes into the Calendar. Of the three major LMS features, Quiz, Assignment, Forum, it is only the Forum that lacks this connection.

            What I see for Forums is something nearly identical to Assignment:
            1) Restrict ratings From (this is the start date)
            2) Due Date (this is what goes into the Calendar)
            3) Cut-off Data (this is identical to what we have today. It still allows posts, but the “rating” ability is no longer available. I really like this because it allows students to always go back and say something more. But if needed, maybe Moodle could have a fourth choice:
            4) End Posting (forum is officially closed and does not permit any more posts.)

            Show
            rjerz Rick Jerz added a comment - Anthony, I don't agree. A "Forum" is one of the major features of an LMS, so it should have a due date that goes into the Calendar. Of the three major LMS features, Quiz, Assignment, Forum, it is only the Forum that lacks this connection. What I see for Forums is something nearly identical to Assignment: 1) Restrict ratings From (this is the start date) 2) Due Date (this is what goes into the Calendar) 3) Cut-off Data (this is identical to what we have today. It still allows posts, but the “rating” ability is no longer available. I really like this because it allows students to always go back and say something more. But if needed, maybe Moodle could have a fourth choice: 4) End Posting (forum is officially closed and does not permit any more posts.)
            Hide
            aborrow Anthony Borrow added a comment -

            Rick,

            Thanks for sharing your comments and perspective. As I continue to think about it, I believe by creating MDL-48344, I am taking care of how ratings should be handled. That frees up this issue to specifically look at the issue of the due date (which I see as separate from the ratings dates).

            I agree that the forum is a major LMS feature and a due date does make sense from a teacher's perspective. In fact, I might go as far as to say in most cases having a due date should be the default (there may be some instances where it does not make sense to have a due date but then the due date functionality could just be turned off in those cases.

            To implement the due date for forum, I think that the approach would be to create a duedate field in the mod_forum and then treat it consistently as due date is treated with other activities including showing up on the calendar. If both issues were fixed, then to have the due date show on the calendar you would set the due date. This functionality would be independent of the ratings start and end dates as this is strictly for grading. So by fixing both of these issues I think we could be consistent in that due dates are shown on the calendar and ratings restrictions (like conditional restrictions) are shown with the activity.

            I also agree that allowing folks to post beyond the ratings date is important. If there is a duedate we may want to have an option to allow or disallow (default to allow) posts past due date. This is consistent with the assignment activity that allows late submissions. Having that ability to me is important not only to encourage students to post any time but also because it is consistent with the underlying constructionist pedagogy.

            What do you think of separating duedate from the ratings? I think we can get the best of both worlds with these two issues and that it would enhance the usability of the forums from a teachers perspective. Peace - Anthony

            Show
            aborrow Anthony Borrow added a comment - Rick, Thanks for sharing your comments and perspective. As I continue to think about it, I believe by creating MDL-48344 , I am taking care of how ratings should be handled. That frees up this issue to specifically look at the issue of the due date (which I see as separate from the ratings dates). I agree that the forum is a major LMS feature and a due date does make sense from a teacher's perspective. In fact, I might go as far as to say in most cases having a due date should be the default (there may be some instances where it does not make sense to have a due date but then the due date functionality could just be turned off in those cases. To implement the due date for forum, I think that the approach would be to create a duedate field in the mod_forum and then treat it consistently as due date is treated with other activities including showing up on the calendar. If both issues were fixed, then to have the due date show on the calendar you would set the due date. This functionality would be independent of the ratings start and end dates as this is strictly for grading. So by fixing both of these issues I think we could be consistent in that due dates are shown on the calendar and ratings restrictions (like conditional restrictions) are shown with the activity. I also agree that allowing folks to post beyond the ratings date is important. If there is a duedate we may want to have an option to allow or disallow (default to allow) posts past due date. This is consistent with the assignment activity that allows late submissions. Having that ability to me is important not only to encourage students to post any time but also because it is consistent with the underlying constructionist pedagogy. What do you think of separating duedate from the ratings? I think we can get the best of both worlds with these two issues and that it would enhance the usability of the forums from a teachers perspective. Peace - Anthony
            Hide
            rjerz Rick Jerz added a comment -

            Anthony, thanks for your reply. Yes, I think separating due date from ratings would be great.

            What I really seek is when creating (or editing) a forum, there should be a place to set a date that immediately goes into the Calendar. I really love how Quizzes and Assignments do this. Quite honestly, this date-to-calendar is a great moodle feature that does not exist in Blackboard. Someday, maybe Blackboard will have this feature, but I am confident that Moodle will have it first.

            So yes, just a simple "Calendar Due Date" would work. I can't wait to see this. As you see, my feature request is over two years old. I keep encouraging folks to vote on it.

            Show
            rjerz Rick Jerz added a comment - Anthony, thanks for your reply. Yes, I think separating due date from ratings would be great. What I really seek is when creating (or editing) a forum, there should be a place to set a date that immediately goes into the Calendar. I really love how Quizzes and Assignments do this. Quite honestly, this date-to-calendar is a great moodle feature that does not exist in Blackboard. Someday, maybe Blackboard will have this feature, but I am confident that Moodle will have it first. So yes, just a simple "Calendar Due Date" would work. I can't wait to see this. As you see, my feature request is over two years old. I keep encouraging folks to vote on it.
            Hide
            mjlambert Matt Lambert added a comment -

            I thought I would take a swing at this issue. I'm about halfway through.
            I'm in this branch: https://github.com/mjlambert/moodle/commits/MDL-31355-master

            Progress:

            • Updated DB
            • Added fields to form

            Todo:

            • Wire up calendar event creation/update
            • unit tests
            Show
            mjlambert Matt Lambert added a comment - I thought I would take a swing at this issue. I'm about halfway through. I'm in this branch: https://github.com/mjlambert/moodle/commits/MDL-31355-master Progress: Updated DB Added fields to form Todo: Wire up calendar event creation/update unit tests
            Hide
            rjerz Rick Jerz added a comment -

            Matt, I am not the best person to experiment, but I did try to download your working copy.

            I see at the bottom your "Due date to display in calendar Day" item, which looks great! However, might you want to somehow add a checkbox for "Show due date in Calendar"? For me, I think I would want to always show a forum's due date in the calendar, but some folks might not want this. Or do you plan to show this due date, or not show it, based upon whether or not "Restrict ratings to items with dates in this range:" is checked?

            By the way, I really do appreciate your work on addressing this issue (since I was the one who originally posted it.)

            Show
            rjerz Rick Jerz added a comment - Matt, I am not the best person to experiment, but I did try to download your working copy. I see at the bottom your "Due date to display in calendar Day" item, which looks great! However, might you want to somehow add a checkbox for "Show due date in Calendar"? For me, I think I would want to always show a forum's due date in the calendar, but some folks might not want this. Or do you plan to show this due date, or not show it, based upon whether or not "Restrict ratings to items with dates in this range:" is checked? By the way, I really do appreciate your work on addressing this issue (since I was the one who originally posted it.)
            Hide
            bltmacomb Barbara Taylor added a comment -

            Thank you Matt. +1 Rick's suggestion of "show due date in calendar." I suggest having that separate from "restrict ratings to items with dates in this range" since many of our instructors don't use that feature but want the due date on the calendar.

            Show
            bltmacomb Barbara Taylor added a comment - Thank you Matt. +1 Rick's suggestion of "show due date in calendar." I suggest having that separate from "restrict ratings to items with dates in this range" since many of our instructors don't use that feature but want the due date on the calendar.
            Hide
            mjlambert Matt Lambert added a comment -

            Hey, thanks for the feedback.
            I actually have a checkbox called 'duedateenabled' under the same heading as the 'duedate' date selector. (I might have missed something in my commit if it's not showing up for you?)
            I'm planning to hook it all up tomorrow, so I'll be able to check everything then.

            Show
            mjlambert Matt Lambert added a comment - Hey, thanks for the feedback. I actually have a checkbox called 'duedateenabled' under the same heading as the 'duedate' date selector. (I might have missed something in my commit if it's not showing up for you?) I'm planning to hook it all up tomorrow, so I'll be able to check everything then.
            Hide
            mjlambert Matt Lambert added a comment -

            I've finished all the functionality in the issue.
            I'm just working on updating the unit test.

            As I do not have permissions to assign this issue to myself, can I request a peer review?
            Here is my working branch: https://github.com/mjlambert/moodle/commits/MDL-31355-master

            Show
            mjlambert Matt Lambert added a comment - I've finished all the functionality in the issue. I'm just working on updating the unit test. As I do not have permissions to assign this issue to myself, can I request a peer review? Here is my working branch: https://github.com/mjlambert/moodle/commits/MDL-31355-master
            Hide
            cibot CiBoT added a comment -

            Fails against automated checks.

            Checked MDL-31355 using repository: git://github.com/mjlambert/moodle.git

            More information about this report

            Show
            cibot CiBoT added a comment - Fails against automated checks. Checked MDL-31355 using repository: git://github.com/mjlambert/moodle.git Testing instructions are missing. master (14 errors / 7 warnings) [branch: MDL-31355-master | CI Job ] phplint (0/0) , php (12/7) , js (0/0) , css (0/0) , phpdoc (0/0) , commit (2/0) , savepoint (0/0) , thirdparty (0/0) , More information about this report
            Hide
            jethac Jetha Chan added a comment - - edited

            Thanks for working on this, Matt!

            [N] Syntax
            [N] Whitespace
            [-] Output
            [N] Language
            [Y] Databases
            [N] Testing (instructions and automated tests)
            [-] Security
            [N] Documentation
            [N] Git
            [-] Third party code
            [Y] Sanity check
            [-] Icons
            

            • Syntax: In mod/forum/lib.php:
              • a new variable is introduced with underscores - underscores are not allowed for variables in Moodle (they are, however, permitted for functions).
              • inline comments need to end with full-stops, exclamation marks or question marks
              • lines 199 and 220, move that comment down one line so you can use the } else { construct
            • Syntax: In mod/forum/mod_form.php:
              • line 187: while I can see that matching the surrounding code was your intent, the surrounding code itself doesn't abide by our guidelines remove this separator and you'll be fine.
            • Whitespace: Extraneous whitespace in mod/forum/db/upgrade.php - three blank lines in a row is too much, one line would be better
            • Language: Your new lang string duedate could potentially be copied from an existing lang string - mod_assign has a lang string also called duedate.
              • We use a system called AMOS in cases like this, which tells our build system what to do with lang strings (refer to our AMOS cheatsheet). In this case, you could instrument the commit in question (cdc77dcfc5b36ed28391a7676ec596b7fe467d90) like so:

                MDL-31355 mod_forum: Add due date element to form
                 
                AMOS BEGIN
                 CPY [duedate,mod_assign],[duedate,mod_forum]
                AMOS END
                

            • Testing: It'd be good to get some automated tests for this, both PHPUnit and Behat. See https://docs.moodle.org/dev/Unit_test_API
            • Testing: Need manual testing instructions.
            • Documentation: This is a UI change - tagging with ui_change and docs_required.
            • Git: Commits 548e1b2 and 77899df have commit messages that are too long - we typically require that commit messages have a maximum line length of 72 characters.

            Sorry for the length - it's a good first effort though! Fix these things up and there's no reason this couldn't get in.

            Show
            jethac Jetha Chan added a comment - - edited Thanks for working on this, Matt! [N] Syntax [N] Whitespace [-] Output [N] Language [Y] Databases [N] Testing (instructions and automated tests) [-] Security [N] Documentation [N] Git [-] Third party code [Y] Sanity check [-] Icons Syntax : In mod/forum/lib.php : a new variable is introduced with underscores - underscores are not allowed for variables in Moodle (they are, however, permitted for functions). inline comments need to end with full-stops, exclamation marks or question marks lines 199 and 220, move that comment down one line so you can use the } else { construct Syntax : In mod/forum/mod_form.php : line 187: while I can see that matching the surrounding code was your intent, the surrounding code itself doesn't abide by our guidelines remove this separator and you'll be fine. Whitespace : Extraneous whitespace in mod/forum/db/upgrade.php - three blank lines in a row is too much, one line would be better Language : Your new lang string duedate could potentially be copied from an existing lang string - mod_assign has a lang string also called duedate . We use a system called AMOS in cases like this, which tells our build system what to do with lang strings (refer to our AMOS cheatsheet ). In this case, you could instrument the commit in question (cdc77dcfc5b36ed28391a7676ec596b7fe467d90) like so: MDL-31355 mod_forum: Add due date element to form   AMOS BEGIN CPY [duedate,mod_assign],[duedate,mod_forum] AMOS END Testing : It'd be good to get some automated tests for this, both PHPUnit and Behat. See https://docs.moodle.org/dev/Unit_test_API Testing : Need manual testing instructions. Documentation : This is a UI change - tagging with ui_change and docs_required . Git : Commits 548e1b2 and 77899df have commit messages that are too long - we typically require that commit messages have a maximum line length of 72 characters. Sorry for the length - it's a good first effort though! Fix these things up and there's no reason this couldn't get in.
            Hide
            cibot CiBoT added a comment -

            Fails against automated checks.

            Checked MDL-31355 using repository: git://github.com/mjlambert/moodle.git

            More information about this report

            Show
            cibot CiBoT added a comment - Fails against automated checks. Checked MDL-31355 using repository: git://github.com/mjlambert/moodle.git Testing instructions are missing. master (1 errors / 0 warnings) [branch: MDL-31355-master | CI Job ] phplint (0/0) , php (1/0) , js (0/0) , css (0/0) , phpdoc (0/0) , commit (0/0) , savepoint (0/0) , thirdparty (0/0) , More information about this report
            Hide
            mjlambert Matt Lambert added a comment -

            Thanks for the feedback.

            I have addressed all the issues listed above and have finished updating the unit test.

            Everything is committed to my working branch: https://github.com/mjlambert/moodle/commits/MDL-31355-master

            Show
            mjlambert Matt Lambert added a comment - Thanks for the feedback. I have addressed all the issues listed above and have finished updating the unit test. Everything is committed to my working branch: https://github.com/mjlambert/moodle/commits/MDL-31355-master
            Hide
            cibot CiBoT added a comment -

            Fails against automated checks.

            Checked MDL-31355 using repository: git://github.com/mjlambert/moodle.git

            More information about this report

            Show
            cibot CiBoT added a comment - Fails against automated checks. Checked MDL-31355 using repository: git://github.com/mjlambert/moodle.git Testing instructions are missing. master (6 errors / 1 warnings) [branch: MDL-31355-master | CI Job ] phplint (0/0) , php (5/1) , js (0/0) , css (0/0) , phpdoc (1/0) , commit (0/0) , savepoint (0/0) , thirdparty (0/0) , More information about this report
            Hide
            jethac Jetha Chan added a comment -

            Thanks for the update Matt! Looking better.

            • Syntax: Some minor coding style errors remain (see the CiBoT comment directly preceding this one); I'm pretty sure you don't need the preceding & in your PHPDocs block for forum_update_duedate(). That said...
            • Syntax: I'm not sure how much I like the current form of forum_update_duedate(), seeing as it requires two objects as parameters - that's a bit heavy for a function that does what it does. Perhaps:
              • Split calendar event creation and deletion into their own functions (perhaps forum_create_duedate() and forum_delete_duedate?) as the logic to do that is now duplicated in two places - needs to be maintainable
              • Refactor forum_update_duedate() such that it doesn't need two objects as parameters - it seems like you could pass in 1) a forum object and 2) the id (if any) of an extant event instance
            • Testing: I'll confess to being a little confused by:

              // Assert true if event cannot be found.
              try {
                  $event = \calendar_event::load($forumduedateevent);
              } catch(dml_missing_record_exception $e) {
                  $this->assertTrue(true);
              }
              

              I suggest removing the try/catch and just modifying this code to assert that $event should not be false (you can use $this->assertNotFalse() to accomplish this).

            • Testing: Great work writing that PHPUnit test, but we still need manual testing instructions - what should a user do to test this new functionality?
            • Sanity check: Would be good to see calendar events created with a link back to the forum instance - the user shouldn't have to manually go to the course and then find the activity with the right name.

            Sorry for the big review!

            Show
            jethac Jetha Chan added a comment - Thanks for the update Matt! Looking better. Syntax : Some minor coding style errors remain (see the CiBoT comment directly preceding this one); I'm pretty sure you don't need the preceding & in your PHPDocs block for forum_update_duedate(). That said... Syntax : I'm not sure how much I like the current form of forum_update_duedate(), seeing as it requires two objects as parameters - that's a bit heavy for a function that does what it does. Perhaps: Split calendar event creation and deletion into their own functions (perhaps forum_create_duedate() and forum_delete_duedate?) as the logic to do that is now duplicated in two places - needs to be maintainable Refactor forum_update_duedate() such that it doesn't need two objects as parameters - it seems like you could pass in 1) a forum object and 2) the id (if any) of an extant event instance Testing : I'll confess to being a little confused by: // Assert true if event cannot be found. try { $event = \calendar_event::load($forumduedateevent); } catch(dml_missing_record_exception $e) { $this->assertTrue(true); } I suggest removing the try/catch and just modifying this code to assert that $event should not be false (you can use $this->assertNotFalse() to accomplish this). Testing : Great work writing that PHPUnit test, but we still need manual testing instructions - what should a user do to test this new functionality? Sanity check : Would be good to see calendar events created with a link back to the forum instance - the user shouldn't have to manually go to the course and then find the activity with the right name. Sorry for the big review!
            Hide
            marina Marina Glancy added a comment -

            Hello, I was just looking at MDL-50338 (forum cut-off date) and then found this issue.
            I looked at the code and my biggest question is - WHY are we introducing this date? From Moodle point of view, what changes if student submits a post before or after this date? As far as I can see - absolutely nothing. So, what's the point? Just to create a calendar event?
            It has to be linked to the completion criteria, or there should be an option to disable posts after due date, or something else.

            Show
            marina Marina Glancy added a comment - Hello, I was just looking at MDL-50338 (forum cut-off date) and then found this issue. I looked at the code and my biggest question is - WHY are we introducing this date? From Moodle point of view, what changes if student submits a post before or after this date? As far as I can see - absolutely nothing. So, what's the point? Just to create a calendar event? It has to be linked to the completion criteria, or there should be an option to disable posts after due date, or something else.
            Hide
            rjerz Rick Jerz added a comment -

            Hi Marina, I am the creator of this feature request, so I will try to answer your question.

            In many courses, the main activities include quizzes, assignments, and discussions. These activities typically are graded, and they can have due dates. For quizzes and assignments, a due date is automatically put into the Calendar when the activity is defined, but this does not happen for forums (i.e., there is no coordination between a due date and the calendar.) This was the basis of my initial request; that a forum due date should be created in the Calendar at the time the forum is created, and to avoid always having to manually enter the due date into the Calendar.

            I am not sure that I follow your position of “what changes if student submits a post before or after this date?” One could make the statement about quizzes and assignments “what changes if the students submits their quiz (or assignment) after the due date?”

            The point is to provide a consistent logical place to create Calendar due dates for main Moodle activities.

            Incidentally, forums already have the ability to “disable posts after due date.” But this serves a different purpose, at least to me. For a quiz, we have the ability to define three dates: open, closed, and grace times (close shows in Calendar). For an assignment, we have the ability to define three dates: start, due, and cut-off times (due shows in Calendar). But for a forum, neither of the two dates, “from” or “to”, show in the calendar. It makes sense to have some date that links to the calendar, and Matt’s way of adding a calendar date makes sense. Yes, maybe there is even a better way, but this feature is long overdue and Matt is doing a good job adding this feature.

            I am willing to try Matt’s approach, and if necessary, request changes to it if I see an even better way to do this. I really appreciate Matt's work on this issue.

            Show
            rjerz Rick Jerz added a comment - Hi Marina, I am the creator of this feature request, so I will try to answer your question. In many courses, the main activities include quizzes, assignments, and discussions. These activities typically are graded, and they can have due dates. For quizzes and assignments, a due date is automatically put into the Calendar when the activity is defined, but this does not happen for forums (i.e., there is no coordination between a due date and the calendar.) This was the basis of my initial request; that a forum due date should be created in the Calendar at the time the forum is created, and to avoid always having to manually enter the due date into the Calendar. I am not sure that I follow your position of “what changes if student submits a post before or after this date?” One could make the statement about quizzes and assignments “what changes if the students submits their quiz (or assignment) after the due date?” The point is to provide a consistent logical place to create Calendar due dates for main Moodle activities. Incidentally, forums already have the ability to “disable posts after due date.” But this serves a different purpose, at least to me. For a quiz, we have the ability to define three dates: open, closed, and grace times (close shows in Calendar). For an assignment, we have the ability to define three dates: start, due, and cut-off times (due shows in Calendar). But for a forum, neither of the two dates, “from” or “to”, show in the calendar. It makes sense to have some date that links to the calendar, and Matt’s way of adding a calendar date makes sense. Yes, maybe there is even a better way, but this feature is long overdue and Matt is doing a good job adding this feature. I am willing to try Matt’s approach, and if necessary, request changes to it if I see an even better way to do this. I really appreciate Matt's work on this issue.
            Hide
            cibot CiBoT added a comment -

            Fails against automated checks.

            Checked MDL-31355 using repository: git://github.com/mjlambert/moodle.git

            More information about this report

            Show
            cibot CiBoT added a comment - Fails against automated checks. Checked MDL-31355 using repository: git://github.com/mjlambert/moodle.git Testing instructions are missing. master (0 errors / 0 warnings) [branch: MDL-31355-master | CI Job ] More information about this report
            Hide
            marina Marina Glancy added a comment -

            This is what the help text for assignment "due date" says:

            This is when the assignment is due. Submissions will still be allowed after this date but any assignments submitted after this date are marked as late. To prevent submissions after a certain date - set the assignment cut off date.

            Which makes perfect sense. But in the case of forum - it is not possible to mark posts as "late", so there is absolutely no visual difference between the posts submitted before or after due date.
            Besides, there is no cut-off date for forums either, that's exactly what MDL-50338 is suggesting. Until we implement the cut-off date the "due date" will only be confusing. My recommendation is to create a single patch for both due date and cut-off date.

            Show
            marina Marina Glancy added a comment - This is what the help text for assignment "due date" says: This is when the assignment is due. Submissions will still be allowed after this date but any assignments submitted after this date are marked as late. To prevent submissions after a certain date - set the assignment cut off date. Which makes perfect sense. But in the case of forum - it is not possible to mark posts as "late", so there is absolutely no visual difference between the posts submitted before or after due date. Besides, there is no cut-off date for forums either, that's exactly what MDL-50338 is suggesting. Until we implement the cut-off date the "due date" will only be confusing. My recommendation is to create a single patch for both due date and cut-off date.
            Hide
            rjerz Rick Jerz added a comment -

            Marina, you have made me think more about these "dates" in Moodle. I will try to gather my thoughts and make a post later. However, I do want to address one of your comments, that for forums, "there is absolutely no visual difference between the posts submitted before or after due date."

            Yes there is. Right now, posts submitted after the due date do not provide the instructor the ability to "rate" late posts. The "rate" feature is not shown for overdue posts.

            I use this (current) feature as a way of not providing points for late posts. It is my strong visual cue for grading posts. In fact, for forums, I really like this feature. I am one who likes to tell students that forums have a due date "for credit", but discussions are always open throughout the semester in case anyone wants to return to an earlier post and provide additional thoughts. These will not get credit, but I find that I never want to reject any students' ideas, and that they are always able to provide thoughts (posts) to any discussion topic throughout the semester.

            However, I do understand that some instructors might want to completely turn off discussions on some particular date, and there really is a need to review the ideas for start, due, cut-off dates for various activities in order to improve their capabilities, and to make them consistent. This also will get us into a discussion about "user overrides," right?

            Show
            rjerz Rick Jerz added a comment - Marina, you have made me think more about these "dates" in Moodle. I will try to gather my thoughts and make a post later. However, I do want to address one of your comments, that for forums, "there is absolutely no visual difference between the posts submitted before or after due date." Yes there is. Right now, posts submitted after the due date do not provide the instructor the ability to "rate" late posts. The "rate" feature is not shown for overdue posts. I use this (current) feature as a way of not providing points for late posts. It is my strong visual cue for grading posts. In fact, for forums, I really like this feature. I am one who likes to tell students that forums have a due date "for credit", but discussions are always open throughout the semester in case anyone wants to return to an earlier post and provide additional thoughts. These will not get credit, but I find that I never want to reject any students' ideas, and that they are always able to provide thoughts (posts) to any discussion topic throughout the semester. However, I do understand that some instructors might want to completely turn off discussions on some particular date, and there really is a need to review the ideas for start, due, cut-off dates for various activities in order to improve their capabilities, and to make them consistent. This also will get us into a discussion about "user overrides," right?
            Hide
            mjlambert Matt Lambert added a comment -

            Hey guys,

            I've fixed all the issues mentioned above and have added a link to the forum in the event.
            Branch: https://github.com/mjlambert/moodle/commits/MDL-31355-master

            Since I don't have permission to add the manual testing instructions myself, I'll just leave them here.

            Test due date event is created

            1. Add a new forum activity to a course.
            2. On the 'Adding a new Forum' page, give the forum a name, description, and enable a due date by clicking the 'enable' checkbox under the 'Due Date' heading.
            3. Click 'Save and return to course' and go to the calendar.
            4. Verify an event has been created on the date you specified.
            5. Verify there is a link in the event description that links to the forum.

            Test due date event is updated when the forum changes

            1. Go back to the course and click 'Edit settings' on the forum you created.
            2. Under the due date heading change the due date
            3. Save the forum and go to the calendar.
            4. Verify the due date event has been updated to the new date.
            5. Edit the forum again and uncheck the 'enable' checkbox under the due date heading
            6. Save the forum and go to the calendar
            7. Verify the due date event has been removed
            8. Edit the forum again and re-enable the due date
            9. Save the forum and go back to the calendar
            10. Verify the due date event has been created again.

            Test due date event is deleted when forum has been deleted

            1. Make sure the due date event still exists
            2. Delete the forum
            3. Go to the calendar
            4. Verify the due date event has been removed.
            Show
            mjlambert Matt Lambert added a comment - Hey guys, I've fixed all the issues mentioned above and have added a link to the forum in the event. Branch: https://github.com/mjlambert/moodle/commits/MDL-31355-master Since I don't have permission to add the manual testing instructions myself, I'll just leave them here. Test due date event is created Add a new forum activity to a course. On the 'Adding a new Forum' page, give the forum a name, description, and enable a due date by clicking the 'enable' checkbox under the 'Due Date' heading. Click 'Save and return to course' and go to the calendar. Verify an event has been created on the date you specified. Verify there is a link in the event description that links to the forum. Test due date event is updated when the forum changes Go back to the course and click 'Edit settings' on the forum you created. Under the due date heading change the due date Save the forum and go to the calendar. Verify the due date event has been updated to the new date. Edit the forum again and uncheck the 'enable' checkbox under the due date heading Save the forum and go to the calendar Verify the due date event has been removed Edit the forum again and re-enable the due date Save the forum and go back to the calendar Verify the due date event has been created again. Test due date event is deleted when forum has been deleted Make sure the due date event still exists Delete the forum Go to the calendar Verify the due date event has been removed.
            Hide
            marina Marina Glancy added a comment -

            Hi Rick, that's a good point and I also saw it in the original description. However Matt's implementation is different. He adds a new date field to the form.

            BTW Matt, I tried to check out your branch to test and got an error:

            Debug info: Unknown column 'duedateenabled' in 'm_forum'
            ALTER TABLE m_forum ADD duedateevent BIGINT(10) NOT NULL DEFAULT 0 AFTER duedateenabled
            Error code: ddlexecuteerror
            Stack trace:
             
                line 449 of /lib/dml/moodle_database.php: ddl_change_structure_exception thrown
                line 905 of /lib/dml/mysqli_native_moodle_database.php: call to moodle_database->query_end()
                line 76 of /lib/ddl/database_manager.php: call to mysqli_native_moodle_database->change_database_structure()
                line 533 of /lib/ddl/database_manager.php: call to database_manager->execute_sql_arr()
                line 262 of /mod/forum/db/upgrade.php: call to database_manager->add_field()
                line 706 of /lib/upgradelib.php: call to xmldb_forum_upgrade()
                line 424 of /lib/upgradelib.php: call to upgrade_plugins_modules()
                line 1630 of /lib/upgradelib.php: call to upgrade_plugins()
                line 433 of /admin/index.php: call to upgrade_noncore()
            

            Show
            marina Marina Glancy added a comment - Hi Rick, that's a good point and I also saw it in the original description. However Matt's implementation is different. He adds a new date field to the form. BTW Matt, I tried to check out your branch to test and got an error: Debug info: Unknown column 'duedateenabled' in 'm_forum' ALTER TABLE m_forum ADD duedateevent BIGINT(10) NOT NULL DEFAULT 0 AFTER duedateenabled Error code: ddlexecuteerror Stack trace:   line 449 of /lib/dml/moodle_database.php: ddl_change_structure_exception thrown line 905 of /lib/dml/mysqli_native_moodle_database.php: call to moodle_database->query_end() line 76 of /lib/ddl/database_manager.php: call to mysqli_native_moodle_database->change_database_structure() line 533 of /lib/ddl/database_manager.php: call to database_manager->execute_sql_arr() line 262 of /mod/forum/db/upgrade.php: call to database_manager->add_field() line 706 of /lib/upgradelib.php: call to xmldb_forum_upgrade() line 424 of /lib/upgradelib.php: call to upgrade_plugins_modules() line 1630 of /lib/upgradelib.php: call to upgrade_plugins() line 433 of /admin/index.php: call to upgrade_noncore()
            Hide
            marina Marina Glancy added a comment -

            Matt, I fixed the error in my branch and was able to finish upgrade.

            So let me do a review. First of all, I would get rid of the field duedateevent . Please see the function update_calendar() in mod/assign/locallib.php - you should have something similar.
            Also position in mod_form that Rick suggested in the issue description seems much more logical than where you placed it (in the bottom of the form). You can even move it to the very top like in assignment.

            And I will still insist that duedate should be implemented together with cutoff date. Cut-off date does not need to appear in the calendar. Feel free to borrow the form interface and copy strings from mod_assign

            Show
            marina Marina Glancy added a comment - Matt, I fixed the error in my branch and was able to finish upgrade. So let me do a review. First of all, I would get rid of the field duedateevent . Please see the function update_calendar() in mod/assign/locallib.php - you should have something similar. Also position in mod_form that Rick suggested in the issue description seems much more logical than where you placed it (in the bottom of the form). You can even move it to the very top like in assignment. And I will still insist that duedate should be implemented together with cutoff date. Cut-off date does not need to appear in the calendar. Feel free to borrow the form interface and copy strings from mod_assign
            Hide
            rjerz Rick Jerz added a comment -

            Marina (and Matt), the addition of another date field (3 in total) matches the number of date fields in the Quiz and Assignment activities (the Quiz has a Grace Period, in a group called "Timing", and the Assignments has a Cut-off Date in a group called "Availability".)

            I do agree with Marina that it would be best if dates were presented in groups. So this new Calendar Date might be best group within the "Ratings" group, probably after the current "To" item in this list. But we would have to be careful not confuse it with "Restrict Ratings to items with dates in this range", I think, meaning that someone might not want to restrict ratings, yet show this forum due date in the calendar. It is, of course, easy for me to suggest this because I am not the programmer, but realistically, I am not sure how easy or difficult it is for you to move your code, Matt.

            Marina, I totally agree with you about separating due date with cutoff date, and the work that is being done on MDL-50338. Even though this would be a fourth date, I think it is a very good idea for instructors who really want to shut down a forum. Maybe Matt's good work, and progress on this item, will inspire the completion of MDL-50338. Although these various activities have different group names (Timing, Availability, and Ratings), this doesn't bother me at all.

            Show
            rjerz Rick Jerz added a comment - Marina (and Matt), the addition of another date field (3 in total) matches the number of date fields in the Quiz and Assignment activities (the Quiz has a Grace Period, in a group called "Timing", and the Assignments has a Cut-off Date in a group called "Availability".) I do agree with Marina that it would be best if dates were presented in groups. So this new Calendar Date might be best group within the "Ratings" group, probably after the current "To" item in this list. But we would have to be careful not confuse it with "Restrict Ratings to items with dates in this range", I think, meaning that someone might not want to restrict ratings, yet show this forum due date in the calendar. It is, of course, easy for me to suggest this because I am not the programmer, but realistically, I am not sure how easy or difficult it is for you to move your code, Matt. Marina, I totally agree with you about separating due date with cutoff date, and the work that is being done on MDL-50338 . Even though this would be a fourth date, I think it is a very good idea for instructors who really want to shut down a forum. Maybe Matt's good work, and progress on this item, will inspire the completion of MDL-50338 . Although these various activities have different group names (Timing, Availability, and Ratings), this doesn't bother me at all.

              People

              • Votes:
                27 Vote for this issue
                Watchers:
                20 Start watching this issue

                Dates

                • Created:
                  Updated: