Moodle
  1. Moodle
  2. MDL-35608

Rewrite Course Completion Block output to be more moodle friendly

    Details

    • Type: Improvement Improvement
    • Status: Closed
    • Priority: Minor Minor
    • Resolution: Fixed
    • Affects Version/s: 2.3.2
    • Fix Version/s: 2.5
    • Component/s: Blocks, Course completion
    • Labels:
    • Testing Instructions:
      Hide
      1. Create a course
      2. Enable and configure course completion
      3. Add the course completion block
      4. Access the course as an Admin, Teacher, Student, and Guest and ensure the Completion block isn't broken
      5. Test multiple themes to ensure the block appears the same
      Show
      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
    • Affected Branches:
      MOODLE_23_STABLE
    • Fixed Branches:
      MOODLE_25_STABLE
    • Pull from Repository:
    • Pull Master Branch:
      wip-MDL-35608-master
    • Rank:
      44334

      Description

      At present the Course Completion Block writes HTML directly for output, rather than using the HTML Writer or Output Renderer.

      Also make all the inline comments in the block comply with the coding standards.

        Issue Links

          Activity

          Hide
          Michael de Raadt added a comment -

          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.

          Show
          Michael de Raadt added a comment - 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.
          Hide
          Jason Fowler added a comment -

          This issue isn't patched, I had just created an empty branch to remind myself that this only needs to be in master

          Show
          Jason Fowler added a comment - This issue isn't patched, I had just created an empty branch to remind myself that this only needs to be in master
          Hide
          Aaron Barnes added a comment -

          Hi Jason,

          What exactly are you planning on doing?

          Cheers,
          Aaron

          Show
          Aaron Barnes added a comment - Hi Jason, What exactly are you planning on doing? Cheers, Aaron
          Hide
          Jason Fowler added a comment -

          Seeing as the block is mostly tables, I am converting the handling of the output to html writer calls, but keeping the output itself exactly the same.

          Show
          Jason Fowler added a comment - Seeing as the block is mostly tables, I am converting the handling of the output to html writer calls, but keeping the output itself exactly the same.
          Hide
          Aaron Barnes added a comment -

          Hi Jason,

          That sounds awesome! Thanks heaps!

          Cheers for keeping me informed.

          Aaron

          Show
          Aaron Barnes added a comment - Hi Jason, That sounds awesome! Thanks heaps! Cheers for keeping me informed. Aaron
          Hide
          Jason Fowler added a comment -

          Aaron, can you please peer review this?

          Show
          Jason Fowler added a comment - Aaron, can you please peer review this?
          Hide
          Aaron Barnes added a comment -

          [Y] Syntax
          [Y] Output
          [Y] Whitespace
          [Y] Language
          [-] Databases
          [Y] Testing
          [-] Security
          [-] Documentation
          [Y] Git
          [Y] Sanity check

          I'm not sure about the use of dashes in the commit message, but otherwise looks great. Sorry it took so long to get to this!

          Show
          Aaron Barnes added a comment - [Y] Syntax [Y] Output [Y] Whitespace [Y] Language [-] Databases [Y] Testing [-] Security [-] Documentation [Y] Git [Y] Sanity check I'm not sure about the use of dashes in the commit message, but otherwise looks great. Sorry it took so long to get to this!
          Hide
          Jason Fowler added a comment -

          rebased

          Show
          Jason Fowler added a comment - rebased
          Hide
          Jason Fowler added a comment -

          Thanks Aaron, pushing for integration now.

          Show
          Jason Fowler added a comment - Thanks Aaron, pushing for integration now.
          Hide
          Dan Poltawski 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
          Dan Poltawski 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 -

          Your branch appears to be missing:
          fatal: Couldn't find remote ref wip-MDL-35608-master

          Show
          Dan Poltawski added a comment - Your branch appears to be missing: fatal: Couldn't find remote ref wip- MDL-35608 -master
          Hide
          Jason Fowler added a comment -

          re-pushed it now.

          Show
          Jason Fowler added a comment - re-pushed it now.
          Hide
          Sam Hemelryk added a comment -

          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.

          1. Whitespace, there's a couple of lines with empty whitespace after your patch.
          2. html_writer has the link method, its a much much better fit than using html_writer::tag('a'....
          3. 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.
          4. 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.
          5. 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

          Show
          Sam Hemelryk added a comment - 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
          Hide
          CiBoT added a comment -

          Moving this reopened issue out from current integration. Please, re-submit it for integration once ready.

          Show
          CiBoT added a comment - Moving this reopened issue out from current integration. Please, re-submit it for integration once ready.
          Hide
          Jason Fowler added a comment -

          Thanks for taking the time to point this out Sam, will get to work on fixing it all up ASAP.

          Show
          Jason Fowler added a comment - Thanks for taking the time to point this out Sam, will get to work on fixing it all up ASAP.
          Hide
          Sam Hemelryk added a comment -

          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

          Show
          Sam Hemelryk added a comment - 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
          Hide
          Jason Fowler added a comment -

          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.

          Show
          Jason Fowler added a comment - 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.
          Hide
          Jason Fowler added a comment -

          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.

          Show
          Jason Fowler added a comment - 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.
          Hide
          Jason Fowler added a comment -

          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.

          Show
          Jason Fowler added a comment - 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.
          Hide
          Jason Fowler added a comment -

          array merging ... that'll solve the issue I have been having ...

          Show
          Jason Fowler added a comment - array merging ... that'll solve the issue I have been having ...
          Hide
          Jason Fowler added a comment -

          It would seem the html_table class adds in a whole boat load of classes to everything. Crap I neither need nor want...

          Show
          Jason Fowler added a comment - It would seem the html_table class adds in a whole boat load of classes to everything. Crap I neither need nor want...
          Hide
          Jason Fowler added a comment -

          Aaron, are you able to peer review this, or should I make it available for someone else to look at?

          Show
          Jason Fowler added a comment - Aaron, are you able to peer review this, or should I make it available for someone else to look at?
          Hide
          Andrew Davis added a comment -

          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.

          Show
          Andrew Davis added a comment - 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.
          Hide
          Jason Fowler added a comment -

          Thanks Andrew.

          Show
          Jason Fowler added a comment - Thanks Andrew.
          Hide
          Jason Fowler added a comment -

          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.

          Show
          Jason Fowler added a comment - 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.
          Hide
          Jason Fowler added a comment -

          Re: phpdocs - there are no phpdocs in this file, and it's not really something that needs to be documented for API access.

          Show
          Jason Fowler added a comment - Re: phpdocs - there are no phpdocs in this file, and it's not really something that needs to be documented for API access.
          Hide
          Damyon Wiese added a comment -

          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.

          Show
          Damyon Wiese added a comment - 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.
          Hide
          Damyon Wiese added a comment - - 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:

          1. Change to use a renderer.
          2. Remove inline CSS.
          3. 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).

          Show
          Damyon Wiese added a comment - - 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).
          Hide
          Jason Fowler added a comment -

          sorry, totally missed those strtolower calls. Can you please assign the new issue to me, when you create it, and I will make sure it gets done.

          Show
          Jason Fowler added a comment - sorry, totally missed those strtolower calls. Can you please assign the new issue to me, when you create it, and I will make sure it gets done.
          Hide
          David Monllaó added a comment -

          It passes, tested with standard, afterburner, boxxie, fusion and bootstrap theme. It looks nice in all of them

          Show
          David Monllaó added a comment - It passes, tested with standard, afterburner, boxxie, fusion and bootstrap theme. It looks nice in all of them
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Your awesome contributions are now part of Moodle, your fav LMS out there.

          Closing this as fixed.

          Many thanks for all the hard work, ciao

          Show
          Eloy Lafuente (stronk7) added a comment - Your awesome contributions are now part of Moodle, your fav LMS out there. Closing this as fixed. Many thanks for all the hard work, ciao
          Hide
          Dan Poltawski added a comment -

          Note this issue put an upgrade.txt message in about 2.6...

          Show
          Dan Poltawski added a comment - Note this issue put an upgrade.txt message in about 2.6...
          Hide
          Mark Nelson added a comment -

          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

          Show
          Mark Nelson added a comment - 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
          Hide
          Jason Fowler added a comment -

          I was under the impression that it wouldn't make it in to 2.5, so I made the change to 2.6, then it got integrated without anyone noticing...

          Show
          Jason Fowler added a comment - I was under the impression that it wouldn't make it in to 2.5, so I made the change to 2.6, then it got integrated without anyone noticing...

            People

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

              Dates

              • Created:
                Updated:
                Resolved: