Moodle

Should be in dev backlog, right?

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

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

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

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

Ok commit updated with latest changes.

To summarize what is left to do:

• MDL-27665
• MDL-27608
• MDL-27609

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

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

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.

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

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.

Jerome this needs to be rebased

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

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

Rebased It was easier that I though it would be.

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

As requested, reopening...

ok fixed, rebased, whitespaces removed. Cheers

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

Thanks Sam

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...

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 ?

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.

Passing as suggested, thanks!

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

Closing.