Uploaded image for project: '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
    • Status: Closed
    • Priority: 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

          Attachments

            Issue Links

              Activity

              Hide
              abgreeve 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
              abgreeve 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_frenz 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_frenz 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_frenz 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_frenz 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
              poltawski 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
              poltawski 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_frenz Ankit Agarwal added a comment -

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

              Show
              ankit_frenz Ankit Agarwal added a comment - Thanks for the feedback Dan. That is something new that I learned today Anyways patch updated. Resubmitting
              Hide
              poltawski 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
              poltawski Dan Poltawski added a comment - Urm, In what way is $sub->eventtype translatable? Perhaps the 'redirect' message could be a bit friendlier?
              Hide
              ankit_frenz 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_frenz 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
              poltawski 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
              poltawski 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
              poltawski 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
              poltawski 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
              markn Mark Nelson added a comment -

              Works as expected. Passing.

              Show
              markn Mark Nelson added a comment - Works as expected. Passing.
              Hide
              stronk7 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
              stronk7 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
              nebgor 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
              nebgor 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
              Hide
              marina Marina Glancy added a comment -

              This caused a regression, see MDL-50637

              Show
              marina Marina Glancy added a comment - This caused a regression, see MDL-50637

                People

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

                  Dates

                  • Created:
                    Updated:
                    Resolved:
                    Fix Release Date:
                    14/Jan/13