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
    • Rank:
      17252

      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.

        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: