Details

    • Rank:
      46663

      Description

      core_calendar_create_calendar_events()
      Should core_calendar_update_calendar_events()
      be handled in this issue as well?

        Issue Links

          Activity

          Hide
          Ankit Agarwal added a comment -

          As per discussion with Jerome and in scrum, this should be two separate apis.

          Show
          Ankit Agarwal added a comment - As per discussion with Jerome and in scrum, this should be two separate apis.
          Hide
          Ankit Agarwal added a comment -
          • Will update testing instructions after the review
          • Will do a version bump before integration to reduce conflicts
          • Requesting a peer-review.
            Thanks
          Show
          Ankit Agarwal added a comment - Will update testing instructions after the review Will do a version bump before integration to reduce conflicts Requesting a peer-review. Thanks
          Hide
          Jérôme Mouneyrac added a comment -

          Thanks Ankit, my note:

          • I dont review get_.... as I already did for MDL-37100
          • in create_..._parameters: 'event', VALUE_REQUIRED, NULL_NOT_ALLOWED
            Two things here: first as it's element of a list it doesn't need to be VALUE_REQUIRED. The list can be empty. Secondly you missed one param between VALUE_REQUIRED and NOT_ALLOWED but anyway you can stop at 'event'). it's enough
          • you donot have (type)
          • For VALUE_DEFAULT in the return value description. They should be VALUE_OPTIONAL. Set the defaults in the function if some default are needed.
          Show
          Jérôme Mouneyrac added a comment - Thanks Ankit, my note: I dont review get_.... as I already did for MDL-37100 in create_..._parameters: 'event', VALUE_REQUIRED, NULL_NOT_ALLOWED Two things here: first as it's element of a list it doesn't need to be VALUE_REQUIRED. The list can be empty. Secondly you missed one param between VALUE_REQUIRED and NOT_ALLOWED but anyway you can stop at 'event'). it's enough you donot have (type) For VALUE_DEFAULT in the return value description. They should be VALUE_OPTIONAL. Set the defaults in the function if some default are needed.
          Hide
          Ankit Agarwal added a comment -

          Hi Jerome,
          Thanks for the review.
          I have made the changes you suggested and did some minor cleanup. I re-based this on top of MDL-37100 to reduce conflicts.
          Although am not sure what you were referring to when you said "you donot have (type)".
          Can you please explain that?
          Thanks

          Show
          Ankit Agarwal added a comment - Hi Jerome, Thanks for the review. I have made the changes you suggested and did some minor cleanup. I re-based this on top of MDL-37100 to reduce conflicts. Although am not sure what you were referring to when you said "you donot have (type)". Can you please explain that? Thanks
          Hide
          Jérôme Mouneyrac added a comment -

          Hi Ankit, I can't remember what was the "you donot have (type)" comment. I checked your changesm all good. Thanks.

          Show
          Jérôme Mouneyrac added a comment - Hi Ankit, I can't remember what was the "you donot have (type)" comment. I checked your changesm all good. Thanks.
          Hide
          Ankit Agarwal added a comment -

          Thanks for the confirmation jerome.
          Submitting for integration.

          Show
          Ankit Agarwal added a comment - Thanks for the confirmation jerome. Submitting for integration.
          Hide
          Eloy Lafuente (stronk7) added a comment -

          The main moodle.git repository has just been updated 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
          Eloy Lafuente (stronk7) added a comment - The main moodle.git repository has just been updated 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
          Eloy Lafuente (stronk7) added a comment -

          I've fixed some conflict plus the "donot" that didn't look correct.

          Show
          Eloy Lafuente (stronk7) added a comment - I've fixed some conflict plus the "donot" that didn't look correct.
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Integrated, thanks!

          Show
          Eloy Lafuente (stronk7) added a comment - Integrated, thanks!
          Hide
          Rajesh Taneja added a comment -

          Thanks Ankit,

          Phpunit ran without any error.

          Show
          Rajesh Taneja added a comment - Thanks Ankit, Phpunit ran without any error.
          Hide
          Eloy Lafuente (stronk7) added a comment -

          A brilliant future is awaiting us out there, better with your code. Let's look towards the future together, this is now closed.

          (and won't be revisiting it unless some regression is found)

          Thanks and ciao

          Show
          Eloy Lafuente (stronk7) added a comment - A brilliant future is awaiting us out there, better with your code. Let's look towards the future together, this is now closed. (and won't be revisiting it unless some regression is found) Thanks and ciao
          Hide
          Ankit Agarwal added a comment -

          Removing docs tag as the api is self documented in the WS embedded docs and it has been added to the ws api list http://docs.moodle.org/dev/Web_services_Roadmap#Core_web_service_functions

          Thanks

          Show
          Ankit Agarwal added a comment - Removing docs tag as the api is self documented in the WS embedded docs and it has been added to the ws api list http://docs.moodle.org/dev/Web_services_Roadmap#Core_web_service_functions Thanks

            People

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

              Dates

              • Created:
                Updated:
                Resolved: