Uploaded image for project: 'Moodle'
  1. Moodle
  2. MDL-36456

Amazon S3 invalid JSON string error could be improved

    Details

    • Type: Improvement
    • Status: Closed
    • Priority: Minor
    • Resolution: Fixed
    • Affects Version/s: 2.2.5, 2.3
    • Fix Version/s: 2.3.4, 2.4.1
    • Component/s: Repositories
    • Labels:
    • Testing Instructions:
      Hide
      1. Go to repositories and enable the Amazon S3 service.
      2. Enter in '1234' for the Access key and Secret Key
      3. Visit a course and click to add a file resource.
      4. Click on 'Add' under 'Select files' to open the file manager
      5. Click on Amazon S3
      6. Make sure a nice error appear instead of a bad JSON error
      7. Try with JS off, and Make sure you get an exception message.
      Show
      Go to repositories and enable the Amazon S3 service. Enter in '1234' for the Access key and Secret Key Visit a course and click to add a file resource. Click on 'Add' under 'Select files' to open the file manager Click on Amazon S3 Make sure a nice error appear instead of a bad JSON error Try with JS off, and Make sure you get an exception message.
    • Affected Branches:
      MOODLE_22_STABLE, MOODLE_23_STABLE
    • Fixed Branches:
      MOODLE_23_STABLE, MOODLE_24_STABLE
    • Pull from Repository:
    • Pull Master Branch:
      MDL-36456-master

      Description

      1. Go to repositories and enable the Amazon S3 service.
      2. Enter in '1234' for the Access key and Secret Key
      3. Visit a course and click to add a file resource.
      4. Click on 'Add' under 'Select files' to open the file manager
      5. Click on Amazon S3

      Expected result: You should see a friendly message like "Could not connect with current setup information"

      Actual result: You get an invalid JSON string error

        Gliffy Diagrams

          Attachments

            Issue Links

              Activity

              Hide
              fred Frédéric Massart added a comment -

              This is error handling related, each repository should handle them better. We might have to create a meta issue for that purpose. Most repositories will badly fail if the access keys are wrong.

              Show
              fred Frédéric Massart added a comment - This is error handling related, each repository should handle them better. We might have to create a meta issue for that purpose. Most repositories will badly fail if the access keys are wrong.
              Hide
              markn Mark Nelson added a comment -

              Hi Michael, I don't think it should be could be improved, but rather should be.

              The error is as follows:

              Invalid JSON string

              Warning: S3::listBuckets(): [InvalidAccessKeyId] The AWS Access Key Id you provided does not exist in our records. in /var/www/mstorage/im/moodle/repository/s3/S3.php on line 222

              Warning: Invalid argument supplied for foreach() in /var/www/mstorage/im/moodle/repository/s3/lib.php on line 101
              {"list":[],"path":[

              {"name":"Amazon S3","path":"","icon":"http:\/\/localhost\/im\/theme\/image.php\/standard\/core\/1352436185\/f\/folder-24"}

              ],"manage":false,"dynload":true,"nologin":true,"nosearch":true,"repo_id":12}

              Show
              markn Mark Nelson added a comment - Hi Michael, I don't think it should be could be improved, but rather should be. The error is as follows: Invalid JSON string Warning: S3::listBuckets(): [InvalidAccessKeyId] The AWS Access Key Id you provided does not exist in our records. in /var/www/mstorage/im/moodle/repository/s3/S3.php on line 222 Warning: Invalid argument supplied for foreach() in /var/www/mstorage/im/moodle/repository/s3/lib.php on line 101 {"list":[],"path":[ {"name":"Amazon S3","path":"","icon":"http:\/\/localhost\/im\/theme\/image.php\/standard\/core\/1352436185\/f\/folder-24"} ],"manage":false,"dynload":true,"nologin":true,"nosearch":true,"repo_id":12}
              Hide
              markn Mark Nelson added a comment -

              There is also a disruptive pop-up. It is not a valid error message to show a user, so it's not like we could improve it by simply wording it better. It needs to be handled correctly.

              Show
              markn Mark Nelson added a comment - There is also a disruptive pop-up. It is not a valid error message to show a user, so it's not like we could improve it by simply wording it better. It needs to be handled correctly.
              Hide
              fred Frédéric Massart added a comment -

              No drama Mark, I will work on that

              Show
              fred Frédéric Massart added a comment - No drama Mark, I will work on that
              Hide
              fred Frédéric Massart added a comment - - edited

              I have created a new generic string which can tell the user that there was a problem while communicating with the plugin. Unfortunately we cannot identify the real problem during the communication with S3, or it would require some logic which might be a bit too much just to display a nice error.

              Another option would have been to display the exception from S3, which tells us that the information provided to login are incorrect, that way there is no need to create a new lang string, but I was not convinced that this would be nice enough for the user.

              Also I changed the die(json_encode()) because now the repositories handle exception properly.

              Show
              fred Frédéric Massart added a comment - - edited I have created a new generic string which can tell the user that there was a problem while communicating with the plugin. Unfortunately we cannot identify the real problem during the communication with S3, or it would require some logic which might be a bit too much just to display a nice error. Another option would have been to display the exception from S3, which tells us that the information provided to login are incorrect, that way there is no need to create a new lang string, but I was not convinced that this would be nice enough for the user. Also I changed the die(json_encode()) because now the repositories handle exception properly.
              Hide
              markn Mark Nelson added a comment -

              Looks great to me. Submitting for integration.

              Show
              markn Mark Nelson added a comment - Looks great to me. Submitting for integration.
              Hide
              poltawski Dan Poltawski added a comment -

              Integrated to master 24 and 23, thanks Fred.

              Show
              poltawski Dan Poltawski added a comment - Integrated to master 24 and 23, thanks Fred.
              Hide
              dmonllao David Monllaó added a comment -

              It passes, tested in 23 and master

              Show
              dmonllao David Monllaó added a comment - It passes, tested in 23 and master
              Hide
              stronk7 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
              stronk7 Eloy Lafuente (stronk7) added a comment - And your fantastic code has met core, hope they become good friends for a long period. Closing, thanks!

                People

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

                  Dates

                  • Created:
                    Updated:
                    Resolved:
                    Fix Release Date:
                    14/Jan/13