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:

          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

            Gliffy Diagrams

              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: