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

New categories are added in externallib as the first in parent instead as the last

    Details

    • Type: Bug
    • Status: Closed
    • Priority: Minor
    • Resolution: Fixed
    • Affects Version/s: 2.4.1
    • Fix Version/s: 2.3.5, 2.4.2
    • Component/s: Web Services
    • Labels:

      Description

      core_course_external::create_categories() sets the sortorder for new category to 999 which adds them in the reverse order

        Gliffy Diagrams

          Attachments

            Issue Links

              Activity

              Hide
              marina Marina Glancy added a comment -

              Jerome, please take a look at this issue. Also if you think it is worth picking up in previous versions, let me know

              Show
              marina Marina Glancy added a comment - Jerome, please take a look at this issue. Also if you think it is worth picking up in previous versions, let me know
              Hide
              jerome Jérôme Mouneyrac added a comment -

              I'm just reading the title of the issue, and it's saying that we need to change the behavior of the function. From what I guess, you are changing the behavior in Moodle master.

              About backport:
              If the previous fix didn't match Moodle behavior and this one does, then you can backport. Otherwise no (if my guess is correct, then don't backport).

              About the change itself:
              Changing behavior mean changing client behavior. However external function behavior should match in Moodle bahvior. If my guess is correct => Dilemna. In this case two solution:

              • it's not an important change (the client will rarely care) => you can edit the function (I think here sorting on category creation is not that important, so your fix is correct)
              • if it's important => then we need to create a new function and maybe deprecate the current one (I don't think it's our case)

              About the code logic:
              I didn't look about this 999 weird value. I think you know better than me about category, I trust you

              Conclusion:
              is my guess is correct, I think it's good and you can just mention the API changes in http://docs.moodle.org/dev/Web_services_API_Changes

              Show
              jerome Jérôme Mouneyrac added a comment - I'm just reading the title of the issue, and it's saying that we need to change the behavior of the function. From what I guess, you are changing the behavior in Moodle master. About backport: If the previous fix didn't match Moodle behavior and this one does, then you can backport. Otherwise no (if my guess is correct, then don't backport). About the change itself: Changing behavior mean changing client behavior. However external function behavior should match in Moodle bahvior. If my guess is correct => Dilemna. In this case two solution: it's not an important change (the client will rarely care) => you can edit the function (I think here sorting on category creation is not that important, so your fix is correct) if it's important => then we need to create a new function and maybe deprecate the current one (I don't think it's our case) About the code logic: I didn't look about this 999 weird value. I think you know better than me about category, I trust you Conclusion: is my guess is correct, I think it's good and you can just mention the API changes in http://docs.moodle.org/dev/Web_services_API_Changes
              Hide
              marina Marina Glancy added a comment -

              Jerome, I did not understand you.
              What is "your guess" and what is "previous fix"?
              So, the resume is that I commit it to master only and mention in dev docs?

              Show
              marina Marina Glancy added a comment - Jerome, I did not understand you. What is "your guess" and what is "previous fix"? So, the resume is that I commit it to master only and mention in dev docs?
              Hide
              jerome Jérôme Mouneyrac added a comment -

              I'm guessing you are changing the function behavior because you changed the front end behavior. Or is it a fix because the function behavior does not match the front end behavior?

              Show
              jerome Jérôme Mouneyrac added a comment - I'm guessing you are changing the function behavior because you changed the front end behavior. Or is it a fix because the function behavior does not match the front end behavior?
              Hide
              marina Marina Glancy added a comment -

              this is a fix because the externallib function behaviour does not match the front end behaviour

              Show
              marina Marina Glancy added a comment - this is a fix because the externallib function behaviour does not match the front end behaviour
              Hide
              jerome Jérôme Mouneyrac added a comment -

              Ah Ok. So +1 to backport it. For client point of view, I don't think it's a big deal to change the sorting order when creating a category - and I don't think the web service function is massively used either atm. For the testing instruction add PHPunit test cmd line, all good, thank you Marina

              Show
              jerome Jérôme Mouneyrac added a comment - Ah Ok. So +1 to backport it. For client point of view, I don't think it's a big deal to change the sorting order when creating a category - and I don't think the web service function is massively used either atm. For the testing instruction add PHPunit test cmd line, all good, thank you Marina
              Hide
              samhemelryk Sam Hemelryk added a comment -

              Thanks Marina - this has been integrated now.

              Show
              samhemelryk Sam Hemelryk added a comment - Thanks Marina - this has been integrated now.
              Hide
              abgreeve Adrian Greeve added a comment -

              Tested on integration 2.3, 2.4 and master.
              The second category appears in the list after the first category.
              No problems.
              Test passed.

              Show
              abgreeve Adrian Greeve added a comment - Tested on integration 2.3, 2.4 and master. The second category appears in the list after the first category. No problems. Test passed.
              Hide
              stronk7 Eloy Lafuente (stronk7) added a comment -

              Because

              A
              MARVELOUS
              A       U
              Z  YOU  P
              I  ARE  E
              N  PPL  R
              G       B
                TNKS! 
              

              Closing, ciao

              Show
              stronk7 Eloy Lafuente (stronk7) added a comment - Because A MARVELOUS A U Z YOU P I ARE E N PPL R G B TNKS! Closing, ciao

                People

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

                  Dates

                  • Created:
                    Updated:
                    Resolved:
                    Fix Release Date:
                    11/Mar/13