Details

    • Type: Sub-task Sub-task
    • Status: Closed
    • Priority: Blocker Blocker
    • Resolution: Fixed
    • Affects Version/s: 2.4
    • Fix Version/s: 2.4
    • Component/s: Calendar
    • Labels:
    • Testing Instructions:
      Hide
      1. Log in to Moodle as a student
      2. Go to Navigation > Site pages > Calendar.
      3. Click the button 'Manage subscriptions'.
      4. Try importing a calendar with the URL webcal://www.mozilla.org/projects/calendar/caldata/AustraliaHolidays.ics , no errors should be generated
      5. Try importing a calendar with the URL webcal://www.mozilla.org/proj{}ects/calendar/caldata/AustraliaHolidays.ics , "invalid url" errors should be generated
      6. Try importing a calendar with the URL http://www.mozi{}lla.org/projects/calendar/caldata/AustraliaHolidays.ics , "invalid url" errors should be generated
      7. Try importing a calendar with the URL http://www.mozilla.org/projects/calendar/caldata/AustraliaHolidays.ics , no errors should be generated
      8. Make sure all urls (the ones with no error) are stored as "http" not "Webcal"
      9. make sure the events are created in the calendar as expected.
      Show
      Log in to Moodle as a student Go to Navigation > Site pages > Calendar. Click the button 'Manage subscriptions'. Try importing a calendar with the URL webcal://www.mozilla.org/projects/calendar/caldata/AustraliaHolidays.ics , no errors should be generated Try importing a calendar with the URL webcal://www.mozilla.org/proj{}ects/calendar/caldata/AustraliaHolidays.ics , "invalid url" errors should be generated Try importing a calendar with the URL http://www.mozi {}lla.org/projects/calendar/caldata/AustraliaHolidays.ics , "invalid url" errors should be generated Try importing a calendar with the URL http://www.mozilla.org/projects/calendar/caldata/AustraliaHolidays.ics , no errors should be generated Make sure all urls (the ones with no error) are stored as "http" not "Webcal" make sure the events are created in the calendar as expected.
    • Affected Branches:
      MOODLE_24_STABLE
    • Fixed Branches:
      MOODLE_24_STABLE
    • Pull Master Branch:
      MDL-36526-master
    • Rank:
      45992

      Description

      If you try to import an external calendar with the following URL...

      webcal://www.mozilla.org/projects/calendar/caldata/AustraliaHolidays.ics

      The form validation regards this as an invalid URL and prompts the user for another.

      Replication steps:

      1. Log in to Moodle as a student
      2. Go to Navigation > Site pages > Calendar.
      3. Click the button 'Manage subscriptions'.
      4. Try importing a calendar with the URL webcal://www.mozilla.org/projects/calendar/caldata/AustraliaHolidays.ics

      Expected result: The file should be used as the basis of a calendar

      Actual result: Validation fails and user is asked to enter another URL

      Note that the calendar import does accept the same URL with the http protocol.

      http://www.mozilla.org/projects/calendar/caldata/AustraliaHolidays.ics

      The problem is that the field for the calendar URL has the type PARAM_URL, which accepts only http, https, mailto and ftp.

      The best solution would be to create some custom validation for this field that checks, if the text does not qualify using PARAM_URL, that it begins with webcal:// and the rest is a valid URL (perhaps substitute webcal for http and test).

        Issue Links

          Activity

          Hide
          Ankit Agarwal added a comment -

          As we decided not to mess around PARAM_URL to allow webcal protocol, I have updated the local manage subscription form in context.
          Requesting a review.
          Thanks

          Show
          Ankit Agarwal added a comment - As we decided not to mess around PARAM_URL to allow webcal protocol, I have updated the local manage subscription form in context. Requesting a review. Thanks
          Hide
          Mark Nelson added a comment -

          Hi Ankit, the logic is good. Two points:

          1) Do you need strlen("webcal://")) on a hard coded string? Why not just enter the length?
          2) You could specify the type of exception that is being thrown (ie. invalid_parameter_exception).

          Regards,

          Mark

          Show
          Mark Nelson added a comment - Hi Ankit, the logic is good. Two points: 1) Do you need strlen("webcal://")) on a hard coded string? Why not just enter the length? 2) You could specify the type of exception that is being thrown (ie. invalid_parameter_exception). Regards, Mark
          Hide
          Ankit Agarwal added a comment -

          Hi Mark,
          Thanks for the review and feedback.

          1) That was intentional from me, since there a hardcoded integer "7" down the line can create confusion. Still if everyone thinks "7" is a better way to go I can update it.
          2) I have updated the patch for that.

          Thanks

          Show
          Ankit Agarwal added a comment - Hi Mark, Thanks for the review and feedback. 1) That was intentional from me, since there a hardcoded integer "7" down the line can create confusion. Still if everyone thinks "7" is a better way to go I can update it. 2) I have updated the patch for that. Thanks
          Hide
          Sam Hemelryk added a comment -

          Hi Ankit,

          The change looks real good thanks.
          There is one minor thing that I think would be better done another way.
          Instead of using validate_param why not use clean_param and checks its output is identical to the input.
          The clean_param and comparison route is the one taken most often in our code and is pretty well understood. It has the advantage of avoiding the costly exception handling.
          Check out the username validation in ./user/editadvanced_form.php for an example if you would like.

          I'll leave this in integration review in progress as the solution works and if you happen not to be in today I will integrate this anyway and reopen the QA test.

          Many thanks
          Sam

          Show
          Sam Hemelryk added a comment - Hi Ankit, The change looks real good thanks. There is one minor thing that I think would be better done another way. Instead of using validate_param why not use clean_param and checks its output is identical to the input. The clean_param and comparison route is the one taken most often in our code and is pretty well understood. It has the advantage of avoiding the costly exception handling. Check out the username validation in ./user/editadvanced_form.php for an example if you would like. I'll leave this in integration review in progress as the solution works and if you happen not to be in today I will integrate this anyway and reopen the QA test. Many thanks Sam
          Hide
          Sam Hemelryk added a comment -

          Oh one more thing as well actually, we should still be specifying a type for the field. PARAM_RAW would be perfect.

          Show
          Sam Hemelryk added a comment - Oh one more thing as well actually, we should still be specifying a type for the field. PARAM_RAW would be perfect.
          Hide
          Ankit Agarwal added a comment -

          Hi Sam,
          Thanks for the feedback.
          I have updated the patch.
          Thanks

          Show
          Ankit Agarwal added a comment - Hi Sam, Thanks for the feedback. I have updated the patch. Thanks
          Hide
          Dan Poltawski added a comment -

          Taking this off Sam, as hes sick today.

          Show
          Dan Poltawski added a comment - Taking this off Sam, as hes sick today.
          Hide
          Dan Poltawski added a comment -

          Integrated, thanks Ankit!

          Show
          Dan Poltawski added a comment - Integrated, thanks Ankit!
          Hide
          Ankit Agarwal added a comment -

          I have replaced the webcall protocol with http:// since curl doesnt recognise webcall://
          If webcalls:// is introduced later on we can decide to change that to https://.
          Thanks

          Show
          Ankit Agarwal added a comment - I have replaced the webcall protocol with http:// since curl doesnt recognise webcall:// If webcalls:// is introduced later on we can decide to change that to https:// . Thanks
          Hide
          Dan Poltawski added a comment -

          Ankit and i discussed this and determined that the change wasn't actually working. 'webcal' isn't really a real protocol, so curl needs to do a http request.

          Show
          Dan Poltawski added a comment - Ankit and i discussed this and determined that the change wasn't actually working. 'webcal' isn't really a real protocol, so curl needs to do a http request.
          Hide
          Dan Poltawski added a comment -

          Hi Ankit,

          I don't think the approach you've taken there with the form is the right way.

          Have a look at:
          blocks/rss_client/editfeed.php

          definition_after_data() to see the approch taken with applyFilter.

          Show
          Dan Poltawski added a comment - Hi Ankit, I don't think the approach you've taken there with the form is the right way. Have a look at: blocks/rss_client/editfeed.php definition_after_data() to see the approch taken with applyFilter.
          Hide
          Ankit Agarwal added a comment -

          Thanks Dan for pointing to the after_data() method.
          I have updated the patch.
          Thanks

          Show
          Ankit Agarwal added a comment - Thanks Dan for pointing to the after_data() method. I have updated the patch. Thanks
          Hide
          Dan Poltawski added a comment -

          As discussed, the new patch is using undefined variables and will not be working. Please update.

          Show
          Dan Poltawski added a comment - As discussed, the new patch is using undefined variables and will not be working. Please update.
          Hide
          Dan Poltawski added a comment -

          Well, Ankit and I went through a saga with this, but its fixed.

          Show
          Dan Poltawski added a comment - Well, Ankit and I went through a saga with this, but its fixed.
          Hide
          Rajesh Taneja added a comment -

          Thanks Ankit, it works as described.
          Although I have a question before passing this:
          If there is a space before/after a valid url, it says invalid url, would you like to trim url before processing?

          User tend to copy paste urls and it might be nice to trim url, before processing.

          Show
          Rajesh Taneja added a comment - Thanks Ankit, it works as described. Although I have a question before passing this: If there is a space before/after a valid url, it says invalid url, would you like to trim url before processing? User tend to copy paste urls and it might be nice to trim url, before processing.
          Hide
          Dan Poltawski added a comment -

          Raj, i've pushed a patch which does that (I ended up doing the last patch in this area).

          Show
          Dan Poltawski added a comment - Raj, i've pushed a patch which does that (I ended up doing the last patch in this area).
          Hide
          Rajesh Taneja added a comment -

          Thanks Dan and Ankit,

          Works Great.

          Show
          Rajesh Taneja added a comment - Thanks Dan and Ankit, Works Great.
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Many, many thanks for your effort!

          Millions of people will enjoy the results of your work, yay!

          Closing as fixed. Ciao

          Show
          Eloy Lafuente (stronk7) added a comment - Many, many thanks for your effort! Millions of people will enjoy the results of your work, yay! Closing as fixed. Ciao

            People

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

              Dates

              • Created:
                Updated:
                Resolved: