Moodle
  1. Moodle
  2. MDL-19024

Assignment types should be allowed to publish mnet rpc functions

    Details

    • Type: Improvement Improvement
    • Status: Closed
    • Priority: Minor Minor
    • Resolution: Fixed
    • Affects Version/s: 1.9.4
    • Fix Version/s: 2.0
    • Component/s: Assignment (2.2), MNet
    • Labels:
      None
    • Database:
      Any
    • Affected Branches:
      MOODLE_19_STABLE
    • Fixed Branches:
      MOODLE_20_STABLE
    • Rank:
      32872

      Description

      I'm developing an assignment type in which the user submits a Mahara View within Moodle. It needs to call a couple of xmlrpc methods on a networked Mahara site. It is a requirement that this works in Moodle 1.9, so I need to ensure that the methods are defined on the Moodle side (even though they will never be called in Moodle), otherwise the user will not be permitted to send the method calls to the Mahara site.

      Modules can publish rpc methods currently in their rpclib.php file, but "sub" modules like assignment types and resource types cannot.

      Here are two possible solutions:

      (1) Add an rpclib.php in mod/assignment which 'collects' the published rpc functions from all the assignment types (see the file assignment_rpclib.patch).

      • The xmlrpc method bar provided by assignment type foo will be called 'mod/assignment/rpclib.php/bar'
      • If resource types want to publish rpc functions, a nearly identical rpclib.php could go in mod/resource too.

      (2) Modify admin/mnet/adminlib.php to look for rpclib.php files in mod/assignment/type/, mod/resource/type/ (see the file mnet_adminlib.patch)

      • The xmlrpc method bar provided by assignment type foo will be called 'mod/assignment/type/foo/rpclib.php/bar'
      • No need to add rpclib.php files in mod/assignment, mod/resource.
      1. 0001-Loop-over-mod-auth-enrol-dirs-when-searching-for-r.patch
        3 kB
        Richard Mansfield
      2. 0002-Allow-more-space-in-rpc-parent_type-column.patch
        3 kB
        Richard Mansfield
      3. 0003-Allow-mnet-functions-to-have-parent_type-independent.patch
        4 kB
        Richard Mansfield
      4. 0004-Search-assignment-types-for-mnet-rpc-functions-in-rp.patch
        2 kB
        Richard Mansfield
      5. admin_mnet_services.patch
        1.0 kB
        Richard Mansfield
      6. assignment_rpclib.patch
        0.8 kB
        Richard Mansfield
      7. mnet_adminlib_1.patch
        8 kB
        Richard Mansfield
      8. mnet_adminlib.patch
        5 kB
        Richard Mansfield

        Issue Links

          Activity

          Hide
          Richard Mansfield added a comment -

          Replaces mnet_adminlib.patch

          Show
          Richard Mansfield added a comment - Replaces mnet_adminlib.patch
          Hide
          Richard Mansfield added a comment -

          Currently the parent_type column of mnet_rpc is only 6 characters wide.
          This is not big enough to hold 'assignment' or 'resource'.

          The only place parent_type is used is in the Admin->Networking->Peers section to get strings describing mnet services. It needs to be able to get strings for services provided by mod/assignment/type/foo from 'assignment_foo.php', so I have modified the patch (mnet_adminlib_1.patch) to add more space for parent_type in the database.

          Show
          Richard Mansfield added a comment - Currently the parent_type column of mnet_rpc is only 6 characters wide. This is not big enough to hold 'assignment' or 'resource'. The only place parent_type is used is in the Admin->Networking->Peers section to get strings describing mnet services. It needs to be able to get strings for services provided by mod/assignment/type/foo from 'assignment_foo.php', so I have modified the patch (mnet_adminlib_1.patch) to add more space for parent_type in the database.
          Hide
          Martin Dougiamas added a comment -

          My +1 to allow extending this field in 1.9.6, but would like to see either Eloy or Petr give this a +1 too before we do it.

          Of course in 2.0 it's a different game, as mnet, portfolios and possibly even assignment are not the same as 1.9.

          Show
          Martin Dougiamas added a comment - My +1 to allow extending this field in 1.9.6, but would like to see either Eloy or Petr give this a +1 too before we do it. Of course in 2.0 it's a different game, as mnet, portfolios and possibly even assignment are not the same as 1.9.
          Hide
          Richard Mansfield added a comment -

          I hope that in 2.0 we will be able to call functions on the remote server that are not defined locally, as described in http://tracker.moodle.org/browse/MDL-16269. That would make this patch unnecessary.

          Show
          Richard Mansfield added a comment - I hope that in 2.0 we will be able to call functions on the remote server that are not defined locally, as described in http://tracker.moodle.org/browse/MDL-16269 . That would make this patch unnecessary.
          Hide
          Petr Škoda added a comment -

          Hello,
          the plugins infrastructure was completely refactored in HEAD, modules are able to specify subplugins - resource module is bing migrated to separate plugins right now.
          I hope we will migrate mnet to standard webservice API (not yet finished), at present it does not fit much imho and is not integrated with the new plugins framework

          I personally think that we should completely rewrite the assignmetn module and move most of the functionality into grading plugins, it would mean no more assignment subplugins but also much easier to create new assignment-like modules - this is just my point of view, no final decision was made yet.

          (2) Modify admin/mnet/adminlib.php to look for rpclib.php files in mod/assignment/type/, mod/resource/type/ (see the file mnet_adminlib.patch)
          fits the current HEAD direction much better and would not need any changes outside of mnet code which should be ok for MOODLE_19_STABLE BRANCH

          I have reviewed mnet_adminlib_1.patch - it is not suitable for stable because the changes there include DB structure and changes in API, instead we could just hardcode one specific exception only for assignment module.

          HEAD is completely different problem - we should imo try to replace mnet with general webservices, assignment subplugins are normalized there already, but I would not count on it much at this stage.

          Summary:

          • patches above are not suitable for stable
          • STABLE - my +1 for direction (2) with assignment specific hardcoded solution in admin/mnet/adminlib.php
          • HEAD - future of both mnet and assignments is not decided yet
          Show
          Petr Škoda added a comment - Hello, the plugins infrastructure was completely refactored in HEAD, modules are able to specify subplugins - resource module is bing migrated to separate plugins right now. I hope we will migrate mnet to standard webservice API (not yet finished), at present it does not fit much imho and is not integrated with the new plugins framework I personally think that we should completely rewrite the assignmetn module and move most of the functionality into grading plugins, it would mean no more assignment subplugins but also much easier to create new assignment-like modules - this is just my point of view, no final decision was made yet. (2) Modify admin/mnet/adminlib.php to look for rpclib.php files in mod/assignment/type/ , mod/resource/type/ (see the file mnet_adminlib.patch) fits the current HEAD direction much better and would not need any changes outside of mnet code which should be ok for MOODLE_19_STABLE BRANCH I have reviewed mnet_adminlib_1.patch - it is not suitable for stable because the changes there include DB structure and changes in API, instead we could just hardcode one specific exception only for assignment module. HEAD is completely different problem - we should imo try to replace mnet with general webservices, assignment subplugins are normalized there already, but I would not count on it much at this stage. Summary: patches above are not suitable for stable STABLE - my +1 for direction (2) with assignment specific hardcoded solution in admin/mnet/adminlib.php HEAD - future of both mnet and assignments is not decided yet
          Hide
          Richard Mansfield added a comment -

          Hi Petr,

          I am completely expecting to have to rewrite any assignment types for 2.0. Right now this work is for a client who only expects to be using the stable version.

          If database changes are forbidden in stable, then we cannot widen the parent_type column of the mnet_rpc table. This would mean that the language strings used in configuration of the mnet service in the admin section (Admin->Networking->Peers) cannot come from the assignment type, because the parent_type column can't fit "assignment" in it when it's only 6 characters long.

          The problem is in admin/mnet/mnet_services.html, around line 34:
          get_string($name.'name', $version['parent_type'].''.$version['parent'] , $mnet_peer->name)

          The normal location for strings in assignment types is files like "assignment_foo.php", so that get_string call won't work, even if we leave the parent_type as "mod".

          So to avoid a database change I cannot just keep the solution to within admin/mnet/adminlib.php. I think what I would have to do is this:
          (1) Put some kind of 6-character abbreviation for assignment type ("atype"?) into the parent_type column when the rpc functions are installed, and
          (2) Restore this abbreviation back to "assignment" after querying the database in admin/mnet/mnet_services.php, if I find a "mod/assignment/type" somewhere in the function_name column.

          This seems like rather a nasty hack to get around changing the width of a column, but if you are sure it is preferable then I will do it.

          Either that or I guess people who write assignment types with mnet functions could put up with configuring their services under untranslated strings like "[[assignment_foo_name]]", but that doesn't seem like a nice solution for stable either.

          Show
          Richard Mansfield added a comment - Hi Petr, I am completely expecting to have to rewrite any assignment types for 2.0. Right now this work is for a client who only expects to be using the stable version. If database changes are forbidden in stable, then we cannot widen the parent_type column of the mnet_rpc table. This would mean that the language strings used in configuration of the mnet service in the admin section (Admin->Networking->Peers) cannot come from the assignment type, because the parent_type column can't fit "assignment" in it when it's only 6 characters long. The problem is in admin/mnet/mnet_services.html, around line 34: get_string($name.' name', $version ['parent_type'] .' '.$version ['parent'] , $mnet_peer->name) The normal location for strings in assignment types is files like "assignment_foo.php", so that get_string call won't work, even if we leave the parent_type as "mod". So to avoid a database change I cannot just keep the solution to within admin/mnet/adminlib.php. I think what I would have to do is this: (1) Put some kind of 6-character abbreviation for assignment type ("atype"?) into the parent_type column when the rpc functions are installed, and (2) Restore this abbreviation back to "assignment" after querying the database in admin/mnet/mnet_services.php, if I find a "mod/assignment/type" somewhere in the function_name column. This seems like rather a nasty hack to get around changing the width of a column, but if you are sure it is preferable then I will do it. Either that or I guess people who write assignment types with mnet functions could put up with configuring their services under untranslated strings like "[ [assignment_foo_name] ]", but that doesn't seem like a nice solution for stable either.
          Hide
          Richard Mansfield added a comment -

          Now broken up into four separate (less scary) patches, this time without changing the order of arguments in mnet_get_functions(). The DB patch is still required; to avoid this would be too painful as explained above.

          Show
          Richard Mansfield added a comment - Now broken up into four separate (less scary) patches, this time without changing the order of arguments in mnet_get_functions(). The DB patch is still required; to avoid this would be too painful as explained above.
          Hide
          Richard Mansfield added a comment -

          Missing 4th patch attached

          Show
          Richard Mansfield added a comment - Missing 4th patch attached
          Hide
          Petr Škoda added a comment -

          No DB changes are allowed in stable, sorry - this can not be committed into any stable branch.

          Show
          Petr Škoda added a comment - No DB changes are allowed in stable, sorry - this can not be committed into any stable branch.
          Hide
          Eloy Lafuente (stronk7) added a comment -

          From a pure DB perspective, no problem at all about to change "parent_type" length to 20 at all. I think we have done that before in stable branches and it's only a matter of performing exactly the same change in HEAD.

          So, if MD accepts to do an exception in the "no DB changes in stable allowed" rule, np as long as that patch doesn't break anything else in the stable branch. Everything else MUST continue working EXACTLY the same. Unlucky I'm far from be able to decide if that is 100% true (I don't know too much about mnet/rpc sorry).

          Can anybody guarantee that, apart from the DB change, nothing changes for the rest of Moodle, and extending rpc support for assignment sub-types is the only change? If so, and MD authorises the DB change, +1.

          Finally... as commented by Petr, how will be that ported to HEAD ? With the pending decisions about mnet/assignments... I can imagine that being a tricky thing. Perhaps we can merge those changes right now (as mnet/assignment in 2.0 haven't changed yet) and then... God will say, Petr?

          Ciao

          Show
          Eloy Lafuente (stronk7) added a comment - From a pure DB perspective, no problem at all about to change "parent_type" length to 20 at all. I think we have done that before in stable branches and it's only a matter of performing exactly the same change in HEAD. So, if MD accepts to do an exception in the "no DB changes in stable allowed" rule, np as long as that patch doesn't break anything else in the stable branch. Everything else MUST continue working EXACTLY the same. Unlucky I'm far from be able to decide if that is 100% true (I don't know too much about mnet/rpc sorry). Can anybody guarantee that, apart from the DB change, nothing changes for the rest of Moodle, and extending rpc support for assignment sub-types is the only change? If so, and MD authorises the DB change, +1. Finally... as commented by Petr, how will be that ported to HEAD ? With the pending decisions about mnet/assignments... I can imagine that being a tricky thing. Perhaps we can merge those changes right now (as mnet/assignment in 2.0 haven't changed yet) and then... God will say, Petr? Ciao
          Hide
          Petr Škoda added a comment -

          aaah, forgot to read Martin's comment above.

          Anyway I can not persuade myself this is a good direction because for the last few years I was always trying to persuade Martin to move away from assignment subtypes. At the same time I was arguing against mnet - I believe it should be based on future WS implementation instead of keeping it as a special case all over the place

          reassigning to Eloy

          Show
          Petr Škoda added a comment - aaah, forgot to read Martin's comment above. Anyway I can not persuade myself this is a good direction because for the last few years I was always trying to persuade Martin to move away from assignment subtypes. At the same time I was arguing against mnet - I believe it should be based on future WS implementation instead of keeping it as a special case all over the place reassigning to Eloy
          Hide
          Richard Mansfield added a comment -

          Yes, nothing changes in this patch for anything that already defines rpc functions. Currently these are the mnet_publishes() functions in mod//rpclib.php, auth//auth.php, enrol/*/enrol.php.

          Show
          Richard Mansfield added a comment - Yes, nothing changes in this patch for anything that already defines rpc functions. Currently these are the mnet_publishes() functions in mod/ /rpclib.php, auth/ /auth.php, enrol/*/enrol.php.
          Hide
          Richard Mansfield added a comment -

          It seems these patches won't make it, so I've given up on getting mnet functions published by assignment types. Instead I've pulled the mnet functions out of my assignment type and into a separate dummy module directly under 'mod', because rpclib.php files already get processed at this level. Users will have to install both a module and an assignment type but I can live with that.

          There's one remaining problem which is that the admin page that configures services for mnet hosts seems to be looking for strings in the wrong place. For services provided in 'mod/foo' it is looking for strings in 'mod_foo.php' when I believe it should be looking in 'foo.php'. The behaviour seems to be correct for auth and enrol plugins but just fails in the case of mod.

          The attached admin_mnet_services.patch fixes the problem.

          Show
          Richard Mansfield added a comment - It seems these patches won't make it, so I've given up on getting mnet functions published by assignment types. Instead I've pulled the mnet functions out of my assignment type and into a separate dummy module directly under 'mod', because rpclib.php files already get processed at this level. Users will have to install both a module and an assignment type but I can live with that. There's one remaining problem which is that the admin page that configures services for mnet hosts seems to be looking for strings in the wrong place. For services provided in 'mod/foo' it is looking for strings in 'mod_foo.php' when I believe it should be looking in 'foo.php'. The behaviour seems to be correct for auth and enrol plugins but just fails in the case of mod. The attached admin_mnet_services.patch fixes the problem.
          Hide
          Jonathan Moore added a comment -

          Richard we have run into this issue before with our Jaspersoft reporting integration and we have developed a method that doesn't require core modifications. I am quoting the notes from our development team below. We are both a Mahara and Moodle partner. We have clients asking for this integration and would love to be able to load it without patching core. Are you open to the suggestions below? If you need further details let me know.

          From RL Dev Team
          The following is in reference to a custom integration between Moodle and Mahara:

          http://wiki.mahara.org/System_Administrator%27s_Guide/Moodle%2F%2FMahara_Integration/View_Submission

          The instructions detail a change required to the assignment langauge file but looking at the lang file for the assignment type, this change is unnecessary as the assignment type lang file is named correctly and contains the correct language string value so that the name will appear in the Moodle UI.

          There is also a link to an issue in the Moodle tracker that relates to allowing assignment types to public MNet RPC functions. This is best accomplished (in Moodle 1.9, at least) by using a dummy authentication plug-in to publish the RPC functions.

          Publishing the functions from an auth plug-in would also remove the need for the patch to the admin_mnet_services.html file to deal with displaying the language string correctly for assignment type plug-in's.

          The dummy authentication plug-in is currently how we accomplish this with our ELIS product and our JasperServer integration.

          Show
          Jonathan Moore added a comment - Richard we have run into this issue before with our Jaspersoft reporting integration and we have developed a method that doesn't require core modifications. I am quoting the notes from our development team below. We are both a Mahara and Moodle partner. We have clients asking for this integration and would love to be able to load it without patching core. Are you open to the suggestions below? If you need further details let me know. From RL Dev Team The following is in reference to a custom integration between Moodle and Mahara: http://wiki.mahara.org/System_Administrator%27s_Guide/Moodle%2F%2FMahara_Integration/View_Submission The instructions detail a change required to the assignment langauge file but looking at the lang file for the assignment type, this change is unnecessary as the assignment type lang file is named correctly and contains the correct language string value so that the name will appear in the Moodle UI. There is also a link to an issue in the Moodle tracker that relates to allowing assignment types to public MNet RPC functions. This is best accomplished (in Moodle 1.9, at least) by using a dummy authentication plug-in to publish the RPC functions. Publishing the functions from an auth plug-in would also remove the need for the patch to the admin_mnet_services.html file to deal with displaying the language string correctly for assignment type plug-in's. The dummy authentication plug-in is currently how we accomplish this with our ELIS product and our JasperServer integration.
          Hide
          Richard Mansfield added a comment -

          Hi Jonathan,

          Yes, you're right, I think I probably should have used a dummy auth plug-in for this. The small patch to the admin/mnet/mnet_services.html has already been applied to 1.9 by Matt Oquist, so the dummy mod plug-in I used also displays the correct strings in the admin area, as of 2009-12-22.

          R.

          Show
          Richard Mansfield added a comment - Hi Jonathan, Yes, you're right, I think I probably should have used a dummy auth plug-in for this. The small patch to the admin/mnet/mnet_services.html has already been applied to 1.9 by Matt Oquist, so the dummy mod plug-in I used also displays the correct strings in the admin area, as of 2009-12-22. R.
          Hide
          Jonathan Moore added a comment -

          Thanks Richard,

          We will give it a shot.

          Show
          Jonathan Moore added a comment - Thanks Richard, We will give it a shot.
          Hide
          Penny Leach added a comment - - edited

          I've recently changed the way mnet-enabled plugins work in HEAD.

          You should now be able to add a db/mnet.php to any plugin that you can find in get_plugin_types() (which supports subtypes).

          Can someone give this a go with the tip of HEAD?

          Edit: Changes summarised (and commits grouped) into: MDL-21261

          Show
          Penny Leach added a comment - - edited I've recently changed the way mnet-enabled plugins work in HEAD. You should now be able to add a db/mnet.php to any plugin that you can find in get_plugin_types() (which supports subtypes). Can someone give this a go with the tip of HEAD? Edit: Changes summarised (and commits grouped) into: MDL-21261
          Hide
          Penny Leach added a comment -

          Does anyone have a problem with me closing this issue now, as it's been fixed in HEAD by some of the MNET rewriting.

          Show
          Penny Leach added a comment - Does anyone have a problem with me closing this issue now, as it's been fixed in HEAD by some of the MNET rewriting.
          Hide
          Richard Mansfield added a comment -

          Not me

          Show
          Richard Mansfield added a comment - Not me

            People

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

              Dates

              • Created:
                Updated:
                Resolved: