Details

    • Testing Instructions:
      Hide
      1. Enrol some users into a course
      2. Enable course completion in the course
      3. Go to Course administration > Reports > Course completion
      4. VERIFY: that the table html code contains thead and tbody.
      5. VERIFY: that you can export in the two csv formats and that the format is correct
      6. Go to Course administration > Reports > Activity completion
      7. VERIFY: that the table html code contains thead and tbody.
      8. VERIFY: that you can export in the two csv formats and that the format is correct
      Show
      Enrol some users into a course Enable course completion in the course Go to Course administration > Reports > Course completion VERIFY: that the table html code contains thead and tbody. VERIFY: that you can export in the two csv formats and that the format is correct Go to Course administration > Reports > Activity completion VERIFY: that the table html code contains thead and tbody. VERIFY: that you can export in the two csv formats and that the format is correct
    • Affected Branches:
      MOODLE_24_STABLE, MOODLE_25_STABLE
    • Fixed Branches:
      MOODLE_25_STABLE
    • Pull Master Branch:
      MDL-37988-master
    • Rank:
      47765

      Description

      Bootstrap styles tables on the basis that the first tr row of th tags is inside a thead tag. This is also general good practice.

      Updating an old bug since Bootstrap is going into core and that makes this much more obvious.

      Specific examples:

      1. If you search for courses in course/search.php in edit mode it provides you with a table of results with associated actions.

      2. Course -> Reports -> activity completion table

        Issue Links

          Activity

          Hide
          David Scotson added a comment -

          This seems to more widespread actually, the same problem can be found:

          http://192.168.56.10/moodle25/course/category.php?id=1 in the list of courses in view mode,

          and a couple of others that I've lost track of, but will note here when I find them again.

          Show
          David Scotson added a comment - This seems to more widespread actually, the same problem can be found: http://192.168.56.10/moodle25/course/category.php?id=1 in the list of courses in view mode, and a couple of others that I've lost track of, but will note here when I find them again.
          Hide
          Paul Holden added a comment -

          Same as MDL-34293? Also, your URL points at an internal address

          Show
          Paul Holden added a comment - Same as MDL-34293 ? Also, your URL points at an internal address
          Hide
          Rajesh Taneja added a comment -

          Thanks for spotting this issue David.

          I've put that on the backlog.

          In the meantime feel free to help us work on this issue. If you are able to provide a patch or links to your Git repository branch, please add a patch label so we will spot it.

          Show
          Rajesh Taneja added a comment - Thanks for spotting this issue David. I've put that on the backlog. In the meantime feel free to help us work on this issue. If you are able to provide a patch or links to your Git repository branch, please add a patch label so we will spot it.
          Hide
          Martin Dougiamas added a comment -

          Jerome can you see if you can fix these obvious ones?

          Everything generated by our table class seems OK already and so are forum discussions.

          Show
          Martin Dougiamas added a comment - Jerome can you see if you can fix these obvious ones? Everything generated by our table class seems OK already and so are forum discussions.
          Hide
          Jérôme Mouneyrac added a comment -

          I don't understand the issue, can someone explain it to me.

          What I see:
          course/category.php and course/search.php don't have any html tables. So I don't understand what I should fix there, or maybe it has been fixed?
          Course -> Reports -> activity completion table: it works as in the description and it says it's expected. I think the specific examples are example of how it should work.

          I'm totally missing what I need to fix here, can you explain to a big newbie like me. It seems like an issue David wrote for himself for later, I need more info to help

          Show
          Jérôme Mouneyrac added a comment - I don't understand the issue, can someone explain it to me. What I see: course/category.php and course/search.php don't have any html tables. So I don't understand what I should fix there, or maybe it has been fixed? Course -> Reports -> activity completion table: it works as in the description and it says it's expected. I think the specific examples are example of how it should work. I'm totally missing what I need to fix here, can you explain to a big newbie like me. It seems like an issue David wrote for himself for later, I need more info to help
          Hide
          Rajesh Taneja added a comment -

          Hello Jerome,

          If you go to Course ► Reports ► Activity completion ► Reports ► Activity completion
          Check table html using firebug. It will show you <table><tbody><tr><th>...</th></tr></tbody></table>

          David's point is first table row should be in <thead> and not in <tbody> tag.

          Show
          Rajesh Taneja added a comment - Hello Jerome, If you go to Course ► Reports ► Activity completion ► Reports ► Activity completion Check table html using firebug. It will show you <table><tbody><tr><th>...</th></tr></tbody></table> David's point is first table row should be in <thead> and not in <tbody> tag.
          Hide
          Jérôme Mouneyrac added a comment -

          Ah I was looking to the wrong report Martin and Raj showed me.

          Show
          Jérôme Mouneyrac added a comment - Ah I was looking to the wrong report Martin and Raj showed me.
          Hide
          Jérôme Mouneyrac added a comment -

          Thanks Raj.

          Show
          Jérôme Mouneyrac added a comment - Thanks Raj.
          Hide
          Jérôme Mouneyrac added a comment -

          It has been fixed in course/search apparently, just fixed activity completion report. Sending for peer-review.

          Show
          Jérôme Mouneyrac added a comment - It has been fixed in course/search apparently, just fixed activity completion report. Sending for peer-review.
          Hide
          Rajesh Taneja added a comment -

          Thanks Jerome,

          Patch looks good, although the issue is to fix it in all places. Seems other reports are doing same thing (report/courseoverview/index.php).

          In addition to this, it will be nice to wrap rest of the columns in <tbody> tag.

          FYI: some browsers add <tbody> tag if it's not present, but we should not rely on browser.

          NOTE: There are few use-case of table in repository (repository/filepicker.php, repository/wikimedia/lib.php), which we probably don't have to consider.

          Show
          Rajesh Taneja added a comment - Thanks Jerome, Patch looks good, although the issue is to fix it in all places. Seems other reports are doing same thing (report/courseoverview/index.php). In addition to this, it will be nice to wrap rest of the columns in <tbody> tag. FYI: some browsers add <tbody> tag if it's not present, but we should not rely on browser. NOTE: There are few use-case of table in repository (repository/filepicker.php, repository/wikimedia/lib.php), which we probably don't have to consider.
          Hide
          Jérôme Mouneyrac added a comment -

          Fixing what Raj mentioned. In the purpose to be reasonable, I'm not going to fix it in all the Moodle code, there are too many tables to check. It's up to developers to fix them when they find a missing thead/tbody.

          This decision is taken because I consider that the issue problem is quite trivial. For my understanding, the missing thead tag causes the display to not be as good as planned by the theme. And the missing tbody, I knew about it too, is automatically added by all common browser I tested (Chrome, Safari, Firefox).

          Show
          Jérôme Mouneyrac added a comment - Fixing what Raj mentioned. In the purpose to be reasonable, I'm not going to fix it in all the Moodle code, there are too many tables to check. It's up to developers to fix them when they find a missing thead/tbody. This decision is taken because I consider that the issue problem is quite trivial. For my understanding, the missing thead tag causes the display to not be as good as planned by the theme. And the missing tbody, I knew about it too, is automatically added by all common browser I tested (Chrome, Safari, Firefox).
          Hide
          Martin Dougiamas added a comment -

          Yes let's not delay this issue with more (probably endless) additions, just get these ones to integration.

          Show
          Martin Dougiamas added a comment - Yes let's not delay this issue with more (probably endless) additions, just get these ones to integration.
          Hide
          Jérôme Mouneyrac added a comment -

          course overview table doesn't have any header so sending to integration, thanks.

          Show
          Jérôme Mouneyrac added a comment - course overview table doesn't have any header so sending to integration, thanks.
          Hide
          Damyon Wiese added a comment -

          Thanks Jerome,

          This patch introduces <tbody></tbody> into the header of the csv download formats - can you add an if statement to check for this?

          Show
          Damyon Wiese added a comment - Thanks Jerome, This patch introduces <tbody></tbody> into the header of the csv download formats - can you add an if statement to check for this?
          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
          Jérôme Mouneyrac added a comment -

          Fixed tbody in csv and made same fix for activity completion report.

          Show
          Jérôme Mouneyrac added a comment - Fixed tbody in csv and made same fix for activity completion report.
          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
          Jérôme Mouneyrac added a comment -

          Hi Mary,
          I saw you changed the issue from minor to blocker. I'd like to understand what I don't know which make this issue a blocker ( I'm not being sarcastic). Thanks

          Show
          Jérôme Mouneyrac added a comment - Hi Mary, I saw you changed the issue from minor to blocker. I'd like to understand what I don't know which make this issue a blocker ( I'm not being sarcastic). Thanks
          Hide
          Dan Poltawski added a comment -

          The testing instructions were not really enough, but I wrote them myself and integrated and tested this.

          Show
          Dan Poltawski added a comment - The testing instructions were not really enough, but I wrote them myself and integrated and tested this.
          Hide
          Jérôme Mouneyrac added a comment -

          Thanks Dan. I read your instructions. Thanks for updating them.

          Show
          Jérôme Mouneyrac added a comment - Thanks Dan. I read your instructions. Thanks for updating them.
          Hide
          Dan Poltawski added a comment -

          Thanks! You're changes are now spread to the world through this git and our source control repositories.

          No time to rest though, we've got days to make 2.5 the best yet!

          ciao

          Show
          Dan Poltawski added a comment - Thanks! You're changes are now spread to the world through this git and our source control repositories. No time to rest though, we've got days to make 2.5 the best yet! ciao

            People

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

              Dates

              • Created:
                Updated:
                Resolved: