Moodle
  1. Moodle
  2. MDL-31636

Comments API needs to allow plugins to set the date format

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: 2.2.1, 2.4.1
    • Fix Version/s: 2.4.2
    • Component/s: Commenting
    • Testing Instructions:
      Hide

      1) you need to be a developer to test this as this is an API option that is as yet unused within core. Please read through entire test before commencing.
      2) edit the comments block display callback to change the display format in each of the comments
      (New version)
      blocks/comments/lib.php:

      function block_comments_comment_display($comments, $args) {
          if ($args->commentarea != 'page_comments') {
              throw new comment_exception('invalidcommentarea');
          }
          if ($args->itemid != 0) {
              throw new comment_exception('invalidcommentitemid');
          }
          foreach ($comments as $comment) {
              $comment->strftimeformat = get_string('strftimerecentfull', 'langconfig');
          }
          return $comments;
      }
      

      3) Check that the comments in the comment block now display the year as well as the date/time.

      Show
      1) you need to be a developer to test this as this is an API option that is as yet unused within core. Please read through entire test before commencing. 2) edit the comments block display callback to change the display format in each of the comments (New version) blocks/comments/lib.php: function block_comments_comment_display($comments, $args) { if ($args->commentarea != 'page_comments') { throw new comment_exception('invalidcommentarea'); } if ($args->itemid != 0) { throw new comment_exception('invalidcommentitemid'); } foreach ($comments as $comment) { $comment->strftimeformat = get_string('strftimerecentfull', 'langconfig'); } return $comments; } 3) Check that the comments in the comment block now display the year as well as the date/time.
    • Affected Branches:
      MOODLE_22_STABLE, MOODLE_24_STABLE
    • Fixed Branches:
      MOODLE_24_STABLE
    • Pull from Repository:
    • Pull 2.4 Branch:
      MDL-31636_m24
    • Pull Master Branch:
    • Rank:
      25842

      Description

      As illustrated in attached screenshot (taken from http://moodle.org/mod/data/view.php?d=13&rid=4124 ) the dates of comments do not include the year. Comments in the modules and plugins database span over several years, and so including the year would be most helpful.

      I've reported this issue as a moodle.org issue, as I don't know whether all sites would want comment dates to include the year, or whether it is better to keep the dates as short as possible.

      Edited to add: On reflection, I think the dates on comments should be the same as elsewhere in Moodle i.e. including a year, and so have moved the issue from MDLSITE to MDL.

        Issue Links

          Activity

          Hide
          Helen Foster added a comment -

          I'm changing the fix version to STABLE backlog and assigning it to moodle.com, as this is actually missing functionality from 1.9, when the date was included in comments.

          Show
          Helen Foster added a comment - I'm changing the fix version to STABLE backlog and assigning it to moodle.com, as this is actually missing functionality from 1.9, when the date was included in comments.
          Hide
          Ray Lawrence added a comment -

          +1 Comments really need this

          Show
          Ray Lawrence added a comment - +1 Comments really need this
          Hide
          Aparup Banerjee added a comment -

          i'm working on this atm for the plugins directory.

          Show
          Aparup Banerjee added a comment - i'm working on this atm for the plugins directory.
          Hide
          Aparup Banerjee added a comment -

          updated branches to 24 and master. up for review.

          integrators: this is designed to backport to 24 to display comments with 'year' on moodle.org/plugins. feel free to backport even further as i've noticed the interesting 'lost_functionality' tag (which kind of makes this a bug in reality, not an improvement imo, so i've just changed bug type now too.)

          Show
          Aparup Banerjee added a comment - updated branches to 24 and master. up for review. integrators: this is designed to backport to 24 to display comments with 'year' on moodle.org/plugins. feel free to backport even further as i've noticed the interesting 'lost_functionality' tag (which kind of makes this a bug in reality, not an improvement imo, so i've just changed bug type now too.)
          Hide
          Martin Dougiamas added a comment -

          Why are you propagating strftimeformat all the way through JS instead of just displaying whatever string the server provides?

          Show
          Martin Dougiamas added a comment - Why are you propagating strftimeformat all the way through JS instead of just displaying whatever string the server provides?
          Hide
          Aparup Banerjee added a comment -

          so that the option can be supplied by differently by different plugins. local_plugins might want to display its comments different to a course comment's block. so then a server's single format doesn't quiet apply across all comment formats so well.

          Show
          Aparup Banerjee added a comment - so that the option can be supplied by differently by different plugins. local_plugins might want to display its comments different to a course comment's block. so then a server's single format doesn't quiet apply across all comment formats so well.
          Hide
          Martin Dougiamas added a comment -

          OK, I'll shut up after this as I don't really know the code but I assumed that whatever was giving JS the times could also choose to format the times as a string first, simplifying a lot of things. But I guess this might break backward compat.

          Show
          Martin Dougiamas added a comment - OK, I'll shut up after this as I don't really know the code but I assumed that whatever was giving JS the times could also choose to format the times as a string first, simplifying a lot of things. But I guess this might break backward compat.
          Hide
          Aparup Banerjee added a comment -

          yep that's basically what's been done within print_comment() while maintaining BC. further to that was changes due to the pagination and needing the time format remembered across those paginated ajax requests (from any plugin) - hence even more propagation.

          Show
          Aparup Banerjee added a comment - yep that's basically what's been done within print_comment() while maintaining BC. further to that was changes due to the pagination and needing the time format remembered across those paginated ajax requests (from any plugin) - hence even more propagation.
          Hide
          Aparup Banerjee added a comment -

          sending for integration.

          Show
          Aparup Banerjee added a comment - sending for integration.
          Hide
          Damyon Wiese added a comment -

          Thanks Aparup,

          Looking at the patch and Martins comment - why don't you set the strftimeformat in the comment object passed to the xxx_comments_display callback? Then allow plugins to change it in the callback if required, and then use that format in the print_comment function. This means there are no changes to function arguments and the format does not need to be passed around by the JS.

          Also - there is a print_r debug in this patch.

          Show
          Damyon Wiese added a comment - Thanks Aparup, Looking at the patch and Martins comment - why don't you set the strftimeformat in the comment object passed to the xxx_comments_display callback? Then allow plugins to change it in the callback if required, and then use that format in the print_comment function. This means there are no changes to function arguments and the format does not need to be passed around by the JS. Also - there is a print_r debug in this patch.
          Hide
          Aparup Banerjee added a comment - - edited

          ah thanks, yes that is better - but alas the xxx_comments_display callback isn't being called via the comments.js render function (upon comments paging). i think at some point here i dug the wrong way through JS. i'm just figuring out how to pass the vars through to the js better now.

          EDIT: i see what you mean - i'll use the callback to completely avoid the JS now yay!

          Show
          Aparup Banerjee added a comment - - edited ah thanks, yes that is better - but alas the xxx_comments_display callback isn't being called via the comments.js render function (upon comments paging). i think at some point here i dug the wrong way through JS. i'm just figuring out how to pass the vars through to the js better now. EDIT: i see what you mean - i'll use the callback to completely avoid the JS now yay!
          Hide
          Aparup Banerjee added a comment -
          Show
          Aparup Banerjee added a comment - Please integrate the patch from here instead: diff: https://github.com/nebgor/moodle/compare/moodle:master...nebgor:MDL-31636_take2 branch: MDL-31636 _take2 this works with https://github.com/nebgor/moodle-local_plugins/compare/MDLSITE-2091 well
          Hide
          Damyon Wiese added a comment -

          Thanks Aparup - this looks much better. Shouldn't we change the default format as well to address the "lost functionality" - strftimerecentfull ?

          Show
          Damyon Wiese added a comment - Thanks Aparup - this looks much better. Shouldn't we change the default format as well to address the "lost functionality" - strftimerecentfull ?
          Hide
          Damyon Wiese added a comment -

          I also had to add

          $commentlist = array($newcmt);
          +
          +            if (!empty($this->plugintype)) {
          +                // moodle module will filter comments
          +                $commentlist = plugin_callback($this->plugintype, $this->pluginname, 'comment', 'display
          +                $newcmt = $commentlist[0];
          +            }
          +            $newcmt->time = userdate($newcmt->timecreated, $newcmt->strftimeformat);
          

          To the add function to get the new date format to apply to newly added comments before the page refreshes.

          Show
          Damyon Wiese added a comment - I also had to add $commentlist = array($newcmt); + + if (!empty($ this ->plugintype)) { + // moodle module will filter comments + $commentlist = plugin_callback($ this ->plugintype, $ this ->pluginname, 'comment', 'display + $newcmt = $commentlist[0]; + } + $newcmt->time = userdate($newcmt->timecreated, $newcmt->strftimeformat); To the add function to get the new date format to apply to newly added comments before the page refreshes.
          Hide
          Aparup Banerjee added a comment -

          I'm not sure on the changing the default date format for stable, it may have unwanted effects on production sites.
          It could be considered for master although with it being modifiable easily now i think generally comments in activities wouldn't span over a year, and wouldn't need to display the year. I'd personally lean towards letting anyone change it to what they need.

          Adding comments seem to look fine without the year (as it was just posted), but its definitely better for a consistent feel (also if time formats get really radical )
          Thanks for your help Damyon.

          Show
          Aparup Banerjee added a comment - I'm not sure on the changing the default date format for stable, it may have unwanted effects on production sites. It could be considered for master although with it being modifiable easily now i think generally comments in activities wouldn't span over a year, and wouldn't need to display the year. I'd personally lean towards letting anyone change it to what they need. Adding comments seem to look fine without the year (as it was just posted), but its definitely better for a consistent feel (also if time formats get really radical ) Thanks for your help Damyon.
          Hide
          Damyon Wiese added a comment -

          Split into 2 issues (this one for the api change and another one for changing the default format for Moodle).

          I'll backport this to 2.4 so it can be applied to moodle.org.

          Show
          Damyon Wiese added a comment - Split into 2 issues (this one for the api change and another one for changing the default format for Moodle). I'll backport this to 2.4 so it can be applied to moodle.org.
          Hide
          Damyon Wiese added a comment -

          Thanks Aparup, I have pushed this to master and 24 branches. This is just the API change - I haven't changed the default format.

          Show
          Damyon Wiese added a comment - Thanks Aparup, I have pushed this to master and 24 branches. This is just the API change - I haven't changed the default format.
          Hide
          Mark Nelson added a comment -

          Works as expected, passing.

          Show
          Mark Nelson added a comment - Works as expected, passing.
          Hide
          Damyon Wiese added a comment -

          Thanks for your hard work - this issue has made it! Moodle is now a little bit better.

          Regards, Damyon

          Show
          Damyon Wiese added a comment - Thanks for your hard work - this issue has made it! Moodle is now a little bit better. Regards, Damyon

            People

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

              Dates

              • Created:
                Updated:
                Resolved: