Details

    • Type: Sub-task Sub-task
    • Status: Closed
    • Priority: Minor Minor
    • Resolution: Fixed
    • Affects Version/s: 2.5
    • Fix Version/s: 2.5
    • Component/s: Forum
    • Labels:
      None
    • Testing Instructions:
      Hide
      1. Create a few courses.
      2. Create multiple forums in each course.
      3. Enrol a user into one of the courses.
      4. Create a web service client (similar to the one attached) that includes the course they are enrolled in in the courseids array.
      5. Execute the client and ensure the forums for that course are returned.
      6. Edit the courseids array so that it is empty.
      7. Execute the client and ensure the forums for the course they are enrolled in are returned.
      8. Add the courseids for the courses they are not enrolled in.
      9. Execute the client and ensure a capability error is thrown.
      10. Run the PHP Unit test (phpunit -c mod/forum/phpunit.xml).
      Show
      Create a few courses. Create multiple forums in each course. Enrol a user into one of the courses. Create a web service client (similar to the one attached) that includes the course they are enrolled in in the courseids array. Execute the client and ensure the forums for that course are returned. Edit the courseids array so that it is empty. Execute the client and ensure the forums for the course they are enrolled in are returned. Add the courseids for the courses they are not enrolled in. Execute the client and ensure a capability error is thrown. Run the PHP Unit test (phpunit -c mod/forum/phpunit.xml).
    • Affected Branches:
      MOODLE_25_STABLE
    • Fixed Branches:
      MOODLE_25_STABLE
    • Pull from Repository:
    • Pull Master Branch:
      MDL-37247_master
    • Rank:
      46846

      Description

      Create a get_forum() WS function to retrieve a forum instance.

      1. ws_client.php
        1 kB
        Mark Nelson
      2. ws_curl.php
        18 kB
        Mark Nelson

        Issue Links

          Activity

          Hide
          Damyon Wiese added a comment -

          Hi Mark,

          When returning the list of forums, the forum intro fields needs to go through format_external_text.

          Also - can you add the phpunit command line to run this unit test as the testing instructions? It would make it easier for the integrators.

          (Everything else looks good).

          Thanks, Damyon

          Show
          Damyon Wiese added a comment - Hi Mark, When returning the list of forums, the forum intro fields needs to go through format_external_text. Also - can you add the phpunit command line to run this unit test as the testing instructions? It would make it easier for the integrators. (Everything else looks good). Thanks, Damyon
          Hide
          Mark Nelson added a comment -

          Hi Damyon,

          Thanks for that. Can you please review again? I added the use of external_api::clean_returnvalue as discussed in chat today after SCRUM, can you ensure it is correctly used?

          Regards,

          Mark

          Show
          Mark Nelson added a comment - Hi Damyon, Thanks for that. Can you please review again? I added the use of external_api::clean_returnvalue as discussed in chat today after SCRUM, can you ensure it is correctly used? Regards, Mark
          Hide
          Damyon Wiese added a comment -

          Thanks Mark - I also found some tabs in mod/forum/db/services.php (line 33 and 34).

          (Still checking the rest of the patch).

          Show
          Damyon Wiese added a comment - Thanks Mark - I also found some tabs in mod/forum/db/services.php (line 33 and 34). (Still checking the rest of the patch).
          Hide
          Damyon Wiese added a comment -

          Also - it's still not calling external_format_text().

          You need to add this code to before it adds the forum to the array.

          list($forum->intro, $forum->introformat) = external_format_text($forum->intro, $forum->introformat, $context->id, 'mod_forum','intro',0);
          

          There is an example in group/externallib.php for that.

          Cheers, Damyon

          Show
          Damyon Wiese added a comment - Also - it's still not calling external_format_text(). You need to add this code to before it adds the forum to the array. list($forum->intro, $forum->introformat) = external_format_text($forum->intro, $forum->introformat, $context->id, 'mod_forum','intro',0); There is an example in group/externallib.php for that. Cheers, Damyon
          Hide
          Mark Nelson added a comment -

          Tabs removed. Sorry I thought external_format_value was only used for the parameter or returns function. I have added that change. Now submitting for integration. Thanks!

          Show
          Mark Nelson added a comment - Tabs removed. Sorry I thought external_format_value was only used for the parameter or returns function. I have added that change. Now submitting for integration. Thanks!
          Hide
          Dan Poltawski added a comment -

          Perfect, integrated thanks Mark!

          Show
          Dan Poltawski added a comment - Perfect, integrated thanks Mark!
          Hide
          Adrian Greeve added a comment -

          The testing instructions were good and everything there passed, but I just happened to have a conditional activity set to prevent access to one of the forums which wasn't checked by the web service.

          p.s. The code looks awesome and is fabulous in every way.

          Show
          Adrian Greeve added a comment - The testing instructions were good and everything there passed, but I just happened to have a conditional activity set to prevent access to one of the forums which wasn't checked by the web service. p.s. The code looks awesome and is fabulous in every way.
          Hide
          Juan Leyva added a comment -

          Hi,

          you can find a good piece of code for getting all the forums for an user here:

          https://github.com/moodle/moodle/blob/master/mod/forum/index.php#L114

          The point here is checking the uservisible attribute returned by the get_fast_modinfo, it checks stuff like conditional activities, groupings, groups, etc...

          Regards

          Show
          Juan Leyva added a comment - Hi, you can find a good piece of code for getting all the forums for an user here: https://github.com/moodle/moodle/blob/master/mod/forum/index.php#L114 The point here is checking the uservisible attribute returned by the get_fast_modinfo, it checks stuff like conditional activities, groupings, groups, etc... Regards
          Hide
          Mark Nelson added a comment -

          Thanks Juan! I will be creating a commit shortly that will incorporate this logic.

          Show
          Mark Nelson added a comment - Thanks Juan! I will be creating a commit shortly that will incorporate this logic.
          Hide
          Mark Nelson added a comment -

          Ok, there is another commit that has been pushed. Once this has been pulled into integration this will be ready for testing.

          Show
          Mark Nelson added a comment - Ok, there is another commit that has been pushed. Once this has been pulled into integration this will be ready for testing.
          Hide
          Dan Poltawski added a comment -

          pulled in the fix, please can it be retested.

          Show
          Dan Poltawski added a comment - pulled in the fix, please can it be retested.
          Hide
          Adrian Greeve added a comment -

          Thanks Mark,
          This works well.
          Test passed.

          Show
          Adrian Greeve added a comment - Thanks Mark, This works well. Test passed.
          Hide
          Eloy Lafuente (stronk7) added a comment -

          And your fantastic code has met core, hope they become good friends for a long period.

          Closing, thanks!

          Show
          Eloy Lafuente (stronk7) added a comment - And your fantastic code has met core, hope they become good friends for a long period. Closing, thanks!
          Hide
          Jérôme Mouneyrac added a comment - - edited

          Mark, I've been implementing a create_module() core function. Below is the list of value I'm sending to this core function in my phpunit test (I'm testing it to create forum). Reading our current external get_forums implementation I think we miss some of them. I suppose we want to return most of these fields (someone having editing capability most likely can see all of them):

          • modulename
          • section (number, not id)
          • course id
          • grouping id
          • group members only
          • visible
          • name
          • showdescription
          • grade category id (from grade item)
          • group mode
          • course module id number
          • completion
          • completionview
          • completiongradeitemnumber
          • completionexpected
          • completionposts
          • completiondiscussions
          • completionreplies
          • availablefrom
          • availableuntil
          • showavailability
          • conditiongradegroup (list of object(conditiongradeitem, conditiongrademin, conditiongrademax) )
          • conditionfieldgroup (list of object(conditionfield, conditionfieldoperator, conditionfieldvalue))
          • conditioncompletiongroup (list of object(conditionsourcecmid, conditionrequiredcompletion))
          • assessed
          • scale
          • assesstimestart
          • assesstimefinish
          • rsstypes
          • rssarticles
          • intro
          • introformat
          • forcesubscribe
          • type
          • trackingtype
          • maxbytes
          • maxattachments
          • blockperiod
          • blockafter
          • warnafter
          • plagiarism settings (list of settingname/value)

          Just for information, for some other module types, there are extra common attributs:

          • grade
          • advancedgradingmethod_XXX

          At the end, this comment is just for your information - and also serves as a reminder for myself for later. I'll probably create an improvement issue for the external get_forums.

          Show
          Jérôme Mouneyrac added a comment - - edited Mark, I've been implementing a create_module() core function. Below is the list of value I'm sending to this core function in my phpunit test (I'm testing it to create forum). Reading our current external get_forums implementation I think we miss some of them. I suppose we want to return most of these fields (someone having editing capability most likely can see all of them): modulename section (number, not id) course id grouping id group members only visible name showdescription grade category id (from grade item) group mode course module id number completion completionview completiongradeitemnumber completionexpected completionposts completiondiscussions completionreplies availablefrom availableuntil showavailability conditiongradegroup (list of object(conditiongradeitem, conditiongrademin, conditiongrademax) ) conditionfieldgroup (list of object(conditionfield, conditionfieldoperator, conditionfieldvalue)) conditioncompletiongroup (list of object(conditionsourcecmid, conditionrequiredcompletion)) assessed scale assesstimestart assesstimefinish rsstypes rssarticles intro introformat forcesubscribe type trackingtype maxbytes maxattachments blockperiod blockafter warnafter plagiarism settings (list of settingname/value) Just for information, for some other module types, there are extra common attributs: grade advancedgradingmethod_XXX At the end, this comment is just for your information - and also serves as a reminder for myself for later. I'll probably create an improvement issue for the external get_forums.

            People

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

              Dates

              • Created:
                Updated:
                Resolved: