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:
    • Rank:
      47970

      Description

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

        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: