Details

    • Type: Task Task
    • Status: Closed
    • Priority: Blocker Blocker
    • Resolution: Fixed
    • Affects Version/s: 2.0.3
    • Fix Version/s: 2.1
    • Labels:
      None
    • Rank:
      17213

      Description

      1. Admin selects "Enable mobile" feature in Advanced features. Moodle displays a javascript alert or mentions in the information text than the Moodle web service feature and the xml-rpc server/protocol will be automatically enabled.

      2. Moodle checks that no "Moodle mobile" service was already created ("Moodle mobile" service id will be saved in config_plugin table). Moodle creates automatically a none built-in "Moodle mobile" service including all web service functions.
      This kind of service is editable. The service will be created enabled for all users by default. Only people having the createtoken capability can generate a token anyway. Please check that the mobile entry point script generating the token does check this capability!

      3. The admin uncheck "Enable mobile" => the service is disabled.

      Other solution:
      ---------------
      We could create a disabled built-in service by default during Moodle install/upgrade. This would be the logical way to do. However the problem being that these built-in services have been designed "not editable" to restrict the admin to make any errors. It implies that at the moment creating built-in service does not allow the admin to restrict the mobile functionalities.

      To think of:

      • save the service id in config table in a generic way so other local plugin can created their own service for their own app. Key => external_service_id_of_xxxx / xxxx => moodle_mobile_service.
      • the upgrade script should clean up the mdl_external_function_service table. We could have in services.php some sort of definition of service/function mapping for service created on the fly:
      $onflycreatedservice = 
          array(
              'moodle_mobile_service' => array('moodle_webservice_mobile_get_siteinfo', 'moodle_course_create_courses')
          )
      

        Issue Links

          Activity

          Hide
          Jérôme Mouneyrac added a comment -

          I updated the user manual: http://docs.moodle.org/en/Enable_mobile_web_services

          We finally went for the built-in solution in a first time.

          Show
          Jérôme Mouneyrac added a comment - I updated the user manual: http://docs.moodle.org/en/Enable_mobile_web_services We finally went for the built-in solution in a first time.
          Hide
          Jérôme Mouneyrac added a comment -

          It would be better to wait for all web services to be done before integration (even though it's not going to break anything, new web service will be automatically added to the mobile service).

          Everything regarding this issue should be done, ready for peer-review.

          Show
          Jérôme Mouneyrac added a comment - It would be better to wait for all web services to be done before integration (even though it's not going to break anything, new web service will be automatically added to the mobile service). Everything regarding this issue should be done, ready for peer-review.
          Hide
          Jérôme Mouneyrac added a comment -

          If it gets integrate in this state we need to create these two issues:

          • better usage of shortname : make it editable by custom field.
          • make built-in service editable (don't forget the case when admin removes a function and then the code get upgraded)
          Show
          Jérôme Mouneyrac added a comment - If it gets integrate in this state we need to create these two issues: better usage of shortname : make it editable by custom field. make built-in service editable (don't forget the case when admin removes a function and then the code get upgraded)
          Hide
          Jérôme Mouneyrac added a comment -

          I updated the git commit with https checking + fixed an issue when web service getting disabled when some services were still enabled.

          Show
          Jérôme Mouneyrac added a comment - I updated the git commit with https checking + fixed an issue when web service getting disabled when some services were still enabled.
          Hide
          Jérôme Mouneyrac added a comment -

          It has been rebased to the latest HEAD, you can peer review. Thanks.

          Show
          Jérôme Mouneyrac added a comment - It has been rebased to the latest HEAD, you can peer review. Thanks.
          Hide
          Jérôme Mouneyrac added a comment -

          It would be nice to throw an expception is the shortname is not a ALPHANUMEXT

          Show
          Jérôme Mouneyrac added a comment - It would be nice to throw an expception is the shortname is not a ALPHANUMEXT
          Hide
          Jérôme Mouneyrac added a comment -

          Good for review again (added clean_param check on shortname during creation/update - tested too)

          Show
          Jérôme Mouneyrac added a comment - Good for review again (added clean_param check on shortname during creation/update - tested too)
          Hide
          Jérôme Mouneyrac added a comment -

          Ah I forgot to automatically enable XML-RPC and to disable it too if no other service enabled.

          Show
          Jérôme Mouneyrac added a comment - Ah I forgot to automatically enable XML-RPC and to disable it too if no other service enabled.
          Hide
          Jérôme Mouneyrac added a comment -

          The settings seems to be always "not set" on upgrade... check this issue...

          Show
          Jérôme Mouneyrac added a comment - The settings seems to be always "not set" on upgrade... check this issue...
          Hide
          Jérôme Mouneyrac added a comment -

          fixed, ready for peer-review

          Show
          Jérôme Mouneyrac added a comment - fixed, ready for peer-review
          Hide
          Jérôme Mouneyrac added a comment -

          Rebased

          Show
          Jérôme Mouneyrac added a comment - Rebased
          Hide
          Rossiani Wijaya added a comment -

          Hi Jerome,

          I made some manual adjustment on my distribution in order to run the patch properly:
          1. removing white-space on line 138
          2. update version.php to $version = 2011060200.02
          3. update version value and add $dbman->add_field() to insert the new field.

          if ($oldversion < 2011060200.02) { //TODO: put the right latest version
           // Define field shortname to be added to external_services
                  $table = new xmldb_table('external_services');
                  $field = new xmldb_field('shortname', XMLDB_TYPE_CHAR, '255', null, null, null, null, 'timemodified');
          
                  if (!$dbman->field_exists($table, $field)) {
                      $dbman->add_field($table, $field);
                  }
                  
                  // Conditionally launch add field shortname
                     // Main savepoint reached
                  upgrade_main_savepoint(true, 2011060200.02);
              }
          

          I will start testing it now and let you know if there is more issue.

          Show
          Rossiani Wijaya added a comment - Hi Jerome, I made some manual adjustment on my distribution in order to run the patch properly: 1. removing white-space on line 138 2. update version.php to $version = 2011060200.02 3. update version value and add $dbman->add_field() to insert the new field. if ($oldversion < 2011060200.02) { //TODO: put the right latest version // Define field shortname to be added to external_services $table = new xmldb_table('external_services'); $field = new xmldb_field('shortname', XMLDB_TYPE_CHAR, '255', null , null , null , null , 'timemodified'); if (!$dbman->field_exists($table, $field)) { $dbman->add_field($table, $field); } // Conditionally launch add field shortname // Main savepoint reached upgrade_main_savepoint( true , 2011060200.02); } I will start testing it now and let you know if there is more issue.
          Hide
          Rossiani Wijaya added a comment -

          Hi Jerome,

          Some comments for the issue:
          1. On 'external services' page, 'moodle documentation' link need to be updated.
          2. Just a suggestion, on 'external service' page, if mobile web service is disable, maybe the functions and edit links for 'Moodle mobile web service' should also disable?

          Other than that, disabling/enabling mobile web service is working great.

          Show
          Rossiani Wijaya added a comment - Hi Jerome, Some comments for the issue: 1. On 'external services' page, 'moodle documentation' link need to be updated. 2. Just a suggestion, on 'external service' page, if mobile web service is disable, maybe the functions and edit links for 'Moodle mobile web service' should also disable? Other than that, disabling/enabling mobile web service is working great.
          Hide
          Jérôme Mouneyrac added a comment -

          Thanks Rosie, good catch, these lines disappeared during resolving a git conflict.
          I'll update the doc to the new moodledocs format.
          I think it's ok to let people edit the Mobile service, they can do that manually if they want, it doesn't change the mobile enabled status anyway

          Show
          Jérôme Mouneyrac added a comment - Thanks Rosie, good catch, these lines disappeared during resolving a git conflict. I'll update the doc to the new moodledocs format. I think it's ok to let people edit the Mobile service, they can do that manually if they want, it doesn't change the mobile enabled status anyway
          Hide
          Jérôme Mouneyrac added a comment -

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

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

          I've just detected that we need xmlrpc capability for everybody!!! Please reject

          Show
          Jérôme Mouneyrac added a comment - I've just detected that we need xmlrpc capability for everybody!!! Please reject
          Hide
          Jérôme Mouneyrac added a comment -

          fixed, rebased, whitespaces removed, bumped last version, fingers crossed.

          Show
          Jérôme Mouneyrac added a comment - fixed, rebased, whitespaces removed, bumped last version, fingers crossed.
          Hide
          Jérôme Mouneyrac added a comment -

          I updated the docs too: http://docs.moodle.org/20/en/Enable_mobile_web_services#Disable_the_mobile_web_services.
          it indicates all the automatic actions.

          Show
          Jérôme Mouneyrac added a comment - I updated the docs too: http://docs.moodle.org/20/en/Enable_mobile_web_services#Disable_the_mobile_web_services . it indicates all the automatic actions.
          Hide
          Sam Hemelryk added a comment -

          Thanks guys this has been integrated now.

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

          Hi Jerome,

          I was going to fail this and revert the commit, but as far as it is a non-clean merge, I've decided to, simply reopen it to be properly fixed.

          To reproduce: Install Moodle 2.1 with the CLI installer, you will get:

          Moodle 2.1dev (Build: 20110602) command line installation program
          -->System
          PHP Notice:  Undefined property: stdClass::$defaultuserroleid in /Users/Shared/Jenkins/Home/moodle_integration_git/lib/adminlib.php on line 6510
          PHP Stack trace:
          PHP   1. {main}() /Users/Shared/Jenkins/Home/moodle_integration_git/admin/cli/install.php:0
          PHP   2. install_cli_database() /Users/Shared/Jenkins/Home/moodle_integration_git/admin/cli/install.php:648
          PHP   3. install_core() /Users/Shared/Jenkins/Home/moodle_integration_git/lib/installlib.php:555
          PHP   4. admin_apply_default_settings() /Users/Shared/Jenkins/Home/moodle_integration_git/lib/upgradelib.php:1376
          PHP   5. admin_apply_default_settings() /Users/Shared/Jenkins/Home/moodle_integration_git/lib/adminlib.php:5855
          PHP   6. admin_apply_default_settings() /Users/Shared/Jenkins/Home/moodle_integration_git/lib/adminlib.php:5855
          PHP   7. admin_apply_default_settings() /Users/Shared/Jenkins/Home/moodle_integration_git/lib/adminlib.php:5855
          PHP   8. admin_setting_enablemobileservice->write_setting() /Users/Shared/Jenkins/Home/moodle_integration_git/lib/adminlib.php:5869
          PHP   9. admin_setting_enablemobileservice->set_xmlrpc_cap() /Users/Shared/Jenkins/Home/moodle_integration_git/lib/adminlib.php:6634
          PHP  10. admin_setting_enablemobileservice->is_xmlrpc_cap_allowed() /Users/Shared/Jenkins/Home/moodle_integration_git/lib/adminlib.php:6528
          

          It seems that you are trying to use $defaultuserroleid before it has been defined, so it fails and I guess it causes the behavior of is_xmlrpc_cap_allowed() to fail too.

          Surely that code needs to check if we are in early installation stages - during_initial_install() - and act in consequence. Or be executed later - lib/db/install.php - or whatever but current incarnation seems wrong.

          Ciao

          Show
          Eloy Lafuente (stronk7) added a comment - Hi Jerome, I was going to fail this and revert the commit, but as far as it is a non-clean merge, I've decided to, simply reopen it to be properly fixed. To reproduce: Install Moodle 2.1 with the CLI installer, you will get: Moodle 2.1dev (Build: 20110602) command line installation program --> System PHP Notice: Undefined property: stdClass::$defaultuserroleid in /Users/Shared/Jenkins/Home/moodle_integration_git/lib/adminlib.php on line 6510 PHP Stack trace: PHP 1. {main}() /Users/Shared/Jenkins/Home/moodle_integration_git/admin/cli/install.php:0 PHP 2. install_cli_database() /Users/Shared/Jenkins/Home/moodle_integration_git/admin/cli/install.php:648 PHP 3. install_core() /Users/Shared/Jenkins/Home/moodle_integration_git/lib/installlib.php:555 PHP 4. admin_apply_default_settings() /Users/Shared/Jenkins/Home/moodle_integration_git/lib/upgradelib.php:1376 PHP 5. admin_apply_default_settings() /Users/Shared/Jenkins/Home/moodle_integration_git/lib/adminlib.php:5855 PHP 6. admin_apply_default_settings() /Users/Shared/Jenkins/Home/moodle_integration_git/lib/adminlib.php:5855 PHP 7. admin_apply_default_settings() /Users/Shared/Jenkins/Home/moodle_integration_git/lib/adminlib.php:5855 PHP 8. admin_setting_enablemobileservice->write_setting() /Users/Shared/Jenkins/Home/moodle_integration_git/lib/adminlib.php:5869 PHP 9. admin_setting_enablemobileservice->set_xmlrpc_cap() /Users/Shared/Jenkins/Home/moodle_integration_git/lib/adminlib.php:6634 PHP 10. admin_setting_enablemobileservice->is_xmlrpc_cap_allowed() /Users/Shared/Jenkins/Home/moodle_integration_git/lib/adminlib.php:6528 It seems that you are trying to use $defaultuserroleid before it has been defined, so it fails and I guess it causes the behavior of is_xmlrpc_cap_allowed() to fail too. Surely that code needs to check if we are in early installation stages - during_initial_install() - and act in consequence. Or be executed later - lib/db/install.php - or whatever but current incarnation seems wrong. Ciao
          Hide
          Eloy Lafuente (stronk7) added a comment -

          So, to clearly state it, code has landed upstream, but needs immediate fixing, ciao

          Show
          Eloy Lafuente (stronk7) added a comment - So, to clearly state it, code has landed upstream, but needs immediate fixing, ciao
          Hide
          Jérôme Mouneyrac added a comment -

          closing it and writing a new issue for the bug (as it has been integrated)

          Show
          Jérôme Mouneyrac added a comment - closing it and writing a new issue for the bug (as it has been integrated)
          Hide
          Jérôme Mouneyrac added a comment -

          issue MDL-27781 created for the bug, I'm working on it. Thanks a lot for you reviews and the integration.

          Show
          Jérôme Mouneyrac added a comment - issue MDL-27781 created for the bug, I'm working on it. Thanks a lot for you reviews and the integration.
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Perfect, thanks Jerome!

          Show
          Eloy Lafuente (stronk7) added a comment - Perfect, thanks Jerome!
          Hide
          Helen Foster added a comment -

          QA test for this issue: MDLQA-930 Thanks Jerome

          Show
          Helen Foster added a comment - QA test for this issue: MDLQA-930 Thanks Jerome

            People

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

              Dates

              • Created:
                Updated:
                Resolved: