Details

    • Type: Improvement Improvement
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: 2.3
    • Fix Version/s: 2.3
    • Component/s: Course, Web Services
    • Labels:
    • Rank:
      24712

      Description

      Implementation for core_course_create_categories()

      1. smurf.xml
        24 kB
        Eloy Lafuente (stronk7)

        Issue Links

          Activity

          Hide
          Fábio Souto added a comment -

          I request a mantainer review and merge of the implementation I developed for this webservice method
          https://github.com/fabiomsouto/moodle/tree/MDL-30081

          Show
          Fábio Souto added a comment - I request a mantainer review and merge of the implementation I developed for this webservice method https://github.com/fabiomsouto/moodle/tree/MDL-30081
          Hide
          Jérôme Mouneyrac added a comment -

          Thanks Fabio, I hope to review your contribution soon after January. As I suggested in another issue, let us know what UI files you took example from. Every external function must replicate capability checks (and other checks) that the Moodle UI does. It's easier for me to know where to/you look at. Cheers.

          Show
          Jérôme Mouneyrac added a comment - Thanks Fabio, I hope to review your contribution soon after January. As I suggested in another issue, let us know what UI files you took example from. Every external function must replicate capability checks (and other checks) that the Moodle UI does. It's easier for me to know where to/you look at. Cheers.
          Hide
          Fábio Souto added a comment -

          Hello,

          Thank you for the reply.

          The code I took inspiration from is located at course/editcategory.php
          There is the logic for creating a category, at lines 18-29 and 74-109 aproximately.

          Show
          Fábio Souto added a comment - Hello, Thank you for the reply. The code I took inspiration from is located at course/editcategory.php There is the logic for creating a category, at lines 18-29 and 74-109 aproximately.
          Hide
          Jérôme Mouneyrac added a comment -

          Hi Fabio, just some tmp modifications (waiting for get_categories to get integrated as most change will most likely need to be done here too): https://github.com/mouneyrac/moodle/commit/77e73c4e4052d5240f829c593ed286fa9bf4334f

          Show
          Jérôme Mouneyrac added a comment - Hi Fabio, just some tmp modifications (waiting for get_categories to get integrated as most change will most likely need to be done here too): https://github.com/mouneyrac/moodle/commit/77e73c4e4052d5240f829c593ed286fa9bf4334f
          Hide
          Fábio Souto added a comment -

          Hello,

          Yes I can see I've missed some things!

          Offtopic:
          I may write the remaining tests for the other code that I've submitted (namely delete categories and get users), if it's OK for you. Probably it won't be as accurate as yours though. Would it help?

          Thanks,
          Fabio

          Show
          Fábio Souto added a comment - Hello, Yes I can see I've missed some things! Offtopic: I may write the remaining tests for the other code that I've submitted (namely delete categories and get users), if it's OK for you. Probably it won't be as accurate as yours though. Would it help? Thanks, Fabio
          Hide
          Jérôme Mouneyrac added a comment -

          I'm happy you are still with us Fabio, it didn't discourage you. Actually it will be very good for me if you do all the changes, I'm a bit busy with ws/hub/mobile. If it's ok with your time I could just peer-review without doing any changes in your two next functions, and let you fix them? For the update_categories function it's going to be very similar to this create_categories function. Also they are easier than get_categories because there are only one place in the UI where you can create categories (from what I know).

          Show
          Jérôme Mouneyrac added a comment - I'm happy you are still with us Fabio, it didn't discourage you. Actually it will be very good for me if you do all the changes, I'm a bit busy with ws/hub/mobile. If it's ok with your time I could just peer-review without doing any changes in your two next functions, and let you fix them? For the update_categories function it's going to be very similar to this create_categories function. Also they are easier than get_categories because there are only one place in the UI where you can create categories (from what I know).
          Hide
          Fábio Souto added a comment -

          Hello,

          Yeah, no problem for me. I'll start working on them later this week. I'll keep in touch.

          Cheers,
          Fabio

          Show
          Fábio Souto added a comment - Hello, Yeah, no problem for me. I'll start working on them later this week. I'll keep in touch. Cheers, Fabio
          Hide
          Fábio Souto added a comment -

          So I would like to continue making changes to what you've done but... I'm having some git doubts.
          Should I copy your branch to my repository? Should I clone your repository? I can't merge your copy with mine because I was adding this to MDL_22_STABLE but you're doing it on the master branch. What is the usual workflow? (sorry if this seems a noobish question but it's better to ask

          Cheers,
          Fabio

          Show
          Fábio Souto added a comment - So I would like to continue making changes to what you've done but... I'm having some git doubts. Should I copy your branch to my repository? Should I clone your repository? I can't merge your copy with mine because I was adding this to MDL_22_STABLE but you're doing it on the master branch. What is the usual workflow? (sorry if this seems a noobish question but it's better to ask Cheers, Fabio
          Hide
          Jérôme Mouneyrac added a comment -

          Hi Fabio, yes if you can finish the issue, it is great for me (PS: I'm in holidays next week).

          To get you back on track, I know this way:
          1- git checkout master
          2- git pull upstream master //to get an up to date local master. if you don't have upstream remote repository, you need to execute this line before: git remote add upstream git://github.com/moodle/moodle.git
          3- git checkout -b MDL-30081-master //actually in the Moodle git process, your 2.2 branch should be named MDL-30081-MOODLE_STABLE_22, and the master just MDL-30081. But that's really not important.
          4- git cherry-pick 85f68920ff3d70645e38cce0f80f0077252b6232 //to get your first commit - if there is some issue Git will tell you the other commands, but I think there will be no issues
          5- git remote add jerome git://github.com/mouneyrac/moodle.git //to add my personal moodle repository
          6- git fetch jerome //to know about my last commits
          7- git cherry-pick 77e73c4e4052d5240f829c593ed286fa9bf4334f //to get my last commit

          You can now finish the function with a new single commit and push everything into MDL-30081-master. I'll review it and send it for integration.

          Some other git commands I find useful (in case you don't know one of them):

          • git rebase master (when you just updated your master, and when you are in your MDL-XXX branch, you can execute this command to get all the new code from master but still having your branch commits on top)
          • git reset --soft HEAD^ (to remove the last commit fomr the history but keeping all the code changes)
          • tig (an external software to see easily the git history in command line. It works on linux/mac.)

          Have fun

          Show
          Jérôme Mouneyrac added a comment - Hi Fabio, yes if you can finish the issue, it is great for me (PS: I'm in holidays next week). To get you back on track, I know this way: 1- git checkout master 2- git pull upstream master //to get an up to date local master. if you don't have upstream remote repository, you need to execute this line before: git remote add upstream git://github.com/moodle/moodle.git 3- git checkout -b MDL-30081 -master //actually in the Moodle git process, your 2.2 branch should be named MDL-30081 -MOODLE_STABLE_22, and the master just MDL-30081 . But that's really not important. 4- git cherry-pick 85f68920ff3d70645e38cce0f80f0077252b6232 //to get your first commit - if there is some issue Git will tell you the other commands, but I think there will be no issues 5- git remote add jerome git://github.com/mouneyrac/moodle.git //to add my personal moodle repository 6- git fetch jerome //to know about my last commits 7- git cherry-pick 77e73c4e4052d5240f829c593ed286fa9bf4334f //to get my last commit You can now finish the function with a new single commit and push everything into MDL-30081 -master. I'll review it and send it for integration. Some other git commands I find useful (in case you don't know one of them): git rebase master (when you just updated your master, and when you are in your MDL-XXX branch, you can execute this command to get all the new code from master but still having your branch commits on top) git reset --soft HEAD^ (to remove the last commit fomr the history but keeping all the code changes) tig (an external software to see easily the git history in command line. It works on linux/mac.) Have fun
          Hide
          Jérôme Mouneyrac added a comment -

          Just reading my tmp changes, the varlog call can be removed that my personal log function.

          Show
          Jérôme Mouneyrac added a comment - Just reading my tmp changes, the varlog call can be removed that my personal log function.
          Hide
          Fábio Souto added a comment - - edited

          Hello,

          Thank you for the instructions, I was able to copy your changes into my repository.

          I completed the unit test so that the exceptions are tested and verified that PARAM_CLEANHTML is indeed enough to prevent malicious script insertion (such as XSS attacks).
          What did you mean by TODO: check exceptions ?
          Also I have added the form so that this function can be tested via the web interface.

          The compare can be seen at https://github.com/fabiomsouto/moodle/compare/MDL-30081-master .

          Tell me what you think
          Fábio.

          Show
          Fábio Souto added a comment - - edited Hello, Thank you for the instructions, I was able to copy your changes into my repository. I completed the unit test so that the exceptions are tested and verified that PARAM_CLEANHTML is indeed enough to prevent malicious script insertion (such as XSS attacks). What did you mean by TODO: check exceptions ? Also I have added the form so that this function can be tested via the web interface. The compare can be seen at https://github.com/fabiomsouto/moodle/compare/MDL-30081-master . Tell me what you think Fábio.
          Hide
          Jérôme Mouneyrac added a comment - - edited

          Hi Fabio,
          the TODO: check exceptions: some exception don't exist

          throw new moodle_exception('idnumbertoolong'); //and 'idnumbertaken' too

          You'll need to add an exception message into lang/en/webservice.php

          $string['newstring'] = 'Error message'; 

          And in externallib.php you 'll need to change the line for:

          throw new moodle_exception('newstring', 'webservice');

          Thanks so much for the unit tests, and the web admin test too!
          Once the left TODO is fixed you can submit it for integration.

          Also don't forget to revert the unit test settings to false.

          Show
          Jérôme Mouneyrac added a comment - - edited Hi Fabio, the TODO: check exceptions: some exception don't exist throw new moodle_exception('idnumbertoolong'); //and 'idnumbertaken' too You'll need to add an exception message into lang/en/webservice.php $string['newstring'] = 'Error message'; And in externallib.php you 'll need to change the line for: throw new moodle_exception('newstring', 'webservice'); Thanks so much for the unit tests, and the web admin test too! Once the left TODO is fixed you can submit it for integration. Also don't forget to revert the unit test settings to false.
          Hide
          Jérôme Mouneyrac added a comment -

          I tested the unit tests, the first time it failed at line 322 because apparently $category3 succeed to be created, then I rerun it again and it was working, weird (the MAX(id) logic seems to be correct). I can not reproduce anymore.

          Show
          Jérôme Mouneyrac added a comment - I tested the unit tests, the first time it failed at line 322 because apparently $category3 succeed to be created, then I rerun it again and it was working, weird (the MAX(id) logic seems to be correct). I can not reproduce anymore.
          Hide
          Fábio Souto added a comment - - edited

          Hello Jerome,

          Thank you for peer reviewing this.
          I have created and reused where possible the existing error messages, creating the non existant on error.php as you suggested on another issue.

          I have also completed the unit tests on normal creation, because I was only counting the number of correctly created categories but I wasn't checking if the provided values were correctly populated.
          Turned out that the description wasn't being correctly populated (it was using a wrong variable for checking if the description was passed) and I fixed it.

          Also I moved the query for getting a non existing category down, after creating the normal categories. There could be a chance that the non existing ID that was being generated would be taken by the creation of the normal categories before. So it should make that error vanish as well.

          So that's it for this one. Submitting for integration (well I'm trying, but I can't find the option, where is it?? )

          Show
          Fábio Souto added a comment - - edited Hello Jerome, Thank you for peer reviewing this. I have created and reused where possible the existing error messages, creating the non existant on error.php as you suggested on another issue. I have also completed the unit tests on normal creation, because I was only counting the number of correctly created categories but I wasn't checking if the provided values were correctly populated. Turned out that the description wasn't being correctly populated (it was using a wrong variable for checking if the description was passed) and I fixed it. Also I moved the query for getting a non existing category down, after creating the normal categories. There could be a chance that the non existing ID that was being generated would be taken by the creation of the normal categories before. So it should make that error vanish as well. So that's it for this one. Submitting for integration (well I'm trying, but I can't find the option, where is it?? )
          Hide
          Jérôme Mouneyrac added a comment -

          Ah maybe you don't have the option yet. I'll ask for you to be able to submit when I'm back at work. For now I'll press the button for you The button is at the left of the Workflow button.

          Show
          Jérôme Mouneyrac added a comment - Ah maybe you don't have the option yet. I'll ask for you to be able to submit when I'm back at work. For now I'll press the button for you The button is at the left of the Workflow button.
          Hide
          Jérôme Mouneyrac added a comment -

          Ah you found it

          Show
          Jérôme Mouneyrac added a comment - Ah you found it
          Hide
          Aparup Banerjee added a comment -

          The main moodle.git repository has just been updated (yesterday) with latest weekly modifications. You may wish to rebase your PULL branches to simplify history and avoid any possible merge conflicts. This would also make integrator's life easier next week.

          TIA and ciao

          Show
          Aparup Banerjee added a comment - The main moodle.git repository has just been updated (yesterday) with latest weekly modifications. You may wish to rebase your PULL branches to simplify history and avoid any possible merge conflicts. This would also make integrator's life easier next week. TIA and ciao
          Hide
          Fábio Souto added a comment -

          Hello, I have rebased as advised.

          Cheers,
          Fábio

          Show
          Fábio Souto added a comment - Hello, I have rebased as advised. Cheers, Fábio
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Hi Fabio,

          apart from the comments from Petr:

          1/ cannot use strlen, must use utf88 savvy 'textlib::strlen()' instead.

          2/ PARAM_RAW should be ok for description, like the create_courses function.

          I would add, also:

          3/ To be similar to other function, I'd add also descriptionformat (like the summary in create_courses has it too). Of course no format_text() or similar needs to be done at all.

          4/ Also... I'd recommend you to check the coding style... I'm adding one smurf file here, hoping it will help you...

          So reopening to tidy it a bit... thanks...ciao

          Show
          Eloy Lafuente (stronk7) added a comment - Hi Fabio, apart from the comments from Petr: 1/ cannot use strlen, must use utf88 savvy 'textlib::strlen()' instead. 2/ PARAM_RAW should be ok for description, like the create_courses function. I would add, also: 3/ To be similar to other function, I'd add also descriptionformat (like the summary in create_courses has it too). Of course no format_text() or similar needs to be done at all. 4/ Also... I'd recommend you to check the coding style... I'm adding one smurf file here, hoping it will help you... So reopening to tidy it a bit... thanks...ciao
          Hide
          Jérôme Mouneyrac added a comment - - edited

          Ah yes I forgot about this in the Moodledocs, adding it: http://docs.moodle.org/dev/How_to_contribute_a_web_service_function_to_core#Create_a_tracker_issue. I talked with Martin and the conclusion was that we should avoid to add text format attribut as much as possible. HTML looked like the way we go. Of course it's open to discussion

          Show
          Jérôme Mouneyrac added a comment - - edited Ah yes I forgot about this in the Moodledocs, adding it: http://docs.moodle.org/dev/How_to_contribute_a_web_service_function_to_core#Create_a_tracker_issue . I talked with Martin and the conclusion was that we should avoid to add text format attribut as much as possible. HTML looked like the way we go. Of course it's open to discussion
          Hide
          Fábio Souto added a comment -

          Hello Eloy,

          Thanks for the feedback.
          I have corrected the pointed issues, namely 1) 2) and 3) .
          I have also corrected all the issues that your smurf ( lol ) file told, except for the package declaration for the class core_course_create_categories_form (file admin/webservice/testclient_forms.php). What would this package be?

          Jerome,

          Regarding the HTML parameter, I did not change it yet as I've seen that create_courses also uses PARAM_TEXT for the course name. If you think that we should use PARAM_HTML for the category name instead of PARAM_TEXT, let me know and I'll change it.

          Submitting again for peer review, cheers.
          Fábio

          Show
          Fábio Souto added a comment - Hello Eloy, Thanks for the feedback. I have corrected the pointed issues, namely 1) 2) and 3) . I have also corrected all the issues that your smurf ( lol ) file told, except for the package declaration for the class core_course_create_categories_form (file admin/webservice/testclient_forms.php). What would this package be? Jerome, Regarding the HTML parameter, I did not change it yet as I've seen that create_courses also uses PARAM_TEXT for the course name. If you think that we should use PARAM_HTML for the category name instead of PARAM_TEXT, let me know and I'll change it. Submitting again for peer review, cheers. Fábio
          Hide
          Jérôme Mouneyrac added a comment - - edited

          Many functions deal in different way with the text fields. We should stick with what we wrote in the Moodledocs and if needed we can update the Moodledocs. We would also need to fix (if possible) the existing but it's another issue to change the existing API.

          In conclusion I think for the moment we should stick with what the Moodledocs say: use PARAM_CLEANHTML, we should not have any format field and we should use format_text() when returning a text field.

          Show
          Jérôme Mouneyrac added a comment - - edited Many functions deal in different way with the text fields. We should stick with what we wrote in the Moodledocs and if needed we can update the Moodledocs. We would also need to fix (if possible) the existing but it's another issue to change the existing API. In conclusion I think for the moment we should stick with what the Moodledocs say: use PARAM_CLEANHTML, we should not have any format field and we should use format_text() when returning a text field.
          Hide
          Fábio Souto added a comment - - edited

          So for this issue, only the description field matters, since name is not HTML I suppose (so it should be kept as PARAM_TEXT). Is that it?

          Edit: if you agree could you submit for integration? I can't do it.

          Cheers,
          Fabio

          Show
          Fábio Souto added a comment - - edited So for this issue, only the description field matters, since name is not HTML I suppose (so it should be kept as PARAM_TEXT). Is that it? Edit: if you agree could you submit for integration? I can't do it. Cheers, Fabio
          Hide
          Jérôme Mouneyrac added a comment -

          Hi Fabio, it's weird you should be able to see the button as you are assignee: https://skitch.com/mouneyrac/81hat/mdl-30081-core-course-create-categories-moodle-tracker

          If you don't I'll submit the issue for you and ask Mike why you don't see it.

          thanks

          Show
          Jérôme Mouneyrac added a comment - Hi Fabio, it's weird you should be able to see the button as you are assignee: https://skitch.com/mouneyrac/81hat/mdl-30081-core-course-create-categories-moodle-tracker If you don't I'll submit the issue for you and ask Mike why you don't see it. thanks
          Hide
          Fábio Souto added a comment -

          Nope, all I see is Stop development and a Workflow which has a dropdown (Start peer-review and Close Issue).
          That's all there is

          Show
          Fábio Souto added a comment - Nope, all I see is Stop development and a Workflow which has a dropdown (Start peer-review and Close Issue). That's all there is
          Hide
          Eloy Lafuente (stronk7) added a comment - - edited

          Oki, so we decided to assume that all texts will be html by default. From http://docs.moodle.org/dev/How_to_contribute_a_web_service_function_to_core :

          • description/summary/textfields are commonly HTML. They are check against PARAM_CLEANHTML and there is no need to send a text format attribut. However don't forget to call format_text() when returning a text field.

          NP with that at all. But that implies that:

          1) On input (insert/update), they must be checked against PARAM_CLEANHTML.
          2) The corresponding format field will be, always = FORMAT_HTML
          3) On output (select), it needs to be prepared (to support files over WS) and then processed by format_text() (to apply filters and clean HTML).
          4) format_text() must NOT be used on input, only on output.

          The only point I cannot agree is about the need to use PARAM_CLEANHTML (point 1), when we always and everywhere have been delegating the cleaning to output (point 3). More yet, users with perms given to execute those webservices are trusted 100%. I only see the PARAM_CLEANHTML type to process some contents allowing HTML that, later, are not processed by format_text() but by (lighter) format_string(), like happens with activity names and other few strings.

          So, I would:

          A) Recommend to change the Docs about how to support text contents. I may agree it can be assumed all them will be HTML format, np with that at all. Then, I'd detail a bit more both the input and output "rules" like:

          • Input:
            • Use PARAM_RAW
            • Never use format_text() nor any other formatting option. it causes filters and other things to be processed.
            • Set the corresponding xxxformat to FORMAT_HTML always
          • Output:

          B) Change this issue to observe the "rules" of correct text processing.

          I'm happy if we discuss the A) point above in HQ if needed. But using PARAM_CLEANHTML + format_text() on input seems to be an incorrect combination 100%. So I'm reopening this again until it gets 100% agreed and documented.

          Ciao

          Edited to add one point about filtering from HQ chat.
          Edited to add another point about cleaning from HQ chat.

          Show
          Eloy Lafuente (stronk7) added a comment - - edited Oki, so we decided to assume that all texts will be html by default. From http://docs.moodle.org/dev/How_to_contribute_a_web_service_function_to_core : description/summary/textfields are commonly HTML. They are check against PARAM_CLEANHTML and there is no need to send a text format attribut. However don't forget to call format_text() when returning a text field. NP with that at all. But that implies that: 1) On input (insert/update), they must be checked against PARAM_CLEANHTML. 2) The corresponding format field will be, always = FORMAT_HTML 3) On output (select), it needs to be prepared (to support files over WS) and then processed by format_text() (to apply filters and clean HTML). 4) format_text() must NOT be used on input, only on output. The only point I cannot agree is about the need to use PARAM_CLEANHTML (point 1), when we always and everywhere have been delegating the cleaning to output (point 3). More yet, users with perms given to execute those webservices are trusted 100%. I only see the PARAM_CLEANHTML type to process some contents allowing HTML that, later, are not processed by format_text() but by (lighter) format_string(), like happens with activity names and other few strings. So, I would: A) Recommend to change the Docs about how to support text contents. I may agree it can be assumed all them will be HTML format, np with that at all. Then, I'd detail a bit more both the input and output "rules" like: Input: Use PARAM_RAW Never use format_text() nor any other formatting option. it causes filters and other things to be processed. Set the corresponding xxxformat to FORMAT_HTML always Output: Always prepare the text to use the webservices pluginfile. Always use format_text() to process filters, perform cleaning if necessary... Don't apply filtering to text going out from Moodle (David pointed about this @ HQ chat, have been problematic in the past with portfolio exports, for example). Not sure about cleaning. See: http://github.com/moodle/moodle/commit/7e514cd7682bee5e72fe74f7f901f6c06080c163 and MDL-25619 . B) Change this issue to observe the "rules" of correct text processing. I'm happy if we discuss the A) point above in HQ if needed. But using PARAM_CLEANHTML + format_text() on input seems to be an incorrect combination 100%. So I'm reopening this again until it gets 100% agreed and documented. Ciao Edited to add one point about filtering from HQ chat. Edited to add another point about cleaning from HQ chat.
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Fabio, other than that, I think the implementation is 99% perfect, the only point is that there is still some invalid whitespace (course/externallib.php #496).

          And then, the text handling commented above, once we get the final version defined, sorry for the troubles, but I think it's worth having it well defined before accepting this and others.

          Many thanks, ciao

          Show
          Eloy Lafuente (stronk7) added a comment - Fabio, other than that, I think the implementation is 99% perfect, the only point is that there is still some invalid whitespace (course/externallib.php #496). And then, the text handling commented above, once we get the final version defined, sorry for the troubles, but I think it's worth having it well defined before accepting this and others. Many thanks, ciao
          Hide
          Jérôme Mouneyrac added a comment -

          I didn't mention that format_text() should be used on input. If you understood it like that it must be my English because I always thought it should not. For the PARAM_CLEANHTML I supposed wrongly the textaera element were cleaned following the format during insert (it meant for me that we could not change the format after that). So PARAM_RAW is what we should use (ws function should always do the same than in the front end). I'm updating the Moodledocs
          Thanks

          Show
          Jérôme Mouneyrac added a comment - I didn't mention that format_text() should be used on input. If you understood it like that it must be my English because I always thought it should not. For the PARAM_CLEANHTML I supposed wrongly the textaera element were cleaned following the format during insert (it meant for me that we could not change the format after that). So PARAM_RAW is what we should use (ws function should always do the same than in the front end). I'm updating the Moodledocs Thanks
          Hide
          Fábio Souto added a comment -

          Hello,

          Now that's some wall of text
          I'm still trying to understand it all, but I get the main idea. Filtering should be done on output, not on input.
          So for this one I'm removing the format_text() call and defining the description as PARAM_RAW. The descriptionformat will be defined as FORMAT_HTML.

          And that's it I guess?
          The updated version can be pulled.

          F.

          Show
          Fábio Souto added a comment - Hello, Now that's some wall of text I'm still trying to understand it all, but I get the main idea. Filtering should be done on output, not on input. So for this one I'm removing the format_text() call and defining the description as PARAM_RAW. The descriptionformat will be defined as FORMAT_HTML. And that's it I guess? The updated version can be pulled. F.
          Hide
          Eloy Lafuente (stronk7) added a comment -

          1) Jerome: No, after reading the Docs, it was clear for me that you was not saying that format_text() had to be used on input. I just remarked it above because Fábio was using it that way.

          2) Fábio: Yes, I think that, with those changes (RAW + FORMAT_HTML + no format_text(), you will be observing all the input rules. (don't forget the existing whitespace commented above).

          3) Jerome: I've quick looked to the Docs and it has not been changed yet, but I assume the 3 input rules above are the ones you are going to put there, correct? If not, plz, comment it here, so Fábio knows.

          4) Jerome: Do we need to revisit other WSs to verify that both the input and output rules are being observed everywhere? (master only I guess). Perhaps creating a new issue about that would be interesting.

          Ciao

          Show
          Eloy Lafuente (stronk7) added a comment - 1) Jerome: No, after reading the Docs, it was clear for me that you was not saying that format_text() had to be used on input. I just remarked it above because Fábio was using it that way. 2) Fábio: Yes, I think that, with those changes (RAW + FORMAT_HTML + no format_text(), you will be observing all the input rules. (don't forget the existing whitespace commented above). 3) Jerome: I've quick looked to the Docs and it has not been changed yet, but I assume the 3 input rules above are the ones you are going to put there, correct? If not, plz, comment it here, so Fábio knows. 4) Jerome: Do we need to revisit other WSs to verify that both the input and output rules are being observed everywhere? (master only I guess). Perhaps creating a new issue about that would be interesting. Ciao
          Hide
          Andrew Nicols added a comment -

          This issue should probably use the new create_course_category function I'm writing unit tests for rather than rewriting the course category wheel.

          Show
          Andrew Nicols added a comment - This issue should probably use the new create_course_category function I'm writing unit tests for rather than rewriting the course category wheel.
          Hide
          Jérôme Mouneyrac added a comment - - edited

          1) after editing the docs I understood that I'm the one adding/using format_text() in input in the code everywhere, not Fabio, I'm the culprit. Sorry Fabio for making you being charged on it and Eloy to make you think it was Fabio fault.

          3) it should be done when you read it.

          4)I agree we should fix it for the rest of the Moodlecode. It should not impact any existing client as it's just make the web service functions less restrictif => http://tracker.moodle.org/browse/MDL-32581

          Show
          Jérôme Mouneyrac added a comment - - edited 1) after editing the docs I understood that I'm the one adding/using format_text() in input in the code everywhere, not Fabio, I'm the culprit. Sorry Fabio for making you being charged on it and Eloy to make you think it was Fabio fault. 3) it should be done when you read it. 4)I agree we should fix it for the rest of the Moodlecode. It should not impact any existing client as it's just make the web service functions less restrictif => http://tracker.moodle.org/browse/MDL-32581
          Hide
          Fábio Souto added a comment - - edited

          Hello,

          No problem, you even convinced me! j/k

          Andrew: where can I view the code for your function?

          Show
          Fábio Souto added a comment - - edited Hello, No problem, you even convinced me! j/k Andrew: where can I view the code for your function?
          Hide
          Jérôme Mouneyrac added a comment -

          Hi Fabio,
          Andrew's improvement just landed in integration so once it gets tested and passed it goes to main moodle git repository. It should take about three days. Otherwise you can get it from: https://git.luns.net.uk/?p=moodle.git;a=commitdiff;h=b1a8aa73b512466a5876c71edf84b03f709b79d8

          It's very good news that we have core lib function for category now, it makes web service implementation/maintenance easier (even if it comes a bit late for Fabio in this case ). Thanks Andrew.

          Cheers,
          Jerome

          Show
          Jérôme Mouneyrac added a comment - Hi Fabio, Andrew's improvement just landed in integration so once it gets tested and passed it goes to main moodle git repository. It should take about three days. Otherwise you can get it from: https://git.luns.net.uk/?p=moodle.git;a=commitdiff;h=b1a8aa73b512466a5876c71edf84b03f709b79d8 It's very good news that we have core lib function for category now, it makes web service implementation/maintenance easier (even if it comes a bit late for Fabio in this case ). Thanks Andrew. Cheers, Jerome
          Hide
          Fábio Souto added a comment -

          Hello all,

          Rebased and now using Andrew's function for category creation (I was waiting for the update of the main git repository).
          Tested and working.

          Cheers,
          Fabio

          Show
          Fábio Souto added a comment - Hello all, Rebased and now using Andrew's function for category creation (I was waiting for the update of the main git repository). Tested and working. Cheers, Fabio
          Hide
          Jérôme Mouneyrac added a comment -

          It seems all good to me with all the things we talked about input description. Submitting.

          Show
          Jérôme Mouneyrac added a comment - It seems all good to me with all the things we talked about input description. Submitting.
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Hi I'm reopening this. See http://tracker.moodle.org/browse/MDL-30084?focusedCommentId=156782&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-156782 for more comments and plan for allowing all the course_categories ws to land next week (last chance before freeze).

          Also note this needs to fix the category->name check because up to 255 chars are allowed, not 30.

          Ciao

          Show
          Eloy Lafuente (stronk7) added a comment - Hi I'm reopening this. See http://tracker.moodle.org/browse/MDL-30084?focusedCommentId=156782&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-156782 for more comments and plan for allowing all the course_categories ws to land next week (last chance before freeze). Also note this needs to fix the category->name check because up to 255 chars are allowed, not 30. Ciao
          Hide
          Jérôme Mouneyrac added a comment -

          Keep going discussion on MDL-32941

          Show
          Jérôme Mouneyrac added a comment - Keep going discussion on MDL-32941
          Hide
          Eloy Lafuente (stronk7) added a comment -

          This has been integrated and tested @ MDL-32941. Yay!

          Show
          Eloy Lafuente (stronk7) added a comment - This has been integrated and tested @ MDL-32941 . Yay!

            People

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

              Dates

              • Created:
                Updated:
                Resolved: