Moodle
  1. Moodle
  2. MDL-38431

Timestamp is shown as undefined in comments

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: 2.3.5, 2.4.2, 2.5
    • Fix Version/s: 2.3.7, 2.4.4
    • Component/s: Blog, Commenting
    • Labels:
    • Testing Instructions:
      Hide
      1. Log in as admin.
      2. Enable blog comments in blog settings (Site administration ► Appearance ► Blog)
      3. Create blog
      4. Add comments (Make sure you are using JS)
      5. You will see proper timestamp.
      6. Reload page and timestamp = proper time stamp
      7. Disable JS and reload page
      8. Make sure time stamp is same.

      Test ON MOODLE_23

      1. Enable xmlstrictheaders under debugging. (Site administration ► Development ► Debugging)
      2. Repeat above steps
      3. Make sure you don't encounter any JS error.
      Show
      Log in as admin. Enable blog comments in blog settings (Site administration ► Appearance ► Blog) Create blog Add comments (Make sure you are using JS) You will see proper timestamp. Reload page and timestamp = proper time stamp Disable JS and reload page Make sure time stamp is same. Test ON MOODLE_23 Enable xmlstrictheaders under debugging. (Site administration ► Development ► Debugging) Repeat above steps Make sure you don't encounter any JS error.
    • Affected Branches:
      MOODLE_23_STABLE, MOODLE_24_STABLE, MOODLE_25_STABLE
    • Fixed Branches:
      MOODLE_23_STABLE, MOODLE_24_STABLE
    • Pull 2.4 Branch:
      wip-mdl-38431-m24
    • Pull Master Branch:
      wip-mdl-38431
    • Rank:
      48363

      Description

      Steps to reproduce:

      1. Log in as admin
      2. Enable blog comments in blog settings (Site administration ► Appearance ► Blog)
      3. Create blog
      4. Add comments (Make sure you are using JS)
      5. You will see proper timestamp.
      6. Reload page and timestamp = undefined

        Issue Links

          Activity

          Hide
          Rajesh Taneja added a comment -

          Increasing priority as it gives JS error on 23 stable.

          Show
          Rajesh Taneja added a comment - Increasing priority as it gives JS error on 23 stable.
          Hide
          Ankit Agarwal added a comment -

          Hi Raj,
          The patch looks great. As we discussed the Js error was only in strict html standard mode, still worth fixing.
          [y] Syntax
          [y] Output
          [y] Whitespace
          [y] Language
          [na] Databases
          [y] Testing
          [na] Security
          [na] Documentation
          [y] Git
          [na] Sanity check

          Feel free to submit for integration.
          Thanks

          Show
          Ankit Agarwal added a comment - Hi Raj, The patch looks great. As we discussed the Js error was only in strict html standard mode, still worth fixing. [y] Syntax [y] Output [y] Whitespace [y] Language [na] Databases [y] Testing [na] Security [na] Documentation [y] Git [na] Sanity check Feel free to submit for integration. Thanks
          Hide
          Jason Fowler added a comment -

          I need this issue integrated before my MDL-35980 issue can be completed.

          Show
          Jason Fowler added a comment - I need this issue integrated before my MDL-35980 issue can be completed.
          Hide
          Rajesh Taneja added a comment -

          Thanks Ankit,

          I have updated test instructions.

          Note: Issue spotted in 23 is replicable with XML strict headers on. In 24/master this setting is not there, but it will be nice to get this fix for them as well.

          Show
          Rajesh Taneja added a comment - Thanks Ankit, I have updated test instructions. Note: Issue spotted in 23 is replicable with XML strict headers on. In 24/master this setting is not there, but it will be nice to get this fix for them as well.
          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
          Damyon Wiese added a comment -

          Didn't finish looking at this - putting back in the pile for tomorrow.

          Show
          Damyon Wiese added a comment - Didn't finish looking at this - putting back in the pile for tomorrow.
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Uhm... I'm not entirely convinced here... so... the real problem is that the comments js is expecting "time" property while the DB and php is using "timecreated".

          Could you, plz, create an improvement issue (for 2.6, surely, aka DEV) about to normalize all the uses of timecreated along the comments API, deciding between time/timecreated and using only one of them?

          Also... IMO... the get_comments() is really doing too much processing and a lot of its work should be done by renderer (print_comment) instead. But again, that's something we cannot change now, grrr. Another issue about better separating getting raw comments and preparing them for output?

          Finally, the "true" in out(true)... seems unnecessary (it's the default). More yet... surely it's better to pass the moodle_url all around instead of "outputting" it soo early.

          But any of them are going to halt this, so integrating as far as the patch fixes the problem. Plz, don't forget to create the related issues for the future.

          Show
          Eloy Lafuente (stronk7) added a comment - Uhm... I'm not entirely convinced here... so... the real problem is that the comments js is expecting "time" property while the DB and php is using "timecreated". Could you, plz, create an improvement issue (for 2.6, surely, aka DEV) about to normalize all the uses of timecreated along the comments API, deciding between time/timecreated and using only one of them? Also... IMO... the get_comments() is really doing too much processing and a lot of its work should be done by renderer (print_comment) instead. But again, that's something we cannot change now, grrr. Another issue about better separating getting raw comments and preparing them for output? Finally, the "true" in out(true)... seems unnecessary (it's the default). More yet... surely it's better to pass the moodle_url all around instead of "outputting" it soo early. But any of them are going to halt this, so integrating as far as the patch fixes the problem. Plz, don't forget to create the related issues for the future.
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Integrated (23, 24 and master), thanks!

          Show
          Eloy Lafuente (stronk7) added a comment - Integrated (23, 24 and master), thanks!
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Jeje, while looking to this... one more thing... the JS, when adding the link to the user profile... somehow produces an unescaped URL (with & instead of &amp.

          It's something the validators ignore... but being strict... they should be proper encoded too.

          Anyway, just a tiny detail. Feel free to create another new issue for that.

          Otherwise... passing this. Ciao

          Show
          Eloy Lafuente (stronk7) added a comment - Jeje, while looking to this... one more thing... the JS, when adding the link to the user profile... somehow produces an unescaped URL (with & instead of &amp . It's something the validators ignore... but being strict... they should be proper encoded too. Anyway, just a tiny detail. Feel free to create another new issue for that. Otherwise... passing this. Ciao
          Hide
          Damyon Wiese added a comment -

          This issue has been integrated upstream and is now available via git (and in some hours, via mirrors and downloads).

          Thanks for your contributions!

          Show
          Damyon Wiese added a comment - This issue has been integrated upstream and is now available via git (and in some hours, via mirrors and downloads). Thanks for your contributions!

            People

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

              Dates

              • Created:
                Updated:
                Resolved: