Moodle
  1. Moodle
  2. MDL-37410

External PHPunit test: used $this->getDataGenerator()->enrol_user instead to manually enrol the user.

    Details

    • Type: Improvement Improvement
    • Status: Closed
    • Priority: Minor Minor
    • Resolution: Fixed
    • Affects Version/s: 2.3.3, 2.4.1, 2.5
    • Fix Version/s: 2.5
    • Component/s: Web Services
    • Labels:
    • Rank:
      47034

      Description

        Issue Links

          Activity

          Hide
          Jérôme Mouneyrac added a comment -

          Done sending to peer-review.

          Show
          Jérôme Mouneyrac added a comment - Done sending to peer-review.
          Hide
          Mark Nelson added a comment -

          Hi Jerome, looks good to me. However, I would rebase this as a new web service was introduced (get_forum_discussions) along with a PHPUnit test that uses the old method for enrolment. It uses the old method twice, where as one of the enrolments can be replaced by $this->getDataGenerator()->enrol_user.

          Show
          Mark Nelson added a comment - Hi Jerome, looks good to me. However, I would rebase this as a new web service was introduced (get_forum_discussions) along with a PHPUnit test that uses the old method for enrolment. It uses the old method twice, where as one of the enrolments can be replaced by $this->getDataGenerator()->enrol_user.
          Hide
          Jérôme Mouneyrac added a comment - - edited

          If I do what you suggest then I can fix this issue each week if each week there is a new web service which get integrated with the wrong enrol_user. Being your web service, it seems fair enough to me that you can fix it

          Show
          Jérôme Mouneyrac added a comment - - edited If I do what you suggest then I can fix this issue each week if each week there is a new web service which get integrated with the wrong enrol_user. Being your web service, it seems fair enough to me that you can fix it
          Hide
          Mark Nelson added a comment -

          Well, this was not the standard before when creating web services. It's easier to fix it all now in one commit rather than me opening up a new tracker issue to do it for one web service. You are doing it now so may as well fix it everywhere, how much longer will it take, 1 minute? Then from now on when reviewing web services reviewers can correct users who use the old method, preventing any new web services from being integrated using the old method, meaning you won't have to keep changing.

          Show
          Mark Nelson added a comment - Well, this was not the standard before when creating web services. It's easier to fix it all now in one commit rather than me opening up a new tracker issue to do it for one web service. You are doing it now so may as well fix it everywhere, how much longer will it take, 1 minute? Then from now on when reviewing web services reviewers can correct users who use the old method, preventing any new web services from being integrated using the old method, meaning you won't have to keep changing.
          Hide
          Mark Nelson added a comment -

          I suggest this is fixed in all occurrences before being integrated, otherwise there is no point in doing a half assed commit.

          Show
          Mark Nelson added a comment - I suggest this is fixed in all occurrences before being integrated, otherwise there is no point in doing a half assed commit.
          Hide
          Jérôme Mouneyrac added a comment - - edited

          ah Mark... I'll do the change but because I'm nice I hope I see only externall PHPunit with $this->getDataGenerator()->enrol_user being integrated by integrators in the meanwhile otherwise I'll blame you
          I need to backport this stuff anyway.

          Show
          Jérôme Mouneyrac added a comment - - edited ah Mark... I'll do the change but because I'm nice I hope I see only externall PHPunit with $this->getDataGenerator()->enrol_user being integrated by integrators in the meanwhile otherwise I'll blame you I need to backport this stuff anyway.
          Hide
          Mark Nelson added a comment -

          Thanks Jerome, appreciated.

          Show
          Mark Nelson added a comment - Thanks Jerome, appreciated.
          Hide
          Mark Nelson added a comment -

          This should be backported when MDL-37964 is done. I suggest waiting.

          Show
          Mark Nelson added a comment - This should be backported when MDL-37964 is done. I suggest waiting.
          Hide
          Jérôme Mouneyrac added a comment -

          I send back to peer-review for the piece of code I added to support getDataGenerator->enrol_users assigning a default roleid with multiple permissions.

          Show
          Jérôme Mouneyrac added a comment - I send back to peer-review for the piece of code I added to support getDataGenerator->enrol_users assigning a default roleid with multiple permissions.
          Hide
          Mark Nelson added a comment - - edited

          Hi Jerome,

          A few things.

          // Assign capabilities to view discussions for forum 1.
          // Need to keep track of this context and role as we use it later in testing.
          $cm1 = get_coursemodule_from_id('forum', $forum1->id, 0, false, MUST_EXIST);
          $context1 = context_module::instance($cm1->id);
          $roleid1 = $this->assignUserCapability('mod/forum:viewdiscussion', $context1->id);
          
          // Assign capabilities to view discussions for forum 1.
          // Need to keep track of this context and role as we use it later in testing.
          $cm = get_coursemodule_from_id('forum', $forum1->id, 0, false, MUST_EXIST);
          $context1 = context_module::instance($cm->id);
          $roleid1 = $this->assignUserCapability('mod/forum:viewdiscussion', $context1->id);
          

          I see this was removed because the data generator's enrol_user function gives them permission to view all the forums in the course, however, there is still excess code for the other enrolments and assigning of capabilities. I propose we use enrol_user everywhere and avoid having to ever assign the mod/forum:viewdiscussion capability. If we changed enrol_user to return an instance, we could then pass this to a new function (unenrol_user) used by the unit tests to unenrol the user.

          // DataGenerator->enrol_user automatically sets a role for the user with the permission mod/form:viewdiscussion.

          DataGenerator->enrol_user does not look nice in a comment, maybe change to "This sets a role for the user with the permission mod/form:viewdiscussion."

          $forums = mod_forum_external::get_forums_by_courses(array($course1->id));

          Why change this? There is no need to assign this to anything.

          // Following line enrol and assign default role id to the user.
          // So the user automatically gets mod/forum:viewdiscussion on all forums of the course.

          This can be set to "This sets a role for the user with the permission mod/form:viewdiscussion." as well.

          // We don't use the dataGenerator as we need to get the $instance2 to unenrol later.

          You can use your previous comment "Execute real Moodle enrolment as we'll call unenrol() method on the instance later.", though I would change this to "Execute a real Moodle enrolment as we'll call unenrol() on the instance later."

          public static function unassignUserCapability($capability, $contextid = null, $roleid = null, $courseid = null, $enrol = 'manual') {

          The function unassignUserCapability takes up quite a bit of space on the one line, I would put the parameters on two lines.

          'courseid'=>$courseid, 'enrol'=>$enrol

          I personally like spaces before and after "=>".

          EDIT I added '--' before each code content because stupid JIRA does not put spaces before the code and text. Even if I added two backslashes which the wiki says will add a line space, code tags ignore this. Also, previewing my changes did not help as originally it showed that there would be spaces.

          Show
          Mark Nelson added a comment - - edited Hi Jerome, A few things. – // Assign capabilities to view discussions for forum 1. // Need to keep track of this context and role as we use it later in testing. $cm1 = get_coursemodule_from_id('forum', $forum1->id, 0, false , MUST_EXIST); $context1 = context_module::instance($cm1->id); $roleid1 = $ this ->assignUserCapability('mod/forum:viewdiscussion', $context1->id); // Assign capabilities to view discussions for forum 1. // Need to keep track of this context and role as we use it later in testing. $cm = get_coursemodule_from_id('forum', $forum1->id, 0, false , MUST_EXIST); $context1 = context_module::instance($cm->id); $roleid1 = $ this ->assignUserCapability('mod/forum:viewdiscussion', $context1->id); I see this was removed because the data generator's enrol_user function gives them permission to view all the forums in the course, however, there is still excess code for the other enrolments and assigning of capabilities. I propose we use enrol_user everywhere and avoid having to ever assign the mod/forum:viewdiscussion capability. If we changed enrol_user to return an instance, we could then pass this to a new function (unenrol_user) used by the unit tests to unenrol the user. – // DataGenerator->enrol_user automatically sets a role for the user with the permission mod/form:viewdiscussion. DataGenerator->enrol_user does not look nice in a comment, maybe change to "This sets a role for the user with the permission mod/form:viewdiscussion." – $forums = mod_forum_external::get_forums_by_courses(array($course1->id)); Why change this? There is no need to assign this to anything. – // Following line enrol and assign default role id to the user. // So the user automatically gets mod/forum:viewdiscussion on all forums of the course. This can be set to "This sets a role for the user with the permission mod/form:viewdiscussion." as well. – // We don't use the dataGenerator as we need to get the $instance2 to unenrol later. You can use your previous comment "Execute real Moodle enrolment as we'll call unenrol() method on the instance later.", though I would change this to "Execute a real Moodle enrolment as we'll call unenrol() on the instance later." – public static function unassignUserCapability($capability, $contextid = null , $roleid = null , $courseid = null , $enrol = 'manual') { The function unassignUserCapability takes up quite a bit of space on the one line, I would put the parameters on two lines. – 'courseid'=>$courseid, 'enrol'=>$enrol I personally like spaces before and after "=>". EDIT I added '--' before each code content because stupid JIRA does not put spaces before the code and text. Even if I added two backslashes which the wiki says will add a line space, code tags ignore this. Also, previewing my changes did not help as originally it showed that there would be spaces.
          Hide
          Jérôme Mouneyrac added a comment - - edited

          If I remember I did fix your PHPunit test when in fact we already mentioned that we should use Datagenerator by the time your PHPunit test got integrated. Being nice I reworked on this and fixed your forum PHPunit. And now seeing that we can not convert all manual enrolment in Datagenerator you ask me to tweak your PHPunit. In my opinion it would be much faster if you have a look to your own PHPunit code

          More we wait for this to be integrated more the core code that people use as example stay "incorrect". Sending to integration, we'll see what they decide (assigning this Mark in case it doesn't get integrated ).

          Show
          Jérôme Mouneyrac added a comment - - edited If I remember I did fix your PHPunit test when in fact we already mentioned that we should use Datagenerator by the time your PHPunit test got integrated. Being nice I reworked on this and fixed your forum PHPunit. And now seeing that we can not convert all manual enrolment in Datagenerator you ask me to tweak your PHPunit. In my opinion it would be much faster if you have a look to your own PHPunit code More we wait for this to be integrated more the core code that people use as example stay "incorrect". Sending to integration, we'll see what they decide (assigning this Mark in case it doesn't get integrated ).
          Hide
          Damyon Wiese added a comment -

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

          Thanks!

          Show
          Damyon Wiese added a comment - The main moodle.git repository has just been updated with latest weekly modifications. You may wish to rebase your PULL branches to simplify history and avoid any possible merge conflicts. This would also make integrator's life easier next week. Thanks!
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Are you serious here? Who is fixing this, who is peer-reviewing? Are all the points listed by Mark above already fixed? WTF!

          Show
          Eloy Lafuente (stronk7) added a comment - Are you serious here? Who is fixing this, who is peer-reviewing? Are all the points listed by Mark above already fixed? WTF!
          Hide
          Damyon Wiese added a comment -

          Reopening at Jeromes request.

          Show
          Damyon Wiese added a comment - Reopening at Jeromes request.
          Hide
          Jérôme Mouneyrac added a comment -

          Reopen it, I'll have a look to the forum part.

          Show
          Jérôme Mouneyrac added a comment - Reopen it, I'll have a look to the forum part.
          Hide
          Jérôme Mouneyrac added a comment -

          thanks.

          Show
          Jérôme Mouneyrac added a comment - thanks.
          Hide
          Damyon Wiese added a comment -

          (for ci counters will reopen this from in review state).

          Show
          Damyon Wiese added a comment - (for ci counters will reopen this from in review state).
          Hide
          CiBoT added a comment -

          Moving this reopened issue out from current integration. Please, re-submit it for integration once ready.

          Show
          CiBoT added a comment - Moving this reopened issue out from current integration. Please, re-submit it for integration once ready.
          Hide
          Jérôme Mouneyrac added a comment - - edited

          I created a new issue for forum externallib PHPunit test (I'll have a look to them later, I don't know the code well enough yet, and I'd like to see this issue integrated) - https://tracker.moodle.org/browse/MDL-38834.

          Show
          Jérôme Mouneyrac added a comment - - edited I created a new issue for forum externallib PHPunit test (I'll have a look to them later, I don't know the code well enough yet, and I'd like to see this issue integrated) - https://tracker.moodle.org/browse/MDL-38834 .
          Hide
          Jérôme Mouneyrac added a comment -

          I fixed the point that Mark mentioned (excepted for the forum file as mentioned in previous comment). Sending for peer-review. Cheers.

          Show
          Jérôme Mouneyrac added a comment - I fixed the point that Mark mentioned (excepted for the forum file as mentioned in previous comment). Sending for peer-review. Cheers.
          Hide
          Dan Poltawski added a comment -

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

          TIA and ciao

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

          Integrated to master - thanks Jerome.

          Show
          Dan Poltawski added a comment - Integrated to master - thanks Jerome.
          Hide
          Dan Poltawski added a comment -

          phpunit passed succesfully

          Show
          Dan Poltawski added a comment - phpunit passed succesfully
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Your awesome contributions are now part of Moodle, your fav LMS out there.

          Closing this as fixed.

          Many thanks for all the hard work, ciao

          Show
          Eloy Lafuente (stronk7) added a comment - Your awesome contributions are now part of Moodle, your fav LMS out there. Closing this as fixed. Many thanks for all the hard work, ciao

            People

            • Votes:
              0 Vote for this issue
              Watchers:
              3 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved: