Moodle
  1. Moodle
  2. MDL-37432

Can add an event to a calendar w/o providing URL or file.

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: 2.4, 2.5
    • Fix Version/s: 2.4.2
    • Component/s: Calendar
    • Labels:
    • Testing Instructions:
      Hide
      1. Log in as an administrator.
      2. Click on the date on the Calendar block.
      3. Click on 'Manage Subscriptions'.
      4. Fill in a name for the Calendar
      5. Select "Import from" = "Calendar URL" and leave Calendar url blank.
      6. You should see "Invalid url" error and no new subscription entry should be added.
      7. Enter invalid url and you should see error click continue and you should go back to manage subscription page.
      8. Enter valid calendar url (https://www.google.com/calendar/ical/en.australian%23holiday%40group.v.calendar.google.com/public/basic.ics) and you should see new subscription entry.
      9. Now try again with "Import from" = "Calendar FILE"
      10. Don't attach file and you should see "Either a URL or a file is required to import a calendar." error and no new subscription should be added.
      11. Attach a file and you should see new subscription.
      Show
      Log in as an administrator. Click on the date on the Calendar block. Click on 'Manage Subscriptions'. Fill in a name for the Calendar Select "Import from" = "Calendar URL" and leave Calendar url blank. You should see "Invalid url" error and no new subscription entry should be added. Enter invalid url and you should see error click continue and you should go back to manage subscription page. Enter valid calendar url ( https://www.google.com/calendar/ical/en.australian%23holiday%40group.v.calendar.google.com/public/basic.ics ) and you should see new subscription entry. Now try again with "Import from" = "Calendar FILE" Don't attach file and you should see "Either a URL or a file is required to import a calendar." error and no new subscription should be added. Attach a file and you should see new subscription.
    • Affected Branches:
      MOODLE_24_STABLE, MOODLE_25_STABLE
    • Fixed Branches:
      MOODLE_24_STABLE
    • Pull 2.4 Branch:
      wip-mdl-37432-m24
    • Pull Master Branch:
      wip-mdl-37432
    • Rank:
      47065

      Description

      1. Log in as an administrator.
      2. Click on the date on the Calendar block.
      3. Click on 'Manage Subscriptions'.
      4. Fill in a name for the Calendar w/o a URL or file being supplied.
      5. Click add to see the message 'File subscription not updated'.
      6. Once redirected you will see it listed.
      7. Blame Ankit Mark.

        Issue Links

          Activity

          Hide
          Michael de Raadt added a comment -

          Some validation is lacking. Well spotted.

          Show
          Michael de Raadt added a comment - Some validation is lacking. Well spotted.
          Hide
          Rajesh Taneja added a comment -

          Currently we add subscription in table and later if link is not valid (private or not accessible), we throw error.
          Doing so we leave invalid subscription. Should we remove it or give user chance to fix access on provided url?

          Show
          Rajesh Taneja added a comment - Currently we add subscription in table and later if link is not valid (private or not accessible), we throw error. Doing so we leave invalid subscription. Should we remove it or give user chance to fix access on provided url?
          Hide
          Ankit Agarwal added a comment - - edited

          Hi Rajesh,
          I donot agree with the changes you made in the second commit. Because:-

          1. we should not be adding subscriptions if the url is not valid in the first place rather than trying to fix it later. I would suggest moving the add_subscription() api lower down the data flow.
          2. The other problem with current solution is that, in extereme cases it might lead to orphan events which is not acceptable at any cost. Also we should use the api to delete the subscription, if we really want to delete it, instead of deleting it directly from DB. you can use calendar_process_subscription_row() for that or create a new api.
          3. Can't the imported files invalid as well?
          4. Also please include this test case in your testing instructions as well.
          5. As far as existing invalid subscriptions goes, I don't think we can do much about it. If you feel strongly about it we can remove it, but it shouldn't happen without a user's confirmation imho.

          Rest of the patch looks great.
          Thanks

          Show
          Ankit Agarwal added a comment - - edited Hi Rajesh, I donot agree with the changes you made in the second commit. Because:- we should not be adding subscriptions if the url is not valid in the first place rather than trying to fix it later. I would suggest moving the add_subscription() api lower down the data flow. The other problem with current solution is that, in extereme cases it might lead to orphan events which is not acceptable at any cost. Also we should use the api to delete the subscription, if we really want to delete it, instead of deleting it directly from DB. you can use calendar_process_subscription_row() for that or create a new api. Can't the imported files invalid as well? Also please include this test case in your testing instructions as well. As far as existing invalid subscriptions goes, I don't think we can do much about it. If you feel strongly about it we can remove it, but it shouldn't happen without a user's confirmation imho. Rest of the patch looks great. Thanks
          Hide
          Rajesh Taneja added a comment -

          Hello Ankit,

          You might want to revisit the code:
          For point 1: calendar_update_subscription_events and calendar_import_icalendar_events, both need subscriptionid, so adding a new subscription later means rework on API
          2: There won't be orphan subscripts as url is not accessible and hence no event is added. I don't think it's valid api to delete event. It might be nice to add one, but leaving it for Sam to decide.
          3: No we don't import any event from invalid file. If it's not valid xml no event is added.
          4: It is already there (test instruction: point 7)
          5: Well, that's why I added it as second commit to get opinion. I think we should not have invalid subscriptions. Although will wait for Sam to comment.

          Show
          Rajesh Taneja added a comment - Hello Ankit, You might want to revisit the code: For point 1: calendar_update_subscription_events and calendar_import_icalendar_events, both need subscriptionid, so adding a new subscription later means rework on API 2: There won't be orphan subscripts as url is not accessible and hence no event is added. I don't think it's valid api to delete event. It might be nice to add one, but leaving it for Sam to decide. 3: No we don't import any event from invalid file. If it's not valid xml no event is added. 4: It is already there (test instruction: point 7) 5: Well, that's why I added it as second commit to get opinion. I think we should not have invalid subscriptions. Although will wait for Sam to comment.
          Hide
          Rajesh Taneja added a comment -

          Hello Ankit, as discussed, I am pushing it back for peer-review.

          FYI:

          1. We discussed that failure in fetching url for new subscription should delete subscription and records (if any).
          2. If url can't be fetched for existing subscription then don't delete anything but user should return to the manage subscription page.
          Show
          Rajesh Taneja added a comment - Hello Ankit, as discussed, I am pushing it back for peer-review. FYI: We discussed that failure in fetching url for new subscription should delete subscription and records (if any). If url can't be fetched for existing subscription then don't delete anything but user should return to the manage subscription page.
          Hide
          Rajesh Taneja added a comment -

          FYI: new API has been introduced and back ported to 24 because this code area is new and not expected to break anything.

          Show
          Rajesh Taneja added a comment - FYI: new API has been introduced and back ported to 24 because this code area is new and not expected to break anything.
          Hide
          Ankit Agarwal added a comment - - edited

          Sorry for the delay in reviewing.
          Patch looks good. A few minor things:-

          1. You missed declaring $DB as global in your new function.
          2. I would throw the same exception back, by just setting the $e->link, so we donot lose any extra info if present. Not sure which is the preferred way though. If you feel current approach is a better alternative, feel free to ignore this comment.

          Rest looks great. Feel free to submit for integration when ready.
          Thanks

          Show
          Ankit Agarwal added a comment - - edited Sorry for the delay in reviewing. Patch looks good. A few minor things:- You missed declaring $DB as global in your new function. I would throw the same exception back, by just setting the $e->link, so we donot lose any extra info if present. Not sure which is the preferred way though. If you feel current approach is a better alternative, feel free to ignore this comment. Rest looks great. Feel free to submit for integration when ready. Thanks
          Hide
          Rajesh Taneja added a comment -

          Thanks Ankit,

          I think new error make it more readable.
          I have fixed global, pushing it for integration.

          Show
          Rajesh Taneja added a comment - Thanks Ankit, I think new error make it more readable. I have fixed global, pushing it for integration.
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Integrated (24 & master), thanks!

          Show
          Eloy Lafuente (stronk7) added a comment - Integrated (24 & master), thanks!
          Hide
          Rossiani Wijaya added a comment -

          This is working as expected.

          Tested for 2.4 and Master

          Test passed.

          Show
          Rossiani Wijaya added a comment - This is working as expected. Tested for 2.4 and Master Test passed.
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Surely you will be happy to know that your code is now part of Moodle upstream. Thanks, thanks!

          Closing as fixed, ciao

          Show
          Eloy Lafuente (stronk7) added a comment - Surely you will be happy to know that your code is now part of Moodle upstream. Thanks, thanks! Closing as fixed, ciao

            People

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

              Dates

              • Created:
                Updated:
                Resolved: