Moodle
  1. Moodle
  2. MDL-36626 Meta: Ical Bug fixes and improvments
  3. MDL-36738

Course calendar - adding site wide calendar has no 'has been saved' feedback

    Details

    • Type: Sub-task Sub-task
    • Status: Closed
    • Priority: Minor Minor
    • Resolution: Fixed
    • Affects Version/s: 2.4
    • Fix Version/s: 2.4.1
    • Component/s: Calendar
    • Labels:
    • Testing Instructions:
      Hide
      1. Log into Moodle as an administrator.
      2. Click on the date on the calendar block.
      3. Add a calendar using the URL "https://www.google.com/calendar/ical/en.australian%23holiday%40group.v.calendar.google.com/public/basic.ics".
      4. When you click 'add' ensure you see a message telling you the number of events imported and updated before being redirected.
      5. On the next page make sure you see a column that shows the type of event for the calendar.
      Show
      Log into Moodle as an administrator. Click on the date on the calendar block. Add a calendar using the URL "https://www.google.com/calendar/ical/en.australian%23holiday%40group.v.calendar.google.com/public/basic.ics". When you click 'add' ensure you see a message telling you the number of events imported and updated before being redirected. On the next page make sure you see a column that shows the type of event for the calendar.
    • Affected Branches:
      MOODLE_24_STABLE
    • Fixed Branches:
      MOODLE_24_STABLE
    • Pull Master Branch:
      MDL-36738-master
    • Rank:
      46244

      Description

      While testing MDL-36614 i noticed:
      from course calendar (from within a course, then going to manage subscriptions of the course calendar), adding a sitewide event is a little confusing as there is nothing to say that it is saved and neither is it represented (understandably) in the course calendar management view.
      i think it would help to display the 'Type of Event' in a column in the manage subscriptions table.

        Issue Links

          Activity

          Hide
          Adrian Greeve added a comment -

          [Y] Syntax
          [Y] Output
          [Y] Whitespace
          [Y] Language
          [-] Databases
          [Y] Testing
          [-] Security
          [-] Documentation
          [Y] Git
          [Y] Sanity check

          Hi Ankit,

          I like the addition of the type of event in the table.

          The removal of the redirect does allow for the display of feedback. I noticed now that if you refresh the page it will re-do the actions that you just did. So if you added a subscription it will re-do that. If you delete a subscription it will try to do that and produce an error.
          I think this is what Sam was originally trying to avoid when he wrote the redirect.

          Show
          Adrian Greeve added a comment - [Y] Syntax [Y] Output [Y] Whitespace [Y] Language [-] Databases [Y] Testing [-] Security [-] Documentation [Y] Git [Y] Sanity check Hi Ankit, I like the addition of the type of event in the table. The removal of the redirect does allow for the display of feedback. I noticed now that if you refresh the page it will re-do the actions that you just did. So if you added a subscription it will re-do that. If you delete a subscription it will try to do that and produce an error. I think this is what Sam was originally trying to avoid when he wrote the redirect.
          Hide
          Ankit Agarwal added a comment -

          Hi Adrian,
          Thanks for the review.
          Yes that is exactly what Sam was talking about . As I mentioned in my commit as well, unfortunately I donot see anyway around it, unless trying to pass the string in the url or something like that which would be crazy. or create an intermediate page or something.
          I have made it as a separate commit incase the integrator decides it is more important to stop redirect than displaying the feedback.

          Submitting for integration.
          Thanks

          Show
          Ankit Agarwal added a comment - Hi Adrian, Thanks for the review. Yes that is exactly what Sam was talking about . As I mentioned in my commit as well, unfortunately I donot see anyway around it, unless trying to pass the string in the url or something like that which would be crazy. or create an intermediate page or something. I have made it as a separate commit incase the integrator decides it is more important to stop redirect than displaying the feedback. Submitting for integration. Thanks
          Hide
          Ankit Agarwal added a comment -

          @integrator
          Although this is a new feature, but it might be nice to have in 24 as well. It should be a clean cherry-pick incase you decide to backport it.
          Thanks

          Show
          Ankit Agarwal added a comment - @integrator Although this is a new feature, but it might be nice to have in 24 as well. It should be a clean cherry-pick incase you decide to backport it. Thanks
          Hide
          Dan Poltawski added a comment -

          Hi Ankit,

          unless trying to pass the string in the url or something like that which would be crazy. or create an intermediate page or something.

          Actually, we have just the ticket for this and its used quite widely.

          e.g. When you add a forum post, you get redirected to a page which says 'Your post has been added, thanks'.

          The key is the $message parameter to the redirect function which allows you to create this crazy idea!

          Show
          Dan Poltawski added a comment - Hi Ankit, unless trying to pass the string in the url or something like that which would be crazy. or create an intermediate page or something. Actually, we have just the ticket for this and its used quite widely. e.g. When you add a forum post, you get redirected to a page which says 'Your post has been added, thanks'. The key is the $message parameter to the redirect function which allows you to create this crazy idea!
          Hide
          Ankit Agarwal added a comment -

          Thanks for the feedback Dan.
          That is something new that I learned today
          Anyways patch updated.
          Resubmitting

          Show
          Ankit Agarwal added a comment - Thanks for the feedback Dan. That is something new that I learned today Anyways patch updated. Resubmitting
          Hide
          Dan Poltawski added a comment -

          Urm,

          1. In what way is $sub->eventtype translatable?
          2. Perhaps the 'redirect' message could be a bit friendlier?
          Show
          Dan Poltawski added a comment - Urm, In what way is $sub->eventtype translatable? Perhaps the 'redirect' message could be a bit friendlier?
          Hide
          Ankit Agarwal added a comment -

          I discussed this with Dan, and the redirect message is better left alone. As it is generated by apis, and is consistent across multiple apis.
          I have made the type translatable.
          Thanks

          Show
          Ankit Agarwal added a comment - I discussed this with Dan, and the redirect message is better left alone. As it is generated by apis, and is consistent across multiple apis. I have made the type translatable. Thanks
          Hide
          Dan Poltawski added a comment -

          I think the summary of our conversation would be better as 'lets not open that can of worms in this issue'

          Show
          Dan Poltawski added a comment - I think the summary of our conversation would be better as 'lets not open that can of worms in this issue'
          Hide
          Dan Poltawski added a comment -

          I suppose you wanted that cherry picked too... so i've done that! Integrated to master and 2.4

          Show
          Dan Poltawski added a comment - I suppose you wanted that cherry picked too... so i've done that! Integrated to master and 2.4
          Hide
          Mark Nelson added a comment -

          Works as expected. Passing.

          Show
          Mark Nelson added a comment - Works as expected. Passing.
          Hide
          Eloy Lafuente (stronk7) added a comment -

          And your fantastic code has met core, hope they become good friends for a long period.

          Closing, thanks!

          Show
          Eloy Lafuente (stronk7) added a comment - And your fantastic code has met core, hope they become good friends for a long period. Closing, thanks!
          Hide
          Aparup Banerjee added a comment -

          lol going back thru old email.. Ankit, "Yes that is exactly what Sam was talking about" ... signs of over-chatting? - thanks for fixing this

          Show
          Aparup Banerjee added a comment - lol going back thru old email.. Ankit, "Yes that is exactly what Sam was talking about" ... signs of over-chatting? - thanks for fixing this

            People

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

              Dates

              • Created:
                Updated:
                Resolved: