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
    • Rank:
      502

      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.

      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: