Uploaded image for project: '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
    • Status: Closed
    • Priority: 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

          Attachments

            Activity

            Hide
            jerome 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
            jerome 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
            jerome 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
            jerome 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
            doraemonyoung Yang Yang added a comment -

            Hi Jerome

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

            Regards,
            Yang

            Show
            doraemonyoung Yang Yang added a comment - Hi Jerome Thanks for your reply. I have made changes according to your review. Regards, Yang
            Hide
            doraemonyoung 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
            doraemonyoung 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
            jerome 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
            jerome 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
            doraemonyoung Yang Yang added a comment -

            Hi Jerome

            Thanks for your reply. I have modified the description.

            Regards,
            Yang

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

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

            Show
            jerome Jérôme Mouneyrac added a comment - Thanks Yang, it looks perfect to me. Submitting for integration.
            Hide
            stronk7 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
            stronk7 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
            poltawski 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
            poltawski 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
            poltawski Dan Poltawski added a comment -

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

            Show
            poltawski Dan Poltawski added a comment - Hi, since i've not heard anything about this i'm reopening it.
            Hide
            doraemonyoung 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
            doraemonyoung 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
            poltawski 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
            poltawski 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
            jerome 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
            jerome 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
            doraemonyoung 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
            doraemonyoung 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
            jerome Jérôme Mouneyrac added a comment -

            Thanks Yang, submitting to integration.

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

            Thanks. It has been updated.

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

            It has been updated.
            Thanks.

            Show
            doraemonyoung Yang Yang added a comment - It has been updated. Thanks.
            Hide
            nebgor 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
            nebgor 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
            poltawski 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
            poltawski 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
            doraemonyoung 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
            doraemonyoung 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
            nebgor 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
            nebgor 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
            doraemonyoung 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
            doraemonyoung 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
            nebgor 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
            nebgor 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
            nebgor Aparup Banerjee added a comment -

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

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

            Good to me Apu. Go with it.

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

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

            Show
            nebgor Aparup Banerjee added a comment - Thanks, this has been integrated into master now. Good to test.
            Hide
            ankit_frenz 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_frenz 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
            poltawski 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
            poltawski 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 CiBoT added a comment -

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

            Show
            cibot CiBoT added a comment - Moving this reopened issue out from current integration. Please, re-submit it for integration once ready.
            Hide
            doraemonyoung 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
            doraemonyoung 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_frenz 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_frenz 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
            doraemonyoung 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
            doraemonyoung 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_frenz 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_frenz 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
            doraemonyoung 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
            doraemonyoung 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
            jerome 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
            jerome 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
            jerome 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
            jerome 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
            nebgor Aparup Banerjee added a comment -

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

            Show
            nebgor Aparup Banerjee added a comment - seems Ankit is getting weird output. filename response contains a number like itemid.
            Hide
            nebgor 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
            nebgor 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
            jerome Jérôme Mouneyrac added a comment -

            Already tested, pass.

            Show
            jerome Jérôme Mouneyrac added a comment - Already tested, pass.
            Hide
            stronk7 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
            stronk7 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
            Hide
            trushal trushal added a comment - - edited

            Hi Please help me in webservice call,

            If anyone of you help me how to get my private file with "core_files_get_files" i had tray lot to get this but i am getting errors
            with same i want to download at document how can i do that ,

            URL of websercie is : http://localhost/moodle/webservice/rest/server.php?wstoken=3afe18c8697dac9518d59bb8f10429b4&wsfunction=core_files_external&moodlewsrestformat=json&contextid=47&component=mod_mymodule&filearea=myarea&itemid=0&filename=

            Show
            trushal trushal added a comment - - edited Hi Please help me in webservice call, If anyone of you help me how to get my private file with "core_files_get_files" i had tray lot to get this but i am getting errors with same i want to download at document how can i do that , URL of websercie is : http://localhost/moodle/webservice/rest/server.php?wstoken=3afe18c8697dac9518d59bb8f10429b4&wsfunction=core_files_external&moodlewsrestformat=json&contextid=47&component=mod_mymodule&filearea=myarea&itemid=0&filename=
            Hide
            dmonllao David Monllaó added a comment -

            Hi trushal,

            If you have any questions about web services the best place to ask is the moodle.org forum (https://moodle.org/mod/forum/view.php?id=6971)

            wsfunction=core_files_external looks wrong to me, you should be probably using core_files_get_files. You can find more info in http://YOURSITE/admin/webservice/documentation.php

            Show
            dmonllao David Monllaó added a comment - Hi trushal, If you have any questions about web services the best place to ask is the moodle.org forum ( https://moodle.org/mod/forum/view.php?id=6971 ) wsfunction=core_files_external looks wrong to me, you should be probably using core_files_get_files. You can find more info in http://YOURSITE/admin/webservice/documentation.php

              People

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

                Dates

                • Created:
                  Updated:
                  Resolved:
                  Fix Release Date:
                  25/Jun/12