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

      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.

        Gliffy Diagrams

          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: