Moodle
  1. Moodle
  2. MDL-38144

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

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Minor 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

          Issue Links

            Activity

            Hide
            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 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
            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
            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 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 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
            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
            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 Glancy added a comment -

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

            Show
            Marina Glancy added a comment - this is a fix because the externallib function behaviour does not match the front end behaviour
            Hide
            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
            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
            Sam Hemelryk added a comment -

            Thanks Marina - this has been integrated now.

            Show
            Sam Hemelryk added a comment - Thanks Marina - this has been integrated now.
            Hide
            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
            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
            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
            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: