Moodle
  1. Moodle
  2. MDL-41634

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

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Minor 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:
    • Rank:
      52748

      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.

        Issue Links

          Activity

          Hide
          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
          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
          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
          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
          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
          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
          Sam Hemelryk added a comment -

          Thanks guys, this has been integrated now.

          Master, 25, and 24.

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

          Works as described. Passing.

          Show
          Andrew Davis added a comment - Works as described. Passing.
          Hide
          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 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
          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
          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
          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
          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
          Michael de Raadt added a comment -

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

          Show
          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: