[MDL-35386] Update Course Completion report to include more useful information - Moodle Tracker

Details

Type: Improvement
Status: Closed
Priority: 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:
git://github.com/srynot4sale/moodle.git
Pull Master Branch:
~~MDL-35386~~b
Pull Master Diff URL:
https://github.com/srynot4sale/moodle/commit/e9d20bbc646e8d75c369f2fb2784746f54afb729

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

Attachments

Options
- Sort By Name
- Sort By Date
- Ascending
- Descending
- Thumbnails
- List
- Download All

Attachments

completion2.zip
21 kB
12/Sep/12 11:38 AM

Issue Links

caused a regression

MDL-44574 Course completion report table needs to be in a div

Open

is a QA test written for

MDL-35556 Improve user completion data permission checking

Closed

will help resolve

MDL-35135 Course completion status export CSV should have a header row

Closed

Activity

Ascending order - Click to sort in descending order

Hide

Permalink

Matt Porritt added a comment - 12/Sep/12 11:38 AM

The updated plugin code

Show

Matt Porritt added a comment - 12/Sep/12 11:38 AM The updated plugin code

Hide

Permalink

Sam Hemelryk added a comment - 03/Oct/12 8:48 AM

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

Show

Sam Hemelryk added a comment - 03/Oct/12 8:48 AM 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 - 04/Oct/12 9:09 AM

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 - 04/Oct/12 9:09 AM 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 - 16/Oct/12 7:45 PM

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 - 16/Oct/12 7:45 PM 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 - 19/Oct/12 6:31 AM

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 - 19/Oct/12 6:31 AM 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 - 19/Oct/12 9:50 AM

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 - 19/Oct/12 9:50 AM 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 - 30/Oct/12 4:15 PM

Thanks Aaron, i've integrated this now.

Show

Dan Poltawski added a comment - 30/Oct/12 4:15 PM Thanks Aaron, i've integrated this now.

Hide

Permalink

David Monllaó added a comment - 31/Oct/12 10:35 AM

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 - 31/Oct/12 10:35 AM 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 - 01/Nov/12 3:03 PM

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 - 01/Nov/12 3:03 PM 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 - 13/Mar/14 5:59 AM

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 - 13/Mar/14 5:59 AM 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

Assignee:

Aaron Barnes

Reporter:

Matt Porritt

Peer reviewer:

Sam Hemelryk

Integrator:

Dan Poltawski

Tester:

David Monllaó

Participants:

Aaron Barnes, Aparup Banerjee, Dan Poltawski, David Monllaó, Matt Porritt, Rebecca Trynes, Sam Hemelryk

Votes:

0 Vote for this issue

Watchers:

2 Start watching this issue

Dates

Created:

12/Sep/12 11:38 AM

Updated:

26/Mar/14 5:21 AM

Resolved:

01/Nov/12 3:03 PM

Fix Release Date:

3/Dec/12