Moodle
  1. Moodle
  2. MDL-24521

Deleting an external RSS feed from Moodle is failing

    Details

    • Affected Branches:
      MOODLE_20_STABLE
    • Fixed Branches:
      MOODLE_20_STABLE

      Description

      Go to the manage my feeds screen (/blocks/rss_client/managefeeds.php) Click the cross next to a feed to delete it. Click yes to confirm. An alert is displayed that says:

      Element of type IMG is not supported by the M.util.show_confirm_dialog function. Use A or INPUT

      When i refresh the page the rss feed has NOT been deleted.

        Gliffy Diagrams

        1. MDL-24521.20101109.patch
          2 kB
          Sam Hemelryk
        2. MDL-24521-Patch.txt
          0.7 kB
          John Anderson
        3. MDL-24521-Patch-ammended.txt
          0.7 kB
          John Anderson

          Issue Links

            Activity

            Hide
            John Anderson added a comment -

            Here is a patch I created that seems to resolve this issue. The issue was caused when parentNode was not included when attempting to get the tagName. Instead of getting the A tag got the IMG tag of the Delete button which caused the error.

            Show
            John Anderson added a comment - Here is a patch I created that seems to resolve this issue. The issue was caused when parentNode was not included when attempting to get the tagName. Instead of getting the A tag got the IMG tag of the Delete button which caused the error.
            Hide
            John Anderson added a comment -

            The above patch included a change that should not be a part of the patch, so I've reattached this fix.

            Show
            John Anderson added a comment - The above patch included a change that should not be a part of the patch, so I've reattached this fix.
            Hide
            James Brisland added a comment -

            I had a quick look at the patch, and applied it to my dev.

            It all works, although I'm a little worried about the change from get('tagName') to get('parentNode.tagName') as it may affect places elsewhere.

            I was going to check on this, but got sidetracked by a quiz bug

            Would someone be able to make sure this patch doesn't affect other places in code that use the confirmation popup (the single_select and single_button classes?)

            Show
            James Brisland added a comment - I had a quick look at the patch, and applied it to my dev. It all works, although I'm a little worried about the change from get('tagName') to get('parentNode.tagName') as it may affect places elsewhere. I was going to check on this, but got sidetracked by a quiz bug Would someone be able to make sure this patch doesn't affect other places in code that use the confirmation popup (the single_select and single_button classes?)
            Hide
            Sam Hemelryk added a comment -

            Hi guys,

            I've just tested this patch out and things work find in the other areas. No regressions that I could see.
            I've also just attached a second patch you may want to have a look at.
            It uses the YUI 3 methods that are now available on the target rather than direct node interaction and has the advantage that if the target is within a anchor element but is not a direct child it will still work ( a > span > img ).

            Cheers
            Sam

            Show
            Sam Hemelryk added a comment - Hi guys, I've just tested this patch out and things work find in the other areas. No regressions that I could see. I've also just attached a second patch you may want to have a look at. It uses the YUI 3 methods that are now available on the target rather than direct node interaction and has the advantage that if the target is within a anchor element but is not a direct child it will still work ( a > span > img ). Cheers Sam
            Hide
            Sam Marshall added a comment -

            Sam - not sure if James has time to look at it here - so you might want to commit your patch if you are happy with it!

            Show
            Sam Marshall added a comment - Sam - not sure if James has time to look at it here - so you might want to commit your patch if you are happy with it!
            Hide
            Dan Poltawski added a comment -

            Sam - do you want to push this change? Its still broken in Moodle 2.0, so it'd be good to get the fix out there!

            Show
            Dan Poltawski added a comment - Sam - do you want to push this change? Its still broken in Moodle 2.0, so it'd be good to get the fix out there!
            Hide
            Sam Hemelryk added a comment -

            Hi Dan,

            Thanks for the prompt, I'll make sure this gets into the next sprint for review.

            Cheers
            Sam

            Show
            Sam Hemelryk added a comment - Hi Dan, Thanks for the prompt, I'll make sure this gets into the next sprint for review. Cheers Sam
            Hide
            Sam Hemelryk added a comment -

            Hi Andrew,

            Could you please peer-review the patch for this: https://github.com/samhemelryk/moodle/compare/master...wip-MDL-24521

            Cheers
            Sam

            Show
            Sam Hemelryk added a comment - Hi Andrew, Could you please peer-review the patch for this: https://github.com/samhemelryk/moodle/compare/master...wip-MDL-24521 Cheers Sam
            Hide
            Andrew Davis added a comment -

            Looks sensible enough and works in both FF and IE.

            Show
            Andrew Davis added a comment - Looks sensible enough and works in both FF and IE.
            Hide
            Sam Hemelryk added a comment -

            Cool thanks Andrew,
            I've now created PULL-73 for this.

            Cheers
            Sam

            Show
            Sam Hemelryk added a comment - Cool thanks Andrew, I've now created PULL-73 for this. Cheers Sam
            Hide
            Eloy Lafuente (stronk7) added a comment -

            Closing as has been integrated and tested. Will land upstream in hours. Thanks everybody!

            Show
            Eloy Lafuente (stronk7) added a comment - Closing as has been integrated and tested. Will land upstream in hours. Thanks everybody!

              People

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

                Dates

                • Created:
                  Updated:
                  Resolved: