Moodle
  1. Moodle
  2. MDL-35386

Update Course Completion report to include more useful information

    Details

    • Type: Improvement Improvement
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: 2.2.4, 2.3.2
    • Fix Version/s: 2.4
    • Component/s: Course completion
    • Labels:
      None
    • Testing Instructions:
      Hide

      Enable course completion site-wide in advanced settings.
      Create a new course with completion enabled.
      Enrol multiple users in the course.
      Apply some criteria to the course view the Completion Tracking course settings page.
      Complete some of the criteria for various users (easiest way to do this is to use the Self Completion criteria and course block).
      View the completion course report.

      Expected:
      Report should look the same as before.
      When hovering over ticks it should show the time the user completed that criteria.
      Export links in footer should work, and exported reports should include times for each criteria completed. Exported reports should also contain a header row that matches the user data.

      Show
      Enable course completion site-wide in advanced settings. Create a new course with completion enabled. Enrol multiple users in the course. Apply some criteria to the course view the Completion Tracking course settings page. Complete some of the criteria for various users (easiest way to do this is to use the Self Completion criteria and course block). View the completion course report. Expected: Report should look the same as before. When hovering over ticks it should show the time the user completed that criteria. Export links in footer should work, and exported reports should include times for each criteria completed. Exported reports should also contain a header row that matches the user data.
    • Affected Branches:
      MOODLE_22_STABLE, MOODLE_23_STABLE
    • Fixed Branches:
      MOODLE_24_STABLE
    • Pull Master Branch:
    • Rank:
      44062

      Description

      I have taken Matt's code and added to it to do the following:

      • include the course completed date, in both the on screen report and the csv export.
      • show criteria completion dates in csv
      • make all completion dates the same formats
      • use moodle_url and csv_reader
      • add header rows to the csv!
      • fix activity criteria display logic
      • changed csv file name to be correct

        Issue Links

          Activity

          Hide
          Matt Porritt added a comment -

          The updated plugin code

          Show
          Matt Porritt added a comment - The updated plugin code
          Hide
          Sam Hemelryk added a comment -

          Hi Aaron,

          Things look really good thanks.
          Just a couple of trivial notes, up to you if you clean them up before putting it up for integration. They're not show stoppers but are perhaps worth cleaning up.

          1. There are a couple of places were you've ended up with two empty lines separating code. We normally avoid this, simply to keep the code uniform and make it easies to read.
          2. There are a couple of ternary operators being used. In cases where it is very simple (lines 205, and 227 as exmaples) that is tolerated, however as soon as they start getting complex (line 649) we prefer them to be converted into proper if..else statements.

          Otherwise looks spot on thanks.

          Cheers
          Sam

          Show
          Sam Hemelryk added a comment - Hi Aaron, Things look really good thanks. Just a couple of trivial notes, up to you if you clean them up before putting it up for integration. They're not show stoppers but are perhaps worth cleaning up. There are a couple of places were you've ended up with two empty lines separating code. We normally avoid this, simply to keep the code uniform and make it easies to read. There are a couple of ternary operators being used. In cases where it is very simple (lines 205, and 227 as exmaples) that is tolerated, however as soon as they start getting complex (line 649) we prefer them to be converted into proper if..else statements. Otherwise looks spot on thanks. Cheers Sam
          Hide
          Matt Porritt added a comment -

          Hi Aaron,
          The other thing I have noticed in testing is the current code that handles paging doesn't do large number of users (2000+ in my testing environment) well. It is showing 86 pages before the "next" link appears, in my example.
          Is it worth changing it to use the paging_bar output renderer function, as used in the partipant view (/user/index.php)?
          I'm happy to make the changes if you like.

          Cheers,
          Matt P

          Show
          Matt Porritt added a comment - Hi Aaron, The other thing I have noticed in testing is the current code that handles paging doesn't do large number of users (2000+ in my testing environment) well. It is showing 86 pages before the "next" link appears, in my example. Is it worth changing it to use the paging_bar output renderer function, as used in the partipant view (/user/index.php)? I'm happy to make the changes if you like. Cheers, Matt P
          Hide
          Aaron Barnes added a comment -

          Hi guys,

          I've fixed your issues Sam.

          I had a look at the paging bar Matt talks about however using the same code as user/index.php would require implementing the report in flexible_table() to do things properly - which is just too big a job for me to look at right now.

          You could just implement the paging part (as opposed to the initials paging part) and avoid flexible_table() I guess. Feels kinda like cheating though

          Cheers,
          Aaron

          Show
          Aaron Barnes added a comment - Hi guys, I've fixed your issues Sam. I had a look at the paging bar Matt talks about however using the same code as user/index.php would require implementing the report in flexible_table() to do things properly - which is just too big a job for me to look at right now. You could just implement the paging part (as opposed to the initials paging part) and avoid flexible_table() I guess. Feels kinda like cheating though Cheers, Aaron
          Hide
          Matt Porritt added a comment -

          Hi Aaron,
          I cheated on my install with the paging bar...
          I'm doing some work with course completions for my companies Moodle, if I end up changing to using the flexible_table() I'll supply the update.
          Otherwise I'm happy

          Matt P

          Show
          Matt Porritt added a comment - Hi Aaron, I cheated on my install with the paging bar... I'm doing some work with course completions for my companies Moodle, if I end up changing to using the flexible_table() I'll supply the update. Otherwise I'm happy Matt P
          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
          Dan Poltawski added a comment -

          Thanks Aaron, i've integrated this now.

          Show
          Dan Poltawski added a comment - Thanks Aaron, i've integrated this now.
          Hide
          David Monllaó added a comment -

          Tested in master, it passes. The time is displayed and exported in the .csv files, headers as well

          FYI, I've seen that in Linux + Chrome the download .csv links appears centered, but the <li> dots appears in the left side.

          Show
          David Monllaó added a comment - Tested in master, it passes. The time is displayed and exported in the .csv files, headers as well FYI, I've seen that in Linux + Chrome the download .csv links appears centered, but the <li> dots appears in the left side.
          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
          Hide
          Rebecca Trynes added a comment -

          Can anybody provide a reason why the wrapper div before the table was removed on line 331? It has caused some awful overflow off the page when there are a lot of activities included in the report. Having that div means that overflow:auto provides a scrollbar instead - preferable to the page going too wide and messing with my theme.

          Show
          Rebecca Trynes added a comment - Can anybody provide a reason why the wrapper div before the table was removed on line 331? It has caused some awful overflow off the page when there are a lot of activities included in the report. Having that div means that overflow:auto provides a scrollbar instead - preferable to the page going too wide and messing with my theme.

            People

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

              Dates

              • Created:
                Updated:
                Resolved: