Moodle
  1. Moodle
  2. MDL-33770

Web service: create_groups should not require enrolmentkey

    Details

    • Rank:
      41805

      Description

      create_groups_parameters should not require enrolmentkey

        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: