Create a course
Enable and configure course completion
Add the course completion block
Access the course as an Admin, Teacher, Student, and Guest and ensure the Completion block isn't broken
Test multiple themes to ensure the block appears the same

Michael de Raadt
added a comment - 25/Sep/12 1:28 PM Aaron: I've added you as a watcher on this issue as you are Course completion component lead. It would be good if you could peer review this when the time comes.

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.

Dan Poltawski
added a comment - 18/Jan/13 7:24 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

Sending this back this week just to tidy up/consider a few things.
The list is a mix of things to do and things to consider.

Whitespace, there's a couple of lines with empty whitespace after your patch.

html_writer has the link method, its a much much better fit than using html_writer::tag('a'....

strtolower(get_string(X)) isn't multibyte safe. That should really be textlib::strtolower(get_string()). Those wern't your doing, they were already there but now would be a great time to tidy them up.

In the case of tables it would be awesome if we could make use of html_table and html_writer::table. It is designed to take care of much of this rendering work for us and handles things like column and row classes easily.

Just out of curiousity have you thought about converting to a renderer? (mentioned in the description) While not essential that would be a really great step forward. The more we convert to renderers the happier those looking to modify/theme Moodle will be as it allows them to tweak etc as they want.

Anyway, sorry about the delay on this issue, I think these things are worth looking at.

Sam Hemelryk
added a comment - 22/Jan/13 9:53 PM Hi Jason,
Sending this back this week just to tidy up/consider a few things.
The list is a mix of things to do and things to consider.
Whitespace, there's a couple of lines with empty whitespace after your patch.
html_writer has the link method, its a much much better fit than using html_writer::tag('a'....
strtolower(get_string(X)) isn't multibyte safe. That should really be textlib::strtolower(get_string()). Those wern't your doing, they were already there but now would be a great time to tidy them up.
In the case of tables it would be awesome if we could make use of html_table and html_writer::table. It is designed to take care of much of this rendering work for us and handles things like column and row classes easily.
Just out of curiousity have you thought about converting to a renderer? (mentioned in the description) While not essential that would be a really great step forward. The more we convert to renderers the happier those looking to modify/theme Moodle will be as it allows them to tweak etc as they want.
Anyway, sorry about the delay on this issue, I think these things are worth looking at.
Many thanks
Sam

I should add that the changes you've made a good improvement anyway.
The things I commented on are more ideas than requirements, if time isn't going to permit one/any of them don't sweat about it put this up and we can proceed anyway.

Sam Hemelryk
added a comment - 29/Jan/13 1:55 AM Cool thanks dude.
I should add that the changes you've made a good improvement anyway.
The things I commented on are more ideas than requirements, if time isn't going to permit one/any of them don't sweat about it put this up and we can proceed anyway.
Many thanks
Sam

Jason Fowler
added a comment - 29/Jan/13 5:38 AM I considered converting it to a renderer, but figured I would try this first, and see how it went, and start with something smaller for tackling a renderer conversion.

I've cleared out the whitespace, and have fixed the link vs tag thing. I've also sorted out the strtolower.

I'm looking in to the html_table and html_writer::table - these both look like they complicate the creation and rendering of a table. I'll continue looking in to them, hoping there is something I am missing that would make using them easier.

Jason Fowler
added a comment - 18/Feb/13 5:48 AM I've cleared out the whitespace, and have fixed the link vs tag thing. I've also sorted out the strtolower.
I'm looking in to the html_table and html_writer::table - these both look like they complicate the creation and rendering of a table. I'll continue looking in to them, hoping there is something I am missing that would make using them easier.

I don't think the block lends itself to the use of the html_table class at all, the rows of the table are populated out of order, which makes putting them in to an array next to impossible. Will continue trying to understand the way this works, in the hopes of making sense of it all.

Jason Fowler
added a comment - 21/Feb/13 6:31 AM I don't think the block lends itself to the use of the html_table class at all, the rows of the table are populated out of order, which makes putting them in to an array next to impossible. Will continue trying to understand the way this works, in the hopes of making sense of it all.

Andrew Davis
added a comment - 04/Apr/13 2:55 AM Its pretty minor but maybe add phpdocs for applicable_formats().
Consider giving rows, srows and prows more descriptive names or at least a comment at their definition saying what they are. Right now its not clear what those three variables are for.
$row->cells [1] ->style = 'text-align: right;'; (line 143, line 156 and line 227)
I know this was in the old version as well but is it worth putting this style info in a css file?
In blocks/completionstatus/db/upgrade.php you need a space either side of =>. Perhaps pull the array out onto its own line as shown at http://docs.moodle.org/dev/Coding_style#Wrapping_Arrays
Same sort of whitespace issue in details.php
echo html_writer::start_tag('table', array('class'=>'generalbox boxaligncenter'));
Once you've looked deep within yourself for the answers to these questions you are go for integration at your leisure.

re: taking the inline style and putting it into a stylesheet file
I am trying to keep this code as true to the output of the original code as possible.
This was just a project I started to learn the html writer better, but I do intend to later take this same block and convert it to a renderer, at which point I will convert that inline style to a class selector with an associated stylesheet entry.

Jason Fowler
added a comment - 09/Apr/13 1:38 AM re: taking the inline style and putting it into a stylesheet file
I am trying to keep this code as true to the output of the original code as possible.
This was just a project I started to learn the html writer better, but I do intend to later take this same block and convert it to a renderer, at which point I will convert that inline style to a class selector with an associated stylesheet entry.

Normally I would complain that you have changed all the comments (to add punctuation). Please don't do this in future because it leads to conflicts and makes it harder to use tools like blame.

We do like fixed comments - but you should create a new issue for it and clean the comments for an entire component/subsystem at once. That way it is clear what is conflicting etc.

My reasoning for allowing it here - is that this does fix all the comments for the component and the general issue is related to code cleanup. I amended the description to include the comments in the scope of the issue.

Damyon Wiese
added a comment - 09/Apr/13 5:36 AM A comment on the comments
Normally I would complain that you have changed all the comments (to add punctuation). Please don't do this in future because it leads to conflicts and makes it harder to use tools like blame.
We do like fixed comments - but you should create a new issue for it and clean the comments for an entire component/subsystem at once. That way it is clear what is conflicting etc.
My reasoning for allowing it here - is that this does fix all the comments for the component and the general issue is related to code cleanup. I amended the description to include the comments in the scope of the issue.

Damyon Wiese
added a comment - 09/Apr/13 6:03 AM - edited Thanks Jason this is a good improvement,
Integrated to master.
Note: I also changed the 2 remaining html_writer::tag('a' ... calls to ::link(.
Outstanding things that were spotted in this issue and still need to be addressed:
Change to use a renderer.
Remove inline CSS.
Fix calls to strtolower.
I'll add new issues for these just so we don't lose them (Actually I wont create an issue for the renderer - that applies almost everywhere and the css can be fixed then as well).

Mark Nelson
added a comment - 15/Apr/13 9:31 AM Yes, I had to make changes to the theme/upgrade.txt file and noticed a comment regarding 2.6 referencing a closed ticket! I have fixed this in the MDL-39047