Moodle
  1. Moodle
  2. MDL-45041

WebService add_user_device doesn't store entries for different users using the same push key

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Minor Minor
    • Resolution: Fixed
    • Affects Version/s: 2.6.4, 2.7.1
    • Fix Version/s: 2.6.5, 2.7.2
    • Component/s: Web Services
    • Labels:
    • Testing Instructions:
      Hide

      Note: I've bumped the version number so the upgrade.php is executed

      Once the site is upgraded, you need to test the WebService:

      Enable Web Services in Advanced features
      Enable Web services for mobile devices in Plugins / WebServices / Services
      Use this client: https://gist.github.com/jleyva/6669774
      The required curl.php file is here: https://github.com/moodlehq/sample-ws-clients/blob/master/PHP-REST/curl.php

      You need two tokens (for different users) related to the Mobile service:

      Create Token:
      Click on Site administration ► Plugins ► Web services ► Manage tokens
      Click add, select user and service (mobile)

      You need two tokens for two different users at the same site:

      1 Edit the client file for replacing the first token and the moodle url
      2 Run the client file in a browser, it should return just []
      3 You should check directly in the Moodle database that a new entry was created (user_devices table) for the user linked to the token
      3 Edit the client file again, replace the token with the second one you created
      4 Run the client in a browser
      5 Check in the Moodle database again that you have two entries in the user_devices tables, the field values must be the same (except for the id and userid fields)
      6 Run the client again in the browser, you will receive a message "Warning this key already exists for this user"
      7 Check that the Unique index pushid, platform in the user_devices table was deleted after upgrade

      Show
      Note: I've bumped the version number so the upgrade.php is executed Once the site is upgraded, you need to test the WebService: Enable Web Services in Advanced features Enable Web services for mobile devices in Plugins / WebServices / Services Use this client: https://gist.github.com/jleyva/6669774 The required curl.php file is here: https://github.com/moodlehq/sample-ws-clients/blob/master/PHP-REST/curl.php You need two tokens (for different users) related to the Mobile service: Create Token: Click on Site administration ► Plugins ► Web services ► Manage tokens Click add, select user and service (mobile) You need two tokens for two different users at the same site: 1 Edit the client file for replacing the first token and the moodle url 2 Run the client file in a browser, it should return just [] 3 You should check directly in the Moodle database that a new entry was created (user_devices table) for the user linked to the token 3 Edit the client file again, replace the token with the second one you created 4 Run the client in a browser 5 Check in the Moodle database again that you have two entries in the user_devices tables, the field values must be the same (except for the id and userid fields) 6 Run the client again in the browser, you will receive a message "Warning this key already exists for this user" 7 Check that the Unique index pushid, platform in the user_devices table was deleted after upgrade
    • Affected Branches:
      MOODLE_26_STABLE, MOODLE_27_STABLE
    • Fixed Branches:
      MOODLE_26_STABLE, MOODLE_27_STABLE
    • Pull from Repository:
    • Pull 2.6 Branch:
      MDL-45041_26_STABLE
    • Pull 2.7 Branch:
      MDL-45041_27_STABLE
    • Pull Master Branch:

      Description

      This may happen if you have two different users in the same Moodle site (a student and teacher account or manager account, etc...) and you want to add both in the Mobile app. Push keys are unique for apps, so the same push key will be valid for two users in the same site.

      The problem is that we prevent duplicate entries for the pair "pushid, platform", we should delete the code check and also the unique key (in upgrade.php and install.xml)

      <KEY NAME="pushid-platform" TYPE="unique" FIELDS="pushid, platform"/>
      

        Gliffy Diagrams

          Issue Links

            Activity

            Hide
            CiBoT added a comment -

            Results for MDL-45041

            • Remote repository: git://github.com/jleyva/moodle.git
            Show
            CiBoT added a comment - Results for MDL-45041 Remote repository: git://github.com/jleyva/moodle.git Remote branch MDL-45041 to be integrated into upstream master Executed job http://integration.moodle.org/job/Precheck%20remote%20branch/3089 Error: The MDL-45041 branch at git://github.com/jleyva/moodle.git does not apply clean to master
            Hide
            CiBoT added a comment -

            Results for MDL-45041

            • Remote repository: git://github.com/jleyva/moodle.git
            Show
            CiBoT added a comment - Results for MDL-45041 Remote repository: git://github.com/jleyva/moodle.git Remote branch MDL-45041 to be integrated into upstream master Executed job http://integration.moodle.org/job/Precheck%20remote%20branch/3091 Details: http://integration.moodle.org/job/Precheck%20remote%20branch/3091/artifact/work/smurf.html
            Hide
            Ankit Agarwal added a comment -

            Hi Juan,
            The patch looks good.
            You have also removed the only usage of query that was filtering on the indexed columns, so should be safe to delete the index.
            Does it need backporting? Also can you please mention in the testing instructions to verify the index was deleted in database.

            On an un-related note, we should be using $DB->record_exists instead of get_record for those checks.

            Cheers.

            Show
            Ankit Agarwal added a comment - Hi Juan, The patch looks good. You have also removed the only usage of query that was filtering on the indexed columns, so should be safe to delete the index. Does it need backporting? Also can you please mention in the testing instructions to verify the index was deleted in database. On an un-related note, we should be using $DB->record_exists instead of get_record for those checks. Cheers.
            Hide
            Juan Leyva added a comment -

            Submitting for integration, I've rebased.

            This bug doesn't need backporting

            Testing instructions updated

            Show
            Juan Leyva added a comment - Submitting for integration, I've rebased. This bug doesn't need backporting Testing instructions updated
            Hide
            Marina Glancy added a comment -

            Thanks Juan, I added a label integration_held and it will be integrated after the end of on-sync period (in two weeks). You will need to rebase again and change version number then. Also think if you want to retain this index without unique-constraint to use in select queries by those two fields.

            Show
            Marina Glancy added a comment - Thanks Juan, I added a label integration_held and it will be integrated after the end of on-sync period (in two weeks). You will need to rebase again and change version number then. Also think if you want to retain this index without unique-constraint to use in select queries by those two fields.
            Hide
            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
            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
            CiBoT added a comment -

            Moving this issue to current integration cycle, will be reviewed soon. Thanks for the hard work!

            Show
            CiBoT added a comment - Moving this issue to current integration cycle, will be reviewed soon. Thanks for the hard work!
            Hide
            Dan Poltawski added a comment -

            Hi Juan,

            1. I was talking with Eloy about this and he recommended the use of drop_key() rather than drop_index() - which will ensure in future if we enforce the referential integrity that things get sorted out properly
            2. What is not entirely clear from this issue is what the use case is for two users with the same push key. Could explain this more? Thinking about it from the iOS point of view, it might be quite confusing if multiple users are logged into the same device - especially shared devices...
            Show
            Dan Poltawski added a comment - Hi Juan, I was talking with Eloy about this and he recommended the use of drop_key() rather than drop_index() - which will ensure in future if we enforce the referential integrity that things get sorted out properly What is not entirely clear from this issue is what the use case is for two users with the same push key. Could explain this more? Thinking about it from the iOS point of view, it might be quite confusing if multiple users are logged into the same device - especially shared devices...
            Hide
            CiBoT added a comment -

            Moving this reopened issue out from current integration. Please, re-submit it for integration once ready.

            Show
            CiBoT added a comment - Moving this reopened issue out from current integration. Please, re-submit it for integration once ready.
            Hide
            Juan Leyva added a comment -

            Hi Dan,

            1. Thanks, I'll change it

            2. Currently the app supports multiple sites for multiple users.
            So imagine that you have two user accounts in the same Moodle installation, you are going to have always the same Push Token (pushid) since is unique per device/app.
            Once the app receives a notification it can detect the site/user where it came from but if the app is in background the notification "short text" is going to be displayed in the notifications tray.

            We are aware of the problem of shared devices, it's possible to receive notifications from different sites in a shared device so a user is going to receive notifications from everywhere, in that case, the user should configure his message settings in order to avoid receiving Push notifications (or just to avoid receive private messages etc... )

            I think this is something that should be explained in the app docs

            Show
            Juan Leyva added a comment - Hi Dan, 1. Thanks, I'll change it 2. Currently the app supports multiple sites for multiple users. So imagine that you have two user accounts in the same Moodle installation, you are going to have always the same Push Token (pushid) since is unique per device/app. Once the app receives a notification it can detect the site/user where it came from but if the app is in background the notification "short text" is going to be displayed in the notifications tray. We are aware of the problem of shared devices, it's possible to receive notifications from different sites in a shared device so a user is going to receive notifications from everywhere, in that case, the user should configure his message settings in order to avoid receiving Push notifications (or just to avoid receive private messages etc... ) I think this is something that should be explained in the app docs
            Hide
            CiBoT added a comment -

            +1 code verified against automated checks.

            Checked MDL-45041 using repository: git://github.com/jleyva/moodle.git

            More information about this report

            Show
            CiBoT added a comment - +1 code verified against automated checks. Checked MDL-45041 using repository: git://github.com/jleyva/moodle.git MOODLE_26_STABLE (branch: MDL-45041_26_STABLE | CI Job ) MOODLE_27_STABLE (branch: MDL-45041_27_STABLE | CI Job ) master (branch: MDL-45041 | CI Job ) More information about this report
            Hide
            CiBoT added a comment -

            Moving this issue to current integration cycle, will be reviewed soon. Thanks for the hard work!

            Show
            CiBoT added a comment - Moving this issue to current integration cycle, will be reviewed soon. Thanks for the hard work!
            Hide
            Sam Hemelryk added a comment -

            Thanks Juan this has been integrated now.

            Show
            Sam Hemelryk added a comment - Thanks Juan this has been integrated now.
            Hide
            Jetha Chan added a comment -

            Thanks for working on this, Juan! Tests passed on 2.6, 2.7 and master.

            Show
            Jetha Chan added a comment - Thanks for working on this, Juan! Tests passed on 2.6, 2.7 and master.
            Hide
            Dan Poltawski added a comment -

            Thanks for your contribution - this change is now part of Moodle!

            True knowledge exists in knowing that you know nothing.
            – Socrates

            Show
            Dan Poltawski added a comment - Thanks for your contribution - this change is now part of Moodle! True knowledge exists in knowing that you know nothing. – Socrates

              People

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

                Dates

                • Created:
                  Updated:
                  Resolved: