Details
-
Type:
Bug
-
Status: Closed
-
Priority:
Minor
-
Resolution: Fixed
-
Affects Version/s: 2.1.2
-
Component/s: Web Services
-
Labels:
-
Testing Instructions:
-
Affected Branches:MOODLE_21_STABLE
-
Fixed Branches:MOODLE_21_STABLE, MOODLE_22_STABLE
-
Pull from Repository:
-
Pull Master Branch:
-
Pull Master Diff URL:
Description
Noticed the following error while testing MDL-30045, to create new token for a student:
Did you remember to make the first column something unique in your call to get_records? Duplicate value '766' found in column 'id'.
|
|
|
line 818 of /lib/dml/mysqli_native_moodle_database.php: call to debugging()
|
line 364 of /webservice/lib.php: call to mysqli_native_moodle_database->get_records_sql()
|
line 391 of /webservice/lib.php: call to webservice->get_user_capabilities()
|
line 7550 of /lib/adminlib.php: call to webservice->get_missing_capabilities_by_users()
|
line 1343 of /lib/adminlib.php: call to admin_setting_managewebservicetokens->output_html()
|
line 126 of /admin/settings.php: call to admin_settingpage->output_html()
|
Gliffy Diagrams
Attachments
Issue Links
- has a non-specific relationship to
-
MDL-31253 META - web service improvements for 2.3
-
- Closed
-
- has been marked as being related by
-
-
- Closed
-
-
MDL-29962 Informing administrator about "required" capabilities to run a web service function
-
- Open
-
- Testing discovered
-
MDL-30045 Backport 2.2 changes for login/token.php in 2.1
-
- Closed
-
Activity
- All
- Comments
- Work Log
- History
- Activity
- Links Hierarchy
Hi Jerome,
This looks good.
However, I'm just wondering if it is necessary to have rc.capability for group by. Removing it from the sql, produces the same output. Maybe this is used for multiDB compability.
As Rosie, I don't know if rc.id is enough (cross-db compatible). I don't think it is a bad thing to group on both fields as it's what I try to obtain anyway. So I let it like that 
Submitting for integration.
I let you cherry-pick this one line commit to previous branches 
(I don't think it make you any more work that you have to do for integration, and it saves me some)
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
Hi Jerome,
I do not think that this group by SQL is correct - instead it should use DISTINCT.
But more than this, the SQL does not seem to make sense, it is retrieving any capability in any context. So if someone has some capability in a block or a PROHIBIT capabilty then this will be returned. The function document even mentions "(with context)" but the context isn't considered at all.
So I think that the functionality of this function also needs to be considered
Hi Dan, I do understand that the SQL does not make sens. The goal is to deliver a help for admin (to have an idea at what capabilities the admin could be missing - before they were left without any clue at all and it was a nightmare for them without looking at the code). We didn't want to add any more complexity to the services.php mentioning cap/context couple, so we just added a plain capabilities text. I agree we miss some cap sometimes (when they don't have access on the context used by the function but do have the cap on other context). There is definitively big improvement to do in this domain, I'll write another issue for that.
Note there is also an existing issue that will refactor the capability part of services.php.
I tried first with DISTINCT but I gave up on it because I think it was not cross-db if I remember, I'll have a look again.
Hi Dan, I linked two issues where you should find more explaination about the 'capabilities' field in service.php. you are welcome to participate to the topic
Hi I did the requested changes, rebased and retested. All good. Thanks for your review Dan. Resubmitting.
Thanks Jerome, this has been integrated. Noting that this id a documentation function only
Ah I forgot to change the phpdoc of the function!
Stopping testing as I have to go for a few hours. Jerome, when you say to add a new web service do you mean as is described at http://docs.moodle.org/22/en/Using_web_services#Creating_a_service
Yes that's it
Dan I added a commit: https://github.com/mouneyrac/moodle/commit/8a2705e64032dd84b4a871ca47e19c1564d0fe5f
Do you have time to add it?
if no I'll create another issue for that (Andrew you can keep going testing this issue )
Hi Jerome, i've added that (to master only)
thank you Dan
Works as described. Passing.
Bonza mate!
Your changes have made it into the Moodle release! Its time to celebrate! Put a shrimp on the barbie and grab a stubby.
Hooroo
ping for peer review