Moodle
  1. Moodle
  2. MDL-37217

Folders Not Showing in Google Docs/Drive Repository

    Details

    • Testing Instructions:
      Hide

      Test pre-requisites

      • Enable Google Drive repository (formerly named Google Docs)
      • If previously enabled, make sure you have enable the service 'Drive API' on your Google apis panel
      • Your Google Drive should contain
        • Folders and files on each folder level
        • Google documents of each type (Drawing is not supported)
        • Real file of multiple types (pictures, documents, zips, spreadsheets, PDFs, presentations, etc...)

      Test steps

      1. Go to your private files
      2. Browse the Google Drive repository and login
      3. Make sure
        • you can browse the Drive (go to folders, sub folders)
        • downloading files from the root folder and sub folders work
        • you can download (and save) each of those type of files:
          • image
          • zip
          • document
          • spreadsheet
          • presentation
          • PDF
          • another of your choice
        • the search works, and downloading files from there too (the search should search in title and file content)
        • the breadcrumb works in both browsing and search modes
      4. Save your private files with new added files
      5. Go to a forum and add an attachment to a forum post
      6. Logout from Moodle
      7. Login as the same user as previously
      8. Go to your privates files
      9. Confirm that the previously added files are there, accessible and downloadable
      10. Open the file picker
      11. Confirm
        • that you need to click "Login" to view your Google Drive repository
        • that you do not have to allow the application any more. (The authorisation popup opens and closes itself)
      12. Go to the previously created forum post
      13. Confirm that the attachment is downloadable/visible
      Show
      Test pre-requisites Enable Google Drive repository (formerly named Google Docs) If previously enabled, make sure you have enable the service 'Drive API' on your Google apis panel Your Google Drive should contain Folders and files on each folder level Google documents of each type (Drawing is not supported) Real file of multiple types (pictures, documents, zips, spreadsheets, PDFs, presentations, etc...) Test steps Go to your private files Browse the Google Drive repository and login Make sure you can browse the Drive (go to folders, sub folders) downloading files from the root folder and sub folders work you can download (and save) each of those type of files: image zip document spreadsheet presentation PDF another of your choice the search works, and downloading files from there too (the search should search in title and file content) the breadcrumb works in both browsing and search modes Save your private files with new added files Go to a forum and add an attachment to a forum post Logout from Moodle Login as the same user as previously Go to your privates files Confirm that the previously added files are there, accessible and downloadable Open the file picker Confirm that you need to click "Login" to view your Google Drive repository that you do not have to allow the application any more. (The authorisation popup opens and closes itself) Go to the previously created forum post Confirm that the attachment is downloadable/visible
    • Affected Branches:
      MOODLE_23_STABLE, MOODLE_24_STABLE
    • Fixed Branches:
      MOODLE_25_STABLE
    • Pull from Repository:
    • Pull Master Branch:
      MDL-37217-master
    • Rank:
      46811

      Description

      The problem with ALL file types showing in the Google Docs (or Google Drive) Repository was solved. We now show a list of all files. This was solved in Moodle version 2.3.

      Although, the corresponding FOLDERS in Google Docs (or Drive) still can not be seen. The Google Repository would be easier to use if all corresponding FOLDERS and FILES were able to be seen in the Google Docs Repository.

        Issue Links

          Activity

          Hide
          Helen Foster added a comment -

          David, thanks for your report. I'm assigning it to Fred as repositories component lead and adding Google Docs repository expert Dan as a watcher.

          Show
          Helen Foster added a comment - David, thanks for your report. I'm assigning it to Fred as repositories component lead and adding Google Docs repository expert Dan as a watcher.
          Hide
          Dan Poltawski added a comment -

          Hi David,

          Unfortunately this is a missing feature of the Google Docs repository. I didn't get round to implementing this when originally creating it. However being a google repository the search works pretty well

          Show
          Dan Poltawski added a comment - Hi David, Unfortunately this is a missing feature of the Google Docs repository. I didn't get round to implementing this when originally creating it. However being a google repository the search works pretty well
          Hide
          Dan Poltawski added a comment -

          Spoke to fred about the potential of moving to the google-provided SDK. By the looks of it, the drive SDK is much nicer and more suitable than when it was first created, so if that is the more pragmatic way to solve this issue it gets +1 from me.

          Show
          Dan Poltawski added a comment - Spoke to fred about the potential of moving to the google-provided SDK. By the looks of it, the drive SDK is much nicer and more suitable than when it was first created, so if that is the more pragmatic way to solve this issue it gets +1 from me.
          Hide
          Frédéric Massart added a comment - - edited

          Pushing for peer review.

          • The Google PHP SDK has been added, unfortunately it uses a global variable $apiConfig everywhere, which is quite bad, but as our naming convention does not allow for camel case, that should not be an issue.
          • Google Docs has been renamed to Google Drive.
          • The folders are now browseable.
          • Files can display their thumbnails instead of their icons.
          • The search has a breadcrumb which allows for folders browsing and going back to search results.
          • Retrieving a file from Google Drive could not be operated by normal Curl call as authentication is needed and provided by the API.
          • Support for file linking (URL) will be investigated in related issue.
          • The default export for documents is stil .rtf as before, a related issue will take care of that.
          • Presentations and Spreadsheets are now .xlsx and .pptx as .ppt and .xls are not available through the API.
          • Added clean up of parameters passed back to the repository (path, source).

          (It is easier to consult the diff of the second commit only https://github.com/FMCorz/moodle/commit/4118b00, as Github struggle generating the compare page. Although I have added a file in the first commit, so you should have a look at it too)

          Show
          Frédéric Massart added a comment - - edited Pushing for peer review. The Google PHP SDK has been added, unfortunately it uses a global variable $apiConfig everywhere, which is quite bad, but as our naming convention does not allow for camel case, that should not be an issue. Google Docs has been renamed to Google Drive. The folders are now browseable. Files can display their thumbnails instead of their icons. The search has a breadcrumb which allows for folders browsing and going back to search results. Retrieving a file from Google Drive could not be operated by normal Curl call as authentication is needed and provided by the API. Support for file linking (URL) will be investigated in related issue. The default export for documents is stil .rtf as before, a related issue will take care of that. Presentations and Spreadsheets are now .xlsx and .pptx as .ppt and .xls are not available through the API. Added clean up of parameters passed back to the repository (path, source). (It is easier to consult the diff of the second commit only https://github.com/FMCorz/moodle/commit/4118b00 , as Github struggle generating the compare page. Although I have added a file in the first commit, so you should have a look at it too)
          Hide
          Damyon Wiese added a comment -

          Hi Fred,

          Looking now - the first thing I spotted is that the google library should be added to lib/thirdpartylibs.xml

          Show
          Damyon Wiese added a comment - Hi Fred, Looking now - the first thing I spotted is that the google library should be added to lib/thirdpartylibs.xml
          Hide
          Damyon Wiese added a comment -

          The code all looks great.

          One issue I found is that when I tested using folders in my google drive - none of the documents in the folder showed up when browsing (but they did on a search).

          My folder was called "Testing folders".

          Show
          Damyon Wiese added a comment - The code all looks great. One issue I found is that when I tested using folders in my google drive - none of the documents in the folder showed up when browsing (but they did on a search). My folder was called "Testing folders".
          Hide
          Damyon Wiese added a comment -

          One other difference to the old implementation (I think it is an improvement) is that files that are shared with you do not show in the listing/search results (unless you add the folder to My files).

          Show
          Damyon Wiese added a comment - One other difference to the old implementation (I think it is an improvement) is that files that are shared with you do not show in the listing/search results (unless you add the folder to My files).
          Hide
          Damyon Wiese added a comment -

          Checklist:

          [Y] Syntax
          [Y] Output
          [Y] Whitespace
          [Y] Language
          [-] Databases
          [Y] Testing
          [Y] Security
          [Y] Documentation (has docs_required label)
          [Y] Git
          [-] Sanity check (Just 2 comments above to sort out).

          Show
          Damyon Wiese added a comment - Checklist: [Y] Syntax [Y] Output [Y] Whitespace [Y] Language [-] Databases [Y] Testing [Y] Security [Y] Documentation (has docs_required label) [Y] Git [-] Sanity check (Just 2 comments above to sort out).
          Hide
          Frédéric Massart added a comment -

          Many thanks Damyon!

          I've added the information in thirdpart.xml and removed the clean up of the path which caused your folders to be empty.

          As you noticed only the files of My files are displayed, but any file on the Drive can be found using the search. An alternative to this would be to create static folders ('My files', 'Recent', 'Shared with me', ...), as I did with the Evernote repository. Of course, by default we can set the user in 'My files'. I'd suggest to raise this as another issue if need be.

          We also noticed that the Google Documents type 'Drawing' are not supported, this is not a regression and could be taken care of in another issue.

          Pushing for integration.
          Cheers,
          Fred

          Show
          Frédéric Massart added a comment - Many thanks Damyon! I've added the information in thirdpart.xml and removed the clean up of the path which caused your folders to be empty. As you noticed only the files of My files are displayed, but any file on the Drive can be found using the search. An alternative to this would be to create static folders ('My files', 'Recent', 'Shared with me', ...), as I did with the Evernote repository. Of course, by default we can set the user in 'My files'. I'd suggest to raise this as another issue if need be. We also noticed that the Google Documents type 'Drawing' are not supported, this is not a regression and could be taken care of in another issue. Pushing for integration. Cheers, Fred
          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
          Dan Poltawski added a comment -

          Hi Fred,

          Looks good. Just a few things

          1. You haven't considered sites with $CFG->proxyhost set. The previous implementation uses our curl class to do the requests to the server which means that the repository will work behind a proxy. You'll need to make the google classes do this too. Ideally you'd use the curl class (so we have a centralised piece of code for http requests which handles all different proxies, problems with open_basedir (MDL-33955) etc).
          2. One thing I noticed is that your search is now only searching titles. Previously I believe it was searching all content. I understand why you made this decision, but just to be clear - are you sure we want to change this behaviour?
          3. get_file_reference() - I didn't think that this method was used when the repo only supports FILE_INTERNAL?

          I'm afraid i'm gonna have to reopen it for the first point, we can't break this repository fro sites behind proxies.

          cheers,
          Dan

          Show
          Dan Poltawski added a comment - Hi Fred, Looks good. Just a few things You haven't considered sites with $CFG->proxyhost set. The previous implementation uses our curl class to do the requests to the server which means that the repository will work behind a proxy. You'll need to make the google classes do this too. Ideally you'd use the curl class (so we have a centralised piece of code for http requests which handles all different proxies, problems with open_basedir ( MDL-33955 ) etc). One thing I noticed is that your search is now only searching titles. Previously I believe it was searching all content. I understand why you made this decision, but just to be clear - are you sure we want to change this behaviour? get_file_reference() - I didn't think that this method was used when the repo only supports FILE_INTERNAL? I'm afraid i'm gonna have to reopen it for the first point, we can't break this repository fro sites behind proxies. cheers, Dan
          Hide
          Frédéric Massart added a comment -

          Thanks Dan,

          1. In fact, I didn't think about that at all! It looks like we can extend the classes from Google quite easily, I will see how I could make them use our curl class.
          2. I'm glad you know why I did that because I don't . I will update the code to do a full search, not only a title one. (The fullText search includes title, description and content)
          3. I think you're mistaking get_file_by_reference() and get_file_reference(). The latter is used to convert the information source from the repository into something else, sometimes depending on the type of source requested. I thought that cleaning the parameter couldn't make any harm.

          Cheers,
          Fred

          Show
          Frédéric Massart added a comment - Thanks Dan, 1. In fact, I didn't think about that at all! It looks like we can extend the classes from Google quite easily, I will see how I could make them use our curl class. 2. I'm glad you know why I did that because I don't . I will update the code to do a full search, not only a title one. (The fullText search includes title, description and content) 3. I think you're mistaking get_file_by_reference() and get_file_reference(). The latter is used to convert the information source from the repository into something else, sometimes depending on the type of source requested. I thought that cleaning the parameter couldn't make any harm. Cheers, Fred
          Hide
          Frédéric Massart added a comment -

          Sending back for a peer review. Please note that very few things have been modified since the last review.

          I have added a new file lib/google/curlio.php which contains a class extending Google one's. So that we can make use of our class curl.

          I also fixed the search which is now full and not title based.

          Show
          Frédéric Massart added a comment - Sending back for a peer review. Please note that very few things have been modified since the last review. I have added a new file lib/google/curlio.php which contains a class extending Google one's. So that we can make use of our class curl. I also fixed the search which is now full and not title based.
          Hide
          Damyon Wiese added a comment -

          Hi Fred,

          I found

          error_log(print_r($optCurlParams, true));
          die();

          in "lib/google/io/Google_CurlIO.php".

          Also - do we need that special handling for the constants in the curl wrapper? The wrapper will still set the option if it is passed as an int (it just wont print nicely when debugging).

          Cheers, Damyon

          Show
          Damyon Wiese added a comment - Hi Fred, I found error_log(print_r($optCurlParams, true)); die(); in "lib/google/io/Google_CurlIO.php". Also - do we need that special handling for the constants in the curl wrapper? The wrapper will still set the option if it is passed as an int (it just wont print nicely when debugging). Cheers, Damyon
          Hide
          Frédéric Massart added a comment -

          Thanks Damyon,

          nice catch on the die(), I hate myself for leaving that there!

          Yes I think we do need to transform constants back to strings when they are passed to the wrapper. I've been facing a few problems, but the main one that I can think of now is that you could set the same constant twice, once with the constant name, once with the constant value, if so, both will be passed to curl in a random order and the result will be random too. Also, some other methods such as cleanopt() should also unset the entries using the constant values.

          Overall I think it's neater and less random to transform the constants back to a string when we can.

          Pushing for integration again, thanks!

          Show
          Frédéric Massart added a comment - Thanks Damyon, nice catch on the die(), I hate myself for leaving that there! Yes I think we do need to transform constants back to strings when they are passed to the wrapper. I've been facing a few problems, but the main one that I can think of now is that you could set the same constant twice, once with the constant name, once with the constant value, if so, both will be passed to curl in a random order and the result will be random too. Also, some other methods such as cleanopt() should also unset the entries using the constant values. Overall I think it's neater and less random to transform the constants back to a string when we can. Pushing for integration again, thanks!
          Hide
          Damyon Wiese 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.

          Cheers!

          Show
          Damyon Wiese 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. Cheers!
          Hide
          Dan Poltawski added a comment -

          How about a verison number increment..?

          Show
          Dan Poltawski added a comment - How about a verison number increment..?
          Hide
          Frédéric Massart added a comment -

          http://tinyurl.com/6plr25c

          Done . It didn't seem required to me as there is no database change, or anything, but there you go!

          Show
          Frédéric Massart added a comment - http://tinyurl.com/6plr25c Done . It didn't seem required to me as there is no database change, or anything, but there you go!
          Hide
          Dan Poltawski added a comment -

          Hi Fred,

          1. MAJOR: Thumbnails seem to be broken to me. The initial images filetype images load and then I get a load of broken images load on top
          2. MAJOR: You are requesting an offline token, why? (Compare the google authorisation prompt between 2.4 and 2.5 'Perform these operations when I'm not using the application' is in)
          3. MAJOR: The way that you are using the token seems to be a regression from previous behaviour. Try this on 2.4:
            1) Login to Moodle
            2) Login to Google
            3) Use google docs plugin (you will be asked for authorisation first time)
            4) Logout of moodle
            5) Login to moodle
            6) Use google docs plugin (you WONT be asked for authorisation on subsequent attempts)
            With your new patch, the user gets an authorisation prompt every time. The user experience here is important (to me, at least). I wouldn't be surprised if this is related to the offline access.
          4. There are no testing instructions for upgrade. And when attempting an upgrade I notice that this will be another google breaking upgrade (i.e. you'll need to go and fiddle with API settings in google). This concerns me, since it was only 2.3 where we last broke it. We need to be really sure that we want to break this for all our users again.
          5. I'd like to see more test steps like I put in MDL-29857
          6. I'd feel much happier about this new library if you'd also successfully converted the picasa plugin & portfolio plugins (will that need more api console tweaking, code changes etc)
          7. You should increment the version number, as you're changing the behaviour, lang strings, code etc. It would be goood to also increment the main version moodle number and change the requires version of the plugin (these will conflict, for sure, but at least it gives the integrator a prompt)
          8. lib/google/curlio.php 'is part of Moodle', so would've preferred to avoid the camelcase inner variables It has trailing whitespace also.

          I know that is a big list and we're going through a few iterations. But that is the peril of changing the core libraries (especially one close to my heart :-P )

          thanks,
          Dan

          Show
          Dan Poltawski added a comment - Hi Fred, MAJOR: Thumbnails seem to be broken to me. The initial images filetype images load and then I get a load of broken images load on top MAJOR: You are requesting an offline token, why? (Compare the google authorisation prompt between 2.4 and 2.5 'Perform these operations when I'm not using the application' is in) MAJOR: The way that you are using the token seems to be a regression from previous behaviour. Try this on 2.4: 1) Login to Moodle 2) Login to Google 3) Use google docs plugin (you will be asked for authorisation first time) 4) Logout of moodle 5) Login to moodle 6) Use google docs plugin (you WONT be asked for authorisation on subsequent attempts) With your new patch, the user gets an authorisation prompt every time. The user experience here is important (to me, at least). I wouldn't be surprised if this is related to the offline access. There are no testing instructions for upgrade. And when attempting an upgrade I notice that this will be another google breaking upgrade (i.e. you'll need to go and fiddle with API settings in google). This concerns me, since it was only 2.3 where we last broke it. We need to be really sure that we want to break this for all our users again. I'd like to see more test steps like I put in MDL-29857 I'd feel much happier about this new library if you'd also successfully converted the picasa plugin & portfolio plugins (will that need more api console tweaking, code changes etc) You should increment the version number, as you're changing the behaviour, lang strings, code etc. It would be goood to also increment the main version moodle number and change the requires version of the plugin (these will conflict, for sure, but at least it gives the integrator a prompt) lib/google/curlio.php 'is part of Moodle', so would've preferred to avoid the camelcase inner variables It has trailing whitespace also. I know that is a big list and we're going through a few iterations. But that is the peril of changing the core libraries (especially one close to my heart :-P ) thanks, Dan
          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
          Frédéric Massart added a comment -

          Thanks for your review Dan!

          1/ As we noticed together, this was due to a 3rd party cookie setting in your browser. I've changed the code so that only a few file types will display the real thumbnail. Those types display the thumbnail whether or not you are logged.
          2/ Thanks for pointing that out, I noticed that the API allows to pass a parameter to require an online token. I've changed the our default settings of the API to require an online token by default.
          3/ Again something that I didn't notice, cheers! I did the same as for the online token, as for now our settings set the prompt not to be forced.
          4/ I've added the api_change label to this issue to mention this in the release notes. As you noticed the upgrade process only requires an admin to login into their Google Console to enable the 'Drive API'.
          5/ I've added a few more steps, are you happy with them the way it is?
          6/ Unfortunately Picasa is not supported out-of-the-box by the SDK, it must be possible to use it with the SDK, but I'd prefer raising an issue for it, and for the portfolios.
          7. Done.
          8. Fixed on the non-class related variables.

          I'm pushing it for integration again as the changes were small and straight forward.

          Cheers!
          Fred

          Show
          Frédéric Massart added a comment - Thanks for your review Dan! 1/ As we noticed together, this was due to a 3rd party cookie setting in your browser. I've changed the code so that only a few file types will display the real thumbnail. Those types display the thumbnail whether or not you are logged. 2/ Thanks for pointing that out, I noticed that the API allows to pass a parameter to require an online token. I've changed the our default settings of the API to require an online token by default. 3/ Again something that I didn't notice, cheers! I did the same as for the online token, as for now our settings set the prompt not to be forced. 4/ I've added the api_change label to this issue to mention this in the release notes. As you noticed the upgrade process only requires an admin to login into their Google Console to enable the 'Drive API'. 5/ I've added a few more steps, are you happy with them the way it is? 6/ Unfortunately Picasa is not supported out-of-the-box by the SDK, it must be possible to use it with the SDK, but I'd prefer raising an issue for it, and for the portfolios. 7. Done. 8. Fixed on the non-class related variables. I'm pushing it for integration again as the changes were small and straight forward. Cheers! Fred
          Hide
          Frédéric Massart added a comment -

          Raised MDL-37984 to investigate Portfolio/Repository for Picasa and Google Docs.

          Show
          Frédéric Massart added a comment - Raised MDL-37984 to investigate Portfolio/Repository for Picasa and Google Docs.
          Hide
          Damyon Wiese 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.

          Thanks!

          Show
          Damyon Wiese 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. Thanks!
          Hide
          Damyon Wiese added a comment -

          Actually - I peer reviewed this one.

          Show
          Damyon Wiese added a comment - Actually - I peer reviewed this one.
          Hide
          Dan Poltawski added a comment -

          Hi Fred,

          I don't like the look of this at all:
          https://github.com/FMCorz/moodle/commit/e7d63bec3b8e11dccb9d0c022c1b46cf24272500

          It seems to me that its only necessary because you are not using the curl library in the way it was designed. (I'm not arguing the design of it good, but I think your addition is ugly and confuses the api further).

          I tried with this change and it seemed to work, but maybe I have missed something:
          https://github.com/danpoltawski/moodle/commit/ac5d61cc131b1652e02c43a33e68fd74daf02b78

          Show
          Dan Poltawski added a comment - Hi Fred, I don't like the look of this at all: https://github.com/FMCorz/moodle/commit/e7d63bec3b8e11dccb9d0c022c1b46cf24272500 It seems to me that its only necessary because you are not using the curl library in the way it was designed. (I'm not arguing the design of it good, but I think your addition is ugly and confuses the api further). I tried with this change and it seemed to work, but maybe I have missed something: https://github.com/danpoltawski/moodle/commit/ac5d61cc131b1652e02c43a33e68fd74daf02b78
          Hide
          Frédéric Massart added a comment -

          To me it does not look ugly, it just adds the possibility of using constants, but as you said, let's not discuss about the design of this class.

          Your patch seems to work but you haven't tested it extensively. An issue that could happen is the Google API using their standard of passing the curl constant set an option, and then our (or again theirs) API trying to overwrite this value using either the constant value or the constant name. Result: twice the same option in $this->options with possibly different values, and one affected before the other.

          This also means that a call to $this->cleanopt() could possibly leave some existing data in the class.

          If you think it's messing with the class, and it's safe to try out your patch, then I'm all for it. I have no problem with ignoring this new feature in curl(). I remember trying out that solution but then ignoring it because I couldn't get the API to work. Though I was probably missing something somewhere else if it works with your patch (if you try it again, keep in mind that Google might cache requests somewhere, though it shouldn't).

          Cheers,
          Fred

          Show
          Frédéric Massart added a comment - To me it does not look ugly, it just adds the possibility of using constants, but as you said, let's not discuss about the design of this class. Your patch seems to work but you haven't tested it extensively. An issue that could happen is the Google API using their standard of passing the curl constant set an option, and then our (or again theirs) API trying to overwrite this value using either the constant value or the constant name. Result: twice the same option in $this->options with possibly different values, and one affected before the other. This also means that a call to $this->cleanopt() could possibly leave some existing data in the class. If you think it's messing with the class, and it's safe to try out your patch, then I'm all for it. I have no problem with ignoring this new feature in curl(). I remember trying out that solution but then ignoring it because I couldn't get the API to work. Though I was probably missing something somewhere else if it works with your patch (if you try it again, keep in mind that Google might cache requests somewhere, though it shouldn't). Cheers, Fred
          Hide
          Dan Poltawski added a comment -
          An issue that could happen is the Google API using their standard of passing the curl constant set an option, and then our (or again theirs) API trying to overwrite this value using either the constant value or the constant name. Result: twice the same option in $this->options with possibly different values, and one affected before the other
          

          Please can you explain how that would happen? moodle_google_curlio does not seem to provide any option setting methods, so how would google be able to do the $curl->setopt() (our API). I don't see it.

          I don't like adding 'mixed methods' to the class unnecessarily, it increases the complexity and this reverse lookup is the opposite flow to how it works.

          Show
          Dan Poltawski added a comment - An issue that could happen is the Google API using their standard of passing the curl constant set an option, and then our (or again theirs) API trying to overwrite this value using either the constant value or the constant name. Result: twice the same option in $ this ->options with possibly different values, and one affected before the other Please can you explain how that would happen? moodle_google_curlio does not seem to provide any option setting methods, so how would google be able to do the $curl->setopt() (our API). I don't see it. I don't like adding 'mixed methods' to the class unnecessarily, it increases the complexity and this reverse lookup is the opposite flow to how it works.
          Hide
          Frédéric Massart added a comment - - edited

          Well the parent method provides setOptions(). Though now that you mention it, I grepped and it does not seem to be used anywhere. I guess what I could do is use your patch, and overwrite the function setOptions() to make sure that the keys are strings and not integers.

          Would you be ok with that solution?

          Show
          Frédéric Massart added a comment - - edited Well the parent method provides setOptions(). Though now that you mention it, I grepped and it does not seem to be used anywhere. I guess what I could do is use your patch, and overwrite the function setOptions() to make sure that the keys are strings and not integers. Would you be ok with that solution?
          Hide
          Dan Poltawski added a comment -

          You've not implemented that method though, so in fact it is impossible for it to be used?

          Show
          Dan Poltawski added a comment - You've not implemented that method though, so in fact it is impossible for it to be used?
          Hide
          Dan Poltawski added a comment -

          Ah aplogies, got my names mixed up.

          Show
          Dan Poltawski added a comment - Ah aplogies, got my names mixed up.
          Hide
          Dan Poltawski added a comment -

          I'll ask for another other integrators opinion on it before getting you to change it. I prefer the approach of keeping it to the google class, but am willing to go either way.

          Show
          Dan Poltawski added a comment - I'll ask for another other integrators opinion on it before getting you to change it. I prefer the approach of keeping it to the google class, but am willing to go either way.
          Hide
          Dan Poltawski added a comment - - edited

          Hi Fred,

          Discussed this with the integrators, and there is agreement to keep this 'int' constant handling in the Google class rather than change the behaviour of the curl class. As you rightly point out, there are many places where the curl class makes an assumption its string rather than the constant value so it makes sense to do that translation in the google wrapper class.

          Its good to go after that and i'll be happy to do this today (sorry its been around the block a few times, but I think we've improved it through the itterations).
          Thanks,
          Dan

          Show
          Dan Poltawski added a comment - - edited Hi Fred, Discussed this with the integrators, and there is agreement to keep this 'int' constant handling in the Google class rather than change the behaviour of the curl class. As you rightly point out, there are many places where the curl class makes an assumption its string rather than the constant value so it makes sense to do that translation in the google wrapper class. Its good to go after that and i'll be happy to do this today (sorry its been around the block a few times, but I think we've improved it through the itterations). Thanks, Dan
          Hide
          Frédéric Massart added a comment -

          Alright,

          I've updated my patch:

          • Curl class does not accept constants as options any more
          • Moodle Google CurlIO does not send constants
          • Moodle Google CurlIO translates the values if needed

          Cheers,
          Fred

          Show
          Frédéric Massart added a comment - Alright, I've updated my patch: Curl class does not accept constants as options any more Moodle Google CurlIO does not send constants Moodle Google CurlIO translates the values if needed Cheers, Fred
          Hide
          Dan Poltawski added a comment -

          Thanks Fred, i've integrated this now.

          I suggest we file a new issue for changing to the drive logo too: https://developers.google.com/drive/branding

          Show
          Dan Poltawski added a comment - Thanks Fred, i've integrated this now. I suggest we file a new issue for changing to the drive logo too: https://developers.google.com/drive/branding
          Hide
          Andrew Davis added a comment -

          Works as described. Passing.

          Show
          Andrew Davis added a comment - Works as described. Passing.
          Hide
          Frédéric Massart added a comment -

          Linking to MDL-38100.

          Show
          Frédéric Massart added a comment - Linking to MDL-38100 .
          Hide
          Damyon Wiese added a comment -

          Thanks for your hard work - this issue has made it! Moodle is now a little bit better.

          Regards, Damyon

          Show
          Damyon Wiese added a comment - Thanks for your hard work - this issue has made it! Moodle is now a little bit better. Regards, Damyon
          Hide
          Dan Poltawski added a comment -

          Fred, we need to get this change documented in the release notes - there is a linked QA failure which isn't really a failure (since it just needs doing on the QA site - but I thought we could use it as an opportunity to make sure this id properly documented).

          Show
          Dan Poltawski added a comment - Fred, we need to get this change documented in the release notes - there is a linked QA failure which isn't really a failure (since it just needs doing on the QA site - but I thought we could use it as an opportunity to make sure this id properly documented).
          Hide
          Helen Foster added a comment -

          Removing the docs_required label, as this improvement is now documented, also a note for administrators to enable the Drive API.

          http://docs.moodle.org/25/en/Google_Drive_repository
          http://docs.moodle.org/25/en/Google_OAuth_2.0_setup
          http://docs.moodle.org/dev/Moodle_2.5_release_notes

          Thanks to Fred for checking the docs.

          Show
          Helen Foster added a comment - Removing the docs_required label, as this improvement is now documented, also a note for administrators to enable the Drive API. http://docs.moodle.org/25/en/Google_Drive_repository http://docs.moodle.org/25/en/Google_OAuth_2.0_setup http://docs.moodle.org/dev/Moodle_2.5_release_notes Thanks to Fred for checking the docs.
          Hide
          Dan Poltawski added a comment -

          Hi Fred,

          This need to enable the 'Drive API' was missing the 'issues which might affect you' upgrade doc. I added a note to http://docs.moodle.org/25/en/Upgrading#Google_Docs_repository_plugin_upgraded_to_Google_Drive

          Could you verify/improve, thanks.

          Show
          Dan Poltawski added a comment - Hi Fred, This need to enable the 'Drive API' was missing the 'issues which might affect you' upgrade doc. I added a note to http://docs.moodle.org/25/en/Upgrading#Google_Docs_repository_plugin_upgraded_to_Google_Drive Could you verify/improve, thanks.
          Hide
          Frédéric Massart added a comment -

          All good Dan, thanks!

          Show
          Frédéric Massart added a comment - All good Dan, thanks!

            People

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

              Dates

              • Created:
                Updated:
                Resolved: