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

Deleting an external RSS feed from Moodle is failing

    Details

    • Type: Bug
    • Status: Closed
    • Priority: Major
    • Resolution: Fixed
    • Affects Version/s: 2.0
    • Fix Version/s: 2.0.2
    • Component/s: Administration, JavaScript, RSS
    • Labels:
      None
    • 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

          Attachments

            Issue Links

              Activity

              Hide
              john_a 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_a 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_a 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_a 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
              mrbriz 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
              mrbriz 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
              samhemelryk 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
              samhemelryk 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
              quen 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
              quen 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
              poltawski 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
              poltawski 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
              samhemelryk 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
              samhemelryk 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
              samhemelryk 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
              samhemelryk 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
              andyjdavis Andrew Davis added a comment -

              Looks sensible enough and works in both FF and IE.

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

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

              Cheers
              Sam

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

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

              Show
              stronk7 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:
                    Fix Release Date:
                    21/Feb/11