Moodle
  1. Moodle
  2. MDL-34274

Activity logs for book don't display page titles

    Details

    • Type: Improvement Improvement
    • Status: Closed
    • Priority: Minor Minor
    • Resolution: Fixed
    • Affects Version/s: 2.3.1
    • Fix Version/s: 2.4
    • Component/s: Book
    • Labels:
      None
    • Testing Instructions:
      Hide
      1. create a book resource and add a few chapters
      2. Run upgrade on your site if it is still not upgraded
        Test
      3. Print a chapter, check logs to make sure the action is "book print chapter" and the info field has chapter name
      4. Print a book, check logs to make sure the action is "book print" and the info field has chapter name
        Test
      5. Import the attached zip (book>import chapter)
      6. Goto log and make sure you see 2 entries, and two "book add chapter" all linking correctly chapters respectively
        Test
      7. Try editing a chapter and check logs for "book update chapter", with correct link
      8. Try adding a chapter and check logs for "book add chapter", with correct link
        Test
      9. goto course home page and click on the book as student
      10. check logs as admin for two entries "book view" linking to the book and "book view chapter" linking to the first visible chapter of the book
      11. as student goto some other chapter
      12. as admin check logs to make sure there is a new entry "book view chapter" linking to that chapter.
      13. Backup the course
      14. make sure you select "include course logs" during backup process
      15. Restore the course as a new course
      16. Make sure the new course has all the records related to book and they are pointing to correct entries.
      Show
      create a book resource and add a few chapters Run upgrade on your site if it is still not upgraded Test Print a chapter, check logs to make sure the action is "book print chapter" and the info field has chapter name Print a book, check logs to make sure the action is "book print" and the info field has chapter name Test Import the attached zip (book>import chapter) Goto log and make sure you see 2 entries, and two "book add chapter" all linking correctly chapters respectively Test Try editing a chapter and check logs for "book update chapter", with correct link Try adding a chapter and check logs for "book add chapter", with correct link Test goto course home page and click on the book as student check logs as admin for two entries "book view" linking to the book and "book view chapter" linking to the first visible chapter of the book as student goto some other chapter as admin check logs to make sure there is a new entry "book view chapter" linking to that chapter. Backup the course make sure you select "include course logs" during backup process Restore the course as a new course Make sure the new course has all the records related to book and they are pointing to correct entries.
    • Affected Branches:
      MOODLE_23_STABLE
    • Fixed Branches:
      MOODLE_24_STABLE
    • Pull Master Branch:
      MDL-34274-master
    • Rank:
      42628

      Description

      The activity logs for the book module only display the title of the book rather than the titles of the individual pages. If I click on the "book view" link in the logs, it reflects the different pages, but when scanning the logs for student activity, it's not easy to see which pages the user viewed.

      I've attached a screen capture of my activity logs. The book entries result from clicking through multiple pages of the book. I've also included the logs for the same content in a lesson where the page titles are included.

      A participant in our webinar said they weren't using the book because of the lack of tracking. This forum post mentions the issue as well: http://moodle.org/mod/forum/discuss.php?d=175060.

        Activity

        Hide
        Ankit Agarwal added a comment -

        It makes sense to add chapter information instead of book information.
        There are two approaches we can do this:-

        • Leave all current log entries as they are and introduce a new format "chapter view" instead of "view". This will keep all existing entries intact.
        • Parse all old log entries (url) and get chapterid from it, update all old log entries replacing book->id with chapter->id in the info field.

        Adding Eloy as watcher to get more feedback.

        Thanks

        Show
        Ankit Agarwal added a comment - It makes sense to add chapter information instead of book information. There are two approaches we can do this:- Leave all current log entries as they are and introduce a new format "chapter view" instead of "view". This will keep all existing entries intact. Parse all old log entries (url) and get chapterid from it, update all old log entries replacing book->id with chapter->id in the info field. Adding Eloy as watcher to get more feedback. Thanks
        Hide
        Eloy Lafuente (stronk7) added a comment -

        Yes, I think that adding "view chapter" (uses to be verb+noun generally) has a lot of sense and would enable also activity completions like:

        • The student has viewed XX% of the book chapters.

        I don't think it's critical to convert old logs at all. Just, from 2.4, the "view" actions will be the general one (when accessing the activity without chapter, from course page, for example) and the "view chapter" will be the new action logged each time one chapter is viewed.

        If anybody requests a log converter, surely it could be considered to be built apart (cli, somewhere). But I think it's not worth the effort of enforcing the conversion for all sites (can imagine paces with millions of entries to parse).

        Of course, all the places where logs are being used... within the module (completion, overview, whatever...) will need review to ensure that the change of actions doesn't affect any of the current behaviors.

        But yes, +1. Better logging will make it able of better/new possibilities.

        Ciao

        Show
        Eloy Lafuente (stronk7) added a comment - Yes, I think that adding "view chapter" (uses to be verb+noun generally) has a lot of sense and would enable also activity completions like: The student has viewed XX% of the book chapters. I don't think it's critical to convert old logs at all. Just, from 2.4, the "view" actions will be the general one (when accessing the activity without chapter, from course page, for example) and the "view chapter" will be the new action logged each time one chapter is viewed. If anybody requests a log converter, surely it could be considered to be built apart (cli, somewhere). But I think it's not worth the effort of enforcing the conversion for all sites (can imagine paces with millions of entries to parse). Of course, all the places where logs are being used... within the module (completion, overview, whatever...) will need review to ensure that the change of actions doesn't affect any of the current behaviors. But yes, +1. Better logging will make it able of better/new possibilities. Ciao
        Hide
        Ankit Agarwal added a comment -

        Hi Eloy,
        Thanks for the feedback.
        I have added also log for chapter updates and chapter printing. when a book is viewed from the course it will log it as "view" and when a specific chapter is viewed it will log it as "view chapter". Completion etc shouldnt be effected , but will do some more testing to make sure.
        Thanks

        Show
        Ankit Agarwal added a comment - Hi Eloy, Thanks for the feedback. I have added also log for chapter updates and chapter printing. when a book is viewed from the course it will log it as "view" and when a specific chapter is viewed it will log it as "view chapter". Completion etc shouldnt be effected , but will do some more testing to make sure. Thanks
        Hide
        Rajesh Taneja added a comment -

        Patch looks good Ankit,

        I am not sure about the logging scenarios here.

        1. In mod/book/view.php we add log for book and chapter view while accessing book (Chapter = 0)
        2. In mod/book/tool/print/index.php while printing book we only log for book printing, should we log for each chapter within.
        3. Similar case is in mod/book/tool/importhtml/locallib.php

        IMO, as we add log for course mod update, we should also add book update for chapter as well. If yes then you might want to update log in mod/book/edit.php as well.

        Feel free to push it for integration if you feel otherwise.

        Show
        Rajesh Taneja added a comment - Patch looks good Ankit, I am not sure about the logging scenarios here. In mod/book/view.php we add log for book and chapter view while accessing book (Chapter = 0) In mod/book/tool/print/index.php while printing book we only log for book printing, should we log for each chapter within. Similar case is in mod/book/tool/importhtml/locallib.php IMO, as we add log for course mod update, we should also add book update for chapter as well. If yes then you might want to update log in mod/book/edit.php as well. Feel free to push it for integration if you feel otherwise.
        Hide
        Ankit Agarwal added a comment -

        Hi Rajesh,
        Thanks for the review.

        1. As we discussed the reason for this being if we want to implement a % of chapter viewed later on, we need to record all chapter view. "Book view" can be removed entirely if decided, but personally I would leave that in.
        2. we have "print book" when someone is printing the whole book and "print chapter" when someone is printing a single chapter. IMHO, I am not sure if it is of any benefit to log print chapter for every chapter when print book is called.
        3. Similar with import
        4. For update I am not sure what is the correct way to do it. I get your point of using generic "book update" instead of "book update chapter" and "book add chapter". Hence I have added this change as a separate commit. I will leave it to the integrator to decide if this needs to pulled in or not.

        Will try to get feedback from Eloy on this.
        Thanks

        Show
        Ankit Agarwal added a comment - Hi Rajesh, Thanks for the review. As we discussed the reason for this being if we want to implement a % of chapter viewed later on, we need to record all chapter view. "Book view" can be removed entirely if decided, but personally I would leave that in. we have "print book" when someone is printing the whole book and "print chapter" when someone is printing a single chapter. IMHO, I am not sure if it is of any benefit to log print chapter for every chapter when print book is called. Similar with import For update I am not sure what is the correct way to do it. I get your point of using generic "book update" instead of "book update chapter" and "book add chapter". Hence I have added this change as a separate commit. I will leave it to the integrator to decide if this needs to pulled in or not. Will try to get feedback from Eloy on this. Thanks
        Hide
        Eloy Lafuente (stronk7) added a comment -

        I think the more granularity we put there, the best. So I think it's perfect to also have different "add/update book" and "add/update chapter". Sam applies to print, view... and practically everything.

        The only 2 details we need to observe for any new log action are:

        • They are defined in the corresponding db/log.php. And used consistently. I think this is ok.
        • Backup & restore supports them. That means that logs are sent to backup and that any dynamic part in the logs, must be converted/remapped on restore. It seems this is missing.

        So, I'm reopening this to implement backup/restore of new log actions... not complex at all if written logs are consistent.

        Ciao

        Show
        Eloy Lafuente (stronk7) added a comment - I think the more granularity we put there, the best. So I think it's perfect to also have different "add/update book" and "add/update chapter". Sam applies to print, view... and practically everything. The only 2 details we need to observe for any new log action are: They are defined in the corresponding db/log.php. And used consistently. I think this is ok. Backup & restore supports them. That means that logs are sent to backup and that any dynamic part in the logs, must be converted/remapped on restore. It seems this is missing. So, I'm reopening this to implement backup/restore of new log actions... not complex at all if written logs are consistent. Ciao
        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
        Ankit Agarwal added a comment -

        Hi Eloy,
        Thanks for your feedback.
        I have add the decode rules.
        Sending for another round of review.
        Thanks

        Show
        Ankit Agarwal added a comment - Hi Eloy, Thanks for your feedback. I have add the decode rules. Sending for another round of review. Thanks
        Hide
        Rajesh Taneja added a comment -

        Looks good Ankit,

        Pushing for integration.

        Show
        Rajesh Taneja added a comment - Looks good Ankit, Pushing for integration.
        Hide
        Aparup Banerjee 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
        Aparup Banerjee 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
        Sam Hemelryk added a comment -

        Thanks, has been integrated now

        Show
        Sam Hemelryk added a comment - Thanks, has been integrated now
        Hide
        David Monllaó added a comment -

        It works as expected, it passes.

        I've seen that logs generated by the admin user in the origin course are not restored in the destiny course

        Show
        David Monllaó added a comment - It works as expected, it passes. I've seen that logs generated by the admin user in the origin course are not restored in the destiny course
        Hide
        Dan Poltawski added a comment -

        Hurray!

        You did it, congratulations! You have on Mojito credit to redeem after the release of Moodle 2.4

        Show
        Dan Poltawski added a comment - Hurray! You did it, congratulations! You have on Mojito credit to redeem after the release of Moodle 2.4

          People

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

            Dates

            • Created:
              Updated:
              Resolved: