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 from Repository:
        • Pull Master Branch:
        • Pull Master Diff URL:
        • 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

            caused a regression

            Bug - A problem which impairs or prevents Moodle from functioning correctly. MDL-44574 Course completion report table needs to be in a div

            • Major - Major loss of function, incorrect output
            • Open
            is a QA test written for

            Improvement - An enhancement to an existing Moodle feature. MDL-35556 Improve user completion data permission checking

            • Minor - Minor loss of function where workaround is possible
            • Closed
            will help resolve

            Improvement - An enhancement to an existing Moodle feature. MDL-35135 Course completion status export CSV should have a header row

            • Minor - Minor loss of function where workaround is possible
            • Closed

              Activity

              Ascending order - Click to sort in descending order
              Hide
              Permalink
              Matt Porritt added a comment -

              The updated plugin code

              Show
              Matt Porritt added a comment - The updated plugin code
              Hide
              Permalink
              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
              Permalink
              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
              Permalink
              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
              Permalink
              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
              Permalink
              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
              Permalink
              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
              Permalink
              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
              Permalink
              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
              Permalink
              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: