Moodle
  1. Moodle
  2. MDL-26249

Create a web service function that returns all courses for a given enrolled user id

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Blocker Blocker
    • Resolution: Fixed
    • Affects Version/s: 2.0.1
    • Fix Version/s: 2.1
    • Component/s: Web Services
    • Labels:
    • Affected Branches:
      MOODLE_20_STABLE
    • Fixed Branches:
      MOODLE_21_STABLE
    • Rank:
      16444

      Description

      In enrol externallib:
      Create a bulk function with userid as bulk parameters
      Return the courses where the given users are enrolled).

        Activity

        Hide
        Martin Dougiamas added a comment -

        get_courses() is too generic. Probably get_user_courses(). This should be in the next sprint.

        Show
        Martin Dougiamas added a comment - get_courses() is too generic. Probably get_user_courses(). This should be in the next sprint.
        Hide
        Jérôme Mouneyrac added a comment -

        I calle the function get_courses_by_enrolled_users()

        Show
        Jérôme Mouneyrac added a comment - I calle the function get_courses_by_enrolled_users()
        Hide
        Jérôme Mouneyrac added a comment -

        Note about filtering on role: Petr suggested that it would be better to have a get_courses_by_users_roles instead of filtering role when requesting enrolments. The reason being that we would miss when a user has a role on a course but is not enrolled into it. It could be confusing. Moreover requesting/filtering course on role is going to be lot slower.

        Show
        Jérôme Mouneyrac added a comment - Note about filtering on role: Petr suggested that it would be better to have a get_courses_by_users_roles instead of filtering role when requesting enrolments. The reason being that we would miss when a user has a role on a course but is not enrolled into it. It could be confusing. Moreover requesting/filtering course on role is going to be lot slower.
        Hide
        Jérôme Mouneyrac added a comment -

        Petr can you review this commit before I do a pull request: https://github.com/mouneyrac/moodle/commit/34c30e0f53592c1a3639c31d9c4e08c26ad709ba
        thanks

        Show
        Jérôme Mouneyrac added a comment - Petr can you review this commit before I do a pull request: https://github.com/mouneyrac/moodle/commit/34c30e0f53592c1a3639c31d9c4e08c26ad709ba thanks
        Hide
        Jérôme Mouneyrac added a comment -

        Created a PULL request to accelerate the review

        Show
        Jérôme Mouneyrac added a comment - Created a PULL request to accelerate the review
        Hide
        Petr Škoda added a comment -

        Reopening because the functions seems to return too much duplicate course information. We already have the get courses functions which could be used to get the details.

        Show
        Petr Škoda added a comment - Reopening because the functions seems to return too much duplicate course information. We already have the get courses functions which could be used to get the details.
        Hide
        Jérôme Mouneyrac added a comment -

        I though that it is better to return more information than doing two web service calls. It was my reason to implement the function with full courses as return value.

        I'll do your requested changes.

        Show
        Jérôme Mouneyrac added a comment - I though that it is better to return more information than doing two web service calls. It was my reason to implement the function with full courses as return value. I'll do your requested changes.
        Hide
        Petr Škoda added a comment -

        reopening, see PULL request for details

        Show
        Petr Škoda added a comment - reopening, see PULL request for details
        Show
        Jérôme Mouneyrac added a comment - new fixes: https://github.com/mouneyrac/moodle/commit/0eaa8bd6121a153c6ffde76153e95b35f306a929
        Hide
        Jérôme Mouneyrac added a comment -

        integration git server has just been updated, PULL request need to be recreated.

        Show
        Jérôme Mouneyrac added a comment - integration git server has just been updated, PULL request need to be recreated.
        Hide
        moodle.com added a comment -

        Assigning to Petr as we discussed, to get this integrated ASAP

        Show
        moodle.com added a comment - Assigning to Petr as we discussed, to get this integrated ASAP
        Hide
        Petr Škoda added a comment -

        Well, first of all we have to decide what is this function going to do. It must deal with access control of current user and the user we are asking about, that is what capabilities are we going to use, how are we going to deal with suspended enrolments and hidden courses. This has to be decided before we do any coding, sorry.

        At present the only place that does something similar in UI is the course profile, we would have to create new capability if we wanted to test access at the system context.

        In any case this looks like one of those mnet style low level functions that would need to work without any access control, we do not have anything in Moodle UI that would return complete list of enrolments for one user...

        Show
        Petr Škoda added a comment - Well, first of all we have to decide what is this function going to do. It must deal with access control of current user and the user we are asking about, that is what capabilities are we going to use, how are we going to deal with suspended enrolments and hidden courses. This has to be decided before we do any coding, sorry. At present the only place that does something similar in UI is the course profile, we would have to create new capability if we wanted to test access at the system context. In any case this looks like one of those mnet style low level functions that would need to work without any access control, we do not have anything in Moodle UI that would return complete list of enrolments for one user...
        Hide
        Petr Škoda added a comment -

        It would be great if each issue for new external lib function contained the use cases, otherwise you might get something else than you expected...

        Show
        Petr Škoda added a comment - It would be great if each issue for new external lib function contained the use cases, otherwise you might get something else than you expected...
        Hide
        Jérôme Mouneyrac added a comment -

        An example of user case: an external app wants to display all courses of a participant.

        I suggest that return everything with status variable indicating 'suspended' or 'hidden'. Capabilities like 'view hidden courses' or 'can suspend enrolment' are used to know if the user is allow to access these information.

        If the user cannot and that the status is suspended/hidden the function doesn't return the course.

        Show
        Jérôme Mouneyrac added a comment - An example of user case: an external app wants to display all courses of a participant. I suggest that return everything with status variable indicating 'suspended' or 'hidden'. Capabilities like 'view hidden courses' or 'can suspend enrolment' are used to know if the user is allow to access these information. If the user cannot and that the status is suspended/hidden the function doesn't return the course.
        Hide
        Petr Škoda added a comment - - edited

        "An example of user case: an external app wants to display all courses of a participant." that is vague from the security pov. You have to say who and why wants to see that list. Does it matter if some courses are missing in that list because the person doing the WS can not enter the course? Do not talk about systems, always think about users because we do not give access to system - it is user.

        Show
        Petr Škoda added a comment - - edited "An example of user case: an external app wants to display all courses of a participant." that is vague from the security pov. You have to say who and why wants to see that list. Does it matter if some courses are missing in that list because the person doing the WS can not enter the course? Do not talk about systems, always think about users because we do not give access to system - it is user.
        Hide
        Jérôme Mouneyrac added a comment -

        humm Martin will tell you better about a real use case. I was just asked to report it. But a teacher wanting to see all the courses where his students are subscribed do not seem impossible to me As I said the capabilities will let him see what he can see.

        There are also few people voting and watching this issue, I'm sure they can give you a more real use case than me. So YOU who voted for this issue, make some noise

        Show
        Jérôme Mouneyrac added a comment - humm Martin will tell you better about a real use case. I was just asked to report it. But a teacher wanting to see all the courses where his students are subscribed do not seem impossible to me As I said the capabilities will let him see what he can see. There are also few people voting and watching this issue, I'm sure they can give you a more real use case than me. So YOU who voted for this issue, make some noise
        Hide
        Petr Škoda added a comment -

        Teacher is assigned at the course level only, they are not supposed to have access to all courses of everybody in their course.

        Show
        Petr Škoda added a comment - Teacher is assigned at the course level only, they are not supposed to have access to all courses of everybody in their course.
        Hide
        Antero added a comment -

        According to our use case, the teacher (logged in to external app / portal) needs a list of all of her courses in order to decide whether to create a new course or link the existing ones to SIS. The ultimate target of this linking is to start syncing the enrolled students from SIS to Moodle (MDL-26250).

        Show
        Antero added a comment - According to our use case, the teacher (logged in to external app / portal) needs a list of all of her courses in order to decide whether to create a new course or link the existing ones to SIS. The ultimate target of this linking is to start syncing the enrolled students from SIS to Moodle ( MDL-26250 ).
        Hide
        Petr Škoda added a comment - - edited

        This issue is not about viewing of own enrolled courses, this is about listing courses of somebody else.

        EDIT: ignore this comment, this applies to the 1/ in the next comment only

        Show
        Petr Škoda added a comment - - edited This issue is not about viewing of own enrolled courses, this is about listing courses of somebody else. EDIT: ignore this comment, this applies to the 1/ in the next comment only
        Hide
        Petr Škoda added a comment -

        Our current WS api is designed to run in a security context of one user - that makes it suitable for mobile applications or other single user applications.

        If you want to join moodle with other multiuser system it gets a lot lot trickier because we would need a branch new access control mechanisms and you would have to implement some access control login in your other system too. We simply do not have capability that would allow some external system to access all course enrolments because we would never need it in standard Moodle web interface.

        so we can:
        1/ replicate the logic we have in current web UI using current capabilities
        2/ or we have to invent new capability for "granting access to all server enrolments"

        Show
        Petr Škoda added a comment - Our current WS api is designed to run in a security context of one user - that makes it suitable for mobile applications or other single user applications. If you want to join moodle with other multiuser system it gets a lot lot trickier because we would need a branch new access control mechanisms and you would have to implement some access control login in your other system too. We simply do not have capability that would allow some external system to access all course enrolments because we would never need it in standard Moodle web interface. so we can: 1/ replicate the logic we have in current web UI using current capabilities 2/ or we have to invent new capability for "granting access to all server enrolments"
        Hide
        Heiko Schach added a comment -

        To expand upon Antero's comment, we are planning to let our external system access moodle as dedicated web services user with its own capabilities as described in Moodle docs: http://docs.moodle.org/en/How_to_enable_web_services_for_an_external_system
        The goal of this function for us would be to retrieve a list of all courses on the site a user is enrolled in, somewhat like the output on moodle's user profile (e.g. /user/view.php?id=2&course=1).

        Show
        Heiko Schach added a comment - To expand upon Antero's comment, we are planning to let our external system access moodle as dedicated web services user with its own capabilities as described in Moodle docs: http://docs.moodle.org/en/How_to_enable_web_services_for_an_external_system The goal of this function for us would be to retrieve a list of all courses on the site a user is enrolled in, somewhat like the output on moodle's user profile (e.g. /user/view.php?id=2&course=1).
        Hide
        Petr Škoda added a comment -

        Patch was submitted for review, it works the same as list of enrolled courses on the users profile page. It is not intended for synchronisation of enrolments between different systems because it does course level access control and respects all capability overrides at lower context level.

        This new method should be suitable for mobile applications that run in the context of a real user - it may ask what courses is user enrolled in or you can query information about other users. The empty result does not mean that user is not enrolled - you may not be allowed to know the courses exist or you are not allowed to access list of participants.

        For system level synchronisation I would personally recommend to write custom local plugin or even enrol plugin with specialised external methods protected by new capabilities.

        Petr

        Show
        Petr Škoda added a comment - Patch was submitted for review, it works the same as list of enrolled courses on the users profile page. It is not intended for synchronisation of enrolments between different systems because it does course level access control and respects all capability overrides at lower context level. This new method should be suitable for mobile applications that run in the context of a real user - it may ask what courses is user enrolled in or you can query information about other users. The empty result does not mean that user is not enrolled - you may not be allowed to know the courses exist or you are not allowed to access list of participants. For system level synchronisation I would personally recommend to write custom local plugin or even enrol plugin with specialised external methods protected by new capabilities. Petr
        Hide
        Martin Dougiamas added a comment -

        +1 to Petr

        Web service functions must necessarily run as a user and capabilities must respect and mirror what the UI does.

        Show
        Martin Dougiamas added a comment - +1 to Petr Web service functions must necessarily run as a user and capabilities must respect and mirror what the UI does.
        Hide
        Jérôme Mouneyrac added a comment -

        Just a little fix, the function should be called get_user_courses() if it's not a bulk function I checked the rest, it's ok
        Thanks

        Show
        Jérôme Mouneyrac added a comment - Just a little fix, the function should be called get_user_courses() if it's not a bulk function I checked the rest, it's ok Thanks
        Hide
        Petr Škoda added a comment -

        It uses the same logic as enrol_get_users_courses meaning "user's courses", feel free to file new issue and change anything there, it is still in dev branch. Thanks.

        Show
        Petr Škoda added a comment - It uses the same logic as enrol_get_users_courses meaning "user's courses", feel free to file new issue and change anything there, it is still in dev branch. Thanks.
        Hide
        Jérôme Mouneyrac added a comment -

        Ah I understand now. It's ok.

        Show
        Jérôme Mouneyrac added a comment - Ah I understand now. It's ok.

          People

          • Votes:
            5 Vote for this issue
            Watchers:
            6 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved: