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

Timestamp is shown as undefined in comments

    Details

    • Type: Bug
    • Status: Closed
    • Priority: 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, Comments
    • 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 Master Branch:
      wip-mdl-38431

      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

        Gliffy Diagrams

          Issue Links

            Activity

            Hide
            rajeshtaneja Rajesh Taneja added a comment -

            Increasing priority as it gives JS error on 23 stable.

            Show
            rajeshtaneja Rajesh Taneja added a comment - Increasing priority as it gives JS error on 23 stable.
            Hide
            ankit_frenz 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_frenz 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
            phalacee Jason Fowler added a comment -

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

            Show
            phalacee Jason Fowler added a comment - I need this issue integrated before my MDL-35980 issue can be completed.
            Hide
            rajeshtaneja 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
            rajeshtaneja 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
            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
            damyon Damyon Wiese added a comment -

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

            Show
            damyon Damyon Wiese added a comment - Didn't finish looking at this - putting back in the pile for tomorrow.
            Hide
            stronk7 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
            stronk7 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
            stronk7 Eloy Lafuente (stronk7) added a comment -

            Integrated (23, 24 and master), thanks!

            Show
            stronk7 Eloy Lafuente (stronk7) added a comment - Integrated (23, 24 and master), thanks!
            Hide
            stronk7 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
            stronk7 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 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 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:
                  Fix Release Date:
                  13/May/13