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: Course
    • Labels:
    • Testing Instructions:
      Hide

      As teacher

      1. set course layout setting to "show one section per page"
      2. access a course (topic/week format) and select couples of section (eg: http://moodle/course/view.php?id=xx&section=3)
      3. check the log report for the course and make sure there's "course view section" entry

      As admin

      1. backup the course and tick on "include course logs"
      2. restore the course as new course
      3. check the log for the new course and make sure there's "course view section" entry.
      Show
      As teacher set course layout setting to "show one section per page" access a course (topic/week format) and select couples of section (eg: http://moodle/course/view.php?id=xx&section=3 ) check the log report for the course and make sure there's "course view section" entry As admin backup the course and tick on "include course logs" restore the course as new course check the log for the new course and make sure there's "course view section" entry.
    • Affected Branches:
      MOODLE_23_STABLE
    • Fixed Branches:
      MOODLE_23_STABLE
    • Pull from Repository:
    • Pull Master Branch:

      Description

      We need to do something about course view entries, perhaps we will need to add a new one for a section page view

      //TODO: danp do we need different urls?
      add_to_log($course->id, 'course', 'view', "view.php?id=$course->id", "$course->id");

        Gliffy Diagrams

          Issue Links

            Activity

            Hide
            Rossiani Wijaya added a comment -

            Adding Eloy to watcher list.

            Hi Eloy,

            When you have a chance could you take a look the patch and provide some feedback?

            Course view section is now restored properly, however the information column still displaying the courseID instead of name.

            Will keep working and do more test on this.

            Thanks
            Rosie.

            Show
            Rossiani Wijaya added a comment - Adding Eloy to watcher list. Hi Eloy, When you have a chance could you take a look the patch and provide some feedback? Course view section is now restored properly, however the information column still displaying the courseID instead of name. Will keep working and do more test on this. Thanks Rosie.
            Hide
            Eloy Lafuente (stronk7) added a comment - - edited

            Hi Rossie,

            in the info field you can put 2 different things:

            A) One literal, so it will be shown en the log reports as introduced.
            B) One id, so it will be matched, with the rules defined @ lib/db/log.php (mdl_log_display), to find the proper description to be shown.

            The main advantage of using literals - option A - is that they remain constant along the time and don't consume any DB query. In the other side, if you change, for example, the course title and you're using literals, the log report will continue showing the old name.

            And the main advantage of using ids + log_display is that they are dynamic, so one change in the source will be displayed immediately in the log reports, although they are slower, consuming one extra DB query to do so.

            So, back to this issue, I think that you need to:

            1) Both for "view" and "view section" actions, use approach B), the dynamic.
            2) Create one new variable, $infoid that, in the case of the "view" action will contain the course->id and in the case of the "view" section will contain the section->id (note it's not the section number, but the section id.
            3) Pass always that $infoid variable to the add_to_log() call.
            4) Edit lib/db/log.php and add one entry for the 'view section' action. It will point to the "course_sections" table and to the "name" field. Note that there is already there the "view" action, defined to use course->id and return course->fullname (in fact your current patch is breaking that, because on it you've switched to approach B) (harcoded string).
            5) Bump version, upgrade and generate some log entries.
            6) Verify that both the course name and the section name are shown as expected.

            Hope this explains the 2 logging alternatives and how to implement the B) one for this case.

            About the backup & restore code, it looks good. If it works, then perfect!

            Ciao

            Show
            Eloy Lafuente (stronk7) added a comment - - edited Hi Rossie, in the info field you can put 2 different things: A) One literal, so it will be shown en the log reports as introduced. B) One id, so it will be matched, with the rules defined @ lib/db/log.php (mdl_log_display), to find the proper description to be shown. The main advantage of using literals - option A - is that they remain constant along the time and don't consume any DB query. In the other side, if you change, for example, the course title and you're using literals, the log report will continue showing the old name. And the main advantage of using ids + log_display is that they are dynamic, so one change in the source will be displayed immediately in the log reports, although they are slower, consuming one extra DB query to do so. So, back to this issue, I think that you need to: 1) Both for "view" and "view section" actions, use approach B), the dynamic. 2) Create one new variable, $infoid that, in the case of the "view" action will contain the course->id and in the case of the "view" section will contain the section->id (note it's not the section number, but the section id. 3) Pass always that $infoid variable to the add_to_log() call. 4) Edit lib/db/log.php and add one entry for the 'view section' action. It will point to the "course_sections" table and to the "name" field. Note that there is already there the "view" action, defined to use course->id and return course->fullname (in fact your current patch is breaking that, because on it you've switched to approach B) (harcoded string). 5) Bump version, upgrade and generate some log entries. 6) Verify that both the course name and the section name are shown as expected. Hope this explains the 2 logging alternatives and how to implement the B) one for this case. About the backup & restore code, it looks good. If it works, then perfect! Ciao
            Hide
            Eloy Lafuente (stronk7) added a comment -

            Trick: In case you want to show the section number in case the section name is empty... you can use SQL framents like:

            field'=>'COALESCE(name, section)';
            

            Anything valid in a SELECT clause can be used there, just imagine that Moodle is going to create the query:

            SELECT $field
            FROM $mtable
            WHERE id = $infoid
            

            Ciao

            Show
            Eloy Lafuente (stronk7) added a comment - Trick: In case you want to show the section number in case the section name is empty... you can use SQL framents like: field'=>'COALESCE(name, section)'; Anything valid in a SELECT clause can be used there, just imagine that Moodle is going to create the query: SELECT $field FROM $mtable WHERE id = $infoid Ciao
            Hide
            Rossiani Wijaya added a comment -

            Thanks Eloy for the feedback.

            I updated the patch according to your suggestion.

            Please take a look and let me know if it is correct.

            Thanks

            Show
            Rossiani Wijaya added a comment - Thanks Eloy for the feedback. I updated the patch according to your suggestion. Please take a look and let me know if it is correct. Thanks
            Hide
            Eloy Lafuente (stronk7) added a comment -

            Looks perfect, IMO +1

            with one tiny detail... I think that enclosing "$infoid" with quotes is completely useless. We have dozens of calls without it.

            Ciao

            Show
            Eloy Lafuente (stronk7) added a comment - Looks perfect, IMO +1 with one tiny detail... I think that enclosing "$infoid" with quotes is completely useless. We have dozens of calls without it. Ciao
            Hide
            Rossiani Wijaya added a comment -

            Thanks Eloy for reviewing.

            Updated the patch to remove quote for $infoid.

            Submitting for integration review.

            Show
            Rossiani Wijaya added a comment - Thanks Eloy for reviewing. Updated the patch to remove quote for $infoid. Submitting for integration review.
            Hide
            Sam Hemelryk added a comment -

            Thanks Rossi, this has been integrated now.
            Would you mind adding test instructions ASAP please.

            Cheers
            Sam

            Show
            Sam Hemelryk added a comment - Thanks Rossi, this has been integrated now. Would you mind adding test instructions ASAP please. Cheers Sam
            Hide
            Ankit Agarwal added a comment -

            I am getting the following error:-

            Table "course_section" does not exist

            Table name should be course_sections.

            Failing this.Sorry.
            Thanks

            Show
            Ankit Agarwal added a comment - I am getting the following error:- Table "course_section" does not exist Table name should be course_sections. Failing this.Sorry. Thanks
            Hide
            Rossiani Wijaya added a comment -

            Thanks Ankit for testing this.

            Patch has been updated.

            Please reset the test. Thanks integrator

            Show
            Rossiani Wijaya added a comment - Thanks Ankit for testing this. Patch has been updated. Please reset the test. Thanks integrator
            Hide
            Dan Poltawski added a comment -

            Ooops, reset this - but its sams thing. Leaving it at this point.

            Show
            Dan Poltawski added a comment - Ooops, reset this - but its sams thing. Leaving it at this point.
            Hide
            Sam Hemelryk added a comment -

            Thanks guys, I've cherry-picked the commit to fix and this is ready for testing again.

            Show
            Sam Hemelryk added a comment - Thanks guys, I've cherry-picked the commit to fix and this is ready for testing again.
            Hide
            Ankit Agarwal added a comment -

            leaving it open for Rosi to make some more changes

            Show
            Ankit Agarwal added a comment - leaving it open for Rosi to make some more changes
            Hide
            Rossiani Wijaya added a comment -

            Thanks Ankit. I'm working with Eloy right now to figure out the issue.

            Show
            Rossiani Wijaya added a comment - Thanks Ankit. I'm working with Eloy right now to figure out the issue.
            Hide
            Eloy Lafuente (stronk7) added a comment -

            Aha, I think I found the logical explanation for the problem.

            1) In the example backup file you sent me, that "view section" log record corresponds to userid = 2. I bet it's the admin user.

            2) The user with id = 2 is not included in the backup, because there is no real course data (forum posts...) with such user being the participant.

            3) On restore of logs, see both restore_course_logs_structure_step and restore_activity_logs_structure_step, we verify that the user is a restored one before proceeding to decode the log entries. As far as such user does not exist in the restore, the record is skipped.

            So I think the operation is performing ok. Just try one of this (or all):

            a) as admin, make some forum post in the course. That will cause such user to be considered a participant in the course and start being included in backup. So then, his/her related logs will be restored.

            b) in the backup file, change the "userid" of that log entry from "2" to "3", that is one user already included in the backup.

            c) logged as one teacher or student, go navigate to the section XX so some log entry is generated for that user and the "view section" action.

            And then, try to restore. As far as the user in the log record is now a "processed" one, his/her log action will be restored ok, including the "view section" action.

            Mystery solved, I hope! Ciao

            PS: Don't forget to change the restore rule specifications, because the 4th param needs to be 'course_section' in order to get the info column matched and the log entry restored properly.

            Show
            Eloy Lafuente (stronk7) added a comment - Aha, I think I found the logical explanation for the problem. 1) In the example backup file you sent me, that "view section" log record corresponds to userid = 2. I bet it's the admin user. 2) The user with id = 2 is not included in the backup, because there is no real course data (forum posts...) with such user being the participant. 3) On restore of logs, see both restore_course_logs_structure_step and restore_activity_logs_structure_step, we verify that the user is a restored one before proceeding to decode the log entries. As far as such user does not exist in the restore, the record is skipped. So I think the operation is performing ok. Just try one of this (or all): a) as admin, make some forum post in the course. That will cause such user to be considered a participant in the course and start being included in backup. So then, his/her related logs will be restored. b) in the backup file, change the "userid" of that log entry from "2" to "3", that is one user already included in the backup. c) logged as one teacher or student, go navigate to the section XX so some log entry is generated for that user and the "view section" action. And then, try to restore. As far as the user in the log record is now a "processed" one, his/her log action will be restored ok, including the "view section" action. Mystery solved, I hope! Ciao PS: Don't forget to change the restore rule specifications, because the 4th param needs to be 'course_section' in order to get the info column matched and the log entry restored properly.
            Hide
            Rossiani Wijaya added a comment -

            Thanks Eloy for your feedback.

            I updated the patch according to your suggestion.

            Will ask integrator to push it for integration.

            Show
            Rossiani Wijaya added a comment - Thanks Eloy for your feedback. I updated the patch according to your suggestion. Will ask integrator to push it for integration.
            Hide
            Rossiani Wijaya added a comment -

            Updating test instruction

            Show
            Rossiani Wijaya added a comment - Updating test instruction
            Hide
            Aparup Banerjee added a comment -

            sending back to integration review stage

            Show
            Aparup Banerjee added a comment - sending back to integration review stage
            Hide
            Aparup Banerjee added a comment -

            and its up for Sam now

            Show
            Aparup Banerjee added a comment - and its up for Sam now
            Hide
            Sam Hemelryk added a comment -

            Thanks Rossi, I've cherry-picked the last commit again. Its up for testing once more

            Show
            Sam Hemelryk added a comment - Thanks Rossi, I've cherry-picked the last commit again. Its up for testing once more
            Hide
            Aparup Banerjee added a comment -

            Ankit is testing this atm

            Show
            Aparup Banerjee added a comment - Ankit is testing this atm
            Hide
            Ankit Agarwal added a comment -

            it is working now!!!


            Passing.
            Thanks

            Show
            Ankit Agarwal added a comment - it is working now!!! Passing. Thanks
            Hide
            Dan Poltawski added a comment -

            Congratulations!

            Your work has made into the latest Moodle release!

            You are only authorised to celebrate after testing 15 Moodle 2.3 QA tests, thanks!

            Show
            Dan Poltawski added a comment - Congratulations! Your work has made into the latest Moodle release! You are only authorised to celebrate after testing 15 Moodle 2.3 QA tests, thanks!

              People

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

                Dates

                • Created:
                  Updated:
                  Resolved: