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:

      Description

        Gliffy Diagrams

          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: