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:
    • Rank:
      38506

      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

      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: