Moodle
  1. Moodle
  2. MDL-33770

Web service: create_groups should not require enrolmentkey

    Details

      Description

      create_groups_parameters should not require enrolmentkey

        Gliffy Diagrams

          Issue Links

            Activity

            Hide
            Sam Hemelryk added a comment -

            Thanks Jerome, this has been integrated now.
            Could you please create some test instructions ASAP. I think all that needs to happen is someone use the external function to create a group and not specify an enrolment key.

            Cheers
            Sam

            Show
            Sam Hemelryk added a comment - Thanks Jerome, this has been integrated now. Could you please create some test instructions ASAP. I think all that needs to happen is someone use the external function to create a group and not specify an enrolment key. Cheers Sam
            Hide
            David Monllaó added a comment -

            Hi,

            Tested following the instructions without the patch to reproduce the problem and with the patch on 2.3, 2.2 and 2.1 with same results; the group/s is/are created, but the response requires a value for the enrolmentkey field, attaching server response:

            2.3 and 2.2

            <?xml version="1.0" encoding="UTF-8" ?>
            <EXCEPTION class="invalid_response_exception">
            <ERRORCODE>invalidresponse</ERRORCODE>
            <MESSAGE>Invalid response value detected</MESSAGE>
            <DEBUGINFO>Error in response - Missing following required key in a single structure: enrolmentkey</DEBUGINFO>
            </EXCEPTION>
            

            2.1

            <?xml version="1.0" encoding="UTF-8" ?>
            <RESPONSE>
            <MULTIPLE>
            <SINGLE>
            <KEY name="id"><VALUE>1</VALUE>
            </KEY>
            <KEY name="courseid"><VALUE>2</VALUE>
            </KEY>
            <KEY name="name"><VALUE>a</VALUE>
            </KEY>
            <KEY name="description"><VALUE>descroiption</VALUE>
            </KEY>
            <ERROR>Missing required key "enrolmentkey"</ERROR></SINGLE>
            </MULTIPLE>
            </RESPONSE>
            

            Show
            David Monllaó added a comment - Hi, Tested following the instructions without the patch to reproduce the problem and with the patch on 2.3, 2.2 and 2.1 with same results; the group/s is/are created, but the response requires a value for the enrolmentkey field, attaching server response: 2.3 and 2.2 <? xml version = "1.0" encoding = "UTF-8" ?> < EXCEPTION class = "invalid_response_exception" > < ERRORCODE >invalidresponse</ ERRORCODE > < MESSAGE >Invalid response value detected</ MESSAGE > < DEBUGINFO >Error in response - Missing following required key in a single structure: enrolmentkey</ DEBUGINFO > </ EXCEPTION > 2.1 <? xml version = "1.0" encoding = "UTF-8" ?> < RESPONSE > < MULTIPLE > < SINGLE > < KEY name = "id" >< VALUE >1</ VALUE > </ KEY > < KEY name = "courseid" >< VALUE >2</ VALUE > </ KEY > < KEY name = "name" >< VALUE >a</ VALUE > </ KEY > < KEY name = "description" >< VALUE >descroiption</ VALUE > </ KEY > < ERROR >Missing required key "enrolmentkey"</ ERROR ></ SINGLE > </ MULTIPLE > </ RESPONSE >
            Hide
            Sam Hemelryk added a comment -

            Thanks for picking that up David, I've run through things this morning, identified the issue, and pushed a fix up to integration now.
            Could you please test again if you are online. Otherwise I'll get Jerome or someone else at HQ to test as I think we are releasing the weeklies in a few hours.

            Cheers
            Sam

            Show
            Sam Hemelryk added a comment - Thanks for picking that up David, I've run through things this morning, identified the issue, and pushed a fix up to integration now. Could you please test again if you are online. Otherwise I'll get Jerome or someone else at HQ to test as I think we are releasing the weeklies in a few hours. Cheers Sam
            Hide
            Sam Hemelryk added a comment -

            By the way, the problem was that if enrolmentkey was not specified then the enrolmentkey property would not be set on the return value (because its just returning what was provided).
            The solution I've implemented was to set it to an empty string if it is not set before returning. This mimics what would occur if the group was fetched from the DB which would be a better solution but also a larger change.
            I've created MDL-34180 to see this change made.

            Show
            Sam Hemelryk added a comment - By the way, the problem was that if enrolmentkey was not specified then the enrolmentkey property would not be set on the return value (because its just returning what was provided). The solution I've implemented was to set it to an empty string if it is not set before returning. This mimics what would occur if the group was fetched from the DB which would be a better solution but also a larger change. I've created MDL-34180 to see this change made.
            Hide
            David Monllaó added a comment -

            Sorry Sam, I just read it, start testing

            Show
            David Monllaó added a comment - Sorry Sam, I just read it, start testing
            Hide
            David Monllaó added a comment -

            Sam, pulled from integration and tested on 2.3, 2.2 and 2.1. It passes everywhere.

            2.1 response

            <?xml version="1.0" encoding="UTF-8" ?>
            <RESPONSE>
            <MULTIPLE>
            <SINGLE>
            <KEY name="id"><VALUE>6</VALUE>
            </KEY>
            <KEY name="courseid"><VALUE>2</VALUE>
            </KEY>
            <KEY name="name"><VALUE>ass</VALUE>
            </KEY>
            <KEY name="description"><VALUE>descroiption</VALUE>
            </KEY>
            <KEY name="enrolmentkey"><VALUE></VALUE>
            </KEY>
            </SINGLE>
            </MULTIPLE>
            </RESPONSE>
            

            Show
            David Monllaó added a comment - Sam, pulled from integration and tested on 2.3, 2.2 and 2.1. It passes everywhere. 2.1 response <? xml version = "1.0" encoding = "UTF-8" ?> < RESPONSE > < MULTIPLE > < SINGLE > < KEY name = "id" >< VALUE >6</ VALUE > </ KEY > < KEY name = "courseid" >< VALUE >2</ VALUE > </ KEY > < KEY name = "name" >< VALUE >ass</ VALUE > </ KEY > < KEY name = "description" >< VALUE >descroiption</ VALUE > </ KEY > < KEY name = "enrolmentkey" >< VALUE ></ VALUE > </ KEY > </ SINGLE > </ MULTIPLE > </ RESPONSE >
            Hide
            Jérôme Mouneyrac added a comment -

            Thanks for picking that up David, a good thing I didn't suggest to test with my PHPunit test, I'll look why my test didn't fail (https://github.com/mouneyrac/moodle/commit/9dbe2047d4695ac71bef9d6626bf8890d38b37de). Thanks Sam for patching.

            PS: when implementing the ws function, the right way would be to set the return value as VALUE_OPTIONAL, but for backward compatibility Sam could not change it anymore here.

            Show
            Jérôme Mouneyrac added a comment - Thanks for picking that up David, a good thing I didn't suggest to test with my PHPunit test, I'll look why my test didn't fail ( https://github.com/mouneyrac/moodle/commit/9dbe2047d4695ac71bef9d6626bf8890d38b37de ). Thanks Sam for patching. PS: when implementing the ws function, the right way would be to set the return value as VALUE_OPTIONAL, but for backward compatibility Sam could not change it anymore here.
            Hide
            Sam Hemelryk added a comment -

            Congratulations your code is upstream - gold star for you!

            This issue + 79 others made it in in time for the minor releases.
            Thank you everyone involved for your exuberant efforts.

            Show
            Sam Hemelryk added a comment - Congratulations your code is upstream - gold star for you! This issue + 79 others made it in in time for the minor releases. Thank you everyone involved for your exuberant efforts.

              People

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

                Dates

                • Created:
                  Updated:
                  Resolved: