Uploaded image for project: 'Moodle'
  1. Moodle
  2. MDL-41485

Do not use string as value for action buttons in calendar manage subscriptions page

          Details

          • Type: Bug
          • Status: Closed
          • Priority: Minor
          • Resolution: Fixed
          • Affects Version/s: 2.4.5, 2.5.1
          • Fix Version/s: 2.4.6, 2.5.2
          • Component/s: Calendar
          • Labels:
          • Testing Instructions:
            Hide
            1. Login as admin.
            2. click on the month in a calendar block.
            3. on the resulting page click on "manage subscriptions"
            4. Add a calendar, you can use this url http://ical.mac.com/ical/US32Holidays.ics
            5. on the resulting page, make sure the update and remove buttons are "<button>" tags and not "<input>"
            6. After the import is done, trying changing the "update" frequency and click on update. Make sure the update works.
            7. Click on "remove". Make sure it works.
            8. Repeat all above steps in two other languages besides English including one RTL like Hebrew.
            Show
            Login as admin. click on the month in a calendar block. on the resulting page click on "manage subscriptions" Add a calendar, you can use this url http://ical.mac.com/ical/US32Holidays.ics on the resulting page, make sure the update and remove buttons are "<button>" tags and not "<input>" After the import is done, trying changing the "update" frequency and click on update. Make sure the update works. Click on "remove". Make sure it works. Repeat all above steps in two other languages besides English including one RTL like Hebrew.
          • Workaround:
            Hide

            Change language back to English to manage subscriptions.

            Show
            Change language back to English to manage subscriptions.
          • Affected Branches:
            MOODLE_24_STABLE, MOODLE_25_STABLE
          • Fixed Branches:
            MOODLE_24_STABLE, MOODLE_25_STABLE
          • Pull from Repository:
          • Pull Master Branch:
            MDL-41485-master
          • Pull Master Diff URL:
          • Sprint:
            BACKEND Sprint 4
          • Sprint:
            BACKEND Sprint 4

            Description

            Donot use string as value for action buttons in calendar manage subscritions page

              Gliffy Diagrams

                Attachments

                  Issue Links

                  is duplicated by

                  Bug - A problem which impairs or prevents Moodle from functioning correctly. MDL-41468 Unable to remove a calendar from "Manage Calendar subscriptions" page, When in RTL mode

                  • Minor - Minor loss of function where workaround is possible
                  • Closed
                  will help resolve

                  Bug - A problem which impairs or prevents Moodle from functioning correctly. MDL-41468 Unable to remove a calendar from "Manage Calendar subscriptions" page, When in RTL mode

                  • Minor - Minor loss of function where workaround is possible
                  • Closed

                    Activity

                    Ascending order - Click to sort in descending order
                    Hide
                    Permalink
                    ankit_frenz Ankit Agarwal added a comment -

                    Using strings as values creates a lot of issues like MDL-41468. This happens all over moodle.

                    The best way to solve this imo is to use <button />. I have not seen any usages of <button> so far in Moodle. So adding a few people here from Frontend, to get more feedback on accessibility related aspects.

                    If we decide to go forward with this, I will create a issue to add a class for this in the forms lib.

                    Thanks

                    Show
                    ankit_frenz Ankit Agarwal added a comment - Using strings as values creates a lot of issues like MDL-41468 . This happens all over moodle. The best way to solve this imo is to use <button />. I have not seen any usages of <button> so far in Moodle. So adding a few people here from Frontend, to get more feedback on accessibility related aspects. If we decide to go forward with this, I will create a issue to add a class for this in the forms lib. Thanks
                    Hide
                    Permalink
                    ankit_frenz Ankit Agarwal added a comment -

                    I am pushing this forward for peer review.
                    Thanks

                    Show
                    ankit_frenz Ankit Agarwal added a comment - I am pushing this forward for peer review. Thanks
                    Hide
                    Permalink
                    ankit_frenz Ankit Agarwal added a comment - - edited

                    Ps:- It does break BC, but I don't think anyone outside core would be using calendar_process_subscription_row(). Hence it doesn't need to clutter up upgrade.txt
                    I will backport it after the review.
                    Thanks

                    Show
                    ankit_frenz Ankit Agarwal added a comment - - edited Ps:- It does break BC, but I don't think anyone outside core would be using calendar_process_subscription_row(). Hence it doesn't need to clutter up upgrade.txt I will backport it after the review. Thanks
                    Hide
                    Permalink
                    markn Mark Nelson added a comment -

                    Hi Ankit,

                    I totally agree that we definitely should not be passing language strings as the value for variables, since it can vary from language to language and also makes filtering not as reliable. So, what you have done so far looks great. I don't think it will really break older Moodle versions since, like you said, no one is going to be calling this function directly. I did a grep in a directory I have on my HD that contains the third party modules (a little outdated though) just to verify.

                    I would suggest someone from FRONTEND verify that the replacement of the input tag with a button tag is acceptable/accessible, and if so, this can be pushed for integration. I understand that the input field needs to be changed to a button in order for you to pass the integer as the value and still keep the translated name, so I would say it is OK.

                    Regards,

                    Mark

                    Show
                    markn Mark Nelson added a comment - Hi Ankit, I totally agree that we definitely should not be passing language strings as the value for variables, since it can vary from language to language and also makes filtering not as reliable. So, what you have done so far looks great. I don't think it will really break older Moodle versions since, like you said, no one is going to be calling this function directly. I did a grep in a directory I have on my HD that contains the third party modules (a little outdated though) just to verify. I would suggest someone from FRONTEND verify that the replacement of the input tag with a button tag is acceptable/accessible, and if so, this can be pushed for integration. I understand that the input field needs to be changed to a button in order for you to pass the integer as the value and still keep the translated name, so I would say it is OK. Regards, Mark
                    Hide
                    Permalink
                    ankit_frenz Ankit Agarwal added a comment -

                    Thanks Mark,
                    For the review. I asked, Jerome's opinion on this as well. He agrees with the change. Pushing forward for integration.

                    Thanks

                    Show
                    ankit_frenz Ankit Agarwal added a comment - Thanks Mark, For the review. I asked, Jerome's opinion on this as well. He agrees with the change. Pushing forward for integration. Thanks
                    Hide
                    Permalink
                    ankit_frenz Ankit Agarwal added a comment -

                    @integrators
                    Please note that only one of MDL-41485 and MDL-41468 should go in. The other should be closed.
                    Thanks

                    Show
                    ankit_frenz Ankit Agarwal added a comment - @integrators Please note that only one of MDL-41485 and MDL-41468 should go in. The other should be closed. Thanks
                    Hide
                    Permalink
                    damyon Damyon Wiese added a comment -

                    Thanks Ankit,

                    I've integrated and tested this on 24, 25 and master.

                    Show
                    damyon Damyon Wiese added a comment - Thanks Ankit, I've integrated and tested this on 24, 25 and master.
                    Hide
                    Permalink
                    damyon Damyon Wiese added a comment -

                    Tested on all branches in integration. Passing test.

                    Show
                    damyon Damyon Wiese added a comment - Tested on all branches in integration. Passing test.
                    Hide
                    Permalink
                    poltawski Dan Poltawski added a comment -

                    Congratulations! This change has been integrated upstream and is now available from our git and download mirrors. To celebrate, here is a joke:

                    A SQL query goes into a bar, walks up to two tables and asks, "Can I join you?"

                    Show
                    poltawski Dan Poltawski added a comment - Congratulations! This change has been integrated upstream and is now available from our git and download mirrors. To celebrate, here is a joke: A SQL query goes into a bar, walks up to two tables and asks, "Can I join you?"

                      People

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

                        Dates

                        • Created:
                          Updated:
                          Resolved:
                          Fix Release Date:
                          9/Sep/13