Moodle
  1. Moodle
  2. MDL-27001

Option to display activity description on course page

    Details

    • Type: Improvement Improvement
    • Status: Closed
    • Priority: Minor 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
    • Rank:
      16633

      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

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

        Issue Links

          Activity

          Hide
          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
          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
          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
          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
          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
          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
          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
          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
          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
          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
          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
          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
          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
          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
          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
          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
          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
          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
          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
          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
          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
          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
          Sam Marshall added a comment -

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

          Show
          Sam Marshall added a comment - Have attached screenshots of course page and forum settings with this patch, + html code of coursepage.
          Hide
          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
          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
          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
          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
          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
          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
          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
          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
          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
          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
          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
          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
          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
          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
          Tim Hunt added a comment -

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

          Show
          Tim Hunt added a comment - Just adding an additional test-case, to reinforce Eloy's last comment.
          Hide
          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
          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
          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
          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
          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
          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
          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
          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
          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
          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
          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
          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
          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
          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
          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
          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
          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
          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
          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
          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
          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
          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
          Sam Hemelryk added a comment -

          Passed thanks guys!

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

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

          Closing and ciao

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

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

          Show
          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: