Uploaded image for project: 'Moodle'
  1. Moodle
  2. MDL-30225

Creating new web service tokens create error

    Details

    • Type: Bug
    • Status: Closed
    • Priority: Minor
    • Resolution: Fixed
    • Affects Version/s: 2.1.2
    • Fix Version/s: 2.1.6, 2.2.3
    • Component/s: Web Services
    • Labels:
    • Testing Instructions:
      Hide

      Add a new web service in the admin, and add all functions to this service. Create a token for a user non admin. Check that the error message is not displayed on the token admin page.

      Show
      Add a new web service in the admin, and add all functions to this service. Create a token for a user non admin. Check that the error message is not displayed on the token admin page.
    • Affected Branches:
      MOODLE_21_STABLE
    • Fixed Branches:
      MOODLE_21_STABLE, MOODLE_22_STABLE
    • Pull Master Branch:

      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

          Issue Links

            Activity

            Hide
            jerome Jérôme Mouneyrac added a comment -

            ping for peer review

            Show
            jerome Jérôme Mouneyrac added a comment - ping for peer review
            Hide
            rwijaya Rossiani Wijaya added a comment -

            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.

            Show
            rwijaya Rossiani Wijaya added a comment - 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.
            Hide
            jerome Jérôme Mouneyrac added a comment -

            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.

            Show
            jerome Jérôme Mouneyrac added a comment - 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.
            Hide
            jerome Jérôme Mouneyrac added a comment - - edited

            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)

            Show
            jerome Jérôme Mouneyrac added a comment - - edited 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)
            Hide
            poltawski Dan Poltawski added a comment -

            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

            Show
            poltawski Dan Poltawski added a comment - 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
            Hide
            poltawski Dan Poltawski added a comment -

            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

            Show
            poltawski Dan Poltawski added a comment - 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
            Hide
            jerome Jérôme Mouneyrac added a comment -

            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.

            Show
            jerome Jérôme Mouneyrac added a comment - 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.
            Hide
            jerome Jérôme Mouneyrac added a comment -

            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

            Show
            jerome Jérôme Mouneyrac added a comment - 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
            Hide
            jerome Jérôme Mouneyrac added a comment -

            Hi I did the requested changes, rebased and retested. All good. Thanks for your review Dan. Resubmitting.

            Show
            jerome Jérôme Mouneyrac added a comment - Hi I did the requested changes, rebased and retested. All good. Thanks for your review Dan. Resubmitting.
            Hide
            poltawski Dan Poltawski added a comment -

            Thanks Jerome, this has been integrated. Noting that this id a documentation function only

            Show
            poltawski Dan Poltawski added a comment - Thanks Jerome, this has been integrated. Noting that this id a documentation function only
            Hide
            jerome Jérôme Mouneyrac added a comment -

            Ah I forgot to change the phpdoc of the function!

            Show
            jerome Jérôme Mouneyrac added a comment - Ah I forgot to change the phpdoc of the function!
            Hide
            andyjdavis Andrew Davis added a comment -

            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

            Show
            andyjdavis Andrew Davis added a comment - 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
            Hide
            jerome Jérôme Mouneyrac added a comment -

            Yes that's it

            Show
            jerome Jérôme Mouneyrac added a comment - Yes that's it
            Hide
            jerome Jérôme Mouneyrac added a comment -
            Show
            jerome Jérôme Mouneyrac added a comment - Dan I added a commit: https://github.com/mouneyrac/moodle/commit/8a2705e64032dd84b4a871ca47e19c1564d0fe5f Do you have time to add it?
            Hide
            jerome Jérôme Mouneyrac added a comment - - edited

            if no I'll create another issue for that (Andrew you can keep going testing this issue )

            Show
            jerome Jérôme Mouneyrac added a comment - - edited if no I'll create another issue for that (Andrew you can keep going testing this issue )
            Hide
            poltawski Dan Poltawski added a comment -

            Hi Jerome, i've added that (to master only)

            Show
            poltawski Dan Poltawski added a comment - Hi Jerome, i've added that (to master only)
            Hide
            jerome Jérôme Mouneyrac added a comment -

            thank you Dan

            Show
            jerome Jérôme Mouneyrac added a comment - thank you Dan
            Hide
            andyjdavis Andrew Davis added a comment -

            Works as described. Passing.

            Show
            andyjdavis Andrew Davis added a comment - Works as described. Passing.
            Hide
            poltawski Dan Poltawski added a comment -

            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

            Show
            poltawski Dan Poltawski added a comment - 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

              People

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

                Dates

                • Created:
                  Updated:
                  Resolved:
                  Fix Release Date:
                  14/May/12