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. Look at Course format -> Format value
      3. Create a new course
      4. If your default course Format (you just looked at the value 2 steps above) is set to something different than weekly format the default end date value shoudl be startdate + 6 months, otherwise it should be startdate + (1 week for each numsections)
      5. Open the JS console to look for errors (better if you can set it so top on errors as there is an ajax request involved and you may not notice a possible JS error)
      6. Change the format to weekly or something different than weekly if it was already weekly, the page will reload
      7. The prefilled end date should change according to what was described above
      8. Set a end date value, save changes and go back to edit setting to check that the value you set has been saved
      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. Set course start date > the course current end date and press 'Reset course'
      11. You should see a form validation error
      12. Enable end date as well and set a date lower than the reset start date. Press 'Reset course'
      13. Expect again a validation error
      14. Now set end date to be > than start date and press 'Reset course'
      15. The reset should proceed and the new start and end dates should be visible in the course settings page
      16. Go to 'Course administration' -> 'Reset' again
      17. Set both end date and start date (start date < end date) and press 'Reset course'
      18. The reset should proceed and the new start and end dates should be visible in the course settings page
      19. 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)
      20. Go to 'Course administration' -> 'Reset' again
      21. 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'
      22. 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, don't set end date -> OK
        • Set start date < end date -> OK
        • Set start date > end date -> KO
        • Don't set start date, set end date -> KO
      Show
      Course forms & defaults Go to admin/settings.php?section=coursesettings, set "Course duration" to 6 months and save changes Look at Course format -> Format value Create a new course If your default course Format (you just looked at the value 2 steps above) is set to something different than weekly format the default end date value shoudl be startdate + 6 months, otherwise it should be startdate + (1 week for each numsections) Open the JS console to look for errors (better if you can set it so top on errors as there is an ajax request involved and you may not notice a possible JS error) Change the format to weekly or something different than weekly if it was already weekly, the page will reload The prefilled end date should change according to what was described above Set a end date value, save changes and go back to edit setting to check that the value you set has been saved 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 Set course start date > the course current end date and press 'Reset course' You should see a form validation error 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 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, don't set end date -> OK Set start date < end date -> OK Set start date > end date -> KO Don't set start date, set end date -> KO
    • Difficulty:
      Easy
    • Affected Branches:
      MOODLE_19_STABLE, MOODLE_26_STABLE, MOODLE_27_STABLE, MOODLE_29_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

                People

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

                  Dates

                  • Created:
                    Updated: