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

      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");
      

        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: