Uploaded image for project: 'Moodle'
  1. Moodle
  2. MDL-27001

Option to display activity description on course page

    Details

    • Type: Improvement
    • Status: Closed
    • Priority: Minor
    • Resolution: Fixed
    • Affects Version/s: 2.0.2
    • Fix Version/s: 2.2
    • Component/s: Resource
    • Labels:
    • Testing Instructions:
      Hide

      Testing (easy):
      1) Apply upgrade. Check there is no error during upgrade.
      2) View an existing course containing a number of activities; check there is no error.
      3) Edit an activity on existing course. Save and return to course. Check there is no error.
      4) Add (or use existing activities) of each type supported by this change:
      assignment, chat, choice, data, folder, forum, glossary, imscp, page, quiz, resource, scorm, survey, url, wiki, workshop.
      5) Check that, with default settings, the descriptions do not display on course page.
      6) Go through each activity. Edit it, tick the 'Description on course page' checkbox, save and return to course. Check the description now appears.
      7) In the case of the resource and url activities, try choosing one of the options that cause a popup window to appear and make sure this still works.

      Additional test (images) (easy)
      1) Create a new Page resource. In the intro, add an image.
      2) Turn on the 'description on course page' option.
      3) Click 'Save and return to course'
      4) Check that the image displays on course page

      Show
      Testing (easy): 1) Apply upgrade. Check there is no error during upgrade. 2) View an existing course containing a number of activities; check there is no error. 3) Edit an activity on existing course. Save and return to course. Check there is no error. 4) Add (or use existing activities) of each type supported by this change: assignment, chat, choice, data, folder, forum, glossary, imscp, page, quiz, resource, scorm, survey, url, wiki, workshop. 5) Check that, with default settings, the descriptions do not display on course page. 6) Go through each activity. Edit it, tick the 'Description on course page' checkbox, save and return to course. Check the description now appears. 7) In the case of the resource and url activities, try choosing one of the options that cause a popup window to appear and make sure this still works. Additional test (images) (easy) 1) Create a new Page resource. In the intro, add an image. 2) Turn on the 'description on course page' option. 3) Click 'Save and return to course' 4) Check that the image displays on course page
    • Affected Branches:
      MOODLE_20_STABLE
    • Fixed Branches:
      MOODLE_22_STABLE
    • Pull Master Branch:
      MDL-27001-master

      Description

      The file resource has option for whether to show description (on its page).

      I would like to add a second option below that one, 'Show description on course page' (default off). If set, the description would appear on the course page below the link to the file, like:

      • Spreadsheet file
        This spreadsheet contains useful data that you should erase.

      (Where the first line is the existing display & the second line is from description.)

      This is only needed for file resources because these are the only ones where typically, you don't actually go to the page, i.e. in most cases they are set up so that once you click it immediately starts downloading the file.

      There are two current workarounds for the lack of this feature:

      1) Set display type to 'Embed' instead of 'Automatic' / 'Force download; this forces users to go via the intervening page when they click, this includes the description.

      2) Add a Label below the file with the description.

      I am probably willing to implement this feature (for Moodle 2.1 probably? Or 2.0.x), it should be easy - just want to check if there is support for it.

      so far, david said +1

        Gliffy Diagrams

        1. coursepage.html
          201 kB
          Sam Marshall
        1. coursepage.png
          58 kB
        2. Editing Forum.png
          43 kB

          Issue Links

            Activity

            Hide
            tsala Helen Foster added a comment -

            Sam, thanks for your suggested improvement. As I mentioned in the dev chat, I find it a bit weird to have a setting for the file resource only, though I can appreciate your reasoning.

            For anyone reading this, please vote for this issue if you'd like it implemented.

            Show
            tsala Helen Foster added a comment - Sam, thanks for your suggested improvement. As I mentioned in the dev chat, I find it a bit weird to have a setting for the file resource only, though I can appreciate your reasoning. For anyone reading this, please vote for this issue if you'd like it implemented.
            Hide
            quen Sam Marshall added a comment -

            Making the option available for all activity types that include an intro (via a setting in course_modules & standard_whatever_it_is controls) is possible instead, though it might be a bit of a larger change.

            Show
            quen Sam Marshall added a comment - Making the option available for all activity types that include an intro (via a setting in course_modules & standard_whatever_it_is controls) is possible instead, though it might be a bit of a larger change.
            Hide
            quen Sam Marshall added a comment -

            I've been looking at some other things and I now also think it would be more appropriate to make this available for all types; in particular there are cases where it would be useful for the url module as well (as that one, similarly, does not normally take you to a page where it displays).

            Show
            quen Sam Marshall added a comment - I've been looking at some other things and I now also think it would be more appropriate to make this available for all types; in particular there are cases where it would be useful for the url module as well (as that one, similarly, does not normally take you to a page where it displays).
            Hide
            drex Mark Drechsler added a comment -

            Funnily enough I'm in a meeting where this was just raised as a suggestion - and then I find a tracker item added just a couple of weeks ago

            I'd also suggest that there is an option to define whether the label is shown above or below the resource/activity. Sound fair?

            Show
            drex Mark Drechsler added a comment - Funnily enough I'm in a meeting where this was just raised as a suggestion - and then I find a tracker item added just a couple of weeks ago I'd also suggest that there is an option to define whether the label is shown above or below the resource/activity. Sound fair?
            Hide
            quen Sam Marshall added a comment -

            Hi Mark, thanks for participating.

            Interesting requirement - you mean you want it to show like:

            This is some blah blah description 
            blah blah about something the user
            hasn't got to yet and hasn't even
            seen the title for.
             
            __Activity title__

            rather than

            __Activity title__
             
            Here is a description underneath the
            title for the activity, like normal.

            Sorry for the slightly leading wording in examples but that seems like a strange requirement. I'm wondering, would the latter one still benefit you at all?

            Moodle display system doesn't currently support the order you want, so that would be a larger task.

            (It already supports 'link with icon, then some text', so in other words, if I just fiddle about with what the modules do, the code to display it like that is already there. It doesn't currently have a way to display the other order. Not impossible to add it but more work.)

            Show
            quen Sam Marshall added a comment - Hi Mark, thanks for participating. Interesting requirement - you mean you want it to show like: This is some blah blah description blah blah about something the user hasn't got to yet and hasn't even seen the title for.   __Activity title__ rather than __Activity title__   Here is a description underneath the title for the activity, like normal. Sorry for the slightly leading wording in examples but that seems like a strange requirement. I'm wondering, would the latter one still benefit you at all? Moodle display system doesn't currently support the order you want, so that would be a larger task. (It already supports 'link with icon, then some text', so in other words, if I just fiddle about with what the modules do, the code to display it like that is already there. It doesn't currently have a way to display the other order. Not impossible to add it but more work.)
            Hide
            drex Mark Drechsler added a comment -

            Hey Sam,

            No problem.

            If I was going to give more valid wording then I'd be saying something along the lines of:

            "In the following resource you will see an example of something blah blah. Check it out first before you do the next blahblah.

            _Activity Title_"

            Maybe its just the way I design courses, but I tend to go

            CONTEXT

            then

            CONTENT

            and rarely the other way around (which is the way you had it already).

            For the users I'm currently dealing with, only having the option to have the context (label) underneath would be as useful as a chocolate teapot.

            Hope this makes sense,

            Mark.

            Show
            drex Mark Drechsler added a comment - Hey Sam, No problem. If I was going to give more valid wording then I'd be saying something along the lines of: "In the following resource you will see an example of something blah blah. Check it out first before you do the next blahblah. _ Activity Title _" Maybe its just the way I design courses, but I tend to go CONTEXT then CONTENT and rarely the other way around (which is the way you had it already). For the users I'm currently dealing with, only having the option to have the context (label) underneath would be as useful as a chocolate teapot. Hope this makes sense, Mark.
            Hide
            quen Sam Marshall added a comment -

            OK, I guess it kind of makes sense - but it doesn't fit well with how Moodle already uses that field.

            For example, if you look at the order of the 'Edit' page (for a URL, say) it has the title first, then it has the description. It would seem odd to edit them in this order, then display them in the other order.

            Similarly once you actually go into (e.g.) a Page, the page breadcrumb and/or heading displays first, and then (if displayed at all; there's an option) the description; not the other way around.

            I think our course staff do want 'context' type text occasionally, but generally it wouldn't be associated with a single activity as such. It would be more like, here's what we are going to do in this week - and then there's a few activities that follow. It wouldn't really be the description of an activity.

            Sorry but I don't think this issue can cover your requirement (unless somebody else disagrees...) - however if this issue is developed, the feature you want would be a potential extension to the then-existing code (ie we will have an option somewhere for 'Show description on course page' which would be an obvious place to add an additional option later), so it still kind of sort of helps you go in that direction...

            Show
            quen Sam Marshall added a comment - OK, I guess it kind of makes sense - but it doesn't fit well with how Moodle already uses that field. For example, if you look at the order of the 'Edit' page (for a URL, say) it has the title first, then it has the description. It would seem odd to edit them in this order, then display them in the other order. Similarly once you actually go into (e.g.) a Page, the page breadcrumb and/or heading displays first, and then (if displayed at all; there's an option) the description; not the other way around. I think our course staff do want 'context' type text occasionally, but generally it wouldn't be associated with a single activity as such. It would be more like, here's what we are going to do in this week - and then there's a few activities that follow. It wouldn't really be the description of an activity. Sorry but I don't think this issue can cover your requirement (unless somebody else disagrees...) - however if this issue is developed, the feature you want would be a potential extension to the then-existing code (ie we will have an option somewhere for 'Show description on course page' which would be an obvious place to add an additional option later), so it still kind of sort of helps you go in that direction...
            Hide
            quen Sam Marshall added a comment -

            I have completed development of this new feature, which is intended for Moodle 2.2; now requesting review. I did it in four logical parts which are separate commits:

            1) New database field including all associated factors (backup, UI, etc). New feature constant that controls whether the UI appears.
            2) New support for ->contentformat in cm_info (previously ->content had to be HTML format which was inconvenient for this use case because intro can be in any format)
            3) Implement automatic behaviour for any module that doesn't have a _get_coursemodule_info function.
            4) Added implementation for all modules except label (not applicable) and lesson (doesn't seem to have a description field). This commit also includes a little tidying up for the get_coursemodule_info functions to use the format introduced in Moodle 2.0.2 or so, including making the onclick links a bit nicer.

            When reviewing the feature it may make more sense to look at the four commits separately rather than everything jumbled up.

            For module developers, implementing this feature is done as follows:

            a) Return true for FEATURE_SHOW_DESCRIPTION in your module's _supports function.

            b) If you have 'intro, introformat' fields in your main table, and if you don't have a module_get_coursemodule_info function, then stop! You're done, it should work. There is no additional performance cost. (Used to be 1 query when building modinfo to get_field the name of your module; now there's 1 query which get_records the name, intro, and introformat.)

            c) If you do have a module_get_coursemodule_info function, or your description is in some other field or something, then you need to implement this feature in your module_get_coursemodule_info function. Usually that looks like

            <code>
            if ($coursemodule->showdescription) {
            $info->content = $mymodule->intro;
            $info->contentformat = $mymodule->introformat;
            }
            <code>

            Show
            quen Sam Marshall added a comment - I have completed development of this new feature, which is intended for Moodle 2.2; now requesting review. I did it in four logical parts which are separate commits: 1) New database field including all associated factors (backup, UI, etc). New feature constant that controls whether the UI appears. 2) New support for ->contentformat in cm_info (previously ->content had to be HTML format which was inconvenient for this use case because intro can be in any format) 3) Implement automatic behaviour for any module that doesn't have a _get_coursemodule_info function. 4) Added implementation for all modules except label (not applicable) and lesson (doesn't seem to have a description field). This commit also includes a little tidying up for the get_coursemodule_info functions to use the format introduced in Moodle 2.0.2 or so, including making the onclick links a bit nicer. When reviewing the feature it may make more sense to look at the four commits separately rather than everything jumbled up. For module developers, implementing this feature is done as follows: a) Return true for FEATURE_SHOW_DESCRIPTION in your module's _supports function. b) If you have 'intro, introformat' fields in your main table, and if you don't have a module_get_coursemodule_info function, then stop! You're done, it should work. There is no additional performance cost. (Used to be 1 query when building modinfo to get_field the name of your module; now there's 1 query which get_records the name, intro, and introformat.) c) If you do have a module_get_coursemodule_info function, or your description is in some other field or something, then you need to implement this feature in your module_get_coursemodule_info function. Usually that looks like <code> if ($coursemodule->showdescription) { $info->content = $mymodule->intro; $info->contentformat = $mymodule->introformat; } <code>
            Hide
            quen Sam Marshall added a comment -

            NOTE: This feature always displays the description below the title, as discussed above. As a future change or a local customisation, it would be possible to extend this code to allow you to choose to have description above the title instead of below, which Mark requested above. This is not a trivial task and I am not going to do it personally, but here is a rough guideline of how that could be done:

            a) Change the user interface from checkbox to a dropdown with options 0=no description, 1=description below, 2=description above.
            b) Extend the cm_info/cached_cm_info interface by adding ->contentabove as a boolean value (& store this in all necessary places).
            c) When ->showdescription is set to 2, set ->contentabove to true by default (so you don't have to change all the modules to take notice of the different value).
            d) In print_section, make it accept ->contentabove and, if set, display content above instead of below. (This may also require some changes to CSS etc so that it displays OK.)

            So although I haven't implemented it, this code certainly doesn't preclude implementing it later.

            Show
            quen Sam Marshall added a comment - NOTE: This feature always displays the description below the title, as discussed above. As a future change or a local customisation, it would be possible to extend this code to allow you to choose to have description above the title instead of below, which Mark requested above. This is not a trivial task and I am not going to do it personally, but here is a rough guideline of how that could be done: a) Change the user interface from checkbox to a dropdown with options 0=no description, 1=description below, 2=description above. b) Extend the cm_info/cached_cm_info interface by adding ->contentabove as a boolean value (& store this in all necessary places). c) When ->showdescription is set to 2, set ->contentabove to true by default (so you don't have to change all the modules to take notice of the different value). d) In print_section, make it accept ->contentabove and, if set, display content above instead of below. (This may also require some changes to CSS etc so that it displays OK.) So although I haven't implemented it, this code certainly doesn't preclude implementing it later.
            Hide
            quen Sam Marshall added a comment -

            Sorry, just noticed the title is out of date (since Helen requested to make this work for all activity types).

            Show
            quen Sam Marshall added a comment - Sorry, just noticed the title is out of date (since Helen requested to make this work for all activity types).
            Hide
            quen Sam Marshall added a comment -

            Is anyone looking at this? It seems to have been assigned to Martin, but surely he doesn't read bugmail...?

            Show
            quen Sam Marshall added a comment - Is anyone looking at this? It seems to have been assigned to Martin, but surely he doesn't read bugmail...?
            Hide
            quen Sam Marshall added a comment -

            Have attached screenshots of course page and forum settings with this patch, + html code of coursepage.

            Show
            quen Sam Marshall added a comment - Have attached screenshots of course page and forum settings with this patch, + html code of coursepage.
            Hide
            dougiamas Martin Dougiamas added a comment -

            Yep, this looks very sensible, Sam.

            I've only skimmed the diff, but it looks OK with the only possible performance issue being that modinfo could get too big on large courses.

            About the UI, I know "Common module settings" makes sense from a modinfo point of view but from a user POV wouldn't it be good to have that checkbox right under the Description field? Is that possible to do cleanly, or would it be too many hacks?

            Show
            dougiamas Martin Dougiamas added a comment - Yep, this looks very sensible, Sam. I've only skimmed the diff, but it looks OK with the only possible performance issue being that modinfo could get too big on large courses. About the UI, I know "Common module settings" makes sense from a modinfo point of view but from a user POV wouldn't it be good to have that checkbox right under the Description field? Is that possible to do cleanly, or would it be too many hacks?
            Hide
            quen Sam Marshall added a comment -

            Thanks Martin.

            Re 1) it should not add description to modinfo unless you turn on the option, so even on a large course, so long as you don't turn it on for everything (in which case that would be a laaaaaaaarge course page) it won't get very big.

            Re 2) I agree - this is because it adds it using the function that adds standard settings which gets called at that point, but I'll have a look to see if maybe I can insert it after existing field instead or something like that.

            Show
            quen Sam Marshall added a comment - Thanks Martin. Re 1) it should not add description to modinfo unless you turn on the option, so even on a large course, so long as you don't turn it on for everything (in which case that would be a laaaaaaaarge course page) it won't get very big. Re 2) I agree - this is because it adds it using the function that adds standard settings which gets called at that point, but I'll have a look to see if maybe I can insert it after existing field instead or something like that.
            Hide
            quen Sam Marshall added a comment -

            OK, since Martin thought it was okay, I'm now submitting for integration review.

            Regarding the checkbox position, I have made a change as requested: after doing something complicated to rearrange its position in the form (there isn't an insertElementAfter, only Before) I realised I could just, uh, add it to the end of the add_intro_editor function. Much simpler. I rebased this in (it's part of my 'commit 1') and pushed the new commits.

            As noted previously, reviewers might find it makes it easier to understand if you look at the commits one at a time.

            Show
            quen Sam Marshall added a comment - OK, since Martin thought it was okay, I'm now submitting for integration review. Regarding the checkbox position, I have made a change as requested: after doing something complicated to rearrange its position in the form (there isn't an insertElementAfter, only Before) I realised I could just, uh, add it to the end of the add_intro_editor function. Much simpler. I rebased this in (it's part of my 'commit 1') and pushed the new commits. As noted previously, reviewers might find it makes it easier to understand if you look at the commits one at a time.
            Hide
            stronk7 Eloy Lafuente (stronk7) added a comment -

            Ho Sam,

            I'm going to delay integration of this to next week, to have some more time to look @ some bits with more detail (running out of time this week).

            Thanks!

            Show
            stronk7 Eloy Lafuente (stronk7) added a comment - Ho Sam, I'm going to delay integration of this to next week, to have some more time to look @ some bits with more detail (running out of time this week). Thanks!
            Hide
            timhunt Tim Hunt added a comment -

            We would quite like to know if this is going to be integrated into 2.2 before next week. (There is an OU deadline next Tuesday.) Do you think it is likely that, in the case that you reject the current patch, it would just be to make some further changes. Can you promise that you will not reject the idea completely? Thanks.

            Show
            timhunt Tim Hunt added a comment - We would quite like to know if this is going to be integrated into 2.2 before next week. (There is an OU deadline next Tuesday.) Do you think it is likely that, in the case that you reject the current patch, it would just be to make some further changes. Can you promise that you will not reject the idea completely? Thanks.
            Hide
            stronk7 Eloy Lafuente (stronk7) 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
            stronk7 Eloy Lafuente (stronk7) 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
            stronk7 Eloy Lafuente (stronk7) added a comment -

            Hi,

            I'm sorry I must reject this because, or I've followed the process wrongly... or I think we can expect problems with those descriptions and any context-related information (filters, files...).

            If you take a look to the currently-working label implementation, it calls format_module_intro() that perform some useful things like:

            • preparing the pluginfile urls.
            • setting the correct options['context'] so filters and friends will be applied ok.

            And that code seems to be missing in the implementation, it uses sort of harcoded course context (double calling format_text??? for labels, uh) and misses pluginfile prepare and correct module context.

            Another point that "stresses" me a bit is the extra traffic happening by fetching always the description in modules having xxx_get_coursemodule_info() own implementation. Perhaps we could try to avoid that? After all, the param, $coursemodule, has the information about description being requested yes/no, isn't it?

            Finally, has the description output some css selector in order to be able to modify it from themes or so? Surely yes, just to be sure, I haven't reviewed the whole print_section() or whatever is in charge of that, lol.

            So, I think it needs a bit more of work. Overall seems to be fine, great work!

            Ciao

            Show
            stronk7 Eloy Lafuente (stronk7) added a comment - Hi, I'm sorry I must reject this because, or I've followed the process wrongly... or I think we can expect problems with those descriptions and any context-related information (filters, files...). If you take a look to the currently-working label implementation, it calls format_module_intro() that perform some useful things like: preparing the pluginfile urls. setting the correct options ['context'] so filters and friends will be applied ok. And that code seems to be missing in the implementation, it uses sort of harcoded course context (double calling format_text??? for labels, uh) and misses pluginfile prepare and correct module context. Another point that "stresses" me a bit is the extra traffic happening by fetching always the description in modules having xxx_get_coursemodule_info() own implementation. Perhaps we could try to avoid that? After all, the param, $coursemodule, has the information about description being requested yes/no, isn't it? Finally, has the description output some css selector in order to be able to modify it from themes or so? Surely yes, just to be sure, I haven't reviewed the whole print_section() or whatever is in charge of that, lol. So, I think it needs a bit more of work. Overall seems to be fine, great work! Ciao
            Hide
            timhunt Tim Hunt added a comment -

            Just adding an additional test-case, to reinforce Eloy's last comment.

            Show
            timhunt Tim Hunt added a comment - Just adding an additional test-case, to reinforce Eloy's last comment.
            Hide
            quen Sam Marshall added a comment -

            Eloy: Thanks very much for review. Just to update, I was away last week but I will get back to this as soon as I can and hopefully can fix the issues raised.

            Show
            quen Sam Marshall added a comment - Eloy: Thanks very much for review. Just to update, I was away last week but I will get back to this as soon as I can and hopefully can fix the issues raised.
            Hide
            quen Sam Marshall added a comment -

            Re two of Eloy's comments that I think probably (but not certain!) do not need addressing:

            1) Re 'extra traffic happening by fetching always the description in modules having xxx_get_coursemodule_info() own implementation' - this is the most efficient way to implement it because xxx_get_coursemodule_info() always retrieves that table row already so doing this makes no extra work, although it does mean a few lines of extra code. I could make it do it automatically by default if you don't set it (like it does if you don't have a _get_coursemodule_info function) but this would mean an extra db query compared to doing it in the function, so I think it's probably better to encourage that. However it would be possible to implement even though it's slightly less efficient?

            2) re 'double calling format_text for labels' I don't get it - this code isn't used for labels, that's one of the modules that doesn't have the feature.

            Other problems identified with images, filters these definitely needs fixing - Tim added filter testcase, I put one for images which currently fails.

            Once I get this working properly, will also do investigation of performance (#queries) with option on/off on multiple items on course page.

            Show
            quen Sam Marshall added a comment - Re two of Eloy's comments that I think probably (but not certain!) do not need addressing: 1) Re 'extra traffic happening by fetching always the description in modules having xxx_get_coursemodule_info() own implementation' - this is the most efficient way to implement it because xxx_get_coursemodule_info() always retrieves that table row already so doing this makes no extra work, although it does mean a few lines of extra code. I could make it do it automatically by default if you don't set it (like it does if you don't have a _get_coursemodule_info function) but this would mean an extra db query compared to doing it in the function, so I think it's probably better to encourage that. However it would be possible to implement even though it's slightly less efficient? 2) re 'double calling format_text for labels' I don't get it - this code isn't used for labels, that's one of the modules that doesn't have the feature. Other problems identified with images, filters these definitely needs fixing - Tim added filter testcase, I put one for images which currently fails. Once I get this working properly, will also do investigation of performance (#queries) with option on/off on multiple items on course page.
            Hide
            quen Sam Marshall added a comment -

            More notes:

            Re the bug Tim identified with activity-specific filter settings, this already applies to Label as it does filtering using course context same way. So is probably a bug with existing system. I will try to fix as part of this but considering performance.

            I see what Eloy means about double calling format_text for labels (that code was not changed in this work which is why I was confused). The first time when building modinfo cache it calls it with filter set to off (I guess just to turn to html), the second time when actually displaying 'live' on the page it filters it. I suppose that's OK. This might mean I can remove some of the changes because doesn't need a format option adding for content if we are going to always format_text to html first.

            Show
            quen Sam Marshall added a comment - More notes: Re the bug Tim identified with activity-specific filter settings, this already applies to Label as it does filtering using course context same way. So is probably a bug with existing system. I will try to fix as part of this but considering performance. I see what Eloy means about double calling format_text for labels (that code was not changed in this work which is why I was confused). The first time when building modinfo cache it calls it with filter set to off (I guess just to turn to html), the second time when actually displaying 'live' on the page it filters it. I suppose that's OK. This might mean I can remove some of the changes because doesn't need a format option adding for content if we are going to always format_text to html first.
            Hide
            quen Sam Marshall added a comment -

            OK, submitting for integration again. I have done the following:

            • Changed code to use format_module_intro as required. Images now work.
            • Removed the change to add a contentformat parameter to cm_info_cached as this is not now needed (it's always using html).
            • Retested performance. My test page has a number of activities with 'show description'; it uses 77 db queries before and after this change (i.e. I switched branches, and rebuilt modinfo). (I reloaded twice to avoid differences caused by short-term caches. This is the number it settles down to.)

            Note that filter settings do NOT work (i.e. this uses the course filter settings not the module filter settings). However that is currently the case for label already and it is the same issue. So I have filed it separately as MDL-29236.

            Show
            quen Sam Marshall added a comment - OK, submitting for integration again. I have done the following: Changed code to use format_module_intro as required. Images now work. Removed the change to add a contentformat parameter to cm_info_cached as this is not now needed (it's always using html). Retested performance. My test page has a number of activities with 'show description'; it uses 77 db queries before and after this change (i.e. I switched branches, and rebuilt modinfo). (I reloaded twice to avoid differences caused by short-term caches. This is the number it settles down to.) Note that filter settings do NOT work (i.e. this uses the course filter settings not the module filter settings). However that is currently the case for label already and it is the same issue. So I have filed it separately as MDL-29236 .
            Hide
            samhemelryk Sam Hemelryk added a comment - - edited

            Hi guys,

            I've had a look over these changes and have had a play with the patch it certainly gets my +1 however I will leave it up to Eloy to actually integrate this so he can give it his once over as well given he's looked at it previously.
            The only thing that I spotted that I think could be improved is the title on the new checkbox `Description on course page` perhaps `Display introduction on course page` or better yet check with Helen to see what she thinks.
            I'll leave it up to you Eloy.

            Thanks for the great work guys.
            Cheers
            Sam

            Show
            samhemelryk Sam Hemelryk added a comment - - edited Hi guys, I've had a look over these changes and have had a play with the patch it certainly gets my +1 however I will leave it up to Eloy to actually integrate this so he can give it his once over as well given he's looked at it previously. The only thing that I spotted that I think could be improved is the title on the new checkbox `Description on course page` perhaps `Display introduction on course page` or better yet check with Helen to see what she thinks. I'll leave it up to you Eloy. Thanks for the great work guys. Cheers Sam
            Hide
            tsala Helen Foster added a comment -

            As agreed in dev chat:

            'Display description on course page' with help string 'If enabled, the introduction / description above will be displayed on the course page just below the link to this activity / resource.'

            Thanks Sam and Sam

            Show
            tsala Helen Foster added a comment - As agreed in dev chat: 'Display description on course page' with help string 'If enabled, the introduction / description above will be displayed on the course page just below the link to this activity / resource.' Thanks Sam and Sam
            Hide
            quen Sam Marshall added a comment -

            Thanks Sam.

            I discussed it with Helen and have updated the wording as she recommended (and rebased). The text is now 'Display description on course page' (i.e. added the word 'display') and we have also changed the wording of the help popup which was slightly inaccurate. Because the help popup has more room, it uses the phrase 'introduction / description' to make clear if anyone is confused on the screens where the above box is titled 'introduction'.

            Additionally, I have filed MDL-29250 about the inconsistency in terms where some modules use Description and others use Introduction. (That is not a prerequisite for this issue, it just came up.)

            Show
            quen Sam Marshall added a comment - Thanks Sam. I discussed it with Helen and have updated the wording as she recommended (and rebased). The text is now 'Display description on course page' (i.e. added the word 'display') and we have also changed the wording of the help popup which was slightly inaccurate. Because the help popup has more room, it uses the phrase 'introduction / description' to make clear if anyone is confused on the screens where the above box is titled 'introduction'. Additionally, I have filed MDL-29250 about the inconsistency in terms where some modules use Description and others use Introduction. (That is not a prerequisite for this issue, it just came up.)
            Hide
            stronk7 Eloy Lafuente (stronk7) added a comment -

            Sam, I'm reviewing this right now... side clarification that will not stop integration:

            When talking above about performance concerns I was thinking above about to add the intro + introformat columns in the get record() calls in xxx_get_coursemodule_info() methods only if the passed $coursemodule->showdescription was requesting it.

            Imagine one course (border case, I know), will 100 activities (all them assignments, pages and urls, the ones having custom xxx_get_coursemodule_info() implementation). And imagine we don't want descriptions to be shown in course page at all. We'll be fetching 100 texts for... nothing. And LOB handling uses to be way slower than int/varchar previous alternative.

            So I was imagining some way to dynamically add those fields to the get_record() only if they have been requested.

            As said, surely not critical right now, but I can imagine that "border case" becoming more and more common as long as we implement the xxx_get_coursemodule_info() in other modules.

            Ciao

            Show
            stronk7 Eloy Lafuente (stronk7) added a comment - Sam, I'm reviewing this right now... side clarification that will not stop integration: When talking above about performance concerns I was thinking above about to add the intro + introformat columns in the get record() calls in xxx_get_coursemodule_info() methods only if the passed $coursemodule->showdescription was requesting it. Imagine one course (border case, I know), will 100 activities (all them assignments, pages and urls, the ones having custom xxx_get_coursemodule_info() implementation). And imagine we don't want descriptions to be shown in course page at all. We'll be fetching 100 texts for... nothing. And LOB handling uses to be way slower than int/varchar previous alternative. So I was imagining some way to dynamically add those fields to the get_record() only if they have been requested. As said, surely not critical right now, but I can imagine that "border case" becoming more and more common as long as we implement the xxx_get_coursemodule_info() in other modules. Ciao
            Hide
            quen Sam Marshall added a comment -

            Get you. But, get_coursemodule_info is only done on rebuild course cache, not on page view. So performance is really not that critical.

            I did a rough performance test (on OU live 1.9 database) to see what effect this has. I was testing by running thousands of queries in pgadmin window so this might not be exactly representative but should be somewhat so.

            for html pages (I was actually using the text of the page not intro, not sure they have intros in 1.9): approx 0.1ms/query if you don't include alltext, approx 1ms/query if you do. Obviously these html pages are likely to have a large quantity of content in each page, typically several KB.

            for forums (the majority have no intro, if they do have one it's small): 0.1ms/query either way (no difference).

            finally I used another table (ou specific) and did this on 11,000 rows where they DO have some text in a text field but in those rows it is less than 250 characters. I think that probably is more equivalent to a typical intro. In that case, it took approx 0.1ms/query either way; it seemed slightly faster if you don't include the textfield, maybe 0.09 instead, but not sure my testing was that accurate...

            so in summary - I think at least in a high-performance installation of Postgres 8.x (db I was using), including a text field does not seem to significantly worsen performance vs. including only numeric fields, EXCEPT when the contents of that text field are large. In that case it does make a huge difference.

            Show
            quen Sam Marshall added a comment - Get you. But, get_coursemodule_info is only done on rebuild course cache, not on page view. So performance is really not that critical. I did a rough performance test (on OU live 1.9 database) to see what effect this has. I was testing by running thousands of queries in pgadmin window so this might not be exactly representative but should be somewhat so. for html pages (I was actually using the text of the page not intro, not sure they have intros in 1.9): approx 0.1ms/query if you don't include alltext, approx 1ms/query if you do. Obviously these html pages are likely to have a large quantity of content in each page, typically several KB. for forums (the majority have no intro, if they do have one it's small): 0.1ms/query either way (no difference). finally I used another table (ou specific) and did this on 11,000 rows where they DO have some text in a text field but in those rows it is less than 250 characters. I think that probably is more equivalent to a typical intro. In that case, it took approx 0.1ms/query either way; it seemed slightly faster if you don't include the textfield, maybe 0.09 instead, but not sure my testing was that accurate... so in summary - I think at least in a high-performance installation of Postgres 8.x (db I was using), including a text field does not seem to significantly worsen performance vs. including only numeric fields, EXCEPT when the contents of that text field are large. In that case it does make a huge difference.
            Hide
            stronk7 Eloy Lafuente (stronk7) added a comment -

            Nice, thanks for the complete testing and details, fair enough... and plz, don't migrate to Oracle, LOL.

            Show
            stronk7 Eloy Lafuente (stronk7) added a comment - Nice, thanks for the complete testing and details, fair enough... and plz, don't migrate to Oracle, LOL.
            Hide
            stronk7 Eloy Lafuente (stronk7) added a comment -

            And this has been integrated, thanks!

            PS: I've forced, via extra commit one bump to 2011083100.02, because we were already using 2011083100.01 (@ integration).

            Show
            stronk7 Eloy Lafuente (stronk7) added a comment - And this has been integrated, thanks! PS: I've forced, via extra commit one bump to 2011083100.02, because we were already using 2011083100.01 (@ integration).
            Hide
            samhemelryk Sam Hemelryk added a comment -

            Passed thanks guys!

            Show
            samhemelryk Sam Hemelryk added a comment - Passed thanks guys!
            Hide
            stronk7 Eloy Lafuente (stronk7) added a comment -

            git & cvs repositories updated with your gorgeous code. Many thanks!

            Closing and ciao

            Show
            stronk7 Eloy Lafuente (stronk7) added a comment - git & cvs repositories updated with your gorgeous code. Many thanks! Closing and ciao
            Hide
            tsala Helen Foster added a comment -

            Linking to master copy of QA test for this improvement. Comments about the QA test welcome!

            Show
            tsala Helen Foster added a comment - Linking to master copy of QA test for this improvement. Comments about the QA test welcome!

              People

              • Votes:
                4 Vote for this issue
                Watchers:
                11 Start watching this issue

                Dates

                • Created:
                  Updated:
                  Resolved:
                  Fix Release Date:
                  5/Dec/11