Moodle
  1. Moodle
  2. MDL-43295

Debugging warning on saving an online text submission with an uploaded image.

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Critical Critical
    • Resolution: Fixed
    • Affects Version/s: 2.6
    • Fix Version/s: 2.6.1, 2.7
    • Component/s: Events API
    • Labels:
      None
    • Testing Instructions:
      Hide

      Pre-requisites

      • Some students enrolled in a course.
      • Developer level debugging must be enabled.

      Testing

      1. Create an assignment (mod_assign).
        • Make sure that the submission type is "Online text" only (untick "File submissions").
      2. Log in as a student.
      3. Make a submission for the assignment. Make sure to embed an image into the online text.
      4. When you click "Save changes" make sure that no debugging message is shown.

      Unit tests

      • Run phpunit mod/assign/submission/onlinetext/tests/events_test.php and check that they pass.
      Show
      Pre-requisites Some students enrolled in a course. Developer level debugging must be enabled. Testing Create an assignment (mod_assign). Make sure that the submission type is "Online text" only (untick "File submissions"). Log in as a student. Make a submission for the assignment. Make sure to embed an image into the online text. When you click "Save changes" make sure that no debugging message is shown. Unit tests Run phpunit mod/assign/submission/onlinetext/tests/events_test.php and check that they pass.
    • Affected Branches:
      MOODLE_26_STABLE
    • Fixed Branches:
      MOODLE_26_STABLE, MOODLE_27_STABLE
    • Pull from Repository:
    • Pull 2.6 Branch:
      wip-MDL-43295-26
    • Pull Master Branch:
      wip-MDL-43295-master
    • Story Points (Obsolete):
      13
    • Sprint:
      BACKEND Sprint 8

      Description

      Warning:

      Before calling format_text(), the content must be processed with file_rewrite_pluginfile_urls() (line 144 of /mod/assign/submission/onlinetext/locallib.php).

      It makes no sense to me that we are calling format_text and storing the result as 'content' in the event. IMO we should store the raw text and format instead. I think this should be a "rule" for all events.

        Gliffy Diagrams

          Issue Links

            Activity

            Hide
            Damyon Wiese added a comment -

            Also note - if the unit test for this event included some files, this would have been detected.

            Show
            Damyon Wiese added a comment - Also note - if the unit test for this event included some files, this would have been detected.
            Hide
            Damyon Wiese added a comment -

            Made this Critical! because it is blocking a Critical! issue.

            Show
            Damyon Wiese added a comment - Made this Critical! because it is blocking a Critical! issue.
            Hide
            Damyon Wiese added a comment -

            [Y] Syntax
            [Y] Whitespace
            [-] Output
            [-] Language
            [-] Databases
            [N] Testing (instructions and automated tests) - Your manual testing instructions wont actually trigger the error in the original code. You need to add an image to the onlinetext field to trigger the warning.
            [-] Security
            [Y] Documentation
            [Y] Git
            [-] Third party code
            [Y] Sanity check

            Can you just tweak the testing instructions?

            Then this looks good for integration. Thanks Adrian!

            Show
            Damyon Wiese added a comment - [Y] Syntax [Y] Whitespace [-] Output [-] Language [-] Databases [N] Testing (instructions and automated tests) - Your manual testing instructions wont actually trigger the error in the original code. You need to add an image to the onlinetext field to trigger the warning. [-] Security [Y] Documentation [Y] Git [-] Third party code [Y] Sanity check Can you just tweak the testing instructions? Then this looks good for integration. Thanks Adrian!
            Hide
            Adrian Greeve added a comment -

            Thanks Damyon,

            I've updated the testing instructions to include adding an image in the submission.

            Sending to integration.

            Show
            Adrian Greeve added a comment - Thanks Damyon, I've updated the testing instructions to include adding an image in the submission. Sending to integration.
            Hide
            Marina Glancy added a comment - - edited

            hi Adrian, looks good. Seems that format_text() change description should be in 2.6.1 section of upgrade.txt and not 2.6
            (edited, sorry Adrian, so used to the assignment issues being always Damyon's)

            Show
            Marina Glancy added a comment - - edited hi Adrian, looks good. Seems that format_text() change description should be in 2.6.1 section of upgrade.txt and not 2.6 (edited, sorry Adrian, so used to the assignment issues being always Damyon's)
            Hide
            Marina Glancy added a comment -

            Thanks Adrian, integrated in 2.6 and master

            Show
            Marina Glancy added a comment - Thanks Adrian, integrated in 2.6 and master
            Hide
            Rajesh Taneja added a comment -

            Thanks for fixing this Adrian,

            Works fine.. Passing...

            Show
            Rajesh Taneja added a comment - Thanks for fixing this Adrian, Works fine.. Passing...
            Hide
            Damyon Wiese added a comment -

            Twas the week before Christmas,
            And all though HQ
            Devs were scrambling to finish peer review.
            They sent all their issues,
            and rushed out the door -
            "To the beach!" someone heard them roar!

            This issue has been released upstream. Thanks!

            Show
            Damyon Wiese added a comment - Twas the week before Christmas, And all though HQ Devs were scrambling to finish peer review. They sent all their issues, and rushed out the door - "To the beach!" someone heard them roar! This issue has been released upstream. Thanks!

              People

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

                Dates

                • Created:
                  Updated:
                  Resolved:

                  Agile