Moodle
  1. Moodle
  2. MDL-40420

Updating lesson_page doesn't set timemodified field

    Details

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

      Description

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

        Activity

        Hide
        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
        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
        Prateek Sachan added a comment -

        Hi Rossiani,

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

        Updated both the commits.

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

        It looks great Prateek.

        Thank you

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

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

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

        Pushing this to integration review for Prateek.

        Show
        Rossiani Wijaya added a comment - Pushing this to integration review for Prateek.
        Hide
        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 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
        Prateek Sachan added a comment -

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

        Show
        Prateek Sachan added a comment - I've updated the patch. Does it look good now?
        Hide
        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 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
        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
        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 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 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 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 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
        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
        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 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
        Aparup Banerjee added a comment -

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

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

        Thanks Prateek - this has been integrated now.

        Show
        Sam Hemelryk added a comment - Thanks Prateek - this has been integrated now.
        Hide
        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
        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 Wiese added a comment -

        Reopening re: comments from Jerome.

        Show
        Damyon Wiese added a comment - Reopening re: comments from Jerome.
        Hide
        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
        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 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 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 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 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
        Jérôme Mouneyrac added a comment -

        No worries Then all good.

        Show
        Jérôme Mouneyrac added a comment - No worries Then all good.
        Hide
        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
        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
        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
        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: