Details

    • Type: Sub-task
    • Status: Closed
    • Priority: Minor
    • Resolution: Fixed
    • Affects Version/s: 2.5
    • Fix Version/s: 2.6
    • Component/s: Web Services
    • Labels:
    • Testing Instructions:
      Hide

      Testing:

      Use this client: https://gist.github.com/jleyva/6669774
      The curl.php file is here: https://github.com/moodlehq/sample-ws-clients/blob/master/PHP-REST/curl.php

      You need a token related to the Mobile service, for get a token you can go to:

      yourmoodle.com/login/token.php?service=moodle_mobile_app&username=XX&password=YYY

      1 Edit the client.php for adding your custom token and Moodle URL
      2 Open the script in a browser
      3 Check that the user device is stored in Moodle (database user_devices)
      4 Open the script again, you will receive a message "Warning this key already exists for this user"

      Remember to upgrade your Moodle once downloaded the code, it requires some upgrading for adding anew webservice to the moodle mobile app service

      Show
      Testing: Use this client: https://gist.github.com/jleyva/6669774 The curl.php file is here: https://github.com/moodlehq/sample-ws-clients/blob/master/PHP-REST/curl.php You need a token related to the Mobile service, for get a token you can go to: yourmoodle.com/login/token.php?service=moodle_mobile_app&username=XX&password=YYY 1 Edit the client.php for adding your custom token and Moodle URL 2 Open the script in a browser 3 Check that the user device is stored in Moodle (database user_devices) 4 Open the script again, you will receive a message "Warning this key already exists for this user" Remember to upgrade your Moodle once downloaded the code, it requires some upgrading for adding anew webservice to the moodle mobile app service
    • Affected Branches:
      MOODLE_25_STABLE
    • Fixed Branches:
      MOODLE_26_STABLE
    • Pull from Repository:
    • Pull Master Branch:
      MDL-41914-weekly

      Description

      We need a core table for storing the user devices ids that have installed the Moodle Mobile app.

      When a user enables receive Notifications in Moodle Mobile the device unique token must be stored in Moodle so messaging plugins can send PUSH messages to the device.

      We are going to:

      • Create a new core table "user_devices"
      • Create a new web service for storing the devices
      • Add this new web service to the existing Mobile Services file

      New core table user_devices:

      id
      userid
      appid (com.moodle.moodlemobile) or (edu.myuniversity.mycustomoodlemobile)
      name (iPhone)
      model (iPhone3,1)
      platform (iOs)
      version (6.1.2)
      key
      uuid
      timecreated
      timemodified

      See MOBILE-452 for more information

        Gliffy Diagrams

          Issue Links

            Activity

            Hide
            jleyva Juan Leyva added a comment -

            Requesting peer review, notice:

            • I bumped the version number
            • The version number used in upgrade savepoint (upgrade.php)
            • I added a basic phpunit test
            Show
            jleyva Juan Leyva added a comment - Requesting peer review, notice: I bumped the version number The version number used in upgrade savepoint (upgrade.php) I added a basic phpunit test
            Hide
            jerome Jérôme Mouneyrac added a comment - - edited

            Hi Juan,
            for me it's good enough to be pushed like that.

            But there are sew stuff from optional improvements to suggestion improvements that you may want to check before to send to integration:

            • points at the end of the line
            • param phpdoc without english desc
            • I won't create that short character fields (16 char seem very short. I don't think it's a big deal to make then a bit longer to be weirdo data proof, database searches should not be that longer to do)
            • One thing I implemented in my airnotifier plugin is the possibility to enable/disable a device (without losing the registration). It's why I added a enabled field: https://github.com/mouneyrac/moodle/compare/moodle:master...MDL-36445-master?w=1#L5R19
            • Maybe add the brand field too: https://github.com/mouneyrac/moodle/compare/moodle:master...MDL-36445-master?w=1#L5R15
            • for all params and for good practice use params['version'] instead of the $version. So new ws dev who look at your external function understandd the params are validated.
            • I was not familiar with array_intersect_key but good to learn. Thanks.
            Show
            jerome Jérôme Mouneyrac added a comment - - edited Hi Juan, for me it's good enough to be pushed like that. But there are sew stuff from optional improvements to suggestion improvements that you may want to check before to send to integration: points at the end of the line param phpdoc without english desc I won't create that short character fields (16 char seem very short. I don't think it's a big deal to make then a bit longer to be weirdo data proof, database searches should not be that longer to do) One thing I implemented in my airnotifier plugin is the possibility to enable/disable a device (without losing the registration). It's why I added a enabled field: https://github.com/mouneyrac/moodle/compare/moodle:master...MDL-36445-master?w=1#L5R19 Maybe add the brand field too: https://github.com/mouneyrac/moodle/compare/moodle:master...MDL-36445-master?w=1#L5R15 for all params and for good practice use params ['version'] instead of the $version. So new ws dev who look at your external function understandd the params are validated. I was not familiar with array_intersect_key but good to learn. Thanks.
            Hide
            jerome Jérôme Mouneyrac added a comment -

            When do you go back from holidays? If this need to be integrated before code freeze (two weeks) and you are not back before, you can send it for integration.

            Show
            jerome Jérôme Mouneyrac added a comment - When do you go back from holidays? If this need to be integrated before code freeze (two weeks) and you are not back before, you can send it for integration.
            Hide
            jleyva Juan Leyva added a comment -

            Hik Jerome,

            thanks for your feedback.

            I'm currently back from holidays but tomorrow starts the Moodlemoot so I want to send it for integration right now, what I did is the following:

            I fixed all the things you mentioned except the new fields:

            • Device brand: There is no way for getting this information "directly" from Phonegap, it gives you device.name and also device.model but not a "device.brand"
            • Enabled field: Good idea, I was thinking in deleting the device entry but it may makes sense a enabled field, it can be added at the same time we add some U.I to allow admins to view (and enable/disable/delete) devices linked to an user, I'm not sure if it will be some privacy concerns so I will create a new bug for this
            Show
            jleyva Juan Leyva added a comment - Hik Jerome, thanks for your feedback. I'm currently back from holidays but tomorrow starts the Moodlemoot so I want to send it for integration right now, what I did is the following: I fixed all the things you mentioned except the new fields: Device brand: There is no way for getting this information "directly" from Phonegap, it gives you device.name and also device.model but not a "device.brand" Enabled field: Good idea, I was thinking in deleting the device entry but it may makes sense a enabled field, it can be added at the same time we add some U.I to allow admins to view (and enable/disable/delete) devices linked to an user, I'm not sure if it will be some privacy concerns so I will create a new bug for this
            Hide
            stronk7 Eloy Lafuente (stronk7) added a comment -

            Hey Juan,

            1- wasn't the device UUID (for iOS) being deprecated? Isn't the "pushid" (combination of application + unique device) enough for that?

            2- is that database structure enough to store also Google's (GCM) information? AFAIK they also use application, server, token… but seems to require some more information (google account..). Or is this out from the scope of the issue.

            3- also, do we need the name, model, version for anything or is it for stats/curiosity? I'd not store not needed data, after all it's user (private) data.

            Otherwise, and apart from it merging badly (np, we can handle that) the new function and code looks ok. Just halting this a few hours to get some feedback about 1-2-3.

            Ciao

            Show
            stronk7 Eloy Lafuente (stronk7) added a comment - Hey Juan, 1- wasn't the device UUID (for iOS) being deprecated? Isn't the "pushid" (combination of application + unique device) enough for that? 2- is that database structure enough to store also Google's (GCM) information? AFAIK they also use application, server, token… but seems to require some more information (google account..). Or is this out from the scope of the issue. 3- also, do we need the name, model, version for anything or is it for stats/curiosity? I'd not store not needed data, after all it's user (private) data. Otherwise, and apart from it merging badly (np, we can handle that) the new function and code looks ok. Just halting this a few hours to get some feedback about 1-2-3. Ciao
            Hide
            jleyva Juan Leyva added a comment -

            Hi Eloy,

            1 - Well, the UUID is interesting in some cases when you want to restrict an app to certain users/devices. In some scenarios, like debugging new versions or restrict demo versions of an app to certain devices it may be interesting store this information to allow admins/developers to search devices for enable/disable/ban etc...
            Also for certain organizations that deliver phones or tablets for their employees they want some control for statistics/monitoring (this is something that someone from a spanish University asked me in the last moot)

            2 - I think so, (as far as I see in the Phonegap PushNotifications plugin) in the Airnotifier part I'm not so sure but since we are not going to support GCM in the first version is something we can change in a near future

            3 - Privacy:

            • Notifications are a feature that must be enabled by the Mobile user, when enabling the feature we will alert the user that some information regarding their phone will be stored in Moodle.
            • This information is more for debugging/statistics and also for specifics hacks for certain devices and platforms. i.e I'm not sure if Notifications will be able to work in old Android versions like 2.1
              In any case, we can drop this field in future versions if are not useful
            Show
            jleyva Juan Leyva added a comment - Hi Eloy, 1 - Well, the UUID is interesting in some cases when you want to restrict an app to certain users/devices. In some scenarios, like debugging new versions or restrict demo versions of an app to certain devices it may be interesting store this information to allow admins/developers to search devices for enable/disable/ban etc... Also for certain organizations that deliver phones or tablets for their employees they want some control for statistics/monitoring (this is something that someone from a spanish University asked me in the last moot) 2 - I think so, (as far as I see in the Phonegap PushNotifications plugin) in the Airnotifier part I'm not so sure but since we are not going to support GCM in the first version is something we can change in a near future 3 - Privacy: Notifications are a feature that must be enabled by the Mobile user, when enabling the feature we will alert the user that some information regarding their phone will be stored in Moodle. This information is more for debugging/statistics and also for specifics hacks for certain devices and platforms. i.e I'm not sure if Notifications will be able to work in old Android versions like 2.1 In any case, we can drop this field in future versions if are not useful
            Hide
            stronk7 Eloy Lafuente (stronk7) added a comment -

            Hi Juan,

            thanks for the explanations, and apologies for not being able to integrate it this week (I've been kaputt the last 24-48 hours).

            So, plz, consider rebasing your path against last weekly and it will be happily integrated next week. I'll keep this under integration status.

            Again, sorry for the delay, TIA!

            Show
            stronk7 Eloy Lafuente (stronk7) added a comment - Hi Juan, thanks for the explanations, and apologies for not being able to integrate it this week (I've been kaputt the last 24-48 hours). So, plz, consider rebasing your path against last weekly and it will be happily integrated next week. I'll keep this under integration status. Again, sorry for the delay, TIA!
            Hide
            stronk7 Eloy Lafuente (stronk7) added a comment -

            Integrated (2.6), thanks!

            Show
            stronk7 Eloy Lafuente (stronk7) added a comment - Integrated (2.6), thanks!
            Hide
            poltawski Dan Poltawski added a comment -

            Just commenting that I was intrigued why we added this to the core tables rather than say the output plugins tables? Do we have other uses in mind?

            Show
            poltawski Dan Poltawski added a comment - Just commenting that I was intrigued why we added this to the core tables rather than say the output plugins tables? Do we have other uses in mind?
            Hide
            jleyva Juan Leyva added a comment -

            Hi Dan,

            thanks for commenting, these are our reasons:

            • For allow developers to create their own PUSH Notifications plugins using services like Pushwoosh or Amazon Instant messages. So we need a core table to allow these plugins to retrieve the phone information.
            • Because we need a core web service function to be added into the "core service" Moodle Mobile. You can't add functions from other plugins to existing core services
            • For having the airnotifier output plugin outside core, so you can install it as a standard add-one

            In this issue you have more information about the previous and new workflow MOBILE-452

            Show
            jleyva Juan Leyva added a comment - Hi Dan, thanks for commenting, these are our reasons: For allow developers to create their own PUSH Notifications plugins using services like Pushwoosh or Amazon Instant messages. So we need a core table to allow these plugins to retrieve the phone information. Because we need a core web service function to be added into the "core service" Moodle Mobile. You can't add functions from other plugins to existing core services For having the airnotifier output plugin outside core, so you can install it as a standard add-one In this issue you have more information about the previous and new workflow MOBILE-452
            Hide
            rwijaya Rossiani Wijaya added a comment -

            This is working as expected.

            Tested for master only.

            Test passed.

            Show
            rwijaya Rossiani Wijaya added a comment - This is working as expected. Tested for master only. Test passed.
            Hide
            stronk7 Eloy Lafuente (stronk7) added a comment -

            Yes, it's happening (somewhere in the French Polynesia, right now). And you did it, raising Moodle to new excellency levels.

            Or, if you prefer, yes, you fixed that boring issue.

            Thanks anyway! Ciao

            Show
            stronk7 Eloy Lafuente (stronk7) added a comment - Yes, it's happening (somewhere in the French Polynesia, right now). And you did it, raising Moodle to new excellency levels. Or, if you prefer, yes, you fixed that boring issue. Thanks anyway! Ciao

              People

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

                Dates

                • Created:
                  Updated:
                  Resolved:
                  Fix Release Date:
                  18/Nov/13