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

URL Download Repository doesn't warn when asked to download non-image files

    Details

    • Type: Bug
    • Status: Closed
    • Priority: Minor
    • Resolution: Fixed
    • Affects Version/s: 2.3.8, 2.4.5, 2.5.1
    • Fix Version/s: 2.4.7, 2.5.3
    • Component/s: Repositories
    • Labels:

      Description

      See MDL-27336 for the real fix to allow the URL download repository to download arbitrary files as it was intended to do. For now we should warn the user why there are no files to choose from.

        Gliffy Diagrams

          Issue Links

            Activity

            Hide
            fred Frédéric Massart added a comment -

            Thanks for reporting this and providing a patch Matthew. I agree with you, until the real issue MDL-27336 it would be better to display an error message. Pushing your issue for integration now.

            If you could provide a patch for each different branch (2.4, 2.5 and master) that would be awesome. Otherwise I assume this would easily be cherry-picked.

            In the future, you could also add some testing instructions to your issue. Instructions that we can follow to confirm that your patch is working as expected.

            Many thanks!
            Fred

            Show
            fred Frédéric Massart added a comment - Thanks for reporting this and providing a patch Matthew. I agree with you, until the real issue MDL-27336 it would be better to display an error message. Pushing your issue for integration now. If you could provide a patch for each different branch (2.4, 2.5 and master) that would be awesome. Otherwise I assume this would easily be cherry-picked. In the future, you could also add some testing instructions to your issue. Instructions that we can follow to confirm that your patch is working as expected. Many thanks! Fred
            Hide
            mnoorenberghe Matthew N added a comment -

            Thanks Fred, I'm glad you saw the issue because I wasn't able to change the status to request peer review. How should I request peer review in the future? This wasn't covered in the development process wiki (except for people who have submitted many patches).

            The commit can indeed be cherry-picked to 2.3 as that's there I first wrote the patch. I can do that in the next few days if you want. I figured I would start with master until I knew others agreed.

            I updated the testing instructions to make it clear not to use an HTML file containing images.

            Show
            mnoorenberghe Matthew N added a comment - Thanks Fred, I'm glad you saw the issue because I wasn't able to change the status to request peer review. How should I request peer review in the future? This wasn't covered in the development process wiki (except for people who have submitted many patches). The commit can indeed be cherry-picked to 2.3 as that's there I first wrote the patch. I can do that in the next few days if you want. I figured I would start with master until I knew others agreed. I updated the testing instructions to make it clear not to use an HTML file containing images.
            Hide
            fred Frédéric Massart added a comment -

            Hi Matthew, in order to submit for peer review, and/or to be the assignee you need to have the permissions for it. You are generally given the permissions once you've provided a couple of patches. Michael de Raadt is usually the person giving those access levels.

            Unfortunately, I don't think this fix will be backported to 2.3 because our policy is only to maintain the last 2 stables releases, except for security patches.

            Thanks for updating the testing instructions.

            Fred

            Show
            fred Frédéric Massart added a comment - Hi Matthew, in order to submit for peer review, and/or to be the assignee you need to have the permissions for it. You are generally given the permissions once you've provided a couple of patches. Michael de Raadt is usually the person giving those access levels. Unfortunately, I don't think this fix will be backported to 2.3 because our policy is only to maintain the last 2 stables releases, except for security patches. Thanks for updating the testing instructions. Fred
            Hide
            samhemelryk Sam Hemelryk added a comment -

            Thanks guys, this has been integrated now.

            Master, 25, and 24.

            Show
            samhemelryk Sam Hemelryk added a comment - Thanks guys, this has been integrated now. Master, 25, and 24.
            Hide
            andyjdavis Andrew Davis added a comment -

            Works as described. Passing.

            Show
            andyjdavis Andrew Davis added a comment - Works as described. Passing.
            Hide
            damyon Damyon Wiese added a comment -

            This issue along with 77 of it's friends has been sent upstream and released to the world.

            Thankyou for your contribution.

            Show
            damyon Damyon Wiese added a comment - This issue along with 77 of it's friends has been sent upstream and released to the world. Thankyou for your contribution.
            Hide
            mnoorenberghe Matthew N added a comment -

            Would it be possible to be made the assignee of the issue so I can track which bugs I have fixed? Thanks

            Show
            mnoorenberghe Matthew N added a comment - Would it be possible to be made the assignee of the issue so I can track which bugs I have fixed? Thanks
            Hide
            fred Frédéric Massart added a comment -

            Unfortunately you do not have the right access to be assignee (just yet), Michael de Raadt can you do something about this?

            Show
            fred Frédéric Massart added a comment - Unfortunately you do not have the right access to be assignee (just yet), Michael de Raadt can you do something about this?
            Hide
            salvetore Michael de Raadt added a comment -

            For now I'll leave this in Fred's hands. Thanks for your involvement, Matthew.

            Show
            salvetore Michael de Raadt added a comment - For now I'll leave this in Fred's hands. Thanks for your involvement, Matthew.

              People

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

                Dates

                • Created:
                  Updated:
                  Resolved:
                  Fix Release Date:
                  11/Nov/13