Details

    • Type: Sub-task Sub-task
    • Status: Closed
    • Priority: Critical Critical
    • Resolution: Fixed
    • Affects Version/s: 2.0.2
    • Fix Version/s: 2.1
    • Component/s: Web Services
    • Labels:
      None

      Description

      This function has many permissions problems (no matching the UI pages), misses many profile fields, or sometimes return incomplete information (custom fields). Refactor it.

        Gliffy Diagrams

          Issue Links

            Activity

            Hide
            Dongsheng Cai added a comment -

            Should be in dev backlog, right?

            Show
            Dongsheng Cai added a comment - Should be in dev backlog, right?
            Hide
            Jérôme Mouneyrac added a comment - - edited

            sorry there is a typo in the git commit comment MDL-26775 => MDL-26774 + it's profile not profil

            Show
            Jérôme Mouneyrac added a comment - - edited sorry there is a typo in the git commit comment MDL-26775 => MDL-26774 + it's profile not profil
            Hide
            Sam Hemelryk added a comment -

            Hi Jerome,

            I've just been having a look at this now and have noted the following things:

            General notes:

            • White space
            • I think you should store the result of is_siteadmin in a var and use the var rather than calling is_siteadmin over and over again (it explodes and then uses in array each time)
            • There are alot of database queries being made within the users foreach loop - I'm not too sure what the priority on this issue is but certainly if this doesn't need to get in as soon as possible you could GREATLY improve the performance of this webservice by absrtacting the database queries and performing them in bulk... perhaps should be looked into anyway... it may pay to talk to Martin about it.
              • Getting the users enrolled courses is one area that could be greatly improved.
              • Users shared enrolments is another area.

            Specific notes:

            • line 363: I wonder whether you would be better using a recordset for those instead of users_get_users_by_id ?
            • line 367: Better yet convert to get a recordset and use SQL to get the users context at the same time (halves the number of DB queries you are making)
            • line 391: No need for the !is_siteadmin check as we already know they are not a site admin
            • line 403 - 405: There is no need to fetch the categories - you fetch all categories and then fetch all fields for each category = all fields that have a valid category. You could either join to the category table of if the category had to be valid (NOT NULL) then selecting all fields is fine.
            • line 433: I'm guessing that the check a user has role assignments is wrong - what is that meant to be checking?
            • line 532: What is the purpose of the $mycourse->category check? Category should always be set.
            • line 558: Are you sure you want to deliver the user preferences? Some will be useful - alot won't be - and most relate to the state of the UI.

            Let me know if you have any questions otherwise I'll look at it again when you've got it ready again.

            Cheers
            Sam

            Show
            Sam Hemelryk added a comment - Hi Jerome, I've just been having a look at this now and have noted the following things: General notes: White space I think you should store the result of is_siteadmin in a var and use the var rather than calling is_siteadmin over and over again (it explodes and then uses in array each time) There are alot of database queries being made within the users foreach loop - I'm not too sure what the priority on this issue is but certainly if this doesn't need to get in as soon as possible you could GREATLY improve the performance of this webservice by absrtacting the database queries and performing them in bulk... perhaps should be looked into anyway... it may pay to talk to Martin about it. Getting the users enrolled courses is one area that could be greatly improved. Users shared enrolments is another area. Specific notes: line 363: I wonder whether you would be better using a recordset for those instead of users_get_users_by_id ? line 367: Better yet convert to get a recordset and use SQL to get the users context at the same time (halves the number of DB queries you are making) line 391: No need for the !is_siteadmin check as we already know they are not a site admin line 403 - 405: There is no need to fetch the categories - you fetch all categories and then fetch all fields for each category = all fields that have a valid category. You could either join to the category table of if the category had to be valid (NOT NULL) then selecting all fields is fine. line 433: I'm guessing that the check a user has role assignments is wrong - what is that meant to be checking? line 532: What is the purpose of the $mycourse->category check? Category should always be set. line 558: Are you sure you want to deliver the user preferences? Some will be useful - alot won't be - and most relate to the state of the UI. Let me know if you have any questions otherwise I'll look at it again when you've got it ready again. Cheers Sam
            Hide
            Jérôme Mouneyrac added a comment -

            Thanks for the review Sam.

            • I'm fixing white spaces and siteadmin problems right now.
            • 433: it matches the current UI code. Agree it doesn't make much sens, I'll write an issue for the front end.
            • 403-405: I'll check it later and create an issue for the generic class if necessary.
            • 532: I'll check it later and create an issue for the front end if necessary.
            • 558: we did for create/update, I guess get should get it too. It was originally requested by Martin for the create/update functions.
            • for SQL performance problems: agree it can be greatly improved. It is currently a simple match of the existing code in Moodle.

            What about creating an issue for the performance problems of this issue? I don't mind to work on it all, I just have really limited time till 15th June and many tasks, I'll talk to Martin

            Show
            Jérôme Mouneyrac added a comment - Thanks for the review Sam. I'm fixing white spaces and siteadmin problems right now. 433: it matches the current UI code. Agree it doesn't make much sens, I'll write an issue for the front end. 403-405: I'll check it later and create an issue for the generic class if necessary. 532: I'll check it later and create an issue for the front end if necessary. 558: we did for create/update, I guess get should get it too. It was originally requested by Martin for the create/update functions. for SQL performance problems: agree it can be greatly improved. It is currently a simple match of the existing code in Moodle. What about creating an issue for the performance problems of this issue? I don't mind to work on it all, I just have really limited time till 15th June and many tasks, I'll talk to Martin
            Hide
            Jérôme Mouneyrac added a comment - - edited

            I'll commit some changes in github:

            • no white spaces
            • no duplicated issiteadmin calls

            433: removed the role_assignements checking that doesn't make sens - I wrote an issue : MDL-27607
            403-405: wrote an issue: MDL-27608
            532: wrote an issue: MDL-27609

            391: get rid of isadmin totaly as cap checking return true for admin anyway

            363: will be fixed by the improvement Sam suggested first and which really worth to be implemented.

            367: totaly worth it too.

            line 367: fixed. I discovered that get_context_by_instance accept array of ids

            Bonus: get rid of $class = ''; not used

            Show
            Jérôme Mouneyrac added a comment - - edited I'll commit some changes in github: no white spaces no duplicated issiteadmin calls 433: removed the role_assignements checking that doesn't make sens - I wrote an issue : MDL-27607 403-405: wrote an issue: MDL-27608 532: wrote an issue: MDL-27609 391: get rid of isadmin totaly as cap checking return true for admin anyway 363: will be fixed by the improvement Sam suggested first and which really worth to be implemented. 367: totaly worth it too. line 367: fixed. I discovered that get_context_by_instance accept array of ids Bonus: get rid of $class = ''; not used
            Hide
            Jérôme Mouneyrac added a comment -

            Ok commit updated with latest changes.

            Show
            Jérôme Mouneyrac added a comment - Ok commit updated with latest changes.
            Hide
            Jérôme Mouneyrac added a comment - - edited

            To summarize what is left to do:

            Show
            Jérôme Mouneyrac added a comment - - edited To summarize what is left to do: MDL-27665 MDL-27608 MDL-27609
            Hide
            Jérôme Mouneyrac added a comment - - edited

            Sam can you review again (the general performance thing will be done later). Cheers.

            Show
            Jérôme Mouneyrac added a comment - - edited Sam can you review again (the general performance thing will be done later). Cheers.
            Hide
            Jérôme Mouneyrac added a comment -

            If it gets integrated in this state I'll write a review for the main performance issue.

            Show
            Jérôme Mouneyrac added a comment - If it gets integrated in this state I'll write a review for the main performance issue.
            Hide
            Jérôme Mouneyrac added a comment -

            Sam I browse these commit files, I don't think cherry-pick would cause any conflict during peer review. If you have any problem, let me know, I'll rebase it. Cheers.

            Show
            Jérôme Mouneyrac added a comment - Sam I browse these commit files, I don't think cherry-pick would cause any conflict during peer review. If you have any problem, let me know, I'll rebase it. Cheers.
            Hide
            Jérôme Mouneyrac added a comment -

            I peer review myself : need to use the correct profile image method

            Show
            Jérôme Mouneyrac added a comment - I peer review myself : need to use the correct profile image method
            Hide
            Jérôme Mouneyrac added a comment -

            Back in peer review status. Note that I created an issue MDL-27676 for default small profile url redirecting to a big profile image. Thanks.

            Show
            Jérôme Mouneyrac added a comment - Back in peer review status. Note that I created an issue MDL-27676 for default small profile url redirecting to a big profile image. Thanks.
            Hide
            Sam Hemelryk added a comment -

            Jerome this needs to be rebased

            Show
            Sam Hemelryk added a comment - Jerome this needs to be rebased
            Hide
            Sam Hemelryk added a comment -

            Just needs rebasing - still lots that could be done for performance but issues created so all good

            Show
            Sam Hemelryk added a comment - Just needs rebasing - still lots that could be done for performance but issues created so all good
            Hide
            Jérôme Mouneyrac added a comment - - edited

            oh crap I already rebased last time Ok I'll rebase it again.

            Show
            Jérôme Mouneyrac added a comment - - edited oh crap I already rebased last time Ok I'll rebase it again.
            Hide
            Jérôme Mouneyrac added a comment -

            Rebased It was easier that I though it would be.

            Show
            Jérôme Mouneyrac added a comment - Rebased It was easier that I though it would be.
            Hide
            Jérôme Mouneyrac added a comment -

            Please reject, need more works on username/firstname/lastname

            Show
            Jérôme Mouneyrac added a comment - Please reject, need more works on username/firstname/lastname
            Hide
            Eloy Lafuente (stronk7) added a comment -

            As requested, reopening...

            Show
            Eloy Lafuente (stronk7) added a comment - As requested, reopening...
            Hide
            Jérôme Mouneyrac added a comment -

            ok fixed, rebased, whitespaces removed. Cheers

            Show
            Jérôme Mouneyrac added a comment - ok fixed, rebased, whitespaces removed. Cheers
            Hide
            Sam Hemelryk added a comment -

            Thanks Jerome - this has been integrated now... hoorah

            Show
            Sam Hemelryk added a comment - Thanks Jerome - this has been integrated now... hoorah
            Hide
            Jérôme Mouneyrac added a comment -

            Thanks Sam

            Show
            Jérôme Mouneyrac added a comment - Thanks Sam
            Hide
            Rajesh Taneja added a comment -

            Works Grt for admin user but getting error for teacher.
            <?xml version="1.0" encoding="UTF-8" ?>
            <EXCEPTION class="webservice_access_exception">
            <MESSAGE>Access control exception</MESSAGE>
            <DEBUGINFO>Access to web service not allowed</DEBUGINFO>
            </EXCEPTION>

            I have added roles and there is no permission issue there...

            Show
            Rajesh Taneja added a comment - Works Grt for admin user but getting error for teacher. <?xml version="1.0" encoding="UTF-8" ?> <EXCEPTION class="webservice_access_exception"> <MESSAGE>Access control exception</MESSAGE> <DEBUGINFO>Access to web service not allowed</DEBUGINFO> </EXCEPTION> I have added roles and there is no permission issue there...
            Hide
            Martin Dougiamas added a comment -

            OK, bit of a big problem here.

            This function is really designed for site-level users profiles (user/profile.php), and yet the mobile app requires course-level profile (/user/view.php).

            After discussion with Dongsheng we decided on a new web service function instead of hacking/extending this one.

            I'm guessing moodle_user_get_course_participants_by_id ?

            Show
            Martin Dougiamas added a comment - OK, bit of a big problem here. This function is really designed for site-level users profiles (user/profile.php), and yet the mobile app requires course-level profile (/user/view.php). After discussion with Dongsheng we decided on a new web service function instead of hacking/extending this one. I'm guessing moodle_user_get_course_participants_by_id ?
            Hide
            Martin Dougiamas added a comment -

            DS, can you link the new issue here about the new function?

            Eloy, can you pass this one please? The function as it is works correctly.

            Show
            Martin Dougiamas added a comment - DS, can you link the new issue here about the new function? Eloy, can you pass this one please? The function as it is works correctly.
            Hide
            Eloy Lafuente (stronk7) added a comment -

            Passing as suggested, thanks!

            Show
            Eloy Lafuente (stronk7) added a comment - Passing as suggested, thanks!
            Hide
            Eloy Lafuente (stronk7) added a comment -

            And now this is part of the best Moodle weeklies ever, thanks!

            Closing.

            Show
            Eloy Lafuente (stronk7) added a comment - And now this is part of the best Moodle weeklies ever, thanks! Closing.

              People

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

                Dates

                • Created:
                  Updated:
                  Resolved: