Details

      Description

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

        Gliffy Diagrams

          Attachments

            Issue Links

              Activity

              Hide
              ankit_frenz Ankit Agarwal added a comment -

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

              Show
              ankit_frenz Ankit Agarwal added a comment - As per discussion with Jerome and in scrum, this should be two separate apis.
              Hide
              ankit_frenz 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_frenz 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
              jerome 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
              jerome 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_frenz 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_frenz 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
              jerome 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
              jerome 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_frenz Ankit Agarwal added a comment -

              Thanks for the confirmation jerome.
              Submitting for integration.

              Show
              ankit_frenz Ankit Agarwal added a comment - Thanks for the confirmation jerome. Submitting for integration.
              Hide
              stronk7 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
              stronk7 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
              stronk7 Eloy Lafuente (stronk7) added a comment -

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

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

              Integrated, thanks!

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

              Thanks Ankit,

              Phpunit ran without any error.

              Show
              rajeshtaneja Rajesh Taneja added a comment - Thanks Ankit, Phpunit ran without any error.
              Hide
              stronk7 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
              stronk7 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_frenz 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_frenz 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:
                    Fix Release Date:
                    14/May/13