Moodle
  1. Moodle
  2. MDL-31409 Incorporate Lightwork into the Moodle core
  3. MDL-31863

Modify existing web service core_files_get_files to check the file modification time

    Details

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

      For each test, you will need to change the code between /// PARAMETERs as specified

      $pararms = array(contextid,"component","filearea",itemid,"filepath","filename");

      Test 1) - With no modified value, the web service returns a list of files according to the rest of the parameters.
      /// PARAMETERS
      $pararms = array(contextid,"component","filearea",itemid,"filepath","filename");
      /// PARAMETERS
      The web services should return a list of files.

      Test 2) - With a specified modified value, the web service returns error message.
      /// PARAMETERS
      $pararms = array(contextid,"component","filearea",itemid,"filepath","filename", modified e.g. 1334265452);
      /// PARAMETERS
      The web services should return a list of files which the modification times are equal or greater than the specified modified value.

      Show
      For each test, you will need to change the code between /// PARAMETERs as specified $pararms = array(contextid,"component","filearea",itemid,"filepath","filename"); Test 1) - With no modified value, the web service returns a list of files according to the rest of the parameters. /// PARAMETERS $pararms = array(contextid,"component","filearea",itemid,"filepath","filename"); /// PARAMETERS The web services should return a list of files. Test 2) - With a specified modified value, the web service returns error message. /// PARAMETERS $pararms = array(contextid,"component","filearea",itemid,"filepath","filename", modified e.g. 1334265452); /// PARAMETERS The web services should return a list of files which the modification times are equal or greater than the specified modified value.
    • Affected Branches:
      MOODLE_23_STABLE
    • Fixed Branches:
      MOODLE_23_STABLE
    • Pull Master Branch:

      Description

      Modify existing web service core_files_get_files to check the file modification time

      After clients have downloaded files from a file area for the first time they will want to make subsequent calls to this web service
      to check for files that have changed since the file was first downloaded.

      The parameters need to be modified so that the client can specify a modified_since value
      The return data needs to be modified to include the file's modification time

        Gliffy Diagrams

        1. MDL-31863-101b1d9.patch
          9 kB
          Aparup Banerjee
        2. smurf MDL-31863.xml
          11 kB
          Aparup Banerjee

          Activity

          Hide
          Jérôme Mouneyrac added a comment -

          Hi Paul,
          yes it would be very good to return modification time in this function. Make the param and return value optionals . I don't move this issue into the web service roadmap as it's not a new function.
          I'm going to assign all your issues to you, but if ever there are some that you can't do, assign them back to me.
          Cheers.

          Show
          Jérôme Mouneyrac added a comment - Hi Paul, yes it would be very good to return modification time in this function. Make the param and return value optionals . I don't move this issue into the web service roadmap as it's not a new function. I'm going to assign all your issues to you, but if ever there are some that you can't do, assign them back to me. Cheers.
          Hide
          Jérôme Mouneyrac added a comment -

          Hi Yang,
          here is my peer review, some minor points to change:

          • modifiedsince is a bit uncommon to me in Moodle code, from memory it is often called 'modified' or 'timemodified'. I would go for 'modified'.
          • the description ('modified since') should be more descriptive. This description gets displayed into the API documentation. Client developers will like to know that it is a timestamp for example.
          • also put this description for the new external function parameter in the phpdoc
          • modifiedsince doesn't need to be VALUE_OPTIONAL in the return value function as it's always returned.
          • I would have added a default value to the parameter:

            function ... (..., modifiedsince = null ) {
            

            instead of

             if (empty($fileinfo['modifiedsince'])) {
                        $modifiedsince = null;
                  }
            

            Once fixed you can submit to integration. Cheers.

          Show
          Jérôme Mouneyrac added a comment - Hi Yang, here is my peer review, some minor points to change: modifiedsince is a bit uncommon to me in Moodle code, from memory it is often called 'modified' or 'timemodified'. I would go for 'modified'. the description ('modified since') should be more descriptive. This description gets displayed into the API documentation. Client developers will like to know that it is a timestamp for example. also put this description for the new external function parameter in the phpdoc modifiedsince doesn't need to be VALUE_OPTIONAL in the return value function as it's always returned. I would have added a default value to the parameter: function ... (..., modifiedsince = null ) { instead of if (empty($fileinfo['modifiedsince'])) { $modifiedsince = null; } Once fixed you can submit to integration. Cheers.
          Hide
          Yang Yang added a comment -

          Hi Jerome

          Thanks for your reply. I have made changes according to your review.

          Regards,
          Yang

          Show
          Yang Yang added a comment - Hi Jerome Thanks for your reply. I have made changes according to your review. Regards, Yang
          Hide
          Yang Yang added a comment -

          Hi Jerome

          I could not find a way to submit the code for integration. Is it something to do with the rights of my account?

          Regards,
          Yang

          Show
          Yang Yang added a comment - Hi Jerome I could not find a way to submit the code for integration. Is it something to do with the rights of my account? Regards, Yang
          Hide
          Jérôme Mouneyrac added a comment -

          Hi Yang,
          a very minor note: I would not mention things like $timemodified >= $modified in the description. The client developer doesn't know about the code (plus it could be using something else than php). This description gets displayed in Admin > Plugins > Web services > API Documentation. If you write your description for people who don't have access to Moodle code and want to use an unknown language, then it should be spot on.

          One you change it I'll submit the issue to integration. Developers and submitting to integration are different group of permissions. I didn't know.

          Show
          Jérôme Mouneyrac added a comment - Hi Yang, a very minor note: I would not mention things like $timemodified >= $modified in the description. The client developer doesn't know about the code (plus it could be using something else than php). This description gets displayed in Admin > Plugins > Web services > API Documentation. If you write your description for people who don't have access to Moodle code and want to use an unknown language, then it should be spot on. One you change it I'll submit the issue to integration. Developers and submitting to integration are different group of permissions. I didn't know.
          Hide
          Yang Yang added a comment -

          Hi Jerome

          Thanks for your reply. I have modified the description.

          Regards,
          Yang

          Show
          Yang Yang added a comment - Hi Jerome Thanks for your reply. I have modified the description. Regards, Yang
          Hide
          Jérôme Mouneyrac added a comment -

          Thanks Yang, it looks perfect to me. Submitting for integration.

          Show
          Jérôme Mouneyrac added a comment - Thanks Yang, it looks perfect to me. Submitting for integration.
          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 Yang, this doesn't appear to be updated against the latest master and isn't applying cleanly at all

          Are you able to rebase on top if master?

          Show
          Dan Poltawski added a comment - Hi Yang, this doesn't appear to be updated against the latest master and isn't applying cleanly at all Are you able to rebase on top if master?
          Hide
          Dan Poltawski added a comment -

          Hi, since i've not heard anything about this i'm reopening it.

          Show
          Dan Poltawski added a comment - Hi, since i've not heard anything about this i'm reopening it.
          Hide
          Yang Yang added a comment -

          Hi Dan

          Sorry for the late reply. I did rebase and everything seems to be up-to-date. Could you please tell me which file is not merging?

          Thanks
          Yang

          Show
          Yang Yang added a comment - Hi Dan Sorry for the late reply. I did rebase and everything seems to be up-to-date. Could you please tell me which file is not merging? Thanks Yang
          Hide
          Dan Poltawski added a comment -

          Hi Yang,

          It seems that the whole of files/externallib.php is conflicting on integration - I think this is perhaps other changes Jerome has put in this week causing this.

          Jerome?

          Show
          Dan Poltawski added a comment - Hi Yang, It seems that the whole of files/externallib.php is conflicting on integration - I think this is perhaps other changes Jerome has put in this week causing this. Jerome?
          Hide
          Jérôme Mouneyrac added a comment -

          Hi,
          I had a look to Yang branch and I tried to rebase it but I could not success. If you cherry-pick there are too much conflicts dure to the lastest code integration. I suppose it's best if Yang you get the lastest Moodle and recommit your changes.
          Cheers,
          Jerome

          Show
          Jérôme Mouneyrac added a comment - Hi, I had a look to Yang branch and I tried to rebase it but I could not success. If you cherry-pick there are too much conflicts dure to the lastest code integration. I suppose it's best if Yang you get the lastest Moodle and recommit your changes. Cheers, Jerome
          Hide
          Yang Yang added a comment -

          Hi Jerome and Dan

          I have updated against the latest master and recommitted. Please let me know if it works fine this time.

          Cheers
          Yang

          Show
          Yang Yang added a comment - Hi Jerome and Dan I have updated against the latest master and recommitted. Please let me know if it works fine this time. Cheers Yang
          Hide
          Jérôme Mouneyrac added a comment -

          Thanks Yang, submitting to integration.

          Show
          Jérôme Mouneyrac added a comment - Thanks Yang, submitting to integration.
          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
          Yang Yang added a comment -

          Thanks. It has been updated.

          Show
          Yang Yang added a comment - Thanks. It has been updated.
          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
          Yang Yang added a comment -

          It has been updated.
          Thanks.

          Show
          Yang Yang added a comment - It has been updated. Thanks.
          Hide
          Aparup Banerjee added a comment -

          Hi Yang Yang,
          i've attached an xml file that lists some code style and whitespace/tabs/indentation issues with the patch. These need to be fixed up before it can be integrated.

          The patch is applying cleanly at the moment.

          Also could you please ensure you editor is setup according to conform to http://docs.moodle.org/dev/Coding_style

          The line terminations need to be LF only. There are CR currently which need to be fixed up too. please read http://docs.moodle.org/dev/Coding_style#Line_Termination

          Show
          Aparup Banerjee added a comment - Hi Yang Yang, i've attached an xml file that lists some code style and whitespace/tabs/indentation issues with the patch. These need to be fixed up before it can be integrated. The patch is applying cleanly at the moment. Also could you please ensure you editor is setup according to conform to http://docs.moodle.org/dev/Coding_style The line terminations need to be LF only. There are CR currently which need to be fixed up too. please read http://docs.moodle.org/dev/Coding_style#Line_Termination
          Hide
          Dan Poltawski 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
          Dan Poltawski 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
          Yang Yang added a comment -

          Hi Aparup

          I have fixed up the whitespace, tabs and indentation along with some other errors and warnings returned from the codechecker.

          Thanks
          Yang

          Show
          Yang Yang added a comment - Hi Aparup I have fixed up the whitespace, tabs and indentation along with some other errors and warnings returned from the codechecker. Thanks Yang
          Hide
          Aparup Banerjee added a comment -

          Hi Yang Yang,

          i'm just about to integrate this but i've one question.

          in trying to achieve "to check for files that have changed since" here, why are we returning files that are changed exactly at the specified time too?
          i'm thinking we could make it simpler to do this : "$modified if specified, will return only files changed after timestamp."

          Show
          Aparup Banerjee added a comment - Hi Yang Yang, i'm just about to integrate this but i've one question. in trying to achieve "to check for files that have changed since" here, why are we returning files that are changed exactly at the specified time too? i'm thinking we could make it simpler to do this : "$modified if specified, will return only files changed after timestamp."
          Hide
          Yang Yang added a comment -

          Hi Aparup

          Returning only files changed after specified timestamp is fine if that sounds more reasonable and makes it simpler.

          Regards,
          Yang

          Show
          Yang Yang added a comment - Hi Aparup Returning only files changed after specified timestamp is fine if that sounds more reasonable and makes it simpler. Regards, Yang
          Hide
          Aparup Banerjee added a comment -

          Hi Yang Yang,
          i've referenced this https://github.com/Lightwork-Marking/moodle/commit/101b1d9ce1adb007577ad6ac976a3f2307131f38 as the commit to integrate.
          i'm not seeing this at all in https://github.com/Lightwork-Marking/moodle/commit/83034614f902c2f1ad4616e29e86b8567a26658d (your diff branch) which looks like things have been messed up in there.
          so i'm sticking with 101b1d which looks good. attaching it here as a patch. (i'll modify it from there to return only files modified after timestamp)

          Show
          Aparup Banerjee added a comment - Hi Yang Yang, i've referenced this https://github.com/Lightwork-Marking/moodle/commit/101b1d9ce1adb007577ad6ac976a3f2307131f38 as the commit to integrate. i'm not seeing this at all in https://github.com/Lightwork-Marking/moodle/commit/83034614f902c2f1ad4616e29e86b8567a26658d (your diff branch) which looks like things have been messed up in there. so i'm sticking with 101b1d which looks good. attaching it here as a patch. (i'll modify it from there to return only files modified after timestamp)
          Hide
          Aparup Banerjee added a comment -

          https://github.com/nebgor/moodle/compare/mistress...MDL-31863 (getting Jerome to check before integrating this)

          Show
          Aparup Banerjee added a comment - https://github.com/nebgor/moodle/compare/mistress...MDL-31863 (getting Jerome to check before integrating this)
          Hide
          Jérôme Mouneyrac added a comment -

          Good to me Apu. Go with it.

          Show
          Jérôme Mouneyrac added a comment - Good to me Apu. Go with it.
          Hide
          Aparup Banerjee added a comment -

          Thanks, this has been integrated into master now. Good to test.

          Show
          Aparup Banerjee added a comment - Thanks, this has been integrated into master now. Good to test.
          Hide
          Ankit Agarwal added a comment -

          Hi guys,
          Thanks to Jerome for the the help with testing
          Am getting the following response

          <?xml version="1.0" encoding="UTF-8" ?>
          <EXCEPTION class="invalid_response_exception">
          <ERRORCODE>invalidresponse</ERRORCODE>
          <MESSAGE>Invalid response value detected</MESSAGE>
          <DEBUGINFO>files =&gt; Invalid response value detected: Only arrays accepted. The bad value is: ''</DEBUGINFO>
          </EXCEPTION>
          

          Have to fail this sorry.

          Show
          Ankit Agarwal added a comment - Hi guys, Thanks to Jerome for the the help with testing Am getting the following response <?xml version="1.0" encoding="UTF-8" ?> <EXCEPTION class="invalid_response_exception"> <ERRORCODE>invalidresponse</ERRORCODE> <MESSAGE>Invalid response value detected</MESSAGE> <DEBUGINFO>files =&gt; Invalid response value detected: Only arrays accepted. The bad value is: ''</DEBUGINFO> </EXCEPTION> Have to fail this sorry.
          Hide
          Dan Poltawski added a comment -

          Doesn't look like there is a fix arriving and we need to roll so I have reverted this.

          Show
          Dan Poltawski added a comment - Doesn't look like there is a fix arriving and we need to roll so I have reverted this.
          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
          Yang Yang added a comment - - edited

          Hi Ankit

          Could you please let me know what PARAMETERS were passed into the function for the testing. I could only replicate the exception by passing into an incorrect files name, itemid, filearea or file path. If this was the case, it seems like it is the current behavior in get_file_info().

          Thanks
          Yang

          Show
          Yang Yang added a comment - - edited Hi Ankit Could you please let me know what PARAMETERS were passed into the function for the testing. I could only replicate the exception by passing into an incorrect files name, itemid, filearea or file path. If this was the case, it seems like it is the current behavior in get_file_info(). Thanks Yang
          Hide
          Ankit Agarwal added a comment - - edited

          Hi Yang,
          When I and Jerome where testing this we used something like this

          $params = array('contextid' => 22,'component' => "user",'filearea' => "icon", 'itemid' => 0, 'filepath' => "/",'filename' => "f1.jpg");
          

          Respective values where taken from mdl_files table.

          Thanks

          Show
          Ankit Agarwal added a comment - - edited Hi Yang, When I and Jerome where testing this we used something like this $params = array('contextid' => 22,'component' => "user",'filearea' => "icon", 'itemid' => 0, 'filepath' => "/",'filename' => "f1.jpg"); Respective values where taken from mdl_files table. Thanks
          Hide
          Yang Yang added a comment - - edited

          Hi Ankit

          Thanks for the reply. Would the same result return on the existing web service core_files_get_files() without the latest changes? Since $timemodified was not specified, I think the exception would likely to be caused by the existing function get_file_info().

          Thanks
          Yang

          Show
          Yang Yang added a comment - - edited Hi Ankit Thanks for the reply. Would the same result return on the existing web service core_files_get_files() without the latest changes? Since $timemodified was not specified, I think the exception would likely to be caused by the existing function get_file_info(). Thanks Yang
          Hide
          Ankit Agarwal added a comment -

          Hey Yang,
          I am getting the same error on current integration. which doesn't contain your patch. Not sure what is the issue.

          Show
          Ankit Agarwal added a comment - Hey Yang, I am getting the same error on current integration. which doesn't contain your patch. Not sure what is the issue.
          Hide
          Yang Yang added a comment -

          Hi Ankit

          Thanks for the testing. The existing function get_files() has been integrated and used over several months. If the same testing result returned without the new patch, then this would be an existing behavior of the get_files() function. Therefore I would not think the response you received is a bug of the new patch.

          Regards
          Yang

          Show
          Yang Yang added a comment - Hi Ankit Thanks for the testing. The existing function get_files() has been integrated and used over several months. If the same testing result returned without the new patch, then this would be an existing behavior of the get_files() function. Therefore I would not think the response you received is a bug of the new patch. Regards Yang
          Hide
          Jérôme Mouneyrac added a comment -

          I confirm that Ankit showed me the reported error. As Yang say it could be not related to the patch however it would be good to find out what is the cause of the issue. Having a look at it...

          Show
          Jérôme Mouneyrac added a comment - I confirm that Ankit showed me the reported error. As Yang say it could be not related to the patch however it would be good to find out what is the cause of the issue. Having a look at it...
          Hide
          Jérôme Mouneyrac added a comment -

          I did a fix to resolve the issue when no file is returned by the function: https://github.com/mouneyrac/moodle/commits/MDL-31863

          Then I added an image to my private file and did this call:
          $params = array('contextid' => 5,"component" => "user","filearea" => "private",
          "itemid" => 0,"filepath" => "/","filename" => "image.jpg");

          It returns the modified time (+ two other information that I was not expecting. This function behavior needs to be look at, I'll write an issue about that.)

          You can integrate this issue and mark it as tested. Thanks.

          Show
          Jérôme Mouneyrac added a comment - I did a fix to resolve the issue when no file is returned by the function: https://github.com/mouneyrac/moodle/commits/MDL-31863 Then I added an image to my private file and did this call: $params = array('contextid' => 5,"component" => "user","filearea" => "private", "itemid" => 0,"filepath" => "/","filename" => "image.jpg"); It returns the modified time (+ two other information that I was not expecting. This function behavior needs to be look at, I'll write an issue about that.) You can integrate this issue and mark it as tested. Thanks.
          Hide
          Aparup Banerjee added a comment -

          seems Ankit is getting weird output. filename response contains a number like itemid.

          Show
          Aparup Banerjee added a comment - seems Ankit is getting weird output. filename response contains a number like itemid.
          Hide
          Aparup Banerjee added a comment -

          ok this really looks fine to me and Jerome too. integrating into master for further testing.

          seems to work for me too.

          Show
          Aparup Banerjee added a comment - ok this really looks fine to me and Jerome too. integrating into master for further testing. seems to work for me too.
          Hide
          Jérôme Mouneyrac added a comment -

          Already tested, pass.

          Show
          Jérôme Mouneyrac added a comment - Already tested, pass.
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Big thanks for the effort. This is now part of Moodle upstream. Let's wait for regressions, yay! LOL

          Ciao

          Show
          Eloy Lafuente (stronk7) added a comment - Big thanks for the effort. This is now part of Moodle upstream. Let's wait for regressions, yay! LOL Ciao

            People

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

              Dates

              • Created:
                Updated:
                Resolved: