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

Updating lesson_page doesn't set timemodified field

    Details

    • Type: Bug
    • Status: Closed
    • Priority: Minor
    • Resolution: Fixed
    • Affects Version/s: 2.5, 2.6
    • Fix Version/s: 2.5.2
    • Component/s: Lesson
    • Labels:
    • Testing Instructions:
      Hide

      1. Add a new lesson_page.
      2. Fill in the details and save it.
      3. Edit the same page.
      4. Make some edits and save the page.
      5. Check the timemodified field of this page in mdl_lesson_pages database table.

      Expected Result: timemodified > timecreated
      Actual Result: timemodified = 0

      Show
      1. Add a new lesson_page. 2. Fill in the details and save it. 3. Edit the same page. 4. Make some edits and save the page. 5. Check the timemodified field of this page in mdl_lesson_pages database table. Expected Result: timemodified > timecreated Actual Result: timemodified = 0
    • Affected Branches:
      MOODLE_25_STABLE, MOODLE_26_STABLE
    • Fixed Branches:
      MOODLE_25_STABLE
    • Pull Master Branch:

      Description

      While adding or updating a lesson page, the timemodified field in mdl_lesson_pages table isn't set or updated.

        Gliffy Diagrams

          Attachments

            Activity

            Hide
            rwijaya Rossiani Wijaya added a comment -

            Hi Prateek,

            Thank you for providing the patch for this issue.

            The fixed for mod/lesson/editpage.php is incorrect. It will triggered the update for timemodified field each time the user click on edit page. The field should only be modified when user make any changes to the page.

            Instead of using $DB->set_field() it would be better to set the property for timemodified before calling the update().

            $data->timemodified = time() 

            Show
            rwijaya Rossiani Wijaya added a comment - Hi Prateek, Thank you for providing the patch for this issue. The fixed for mod/lesson/editpage.php is incorrect. It will triggered the update for timemodified field each time the user click on edit page. The field should only be modified when user make any changes to the page. Instead of using $DB->set_field() it would be better to set the property for timemodified before calling the update(). $data->timemodified = time()
            Hide
            xan Prateek Sachan added a comment -

            Hi Rossiani,

            Thanks for the quick feedback. Indeed, I didn't thought about that.

            Updated both the commits.

            Show
            xan Prateek Sachan added a comment - Hi Rossiani, Thanks for the quick feedback. Indeed, I didn't thought about that. Updated both the commits.
            Hide
            rwijaya Rossiani Wijaya added a comment -

            It looks great Prateek.

            Thank you

            Show
            rwijaya Rossiani Wijaya added a comment - It looks great Prateek. Thank you
            Hide
            xan Prateek Sachan added a comment -

            Thanks Rossiani for looking into it. Hope it gets fixed soon.

            Show
            xan Prateek Sachan added a comment - Thanks Rossiani for looking into it. Hope it gets fixed soon.
            Hide
            rwijaya Rossiani Wijaya added a comment -

            Pushing this to integration review for Prateek.

            Show
            rwijaya Rossiani Wijaya added a comment - Pushing this to integration review for Prateek.
            Hide
            marina Marina Glancy added a comment -

            Sorry guys, this is not exactly correct to tweak the form data.

            lesson_page::update() should take care of updating the timemodified as well as you did in lesson_page::create().

            Unfortunately not all classes extending lesson_page call parent::update() inside their update() function. So this code has to be duplicated as well call to file_postupdate_standard_editor() is duplicated inside each update() function.

            I'm leaving this issue open till tomorrow, please submit a patch and I'll integrate it.

            Show
            marina Marina Glancy added a comment - Sorry guys, this is not exactly correct to tweak the form data. lesson_page::update() should take care of updating the timemodified as well as you did in lesson_page::create(). Unfortunately not all classes extending lesson_page call parent::update() inside their update() function. So this code has to be duplicated as well call to file_postupdate_standard_editor() is duplicated inside each update() function. I'm leaving this issue open till tomorrow, please submit a patch and I'll integrate it.
            Hide
            xan Prateek Sachan added a comment -

            I've updated the patch. Does it look good now?

            Show
            xan Prateek Sachan added a comment - I've updated the patch. Does it look good now?
            Hide
            marina Marina Glancy added a comment -

            Prateek, there are 3 or 4 classes that extend lesson_page, they overwrite method update() but do not call parent::update(), they must be modified as well

            Show
            marina Marina Glancy added a comment - Prateek, there are 3 or 4 classes that extend lesson_page, they overwrite method update() but do not call parent::update(), they must be modified as well
            Hide
            xan Prateek Sachan added a comment -

            I now understood what I was missing. I completely forgot that the lesson module has different types of pages. Sorry for any inconveniences.

            I'll update the patch asap. (I really need to fix this bug for my GSoC project).

            Show
            xan Prateek Sachan added a comment - I now understood what I was missing. I completely forgot that the lesson module has different types of pages. Sorry for any inconveniences. I'll update the patch asap. (I really need to fix this bug for my GSoC project).
            Hide
            marina Marina Glancy added a comment -

            sure, waiting.

            Just to note that by itself this issue does not make any difference. This field is not used/queried anywhere. It does not affect recent activity block/report either. So I'm really not sure what you are trying to achieve here and how can the bug that you reported yourself can affect your GSoC evaluation.

            Please note that I will only integrate it in master because it's not fixing any real bug.

            Show
            marina Marina Glancy added a comment - sure, waiting. Just to note that by itself this issue does not make any difference. This field is not used/queried anywhere. It does not affect recent activity block/report either. So I'm really not sure what you are trying to achieve here and how can the bug that you reported yourself can affect your GSoC evaluation. Please note that I will only integrate it in master because it's not fixing any real bug.
            Hide
            marina Marina Glancy added a comment -

            Prateek, unfortunately 10am Perth time is the deadline for the integration. Please resubmit this issue when it's ready.
            If you really need this in 2.5 please explain why and the integrator who picks it next week will decide on it.
            Thanks

            Show
            marina Marina Glancy added a comment - Prateek, unfortunately 10am Perth time is the deadline for the integration. Please resubmit this issue when it's ready. If you really need this in 2.5 please explain why and the integrator who picks it next week will decide on it. Thanks
            Hide
            xan Prateek Sachan added a comment - - edited

            Actually, timemodified (timestamp of last modification) is the basis on which I will be indexing content for searching and I've planned to include lesson module into it as it is a very important module in Moodle. You can view this code structure to know about it.

            I guess it is really a bug here (though partially). On addition of new lesson_pages, timemodified needn't be set. But on updating those lesson_pages, timestamp of modification should be tracked as happens throughout Moodle.
            Also, my GSoC project is build on Moodle_25.

            (I've edited the issue to set it only for modified timestamp).

            I've updated the patch.

            Show
            xan Prateek Sachan added a comment - - edited Actually, timemodified (timestamp of last modification) is the basis on which I will be indexing content for searching and I've planned to include lesson module into it as it is a very important module in Moodle. You can view this code structure to know about it. I guess it is really a bug here (though partially). On addition of new lesson_pages, timemodified needn't be set. But on updating those lesson_pages, timestamp of modification should be tracked as happens throughout Moodle. Also, my GSoC project is build on Moodle_25. (I've edited the issue to set it only for modified timestamp). I've updated the patch.
            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
            nebgor Aparup Banerjee added a comment -

            Looks like Prateek has this done on the sub page types now, sending for integration.

            Show
            nebgor Aparup Banerjee added a comment - Looks like Prateek has this done on the sub page types now, sending for integration.
            Hide
            samhemelryk Sam Hemelryk added a comment -

            Thanks Prateek - this has been integrated now.

            Show
            samhemelryk Sam Hemelryk added a comment - Thanks Prateek - this has been integrated now.
            Hide
            jerome Jérôme Mouneyrac added a comment - - edited

            All passed. However I noticed the bug is there in 2.4. Why is it not backported?

            Show
            jerome Jérôme Mouneyrac added a comment - - edited All passed. However I noticed the bug is there in 2.4. Why is it not backported?
            Hide
            damyon Damyon Wiese added a comment -

            Reopening re: comments from Jerome.

            Show
            damyon Damyon Wiese added a comment - Reopening re: comments from Jerome.
            Hide
            jerome Jérôme Mouneyrac added a comment -

            Thanks Damyon. I think we should back-port the issue as it exists in previous version too. I asked Damyon to reopen the issue.

            Show
            jerome Jérôme Mouneyrac added a comment - Thanks Damyon. I think we should back-port the issue as it exists in previous version too. I asked Damyon to reopen the issue.
            Hide
            marina Marina Glancy added a comment -

            It's not really a bug. This field is not accessed by anybody. Prateek just needs it for his global search GSoC project. I would not reopen and backport.

            Show
            marina Marina Glancy added a comment - It's not really a bug. This field is not accessed by anybody. Prateek just needs it for his global search GSoC project. I would not reopen and backport.
            Hide
            damyon Damyon Wiese added a comment -

            I agree with Marina - strictly it should not have even gone in 2.5 but I guess we were lenient because the change was so minor.

            Show
            damyon Damyon Wiese added a comment - I agree with Marina - strictly it should not have even gone in 2.5 but I guess we were lenient because the change was so minor.
            Hide
            jerome Jérôme Mouneyrac added a comment -

            No worries Then all good.

            Show
            jerome Jérôme Mouneyrac added a comment - No worries Then all good.
            Hide
            xan Prateek Sachan added a comment - - edited

            Thanks everyone.

            I also agree with Marina. She informed me that it would only go to master. But, due to my GSoC project, I had to make it go into 2.5 as well.

            Show
            xan Prateek Sachan added a comment - - edited Thanks everyone. I also agree with Marina. She informed me that it would only go to master. But, due to my GSoC project, I had to make it go into 2.5 as well.
            Hide
            samhemelryk Sam Hemelryk added a comment -

            Huzzah, your code made it into Moodle. Perhaps now things are ever so slightly better!

            "The ship can't take this much pressure. Sometimes it falls apart just sitting in the hangar."
            ~ Professor Farnsworth

            Show
            samhemelryk Sam Hemelryk added a comment - Huzzah, your code made it into Moodle. Perhaps now things are ever so slightly better! "The ship can't take this much pressure. Sometimes it falls apart just sitting in the hangar." ~ Professor Farnsworth

              People

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

                Dates

                • Created:
                  Updated:
                  Resolved:
                  Fix Release Date:
                  9/Sep/13