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

Default Enrollment duration is not applied when manually enrolling a user if enrollment option is not expanded.

    Details

    • Testing Instructions:
      Hide

      Steps:
      -Go into a course
      -Course Admin>Enrollment Methods>Manual Enrollments>Settings
      -Set and Enable a Default Enrollment Duration
      -Go to Enrolled users
      -Enroll Users
      -Just enrol a user
      Check: end date is applied to the user enrollment

      Show
      Steps: -Go into a course -Course Admin>Enrollment Methods>Manual Enrollments>Settings -Set and Enable a Default Enrollment Duration -Go to Enrolled users -Enroll Users -Just enrol a user Check: end date is applied to the user enrollment
    • Affected Branches:
      MOODLE_24_STABLE, MOODLE_25_STABLE
    • Fixed Branches:
      MOODLE_24_STABLE, MOODLE_25_STABLE, MOODLE_26_STABLE
    • Pull Master Branch:
      MDL-42085-master
    • Sprint:
      FRONTEND Sprint 7
    • Story Points (Obsolete):
      5
    • Sprint:
      FRONTEND Sprint 7

      Description

      Issue: Enrollment duration is not applied when manually enrolling a user if enrollment option is not expanded.

      Steps to Replicate:
      -Go into a course
      -Course Admin>Enrollment Methods>Manual Enrollments>Settings
      -Set and Enable a Default Enrollment Duration
      -Go to Enrolled users
      -Enroll Users
      -Just enrol a user
      Result: No end date is applied to the user enrollment

      To get the user enrollment duration applied:
      -Go to Enrolled Users
      -Click the Enrollment Options arrow to drop that menu down
      -Make no changes to it (you will see the default is populated)
      -Enrol User
      -Finish Enrolling users
      -End Date is populated

        Gliffy Diagrams

          Attachments

            Issue Links

              Activity

              Hide
              skodak Petr Skoda added a comment -

              Thanks for the report.

              Show
              skodak Petr Skoda added a comment - Thanks for the report.
              Hide
              salvetore Michael de Raadt added a comment -

              This issue has been duplicated. I'm giving it a nudge.

              Show
              salvetore Michael de Raadt added a comment - This issue has been duplicated. I'm giving it a nudge.
              Hide
              heather.williams Heather Williams added a comment -

              I can replicate this behavior in 2.5.2 as well.

              Show
              heather.williams Heather Williams added a comment - I can replicate this behavior in 2.5.2 as well.
              Hide
              jerome Jérôme Mouneyrac added a comment -

              Thanks for the great step Kevin. Sent to peer-review.

              Show
              jerome Jérôme Mouneyrac added a comment - Thanks for the great step Kevin. Sent to peer-review.
              Hide
              dobedobedoh Andrew Nicols added a comment -

              Hi Jerome,

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

              Documentation:
              There isn't any to explain this change - can you add into the issue why we're doing this so that in 6 months time when I come to blame you I understand why

              Git:
              The commit message could do with a few tweaks:

              • We code do Australian English, where Enrolment has a single l;
              • The commit message summary is a touch long
              • If you could explain the rationale for the change in the commit message that would be grand

              Sanity check:
              I must admit, possibly because of the lack of comments and explanation, I have no idea what this is doing so I'm unable to sanity check this.

              Show
              dobedobedoh Andrew Nicols added a comment - Hi Jerome, [Y] Syntax [Y] Whitespace [Y] Output [-] Language [-] Databases [Y] Testing (instructions and automated tests) [Y] Security [N] Documentation [N] Git [-] Third party code [N] Sanity check Documentation: There isn't any to explain this change - can you add into the issue why we're doing this so that in 6 months time when I come to blame you I understand why Git: The commit message could do with a few tweaks: We code do Australian English, where Enrolment has a single l; The commit message summary is a touch long If you could explain the rationale for the change in the commit message that would be grand Sanity check: I must admit, possibly because of the lack of comments and explanation, I have no idea what this is doing so I'm unable to sanity check this.
              Hide
              jerome Jérôme Mouneyrac added a comment - - edited

              Basically before the fix, the startdates and duration fields were integrated to HTML code when the user clicked on the collapsible region to see them. It was wrong because the value were not sent by the form if ever the user didn't expand their region - the Enrolment options.

              Show
              jerome Jérôme Mouneyrac added a comment - - edited Basically before the fix, the startdates and duration fields were integrated to HTML code when the user clicked on the collapsible region to see them. It was wrong because the value were not sent by the form if ever the user didn't expand their region - the Enrolment options.
              Hide
              jerome Jérôme Mouneyrac added a comment - - edited

              I talked with Andrew, I reworded the commit and added some explanation in the commit message lines below the first line (I should use them more often). Thanks for the peer-review Andrew. Sending to integration.

              Show
              jerome Jérôme Mouneyrac added a comment - - edited I talked with Andrew, I reworded the commit and added some explanation in the commit message lines below the first line (I should use them more often). Thanks for the peer-review Andrew. Sending to integration.
              Hide
              samhemelryk Sam Hemelryk added a comment -

              Thanks Jerome - this has been integrated now.

              Just noting there were a couple of things that struck me about this code.

              1. The php processing the AJAX request is not using the defaults either, as they are always applied that is not really a bug [right now].
              2. As we always populate the duration there is no longer a need for the default 0 to be added during the construction of the select (in the init method above your changes). Again it has no impact on functionality so not an issue.

              Both points would be nice to clean up, however as your changes work perfectly getting it in now is better than delaying it.

              Many thanks
              Sam

              Show
              samhemelryk Sam Hemelryk added a comment - Thanks Jerome - this has been integrated now. Just noting there were a couple of things that struck me about this code. The php processing the AJAX request is not using the defaults either, as they are always applied that is not really a bug [right now] . As we always populate the duration there is no longer a need for the default 0 to be added during the construction of the select (in the init method above your changes). Again it has no impact on functionality so not an issue. Both points would be nice to clean up, however as your changes work perfectly getting it in now is better than delaying it. Many thanks Sam
              Hide
              salvetore Michael de Raadt added a comment -

              Test result: Success!

              Tested in 2.4, 2.5, 2.6 and master.

              Show
              salvetore Michael de Raadt added a comment - Test result: Success! Tested in 2.4, 2.5, 2.6 and master.
              Hide
              damyon Damyon Wiese added a comment -

              Twas the week before Christmas,
              And all though HQ
              Devs were scrambling to finish peer review.
              They sent all their issues,
              and rushed out the door -
              "To the beach!" someone heard them roar!

              This issue has been released upstream. Thanks!

              Show
              damyon Damyon Wiese added a comment - Twas the week before Christmas, And all though HQ Devs were scrambling to finish peer review. They sent all their issues, and rushed out the door - "To the beach!" someone heard them roar! This issue has been released upstream. Thanks!

                People

                • Votes:
                  2 Vote for this issue
                  Watchers:
                  7 Start watching this issue

                  Dates

                  • Created:
                    Updated:
                    Resolved:
                    Fix Release Date:
                    13/Jan/14