Details

    • Testing Instructions:
      Hide

      Course forms & defaults

      1. Go to admin/settings.php?section=coursesettings, set "Course duration" to 6 months and save changes
      2. Set your default course format to topics
      3. Create a new course
      4. The default end date value should be startdate + 6 months
      5. Change the default course format in admin/settings.php?section=coursesettings to weekly
      6. Create a new course
      7. The default course end date should be startdate + (1 week for each section = numsections setting value)
      1. Go to an existing course without end date, end date should be disabled
      2. Set end date < start date and try to save changes
      3. The form server validation shouldn't allow you to do it
      4. Set the end date > start date
      5. You should be able to save changes
      6. Return to the form and check that your value is stored
      7. Uncheck end date and save changes
      8. Return to the form and check that there is no end date

      Course reset

      1. Using this same course used above (have both course start and end dates) go to 'Course administration' -> 'Reset'
      2. Just press 'Reset course'
      3. Check the course settings, the shouldn't have changed
      4. Go to 'Course administration' -> 'Reset' again
      5. Set course end date < the course current start date and press 'Reset course'
      6. You should see a form validation error
      7. Set course end date > the start date you noted down (the course current start date) and press 'Reset course'
      8. The process should finish, check that the course end date was reset in the course settings page
      9. Go to 'Course administration' -> 'Reset' again
      10. Enable end date as well and set a date lower than the reset start date. Press 'Reset course'
      11. Expect again a validation error
      12. Now set end date to be > than the reset start date and press 'Reset course'
      13. The reset should proceed and the new start and end dates should be visible in the course settings page
      14. Go to 'Course administration' -> 'Reset' again
      15. Set both end date and start date (start date < end date) and press 'Reset course'
      16. The reset should proceed and the new start and end dates should be visible in the course settings page
      17. Note the current end date (you will need it a few steps later to check that the time shift has been applied to this end date value)
      18. Go to 'Course administration' -> 'Reset' again
      19. Set the start date, note the difference in days (or months or years) between the previous start date and the new one you set. Press 'Reset course'
      20. The reset should proceed and the new start date should be visible and end date should be the previous end date value + the difference between the previous start date and the new start date

      Backup & restore

      1. Backup a 31 course with a start date (will have a start date if you create it through 'Manage courses and categories' for example)
      2. Restore it to master integration, go to the course settings page and check that the start date is the same than it was and there is no end date
      3. Restore it to master integration changing the start date, go to the course settings page and check that the start date is the one you set and there is no end date
      1. Backup a master integration course with start date and no end date
      2. Restore it to master integration, go to the course settings page and check that the start date is the same than it was and there is no end date
      3. Restore it to master integration changing the start date, go to the course settings page and check that the start date is the one you set and there is no end date
      1. Backup a master integration course with start date and end date
      2. Restore it to master integration, go to the course settings page and check that the start date is the same than it was and the end date is the same than it was
      3. Restore it to master integration changing the start date, go to the course settings page and check that the start date is the one you set and the end date has been updated according to the changed you applied to the start date (e.g. if you moved it 5 days in future end date should also be moved 5 days in future)

      Web services & course API

      1. Call create_courses external function (you don't need to use web services, you can just call core_course_external::create_courses()) and try different startdate and enddate combinations:
        • Set start date > end date -> KO
        • Don't set start date, set end date -> KO
        • Set start date, don't set end date -> OK
        • Set start date < end date -> OK
      1. Call core_course_duplicate_course WS specifying a course with start and end date correctly set up (latest combination above should be ok)
      2. Go to the course and check that the start and end dates are the same than in the origin course

      Upload courses

      1. Prepare a simple csv to upload a course, something like this:
        • shortname,fullname,category
        • uploadedcourses1,Uploaded 1,1
      2. Use upload course tool selecting "Create new courses, or update existing ones" as upload mode and "Update with CSV data and defaults" as update mode, move to the next form
      3. Check that the default end date date is after the start date (will depend on the default course format as before)
      4. In default course values section test the form validation, updating end date to be earlier than start date, you should see a form validation error
      5. Now restore end date value to be after the start date
      6. Press 'Upload courses', it should succeed
      7. Modify the csv to include a start date and an end date, for example
        • shortname,fullname,category,startdate,enddate
        • uploadedcourses2,Uploaded 2,1,10 June 2005,10 March 2005
      8. Try to upload the course, you will see a red cross in the preview result as the enddate must be after the start date
      9. Update the csv to set the end date after the start date and start the upload process again
      10. Now it should succeed
      Show
      Course forms & defaults Go to admin/settings.php?section=coursesettings, set "Course duration" to 6 months and save changes Set your default course format to topics Create a new course The default end date value should be startdate + 6 months Change the default course format in admin/settings.php?section=coursesettings to weekly Create a new course The default course end date should be startdate + (1 week for each section = numsections setting value) Go to an existing course without end date, end date should be disabled Set end date < start date and try to save changes The form server validation shouldn't allow you to do it Set the end date > start date You should be able to save changes Return to the form and check that your value is stored Uncheck end date and save changes Return to the form and check that there is no end date Course reset Using this same course used above (have both course start and end dates) go to 'Course administration' -> 'Reset' Just press 'Reset course' Check the course settings, the shouldn't have changed Go to 'Course administration' -> 'Reset' again Set course end date < the course current start date and press 'Reset course' You should see a form validation error Set course end date > the start date you noted down (the course current start date) and press 'Reset course' The process should finish, check that the course end date was reset in the course settings page Go to 'Course administration' -> 'Reset' again Enable end date as well and set a date lower than the reset start date. Press 'Reset course' Expect again a validation error Now set end date to be > than the reset start date and press 'Reset course' The reset should proceed and the new start and end dates should be visible in the course settings page Go to 'Course administration' -> 'Reset' again Set both end date and start date (start date < end date) and press 'Reset course' The reset should proceed and the new start and end dates should be visible in the course settings page Note the current end date (you will need it a few steps later to check that the time shift has been applied to this end date value) Go to 'Course administration' -> 'Reset' again Set the start date, note the difference in days (or months or years) between the previous start date and the new one you set. Press 'Reset course' The reset should proceed and the new start date should be visible and end date should be the previous end date value + the difference between the previous start date and the new start date Backup & restore Backup a 31 course with a start date (will have a start date if you create it through 'Manage courses and categories' for example) Restore it to master integration, go to the course settings page and check that the start date is the same than it was and there is no end date Restore it to master integration changing the start date, go to the course settings page and check that the start date is the one you set and there is no end date Backup a master integration course with start date and no end date Restore it to master integration, go to the course settings page and check that the start date is the same than it was and there is no end date Restore it to master integration changing the start date, go to the course settings page and check that the start date is the one you set and there is no end date Backup a master integration course with start date and end date Restore it to master integration, go to the course settings page and check that the start date is the same than it was and the end date is the same than it was Restore it to master integration changing the start date, go to the course settings page and check that the start date is the one you set and the end date has been updated according to the changed you applied to the start date (e.g. if you moved it 5 days in future end date should also be moved 5 days in future) Web services & course API Call create_courses external function (you don't need to use web services, you can just call core_course_external::create_courses()) and try different startdate and enddate combinations: Set start date > end date -> KO Don't set start date, set end date -> KO Set start date, don't set end date -> OK Set start date < end date -> OK Call core_course_duplicate_course WS specifying a course with start and end date correctly set up (latest combination above should be ok) Go to the course and check that the start and end dates are the same than in the origin course Upload courses Prepare a simple csv to upload a course, something like this: shortname,fullname,category uploadedcourses1,Uploaded 1,1 Use upload course tool selecting "Create new courses, or update existing ones" as upload mode and "Update with CSV data and defaults" as update mode, move to the next form Check that the default end date date is after the start date (will depend on the default course format as before) In default course values section test the form validation, updating end date to be earlier than start date, you should see a form validation error Now restore end date value to be after the start date Press 'Upload courses', it should succeed Modify the csv to include a start date and an end date, for example shortname,fullname,category,startdate,enddate uploadedcourses2,Uploaded 2,1,10 June 2005,10 March 2005 Try to upload the course, you will see a red cross in the preview result as the enddate must be after the start date Update the csv to set the end date after the start date and start the upload process again Now it should succeed
    • Difficulty:
      Easy
    • Affected Branches:
      MOODLE_19_STABLE, MOODLE_26_STABLE, MOODLE_27_STABLE, MOODLE_29_STABLE, MOODLE_30_STABLE, MOODLE_31_STABLE, MOODLE_32_STABLE
    • Fixed Branches:
      MOODLE_32_STABLE
    • Pull from Repository:
    • Pull Master Branch:
      MDL-22078_master

      Description

      Create a Course End Date Value that when using the Weekly Format of a course, you can have the last week end sooner than a whole week. Example a 3 week course that starts on Jan 1, but ends of Jan 20, not Jan 21 which is what the weekly format will show.

        Gliffy Diagrams

          Attachments

            Issue Links

              Activity

              Hide
              bfriesen B. Friesen added a comment -

              I'm definitely interested in a course end date. Not necessarily for this reason, but just for the sake of being able to identify courses that are over and should be closed.

              Show
              bfriesen B. Friesen added a comment - I'm definitely interested in a course end date. Not necessarily for this reason, but just for the sake of being able to identify courses that are over and should be closed.
              Hide
              lulrich Lorenz Ulrich added a comment -

              My +1 for this feature request for the same reason as B. Friesen. With a set end date it's much easier to do a date-based cleanup of courses.

              Show
              lulrich Lorenz Ulrich added a comment - My +1 for this feature request for the same reason as B. Friesen. With a set end date it's much easier to do a date-based cleanup of courses.
              Hide
              mileginy Mei Ling added a comment -

              why this new feature no more update and develop? Will it be included in version 2.6 route map ?

              Show
              mileginy Mei Ling added a comment - why this new feature no more update and develop? Will it be included in version 2.6 route map ?
              Hide
              georgi.samaras Georgi Samaras added a comment -

              I'm with B. Friesen and Lorenz Ulrich as well in that I'd like to see an End Date value to assist in upkeep regarding course closures.

              Show
              georgi.samaras Georgi Samaras added a comment - I'm with B. Friesen and Lorenz Ulrich as well in that I'd like to see an End Date value to assist in upkeep regarding course closures.
              Hide
              pferre22 Pau Ferrer added a comment -

              I would be interesting to add the end course date field to add advanced administration features.
              The cron can trigger an event that can be used by different modules like enrolment modules and local.

              Some actions can be done with that event:

              • Notify students that the course has ended
              • Syncronization with external databases (with grades)
              • Hide the course or make it not visible to students.
              • Clean up logs
              Show
              pferre22 Pau Ferrer added a comment - I would be interesting to add the end course date field to add advanced administration features. The cron can trigger an event that can be used by different modules like enrolment modules and local. Some actions can be done with that event: Notify students that the course has ended Syncronization with external databases (with grades) Hide the course or make it not visible to students. Clean up logs
              Hide
              marina Marina Glancy added a comment -

              In my opinion this can be resolved by introducing custom course fields (MDL-18319) plus some custom plugin that runs cron to check for the ending courses and executes the necessary actions.

              Show
              marina Marina Glancy added a comment - In my opinion this can be resolved by introducing custom course fields ( MDL-18319 ) plus some custom plugin that runs cron to check for the ending courses and executes the necessary actions.
              Hide
              bfriesen B. Friesen added a comment -

              I'd love to see some movement on this. There doesn't seem to be much happening with MDL-18319, either.

              I still don't understand why something so obvious to me (and a few others) as an end date isn't included by default. There has to be some logic that I'm missing, here.

              Show
              bfriesen B. Friesen added a comment - I'd love to see some movement on this. There doesn't seem to be much happening with MDL-18319 , either. I still don't understand why something so obvious to me (and a few others) as an end date isn't included by default. There has to be some logic that I'm missing, here.
              Hide
              pferre22 Pau Ferrer added a comment - - edited

              I've started working on that, but I don't have so much time.
              https://github.com/crazyserver/moodle/compare/MDL-22078_master

              Show
              pferre22 Pau Ferrer added a comment - - edited I've started working on that, but I don't have so much time. https://github.com/crazyserver/moodle/compare/MDL-22078_master
              Hide
              marina Marina Glancy added a comment -

              Hi Pau, thanks for working on it. Just to notice that any new field in course table should be included in backup/restore process, web services, course create/update functions, and so on.

              Show
              marina Marina Glancy added a comment - Hi Pau, thanks for working on it. Just to notice that any new field in course table should be included in backup/restore process, web services, course create/update functions, and so on.
              Hide
              pferre22 Pau Ferrer added a comment -

              Yes, thank you!

              Show
              pferre22 Pau Ferrer added a comment - Yes, thank you!
              Hide
              danielneis Daniel Neis Araujo added a comment -

              Hello,

              here is a patch to add this to course table as we already have start date,
              it seems that for "completeness" we can have end date.

              As custom course fields are too much complicated and not adopted yet,
              this is a simple version that might help people with reports and so.

              Hope that you like.

              Kind regards,
              Daniel

              Show
              danielneis Daniel Neis Araujo added a comment - Hello, here is a patch to add this to course table as we already have start date, it seems that for "completeness" we can have end date. As custom course fields are too much complicated and not adopted yet, this is a simple version that might help people with reports and so. Hope that you like. Kind regards, Daniel
              Hide
              cibot CiBoT added a comment -
              Show
              cibot CiBoT added a comment - Fails against automated checks. Checked MDL-22078 using repository: https://github.com/danielneis/moodle master (2 errors / 1 warnings) [branch: MDL-22078 | CI Job ] phplint (0/0) , php (2/1) , js (0/0) , css (0/0) , phpdoc (0/0) , commit (0/0) , savepoint (0/0) , thirdparty (0/0) , More information about this report
              Hide
              danielneis Daniel Neis Araujo added a comment -

              Have fixed cibot warnings and errors.

              Show
              danielneis Daniel Neis Araujo added a comment - Have fixed cibot warnings and errors.
              Hide
              pferre22 Pau Ferrer added a comment -

              Sounds great Daniel Neis Araujo!!

              I would add a handler to cron that, when reaching the enddate it triggers the event and modules can use it to work over that.

              Thanks!!!

              Show
              pferre22 Pau Ferrer added a comment - Sounds great Daniel Neis Araujo !! I would add a handler to cron that, when reaching the enddate it triggers the event and modules can use it to work over that. Thanks!!!
              Hide
              marina Marina Glancy added a comment -

              Re last comment from Pau - let's do it in a separate issue that is blocked by this one. As well as making weeks format respect the end date. At the moment this field does not make any visible changes to the UI but it's a good start.

              Daniel, your code looks pretty good in general, this is not yet a complete peer review yet but I noticed that you have added a folder course/format/onetopic which is unrelated (and breaks a lot as well because moodle is looking for plugin there).

              Also unittests fail in tool_uploadcourse, which reminded me that the end date is not supported there.

              But the failure itself indicates another problem: if course is restored from backup that DOES NOT contain enddate (backup made before upgrade) - there is a warning.

              I guess you have not run unittests or behat tests on your branch but I will recommend you to do it to make sure that nothing else pops up.

              TIA

              Show
              marina Marina Glancy added a comment - Re last comment from Pau - let's do it in a separate issue that is blocked by this one. As well as making weeks format respect the end date. At the moment this field does not make any visible changes to the UI but it's a good start. Daniel, your code looks pretty good in general, this is not yet a complete peer review yet but I noticed that you have added a folder course/format/onetopic which is unrelated (and breaks a lot as well because moodle is looking for plugin there). Also unittests fail in tool_uploadcourse, which reminded me that the end date is not supported there. But the failure itself indicates another problem: if course is restored from backup that DOES NOT contain enddate (backup made before upgrade) - there is a warning. I guess you have not run unittests or behat tests on your branch but I will recommend you to do it to make sure that nothing else pops up. TIA
              Hide
              marina Marina Glancy added a comment -

              Daniel, please add enddate in some unittests assertions, for example in tool_uploadcourse_course_testcase and core_course_courselib_testcase
              you don't need to create new tests, just add enddate to one of existing ones

              Show
              marina Marina Glancy added a comment - Daniel, please add enddate in some unittests assertions, for example in tool_uploadcourse_course_testcase and core_course_courselib_testcase you don't need to create new tests, just add enddate to one of existing ones
              Hide
              danielneis Daniel Neis Araujo added a comment -

              Hello, Marina

              thanks for your review! =)

              I agree that is just a simple start and it sould be. Let's handle another uses of this field in other issues =)

              Sorry for the onetopic formato, just removed that.

              I've added the enddate support to upload course tool.

              Also added some enddate related assertions to admin/tool/uploadcourse/tests/course_test.php and course/tests/externallib_test.php

              I did not found any mention to startdate on core_course_courselib_testcase so i've not added enddate there.

              I will fix the restore when when there is no enddate on backup file and let you know.

              Another thing that I was thinking about is that we are using date_selector to start and enddates.
              It would be better to have datetime_selector like MDL-42842, MDL-41173, MDL-42844 and so.
              We could use this issue to solve that, must not be much work.

              Kind regards,
              Daniel

              Show
              danielneis Daniel Neis Araujo added a comment - Hello, Marina thanks for your review! =) I agree that is just a simple start and it sould be. Let's handle another uses of this field in other issues =) Sorry for the onetopic formato, just removed that. I've added the enddate support to upload course tool. Also added some enddate related assertions to admin/tool/uploadcourse/tests/course_test.php and course/tests/externallib_test.php I did not found any mention to startdate on core_course_courselib_testcase so i've not added enddate there. I will fix the restore when when there is no enddate on backup file and let you know. Another thing that I was thinking about is that we are using date_selector to start and enddates. It would be better to have datetime_selector like MDL-42842 , MDL-41173 , MDL-42844 and so. We could use this issue to solve that, must not be much work. Kind regards, Daniel
              Hide
              cibot CiBoT added a comment -
              Show
              cibot CiBoT added a comment - Fails against automated checks. Checked MDL-22078 using repository: https://github.com/danielneis/moodle master (2 errors / 2 warnings) [branch: MDL-22078 | CI Job ] phplint (0/0) , php (2/2) , js (0/0) , css (0/0) , phpdoc (0/0) , commit (0/0) , savepoint (0/0) , thirdparty (0/0) , More information about this report
              Hide
              danielneis Daniel Neis Araujo added a comment - - edited

              Hello,

              I've fixed the restore with no enddate on backup file, and also made the field optional.

              Let me know if there is anything missing and if I should change the date_selector to date_time_selector.

              Kind regards,
              Daniel

              Show
              danielneis Daniel Neis Araujo added a comment - - edited Hello, I've fixed the restore with no enddate on backup file, and also made the field optional. Let me know if there is anything missing and if I should change the date_selector to date_time_selector. Kind regards, Daniel
              Hide
              cibot CiBoT added a comment -
              Show
              cibot CiBoT added a comment - Fails against automated checks. Checked MDL-22078 using repository: https://github.com/danielneis/moodle master (1 errors / 0 warnings) [branch: MDL-22078 | CI Job ] phplint (0/0) , php (0/0) , js (0/0) , css (0/0) , phpdoc (1/0) , commit (0/0) , savepoint (0/0) , thirdparty (0/0) , More information about this report
              Hide
              marina Marina Glancy added a comment -

              Thanks Daniel, either me or somebody else from hq will do a proper review soon.

              There is an issue about time in course start date - MDL-43648 , feel free to implement it under this ticket and we will close MDL-43648 when it's integrated.

              Show
              marina Marina Glancy added a comment - Thanks Daniel, either me or somebody else from hq will do a proper review soon. There is an issue about time in course start date - MDL-43648 , feel free to implement it under this ticket and we will close MDL-43648 when it's integrated.
              Hide
              marina Marina Glancy added a comment -

              uh-oh, Mark's in the hospital, I will assign it for peer review to somebody else shortly. Sorry for the delay!

              Show
              marina Marina Glancy added a comment - uh-oh, Mark's in the hospital, I will assign it for peer review to somebody else shortly. Sorry for the delay!
              Hide
              marina Marina Glancy added a comment -

              Sorry Daniel Neis Araujo, this issue is very unlucky. Andrew Nicols probably overlooked it and then went on holidays and now is on integration, unassigning from him

              Show
              marina Marina Glancy added a comment - Sorry Daniel Neis Araujo , this issue is very unlucky. Andrew Nicols probably overlooked it and then went on holidays and now is on integration, unassigning from him
              Hide
              danielneis Daniel Neis Araujo added a comment -

              Hello, Mark Nelson

              you had the chance to review it?

              Kind regards,
              Daniel

              Show
              danielneis Daniel Neis Araujo added a comment - Hello, Mark Nelson you had the chance to review it? Kind regards, Daniel
              Hide
              markn Mark Nelson added a comment -

              Hi Daniel - sorry. Will have it done in a couple of hours.

              Show
              markn Mark Nelson added a comment - Hi Daniel - sorry. Will have it done in a couple of hours.
              Hide
              markn Mark Nelson added a comment -

              Hi Daniel,

              Thanks for working on this. Sorry for the delay in the review.

              admin/tool/uploadcourse/classes/step2_form.php
              1. Why was it decided that the end date should be 10 days after the start date? This new setting should be disabled by default (a value of 0). I explain this further on.
              admin/tool/uploadcourse/cli/uploadcourse.php
              1. Same as above.
              backup/moodle2/restore_course_task.class.php
              1. No need to add that comment “// Added by MDL-22078 .” as git will show the history.
              2. I can see the if statement is used for BC for when an earlier backup won’t have the data - if that is the case a comment explaining that will do.
              3. We should not be setting the enddate here to 10 days after the start date if the enddate value is not present, but leave it at 0, so the enddate result is disabled by default. If we were to backup a course in 2.9 and then quickly restore it in 3.0 we may have no idea that this new setting was added (and may just skip over the settings when restoring - which I do myself) and then realise later that the date was set to 10 days in advance.
              backup/util/checks/restore_check.class.php
              1. I see you renamed $restore_controller to $restorecontroller to please CiBot, but in some circumstances he/she/it can be ignored. If it involves renaming an existing variable in the file then it is not necessary. Just letting you know for future sake.
              backup/util/helper/backup_general_helper.class.php
              1. Again, the comment listing the MDL issue # is not necessary. An actual explanation is preferable.
              2. After ‘original_course_enddate’ you have unnecessary white spaces before ‘=‘.
              course/edit_form.php
              1. When creating a course now, the ‘Course end date’ value is set to ‘Enabled’ by default. By default this should be disabled. This also means for users who are used to creating courses in older version of Moodle they don’t accidentally create a bunch of courses that only last 10 days as they were not familiar with the new setting. The only way of doing this with the date_selector form element is by not passing a default date - which is why I don’t think we should be using the magic 10 day number.
              lib/db/install.xml
              1. Why are we allowing false into the new field? It will either be 0 (not used) or a timestamp, correct?
              General

              I am not sure why the course end date is being introduced as far as I can tell it doesn’t actually prevent a user from entering the course. I can see this being useful if you only wanted your course accessible between a certain period, but if the end date doesn’t actually do anything I don’t see why we would want it. You can still create a course with an end date then have topics that exceed this end date! I think there needs to be some sort of agreed purpose for this new setting. IMO we should not allow users to enter a course if the time exceeds the end date - as well as not allowing users to create unlimited topics/sections if an end date is specified. However, I am no teacher so maybe there are more common usages.

              Regards,

              Mark

              Show
              markn Mark Nelson added a comment - Hi Daniel, Thanks for working on this. Sorry for the delay in the review. admin/tool/uploadcourse/classes/step2_form.php Why was it decided that the end date should be 10 days after the start date? This new setting should be disabled by default (a value of 0). I explain this further on. admin/tool/uploadcourse/cli/uploadcourse.php Same as above. backup/moodle2/restore_course_task.class.php No need to add that comment “// Added by MDL-22078 .” as git will show the history. I can see the if statement is used for BC for when an earlier backup won’t have the data - if that is the case a comment explaining that will do. We should not be setting the enddate here to 10 days after the start date if the enddate value is not present, but leave it at 0, so the enddate result is disabled by default. If we were to backup a course in 2.9 and then quickly restore it in 3.0 we may have no idea that this new setting was added (and may just skip over the settings when restoring - which I do myself) and then realise later that the date was set to 10 days in advance. backup/util/checks/restore_check.class.php I see you renamed $restore_controller to $restorecontroller to please CiBot, but in some circumstances he/she/it can be ignored. If it involves renaming an existing variable in the file then it is not necessary. Just letting you know for future sake. backup/util/helper/backup_general_helper.class.php Again, the comment listing the MDL issue # is not necessary. An actual explanation is preferable. After ‘original_course_enddate’ you have unnecessary white spaces before ‘=‘. course/edit_form.php When creating a course now, the ‘Course end date’ value is set to ‘Enabled’ by default. By default this should be disabled. This also means for users who are used to creating courses in older version of Moodle they don’t accidentally create a bunch of courses that only last 10 days as they were not familiar with the new setting. The only way of doing this with the date_selector form element is by not passing a default date - which is why I don’t think we should be using the magic 10 day number. lib/db/install.xml Why are we allowing false into the new field? It will either be 0 (not used) or a timestamp, correct? General I am not sure why the course end date is being introduced as far as I can tell it doesn’t actually prevent a user from entering the course. I can see this being useful if you only wanted your course accessible between a certain period, but if the end date doesn’t actually do anything I don’t see why we would want it. You can still create a course with an end date then have topics that exceed this end date! I think there needs to be some sort of agreed purpose for this new setting. IMO we should not allow users to enter a course if the time exceeds the end date - as well as not allowing users to create unlimited topics/sections if an end date is specified. However, I am no teacher so maybe there are more common usages. Regards, Mark
              Hide
              danielneis Daniel Neis Araujo added a comment - - edited

              Hello, Mark

              thanks for the review! No problem with the delay, I was just trying to get this for 3.0 =)

              I've made the changes you mentioned and rebased the branch to current master.

              Like Marina said, we'll just introduce the "concept" in this issue and make other (more visible) changes in other issues.
              https://tracker.moodle.org/browse/MDL-22078?focusedCommentId=363722&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-363722

              Other nice cases where it could be used is to show in calendar (course start dates already are?) and also on course listing and course enrol page, along with course summary and image =)

              Kind regards,
              Daniel

              Show
              danielneis Daniel Neis Araujo added a comment - - edited Hello, Mark thanks for the review! No problem with the delay, I was just trying to get this for 3.0 =) I've made the changes you mentioned and rebased the branch to current master. Like Marina said, we'll just introduce the "concept" in this issue and make other (more visible) changes in other issues. https://tracker.moodle.org/browse/MDL-22078?focusedCommentId=363722&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-363722 Other nice cases where it could be used is to show in calendar (course start dates already are?) and also on course listing and course enrol page, along with course summary and image =) Kind regards, Daniel
              Hide
              cibot CiBoT added a comment -
              Show
              cibot CiBoT added a comment - Fails against automated checks. Checked MDL-22078 using repository: https://github.com/danielneis/moodle master (2 errors / 2 warnings) [branch: MDL-22078 | CI Job ] phplint (0/0) , php (2/2) , js (0/0) , css (0/0) , phpdoc (0/0) , commit (0/0) , savepoint (0/0) , thirdparty (0/0) , More information about this report
              Hide
              cibot CiBoT added a comment -
              Show
              cibot CiBoT added a comment - Fails against automated checks. Checked MDL-22078 using repository: https://github.com/danielneis/moodle master (1 errors / 1 warnings) [branch: MDL-22078 | CI Job ] phplint (0/0) , php (1/1) , js (0/0) , css (0/0) , phpdoc (0/0) , commit (0/0) , savepoint (0/0) , thirdparty (0/0) , More information about this report
              Hide
              markn Mark Nelson added a comment -

              Thanks for the link to that comment Daniel. I have to say I am not sold on the idea of adding this setting then changing it's behaviour later - I can just imagine someone setting the course end date, then upgrading later and finding out later their course is behaving completely different because we decided to change the behaviour of the setting. What you have done right now is definitely a good foundation but I think in order to get this into core we have to decide what this setting actually does. I think it should at least limit the number of topics that are displayed and disallow users from entering the course after this time is exceeded - I am sure there are a lot of other things to consider as well.

              Show
              markn Mark Nelson added a comment - Thanks for the link to that comment Daniel. I have to say I am not sold on the idea of adding this setting then changing it's behaviour later - I can just imagine someone setting the course end date, then upgrading later and finding out later their course is behaving completely different because we decided to change the behaviour of the setting. What you have done right now is definitely a good foundation but I think in order to get this into core we have to decide what this setting actually does. I think it should at least limit the number of topics that are displayed and disallow users from entering the course after this time is exceeded - I am sure there are a lot of other things to consider as well.
              Hide
              marina Marina Glancy added a comment -

              ==CODEFREEZE3.0==

              This is a bulk message to all improvement issues that have either "Waiting for peer review" or "Peer review in progress" status at the moment of 3.0 code freeze.

              Please remember that peer review of this issue needs to be completed ASAP in order to be included in 3.0.

              Thank you!

              Show
              marina Marina Glancy added a comment - ==CODEFREEZE3.0== This is a bulk message to all improvement issues that have either "Waiting for peer review" or "Peer review in progress" status at the moment of 3.0 code freeze. Please remember that peer review of this issue needs to be completed ASAP in order to be included in 3.0. Thank you!
              Hide
              moodle.com moodle.com added a comment -

              Hello Daniel,

              Thank you for this patch. It is a good foundation for future functionality. This issue just requires some additional code to actually make use of this setting. Please leave the code that you have provided here. We need to decide how to use this setting. It would be useful if there was a forum discussion about this so that the community has some input as to how this setting would be used.

              Show
              moodle.com moodle.com added a comment - Hello Daniel, Thank you for this patch. It is a good foundation for future functionality. This issue just requires some additional code to actually make use of this setting. Please leave the code that you have provided here. We need to decide how to use this setting. It would be useful if there was a forum discussion about this so that the community has some input as to how this setting would be used.
              Hide
              danielneis Daniel Neis Araujo added a comment -

              Hello,

              there are some discussions on forum about related stuff:

              https://moodle.org/mod/forum/discuss.php?d=278470 (it mentions related issue MDL-48762)
              https://moodle.org/mod/forum/discuss.php?d=264420 (this is about enrol end date "versus" course end date)
              https://moodle.org/mod/forum/discuss.php?d=269818 (similar to above)
              https://moodle.org/mod/forum/discuss.php?d=208722 (similar to above)
              https://moodle.org/mod/forum/discuss.php?d=113095 (similar to above)
              https://moodle.org/mod/forum/discuss.php?d=217021 (similar to MDL-48762)
              https://moodle.org/mod/forum/discuss.php?d=156775 (similar to MDL-48762)
              https://moodle.org/mod/forum/discuss.php?d=156078 (similar to MDL-48762)
              https://moodle.org/mod/forum/discuss.php?d=158920 (this is from 2010)
              https://moodle.org/mod/forum/discuss.php?d=192975 (this is from 2011 and related to MDL-48762)
              https://moodle.org/mod/forum/discuss.php?d=195277 (it mentions course end date but is so vague it got no responses)

              I've just looked for the first five pages of a search for "Course end format" :
              https://moodle.org/mod/forum/search.php?search=%22Course%20End%20Date%22&id=5&perpage=10&page=4

              It seems that if we could integrate this issue along with MDL-48762 we would have a nice use of this field/feature.

              Kind regards,
              Daniel

              Show
              danielneis Daniel Neis Araujo added a comment - Hello, there are some discussions on forum about related stuff: https://moodle.org/mod/forum/discuss.php?d=278470 (it mentions related issue MDL-48762 ) https://moodle.org/mod/forum/discuss.php?d=264420 (this is about enrol end date "versus" course end date) https://moodle.org/mod/forum/discuss.php?d=269818 (similar to above) https://moodle.org/mod/forum/discuss.php?d=208722 (similar to above) https://moodle.org/mod/forum/discuss.php?d=113095 (similar to above) https://moodle.org/mod/forum/discuss.php?d=217021 (similar to MDL-48762 ) https://moodle.org/mod/forum/discuss.php?d=156775 (similar to MDL-48762 ) https://moodle.org/mod/forum/discuss.php?d=156078 (similar to MDL-48762 ) https://moodle.org/mod/forum/discuss.php?d=158920 (this is from 2010) https://moodle.org/mod/forum/discuss.php?d=192975 (this is from 2011 and related to MDL-48762 ) https://moodle.org/mod/forum/discuss.php?d=195277 (it mentions course end date but is so vague it got no responses) I've just looked for the first five pages of a search for "Course end format" : https://moodle.org/mod/forum/search.php?search=%22Course%20End%20Date%22&id=5&perpage=10&page=4 It seems that if we could integrate this issue along with MDL-48762 we would have a nice use of this field/feature. Kind regards, Daniel
              Hide
              dmonllao David Monllaó added a comment -

              Hi Daniel and all,

              I'm assigning this to me, I hope you don't mind. We see no movement in the last months and this is important for a few upcoming projects.

              Show
              dmonllao David Monllaó added a comment - Hi Daniel and all, I'm assigning this to me, I hope you don't mind. We see no movement in the last months and this is important for a few upcoming projects.
              Hide
              dmonllao David Monllaó added a comment -
              • As suggested by Marina we propose a new site setting to set the default course duration, this setting can by used by course formats to set the default course end date (course start + duration setting)
              • Course formats can overwrite the default course end date. E.g. weeks course format sets it to course start date + (numsections * week duration)
              • In any case the course end date can be set by the teacher as the course start date, all the stuff above is just to set the default value
              • Course access is not restricted after the course end date passes, nothing happens actually
              • Can someone please give an opinion about "The end date must be after start date." sounds weird to me, but I am not the best person to opine about EN lang strings...
              • I've enabled the setting by default in new courses, IMO there is no harm in enabling it and we want to encourage people to set an end date because it will help with reporting and other stuff
              • I don't like $fieldnames argument, happy if anyone can think of an alternative
              • I've included new requires to course/lib.php in a few pages, it was included anyway in most of moodle pages by navigation, I don't think I'm including any new file in any performance-critical file.
              Show
              dmonllao David Monllaó added a comment - As suggested by Marina we propose a new site setting to set the default course duration, this setting can by used by course formats to set the default course end date (course start + duration setting) Course formats can overwrite the default course end date. E.g. weeks course format sets it to course start date + (numsections * week duration) In any case the course end date can be set by the teacher as the course start date, all the stuff above is just to set the default value Course access is not restricted after the course end date passes, nothing happens actually Can someone please give an opinion about "The end date must be after start date." sounds weird to me, but I am not the best person to opine about EN lang strings... I've enabled the setting by default in new courses, IMO there is no harm in enabling it and we want to encourage people to set an end date because it will help with reporting and other stuff I don't like $fieldnames argument, happy if anyone can think of an alternative I've included new requires to course/lib.php in a few pages, it was included anyway in most of moodle pages by navigation, I don't think I'm including any new file in any performance-critical file.
              Hide
              dmonllao David Monllaó added a comment -

              Raising priority as this is important for analytics and a MUA potential project.

              Show
              dmonllao David Monllaó added a comment - Raising priority as this is important for analytics and a MUA potential project.
              Hide
              cibot CiBoT added a comment -
              Show
              cibot CiBoT added a comment - Fails against automated checks. Checked MDL-22078 using repository: https://github.com/dmonllao/moodle master (13 errors / 7 warnings) [branch: MDL-22078_master | CI Job ] phplint (0/0) , phpcs (6/5) , js (0/1) , css (0/0) , phpdoc (5/0) , commit (2/1) , savepoint (0/0) , thirdparty (0/0) , grunt (0/0) , shifter (0/0) , travis (0/0) , Should these errors be fixed?
              Hide
              mr-russ Russell Smith added a comment -

              Hi,

              I thought I've have an initial look at this. It's more a details review rather than concept. I think the approach is find and there aren't really any choices about that;

              https://github.com/dmonllao/moodle/compare/MDL-22078_master~4...MDL-22078_master#diff-7d09b68416e38b8d3c34f3da3722fa81R924
              Why no $resetdata->reset_end_date_old?

              https://github.com/dmonllao/moodle/compare/MDL-22078_master~4...MDL-22078_master#diff-338abac133b36377201a18fb90d09d62R207
              Should you use the DAYSECS contants?

              https://github.com/dmonllao/moodle/compare/MDL-22078_master~4...MDL-22078_master#diff-5c217e6ca009bf0fc8e32bf1d8eafbeeR343
              This feels like a behaviour change because of the binding of and within an if. Even if it's still correct, it's difficult to understand.

              https://github.com/dmonllao/moodle/compare/MDL-22078_master~4...MDL-22078_master#diff-5c217e6ca009bf0fc8e32bf1d8eafbeeR359
              I don't understand the change here, It doesn't seem to impact end date, but I'm sure it does something. Can you explain?

              https://github.com/dmonllao/moodle/compare/MDL-22078_master~4...MDL-22078_master#diff-3c138d17b4b1dd855388d0542d90030fR387
              Is this a bugfix to startdate rather than required for enddate?

              Those are a few small things. I'm sure I'll have other feedback as I get into understanding what's going on. Thanks for your work on this. It is a feature I personally would have liked for a long time. I ended up building it into a course format in the interim.

              Show
              mr-russ Russell Smith added a comment - Hi, I thought I've have an initial look at this. It's more a details review rather than concept. I think the approach is find and there aren't really any choices about that; https://github.com/dmonllao/moodle/compare/MDL-22078_master~4...MDL-22078_master#diff-7d09b68416e38b8d3c34f3da3722fa81R924 Why no $resetdata->reset_end_date_old? https://github.com/dmonllao/moodle/compare/MDL-22078_master~4...MDL-22078_master#diff-338abac133b36377201a18fb90d09d62R207 Should you use the DAYSECS contants? https://github.com/dmonllao/moodle/compare/MDL-22078_master~4...MDL-22078_master#diff-5c217e6ca009bf0fc8e32bf1d8eafbeeR343 This feels like a behaviour change because of the binding of and within an if . Even if it's still correct, it's difficult to understand. https://github.com/dmonllao/moodle/compare/MDL-22078_master~4...MDL-22078_master#diff-5c217e6ca009bf0fc8e32bf1d8eafbeeR359 I don't understand the change here, It doesn't seem to impact end date, but I'm sure it does something. Can you explain? https://github.com/dmonllao/moodle/compare/MDL-22078_master~4...MDL-22078_master#diff-3c138d17b4b1dd855388d0542d90030fR387 Is this a bugfix to startdate rather than required for enddate? Those are a few small things. I'm sure I'll have other feedback as I get into understanding what's going on. Thanks for your work on this. It is a feature I personally would have liked for a long time. I ended up building it into a course format in the interim.
              Hide
              dmonllao David Monllaó added a comment -

              Thanks for looking at the patch Russell, I've updated it (also making cibot happier)

              1.- reset_start_date_old is used to calculate the timeshift but there is no shift if end date changes, I don't see any reason to store the previous end date
              2.- I would have used it if would be new code
              3.- Wow yes, this is completely wrong, thanks
              4.- Sure, I've added a comment in the code. Course formats can overwrite the method that sets the default end date, if we are loading an existing course settings page, the course format may be interested in fetching other data related with that course to work out the best default end date. They would need to overwrite create_edit_form_elements as well (empty($this->courseid), as by default we protect existing courses enddate value. Looking at this again I am not 100% sure it is a good idea to give them the power to overwrite the course setting... I will think about it for a while, I may end up removing it if I can't think about a good use case.
              5.- I've added the new argument to reuse the function (for enddate calculation) When now call this function even when the format is not linked to any course.

              Show
              dmonllao David Monllaó added a comment - Thanks for looking at the patch Russell, I've updated it (also making cibot happier) 1.- reset_start_date_old is used to calculate the timeshift but there is no shift if end date changes, I don't see any reason to store the previous end date 2.- I would have used it if would be new code 3.- Wow yes, this is completely wrong, thanks 4.- Sure, I've added a comment in the code. Course formats can overwrite the method that sets the default end date, if we are loading an existing course settings page, the course format may be interested in fetching other data related with that course to work out the best default end date. They would need to overwrite create_edit_form_elements as well (empty($this->courseid), as by default we protect existing courses enddate value. Looking at this again I am not 100% sure it is a good idea to give them the power to overwrite the course setting... I will think about it for a while, I may end up removing it if I can't think about a good use case. 5.- I've added the new argument to reuse the function (for enddate calculation) When now call this function even when the format is not linked to any course.
              Hide
              cibot CiBoT added a comment -

              Fails against automated checks.

              Checked MDL-22078 using repository: https://github.com/dmonllao/moodle

              Should these errors be fixed?

              Show
              cibot CiBoT added a comment - Fails against automated checks. Checked MDL-22078 using repository: https://github.com/dmonllao/moodle master [branch: MDL-22078_master | CI Job ] Error: The MDL-22078 _master branch at https://github.com/dmonllao/moodle does not apply clean to origin/master Should these errors be fixed?
              Hide
              dmonllao David Monllaó added a comment -

              that was the weeklies release a few hours ago.

              Show
              dmonllao David Monllaó added a comment - that was the weeklies release a few hours ago.
              Hide
              cibot CiBoT added a comment -
              Show
              cibot CiBoT added a comment - Fails against automated checks. Checked MDL-22078 using repository: https://github.com/dmonllao/moodle master (3 errors / 6 warnings) [branch: MDL-22078_master | CI Job ] phplint (0/0) , phpcs (1/5) , js (0/0) , css (0/0) , phpdoc (0/0) , commit (2/1) , savepoint (0/0) , thirdparty (0/0) , grunt (0/0) , shifter (0/0) , travis (0/0) , Should these errors be fixed?
              Hide
              mr-russ Russell Smith added a comment -

              ci bot is still pretty unhappy. However most of those changes are fine issues. I consider fixing things like variable names when I alter code like this, otherwise you just don't ever get to the point of making the old code new. I know there are refactoring risks, but unit tests and behat tests are supposed to raise our refactoring confidence. IDE's usually get it right too

              With my (1). You are correct that end date can't be set. So it doesn't need addition there. It uses start date difference to timeshift everything when it calls reset_course_userdata(). However I disagree with the comment at https://github.com/dmonllao/moodle/compare/MDL-22078_master~4...MDL-22078_master#diff-d46e32d459d55f2123253fbc22108fc7R5125 I believe that if you alter the start date, you want the end date rolled forward when resetting the course. As a user I would normally know when the next iteration of the course starts and expect it to run for the same period of time. However I don't know what that is. Intuitively I'd expect end date to roll forward as well. The current implementation uses that strategy on restore rather than the select and end date. I would see keeping them consistent as very important. It's quite easy to adjust the end date in the edit screen if required. I'm not against either method in total. I just feel strongly that they should be the same in each case. Others opinions may differ. Anybody else have a view on end date rolling forward during course reset and course end date on restore?

              Show
              mr-russ Russell Smith added a comment - ci bot is still pretty unhappy. However most of those changes are fine issues. I consider fixing things like variable names when I alter code like this, otherwise you just don't ever get to the point of making the old code new. I know there are refactoring risks, but unit tests and behat tests are supposed to raise our refactoring confidence. IDE's usually get it right too With my (1). You are correct that end date can't be set. So it doesn't need addition there. It uses start date difference to timeshift everything when it calls reset_course_userdata(). However I disagree with the comment at https://github.com/dmonllao/moodle/compare/MDL-22078_master~4...MDL-22078_master#diff-d46e32d459d55f2123253fbc22108fc7R5125 I believe that if you alter the start date, you want the end date rolled forward when resetting the course. As a user I would normally know when the next iteration of the course starts and expect it to run for the same period of time. However I don't know what that is. Intuitively I'd expect end date to roll forward as well. The current implementation uses that strategy on restore rather than the select and end date. I would see keeping them consistent as very important. It's quite easy to adjust the end date in the edit screen if required. I'm not against either method in total. I just feel strongly that they should be the same in each case. Others opinions may differ. Anybody else have a view on end date rolling forward during course reset and course end date on restore?
              Hide
              mr-russ Russell Smith added a comment -

              I had a bit more of a think about testing;

              In terms of a starting point for possible backup/restore testing, backup/moodle2/tests/moodle2_course_format_test.php has examples of how course_format_options backup/restore was tested. And backup/moodle2/tests/moodle2_test.php has a function test_restore_dates() which appears to be testing what you are changing. You've done a good job of covering reset and the upload tool. But adding tests to backup restore would really cover some of the scenarios in your test plan more automatically.

              Show
              mr-russ Russell Smith added a comment - I had a bit more of a think about testing; In terms of a starting point for possible backup/restore testing, backup/moodle2/tests/moodle2_course_format_test.php has examples of how course_format_options backup/restore was tested. And backup/moodle2/tests/moodle2_test.php has a function test_restore_dates() which appears to be testing what you are changing. You've done a good job of covering reset and the upload tool. But adding tests to backup restore would really cover some of the scenarios in your test plan more automatically.
              Hide
              dmonllao David Monllaó added a comment -

              Thanks for looking at it again Russell.

              We discourage people from changing var names in cases like this, the reasons are the ones you explain + noise for the code reviewer, it can be acceptable though if the amount of code that is modified by a patch is a big portion of the var references.

              I don't have strong feelings towards shifting end date or not so I've did shifted it, only when the user didn't set any reset end date and the _old end date was set

              Yeah, forgot about backup/restore testing I've added some tests there, some extra tests to format_weeks and to format_topics

              Show
              dmonllao David Monllaó added a comment - Thanks for looking at it again Russell. We discourage people from changing var names in cases like this, the reasons are the ones you explain + noise for the code reviewer, it can be acceptable though if the amount of code that is modified by a patch is a big portion of the var references. I don't have strong feelings towards shifting end date or not so I've did shifted it, only when the user didn't set any reset end date and the _old end date was set Yeah, forgot about backup/restore testing I've added some tests there, some extra tests to format_weeks and to format_topics
              Hide
              emdalton1 Elizabeth Dalton added a comment -

              I don't think course formats should change the end date, but an alert could be shown if someone edits the course format and the end date doesn't appear to match.

              Real life use case: the college I worked at officially started courses on Monday and officially ended on Friday 12 weeks later (i.e. courses were 12 weeks minus 2 days long). Rather than having the Weeks format change the course end date, I would rather see the Weeks format reflect the course end date as set, possibly highlighting it if it creates a "short" week.

              Show
              emdalton1 Elizabeth Dalton added a comment - I don't think course formats should change the end date, but an alert could be shown if someone edits the course format and the end date doesn't appear to match. Real life use case: the college I worked at officially started courses on Monday and officially ended on Friday 12 weeks later (i.e. courses were 12 weeks minus 2 days long). Rather than having the Weeks format change the course end date, I would rather see the Weeks format reflect the course end date as set, possibly highlighting it if it creates a "short" week.
              Show
              dmonllao David Monllaó added a comment - Course formats are not changing the end date ( https://tracker.moodle.org/browse/MDL-22078?focusedCommentId=424895&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-424895 -> #2)
              Hide
              emdalton1 Elizabeth Dalton added a comment -

              David, I know we talked about this, but I'm still a bit confused by what this means:

              "Course formats can overwrite the default course end date. E.g. weeks course format sets it to course start date + (numsections * week duration)"

              If I have created a course, and I have set the start date to Feb 1 and the end date to Feb 18, and I change the format to Weeks, what will happen to the end date? How will the last section display dates in this case?

              Also, will third party course formats, like Collapsed Topics, need to change code to deal with the course end date?

              Show
              emdalton1 Elizabeth Dalton added a comment - David, I know we talked about this, but I'm still a bit confused by what this means: "Course formats can overwrite the default course end date. E.g. weeks course format sets it to course start date + (numsections * week duration)" If I have created a course, and I have set the start date to Feb 1 and the end date to Feb 18, and I change the format to Weeks, what will happen to the end date? How will the last section display dates in this case? Also, will third party course formats, like Collapsed Topics, need to change code to deal with the course end date?
              Hide
              dmonllao David Monllaó added a comment -

              I'm talking about course formats API and default end dates (default = when there is no value):

              • If I have created a course, and I have set the start date to Feb 1 and the end date to Feb 18, and I change the format to Weeks, what will happen to the end date? How will the last section display dates in this case? You already set a date and, again, course formats only define the default value when there is no value and the course is new. This patch is not changing how the last section displays dates
              • Also, will third party course formats, like Collapsed Topics, need to change code to deal with the course end date? https://tracker.moodle.org/browse/MDL-22078?focusedCommentId=424895&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-424895 -> #1. They don't need to change anything, they can change it if they want though. Imagine a format_months format, they don't need to change the default course start + duration value, but they may want to change it to start date + (number of months * month duration)
              Show
              dmonllao David Monllaó added a comment - I'm talking about course formats API and default end dates (default = when there is no value): If I have created a course, and I have set the start date to Feb 1 and the end date to Feb 18, and I change the format to Weeks, what will happen to the end date? How will the last section display dates in this case? You already set a date and, again, course formats only define the default value when there is no value and the course is new. This patch is not changing how the last section displays dates Also, will third party course formats, like Collapsed Topics, need to change code to deal with the course end date? https://tracker.moodle.org/browse/MDL-22078?focusedCommentId=424895&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-424895 -> #1. They don't need to change anything, they can change it if they want though. Imagine a format_months format, they don't need to change the default course start + duration value, but they may want to change it to start date + (number of months * month duration)
              Hide
              emdalton1 Elizabeth Dalton added a comment -

              Thanks for your patience in explaining it to me.

              Show
              emdalton1 Elizabeth Dalton added a comment - Thanks for your patience in explaining it to me.
              Hide
              lameze Simey Lameze added a comment -

              Hi David, nice work! Just saw this issue is waiting for a peer review for a while so I've decided to review it.

              I've tried to pull the patch in and it is conflicting badly, any chance of fix those conflicts so I can play with the feature?

              I'm gonna review by the diff meanwhile.

              Thanks!

              Show
              lameze Simey Lameze added a comment - Hi David, nice work! Just saw this issue is waiting for a peer review for a while so I've decided to review it. I've tried to pull the patch in and it is conflicting badly, any chance of fix those conflicts so I can play with the feature? I'm gonna review by the diff meanwhile. Thanks!
              Hide
              dmonllao David Monllaó added a comment -

              Thanks Simey. Let me rebase it, 5mins please

              Show
              dmonllao David Monllaó added a comment - Thanks Simey. Let me rebase it, 5mins please
              Hide
              dmonllao David Monllaó added a comment -

              By the way, this issue is blocked by MDL-55804, which patch is quite smaller than this issue's one, feel free to pick it for review if you are keen to it. I will rebase both issues (note that this issue's patch includes MDL-55804 one)

              Show
              dmonllao David Monllaó added a comment - By the way, this issue is blocked by MDL-55804 , which patch is quite smaller than this issue's one, feel free to pick it for review if you are keen to it. I will rebase both issues (note that this issue's patch includes MDL-55804 one)
              Hide
              dmonllao David Monllaó added a comment -

              Sorry for the delay, I had to leave for a while and this issue is a pain to rebase.

              Show
              dmonllao David Monllaó added a comment - Sorry for the delay, I had to leave for a while and this issue is a pain to rebase.
              Hide
              lameze Simey Lameze added a comment -

              Hi David, I've started to play with the patch but there is an issue with the current patch.

              I have an existing course, so I've enabled course end date field set a date in the future and saved. When I edited the course again, the course end field was not setting the date I have choose and it resets to the default date.

              Thanks.

              Show
              lameze Simey Lameze added a comment - Hi David, I've started to play with the patch but there is an issue with the current patch. I have an existing course, so I've enabled course end date field set a date in the future and saved. When I edited the course again, the course end field was not setting the date I have choose and it resets to the default date. Thanks.
              Hide
              lameze Simey Lameze added a comment - - edited

              Thanks for work on this David, nice work.

              I have reviewed your patch, overall it looks good I just have some comments/suggestions:

              1. One point that got my attention is course_validate_dates(), that method does the job although I have a suggestion.
                Have you thought about create a generic validate method, so it can be used to validate other course data not just start and end date?
                It's a pity we do not have a core_course at the moment, otherwise I would suggest create a static method (\core_course::validate()) that could be called from anywhere.
                If you decide to go ahead with the present solution, I would suggest throw exceptions inside that method, not return the lang string name as it is now.
              2. Incorrect phpDoc here, I guess the correct is test_course_dates_reset, also I would suggest improve that comment block by adding description to those parameters.
              3. Why not use $this->setExpectedException instead of this try/catch block here?
              4. Missing parameter declaration here
              5. Another suggestion is to ask Helen to rewrite some of the language strings, even though the course end date won't prevent the course to be accessed I still think that we could come up with a better description for:
                • Default course duration, used to set the default course end date.
                • ..."of the course. Nothing special happens, but you can use on your reports and plugins."
              6. I've managed to set courseduration to 0 without any problem, is that correct? And when I tried to set duration to a big number (99999999999999) and saved, it automatically converted the days to (3458764513820540928). I think we could do a proper validation there, for instance I tried to use letters on the setting and didn't get any error.
              7. Will course end field enabled by default during the course creation?

              Overall I think the solution is good, I think a bit funny the concept of a course end date that nothing special happens but I know there are other reasons to do so and this was widely discussed already. As you pointed, it is important this feature land on 3.2 so it can be used by other projects so you have my +1 to send for integration review. So you have enough if there are other changes to be made.

              Thanks

              Show
              lameze Simey Lameze added a comment - - edited Thanks for work on this David, nice work. I have reviewed your patch, overall it looks good I just have some comments/suggestions: One point that got my attention is course_validate_dates() , that method does the job although I have a suggestion. Have you thought about create a generic validate method, so it can be used to validate other course data not just start and end date? It's a pity we do not have a core_course at the moment, otherwise I would suggest create a static method (\core_course::validate()) that could be called from anywhere. If you decide to go ahead with the present solution, I would suggest throw exceptions inside that method, not return the lang string name as it is now. Incorrect phpDoc here , I guess the correct is test_course_dates_reset , also I would suggest improve that comment block by adding description to those parameters. Why not use $this->setExpectedException instead of this try/catch block here ? Missing parameter declaration here Another suggestion is to ask Helen to rewrite some of the language strings, even though the course end date won't prevent the course to be accessed I still think that we could come up with a better description for: Default course duration, used to set the default course end date. ..."of the course. Nothing special happens, but you can use on your reports and plugins." I've managed to set courseduration to 0 without any problem, is that correct? And when I tried to set duration to a big number (99999999999999) and saved, it automatically converted the days to (3458764513820540928). I think we could do a proper validation there, for instance I tried to use letters on the setting and didn't get any error. Will course end field enabled by default during the course creation? Overall I think the solution is good, I think a bit funny the concept of a course end date that nothing special happens but I know there are other reasons to do so and this was widely discussed already. As you pointed, it is important this feature land on 3.2 so it can be used by other projects so you have my +1 to send for integration review. So you have enough if there are other changes to be made. Thanks
              Hide
              dmonllao David Monllaó added a comment -

              Thanks for the review Simey.

              1.- I agree it would be nice, it is out of the scope of this issue though
              2.- Updated, thanks
              3.- The test is using a data provider, using try & catch is more flexible as the test changes depending on the inputs
              4.- Updated, thanks
              5.- Yeah, agree it would be very nice to have Helen Foster opinion about the new strings. These are error strings (https://github.com/dmonllao/moodle/compare/MDL-22078_master~4...MDL-22078_master#diff-199e52e52aca608308c4643a27490f9fR226) and these (https://github.com/dmonllao/moodle/compare/MDL-22078_master~4...MDL-22078_master#diff-2e136085effc02d3439b70caf8fb97c9R711) are the two new settings strings. Helen, 'courseend' is a new course field and 'courseduration' is a site setting to automatically populate the course end with a default value. Ping me if you have any doubts and thanks in advance.
              6.- I am using admin_setting_configduration, I agree ideally we would control the max value over what is reasonable (I would say that the 0 is not necessarily wrong) but it is a separate issue, all admin_setting_configduration settings are affected by this, they still trigger an error if a very high value is set (tried in postgres)
              7.- Yes it will, we want to encourage people to use this setting as it is quite important for 2 projects in the roadmap, users progress and analytics

              Show
              dmonllao David Monllaó added a comment - Thanks for the review Simey. 1.- I agree it would be nice, it is out of the scope of this issue though 2.- Updated, thanks 3.- The test is using a data provider, using try & catch is more flexible as the test changes depending on the inputs 4.- Updated, thanks 5.- Yeah, agree it would be very nice to have Helen Foster opinion about the new strings. These are error strings ( https://github.com/dmonllao/moodle/compare/MDL-22078_master~4...MDL-22078_master#diff-199e52e52aca608308c4643a27490f9fR226 ) and these ( https://github.com/dmonllao/moodle/compare/MDL-22078_master~4...MDL-22078_master#diff-2e136085effc02d3439b70caf8fb97c9R711 ) are the two new settings strings. Helen, 'courseend' is a new course field and 'courseduration' is a site setting to automatically populate the course end with a default value. Ping me if you have any doubts and thanks in advance. 6.- I am using admin_setting_configduration, I agree ideally we would control the max value over what is reasonable (I would say that the 0 is not necessarily wrong) but it is a separate issue, all admin_setting_configduration settings are affected by this, they still trigger an error if a very high value is set (tried in postgres) 7.- Yes it will, we want to encourage people to use this setting as it is quite important for 2 projects in the roadmap, users progress and analytics
              Hide
              dmonllao David Monllaó added a comment -

              I will wait a few days before sending this to integration to see if Helen can provide some feedback, in any case it is only master and we would have time later to update the lang strings if there is something wrong.

              Show
              dmonllao David Monllaó added a comment - I will wait a few days before sending this to integration to see if Helen can provide some feedback, in any case it is only master and we would have time later to update the lang strings if there is something wrong.
              Hide
              marina Marina Glancy added a comment -

              David, should we send it for integration today? Otherwise it will miss this week cycle

              Show
              marina Marina Glancy added a comment - David, should we send it for integration today? Otherwise it will miss this week cycle
              Hide
              tsala Helen Foster added a comment -

              Thanks David for chatting with me about this issue. Just noting that we agreed that it would be confusing if the course end date changed automatically according to the number of sections for a course in weekly format.

              A better solution would be for the course end date setting to only be changeable manually and have a warning message displayed when the course end date doesn't match the number of sections and perhaps suggesting the correct course end date.

              Show
              tsala Helen Foster added a comment - Thanks David for chatting with me about this issue. Just noting that we agreed that it would be confusing if the course end date changed automatically according to the number of sections for a course in weekly format. A better solution would be for the course end date setting to only be changeable manually and have a warning message displayed when the course end date doesn't match the number of sections and perhaps suggesting the correct course end date.
              Hide
              tsala Helen Foster added a comment -

              Thanks for asking me to look at the new language strings. I have a few small suggested improvements as follows:

              For enddatebeforestartdate

              'The course end date must be after the start date.'

              For nostartdatenoenddate

              'A course end date can only be set if a start date is also set.'

              For enddate_help:

              'The course end date is only used for reports. Users can still enter the course after the end date.'

              Just noting that when further developments are made which utilise the course end date, we can change the enddate_help text to mention them.

              Also noting (for the docs) that logs are still available after the course end date.

              For courseduration_desc

              'The course duration is used to calculate the default course end date. The course end date is only used for reports. Users can still enter the course after the end date.'

              Just noting that the courseduration is a weird setting, since it only appears at site level and not at course level.

              Show
              tsala Helen Foster added a comment - Thanks for asking me to look at the new language strings. I have a few small suggested improvements as follows: For enddatebeforestartdate 'The course end date must be after the start date.' For nostartdatenoenddate 'A course end date can only be set if a start date is also set.' For enddate_help: 'The course end date is only used for reports. Users can still enter the course after the end date.' Just noting that when further developments are made which utilise the course end date, we can change the enddate_help text to mention them. Also noting (for the docs) that logs are still available after the course end date. For courseduration_desc 'The course duration is used to calculate the default course end date. The course end date is only used for reports. Users can still enter the course after the end date.' Just noting that the courseduration is a weird setting, since it only appears at site level and not at course level.
              Hide
              marina Marina Glancy added a comment -

              Hi David,
              you asked me on Friday about my opinion on how to display a warning for not matching end date in topic_weeks. I think we should not do it inside the course edit form - first of all, as you said yourself, there is no "warnings" in forms api and you it's not quite clear how to add them. Besides, it will not help when teacher changes the number of sections by clicking = / - buttons in the bottom of the course, deleting sections, restoring another course into current, etc.

              I would suggest to add a warning (visible only to users with capability to edit course settings) when they view the course page. Something like "Course end date is set to X but it should be Y based on the start date Z and duration of N weeks. Adjust"
              And a quick action link to fix the end date without filling the form.

              The only question is when to display this warning - every time when teacher views the page or only after the actions that led to this situation (editing settings, calling course/changenumsections.php, deleting sections, restoring). There could be situations when the teacher decides to set end date different on purpose. What do you think?

              Show
              marina Marina Glancy added a comment - Hi David, you asked me on Friday about my opinion on how to display a warning for not matching end date in topic_weeks . I think we should not do it inside the course edit form - first of all, as you said yourself, there is no "warnings" in forms api and you it's not quite clear how to add them. Besides, it will not help when teacher changes the number of sections by clicking = / - buttons in the bottom of the course, deleting sections, restoring another course into current, etc. I would suggest to add a warning (visible only to users with capability to edit course settings) when they view the course page. Something like "Course end date is set to X but it should be Y based on the start date Z and duration of N weeks. Adjust " And a quick action link to fix the end date without filling the form. The only question is when to display this warning - every time when teacher views the page or only after the actions that led to this situation (editing settings, calling course/changenumsections.php, deleting sections, restoring). There could be situations when the teacher decides to set end date different on purpose. What do you think?
              Hide
              marina Marina Glancy added a comment -

              P.S. This is a small addition for individual format (format_weeks) and it should not block this issue and can be done as a separate one. Code freeze is too soon, you really need to push this code for integration review

              Show
              marina Marina Glancy added a comment - P.S. This is a small addition for individual format (format_weeks) and it should not block this issue and can be done as a separate one. Code freeze is too soon, you really need to push this code for integration review
              Hide
              cibot CiBoT added a comment -
              Show
              cibot CiBoT added a comment - Fails against automated checks. Checked MDL-22078 using repository: https://github.com/dmonllao/moodle master (6 errors / 8 warnings) [branch: MDL-22078_master | CI Job ] phplint (0/0) , phpcs (3/7) , js (0/0) , css (0/0) , phpdoc (3/1) , commit (0/0) , savepoint (0/0) , thirdparty (0/0) , grunt (0/0) , shifter (0/0) , travis (0/0) , Should these errors be fixed?
              Hide
              cibot CiBoT added a comment -
              Show
              cibot CiBoT added a comment - Fails against automated checks. Checked MDL-22078 using repository: https://github.com/dmonllao/moodle master (1 errors / 7 warnings) [branch: MDL-22078_master | CI Job ] phplint (0/0) , phpcs (1/7) , js (0/0) , css (0/0) , phpdoc (0/0) , commit (0/0) , savepoint (0/0) , thirdparty (0/0) , grunt (0/0) , shifter (0/0) , travis (0/0) , Should these errors be fixed?
              Hide
              dmonllao David Monllaó added a comment -

              Thanks all for commenting

              Helen, I've updated the language strings according to your suggestions, I agree with your concerns about how the order users follow to set a new course settings may lead to the course end date not matching the calculated end date (dependant on the course format) I also agree with Marina that we can move the discussio and solution to a separate issue as we are close to the code freeze and there are many UI options to solve this lack of consistency. I removed though the JS hack I added to reset the end date value when the course format changes (was applied to new courses only), so we always respect what the user has seen when going through the form.

              Show
              dmonllao David Monllaó added a comment - Thanks all for commenting Helen, I've updated the language strings according to your suggestions, I agree with your concerns about how the order users follow to set a new course settings may lead to the course end date not matching the calculated end date (dependant on the course format) I also agree with Marina that we can move the discussio and solution to a separate issue as we are close to the code freeze and there are many UI options to solve this lack of consistency. I removed though the JS hack I added to reset the end date value when the course format changes (was applied to new courses only), so we always respect what the user has seen when going through the form.
              Hide
              dmonllao David Monllaó added a comment -

              Waiting for the integrator opinion to create the follow up issue.

              Show
              dmonllao David Monllaó added a comment - Waiting for the integrator opinion to create the follow up issue.
              Hide
              cibot CiBoT added a comment -
              Show
              cibot CiBoT added a comment - Fails against automated checks. Checked MDL-22078 using repository: https://github.com/dmonllao/moodle master (1 errors / 7 warnings) [branch: MDL-22078_master | CI Job ] phplint (0/0) , phpcs (1/7) , js (0/0) , css (0/0) , phpdoc (0/0) , commit (0/0) , savepoint (0/0) , thirdparty (0/0) , grunt (0/0) , shifter (0/0) , travis (0/0) , Should these errors be fixed?
              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!
              Hide
              cibot CiBoT added a comment -
              Show
              cibot CiBoT added a comment - Fails against automated checks. Checked MDL-22078 using repository: https://github.com/dmonllao/moodle master (1 errors / 7 warnings) [branch: MDL-22078_master | CI Job ] phplint (0/0) , phpcs (1/7) , js (0/0) , css (0/0) , phpdoc (0/0) , commit (0/0) , savepoint (0/0) , thirdparty (0/0) , grunt (0/0) , shifter (0/0) , travis (0/0) , Should these errors be fixed?
              Hide
              mr-russ Russell Smith added a comment -

              Hi,

              I've just been looking at this for a brief review of the diff again and test instructions. I think course_duplicate_course() webservice should get the same treatment as the backup/restore functions. Or is it covered enough that it just needs calling once to ensure it's activating the end_date code? It may not specifically need testing at all if we understand that interactive backup/restore does all of the same functions. I wouldn't like to miss it if needed.

              Show
              mr-russ Russell Smith added a comment - Hi, I've just been looking at this for a brief review of the diff again and test instructions. I think course_duplicate_course() webservice should get the same treatment as the backup/restore functions. Or is it covered enough that it just needs calling once to ensure it's activating the end_date code? It may not specifically need testing at all if we understand that interactive backup/restore does all of the same functions. I wouldn't like to miss it if needed.
              Hide
              mr-russ Russell Smith added a comment -

              I decided to to a little bit more review on the course webservices and duplicate course doesn't take any of those parameters, but some of the others do. And the modifications don't appear in the test instruction list. They should be relatively simple tests additions, but I think key to ensure it's all correct.

              I would expect duplicate_course followed by get_courses(newid) and then update_course would be a likely common roll forward strategy.

              Sorry it's coming in little bits, I only have access to a mobile device and it's not as easy to cover everything.

              Show
              mr-russ Russell Smith added a comment - I decided to to a little bit more review on the course webservices and duplicate course doesn't take any of those parameters, but some of the others do. And the modifications don't appear in the test instruction list. They should be relatively simple tests additions, but I think key to ensure it's all correct. I would expect duplicate_course followed by get_courses(newid) and then update_course would be a likely common roll forward strategy. Sorry it's coming in little bits, I only have access to a mobile device and it's not as easy to cover everything.
              Hide
              dmonllao David Monllaó added a comment -

              Thanks Russell, yes, it should be fine. I've added testing instructions to cover duplicate_course

              Show
              dmonllao David Monllaó added a comment - Thanks Russell, yes, it should be fine. I've added testing instructions to cover duplicate_course
              Hide
              marina Marina Glancy added a comment -

              I created MDL-56251 for the format_weeks follow up

              Show
              marina Marina Glancy added a comment - I created MDL-56251 for the format_weeks follow up
              Hide
              stronk7 Eloy Lafuente (stronk7) added a comment - - edited

              Thanks David (et all), the patch looks really complete. Some (really minor) comments:

              1) This is the only change in the patch that I don't understand.
              2) Changes to params & return values in WS must be documented in upgrade.txt files.
              3) Don't we need manual testing of upload courses. Seems to be the exception.
              4) When auto-calculating enddate for weeks format in a new course... it does not match the last week end (i get here December 13th for the 10th week and the course enddate is set to December 14th by default). Not critical but it's a small inconsistency.
              5) In any case, what mostly surprised me is why those December dates were being shown (I was expecting 1year default to be applied), till I realized it was a weekly formatted course. Not saying that it's wrong, just that it's not automatic/trivial to understand why enddate is enabled by default for new courses (I'm not fully convinced it's a best idea) and why it shows those (mysterious, unexpected) dates.
              6) An immediate relation that came to my brain when looking to this code is that, surely, a new course-completion rule should be created to allow completion once enddate has happened (think there is already some rule based on dates, but this new endate sounds like a logical one to have there). Of course, that is another issue, for your consideration.

              Surely the 5th point is the one that I'd put more importance (coz it's behavioral and affecting all new courses). I've read your rationale for enabling enddates on new courses and I'm not really sure it's a good default behavior. Many courses don't mind start date at all, why should them mind end date? Not sure really, maybe an alternative could be that, if the default is 0, then enddates are not enabled (although it sounds hacky). Anyway, just discuss it there. I'm not really strong on it, just "feel" that enddates should not be enabled by default. Whatever is decided I'm ok.

              Ciao

              Show
              stronk7 Eloy Lafuente (stronk7) added a comment - - edited Thanks David (et all), the patch looks really complete. Some (really minor) comments: 1) This is the only change in the patch that I don't understand. 2) Changes to params & return values in WS must be documented in upgrade.txt files. 3) Don't we need manual testing of upload courses. Seems to be the exception. 4) When auto-calculating enddate for weeks format in a new course... it does not match the last week end (i get here December 13th for the 10th week and the course enddate is set to December 14th by default). Not critical but it's a small inconsistency. 5) In any case, what mostly surprised me is why those December dates were being shown (I was expecting 1year default to be applied), till I realized it was a weekly formatted course. Not saying that it's wrong, just that it's not automatic/trivial to understand why enddate is enabled by default for new courses (I'm not fully convinced it's a best idea) and why it shows those (mysterious, unexpected) dates. 6) An immediate relation that came to my brain when looking to this code is that, surely, a new course-completion rule should be created to allow completion once enddate has happened (think there is already some rule based on dates, but this new endate sounds like a logical one to have there). Of course, that is another issue, for your consideration. Surely the 5th point is the one that I'd put more importance (coz it's behavioral and affecting all new courses). I've read your rationale for enabling enddates on new courses and I'm not really sure it's a good default behavior. Many courses don't mind start date at all, why should them mind end date? Not sure really, maybe an alternative could be that, if the default is 0, then enddates are not enabled (although it sounds hacky). Anyway, just discuss it there. I'm not really strong on it, just "feel" that enddates should not be enabled by default. Whatever is decided I'm ok. Ciao
              Hide
              stronk7 Eloy Lafuente (stronk7) added a comment -

              7) Forgot it in my previous comment... while I'm not a fan of executable fixtures... no matter of that... the current one in your patch is missing MOODLE_INTERNAL check, that is mandatory 100%. Easy to add.

              Show
              stronk7 Eloy Lafuente (stronk7) added a comment - 7) Forgot it in my previous comment... while I'm not a fan of executable fixtures... no matter of that... the current one in your patch is missing MOODLE_INTERNAL check, that is mandatory 100%. Easy to add.
              Hide
              dmonllao David Monllaó added a comment -

              Many thanks for the review Eloy.

              We enable end date by default for new courses because we want to encourage people to set the course end date, it is a key value for analytics and another project I don't know much details about, but related with dashboard (MUA). As part of this issue we just tried to set a reasonable default end date according to the start date. We defered to MDL-56251 UX improvements to encourage people to set this value properly (for example a warning if the end date that is set does not match the calculated one) but if we disable it people will just ignore the setting. I understand your concerns regarding people that don't mind start date, but if they don't care about it they can just disable end date, start date it not even optional. My concern is that we have no guarantee that the end date that is set is the correct one (same with start date) but that will be detected by tools using those course start and end date values, IMO there is no need to bother about it now that users don't have any benefit on setting and end date, would make more sense to push users to set it up correctly when they see that otherwise other stuff will not work properly

              1) I moved it https://github.com/dmonllao/moodle/compare/MDL-22078_master~3...MDL-22078_master#diff-52258fb3acdb62b604b7c4e04f2e0c7bR278 so in https://github.com/dmonllao/moodle/compare/MDL-22078_master~3...MDL-22078_master#diff-52258fb3acdb62b604b7c4e04f2e0c7bR276 I can test that I upload doesn't work with enddate but no startdate
              2) Done
              3) Added
              4) This is because of https://github.com/moodle/moodle/blob/master/course/format/weeks/lib.php#L80. We are using the same calculation than format_weeks uses everywhere to calculate the end of the week, only that without removing the last day. I agree it is not nice that it differs from what is displayed in screen though.
              5) Replied above
              7) Fixed

              Show
              dmonllao David Monllaó added a comment - Many thanks for the review Eloy. We enable end date by default for new courses because we want to encourage people to set the course end date, it is a key value for analytics and another project I don't know much details about, but related with dashboard (MUA). As part of this issue we just tried to set a reasonable default end date according to the start date. We defered to MDL-56251 UX improvements to encourage people to set this value properly (for example a warning if the end date that is set does not match the calculated one) but if we disable it people will just ignore the setting. I understand your concerns regarding people that don't mind start date, but if they don't care about it they can just disable end date, start date it not even optional. My concern is that we have no guarantee that the end date that is set is the correct one (same with start date) but that will be detected by tools using those course start and end date values, IMO there is no need to bother about it now that users don't have any benefit on setting and end date, would make more sense to push users to set it up correctly when they see that otherwise other stuff will not work properly 1) I moved it https://github.com/dmonllao/moodle/compare/MDL-22078_master~3...MDL-22078_master#diff-52258fb3acdb62b604b7c4e04f2e0c7bR278 so in https://github.com/dmonllao/moodle/compare/MDL-22078_master~3...MDL-22078_master#diff-52258fb3acdb62b604b7c4e04f2e0c7bR276 I can test that I upload doesn't work with enddate but no startdate 2) Done 3) Added 4) This is because of https://github.com/moodle/moodle/blob/master/course/format/weeks/lib.php#L80 . We are using the same calculation than format_weeks uses everywhere to calculate the end of the week, only that without removing the last day. I agree it is not nice that it differs from what is displayed in screen though. 5) Replied above 7) Fixed
              Hide
              stronk7 Eloy Lafuente (stronk7) added a comment -

              Thanks David,

              while I continue not fully convinced about having it enabled and set by default for new courses (both because it's a behavioral change and if somebody wants course-end dependent analytics go and enable it), as said, I'm not strong in that point, so accepted.

              Regarding 6), I've added a comment in MDL-48762, that could be a good existing issue to talk about dependencies based on this new setting.

              Re-reviewing the code...

              Show
              stronk7 Eloy Lafuente (stronk7) added a comment - Thanks David, while I continue not fully convinced about having it enabled and set by default for new courses (both because it's a behavioral change and if somebody wants course-end dependent analytics go and enable it), as said, I'm not strong in that point, so accepted. Regarding 6), I've added a comment in MDL-48762 , that could be a good existing issue to talk about dependencies based on this new setting. Re-reviewing the code...
              Hide
              stronk7 Eloy Lafuente (stronk7) added a comment -

              Integrated (master only), thanks!

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

              Thanks for working on this David,

              unfortunately this is causing behat failure as course end date has to be now more then start date.

              001 Scenario: Use course reset to update chat start date         # /store/moodle/behat_whole_suite_m_parallel/mod/chat/tests/behat/chat_course_reset.feature:23
                    And I should see "Date changed" in the "Chats" "table_row" # /store/moodle/behat_whole_suite_m_parallel/mod/chat/tests/behat/chat_course_reset.feature:49
                      Table_row matching locator "'Chats'" not found.
              

              Thanks to Eloy Lafuente (stronk7), just realised that reset_end_date could be empty but after this patch it can't be.

              With reset_end_date empty, we calculate timeshift, which is not valid after this patch.

              Show
              rajeshtaneja Rajesh Taneja added a comment - - edited Thanks for working on this David, unfortunately this is causing behat failure as course end date has to be now more then start date. 001 Scenario: Use course reset to update chat start date # /store/moodle/behat_whole_suite_m_parallel/mod/chat/tests/behat/chat_course_reset.feature:23 And I should see "Date changed" in the "Chats" "table_row" # /store/moodle/behat_whole_suite_m_parallel/mod/chat/tests/behat/chat_course_reset.feature:49 Table_row matching locator "'Chats'" not found. Thanks to Eloy Lafuente (stronk7) , just realised that reset_end_date could be empty but after this patch it can't be. With reset_end_date empty, we calculate timeshift, which is not valid after this patch.
              Hide
              dmonllao David Monllaó added a comment -

              Nice, behat catched a valid bug in the patch logic, the problem is that, reset_form::validation is applying the timeshift to the end date even if the course end date was 0, this is easy to fix and I have a patch, although I want some extra time to check enddate and the testing infrastructure; my main concern is that the testing site should be consistent with the production default site, by default we enable the end date when creating a course, but data generators and the database field default is 0.

              Show
              dmonllao David Monllaó added a comment - Nice, behat catched a valid bug in the patch logic, the problem is that, reset_form::validation is applying the timeshift to the end date even if the course end date was 0, this is easy to fix and I have a patch, although I want some extra time to check enddate and the testing infrastructure; my main concern is that the testing site should be consistent with the production default site, by default we enable the end date when creating a course, but data generators and the database field default is 0.
              Hide
              dmonllao David Monllaó added a comment -

              This is the patch to fix the behat failure (https://github.com/dmonllao/moodle/compare/MDL-22078_master-fix~1...MDL-22078_master-fix) This case was not covered by phpunit because the bug is in the form's validation. I've run phpunit and behat tests related with course reset and all good.

              git pull git://github.com/dmonllao/moodle.git MDL-22078_master-fix

              Show
              dmonllao David Monllaó added a comment - This is the patch to fix the behat failure ( https://github.com/dmonllao/moodle/compare/MDL-22078_master-fix~1...MDL-22078_master-fix ) This case was not covered by phpunit because the bug is in the form's validation. I've run phpunit and behat tests related with course reset and all good. git pull git://github.com/dmonllao/moodle.git MDL-22078 _master-fix
              Hide
              dobedobedoh Andrew Nicols added a comment -

              Thanks David,

              That patch looks good and I've pulled it now.

              Patch integrated to master only. Sending back to testing.

              Show
              dobedobedoh Andrew Nicols added a comment - Thanks David, That patch looks good and I've pulled it now. Patch integrated to master only. Sending back to testing.
              Hide
              dmonllao David Monllaó added a comment - - edited

              I just had a chat with Rajesh regarding the default end date. He was also a bit confused about the end date purpose and default value. Things were "less unclear" once I explained that course start date is currently a compulsory course setting and that it does not block users from accessing the course. Rajesh Taneja feel free to comment if I am missing any important point.

              I will resume how the default end date works from the lowest level to the higher one:

              1. At database level the default value is 0 (no course end date)
                • Any other default value that this would be wrong as the end date always depends on the course start date.
              2. At Moodle API level create_course() (and testing/development callers - data_generator::create_course()/tool_generator_course_backend::create_course) the default is no course end date as well.
                • This default is good for backwards compatibility and third parties
              3. At UI level Moodle admins create new courses using Moodle's UI through "Site admin" > "Courses" > "Manage courses and categories" > "Create new course". Here, by default, we enable the "Course end date" setting and we set the default value to a calculated value that depends on the default course format
                • Course creators can always disable the end date
                • We encourage people to set the value if an end date make sense for them

              It is important to note that only a portion of all courses are created using Moodle UI, sites with many courses have synchronization processes to create courses automatically from external information systems. In those cases courses are created through create_course (defaults to no end date)

              All this is the rationale behind the current design, although it is not perfect because:

              1. Data generators should mimic default moodle values as much as possible. Testing uses Moodle APIs to create courses, which defaults to end date disabled. In behat case this is less ideal than when writing phpunit tests because we want the elements (courses, users...) created using a data generator to be as close as possible to a course created manually by a user
              2. We would like to encourage people to set the end date, even if they create the course through an automatic sync process

              IMO the current design has a good balance between backwards compatibility (we shouldn't annoy 3rd parties with a new setting when there is no real benefit on setting it yet) and encouraging people to set the end date for reporting purposes and all my doubts could be resumed in "Should behat data generator set a default course end date or not?"

              Show
              dmonllao David Monllaó added a comment - - edited I just had a chat with Rajesh regarding the default end date. He was also a bit confused about the end date purpose and default value. Things were "less unclear" once I explained that course start date is currently a compulsory course setting and that it does not block users from accessing the course. Rajesh Taneja feel free to comment if I am missing any important point. I will resume how the default end date works from the lowest level to the higher one: At database level the default value is 0 (no course end date) Any other default value that this would be wrong as the end date always depends on the course start date. At Moodle API level create_course() (and testing/development callers - data_generator::create_course()/tool_generator_course_backend::create_course) the default is no course end date as well. This default is good for backwards compatibility and third parties At UI level Moodle admins create new courses using Moodle's UI through "Site admin" > "Courses" > "Manage courses and categories" > "Create new course". Here, by default, we enable the "Course end date" setting and we set the default value to a calculated value that depends on the default course format Course creators can always disable the end date We encourage people to set the value if an end date make sense for them It is important to note that only a portion of all courses are created using Moodle UI, sites with many courses have synchronization processes to create courses automatically from external information systems. In those cases courses are created through create_course (defaults to no end date) All this is the rationale behind the current design, although it is not perfect because: Data generators should mimic default moodle values as much as possible. Testing uses Moodle APIs to create courses, which defaults to end date disabled. In behat case this is less ideal than when writing phpunit tests because we want the elements (courses, users...) created using a data generator to be as close as possible to a course created manually by a user We would like to encourage people to set the end date, even if they create the course through an automatic sync process IMO the current design has a good balance between backwards compatibility (we shouldn't annoy 3rd parties with a new setting when there is no real benefit on setting it yet) and encouraging people to set the end date for reporting purposes and all my doubts could be resumed in "Should behat data generator set a default course end date or not?"
              Hide
              ryanwyllie Ryan Wyllie added a comment -

              Thanks David. Everything worked as described. Testing passed.

              Show
              ryanwyllie Ryan Wyllie added a comment - Thanks David. Everything worked as described. Testing passed.
              Hide
              stronk7 Eloy Lafuente (stronk7) added a comment -

              And this, now, can be safely closed, while Moodle's hordes start using it! Many, many thanks!

              "I don't know half of you half as well as I should like; and I like
              less than half of you half as well as you deserve."
              — J.R.R. Tolkien

              Show
              stronk7 Eloy Lafuente (stronk7) added a comment - And this, now, can be safely closed, while Moodle's hordes start using it! Many, many thanks! "I don't know half of you half as well as I should like; and I like less than half of you half as well as you deserve." — J.R.R. Tolkien

                People

                • Votes:
                  19 Vote for this issue
                  Watchers:
                  29 Start watching this issue

                  Dates

                  • Created:
                    Updated:
                    Resolved:
                    Fix Release Date:
                    5/Dec/16