Details

    • Type: Sub-task Sub-task
    • Status: Closed
    • Priority: Minor Minor
    • Resolution: Fixed
    • Affects Version/s: 2.1
    • Fix Version/s: 2.1
    • Component/s: Web Services
    • Labels:
      None
    • Testing Instructions:
      Hide

      Can be tested by developers only:

      Run the webservice/simpletest unit test: to do it you need to enable xmlrpc protocol + the function in the unit test file (and they also need to be set up in the Moodle admin). You also need to change the token.

      Then you can play with the moodle_notes_create_notes unit test function even thought the default data should be enough to test all error cases. You can also disable Notes in Advanced Feature to check if an exception is thrown.

      Show
      Can be tested by developers only: Run the webservice/simpletest unit test: to do it you need to enable xmlrpc protocol + the function in the unit test file (and they also need to be set up in the Moodle admin). You also need to change the token. Then you can play with the moodle_notes_create_notes unit test function even thought the default data should be enough to test all error cases. You can also disable Notes in Advanced Feature to check if an exception is thrown.
    • Affected Branches:
      MOODLE_21_STABLE
    • Fixed Branches:
      MOODLE_21_STABLE
    • Pull Master Branch:
      MLD-27565-wip
    • Rank:
      17192

      Description

      implement a bulk function for notes, it should match the front-end UI validation + server checks.

        Activity

        Hide
        Jérôme Mouneyrac added a comment -

        updated to last master code, no whitespaces and latest version bumped. Cheers.

        Show
        Jérôme Mouneyrac added a comment - updated to last master code, no whitespaces and latest version bumped. Cheers.
        Hide
        Jérôme Mouneyrac added a comment -

        It's pretty similar to MDL-27566 so I send it for integration as it starts tomorrow

        Show
        Jérôme Mouneyrac added a comment - It's pretty similar to MDL-27566 so I send it for integration as it starts tomorrow
        Hide
        Sam Hemelryk added a comment -

        MLD?

        Show
        Sam Hemelryk added a comment - MLD?
        Hide
        Sam Hemelryk added a comment -

        Hi Jerome,
        I've been having a look at this now in regards to integration.
        The following is what I have spotted:

        • In several places you use 8 spaces for indenting rather than 4.
        • send_messages_parameters unused global $CFG
        • doesn't check to make sure the user hasn't been deleted.
        • If publishstate is site then the courseid needs to be checked/overriden to SITEID (see note below)
        • The text property is cleaned using PARAM_CLEANHTML but the format for the note is FORMAT_PLAIN. At a glance I think that the solution to this is to make format a parameter, set the text type as PARAM_RAW, and then clean using format (which should be verified to be either plain text or html).

        General notes:
        There is a bug in the notes in general that you can create a site note with a course id that is not SITEID.
        However site events must be under SITEID in order to be displayed, through he UI you can't achieve this unless you hack the form, however with the webservice I think we should probably enforce this.

        Presently to me it looks like this needs more work however I think there is some urgency here right?
        If so lets talk about it with Martin quickly and sort out what to do.

        Cheers
        Sam

        Show
        Sam Hemelryk added a comment - Hi Jerome, I've been having a look at this now in regards to integration. The following is what I have spotted: In several places you use 8 spaces for indenting rather than 4. send_messages_parameters unused global $CFG doesn't check to make sure the user hasn't been deleted. If publishstate is site then the courseid needs to be checked/overriden to SITEID (see note below) The text property is cleaned using PARAM_CLEANHTML but the format for the note is FORMAT_PLAIN. At a glance I think that the solution to this is to make format a parameter, set the text type as PARAM_RAW, and then clean using format (which should be verified to be either plain text or html). General notes: There is a bug in the notes in general that you can create a site note with a course id that is not SITEID. However site events must be under SITEID in order to be displayed, through he UI you can't achieve this unless you hack the form, however with the webservice I think we should probably enforce this. Presently to me it looks like this needs more work however I think there is some urgency here right? If so lets talk about it with Martin quickly and sort out what to do. Cheers Sam
        Hide
        Sam Hemelryk added a comment -

        Hi Jerome as discussed I'll fail this for the time being so that the format issue can be sorted out.
        AS for the other points you can fix them if they are easy enough otherwise we can create bugs to fix them after the fact.
        Cheers
        Sam

        Show
        Sam Hemelryk added a comment - Hi Jerome as discussed I'll fail this for the time being so that the format issue can be sorted out. AS for the other points you can fix them if they are easy enough otherwise we can create bugs to fix them after the fact. Cheers Sam
        Hide
        Jérôme Mouneyrac added a comment -

        fixing it

        Show
        Jérôme Mouneyrac added a comment - fixing it
        Hide
        Jérôme Mouneyrac added a comment -

        fixed, resubmitting to integration

        Show
        Jérôme Mouneyrac added a comment - fixed, resubmitting to integration
        Hide
        Sam Hemelryk added a comment -

        Thanks Jerome this has been integrated now.

        Show
        Sam Hemelryk added a comment - Thanks Jerome this has been integrated now.
        Hide
        Eloy Lafuente (stronk7) added a comment -

        Passing based one tests.

        One side comment, I think we need some automated script (CLI?) or function in PHP that, once executed, it sets all the WS required configuration in order to run all the WS tests.

        Do you think that is possible, I'd be happy if doing something like:

        admin/cli/install.php
        admin/cli/generator.php --prepare_for_ws_testing

        we could run all the unittests quickly.

        Note it can be generator.php or any new cli script, that was just one suggestion.

        Ciao

        Show
        Eloy Lafuente (stronk7) added a comment - Passing based one tests. One side comment, I think we need some automated script (CLI?) or function in PHP that, once executed, it sets all the WS required configuration in order to run all the WS tests. Do you think that is possible, I'd be happy if doing something like: admin/cli/install.php admin/cli/generator.php --prepare_for_ws_testing we could run all the unittests quickly. Note it can be generator.php or any new cli script, that was just one suggestion. Ciao
        Hide
        Jérôme Mouneyrac added a comment -

        Yes agree too it would be useful on test site that got reset everyday day/hour. Writing an issue...

        Show
        Jérôme Mouneyrac added a comment - Yes agree too it would be useful on test site that got reset everyday day/hour. Writing an issue...
        Hide
        Jérôme Mouneyrac added a comment -

        Done in MDL-27779

        Show
        Jérôme Mouneyrac added a comment - Done in MDL-27779
        Hide
        Jérôme Mouneyrac added a comment -

        For Sam and Eloy: Thanks for all your recent reviews related to mobile.
        Dongsheng and I

        Show
        Jérôme Mouneyrac added a comment - For Sam and Eloy: Thanks for all your recent reviews related to mobile. Dongsheng and I
        Hide
        Eloy Lafuente (stronk7) added a comment -

        This is now upstream, yay! Many thanks!

        Show
        Eloy Lafuente (stronk7) added a comment - This is now upstream, yay! Many thanks!

          People

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

            Dates

            • Created:
              Updated:
              Resolved: