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

      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

        Gliffy Diagrams

          Issue Links

            Activity

            Hide
            bawjaws 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
            bawjaws 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
            pholden Paul Holden added a comment -

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

            Show
            pholden Paul Holden added a comment - Same as MDL-34293 ? Also, your URL points at an internal address
            Hide
            rajeshtaneja 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
            rajeshtaneja 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
            dougiamas 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
            dougiamas 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
            jerome 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
            jerome 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
            rajeshtaneja 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
            rajeshtaneja 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
            jerome Jérôme Mouneyrac added a comment -

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

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

            Thanks Raj.

            Show
            jerome Jérôme Mouneyrac added a comment - Thanks Raj.
            Hide
            jerome 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
            jerome 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
            rajeshtaneja 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
            rajeshtaneja 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
            jerome 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
            jerome 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
            dougiamas Martin Dougiamas added a comment -

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

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

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

            Show
            jerome Jérôme Mouneyrac added a comment - course overview table doesn't have any header so sending to integration, thanks.
            Hide
            damyon 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 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 CiBoT added a comment -

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

            Show
            cibot CiBoT added a comment - Moving this reopened issue out from current integration. Please, re-submit it for integration once ready.
            Hide
            jerome Jérôme Mouneyrac added a comment -

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

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

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

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

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

            Show
            jerome Jérôme Mouneyrac added a comment - Thanks Dan. I read your instructions. Thanks for updating them.
            Hide
            poltawski 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
            poltawski 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:
                  Fix Release Date:
                  14/May/13